feat(cketh): per-(deposit address, ERC-20) deposit granularity - #11128
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes ckERC20 automatic deposit registration and processing from per-deposit-address to per (deposit address, ERC-20 token) granularity, aligning scanning, status reporting, audit events, and docs with the new per-token deposit UX while keeping a shared derived address per account.
Changes:
deposit_erc20now requires an expliciterc20_contract_addressand validates it is a minter-supported ckERC20.- Automatic deposit watchlist/scanning/sweep-queue are keyed by
(account, token);AwaitingSweepand related events carry a single detected deposit per token. - Tests, Candid, and the
deposit_from_cex.mdspec are updated to reflect the new per-pair behavior and event shapes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/ethereum/cketh/test_utils/src/live_scan.rs | Updates test helper APIs to register/await scans per (caller, token) and pass erc20_contract_address. |
| rs/ethereum/cketh/test_utils/src/ckerc20.rs | Extends test deposit flow helper to include erc20_contract_address in DepositErc20Arg. |
| rs/ethereum/cketh/minter/tests/dump_stable_memory.rs | Updates event mapping to new per-token registration and per-pair AutomaticDepositReceived payload. |
| rs/ethereum/cketh/minter/tests/deposit_from_cex.rs | Adjusts integration test expectations for per-token AwaitingSweep and per-token registration. |
| rs/ethereum/cketh/minter/tests/ckerc20.rs | Updates minter endpoint tests for new arg shape and per-pair scan/sweep semantics. |
| rs/ethereum/cketh/minter/src/state/tests.rs | Updates state equivalence test setup to arm deposits per (account, token). |
| rs/ethereum/cketh/minter/src/state/event.rs | Reshapes audit event payloads for per-pair deposits (adds token, removes per-event deposits list). |
| rs/ethereum/cketh/minter/src/state/automatic_deposits/tests.rs | Refactors unit tests to cover per-pair watchlist/sweep behavior and per-account token caps. |
| rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs | Implements per-pair watchlist keys, per-account token cap, scan targeting, and per-pair sweep queue. |
| rs/ethereum/cketh/minter/src/state/audit/tests.rs | Updates audit test parsing/mapping for new event and registration shapes. |
| rs/ethereum/cketh/minter/src/state.rs | Adds is_supported_ckerc20 and threads token through deposit registration. |
| rs/ethereum/cketh/minter/src/main.rs | Validates erc20_contract_address, checks support, and registers/reports status per (account, token). |
| rs/ethereum/cketh/minter/src/endpoints.rs | Updates Candid types for new deposit arg, per-token AwaitingSweep, and new error variants. |
| rs/ethereum/cketh/minter/src/balance_scan/tests.rs | Refactors scan tests to per-pair calls/chunking and updated watchlist interactions. |
| rs/ethereum/cketh/minter/src/balance_scan/mod.rs | Refactors balance scan to scan only registered (address, token) pairs and emit per-pair events. |
| rs/ethereum/cketh/minter/cketh_minter.did | Updates DID to new DepositErc20Arg, DepositStatus, and event payload shapes. |
| rs/ethereum/cketh/docs/deposit_from_cex.md | Updates spec narrative and diagrams for per-pair registration/scanning/status/sweep behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3522f25 to
51d05ea
Compare
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t, token)
Introduce DepositKey{account, token} as the unit of registration, scanning, and
sweeping; the sweep queue holds one entry per key. Cap the armed tokens per account
at MAX_TOKENS_PER_ACCOUNT=5.
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>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Copilot review: the global cap counts armed (account, token) pairs; fix the spec's deposit_erc20 signature and the renamed scan_targets_iter reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename the `token` field to `erc20_contract_address` in the three Candid types that expose an ERC-20 contract address: `DetectedDeposit` and the `RegisteredDepositAddresses` / `AutomaticDepositReceived` event payloads. This matches the naming already used by `deposit_erc20` and `MintedCkErc20`. The internal `state::event` types keep `token: Address`, which is unambiguous in Rust and CBOR-indexed, so the stable-memory encoding is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow the behaviour `withdraw_erc20` already has for its inputs: * An `erc20_contract_address` that cannot be parsed as an Ethereum address now traps instead of returning `DepositErc20Error::InvalidErc20ContractAddress`. * An address that parses but is not a supported ckERC20 token now returns `DepositErc20Error::TokenNotSupported`, which carries the list of supported tokens, instead of the empty `DepositErc20Error::UnsupportedCkErc20Token`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The deposit flow constantly handles two `Address` values with opposite
meanings: the ERC-20 contract of the token being deposited, and the EOA
the minter derives for the depositing account. Sharing one type made
every place they meet a silent-swap hazard that still compiled —
`watch_deposit(.., token, address)`, `BalanceOfCall { token, holder }`,
`ScanTarget::token()` next to `ScanTarget::address()`, and a blanket
`From<Address> for DepositRequest`.
Lift only the derived address into a `DepositAddress` newtype, leaving
the ERC-20 side as a plain `Address`. Tagging one of the pair is enough
to make a transposition a compile error, and it is the side with the
smaller, newer blast radius.
`DepositAddress` is `#[cbor(transparent)]` over `Address`, which is
itself transparent over its bytes, so the recorded event log encoding is
unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename `token` to `erc20_contract_address` in `AutomaticDeposit` and `DepositAddressRegistration`, so the internal event structs match the Candid types they mirror. The CBOR indices are unchanged, so the recorded event log encoding is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DepositKey` named the type after its role as a map key rather than after what it is: the `(account, token)` pair a caller asks the minter to watch via `deposit_erc20`. Rename it to `DepositRequest`, mirroring `WithdrawalRequest` on the withdrawal side. That name was taken by the watchlist *value*, which holds a deposit address and the backoff-schedule state rather than anything a caller requested, so rename it to `ScanProgress`. The watchlist now reads `TimedSizedMap<DepositRequest, ScanProgress>`. Bindings, parameters and `ScanTarget`'s field follow the types, and `DepositRequest` gains a doc note that nothing has been deposited yet — the request only arms the pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
rs/ethereum/cketh/minter/src/endpoints.rs:261
- The documentation for
DepositStatusstill refers to a “deposit address”, but the API is now keyed per(deposit address, ERC-20 token)pair (including scan count / last scanned block). Updating these doc comments will make the Candid/Rust API clearer for consumers.
/// The stage a ckERC20 deposit address is at. Extensible with `Sweeping`/`Swept`
/// once sweeping lands (DEFI-2924).
#[derive(CandidType, Deserialize, Clone, Debug, Eq, PartialEq)]
pub enum DepositStatus {
/// Armed and being scanned; no deposit at or above the minimum detected yet.
Scanning {
/// Timestamp in nanoseconds since the Unix epoch until which a deposit
/// sent to the address is guaranteed to be noticed by the minter.
valid_until: u64,
/// The latest Ethereum block at which the address' balance was scanned,
/// or `None` if it has not been scanned yet.
last_scanned_block: Option<Nat>,
/// How many times the address' balance has been scanned so far.
scan_count: u64,
4e69c35 to
3e4767c
Compare
`deposit_erc20` reads a pair's status, returns early if it is already armed or funded, and otherwise awaits the minter's ECDSA public key before arming it. The await breaks the atomicity of that check: a second call from the same principal could arm the pair, and a balance scan move it to the sweep queue, while the first call is suspended. The first call would then arm a pair that is already awaiting sweep, leaving it in both the watchlist and the sweep queue — and the next scan to detect those same funds would trip the duplicate-entry assertion in `record_automatic_deposit_received`, trapping the scan on every tick until the entry expires. Take a per-principal guard for the whole call, as `withdraw_erc20` already does. Arming a pair requires a `deposit_erc20` call from that account's owner, so serializing per principal removes the only way the pair can change underneath a suspended call. The guard reports no pending-request count: a registration completes within the call rather than joining a backlog, and how many pairs may be armed is already bounded by the watchlist capacity and the per-account token cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pair must never sit in the watchlist and the sweep queue at once: a scan would then detect the same funds a second time and trip the duplicate-entry assertion in `record_automatic_deposit_received`. `watch_deposit` owns both maps but only consulted the watchlist, leaving the invariant to its caller. Assert it where the maps live. `deposit_erc20` already rules this out — it reports an already-funded pair from `deposit_status` and holds a per-principal guard so that check cannot go stale across its await — so this is a backstop against a future caller reopening the hole rather than a reachable path. Unlike its sibling, this assertion cannot trap `post_upgrade`: replay rebuilds the watchlist via `rebuild_watchlist` and never calls `watch_deposit`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 9dab822. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay, just a few minor comments!
`deposit_erc20` checks `deposit_status`, then awaits the ECDSA public key before registering. A concurrent balance scan can detect a deposit and move the pair into the sweep queue during that await (only possible right after an upgrade, before the key is cached), after which `register_deposit_address` would trap in `watch_deposit` (the pair is already queued for sweeping). The call would roll back and self-heal on retry, but that is a needless trap. Re-check `deposit_status` after the await and return the pair's status if it is now queued; from there the call is synchronous, so no scan can interleave before the registration. Raised in review of #11128. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename `DepositErc20Error::TooManyActiveAddresses` to `TooManyActiveDeposits`, matching the earlier `MAX_ACTIVE_DEPOSITS` rename. - Refresh the stale `MAX_CALLS_PER_BATCH` doc to describe `scan_balances`' per-chunk all-or-nothing behaviour. - Fix a missing word in a `balance_scan` test comment. - Use `DEFAULT_USER_SUBACCOUNT` instead of the bare `[42u8; 32]` literal in the ckerc20 deposit test. Raised in review of #11128. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
Automatic ERC-20 deposit detection was keyed at the granularity of a deposit address: one
deposit_erc20call armed an address, and the balance scan read every armed address against every supported ERC-20 in gas-capped batches. That per-address unit is the source of most of the machinery (keeping an address's per-token calls in one batch, advancing a whole address, dropping all of an address's tokens the moment any one funds) and a status wart (AwaitingSweeplumping every token together).This PR makes the natural finer unit — (deposit address, ERC-20 token) — the unit of registration, scanning, status, and sweeping, which is also the standard CEX deposit UX: pick the token, the network is always Ethereum, then show the (shared) deposit address.
Nothing here is live yet
The automatic-deposit feature as a whole —
deposit_erc20and the balance scan behind it — has never shipped to mainnet. No deployed minter exposes the endpoint, so no production event log contains any of its events and no client calls it.That is what makes the rest of this PR cheap, and it is worth keeping in mind while reviewing:
What
deposit_erc20takes an expliciterc20_contract_address, required to be a minter-supported ckERC20. The deposit address stays shared across a caller's tokens.(account, token); funding one token no longer removes its siblings. Per-account cap of 5 concurrently armed tokens.(address, token)pairs — the cartesian batching (split-assert, whole-address advance) is gone; each pair is onebalanceOfcall.AwaitingSweepnow carries a single detected deposit; the automatic-deposit events carry one entry per token.deposit_from_cex.mdspec is updated to match.Review follow-ups
erc20_contract_addressrather thantoken, matching the existingdeposit_erc20argument and the minted-token event. The internal event payloads follow suit, so the two layers no longer disagree.deposit_erc20's failure handling now matcheswithdraw_erc20: an unparseable contract address traps, and an unsupported token returns the list of tokens the minter does support instead of an empty error.(account, token)pair and the scan bookkeeping stored against it were named the wrong way round; they now say what they are, mirroringWithdrawalRequeston the withdrawal side.Rebased onto master now that #10946 has merged. Tracks DEFI-2976.