chore(ERTP): additional input validation and clean up #3892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
katelynsills
force-pushed
the
3885-ertp-input-validation
branch
3 times, most recently
from
4021a0b to
faed256
Compare
katelynsills
force-pushed
the
3885-ertp-input-validation
branch
4 times, most recently
from
09e64a4 to
364afca
Compare
katelynsills
force-pushed
the
3885-ertp-input-validation
branch
3 times, most recently
from
30ed68a to
facb255
Compare
This was referenced Oct 26, 2021
erights
reviewed
Nov 2, 2021
There was a problem hiding this comment.
Review so far. Not done yet.
Leaving aside test files, I only have 6 files to go. My plan is to not review the test files, since the point of my review is to catch input validation issues that the tests may have missed. (None so far! Only stylistic suggestions so far.)
If there is anything in the test files you'd like to call my attention to, please do. Or anywhere else for that matter, in case I miss it.
erights
reviewed
Nov 3, 2021
katelynsills
force-pushed
the
3885-ertp-input-validation
branch
from
1570ed3 to
d4401f2
Compare
BREAKING CHANGE: NatValues now only accept bigints, lower-case amountMath is removed, and AmountMath methods always follow the order of: brand, value
katelynsills
force-pushed
the
3885-ertp-input-validation
branch
from
d4401f2 to
5949f61
Compare
dckc
added a commit
to Agoric/dapp-nft-drop
that referenced
this pull request
Dec 15, 2021
This was referenced Jan 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is the result of doing an evaluation of ERTP for missing input validation. I also did some
smalllarge clean up to ensure that the input validation was easy to audit. Namely, NatValues now only accept bigints, lower-caseamountMathis removed, and AmountMath methods always follow the order of:brand,value. I removed thelooksLikeusage to make it clearer when something was actually trying to validate inputs rather than sniff to see what kind of validation to do.@agoric/same-structureis deprecated, so I changed to using@agoric/marshalreallocatefunction tomoveAssetsfor claritycheckForDupestoassertNoDuplicatesto fit the standard of usingassertfor a function that throwsCloses: #3202
Closes: #3867
Closes: #3885
Closes: #3960
Closes: #3089
Closes: #3045
#documentation-branch: 3885-ertp-input-validation
#dapp-card-store-branch: 3885-ertp-input-validation
#dapp-oracle-branch: 3885-ertp-input-validation
#dapp-otc-branch: 3885-ertp-input-validation