feat(cketh): add the SweeperFunding withdrawal-request variant [override-didc-check] - #11072
Conversation
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>
mbjorkqvist
changed the title
mbjorkqvist
added
the
CI_OVERRIDE_DIDC_CHECK
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>
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.
| WithdrawalRequest::SweeperFunding(request) => { | ||
| if receipt.status == TransactionStatus::Failure { |
There was a problem hiding this comment.
Confirmed and fixed in 3c5f94d. Good find — this was a real defect, and a public-facing one.
Traced the flow to be sure: the funding burns ckETH at index N, its transaction finalizes with receipt status Failure, record_finalized_transaction takes the funding arm so no reimbursement request is created and N was never in maybe_reimburse — and then retrieve_eth_status(N) finds a finalized transaction with no reimbursed entry and a Failure status, returning PendingReimbursement. It stays that way permanently, because only recording a reimbursement moves that status on and nothing ever will. Both retrieve_eth_status and withdrawal_status are reachable for a funding index — there is no sender filter — so anyone can observe it.
TxFinalizedStatus had no variant meaning "failed, and will not be reimbursed", so one was added:
// Transaction failed and will not be reimbursed.
Failed : EthTransaction;
processed_transaction_status now picks between the two by asking the request whether it is reimbursable, rather than assuming failure implies reimbursement. Adding a case to a returned variant trips the Candid subtype rule, so this rides the same CI_OVERRIDE_DIDC_CHECK the event variant already needed.
On the missing coverage, also fixed — three tests rather than one, since the interesting part is the branch:
should_report_a_failed_funding_as_failed_not_pending_reimbursementshould_report_a_successful_funding_as_successshould_still_report_a_failed_user_withdrawal_as_pending_reimbursement
The third is what gives the first its value: without it, an inverted branch would still pass.
Worth recording how reachable the failure itself is today, since it affects how you read the severity. A bare 21'000-gas transfer to a code-less address cannot revert, which is exactly why the sweeper is a plain EOA — so in production this is currently latent. It becomes live once the sweeper address can carry code, which EIP-7702 delegation introduces later in this feature: a delegated account executes its delegate on a plain transfer, and at 21'000 gas that runs out of gas and the transaction fails. PR 7 of this stack also forces it deliberately in a test.
Latent or not, it was worth fixing here rather than later: the burn is real, the ckETH is gone and will not be replaced, so telling an auditor reconciling supply that a reimbursement is pending is materially wrong.
There was a problem hiding this comment.
Addendum to my reply above, since it left out the part that determines how bad this actually is.
Saying the burned ckETH "will not be replaced" is true but incomplete: the ETH never moved either. On a failed funding state.rs records transferred = 0 and only the effective gas as spent, so burned_not_yet_spent becomes ≈ the full funding amount, and burn_required_for then lets the next funding move that amount while burning (almost) nothing. ckETH ends up momentarily over-backed, never under-backed, and the only real cost is the failed transaction's 21'000 base gas.
So the wrong status was the actual defect here, not a stranded balance — nobody needs to intervene to recover funds. What does warrant attention is a persistent failure: each attempt still burns at least the ledger minimum, so the fee account would drain at that rate per funding interval while no gas is delivered. That belongs to observability rather than to this PR, and the alert condition is now recorded on DEFI-2965 along with the rest of the funding metrics.
`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>
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.
| WithdrawalRequest::SweeperFunding(request) => { | ||
| if receipt.status == TransactionStatus::Failure { |
There was a problem hiding this comment.
Addendum to my reply above, since it left out the part that determines how bad this actually is.
Saying the burned ckETH "will not be replaced" is true but incomplete: the ETH never moved either. On a failed funding state.rs records transferred = 0 and only the effective gas as spent, so burned_not_yet_spent becomes ≈ the full funding amount, and burn_required_for then lets the next funding move that amount while burning (almost) nothing. ckETH ends up momentarily over-backed, never under-backed, and the only real cost is the failed transaction's 21'000 base gas.
So the wrong status was the actual defect here, not a stranded balance — nobody needs to intervene to recover funds. What does warrant attention is a persistent failure: each attempt still burns at least the ledger minimum, so the fee account would drain at that rate per funding interval while no gas is delivered. That belongs to observability rather than to this PR, and the alert condition is now recorded on DEFI-2965 along with the rest of the funding metrics.
|
✅ No security or compliance issues detected. Reviewed everything up to 3c5f94d. Security Overview
Detected Code Changes
|
Part of DEFI-2933 (sweeper fee funding), third of a seven-PR stack. Targets #11065.
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.
Tests pin the difference, including a deliberate contrast test asserting that 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 rather than by inspecting stored state, and the new event tag round-trips as CBOR.
Stack
Merge in order; each PR targets the one above it.