Merged
Conversation
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAnte handlers now accept sdk.Context and baseevmante options; fee-market params and dynamic-fee wiring were centralized. The app switches to axrp denomination constants, restructures genesis into module builders, removes the precompiles factory, adds v9/v10 upgrade flows and keeper interfaces, adjusts CLI/tests, and updates dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as TxSender
participant Ante as NewAnteHandler
participant FeeMarket as FeeMarketKeeper
participant EVMK as EvmKeeper
participant Next as NextHandler
Client->>Ante: Deliver tx
Ante->>Ante: Check extension options (dynamic fee)
Ante->>FeeMarket: GetParams(ctx)
FeeMarket-->>Ante: feemarketParams
Ante->>EVMK: GetParams(ctx)
EVMK-->>Ante: evmParams
Ante->>Ante: Build decorator chain (gas fee, deduct, evm mono)
Ante->>Next: Pass to next handler/decorators
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/app.go (2)
195-213:⚠️ Potential issue | 🔴 CriticalInitialize
EVMMempoolor add nil handling inGetMempool().
EVMMempoolis declared but never initialized in theNew()constructor. The method returns a nil pointer, which violates thesdkmempool.ExtMempoolinterface contract and will panic any caller. Either instantiateEVMMempoolduring app setup or add a safe default fallback inGetMempool(). The FIXME comment on line 196 confirms this is a known gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 195 - 213, GetMempool currently returns the possibly-nil field EVMMempool which causes panics; either initialize EVMMempool in the app constructor (New()) using the appropriate constructor (e.g., evmmempool.New... or the project's EVMMempool initializer) so the field is non-nil, or make GetMempool defensive by checking if app.EVMMempool == nil and returning a safe fallback/no-op implementation that satisfies sdkmempool.ExtMempool (or lazily creating and assigning app.EVMMempool before returning); update references to GetMempool and the New() constructor accordingly.
471-494:⚠️ Potential issue | 🟠 MajorGovKeeper is uninitialized when captured by DefaultStaticPrecompiles.
DefaultStaticPrecompiles(...)is called at line 472 beforeapp.GovKeeperis initialized at line 574. Since GovKeeper is passed by value to the precompiles, they receive and permanently capture a zero-value keeper. This is acknowledged as a temporary workaround in the existing FIX comment. Either initialize GovKeeper earlier or defer precompile registration until after GovKeeper is ready.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 471 - 494, The EVM precompiles are being created with a zero-value app.GovKeeper because DefaultStaticPrecompiles(...) is invoked before app.GovKeeper is initialized; fix this by deferring the precompile registration: call evmkeeper.NewKeeper(...) to create app.EvmKeeper first (without chaining WithStaticPrecompiles), then after app.GovKeeper is assigned initialize the static precompiles by calling app.EvmKeeper = app.EvmKeeper.WithStaticPrecompiles(DefaultStaticPrecompiles(*app.StakingKeeper, app.DistrKeeper, app.BankKeeper, &app.Erc20Keeper, &app.TransferKeeper, app.IBCKeeper.ChannelKeeper, app.GovKeeper, app.SlashingKeeper, appCodec)). Ensure you remove the inline WithStaticPrecompiles chain at creation so DefaultStaticPrecompiles receives the fully-initialized app.GovKeeper.
🧹 Nitpick comments (9)
app/upgrades/v10/upgrades.go (1)
15-15: PreferconstforMainnetChainID.
This is a fixed identifier and doesn’t need to be mutable.♻️ Proposed tweak
-var MainnetChainID = "xrplevm_1440000-1" +const MainnetChainID = "xrplevm_1440000-1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/upgrades/v10/upgrades.go` at line 15, MainnetChainID is declared with var but is a fixed, immutable identifier; change its declaration from var MainnetChainID = "xrplevm_1440000-1" to a constant (const MainnetChainID = "xrplevm_1440000-1") so the symbol MainnetChainID is immutable and the intent is clear (adjust any code that tries to reassign it if necessary).testutil/integration/exrp/integration/clients.go (1)
20-20: Consider adding a fail-fast mechanism for defensive coding.
WhileGetPreciseBankClient()is currently never called and the precisebank keeper does not exist in the application, the pattern of returningnilcould mask issues if the method is accidentally called. The// NOTE: Not neededcomment makes the intent clear, but replacing the nil return with a panic provides better fail-fast behavior and prevents silent nil-dereference errors. This aligns with similar defensive practices already adopted elsewhere in the codebase.Optional: Fail fast if called
func (n *IntegrationNetwork) GetPreciseBankClient() precisebanktypes.QueryClient { - return nil + panic("GetPreciseBankClient is not wired in IntegrationNetwork") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testutil/integration/exrp/integration/clients.go` at line 20, GetPreciseBankClient currently returns nil which can silently hide misuse; update the GetPreciseBankClient function to fail fast by panicking with a clear message (e.g., "GetPreciseBankClient called but precisebank keeper not implemented") instead of returning nil, while preserving the existing NOTE comment so intent remains explicit and callers immediately surface incorrect usage.tests/integration/network.go (1)
82-85: Avoid returning nil from a client accessor.Returning nil can lead to nil‑pointer panics if any test calls this. Consider failing fast or wiring a real client.
🧯 Fail fast suggestion
func (n *Network) GetPreciseBankClient() precisebanktypes.QueryClient { - return nil + panic("GetPreciseBankClient not implemented") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/network.go` around lines 82 - 85, The GetPreciseBankClient method on type Network currently returns nil which can cause nil-pointer panics when tests call it; update Network.GetPreciseBankClient to fail fast or provide a real client: either return a properly initialized precisebanktypes.QueryClient instance or make the accessor panic/raise a test-failing error with a clear message (e.g., "precise bank client not initialized") so callers don't receive nil; locate the Network.GetPreciseBankClient definition and replace the nil return with an initialized client or an explicit fail path that surfaces the misconfiguration immediately.app/app.go (2)
262-274: Guard against a missingevm-chain-idvalue.
cast.ToUint64yields 0 on an unset/invalid option. Consider validating and failing fast (or deriving from chain-id) to avoid an unintended 0 chain ID.🛡️ Suggested guard
- evmChainID := cast.ToUint64(appOpts.Get(srvflags.EVMChainID)) + evmChainID := cast.ToUint64(appOpts.Get(srvflags.EVMChainID)) + if evmChainID == 0 { + panic("evm-chain-id must be set") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 262 - 274, The code in New uses cast.ToUint64(appOpts.Get(srvflags.EVMChainID)) which returns 0 when the option is missing, so add a guard after computing evmChainID to detect an unset/invalid value and fail fast (or derive a value from the chain-id) instead of proceeding with a 0; specifically, after evmChainID is computed in New, check if evmChainID == 0 and then either return an error/panic with a clear message or compute evmChainID from the existing chain-id, ensuring the validation references evmChainID, appOpts.Get(srvflags.EVMChainID) and MakeEncodingConfig so the fix is localized to the New constructor.
954-965: KeepDefaultGenesisin sync with the new genesis builders.
DefaultGenesisstill uses direct EVM/ERC20 defaults, while the new helpers set base denom, preinstalls, and token pairs. Using the helpers here avoids drift in tests/simulations.♻️ Suggested alignment
- evmGenState := evmtypes.DefaultGenesisState() - evmGenState.Params.ActiveStaticPrecompiles = evmtypes.AvailableStaticPrecompiles + evmGenState := NewEVMGenesisState() genesis[evmtypes.ModuleName] = app.AppCodec().MustMarshalJSON(evmGenState) - erc20GenState := erc20types.DefaultGenesisState() + erc20GenState := NewErc20GenesisState() genesis[erc20types.ModuleName] = app.AppCodec().MustMarshalJSON(erc20GenState)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 954 - 965, DefaultGenesis currently constructs EVM and ERC20 genesis by calling evmtypes.DefaultGenesisState() and erc20types.DefaultGenesisState(), which diverges from the new genesis builders that set base denom, preinstalls and token pairs; update DefaultGenesis to use the new genesis builder helpers instead of the direct DefaultGenesisState calls (replace the evmGenState and erc20GenState construction with the corresponding new builder functions), ensure the returned genesis JSON uses app.AppCodec().MustMarshalJSON(...) as before, and make sure the builder results include Params.ActiveStaticPrecompiles, base denom, preinstalled contracts, and ERC20 token pair config so tests/simulations remain in sync.app/simulation_test.go (1)
34-37:SimAppEVMChainIDconstant appears unused.The constant
SimAppEVMChainID = 777is defined but doesn't appear to be used after the refactoring removed EVM chain ID parameters from the app constructor. Consider removing it if it's no longer needed.#!/bin/bash # Check if SimAppEVMChainID is used anywhere rg -n "SimAppEVMChainID" --type=go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/simulation_test.go` around lines 34 - 37, The SimAppEVMChainID constant is defined but appears unused after refactoring; remove the unused constant SimAppEVMChainID (or if it was intended to be kept, reintroduce its use where EVM chain ID is required) — locate the constant declaration of SimAppEVMChainID in simulation_test.go and delete the line `SimAppEVMChainID = 777` (or reintegrate it into the app constructor/test setup if you actually need an EVM chain id).cmd/exrpd/cmd/root.go (2)
386-404: Consolidate app instantiation inappExport.The
newAppinstantiation is duplicated across theif/elsebranches with only minor differences (falsevstruefor the trace parameter and the presence ofLoadHeight). Consider refactoring to reduce duplication.♻️ Suggested refactor
func (a appCreator) appExport( logger log.Logger, db dbm.DB, traceStore io.Writer, height int64, forZeroHeight bool, jailAllowedAddrs []string, appOpts servertypes.AppOptions, modulesToExport []string, ) (servertypes.ExportedApp, error) { chainID, err := getChainIDFromOpts(appOpts) if err != nil { return servertypes.ExportedApp{}, err } + loadLatest := height == -1 + newApp := app.New(logger, db, traceStore, loadLatest, map[int64]bool{}, uint(1), appOpts, baseapp.SetChainID(chainID)) - if height != -1 { - newApp = app.New(logger, db, traceStore, false, map[int64]bool{}, uint(1), appOpts, baseapp.SetChainID(chainID)) - - if err := newApp.LoadHeight(height); err != nil { + if !loadLatest { + if err := newApp.LoadHeight(height); err != nil { return servertypes.ExportedApp{}, err } - } else { - newApp = app.New(logger, db, traceStore, true, map[int64]bool{}, uint(1), appOpts, baseapp.SetChainID(chainID)) } return newApp.ExportAppStateAndValidators(forZeroHeight, jailAllowedAddrs, modulesToExport) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/root.go` around lines 386 - 404, The app instantiation is duplicated; set a trace boolean based on height (e.g., trace := height == -1), call newApp = app.New(logger, db, trace, map[int64]bool{}, uint(1), appOpts, baseapp.SetChainID(chainID)) (keeping the same args and using the existing app.New signature), then if height != -1 call newApp.LoadHeight(height) and handle its error, and finally call newApp.ExportAppStateAndValidators(forZeroHeight, jailAllowedAddrs, modulesToExport); update references to newApp, app.New, LoadHeight and ExportAppStateAndValidators accordingly to remove the duplicated branches.
128-132: Document the fallback chain ID magic number.The fallback value
9999is used whenCosmosChainIDToEvmChainIDfails, but this magic number lacks documentation. Consider extracting it to a named constant with a comment explaining when this fallback is expected to be used and its implications.📝 Suggested improvement
+// DefaultFallbackEVMChainID is used when the chain ID cannot be parsed from +// the Cosmos chain ID format. This typically occurs during initial setup +// before the chain ID is configured. +const DefaultFallbackEVMChainID = 9999 + chainID, err := CosmosChainIDToEvmChainID(initClientCtx.ChainID) if err != nil { - chainID = 9999 + chainID = DefaultFallbackEVMChainID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/root.go` around lines 128 - 132, Extract the magic number 9999 into a named constant (e.g. FALLBACK_EVM_CHAIN_ID = 9999) and add a comment explaining this is a fallback EVM chain ID used when CosmosChainIDToEvmChainID(initClientCtx.ChainID) fails (include when this can happen and any implications for InitAppConfig). Replace the inline 9999 assignment to chainID with the constant and ensure references around CosmosChainIDToEvmChainID and the subsequent InitAppConfig(app.BaseDenom, chainID) call use the new constant.cmd/exrpd/cmd/utils.go (1)
14-31: Edge case: chain IDs with multiple underscores or dashes are not handled gracefully.The parsing logic will reject valid-looking chain IDs like
xrp_evm_12345-1(multiple underscores) orxrplevm_12345-1-hotfix(multiple dashes after the number). While this may be intentional, consider whether such formats could appear in practice.Additionally, consider adding a brief unit test for this function to verify expected behavior with various inputs (valid formats, edge cases, and invalid formats).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/exrpd/cmd/utils.go` around lines 14 - 31, The function CosmosChainIDToEvmChainID fails on IDs with extra underscores or multiple dashes; change the parsing to split on the last underscore (use strings.LastIndex to separate name and numeric segment) and then split the numeric segment on the first dash (use strings.Index or strings.SplitN(segment, "-", 2)) to extract the EVM numeric id, parse that with strconv.ParseUint, and return clearer error messages on invalid format or parse failure; also add unit tests for CosmosChainIDToEvmChainID covering valid cases like "xrp_evm_12345-1", "xrplevm_12345-1-hotfix", names with underscores, and invalid inputs to assert expected errors.
🤖 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 28-33: The switch case uses the wrong protobuf type URL: replace
the string literal "/cosmos.evm.ante.v1.ExtensionOptionDynamicFeeTx" with the
actual registered type URL "/ethermint.types.v1.ExtensionOptionDynamicFeeTx" so
the extension option is recognized; update the case in the same switch where
"/cosmos.evm.vm.v1.ExtensionOptionsEthereumTx" is handled (the block that
assigns anteHandler via newMonoEVMAnteHandler and newCosmosAnteHandler) to use
the corrected type URL.
In `@app/upgrades/v9/upgrades.go`:
- Around line 45-55: The migration calls MigrateEthAccountsToBaseAccounts and
MigrateErc20Module lack error handling; change those helpers to return an error
(e.g., MigrateEthAccountsToBaseAccounts(ctx, accountKeeper, evmKeeper) (error)
and MigrateErc20Module(ctx, storeKeys, erc20Keeper) (error)), update this
upgrade path to capture their returned errors, and handle failures by logging
the error with ctx.Logger().Error and returning or panicking appropriately (or
wrap and return the error from the upgrade function); alternatively, if you
choose not to change signatures, at minimum check for internal failure
indicators from those functions and replace the FIXME with a clear logged error
and deterministic failure behavior.
- Around line 106-110: The FIXME comment above the construction of
evmtypes.Params should be replaced with an explanatory note that discarding
legacyEvmParams.AllowUnprotectedTxs is intentional due to the Cosmos EVM v0.5.0
breaking change; update the comment near params := evmtypes.Params (and next to
the commented AllowUnprotectedTxs line) to say something like
"AllowUnprotectedTxs was removed in v0.5.0; unprotected tx acceptance is now
controlled per-node config" so future readers know this removal is deliberate.
In `@cmd/exrpd/cmd/root.go`:
- Around line 180-183: The code inconsistently calls two different
MustGetDefaultNodeHome implementations; replace the call in rootCmd (currently
using config.MustGetDefaultNodeHome()) with the same local utility function used
elsewhere (MustGetDefaultNodeHome()) or vice versa—pick one canonical
implementation and update references so all callers (e.g., in
cmd/exrpd/cmd/root.go and cmd/exrpd/cmd/utils.go) use that single symbol
(MustGetDefaultNodeHome) to ensure a consistent default node home path across
the codebase.
In `@go.mod`:
- Around line 21-23: The go.mod currently references upstream modules
(github.com/cosmos/cosmos-sdk and github.com/cosmos/evm) while the fork replace
directives are commented out; confirm which source should be authoritative for
this PR and then either uncomment and point the replace directives to the
xrplevm forks (to enable forked fixes like slashing invariants and ERC20
precompile) or remove/commented fork references if upstream already contains the
changes. Specifically, check the replace entries for
github.com/cosmos/cosmos-sdk and add/verify a replace for github.com/cosmos/evm
if needed, then update go.mod so the active replace directives reflect the
chosen fork or upstream versions before merging.
In `@local-node.sh`:
- Around line 61-69: Replace the hardcoded address literal
"ethm1zrxl239wa6ad5xge3gs68rt98227xgnjq0xyw2" in local-node.sh (used in the
bin/exrpd genesis add-genesis-account command) with a value derived from
configuration or a shell variable; e.g., read the eth-m prefix/address from
app/config.go or export it as an environment variable (e.g., ETHM_ADDR) and
reference that variable in the script so the address automatically matches the
configured ethm prefix and can be changed without editing the script.
---
Outside diff comments:
In `@app/app.go`:
- Around line 195-213: GetMempool currently returns the possibly-nil field
EVMMempool which causes panics; either initialize EVMMempool in the app
constructor (New()) using the appropriate constructor (e.g., evmmempool.New...
or the project's EVMMempool initializer) so the field is non-nil, or make
GetMempool defensive by checking if app.EVMMempool == nil and returning a safe
fallback/no-op implementation that satisfies sdkmempool.ExtMempool (or lazily
creating and assigning app.EVMMempool before returning); update references to
GetMempool and the New() constructor accordingly.
- Around line 471-494: The EVM precompiles are being created with a zero-value
app.GovKeeper because DefaultStaticPrecompiles(...) is invoked before
app.GovKeeper is initialized; fix this by deferring the precompile registration:
call evmkeeper.NewKeeper(...) to create app.EvmKeeper first (without chaining
WithStaticPrecompiles), then after app.GovKeeper is assigned initialize the
static precompiles by calling app.EvmKeeper =
app.EvmKeeper.WithStaticPrecompiles(DefaultStaticPrecompiles(*app.StakingKeeper,
app.DistrKeeper, app.BankKeeper, &app.Erc20Keeper, &app.TransferKeeper,
app.IBCKeeper.ChannelKeeper, app.GovKeeper, app.SlashingKeeper, appCodec)).
Ensure you remove the inline WithStaticPrecompiles chain at creation so
DefaultStaticPrecompiles receives the fully-initialized app.GovKeeper.
---
Nitpick comments:
In `@app/app.go`:
- Around line 262-274: The code in New uses
cast.ToUint64(appOpts.Get(srvflags.EVMChainID)) which returns 0 when the option
is missing, so add a guard after computing evmChainID to detect an unset/invalid
value and fail fast (or derive a value from the chain-id) instead of proceeding
with a 0; specifically, after evmChainID is computed in New, check if evmChainID
== 0 and then either return an error/panic with a clear message or compute
evmChainID from the existing chain-id, ensuring the validation references
evmChainID, appOpts.Get(srvflags.EVMChainID) and MakeEncodingConfig so the fix
is localized to the New constructor.
- Around line 954-965: DefaultGenesis currently constructs EVM and ERC20 genesis
by calling evmtypes.DefaultGenesisState() and erc20types.DefaultGenesisState(),
which diverges from the new genesis builders that set base denom, preinstalls
and token pairs; update DefaultGenesis to use the new genesis builder helpers
instead of the direct DefaultGenesisState calls (replace the evmGenState and
erc20GenState construction with the corresponding new builder functions), ensure
the returned genesis JSON uses app.AppCodec().MustMarshalJSON(...) as before,
and make sure the builder results include Params.ActiveStaticPrecompiles, base
denom, preinstalled contracts, and ERC20 token pair config so tests/simulations
remain in sync.
In `@app/simulation_test.go`:
- Around line 34-37: The SimAppEVMChainID constant is defined but appears unused
after refactoring; remove the unused constant SimAppEVMChainID (or if it was
intended to be kept, reintroduce its use where EVM chain ID is required) —
locate the constant declaration of SimAppEVMChainID in simulation_test.go and
delete the line `SimAppEVMChainID = 777` (or reintegrate it into the app
constructor/test setup if you actually need an EVM chain id).
In `@app/upgrades/v10/upgrades.go`:
- Line 15: MainnetChainID is declared with var but is a fixed, immutable
identifier; change its declaration from var MainnetChainID = "xrplevm_1440000-1"
to a constant (const MainnetChainID = "xrplevm_1440000-1") so the symbol
MainnetChainID is immutable and the intent is clear (adjust any code that tries
to reassign it if necessary).
In `@cmd/exrpd/cmd/root.go`:
- Around line 386-404: The app instantiation is duplicated; set a trace boolean
based on height (e.g., trace := height == -1), call newApp = app.New(logger, db,
trace, map[int64]bool{}, uint(1), appOpts, baseapp.SetChainID(chainID)) (keeping
the same args and using the existing app.New signature), then if height != -1
call newApp.LoadHeight(height) and handle its error, and finally call
newApp.ExportAppStateAndValidators(forZeroHeight, jailAllowedAddrs,
modulesToExport); update references to newApp, app.New, LoadHeight and
ExportAppStateAndValidators accordingly to remove the duplicated branches.
- Around line 128-132: Extract the magic number 9999 into a named constant (e.g.
FALLBACK_EVM_CHAIN_ID = 9999) and add a comment explaining this is a fallback
EVM chain ID used when CosmosChainIDToEvmChainID(initClientCtx.ChainID) fails
(include when this can happen and any implications for InitAppConfig). Replace
the inline 9999 assignment to chainID with the constant and ensure references
around CosmosChainIDToEvmChainID and the subsequent InitAppConfig(app.BaseDenom,
chainID) call use the new constant.
In `@cmd/exrpd/cmd/utils.go`:
- Around line 14-31: The function CosmosChainIDToEvmChainID fails on IDs with
extra underscores or multiple dashes; change the parsing to split on the last
underscore (use strings.LastIndex to separate name and numeric segment) and then
split the numeric segment on the first dash (use strings.Index or
strings.SplitN(segment, "-", 2)) to extract the EVM numeric id, parse that with
strconv.ParseUint, and return clearer error messages on invalid format or parse
failure; also add unit tests for CosmosChainIDToEvmChainID covering valid cases
like "xrp_evm_12345-1", "xrplevm_12345-1-hotfix", names with underscores, and
invalid inputs to assert expected errors.
In `@tests/integration/network.go`:
- Around line 82-85: The GetPreciseBankClient method on type Network currently
returns nil which can cause nil-pointer panics when tests call it; update
Network.GetPreciseBankClient to fail fast or provide a real client: either
return a properly initialized precisebanktypes.QueryClient instance or make the
accessor panic/raise a test-failing error with a clear message (e.g., "precise
bank client not initialized") so callers don't receive nil; locate the
Network.GetPreciseBankClient definition and replace the nil return with an
initialized client or an explicit fail path that surfaces the misconfiguration
immediately.
In `@testutil/integration/exrp/integration/clients.go`:
- Line 20: GetPreciseBankClient currently returns nil which can silently hide
misuse; update the GetPreciseBankClient function to fail fast by panicking with
a clear message (e.g., "GetPreciseBankClient called but precisebank keeper not
implemented") instead of returning nil, while preserving the existing NOTE
comment so intent remains explicit and callers immediately surface incorrect
usage.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go.mod`:
- Line 22: go.mod currently references upstream module github.com/cosmos/evm
v0.5.1 but the PR requires using the xrplevm fork; add a replace directive in
go.mod such as replacing github.com/cosmos/evm with the xrplevm fork (e.g.,
replace github.com/cosmos/evm => github.com/xrplevm/evm <fork-version>) so the
build uses the forked code, then run go mod tidy to update the dependency graph
and verify imports referencing github.com/cosmos/evm now resolve to the fork.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/app.go (1)
438-445: Address the TODO comment about IBC subspace removal.The TODO on line 442 questions why the subspace parameter is removed in the upstream
evmdreference. This should be investigated and documented before merging to avoid maintenance confusion.Do you want me to search the upstream
cosmos/evmrepository for context on this change?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 438 - 445, Investigate and resolve the TODO about the removed subspace parameter: check the upstream cosmos/evm (evmd) change that removed passing app.GetSubspace(ibcexported.ModuleName) to ibckeeper.NewKeeper, confirm whether ibckeeper.NewKeeper signature or module parameter handling changed (search for ibckeeper.NewKeeper usages and its constructor definition), then either restore passing app.GetSubspace(ibcexported.ModuleName) to app.IBCKeeper = ibckeeper.NewKeeper(...) if required, or remove the TODO and add a concise comment documenting the upstream rationale (referencing IBCKeeper, ibckeeper.NewKeeper, GetSubspace, and evmd) so future maintainers understand why the subspace is omitted.app/upgrades/v10/upgrades.go (2)
28-43: Inconsistent logger usage.The function initializes a structured logger with upgrade context on line 28, but then switches to
ctx.Logger()directly for subsequent log statements (lines 38-43). Use the structuredloggerconsistently for better log correlation.♻️ Proposed fix for consistent logger usage
} - ctx.Logger().Info("Running v10 upgrade handler...") - ctx.Logger().Info("Init evm coin info...") + logger.Info("Running v10 upgrade handler...") + logger.Info("Init evm coin info...") if err := evmKeeper.InitEvmCoinInfo(ctx); err != nil { return nil, err } - ctx.Logger().Info("Finished v10 upgrade handler") + logger.Info("Finished v10 upgrade handler") return mm.RunMigrations(ctx, configurator, vm)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/upgrades/v10/upgrades.go` around lines 28 - 43, The code creates a structured logger named logger := ctx.Logger().With("upgrade", UpgradeName) but then uses ctx.Logger() for later messages; replace the direct ctx.Logger().Info calls with logger.Info to keep the structured context for all log messages (specifically the "Running v10 upgrade handler...", "Init evm coin info...", and "Finished v10 upgrade handler" messages) and ensure any error logging around evmKeeper.InitEvmCoinInfo and v9.UpgradeHandler also uses logger so the "upgrade" field is consistently attached to all logs.
15-15: Potential duplicate constant declaration.The
MainnetChainIDconstant is defined here, but based on the AI summary,app/upgrades/v10/constants.goalso exports constants for this package. Consider consolidating this constant inconstants.goalongsideUpgradeNamefor consistency and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/upgrades/v10/upgrades.go` at line 15, The MainnetChainID constant declared in upgrades.go duplicates constants stored in constants.go; remove the duplicate from app/upgrades/v10/upgrades.go and consolidate it into app/upgrades/v10/constants.go alongside UpgradeName (or, if it already exists there, delete the one in upgrades.go and import/reference the existing constant), then update any local references to use the single MainnetChainID definition to avoid duplicate declarations.
🤖 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/app.go`:
- Around line 196-199: GetMempool currently returns app.EVMMempool which is
never initialized and will be nil; update the New() constructor (where App is
created) to initialize the EVMMempool field with a proper sdkmempool.ExtMempool
instance (or a safe no-op/mock implementation) so GetMempool always returns a
non-nil mempool, or alternatively modify GetMempool to lazily create and assign
a default implementation to app.EVMMempool on first call; reference symbols:
New(), App struct, EVMMempool, and GetMempool().
---
Nitpick comments:
In `@app/app.go`:
- Around line 438-445: Investigate and resolve the TODO about the removed
subspace parameter: check the upstream cosmos/evm (evmd) change that removed
passing app.GetSubspace(ibcexported.ModuleName) to ibckeeper.NewKeeper, confirm
whether ibckeeper.NewKeeper signature or module parameter handling changed
(search for ibckeeper.NewKeeper usages and its constructor definition), then
either restore passing app.GetSubspace(ibcexported.ModuleName) to app.IBCKeeper
= ibckeeper.NewKeeper(...) if required, or remove the TODO and add a concise
comment documenting the upstream rationale (referencing IBCKeeper,
ibckeeper.NewKeeper, GetSubspace, and evmd) so future maintainers understand why
the subspace is omitted.
In `@app/upgrades/v10/upgrades.go`:
- Around line 28-43: The code creates a structured logger named logger :=
ctx.Logger().With("upgrade", UpgradeName) but then uses ctx.Logger() for later
messages; replace the direct ctx.Logger().Info calls with logger.Info to keep
the structured context for all log messages (specifically the "Running v10
upgrade handler...", "Init evm coin info...", and "Finished v10 upgrade handler"
messages) and ensure any error logging around evmKeeper.InitEvmCoinInfo and
v9.UpgradeHandler also uses logger so the "upgrade" field is consistently
attached to all logs.
- Line 15: The MainnetChainID constant declared in upgrades.go duplicates
constants stored in constants.go; remove the duplicate from
app/upgrades/v10/upgrades.go and consolidate it into
app/upgrades/v10/constants.go alongside UpgradeName (or, if it already exists
there, delete the one in upgrades.go and import/reference the existing
constant), then update any local references to use the single MainnetChainID
definition to avoid duplicate declarations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/upgrades/v9/upgrades.go (1)
42-69: Inconsistent logger usage.A logger with
"upgrade"context is created at line 43 but only used at lines 44 and 67. The intermediate log statements (lines 46, 49, 55, 56, 65) usectx.Logger()directly, losing the upgrade context.♻️ Proposed fix for consistent logger usage
func UpgradeHandler(ctx sdk.Context, storeKeys map[string]*storetypes.KVStoreKey, appCodec codec.Codec, accountKeeper authkeeper.AccountKeeper, evmKeeper EvmKeeper, erc20Keeper ERC20Keeper) error { logger := ctx.Logger().With("upgrade", UpgradeName) logger.Info("Running v9 upgrade handler...") - ctx.Logger().Info("migration EthAccounts to BaseAccounts...") + logger.Info("migrating EthAccounts to BaseAccounts...") MigrateEthAccountsToBaseAccounts(ctx, accountKeeper, evmKeeper) - ctx.Logger().Info("migrating erc20 module...") + logger.Info("migrating erc20 module...") MigrateErc20Module( ctx, storeKeys, erc20Keeper, ) - ctx.Logger().Info("erc20 module migrated successfully") - ctx.Logger().Info("migrating evm module...") + logger.Info("erc20 module migrated successfully") + logger.Info("migrating evm module...") if err := MigrateEvmModule( ctx, storeKeys, appCodec, evmKeeper, ); err != nil { return err } - ctx.Logger().Info("evm module migrated successfully") + logger.Info("evm module migrated successfully") logger.Info("Finished v9 upgrade handler") return nil }Also note there's a typo at line 46:
"migration"should be"migrating".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/upgrades/v9/upgrades.go` around lines 42 - 69, The upgrade handler creates a contextual logger named logger in UpgradeHandler but several intermediate logs call ctx.Logger().Info which loses the "upgrade" context; replace all ctx.Logger().Info(...) calls in UpgradeHandler with logger.Info(...) so every log (e.g., the messages around MigrateEthAccountsToBaseAccounts, MigrateErc20Module, and MigrateEvmModule) uses the same contextual logger, and fix the typo by changing the message "migration EthAccounts to BaseAccounts..." to "migrating EthAccounts to BaseAccounts...".
🤖 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/upgrades/v9/upgrades.go`:
- Around line 147-149: The per-account warning using
ctx.Logger().Warn(fmt.Sprintf("skipping non EthAccount %s",
account.GetAddress().String())) inside ak.IterateAccounts should be removed to
avoid millions of logs; instead, declare a counter before calling
ak.IterateAccounts, increment that counter when the account is not an EthAccount
in the IterateAccounts callback (do not log there), and after iteration emit a
single summary log (e.g., ctx.Logger().Info or Debug) reporting how many
non-EthAccount entries were skipped; update references to EthAccount,
ak.IterateAccounts, account.GetAddress().String(), and ctx.Logger().Warn
accordingly.
---
Duplicate comments:
In `@app/upgrades/v9/upgrades.go`:
- Around line 46-55: Both migration helpers currently swallow failures; update
MigrateEthAccountsToBaseAccounts and MigrateErc20Module to return error (e.g.,
func MigrateEthAccountsToBaseAccounts(ctx ..., ...) error and similar for
MigrateErc20Module), propagate errors up to the caller in upgrades.go, and
handle them where called (check returned error after each call, log a
descriptive error via ctx.Logger().Error and return the error from the outer
upgrade function or abort migration) so failures no longer silently proceed;
also update any callers/tests to handle the new error-returning signatures.
---
Nitpick comments:
In `@app/upgrades/v9/upgrades.go`:
- Around line 42-69: The upgrade handler creates a contextual logger named
logger in UpgradeHandler but several intermediate logs call ctx.Logger().Info
which loses the "upgrade" context; replace all ctx.Logger().Info(...) calls in
UpgradeHandler with logger.Info(...) so every log (e.g., the messages around
MigrateEthAccountsToBaseAccounts, MigrateErc20Module, and MigrateEvmModule) uses
the same contextual logger, and fix the typo by changing the message "migration
EthAccounts to BaseAccounts..." to "migrating EthAccounts to BaseAccounts...".
app/upgrades/v9/upgrades.go
Outdated
Comment on lines
147
to
149
| ctx.Logger().Warn(fmt.Sprintf("skipping non EthAccount %s", account.GetAddress().String())) | ||
| return false | ||
| } |
There was a problem hiding this comment.
Warning log will fire for every non-EthAccount, which is expected behavior.
ak.IterateAccounts iterates over all accounts in the store, including module accounts and standard Cosmos accounts. On a production chain, the vast majority of accounts are NOT EthAccounts—this would produce potentially millions of warning logs during the upgrade, obscuring actual issues.
The message "skipping non EthAccount" implies something unexpected, but non-EthAccounts are normal and expected. Consider removing this log entirely or changing it to Debug level with a counter that logs a summary at the end.
♻️ Proposed fix: aggregate count instead of per-account warning
func MigrateEthAccountsToBaseAccounts(ctx sdk.Context, ak authkeeper.AccountKeeper, ek EvmKeeper) {
+ var migratedCount, skippedCount int
ak.IterateAccounts(ctx, func(account sdk.AccountI) (stop bool) {
ethAcc, ok := account.(*legacytypes.EthAccount)
if !ok {
- ctx.Logger().Warn(fmt.Sprintf("skipping non EthAccount %s", account.GetAddress().String()))
+ skippedCount++
return false
}
// NOTE: we only need to add store entries for smart contracts
codeHashBytes := common.HexToHash(ethAcc.CodeHash).Bytes()
if !evmtypes.IsEmptyCodeHash(codeHashBytes) {
ek.SetCodeHash(ctx, ethAcc.EthAddress().Bytes(), codeHashBytes)
}
// Set the base account in the account keeper instead of the EthAccount
ak.SetAccount(ctx, ethAcc.BaseAccount)
+ migratedCount++
return false
})
+ ctx.Logger().Info(fmt.Sprintf("EthAccount migration complete: migrated=%d, skipped=%d", migratedCount, skippedCount))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/upgrades/v9/upgrades.go` around lines 147 - 149, The per-account warning
using ctx.Logger().Warn(fmt.Sprintf("skipping non EthAccount %s",
account.GetAddress().String())) inside ak.IterateAccounts should be removed to
avoid millions of logs; instead, declare a counter before calling
ak.IterateAccounts, increment that counter when the account is not an EthAccount
in the IterateAccounts callback (do not log there), and after iteration emit a
single summary log (e.g., ctx.Logger().Info or Debug) reporting how many
non-EthAccount entries were skipped; update references to EthAccount,
ak.IterateAccounts, account.GetAddress().String(), and ctx.Logger().Warn
accordingly.
GuillemGarciaDev
suggested changes
Feb 19, 2026
GuillemGarciaDev
approved these changes
Feb 19, 2026
…-for-private-repo feat: add ci cd workflows for private repo
…' into feat/upgrade/bump-package-to-v10
feat: bump package version to v10
feat(upgrade): upgrade to cosmos/evm v0.6.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(upgrade): add v10 upgrade
Summary 💡
This PR includes the changes needed for the upgrade to cosmos/evm v0.5.1
References used:
/evmdfolder: cosmos/evm@v0.4.2...v0.5.1Also changed:
evm-chain-idfrom the cosmos chain idcosmos/evmDefaultGenesisStateto include basic XRP and native coin representationv9.CreateUpgradeHandlerin order to be used inv10for mainnetTo Do 🛠
NOTES 🤔
x/bankSummary by CodeRabbit
New Features
Bug Fixes
Chores