From e8935fad4b66dbc5d177138ce26803bf21dc262c Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Tue, 18 Aug 2026 13:28:20 +0200 Subject: [PATCH 01/16] refactor(cketh): make the transaction pipeline generic over its request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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` generic over it. `WithdrawalRequest` is the only implementation, so `WithdrawalTransactions` now wraps `TransactionPipeline` 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) --- rs/ethereum/cketh/minter/src/dashboard.rs | 2 +- .../cketh/minter/src/dashboard/tests.rs | 49 +- rs/ethereum/cketh/minter/src/guard/mod.rs | 2 +- rs/ethereum/cketh/minter/src/guard/tests.rs | 2 +- rs/ethereum/cketh/minter/src/main.rs | 2 +- rs/ethereum/cketh/minter/src/state.rs | 5 +- rs/ethereum/cketh/minter/src/state/audit.rs | 4 +- rs/ethereum/cketh/minter/src/state/tests.rs | 48 +- .../minter/src/state/transactions/mod.rs | 729 +++++++++--------- .../minter/src/state/transactions/tests.rs | 283 ++++--- rs/ethereum/cketh/minter/src/withdraw.rs | 19 +- 11 files changed, 590 insertions(+), 555 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/dashboard.rs b/rs/ethereum/cketh/minter/src/dashboard.rs index 351122a85ab9..74bda1e54667 100644 --- a/rs/ethereum/cketh/minter/src/dashboard.rs +++ b/rs/ethereum/cketh/minter/src/dashboard.rs @@ -338,7 +338,7 @@ impl DashboardTemplate { let mut withdrawal_requests: Vec<_> = state .withdrawal_transactions - .withdrawal_requests_iter() + .requests_iter() .cloned() .map(|request| match request { WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => { diff --git a/rs/ethereum/cketh/minter/src/dashboard/tests.rs b/rs/ethereum/cketh/minter/src/dashboard/tests.rs index b5a5e224faf5..e0414aa8df64 100644 --- a/rs/ethereum/cketh/minter/src/dashboard/tests.rs +++ b/rs/ethereum/cketh/minter/src/dashboard/tests.rs @@ -14,8 +14,8 @@ use ic_cketh_minter::numeric::{ use ic_cketh_minter::state::audit::{EventType, apply_state_transition}; use ic_cketh_minter::state::eth_logs_scraping::LogScrapingId; use ic_cketh_minter::state::transactions::{ - Erc20WithdrawalRequest, EthWithdrawalRequest, ReimbursementIndex, WithdrawalRequest, - create_transaction, + Erc20WithdrawalRequest, EthWithdrawalRequest, PipelineRequest, ReimbursementIndex, + WithdrawalRequest, }; use ic_cketh_minter::state::{MintedEvent, State}; use ic_cketh_minter::tx::{ @@ -533,10 +533,7 @@ fn should_display_pending_transactions_sorted_by_decreasing_cketh_ledger_burn_in TransactionStatus::Success, ), ] { - apply_state_transition( - &mut state, - &req.clone().into_accepted_withdrawal_request_event(), - ); + apply_state_transition(&mut state, &accepted_withdrawal_request_event(req.clone())); apply_state_transition( &mut state, &EventType::CreatedTransaction { @@ -560,7 +557,7 @@ fn should_display_pending_transactions_sorted_by_decreasing_cketh_ledger_burn_in ), ] { let withdrawal_id = req.cketh_ledger_burn_index(); - apply_state_transition(&mut state, &req.into_accepted_withdrawal_request_event()); + apply_state_transition(&mut state, &accepted_withdrawal_request_event(req)); apply_state_transition( &mut state, &EventType::CreatedTransaction { @@ -674,7 +671,7 @@ fn should_display_finalized_transactions_sorted_by_decreasing_cketh_ledger_burn_ ), ] { let id = req.cketh_ledger_burn_index(); - apply_state_transition(&mut state, &req.into_accepted_withdrawal_request_event()); + apply_state_transition(&mut state, &accepted_withdrawal_request_event(req)); apply_state_transition( &mut state, &EventType::CreatedTransaction { @@ -840,10 +837,7 @@ fn should_display_reimbursed_requests() { ), ] { let id = req.cketh_ledger_burn_index(); - apply_state_transition( - &mut state, - &req.clone().into_accepted_withdrawal_request_event(), - ); + apply_state_transition(&mut state, &accepted_withdrawal_request_event(req.clone())); apply_state_transition( &mut state, &EventType::CreatedTransaction { @@ -1223,7 +1217,7 @@ fn add_finalized_transactions(state: &mut State, num_transactions: u64) { TransactionStatus::Success, ); let id = req.cketh_ledger_burn_index(); - apply_state_transition(state, &req.into_accepted_withdrawal_request_event()); + apply_state_transition(state, &accepted_withdrawal_request_event(req)); apply_state_transition( state, &EventType::CreatedTransaction { @@ -1274,7 +1268,7 @@ fn add_reimbursed_transactions(state: &mut State, num_transactions: u64) { TransactionStatus::Failure, ); let id = req.cketh_ledger_burn_index(); - apply_state_transition(state, &req.into_accepted_withdrawal_request_event()); + apply_state_transition(state, &accepted_withdrawal_request_event(req)); apply_state_transition( state, &EventType::CreatedTransaction { @@ -1369,6 +1363,16 @@ pub fn ckusdt() -> CkErc20Token { } } +fn accepted_withdrawal_request_event(request: WithdrawalRequest) -> EventType { + match request { + WithdrawalRequest::CkEth(request) => EventType::AcceptedEthWithdrawalRequest(request), + WithdrawalRequest::CkErc20(request) => EventType::AcceptedErc20WithdrawalRequest(request), + WithdrawalRequest::SweeperFunding(request) => { + EventType::AcceptedSweeperFundingRequest(request) + } + } +} + fn cketh_withdrawal_request_with_index(ledger_burn_index: LedgerBurnIndex) -> EthWithdrawalRequest { const DEFAULT_WITHDRAWAL_AMOUNT: u128 = 1_100_000_000_000_000; const DEFAULT_PRINCIPAL: &str = @@ -1483,14 +1487,15 @@ fn ckerc20_withdrawal_flow( base_fee_per_gas: WeiPerGas::from(250_000_000_u64), max_priority_fee_per_gas: WeiPerGas::from(1_500_000_000_u64), }; - let transaction = create_transaction( - &withdrawal_request.clone().into(), - nonce, - gas_fee, - GasAmount::from(65_000_u32), - EthereumNetwork::Sepolia, - ) - .unwrap(); + let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); + let transaction = pipeline_request + .to_transaction( + nonce, + gas_fee, + GasAmount::from(65_000_u32), + EthereumNetwork::Sepolia, + ) + .unwrap(); let dummy_signature = TransactionSignature { signature_y_parity: false, r: Default::default(), diff --git a/rs/ethereum/cketh/minter/src/guard/mod.rs b/rs/ethereum/cketh/minter/src/guard/mod.rs index 24d9766335b0..537fcb8d5dd5 100644 --- a/rs/ethereum/cketh/minter/src/guard/mod.rs +++ b/rs/ethereum/cketh/minter/src/guard/mod.rs @@ -30,7 +30,7 @@ impl RequestsGuardedByPrincipal for PendingWithdrawalRequests { } fn pending_requests_count(state: &State) -> usize { - state.withdrawal_transactions.withdrawal_requests_len() + state.withdrawal_transactions.requests_len() } } diff --git a/rs/ethereum/cketh/minter/src/guard/tests.rs b/rs/ethereum/cketh/minter/src/guard/tests.rs index 8a9914dd1ade..ac0f3481926d 100644 --- a/rs/ethereum/cketh/minter/src/guard/tests.rs +++ b/rs/ethereum/cketh/minter/src/guard/tests.rs @@ -70,7 +70,7 @@ mod retrieve_eth_guard { fn record_withdrawal_request(ledger_burn_index: LedgerBurnIndex) { mutate_state(|s| { s.withdrawal_transactions - .record_withdrawal_request(EthWithdrawalRequest { + .record_request(EthWithdrawalRequest { withdrawal_amount: Wei::ONE, destination: Address::ZERO, ledger_burn_index, diff --git a/rs/ethereum/cketh/minter/src/main.rs b/rs/ethereum/cketh/minter/src/main.rs index 828f3b5ee317..ff37b4b9c8c2 100644 --- a/rs/ethereum/cketh/minter/src/main.rs +++ b/rs/ethereum/cketh/minter/src/main.rs @@ -1136,7 +1136,7 @@ fn http_request(req: HttpRequest) -> HttpResponse { let now_nanos = ic_cdk::api::time(); let age_nanos = now_nanos.saturating_sub( s.withdrawal_transactions - .oldest_incomplete_withdrawal_timestamp() + .oldest_incomplete_request_timestamp() .unwrap_or(now_nanos), ); w.encode_gauge( diff --git a/rs/ethereum/cketh/minter/src/state.rs b/rs/ethereum/cketh/minter/src/state.rs index 48ae8762b47f..41019cdadba4 100644 --- a/rs/ethereum/cketh/minter/src/state.rs +++ b/rs/ethereum/cketh/minter/src/state.rs @@ -377,8 +377,7 @@ impl State { "BUG: unsupported ERC-20 token {}", request.erc20_contract_address ); - self.withdrawal_transactions - .record_withdrawal_request(request); + self.withdrawal_transactions.record_request(request); } pub fn record_finalized_transaction( @@ -420,7 +419,7 @@ impl State { .expect("BUG: missing finalized transaction"); let withdrawal_request = self .withdrawal_transactions - .get_processed_withdrawal_request(withdrawal_id) + .get_processed_request(withdrawal_id) .expect("BUG: missing withdrawal request"); let charged_tx_fee = match withdrawal_request { WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => req diff --git a/rs/ethereum/cketh/minter/src/state/audit.rs b/rs/ethereum/cketh/minter/src/state/audit.rs index 93502baeff1d..4ccccb112a97 100644 --- a/rs/ethereum/cketh/minter/src/state/audit.rs +++ b/rs/ethereum/cketh/minter/src/state/audit.rs @@ -72,7 +72,7 @@ pub fn apply_state_transition(state: &mut State, payload: &EventType) { EventType::AcceptedEthWithdrawalRequest(request) => { state .withdrawal_transactions - .record_withdrawal_request(request.clone()); + .record_request(request.clone()); } EventType::AcceptedSweeperFundingRequest(request) => { state.sweeper_funding.record_burn(request.withdrawal_amount); @@ -80,7 +80,7 @@ pub fn apply_state_transition(state: &mut State, payload: &EventType) { // funding reimbursable. state .withdrawal_transactions - .record_withdrawal_request(WithdrawalRequest::SweeperFunding(request.clone())); + .record_request(WithdrawalRequest::SweeperFunding(request.clone())); } EventType::CreatedTransaction { withdrawal_id, diff --git a/rs/ethereum/cketh/minter/src/state/tests.rs b/rs/ethereum/cketh/minter/src/state/tests.rs index d57aaa0d94d6..574c0d52c284 100644 --- a/rs/ethereum/cketh/minter/src/state/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/tests.rs @@ -923,6 +923,19 @@ proptest! { } } +fn accepted_withdrawal_request_event( + request: crate::state::transactions::WithdrawalRequest, +) -> EventType { + use crate::state::transactions::WithdrawalRequest; + match request { + WithdrawalRequest::CkEth(request) => EventType::AcceptedEthWithdrawalRequest(request), + WithdrawalRequest::CkErc20(request) => EventType::AcceptedErc20WithdrawalRequest(request), + WithdrawalRequest::SweeperFunding(request) => { + EventType::AcceptedSweeperFundingRequest(request) + } + } +} + #[test] fn state_equivalence() { use crate::EVM_RPC_ID_PRODUCTION; @@ -978,13 +991,13 @@ fn state_equivalence() { ledger_burn_index: LedgerBurnIndex::new(20), ..withdrawal_request1.clone() }; - let pending_withdrawal_requests: VecDeque = vec![ + let pending_requests: VecDeque = vec![ withdrawal_request1.clone().into(), withdrawal_request2.clone().into(), ] .into_iter() .collect(); - let processed_withdrawal_requests = btreemap! { + let processed_requests = btreemap! { LedgerBurnIndex::new(4) => EthWithdrawalRequest { withdrawal_amount: Wei::new(1_000_000_000_000), ledger_burn_index: LedgerBurnIndex::new(4), @@ -1107,8 +1120,8 @@ fn state_equivalence() { }), }; let builder = WithdrawalTransactionsBuilder::default() - .with_pending_withdrawal_requests(pending_withdrawal_requests) - .with_processed_withdrawal_requests(processed_withdrawal_requests) + .with_pending_withdrawal_requests(pending_requests) + .with_processed_withdrawal_requests(processed_requests) .with_created_tx(created_tx) .with_sent_tx(sent_tx) .with_finalized_tx(finalized_tx) @@ -1511,7 +1524,7 @@ mod sweeper_funding { let request = state .withdrawal_transactions - .withdrawal_requests_iter() + .requests_iter() .next() .expect("BUG: the funding request was not recorded"); assert_matches!(request, WithdrawalRequest::SweeperFunding(_)); @@ -1532,7 +1545,7 @@ mod eth_balance { use crate::state::audit::{EventType, apply_state_transition}; use crate::state::tests::checked_sub; use crate::state::tests::{initial_state, received_eth_event}; - use crate::state::transactions::{EthWithdrawalRequest, WithdrawalRequest, create_transaction}; + use crate::state::transactions::{EthWithdrawalRequest, PipelineRequest, WithdrawalRequest}; use crate::state::{EthBalance, State}; use crate::test_fixtures::sweeper_funding_request; use crate::tx::{SignedEip1559TransactionRequest, TransactionSignature}; @@ -2008,20 +2021,19 @@ mod eth_balance { } fn apply(self, state: &mut State) -> TransactionReceipt { - let accepted_withdrawal_request_event = self - .withdrawal_request - .clone() - .into_accepted_withdrawal_request_event(); + let accepted_withdrawal_request_event = + accepted_withdrawal_request_event(self.withdrawal_request.clone()); apply_state_transition(state, &accepted_withdrawal_request_event); - let transaction = create_transaction( - &self.withdrawal_request, - self.nonce, - self.tx_fee, - self.gas_limit, - EthereumNetwork::Sepolia, - ) - .expect("BUG: failed to create transaction"); + let transaction = self + .withdrawal_request + .to_transaction( + self.nonce, + self.tx_fee, + self.gas_limit, + EthereumNetwork::Sepolia, + ) + .expect("BUG: failed to create transaction"); apply_state_transition( state, &EventType::CreatedTransaction { diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 4dd3a03d9bc5..3569dd6fdc6e 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -13,7 +13,6 @@ use crate::numeric::{ CkTokenAmount, Erc20Value, GasAmount, LedgerBurnIndex, LedgerMintIndex, TransactionCount, TransactionNonce, Wei, }; -use crate::state::event::EventType; use crate::tx::{ Eip1559TransactionRequest, FinalizedEip1559Transaction, GasFeeEstimate, ResubmissionStrategy, SignedEip1559TransactionRequest, SignedTransactionRequest, TransactionRequest, @@ -108,18 +107,6 @@ impl WithdrawalRequest { } } - pub fn into_accepted_withdrawal_request_event(self) -> EventType { - match self { - WithdrawalRequest::CkEth(request) => EventType::AcceptedEthWithdrawalRequest(request), - WithdrawalRequest::CkErc20(request) => { - EventType::AcceptedErc20WithdrawalRequest(request) - } - WithdrawalRequest::SweeperFunding(request) => { - EventType::AcceptedSweeperFundingRequest(request) - } - } - } - pub fn match_parameter(&self, parameter: &WithdrawalSearchParameter) -> bool { use WithdrawalSearchParameter::*; match parameter { @@ -377,31 +364,208 @@ impl fmt::Debug for Erc20WithdrawalRequest { } } -/// State machine holding Ethereum transactions issued by the minter. +/// A request that can flow through a [`TransactionPipeline`]: it carries an identity used as the +/// pipeline's alternate map key, and knows the EIP-1559 transaction it turns into. +/// +/// Implemented so far only by [`WithdrawalRequest`], the minter's main-address pipeline +/// (`Id = LedgerBurnIndex`); a second sender address will bring a second implementation. +pub trait PipelineRequest: Clone + Eq + fmt::Debug { + /// The pipeline's alternate map key — a ckETH `LedgerBurnIndex` for withdrawals. + type Id: Copy + Ord + fmt::Debug + fmt::Display; + + /// The identity of this request, used as the pipeline's alternate map key. + fn id(&self) -> Self::Id; + + /// Address the transaction is sent to. + fn destination(&self) -> Address; + + /// IC time at which the request was created, if tracked. + fn created_at(&self) -> Option; + + /// The fee-bump strategy for this request's resubmitted transactions. + fn resubmission_strategy(&self) -> ResubmissionStrategy; + + /// Assert that a freshly created transaction is consistent with the request (amount). + fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest); + + /// Build the EIP-1559 transaction that fulfils this request. + fn to_transaction( + &self, + nonce: TransactionNonce, + gas_fee_estimate: GasFeeEstimate, + gas_limit: GasAmount, + ethereum_network: EthereumNetwork, + ) -> Result; +} + +impl PipelineRequest for WithdrawalRequest { + type Id = LedgerBurnIndex; + + fn id(&self) -> LedgerBurnIndex { + self.cketh_ledger_burn_index() + } + + fn destination(&self) -> Address { + WithdrawalRequest::destination(self) + } + + fn created_at(&self) -> Option { + WithdrawalRequest::created_at(self) + } + + fn resubmission_strategy(&self) -> ResubmissionStrategy { + match self { + WithdrawalRequest::CkEth(cketh) | WithdrawalRequest::SweeperFunding(cketh) => { + ResubmissionStrategy::ReduceEthAmount { + withdrawal_amount: cketh.withdrawal_amount, + } + } + WithdrawalRequest::CkErc20(ckerc20) => ResubmissionStrategy::GuaranteeEthAmount { + allowed_max_transaction_fee: ckerc20.max_transaction_fee, + }, + } + } + + fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest) { + match self { + WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => { + assert!( + req.withdrawal_amount > transaction.amount, + "BUG: transaction amount should be the withdrawal amount deducted from transaction fees" + ); + } + WithdrawalRequest::CkErc20(_req) => { + assert_eq!( + Wei::ZERO, + transaction.amount, + "BUG: ERC-20 transaction amount should be zero" + ); + } + } + } + + fn to_transaction( + &self, + nonce: TransactionNonce, + gas_fee_estimate: GasFeeEstimate, + gas_limit: GasAmount, + ethereum_network: EthereumNetwork, + ) -> Result { + assert!( + gas_limit > GasAmount::ZERO, + "BUG: gas limit should be non-zero" + ); + match self { + WithdrawalRequest::CkEth(EthWithdrawalRequest { + withdrawal_amount, + destination, + ledger_burn_index, + .. + }) + | WithdrawalRequest::SweeperFunding(EthWithdrawalRequest { + withdrawal_amount, + destination, + ledger_burn_index, + .. + }) => { + let transaction_price = gas_fee_estimate.to_price(gas_limit); + let max_transaction_fee = transaction_price.max_transaction_fee(); + let tx_amount = match withdrawal_amount.checked_sub(max_transaction_fee) { + Some(tx_amount) => tx_amount, + None => { + return Err(CreateTransactionError::InsufficientTransactionFee { + cketh_ledger_burn_index: *ledger_burn_index, + allowed_max_transaction_fee: *withdrawal_amount, + actual_max_transaction_fee: max_transaction_fee, + }); + } + }; + Ok(Eip1559TransactionRequest { + chain_id: ethereum_network.chain_id(), + nonce, + max_priority_fee_per_gas: transaction_price.max_priority_fee_per_gas, + max_fee_per_gas: transaction_price.max_fee_per_gas, + gas_limit: transaction_price.gas_limit, + destination: *destination, + amount: tx_amount, + data: Vec::new(), + access_list: Default::default(), + }) + } + WithdrawalRequest::CkErc20(request) => { + // The transaction fee is already paid and must be at most + // the `max_transaction_fee` in the withdrawal request, which, given a gas limit, gives us an upper bound on + // the `max_fee_per_gas`. We allocate the maximum from the beginning to minimize + // transaction resubmissions: even if the `base_fee_per_gas` increases considerably, + // the transaction could still make it as long as `transaction.max_fee_per_gas >= block.base_fee_per_gas`, + // since the `priority_fee_per_gas` received by the miner is capped to (see https://eips.ethereum.org/EIPS/eip-1559) + // min(transaction.max_priority_fee_per_gas, transaction.max_fee_per_gas - block.base_fee_per_gas). + let request_max_fee_per_gas = request + .max_transaction_fee + .into_wei_per_gas(gas_limit) + .expect("BUG: gas_limit should be non-zero"); + let actual_min_max_fee_per_gas = gas_fee_estimate.min_max_fee_per_gas(); + if actual_min_max_fee_per_gas > request_max_fee_per_gas { + return Err(CreateTransactionError::InsufficientTransactionFee { + cketh_ledger_burn_index: request.cketh_ledger_burn_index, + allowed_max_transaction_fee: request.max_transaction_fee, + actual_max_transaction_fee: actual_min_max_fee_per_gas + .transaction_cost(gas_limit) + .unwrap_or(Wei::MAX), + }); + } + Ok(Eip1559TransactionRequest { + chain_id: ethereum_network.chain_id(), + nonce, + max_priority_fee_per_gas: gas_fee_estimate.max_priority_fee_per_gas, + max_fee_per_gas: request_max_fee_per_gas, + gas_limit, + destination: request.erc20_contract_address, + amount: Wei::ZERO, + data: TransactionCallData::Erc20Transfer { + to: request.destination, + value: request.withdrawal_amount, + } + .encode(), + access_list: Default::default(), + }) + } + } + } +} + +/// State machine holding Ethereum transactions issued by the minter from a **single sender +/// address**, on that address' **own nonce sequence** — generic over the request type `R` so a +/// second sender address can drive the same machinery on a nonce sequence of its own without +/// interfering with user withdrawals (`R = WithdrawalRequest`, wrapped by [`WithdrawalTransactions`]). +/// /// Overall the transaction lifecycle is as follows: -/// 1. A withdrawal request is enqueued and processed in a FIFO order. -/// 2. A transaction is created by either consuming a withdrawal request -/// (the first time a transaction is created for that nonce and burn index) -/// or re-submitting an already sent transaction for that nonce and burn index. +/// 1. The request is enqueued and processed in a FIFO order. +/// 2. A transaction is created by either consuming a request +/// (the first time a transaction is created for that nonce and id) +/// or re-submitting an already sent transaction for that nonce and id. /// 3. The transaction is signed via threshold ECDSA and recorded by either consuming the /// previously created transaction or re-submitting an already sent transaction as is. /// 4. The transaction is sent to Ethereum. There may have been multiple -/// sent transactions for that nonce and burn index in case of resubmissions. -/// 5. For a given nonce (and burn index), at most one sent transaction is finalized; a failed -/// one is reported as such. The other sent transactions for that nonce were never mined and -/// can be discarded. Paying the requester back is not the pipeline's concern — see +/// sent transactions for that nonce and id in case of resubmissions. +/// 5. For a given nonce (and id), at most one sent transaction is finalized; a failed one is +/// reported as such. The other sent transactions for that nonce were never mined and can be +/// discarded. Paying the requester back is not the pipeline's concern — see /// [`WithdrawalTransactions`]. #[derive(Clone, Eq, PartialEq, Debug)] -pub(in crate::state) struct TransactionPipeline { - pending_withdrawal_requests: VecDeque, - // Processed withdrawal requests (transaction created, sent, or finalized). - processed_withdrawal_requests: BTreeMap, - created_tx: MultiKeyMap, - sent_tx: MultiKeyMap>, - finalized_tx: MultiKeyMap, +pub struct TransactionPipeline { + pending_requests: VecDeque, + // Processed requests (transaction created, sent, or finalized). + processed_requests: BTreeMap, + created_tx: MultiKeyMap, + sent_tx: MultiKeyMap>, + finalized_tx: MultiKeyMap, next_nonce: TransactionNonce, } +/// The pipeline sending from the minter's main address, on which user withdrawals travel. +pub type MinterTransactionPipeline = TransactionPipeline; + #[derive(Clone, Eq, PartialEq, Debug)] pub enum CreateTransactionError { InsufficientTransactionFee { @@ -412,9 +576,9 @@ pub enum CreateTransactionError { } #[derive(Clone, Eq, PartialEq, Debug)] -pub enum ResubmitTransactionError { +pub enum ResubmitTransactionError { InsufficientTransactionFee { - ledger_burn_index: LedgerBurnIndex, + id: Id, transaction_nonce: TransactionNonce, allowed_max_transaction_fee: Wei, max_transaction_fee: Wei, @@ -431,11 +595,15 @@ pub(in crate::state) enum TransactionStage<'a> { Finalized(&'a FinalizedEip1559Transaction), } -impl TransactionPipeline { +/// One outcome of [`TransactionPipeline::create_resubmit_transactions`]: the fee-bumped transaction to +/// re-sign (paired with its pipeline id), or why it could not be bumped. +type ResubmitResult = Result<(Id, Eip1559TransactionRequest), ResubmitTransactionError>; + +impl TransactionPipeline { pub fn new(next_nonce: TransactionNonce) -> Self { Self { - pending_withdrawal_requests: VecDeque::new(), - processed_withdrawal_requests: BTreeMap::new(), + pending_requests: VecDeque::new(), + processed_requests: BTreeMap::new(), created_tx: MultiKeyMap::default(), sent_tx: MultiKeyMap::default(), finalized_tx: MultiKeyMap::default(), @@ -451,107 +619,73 @@ impl TransactionPipeline { self.next_nonce = new_nonce; } - pub fn record_withdrawal_request>(&mut self, request: R) { + pub fn record_request>(&mut self, request: Req) { let request = request.into(); - let burn_index = request.cketh_ledger_burn_index(); - if self - .pending_withdrawal_requests - .iter() - .any(|r| r.cketh_ledger_burn_index() == burn_index) + let burn_index = request.id(); + if self.pending_requests.iter().any(|r| r.id() == burn_index) || self.created_tx.contains_alt(&burn_index) || self.sent_tx.contains_alt(&burn_index) || self.finalized_tx.contains_alt(&burn_index) { - panic!("BUG: duplicate ckETH ledger burn index {burn_index}"); + panic!("BUG: duplicate transaction id {burn_index}"); } - self.pending_withdrawal_requests.push_back(request); + self.pending_requests.push_back(request); } - /// Move an existing withdrawal request to the back of the queue. - pub fn reschedule_withdrawal_request>(&mut self, request: R) { + /// Move an existing request to the back of the queue. + pub fn reschedule_request>(&mut self, request: Req) { let request = request.into(); assert_eq!( - self.pending_withdrawal_requests + self.pending_requests .iter() - .filter(|r| r.cketh_ledger_burn_index() == request.cketh_ledger_burn_index()) + .filter(|r| r.id() == request.id()) .count(), 1, - "BUG: expected exactly one withdrawal request with ckETH ledger burn index {}", - request.cketh_ledger_burn_index() + "BUG: expected exactly one request with id {}", + request.id() ); - self.remove_withdrawal_request(&request); - self.record_withdrawal_request(request); + self.remove_request(&request); + self.record_request(request); } pub fn record_created_transaction( &mut self, - withdrawal_id: LedgerBurnIndex, + id: R::Id, transaction: Eip1559TransactionRequest, ) { - let withdrawal_request = self - .pending_withdrawal_requests + let request = self + .pending_requests .iter() - .find(|req| req.cketh_ledger_burn_index() == withdrawal_id) + .find(|req| req.id() == id) .cloned() - .unwrap_or_else(|| panic!("BUG: withdrawal request {withdrawal_id} not found")); + .unwrap_or_else(|| panic!("BUG: request {id} not found")); assert!( - self.pending_withdrawal_requests - .contains(&withdrawal_request), - "BUG: withdrawal request not found" + self.pending_requests.contains(&request), + "BUG: request not found" ); assert_eq!( - withdrawal_request.destination(), + request.destination(), transaction.destination, - "BUG: withdrawal request and transaction destination mismatch" + "BUG: request and transaction destination mismatch" ); - match &withdrawal_request { - WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => { - assert!( - req.withdrawal_amount > transaction.amount, - "BUG: transaction amount should be the withdrawal amount deducted from transaction fees" - ); - } - WithdrawalRequest::CkErc20(_req) => { - assert_eq!( - Wei::ZERO, - transaction.amount, - "BUG: ERC-20 transaction amount should be zero" - ); - } - } + request.assert_created_transaction(&transaction); let nonce = self.next_nonce; assert_eq!(transaction.nonce, nonce, "BUG: transaction nonce mismatch"); self.next_nonce = self .next_nonce .checked_increment() .expect("Transaction nonce overflow"); - self.remove_withdrawal_request(&withdrawal_request); + self.remove_request(&request); let transaction_request = TransactionRequest { transaction, - resubmission: match &withdrawal_request { - WithdrawalRequest::CkEth(cketh) | WithdrawalRequest::SweeperFunding(cketh) => { - ResubmissionStrategy::ReduceEthAmount { - withdrawal_amount: cketh.withdrawal_amount, - } - } - WithdrawalRequest::CkErc20(ckerc20) => ResubmissionStrategy::GuaranteeEthAmount { - allowed_max_transaction_fee: ckerc20.max_transaction_fee, - }, - }, + resubmission: request.resubmission_strategy(), }; assert_eq!( - self.created_tx.try_insert( - nonce, - withdrawal_request.cketh_ledger_burn_index(), - transaction_request - ), + self.created_tx + .try_insert(nonce, request.id(), transaction_request), Ok(()) ); - assert_eq!( - self.processed_withdrawal_requests - .insert(withdrawal_id, withdrawal_request), - None - ); + assert_eq!(self.processed_requests.insert(id, request), None); } pub fn record_signed_transaction( @@ -596,7 +730,7 @@ impl TransactionPipeline { &self, latest_transaction_count: TransactionCount, current_gas_fee: GasFeeEstimate, - ) -> Vec> { + ) -> Vec> { // If transaction count at block height H is c > 0, then transactions with nonces // 0, 1, ..., c - 1 were mined. If transaction count is 0, then no transactions were mined. // The nonce of the first pending transaction is then exactly c. @@ -623,7 +757,7 @@ impl TransactionPipeline { }) => { transactions_to_resubmit.push(Err( ResubmitTransactionError::InsufficientTransactionFee { - ledger_burn_index: *burn_index, + id: *burn_index, transaction_nonce: *nonce, allowed_max_transaction_fee, max_transaction_fee: actual_max_transaction_fee, @@ -656,7 +790,7 @@ impl TransactionPipeline { pub fn sent_transactions_to_finalize( &self, finalized_transaction_count: &TransactionCount, - ) -> BTreeMap { + ) -> BTreeMap { let first_non_finalized_tx_nonce: TransactionNonce = finalized_transaction_count.change_units(); let mut transactions = BTreeMap::new(); @@ -683,12 +817,12 @@ impl TransactionPipeline { /// superseded resubmissions, returning the finalized transaction. pub fn record_finalized_transaction( &mut self, - ledger_burn_index: LedgerBurnIndex, + id: R::Id, receipt: &TransactionReceipt, ) -> FinalizedEip1559Transaction { let sent_tx = self .sent_tx - .get_alt(&ledger_burn_index) + .get_alt(&id) .expect("BUG: missing sent transactions") .iter() .find(|sent_tx| sent_tx.as_ref().hash() == receipt.transaction_hash) @@ -706,13 +840,13 @@ impl TransactionPipeline { } assert_eq!( self.finalized_tx - .try_insert(nonce, ledger_burn_index, finalized_tx.clone()), + .try_insert(nonce, id, finalized_tx.clone()), Ok(()) ); finalized_tx } - pub fn withdrawal_requests_batch(&self, requested_batch_size: usize) -> Vec { + pub fn requests_batch(&self, requested_batch_size: usize) -> Vec { // The number of pending transaction nonces is counted and not the number of pending transactions // because a nonce may be associated with several distinct transactions (due to re-submission and dynamic fees). // However, once a nonce is chosen for a withdrawal request, it's in our interest that the corresponding transaction be finalized asap. @@ -725,29 +859,23 @@ impl TransactionPipeline { .saturating_sub(unique_pending_transaction_nonces.len()), requested_batch_size, ); - self.withdrawal_requests_iter() + self.requests_iter() .take(actual_batch_size) .cloned() .collect() } - pub fn withdrawal_requests_iter(&self) -> impl Iterator { - self.pending_withdrawal_requests.iter() + pub fn requests_iter(&self) -> impl Iterator { + self.pending_requests.iter() } - pub fn withdrawal_requests_len(&self) -> usize { - self.pending_withdrawal_requests.len() + pub fn requests_len(&self) -> usize { + self.pending_requests.len() } pub fn transactions_to_sign_iter( &self, - ) -> impl Iterator< - Item = ( - &TransactionNonce, - &LedgerBurnIndex, - &Eip1559TransactionRequest, - ), - > { + ) -> impl Iterator { self.created_tx .iter() .map(|(nonce, ledger_burn_index, tx)| (nonce, ledger_burn_index, tx.as_ref())) @@ -756,7 +884,7 @@ impl TransactionPipeline { pub fn transactions_to_sign_batch( &self, batch_size: usize, - ) -> Vec<(LedgerBurnIndex, Eip1559TransactionRequest)> { + ) -> Vec<(R::Id, Eip1559TransactionRequest)> { self.transactions_to_sign_iter() .take(batch_size) .map(|(_nonce, withdrawal_id, tx)| (*withdrawal_id, tx.clone())) @@ -787,7 +915,7 @@ impl TransactionPipeline { ) -> impl Iterator< Item = ( &TransactionNonce, - &LedgerBurnIndex, + &R::Id, Vec<&SignedEip1559TransactionRequest>, ), > { @@ -798,45 +926,36 @@ impl TransactionPipeline { pub fn get_finalized_transaction( &self, - burn_index: &LedgerBurnIndex, + burn_index: &R::Id, ) -> Option<&FinalizedEip1559Transaction> { self.finalized_tx.get_alt(burn_index) } - pub fn processed_withdrawal_requests_iter(&self) -> impl Iterator { - self.processed_withdrawal_requests.values() + pub fn processed_requests_iter(&self) -> impl Iterator { + self.processed_requests.values() } - /// How far the transaction for `burn_index` has got, or `None` if the pipeline holds none. - pub fn transaction_stage(&self, burn_index: &LedgerBurnIndex) -> Option> { - if let Some(tx) = self.created_tx.get_alt(burn_index) { + /// How far the transaction for `id` has got, or `None` if the pipeline holds none. + pub fn transaction_stage(&self, id: &R::Id) -> Option> { + if let Some(tx) = self.created_tx.get_alt(id) { return Some(TransactionStage::Created(tx.as_ref())); } // The last one sent is the one with the highest fee, so it is the one that may be mined. - if let Some(tx) = self.sent_tx.get_alt(burn_index).and_then(|txs| txs.last()) { + if let Some(tx) = self.sent_tx.get_alt(id).and_then(|txs| txs.last()) { return Some(TransactionStage::Sent(tx)); } self.finalized_tx - .get_alt(burn_index) + .get_alt(id) .map(TransactionStage::Finalized) } - pub fn get_processed_withdrawal_request( - &self, - burn_index: &LedgerBurnIndex, - ) -> Option<&WithdrawalRequest> { - self.processed_withdrawal_requests.get(burn_index) + pub fn get_processed_request(&self, burn_index: &R::Id) -> Option<&R> { + self.processed_requests.get(burn_index) } pub fn finalized_transactions_iter( &self, - ) -> impl Iterator< - Item = ( - &TransactionNonce, - &LedgerBurnIndex, - &FinalizedEip1559Transaction, - ), - > { + ) -> impl Iterator { self.finalized_tx.iter() } @@ -845,19 +964,17 @@ impl TransactionPipeline { } pub fn has_pending_requests(&self) -> bool { - !self.pending_withdrawal_requests.is_empty() - || !self.created_tx.is_empty() - || !self.sent_tx.is_empty() + !self.pending_requests.is_empty() || !self.created_tx.is_empty() || !self.sent_tx.is_empty() } - fn remove_withdrawal_request(&mut self, request: &WithdrawalRequest) { - self.pending_withdrawal_requests.retain(|r| r != request); + fn remove_request(&mut self, request: &R) { + self.pending_requests.retain(|r| r != request); } fn expect_last_sent_tx_entry<'a>( - sent_tx: &'a MultiKeyMap>, + sent_tx: &'a MultiKeyMap>, nonce: &TransactionNonce, - ) -> (&'a LedgerBurnIndex, &'a SignedTransactionRequest) { + ) -> (&'a R::Id, &'a SignedTransactionRequest) { let (ledger_burn_index, sent_txs) = sent_tx .get_entry(nonce) .expect("BUG: sent transaction not found"); @@ -866,7 +983,7 @@ impl TransactionPipeline { } fn cleanup_failed_resubmitted_transactions( - created_tx: &mut MultiKeyMap, + created_tx: &mut MultiKeyMap, nonce: &TransactionNonce, ) { use crate::logs::INFO; @@ -884,17 +1001,17 @@ impl TransactionPipeline { pub fn is_equivalent_to(&self, other: &Self) -> Result<(), String> { use ic_utils_ensure::ensure_eq; - fn sorted_requests(requests: &VecDeque) -> Vec { + fn sorted_requests(requests: &VecDeque) -> Vec { let mut buf: Vec<_> = requests.iter().cloned().collect(); - buf.sort_unstable_by_key(|req| req.cketh_ledger_burn_index()); + buf.sort_unstable_by_key(|req| req.id()); buf } - // We can reorder request in `reschedule_withdrawal_request`. The audit log won't + // We can reorder request in `reschedule_request`. The audit log won't // reflect this change, so we must sort the queues before comparing them. ensure_eq!( - sorted_requests(&self.pending_withdrawal_requests), - sorted_requests(&other.pending_withdrawal_requests) + sorted_requests(&self.pending_requests), + sorted_requests(&other.pending_requests) ); ensure_eq!(self.created_tx, other.created_tx); ensure_eq!(self.sent_tx, other.sent_tx); @@ -905,13 +1022,13 @@ impl TransactionPipeline { } } -/// The minter's transaction pipeline, carrying user withdrawals, together with the reimbursement -/// bookkeeping only a withdrawal can need: a failed ckETH/ckERC20 transaction pays the user back, -/// so the send machinery and the ledger-side refund have to stay in step. +/// The minter's main-address pipeline, carrying user withdrawals, together with the reimbursement +/// bookkeeping that only a withdrawal can need: a failed ckETH/ckERC20 transaction pays the user +/// back, so the pipeline's send machinery and the ledger-side refund have to stay in step. #[derive(Clone, Eq, PartialEq, Debug)] pub struct WithdrawalTransactions { - pipeline: TransactionPipeline, - /// Requests whose transaction was created but has not finally settled, and which would + pipeline: MinterTransactionPipeline, + /// Requests whose transaction was created but has not yet finally settled, and which would /// therefore have to be paid back if it failed. maybe_reimburse: BTreeSet, reimbursement_requests: BTreeMap, @@ -928,16 +1045,15 @@ impl WithdrawalTransactions { } } - /// Record a created transaction, and remember the request may still need paying back. + /// Record a created transaction, and remember that the request may still need paying back. pub fn record_created_transaction( &mut self, - withdrawal_id: LedgerBurnIndex, + id: LedgerBurnIndex, transaction: Eip1559TransactionRequest, ) { - self.pipeline - .record_created_transaction(withdrawal_id, transaction); - if self.is_reimbursable(&withdrawal_id) { - assert!(self.maybe_reimburse.insert(withdrawal_id)); + self.pipeline.record_created_transaction(id, transaction); + if self.is_reimbursable(&id) { + assert!(self.maybe_reimburse.insert(id)); } } @@ -945,7 +1061,7 @@ impl WithdrawalTransactions { /// funding never is, so it is never armed for reimbursement in the first place. fn is_reimbursable(&self, withdrawal_id: &LedgerBurnIndex) -> bool { self.pipeline - .get_processed_withdrawal_request(withdrawal_id) + .get_processed_request(withdrawal_id) .expect("BUG: missing processed withdrawal request") .is_reimbursable() } @@ -969,7 +1085,7 @@ impl WithdrawalTransactions { let request = self .pipeline - .get_processed_withdrawal_request(&ledger_burn_index) + .get_processed_request(&ledger_burn_index) .expect("BUG: missing processed withdrawal request"); if receipt.status != TransactionStatus::Failure { return; @@ -1026,6 +1142,20 @@ impl WithdrawalTransactions { self.record_reimbursement_request(index, reimbursement); } + /// Whether any request is still in flight, either awaiting a transaction or a reimbursement. + pub fn oldest_incomplete_request_timestamp(&self) -> Option { + self.requests_iter() + .chain(self.maybe_reimburse_requests_iter()) + .flat_map(|req| req.created_at().into_iter()) + .min() + } + + fn maybe_reimburse_requests_iter(&self) -> impl Iterator { + self.maybe_reimburse + .iter() + .filter_map(|index| self.pipeline.get_processed_request(index)) + } + pub fn is_equivalent_to(&self, other: &Self) -> Result<(), String> { use ic_utils_ensure::ensure_eq; @@ -1035,6 +1165,71 @@ impl WithdrawalTransactions { self.pipeline.is_equivalent_to(&other.pipeline) } + pub fn reimbursement_requests_iter( + &self, + ) -> impl Iterator { + self.reimbursement_requests.iter() + } + + pub fn reimbursed_transactions_iter( + &self, + ) -> impl Iterator { + self.reimbursed.iter() + } + + fn find_reimbursed_transaction_by_cketh_ledger_burn_index( + &self, + searched_burn_index: &LedgerBurnIndex, + ) -> Option<&ReimbursedResult> { + self.reimbursed + .iter() + .find_map(|(index, value)| match index { + ReimbursementIndex::CkEth { ledger_burn_index } + if ledger_burn_index == searched_burn_index => + { + Some(value) + } + ReimbursementIndex::CkErc20 { + cketh_ledger_burn_index, + .. + } if cketh_ledger_burn_index == searched_burn_index => Some(value), + _ => None, + }) + } + + /// Quarantine the reimbursement request identified by its index to prevent double minting. + /// WARNING!: It's crucial that this method does not panic, + /// since it's called inside the clean-up callback, when an unexpected panic did occur before. + pub fn record_quarantined_reimbursement(&mut self, index: ReimbursementIndex) { + self.reimbursement_requests.remove(&index); + self.reimbursed + .insert(index, Err(ReimbursedError::Quarantined)); + } + + pub fn record_finalized_reimbursement( + &mut self, + index: ReimbursementIndex, + reimbursed_in_block: LedgerMintIndex, + ) { + let reimbursement_request = self + .reimbursement_requests + .remove(&index) + .unwrap_or_else(|| panic!("BUG: missing reimbursement request with index {index:?}")); + let burn_in_block = index.burn_in_block(); + assert_eq!( + self.reimbursed.insert( + index, + Ok(Reimbursed { + burn_in_block, + reimbursed_in_block, + reimbursed_amount: reimbursement_request.reimbursed_amount, + transaction_hash: reimbursement_request.transaction_hash, + }), + ), + None + ); + } + pub fn next_transaction_nonce(&self) -> TransactionNonce { self.pipeline.next_transaction_nonce() } @@ -1043,12 +1238,12 @@ impl WithdrawalTransactions { self.pipeline.update_next_transaction_nonce(new_nonce) } - pub fn record_withdrawal_request>(&mut self, request: R) { - self.pipeline.record_withdrawal_request(request) + pub fn record_request>(&mut self, request: Req) { + self.pipeline.record_request(request) } - pub fn reschedule_withdrawal_request>(&mut self, request: R) { - self.pipeline.reschedule_withdrawal_request(request) + pub fn reschedule_request>(&mut self, request: Req) { + self.pipeline.reschedule_request(request) } pub fn record_signed_transaction( @@ -1062,7 +1257,7 @@ impl WithdrawalTransactions { &self, latest_transaction_count: TransactionCount, current_gas_fee: GasFeeEstimate, - ) -> Vec> { + ) -> Vec> { self.pipeline .create_resubmit_transactions(latest_transaction_count, current_gas_fee) } @@ -1079,17 +1274,16 @@ impl WithdrawalTransactions { .sent_transactions_to_finalize(finalized_transaction_count) } - pub fn withdrawal_requests_batch(&self, requested_batch_size: usize) -> Vec { - self.pipeline - .withdrawal_requests_batch(requested_batch_size) + pub fn requests_batch(&self, requested_batch_size: usize) -> Vec { + self.pipeline.requests_batch(requested_batch_size) } - pub fn withdrawal_requests_iter(&self) -> impl Iterator { - self.pipeline.withdrawal_requests_iter() + pub fn requests_iter(&self) -> impl Iterator { + self.pipeline.requests_iter() } - pub fn withdrawal_requests_len(&self) -> usize { - self.pipeline.withdrawal_requests_len() + pub fn requests_len(&self) -> usize { + self.pipeline.requests_len() } pub fn transactions_to_sign_iter( @@ -1139,11 +1333,11 @@ impl WithdrawalTransactions { self.pipeline.get_finalized_transaction(burn_index) } - pub fn get_processed_withdrawal_request( + pub fn get_processed_request( &self, burn_index: &LedgerBurnIndex, ) -> Option<&WithdrawalRequest> { - self.pipeline.get_processed_withdrawal_request(burn_index) + self.pipeline.get_processed_request(burn_index) } pub fn finalized_transactions_iter( @@ -1165,72 +1359,13 @@ impl WithdrawalTransactions { pub fn has_pending_requests(&self) -> bool { self.pipeline.has_pending_requests() } +} - pub fn reimbursement_requests_iter( - &self, - ) -> impl Iterator { - self.reimbursement_requests.iter() - } - - pub fn reimbursed_transactions_iter( - &self, - ) -> impl Iterator { - self.reimbursed.iter() - } - - fn find_reimbursed_transaction_by_cketh_ledger_burn_index( - &self, - searched_burn_index: &LedgerBurnIndex, - ) -> Option<&ReimbursedResult> { - self.reimbursed - .iter() - .find_map(|(index, value)| match index { - ReimbursementIndex::CkEth { ledger_burn_index } - if ledger_burn_index == searched_burn_index => - { - Some(value) - } - ReimbursementIndex::CkErc20 { - cketh_ledger_burn_index, - .. - } if cketh_ledger_burn_index == searched_burn_index => Some(value), - _ => None, - }) - } - - /// Quarantine the reimbursement request identified by its index to prevent double minting. - /// WARNING!: It's crucial that this method does not panic, - /// since it's called inside the clean-up callback, when an unexpected panic did occur before. - pub fn record_quarantined_reimbursement(&mut self, index: ReimbursementIndex) { - self.reimbursement_requests.remove(&index); - self.reimbursed - .insert(index, Err(ReimbursedError::Quarantined)); - } - - pub fn record_finalized_reimbursement( - &mut self, - index: ReimbursementIndex, - reimbursed_in_block: LedgerMintIndex, - ) { - let reimbursement_request = self - .reimbursement_requests - .remove(&index) - .unwrap_or_else(|| panic!("BUG: missing reimbursement request with index {index:?}")); - let burn_in_block = index.burn_in_block(); - assert_eq!( - self.reimbursed.insert( - index, - Ok(Reimbursed { - burn_in_block, - reimbursed_in_block, - reimbursed_amount: reimbursement_request.reimbursed_amount, - transaction_hash: reimbursement_request.transaction_hash, - }), - ), - None - ); - } - +/// Main-address pipeline behavior that is specific to ckETH/ckERC20 withdrawals: reimbursing a failed +/// transaction and answering the withdrawal-status endpoints. The sweeper pipeline has none of this. +impl WithdrawalTransactions { + /// Finalize the transaction for `ledger_burn_index` matching `receipt`, then — if it failed on + /// chain — record the corresponding ckETH/ckERC20 reimbursement. pub fn record_reimbursement_request( &mut self, index: ReimbursementIndex, @@ -1253,19 +1388,6 @@ impl WithdrawalTransactions { ); } - pub fn maybe_reimburse_requests_iter(&self) -> impl Iterator { - self.maybe_reimburse - .iter() - .filter_map(|index| self.pipeline.get_processed_withdrawal_request(index)) - } - - pub fn oldest_incomplete_withdrawal_timestamp(&self) -> Option { - self.withdrawal_requests_iter() - .chain(self.maybe_reimburse_requests_iter()) - .flat_map(|req| req.created_at().into_iter()) - .min() - } - pub fn withdrawal_status( &self, parameter: &WithdrawalSearchParameter, @@ -1275,7 +1397,7 @@ impl WithdrawalTransactions { Option<&Eip1559TransactionRequest>, )> { // Pending requests matching the given search parameter - let pending = self.pipeline.withdrawal_requests_iter().filter_map(|r| { + let pending = self.pipeline.requests_iter().filter_map(|r| { r.match_parameter(parameter) .then_some((r, WithdrawalStatus::Pending, None)) }); @@ -1283,7 +1405,7 @@ impl WithdrawalTransactions { // Processed withdrawal requests matching the given search parameter. let processed = self .pipeline - .processed_withdrawal_requests_iter() + .processed_requests_iter() .filter(|r| r.match_parameter(parameter)) .map(|request| { match self.processed_transaction_status(&request.cketh_ledger_burn_index()) { @@ -1308,7 +1430,7 @@ impl WithdrawalTransactions { pub fn transaction_status(&self, burn_index: &LedgerBurnIndex) -> RetrieveEthStatus { if self .pipeline - .withdrawal_requests_iter() + .requests_iter() .any(|r| &r.cketh_ledger_burn_index() == burn_index) { return RetrieveEthStatus::Pending; @@ -1372,101 +1494,12 @@ impl WithdrawalTransactions { } } -/// Creates an EIP-1559 transaction for the given withdrawal request. +/// Creates an EIP-1559 transaction for the given pipeline request. /// The transaction fees are paid by the beneficiary, /// meaning that the fees will be deducted from the withdrawal amount. /// /// # Errors /// * `CreateTransactionError::InsufficientTransactionFee` if the ETH withdrawal amount does not cover the transaction fee. -pub fn create_transaction( - withdrawal_request: &WithdrawalRequest, - nonce: TransactionNonce, - gas_fee_estimate: GasFeeEstimate, - gas_limit: GasAmount, - ethereum_network: EthereumNetwork, -) -> Result { - assert!( - gas_limit > GasAmount::ZERO, - "BUG: gas limit should be non-zero" - ); - match withdrawal_request { - WithdrawalRequest::CkEth(EthWithdrawalRequest { - withdrawal_amount, - destination, - ledger_burn_index, - .. - }) - | WithdrawalRequest::SweeperFunding(EthWithdrawalRequest { - withdrawal_amount, - destination, - ledger_burn_index, - .. - }) => { - let transaction_price = gas_fee_estimate.to_price(gas_limit); - let max_transaction_fee = transaction_price.max_transaction_fee(); - let tx_amount = match withdrawal_amount.checked_sub(max_transaction_fee) { - Some(tx_amount) => tx_amount, - None => { - return Err(CreateTransactionError::InsufficientTransactionFee { - cketh_ledger_burn_index: *ledger_burn_index, - allowed_max_transaction_fee: *withdrawal_amount, - actual_max_transaction_fee: max_transaction_fee, - }); - } - }; - Ok(Eip1559TransactionRequest { - chain_id: ethereum_network.chain_id(), - nonce, - max_priority_fee_per_gas: transaction_price.max_priority_fee_per_gas, - max_fee_per_gas: transaction_price.max_fee_per_gas, - gas_limit: transaction_price.gas_limit, - destination: *destination, - amount: tx_amount, - data: Vec::new(), - access_list: Default::default(), - }) - } - WithdrawalRequest::CkErc20(request) => { - // The transaction fee is already paid and must be at most - // the `max_transaction_fee` in the withdrawal request, which, given a gas limit, gives us an upper bound on - // the `max_fee_per_gas`. We allocate the maximum from the beginning to minimize - // transaction resubmissions: even if the `base_fee_per_gas` increases considerably, - // the transaction could still make it as long as `transaction.max_fee_per_gas >= block.base_fee_per_gas`, - // since the `priority_fee_per_gas` received by the miner is capped to (see https://eips.ethereum.org/EIPS/eip-1559) - // min(transaction.max_priority_fee_per_gas, transaction.max_fee_per_gas - block.base_fee_per_gas). - let request_max_fee_per_gas = request - .max_transaction_fee - .into_wei_per_gas(gas_limit) - .expect("BUG: gas_limit should be non-zero"); - let actual_min_max_fee_per_gas = gas_fee_estimate.min_max_fee_per_gas(); - if actual_min_max_fee_per_gas > request_max_fee_per_gas { - return Err(CreateTransactionError::InsufficientTransactionFee { - cketh_ledger_burn_index: request.cketh_ledger_burn_index, - allowed_max_transaction_fee: request.max_transaction_fee, - actual_max_transaction_fee: actual_min_max_fee_per_gas - .transaction_cost(gas_limit) - .unwrap_or(Wei::MAX), - }); - } - Ok(Eip1559TransactionRequest { - chain_id: ethereum_network.chain_id(), - nonce, - max_priority_fee_per_gas: gas_fee_estimate.max_priority_fee_per_gas, - max_fee_per_gas: request_max_fee_per_gas, - gas_limit, - destination: request.erc20_contract_address, - amount: Wei::ZERO, - data: TransactionCallData::Erc20Transfer { - to: request.destination, - value: request.withdrawal_amount, - } - .encode(), - access_list: Default::default(), - }) - } - } -} - // First 4 bytes of keccak256(transfer(address,uint256)) const ERC_20_TRANSFER_FUNCTION_SELECTOR: [u8; 4] = hex_literal::hex!("a9059cbb"); diff --git a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs index d59e8d535881..8e9e54fef1d0 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs @@ -6,10 +6,9 @@ use crate::lifecycle::EthereumNetwork; use crate::numeric::{ BlockNumber, Erc20Value, GasAmount, LedgerBurnIndex, TransactionNonce, Wei, WeiPerGas, }; -use crate::state::transactions::TransactionPipeline; use crate::state::transactions::{ - Erc20WithdrawalRequest, EthWithdrawalRequest, WithdrawalRequest, WithdrawalTransactions, - create_transaction, + Erc20WithdrawalRequest, EthWithdrawalRequest, MinterTransactionPipeline, PipelineRequest, + WithdrawalRequest, WithdrawalTransactions, }; use crate::tx::{ AccessList, Eip1559TransactionRequest, GasFeeEstimate, SignedEip1559TransactionRequest, @@ -42,7 +41,7 @@ mod withdrawal_transactions { TransactionStatus, WithdrawalRequest, WithdrawalTransactions, }; - mod record_withdrawal_request { + mod record_request { use super::*; use crate::state::transactions::WithdrawalRequest; use crate::state::transactions::tests::{ @@ -55,10 +54,10 @@ mod withdrawal_transactions { fn should_record_withdrawal_request() { fn test + Clone>(withdrawal_request: R) { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); assert_eq!( - transactions.withdrawal_requests_batch(5), + transactions.requests_batch(5), vec![withdrawal_request.into()] ); } @@ -79,11 +78,11 @@ mod withdrawal_transactions { duplicate_index: S, ) { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); expect_panic_with_message( - || transactions.record_withdrawal_request(duplicate_index.clone()), - "duplicate ckETH ledger burn index", + || transactions.record_request(duplicate_index.clone()), + "duplicate transaction id", ); let created_tx = create_and_record_transaction( @@ -92,14 +91,14 @@ mod withdrawal_transactions { gas_fee_estimate(), ); expect_panic_with_message( - || transactions.record_withdrawal_request(duplicate_index.clone()), - "duplicate ckETH ledger burn index", + || transactions.record_request(duplicate_index.clone()), + "duplicate transaction id", ); let signed_tx = create_and_record_signed_transaction(&mut transactions, created_tx); expect_panic_with_message( - || transactions.record_withdrawal_request(duplicate_index.clone()), - "duplicate ckETH ledger burn index", + || transactions.record_request(duplicate_index.clone()), + "duplicate transaction id", ); transactions.record_finalized_transaction( @@ -107,8 +106,8 @@ mod withdrawal_transactions { transaction_receipt(&signed_tx, TransactionStatus::Success), ); expect_panic_with_message( - || transactions.record_withdrawal_request(duplicate_index.clone()), - "duplicate ckETH ledger burn index", + || transactions.record_request(duplicate_index.clone()), + "duplicate transaction id", ); } @@ -133,7 +132,7 @@ mod withdrawal_transactions { } } - mod withdrawal_requests_batch { + mod requests_batch { use super::*; use crate::state::transactions::WithdrawalRequest; use crate::state::transactions::tests::{ @@ -147,7 +146,7 @@ mod withdrawal_transactions { #[test] fn should_be_empty_when_no_withdrawal_requests() { let transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); - assert_eq!(transactions.withdrawal_requests_batch(5), vec![]); + assert_eq!(transactions.requests_batch(5), vec![]); } #[test] @@ -157,13 +156,13 @@ mod withdrawal_transactions { let withdrawal_requests: [WithdrawalRequest; 5] = create_and_record_ck_withdrawal_requests(&mut transactions, &mut rng); - let requests = transactions.withdrawal_requests_batch(0); + let requests = transactions.requests_batch(0); assert_eq!(requests, vec![]); - let requests = transactions.withdrawal_requests_batch(1); + let requests = transactions.requests_batch(1); assert_eq!(requests.as_slice(), &withdrawal_requests[0..=0]); - let requests = transactions.withdrawal_requests_batch(2); + let requests = transactions.requests_batch(2); assert_eq!(&requests, &withdrawal_requests[0..=1]); } @@ -175,7 +174,7 @@ mod withdrawal_transactions { let withdrawal_requests: [WithdrawalRequest; 3] = create_and_record_ck_withdrawal_requests(&mut transactions, &mut rng); - let requests = transactions.withdrawal_requests_batch(batch_size); + let requests = transactions.requests_batch(batch_size); prop_assert_eq!(requests, withdrawal_requests); } @@ -199,7 +198,7 @@ mod withdrawal_transactions { }); assert_eq!( - transactions.withdrawal_requests_batch(3).as_slice(), + transactions.requests_batch(3).as_slice(), &withdrawal_requests[997..=999] ); @@ -209,7 +208,7 @@ mod withdrawal_transactions { rng.r#gen(), ); assert_eq!( - transactions.withdrawal_requests_batch(3).as_slice(), + transactions.requests_batch(3).as_slice(), &withdrawal_requests[998..=999] ); @@ -219,7 +218,7 @@ mod withdrawal_transactions { rng.r#gen(), ); assert_eq!( - transactions.withdrawal_requests_batch(3).as_slice(), + transactions.requests_batch(3).as_slice(), &withdrawal_requests[999..=999] ); @@ -228,7 +227,7 @@ mod withdrawal_transactions { withdrawal_requests[999].clone(), rng.r#gen(), ); - assert_eq!(transactions.withdrawal_requests_batch(3), vec![]); + assert_eq!(transactions.requests_batch(3), vec![]); } fn create_and_record_pending_transaction>( @@ -244,7 +243,7 @@ mod withdrawal_transactions { } } - mod reschedule_withdrawal_request { + mod reschedule_request { use crate::numeric::TransactionNonce; use crate::state::transactions::WithdrawalTransactions; use crate::state::transactions::tests::create_and_record_ck_withdrawal_requests; @@ -258,7 +257,7 @@ mod withdrawal_transactions { create_and_record_ck_withdrawal_requests(&mut transactions, &mut rng); // 3 -> 2 -> 1 assert_eq!( - transactions.withdrawal_requests_batch(5), + transactions.requests_batch(5), vec![ first_request.clone(), second_request.clone(), @@ -266,10 +265,10 @@ mod withdrawal_transactions { ] ); - transactions.reschedule_withdrawal_request(first_request.clone()); + transactions.reschedule_request(first_request.clone()); // 1 -> 3 -> 2 assert_eq!( - transactions.withdrawal_requests_batch(5), + transactions.requests_batch(5), vec![ second_request.clone(), third_request.clone(), @@ -277,10 +276,10 @@ mod withdrawal_transactions { ] ); - transactions.reschedule_withdrawal_request(second_request.clone()); + transactions.reschedule_request(second_request.clone()); // 2 -> 1 -> 3 assert_eq!( - transactions.withdrawal_requests_batch(5), + transactions.requests_batch(5), vec![ third_request.clone(), first_request.clone(), @@ -288,10 +287,10 @@ mod withdrawal_transactions { ] ); - transactions.reschedule_withdrawal_request(third_request.clone()); + transactions.reschedule_request(third_request.clone()); // 3 -> 2 -> 1 assert_eq!( - transactions.withdrawal_requests_batch(5), + transactions.requests_batch(5), vec![first_request, second_request, third_request] ); } @@ -306,7 +305,9 @@ mod withdrawal_transactions { cketh_withdrawal_request_with_index, create_and_record_ck_withdrawal_requests, create_and_record_transaction, create_ck_withdrawal_requests, gas_fee_estimate, }; - use crate::state::transactions::{WithdrawalTransactions, create_transaction}; + use crate::state::transactions::{ + PipelineRequest, WithdrawalRequest, WithdrawalTransactions, + }; use crate::test_fixtures::expect_panic_with_message; use crate::tx::Eip1559TransactionRequest; use crate::withdraw::{ @@ -323,19 +324,20 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let mut rng = reproducible_rng(); let [withdrawal_request] = create_ck_withdrawal_requests(&mut rng); - let tx = create_transaction( - &withdrawal_request.clone(), - TransactionNonce::ZERO, - gas_fee_estimate(), - estimate_gas_limit(&withdrawal_request), - EthereumNetwork::Sepolia, - ) - .unwrap(); + let tx = withdrawal_request + .clone() + .to_transaction( + TransactionNonce::ZERO, + gas_fee_estimate(), + estimate_gas_limit(&withdrawal_request), + EthereumNetwork::Sepolia, + ) + .unwrap(); let burn_index = withdrawal_request.cketh_ledger_burn_index(); expect_panic_with_message( || transactions.record_created_transaction(burn_index, tx), - &format!("withdrawal request {burn_index} not found"), + &format!("request {burn_index} not found"), ); } @@ -343,15 +345,16 @@ mod withdrawal_transactions { fn should_fail_when_mismatch_with_cketh_withdrawal_request() { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let withdrawal_request = cketh_withdrawal_request_with_index(LedgerBurnIndex::new(15)); - transactions.record_withdrawal_request(withdrawal_request.clone()); - let correct_tx = create_transaction( - &withdrawal_request.clone().into(), - TransactionNonce::ZERO, - gas_fee_estimate(), - estimate_gas_limit(&withdrawal_request.clone().into()), - EthereumNetwork::Sepolia, - ) - .unwrap(); + transactions.record_request(withdrawal_request.clone()); + let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); + let correct_tx = pipeline_request + .to_transaction( + TransactionNonce::ZERO, + gas_fee_estimate(), + estimate_gas_limit(&withdrawal_request.clone().into()), + EthereumNetwork::Sepolia, + ) + .unwrap(); let tx_with_wrong_destination = Eip1559TransactionRequest { destination: Address::ZERO, @@ -394,15 +397,16 @@ mod withdrawal_transactions { LedgerBurnIndex::new(3), LedgerBurnIndex::new(7), ); - transactions.record_withdrawal_request(withdrawal_request.clone()); - let correct_tx = create_transaction( - &withdrawal_request.clone().into(), - TransactionNonce::ZERO, - gas_fee_estimate(), - estimate_gas_limit(&withdrawal_request.clone().into()), - EthereumNetwork::Sepolia, - ) - .unwrap(); + transactions.record_request(withdrawal_request.clone()); + let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); + let correct_tx = pipeline_request + .to_transaction( + TransactionNonce::ZERO, + gas_fee_estimate(), + estimate_gas_limit(&withdrawal_request.clone().into()), + EthereumNetwork::Sepolia, + ) + .unwrap(); let tx_mixing_payee_address_with_erc20_address = Eip1559TransactionRequest { destination: withdrawal_request.destination, ..correct_tx.clone() @@ -443,8 +447,7 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(current_nonce); let mut rng = reproducible_rng(); let [withdrawal_request] = create_and_record_ck_withdrawal_requests(&mut transactions, &mut rng); - let tx_with_wrong_nonce = create_transaction( - &withdrawal_request.clone(), + let tx_with_wrong_nonce = withdrawal_request.clone().to_transaction( wrong_nonce, gas_fee_estimate(), CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, @@ -466,7 +469,7 @@ mod withdrawal_transactions { for i in 0..100_u64 { let ledger_burn_index = LedgerBurnIndex::new(15 + i); let withdrawal_request = cketh_withdrawal_request_with_index(ledger_burn_index); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let expected_tx_amount = withdrawal_request .withdrawal_amount .checked_sub( @@ -515,7 +518,7 @@ mod withdrawal_transactions { cketh_ledger_burn_index, ckerc20_ledger_burn_index, ); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -601,7 +604,7 @@ mod withdrawal_transactions { gas_fee_estimate(), ); - assert_eq!(transactions.withdrawal_requests_batch(1), vec![]); + assert_eq!(transactions.requests_batch(1), vec![]); } } @@ -867,7 +870,7 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let withdrawal_request = withdrawal_request.into(); let cketh_ledger_burn_index = withdrawal_request.cketh_ledger_burn_index(); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let initial_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -1027,7 +1030,7 @@ mod withdrawal_transactions { assert_eq!( resubmitted_txs, vec![Err(ResubmitTransactionError::InsufficientTransactionFee { - ledger_burn_index: 93_u64.into(), + id: 93_u64.into(), transaction_nonce: 30_u8.into(), allowed_max_transaction_fee: DEFAULT_MAX_TRANSACTION_FEE.into(), max_transaction_fee: 30_000_000_000_165_000_u128.into(), @@ -1091,7 +1094,7 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let withdrawal_request = withdrawal_request.into(); let cketh_ledger_burn_index = withdrawal_request.cketh_ledger_burn_index(); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request, @@ -1206,7 +1209,7 @@ mod withdrawal_transactions { }; let withdrawal_request = withdrawal_request.into(); let cketh_ledger_burn_index = withdrawal_request.cketh_ledger_burn_index(); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -1555,7 +1558,7 @@ mod withdrawal_transactions { let cketh_ledger_burn_index = LedgerBurnIndex::new(15); let withdrawal_request: WithdrawalRequest = cketh_withdrawal_request_with_index(cketh_ledger_burn_index).into(); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -1585,7 +1588,7 @@ mod withdrawal_transactions { cketh_ledger_burn_index, ckerc20_ledger_burn_index, ); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -1619,7 +1622,7 @@ mod withdrawal_transactions { ckerc20_ledger_burn_index, ) }; - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -1653,7 +1656,7 @@ mod withdrawal_transactions { cketh_ledger_burn_index, ckerc20_ledger_burn_index, ); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let created_tx = create_and_record_transaction( &mut transactions, withdrawal_request.clone(), @@ -1696,7 +1699,7 @@ mod withdrawal_transactions { { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let withdrawal_request = cketh_withdrawal_request_with_index(LedgerBurnIndex::new(15)); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); let cketh_ledger_burn_index = withdrawal_request.ledger_burn_index; let created_tx = create_and_record_transaction( &mut transactions, @@ -2083,7 +2086,7 @@ mod withdrawal_transactions { RetrieveEthStatus::NotFound ); assert_withdrawal_status(transactions, &withdrawal_request.clone(), vec![]); - transactions.record_withdrawal_request(withdrawal_request.clone()); + transactions.record_request(withdrawal_request.clone()); assert_eq!( transactions.transaction_status(&cketh_ledger_burn_index), RetrieveEthStatus::Pending @@ -2142,8 +2145,8 @@ mod withdrawal_transactions { create_and_record_signed_transaction, }; use crate::state::transactions::{ - CreateTransactionError, EthWithdrawalRequest, NotReimbursable, ReimbursementIndex, - create_transaction, + CreateTransactionError, EthWithdrawalRequest, NotReimbursable, PipelineRequest, + ReimbursementIndex, }; use crate::tx::GasFeeEstimate; use crate::withdraw::CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT; @@ -2184,7 +2187,7 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let funding = sweeper_funding_request(); - transactions.record_withdrawal_request(funding.clone()); + transactions.record_request(funding.clone()); let created_tx = create_and_record_transaction( &mut transactions, funding.clone(), @@ -2243,14 +2246,14 @@ mod withdrawal_transactions { .to_price(CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT) .max_transaction_fee(); - let tx = create_transaction( - &WithdrawalRequest::SweeperFunding(funding.clone()), - TransactionNonce::ZERO, - gas_fee, - CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, - EthereumNetwork::Mainnet, - ) - .expect("the funded amount must cover the fee"); + let tx = WithdrawalRequest::SweeperFunding(funding.clone()) + .to_transaction( + TransactionNonce::ZERO, + gas_fee, + CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, + EthereumNetwork::Mainnet, + ) + .expect("the funded amount must cover the fee"); assert_eq!(tx.destination, funding.destination); assert_eq!( @@ -2273,8 +2276,7 @@ mod withdrawal_transactions { let expected_index = funding.ledger_burn_index; assert_matches!( - create_transaction( - &WithdrawalRequest::SweeperFunding(funding), + WithdrawalRequest::SweeperFunding(funding).to_transaction( TransactionNonce::ZERO, gas_fee_estimate(), CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, @@ -2294,7 +2296,7 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); let funding = sweeper_funding_payload(); let request = WithdrawalRequest::SweeperFunding(funding.clone()); - transactions.record_withdrawal_request(request.clone()); + transactions.record_request(request.clone()); let created_tx = create_and_record_transaction(&mut transactions, request, gas_fee_estimate()); create_and_record_signed_transaction(&mut transactions, created_tx); @@ -2329,14 +2331,14 @@ mod withdrawal_transactions { } } -mod oldest_incomplete_withdrawal_timestamp { +mod oldest_incomplete_request_timestamp { use super::*; use ic_crypto_test_utils_reproducible_rng::reproducible_rng; #[test] fn should_return_none_when_no_requests() { let transactions = WithdrawalTransactions::new(TransactionNonce::ZERO); - assert_eq!(None, transactions.oldest_incomplete_withdrawal_timestamp()); + assert_eq!(None, transactions.oldest_incomplete_request_timestamp()); } #[test] @@ -2347,7 +2349,7 @@ mod oldest_incomplete_withdrawal_timestamp { create_and_record_ck_withdrawal_requests(&mut transactions, &mut rng); assert_eq!( - transactions.oldest_incomplete_withdrawal_timestamp(), + transactions.oldest_incomplete_request_timestamp(), withdrawal_request.created_at(), ); } @@ -2359,13 +2361,10 @@ mod oldest_incomplete_withdrawal_timestamp { let [mut first_request, mut second_request] = create_ck_withdrawal_requests(&mut rng); set_created_at(&mut first_request, 10); set_created_at(&mut second_request, 20); - transactions.record_withdrawal_request(first_request); - transactions.record_withdrawal_request(second_request); + transactions.record_request(first_request); + transactions.record_request(second_request); - assert_eq!( - transactions.oldest_incomplete_withdrawal_timestamp(), - Some(10), - ); + assert_eq!(transactions.oldest_incomplete_request_timestamp(), Some(10),); } #[test] @@ -2381,7 +2380,7 @@ mod oldest_incomplete_withdrawal_timestamp { ); assert_eq!( - transactions.oldest_incomplete_withdrawal_timestamp(), + transactions.oldest_incomplete_request_timestamp(), withdrawal_request.created_at(), ); } @@ -2394,14 +2393,11 @@ mod oldest_incomplete_withdrawal_timestamp { set_created_at(&mut first_request, 10); set_created_at(&mut second_request, 20); - transactions.record_withdrawal_request(first_request.clone()); - transactions.record_withdrawal_request(second_request.clone()); + transactions.record_request(first_request.clone()); + transactions.record_request(second_request.clone()); create_and_record_transaction(&mut transactions, first_request, gas_fee_estimate()); - assert_eq!( - transactions.oldest_incomplete_withdrawal_timestamp(), - Some(10), - ); + assert_eq!(transactions.oldest_incomplete_request_timestamp(), Some(10),); } #[test] @@ -2422,7 +2418,7 @@ mod oldest_incomplete_withdrawal_timestamp { transaction_receipt(&signed_tx, TransactionStatus::Success), ); - assert_eq!(transactions.oldest_incomplete_withdrawal_timestamp(), None); + assert_eq!(transactions.oldest_incomplete_request_timestamp(), None); } fn set_created_at(withdrawal_request: &mut WithdrawalRequest, created_at: u64) { @@ -2473,8 +2469,8 @@ mod create_transaction { gas_fee_estimate, }; use crate::state::transactions::{ - CreateTransactionError, Erc20WithdrawalRequest, EthWithdrawalRequest, TransactionCallData, - create_transaction, + CreateTransactionError, Erc20WithdrawalRequest, EthWithdrawalRequest, PipelineRequest, + TransactionCallData, WithdrawalRequest, }; use crate::tx::GasFeeEstimate; use crate::tx::{AccessList, Eip1559TransactionRequest}; @@ -2497,8 +2493,8 @@ mod create_transaction { withdrawal_amount: insufficient_amount, ..cketh_withdrawal_request_with_index(cketh_ledger_burn_index) }; - let result = create_transaction( - &cketh_withdrawal_request.clone().into(), + let pipeline_request: WithdrawalRequest = cketh_withdrawal_request.clone().into(); + let result = pipeline_request.to_transaction( TransactionNonce::TWO, gas_fee.clone(), gas_limit, @@ -2519,8 +2515,8 @@ mod create_transaction { max_transaction_fee: insufficient_amount, ..ckerc20_withdrawal_request_with_index(cketh_ledger_burn_index, LedgerBurnIndex::new(2)) }; - let result = create_transaction( - &ckerc20_withdrawal_request.clone().into(), + let pipeline_request: WithdrawalRequest = ckerc20_withdrawal_request.clone().into(); + let result = pipeline_request.to_transaction( TransactionNonce::TWO, gas_fee, gas_limit, @@ -2555,8 +2551,8 @@ mod create_transaction { Wei::from(31_500_001_050_000_u64) ); - let result = create_transaction( - &withdrawal_request.clone().into(), + let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); + let result = pipeline_request.to_transaction( TransactionNonce::TWO, gas_fee, gas_limit, @@ -2609,8 +2605,8 @@ mod create_transaction { ) }; - let result = create_transaction( - &withdrawal_request.clone().into(), + let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); + let result = pipeline_request.to_transaction( TransactionNonce::from(0x57_u32), gas_fee.clone(), gas_limit, @@ -2656,7 +2652,7 @@ mod withdrawal_flow { use super::arbitrary::{arb_checked_amount_of, arb_gas_fee_estimate, arb_withdrawal_request}; use crate::numeric::TransactionNonce; use crate::state::transactions::tests::sign_transaction; - use crate::state::transactions::{EthereumNetwork, WithdrawalTransactions, create_transaction}; + use crate::state::transactions::{EthereumNetwork, PipelineRequest, WithdrawalTransactions}; use crate::withdraw::estimate_gas_limit; use proptest::proptest; use std::cell::RefCell; @@ -2668,7 +2664,7 @@ mod withdrawal_flow { let wrapped_txs = RefCell::new(transactions); proptest!(|(request in arb_withdrawal_request())| { - wrapped_txs.borrow_mut().record_withdrawal_request(request) + wrapped_txs.borrow_mut().record_request(request) }); proptest!(|(gas_fee_estimate in arb_gas_fee_estimate(), transaction_count in arb_checked_amount_of())| { @@ -2677,11 +2673,10 @@ mod withdrawal_flow { wrapped_txs.borrow_mut().record_resubmit_transaction(resubmit_tx); } - let withdrawal_requests = wrapped_txs.borrow().withdrawal_requests_batch(5); + let withdrawal_requests = wrapped_txs.borrow().requests_batch(5); for request in withdrawal_requests { let nonce = wrapped_txs.borrow().next_transaction_nonce(); - if let Ok(created_tx) = create_transaction( - &request, + if let Ok(created_tx) = request.to_transaction( nonce, gas_fee_estimate.clone(), estimate_gas_limit(&request), @@ -2977,7 +2972,7 @@ fn create_and_record_ck_withdrawal_requests( ) -> [WithdrawalRequest; N] { let requests = create_ck_withdrawal_requests(rng); for request in &requests { - transactions.record_withdrawal_request(request.clone()); + transactions.record_request(request.clone()); } requests } @@ -2988,7 +2983,7 @@ fn create_and_record_cketh_withdrawal_requests( ) -> [WithdrawalRequest; N] { let requests = create_cketh_withdrawal_requests(); for request in &requests { - transactions.record_withdrawal_request(request.clone()); + transactions.record_request(request.clone()); } requests } @@ -2999,7 +2994,7 @@ fn create_and_record_ckerc20_withdrawal_requests( ) -> [WithdrawalRequest; N] { let requests = create_ckerc20_withdrawal_requests(); for request in &requests { - transactions.record_withdrawal_request(request.clone()); + transactions.record_request(request.clone()); } requests } @@ -3059,14 +3054,14 @@ fn create_and_record_transaction>( gas_fee_estimate: GasFeeEstimate, ) -> Eip1559TransactionRequest { let withdrawal_request = withdrawal_request.into(); - let tx = create_transaction( - &withdrawal_request, - transactions.next_transaction_nonce(), - gas_fee_estimate, - estimate_gas_limit(&withdrawal_request), - EthereumNetwork::Sepolia, - ) - .expect("failed to create transaction"); + let tx = withdrawal_request + .to_transaction( + transactions.next_transaction_nonce(), + gas_fee_estimate, + estimate_gas_limit(&withdrawal_request), + EthereumNetwork::Sepolia, + ) + .expect("failed to create transaction"); let burn_index = withdrawal_request.cketh_ledger_burn_index(); transactions.record_created_transaction(burn_index, tx.clone()); tx @@ -3173,8 +3168,8 @@ use std::collections::{BTreeMap, BTreeSet, VecDeque}; /// that field alone changes. #[derive(Clone)] pub(in crate::state) struct WithdrawalTransactionsBuilder { - pending_withdrawal_requests: VecDeque, - processed_withdrawal_requests: BTreeMap, + pending_requests: VecDeque, + processed_requests: BTreeMap, created_tx: MultiKeyMap, sent_tx: MultiKeyMap>, finalized_tx: MultiKeyMap, @@ -3187,8 +3182,8 @@ pub(in crate::state) struct WithdrawalTransactionsBuilder { impl Default for WithdrawalTransactionsBuilder { fn default() -> Self { Self { - pending_withdrawal_requests: Default::default(), - processed_withdrawal_requests: Default::default(), + pending_requests: Default::default(), + processed_requests: Default::default(), created_tx: Default::default(), sent_tx: Default::default(), finalized_tx: Default::default(), @@ -3203,17 +3198,17 @@ impl Default for WithdrawalTransactionsBuilder { impl WithdrawalTransactionsBuilder { pub(in crate::state) fn with_pending_withdrawal_requests( mut self, - pending_withdrawal_requests: VecDeque, + pending_requests: VecDeque, ) -> Self { - self.pending_withdrawal_requests = pending_withdrawal_requests; + self.pending_requests = pending_requests; self } pub(in crate::state) fn with_processed_withdrawal_requests( mut self, - processed_withdrawal_requests: BTreeMap, + processed_requests: BTreeMap, ) -> Self { - self.processed_withdrawal_requests = processed_withdrawal_requests; + self.processed_requests = processed_requests; self } @@ -3272,9 +3267,9 @@ impl WithdrawalTransactionsBuilder { pub(in crate::state) fn build(self) -> WithdrawalTransactions { WithdrawalTransactions { - pipeline: TransactionPipeline { - pending_withdrawal_requests: self.pending_withdrawal_requests, - processed_withdrawal_requests: self.processed_withdrawal_requests, + pipeline: MinterTransactionPipeline { + pending_requests: self.pending_requests, + processed_requests: self.processed_requests, created_tx: self.created_tx, sent_tx: self.sent_tx, finalized_tx: self.finalized_tx, diff --git a/rs/ethereum/cketh/minter/src/withdraw.rs b/rs/ethereum/cketh/minter/src/withdraw.rs index a0a73e3983c6..ec4c4921857d 100644 --- a/rs/ethereum/cketh/minter/src/withdraw.rs +++ b/rs/ethereum/cketh/minter/src/withdraw.rs @@ -13,8 +13,8 @@ use crate::{ audit::{EventType, process_event}, minter_address, mutate_state, read_state, transactions::{ - CreateTransactionError, Reimbursed, ReimbursementIndex, ReimbursementRequest, - WithdrawalRequest, create_transaction, + CreateTransactionError, PipelineRequest, Reimbursed, ReimbursementIndex, + ReimbursementRequest, WithdrawalRequest, }, }, tx::{GasFeeEstimate, lazy_refresh_gas_fee_estimate}, @@ -251,19 +251,13 @@ async fn resubmit_transactions_batch( fn create_transactions_batch(gas_fee_estimate: GasFeeEstimate) { for request in read_state(|s| { s.withdrawal_transactions - .withdrawal_requests_batch(WITHDRAWAL_REQUESTS_BATCH_SIZE) + .requests_batch(WITHDRAWAL_REQUESTS_BATCH_SIZE) }) { log!(DEBUG, "[create_transactions_batch]: processing {request:?}",); let ethereum_network = read_state(State::ethereum_network); let nonce = read_state(|s| s.withdrawal_transactions.next_transaction_nonce()); let gas_limit = estimate_gas_limit(&request); - match create_transaction( - &request, - nonce, - gas_fee_estimate.clone(), - gas_limit, - ethereum_network, - ) { + match request.to_transaction(nonce, gas_fee_estimate.clone(), gas_limit, ethereum_network) { Ok(transaction) => { log!( DEBUG, @@ -289,10 +283,7 @@ fn create_transactions_batch(gas_fee_estimate: GasFeeEstimate) { INFO, "[create_transactions_batch]: Withdrawal request with burn index {ledger_burn_index} has insufficient amount {withdrawal_amount:?} to cover transaction fees: {max_transaction_fee:?}. Request moved back to end of queue." ); - mutate_state(|s| { - s.withdrawal_transactions - .reschedule_withdrawal_request(request) - }); + mutate_state(|s| s.withdrawal_transactions.reschedule_request(request)); } }; } From b3b64576914adb1691f517357730b595f4d01aa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Thu, 20 Aug 2026 08:43:27 +0000 Subject: [PATCH 02/16] refactor(cketh): keep TransactionStage as visible as its pipeline #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) --- rs/ethereum/cketh/minter/src/state/transactions/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 3569dd6fdc6e..c4846328c94a 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -588,7 +588,7 @@ pub enum ResubmitTransactionError { /// How far a transaction has got through the pipeline. Carries the transaction itself, since /// every caller that asks the stage also wants the transaction at it. #[derive(Clone, Eq, PartialEq, Debug)] -pub(in crate::state) enum TransactionStage<'a> { +pub enum TransactionStage<'a> { Created(&'a Eip1559TransactionRequest), /// The most recently sent transaction, i.e. the one with the highest fee. Sent(&'a SignedTransactionRequest), From 51e3b14c49ff7195fc6f582c013810cd7abecca9 Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Wed, 19 Aug 2026 11:24:44 +0200 Subject: [PATCH 03/16] refactor(cketh): let each request type name its own creation error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../cketh/minter/src/dashboard/tests.rs | 2 +- rs/ethereum/cketh/minter/src/state/tests.rs | 2 +- .../minter/src/state/transactions/mod.rs | 25 +++++++++++-------- .../minter/src/state/transactions/tests.rs | 24 +++++++++--------- rs/ethereum/cketh/minter/src/withdraw.rs | 7 +++++- 5 files changed, 35 insertions(+), 25 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/dashboard/tests.rs b/rs/ethereum/cketh/minter/src/dashboard/tests.rs index e0414aa8df64..e23fee59e3c2 100644 --- a/rs/ethereum/cketh/minter/src/dashboard/tests.rs +++ b/rs/ethereum/cketh/minter/src/dashboard/tests.rs @@ -1489,7 +1489,7 @@ fn ckerc20_withdrawal_flow( }; let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); let transaction = pipeline_request - .to_transaction( + .create_transaction( nonce, gas_fee, GasAmount::from(65_000_u32), diff --git a/rs/ethereum/cketh/minter/src/state/tests.rs b/rs/ethereum/cketh/minter/src/state/tests.rs index 574c0d52c284..35c14bd31400 100644 --- a/rs/ethereum/cketh/minter/src/state/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/tests.rs @@ -2027,7 +2027,7 @@ mod eth_balance { let transaction = self .withdrawal_request - .to_transaction( + .create_transaction( self.nonce, self.tx_fee, self.gas_limit, diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index c4846328c94a..448ec1e783d2 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -373,6 +373,11 @@ pub trait PipelineRequest: Clone + Eq + fmt::Debug { /// The pipeline's alternate map key — a ckETH `LedgerBurnIndex` for withdrawals. type Id: Copy + Ord + fmt::Debug + fmt::Display; + /// Why [`Self::create_transaction`] could not build a transaction. A request that always funds + /// its own fee can set this to [`std::convert::Infallible`], making the failure unrepresentable + /// rather than merely unreachable. + type Error; + /// The identity of this request, used as the pipeline's alternate map key. fn id(&self) -> Self::Id; @@ -388,18 +393,22 @@ pub trait PipelineRequest: Clone + Eq + fmt::Debug { /// Assert that a freshly created transaction is consistent with the request (amount). fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest); - /// Build the EIP-1559 transaction that fulfils this request. - fn to_transaction( + /// Creates the EIP-1559 transaction that fulfils this request. + /// + /// # Errors + /// * [`Self::Error`] if the request cannot cover the transaction fee. + fn create_transaction( &self, nonce: TransactionNonce, gas_fee_estimate: GasFeeEstimate, gas_limit: GasAmount, ethereum_network: EthereumNetwork, - ) -> Result; + ) -> Result; } impl PipelineRequest for WithdrawalRequest { type Id = LedgerBurnIndex; + type Error = CreateTransactionError; fn id(&self) -> LedgerBurnIndex { self.cketh_ledger_burn_index() @@ -444,7 +453,9 @@ impl PipelineRequest for WithdrawalRequest { } } - fn to_transaction( + /// The transaction fees are paid by the beneficiary, meaning that the fees will be deducted + /// from the withdrawal amount. + fn create_transaction( &self, nonce: TransactionNonce, gas_fee_estimate: GasFeeEstimate, @@ -1494,12 +1505,6 @@ impl WithdrawalTransactions { } } -/// Creates an EIP-1559 transaction for the given pipeline request. -/// The transaction fees are paid by the beneficiary, -/// meaning that the fees will be deducted from the withdrawal amount. -/// -/// # Errors -/// * `CreateTransactionError::InsufficientTransactionFee` if the ETH withdrawal amount does not cover the transaction fee. // First 4 bytes of keccak256(transfer(address,uint256)) const ERC_20_TRANSFER_FUNCTION_SELECTOR: [u8; 4] = hex_literal::hex!("a9059cbb"); diff --git a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs index 8e9e54fef1d0..ace07b3e055d 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs @@ -326,7 +326,7 @@ mod withdrawal_transactions { let [withdrawal_request] = create_ck_withdrawal_requests(&mut rng); let tx = withdrawal_request .clone() - .to_transaction( + .create_transaction( TransactionNonce::ZERO, gas_fee_estimate(), estimate_gas_limit(&withdrawal_request), @@ -348,7 +348,7 @@ mod withdrawal_transactions { transactions.record_request(withdrawal_request.clone()); let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); let correct_tx = pipeline_request - .to_transaction( + .create_transaction( TransactionNonce::ZERO, gas_fee_estimate(), estimate_gas_limit(&withdrawal_request.clone().into()), @@ -400,7 +400,7 @@ mod withdrawal_transactions { transactions.record_request(withdrawal_request.clone()); let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); let correct_tx = pipeline_request - .to_transaction( + .create_transaction( TransactionNonce::ZERO, gas_fee_estimate(), estimate_gas_limit(&withdrawal_request.clone().into()), @@ -447,7 +447,7 @@ mod withdrawal_transactions { let mut transactions = WithdrawalTransactions::new(current_nonce); let mut rng = reproducible_rng(); let [withdrawal_request] = create_and_record_ck_withdrawal_requests(&mut transactions, &mut rng); - let tx_with_wrong_nonce = withdrawal_request.clone().to_transaction( + let tx_with_wrong_nonce = withdrawal_request.clone().create_transaction( wrong_nonce, gas_fee_estimate(), CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, @@ -2247,7 +2247,7 @@ mod withdrawal_transactions { .max_transaction_fee(); let tx = WithdrawalRequest::SweeperFunding(funding.clone()) - .to_transaction( + .create_transaction( TransactionNonce::ZERO, gas_fee, CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, @@ -2276,7 +2276,7 @@ mod withdrawal_transactions { let expected_index = funding.ledger_burn_index; assert_matches!( - WithdrawalRequest::SweeperFunding(funding).to_transaction( + WithdrawalRequest::SweeperFunding(funding).create_transaction( TransactionNonce::ZERO, gas_fee_estimate(), CKETH_WITHDRAWAL_TRANSACTION_GAS_LIMIT, @@ -2494,7 +2494,7 @@ mod create_transaction { ..cketh_withdrawal_request_with_index(cketh_ledger_burn_index) }; let pipeline_request: WithdrawalRequest = cketh_withdrawal_request.clone().into(); - let result = pipeline_request.to_transaction( + let result = pipeline_request.create_transaction( TransactionNonce::TWO, gas_fee.clone(), gas_limit, @@ -2516,7 +2516,7 @@ mod create_transaction { ..ckerc20_withdrawal_request_with_index(cketh_ledger_burn_index, LedgerBurnIndex::new(2)) }; let pipeline_request: WithdrawalRequest = ckerc20_withdrawal_request.clone().into(); - let result = pipeline_request.to_transaction( + let result = pipeline_request.create_transaction( TransactionNonce::TWO, gas_fee, gas_limit, @@ -2552,7 +2552,7 @@ mod create_transaction { ); let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); - let result = pipeline_request.to_transaction( + let result = pipeline_request.create_transaction( TransactionNonce::TWO, gas_fee, gas_limit, @@ -2606,7 +2606,7 @@ mod create_transaction { }; let pipeline_request: WithdrawalRequest = withdrawal_request.clone().into(); - let result = pipeline_request.to_transaction( + let result = pipeline_request.create_transaction( TransactionNonce::from(0x57_u32), gas_fee.clone(), gas_limit, @@ -2676,7 +2676,7 @@ mod withdrawal_flow { let withdrawal_requests = wrapped_txs.borrow().requests_batch(5); for request in withdrawal_requests { let nonce = wrapped_txs.borrow().next_transaction_nonce(); - if let Ok(created_tx) = request.to_transaction( + if let Ok(created_tx) = request.create_transaction( nonce, gas_fee_estimate.clone(), estimate_gas_limit(&request), @@ -3055,7 +3055,7 @@ fn create_and_record_transaction>( ) -> Eip1559TransactionRequest { let withdrawal_request = withdrawal_request.into(); let tx = withdrawal_request - .to_transaction( + .create_transaction( transactions.next_transaction_nonce(), gas_fee_estimate, estimate_gas_limit(&withdrawal_request), diff --git a/rs/ethereum/cketh/minter/src/withdraw.rs b/rs/ethereum/cketh/minter/src/withdraw.rs index ec4c4921857d..75c13f37db8e 100644 --- a/rs/ethereum/cketh/minter/src/withdraw.rs +++ b/rs/ethereum/cketh/minter/src/withdraw.rs @@ -257,7 +257,12 @@ fn create_transactions_batch(gas_fee_estimate: GasFeeEstimate) { let ethereum_network = read_state(State::ethereum_network); let nonce = read_state(|s| s.withdrawal_transactions.next_transaction_nonce()); let gas_limit = estimate_gas_limit(&request); - match request.to_transaction(nonce, gas_fee_estimate.clone(), gas_limit, ethereum_network) { + match request.create_transaction( + nonce, + gas_fee_estimate.clone(), + gas_limit, + ethereum_network, + ) { Ok(transaction) => { log!( DEBUG, From c99bc90ccc32f7157d3426d7730bacf0bad5dbcc Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 11:02:15 +0200 Subject: [PATCH 04/16] refactor(cketh): finish speaking the pipeline's vocabulary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 TransactionPipeline`, 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) --- rs/ethereum/cketh/minter/src/state/tests.rs | 35 +++-- .../minter/src/state/transactions/mod.rs | 124 ++++++++---------- .../minter/src/state/transactions/tests.rs | 4 +- 3 files changed, 76 insertions(+), 87 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/tests.rs b/rs/ethereum/cketh/minter/src/state/tests.rs index 35c14bd31400..260cf266eb70 100644 --- a/rs/ethereum/cketh/minter/src/state/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/tests.rs @@ -923,19 +923,6 @@ proptest! { } } -fn accepted_withdrawal_request_event( - request: crate::state::transactions::WithdrawalRequest, -) -> EventType { - use crate::state::transactions::WithdrawalRequest; - match request { - WithdrawalRequest::CkEth(request) => EventType::AcceptedEthWithdrawalRequest(request), - WithdrawalRequest::CkErc20(request) => EventType::AcceptedErc20WithdrawalRequest(request), - WithdrawalRequest::SweeperFunding(request) => { - EventType::AcceptedSweeperFundingRequest(request) - } - } -} - #[test] fn state_equivalence() { use crate::EVM_RPC_ID_PRODUCTION; @@ -1120,8 +1107,8 @@ fn state_equivalence() { }), }; let builder = WithdrawalTransactionsBuilder::default() - .with_pending_withdrawal_requests(pending_requests) - .with_processed_withdrawal_requests(processed_requests) + .with_pending_requests(pending_requests) + .with_processed_requests(processed_requests) .with_created_tx(created_tx) .with_sent_tx(sent_tx) .with_finalized_tx(finalized_tx) @@ -1346,7 +1333,7 @@ fn state_equivalence() { state.is_equivalent_to(&State { withdrawal_transactions: builder .clone() - .with_pending_withdrawal_requests( + .with_pending_requests( vec![ withdrawal_request2.clone().into(), withdrawal_request1.clone().into() @@ -1365,9 +1352,7 @@ fn state_equivalence() { state.is_equivalent_to(&State { withdrawal_transactions: builder .clone() - .with_pending_withdrawal_requests( - vec![withdrawal_request1.into()].into_iter().collect() - ) + .with_pending_requests(vec![withdrawal_request1.into()].into_iter().collect()) .build(), ..state.clone() }), @@ -2084,6 +2069,18 @@ mod eth_balance { state } + fn accepted_withdrawal_request_event(request: WithdrawalRequest) -> EventType { + match request { + WithdrawalRequest::CkEth(request) => EventType::AcceptedEthWithdrawalRequest(request), + WithdrawalRequest::CkErc20(request) => { + EventType::AcceptedErc20WithdrawalRequest(request) + } + WithdrawalRequest::SweeperFunding(request) => { + EventType::AcceptedSweeperFundingRequest(request) + } + } + } + fn add_erc20_token(state: &mut State) { use crate::state::CkErc20Token; apply_state_transition( diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 448ec1e783d2..8e9acc112f38 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -587,7 +587,7 @@ pub enum CreateTransactionError { } #[derive(Clone, Eq, PartialEq, Debug)] -pub enum ResubmitTransactionError { +pub enum ResubmitTransactionError { InsufficientTransactionFee { id: Id, transaction_nonce: TransactionNonce, @@ -632,13 +632,13 @@ impl TransactionPipeline { pub fn record_request>(&mut self, request: Req) { let request = request.into(); - let burn_index = request.id(); - if self.pending_requests.iter().any(|r| r.id() == burn_index) - || self.created_tx.contains_alt(&burn_index) - || self.sent_tx.contains_alt(&burn_index) - || self.finalized_tx.contains_alt(&burn_index) + let id = request.id(); + if self.pending_requests.iter().any(|r| r.id() == id) + || self.created_tx.contains_alt(&id) + || self.sent_tx.contains_alt(&id) + || self.finalized_tx.contains_alt(&id) { - panic!("BUG: duplicate transaction id {burn_index}"); + panic!("BUG: duplicate transaction id {id}"); } self.pending_requests.push_back(request); } @@ -713,18 +713,14 @@ impl TransactionPipeline { "BUG: mismatch between sent transaction and created transaction" ); let signed_tx = created_tx.clone_resubmission_strategy(signed_transaction); - let (nonce, ledger_burn_index, _created_tx) = self + let (nonce, id, _created_tx) = self .created_tx .remove_entry(&signed_tx.as_ref().nonce()) .expect("BUG: missing created transaction"); if let Some(sent_tx) = self.sent_tx.get_mut(&nonce) { sent_tx.push(signed_tx); } else { - assert_eq!( - self.sent_tx - .try_insert(nonce, ledger_burn_index, vec![signed_tx]), - Ok(()) - ); + assert_eq!(self.sent_tx.try_insert(nonce, id, vec![signed_tx]), Ok(())); } } @@ -747,15 +743,15 @@ impl TransactionPipeline { // The nonce of the first pending transaction is then exactly c. let first_pending_tx_nonce: TransactionNonce = latest_transaction_count.change_units(); let mut transactions_to_resubmit = Vec::new(); - for (nonce, burn_index, signed_tx) in self + for (nonce, id, signed_tx) in self .sent_tx .iter() - .filter(|(nonce, _burn_index, _signed_tx)| *nonce >= &first_pending_tx_nonce) + .filter(|(nonce, _id, _signed_tx)| *nonce >= &first_pending_tx_nonce) { let last_signed_tx = signed_tx.last().expect("BUG: empty sent transactions list"); match last_signed_tx.resubmit(current_gas_fee.clone()) { Ok(Some(new_tx)) => { - transactions_to_resubmit.push(Ok((*burn_index, new_tx))); + transactions_to_resubmit.push(Ok((*id, new_tx))); } Ok(None) => { // the transaction fee is still up-to-date but because the transaction did not get included, @@ -768,7 +764,7 @@ impl TransactionPipeline { }) => { transactions_to_resubmit.push(Err( ResubmitTransactionError::InsufficientTransactionFee { - id: *burn_index, + id: *id, transaction_nonce: *nonce, allowed_max_transaction_fee, max_transaction_fee: actual_max_transaction_fee, @@ -783,19 +779,14 @@ impl TransactionPipeline { pub fn record_resubmit_transaction(&mut self, new_tx: Eip1559TransactionRequest) { let nonce = new_tx.nonce; - let (ledger_burn_index, last_sent_tx) = - Self::expect_last_sent_tx_entry(&self.sent_tx, &nonce); + let (id, last_sent_tx) = Self::expect_last_sent_tx_entry(&self.sent_tx, &nonce); assert!( equal_ignoring_fee_and_amount(last_sent_tx.as_ref().transaction(), &new_tx), "BUG: mismatch between last sent transaction {last_sent_tx:?} and the transaction to resubmit {new_tx:?}" ); Self::cleanup_failed_resubmitted_transactions(&mut self.created_tx, &nonce); let new_tx = last_sent_tx.clone_resubmission_strategy(new_tx); - assert_eq!( - self.created_tx - .try_insert(nonce, *ledger_burn_index, new_tx), - Ok(()) - ); + assert_eq!(self.created_tx.try_insert(nonce, *id, new_tx), Ok(())); } pub fn sent_transactions_to_finalize( @@ -808,7 +799,7 @@ impl TransactionPipeline { for (_nonce, index, sent_txs) in self .sent_tx .iter() - .filter(|(nonce, _burn_index, _signed_txs)| *nonce < &first_non_finalized_tx_nonce) + .filter(|(nonce, _id, _signed_txs)| *nonce < &first_non_finalized_tx_nonce) { for sent_tx in sent_txs { if let Some(prev_index) = transactions.insert(sent_tx.as_ref().hash(), *index) { @@ -860,7 +851,7 @@ impl TransactionPipeline { pub fn requests_batch(&self, requested_batch_size: usize) -> Vec { // The number of pending transaction nonces is counted and not the number of pending transactions // because a nonce may be associated with several distinct transactions (due to re-submission and dynamic fees). - // However, once a nonce is chosen for a withdrawal request, it's in our interest that the corresponding transaction be finalized asap. + // However, once a nonce is chosen for a request, it's in our interest that the corresponding transaction be finalized asap. // Limiting the number of transactions would be counter-productive. const MAX_NUM_PENDING_TRANSACTION_NONCES: usize = 1000; let unique_pending_transaction_nonces: BTreeSet<_> = @@ -889,7 +880,7 @@ impl TransactionPipeline { ) -> impl Iterator { self.created_tx .iter() - .map(|(nonce, ledger_burn_index, tx)| (nonce, ledger_burn_index, tx.as_ref())) + .map(|(nonce, id, tx)| (nonce, id, tx.as_ref())) } pub fn transactions_to_sign_batch( @@ -898,7 +889,7 @@ impl TransactionPipeline { ) -> Vec<(R::Id, Eip1559TransactionRequest)> { self.transactions_to_sign_iter() .take(batch_size) - .map(|(_nonce, withdrawal_id, tx)| (*withdrawal_id, tx.clone())) + .map(|(_nonce, id, tx)| (*id, tx.clone())) .collect() } @@ -910,10 +901,10 @@ impl TransactionPipeline { let first_pending_tx_nonce: TransactionNonce = latest_transaction_count.change_units(); self.sent_tx .iter() - .filter_map(move |(nonce, ledger_burn_index, txs)| { + .filter_map(move |(nonce, id, txs)| { txs.last() - .map(|tx| (nonce, ledger_burn_index, tx)) - .filter(|(nonce, _ledger_burn_index, _tx)| *nonce >= &first_pending_tx_nonce) + .map(|tx| (nonce, id, tx)) + .filter(|(nonce, _id, _tx)| *nonce >= &first_pending_tx_nonce) }) .take(batch_size) .map(|(_nonce, _index, tx)| tx.as_ref()) @@ -935,11 +926,8 @@ impl TransactionPipeline { .map(|(nonce, index, txs)| (nonce, index, txs.iter().map(|tx| tx.as_ref()).collect())) } - pub fn get_finalized_transaction( - &self, - burn_index: &R::Id, - ) -> Option<&FinalizedEip1559Transaction> { - self.finalized_tx.get_alt(burn_index) + pub fn get_finalized_transaction(&self, id: &R::Id) -> Option<&FinalizedEip1559Transaction> { + self.finalized_tx.get_alt(id) } pub fn processed_requests_iter(&self) -> impl Iterator { @@ -960,8 +948,8 @@ impl TransactionPipeline { .map(TransactionStage::Finalized) } - pub fn get_processed_request(&self, burn_index: &R::Id) -> Option<&R> { - self.processed_requests.get(burn_index) + pub fn get_processed_request(&self, id: &R::Id) -> Option<&R> { + self.processed_requests.get(id) } pub fn finalized_transactions_iter( @@ -986,11 +974,11 @@ impl TransactionPipeline { sent_tx: &'a MultiKeyMap>, nonce: &TransactionNonce, ) -> (&'a R::Id, &'a SignedTransactionRequest) { - let (ledger_burn_index, sent_txs) = sent_tx + let (id, sent_txs) = sent_tx .get_entry(nonce) .expect("BUG: sent transaction not found"); let last_sent_tx = sent_txs.last().expect("BUG: empty sent transactions list"); - (ledger_burn_index, last_sent_tx) + (id, last_sent_tx) } fn cleanup_failed_resubmitted_transactions( @@ -1208,6 +1196,33 @@ impl WithdrawalTransactions { }) } + /// Arm the reimbursement for a withdrawal whose transaction failed on chain. + /// + /// # Panics + /// If the withdrawal is still armed for reimbursement, or was already reimbursed — either + /// would let the same burn be minted back twice. + pub fn record_reimbursement_request( + &mut self, + index: ReimbursementIndex, + request: ReimbursementRequest, + ) { + assert_eq!( + self.maybe_reimburse.get(&index.withdrawal_id()), + None, + "BUG: withdrawal request still in maybe_reimburse could lead to double minting!" + ); + assert_eq!( + self.reimbursed.get(&index), + None, + "BUG: reimbursement request was already processed" + ); + assert_eq!( + self.reimbursement_requests.insert(index.clone(), request), + None, + "BUG: reimbursement request for withdrawal {index:?} already exists" + ); + } + /// Quarantine the reimbursement request identified by its index to prevent double minting. /// WARNING!: It's crucial that this method does not panic, /// since it's called inside the clean-up callback, when an unexpected panic did occur before. @@ -1372,33 +1387,10 @@ impl WithdrawalTransactions { } } -/// Main-address pipeline behavior that is specific to ckETH/ckERC20 withdrawals: reimbursing a failed -/// transaction and answering the withdrawal-status endpoints. The sweeper pipeline has none of this. +/// The withdrawal-status queries backing the minter's `retrieve_eth_status` and +/// `withdrawal_status` endpoints. They read the pipeline and the reimbursement records together, +/// which is why they live on the wrapper; the sweeper pipeline answers no such endpoint. impl WithdrawalTransactions { - /// Finalize the transaction for `ledger_burn_index` matching `receipt`, then — if it failed on - /// chain — record the corresponding ckETH/ckERC20 reimbursement. - pub fn record_reimbursement_request( - &mut self, - index: ReimbursementIndex, - request: ReimbursementRequest, - ) { - assert_eq!( - self.maybe_reimburse.get(&index.withdrawal_id()), - None, - "BUG: withdrawal request still in maybe_reimburse could lead to double minting!" - ); - assert_eq!( - self.reimbursed.get(&index), - None, - "BUG: reimbursement request was already processed" - ); - assert_eq!( - self.reimbursement_requests.insert(index.clone(), request), - None, - "BUG: reimbursement request for withdrawal {index:?} already exists" - ); - } - pub fn withdrawal_status( &self, parameter: &WithdrawalSearchParameter, diff --git a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs index ace07b3e055d..e6d9a53b232b 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs @@ -3196,7 +3196,7 @@ impl Default for WithdrawalTransactionsBuilder { } impl WithdrawalTransactionsBuilder { - pub(in crate::state) fn with_pending_withdrawal_requests( + pub(in crate::state) fn with_pending_requests( mut self, pending_requests: VecDeque, ) -> Self { @@ -3204,7 +3204,7 @@ impl WithdrawalTransactionsBuilder { self } - pub(in crate::state) fn with_processed_withdrawal_requests( + pub(in crate::state) fn with_processed_requests( mut self, processed_requests: BTreeMap, ) -> Self { From 78990bb0866be0dc9c6ef0fec4efa61daa62fb2c Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 11:25:32 +0200 Subject: [PATCH 05/16] refactor(cketh): give the pipeline's request contract its own file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../minter/src/state/transactions/mod.rs | 190 +----------------- .../minter/src/state/transactions/request.rs | 190 ++++++++++++++++++ .../minter/src/state/transactions/tests.rs | 3 +- 3 files changed, 198 insertions(+), 185 deletions(-) create mode 100644 rs/ethereum/cketh/minter/src/state/transactions/request.rs diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 8e9acc112f38..814f4dc91a3e 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -1,20 +1,23 @@ +mod request; + #[cfg(test)] pub(in crate::state) mod tests; +pub use request::PipelineRequest; + use crate::endpoints::{EthTransaction, RetrieveEthStatus, TxFinalizedStatus, WithdrawalStatus}; use crate::eth_logs::LedgerSubaccount; use crate::eth_rpc::Hash; use crate::eth_rpc_client::responses::TransactionReceipt; use crate::eth_rpc_client::responses::TransactionStatus; -use crate::lifecycle::EthereumNetwork; use crate::logs::INFO; use crate::map::MultiKeyMap; use crate::numeric::{ - CkTokenAmount, Erc20Value, GasAmount, LedgerBurnIndex, LedgerMintIndex, TransactionCount, + CkTokenAmount, Erc20Value, LedgerBurnIndex, LedgerMintIndex, TransactionCount, TransactionNonce, Wei, }; use crate::tx::{ - Eip1559TransactionRequest, FinalizedEip1559Transaction, GasFeeEstimate, ResubmissionStrategy, + Eip1559TransactionRequest, FinalizedEip1559Transaction, GasFeeEstimate, SignedEip1559TransactionRequest, SignedTransactionRequest, TransactionRequest, }; use candid::Principal; @@ -364,187 +367,6 @@ impl fmt::Debug for Erc20WithdrawalRequest { } } -/// A request that can flow through a [`TransactionPipeline`]: it carries an identity used as the -/// pipeline's alternate map key, and knows the EIP-1559 transaction it turns into. -/// -/// Implemented so far only by [`WithdrawalRequest`], the minter's main-address pipeline -/// (`Id = LedgerBurnIndex`); a second sender address will bring a second implementation. -pub trait PipelineRequest: Clone + Eq + fmt::Debug { - /// The pipeline's alternate map key — a ckETH `LedgerBurnIndex` for withdrawals. - type Id: Copy + Ord + fmt::Debug + fmt::Display; - - /// Why [`Self::create_transaction`] could not build a transaction. A request that always funds - /// its own fee can set this to [`std::convert::Infallible`], making the failure unrepresentable - /// rather than merely unreachable. - type Error; - - /// The identity of this request, used as the pipeline's alternate map key. - fn id(&self) -> Self::Id; - - /// Address the transaction is sent to. - fn destination(&self) -> Address; - - /// IC time at which the request was created, if tracked. - fn created_at(&self) -> Option; - - /// The fee-bump strategy for this request's resubmitted transactions. - fn resubmission_strategy(&self) -> ResubmissionStrategy; - - /// Assert that a freshly created transaction is consistent with the request (amount). - fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest); - - /// Creates the EIP-1559 transaction that fulfils this request. - /// - /// # Errors - /// * [`Self::Error`] if the request cannot cover the transaction fee. - fn create_transaction( - &self, - nonce: TransactionNonce, - gas_fee_estimate: GasFeeEstimate, - gas_limit: GasAmount, - ethereum_network: EthereumNetwork, - ) -> Result; -} - -impl PipelineRequest for WithdrawalRequest { - type Id = LedgerBurnIndex; - type Error = CreateTransactionError; - - fn id(&self) -> LedgerBurnIndex { - self.cketh_ledger_burn_index() - } - - fn destination(&self) -> Address { - WithdrawalRequest::destination(self) - } - - fn created_at(&self) -> Option { - WithdrawalRequest::created_at(self) - } - - fn resubmission_strategy(&self) -> ResubmissionStrategy { - match self { - WithdrawalRequest::CkEth(cketh) | WithdrawalRequest::SweeperFunding(cketh) => { - ResubmissionStrategy::ReduceEthAmount { - withdrawal_amount: cketh.withdrawal_amount, - } - } - WithdrawalRequest::CkErc20(ckerc20) => ResubmissionStrategy::GuaranteeEthAmount { - allowed_max_transaction_fee: ckerc20.max_transaction_fee, - }, - } - } - - fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest) { - match self { - WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => { - assert!( - req.withdrawal_amount > transaction.amount, - "BUG: transaction amount should be the withdrawal amount deducted from transaction fees" - ); - } - WithdrawalRequest::CkErc20(_req) => { - assert_eq!( - Wei::ZERO, - transaction.amount, - "BUG: ERC-20 transaction amount should be zero" - ); - } - } - } - - /// The transaction fees are paid by the beneficiary, meaning that the fees will be deducted - /// from the withdrawal amount. - fn create_transaction( - &self, - nonce: TransactionNonce, - gas_fee_estimate: GasFeeEstimate, - gas_limit: GasAmount, - ethereum_network: EthereumNetwork, - ) -> Result { - assert!( - gas_limit > GasAmount::ZERO, - "BUG: gas limit should be non-zero" - ); - match self { - WithdrawalRequest::CkEth(EthWithdrawalRequest { - withdrawal_amount, - destination, - ledger_burn_index, - .. - }) - | WithdrawalRequest::SweeperFunding(EthWithdrawalRequest { - withdrawal_amount, - destination, - ledger_burn_index, - .. - }) => { - let transaction_price = gas_fee_estimate.to_price(gas_limit); - let max_transaction_fee = transaction_price.max_transaction_fee(); - let tx_amount = match withdrawal_amount.checked_sub(max_transaction_fee) { - Some(tx_amount) => tx_amount, - None => { - return Err(CreateTransactionError::InsufficientTransactionFee { - cketh_ledger_burn_index: *ledger_burn_index, - allowed_max_transaction_fee: *withdrawal_amount, - actual_max_transaction_fee: max_transaction_fee, - }); - } - }; - Ok(Eip1559TransactionRequest { - chain_id: ethereum_network.chain_id(), - nonce, - max_priority_fee_per_gas: transaction_price.max_priority_fee_per_gas, - max_fee_per_gas: transaction_price.max_fee_per_gas, - gas_limit: transaction_price.gas_limit, - destination: *destination, - amount: tx_amount, - data: Vec::new(), - access_list: Default::default(), - }) - } - WithdrawalRequest::CkErc20(request) => { - // The transaction fee is already paid and must be at most - // the `max_transaction_fee` in the withdrawal request, which, given a gas limit, gives us an upper bound on - // the `max_fee_per_gas`. We allocate the maximum from the beginning to minimize - // transaction resubmissions: even if the `base_fee_per_gas` increases considerably, - // the transaction could still make it as long as `transaction.max_fee_per_gas >= block.base_fee_per_gas`, - // since the `priority_fee_per_gas` received by the miner is capped to (see https://eips.ethereum.org/EIPS/eip-1559) - // min(transaction.max_priority_fee_per_gas, transaction.max_fee_per_gas - block.base_fee_per_gas). - let request_max_fee_per_gas = request - .max_transaction_fee - .into_wei_per_gas(gas_limit) - .expect("BUG: gas_limit should be non-zero"); - let actual_min_max_fee_per_gas = gas_fee_estimate.min_max_fee_per_gas(); - if actual_min_max_fee_per_gas > request_max_fee_per_gas { - return Err(CreateTransactionError::InsufficientTransactionFee { - cketh_ledger_burn_index: request.cketh_ledger_burn_index, - allowed_max_transaction_fee: request.max_transaction_fee, - actual_max_transaction_fee: actual_min_max_fee_per_gas - .transaction_cost(gas_limit) - .unwrap_or(Wei::MAX), - }); - } - Ok(Eip1559TransactionRequest { - chain_id: ethereum_network.chain_id(), - nonce, - max_priority_fee_per_gas: gas_fee_estimate.max_priority_fee_per_gas, - max_fee_per_gas: request_max_fee_per_gas, - gas_limit, - destination: request.erc20_contract_address, - amount: Wei::ZERO, - data: TransactionCallData::Erc20Transfer { - to: request.destination, - value: request.withdrawal_amount, - } - .encode(), - access_list: Default::default(), - }) - } - } - } -} - /// State machine holding Ethereum transactions issued by the minter from a **single sender /// address**, on that address' **own nonce sequence** — generic over the request type `R` so a /// second sender address can drive the same machinery on a nonce sequence of its own without diff --git a/rs/ethereum/cketh/minter/src/state/transactions/request.rs b/rs/ethereum/cketh/minter/src/state/transactions/request.rs new file mode 100644 index 000000000000..76e97ec0518b --- /dev/null +++ b/rs/ethereum/cketh/minter/src/state/transactions/request.rs @@ -0,0 +1,190 @@ +//! What it takes for a request to travel a [`TransactionPipeline`], and the minter's own +//! implementation of it. + +use super::{CreateTransactionError, EthWithdrawalRequest, TransactionCallData, WithdrawalRequest}; +use crate::lifecycle::EthereumNetwork; +use crate::numeric::{GasAmount, LedgerBurnIndex, TransactionNonce, Wei}; +use crate::tx::{Eip1559TransactionRequest, GasFeeEstimate, ResubmissionStrategy}; +use ic_ethereum_types::Address; +use std::fmt; + +/// A request that can flow through a [`TransactionPipeline`]: it carries an identity used as the +/// pipeline's alternate map key, and knows the EIP-1559 transaction it turns into. +/// +/// Implemented so far only by [`WithdrawalRequest`], the minter's main-address pipeline +/// (`Id = LedgerBurnIndex`); a second sender address will bring a second implementation. +pub trait PipelineRequest: Clone + Eq + fmt::Debug { + /// The pipeline's alternate map key — a ckETH `LedgerBurnIndex` for withdrawals. + type Id: Copy + Ord + fmt::Debug + fmt::Display; + + /// Why [`Self::create_transaction`] could not build a transaction. A request that always funds + /// its own fee can set this to [`std::convert::Infallible`], making the failure unrepresentable + /// rather than merely unreachable. + type Error; + + /// The identity of this request, used as the pipeline's alternate map key. + fn id(&self) -> Self::Id; + + /// Address the transaction is sent to. + fn destination(&self) -> Address; + + /// IC time at which the request was created, if tracked. + fn created_at(&self) -> Option; + + /// The fee-bump strategy for this request's resubmitted transactions. + fn resubmission_strategy(&self) -> ResubmissionStrategy; + + /// Assert that a freshly created transaction is consistent with the request (amount). + fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest); + + /// Creates the EIP-1559 transaction that fulfils this request. + /// + /// # Errors + /// * [`Self::Error`] if the request cannot cover the transaction fee. + fn create_transaction( + &self, + nonce: TransactionNonce, + gas_fee_estimate: GasFeeEstimate, + gas_limit: GasAmount, + ethereum_network: EthereumNetwork, + ) -> Result; +} + +impl PipelineRequest for WithdrawalRequest { + type Id = LedgerBurnIndex; + type Error = CreateTransactionError; + + fn id(&self) -> LedgerBurnIndex { + self.cketh_ledger_burn_index() + } + + fn destination(&self) -> Address { + WithdrawalRequest::destination(self) + } + + fn created_at(&self) -> Option { + WithdrawalRequest::created_at(self) + } + + fn resubmission_strategy(&self) -> ResubmissionStrategy { + match self { + WithdrawalRequest::CkEth(cketh) | WithdrawalRequest::SweeperFunding(cketh) => { + ResubmissionStrategy::ReduceEthAmount { + withdrawal_amount: cketh.withdrawal_amount, + } + } + WithdrawalRequest::CkErc20(ckerc20) => ResubmissionStrategy::GuaranteeEthAmount { + allowed_max_transaction_fee: ckerc20.max_transaction_fee, + }, + } + } + + fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest) { + match self { + WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => { + assert!( + req.withdrawal_amount > transaction.amount, + "BUG: transaction amount should be the withdrawal amount deducted from transaction fees" + ); + } + WithdrawalRequest::CkErc20(_req) => { + assert_eq!( + Wei::ZERO, + transaction.amount, + "BUG: ERC-20 transaction amount should be zero" + ); + } + } + } + + /// The transaction fees are paid by the beneficiary, meaning that the fees will be deducted + /// from the withdrawal amount. + fn create_transaction( + &self, + nonce: TransactionNonce, + gas_fee_estimate: GasFeeEstimate, + gas_limit: GasAmount, + ethereum_network: EthereumNetwork, + ) -> Result { + assert!( + gas_limit > GasAmount::ZERO, + "BUG: gas limit should be non-zero" + ); + match self { + WithdrawalRequest::CkEth(EthWithdrawalRequest { + withdrawal_amount, + destination, + ledger_burn_index, + .. + }) + | WithdrawalRequest::SweeperFunding(EthWithdrawalRequest { + withdrawal_amount, + destination, + ledger_burn_index, + .. + }) => { + let transaction_price = gas_fee_estimate.to_price(gas_limit); + let max_transaction_fee = transaction_price.max_transaction_fee(); + let tx_amount = match withdrawal_amount.checked_sub(max_transaction_fee) { + Some(tx_amount) => tx_amount, + None => { + return Err(CreateTransactionError::InsufficientTransactionFee { + cketh_ledger_burn_index: *ledger_burn_index, + allowed_max_transaction_fee: *withdrawal_amount, + actual_max_transaction_fee: max_transaction_fee, + }); + } + }; + Ok(Eip1559TransactionRequest { + chain_id: ethereum_network.chain_id(), + nonce, + max_priority_fee_per_gas: transaction_price.max_priority_fee_per_gas, + max_fee_per_gas: transaction_price.max_fee_per_gas, + gas_limit: transaction_price.gas_limit, + destination: *destination, + amount: tx_amount, + data: Vec::new(), + access_list: Default::default(), + }) + } + WithdrawalRequest::CkErc20(request) => { + // The transaction fee is already paid and must be at most + // the `max_transaction_fee` in the withdrawal request, which, given a gas limit, gives us an upper bound on + // the `max_fee_per_gas`. We allocate the maximum from the beginning to minimize + // transaction resubmissions: even if the `base_fee_per_gas` increases considerably, + // the transaction could still make it as long as `transaction.max_fee_per_gas >= block.base_fee_per_gas`, + // since the `priority_fee_per_gas` received by the miner is capped to (see https://eips.ethereum.org/EIPS/eip-1559) + // min(transaction.max_priority_fee_per_gas, transaction.max_fee_per_gas - block.base_fee_per_gas). + let request_max_fee_per_gas = request + .max_transaction_fee + .into_wei_per_gas(gas_limit) + .expect("BUG: gas_limit should be non-zero"); + let actual_min_max_fee_per_gas = gas_fee_estimate.min_max_fee_per_gas(); + if actual_min_max_fee_per_gas > request_max_fee_per_gas { + return Err(CreateTransactionError::InsufficientTransactionFee { + cketh_ledger_burn_index: request.cketh_ledger_burn_index, + allowed_max_transaction_fee: request.max_transaction_fee, + actual_max_transaction_fee: actual_min_max_fee_per_gas + .transaction_cost(gas_limit) + .unwrap_or(Wei::MAX), + }); + } + Ok(Eip1559TransactionRequest { + chain_id: ethereum_network.chain_id(), + nonce, + max_priority_fee_per_gas: gas_fee_estimate.max_priority_fee_per_gas, + max_fee_per_gas: request_max_fee_per_gas, + gas_limit, + destination: request.erc20_contract_address, + amount: Wei::ZERO, + data: TransactionCallData::Erc20Transfer { + to: request.destination, + value: request.withdrawal_amount, + } + .encode(), + access_list: Default::default(), + }) + } + } + } +} diff --git a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs index e6d9a53b232b..5ab26736b0ed 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs @@ -2650,9 +2650,10 @@ mod create_transaction { mod withdrawal_flow { use super::arbitrary::{arb_checked_amount_of, arb_gas_fee_estimate, arb_withdrawal_request}; + use crate::lifecycle::EthereumNetwork; use crate::numeric::TransactionNonce; use crate::state::transactions::tests::sign_transaction; - use crate::state::transactions::{EthereumNetwork, PipelineRequest, WithdrawalTransactions}; + use crate::state::transactions::{PipelineRequest, WithdrawalTransactions}; use crate::withdraw::estimate_gas_limit; use proptest::proptest; use std::cell::RefCell; From 9e0e3da8f0e78288df093de8a77e21a4288f200a Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 11:41:55 +0200 Subject: [PATCH 06/16] refactor(cketh): require Clone, Eq and Debug where they are used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- rs/ethereum/cketh/minter/src/state/transactions/mod.rs | 4 ++-- rs/ethereum/cketh/minter/src/state/transactions/request.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 814f4dc91a3e..419ef061a420 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -432,7 +432,7 @@ pub enum TransactionStage<'a> { /// re-sign (paired with its pipeline id), or why it could not be bumped. type ResubmitResult = Result<(Id, Eip1559TransactionRequest), ResubmitTransactionError>; -impl TransactionPipeline { +impl TransactionPipeline { pub fn new(next_nonce: TransactionNonce) -> Self { Self { pending_requests: VecDeque::new(), @@ -822,7 +822,7 @@ impl TransactionPipeline { pub fn is_equivalent_to(&self, other: &Self) -> Result<(), String> { use ic_utils_ensure::ensure_eq; - fn sorted_requests(requests: &VecDeque) -> Vec { + fn sorted_requests(requests: &VecDeque) -> Vec { let mut buf: Vec<_> = requests.iter().cloned().collect(); buf.sort_unstable_by_key(|req| req.id()); buf diff --git a/rs/ethereum/cketh/minter/src/state/transactions/request.rs b/rs/ethereum/cketh/minter/src/state/transactions/request.rs index 76e97ec0518b..c57d035e1901 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/request.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/request.rs @@ -13,7 +13,7 @@ use std::fmt; /// /// Implemented so far only by [`WithdrawalRequest`], the minter's main-address pipeline /// (`Id = LedgerBurnIndex`); a second sender address will bring a second implementation. -pub trait PipelineRequest: Clone + Eq + fmt::Debug { +pub trait PipelineRequest { /// The pipeline's alternate map key — a ckETH `LedgerBurnIndex` for withdrawals. type Id: Copy + Ord + fmt::Debug + fmt::Display; From 59aad5fa79936395493e510cc9fed8c7e9017632 Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 11:53:15 +0200 Subject: [PATCH 07/16] refactor(cketh): let the pipeline's panic messages name the id's type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- rs/ethereum/cketh/minter/src/state/transactions/mod.rs | 8 ++++---- .../cketh/minter/src/state/transactions/request.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 419ef061a420..e2c7c3d33cd8 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -460,7 +460,7 @@ impl TransactionPipeline { || self.sent_tx.contains_alt(&id) || self.finalized_tx.contains_alt(&id) { - panic!("BUG: duplicate transaction id {id}"); + panic!("BUG: duplicate transaction id {id:?}"); } self.pending_requests.push_back(request); } @@ -474,7 +474,7 @@ impl TransactionPipeline { .filter(|r| r.id() == request.id()) .count(), 1, - "BUG: expected exactly one request with id {}", + "BUG: expected exactly one request with id {:?}", request.id() ); self.remove_request(&request); @@ -491,7 +491,7 @@ impl TransactionPipeline { .iter() .find(|req| req.id() == id) .cloned() - .unwrap_or_else(|| panic!("BUG: request {id} not found")); + .unwrap_or_else(|| panic!("BUG: request {id:?} not found")); assert!( self.pending_requests.contains(&request), "BUG: request not found" @@ -628,7 +628,7 @@ impl TransactionPipeline { assert_eq!( prev_index, *index, - "BUG: duplicate transaction hash {} for burn indices {prev_index} and {index}", + "BUG: duplicate transaction hash {} for ids {prev_index:?} and {index:?}", sent_tx.as_ref().hash() ); } diff --git a/rs/ethereum/cketh/minter/src/state/transactions/request.rs b/rs/ethereum/cketh/minter/src/state/transactions/request.rs index c57d035e1901..f7c46bd2e489 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/request.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/request.rs @@ -15,7 +15,7 @@ use std::fmt; /// (`Id = LedgerBurnIndex`); a second sender address will bring a second implementation. pub trait PipelineRequest { /// The pipeline's alternate map key — a ckETH `LedgerBurnIndex` for withdrawals. - type Id: Copy + Ord + fmt::Debug + fmt::Display; + type Id: Copy + Ord + fmt::Debug; /// Why [`Self::create_transaction`] could not build a transaction. A request that always funds /// its own fee can set this to [`std::convert::Infallible`], making the failure unrepresentable From 18674e59c873785325efc98010482baaa768f9b8 Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 13:16:25 +0200 Subject: [PATCH 08/16] refactor(cketh): take the request out of the queue instead of cloning it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../minter/src/state/transactions/mod.rs | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index e2c7c3d33cd8..cf51b2376ab1 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -467,17 +467,24 @@ impl TransactionPipeline { /// Move an existing request to the back of the queue. pub fn reschedule_request>(&mut self, request: Req) { - let request = request.into(); + let id = request.into().id(); assert_eq!( self.pending_requests .iter() - .filter(|r| r.id() == request.id()) + .filter(|r| r.id() == id) .count(), 1, - "BUG: expected exactly one request with id {:?}", - request.id() + "BUG: expected exactly one request with id {id:?}" ); - self.remove_request(&request); + let position = self + .pending_requests + .iter() + .position(|r| r.id() == id) + .expect("BUG: exactly one request with this id was just counted"); + let request = self + .pending_requests + .remove(position) + .expect("BUG: position was just found in the queue"); self.record_request(request); } @@ -486,32 +493,32 @@ impl TransactionPipeline { id: R::Id, transaction: Eip1559TransactionRequest, ) { - let request = self + let position = self .pending_requests .iter() - .find(|req| req.id() == id) - .cloned() + .position(|req| req.id() == id) .unwrap_or_else(|| panic!("BUG: request {id:?} not found")); - assert!( - self.pending_requests.contains(&request), - "BUG: request not found" - ); + let request = &self.pending_requests[position]; assert_eq!( request.destination(), transaction.destination, "BUG: request and transaction destination mismatch" ); request.assert_created_transaction(&transaction); + let resubmission = request.resubmission_strategy(); let nonce = self.next_nonce; assert_eq!(transaction.nonce, nonce, "BUG: transaction nonce mismatch"); self.next_nonce = self .next_nonce .checked_increment() .expect("Transaction nonce overflow"); - self.remove_request(&request); + let request = self + .pending_requests + .remove(position) + .expect("BUG: position was just found in the queue"); let transaction_request = TransactionRequest { transaction, - resubmission: request.resubmission_strategy(), + resubmission, }; assert_eq!( self.created_tx @@ -788,10 +795,6 @@ impl TransactionPipeline { !self.pending_requests.is_empty() || !self.created_tx.is_empty() || !self.sent_tx.is_empty() } - fn remove_request(&mut self, request: &R) { - self.pending_requests.retain(|r| r != request); - } - fn expect_last_sent_tx_entry<'a>( sent_tx: &'a MultiKeyMap>, nonce: &TransactionNonce, From e4975f0b283e91102888f69d62665f18c02d255e Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 13:38:11 +0200 Subject: [PATCH 09/16] refactor(cketh): let the request check its own destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../cketh/minter/src/state/transactions/mod.rs | 5 ----- .../minter/src/state/transactions/request.rs | 16 +++++++--------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index cf51b2376ab1..18010fbde4ff 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -499,11 +499,6 @@ impl TransactionPipeline { .position(|req| req.id() == id) .unwrap_or_else(|| panic!("BUG: request {id:?} not found")); let request = &self.pending_requests[position]; - assert_eq!( - request.destination(), - transaction.destination, - "BUG: request and transaction destination mismatch" - ); request.assert_created_transaction(&transaction); let resubmission = request.resubmission_strategy(); let nonce = self.next_nonce; diff --git a/rs/ethereum/cketh/minter/src/state/transactions/request.rs b/rs/ethereum/cketh/minter/src/state/transactions/request.rs index f7c46bd2e489..5f4f39960627 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/request.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/request.rs @@ -5,7 +5,6 @@ use super::{CreateTransactionError, EthWithdrawalRequest, TransactionCallData, W use crate::lifecycle::EthereumNetwork; use crate::numeric::{GasAmount, LedgerBurnIndex, TransactionNonce, Wei}; use crate::tx::{Eip1559TransactionRequest, GasFeeEstimate, ResubmissionStrategy}; -use ic_ethereum_types::Address; use std::fmt; /// A request that can flow through a [`TransactionPipeline`]: it carries an identity used as the @@ -25,16 +24,14 @@ pub trait PipelineRequest { /// The identity of this request, used as the pipeline's alternate map key. fn id(&self) -> Self::Id; - /// Address the transaction is sent to. - fn destination(&self) -> Address; - /// IC time at which the request was created, if tracked. fn created_at(&self) -> Option; /// The fee-bump strategy for this request's resubmitted transactions. fn resubmission_strategy(&self) -> ResubmissionStrategy; - /// Assert that a freshly created transaction is consistent with the request (amount). + /// Assert that a freshly created transaction is consistent with the request: it goes to the + /// right address, and moves the right amount. fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest); /// Creates the EIP-1559 transaction that fulfils this request. @@ -58,10 +55,6 @@ impl PipelineRequest for WithdrawalRequest { self.cketh_ledger_burn_index() } - fn destination(&self) -> Address { - WithdrawalRequest::destination(self) - } - fn created_at(&self) -> Option { WithdrawalRequest::created_at(self) } @@ -80,6 +73,11 @@ impl PipelineRequest for WithdrawalRequest { } fn assert_created_transaction(&self, transaction: &Eip1559TransactionRequest) { + assert_eq!( + self.destination(), + transaction.destination, + "BUG: request and transaction destination mismatch" + ); match self { WithdrawalRequest::CkEth(req) | WithdrawalRequest::SweeperFunding(req) => { assert!( From 2b763a2748721cd576a66276f1b23d1a905ddc6a Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 13:50:38 +0200 Subject: [PATCH 10/16] refactor(cketh): keep WithdrawalTransactions in one impl block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- rs/ethereum/cketh/minter/src/state/transactions/mod.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 18010fbde4ff..2ef8e4b37bff 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -1205,12 +1205,7 @@ impl WithdrawalTransactions { pub fn has_pending_requests(&self) -> bool { self.pipeline.has_pending_requests() } -} -/// The withdrawal-status queries backing the minter's `retrieve_eth_status` and -/// `withdrawal_status` endpoints. They read the pipeline and the reimbursement records together, -/// which is why they live on the wrapper; the sweeper pipeline answers no such endpoint. -impl WithdrawalTransactions { pub fn withdrawal_status( &self, parameter: &WithdrawalSearchParameter, From a9ad1179b794ab0cdf4f8414729325937f4cbee1 Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 13:58:40 +0200 Subject: [PATCH 11/16] refactor(cketh): put the reimbursement methods back where they were 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) --- .../minter/src/state/transactions/mod.rs | 231 ++++++++---------- 1 file changed, 98 insertions(+), 133 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 2ef8e4b37bff..66201a7abb89 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -863,7 +863,6 @@ impl WithdrawalTransactions { reimbursed: Default::default(), } } - /// Record a created transaction, and remember that the request may still need paying back. pub fn record_created_transaction( &mut self, @@ -875,7 +874,6 @@ impl WithdrawalTransactions { assert!(self.maybe_reimburse.insert(id)); } } - /// Whether a failed transaction for this request would pay the requester back. A sweeper /// funding never is, so it is never armed for reimbursement in the first place. fn is_reimbursable(&self, withdrawal_id: &LedgerBurnIndex) -> bool { @@ -884,7 +882,6 @@ impl WithdrawalTransactions { .expect("BUG: missing processed withdrawal request") .is_reimbursable() } - /// Finalize the transaction for `ledger_burn_index` matching `receipt`, then — if it failed on /// chain — record the corresponding ckETH/ckERC20 reimbursement. pub fn record_finalized_transaction( @@ -960,21 +957,6 @@ impl WithdrawalTransactions { }; self.record_reimbursement_request(index, reimbursement); } - - /// Whether any request is still in flight, either awaiting a transaction or a reimbursement. - pub fn oldest_incomplete_request_timestamp(&self) -> Option { - self.requests_iter() - .chain(self.maybe_reimburse_requests_iter()) - .flat_map(|req| req.created_at().into_iter()) - .min() - } - - fn maybe_reimburse_requests_iter(&self) -> impl Iterator { - self.maybe_reimburse - .iter() - .filter_map(|index| self.pipeline.get_processed_request(index)) - } - pub fn is_equivalent_to(&self, other: &Self) -> Result<(), String> { use ic_utils_ensure::ensure_eq; @@ -983,122 +965,24 @@ impl WithdrawalTransactions { ensure_eq!(self.reimbursed, other.reimbursed); self.pipeline.is_equivalent_to(&other.pipeline) } - - pub fn reimbursement_requests_iter( - &self, - ) -> impl Iterator { - self.reimbursement_requests.iter() - } - - pub fn reimbursed_transactions_iter( - &self, - ) -> impl Iterator { - self.reimbursed.iter() - } - - fn find_reimbursed_transaction_by_cketh_ledger_burn_index( - &self, - searched_burn_index: &LedgerBurnIndex, - ) -> Option<&ReimbursedResult> { - self.reimbursed - .iter() - .find_map(|(index, value)| match index { - ReimbursementIndex::CkEth { ledger_burn_index } - if ledger_burn_index == searched_burn_index => - { - Some(value) - } - ReimbursementIndex::CkErc20 { - cketh_ledger_burn_index, - .. - } if cketh_ledger_burn_index == searched_burn_index => Some(value), - _ => None, - }) - } - - /// Arm the reimbursement for a withdrawal whose transaction failed on chain. - /// - /// # Panics - /// If the withdrawal is still armed for reimbursement, or was already reimbursed — either - /// would let the same burn be minted back twice. - pub fn record_reimbursement_request( - &mut self, - index: ReimbursementIndex, - request: ReimbursementRequest, - ) { - assert_eq!( - self.maybe_reimburse.get(&index.withdrawal_id()), - None, - "BUG: withdrawal request still in maybe_reimburse could lead to double minting!" - ); - assert_eq!( - self.reimbursed.get(&index), - None, - "BUG: reimbursement request was already processed" - ); - assert_eq!( - self.reimbursement_requests.insert(index.clone(), request), - None, - "BUG: reimbursement request for withdrawal {index:?} already exists" - ); - } - - /// Quarantine the reimbursement request identified by its index to prevent double minting. - /// WARNING!: It's crucial that this method does not panic, - /// since it's called inside the clean-up callback, when an unexpected panic did occur before. - pub fn record_quarantined_reimbursement(&mut self, index: ReimbursementIndex) { - self.reimbursement_requests.remove(&index); - self.reimbursed - .insert(index, Err(ReimbursedError::Quarantined)); - } - - pub fn record_finalized_reimbursement( - &mut self, - index: ReimbursementIndex, - reimbursed_in_block: LedgerMintIndex, - ) { - let reimbursement_request = self - .reimbursement_requests - .remove(&index) - .unwrap_or_else(|| panic!("BUG: missing reimbursement request with index {index:?}")); - let burn_in_block = index.burn_in_block(); - assert_eq!( - self.reimbursed.insert( - index, - Ok(Reimbursed { - burn_in_block, - reimbursed_in_block, - reimbursed_amount: reimbursement_request.reimbursed_amount, - transaction_hash: reimbursement_request.transaction_hash, - }), - ), - None - ); - } - pub fn next_transaction_nonce(&self) -> TransactionNonce { self.pipeline.next_transaction_nonce() } - pub fn update_next_transaction_nonce(&mut self, new_nonce: TransactionNonce) { self.pipeline.update_next_transaction_nonce(new_nonce) } - pub fn record_request>(&mut self, request: Req) { self.pipeline.record_request(request) } - pub fn reschedule_request>(&mut self, request: Req) { self.pipeline.reschedule_request(request) } - pub fn record_signed_transaction( &mut self, signed_transaction: SignedEip1559TransactionRequest, ) { self.pipeline.record_signed_transaction(signed_transaction) } - pub fn create_resubmit_transactions( &self, latest_transaction_count: TransactionCount, @@ -1107,11 +991,9 @@ impl WithdrawalTransactions { self.pipeline .create_resubmit_transactions(latest_transaction_count, current_gas_fee) } - pub fn record_resubmit_transaction(&mut self, new_tx: Eip1559TransactionRequest) { self.pipeline.record_resubmit_transaction(new_tx) } - pub fn sent_transactions_to_finalize( &self, finalized_transaction_count: &TransactionCount, @@ -1119,19 +1001,15 @@ impl WithdrawalTransactions { self.pipeline .sent_transactions_to_finalize(finalized_transaction_count) } - pub fn requests_batch(&self, requested_batch_size: usize) -> Vec { self.pipeline.requests_batch(requested_batch_size) } - pub fn requests_iter(&self) -> impl Iterator { self.pipeline.requests_iter() } - pub fn requests_len(&self) -> usize { self.pipeline.requests_len() } - pub fn transactions_to_sign_iter( &self, ) -> impl Iterator< @@ -1143,14 +1021,12 @@ impl WithdrawalTransactions { > { self.pipeline.transactions_to_sign_iter() } - pub fn transactions_to_sign_batch( &self, batch_size: usize, ) -> Vec<(LedgerBurnIndex, Eip1559TransactionRequest)> { self.pipeline.transactions_to_sign_batch(batch_size) } - pub fn transactions_to_send_batch( &self, latest_transaction_count: TransactionCount, @@ -1159,7 +1035,6 @@ impl WithdrawalTransactions { self.pipeline .transactions_to_send_batch(latest_transaction_count, batch_size) } - pub fn sent_transactions_iter( &self, ) -> impl Iterator< @@ -1171,21 +1046,18 @@ impl WithdrawalTransactions { > { self.pipeline.sent_transactions_iter() } - pub fn get_finalized_transaction( &self, burn_index: &LedgerBurnIndex, ) -> Option<&FinalizedEip1559Transaction> { self.pipeline.get_finalized_transaction(burn_index) } - pub fn get_processed_request( &self, burn_index: &LedgerBurnIndex, ) -> Option<&WithdrawalRequest> { self.pipeline.get_processed_request(burn_index) } - pub fn finalized_transactions_iter( &self, ) -> impl Iterator< @@ -1197,15 +1069,110 @@ impl WithdrawalTransactions { > { self.pipeline.finalized_transactions_iter() } - pub fn is_sent_tx_empty(&self) -> bool { self.pipeline.is_sent_tx_empty() } - pub fn has_pending_requests(&self) -> bool { self.pipeline.has_pending_requests() } - + pub fn reimbursement_requests_iter( + &self, + ) -> impl Iterator { + self.reimbursement_requests.iter() + } + pub fn reimbursed_transactions_iter( + &self, + ) -> impl Iterator { + self.reimbursed.iter() + } + fn find_reimbursed_transaction_by_cketh_ledger_burn_index( + &self, + searched_burn_index: &LedgerBurnIndex, + ) -> Option<&ReimbursedResult> { + self.reimbursed + .iter() + .find_map(|(index, value)| match index { + ReimbursementIndex::CkEth { ledger_burn_index } + if ledger_burn_index == searched_burn_index => + { + Some(value) + } + ReimbursementIndex::CkErc20 { + cketh_ledger_burn_index, + .. + } if cketh_ledger_burn_index == searched_burn_index => Some(value), + _ => None, + }) + } + /// Quarantine the reimbursement request identified by its index to prevent double minting. + /// WARNING!: It's crucial that this method does not panic, + /// since it's called inside the clean-up callback, when an unexpected panic did occur before. + pub fn record_quarantined_reimbursement(&mut self, index: ReimbursementIndex) { + self.reimbursement_requests.remove(&index); + self.reimbursed + .insert(index, Err(ReimbursedError::Quarantined)); + } + pub fn record_finalized_reimbursement( + &mut self, + index: ReimbursementIndex, + reimbursed_in_block: LedgerMintIndex, + ) { + let reimbursement_request = self + .reimbursement_requests + .remove(&index) + .unwrap_or_else(|| panic!("BUG: missing reimbursement request with index {index:?}")); + let burn_in_block = index.burn_in_block(); + assert_eq!( + self.reimbursed.insert( + index, + Ok(Reimbursed { + burn_in_block, + reimbursed_in_block, + reimbursed_amount: reimbursement_request.reimbursed_amount, + transaction_hash: reimbursement_request.transaction_hash, + }), + ), + None + ); + } + /// Arm the reimbursement for a withdrawal whose transaction failed on chain. + /// + /// # Panics + /// If the withdrawal is still armed for reimbursement, or was already reimbursed — either + /// would let the same burn be minted back twice. + pub fn record_reimbursement_request( + &mut self, + index: ReimbursementIndex, + request: ReimbursementRequest, + ) { + assert_eq!( + self.maybe_reimburse.get(&index.withdrawal_id()), + None, + "BUG: withdrawal request still in maybe_reimburse could lead to double minting!" + ); + assert_eq!( + self.reimbursed.get(&index), + None, + "BUG: reimbursement request was already processed" + ); + assert_eq!( + self.reimbursement_requests.insert(index.clone(), request), + None, + "BUG: reimbursement request for withdrawal {index:?} already exists" + ); + } + fn maybe_reimburse_requests_iter(&self) -> impl Iterator { + self.maybe_reimburse + .iter() + .filter_map(|index| self.pipeline.get_processed_request(index)) + } + /// Whether any request is still in flight, either awaiting a transaction or a reimbursement. + pub fn oldest_incomplete_request_timestamp(&self) -> Option { + self.requests_iter() + .chain(self.maybe_reimburse_requests_iter()) + .flat_map(|req| req.created_at().into_iter()) + .min() + } pub fn withdrawal_status( &self, parameter: &WithdrawalSearchParameter, @@ -1244,7 +1211,6 @@ impl WithdrawalTransactions { pending.chain(processed).collect() } - pub fn transaction_status(&self, burn_index: &LedgerBurnIndex) -> RetrieveEthStatus { if self .pipeline @@ -1255,7 +1221,6 @@ impl WithdrawalTransactions { } self.processed_transaction_status(burn_index).0 } - fn processed_transaction_status( &self, burn_index: &LedgerBurnIndex, From 641c22f521a83391b3eb4cd24e1ed5a07cbb5c72 Mon Sep 17 00:00:00 2001 From: gregorydemay Date: Thu, 20 Aug 2026 14:13:09 +0200 Subject: [PATCH 12/16] style(cketh): restore the blank lines between WithdrawalTransactions' methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../minter/src/state/transactions/mod.rs | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 66201a7abb89..4dc42fd89632 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -863,6 +863,7 @@ impl WithdrawalTransactions { reimbursed: Default::default(), } } + /// Record a created transaction, and remember that the request may still need paying back. pub fn record_created_transaction( &mut self, @@ -874,6 +875,7 @@ impl WithdrawalTransactions { assert!(self.maybe_reimburse.insert(id)); } } + /// Whether a failed transaction for this request would pay the requester back. A sweeper /// funding never is, so it is never armed for reimbursement in the first place. fn is_reimbursable(&self, withdrawal_id: &LedgerBurnIndex) -> bool { @@ -882,6 +884,7 @@ impl WithdrawalTransactions { .expect("BUG: missing processed withdrawal request") .is_reimbursable() } + /// Finalize the transaction for `ledger_burn_index` matching `receipt`, then — if it failed on /// chain — record the corresponding ckETH/ckERC20 reimbursement. pub fn record_finalized_transaction( @@ -957,6 +960,7 @@ impl WithdrawalTransactions { }; self.record_reimbursement_request(index, reimbursement); } + pub fn is_equivalent_to(&self, other: &Self) -> Result<(), String> { use ic_utils_ensure::ensure_eq; @@ -965,24 +969,30 @@ impl WithdrawalTransactions { ensure_eq!(self.reimbursed, other.reimbursed); self.pipeline.is_equivalent_to(&other.pipeline) } + pub fn next_transaction_nonce(&self) -> TransactionNonce { self.pipeline.next_transaction_nonce() } + pub fn update_next_transaction_nonce(&mut self, new_nonce: TransactionNonce) { self.pipeline.update_next_transaction_nonce(new_nonce) } + pub fn record_request>(&mut self, request: Req) { self.pipeline.record_request(request) } + pub fn reschedule_request>(&mut self, request: Req) { self.pipeline.reschedule_request(request) } + pub fn record_signed_transaction( &mut self, signed_transaction: SignedEip1559TransactionRequest, ) { self.pipeline.record_signed_transaction(signed_transaction) } + pub fn create_resubmit_transactions( &self, latest_transaction_count: TransactionCount, @@ -991,9 +1001,11 @@ impl WithdrawalTransactions { self.pipeline .create_resubmit_transactions(latest_transaction_count, current_gas_fee) } + pub fn record_resubmit_transaction(&mut self, new_tx: Eip1559TransactionRequest) { self.pipeline.record_resubmit_transaction(new_tx) } + pub fn sent_transactions_to_finalize( &self, finalized_transaction_count: &TransactionCount, @@ -1001,15 +1013,19 @@ impl WithdrawalTransactions { self.pipeline .sent_transactions_to_finalize(finalized_transaction_count) } + pub fn requests_batch(&self, requested_batch_size: usize) -> Vec { self.pipeline.requests_batch(requested_batch_size) } + pub fn requests_iter(&self) -> impl Iterator { self.pipeline.requests_iter() } + pub fn requests_len(&self) -> usize { self.pipeline.requests_len() } + pub fn transactions_to_sign_iter( &self, ) -> impl Iterator< @@ -1021,12 +1037,14 @@ impl WithdrawalTransactions { > { self.pipeline.transactions_to_sign_iter() } + pub fn transactions_to_sign_batch( &self, batch_size: usize, ) -> Vec<(LedgerBurnIndex, Eip1559TransactionRequest)> { self.pipeline.transactions_to_sign_batch(batch_size) } + pub fn transactions_to_send_batch( &self, latest_transaction_count: TransactionCount, @@ -1035,6 +1053,7 @@ impl WithdrawalTransactions { self.pipeline .transactions_to_send_batch(latest_transaction_count, batch_size) } + pub fn sent_transactions_iter( &self, ) -> impl Iterator< @@ -1046,18 +1065,21 @@ impl WithdrawalTransactions { > { self.pipeline.sent_transactions_iter() } + pub fn get_finalized_transaction( &self, burn_index: &LedgerBurnIndex, ) -> Option<&FinalizedEip1559Transaction> { self.pipeline.get_finalized_transaction(burn_index) } + pub fn get_processed_request( &self, burn_index: &LedgerBurnIndex, ) -> Option<&WithdrawalRequest> { self.pipeline.get_processed_request(burn_index) } + pub fn finalized_transactions_iter( &self, ) -> impl Iterator< @@ -1069,22 +1091,27 @@ impl WithdrawalTransactions { > { self.pipeline.finalized_transactions_iter() } + pub fn is_sent_tx_empty(&self) -> bool { self.pipeline.is_sent_tx_empty() } + pub fn has_pending_requests(&self) -> bool { self.pipeline.has_pending_requests() } + pub fn reimbursement_requests_iter( &self, ) -> impl Iterator { self.reimbursement_requests.iter() } + pub fn reimbursed_transactions_iter( &self, ) -> impl Iterator { self.reimbursed.iter() } + fn find_reimbursed_transaction_by_cketh_ledger_burn_index( &self, searched_burn_index: &LedgerBurnIndex, @@ -1104,6 +1131,7 @@ impl WithdrawalTransactions { _ => None, }) } + /// Quarantine the reimbursement request identified by its index to prevent double minting. /// WARNING!: It's crucial that this method does not panic, /// since it's called inside the clean-up callback, when an unexpected panic did occur before. @@ -1112,6 +1140,7 @@ impl WithdrawalTransactions { self.reimbursed .insert(index, Err(ReimbursedError::Quarantined)); } + pub fn record_finalized_reimbursement( &mut self, index: ReimbursementIndex, @@ -1135,6 +1164,7 @@ impl WithdrawalTransactions { None ); } + /// Arm the reimbursement for a withdrawal whose transaction failed on chain. /// /// # Panics @@ -1161,11 +1191,13 @@ impl WithdrawalTransactions { "BUG: reimbursement request for withdrawal {index:?} already exists" ); } + fn maybe_reimburse_requests_iter(&self) -> impl Iterator { self.maybe_reimburse .iter() .filter_map(|index| self.pipeline.get_processed_request(index)) } + /// Whether any request is still in flight, either awaiting a transaction or a reimbursement. pub fn oldest_incomplete_request_timestamp(&self) -> Option { self.requests_iter() @@ -1173,6 +1205,7 @@ impl WithdrawalTransactions { .flat_map(|req| req.created_at().into_iter()) .min() } + pub fn withdrawal_status( &self, parameter: &WithdrawalSearchParameter, @@ -1211,6 +1244,7 @@ impl WithdrawalTransactions { pending.chain(processed).collect() } + pub fn transaction_status(&self, burn_index: &LedgerBurnIndex) -> RetrieveEthStatus { if self .pipeline @@ -1221,6 +1255,7 @@ impl WithdrawalTransactions { } self.processed_transaction_status(burn_index).0 } + fn processed_transaction_status( &self, burn_index: &LedgerBurnIndex, From a49fb1409255c9235ec248e99aa0a38600f1256f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Mon, 24 Aug 2026 08:53:19 +0000 Subject: [PATCH 13/16] docs(cketh): resolve the TransactionPipeline doc links in request.rs Co-Authored-By: Claude Opus 5 (1M context) --- rs/ethereum/cketh/minter/src/state/transactions/request.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/request.rs b/rs/ethereum/cketh/minter/src/state/transactions/request.rs index 5f4f39960627..d2a58aea6ec7 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/request.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/request.rs @@ -1,5 +1,5 @@ -//! What it takes for a request to travel a [`TransactionPipeline`], and the minter's own -//! implementation of it. +//! What it takes for a request to travel a [`TransactionPipeline`](super::TransactionPipeline), +//! and the minter's own implementation of it. use super::{CreateTransactionError, EthWithdrawalRequest, TransactionCallData, WithdrawalRequest}; use crate::lifecycle::EthereumNetwork; @@ -7,7 +7,7 @@ use crate::numeric::{GasAmount, LedgerBurnIndex, TransactionNonce, Wei}; use crate::tx::{Eip1559TransactionRequest, GasFeeEstimate, ResubmissionStrategy}; use std::fmt; -/// A request that can flow through a [`TransactionPipeline`]: it carries an identity used as the +/// A request that can flow through a `TransactionPipeline`: it carries an identity used as the /// pipeline's alternate map key, and knows the EIP-1559 transaction it turns into. /// /// Implemented so far only by [`WithdrawalRequest`], the minter's main-address pipeline From 4545f028892687d91c7dd107f609fd426808d630 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Mon, 24 Aug 2026 08:53:40 +0000 Subject: [PATCH 14/16] refactor(cketh): keep the transaction pipeline inside crate::state Co-Authored-By: Claude Opus 5 (1M context) --- rs/ethereum/cketh/minter/src/state/transactions/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 4dc42fd89632..6a960f46c2f4 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -386,7 +386,7 @@ impl fmt::Debug for Erc20WithdrawalRequest { /// discarded. Paying the requester back is not the pipeline's concern — see /// [`WithdrawalTransactions`]. #[derive(Clone, Eq, PartialEq, Debug)] -pub struct TransactionPipeline { +pub(in crate::state) struct TransactionPipeline { pending_requests: VecDeque, // Processed requests (transaction created, sent, or finalized). processed_requests: BTreeMap, @@ -397,7 +397,7 @@ pub struct TransactionPipeline { } /// The pipeline sending from the minter's main address, on which user withdrawals travel. -pub type MinterTransactionPipeline = TransactionPipeline; +pub(in crate::state) type MinterTransactionPipeline = TransactionPipeline; #[derive(Clone, Eq, PartialEq, Debug)] pub enum CreateTransactionError { @@ -421,7 +421,7 @@ pub enum ResubmitTransactionError { /// How far a transaction has got through the pipeline. Carries the transaction itself, since /// every caller that asks the stage also wants the transaction at it. #[derive(Clone, Eq, PartialEq, Debug)] -pub enum TransactionStage<'a> { +pub(in crate::state) enum TransactionStage<'a> { Created(&'a Eip1559TransactionRequest), /// The most recently sent transaction, i.e. the one with the highest fee. Sent(&'a SignedTransactionRequest), From 0c1ac3285a6cfa476747e91f4cf46688af99fd38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Mon, 24 Aug 2026 08:54:03 +0000 Subject: [PATCH 15/16] refactor(cketh): drop the never-called PipelineRequest::created_at 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) --- rs/ethereum/cketh/minter/src/state/transactions/request.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/request.rs b/rs/ethereum/cketh/minter/src/state/transactions/request.rs index d2a58aea6ec7..14d8780e6837 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/request.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/request.rs @@ -24,9 +24,6 @@ pub trait PipelineRequest { /// The identity of this request, used as the pipeline's alternate map key. fn id(&self) -> Self::Id; - /// IC time at which the request was created, if tracked. - fn created_at(&self) -> Option; - /// The fee-bump strategy for this request's resubmitted transactions. fn resubmission_strategy(&self) -> ResubmissionStrategy; @@ -55,10 +52,6 @@ impl PipelineRequest for WithdrawalRequest { self.cketh_ledger_burn_index() } - fn created_at(&self) -> Option { - WithdrawalRequest::created_at(self) - } - fn resubmission_strategy(&self) -> ResubmissionStrategy { match self { WithdrawalRequest::CkEth(cketh) | WithdrawalRequest::SweeperFunding(cketh) => { From daafc748fbc6d9a586f65914ac5fa0ef200f2e58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Demay?= Date: Mon, 24 Aug 2026 08:54:28 +0000 Subject: [PATCH 16/16] refactor(cketh): reschedule a request by id rather than by value 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) --- rs/ethereum/cketh/minter/src/state/transactions/mod.rs | 7 +++---- rs/ethereum/cketh/minter/src/state/transactions/tests.rs | 6 +++--- rs/ethereum/cketh/minter/src/withdraw.rs | 5 ++++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs index 6a960f46c2f4..02f65dff6757 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/mod.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/mod.rs @@ -466,8 +466,7 @@ impl TransactionPipeline { } /// Move an existing request to the back of the queue. - pub fn reschedule_request>(&mut self, request: Req) { - let id = request.into().id(); + pub fn reschedule_request(&mut self, id: R::Id) { assert_eq!( self.pending_requests .iter() @@ -982,8 +981,8 @@ impl WithdrawalTransactions { self.pipeline.record_request(request) } - pub fn reschedule_request>(&mut self, request: Req) { - self.pipeline.reschedule_request(request) + pub fn reschedule_request(&mut self, id: LedgerBurnIndex) { + self.pipeline.reschedule_request(id) } pub fn record_signed_transaction( diff --git a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs index 5ab26736b0ed..19ba2fe7d67e 100644 --- a/rs/ethereum/cketh/minter/src/state/transactions/tests.rs +++ b/rs/ethereum/cketh/minter/src/state/transactions/tests.rs @@ -265,7 +265,7 @@ mod withdrawal_transactions { ] ); - transactions.reschedule_request(first_request.clone()); + transactions.reschedule_request(first_request.cketh_ledger_burn_index()); // 1 -> 3 -> 2 assert_eq!( transactions.requests_batch(5), @@ -276,7 +276,7 @@ mod withdrawal_transactions { ] ); - transactions.reschedule_request(second_request.clone()); + transactions.reschedule_request(second_request.cketh_ledger_burn_index()); // 2 -> 1 -> 3 assert_eq!( transactions.requests_batch(5), @@ -287,7 +287,7 @@ mod withdrawal_transactions { ] ); - transactions.reschedule_request(third_request.clone()); + transactions.reschedule_request(third_request.cketh_ledger_burn_index()); // 3 -> 2 -> 1 assert_eq!( transactions.requests_batch(5), diff --git a/rs/ethereum/cketh/minter/src/withdraw.rs b/rs/ethereum/cketh/minter/src/withdraw.rs index 75c13f37db8e..471692a369e5 100644 --- a/rs/ethereum/cketh/minter/src/withdraw.rs +++ b/rs/ethereum/cketh/minter/src/withdraw.rs @@ -288,7 +288,10 @@ fn create_transactions_batch(gas_fee_estimate: GasFeeEstimate) { INFO, "[create_transactions_batch]: Withdrawal request with burn index {ledger_burn_index} has insufficient amount {withdrawal_amount:?} to cover transaction fees: {max_transaction_fee:?}. Request moved back to end of queue." ); - mutate_state(|s| s.withdrawal_transactions.reschedule_request(request)); + mutate_state(|s| { + s.withdrawal_transactions + .reschedule_request(ledger_burn_index) + }); } }; }