test(e2e): add withdraw and deposit tests with big payload#3985
test(e2e): add withdraw and deposit tests with big payload#3985
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 📝 WalkthroughWalkthroughA new end-to-end test, Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as E2ERunner
participant EVMChain as EVM Chain
participant ZEVM as ZEVM Gateway
participant Contract as TestDAppEmpty
TestRunner->>EVMChain: Deploy TestDAppEmpty contract
TestRunner->>ZEVM: Increase gas limit for authorization context
TestRunner->>ZEVM: Approve ETH ZRC20 for gateway contract
TestRunner->>ZEVM: Withdraw ETH with call (large payload) to Contract
ZEVM->>EVMChain: Execute contract call with payload
EVMChain->>Contract: onCall invoked with large payload
EVMChain-->>ZEVM: Return transaction result
TestRunner->>ZEVM: Wait for cctx to be mined
ZEVM-->>TestRunner: cctx status (OutboundMined)
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
|
![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 comments (1)
e2e/e2etests/helpers.go (1)
17-26: Expose size parameter but guard against pathological values
randomPayloadWithSizeis useful; however nothing prevents a caller from passing a multi-MB size that could bloat logs or exceed chain call limits.func randomPayloadWithSize(r *runner.E2ERunner, size int) string { - bytes := make([]byte, size) + const max = 4 * 1024 // 4 KB hard cap – adjust as needed + if size <= 0 || size > max { + require.Fail(r, fmt.Sprintf("payload size %d exceeds maximum %d", size, max)) + } + bytes := make([]byte, size)This keeps accidental OOM / huge calldata in check while remaining flexible.
🧹 Nitpick comments (5)
e2e/e2etests/e2etests.go (1)
364-369: Provide explicit args for new test to stay consistentAll neighbouring withdraw-and-call tests specify at least
amount(and oftengasLimit). Omitting them here makes the CLI help inconsistent and forces the test to rely on hard-coded values.- []runner.ArgDefinition{}, + []runner.ArgDefinition{ + {Description: "amount in wei", DefaultValue: "100000"}, + {Description: "gas limit for withdraw", DefaultValue: "500000"}, + },Add whichever parameters the test actually reads so automation & docs remain uniform.
e2e/contracts/testdappempty/bindings.go (1)
1-4: Harden go:generate pipeline for reproducibility
set -euo pipefailbefore the command chain stops partially-generated artifacts on failure.--overwriteforabigenavoids manual cleanup on re-runs.- Consider deleting the intermediate JSON once the ABI/BIN are extracted to keep the repo clean (or generate into
$(mktemp)).Example:
-//go:generate sh -c "solc TestDAppEmpty.sol --evm-version london --combined-json abi,bin | jq '.contracts.\"TestDAppEmpty.sol:TestDAppEmpty\"' > TestDAppEmpty.json" +//go:generate sh -ceu "solc TestDAppEmpty.sol --evm-version london --combined-json abi,bin | \ + jq '.contracts[\"TestDAppEmpty.sol:TestDAppEmpty\"]' > TestDAppEmpty.json" ... -//go:generate sh -c "abigen --abi TestDAppEmpty.abi --bin TestDAppEmpty.bin --pkg testdappempty --type TestDAppEmpty --out TestDAppEmpty.go" +//go:generate sh -c "abigen --overwrite --abi TestDAppEmpty.abi --bin TestDAppEmpty.bin \ + --pkg testdappempty --type TestDAppEmpty --out TestDAppEmpty.go"These tweaks make regeneration deterministic and developer-friendly.
e2e/e2etests/test_eth_withdraw_and_call_big_payload.go (2)
20-24: Prefert.Cleanupover a manualdeferfor state restorationThe test relies on a
deferto restorer.ZEVMAuth.GasLimit.
When the helper evolves to use sub-tests (t.Run) or when an earlyr.T().Fatalis introduced, thedeferwill not run for the parent test, potentially leaving the runner in an inconsistent state for subsequent tests.
Registering the restoration withr.T().Cleanup(ort.Cleanup) keeps the guarantee even with nested test invocations.previousGasLimit := r.ZEVMAuth.GasLimit -r.ZEVMAuth.GasLimit = 10000000 -defer func() { - r.ZEVMAuth.GasLimit = previousGasLimit -}() +r.ZEVMAuth.GasLimit = 10_000_000 +r.T().Cleanup(func() { r.ZEVMAuth.GasLimit = previousGasLimit })
26-28: Add an assertion on the payload size for self-documenting safetyThe comment explains the expected encoded length, but the code never asserts it.
A quick sanity check prevents silent regressions of the helper.payload := randomPayloadWithSize(r, 1440) require.Len(r, payload, 2880, "hex payload length should remain within gateway limit")e2e/contracts/testdappempty/TestDAppEmpty.sol (1)
34-42: Mark stub functionsexternal payable/pureto minimise gas where possible
onCallcurrently does nothing yet is declarednonpayablewithoutview/pure.
Explicitly flagging intent keeps the ABI clear and lowers deployment & invocation gas.-function onCall( +function onCall( zContext calldata /*context*/, address /*_zrc20*/, uint256 /*amount*/, bytes calldata /*message*/ -) -external -{ -} +) +external +pure +{}Apply the same treatment to the other empty handlers (
onRevert,onCalloverload) as appropriate.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e/contracts/testdappempty/TestDAppEmpty.binis excluded by!**/*.bingo.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/zetae2e/local/evm.go(1 hunks)e2e/contracts/testdappempty/TestDAppEmpty.abi(1 hunks)e2e/contracts/testdappempty/TestDAppEmpty.go(1 hunks)e2e/contracts/testdappempty/TestDAppEmpty.json(1 hunks)e2e/contracts/testdappempty/TestDAppEmpty.sol(1 hunks)e2e/contracts/testdappempty/bindings.go(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/helpers.go(1 hunks)e2e/e2etests/test_eth_withdraw_and_call_big_payload.go(1 hunks)go.mod(1 hunks)
🧰 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.
cmd/zetae2e/local/evm.goe2e/e2etests/helpers.goe2e/contracts/testdappempty/bindings.goe2e/e2etests/e2etests.goe2e/e2etests/test_eth_withdraw_and_call_big_payload.goe2e/contracts/testdappempty/TestDAppEmpty.go
🔇 Additional comments (7)
go.mod (1)
55-55: Check downstream compatibility of protocol-contracts bumpThe update to
github.com/zeta-chain/protocol-contractspulls in an Ath3.0 build produced two weeks after the previous pin. Ensure:
•go mod tidy(and CI) passes – the new tag may have extra indirect deps.
• Any generated bindings or interface changes expected by the E2E tests are present – otherwise the tests will silently break at runtime.cmd/zetae2e/local/evm.go (1)
25-25: New big-payload test wired in – looks goodHooking
TestETHWithdrawAndCallBigPayloadNameinto the happy-path suite keeps the order/parallelism untouched and requires no extra funding. 👍e2e/e2etests/e2etests.go (1)
25-25: Constant added – no concernse2e/contracts/testdappempty/bindings.go (1)
6-8: Necessity of dummy reference is questionable
var _ TestDAppEmptyexists only to silencego vetabout empty packages; the generated binding already introduces usable identifiers. If future code imports the package it becomes redundant.e2e/contracts/testdappempty/TestDAppEmpty.go (1)
1-308: Generated file – no manual changes expected.e2e/contracts/testdappempty/TestDAppEmpty.json (1)
1-125: Compiled artefact – nothing to review.e2e/contracts/testdappempty/TestDAppEmpty.abi (1)
1-122: Compiled artefact – nothing to review.
…3989) lower Bitcoin E2E deposit test tx fee
* 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
Signed-off-by: yingshanghuangqiao <yingshanghuangqiao@foxmail.com>
* 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
update generated files
* update sim.yml * add changelog * update generated files
lumtis
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3985 +/- ##
========================================
Coverage 64.83% 64.83%
========================================
Files 475 475
Lines 34860 34860
========================================
Hits 22602 22602
Misses 11209 11209
Partials 1049 1049
🚀 New features to boost your workflow:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
6 participants
Summary by CodeRabbit