refactor(cketh): make the transaction pipeline generic over its request - #11178
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the ckETH minter’s transaction state machine by making TransactionLane generic over a new LaneRequest trait, so the same send/resubmit/finalize machinery can later be reused for a second sender address without coupling it to withdrawal-specific concepts.
Changes:
- Introduces
LaneRequestand makesTransactionLane<R>generic over it (withWithdrawalRequestas the current implementation). - Moves transaction construction from a free function (
create_transaction) intoLaneRequest::to_transaction, and updates all call sites accordingly. - Renames lane-facing APIs/vocabulary from “withdrawal” to generic “request” (e.g.,
record_request,requests_batch,oldest_incomplete_request_timestamp) while keeping withdrawal/reimbursement concepts onEthTransactions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/ethereum/cketh/minter/src/withdraw.rs | Updates batching flow to use generic request APIs and LaneRequest::to_transaction. |
| rs/ethereum/cketh/minter/src/state/transactions/tests.rs | Updates tests for renamed APIs and moves the “accepted withdrawal request event” helper into tests. |
| rs/ethereum/cketh/minter/src/state/transactions/mod.rs | Introduces LaneRequest, makes TransactionLane generic, migrates transaction creation into to_transaction, and updates request-centric terminology. |
| rs/ethereum/cketh/minter/src/state/tests.rs | Updates state tests to construct transactions via to_transaction. |
| rs/ethereum/cketh/minter/src/state/audit.rs | Updates audit transition handling to call record_request. |
| rs/ethereum/cketh/minter/src/state.rs | Updates state methods to use renamed request accessors (record_request, get_processed_request). |
| rs/ethereum/cketh/minter/src/main.rs | Updates metric to use oldest_incomplete_request_timestamp. |
| rs/ethereum/cketh/minter/src/guard/tests.rs | Updates guard tests to call record_request. |
| rs/ethereum/cketh/minter/src/guard/mod.rs | Updates pending-request count to use requests_len. |
| rs/ethereum/cketh/minter/src/dashboard/tests.rs | Replaces removed event-conversion method with a local helper and uses to_transaction. |
| rs/ethereum/cketh/minter/src/dashboard.rs | Updates dashboard to iterate pending requests via requests_iter. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 09:10
65eda6f to
f597a7d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:1354
remove_requestremoves by fullEq(r != request), butreschedule_requestand the lane invariants are keyed byid(). If a caller passes a request value with the sameid()but different non-id fields,reschedule_requestwill assert there is exactly one matching id, butremove_requestwon't remove it (andrecord_requestwill then panic on duplicate id). Removing byid()makes the API consistent and robust for non-withdrawal lane types.
fn remove_request(&mut self, request: &R) {
self.pending_requests.retain(|r| r != request);
rs/ethereum/cketh/minter/src/state/transactions/mod.rs:599
- This doc comment says the timestamp covers requests “awaiting a transaction or a reimbursement”, but the implementation only considers pending requests and
maybe_reimburse(created-but-not-finalized). It does not include requests with a pendingreimbursement_requestsentry. Either adjust the wording to match the current behavior, or extend the calculation to include reimbursement backlog.
/// Whether any request is still in flight, either awaiting a transaction or a reimbursement.
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 11:39
db25106 to
00d183e
Compare
gregorydemay
changed the title
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 13:06
00d183e to
c69e4ce
Compare
gregorydemay
changed the base branch from
greg/cketh-lane-tidy
to
greg/cketh-extract-pipeline
August 18, 2026 13:08
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 13:41
c69e4ce to
3c390a4
Compare
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 14:03
3c390a4 to
6a2a1c8
Compare
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 14:28
6a2a1c8 to
6be91c0
Compare
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 18, 2026 15:06
6be91c0 to
b659e58
Compare
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 19, 2026 08:18
b659e58 to
836ce92
Compare
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 20, 2026 08:45
836ce92 to
d483cb8
Compare
gregorydemay
commented
Aug 20, 2026
|
✅ No security or compliance issues detected. Reviewed everything up to daafc74. Security Overview
Detected Code Changes
|
`TransactionPipeline` served exactly one sender: the minter's main address. One nonce, every map keyed by a ckETH `LedgerBurnIndex`, and the request type baked in. A second sender address cannot reuse any of it. Introduce `PipelineRequest` — an identity usable as the pipeline's alternate map key, a destination, a creation time, a fee-bump strategy and the EIP-1559 transaction the request turns into — and make `TransactionPipeline<R>` generic over it. `WithdrawalRequest` is the only implementation, so `WithdrawalTransactions` now wraps `TransactionPipeline<WithdrawalRequest>` and every existing call site and behaviour is unchanged. With the request type behind a trait, the pipeline's vocabulary follows: it speaks of requests and ids rather than withdrawals, since a future pipeline's requests are not withdrawals and its ids are not ledger burn indices. Reimbursement and withdrawal status keep their names — they live on `WithdrawalTransactions` and really are withdrawal concepts. Two things fall out. `create_transaction` was a five-argument free function reaching into a request to build its transaction; that is now `PipelineRequest::to_transaction`, so the function goes. And `into_accepted_withdrawal_request_event` had no production caller left, so it moves to the test file that was its only consumer — `EventType` no longer appears anywhere in `state::transactions`. Preparatory, with no second pipeline yet: nothing instantiates `TransactionPipeline` with anything but `WithdrawalRequest`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#11190 narrowed `TransactionStage` to `pub(in crate::state)`, which matched a `TransactionPipeline` that was equally narrow. Making the pipeline generic also makes it `pub`, so a `pub` method returning the narrower type is a private-interface warning, and CI denies warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`PipelineRequest::to_transaction` returned `Result<_, CreateTransactionError>`,
whose only variant carries a `cketh_ledger_burn_index: LedgerBurnIndex`. A
request type that burns no ckETH cannot construct that value, so the trait
promised a failure such an implementor has no way to represent — and its driver
would have to handle an `Err` that can never arrive.
The error becomes an associated type. `WithdrawalRequest` sets it to
`CreateTransactionError` and is unchanged; a request that funds its own fee can
set it to `Infallible`, which turns the unreachable arm into an uninhabited one
the compiler discharges (`Err(never) => match never {}`). Both drivers call the
method on a concrete request type, so nothing needs a new bound.
Rename `to_transaction` to `create_transaction`: `to_*` announces a cheap
borrowing conversion, but this is fallible and takes four further arguments. The
new name also matches `CreateTransactionError`, `record_created_transaction` and
`EventType::CreatedTransaction`.
Also cleaned up: dropped the doc comment orphaned by the removal of the
`create_transaction` free function, which had come to document
`ERC_20_TRANSFER_FUNCTION_SELECTOR`; its `# Errors` section now sits on the trait
method and its fee-payer note on the withdrawal implementation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the generalization: the methods were renamed, their parameters and locals were not, so generic code still said `burn_index`, `ledger_burn_index` and `withdrawal_id` — including in two public signatures, and in a `panic!` whose own message already said "id". - Rename those 26 lines inside `impl<R: PipelineRequest> TransactionPipeline<R>`, and the comment that still spoke of a withdrawal request. The withdrawal vocabulary stays wherever it is accurate: the `WithdrawalRequest` trait impl, and `WithdrawalTransactions`. - Rename the test builder's `with_pending_withdrawal_requests` and `with_processed_withdrawal_requests`, whose own fields were already `pending_requests`/`processed_requests`. - Drop `ResubmitTransactionError`'s unused `Id = LedgerBurnIndex` default, which let the bare name keep meaning the withdrawal one. - Give `record_reimbursement_request` a doc of its own: it carried a verbatim copy of `record_finalized_transaction`'s, describing a `receipt` parameter it does not have and a finalization it does not perform. Move it next to the other reimbursement bookkeeping, leaving a second `impl` block that really is only the withdrawal-status queries, and say so. - Move `accepted_withdrawal_request_event` into `mod eth_balance`, its only caller, where `WithdrawalRequest` and `EventType` are already in scope, so the helper needs neither a fully-qualified parameter type nor an inner `use`. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`transactions/mod.rs` was 1562 lines covering six concerns. Move the smallest self-contained one out: `PipelineRequest` — what a request must answer to travel a `TransactionPipeline` — together with the minter's implementation of it for `WithdrawalRequest`, which is most of the bulk (the two `create_transaction` arms). `mod.rs` drops to 1384 lines and loses three imports that existed only for the transaction-building code (`EthereumNetwork`, `GasAmount`, `ResubmissionStrategy`). The trait is re-exported, so no caller outside the module changes. One test was reaching `EthereumNetwork` through `state::transactions`, which worked only because a child module can see its parent's private `use`. It now imports from `crate::lifecycle`, where the type actually lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Clone + Eq + fmt::Debug` were supertraits of `PipelineRequest`, so every present and future request type owed them whether or not it ever met the code that needs them. None of the three is part of the contract a request answers: they are what `TransactionPipeline` needs to hand out owned requests (`requests_batch`, `record_created_transaction`), to remove one by value (`retain(|r| r != request)`) and to format one in an assertion. Move them onto the impl block that uses them, leaving `PipelineRequest` to say only what a request must answer. Purely a compile-time change: the recompiled test binaries were bit-identical, so Bazel replayed the cached results. `Eq` rather than `PartialEq`, though only `PartialEq` is needed to compile: `remove_request` deletes the queue entry equal to a given request, which is correct only if equality is reflexive. The `Id` bounds stay on the associated type. They are not the same case — moving them needs a `where` clause repeated on the impl and defeats the struct's derives, which cannot prove `MultiKeyMap<_, R::Id, _>: Clone` from a bound on `R` alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Id` required `Display`, and four panic messages in the generic pipeline used it. Those messages are shared by every sender address, so the id alone does not say which pipeline trapped — and a bare number in a canister's trap message is exactly where that matters. Use `Debug` instead and drop the `Display` bound. `LedgerBurnIndex` is unaffected (`phantom_newtype::Id` renders both through the inner `u64`), while a request type whose id is an ordinary newtype now reports `SweepId(42)` rather than `42`, naming the pipeline for free. `Debug` was already required, for the `assert_eq!`s over id-keyed maps, so this removes an obligation rather than trading one for another. Also: the "duplicate transaction hash" assertion still said "burn indices" for what are now ids of either kind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`record_created_transaction` cloned the pending request, checked the clone, then deleted the original by value equality. Find its position instead: the asserts read it in place, and `VecDeque::remove` hands over the very entry that was found. The removal still happens after the asserts, not at the top. Two tests call this method twice on one pipeline, each expecting its own panic, which only works while a rejected transaction leaves the request pending. `reschedule_request` gets the same treatment, which fixes a latent trap: it removed by equality against the request the *caller* passed, so a caller whose copy had drifted from the queued one would have removed nothing and then panicked on a duplicate id from `record_request`. It now re-enqueues the entry it took out. That leaves `remove_request` without callers, and no code that identifies a request by anything but its id. `Eq` is still needed, but only to compare requests in assertions and in `is_equivalent_to` — no longer to decide which queue entry to drop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Checking the created transaction against the request is what `assert_created_transaction` is for, and the destination is one of those checks. It sat in the pipeline instead, which now asserts only the nonce — the one part of a created transaction the pipeline, not the request, decides. `PipelineRequest::destination` had no other caller, so the trait loses an obligation too: the pipeline never needed to know where a request sends its funds, only that the transaction agrees with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The second `impl WithdrawalTransactions` came in with the generic pipeline, with a doc claiming it held the reimbursement and withdrawal-status behaviour — while eight of the nine reimbursement methods stayed in the first block. Repairing that boundary earlier only narrowed the claim; the split itself buys nothing. Both blocks are plain inherent impls on the same type in the same file, so the second header was a section comment with an `impl` attached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generic-pipeline commit hoisted five reimbursement methods above the delegating ones. Nothing about the change needed that, and it made them read as deleted and re-added, in a commit that should read as being about generics. Restore master's order. `mod.rs` now differs from master by 532 changed lines rather than 689, all of them real. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… methods The previous commit reordered the methods by rejoining their bodies, and joined them with a single newline instead of two, so all 35 separating blank lines were lost. Nothing downstream caught it: rustfmt preserves blank lines between items but never adds them, and the check that the reorder was a pure permutation compared method bodies with surrounding whitespace stripped — precisely the thing that had changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gregorydemay
force-pushed
the
greg/cketh-lane-generic
branch
from
August 21, 2026 09:13
5cea49a to
641c22f
Compare
mbjorkqvist
approved these changes
Aug 24, 2026
mbjorkqvist
left a comment
mbjorkqvist
left a comment
Contributor
There was a problem hiding this comment.
Thanks @gregorydemay, just a few, mainly minor comments.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The inherent WithdrawalRequest::created_at shadows it at every call site, so no request ever answers it through the trait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The method only ever consumed the request to read its id, and rescheduled whatever sat in the queue under that id, so a caller passing a modified request had its modifications silently dropped. Every caller already holds the id. 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.
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 #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.
Stack created with GitHub Stacks CLI • Give Feedback 💬