test: add connector fund migration e2e test using contracts only #3976
test: add connector fund migration e2e test using contracts only #3976
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 update introduces a new end-to-end test suite for V2 connector migration, involving Makefile, workflow, and test code changes. It adds contract deployment, migration logic, and configuration for the new connector contract, with supporting helpers and workflow automation. Minor dependency, error message, and import adjustments are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Makefile
participant Localnet
participant E2E Test Runner
participant Contracts
Developer->>GitHub Actions: Push with V2_CONNECTOR_MIGRATION_TESTS label
GitHub Actions->>Makefile: Run start-v2-connector-migration-test
Makefile->>Localnet: Launch upgrade test environment
Localnet->>E2E Test Runner: Execute migration tests (flag enabled)
E2E Test Runner->>Contracts: Deploy V2 Connector, migrate funds, verify
E2E Test Runner->>Contracts: Run V2 Zeta deposit test
Contracts-->>E2E Test Runner: Return results
E2E Test Runner-->>GitHub Actions: Report test outcome
Suggested labels
Suggested reviewers
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. 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
|
github-actions
bot
added
ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3976 +/- ##
========================================
Coverage 64.82% 64.82%
========================================
Files 474 474
Lines 34809 34808 -1
========================================
Hits 22566 22566
+ Misses 11192 11191 -1
Partials 1051 1051
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
go.mod (1)
3-3:⚠️ Potential issueInvalid
godirective
go 1.22.11is not allowed; the directive must be justmajor.minor(e.g.,go 1.22). Tooling such asgo vet,go test, and CI pipelines will fail.-go 1.22.11 +go 1.22
♻️ Duplicate comments (1)
e2e/runner/runner.go (1)
271-293: Address copy omission (see previous comment)
ConnectorNativeAddris missing from the bulk-copy block, leading to zero-value propagation.
Once the fix above is applied, include the copy here as well.
🧹 Nitpick comments (12)
changelog.md (1)
21-23: Nit: maintain section orderingElsewhere in the changelog “Tests” follows “Refactor”. Consider moving the new entry accordingly to preserve the document’s structure.
e2e/config/config.go (1)
169-178: Consider follow-up validation for the newconnector_nativefieldThe extra
ConnectorNativeAddrfield is necessary, but nothing in this file (or itsValidatemethod) ensures the value is present / well-formed when V2 flows are enabled.
If the address is mandatory for certain test modes, add a quick sanity check similar to the account validations to fail fast when the YAML omits it.@@ func (c Config) Validate() error { // existing policy account checks… + + // validate mandatory contract addresses when native connector tests run + if os.Getenv("E2E_ENABLE_V2_CONNECTOR") == "true" && + !ethcommon.IsHexAddress(c.Contracts.EVM.ConnectorNativeAddr.String()) { + return errors.New("invalid or missing connector_native address in config") + }e2e/runner/legacy_zevm.go (1)
54-57: Race-window remains after the paused checkThe added guard prevents deposits while the connector is paused – good catch.
Two minor considerations:
The state could change between
Paused()andSend(). Using the contract’s built-in revert on paused state already protects you, so the pre-check mainly saves one failed tx. Acceptable for tests, but document this intent.
bind.CallOpts{}uses a background context; if tests hang, consider wiringCtx:-paused, err := r.ConnectorEth.Paused(&bind.CallOpts{}) +paused, err := r.ConnectorEth.Paused(&bind.CallOpts{Context: r.Ctx})Not a blocker, just a robustness tweak.
x/fungible/keeper/evm_gateway.go (1)
198-207: Doc-string no longer matches the invoked contract methodThe implementation now calls
depositAndRevert0, which (per the comment abovedepositAndCallZRC20) is the ZRC20 variant.
However, the header comment forCallDepositAndRevertstill describes the genericdepositAndRevertsignature. This will quickly become misleading for anyone reading godocs or grepping the codebase.-// CallDepositAndRevert calls the depositAndRevert function on the gateway contract +// CallDepositAndRevert calls the depositAndRevert0 (ZRC20 variant) function on the gatewayPlease update the comment (and, if needed, the surrounding explanation) to reflect the new selector.
Optionally consider renaming the Go method toCallDepositAndRevertZRC20to keep the vocabulary consistent withCallDepositAndCallZRC20.Also applies to: 239-239
e2e/runner/runner.go (1)
32-35: Alias typo in import ‑ will confuse future readers
zetaconnnectornative(three “n”) is very likely an unintended typo. While Go allows any alias, deviating from the package name without a clear reason reduces readability.- zetaconnnectornative "github.com/zeta-chain/protocol-contracts/pkg/zetaconnectornative.sol" + zetaconnectornative "github.com/zeta-chain/protocol-contracts/pkg/zetaconnectornative.sol"Makefile (1)
408-413: Target message & env flags inconsistent with intentThe new target is specifically for the V2 connector migration test, yet it re-uses the log message from the “light upgrade” target, which can confuse CI logs.
- @echo "--> Starting light upgrade test (no ZetaChain state populating before upgrade)" + @echo "--> Starting V2 connector migration test (light upgrade, no pre-state)"Consider also appending
--skip-regulartoE2E_ARGS(as done in other focused test targets) unless running the regular suite is intentional.e2e/e2etests/helpers.go (1)
41-57: Helper should mark itself as testing helperTo keep stack traces clean, test helpers generally call
t.Helper().
E2ERunnerimplements the required methods, so you can signal Testify to skip this frame:func requireCctxStatus( r *runner.E2ERunner, expectedStatus crosschaintypes.CctxStatus, cctx *crosschaintypes.CrossChainTx, ) { + r.Helper() // mark helper if expectedStatus == cctx.CctxStatus.Status { return }(If
E2ERunnerdoes not yet exposeHelper(), add a trivialfunc (r *E2ERunner) Helper() {}.).github/workflows/e2e.yml (1)
128-130: Missing final newline triggers YAML-lint warning
yamllintcomplains about “no new line character at the end of file”.
Add a single\nafter the last line to silence the linter and avoid tooling noise.e2e/e2etests/test_v2_zeta_deposit.go (1)
33-35: Left-over TODO commentThe explanatory comment is fine, but spelling/spacing is off and the link is already present in the commit message. Tighten it or remove to keep the file crisp.
e2e/runner/setup_evm.go (1)
107-108: Duplicate receipt check – second call is redundant
txCustodyis already checked for success at line 80.
The second invocation at lines 107-108 is a no-op that adds noise and can be safely removed.- ensureTxReceipt(txCustody, "ERC20Custody deployment failed")cmd/zetae2e/local/local.go (1)
576-592: Graceful failure path missing context cancellationOn migration-test failure the process exits immediately (
os.Exit(1)), but the surrounding goroutines (block-monitor, stress tests, etc.) keep running until forcibly killed.
InvokedeployerRunner.CtxCancel(err)beforeos.Exitto release resources and have a single shutdown path.- logger.Print("❌ %v", err) - logger.Print("❌ v2 connector migration failed") - os.Exit(1) + deployerRunner.CtxCancel(err) + logger.Print("❌ %v", err) + logger.Print("❌ v2 connector migration failed") + os.Exit(1)e2e/e2etests/test_migrate_connector_funds.go (1)
103-119: Dead code –updateChainParamsis never invokedThe helper is defined but not used; retaining it adds cognitive overhead.
Either remove it or call it after a successful migration.-// updateChainParams updates the chain parameters to use the new connector address -func updateChainParams(…) - … -}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.github/workflows/e2e.yml(5 hunks)Makefile(2 hunks)changelog.md(1 hunks)cmd/zetae2e/config/config.go(1 hunks)cmd/zetae2e/config/contracts.go(2 hunks)cmd/zetae2e/local/local.go(4 hunks)contrib/localnet/docker-compose.yml(2 hunks)contrib/rpctest/main.go(1 hunks)e2e/config/config.go(1 hunks)e2e/e2etests/e2etests.go(4 hunks)e2e/e2etests/helpers.go(2 hunks)e2e/e2etests/test_erc20_withdraw_and_call_revert_with_call.go(1 hunks)e2e/e2etests/test_migrate_connector_funds.go(1 hunks)e2e/e2etests/test_v2_zeta_deposit.go(1 hunks)e2e/runner/evm.go(2 hunks)e2e/runner/legacy_setup_evm.go(1 hunks)e2e/runner/legacy_zevm.go(2 hunks)e2e/runner/runner.go(2 hunks)e2e/runner/setup_evm.go(3 hunks)go.mod(2 hunks)pkg/constant/constant.go(0 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(2 hunks)x/fungible/keeper/evm_gateway.go(1 hunks)zetaclient/chains/evm/observer/v2_inbound.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/constant/constant.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
contrib/rpctest/main.gocmd/zetae2e/config/config.goe2e/runner/legacy_setup_evm.gozetaclient/chains/evm/observer/v2_inbound.goe2e/e2etests/test_erc20_withdraw_and_call_revert_with_call.goe2e/config/config.gox/fungible/keeper/evm_gateway.goe2e/runner/legacy_zevm.goe2e/e2etests/helpers.gocmd/zetae2e/config/contracts.goe2e/e2etests/test_v2_zeta_deposit.gocmd/zetae2e/local/local.goe2e/runner/runner.goe2e/runner/setup_evm.goe2e/runner/evm.gox/crosschain/keeper/cctx_orchestrator_validate_outbound.goe2e/e2etests/e2etests.goe2e/e2etests/test_migrate_connector_funds.go
🧬 Code Graph Analysis (6)
cmd/zetae2e/config/config.go (1)
e2e/config/config.go (3)
Contracts(128-134)EVM(168-178)DoubleQuotedString(27-27)
e2e/e2etests/helpers.go (2)
e2e/runner/runner.go (1)
E2ERunner(81-213)testutil/sample/crosschain.go (1)
CrossChainTx(266-280)
cmd/zetae2e/config/contracts.go (1)
e2e/config/config.go (2)
Contracts(128-134)EVM(168-178)
e2e/e2etests/test_v2_zeta_deposit.go (3)
e2e/runner/runner.go (1)
E2ERunner(81-213)e2e/utils/parsing.go (1)
ParseBigInt(27-32)e2e/utils/zetacore.go (1)
WaitCctxMinedByInboundHash(53-65)
cmd/zetae2e/local/local.go (1)
e2e/e2etests/e2etests.go (2)
TestMigrateConnectorFundsName(203-203)TestLegacyZetaDepositName(261-261)
e2e/e2etests/e2etests.go (3)
e2e/runner/e2etest.go (2)
NewE2ETest(41-58)ArgDefinition(88-91)e2e/e2etests/test_v2_zeta_deposit.go (1)
TestV2ZetaDeposit(14-35)e2e/e2etests/test_migrate_connector_funds.go (1)
TestMigrateConnectorFunds(17-35)
🪛 YAMLlint (1.37.1)
.github/workflows/e2e.yml
[error] 389-389: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (12)
contrib/rpctest/main.go (1)
20-21: Consistent import style – looks goodDropping the explicit alias in favour of the package’s own name keeps imports uniform and removes an unnecessary indirection. No further action required.
zetaclient/chains/evm/observer/v2_inbound.go (1)
176-181: Comment removal OKBehaviour is unchanged;
coinTypeis still resolved correctly based onevent.Asset.e2e/runner/legacy_setup_evm.go (1)
11-12: Alias dropped – clean importMatching the package’s declared name (
zetaeth) eliminates redundant aliasing and keeps usage intact.e2e/e2etests/test_erc20_withdraw_and_call_revert_with_call.go (1)
43-43: Refactor aligns tests – nice consolidationSwitching to
requireCctxStatuscentralises status assertions and improves readability/re-use across tests. No issues spotted.cmd/zetae2e/config/config.go (1)
78-80: Symmetry check: make suresetContractsFromConfigpopulates the runnerYou export
ConnectorNativeAddrhere, but the inverse path (setContractsFromConfig) must also assignr.ConnectorNativeAddrwhen the runner is initialised from an existing config. Otherwise, subsequent tests that rely on the address will receive a zero address and silently mis-behave.
Please verify that file includes:r.ConnectorNativeAddr, _ = conf.Contracts.EVM.ConnectorNativeAddr.AsEVMAddress()and that contract bindings are initialised accordingly.
contrib/localnet/docker-compose.yml (1)
71-76: Profiles broadened – no blocking concernsAdding the
allprofile to extra validator / client services is harmless and improves composability of docker-compose invocations. Ports, volumes and health-checks remain unchanged, so nothing else to flag.Also applies to: 89-94, 181-182, 198-199
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)
240-241: 👍 Better error propagationPassing the actual
errinto the JSON error message and clarifying the wrap text greatly improves observability. No further action required.Also applies to: 433-434
cmd/zetae2e/config/contracts.go (1)
170-179: ConnectorNative wiring looks solidThe new
ConnectorNativeaddress parsing and contract binding are consistent with the existing pattern used for the other EVM contracts. Nothing to flag here.e2e/e2etests/e2etests.go (2)
285-295:v2_zeta_deposittest is always registered despite the “not-used” commentThe block is commented as not used yet the test is still inserted into
AllE2ETests.
If the protocol logic is indeed incomplete, registering it un-conditionally will cause CI failures once the test is selected by any target (e.g.default-test).Consider one of:
- runner.NewE2ETest( + // TODO: enable once protocol supports ZETA in V2 + /*runner.NewE2ETest( … - ), + ),*/or gate it behind
runner.WithMinimumVersion(...)/ a dedicated label.
1412-1417: Connector-funds migration test addedLooks good and follows the existing admin-test pattern. No issues spotted.
e2e/runner/evm.go (1)
100-116:ZETADeposit: good additionThe ZETA token address sanity-check is a nice touch; implementation mirrors ERC20 deposit flow.
e2e/e2etests/test_migrate_connector_funds.go (1)
55-57: Typo in comment
messaage→message.
kingpinXD
added
UPGRADE_TESTS
lumtis
left a comment
lumtis
left a comment
There was a problem hiding this comment.
Might be good @s2imonovic you have a look at this one
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* update go mod * add a new message for migrating funds * add migrate funds to e2e * rebase from develop * add v2 e2e test to check flow * add zeta gateway deposit to zetaclient * add github workflow * update comments * fix code formating * fix code formating * fix code formating * fix unit tests * fix unit tests * revert to old command to start e2e test * remove message for migration and refactor to using contract directly * add changelog * generate files after removing new message * update generated files * update generated files * resolve comments 1 * remove v2 from naming * generate files
* add empty test dapp * add test * generate * update gomod * fix imports * smaller message * fix size * ci: generate TypeScript types (#3978) * test: lower Bitcoin E2E deposit tx fee to make nightly test cheaper (#3989) lower Bitcoin E2E deposit test tx fee * test: add connector fund migration e2e test using contracts only (#3976) * update go mod * add a new message for migrating funds * add migrate funds to e2e * rebase from develop * add v2 e2e test to check flow * add zeta gateway deposit to zetaclient * add github workflow * update comments * fix code formating * fix code formating * fix code formating * fix unit tests * fix unit tests * revert to old command to start e2e test * remove message for migration and refactor to using contract directly * add changelog * generate files after removing new message * update generated files * update generated files * resolve comments 1 * remove v2 from naming * generate files * chore: fix some comments (#3993) Signed-off-by: yingshanghuangqiao <yingshanghuangqiao@foxmail.com> * feat(ton): integrate new functionality (#3977) * Add ton.call operation * vote inbound call * update ton's gateway code * e2e: ton_to_zevm_call * Add increaseSeqno parsing * ton: signer: increase_seqno integration * ton: observer: increase_seqno integration * Fix bugs. E2E for increase_seqno * Update changelog * Address PR comments * Simplify inbound voting * outbounds: validate nonce & simplify code * bump gw * refactor: update generated files (#4000) update generated files * ci: run simualtion tests nightly (#3999) * update sim.yml * add changelog * update generated files * generate * add test for deposit with big payload * generate * update contract version * try removing test * fix wrong method called * remove redundant log * generate * add version condition --------- Signed-off-by: yingshanghuangqiao <yingshanghuangqiao@foxmail.com> Co-authored-by: Denis Fadeev <denis@fadeev.org> Co-authored-by: Charlie Chen <34498985+ws4charlie@users.noreply.github.com> Co-authored-by: Tanmay <tanmay@zetachain.com> Co-authored-by: yingshanghuangqiao <yingshanghuangqiao@foxmail.com> Co-authored-by: Dmitry S <11892559+swift1337@users.noreply.github.com>
4 participants
Description
Depends on : #3976 to be merged to show accurate changes
Closes : #3946
The ChainParams are not updated as the protocol changes for v2 have not been added yet
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation