Bump EVM to v0.5.1#180
Conversation
|
Important Review skippedToo many files! 30 files out of 180 files are above the max files limit of 150. You can disable this status message by setting the WalkthroughThis pull request refactors EVM integration and application initialization throughout the codebase. The changes include: passing context and parameter structs to ante handlers instead of raw keeper references; replacing EVMAppOptions with evmChainID derivation via app options; restructuring precompile constructors to accept explicit MsgServer and Querier implementations; updating genesis configuration with denom metadata for the native token; modifying EVM mempool setup from a public method to a private configuration function; updating test helpers to initialize EVM configuration and apply genesis state changes; and adjusting method signatures across the stack to thread parameters through new initialization paths. Dependency versions are updated, including cosmos/evm from v0.4.2 to v0.5.1. Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
app/app.go (1)
138-139: Consider validating evmChainID.
cast.ToUint64will return0if the option is missing or invalid. A chain ID of0may cause unexpected behavior in EVM transaction processing.🔎 Proposed validation
evmChainID := cast.ToUint64(appOpts.Get(srvflags.EVMChainID)) + if evmChainID == 0 { + panic("EVM chain ID must be set and non-zero") + } encodingConfig := evmencoding.MakeConfig(evmChainID)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
CHANGELOG.mdMakefileante/ante.goante/ante_cosmos.goante/ante_evm.goapp/app.goapp/app_test.goapp/config.goapp/evm_mempool.goapp/helpers/test_helpers.goapp/keepers/keepers.goapp/keepers/precompiles.goapp/modules.goapp/params/config.goapp/sim_bench_test.goapp/sim_test.gocmd/kiichaind/cmd/root.gocontrib/local_node.shgo.modprecompiles/oracle/integration_test.goprecompiles/oracle/oracle.goprecompiles/wasmd/integration_test.goprecompiles/wasmd/realworld_attack_test.goprecompiles/wasmd/security_test.goprecompiles/wasmd/tx.goprecompiles/wasmd/tx_test.goprecompiles/wasmd/wasmd.gotests/e2e/chain.gotests/e2e/e2e_setup_test.gotests/integration/create_app.gox/feeabstraction/ante/cosmos/fee_test.gox/feeabstraction/ante/evm/account_verification.gox/feeabstraction/ante/evm/mono_decorator.gox/feeabstraction/ante/evm/mono_decorator_test.go
💤 Files with no reviewable changes (6)
- app/config.go
- app/app_test.go
- tests/integration/create_app.go
- tests/e2e/chain.go
- app/sim_bench_test.go
- app/sim_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-23T19:37:59.730Z
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
Applied to files:
go.mod
📚 Learning: 2025-08-23T19:37:59.730Z
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the current Cosmos SDK version used by the KiiChain codebase, sdk.Context directly implements the context.Context interface and can be passed directly to query methods without needing sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods.
Applied to files:
go.mod
🧬 Code graph analysis (19)
precompiles/oracle/integration_test.go (3)
precompiles/oracle/oracle.go (1)
NewPrecompile(54-66)precompiles/wasmd/wasmd.go (1)
NewPrecompile(56-68)x/feeabstraction/types/expected_keepers.go (1)
OracleKeeper(39-43)
precompiles/wasmd/security_test.go (2)
precompiles/oracle/oracle.go (2)
Precompile(47-51)Precompile(120-123)precompiles/wasmd/wasmd.go (2)
Precompile(49-53)Precompile(134-142)
precompiles/wasmd/tx.go (1)
precompiles/wasmd/wasmd.go (2)
Precompile(49-53)Precompile(134-142)
cmd/kiichaind/cmd/root.go (1)
app/config.go (1)
KiichainID(64-64)
precompiles/wasmd/realworld_attack_test.go (1)
precompiles/wasmd/wasmd.go (2)
Precompile(49-53)Precompile(134-142)
precompiles/wasmd/tx_test.go (1)
precompiles/wasmd/wasmd.go (2)
Precompile(49-53)Precompile(134-142)
x/feeabstraction/ante/cosmos/fee_test.go (1)
x/feeabstraction/ante/cosmos/fee.go (1)
NewDeductFeeDecorator(34-50)
x/feeabstraction/ante/evm/mono_decorator_test.go (1)
x/feeabstraction/ante/evm/mono_decorator.go (1)
NewEVMMonoDecorator(61-80)
app/keepers/keepers.go (2)
app/params/config.go (3)
BaseDenom(33-33)DisplayDenom(31-31)BaseDenomUnit(35-35)app/keepers/precompiles.go (1)
NewAvailableStaticPrecompiles(92-197)
ante/ante_cosmos.go (1)
ante/handler_options.go (1)
HandlerOptions(29-51)
app/evm_mempool.go (1)
app/app.go (1)
KiichainApp(94-114)
app/modules.go (5)
x/oracle/module.go (1)
NewAppModule(114-123)x/rewards/module.go (1)
NewAppModule(123-132)x/tokenfactory/module.go (1)
NewAppModule(124-139)x/feeabstraction/types/expected_keepers.go (1)
BankKeeper(33-36)x/tokenfactory/simulation/operations.go (1)
BankKeeper(47-51)
app/helpers/test_helpers.go (4)
wasmbinding/tokenfactory/types/types.go (2)
Metadata(7-20)DenomUnit(22-33)app/keepers/keepers.go (1)
CoinInfo(101-106)app/params/config.go (1)
DisplayDenom(31-31)app/config.go (1)
KiichainID(64-64)
ante/ante.go (1)
ante/ante_cosmos.go (1)
NewCosmosAnteHandler(24-81)
ante/ante_evm.go (2)
ante/handler_options.go (1)
HandlerOptions(29-51)ante/types/expected_keepers.go (1)
FeeAbstractionKeeper(8-10)
precompiles/wasmd/wasmd.go (4)
precompiles/oracle/oracle.go (4)
ABI(34-34)Precompile(47-51)Precompile(120-123)NewPrecompile(54-66)tests/e2e/e2e_wasmd_precompile_test.go (1)
WasmdPrecompileAddress(17-17)tests/e2e/address.go (1)
Address(37-39)precompiles/wasmd/tx.go (2)
InstantiateMethod(17-17)ExecuteMethod(19-19)
precompiles/wasmd/integration_test.go (2)
precompiles/oracle/oracle.go (3)
NewPrecompile(54-66)Precompile(47-51)Precompile(120-123)precompiles/wasmd/wasmd.go (3)
NewPrecompile(56-68)Precompile(49-53)Precompile(134-142)
precompiles/oracle/oracle.go (1)
precompiles/wasmd/wasmd.go (4)
ABI(36-36)Precompile(49-53)Precompile(134-142)NewPrecompile(56-68)
app/keepers/precompiles.go (7)
precompiles/oracle/oracle.go (1)
NewPrecompile(54-66)precompiles/wasmd/wasmd.go (1)
NewPrecompile(56-68)x/rewards/keeper/msg_server.go (1)
NewMsgServerImpl(21-23)x/tokenfactory/keeper/msg_server.go (1)
NewMsgServerImpl(20-22)x/feeabstraction/keeper/grpc_query.go (1)
NewQuerier(18-20)x/rewards/keeper/grpc_query.go (1)
NewQuerier(15-17)x/oracle/keeper/query_server.go (1)
NewQueryServer(23-27)
🪛 GitHub Actions: Lint
app/evm_mempool.go
[error] 63-63: KiichainApp.createMempoolConfig - result 1 (error) is always nil (unparam)
app/helpers/test_helpers.go
[error] 36-36: gci: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos) -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/CosmWasm/wasmd) -s prefix(github.com/cosmos/evm) -s prefix(github.com/kiichain/kiichain) --custom-order (gci)
x/feeabstraction/ante/evm/mono_decorator.go
[error] 69-69: gofumpt: File is not gofumpt-ed (gofumpt)
🪛 GitHub Check: golangci-lint
app/evm_mempool.go
[failure] 18-18:
func (*KiichainApp).configureEVMMempool is unused (unused)
[failure] 63-63:
(*KiichainApp).createMempoolConfig - result 1 (error) is always nil (unparam)
🪛 OSV Scanner (2.3.1)
go.mod
[HIGH] 58-58: github.com/consensys/gnark-crypto 0.18.0: Unchecked memory allocation during vector deserialization in github.com/consensys/gnark-crypto
(GO-2025-4087)
[HIGH] 58-58: github.com/consensys/gnark-crypto 0.18.0: gnark-crypto allows unchecked memory allocation during vector deserialization
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: tests
- GitHub Check: test-e2e
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: Analyze
🔇 Additional comments (42)
app/params/config.go (1)
10-11: LGTM - Package imports updated for EVM v0.5.1.The migration from
evmtypestohdandevmutilspackages aligns with the EVM v0.5.1 restructuring. The BIP44 coin type configuration and power reduction logic remain functionally equivalent.Also applies to: 50-52, 67-67
Makefile (1)
289-292: LGTM - Test build tag added consistently.The
-tags=testflag is properly added to both the tparse-enabled and fallback test execution paths, ensuring consistent test behavior.x/feeabstraction/ante/cosmos/fee_test.go (3)
289-299: LGTM - Tests updated for newNewDynamicFeeCheckerAPI.The pattern of fetching
feemarketParamsfrom the keeper and passing a pointer toNewDynamicFeeCheckeris correctly applied. This aligns with the EVM v0.5.1 API where fee market parameters are passed directly rather than through a keeper reference.
352-362: Consistent update forTestDeductFeeDecoratorGasZero.Same pattern correctly applied.
390-400: Consistent update forTestDeductFeeDecoratorFeeGranterNoFeeKeeper.Same pattern correctly applied.
go.mod (2)
23-23: Using prerelease cosmos-sdk commit.The SDK version
v0.53.5-0.20251030204916-768cb210885cis a pseudo-version pointing to a specific commit rather than a tagged release. Ensure this is intentional and track when the official v0.53.5 release is available.
307-324: Replace directives look appropriate for the fork strategy.The replace directives for
cosmos/evm,go-ethereum, andgin-gonicare intentional forks. The EVM fork (KiiChain/evm v0.5.1-fork.2) supports fee abstraction as noted in the comment.x/feeabstraction/ante/evm/mono_decorator.go (2)
51-52: LGTM - Parameter threading updated for EVM v0.5.1.The decorator now properly stores and passes
evmParamsandfeemarketParamstoNewMonoDecoratorUtils, aligning with the updated EVM library API that expects explicit parameter passing rather than fetching from keepers.Also applies to: 77-78, 105-105
183-189: Signature verification API updated.
SignatureVerificationnow requires theethTxparameter in addition toethMsgandSigner, matching the EVM v0.5.1 signature.x/feeabstraction/ante/evm/account_verification.go (1)
30-36: LGTM - EOA verification correctly uses HasCodeHash().The condition properly identifies contract accounts: accounts with a code hash cannot send transactions directly. The logic handles the nil check correctly, allowing new EOAs to be created while rejecting existing contracts.
contrib/local_node.sh (1)
119-121: LGTM! Bank denom metadata added for EVM compatibility.The denom metadata correctly defines the base unit (
akii) and display unit (kiiwith 18 decimals), which is required for proper EVM integration with the native staking token.precompiles/oracle/integration_test.go (1)
52-53: LGTM! Test updated to match new constructor signature.The oracle precompile constructor no longer returns an error, so the test correctly removes error handling.
precompiles/wasmd/security_test.go (3)
141-141: LGTM! Method call updated toExecuteWasm.The test correctly uses the renamed method that reflects the Wasm-specific execution path.
209-214: LGTM! Gas handling tests updated to useExecuteWasm.Both the panic-expected and normal execution paths correctly use the renamed method.
396-397: LGTM! Keeper reference test updated.The test correctly uses
ExecuteWasmto verify the keeper is accessible and functional.precompiles/wasmd/realworld_attack_test.go (2)
231-232: LGTM! Gas exhaustion test panic path updated.The test correctly uses
ExecuteWasmfor the out-of-gas panic case.
238-239: LGTM! Gas exhaustion test normal path updated.The successful execution path correctly uses
ExecuteWasm.tests/e2e/e2e_setup_test.go (2)
398-413: LGTM! Bank denom metadata uses centralized CoinInfo.The metadata now references
keepers.CoinInfofor consistency across the codebase. The structure correctly defines both base and display denomination units.
486-494: EVM genesis configuration added for v0.5.1 compatibility.The EVM module genesis state is now properly configured with
EvmDenomandExtendedDenomOptionsfrom the cosmos/evm v0.5.1-fork.2 dependency. This ensures the E2E tests use the correct EVM denomination settings.cmd/kiichaind/cmd/root.go (1)
433-438: EVM chain ID now injected via viper instead of EVMAppOptions.This refactors the EVM chain ID configuration to use viper directly, which simplifies the
NewKiichainAppsignature. The pattern is consistent with theappExportfunction (lines 470-476) which also casts to*viper.Viper.precompiles/wasmd/tx_test.go (1)
286-286: LGTM! Test updated to useExecuteWasm.The test correctly uses the renamed method for executing CosmWasm contracts through the precompile.
app/keepers/precompiles.go (1)
129-179: Precompile constructors updated to cosmos/evm v0.5.1 API.Constructors now accept explicit
MsgServerImplandQuerierimplementations instead of constructing them internally, improving dependency injection and testability. The removal of error returns fromNewPrecompileis correct—these constructors no longer fail.ante/ante_cosmos.go (1)
24-26: LGTM: Context-aware parameter initialization.The refactoring to accept
ctx sdk.Contextand retrieve fee market parameters once from context is clean and efficient. Passing pointers to these parameters throughout the decorator chain ensures consistency.precompiles/wasmd/tx.go (1)
70-77: LGTM: Method renamed for clarity.The rename from
ExecutetoExecuteWasmimproves clarity and aligns with the Wasmd-specific execution path described in the related changes.ante/ante.go (1)
30-48: LGTM: Consistent context propagation.Forwarding
ctxto internal ante handler constructors enables context-aware parameter retrieval in the decoration chains, aligning with the broader refactoring pattern.precompiles/wasmd/integration_test.go (1)
85-93: LGTM: Simplified precompile initialization.The updated test scaffolding correctly reflects the simplified
NewPrecompileconstructor that no longer returns an error, and the transition toNewEmptyTxConfig()aligns with the removal of header hash dependencies.ante/ante_evm.go (1)
12-29: LGTM: EVM ante handler follows the established pattern.The context-aware parameter retrieval and pointer passing to
NewEVMMonoDecoratoris consistent with the broader refactoring across ante handlers.x/feeabstraction/ante/evm/mono_decorator_test.go (2)
74-87: LGTM: Test correctly updated for new constructor signature.The test properly retrieves EVM and fee market parameters from context before passing them to
NewEVMMonoDecorator, matching the updated constructor signature.
397-410: LGTM: Consistent parameter handling in second test case.The test follows the same correct pattern of retrieving parameters before decorator construction.
app/helpers/test_helpers.go (1)
109-163: LGTM: Comprehensive EVM test initialization.The extended test scaffolding properly configures bank denom metadata and EVM genesis state using
keepers.CoinInfo, ensuring tests have consistent denomination configuration. The configurator reset pattern aroundInitChainandFinalizeBlockensures clean state between test runs.app/modules.go (1)
126-126: The EVM module constructor signature matches cosmos/evm v0.5.1 — all four parameters (EVMKeeper, AccountKeeper, BankKeeper, and AddressCodec) are correctly ordered and typed.app/keepers/keepers.go (2)
98-106: LGTM on CoinInfo definition.The
CoinInfovariable is correctly initialized using the chain's parameter constants fromkiiparams, consolidating the EVM coin configuration in one place.
170-171: LGTM on evmChainID parameter addition.The new
evmChainIDparameter properly threads the chain ID from app options into the keeper initialization, aligning with the EVM v0.5.1 configuration pattern.app/app.go (3)
61-66: LGTM on import alias updates.The import aliases (
txlistener,cosmosevmantetypes,evmtypes) are correctly updated to reflect the package reorganization in cosmos/evm v0.5.1.
104-104: LGTM on type and reference updates.The
pendingTxListenerstype change andExtensionOptionCheckerreference update correctly align with the cosmos/evm v0.5.1 API changes.Also applies to: 332-332
210-214: EVM pre-blocker ordering aligns with v0.5.1 integration.The addition of
evmtypes.ModuleNameto pre-blockers follows standard Cosmos SDK patterns and is consistent with EVM v0.5.1 integration (which includes a pre-blocker fix in v0.5.0 for non-deterministic state mutation). The ordering—upgrade, auth, evm—respects the documented requirement that the upgrade module be prioritized and does not introduce conflicts with existing module logic.precompiles/oracle/oracle.go (3)
29-44: LGTM on ABI embedding pattern.The ABI embedding with package-level initialization in
init()is a clean pattern that ensures the ABI is loaded once at startup. The panic on error is appropriate here since a missing or malformed ABI would make the precompile non-functional.
54-66: LGTM on simplified NewPrecompile constructor.Removing the error return from
NewPrecompileis appropriate since ABI loading is now handled ininit(). This aligns with the pattern in the wasmd precompile.
88-117: LGTM on Run/Execute refactoring.The separation of
Run(handling EVM context) andExecute(handling method dispatch) improves clarity. Thecmn.SetupABIcall correctly handles read-only enforcement for transaction methods.precompiles/wasmd/wasmd.go (3)
31-46: LGTM on ABI embedding pattern.Consistent with the oracle precompile, the ABI is embedded and loaded at package initialization. This ensures a single source of truth for the ABI definition.
91-129: LGTM on Run/Execute flow with reentrancy protection.The refactored execution flow correctly:
- Delegates from
RuntoExecuteviaRunNativeAction- Threads
evm.Originthrough for reentrancy lock generation- Ensures the lock before executing any transactional methods
- Properly dispatches to the renamed
ExecuteWasmmethod (avoiding conflict with theExecuteentry point)The reentrancy lock using origin address and nonce provides transaction-scoped protection as documented.
55-68: LGTM on simplified NewPrecompile.The constructor is now consistent with the oracle precompile pattern, directly assigning the package-level
ABIandContractAddress.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/evm_mempool.go (1)
15-52: Unused function issue already flagged—still pending resolution.The
//nolint:unuseddirective suppresses the lint warning but the function remains dead code. As noted in the previous review, this function needs to be wired into the initialization flow (likely called fromapp.goafter keepers and ante handlers are set up) or removed entirely.The internal logic—early exits for missing chain config or disabled mempool, mempool instantiation, and handler wiring—looks correct.
x/feeabstraction/ante/evm/mono_decorator.go (1)
335-337: Address the past review comment about the commented-out line.The commented-out context value at line 336 still lacks explanation or a TODO comment as requested in the previous review. Either remove this line if the gas refunding feature is no longer planned, or add a TODO comment explaining when/if this should be re-enabled (e.g., when
x/vm/keeper/gas.goor equivalent gas refunding consumer is implemented).
🧹 Nitpick comments (1)
cmd/kiichaind/cmd/root.go (1)
416-420: Consider improving error handling instead of panic.The type assertion failure triggers a panic during app initialization. While this indicates a programming error, consider returning an error from the
newAppfunction instead, allowing callers to handle the failure more gracefully and providing better diagnostics in production environments.🔎 Proposed improvement
Modify the function signature to return an error and handle the type assertion failure gracefully:
func (a appCreator) newApp( logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts servertypes.AppOptions, -) evmserver.Application { +) (evmserver.Application, error) { var cache storetypes.MultiStorePersistentCache if cast.ToBool(appOpts.Get(server.FlagInterBlockCache)) { cache = store.NewCommitKVStoreCacheManager() } var wasmOpts []wasmkeeper.Option if cast.ToBool(appOpts.Get("telemetry.enabled")) { wasmOpts = append(wasmOpts, wasmkeeper.WithVMCacheMetrics(prometheus.DefaultRegisterer)) } pruningOpts, err := server.GetPruningOptionsFromFlags(appOpts) if err != nil { - panic(err) + return nil, err } skipUpgradeHeights := make(map[int64]bool) for _, h := range cast.ToIntSlice(appOpts.Get(server.FlagUnsafeSkipUpgrades)) { skipUpgradeHeights[int64(h)] = true } // Get the chain id homeDir := cast.ToString(appOpts.Get(flags.FlagHome)) chainID, err := getChainIDFromOpts(appOpts) if err != nil { - panic(err) + return nil, err } snapshotDir := filepath.Join(homeDir, "data", "snapshots") snapshotDB, err := dbm.NewDB("metadata", server.GetAppDBBackend(appOpts), snapshotDir) if err != nil { - panic(err) + return nil, err } snapshotStore, err := snapshots.NewStore(snapshotDB, snapshotDir) if err != nil { - panic(err) + return nil, err } // BaseApp Opts snapshotOptions := snapshottypes.NewSnapshotOptions( cast.ToUint64(appOpts.Get(server.FlagStateSyncSnapshotInterval)), cast.ToUint32(appOpts.Get(server.FlagStateSyncSnapshotKeepRecent)), ) baseappOptions := []func(*baseapp.BaseApp){ baseapp.SetChainID(chainID), baseapp.SetPruning(pruningOpts), baseapp.SetMinGasPrices(cast.ToString(appOpts.Get(server.FlagMinGasPrices))), baseapp.SetHaltHeight(cast.ToUint64(appOpts.Get(server.FlagHaltHeight))), baseapp.SetHaltTime(cast.ToUint64(appOpts.Get(server.FlagHaltTime))), baseapp.SetMinRetainBlocks(cast.ToUint64(appOpts.Get(server.FlagMinRetainBlocks))), baseapp.SetInterBlockCache(cache), baseapp.SetTrace(cast.ToBool(appOpts.Get(server.FlagTrace))), baseapp.SetIndexEvents(cast.ToStringSlice(appOpts.Get(server.FlagIndexEvents))), baseapp.SetSnapshot(snapshotStore, snapshotOptions), baseapp.SetIAVLCacheSize(cast.ToInt(appOpts.Get(server.FlagIAVLCacheSize))), } viperAppOpts, ok := appOpts.(*viper.Viper) if !ok { - panic("appOpts is not viper.Viper") + return nil, fmt.Errorf("appOpts must be of type *viper.Viper, got %T", appOpts) } viperAppOpts.Set(srvflags.EVMChainID, kiichain.KiichainID) - return kiichain.NewKiichainApp( + return kiichain.NewKiichainApp( logger, db, traceStore, true, skipUpgradeHeights, cast.ToString(appOpts.Get(flags.FlagHome)), appOpts, wasmOpts, baseappOptions..., - ) + ), nil }Note: This change requires updating the caller to handle the returned error.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/config.goapp/evm_mempool.goapp/helpers/test_helpers.gocmd/kiichaind/cmd/root.gox/feeabstraction/ante/evm/mono_decorator.go
💤 Files with no reviewable changes (1)
- app/config.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
🧬 Code graph analysis (3)
app/helpers/test_helpers.go (4)
wasmbinding/tokenfactory/types/types.go (2)
Metadata(7-20)DenomUnit(22-33)app/keepers/keepers.go (1)
CoinInfo(101-106)app/params/config.go (1)
DisplayDenom(31-31)app/config.go (1)
KiichainID(46-46)
app/evm_mempool.go (1)
app/app.go (1)
KiichainApp(94-114)
cmd/kiichaind/cmd/root.go (1)
app/config.go (1)
KiichainID(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: test-e2e
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: golangci-lint
🔇 Additional comments (6)
app/evm_mempool.go (1)
54-63: Previous feedback addressed—error return removed.The unnecessary error return flagged in the previous review has been correctly removed. The function now returns only
*evmmempool.EVMMempoolConfig, making the signature cleaner.Note: This function remains transitively unused since
configureEVMMempoolis not called.x/feeabstraction/ante/evm/mono_decorator.go (3)
51-52: Parameter threading looks good.The addition of
evmParamsandfeemarketParamsfields toMonoDecoratorand the corresponding constructor updates correctly propagate the EVM and fee market parameters into the decorator, eliminating the need to fetch them repeatedly during ante handling.Also applies to: 67-68, 76-77
104-104: The function call toevmante.NewMonoDecoratorUtilspasses four parameters:ctx,md.evmKeeper,md.evmParams, andmd.feemarketParams. This appears consistent with the EVM v0.5.1 version specified in go.mod, though the external dependency is not accessible for direct signature verification.
182-188: TheSignatureVerificationsignature change is correct.The function call at lines 182-186 properly passes
ethTxas the second parameter followingethMsg, withdecUtils.Signeras the third parameter. TheethTxvariable is correctly initialized fromUnpackEthMsgat line 137, and this parameter order matches the EVM v0.5.1 API (using the forked version v0.5.1-fork.2).app/helpers/test_helpers.go (2)
110-128: LGTM! Bank denomination metadata setup is correct.The bank genesis state is properly extended with denomination metadata including units, base/display denoms, and symbol. The configuration correctly uses
keepers.CoinInfoto ensure consistency with the EVM module's denomination settings.
130-136: LGTM! EVM genesis configuration is properly initialized.The EVM genesis state correctly sets the EvmDenom and ExtendedDenomOptions using
keepers.CoinInfo, ensuring consistency with the bank metadata configuration above.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/feeabstraction/ante/evm/mono_decorator.go (1)
335-337: Remove unused context value assignment or implement the consumer.Line 336 sets
ContextPaidFeesKeyin the context, but the consumer filex/vm/keeper/gas.godoes not exist. No usage ofContextPaidFeesKeyexists anywhere in the codebase except this assignment, making it dead code. Either remove this line and the misleading comment on lines 8-9, or implement the promised consumer in the EVM module.
♻️ Duplicate comments (1)
CHANGELOG.md (1)
7-7: Complete the link formatting fix.Line 7 is still missing the
https://prefix. The past review flagged both lines 6-7, but only line 6 was corrected.🔎 Proposed fix
-- Bump [cosmos go-ethereum](github.com/cosmos/go-ethereum) to [v1.16.2](https://github.com/cosmos/go-ethereum/releases/tag/v1.16.2-cosmos-1) +- Bump [cosmos go-ethereum](https://github.com/cosmos/go-ethereum) to [v1.16.2](https://github.com/cosmos/go-ethereum/releases/tag/v1.16.2-cosmos-1)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdx/feeabstraction/ante/evm/mono_decorator.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: golangci-lint
- GitHub Check: test-e2e
- GitHub Check: liveness-test
- GitHub Check: tests
- GitHub Check: Analyze
- GitHub Check: golangci-lint
🔇 Additional comments (3)
x/feeabstraction/ante/evm/mono_decorator.go (3)
61-69: LGTM!The constructor signature correctly extends to accept
evmParamsandfeemarketParamsfor the v0.5.1 integration. The previous gofumpt formatting issue has been resolved.
305-307: VerifyCheckGasWantedsignature against your KiiChain/evm fork. Your codebase uses a custom fork (v0.5.1-fork.2), not standard cosmos/evm v0.5.1. The call at line 305 passes 5 parameters—confirm the fork's actual signature accepts this parameter count.
182-186: Verify the SignatureVerification API signature against cosmos/evm v0.5.1 source code.The call uses three parameters
(ethMsg, ethTx, decUtils.Signer). This should be verified directly in the cosmos/evm v0.5.1 repository or by checking the dependency after a build to ensure the parameter order and types match the expected API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/create_app.go (1)
33-37: Temporary directory is not cleaned up.The temp directory created at line 34 is never removed, which could accumulate test artifacts over time. Consider returning a cleanup function or deferring removal.
🔎 Optional: Return cleanup function for callers
Alternatively, if the calling test handles cleanup, ensure it's documented. A simple approach would be to add a comment noting that callers are responsible for cleanup, or modify the function signature to return a cleanup function:
// Example cleanup approach in calling test: // defer os.RemoveAll(dir)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/create_app.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the current Cosmos SDK version used by the KiiChain codebase, sdk.Context directly implements the context.Context interface and can be passed directly to query methods without needing sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods.
🧬 Code graph analysis (1)
tests/integration/create_app.go (2)
app/app.go (2)
DefaultNodeHome(77-77)NewKiichainApp(126-319)app/config.go (1)
KiichainID(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze
- GitHub Check: tests
- GitHub Check: liveness-test
- GitHub Check: golangci-lint
- GitHub Check: test-e2e
- GitHub Check: golangci-lint
🔇 Additional comments (2)
tests/integration/create_app.go (2)
39-62: EVM configurator setup follows the expected pattern.The two-phase
ResetTestConfig()approach (before and after app creation) aligns with the pattern documented inapp/helpers/test_helpers.go. The chain config setup with proper error handling is appropriate.
31-31: Integration tests do not run in parallel (not.Parallel()calls found in any test files), so there is no race condition. The global variable modification is safe in this sequential test context. This concern can be removed.
Update gnark vulnerable dependency
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/failing-tests.md (1)
6-8: Clarify test removal rationale and status.The explanation for removing
vm.NewNestedEVMExtensionCallSuiteis informal and ambiguous. It's unclear whether this removal is temporary (pending a fix) or permanent. The phrasing "I could not find a straightforward way to add balance to it" reads more like a development note than a documented decision.Consider clarifying:
- Whether this is a known limitation that should be addressed in a future PR
- The specific validator balance issue and potential solutions (e.g., test setup adjustments, genesis configuration, or module dependencies)
- Whether the flash bound exploit test should be preserved/migrated elsewhere or if its removal is acceptable
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
Makefilego.modtests/integration/failing-tests.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the current Cosmos SDK version used by the KiiChain codebase, sdk.Context directly implements the context.Context interface and can be passed directly to query methods without needing sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods.
📚 Learning: 2025-08-23T19:37:59.730Z
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the KiiChain codebase using the current Cosmos SDK version, sdk.Context directly implements the context.Context interface and should be passed directly to query methods without using sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods like GetTokenfactoryDenomAdmin.
Applied to files:
go.mod
📚 Learning: 2025-08-23T19:37:59.730Z
Learnt from: watarus
Repo: KiiChain/kiichain PR: 111
File: wasmbinding/tokenfactory/queries_test.go:185-192
Timestamp: 2025-08-23T19:37:59.730Z
Learning: In the current Cosmos SDK version used by the KiiChain codebase, sdk.Context directly implements the context.Context interface and can be passed directly to query methods without needing sdk.WrapSDKContext() wrapper. The test pattern in wasmbinding/tokenfactory/queries_test.go demonstrates this by passing ctx directly to query plugin methods.
Applied to files:
go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: liveness-test
- GitHub Check: test-e2e
- GitHub Check: golangci-lint
🔇 Additional comments (7)
go.mod (7)
23-23: Verify pseudo-version usage for cosmos-sdk and ibc-go.Lines 23 and 29 use pseudo-versions (e.g.,
v0.53.5-0.20251030204916-768cb210885c) rather than released versions. These suggest development/unreleased builds with specific fixes. Ensure this is intentional and documented in the PR description or commit messages.Can you confirm that these pseudo-versions are temporary until official releases become available, or if they address specific fixes required for EVM v0.5.1 integration?
Also applies to: 29-29
58-58: Confirmed: gnark-crypto vulnerability patched.Line 58 correctly specifies gnark-crypto v0.18.1, which resolves the HIGH severity vulnerability (GO-2025-4087 / GHSA-fj2x-735w-74vq) that was previously flagged. This patch is in place. ✓
Also applies to: 58-58
244-246: Verify prometheus metrics library compatibility.Lines 244-246 update prometheus/client_model, prometheus/common, and prometheus/procfs. Ensure these versions are compatible with the overall monitoring and observability setup in KiiChain.
Can you verify that the prometheus library updates (client_model v0.6.2, common v0.65.0, procfs v0.16.1) are compatible with your metrics collection and expose endpoints?
312-312: Clarify the purpose of cosmos/evm fork version v0.5.1-fork.2.Line 312 points to
github.com/KiiChain/evm v0.5.1-fork.2, which is a forked version with 2 additional commits beyond the upstream v0.5.1 release. Per the AI summary, the fork introduces context-aware ante decorators and fee-market parameter handling.Please confirm:
- What are the specific enhancements in fork.2 (commits beyond v0.5.1)?
- Is there a tracking issue to merge these changes back to the upstream cosmos/evm repository?
- Are these changes specific to KiiChain or could they benefit the wider Cosmos ecosystem?
Also applies to: 312-312
318-318: Verify cosmos/go-ethereum fork version is correct.Line 318 uses
cosmos/go-ethereum v1.16.2-cosmos-1. Ensure this is the intended version and is compatible with the EVM v0.5.1 upgrade and ethereum/go-ethereum v1.15.11 (line 30) specified in the requires section.Can you confirm the version alignment between:
github.com/ethereum/go-ethereum v1.15.11(required, line 30)cosmos/go-ethereum v1.16.2-cosmos-1(replace fork, line 318)Are these versions compatible and intentionally different?
Also applies to: 318-318
1-3: Module and Go version look appropriate.Module declaration and Go version (1.23.8) are consistent with modern Cosmos SDK standards. No concerns here. ✓
56-56: No action required. Verification of the three indirect dependencies shows no known vulnerabilities:
- bytedance/gopkg v0.1.3: No CVEs found
- minio/sha256-simd v1.0.0: No CVEs found
- zondax/golem v0.27.0: No direct CVEs found; potential indirect dependency on golang.org/x/crypto is mitigated (project uses v0.41.0, which exceeds the fix version v0.31.0 for CVE-2024-45337)
feat: add evm upgrade v5
Upgrade kiichain to version v7.0.0
Description
Bump EVM to v0.5.1
Type of change
How Has This Been Tested?