Fix MarkerTransferAuthorization validation. #1856
Merged
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
…valid Coins and the addresses in the allow list are valid.
SpicyLemon
changed the title
WalkthroughThe updates focus on enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/marker/types/authz.go (1 hunks)
- x/marker/types/authz_test.go (2 hunks)
Additional comments: 4
x/marker/types/authz.go (2)
- 63-67: The addition of
TransferLimit.Validate()and the zero-value check enhances the robustness of theMarkerTransferAuthorizationvalidation by ensuring that theTransferLimitis not only valid but also non-zero. This is a critical improvement for preventing potential issues related to invalid or zeroTransferLimitvalues.- 71-78: The updated validation logic for the
AllowListsignificantly improves the security and integrity of theMarkerTransferAuthorization. By ensuring all addresses in theAllowListare valid bech32 addresses and checking for duplicates, the changes mitigate the risk of including invalid or duplicate addresses. The use of detailed error messages for each scenario is a good practice, as it aids in debugging and issue resolution.x/marker/types/authz_test.go (2)
- 10-15: The addition of imports for
sdkmathandassertionsis necessary for the updated test cases. Thesdkmathimport is used for creatingCoininstances with specific amounts, andassertionsis utilized for asserting error messages in test cases. These additions support the new test scenarios introduced by the changes in validation logic.- 62-139: The updated and new test cases in
TestMarkerTransferAuthorizationValidateBasiceffectively cover a wide range of scenarios for bothTransferLimitandAllowListvalidations. This includes tests for nil and emptyTransferLimit, invalid denominations, zero and negative coin amounts, unsortedTransferLimit, and variousAllowListscenarios such as invalid entries and duplicates. The use of detailed error messages in assertions is commendable, as it ensures that the validation logic behaves as expected in each scenario.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 2
CHANGELOG.md (2)
- 42-42: The link provided in the bug fix entry for
MarkerTransferAuthorizationvalidation correctly points to a GitHub pull request, which is expected in a changelog. This allows readers to explore the specific changes made in more detail.- 42-42: The categorization of the change related to
MarkerTransferAuthorizationvalidation under "Bug Fixes" is appropriate, as it addresses a specific issue that was corrected. This helps readers quickly identify the nature of the change.
iramiller
previously approved these changes
Feb 23, 2024
nullpointer0x00
previously approved these changes
Feb 23, 2024
Taztingo
previously approved these changes
Feb 25, 2024
SpicyLemon
dismissed stale reviews from Taztingo, nullpointer0x00, and iramiller
via
1ba06df
Taztingo
approved these changes
Feb 26, 2024
iramiller
approved these changes
Feb 28, 2024
SpicyLemon
added a commit
that referenced
this pull request
Mar 18, 2024
* Fix MarkerTransferAuthorization validation to make sure the limit is valid Coins and the addresses in the allow list are valid. * Add changelog entry.
SpicyLemon
added a commit
that referenced
this pull request
Mar 18, 2024
* Fix MarkerTransferAuthorization validation to make sure the limit is valid Coins and the addresses in the allow list are valid. * Add changelog entry.
Merged
8 tasks
SpicyLemon
added a commit
that referenced
this pull request
Mar 18, 2024
…fer authz validation). (#1876) * Allow force transfers from marker and market accounts. (#1855) * Allow force transfers out of marker and market accounts. * Add some test cases for marker and market accounts with canForceTransferFrom. * Add changelog entry. * Tweak TestCanForceTransferFrom a little based on coderabbit feedback. Namely, have the account creators take in a string to use for the addr instead of the full address that it's going to just turn around and return. * Lint fix (needed extra empty line in keeper/marker.go). * Fix MarkerTransferAuthorization validation. (#1856) * Fix MarkerTransferAuthorization validation to make sure the limit is valid Coins and the addresses in the allow list are valid. * Add changelog entry. * Fix one of the changelog lines.
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.
Description
The
MarkerTransferAuthorization.ValidateBasic()method now ensures that theTransferLimitis validCoins(and still can't be zero), and also ensures that theAllowListentries are all valid bech32 addresses.The change to
TransferLimitvalidation is the primary fix here. What was happening is that a person could create a grant with aMarkerTransferAuthorizationin it that had an unsortedTransferLimit. Later, when that grant is used, theSafeSub(inDecreaseTransferLimit) panics because theTransferLimitisn't sorted.The extra limitation on the
AllowListis merely a nicety to help prevent people from accidentally having a worthless/incorrect entry in the list. Having a non-bech32 entry in there doesn't cause a panic or any other errors, but the value they're being checked against will always be a bech32 address string, so we might as well not allow these authorizations to have non-bech32 entries in it.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passesSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests