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.

Sign up for GitHub

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

Jump to bottom

Set From address for gas limit estimation feature #14246

Merged
merged 4 commits into from
Aug 27, 2024
Merged

Set From address for gas limit estimation feature #14246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40db6fd
Set from address for gas limit estimation
amit-momin Aug 27, 2024
41ade0f
Merge branch 'develop' into fix/set-from-address-gas-limit-estimation
amit-momin Aug 27, 2024
ef4e7a6
Updated tests
amit-momin Aug 27, 2024
5f4dd22
Added changeset
amit-momin Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/great-timers-agree.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

Updated gas limit estimation feature to set From address #internal
70 changes: 36 additions & 34 deletions core/chains/evm/gas/mocks/evm_fee_estimator.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 10 additions & 7 deletions core/chains/evm/gas/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ type EvmFeeEstimator interface {

// L1Oracle returns the L1 gas price oracle only if the chain has one, e.g. OP stack L2s and Arbitrum.
L1Oracle() rollups.L1Oracle
GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, toAddress *common.Address, opts ...feetypes.Opt) (fee EvmFee, estimatedFeeLimit uint64, err error)
GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, fromAddress, toAddress *common.Address, opts ...feetypes.Opt) (fee EvmFee, estimatedFeeLimit uint64, err error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a big fan of adding more and more fields in the signature. At some point, it might make sense to just pass the transaction and access the fields internally, but I guess it's ok for now.

Copy link
Contributor Author

@amit-momin amit-momin Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ya I agree. I even considered splitting gas price estimation from gas limit estimation and have a separate method for limits. Externally, it seems for the most part products and CW only care about the gas price so we could remove calldata, feeLimit, fromAddress, and toAddress. But think we're bound by Arbitrum's estimator for now so maybe it's something we can cleanup in the future

BumpFee(ctx context.Context, originalFee EvmFee, feeLimit uint64, maxFeePrice *assets.Wei, attempts []EvmPriorAttempt) (bumpedFee EvmFee, chainSpecificFeeLimit uint64, err error)

// GetMaxCost returns the total value = max price x fee units + transferred value
GetMaxCost(ctx context.Context, amount assets.Eth, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, toAddress *common.Address, opts ...feetypes.Opt) (*big.Int, error)
GetMaxCost(ctx context.Context, amount assets.Eth, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, fromAddress, toAddress *common.Address, opts ...feetypes.Opt) (*big.Int, error)
}

type feeEstimatorClient interface {
Expand Down Expand Up @@ -270,7 +270,7 @@ func (e *evmFeeEstimator) L1Oracle() rollups.L1Oracle {

// GetFee returns an initial estimated gas price and gas limit for a transaction
// The gas limit provided by the caller can be adjusted by gas estimation or for 2D fees
func (e *evmFeeEstimator) GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, toAddress *common.Address, opts ...feetypes.Opt) (fee EvmFee, estimatedFeeLimit uint64, err error) {
func (e *evmFeeEstimator) GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, fromAddress, toAddress *common.Address, opts ...feetypes.Opt) (fee EvmFee, estimatedFeeLimit uint64, err error) {
var chainSpecificFeeLimit uint64
// get dynamic fee
if e.EIP1559Enabled {
Expand All @@ -290,12 +290,12 @@ func (e *evmFeeEstimator) GetFee(ctx context.Context, calldata []byte, feeLimit
}
}

estimatedFeeLimit, err = e.estimateFeeLimit(ctx, chainSpecificFeeLimit, calldata, toAddress)
estimatedFeeLimit, err = e.estimateFeeLimit(ctx, chainSpecificFeeLimit, calldata, fromAddress, toAddress)
return
}

func (e *evmFeeEstimator) GetMaxCost(ctx context.Context, amount assets.Eth, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, toAddress *common.Address, opts ...feetypes.Opt) (*big.Int, error) {
fees, gasLimit, err := e.GetFee(ctx, calldata, feeLimit, maxFeePrice, toAddress, opts...)
func (e *evmFeeEstimator) GetMaxCost(ctx context.Context, amount assets.Eth, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, fromAddress, toAddress *common.Address, opts ...feetypes.Opt) (*big.Int, error) {
fees, gasLimit, err := e.GetFee(ctx, calldata, feeLimit, maxFeePrice, fromAddress, toAddress, opts...)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -346,7 +346,7 @@ func (e *evmFeeEstimator) BumpFee(ctx context.Context, originalFee EvmFee, feeLi
return
}

func (e *evmFeeEstimator) estimateFeeLimit(ctx context.Context, feeLimit uint64, calldata []byte, toAddress *common.Address) (estimatedFeeLimit uint64, err error) {
func (e *evmFeeEstimator) estimateFeeLimit(ctx context.Context, feeLimit uint64, calldata []byte, fromAddress, toAddress *common.Address) (estimatedFeeLimit uint64, err error) {
// Use the feeLimit * LimitMultiplier as the provided gas limit since this multiplier is applied on top of the caller specified gas limit
providedGasLimit, err := commonfee.ApplyMultiplier(feeLimit, e.geCfg.LimitMultiplier())
if err != nil {
Expand All @@ -362,6 +362,9 @@ func (e *evmFeeEstimator) estimateFeeLimit(ctx context.Context, feeLimit uint64,
To: toAddress,
Data: calldata,
}
if fromAddress != nil {
Copy link
Collaborator

@dimriou dimriou Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't fromAddress be treated the same way we treat toAddress ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't because of callMsg's typing

type CallMsg struct {
	From  common.Address
	To    *common.Address
}

callMsg.From = *fromAddress
}
estimatedGas, estimateErr := e.ethClient.EstimateGas(ctx, callMsg)
if estimateErr != nil {
if providedGasLimit > 0 {
Expand Down
Loading
Loading