test(cketh): end-to-end balance scan against a live anvil node - #10947
Conversation
…s cadence) Adds the scheduling/selection layer for the ckERC20 deposit-address balance scan, without any Multicall/eth_call execution (a later PR): - a dedicated timer refreshes the latest Ethereum block height into state (MinByKey reduction, divergence-tolerant at the latest tag); - DepositRequest gains last_scanned_block + scan_count, persisted in the RegisteredDepositAddresses snapshot event (so upgrade equivalence holds); get_events Candid is unchanged (no .did change); - AutomaticDeposits::addresses_due_for_scan selects the live addresses due for a scan, using elapsed blocks x ~12s as a proxy for the burst/ramp/tail backoff schedule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… mapping Fixes the --all-targets / Bazel Test All build: the integration test's candid->event mapping constructs DepositAddressRegistration and needs the new last_scanned_block/scan_count fields (Candid does not carry them -> None). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Guard the latest block height update in refresh_latest_block_height so the state is only mutated when the newly fetched block number is strictly greater than the previously known one. Log a warning when the fetched block number regresses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ng test The refresh_latest_block_height timer issues a parallel eth_getBlockByNumber query at the latest tag alongside the scraper's finalized query. Answer both in should_be_able_to_stop_canister_during_scraping so the refresh outcalls do not linger as open call contexts, and surface the open outcalls in the assertion message via a now-public debug_http_outcalls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The refresh_latest_block_height timer adds concurrent timer work after advancing time, so the reimbursement ledger mint needs an extra execution round before it is reflected in the caller's balance in should_error_when_minter_fails_to_burn_ckerc20_and_reimburse_cketh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a cketh_minter_latest_block_height gauge so the block height used to
schedule balance scans is observable, and an integration test that drives
the refresh timer through mocked eth_getBlockByNumber("latest") responses
and asserts via MetricsAssert that the metric advances on a higher block
and never regresses on a lower one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kNumber The scan-due check converts elapsed blocks into an elapsed duration to compare against the per-address backoff schedule. Represent that duration as u64 seconds, matching SECS_PER_BLOCK and SCAN_GAP_SECS, instead of overloading BlockNumber for a value that is not a block height. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The scan_count field on the RegisteredDepositAddresses event was optional only to decode registrations emitted before scan scheduling existed. Since that event type has never been deployed, no such events exist, so make scan_count a plain u32 (matching DepositRequest) and drop the corresponding backward-compatibility test. last_scanned_block stays optional as None still denotes a never-scanned address. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drive the refresh with explicit ticks around the mocked latest-block response instead of stopping ongoing outcalls and draining MAX_TICKS, so the single refresh is answered deterministically without the extra outcall-quiescing dance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PC mock tick_until_next_http_request only compared the JSON-RPC method, so a stub constrained by request params (e.g. eth_getBlockByNumber "latest") could stop ticking as soon as a same-method call with different params (e.g. "finalized") was in flight, then fail to find its target. Wait on the full matcher instead, and drop the manual pre-ticks that worked around this in the latest block height test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the four tests exercising addresses_due_for_scan into a dedicated addresses_due_for_scan submodule with explicit imports of the shared test helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add last_scanned_block and scan_count to the RegisteredDepositAddresses event returned by get_events so the balance-scan scheduling metadata is observable, and rename its addresses field to registrations to match the internal registry and reflect that each entry is a full registration record, not just an address. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename the local `previous_lastest_block_number` to `previous_latest_block_number`, addressing a Copilot review note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a periodic background task that reads the on-chain ERC-20 balance of every armed deposit address in one Multicall3 aggregate3 eth_call (at the latest block, reduced with AnyOf as a scheduling hint) and counts the (account, token) pairs at or above a placeholder minimum as deposit candidates. This is filter 1 of the deposit-detection funnel; it does nothing downstream yet (filter 2 / crediting is a later PR) and only surfaces scan stats via metrics. Includes a hand-rolled Multicall3 aggregate3 + balanceOf ABI encoder/ decoder (no new dependency), a live-only watchlist accessor, and the task/timer/metrics wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cargo clippy --all-targets (CI) flags 1*32 as clippy::identity_op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tcher Filter 1 now reads ERC-20 balances via a create-style eth_call (`to` omitted) that runs a fixed ~163-byte init-code "balance batcher": it takes the (token, holder) pairs as appended calldata, STATICCALLs balanceOf for each, and returns the balances as a flat uint256[] (a reverting or non-contract call reads back as 0, mirroring aggregate3's allowFailure). This replaces the Multicall3 aggregate3 path, whose (bool, bytes)[] return wrapped each 32-byte balance in ~160 bytes of ABI framing (~5x response overhead and a nested-offset decode). The batcher's flat return is 32 bytes per result with a trivial fixed-width decode and needs no deployed contract. The approach was validated against Ethereum mainnet: byte-identical results across all four providers the minter uses, with the EVM-RPC canister forwarding an absent `to` unchanged. The docs spec is updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire filter 1 to the scheduling layer instead of scanning every armed address every tick: - select via addresses_to_scan_iter(now, latest_block_height) so only addresses due per the backoff schedule are scanned, skipping the tick until the latest block height has been refreshed; - pin the batcher eth_call to that block height (BlockTag::Number) so every provider reads the same block and the scanned block is known; - after a successful scan, advance each scanned address' schedule via a new AutomaticDeposits::record_scan (last_scanned_block, scan_count), backed by a new TimedSizedMap::get_value_mut; failed chunks are retried next tick; - chunk by address so an address' per-token calls never straddle a chunk boundary, keeping the advance all-or-nothing per chunk. Removes live_addresses, now subsumed by addresses_to_scan_iter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `last_scanned_block` and `scan_count` to DepositErc20Response so the per-address scan schedule progress is observable through the same endpoint that registers/looks up a deposit address. This makes the balance scan end-to-end testable: register an address, run a scan tick, then re-query deposit_erc20 to see the schedule advance. Adding fields to an output record is candid-compatible (a subtype), so no new backwards-incompatibility beyond the stack's existing get_events change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ress Add an eth_call mock (JsonRpcMethod::EthCall) and a balance_scan_response helper that encodes the deployless batcher's flat uint256[] return (a flat list of Uint tokens, no ABI array header), reusing ethers-core. Extract the refresh-latest-block helper into CkErc20Setup::refresh_latest_block and add CkErc20Setup::run_balance_scan, then use them in a new integration test that registers a deposit address, refreshes the latest block height, runs one balance-scan tick, and asserts deposit_erc20 reports the address as scanned once at that block height. The existing latest-block-height metric test now reuses the extracted helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the single placeholder minimum with a hard-coded per-token table (MIN_DEPOSITS) covering every ckERC20 token the mainnet and Sepolia minters support, each worth about 0.005 ETH (5e15 wei) at a rate snapshot. The candidate count now looks up the minimum per token (a token absent from the table never counts); tokens with mismatched decimals (e.g. 6-decimal ckUSDC vs 18-decimal ckPEPE) get sensible, comparable thresholds. The static table is a placeholder; DEFI-2961 tracks refreshing it daily from the exchange-rate canister. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The token list is a trusted whitelist, so a balanceOf that reverts or does not return exactly 32 bytes (e.g. a non-contract address) is an anomaly, not "no balance". Instead of masking it as 0 (which would look like an empty address and wrongly advance its scan schedule), the batcher now REVERTs the whole eth_call on any failed/short sub-call. It surfaces as a chunk error (logged + chunks_failed metric), and the affected addresses are retried next tick rather than recorded as scanned-empty. Re-validated against mainnet: happy path matches individual balanceOf; a reverting token and a non-contract token both revert the whole call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`BALANCE_OF_SELECTOR` is only referenced by the batcher's initcode test, so in the non-test lib build it tripped `cargo clippy --all-targets` with "constant is never used". Gate it on cfg(test). Also document why the balance scan chunks by whole addresses (the per-address scan-state advance is all-or-nothing, so an address is never split across chunks), addressing a review comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the spuriously precise per-token MIN_DEPOSITS thresholds with round token amounts close to $10 (e.g. 10 USDC, 1 LINK, 3.5M PEPE), so the minimums are easier to reason about. The values remain a hard-coded rate snapshot pending the exchange-rate-canister recompute (DEFI-2961). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the select helper with inline reads in balance_scan that short-circuit on each precondition (unknown latest block, no supported ERC-20 tokens, no addresses due) and log a DEBUG reason for the skip, reporting how many of the watchlisted addresses were ready. Add AutomaticDeposits::watchlist_len for the count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> # Conflicts: # rs/ethereum/cketh/minter/src/balance_scan/mod.rs
Move the anvil node client and its ABI/solc helpers out of live_scan into a dedicated ic_cketh_test_utils::anvil module, shared by both the standalone batcher tests and the live-scan harness. Also close a balance-scan race in credit_deposit: write every token balance before placing any code, so the fail-loud batcher only advances an address once all balances are in place — a concurrent scan can no longer observe a partially-credited address (addresses the review comment on candidates_found nondeterminism). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clean-code pass over the modules added in this PR: - name the harness controller principal via a `controller()` helper instead of an inline magic value, which also lets the install helpers drop the repeated argument (G16, F1). - tighten the anvil client's visibility: keep `pub` only what the standalone batcher tests call, demoting the crate-internal methods to `pub(crate)`/private (G8). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to b508bce. Security Overview
Detected Code Changes
|
…e-scan-e2e-anvil # Conflicts: # rs/ethereum/cketh/docs/deposit_from_cex.md # rs/ethereum/cketh/minter/BUILD.bazel # rs/ethereum/cketh/minter/src/balance_scan/mod.rs # rs/ethereum/cketh/minter/src/balance_scan/tests.rs # rs/ethereum/cketh/minter/src/lib.rs # rs/ethereum/cketh/minter/src/main.rs # rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs # rs/ethereum/cketh/minter/src/state/automatic_deposits/tests.rs # rs/ethereum/cketh/minter/tests/deposit_from_cex.rs
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay!
Drive the end-to-end anvil scan with three independent depositors — 20 USDT, 15 USDC and 1 USDT — so it reads several addresses and tokens and must apply the per-token minimum to each. Only the two at-or-above-minimum deposits are flagged as candidates; the 1 USDT deposit is scanned but, below the ~$10 minimum, is not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new anvil JSON-RPC client uses blocking HTTP requests without timeouts, which can hang CI indefinitely if the local node becomes unresponsive.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (5)
rs/ethereum/cketh/test_utils/src/anvil.rs:194
- Like
rpc_result, this readiness probe has no HTTP timeout and can hang forever on a stuck TCP connect/read, preventing the 30s deadline from being enforced. Add a short request timeout so the loop can retry/fail fast.
let ready = reqwest::blocking::Client::new()
.post(url)
.json(&serde_json::json!({"jsonrpc": "2.0", "id": 1, "method": "eth_blockNumber", "params": []}))
.send()
.map(|r| r.status().is_success())
.unwrap_or(false);
rs/ethereum/cketh/test_utils/src/live_scan.rs:10
- This docs block says the EVM RPC canister issues "genuine HTTPS outcalls", but the configured
overrideProviderreplacement is the anvil URL (currentlyhttp://127.0.0.1:...). Updating the wording avoids confusion when reading the test harness.
//! harness runs PocketIC in *live* mode so the EVM RPC canister issues genuine HTTPS outcalls, and
//! installs it with an `overrideProvider` that rewrites every provider URL to the harness' anvil
//! node (mirroring the `evm_rpc_local` configuration of the EVM RPC canister). The minter therefore
//! reads real Ethereum state from anvil, exercising the balance scan end to end: minter → EVM RPC
//! canister → anvil.
rs/ethereum/cketh/test_utils/src/anvil.rs:64
reqwest::blocking::Clientdefaults to no request timeout; if anvil becomes unresponsive, this JSON-RPC call can hang indefinitely and stall CI. Add an explicit per-request timeout.
This issue also appears on line 189 of the same file.
let body: Value = reqwest::blocking::Client::new()
.post(&self.url)
.json(
&serde_json::json!({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}),
)
.send()
.unwrap()
.json()
.unwrap();
rs/ethereum/cketh/test_utils/src/live_scan.rs:2
- The harness routes providers to
Anvil::url(), which is anhttp://127.0.0.1:...URL; calling these "HTTPS outcalls" in the module docs is misleading. Consider describing this as real HTTP (or HTTP(S)) outcalls instead.
This issue also appears on line 6 of the same file.
//! A live [`PocketIc`] harness for the ckERC20 balance scan, driving *real* HTTPS outcalls against
//! a local anvil node that the harness owns (see [`crate::anvil`]).
rs/ethereum/cketh/minter/tests/deposit_from_cex.rs:182
- This comment says the scan uses "genuine HTTPS outcalls", but the live harness routes providers to an
http://127.0.0.1:...anvil URL. Consider changing this to "HTTP" (or "HTTP(S)") to match what is actually configured.
/// End-to-end balance scan against a real EVM: a live PocketIC runs the minter and the *real* EVM
/// RPC canister (configured to route every provider to the harness' anvil node), so the minter's
/// periodic balance scan issues genuine HTTPS outcalls and reads real ERC-20 balances from anvil.
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Give the blocking anvil JSON-RPC client an explicit per-request timeout so a wedged node fails fast instead of hanging a send() past wait_until_ready's deadline (and the bazel test timeout). Also reword the live-scan docs: the EVM RPC canister makes real outcalls through the IC's HTTPS-outcalls feature, but the overrideProvider routes them to the local anvil node over HTTP, so drop the misleading "HTTPS" wording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Thanks — addressed both points from the Copilot review in b0e768e:
Live |
There was a problem hiding this comment.
🟡 Not ready to approve
The new Anvil JSON-RPC helper currently panics on transport/JSON failures despite returning Result, which can reduce test reliability and debuggability for the added live end-to-end test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
rs/ethereum/cketh/test_utils/src/anvil.rs:33
rpc_client()builds a newreqwest::blocking::Clientfor every JSON-RPC call. Creating clients is relatively expensive and prevents connection reuse; this test helper makes many RPCs (including receipt polling), so caching the client reduces overhead and improves test runtime stability.
fn rpc_client() -> reqwest::blocking::Client {
reqwest::blocking::Client::builder()
.timeout(RPC_TIMEOUT)
.build()
.expect("failed to build the anvil RPC client")
}
rs/ethereum/cketh/test_utils/src/anvil.rs:81
rpc_result()is documented as returning aResult, but transport / decode failures currentlyunwrap()and panic. That defeats the caller’s ability to handle transient failures (e.g., timeouts) and also loses context about which RPC failed.
fn rpc_result(&self, method: &str, params: Value) -> Result<Value, String> {
let body: Value = rpc_client()
.post(&self.url)
.json(
&serde_json::json!({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}),
)
.send()
.unwrap()
.json()
.unwrap();
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…rt errors Build the blocking JSON-RPC client once and store it on `Anvil`, so RPCs share a connection pool instead of constructing a fresh client (and TCP connection) per call. Make `rpc_result` honor its `Result`: transport and decode failures — including an `RPC_TIMEOUT` timeout — are now returned as `Err` tagged with the failing method, rather than `unwrap()`-panicking, so callers (e.g. `eth_call_create`) can distinguish a transient failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Addressed both points from the latest Copilot review in b508bce:
Live |
There was a problem hiding this comment.
🟡 Human review recommended
It introduces a live PocketIC + anvil end-to-end test with wall-clock polling and real HTTP outcalls/process management that warrants human validation for CI reliability/flakiness.
Review details
Comments suppressed due to low confidence (1)
rs/ethereum/cketh/test_utils/src/anvil.rs:88
rpc_resultis documented as returningErrvalues “tagged with the method”, but JSON-RPC errors currently returnerror.to_string()without any method context. This makes failures harder to diagnose and contradicts the doc comment; wrap the JSON-RPC error with the method name (or update the docs to match).
match body.get("error") {
Some(error) if !error.is_null() => Err(error.to_string()),
_ => Ok(body["result"].clone()),
}
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Adds a single end-to-end test proving the ckERC20 balance scan works against a real EVM, complementing the existing mock-based integration tests.
The test drives the full production path — minter → real EVM RPC canister → Ethereum — with no JSON-RPC mocking:
test_utilscrate owns a localanvilnode and installs the minter and the EVM RPC canister, configuring the latter with anoverrideProviderthat routes every provider to that node. In live mode the EVM RPC canister issues genuine HTTPS outcalls that reach anvil for real. The new harness is required because current integration tests infrastructure uses theStateMachinethat cannot issue real HTTP request. Migrating the integration test infrastructure to PocketIC is deferred to DEFI-2262 (see test(ledger-suite-orchestrator): migrate integration tests to PocketIC #10949).add_ckerc20_tokenendpoint by pointing its orchestrator id at a principal the harness controls, so no real orchestrator or spawned ledgers are needed.anvil_setCode) and credits the minter's derived deposit address above the candidate threshold (anvil_setStorageAt). The test then asserts the minter's periodic scan reads those real balances and flags the address as a deposit candidate for both tokens.Stacked on top of #10873.