Add bump legacy gas implementation to the Suggested Price Estimator #11805
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package gas | |
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "slices" | ||
| "sync" | ||
| "time" | ||
|
|
@@ -12,7 +13,9 @@ import ( | |
| "github.com/smartcontractkit/chainlink-common/pkg/logger" | ||
| "github.com/smartcontractkit/chainlink-common/pkg/services" | ||
| "github.com/smartcontractkit/chainlink-common/pkg/utils" | ||
| bigmath "github.com/smartcontractkit/chainlink-common/pkg/utils/big_math" | ||
|
|
||
| "github.com/smartcontractkit/chainlink/v2/common/fee" | ||
| feetypes "github.com/smartcontractkit/chainlink/v2/common/fee/types" | ||
| "github.com/smartcontractkit/chainlink/v2/core/chains/evm/assets" | ||
| evmclient "github.com/smartcontractkit/chainlink/v2/core/chains/evm/client" | ||
|
|
@@ -23,6 +26,11 @@ var ( | |
| _ EvmEstimator = &SuggestedPriceEstimator{} | ||
| ) | ||
|
|
||
| type suggestedPriceConfig interface { | ||
| BumpPercent() uint16 | ||
| BumpMin() *assets.Wei | ||
| } | ||
|
|
||
| //go:generate mockery --quiet --name rpcClient --output ./mocks/ --case=underscore --structname RPCClient | ||
| type rpcClient interface { | ||
| CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error | ||
|
|
@@ -32,6 +40,7 @@ type rpcClient interface { | |
| type SuggestedPriceEstimator struct { | ||
| services.StateMachine | ||
|
|
||
| cfg suggestedPriceConfig | ||
| client rpcClient | ||
| pollPeriod time.Duration | ||
| logger logger.Logger | ||
|
|
@@ -46,11 +55,12 @@ type SuggestedPriceEstimator struct { | |
| } | ||
|
|
||
| // NewSuggestedPriceEstimator returns a new Estimator which uses the suggested gas price. | ||
| func NewSuggestedPriceEstimator(lggr logger.Logger, client rpcClient, cfg suggestedPriceConfig) EvmEstimator { | ||
| return &SuggestedPriceEstimator{ | ||
| client: client, | ||
| pollPeriod: 10 * time.Second, | ||
| logger: logger.Named(lggr, "SuggestedPriceEstimator"), | ||
| cfg: cfg, | ||
| chForceRefetch: make(chan (chan struct{})), | ||
| chInitialised: make(chan struct{}), | ||
| chStop: make(chan struct{}), | ||
|
|
@@ -122,42 +132,43 @@ func (o *SuggestedPriceEstimator) refreshPrice() (t *time.Timer) { | |
| return | ||
| } | ||
|
|
||
| // Uses the force refetch chan to trigger a price update and blocks until complete | ||
| func (o *SuggestedPriceEstimator) forceRefresh(ctx context.Context) (err error) { | ||
| ch := make(chan struct{}) | ||
| select { | ||
| case o.chForceRefetch <- ch: | ||
| case <-o.chStop: | ||
| return errors.New("estimator stopped") | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
| select { | ||
| case <-ch: | ||
| case <-o.chStop: | ||
| return errors.New("estimator stopped") | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func (o *SuggestedPriceEstimator) OnNewLongestChain(context.Context, *evmtypes.Head) {} | ||
|
|
||
| func (*SuggestedPriceEstimator) GetDynamicFee(_ context.Context, _ uint32, _ *assets.Wei) (fee DynamicFee, chainSpecificGasLimit uint32, err error) { | ||
| err = errors.New("dynamic fees are not implemented for this estimator") | ||
| return | ||
| } | ||
|
|
||
| func (*SuggestedPriceEstimator) BumpDynamicFee(_ context.Context, _ DynamicFee, _ uint32, _ *assets.Wei, _ []EvmPriorAttempt) (bumped DynamicFee, chainSpecificGasLimit uint32, err error) { | ||
| err = errors.New("dynamic fees are not implemented for this estimator") | ||
| return | ||
| } | ||
|
|
||
| func (o *SuggestedPriceEstimator) GetLegacyGas(ctx context.Context, _ []byte, GasLimit uint32, maxGasPriceWei *assets.Wei, opts ...feetypes.Opt) (gasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) { | ||
| chainSpecificGasLimit = GasLimit | ||
| ok := o.IfStarted(func() { | ||
| if slices.Contains(opts, feetypes.OptForceRefetch) { | ||
| err = o.forceRefresh(ctx) | ||
| } | ||
| if gasPrice = o.getGasPrice(); gasPrice == nil { | ||
| err = errors.New("failed to estimate gas; gas price not set") | ||
|
|
@@ -177,8 +188,47 @@ func (o *SuggestedPriceEstimator) GetLegacyGas(ctx context.Context, _ []byte, Ga | |
| return | ||
| } | ||
|
|
||
| // Refreshes the gas price by making a call to the RPC in case the current one has gone stale. | ||
| // Adds the larger of BumpPercent and BumpMin configs as a buffer on top of the price returned from the RPC. | ||
| // The only reason bumping logic would be called on the SuggestedPriceEstimator is if there was a significant price spike | ||
| // between the last price update and when the tx was submitted. Refreshing the price helps ensure the latest market changes are accounted for. | ||
| func (o *SuggestedPriceEstimator) BumpLegacyGas(ctx context.Context, originalFee *assets.Wei, feeLimit uint32, maxGasPriceWei *assets.Wei, _ []EvmPriorAttempt) (newGasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) { | ||
| chainSpecificGasLimit = feeLimit | ||
| ok := o.IfStarted(func() { | ||
| // Immediately return error if original fee is greater than or equal to the max gas price | ||
| // Prevents a loop of resubmitting the attempt with the max gas price | ||
| if originalFee.Cmp(maxGasPriceWei) >= 0 { | ||
| err = fmt.Errorf("original fee (%s) greater than or equal to max gas price (%s) so cannot be bumped further", originalFee.String(), maxGasPriceWei.String()) | ||
| return | ||
| } | ||
| err = o.forceRefresh(ctx) | ||
| if newGasPrice = o.getGasPrice(); newGasPrice == nil { | ||
| err = errors.New("failed to refresh and return gas; gas price not set") | ||
| return | ||
| } | ||
| o.logger.Debugw("GasPrice", newGasPrice, "GasLimit", feeLimit) | ||
| }) | ||
| if !ok { | ||
| return nil, 0, errors.New("estimator is not started") | ||
| } else if err != nil { | ||
| return | ||
| } | ||
| if newGasPrice != nil && newGasPrice.Cmp(maxGasPriceWei) > 0 { | ||
|
There was a problem hiding this comment. No need to return error here, we can let it cap on the max price down below. Just add a log below if the price gets capped so we get alerted. There was a problem hiding this comment. I think we only want to cap at the max gas price if the price from the RPC is still lower than the max. In that scenario, we'd still be using a price that's equal or above the one recommended by the RPC. This clause checks if the price from the RPC is higher than the max gas price. If it is then capping at the max gas price would submit a tx below the recommended price which wouldn't succeed. I was thinking to treat this the same as the other estimators that have bumped past the max and return an error. There was a problem hiding this comment. So here our maxGasPriceWei is lower than what the chain recommends. I see this as a bad configuration by the NOP. |
||
| return nil, 0, errors.Errorf("estimated gas price: %s is greater than the maximum gas price configured: %s", newGasPrice.String(), maxGasPriceWei.String()) | ||
| } | ||
| // Add a buffer on top of the gas price returned by the RPC. | ||
| // Bump logic when using the suggested gas price from an RPC is realistically only needed when there is increased volatility in gas price. | ||
| // This buffer is a precaution to increase the chance of getting this tx on chain | ||
| bufferedPrice := fee.MaxBumpedFee(newGasPrice.ToInt(), o.cfg.BumpPercent(), o.cfg.BumpMin().ToInt()) | ||
|
There was a problem hiding this comment. This is a weird thing to do here. Suggested estimator is supposed to only fetch what the RPC suggests. Although bumping seems good on paper it comes with a few drawbacks. On specific chains, the buffer needs to be disabled as it will mess up the tx inclusion unless you post exactly what the RPC suggests (i.e. Fantom, Klaytn). For chains that do want to enable this, we just rely on the same configs from the block history estimator, which might not be entirely applicable here. SCALE would have to figure those values out for existing and new chains. There was a problem hiding this comment. Would it be possible to disable this buffering behavior by setting +1 on updating the docs as well. There was a problem hiding this comment. Setting There was a problem hiding this comment. By default for SuggestedPrice, we already have BumpThreshold = 0 I think this is reasonable There was a problem hiding this comment.
@prashantkumar1982 where is this logic set? Looking at the config docs the https://github.com/smartcontractkit/chainlink/blob/develop/docs/CONFIG.md#bumpthreshold |
||
| // If the new suggested price is less than or equal to the max and the buffer puts the new price over the max, return the max price instead | ||
| // The buffer is added on top of the suggested price during bumping as just a precaution. It is better to resubmit the transaction with the max gas price instead of erroring. | ||
| newGasPrice = assets.NewWei(bigmath.Min(bufferedPrice, maxGasPriceWei.ToInt())) | ||
|
|
||
| // Return the original price if the refreshed price with the buffer is lower to ensure the bumped gas price is always equal or higher to the previous attempt | ||
| if originalFee != nil && originalFee.Cmp(newGasPrice) > 0 { | ||
| return originalFee, chainSpecificGasLimit, nil | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func (o *SuggestedPriceEstimator) getGasPrice() (GasPrice *assets.Wei) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add some comments describing how bumping makes use of the RPC call, and bump% and bumpMin settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comments in the latest commit