feat(cketh): record the deposits a sweep moves and size its gas to the delegate's work - #11330
feat(cketh): record the deposits a sweep moves and size its gas to the delegate's work#11330gregorydemay wants to merge 4 commits into
Conversation
…e delegate's work A sweep request now carries the (account, token) deposits it takes, so the sweep queue and the set of delegated addresses are both reconstructible from the log without decoding call data. Its gas limit follows what the delegate executes rather than the number of deposits: sweepErc20Batch hands the whole token array to every address it sweeps, so both the balance checks and the transfers grow as distinct addresses times distinct tokens, and it moves any pair it finds a balance at — not only the pairs the request names. Only the addresses a sweep delegates pay an authorization on top. The limit sits well above what a sweep consumes: a four-deep call nest gives each level 63/64 of the gas left (EIP-150), so the limit a batch needs is strictly more than the gas it uses. Overshooting only asks the sweeper address to prepay more, the remainder being refunded, whereas an underestimate burns the whole transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A sweep no longer distinguishes the addresses it delegates from the ones already delegated: it carries an authorization tuple for every deposit address it touches, always signed for nonce 0. Applying an EIP-7702 authorization increments the authority's nonce, so nonce 0 means the address has never been delegated. A tuple signed for it therefore installs the delegation if there is none, and is skipped otherwise — the transaction stays valid, the designator already in place is untouched, and the delegated call works. Both outcomes are correct in every ordering, without stored state and without reading the chain, which is what makes tracking who is delegated unnecessary. `SweptDeposit::delegating` goes with that tracking, and the gas limit charges one authorization per distinct address rather than per tuple the request happens to carry. A skipped tuple still pays its base cost, so the budget is unchanged in the worst case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…okens Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 098ae8c. Security Overview
Detected Code Changes
|
mbjorkqvist
left a comment
mbjorkqvist
left a comment
There was a problem hiding this comment.
Thanks @gregorydemay!
The PR description still describes the first commit. Since ab70606 dropped SweptDeposit::delegating and gave every touched address a tuple, two claims under "What" no longer hold:
- "the sweep queue and the set of delegated addresses are both reconstructible from the event log" — only the sweep queue is; a skipped tuple leaves no trace of whether the address was already delegated.
- "Only the addresses a sweep delegates pay an authorization cost on top" - every address does now.
| /// | ||
| /// The enqueuing side sends one token per sweep, so the product is the addresses in practice; | ||
| /// the general form is what keeps the limit safe for any request that reaches here. | ||
| pub fn gas_limit(&self) -> GasAmount { |
There was a problem hiding this comment.
This gas_limit() is now used in tests, but in production, the SWEEP_TRANSACTION_GAS_LIMIT is still used, is that on purpose?
| [ | ||
| (SWEEP_GAS_PER_BALANCE_CHECK, pairs), | ||
| (SWEEP_GAS_PER_TRANSFER, pairs), | ||
| (SWEEP_GAS_PER_AUTHORIZATION, addresses), |
There was a problem hiding this comment.
Should this be:
| (SWEEP_GAS_PER_AUTHORIZATION, addresses), | |
| (SWEEP_GAS_PER_AUTHORIZATION, as_u64(self.authorizations.len())), |
?
Otherwise, e.g., delegating_sweep_request() has 2 tuples but deposits: vec![], so gas_limit() == 60_000, while the intrinsic cost alone is 21_000 + 2 × 25_000 = 71_000.
Alternatively, if they should always be the same length (although the deposit_from_cex.md design doc says:
mixed batches are fine (tuples only for not-yet-delegated addresses, already-delegated ones ride along without tuples)
), add enforcement of this.
| /// Gas one EIP-7702 authorization costs: 12'500 (`PER_AUTH_BASE_COST`) plus the 25'000 | ||
| /// (`PER_EMPTY_ACCOUNT_COST`) a deposit EOA pays, its account holding no ETH and no code yet. |
There was a problem hiding this comment.
This doesn't seem to match what the design doc says:
Upfront gas, charged to
M: 21'000 base + calldata + 25'000 per tuple (PER_EMPTY_ACCOUNT_COST; the 12'500 refund does not apply sinceD, holding only token balances, is not in the state trie).
The value of the constant itself seems fine.
| /// with the transfers. Each is deliberately generous: unspent gas is refunded, whereas an | ||
| /// underestimate wastes the whole transaction. |
There was a problem hiding this comment.
create_transaction derives max_fee_per_gas = max_transaction_fee / gas_limit, so overshooting here doesn't reserve more - it bids proportionally less, and creation is refused once base_fee + priority exceeds the quotient (resubmission can't recover it either: the whole allowance is already committed). Is the plan for the enqueuing side to size max_transaction_fee as gas_limit() x ceiling? If so, worth a line here - as written, "unspent gas is refunded" reads as though the slack were free, when it's what sets the base-fee headroom.
| #[n(6)] | ||
| pub authorizations: Vec<SignedAuthorization>, | ||
| /// The deposits this sweep moves, one per `(account, token)` pair, in the order the sweep queue | ||
| /// offered them. Not the shape of `data`, which names one item per deposit address: an account |
There was a problem hiding this comment.
gas_limit() now derives the limit from deposits, but nothing ties deposits to data: assert_created_transaction checks destination, amount, data and authorizations, but not that the deposits describe the calldata. A request whose deposits under-describe its batch would silently get an undersized limit.
Is the plan to enforce the correspondence at the (still to come) construction site, or should there be an assertion here?
| /// A transaction sent from the minter's dedicated sweeper address: an EIP-7702 transaction | ||
| /// (`0x04`) whose authorization list carries one tuple per deposit address it sweeps, or a plain | ||
| /// EIP-1559 one (`0x02`) for a sweep that delegates nothing — the native-ETH deposit addresses are | ||
| /// deliberately never delegated, so their sweeps take that shape. |
There was a problem hiding this comment.
Is this the right justification for the EIP-1559 variant? Per the design doc, a native-ETH sweep is paid by the deposit address itself:
ETH sweeps need no EIP-7702 at all (deposit pays its own gas, 21'000 gas, cheapest possible)
so it is sent from that address rather than from the sweeper address on its own nonce sequence, which is what a SweepTransaction is. And SweptDeposit requires an erc20_contract_address, so a native-ETH sweep cannot be represented in deposits either.
Now that every touched address carries a tuple, is SweepTransaction::Eip1559 reachable for any real sweep?
2 participants
Why
What
(account, token)deposits it moves, so the sweep queue and the set of delegated addresses are both reconstructible from the event log.sweepErc20Batchhands the whole token array to every address it sweeps, so balance checks and transfers both grow as distinct addresses times distinct tokens.sweepErc20moves any balance it finds, and a deposit address accumulates residue.Tests