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

Merge pull request #1012 from nuttycom/wallet/proposal_change_memos 

zcash_client_backend: Move change memos into the `ChangeValue` components of `Proposal`s
This commit is contained in:
Kris Nuttycombe 2023-10-12 11:51:08 -06:00 committed by GitHub
parent 0ad81a09c0 bcea060c86
commit c4c2c59211
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 121 additions and 57 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`.

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

@ -118,6 +118,7 @@ where
/// 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
/// not supported.
/// * `change_memo`: A memo to be included in the change output
///
/// # Examples
///
@ -174,7 +175,8 @@ where
/// Amount::from_u64(1).unwrap(),
/// None,
/// OvkPolicy::Sender,
/// 10
/// 10,
/// None
/// )
///
/// # }
@ -196,6 +198,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 +227,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 +338,6 @@ where
ovk_policy,
proposal,
min_confirmations,
None,
)
}
@ -418,7 +420,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 +567,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 +580,7 @@ where
account,
PoolType::Shielded(ShieldedProtocol::Sapling),
),
amount.into(),
value.into(),
Some(memo),
))
}
@ -682,10 +681,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 +698,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 +731,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(),
}
}
}

31
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,17 @@ 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 }
/// Constructs a new [`SingleOutputChangeStrategy`] with the specified fee rule
/// and change memo.
pub fn new(fee_rule: FixedFeeRule, change_memo: Option<MemoBytes>) -> Self {
Self {
fee_rule,
change_memo,
}
}
}
@ -136,7 +142,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 +157,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 +197,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 +217,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 +227,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(

39
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 }
/// fee parameters and change memo.
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(_)
);