From cc3d1be7d67499f3570e839589c45be8a01d9110 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 27 Sep 2024 18:21:24 +0000 Subject: [PATCH] Rework `WalletTest::get_confirmed_sends` trait method Closes zcash/librustzcash#1544. --- zcash_client_backend/src/data_api.rs | 33 +++++++++++++++-- .../src/data_api/testing/pool.rs | 36 ++++++++++--------- zcash_client_sqlite/src/lib.rs | 30 +++++++++++----- 3 files changed, 72 insertions(+), 27 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 72dd062a3a..40e552925e 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -67,6 +67,7 @@ use incrementalmerkletree::{frontier::Frontier, Retention}; use nonempty::NonEmpty; use secrecy::SecretVec; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; +use zcash_keys::address::Address; use zip32::fingerprint::SeedFingerprint; use self::{ @@ -1233,11 +1234,12 @@ pub trait WalletTest: InputSource + WalletRead { _protocol: ShieldedProtocol, ) -> Result, ::Error>; + /// Returns the outputs for a transaction sent by the wallet. #[allow(clippy::type_complexity)] - fn get_confirmed_sends( + fn get_sent_outputs( &self, txid: &TxId, - ) -> Result, Option, Option)>, ::Error>; + ) -> Result, ::Error>; #[allow(clippy::type_complexity)] fn get_checkpoint_history( @@ -1270,6 +1272,33 @@ pub trait WalletTest: InputSource + WalletRead { ) -> Result>, ::Error>; } +/// The output of a transaction sent by the wallet. +/// +/// This type is opaque, and exists for use by tests defined in this crate. +#[cfg(any(test, feature = "test-dependencies"))] +#[derive(Clone, Debug)] +pub struct OutputOfSentTx { + value: NonNegativeAmount, + external_recipient: Option
, + ephemeral_address: Option<(Address, u32)>, +} + +#[cfg(any(test, feature = "test-dependencies"))] +impl OutputOfSentTx { + /// Constructs an output from its test-relevant parts. + pub fn from_parts( + value: NonNegativeAmount, + external_recipient: Option
, + ephemeral_address: Option<(Address, u32)>, + ) -> Self { + Self { + value, + external_recipient, + ephemeral_address, + } + } +} + /// The relevance of a seed to a given wallet. /// /// This is the return type for [`WalletRead::seed_relevance_to_derived_accounts`]. diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index aaf70a1ae7..4d28694466 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -318,7 +318,7 @@ pub fn send_multi_step_proposed_transfer( DSF: DataStoreFactory, ::AccountId: std::fmt::Debug, { - use crate::data_api::GAP_LIMIT; + use crate::data_api::{OutputOfSentTx, GAP_LIMIT}; let mut st = TestBuilder::new() .with_data_store_factory(ds_factory) @@ -409,7 +409,7 @@ pub fn send_multi_step_proposed_transfer( // Check that there are sent outputs with the correct values. let confirmed_sent: Vec> = txids .iter() - .map(|sent_txid| st.wallet().get_confirmed_sends(sent_txid).unwrap()) + .map(|sent_txid| st.wallet().get_sent_outputs(sent_txid).unwrap()) .collect(); // Verify that a status request has been generated for the second transaction of @@ -420,21 +420,24 @@ pub fn send_multi_step_proposed_transfer( assert!(expected_step0_change < expected_ephemeral); assert_eq!(confirmed_sent.len(), 2); assert_eq!(confirmed_sent[0].len(), 2); + assert_eq!(confirmed_sent[0][0].value, expected_step0_change); + let OutputOfSentTx { + value: ephemeral_v, + external_recipient: to_addr, + ephemeral_address, + } = confirmed_sent[0][1].clone(); + assert_eq!(ephemeral_v, expected_ephemeral); + assert!(to_addr.is_some()); assert_eq!( - confirmed_sent[0][0].0, - u64::try_from(expected_step0_change).unwrap() + ephemeral_address, + to_addr.map(|addr| (addr, expected_index)), ); - let (ephemeral_v, to_addr, ephemeral_addr, index) = confirmed_sent[0][1].clone(); - assert_eq!(ephemeral_v, u64::try_from(expected_ephemeral).unwrap()); - assert!(to_addr.is_some()); - assert_eq!(ephemeral_addr, to_addr); - assert_eq!(index, Some(expected_index)); assert_eq!(confirmed_sent[1].len(), 1); assert_matches!( - confirmed_sent[1][0].clone(), - (sent_v, sent_to_addr, None, None) - if sent_v == u64::try_from(transfer_amount).unwrap() && sent_to_addr == Some(tex_addr.encode(st.network()))); + &confirmed_sent[1][0], + OutputOfSentTx { value: sent_v, external_recipient: sent_to_addr, ephemeral_address: None } + if sent_v == &transfer_amount && sent_to_addr == &Some(tex_addr)); // Check that the transaction history matches what we expect. let tx_history = st.wallet().get_tx_history().unwrap(); @@ -466,7 +469,7 @@ pub fn send_multi_step_proposed_transfer( -ZatBalance::from(expected_ephemeral), ); - (ephemeral_addr.unwrap(), txids) + (ephemeral_address.unwrap().0, txids) }; // Each transfer should use a different ephemeral address. @@ -476,9 +479,8 @@ pub fn send_multi_step_proposed_transfer( let height = add_funds(&mut st, value); - let ephemeral_taddr = Address::decode(st.network(), &ephemeral0).expect("valid address"); assert_matches!( - ephemeral_taddr, + ephemeral0, Address::Transparent(TransparentAddress::PublicKeyHash(_)) ); @@ -488,7 +490,7 @@ pub fn send_multi_step_proposed_transfer( account_id, StandardFeeRule::Zip317, NonZeroU32::new(1).unwrap(), - &ephemeral_taddr, + &ephemeral0, transfer_amount, None, None, @@ -503,7 +505,7 @@ pub fn send_multi_step_proposed_transfer( ); assert_matches!( &create_proposed_result, - Err(Error::PaysEphemeralTransparentAddress(address_str)) if address_str == &ephemeral0); + Err(Error::PaysEphemeralTransparentAddress(address_str)) if address_str == &ephemeral0.encode(st.network())); // Simulate another wallet sending to an ephemeral address with an index // within the current gap limit. The `PaysEphemeralTransparentAddress` error diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index c735aeb3a2..10da43eca3 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -99,7 +99,10 @@ use maybe_rayon::{ }; #[cfg(any(test, feature = "test-dependencies"))] -use zcash_client_backend::data_api::{testing::TransactionSummary, WalletTest}; +use { + zcash_client_backend::data_api::{testing::TransactionSummary, OutputOfSentTx, WalletTest}, + zcash_keys::address::Address, +}; /// `maybe-rayon` doesn't provide this as a fallback, so we have to. #[cfg(not(feature = "multicore"))] @@ -654,11 +657,10 @@ impl, P: consensus::Parameters> WalletTest for W Ok(note_ids) } - fn get_confirmed_sends( + fn get_sent_outputs( &self, txid: &TxId, - ) -> Result, Option, Option)>, ::Error> - { + ) -> Result, ::Error> { let mut stmt_sent = self .conn.borrow() .prepare( @@ -672,12 +674,24 @@ impl, P: consensus::Parameters> WalletTest for W let sends = stmt_sent .query_map(rusqlite::params![txid.as_ref()], |row| { - let v: u32 = row.get(0)?; - let to_address: Option = row.get(1)?; - let ephemeral_address: Option = row.get(2)?; + let v = row.get(0)?; + let to_address = row + .get::<_, Option>(1)? + .and_then(|s| Address::decode(&self.params, &s)); + let ephemeral_address = row + .get::<_, Option>(2)? + .and_then(|s| Address::decode(&self.params, &s)); let address_index: Option = row.get(3)?; - Ok((u64::from(v), to_address, ephemeral_address, address_index)) + Ok((v, to_address, ephemeral_address.zip(address_index))) })? + .map(|res| { + let (amount, external_recipient, ephemeral_address) = res?; + Ok::<_, ::Error>(OutputOfSentTx::from_parts( + NonNegativeAmount::from_u64(amount)?, + external_recipient, + ephemeral_address, + )) + }) .collect::>()?; Ok(sends)