refactor(cketh): extract TransactionPipeline from WithdrawalTransactions

[gregorydemay]

Why

WithdrawalTransactions does two jobs. It drives transactions through create → sign → send → resubmit → finalize on the minter's nonce sequence, and it tracks which withdrawals still owe the user a refund. The two meet at only two moments — creating a transaction arms the refund, finalizing it disarms it — and everything else about sending a transaction is indifferent to whether anyone gets paid back.

The minter's sweeper address will need that send machinery on a nonce sequence of its own. It burns no ckETH, so it must be able to reuse the machinery without inheriting a refund path it can never use.

What

TransactionPipeline becomes the send machinery alone — the request queues, the three transaction maps and the nonce. WithdrawalTransactions owns a pipeline plus the reimbursement bookkeeping, forwards the pipeline's API so callers are unchanged, and intercepts those two moments so the invariant lives in one place. Reimbursement records and withdrawal status move with it.

Two things follow from the split:

• record_finalized_transaction exists on both types: the pipeline's performs the finalize mechanics and hands back the finalized transaction; the wrapper's uses that to decide the refund. It keeps the same name on both, as record_created_transaction already did.
• transaction_stage reports where a transaction sits — created, sent or finalized — returning Option, so absence is the type's business rather than a variant. The wrapper maps that to RetrieveEthStatus, consulting reimbursement only once finalized. RetrieveEthStatus cannot move to the pipeline: its finalized payload is Success | PendingReimbursement | Reimbursed, which is reimbursement vocabulary throughout.

state::tests is untouched: it builds its fixture through a builder, so the new nesting is absorbed in build() and no assertion has to be restated.

[Copilot]

Pull request overview

Refactors the ckETH minter’s withdrawal-transaction state by extracting the generic “send machinery” into a dedicated TransactionPipeline, leaving WithdrawalTransactions responsible for reimbursement bookkeeping and delegating transaction lifecycle operations to the pipeline.

Changes:

• Extracts transaction queue/nonce/maps logic into TransactionPipeline and wraps it inside WithdrawalTransactions.
• Splits finalization so the pipeline finalizes the on-chain transaction, while WithdrawalTransactions handles reimbursement side-effects on failure.
• Updates unit/state tests to construct and assert against the new pipeline-containing structure.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
rs/ethereum/cketh/minter/src/state/transactions/mod.rs	Introduces TransactionPipeline, moves core transaction lifecycle logic into it, and re-implements WithdrawalTransactions as a wrapper with reimbursement bookkeeping.
rs/ethereum/cketh/minter/src/state/transactions/tests.rs	Updates transaction tests to account for the new WithdrawalTransactions { pipeline, ... } structure and adds new_for_test for TransactionPipeline.
rs/ethereum/cketh/minter/src/state/tests.rs	Updates state equivalence tests to construct WithdrawalTransactions using TransactionPipeline::new_for_test(...).

Suppressed comments (1)

rs/ethereum/cketh/minter/src/state/transactions/mod.rs:1216

• WithdrawalTransactions::transaction_status directly accesses TransactionPipeline’s private pending_withdrawal_requests. Using the existing withdrawal_requests_iter() accessor avoids coupling WithdrawalTransactions to pipeline internals.

        if self
            .pipeline
            .pending_withdrawal_requests
            .iter()
            .any(|r| &r.cketh_ledger_burn_index() == burn_index)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[gregorydemay]

🤖 Reading guide — the diff in state/transactions/mod.rs is one large block because a single 38-method impl WithdrawalTransactions becomes two impls (29 + 36 methods). A line-based diff cannot express "this method is now two methods on two types", so most of what looks like churn is code that changed impl block without changing.

I tried reordering the file to make it align method-by-method — both putting the wrapper first and reordering its methods into master's order. Neither helps (11 hunks → 10, the two large hunks remain), so the file keeps the order that reads best rather than one contorted for the diff.

Instead, here is what the 41 methods actually are:

role	count	what to do
pure delegate	20	skim — the body is self.pipeline.<same name>(args)
moved verbatim into TransactionPipeline	23	skip — byte-identical to master
wrapper only (reimbursement + status)	12	skip — unchanged from master
pipeline only	5	3 verbatim helpers, 2 new: transaction_stage, processed_withdrawal_requests_iter
genuinely different	4	read carefully

The four worth your attention, which are exactly what the split is about:

• new — constructs a pipeline plus the reimbursement bookkeeping around it
• is_equivalent_to — compares the reimbursement state, then delegates the rest
• record_created_transaction — delegates, then arms the refund
• record_finalized_transaction — delegates the finalize mechanics, disarms the refund, then records the reimbursement

Those last two are the only places where the pipeline and the refund bookkeeping must stay in step, which is why they intercept rather than delegate. Both are now guarded by is_reimbursable, since a SweeperFunding request is never paid back — that variant arrived on master while this branch was in review.

To collapse the relocated code while reading:

git diff --color-moved=zebra --color-moved-ws=allow-indentation-change origin/master...greg/cketh-extract-pipeline

Pink and blue are removals that reappear elsewhere, cyan and yellow their arrivals; skipping all four leaves only what genuinely changed.

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to 0cd82ae.

Security Overview

• 🔎 Scanned files: 2 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/783a9bd4-fb4e-4e10-ab3c-c2fb2ddfb92f?scanId=6da2f878-63eb-486b-9a80-5efcf5119e71&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Refactor	► rs/ethereum/cketh/minter/src/state/transactions/mod.rs     Rename WithdrawalTransactions to TransactionPipeline and adjust visibility; introduce TransactionStage enum and new fields; migrate related logic accordingly; remove outdated reimbursement-related fields and methods; update related logic and documentation comments
Enhancement	► rs/ethereum/cketh/minter/src/state/transactions/mod.rs     Introduce TransactionStage enum and its usage ► rs/ethereum/cketh/minter/src/state/transactions/mod.rs     Replace direct fields with a new WithdrawalTransactions struct that contains a pipeline and reimbursement bookkeeping ► rs/ethereum/cketh/minter/src/state/transactions/tests.rs     Adjust tests to reflect renamed/new API (next_transaction_nonce, sent_transactions helper, etc.)

[mbjorkqvist]

Thanks @gregorydemay! Just a few minor comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

rs/ethereum/cketh/minter/src/state/transactions/tests.rs:3082

• In sent_transactions, the find predicate compares a dereferenced LedgerBurnIndex (*index) to a reference (burn_index). LedgerBurnIndex is an Id<_, u64> newtype and only implements PartialEq with itself, so *index == burn_index is likely a type mismatch. Comparing the references directly avoids the issue and is clearer.

    transactions
        .sent_transactions_iter()
        .find(|(_nonce, index, _txs)| *index == burn_index)
        .map(|(_nonce, _index, txs)| txs)