feat(cketh): detect deposits above minimum - #10873
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the first “filter 1” consumer of the ckERC20 deposit-address watchlist by introducing a periodic background balance scan that batches ERC-20 balanceOf checks via Multicall3 aggregate3, then exposes last-run scan stats via metrics.
Changes:
- Introduces
balance_scanmodule: builds(account, token)call sets from live deposit addresses + supported tokens, runs chunked Multicall3eth_call, and records scan stats in state. - Adds a small hand-rolled Multicall3 ABI encoder/decoder (with unit tests focused on bounds/overflow safety).
- Wires the scan into canister timers and exposes scan gauges in the HTTP metrics endpoint; adds a
live_addressesiterator on the automatic deposits watchlist.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rs/ethereum/cketh/minter/src/state/tests.rs | Updates state test construction for the new last_balance_scan field. |
| rs/ethereum/cketh/minter/src/state/automatic_deposits/tests.rs | Adds unit test coverage for yielding only live (non-expired) deposit addresses. |
| rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs | Adds live_addresses() iterator for filtering watchlist entries by expiry. |
| rs/ethereum/cketh/minter/src/state.rs | Adds last_balance_scan to state and a new TaskType::BalanceScan. |
| rs/ethereum/cketh/minter/src/main.rs | Schedules balance scan timers and exports last scan stats as metrics gauges. |
| rs/ethereum/cketh/minter/src/lifecycle/init.rs | Initializes last_balance_scan to None on init. |
| rs/ethereum/cketh/minter/src/lib.rs | Exposes balance_scan module and adds BALANCE_SCAN_INTERVAL constant. |
| rs/ethereum/cketh/minter/src/balance_scan/tests.rs | Adds unit tests for candidate counting and call-building behavior. |
| rs/ethereum/cketh/minter/src/balance_scan/multicall3/tests.rs | Adds golden-vector and adversarial decoding tests for Multicall3 ABI handling. |
| rs/ethereum/cketh/minter/src/balance_scan/multicall3.rs | Implements Multicall3 aggregate3 + ERC-20 balanceOf ABI encoder/decoder. |
| rs/ethereum/cketh/minter/src/balance_scan/mod.rs | Implements the periodic scan task, chunking logic, stats tracking, and RPC call arguments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a051913 to
ff895cd
Compare
…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>
|
✅ No security or compliance issues detected. Reviewed everything up to 7db8031. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
rs/ethereum/cketh/minter/tests/deposit_from_cex.rs:417
wait_until_readyalso performs blocking HTTP requests without a timeout. A stuck connection can make the readiness loop hang longer than the intended 30s deadline.
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())
rs/ethereum/cketh/minter/src/main.rs:1041
- The metric help text still refers to "multicall" chunks, but the balance scan now uses the deployless batcher (create-style
eth_call). This makes the exported metric documentation misleading for operators.
w.encode_gauge(
"cketh_minter_balance_scan_failed_chunks",
stats.chunks_failed as f64,
"Number of multicall chunks that failed in the last balance scan.",
)?;
rs/ethereum/cketh/minter/src/balance_scan/mod.rs:208
min_depositusesunwrap_or(Erc20Value::MAX)to represent an unsupported token, butcount_candidatesuses>=, so an unsupported token can still be counted as a candidate if its balance happens to equalErc20Value::MAX. This contradicts the function doc (“absent … never counts”).
/// Minimum balance for `token` to count as a scan candidate; a token absent from
/// [`MIN_DEPOSITS`] never counts.
fn min_deposit(token: &Address) -> Erc20Value {
MIN_DEPOSITS
.iter()
rs/ethereum/cketh/minter/tests/deposit_from_cex.rs:325
rpc_resultusesreqwest::blocking::Client::new()with no request timeout. If the local node stops responding mid-test, this can hang the test indefinitely rather than failing fast.
This issue also appears on line 413 of the same file.
fn rpc_result(&self, method: &str, params: Value) -> Result<Value, String> {
let body: Value = reqwest::blocking::Client::new()
.post(&self.url)
.json(
&serde_json::json!({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}),
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e-scan-filter-1 # Conflicts: # rs/ethereum/cketh/minter/src/lib.rs # rs/ethereum/cketh/minter/src/main.rs # rs/ethereum/cketh/minter/src/state.rs # rs/ethereum/cketh/minter/src/state/automatic_deposits/tests.rs # rs/ethereum/cketh/minter/tests/ckerc20.rs
The deposit_from_cex test targets were pinned to edition 2021 while the crate is edition 2024. rustfmt sorts a mixed Type/function import group differently across editions, so the bazel autofix (formatting the 2021 target) and `cargo fmt --check` (formatting the 2024 crate) disagreed on the batcher import order and deadlocked CI. Match the targets to the crate's edition so both formatters agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rules_rust toolchain already defaults to edition 2024, and no other rust target in the repository sets `edition` explicitly. Rely on the default instead of pinning it, which keeps the bazel rustfmt and `cargo fmt` in agreement (both format at edition 2024). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scheduling/selection layer for the ckERC20 deposit-address balance scan (DEFI-2923), **without** any balance-querying `eth_call` execution — that lands in the stacked PR dfinity#10873. Three pieces: - **Latest block height on a timer** — a dedicated task refreshes the latest Ethereum block height into `State` (queried at the `latest` tag, reduced with `MinByKey` so a divergence across providers still yields a height they all agree exists, and only advanced when it increases). This is the block the balance batcher (dfinity#10873) is pinned to (avoiding the consensus problem of querying `latest` per-provider). It is exposed via the `cketh_minter_latest_block_height` metric. - **Per-address scan state** — `DepositRequest` gains `last_scanned_block` and `scan_count`, persisted in the `RegisteredDepositAddresses` snapshot event so it survives upgrades (keeping the audit-log equivalence check intact) and surfaced through the `get_events` debug endpoint. - **Due-address selector** — `AutomaticDeposits::addresses_to_scan_iter(now, latest_block)` yields the live addresses due for a scan, using elapsed blocks × ~12s as a proxy for elapsed time against the burst→ramp→tail backoff schedule. No execution yet. No balance querying here. **Candid**: surfacing the new scan fields through `get_events` changes the return type of that endpoint, which is not backwards-compatible. `get_events` is a debug-only endpoint, so breaking its Candid interface is acceptable here — hence the `CI_OVERRIDE_DIDC_CHECK` label. ## PR stack - **dfinity#10881 (this PR)** — scan-scheduling layer — base `master` - **dfinity#10873** — balance scan filter 1 (deployless-batcher balances) — stacked on this PR dfinity#10878 (the earlier wall-clock scan-schedule) was closed as superseded by this PR. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…entry Addresses two review findings on the balance-scan schedule: - BALANCE_SCAN_INTERVAL was 60s while the first backoff gaps are 30s, so those gaps were never actually reachable. Lower the tick interval to 30s to match the smallest gap. - addresses_to_scan_iter indexed SCAN_GAP_SECS by scan_count, but the first scan takes the None branch and record_scan increments scan_count before it is ever used as an index, so scan_count was always >= 1 here and SCAN_GAP_SECS[0] was dead. Index by scan_count - 1 so the first post-registration gap uses SCAN_GAP_SECS[0] and all 33 entries are used. Retarget the schedule tests to the N -> SCAN_GAP_SECS[N-1] mapping, including the exhaustion boundary (now scan_count > number of gaps). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…or counters Addresses the review comment that the single last-run `chunks_failed` gauge lumps decode and eth_call failures together, is a gauge rather than an alertable counter, and carries stale "multicall" wording. Replace it with two cumulative in-memory counters on State (`balance_scan_decode_errors`, `balance_scan_call_errors`), exposed as the monotonic metrics `cketh_minter_balance_scan_decode_errors_total` and `cketh_minter_balance_scan_call_errors_total` with deployless-batcher wording. The counters reset on upgrade (standard Prometheus counter-reset semantics), so they are excluded from `is_equivalent_to` like the transient `last_balance_scan`. Drop `chunks_failed` from `BalanceScanStats` and the failed-chunks gauge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…DEPOSITS entry Addresses the review concern that supported_ck_erc20_tokens and MIN_DEPOSITS can silently diverge: a supported token with no MIN_DEPOSITS entry gets scanned but returns Erc20Value::MAX as its threshold, so it is never flagged and its deposits go undetected. Add a unit test that asserts every currently-deployed supported ckERC20 contract (mainnet and Sepolia minters) is present in MIN_DEPOSITS, using an independently transcribed hex list so a dropped or typo'd entry is caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the review nit on deposit_from_cex.md: state that the detection schedule is block-based (elapsed measured as elapsed_blocks * SECS_PER_BLOCK, ~12s/block) rather than wall-clock, and that the scan task fires on a fixed 30s timer. Reconcile the cadence table with the actual 33-entry SCAN_GAP_SECS (all now used): an immediate initial scan plus 33 gap-gated scans (tail is 23 hourly, not 24), 34 total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
rs/ethereum/cketh/minter/tests/deposit_from_cex.rs:418
wait_until_readyalso performs blocking HTTP requests without a timeout, so a network hang can block the readiness loop longer than intended. Use a client with a per-request timeout here as well.
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())
rs/ethereum/cketh/minter/src/balance_scan/mod.rs:94
watchlist_lencounts all entries in the underlyingTimedSizedMap, including expired-but-not-yet-evicted entries. The log message currently reads as if allwatchlist_lenentries are "ready to be scanned", which can be misleading when most/all entries are expired.
log!(
DEBUG,
"[balance_scan] SKIPPING: 0/{watchlist_len} user addresses ready to be scanned"
);
rs/ethereum/cketh/minter/src/balance_scan/mod.rs:211
min_depositclaims that a token absent fromMIN_DEPOSITSnever counts, but the current implementation returnsErc20Value::MAXandcount_candidatesuses>=. That means an unsupported/unknown token would be counted as a candidate if it ever reports au256::MAXbalance (e.g., a misbehaving ERC-20), contradicting the function contract.
fn count_candidates(calls: &[BalanceOfCall], balances: &[Erc20Value]) -> usize {
calls
.iter()
.zip(balances)
.filter(|(call, balance)| **balance >= min_deposit(&call.token))
.count()
rs/ethereum/cketh/minter/tests/deposit_from_cex.rs:320
- These
reqwest::blockingcalls use the default client configuration, which has no request timeout. Ifanvilstalls or the connection hangs, this test can block indefinitely and stall CI. Set an explicit timeout on the HTTP client (even a few seconds is enough for local anvil).
let body: Value = reqwest::blocking::Client::new()
.post(&self.url)
.json(
&serde_json::json!({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}),
)
The author decided to strip balance-scan observability from this PR and handle it holistically in a follow-up (DEFI-2965) rather than land piecemeal gauges. Remove the BalanceScanStats struct, the last_balance_scan field, the two per-kind error counter fields, the record_stats helper, and all balance-scan-stat metrics from encode_metrics (last-run timestamp, addresses scanned, candidates, decode/call error counters). Candidate detection and the per-arm INFO error logs plus a summary INFO log (from local tallies only) are kept. The scheduling metric cketh_minter_latest_block_height is unrelated and stays. Tests that asserted the removed stats/counters are retargeted to assert behaviour via the watchlist state (scanned addresses advance; a failed chunk is not advanced). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adapt #10946's event-sourcing to the updated #10873, which removed all balance-scan metrics, and drop #10946's own sweep gauge — observability is deferred to DEFI-2965. Conflict resolution keeps #10946's event-sourcing (candidates grouped per account, sweep_moves + process_event(MovedToSweepQueue), record_scan for the rest) on top of #10873's removals (no BalanceScanStats, no record_stats, no State error counters). Removes the cketh_minter_sweep_queue_size gauge from encode_metrics. The sweep integration test now observes the move solely via get_events (one MovedToSweepQueue event per funded (account, token), surviving the pre/post-upgrade replay) and the watchlist re-registration check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ty#10947) 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: - A new PocketIC *live* harness in the cketh `test_utils` crate owns a local `anvil` node and installs the minter and the EVM RPC canister, configuring the latter with an `overrideProvider` that 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 the `StateMachine` that cannot issue real HTTP request. Migrating the integration test infrastructure to PocketIC is deferred to DEFI-2262 (see dfinity#10949). - Supported tokens (ckUSDC, ckUSDT) are registered directly through the minter's `add_ckerc20_token` endpoint by pointing its orchestrator id at a principal the harness controls, so no real orchestrator or spawned ledgers are needed. - The harness places the two tokens at their real mainnet addresses on anvil (`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 dfinity#10873. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>
A background task periodically reads the on-chain ERC-20 balance of the armed deposit addresses that are due for a scan and flags the ones at or above a per-token minimum as deposit candidates (reported via metrics). A balance is only a cheap scheduling hint — it triggers work but never credits; filter 2 (DEFI-2924) re-verifies against finalized logs before any mint. Nothing runs downstream yet.
Highlights:
eth_call(toomitted) that runs a small program returning them as a compact flat array — ~5× less response overhead and a trivial decode, with no deployed contract. The shipped bytecode is pinned by a byte-for-byte assembly golden and its behaviour verified on a local anvil node (balances across tokens/holders; fail-loud on a bad token), in addition to end-to-end validation against Ethereum mainnet across all four providers the minter uses and through the deployed EVM-RPC canister.deposit_erc20now returns each address'last_scanned_blockandscan_count, and an end-to-end test exercises register → scan → observe.Deferred: filter 2, blocklist/dedup and crediting (DEFI-2924); exposing the minimum to users (DEFI-2962).
PR stack
master#10878 (the earlier wall-clock scan-schedule) was closed as superseded by #10881.