Conversation
|
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 📝 WalkthroughWalkthroughThis change set removes all stateful precompile contract tests and their associated Solidity contracts, Go bindings, and helper code from the end-to-end (E2E) test suite. All files, interfaces, and test logic related to the bank, staking, distribute, and prototype precompiles—both direct and through contract calls—are deleted. The corresponding Go bindings, ABI, and JSON artifacts are also removed, along with test registration and helper functions. Changes
Sequence Diagram(s)Omitted due to the nature of the changes (mass file and feature removal, no new control flow introduced). Estimated code review effort🎯 5 (Critical) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Looks good to me.
Huge work.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (2)
e2e/runner/setup_zevm.go (1)
303-305: Correct ZEVM gateway address in TestDAppV2 deploymentIn
e2e/runner/setup_zevm.go, the call toDeployTestDAppV2is still usingr.GatewayEVMAddr. For ZEVM setup it should passr.GatewayZEVMAddrinstead.• Location:
e2e/runner/setup_zevm.go, around line 303
• Change the final parameter of the deploy call accordingly:- testDAppV2Addr, txTestDAppV2, _, err := testdappv2.DeployTestDAppV2( - r.ZEVMAuth, r.ZEVMClient, true, r.GatewayEVMAddr, - ) + testDAppV2Addr, txTestDAppV2, _, err := testdappv2.DeployTestDAppV2( + r.ZEVMAuth, r.ZEVMClient, true, r.GatewayZEVMAddr, + ) require.NoError(r, err)This ensures the ZEVM gateway is correctly targeted rather than reusing the EVM gateway.
contrib/localnet/orchestrator/start-zetae2e.sh (1)
10-12: Ensure the trap also fires on normal script terminationThe current trap only listens to
SIGINTandSIGTERM. When the script finishes successfully (e.g.,exit 0), orphaned background jobs will survive.
AddEXITto the trap list to guarantee cleanup in all exit paths.-trap 'kill -- -$$' SIGINT SIGTERM +trap 'kill -- -$$' SIGINT SIGTERM EXIT
♻️ Duplicate comments (6)
cmd/zetae2e/local/monitor_priority_txs.go (1)
115-115: LGTM: Cosmos EVM module migration implemented correctly.The type URL string has been properly updated from
/ethermint.evm.v1.MsgEthereumTxto/cosmos.evm.vm.v1.MsgEthereumTx, which aligns with the broader codebase migration to Cosmos EVM modules.go.mod (2)
297-298: Core cosmos/evm dependency added successfullyThe addition of
github.com/cosmos/evm v1.0.0-rc2is the foundation for migrating from Ethermint to Cosmos EVM modules. Note that this is a release candidate version.
417-420: Replace directives properly configured for EVM migrationThe replace directives correctly point to:
- ZetaChain's fork of cosmos/evm for custom modifications
- Cosmos's fork of go-ethereum for EVM compatibility
This is a standard approach for maintaining project-specific modifications.
app/ante/sig_verification.go (1)
21-47: Signature verification gas consumer correctly implementedThe implementation properly handles different public key types:
- Fixed gas cost for secp256k1 variants
- Delegates multisig handling to a separate function
- Falls back to SDK default for other types
The separation of multisig handling avoids the recursion concern mentioned in past reviews.
cmd/zetacored/root.go (2)
69-69: Address TODO comments regarding default EVM chain ID usage.These TODO comments indicate uncertainty about using the default EVM chain ID for the temporary app instance. Since this temporary app is only used to obtain autoCLI options and is not persisted, using the default value is acceptable. Consider removing these TODOs or documenting why the default is appropriate in this context.
Also applies to: 73-73
280-280: Resolve key format decision for Cosmos EVM commands.The TODO questions whether to default to Ethereum keys. Based on past review feedback, it's recommended to maintain Cosmos address format for Cosmos commands to ensure consistency with existing behavior.
🧹 Nitpick comments (32)
changelog.md (1)
12-12: Entry ordering breaks numerical chronology
The new PR link#3991is inserted after#4063, interrupting the ascending order used for the surrounding items (3961‒4063). Re-order the list (or document the intended sorting rule) to preserve quick visual scanning of recent additions.contrib/rpcimportable/go.mod (1)
8-13: Comment now contradicts reality
Lines 8-10 state that “we should not tolerate any other replacements”, yet additionalreplacedirectives are introduced below. Update the comment to reflect the new, sanctioned exceptions (Cosmos Geth & EVM fork) so future maintainers are not mis-led.cmd/zetae2e/local/evm.go (1)
98-98: Clarify the TODO comment and add tracking.The comment indicates temporary disablement due to setup differences, but the "zeta" test goroutine still executes. Please clarify what exactly is disabled and consider adding an issue reference for tracking this temporary change.
Consider improving the comment:
- // TODO evm: tmp comment out because of setup difference between zetae2e-ante and zetae2e + // TODO(issue-link): Temporarily disabled due to setup differences between zetae2e-ante and zetae2e binaries + // Related to upgrade testing infrastructure - see PR #3991cmd/zetacored/config/activators.go (1)
11-15: Consider using decimal notation for EIP identifiers.The octal notation (
0o000,0o001,0o002) for EIP identifiers may cause confusion since EIPs are conventionally referenced in decimal. Consider using decimal notation for better readability and consistency with EIP standards.-var CosmosEVMActivators = map[int]func(*vm.JumpTable){ - 0o000: eips.Enable0000, - 0o001: eips.Enable0001, - 0o002: eips.Enable0002, -} +var CosmosEVMActivators = map[int]func(*vm.JumpTable){ + 0: eips.Enable0000, + 1: eips.Enable0001, + 2: eips.Enable0002, +}e2e/runner/upgrade_v33.go (1)
13-21: Consider extracting duplicate receipt validation logic.Both
ensureReceiptEVMandensureReceiptZEVMshare identical logic except for the client. Consider extracting a generic helper to reduce duplication.+func ensureReceipt(r *E2ERunner, client *ethclient.Client, tx *ethtypes.Transaction, failMessage string) { + receipt := utils.MustWaitForTxReceipt(r.Ctx, client, tx, r.Logger, r.ReceiptTimeout) + msg := "tx %s receipt status is not successful: %s" + require.Equal( + r, + ethtypes.ReceiptStatusSuccessful, + receipt.Status, + msg, + receipt.TxHash.String(), + failMessage, + ) +} + ensureReceiptEVM := func(tx *ethtypes.Transaction, failMessage string) { - receipt := utils.MustWaitForTxReceipt(r.Ctx, r.EVMClient, tx, r.Logger, r.ReceiptTimeout) - msg := "tx %s receipt status is not successful: %s" - require.Equal( - r, - ethtypes.ReceiptStatusSuccessful, - receipt.Status, - msg, - receipt.TxHash.String(), - failMessage, - ) + ensureReceipt(r, r.EVMClient, tx, failMessage) } ensureReceiptZEVM := func(tx *ethtypes.Transaction, failMessage string) { - receipt := utils.MustWaitForTxReceipt(r.Ctx, r.ZEVMClient, tx, r.Logger, r.ReceiptTimeout) - msg := "tx %s receipt status is not successful: %s" - require.Equal( - r, - ethtypes.ReceiptStatusSuccessful, - receipt.Status, - msg, - receipt.TxHash.String(), - failMessage, - ) + ensureReceipt(r, r.ZEVMClient, tx, failMessage) }Also applies to: 25-33
app/eips/eips.go (2)
7-10: Document the rationale for constant values.The constants
MultiplierandSstoreConstantGaslack documentation explaining their specific values. Consider adding comments that explain whyMultiplieris set to 10 andSstoreConstantGasto 500, referencing the relevant EIP specifications.var ( - Multiplier = uint64(10) - SstoreConstantGas = uint64(500) + // Multiplier increases gas costs for CREATE, CREATE2, and CALL opcodes by 10x + // as specified in the corresponding EIP implementations + Multiplier = uint64(10) + // SstoreConstantGas sets a fixed gas cost for SSTORE operations + // as defined in the EIP specification + SstoreConstantGas = uint64(500) )
16-22: Consider adding input validation for robustness.The function directly modifies the JumpTable without validating that the input parameter is non-nil. While this may be acceptable in the current controlled usage context, adding a nil check would improve robustness.
func Enable0000(jt *vm.JumpTable) { + if jt == nil { + return + } currentValCreate := jt[vm.CREATE].GetConstantGas() jt[vm.CREATE].SetConstantGas(currentValCreate * Multiplier)e2e/txserver/zeta_tx_server.go (1)
120-120: Improve the unsafe conversion comment.The
#nosec G115comment could be more descriptive about why this conversion is safe in this specific context.- cdc, reg := newCodec(uint64(zetachain.ChainId)) //#nosec G115 chain id won't exceed uint64 + cdc, reg := newCodec(uint64(zetachain.ChainId)) //#nosec G115 -- ZetaChain.ChainId is int64, safe to convert to uint64Makefile (1)
354-372: Upgrade configuration updates look goodThe changes properly update the upgrade test configuration:
- OLD_VERSION correctly updated to v32.0.2
- NODE_VERSION and NODE_COMMIT build args properly passed
- Trailing backslash fix prevents build errors
Consider keeping the
.PHONYdeclaration immediately before the target for better organization:+.PHONY: zetanode-upgrade zetanode-upgrade: e2e-imagese2e/runner/setup_evm.go (1)
114-117: Consider consolidating deployment methods to reduce function parameter passing.The
ensureTxReceiptfunction is passed as a parameter to both deployment methods. Since these methods are part of the same runner struct, consider makingensureTxReceipta method of the runner to avoid parameter passing and improve cohesion.-func (r *E2ERunner) DeployTestDAppV2(ensureTxReceipt func(tx *ethtypes.Transaction, failMessage string)) { +func (r *E2ERunner) DeployTestDAppV2() { + ensureTxReceipt := func(tx *ethtypes.Transaction, failMessage string) { + receipt := utils.MustWaitForTxReceipt(r.Ctx, r.EVMClient, tx, r.Logger, r.ReceiptTimeout) + r.requireTxSuccessful(receipt, failMessage) + }cmd/zetae2e/local/local.go (1)
319-321: Add documentation for the SUI ZRC20 balance assertion logic.The condition checking
!testSui || deployerRunner.IsRunningTssMigration()before asserting the balance is not immediately clear. Consider adding a comment explaining why the balance should be zero after upgrade v32.0.2.+ // After upgrade v32.0.2, the SUI ZRC20 balance in the Gas Stability Pool should be zero + // unless we're running TSS migration tests (which may have different balance expectations) balance, err := deployerRunner.SUIZRC20.BalanceOf(&bind.CallOpts{}, fungibletypes.GasStabilityPoolAddressEVM())contrib/localnet/orchestrator/start-zetae2e.sh (2)
41-58: Consider exponential back-off when polling for zetacored versionA fixed 1 s delay for up to 10 retries may still hit the node before it’s ready, yet wastes time when the node is slow. Using exponential back-off (e.g., 1 s, 2 s, 4 s …) shortens happy-path latency while remaining robust.
No functional breakage, but improves resilience and CI run-time.
190-196: Suppress ShellCheck SC2155 warnings or split assignmentInline command substitution inside assignment (
relayer=$(config_str ...)) shadows the command’s exit status and triggers SC2155. Either:
- Split declaration and assignment:
local relayer relayer=$(config_str '.observer_relayer_accounts.relayer_accounts[0].solana_address')
- Or explicitly suppress per line with
# shellcheck disable=SC2155.Keeps the script free of static-analysis noise.
docs/cli/zetacored/cli.md (17)
24-27: Replace hard-tabs with spaces in the top-level bulletThe newly-added bullet still contains a hard tab character, triggering MD010 and breaking visual alignment in many Markdown renderers.
-* [zetacored comet](#zetacored-comet) - CometBFT subcommands +* [zetacored comet](#zetacored-comet) - CometBFT subcommands
214-447: Missing fenced-code languages & hard-tabs across the entire “comet” sectionDozens of code blocks lack a language identifier (MD040) and every bullet continues to use hard tabs (MD010).
Please:
- Prefix each triple-backtick with
bash(these blocks are CLI invocations, not Go).- Replace the tab after every
*with two spaces for consistent indentation.This cleans up lint errors and renders the syntax-highlighted commands correctly.
1310-1311: Eliminate hard-tab in keys menuSame MD010 violation as earlier.
-* [zetacored keys list-key-types](#zetacored-keys-list-key-types) - List all key types +* [zetacored keys list-key-types](#zetacored-keys-list-key-types) - List all key types
1583-1616:keys list-key-typesdocs – add language tag & untabifyThe synopsis, examples and inherited-options blocks miss
bashand still contain hard tabs. Fix as in previous comments.
5104-5108: Add language for evidence example-``` +```bash zetacored query evidence evidence DF0C23E8634E...
5148-5152: Add language for list example-``` +```bash zetacored query evidence list --page=2 --page-limit=50
5270-5280: Add language tag toaccountsynopsis block-``` +```bash zetacored query evm account ADDRESS [flags]
9518-9528: Snapshots dump example – add language tag-``` +```bash zetacored snapshots dump [height] [format] [flags]
9543-9557: Snapshots export – language tag & hard-tabSame fix pattern as earlier.
9572-9585: Snapshots list – addbashand remove tabConsistent with other snapshot commands.
9600-9614: Snapshots load – language tag requiredAdd
bashto fenced code block.
9628-9646: Snapshots restore – language tag missingAdd
bashto fenced code block.
9814-9815: Remove tab in status section bullet-* [zetacored](#zetacored) - Zetacore Daemon (server) +* [zetacored](#zetacored) - Zetacore Daemon (server)
9854-9858: Hard-tab in tx menuReplace the tab after “evm”.
12530-12533: Untabify bullet-* [zetacored tx evm send](#zetacored-tx-evm-send) - Send funds from one account to another. +* [zetacored tx evm send](#zetacored-tx-evm-send) - Send funds from one account to another.
16854-16855: Untabify bullet in upgrade menuSwap tab for space to kill MD010.
16921-16970: Cancel-upgrade-proposal docs: add language tags & remove tabsApply the same fixes:
bashon fenced blocks, spaces instead of tabs in bullets.cmd/zetacored/root.go (2)
378-392: Dynamic EVM chain ID resolution implemented correctly.The implementation properly:
- Extracts chain ID from app options
- Converts to ZetaChain format
- Passes the chain ID to the app constructor
The TODO comment on line 391 appears redundant as the code already correctly uses the EVM chain ID.
- uint64(zetachain.ChainId), //#nosec G115 chain id won't exceed uint64 // TODO evm: evm chain id? + uint64(zetachain.ChainId), //#nosec G115 chain id won't exceed uint64
412-431: Consistent chain ID handling in app export.The implementation maintains consistency with the
newAppmethod for chain ID extraction. The TODO comment on line 430 is redundant.- uint64(zetachain.ChainId), //#nosec G115 chain id won't exceed uint64 // TODO evm: evm chain id? + uint64(zetachain.ChainId), //#nosec G115 chain id won't exceed uint64
swift1337
left a comment
swift1337
left a comment
There was a problem hiding this comment.
Left some minor comments/notes.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| // Copyright 2023 ZetaChain | ||
| // modified to exclude gentx transaction type from the min gas price check | ||
| package ante | ||
|
|
There was a problem hiding this comment.
EthMinGasPriceDecorator and EthMempoolFeeDecorator decorators are both unused and can be removed.
| func (b *Backend) FeeHistory( | ||
| userBlockCount ethmath.HexOrDecimal64, // number blocks to fetch, maximum is 100 | ||
| userBlockCount math.HexOrDecimal64, // number blocks to fetch, maximum is 100 | ||
| lastBlock rpc.BlockNumber, // the block to start search , to oldest | ||
| rewardPercentiles []float64, // percentiles to fetch reward |
There was a problem hiding this comment.
rewardPercentiles can be a very large slice containing many elements, and if so, it can result in a panic when creating the slice in line 205. Limit the number of elements to e.g. 100.
| blocks := int64(userBlockCount) | ||
| maxBlockCount := int64(b.cfg.JSONRPC.FeeHistoryCap) | ||
|
|
||
| blocks := int64(userBlockCount) // #nosec G115 -- checked for int overflow already |
There was a problem hiding this comment.
Contrary to the comment, the value is not checked if it can overflow.
The value can be set larger than what fits into int64 -> overflows to negative value. As a result, it will panic when creating the slice in line 202
node/rpc/backend/chain_info.go
Line 202 in 1efa712
berndartmueller
left a comment
berndartmueller
left a comment
There was a problem hiding this comment.
In app/app.go#L180:
emissionstypes.UndistributedTSSRewardsPool: nilFor completeness, add feemarkettypes.ModuleName: nil
In app/app.go#L189:
govtypes.ModuleName: true,Add evmtypes.ModuleName and feemarkettypes.ModuleName to blockedReceivingModAcc. No direct implications, but still good to have.
In app/modules.goL#247:
evmtypes.ModuleName,According to the Cosmos EVM docs (https://evm.cosmos.network/integrate), this should be before feemarkettypes.ModuleName in orderEndBlockers. Cannot find an impact, though.
| options.AccountKeeper, | ||
| options.FeeMarketKeeper, | ||
| options.EvmKeeper, | ||
| options.MaxTxGasWanted, |
There was a problem hiding this comment.
Currently it does not use the value of srvflags.EVMMaxTxGasWanted, instead it resolves to TransactionGasLimit uint64 = 10_000_000
There was a problem hiding this comment.
this was intentional it seems https://github.com/zeta-chain/node/pull/3344/files so we can keep it like this
| feemarket.NewAppModule(app.FeeMarketKeeper, feeSs), | ||
| evm.NewAppModule(app.EvmKeeper, app.AccountKeeper, evmSs), | ||
| feemarket.NewAppModule(app.FeeMarketKeeper), | ||
| vm.NewAppModule(app.EvmKeeper, app.AccountKeeper, app.AccountKeeper.AddressCodec()), |
There was a problem hiding this comment.
Is it intentional that the erc20 module is not added here?
There was a problem hiding this comment.
not added because we are not using erc20 module atm directly, just as part of evm keeper constructor, but do you think it should be added regardless?
5 participants
Description
diff for evm fork to review together: zeta-chain/evm@main...zeta-chain:evm:integration-fixes-latest
areas of concerns:
...
Related
Dependencies
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation