From 664526f74a3f29630bd88e8403cdc2616f0c6ff0 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 23 Jul 2024 19:47:37 -0600 Subject: [PATCH] Fixes for nearly all PR comments --- zcash_client_backend/CHANGELOG.md | 2 +- zcash_client_backend/src/data_api.rs | 35 +++++++++++++++++++++------- zcash_client_sqlite/src/error.rs | 2 +- zcash_client_sqlite/src/lib.rs | 21 ++++++++++------- zcash_client_sqlite/src/testing.rs | 5 +++- zcash_client_sqlite/src/wallet.rs | 17 ++++---------- 6 files changed, 49 insertions(+), 33 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 3937e61c9..9f4cb9905 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -26,7 +26,6 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail ### Added - `zcash_client_backend::data_api`: - `chain::BlockCache` trait, behind the `sync` feature flag. - - `WalletWrite` trait methods `import_account_hd` and `import_account_ufvk`. - `WalletRead::get_spendable_transparent_outputs`. - `zcash_client_backend::fees`: - `EphemeralBalance` @@ -63,6 +62,7 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail methods when the "transparent-inputs" feature is enabled. - `WalletWrite` has a new `reserve_next_n_ephemeral_addresses` method when the "transparent-inputs" feature is enabled. + - `WalletWrite` has new methods `import_account_hd` and `import_account_ufvk`. - `error::Error` has new `Address` and (when the "transparent-inputs" feature is enabled) `PaysEphemeralTransparentAddress` variants. - `wallet::input_selection::InputSelectorError` has a new `Address` variant. diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 7f5aaf4c2..5df58c767 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1565,6 +1565,13 @@ pub trait WalletWrite: WalletRead { /// Tells the wallet to track the next available account-level spend authority, given the /// current set of [ZIP 316] account identifiers known to the wallet database. /// + /// The "next available account" is defined as the first unused ZIP-32 account index (counting from 0) + /// among all accounts that share the given seed. + /// When [`Self::import_account_hd`] is used to import an account with a specific index, + /// account indexes *may* become fragmented instead of one contiguous range. + /// Where fragmentation occurs, the implementations *may* choose to find the first unused + /// account index or add 1 to the highest existing account index. + /// /// 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. It is an opaque identifier for a pool of @@ -1605,13 +1612,11 @@ pub trait WalletWrite: WalletRead { /// Tells the wallet to track a specific account index for a given seed. /// - /// Returns the account 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. It is an opaque identifier for a pool of - /// funds or set of outputs controlled by a single spending authority. - /// - /// The ZIP-32 account index may be obtained by calling [`WalletRead::get_account`] - /// with the returned account identifier. + /// Returns details about the imported account, including the unique 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. 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 @@ -1631,6 +1636,14 @@ pub trait WalletWrite: WalletRead { /// By convention, wallets should only allow a new account to be generated after confirmed /// funds have been received by the currently-available account (in order to enable automated /// account recovery). + /// The [`Self::create_occount`] function helps to conform with this convention by automatically + /// selecting the next unused account index, while *this* function should be used only + /// to import an existing account with a specific index. + /// + /// Avoid fragmenting the account indexes by importing accounts with indexes that are one greater + /// than the highest existing account index. + /// + /// # Panics /// /// Panics if the length of the seed is not between 32 and 252 bytes inclusive. /// @@ -1644,7 +1657,11 @@ pub trait WalletWrite: WalletRead { /// Tells the wallet to track an account using a unified full viewing key. /// - /// Returns the account for the newly-created wallet database entry. + /// Returns details about the imported account, including the unique account + /// identifier for the newly-created wallet database entry. Unlike the other account + /// creation APIs ([`Self::create_account`] and [`Self::import_account_hd`]), no + /// spending key is returned because the wallet has no information about how the UFVK + /// was derived. /// /// 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 @@ -1661,6 +1678,8 @@ pub trait WalletWrite: WalletRead { /// were used for the wallet birthday, a transaction targeted at a height greater than the /// chain tip could be mined at a height below that tip as part of a reorg. /// + /// # Panics + /// /// Panics if the length of the seed is not between 32 and 252 bytes inclusive. fn import_account_ufvk( &mut self, diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index b15a06451..b52f7bf3a 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -171,7 +171,7 @@ impl fmt::Display for SqliteClientError { SqliteClientError::BadAccountData(e) => write!(f, "Failed to add account: {}", e), SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."), SqliteClientError::AccountIdOutOfRange => write!(f, "Wallet account identifiers must be less than 0x7FFFFFFF."), - SqliteClientError::AccountCollision(id) => write!(f, "An account with ID {} already exists in the wallet.", id.0), + SqliteClientError::AccountCollision(id) => write!(f, "An account corresponding to the data provided already exists in the wallet with internal identifier {}.", id.0), #[cfg(feature = "transparent-inputs")] SqliteClientError::AddressNotRecognized(_) => write!(f, "The address associated with a received txo is not identifiable as belonging to the wallet."), SqliteClientError::CommitmentTree(err) => write!(f, "An error occurred accessing or updating note commitment tree data: {}.", err), diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 1146880ac..977d25e6a 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -2078,7 +2078,7 @@ mod tests { pub(crate) fn import_account_hd_0() { let st = TestBuilder::new() .with_account_from_sapling_activation(BlockHash([0; 32])) - .with_account_having_index(zip32::AccountId::ZERO) + .set_account_index(zip32::AccountId::ZERO) .build(); assert_matches!( st.test_account().unwrap().account().source(), @@ -2095,23 +2095,24 @@ mod tests { ); let seed = Secret::new(vec![0u8; 32]); - let zip32_index = zip32::AccountId::ZERO.next().unwrap(); + let zip32_index_1 = zip32::AccountId::ZERO.next().unwrap(); let first = st .wallet_mut() - .import_account_hd(&seed, zip32_index, &birthday) + .import_account_hd(&seed, zip32_index_1, &birthday) .unwrap(); assert_matches!( first.0.source(), - AccountSource::Derived { seed_fingerprint: _, account_index } if account_index == zip32_index); + AccountSource::Derived { seed_fingerprint: _, account_index } if account_index == zip32_index_1); + let zip32_index_2 = zip32_index_1.next().unwrap(); let second = st .wallet_mut() - .import_account_hd(&seed, zip32_index.next().unwrap(), &birthday) + .import_account_hd(&seed, zip32_index_2, &birthday) .unwrap(); assert_matches!( second.0.source(), - AccountSource::Derived { seed_fingerprint: _, account_index } if account_index == zip32_index.next().unwrap()); + AccountSource::Derived { seed_fingerprint: _, account_index } if account_index == zip32_index_2); } #[test] @@ -2124,15 +2125,15 @@ mod tests { ); let seed = Secret::new(vec![0u8; 32]); - let zip32_index = zip32::AccountId::ZERO.next().unwrap(); + let zip32_index_1 = zip32::AccountId::ZERO.next().unwrap(); let first = st .wallet_mut() - .import_account_hd(&seed, zip32_index, &birthday) + .import_account_hd(&seed, zip32_index_1, &birthday) .unwrap(); assert_matches!( - st.wallet_mut().import_account_hd(&seed, zip32_index, &birthday), + st.wallet_mut().import_account_hd(&seed, zip32_index_1, &birthday), Err(SqliteClientError::AccountCollision(id)) if id == first.0.id()); } @@ -2158,6 +2159,8 @@ mod tests { ufvk.encode(&st.wallet().params), account.ufvk().unwrap().encode(&st.wallet().params) ); + + assert_matches!(account.source(), AccountSource::Imported); } #[test] diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index 2af48914a..517d0d47a 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -216,7 +216,10 @@ impl TestBuilder { self } - pub(crate) fn with_account_having_index(mut self, index: zip32::AccountId) -> Self { + /// Sets the [`account_index`] field for the test account + /// + /// Call either [`with_account_from_sapling_activation`] or [`with_account_having_current_birthday`] before calling this method. + pub(crate) fn set_account_index(mut self, index: zip32::AccountId) -> Self { assert!(self.account_index.is_none()); self.account_index = Some(index); self diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 27876f062..5f0d47d89 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -81,7 +81,7 @@ use zcash_address::ZcashAddress; use zcash_client_backend::{ data_api::{ scanning::{ScanPriority, ScanRange}, - AccountBalance, AccountBirthday, AccountSource, BlockMetadata, Ratio, + Account as _, AccountBalance, AccountBirthday, AccountSource, BlockMetadata, Ratio, SentTransactionOutput, WalletSummary, SAPLING_SHARD_HEIGHT, }, encoding::AddressCodec, @@ -204,13 +204,6 @@ impl Account { ) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> { self.uivk().default_address(request) } - - pub(crate) fn uivk(&self) -> UnifiedIncomingViewingKey { - match &self.viewing_key { - ViewingKey::Full(ufvk) => ufvk.to_unified_incoming_viewing_key(), - ViewingKey::Incoming(uivk) => *uivk.clone(), - } - } } impl zcash_client_backend::data_api::Account for Account { @@ -534,7 +527,9 @@ pub(crate) fn add_account( )?; } - // Always derive the default Unified Address for the account. + // Always derive the default Unified Address for the account. If the account's viewing + // key has fewer components than the wallet supports (most likely due to this being an + // imported viewing key), derive an address containing the common subset of receivers. let ua_request = account .uivk() .to_address_request() @@ -549,10 +544,6 @@ pub(crate) fn add_account( #[cfg(feature = "transparent-inputs")] transparent::ephemeral::init_account(conn, params, account_id)?; - // Initialize the `ephemeral_addresses` table. - #[cfg(feature = "transparent-inputs")] - transparent::ephemeral::init_account(conn, params, account_id)?; - Ok(account) }