Address comments from code review.

zcash_client_backend/CHANGELOG.md

@@ -156,8 +156,7 @@ and this library adheres to Rust's notion of
     - It returns a `NonEmpty<TxId>` instead of a single `TxId` value.
   - `wallet::create_spend_to_address` now takes additional `change_memo` and
     `fallback_change_pool` arguments. It also returns its result as a
-    `NonEmpty<TxId>` instead of a
-    single `TxId`.
+    `NonEmpty<TxId>` instead of a single `TxId`.
   - `wallet::spend` returns its result as a `NonEmpty<TxId>` instead of a
     single `TxId`.
   - The error type of `wallet::create_spend_to_address` has been changed to use
@@ -237,9 +236,10 @@ and this library adheres to Rust's notion of
     are provided for each previously public field.
   - `Recipient` is now polymorphic in the type of the payload for wallet-internal
     recipients. This simplifies the handling of wallet-internal outputs.
-  - `SentTransactionOutput::from_parts` now takes a `Recipient<Note>`
-  - `SentTransactionOutput::recipient` now returns a `Recipient<Note>`
-  - `OvkPolicy::Custom` now wraps a bare `[u8; 32]` instead of a Sapling `OutgoingViewingKey`.
+  - `SentTransactionOutput::from_parts` now takes a `Recipient<Note>`.
+  - `SentTransactionOutput::recipient` now returns a `Recipient<Note>`.
+  - `OvkPolicy::Custom` is now a structured variant that can contain independent 
+    Sapling and Orchard `OutgoingViewingKey`s.
 - `zcash_client_backend::scanning::ScanError` has a new variant, `TreeSizeInvalid`.
 - `zcash_client_backend::zip321::TransactionRequest::payments` now returns a
   `BTreeMap<usize, Payment>` instead of `&[Payment]` so that parameter

zcash_client_backend/src/data_api.rs

@@ -8,7 +8,6 @@ use std::{
 };
 
 use incrementalmerkletree::{frontier::Frontier, Retention};
-use sapling;
 use secrecy::SecretVec;
 use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree};
 use zcash_primitives::{
@@ -815,7 +814,9 @@ pub struct SentTransaction<'a> {
     pub utxos_spent: Vec<OutPoint>,
 }
 
