feat: gas estimator #7
Conversation
dlipicar
force-pushed
the
feat/gas-estimator
branch
from
c7344bc to
e44b74d
Compare
dlipicar
force-pushed
the
feat/ethclient
branch
3 times, most recently
from
a7ee8ac to
c21c8e6
Compare
dlipicar
force-pushed
the
feat/gas-estimator
branch
2 times, most recently
from
21280d6 to
b6cfb2b
Compare
dlipicar
force-pushed
the
feat/gas-estimator
branch
from
b6cfb2b to
7493005
Compare
dlipicar
force-pushed
the
feat/gas-estimator
branch
2 times, most recently
from
b7beaea to
d9d02d6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7 +/- ##
==========================================
+ Coverage 37.46% 42.08% +4.62%
==========================================
Files 35 50 +15
Lines 1906 2578 +672
==========================================
+ Hits 714 1085 +371
- Misses 1166 1449 +283
- Partials 26 44 +18
🚀 New features to boost your workflow:
|
dlipicar
force-pushed
the
feat/gas-estimator
branch
from
31c05bf to
3a53827
Compare
|
|
||
| // GetGasSuggestions retrieves gas fee suggestions from Infura's Gas API | ||
| func (c *Client) GetGasSuggestions(ctx context.Context, networkID int) (*GasResponse, error) { | ||
| url := fmt.Sprintf("%s/networks/%d/suggestedGasFees", c.baseURL, networkID) |
There was a problem hiding this comment.
We can move this pattern string as const on top of the file for more convenient editing later.
Also baseURL used 2 times above could be a const on top.
wdyt?
There was a problem hiding this comment.
hmm I never usually extract printf pattern strings as constants, but I'm not sure I can give a very good reason why. I guess they're too tied to the variable types you use in the instruction so it's better to keep them close together?
baseURL as a member allows overriding it to mock an http server in tests.
pkg/gas/suggestions_l1.go
Outdated
| gasPrice := suggestGasPrice(feeHistory, config.GasPriceEstimationBlocks) | ||
|
|
||
| // Apply multiplier to BaseFee | ||
| suggestedBaseFee := new(big.Int).Mul(gasPrice.BaseFeePerGas, big.NewInt(int64(config.BaseFeeMultiplier*1000))) |
There was a problem hiding this comment.
Question: do we use multiplying and dividing to avoid using big.Float?
There was a problem hiding this comment.
yeah it is a bit sketchy, I'll use Float
pkg/gas/utils.go
Outdated
| return big.NewInt(0) | ||
| } | ||
|
|
||
| index := (len(sortedData) * int(math.Ceil(percentile))) / 100 |
There was a problem hiding this comment.
It seems that this implementation tends to return bigger indexes. And the check index >= len fixes only the last case.
If we will be looking for 10th percentile in array [10,20,30,40,50,60,70,80,90,100], it will return index 1, which will result in value 20.
Example testcase that I've used:
func TestGetPercentile_TenValues(t *testing.T) {
// Test with 10 values evenly distributed from 10 to 100
data := []*big.Int{
big.NewInt(10), big.NewInt(20), big.NewInt(30), big.NewInt(40), big.NewInt(50),
big.NewInt(60), big.NewInt(70), big.NewInt(80), big.NewInt(90), big.NewInt(100),
}
assert.Equal(t, big.NewInt(10), getPercentile(data, 10))
assert.Equal(t, big.NewInt(30), getPercentile(data, 25))
assert.Equal(t, big.NewInt(50), getPercentile(data, 50))
assert.Equal(t, big.NewInt(80), getPercentile(data, 75))
assert.Equal(t, big.NewInt(90), getPercentile(data, 90))
assert.Equal(t, big.NewInt(100), getPercentile(data, 95))
assert.Equal(t, big.NewInt(100), getPercentile(data, 99))
assert.Equal(t, big.NewInt(100), getPercentile(data, 100))
// Edge cases
assert.Equal(t, big.NewInt(10), getPercentile(data, 0)) // 0th percentile = minimum
assert.Equal(t, big.NewInt(10), getPercentile(data, 5)) // 5th percentile
}As an alternative, here is a function with "nearest-rank" percentile methods and few more checks:
// getPercentile calculates the value at a given percentile from sorted data
// Uses the "nearest-rank" method: index = ceil(percentile/100 * n) - 1
func getPercentile(sortedData []*big.Int, percentile float64) *big.Int {
if len(sortedData) == 0 {
return big.NewInt(0)
}
n := len(sortedData)
// Handle edge cases
if percentile <= 0 {
return new(big.Int).Set(sortedData[0])
}
if percentile >= 100 {
return new(big.Int).Set(sortedData[n-1])
}
// Calculate the rank using nearest-rank method
rank := math.Ceil(percentile / 100.0 * float64(n))
// Convert to 0-based index
index := int(rank) - 1
// Ensure index is within bounds (should already be, but safety check)
if index < 0 {
index = 0
}
if index >= n {
index = n - 1
}
return new(big.Int).Set(sortedData[index])
}
pkg/gas/gasprice.go
Outdated
| ) | ||
|
|
||
| func suggestGasPrice(feeHistory *ethclient.FeeHistory, nBlocks int) *GasPrice { | ||
| estimatedBaseFee := feeHistory.BaseFeePerGas[len(feeHistory.BaseFeePerGas)-1] |
There was a problem hiding this comment.
if feeHistory is empty for some reason we might want to return fallback prices
friofry
left a comment
friofry
left a comment
There was a problem hiding this comment.
Looks good to me!
It's easy to review small files. The examples work well.
Added a few suggestions
|
|
||
| // Transaction types. | ||
| const ( | ||
| LegacyTxType = 0x00 |
There was a problem hiding this comment.
We certainly depend on go-ethereum, why don't we use these constants directly from go-etherem's types package?
There was a problem hiding this comment.
geth only includes L1 transaction types, defining this here allows us to have a combined list with types from other L2s (ie https://github.com/EspressoSystems/geth-arbitrum/blob/integration/core/types/transaction.go#L48) like we had in our go-ethereum fork.
dlipicar
force-pushed
the
feat/gas-estimator
branch
from
3a53827 to
7a30b01
Compare
dlipicar
force-pushed
the
feat/gas-estimator
branch
3 times, most recently
from
b4d03f6 to
48fe7e1
Compare
dlipicar
force-pushed
the
feat/gas-estimator
branch
4 times, most recently
from
a9ce756 to
ee049d1
Compare
dlipicar
force-pushed
the
feat/gas-estimator
branch
from
ee049d1 to
f342814
Compare
5 participants
Introduce module to estimate/suggest:
Comparison output: