-
Notifications
You must be signed in to change notification settings - Fork 167
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
pm: lower avgGasPrice #2481
pm: lower avgGasPrice #2481
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2481 +/- ##
===================================================
- Coverage 54.70159% 54.67822% -0.02337%
===================================================
Files 95 95
Lines 19855 19858 +3
===================================================
- Hits 10861 10858 -3
- Misses 8390 8396 +6
Partials 604 604
Continue to review full report at Codecov.
|
1ade1c6
to
5cd3251
Compare
Just to clarify my own understanding (I wrote this avgGasPrice check awhile ago, but I need to jog my memory around how it works...):
The check With this in mind, it makes sense that a lower I think this PR is fine as-is though as the more ideal solution is to not hardcode a value for |
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.
Minor comment about fixing an outdated comment.
Other than that LGTM.
pm/recipient.go
Outdated
@@ -28,7 +28,7 @@ var evMultiplier = big.NewInt(100) | |||
|
|||
// Hardcode to 200 gwei |
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.
Let's update this comment
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.
Updated in 98cad6d
Yes, everything you described is correct. You may be right that we should maybe not make any assumptions about our B reserve. So how, about setting average gas price to some more realistic number in Arbitrum, like |
👍 |
Updated in 98cad6d |
98cad6d
to
eefa358
Compare
What does this pull request do? Explain your changes. (required)
Lower the hardcoded avg gas price to prevent Os from dropping streams in case of the gas price spikes.
The value is changed from
200
=>140
. Here's the rationale why this value was chosen. In our problematic scenario, in the case whenmaxFaceValue
is not set, then:faceValue.Cmp(r.txCostWithGasPrice(avgGasPrice)) < 0
must befalse
180000000000000000
)txCostWithGasPrice()
is calculated asredeemGas * avgGasPrice
, where ArbitrumredeemGas
is hardcoded as1200000
180000000000000000 < 1200000 * avgGasPrice
must befalse
180000000000000000 >= 1200000 * avgGasPrice
must betrue
avgGasPrice <= 180000000000000000 / 1200000
=>avgGasPrice <= 150000000000
So, with the current B's reserve and the current hardcoded
redeemGas
, theavgGasPrice
must be smaller than150 Gwei
. To make a buffer, I changed it to140
.Specific updates (required)
avgGasPrice
How did you test each of these updates (required)
Debugged with Orchestrator.
Does this pull request close any open issues?
fix #2473
Checklist:
make
runs successfully./test.sh
passREADME and other documentation updated