Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: upgrade go-ethereum #3031

Merged
merged 2 commits into from
Nov 5, 2024
Merged

chore: upgrade go-ethereum #3031

merged 2 commits into from
Nov 5, 2024

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented Oct 22, 2024

Description

See zeta-chain/ethermint#141 for a detailed description of all go-ethereum changes.

Changes only effecting node:

  • tolerate eth.Address{}.Hash() removal (ref)
  • fix panic in hasher.Reset() (use trie.NewEmpty(nil)) instead

TODO:

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced logging capabilities with a new structured logging handler.
    • Updated transaction signing to utilize Ethermint's types for better integration.
  • Bug Fixes

    • Improved error handling in contract interactions and tests, ensuring accurate responses to various scenarios.
  • Refactor

    • Simplified message handling in multiple components by switching from value to pointer types for core.Message.
    • Transitioned from big.Int to uint256 for handling large integers across various modules, improving performance and consistency.
  • Documentation

    • Updated method signatures and parameters to reflect changes in data types and logging practices.

@gartnera gartnera added no-changelog Skip changelog CI check UPGRADE_TESTS Run make start-upgrade-tests labels Oct 22, 2024
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 64.70588% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.38%. Comparing base (44c377a) to head (707e814).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
precompiles/staking/method_stake.go 0.00% 7 Missing ⚠️
x/crosschain/keeper/evm_hooks.go 0.00% 4 Missing ⚠️
pkg/proofs/ethereum/proof.go 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3031      +/-   ##
===========================================
- Coverage    63.47%   63.38%   -0.09%     
===========================================
  Files          422      422              
  Lines        29821    29915      +94     
===========================================
+ Hits         18928    18963      +35     
- Misses       10059    10111      +52     
- Partials       834      841       +7     
Files with missing lines Coverage Δ
precompiles/precompiles.go 100.00% <100.00%> (ø)
x/fungible/keeper/evm.go 85.61% <100.00%> (ø)
x/fungible/keeper/evm_hooks.go 100.00% <100.00%> (ø)
pkg/proofs/ethereum/proof.go 91.58% <80.00%> (-0.08%) ⬇️
x/crosschain/keeper/evm_hooks.go 82.93% <0.00%> (ø)
precompiles/staking/method_stake.go 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

@gartnera gartnera force-pushed the upgrade-go-ethereum branch 2 times, most recently from 1b84659 to ab8dda8 Compare October 22, 2024 22:36
@gartnera gartnera marked this pull request as ready for review October 23, 2024 01:12
Copy link
Contributor

coderabbitai bot commented Oct 23, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request encompasses several changes across multiple files, primarily focusing on updating method signatures to use pointer types for core.Message, enhancing logging mechanisms, and modifying dependency versions in go.mod. Key alterations include the introduction of the uint256 package for handling large integers, updates to the FeeHistory method signatures to improve type consistency, and the removal of the SeedHash method from the API struct. These changes collectively refine the codebase's structure and maintainability while ensuring compatibility with the updated functionalities.

Changes

File Path Change Summary
app/ante/interfaces.go Updated NewEVM method signature from msg core.Message to msg *core.Message.
contrib/rpcimportable/go.mod Added replacement for github.com/ethereum/go-ethereum to a specific fork and clarified the go.mod file requirements.
e2e/runner/setup_evm.go Modified logging statements in SetupEVM to reflect transaction hashes instead of contract address hashes.
go.mod Updated multiple dependency versions and added new dependencies, including github.com/zeta-chain/go-ethereum.
pkg/proofs/ethereum/proof.go Changed GenerateProof method signature by removing a placeholder argument and updated NewTrie initialization method.
precompiles/bank/bank_test.go Replaced big.NewInt(0) with uint256.NewInt(0) in tests, reflecting a transition to a custom integer type.
precompiles/bank/method_test.go Updated mockVMContract initialization to use uint256.NewInt(0) instead of big.NewInt(0).
precompiles/precompiles.go Changed return types of several contract functions from vm.PrecompiledContract to vm.StatefulPrecompiledContract.
precompiles/staking/staking.go Updated Stake method to handle uint256 for amounts, ensuring overflow checks.
rpc/backend/backend.go Updated FeeHistory method parameter type from rpc.DecimalOrHex to math.HexOrDecimal64.
rpc/backend/chain_info.go Changed userBlockCount parameter type in FeeHistory method from rpc.DecimalOrHex to ethmath.HexOrDecimal64.
rpc/backend/chain_info_test.go Updated userBlockCount variable in tests to use ethmath.HexOrDecimal64.
rpc/backend/sign_tx.go Replaced ethtypes.MakeSigner with ethermint.MakeSigner in SendTransaction method.
rpc/backend/utils.go Changed import for base fee calculation from misc to eip1559.
rpc/namespaces/ethereum/debug/api.go Removed SeedHash method from API struct.
rpc/namespaces/ethereum/eth/api.go Updated FeeHistory method signature to change blockCount parameter type from rpc.DecimalOrHex to ethmath.HexOrDecimal64.
server/json_rpc.go Introduced new logging handler gethLogsToTm for structured logging.
testutil/keeper/mocks/fungible/evm.go Updated ApplyMessage method to accept *core.Message instead of core.Message.
testutil/sample/crypto.go Modified Hash function to compute hash using ethcommon.BytesToHash(EthAddress().Bytes()).
x/crosschain/keeper/evm_hooks.go Updated PostTxProcessing method to accept *core.Message instead of core.Message.
x/crosschain/keeper/msg_server_vote_inbound_tx_test.go Changed balance initialization in tests to use uint256.NewInt(0).
x/fungible/keeper/evm.go Updated message handling to use core.Message and refined transaction attribute handling.
x/fungible/keeper/evm_hooks.go Updated PostTxProcessing method to accept *core.Message.
x/fungible/keeper/zevm_message_passing_test.go Updated balance initialization in tests to use uint256.NewInt(0).
x/fungible/types/expected_keepers.go Updated ApplyMessage method signature to accept *core.Message.

Suggested labels

breaking:cli, SOLANA_TESTS

Suggested reviewers

  • fbac
  • kingpinXD
  • swift1337
  • ws4charlie
  • lumtis
  • skosito
  • brewmaster012

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (12)
contrib/rpcimportable/go.mod (1)

Line range hint 4-6: Clarify the comment about empty go.mod

The comment states that "this go.mod should be empty when committed", but the file necessarily contains replacements for proper functionality. Consider updating the comment to better reflect the actual requirements, perhaps indicating that it should contain only essential replacements.

-// this go.mod should be empty when committed
-// the go.sum should not be committed
+// this go.mod should contain only essential replacements when committed
+// the go.sum should not be committed
x/fungible/keeper/evm_hooks.go (2)

25-27: Consider enhancing documentation and adding invariant checks.

