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

refactor(zetaclient): converge EVM clients #3428

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

swift1337
Copy link
Contributor

@swift1337 swift1337 commented Jan 28, 2025

This PR drops https://github.com/onrik/ethrpc in favor of geth rpc + custom parsing logic for some methods that fail on L2s

  • Ensure geth RPC only
  • Ensure all RPC methods have context
  • Drop old package
  • Fix unit tests
  • Fix e2e tests

Closes #3386 (⬅️ all PR context located here)
Related ethereum/go-ethereum#29407

Summary by CodeRabbit

Based on the comprehensive summary of changes, here are the release notes:

Release Notes

  • New Features

    • Added liquidity cap parameter for ZRC20 creation
    • Introduced mechanism to register aborted CCTX for Bitcoin inbound transactions
    • Added CLI command to fetch inbound ballots from an inbound hash
    • Enhanced EVM client convergence in ZetaClient
  • Improvements

    • Upgraded cosmos-sdk to version 50.x
    • Updated orchestrator to version 2
    • Refactored EVM observer-signer
    • Added support for additional chains (Avalanche, Arbitrum, World Chain)
    • Implemented new Bitcoin RPC in ZetaClient
  • Bug Fixes

    • Removed minimum rent exempt check for SPL token withdrawals
    • Added support for withdrawals in ZetaChain during onRevert calls
    • Added nil gas price check in transaction fee validation
  • Technical Updates

    • Consolidated protocol contract imports
    • Removed dependencies on specific Ethereum RPC libraries
    • Enhanced context management across various client methods

Copy link
Contributor

coderabbitai bot commented Jan 28, 2025

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

This pull request introduces a comprehensive refactoring of the EVM client and RPC interaction mechanisms within the ZetaChain node. The primary objective is to converge different Ethereum RPC client implementations, replacing the ethrpc library with a more unified and robust client approach. The changes span multiple packages, focusing on transaction handling, observer functionality, and testing infrastructure.

Changes

