Set From address for gas limit estimation feature #14246
Conversation
|
| @@ -191,7 +191,7 @@ func (w *chainWriter) GetFeeComponents(ctx context.Context) (*commontypes.ChainF | |||
| return nil, fmt.Errorf("gas estimator not available") | |||
| } | |||
|
|
|||
| fee, _, err := w.ge.GetFee(ctx, nil, 0, w.maxGasPrice, nil) | |||
| fee, _, err := w.ge.GetFee(ctx, nil, 0, w.maxGasPrice, nil, nil) | |||
There was a problem hiding this comment.
Is it possible for this being nil to cause errors?
There was a problem hiding this comment.
It shouldn't in this case. This parameter is only used for gas limit estimation and every parameter for thateth_estimateGas is optional. Since we're passing nil for calldata, from, and to, the gas estimation call would just return some default value. But that shouldn't matter here since the gas limit isn't being used.
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -362,6 +362,9 @@ func (e *evmFeeEstimator) estimateFeeLimit(ctx context.Context, feeLimit uint64, | |||
| To: toAddress, | |||
| Data: calldata, | |||
| } | |||
| if fromAddress != nil { | |||
There was a problem hiding this comment.
Shouldn't fromAddress be treated the same way we treat toAddress ?
There was a problem hiding this comment.
We can't because of callMsg's typing
type CallMsg struct {
From common.Address
To *common.Address
}
prashantkumar1982
deleted the
fix/set-from-address-gas-limit-estimation
branch
Gas limit estimation can throw an error if the
Fromaddress is not set and the call being estimated is privileged.eth_estimateGaswould return an unauthorized error preventing proper gas limit estimation. In this case, we would still broadcast the tx but we would fallback to the provide gas limit which could be set very high compared to what is actually needed. This change passes theFromaddress down to the gas estimator so it can be set in thecallMsg.