Skip to content

refactor(cketh): rename EthTransactions to WithdrawalTransactions and hide its fields - #11177

Merged
gregorydemay merged 1 commit into
masterfrom
greg/cketh-lane-tidy
Aug 19, 2026
Merged

gregorydemay merged 1 commit into
masterfrom
greg/cketh-lane-tidy

Conversation

@gregorydemay

@gregorydemay gregorydemay commented Aug 18, 2026 •

Copy link
Copy Markdown
Contributor

Part of DEFI-2917 (deposit-from-CEX), the first of a stack preparing the minter to send Ethereum transactions from a second address. Pure refactoring: no behaviour change, and no test loses or gains coverage.

Wide but shallow — a rename across twelve files plus one genuine change. The substantive work sits in the PRs above.

What

  • Renames EthTransactions to WithdrawalTransactions. The name has to give way: a second sender address is coming, and its transactions are equally "Eth transactions". What distinguishes this one is that it carries user withdrawals.
  • Makes the fields private. No production code ever read one — the visibility existed solely so state::tests could assemble a populated value and vary one field at a time to exercise is_equivalent_to. A builder in transactions::tests covers that without exposing the fields. It does nothing but collect them, so what a test builds is still spelled out at the test; and since varying a field is what the test is for, each case clones the builder and changes only the field it is probing rather than restating the other eight:
withdrawal_transactions: builder.clone().with_sent_tx(Default::default()).build(),

state_equivalence keeps every assertion it had, in place.

Note on visibility

transactions::tests is pub(in crate::state) so state::tests can name the builder. A plain associated function would not need that — method resolution does not go through the module path — but a builder is a type, and types need a reachable path. The fields themselves stay private, which is the point of the change.

Stack created with GitHub Stacks CLI • Give Feedback 💬

@gregorydemay gregorydemay changed the title greg/cketh lane tidy refactor(cketh): separate reimbursement from the transaction state machine Aug 18, 2026
@gregorydemay
gregorydemay requested a lite review from Copilot August 18, 2026 07:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the ckETH minter’s Ethereum transaction tracking by separating “send/nonce state machine” concerns from withdrawal reimbursement bookkeeping, preparing the codebase for supporting multiple independent transaction lanes (e.g., a sweeper address) without inheriting refund logic.

Changes:

  • Split the original EthTransactions into a private TransactionLane (send machinery) plus a new EthTransactions wrapper that owns a lane and reimbursement state, forwarding the lane API to keep callers stable.
  • Centralized the “arm reimbursement on create / disarm on finalize” invariant in the wrapper (record_created_transaction / record_finalized_transaction).
  • Moved state-equivalence fixture logic into the transactions test module and adjusted tests to account for the new lane encapsulation.

Reviewed changes

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

File Description
rs/ethereum/cketh/minter/src/state/transactions/mod.rs Introduces TransactionLane and reworks EthTransactions into a wrapper that couples lane finalization with reimbursement tracking.
rs/ethereum/cketh/minter/src/state/transactions/tests.rs Updates unit tests for the new lane field and adds a shared fixture for state-equivalence tests.
rs/ethereum/cketh/minter/src/state/tests.rs Switches state-equivalence test setup to reuse the fixture now hosted under state::transactions::tests.

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

@gregorydemay
gregorydemay force-pushed the greg/cketh-lane-tidy branch 6 times, most recently from dceea28 to f134d3d Compare August 18, 2026 12:50
@gregorydemay gregorydemay changed the title refactor(cketh): separate reimbursement from the transaction state machine refactor(cketh): rename EthTransactions to WithdrawalTransactions and hide its fields Aug 18, 2026
@gregorydemay
gregorydemay marked this pull request as ready for review August 18, 2026 13:44
@gregorydemay
gregorydemay requested a review from a team as a code owner August 18, 2026 13:44
@github-actions github-actions Bot added the @defi label Aug 18, 2026
@zeropath-ai

zeropath-ai Bot commented Aug 18, 2026 •

Copy link
Copy Markdown

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

Security Overview
Detected Code Changes
Change Type Relevant files
Refactor ► rs/ethereum/cketh/minter/src/dashboard.rs
     Replace eth_transactions with withdrawal_transactions in multiple places, adjust iteration methods and nonce references
► rs/ethereum/cketh/minter/src/guard/mod.rs
     Use withdrawal_transactions.withdrawal_requests_len() instead of eth_transactions
► rs/ethereum/cketh/minter/src/guard/tests.rs
     Update test to use withdrawal_transactions for withdrawal requests
► rs/ethereum/cketh/minter/src/lifecycle/init.rs
     Import WithdrawalTransactions and initialize withdrawal_transactions instead of eth_transactions
► rs/ethereum/cketh/minter/src/lifecycle/tests.rs
     Update references from eth_transactions to withdrawal_transactions in assertions
► rs/ethereum/cketh/minter/src/main.rs
     Access withdrawal_transactions for status/nonce calculations
► rs/ethereum/cketh/minter/src/state.rs
     Replace EthTransactions with WithdrawalTransactions; rename fields accordingly
► rs/ethereum/cketh/minter/src/state/audit.rs
     Update state mutations to use withdrawal_transactions
► rs/ethereum/cketh/minter/src/state/tests.rs
     Update tests to use WithdrawalTransactions and related builders
► rs/ethereum/cketh/minter/src/state/transactions/mod.rs
     Rename EthTransactions to WithdrawalTransactions; adjust visibility and field names
► rs/ethereum/cketh/minter/src/state/transactions/tests.rs
     Update tests to import WithdrawalTransactions, adjust module names from eth_transactions to withdrawal_transactions
► rs/ethereum/cketh/minter/src/state/transactions/mod.rs (other files referencing type names)
     Update exports and internal references from EthTransactions to WithdrawalTransactions
Bug Fix ► rs/ethereum/cketh/minter/src/lifecycle/init.rs
     Initialize withdrawal_transactions instead of eth_transactions to align with new data model
Enhancement ► rs/ethereum/cketh/minter/src/state.rs
     Switch to WithdrawalTransactions across state, ensure consistency in record/update methods
► rs/ethereum/cketh/minter/src/state/audit.rs
     Apply state transitions to withdrawal_transactions
► rs/ethereum/cketh/minter/src/state/transactions/tests.rs
     Extend tests to accommodate WithdrawalTransactions structure and builder usage

@gregorydemay
gregorydemay added this pull request to the merge queue Aug 19, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 19, 2026
The nine fields were `pub(in crate::state)`, but no production code ever read
one. The visibility existed solely so `state::tests` could assemble a populated
value and vary one field at a time to exercise `is_equivalent_to`.

A builder in `transactions::tests` covers that without exposing the fields. It
does nothing but collect them, so what a test builds is still spelled out at the
test — and since varying a field is what the test is for, each case clones the
builder and changes only the field it is probing, rather than restating the
other eight.

`state_equivalence` keeps every assertion it had, in place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gregorydemay
gregorydemay added this pull request to the merge queue Aug 19, 2026
Merged via the queue into master with commit b074ca2 Aug 19, 2026
40 checks passed
@gregorydemay
gregorydemay deleted the greg/cketh-lane-tidy branch August 19, 2026 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants