Submitter: setGasPrice() refactor #2462
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent updates focus on enhancing gas price handling in the EthErgo project. Key changes include safeguarding against Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Deploying sanguine-fe with
|
| Latest commit: |
a4057c5
|
| Status: | 🚫 Build failed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## submitter/dynamic-tx-fees #2462 +/- ##
===================================================================
+ Coverage 46.86050% 47.09232% +0.23181%
===================================================================
Files 400 400
Lines 30610 30729 +119
Branches 83 83
===================================================================
+ Hits 14344 14471 +127
+ Misses 14731 14722 -9
- Partials 1535 1536 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dwasse
changed the title
ethergo/submitter/config/config.go
Outdated
| // DefaultBaseGasPrice is the default max price of a tx. | ||
| var DefaultBaseGasPrice = big.NewInt(1 * params.GWei) | ||
| // DefaultMinGasPrice is the default min price of a tx. | ||
| var DefaultMinGasPrice = big.NewInt(10 * params.GWei) |
There was a problem hiding this comment.
Min gas price of 10 Gwei is an overshoot.
There was a problem hiding this comment.
This makes sense- I'm confused of the intention here though in the base PR?
To me this seems like we will never set gas price below 10 gwei if no prevTx is supplied
There was a problem hiding this comment.
The unit there is in wei
There was a problem hiding this comment.
Let's introduce a min value for GasPrice/GasFeeCap (DefaultMinGasPrice) and a separate one for GasTipCap. The latter one is set to 10 Wei (aka just 10) in the base PR.
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- ethergo/chain/gas/cmp.go (1 hunks)
- ethergo/submitter/config/config.go (4 hunks)
- ethergo/submitter/config/iconfig_generated.go (2 hunks)
- ethergo/submitter/submitter.go (1 hunks)
- ethergo/submitter/submitter_test.go (2 hunks)
Additional Context Used
GitHub Check Runs (1)
codecov/patch success (9)
ethergo/chain/gas/cmp.go: [warning] 110-111: ethergo/chain/gas/cmp.go#L110-L111
Added lines #L110 - L111 were not covered by tests
ethergo/submitter/config/config.go: [warning] 114-115: ethergo/submitter/config/config.go#L114-L115
Added lines #L114 - L115 were not covered by tests
ethergo/submitter/config/config.go: [warning] 118-119: ethergo/submitter/config/config.go#L118-L119
Added lines #L118 - L119 were not covered by tests
ethergo/submitter/config/config.go: [warning] 122-123: ethergo/submitter/config/config.go#L122-L123
Added lines #L122 - L123 were not covered by tests
ethergo/submitter/config/config.go: [warning] 221-222: ethergo/submitter/config/config.go#L221-L222
Added lines #L221 - L222 were not covered by tests
ethergo/submitter/submitter.go: [warning] 375-376: ethergo/submitter/submitter.go#L375-L376
Added lines #L375 - L376 were not covered by tests
ethergo/submitter/submitter.go: [warning] 466-467: ethergo/submitter/submitter.go#L466-L467
Added lines #L466 - L467 were not covered by tests
ethergo/submitter/submitter.go: [warning] 511-512: ethergo/submitter/submitter.go#L511-L512
Added lines #L511 - L512 were not covered by tests
ethergo/submitter/submitter.go: [warning] 527-528: ethergo/submitter/submitter.go#L527-L528
Added lines #L527 - L528 were not covered by tests
Additional comments not posted (4)
ethergo/submitter/config/config.go (1)
31-38: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [34-56]
Please ensure that the renaming from
BaseGasPricetoMinGasPriceis covered by unit tests, particularly for theGetMinGasPriceandSetMinGasPricemethods.Would you like assistance in creating or updating the tests to cover these changes?
ethergo/submitter/submitter_test.go (1)
30-208: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [9-205]
Ensure comprehensive test coverage for the new gas pricing logic, including scenarios for legacy and dynamic transactions, to validate the changes thoroughly.
ethergo/submitter/submitter.go (2)
391-424: Ensure null safety forprevTx.GasPrice(),prevTx.GasFeeCap(), andprevTx.GasTipCap()inbumpGasFromPrevTx.Consider adding null checks for
prevTx.GasPrice(),prevTx.GasFeeCap(), andprevTx.GasTipCap()before using them to prevent potential null pointer dereferences.
494-519: Verify the logic inapplyGasCeilfor handling gas tip cap exceeding gas fee cap.Ensure that the logic in
applyGasCeilcorrectly handles scenarios where the gas tip cap exceeds the gas fee cap, especially in dynamic transactions. This is crucial for maintaining transaction validity and preventing unexpected costs.
ethergo/submitter/config/config.go
Outdated
| // GetBaseGasPrice returns the maximum gas price to use for transactions. | ||
| func (c *Config) GetBaseGasPrice(chainID int) (basePrice *big.Int) { | ||
| basePrice = c.BaseGasPrice | ||
| // GetMinGasPrice returns the maximum gas price to use for transactions. |
There was a problem hiding this comment.
| // GetMinGasPrice returns the maximum gas price to use for transactions. | |
| // GetMinGasPrice returns the minimum gas price to use for transactions. |
* attempt to fix dynamic gas fees bumping * [goreleaser] * metrics sync * [goreleaser] * allow zero gas tip cap [goreleaser] * fix errors [goreleaser] * [goreleaser] fix interface * fix [goreleaser] * [goreleaser] fix tx * 1 wei->10 wei [goreleaser] * Revert "1 wei->10 wei [goreleaser]" This reverts commit 21abb18. * Revert "[goreleaser] fix tx" This reverts commit 94d3d6d. * revert bumpDynamicTxFees to master * newFeeCap [goreleaser] * f * [goreleaser] revert me extremely, extremely aggressive printing * [goreleaser] * [goreleaser] * Revert "[goreleaser] revert me extremely, extremely aggressive printing" This reverts commit e961d54. * return nil a * [goreleaser] * Fix: check for eip1559 before flooring GasTipCap * [goreleaser] * Submitter: setGasPrice() refactor (#2462) * Feat: initial refactor of setGasPrice() * Feat: add more tracing * Cleanup: comments * Feat: BaseGasPrice -> MinGasPrice * Cleanup: remove applyMinGasPrice() * Feat: initial test cases * Feat: add WithOracleOverride cases * Feat: add PrevTx cases * Feat: add assertGasValues() helper * Cleanup: pass useDynamic to applyGasFloor() * Feat: remove old submitter test * Cleanup: lint * [goreleaser] * Cleanup: populateFromPrevTx() -> bumpFromPrevTx() * Feat: cap GasTipCap by GasFeeCap * [goreleaser] * Feat: default min gas price to 1 gwei * [goreleaser] * Feat: add minTipCap var * Fix: minTipCap = 10 wei * Cleanup: comment * [goreleaser] * Fix: test --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com> Co-authored-by: Daniel Wasserman <wassermandaniel8@gmail.com>
Summary by CodeRabbit
New Features
Bug Fixes
nilgas prices could cause errors by adding anilcheck.Refactor
BaseGasPricetoMinGasPriceacross several files for clarity.IConfiginterface to reflect the new gas price terminology.Tests