fix(zetaclient): tolerate priorityFee > gasFee #2955
Conversation
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve updates to the Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2955 +/- ##
========================================
Coverage 66.39% 66.40%
========================================
Files 389 389
Lines 21758 21762 +4
========================================
+ Hits 14447 14451 +4
Misses 6584 6584
Partials 727 727
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
zetaclient/chains/evm/signer/gas_test.go (1)
101-110: Approve changes with a minor suggestion for improvement.The new test case effectively covers the scenario where
gasPriceis less thanpriorityFee, aligning with the PR objective. The assertion correctly validates that thepriorityFeeis adjusted to match thegasPricewhen it exceeds it.To enhance clarity, consider adding a comment explaining the expected behavior:
{ name: "gasPrice is less than priorityFee", cctx: makeCCTX(123_000, gwei(4).String(), gwei(5).String()), + // Expected behavior: priorityFee should be adjusted to match gasPrice assert: func(t *testing.T, g Gas) { assert.False(t, g.isLegacy()) assertGasEquals(t, Gas{ Limit: 123_000, Price: gwei(4), PriorityFee: gwei(4), }, g) }, },This addition would provide immediate context for the test's purpose and expected outcome.
zetaclient/chains/evm/signer/gas.go (1)
103-103: Correct Typographical Error in CommentThere is a typographical error in the comment. The word "help" should be "helps" to ensure proper subject-verb agreement.
Apply this correction:
-// the gas stability pool mechanism help to mitigate this issue +// the gas stability pool mechanism helps to mitigate this issue
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- zetaclient/chains/evm/signer/gas.go (1 hunks)
- zetaclient/chains/evm/signer/gas_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
zetaclient/chains/evm/signer/gas.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/gas_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (1)
zetaclient/chains/evm/signer/gas.go (1)
95-104: Ensure Transaction Validity When AdjustingpriorityFeeAdjusting
priorityFeeto matchgasPriceprevents transaction invalidation due topriorityFeeexceedinggasPrice. However, there is a potential scenario where the adjustedpriorityFeemight not cover thebaseFee, leading to transaction failure. Consider implementing additional checks to ensure thatpriorityFeeis sufficient to cover thebaseFee, or provide fallback mechanisms to handle such cases gracefully.To assess the impact, you could verify whether the
priorityFeemeets the minimum requirements using the following script:✅ Verification successful
Transaction Validity Confirmation
After thorough analysis, it is confirmed that
priorityFeeadjustments appropriately cover thebaseFee, asbaseFeeis consistently zero across all relevant transactions. No additional checks or fallback mechanisms are required at this time.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that adjusted priorityFee covers the baseFee. # Expected: There should be no instances where priorityFee is less than baseFee. # Extract baseFee from relevant transactions rg --type go 'baseFee.*' -A 3 # Find all occurences where priorityFee may be insufficient rg --type go 'priorityFee =.*gasPrice' -A 5Length of output: 37493
Description
Cherry pick changes from #2950 and add one additional comment
Closes: #2951
Summary by CodeRabbit
New Features
gasPriceis less thanpriorityFee.Bug Fixes
Style