Address comments from code review.

Co-authored-by: Jack Grigg <jack@electriccoin.co>

zcash_client_backend/CHANGELOG.md

@@ -17,11 +17,10 @@ and this library adheres to Rust's notion of
   - `ScannedBlockCommitments`
   - `Balance::{add_spendable_value, add_pending_change_value, add_pending_spendable_value}`
   - `AccountBalance::{
-      with_sapling_balance_mut, 
-      with_orchard_balance_mut, 
+      with_sapling_balance_mut,
+      with_orchard_balance_mut,
       add_unshielded_value
     }`
-  - `WalletRead::get_orchard_nullifiers`
   - `wallet::propose_standard_transfer_to_address`
   - `wallet::input_selection::Proposal::{from_parts, shielded_inputs}`
   - `wallet::input_selection::ShieldedInputs`
@@ -59,7 +58,7 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::data_api::{NoteId, Recipient}` have
   been moved into the `zcash_client_backend::wallet` module.
 - `ScannedBlock::{sapling_tree_size, sapling_nullifier_map, sapling_commitments}`
-  have been moved to `ScannedBlockSapling` and in that context are now 
+  have been moved to `ScannedBlockSapling` and in that context are now
   named `{tree_size, nullifier_map, commitments}` respectively.
 
 ### Changed
@@ -70,9 +69,9 @@ and this library adheres to Rust's notion of
   - `WalletShieldedOutput` has an additional type parameter which is used for
     key scope. `WalletShieldedOutput::from_parts` now takes an additional
     argument of this type.
-  - `WalletTx` has an additional type parameter as a consequence of the 
+  - `WalletTx` has an additional type parameter as a consequence of the
     `WalletShieldedOutput` change.
-  - `ScannedBlock` has an additional type parameter as a consequence of the 
+  - `ScannedBlock` has an additional type parameter as a consequence of the
     `WalletTx` change.
   - `ScannedBlock::metadata` has been renamed to `to_block_metadata` and now
     returns an owned value rather than a reference.
@@ -126,9 +125,16 @@ and this library adheres to Rust's notion of
     argument, instead taking explicit `target_height` and `anchor_height`
     arguments. This helps to minimize the set of capabilities that the
     `data_api::InputSource` must expose.
-  - `WalletRead::get_checkpoint_depth` has been removed without replacement. This
-    is no longer needed given the change to use the stored anchor height for transaction
-    proposal execution.
+  - Changes to the `WalletRead` trait:
+    - Added `get_orchard_nullifiers` (under the `orchard` feature flag.)
+    - `get_checkpoint_depth` has been removed without replacement. This
+      is no longer needed given the change to use the stored anchor height for transaction
+      proposal execution.
+    - `is_valid_account_extfvk` has been removed; it was unused in
+      the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`.
+    - `get_spendable_sapling_notes`, `select_spendable_sapling_notes`, and
+      `get_unspent_transparent_outputs` have been removed; use
+      `data_api::InputSource` instead.
   - `wallet::{propose_shielding, shield_transparent_funds}` now takes their
     `min_confirmations` arguments as `u32` rather than a `NonZeroU32` to permit
     implmentations to enable zero-conf shielding.
@@ -146,8 +152,10 @@ and this library adheres to Rust's notion of
   - `ChangeValue` is now a struct. In addition to the existing change value, it
     now also provides the output pool to which change should be sent and an
     optional memo to be associated with the change output.
-  - `fixed::SingleOutputChangeStrategy::new` and `zip317::SingleOutputChangeStrategy::new` 
-    each now accept an additional `change_memo` argument
+  - `ChangeError` has a new `BundleError` variant.
+  - `fixed::SingleOutputChangeStrategy::new` and
+    `zip317::SingleOutputChangeStrategy::new` each now accept an additional
+    `change_memo` argument.
 - `zcash_client_backend::wallet`:
   - The fields of `ReceivedSaplingNote` are now private. Use
     `ReceivedSaplingNote::from_parts` for construction instead. Accessor methods
