feat: add maximum gas price for rebalancing txs per chain as configurable field #11
Conversation
| chainConfig, err := config.GetConfigReader(ctx).GetChainConfig(rebalanceFromChainID) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("getting chain config for gas threshold check: %w", err) | ||
| } | ||
|
|
||
| if chainConfig.MaxRebalancingGasThreshold != 0 { | ||
| // Check if total gas needed exceeds threshold to rebalance funds from this chain | ||
| totalRebalancingGas, err := r.estimateTotalGas(txns) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("estimating total gas for transactions: %w", err) | ||
| } | ||
|
|
||
| if totalRebalancingGas > chainConfig.MaxRebalancingGasThreshold { | ||
| lmt.Logger(ctx).Info( | ||
| "skipping rebalance from chain "+rebalanceFromChainID+" due to rebalancing txs exceeding gas threshold", | ||
| zap.String("sourceChainID", rebalanceFromChainID), | ||
| zap.String("destinationChainID", rebalanceToChain), | ||
| zap.Uint64("estimatedGas", totalRebalancingGas), | ||
| zap.Uint64("gasThreshold", chainConfig.MaxRebalancingGasThreshold), | ||
| ) | ||
| continue | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I feel like this should be moved to a helper function, maybe something like
func (r *FundRebalancer) isGasAcceptable(...) {
// this logic
}| signedTxns, err := r.SignTxns(ctx, txns) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("signing txns required for fund rebalancing: %w", err) | ||
| } | ||
|
|
||
| txnHashes, err := r.SubmitTxns(ctx, signedTxns) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("submitting signed txns required for fund rebalancing: %w", err) | ||
| return nil, nil, fmt.Errorf("error submitting signed txns required for fund rebalancing: %w", err) |
There was a problem hiding this comment.
a personal nit of mine but remove the "error..." from the start of the wrap. its more convention to say just what happened in the wrapped error message, instead of saying "error..." or "failed..." at the start
| @@ -357,7 +357,7 @@ func TestFundRebalancer_Rebalance(t *testing.T) { | |||
| Return(ethTxs, nil).Once() | |||
|
|
|||
| mockSkipGo.EXPECT().SubmitTx(mockContext, mock.Anything, ethChainID). | |||
| Return(skipgo.TxHash("ethhash"), nil).Once() | |||
| func WithEstimatedGasLimit(from, to, value string, data []byte) TxBuildOption { | ||
| return func(ctx context.Context, b TxBuilder, tx *types.DynamicFeeTx) error { | ||
| to := common.HexToAddress(to) | ||
| // EstimateGasForTx estimates the gas needed for a transaction with the given parameters | ||
| func (b TxBuilder) EstimateGasForTx(ctx context.Context, from, to, value string, data []byte) (uint64, error) { | ||
| toAddr := common.HexToAddress(to) |
There was a problem hiding this comment.
i feel like the TxBuilder WithEstimatedGasLimit function should keep the node calling logic in it, so it is consistent with how all of the other functions work, like WithNonce, WithEstimatedGasTipCap, etc. I don't think its a big deal to make an extra node call to estimate the gas twice (once when building the actual tx that we are going to send, and once when checking if the estimated gas for the tx is > their configured max rebalance tx gas amount). I think if we want a separate function to do the gas estimation and return the amount, it should maybe be on a different struct, since it doesn't really make sense for the tx builder.
No description provided.