While the implementation is correct, consider these improvements:

  1. Document why the message parameter is unused (marked with _)
  2. Add an invariant check for nil message, even though it's unused, to maintain robustness

Here's a suggested improvement:

+// PostTxProcessing implements the EvmHooks interface. The message parameter is unused
+// as the hook only needs to examine the transaction receipt for ZRC20 contract events.
-func (h EVMHooks) PostTxProcessing(ctx sdk.Context, _ *core.Message, receipt *ethtypes.Receipt) error {
+func (h EVMHooks) PostTxProcessing(ctx sdk.Context, msg *core.Message, receipt *ethtypes.Receipt) error {
+    if msg == nil {
+        return cosmoserrors.Wrap(types.ErrInvalidMessage, "nil message")
+    }
     return h.k.checkPausedZRC20(ctx, receipt)
 }

Line range hint 30-57: Consider optimizing address collection.

The current implementation is efficient, but we can make it more memory-efficient by pre-allocating the slice based on the number of logs and using a more idiomatic Go approach.

Here's a suggested optimization:

 func (k Keeper) checkPausedZRC20(ctx sdk.Context, receipt *ethtypes.Receipt) error {
     if receipt == nil {
         return nil
     }
 
-    // get non-duplicated list of all addresses that emitted logs
-    var addresses []ethcommon.Address
-    addressExist := make(map[ethcommon.Address]struct{})
+    // Pre-allocate maps for better performance with known size
+    addressExist := make(map[ethcommon.Address]struct{}, len(receipt.Logs))
+    
     for _, log := range receipt.Logs {
         if log == nil {
             continue
         }
-        if _, ok := addressExist[log.Address]; !ok {
-            addressExist[log.Address] = struct{}{}
-            addresses = append(addresses, log.Address)
-        }
+        addressExist[log.Address] = struct{}{}
     }
 
     // check if any of the addresses are from a paused ZRC20 contract
-    for _, address := range addresses {
+    for address := range addressExist {
         fc, found := k.GetForeignCoins(ctx, address.Hex())
         if found && fc.Paused {
             return cosmoserrors.Wrap(types.ErrPausedZRC20, address.Hex())
         }
     }
 
     return nil
 }

This optimization:

  1. Eliminates the need for a separate slice
  2. Pre-allocates the map with known capacity
  3. Reduces memory allocations
  4. Uses a more idiomatic Go approach for map iteration
precompiles/precompiles.go (1)

Line range hint 65-65: Fix incorrect comment reference

The comment incorrectly refers to "staking contract" instead of "bank contract".

-		// Append the staking contract to the precompiledContracts slice.
+		// Append the bank contract to the precompiledContracts slice.
server/json_rpc.go (1)

38-41: Add documentation for the new struct.

Consider adding documentation comments to describe the purpose and behavior of the gethLogsToTm struct and its fields.

+// gethLogsToTm implements slog.Handler to convert go-ethereum logs to Tendermint logger format
 type gethLogsToTm struct {
+    // logger is the underlying Tendermint logger
     logger tmlog.Logger
+    // attrs contains the accumulated logging attributes
     attrs  []slog.Attr
 }
e2e/runner/setup_evm.go (2)

79-79: LGTM! Consider standardizing the logging format.

The change to use transaction hash instead of contract address hash improves traceability and aligns with the logging pattern used for other contracts in this file.

For consistency with other logging statements in this file, consider using .Hex() for the transaction hash:

-r.Logger.Info("ZetaEth contract address: %s, tx hash: %s", zetaEthAddr.Hex(), txZetaEth.Hash())
+r.Logger.Info("ZetaEth contract address: %s, tx hash: %s", zetaEthAddr.Hex(), txZetaEth.Hash().Hex())

Line range hint 156-157: Consider addressing TODOs and hardcoded values.

The file contains TODOs about configuration management and uses hardcoded chain IDs. Consider:

  1. Creating a follow-up ticket to refactor the configuration logic.
  2. Moving the chain ID to a configuration parameter for better flexibility in testing different networks.

Would you like me to help create GitHub issues to track these improvements?

Also applies to: 186-187

rpc/backend/utils.go (1)

Line range hint 158-166: Consider using big.Float for gas calculations to prevent precision loss.

The current implementation uses float64 for gas calculations which might lead to precision loss in edge cases. Consider using big.Float throughout the calculations for consistent precision.

Apply this diff to improve precision:

-	gasusedfloat, _ := new(big.Float).SetInt(gasUsedBig.ToInt()).Float64()
+	gasUsedFloat := new(big.Float).SetInt(gasUsedBig.ToInt())
+	gasLimitFloat := new(big.Float).SetUint64(uint64(gasLimitUint64))
 
 	if gasLimitUint64 <= 0 {
 		return fmt.Errorf(
 			"gasLimit of block height %d should be bigger than 0 , current gaslimit %d",
 			blockHeight,
 			gasLimitUint64,
 		)
 	}
 
-	gasUsedRatio := gasusedfloat / float64(gasLimitUint64)
-	blockGasUsed := gasusedfloat
+	gasUsedRatio, _ := new(big.Float).Quo(gasUsedFloat, gasLimitFloat).Float64()
+	blockGasUsed, _ := gasUsedFloat.Float64()
x/crosschain/keeper/msg_server_vote_inbound_tx_test.go (2)

Line range hint 42-42: Address the TODO comment for test completeness

The comment references incomplete test cases tracked in issue #1542. While the current test coverage is comprehensive, completing the remaining test cases would ensure full coverage of edge cases.

Would you like assistance in identifying and implementing the remaining test cases from issue #1542?


Line range hint 249-286: Enhance test case descriptions in table-driven tests

Consider making the test cases more descriptive by:

  1. Including the initial and expected final states in the test names
  2. Adding comments explaining the test scenarios
  3. Using consistent error message assertions across all test cases

Example improvement:

 tt := []struct {
     Name         string
     Status       types.Status
     NonErrStatus types.CctxStatus
     Msg          string
     IsErr        bool
     ErrStatus    types.CctxStatus
 }{
     {
-        Name: "Transition on finalize Inbound",
+        Name: "Should transition from PendingInbound to PendingOutbound on successful finalization",
         Status: types.Status{
             Status:              types.CctxStatus_PendingInbound,
             StatusMessage:       "Getting InTX Votes",
             LastUpdateTimestamp: 0,
         },
         // ... rest of the test case
     },
     {
-        Name: "Transition on finalize Inbound Fail",
+        Name: "Should transition from PendingInbound to OutboundMined on finalization failure",
         Status: types.Status{
             Status:              types.CctxStatus_PendingInbound,
             StatusMessage:       "Getting InTX Votes",
             LastUpdateTimestamp: 0,
         },
         // ... rest of the test case
     },
 }
go.mod (1)

Line range hint 1-3: Update Go version specification.

The toolchain directive specifies Go 1.22.5, but the go directive uses 1.22.2. Consider updating the go directive to match the toolchain version for consistency.

 module github.com/zeta-chain/node
 
-go 1.22.2
+go 1.22.5
precompiles/bank/method_test.go (1)

Line range hint 1-700: Consider adding gas consumption tests

While the test coverage is comprehensive for functionality, consider adding specific tests for gas consumption patterns to ensure the upgrade hasn't introduced any significant changes in gas costs.

Example test case:

t.Run("should maintain expected gas consumption for deposit", func(t *testing.T) {
    ts := setupChain(t)
    caller := fungibletypes.ModuleAddressEVM
    
    // Setup initial state
    ts.fungibleKeeper.DepositZRC20(ts.ctx, ts.zrc20Address, caller, big.NewInt(1000))
    allowBank(t, ts, big.NewInt(500))
    
    // Record initial gas
    initialGas := ts.ctx.GasMeter().GasConsumed()
    
    // Execute deposit
    methodID := ts.bankABI.Methods[DepositMethodName]
    ts.mockVMContract.CallerAddress = caller
    ts.mockEVM.Origin = caller
    ts.mockVMContract.Input = packInputArgs(
        t,
        methodID,
        []interface{}{ts.zrc20Address, big.NewInt(500)}...,
    )
    
    _, err := ts.bankContract.Run(ts.mockEVM, ts.mockVMContract, false)
    require.NoError(t, err)
    
    // Verify gas consumption
    gasUsed := ts.ctx.GasMeter().GasConsumed() - initialGas
    require.LessOrEqual(t, gasUsed, uint64(50000), "deposit operation consumed too much gas")
})
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 0f8a4c2 and ab8dda8.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (26)
  • app/ante/interfaces.go (1 hunks)
  • contrib/rpcimportable/go.mod (1 hunks)
  • e2e/runner/setup_evm.go (1 hunks)
  • go.mod (10 hunks)
  • pkg/proofs/ethereum/proof.go (3 hunks)
  • precompiles/bank/bank_test.go (2 hunks)
  • precompiles/bank/method_test.go (2 hunks)
  • precompiles/precompiles.go (3 hunks)
  • precompiles/staking/staking.go (2 hunks)
  • precompiles/staking/staking_test.go (2 hunks)
  • rpc/backend/backend.go (2 hunks)
  • rpc/backend/chain_info.go (2 hunks)
  • rpc/backend/chain_info_test.go (2 hunks)
  • rpc/backend/sign_tx.go (2 hunks)
  • rpc/backend/utils.go (2 hunks)
  • rpc/namespaces/ethereum/debug/api.go (0 hunks)
  • rpc/namespaces/ethereum/eth/api.go (3 hunks)
  • server/json_rpc.go (3 hunks)
  • testutil/keeper/mocks/fungible/evm.go (2 hunks)
  • testutil/sample/crypto.go (1 hunks)
  • x/crosschain/keeper/evm_hooks.go (1 hunks)
  • x/crosschain/keeper/msg_server_vote_inbound_tx_test.go (2 hunks)
  • x/fungible/keeper/evm.go (3 hunks)
  • x/fungible/keeper/evm_hooks.go (1 hunks)
  • x/fungible/keeper/zevm_message_passing_test.go (5 hunks)
  • x/fungible/types/expected_keepers.go (1 hunks)
💤 Files with no reviewable changes (1)
  • rpc/namespaces/ethereum/debug/api.go
🧰 Additional context used
📓 Path-based instructions (23)
app/ante/interfaces.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/runner/setup_evm.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

pkg/proofs/ethereum/proof.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

precompiles/bank/bank_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

precompiles/bank/method_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

precompiles/precompiles.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

precompiles/staking/staking.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

precompiles/staking/staking_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/backend/backend.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/backend/chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/backend/chain_info_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/backend/sign_tx.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/backend/utils.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

rpc/namespaces/ethereum/eth/api.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

server/json_rpc.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

testutil/keeper/mocks/fungible/evm.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

testutil/sample/crypto.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/evm_hooks.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/msg_server_vote_inbound_tx_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/evm.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/evm_hooks.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/zevm_message_passing_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/types/expected_keepers.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

🪛 GitHub Check: codecov/patch
pkg/proofs/ethereum/proof.go

[warning] 173-173: pkg/proofs/ethereum/proof.go#L173
Added line #L173 was not covered by tests

precompiles/staking/staking.go

[warning] 238-239: precompiles/staking/staking.go#L238-L239
Added lines #L238 - L239 were not covered by tests

x/crosschain/keeper/evm_hooks.go

[warning] 43-43: x/crosschain/keeper/evm_hooks.go#L43
Added line #L43 was not covered by tests


[warning] 54-55: x/crosschain/keeper/evm_hooks.go#L54-L55
Added lines #L54 - L55 were not covered by tests


[warning] 57-57: x/crosschain/keeper/evm_hooks.go#L57
Added line #L57 was not covered by tests

🔇 Additional comments (44)
contrib/rpcimportable/go.mod (1)

14-17: LGTM: Well-structured go-ethereum replacement

The replacement directive is correctly formatted and points to a specific commit hash, which is a good practice for dependency stability. The comment clearly explains the reason for the replacement (incompatible pebbledb version).

Let's verify the go-ethereum version compatibility:

✅ Verification successful

This script will:

  1. Find all go.mod files in the repository
  2. For each go.mod file, show any lines containing go-ethereum references and the line after it
  3. This will help us verify if the go-ethereum replacement is consistent across the codebase

Go-ethereum replacement is consistently applied across the codebase

The verification confirms that both go.mod files in the repository (root and contrib/rpcimportable) use the exact same replacement directive for go-ethereum:

github.com/ethereum/go-ethereum => github.com/zeta-chain/go-ethereum v1.13.16-0.20241022183758-422c6ef93ccc

This consistent usage ensures that the codebase maintains dependency compatibility, particularly regarding the pebbledb version mentioned in the comment.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify go-ethereum version usage across the codebase
# Expected: All go.mod files should use the same version if they depend on go-ethereum

# Search for any go.mod files containing go-ethereum references
rg -l "github.com/ethereum/go-ethereum" --type go-mod

# Show the specific versions/replacements used
rg "github.com/ethereum/go-ethereum =>" --type go-mod -A 1

Length of output: 176


Script:

#!/bin/bash
# Let's try a different approach without relying on file type filtering
# First, find all go.mod files
fd "go.mod$"

# Then search for go-ethereum references and replacements in these files
echo "=== Searching for go-ethereum dependencies and replacements ==="
for f in $(fd "go.mod$"); do
    echo "=== File: $f ==="
    # Show the content focusing on go-ethereum
    grep -A 1 "github.com/ethereum/go-ethereum" "$f" || true
done

Length of output: 926

x/fungible/keeper/evm_hooks.go (1)

25-27: LGTM: Parameter type change aligns with upstream modifications.

The change from core.Message to *core.Message is consistent with the go-ethereum upgrade and maintains uniformity across the codebase.

x/fungible/types/expected_keepers.go (1)

52-54: LGTM! Verify interface implementations.

The change from core.Message to *core.Message aligns with the go-ethereum upgrade objectives.

Let's verify all implementations of this interface have been updated:

✅ Verification successful

Let me try a different approach to verify the implementations.

This should help us find:

  1. All implementations of ApplyMessage method
  2. Potential keeper structs that might implement the EVMKeeper interface

Let's analyze the results to verify if all implementations have been updated to use pointer semantics for the msg parameter.


Let me try one more search to find the actual implementation of the EVMKeeper interface, as we've only found the mock implementation so far.

This should help us locate the actual implementation of the EVMKeeper interface to verify if it has been properly updated to match the interface changes.


Let's check the actual implementation in x/fungible/keeper/evm.go to verify if it matches the updated interface.

This will show us the actual implementation to verify if it's using the correct pointer semantics for the msg parameter.


Let me try one final search with a different approach to locate the implementation.

This should help us confirm the implementation details or verify if the file structure is different than expected.


Based on the search results and code analysis, I can now provide a final response.

Interface change correctly implemented

The change from core.Message to *core.Message in the EVMKeeper interface is correctly reflected in the actual implementation. The mock implementation in testutil/keeper/mocks/fungible/evm.go already uses the pointer semantics, and the actual usage in x/fungible/keeper/evm.go shows that the msg parameter is consistently used as a pointer in the CallEVMWithData function.

Key evidence:

  1. Mock implementation: func (_m *FungibleEVMKeeper) ApplyMessage(ctx types.Context, msg *core.Message, ...)
  2. Actual usage: msg := &core.Message{...} in CallEVMWithData
  3. No other direct implementations of ApplyMessage found, indicating these are the only relevant occurrences
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for EVMKeeper implementations and verify ApplyMessage signature
ast-grep --pattern 'type $_ struct {
  $$$
}

func ($_ $_) ApplyMessage(ctx sdk.Context, msg core.Message, $$$) {
  $$$
}'

Length of output: 124


Script:

#!/bin/bash
# Search for ApplyMessage implementations using ripgrep for better context
rg "func.*ApplyMessage.*\(" -A 5

# Also search for any struct that might implement EVMKeeper
rg "type.*Keeper struct" -A 5

Length of output: 16065


Script:

#!/bin/bash
# Search for files that might contain the actual EVMKeeper implementation
rg -l "EVMKeeper" --type go | grep -v "mock" | grep -v "test"

# Also search for any file containing ApplyMessage implementation
rg -l "ApplyMessage" --type go | grep -v "mock" | grep -v "test"

Length of output: 313


Script:

#!/bin/bash
# Check the implementation of ApplyMessage in the actual keeper
rg "func.*ApplyMessage.*\(" x/fungible/keeper/evm.go -A 5

Length of output: 59


Script:

#!/bin/bash
# Use ast-grep to find the implementation with a more specific pattern
ast-grep --pattern 'func ($_ $_) ApplyMessage(ctx $_, msg *core.Message, $$$) {
  $$$
}'

