test: cover ante routing and upgrade handlers v5-v10#128
Conversation
📝 WalkthroughWalkthroughIntroduces a DI-style ante router that dispatches to Cosmos or EVM ante handlers based on tx extension options, adds unit tests covering routing and error cases, adds multiple upgrade migration unit tests, and narrows coverage/test exclusions to only ChangesCoverage & Build
Ante Handler Routing
Upgrade Handlers — Tests
Sequence Diagram(s)sequenceDiagram
participant Submitter as TxSubmitter
participant Router as AnteRouter
participant Cosmos as CosmosAnteHandler
participant EVM as EVMAnteHandler
Submitter->>Router: Submit sdk.Tx
Router->>Router: if tx == nil -> return ErrUnknownRequest
Router->>Router: extract extension options (opts)
alt no opts
Router->>Cosmos: invoke CosmosAnteHandler(ctx, options)
Cosmos-->>Router: result
Router-->>Submitter: return result
else opts present
Router->>Router: switch on opts[0].TypeUrl
alt TypeUrl == /cosmos.evm.vm.v1.ExtensionOptionsEthereumTx
Router->>EVM: invoke EVMAnteHandler(ctx, options)
EVM-->>Router: result
Router-->>Submitter: return result
else TypeUrl == /cosmos.evm.ante.v1.ExtensionOptionDynamicFeeTx
Router->>Cosmos: invoke CosmosAnteHandler(ctx, options)
Cosmos-->>Router: result
Router-->>Submitter: return result
else
Router-->>Submitter: return ErrUnknownExtensionOptions
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/ante/ante.go (1)
55-61: Consider simplifying the default Cosmos path check.The type switch on
sdk.Txis effectively a nil/non-nil gate here; an explicit nil check would be clearer.♻️ Optional clarity refactor
- // handle as totally normal Cosmos SDK tx - switch tx.(type) { - case sdk.Tx: - anteHandler = cosmosHandler(ctx, options) - default: - return ctx, errorsmod.Wrapf(errortypes.ErrUnknownRequest, "invalid transaction type: %T", tx) - } + // handle as totally normal Cosmos SDK tx + if tx == nil { + return ctx, errorsmod.Wrapf(errortypes.ErrUnknownRequest, "invalid transaction type: %T", tx) + } + anteHandler = cosmosHandler(ctx, options)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/ante/ante.go` around lines 55 - 61, The type switch around sdk.Tx is just checking for nil; replace it with an explicit nil check on tx: if tx == nil then return the same wrapped ErrUnknownRequest error, otherwise set anteHandler = cosmosHandler(ctx, options). Update the branch that currently uses "case sdk.Tx:" to the else path and keep the return error message identical when tx is nil; reference symbols: tx, sdk.Tx, anteHandler, cosmosHandler, ctx, options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/ante/ante.go`:
- Around line 55-61: The type switch around sdk.Tx is just checking for nil;
replace it with an explicit nil check on tx: if tx == nil then return the same
wrapped ErrUnknownRequest error, otherwise set anteHandler = cosmosHandler(ctx,
options). Update the branch that currently uses "case sdk.Tx:" to the else path
and keep the return error message identical when tx is nil; reference symbols:
tx, sdk.Tx, anteHandler, cosmosHandler, ctx, options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab52f4ab-9a6c-4c82-a2fc-f43c02b747b4
📒 Files selected for processing (4)
.testcoverage.ymlMakefileapp/ante/ante.goapp/ante/ante_test.go
kpitapeersyst
force-pushed
the
test/ante-handler-routing
branch
from
2281e5d to
394e196
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/ante/ante.go`:
- Around line 45-56: Validate the entire opts slice and guard against nil before
accessing opts[0]: first ensure opts != nil and len(opts) > 0 (or return ctx
with ErrUnknownExtensionOptions), then iterate over all extension options (for
_, o := range opts) and call o.GetTypeUrl() to validate each type URL, rejecting
any unknown/critical option using errorsmod.Wrapf with
errortypes.ErrUnknownExtensionOptions; only after full validation decide routing
(use the first known type URL to pick between evmHandler(ctx, options) and
cosmosHandler(ctx, options) as the existing logic does) so no non-first options
can bypass validation and no nil deref occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff96b40b-48d1-40a8-a966-0c58757bb03f
📒 Files selected for processing (2)
app/ante/ante.goapp/ante/ante_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- app/ante/ante_test.go
| switch typeURL := opts[0].GetTypeUrl(); typeURL { | ||
| case "/cosmos.evm.vm.v1.ExtensionOptionsEthereumTx": | ||
| // handle as *evmtypes.MsgEthereumTx | ||
| anteHandler = evmHandler(ctx, options) | ||
| case "/cosmos.evm.ante.v1.ExtensionOptionDynamicFeeTx": | ||
| // cosmos-sdk tx with dynamic fee extension | ||
| anteHandler = cosmosHandler(ctx, options) | ||
| default: | ||
| return ctx, errorsmod.Wrapf(errortypes.ErrUnknownRequest, "invalid transaction type: %T", tx) | ||
| return ctx, errorsmod.Wrapf( | ||
| errortypes.ErrUnknownExtensionOptions, | ||
| "rejecting tx with unsupported extension option: %s", typeURL, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Cosmos SDK v0.53.x, can a transaction include multiple TxBody.extension_options, and what is the expected validation behavior for unknown options beyond index 0?
💡 Result:
Yes, a transaction can include multiple TxBody.extension_options. The TxBody protobuf message defines extension_options as a repeated google.protobuf.Any extension_options = 1023;, allowing zero or more elements. For validation of unknown options: extension_options (field 1023) are critical (bit 11 not set, as 1023 & 2048 == 0). Unknown fields in critical ranges, including unknown Any types packed into extension_options, cause the transaction to be rejected during decoding via the TxDecoder, which uses unknown field rejection. The comment in the proto states: "If any of these are present and can’t be handled, the transaction will be rejected". non_critical_extension_options (field 2047) are for options that can be safely ignored if unknown (bit 11 set). The query specifies extension_options, which are the critical ones requiring handling or rejection. This behavior is consistent in v0.53.x, as the proto definition remains the same from v0.50 to v0.53, and unknown field rejection is enforced in TxDecoder and ante handlers.
Citations:
- 1: https://docs.rs/cosmos-sdk-proto/0.26.1/cosmos_sdk_proto/cosmos/tx/v1beta1/struct.TxBody.html
- 2: https://github.com/cosmos/cosmos-sdk/blob/v0.50.10/proto/cosmos/tx/v1beta1/tx.proto
- 3: https://docs.cosmos.network/v0.53/build/architecture/adr-020-protobuf-transaction-encoding
- 4: Reject unknown fields in TxDecoder and sign mode handlers cosmos/cosmos-sdk#6883
- 5: Protobuf unknown field rejection cosmos/cosmos-sdk#6192
- 6: https://github.com/cosmos/cosmos-sdk/blob/main/codec/unknownproto/unknown_fields.go
- 7: https://github.com/cosmos/cosmos-sdk/blob/v0.53.0/x/auth/tx/builder.go
🏁 Script executed:
# First, let's see the full function context around lines 45-56
cat -n app/ante/ante.go | head -100Repository: xrplevm/node
Length of output: 2412
🏁 Script executed:
# Also check if there are any guards or length checks before line 45
cat -n app/ante/ante.go | grep -A 50 "opts\[0\]" | head -60Repository: xrplevm/node
Length of output: 709
🏁 Script executed:
# Check test file for coverage patterns
cat -n app/ante/ante_test.go | head -150Repository: xrplevm/node
Length of output: 4323
🏁 Script executed:
# Search for how evmHandler and cosmosHandler handle extension options
rg -A 20 "func newMonoEVMAnteHandler|func newCosmosAnteHandler" app/ante/ante.goRepository: xrplevm/node
Length of output: 38
🏁 Script executed:
# Check if there's any validation of extension options in handlers
rg "GetExtensionOptions|extension_options" app/ante/ -A 3Repository: xrplevm/node
Length of output: 424
🏁 Script executed:
# Look for any nil pointer protection patterns in the codebase
rg "opts\[0\].*nil|== nil" app/ante/Repository: xrplevm/node
Length of output: 90
🏁 Script executed:
# Check what files exist in app/ante
ls -la app/ante/Repository: xrplevm/node
Length of output: 418
🏁 Script executed:
# Search for handler definitions in the entire codebase
find . -name "*.go" -type f -exec grep -l "newMonoEVMAnteHandler\|newCosmosAnteHandler" {} \;Repository: xrplevm/node
Length of output: 130
🏁 Script executed:
# Check if these handlers are external (imported from baseevmante)
rg "newMonoEVMAnteHandler|newCosmosAnteHandler" --type goRepository: xrplevm/node
Length of output: 622
🏁 Script executed:
# Check the evm_handler to see if it validates extension options
cat -n app/ante/evm_handler.goRepository: xrplevm/node
Length of output: 1025
🏁 Script executed:
# Check the cosmos_handler to see if it validates extension options
cat -n app/ante/cosmos_handler.goRepository: xrplevm/node
Length of output: 2943
🏁 Script executed:
# Check what ExtensionOptionsDecorator does - search for its usage pattern
rg "ExtensionOptionsDecorator" --type go -B 2 -A 5Repository: xrplevm/node
Length of output: 630
🏁 Script executed:
# Verify the imports to understand where ExtensionOptionChecker comes from
rg "ExtensionOptionChecker" --type go -B 2 -A 2Repository: xrplevm/node
Length of output: 1034
🏁 Script executed:
# Check what HasDynamicFeeExtensionOption does
rg "HasDynamicFeeExtensionOption" --type go -B 3 -A 10Repository: xrplevm/node
Length of output: 1750
🏁 Script executed:
# Look for the antetypes package
find . -name "*.go" -type go -exec grep -l "HasDynamicFeeExtensionOption" {} \;Repository: xrplevm/node
Length of output: 91
🏁 Script executed:
# Check the protobuf definition to understand if pointers in a slice can be nil after len check
rg "codectypes.Any" --type go | head -20Repository: xrplevm/node
Length of output: 770
🏁 Script executed:
# Check if there's a way opts[0] can be nil after len check
# Understand the structure better - is it possible for a pointer in a slice to be nil?
python3 << 'EOF'
# In Go, []*T is a slice of pointers. len() checks slice length, not pointer validity.
# Example: make([]*int, 1) creates a slice with one element that is nil by default
print("In Go, a slice like []*codectypes.Any after len(opts) == 0 check")
print("can still have nil pointers at individual indices.")
print("")
print("So opts[0] CAN be nil even when len(opts) > 0")
print("This would cause a panic on opts[0].GetTypeUrl()")
EOFRepository: xrplevm/node
Length of output: 268
🏁 Script executed:
# Check if there are actual scenarios with multiple extension options
rg "extension_options.*=" --type go -B 2 -A 2 | head -40Repository: xrplevm/node
Length of output: 38
🏁 Script executed:
# Final verification: check if EVM handler validates extension options via any decorator
cat -n app/ante/evm_handler.goRepository: xrplevm/node
Length of output: 1025
Validate all extension options before routing and guard nil pointer dereference.
On Line 45, opts[0] is dereferenced without a nil check, risking a panic. More critically, the routing logic only validates the first extension option—any unsupported options at indices 1+ bypass validation entirely. Since the EVM handler chain does not include an ExtensionOptionsDecorator, selecting it leaves non-first options unvalidated. Per Cosmos SDK spec, unknown critical extension options must be rejected, but options beyond index 0 silently pass through.
Suggested fix
var anteHandler sdk.AnteHandler
+ if opts[0] == nil {
+ return ctx, errorsmod.Wrap(
+ errortypes.ErrUnknownExtensionOptions,
+ "rejecting tx with nil extension option",
+ )
+ }
+
+ // Validate all extension options before routing.
+ for _, opt := range opts {
+ switch opt.GetTypeUrl() {
+ case "/cosmos.evm.vm.v1.ExtensionOptionsEthereumTx", "/cosmos.evm.ante.v1.ExtensionOptionDynamicFeeTx":
+ // supported
+ default:
+ return ctx, errorsmod.Wrapf(
+ errortypes.ErrUnknownExtensionOptions,
+ "rejecting tx with unsupported extension option: %s", opt.GetTypeUrl(),
+ )
+ }
+ }
+
switch typeURL := opts[0].GetTypeUrl(); typeURL {
case "/cosmos.evm.vm.v1.ExtensionOptionsEthereumTx":
// handle as *evmtypes.MsgEthereumTx
anteHandler = evmHandler(ctx, options)
case "/cosmos.evm.ante.v1.ExtensionOptionDynamicFeeTx":
// cosmos-sdk tx with dynamic fee extension
anteHandler = cosmosHandler(ctx, options)
- default:
- return ctx, errorsmod.Wrapf(
- errortypes.ErrUnknownExtensionOptions,
- "rejecting tx with unsupported extension option: %s", typeURL,
- )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/ante/ante.go` around lines 45 - 56, Validate the entire opts slice and
guard against nil before accessing opts[0]: first ensure opts != nil and
len(opts) > 0 (or return ctx with ErrUnknownExtensionOptions), then iterate over
all extension options (for _, o := range opts) and call o.GetTypeUrl() to
validate each type URL, rejecting any unknown/critical option using
errorsmod.Wrapf with errortypes.ErrUnknownExtensionOptions; only after full
validation decide routing (use the first known type URL to pick between
evmHandler(ctx, options) and cosmosHandler(ctx, options) as the existing logic
does) so no non-first options can bypass validation and no nil deref occurs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/upgrades/v10/upgrades_test.go (1)
20-23: ⚡ Quick win
initErris dead mock state — add an error-path test.
initErrwas evidently intended to support a test whereInitEvmCoinInforeturns an error, but no such test exists. The error-propagation branch of the v10 upgrade handler is therefore untested, andinitErris effectively dead code.✅ Suggested additional test case
+func TestCreateUpgradeHandlerPropagatesInitEvmCoinInfoError(t *testing.T) { + ctx := sdk.NewContext(nil, cmtproto.Header{}, false, log.NewNopLogger()) + moduleManager := module.NewManager() + configurator := module.NewConfigurator( + codec.NewProtoCodec(codectypes.NewInterfaceRegistry()), + grpc.NewServer(), + grpc.NewServer(), + ) + initErr := errors.New("init coin info failed") + keeper := &mockEvmKeeper{initErr: initErr} + versions := module.VersionMap{"bank": 1} + + _, err := CreateUpgradeHandler(moduleManager, configurator, keeper)( + ctx, + upgradetypes.Plan{Name: UpgradeName}, + versions, + ) + + require.ErrorIs(t, err, initErr) + require.True(t, keeper.initCalled) +}Also applies to: 28-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/upgrades/v10/upgrades_test.go` around lines 20 - 23, Add a test that exercises the error path by configuring mockEvmKeeper with initErr set to a non-nil error and verifying the v10 upgrade handler returns that error; update or add a test that calls the v10 upgrade handler (the function that invokes mockEvmKeeper.InitEvmCoinInfo), assert the handler returns the same error and that mockEvmKeeper.initCalled is true, and implement mockEvmKeeper.InitEvmCoinInfo to return initErr so the error branch is actually covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/upgrades/v9/upgrades_test.go`:
- Around line 221-228: The test helper setLegacyEvmParams writes legacy params
to evmtypes.KeyPrefixParams but the real legacy Ethermint stored params in a
params.Subspace key; update the migration MigrateEvmModule to read legacy params
from the correct params subspace key used by the legacy Ethermint EVM (instead
of evmtypes.KeyPrefixParams), and modify setLegacyEvmParams to write the same
subspace key so tests mirror production; locate usages of MigrateEvmModule and
setLegacyEvmParams and replace reads/writes of evmtypes.KeyPrefixParams with the
legacy params subspace key (the key name used by the legacy Ethermint
params.Subspace for the evm module).
---
Nitpick comments:
In `@app/upgrades/v10/upgrades_test.go`:
- Around line 20-23: Add a test that exercises the error path by configuring
mockEvmKeeper with initErr set to a non-nil error and verifying the v10 upgrade
handler returns that error; update or add a test that calls the v10 upgrade
handler (the function that invokes mockEvmKeeper.InitEvmCoinInfo), assert the
handler returns the same error and that mockEvmKeeper.initCalled is true, and
implement mockEvmKeeper.InitEvmCoinInfo to return initErr so the error branch is
actually covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e88adaf-7efc-49ec-aa74-db10d68cef36
📒 Files selected for processing (8)
.testcoverage.ymlMakefileapp/upgrades/v10/upgrades_test.goapp/upgrades/v5/upgrades_test.goapp/upgrades/v6/upgrades_test.goapp/upgrades/v7/upgrades_test.goapp/upgrades/v8/upgrades_test.goapp/upgrades/v9/upgrades_test.go
✅ Files skipped from review due to trivial changes (1)
- Makefile
There was a problem hiding this comment.
We should not modify the ante handler since it is copy-pasted from cosmos/evm
2 participants
test: cover ante routing and upgrade handlers v5-v10
Motivation 💡
The
app/anteandapp/upgradespackages were globally excluded from coverage and had no unit tests.NewAnteHandlerconstructed its cosmos and EVM sub-handlers inline, so routing decisions could not be asserted without spinning up the full ante chain, and every upgrade handler shipped untested. This PR brings both packages under coverage tooling and adds focused tests for the routing logic and each upgrade migration from v5 through v10.Changes 🛠
app/ante/ante.gointo ananteHandlerFactorytype and a privatenewAnteHandler(options, cosmosHandler, evmHandler), with the publicNewAnteHandlerwiring in the realnewCosmosAnteHandler/newMonoEVMAnteHandlerfactories.app/ante/ante_test.gocovering routing forExtensionOptionsEthereumTx,ExtensionOptionDynamicFeeTx, unknown extension options, nil transactions, and the default Cosmos SDK path, using stub factories that return sentinel errors to identify the selected branch.app/upgrades/<v>/upgrades_test.go, asserting migration behavior and store changes for each upgrade..testcoverage.ymlexclusion from the wholeapptree to just^app/app\.go$, soapp/anteand allapp/upgrades/*subpackages are now measured.EXCLUDED_UNIT_PACKAGESinMakefileto anchor the exclusion (/app$$) somake test-unitruns the new tests inapp/anteandapp/upgrades.