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

support multiple chain evm connection for TXM gas estimator allowing L1 oracle fee fetching #14781

Merged
merged 47 commits into from
Nov 4, 2024

Conversation

huangzhen1997
Copy link
Contributor

@huangzhen1997 huangzhen1997 commented Oct 15, 2024

Description

CCIP wants to implement heuristic-based l1 oracles, that has access to ethClient for a number of chains in addition to the target chain that the TXM is currently pointing at. This PR allows toml config to nodes config for multiple chains, and a map with lazy initialization will be passed down to txm gas estimator, which will be referenced by L1 Oracle component.

A new flag is added to [EVM.GasEstimator.DAOracle] called L1ChainID, by default it's "0" indicating the L1 oracle client is disabled.

Reference:

https://smartcontract-it.atlassian.net/browse/BCFR-911

Copy link
Contributor

github-actions bot commented Oct 15, 2024

Below is an analysis created by an LLM. Be mindful of hallucinations and verify accuracy.

WF: CI Core#a5a57f5

1. HTTP 503 Service Temporarily Unavailable error during push request:[Flakey Test Detection]

Source of Error:
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0741381Z 2024/10/15 22:50:59 Error re-running flakey tests: push request failed: status=503, body=<html>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0743612Z <head><title>503 Service Temporarily Unavailable</title></head>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0744832Z <body>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0745887Z <center><h1>503 Service Temporarily Unavailable</h1></center>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0746732Z <hr><center>nginx</center>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0747080Z </body>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0747317Z </html>
Flakey Test Detection	Re-run tests	2024-10-15T22:50:59.0769430Z ##[error]Process completed with exit code 1.

Why: The error occurred because the server handling the push request was temporarily unavailable, likely due to maintenance or overload, indicated by the HTTP 503 status code.

Suggested fix: Retry the request after a brief wait or during off-peak hours. Implement retry logic with exponential backoff to handle such temporary issues automatically.

@@ -54,7 +55,7 @@ type feeEstimatorClient interface {
}

// NewEstimator returns the estimator for a given config
func NewEstimator(lggr logger.Logger, ethClient feeEstimatorClient, chaintype chaintype.ChainType, geCfg evmconfig.GasEstimator) (EvmFeeEstimator, error) {
func NewEstimator(lggr logger.Logger, ethClient feeEstimatorClient, chaintype chaintype.ChainType, geCfg evmconfig.GasEstimator, getChainClientByID func(id string) (evmclient.Client, error)) (EvmFeeEstimator, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why pass a function instead of the actual map?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel that returning the evmclient.Client interface creates a huge dependency on the estimator's side. Don't we already know the method we will use from the Client? Can't we declare a much smaller interface in its place?

Copy link
Contributor

github-actions bot commented Oct 16, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@huangzhen1997 huangzhen1997 marked this pull request as ready for review October 16, 2024 17:35
@huangzhen1997 huangzhen1997 requested review from a team as code owners October 16, 2024 17:35
if err2 != nil {
err = multierr.Combine(err, fmt.Errorf("failed to create chain %s: %w", cid, err2))
continue
}

chainIDToClientMap[cid] = chain.Client()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but for LOOPP support we will have to pass relayers instead of local clients. Just something to keep in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

We won't be able to support the ethereum.FeeHistory type there, for example, but we can sort that out later since it looks like a simple/generic type anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's LOOPP support ?

Copy link
Contributor

@jmank88 jmank88 Oct 16, 2024

Choose a reason for hiding this comment

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

LOOP Plugins. The chain-agnostic "relayer" API that sits in front of each chain family implementation. We'll have to update this for the EVM LOOP Plugin.

https://pkg.go.dev/github.com/smartcontractkit/[email protected]/pkg/types#Relayer

@huangzhen1997 huangzhen1997 requested a review from a team as a code owner October 30, 2024 22:29
if daOracle != nil {
if clientsByChainID == nil {
lggr.Debugf("clientsByChainID map is missing")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@makramkd makramkd left a comment

Choose a reason for hiding this comment

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

core/capabilities/ccip changes seem good

@dimriou dimriou added this pull request to the merge queue Nov 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2024
@dimriou dimriou added this pull request to the merge queue Nov 4, 2024
Merged via the queue into develop with commit bfd9a3f Nov 4, 2024
160 checks passed
@dimriou dimriou deleted the BCFR-911/add-L1-eth-client-support-for-gas-estimator branch November 4, 2024 17:38
cedric-cordenier pushed a commit that referenced this pull request Nov 5, 2024
…L1 oracle fee fetching (#14781)

* add lazy initialization to evm client map and pass it to gas estimator for l1 oracle

* remove unrelated

* rename and fix lint

* add changeset

* modify err msg

* address comments

* go import lint

* address comments

* add default value of L1ChainID for the DAOracle config

* fix doc

* update config

* fix testdata

* update

* fix make

* update doc

* update

* fix test

* update changeset

* update changeset

* address comments

* rename, need to fix test

* fix test, part 1

* rebuild doc

* fix test

* fix test

* update comment

* update types to use pointers

* fix test

* fix

* fix test

* fix make

* fix make

* fix make

* fix test

* refactor function name

* address comments

* update msg

* update test helper

* fix test

* rename and remove fallback

* update changeset

* update docs

* remove unused
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.

9 participants