-
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
fix: assorted fixes #570
fix: assorted fixes #570
Conversation
elif self.signer_quorum <= 0: | ||
errors[ | ||
"signer_quorum" | ||
] = "`signer_quorum` must be greater than or equal to 0." |
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 error says that signer_quorum
must be greater than or equal to 0, but if it is 0 this error still happens. Is that a typo?
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.
Hm, not sure - it appears to have been the same previously.
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.
Based on definition here:
https://xrpl.org/signerlistset.html#signerlistset-fields
It seems 0 is still valid to delete the signer list so the if condition should be modified?
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.
Looking at the code again, I think this condition is fine - line 97 returns errors
if signer_entries
is None
, which should handle that situation.
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.
You're right. Then should the error message be "signer_quorum
must be greater than 0 when signer_list
is not being deleted." since deletion cases have been handled in lines 86-98?
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.
@mvadari I think updating the error to what @pdp2121 suggested makes sense here -
Relevant quote from xrpl.org on SignerlistSet fields: (https://xrpl.org/signerlistset.html#signerlistset-fields)
"You cannot create a signer list such that the SignerQuorum could never be met. The SignerQuorum must be greater than 0 but less than or equal to the sum of the SignerWeight values in the list. Otherwise, the transaction fails with the error temMALFORMED."
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.
Error message was fixed in this commit: e3b0f0eb9f262ed3ae3cdcbc5f6c59fc47432f3c
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.
LGTM after updating the error message (logic looks good as is, just think we can be a bit clearer)
High Level Overview of Change
This PR:
keypairs.sign
to take a hex string in addition to bytesSignerListSet
validationContext of Change
These were all issues I came across while working on #571.
Type of Change
Test Plan
CI passes.