refactor(bank, feegrant, authz): avoid creating baseaccount #19188
refactor(bank, feegrant, authz): avoid creating baseaccount #19188tac0turtle merged 47 commits intomainfrom
Conversation
|
Warning Rate Limit Exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 48 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. WalkthroughThe significant changes revolve around enhancing account creation and initialization processes within a blockchain system, particularly focusing on managing accounts without existing blockchain presence. These updates aim to prevent the automatic initialization of Changes
Assessment against linked issues
The changes align with the objectives outlined in the linked issue, addressing the need to prevent automatic 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 (
|
tac0turtle
force-pushed
the
marko/19092
branch
from
b2f9f0d to
4edf881
Compare
tac0turtle
changed the title
tac0turtle
force-pushed
the
marko/19092
branch
from
abce524 to
ee38e20
Compare
alexanderbez
left a comment
alexanderbez
left a comment
There was a problem hiding this comment.
After the updates, this looks good to me know (pending addressing @julienrbrt's comment)
![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.yml
Files selected for processing (2)
- client/test_helpers.go (3 hunks)
- x/auth/types/account_retriever.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- client/test_helpers.go
- x/auth/types/account_retriever.go
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
lgtm! one nit about bringing back the error check in client factory, now we return no error when the account doesn't exist in the account retriever.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (7)
- tests/integration/gov/abci_test.go (3 hunks)
- tests/integration/gov/keeper/tally_test.go (1 hunks)
- x/auth/ante/ante_test.go (2 hunks)
- x/auth/ante/feegrant_test.go (2 hunks)
- x/auth/types/account_retriever.go (2 hunks)
- x/authz/keeper/msg_server.go (1 hunks)
- x/feegrant/keeper/keeper.go (1 hunks)
Additional comments: 9
x/auth/types/account_retriever.go (1)
- 78-80: LGTM. The addition of handling for
codes.NotFoundimproves clarity and error handling for non-existent accounts.x/auth/ante/feegrant_test.go (3)
- 59-59: LGTM. Adjusting the
validflag totruefor cases without an existing account aligns with the PR's objectives.- 63-66: LGTM. Using
authtypes.NewBaseAccountWithAddressfor creating test accounts without automaticBaseAccountcreation aligns with the PR's objectives.- 81-81: LGTM. The adjustment for the "no fee with no account" case aligns with the PR's objectives to handle non-existent accounts more gracefully.
tests/integration/gov/keeper/tally_test.go (1)
- 356-361: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file focuses on integration tests for the governance module's tallying functionality. Ensure that the tests cover all relevant scenarios for vote tallying, including edge cases.
tests/integration/gov/abci_test.go (3)
- 363-364: Account creation logic added without checking if the account already exists. This issue was previously flagged and a solution was provided. Ensure the fix is applied to prevent redundant account creation.
- 426-428: Account creation logic added without checking if the account already exists. This issue was previously flagged and a solution was provided. Ensure the fix is applied to prevent redundant account creation.
- 507-508: Account creation logic added without checking if the account already exists. This issue was previously flagged and a solution was provided. Ensure the fix is applied to prevent redundant account creation.
x/auth/ante/ante_test.go (1)
- 164-171: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [157-168]
The change in expected behavior for
SendCoinsFromAccountToModuleto returnnilinstead of an error in theTestAnteHandlerSigErrorsfunction aligns with the PR's objective to modify account creation logic. Ensure this change is consistent with the intended logic across the SDK and does not affect other dependent tests or functionalities.
| } | ||
|
|
||
| // if the account number is 0 and the account is signing, the sign doc will not have an account number | ||
| if acc.GetSequence() == 0 && newlyCreated { |
There was a problem hiding this comment.
Isn't the acc.GetSequence() == 0 obsolete here because whenever newlyCreated is true, the sequence is 0?
There was a problem hiding this comment.
Yes, but it doesn't hurt in the grand scheme of things.
|
reminder: add changelog |
5 participants
Description
Closes: #19092
avoid creating baseaccount if it does not exist, preventing front running of accounts
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
MsgBurn, for burning coins.x/feegrantmodule is now a standalone module with enhanced functionality.BaseAccountcreation for non-existent accounts in transactions, impacting several modules.