Monitor avg gas price #3752
Monitor avg gas price #3752
Conversation
|
this seemed like a reasonably simple avg gas price calculation: each new sample contributes 20% weight, the previous rolling value keeps 80%. of course we could also keep a set of samples or change these weights (or make them configurable, but this feels like something that should just have sane defaults and no-one should really want to touch) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3752 +/- ##
===================================================
+ Coverage 31.94444% 31.97435% +0.02991%
===================================================
Files 158 158
Lines 47520 47560 +40
===================================================
+ Hits 15180 15207 +27
- Misses 31437 31447 +10
- Partials 903 906 +3
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes the hardcoded average gas price and implements a proper moving average calculation within the GasPriceMonitor. The change replaces a static 3 gwei hardcoded value with a dynamic rolling average that tracks actual gas price samples over time.
Key changes:
- Remove hardcoded 3 gwei average gas price constant
- Add AvgGasPrice() method to GasPriceMonitor interface and implementation
- Implement exponential moving average calculation with configurable smoothing parameters
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pm/recipient.go | Removes hardcoded avgGasPrice constant and updates faceValue function to use monitor's AvgGasPrice() method |
| eth/gaspricemonitor.go | Adds AvgGasPrice() method and implements exponential moving average calculation logic |
| pm/stub.go | Adds AvgGasPrice() method to stubGasPriceMonitor for testing compatibility |
| pm/recipient_test.go | Updates test fixtures to initialize avgPrice field in stub monitor |
| eth/gaspricemonitor_test.go | Adds comprehensive tests for new AvgGasPrice functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| term1 := new(big.Int).Mul(gpm.avgGasPrice, big.NewInt(den-num)) | ||
| term2 := new(big.Int).Mul(gasPrice, big.NewInt(num)) | ||
| term1.Add(term1, term2) | ||
| term1.Add(term1, new(big.Int).Div(big.NewInt(den), big.NewInt(2))) |
There was a problem hiding this comment.
The addition of den/2 appears to be for rounding purposes but is not documented. This magic number makes the calculation unclear. Consider adding a comment explaining this is for rounding to nearest integer in the division, or extract it to a named variable like roundingOffset.
| term1.Add(term1, new(big.Int).Div(big.NewInt(den), big.NewInt(2))) | |
| // Add roundingOffset to round to the nearest integer when dividing | |
| roundingOffset := big.NewInt(den / 2) | |
| term1.Add(term1, roundingOffset) |
Copilot uses AI. Check for mistakes.
| num := int64(gpm.smoothingNum) | ||
| if den <= 0 || num <= 0 || num >= den { | ||
| num = 1 | ||
| den = 5 | ||
| } |
There was a problem hiding this comment.
The fallback values num = 1 and den = 5 are magic numbers that duplicate the default values set in NewGasPriceMonitor. Consider extracting these as named constants to maintain consistency and make the relationship clear.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think it looks good. Maybe just one (stupid) question, do we bother about the gas price anymore? Maybe we should just hardcode it to something super low like 0.3 gwei or remove from the calculations at all?
Then, paying for the gas would be on the Orchestrator. I'm saying that because the gas price is very low and it makes all this price calculation very complex, plus less predictable. E.g. you can imagine a short spike of gas and suddenly the price increases and it starts to exceed the max ticket ev. Then, all stops working.
I think the whole idea of Gateway/Broadcaster paying for the gas was because the gas price was high in Ethereum and wanted to take this number into the picture. Now, gas fee is low, so I'd rather remove it from everywhere and make it simpler.
What does this pull request do? Explain your changes. (required)
Removes the hardcoded avg gas price, rather track an actual moving average in the GasPriceMonitor
Specific updates (required)
How did you test each of these updates (required)
Not yet
Does this pull request close any open issues?
Related to #3746 and #3744
Checklist:
makeruns successfully./test.shpass