-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(x/accounts)!: accounts and auth module use the same account number tracking #20405
base: main
Are you sure you want to change the base?
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 (
|
…m/cosmos/cosmos-sdk into auth_accounts_use_same_acc_number
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the migration in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pending migration location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/accounts
andx/auth
modules, 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 ofNextAccountNumber
method toAccountsModKeeper
aligns with the PR's objectives to synchronize account numbers.simapp/upgrades.go (1)
28-36
: Proper implementation of account number synchronization betweenauth
andaccounts
modules 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 forNextAccountNumber
is 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
gov
module with the account number synchronization changes.tests/integration/staking/keeper/common_test.go (1)
191-196
: Ensure the mock forNextAccountNumber
is 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
staking
module with the account number synchronization changes.tests/integration/example/example_test.go (1)
53-58
: Ensure the mock forNextAccountNumber
is 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
example
module with the account number synchronization changes.x/auth/keeper/keeper_test.go (1)
66-71
: Ensure the mock forNextAccountNumber
is 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
auth
module with the account number synchronization changes.x/auth/ante/testutil_test.go (1)
83-88
: Ensure the mock forNextAccountNumber
is 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
ante
module with the account number synchronization changes.x/auth/keeper/deterministic_test.go (1)
73-78
: LGTM! The mock setup forNextAccountNumber
correctly 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 ofNextAccountNumber
usescollections.Sequence
for atomic increments, ensuring thread safety and correctness in concurrent environments.
121-125
: LGTM! TheSetAccountNumber
method 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 forNextAccountNumber
in 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 forNextAccountNumber
is correctly implemented and suitable for integration testing scenarios.tests/integration/slashing/keeper/keeper_test.go (1)
85-92
: The mock setup forNextAccountNumber
is correctly implemented and suitable for integration testing scenarios.tests/integration/staking/keeper/deterministic_test.go (1)
96-101
: The mocking ofNextAccountNumber
with 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
NextAccountNumber
method 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
: TheMigrate
function 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 inMigrate5To6
is correctly implemented, ensuring a smooth transition of account numbers between modules.
55-62
: Thev56SetAccounts
function correctly handles the retrieval and setting of the account number, including proper error handling.x/auth/module.go (2)
26-26
: Updating theConsensusVersion
to 6 correctly reflects the new version after migration, aligning with the PR's objectives.
112-114
: The registration of theMigrate5To6
function inRegisterMigrations
is correctly implemented, ensuring the migration logic is properly integrated into the module lifecycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/accounts/keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/accounts/keeper.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/accounts/keeper.go (1)
121-132
: LGTM! TheInitAccountNumberSeq
function 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! TheNextAccountNumber
function 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
panic
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.md
Reviewers 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