feat(evm)!: update to geth v1.14 with new tracers, EIP-1153, PRECOMPILE_ADDRS, and transient storage support #2274
Conversation
📝 WalkthroughWalkthroughThis update upgrades the Ethereum Virtual Machine (EVM) integration to Geth v1.14, introducing support for EIP-1153 transient storage, new precompile address handling, and enhanced tracing using Go's slog and tracing hooks. It refactors message handling to use explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JSON-RPC Server
participant Geth Logger (slog)
participant EVM Keeper
participant StateDB
participant Tracer
participant Precompile
User->>JSON-RPC Server: Sends EVM tx or query
JSON-RPC Server->>EVM Keeper: Process tx/query
EVM Keeper->>StateDB: Prepare state, balances, access list
EVM Keeper->>Tracer: Initialize tracing hooks
EVM Keeper->>Precompile: (if precompile call) Execute with new interface
Precompile->>StateDB: Read/write, use transient storage if needed
EVM Keeper->>StateDB: Apply tx, update balances (uint256), handle selfdestruct
StateDB->>Tracer: Emit trace events
EVM Keeper->>JSON-RPC Server: Return result
JSON-RPC Server->>User: Respond with tx/query result
Note over EVM Keeper, StateDB: All balance and storage ops now use uint256 and support EIP-1153 transient storage
Note over Tracer, Geth Logger: Tracing uses new Geth tracing hooks and slog handler
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit aaf6431 Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:25:36 2025 -0500 refactor: linter and formatter commit f36399c Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:07:42 2025 -0500 chore: changelog commit f6bb649 Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:07:42 2025 -0500 chore: changelog commit a6877ee Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:06:45 2025 -0500 chore: move Go to v1.22 because it's required for geth commit a1e74e9 Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:06:45 2025 -0500 chore: move Go to v1.22 because it's required for geth commit b546151 Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:04:56 2025 -0500 feat(evm)!: update to geth v1.13 with EIP-1153, PRECOMPILE_ADDRS, and transient storage support This commit upgrades the Nibiru EVM module for compatibility with go-ethereum v1.13.14, introducing the following changes: - Updated all references from deprecated types in `rpc` to their new equivalents under `common/math`, such as replacing `rpc.DecimalOrHex` with `math.HexOrDecimal64`. - Removed deprecated EIP-155 seed hash API (`debug_seedHash`) and associated `ethash` import. - Aligned `vm.Config` and fee calculation logic with new EIP-based gas cost parameters (`isShanghai`, `isEIP3860`). - Introduced explicit `PRECOMPILE_ADDRS` constant to aggregate EVM precompiles with Nibiru’s extensions (FunToken, Wasm, Oracle). - Implemented support for **EIP-1153** (transient storage): - Added `transientStorage` map to `StateDB`, with getters/setters and journaling support. - Added `Prepare` method to reset access lists and transient storage for each tx. - Refactored `SelfDestruct` logic and exposed `HasSelfDestructed` (was `Suicide`) for better clarity and future EIP-6780 readiness. - Reworked balance mutation logic (`AddBalanceSigned`) to correctly handle signed values and prevent `uint256` overflow errors. - Minor typo corrections (e.g., "occured" → "occurred") in proto files and comments. This upgrade also adjusts `go.mod` to: - Replace `go-ethereum v1.10.x` with `v1.13.14` - Downgrade Go version to 1.21 for compatibility with `go-ethereum` and its Pebble dependency - Pin Pebble to a compatible commit required by geth's internal `ethdb` commit 803f9b4 Author: Unique-Divine <[email protected]> Date: Mon Apr 14 10:04:56 2025 -0500 feat(evm)!: update to geth v1.13 with EIP-1153, PRECOMPILE_ADDRS, and transient storage support This commit upgrades the Nibiru EVM module for compatibility with go-ethereum v1.13.14, introducing the following changes: - Updated all references from deprecated types in `rpc` to their new equivalents under `common/math`, such as replacing `rpc.DecimalOrHex` with `math.HexOrDecimal64`. - Removed deprecated EIP-155 seed hash API (`debug_seedHash`) and associated `ethash` import. - Aligned `vm.Config` and fee calculation logic with new EIP-based gas cost parameters (`isShanghai`, `isEIP3860`). - Introduced explicit `PRECOMPILE_ADDRS` constant to aggregate EVM precompiles with Nibiru’s extensions (FunToken, Wasm, Oracle). - Implemented support for **EIP-1153** (transient storage): - Added `transientStorage` map to `StateDB`, with getters/setters and journaling support. - Added `Prepare` method to reset access lists and transient storage for each tx. - Refactored `SelfDestruct` logic and exposed `HasSelfDestructed` (was `Suicide`) for better clarity and future EIP-6780 readiness. - Reworked balance mutation logic (`AddBalanceSigned`) to correctly handle signed values and prevent `uint256` overflow errors. - Minor typo corrections (e.g., "occured" → "occurred") in proto files and comments. This upgrade also adjusts `go.mod` to: - Replace `go-ethereum v1.10.x` with `v1.13.14` - Downgrade Go version to 1.21 for compatibility with `go-ethereum` and its Pebble dependency - Pin Pebble to a compatible commit required by geth's internal `ethdb` commit 1834a61 Author: Unique-Divine <[email protected]> Date: Sat Apr 12 19:51:44 2025 -0500 feat(evm): Adapt module to Geth v1.13 core package changes This commit updates the Nibiru EVM module to align with significant breaking changes introduced in the upstream go-ethereum v1.13 `core/vm` and `core/types` packages. The goal is to leverage the updated Geth dependencies while ensuring compatibility with Nibiru's specific requirements, particularly its custom precompiles that interact with the Cosmos SDK. This addresses several LSP errors and runtime issues arising from removed or modified upstream functions and interfaces: 1. **Replace `core.NewMessage` Calls:** - The `core.NewMessage` factory function was removed upstream in favor of direct struct instantiation. - All instances have been replaced with `core.Message{...}` struct literals, correctly mapping arguments to fields like `From`, `To`, `Nonce`, `Value`, `GasLimit`, gas price fields (`GasPrice`, `GasFeeCap`, `GasTipCap`), `Data`, `AccessList`. - Monetary and gas values are initialized as `*big.Int` per the `core.Message` definition. - Newer fields relevant to Cancun/EIP-4844 (`BlobGasFeeCap`, `BlobHashes`) and Nibiru-specific fields (`SkipAccountChecks`) are now correctly initialized. 2. **Revert `vm.PrecompiledContract` Interface for Nibiru Precompiles:** - Geth v1.13 simplified the `PrecompiledContract.Run` signature to `Run(input []byte)`. - This change breaks Nibiru's custom precompiles (e.g., Wasm) which require the `*vm.EVM` pointer to access `StateDB` and derive the `sdk.Context` needed for Cosmos SDK keeper interactions. - This commit *reverts* the interface definition within Nibiru's fork back to `Run(evm *vm.EVM, contract *vm.Contract, readonly bool)`. - The `Address() common.Address` method is also restored to the interface and implementations for use by the execution logic. - Standard precompile implementations included in this module have been adapted to match this reverted interface signature. - The `vm.RunPrecompiledContract` helper function is updated to pass the necessary `*vm.EVM`, `*vm.Contract`, and `readonly` context. 3. **Adopt `uint256` for VM Value/Balance Interactions:** - While `core.Message` retains `*big.Int`, the internal VM logic and `StateDB` methods (e.g., `vm.Call`, `vm.Create`, `AddBalance`, `SubBalance`, `Contract.value`) were updated upstream to use `*uint256.Int` (from `holiman/uint256`). - Code passing values into these VM contexts has been updated to perform the necessary `*big.Int` -> `*uint256.Int` conversions. - Added `holiman/uint256` as a direct dependency in `go.mod`. 4. **Replace `StateDB.PrepareAccessList`:** - The `PrepareAccessList` method was removed from the `vm.StateDB` interface upstream. - Calls have been replaced with the new, more comprehensive `StateDB.Prepare(rules, sender, ...)` method. 5. **Replace `evm.ActivePrecompiles` Method:** - The `ActivePrecompiles` method on the `vm.EVM` struct was removed. - Calls have been replaced with the standalone package function `vm.ActivePrecompiles(rules)`, passing the appropriate chain rules. These changes resolve the identified compatibility errors and ensure the EVM module integrates correctly with both the updated Geth core components and Nibiru's specific architecture and custom precompiles. commit af71ded Author: Unique-Divine <[email protected]> Date: Sat Apr 12 19:51:44 2025 -0500 feat(evm): Adapt module to Geth v1.13 core package changes This commit updates the Nibiru EVM module to align with significant breaking changes introduced in the upstream go-ethereum v1.13 `core/vm` and `core/types` packages. The goal is to leverage the updated Geth dependencies while ensuring compatibility with Nibiru's specific requirements, particularly its custom precompiles that interact with the Cosmos SDK. This addresses several LSP errors and runtime issues arising from removed or modified upstream functions and interfaces: 1. **Replace `core.NewMessage` Calls:** - The `core.NewMessage` factory function was removed upstream in favor of direct struct instantiation. - All instances have been replaced with `core.Message{...}` struct literals, correctly mapping arguments to fields like `From`, `To`, `Nonce`, `Value`, `GasLimit`, gas price fields (`GasPrice`, `GasFeeCap`, `GasTipCap`), `Data`, `AccessList`. - Monetary and gas values are initialized as `*big.Int` per the `core.Message` definition. - Newer fields relevant to Cancun/EIP-4844 (`BlobGasFeeCap`, `BlobHashes`) and Nibiru-specific fields (`SkipAccountChecks`) are now correctly initialized. 2. **Revert `vm.PrecompiledContract` Interface for Nibiru Precompiles:** - Geth v1.13 simplified the `PrecompiledContract.Run` signature to `Run(input []byte)`. - This change breaks Nibiru's custom precompiles (e.g., Wasm) which require the `*vm.EVM` pointer to access `StateDB` and derive the `sdk.Context` needed for Cosmos SDK keeper interactions. - This commit *reverts* the interface definition within Nibiru's fork back to `Run(evm *vm.EVM, contract *vm.Contract, readonly bool)`. - The `Address() common.Address` method is also restored to the interface and implementations for use by the execution logic. - Standard precompile implementations included in this module have been adapted to match this reverted interface signature. - The `vm.RunPrecompiledContract` helper function is updated to pass the necessary `*vm.EVM`, `*vm.Contract`, and `readonly` context. 3. **Adopt `uint256` for VM Value/Balance Interactions:** - While `core.Message` retains `*big.Int`, the internal VM logic and `StateDB` methods (e.g., `vm.Call`, `vm.Create`, `AddBalance`, `SubBalance`, `Contract.value`) were updated upstream to use `*uint256.Int` (from `holiman/uint256`). - Code passing values into these VM contexts has been updated to perform the necessary `*big.Int` -> `*uint256.Int` conversions. - Added `holiman/uint256` as a direct dependency in `go.mod`. 4. **Replace `StateDB.PrepareAccessList`:** - The `PrepareAccessList` method was removed from the `vm.StateDB` interface upstream. - Calls have been replaced with the new, more comprehensive `StateDB.Prepare(rules, sender, ...)` method. 5. **Replace `evm.ActivePrecompiles` Method:** - The `ActivePrecompiles` method on the `vm.EVM` struct was removed. - Calls have been replaced with the standalone package function `vm.ActivePrecompiles(rules)`, passing the appropriate chain rules. These changes resolve the identified compatibility errors and ensure the EVM module integrates correctly with both the updated Geth core components and Nibiru's specific architecture and custom precompiles. commit 6370b96 Author: Unique-Divine <[email protected]> Date: Sat Apr 12 10:38:30 2025 -0500 fix(evm): Pass block timestamp to MakeSigner and refactor MsgEthereumTx field usage - Update all calls to `gethcore.MakeSigner` to include the block time as a Unix timestamp (seconds), using `evm.ParseBlockTimeUnixU64(ctx)`. - Add `ParseBlockTimeUnixU64` utility to extract block time from `sdk.Context` in a safe, reusable way. - Refactor usage of MsgEthereumTx field getters to use direct struct fields (`From`, `To`, `Value`, `GasFeeCap`, etc.), improving efficiency and clarity. - Enhance `ParseWeiAsMultipleOfMicronibi` to return `uint256.Int` and handle nil, zero, negative, and overflow edge cases with clear error messages. commit 0fad842 Author: Unique-Divine <[email protected]> Date: Sat Apr 12 10:38:30 2025 -0500 fix(evm): Pass block timestamp to MakeSigner and refactor MsgEthereumTx field usage - Update all calls to `gethcore.MakeSigner` to include the block time as a Unix timestamp (seconds), using `evm.ParseBlockTimeUnixU64(ctx)`. - Add `ParseBlockTimeUnixU64` utility to extract block time from `sdk.Context` in a safe, reusable way. - Refactor usage of MsgEthereumTx field getters to use direct struct fields (`From`, `To`, `Value`, `GasFeeCap`, etc.), improving efficiency and clarity. - Enhance `ParseWeiAsMultipleOfMicronibi` to return `uint256.Int` and handle nil, zero, negative, and overflow edge cases with clear error messages. commit 1002634 Author: Unique-Divine <[email protected]> Date: Sat Apr 12 03:21:08 2025 -0500 refactor(evm): add more compatibility the new geth StateDB inteface, updating AddBalance and SubBalance to use uint256 commit d69a8c3 Author: Unique-Divine <[email protected]> Date: Sat Apr 12 03:21:08 2025 -0500 refactor(evm): add more compatibility the new geth StateDB inteface, updating AddBalance and SubBalance to use uint256 commit d07093c Author: Unique-Divine <[email protected]> Date: Fri Apr 11 21:18:53 2025 -0500 feat: impl slog.Handler for geth v1.13. used in json-rpc These changes update Nibiru's integration with the `go-ethereum/log` package to align with its upstream migration to Go's standard structured logging library (`slog`). The previous logging setup in Nibiru, which relied on `go-ethereum/log`'s deprecated `FuncHandler` and `Record` types, was removed. 1. **Added `LogHandler`:** A new file `app/server/geth_log_handler.go` introduces the `LogHandler` type. This type implements the standard `slog.Handler` interface. Its primary role is to receive log records generated by Geth components (which now use `slog`) and translate them into corresponding calls on Nibiru's standard `cmtlog.Logger` (CometBFT logger). It correctly maps Geth/`slog` levels (including `Trace` and `Crit`) and formats log attributes for compatibility. 2. **Updated Initialization:** In `app/server/json_rpc.go`, the old `ethlog.Root().SetHandler(...)` block was replaced. The code now instantiates the new `LogHandler` (providing it the context logger `ctx.Logger.With("module", "geth")`), wraps it using `gethlog.NewLogger()`, and sets the result as the default logger for `go-ethereum` components via `gethlog.SetDefault()`. The primary reason for this refactor was the breaking change in the `go-ethereum/log` dependency, which deprecated its custom logging implementation in favor of Go's standard `slog`. These changes adapt Nibiru to the new `slog`-based API, ensuring that logs generated within embedded Geth components are correctly captured and processed by Nibiru's existing logging infrastructure (`cmtlog.Logger`). This maintains consistent logging behavior and compatibility with the updated dependency. commit 5937bbe Author: Unique-Divine <[email protected]> Date: Fri Apr 11 21:18:53 2025 -0500 feat: impl slog.Handler for geth v1.13. used in json-rpc These changes update Nibiru's integration with the `go-ethereum/log` package to align with its upstream migration to Go's standard structured logging library (`slog`). The previous logging setup in Nibiru, which relied on `go-ethereum/log`'s deprecated `FuncHandler` and `Record` types, was removed. 1. **Added `LogHandler`:** A new file `app/server/geth_log_handler.go` introduces the `LogHandler` type. This type implements the standard `slog.Handler` interface. Its primary role is to receive log records generated by Geth components (which now use `slog`) and translate them into corresponding calls on Nibiru's standard `cmtlog.Logger` (CometBFT logger). It correctly maps Geth/`slog` levels (including `Trace` and `Crit`) and formats log attributes for compatibility. 2. **Updated Initialization:** In `app/server/json_rpc.go`, the old `ethlog.Root().SetHandler(...)` block was replaced. The code now instantiates the new `LogHandler` (providing it the context logger `ctx.Logger.With("module", "geth")`), wraps it using `gethlog.NewLogger()`, and sets the result as the default logger for `go-ethereum` components via `gethlog.SetDefault()`. The primary reason for this refactor was the breaking change in the `go-ethereum/log` dependency, which deprecated its custom logging implementation in favor of Go's standard `slog`. These changes adapt Nibiru to the new `slog`-based API, ensuring that logs generated within embedded Geth components are correctly captured and processed by Nibiru's existing logging infrastructure (`cmtlog.Logger`). This maintains consistent logging behavior and compatibility with the updated dependency. commit ca3c821 Author: Unique-Divine <[email protected]> Date: Fri Apr 11 19:03:04 2025 -0500 wip!: start with a local brute force jump to a geth v1.13.15 with no changes commit 10010d6 Author: Unique-Divine <[email protected]> Date: Fri Apr 11 19:03:04 2025 -0500 wip!: start with a local brute force jump to a geth v1.13.15 with no changes
Unique-Divine
force-pushed
the
ud/geth/v1.13.15
branch
from
4fd7993 to
00e77d1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2274 +/- ##
==========================================
+ Coverage 14.23% 14.41% +0.17%
==========================================
Files 381 383 +2
Lines 100855 101359 +504
==========================================
+ Hits 14354 14608 +254
- Misses 85503 85741 +238
- Partials 998 1010 +12
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
x/evm/keeper/msg_server.go (1)
82-93: 🛠️ Refactor suggestionCheck leftover gas refund logic.
The code computes a gas refund ifGasLimit > GasUsed. Although correct, consider verifying that negative or excessive refund cases cannot arise, especially if fees shift in future EIPs or updates. Adding coverage for boundary cases would help ensure reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-92: x/evm/keeper/msg_server.go#L92
Added line #L92 was not covered by tests
🧹 Nitpick comments (9)
app/server/geth_log_handler.go (2)
27-33: Enabled method
Returningtrueensures no filtering at this layer. If you need to respect specific log levels, consider adding a level check or bridging CometBFT’s level config.
95-110: WithGroup method
Allowing a basic group name for attributes is a nice addition for structured logging.
Similarly, lines 96-109 are not covered by tests; consider a test scenario that confirms group labeling in logs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 96-109: app/server/geth_log_handler.go#L96-L109
Added lines #L96 - L109 were not covered by testsgo.mod (1)
71-76: Review of Indirect Dependency Updates
Several indirect dependencies (e.g.,github.com/status-im/keycard-go,golang.org/x/crypto,golang.org/x/exp, etc.) have been updated. While these updates likely improve compatibility and performance, it’s important to run a full dependency audit to ensure that none of these changes introduce unexpected issues.x/evm/precompile/precompile.go (1)
69-69: Intentional removal of return statement
The updated function concludes without returning the merged map of precompiles. This can simplify usage but can be a breaking change if previously consumed by other parts of the code.x/evm/keeper/grpc_query.go (2)
402-416: Validate new account creation path.
Whenacct == nil, an account is created and immediately has its nonce set tononce + 1. This is a suitable approach for simulation, but please confirm that it matches any upstream logic about account creation (e.g., do we need to set a starting balance or check whether the account was intentionally empty?). If so, add appropriate checks or documentation.
614-626: Consider clarifying SkipAccountChecks usage.
You initialize a newcore.MessagewithSkipAccountChecks: false. Confirm whether you consistently want to enforce account balance checks or if some edge cases need them skipped. Otherwise, the message construction logic looks correct.x/evm/statedb/statedb_test.go (1)
100-101: Check selfdestruct usage on non-existent accounts.
Callingdb.SelfDestruct(address)followed bys.Require().False(db.HasSuicided(address))can be confusing. By design,HasSuicidedmight remain false if the account was never created. Document the expected behavior clearly to avoid confusion for future maintainers.x/evm/statedb/statedb.go (2)
480-480: Minor coverage gap.
Line 480 references warm coinbase access inPrepare. Consider adding a test scenario to ensure that coinbase is being correctly added to access lists for the Shanghai activation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 480-480: x/evm/statedb/statedb.go#L480
Added line #L480 was not covered by tests
718-723: Deep copy logic.
TheCopy()method duplicates the entiretransientStoragemap. This is correct for snapshots, but ensure large mapping usage doesn’t cause significant performance overhead or memory spikes.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 718-723: x/evm/statedb/statedb.go#L718-L723
Added lines #L718 - L723 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumx/sudo/types/event.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (49)
.github/workflows/e2e-evm.yml(1 hunks).github/workflows/golangci-lint.yml(1 hunks).github/workflows/integration-tests.yml(1 hunks).github/workflows/simulation-tests.yml(4 hunks).github/workflows/unit-tests.yml(2 hunks)CHANGELOG.md(1 hunks)Dockerfile(2 hunks)api/nibiru/sudo/v1/event.pulsar.go(1 hunks)app/evmante/evmante_can_transfer.go(2 hunks)app/evmante/evmante_gas_consume_test.go(2 hunks)app/evmante/evmante_handler_test.go(1 hunks)app/evmante/evmante_increment_sender_seq_test.go(2 hunks)app/evmante/evmante_sigverify.go(1 hunks)app/evmante/evmante_verify_eth_acc_test.go(1 hunks)app/keepers.go(1 hunks)app/server/geth_log_handler.go(1 hunks)app/server/json_rpc.go(3 hunks)eth/rpc/backend/chain_info.go(2 hunks)eth/rpc/backend/chain_info_test.go(2 hunks)eth/rpc/backend/utils.go(2 hunks)eth/rpc/rpcapi/debugapi/api.go(0 hunks)eth/rpc/rpcapi/eth_api.go(3 hunks)go.mod(13 hunks)proto/nibiru/sudo/v1/event.proto(1 hunks)x/evm/chain_config.go(1 hunks)x/evm/const.go(3 hunks)x/evm/evmtest/tx.go(2 hunks)x/evm/json_tx_args.go(3 hunks)x/evm/keeper/call_contract.go(2 hunks)x/evm/keeper/funtoken_from_coin.go(2 hunks)x/evm/keeper/funtoken_from_erc20.go(2 hunks)x/evm/keeper/gas_fees.go(1 hunks)x/evm/keeper/grpc_query.go(11 hunks)x/evm/keeper/keeper.go(0 hunks)x/evm/keeper/msg_server.go(15 hunks)x/evm/keeper/precompiles.go(0 hunks)x/evm/keeper/vm_config.go(0 hunks)x/evm/msg.go(1 hunks)x/evm/precompile/funtoken.go(1 hunks)x/evm/precompile/oracle.go(1 hunks)x/evm/precompile/precompile.go(2 hunks)x/evm/precompile/wasm.go(1 hunks)x/evm/statedb/interfaces.go(0 hunks)x/evm/statedb/journal.go(1 hunks)x/evm/statedb/journal_test.go(4 hunks)x/evm/statedb/state_object.go(4 hunks)x/evm/statedb/statedb.go(12 hunks)x/evm/statedb/statedb_test.go(11 hunks)x/evm/vmtracer.go(2 hunks)
💤 Files with no reviewable changes (5)
- x/evm/statedb/interfaces.go
- x/evm/keeper/vm_config.go
- x/evm/keeper/keeper.go
- eth/rpc/rpcapi/debugapi/api.go
- x/evm/keeper/precompiles.go
🧰 Additional context used
🧬 Code Graph Analysis (15)
app/evmante/evmante_sigverify.go (1)
x/evm/const.go (1)
ParseBlockTimeUnixU64(175-181)
app/keepers.go (2)
x/evm/precompile/precompile.go (1)
InitPrecompiles(40-69)app/keepers/all_keepers.go (1)
PublicKeepers(33-72)
x/evm/keeper/call_contract.go (1)
x/evm/tx_data_access_list.go (1)
AccessList(18-18)
x/evm/precompile/oracle.go (2)
app/keepers/all_keepers.go (1)
PublicKeepers(33-72)x/evm/precompile/precompile.go (1)
NibiruCustomPrecompile(71-74)
x/evm/precompile/funtoken.go (2)
app/keepers/all_keepers.go (1)
PublicKeepers(33-72)x/evm/precompile/precompile.go (1)
NibiruCustomPrecompile(71-74)
x/evm/precompile/wasm.go (2)
app/keepers/all_keepers.go (1)
PublicKeepers(33-72)x/evm/precompile/precompile.go (1)
NibiruCustomPrecompile(71-74)
x/evm/keeper/funtoken_from_erc20.go (2)
x/evm/const.go (2)
EVM_MODULE_ADDRESS(104-104)Big0(183-183)x/evm/tx_data_access_list.go (1)
AccessList(18-18)
x/evm/chain_config.go (1)
x/evm/const.go (1)
Big0(183-183)
x/evm/evmtest/tx.go (2)
x/evm/const.go (2)
EVM_MODULE_ADDRESS(104-104)Big0(183-183)x/evm/tx_data_access_list.go (1)
AccessList(18-18)
app/server/geth_log_handler.go (1)
x/common/testutil/testnetwork/logger.go (1)
Logger(13-16)
x/evm/vmtracer.go (2)
x/evm/const.go (1)
PRECOMPILE_ADDRS(24-35)x/evm/tx_data_access_list.go (1)
AccessList(18-18)
eth/rpc/rpcapi/eth_api.go (1)
eth/rpc/block.go (1)
BlockNumber(25-25)
x/evm/keeper/funtoken_from_coin.go (3)
x/evm/const.go (1)
EVM_MODULE_ADDRESS(104-104)x/evm/keeper/erc20.go (1)
Erc20GasLimitDeploy(22-22)x/evm/tx_data_access_list.go (1)
AccessList(18-18)
x/evm/keeper/msg_server.go (6)
x/evm/const.go (4)
ParseBlockTimeUnixU64(175-181)ParseWeiAsMultipleOfMicronibi(155-171)NativeToWei(121-124)WeiToNative(134-137)x/evm/tx_data_access_list.go (1)
AccessList(18-18)x/evm/statedb/statedb.go (1)
StateDB(42-82)x/evm/keeper/erc20.go (1)
Erc20GasLimitExecute(28-28)x/evm/events.go (1)
MessageEventAttrTxType(28-28)x/evm/events.pb.go (9)
EventContractDeployed(419-422)EventContractDeployed(426-426)EventContractDeployed(427-429)EventContractExecuted(472-475)EventContractExecuted(479-479)EventContractExecuted(480-482)EventTransfer(358-362)EventTransfer(366-366)EventTransfer(367-369)
x/evm/keeper/grpc_query.go (5)
x/evm/tx_data_access_list.go (1)
AccessList(18-18)x/evm/const.go (1)
CallTypeRPC(98-98)eth/gas_limit.go (1)
NewInfiniteGasMeterWithLimit(45-50)x/evm/statedb/statedb.go (1)
New(89-98)x/evm/evm.pb.go (3)
TraceConfig(416-438)TraceConfig(442-442)TraceConfig(443-445)
🪛 GitHub Check: codecov/patch
app/server/json_rpc.go
[warning] 49-49: app/server/json_rpc.go#L49
Added line #L49 was not covered by tests
app/evmante/evmante_can_transfer.go
[warning] 64-64: app/evmante/evmante_can_transfer.go#L64
Added line #L64 was not covered by tests
x/evm/const.go
[warning] 175-180: x/evm/const.go#L175-L180
Added lines #L175 - L180 were not covered by tests
x/evm/msg.go
[warning] 315-316: x/evm/msg.go#L315-L316
Added lines #L315 - L316 were not covered by tests
x/evm/statedb/journal.go
[warning] 377-378: x/evm/statedb/journal.go#L377-L378
Added lines #L377 - L378 were not covered by tests
[warning] 381-382: x/evm/statedb/journal.go#L381-L382
Added lines #L381 - L382 were not covered by tests
app/server/geth_log_handler.go
[warning] 46-50: app/server/geth_log_handler.go#L46-L50
Added lines #L46 - L50 were not covered by tests
[warning] 62-63: app/server/geth_log_handler.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 71-72: app/server/geth_log_handler.go#L71-L72
Added lines #L71 - L72 were not covered by tests
[warning] 96-109: app/server/geth_log_handler.go#L96-L109
Added lines #L96 - L109 were not covered by tests
x/evm/vmtracer.go
[warning] 32-32: x/evm/vmtracer.go#L32
Added line #L32 was not covered by tests
[warning] 34-36: x/evm/vmtracer.go#L34-L36
Added lines #L34 - L36 were not covered by tests
[warning] 93-93: x/evm/vmtracer.go#L93
Added line #L93 was not covered by tests
x/evm/keeper/msg_server.go
[warning] 92-92: x/evm/keeper/msg_server.go#L92
Added line #L92 was not covered by tests
[warning] 368-369: x/evm/keeper/msg_server.go#L368-L369
Added lines #L368 - L369 were not covered by tests
[warning] 388-388: x/evm/keeper/msg_server.go#L388
Added line #L388 was not covered by tests
[warning] 392-392: x/evm/keeper/msg_server.go#L392
Added line #L392 was not covered by tests
[warning] 407-408: x/evm/keeper/msg_server.go#L407-L408
Added lines #L407 - L408 were not covered by tests
x/evm/statedb/state_object.go
[warning] 97-102: x/evm/statedb/state_object.go#L97-L102
Added lines #L97 - L102 were not covered by tests
[warning] 105-109: x/evm/statedb/state_object.go#L105-L109
Added lines #L105 - L109 were not covered by tests
x/evm/keeper/grpc_query.go
[warning] 768-770: x/evm/keeper/grpc_query.go#L768-L770
Added lines #L768 - L770 were not covered by tests
x/evm/statedb/statedb.go
[warning] 440-441: x/evm/statedb/statedb.go#L440-L441
Added lines #L440 - L441 were not covered by tests
[warning] 447-454: x/evm/statedb/statedb.go#L447-L454
Added lines #L447 - L454 were not covered by tests
[warning] 480-480: x/evm/statedb/statedb.go#L480
Added line #L480 was not covered by tests
[warning] 701-705: x/evm/statedb/statedb.go#L701-L705
Added lines #L701 - L705 were not covered by tests
[warning] 709-714: x/evm/statedb/statedb.go#L709-L714
Added lines #L709 - L714 were not covered by tests
[warning] 718-723: x/evm/statedb/statedb.go#L718-L723
Added lines #L718 - L723 were not covered by tests
[warning] 730-731: x/evm/statedb/statedb.go#L730-L731
Added lines #L730 - L731 were not covered by tests
[warning] 740-750: x/evm/statedb/statedb.go#L740-L750
Added lines #L740 - L750 were not covered by tests
🔇 Additional comments (95)
eth/rpc/backend/chain_info_test.go (2)
6-6: Import change for gethmath aligns with go-ethereum v1.13.14This import addition is necessary to support the type change from
gethrpc.DecimalOrHextogethmath.HexOrDecimal64, which is part of the go-ethereum v1.13.14 upgrade mentioned in the PR objectives.
69-69: Type casting updated to use HexOrDecimal64The change from
gethrpc.DecimalOrHextogethmath.HexOrDecimal64is consistent with go-ethereum v1.13.14 API changes. This ensures compatibility with the updated Ethereum API specifications.eth/rpc/backend/chain_info.go (2)
12-12: Import addition for gethmath packageAdding this import is necessary to support the type changes required by go-ethereum v1.13.14.
83-83: Parameter type updated for FeeHistory methodThe parameter type change from
gethrpc.DecimalOrHextogethmath.HexOrDecimal64aligns with go-ethereum v1.13.14 API requirements. This is a breaking change in the upstream dependency that needs to be propagated throughout the codebase.eth/rpc/rpcapi/eth_api.go (3)
7-7: Import gethmath for HexOrDecimal64 typeThis import is necessary to support the updated type signatures required by go-ethereum v1.13.14.
90-93: Updated interface method signature for FeeHistoryThe parameter type change in the interface definition is consistent with the implementation changes in other files and aligns with go-ethereum v1.13.14 API requirements.
327-327: Updated implementation of FeeHistory methodThe implementation correctly uses the new parameter type
gethmath.HexOrDecimal64, maintaining consistency with the interface definition and other components using this method.api/nibiru/sudo/v1/event.pulsar.go (1)
535-538: Minor Typo Correction in Comment
The comment for theActionfield now correctly uses "occurred" instead of the misspelled "occured", which improves clarity and consistency.proto/nibiru/sudo/v1/event.proto (1)
13-17: Cosmetic and Comment Enhancements in Proto Definition
The formatting adjustment on thesudoersfield and the correction in the comment for theactionfield (changing "occured" to "occurred") enhance readability and maintain consistency across the codebase.eth/rpc/backend/utils.go (2)
18-18: Adoption of eip1559 package for base fee calculation
Switching from the oldmiscpackage tomisc/eip1559aligns the codebase with the EIP-1559 fee mechanism. This import change looks correct and should help maintain compliance with the updated go-ethereum logic.
128-128:❓ Verification inconclusive
Confirm EIP-1559 compatibility in chain configuration
Usingeip1559.CalcBaseFee(cfg, header)is correct for EIP-1559-based fee calculations, but please ensure your chain config explicitly enables EIP-1559.Would you like to run a quick code search to confirm that the chain config is set with EIP-1559 parameters?
🏁 Script executed:
#!/bin/bash # Verify chain config for EIP-1559 enablement rg -A 3 "EIP1559"Length of output: 3441
Confirm Explicit EIP-1559 Enablement in Chain Configuration
The use of
eip1559.CalcBaseFee(cfg, header)for EIP-1559 fee calculations is correct. Our recent search shows that several components (e.g., the QueryBaseFee request/response definitions in files likex/evm/query.pb.goand related proto files) reference EIP-1559 parameters, which indicates that the codebase is structured to support EIP-1559. However, please double-check that the main chain configuration explicitly enables all necessary EIP-1559 parameters, rather than relying solely on these API definitions.app/server/json_rpc.go (4)
17-18: Refactor to new Geth imports
Replacingethlogandethrpcwithgethlogandgethrpcis consistent with the go-ethereum 1.13 updates.
34-38: Bridging CometBFT and Geth logs
Creating aLogHandlerinstance and setting it as the default Geth logger is a clear approach to unify logging. This is a clean integration.
40-40: Initialization of the new RPC server
Usinggethrpc.NewServer()is aligned with the updated go-ethereum interface. Nothing further stands out here.
49-49: Uncovered error path in tests
This error logging line is not covered by the tests, per the code coverage tooling. Consider adding a test that triggers a failure inRegisterNameto ensure coverage and confirm proper error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-49: app/server/json_rpc.go#L49
Added line #L49 was not covered by testsapp/server/geth_log_handler.go (4)
1-16: New file and package import structure
This file cleanly sets up the new integration withslogand Geth logging. No issues are apparent in the initial package declaration or imports.
18-25: Definition of the LogHandler struct
The struct design—storing CometBFT’s logger, attributes, and an optional group name—seems straightforward and will support flexible logging configurations.
35-81: Handle method with log-level mapping
Proactively bridging between Geth’s log levels and CometBFT’s logger is well-structured.
However, lines 46-50, 62-63, and 71-72 are flagged in the coverage report, indicating certain logging paths—particularly trace or crit—may not be tested.Please confirm that these scenarios are tested (e.g., by logging crit/trace entries) to improve coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-50: app/server/geth_log_handler.go#L46-L50
Added lines #L46 - L50 were not covered by tests
[warning] 62-63: app/server/geth_log_handler.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 71-72: app/server/geth_log_handler.go#L71-L72
Added lines #L71 - L72 were not covered by tests
83-93: WithAttrs method
Cloning the existing attributes and appending new ones helps maintain immutability. The approach is consistent with slog’s contract.go.mod (4)
3-7: Ensure Consistent Go Version Across Environments
Thegodirective is now set to1.21with comments explaining incompatibility issues with Go 1.23. However, later in the workflows and Dockerfile the Go version is updated to1.22. Consider verifying if this version discrepancy is intentional or if the Go version should be aligned across all environments.
22-22: Updated geth Dependency Version
The dependency ongithub.zerozr99.workers.dev/ethereum/go-ethereumhas been updated tov1.13.14. This change is in line with the PR’s objective to upgrade geth and support new EIPs. Please ensure any API changes from this version are fully integrated in the code.
262-265: Updated Replace Directive for wasmd and Pebble
The replace directive now pointsgithub.zerozr99.workers.dev/CosmWasm/wasmdtogithub.zerozr99.workers.dev/NibiruChain/wasmd v0.44.0-nibiruand pinsgithub.zerozr99.workers.dev/cockroachdb/pebbleto a specific commit for compatibility with geth’s storage requirements. Confirm that these changes have been validated against all dependent modules.
270-271: Forked geth Replacement Directive
Replacing the upstreamgithub.zerozr99.workers.dev/ethereum/go-ethereumwithgithub.zerozr99.workers.dev/NibiruChain/go-ethereum v1.13.15-nibiruis critical for incorporating the necessary internal modifications. Please double-check that all integration points using geth features are compatible with this fork..github/workflows/e2e-evm.yml (1)
31-34: Align Go Version in E2E Workflow
The Go version in the E2E workflow has been updated to1.22, which is in line with other CI configurations. Please verify that using Go 1.22 (whilego.modspecifies 1.21) does not lead to any build or runtime discrepancies..github/workflows/golangci-lint.yml (1)
40-44: Updated Go Version in Linter Workflow
The linter workflow now sets the Go version to1.22. This update is consistent with the other workflow files. Ensure that this change does not affect linting rules or caching behavior adversely..github/workflows/unit-tests.yml (2)
39-43: Updated Go Version for Unit Tests
The unit tests job now installs Go using version1.22, ensuring consistency with the CI environment. Given that the module’s directive ingo.modstates Go 1.21, please verify that the tests run as expected under Go 1.22.
94-98: Consistent Build Environment Setup
In the build job, the Go version is updated to1.22. This consistency across testing and build stages is beneficial. Confirm that all build scripts and commands work as intended with this version.Dockerfile (2)
3-5: Updated Base Go Image in Dockerfile
The build stage now uses thegolang:1.22base image instead of1.24. This change aligns the Docker build environment with the CI workflows. Just verify that any build-specific configurations (e.g., caching mounts) continue to function correctly under this image.
76-78: Minor Formatting in CMD
The finalCMD ["start"]remains functionally the same—the tilde marker suggests a minor formatting or newline adjustment. No further action is required here..github/workflows/integration-tests.yml (1)
44-44:❓ Verification inconclusive
Go version downgrade for compatibility with ethereum v1.13.14
The Go version has been downgraded from 1.24 to 1.22, which aligns with the PR objective to upgrade the EVM module and ensure compatibility with go-ethereum v1.13.14.
🏁 Script executed:
#!/bin/bash # Verify go.mod to confirm the required Go version and go-ethereum dependency version grep -A 3 "go " go.mod grep -A 1 "github.com/ethereum/go-ethereum" go.modLength of output: 2630
Update: Verify Go Version Consistency
The integration tests workflow now specifies
go-version: 1.22, as noted on line 44 of.github/workflows/integration-tests.yml, to meet the compatibility requirements withgo-ethereumv1.13.14 (using the NibiruChain replacement). However, the module file (go.mod) currently declares the Go version as1.21. Please confirm that this discrepancy between the test environment (1.22) and the module declaration (1.21) is intentional and will not lead to any compatibility issues during build or testing.x/evm/precompile/funtoken.go (1)
113-117: Return type updated to match new precompile interface requirementsThe function signature now returns
NibiruCustomPrecompileinstead ofvm.PrecompiledContract. This aligns with the PR objectives to adapt Nibiru-specific precompiles to the new interface requirements in go-ethereum v1.13.14.The implementation of
precompileFunTokenalready satisfies this interface by providing the requiredAddress()method (see line 31)..github/workflows/simulation-tests.yml (1)
19-19: Consistent Go version downgrade across workflowsThe Go version has been downgraded from 1.24 to 1.22 across all simulation test jobs, maintaining consistency with other workflow files and ensuring compatibility with the go-ethereum v1.13.14 dependency.
Also applies to: 31-31, 43-43, 55-55
app/keepers.go (1)
362-362:✅ Verification successful
Refactored precompile initialization approach
The code now directly calls
precompile.InitPrecompiles()instead of usingapp.EvmKeeper.AddPrecompiles(). This change centralizes precompile management in theprecompilepackage instead of theEvmKeeper.Looking at the provided context,
InitPrecompilesnow handles registration of all custom precompiles (FunToken, Wasm, Oracle) with the appropriate EVM precompile maps.
🏁 Script executed:
#!/bin/bash # Verify the EvmKeeper no longer has the AddPrecompiles method grep -r "func.*AddPrecompiles" x/evm/ # Check the new approach in precompile.go grep -A 20 "func InitPrecompiles" x/evm/precompile/precompile.goLength of output: 705
Precompile Initialization Refactor Verified
- The EvmKeeper’s
AddPrecompilesmethod has been successfully removed.- The new approach in
precompile.InitPrecompiles(app.AppKeepers.PublicKeepers)correctly registers all custom precompiles (FunToken, Wasm, Oracle) across the supported EVM precompile maps.- This centralizes precompile registration within the
precompilepackage, aligning with the intended design change.app/evmante/evmante_handler_test.go (1)
31-36: Updated to use signed balance methodsThe change from
AddBalancetoAddBalanceSignedaligns with the PR objectives to update balance mutation logic to prevent overflow errors. This is part of the broader update to go-ethereum v1.13.14, which introduces safer handling of balance operations.x/evm/precompile/wasm.go (1)
117-125: Return type updated to NibiruCustomPrecompileThe return type change from
vm.PrecompiledContracttoNibiruCustomPrecompileenhances type safety and interface clarity. SinceprecompileWasmalready implements theAddress()method required byNibiruCustomPrecompile, this is a non-breaking change that better documents the contract's capabilities.x/evm/precompile/oracle.go (1)
74-78: Return type updated to NibiruCustomPrecompileConsistent with the pattern across precompiles, the return type change from
vm.PrecompiledContracttoNibiruCustomPrecompileprovides better type safety and explicitness. The implementation already satisfies the interface requirements, so this refactoring aligns with go-ethereum v1.13.14 updates without changing functionality.app/evmante/evmante_sigverify.go (1)
39-43: Updated MakeSigner to include block time parameterThe
MakeSignercall now includes block time information (evm.ParseBlockTimeUnixU64(ctx)), which is required by go-ethereum v1.13.14. This change ensures compatibility with the updated API and potentially enhances signature validation by incorporating timing information.CHANGELOG.md (1)
44-44: Appropriate changelog entry for a breaking feature.This changelog entry correctly indicates the EVM update to Geth v1.13 with appropriate features mentioned and the breaking change marker (!). The reference to EIP-1153 and transient storage matches the PR objectives.
app/evmante/evmante_verify_eth_acc_test.go (1)
24-24: Updated to use signed balance method in tests.The change from
AddBalancetoAddBalanceSignedaligns with Geth v1.13's updated StateDB methods for balance manipulation, which now use signed methods to align with EIP changes.x/evm/keeper/call_contract.go (1)
47-61: Updated Message creation to use new struct format with blob transaction support.This change adapts to go-ethereum v1.13's API by replacing the deprecated
NewMessageconstructor with direct struct initialization. The new fields (BlobGasFeeCap,BlobHashes) support EIP-4844 blob transactions, andSkipAccountChecksreplaces the oldisFakeparameter.app/evmante/evmante_gas_consume_test.go (1)
29-29: Consistent update to signed balance methods in tests.Changed from
AddBalancetoAddBalanceSignedto conform with Geth v1.13's updated StateDB API, maintaining consistency with similar changes in other test files.Also applies to: 50-50
app/evmante/evmante_increment_sender_seq_test.go (2)
25-25: Updated balance API for transaction tests.The change from
AddBalancetoAddBalanceSignedaligns with the PR's objective of revising balance mutation logic to prevent overflow errors in go-ethereum v1.13.14.
37-37: Updated balance API for transaction tests.Consistent update to use
AddBalanceSignedinstead ofAddBalance, ensuring proper balance management in the test scenarios.x/evm/const.go (2)
24-35: Added PRECOMPILE_ADDRS with proper duplicate handling.This implementation efficiently aggregates Ethereum precompiled addresses by using a set data structure to eliminate potential duplicates. The constant combines the standard Cancun precompiled addresses with Nibiru-specific ones (FunToken, Wasm, and Oracle).
183-183: Added Big0 constant for reusable zero big.Int value.Adding a reusable zero big.Int value improves performance by avoiding repeated creation of new big.Int instances with
big.NewInt(0).app/evmante/evmante_can_transfer.go (3)
28-32: Updated signer creation to include block time parameter.The
MakeSignerfunction now includes block time as an additional parameter, which is required for compatibility with go-ethereum v1.13.14. This ensures correct transaction signing behavior based on the chain's current block time.
71-83: Switched from method calls to direct field access for transaction details.Changed multiple method calls to direct field access for
ValueandFromfields, which aligns with the upstream go-ethereum v1.13.14 struct-based approach instead of the previous method-based access pattern.
64-64:❓ Verification inconclusive
Switched from method call to direct field access.
Changed from
evmMsg.GasFeeCap()to direct field accessevmMsg.GasFeeCap, consistent with upstream changes in go-ethereum v1.13.14. This line needs test coverage.This modification in the error message formatting should be covered by tests to ensure proper error reporting when gas fee cap is insufficient.
🏁 Script executed:
#!/bin/bash # Check for test coverage of error conditions in CanTransferDecorator echo "Checking for tests related to gas fee cap validation in CanTransferDecorator..." rg -A 5 -B 5 "CanTransferDecorator.*insufficient" --type=goLength of output: 230
Direct Field Access Update & Test Coverage Request
In
app/evmante/evmante_can_transfer.goat line 64, the code now accessesevmMsg.GasFeeCapdirectly (instead of callingevmMsg.GasFeeCap()) to align with upstream changes in go-ethereum v1.13.14.
- Action Required: Please add test coverage to verify that, when the gas fee cap is insufficient, the error message is generated appropriately. Our current search for tests (using
"CanTransferDecorator.*insufficient") did not yield any results, so manual verification is needed.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-64: app/evmante/evmante_can_transfer.go#L64
Added line #L64 was not covered by testsx/evm/chain_config.go (3)
15-32: Improved code efficiency with Big0 constantThe change from multiple
big.NewInt(0)instances to a singleBig0constant is a good optimization. This reduces memory allocations and improves code maintainability.
33-37: Added support for newer Ethereum forksThe addition of
ShanghaiTime,CancunTime,PragueTime, andVerkleTimeparameters correctly implements the fork activation logic required by go-ethereum v1.13.14. This ensures compatibility with the latest Ethereum protocol upgrades, particularly for EIP-1153 (transient storage) which was part of the Cancun upgrade.
44-47: Well-implemented utility functionThe
ptrU64helper function is a clean implementation for converting uint values to uint64 pointers needed by the updated go-ethereum API.x/evm/keeper/gas_fees.go (1)
153-160: Added EIP-3860 support for gas calculationsThe additional parameter
trueforisEIP3860(Shanghai) correctly updates the intrinsic gas calculation to be compatible with go-ethereum v1.13.14. This parameter is now required for alignment with the Shanghai upgrade which includes EIP-3860 (limit and meter initcode).x/evm/evmtest/tx.go (1)
363-375: Updated message creation to use struct-based initializationThe conversion from function-based (
gethcore.NewMessage) to struct-based initialization ofcore.Messagealigns with the upstream go-ethereum v1.13.14 API changes. This approach now explicitly initializes all required fields including the newSkipAccountChecksfield.x/evm/json_tx_args.go (2)
162-166: Updated return type and error handling for ToMessageThe method signature has been correctly updated to return
core.Messageinstead ofgeth.Message, maintaining compatibility with go-ethereum v1.13.14. The error handling has been appropriately adjusted to match the new return type.
232-247: Implemented struct-based message initializationThe message creation has been properly refactored to use the new struct-based initialization pattern with
core.Message. All necessary fields are correctly populated, including theSkipAccountChecksflag, which is important for controlled EVM message execution.x/evm/statedb/journal_test.go (4)
12-12: Use of uint256 import looks appropriate
The addition of"github.com/holiman/uint256"aligns with the new balance methods and integer types.
62-66: Confirm handling of signed balances
ReplacingAddBalancewithAddBalanceSignedchanges behavior for potentially negative or overflow scenarios. Kindly verify that all inputs and test scenarios cover these edge cases.
130-130: Switch from big.Int to uint256
Usinguint256.NewInt(0)is consistent with the new numeric type across the EVM. This looks good.
157-157: Consistency in numeric type usage
Continuing withuint256.NewInt(0)maintains consistency with the rest of the codebase.x/evm/keeper/funtoken_from_erc20.go (2)
13-13: New import for core package
Importing"github.com/ethereum/go-ethereum/core"is valid for accessing the newly introducedcore.Messagestruct.
139-151: Verify correctness of core.Message usage
Switching to thecore.Messagestruct (instead ofgethcore.NewMessage) is fine. Please verify that settingNonce,GasLimit, andSkipAccountChecksto these defaults suits all usage scenarios.x/evm/precompile/precompile.go (3)
42-42: Removal of map return in InitPrecompiles
The function no longer returns a map of precompiles. Ensure no downstream callers rely on the previous return value.
44-59: Insertion of custom precompiles into multiple vm.PrecompiledContracts maps
Modifying package-level maps in a loop is convenient. IfInitPrecompilesmight be called concurrently, confirm thread-safety, as these maps are shared ingo-ethereum/core/vm.
71-73: New NibiruCustomPrecompile interface
Extendingvm.PrecompiledContractwithAddress()clarifies how custom precompiles integrate. This separation of concerns is cleanly implemented.x/evm/keeper/funtoken_from_coin.go (2)
11-11: Added import for go-ethereum core package to support the new Message struct.This import is necessary to replace the function-based message creation with the struct-based approach used in newer geth versions.
83-96:❓ Verification inconclusive
Refactored message creation to use the new core.Message struct.
This change is part of upgrading to geth v1.13 and replaces the deprecated
gethcore.NewMessagefunction call with the struct-based approach. The functionality remains the same, but the implementation follows the new go-ethereum API.The use of
unusedBigIntfor multiple zero-value fields is an optimization that reduces unnecessary object creation.
🏁 Script executed:
#!/bin/bash # Check if this pattern is consistently applied across the codebase # Search for other instances of core.Message struct usage rg "core\.Message\s*{" --type goLength of output: 132
#!/bin/bash # Re-run the search with a corrected regex to verify consistent usage of core.Message struct rg "core\\.Message\\s*\\{" --type goRefactored Message Creation – Please Verify Consistent Usage
The updated code correctly constructs the new
core.Messagestruct for geth v1.13 upgrades, replacing the deprecated function call. The reuse ofunusedBigIntfor all zero-value fields is an intended optimization.Before final approval, please verify that this pattern is applied consistently across the repository by re-running the updated script above.
</details> <details> <summary>x/evm/vmtracer.go (3)</summary> `34-37`: **Updated message field access to match new core.Message struct pattern.** The code now directly accesses struct fields from the Message struct instead of calling methods. This is consistent with the changes observed in other files for the geth v1.13 upgrade. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 GitHub Check: codecov/patch</summary> [warning] 34-36: x/evm/vmtracer.go#L34-L36 Added lines #L34 - L36 were not covered by tests </details> </details> --- `93-93`: <details> <summary>✅ Verification successful</summary> **Simplified CaptureEnd signature by removing the unused time.Duration parameter.** This change aligns with the updated EVMLogger interface in geth v1.13. The method no longer requires the execution time parameter. --- 🏁 Script executed: ```shell #!/bin/bash # Check if all implementations of the EVMLogger interface have been updated rg "func\s*\(\w+\s*\w+\)\s*CaptureEnd\s*\(" --type goLength of output: 150
Review Resolution: The implementation of
CaptureEndinx/evm/vmtracer.gocorrectly reflects the updated interface: the unusedtime.Durationparameter has been removed, aligning with geth v1.13'sEVMLoggerinterface. The verification confirms the change is complete.
- Verified Change: Only
CaptureEnd(output []byte, gasUsed uint64, err error)is present in the repository.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: x/evm/vmtracer.go#L93
Added line #L93 was not covered by tests
32-32:✅ Verification successful
Simplified precompile address resolution by using the PRECOMPILE_ADDRS constant.
This change improves maintainability by using the centralized constant instead of calculating the precompile addresses dynamically, which aligns with the PR's goal of standardizing precompile management.
🏁 Script executed:
#!/bin/bash # Verify the definition of PRECOMPILE_ADDRS to ensure it's properly configured rg "PRECOMPILE_ADDRS\s*\=" --type go -A 10Length of output: 151
Action Required: Confirm Correct PRECOMPILE_ADDRS Configuration
The intent of replacing dynamic precompile address calculations with the centralized
PRECOMPILE_ADDRSconstant is sound for maintainability and consistency. However, our initial regex search encountered an escape sequence error. Please re-run the above script to verify the correct definition and configuration ofPRECOMPILE_ADDRSin the codebase. Once you confirm that this constant is properly defined (and that its usage inx/evm/vmtracer.goline 32 is appropriate), this change aligns well with the PR’s goal.
🏁 Script executed:
#!/bin/bash # Verify the definition of PRECOMPILE_ADDRS using a fixed string search for better compatibility rg --fixed-strings "PRECOMPILE_ADDRS" --type go -A 10Length of output: 1935
Precompile address resolution confirmed
The grep output verifies that the
PRECOMPILE_ADDRSconstant is properly defined inx/evm/const.goand is consistently used inx/evm/vmtracer.go(as well as in other parts of the codebase). This aligns with the intent to simplify and standardize precompile address management, improving maintainability.No further changes are required.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: x/evm/vmtracer.go#L32
Added line #L32 was not covered by tests
x/evm/statedb/journal.go (1)
364-383:
❓ Verification inconclusive
Added transientStorageChange type to implement EIP-1153 transient storage.
This new JournalChange implementation supports the transient storage feature from EIP-1153 mentioned in the PR description. The implementation follows the established pattern of other journal change types, with proper Revert and Dirtied method implementations.
The code looks correct, but I notice it's not covered by tests according to the coverage report.
Let's check if there are existing tests for other journal change types that could be used as patterns for testing this new type:
🏁 Script executed:
#!/bin/bash
# Look for test patterns that could be applied to the new transientStorageChange
rg "Test.*Journal" --type go
# Check for uses of this new type
rg "transientStorageChange" --type goLength of output: 716
Action: Ensure Explicit Testing for transientStorageChange
The implementation of transientStorageChange follows the established pattern by correctly implementing the Revert and Dirtied methods to support EIP-1153 transient storage. While its usage is evident in the state database (e.g., via s.Journal.append(transientStorageChange{...}) in x/evm/statedb/statedb.go), our grep results confirm that no tests explicitly reference transientStorageChange.
Although similar journal change types are indirectly exercised under tests like TestJournalReversion in x/evm/statedb/journal_test.go, the coverage report indicates that this new type isn’t directly tested. To ensure complete validation of its behavior, please verify that its functionality is sufficiently covered in existing tests or add dedicated tests mirroring the patterns found in the related journal change tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 377-378: x/evm/statedb/journal.go#L377-L378
Added lines #L377 - L378 were not covered by tests
[warning] 381-382: x/evm/statedb/journal.go#L381-L382
Added lines #L381 - L382 were not covered by tests
x/evm/statedb/state_object.go (6)
7-7: Added fmt import for the new String method.
This import is needed for the new Storage.String() method implemented below.
97-103: Added Copy method to Storage type for creating shallow copies.
This method enhances the Storage type by providing a clean way to create copies of storage, which is useful for state management operations like snapshotting.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-102: x/evm/statedb/state_object.go#L97-L102
Added lines #L97 - L102 were not covered by tests
105-110: Added String method to Storage type for debugging.
This method provides a formatted string representation of the storage entries, which is helpful for debugging and logging. The format uses uppercase hexadecimal representation for both keys and values.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-109: x/evm/statedb/state_object.go#L105-L109
Added lines #L105 - L109 were not covered by tests
142-143: Updated Suicided field description and added createdThisBlock tracking.
The added createdThisBlock field helps track state objects created within the current block, which can be important for certain EVM operations and state reverts.
147-152: Modified newObject to accept pointer to Account and handle nil case.
The function now accepts a nil account and initializes a new empty account in that case, while also setting the createdThisBlock flag accordingly. This improves the robustness of object creation and lifecycle tracking.
162-166: Updated stateObject initialization with new createdThisBlock field.
This change properly initializes the new field in the stateObject struct based on whether the account was created in this block (was nil in the function parameters).
x/evm/keeper/grpc_query.go (6)
382-396: Confirm correct reinitialization of message fields.
The code sets GasLimit = gas but retains other fields from evmMsg to build a new core.Message. This approach is valid for the binary search logic on gas usage. Ensure that no relevant fields from the original evmMsg are inadvertently lost, as that could affect subsequent transaction execution.
544-545: Check chain configuration consistency.
You are calling core.TransactionToMessage with evmCfg.BaseFeeWei. Verify that the chain ID, base fee, and London signer values all align with the network configuration. Inconsistent configurations might lead to incorrect gas pricing or signer mismatches.
549-550: No immediate concerns.
The call to k.TraceEthTxMsg is straightforward. Ensure you handle all returned errors properly, including tracer-related issues if they arise.
688-692: Ensure correct block-based signing.
Using gethcore.MakeSigner(evmCfg.ChainConfig, big.NewInt(ctx.BlockHeight()), evm.ParseBlockTimeUnixU64(ctx)) is valid. Just confirm that the block time or chain config changes do not break older or future-block transactions.
703-704: Message creation call looks consistent.
The logic to create the core.Message from ethTx uses the same base fee. No further issues found here.
708-709: Trace function usage appears correctly integrated.
The TraceEthTxMsg usage aligns with the updated message structure and config. No concerns noted.
x/evm/keeper/msg_server.go (3)
50-53: Confirm chain ID parameter for the London signer.
Using NewLondonSigner(evmCfg.ChainConfig.ChainID) in TransactionToMessage aligns with the go-ethereum approach. Just ensure the chain ID is the correct one for your environment, otherwise transaction signatures could be invalid.
66-70: Initialization of EVM with the updated message is valid.
Passing *evmMsg to ApplyEvmMsg is consistent with the rest of the code's usage. Ensure the stateDB is correctly set up before calling ApplyEvmMsg to avoid partial commits.
95-95: Event emission approach is consistent.
Emitting events with EmitEthereumTxEvents after the EVM execution is standard. No additional concerns.
x/evm/statedb/statedb_test.go (5)
75-76: Balanced usage of uint256.NewInt(0).
This ensures a zero balance check for new or empty accounts. No issues found here.
92-93: Consistent with zero-balance checks.
Confirms that post-commit or newly created accounts have a zero balance. Looks good.
106-106: Use of AddBalanceSigned in tests.
Adjusting the account’s balance with the new AddBalanceSigned method is correct. This ensures overflow checks for negative or exceedingly large values are tested. Nothing to change.
116-116: Confirm suicided state transitions.
Verifying that an account is indeed marked suicided post-commit is appropriate. This test coverage helps ensure correct contract termination logic.
120-120: Accurate zeroing of balances after selfdestruct.
Ensuring the test confirms a zero balance after self-destruction is an important detail and indeed verifies the intangible behavior of EVM self-destruct semantics.
x/evm/statedb/statedb.go (5)
418-436: SelfDestruct behavior clarifications.
After marking the account as suicided, the code zeroes out its balance. This aligns well with standard EVM semantics. However, be sure that any final settlement logic (if required by your chain) occurs before the balance is cleared.
447-454: Conditionally calling SelfDestruct for EIP-6780.
Selfdestruct6780 only performs self-destruction if the contract was created this block. This forks the logic from standard EVM. Make sure your code and tests handle these special-casing rules accurately.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 447-454: x/evm/statedb/statedb.go#L447-L454
Added lines #L447 - L454 were not covered by tests
701-705: Transient storage initialization.
Creating a new map entry in transientStorage if missing is correct for EIP-1153. Check for possible duplicates or partial overwrites if a key is set multiple times in a short scope.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 701-705: x/evm/statedb/statedb.go#L701-L705
Added lines #L701 - L705 were not covered by tests
709-714: Transient storage lookups handle missing addresses gracefully.
Returning an empty common.Hash{} if no entry is found is consistent. No issues found here.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 709-714: x/evm/statedb/statedb.go#L709-L714
Added lines #L709 - L714 were not covered by tests
730-731: Defaulting to an empty hash.
Returning an empty hash if transient data is absent aligns with typical EVM fallback semantics. Implementation is accurate.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 730-731: x/evm/statedb/statedb.go#L730-L731
Added lines #L730 - L731 were not covered by tests
This upgrade bumps Nibiru's Geth dependency from v1.13 to v1.14.13 and updates the Go version to 1.24 across the codebase and CI workflows. The upgrade is motivated by the need to support modern database backends (e.g., Cockroach Pebble), leverage recent Go generics improvements, and remain aligned with upstream changes in the Geth EVM architecture. Key changes: - Refactored EVM backend logic to use Geth's new `core.Block` constructor signature, which requires `core.Body`, `Receipts`, and `TrieHasher` explicitly. - Migrated `TraceTransaction`, `TraceCall`, and `TraceBlock` methods to return `json.RawMessage` as required by Geth's new trace API interfaces. - Replaced `go-ethereum/log` with the new `log/slog` stdlib implementation, per Geth v1.14’s logging changes. - Introduced `SuccessfulTx` struct in EVM tests to simplify managing and reusing stateful block hashes and numbers across assertions. - Updated all Dockerfile and CI workflows to Go 1.24 to ensure compatibility. - Disabled the `revive` linter’s `exported` rule for smoother CI linting. Compatibility notes: - Nibiru remains on the Berlin hard fork to avoid breaking changes from Ethereum’s blob transactions (Cancun) and Verkle tree requirements that are not applicable to Nibiru’s consensus and storage design. - Geth v1.14 is the last compatible version before Cancun-related features become mandatory. BREAKING CHANGE: Trace API return types and internal block construction logic have changed to comply with Geth v1.14. Any downstream tools or integrations depending on older behaviors may need to be updated.
Unique-Divine
changed the title
There was a problem hiding this comment.
Because it was outdated and not adding much value at all
There was a problem hiding this comment.
The "misspell" linter was in conflict with the use of British English in imports from go-ethereum. It kept renaming StateDB functions and breaking interface compatibility
There was a problem hiding this comment.
The gethcore.Transaction now has a mutex field on it that prevents it from being copied by value, so a few functions needed to be updated to have pointer receivers
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
x/evm/keeper/grpc_query.go (1)
597-600:⚠️ Potential issueBase‑fee is stored in the wrong unit (micronibi ≠ wei).
BaseFeeMicronibiPerGasreturns a micronibi amount, but the field you overwrite (evmCfg.BaseFeeWei) must always be wei. Storing micronibi here makes the EVM believe gas is 10¹² times cheaper than it really is, breaking intrinsic‑gas checks and any fee calculations performed in the VM and downstream JSON‑RPC consumers.- baseFeeMicronibi := k.BaseFeeMicronibiPerGas(ctx) - if baseFeeMicronibi != nil { - evmCfg.BaseFeeWei = baseFeeMicronibi - } + baseFeeMicronibi := k.BaseFeeMicronibiPerGas(ctx) + if baseFeeMicronibi != nil { + baseFeeWei := evm.NativeToWei(baseFeeMicronibi) + evmCfg.BaseFeeWei = baseFeeWei + }
🧹 Nitpick comments (13)
.golangci.yml (1)
22-25: Reconsider globally disablingrevive’sexportedruleTurning the rule off project‑wide means newly exported functions, types, and packages can slip in without any doc‑comments, eroding API discoverability over time. If the intent is to silence a handful of false positives, prefer either:
- Disabling the rule per‑file / per‑line with
//revive:disable:exported … //revive:enable:exported, or- Using
revive’s file‑pattern exclude list to narrow the scope.Keeping the rule on by default preserves valuable documentation guard‑rails.
CHANGELOG.md (1)
45-53: Fix minor Markdown lint & typo issues
markdownlintflags the new bullet entries for wrong indent (MD007) and there’s a small spelling mistake:- - This upgrade keeps Nibiru's EVM on the Berlin upgrade to avoid - incompatibilities stemming from functionality specific to Ethereum's consesnus + - This upgrade keeps Nibiru's EVM on the Berlin upgrade to avoid + incompatibilities stemming from functionality specific to Ethereum's consensusIndent with two spaces so child bullets align with the parent
- [#2275]item, and correct “consesnus → consensus”.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
48-48: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
eth/rpc/backend/blocks.go (1)
490-492: TODOs should include more specific action items.The current TODOs are informative but could be more actionable by including specific steps or design decisions for adding tx receipts and simulating Trie behavior.
Consider enhancing the TODOs with more details:
-// TODO: feat(evm-backend): Add tx receipts in gethcore.NewBlock for the -// EthBlockFromTendermintBlock function. -// TODO: feat: See if we can simulate Trie behavior on CometBFT. +// TODO(evm-backend): Add tx receipts in gethcore.NewBlock by implementing +// a receipt generation mechanism that builds receipts from transaction execution results. +// TODO(evm-backend): Investigate implementing a custom TrieHasher that maps CometBFT's +// storage model to Ethereum's state trie for more accurate block representation.eth/rpc/backend/tracing_test.go (2)
53-60: Add coverage for the default tracer inTestTraceTransaction.You already have helper configs for both
callTracerand the default tracer, but the default tracer path is commented‑out. Including it in the test loop (similar toTestTraceBlock) guarantees we exercise both code paths and prevents silent regressions when the default tracer evolves.- res, err := s.backend.TraceTransaction( - tc.txHash, - traceConfigCallTracer(), - // traceConfigDefaultTracer(), - ) + res, err := s.backend.TraceTransaction( + tc.txHash, + traceConfigCallTracer(), // keep + // also run with the default tracer + )Consider parameterising the test cases (like you did for
TraceBlock) or running the sub‑test twice with each tracer config.
53-68: Enablet.Parallel()inside sub‑tests for faster execution.Each sub‑test (
s.Run(...)) is independent and performs only read‑only RPC calls. Callingt.Parallel()(ors.T().Parallel()inside the closure) will cut CI time for large suites without behavioural changes.for _, tc := range testCases { s.Run(tc.name, func() { + s.T().Parallel()x/evm/statedb/statedb_test.go (1)
49-57: Unsafe type assertion in helper may panic
CollectContractStorageunconditionally castsvm.StateDBto*statedb.StateDB.
If this helper is ever reused with another implementation ofvm.StateDB, the test will panic.- sdb := db.(*statedb.StateDB) + sdb, ok := db.(*statedb.StateDB) + if !ok { + return nil // or t.Fatalf("expected *statedb.StateDB, got %T", db) + }While it is safe in the current test suite, a defensive check makes the helper future‑proof.
eth/rpc/backend/backend_suite_test.go (2)
80-99: Network height wait loop lacks upper‑bound guard
WaitForHeight(10)is followed by anotherWaitForNextBlock()after funding.
If the node stalls the suite may hang indefinitely.Consider adding a context timeout:
- _, err = s.network.WaitForHeight(10) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + _, err = s.network.WaitForHeightContext(ctx, 10)Also propagate the error instead of ignoring it in later calls to
WaitForNextBlock.
150-155: Nil‑pointer risk in log helper
SuccessfulTx.LogcallstestTx.BlockHash.Hex()without nil‑checking
BlockHash. Although the current call‑sites guarantee non‑nil, adding a guard
prevents accidental panics:-func (testTx SuccessfulTx) Log(t *testing.T) { - t.Logf("SuccessfulTx{BlockNumber: %s, BlockHash: %s, TxHash: %s}", - testTx.BlockNumber, testTx.BlockHash.Hex(), testTx.Receipt.TxHash.Hex()) +func (testTx SuccessfulTx) Log(t *testing.T) { + var blkHash string + if testTx.BlockHash != nil { + blkHash = testTx.BlockHash.Hex() + } + t.Logf("SuccessfulTx{BlockNumber: %s, BlockHash: %s, TxHash: %s}", + testTx.BlockNumber, blkHash, testTx.Receipt.TxHash.Hex()) }x/evm/statedb/journal.go (3)
75-81:journal.dirtyside‑effects bypass change tracking
dirtyincrements the dirties counter without adding a corresponding
JournalChange, meaning a laterRevertcannot undo this mutation.
Double‑check that callers reset the counter manually when snapshot‑reverting
(or consider journaling a dedicateddirtyChange).
169-179: Potential nil‑dereference inbalanceChange.Revert
prevWeiis dereferenced without a nil check. If any caller constructs
balanceChangewith a nilprevWeithe revert will panic.func (ch balanceChange) Revert(s *StateDB) { if ch.prevWei == nil { return } s.getStateObject(*ch.account).setBalance(ch.prevWei.ToBig()) }
232-241:storageChange.Revertignores error propagation
setStatecan fail (e.g., out‑of‑gas when journalling). Errors are currently
silently dropped, which could hide state‑corruption during complex reverts.
Consider returning an error fromJournalChange.Revertor at least logging the
failure.x/evm/keeper/grpc_query.go (1)
383-398: Re‑buildingcore.Messageeach iteration: risk of state leakage / field drift.You overwrite
evmMsgon every binary‑search iteration.
Although functional, this:
- Obscures intent – a reader must diff every field to spot the single one that changes.
- Becomes brittle when new fields are added to
core.Message(they’ll default to zero here).A minimal pattern is easier to reason about and future‑proof:
- evmMsg = core.Message{ - To: evmMsg.To, - From: evmMsg.From, - Nonce: evmMsg.Nonce, - Value: evmMsg.Value, - GasLimit: gas, - ... - } + evmMsg.GasLimit = gasIf immutability is required, make a shallow copy then mutate
GasLimitonly.x/evm/statedb/state_object.go (1)
107-111:Storage.String()is O(n²) – usestrings.Builderfor large contracts.For hot‑path debugging utilities, building the string via repeated concatenation reallocates each time.
-func (s Storage) String() (str string) { - for key, value := range s { - str += fmt.Sprintf("%X : %X\n", key, value) - } - return +func (s Storage) String() string { + var b strings.Builder + for key, value := range s { + fmt.Fprintf(&b, "%X : %X\n", key, value) + } + return b.String() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contrib/scripts/e2e/contracts/cw_nameservice.wasmis excluded by!**/*.wasmgo.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
.github/workflows/e2e-wasm.yml(0 hunks).golangci.yml(1 hunks)CHANGELOG.md(1 hunks)Dockerfile(1 hunks)app/evmante/evmante_verify_eth_acc.go(1 hunks)app/keepers.go(1 hunks)app/server/geth_log_handler.go(1 hunks)contrib/scripts/e2e/deploy-wasm.sh(0 hunks)eth/rpc/backend/account_info_test.go(4 hunks)eth/rpc/backend/backend_suite_test.go(7 hunks)eth/rpc/backend/blocks.go(1 hunks)eth/rpc/backend/blocks_test.go(4 hunks)eth/rpc/backend/nonce_test.go(1 hunks)eth/rpc/backend/tracing.go(5 hunks)eth/rpc/backend/tracing_test.go(6 hunks)eth/rpc/backend/tx_info_test.go(4 hunks)eth/rpc/backend/utils_test.go(1 hunks)eth/rpc/rpc.go(2 hunks)eth/rpc/rpcapi/eth_filters_api.go(1 hunks)evm-e2e/test/debug_queries.test.ts(2 hunks)evm-e2e/test/wrapped_nibiru_wnibi.test.ts(1 hunks)go.mod(12 hunks)x/common/nmath/nmath.go(1 hunks)x/evm/chain_config.go(1 hunks)x/evm/const.go(4 hunks)x/evm/evmtest/test_deps.go(2 hunks)x/evm/evmtest/tx.go(2 hunks)x/evm/json_tx_args.go(4 hunks)x/evm/keeper/call_contract.go(2 hunks)x/evm/keeper/funtoken_from_coin.go(2 hunks)x/evm/keeper/funtoken_from_erc20.go(2 hunks)x/evm/keeper/grpc_query.go(13 hunks)x/evm/keeper/grpc_query_test.go(1 hunks)x/evm/keeper/keeper.go(0 hunks)x/evm/keeper/msg_server.go(13 hunks)x/evm/keeper/statedb.go(3 hunks)x/evm/keeper/vm_config.go(2 hunks)x/evm/msg_test.go(1 hunks)x/evm/precompile/funtoken.go(4 hunks)x/evm/precompile/oracle.go(3 hunks)x/evm/precompile/wasm.go(4 hunks)x/evm/statedb/journal.go(6 hunks)x/evm/statedb/journal_test.go(5 hunks)x/evm/statedb/state_object.go(12 hunks)x/evm/statedb/statedb.go(15 hunks)x/evm/statedb/statedb_test.go(11 hunks)x/evm/tx_data_dynamic_fee.go(2 hunks)x/evm/vmtracer.go(3 hunks)
💤 Files with no reviewable changes (3)
- contrib/scripts/e2e/deploy-wasm.sh
- .github/workflows/e2e-wasm.yml
- x/evm/keeper/keeper.go
✅ Files skipped from review due to trivial changes (7)
- evm-e2e/test/wrapped_nibiru_wnibi.test.ts
- evm-e2e/test/debug_queries.test.ts
- Dockerfile
- x/common/nmath/nmath.go
- x/evm/tx_data_dynamic_fee.go
- app/keepers.go
- x/evm/msg_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- x/evm/keeper/funtoken_from_erc20.go
- x/evm/evmtest/tx.go
- x/evm/statedb/journal_test.go
- x/evm/json_tx_args.go
- x/evm/chain_config.go
- x/evm/keeper/vm_config.go
- x/evm/keeper/call_contract.go
- x/evm/keeper/funtoken_from_coin.go
- x/evm/precompile/oracle.go
- x/evm/precompile/funtoken.go
- x/evm/precompile/wasm.go
- x/evm/const.go
- go.mod
- x/evm/statedb/statedb.go
🧰 Additional context used
🧠 Learnings (2)
x/evm/evmtest/test_deps.go (4)
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns only an EVM object and does not return an error. The method is used in test files to create a new EVM instance for testing purposes.
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
x/evm/keeper/msg_server.go (1)
Learnt from: Unique-Divine
PR: NibiruChain/nibiru#2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
🧬 Code Graph Analysis (4)
app/evmante/evmante_verify_eth_acc.go (1)
x/evm/const.go (1)
NativeToWei(123-126)
eth/rpc/rpc.go (1)
x/common/nmath/nmath.go (1)
BigMin(6-11)
eth/rpc/backend/blocks_test.go (1)
eth/rpc/backend/backend_suite_test.go (1)
BackendSuite(50-61)
app/server/geth_log_handler.go (1)
x/common/testutil/testnetwork/logger.go (1)
Logger(13-16)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
48-48: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (34)
eth/rpc/rpc.go (1)
18-18: LGTM! Clean transition from external to internal math utility.The change correctly replaces the external
gethmathdependency with the localnmathimplementation for theBigMinfunction. The implementation innmath.BigMincorrectly handles the minimum calculation for*big.Intvalues, maintaining the same functionality while reducing external dependencies.Also applies to: 228-228
x/evm/keeper/grpc_query_test.go (1)
811-811: Enhances JSON validation for trace responseGood addition of JSON validation to ensure the trace response data is properly formatted after the geth v1.14 upgrade. This helps verify that the new tracer implementation produces valid JSON output that client applications can properly parse.
app/evmante/evmante_verify_eth_acc.go (1)
74-74: Updated balance conversion for compatibility with uint256.IntThis change properly adapts the code to work with the new balance type system. You're now converting from the new
uint256.Inttype to*big.Intusing the.ToBig()method before passing it toNativeToWei, which aligns with the PR's goal of transitioning touint256.Intfor balance handling.eth/rpc/backend/utils_test.go (1)
16-16: Improved test stability with dynamic block numbersGreat improvement to use a dynamic block number from a successful contract deployment rather than a hard-coded value. This makes the test more resilient to changes in the test environment and setup.
eth/rpc/backend/account_info_test.go (3)
24-25: Enhanced test robustness with dynamic block numbersGood replacement of static block numbers with dynamically retrieved ones from successful transactions. This approach makes the tests more maintainable and less likely to break when the test environment changes.
Also applies to: 30-31
65-66: Improved test consistency with dynamic block referencesWell done replacing hard-coded block numbers with dynamically retrieved values from contract deployment transactions. This ensures test consistency across different test runs and environments.
Also applies to: 73-74
107-108: Better test maintainability with dynamic block referencesGood practice to use dynamically obtained block numbers rather than hard-coded values. This makes tests more resilient to changes in the test setup and improves overall maintainability.
Also applies to: 117-118
x/evm/evmtest/test_deps.go (1)
52-59: Improved tracing configuration with structured loggerThe addition of a structured logger with debug mode enabled enhances EVM test observability. This implementation aligns with the PR's broader updates to the tracing infrastructure and go-ethereum v1.14 integration.
The commented-out JSON logger provides a good alternative if more detailed debug output becomes necessary in the future.
eth/rpc/backend/tx_info_test.go (3)
24-24: Good improvement: Dynamic transaction referencesReplacing the static
transferTxHashwith a dynamic reference tos.SuccessfulTxTransfer().Receipt.TxHashimproves test reliability and maintainability.
131-131: Consistent dynamic transaction reference patternUsing dynamic transaction references in
AssertTxResultsensures test consistency and reduces maintenance overhead.
172-172: Consistent usage of dynamic transaction referencesGood consistency in using the same dynamic reference pattern throughout the tests.
eth/rpc/rpcapi/eth_filters_api.go (1)
637-637: Simplified filter type handlingRemoving the
MinedAndPendingLogsSubscriptioncase in favor of just handlingLogsSubscriptionaligns with go-ethereum v1.14 updates. This simplification helps maintain compatibility with the latest go-ethereum core packages.eth/rpc/backend/blocks_test.go (6)
28-28: Updated type assertion to modern Go idiomReplacing
[]interface{}with[]anyimproves code readability while maintaining the same functionality.
35-35: Dynamic block hash referenceGood replacement of static block hash with a dynamic reference from the successful transaction fixture.
58-58: Consistent use of dynamic block hashUsing the transaction's block hash from suite state maintains consistency across tests.
96-96: Dynamic block hash for test consistencyUsing the dynamic block hash reference ensures tests are resilient to test environment changes.
105-105: Modernized function signatureUpdated parameter type from
map[string]interface{}to the more idiomaticmap[string]any.
107-107: Consistent type assertion updateUsing
[]anyhere makes the type assertions consistent throughout the codebase.x/evm/keeper/statedb.go (3)
9-9: Added uint256 library for efficient balance handling.The addition of the
uint256package is aligned with the EVM 1.14 update and enhances performance for handling 256-bit integers common in Ethereum.
133-133: Updated balance conversion in SetAccBalance.Properly converts the uint256.Int balance back to big.Int when interfacing with the Cosmos SDK bank module, maintaining type consistency across module boundaries.
34-34:✅ Verification successful
Improved balance type conversion to align with geth v1.14.
Replaced direct
big.Intusage withuint256.MustFromBigfor balance representation, which is more efficient for EVM operations and aligns with go-ethereum's internal type system.Note that
MustFromBigwill panic if the input value doesn't fit in 256 bits. While highly unlikely for valid Ethereum balances, it's important to verify this never happens in production:
🏁 Script executed:
#!/bin/bash # Check for any error handling around MustFromBig calls rg -A 3 -B 3 "MustFromBig" --type goLength of output: 1476
🏁 Script executed:
#!/bin/bash # Locate the definition and usages of GetEvmGasBalance rg -n "GetEvmGasBalance" --type goLength of output: 442
MustFromBig usage verified safe—no overflow risk for EVM balances
uint256.MustFromBigis already used without error handling in:
- x/evm/statedb/state_object.go
- x/evm/const.go
- x/evm/statedb/statedb_test.go
GetEvmGasBalancereturns ansdk.Coin.Amountas a*big.Int, which is always non‑negative and bounded by the chain’s total token supply—orders of magnitude below 2^256.- Given existing widespread usage and on‑chain supply limits, a panic from overflow is effectively impossible in production.
No changes required.
eth/rpc/backend/blocks.go (2)
490-497: Improved block construction using structured gethcore.Body.The code now uses a
gethcore.Bodystruct to hold block components (transactions, uncles, withdrawals), which aligns with go-ethereum v1.14's block construction API and improves the structure of the code.
499-503: Updated block creation with explicit hasher.This change properly initializes the block creation parameters with an explicit TrieHasher, which is required by go-ethereum v1.14. The hasher is created using
trie.NewStackTrie(nil), which is the appropriate implementation for this context.eth/rpc/backend/nonce_test.go (3)
44-44: Updated transaction handling to use pointer semantics.The change from iterating over value types to pointer types aligns with go-ethereum v1.14's transaction handling, which now consistently uses pointer semantics for
gethcore.Transactionobjects.
52-52: Updated method signature to use transaction pointers.Changed
buildSDKTxWithEVMMessagesto accept variadic transaction pointers, ensuring consistency with go-ethereum v1.14's API changes and improving performance by avoiding unnecessary copying of transaction objects.
56-57: Simplified transaction conversion.The transaction conversion now works directly with pointers, which is more efficient and aligns with go-ethereum v1.14's updated API for transaction handling.
eth/rpc/backend/tracing.go (5)
21-24: Improved function signature with named return variables.Using named return variables (
res json.RawMessage, err error) enhances code clarity and helps prevent variable shadowing, which is particularly important in complex methods like transaction tracing.
77-83: Simplified loop construct for better readability.The loop now directly iterates over the range of messages by index, making the code more concise and easier to follow.
116-123: Enhanced contextual comments and improved height calculation.Added clear explanations about trace context height calculation and used the
maxfunction to ensure the context height is at least 1, preventing issues with special values inrpc.ContextWithHeight.
130-135: Simplified JSON unmarshalling and return logic.Streamlined the JSON unmarshalling directly into the named return variable and simplified the return statement, making the code more concise without changing its behavior.
254-262: Refactored TraceCall response handling.Simplified JSON unmarshalling and return in
TraceCallmethod to match the pattern used inTraceTransaction, improving consistency across tracing methods.x/evm/statedb/statedb_test.go (1)
224-233: Overflow test could yield false‑negative
xcommon.TryCatchwraps the panic, returningnilwhen no panic occurs.
IfAddBalanceSignedever returns an error instead of panicking for
overflow, the test will silently pass.Recommend asserting that a panic or an explicit error is raised:
maybeErr := xcommon.TryCatch(func() { db.AddBalanceSigned(address, tooBig) })() s.Require().NotNil(maybeErr, "expected overflow to be reported")Also applies to: 240-249
eth/rpc/backend/backend_suite_test.go (1)
215-235: Busy‑wait loop may swallow retriable errors
WaitForReceiptreturnsnil, nil, nilon any error from
GetTransactionReceipt, discarding network glitches that could be retried.
Additionally, the loop sleeps unconditionally even after an error.Recommend:
- Distinguish “receipt not found” (nil receipt, nil error) from real errors.
- Back‑off or fail fast on non‑transient errors.
x/evm/keeper/grpc_query.go (1)
796-813:tracermay still benil– extra guard required.
tracers.DefaultDirectory.Newcan (rarely) return aniltracer together witherr == nil(e.g. an unrecognised JS tracer string).
WhenusingCallTracer == truethe subsequent fallback does not run, andtracer.Hookswill panic.Consider a second‐level check:
tracer, err = tracers.DefaultDirectory.New( traceConfig.Tracer, tCtx, tracerJSONConfig, evmCfg.ChainConfig, ) if err != nil { return nil, 0, grpcstatus.Error(grpccodes.Internal, err.Error()) } + if tracer == nil { // safety‑net + return nil, 0, grpcstatus.Error( + grpccodes.Internal, + fmt.Sprintf("failed to initialise tracer %q", traceConfig.Tracer), + ) + }Ensure existing tests cover this panic path; otherwise add one (can use an invalid tracer name to trigger).
| cmtlog "github.com/cometbft/cometbft/libs/log" | ||
| gethlog "github.com/ethereum/go-ethereum/log" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
gethlog import becomes unused after level‑mapping fix
Once the switch statement stops referencing gethlog.Level* constants (see next comment) the gethlog import is redundant and should be dropped to keep go vet happy.
-import (
- …
- gethlog "github.com/ethereum/go-ethereum/log"
-)
+import (
+ …
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmtlog "github.com/cometbft/cometbft/libs/log" | |
| gethlog "github.com/ethereum/go-ethereum/log" | |
| ) | |
| import ( | |
| cmtlog "github.com/cometbft/cometbft/libs/log" | |
| ) |
| switch r.Level { | ||
| // Check against Geth's custom levels first if they exist | ||
| // This handler covers all defined slog and go-ethereum log levels. | ||
| case gethlog.LevelTrace, slog.LevelDebug: // Handles -8, -4 | ||
| logFunc = h.CmtLogger.Debug | ||
| case slog.LevelInfo, slog.LevelWarn: // Handles 0, 4 | ||
| logFunc = h.CmtLogger.Info | ||
| case gethlog.LevelCrit, slog.LevelError: // Handles 12, 8 | ||
| // Map Geth Crit level along with standard Error | ||
| logFunc = h.CmtLogger.Error | ||
| default: // Handle any other levels (e.g., below Debug) | ||
| logFunc = h.CmtLogger.Debug // Default to Debug or Info as appropriate | ||
| } |
There was a problem hiding this comment.
Compile‑time type mismatch between slog.Level and gethlog.Lvl
r.Level is of type slog.Level, whereas gethlog.LevelTrace / LevelCrit are of the distinct type gethlog.Lvl.
The comparison in the switch therefore fails to compile:
cannot use gethlog.LevelTrace (untyped int constant) as type slog.Level in case expression
A simple fix is to map purely on slog.Level:
- switch r.Level {
- case gethlog.LevelTrace, slog.LevelDebug: // Handles -8, -4
- logFunc = h.CmtLogger.Debug
- case slog.LevelInfo, slog.LevelWarn: // Handles 0, 4
- logFunc = h.CmtLogger.Info
- case gethlog.LevelCrit, slog.LevelError: // Handles 12, 8
- logFunc = h.CmtLogger.Error
- default:
- logFunc = h.CmtLogger.Debug
- }
+ switch {
+ case r.Level < slog.LevelInfo: // Debug & anything more verbose
+ logFunc = h.CmtLogger.Debug
+ case r.Level < slog.LevelError: // Info & Warn
+ logFunc = h.CmtLogger.Info
+ default: // Error and above
+ logFunc = h.CmtLogger.Error
+ }This removes the cross‑package constant dependency, compiles cleanly, and still preserves the intended level mapping.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch r.Level { | |
| // Check against Geth's custom levels first if they exist | |
| // This handler covers all defined slog and go-ethereum log levels. | |
| case gethlog.LevelTrace, slog.LevelDebug: // Handles -8, -4 | |
| logFunc = h.CmtLogger.Debug | |
| case slog.LevelInfo, slog.LevelWarn: // Handles 0, 4 | |
| logFunc = h.CmtLogger.Info | |
| case gethlog.LevelCrit, slog.LevelError: // Handles 12, 8 | |
| // Map Geth Crit level along with standard Error | |
| logFunc = h.CmtLogger.Error | |
| default: // Handle any other levels (e.g., below Debug) | |
| logFunc = h.CmtLogger.Debug // Default to Debug or Info as appropriate | |
| } | |
| switch { | |
| case r.Level < slog.LevelInfo: // Debug & anything more verbose | |
| logFunc = h.CmtLogger.Debug | |
| case r.Level < slog.LevelError: // Info & Warn | |
| logFunc = h.CmtLogger.Info | |
| default: // Error and above | |
| logFunc = h.CmtLogger.Error | |
| } |
| precompileAddrs := PRECOMPILE_ADDRS | ||
| return logger.NewAccessListTracer( | ||
| msg.AccessList(), | ||
| msg.From(), | ||
| *msg.To(), | ||
| msg.AccessList, | ||
| msg.From, | ||
| *msg.To, | ||
| precompileAddrs, | ||
| ) | ||
| ).Hooks() | ||
| case TracerJSON: |
There was a problem hiding this comment.
Nil‑pointer panic when tracing contract‑creation txs.
*msg.To is dereferenced unconditionally. For contract‑creation messages msg.To == nil, so the TracerAccessList branch panics.
- return logger.NewAccessListTracer(
- msg.AccessList,
- msg.From,
- *msg.To, // <-- may panic
- precompileAddrs,
- ).Hooks()
+ var toAddr gethcommon.Address
+ if msg.To != nil {
+ toAddr = *msg.To
+ }
+ return logger.NewAccessListTracer(
+ msg.AccessList,
+ msg.From,
+ toAddr,
+ precompileAddrs,
+ ).Hooks()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| precompileAddrs := PRECOMPILE_ADDRS | |
| return logger.NewAccessListTracer( | |
| msg.AccessList(), | |
| msg.From(), | |
| *msg.To(), | |
| msg.AccessList, | |
| msg.From, | |
| *msg.To, | |
| precompileAddrs, | |
| ) | |
| ).Hooks() | |
| case TracerJSON: | |
| precompileAddrs := PRECOMPILE_ADDRS | |
| // Guard against nil msg.To for contract‑creation transactions | |
| var toAddr gethcommon.Address | |
| if msg.To != nil { | |
| toAddr = *msg.To | |
| } | |
| return logger.NewAccessListTracer( | |
| msg.AccessList, | |
| msg.From, | |
| toAddr, | |
| precompileAddrs, | |
| ).Hooks() | |
| case TracerJSON: |
| {"suicide", func(deps *evmtest.TestDeps, db *statedb.StateDB) { | ||
| // non-exist account. | ||
| s.Require().False(db.Suicide(address)) | ||
| s.Require().False(db.HasSuicided(address)) | ||
| db.SelfDestruct(address) | ||
| s.Require().False(db.HasSuicided(address)) | ||
|
|
||
| // create a contract account |
There was a problem hiding this comment.
Out‑of‑date API call will fail to compile
HasSuicided was renamed to HasSelfDestructed, however several assertions here still use the old name.
Trying to run go test will result in an “undefined: (*StateDB).HasSuicided” compilation error.
- s.Require().False(db.HasSuicided(address))
+ s.Require().False(db.HasSelfDestructed(address))
...
- db.SelfDestruct(address)
- s.Require().False(db.HasSuicided(address))
+ db.SelfDestruct(address)
+ s.Require().False(db.HasSelfDestructed(address))Do the same replacement for the check after the second SelfDestruct call.
Also applies to: 112-118
| func (s *stateObject) AddBalance(amount *uint256.Int) (prevWei uint256.Int) { | ||
| if amount.Sign() == 0 { | ||
| return | ||
| if s.isEmpty() { | ||
| s.touch() | ||
| } | ||
| return *(s.Balance()) | ||
| } | ||
| s.SetBalance(new(big.Int).Add(s.Balance(), amount)) | ||
| return s.SetBalance(new(big.Int).Add(s.Balance().ToBig(), amount.ToBig())) | ||
| } | ||
|
|
||
| // SubBalance removes amount from s's balance. | ||
| // It is used to remove funds from the origin account of a transfer. | ||
| func (s *stateObject) SubBalance(amount *big.Int) { | ||
| func (s *stateObject) SubBalance(amount *big.Int) (prevWei uint256.Int) { | ||
| if amount.Sign() == 0 { | ||
| return | ||
| return *s.Balance() | ||
| } | ||
| s.SetBalance(new(big.Int).Sub(s.Balance(), amount)) | ||
| return s.SetBalance(new(big.Int).Sub(s.Balance().ToBig(), amount)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent parameter types & missing under‑/overflow checks in balance mutations.
Problems
AddBalanceaccepts*uint256.Int, whereasSubBalanceaccepts*big.Int.
Call‑sites now need to know which flavour to pass – error‑prone and surprising.SubBalancecan create a negative*big.Int;uint256.MustFromBigwill panic on the nextSetBalance.- No guard against
amount > balancein either path; silent wrap‑around is possible.
Fix
-func (s *stateObject) AddBalance(amount *uint256.Int) (prevWei uint256.Int) {
+func (s *stateObject) AddBalance(amount *big.Int) (prevWei uint256.Int) {
...
- return s.SetBalance(new(big.Int).Add(s.Balance().ToBig(), amount.ToBig()))
+ newBal := new(big.Int).Add(s.Balance().ToBig(), amount)
+ if newBal.Sign() < 0 {
+ panic("balance underflow") // or return error
+ }
+ return s.SetBalance(newBal)
}
-func (s *stateObject) SubBalance(amount *big.Int) (prevWei uint256.Int) {
+func (s *stateObject) SubBalance(amount *big.Int) (prevWei uint256.Int) {
if amount.Sign() == 0 {
return *s.Balance()
}
- return s.SetBalance(new(big.Int).Sub(s.Balance().ToBig(), amount))
+ newBal := new(big.Int).Sub(s.Balance().ToBig(), amount)
+ if newBal.Sign() < 0 {
+ panic("balance underflow") // or return error
+ }
+ return s.SetBalance(newBal)
}Then remove the uint256 import from callers (or pass amount.ToBig()).
This harmonises the API and eliminates a class of runtime panics.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *stateObject) AddBalance(amount *uint256.Int) (prevWei uint256.Int) { | |
| if amount.Sign() == 0 { | |
| return | |
| if s.isEmpty() { | |
| s.touch() | |
| } | |
| return *(s.Balance()) | |
| } | |
| s.SetBalance(new(big.Int).Add(s.Balance(), amount)) | |
| return s.SetBalance(new(big.Int).Add(s.Balance().ToBig(), amount.ToBig())) | |
| } | |
| // SubBalance removes amount from s's balance. | |
| // It is used to remove funds from the origin account of a transfer. | |
| func (s *stateObject) SubBalance(amount *big.Int) { | |
| func (s *stateObject) SubBalance(amount *big.Int) (prevWei uint256.Int) { | |
| if amount.Sign() == 0 { | |
| return | |
| return *s.Balance() | |
| } | |
| s.SetBalance(new(big.Int).Sub(s.Balance(), amount)) | |
| return s.SetBalance(new(big.Int).Sub(s.Balance().ToBig(), amount)) | |
| } | |
| func (s *stateObject) AddBalance(amount *big.Int) (prevWei uint256.Int) { | |
| if amount.Sign() == 0 { | |
| if s.isEmpty() { | |
| s.touch() | |
| } | |
| return *(s.Balance()) | |
| } | |
| newBal := new(big.Int).Add(s.Balance().ToBig(), amount) | |
| if newBal.Sign() < 0 { | |
| panic("balance underflow") // or return an error | |
| } | |
| return s.SetBalance(newBal) | |
| } | |
| // SubBalance removes amount from s's balance. | |
| // It is used to remove funds from the origin account of a transfer. | |
| func (s *stateObject) SubBalance(amount *big.Int) (prevWei uint256.Int) { | |
| if amount.Sign() == 0 { | |
| return *s.Balance() | |
| } | |
| newBal := new(big.Int).Sub(s.Balance().ToBig(), amount) | |
| if newBal.Sign() < 0 { | |
| panic("balance underflow") // or return an error | |
| } | |
| return s.SetBalance(newBal) | |
| } |
| // and returns them as a JSON object. | ||
| func (b *Backend) TraceTransaction(hash gethcommon.Hash, config *evm.TraceConfig) (any, error) { | ||
| func (b *Backend) TraceTransaction( | ||
| hash gethcommon.Hash, | ||
| config *evm.TraceConfig, | ||
| ) (res json.RawMessage, err error) { | ||
| // Get transaction by hash |
There was a problem hiding this comment.
Since we json.Marshal the any results, the function signature can have stronger typing
| index := int(transaction.MsgIndex) // #nosec G701 | ||
| for i := 0; i < index; i++ { | ||
| for i := range tx.GetMsgs()[:int(transaction.MsgIndex)] { |
There was a problem hiding this comment.
Equivalent for loop recommendation in Modern Go. My LSP kept giving me a warning about this line before
There was a problem hiding this comment.
These cases no longer exist
There was a problem hiding this comment.
Copilot reviewed 68 out of 70 changed files in this pull request and generated no comments.
Files not reviewed (2)
- .github/workflows/e2e-wasm.yml: Language not supported
- contrib/scripts/e2e/deploy-wasm.sh: Language not supported
Comments suppressed due to low confidence (1)
app/evmante/evmante_can_transfer.go:71
- Verify that the conversion from method calls to direct field access (e.g. evmMsg.Value, evmMsg.From, evmMsg.GasFeeCap) aligns with the updated EVM message struct definitions to ensure consistent behavior across the application.
if evmMsg.Value.Sign() > 0 {
| evmResp, err = k.ApplyEvmMsg( | ||
| ctx, evmMsg, evmObj, evm.NewNoOpTracer(), commit, txConfig.TxHash, | ||
| ctx, evmMsg, evmObj, commit, txConfig.TxHash, | ||
| ) |
There was a problem hiding this comment.
We never use a no-op or nil tracer anymore. Passing nil automatically configures a default tracer
| // GetEthIntrinsicGas returns the intrinsic gas cost for the transaction | ||
| func (k *Keeper) GetEthIntrinsicGas( | ||
| ctx sdk.Context, | ||
| msg core.Message, | ||
| cfg *params.ChainConfig, | ||
| isContractCreation bool, |
There was a problem hiding this comment.
This abstraction became unnecessary, as geth expects all precompiles to be set on the global vm variables by convention, and the internal precompile methods read from those variables
|
|
||
| acct.BalanceNative = k.GetEvmGasBalance(ctx, addr) | ||
| acct.BalanceNative = uint256.MustFromBig(k.GetEvmGasBalance(ctx, addr)) | ||
| return acct |
There was a problem hiding this comment.
It's impossible to have a microNIBI or wei balance of 2^{256} - 1, so this is perfectly safe
| { | ||
| name: "add balance", | ||
| do: func(db *statedb.StateDB) { | ||
| db.AddBalanceSigned(address, big.NewInt(10)) | ||
| }, | ||
| expBalance: uint256.NewInt(10), | ||
| }, | ||
| { | ||
| name: "sub balance", | ||
| do: func(db *statedb.StateDB) { | ||
| db.AddBalanceSigned(address, big.NewInt(10)) | ||
| s.Require().Equal(uint256.NewInt(10), db.GetBalance(address)) | ||
| db.AddBalanceSigned(address, big.NewInt(-2)) | ||
| }, | ||
| expBalance: uint256.NewInt(8), | ||
| }, | ||
| { | ||
| name: "add zero balance", | ||
| do: func(db *statedb.StateDB) { | ||
| db.AddBalanceSigned(address, big.NewInt(0)) | ||
| }, | ||
| expBalance: uint256.NewInt(0), | ||
| }, | ||
| { | ||
| name: "sub zero balance", | ||
| do: func(db *statedb.StateDB) { | ||
| db.AddBalanceSigned(address, big.NewInt(0)) | ||
| }, | ||
| expBalance: uint256.NewInt(0), | ||
| }, | ||
| { | ||
| name: "overflow on addition", | ||
| do: func(db *statedb.StateDB) { | ||
| db.AddBalanceSigned(address, big.NewInt(69)) | ||
| tooBig := new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil) | ||
| maybeErr := xcommon.TryCatch(func() { | ||
| db.AddBalanceSigned(address, tooBig) | ||
| })() | ||
| s.ErrorContains(maybeErr, "uint256 overflow occurred for big.Int") | ||
| }, | ||
| expBalance: uint256.NewInt(69), | ||
| }, | ||
| { | ||
| name: "overflow on subtraction", | ||
| do: func(db *statedb.StateDB) { | ||
| db.AddBalanceSigned(address, big.NewInt(420)) | ||
| db.AddBalanceSigned(address, big.NewInt(-20)) // balance: 400 | ||
|
|
||
| // Construct -2^256 | ||
| tooBig := new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil) | ||
| tooBig.Neg(tooBig) | ||
|
|
||
| maybeErr := xcommon.TryCatch(func() { | ||
| db.AddBalanceSigned(address, tooBig) | ||
| })() | ||
|
|
||
| s.ErrorContains(maybeErr, "uint256 overflow occurred for big.Int") | ||
| }, | ||
| expBalance: uint256.NewInt(400), |
There was a problem hiding this comment.
Added some overflow test cases for completeness
| // TODO: UD-DEBUG: feat: Emit EVM events | ||
| // https://github.com/NibiruChain/nibiru/issues/2121 | ||
| // TODO: emit event for balance change of sender | ||
| // TODO: emit event for balance change of recipient | ||
|
|
There was a problem hiding this comment.
That ticket's already completed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
x/evm/statedb/journal.go (1)
423-440: Add contract creation tracking.The
createContractChangetype properly tracks when an account becomes a contract account, managing the created flag for proper handling of same-transaction destruction.Consider adding error logging in the
Revertmethod when the state object is nil, to aid in debugging:func (ch createContractChange) Revert(s *StateDB) { obj := s.getStateObject(ch.account) if obj == nil { + // Log that we couldn't find the state object for this account return } obj.newContract = false }x/evm/keeper/grpc_query.go (2)
731-736: Add helper function for gas tracking.The new
gasRemainingTxPartialfunction creates a minimal transaction structure for gas tracking purposes, which is a good abstraction.Consider adding a comment explaining why
LegacyTxis specifically chosen for this purpose:// gasRemainingTxPartial returns a [gethcore.Transaction] that only has its "Gas" // field set. +// Note: LegacyTx is used since we only need the gas limit for tracking purposes +// and don't need the additional fields from newer transaction types. func gasRemainingTxPartial(gasLimit uint64) *gethcore.Transaction { txData := gethcore.LegacyTx{Gas: gasLimit} return gethcore.NewTx(&txData) }
787-815: Update tracer initialization to use the new go-ethereum tracing framework.The code properly implements the new tracer initialization flow, with fallbacks to ensure a tracer is always available.
The fallback logic could be simplified to reduce duplication:
if traceConfig.Tracer == evm.TracerStruct { logger := logger.NewStructLogger(&logConfig) tracer = &tracers.Tracer{ Hooks: logger.Hooks(), GetResult: logger.GetResult, Stop: logger.Stop, } } else { if traceConfig.Tracer == "" || !gethTracerNames.Has(traceConfig.Tracer) { traceConfig.Tracer = "callTracer" usingCallTracer = true } tracer, err = tracers.DefaultDirectory.New( traceConfig.Tracer, tCtx, tracerJSONConfig, evmCfg.ChainConfig, ) if err != nil { return nil, 0, grpcstatus.Error(grpccodes.Internal, err.Error()) } } - if tracer == nil && !usingCallTracer { + if tracer == nil { traceConfig.Tracer = "callTracer" tracer, err = tracers.DefaultDirectory.New( traceConfig.Tracer, tCtx, tracerJSONConfig, evmCfg.ChainConfig, ) if err != nil { return nil, 0, grpcstatus.Error(grpccodes.Internal, err.Error()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(13 hunks)x/evm/chain_config.go(1 hunks)x/evm/keeper/grpc_query.go(13 hunks)x/evm/precompile/precompile.go(2 hunks)x/evm/statedb/journal.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/evm/chain_config.go
- x/evm/precompile/precompile.go
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: integration-tests
- GitHub Check: e2e-evm
🔇 Additional comments (13)
x/evm/statedb/journal.go (6)
75-81: Maintain consistency with go-ethereum implementation for edge case handling.The
dirtymethod is explicitly added to handle the RIPEMD precompile consensus exception, matching go-ethereum's implementation. This ensures compatibility with the reference implementation for this edge case.
158-158: Clarify terminology by renaming Suicided to SelfDestructed.Good update to use clearer terminology, consistent with the PR objective to rename "Suicide" to "HasSelfDestructed" for improved clarity and readability.
173-173: Migrate from big.Int to uint256.Int for better performance.The change from
*big.Intto*uint256.Intfor balance tracking aligns with Ethereum's optimizations for handling 256-bit integers, with proper conversion back tobig.Intwhen needed.Also applies to: 179-179
235-235: Add origin tracking to storage changes.The addition of the
originfield enhances storage tracking capabilities by recording the original value, which is important for proper state management during EVM execution.Also applies to: 241-241
375-393: Implement EIP-1153 transient storage support.The new
transientStorageChangetype properly implements support for EIP-1153 transient storage, which allows contracts to store data that only persists during transaction execution. The implementation correctly handles reverting to previous values.
395-421: Implement EIP-161 account touch tracking.The
touchChangetype correctly implements account "touching" semantics as required by EIP-161, which is essential for handling empty accounts consistently. The implementation includes appropriate comments explaining its purpose.x/evm/keeper/grpc_query.go (7)
290-290: Remove unnecessary tracer parameter.The tracer parameter is no longer needed in this call to
ApplyEvmMsgsince tracing is handled differently in the updated Go-Ethereum implementation.
383-398: Update to use direct Message struct initialization.This properly adapts to breaking changes in go-ethereum v1.14 by replacing factory method calls with direct struct initialization, making the code more resilient to future API changes.
505-506: Add block timestamp to MakeSigner calls.This update correctly passes block timestamps to
MakeSigner, which is necessary for proper transaction signature verification in newer versions of go-ethereum.
515-516: Update to use core.TransactionToMessage.Properly updates the code to use the new
TransactionToMessagefunction instead of the deprecatedAsMessagemethod, ensuring compatibility with go-ethereum v1.14.
738-749: Add structured tracer name handling.Good addition of a set of known tracer names for better organization and validation of tracer types.
846-847: Update EVM initialization to use the new tracer hooks API.Correctly updates the code to use
tracer.Hookswhich is the new way to access tracing hooks in go-ethereum v1.14.
852-852: Update to use the new tracer result retrieval method.Properly updates the code to use the new
GetResultmethod instead of direct casting, ensuring compatibility with the updated tracer interface.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eth/rpc/backend/tracing_test.go (1)
171-178: Updated AssertTraceCall function to handle JSON results.The function signature has been updated to accept a
json.RawMessageparameter, which aligns with the updated return types in the backend methods. The improved error handling with more descriptive error messages will help with debugging.Consider adding more specific assertions for fields that might vary based on the tracer type used, to ensure that the different tracers are producing the expected output formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/evmante/evmante_gas_consume.go(1 hunks)eth/rpc/backend/account_info_test.go(6 hunks)eth/rpc/backend/backend_suite_test.go(7 hunks)eth/rpc/backend/blocks.go(1 hunks)eth/rpc/backend/blocks_test.go(3 hunks)eth/rpc/backend/chain_info_test.go(4 hunks)eth/rpc/backend/tracing_test.go(6 hunks)eth/rpc/backend/tx_info_test.go(7 hunks)eth/rpc/backend/utils_test.go(1 hunks)x/evm/const.go(4 hunks)x/evm/evmtest/test_deps.go(2 hunks)x/evm/keeper/gas_fees.go(4 hunks)x/evm/keeper/gas_fees_test.go(1 hunks)x/evm/precompile/funtoken.go(7 hunks)x/evm/precompile/oracle.go(3 hunks)x/evm/precompile/wasm.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- eth/rpc/backend/utils_test.go
- eth/rpc/backend/account_info_test.go
- x/evm/evmtest/test_deps.go
- eth/rpc/backend/chain_info_test.go
- eth/rpc/backend/blocks.go
- eth/rpc/backend/blocks_test.go
- x/evm/precompile/oracle.go
- eth/rpc/backend/tx_info_test.go
- x/evm/precompile/funtoken.go
- x/evm/precompile/wasm.go
- x/evm/const.go
- x/evm/keeper/gas_fees.go
- eth/rpc/backend/backend_suite_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
x/evm/keeper/gas_fees_test.go (1)
x/evm/keeper/gas_fees.go (1)
VerifyFee(142-197)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit-tests
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
🔇 Additional comments (8)
x/evm/keeper/gas_fees_test.go (2)
105-105: Correctly set up test context with IsCheckTx.The change creates a proper SDK context with IsCheckTx flag set to true, which better reflects how the context is used in production code.
108-108: Adapts to the updated VerifyFee function signature.The test now passes the full context to VerifyFee instead of a boolean flag, properly aligning with the refactored function signature that now derives check transaction mode from the context.
app/evmante/evmante_gas_consume.go (1)
103-103: Correctly uses context-aware fee verification.The call to keeper.VerifyFee now passes the full SDK context rather than just a boolean flag, aligning with the refactored function signature. This change correctly adapts the ante handler to use context-based verification and allows access to Ethereum protocol rules from the context.
eth/rpc/backend/tracing_test.go (5)
17-33: Good addition of configuration helper functions.The new helper functions
traceConfigCallTracer()andtraceConfigDefaultTracer()improve code organization and maintainability by standardizing trace configuration across tests. This aligns well with the refactoring of tracer handling mentioned in the PR objectives.Consider adding test cases with
OnlyTopCallset to false to test nested call tracing behavior, which would expand test coverage of the new tracers.
72-74: Dynamic block reference improves test data management.Using
s.SuccessfulTxTransfer().BlockNumberRpcprovides a consistent way to reference test blocks with transactions, which is more maintainable than hardcoded block numbers.
81-115: Good expansion of test cases to cover multiple tracers.The expanded test cases now thoroughly test both the default and call tracers with different block conditions (with/without transactions). This significantly improves test coverage for the new tracer implementations from Geth v1.14.
127-138: Improved error handling and result validation.The addition of JSON pretty-printing for logging and the more robust type checking and error reporting for trace results will make debugging easier. The conversion to use
json.MarshalandAssertTraceCallwith the JSON result is consistent with the updated return types.
160-160: Explicit trace configuration in TestTraceCall.Using the helper function for trace configuration makes the test more explicit about its requirements and consistent with other test cases.
| { | ||
| name: "happy: tx found", | ||
| txHash: transferTxHash, | ||
| txHash: s.SuccessfulTxTransfer().Receipt.TxHash, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good use of the test suite's transaction data accessor.
Using s.SuccessfulTxTransfer() makes tests more maintainable by centralizing test data access logic. The improved error reporting on line 64 will help with debugging test failures.
Note that line 58 has a commented-out reference to traceConfigDefaultTracer(). Is this intentional or should both tracer configurations be tested?
Also applies to: 57-59, 64-66
🏁 Script executed:
#!/bin/bash
# Check if TestTraceTransaction has other test cases using the default tracer
rg -A 3 -B 3 "TestTraceTransaction.*traceConfigDefaultTracer" --type goLength of output: 73
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for 'traceConfigDefaultTracer' references:"
rg -n "traceConfigDefaultTracer" || echo "No matches found."
echo
echo "Inspecting TestTraceTransaction definitions and context:"
rg -n -C2 "TestTraceTransaction" --type go || echo "No TestTraceTransaction found."Length of output: 998
🏁 Script executed:
#!/bin/bash
# Display lines 1-200 of tracing_test.go to inspect TestTraceTransaction context
sed -n '1,200p' eth/rpc/backend/tracing_test.goLength of output: 4748
Approve tests but add default tracer case in TestTraceTransaction
Good use of the test suite’s transaction data accessor—calling s.SuccessfulTxTransfer() centralizes test data and improves maintainability, and the enhanced error reporting on failures will aid debugging.
However, in TestTraceTransaction you’ve only exercised the call tracer (via traceConfigCallTracer()), while the default tracer invocation is commented out. To ensure full coverage, please add a subtest for the default tracer:
• File eth/rpc/backend/tracing_test.go, within TestTraceTransaction:
{
name: "happy: tx found, default tracer",
txHash: s.SuccessfulTxTransfer().Receipt.TxHash,
wantErr: "",
tracer: traceConfigDefaultTracer(),
}
...
res, err := s.backend.TraceTransaction(
tc.txHash,
tc.tracer,
)This will align TestTraceTransaction with your TraceBlock and TraceCall tests, which already cover both tracers.
* Update LEGACY-CHANGELOG.md * Update HACKING.md
Signed-off-by: tsinghuacoder <[email protected]> Co-authored-by: Kevin Yang <[email protected]>
Co-authored-by: Unique Divine <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
CHANGELOG.md (1)
49-49: Align nested list indentation.
Nested list items are indented by 3 spaces causing markdownlint MD007 errors. Use 2 spaces per indent level, e.g.:- [#2275](...) - feat(evm)!: update ... - This upgrade keeps... - The jump to v1.14...Also applies to: 52-52
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
49-49: Unordered list indentation
Expected: 2; Actual: 3(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)proto/nibiru/oracle/v1/oracle.proto(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- proto/nibiru/oracle/v1/oracle.proto
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
49-49: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: e2e-evm
- GitHub Check: lint
- GitHub Check: integration-tests
🔇 Additional comments (1)
CHANGELOG.md (1)
46-46: Validate PR #2274 version label.
The entry for[#2274]references an update to geth v1.13, but the PR summary states it should upgrade to v1.14. Confirm the correct version and adjust the description accordingly.
CHANGELOG.md
Outdated
| - [#2275](https://github.com/NibiruChain/nibiru/pull/2275) - feat(evm)!: update | ||
| to geth v1.14 with tracing updates and new StateDB methods. |
There was a problem hiding this comment.
Fix bullet formatting for PR #2275 entry.
The PR entry for [#2275] is split across two lines without proper continuation indentation. Combine it into a single bullet or indent the second line as a nested continuation, for example:
- [#2275](.../2275) - feat(evm)!: update to geth v1.14 with tracing updates and new StateDB methods.
…ed by Nibiru's EVM JSON-RPC (#2289) * test(rpcapi): prevent more regressions with runtime service inspection * fix(backend): fix error propagation * fix(eth): error propagation fixes and tests for the methods exposed by Nibiru's EVM JSON-RPC Replace legacy error aliases throughout the eth module with sdkioerrors.Wrapf and sdkerrors.Err* to ensure consistent wrapping of Cosmos SDK errors. Simplify EVM backend error reporting by using fmt.Errorf with clear method prefixes (e.g. “BlockNumberError”, “RPCBlockFromTendermintBlock error”) and defaulting the base fee to evm.BASE_FEE_WEI. Suppress pruning‑related bloom errors by returning empty blooms when data is missing. Unify RPC transaction conversion logic by renaming NewRPCTxFromMsg to NewRPCTxFromMsgEthTx, retiring the older NewRPCTxFromEthTx, and centralizing signer and signature‑value extraction. Refactor BackendSuite tests to use a SuccessfulTx map for structured receipt capture—eliminating global variables and improving setup clarity. Stub out unimplemented debugapi methods with context‑aware signatures and adjust filters, tracing, and utils to inline BlockBloom usage and align imports (e.g. eip1559, tracers) with modern patterns. * chore: changelog PR number
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
eth/rpc/rpcapi/event_subscriber_test.go (1)
66-122:⚠️ Potential issueTest function doesn't match class definition
This test function
TestParseBloomFromEventsappears to be part of aSuitestruct that's no longer defined in this file. Since the file has moved away from using thetestify/suitepackage, this test is now broken.Either remove this test method or update it to use the new testing approach without the suite dependency.
🧹 Nitpick comments (8)
eth/rpc/rpcapi/filters.go (1)
118-118: Simplified error handling for BlockBloom retrievalThe code now directly uses the result of
f.backend.BlockBloom(blockRes)without error checking, aligning with broader backend changes that simplified block bloom filter retrieval across the codebase.Consider adding a comment explaining why error handling was removed here, to provide context for future maintainers.
eth/rpc/rpcapi/apis.go (1)
167-206: Minor: unreachable methods due to optional‑context detection
hasCtx := sig.NumIn() > 1 && sig.In(1) == ctxTypeassumes the receiver is always present (it is), but locks the context position to index 1.
If a service method embeds another receiver (e.g. via struct embedding) the actualcontext.Contextparameter index may differ.
Consider scanning every input after the receiver until you hit a non‑context argument to make this helper future‑proof.eth/rpc/rpc.go (1)
156-214: ParameterchainIDsilently overwritten for EIP‑1559 & blob txsInside the
switch,result.ChainIDis reassigned fromchainID(function arg) totx.ChainId():result.ChainID = (*hexutil.Big)(tx.ChainId())If callers purposely pass in a canonical
chainID(e.g. pulled from config rather than the tx) this overwrite might be surprising.
Confirm that this is intentional for all call‑sites; otherwise drop the reassignment.eth/rpc/backend/blocks.go (2)
59-60: Naming confusion:ErrNilBlockSuccessis an errorThe term “…Success” suggests a non‑error outcome, yet the value is used as a sentinel
error.
Consider renaming toErrNilBlockorErrBlockNotFoundButQueryOKto avoid mis‑interpretation.
337-355: Unchecked hex decoding may panic on malformed data
hexutils.HexToByteswill panic on invalid hex strings.
Although EVM events should emit valid data, defensive code would handle the error instead of relying on the runtime panic.- return gethcore.BytesToBloom(hexutils.HexToBytes(blockBloomEvent.Bloom)) + bytes, err := hexutils.DecodeHex(blockBloomEvent.Bloom) + if err != nil { + b.logger.Debug("invalid bloom hex in EventBlockBloom", "error", err) + return bloom // zero bloom + } + return gethcore.BytesToBloom(bytes)eth/rpc/rpcapi/debugapi/api.go (2)
352-361: Consider a sentinel error instead of building a new one for every call
ErrNotImplementedcurrently creates a freshfmt.Errorfeach time, which allocates and hampers equality checks. A single exported (or package‑private) sentinel plusfmt.Errorf("%w: %s", errNotImplemented, method)avoids that and still preserves the extra context.-var ErrNotImplemented(method string) error { - return fmt.Errorf("method is not implemented: %v", method) -} +var errNotImplemented = errors.New("method is not implemented") + +func wrapNotImplemented(method string) error { + return fmt.Errorf("%w: %s", errNotImplemented, method) +}Then replace the existing call‑sites with
wrapNotImplemented(...).
469-473: Minor style nit: remove named returns that are never used
TraceChaindeclaressubscription any, err errorbut immediately returns explicit values. Dropping the named returns simplifies the signature and avoids confusion.-func (a *DebugAPI) TraceChain( - ctx context.Context, - start, end rpc.BlockNumber, - config *tracers.TraceConfig, -) (subscription any, err error) { +func (a *DebugAPI) TraceChain( + ctx context.Context, + start, end rpc.BlockNumber, + config *tracers.TraceConfig, +) (any, error) {eth/rpc/rpcapi/eth_api.go (1)
153-158: Helper logs look good, but considerWarnlevel on errors
logErroralways logs atDebug. Production ops teams typically want backend failures surfaced atInfoorWarnso they are not filtered out. A tiny tweak:-logger.Debug(methodName+" failed", "error", err.Error()) +logger.Warn(methodName+" failed", "error", err.Error())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
CHANGELOG.md(1 hunks)eth/assert.go(3 hunks)eth/crypto/ethsecp256k1/ethsecp256k1.go(2 hunks)eth/eip712/eip712_legacy.go(4 hunks)eth/eip712/message.go(5 hunks)eth/eip712/types.go(7 hunks)eth/errors.go(1 hunks)eth/indexer/evm_tx_indexer.go(8 hunks)eth/rpc/backend/account_info.go(2 hunks)eth/rpc/backend/backend_suite_test.go(7 hunks)eth/rpc/backend/blocks.go(14 hunks)eth/rpc/backend/blocks_test.go(3 hunks)eth/rpc/backend/call_tx.go(2 hunks)eth/rpc/backend/chain_info_test.go(3 hunks)eth/rpc/backend/tx_info.go(7 hunks)eth/rpc/backend/utils.go(3 hunks)eth/rpc/rpc.go(4 hunks)eth/rpc/rpcapi/apis.go(2 hunks)eth/rpc/rpcapi/debugapi/api.go(5 hunks)eth/rpc/rpcapi/debugapi/trace.go(3 hunks)eth/rpc/rpcapi/eth_api.go(12 hunks)eth/rpc/rpcapi/eth_api_test.go(2 hunks)eth/rpc/rpcapi/event_subscriber_test.go(1 hunks)eth/rpc/rpcapi/filters.go(2 hunks)eth/safe_math.go(2 hunks)evm-e2e/test/debug_queries.test.ts(2 hunks)go.mod(14 hunks)x/common/error.go(0 hunks)
💤 Files with no reviewable changes (1)
- x/common/error.go
✅ Files skipped from review due to trivial changes (11)
- eth/rpc/backend/account_info.go
- eth/rpc/rpcapi/debugapi/trace.go
- eth/rpc/backend/call_tx.go
- eth/errors.go
- eth/eip712/types.go
- eth/assert.go
- eth/safe_math.go
- eth/eip712/message.go
- eth/crypto/ethsecp256k1/ethsecp256k1.go
- eth/eip712/eip712_legacy.go
- eth/indexer/evm_tx_indexer.go
🚧 Files skipped from review as they are similar to previous changes (6)
- evm-e2e/test/debug_queries.test.ts
- eth/rpc/backend/chain_info_test.go
- eth/rpc/backend/utils.go
- CHANGELOG.md
- eth/rpc/backend/blocks_test.go
- eth/rpc/backend/backend_suite_test.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
eth/rpc/rpcapi/event_subscriber_test.go (1)
eth/rpc/rpcapi/event_subscriber.go (1)
MakeSubscription(314-327)
eth/rpc/backend/tx_info.go (2)
x/evm/const.go (1)
BASE_FEE_WEI(23-23)eth/rpc/rpc.go (1)
NewRPCTxFromMsgEthTx(158-223)
eth/rpc/rpcapi/eth_api_test.go (2)
x/common/testutil/cases.go (1)
RetrySuiteRunIfDbClosed(43-74)eth/rpc/rpcapi/apis.go (4)
GetRPCAPIs(123-141)NamespaceEth(29-29)NamespaceDebug(32-32)ParseAPIMethods(156-209)
eth/rpc/rpcapi/debugapi/api.go (1)
eth/rpc/block.go (2)
BlockNumberOrHash(132-135)BlockNumber(25-25)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-tests
- GitHub Check: e2e-evm
- GitHub Check: integration-tests
- GitHub Check: lint
🔇 Additional comments (18)
eth/rpc/rpcapi/filters.go (1)
166-166: Simplified BlockBloom retrieval consistent with line 118This change follows the same pattern as the earlier change on line 118, removing error handling for bloom filter retrieval.
eth/rpc/rpcapi/event_subscriber_test.go (3)
50-50: Improved error reporting with event contextReplacing
t.Errorwitht.Errorfand including the event identifier enhances diagnostic clarity when topic channels are missing.
58-58: Enhanced error message with event identifierSimilar to line 50, this change improves error reporting by including the specific event that's missing.
62-62: Added event context to error messageConsistent with previous changes, this improves error diagnostics by including the event identifier in the error message.
eth/rpc/backend/tx_info.go (7)
10-10: Updated error package import aliasChanged the import alias from
errorsmodtosdkioerrorsfor consistency across the codebase.
74-74: Replaced dynamic base fee with constantUsing the constant
evm.BASE_FEE_WEIinstead of dynamically retrieving the base fee simplifies the code and aligns with the PR's objective to improve gas fee calculation logic.
77-84: Updated transaction construction methodSwitched from
rpc.NewRPCTxFromMsgtorpc.NewRPCTxFromMsgEthTx, which improves transaction handling by explicitly initializing signers and properly calculating gas prices.
106-113: Consistent use of improved tx constructorUpdated to use
rpc.NewRPCTxFromMsgEthTxfor pending transactions as well, maintaining consistency with the change on line 77.
281-282: Simplified dynamic fee tx gas price calculationUsing the constant
evm.BASE_FEE_WEIinstead of dynamic retrieval simplifies the code and ensures consistent gas pricing behavior.
432-442: Consistent base fee usage in transaction retrievalSame pattern of using the constant base fee is applied here for consistency across all transaction-related methods.
342-342: Updated error wrapping to use new aliasUpdated error wrapping calls to use the new import alias
sdkioerrorsconsistently with the import change on line 10.Also applies to: 365-365
eth/rpc/rpcapi/eth_api_test.go (2)
65-71: Simplified test entry point functionRenamed from
TestSuite_RunAlltoTestand added retry logic for database closure issues, improving test reliability.
73-183: Added comprehensive API method verification testsExcellent addition of tests that verify the presence of all expected RPC methods in the "eth" and "debug" namespaces. This ensures the API surface is complete and consistent after upgrading to Geth v1.14.
The test validates:
- Expected API service count
- All required Ethereum JSON-RPC methods
- All debug tracing methods needed for EVM compatibility
This is particularly important for ensuring compatibility after the significant updates in this PR.
eth/rpc/rpcapi/debugapi/api.go (1)
223-230: Re‑wrapping errors with%wis the right callSwitching from
github.com/pkg/errorstofmt.Errorf(...%w)gives you the standard‑library error‑wrapping semantics and keeps the dependency list leaner. No functional issues spotted here.Also applies to: 252-255
eth/rpc/rpcapi/eth_api.go (2)
195-201: Good API hardening – returning an error eliminates silent nilsChanging
GetBlockTransactionCountByHashto(*hexutil.Uint, error)prevents callers from mistaking anilcount for zero. Implementation correctly threads the backend error through.
493-500: Nice swap toNewRPCTxFromMsgEthTx, but keep an eye on chain‑IDThe helper now receives
e.backend.ChainConfig().ChainID, which is a pointer to a big.Int. Double‑check thatChainID()never returnsnilon mis‑configured chains; otherwise this call will panic.go.mod (2)
3-3:go 1.24is aspirational – verify CI image and downstream toolingGo 1.24 hasn’t been released yet. Using an unreleased version can break module consumers and most public CIs. Unless you have a forked toolchain, pin to the latest stable (1.22/1.23) and upgrade later.
-go 1.24 +go 1.22
269-269: Replace directive version matchesrequire, good!
github.com/NibiruChain/go-ethereum v1.14.13-nibiru.2lines up with the newrequireentry – ensuresgo mod tidystays clean.
| for i := range svcType.NumMethod() { | ||
| m := svcType.Method(i) | ||
| if m.PkgPath != "" { | ||
| continue // unexported | ||
| } | ||
| sig := m.Type | ||
|
|
There was a problem hiding this comment.
Invalid for loop over integer causes compilation error
range can’t be used directly on an int.
Current code:
for i := range svcType.NumMethod() {should be:
-for i := range svcType.NumMethod() {
+for i := 0; i < svcType.NumMethod(); i++ {Without this fix the code does not compile.
| var ( | ||
| ctxType = reflect.TypeOf((*context.Context)(nil)).Elem() | ||
| errType = reflect.TypeOf((*error)(nil)).Elem() | ||
| subscriptionType = reflect.TypeOf(Subscription{}) | ||
| ) |
There was a problem hiding this comment.
Compilation breaks: undefined identifier Subscription
Subscription is declared in the github.com/ethereum/go-ethereum/rpc package, not in rpcapi.
You must qualify the identifier, otherwise the file will not compile.
-subscriptionType = reflect.TypeOf(Subscription{})
+subscriptionType = reflect.TypeOf(rpc.Subscription{})If you want the non‑pointer type (to mirror how ctxType/errType were obtained), do:
subscriptionType = reflect.TypeOf((*rpc.Subscription)(nil)).Elem()
Purpose / Abstract
1 - Migration from geth v1.13 to v1.14
The geth v1.14 changes were merged into the current branch in #2275.
Related PRs in NibiruChain/go-ethereum:
2 - feat(evm)!: update to geth v1.13 with EIP-1153, PRECOMPILE_ADDRS, and transient storage support
[Expand to Display Details]
This commit upgrades the Nibiru EVM module for compatibility with go-ethereum v1.13.14, introducing the following changes:
rpcto their new equivalents undercommon/math, such as replacingrpc.DecimalOrHexwithmath.HexOrDecimal64.debug_seedHash) and associatedethashimport.vm.Configand fee calculation logic with new EIP-based gas cost parameters (isShanghai,isEIP3860).PRECOMPILE_ADDRSconstant to aggregate EVM precompiles with NibiruΓÇÖs extensions (FunToken, Wasm, Oracle).transientStoragemap toStateDB, with getters/setters and journaling support.Preparemethod to reset access lists and transient storage for each tx.SelfDestructlogic and exposedHasSelfDestructed(wasSuicide) for better clarity and future EIP-6780 readiness.AddBalanceSigned) to correctly handle signed values and preventuint256overflow errors.This upgrade also adjusts
go.modto:go-ethereum v1.10.xwithv1.13.14go-ethereumand its Pebble dependencyethdb3 - feat(evm): Adapt module to Geth v1.14 core package changes
[Expand to Display Details]
This commit updates the Nibiru EVM module to align with significant
breaking changes introduced in the upstream go-ethereum v1.13
core/vmand
core/typespackages. The goal is to leverage the updated Gethdependencies while ensuring compatibility with Nibiru's specific
requirements, particularly its custom precompiles that interact with the
Cosmos SDK.
This addresses several LSP errors and runtime issues arising from removed
or modified upstream functions and interfaces:
Replace
core.NewMessageCalls:core.NewMessagefactory function was removed upstream in favorof direct struct instantiation.
core.Message{...}structliterals, correctly mapping arguments to fields like
From,To,Nonce,Value,GasLimit, gas price fields (GasPrice,GasFeeCap,GasTipCap),Data,AccessList.*big.Intper thecore.Messagedefinition.BlobGasFeeCap,BlobHashes) and Nibiru-specific fields (SkipAccountChecks)are now correctly initialized.
Revert
vm.PrecompiledContractInterface for Nibiru Precompiles:PrecompiledContract.Runsignature toRun(input []byte).require the
*vm.EVMpointer to accessStateDBand derive thesdk.Contextneeded for Cosmos SDK keeper interactions.fork back to
Run(evm *vm.EVM, contract *vm.Contract, readonly bool).Address() common.Addressmethod is also restored to theinterface and implementations for use by the execution logic.
been adapted to match this reverted interface signature.
vm.RunPrecompiledContracthelper function is updated to passthe necessary
*vm.EVM,*vm.Contract, andreadonlycontext.Adopt
uint256for VM Value/Balance Interactions:core.Messageretains*big.Int, the internal VM logicand
StateDBmethods (e.g.,vm.Call,vm.Create,AddBalance,SubBalance,Contract.value) were updated upstream to use*uint256.Int(fromholiman/uint256).perform the necessary
*big.Int->*uint256.Intconversions.holiman/uint256as a direct dependency ingo.mod.Replace
StateDB.PrepareAccessList:PrepareAccessListmethod was removed from thevm.StateDBinterface upstream.
StateDB.Prepare(rules, sender, ...)method.Replace
evm.ActivePrecompilesMethod:ActivePrecompilesmethod on thevm.EVMstruct was removed.vm.ActivePrecompiles(rules), passing the appropriate chain rules.4 - fix(evm): Pass block timestamp to MakeSigner and refactor MsgEthereumTx field usage
[Expand to Display Details]
gethcore.MakeSignerto include the block time as a Unix timestamp (seconds), usingevm.ParseBlockTimeUnixU64(ctx).ParseBlockTimeUnixU64utility to extract block time fromsdk.Contextin a safe, reusable way.From,To,Value,GasFeeCap, etc.), improving efficiency and clarity.ParseWeiAsMultipleOfMicronibito returnuint256.Intand handle nil, zero, negative, and overflow edge cases with clear error messages.5 - refactor(evm): add more compatibility the new geth StateDB inteface, updating AddBalance and SubBalance to use uint256
6 - feat: impl slog.Handler for geth v1.14. used in json-rpc
[Expand to Display Details]
These changes update Nibiru's integration with the
go-ethereum/logpackage to align with its upstream migration to Go's standard structured logging library (slog). The previous logging setup in Nibiru, which relied ongo-ethereum/log's deprecatedFuncHandlerandRecordtypes, was removed.LogHandler: A new fileapp/server/geth_log_handler.gointroduces theLogHandlertype. This type implements the standardslog.Handlerinterface. Its primary role is to receive log records generated by Geth components (which now useslog) and translate them into corresponding calls on Nibiru's standardcmtlog.Logger(CometBFT logger). It correctly maps Geth/sloglevels (includingTraceandCrit) and formats log attributes for compatibility.app/server/json_rpc.go, the oldethlog.Root().SetHandler(...)block was replaced. The code now instantiates the newLogHandler(providing it the context loggerctx.Logger.With("module", "geth")), wraps it usinggethlog.NewLogger(), and sets the result as the default logger forgo-ethereumcomponents viagethlog.SetDefault().The primary reason for this refactor was the breaking change in the
go-ethereum/logdependency, which deprecated its custom logging implementation in favor of Go's standardslog. These changes adapt Nibiru to the newslog-based API, ensuring that logs generated within embedded Geth components are correctly captured and processed by Nibiru's existing logging infrastructure (cmtlog.Logger). This maintains consistent logging behavior and compatibility with the updated dependency.