-
Notifications
You must be signed in to change notification settings - Fork 94
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
add CTID field in the tx command request #672
Conversation
xrpl/models/requests/tx.py
Outdated
present_items = [ | ||
item for item in [self.transaction, self.ctid] if item is not None | ||
] | ||
return len(present_items) == 1 |
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.
Instead of list comprehension here, you can use filter
to remove items which don't pass a test. If you're just trying to remove falsey values like None
bool
is a good enough function. (https://www.geeksforgeeks.org/remove-falsy-values-from-a-list-in-python/)
Explanation of truthy / falsey values in python: truthy
And last note, for readability I'd just recommend pulling the array [self.transaction, self.ctid]
into a separate variable as there's already a lot going on in the one-liner, and that'll allow you to label why those two variables are being grouped. Ex. unique_ids = [self.transaction, self.ctid]
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.
okay, fixed in ce890c0
can I merge this PR? I have addressed the residual comments |
High Level Overview of Change
This PR incorporates the CTID update (XLS-37d) in the Python wrapper library.
Context of Change
Based on a cursory read of the CTID PR (https://github.com/XRPLF/rippled/pull/4418/files), I believe the
tx
request accepts a new parameter titled "ctid". This has been incorporated into the Python Request wrapper class.I couldn't identify any other commands that might have been impacted by the CTID feature.
Type of Change
Did you update CHANGELOG.md?
Test Plan
This PR adds a unit test file for the
tx
request. Three combinations of thectid
andtransaction
inputs have been verified.