-
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 2 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.
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.