feat(cketh): move funded deposit addresses to a balance-sweep queue - #10946
Open
gregorydemay wants to merge 82 commits into
Open
feat(cketh): move funded deposit addresses to a balance-sweep queue#10946gregorydemay wants to merge 82 commits into
gregorydemay wants to merge 82 commits into
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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs:243
sweep_entries_for_accountcurrently scans the entire sweep queue (self.sweep.iter().filter(...)), makingdeposit_erc20O(total_sweep_entries) even thoughSweepKeyis ordered by(account, token). This can become a hot path as the sweep queue grows (especially since nothing drains it yet). Consider using aBTreeMap::rangeover the contiguous key interval for the requested account so lookup is O(log N + M).
pub fn sweep_entries_for_account(&self, account: &Account) -> Vec<DetectedSweep> {
self.sweep
.iter()
.filter(|(key, _)| key.account == *account)
.map(|(key, entry)| DetectedSweep {
token: key.token,
address: entry.address,
scanned_balance: entry.scanned_balance,
last_scanned_block: entry.last_scanned_block,
})
.collect()
}
Pure rename per review nit, no behavior change: - EventType::MovedToSweepQueue -> AutomaticDepositReceived (cbor index #[n(26)] unchanged), and the candid EventPayload variant + .did to match. - AutomaticDeposits::apply_sweep_move -> record_automatic_deposit_received. - The event payload struct SweepMove -> AutomaticDeposit so the variant reads coherently, cascading the balance_scan producer (sweep_moves -> automatic_deposits_received) and the test helpers. The internal sweep-queue names (sweep/SweepKey/SweepEntry/DetectedSweep/ sweep_entries_for_account) stay — they describe the queue, not the event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs:241
sweep_entries_for_accountcurrently iterates the entire sweep BTreeMap and filters by account, makingdeposit_erc20O(total_sweep_entries). SinceSweepKeyis ordered by(account, token), this can be implemented as aBTreeMap::rangeover just the keys for the requested account to avoid a linear scan as the sweep queue grows.
pub fn sweep_entries_for_account(&self, account: &Account) -> Vec<DetectedSweep> {
self.sweep
.iter()
.filter(|(key, _)| key.account == *account)
.map(|(key, entry)| DetectedSweep {
token: key.token,
address: entry.address,
scanned_balance: entry.scanned_balance,
last_scanned_block: entry.last_scanned_block,
})
.collect()
rs/ethereum/cketh/minter/src/state/event.rs:184
- PR description and notes mention a
MovedToSweepQueueaudit/get_events variant, but the code introducesAutomaticDepositReceived. This mismatch can confuse API consumers and reviewers; either update the PR description/design wording to match the implemented event name or rename the event consistently across the codebase and Candid.
/// A funded deposit address was found by a balance scan and moved out of the
/// watchlist into the balance-sweep queue for one `(account, token)`. Recorded
/// the moment the funds are detected, so the sweep queue is durable even across
/// an ungraceful trap (unlike the pre-upgrade snapshot).
#[n(26)]
AutomaticDepositReceived(#[n(0)] AutomaticDeposit),
rs/ethereum/cketh/minter/tests/ckerc20.rs:341
- Grammar: use "an" before a vowel sound (Automatic...).
ckerc20.cketh.get_all_events().iter().all(|event| !matches!(
event.payload,
EventPayload::AutomaticDepositReceived { .. }
)),
"below-minimum balances must not emit a AutomaticDepositReceived event"
);
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> # Conflicts: # rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs
The erc20_token rename missed the field read in the accessor, breaking the build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Change the sweep queue from BTreeMap<SweepKey, SweepEntry> to BTreeMap<Account, Vec<SweepEntry>>, folding the ERC-20 token into SweepEntry and dropping the SweepKey struct. Looking up an account's funded tokens becomes a direct map lookup instead of a full scan, while per-(account, token) idempotency is preserved on record. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sit_status Move the deposit_erc20 status logic into a single AutomaticDeposits::deposit_status method that reports AwaitingSweep or Scanning (or None when the account must be registered), inlining the sweep-entry projection and scanning-response helpers that only fed it. Key the sweep queue by a DepositAccount struct so the per-account deposit address is carried once on the key instead of repeated on every sweep entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A sweep-queue key never holds an empty entry list, so the is_empty guard on the AwaitingSweep branch is dead; remove it along with the now-unused imports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give DepositAccount custom Eq/Ord keyed on the account alone (the derived address is a pure function of it) plus a Borrow<Account> impl, so the sweep queue orders by account and can be looked up by account while carrying the address once on the key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the (Account, Address) tuples threaded through the balance-scan path with the DepositAccount struct, so addresses_to_scan_iter, ScanBatch, and plan_batches name the account and its derived deposit address as typed fields instead of tuple positions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace DepositAccount's public fields with a pub(crate) constructor and account/ address getters, so a value cannot be mutated after construction. This keeps its account-only equality, ordering, and Borrow<Account> impls sound: the key identity can never drift once the value is stored in the sweep queue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ion is trivial Restructure ScanBatch to own a Vec<ScanCall>, where each ScanCall pairs a DepositAccount with the token to query. Collecting candidates then zips the calls with the decoded balances directly, dropping the holder-index arithmetic that mapped a flat call index back to its account. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ch/ScanCall Drop the ScanBatch and ScanCall structs: the scan loop now chunks the due DepositAccounts directly with slice::chunks, builds each batch's balanceOf calls with balance_of_calls, and collect_candidates regenerates the same holder-major/token-minor (account, token) order to zip against the decoded balances. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give BalanceScan a shared (account, token) iterator backing both balance_calls (the balanceOf sub-calls to encode) and collect_balances (attributing each decoded balance to its deposit account and token), and size the batch chunk from the requested batch size. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire BalanceScan into scan(): batch the due deposit accounts, build each batch's balanceOf calls, and attribute the decoded balances back to their account and token via collect_balances. Fold the candidate map and scanned-accounts list into a single per-account map, and remove the now-unused balance_of_calls, addresses_per_chunk, collect_candidates, and Candidate helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…funded tokens
Change the AutomaticDeposit event from one record per (account, token) to a single
event per scanned account carrying a Vec of Erc20Balance { token, scanned_balance }.
Replaying one event now removes the watchlist entry once and queues every detected
token, and the balance scan emits a single event per funded account. Candid event
payload, the .did, and the sweep queue's per-token entries are updated to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t event Fix the cketh integration tests for the one-event-per-account AutomaticDeposit shape: ckerc20 now expects a single AutomaticDepositReceived event listing every funded token (rather than one per token), and dump_stable_memory maps the candid event's deposits vec into the internal Erc20Balance entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fold the private automatic_deposit builder into the balance-scan mutate loop, which reads the watchlist entry and constructs the AutomaticDeposit event inline. Drop the unit test that exercised that private helper directly; the event's contents are already covered end-to-end by the ckerc20 integration test that drives a real scan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A funded deposit address leaves the watchlist and is never re-scanned, so every (account, token) reaches the sweep queue at most once. Recording a second entry for the same token means the same funds were queued twice, so assert against it rather than silently overwriting the existing entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… test Adapt the live-scan harness to the status-based DepositErc20Response: await_scan now treats an address as scanned once it reports Scanning with a scan_count or AwaitingSweep. Tighten the end-to-end test to assert the exact per-depositor status — AwaitingSweep carrying the funded token and balance for the two at-or-above-minimum deposits, Scanning for the below-minimum one — replacing the loose scan checks and the log-parsed candidate count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gregorydemay
commented
Jul 30, 2026
- Drop the internal DEFI-2924 ticket reference from the public DepositStatus doc comment in cketh_minter.did. - Rename DetectedDeposit.amount to scanned_balance (.did, endpoint, producer and tests), since the balance may change before the sweep; matches the AutomaticDepositReceived event field. - Revert the watchlist persistence pair to its accurate names: snapshot -> watchlist_snapshot and rebuild -> rebuild_watchlist. Both only touch the watchlist (the sweep queue is event-sourced), so the generic names were misleading. - Fix a clippy::cmp_owned break in the live deposit test by comparing the candid Nat against primitives directly instead of Nat::from(..). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 3067be9. Security Overview
Detected Code Changes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the periodic balance scan finds a registered ckERC20 deposit address holding at least a token's minimum, that funded address is moved out of the time-expiring watchlist into a dedicated sweep queue in
AutomaticDeposits, keyed per account with one entry per funded token. This hands the deposit off to the future sweeper and stops re-scanning the address. A deposit address is per-account and token-agnostic, so one address can yield several sweep entries (one per funded token) in a single scan. Nothing drains the queue yet.Highlights:
AutomaticDepositReceivedaudit event — one event per scanned account, listing every funded token — durable across an ungraceful trap, replayed on upgrade to rebuild the queue, and surfaced viaget_eventslike every other event, rather than only via a pre-upgrade snapshot.deposit_erc20. The response now carries astatus:Scanning(armed, still looking) orAwaitingSweep(detected, queued — one entry per funded token, with amount and detection block), so a user can follow the multi-minute flow. A detected address is not re-armed while its funds are queued, preventing duplicate detection/registration. The status variant is extensible for the sweeper's later states (DEFI-2924).Deferred: draining/consuming the sweep queue and minting (DEFI-2924); observability metrics (DEFI-2965); the 2-of-3
NoReductionRPC client for latest-block scans (DEFI-2964).Note: adds an
AutomaticDepositReceivedvariant to theget_eventsoutput and reshapesDepositErc20Response(both on the debug/query surface of an unreleased feature), so the Candid change is gated by theCI_OVERRIDE_DIDC_CHECKlabel.See the design doc:
rs/ethereum/cketh/docs/deposit_from_cex.md.