feat(cketh): add the SweeperFunding withdrawal-request variant - #11072
Merged
Conversation
This was referenced Aug 7, 2026
mbjorkqvist
changed the title
mbjorkqvist
added
the
CI_OVERRIDE_DIDC_CHECK
Contributor
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.
Contributor
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
commented
Aug 10, 2026
mbjorkqvist
left a comment
mbjorkqvist
left a comment
Contributor
Author
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.
mbjorkqvist
marked this pull request as ready for review
August 10, 2026 09:35
|
✅ No security or compliance issues detected. Reviewed everything up to 46e91ba. Security Overview
Detected Code Changes
|
This was referenced Aug 10, 2026
Rachit2323
pushed a commit
to Rachit2323/ic
that referenced
this pull request
Aug 12, 2026
…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>
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>
mbjorkqvist
force-pushed
the
mathias/DEFI-2933-sweeper-funding-request
branch
from
August 14, 2026 08:48
3c5f94d to
b3bb391
Compare
mbjorkqvist
changed the base branch from
mathias/DEFI-2933-fee-account-burn
to
master
August 14, 2026 08:48
|
✅ 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
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.
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>
mbjorkqvist
commented
Aug 14, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
There was a problem hiding this comment.
Thanks @mbjorkqvist ! Generally looks good to me, I have an idea to simplify things further (see comment below) and please do challenge it!
Reuses `CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT` instead of a local copy of 21'000, so the test no longer asserts against its own duplicate of the value under test, and asserts the burn index from the request rather than restating the literal. Drops the hand-written event round-trip test: `event_encoding_roundtrip` already proptests it, since `arb_event_type` includes the accepted-funding event, and arbitrary values beat one fixed instance. Says "unreachable" where the comment on a failed funding's status said "imprecise" — the state cannot arise for a bare transfer to an address derived from the minter's own key, which is the reason the imprecision is tolerable rather than a separate excuse. The two balance tests now share the flow they had in common, so each states only what its own request type makes different. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregorydemay
approved these changes
Aug 18, 2026
A funding is a ckETH burn from the minter's own fee subaccount sent to an address the minter controls; the only thing that sets it apart from a user's ckETH withdrawal is that it is never reimbursed, which the variant itself already says. So drop the separate payload struct and reuse `EthWithdrawalRequest`, which lets every arm that was duplicating the ckETH behaviour merge with it. The event's `created_at` becomes optional, exactly like the one on `AcceptedEthWithdrawalRequest`.
`R14` had the minter track what it burned but did not spend and discount it from the next funding burn. Dropping that keeps the burn-first invariant -- the surplus is simply left as backing, as with the unspent gas of a user withdrawal -- and removes the only piece of funding accounting that had no counterpart in the withdrawal flow. A failed funding is likewise not reimbursed, which the spec now says as well.
Sharing `EthWithdrawalRequest` between the two variants made the conversion at this call site silent: `record_withdrawal_request` takes anything that converts into a `WithdrawalRequest`, and the payload's own conversion yields `CkEth`. So a funding replayed from the audit log came back as an ordinary user withdrawal -- reimbursable, and invisible to the funding accounting that keys off the variant. Names the variant instead, and pins it with a test that replays the event and checks both the variant and that it cannot be reimbursed. Nothing caught this before because balance accounting treats the two identically.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
rs/ethereum/cketh/docs/deposit_from_cex.md:360
- “The whole burn stays as backing” contradicts the implemented failure accounting:
state.rs:378-386still debits the effective transaction fee. A failed funding is not reimbursed, but the resulting surplus is the burn minus that fee, not the whole burn.
withdrawal. In the same spirit, a funding whose transaction fails is not
reimbursed — the whole burn stays as backing. That is a plain-transfer send to an
address derived from the minter's own key, so there is no code there to revert in;
accepting the loss buys an accounting with no reimbursement path to audit.
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:810
- This message overstates the failed-funding surplus: even when the value transfer fails, the effective gas fee leaves the main address (
state.rs:378-386). Only the transaction value remains, so the additional backing is the burn minus the effective fee. Please make the operational log explicit about that fee.
"[record_finalized_transaction]: UNEXPECTED: sweeper funding {} of {} to \
{} FAILED (tx {}), which should be impossible for a transfer to an \
address the minter controls; the burn is NOT reimbursed: the ETH \
never left the main address and now over-backs ckETH",
A failed transaction is included in a block and its gas is charged, which `record_finalized_transaction` duly debits. Saying the whole burn stays as backing therefore overstated it by exactly that fee, in the design doc and in the operational log, which now names the amount. The behaviour and its tests are unchanged.
pull Bot
pushed a commit
to bit-cook/ic
that referenced
this pull request
Aug 20, 2026
…1083) Part of [DEFI-2933](https://dfinity.atlassian.net/browse/DEFI-2933) (sweeper fee funding), first of the four remaining PRs. Targets `master`, now that dfinity#11072 has merged. ## Why Sweep gas is prepaid: ckETH is burned from the minter's fee subaccount before the ETH moves, so that at every instant ``` cumulative ckETH burned for sweeping >= cumulative ETH debited from the main address for sweeping ``` Nothing yet keeps track of either side, so nothing can check it. This PR adds that bookkeeping, plus the bounds deciding when a top-up is due. ## What The accounting is a fold over events the minter already persists — the accepted funding request and the finalized transaction's receipt — so it is reconstructed exactly on replay and needs no event type of its own. It is deliberately not serializable, which keeps that property honest. Writing it surfaced something worth stating plainly: a surplus arises on **every** funding, not only failed ones. The value transferred is the burn minus the transaction's *max* fee, while only the *effective* fee is ever spent, so the unused fee allowance stays at the main address. Nothing reuses it — each funding burns for its own transfer, and the surplus simply stays as ckETH backing, exactly like the unspent gas of a user withdrawal. A failed funding is the extreme case of the same thing. That surplus is tracked rather than read back from the chain because it sits at the **main** address, not the sweeper's. The sweeper's on-chain balance answers "how much prepaid gas is in place", which is a different question from "how far has burn run ahead of spend" — the quantity the invariant is about, and the one an operator needs in order to check it. Spending more than was burned would mean ckETH is under-backed, so that traps rather than saturating to zero — and it is checked eagerly at each finalized funding, so a violation surfaces at the transition that caused it rather than whenever someone next reads the surplus. ## The bounds Proposal-configurable, validated as a **pair** rather than individually: a target at or below the low-water mark would make a funding immediately due again and loop, and headroom below the minimum burn would make every cycle burn more ckETH than the ETH it moves. Both are rejected wholesale rather than partially applied. The same check runs when only the *minimum withdrawal amount* changes, since the invariant relates two independently configurable amounts and raising one alone would silently invalidate bounds that were valid when set. Defaults are 0.02 / 0.1 ETH — deliberately provisional, sized so a funding covers many sweeps and its own fee stays a small fraction of the amount moved. They are meant to be calibrated during the Sepolia rollout once real sweep gas costs are known. ## Stack Merge in order; each PR targets the one above it. | # | PR | Status | |---|----|--------| | — | dfinity#11060 — Read a native ETH balance via the EVM RPC canister | merged | | — | dfinity#11065 — Burn ckETH from the minter's own fee subaccount | closed; folded into dfinity#11072 and dfinity#11086 | | — | dfinity#11072 — Add the SweeperFunding withdrawal-request variant | merged | | 1 | Burn-first accounting for sweeper fee funding | **this PR** | | 2 | dfinity#11086 — Sweeper fee-funding task, with an end-to-end test | draft | | 3 | dfinity#11094 — Sweeper funding observability and the prepaid-gas gate | draft | | 4 | dfinity#11097 — Adversarial end-to-end coverage of sweeper fee funding | draft | [DEFI-2933]: https://dfinity.atlassian.net/browse/DEFI-2933?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
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.
Since reimbursement is the only difference, the variant carries an
EthWithdrawalRequest— the same payload a user's ckETH withdrawal carries — rather than a near-copy of it, and the spec drops the one piece of funding accounting that had no counterpart in the withdrawal flow: burned-but-unspent amounts are no longer credited against the next funding's burn. They stay as backing, just like the unspent gas a user's withdrawal leaves behind.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.Stack
Merge in order; each PR targets the one above it.