# Also get the content of the evm.go file to verify its existence and content
cat x/fungible/keeper/evm.go

Length of output: 54088

app/ante/interfaces.go (1)

44-44: LGTM! Note the breaking interface change.

The change from core.Message to *core.Message is appropriate as it aligns with Go best practices for large structs and the broader go-ethereum upgrade. This is a breaking change that requires updates to all implementations.

Let's verify that all implementations have been updated:

✅ Verification successful

The previous search might have failed because:

  1. The pattern was too strict
  2. We need to find both the interface implementations and the actual struct
  3. We should look for both pointer and non-pointer Message usage to ensure completeness

This new approach should help us:

  • Find files containing NewEVM implementations
  • Locate the actual implementation with the correct signature
  • Identify structs that implement the interface to verify their conformance

The previous search revealed that the interface file exists, but we need to:

  1. Find the actual implementation files
  2. Verify the parameter types in implementations
  3. Check for any remaining non-pointer Message usages
  4. Look for the concrete types implementing EVMKeeper

This new approach uses simpler regex patterns and includes context lines to better understand the implementations.


Interface change verification complete - all usages are pointer-compliant

The verification results show that all core.Message usages across the codebase are consistently using pointer types (*core.Message). Specifically:

  • Interface definition in app/ante/interfaces.go
  • Implementation in mock files for testing
  • Usage in hooks and keepers
  • No instances of non-pointer core.Message were found

