"FeeBoost" : SVR Gas Strategy#417
Conversation
product-security-plaid-production
Bot
requested review from
amit-momin and
pavel-raykov
|
👋 augustbleeds, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
There was a problem hiding this comment.
Pull request overview
Adds a FeeBoost configuration flag for TxM v2 (intended for SVR/DualBroadcast) that forces transaction attempts to use the estimator’s “max fee” path for more aggressive gas pricing, and wires the flag through builder/test/docs updates.
Changes:
- Introduce
FeeBoostin TxM v2 TOML/config interfaces and documentation. - Plumb a
feeBoostflag intotxm.NewAttemptBuilderand use it to selectGetMaxFee(and skip bumping) for attempts. - Update unit/integration tests and example configs to include the new parameter/setting.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/txmgr/builder.go | Computes feeBoost (gated by DualBroadcast) and passes it into the attempt builder. |
| pkg/txm/attempt_builder.go | Adds feeBoost flag; switches attempt fee selection to GetMaxFee when enabled and skips bumping. |
| pkg/txm/attempt_builder_test.go | Updates constructor call sites for the new feeBoost parameter. |
| pkg/txm/txm_test.go | Updates constructor call sites for the new feeBoost parameter. |
| pkg/txm/integration-tests/integration_test.go | Updates constructor call sites for the new feeBoost parameter. |
| pkg/config/config.go | Extends TransactionManagerV2 interface with FeeBoost() bool. |
| pkg/config/chain_scoped_transactions.go | Implements FeeBoost() for chain-scoped TMv2 config. |
| pkg/config/toml/config.go | Adds FeeBoost to TOML struct and merge behavior. |
| pkg/config/toml/config_test.go | Updates defaults/docs alignment tests and full-config expectations for FeeBoost. |
| pkg/config/toml/docs.toml | Documents the new FeeBoost option. |
| pkg/config/toml/testdata/config-full.toml | Adds FeeBoost = true to the full TOML example. |
| CONFIG.md | Documents the new FeeBoost option in the generated config docs. |
Comments suppressed due to low confidence (1)
pkg/txm/attempt_builder.go:49
- When feeBoost is enabled, NewAttempt calls GetMaxFee with a.feeBoost but still passes a.emptyTxLimitDefault as the feeLimit. This value is intended for purge/empty transactions and can cause normal transactions to either (a) fail with ErrFeeLimitTooLow when EstimateLimit is enabled (because providedGasLimit becomes too low), or (b) be broadcast with an under-sized gas limit when EstimateLimit is disabled. Use tx.SpecifiedGasLimit for the feeLimit in the feeBoost path (and only fall back to emptyTxLimitDefault when tx.IsPurgeable).
if tx.IsPurgeable || a.feeBoost {
fee, estimatedGasLimit, err = a.EvmFeeEstimator.GetMaxFee(ctx, tx.Data, a.emptyTxLimitDefault, a.priceMaxKey(tx.FromAddress), &tx.FromAddress, &tx.ToAddress)
if err != nil {
return nil, err
}
} else {
fee, estimatedGasLimit, err = a.EvmFeeEstimator.GetFee(ctx, tx.Data, tx.SpecifiedGasLimit, a.priceMaxKey(tx.FromAddress), &tx.FromAddress, &tx.ToAddress)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (a *attemptBuilder) NewAgnosticBumpAttempt(ctx context.Context, lggr logger.Logger, tx *types.Transaction, dynamic bool) (attempt *types.Attempt, err error) { | ||
| // if the transaction is purgeable, NewAttempt will return the max fee instantly, so there is no need to bump | ||
| // if the transaction is purgeable or feeBoost is enabled, NewAttempt will return the max fee instantly, so there is no need to bump | ||
| attempt, err = a.NewAttempt(ctx, lggr, tx, dynamic) | ||
| if tx.IsPurgeable || err != nil { | ||
| if tx.IsPurgeable || a.feeBoost || err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
The new feeBoost behavior (always using GetMaxFee and skipping bumping) isn’t covered by tests. Add a unit test that constructs an attemptBuilder with feeBoost=true and asserts (1) GetMaxFee is called with the transaction’s SpecifiedGasLimit (or 0) for non-purgeable txs, and (2) NewAgnosticBumpAttempt returns after the initial attempt without calling BumpFee.
| // if the transaction is purgeable or feeBoost is enabled, NewAttempt will return the max fee instantly, so there is no need to bump | ||
| attempt, err = a.NewAttempt(ctx, lggr, tx, dynamic) | ||
| if tx.IsPurgeable || err != nil { | ||
| if tx.IsPurgeable || a.feeBoost || err != nil { |
There was a problem hiding this comment.
Since we have another case, I would split the error check from the rest, just for better clarity.
| var err error | ||
| if tx.IsPurgeable { | ||
| if tx.IsPurgeable || a.feeBoost { | ||
| fee, estimatedGasLimit, err = a.EvmFeeEstimator.GetMaxFee(ctx, tx.Data, a.emptyTxLimitDefault, a.priceMaxKey(tx.FromAddress), &tx.FromAddress, &tx.ToAddress) |
There was a problem hiding this comment.
There is a bug here: GetMaxFee uses the emptyTxLimitDefault which is only correct in case of an empty transaction. We need to use SpecifiedGasLimit instead.
There was a problem hiding this comment.
Please add a test case in attempt_builder_test.go
Instead of a separate gas config, we opt for a simpler approach to just use the max fee each if the feeboost ie enabled on txmv2