File Path Change Summary
go.mod Removed dependencies github.com/nanmu42/etherscan-api and github.com/onrik/ethrpc
zetaclient/chains/evm/client/* Introduced new client implementation with enhanced transaction and block handling
zetaclient/chains/interfaces/interfaces.go Updated EVMRPCClient interface with new methods for block and transaction retrieval
zetaclient/testutils/mocks/* Updated mock implementations to support new client structure

Assessment against linked issues

Objective Addressed Explanation
Converge geth RPC with HTTP JSON RPC clients Replaced ethrpc with a unified client implementation
Ensure compatibility with L2 chains New client structure provides more flexible RPC interaction
Remove dependency on third-party RPC library Eliminated ethrpc dependency

Possibly related PRs

Suggested Labels

breaking:cli, no-changelog, E2E, UPGRADE_LIGHT_TESTS

Suggested Reviewers

  • fbac
  • kingpinXD
  • lumtis
  • skosito
  • brewmaster012

The refactoring represents a significant step towards standardizing and improving the EVM client interaction in the ZetaChain ecosystem, addressing potential inconsistencies and enhancing overall code maintainability.


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@swift1337 swift1337 changed the title Refactor(zetaclient):! drop github.com/onrik/ethrpc Refactor(zetaclient)!: drop github.com/onrik/ethrpc Jan 28, 2025
@swift1337 swift1337 changed the title Refactor(zetaclient)!: drop github.com/onrik/ethrpc Refactor(zetaclient): converge EVM clients Jan 28, 2025
@swift1337 swift1337 self-assigned this Jan 28, 2025
@swift1337 swift1337 changed the title Refactor(zetaclient): converge EVM clients refactor(zetaclient): converge EVM clients Jan 28, 2025
@swift1337 swift1337 marked this pull request as ready for review January 28, 2025 15:36
@swift1337 swift1337 requested a review from a team as a code owner January 28, 2025 15:36
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 20.45455% with 140 lines in your changes missing coverage. Please review.

Project coverage is 62.64%. Comparing base (d162dda) to head (2326cfd).

Files with missing lines Patch % Lines
zetaclient/chains/evm/client/client2.go 0.00% 133 Missing ⚠️
zetaclient/chains/evm/client/client.go 0.00% 2 Missing ⚠️
zetaclient/chains/evm/observer/inbound.go 50.00% 2 Missing ⚠️
zetaclient/chains/evm/observer/observer.go 86.66% 2 Missing ⚠️
zetaclient/testutils/testdata.go 92.30% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3428      +/-   ##
===========================================
- Coverage    62.89%   62.64%   -0.26%     
===========================================
  Files          436      437       +1     
  Lines        30796    30917     +121     
===========================================
- Hits         19369    19367       -2     
- Misses       10615    10739     +124     
+ Partials       812      811       -1     
Files with missing lines Coverage Δ
zetaclient/chains/evm/common/validation.go 100.00% <100.00%> (ø)
zetaclient/chains/evm/observer/observer_gas.go 58.57% <100.00%> (ø)
zetaclient/chains/evm/observer/outbound.go 62.35% <100.00%> (ø)
...taclient/chains/evm/observer/v2_inbound_tracker.go 0.00% <ø> (ø)
zetaclient/chains/evm/signer/signer.go 41.03% <100.00%> (ø)
zetaclient/orchestrator/v2_bootstrap.go 70.79% <100.00%> (+1.04%) ⬆️
zetaclient/testutils/testdata.go 86.74% <92.30%> (+0.07%) ⬆️
zetaclient/chains/evm/client/client.go 0.00% <0.00%> (ø)
zetaclient/chains/evm/observer/inbound.go 39.63% <50.00%> (ø)
zetaclient/chains/evm/observer/observer.go 70.28% <86.66%> (ø)
... and 1 more

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: 2

🧹 Nitpick comments (9)
zetaclient/chains/evm/client/client.go (1)

18-18: Consider potential method overshadowing due to embedded struct
By embedding *ethclient.Client in the Client struct, any method name clash would override the embedded method. If this is an intentional design choice for convenience, it is fine; otherwise, developers should be aware of potential overshadowing issues.

zetaclient/chains/evm/client/client2.go (2)

1-36: Avoid using basic int for large block numbers
Using a plain int for Block.Number or Transaction.Nonce may limit handling of higher block heights or large nonce values. Consider using uint64 or a *big.Int if 32-bit integer overflow is a concern.

-type Block struct {
-   Number int
+type Block struct {
+   Number int64  // or uint64
    ...

92-150: Comprehensive parsing with potential improvements on partial failures
Parsing transactions individually within the block is a good approach. However, if one transaction fails to parse, the entire block parsing is aborted. Depending on use cases, you may consider skipping or logging partial transaction parse failures.

zetaclient/chains/evm/observer/observer.go (2)

155-156: Solid block retrieval and caching approach
CheckTxInclusion’s reliance on GetBlockByNumberCached is a neat performance optimization. Ensure block caching size is sufficiently bounded for large-scale usage.


197-202: Robust caching logic
The GetBlockByNumberCached function effectively handles cache hits. Ensure appropriate cache invalidation for blocks that might be reorganized on ephemeral test networks or near chain heads.

Also applies to: 208-208

zetaclient/testutils/testrpc/rpc_evm.go (1)

33-37: Enhance MockSendTransaction to support more test scenarios.

The current implementation always returns success (nil, nil). Consider enhancing it to:

  1. Return a mock transaction hash
  2. Support error injection for failure scenarios
-func (s *EVMServer) MockSendTransaction() {
+func (s *EVMServer) MockSendTransaction(mockHash string, err error) {
 	s.On("eth_sendRawTransaction", func(_ []any) (any, error) {
-		return nil, nil
+		return mockHash, err
 	})
 }
zetaclient/chains/evm/client/client_live_test.go (1)

50-103: Consider implementing proper rate limiting instead of using sleep.

While the tests are well-structured using table-driven tests, the time.Sleep(1 * time.Second) between API calls suggests potential rate limiting issues.

Consider implementing a proper rate limiter:

-time.Sleep(1 * time.Second)
+// Add at the top of the file
+var rateLimiter = rate.NewLimiter(rate.Every(time.Second), 1)

+// Replace sleep with
+err := rateLimiter.Wait(ts.ctx)
+require.NoError(t, err)
zetaclient/chains/evm/observer/inbound_test.go (1)

449-472: Enhance mock setup with more specific error cases.

The mock setup could be improved by adding test cases for specific RPC error scenarios.

Consider adding test cases for:

{
    name: "should handle rate limiting errors",
    mockEVMClient: func(m *mocks.EVMRPCClient) {
        m.On("BlockNumber", mock.Anything).Return(uint64(0), errors.New("rate limit exceeded"))
        m.On("BlockByNumber2", mock.Anything, mock.Anything).Return(nil, errors.New("rate limit exceeded"))
    },
    errMsg: "rate limit exceeded",
},
{
    name: "should handle timeout errors",
    mockEVMClient: func(m *mocks.EVMRPCClient) {
        m.On("BlockNumber", mock.Anything).Return(uint64(0), context.DeadlineExceeded)
        m.On("BlockByNumber2", mock.Anything, mock.Anything).Return(nil, context.DeadlineExceeded)
    },
    errMsg: "context deadline exceeded",
}
zetaclient/chains/evm/observer/inbound.go (1)

Line range hint 222-222: Consider adding retry mechanism for failed transactions.

The comment "TODO: simplify this function and reduce the number of argument" could be addressed while also improving error handling.

Consider implementing a retry mechanism with exponential backoff:

func parseOutboundReceivedValue(
    ctx context.Context,
    cctx *crosschaintypes.CrossChainTx,
    receipt *ethtypes.Receipt,
    transaction *ethtypes.Transaction,
    params ParseParams, // New struct to group related parameters
) (*big.Int, chains.ReceiveStatus, error) {
    var lastErr error
    b := backoff.NewExponentialBackOff()
    b.MaxElapsedTime = 30 * time.Second

    operation := func() error {
        value, status, err := tryParseOutboundValue(ctx, cctx, receipt, transaction, params)
        if err != nil {
            lastErr = err
            return err
        }
        return nil
    }

    if err := backoff.Retry(operation, b); err != nil {
        return nil, chains.ReceiveStatus_failed, fmt.Errorf("failed after retries: %w", lastErr)
    }
    return value, status, nil
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d162dda and 2326cfd.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (26)
  • changelog.md (1 hunks)
  • go.mod (1 hunks)
  • zetaclient/chains/evm/client/client.go (3 hunks)
  • zetaclient/chains/evm/client/client2.go (1 hunks)
  • zetaclient/chains/evm/client/client_live_test.go (3 hunks)
  • zetaclient/chains/evm/common/validation.go (2 hunks)
  • zetaclient/chains/evm/common/validation_test.go (17 hunks)
  • zetaclient/chains/evm/observer/inbound.go (9 hunks)
  • zetaclient/chains/evm/observer/inbound_test.go (3 hunks)
  • zetaclient/chains/evm/observer/observer.go (7 hunks)
  • zetaclient/chains/evm/observer/observer_gas.go (2 hunks)
  • zetaclient/chains/evm/observer/observer_test.go (11 hunks)
  • zetaclient/chains/evm/observer/outbound.go (4 hunks)
  • zetaclient/chains/evm/observer/outbound_test.go (2 hunks)
  • zetaclient/chains/evm/observer/v2_inbound_tracker.go (2 hunks)
  • zetaclient/chains/evm/signer/signer.go (2 hunks)
  • zetaclient/chains/evm/signer/signer_test.go (3 hunks)
  • zetaclient/chains/interfaces/interfaces.go (4 hunks)
  • zetaclient/orchestrator/v2_bootstrap.go (1 hunks)
  • zetaclient/testutils/constant.go (0 hunks)
  • zetaclient/testutils/mocks/evm_json_rpc.go (0 hunks)
  • zetaclient/testutils/mocks/evm_rpc.go (8 hunks)
  • zetaclient/testutils/mocks/zetacore_client.go (3 hunks)
  • zetaclient/testutils/testdata.go (8 hunks)
  • zetaclient/testutils/testrpc/rpc_evm.go (1 hunks)
  • zetaclient/testutils/types/ethrpc.go (0 hunks)
💤 Files with no reviewable changes (3)
  • zetaclient/testutils/constant.go
  • zetaclient/testutils/types/ethrpc.go
  • zetaclient/testutils/mocks/evm_json_rpc.go
👮 Files not reviewed due to content moderation or server errors (4)
  • zetaclient/testutils/mocks/zetacore_client.go
  • go.mod
  • zetaclient/chains/evm/observer/outbound_test.go
  • changelog.md
🧰 Additional context used
📓 Path-based instructions (21)
zetaclient/chains/evm/observer/outbound_test.go (1)

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

zetaclient/chains/evm/observer/observer_gas.go (1)

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

zetaclient/chains/evm/common/validation.go (1)

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

zetaclient/testutils/testrpc/rpc_evm.go (1)

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

zetaclient/chains/evm/signer/signer.go (1)

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

zetaclient/chains/evm/client/client.go (1)

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

zetaclient/chains/evm/client/client_live_test.go (1)

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

zetaclient/orchestrator/v2_bootstrap.go (1)

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

zetaclient/chains/interfaces/interfaces.go (1)

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

zetaclient/chains/evm/common/validation_test.go (1)

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

zetaclient/testutils/mocks/zetacore_client.go (1)

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

zetaclient/chains/evm/observer/v2_inbound_tracker.go (1)

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

zetaclient/chains/evm/signer/signer_test.go (1)

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

zetaclient/chains/evm/observer/observer_test.go (1)

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

zetaclient/chains/evm/observer/outbound.go (1)

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

zetaclient/chains/evm/observer/inbound_test.go (1)

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

zetaclient/chains/evm/client/client2.go (1)

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

zetaclient/testutils/mocks/evm_rpc.go (1)

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

zetaclient/chains/evm/observer/observer.go (1)

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

zetaclient/chains/evm/observer/inbound.go (1)

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

zetaclient/testutils/testdata.go (1)

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

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: build-zetanode
  • GitHub Check: build-and-test
  • GitHub Check: gosec
  • GitHub Check: lint
  • GitHub Check: build
  • GitHub Check: Analyze (go)
🔇 Additional comments (43)
zetaclient/chains/evm/client/client.go (2)

47-47: Initialization approach is clear
The New constructor is concise, leveraging direct embedding. This simplifies retrieval of the necessary methods from ethclient.Client for RPC interactions. No issues found.


103-103: Add a nil check for returned header
While it’s rare, consider verifying that header is non-nil after calling HeaderByNumber. If the node returns no header for the given block, a subsequent dereference could cause a panic.

 header, err := c.HeaderByNumber(ctx, new(big.Int).SetUint64(bn))
 if err != nil {
   return time.Time{}, errors.Wrapf(err, "unable to get block header for block %d", bn)
 }
+if header == nil {
+  return time.Time{}, errors.Errorf("nil header returned for block %d", bn)
+}
✅ Verification successful

Add nil check for header - Confirmed Necessary

The suggestion to add a nil check for the header is validated by established patterns across the codebase. Multiple components, including the filters package and backend implementation, explicitly handle nil header cases. The proposed change follows existing defensive programming practices and prevents potential panics.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Search for HeaderByNumber interface and implementation
ast-grep --pattern 'HeaderByNumber($$$)'

# Look for any existing nil checks on header
rg 'header.*== nil' -A 2

# Find the interface definition
rg 'interface.*\{.*HeaderByNumber' -A 5 --multiline

Length of output: 583


Script:

#!/bin/bash
# Look for the client implementation and related comments
rg -B 2 -A 2 'func.*HeaderByNumber'

# Search for any comments or documentation about header handling
rg -B 2 'HeaderByNumber' --type go --context 3

Length of output: 10292

zetaclient/chains/evm/client/client2.go (4)

70-79: Transaction retrieval logic looks straightforward
The code aligns with BlockByNumber2. Any chain-specific transaction type differences are parsed via parseTransaction. This is a solid approach.


81-90: Method abstraction is clear
The call method centralizes the JSON-RPC invocation. This is a clean design choice, ensuring consistent error handling. No immediate issues.


152-186: Conscious use of pointer fields
Storing BlockNumber and TransactionIndex as pointer fields allows indicating absent or pending transactions. This is valid. Be aware of potential nil dereferences in usage sites.


188-228: Hex parsing utilities are well-structured
The hexInt and hexBig types, along with parseBigInt and parseInt, neatly separate parsing logic, improving readability. The error messages are sufficiently descriptive.

zetaclient/chains/evm/observer/observer.go (6)

8-8: Import of "math/big" is warranted
Adding "math/big" appears justified to manage unbounded integers consistently for block numbers. No issues found.


34-34: Use of interfaces.EVMRPCClient promotes design flexibility
Injecting the interface fosters clean abstractions in the observer. This approach facilitates testing and decouples the observer from ethclient.Client specifics.


54-58: Constructor is streamlined
Shifting to a single client parameter in New simplifies instantiation. The direct usage and loading of the last block scanned is a concise approach.


149-150: Storing id improves readability
Using a local variable clarifies the code and avoids calling OutboundID(nonce) multiple times.


179-180: Switchover from TransactionByHash
Using TransactionByHash2 and validating transactions with common.ValidateEvmTransaction is consistent with the new client approach. No issues.


222-223: New bridging method
BlockByNumber2 usage aligns well with the rest of the refactor. This consistency simplifies the observer logic and fosters a single point of truth for block retrieval.

zetaclient/testutils/testrpc/rpc_evm.go (1)

27-31: LGTM! Clean implementation of block number failure mocking.

The implementation follows testing best practices by allowing explicit error injection.

zetaclient/chains/evm/observer/observer_gas.go (2)

13-13: LGTM! Clean transition to direct evmClient usage.

The change aligns with the PR objective of streamlining EVM client implementation.


94-94: LGTM! Consistent implementation of direct client usage.

The HeaderByNumber call follows the same pattern of direct evmClient usage.

zetaclient/chains/evm/common/validation.go (1)

35-35: LGTM! Clean transition to internal client.Transaction type.

The change successfully removes the ethrpc dependency while maintaining the same robust validation logic.

zetaclient/chains/evm/observer/v2_inbound_tracker.go (2)

24-24: LGTM! Consistent use of internal client.Transaction type.

The change aligns with the PR objective of removing ethrpc dependency.


Line range hint 19-20: Address TODO: Add test coverage for ProcessInboundTrackerV2.

This is a critical component handling inbound transactions. Test coverage should be prioritized.

Would you like me to help create comprehensive test cases for this functionality?

✅ Verification successful

Test coverage needed for ProcessInboundTrackerV2 - Review confirmed.

The verification process confirms that there is no test coverage for the ProcessInboundTrackerV2 function. While there are tests for inbound trackers in other components of the system, this specific implementation remains untested. Given that this is a critical component handling inbound transactions, implementing test coverage should be prioritized.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check current test coverage for the file
go test -coverprofile=coverage.out ./zetaclient/chains/evm/observer/
go tool cover -func=coverage.out | grep "v2_inbound_tracker.go"

Length of output: 16601


Script:

#!/bin/bash
# Search for test files and implementations
echo "=== Searching for test files ==="
fd "v2_inbound_tracker_test.go"

echo -e "\n=== Searching for test implementations ==="
ast-grep --pattern 'func Test$_($_) {
  $$$
}'

echo -e "\n=== Searching for test patterns ==="
rg -l "TestProcessInboundTrackerV2" .
rg -l "Test.*InboundTracker" .

Length of output: 819

zetaclient/orchestrator/v2_bootstrap.go (1)

94-94: LGTM! Clean refactoring of observer instantiation.

The simplified observer creation aligns well with the goal of converging EVM clients, removing unnecessary complexity while maintaining functionality.

zetaclient/chains/evm/client/client_live_test.go (1)

105-156: LGTM! Comprehensive test coverage for transaction types.

The test cases effectively cover both regular transactions and L1 deposits, ensuring compatibility with different EVM chains.

zetaclient/chains/evm/signer/signer_test.go (1)

48-54: LGTM! Improved test infrastructure with EVM server.

The transition from mocks to a test server provides a more realistic testing environment. The setup properly handles chain ID configuration and transaction mocking.

zetaclient/chains/interfaces/interfaces.go (1)

167-169: LGTM! Enhanced RPC client interface with modern capabilities.

The addition of BlockByNumber2, TransactionByHash2, and HealthCheck methods improves the interface's functionality and operational monitoring capabilities.

zetaclient/testutils/testdata.go (4)

18-19: LGTM! Import changes align with refactoring goals.

The addition of the evmclient import supports the transition to using Geth client exclusively for EVM operations.


97-107: LGTM! Clean transition to evmclient.Block.

The function has been properly updated to use the new block type while maintaining existing functionality and error handling.


Line range hint 163-247: LGTM! Consistent updates across transaction-related functions.

All transaction-related functions have been systematically updated to use evmclient.Transaction, maintaining consistency throughout the codebase.


Line range hint 364-368: LGTM! Clean type transition in helper function.

The function has been correctly updated to use evmclient.Block while preserving its original functionality.

zetaclient/chains/evm/observer/observer_test.go (3)

82-94: LGTM! Enhanced test setup with proper RPC server.

The test setup has been improved by:

  • Using context properly
  • Replacing mock with a dedicated test RPC server
  • Following better testing practices

130-143: LGTM! More realistic RPC failure testing.

The test case has been improved to use a more realistic approach for simulating RPC failures through the test RPC server.


Line range hint 258-281: LGTM! Proper type transition in cache tests.

The block cache tests have been correctly updated to use client.Block while maintaining comprehensive error handling and validation.

zetaclient/chains/evm/common/validation_test.go (1)

Line range hint 1-143: LGTM! Comprehensive test updates.

The test file has been properly updated with:

  • Latest mockery version
  • Appropriate imports
  • Consistent use of client.Transaction
zetaclient/testutils/mocks/evm_rpc.go (3)

57-85: LGTM! Well-structured mock method addition.

The BlockByNumber2 method has been properly implemented with comprehensive error handling and type assertions.


263-289: LGTM! Proper health check implementation.

The HealthCheck method has been correctly implemented with appropriate time.Time handling and error management.


494-522: LGTM! Well-implemented transaction retrieval method.

The TransactionByHash2 method has been properly implemented with appropriate type handling and error management.

zetaclient/chains/evm/signer/signer.go (2)

184-184: LGTM! Improved signature handling.

The change to use signer.client.Signer instead of signer.client directly is more explicit and follows better encapsulation principles.


232-232: LGTM! Simplified transaction broadcasting.

The change to use signer.client.SendTransaction directly instead of going through EVMRPCClient reduces unnecessary indirection and aligns with the PR's goal of streamlining the EVM client implementation.

zetaclient/chains/evm/observer/outbound.go (2)

388-388: LGTM! Enhanced context propagation.

Adding context to GetBlockByNumberCached improves request lifecycle management and allows for proper cancellation/timeout handling.


493-493: LGTM! Improved transaction validation.

Adding context to CheckTxInclusion ensures proper request lifecycle management during transaction validation.

zetaclient/chains/evm/observer/inbound_test.go (2)

15-15: LGTM! Updated import for new client package.

Replacing ethrpc with the new client package aligns with the PR's objective of converging EVM clients.


412-412: LGTM! Updated transaction type.

The change to use *client.Transaction aligns with the new client structure.

zetaclient/chains/evm/observer/inbound.go (4)

26-26: LGTM! Updated import for new client package.

Replacing ethrpc with the new client package aligns with the PR's objective.


45-45: LGTM! Enhanced context propagation in transaction retrieval.

Adding context to TransactionByHash calls improves request lifecycle management.

Also applies to: 345-345


401-401: LGTM! Updated transaction type usage.

The change to use *client.Transaction consistently across all methods improves code consistency and type safety.

Also applies to: 456-456, 507-507, 660-660


710-710: LGTM! Improved block retrieval with context.

Adding context to GetBlockByNumberCached ensures proper request lifecycle management.

Comment on lines 56 to 69
// BlockByNumber2 is alternative to geth BlockByNumber that supports NON-ETH chains.
// For example, OP stack has different tx types that result in err="transaction type not supported"
//
// See https://github.com/zeta-chain/node/issues/3386
// See https://github.com/ethereum/go-ethereum/issues/29407
func (c *Client) BlockByNumber2(ctx context.Context, blockNumber *big.Int) (*Block, error) {
raw, err := c.call(ctx, "eth_getBlockByNumber", hexutil.EncodeBig(blockNumber), true)
if err != nil {
return nil, errors.Wrapf(err, "block %d", blockNumber.Uint64())
}

return parseBlock(raw)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Add block number validation to prevent subtle RPC failures

The codebase consistently validates big.Int parameters. While the immediate caller ensures non-nil values, adding validation in BlockByNumber2 aligns with established patterns and prevents potential RPC errors from negative block numbers.

if blockNumber == nil || blockNumber.Sign() < 0 {
    return nil, errors.New("invalid block number")
}
🔗 Analysis chain

Ensure validity of blockNumber in BlockByNumber2
The method is well-structured. However, verify that blockNumber is not nil or negative in upstream code. The RPC call may fail subtly if an unexpected value is passed.

func (c *Client) BlockByNumber2(ctx context.Context, blockNumber *big.Int) (*Block, error) {
+   if blockNumber == nil || blockNumber.Sign() < 0 {
+       return nil, errors.New("invalid block number")
+   }
    raw, err := c.call(ctx, "eth_getBlockByNumber", hexutil.EncodeBig(blockNumber), true)
    ...
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Find all calls to BlockByNumber2
echo "=== Finding BlockByNumber2 callers ==="
rg "BlockByNumber2" -B 2 -A 2

echo -e "\n=== Looking for similar block number validation patterns ==="
rg "blockNumber.*nil|Sign\(\).*0" -B 2 -A 2

echo -e "\n=== Finding original geth BlockByNumber implementation ==="
rg "func.*BlockByNumber.*big\.Int.*error" -B 2 -A 2

Length of output: 12310

zetaclient/chains/interfaces/interfaces.go Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of using the something2 naming, name could be made more explicit I believe. By L2 I think we refer to Base for now so the OP Stack? Maybe client_optimism can fit as a name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is relevant to any chain that has custom TX types that diverge from ETH. I don't have a better name 😅

//
// See https://github.com/zeta-chain/node/issues/3386
// See https://github.com/ethereum/go-ethereum/issues/29407
func (c *Client) BlockByNumber2(ctx context.Context, blockNumber *big.Int) (*Block, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment for the file name

@swift1337 swift1337 requested a review from gartnera January 29, 2025 10:03
Comment on lines +167 to +169
BlockByNumber2(ctx context.Context, number *big.Int) (*ethclient.Block, error)
TransactionByHash2(ctx context.Context, hash string) (*ethclient.Transaction, error)
HealthCheck(ctx context.Context) (time.Time, error)
Copy link
Member

Choose a reason for hiding this comment

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

Remove BlockByNumber and TransactionByHash from the interface so they are not accidentally used

Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer a custom client that explicitly wraps the underlaying client which ensures that you cannot accidentally use HeaderByNumber

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVM: converge geth rpc with http json rpc clients
3 participants