The change is properly implemented and maintains consistency across the codebase, including test files and mock implementations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all implementations of NewEVM to ensure they've been updated
# to use *core.Message

# Search for NewEVM method implementations
ast-grep --pattern 'func $_($_ sdk.Context, msg core.Message, $$$) *vm.EVM'

# Also check for any remaining non-pointer Message usage
rg -g '*.go' 'msg(\s+)core\.Message[^*]'

Length of output: 120


Script:

#!/bin/bash
# Let's try a different approach to find NewEVM implementations
# First, find all files that might contain the implementation
fd -e go | xargs rg -l "NewEVM.*core\.Message"

# Then use ast-grep with a more specific pattern for the interface implementation
ast-grep --pattern 'func ($_ *$_) NewEVM(ctx sdk.Context, msg *core.Message, $$$) *vm.EVM'

# Also search for any struct that embeds EVMKeeper interface
rg -g '*.go' "type.*struct.*{.*EVMKeeper.*}"

Length of output: 338


Script:

#!/bin/bash
# Let's try one more approach to find all relevant implementations and usages

# Find files containing NewEVM implementation
rg -l "func.*NewEVM.*Message" -g "*.go"

# Look for EVMKeeper interface implementations
rg -l "type.*EVMKeeper" -g "*.go"

# Search for actual NewEVM implementations with context
rg "func.*NewEVM.*Message" -g "*.go" -A 2

# Look for any Message parameter usage in the codebase
rg "msg\s+(?:\*)?core\.Message" -g "*.go"

Length of output: 1044

precompiles/precompiles.go (4)

42-45: LGTM: Appropriate use of StatefulPrecompiledContract

The change from PrecompiledContract to StatefulPrecompiledContract correctly reflects the stateful nature of the prototype contract.


52-55: LGTM: Consistent type usage for staking contract

The staking contract's transition to StatefulPrecompiledContract aligns with its state management requirements and maintains consistency with other contracts.


61-64: LGTM: Appropriate StatefulPrecompiledContract implementation

The bank contract's implementation correctly uses the new type while maintaining its context-aware behavior.


Line range hint 42-64: Verify StatefulPrecompiledContract interface implementation

Let's ensure all contract implementations properly implement the required interface methods.

✅ Verification successful

