-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove deprecated accounts
and type
parameters from ledger request model
#724
Conversation
@@ -19,10 +17,8 @@ class Ledger(Request, LookupByLedgerRequest): | |||
""" | |||
|
|||
method: RequestMethod = field(default=RequestMethod.LEDGER, init=False) | |||
accounts: bool = False | |||
transactions: bool = False | |||
expand: bool = False | |||
owner_funds: bool = False | |||
binary: bool = False | |||
queue: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me.
But, the current model does not represent ledger_hash
or ledger_index
fields in the Ledger
request. I think we can include these changes in this PR (since there are no methods in this Ledger
class, the complexity is less)
Alternatively, I can note this down and work on it later too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would like to we keep the scope of this PR to removing deprecated fields. If we can make those changes in a separate PR that would be greatly appreciated 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, the current model does not represent
ledger_hash
orledger_index
fields in theLedger
request.
It's included via inheritance - see LookupByLedgerRequest
.
High Level Overview of Change
Remove deprecated
accounts
andtype
parameters from ledger request modelContext of Change
In the ledger API, the accounts and type request parameters has been deprecated for quite awhile now. As of Clio 2.2.0 with this particular commit XRPLF/clio#1360, this API will return a request error if you provide that parameter. This change removes this parameter from the ledger request model. This change would not impact lib users as this setting this parameter previously would not have been used by rippled or Clio
Type of Change
Did you update CHANGELOG.md?
Test Plan
Existing integration tests should continue to pass and manual testing of ledger API with xrpl-py against Clio 2.2.0 should succeed