Refactor change strategies so that we can use a CommonChangeStrategy

that is generic in the fee rule. This should simplify future
generalizations of the change strategy. We also undo the changes to
`{fixed,standard,zip317}::SingleOutputChangeStrategy` in this commit,
and deprecate those types' `new` constructors.

Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>

zcash_client_backend/CHANGELOG.md

@@ -13,8 +13,10 @@ and this library adheres to Rust's notion of
   - `WalletSummary::progress`
   - `WalletMeta`
   - `impl Default for wallet::input_selection::GreedyInputSelector`
-- `zcash_client_backend::fees::SplitPolicy`
-- `zcash_client_backend::fees::zip317::MultiOutputChangeStrategy`
+- `zcash_client_backend::fees::{SplitPolicy, CommonChangeStrategy}`. These
+  allow specifying a policy for splitting change into more than one output,
+  which can help to make enough notes available to perform multiple spends
+  without waiting for change.
 
 ### Changed
 - `zcash_client_backend::data_api`:
@@ -54,11 +56,6 @@ and this library adheres to Rust's notion of
     and `WalletMeta`, and its `FeeRule` associated type now has an additional
     `Clone` bound. In addition, it defines a new `fetch_wallet_meta` method, and
     the arguments to `compute_balance` have changed.
-  - The following methods now take an additional `DustOutputPolicy` argument,
-    and carry an additional type parameter:
-    - `fixed::SingleOutputChangeStrategy::new`
-    - `standard::SingleOutputChangeStrategy::new`
-    - `zip317::SingleOutputChangeStrategy::new`
 - `zcash_client_backend::proto`:
   - The `FeeRuleNotSpecified` variant of `FeeProposalDecodingError` has been
     renamed to `FeeRuleNotSupported`. It is now used to report attempted use
@@ -67,12 +64,19 @@ and this library adheres to Rust's notion of
 ### Changed
 - Migrated to `arti-client 0.22`.
 
+### Deprecated
+- `zcash_client_backend::fees::{fixed,standard,zip317}::SingleOutputChangeStrategy::new`
+  have been deprecated: instead use `CommonChangeStrategy::simple`.
+  (For most uses, the type parameter of the fee rule will be inferred.)
+
 ### Removed
 - `zcash_client_backend::data_api`:
   - `WalletSummary::scan_progress` and `WalletSummary::recovery_progress` have
     been removed. Use `WalletSummary::progress` instead.
-  - `testing::input_selector` use explicit `InputSelector` constructors
+  - `testing::input_selector`. Use explicit `InputSelector` constructors
     directly instead.
+  - `testing::single_output_change_strategy`. Use `fees::CommonChangeStrategy::simple`
+    instead.
 - `zcash_client_backend::fees`:
   - `impl From<BalanceError> for ChangeError<...>`
 

zcash_client_backend/src/data_api.rs

@@ -907,7 +907,7 @@ pub trait InputSource {
         account: Self::AccountId,
         min_value: NonNegativeAmount,
         exclude: &[Self::NoteRef],
-    ) -> Result<WalletMeta, Self::Error>;
+    ) -> Result<Option<WalletMeta>, Self::Error>;
 
     /// Fetches the transparent output corresponding to the provided `outpoint`.
     ///

zcash_client_backend/src/data_api/testing.rs