The shell script results show that all three precompile contracts (Staking, Prototype, and Bank) properly implement the Run method from the vm.StatefulPrecompiledContract interface. Each implementation follows a similar pattern:

  1. Extracts and validates the method from input
  2. Unpacks arguments
  3. Handles state operations through ExtStateDB
  4. Implements proper error handling
  5. Returns appropriate responses

Let me verify if there are any other precompile contracts that should implement this interface:


All precompile contracts correctly implement the StatefulPrecompiledContract interface

The verification confirms that all three precompile contracts (Staking, Prototype, and Bank) properly implement the Run method from the vm.StatefulPrecompiledContract interface. Each contract:

  • Embeds ptypes.BaseContract for common functionality
  • Implements the Run method with consistent error handling and state management
  • Follows the same pattern for method dispatch and argument handling
  • Properly handles read-only mode where applicable

The implementation is consistent across all precompile contracts in the codebase, with no missing implementations found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all contract implementations properly implement the StatefulPrecompiledContract interface

# Search for contract struct definitions and their methods
ast-grep --pattern 'type $_ struct {
  $$$
}'

# Search for Run method implementations which should be present for StatefulPrecompiledContract
ast-grep --pattern 'func ($_ $_) Run($$$) ([]byte, error) {
  $$$
}'

Length of output: 479197


Script:

#!/bin/bash
# Find all precompile contract definitions
ast-grep --pattern 'type Contract struct {
  $$$
}'

Length of output: 1308

testutil/sample/crypto.go (1)

94-94: Implementation aligns with go-ethereum upgrade requirements.

The change from EthAddress().Hash() to ethcommon.BytesToHash(EthAddress().Bytes()) correctly addresses the removal of eth.Address{}.Hash() mentioned in the PR objectives. The new implementation achieves the same outcome by first converting the address to bytes and then creating a hash, which is the recommended approach.

Let's verify there are no remaining uses of the deprecated .Hash() method:

pkg/proofs/ethereum/proof.go (2)

149-149: LGTM: Improved trie initialization using recommended approach.

The change to trie.NewEmpty(nil) aligns with the go-ethereum upgrade requirements and fixes the potential panic issue mentioned in the PR objectives.


139-139: LGTM: API signature update aligned with go-ethereum upgrade.

The simplified Prove method call correctly adapts to the updated go-ethereum API while maintaining the same functionality.

Let's verify there are no remaining old-style calls:

✅ Verification successful

The previous search might have been too specific. Let's broaden our search to verify the API changes and ensure all related code is properly updated. We'll look for all Prove-related calls and check the go-ethereum version to confirm the upgrade.


I need to verify the specific implementation context of the GenerateProof function to confirm the API change is correctly implemented.


I need to see the complete implementation and verify the go-ethereum trie package usage to confirm the API change is correctly aligned with the dependency upgrade.


API update correctly implemented and verified

The change from t.Prove(indexBuf, 0, proof) to t.Prove(indexBuf, proof) is correctly aligned with the go-ethereum trie package upgrade. The implementation follows the proper pattern where:

  1. The code is properly attributed with go-ethereum license
  2. The trie package is directly imported from go-ethereum
  3. The Prove method is used consistently throughout the codebase
  4. The proof verification logic remains intact
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of the old three-argument Prove call
rg 'Prove\([^,]+,\s*0,\s*[^,]+\)' 

Length of output: 54


Script:

#!/bin/bash
# Let's search for all Prove method calls to verify the API usage
rg 'Prove\(' -A 2 -B 2

# Also search for any test files that might contain Prove calls
fd -e go -x rg 'func.*Test.*Prove' -A 5 {}

# Check imports to verify go-ethereum version
rg '"github.com/ethereum/go-ethereum' 

Length of output: 72458


Script:

#!/bin/bash
# Let's check the specific file and its context around the Prove call
rg -A 5 -B 5 'func.*GenerateProof' pkg/proofs/ethereum/proof.go

# Also check for any test files that verify this functionality
fd -e go -x rg 'func.*Test.*GenerateProof' {}

Length of output: 463


Script:

#!/bin/bash
# Let's examine the entire proof.go file to understand the full context
cat pkg/proofs/ethereum/proof.go

# Also check for any imports of go-ethereum/trie to verify version compatibility
rg '"github.com/ethereum/go-ethereum/trie"'

Length of output: 5113

precompiles/bank/bank_test.go (2)

10-10: LGTM: Import addition aligns with go-ethereum upgrade.

The addition of the uint256 package import is consistent with the PR objectives to upgrade go-ethereum and transition from math/big to uint256.


142-142: LGTM: Appropriate transition to uint256.

The change from big.NewInt(0) to uint256.NewInt(0) aligns with the go-ethereum upgrade objectives. Let's verify the consistency of this transition across the codebase.

rpc/backend/sign_tx.go (2)

31-31: LGTM: Import statement aligns with ethermint integration

The addition of the ethermint types import is consistent with the PR objectives of upgrading go-ethereum and integrating with ethermint.


73-73: Verify transaction signing compatibility

The switch to ethermint.MakeSigner is a significant change in the transaction signing mechanism. While this aligns with the PR objectives, we should verify:

  1. Backward compatibility with existing transactions
  2. Proper handling of different signer types
  3. Consistency with chain parameters

Let's verify the signer implementation:

testutil/keeper/mocks/fungible/evm.go (1)

Line range hint 30-56: Verify mock regeneration after interface changes.

The changes to ApplyMessage method signature and its implementation correctly reflect the upgrade to use pointer type for core.Message. This aligns with the PR objectives to upgrade go-ethereum.

Since this is a generated file (mockery v2.38.0), please ensure that:

  1. The source interface in the actual implementation has been updated
  2. The mocks have been regenerated using the same mockery version
✅ Verification successful

Let me gather more information to verify the interface definition and ensure the mock is properly generated.

The first command will help us find the interface definition to verify if it matches the mock implementation. The second command will show us the context around other usages of ApplyMessage to ensure consistency across the codebase.

This will help us determine if:

  1. The interface definition matches the mock implementation
  2. Other implementations or usages are consistent with the changes
  3. The mock generation is properly aligned with the source interface

Mock implementation correctly reflects the interface definition

The verification confirms that:

  1. The interface definition in x/fungible/types/expected_keepers.go declares ApplyMessage with the exact same signature using *core.Message
  2. The mock implementation in testutil/keeper/mocks/fungible/evm.go correctly implements this interface
  3. All mock files are consistently generated using mockery v2.38.0

The changes are properly aligned with the source interface and the mock generation is valid. No regeneration is needed as the mock already reflects the current interface definition.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify interface consistency and mock generation

# Test 1: Check if the source interface matches the mock
rg -A 2 "ApplyMessage.*core\.Message.*tracer.*commit" --type go

# Test 2: Verify mockery version in other generated files
rg "mockery v2.38.0" --type go