@@ -183,7 +191,7 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::address`:
   - `RecipientAddress` has been renamed to `Address`
   - `Address::Shielded` has been renamed to `Address::Sapling`
-  - `UnifiedAddress::from_receivers` no longer takes an Orchard receiver 
+  - `UnifiedAddress::from_receivers` no longer takes an Orchard receiver
     argument unless the `orchard` feature is enabled.
   - `UnifiedAddress::orchard` is now only available when the `orchard` feature
     is enabled.
@@ -199,16 +207,9 @@ and this library adheres to Rust's notion of
 ### Removed
 - `zcash_client_backend::wallet::ReceivedSaplingNote` has been replaced by
   `zcash_client_backend::ReceivedNote`.
+- `zcash_client_backend::wallet::input_selection::Proposal::sapling_inputs` has
+  been replaced by `Proposal::shielded_inputs`
 - `zcash_client_backend::data_api`
-  - `WalletRead::is_valid_account_extfvk` has been removed; it was unused in
-    the ECC mobile wallet SDKs and has been superseded by `get_account_for_ufvk`.
-  - `wallet::input_selection::Proposal::sapling_inputs` has been replaced
-    by `Proposal::shielded_inputs`
-- `zcash_client_backend::data_api::WalletRead::{
-     get_spendable_sapling_notes
-     select_spendable_sapling_notes,
-     get_unspent_transparent_outputs,
-   }` - use `data_api::InputSource` instead.
 - `zcash_client_backend::data_api::ScannedBlock::from_parts` has been made crate-private.
 - `zcash_client_backend::data_api::ScannedBlock::into_sapling_commitments` has been
   replaced by `into_commitments` which returns both Sapling and Orchard note commitments

zcash_client_backend/src/fees.rs

@@ -16,6 +16,7 @@ use crate::ShieldedProtocol;
 
 pub(crate) mod common;
 pub mod fixed;
+#[cfg(feature = "orchard")]
 pub mod orchard;
 pub mod sapling;
 pub mod standard;

zcash_client_backend/src/fees/common.rs

@@ -87,10 +87,10 @@ where
     // TODO: implement a less naive strategy for selecting the pool to which change will be sent.
     #[cfg(feature = "orchard")]
     let (change_pool, sapling_change, orchard_change) =
-        if orchard_in > NonNegativeAmount::ZERO || orchard_out > NonNegativeAmount::ZERO {
+        if orchard_in.is_positive() || orchard_out.is_positive() {
             // Send change to Orchard if we're spending any Orchard inputs or creating any Orchard outputs
             (ShieldedProtocol::Orchard, 0, 1)
-        } else if sapling_in > NonNegativeAmount::ZERO || sapling_out > NonNegativeAmount::ZERO {
+        } else if sapling_in.is_positive() || sapling_out.is_positive() {
             // Otherwise, send change to Sapling if we're spending any Sapling inputs or creating any
             // Sapling outputs, so that we avoid pool-crossing.
             (ShieldedProtocol::Sapling, 1, 0)
@@ -119,7 +119,10 @@ where
             target_height,
             transparent_inputs,
             transparent_outputs,
-            sapling.inputs().len(),
+            sapling
+                .bundle_type()
+                .num_spends(sapling.inputs().len())
+                .map_err(ChangeError::BundleError)?,
             sapling
                 .bundle_type()
                 .num_outputs(
@@ -139,7 +142,7 @@ where
         required: total_out,
     })?;
 
-    if proposed_change == NonNegativeAmount::ZERO {
+    if proposed_change.is_zero() {
         TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow())
     } else {
         let dust_threshold = dust_output_policy

zcash_client_backend/src/fees/orchard.rs

@@ -4,12 +4,10 @@
 use std::convert::Infallible;
 use zcash_primitives::transaction::components::amount::NonNegativeAmount;
 
-#[cfg(feature = "orchard")]
 use orchard::builder::BundleType;
 
 /// A trait that provides a minimized view of Orchard bundle configuration
 /// suitable for use in fee and change calculation.
-#[cfg(feature = "orchard")]
 pub trait BundleView<NoteRef> {
     /// The type of inputs to the bundle.
     type In: InputView<NoteRef>;
@@ -24,7 +22,6 @@ pub trait BundleView<NoteRef> {
     fn outputs(&self) -> &[Self::Out];
 }
 
-#[cfg(feature = "orchard")]
 impl<'a, NoteRef, In: InputView<NoteRef>, Out: OutputView> BundleView<NoteRef>
     for (BundleType, &'a [In], &'a [Out])
 {

zcash_client_backend/src/wallet.rs

@@ -259,6 +259,7 @@ impl Note {
         }
     }
 
+    /// Returns the shielded protocol used by this note.
     pub fn protocol(&self) -> ShieldedProtocol {
         match self {
             Note::Sapling(_) => ShieldedProtocol::Sapling,
@@ -340,16 +341,6 @@ impl<NoteRef, NoteT> ReceivedNote<NoteRef, NoteT> {
     }
 }
 
-impl<NoteRef> ReceivedNote<NoteRef, Note> {
-    pub fn protocol(&self) -> ShieldedProtocol {
-        match self.note() {
-            Note::Sapling(_) => ShieldedProtocol::Sapling,
-            #[cfg(feature = "orchard")]
-            Note::Orchard(_) => ShieldedProtocol::Orchard,
-        }
-    }
-}
-
 impl<NoteRef> sapling_fees::InputView<NoteRef> for ReceivedNote<NoteRef, sapling::Note> {
     fn note_id(&self) -> &NoteRef {
         &self.note_id

zcash_client_sqlite/Cargo.toml

@@ -22,7 +22,6 @@ rustdoc-args = ["--cfg", "docsrs"]
 zcash_client_backend = { workspace = true, features = ["unstable-serialization", "unstable-spanning-tree"] }
 zcash_encoding.workspace = true
 zcash_primitives.workspace = true
-orchard.workspace = true
 
 # Dependencies exposed in a public API:
 # (Breaking upgrades to these require a breaking upgrade to this crate.)
@@ -43,6 +42,7 @@ jubjub.workspace = true
 secrecy.workspace = true
 
 # - Shielded protocols
+orchard.workspace = true
 sapling.workspace = true
 
 # - Note commitment trees

zcash_primitives/CHANGELOG.md

@@ -41,6 +41,8 @@ and this library adheres to Rust's notion of
   - `NonNegativeAmount::const_from_u64`
   - `NonNegativeAmount::from_nonnegative_i64_le_bytes`
   - `NonNegativeAmount::to_i64_le_bytes`
+  - `NonNegativeAmount::is_zero`
+  - `NonNegativeAmount::is_positive`
   - `impl From<&NonNegativeAmount> for Amount`
   - `impl From<NonNegativeAmount> for u64`
   - `impl From<NonNegativeAmount> for zcash_primitives::sapling::value::NoteValue`

zcash_primitives/src/transaction/components/amount.rs

@@ -297,6 +297,16 @@ impl NonNegativeAmount {
     pub fn to_i64_le_bytes(self) -> [u8; 8] {
         self.0.to_i64_le_bytes()
     }
+
+    /// Returns whether or not this `NonNegativeAmount` is the zero value.
+    pub fn is_zero(&self) -> bool {
+        self == &NonNegativeAmount::ZERO
+    }
+
+    /// Returns whether or not this `NonNegativeAmount` is positive.
+    pub fn is_positive(&self) -> bool {
+        self > &NonNegativeAmount::ZERO
+    }
 }
 
 impl From<NonNegativeAmount> for Amount {