EvmFeeEstimator return Optimistic Rollup's L1BaseFee #10557

matYang · 2023-09-08T06:18:13Z

In CCIP, when user sends a message from Chain A -> Chain B, the OnRamp contract on Chain A needs to calculate the tx cost on Chain B in order to charge user such cost.

This requires OnRamp on Chain A to know Chain B's tx price components. When Chain B is an optimistic rollup, Chain A needs to know Chain B's l1GasPrice, in addition to regular gasPrice. This PR proposes to extend EvmFeeEstimator to return l1GasPrice, by adding a L1Oracle service into WrappedEvmEstimator

Inside CCIP, we can consume it as

if l1Oracle := r.config.sourceFeeEstimator.L1Oracle(); l1Oracle != nil {
   l1gasPrice = l1Oracle.L1GasPrice()
}

github-actions · 2023-09-08T06:18:32Z

I see that you haven't updated any README files. Would it make sense to do so?

github-actions · 2023-09-08T06:18:49Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6118401967

github-actions · 2023-09-10T03:55:08Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6134940125

github-actions · 2023-09-10T04:08:31Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6134995445

github-actions · 2023-09-10T04:25:29Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6135066158

github-actions · 2023-09-10T04:37:42Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6135119080

dimriou · 2023-09-13T11:10:13Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+	case config.ChainArbitrum:
+		address = ArbGasInfoAddress
+		selector = ArbGasInfo_getL1BaseFeeEstimate
+	case config.ChainOptimismBedrock:


Since Base also uses OptimismBedorck chaintype, can we confirm the address and the hex code are the same?

Yup, Base uses the same precompiles as Optimism: https://basescan.org/address/0x420000000000000000000000000000000000000F#readProxyContract

prashantkumar1982 · 2023-09-14T08:00:19Z

core/chains/evm/gas/fixed_price_estimator.go

@@ -4,6 +4,7 @@ import (
 	"context"

 	"github.com/pkg/errors"
+


nit: revert this change, and keep this file untouched

prashantkumar1982 · 2023-09-14T08:16:19Z

core/chains/evm/gas/models.go

 	}
 }

