refactor(x/accounts)!: accounts and auth module use the same account number tracking#20405
Conversation
WalkthroughThe changes introduce a new logic to synchronize account numbers between the accounts and auth modules in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant SimApp
participant AccountsModule
participant AuthModule
User->>SimApp: Initiate Upgrade
SimApp->>AuthModule: Retrieve Current Account Number
AuthModule-->>SimApp: Return Account Number
SimApp->>AccountsModule: Set Account Number
SimApp->>SimApp: Run Migrations
SimApp-->>User: Upgrade Complete
sequenceDiagram
participant Test
participant MockAccountsModKeeper
participant Context
Test->>MockAccountsModKeeper: Expect NextAccountNumber
MockAccountsModKeeper->>Context: Retrieve Account Number
Context-->>MockAccountsModKeeper: Return Mock Account Number
MockAccountsModKeeper-->>Test: Return Account Number
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
| 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
testinginprod
left a comment
testinginprod
left a comment
There was a problem hiding this comment.
LGTM, pending migration location
![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.
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
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
x/auth/keeper/migrations.go (1)
16-17: Consider adding a comment explaining the purpose ofaccNum.Adding a comment here would improve code readability and maintainability, especially for new developers or external contributors who might work on this code in the future.
x/accounts/keeper.go (1)
Line range hint
17-17: Remove the use of the internal package to adhere to best practices.Using internal packages from other modules can lead to maintenance issues and tightly coupled code. Consider refactoring to use public interfaces or duplicating the necessary parts within this module.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- testutil/mock/logger.go (1 hunks)
- x/accounts/keeper.go (1 hunks)
- x/auth/keeper/keeper.go (3 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 (3)
- 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 (4)
x/auth/keeper/migrations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/mock/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
x/auth/keeper/migrations.go
15-15: undefined: AccountKeeper
21-21: undefined: AccountKeeper
x/auth/keeper/keeper.go
107-107: cannot use &AccountKeeper{} (value of type *AccountKeeper) as AccountKeeperI value in variable declaration: *AccountKeeper does not implement AccountKeeperI (missing method GetAccount)
163-163: ak.GetAccount undefined (type AccountKeeper has no field or method GetAccount)
173-173: ak.GetAccount undefined (type AccountKeeper has no field or method GetAccount)
239-239: ak.GetAccount undefined (type AccountKeeper has no field or method GetAccount)
250-250: ak.NewAccount undefined (type AccountKeeper has no field or method NewAccount)
265-265: ak.SetAccount undefined (type AccountKeeper has no field or method SetAccount)
x/accounts/keeper.go
17-17: use of internal package cosmossdk.io/x/accounts/internal/implementation not allowed
Additional comments not posted (3)
x/auth/keeper/migrations.go (2)
22-25: Ensure proper error handling when creatingaccNumSeq.
51-52: Ensure that the migration logic inMigrate4To5is thoroughly tested.Verification successful
The search did not reveal any test functions directly related to
Migrate4To5. However, there was a test function namedTestMigrateinx/auth/migrations/v5/migrate_test.go, which might be relevant.To ensure comprehensive verification, we should inspect the contents of
x/auth/migrations/v5/migrate_test.goto confirm if it covers theMigrate4To5function.
The
TestMigratefunction inx/auth/migrations/v5/migrate_test.godoes indeed test the migration logic, specifically theMigratefunction, which is called withinMigrate4To5. This confirms that the migration logic is being tested.
TestMigratesets up a mock store and schema, sets a legacy sequence value, and verifies that the migration correctly updates the sequence.- It also tests the case where the global account number is not set, ensuring the migration handles this scenario correctly.
Thus, the migration logic in
Migrate4To5is thoroughly tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for the migration logic in `Migrate4To5`. # Test: Search for tests related to `Migrate4To5`. rg --type go $'Migrate4To5' --context 5Length of output: 1830
Script:
#!/bin/bash # Description: Verify the test coverage for the migration logic in `Migrate4To5`. # Find all Go test files in the x/auth directory. test_files=$(fd --type f --extension go --search-path x/auth --exec echo {}) # Search for test functions related to migration in the found test files. for file in $test_files; do echo "Searching in $file" rg --type go --context 5 'func Test.*Migrate' $file doneLength of output: 30192
Script:
#!/bin/bash # Description: Inspect the contents of the test file that might cover the Migrate4To5 function. # Display the contents of the test file to verify if it tests Migrate4To5. cat x/auth/migrations/v5/migrate_test.goLength of output: 1190
testutil/mock/logger.go (1)
10-10: Update to the new import path for thelogpackage is correct and follows the project's dependency updates.This change aligns with the recent updates to the logging infrastructure, ensuring compatibility and maintaining the project's standards.
| func (m Migrator) Migrate5To6(ctx context.Context) error { | ||
| currentAccNum, err := m.accNum.Peek(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return m.keeper.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum) |
There was a problem hiding this comment.
Refactor to handle errors more gracefully instead of just returning them.
- if err != nil {
- return err
- }
+ if err != nil {
+ // Consider logging the error or handling it according to your error policy
+ return fmt.Errorf("failed to peek account number: %w", err)
+ }This change enhances error reporting and maintains consistency with error handling practices throughout the codebase.
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.
| func (m Migrator) Migrate5To6(ctx context.Context) error { | |
| currentAccNum, err := m.accNum.Peek(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| return m.keeper.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum) | |
| func (m Migrator) Migrate5To6(ctx context.Context) error { | |
| currentAccNum, err := m.accNum.Peek(ctx) | |
| if err != nil { | |
| // Consider logging the error or handling it according to your error policy | |
| return fmt.Errorf("failed to peek account number: %w", err) | |
| } | |
| return m.keeper.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum) |
| // InitAccountNumberSeqUnsafe use to set accounts account number tracking. | ||
| // Only use for account number migration. | ||
| func (k Keeper) InitAccountNumberSeqUnsafe(ctx context.Context, accNum uint64) error { | ||
| currentNum, err := k.AccountNumber.Peek(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if currentNum >= accNum { | ||
| return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum) | ||
| } | ||
| return k.AccountNumber.Set(ctx, accNum) | ||
| } |
There was a problem hiding this comment.
Add comprehensive documentation for InitAccountNumberSeqUnsafe.
+// InitAccountNumberSeqUnsafe sets the initial account number sequence.
+// This method is unsafe and should only be used during migrations.Adding detailed comments will improve code readability and help other developers understand the context and risks associated with using this method.
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.
| // InitAccountNumberSeqUnsafe use to set accounts account number tracking. | |
| // Only use for account number migration. | |
| func (k Keeper) InitAccountNumberSeqUnsafe(ctx context.Context, accNum uint64) error { | |
| currentNum, err := k.AccountNumber.Peek(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| if currentNum >= accNum { | |
| return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum) | |
| } | |
| return k.AccountNumber.Set(ctx, accNum) | |
| } | |
| // InitAccountNumberSeqUnsafe sets the initial account number sequence. | |
| // This method is unsafe and should only be used during migrations. | |
| func (k Keeper) InitAccountNumberSeqUnsafe(ctx context.Context, accNum uint64) error { | |
| currentNum, err := k.AccountNumber.Peek(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| if currentNum >= accNum { | |
| return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum) | |
| } | |
| return k.AccountNumber.Set(ctx, accNum) | |
| } |
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
This looks good now. A small nit on an error message but good to go 🏄
| return err | ||
| } | ||
|
|
||
| return m.keeper.AccountsModKeeper.InitAccountNumberSeqUnsafe(ctx, currentAccNum) |
…m/cosmos/cosmos-sdk into auth_accounts_use_same_acc_number
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/accounts/keeper.go (1 hunks)
- x/auth/keeper/keeper.go (4 hunks)
Additional context used
Path-based instructions (2)
x/auth/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
x/auth/keeper/keeper.go
109-109: cannot use &AccountKeeper{} (value of type *AccountKeeper) as AccountKeeperI value in variable declaration: *AccountKeeper does not implement AccountKeeperI (missing method GetAccount)
165-165: ak.GetAccount undefined (type AccountKeeper has no field or method GetAccount)
175-175: ak.GetAccount undefined (type AccountKeeper has no field or method GetAccount)
241-241: ak.GetAccount undefined (type AccountKeeper has no field or method GetAccount)
252-252: ak.NewAccount undefined (type AccountKeeper has no field or method NewAccount)
267-267: ak.SetAccount undefined (type AccountKeeper has no field or method SetAccount)
x/accounts/keeper.go
17-17: use of internal package cosmossdk.io/x/accounts/internal/implementation not allowed
Additional comments not posted (1)
x/accounts/keeper.go (1)
110-119: The implementation ofNextAccountNumberlooks correct and well-handled.
| // Deprecated: NextAccountNumber is deprecated | ||
| 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.
Address the error handling issue in NextAccountNumber to prevent potential service disruptions.
- panic(err)
+ if err != nil {
+ return 0, err
+ }This change allows the caller to handle the error appropriately instead of causing a panic, enhancing the robustness of the service.
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 | |
| } |
| return err | ||
| } | ||
| if currentNum > accNum { | ||
| return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum) |
There was a problem hiding this comment.
Consider refining the error message in InitAccountNumberSeqUnsafe for clarity and accuracy.
- return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum)
+ return fmt.Errorf("attempted to set account number to %v, which is lower than the current account number %v", accNum, currentNum)This change makes the error message more direct and easier to understand.
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.
| return fmt.Errorf("cannot set number lower than current account number got %v while current account number is %v", accNum, currentNum) | |
| return fmt.Errorf("attempted to set account number to %v, which is lower than the current account number %v", accNum, currentNum) |
* main: refactor(x/feegrant): set environment in context (#20529) refactor(x/accounts)!: accounts and auth module use the same account number tracking (#20405) chore: remove sonar from simapp (#20528) docs: add docs on permissions (#20526) refactor(x/gov): set environment in context for legacy proposals (#20521) docs: migrate diagrams to mermaidjs (#20503) refactor(tools/hubl): don't use nil panic (#20515) refactor(x/authz): set environment in context (#20502) build(deps): Bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#20519) feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins (#20517) chore: sonar ignore directories with their own go.mods (#20509) ci: run action in merge queue (#20508) refactor(server/v2/cometbft): update function comments (#20506)
6 participants
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
Tests
Chores