feat(zetaclient): disable EIP-1559 #3222
Conversation
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito 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📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes across three files related to transaction signing and gas handling in the Ethereum Virtual Machine (EVM) context. Key modifications include the addition of a linting directive to 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
|
|
Shouldn't the base branch be v23? |
![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 (5)
zetaclient/chains/evm/signer/gas.go (3)
Line range hint
11-12: Consider making gas limits configurableThe
minGasLimitandmaxGasLimitare hardcoded constants. Consider making these configurable to allow for chain-specific adjustments and easier updates.-const ( - minGasLimit = 100_000 - maxGasLimit = 1_000_000 -) +type GasConfig struct { + MinGasLimit uint64 + MaxGasLimit uint64 +}
Line range hint
17-29: Consider documenting the EIP-1559 removal strategyThe current code comments extensively document EIP-1559 support, but the implementation is being disabled. Consider updating this documentation to:
- Explain why EIP-1559 is being disabled
- Document the transition strategy
- Clarify how legacy transactions will be handled going forward
EIP-1559 code is already disabled, but priority fee handling should be removed
The code shows that EIP-1559 transaction creation (
DynamicFeeTx) is already commented out inzetaclient/chains/evm/signer/signer.go, and only legacy transactions are being created. However, the priority fee handling code in the reviewed file is now redundant since it's not being used in transaction creation. This code should be simplified to only handle legacy gas pricing.
- Remove priority fee handling in
zetaclient/chains/evm/signer/gas.gosince it's not used in the legacy transaction creation- Consider removing the
PriorityFeefield from theGasstruct if it's not needed elsewhere🔗 Analysis chain
Line range hint
92-106: Review priority fee handling for EIP-1559 removalThe code still handles priority fees and includes logic for adjusting them. If EIP-1559 is being disabled, this section might need to be simplified to handle only legacy gas pricing.
Let's verify the transaction creation flow:
Let me search for transaction creation and EIP-1559 related code to verify the concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for transaction creation code to verify how gas parameters are used echo "Searching for transaction creation patterns..." ast-grep --pattern 'func newTx($$$) { $$$ }' # Search for priority fee usage echo "Searching for priority fee usage..." rg -A 5 "GasPriorityFee|priorityFee"Length of output: 65387
Script:
#!/bin/bash # Search for transaction creation and EIP-1559 related code echo "Searching for transaction creation and EIP-1559 related code..." rg -A 10 "NewTx|NewTransaction|NewDynamicFeeTx" zetaclient/chains/evm/ # Search for gas price and priority fee usage in transaction creation echo "Searching for gas price and priority fee usage in transaction creation..." ast-grep --pattern 'func $_($$) { $$$ NewTx($$$) $$$ }'Length of output: 2470
zetaclient/chains/evm/signer/signer.go (2)
233-234: Add documentation explaining EIP-1559 disablementWhile the issue link provides context, it would be beneficial to add a detailed comment explaining why EIP-1559 is being disabled and any implications this might have on gas optimization.
// https://github.com/zeta-chain/node/issues/3221 +// EIP-1559 support is temporarily disabled to [explain rationale] +// Impact: This means we'll use legacy transactions which might result in different gas optimization characteristics
222-222: Consider removing the unused parameter completelySince the chainID parameter is no longer used in the legacy transaction flow, consider removing it completely from the function signature rather than marking it as unused with an underscore.
-func newTx( - _ *big.Int, +func newTx( data []byte, to ethcommon.Address, amount *big.Int, gas Gas, nonce uint64, ) (*ethtypes.Transaction, error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
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 (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/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/signer/gas.go (1)
60-61: Verify complete EIP-1559 removal strategy
While the isLegacy() method is marked as unused, its presence alongside the EIP-1559 related fields (PriorityFee) suggests that the EIP-1559 support removal might be incomplete. Consider whether this method and associated fields should be removed entirely to maintain code clarity.
Let's verify other EIP-1559 related code that might need attention:
✅ Verification successful
EIP-1559 removal is correctly implemented
The code shows that EIP-1559 support has been properly disabled:
- All transactions are now using
LegacyTxtype, withDynamicFeeTxcode commented out insigner.go - The
isLegacy()method is correctly marked as unused with a reference to issue Fix all priorityFee edge-cases #3221 - Tests for
DynamicFeeTxare explicitly skipped with the same issue reference - The remaining EIP-1559 related fields (
PriorityFee) are still used but only for gas price calculations, not for transaction type determination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EIP-1559 related patterns in the codebase
echo "Searching for EIP-1559 related code..."
rg -l "EIP-1559|DynamicFeeTx|maxFeePerGas|priorityFee|baseFee"
# Search for transaction type specifications
echo "Searching for transaction type specifications..."
ast-grep --pattern 'type = "0x2"' # EIP-1559 transaction type
Length of output: 6250
Script:
#!/bin/bash
# Let's examine the specific EIP-1559 related code in the gas.go file and its tests
echo "Checking gas.go implementation:"
rg -A 5 "EIP-1559|DynamicFeeTx|maxFeePerGas|priorityFee|baseFee" zetaclient/chains/evm/signer/gas.go
echo "\nChecking gas_test.go:"
rg -A 5 "EIP-1559|DynamicFeeTx|maxFeePerGas|priorityFee|baseFee" zetaclient/chains/evm/signer/gas_test.go
echo "\nChecking for transaction type usage in signer:"
rg -A 5 "LegacyTx|DynamicFeeTx|LegacyTxType|DynamicFeeTxType" zetaclient/chains/evm/signer/
Length of output: 6173
zetaclient/chains/evm/signer/signer.go (1)
235-241: Verify gas price optimization for legacy transactions
With EIP-1559 disabled, ensure that the gas price calculation in legacy transactions is optimized for the current network conditions. Consider implementing a gas price suggestion mechanism based on network congestion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
zetaclient/chains/evm/signer/sign_test.go (1)
Line range hint
62-98: Consider maintaining test coverage with legacy transaction assertionsWhile skipping the DynamicFeeTx test is appropriate given the removal of EIP-1559 support, consider adding equivalent test cases for legacy transactions to maintain coverage of the gas price handling logic.
Consider adding a test case like this:
t.Run("SignOutbound - should correctly handle gas price for legacy tx", func(t *testing.T) { const gasPrice = 3 * 1_000_000_000 // 3 gwei cctx := getCCTX(t) cctx.OutboundParams[0].GasPrice = big.NewInt(gasPrice).String() txData, skip, err := NewOutboundData(ctx, cctx, 123, makeLogger(t)) require.False(t, skip) require.NoError(t, err) tx, err := evmSigner.SignConnectorOnReceive(ctx, txData) require.NoError(t, err) assert.Equal(t, ethtypes.LegacyTxType, int(tx.Type())) assert.Equal(t, int64(gasPrice), tx.GasPrice().Int64()) })zetaclient/chains/evm/signer/signer.go (2)
222-222: Consider removing the unused parameterThe chainID parameter is marked as unused with an underscore but could be removed entirely since it's no longer needed after disabling EIP-1559 support.
-func newTx( - _ *big.Int, +func newTx( data []byte, to ethcommon.Address, amount *big.Int, gas Gas, nonce uint64, ) (*ethtypes.Transaction, error) {
235-242: Clean up commented codeInstead of keeping the commented-out code, consider removing it entirely and tracking the changes in version control. This improves code readability and maintenance.
- // https://github.com/zeta-chain/node/issues/3221 - //if gas.isLegacy() { return ethtypes.NewTx(ðtypes.LegacyTx{ To: &to, Value: amount, Data: data, GasPrice: gas.Price, Gas: gas.Limit, Nonce: nonce, }), nil - //} - // - //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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
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/gas.go
🧰 Additional context used
📓 Path-based instructions (2)
zetaclient/chains/evm/signer/sign_test.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/signer/sign_test.go (1)
Line range hint 1-98: Test structure and coverage are well-maintained
The overall test structure is robust with comprehensive coverage of signing operations, proper error handling, and consistent test patterns. The changes maintain this quality while appropriately handling the EIP-1559 deprecation.
zetaclient/chains/evm/signer/signer.go (1)
233-254: Verify gas price implications of disabling EIP-1559
The change to use only legacy transactions by removing EIP-1559 support might affect gas price optimization. Legacy transactions use a simpler but less efficient gas pricing model.
Run the following script to analyze gas price patterns in the codebase:
Consider:
- Adding a comment explaining why EIP-1559 is disabled
- Documenting the gas price strategy for legacy transactions
- Monitoring transaction confirmation times after this change
✅ Verification successful
Let me gather more information about the gas price implementation and EIP-1559 support in the codebase.
EIP-1559 support temporarily disabled for stability and consistency
The change to use only legacy transactions is an intentional stability measure, as evidenced by issue #3221 referenced in the code. The EIP-1559 implementation exists but is currently commented out to ensure consistent gas pricing behavior across different EVM chains.
Key findings:
- The
isLegacy()function is marked with//nolint:unusedindicating a temporary state - Test cases for DynamicFeeTx are skipped with explicit reference to issue Fix all priorityFee edge-cases #3221
- The codebase maintains the EIP-1559 code structure for future re-enablement
Recommendations:
- Add a comment explaining the temporary nature of legacy-only transactions:
// Currently using legacy transactions only for stability across EVM chains
// EIP-1559 support is temporarily disabled - see issue #3221- Consider adding configuration flags to enable EIP-1559 per chain when the stability issues are resolved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for gas price calculations and EIP-1559 related code
rg -A 3 "GasPrice|GasFee|GasTip"
# Look for any remaining EIP-1559 related code
rg -l "DynamicFeeTx|GasFeeCap|GasTipCap"
Length of output: 151144
Script:
#!/bin/bash
# Check for EIP-1559 related code and gas price patterns in the codebase
rg -A 3 "isLegacy|DynamicFeeTx|GasFeeCap|GasTipCap" zetaclient/chains/evm/signer/
# Look for any gas price related comments or documentation
rg -A 3 "https://github.com/zeta-chain/node/issues/3221" zetaclient/
Length of output: 5409
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v23 #3222 +/- ##
===============================================
- Coverage 62.69% 62.63% -0.06%
===============================================
Files 425 425
Lines 30231 30236 +5
===============================================
- Hits 18954 18939 -15
- Misses 10437 10461 +24
+ Partials 840 836 -4
|
swift1337
force-pushed
the
feat/disable-eip-1559
branch
from
892a488
to
e38aff0
Compare
With recent changes, all EVMs should have |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
swift1337
force-pushed
the
feat/disable-eip-1559
branch
from
e38aff0
to
294af38
Compare
| @@ -95,7 +95,12 @@ func NewObserver( | |||
| evmJSONRPC: evmJSONRPC, | |||
| outboundConfirmedReceipts: make(map[string]*ethtypes.Receipt), | |||
| outboundConfirmedTransactions: make(map[string]*ethtypes.Transaction), | |||
| priorityFeeConfig: priorityFeeConfig{}, | |||
| priorityFeeConfig: priorityFeeConfig{ | |||
There was a problem hiding this comment.
Just to confirm, to prevent:
- 1 voting new priorityFee?
- 2 Using EIP1559 for the transactions?
If we have 1 but not 2, isn't some possible issue of using eip1559 while priorityFee is not updated?
There was a problem hiding this comment.
checked bool means we've already determined whether this chain supports priority fee. from the second time and on it will directly use supported bool
Related:
Summary by CodeRabbit