refactor(minter): model submitted transactions as a MinterTransaction enum

minter/src/monitor/mod.rs

@@ -71,7 +71,7 @@ async fn check_submitted_transactions<R: CanisterRuntime>(runtime: &R) -> bool {
         state
             .submitted_transactions()
             .iter()
-            .map(|(sig, tx)| (*sig, tx.block_height))
+            .map(|(sig, tx)| (*sig, tx.block_height()))
             .collect()
     });
     if all_transactions.is_empty() {
@@ -162,8 +162,8 @@ pub async fn resubmit_transactions<R: CanisterRuntime>(runtime: R) {
             .map(|(sig, tx)| {
                 (
                     *sig,
-                    tx.message.clone(),
-                    tx.signers
+                    tx.message().clone(),
+                    tx.signers()
                         .iter()
                         .map(Signer::derivation_path)
                         .collect::<Vec<DerivationPath>>(),

minter/src/monitor/tests.rs

@@ -400,7 +400,7 @@ mod resubmission {
         read_state(|s| {
             assert_eq!(s.submitted_transactions().len(), 1);
             let resubmitted = s.submitted_transactions().get(&new_signature).unwrap();
-            assert_eq!(resubmitted.block_height, RESUBMISSION_BLOCK_HEIGHT);
+            assert_eq!(resubmitted.block_height(), RESUBMISSION_BLOCK_HEIGHT);
         });
     }
 

minter/src/sol_transfer/mod.rs

@@ -116,17 +116,21 @@ async fn sign_transaction(
     signer_derivation_paths: impl IntoIterator<Item = DerivationPath>,
     signer: &impl SchnorrSigner,
 ) -> Result<(), CreateTransferError> {
-    let message_bytes = transaction.message_data();
-    let message_len = message_bytes.len();
-    transaction.signatures = sign_bytes(signer_derivation_paths, signer, message_bytes).await?;
+    ensure_within_transaction_size(&transaction.message)?;
+    transaction.signatures =
+        sign_bytes(signer_derivation_paths, signer, transaction.message_data()).await?;
+    Ok(())
+}
 
-    let tx_size = 1 + message_len + transaction.signatures.len() * BYTES_PER_SIGNATURE;
+fn ensure_within_transaction_size(message: &Message) -> Result<(), CreateTransferError> {
+    let tx_size = 1
+        + message.serialize().len()
+        + message.header.num_required_signatures as usize * BYTES_PER_SIGNATURE;
     if tx_size > MAX_TX_SIZE {
         return Err(CreateTransferError::TransactionTooLarge {
             max: MAX_TX_SIZE,
             got: tx_size,
         });
     }
-
     Ok(())
 }

minter/src/sol_transfer/tests.rs

@@ -276,9 +276,12 @@ mod batch_withdrawal_tests {
             })
             .collect();
 
-        let result =
-            create_signed_batch_withdrawal_transaction(&minter_signing_once(), &targets, blockhash)
-                .await;
+        let result = create_signed_batch_withdrawal_transaction(
+            &TestCanisterRuntime::new(),
+            &targets,
+            blockhash,
+        )
+        .await;
 
         assert_matches!(
             result,

minter/src/state/mod.rs

@@ -111,10 +111,10 @@ pub struct State {
     sent_withdrawal_requests: BTreeMap<LedgerBurnIndex, SentWithdrawalRequest>,
     successful_withdrawal_requests: BTreeMap<LedgerBurnIndex, SentWithdrawalRequest>,
     failed_withdrawal_requests: BTreeMap<LedgerBurnIndex, SentWithdrawalRequest>,
-    submitted_transactions: InsertionOrderedMap<Signature, SolanaTransaction>,
-    transactions_to_resubmit: InsertionOrderedMap<Signature, SolanaTransaction>,
+    submitted_transactions: InsertionOrderedMap<Signature, MinterTransaction>,
+    transactions_to_resubmit: InsertionOrderedMap<Signature, MinterTransaction>,
     succeeded_transactions: BTreeSet<Signature>,
-    failed_transactions: InsertionOrderedMap<Signature, SolanaTransaction>,
+    failed_transactions: InsertionOrderedMap<Signature, MinterTransaction>,
     nonce_pool: DurableNoncePool,
     active_tasks: BTreeSet<TaskType>,
     balance: Lamport,
@@ -196,11 +196,11 @@ impl State {
         &self.failed_withdrawal_requests
     }
 
-    pub fn submitted_transactions(&self) -> &InsertionOrderedMap<Signature, SolanaTransaction> {
+    pub fn submitted_transactions(&self) -> &InsertionOrderedMap<Signature, MinterTransaction> {
         &self.submitted_transactions
     }
 
-    pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap<Signature, SolanaTransaction> {
+    pub fn transactions_to_resubmit(&self) -> &InsertionOrderedMap<Signature, MinterTransaction> {
         &self.transactions_to_resubmit
     }
 
@@ -219,23 +219,22 @@ impl State {
             .unwrap_or_else(|| {
                 panic!("BUG: cannot mark non-submitted transaction {signature} for resubmission")
             });
-        if let TransactionPurpose::SweepDeposits { .. } = &transaction.purpose {
-            self.deposits.drop_swept(signature);
-            return;
+        match transaction {
+            MinterTransaction::SweepDeposit { .. } => self.deposits.drop_swept(signature),
+            MinterTransaction::Withdrawal { .. } => assert!(
+                self.transactions_to_resubmit
+                    .insert(*signature, transaction)
+                    .is_none(),
+                "BUG: transaction {signature} is already queued for resubmission"
+            ),
         }
-        assert!(
-            self.transactions_to_resubmit
-                .insert(*signature, transaction)
-                .is_none(),
-            "BUG: transaction {signature} is already queued for resubmission"
-        );
     }
 
     pub fn succeeded_transactions(&self) -> &BTreeSet<Signature> {
         &self.succeeded_transactions
     }
 
-    pub fn failed_transactions(&self) -> &InsertionOrderedMap<Signature, SolanaTransaction> {
+    pub fn failed_transactions(&self) -> &InsertionOrderedMap<Signature, MinterTransaction> {
         &self.failed_transactions
     }
 
@@ -532,7 +531,9 @@ impl State {
             !self.failed_transactions.contains_key(signature),
             "Attempted to submit already failed transaction {signature:?}"
         );
-        let amount = match purpose {
+        let message = transaction.clone();
+        let signers = signers.to_vec();
+        let submitted_transaction = match purpose {
             TransactionPurpose::WithdrawSol { burn_indices } => {
                 let mut total: Lamport = 0;
                 for burn_index in burn_indices {
@@ -561,27 +562,28 @@ impl State {
                     .balance
                     .checked_sub(total + tx_fee)
                     .expect("BUG: insufficient minter balance for withdrawal");
-                total
+                MinterTransaction::Withdrawal {
+                    message,
+                    signers,
+                    block_height,
+                }
             }
             TransactionPurpose::SweepDeposits { deposit_ids } => {
                 let sweep_destination = minter_address(self.minter_public_key.as_ref().expect(
                     "BUG: a sweep was submitted before the minter public key was recorded",
                 ));
                 self.deposits
-                    .sweep(deposit_ids, sweep_destination, transaction, signature)
+                    .sweep(deposit_ids, sweep_destination, transaction, signature);
+                MinterTransaction::SweepDeposit {
+                    message,
+                    signers,
+                    block_height,
+                }
             }
         };
         assert_eq!(
-            self.submitted_transactions.insert(
-                *signature,
-                SolanaTransaction {
-                    message: transaction.clone(),
-                    signers: signers.to_vec(),
-                    block_height,
-                    purpose: purpose.clone(),
-                    amount,
-                }
-            ),
+            self.submitted_transactions
+                .insert(*signature, submitted_transaction),
             None,
             "Attempted to submit transaction with signature {signature:?} twice"
         );
@@ -599,13 +601,20 @@ impl State {
             .unwrap_or_else(|| {
                 panic!("Attempted to resubmit unknown transaction with signature {old_signature:?}")
             });
-        assert!(
-            !matches!(
-                old_transaction.purpose,
-                TransactionPurpose::SweepDeposits { .. }
+        let new_transaction = match old_transaction {
+            MinterTransaction::SweepDeposit { .. } => panic!(
+                "BUG: sweep transaction {old_signature} must be dropped instead of resubmitted"
             ),
-            "BUG: sweep transaction {old_signature} must be dropped instead of resubmitted"
-        );
+            MinterTransaction::Withdrawal {
+                message,
+                signers,
+                block_height: _,
+            } => MinterTransaction::Withdrawal {
+                message,
+                signers,
+                block_height: new_block_height,
+            },
+        };
         assert!(
             !self.succeeded_transactions.contains(new_signature),
             "Attempted to resubmit with signature {new_signature:?} that already succeeded"
@@ -614,10 +623,6 @@ impl State {
             !self.failed_transactions.contains_key(new_signature),
             "Attempted to resubmit with signature {new_signature:?} that already failed"
         );
-        let new_transaction = SolanaTransaction {
-            block_height: new_block_height,
-            ..old_transaction
-        };
         assert_eq!(
             self.submitted_transactions
                 .insert(*new_signature, new_transaction),
@@ -642,9 +647,9 @@ impl State {
             .unwrap_or_else(|| {
                 panic!("Attempted to mark unknown transaction {signature:?} as succeeded")
             });
-        match &transaction.purpose {
-            TransactionPurpose::WithdrawSol { .. } => {}
-            TransactionPurpose::SweepDeposits { .. } => self.deposits.finalize_swept(signature),
+        match transaction {
+            MinterTransaction::SweepDeposit { .. } => self.deposits.finalize_swept(signature),
+            MinterTransaction::Withdrawal { .. } => {}
         }
         assert!(
             !self.transactions_to_resubmit.contains_key(signature),
@@ -676,8 +681,9 @@ impl State {
             !self.transactions_to_resubmit.contains_key(signature),
             "BUG: transaction {signature} is queued for resubmission but is being marked as failed"
         );
-        if let TransactionPurpose::SweepDeposits { .. } = &transaction.purpose {
-            self.deposits.drop_swept(signature);
+        match transaction {
+            MinterTransaction::SweepDeposit { .. } => self.deposits.drop_swept(signature),
+            MinterTransaction::Withdrawal { .. } => {}
         }
         assert_eq!(
             self.failed_transactions.insert(*signature, transaction),
@@ -861,12 +867,40 @@ pub enum TaskType {
 }
 
 #[derive(Clone, Debug, PartialEq, Eq)]
-pub struct SolanaTransaction {
-    pub message: VersionedMessage,
-    pub signers: Vec<Signer>,
-    /// The block height of the block whose blockhash the transaction uses.
-    pub block_height: BlockHeight,
-    pub purpose: TransactionPurpose,
-    /// Total transfer amount in lamports (excluding fees).
-    pub amount: Lamport,
+pub enum MinterTransaction {
+    SweepDeposit {
+        message: VersionedMessage,
+        signers: Vec<Signer>,
+        /// The block height of the block whose blockhash the transaction uses.
+        block_height: BlockHeight,
+    },
+    Withdrawal {
+        message: VersionedMessage,
+        signers: Vec<Signer>,
+        /// The block height of the block whose blockhash the transaction uses.
+        block_height: BlockHeight,
+    },
+}
+
+impl MinterTransaction {
+    pub fn message(&self) -> &VersionedMessage {
+        match self {
+            MinterTransaction::SweepDeposit { message, .. }
+            | MinterTransaction::Withdrawal { message, .. } => message,
+        }
+    }
+
+    pub fn signers(&self) -> &[Signer] {
+        match self {
+            MinterTransaction::SweepDeposit { signers, .. }
+            | MinterTransaction::Withdrawal { signers, .. } => signers,
+        }
+    }
+
+    pub fn block_height(&self) -> BlockHeight {
+        match self {
+            MinterTransaction::SweepDeposit { block_height, .. }
+            | MinterTransaction::Withdrawal { block_height, .. } => *block_height,
+        }
+    }
 }

minter/src/state/tests.rs

@@ -167,22 +167,16 @@ mod swept_deposits {
                 Some(&planned_sweep([(0, first), (2, third)]))
             );
             assert_eq!(s.deposits().queued().keys().collect::<Vec<_>>(), vec![&1]);
-            let transaction = s.submitted_transactions().get(&sweep_signature).unwrap();
-            assert_eq!(
-                transaction.amount,
-                first.sweepable_amount() + third.sweepable_amount() - 2 * FEE_PER_SIGNATURE
-            );
-            assert_eq!(
-                transaction.signers,
-                vec![
-                    Signer::Account(third.account),
-                    Signer::Account(first.account)
-                ]
-            );
-            assert_eq!(
-                transaction.purpose,
-                TransactionPurpose::SweepDeposits {
-                    deposit_ids: vec![2, 0],
+            assert_matches!(
+                s.submitted_transactions().get(&sweep_signature).unwrap(),
+                MinterTransaction::SweepDeposit { signers, .. } => {
+                    assert_eq!(
+                        *signers,
+                        vec![
+                            Signer::Account(third.account),
+                            Signer::Account(first.account)
+                        ]
+                    );
                 }
             );
             assert_eq!(s.balance(), 0);

minter/src/test_fixtures/runtime.rs

@@ -35,7 +35,7 @@ pub struct TestCanisterRuntime {
     msg_cycles_accepted: Arc<Mutex<Vec<u128>>>,
     msg_cycles_available: Stubs<u128>,
     msg_cycles_refunded: Stubs<u128>,
-    set_timer_call_count: Arc<Mutex<usize>>,
+    set_timer_delays: Arc<Mutex<Vec<Duration>>>,
     schnorr_public_key_results: Stubs<Result<SchnorrPublicKeyResult, CallError>>,
     schnorr_public_key_call_count: Arc<Mutex<usize>>,
 }
@@ -145,7 +145,11 @@ impl TestCanisterRuntime {
     }
 
     pub(crate) fn set_timer_call_count(&self) -> usize {
-        *self.set_timer_call_count.lock().unwrap()
+        self.set_timer_delays().len()
+    }
+
+    pub(crate) fn set_timer_delays(&self) -> Vec<Duration> {
+        self.set_timer_delays.lock().unwrap().clone()
     }
 
     pub(crate) fn schnorr_public_key_call_count(&self) -> usize {
@@ -194,13 +198,13 @@ impl CanisterRuntime for TestCanisterRuntime {
         self.msg_cycles_refunded.next()
     }
 
-    fn set_timer<F, Fut>(&self, _delay: Duration, _f: F) -> ic_cdk_timers::TimerId
+    fn set_timer<F, Fut>(&self, delay: Duration, _f: F) -> ic_cdk_timers::TimerId
     where
         Self: Sized,
         F: FnOnce(Self) -> Fut + 'static,
         Fut: Future<Output = ()> + 'static,
     {
-        *self.set_timer_call_count.lock().unwrap() += 1;
+        self.set_timer_delays.lock().unwrap().push(delay);
         Default::default()
     }
 

minter/src/withdraw/tests.rs

@@ -4,7 +4,7 @@ use crate::{
     guard::{TimerGuard, withdrawal_guard},
     rpc::BlockHeight,
     sol_transfer::MAX_WITHDRAWALS_PER_TX,
-    state::{TaskType, event::TransactionPurpose, read_state},
+    state::{MinterTransaction, TaskType, read_state},
     test_fixtures::{
         EventsAssert, MINIMUM_WITHDRAWAL_AMOUNT, MINTER_ACCOUNT, MINTER_ADDRESS, NONCE_ACCOUNT,
         WITHDRAWAL_FEE, account, confirmed_block_at_height, events, init_balance, init_balance_to,
@@ -486,12 +486,13 @@ mod process_pending_withdrawals_tests {
         assert_matches!(withdrawal_status(1), WithdrawalStatus::TxSent { .. });
         read_state(|s| {
             let submitted = s.submitted_transactions().get(&tx_signature).unwrap();
-            assert_eq!(submitted.block_height, block_height);
+            assert_eq!(submitted.block_height(), block_height);
+            assert_matches!(submitted, MinterTransaction::Withdrawal { .. });
             assert_eq!(
-                submitted.purpose,
-                TransactionPurpose::WithdrawSol {
-                    burn_indices: vec![1_u64.into()]
-                }
+                s.sent_withdrawal_requests()
+                    .get(&1_u64.into())
+                    .map(|sent| sent.signature),
+                Some(tx_signature)
             );
         });
     }