test: x/accounts systemtests #22320
test: x/accounts systemtests #22320
Conversation
lucaslopezf
changed the title
lucaslopezf
requested review from
testinginprod,
sontrinh16 and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new system test function, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/account_test.go (2)
90-91: Fix grammatical error in comment.Correct the comment for better readability.
Apply this diff to correct the comment:
-// Verify the account not exist in account +// Verify that the account does not exist in x/accounts
110-110: Fix grammatical error in comment.Adjust the comment for grammatical correctness.
Apply this diff to improve the comment:
-// 3. Now the account should be existed, query the account Sequence +// 3. Now the account should exist; query the account sequence
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- tests/systemtests/account_test.go (2 hunks)
- tests/systemtests/cli.go (1 hunks)
- x/accounts/module.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/systemtests/account_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/systemtests/cli.go (2)
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"x/accounts/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
x/accounts/module.go (6)
10-15: LGTM: Import statements are well-organized.The new import statements are correctly added and grouped logically, adhering to the Uber Golang style guide.
47-47: LGTM: Method signature improved.The update to
func (AppModule) IsAppModule() {}enhances clarity and follows the Uber Golang style guide.
51-52: LGTM: Method signature and formatting improved.The update to
func (am AppModule) Name() stringenhances consistency. The added blank line improves readability. Both changes align with the Uber Golang style guide.
67-69: LGTM: Method signature and implementation updated correctly.The
RegisterServicesmethod has been updated consistently with other changes. The use ofam.kin the implementation is correct and maintains the existing functionality.
Line range hint
1-114: Overall LGTM: Changes align well with PR objectives.The modifications to
x/accounts/module.goeffectively support the PR objectives for account migration and enhanced functionality. The updates to method signatures, interface registrations, and service registrations are consistent and follow the Uber Golang style guide. These changes provide the necessary groundwork for the system tests introduced inTestAccountsMigration.To ensure full coverage of the PR objectives, please confirm that all necessary message types for account migration, key swapping, and sequence querying are registered. Run the following command to check:
✅ Verification successful
All necessary message types are properly registered across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for all registered message types related to account operations rg -i "register.*implementation.*msg" x/accounts/Length of output: 619
53-60: LGTM with a note: New message types registered correctly.The method signature update and registration of new message types are correct and align with the PR objectives. However, the manual registration approach mentioned in the previous comment is still in use.
Could you clarify why dynamic registration is not working as expected? If it's a known issue, consider adding a TODO comment explaining the situation and linking to any relevant issues or discussions.
i think other account type ( lockup, multisig ) need it own seperate migration logic |
Do you mean add it here or in another PR? @sontrinh16 |
|
lets keep it simple with only base migration, no need to over complicate |
oh another PR, sorry for the confusion |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/systemtests/account_test.go (4)
77-108: LGTM: Account verification and migration process.The account verification and migration steps are well-implemented. The code correctly checks the account's existence in the auth module, verifies its absence in the accounts module before migration, and then executes the migration transaction.
Consider adding an explicit error check after the migration transaction to ensure it was successful:
require.NoError(t, cli.TxSucceeded(rsp), "Migration transaction failed")This will provide more detailed error information if the migration fails.
110-113: Enhance post-migration sequence check.While the code correctly verifies the existence of the sequence field, it would be more robust to check the actual value of the sequence. Consider updating the check as follows:
sequence := gjson.Get(rsp, "sequence").Int() require.Equal(t, int64(0), sequence, "Initial sequence should be 0 after migration")This ensures that the sequence is not only present but also initialized to the expected value of 0 after migration.
142-160: Enhance key swapping verification.The key swapping test is well-implemented, but it lacks verification that the swap was successful. Consider adding a check after the swap to ensure the new public key is associated with the account:
// Verify the key swap was successful rsp = cli.CustomQuery("q", "accounts", "query", legacyAddress, "cosmos.accounts.defaults.base.v1.QueryPubKey", "{}") swappedPubKey := gjson.Get(rsp, "pub_key.key").String() require.Equal(t, newPubKey, swappedPubKey, "Public key should be updated after swap")This additional check will ensure that the key swapping functionality works as expected.
56-161: Minor style improvements for consistency.The code generally adheres well to the Uber Golang style guide. To further improve consistency:
- Consider using constants for repeated string literals, such as "stake" for the token denomination.
- Group related constants together at the beginning of the function, as suggested in a previous comment about using constants for balances and fees.
- Use
t.Helper()in helper functions likeRequireTxSuccessto improve test output.These minor adjustments will enhance the overall code quality and maintainability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/systemtests/account_test.go (2 hunks)
- x/accounts/module.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/accounts/module.go
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/account_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 (3)
tests/systemtests/account_test.go (3)
56-75: LGTM: Test setup and initialization.The test environment setup is well-structured. It correctly initializes the chain, creates necessary accounts, and modifies the genesis state. This provides a solid foundation for the subsequent test cases.
115-140: Refer to previous comment on using constants.The balance checks and transaction execution are correct. However, as mentioned in a previous review comment, consider using constants for initial balances, fees, and transfer amounts to improve readability and maintainability.
56-161: LGTM: Comprehensive test coverage for account migration.The
TestAccountsMigrationfunction provides excellent coverage of the account migration process, aligning well with the objectives outlined in issue #21954. It successfully tests:
- Creation of a legacy x/auth account
- Migration to the x/accounts module
- Transaction execution using the x/accounts interface
- Querying the account's sequence
- Swapping the public key
The test structure is logical and easy to follow. While there's room for minor improvements in error handling and verification steps (as mentioned in previous comments), the overall implementation is solid and achieves the intended goals.
lucaslopezf
requested review from
facundomedica and
julienrbrt
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
simapp/simd/cmd/root_di.go (1)
48-53: LGTM: New providers enhance account management capabilities.The added providers for multisig, base, and lockup account types, along with the Secp256K1 public key provider, align well with the PR objectives. They enhance the flexibility of account management in the application.
Consider grouping related providers together for better readability:
multisigdepinject.ProvideAccount, basedepinject.ProvideAccount, lockupdepinject.ProvideAllLockupAccounts, basedepinject.ProvideSecp256K1PubKey,
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- simapp/simd/cmd/root_di.go (2 hunks)
- x/accounts/keeper.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
simapp/simd/cmd/root_di.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.
🔇 Additional comments (3)
simapp/simd/cmd/root_di.go (2)
19-21: LGTM: New imports for account management.The added import statements for base, lockup, and multisig account types are well-organized and follow Go best practices. They align with the PR objectives of enhancing account management capabilities.
Line range hint
19-53: Changes align well with PR objectives.The modifications to
root_di.goeffectively enhance the account management capabilities of the application by introducing new providers for various account types. These changes are well-contained and don't disrupt the existing structure or functionality of the file.The additions align perfectly with the PR objectives of introducing system tests for the x/accounts module and improving account management. The new providers will support the account migration and management functionalities being tested in the
TestAccountsMigrationfunction mentioned in the PR description.To ensure comprehensive coverage, please confirm that corresponding system tests have been implemented for each new account type (base, lockup, multisig) in the
TestAccountsMigrationfunction.x/accounts/keeper.go (1)
13-13: Import for Side Effects Is Appropriately AddedThe import of
cosmossdk.io/api/cosmos/accounts/defaults/base/v1with an anonymous identifier is correctly used for side-effects, which aligns with the intended functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
simapp/v2/simdv2/cmd/root_di.go (1)
42-50: Consider enhancing provider documentation.The dependency injection setup correctly includes all necessary account providers for the system tests. However, the comments could be more descriptive.
Consider updating the comment to be more specific:
- // inject desired account types: + // inject account type providers for base, multisig, and lockup accounts:
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- simapp/v2/simdv2/cmd/root_di.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
simapp/v2/simdv2/cmd/root_di.go (2)
17-19: LGTM: New imports are well-organized and follow conventions.The added imports for base, lockup, and multisig account providers align with the PR objectives and follow proper Go import conventions.
42-50: Verify account type availability for system tests.The dependency injection setup appears complete, but let's verify that all required account types are available for the system tests.
✅ Verification successful
Let me gather more information about the account types and their registration to make a thorough verification.
Let me gather one final piece of information about the account migration test to ensure we have a complete picture.
Account type dependency injection setup is correctly configured
Based on the verification results, the dependency injection setup in
root_di.gocorrectly provides all necessary account types for the system tests:
- The provided account types (multisig, base, and lockup) align with the comprehensive account migration test implementation.
- The test suite demonstrates successful:
- Base account creation and operations
- Account migration from x/auth to x/accounts
- Public key management
- Transaction capabilities post-migration
The dependency injection configuration supports all the account-related functionality being tested in the system tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary account types are properly registered # and available for the system tests. # Check for account type registrations in the codebase echo "Checking account type registrations..." rg -A 3 "RegisterImplementations|RegisterInterface" tests/systemtests/ # Verify account migration test coverage echo "Checking account migration test coverage..." rg -A 5 "TestAccountsMigration" tests/systemtests/Length of output: 933
Script:
#!/bin/bash # Check for account-related code in the test files echo "Checking account-related test implementations..." rg -A 5 "type.*Account struct" tests/systemtests/ # Check for account type providers echo "Checking account type providers..." rg -A 3 "Provide.*Account" simapp/v2/simdv2/ # Check account migration test details echo "Checking account migration test implementation..." rg -B 5 -A 10 "TestAccountsMigration" tests/systemtests/account_test.goLength of output: 1485
Script:
#!/bin/bash # Check the complete account migration test implementation echo "Checking full account migration test..." cat tests/systemtests/account_test.go # Check for any additional account-related configurations echo "Checking account configurations..." rg -A 3 "account.*config" simapp/v2/simdv2/Length of output: 5902
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 8fa3090)
Co-authored-by: Lucas Francisco López <[email protected]>
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Description
Closes: #21954
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation