Apply suggestions from code review

Co-authored-by: Jack Grigg <[email protected]>
Co-authored-by: Daira-Emma Hopwood <[email protected]>

zcash_client_backend/CHANGELOG.md

@@ -24,12 +24,12 @@ and this library adheres to Rust's notion of
     `ChangeErrT` and `NoteRefT`.
   - The following methods each now take an additional `change_strategy`
     argument, along with an associated `ChangeT` type parameter:
-    - `zcash_client_backend::data_api::wallet::spend`
-    - `zcash_client_backend::data_api::wallet::propose_transfer`
-    - `zcash_client_backend::data_api::wallet::propose_shielding`. This method
-      also now takes an additional `to_account` argument.
-    - `zcash_client_backend::data_api::wallet::shield_transparent_funds`. This
-      method also now takes an additional `to_account` argument.
+    - `wallet::spend`
+    - `wallet::propose_transfer`
+    - `wallet::propose_shielding`. This method also now takes an additional
+      `to_account` argument.
+    - `wallet::shield_transparent_funds`. This method also now takes an
+      additional `to_account` argument.
   - `wallet::input_selection::InputSelectionError` now has an additional `Change`
     variant. This necessitates the addition of two type parameters.
   - `wallet::input_selection::InputSelector::propose_transaction` takes an
@@ -49,20 +49,16 @@ and this library adheres to Rust's notion of
     has been removed, along with the additional type parameters it necessitated.
   - The arguments to `wallet::input_selection::GreedyInputSelector::new` have
     changed.
-- `zcash_client_backend::fees::ChangeStrategy` has changed. It has two new
-  associated types, `MetaSource` and `WalletMeta`, and its `FeeRule` associated
-  type now has an additional `Clone` bound. In addition, it defines a new
-  `fetch_wallet_meta` method, and the arguments to `compute_balance` have
-  changed.
-- `zcash_client_backend::fees::fixed::SingleOutputChangeStrategy::new`
-  now takes an additional `DustOutputPolicy` argument. It also now carries
-  an additional type parameter.
-- `zcash_client_backend::fees::standard::SingleOutputChangeStrategy::new`
-  now takes an additional `DustOutputPolicy` argument. It also now carries
-  an additional type parameter.
-- `zcash_client_backend::fees::zip317::SingleOutputChangeStrategy::new`
-  now takes an additional `DustOutputPolicy` argument. It also now carries
-  an additional type parameter.
+- `zcash_client_backend::fees`:
+  - `ChangeStrategy` has changed. It has two new associated types, `MetaSource`
+    and `WalletMeta`, and its `FeeRule` associated type now has an additional
+    `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and
+    the arguments to `compute_balance` have changed.
+  - The following methods now take an additional `DustOutputPolicy` argument,
+    and carry an additional type parameter:
+    - `fixed::SingleOutputChangeStrategy::new`
+    - `standard::SingleOutputChangeStrategy::new`
+    - `zip317::SingleOutputChangeStrategy::new`
 
 ### Changed
 - Migrated to `arti-client 0.22`.
@@ -71,6 +67,8 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::data_api`:
   - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have
     been removed. Use `WalletSummary::progress` instead.
+  - `testing::input_selector` use explicit `InputSelector` constructors
+    directly instead.
 - `zcash_client_backend::fees`:
   - `impl From<BalanceError> for ChangeError<...>`
 

zcash_client_backend/src/data_api.rs

@@ -848,12 +848,7 @@ impl WalletMeta {
     /// Returns the total number of unspent shielded notes belonging to the account for which this
     /// was generated.
     pub fn total_note_count(&self) -> usize {
-        #[cfg(feature = "orchard")]
-        let orchard_note_count = self.orchard_note_count;
-        #[cfg(not(feature = "orchard"))]
-        let orchard_note_count = 0;
-
-        self.sapling_note_count + orchard_note_count
+        self.sapling_note_count + self.note_count(ShieldedProtocol::Orchard)
     }
 }
 