-/// A type that represents an output (either Sapling or transparent) that was sent by the wallet.
+/// An output of a transaction generated by the wallet.
+///
+/// This type is capable of representing both shielded and transparent outputs.
 pub struct SentTransactionOutput {
     output_index: usize,
     recipient: Recipient<Note>,

zcash_client_backend/src/data_api/error.rs

@@ -67,7 +67,11 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
     /// It is forbidden to provide a memo when constructing a transparent output.
     MemoForbidden,
 
-    /// Attempted to create a send change to an unsupported pool.
+    /// Attempted to send change to an unsupported pool.
+    ///
+    /// This is indicative of a programming error; execution of a transaction proposal that
+    /// presumes support for the specified pool was performed using an application that does not
+    /// provide such support.
     UnsupportedChangeType(PoolType),
 
     /// Attempted to create a spend to an unsupported Unified Address receiver
@@ -142,7 +146,11 @@ where
             Error::Builder(e) => write!(f, "An error occurred building the transaction: {}", e),
             Error::MemoForbidden => write!(f, "It is not possible to send a memo to a transparent address."),
             Error::UnsupportedChangeType(t) => write!(f, "Attempted to send change to an unsupported pool type: {}", t),
-            Error::NoSupportedReceivers(_) => write!(f, "A recipient's unified address does not contain any receivers to which the wallet can send funds."),
+            Error::NoSupportedReceivers(ua) => write!(
+                f,
+                "A recipient's unified address does not contain any receivers to which the wallet can send funds; required {}",
+                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),
             Error::NoteMismatch(n) => write!(f, "A note being spent ({:?}) does not correspond to either the internal or external full viewing key for the provided spending key.", n),
 

zcash_client_backend/src/data_api/wallet.rs

@@ -846,9 +846,9 @@ where
     let orchard_fvk: orchard::keys::FullViewingKey = usk.orchard().into();
 
     #[cfg(feature = "orchard")]
-    let orchard_external_ovk = match ovk_policy {
+    let orchard_external_ovk = match &ovk_policy {
         OvkPolicy::Sender => Some(orchard_fvk.to_ovk(orchard::keys::Scope::External)),
-        OvkPolicy::Custom(ovk) => Some(orchard::keys::OutgoingViewingKey::from(ovk)),
+        OvkPolicy::Custom { orchard, .. } => Some(orchard.clone()),
         OvkPolicy::Discard => None,
     };
 
@@ -870,9 +870,9 @@ where
     let sapling_dfvk = usk.sapling().to_diversifiable_full_viewing_key();
 
     // Apply the outgoing viewing key policy.
-    let sapling_external_ovk = match ovk_policy {
+    let sapling_external_ovk = match &ovk_policy {
         OvkPolicy::Sender => Some(sapling_dfvk.to_ovk(Scope::External)),
-        OvkPolicy::Custom(ovk) => Some(sapling::keys::OutgoingViewingKey(ovk)),
+        OvkPolicy::Custom { sapling, .. } => Some(*sapling),
         OvkPolicy::Discard => None,
     };
 
@@ -1040,7 +1040,7 @@ where
                     .expect("An action should exist in the transaction for each Orchard output.");
 
                 let recipient = recipient
-                    .map(|pool| {
+                    .map_internal_account(|pool| {
                         assert!(pool == PoolType::Shielded(ShieldedProtocol::Orchard));
                         build_result
                             .transaction()
@@ -1051,7 +1051,7 @@ where
                                     .map(|(note, _, _)| Note::Orchard(note))
                             })
                     })
-                    .transpose()
+                    .internal_account_transpose_option()
                     .expect("Wallet-internal outputs must be decryptable with the wallet's IVK");
 
                 SentTransactionOutput::from_parts(output_index, recipient, value, memo)
@@ -1070,7 +1070,7 @@ where
                     .expect("An output should exist in the transaction for each Sapling payment.");
 
                 let recipient = recipient
-                    .map(|pool| {
+                    .map_internal_account(|pool| {
                         assert!(pool == PoolType::Shielded(ShieldedProtocol::Sapling));
                         build_result
                             .transaction()
@@ -1087,7 +1087,7 @@ where
                                 .map(|(note, _, _)| Note::Sapling(note))
                             })
                     })
-                    .transpose()
+                    .internal_account_transpose_option()
                     .expect("Wallet-internal outputs must be decryptable with the wallet's IVK");
 
                 SentTransactionOutput::from_parts(output_index, recipient, value, memo)

zcash_client_backend/src/wallet.rs

@@ -73,7 +73,7 @@ pub enum Recipient<N> {
 }
 
 impl<N> Recipient<N> {
-    pub fn map<B, F: FnOnce(N) -> B>(self, f: F) -> Recipient<B> {
+    pub fn map_internal_account<B, F: FnOnce(N) -> B>(self, f: F) -> Recipient<B> {
         match self {
             Recipient::Transparent(t) => Recipient::Transparent(t),
             Recipient::Sapling(s) => Recipient::Sapling(s),
@@ -84,7 +84,7 @@ impl<N> Recipient<N> {
 }
 
 impl<N> Recipient<Option<N>> {
-    pub fn transpose(self) -> Option<Recipient<N>> {
+    pub fn internal_account_transpose_option(self) -> Option<Recipient<N>> {
         match self {
             Recipient::Transparent(t) => Some(Recipient::Transparent(t)),
             Recipient::Sapling(s) => Some(Recipient::Sapling(s)),
@@ -415,13 +415,30 @@ pub enum OvkPolicy {
     ///
     /// Transaction outputs will be decryptable by the recipients, and whoever controls
     /// the provided outgoing viewing key.
-    Custom([u8; 32]),
-
+    Custom {
+        sapling: sapling::keys::OutgoingViewingKey,
+        #[cfg(feature = "orchard")]
+        orchard: orchard::keys::OutgoingViewingKey,
+    },
     /// Use no outgoing viewing key. 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.
+    ///
+    /// Transactions constructed under this OVK policy will have their outputs
+    /// 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),
+            #[cfg(feature = "orchard")]
+            orchard: orchard::keys::OutgoingViewingKey::from(*key),
+        }
+    }
+}
+
 /// Metadata related to the ZIP 32 derivation of a transparent address.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[cfg(feature = "transparent-inputs")]

zcash_client_sqlite/src/testing.rs

@@ -445,6 +445,7 @@ impl<Cache> TestState<Cache> {
         ovk_policy: OvkPolicy,
         min_confirmations: NonZeroU32,
         change_memo: Option<MemoBytes>,
+        fallback_change_pool: ShieldedProtocol,
     ) -> Result<
         NonEmpty<TxId>,
         data_api::error::Error<
@@ -468,7 +469,7 @@ impl<Cache> TestState<Cache> {
             ovk_policy,
             min_confirmations,
             change_memo,
-            ShieldedProtocol::Sapling,
+            fallback_change_pool,
         )
     }
 
@@ -551,6 +552,7 @@ impl<Cache> TestState<Cache> {
         amount: NonNegativeAmount,
         memo: Option<MemoBytes>,
         change_memo: Option<MemoBytes>,
+        fallback_change_pool: ShieldedProtocol,
     ) -> Result<
         Proposal<StandardFeeRule, ReceivedNoteId>,
         data_api::error::Error<
@@ -571,7 +573,7 @@ impl<Cache> TestState<Cache> {
             amount,
             memo,
             change_memo,
-            ShieldedProtocol::Sapling,
+            fallback_change_pool,
         );
 
         if let Ok(proposal) = &result {
@@ -1058,13 +1060,14 @@ impl TestCache for FsBlockCache {
 pub(crate) fn input_selector(
     fee_rule: StandardFeeRule,
     change_memo: Option<&str>,
+    fallback_change_pool: ShieldedProtocol,
 ) -> GreedyInputSelector<
     WalletDb<rusqlite::Connection, Network>,
     standard::SingleOutputChangeStrategy,
 > {
     let change_memo = change_memo.map(|m| MemoBytes::from(m.parse::<Memo>().unwrap()));
     let change_strategy =
-        standard::SingleOutputChangeStrategy::new(fee_rule, change_memo, ShieldedProtocol::Sapling);
+        standard::SingleOutputChangeStrategy::new(fee_rule, change_memo, fallback_change_pool);
     GreedyInputSelector::new(change_strategy, DustOutputPolicy::default())
 }
 

zcash_client_sqlite/src/wallet/sapling.rs

@@ -859,7 +859,8 @@ pub(crate) mod tests {
                 None,
                 OvkPolicy::Sender,
                 NonZeroU32::new(1).unwrap(),
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Err(data_api::error::Error::KeyNotRecognized)
         );
@@ -888,7 +889,8 @@ pub(crate) mod tests {
                 &to,
                 NonNegativeAmount::const_from_u64(1),
                 None,
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Err(data_api::error::Error::ScanRequired)
         );
@@ -955,7 +957,8 @@ pub(crate) mod tests {
                 &to,
                 NonNegativeAmount::const_from_u64(70000),
                 None,
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Err(data_api::error::Error::InsufficientFunds {
                 available,
@@ -984,7 +987,8 @@ pub(crate) mod tests {
                 &to,
                 NonNegativeAmount::const_from_u64(70000),
                 None,
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Err(data_api::error::Error::InsufficientFunds {
                 available,
@@ -1019,6 +1023,7 @@ pub(crate) mod tests {
                 amount_sent,
                 None,
                 None,
+                ShieldedProtocol::Sapling,
             )
             .unwrap();
 
@@ -1076,6 +1081,7 @@ pub(crate) mod tests {
                 NonNegativeAmount::const_from_u64(15000),
                 None,
                 None,
+                ShieldedProtocol::Sapling,
             )
             .unwrap();
 
@@ -1094,7 +1100,8 @@ pub(crate) mod tests {
                 &to,
                 NonNegativeAmount::const_from_u64(2000),
                 None,
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Err(data_api::error::Error::InsufficientFunds {
                 available,
@@ -1123,7 +1130,8 @@ pub(crate) mod tests {
                 &to,
                 NonNegativeAmount::const_from_u64(2000),
                 None,
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Err(data_api::error::Error::InsufficientFunds {
                 available,
@@ -1156,6 +1164,7 @@ pub(crate) mod tests {
                 amount_sent2,
                 None,
                 None,
+                ShieldedProtocol::Sapling,
             )
             .unwrap();
 
@@ -1223,6 +1232,7 @@ pub(crate) mod tests {
                 NonNegativeAmount::const_from_u64(15000),
                 None,
                 None,
+                ShieldedProtocol::Sapling,
             )?;
 
             // Executing the proposal should succeed
@@ -1325,6 +1335,7 @@ pub(crate) mod tests {
                 NonNegativeAmount::const_from_u64(50000),
                 None,
                 None,
+                ShieldedProtocol::Sapling,
             )
             .unwrap();
 
@@ -1387,6 +1398,7 @@ pub(crate) mod tests {
                 NonNegativeAmount::const_from_u64(50000),
                 None,
                 None,
+                ShieldedProtocol::Sapling,
             )
             .unwrap();
 
@@ -1555,7 +1567,8 @@ pub(crate) mod tests {
         assert_eq!(st.get_total_balance(account), total);
         assert_eq!(st.get_spendable_balance(account, 1), total);
 
-        let input_selector = input_selector(StandardFeeRule::Zip317, None);
+        let input_selector =
+            input_selector(StandardFeeRule::Zip317, None, ShieldedProtocol::Sapling);
 
         // This first request will fail due to insufficient non-dust funds
         let req = TransactionRequest::new(vec![Payment {
@@ -1828,7 +1841,8 @@ pub(crate) mod tests {
                 None,
                 OvkPolicy::Sender,
                 NonZeroU32::new(5).unwrap(),
-                None
+                None,
+                ShieldedProtocol::Sapling
             ),
             Ok(_)
         );

zcash_keys/CHANGELOG.md

@@ -14,7 +14,8 @@ The entries below are relative to the `zcash_client_backend` crate as of
   - `address`
   - `encoding`
   - `keys`
-- `zcash_keys::address::UnifiedAddress::{unknown, has_orchard, has_sapling, has_transparent}`:
+- `zcash_keys::address::UnifiedAddress::{unknown, has_orchard, has_sapling, 
+   has_transparent, receiver_types}`:
 - `zcash_keys::keys`:
   - `AddressGenerationError`
   - `UnifiedAddressRequest`