zcash_client_backend: Move change memos into the ChangeValue compon…

…ents of `Proposal`s.

The existing API limited change outputs to having only a single memo
repeated across each change output. This change makes it so that each
proposed change output can have its own associated memo, and leaves it
up to the input selector to determine how requested change memos are
associated with change outputs.

zcash_client_backend/CHANGELOG.md

@@ -10,6 +10,27 @@ and this library adheres to Rust's notion of
 ### Changed
 - `zcash_client_backend::data_api::chain::scan_cached_blocks` now returns
   a `ScanSummary` containing metadata about the scanned blocks on success.
+- The fields of `zcash_client_backend::wallet::ReceivedSaplingNote` are now
+  private. Use `ReceivedSaplingNote::from_parts` for construction instead.
+  Accessor methods are provided for each previously-public field.
+- `zcash_client_backend::data_api` changes:
+  - `wallet::shield_transparent_funds` no longer
+    takes a `memo` argument; instead, memos to be associated with the shielded
+    outputs should be specified in the construction of the value of the
+    `input_selector` argument, which is used to construct the proposed shielded
+    values as internal "change" outputs.
+  - `wallet::create_proposed_transaction` no longer takes a
+    `change_memo` argument; instead, change memos are represented in the
+    individual values of the `proposed_change` field of the `Proposal`'s
+    `TransactionBalance`.
+  - `wallet::create_spend_to_address` now takes an additional
+    `change_memo` argument.
+- `zcash_client_backend::fees::ChangeValue::Sapling` is now a structured variant.
+  In addition to the existing change value, it now also carries an optional memo
+  to be associated with the change output.
+- `zcash_client_backend::fees::fixed::SingleOutputChangeStrategy::new` and
+  `zcash_client_backend::fees::zip317::SingleOutputChangeStrategy::new` each now
+  accept an additional `change_memo` argument.
 
 ## [0.10.0] - 2023-09-25
 

zcash_client_backend/src/data_api/wallet.rs

@@ -199,6 +199,7 @@ pub fn create_spend_to_address<DbT, ParamsT>(
     memo: Option<MemoBytes>,
     ovk_policy: OvkPolicy,
     min_confirmations: NonZeroU32,
+    change_memo: Option<MemoBytes>,
 ) -> Result<
     TxId,
     Error<
@@ -228,7 +229,7 @@ where
 
     #[allow(deprecated)]
     let fee_rule = fixed::FeeRule::standard();
-    let change_strategy = fees::fixed::SingleOutputChangeStrategy::new(fee_rule);
+    let change_strategy = fees::fixed::SingleOutputChangeStrategy::new(fee_rule, change_memo);
     spend(
         wallet_db,
         params,
@@ -340,7 +341,6 @@ where
         ovk_policy,
         proposal,
         min_confirmations,
-        None,
     )
 }
 
@@ -435,7 +435,6 @@ pub fn create_proposed_transaction<DbT, ParamsT, InputsErrT, FeeRuleT>(
     ovk_policy: OvkPolicy,
     proposal: Proposal<FeeRuleT, DbT::NoteRef>,
     min_confirmations: NonZeroU32,
-    change_memo: Option<MemoBytes>,
 ) -> Result<
     TxId,
     Error<
