chore(all): libevm phase 2.5 changes needed for subnet-evm #3923

qdm12 · 2025-05-01T09:11:00Z

Why this should be merged

This was meant to be merged in the master branch post libevm 2.5 completion on both coreth and subnet-evm, since it was the branch used as the avalanchego base for both coreth and subnet-evm.

How this works

Import libevm instead of coreth wherever possible
Use libevm ethclient.Client whenever possible
- Use NonceAt with nil block number instead of AcceptedNonceAt
- Use libevm's *ethclient.Client or local interfaces, instead of coreth's ethclient.Client interface
Define local clients when the libevm (aka geth) ethclient is not enough:
- tests/e2e EthClient
- wallet/subnet/primary ethClient since this is used in production and should not be exported, and we should not import tests/e2e from there ideally imo

How this was tested

CI passing + used in the libevm branches for both coreth and subnet-evm

Need to be documented in RELEASES.md?

- Use coreth libevm branch (2025-03-11, ava-labs/coreth@79c693f) - Use libevm v1.13.14-0.2.0.rc.3

- Use `NonceAt` with `nil` block number instead of `AcceptedNonceAt` - Return and use concrete type `*ethclient.Client`

….Client

…hclient.Client

… coreth/ethclient

- Side effect is coreth has to be upgraded as well to support the newer libevm

- See ava-labs/coreth#910

tests/e2e/c/dynamic_fees.go

maru-ava · 2025-05-01T15:20:41Z

wallet/subnet/primary/ethclient.go

