feat(cketh): add the SweeperFunding withdrawal-request variant - #11072
feat(cketh): add the SweeperFunding withdrawal-request variant#11072mbjorkqvist wants to merge 7 commits into
Conversation
mbjorkqvist
changed the title
mbjorkqvist
added
the
CI_OVERRIDE_DIDC_CHECK
There was a problem hiding this comment.
Pull request overview
Adds the third withdrawal variant for sweeper gas funding while preventing reimbursement.
Changes:
- Adds stable-event, Candid, dashboard, and status support.
- Reuses ckETH transaction creation, gas limits, and resubmission.
- Excludes funding requests from reimbursement and adds focused tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cketh_minter.did |
Exposes the funding event. |
src/dashboard.rs |
Displays funding requests. |
src/dashboard/tests.rs |
Adapts reimbursement tests. |
src/endpoints.rs |
Defines the public event payload. |
src/main.rs |
Maps funding into queries and events. |
src/state.rs |
Accounts for finalized funding transactions. |
src/state/audit.rs |
Replays funding events. |
src/state/audit/tests.rs |
Maps funding test events. |
src/state/event.rs |
Adds the stable event variant. |
src/state/tests.rs |
Extends event generators and helpers. |
src/state/transactions/mod.rs |
Implements funding transaction behavior. |
src/state/transactions/tests.rs |
Tests fees, resubmission, and reimbursement exclusion. |
src/withdraw.rs |
Assigns the plain-transfer gas limit. |
tests/dump_stable_memory.rs |
Decodes funding events from dumps. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:645
- This conditional also removes in-flight funding from the existing
cketh_oldest_incomplete_eth_withdrawal_request_age_secondsmetric.oldest_incomplete_withdrawal_timestamponly scans pending requests plusmaybe_reimburse; once a funding transaction is created it is in neither collection, so the gauge reports no incomplete request even while that transaction remains created or sent. Keep funding out of reimbursement, but derive incomplete requests independently (for example, from pending requests plus processed requests without a finalized transaction).
if is_reimbursable {
assert!(self.maybe_reimburse.insert(withdrawal_id));
}
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
On the suppressed comment (state/transactions/mod.rs:645, the cketh_oldest_incomplete_eth_withdrawal_request_age_seconds gauge)
The mechanics are exactly as described, and worth confirming: oldest_incomplete_withdrawal_timestamp chains withdrawal_requests_iter() with maybe_reimburse_requests_iter(), so once a funding's transaction is created it is in neither collection and contributes nothing to that gauge while it sits created or sent.
I am deliberately not changing it, for three reasons.
The gauge is about user withdrawals, and it is alerted on as such. Its name and help text say "ETH withdrawal request", and there is a stuck-withdrawal runbook attached to the alert (DEFI-2756). A minter-internal gas top-up appearing there would route an operator to a runbook about a user's stuck funds, for something no user is waiting on.
The user-facing alert still fires when a wedged funding actually hurts. A funding that never finalizes holds its nonce, which head-of-line blocks every later withdrawal. Those withdrawals are in maybe_reimburse or still pending, so they age and the alert fires — on the user-visible symptom, which is what it exists for. The only case it misses is a wedged funding with no user withdrawals behind it, where nothing user-facing is wrong yet.
That remaining case has its own metric, in PR 6 of this stack. cketh_minter_sweeper_in_flight_funding_age_seconds tracks a funding from acceptance through finalization, covering precisely the created and sent phases. Its own doc comment records why it cannot be folded into the balance-age gauge: the funding task refreshes the balance observation before consulting the in-flight guard, so that age resets every tick regardless.
The alternative you suggest — deriving incomplete requests from pending plus processed-without-a-finalized-transaction — would also change what the gauge reports for user withdrawals, on a metric that already has an alert and a runbook pointed at it. That is a behaviour change to production alerting, and it does not belong in the PR that introduces the variant.
Worth naming the one real cost: between this PR merging and PR 6 merging, a wedged funding has no age metric at all. That window is transient given the merge order, and the alert conditions for all six funding metrics are now recorded on DEFI-2965.
|
✅ No security or compliance issues detected. Reviewed everything up to f8b70da. Security Overview
Detected Code Changes
|
…ity#11060) Part of [DEFI-2933](https://dfinity.atlassian.net/browse/DEFI-2933) (sweeper fee funding), first of a seven-PR stack. ## Why Funding the sweeper address with gas requires knowing how much gas it already holds. The EVM RPC canister exposes no endpoint for a native ETH balance, and its Rust client offers no getter for one, so the minter currently has no way to ask. ## What Reads the balance through the EVM RPC canister's generic JSON-RPC passthrough, which forwards a payload to every provider and agrees on one answer under the configured consensus strategy. That strategy is a threshold of the providers — 3 of 4 on mainnet, 2 of 4 on Sepolia — and it is the only agreement accepted: there is no client-side reduction, so a result the canister reports as inconsistent stays an error rather than being resolved by picking a winner. Because the canister deserializes each response's `result` field, what the minter receives is the quantity itself rather than any surrounding JSON. It is therefore decoded exactly: quotes, padding, leading zeros and sign characters are the provider's own malformation and are rejected rather than repaired. A failed read is an error, never a zero. This is the decision the rest of the stack depends on: confusing "could not read the balance" with "no gas left" would burn ckETH to top up an address that is already funded, which is pure loss. The request builder and the result decoder are pure functions so both sides of that guarantee are pinned directly, including a test asserting that no error input can decode to a zero balance. The route was also proven end to end against a live EVM RPC canister and a local anvil node, reaching 3-of-4 consensus at both `latest` and `finalized`. ## Stack Merge in order; each PR targets the one above it. | # | PR | Status | |---|----|--------| | 1 | Read a native ETH balance via the EVM RPC canister | **this PR** | | 2 | dfinity#11065 — Burn ckETH from the minter's own fee subaccount | ready for review | | 3 | dfinity#11072 — Add the SweeperFunding withdrawal-request variant | ready for review | | 4 | dfinity#11083 — Burn-first accounting for sweeper fee funding | ready for review | | 5 | dfinity#11086 — Sweeper fee-funding task, with an end-to-end test | Copilot re-review pending, CI green incl. long tests | | 6 | dfinity#11094 — Sweeper funding observability and the prepaid-gas gate | open | | 7 | dfinity#11097 — Adversarial end-to-end coverage of sweeper fee funding | open | [DEFI-2933]: https://dfinity.atlassian.net/browse/DEFI-2933?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // Transaction failed and will not be reimbursed. | ||
| Failed : EthTransaction; |
There was a problem hiding this comment.
That's a breaking change:
- TxFinalizedStatus -> RetrieveEthStatus return type of the endpoint
retrieve_eth_status
I think that an ETH transaction to an address controlled by the minter cannot fail. Generally the only way an ETH transaction can fail (meaning it's included in a block but its execution was reverted) is that the destination address has some smart contract code installed so that the 21k gas is not sufficient. This is not the case here. Something definitely to double check.
There was a problem hiding this comment.
You are right on both counts. Reverted in 51c3ccf — the variant, the .did line, the status branch and its tests are gone, and I dropped [override-didc-check] from the title, since needing that tag was the tell I should have followed up on rather than worked around.
On the breaking change: TxFinalizedStatus is reachable from retrieve_eth_status and from the withdrawal-status path, and adding a case to a variant in return position is not a Candid subtype — a client generated against the old interface cannot decode the new tag.
On whether a funding transaction can fail, I checked rather than assumed, and it cannot:
- The destination is
sweeper_address(), derived from the minter's master threshold-ECDSA key (deposit_address/mod.rs:48), so it is an EOA the minter controls with no code installed. - The transaction is a plain value transfer —
data: Vec::new(), empty access list, the 21'000 intrinsic gas — so there is no execution to revert. A gas limit below intrinsic cost means the transaction is never mined, rather than mined and failed. - An EOA acquires code only through an EIP-7702 authorization signed by its own key, which only the minter holds, and landing CREATE2 code on one chosen address is infeasible.
So the branch was dead code bought with a breaking interface change. A failed funding now reports the pre-existing pending-reimbursement status. That is imprecise for a request nothing will reimburse, so the site carries a comment saying why it is tolerable and what would invalidate it — funding routed through a contract, where a revert becomes possible. Worth noting because the demo already contains CkSweeperViaHelper.sol, so that is not a hypothetical direction.
Visibility is unchanged: record_finalized_transaction already logged this case, and I only sharpened the wording to UNEXPECTED: … which should be impossible for a transfer to an address the minter controls, since at INFO it otherwise reads like a routine outcome. INFO is the loudest sink the minter has.
One alternative I considered and did not take: making funding requests reimbursable, crediting {minter, 0fee} where the ckETH was burned from. That would make the status honest without any new API surface, but it changes the burn-first invariant in #11083, since a reimbursement credits ckETH back. Happy to go that way instead if you would rather not rest on an impossibility argument.
is_reimbursable() stays, incidentally — it drives the maybe_reimburse bookkeeping and predates the reverted commit.
The ckETH ledger credits withdrawal fees to a subaccount of the minter, and the sweeper funding that follows spends from it. Naming it here rather than repeating the bytes keeps the withdrawal-request tests and the funding task talking about the same account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sweeper fee funding is mechanically an ordinary ckETH withdrawal — same nonce sequence, same tECDSA signing, same fee-bumped resubmission — so it becomes a third `WithdrawalRequest` variant rather than a parallel pipeline. It differs in exactly one respect, and everything here turns on it: the ckETH burned for funding is NEVER re-minted, so a funding request must never reach the reimbursement machinery. Three places enforce that, all of which would otherwise fail only at runtime: - `maybe_reimburse` is the double-minting guard, and `record_reimbursement_request` asserts membership has been cleared before minting. Funding is now kept out of the set on insert, and the corresponding `assert!` on removal is conditional. Both are production asserts, so a missed branch would trap the canister. - `From<&WithdrawalRequest> for ReimbursementIndex` becomes `TryFrom` with a `NotReimbursable` error, deliberately fallible so the compiler proves at every call site that funding cannot produce an index — rather than a panicking arm that traps if a site is missed. The two callers construct their index inside the reimbursable arms instead. - A failed funding transaction records nothing to pay back, only a log line. Everything a later funding needs in order to offset against the unspent burn is already reconstructible from the accepted-request event plus the finalized transaction's receipt, so no second event type is introduced. Everything else follows ckETH: the 21'000 gas limit (a plain value transfer to a code-less EOA, which cannot revert), `ResubmissionStrategy::ReduceEthAmount` with the burned amount as the ceiling — so a climbing gas price shrinks the ETH delivered to the sweeper instead of breaking the invariant — and a fee carved out of the burned amount so `eth_balance` accounting needs no change. Funding is reported in `withdrawal_status`, the dashboard and the event log rather than hidden: it is a public, auditable action. A dedicated dashboard section with the prepaid-gas balance arrives with the observability work. Tests pin that difference, including a deliberate contrast test asserting a failed *user* withdrawal is still reimbursed — without it the no-reimbursement tests would also pass if reimbursement were broken for everything. The fee ceiling is asserted through behaviour (a spike past the burn yields InsufficientTransactionFee) rather than by inspecting the stored strategy, and the new CBOR event tag round-trips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Candid file documents no other event variant, and neither do the corresponding variants in `endpoints.rs` nor the surrounding code in `state.rs`, `state/audit.rs` and `withdraw.rs`. What stays is what the surrounding code already does: `event.rs` gives every variant a one-line doc, and `state/transactions/mod.rs` documents struct fields one line each, so `SweeperFundingRequest` mirrors `EthWithdrawalRequest`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`processed_transaction_status` classified every failed finalized transaction as `PendingReimbursement`, which the Candid interface defines as "transaction failed and will be reimbursed". A sweeper funding is never reimbursed, so `retrieve_eth_status` and `withdrawal_status` promised a reimbursement that nothing will ever settle — and the status would stay wrong forever, since only recording a reimbursement moves it on. Add a `Failed` variant to `TxFinalizedStatus` for a failure that will not be reimbursed, and pick between the two by asking the request whether it is reimbursable. This needs the didc override, as any addition to a returned variant does. Tests cover all three paths: a failed funding reports `Failed`, a successful one still reports `Success`, and a failed *user* withdrawal still reports `PendingReimbursement` — without that last one the first would also pass if the branch were inverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3c5f94d to
b3bb391
Compare
|
✅ No security or compliance issues detected. Reviewed everything up to b3bb391. Security Overview
Detected Code Changes
|
…imbursement" This reverts commit b3bb391.
Records the reasoning behind the revert above it. A funding transaction is a plain value transfer to an address derived from the minter's own key, so there is no code for it to revert in and the failure branch cannot be reached; reporting a pending reimbursement that will never come is imprecise but unreachable, which is cheaper than breaking every client of retrieve_eth_status. The log at finalization now says the same thing, so if the assumption ever breaks it says so rather than reading like a routine outcome. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mbjorkqvist
changed the title
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.
Suppressed comments (1)
rs/ethereum/cketh/minter/src/state.rs:377
- The new funding branch affects the minter's core ETH-balance accounting, but the analogous state tests currently cover only ckETH and ckERC20 withdrawals (
state/tests.rs:1433andstate/tests.rs:1528). The transaction-layer funding tests do not callState::record_finalized_transaction, so they cannot catch an incorrect successful debit, failed-transaction fee debit, or fee-counter update here. Please add successful and failed sweeper-funding cases to the state balance tests.
WithdrawalRequest::SweeperFunding(req) => req
.withdrawal_amount
.checked_sub(tx.transaction().amount)
.expect("BUG: funded amount MUST always be at least the transaction amount"),
Funding takes its own arm in record_finalized_transaction, and the ckETH and ckERC20 balance tests cannot reach it, so nothing here caught an incorrect debit, an uncounted fee, or the expect that traps when a funding's ceiling is below what its transaction moved. Asserted as the identity the accounting must satisfy — the ceiling covers the ETH delivered plus the fee, so the part of the fee left unspent is what stays with the minter — rather than as fixed numbers, which is also what makes the failing case's "only the fee is debited" meaningful. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| .checked_sub(tx.transaction().amount) | ||
| .expect("BUG: withdrawal amount MUST always be at least the transaction amount"), | ||
| WithdrawalRequest::CkErc20(req) => req.max_transaction_fee, | ||
| WithdrawalRequest::SweeperFunding(req) => req |
There was a problem hiding this comment.
Addressing the suppressed comment from the latest Copilot round, which flagged that this arm has no coverage: correct, and fixed in f8b70da with should_update_after_successful_and_failed_sweeper_funding next to the ckETH and ckERC20 balance tests.
One detail of the report is worth correcting for anyone reading later: nothing in state/tests.rs calls State::record_finalized_transaction directly — the existing balance tests reach it by applying events, which is the idiom there. So the gap was not a missing call style but a missing case: the ckETH and ckERC20 tests cannot enter this arm, so an incorrect debit, an uncounted fee, or the expect above trapping on a funding whose ceiling is below its transaction amount would all have gone unnoticed.
The new test asserts the identity the accounting has to satisfy rather than fixed numbers — the funding ceiling covers the ETH delivered plus the fee, so whatever part of the fee goes unspent is exactly what stays with the minter — and then asserts the failing case debits only the fee actually paid, with both fee counters unchanged between the two outcomes. Numbers copied from the ckETH fixture would have hidden the first arithmetic mistake I made here, which is what pushed me to the identity form.
For what it is worth, richer funding-accounting tests do exist later in the stack (should_account_for_a_successful_sweeper_funding and should_keep_a_failed_sweeper_funding_as_prepaid_gas), but they cannot move here: they depend on the burn-first accounting added in #11083 and on the cketh_burned field added in #11086. This test is deliberately scoped to what this PR introduces.
Part of DEFI-2933 (sweeper fee funding). Now targets
master: #11060 has merged, and #11065 is being closed with its contents folded into the PRs that use them — the fee-subaccount constant into this one, the burn helper into #11086 alongside its caller.Why
Sweeper fee funding is mechanically an ordinary ckETH withdrawal — same nonce sequence, same threshold-ECDSA signing, same fee-bumped resubmission — so it becomes a third
WithdrawalRequestvariant rather than a parallel pipeline.It differs in exactly one respect, and that difference is what the whole feature turns on: the ckETH burned for funding is never re-minted. A funding request must therefore never reach the reimbursement machinery.
What
Three places enforce that, all of which would otherwise fail only at runtime:
maybe_reimburseis the double-minting guard, andrecord_reimbursement_requestasserts membership has been cleared before minting. Funding is kept out of the set on insert, and the corresponding assertion on removal is made conditional. Both are production assertions, so a missed branch traps the canister.Everything else follows ckETH: the 21'000 gas limit of a plain value transfer to a code-less address, a resubmission strategy ceilinged at the burned amount — so a climbing gas price shrinks the ETH delivered to the sweeper rather than spending more than was burned — and a fee carved out of that same amount, so balance accounting needs no change.
Funding appears in the withdrawal status endpoint, the dashboard and the event log rather than being hidden: it moves ckETH-denominated value and is a public, auditable action. A user query never matches one, since the sender is the minter itself.
The new event takes tag 27; 26 went to
AutomaticDepositReceivedon master while this branch was open, and the tags are the durable CBOR encoding, so they cannot collide.Not a new withdrawal status
An earlier revision reported a failed funding as its own
TxFinalizedStatus::Failed, which meant adding a variant to the return type ofretrieve_eth_status— a breaking change for every existing client, and one the repository's own didc check flagged. Reverted after review, because the state it described cannot occur: funding is a plain value transfer to an address derived from the minter's own key, so there is no code for it to revert in. A failed funding therefore reports the pre-existing pending-reimbursement status, documented at the site along with the condition that would invalidate the reasoning, and the log at finalization says the outcome was unexpected rather than routine.Stack
Merge in order; each PR targets the one above it.
Related, outside the stack: #11139 covers the ICRC-2 self-spend rule the burn relies on, for the ICP ledger and both ICRC token types.