Length of output: 2792


Script:

#!/bin/bash
# Search for the interface definition that declares ApplyMessage
ast-grep --pattern 'type $_ interface {
  $$$
  ApplyMessage($$$) ($$$)
  $$$
}'

# Search for any other implementations or usages of ApplyMessage
rg "ApplyMessage.*Message.*tracer.*commit" --type go -B 2 -A 2

Length of output: 2080

rpc/backend/backend.go (2)

31-31: LGTM: Import addition is correct.

The addition of the math package import is necessary for the new HexOrDecimal64 type and follows Go import conventions.


135-138: LGTM: FeeHistory signature update aligns with go-ethereum upgrade.

The change from rpc.DecimalOrHex to math.HexOrDecimal64 maintains compatibility with the upgraded go-ethereum library.

Let's verify the consistency of this change across the codebase:

✅ Verification successful

The previous search might have been too specific. Let's broaden our search to:

  1. Find all FeeHistory method occurrences
  2. Look for both the new and old type references
  3. Search without namespace prefixes to catch all possible usages

This will help us verify the consistency of the type change across the codebase.


Type change verification complete - Implementation is consistent

The verification confirms that the change from rpc.DecimalOrHex to math.HexOrDecimal64 has been consistently implemented across the codebase:

  • All FeeHistory implementations use ethmath.HexOrDecimal64 or math.HexOrDecimal64:
    • Interface definition in rpc/backend/backend.go
    • Implementation in rpc/backend/chain_info.go
    • API layer in rpc/namespaces/ethereum/eth/api.go
    • Test cases in rpc/backend/chain_info_test.go

No instances of the old DecimalOrHex type remain in the codebase, confirming complete migration to the new type.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify FeeHistory implementation consistency
# Expected: All implementations should use math.HexOrDecimal64

# Search for FeeHistory implementations
ast-grep --pattern 'func $$$FeeHistory(blockCount $_, lastBlock $_, rewardPercentiles $_) $_'

# Search for any remaining usage of rpc.DecimalOrHex
rg "rpc\.DecimalOrHex"

Length of output: 120


Script:

#!/bin/bash
# Let's try a more comprehensive search approach

# Search for FeeHistory method signatures and implementations
rg -A 5 "FeeHistory.*\(" 

# Search for all occurrences of HexOrDecimal64
rg "HexOrDecimal64"

# Search for any remaining usage of DecimalOrHex (without rpc prefix to be thorough)
rg "DecimalOrHex"

Length of output: 5266

rpc/backend/utils.go (1)

33-33: LGTM: Import path and CalcBaseFee changes align with go-ethereum updates.

The changes correctly reflect the reorganization in go-ethereum where EIP-1559 related functionality was moved to a dedicated package. This improves code organization and makes dependencies more explicit.

Also applies to: 140-140

rpc/backend/chain_info.go (2)

28-28: LGTM: Import addition is well-placed and necessary.

The addition of the ethmath import is correctly placed and properly aliased, supporting the type change in the FeeHistory method.


174-174: LGTM: Type change aligns with go-ethereum upgrade.

The parameter type change from rpc.DecimalOrHex to ethmath.HexOrDecimal64 is well-implemented with proper validation. The change maintains backward compatibility while providing better type safety.

Let's verify that this change is consistently applied across all RPC endpoints:

✅ Verification successful

The previous search might have been too specific. By broadening our search criteria, we can better understand the type usage across the codebase and verify if the change is consistently applied.


Type change is consistently applied across the codebase

The verification confirms that:

  • All instances of ethmath.HexOrDecimal64 in fee-related contexts are consistently using the new type
  • The type is used in both the RPC API (rpc/namespaces/ethereum/eth/api.go) and backend implementation (rpc/backend/chain_info.go)
  • No legacy usages of rpc.DecimalOrHex remain in fee history related code

The type change has been properly propagated across all relevant components, maintaining consistency in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining usages of rpc.DecimalOrHex in fee-related functions
rg -A 2 "rpc\.DecimalOrHex.*Fee"

# Search for new type usage to ensure consistency
rg -A 2 "ethmath\.HexOrDecimal64.*Fee"

Length of output: 75


Script:

#!/bin/bash
# Let's broaden our search to find all usages of both types
# First, find all files that might be related to fee history
rg -l "FeeHistory" 

# Then search for all usages of both types without restricting to fee-related context
rg "rpc\.DecimalOrHex" 
rg "ethmath\.HexOrDecimal64"

# Also search for the method signature to understand the context better
ast-grep --pattern 'FeeHistory($$$) {
  $$$
}'

Length of output: 744

x/fungible/keeper/zevm_message_passing_test.go (3)

10-10: LGTM: Appropriate addition of uint256 package

The addition of the uint256 package aligns with the go-ethereum upgrade and provides more efficient handling of 256-bit integers commonly used in Ethereum-based operations.


86-86: LGTM: Consistent migration to uint256 for account balances

The systematic replacement of big.NewInt(0) with uint256.NewInt(0) for account balances is consistent with the go-ethereum upgrade. The changes maintain test coverage while adopting the more efficient uint256 implementation across all test scenarios.

Also applies to: 145-145, 232-232, 295-295


86-86: Verify consistent usage of uint256 across the codebase

Let's ensure all account balance initializations have been updated to use uint256.

✅ Verification successful

Let me gather more context about these balance initializations to ensure a thorough verification.

This will help us understand:

  1. The context of balance initializations in test files
  2. The struct definitions containing Balance fields to verify type consistency
  3. Whether the uint256 usage is consistent within the test context

Let me check one more aspect to verify the EVM account structure and its usage across the codebase.

This will help us:

  1. Verify the Account struct definition to confirm Balance field type
  2. Ensure all SetAccount calls consistently use uint256 for Balance initialization

Let me try one final search with a corrected pattern to find SetAccount usage.

This will help us:

  1. Find all SetAccount calls with proper regex syntax
  2. Locate and examine the EVM statedb Account definition to verify the Balance field type

Balance initialization is consistent in test files

Based on the verification results:

  1. All SetAccount calls in test files consistently use uint256.NewInt(0) for Balance initialization
  2. These initializations appear in test files only (zevm_message_passing_test.go and msg_server_vote_inbound_tx_test.go)
  3. The changes are localized to test scenarios and don't affect production code
  4. Other Account structs found in the codebase are unrelated to EVM account balances (they belong to config and Solana contracts)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining big.Int balance initializations in account-related code
rg -l 'Balance:\s+big\.NewInt' 

# Search for new uint256 balance initializations to confirm migration
rg -l 'Balance:\s+uint256\.NewInt'

Length of output: 171


Script:

#!/bin/bash
# Get the context around Balance initializations in both files
echo "=== zevm_message_passing_test.go ==="
rg -B 3 -A 3 'Balance:\s+uint256\.NewInt' x/fungible/keeper/zevm_message_passing_test.go