@@ -903,8 +898,10 @@ pub trait InputSource {
 
     /// Returns metadata describing the structure of the wallet for the specified account.
     ///
-    /// The returned metadata value must exclude spent notes and unspent notes having value less
-    /// than the specified minimum value or identified in the given exclude list.
+    /// The returned metadata value must exclude:
+    /// - spent notes;
+    /// - unspent notes having value less than the specified minimum value;
+    /// - unspent notes identified in the given `exclude` list.
     fn get_wallet_metadata(
         &self,
         account: Self::AccountId,

zcash_client_backend/src/data_api/testing/pool.rs

@@ -422,7 +422,7 @@ pub fn send_with_multiple_change_outputs<T: ShieldedPoolTester>(
         .unwrap();
     assert_eq!(sent_note_ids.len(), 3);
 
-    // The sent memo should be the empty memo for the sent output, and the
+    // The sent memo should be the empty memo for the sent output, and each
     // change output's memo should be as specified.
     let mut change_memo_count = 0;
     let mut found_sent_empty_memo = false;

zcash_client_backend/src/data_api/wallet.rs

@@ -115,6 +115,8 @@ where
     Ok(())
 }
 
+/// Errors that may be generated in construction of proposals for shielded->shielded or
+/// shielded->transparent transfers.
 pub type ProposeTransferErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error<
     <DbT as WalletRead>::Error,
     CommitmentTreeErrT,
@@ -124,6 +126,8 @@ pub type ProposeTransferErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error<
     <<InputsT as InputSelector>::InputSource as InputSource>::NoteRef,
 >;
 
+/// Errors that may be generated in construction of proposals for transparent->shielded
+/// wallet-internal transfers.
 #[cfg(feature = "transparent-inputs")]
 pub type ProposeShieldingErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error<
     <DbT as WalletRead>::Error,
@@ -134,6 +138,7 @@ pub type ProposeShieldingErrT<DbT, CommitmentTreeErrT, InputsT, ChangeT> = Error
     Infallible,
 >;
 
+/// Errors that may be generated in combined creation and execution of transaction proposals.
 pub type CreateErrT<DbT, InputsErrT, FeeRuleT, ChangeErrT, N> = Error<
     <DbT as WalletRead>::Error,
     <DbT as WalletCommitmentTrees>::Error,
@@ -143,6 +148,7 @@ pub type CreateErrT<DbT, InputsErrT, FeeRuleT, ChangeErrT, N> = Error<
     N,
 >;
 
+/// Errors that may be generated in the execution of proposals that may send shielded inputs.
 pub type TransferErrT<DbT, InputsT, ChangeT> = Error<
     <DbT as WalletRead>::Error,
     <DbT as WalletCommitmentTrees>::Error,
@@ -152,6 +158,7 @@ pub type TransferErrT<DbT, InputsT, ChangeT> = Error<
     <<InputsT as InputSelector>::InputSource as InputSource>::NoteRef,
 >;
 
+/// Errors that may be generated in the execution of shielding proposals.
 #[cfg(feature = "transparent-inputs")]
 pub type ShieldErrT<DbT, InputsT, ChangeT> = Error<
     <DbT as WalletRead>::Error,

zcash_client_backend/src/fees.rs

@@ -393,13 +393,13 @@ impl SplitPolicy {
                 )
                 .unwrap(),
             );
-            if split_count > NonZeroUsize::MIN
-                && *per_output_change.quotient() < self.min_split_output_size
-            {
-                split_count = NonZeroUsize::new(usize::from(split_count) - 1)
-                    .expect("split_count checked to be > 1");
-            } else {
+            if *per_output_change.quotient() >= self.min_split_output_size {
                 return split_count;
+            } else if let Some(new_count) = NonZeroUsize::new(usize::from(split_count) - 1) {
+                split_count = new_count;
+            } else {
+                // We always create at least one change output.
+                return NonZeroUsize::MIN;
             }
         }
     }

zcash_client_backend/src/fees/common.rs

@@ -233,27 +233,20 @@ where
     )?;
 
     let change_pool = select_change_pool(&net_flows, cfg.fallback_change_pool);
+    let target_change_count = wallet_meta.map_or(1, |m| {
+        usize::from(cfg.split_policy.target_output_count)
+            .saturating_sub(m.total_note_count())
+            .max(1)
+    });
     let target_change_counts = OutputManifest {
         transparent: 0,
         sapling: if change_pool == ShieldedProtocol::Sapling {
-            wallet_meta.map_or(1, |m| {
-                std::cmp::max(
-                    usize::from(cfg.split_policy.target_output_count)
-                        .saturating_sub(m.total_note_count()),
-                    1,
-                )
-            })
+            target_change_count
         } else {
             0
         },
         orchard: if change_pool == ShieldedProtocol::Orchard {
-            wallet_meta.map_or(1, |m| {
-                std::cmp::max(
-                    usize::from(cfg.split_policy.target_output_count)
-                        .saturating_sub(m.total_note_count()),
-                    1,
-                )
-            })
+            target_change_count
         } else {
             0
         },

zcash_client_backend/src/fees/zip317.rs

@@ -139,10 +139,10 @@ impl<I> MultiOutputChangeStrategy<I> {
     /// change value is available to create notes with at least the minimum value dictated by the
     /// split policy.
     ///
-    /// `fallback_change_pool`: the pool to which change will be sent if when more than one
-    /// shielded pool is enabled via feature flags, and the transaction has no shielded inputs.
-    /// `split_policy`: A policy value describing how the change value should be returned as
-    /// multiple notes.
+    /// - `fallback_change_pool`: the pool to which change will be sent if when more than one
+    ///   shielded pool is enabled via feature flags, and the transaction has no shielded inputs.
+    /// - `split_policy`: A policy value describing how the change value should be returned as
+    ///   multiple notes.
     pub fn new(
         fee_rule: Zip317FeeRule,
         change_memo: Option<MemoBytes>,

zcash_client_sqlite/src/lib.rs

@@ -354,12 +354,16 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
         min_value: NonNegativeAmount,
         exclude: &[Self::NoteRef],
     ) -> Result<WalletMeta, Self::Error> {
+        let chain_tip_height = wallet::chain_tip_height(self.conn.borrow())?
+            .ok_or(SqliteClientError::ChainHeightUnknown)?;
+
         let sapling_note_count = count_outputs(
             self.conn.borrow(),
             account_id,
             min_value,
             exclude,
             ShieldedProtocol::Sapling,
+            chain_tip_height,
         )?;
 
         #[cfg(feature = "orchard")]
@@ -369,6 +373,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
             min_value,
             exclude,
             ShieldedProtocol::Orchard,
+            chain_tip_height,
         )?;
 
         Ok(WalletMeta::new(

zcash_client_sqlite/src/wallet/common.rs

@@ -232,6 +232,7 @@ pub(crate) fn count_outputs(
     min_value: NonNegativeAmount,
     exclude: &[ReceivedNoteId],
     protocol: ShieldedProtocol,
+    chain_tip_height: BlockHeight,
 ) -> Result<usize, rusqlite::Error> {
     let (table_prefix, _, _) = per_protocol_names(protocol);
 
@@ -265,13 +266,14 @@ pub(crate) fn count_outputs(
                JOIN transactions stx ON stx.id_tx = transaction_id
                WHERE stx.block IS NOT NULL -- the spending tx is mined
                OR stx.expiry_height IS NULL -- the spending tx will not expire
-               OR stx.expiry_height > :anchor_height -- the spending tx is unexpired
+               OR stx.expiry_height > :chain_tip_height -- the spending tx is unexpired
              )"
         ),
         named_params![
             ":account_id": account.0,
             ":min_value": u64::from(min_value),
-            ":exclude": &excluded_ptr
+            ":exclude": &excluded_ptr,
+            ":chain_tip_height": u32::from(chain_tip_height)
         ],
         |row| row.get(0),
     )