refactor(x/accounts)!: accounts and auth module use the same account number tracking #20405
Conversation
WalkthroughThe changes introduce a mechanism to synchronize account numbers between the accounts and auth modules in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant SimApp
participant AccountsModKeeper
participant AuthModKeeper
participant ModuleManager
SimApp->>AccountsModKeeper: Retrieve current account number
AccountsModKeeper-->>SimApp: Return account number
SimApp->>AuthModKeeper: Set account number
SimApp->>ModuleManager: Run migrations
ModuleManager-->>SimApp: Migrations complete
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 (
|
github-actions
bot
added
C:x/distribution
sontrinh16
changed the title
…m/cosmos/cosmos-sdk into auth_accounts_use_same_acc_number
sontrinh16
changed the title
simapp/upgrades.go
Outdated
| @@ -25,6 +25,15 @@ func (app SimApp) RegisterUpgradeHandlers() { | |||
| app.UpgradeKeeper.SetUpgradeHandler( | |||
| UpgradeName, | |||
| func(ctx context.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) { | |||
| // sync accounts module and auth module account number | |||
There was a problem hiding this comment.
@julienrbrt @tac0turtle where should the migration live?
it needs to set x/accounts Account number to auth's account number.
There was a problem hiding this comment.
if accounts does not have a dependency on auth but auth has a dep on accounts then I think it would be good for auth to have the migration and set it in the other module
There was a problem hiding this comment.
added the migration in
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
tests/integration/staking/keeper/deterministic_test.go (1)
Line range hint
1-1027: Consider adding specific tests to directly verify the synchronization of account numbers between thex/accountsandx/authmodules, ensuring that the unified tracking is functioning as expected across different scenarios.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (19)
- simapp/upgrades.go (1 hunks)
- tests/integration/bank/keeper/deterministic_test.go (2 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (2 hunks)
- tests/integration/evidence/keeper/infraction_test.go (1 hunks)
- tests/integration/example/example_test.go (3 hunks)
- tests/integration/gov/keeper/keeper_test.go (2 hunks)
- tests/integration/slashing/keeper/keeper_test.go (2 hunks)
- tests/integration/staking/keeper/common_test.go (3 hunks)
- tests/integration/staking/keeper/deterministic_test.go (2 hunks)
- x/accounts/keeper.go (1 hunks)
- x/auth/ante/testutil_test.go (2 hunks)
- x/auth/keeper/deterministic_test.go (2 hunks)
- x/auth/keeper/keeper.go (2 hunks)
- x/auth/keeper/keeper_test.go (2 hunks)
- x/auth/testutil/expected_keepers_mocks.go (1 hunks)
- x/auth/types/expected_keepers.go (1 hunks)
- x/auth/vesting/testutil/expected_keepers_mocks.go (1 hunks)
- x/distribution/migrations/v4/migrate_funds_test.go (2 hunks)
- x/group/migrations/v2/migrate_test.go (3 hunks)
Files not reviewed due to errors (1)
- x/auth/testutil/expected_keepers_mocks.go (no review received)
Additional Context Used
Path-based Instructions (19)
x/auth/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/upgrades.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/migrations/v4/migrate_funds_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/group/migrations/v2/migrate_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/vesting/testutil/expected_keepers_mocks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/testutil/expected_keepers_mocks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/gov/keeper/keeper_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/staking/keeper/common_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/example/example_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/ante/testutil_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/auth/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/keeper/deterministic_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/bank/keeper/deterministic_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/evidence/keeper/infraction_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/slashing/keeper/keeper_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/staking/keeper/deterministic_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/distribution/keeper/msg_server_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (18)
x/auth/types/expected_keepers.go (1)
22-22: Addition ofNextAccountNumbermethod toAccountsModKeeperaligns with the PR's objectives to synchronize account numbers.simapp/upgrades.go (1)
28-36: Proper implementation of account number synchronization betweenauthandaccountsmodules during upgrades.x/distribution/migrations/v4/migrate_funds_test.go (1)
57-63: Proper testing of account number generation logic in migration tests.x/group/migrations/v2/migrate_test.go (1)
Line range hint
108-130: Proper testing of account number generation logic in group migration tests.tests/integration/gov/keeper/keeper_test.go (1)
81-86: Ensure the mock forNextAccountNumberis correctly simulating the account number increment.This mock setup correctly simulates the behavior of incrementing account numbers, which is crucial for testing the integration of the
govmodule with the account number synchronization changes.tests/integration/staking/keeper/common_test.go (1)
191-196: Ensure the mock forNextAccountNumberis correctly simulating the account number increment.This mock setup correctly simulates the behavior of incrementing account numbers, which is crucial for testing the integration of the
stakingmodule with the account number synchronization changes.tests/integration/example/example_test.go (1)
53-58: Ensure the mock forNextAccountNumberis correctly simulating the account number increment.This mock setup correctly simulates the behavior of incrementing account numbers, which is crucial for testing the integration of the
examplemodule with the account number synchronization changes.x/auth/keeper/keeper_test.go (1)
66-71: Ensure the mock forNextAccountNumberis correctly simulating the account number increment.This mock setup correctly simulates the behavior of incrementing account numbers, which is crucial for testing the integration of the
authmodule with the account number synchronization changes.x/auth/ante/testutil_test.go (1)
83-88: Ensure the mock forNextAccountNumberis correctly simulating the account number increment.This mock setup correctly simulates the behavior of incrementing account numbers, which is crucial for testing the integration of the
antemodule with the account number synchronization changes.x/auth/keeper/deterministic_test.go (1)
73-78: LGTM! The mock setup forNextAccountNumbercorrectly simulates sequential account number generation, which is essential for testing the integration of account number synchronization between modules.x/accounts/keeper.go (2)
110-118: LGTM! The implementation ofNextAccountNumberusescollections.Sequencefor atomic increments, ensuring thread safety and correctness in concurrent environments.
121-125: LGTM! TheSetAccountNumbermethod provides a controlled way to set the account number directly, which is essential for operations like migrations.tests/integration/bank/keeper/deterministic_test.go (1)
88-93: LGTM! The mock setup forNextAccountNumberin the bank keeper tests correctly simulates sequential account number generation, aligning with the integration needs of the account number synchronization feature.tests/integration/evidence/keeper/infraction_test.go (1)
110-115: The mock setup forNextAccountNumberis correctly implemented and suitable for integration testing scenarios.tests/integration/slashing/keeper/keeper_test.go (1)
85-92: The mock setup forNextAccountNumberis correctly implemented and suitable for integration testing scenarios.tests/integration/staking/keeper/deterministic_test.go (1)
96-101: The mocking ofNextAccountNumberwith a sequential increment setup is well-implemented and aligns with the integration test's needs for simulating account number generation.tests/integration/distribution/keeper/msg_server_test.go (2)
4-4: Added import of "context" package.This change is necessary for the new context handling in the test functions.
99-104: Added logic for simulating account number generation.This change aligns with the PR's objective to synchronize account numbers across modules. It effectively mocks the
NextAccountNumbermethod to simulate sequential account number generation, which is crucial for testing the integrated behavior of the distribution module with the new account number handling.
x/auth/keeper/keeper.go
Outdated
| @@ -182,7 +184,7 @@ func (ak AccountKeeper) GetSequence(ctx context.Context, addr sdk.AccAddress) (u | |||
| // NextAccountNumber returns and increments the global account number counter. | |||
| // If the global account number is not set, it initializes it with value 0. | |||
| func (ak AccountKeeper) NextAccountNumber(ctx context.Context) uint64 { | |||
| n, err := ak.AccountNumber.Next(ctx) | |||
| n, err := ak.AccountsModKeeper.NextAccountNumber(ctx) | |||
There was a problem hiding this comment.
Ensure error handling is robust in NextAccountNumber.
The current implementation panics if an error occurs when fetching the next account number. Consider handling this error more gracefully to avoid potential service disruptions. Here's a suggested change:
- if err != nil {
- panic(err)
- }
+ if err != nil {
+ return 0, err
+ }This change will allow the caller to handle the error appropriately instead of causing a panic.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| n, err := ak.AccountsModKeeper.NextAccountNumber(ctx) | |
| n, err := ak.AccountsModKeeper.NextAccountNumber(ctx) | |
| if err != nil { | |
| return 0, err | |
| } |
…m/cosmos/cosmos-sdk into auth_accounts_use_same_acc_number
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- x/accounts/keeper.go (1 hunks)
- x/auth/keeper/migrations.go (2 hunks)
- x/auth/migrations/v6/migrate.go (1 hunks)
- x/auth/module.go (2 hunks)
- x/auth/testutil/expected_keepers_mocks.go (2 hunks)
- x/auth/types/expected_keepers.go (1 hunks)
- x/auth/vesting/testutil/expected_keepers_mocks.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- x/accounts/keeper.go
- x/auth/testutil/expected_keepers_mocks.go
- x/auth/types/expected_keepers.go
- x/auth/vesting/testutil/expected_keepers_mocks.go
Additional Context Used
Path-based Instructions (3)
x/auth/migrations/v6/migrate.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/keeper/migrations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
x/auth/migrations/v6/migrate.go (1)
9-12: TheMigratefunction implementation is clean and adheres to good practices by abstracting the migration logic into a passed function.x/auth/keeper/migrations.go (2)
49-53: The migration logic inMigrate5To6is correctly implemented, ensuring a smooth transition of account numbers between modules.
55-62: Thev56SetAccountsfunction correctly handles the retrieval and setting of the account number, including proper error handling.x/auth/module.go (2)
26-26: Updating theConsensusVersionto 6 correctly reflects the new version after migration, aligning with the PR's objectives.
112-114: The registration of theMigrate5To6function inRegisterMigrationsis correctly implemented, ensuring the migration logic is properly integrated into the module lifecycle.
There was a problem hiding this comment.
Good start!
I would love to see some sanity check on the account number, just in case. Other than this, I added personal preferences and nits only.
Have you considered to do the migration in the accounts module instead? With this, you may be able to avoid a public SetAccountNumber method
hmmmm, i thought of that before but like accounts is a new module that we are adding in v51, the migration here wont make much sense to me. it would be easier just set it in accounts in the upgrade handler before run migration and add a note so that user remember to add it in their chain upgrade handler like this |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/accounts/keeper.go (1)
121-132: LGTM! TheInitAccountNumberSeqfunction includes a safety check to ensure the account number is not set to a lower value than the current, which is crucial for data integrity.Consider adding detailed documentation for this method, explaining its usage and the importance of the safety check.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (9)
- x/accounts/keeper.go (1 hunks)
- x/auth/ante/testutil_test.go (2 hunks)
- x/auth/keeper/deterministic_test.go (2 hunks)
- x/auth/keeper/keeper.go (3 hunks)
- x/auth/keeper/keeper_test.go (2 hunks)
- x/auth/keeper/migrations.go (3 hunks)
- x/auth/testutil/expected_keepers_mocks.go (2 hunks)
- x/auth/types/expected_keepers.go (1 hunks)
- x/auth/vesting/testutil/expected_keepers_mocks.go (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- x/auth/ante/testutil_test.go
- x/auth/keeper/deterministic_test.go
- x/auth/keeper/keeper.go
- x/auth/keeper/keeper_test.go
- x/auth/keeper/migrations.go
- x/auth/testutil/expected_keepers_mocks.go
- x/auth/types/expected_keepers.go
- x/auth/vesting/testutil/expected_keepers_mocks.go
Additional Context Used
Path-based Instructions (1)
x/accounts/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
x/accounts/keeper.go (2)
110-119: LGTM! TheNextAccountNumberfunction correctly fetches and increments the account number, with proper error handling.
Line range hint
1-450: The rest of the file appears well-structured and adheres to best practices. The methods related to account handling are implemented correctly, and there are no apparent performance issues.
[APROVED]
…m/cosmos/cosmos-sdk into auth_accounts_use_same_acc_number
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
server/config/priv_validator_key.json (1)
1-11: Ensure secure handling of private keys.This file contains sensitive information, including private keys. It's crucial to ensure that this file is securely stored and access is restricted to prevent unauthorized access.
x/auth/keeper/account.go (1)
25-30: Consider alternative error handling strategies topanic.Using
panicfor error handling can be appropriate for unrecoverable errors, but it also terminates the program, which might not always be desirable. Consider alternatives like returning errors to the caller, especially in a library or module that might be used in various contexts where more graceful error handling could be necessary.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (10)
- server/config/config.toml (1 hunks)
- server/config/genesis.json (1 hunks)
- server/config/node_key.json (1 hunks)
- server/config/priv_validator_key.json (1 hunks)
- server/data/priv_validator_state.json (1 hunks)
- x/auth/keeper/account.go (1 hunks)
- x/auth/keeper/genesis.go (1 hunks)
- x/auth/keeper/keeper.go (4 hunks)
- x/auth/keeper/keeper_test.go (4 hunks)
- x/group/migrations/v2/migrate_test.go (3 hunks)
Files skipped from review due to trivial changes (4)
- server/config/config.toml
- server/config/genesis.json
- server/config/node_key.json
- server/data/priv_validator_state.json
Files skipped from review as they are similar to previous changes (3)
- x/auth/keeper/keeper.go
- x/auth/keeper/keeper_test.go
- x/group/migrations/v2/migrate_test.go
Additional Context Used
Path-based Instructions (2)
x/auth/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/keeper/account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
| n, err := ak.AccountsModKeeper.NextAccountNumber(ctx) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Potential logical flaw in setting the last account number.
The logic for updating lastAccNum seems to be inside a loop that checks if lastAccNum is less than accNum. This could lead to incorrect behavior if not all accounts are processed or if the account numbers are not strictly increasing. Consider revising this logic to ensure that lastAccNum correctly reflects the highest account number after all accounts are processed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err := acc.SetAccountNumber(ak.NextAccountNumber(ctx)); err != nil { | ||
| accNum, err := ak.AccountsModKeeper.NextAccountNumber(ctx) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/auth/keeper/keeper.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/auth/keeper/keeper.go
Description
As for right now, x/accounts use a seperate account number sequence which we should merge it with auth account sequece, this is a first step to make x/auth -> x/accounts compat
continue work from #20234
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
New Features
Bug Fixes
Documentation
Chores