echo -e "\n=== vote_inbound_tx_test.go ==="
rg -B 3 -A 3 'Balance:\s+uint256\.NewInt' x/crosschain/keeper/msg_server_vote_inbound_tx_test.go

# Search for any other balance-related struct fields or types
echo -e "\n=== Balance field definitions ==="
ast-grep --pattern 'type $_ struct {
  $$$
  Balance $_
  $$$
}'

Length of output: 3134


Script:

#!/bin/bash
# Check for the statedb.Account structure definition and its usage
echo "=== Account struct definition ==="
ast-grep --pattern 'type Account struct {
  $$$
}'

# Look for any other SetAccount calls to ensure consistency
echo -e "\n=== SetAccount usage ==="
rg -B 2 -A 2 'SetAccount.*Account{'

Length of output: 1275


Script:

#!/bin/bash
# Search for SetAccount calls with fixed regex
echo "=== SetAccount usage ==="
rg -B 2 -A 2 'SetAccount.*Account\{'

# Look for the EVM statedb Account definition
echo -e "\n=== EVM Account definition ==="
rg -l "type Account struct" | xargs rg -B 1 -A 5 "type Account struct"

Length of output: 3213

rpc/backend/chain_info_test.go (1)

17-17: LGTM: Import addition aligns with go-ethereum upgrade

The addition of the ethmath import is consistent with the go-ethereum upgrade and follows proper import organization.

precompiles/staking/staking.go (3)

16-16: LGTM: Appropriate addition of uint256 package

The addition of the uint256 package is aligned with the go-ethereum upgrade and provides efficient handling of 256-bit integers.


236-240: Add test coverage for uint256 overflow handling

While the implementation of uint256 conversion and overflow checking is correct, the error path (lines 238-239) lacks test coverage. Please add test cases to verify the overflow handling behavior.

#!/bin/bash
# Description: Check test coverage for overflow scenarios
# Look for existing test cases that might cover overflow scenarios
rg -l "uint256.*FromBig.*overflow" --type go
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 238-239: precompiles/staking/staking.go#L238-L239
Added lines #L238 - L239 were not covered by tests


246-246: Verify impact of SubBalance type change

The change from *big.Int to *uint256.Int in SubBalance call is correct, but we should verify all callers handle uint256 properly.

✅ Verification successful

Let me gather more comprehensive information about the usage of SubBalance and type handling.

This will help us understand:

  1. How SubBalance is being used across the codebase
  2. The context of uint256 usage
  3. Any type conversion patterns between big.Int and uint256

Let me search for the actual usage of SubBalance to understand the type handling.


Let me check the type conversion and validation before the SubBalance call.


Type conversion to uint256 is properly handled

The code correctly validates and converts the amount to uint256 before calling SubBalance:

  1. Uses uint256.FromBig() with overflow check
  2. Returns error if conversion fails
  3. Only proceeds with SubBalance if conversion succeeds

The implementation is safe and handles the uint256 type properly. No changes needed in other parts of the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all SubBalance calls to ensure proper type handling
# Look for potential callers that might still use big.Int
rg "SubBalance.*big\.Int" --type go

Length of output: 46


Script:

#!/bin/bash
# Search for all SubBalance calls to understand the type patterns
rg "SubBalance\(" --type go -A 2

# Search for uint256 imports and usage patterns
rg "uint256" --type go -A 1

# Look specifically for big.Int to uint256 conversions
rg "FromBig|ToBig" --type go -A 1

Length of output: 281259


Script:

#!/bin/bash
# Search for SubBalance implementation and usage
ast-grep --pattern 'func $_ SubBalance($_, $amount $_) {
  $$$
}'

# Search for any direct SubBalance calls
rg "SubBalance\(" --type go -A 2

Length of output: 247


Script:

#!/bin/bash
# Look for the context around SubBalance to see type handling
rg -B 10 -A 2 "SubBalance\(" precompiles/staking/staking.go

# Look for uint256 imports and usage in the same file
rg "uint256|FromBig" precompiles/staking/staking.go

Length of output: 911

x/crosschain/keeper/evm_hooks.go (1)

43-43: Add test coverage for PostTxProcessing methods.

The signature changes from core.Message to *core.Message look good and align with the go-ethereum upgrade. However, these critical methods lack test coverage.

Would you like me to help create comprehensive test cases for these methods? Here's a verification script to confirm the test coverage:

Also applies to: 50-50

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 43-43: x/crosschain/keeper/evm_hooks.go#L43
Added line #L43 was not covered by tests

x/crosschain/keeper/msg_server_vote_inbound_tx_test.go (1)

14-14: LGTM: Appropriate migration to uint256

The change from big.Int to uint256.Int aligns with the go-ethereum upgrade objectives. The uint256 package provides more efficient handling of 256-bit integers commonly used in Ethereum-based systems.

Also applies to: 70-70

go.mod (3)

338-350: Review new dependencies for security implications.

Several new dependencies have been added:

  • Microsoft/go-winio
  • bits-and-blooms/bitset
  • consensys packages
  • ethereum/c-kzg-4844

These additions appear to be related to the go-ethereum upgrade. However, we should verify their necessity and security status.

#!/bin/bash
# Check if new dependencies are actually used in the codebase
echo "=== Checking usage of new dependencies ==="
for dep in "go-winio" "bitset" "c-kzg-4844" "go-verkle"; do
  echo "Checking usage of $dep:"
  rg -l "$dep" | grep -v "go.mod"
done

380-383: Validate the custom fork specifications.

The replace directives point to custom forks of critical dependencies:

  • go-ethereum
  • go-libp2p
  • go-tss

While this aligns with the PR objectives, ensure these forks are maintained and up-to-date with their upstream repositories.

#!/bin/bash
# Verify the existence and activity of forked repositories
echo "=== Checking fork status ==="
for repo in "zeta-chain/go-ethereum" "zeta-chain/go-libp2p" "zeta-chain/go-tss"; do
  echo "Checking $repo:"
  gh repo view "$repo" --json updatedAt,pushedAt,defaultBranch
done

27-27: Verify compatibility with the forked go-ethereum version.

The upgrade from go-ethereum v1.10.26 to v1.13.15 is significant, and the codebase is using a custom fork. While this aligns with the PR objectives, we should verify the compatibility of other dependencies that rely on go-ethereum.

Also applies to: 380-380

✅ Verification successful

Let me try a different approach to verify the go-ethereum usage and potential compatibility issues.

This script will help us:

  1. Identify direct imports and their context
  2. Confirm the replace directive details
  3. Find actual usage of ethereum types and interfaces in the code

This information will help us better assess the compatibility impact of the upgrade and fork usage.


Based on the comprehensive analysis of the codebase, I can now provide a final response regarding the go-ethereum upgrade compatibility.

The go-ethereum upgrade appears to be safely implemented with proper version control

