test(accounts): fix integration tests #22418
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the account management capabilities within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the 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
|
Still some tests are failing without build tag. Will try to fix them. |
akhilkumarpilli
changed the title
julienrbrt
added
the
backport/v0.52.x
akhilkumarpilli
requested review from
kocubinski,
a team,
testinginprod,
sontrinh16 and
facundomedica
as code owners
| FeegrantKeeper: in.FeeGrantKeeper, | ||
| SigGasConsumer: ante.DefaultSigVerificationGasConsumer, | ||
| UnorderedTxManager: in.UnorderedTxManager, | ||
| AccountAbstractionKeeper: in.AccountAbstractionKeeper, |
![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 (5)
x/accounts/testing/account_abstraction/minimal.go (2)
78-80: LGTM! Clean dependency injection implementation.The
ProvideAccountfunction follows the dependency injection pattern correctly by registering the account constructor with a clear identifier.Consider documenting the "aa_minimal" identifier's significance in the function comment to help other developers understand its purpose in the DI container.
Missing test coverage for minimal account abstraction implementation
The MinimalAbstractedAccount implementation lacks dedicated test coverage. While it's used in simapp and there are general authentication tests in the base package, this testing utility requires its own test suite to ensure:
- Proper initialization and dependency injection via
ProvideAccount- Authentication flow with sequence increment
- Event emission during authentication
- Handler registration functionality
Key areas needing tests:
x/accounts/testing/account_abstraction/minimal_test.goshould be created- Integration tests for
aa_minimalaccount type in simapp- Verification of interface compliance beyond the compile-time check
🔗 Analysis chain
Line range hint
1-80: Verify test coverage for the minimal implementation.Since this is a testing utility, it's crucial to ensure it has proper test coverage, especially for the new dependency injection functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test files and coverage # Test 1: Look for corresponding test files echo "Checking for test files..." fd -e go --full-path ".*minimal_test\.go$" x/accounts/testing/account_abstraction/ # Test 2: Check if the new ProvideAccount function is tested echo "Checking for ProvideAccount tests..." rg -l "TestProvideAccount|TestMinimalAbstractedAccount.*Provide" x/accounts/testing/account_abstraction/Length of output: 337
Script:
#!/bin/bash # Let's try a broader search to understand the testing context # Check for any test files in the directory echo "All test files in account_abstraction directory:" fd -e go "test" x/accounts/testing/account_abstraction/ # Look for any tests that might reference MinimalAbstractedAccount echo -e "\nReferences to MinimalAbstractedAccount in test files:" rg "MinimalAbstractedAccount" --type go -C 2 x/accounts/testing/account_abstraction/ # Check if this is used in integration tests elsewhere echo -e "\nUsage in other test files:" rg "aa_minimal" --type go -C 2Length of output: 6060
Script:
#!/bin/bash # Let's check for integration tests in simapp since we found usage there echo "Checking simapp tests for MinimalAbstractedAccount usage:" rg -C 3 "aa_minimal|MinimalAbstractedAccount" simapp/**/*_test.go # Let's also check for any test files that might test the account interface implementation echo -e "\nChecking for account interface tests:" rg -C 3 "TestAccount.*Interface|account\.Interface" --type go x/accounts/ # Check for any authentication related tests echo -e "\nChecking for authentication related tests:" rg -C 3 "TestAuthenticate|authentication.*test" --type go x/accounts/Length of output: 1000
tests/integration/accounts/base_account_test.go (2)
103-114: LGTM: Helper functions are well-structured.The helper functions follow good testing practices with proper error handling and test helper marking. However, consider enhancing
setupAppto support different test scenarios.Consider making setupApp more flexible:
-func setupApp(t *testing.T) *simapp.SimApp { +func setupApp(t *testing.T, opts ...func(*simapp.SimApp)) *simapp.SimApp { t.Helper() app := simapp.Setup(t, false) + for _, opt := range opts { + opt(app) + } return app }This would allow tests to customize the app setup as needed:
// Example usage: app := setupApp(t, func(app *simapp.SimApp) { // Custom setup logic })
Test coverage needs improvement for base account functionality
The current test implementation only covers the basic happy path scenario. Critical test cases are missing:
- No validation of error cases (e.g., invalid public key format, insufficient funds)
- No tests for edge cases (e.g., zero amount transfers, sequence number overflow)
- No tests for account reinitialization attempts
- No tests for invalid message handling or signature verification failures
- No tests for account state queries and validation
Suggested locations to add tests:
tests/integration/accounts/base_account_test.go: Add new test functions for each scenario- Consider splitting into multiple test files for better organization of different test categories
🔗 Analysis chain
Line range hint
28-48: Enhance test coverage with additional scenarios.While the current test covers the happy path, consider adding test cases for:
- Error scenarios (invalid public key, insufficient funds)
- Edge cases (zero amounts, max uint64 sequence numbers)
- Account reinitialization attempts
- Invalid message handling
Let's check the current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for account-related functionality fd -e go . | grep -i "account.*test" | xargs -I {} sh -c 'echo "Coverage for {}:"; go test -coverprofile=coverage.out {} && go tool cover -func=coverage.out'Length of output: 4209
Script:
#!/bin/bash # Let's check the test files content to analyze coverage rg -A 5 "func Test.*Account" tests/integration/accounts/base_account_test.go # Check for error test cases rg -l "require\.Error|assert\.Error" tests/integration/accounts/ # Look for edge case tests rg "TestBaseAccount.*Edge|TestBaseAccount.*Invalid" tests/integration/accounts/ # Check for reinitialization tests rg "reinit|reinitialization" tests/integration/accounts/base_account_test.goLength of output: 543
Script:
#!/bin/bash # Let's check the entire base_account_test.go content cat tests/integration/accounts/base_account_test.go # Also check for any other test files that might test base account functionality rg -l "baseaccountv1" tests/integration/accounts/Length of output: 3438
x/accounts/testing/counter/counter.go (1)
Line range hint
1-155: Consider adding package documentation.While the implementation is solid, the package would benefit from a package-level documentation comment (doc.go or at the top of this file) explaining:
- The purpose of this test package
- Usage examples
- Relationship with the account abstraction system
Example addition:
// Package counter implements a test account type that maintains // a simple counter value. It serves as an example implementation // of the accountstd.Interface and demonstrates proper usage of // account abstraction patterns.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
simapp/app_di.go(3 hunks)tests/integration/accounts/base_account_test.go(2 hunks)tests/integration/accounts/wiring_test.go(0 hunks)x/accounts/testing/account_abstraction/minimal.go(1 hunks)x/accounts/testing/counter/counter.go(1 hunks)x/validate/depinject.go(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/accounts/wiring_test.go
🧰 Additional context used
📓 Path-based instructions (5)
simapp/app_di.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/accounts/base_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"
x/accounts/testing/account_abstraction/minimal.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/testing/counter/counter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/validate/depinject.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
tests/integration/accounts/base_account_test.go (1)
16-16: LGTM: Import addition is appropriate.
The secp256k1 import is necessary for private key generation used in the test setup.
x/accounts/testing/counter/counter.go (1)
153-155: LGTM! Clean implementation of the dependency injection provider.
The ProvideAccount function follows good practices by:
- Providing a clear, single responsibility
- Using the standard account dependency injection pattern
- Maintaining consistency with the existing codebase structure
x/validate/depinject.go (1)
141-149: Verify the impact on existing transactions.
The addition of AccountAbstractionKeeper to the ante handler options looks good, but we should ensure this change doesn't affect the processing of regular transactions when account abstraction is not in use.
Let's verify the ante handler behavior:
✅ Verification successful
The AccountAbstractionKeeper integration is safely handled
The verification shows that the ante handler properly handles cases when AccountAbstractionKeeper is nil:
- The SigVerificationDecorator explicitly checks
if svd.aaKeeper != nilbefore attempting to use it - Regular transaction processing continues unaffected when AccountAbstractionKeeper is nil
- The keeper is properly initialized as an optional dependency with
optional:"true"tag
This confirms that the addition of AccountAbstractionKeeper won't affect existing transaction processing when account abstraction is not in use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any conditional handling of regular transactions when AccountAbstractionKeeper is nil
# Search for any special handling of nil AccountAbstractionKeeper in ante handler
ast-grep --pattern 'if $keeper == nil {
$$$
}'
# Look for any related test cases
rg -l "TestAnteHandler.*AccountAbstraction"
Length of output: 96
Script:
#!/bin/bash
# Let's check the ante handler implementation to understand how AccountAbstractionKeeper is used
# Search for AccountAbstractionKeeper usage in ante handler
rg -A 5 "AccountAbstractionKeeper"
# Check for any special handling or validation in ante decorators
ast-grep --pattern 'func ($decorator $_) AnteHandle($ctx $_,$tx $_,$simulate bool) (sdk.Context, error) {
$$$
}'
# Look for test files related to ante handler
fd -e go test.*ante
Length of output: 4247
Script:
#!/bin/bash
# Let's check how the SigVerificationDecorator handles nil AccountAbstractionKeeper
# Search for IsAbstractedAccount usage in signature verification
rg -B 5 -A 10 "IsAbstractedAccount"
# Look for signature verification implementation
ast-grep --pattern 'func ($d SigVerificationDecorator) AnteHandle($ctx $_,$tx $_,$simulate bool) (sdk.Context, error) {
$$$
}'
# Check for any nil checks on AccountAbstractionKeeper
rg "if.*aaKeeper.*nil|if.*AccountAbstractionKeeper.*nil"
Length of output: 3708
simapp/app_di.go (3)
24-25: LGTM! Note testing-specific imports.
The new imports are correctly placed and properly grouped with other account-related imports. These are specifically for testing purposes, which aligns with the PR's objective of fixing integration tests.
188-191: LGTM! Testing-only account providers properly isolated.
The new account providers are correctly added and properly marked with a clear comment indicating their testing-only nature. This isolation helps prevent accidental use in production code.
308-316: LGTM! Verify AccountAbstractionKeeper integration.
The AccountAbstractionKeeper is properly added to HandlerOptions and correctly initialized. Let's verify its integration with the ante handler chain.
| var ( | ||
| privKey = secp256k1.GenPrivKey() | ||
| accCreator = []byte("creator") | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test determinism and maintainability.
The current implementation has several potential issues:
- Using
GenPrivKey()at the package level could lead to non-deterministic tests. Consider moving key generation into a test setup function with a fixed seed. - The magic string "creator" should be documented or defined as a named constant.
- Global variables can make tests harder to maintain and understand. Consider using a test fixture struct.
Here's a suggested refactor:
-var (
- privKey = secp256k1.GenPrivKey()
- accCreator = []byte("creator")
-)
+const (
+ testCreatorID = "creator"
+)
+
+type testFixture struct {
+ privKey cryptotypes.PrivKey
+ accCreator []byte
+}
+
+func setupTestFixture(t *testing.T) *testFixture {
+ t.Helper()
+ return &testFixture{
+ privKey: secp256k1.GenPrivKeyFromSecret([]byte("fixed_test_seed")),
+ accCreator: []byte(testCreatorID),
+ }
+}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ( | |
| privKey = secp256k1.GenPrivKey() | |
| accCreator = []byte("creator") | |
| ) | |
| const ( | |
| testCreatorID = "creator" | |
| ) | |
| type testFixture struct { | |
| privKey cryptotypes.PrivKey | |
| accCreator []byte | |
| } | |
| func setupTestFixture(t *testing.T) *testFixture { | |
| t.Helper() | |
| return &testFixture{ | |
| privKey: secp256k1.GenPrivKeyFromSecret([]byte("fixed_test_seed")), | |
| accCreator: []byte(testCreatorID), | |
| } | |
| } |
(cherry picked from commit 78f08e8)
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
Description
ref: #22364 (comment)
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
Chores