-
Notifications
You must be signed in to change notification settings - Fork 206
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
refactor!(zoe): handle subtracting empty in a less fragile way #3345
Conversation
823347b
to
648b352
Compare
@michaelfig, added you as a backup reviewer in case @erights is too busy and we are having trouble getting it in by Monday. |
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!
amountToSubtract === undefined || | ||
(amountToSubtract !== undefined && AmountMath.isEmpty(amountToSubtract)) |
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.
Is this just equivalent to:
amountToSubtract === undefined || | |
(amountToSubtract !== undefined && AmountMath.isEmpty(amountToSubtract)) | |
amountToSubtract === undefined || AmountMath.isEmpty(amountToSubtract) |
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.
Yes, it is. I'll simplify :)
648b352
to
47fd185
Compare
Closes #3335
#3184 required contracts to ensure that a keyword is specified before subtracting from the amount at that keyword. For instance:
agoric-sdk/packages/zoe/src/contracts/newSwap/collectFees.js
Lines 6 to 13 in eba3074
On second thought, this is way too fragile, and too much to require of contract developers.
This PR changes Zoe such that subtracting an empty amount, even from a keyword that does not exist, will not error. This will resolve the bug in #3335 in a way that handles the edge case for developers rather than requiring them to anticipate it.