The analysis reveals:

  • Limited direct imports of go-ethereum, primarily for common types and interfaces
  • Usage is concentrated in test utilities, contracts, and RPC implementations
  • The replace directive correctly points to a specific commit of the forked version
  • Most usages are standard ethereum types (Subscription, FilterQuery, CallMsg) that are stable across versions

The upgrade from v1.10.26 to v1.13.15 with the fork at v1.13.16-0.20241022183758-422c6ef93ccc is well-controlled, and the codebase shows no signs of using deprecated or breaking features. The fork's version being slightly ahead (v1.13.16-0) suggests it includes necessary patches while maintaining compatibility.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential version conflicts with go-ethereum
rg -l "github.com/ethereum/go-ethereum" | grep -v "go.mod" | while read -r file; do
  echo "=== Checking $file for go-ethereum imports ==="
  rg "github.com/ethereum/go-ethereum" "$file" -A 5
done

Length of output: 4143


Script:

#!/bin/bash
# Check for direct imports of go-ethereum packages
echo "=== Direct imports of go-ethereum packages ==="
rg --type go "^import.*ethereum/go-ethereum" -A 3

# Check for replace directives related to go-ethereum
echo -e "\n=== Replace directives for go-ethereum ==="
grep -A 1 "github.com/ethereum/go-ethereum" go.mod

# Check for any references to ethereum types or interfaces
echo -e "\n=== Usage of ethereum types and interfaces ==="
rg --type go "ethereum\." -B 1 -A 1

Length of output: 23284

precompiles/bank/method_test.go (3)

13-13: LGTM: Import addition aligns with go-ethereum upgrade

The addition of the uint256 package import is consistent with the PR objectives of upgrading go-ethereum and its dependencies.


554-554: LGTM: Updated contract initialization to use uint256

The change from big.NewInt(0) to uint256.NewInt(0) aligns with the go-ethereum upgrade requirements and maintains type consistency.


Line range hint 1-700: Comprehensive test coverage verified

The test suite thoroughly covers:

  • Error cases for deposit/withdraw operations
  • Balance checks
  • Permission validations
  • Edge cases for amounts
  • Read-only method restrictions

This robust test coverage helps ensure the stability of the bank precompile contract through the go-ethereum upgrade.

rpc/namespaces/ethereum/eth/api.go (3)

24-24: LGTM: Import addition is appropriate.

The addition of the ethmath import alias is well-structured and necessary for the updated FeeHistory method signature.


Line range hint 354-359: LGTM: Implementation aligns with interface changes.

The FeeHistory implementation correctly adopts the ethmath.HexOrDecimal64 type while maintaining the existing functionality and proper debug logging.


102-105: Verify interface implementation updates.

The parameter type change from rpc.DecimalOrHex to ethmath.HexOrDecimal64 needs to be reflected in all implementations of the EthereumAPI interface.

precompiles/staking/staking_test.go (2)

82-82: LGTM: Appropriate use of uint256 for contract value initialization

The change from big.NewInt(0) to uint256.NewInt(0) aligns with the go-ethereum upgrade objectives and provides better type consistency for EVM operations.


Line range hint 4-6: Verify test coverage restoration plan

Most test cases are currently commented out with reference to issue #3005. While this aligns with the current disabled state of staking methods, we should ensure there's a plan to restore test coverage once the functionality is re-enabled.

Let's check the referenced issue and related test coverage:

✅ Verification successful

Let me gather more information about the test coverage and commented tests.

This will help us understand:

  1. The extent of commented test cases
  2. Any plans or markers for test restoration
  3. Currently active test cases

Test coverage is appropriately maintained during feature disablement

The test suite is well-structured with the following observations:

  • Issue Disable stake, unstake and moveStake #3005 (now closed) explicitly required disabling stake, unstake, and moveStake methods
  • All core test cases are present but appropriately modified to verify the disabled state
  • Tests for disabled methods (Test_Stake, Test_Unstake, Test_MoveStake) verify they return the expected "disabled" error
  • Supporting functionality tests remain active and comprehensive:
    • ABI method presence verification
    • Gas requirement checks
    • Validator querying
    • Share retrieval
    • Error cases

The current test coverage aligns with the intentional disablement of specific functionality, maintaining proper error validation while preserving the test structure for future re-enablement.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the referenced issue status
gh issue view 3005 --repo zeta-chain/node

# Check current test coverage for this package
go test -coverprofile=coverage.out ./precompiles/staking/...
go tool cover -func=coverage.out

Length of output: 561


Script:

#!/bin/bash
# Check for commented test cases in the staking test file
rg -U "func.*Test.*\{[\s\S]*?\}" precompiles/staking/staking_test.go -A 5

# Look for any TODOs or related comments about test restoration
rg -i "todo|fixme|uncomment|restore" precompiles/staking/staking_test.go

# Check if there are any active test cases
rg "^func.*Test.*\{.*$" precompiles/staking/staking_test.go

Length of output: 7524

x/fungible/keeper/evm.go (3)

20-20: Importing the core package for Message struct

The addition of the "github.com/ethereum/go-ethereum/core" import is necessary for utilizing the updated core.Message struct introduced in the new version of go-ethereum.


739-751: Validate gas field initialization in core.Message

In the construction of core.Message, all gas-related fields (GasFeeCap, GasTipCap, and GasPrice) are set to big.NewInt(0). Given the adoption of EIP-1559, it is standard to use GasFeeCap and GasTipCap for transaction fees, while GasPrice may be deprecated or unnecessary.

Please verify if setting GasPrice to zero is required, or if it can be omitted to prevent potential confusion or unintended behavior.


803-803: Correct usage of msg.Data field

The change from msg.Data() to msg.Data aligns with the structural updates in the core.Message object. Accessing the Data field directly is appropriate and ensures accurate encoding of the transaction data.

server/json_rpc.go Show resolved Hide resolved
server/json_rpc.go Show resolved Hide resolved
server/json_rpc.go Show resolved Hide resolved
server/json_rpc.go Show resolved Hide resolved
pkg/proofs/ethereum/proof.go Show resolved Hide resolved
rpc/backend/utils.go Show resolved Hide resolved
rpc/backend/chain_info_test.go Show resolved Hide resolved
x/crosschain/keeper/evm_hooks.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
@ws4charlie
Copy link
Contributor

Nice. Once we have this update, the Ethereum blob-tx can be handled in a native way.

tolerate 1.13 changes

fix eth proof trie initilization

fix go-ethereum spc

lint fix

final pre merge version
@gartnera gartnera added this pull request to the merge queue Nov 5, 2024
Merged via the queue into develop with commit 55c01c2 Nov 5, 2024
41 checks passed
@gartnera gartnera deleted the upgrade-go-ethereum branch November 5, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Skip changelog CI check UPGRADE_TESTS Run make start-upgrade-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants