chore: remove baseapp from x/accounts
#23355
Conversation
github-actions
bot
added
C:x/upgrade
C:x/feegrant
C:x/authz
C:x/group
C:x/nft
C:x/consensus
C:x/circuit
C:server/v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
go.mod (1)
Line range hint
1-54: Security: Update golang.org/x/net to address high severity vulnerability.A high severity vulnerability has been detected in golang.org/x/net@0.30.0 related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63).
Please update the golang.org/x/net dependency to a patched version.
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
🧹 Nitpick comments (3)
core/testing/msgrouter/msgrouter.go (2)
33-35: Add validation in RegisterHandler.Consider adding validation to ensure neither the handler nor typeUrl is nil/empty.
func (rs *RouterService) RegisterHandler(handler routerHandler, typeUrl string) { + if handler == nil { + panic("handler cannot be nil") + } + if typeUrl == "" { + panic("typeUrl cannot be empty") + } rs.handlers[typeUrl] = handler }
37-50: Optimize handler lookup.The handler lookup logic is duplicated between
CanInvokeandInvoke. Consider extracting it to a private method to improve maintainability.+func (rs RouterService) getHandler(typeUrl string) (routerHandler, error) { + handler := rs.handlers[typeUrl] + if handler == nil { + return nil, fmt.Errorf("no handler for typeURL %s", typeUrl) + } + return handler, nil +} func (rs RouterService) CanInvoke(ctx context.Context, typeUrl string) error { - if rs.handlers[typeUrl] == nil { - return fmt.Errorf("no handler for typeURL %s", typeUrl) - } + _, err := rs.getHandler(typeUrl) + return err } func (rs RouterService) Invoke(ctx context.Context, req transaction.Msg) (transaction.Msg, error) { typeUrl := msgTypeURL(req) - if rs.handlers[typeUrl] == nil { - return nil, fmt.Errorf("no handler for typeURL %s", typeUrl) - } - return rs.handlers[typeUrl](ctx, req) + handler, err := rs.getHandler(typeUrl) + if err != nil { + return nil, err + } + return handler(ctx, req) }x/accounts/utils_test.go (1)
76-81: Consider parameterizing the mock balance responseThe current implementation returns a fixed balance of 1000 atoms. Consider making the balance amount configurable to support various test scenarios.
-func Balance(context.Context, transaction.Msg) (transaction.Msg, error) { +func Balance(ctx context.Context, msg transaction.Msg) (transaction.Msg, error) { + req, ok := msg.(*banktypes.QueryBalanceRequest) + if !ok { + return nil, fmt.Errorf("invalid request type: %T", msg) + } return &banktypes.QueryBalanceResponse{Balance: &sdk.Coin{ - Denom: "atom", - Amount: math.NewInt(1000), + Denom: req.Denom, + Amount: math.NewInt(1000), // Consider making this configurable }}, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
client/v2/go.sumis excluded by!**/*.sumcore/testing/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtools/benchmark/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
client/v2/go.mod(2 hunks)core/testing/go.mod(1 hunks)core/testing/msgrouter/msgrouter.go(1 hunks)core/testing/queryclient/queryclient.go(1 hunks)go.mod(1 hunks)server/v2/cometbft/go.mod(1 hunks)simapp/v2/go.mod(2 hunks)tests/go.mod(2 hunks)tests/integration/v2/accounts/fixture_test.go(4 hunks)tests/integration/v2/accounts/lockup/utils.go(4 hunks)tests/integration/v2/accounts/multisig/test_suite.go(4 hunks)tests/integration/v2/auth/app_test.go(4 hunks)tests/integration/v2/distribution/fixture_test.go(4 hunks)tests/integration/v2/gov/common_test.go(4 hunks)tests/integration/v2/gov/keeper/fixture_test.go(4 hunks)tools/benchmark/go.mod(1 hunks)x/accounts/defaults/base/go.mod(1 hunks)x/accounts/defaults/lockup/go.mod(1 hunks)x/accounts/defaults/multisig/go.mod(1 hunks)x/accounts/go.mod(2 hunks)x/accounts/utils_test.go(2 hunks)x/authz/go.mod(2 hunks)x/bank/go.mod(1 hunks)x/bank/keeper/keeper_test.go(3 hunks)x/circuit/go.mod(1 hunks)x/consensus/go.mod(1 hunks)x/distribution/go.mod(1 hunks)x/epochs/go.mod(1 hunks)x/evidence/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/gov/go.mod(2 hunks)x/group/go.mod(2 hunks)x/mint/go.mod(1 hunks)x/nft/go.mod(1 hunks)x/protocolpool/go.mod(1 hunks)x/slashing/go.mod(1 hunks)x/staking/go.mod(1 hunks)x/upgrade/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- x/bank/go.mod
- x/consensus/go.mod
- x/staking/go.mod
- x/epochs/go.mod
- tools/benchmark/go.mod
- x/distribution/go.mod
- x/nft/go.mod
- x/accounts/defaults/base/go.mod
- x/evidence/go.mod
- x/slashing/go.mod
- x/circuit/go.mod
- x/protocolpool/go.mod
- x/upgrade/go.mod
- x/feegrant/go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- x/accounts/go.mod
- tests/integration/v2/gov/keeper/fixture_test.go
- tests/integration/v2/gov/common_test.go
🧰 Additional context used
📓 Path-based instructions (10)
tests/integration/v2/accounts/fixture_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/v2/auth/app_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/v2/accounts/multisig/test_suite.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"
tests/integration/v2/distribution/fixture_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/v2/accounts/lockup/utils.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/utils_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"
core/testing/msgrouter/msgrouter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
core/testing/queryclient/queryclient.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/bank/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"
📓 Learnings (1)
simapp/v2/go.mod (1)
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#21964
File: simapp/v2/go.mod:49-49
Timestamp: 2024-11-19T11:18:25.531Z
Learning: In `simapp/v2/go.mod`, the project uses `google.golang.org/grpc v1.68.0`, which is valid and acceptable. Do not suggest changing it to a lower version.
🪛 GitHub Actions: Dependency Review
go.mod
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
core/testing/go.mod
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
🪛 GitHub Actions: Lint
x/bank/keeper/keeper_test.go
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Tests / Code Coverage
x/bank/keeper/keeper_test.go
[error] 31-31: Variable 'codec' redeclared in this block
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (23)
core/testing/queryclient/queryclient.go (1)
18-34: LGTM!The changes look good. The GRPCQueryHandler type definition is well-documented and the NewQueryHelper function correctly initializes the QueryHelper with the provided codec.
tests/integration/v2/accounts/lockup/utils.go (1)
82-90: LGTM!The transition from integration.RouterService to msgrouter.RouterService is implemented correctly, with proper initialization and registration of message and query handlers.
tests/integration/v2/accounts/multisig/test_suite.go (3)
9-9: LGTM! Import added for the new msgrouter package.The import aligns with the transition from integration to msgrouter package for router services.
77-84: LGTM! Router service initialization updated to use msgrouter.The change correctly replaces
integration.NewRouterService()withmsgrouter.NewRouterService()for both message and query routers, maintaining the same functionality with the new implementation.
119-119: LGTM! Method signatures updated to use msgrouter.RouterService.The method signatures have been correctly updated to use the new router type from the msgrouter package.
Also applies to: 146-146
x/accounts/defaults/lockup/go.mod (1)
146-146: Verify compatibility with the updated gRPC version.The gRPC dependency has been updated from v1.69.2 to v1.69.4. While this is a minor version update, it's good practice to verify compatibility.
Run this script to check for any breaking changes or security advisories:
x/mint/go.mod (1)
25-25: Same gRPC version update as in other modules.The gRPC dependency has been updated from v1.69.2 to v1.69.4, consistent with other modules.
x/accounts/defaults/multisig/go.mod (1)
154-154: Same gRPC version update as in other modules.The gRPC dependency has been updated from v1.69.2 to v1.69.4, consistent with other modules.
client/v2/go.mod (2)
16-16: Same gRPC version update as in other modules.The gRPC dependency has been updated from v1.69.2 to v1.69.4, consistent with other modules.
168-171: LGTM! Improved organization of replace directives.The replace directives have been restructured into a block format, improving readability and maintainability.
x/authz/go.mod (1)
23-23: LGTM: Dependency update and replace directive changes look good.The changes are consistent with the module's requirements:
- Updated google.golang.org/grpc to v1.69.4
- Added replace directive for core/testing in a standard block format
Also applies to: 171-174
x/group/go.mod (1)
26-26: LGTM: Consistent dependency update and replace directive changes.The changes align with other modules:
- Updated google.golang.org/grpc to v1.69.4
- Added replace directive for core/testing in a standard block format
Also applies to: 175-178
x/gov/go.mod (1)
30-30: LGTM: Dependency update and replace directive changes are consistent.The changes maintain consistency across modules:
- Updated google.golang.org/grpc to v1.69.4
- Added replace directive for core/testing in a standard block format
Also applies to: 175-178
go.mod (1)
54-54: LGTM: gRPC dependency update is consistent.The update to google.golang.org/grpc v1.69.4 aligns with changes across all modules.
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
server/v2/cometbft/go.mod (1)
36-36: LGTM: gRPC dependency update is consistent.The update to google.golang.org/grpc v1.69.4 maintains consistency with other modules.
tests/go.mod (1)
9-9: LGTM!The dependency updates and replace directives look good:
- New dependencies added for the msgrouter functionality.
- Minor patch version update for gRPC from v1.69.2 to v1.69.4.
- Replace directive added for
cosmossdk.io/core/testing.Also applies to: 13-14, 16-16, 19-20, 23-25, 27-27, 29-29, 31-31, 269-269
simapp/v2/go.mod (1)
246-246: LGTM!The dependency updates and replace directives look good:
- Minor patch version update for gRPC from v1.69.2 to v1.69.4.
- Replace directives added for server v2 integration.
Also applies to: 303-311
x/accounts/utils_test.go (2)
58-62: LGTM: Router service initializationThe transition from baseapp to msgrouter is implemented correctly, with proper handler registration for both message and query routers.
64-73: LGTM: Test environment configurationThe test environment is properly configured with the new router services, maintaining proper error handling and validation.
tests/integration/v2/auth/app_test.go (1)
69-76: LGTM: Router service implementationThe transition to msgrouter is implemented correctly with proper error handling and type assertions in the handlers.
tests/integration/v2/distribution/fixture_test.go (1)
Line range hint
158-161: Verify if query router implementation is neededThe query router implementation is empty. Please verify if this is intentional or if query handlers should be implemented for distribution-related queries.
tests/integration/v2/accounts/fixture_test.go (2)
Line range hint
192-206: LGTM: Comprehensive router service implementationThe message router implementation is thorough with proper error handling and type assertions.
Line range hint
207-222: LGTM: Well-structured query router implementationThe query router implementation properly handles type assertions and error cases.
| @@ -25,9 +28,9 @@ import ( | |||
| banktestutil "cosmossdk.io/x/bank/testutil" | |||
| banktypes "cosmossdk.io/x/bank/types" | |||
|
|
|||
| "github.com/cosmos/cosmos-sdk/codec" | |||
There was a problem hiding this comment.
Remove duplicate import of codec package.
The codec package is already imported on line 13. This duplicate import is causing the pipeline failures.
- "github.com/cosmos/cosmos-sdk/codec"🧰 Tools
🪛 GitHub Actions: Lint
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Tests / Code Coverage
[error] 31-31: Variable 'codec' redeclared in this block
core/testing/go.mod
Outdated
| cosmossdk.io/schema v1.0.0 // indirect | ||
| github.com/golang/protobuf v1.5.4 // indirect | ||
| github.com/google/go-cmp v0.6.0 // indirect | ||
| golang.org/x/net v0.30.0 // indirect |
There was a problem hiding this comment.
Critical: Update golang.org/x/net to patch security vulnerability
The current version (v0.30.0) of golang.org/x/net has a high severity vulnerability (GHSA-w32m-9786-jp63) related to non-linear parsing of case-insensitive content in the html package.
Please update to a patched version of golang.org/x/net.
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
| func Send(context.Context, transaction.Msg) (transaction.Msg, error) { | ||
| return &bankv1beta1.MsgSendResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type assertion and error handling to Send handler
The current implementation lacks proper type assertion and error handling for the input message.
-func Send(context.Context, transaction.Msg) (transaction.Msg, error) {
+func Send(ctx context.Context, msg transaction.Msg) (transaction.Msg, error) {
+ _, ok := msg.(*banktypes.MsgSend)
+ if !ok {
+ return nil, fmt.Errorf("invalid request type: %T", msg)
+ }
return &bankv1beta1.MsgSendResponse{}, nil
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/testing/msgrouter/msgrouter.go (1)
37-50: Consider reducing error handling duplication.The error handling logic for missing handlers is duplicated between
CanInvokeandInvoke. Consider extracting this check into a helper method.+func (rs RouterService) validateHandler(typeUrl string) error { + if rs.handlers[typeUrl] == nil { + return fmt.Errorf("no handler for typeURL %s", typeUrl) + } + return nil +} func (rs RouterService) CanInvoke(ctx context.Context, typeUrl string) error { - if rs.handlers[typeUrl] == nil { - return fmt.Errorf("no handler for typeURL %s", typeUrl) - } + return rs.validateHandler(typeUrl) } func (rs RouterService) Invoke(ctx context.Context, req transaction.Msg) (transaction.Msg, error) { typeUrl := msgTypeURL(req) - if rs.handlers[typeUrl] == nil { - return nil, fmt.Errorf("no handler for typeURL %s", typeUrl) + if err := rs.validateHandler(typeUrl); err != nil { + return nil, err } return rs.handlers[typeUrl](ctx, req) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
client/v2/go.sumis excluded by!**/*.sumcore/testing/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtools/benchmark/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
client/v2/go.mod(2 hunks)core/testing/go.mod(1 hunks)core/testing/msgrouter/msgrouter.go(1 hunks)core/testing/queryclient/queryclient.go(1 hunks)go.mod(1 hunks)server/v2/cometbft/go.mod(1 hunks)simapp/v2/go.mod(2 hunks)tests/go.mod(2 hunks)tests/integration/v2/accounts/fixture_test.go(4 hunks)tests/integration/v2/accounts/lockup/utils.go(4 hunks)tests/integration/v2/accounts/multisig/test_suite.go(4 hunks)tests/integration/v2/auth/app_test.go(4 hunks)tests/integration/v2/distribution/fixture_test.go(4 hunks)tests/integration/v2/gov/common_test.go(4 hunks)tests/integration/v2/gov/keeper/fixture_test.go(4 hunks)tools/benchmark/go.mod(1 hunks)x/accounts/defaults/base/go.mod(1 hunks)x/accounts/defaults/lockup/go.mod(1 hunks)x/accounts/defaults/multisig/go.mod(1 hunks)x/accounts/go.mod(2 hunks)x/accounts/utils_test.go(2 hunks)x/authz/go.mod(2 hunks)x/bank/go.mod(1 hunks)x/bank/keeper/keeper_test.go(3 hunks)x/circuit/go.mod(1 hunks)x/consensus/go.mod(1 hunks)x/distribution/go.mod(1 hunks)x/epochs/go.mod(1 hunks)x/evidence/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/gov/go.mod(2 hunks)x/group/go.mod(2 hunks)x/mint/go.mod(1 hunks)x/nft/go.mod(1 hunks)x/protocolpool/go.mod(1 hunks)x/slashing/go.mod(1 hunks)x/staking/go.mod(1 hunks)x/upgrade/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (16)
- x/slashing/go.mod
- x/upgrade/go.mod
- x/feegrant/go.mod
- server/v2/cometbft/go.mod
- x/distribution/go.mod
- x/staking/go.mod
- tools/benchmark/go.mod
- x/accounts/defaults/multisig/go.mod
- x/accounts/defaults/lockup/go.mod
- x/accounts/defaults/base/go.mod
- x/mint/go.mod
- x/circuit/go.mod
- x/nft/go.mod
- x/epochs/go.mod
- x/evidence/go.mod
- x/protocolpool/go.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- x/accounts/go.mod
- tests/integration/v2/accounts/lockup/utils.go
- tests/integration/v2/gov/keeper/fixture_test.go
- tests/integration/v2/gov/common_test.go
- tests/integration/v2/accounts/multisig/test_suite.go
🧰 Additional context used
📓 Path-based instructions (8)
tests/integration/v2/distribution/fixture_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/v2/auth/app_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/v2/accounts/fixture_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"
core/testing/queryclient/queryclient.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/utils_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"
core/testing/msgrouter/msgrouter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/bank/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"
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
📓 Learnings (1)
simapp/v2/go.mod (1)
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#21964
File: simapp/v2/go.mod:49-49
Timestamp: 2024-11-19T11:18:25.531Z
Learning: In `simapp/v2/go.mod`, the project uses `google.golang.org/grpc v1.68.0`, which is valid and acceptable. Do not suggest changing it to a lower version.
🪛 GitHub Actions: Dependency Review
go.mod
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
core/testing/go.mod
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
🪛 GitHub Actions: Lint
x/bank/keeper/keeper_test.go
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Tests / Code Coverage
x/bank/keeper/keeper_test.go
[error] 31-31: Variable 'codec' redeclared in this block
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (20)
core/testing/msgrouter/msgrouter.go (3)
1-11: Clean and well-organized package structure!The imports are properly organized and include only the necessary dependencies.
13-25: Well-structured type definitions with proper interface validation!The code demonstrates good practices:
- Clear documentation for the
msgTypeURLhelper- Type definitions are concise and purposeful
- Interface compliance is explicitly checked
27-35: Clean constructor and registration implementation!The code follows best practices:
- Proper map initialization in the constructor
- Simple and focused registration method
core/testing/queryclient/queryclient.go (1)
18-20: Good code organization improvement!Moving the
GRPCQueryHandlertype definition to the top of the file improves readability and follows better code organization practices.tests/integration/v2/distribution/fixture_test.go (2)
92-101: Clean transition to the new router service!The initialization of the new
msgrouter.NewRouterService()is properly implemented and integrated with the test fixture.
Line range hint
143-160: Verify if query router service implementation is needed.The
registerQueryRouterServicemethod is empty. Please verify if query routing functionality is required for these tests.✅ Verification successful
Empty query router implementation is acceptable
The empty implementation in
registerQueryRouterServiceis valid as the distribution tests don't require any query routing functionality. Other test suites implement query handlers only when specific to their testing needs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for query handler registrations in test files rg -A 5 "registerQueryRouterService.*\{" "tests/integration/v2/"Length of output: 3072
tests/integration/v2/auth/app_test.go (1)
69-69: LGTM: Clean transition to msgrouter package.The migration from integration.NewRouterService to msgrouter.NewRouterService is well implemented and aligns with the PR objective of removing baseapp dependencies.
Also applies to: 76-76
tests/integration/v2/accounts/fixture_test.go (1)
148-148: LGTM: Consistent implementation of msgrouter.The transition to msgrouter.NewRouterService is consistent with other files and maintains proper test fixture setup.
Also applies to: 155-155
x/consensus/go.mod (1)
22-22: LGTM: Dependency version bump.The update to google.golang.org/grpc v1.69.4 is consistent with the standardization of gRPC versions across modules.
x/bank/go.mod (1)
26-26: Verify gRPC version compatibility and security.The update to
google.golang.org/grpc v1.69.4needs verification for compatibility and security advisories.✅ Verification successful
gRPC version v1.69.4 is verified and secure
The version is the latest release (published 2025-01-13) and is not affected by any known security vulnerabilities. All previously reported security issues were patched in versions prior to v1.64.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the gRPC library. # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "google.golang.org/grpc") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1480
Script:
#!/bin/bash # Check latest gRPC version gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/grpc/grpc-go/releases/latest # Verify if the module exists go list -m -versions google.golang.org/grpcLength of output: 2412
client/v2/go.mod (1)
168-171: Well-organized replace directives.The replace directives are now properly organized in a block format, improving readability and maintainability.
x/authz/go.mod (1)
171-174: Well-structured replace directives with proper relative paths.The replace directives are correctly organized and use proper relative paths for both the core testing module and the cosmos-sdk repository.
x/group/go.mod (1)
169-172: Consistent replace directive structure.The replace directives maintain consistency with other modules in the repository.
x/gov/go.mod (2)
175-178: Properly formatted replace directives.The replace directives follow the established pattern across the repository.
30-30: Verify consistency of gRPC version across modules.Let's ensure all modules are using the same version of gRPC to avoid potential compatibility issues.
go.mod (2)
54-54: LGTM: gRPC version updateThe update to google.golang.org/grpc v1.69.4 is a minor version bump that likely contains bug fixes.
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
54-54:⚠️ Potential issueAddress high severity vulnerability in golang.org/x/net dependency
The pipeline indicates a high severity vulnerability (GHSA-w32m-9786-jp63) in golang.org/x/net@0.30.0 related to non-linear parsing of case-insensitive content.
Let's verify the current version and available updates:
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
tests/go.mod (1)
31-31: LGTM: Consistent gRPC version updateThe update to google.golang.org/grpc v1.69.4 maintains version consistency with the main module.
simapp/v2/go.mod (2)
246-246: LGTM: Consistent gRPC version updateThe update to google.golang.org/grpc v1.69.4 maintains version consistency across modules.
303-311: LGTM: Well-organized replace directivesThe server v2 integration replace directives are properly organized and use correct relative paths.
| @@ -25,9 +28,9 @@ import ( | |||
| banktestutil "cosmossdk.io/x/bank/testutil" | |||
| banktypes "cosmossdk.io/x/bank/types" | |||
|
|
|||
| "github.com/cosmos/cosmos-sdk/codec" | |||
There was a problem hiding this comment.
Fix duplicate import of 'codec' package.
The 'codec' package is imported twice (lines 13 and 31), causing build failures. Remove the duplicate import.
- "github.com/cosmos/cosmos-sdk/codec"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Tests / Code Coverage
[error] 31-31: Variable 'codec' redeclared in this block
core/testing/go.mod
Outdated
| cosmossdk.io/schema v1.0.0 // indirect | ||
| github.com/golang/protobuf v1.5.4 // indirect | ||
| github.com/google/go-cmp v0.6.0 // indirect | ||
| golang.org/x/net v0.30.0 // indirect |
There was a problem hiding this comment.
Critical: Update golang.org/x/net to patch security vulnerability.
The current version (0.30.0) of golang.org/x/net contains a high severity vulnerability (GHSA-w32m-9786-jp63) related to non-linear parsing of case-insensitive content in the HTML package.
Update to the latest patched version:
- golang.org/x/net v0.30.0 // indirect
+ golang.org/x/net v0.31.0 // indirect📝 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.
| golang.org/x/net v0.30.0 // indirect | |
| golang.org/x/net v0.31.0 // indirect |
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
core/testing/msgrouter/msgrouter.go (1)
37-50: Consider extracting the error message construction.The error message construction is duplicated in both
CanInvokeandInvokemethods. Consider extracting it to a helper function to improve maintainability.+func newNoHandlerError(typeUrl string) error { + return fmt.Errorf("no handler for typeURL %s", typeUrl) +} func (rs RouterService) CanInvoke(ctx context.Context, typeUrl string) error { if rs.handlers[typeUrl] == nil { - return fmt.Errorf("no handler for typeURL %s", typeUrl) + return newNoHandlerError(typeUrl) } return nil } func (rs RouterService) Invoke(ctx context.Context, req transaction.Msg) (transaction.Msg, error) { typeUrl := msgTypeURL(req) if rs.handlers[typeUrl] == nil { - return nil, fmt.Errorf("no handler for typeURL %s", typeUrl) + return nil, newNoHandlerError(typeUrl) } return rs.handlers[typeUrl](ctx, req) }x/accounts/utils_test.go (2)
76-80: Consider adding more test cases for different scenarios.While the current implementation works for basic testing, consider adding test cases for:
- Zero balance
- Multiple denominations
- Invalid denominations
83-85: Add error handling scenarios to the Send handler.The current implementation always succeeds without any validation. Consider adding test cases for:
- Invalid sender/recipient addresses
- Insufficient funds
- Invalid denomination
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
client/v2/go.sumis excluded by!**/*.sumcore/testing/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtools/benchmark/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
client/v2/go.mod(2 hunks)core/testing/go.mod(1 hunks)core/testing/msgrouter/msgrouter.go(1 hunks)core/testing/queryclient/queryclient.go(1 hunks)go.mod(1 hunks)server/v2/cometbft/go.mod(1 hunks)simapp/v2/go.mod(2 hunks)tests/go.mod(2 hunks)tests/integration/v2/accounts/fixture_test.go(4 hunks)tests/integration/v2/accounts/lockup/utils.go(4 hunks)tests/integration/v2/accounts/multisig/test_suite.go(4 hunks)tests/integration/v2/auth/app_test.go(4 hunks)tests/integration/v2/distribution/fixture_test.go(4 hunks)tests/integration/v2/gov/common_test.go(4 hunks)tests/integration/v2/gov/keeper/fixture_test.go(4 hunks)tools/benchmark/go.mod(1 hunks)x/accounts/defaults/base/go.mod(1 hunks)x/accounts/defaults/lockup/go.mod(1 hunks)x/accounts/defaults/multisig/go.mod(1 hunks)x/accounts/go.mod(2 hunks)x/accounts/utils_test.go(2 hunks)x/authz/go.mod(2 hunks)x/bank/go.mod(1 hunks)x/bank/keeper/keeper_test.go(3 hunks)x/circuit/go.mod(1 hunks)x/consensus/go.mod(1 hunks)x/distribution/go.mod(1 hunks)x/epochs/go.mod(1 hunks)x/evidence/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/gov/go.mod(2 hunks)x/group/go.mod(2 hunks)x/mint/go.mod(1 hunks)x/nft/go.mod(1 hunks)x/protocolpool/go.mod(1 hunks)x/slashing/go.mod(1 hunks)x/staking/go.mod(1 hunks)x/upgrade/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- go.mod
- server/v2/cometbft/go.mod
- x/bank/go.mod
- x/distribution/go.mod
- tools/benchmark/go.mod
- x/nft/go.mod
- x/slashing/go.mod
- x/feegrant/go.mod
- x/staking/go.mod
- x/accounts/defaults/lockup/go.mod
- x/accounts/defaults/base/go.mod
- x/upgrade/go.mod
- x/circuit/go.mod
- x/consensus/go.mod
- x/protocolpool/go.mod
- x/epochs/go.mod
- x/accounts/defaults/multisig/go.mod
- x/evidence/go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- x/accounts/go.mod
- tests/integration/v2/gov/common_test.go
- tests/integration/v2/accounts/lockup/utils.go
- tests/integration/v2/gov/keeper/fixture_test.go
🧰 Additional context used
📓 Path-based instructions (9)
tests/integration/v2/auth/app_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"
core/testing/queryclient/queryclient.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/accounts/fixture_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/v2/distribution/fixture_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"
core/testing/msgrouter/msgrouter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/accounts/multisig/test_suite.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/bank/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/accounts/utils_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"
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
📓 Learnings (1)
simapp/v2/go.mod (1)
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#21964
File: simapp/v2/go.mod:49-49
Timestamp: 2024-11-19T11:18:25.531Z
Learning: In `simapp/v2/go.mod`, the project uses `google.golang.org/grpc v1.68.0`, which is valid and acceptable. Do not suggest changing it to a lower version.
🪛 GitHub Actions: Lint
x/bank/keeper/keeper_test.go
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Tests / Code Coverage
x/bank/keeper/keeper_test.go
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Dependency Review
core/testing/go.mod
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
- GitHub Check: Summary
🔇 Additional comments (25)
core/testing/msgrouter/msgrouter.go (1)
14-16: LGTM! Clean implementation of msgTypeURL function.The function correctly retrieves the TypeURL using gogoproto's MessageName.
core/testing/queryclient/queryclient.go (1)
18-20: LGTM! Clean type definition and documentation.The GRPCQueryHandler type definition is well-documented and properly placed before its usage.
tests/integration/v2/auth/app_test.go (3)
69-69: LGTM! Clean transition to msgrouter package.The router service initialization has been correctly updated to use
msgrouter.NewRouterService()for both message and query routers.Also applies to: 76-76
Line range hint
107-119: LGTM! Handler registration maintained.The message handler registration is properly maintained with the new router service type, preserving the existing functionality.
Line range hint
122-134: LGTM! Query handler registration preserved.The query handler registration is correctly implemented with the new router service type.
tests/integration/v2/distribution/fixture_test.go (2)
92-92: LGTM! Router service transition implemented correctly.The router service initialization has been properly updated to use
msgrouter.NewRouterService().Also applies to: 99-99
Line range hint
143-156: LGTM! Handler registrations maintained.The message handler registration is properly implemented, and the empty query router registration is acceptable as it's intentionally left empty.
Also applies to: 158-160
tests/integration/v2/accounts/fixture_test.go (2)
148-148: LGTM! Router service transition implemented comprehensively.The router service initialization has been properly updated to use
msgrouter.NewRouterService()for both message and query routers.Also applies to: 155-155
Line range hint
192-206: LGTM! Handlers properly registered with type safety.Both message and query handlers are correctly implemented with proper type checking and error handling.
Also applies to: 207-221
tests/integration/v2/accounts/multisig/test_suite.go (3)
77-77: LGTM! Router service transition implemented correctly.The router service initialization has been properly updated to use
msgrouter.NewRouterService().Also applies to: 84-84
Line range hint
119-144: LGTM! Multiple message handlers properly registered.The message handlers for both bank and accounts modules are correctly implemented with proper type checking.
Line range hint
146-159: LGTM! Query handler properly implemented.The bank balance query handler is correctly implemented with proper type checking.
x/mint/go.mod (1)
25-25: Verify gRPC version compatibility.The update to
google.golang.org/grpc v1.69.4is part of a coordinated version bump across multiple modules. Please ensure this version is compatible with the project's minimum Go version and other dependencies.✅ Verification successful
gRPC version v1.69.4 is compatible with the project requirements.
The version update is part of a coordinated bump across modules and is compatible with:
- Project's minimum gRPC-Go requirement (v1.64.0)
- Go runtime versions in use (1.23.x)
- Existing gRPC usage patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any compatibility issues with the new gRPC version go mod graph | grep "google.golang.org/grpc" | sort -uLength of output: 147
Script:
#!/bin/bash # Find and show contents of all go.mod files echo "=== Contents of go.mod files ===" fd -e mod -x cat {} # Search for gRPC imports and usage echo -e "\n=== gRPC imports and usage ===" rg "google.golang.org/grpc" -l # Look for Go version specifications echo -e "\n=== Go version specifications ===" rg "^go \d+" --type modLength of output: 69537
Script:
#!/bin/bash # Search for gRPC version constraints or requirements echo "=== gRPC version constraints ===" rg -i "require.*grpc.*v1\." -A 3 -B 3 # Look for gRPC configuration code echo -e "\n=== gRPC configuration ===" rg "grpc\.(New|Dial|Server)" -A 2Length of output: 68135
client/v2/go.mod (1)
168-171: LGTM! Replace directive restructuring improves readability.The grouping of replace directives into a block with consistent formatting enhances code organization.
x/authz/go.mod (1)
171-174: LGTM! Replace directive paths are correctly specified.The relative paths in the replace directives are properly formatted and consistent with the project structure.
x/group/go.mod (1)
169-172: LGTM! Replace directive structure matches other modules.The replace directive structure maintains consistency with other module files.
x/gov/go.mod (1)
175-178: LGTM! Replace directives follow the established pattern.The replace directive structure aligns with the project-wide standardization effort.
tests/go.mod (2)
31-31: LGTM: gRPC version update.The update to
google.golang.org/grpc v1.69.4is consistent with similar updates across other modules in the project.
269-269: LGTM: Added replace directive for core/testing.The addition of
cosmossdk.io/core/testing => ../core/testingaligns with the PR's objective of restructuring test-related components.simapp/v2/go.mod (2)
246-246: LGTM: gRPC version update.The update to
google.golang.org/grpc v1.69.4is consistent with the version update in other modules.
303-311: LGTM: Added replace directives for server v2 integration.The replace directives are properly structured and use correct relative paths for local development.
x/accounts/utils_test.go (4)
14-16: LGTM: Import changes align with PR objectives.The new imports appropriately support the transition from baseapp to msgrouter and include necessary dependencies for the updated implementation.
Also applies to: 18-18, 23-23
50-57: LGTM: Comprehensive message type registration.The message registration correctly implements the transaction.Msg interface and includes all necessary bank message types.
58-62: LGTM: Clean router service implementation.The implementation properly separates message and query routing concerns while successfully transitioning away from baseapp.
64-70: LGTM: Well-structured test environment setup.The test environment is properly configured with all necessary components and follows testing best practices.
| @@ -25,9 +28,9 @@ import ( | |||
| banktestutil "cosmossdk.io/x/bank/testutil" | |||
| banktypes "cosmossdk.io/x/bank/types" | |||
|
|
|||
| "github.com/cosmos/cosmos-sdk/codec" | |||
There was a problem hiding this comment.
Remove duplicate codec import.
The github.com/cosmos/cosmos-sdk/codec package is already imported at line 13. This duplicate import causes a compilation error.
- "github.com/cosmos/cosmos-sdk/codec"🧰 Tools
🪛 GitHub Actions: Lint
[error] 31-31: Variable 'codec' redeclared in this block
🪛 GitHub Actions: Tests / Code Coverage
[error] 31-31: Variable 'codec' redeclared in this block
core/testing/go.mod
Outdated
| cosmossdk.io/schema v1.0.0 // indirect | ||
| github.com/golang/protobuf v1.5.4 // indirect | ||
| github.com/google/go-cmp v0.6.0 // indirect | ||
| golang.org/x/net v0.30.0 // indirect |
There was a problem hiding this comment.
Security: Update golang.org/x/net to patch vulnerability.
The current version (v0.30.0) of golang.org/x/net has a high severity vulnerability (GHSA-w32m-9786-jp63) related to non-linear parsing of case-insensitive content in the html package.
Please update to a patched version when available. In the meantime, if the html package is not used, this vulnerability may not affect your application.
🧰 Tools
🪛 GitHub Actions: Dependency Review
[high] Vulnerable dependency: golang.org/x/net@0.30.0 has a high severity vulnerability related to non-linear parsing of case-insensitive content in golang.org/x/net/html (GHSA-w32m-9786-jp63)
core/testing/go.mod
Outdated
|
|
||
| require ( | ||
| cosmossdk.io/core v1.0.0 | ||
| github.com/cometbft/cometbft/api v1.0.0 |
There was a problem hiding this comment.
can we not add this dep however? even if it is the api only, we should try to have core/testing have no dep on cometbft whatsoever.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/testing/queryclient/queryclient.go (1)
39-39: Consider adding parameter validation.The
cdcparameter should be validated to prevent potential nil pointer dereferences in theInvokeandregisterABCIQueryHandlermethods.func NewQueryHelper(cdc encoding.Codec) *QueryHelper { + if cdc == nil { + panic("codec cannot be nil") + } qh := &QueryHelper{
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (26)
client/v2/go.sumis excluded by!**/*.sumcore/testing/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtools/benchmark/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
client/v2/go.mod(2 hunks)core/testing/go.mod(1 hunks)core/testing/queryclient/queryclient.go(4 hunks)go.mod(1 hunks)server/v2/cometbft/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(2 hunks)tools/benchmark/go.mod(1 hunks)x/accounts/defaults/base/go.mod(1 hunks)x/accounts/defaults/lockup/go.mod(1 hunks)x/accounts/defaults/multisig/go.mod(1 hunks)x/accounts/go.mod(2 hunks)x/authz/go.mod(2 hunks)x/bank/go.mod(1 hunks)x/bank/keeper/keeper_test.go(3 hunks)x/circuit/go.mod(1 hunks)x/consensus/go.mod(1 hunks)x/distribution/go.mod(1 hunks)x/epochs/go.mod(1 hunks)x/evidence/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/gov/go.mod(2 hunks)x/group/go.mod(2 hunks)x/mint/go.mod(1 hunks)x/nft/go.mod(1 hunks)x/protocolpool/go.mod(1 hunks)x/slashing/go.mod(1 hunks)x/staking/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (27)
- x/slashing/go.mod
- x/feegrant/go.mod
- x/nft/go.mod
- x/mint/go.mod
- go.mod
- x/epochs/go.mod
- x/distribution/go.mod
- x/consensus/go.mod
- x/accounts/defaults/multisig/go.mod
- x/circuit/go.mod
- x/accounts/defaults/lockup/go.mod
- x/staking/go.mod
- x/evidence/go.mod
- x/protocolpool/go.mod
- x/authz/go.mod
- x/accounts/defaults/base/go.mod
- tools/benchmark/go.mod
- tests/go.mod
- x/bank/go.mod
- simapp/v2/go.mod
- x/accounts/go.mod
- x/gov/go.mod
- server/v2/cometbft/go.mod
- x/group/go.mod
- x/bank/keeper/keeper_test.go
- client/v2/go.mod
- core/testing/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
core/testing/queryclient/queryclient.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
core/testing/queryclient/queryclient.go (2)
17-31: Well-structured mock types with clear documentation!The simplified mock types
QueryRequestandQueryResponseeffectively replace theabcidependencies while maintaining the essential functionality needed for testing.
59-59: Clean implementation with proper error handling!The changes effectively adapt the query handling to use the new mock types while maintaining proper error handling and request/response field mapping.
Also applies to: 108-126
| // QueryRequest is a light mock of cometbft abci.QueryRequest. | ||
| type QueryRequest struct { | ||
| Data []byte | ||
| Height int64 | ||
| } |
There was a problem hiding this comment.
would there be any benefits to including the other fields from abci's QueryRequest type?
There was a problem hiding this comment.
No, this is just mocking the data, and we only use it to marshal the query info these two fields
Description
part of #22903
integrationtest MsgRouter logic tocoretestingso that it can be used as a common function in keeper and integration level tests.baseappimport entirely in `x/accountsAuthor 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
Dependency Updates
google.golang.org/grpcfrom v1.69.2 to v1.69.4 across multiple modulescosmossdk.io/core/testingreplace directive in severalgo.modfilesTesting Infrastructure
msgrouterpackage for message routing in integration testsCode Refactoring
integration.RouterService()withmsgrouter.RouterService()Module Maintenance
go.modfiles