+	"github.com/ava-labs/libevm/rpc"
+)
+
+type ethClient struct {


I'm not sure I understand why it would be inappropriate for this to be exported for reuse by the e2e tests. What am I missing?

wallet/subnet/primary is used in various places in avalanchego (notably examples) which should not have access to this implementation detail.
We could modify the tests/fixture/e2e NewEthClient function to take in a ctx context.Context, nodeURI string and import that, but that seems a bit wrong to import "test" packages in there, and might create cycle dependencies.

wallet/subnet/primary is used in various places in avalanchego (notably examples) which should not have access to this implementation detail.

This would appear to be a similar assertion to the one made in the PR description, but I'm still missing the potential harm to making this exportable. 'should not have access' - why not?

Generally it's best to keep symbols unexported as long as they are not needed, to reduce API/documentation surface for the developer using the wallet/subnet/primary package, and also to reduce package inter-dependencies.
On top of this, right now the extension from the libevm/geth eth client is smaller in wallet/subnet/primary than in tests/fixture/e2e. If tests/fixture/e2e imports wallet/subnet/primary for its eth client, we would have to add methods to this client to satisfy the e2e tests. A developer using the wallet/subnet/primary would notice these methods and might wonder why they are here. There might be further divergences in the future between the needs of the 2 packages, making the "seemingly unneeded" methods count grow.

Generally it's best to keep symbols unexported as long as they are not needed, to reduce API/documentation surface for the developer using the wallet/subnet/primary package, and also to reduce package inter-dependencies.

I find this argument entirely unpersuasive. To me, complicating the implementation has an immediate impact on maintainability and I don't see any reason to avoid test code depending on non-test code.

On top of this, right now the extension from the libevm/geth eth client is smaller in wallet/subnet/primary than in tests/fixture/e2e. If tests/fixture/e2e imports wallet/subnet/primary for its eth client, we would have to add methods to this client to satisfy the e2e tests. A developer using the wallet/subnet/primary would notice these methods and might wonder why they are here. There might be further divergences in the future between the needs of the 2 packages, making the "seemingly unneeded" methods count grow.

Except it would be trivial to put the common stuff in the wallet and extend it in e2e.

Copilot

Pull Request Overview

This PR migrates various components from using coreth to libevm by replacing coreth ethclient and related types with libevm's alternatives. Key changes include:

Introducing and using a new BaseFeeEstimator interface in multiple packages.
Replacing direct coreth/ethclient imports with libevm/ethclient and updating related functions.
Updating tests and genesis files to use libevm components.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
wallet/subnet/primary/interfaces.go	Added BaseFeeEstimator interface definition for primary client usage
wallet/subnet/primary/ethclient.go	Introduces a new ethClient implementation using libevm
wallet/subnet/primary/api.go	Updated EthState definition and client initialization to use BaseFeeEstimator
wallet/chain/c/wallet.go	Replaced coreth/ethclient with BaseFeeEstimator in production wallet
wallet/chain/c/interfaces.go	Added BaseFeeEstimator interface for wallet chain client integration
tests/fixture/tmpnet/genesis.go	Updated genesis imports from coreth to libevm
tests/fixture/e2e/helpers.go	Modified NewEthClient and related functions for libevm integration
tests/e2e/c/dynamic_fees.go	Updated params import to use libevm's params
genesis/genesis_test.go	Updated genesis test to import libevm core

Comments suppressed due to low confidence (2)

wallet/subnet/primary/ethclient.go:16

[nitpick] Consider renaming 'ethClient' to 'EthClient' to follow Go's convention for exported vs unexported types or to avoid potential confusion with similar types in other packages.

type ethClient struct {

wallet/chain/c/interfaces.go:11

The BaseFeeEstimator interface is defined in both 'wallet/subnet/primary/interfaces.go' and 'wallet/chain/c/interfaces.go'. Consider consolidating this interface into a shared package to ensure consistency across the codebase.

type BaseFeeEstimator interface {

maru-ava · 2025-05-02T14:57:59Z

wallet/chain/c/interfaces.go

+	"math/big"
+)
+
+type BaseFeeEstimator interface {


As per the comment from chatgpt, why does the identical interface need to be defined twice?

JuanLeon2 · 2025-05-05T15:58:17Z

Does this have any impact on pr 3918?

qdm12 · 2025-05-08T12:13:48Z

This may be closed or significantly reduced, moving this as draft.
We have ava-labs/coreth#943 ongoing which would fix the ethclient issues.

qdm12 · 2025-05-23T09:16:59Z

Unneeded so closing this.

qdm12 added 13 commits March 27, 2025 16:15

Use coreth-libevm and libevm

ab75243

- Use coreth libevm branch (2025-03-11, ava-labs/coreth@79c693f) - Use libevm v1.13.14-0.2.0.rc.3

Import from libevm instead of coreth when possible

873d582

tests/e2e/c: use libevm/ethclient

f0a1c57

- Use `NonceAt` with `nil` block number instead of `AcceptedNonceAt` - Return and use concrete type `*ethclient.Client`

wallet/chain/c: use BaseFeeEstimator interface instead of ethclient…

16036ad

….Client

wallet/subnet/primary: use BaseFeeEstimator interface instead of et…

ce458ba

…hclient.Client

wallet/subnet/primary: define local ethClient to drop dependency on…

ba1c2b3

… coreth/ethclient

Bump libevm to v1.13.14-0.2.0.rc.4

3a6bfac

- Side effect is coreth has to be upgraded as well to support the newer libevm

Bump coreth to have a metric fix

40a5268

- See ava-labs/coreth#910

Fix pointer type for metric in coreth

8076866

Bump coreth from metrics fix PR

07ce7f7

Merge branch 'master' into qdm12/libevm

5f2dca3

Merge branch 'master' into qdm12/libevm

b6cb5a9

Add custom eth client for EstimateBaseFee in c package

efa0661

qdm12 changed the title ~~chore(~~ chore(all): libevm phase 2.5 minor changes needed for coreth and subnet-evm May 1, 2025

qdm12 changed the title ~~chore(all): libevm phase 2.5 minor changes needed for coreth and subnet-evm~~ chore(all): libevm phase 2.5 minor changes needed for subnet-evm May 1, 2025

qdm12 changed the title ~~chore(all): libevm phase 2.5 minor changes needed for subnet-evm~~ chore(all): libevm phase 2.5 changes needed for subnet-evm May 1, 2025

qdm12 added 3 commits May 1, 2025 12:14

Fix copyright headers to pass linter

4d9224f

Single eth client inheriting from libevm eth client

60ddfe8

Fix gci formatting

6906a7f

qdm12 marked this pull request as ready for review May 1, 2025 11:56

qdm12 requested review from maru-ava and StephenButtolph as code owners May 1, 2025 11:56

maru-ava reviewed May 1, 2025

View reviewed changes

AcceptedNonceAt EthClient method

78d8a82

StephenButtolph requested a review from Copilot May 1, 2025 19:41

Copilot AI reviewed May 1, 2025

View reviewed changes

maru-ava reviewed May 2, 2025

View reviewed changes

qdm12 mentioned this pull request May 5, 2025

reduce customtypes import ava-labs/coreth#943

Merged

JuanLeon2 added this to avalanchego May 5, 2025

JuanLeon2 moved this to Ready 🚦 in avalanchego May 5, 2025

JuanLeon2 added the libevm label May 5, 2025

qdm12 marked this pull request as draft May 8, 2025 12:13

qdm12 closed this May 23, 2025

github-project-automation bot moved this from Ready 🚦 to Done 🎉 in avalanchego May 23, 2025

qdm12 deleted the qdm12/libevm branch May 23, 2025 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(all): libevm phase 2.5 changes needed for subnet-evm #3923

chore(all): libevm phase 2.5 changes needed for subnet-evm #3923

Uh oh!

qdm12 commented May 1, 2025 •

edited

Loading

Uh oh!

Uh oh!

maru-ava May 1, 2025

Uh oh!

qdm12 May 1, 2025

Uh oh!

maru-ava May 1, 2025

Uh oh!

qdm12 May 2, 2025 •

edited

Loading

Uh oh!

maru-ava May 2, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

maru-ava May 2, 2025

Uh oh!

JuanLeon2 commented May 5, 2025

Uh oh!

qdm12 commented May 8, 2025

Uh oh!

qdm12 commented May 23, 2025

Uh oh!

Uh oh!

chore(all): libevm phase 2.5 changes needed for subnet-evm #3923

chore(all): libevm phase 2.5 changes needed for subnet-evm #3923

Uh oh!

Conversation

qdm12 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

Uh oh!

maru-ava May 1, 2025

Choose a reason for hiding this comment

Uh oh!

qdm12 May 1, 2025

Choose a reason for hiding this comment

Uh oh!

maru-ava May 1, 2025

Choose a reason for hiding this comment

Uh oh!

qdm12 May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maru-ava May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

maru-ava May 2, 2025

Choose a reason for hiding this comment

Uh oh!

JuanLeon2 commented May 5, 2025

Uh oh!

qdm12 commented May 8, 2025

Uh oh!

qdm12 commented May 23, 2025

Uh oh!

Uh oh!

qdm12 commented May 1, 2025 •

edited

Loading

qdm12 May 2, 2025 •

edited

Loading

maru-ava May 2, 2025 •

edited

Loading