Apply suggestions from code review.

Co-authored by: Jack Grig <jack@electriccoin.co>
Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>

components/zcash_protocol/src/value.rs

@@ -321,7 +321,8 @@ impl Zatoshis {
     /// Divides this `Zatoshis` value by the given divisor and returns the quotient and remainder.
     pub fn div_with_remainder(&self, divisor: NonZeroU64) -> QuotRem<Zatoshis> {
         let divisor = u64::from(divisor);
-        // `self` is already bounds-checked, so we don't need to re-check it in division
+        // `self` is already bounds-checked, and both the quotient and remainder
+        // are <= self, so we don't need to re-check them in division.
         QuotRem {
             quotient: Zatoshis(self.0 / divisor),
             remainder: Zatoshis(self.0 % divisor),
@@ -427,7 +428,8 @@ impl Div<NonZeroU64> for Zatoshis {
     type Output = Zatoshis;
 
     fn div(self, rhs: NonZeroU64) -> Zatoshis {
-        // `self` is already bounds-checked, so we don't need to re-check it
+        // `self` is already bounds-checked and the quotient is <= self, so
+        // we don't need to re-check it
         Zatoshis(self.0 / u64::from(rhs))
     }
 }

zcash_client_backend/src/fees.rs

@@ -368,6 +368,8 @@ impl SplitPolicy {
     /// In the case that no other conditions provided by the user are available to fall back on,
     /// a default value of [`MARGINAL_FEE`] * 100 will be used as the "minimum usable note value"
     /// when retrieving wallet metadata.
+    ///
+    /// [`MARGINAL_FEE`]: zcash_primitives::transaction::fees::zip317::MARGINAL_FEE
     pub(crate) const MIN_NOTE_VALUE: NonNegativeAmount = NonNegativeAmount::const_from_u64(500000);
 
     /// Constructs a new [`SplitPolicy`] that splits change to ensure the given number of spendable
@@ -404,9 +406,9 @@ impl SplitPolicy {
     /// Returns the number of output notes to produce from the given total change value, given the
     /// total value and number of existing unspent notes in the account and this policy.
     ///
-    /// If splitting change to produce [`Self::target_output_count`] would result in notes of value less than
-    /// [`Self::min_split_output_value`], then this will produce
-    ///
+    /// If splitting change to produce [`Self::target_output_count`] would result in notes of value
+    /// less than [`Self::min_split_output_value`], then this will suggest a smaller number of
+    /// splits so that each resulting change note has sufficient value.
     pub fn split_count(
         &self,
         existing_notes: Option<usize>,

zcash_client_sqlite/CHANGELOG.md

@@ -15,7 +15,7 @@ and this library adheres to Rust's notion of
 - MSRV is now 1.77.0.
 - Migrated from `schemer` to our fork `schemerz`.
 - Migrated to `rusqlite 0.32`.
-- `error::SqliteClientError` has additional variant `NoteSelectorInvalid`
+- `error::SqliteClientError` has additional variant `NoteFilterInvalid`
 
 ## [0.12.2] - 2024-10-21
 

zcash_client_sqlite/src/error.rs

@@ -191,7 +191,7 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::ChainHeightUnknown => write!(f, "Chain height unknown; please call `update_chain_tip`"),
             SqliteClientError::UnsupportedPoolType(t) => write!(f, "Pool type is not currently supported: {}", t),
             SqliteClientError::BalanceError(e) => write!(f, "Balance error: {}", e),
-            SqliteClientError::NoteFilterInvalid(s) => write!(f, "Could not evaluate selection query: {:?}", s),
+            SqliteClientError::NoteFilterInvalid(s) => write!(f, "Could not evaluate filter query: {:?}", s),
             #[cfg(feature = "transparent-inputs")]
             SqliteClientError::ReachedGapLimit(account_id, bad_index) => write!(f,
                 "The proposal cannot be constructed until transactions with previously reserved ephemeral address outputs have been mined. \

zcash_client_sqlite/src/wallet/common.rs

@@ -241,7 +241,7 @@ pub(crate) fn spendable_notes_meta(
     protocol: ShieldedProtocol,
     chain_tip_height: BlockHeight,
     account: AccountId,
-    selector: &NoteFilter,
+    filter: &NoteFilter,
     exclude: &[ReceivedNoteId],
 ) -> Result<Option<PoolMeta>, SqliteClientError> {
     let (table_prefix, _, _) = per_protocol_names(protocol);
@@ -306,10 +306,10 @@ pub(crate) fn spendable_notes_meta(
         conn: &rusqlite::Connection,
         table_prefix: &str,
         account: AccountId,
-        selector: &NoteFilter,
+        filter: &NoteFilter,
         chain_tip_height: BlockHeight,
     ) -> Result<Option<NonNegativeAmount>, SqliteClientError> {
-        match selector {
+        match filter {
             NoteFilter::ExceedsMinValue(v) => Ok(Some(*v)),
             NoteFilter::ExceedsPriorSendPercentile(n) => {
                 let mut bucket_query = conn.prepare(
@@ -351,27 +351,24 @@ pub(crate) fn spendable_notes_meta(
             }
             NoteFilter::ExceedsBalancePercentage(p) => {
                 let balance = conn.query_row_and_then::<_, SqliteClientError, _, _>(
-                    &format!(
-                        "SELECT SUM(rn.value)
-                         FROM v_received_outputs rn
-                         INNER JOIN accounts a ON a.id = rn.account_id
-                         INNER JOIN transactions ON transactions.id_tx = rn.transaction_id
-                         WHERE rn.account_id = :account_id
-                         AND a.ufvk IS NOT NULL
-                         AND transactions.mined_height IS NOT NULL
-                         AND rn.pool != :transparent_pool
-                         AND rn.id_within_pool_table NOT IN (
-                           SELECT rns.received_output_id
-                           FROM v_received_output_spends rns
-                           JOIN transactions stx ON stx.id_tx = rns.transaction_id
-                           WHERE rns.pool == rn.pool
-                           AND (
-                               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 > :chain_tip_height -- the spending tx is unexpired
-                           )
-                         )"
-                    ),
+                    "SELECT SUM(rn.value)
+                     FROM v_received_outputs rn
+                     INNER JOIN accounts a ON a.id = rn.account_id
+                     INNER JOIN transactions ON transactions.id_tx = rn.transaction_id
+                     WHERE rn.account_id = :account_id
+                     AND a.ufvk IS NOT NULL
+                     AND transactions.mined_height IS NOT NULL
+                     AND rn.pool != :transparent_pool
+                     AND (rn.pool, rn.id_within_pool_table) NOT IN (
+                       SELECT rns.pool, rns.received_output_id
+                       FROM v_received_output_spends rns
+                       JOIN transactions stx ON stx.id_tx = rns.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 > :chain_tip_height -- the spending tx is unexpired
+                       )
+                     )",
                     named_params![
                         ":account_id": account.0,
                         ":chain_tip_height": u32::from(chain_tip_height),
@@ -423,8 +420,7 @@ pub(crate) fn spendable_notes_meta(
 
     // TODO: Simplify the query before executing it. Not worrying about this now because queries
     // will be developer-configured, not end-user defined.
-    if let Some(min_value) =
-        min_note_value(conn, table_prefix, account, selector, chain_tip_height)?
+    if let Some(min_value) = min_note_value(conn, table_prefix, account, filter, chain_tip_height)?
     {
         let (note_count, total_value) = run_selection(min_value)?;