feat(zetaclient): disable EIP-1559 [backport] #3237
Conversation
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie and
skosito
as code owners
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files. Key changes include the commenting out of Ethereum transaction receipt verification in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 #3237 +/- ##
===========================================
- Coverage 61.81% 61.80% -0.02%
===========================================
Files 431 431
Lines 30759 30747 -12
===========================================
- Hits 19015 19003 -12
Misses 10886 10886
Partials 858 858
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
e2e/e2etests/test_eth_withdraw.go (2)
42-44: Consider using feature flags instead of commented code.Rather than commenting out the EIP-1559 verification code, consider implementing a feature flag to control this behavior. This would make it easier to enable/disable the feature and maintain test coverage.
- //Skipped due to https://github.com/zeta-chain/node/issues/3221 - //withdrawalReceipt := mustFetchEthReceipt(r, cctx) - //require.Equal(r, uint8(ethtypes.DynamicFeeTxType), withdrawalReceipt.Type, "receipt type mismatch") + if config.IsEIP1559Enabled() { + withdrawalReceipt := mustFetchEthReceipt(r, cctx) + require.Equal(r, uint8(ethtypes.DynamicFeeTxType), withdrawalReceipt.Type, "receipt type mismatch") + }
49-49: Document the temporary nature of the nolint directive.The
nolint:unuseddirective is tied to issue #3221. Consider adding a TODO comment to remove this directive once the issue is resolved.-// nolint:unused // https://github.com/zeta-chain/node/issues/3221 +// TODO: Remove nolint directive once https://github.com/zeta-chain/node/issues/3221 is resolved +// nolint:unused // temporary: https://github.com/zeta-chain/node/issues/3221zetaclient/chains/evm/signer/gas.go (1)
60-61: Document the method's importance despite being temporarily unused.While the method is currently unused due to EIP-1559 being disabled, it's a crucial part of the gas handling logic. Consider adding documentation about when this method would be re-enabled.
-//nolint:unused // https://github.com/zeta-chain/node/issues/3221 +// isLegacy determines whether to use legacy transaction format. +// Currently unused due to EIP-1559 being disabled (https://github.com/zeta-chain/node/issues/3221). +// Will be re-enabled when EIP-1559 support is restored. +//nolint:unused // temporary: https://github.com/zeta-chain/node/issues/3221zetaclient/chains/evm/signer/signer.go (1)
222-222: Consider documenting the ignored chainID parameterThe chainID parameter is now ignored (replaced with
_), but this change might not be immediately clear to other developers.Add a comment explaining why chainID is ignored:
- _ *big.Int, + // chainID is ignored as we're using legacy transactions only + _ *big.Int,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
e2e/e2etests/test_eth_withdraw.go(1 hunks)zetaclient/chains/evm/observer/observer.go(1 hunks)zetaclient/chains/evm/signer/gas.go(1 hunks)zetaclient/chains/evm/signer/sign_test.go(1 hunks)zetaclient/chains/evm/signer/signer.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- zetaclient/chains/evm/signer/sign_test.go
🧰 Additional context used
📓 Path-based instructions (4)
e2e/e2etests/test_eth_withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/observer.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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
zetaclient/chains/evm/observer/observer.go (1)
56-61: LGTM! Clear and informative documentation.
The added documentation for the priorityFeeConfig struct fields clearly explains their purpose and caching behavior, which is essential for understanding the EIP-1559 support detection mechanism.
zetaclient/chains/evm/signer/signer.go (1)
233-254: Verify impact of disabling EIP-1559 support
The code now exclusively uses legacy transactions by commenting out the dynamic fee transaction logic. While this aligns with the PR objective to disable EIP-1559, we should ensure this doesn't affect chains that might require EIP-1559 transactions.
Let's verify if any connected chains require EIP-1559:
Additionally, consider:
- Adding a TODO comment with the issue link
- Making the code change more maintainable for future re-enablement
Consider this refactor to improve maintainability:
-// https://github.com/zeta-chain/node/issues/3221
-//if gas.isLegacy() {
+// TODO: EIP-1559 support temporarily disabled
+// See: https://github.com/zeta-chain/node/issues/3221
+const eip1559Enabled = false
+
+if eip1559Enabled && !gas.isLegacy() {
+ return ethtypes.NewTx(ðtypes.DynamicFeeTx{
+ ChainID: chainID,
+ To: &to,
+ Value: amount,
+ Data: data,
+ GasFeeCap: gas.Price,
+ GasTipCap: gas.PriorityFee,
+ Gas: gas.Limit,
+ Nonce: nonce,
+ }), nil
+}
return ethtypes.NewTx(ðtypes.LegacyTx{
To: &to,
Value: amount,
Data: data,
GasPrice: gas.Price,
Gas: gas.Limit,
Nonce: nonce,
}), nil✅ Verification successful
EIP-1559 disablement appears safe for current chain configurations
Based on the verification results:
- The only chain configuration referencing Ethereum (in
x/observer/types/chain_params.go) doesn't specify EIP-1559 requirements - Tests related to DynamicFeeTx are already skipped with reference to the same issue (Gas pricing: ensure
priorityFee<gasFee[EIP-1559] #3221) - The
isLegacy()function is marked as unused with the same issue reference
The code change aligns with a coordinated effort to disable EIP-1559 across the codebase. The refactoring suggestion in the review would indeed improve maintainability, but the current implementation is acceptable given the temporary nature of the change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for EIP-1559 requirements in chain configurations
fd -t f "chain.*\.go" | xargs rg -A 5 "ChainId.*(?:Ethereum|ETH)"
# Check for any existing dynamic fee transaction usage
rg -A 5 "DynamicFeeTx"
Length of output: 4233
Backport of #3222
Summary by CodeRabbit
Bug Fixes
New Features
priorityFeeConfigstruct with comments indicating support for EIP-1559.Tests
Documentation