-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
+dynamic_gas_unit_add_percentage flag #3494
Changes from 11 commits
dfbf6ba
93459f1
905a3a5
09816fb
63f730c
31ca3cf
d8481f2
d7f1cad
3d7eea2
4c11d33
145583d
2005d91
a8a48bb
75c9c44
96e16c3
dada2b0
7abfdb0
0e7c236
220e0d4
b995385
4782b44
d699cd0
04ccdc2
c8f2dfe
d199850
e8fd10e
ee9eb85
f6bfa43
c79a81b
82b4f99
5b02cca
965802d
7b62df2
73442fa
ab18ea9
cf43fdc
c24c568
0e35f40
8adb278
961fbfc
6586d73
528839f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,13 +77,15 @@ gas_bump_percentage: 10 | |
gas_estimate: 1000 | ||
is_l2: true | ||
dynamic_gas_estimate: true | ||
dynamic_gas_unit_add_percentage: 20 | ||
supports_eip_1559: true` | ||
var cfg config.Config | ||
err := yaml.Unmarshal([]byte(cfgStr), &cfg) | ||
assert.NoError(t, err) | ||
assert.Equal(t, big.NewInt(250000000000), cfg.MaxGasPrice) | ||
assert.Equal(t, 60, cfg.BumpIntervalSeconds) | ||
assert.Equal(t, 10, cfg.GasBumpPercentage) | ||
assert.Equal(t, 20, cfg.DynamicGasUnitAddPercentage) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance test coverage for the new configuration parameter. While the basic YAML parsing test is good, consider adding the following test cases:
Here's a suggested implementation: func TestGetters(t *testing.T) {
cfg := config.Config{
ChainConfig: config.ChainConfig{
MaxBatchSize: 5,
DoNotBatch: false,
MaxGasPrice: big.NewInt(250 * params.GWei),
+ DynamicGasUnitAddPercentage: 15,
},
Chains: map[int]config.ChainConfig{
MaxBatchSize: 8,
DoNotBatch: true,
MaxGasPrice: big.NewInt(300 * params.GWei),
+ DynamicGasUnitAddPercentage: 25,
},
MaxBatchSize: 0, // Should use global config value
},
},
}
// ... existing test cases ...
+ t.Run("GetDynamicGasUnitAddPercentage", func(t *testing.T) {
+ // Test chain-specific value
+ assert.Equal(t, 25, cfg.GetDynamicGasUnitAddPercentage(1))
+ // Test fallback to global value
+ assert.Equal(t, 15, cfg.GetDynamicGasUnitAddPercentage(2))
+ // Test nonexistent chain
+ assert.Equal(t, 15, cfg.GetDynamicGasUnitAddPercentage(999))
+
+ // Test default value
+ emptyCfg := config.Config{}
+ assert.Equal(t, config.DefaultDynamicGasUnitAddPercentage,
+ emptyCfg.GetDynamicGasUnitAddPercentage(1))
+
+ // Test validation (if implemented)
+ invalidCfg := config.Config{
+ ChainConfig: config.ChainConfig{
+ DynamicGasUnitAddPercentage: -1,
+ },
+ }
+ assert.Equal(t, config.DefaultDynamicGasUnitAddPercentage,
+ invalidCfg.GetDynamicGasUnitAddPercentage(1))
+ })
}
|
||
assert.Equal(t, uint64(1000), cfg.GasEstimate) | ||
assert.Equal(t, true, cfg.DynamicGasEstimate) | ||
assert.Equal(t, true, cfg.SupportsEIP1559(0)) | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -392,9 +392,6 @@ | |||||
if err != nil { | ||||||
span.AddEvent("could not set gas price", trace.WithAttributes(attribute.String("error", err.Error()))) | ||||||
} | ||||||
if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | ||||||
transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | ||||||
} | ||||||
|
||||||
transactor.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | ||||||
locker = t.nonceMux.Lock(chainID) | ||||||
|
@@ -421,9 +418,27 @@ | |||||
//nolint: wrapcheck | ||||||
return parentTransactor.Signer(address, transaction) | ||||||
} | ||||||
|
||||||
transactor.NoSend = true | ||||||
|
||||||
tx_forEstimate, err := call(transactor) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Follow Go naming conventions The variable name - tx_forEstimate, err := call(transactor)
+ txForEstimate, err := call(transactor) 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: Lint (ethergo)[failure] 424-424: |
||||||
if err != nil { | ||||||
return 0, fmt.Errorf("err contract call sim: %w", err) | ||||||
} | ||||||
|
||||||
gasEstimate, err := t.getGasEstimate(ctx, chainClient, int(chainID.Uint64()), tx_forEstimate) | ||||||
parodime marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if err != nil { | ||||||
return 0, fmt.Errorf("err getGasEstimate: %w", err) | ||||||
} | ||||||
|
||||||
transactor.GasLimit = gasEstimate | ||||||
|
||||||
transactor.NoSend = false | ||||||
|
||||||
tx, err := call(transactor) | ||||||
|
||||||
if err != nil { | ||||||
return 0, fmt.Errorf("could not call contract: %w", err) | ||||||
return 0, fmt.Errorf("err contract call exec: %w", err) | ||||||
} | ||||||
defer locker.Unlock() | ||||||
|
||||||
|
@@ -677,13 +692,20 @@ | |||||
// getGasEstimate gets the gas estimate for the given transaction. | ||||||
// TODO: handle l2s w/ custom gas pricing through contracts. | ||||||
func (t *txSubmitterImpl) getGasEstimate(ctx context.Context, chainClient client.EVM, chainID int, tx *types.Transaction) (gasEstimate uint64, err error) { | ||||||
|
||||||
// if dynamic gas estimation is not enabled, use cfg var gas_estimate as a default | ||||||
if !t.config.GetDynamicGasEstimate(chainID) { | ||||||
return t.config.GetGasEstimate(chainID), nil | ||||||
} | ||||||
|
||||||
gasUnitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(chainID) | ||||||
|
||||||
ctx, span := t.metrics.Tracer().Start(ctx, "submitter.getGasEstimate", trace.WithAttributes( | ||||||
attribute.Int(metrics.ChainID, chainID), | ||||||
|
||||||
attribute.String(metrics.TxHash, tx.Hash().String()), | ||||||
|
||||||
attribute.Int("gasUnitAddPercentage", gasUnitAddPercentage), | ||||||
)) | ||||||
|
||||||
defer func() { | ||||||
|
@@ -696,20 +718,18 @@ | |||||
if err != nil { | ||||||
return 0, fmt.Errorf("could not convert tx to call: %w", err) | ||||||
} | ||||||
// tmpdebug | ||||||
fmt.Printf("Debug Calling EstimateGas") | ||||||
|
||||||
gasEstimate, err = chainClient.EstimateGas(ctx, *call) | ||||||
if err != nil { | ||||||
span.AddEvent("could not estimate gas", trace.WithAttributes(attribute.String("error", err.Error()))) | ||||||
|
||||||
// tmpdebug | ||||||
fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID)) | ||||||
|
||||||
// fallback to default | ||||||
return t.config.GetGasEstimate(chainID), nil | ||||||
} | ||||||
|
||||||
// Modify the gasEstimate by the configured percentage | ||||||
gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) | ||||||
Check failure on line 731 in ethergo/submitter/submitter.go GitHub Actions / Lint (ethergo)
|
||||||
|
||||||
parodime marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return gasEstimate, nil | ||||||
} | ||||||
|
||||||
|
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.
Are we sure we don't want to default to zero? This is fine with me, just making sure since may not be obvious to others
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.
with zaps we're going to need to start doing dynamic estimates all the time and relying on the simulation to give us enough limit. a small boost by default on top of the simulation is a good idea just to further reduce chance of OOG
also -- our flat limits that we use today, by comparison, give us a boost of like +5000% so +5% is pretty small by that measure
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.
For reference, most wallets are using a 50% buffer on top of the estimated gas limit. So I'd recommend increasing the default value a bit here.