From 688c36166a51d3120049fd011aff10481ba34cd3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 28 Feb 2024 18:41:30 -0700 Subject: [PATCH] Address comments from code review. --- zcash_client_backend/CHANGELOG.md | 1 - zcash_client_backend/src/data_api.rs | 50 +++++++------- zcash_client_backend/src/data_api/chain.rs | 2 +- zcash_client_backend/src/data_api/wallet.rs | 67 ++----------------- .../src/data_api/wallet/input_selection.rs | 1 - zcash_client_backend/src/scanning.rs | 6 +- zcash_client_backend/src/wallet.rs | 4 +- zcash_client_sqlite/src/lib.rs | 7 -- zcash_client_sqlite/src/wallet.rs | 24 ------- 9 files changed, 38 insertions(+), 124 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index d5554be5b..8e1d13a6f 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -56,7 +56,6 @@ and this library adheres to Rust's notion of }` - `WalletSummary::next_sapling_subtree_index` - `wallet`: - - `Account` - `propose_standard_transfer_to_address` - `create_proposed_transactions` - `input_selection`: diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 4568e49bd..8af3b4b04 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -3,12 +3,16 @@ use std::{ collections::HashMap, fmt::Debug, + hash::Hash, io, num::{NonZeroU32, TryFromIntError}, }; -use self::scanning::ScanRange; -use self::{chain::CommitmentTreeRoot, wallet::Account}; +use incrementalmerkletree::{frontier::Frontier, Retention}; +use secrecy::SecretVec; +use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; + +use self::{chain::CommitmentTreeRoot, scanning::ScanRange}; use crate::{ address::UnifiedAddress, decrypt::DecryptedOutput, @@ -17,9 +21,6 @@ use crate::{ wallet::{Note, NoteId, ReceivedNote, Recipient, WalletTransparentOutput, WalletTx}, ShieldedProtocol, }; -use incrementalmerkletree::{frontier::Frontier, Retention}; -use secrecy::SecretVec; -use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; use zcash_primitives::{ block::BlockHash, consensus::BlockHeight, @@ -289,7 +290,7 @@ impl Ratio { /// this circumstance it is possible that a newly created transaction could conflict with a /// not-yet-mined transaction in the mempool. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct WalletSummary { +pub struct WalletSummary { account_balances: HashMap, chain_tip_height: BlockHeight, fully_scanned_height: BlockHeight, @@ -297,7 +298,7 @@ pub struct WalletSummary { next_sapling_subtree_index: u64, } -impl WalletSummary { +impl WalletSummary { /// Constructs a new [`WalletSummary`] from its constituent parts. pub fn new( account_balances: HashMap, @@ -359,13 +360,17 @@ pub trait InputSource { /// The type of errors produced by a wallet backend. type Error; - /// The type used to track unique account identifiers. - type AccountId; + /// Backend-specific account identifier. + /// + /// An account identifier corresponds to at most a single unified spending key's worth of spend + /// authority, such that both received notes and change spendable by that spending authority + /// will be interpreted as belonging to that account. This might be a database identifier type + /// or a UUID. + type AccountId: Copy + Debug + Eq + Hash; /// Backend-specific note identifier. /// - /// For example, this might be a database identifier type - /// or a UUID. + /// For example, this might be a database identifier type or a UUID. type NoteRef: Copy + Debug + Eq + Ord; /// Fetches a spendable note by indexing into a transaction's shielded outputs for the @@ -426,11 +431,12 @@ pub trait WalletRead { /// The type of errors that may be generated when querying a wallet data store. type Error; - /// The type used to track unique account identifiers. - type AccountId: Eq + std::hash::Hash; - - /// Gets the parameters that went into creating an account (e.g. seed+index or uvk). - fn get_account(&self, account_id: &Self::AccountId) -> Result, Self::Error>; + /// The type of the account identifier. + /// + /// An account identifier corresponds to at most a single unified spending key's worth of spend + /// authority, such that both received notes and change spendable by that spending authority + /// will be interpreted as belonging to that account. + type AccountId: Copy + Debug + Eq + Hash; /// Returns the height of the chain as known to the wallet as of the most recent call to /// [`WalletWrite::update_chain_tip`]. @@ -1005,8 +1011,9 @@ pub trait WalletWrite: WalletRead { /// current set of [ZIP 316] account identifiers known to the wallet database. /// /// Returns the account identifier for the newly-created wallet database entry, along with the - /// associated [`UnifiedSpendingKey`]. - /// Note that the unique account identifier should *not* be assumed equivalent to the ZIP-32 account index. + /// associated [`UnifiedSpendingKey`]. Note that the unique account identifier should *not* be + /// assumed equivalent to the ZIP 32 account index. It is an opaque identifier for a pool of + /// funds or set of outputs controlled by a single spending authority. /// /// If `birthday.height()` is below the current chain tip, this operation will /// trigger a re-scan of the blocks at and above the provided height. The birthday height is @@ -1259,13 +1266,6 @@ pub mod testing { type Error = (); type AccountId = u32; - fn get_account( - &self, - _account_id: &Self::AccountId, - ) -> Result, Self::Error> { - Ok(None) - } - fn chain_height(&self) -> Result, Self::Error> { Ok(None) } diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index 996beadc6..d3283126e 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -280,7 +280,7 @@ where ParamsT: consensus::Parameters + Send + 'static, BlockSourceT: BlockSource, DbT: WalletWrite, - ::AccountId: Clone + ConditionallySelectable + Default + Send + 'static, + ::AccountId: ConditionallySelectable + Default + Send + 'static, { // Fetch the UnifiedFullViewingKeys we are tracking let ufvks = data_db diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 9269d2f3a..96806d69b 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -6,10 +6,6 @@ use sapling::{ }; use std::num::NonZeroU32; -use zcash_keys::{ - address::UnifiedAddress, - keys::{UnifiedAddressRequest, UnifiedFullViewingKey}, -}; use zcash_primitives::{ consensus::{self, BlockHeight, NetworkUpgrade}, memo::MemoBytes, @@ -21,7 +17,6 @@ use zcash_primitives::{ }, zip32::Scope, }; -use zip32::DiversifierIndex; use super::InputSource; use crate::{ @@ -53,45 +48,6 @@ use input_selection::{ GreedyInputSelector, GreedyInputSelectorError, InputSelector, InputSelectorError, }; -/// The inputs for adding an account to a wallet. -#[derive(Debug, Clone)] -pub enum Account { - /// An account that was derived from a ZIP-32 HD seed and account index. - Zip32 { - account_id: zip32::AccountId, - ufvk: UnifiedFullViewingKey, - }, - /// An account for which the seed and ZIP-32 account ID are not known. - ImportedUfvk(UnifiedFullViewingKey), -} - -impl Account { - /// Gets the default UA for the account. - pub fn default_address( - &self, - request: UnifiedAddressRequest, - ) -> (UnifiedAddress, DiversifierIndex) { - match self { - Account::Zip32 { ufvk, .. } => ufvk.default_address(request), - Account::ImportedUfvk(ufvk) => ufvk.default_address(request), - } - } - - /// Gets the unified full viewing key for this account, if available. - /// - /// Accounts initialized with an incoming viewing key will not have a unified full viewing key. - pub fn ufvk(&self) -> Option<&UnifiedFullViewingKey> { - match self { - Account::Zip32 { ufvk, .. } => Some(ufvk), - Account::ImportedUfvk(ufvk) => Some(ufvk), - } - } - - // TODO: When a UnifiedIncomingViewingKey is available, add a function that - // will return it. Even if the Account was initialized with a UFVK, we can always - // derive a UIVK from that. -} - /// Scans a [`Transaction`] for any information that can be decrypted by the accounts in /// the wallet, and saves it to the wallet. pub fn decrypt_and_store_transaction( @@ -102,7 +58,6 @@ pub fn decrypt_and_store_transaction( where ParamsT: consensus::Parameters, DbT: WalletWrite, - ::AccountId: Clone, { // Fetch the UnifiedFullViewingKeys we are tracking let ufvks = data.get_unified_full_viewing_keys()?; @@ -272,8 +227,6 @@ where AccountId = ::AccountId, >, DbT: WalletCommitmentTrees, - ::AccountId: Clone, - ::NoteRef: Copy + Eq + Ord, { let account = wallet_db .get_account_for_ufvk(&usk.to_unified_full_viewing_key()) @@ -384,8 +337,6 @@ where AccountId = ::AccountId, >, DbT: WalletCommitmentTrees, - ::AccountId: Clone, - ::NoteRef: Copy + Eq + Ord, ParamsT: consensus::Parameters + Clone, InputsT: InputSelector, { @@ -489,7 +440,7 @@ pub fn propose_standard_transfer_to_address( wallet_db: &mut DbT, params: &ParamsT, fee_rule: StandardFeeRule, - spend_from_account: ::AccountId, + spend_from_account: ::AccountId, min_confirmations: NonZeroU32, to: &Address, amount: NonNegativeAmount, @@ -512,7 +463,6 @@ where Error = ::Error, AccountId = ::AccountId, >, - ::AccountId: Clone, DbT::NoteRef: Copy + Eq + Ord, { let request = zip321::TransactionRequest::new(vec![Payment { @@ -620,7 +570,6 @@ pub fn create_proposed_transactions( > where DbT: WalletWrite + WalletCommitmentTrees, - ::AccountId: Clone, ParamsT: consensus::Parameters + Clone, FeeRuleT: FeeRule, { @@ -674,7 +623,6 @@ fn create_proposed_transaction( > where DbT: WalletWrite + WalletCommitmentTrees, - ::AccountId: Clone, ParamsT: consensus::Parameters + Clone, FeeRuleT: FeeRule, { @@ -1069,7 +1017,7 @@ where )?; sapling_output_meta.push(( Recipient::InternalAccount( - account.clone(), + account, PoolType::Shielded(ShieldedProtocol::Sapling), ), change_value.value(), @@ -1092,7 +1040,7 @@ where )?; orchard_output_meta.push(( Recipient::InternalAccount( - account.clone(), + account, PoolType::Shielded(ShieldedProtocol::Orchard), ), change_value.value(), @@ -1120,7 +1068,7 @@ where .expect("An action should exist in the transaction for each Orchard output."); let recipient = recipient - .map_internal_note(|pool| { + .map_internal_account_note(|pool| { assert!(pool == PoolType::Shielded(ShieldedProtocol::Orchard)); build_result .transaction() @@ -1131,7 +1079,7 @@ where .map(|(note, _, _)| Note::Orchard(note)) }) }) - .internal_note_transpose_option() + .internal_account_note_transpose_option() .expect("Wallet-internal outputs must be decryptable with the wallet's IVK"); SentTransactionOutput::from_parts(output_index, recipient, value, memo) @@ -1150,7 +1098,7 @@ where .expect("An output should exist in the transaction for each Sapling payment."); let recipient = recipient - .map_internal_note(|pool| { + .map_internal_account_note(|pool| { assert!(pool == PoolType::Shielded(ShieldedProtocol::Sapling)); build_result .transaction() @@ -1167,7 +1115,7 @@ where .map(|(note, _, _)| Note::Sapling(note)) }) }) - .internal_note_transpose_option() + .internal_account_note_transpose_option() .expect("Wallet-internal outputs must be decryptable with the wallet's IVK"); SentTransactionOutput::from_parts(output_index, recipient, value, memo) @@ -1269,7 +1217,6 @@ pub fn shield_transparent_funds( where ParamsT: consensus::Parameters, DbT: WalletWrite + WalletCommitmentTrees + InputSource::Error>, - ::AccountId: Clone, InputsT: ShieldingSelector, { let proposal = propose_shielding( diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs index 08848c9b5..ebf891ca1 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -314,7 +314,6 @@ impl GreedyInputSelector { impl InputSelector for GreedyInputSelector where DbT: InputSource, - ::AccountId: Clone, ChangeT: ChangeStrategy, ChangeT::FeeRule: Clone, { diff --git a/zcash_client_backend/src/scanning.rs b/zcash_client_backend/src/scanning.rs index 988f939c0..348756a61 100644 --- a/zcash_client_backend/src/scanning.rs +++ b/zcash_client_backend/src/scanning.rs @@ -252,7 +252,7 @@ impl fmt::Display for ScanError { pub fn scan_block< P: consensus::Parameters + Send + 'static, K: ScanningKey, - A: Clone + Default + Eq + Hash + Send + ConditionallySelectable + 'static, + A: Default + Eq + Hash + Send + ConditionallySelectable + 'static, >( params: &P, block: CompactBlock, @@ -283,7 +283,7 @@ pub(crate) fn add_block_to_runner( P: consensus::Parameters + Send + 'static, S: Clone + Send + 'static, T: Tasks>, - A: Clone + Default + Eq + Send + 'static, + A: Copy + Default + Eq + Send + 'static, { let block_hash = block.hash(); let block_height = block.height(); @@ -336,7 +336,7 @@ pub(crate) fn scan_block_with_runner< P: consensus::Parameters + Send + 'static, K: ScanningKey, T: Tasks> + Sync, - A: Send + Clone + Default + Eq + Hash + ConditionallySelectable + 'static, + A: Send + Default + Eq + Hash + ConditionallySelectable + 'static, >( params: &P, block: CompactBlock, diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index d33dc5565..ea0f3372c 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -73,7 +73,7 @@ pub enum Recipient { } impl Recipient { - pub fn map_internal_note B>(self, f: F) -> Recipient { + pub fn map_internal_account_note B>(self, f: F) -> Recipient { match self { Recipient::Transparent(t) => Recipient::Transparent(t), Recipient::Sapling(s) => Recipient::Sapling(s), @@ -84,7 +84,7 @@ impl Recipient { } impl Recipient> { - pub fn internal_note_transpose_option(self) -> Option> { + pub fn internal_account_note_transpose_option(self) -> Option> { match self { Recipient::Transparent(t) => Some(Recipient::Transparent(t)), Recipient::Sapling(s) => Some(Recipient::Sapling(s)), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 34ebc2074..af9c3dbe4 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -386,13 +386,6 @@ impl, P: consensus::Parameters> WalletRead for W fn get_account_ids(&self) -> Result, Self::Error> { wallet::get_account_ids(self.conn.borrow()) } - - fn get_account( - &self, - account_id: &Self::AccountId, - ) -> Result, Self::Error> { - wallet::get_account(self.conn.borrow(), &self.params, *account_id) - } } impl WalletWrite for WalletDb { diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 723b074e6..7db47d201 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -78,7 +78,6 @@ use zcash_client_backend::{ address::{Address, UnifiedAddress}, data_api::{ scanning::{ScanPriority, ScanRange}, - wallet::Account, AccountBalance, AccountBirthday, BlockMetadata, Ratio, SentTransactionOutput, WalletSummary, SAPLING_SHARD_HEIGHT, }, @@ -985,29 +984,6 @@ pub(crate) fn block_height_extrema( }) } -pub(crate) fn get_account( - conn: &rusqlite::Connection, - params: &P, - account_id: AccountId, -) -> Result, SqliteClientError> { - conn.query_row( - r#" - SELECT ufvk - FROM accounts - WHERE id = :account_id - "#, - named_params![":account_id": u32::from(account_id)], - |row| row.get::<_, String>(0), - ) - .optional()? - .map(|ufvk_bytes| { - let ufvk = UnifiedFullViewingKey::decode(params, &ufvk_bytes) - .map_err(SqliteClientError::CorruptedData)?; - Ok(Account::Zip32 { account_id, ufvk }) - }) - .transpose() -} - /// Returns the minimum and maximum heights of blocks in the chain which may be scanned. pub(crate) fn scan_queue_extrema( conn: &rusqlite::Connection,