diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 449efeb5f3..62b85c1c82 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/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 diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 0d1adda6fe..df535446c4 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -199,6 +199,7 @@ pub fn create_spend_to_address( memo: Option, ovk_policy: OvkPolicy, min_confirmations: NonZeroU32, + change_memo: Option, ) -> 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( ovk_policy: OvkPolicy, proposal: Proposal, min_confirmations: NonZeroU32, - change_memo: Option, ) -> Result< TxId, Error< @@ -579,14 +578,12 @@ 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(( @@ -594,7 +591,7 @@ where 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( 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()), ) } diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 050f3dbfa1..ed1720a3a5 100644 --- a/zcash_client_backend/src/fees.rs +++ b/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, + }, } impl ChangeValue { + pub fn sapling(value: Amount, memo: Option) -> Self { + Self::Sapling { value, memo } + } + pub fn value(&self) -> Amount { match self { - ChangeValue::Sapling(value) => *value, + ChangeValue::Sapling { value, .. } => *value, } } } diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 59732f8382..0e32241d1f 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/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, } 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) -> 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( diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 6d2ddc62ab..a65b9cb1de 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/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, } 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) -> 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( diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index b7a7c63e04..1afa9fa3de 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/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!( diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index 386543c10c..dba714a5ef 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -430,6 +430,7 @@ impl TestState { /// 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 TestState { memo: Option, ovk_policy: OvkPolicy, min_confirmations: NonZeroU32, + change_memo: Option, ) -> Result< TxId, data_api::error::Error< @@ -459,6 +461,7 @@ impl TestState { memo, ovk_policy, min_confirmations, + change_memo, ) } @@ -570,7 +573,6 @@ impl TestState { ovk_policy: OvkPolicy, proposal: Proposal, min_confirmations: NonZeroU32, - change_memo: Option, ) -> Result< TxId, data_api::error::Error< @@ -593,7 +595,6 @@ impl TestState { ovk_policy, proposal, min_confirmations, - change_memo, ) } @@ -606,7 +607,6 @@ impl TestState { shielding_threshold: NonNegativeAmount, usk: &UnifiedSpendingKey, from_addrs: &[TransparentAddress], - memo: &MemoBytes, min_confirmations: NonZeroU32, ) -> Result< TxId, @@ -630,7 +630,6 @@ impl TestState { shielding_threshold, usk, from_addrs, - memo, min_confirmations, ) } diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 715d30f7c5..22cafdb93b 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1947,7 +1947,6 @@ mod tests { }, zcash_primitives::{ consensus::BlockHeight, - memo::MemoBytes, transaction::{ components::{amount::NonNegativeAmount, Amount, OutPoint, TxOut}, fees::fixed::FeeRule as FixedFeeRule, @@ -2160,7 +2159,10 @@ mod tests { // Shield the output. let input_selector = GreedyInputSelector::new( - fixed::SingleOutputChangeStrategy::new(FixedFeeRule::non_standard(Amount::zero())), + fixed::SingleOutputChangeStrategy::new( + FixedFeeRule::non_standard(Amount::zero()), + None, + ), DustOutputPolicy::default(), ); let txid = st @@ -2169,7 +2171,6 @@ mod tests { value, &usk, &[*taddr], - &MemoBytes::empty(), NonZeroU32::new(1).unwrap(), ) .unwrap(); diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 6bf84cf007..078fe0a32f 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -539,7 +539,9 @@ pub(crate) mod tests { .unwrap(); let fee_rule = FixedFeeRule::standard(); - let change_strategy = fixed::SingleOutputChangeStrategy::new(fee_rule); + let change_memo = "Test change memo".parse::().unwrap(); + let change_strategy = + fixed::SingleOutputChangeStrategy::new(fee_rule, Some(change_memo.clone().into())); let input_selector = &GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()); let proposal_result = st.propose_transfer( @@ -550,13 +552,11 @@ pub(crate) mod tests { ); assert_matches!(proposal_result, Ok(_)); - let change_memo = "Test change memo".parse::().unwrap(); let create_proposed_result = st.create_proposed_transaction( &usk, OvkPolicy::Sender, proposal_result.unwrap(), NonZeroU32::new(1).unwrap(), - Some(change_memo.clone().into()), ); assert_matches!(create_proposed_result, Ok(_)); @@ -666,6 +666,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Err(data_api::error::Error::KeyNotRecognized) ); @@ -693,6 +694,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Err(data_api::error::Error::ScanRequired) ); @@ -759,6 +761,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(10).unwrap(), + None ), Err(data_api::error::Error::InsufficientFunds { available, @@ -787,6 +790,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(10).unwrap(), + None ), Err(data_api::error::Error::InsufficientFunds { available, @@ -819,6 +823,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(10).unwrap(), + None, ) .unwrap(); @@ -864,6 +869,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Ok(_) ); @@ -877,6 +883,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Err(data_api::error::Error::InsufficientFunds { available, @@ -905,6 +912,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Err(data_api::error::Error::InsufficientFunds { available, @@ -935,6 +943,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None, ) .unwrap(); @@ -992,6 +1001,7 @@ pub(crate) mod tests { None, ovk_policy, NonZeroU32::new(1).unwrap(), + None, )?; // Fetch the transaction from the database @@ -1079,6 +1089,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Ok(_) ); @@ -1120,6 +1131,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(1).unwrap(), + None ), Ok(_) ); @@ -1185,7 +1197,7 @@ pub(crate) mod tests { let fee_rule = FixedFeeRule::standard(); let input_selector = GreedyInputSelector::new( - fixed::SingleOutputChangeStrategy::new(fee_rule), + fixed::SingleOutputChangeStrategy::new(fee_rule, None), DustOutputPolicy::default(), ); @@ -1278,7 +1290,7 @@ pub(crate) mod tests { assert_eq!(st.get_spendable_balance(account, 1), total); let input_selector = GreedyInputSelector::new( - zip317::SingleOutputChangeStrategy::new(Zip317FeeRule::standard()), + zip317::SingleOutputChangeStrategy::new(Zip317FeeRule::standard(), None), DustOutputPolicy::default(), ); @@ -1380,7 +1392,7 @@ pub(crate) mod tests { assert!(matches!(res0, Ok(_))); let input_selector = GreedyInputSelector::new( - fixed::SingleOutputChangeStrategy::new(FixedFeeRule::standard()), + fixed::SingleOutputChangeStrategy::new(FixedFeeRule::standard(), None), DustOutputPolicy::default(), ); @@ -1390,7 +1402,6 @@ pub(crate) mod tests { NonNegativeAmount::from_u64(10000).unwrap(), &usk, &[*taddr], - &MemoBytes::empty(), NonZeroU32::new(1).unwrap() ), Ok(_) @@ -1546,6 +1557,7 @@ pub(crate) mod tests { None, OvkPolicy::Sender, NonZeroU32::new(5).unwrap(), + None ), Ok(_) );