test(cketh): build the live balance-scan harness on the shared fixtures - #11124
test(cketh): build the live balance-scan harness on the shared fixtures#11124gregorydemay wants to merge 27 commits into
Conversation
Shares the EVM RPC/minter install-args construction between the mocked CkEthSetup fixture and the live anvil-backed balance-scan harness via a new EvmRpcBackend enum, instead of live_scan.rs duplicating those functions wholesale. Also extracts canister creation and ledger install out of CkEthSetup::new into reusable helpers, and fixes the module doc comment, which predated the ckerc20 mocked fixture's migration to PocketIC and still claimed it ran on StateMachine. The live harness keeps its own canister-creation, cycles and non-anonymous controller setup separate from CkEthSetup::new: converging it onto the shared (anonymous-controller) construction reproducibly crashed the PocketIC replica with a cycle-accounting assertion failure once the minter went live, so that axis stays intentionally distinct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors the ckETH/ckERC20 PocketIC test fixtures to reduce duplicated canister setup/installation logic, while keeping the live (anvil-backed) harness’s intentionally distinct controller/cycles behavior.
Changes:
- Extracts shared canister creation + ckETH ledger installation into reusable helpers.
- Introduces an
EvmRpcBackend(MockedvsAnvil) to centralize EVM RPC provider override + block-height/last-scraped assumptions. - Updates the live balance-scan harness to reuse the shared PocketIC builder chain and refreshes its module documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
rs/ethereum/cketh/test_utils/src/live_scan.rs |
Updates live scan harness docs and reuses shared builder + EvmRpcBackend-driven install configuration. |
rs/ethereum/cketh/test_utils/src/lib.rs |
Extracts shared canister creation/ledger install helpers and adds EvmRpcBackend + pocket_ic_builder() for reuse across fixtures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🤖🧐 VERDICT: CHANGES_REQUESTED — 0 blockers, 2 mediums, 4 nits; CI pending ( Review detailsVerified claims
The three axes kept separate
Maintainability accounting
TestingNo behaviour change, so no new test is owed; the existing suites are the regression net and all pass locally:
All run with |
Carries an install/upgrade sender (Option<Principal>) on CkEthCanisters so the shared install_minter/install_evm_rpc can serve both the mocked fixture (anonymous sender) and the live harness (its own non-anonymous controller), instead of live_scan.rs hand-building near-identical copies of both functions. Deletes those copies. Addresses PR #11124 review comment (Medium): #11124 (comment)
The previous commit's dedup removed live_scan.rs's local install_minter, so "key_1" was only written once already; this replaces lib.rs's remaining `.parse().unwrap()` literal with the named constant the duplicate used to carry, avoiding an unnecessary fallible conversion. Addresses PR #11124 review comments (Nit + Copilot): #11124 (comment) #11124 (comment)
The controller() doc wrongly blamed the anonymous-controller crash on the controller identity. Re-tested per review: converging everything else but keeping u128::MAX cycles (the shared mocked-fixture helper's amount) reproduces the same crash with controller() left untouched, confirming it is cycle-balance saturation (`AddAssign for Cycles` saturates, so a canister already at u128::MAX cannot observe any further addition, tripping the replica's cycle-accounting assertion on its first live HTTPS outcall) — not the controller. controller() keeps its own doc, now stating its real, sufficient justifications; the cycles constraint is now documented at the add_cycles call sites. Addresses PR #11124 review comment (Medium): #11124 (comment)
Now that install_minter/install_evm_rpc are genuinely shared between the two fixtures (previous commits), the enum backs both the EVM RPC canister's install args and the minter's chain-state init args, not just the EVM RPC side its old name implied. Addresses PR #11124 review comment (Nit): #11124 (comment)
…NUMBER_AT_INSTALL Both Anvil arms already carried a one-clause reason; the Mocked arms didn't, leaving a reader unable to tell why replaying canned mainnet responses implies these two particular values without also reading the enum's top-level doc. Addresses PR #11124 review comment (Nit): #11124 (comment)
…ses it "every ckETH fixture creates" overstated it: the live harness creates its 3 canisters itself, in a different order (ledger first, not minter first) and under its own controller, then assembles this same struct to feed the shared installers rather than creating it via create_cketh_canisters. Addresses PR #11124 review comment (Nit): #11124 (comment)
|
🤖 Addressed all 6 review findings, one commit each:
Cycles experiment (per the Medium-1 root-cause comment): re-tested with Verification (all synchronous,
|
|
🤖🧐 VERDICT: READY (re-review) — 0 blockers, 0 mediums, 1 nit; CI 21 pass / 0 fail / 6 pending. Merge gate: Re-review detailsPer-finding resolution — all six resolved
Does
|
Drop the unverified "never executed as real cycles since it never goes live" parenthetical: the experiment only established that this harness's cycles amount triggers the crash, not why the mocked fixture survives u128::MAX (PocketIC runs the same replica execution live or not; the likelier distinction is the NNS subnet this harness adds and whatever cost schedule follows from it, which was not verified either). State only what the evidence supports. Also move the comment to sit directly above the add_cycles call it describes, and note it applies to minter_id's identical add_cycles below too. Addresses PR #11124 review comment (Nit).
…self Deletes CkErc20LiveScanSetup and its new_live(), which still ran their own canister creation, settings, cycles and install sequencing in parallel to CkEthSetup::new — a second fixture in substance, not just in name. Adds a builder on CkEthSetup (CkEthSetup::builder()) with two knobs: with_ethereum_backend (Mocked/Anvil, already shared via EthereumBackend) and with_live_mode, which switches on every axis the live harness needs and off by default: an NNS subnet in addition to the fiduciary one, a fixed non-anonymous controller, u64::MAX cycles (headroom below the saturating balance ceiling established in the prior fix), an uninstalled placeholder ckETH ledger, and make_live before the minter install. CkEthSetup::new/Default keep today's behaviour unchanged, now delegating to the same builder in its default (mocked) mode. The live-scan-specific behaviour (Holding, SupportedToken, credit_deposits, await_scan, balance_scan_candidates, deposit_erc20, register_deposit_address, depositor, ckERC20 activation and token registration) now hangs off a thin LiveBalanceScanSetup that wraps CkEthSetup plus the anvil node, rather than reimplementing any of it. Built on CkEthSetup rather than CkErc20Setup: the balance scan needs only the minter and EVM RPC canister, and CkErc20Setup would drag in a real ledger-suite-orchestrator and spawned per-token ledgers for no benefit here. deposit_from_cex.rs updated to construct LiveBalanceScanSetup instead of the deleted CkErc20LiveScanSetup.
…ence
The prior split kept the same duplication under a new name: two
sibling functions each spelling out the full canister-creation/install
sequence. Collapses them into a single CkEthSetupBuilder::build with a
small number of conditionals instead:
- One new_env(live) builds the PocketIC instance for both modes:
fiduciary subnet always, NNS subnet added and made live in live mode.
make_live now runs before any canister of this fixture exists (no
install has scheduled a timer yet), rather than between the EVM RPC
and minter installs — verified against the live suite, not assumed.
- create_cketh_canisters takes the single controller: Option<Principal>
CkEthCanisters already carried, via a create_canister(env, controller)
helper; no separate CanisterSettings axis on the builder.
- Cycle amount is unified to u128::from(u64::MAX) for both modes,
removing the last hardcoded fork; verified no test asserts a cycle
balance by running the full suite.
- Canister creation order (minter first) is untouched, so it is shared
by construction rather than by convention.
The one axis that stayed conditional: installing the real ckETH ledger
for the live harness too. Tried it; it fails outright, because the
anvil-backed deposit_from_cex Bazel target does not declare the ledger
canister Wasm as a data dependency, so load_wasm's cargo-metadata
fallback has no access to the ledger crate's directory in that
sandbox. Kept as `if !self.live { install_ledger(..) }`, documented at
the call site with the failure mode that was actually observed.
CkEthSetup::new/Default are unaffected in creation order, MINTER_ADDRESS
derivation and the cold-start property (new_env(false) never calls
make_live).
| fn create_cketh_canisters(env: &PocketIc, controller: Option<Principal>) -> CkEthCanisters { | ||
| // Create minter canister first to match canister ID and Ethereum address hardcoded in tests. | ||
| let minter_id = create_canister(env, controller); | ||
| env.add_cycles(minter_id, u128::from(u64::MAX)); |
There was a problem hiding this comment.
🧐 🟠 Medium — unifying the cycle amount silently deleted the only record of why it is not u128::MAX. d14bc70 deliberately placed a comment above this add_cycles explaining that the value must leave headroom below the saturating Cycles ceiling, because a canister already at the ceiling cannot observe a further addition and the first HTTPS outcall a live canister accepts cycles for then trips a cycle-accounting assertion in the replica. That constraint still holds and is still load-bearing for deposit_from_cex, but nothing in the tree says so any more: u128::from(u64::MAX) now reads as an arbitrary magic value repeated three times (G25/G32), and the obvious "simplification" back to u128::MAX would break only the anvil-backed target. Please restore the rationale once — ideally as a named constant carrying the doc, e.g. /// … const CANISTER_CYCLES: u128 = u64::MAX as u128; — so the three call sites share one authoritative explanation (G5). C2 also applies in reverse: the comment was not obsolete, the code it described is still here.
There was a problem hiding this comment.
🤖 Fixed in ad8ce46 — restored the rationale as a named CANISTER_CYCLES constant carrying the doc, used by all three add_cycles sites in create_cketh_canisters.
| let controller = self.live.then(live_controller); | ||
| let canisters = create_cketh_canisters(&env, controller); | ||
| if !self.live { | ||
| // Tried installing the real ledger for the live harness too, to drop this |
There was a problem hiding this comment.
🧐 🟠 Medium — this comment leads with the incidental obstacle and buries the reason. The reason the live harness skips the ledger is the last sentence: the balance scan never calls it. The Bazel data-dependency gap is a fixable accident of the deposit_from_cex target, not a justification — as written a reader concludes "declare the ledger Wasm as a data dep and this conditional disappears", which inverts cause and effect and invites work that would buy nothing. Please reorder to lead with the reason and mention the obstacle second (one clause is enough). Separately, six lines of prose about Bazel sandboxing sit at a different abstraction level from the three-line install sequence they interrupt (G6/G34) — the "why live mode has no ledger" half reads better on with_live_mode's doc, next to the other axes it switches, leaving a one-liner here.
There was a problem hiding this comment.
🤖 Fixed in 5d619c3 — reordered to lead with the reason (balance scan never calls the ledger), moved the Bazel-obstacle explanation to with_live_mode's doc (also folds in nit 8 below), and left a one-line pointer at the call site.
| } | ||
|
|
||
| fn register_supported_tokens(env: &PocketIc, minter_id: Principal) { | ||
| fn register_supported_tokens(cketh: &CkEthSetup) { |
There was a problem hiding this comment.
🧐 🟠 Medium — register_supported_tokens hand-rolls the update_call that CkEthSetup::add_ckerc20_token(&self, from, erc20) already wraps, so the harness reimplements a shared method it is now sitting directly on top of (G5). Given this PR's premise is reuse over relocated duplication, this is the one place it still does not hold. cketh.add_ckerc20_token(live_controller(), &arg).expect("BUG: add_ckerc20_token was rejected"); is a drop-in.
There was a problem hiding this comment.
🤖 Fixed in a114ffd — register_supported_tokens now calls cketh.add_ckerc20_token(live_controller(), &arg) instead of hand-rolling the update_call.
| /// ckETH fixture needs for the secp256k1 `key_1` used by the minter, plus — in live mode — an NNS | ||
| /// subnet (required by [`PocketIc::make_live`]) and going live immediately, before any canister of | ||
| /// this fixture exists to schedule a timer whose outcall could stall waiting for an answer. | ||
| fn new_env(live: bool) -> PocketIc { |
There was a problem hiding this comment.
🧐 🔵 Nit — new_env(live: bool) is a flag argument driving two materially different instances (F3/G15), and it reads badly at the call sites: new_env(false) on L815 tells the reader nothing. An enum would name both modes and make the asymmetry below self-documenting, e.g. enum EnvMode { Mocked, Live } / fn new_env(mode: EnvMode). Low stakes — the function is private with two call sites — but it is cheap here.
There was a problem hiding this comment.
🤖 Fixed in d000a55 — introduced enum EnvMode { Mocked, Live }, new_env now takes EnvMode instead of a bool.
| let mut env = pocket_ic_builder().with_nns_subnet().build(); | ||
| let _gateway = env.make_live(None); | ||
| env | ||
| } else { |
There was a problem hiding this comment.
🧐 🔵 Nit — the live branch does not set IcpConfig { canister_execution_rate_limiting: Disabled } that the mocked branch sets two lines below. This matches the pre-refactor build_live, so it is preserved behaviour rather than a regression, but now that both modes are built by one function the divergence is visible and unexplained: a reader cannot tell whether live mode wants the rate limiter on (real rounds, real timing) or whether the flag was simply never carried over. One clause saying which would settle it.
There was a problem hiding this comment.
🤖 Tried applying it to both modes and ran deposit_from_cex fresh (--nocache_test_results): still passes. Collapsed in 501c22f — no divergence left to explain.
| fn new_env(live: bool) -> PocketIc { | ||
| if live { | ||
| let mut env = pocket_ic_builder().with_nns_subnet().build(); | ||
| let _gateway = env.make_live(None); |
There was a problem hiding this comment.
🧐 🔵 Nit — make_live returns a Url, not an RAII guard, so naming the binding _gateway implies a lifetime contract that does not exist (the value is dropped immediately either way, and the gateway stays up). env.make_live(None); says exactly what happens (N1/N6, G16). Carried over from build_live, so purely a Boy-Scout item.
There was a problem hiding this comment.
🤖 Fixed in 501c22f (same commit as the rate-limiting unification, since both touch new_env) — dropped the _gateway binding, now plain env.make_live(None);.
Unifying the cycle amount to u128::from(u64::MAX) deleted the comment explaining why it isn't u128::MAX, leaving a magic value repeated three times with no record that the constraint is still load-bearing: u128::MAX reproducibly crashes the live harness' PocketIC replica with a cycle-accounting assertion failure on the minter's first HTTPS outcall, because a canister already at the saturating balance ceiling cannot observe any further addition. Restores it once as CANISTER_CYCLES, carrying the doc, used by all three add_cycles call sites in create_cketh_canisters. Addresses PR #11124 review comment (Medium): #11124 (comment)
…tacle The comment above install_ledger's `if !self.live` led with the Bazel data-dependency gap and buried the actual reason (the balance scan never calls the ledger) in the last sentence — inverting cause and effect, and reading as if declaring the Wasm as a data dependency would make the conditional disappear. Moves the "why live mode has no ledger" explanation to with_live_mode's doc, next to the other axes it switches (which also folds in the omitted ledger axis there), leaving a one-line pointer at the call site. Addresses PR #11124 review comments (Medium + Nit): #11124 (comment) #11124 (comment)
…orted_tokens register_supported_tokens hand-rolled the update_call that CkEthSetup::add_ckerc20_token already wraps, reimplementing a shared method the live harness now sits directly on top of — the one place this PR's reuse-over-relocated-duplication premise still didn't hold. Addresses PR #11124 review comment (Medium): #11124 (comment)
new_env's live branch omitted canister_execution_rate_limiting: Disabled, carried over unexplained from the pre-refactor build_live. Tried applying it to both modes and ran deposit_from_cex fresh: it still passes, so this axis collapses like the others rather than needing a divergence explained in prose. Also drops the _gateway binding for PocketIc::make_live's return value (a Url, not an RAII guard — the name implied a lifetime contract that never existed). Addresses PR #11124 review comments (Nits): #11124 (comment) #11124 (comment)
new_env(live: bool) was a flag argument driving two materially
different PocketIC instances, and read as opaque at its call sites
(new_env(false) tells the reader nothing). EnvMode::{Mocked, Live}
names both.
Addresses PR #11124 review comment (Nit):
#11124 (comment)
If both were set, build would use the supplied env as-is while self.live still selected the controller and skipped the ledger install, silently producing a live-flagged fixture on a non-live instance. The doc only asserted this in prose; a debug_assert! now encodes it. Addresses PR #11124 review comment (Nit): #11124 (comment)
…cked/build_live split "shared by every CkEthSetupBuilder::build" was a leftover from before build_mocked/build_live were collapsed into one build; there is exactly one now, so "every" read as if several still existed. Addresses PR #11124 review comment (Nit): #11124 (comment)
|
🤖 Addressed all 9 findings from the latest review, one commit each (2 pairs combined where they touched the exact same lines):
Rate-limiting experiment: tried setting PR description rewritten to match the current single- Verification (all synchronous, final pass
|
gregorydemay
left a comment
There was a problem hiding this comment.
🧐 VERDICT: READY — 0 blockers, 0 mediums, 2 nits (both new, both non-gating); CI green. All 9 findings from the previous round are genuinely resolved.
Review details
Previous 9 findings
| # | Finding | Status |
|---|---|---|
| 🟠 1 | cycles rationale deleted | Resolved (ad8ce46e58) |
| 🟠 2 | ledger comment leads with the obstacle | Resolved (5d619c334d) |
| 🟠 3 | register_supported_tokens hand-rolls add_ckerc20_token |
Resolved (a114ffdec4) |
| 🔵 4 | new_env(bool) flag argument |
Resolved (d000a55ab7) |
| 🔵 5 | rate-limiting divergence unexplained | Resolved by collapsing (501c22f0d7) — verified, see below |
| 🔵 6 | _gateway binding |
Resolved (501c22f0d7) |
| 🔵 7 | with_env/with_live_mode invariant unenforced |
Resolved (78aa809289) |
| 🔵 8 | with_live_mode doc missing the ledger axis |
Resolved (5d619c334d) |
| 🔵 9 | stale CkEthCanisters doc |
Resolved (986fd3886c) |
Spot-checks on the three I flagged as needing evidence rather than inspection:
- (1)
CANISTER_CYCLEScarries the constraint accurately —u128::MAXcrashes the live replica withInvalid cycle changeon the minter's first HTTPS outcall because a canister at the saturatingCyclesceiling cannot observe a further addition — and is used at all threeadd_cyclessites, with no remaining literal. - (2)
with_live_mode's doc now leads with "the balance scan never calls it" and demotes the Bazel data-dependency to a trailing clause; the install site keeps a one-line pointer. - (3)
cketh.add_ckerc20_token(live_controller(), &arg)is semantically identical to the code it replaced: same sender, sameResult<Vec<u8>, RejectResponse>, same.expecton the reject. The reply stays undecoded exactly as before.
Nit 5 — the behaviour change to the live instance, verified
canister_execution_rate_limiting: Disabled now applies to live mode too. Ran in-container, uncached (--nocache_test_results):
//rs/ethereum/cketh/minter:deposit_from_cex— 4 uncached runs, all PASSED (one standalone +--runs_per_test=3); 63.4s–70.5s, dev 2.9s. No flakiness signal.integration_tests_tests/cketh_testPASSED (73.0s),integration_tests_tests/ckerc20_testPASSED (138.2s),test_utils:lib_testsPASSED.
Non-negotiables re-verified from the call graph
make_liveunreachable fromCkEthSetup::new/Default.make_liveappears once, insidenew_envunderEnvMode::Live.EnvMode::Liveis produced only bybuild()whenself.live, which onlywith_live_mode()sets, whose only caller isLiveBalanceScanSetup::new_live().Default→builder().build()takes theEnvMode::Mockedpath;new(env)short-circuitsnew_enventirely viaunwrap_or_else. The cold-start property holds.- Mocked creation/install order matches
origin/master'sCkEthSetup::new. Minter created first (comment preserved), then ledger, then EVM RPC,add_cyclesafter each; installs ledger → EVM RPC → minter, senderNonethroughout. The single intentional delta is the cycle amount (u128::MAX→u64::MAX), which is the documented unification and is called out in the description. MINTER_ADDRESSderivation untouched. Same creation order and same"key_1"("key_1".parse().unwrap()→ECDSA_KEY_NAME.to_string(), same value).tests/cketh.rs:1186assertsminter_address() == MINTER_ADDRESSand passes uncached.
Maintainability accounting
- Duplication: none found. The last in-diff instance (finding 3) is gone. Structural duplication against the codebase: the sibling that motivated this PR (
CkErc20LiveScanSetup) is deleted, not mirrored. - Unused derives: cleared.
#[derive(Clone, Copy)]onEnvModeis load-bearing —new_envmatchesmodeby value twice, which would not compile withoutCopy. No other new derives. - Primitive-obsession parameters:
new_env's bool parameter is gone. What remains is a private builder field (live: bool) — raised as a 🔵 below. - Divergent invariant handling: none. The live/mocked fork is decided once in
build; thewith_env/liveinvariant has exactly one check. - Silent fallbacks: none new.
self.env.unwrap_or_else(..)is a builder default for an expected-absent input, not an invariant breach;balance_scan_candidates's.unwrap_or(0)is pre-existing and documented. - Test-only code in production modules: none — the diff is confined to
test_utilsplus one test file.
Docs
All doc references resolve against the code; no leftovers from the build_mocked/build_live era remain. On the re-review question about length: with_live_mode's doc is six lines covering four axes at roughly a clause each, and it is now the single definition of what live mode means — I judge that proportionate, not overgrown.
PR description
Accurate against the code. The previously false claim about a cycle-amount knob "documented at the add_cycles call sites" is gone; every artefact the rewrite names (CANISTER_CYCLES = u64::MAX as u128, new_env/EnvMode, create_cketh_canisters(env, controller: Option<Principal>), the rate-limiting unification, the reuse of CkEthSetup::add_ckerc20_token) matches what is in the tree, and the rate-limiting claim reproduces locally. Purpose-focused, no "Test plan" section. No 📚 stack section — and none to preserve: this PR bases directly on master, as does its sibling #11123.
Testing
No new tests are owed: this is a fixture refactor whose behaviour is pinned by the ~70 existing integration tests that run through it, all of which pass uncached above.
The builder tracked liveness on two extra axes — a `live` flag and an `EnvMode` enum — that were both functions of the Ethereum backend: only a chain reachable over the network needs genuine outcalls, so Anvil implies live and Mocked implies non-live. That left two of the four (live, backend) combinations unreachable and incoherent, guarded by a debug_assert over the private builder's two call sites. Make EthereumBackend the single mode axis and move the live harness' rationale onto its Anvil variant, where a reader meets it before the branches that act on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The variant borrowed the node's URL, which forced a lifetime parameter through EthereumBackend, CkEthSetupBuilder and their impls to describe a string used once, and let any `&str` stand in for a node that may not exist. Share the node behind an Arc instead: the type now proves a real node backs the URL, and every lifetime annotation goes away. The live harness keeps its own handle, so reaching the node stays infallible there rather than going through a fixture accessor that mocked setups could only answer by panicking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pocket_ic_builder() had exactly one call site (new_env) and its doc's first clause restated new_env's own almost verbatim. Inlines the PocketIcBuilder::new().with_fiduciary_subnet() chain into new_env and deletes the function and its doc; the fiduciary-subnet rationale (the secp256k1 key_1 the minter needs) stays stated once, on new_env's own doc comment. Confirmed via grep across rs/ that no other caller or re-export exists. Addresses PR #11124 review comment: #11124 (comment)
The Ethereum backend decides how the instance is built — a live one for anvil, a plain one for the mocked responses — yet the builder still accepted a caller-supplied instance, with nothing checking that it matched. Every caller passed the very instance the mocked backend would have built anyway, so the parameter only offered a way to get it wrong. Build the instance from the backend alone and drop the whole chain that threaded one in: with_env, CkEthSetup::new, new_pocket_ic, and CkErc20Setup's env arguments. CkErc20Setup now takes the shared instance from the ckETH fixture it builds first, which is the sharing it actually needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ery backend The live harness owned its canisters with a dedicated non-anonymous principal and skipped the ckETH ledger install, so which canisters existed and who controlled them depended on the Ethereum backend — a chain the fixture talks to, which has no bearing on either. Neither difference was load-bearing. The anonymous principal already controls every canister PocketIC creates by default, which is what makes the minter's controller-only logs readable as the anonymous caller, and the minter's orchestrator check is a plain caller comparison that an anonymous stand-in id satisfies. The skipped ledger install was down to the ledger Wasm missing from the anvil-backed test target's Bazel data, so declare it there. Canister creation and installation now match master exactly, for both backends, and the backend decides only what it actually determines: how the PocketIC instance is built and what the minter and EVM RPC canister are initialised with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The harness activated the ckERC20 feature and registered ckUSDC/ckUSDT itself, standing in for an orchestrator: it pointed the minter's orchestrator id at a principal it could call as, and invented a placeholder ledger id per token that nothing ever calls. That was only to avoid the ledger suites the balance scan does not read — but testing the deposit flow will need them. Wrap the anvil-backed ckETH fixture in CkErc20Setup instead, whose real orchestrator registers the same two tokens at the same mainnet addresses the scan reads, and drop both stand-ins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Integrates the pocket_ic_builder inlining pushed to the branch. That change is already present in this branch's tree, so the merged tree is unchanged; the conflicting hunks were EnvMode and new_pocket_ic, both removed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
rs/ethereum/cketh/test_utils/src/live_scan.rs:18
- This paragraph still claims only the minter + EVM RPC canister are installed and that no real orchestrator/ledgers are created. However
LiveBalanceScanSetup::new_livenow callsCkErc20Setup::with_cketh(..).add_supported_erc20_tokens(), which installs the ledger-suite orchestrator and spawns ledger/index canisters. Update the docs to match the current behavior (or change the setup to avoid the orchestrator).
//! Only the minter and the EVM RPC canister are installed. The full ckERC20 feature is activated by
//! pointing the minter's ledger-suite-orchestrator id at a principal this harness controls, so
//! supported tokens can be registered directly via `add_ckerc20_token` without a real orchestrator
//! or any spawned ledgers — the balance scan only needs the token contract addresses in the
//! minter's state.
rs/ethereum/cketh/minter/BUILD.bazel:320
- PR description says "no Bazel files changed", but this PR updates the
deposit_from_cexBazel target (adds wasm data deps + env vars). Please update the PR description to reflect this, since it affects how the live harness is built/executed under Bazel.
"tests/deposit_from_cex_demo/MockUSDT.sol",
# End-to-end balance scan on a live PocketIC + local anvil node.
":cketh_minter_debug.wasm.gz",
"//rs/ethereum/ledger-suite-orchestrator:ledger_suite_orchestrator_canister.wasm.gz",
"//rs/ledger_suite/icrc1/archive:archive_canister_u256.wasm.gz",
"//rs/ledger_suite/icrc1/index-ng:index_ng_canister_u256.wasm.gz",
"//rs/ledger_suite/icrc1/ledger:ledger_canister_u256.wasm.gz",
"//rs/pocket_ic_server:pocket-ic-server",
"@evm_rpc.wasm.gz//file",
],
env = {
"ANVIL_BIN": "$(rootpath //:anvil)",
"MOCKUSDT_SOL": "$(rootpath tests/deposit_from_cex_demo/MockUSDT.sol)",
"SOLC_BIN": "$(rootpath //:solc)",
"CARGO_MANIFEST_DIR": "rs/ethereum/cketh/minter",
"CKETH_MINTER_WASM_PATH": "$(rootpath :cketh_minter_debug.wasm.gz)",
"EVM_RPC_CANISTER_WASM_PATH": "$(rootpath @evm_rpc.wasm.gz//file)",
"INDEX_CANISTER_WASM_PATH": "$(rootpath //rs/ledger_suite/icrc1/index-ng:index_ng_canister_u256.wasm.gz)",
"LEDGER_ARCHIVE_NODE_CANISTER_WASM_PATH": "$(rootpath //rs/ledger_suite/icrc1/archive:archive_canister_u256.wasm.gz)",
"LEDGER_CANISTER_WASM_PATH": "$(rootpath //rs/ledger_suite/icrc1/ledger:ledger_canister_u256.wasm.gz)",
"LEDGER_SUITE_ORCHESTRATOR_WASM_PATH": "$(rootpath //rs/ethereum/ledger-suite-orchestrator:ledger_suite_orchestrator_canister.wasm.gz)",
"POCKET_IC_BIN": "$(rootpath //rs/pocket_ic_server:pocket-ic-server)",
rs/ethereum/cketh/test_utils/src/live_scan.rs:9
- The module docs say this harness is "Unlike
CkErc20Setup" because that fixture uses canned JSON-RPC mocks, butLiveBalanceScanSetup::new_livenow constructs aCkErc20Setupon top of a liveCkEthSetupwith an anvil backend (real HTTPS outcalls). The doc should be updated to reflect thatCkErc20Setupis reused here, but with a live backend rather than mocks.
//! Unlike [`crate::ckerc20::CkErc20Setup`] — which also runs on PocketIC but answers the EVM RPC
//! canister's JSON-RPC outcalls with canned canister-http mocks ([`crate::mock::MockJsonRpcProviders`])
//! — this harness runs PocketIC in *live* mode so the EVM RPC canister makes genuine outcalls
//! through the IC's HTTPS-outcalls feature, and installs it with an `overrideProvider` that
//! rewrites every provider URL to the harness' anvil node (reached over HTTP, mirroring the
|
✅ No security or compliance issues detected. Reviewed everything up to 3cb7de4. Security Overview
Detected Code Changes
|
deposit_from_cex failed 5/5 under CPU contention (--runs_per_test=5 --local_test_jobs=5), each run timing out after "100 rounds" inside CkErc20Setup's construction. new_env called make_live before any canister existed, so the whole fixture — now including the orchestrator spawning ledger/index canisters — was built against an auto-progressing instance, where rounds advance on wall-clock time rather than per call; under contention, each setup ingress raced a deadline it did not control and lost. Fixes it by building the whole fixture on an ordinary non-live instance (where await_call ticks deterministically) and switching to live outcalls only once construction is complete, right before the balance scan needs them: LiveBalanceScanSetup::new_live now calls env.auto_progress() after CkErc20Setup::with_cketh(..) .add_supported_erc20_tokens() returns, instead of new_env calling make_live up front. auto_progress()/stop_progress() take &self, unlike make_live's &mut self, so this works straight through the shared Arc<PocketIc> with no restructuring of who owns the env. Two things tried and dropped after actually being unneeded (verified, not assumed): - Failing pending canister-http requests before going live (CkEthSetup::fail_pending_https_outcalls exists for exactly this). The 5-way contention command passed 10/10 across two full runs without it, so it's left out. - with_nns_subnet(), which make_live required for its HTTP gateway. Nothing here creates a gateway (canisters are driven through the client API; anvil is reached by the replica's canister-http adapter), and the suite stays green without it, so new_env no longer depends on EthereumBackend at all. EthereumBackend::is_live() is now dead and removed with it. Timing: deposit_from_cex alone, uncached, is ~71s after this change vs ~65-76s observed for the same test before across prior verification rounds — no material regression from going live later. Fixes the stale live_scan.rs module doc, which predated a87674b rebuilding the harness on CkErc20Setup and still claimed only the minter and EVM RPC canister were installed, with no real orchestrator or spawned ledgers. Verification: rustfmt, cargo check --all-targets --all-features, clippy (pinned flags, --all-targets) all clean. bazel test, all --nocache_test_results: cketh_test, ckerc20_test, test_utils:lib_tests all pass; deposit_from_cex passes solo and 10/10 under --runs_per_test=5 --local_test_jobs=5 (two full runs).
After with_env was removed upstream and with_live_mode went away in 7a5682b, CkEthSetupBuilder was a one-field struct with a single setter and two call sites: a constructor spelled in three calls (builder(), with_ethereum_backend(..), build()). CkEthSetup::new(env: Arc<PocketIc>) disappeared when the fixture took over building its own PocketIC instance, freeing the name (confirmed via grep: no remaining caller anywhere in rs/). CkEthSetup::new(backend: EthereumBackend) now does what CkEthSetupBuilder::build did; Default calls it with EthereumBackend::Mocked, and live_scan calls it directly with EthereumBackend::Anvil(..). Privacy is unchanged: new is private, and live_scan reaches it exactly as it reached builder before, being a descendant module of the crate root — no pub/pub(crate) added, confirmed by cargo check. EthereumBackend itself is untouched: it still drives install_args, ethereum_block_height and last_scraped_block_number, which are real Mocked-vs-Anvil differences. Verification: rustfmt, cargo check --all-targets --all-features, clippy (pinned flags, --all-targets) all clean. bazel test, all --nocache_test_results: cketh_test, ckerc20_test, test_utils:lib_tests all pass; deposit_from_cex passes 5/5 under --runs_per_test=5 --local_test_jobs=5.
Resolves the conflicts #10946 (move funded deposit addresses to a balance-sweep queue) created with this branch's rework of the live balance-scan harness. Both sides touched live_scan.rs. Conflicts and how they were resolved: - live_scan.rs imports: took master's DepositStatus, dropped AddCkErc20Token. This branch no longer registers supported tokens by hand -- CkErc20Setup does it -- so the type is unused here. - live_scan.rs balance_scan_candidates()/candidates_in_log(): took master's deletion. #10946 surfaces a funded address through DepositStatus::AwaitingSweep instead of the [balance_scan] log line those helpers parsed, and the merged deposit_from_cex.rs asserts on that status rather than on a candidate count, so nothing calls them. - tests/ckerc20.rs imports: kept only BTreeSet. master's new_pocket_ic and Arc were needed for CkErc20Setup::new_without_ckerc20_active(Arc::new(new_pocket_ic())), and on this branch the fixture builds its own PocketIC instance, so both call sites take no argument and new_pocket_ic no longer exists. await_scan came through the merge with master's DepositStatus-based body, which is what the merged deposit_from_cex.rs expects.
Summary
The live-anvil balance-scan harness had grown into a second test fixture: it created and funded canisters, sequenced their installs, and registered ckERC20 tokens on its own, in parallel with the fixtures every other cketh integration test already uses. This PR removes that duplication — the harness now builds on the shared fixtures and keeps only what is genuinely specific to it: the local Ethereum node it owns, and the balance-scan assertions.
Collapsing the two fixtures meant removing the knobs the duplicate had accumulated:
Behaviour of the mocked fixtures is unchanged: same canister creation order, same derived minter address, and the same cold-start property the deposit-flow tests depend on.
🤖 Generated with Claude Code