From 22b6ddd7544f9a8da8d5069b28feb7901f7d6fde Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 29 Aug 2023 16:32:18 -0600 Subject: [PATCH] Apply suggestions from code review Co-authored-by: str4d --- zcash_client_backend/src/data_api.rs | 11 +++++ zcash_client_sqlite/src/testing.rs | 52 +++++++++------------- zcash_client_sqlite/src/wallet/init.rs | 17 ++++--- zcash_client_sqlite/src/wallet/sapling.rs | 6 +-- zcash_client_sqlite/src/wallet/scanning.rs | 2 +- 5 files changed, 45 insertions(+), 43 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 10b428555..cda26813b 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -466,6 +466,8 @@ impl SentTransactionOutput { } } +/// A data structure used to set the birthday height for an account, and ensure that the initial +/// note commitment tree state is recorded at that height. #[derive(Clone, Debug)] pub struct AccountBirthday { height: BlockHeight, @@ -473,6 +475,12 @@ pub struct AccountBirthday { } impl AccountBirthday { + /// Constructs a new [`AccountBirthday`] from its constituent parts. + /// + /// * `height`: The birthday height of the account. This is defined as the height of the last + /// block that is known to contain no transactions sent to addresses belonging to the account. + /// * `sapling_frontier`: The Sapling note commitment tree frontier as of the end of the block + /// at `height`. pub fn from_parts( height: BlockHeight, sapling_frontier: Frontier, @@ -483,10 +491,13 @@ impl AccountBirthday { } } + /// Returns the Sapling note commitment tree frontier as of the end of the block at + /// [`Self::height`]. pub fn sapling_frontier(&self) -> &Frontier { &self.sapling_frontier } + /// Returns the birthday height of the account. pub fn height(&self) -> BlockHeight { self.height } diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index e74bcd976..b98e546ec 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -136,7 +136,6 @@ impl TestBuilder { }; TestState { - params: self.network, cache: self.cache, latest_cached_block: None, _data_file: data_file, @@ -148,7 +147,6 @@ impl TestBuilder { /// The state for a `zcash_client_sqlite` test. pub(crate) struct TestState { - params: Network, cache: Cache, latest_cached_block: Option<(BlockHeight, BlockHash, u32)>, _data_file: NamedTempFile, @@ -177,15 +175,7 @@ where let (height, prev_hash, initial_sapling_tree_size) = self .latest_cached_block .map(|(prev_height, prev_hash, end_size)| (prev_height + 1, prev_hash, end_size)) - .unwrap_or_else(|| { - ( - self.params - .activation_height(NetworkUpgrade::Sapling) - .unwrap(), - BlockHash([0; 32]), - 0, - ) - }); + .unwrap_or_else(|| (self.sapling_activation_height(), BlockHash([0; 32]), 0)); let (res, nf) = self.generate_block_at( height, @@ -214,7 +204,7 @@ where initial_sapling_tree_size: u32, ) -> (Cache::InsertResult, Nullifier) { let (cb, nf) = fake_compact_block( - &self.params, + &self.network(), height, prev_hash, dfvk, @@ -246,18 +236,10 @@ where let (height, prev_hash, initial_sapling_tree_size) = self .latest_cached_block .map(|(prev_height, prev_hash, end_size)| (prev_height + 1, prev_hash, end_size)) - .unwrap_or_else(|| { - ( - self.params - .activation_height(NetworkUpgrade::Sapling) - .unwrap(), - BlockHash([0; 32]), - 0, - ) - }); + .unwrap_or_else(|| (self.sapling_activation_height(), BlockHash([0; 32]), 0)); let cb = fake_compact_block_spending( - &self.params, + &self.network(), height, prev_hash, note, @@ -296,7 +278,7 @@ where >, > { scan_cached_blocks( - &self.params, + &self.network(), self.cache.block_source(), &mut self.db_data, from_height, @@ -316,9 +298,9 @@ impl TestState { &mut self.db_data } - /// Exposes an immutable reference to the network in use. - pub(crate) fn network(&self) -> &Network { - &self.db_data.params + /// Exposes the network in use. + pub(crate) fn network(&self) -> Network { + self.db_data.params } /// Convenience method for obtaining the Sapling activation height for the network under test. @@ -362,9 +344,10 @@ impl TestState { ReceivedNoteId, >, > { + let params = self.network(); create_spend_to_address( &mut self.db_data, - &self.params, + ¶ms, test_prover(), usk, to, @@ -397,9 +380,10 @@ impl TestState { where InputsT: InputSelector>, { + let params = self.network(); spend( &mut self.db_data, - &self.params, + ¶ms, test_prover(), input_selector, usk, @@ -430,9 +414,10 @@ impl TestState { where InputsT: InputSelector>, { + let params = self.network(); propose_transfer::<_, _, _, Infallible>( &mut self.db_data, - &self.params, + ¶ms, spend_from_account, input_selector, request, @@ -462,9 +447,10 @@ impl TestState { where InputsT: InputSelector>, { + let params = self.network(); propose_shielding::<_, _, _, Infallible>( &mut self.db_data, - &self.params, + ¶ms, input_selector, shielding_threshold, from_addrs, @@ -493,9 +479,10 @@ impl TestState { where FeeRuleT: FeeRule, { + let params = self.network(); create_proposed_transaction::<_, _, Infallible, _>( &mut self.db_data, - &self.params, + ¶ms, test_prover(), usk, ovk_policy, @@ -529,9 +516,10 @@ impl TestState { where InputsT: InputSelector>, { + let params = self.network(); shield_transparent_funds( &mut self.db_data, - &self.params, + ¶ms, test_prover(), input_selector, shielding_threshold, diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 354016fe4..5856970ae 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -391,7 +391,10 @@ mod tests { zip32::sapling::ExtendedFullViewingKey, }; - use crate::{error::SqliteClientError, wallet::scanning::priority_code, AccountId, WalletDb}; + use crate::{ + error::SqliteClientError, testing::TestBuilder, wallet::scanning::priority_code, AccountId, + WalletDb, + }; use super::{init_accounts_table, init_blocks_table, init_wallet_db}; @@ -405,9 +408,7 @@ mod tests { #[test] fn verify_schema() { - let data_file = NamedTempFile::new().unwrap(); - let mut db_data = WalletDb::for_path(data_file.path(), Network::TestNetwork).unwrap(); - init_wallet_db(&mut db_data, None).unwrap(); + let st = TestBuilder::new().build(); use regex::Regex; let re = Regex::new(r"\s+").unwrap(); @@ -560,7 +561,8 @@ mod tests { )", ]; - let mut tables_query = db_data + let mut tables_query = st + .wallet() .conn .prepare("SELECT sql FROM sqlite_schema WHERE type = 'table' ORDER BY tbl_name") .unwrap(); @@ -600,7 +602,7 @@ mod tests { AND (scan_queue.block_range_end - 1) >= shard.subtree_end_height ) WHERE scan_queue.priority > {}", - u32::from(db_data.params.activation_height(NetworkUpgrade::Sapling).unwrap()), + u32::from(st.network().activation_height(NetworkUpgrade::Sapling).unwrap()), priority_code(&ScanPriority::Scanned), ), // v_transactions @@ -741,7 +743,8 @@ mod tests { OR sapling_received_notes.is_change = 0".to_owned(), ]; - let mut views_query = db_data + let mut views_query = st + .wallet() .conn .prepare("SELECT sql FROM sqlite_schema WHERE type = 'view' ORDER BY tbl_name") .unwrap(); diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index db1d126c7..9c254c789 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -532,7 +532,7 @@ pub(crate) mod tests { let ufvks = [(account, usk.to_unified_full_viewing_key())] .into_iter() .collect(); - let decrypted_outputs = decrypt_transaction(st.network(), h + 1, &tx, &ufvks); + let decrypted_outputs = decrypt_transaction(&st.network(), h + 1, &tx, &ufvks); assert_eq!(decrypted_outputs.len(), 2); let mut found_tx_change_memo = false; @@ -619,7 +619,7 @@ pub(crate) mod tests { // Create a USK that doesn't exist in the wallet let acct1 = AccountId::from(1); - let usk1 = UnifiedSpendingKey::from_seed(st.network(), &[1u8; 32], acct1).unwrap(); + let usk1 = UnifiedSpendingKey::from_seed(&st.network(), &[1u8; 32], acct1).unwrap(); // Attempting to spend with a USK that is not in the wallet results in an error assert_matches!( @@ -934,7 +934,7 @@ pub(crate) mod tests { for output in tx.sapling_bundle().unwrap().shielded_outputs() { // Find the output that decrypts with the external OVK let result = try_sapling_output_recovery( - st.network(), + &st.network(), h1, &dfvk.to_ovk(Scope::External), output, diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index ff1d91c85..e7d8c7668 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -1200,7 +1200,7 @@ mod tests { fn create_account_creates_ignored_range() { use ScanPriority::*; - let mut st = TestBuilder::new().with_block_cache().build(); + let mut st = TestBuilder::new().build(); let sap_active = st.sapling_activation_height();