test: add fork tests for upgradable contracts #594
Conversation
s2imonovic
changed the title
📝 WalkthroughWalkthroughAdds a Foundry fork-testing framework (BaseForkTest) and three concrete fork tests (ERC20CustodyForkTest, GatewayEVMForkTest, GatewayZEVMForkTest) that configure multi-chain forks and perform per-chain proxy upgrade flows with pre/post-state checks. Also includes mostly formatting changes across contracts and an added TSS_ROLE restriction on GatewayEVM.execute/executeRevert. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Fork Test Runner
participant Base as BaseForkTest
participant VM as Foundry VM (vm)
participant Proxy as On-chain Proxy
participant Impl as New Implementation
Runner->>Base: setUp()
activate Base
Base->>VM: createFork(rpcUrl) for each chain -> forkId
Base->>Runner: _setupChains()
deactivate Base
Runner->>Base: testUpgradeGatewayOnAllChains()
activate Base
loop per ChainConfig
Base->>VM: selectFork(config.forkId)
Base->>Runner: _testUpgradeContract(config)
activate Runner
Runner->>Proxy: read pre-upgrade state (storage)
Runner->>VM: startPrank(admin) [if required]
Runner->>VM: deploy Impl
VM-->>Runner: implAddr
Runner->>Proxy: upgradeToAndCall(implAddr, initData)
Proxy-->>Runner: implementation updated
Runner->>Proxy: read post-upgrade state (verify preserved)
Runner->>VM: stopPrank()
Runner-->>Base: log success/failure
deactivate Runner
end
deactivate Base
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
test/fork/BaseForkTest.sol(1 hunks)test/fork/ERC20CustodyFork.t.sol(1 hunks)test/fork/GatewayEVMFork.t.sol(1 hunks)test/fork/GatewayZEVMFork.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/fork/GatewayEVMFork.t.soltest/fork/ERC20CustodyFork.t.soltest/fork/GatewayZEVMFork.t.soltest/fork/BaseForkTest.sol
🪛 GitHub Actions: Test
test/fork/GatewayEVMFork.t.sol
[error] 1-1: vm.envString: environment variable "ETHEREUM_RPC_URL" not found
test/fork/ERC20CustodyFork.t.sol
[error] 1-1: vm.envString: environment variable "ETHEREUM_RPC_URL" not found
test/fork/GatewayZEVMFork.t.sol
[error] 1-1: vm.envString: environment variable "ETHEREUM_RPC_URL" not found
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: slither
- GitHub Check: generate
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/fork/BaseForkTest.sol (1)
9-15: Guard missing RPC env vars to prevent CI failures.This issue was already flagged in a previous review:
vm.envStringreverts when the environment variable is unset, causing CI failures when RPC URLs are missing. Usevm.envOrwith a default value or checkvm.envExistsfirst, and updatesetUp()to gracefully skip or bail out when required RPC URLs are unavailable.
🧹 Nitpick comments (3)
test/fork/BaseForkTest.sol (3)
59-66: PreferassertNotEqoverassertfor better test output.Using
assertprovides minimal information on failure. Replace withassertNotEq(ethereumForkId, bscForkId, "Fork IDs must differ")(and so on) to get clearer diagnostics showing which fork IDs collided.Apply this diff:
function testForkIdDiffer() public view { - assert(ethereumForkId != bscForkId); - assert(bscForkId != polygonForkId); - assert(polygonForkId != baseForkId); - assert(baseForkId != arbitrumForkId); - assert(arbitrumForkId != avalancheForkId); - assert(avalancheForkId != zetachainForkId); + assertNotEq(ethereumForkId, bscForkId, "Ethereum and BSC fork IDs must differ"); + assertNotEq(bscForkId, polygonForkId, "BSC and Polygon fork IDs must differ"); + assertNotEq(polygonForkId, baseForkId, "Polygon and Base fork IDs must differ"); + assertNotEq(baseForkId, arbitrumForkId, "Base and Arbitrum fork IDs must differ"); + assertNotEq(arbitrumForkId, avalancheForkId, "Arbitrum and Avalanche fork IDs must differ"); + assertNotEq(avalancheForkId, zetachainForkId, "Avalanche and ZetaChain fork IDs must differ"); }
75-80: Consider renaming for clarity.The function name
testUpgradeGatewayOnAllChainsis specific to Gateway contracts, but this is a base class that can test any upgradable contract (as evidenced byERC20CustodyForkTestalso using this base). Consider renaming totestUpgradeContractOnAllChainsfor better clarity.
45-55: Refactor fork creation to avoid requiring all RPC env vars
Eagerly callingvm.createForkfor every supported chain insetUp()forces all sevenRPC_URLenv vars to be set, even if a subclass only needs a subset. Move thosecreateForkcalls into_setupChains()or guard against empty URLs so tests can opt into only the networks they actually use.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/fork/BaseForkTest.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/fork/BaseForkTest.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: generate
- GitHub Check: slither
- GitHub Check: test
There was a problem hiding this comment.
We can now remove these scripts right? https://github.com/zeta-chain/protocol-contracts-evm/tree/develop/scripts/upgrade
| uint256 avalancheForkId; | ||
| uint256 zetachainForkId; | ||
|
|
||
| // Mainnet admins. |
There was a problem hiding this comment.
The test only work for mainnet? We should precise it somewhere. Otherwise can't we make it dependent on the network used?
| assertEq(address(custody.gateway()), gatewayBefore, "Gateway address changed."); | ||
| assertEq(custody.tssAddress(), tssAddressBefore, "TSS address changed."); | ||
| assertEq(custody.supportsLegacy(), supportsLegacyBefore, "Supports legacy changed."); | ||
| console.log("State preserved after upgrade"); |
There was a problem hiding this comment.
Maybe we can also check the the admin is preserved? Same comment for other contracts
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1)
47-56: Action: Align withdraw() Natspec with actual role check (TSS vs WITHDRAWER_ROLE).Natspec says "only TSS address" but the function uses onlyRole(WITHDRAWER_ROLE). Update the @dev to mention WITHDRAWER_ROLE or change the modifier to onlyRole(TSS_ROLE) depending on intended authority. File: test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (withdraw()).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (32)
contracts/evm/ERC20Custody.sol(2 hunks)contracts/evm/GatewayEVM.sol(6 hunks)contracts/evm/Registry.sol(7 hunks)contracts/evm/ZetaConnectorBase.sol(1 hunks)contracts/evm/ZetaConnectorNative.sol(1 hunks)contracts/evm/ZetaConnectorNonNative.sol(2 hunks)contracts/evm/interfaces/IGatewayEVM.sol(3 hunks)contracts/evm/legacy/ZetaConnector.base.sol(2 hunks)contracts/evm/legacy/ZetaConnector.eth.sol(3 hunks)contracts/evm/legacy/ZetaConnector.non-eth.sol(3 hunks)contracts/evm/legacy/ZetaInterfaces.sol(1 hunks)contracts/helpers/BaseRegistry.sol(3 hunks)contracts/helpers/interfaces/IBaseRegistry.sol(3 hunks)contracts/zevm/CoreRegistry.sol(7 hunks)contracts/zevm/GatewayZEVM.sol(5 hunks)contracts/zevm/ZRC20.sol(1 hunks)contracts/zevm/interfaces/IGatewayZEVM.sol(3 hunks)contracts/zevm/interfaces/UniversalContract.sol(2 hunks)contracts/zevm/legacy/ZetaConnectorZEVM.sol(3 hunks)test/CoreRegistry.t.sol(1 hunks)test/ERC20Custody.t.sol(3 hunks)test/GatewayEVM.t.sol(1 hunks)test/GatewayEVMZEVM.t.sol(1 hunks)test/GatewayZEVM.t.sol(2 hunks)test/Registry.t.sol(1 hunks)test/ZRC20.t.sol(1 hunks)test/utils/ReceiverEVM.sol(1 hunks)test/utils/upgrades/ERC20CustodyUpgradeTest.sol(2 hunks)test/utils/upgrades/GatewayEVMUpgradeTest.sol(7 hunks)test/utils/upgrades/GatewayZEVMUpgradeTest.sol(4 hunks)test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol(1 hunks)test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol(2 hunks)
✅ Files skipped from review due to trivial changes (25)
- contracts/evm/interfaces/IGatewayEVM.sol
- contracts/zevm/legacy/ZetaConnectorZEVM.sol
- contracts/evm/ZetaConnectorBase.sol
- contracts/zevm/CoreRegistry.sol
- test/GatewayEVMZEVM.t.sol
- test/CoreRegistry.t.sol
- contracts/evm/ERC20Custody.sol
- contracts/zevm/ZRC20.sol
- test/Registry.t.sol
- test/ZRC20.t.sol
- contracts/evm/legacy/ZetaConnector.non-eth.sol
- contracts/evm/ZetaConnectorNonNative.sol
- contracts/zevm/GatewayZEVM.sol
- contracts/evm/ZetaConnectorNative.sol
- contracts/evm/legacy/ZetaConnector.base.sol
- test/ERC20Custody.t.sol
- test/utils/upgrades/ERC20CustodyUpgradeTest.sol
- test/GatewayZEVM.t.sol
- test/GatewayEVM.t.sol
- contracts/evm/legacy/ZetaConnector.eth.sol
- contracts/helpers/interfaces/IBaseRegistry.sol
- test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol
- contracts/zevm/interfaces/IGatewayZEVM.sol
- contracts/zevm/interfaces/UniversalContract.sol
- test/utils/upgrades/GatewayEVMUpgradeTest.sol
🧰 Additional context used
📓 Path-based instructions (2)
contracts/**
⚙️ CodeRabbit configuration file
Review the Solidity contracts for security vulnerabilities and best practices.
Files:
contracts/helpers/BaseRegistry.solcontracts/evm/Registry.solcontracts/evm/legacy/ZetaInterfaces.solcontracts/evm/GatewayEVM.sol
test/**
⚙️ CodeRabbit configuration file
Review the test files for proper coverage, edge cases, and best practices.
Files:
test/utils/ReceiverEVM.soltest/utils/upgrades/GatewayZEVMUpgradeTest.soltest/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: generate
- GitHub Check: slither
🔇 Additional comments (10)
test/utils/ReceiverEVM.sol (1)
76-83: LGTM! Formatting change only.The function signature has been consolidated to a single line without any functional changes.
contracts/helpers/BaseRegistry.sol (1)
284-284: LGTM! Formatting changes align with PR-wide standardization.These function declarations have been reformatted from multiline to single-line signatures without any semantic changes. The modifications are consistent with the broader formatting effort across the PR and introduce no security, correctness, or behavioral issues.
Also applies to: 298-298, 339-339
contracts/evm/Registry.sol (1)
35-35: LGTM! Formatting changes improve consistency.The function declarations have been reformatted from multi-line to single-line forms without altering signatures, visibility, or behavior. This is consistent with the broader formatting standardization effort across the codebase.
Also applies to: 65-65, 96-96, 110-110, 124-124, 200-200, 222-222
contracts/evm/legacy/ZetaInterfaces.sol (1)
96-98: Formatting-only change; ABI/signature unchanged — OK.Params, visibility, and return type are identical; selector unaffected. Good to merge.
contracts/evm/GatewayEVM.sol (1)
106-106: LGTM: Formatting consolidation improves consistency.The function signature formatting changes (multi-line to single-line) are purely cosmetic and don't alter parameter types, names, visibility, or modifiers. The consolidation improves code consistency across the contract.
Also applies to: 127-127, 246-246, 263-263, 308-308, 426-426
test/utils/upgrades/GatewayZEVMUpgradeTest.sol (4)
149-171: Formatting-only change; behavior unchanged. Confirm event rename scope.Signature reflow looks good. Also, only this ZRC20 withdraw emits WithdrawnV2 while other withdraw/withdrawAndCall still emit Withdrawn—confirm this asymmetry is intentional for the upgrade test.
255-277: LGTM: single-line signature reflow.No ABI/behavior change introduced.
475-486: LGTM: signature reflow only.No logic changes.
503-513: LGTM: signature reflow only.No logic changes.
test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1)
24-32: LGTM: initialize signature reflow.No functional change.
Sherlock AI FindingsThe automated tool identified the following potential security issues in the codebase. Please review the details for each issue in the linked dashboard.
Next Steps: Review the linked issues in the dashboard and address high-severity bugs first. Contact the team if you need assistance. Full report available at: https://ai.sherlock.xyz/runs/e2e6633a-9ed0-4774-9a11-1c38ded4ee9c |
2 participants
Closes: #571
Summary by CodeRabbit
New Features
Tests
Security
Style