-func (e WrappedEvmEstimator) GetFee(ctx context.Context, calldata []byte, feeLimit uint32, maxFeePrice *assets.Wei, opts ...feetypes.Opt) (fee EvmFee, chainSpecificFeeLimit uint32, err error) {
+func NewWrappedEvmEstimatorWithL1Oracle(e EvmEstimator, eip1559Enabled bool, o chainoracles.L1Oracle) EvmFeeEstimator {


No need to create this new function.
Just add the L1Oracle param to the existing NewWrappedEvmEstimator().
For chains that don't support the L1Oracle, just pass nil there.

prashantkumar1982 · 2023-09-14T08:17:35Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+		address = OPGasOracleAddress
+		selector = OPGasOracle_l1BaseFee
+	default:
+		return nil


Since you are calling this NewL1GasPriceOracle() only for chains that support it, the default case here should just call panic().
This will help surface up a misconfig or error faster.

prashantkumar1982 · 2023-09-14T08:37:29Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+	defer close(o.chDone)
+
+	t := o.refresh()
+	close(o.chInitialised)


Instead of from here, close this from within refresh(), once we successfully are able to set l1GasPrice there.
This way, this component is in started mode only after it fetches a valid l1GasPrice.

The Start() call is blocked on the chInitialised channel, so we need to close it regardless of fetching prices (refresh()) was successful or not right, closing it once here makes sense to me. This pattern is same in L2Suggested and Arbitrum estimators.

prashantkumar1982 · 2023-09-14T08:39:05Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+	}
+
+	if len(b) != 32 { // returns uint256;
+		o.logger.Warnf("return data length (%d) different than expected (%d)", len(b), 32)


This should be a critical error. It indicates the response format has changed.

prashantkumar1982 · 2023-09-14T08:39:18Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+		Data: common.Hex2Bytes(o.selector),
+	}, big.NewInt(-1))
+	if err != nil {
+		return


Add an error log here too

prashantkumar1982 · 2023-09-14T08:41:51Z

core/chains/evm/gas/models.go

+		errOracle = e.l1Oracle.Ready()
+	}
+
+	if errEVM != nil {


This block of code can happen before even calling l1Oracle.Ready()

it'd only execute either when e.l1Oracle is nil, or e.l1Oracle.Ready() has been called. The drawback is if EvmEstimator and l1Oracle both error, only errEVM will be surfaced. I think that's fine since it doesn't alter the outcome and EvmEstimator is the more critical component.

dimriou · 2023-09-14T12:40:52Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+	b, err := o.client.CallContract(ctx, ethereum.CallMsg{
+		To:   &precompile,
+		Data: common.Hex2Bytes(o.selector),
+	}, big.NewInt(-1))


This should be nil instead. Not sure why on Arbitrum Estimator is also -1

dimriou · 2023-09-14T12:44:29Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+		pollPeriod:    PollPeriod,
+		logger:        lggr.Named(fmt.Sprintf("L1GasPriceOracle(%s)", chainType)),
+		address:       address,
+		selector:      selector,


How about call instead, since this is a hex-encoded call? Selector doesn't provide much context.

prashantkumar1982 · 2023-09-15T09:25:37Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

@@ -0,0 +1,176 @@
+package chainoracles


nit: Just wondering, if this name is appropriate.
This seems to be a package for Rollups.
Could rollups be a better name?

prashantkumar1982 · 2023-09-15T09:29:14Z

core/chains/evm/gas/chainoracles/l1_gas_price_oracle.go

+
+var supportedChainTypes = []config.ChainType{config.ChainArbitrum, config.ChainOptimismBedrock}
+
+func IsL1OracleChain(chainType config.ChainType) bool {


This name can confuse someone into thinking if this chain itself is an L1 chain.
Is this a better name: IsRollupWithL1Support.

prashantkumar1982 · 2023-09-15T09:31:19Z

core/chains/evm/gas/chainoracles/models.go

+// For example, on Optimistic Rollups, this oracle can return rollup-specific l1BaseFee
+//
+//go:generate mockery --quiet --name L1Oracle --output ./mocks/ --case=underscore
+type L1Oracle interface {


What about renaming to UnderlyingL1.

I would prefer L1Oracle since it is not actually calling the underlying L1, it is fetching from the L1 oracle contract provided by the L2

prashantkumar1982 · 2023-09-15T09:31:56Z

core/chains/evm/gas/chainoracles/models.go

+type L1Oracle interface {
+	services.ServiceCtx
+
+	L1GasPrice(ctx context.Context) (*assets.Wei, error)


Since the interface itself is named L1..., this function doesn't need to repeat it. Just say GasPrice()

prashantkumar1982

Looks pretty good. Just asked a few question on the right namings.

dimriou

I agree with Prashant on the names. Other than that LGTM

cl-sonarqube-production · 2023-09-15T23:00:57Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

84.3% Coverage
0.0% Duplication

prashantkumar1982

Thanks a lot!

matYang added 2 commits September 8, 2023 02:01

Draft impl

af3e893

update configs

33af306

matYang added the do not merge label Sep 8, 2023

move l1 knowledge from EVMEstimator to WrappedEVMFeeEstimator

e5f5aff

nit fixes

d6e8274

matYang requested review from dimriou and connorwstein September 10, 2023 04:09

matYang changed the title ~~Evm estimator return gas price~~ EvmFeeEstimator return gas price components Sep 10, 2023

matYang changed the title ~~EvmFeeEstimator return gas price components~~ EvmFeeEstimator return Optimistic Rollup's L1BaseFee Sep 10, 2023

change L1BaseFee to L1GasPrice

d5ab581

update mock

56efe1d

matYang marked this pull request as ready for review September 10, 2023 04:37

matYang requested review from samsondav and prashantkumar1982 as code owners September 10, 2023 04:37

matYang added 2 commits September 13, 2023 00:31

use chain type instead of modifying chain config

41f5bd4

add mocks

055aeb1

dimriou requested a review from aalu1418 September 13, 2023 11:04

dimriou reviewed Sep 13, 2023

View reviewed changes

matYang added 2 commits September 13, 2023 18:05

beef up models tests

20272a4

beef up l1 oracle tests

e5d1295

matYang requested a review from dimriou September 14, 2023 06:08

small nits

008a455

prashantkumar1982 reviewed Sep 14, 2023

View reviewed changes

dimriou reviewed Sep 14, 2023

View reviewed changes

matYang removed the do not merge label Sep 14, 2023

matYang added 2 commits September 14, 2023 16:58

address comments

19282f2

Merge branch 'develop' into evm-estimator-return-gas-price

7074410

matYang requested a review from jmank88 as a code owner September 14, 2023 21:08

matYang requested review from dimriou and prashantkumar1982 September 14, 2023 21:31

fix chaintype panic

06b61ba

prashantkumar1982 reviewed Sep 15, 2023

View reviewed changes

dimriou previously approved these changes Sep 15, 2023

View reviewed changes

address comments

42c5a00

matYang dismissed dimriou’s stale review via 42c5a00 September 15, 2023 22:45

prashantkumar1982 approved these changes Sep 16, 2023

View reviewed changes

matYang added this pull request to the merge queue Sep 16, 2023

Merged via the queue into develop with commit f6256c3 Sep 16, 2023

matYang deleted the evm-estimator-return-gas-price branch September 16, 2023 16:38

snehaagni mentioned this pull request Oct 18, 2023

release/2.6.0 -> master #10997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EvmFeeEstimator return Optimistic Rollup's L1BaseFee #10557

EvmFeeEstimator return Optimistic Rollup's L1BaseFee #10557

matYang commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

dimriou Sep 13, 2023

matYang Sep 14, 2023

prashantkumar1982 Sep 14, 2023

prashantkumar1982 Sep 14, 2023

prashantkumar1982 Sep 14, 2023

prashantkumar1982 Sep 14, 2023

matYang Sep 14, 2023 •

edited

Loading

prashantkumar1982 Sep 14, 2023

prashantkumar1982 Sep 14, 2023

prashantkumar1982 Sep 14, 2023

matYang Sep 14, 2023

dimriou Sep 14, 2023

dimriou Sep 14, 2023

prashantkumar1982 Sep 15, 2023

prashantkumar1982 Sep 15, 2023

prashantkumar1982 Sep 15, 2023

matYang Sep 15, 2023

prashantkumar1982 Sep 15, 2023

prashantkumar1982 left a comment

dimriou left a comment

cl-sonarqube-production bot commented Sep 15, 2023

prashantkumar1982 left a comment


		var supportedChainTypes = []config.ChainType{config.ChainArbitrum, config.ChainOptimismBedrock}

		func IsL1OracleChain(chainType config.ChainType) bool {

EvmFeeEstimator return Optimistic Rollup's L1BaseFee #10557

EvmFeeEstimator return Optimistic Rollup's L1BaseFee #10557

Conversation

matYang commented Sep 8, 2023 • edited Loading

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matYang Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantkumar1982 left a comment

Choose a reason for hiding this comment

dimriou left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Sep 15, 2023

prashantkumar1982 left a comment

Choose a reason for hiding this comment

matYang commented Sep 8, 2023 •

edited

Loading

matYang Sep 14, 2023 •

edited

Loading