@@ -46,10 +46,7 @@ use zip32::{fingerprint::SeedFingerprint, DiversifierIndex};
 
 use crate::{
     address::UnifiedAddress,
-    fees::{
-        standard::{self, SingleOutputChangeStrategy},
-        ChangeStrategy, DustOutputPolicy,
-    },
+    fees::{standard::SingleOutputChangeStrategy, ChangeStrategy},
     keys::{UnifiedAddressRequest, UnifiedFullViewingKey, UnifiedSpendingKey},
     proposal::Proposal,
     proto::compact_formats::{
@@ -60,13 +57,15 @@ use crate::{
 };
 
 #[allow(deprecated)]
+use super::wallet::{create_spend_to_address, spend};
+
 use super::{
     chain::{scan_cached_blocks, BlockSource, ChainState, CommitmentTreeRoot, ScanSummary},
     scanning::ScanRange,
     wallet::{
-        create_proposed_transactions, create_spend_to_address,
+        create_proposed_transactions,
         input_selection::{GreedyInputSelector, InputSelector},
-        propose_standard_transfer_to_address, propose_transfer, spend,
+        propose_standard_transfer_to_address, propose_transfer,
     },
     Account, AccountBalance, AccountBirthday, AccountPurpose, AccountSource, BlockMetadata,
     DecryptedTransaction, InputSource, NullifierQuery, ScannedBlock, SeedRelevance,
@@ -877,7 +876,7 @@ where
         fallback_change_pool: ShieldedProtocol,
     ) -> Result<
         NonEmpty<TxId>,
-        super::wallet::TransferErrT<DbT, GreedyInputSelector<DbT>, SingleOutputChangeStrategy<DbT>>,
+        super::wallet::TransferErrT<DbT, GreedyInputSelector<DbT>, SingleOutputChangeStrategy>,
     > {
         let prover = LocalTxProver::bundled();
         let network = self.network().clone();
@@ -977,7 +976,7 @@ where
             DbT,
             CommitmentTreeErrT,
             GreedyInputSelector<DbT>,
-            SingleOutputChangeStrategy<DbT>,
+            SingleOutputChangeStrategy,
         >,
     > {
         let network = self.network().clone();
@@ -1221,20 +1220,6 @@ impl<Cache, DbT: WalletRead + Reset> TestState<Cache, DbT, LocalNetwork> {
     //    }
 }
 
-pub fn single_output_change_strategy<DbT: InputSource>(
-    fee_rule: StandardFeeRule,
-    change_memo: Option<&str>,
-    fallback_change_pool: ShieldedProtocol,
-) -> standard::SingleOutputChangeStrategy<DbT> {
-    let change_memo = change_memo.map(|m| MemoBytes::from(m.parse::<Memo>().unwrap()));
-    standard::SingleOutputChangeStrategy::new(
-        fee_rule,
-        change_memo,
-        fallback_change_pool,
-        DustOutputPolicy::default(),
-    )
-}
-
 // Checks that a protobuf proposal serialized from the provided proposal value correctly parses to
 // the same proposal value.
 fn check_proposal_serialization_roundtrip<DbT: InputSource>(
@@ -2379,7 +2364,7 @@ impl InputSource for MockWalletDb {
         _account: Self::AccountId,
         _min_value: NonNegativeAmount,
         _exclude: &[Self::NoteRef],
-    ) -> Result<WalletMeta, Self::Error> {
+    ) -> Result<Option<WalletMeta>, Self::Error> {
         Err(())
     }
 }

zcash_client_backend/src/data_api/testing/pool.rs

@@ -40,10 +40,7 @@ use crate::{
         self,
         chain::{self, ChainState, CommitmentTreeRoot, ScanSummary},
         error::Error,
-        testing::{
-            single_output_change_strategy, AddressType, FakeCompactOutput, InitialChainState,
-            TestBuilder,
-        },
+        testing::{AddressType, FakeCompactOutput, InitialChainState, TestBuilder},
         wallet::{
             decrypt_and_store_transaction, input_selection::GreedyInputSelector, TransferErrT,
         },
@@ -52,9 +49,7 @@ use crate::{
     },
     decrypt_transaction,
     fees::{
-        self,
-        standard::{self, SingleOutputChangeStrategy},
-        DustOutputPolicy, SplitPolicy,
+        standard::SingleOutputChangeStrategy, CommonChangeStrategy, DustOutputPolicy, SplitPolicy,
     },
     scanning::ScanError,
     wallet::{Note, NoteId, OvkPolicy, ReceivedNote},
@@ -217,14 +212,11 @@ pub fn send_single_step_proposed_transfer<T: ShieldedPoolTester>(
     )])
     .unwrap();
 
-    let fee_rule = StandardFeeRule::Zip317;
-
     let change_memo = "Test change memo".parse::<Memo>().unwrap();
-    let change_strategy = standard::SingleOutputChangeStrategy::new(
-        fee_rule,
+    let change_strategy = CommonChangeStrategy::simple(
+        StandardFeeRule::Zip317,
         Some(change_memo.clone().into()),
         T::SHIELDED_PROTOCOL,
-        DustOutputPolicy::default(),
     );
     let input_selector = GreedyInputSelector::new();
 
@@ -361,7 +353,7 @@ pub fn send_with_multiple_change_outputs<T: ShieldedPoolTester>(
 
     let input_selector = GreedyInputSelector::new();
     let change_memo = "Test change memo".parse::<Memo>().unwrap();
-    let change_strategy = fees::zip317::MultiOutputChangeStrategy::new(
+    let change_strategy = CommonChangeStrategy::new(
         Zip317FeeRule::standard(),
         Some(change_memo.clone().into()),
         T::SHIELDED_PROTOCOL,
@@ -464,7 +456,7 @@ pub fn send_with_multiple_change_outputs<T: ShieldedPoolTester>(
 
     // Now, create another proposal with more outputs requested. We have two change notes;
     // we'll spend one of them, and then we'll generate 7 splits.
-    let change_strategy = fees::zip317::MultiOutputChangeStrategy::new(
+    let change_strategy = CommonChangeStrategy::new(
         Zip317FeeRule::standard(),
         Some(change_memo.into()),
         T::SHIELDED_PROTOCOL,
@@ -1377,7 +1369,7 @@ pub fn ovk_policy_prevents_recovery_from_chain<T: ShieldedPoolTester, DSF>(
         TransferErrT<
             DSF::DataStore,
             GreedyInputSelector<DSF::DataStore>,
-            SingleOutputChangeStrategy<DSF::DataStore>,
+            SingleOutputChangeStrategy,
         >,
     > {
         let min_confirmations = NonZeroU32::new(1).unwrap();
@@ -1587,12 +1579,7 @@ pub fn external_address_change_spends_detected_in_restore_from_seed<T: ShieldedP
 
     #[allow(deprecated)]
     let fee_rule = FixedFeeRule::non_standard(MINIMUM_FEE);
-    let change_strategy = fees::fixed::SingleOutputChangeStrategy::new(
-        fee_rule,
-        None,
-        T::SHIELDED_PROTOCOL,
-        DustOutputPolicy::default(),
-    );
+    let change_strategy = CommonChangeStrategy::simple(fee_rule, None, T::SHIELDED_PROTOCOL);
     let input_selector = GreedyInputSelector::new();
 
     let txid = st
@@ -1681,7 +1668,7 @@ pub fn zip317_spend<T: ShieldedPoolTester, DSF: DataStoreFactory>(
 
     let input_selector = GreedyInputSelector::<DSF::DataStore>::new();
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL);
 
     // This first request will fail due to insufficient non-dust funds
     let req = TransactionRequest::new(vec![Payment::without_memo(
@@ -1780,7 +1767,7 @@ where
 
     let input_selector = GreedyInputSelector::new();
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL);
 
     let txids = st
         .shield_transparent_funds(
@@ -2027,7 +2014,7 @@ pub fn pool_crossing_required<P0: ShieldedPoolTester, P1: ShieldedPoolTester>(
 
     let input_selector = GreedyInputSelector::new();
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL);
     let proposal0 = st
         .propose_transfer(
             account.id(),
@@ -2119,7 +2106,7 @@ pub fn fully_funded_fully_private<P0: ShieldedPoolTester, P1: ShieldedPoolTester
     // We set the default change output pool to P0, because we want to verify later that
     // change is actually sent to P1 (as the transaction is fully fundable from P1).
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL);
     let proposal0 = st
         .propose_transfer(
             account.id(),
@@ -2209,7 +2196,7 @@ pub fn fully_funded_send_to_t<P0: ShieldedPoolTester, P1: ShieldedPoolTester>(
     // We set the default change output pool to P0, because we want to verify later that
     // change is actually sent to P1 (as the transaction is fully fundable from P1).
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P0::SHIELDED_PROTOCOL);
     let proposal0 = st
         .propose_transfer(
             account.id(),
@@ -2307,7 +2294,7 @@ pub fn multi_pool_checkpoint<P0: ShieldedPoolTester, P1: ShieldedPoolTester>(
     // Set up the fee rule and input selector we'll use for all the transfers.
     let input_selector = GreedyInputSelector::new();
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, P1::SHIELDED_PROTOCOL);
 
     // First, send funds just to P0
     let transfer_amount = NonNegativeAmount::const_from_u64(200000);
@@ -2791,10 +2778,9 @@ pub fn scan_cached_blocks_allows_blocks_out_of_order<T: ShieldedPoolTester>(
     )])
     .unwrap();
 
-    #[allow(deprecated)]
     let input_selector = GreedyInputSelector::new();
     let change_strategy =
-        single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL);
+        CommonChangeStrategy::simple(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL);
 
     assert_matches!(
         st.spend(

zcash_client_backend/src/data_api/testing/transparent.rs

@@ -1,11 +1,12 @@
 use crate::{
     data_api::{
-        testing::{AddressType, TestBuilder, TestState},
-        testing::{DataStoreFactory, ShieldedProtocol, TestCache},
+        testing::{
+            AddressType, DataStoreFactory, ShieldedProtocol, TestBuilder, TestCache, TestState,
+        },
         wallet::input_selection::GreedyInputSelector,
         Account as _, InputSource, WalletRead, WalletWrite,
     },
-    fees::{fixed, DustOutputPolicy},
+    fees::CommonChangeStrategy,
     wallet::WalletTransparentOutput,
 };
 use assert_matches::assert_matches;
@@ -198,12 +199,11 @@ where
 
     // Shield the output.
     let input_selector = GreedyInputSelector::new();
-    let change_strategy = fixed::SingleOutputChangeStrategy::new(
+    let change_strategy = CommonChangeStrategy::simple(
         #[allow(deprecated)]
         FixedFeeRule::non_standard(NonNegativeAmount::ZERO),
         None,
         ShieldedProtocol::Sapling,
-        DustOutputPolicy::default(),
     );
     let txid = st
         .shield_transparent_funds(

zcash_client_backend/src/data_api/wallet.rs

@@ -50,7 +50,10 @@ use crate::{
         WalletRead, WalletWrite,
     },
     decrypt_transaction,
-    fees::{standard::SingleOutputChangeStrategy, ChangeStrategy, DustOutputPolicy},
+    fees::{
+        standard::SingleOutputChangeStrategy, ChangeStrategy, CommonChangeStrategy,
+        DustOutputPolicy, SplitPolicy,
+    },
     keys::UnifiedSpendingKey,
     proposal::{Proposal, ProposalError, Step, StepOutputIndex},
     wallet::{Note, OvkPolicy, Recipient},
@@ -301,10 +304,7 @@ pub fn create_spend_to_address<DbT, ParamsT>(
     min_confirmations: NonZeroU32,
     change_memo: Option<MemoBytes>,
     fallback_change_pool: ShieldedProtocol,
-) -> Result<
-    NonEmpty<TxId>,
-    TransferErrT<DbT, GreedyInputSelector<DbT>, SingleOutputChangeStrategy<DbT>>,
->
+) -> Result<NonEmpty<TxId>, TransferErrT<DbT, GreedyInputSelector<DbT>, SingleOutputChangeStrategy>>
 where
     ParamsT: consensus::Parameters + Clone,
     DbT: InputSource,
@@ -533,7 +533,7 @@ pub fn propose_standard_transfer_to_address<DbT, ParamsT, CommitmentTreeErrT>(
         DbT,
         CommitmentTreeErrT,
         GreedyInputSelector<DbT>,
-        SingleOutputChangeStrategy<DbT>,
+        SingleOutputChangeStrategy,
     >,
 >
 where
@@ -559,11 +559,12 @@ where
     );
 
     let input_selector = GreedyInputSelector::<DbT>::new();
-    let change_strategy = SingleOutputChangeStrategy::<DbT>::new(
+    let change_strategy = CommonChangeStrategy::<DbT, _>::new(
         fee_rule,
         change_memo,
         fallback_change_pool,
         DustOutputPolicy::default(),
+        SplitPolicy::single_output(), // TODO: consider changing
     );
 
     propose_transfer(

zcash_client_backend/src/data_api/wallet/input_selection.rs

@@ -617,7 +617,7 @@ impl<DbT: InputSource> InputSelector for GreedyInputSelector<DbT> {
                     &orchard_outputs[..],
                 ),
                 ephemeral_balance.as_ref(),
-                Some(&wallet_meta),
+                wallet_meta.as_ref(),
             );
 
             match balance {
@@ -819,7 +819,7 @@ impl<DbT: InputSource> ShieldingSelector for GreedyInputSelector<DbT> {
             #[cfg(feature = "orchard")]
             &orchard_fees::EmptyBundleView,
             None,
-            Some(&wallet_meta),
+            wallet_meta.as_ref(),
         );
 
         let balance = match trial_balance {
@@ -837,7 +837,7 @@ impl<DbT: InputSource> ShieldingSelector for GreedyInputSelector<DbT> {
                     #[cfg(feature = "orchard")]
                     &orchard_fees::EmptyBundleView,
                     None,
-                    Some(&wallet_meta),
+                    wallet_meta.as_ref(),
                 )?
             }
             Err(other) => return Err(InputSelectorError::Change(other)),