generalized fee estimation interface #8561

aalu1418 · 2023-02-24T21:22:54Z

goal: all fee estimation logic should be handled by the estimator rather than the txm/broadcaster/confirmer logic

move legacy + dynamic fee estimation logic into fee estimator
fix test cases
rename Estimator interface => EvmEstimator
condense gas estimation logic flows in txm
add test cases for wrapping evm estimator logic
generically typed interface for fee estimator

Associated tickets:
https://smartcontract-it.atlassian.net/browse/BCI-888
https://smartcontract-it.atlassian.net/browse/BCI-873

github-actions · 2023-02-24T21:33:59Z

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

augustbleeds · 2023-03-01T16:48:25Z

Can you link the ticket in the PR description?

core/chains/evm/gas/models.go

core/chains/evm/txmgr/eth_broadcaster.go

prashantkumar1982 · 2023-03-02T04:45:17Z

Can you link the ticket in the PR description?

Done. Added in the main comment

core/chains/evm/gas/models.go

core/chains/evm/gas/models_test.go

prashantkumar1982 · 2023-03-03T17:54:49Z

core/chains/evm/gas/models.go

-type PriorAttempt interface {
-	GetGasPrice() *assets.Wei
-	DynamicFee() DynamicFee
+type PriorAttempt[FEE any, HASH any] interface {


Are all the below functions for PriorAttempt needed by generic components?
I see most are only needed by the EVM gas estimators.

Could we try to expose here only the minimum set of fields?

The PriorAttempt is kind of interesting since all of the methods defined in that interface are only used by gas estimators. The TXM directly interacts with the TxAttempt struct underneath and provides these interfaces for the estimators to access the data. It seems like the opposite to the headtracker where the headtracker service defines the head struct/object and the txm + estimators are consumers vs the estimators are consumers of the tx object the txm creates

I think as I'm working on the TxAttemptBuilder I will have to look at this part again since we might be able to remove that interface abstraction and use a generically typed object...either way the user shouldn't need to define functionality for PriorAttempt

as for a minimum set of fields, I think it's already pretty minimal with returning the fee, tx type, tx hash, fee limit, block number restrictions

Sounds good. Let us look at it while doing the TxAttemtBuilder.

core/chains/evm/txmgr/eth_confirmer.go

core/chains/evm/txmgr/eth_broadcaster.go

core/chains/evm/txmgr/eth_broadcaster_test.go

…ck fixes

core/chains/evm/txmgr/eth_confirmer.go

prashantkumar1982 · 2023-03-07T20:59:04Z

core/chains/evm/txmgr/eth_broadcaster.go

 	if err != nil {
 		return errors.Wrap(err, "tryAgainWithNewEstimation failed to estimate gas"), true
 	}
 	lgr.Warnw("L2 rejected transaction due to incorrect fee, re-estimated and will try again",
 		"etxID", etx.ID, "err", err, "newGasPrice", gasPrice, "newGasLimit", gasLimit)
-	return eb.tryAgainWithNewLegacyGas(ctx, lgr, etx, attempt, initialBroadcastAt, gasPrice, gasLimit)
+
+	// TODO: nil handling for gasPrice.Legacy? will this ever be reached where EIP1559 is enabled on a L2 but a legacy tx?


I think the TxType is already decided when we initialize a chain, so this shouldn't ever happen.
We should actually extract out legacy/dynamic fees from here eventually, and even in EVM specific gas estimators, they can be instantiated once, with config option telling whether they're on legacy fees, or dynamic.

the scenario I was thinking of is if we are already running on a chain and it enables EIP1559 fees, and we still have legacy txs in the DB - is that possible?

also this abstraction will be done in the TxBuilder since it has to do with assembling the tx 😄

Yes, good point about DB containing prior legacy attempts.
We could ask Node operators at the time of upgrade, to clear pending txes and attempts, but that is not ideal.

prashantkumar1982 · 2023-03-08T02:18:40Z

common/txmgr/types/fee_estimator.go

+)
+
+// PriorAttempt provides a generic interface for reading tx data to be used in the fee esimators
+type PriorAttempt[FEE any, HASH any] interface {


Btw, I don't think there is any reason for the common TxMgr components to use PriorAttempt interface.
It already doesn't use it, except to pass it to Estimator.BumpGas().
I believe we could always just pass in previous Attempts in the form of a generic TxAttempt. And the chain specific code should know what to extract from it, to bump gas correctly.

Not for this PR, but for later, let us aim to completely do away with this interface, and just reuse TxAttempt.

If you want, create a separate subtask or ticket for this, for later.

👍🏼 I will take a look at this part when I'm working with the TxBuilder

prashantkumar1982 · 2023-03-08T02:40:13Z

common/txmgr/types/fee_estimator.go

+// FeeEstimator provides a generic interface for fee estimation
+//
+//go:generate mockery --quiet --name FeeEstimator --output ./mocks/ --case=underscore
+type FeeEstimator[HEAD any, FEE any, MAXPRICE any, HASH any] interface {


The MAXPRICE actually is also never needed to be passed in to estimator by the generic txmgr.
The Txmgr just reads a static config, and passes it in. Ideally it should be fetched directly by the estimator.

I would say, create a sub-task or ticket, to remove it from generic interface here later.

How would it be fetched directly?

This is used on a job-specific basis

I would agree with Sam, I think it's still needed as it's a different config variable then the global chain configs and can be different for each tx

This value is a keySpecfic config currently. GasEstimator can directly fetch that from config, if it has the Key available..

Here is a place where TxMgr currently fetches it:

chainlink/core/chains/evm/txmgr/eth_confirmer.go

Line 769 in 6b4bcc5

keySpecificMaxGasPriceWei := ec.config.KeySpecificMaxGasPriceWei(etx.FromAddress)

Doing this just makes it clear that the interface between coreTxm <--> GasEstimator doesn't require this value to be passed in.
This value is a static config defined for each Key.

but then each gas estimator would need knowledge of the FromAddress for each key. it seems simpler to just pass gas parameters instead of having the estimators have knowledge of additional configs + addresses

I think this should be passed in. It is used in keepers I think, specifically (or used to be)

prashantkumar1982

For now, changes look good.
The pending things, we can tackle later, as doing it in 1 PR will just bloat this one.

samsondav · 2023-03-08T19:12:51Z

common/txmgr/types/fee_estimator.go

+// FeeEstimator provides a generic interface for fee estimation
+//
+//go:generate mockery --quiet --name FeeEstimator --output ./mocks/ --case=underscore
+type FeeEstimator[HEAD any, FEE any, MAXPRICE any, HASH any] interface {


I think this should be passed in. It is used in keepers I think, specifically (or used to be)

cl-sonarqube-production · 2023-03-08T19:26:18Z

SonarQube Quality Gate

80.6% Coverage on New Code (is less than 90%)

See analysis details on SonarQube

aalu1418 force-pushed the generalized-fee-estimation-interface branch from 66d6f7f to 625c44f Compare February 24, 2023 21:33

aalu1418 added 2 commits February 27, 2023 15:53

combine EVM legacy + dynamic fee estimation behind a single interface

5edd3ba

fix components that use txm estimator + regenerate mocks

dd078be

aalu1418 force-pushed the generalized-fee-estimation-interface branch from 1a16b68 to 1a7ed11 Compare February 27, 2023 22:54

fix tests

68f7dad

aalu1418 force-pushed the generalized-fee-estimation-interface branch from 0b7a3f3 to 68f7dad Compare February 27, 2023 23:11

aalu1418 added 4 commits February 28, 2023 10:44

old Estimator => EvmEstimator

ce243aa

condense gas estimation logic in broadcaster and confirmer

e543b43

test coverage for WrappedEvmEstimator

e32bfec

generic FeeEstimator interface

69ce9a2

generic PriorAttempt interface

afc7859

prashantkumar1982 reviewed Mar 1, 2023

View reviewed changes

core/chains/evm/gas/models.go Outdated Show resolved Hide resolved

prashantkumar1982 reviewed Mar 1, 2023

View reviewed changes

core/chains/evm/txmgr/eth_broadcaster.go Show resolved Hide resolved

aalu1418 marked this pull request as ready for review March 1, 2023 22:39

aalu1418 requested review from jkongie, a team, samsondav and jmank88 as code owners March 1, 2023 22:39

augustbleeds reviewed Mar 3, 2023

View reviewed changes

core/chains/evm/gas/models.go Outdated Show resolved Hide resolved

augustbleeds reviewed Mar 3, 2023

View reviewed changes

core/chains/evm/gas/models_test.go Show resolved Hide resolved

augustbleeds previously approved these changes Mar 3, 2023

View reviewed changes

prashantkumar1982 reviewed Mar 3, 2023

View reviewed changes

core/chains/evm/txmgr/eth_confirmer.go Outdated Show resolved Hide resolved

jmank88 reviewed Mar 7, 2023

View reviewed changes

core/chains/evm/txmgr/eth_broadcaster.go Outdated Show resolved Hide resolved

jmank88 reviewed Mar 7, 2023

View reviewed changes

core/chains/evm/txmgr/eth_broadcaster_test.go Outdated Show resolved Hide resolved

move generic fee estimator types to common/txmgr/types + small feedba…

94c99d8

…ck fixes

aalu1418 dismissed augustbleeds’s stale review via 94c99d8 March 7, 2023 16:10

aalu1418 requested a review from a team as a code owner March 7, 2023 16:10

Merge branch 'develop' into generalized-fee-estimation-interface

839ee60

aalu1418 requested review from jmank88, augustbleeds and prashantkumar1982 March 7, 2023 17:09

jmank88 reviewed Mar 7, 2023

View reviewed changes

core/chains/evm/txmgr/eth_confirmer.go Outdated Show resolved Hide resolved

aalu1418 added 2 commits March 7, 2023 11:00

remove duplicate geth/common import

21043ff

Merge branch 'develop' into generalized-fee-estimation-interface

24e105d

prashantkumar1982 reviewed Mar 7, 2023

View reviewed changes

prashantkumar1982 reviewed Mar 8, 2023

View reviewed changes

prashantkumar1982 approved these changes Mar 8, 2023

View reviewed changes

Merge branch 'develop' into generalized-fee-estimation-interface

2f69803

aalu1418 requested a review from jmank88 March 8, 2023 15:46

samsondav approved these changes Mar 8, 2023

View reviewed changes

Merge branch 'develop' into generalized-fee-estimation-interface

ced4a24

prashantkumar1982 merged commit c90fc0e into develop Mar 8, 2023

prashantkumar1982 deleted the generalized-fee-estimation-interface branch March 8, 2023 19:45

chainchad mentioned this pull request Mar 16, 2023

Resolve v1.13.0 merge conflicts with develop #8736

Merged

chainchad mentioned this pull request Apr 21, 2023

release/2.0.0 -> master #9024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generalized fee estimation interface #8561

generalized fee estimation interface #8561

aalu1418 commented Feb 24, 2023 •

edited by prashantkumar1982

Loading

github-actions bot commented Feb 24, 2023

augustbleeds commented Mar 1, 2023

prashantkumar1982 commented Mar 2, 2023

prashantkumar1982 Mar 3, 2023

aalu1418 Mar 7, 2023

prashantkumar1982 Mar 7, 2023

prashantkumar1982 Mar 7, 2023

aalu1418 Mar 7, 2023

aalu1418 Mar 7, 2023

prashantkumar1982 Mar 7, 2023

prashantkumar1982 Mar 8, 2023

aalu1418 Mar 8, 2023

prashantkumar1982 Mar 8, 2023

samsondav Mar 8, 2023

samsondav Mar 8, 2023

aalu1418 Mar 8, 2023

prashantkumar1982 Mar 8, 2023

aalu1418 Mar 8, 2023

samsondav Mar 8, 2023

prashantkumar1982 left a comment

samsondav Mar 8, 2023

cl-sonarqube-production bot commented Mar 8, 2023

generalized fee estimation interface #8561

generalized fee estimation interface #8561

Conversation

aalu1418 commented Feb 24, 2023 • edited by prashantkumar1982 Loading

github-actions bot commented Feb 24, 2023

augustbleeds commented Mar 1, 2023

prashantkumar1982 commented Mar 2, 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

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

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Mar 8, 2023

aalu1418 commented Feb 24, 2023 •

edited by prashantkumar1982

Loading