zec
/
librustzcash
mirror of https://github.com/zcash/librustzcash.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

zcash_client_backend: Move change memos into the `ChangeValue` components 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.
This commit is contained in:
Kris Nuttycombe 2023-08-01 15:03:24 -06:00
parent 0ad81a09c0
commit 1447d8ea01
9 changed files with 114 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
zcash_client_backend/CHANGELOG.md
View File

@ -26,6 +26,23 @@ and this library adheres to Rust's notion of
an `Amount`.
- All uses of `Amount` in `data_api::wallet::input_selection` have been replaced
with `NonNegativeAmount`.
- `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.
- All uses of `Amount` in `zcash_client_backend::fees` have been replaced
with `NonNegativeAmount`.

21
zcash_client_backend/src/data_api/wallet.rs
View File

@ -196,6 +196,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<
@ -224,7 +225,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,
@ -335,7 +336,6 @@ where
ovk_policy,
proposal,
min_confirmations,
None,
)
}
@ -418,7 +418,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<
@ -566,14 +565,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.into(),
value.into(),
memo.clone(),
)?;
sapling_output_meta.push((
@ -581,7 +578,7 @@ where
account,
PoolType::Shielded(ShieldedProtocol::Sapling),
),
amount.into(),
value.into(),
Some(memo),
))
}
@ -682,10 +679,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
@ -703,7 +696,6 @@ pub fn shield_transparent_funds<DbT, ParamsT, InputsT>(
shielding_threshold: NonNegativeAmount,
usk: &UnifiedSpendingKey,
from_addrs: &[TransparentAddress],
memo: &MemoBytes,
min_confirmations: NonZeroU32,
) -> Result<
TxId,
@ -737,7 +729,6 @@ where
OvkPolicy::Sender,
proposal,
min_confirmations,
Some(memo.clone()),
)
}

20
zcash_client_backend/src/fees.rs
View File

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

28
zcash_client_backend/src/fees/fixed.rs
View File

@ -3,6 +3,7 @@ use std::cmp::Ordering;
use zcash_primitives::{
consensus::{self, BlockHeight},
memo::MemoBytes,
transaction::{
components::{
amount::{Amount, BalanceError, NonNegativeAmount},
@ -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,
}
}
}
@ -136,7 +141,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,
)
.map_err(overflow),
@ -148,7 +156,10 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
}
} else {
TransactionBalance::new(
vec![ChangeValue::Sapling(proposed_change)],
vec![ChangeValue::sapling(
proposed_change,
self.change_memo.clone(),
)],
fee_amount,
)
.map_err(overflow)
@ -185,7 +196,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(
@ -205,8 +216,9 @@ mod tests {
assert_matches!(
result,
Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(NonNegativeAmount::const_from_u64(10000))]
&& balance.fee_required() == NonNegativeAmount::const_from_u64(10000)
Ok(balance) if
balance.proposed_change() == [ChangeValue::sapling(NonNegativeAmount::const_from_u64(10000), None)] &&
balance.fee_required() == NonNegativeAmount::const_from_u64(10000)
);
}
@ -214,7 +226,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(

37
zcash_client_backend/src/fees/zip317.rs
View File

@ -7,6 +7,7 @@ use core::cmp::Ordering;
use zcash_primitives::{
consensus::{self, BlockHeight},
memo::MemoBytes,
transaction::{
components::{
amount::{Amount, BalanceError, NonNegativeAmount},
@ -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,
}
}
}
@ -204,7 +209,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,
)
.map_err(|_| overflow()),
@ -215,8 +223,14 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
.map_err(|_| overflow()),
}
} else {
TransactionBalance::new(vec![ChangeValue::Sapling(proposed_change)], fee_amount)
.map_err(|_| overflow())
TransactionBalance::new(
vec![ChangeValue::sapling(
proposed_change,
self.change_memo.clone(),
)],
fee_amount,
)
.map_err(|_| overflow())
}
}
}
@ -249,7 +263,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(
@ -269,14 +283,15 @@ mod tests {
assert_matches!(
result,
Ok(balance) if balance.proposed_change() == [ChangeValue::Sapling(NonNegativeAmount::const_from_u64(5000))]
&& balance.fee_required() == NonNegativeAmount::const_from_u64(10000)
Ok(balance) if
balance.proposed_change() == [ChangeValue::sapling(NonNegativeAmount::const_from_u64(5000), None)] &&
balance.fee_required() == NonNegativeAmount::const_from_u64(10000)
);
}
#[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(
@ -306,7 +321,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(
@ -339,7 +354,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(

2
zcash_client_sqlite/src/chain.rs
View File

@ -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!(

7
zcash_client_sqlite/src/testing.rs
View File

@ -427,6 +427,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,
@ -435,6 +436,7 @@ impl<Cache> TestState<Cache> {
memo: Option<MemoBytes>,
ovk_policy: OvkPolicy,
min_confirmations: NonZeroU32,
change_memo: Option<MemoBytes>,
) -> Result<
TxId,
data_api::error::Error<
@ -455,6 +457,7 @@ impl<Cache> TestState<Cache> {
memo,
ovk_policy,
min_confirmations,
change_memo,
)
}
@ -563,7 +566,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<
@ -585,7 +587,6 @@ impl<Cache> TestState<Cache> {
ovk_policy,
proposal,
min_confirmations,
change_memo,
)
}
@ -598,7 +599,6 @@ impl<Cache> TestState<Cache> {
shielding_threshold: NonNegativeAmount,
usk: &UnifiedSpendingKey,
from_addrs: &[TransparentAddress],
memo: &MemoBytes,
min_confirmations: NonZeroU32,
) -> Result<
TxId,
@ -621,7 +621,6 @@ impl<Cache> TestState<Cache> {
shielding_threshold,
usk,
from_addrs,
memo,
min_confirmations,
)
}

9
zcash_client_sqlite/src/wallet.rs
View File

@ -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,9 +2159,10 @@ mod tests {
// Shield the output.
let input_selector = GreedyInputSelector::new(
fixed::SingleOutputChangeStrategy::new(FixedFeeRule::non_standard(
NonNegativeAmount::ZERO,
)),
fixed::SingleOutputChangeStrategy::new(
FixedFeeRule::non_standard(NonNegativeAmount::ZERO),
None,
),
DustOutputPolicy::default(),
);
let txid = st
@ -2171,7 +2171,6 @@ mod tests {
value,
&usk,
&[*taddr],
&MemoBytes::empty(),
NonZeroU32::new(1).unwrap(),
)
.unwrap();

26
zcash_client_sqlite/src/wallet/sapling.rs
View File

@ -543,7 +543,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::<Memo>().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(
@ -554,13 +556,11 @@ pub(crate) mod tests {
);
assert_matches!(proposal_result, Ok(_));
let change_memo = "Test change memo".parse::<Memo>().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(_));
@ -670,6 +670,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Err(data_api::error::Error::KeyNotRecognized)
);
@ -697,6 +698,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Err(data_api::error::Error::ScanRequired)
);
@ -763,6 +765,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(10).unwrap(),
None
),
Err(data_api::error::Error::InsufficientFunds {
available,
@ -791,6 +794,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(10).unwrap(),
None
),
Err(data_api::error::Error::InsufficientFunds {
available,
@ -823,6 +827,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(10).unwrap(),
None,
)
.unwrap();
@ -868,6 +873,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Ok(_)
);
@ -881,6 +887,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Err(data_api::error::Error::InsufficientFunds {
available,
@ -909,6 +916,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Err(data_api::error::Error::InsufficientFunds {
available,
@ -939,6 +947,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None,
)
.unwrap();
@ -995,6 +1004,7 @@ pub(crate) mod tests {
None,
ovk_policy,
NonZeroU32::new(1).unwrap(),
None,
)?;
// Fetch the transaction from the database
@ -1082,6 +1092,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Ok(_)
);
@ -1123,6 +1134,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(1).unwrap(),
None
),
Ok(_)
);
@ -1188,7 +1200,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(),
);
@ -1280,7 +1292,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(),
);
@ -1382,7 +1394,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(),
);
@ -1392,7 +1404,6 @@ pub(crate) mod tests {
NonNegativeAmount::from_u64(10000).unwrap(),
&usk,
&[*taddr],
&MemoBytes::empty(),
NonZeroU32::new(1).unwrap()
),
Ok(_)
@ -1548,6 +1559,7 @@ pub(crate) mod tests {
None,
OvkPolicy::Sender,
NonZeroU32::new(5).unwrap(),
None
),
Ok(_)
);