test(cketh): credit twenty CEX deposits through one EIP-7702 sweep per token - #11363
Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end validation of ckERC20 CEX-deposit sweeping against a live PocketIC and Anvil environment.
Changes:
- Adds a 20-user, two-token sweep integration test and live harness support.
- Deploys and inspects real sweep contracts and transactions.
- Scales sweep gas limits by batch size.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test_utils/src/live.rs |
Extends the live sweep harness. |
test_utils/src/lib.rs |
Configures Anvil sweep contracts. |
test_utils/src/ckerc20.rs |
Supports custom deposit helpers. |
test_utils/src/anvil.rs |
Adds chain and transaction utilities. |
tests/deposit_from_cex_sweep.rs |
Adds the end-to-end sweep test. |
src/sweep/mod.rs |
Applies dynamic sweep gas limits. |
src/state/transactions/tests.rs |
Localizes the legacy test gas constant. |
src/state/transactions/mod.rs |
Calculates batch-dependent gas limits. |
BUILD.bazel |
Registers the new integration test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| amount: deposit.amount, | ||
| }) | ||
| .collect(); | ||
| setup.credit_deposits(&holdings); |
There was a problem hiding this comment.
🤖 The mechanism claim is accurate at the current head: LiveSetup::credit_deposits (test_utils/src/live.rs:276-283) still credits by set_storage_at on each erc20_balance_slot, so no funding transaction is mined. The test's own comment ("The CEX withdrawals: a plain ERC-20 transfer to each address") describes something the harness does not do.
Two facts for whoever decides this, since the swap is not a drop-in:
- The helper you suggest does exist —
Anvil::fund(token, from, to, amount)attest_utils/src/anvil.rs:201— anddeposit_from_cex_demo.rs:666already funds its deposits that way, so there is precedent. - But
credit_depositswrites every balance before any token gets code, deliberately: its doc says the fail-loud batcher only advances a scan once every token has code, so a concurrent scan can never observe a partially-credited state. A realtransferneeds the code first, which inverts that ordering and reopens the race the comment guards against.
So it is a reword of the claim, or a funding leg that also handles the ordering — not a one-liner. Left for the author.
6f40a7c to
ac60461
Compare
|
🤖 Cross-checked the reviewer comments from the previous PR stack (#11259, #11261) against this test. Fixed here: the Bazel target carries
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
rs/ethereum/cketh/test_utils/src/anvil.rs:316
last_transaction_ofreverses block heights but not the transactions within a block. Ethereum block transactions are ordered oldest-first, so when this sender has multiple transactions in the newest block (which interval mining explicitly permits), this returns the oldest one rather than the most recent. Reverse the inner iterator as well.
for transaction in transactions {
rs/ethereum/cketh/minter/tests/deposit_from_cex_sweep.rs:96
- This does not exercise the CEX-style plain ERC-20 transfers promised by the PR:
credit_depositswrites each recipient's balance directly withanvil_setStorageAt(live.rs:257-279). Consequently the test can pass without anytransfertransaction occurring. Provision a sender balance on each mock token and callAnvil::fundfor every deposit so the stated flow is actually covered.
setup.credit_deposits(&holdings);
| // One sweep per token, and nothing more. | ||
| let sweeps = await_sweeps(&setup, &sweeper, 2); |
There was a problem hiding this comment.
🤖 Agreed — two successful transactions plus aggregate balances did not rule out the mixed-batch-plus-redundant-transaction shape. Pinned in 94a21f1 off the minter's own audit log, which LiveSetup::minter_events already exposes: the test now collects every AcceptedSweepRequest as (token, items.len()) and asserts the sorted result is exactly [(USDC, 10), (USDT, 10)]. The token is compared as a parsed Address rather than a string, so checksummed and lowercase spellings cannot diverge.
That fails on a mixed batch, on a third sweep, and on a short batch. Note also that EventPayload::AcceptedSweepRequest carries a single token field (endpoints.rs:598-607), so a genuinely mixed batch is not representable today — the assertion pins the property rather than merely testing for it.
| let mut setup = Self::go_live(ckerc20, anvil); | ||
| setup.sweep_contracts = Some(contracts); | ||
| setup.anvil.set_balance(sweeper, sweeper_gas_wei); |
There was a problem hiding this comment.
🤖 Real gap, and cheap to close: go_live already fetches the installed minter's address into LiveSetup::minter_address (test_utils/src/live.rs:468, via fetch_minter_address at :728), so the deployment-time constant can be checked against it.
Added that assertion in 94a21f1, right after go_live in new_sweep. The contracts still have to be deployed before the minter exists — the helper needs a payout address up front — but a stale MINTER_ADDRESS now fails the fixture instead of silently sweeping into an address the minter does not control.
| EventPayload::AcceptedDeposit { .. } | EventPayload::MintedCkErc20 { .. } => { | ||
| "minted" | ||
| } |
There was a problem hiding this comment.
🤖 Verified and fixed in be02e74. EventPayload::AcceptedErc20Deposit does exist (minter/src/endpoints.rs:525) and sweep_stages matched only AcceptedDeposit, the ETH-only variant — so in this ERC-20 flow the scraped-log stage never appeared, exactly as you describe.
It now also reads separately rather than being folded into the mint, since those are different failures:
AcceptedDeposit | AcceptedErc20Deposit => "scraped"
MintedCkErc20 => "minted"
So a run that scrapes but never mints now times out reporting scraped without minted, instead of stopping at finalized.
| /// The minter's dedicated sweeper address, derived from the same test key as [`MINTER_ADDRESS`] | ||
| /// under the sweeper derivation path. Hardcoded because no endpoint reports it yet; a test that | ||
| /// funds it asserts the sweep really was sent from here, so a stale value fails loudly. |
There was a problem hiding this comment.
🤖 Right, MinterInfo does carry sweeper_address (minter/src/endpoints.rs:75), so "no endpoint reports it yet" was wrong. Reworded in be02e74: the constant is now described as the value the derivation is expected to produce, so a test can name it before the minter is installed, with a pointer to MinterInfo::sweeper_address as what the running minter actually derived. The fail-loud note stays.
That ordering is the real reason it is hardcoded — LiveSetup::new_sweep takes the sweeper address as an argument to pre-fund it, before go_live exists to be asked.
| // The CEX withdrawals: a plain ERC-20 transfer to each address, carrying no principal. | ||
| let holdings: Vec<Holding<'_>> = deposits | ||
| .iter() | ||
| .map(|deposit| Holding { | ||
| deposit: deposit.address, | ||
| token: deposit.token, | ||
| amount: deposit.amount, | ||
| }) | ||
| .collect(); | ||
| setup.credit_deposits(&holdings); |
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay!
| /// Prints what each sweep cost next to `deposit_from_cex_demo`'s measurements for the same delegate, | ||
| /// so a change in either shows up as a difference rather than having to be recomputed by hand. | ||
| fn report_gas(sweeps: &[SentTransaction], deposits_per_sweep: u64) { | ||
| // `ATTESTED_SCENARIOS` in deposit_from_cex_demo.rs, EIP-7702 (first sweep) column. | ||
| const DEMO_ONE_DEPOSIT: u64 = 98_000; | ||
| const DEMO_TEN_DEPOSITS: u64 = 609_431; | ||
|
|
||
| for sweep in sweeps { | ||
| let per_deposit = sweep.gas_used / deposits_per_sweep; | ||
| println!( | ||
| "[gas] {} deposits: {} total, {} per deposit \ | ||
| (demo: {DEMO_TEN_DEPOSITS} total, {} per deposit for ten; {DEMO_ONE_DEPOSIT} for one)", | ||
| deposits_per_sweep, | ||
| sweep.gas_used, | ||
| per_deposit, | ||
| DEMO_TEN_DEPOSITS / 10, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this be an assert (as well), if the cost goes (much) higher (or lower?) than expected?
| # The end-to-end tests each run a live PocketIC and wait (wall-clock) for the minter's periodic | ||
| # tasks — and the sweep test drives twenty deposits through two sweeps — so the target needs a | ||
| # far larger timeout than the pure-anvil tests. | ||
| size = "large", |
There was a problem hiding this comment.
For size = "large", I believe we should be tagging the test with long_test so that it doesn't run on PRs. We may even want to set size = "enormous", but we could also wait and see how long it takes on CI and only bump it if it starts timing out. Alternatively, we could try splitting up the test, but that may also cause issues on CI if multiple tests are run in parallel (from a resource-contention PoV) - we already now have "RUST_TEST_THREADS": "1".
be02e74 to
8e02651
Compare
…r token Twenty users, each with its own principal and subaccount, register deposit addresses for USDC and USDT; a CEX-style plain transfer funds each; the minter detects all twenty, sweeps them in one transaction per token, and credits every user in full with no further user action. It gets its own target rather than another case in `deposit_from_cex`: driving twenty deposits through two sweeps on one live PocketIC starves the other end-to-end tests of that (sequential) target until their ingress messages expire unanswered.
…nterval `await_scan` polled in real time for the minter's 30s balance-scan timer, so every live scan test idled for one or two full intervals. It now buys `BALANCE_SCAN_INTERVAL`-sized ticks through the same `settle`/`drive_until` machinery the sweep and funding waits already use, which also subsumes `drive_scan`. The sweep e2e test drops from ~93s to ~44s and the per-token minimum test from ~73s to ~24s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YK3ggw1xtE3HZTGGBu66oD
The twenty-deposit sweep test had its own file and Bazel target, on the theory that sharing a sequential target would starve the other live tests of CPU. It doesn't: the merged target runs all six tests in 81 seconds. One file and one target now carry the whole deposit-from-CEX story — batcher semantics, balance scan, sweeper funding, and the sweep itself — with the target sized large for the longer run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRarGTxdVa9ZQejC9ZYR3N
The sweep e2e carried its own harness plumbing: waits for the sweeper's transactions and the ledger credits, the pipeline-stage diagnostic their failure messages print, and a copy of the contract_address parser the harness already had. The waits now live on LiveSetup next to the scan and funding ones they parallel, the duplicate parser is gone, and the test reads as scenario and assertions only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRarGTxdVa9ZQejC9ZYR3N
…rness trusts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YK3ggw1xtE3HZTGGBu66oD
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YK3ggw1xtE3HZTGGBu66oD
8e02651 to
4933711
Compare
Why
What
transferfunds each, carrying no principal. The minter detects all twenty, sweeps them in one transaction per token, and credits every user in full with no further user action.