feat(cketh): give the sweeper address its own transaction pipeline - #11144
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prepares ckETH minter support for Ethereum transactions sent from the dedicated sweeper address by introducing a second, independent transaction “send lane” with its own nonce sequence, ensuring sweeps cannot head-of-line block withdrawals from the main minter address.
Changes:
- Generalizes the existing
EthTransactionsstate machine into a reusableTransactionLane<R>and keeps main-lane behavior via a type alias. - Introduces
SweepId/SweepRequest, a dedicatedState::sweeper_transactionslane, and a new timer-drivenprocess_sweeper_transactionspipeline that signs with derivation path[3]. - Extends audit/event plumbing (events
n27..n31) and Candid surface area (DID + endpoint payload mirrors) for sweeper-lane observability and replay.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/ethereum/cketh/minter/tests/dump_stable_memory.rs | Extends stable-memory dump test mapping to cover new sweeper-lane audit event payloads. |
| rs/ethereum/cketh/minter/src/withdraw.rs | Refactors RPC helpers to be sender-agnostic (reused by both lanes) and threads sender into count/finalization. |
| rs/ethereum/cketh/minter/src/sweep.rs | Adds the sweeper transaction processing task (create/sign/send/resubmit/finalize) for the sweeper lane. |
| rs/ethereum/cketh/minter/src/state/transactions/tests.rs | Updates expectations for generalized lane terminology and adds dedicated sweep-lane unit tests. |
| rs/ethereum/cketh/minter/src/state/transactions/mod.rs | Introduces TransactionLane<R>, adds SweepId/SweepRequest, and centralizes lane-generic mechanics + main-lane-only reimbursement/status logic. |
| rs/ethereum/cketh/minter/src/state/tests.rs | Updates state fixtures/equivalence tests to include new sweeper-lane state and upgrade arg field. |
| rs/ethereum/cketh/minter/src/state/event.rs | Adds new EventType variants for sweep request lifecycle and sweeper transaction lifecycle. |
| rs/ethereum/cketh/minter/src/state/audit/tests.rs | Extends audit replay test mapping for sweeper-lane events. |
| rs/ethereum/cketh/minter/src/state/audit.rs | Extends state-transition application logic to replay sweeper-lane events into state. |
| rs/ethereum/cketh/minter/src/state.rs | Adds sweeper_transactions + next_sweep_id to state and supports overriding the sweeper nonce via upgrade args. |
| rs/ethereum/cketh/minter/src/main.rs | Schedules the sweeper-lane timer task and exposes sweeper-lane events via get_events. |
| rs/ethereum/cketh/minter/src/lifecycle/upgrade.rs | Adds next_sweeper_transaction_nonce upgrade argument. |
| rs/ethereum/cketh/minter/src/lifecycle/init.rs | Initializes the sweeper lane at nonce 0 and initializes next_sweep_id. |
| rs/ethereum/cketh/minter/src/lib.rs | Exposes the new sweep module. |
| rs/ethereum/cketh/minter/src/endpoints.rs | Extends public event payload variants with sweeper-lane events. |
| rs/ethereum/cketh/minter/src/deposit_address/mod.rs | Makes sweeper_derivation_path available to the new sweeper-lane signing code. |
| rs/ethereum/cketh/minter/cketh_minter.did | Updates DID to include new upgrade arg and sweeper-lane event variants. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
94dc94f to
a3fe1e3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
rs/ethereum/cketh/minter/src/state/audit.rs:112
next_sweep_idis updated unconditionally from the accepted request’s id. If events are ever replayed out-of-order or a malformed event is recorded, this can movenext_sweep_idbackwards and allow duplicateSweepIds to be minted later. Safer to monotonically advance it (take the max of the current value andrequest.id + 1).
EventType::AcceptedSweepRequest(request) => {
state.next_sweep_id = SweepId(request.id.0.saturating_add(1));
state
.sweeper_transactions
.record_withdrawal_request(request.clone());
}
rs/ethereum/cketh/minter/src/withdraw.rs:499
fetch_finalized_receiptsis documented to returnNonewhen the batch should be retried later, but it currentlyassert_eq!s that every expected id has a receipt. If the RPC temporarily returnsOk(None)for all hashes of some finalized id (RPC lag / pruning / transient issues), this will trap the canister instead of retrying.
let actual_finalized_ids: BTreeSet<Id> = receipts.keys().copied().collect();
assert_eq!(
expected_finalized_ids, actual_finalized_ids,
"ERROR: unexpected transaction receipts for some ids"
);
Some(receipts)
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:300
- The failure log says the sweep funds “stay at the deposit address”, but sweep transactions are sent from the dedicated sweeper address (so on failure funds remain at the sweeper address). This message is misleading for operators/debugging.
log!(
INFO,
"[record_finalized_transaction]: sweep {} to {} FAILED (tx {}); sweeps are never \
reimbursed, the funds stay at the deposit address for a later sweep",
self.id,
self.destination,
receipt.transaction_hash,
);
4983cd1 to
091aac8
Compare
091aac8 to
ef55c9e
Compare
ef55c9e to
9e051b7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:482
- The
LaneRequestdoc comment says the trait also covers reimbursement behavior, but the trait itself doesn’t define any reimbursement-related API; reimbursement is handled elsewhere (e.g., onEthTransactions/WithdrawalRequest). This is misleading now thatSweepRequestalso implementsLaneRequest.
/// A request that can flow through a [`TransactionLane`]: it carries an identity used as the
/// lane's alternate map key, knows the EIP-1559 transaction it turns into, and (for lanes whose
/// failed transactions are paid back) how to reimburse a failure.
///
/// Implemented by [`WithdrawalRequest`] — the minter **main address** lane (`Id = LedgerBurnIndex`)
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:247
max_transaction_feeis only used byresubmission_strategy;to_transactiondoesn’t consult it when settingmax_fee_per_gas, so describing it as a general “ceiling on the transaction fee” is inaccurate. Either enforce the cap into_transactionor adjust the field documentation to match the current behavior.
/// Ceiling on the transaction fee, used as the resubmission fee cap.
9e051b7 to
705846a
Compare
|
✅ No security or compliance issues detected. Reviewed everything up to 2d3a6be. Security Overview
Detected Code Changes
|
de2eb00 to
6fbf7c1
Compare
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay!
30c178a to
301322f
Compare
The minter's dedicated sweeper address needs to send Ethereum transactions, and it must not share the main address' nonce sequence: a sweep stuck behind a fee-starved transaction would head-of-line-block every user withdrawal. Instantiate the transaction pipeline a second time, for the sweeper. `SweepRequest` implements `PipelineRequest` with a plain counter for identity — a sweep burns no ckETH, so there is no ledger burn index to key it by — and `State::sweeper_transactions` drives it on a nonce sequence of its own, seeded at 0 and overridable through the new `next_sweeper_transaction_nonce` upgrade argument. `SweepRequest::Error` is `Infallible`: a sweep pays its gas from the sweeper's prepaid balance rather than out of the amount moved, so it has no fee to fail to cover — and `CreateTransactionError`, keyed by a ckETH burn index, is a value it could not construct anyway. Creating a sweep transaction therefore needs no error arm at all, rather than an unreachable one. Five audit events (`n28`..`n32`) record the sweeper pipeline's transitions, with reconstruction, Candid `EventPayload` mirrors and `.did`. This is the pipeline only: no timer drives it and nothing enqueues a sweep, so the new state stays empty until the sending task lands under DEFI-2926. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`next_sweeper_transaction_nonce` could only be set on upgrade, so a fresh install always started the sweeper pipeline at nonce 0. That is right for the production minter, whose sweeper address is freshly derived, but wrong for a developer reinstalling a minter against an address that has already sent transactions: the pipeline would resubmit nonces the chain has consumed. Accept the same value in `InitArg`, mirroring `next_transaction_nonce`. It is optional where the main address' is required, because `InitArg` is replayed from the event log: a required field would be absent from every Init event already written, and the minter would trap decoding it on the next upgrade. Absent means 0, so a fresh install is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SweepRequest::assert_created_transaction` checked the destination and the amount but not the call data, which for a sweep is the delegate call that does the actual work: a `CreatedSweeperTransaction` event whose transaction moves the right amount to the right address while calling something else would have replayed without complaint. Safe to tighten now rather than later, because tightening an assertion reachable from `apply_state_transition` is only safe while no event it applies to exists: nothing enqueues a sweep, so no minter version has ever written one of these events. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`arb_upgrade_arg` hardcoded `next_sweeper_transaction_nonce: None`, so `event_encoding_roundtrip` never encoded an `Upgrade` event carrying the field — the one test that would catch a mistake in the CBOR shape of the event log, which is exactly where a mistake cannot be undone. `arb_init_arg` generates its equivalent; this makes the upgrade fixture match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`next_sweeper_transaction_nonce` was given CBOR index 12 in `UpgradeArg`, whose previous field is 10, leaving an unexplained gap at 11. The 11 belongs to `InitArg`, whose own equivalent field it is — the two structs have separate index spaces, and this one continued the wrong count. Free to renumber only because no minter version has written an `Upgrade` event carrying the field yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both event round-trip mappers spelled the Candid-to-minter `TransactionReceipt` conversion out twice, once for `FinalizedTransaction` and once for `FinalizedSweeperTransaction` — fourteen identical lines each, in files whose whole purpose is to prove the two representations agree. `map_transaction_receipt` joins `map_nat` and `map_unsigned_transaction` as one more named conversion, and the arms shrink to a call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AcceptedSweepRequest.data` was a hex-encoded `text`, while `UnsignedTransaction.data` — the other call data in the same `Event` variant — is a `blob`. Two spellings of the same thing, and the odd one out was the new one. As a `blob` the field needs no encoding on the way out and no decoding on the way back, which is what the `hex` dependency of the `dump_stable_memory` tool was for; the tool builds without it now. The event is introduced in this PR, so nothing has ever read the hex form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`destination` was documented as the delegated deposit address, which is the shape the design did not take: a sweep batches, and a transaction has one `to`, so it goes to the sweeper contract, whose batch entry point loops into every delegated deposit address the call data names. `data` had the same problem, one level down — it is that batch call, not a single-address `sweepErc20`, and never a plain transfer. Neither is the deposit helper: that one is called from inside the delegate code running in a deposit address' context, never sent a transaction of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sweeper pipeline is constructed in lifecycle::init and held as a field of State, both outside crate::state, so the pipeline and its stage cannot stay confined to that module once a second lane exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every other event variant numbers its fields from zero; nothing has written a sweeper event yet, so the numbering is still free to change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pipeline dropped the Display bound on a request id, and nothing formats a sweep id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Creating a sweep took whatever the fee estimate gave and never looked at max_transaction_fee, while resubmission refuses to exceed it, so a sweep created above its allowance could never be fee-bumped: it got stuck rather than being refused. Allocate the whole allowance up front, as the ckERC20 withdrawal path does, which keeps the sweep within its ceiling by construction and leaves the failure unrepresentable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b725698 to
978f172
Compare
Pricing the sweep at its allowance regardless of the estimate produced a transaction that could not be mined once base fee plus tip passed the allowance: it held the sweeper's nonce while the resubmission strategy refused to bump it past that same allowance, blocking every later sweep on the lane. Refuse at creation instead, as the ckERC20 withdrawal path does, so the sweep stays queued for a cheaper block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…st (dfinity#11178) ## Why The transaction pipeline served exactly one sender, the minter's main address: one nonce sequence, every map keyed by a ckETH ledger burn index, the request type baked in. A second sender address cannot reuse any of it, and giving the sweeper its own address is the point of the stack. ## What Introduces a trait for what a request must answer in order to travel the pipeline, and makes the pipeline generic over it. Withdrawals are the only implementation here, wrapped so that every existing call site and behaviour is unchanged; the sweeper's implementation arrives in dfinity#11144. The pipeline's vocabulary follows the type: it speaks of requests and ids rather than withdrawals, since another pipeline's requests are not withdrawals and its ids are not ledger burn indices. Reimbursement and withdrawal status keep their names — they belong to the wrapper and really are withdrawal concepts. No behaviour change. <sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub> [DEFI-2917]: https://dfinity.atlassian.net/browse/DEFI-2917?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps the Display impl dropped in #11144 dropped: the id only ever reaches a log line, and the trait already requires Debug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps the Display impl dropped in #11144 dropped: the id only ever reaches a log line, and the trait already requires Debug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Why
The minter's dedicated sweeper address needs to send Ethereum transactions, and it must not share the main address' nonce sequence: a sweep stuck behind a fee-starved transaction would head-of-line-block every user withdrawal.
What
A second instance of the transaction pipeline, for the sweeper address, on a nonce sequence of its own. A sweep burns no ckETH, so it is keyed by a plain counter rather than a ledger burn index, and is never reimbursed; it pays gas from the sweeper's prepaid balance, so it has no transaction fee it can fail to cover. Five audit events record the pipeline's transitions, with reconstruction, Candid mirrors and
.did.The sweeper's start nonce can be set from both lifecycle arguments. It is optional on install where the main address' equivalent is required, because install arguments are replayed from the event log and a required field would be missing from every event already written.
Scope
The pipeline only. No timer drives it and nothing enqueues a sweep, so the new state stays empty: the sending task is DEFI-2926 in #11237, and EIP-7702 first-time delegation is #11250, stacked above. The sweep-queue source and prepaid-gas gating are still to come.
Sweeper funding — burning ckETH from the minter's fee subaccount to prepay that gas — is a separate stack (DEFI-2933) that meets this one only in the withdrawal pipeline, where its request variant has already landed. Hence the audit events here are numbered from
n28, above the funding event master now owns.Candid compatibility
CI_OVERRIDE_DIDC_CHECKis set. The check flags the five cases this PR adds to theEventpayload variant, since a variant returned to callers may not grow under Candid subtyping. It is the additive shape every new audit event has taken: existing cases keep their names and fields, and callers matching exhaustively on the old set see the new ones only for sweeper activity, which nothing enqueues yet. The new optional nonce field on the two argument types is compatible on its own.Stack created with GitHub Stacks CLI • Give Feedback 💬