Allow force transfers from marker and market accounts. #1855
Conversation
|
Warning Rate Limit Exceeded@SpicyLemon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThese updates introduce an improvement to support force transfers from marker and market accounts, enhancing the system's flexibility. The changes involve adding necessary functions and modifications in the test setup to accommodate the new feature, alongside ensuring the correct account types can perform force transfers. This enhancement streamlines interactions with marker and market accounts, reflecting a thoughtful addition to the system's capabilities. 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: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- x/marker/keeper/keeper_test.go (4 hunks)
- x/marker/keeper/marker.go (2 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 2
x/marker/keeper/marker.go (2)
- 14-14: The addition of the
exchangeandmarker/typesimports is necessary for the new functionality introduced in this PR. Ensure that these packages are used effectively within the file and that there are no unused imports to maintain code cleanliness.- 711-743: The modifications to the
canForceTransferFromfunction introduce logic to allow force transfers from specific account types, including group addresses, marker accounts, and market accounts. This change is crucial for enabling the new force transfer functionality for marker and market accounts. However, it's important to ensure that this does not inadvertently allow unauthorized transfers or affect the security of the system.
- The logic to allow force transfers from group addresses and accounts with a non-zero sequence is reasonable, as it aligns with the intention to enable force transfers while maintaining security constraints.
- The explicit checks for marker accounts (
types.MarkerAccountI) and market accounts (exchange.MarketAccount) are essential for the new feature. It's important that these checks are accurate and that the types used (types.MarkerAccountIandexchange.MarketAccount) correctly represent the intended account types.- The comment on line 724-727 provides a good explanation of the rationale behind allowing force transfers only from accounts that have signed at least one transaction. This approach helps mitigate potential security risks associated with force transfers from accounts that have never been used.
Overall, the changes to this function seem to align with the objectives of the PR. However, it's crucial to ensure that these changes have been thoroughly tested, especially considering the potential security implications of force transfers.
… 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.
It is about time to rebuild the spec for the marker module in a far more detailed and expressive way ... things like state machine/flow diagrams, theory of operation, and example uses... |
I feel like those should just stay in the code. The spec docs already have info on how it all behaves. Those comments (for the most part) only apply to a few lines of code and are really dependent on how we've organized all those checks. They're really just there to help us the next time we need to go in and make changes or review the code. |
I've been keeping the transfer/send-restriction stuff up-to-date at least. But yeah, it'd be good to review/update/refactor the spec docs at some point. |
* 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).
…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.
Description
Allow force transfers from marker and market accounts.
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