Apply suggestions from code review

Co-authored-by: str4d <thestr4d@gmail.com>

zcash_client_backend/CHANGELOG.md

@@ -65,7 +65,7 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::wallet`:
   - `Note`
   - `ReceivedNote`
-  - `Recipient::{map, transpose}`
+  - `Recipient::{map_internal_account, internal_account_transpose_option}`
   - `WalletSaplingOutput::recipient_key_scope`
   - `TransparentAddressMetadata` (which replaces `zcash_keys::address::AddressMetadata`).
   - `impl {Debug, Clone} for OvkPolicy`

zcash_client_backend/src/data_api/error.rs

@@ -148,7 +148,7 @@ where
             Error::UnsupportedChangeType(t) => write!(f, "Attempted to send change to an unsupported pool type: {}", t),
             Error::NoSupportedReceivers(ua) => write!(
                 f,
-                "A recipient's unified address does not contain any receivers to which the wallet can send funds; required {}",
+                "A recipient's unified address does not contain any receivers to which the wallet can send funds; required one of {}",
                 ua.receiver_types().iter().enumerate().map(|(i, tc)| format!("{}{:?}", if i > 0 { ", " } else { "" }, tc)).collect::<String>()
             ),
             Error::NoSpendingKey(addr) => write!(f, "No spending key available for address: {}", addr),

zcash_client_backend/src/data_api/wallet.rs

@@ -894,7 +894,20 @@ where
     let mut orchard_output_meta = vec![];
     let mut sapling_output_meta = vec![];
     let mut transparent_output_meta = vec![];
-    for payment in proposal_step.transaction_request().payments().values() {
+    for (payment, output_pool) in proposal_step
+        .payment_pools()
+        .iter()
+        .map(|(idx, output_pool)| {
+            let payment = proposal_step
+                .transaction_request()
+                .payments()
+                .get(idx)
+                .expect(
+                    "The mapping between payment index and payment is checked in step construction",
+                );
+            (payment, output_pool)
+        })
+    {
         match &payment.recipient_address {
             Address::Unified(ua) => {
                 let memo = payment
@@ -902,54 +915,57 @@ where
                     .as_ref()
                     .map_or_else(MemoBytes::empty, |m| m.clone());
 
-                #[cfg(feature = "orchard")]
-                if let Some(orchard_receiver) = ua.orchard() {
-                    builder.add_orchard_output(
-                        orchard_external_ovk.clone(),
-                        *orchard_receiver,
-                        payment.amount.into(),
-                        memo.clone(),
-                    )?;
-                    orchard_output_meta.push((
-                        Recipient::Unified(
-                            ua.clone(),
-                            PoolType::Shielded(ShieldedProtocol::Orchard),
-                        ),
-                        payment.amount,
-                        Some(memo),
-                    ));
-                    continue;
-                }
+                match output_pool {
+                    #[cfg(not(feature = "orchard"))]
+                    PoolType::Shielded(ShieldedProtocol::Orchard) => {
+                        return Err(Error::ProposalNotSupported);
+                    }
+                    #[cfg(feature = "orchard")]
+                    PoolType::Shielded(ShieldedProtocol::Orchard) => {
+                        builder.add_orchard_output(
+                            orchard_external_ovk.clone(),
+                            *ua.orchard().expect("The mapping between payment pool and receiver is checked in step construction"),
+                            payment.amount.into(),
+                            memo.clone(),
+                        )?;
+                        orchard_output_meta.push((
+                            Recipient::Unified(
+                                ua.clone(),
+                                PoolType::Shielded(ShieldedProtocol::Orchard),
+                            ),
+                            payment.amount,
+                            Some(memo),
+                        ));
+                    }
 
-                if let Some(sapling_receiver) = ua.sapling() {
-                    builder.add_sapling_output(
-                        sapling_external_ovk,
-                        *sapling_receiver,
-                        payment.amount,
-                        memo.clone(),
-                    )?;
-                    sapling_output_meta.push((
-                        Recipient::Unified(
-                            ua.clone(),
-                            PoolType::Shielded(ShieldedProtocol::Sapling),
-                        ),
-                        payment.amount,
-                        Some(memo),
-                    ));
+                    PoolType::Shielded(ShieldedProtocol::Sapling) => {
+                        builder.add_sapling_output(
+                            sapling_external_ovk,
+                            *ua.sapling().expect("The mapping between payment pool and receiver is checked in step construction"),
+                            payment.amount,
+                            memo.clone(),
+                        )?;
+                        sapling_output_meta.push((
+                            Recipient::Unified(
+                                ua.clone(),
+                                PoolType::Shielded(ShieldedProtocol::Sapling),
+                            ),
+                            payment.amount,
+                            Some(memo),
+                        ));
+                    }
 
-                    continue;
-                }
-
-                if let Some(taddr) = ua.transparent() {
-                    if payment.memo.is_some() {
-                        return Err(Error::MemoForbidden);
-                    } else {
-                        builder.add_transparent_output(taddr, payment.amount)?;
-                        continue;
+                    PoolType::Transparent => {
+                        if payment.memo.is_some() {
+                            return Err(Error::MemoForbidden);
+                        } else {
+                            builder.add_transparent_output(
+                                ua.transparent().expect("The mapping between payment pool and receiver is checked in step construction."),
+                                payment.amount
+                            )?;
+                        }
                     }
                 }
-
-                return Err(Error::NoSupportedReceivers(Box::new(ua.clone())));
             }
             Address::Sapling(addr) => {
                 let memo = payment

zcash_client_backend/src/fees/fixed.rs

@@ -30,6 +30,9 @@ pub struct SingleOutputChangeStrategy {
 impl SingleOutputChangeStrategy {
     /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule
     /// and change memo.
+    ///
+    /// `fallback_change_pool` is used when more than one shielded pool is enabled via
+    /// feature flags, and the transaction has no shielded inputs.
     pub fn new(
         fee_rule: FixedFeeRule,
         change_memo: Option<MemoBytes>,

zcash_client_backend/src/fees/standard.rs

@@ -35,6 +35,9 @@ pub struct SingleOutputChangeStrategy {
 impl SingleOutputChangeStrategy {
     /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317
     /// fee parameters.
+    ///
+    /// `fallback_change_pool` is used when more than one shielded pool is enabled via
+    /// feature flags, and the transaction has no shielded inputs.
     pub fn new(
         fee_rule: StandardFeeRule,
         change_memo: Option<MemoBytes>,

zcash_client_backend/src/fees/zip317.rs

@@ -34,6 +34,9 @@ pub struct SingleOutputChangeStrategy {
 impl SingleOutputChangeStrategy {
     /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317
     /// fee parameters and change memo.
+    ///
+    /// `fallback_change_pool` is used when more than one shielded pool is enabled via
+    /// feature flags, and the transaction has no shielded inputs.
     pub fn new(
         fee_rule: Zip317FeeRule,
         change_memo: Option<MemoBytes>,

zcash_client_backend/src/proposal.rs

@@ -520,10 +520,10 @@ impl<NoteRef> Step<NoteRef> {
 
                 sapling_in || sapling_out || sapling_change
             }
-            #[cfg(not(feature = "orchard"))]
-            PoolType::Shielded(ShieldedProtocol::Orchard) => false,
-            #[cfg(feature = "orchard")]
             PoolType::Shielded(ShieldedProtocol::Orchard) => {
+                #[cfg(not(feature = "orchard"))]
+                let orchard_in = false;
+                #[cfg(feature = "orchard")]
                 let orchard_in = self.shielded_inputs.iter().any(|s_in| {
                     s_in.notes()
                         .iter()

zcash_client_backend/src/wallet.rs

@@ -402,34 +402,34 @@ impl<NoteRef> orchard_fees::InputView<NoteRef> for ReceivedNote<NoteRef, orchard
 /// [ZIP 310]: https://zips.z.cash/zip-0310
 #[derive(Debug, Clone)]
 pub enum OvkPolicy {
-    /// Use the outgoing viewing key from the sender's [`ExtendedFullViewingKey`].
+    /// Use the outgoing viewing key from the sender's [`UnifiedFullViewingKey`].
     ///
     /// Transaction outputs will be decryptable by the sender, in addition to the
     /// recipients.
     ///
-    /// [`ExtendedFullViewingKey`]: sapling::zip32::ExtendedFullViewingKey
+    /// [`UnifiedFullViewingKey`]: zcash_keys::keys::UnifiedFullViewingKey
     Sender,
 
-    /// Use a custom outgoing viewing key. This might for instance be derived from a
-    /// separate seed than the wallet's spending keys.
+    /// Use custom outgoing viewing keys. These might for instance be derived from a
+    /// different seed than the wallet's spending keys.
     ///
     /// Transaction outputs will be decryptable by the recipients, and whoever controls
-    /// the provided outgoing viewing key.
+    /// the provided outgoing viewing keys.
     Custom {
         sapling: sapling::keys::OutgoingViewingKey,
         #[cfg(feature = "orchard")]
         orchard: orchard::keys::OutgoingViewingKey,
     },
-    /// Use no outgoing viewing key. Transaction outputs will be decryptable by their
+    /// Use no outgoing viewing keys. Transaction outputs will be decryptable by their
     /// recipients, but not by the sender.
     Discard,
 }
 
 impl OvkPolicy {
-    /// Construct an [`OvkPolicy::Custom`] value from an arbitrary 32-byte key.
+    /// Constructs an [`OvkPolicy::Custom`] value from a single arbitrary 32-byte key.
     ///
-    /// Transactions constructed under this OVK policy will have their outputs
-    /// recoverable using this key irrespective of the output pool.
+    /// Outputs of transactions created with this OVK policy will be recoverable using
+    /// this key irrespective of the output pool.
     pub fn custom_from_common_bytes(key: &[u8; 32]) -> Self {
         OvkPolicy::Custom {
             sapling: sapling::keys::OutgoingViewingKey(*key),

zcash_client_sqlite/src/lib.rs

@@ -183,10 +183,18 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
     fn get_spendable_note(
         &self,
         txid: &TxId,
-        _protocol: ShieldedProtocol,
+        protocol: ShieldedProtocol,
         index: u32,
     ) -> Result<Option<ReceivedNote<Self::NoteRef, Note>>, Self::Error> {
-        wallet::sapling::get_spendable_sapling_note(self.conn.borrow(), &self.params, txid, index)
+        match protocol {
+            ShieldedProtocol::Sapling => wallet::sapling::get_spendable_sapling_note(
+                self.conn.borrow(),
+                &self.params,
+                txid,
+                index,
+            ),
+            ShieldedProtocol::Orchard => Ok(None),
+        }
     }
 
     fn select_spendable_notes(

zcash_keys/src/address.rs

@@ -193,7 +193,10 @@ impl UnifiedAddress {
         #[cfg(feature = "orchard")]
         let result = result.chain(self.orchard.map(|_| Typecode::Orchard));
         let result = result.chain(self.sapling.map(|_| Typecode::Sapling));
-        let result = result.chain(self.transparent.map(|_| Typecode::P2pkh));
+        let result = result.chain(self.transparent.map(|taddr| match taddr {
+            TransparentAddress::PublicKeyHash(_) => Typecode::P2pkh,
+            TransparentAddress::ScriptHash(_) => Typecode::P2sh,
+        }));
         let result = result.chain(
             self.unknown()
                 .iter()