@@ -579,22 +578,20 @@ where
 
     for change_value in proposal.balance().proposed_change() {
         match change_value {
-            ChangeValue::Sapling(amount) => {
-                let memo = change_memo
-                    .as_ref()
-                    .map_or_else(MemoBytes::empty, |m| m.clone());
+            ChangeValue::Sapling { value, memo } => {
+                let memo = memo.as_ref().map_or_else(MemoBytes::empty, |m| m.clone());
                 builder.add_sapling_output(
                     internal_ovk(),
                     dfvk.change_address().1,
-                    *amount,
+                    *value,
                     memo.clone(),
                 )?;
                 sapling_output_meta.push((
                     Recipient::InternalAccount(
                         account,
                         PoolType::Shielded(ShieldedProtocol::Sapling),
                     ),
-                    *amount,
+                    *value,
                     Some(memo),
                 ))
             }
@@ -695,10 +692,6 @@ where
 /// * `from_addrs`: The list of transparent addresses that will be used to filter transaparent
 ///   UTXOs received by the wallet. Only UTXOs received at one of the provided addresses will
 ///   be selected to be shielded.
-/// * `memo`: A memo to be included in the output to the (internal) recipient.
-///   This can be used to take notes about auto-shielding operations internal
-///   to the wallet that the wallet can use to improve how it represents those
-///   shielding transactions to the user.
 /// * `min_confirmations`: The minimum number of confirmations that a previously
 ///   received note must have in the blockchain in order to be considered for being
 ///   spent. A value of 10 confirmations is recommended and 0-conf transactions are
@@ -716,7 +709,6 @@ pub fn shield_transparent_funds<DbT, ParamsT, InputsT>(
     shielding_threshold: NonNegativeAmount,
     usk: &UnifiedSpendingKey,
     from_addrs: &[TransparentAddress],
-    memo: &MemoBytes,
     min_confirmations: NonZeroU32,
 ) -> Result<
     TxId,
@@ -751,7 +743,6 @@ where
         OvkPolicy::Sender,
         proposal,
         min_confirmations,
-        Some(memo.clone()),
     )
 }
 

zcash_client_backend/src/fees.rs

@@ -2,6 +2,7 @@ use std::fmt;
 
 use zcash_primitives::{
     consensus::{self, BlockHeight},
+    memo::MemoBytes,
     transaction::{
         components::{
             amount::{Amount, BalanceError},
@@ -19,13 +20,20 @@ pub mod zip317;
 /// A proposed change amount and output pool.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ChangeValue {
-    Sapling(Amount),
+    Sapling {
+        value: Amount,
+        memo: Option<MemoBytes>,
+    },
 }
 
 impl ChangeValue {
+    pub fn sapling(value: Amount, memo: Option<MemoBytes>) -> Self {
+        Self::Sapling { value, memo }
+    }
+
     pub fn value(&self) -> Amount {
         match self {
-            ChangeValue::Sapling(value) => *value,
+            ChangeValue::Sapling { value, .. } => *value,
         }
     }
 }

zcash_client_backend/src/fees/fixed.rs

@@ -3,6 +3,7 @@ use std::cmp::Ordering;
 
 use zcash_primitives::{
     consensus::{self, BlockHeight},
+    memo::MemoBytes,
     transaction::{
         components::{
             amount::{Amount, BalanceError},
@@ -21,12 +22,16 @@ use super::{
 /// shielded pool and delegates fee calculation to the provided fee rule.
 pub struct SingleOutputChangeStrategy {
     fee_rule: FixedFeeRule,
+    change_memo: Option<MemoBytes>,
 }
 
 impl SingleOutputChangeStrategy {
     /// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule.
-    pub fn new(fee_rule: FixedFeeRule) -> Self {
-        Self { fee_rule }
+    pub fn new(fee_rule: FixedFeeRule, change_memo: Option<MemoBytes>) -> Self {
+        Self {
+            fee_rule,
+            change_memo,
+        }
     }
 }
 
@@ -131,7 +136,10 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                                 })
                             }
                             DustAction::AllowDustChange => TransactionBalance::new(
-                                vec![ChangeValue::Sapling(proposed_change)],
+                                vec![ChangeValue::sapling(
+                                    proposed_change,
+                                    self.change_memo.clone(),
+                                )],
                                 fee_amount,
                             )
                             .ok_or_else(|| BalanceError::Overflow.into()),
@@ -143,7 +151,10 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                         }
                     } else {
                         TransactionBalance::new(
-                            vec![ChangeValue::Sapling(proposed_change)],
+                            vec![ChangeValue::sapling(
+                                proposed_change,
+                                self.change_memo.clone(),
+                            )],
                             fee_amount,
                         )
                         .ok_or_else(|| BalanceError::Overflow.into())
@@ -177,7 +188,7 @@ mod tests {
     fn change_without_dust() {
         #[allow(deprecated)]
         let fee_rule = FixedFeeRule::standard();
-        let change_strategy = SingleOutputChangeStrategy::new(fee_rule);
+        let change_strategy = SingleOutputChangeStrategy::new(fee_rule, None);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(
@@ -197,7 +208,7 @@ mod tests {
 
         assert_matches!(
             result,
-            Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(Amount::from_u64(10000).unwrap())]
+            Ok(balance) if balance.proposed_change() == [ChangeValue::sapling(Amount::from_u64(10000).unwrap(), None)]
                 && balance.fee_required() == Amount::from_u64(10000).unwrap()
         );
     }
@@ -206,7 +217,7 @@ mod tests {
     fn dust_change() {
         #[allow(deprecated)]
         let fee_rule = FixedFeeRule::standard();
-        let change_strategy = SingleOutputChangeStrategy::new(fee_rule);
+        let change_strategy = SingleOutputChangeStrategy::new(fee_rule, None);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(

zcash_client_backend/src/fees/zip317.rs

@@ -7,6 +7,7 @@ use core::cmp::Ordering;
 
 use zcash_primitives::{
     consensus::{self, BlockHeight},
+    memo::MemoBytes,
     transaction::{
         components::{
             amount::{Amount, BalanceError},
@@ -28,13 +29,17 @@ use super::{
 /// shielded pool and delegates fee calculation to the provided fee rule.
 pub struct SingleOutputChangeStrategy {
     fee_rule: Zip317FeeRule,
+    change_memo: Option<MemoBytes>,
 }
 
 impl SingleOutputChangeStrategy {
     /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317
     /// fee parameters.
-    pub fn new(fee_rule: Zip317FeeRule) -> Self {
-        Self { fee_rule }
+    pub fn new(fee_rule: Zip317FeeRule, change_memo: Option<MemoBytes>) -> Self {
+        Self {
+            fee_rule,
+            change_memo,
+        }
     }
 }
 
@@ -203,7 +208,10 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                             })
                         }
                         DustAction::AllowDustChange => TransactionBalance::new(
-                            vec![ChangeValue::Sapling(proposed_change)],
+                            vec![ChangeValue::sapling(
+                                proposed_change,
+                                self.change_memo.clone(),
+                            )],
                             fee_amount,
                         )
                         .ok_or_else(overflow),
@@ -213,8 +221,14 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
                         }
                     }
                 } else {
-                    TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount)
-                        .ok_or_else(overflow)
+                    TransactionBalance::new(
+                        vec![ChangeValue::sapling(
+                            proposed_change,
+                            self.change_memo.clone(),
+                        )],
+                        fee_amount,
+                    )
+                    .ok_or_else(overflow)
                 }
             }
         }
@@ -244,7 +258,7 @@ mod tests {
 
     #[test]
     fn change_without_dust() {
-        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard());
+        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard(), None);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(
@@ -264,14 +278,14 @@ mod tests {
 
         assert_matches!(
             result,
-            Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(Amount::from_u64(5000).unwrap())]
+            Ok(balance) if balance.proposed_change() == [ChangeValue::sapling(Amount::from_u64(5000).unwrap(), None)]
                 && balance.fee_required() == Amount::from_u64(10000).unwrap()
         );
     }
 
     #[test]
     fn change_with_transparent_payments() {
-        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard());
+        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard(), None);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(
@@ -301,7 +315,7 @@ mod tests {
 
     #[test]
     fn change_with_allowable_dust() {
-        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard());
+        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard(), None);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(
@@ -334,7 +348,7 @@ mod tests {
 
     #[test]
     fn change_with_disallowed_dust() {
-        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard());
+        let change_strategy = SingleOutputChangeStrategy::new(Zip317FeeRule::standard(), None);
 
         // spend a single Sapling note that is sufficient to pay the fee
         let result = change_strategy.compute_balance(

zcash_client_sqlite/src/chain.rs

@@ -532,7 +532,7 @@ mod tests {
         }])
         .unwrap();
         let input_selector = GreedyInputSelector::new(
-            SingleOutputChangeStrategy::new(FeeRule::standard()),
+            SingleOutputChangeStrategy::new(FeeRule::standard(), None),
             DustOutputPolicy::default(),
         );
         assert_matches!(

zcash_client_sqlite/src/testing.rs

@@ -430,6 +430,7 @@ impl<Cache> TestState<Cache> {
     /// Invokes [`create_spend_to_address`] with the given arguments.
     #[allow(deprecated)]
     #[allow(clippy::type_complexity)]
+    #[allow(clippy::too_many_arguments)]
     pub(crate) fn create_spend_to_address(
         &mut self,
         usk: &UnifiedSpendingKey,
@@ -438,6 +439,7 @@ impl<Cache> TestState<Cache> {
         memo: Option<MemoBytes>,
         ovk_policy: OvkPolicy,
         min_confirmations: NonZeroU32,
+        change_memo: Option<MemoBytes>,
     ) -> Result<
         TxId,
         data_api::error::Error<
@@ -459,6 +461,7 @@ impl<Cache> TestState<Cache> {
             memo,
             ovk_policy,
             min_confirmations,
+            change_memo,
         )
     }
 
@@ -570,7 +573,6 @@ impl<Cache> TestState<Cache> {
         ovk_policy: OvkPolicy,
         proposal: Proposal<FeeRuleT, ReceivedNoteId>,
         min_confirmations: NonZeroU32,
-        change_memo: Option<MemoBytes>,
     ) -> Result<
         TxId,
         data_api::error::Error<
@@ -593,7 +595,6 @@ impl<Cache> TestState<Cache> {
             ovk_policy,
             proposal,
             min_confirmations,
-            change_memo,
         )
     }
 
@@ -606,7 +607,6 @@ impl<Cache> TestState<Cache> {
         shielding_threshold: NonNegativeAmount,
         usk: &UnifiedSpendingKey,
         from_addrs: &[TransparentAddress],
-        memo: &MemoBytes,
         min_confirmations: NonZeroU32,
     ) -> Result<
         TxId,
@@ -630,7 +630,6 @@ impl<Cache> TestState<Cache> {
             shielding_threshold,
             usk,
             from_addrs,
-            memo,
             min_confirmations,
         )
     }