From cc0cc2de8459ec83d4b8e672394533c5f8f8d9ca Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 12 Oct 2023 13:48:56 -0600 Subject: [PATCH 1/3] zcash_primitives: add StandardFeeRule `StandardFeeRule` is an enumeration of the standard fees that have existed in the history of Zcash zips. It is provided to simplify transition to new fee strategies; legacy elements of this enumeration are introduced already-deprecated. --- zcash_client_backend/CHANGELOG.md | 1 + zcash_client_backend/src/data_api/wallet.rs | 17 ++-- zcash_client_backend/src/fees.rs | 23 +++++ zcash_client_backend/src/fees/standard.rs | 101 ++++++++++++++++++++ zcash_client_sqlite/src/testing.rs | 20 ++-- zcash_client_sqlite/src/wallet/sapling.rs | 22 +++-- zcash_primitives/CHANGELOG.md | 2 + zcash_primitives/src/transaction/fees.rs | 40 ++++++++ 8 files changed, 196 insertions(+), 30 deletions(-) create mode 100644 zcash_client_backend/src/fees/standard.rs diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 9c1da9ada..013662a97 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -14,6 +14,7 @@ and this library adheres to Rust's notion of before it is fully supported. - `zcash_client_backend::data_api::error::Error` has new error variant: - `Error::UnsupportedPoolType(zcash_client_backend::data_api::PoolType)` +- Added module `zcash_client_backend::fees::standard` - Added methods to `zcash_client_backend::wallet::ReceivedSaplingNote`: `{from_parts, txid, output_index, diversifier, rseed, note_commitment_tree_position}`. diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 98454d355..4179c3cd9 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -1,7 +1,6 @@ -use std::{convert::Infallible, num::NonZeroU32}; +use std::num::NonZeroU32; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; -use zcash_primitives::transaction::TxId; use zcash_primitives::{ consensus::{self, BlockHeight, NetworkUpgrade}, memo::MemoBytes, @@ -13,9 +12,9 @@ use zcash_primitives::{ }, transaction::{ builder::Builder, - components::amount::{Amount, BalanceError, NonNegativeAmount}, - fees::{fixed, FeeRule}, - Transaction, + components::amount::{Amount, NonNegativeAmount}, + fees::{zip317::FeeError as Zip317FeeError, FeeRule, StandardFeeRule}, + Transaction, TxId, }, zip32::{sapling::DiversifiableFullViewingKey, sapling::ExtendedSpendingKey, AccountId, Scope}, }; @@ -204,8 +203,8 @@ pub fn create_spend_to_address( Error< ::Error, ::Error, - GreedyInputSelectorError, - Infallible, + GreedyInputSelectorError, + Zip317FeeError, >, > where @@ -226,8 +225,8 @@ where ); #[allow(deprecated)] - let fee_rule = fixed::FeeRule::standard(); - let change_strategy = fees::fixed::SingleOutputChangeStrategy::new(fee_rule, change_memo); + let fee_rule = StandardFeeRule::PreZip313; + let change_strategy = fees::standard::SingleOutputChangeStrategy::new(fee_rule, change_memo); spend( wallet_db, params, diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 5ddcffc1b..2f72158d0 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -15,6 +15,7 @@ use zcash_primitives::{ }; pub mod fixed; +pub mod standard; pub mod zip317; /// A proposed change amount and output pool. @@ -118,6 +119,28 @@ pub enum ChangeError { StrategyError(E), } +impl ChangeError { + pub fn map E0>(self, f: F) -> ChangeError { + match self { + ChangeError::InsufficientFunds { + available, + required, + } => ChangeError::InsufficientFunds { + available, + required, + }, + ChangeError::DustInputs { + transparent, + sapling, + } => ChangeError::DustInputs { + transparent, + sapling, + }, + ChangeError::StrategyError(e) => ChangeError::StrategyError(f(e)), + } + } +} + impl fmt::Display for ChangeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs new file mode 100644 index 000000000..c0443cd09 --- /dev/null +++ b/zcash_client_backend/src/fees/standard.rs @@ -0,0 +1,101 @@ +//! Change strategies designed for use with a standard fee. + +use zcash_primitives::{ + consensus::{self, BlockHeight}, + memo::MemoBytes, + transaction::{ + components::{ + amount::NonNegativeAmount, sapling::fees as sapling, transparent::fees as transparent, + }, + fees::{ + fixed::FeeRule as FixedFeeRule, + zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, + StandardFeeRule, + }, + }, +}; + +use super::{fixed, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, TransactionBalance}; + +/// A change strategy that and proposes change as a single output to the most current supported +/// shielded pool and delegates fee calculation to the provided fee rule. +pub struct SingleOutputChangeStrategy { + fee_rule: StandardFeeRule, + change_memo: Option, +} + +impl SingleOutputChangeStrategy { + /// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317 + /// fee parameters. + pub fn new(fee_rule: StandardFeeRule, change_memo: Option) -> Self { + Self { + fee_rule, + change_memo, + } + } +} + +impl ChangeStrategy for SingleOutputChangeStrategy { + type FeeRule = StandardFeeRule; + type Error = Zip317FeeError; + + fn fee_rule(&self) -> &Self::FeeRule { + &self.fee_rule + } + + fn compute_balance( + &self, + params: &P, + target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_inputs: &[impl sapling::InputView], + sapling_outputs: &[impl sapling::OutputView], + dust_output_policy: &DustOutputPolicy, + ) -> Result> { + #[allow(deprecated)] + match self.fee_rule() { + StandardFeeRule::PreZip313 => fixed::SingleOutputChangeStrategy::new( + FixedFeeRule::non_standard(NonNegativeAmount::const_from_u64(10000)), + self.change_memo.clone(), + ) + .compute_balance( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_inputs, + sapling_outputs, + dust_output_policy, + ) + .map_err(|e| e.map(Zip317FeeError::Balance)), + StandardFeeRule::Zip313 => fixed::SingleOutputChangeStrategy::new( + FixedFeeRule::non_standard(NonNegativeAmount::const_from_u64(1000)), + self.change_memo.clone(), + ) + .compute_balance( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_inputs, + sapling_outputs, + dust_output_policy, + ) + .map_err(|e| e.map(Zip317FeeError::Balance)), + StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::new( + Zip317FeeRule::standard(), + self.change_memo.clone(), + ) + .compute_balance( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_inputs, + sapling_outputs, + dust_output_policy, + ), + } + } +} diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index 49dd50a8d..06dc2a456 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -14,20 +14,18 @@ use tempfile::NamedTempFile; #[cfg(feature = "unstable")] use tempfile::TempDir; -use zcash_client_backend::data_api::chain::ScanSummary; -use zcash_client_backend::data_api::{AccountBalance, WalletRead}; #[allow(deprecated)] use zcash_client_backend::{ address::RecipientAddress, data_api::{ self, - chain::{scan_cached_blocks, BlockSource}, + chain::{scan_cached_blocks, BlockSource, ScanSummary}, wallet::{ create_proposed_transaction, create_spend_to_address, input_selection::{GreedyInputSelectorError, InputSelector, Proposal}, propose_transfer, spend, }, - AccountBirthday, WalletSummary, WalletWrite, + AccountBalance, AccountBirthday, WalletRead, WalletSummary, WalletWrite, }, keys::UnifiedSpendingKey, proto::compact_formats::{ @@ -48,8 +46,8 @@ use zcash_primitives::{ Note, Nullifier, PaymentAddress, }, transaction::{ - components::amount::{BalanceError, NonNegativeAmount}, - fees::FeeRule, + components::amount::NonNegativeAmount, + fees::{zip317::FeeError as Zip317FeeError, FeeRule}, Transaction, TxId, }, zip32::{sapling::DiversifiableFullViewingKey, DiversifierIndex}, @@ -442,8 +440,8 @@ impl TestState { data_api::error::Error< SqliteClientError, commitment_tree::Error, - GreedyInputSelectorError, - Infallible, + GreedyInputSelectorError, + Zip317FeeError, >, > { let params = self.network(); @@ -560,7 +558,7 @@ impl TestState { } /// Invokes [`create_proposed_transaction`] with the given arguments. - pub(crate) fn create_proposed_transaction( + pub(crate) fn create_proposed_transaction( &mut self, usk: &UnifiedSpendingKey, ovk_policy: OvkPolicy, @@ -571,7 +569,7 @@ impl TestState { data_api::error::Error< SqliteClientError, commitment_tree::Error, - Infallible, + InputsErrT, FeeRuleT::Error, >, > @@ -579,7 +577,7 @@ impl TestState { FeeRuleT: FeeRule, { let params = self.network(); - create_proposed_transaction::<_, _, Infallible, _>( + create_proposed_transaction( &mut self.db_data, ¶ms, test_prover(), diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index c41a91463..8a9c809a0 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -456,11 +456,12 @@ pub(crate) mod tests { PaymentAddress, }, transaction::{ - components::{ - amount::{BalanceError, NonNegativeAmount}, - Amount, + components::{amount::NonNegativeAmount, Amount}, + fees::{ + fixed::FeeRule as FixedFeeRule, + zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, + StandardFeeRule, }, - fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule}, Transaction, }, zip32::{sapling::ExtendedSpendingKey, Scope}, @@ -477,7 +478,7 @@ pub(crate) mod tests { WalletWrite, }, decrypt_transaction, - fees::{fixed, zip317, DustOutputPolicy}, + fees::{fixed, standard, zip317, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::OvkPolicy, zip321::{self, Payment, TransactionRequest}, @@ -547,10 +548,10 @@ pub(crate) mod tests { }]) .unwrap(); - let fee_rule = FixedFeeRule::standard(); + let fee_rule = StandardFeeRule::PreZip313; let change_memo = "Test change memo".parse::().unwrap(); let change_strategy = - fixed::SingleOutputChangeStrategy::new(fee_rule, Some(change_memo.clone().into())); + standard::SingleOutputChangeStrategy::new(fee_rule, Some(change_memo.clone().into())); let input_selector = &GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()); let proposal_result = st.propose_transfer( @@ -561,7 +562,7 @@ pub(crate) mod tests { ); assert_matches!(proposal_result, Ok(_)); - let create_proposed_result = st.create_proposed_transaction( + let create_proposed_result = st.create_proposed_transaction::( &usk, OvkPolicy::Sender, proposal_result.unwrap(), @@ -655,6 +656,7 @@ pub(crate) mod tests { } #[test] + #[allow(deprecated)] fn create_to_address_fails_on_incorrect_usk() { let mut st = TestBuilder::new() .with_test_account(AccountBirthday::from_sapling_activation) @@ -998,8 +1000,8 @@ pub(crate) mod tests { Error< SqliteClientError, commitment_tree::Error, - GreedyInputSelectorError, - Infallible, + GreedyInputSelectorError, + Zip317FeeError, >, > { let txid = st.create_spend_to_address( diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 4a4ff34fe..ea11f1e97 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -78,6 +78,8 @@ and this library adheres to Rust's notion of - All `const` values (moved to `zcash_primitives::sapling::constants`). - `impl From for u64` +### Added +- `transaction::fees::StandardFeeRule` ## [0.13.0] - 2023-09-25 ### Added - `zcash_primitives::consensus::BlockHeight::saturating_sub` diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index 9ca89b9c6..a2c7a4501 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -56,3 +56,43 @@ pub trait FutureFeeRule: FeeRule { tze_outputs: &[impl tze::OutputView], ) -> Result; } + +/// An enumeration of the standard fee rules supported by the wallet. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum StandardFeeRule { + #[deprecated(note = "It is recommended to use `StandardFeeRule::Zip317` instead.")] + PreZip313, + #[deprecated(note = "It is recommended to use `StandardFeeRule::Zip317` instead.")] + Zip313, + Zip317, +} + +impl FeeRule for StandardFeeRule { + type Error = zip317::FeeError; + + fn fee_required( + &self, + params: &P, + target_height: BlockHeight, + transparent_inputs: &[impl transparent::InputView], + transparent_outputs: &[impl transparent::OutputView], + sapling_input_count: usize, + sapling_output_count: usize, + orchard_action_count: usize, + ) -> Result { + #[allow(deprecated)] + match self { + Self::PreZip313 => Ok(zip317::MINIMUM_FEE), + Self::Zip313 => Ok(NonNegativeAmount::const_from_u64(1000)), + Self::Zip317 => zip317::FeeRule::standard().fee_required( + params, + target_height, + transparent_inputs, + transparent_outputs, + sapling_input_count, + sapling_output_count, + orchard_action_count, + ), + } + } +} From f1c08693a5915ddf034c756cb461757db39aeb67 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 12 Oct 2023 13:49:06 -0600 Subject: [PATCH 2/3] zcash_client_backend: Add propose_standard_transfer. --- zcash_client_backend/src/data_api/wallet.rs | 106 ++++++++-- zcash_client_sqlite/src/testing.rs | 60 +++++- zcash_client_sqlite/src/wallet/sapling.rs | 209 +++++++++++++------- 3 files changed, 288 insertions(+), 87 deletions(-) diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 4179c3cd9..f07b87194 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -1,6 +1,7 @@ use std::num::NonZeroU32; use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree}; + use zcash_primitives::{ consensus::{self, BlockHeight, NetworkUpgrade}, memo::MemoBytes, @@ -212,29 +213,31 @@ where DbT: WalletWrite + WalletCommitmentTrees, DbT::NoteRef: Copy + Eq + Ord, { - let req = zip321::TransactionRequest::new(vec![Payment { - recipient_address: to.clone(), - amount, - memo, - label: None, - message: None, - other_params: vec![], - }]) - .expect( - "It should not be possible for this to violate ZIP 321 request construction invariants.", - ); + let account = wallet_db + .get_account_for_ufvk(&usk.to_unified_full_viewing_key()) + .map_err(Error::DataSource)? + .ok_or(Error::KeyNotRecognized)?; #[allow(deprecated)] - let fee_rule = StandardFeeRule::PreZip313; - let change_strategy = fees::standard::SingleOutputChangeStrategy::new(fee_rule, change_memo); - spend( + let proposal = propose_standard_transfer_to_address( + wallet_db, + params, + StandardFeeRule::PreZip313, + account, + min_confirmations, + to, + amount, + memo, + change_memo, + )?; + + create_proposed_transaction( wallet_db, params, prover, - &GreedyInputSelector::::new(change_strategy, DustOutputPolicy::default()), usk, - req, ovk_policy, + proposal, min_confirmations, ) } @@ -373,6 +376,77 @@ where .map_err(Error::from) } +/// Proposes a transaction paying the specified address from the given account. +/// +/// Returns the proposal, which may then be executed using [`create_proposed_transaction`] +/// +/// Parameters: +/// * `wallet_db`: A read/write reference to the wallet database +/// * `params`: Consensus parameters +/// * `fee_rule`: The fee rule to use in creating the transaction. +/// * `spend_from_account`: The unified account that controls the funds that will be spent +/// in the resulting transaction. This procedure will return an error if the +/// account ID does not correspond to an account known to the wallet. +/// * `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 +/// not supported. +/// * `to`: The address to which `amount` will be paid. +/// * `amount`: The amount to send. +/// * `memo`: A memo to be included in the output to the recipient. +/// * `change_memo`: A memo to be included in any change output that is created. +#[allow(clippy::too_many_arguments)] +#[allow(clippy::type_complexity)] +pub fn propose_standard_transfer_to_address( + wallet_db: &mut DbT, + params: &ParamsT, + fee_rule: StandardFeeRule, + spend_from_account: AccountId, + min_confirmations: NonZeroU32, + to: &RecipientAddress, + amount: NonNegativeAmount, + memo: Option, + change_memo: Option, +) -> Result< + Proposal, + Error< + DbT::Error, + CommitmentTreeErrT, + GreedyInputSelectorError, + Zip317FeeError, + >, +> +where + ParamsT: consensus::Parameters + Clone, + DbT: WalletWrite, + DbT::NoteRef: Copy + Eq + Ord, +{ + let request = zip321::TransactionRequest::new(vec![Payment { + recipient_address: to.clone(), + amount, + memo, + label: None, + message: None, + other_params: vec![], + }]) + .expect( + "It should not be possible for this to violate ZIP 321 request construction invariants.", + ); + + let change_strategy = fees::standard::SingleOutputChangeStrategy::new(fee_rule, change_memo); + let input_selector = + GreedyInputSelector::::new(change_strategy, DustOutputPolicy::default()); + + propose_transfer( + wallet_db, + params, + spend_from_account, + &input_selector, + request, + min_confirmations, + ) +} + #[cfg(feature = "transparent-inputs")] #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index 06dc2a456..acce89adf 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -14,6 +14,7 @@ use tempfile::NamedTempFile; #[cfg(feature = "unstable")] use tempfile::TempDir; +use zcash_client_backend::fees::{standard, DustOutputPolicy}; #[allow(deprecated)] use zcash_client_backend::{ address::RecipientAddress, @@ -22,8 +23,10 @@ use zcash_client_backend::{ chain::{scan_cached_blocks, BlockSource, ScanSummary}, wallet::{ create_proposed_transaction, create_spend_to_address, - input_selection::{GreedyInputSelectorError, InputSelector, Proposal}, - propose_transfer, spend, + input_selection::{ + GreedyInputSelector, GreedyInputSelectorError, InputSelector, Proposal, + }, + propose_standard_transfer_to_address, propose_transfer, spend, }, AccountBalance, AccountBirthday, WalletRead, WalletSummary, WalletWrite, }, @@ -38,7 +41,7 @@ use zcash_note_encryption::Domain; use zcash_primitives::{ block::BlockHash, consensus::{self, BlockHeight, Network, NetworkUpgrade, Parameters}, - memo::MemoBytes, + memo::{Memo, MemoBytes}, sapling::{ note_encryption::{sapling_note_encryption, SaplingDomain}, util::generate_random_rseed, @@ -47,7 +50,7 @@ use zcash_primitives::{ }, transaction::{ components::amount::NonNegativeAmount, - fees::{zip317::FeeError as Zip317FeeError, FeeRule}, + fees::{zip317::FeeError as Zip317FeeError, FeeRule, StandardFeeRule}, Transaction, TxId, }, zip32::{sapling::DiversifiableFullViewingKey, DiversifierIndex}, @@ -524,6 +527,43 @@ impl TestState { ) } + /// Invokes [`propose_standard_transfer`] with the given arguments. + #[allow(clippy::type_complexity)] + #[allow(clippy::too_many_arguments)] + pub(crate) fn propose_standard_transfer( + &mut self, + spend_from_account: AccountId, + fee_rule: StandardFeeRule, + min_confirmations: NonZeroU32, + to: &RecipientAddress, + amount: NonNegativeAmount, + memo: Option, + change_memo: Option, + ) -> Result< + Proposal, + data_api::error::Error< + SqliteClientError, + CommitmentTreeErrT, + GreedyInputSelectorError, + Zip317FeeError, + >, + > { + let params = self.network(); + let result = propose_standard_transfer_to_address::<_, _, CommitmentTreeErrT>( + &mut self.db_data, + ¶ms, + fee_rule, + spend_from_account, + min_confirmations, + to, + amount, + memo, + change_memo, + ); + + result + } + /// Invokes [`propose_shielding`] with the given arguments. #[cfg(feature = "transparent-inputs")] #[allow(clippy::type_complexity)] @@ -993,3 +1033,15 @@ impl TestCache for FsBlockCache { meta } } + +pub(crate) fn input_selector( + fee_rule: StandardFeeRule, + change_memo: Option<&str>, +) -> GreedyInputSelector< + WalletDb, + standard::SingleOutputChangeStrategy, +> { + let change_memo = change_memo.map(|m| MemoBytes::from(m.parse::().unwrap())); + let change_strategy = standard::SingleOutputChangeStrategy::new(fee_rule, change_memo); + GreedyInputSelector::new(change_strategy, DustOutputPolicy::default()) +} diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 8a9c809a0..c0cc7cf98 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -458,9 +458,7 @@ pub(crate) mod tests { transaction::{ components::{amount::NonNegativeAmount, Amount}, fees::{ - fixed::FeeRule as FixedFeeRule, - zip317::{FeeError as Zip317FeeError, FeeRule as Zip317FeeRule}, - StandardFeeRule, + fixed::FeeRule as FixedFeeRule, zip317::FeeError as Zip317FeeError, StandardFeeRule, }, Transaction, }, @@ -478,7 +476,7 @@ pub(crate) mod tests { WalletWrite, }, decrypt_transaction, - fees::{fixed, standard, zip317, DustOutputPolicy}, + fees::{fixed, standard, DustOutputPolicy}, keys::UnifiedSpendingKey, wallet::OvkPolicy, zip321::{self, Payment, TransactionRequest}, @@ -486,7 +484,7 @@ pub(crate) mod tests { use crate::{ error::SqliteClientError, - testing::{AddressType, BlockCache, TestBuilder, TestState}, + testing::{input_selector, AddressType, BlockCache, TestBuilder, TestState}, wallet::{ block_max_scanned, commitment_tree, sapling::select_spendable_sapling_notes, scanning::tests::test_with_canopy_birthday, @@ -689,7 +687,7 @@ pub(crate) mod tests { .with_test_account(AccountBirthday::from_sapling_activation) .build(); - let (_, usk, _) = st.test_account().unwrap(); + let (account, _, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); let to = dfvk.default_address().1.into(); @@ -698,13 +696,13 @@ pub(crate) mod tests { // We cannot do anything if we aren't synchronised assert_matches!( - st.create_spend_to_address( - &usk, + st.propose_standard_transfer::( + account, + StandardFeeRule::PreZip313, + NonZeroU32::new(1).unwrap(), &to, NonNegativeAmount::const_from_u64(1), None, - OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), None ), Err(data_api::error::Error::ScanRequired) @@ -712,7 +710,7 @@ pub(crate) mod tests { } #[test] - fn create_to_address_fails_on_unverified_notes() { + fn spend_fails_on_unverified_notes() { let mut st = TestBuilder::new() .with_block_cache() .with_test_account(AccountBirthday::from_sapling_activation) @@ -765,13 +763,13 @@ pub(crate) mod tests { let extsk2 = ExtendedSpendingKey::master(&[]); let to = extsk2.default_address().1.into(); assert_matches!( - st.create_spend_to_address( - &usk, + st.propose_standard_transfer::( + account, + StandardFeeRule::Zip317, + NonZeroU32::new(10).unwrap(), &to, NonNegativeAmount::const_from_u64(70000), None, - OvkPolicy::Sender, - NonZeroU32::new(10).unwrap(), None ), Err(data_api::error::Error::InsufficientFunds { @@ -794,13 +792,13 @@ pub(crate) mod tests { // Spend still fails assert_matches!( - st.create_spend_to_address( - &usk, + st.propose_standard_transfer::( + account, + StandardFeeRule::Zip317, + NonZeroU32::new(10).unwrap(), &to, NonNegativeAmount::const_from_u64(70000), None, - OvkPolicy::Sender, - NonZeroU32::new(10).unwrap(), None ), Err(data_api::error::Error::InsufficientFunds { @@ -824,20 +822,31 @@ pub(crate) mod tests { (value * 9).unwrap() ); - // Spend should now succeed + // Should now be able to generate a proposal let amount_sent = NonNegativeAmount::from_u64(70000).unwrap(); - let txid = st - .create_spend_to_address( - &usk, + let min_confirmations = NonZeroU32::new(10).unwrap(); + let proposal = st + .propose_standard_transfer::( + account, + StandardFeeRule::Zip317, + min_confirmations, &to, amount_sent, None, - OvkPolicy::Sender, - NonZeroU32::new(10).unwrap(), None, ) .unwrap(); + // Executing the proposal should succeed + let txid = st + .create_proposed_transaction::( + &usk, + OvkPolicy::Sender, + proposal, + min_confirmations, + ) + .unwrap(); + let (h, _) = st.generate_next_block_including(txid); st.scan_cached_blocks(h, 1); @@ -851,7 +860,7 @@ pub(crate) mod tests { } #[test] - fn create_to_address_fails_on_locked_notes() { + fn spend_fails_on_locked_notes() { let mut st = TestBuilder::new() .with_block_cache() .with_test_account(AccountBirthday::from_sapling_activation) @@ -860,6 +869,11 @@ pub(crate) mod tests { let (account, usk, _) = st.test_account().unwrap(); let dfvk = st.test_account_sapling().unwrap(); + // TODO: This test was originally written to use the pre-zip-313 fee rule + // and has not yet been updated. + #[allow(deprecated)] + let fee_rule = StandardFeeRule::PreZip313; + // Add funds to the wallet in a single note let value = NonNegativeAmount::const_from_u64(50000); let (h1, _, _) = st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); @@ -872,28 +886,39 @@ pub(crate) mod tests { // Send some of the funds to another address, but don't mine the tx. let extsk2 = ExtendedSpendingKey::master(&[]); let to = extsk2.default_address().1.into(); - assert_matches!( - st.create_spend_to_address( - &usk, + let min_confirmations = NonZeroU32::new(1).unwrap(); + let proposal = st + .propose_standard_transfer::( + account, + fee_rule, + min_confirmations, &to, NonNegativeAmount::const_from_u64(15000), None, + None, + ) + .unwrap(); + + // Executing the proposal should succeed + assert_matches!( + st.create_proposed_transaction::( + &usk, OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), - None + proposal, + min_confirmations ), Ok(_) ); - // A second spend fails because there are no usable notes + // A second proposal fails because there are no usable notes assert_matches!( - st.create_spend_to_address( - &usk, + st.propose_standard_transfer::( + account, + fee_rule, + NonZeroU32::new(1).unwrap(), &to, NonNegativeAmount::const_from_u64(2000), None, - OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), None ), Err(data_api::error::Error::InsufficientFunds { @@ -914,15 +939,15 @@ pub(crate) mod tests { } st.scan_cached_blocks(h1 + 1, 41); - // Second spend still fails + // Second proposal still fails assert_matches!( - st.create_spend_to_address( - &usk, + st.propose_standard_transfer::( + account, + fee_rule, + NonZeroU32::new(1).unwrap(), &to, NonNegativeAmount::const_from_u64(2000), None, - OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), None ), Err(data_api::error::Error::InsufficientFunds { @@ -945,19 +970,29 @@ pub(crate) mod tests { assert_eq!(st.get_spendable_balance(account, 1), value); // Second spend should now succeed - let amount_sent2 = NonNegativeAmount::from_u64(2000).unwrap(); - let txid2 = st - .create_spend_to_address( - &usk, + let amount_sent2 = NonNegativeAmount::const_from_u64(2000); + let min_confirmations = NonZeroU32::new(1).unwrap(); + let proposal = st + .propose_standard_transfer::( + account, + fee_rule, + min_confirmations, &to, amount_sent2, None, - OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), None, ) .unwrap(); + let txid2 = st + .create_proposed_transaction::( + &usk, + OvkPolicy::Sender, + proposal, + min_confirmations, + ) + .unwrap(); + let (h, _) = st.generate_next_block_including(txid2); st.scan_cached_blocks(h, 1); @@ -992,6 +1027,11 @@ pub(crate) mod tests { let addr2 = extsk2.default_address().1; let to = addr2.into(); + // TODO: This test was originally written to use the pre-zip-313 fee rule + // and has not yet been updated. + #[allow(deprecated)] + let fee_rule = StandardFeeRule::PreZip313; + #[allow(clippy::type_complexity)] let send_and_recover_with_policy = |st: &mut TestState, ovk_policy| @@ -1004,16 +1044,21 @@ pub(crate) mod tests { Zip317FeeError, >, > { - let txid = st.create_spend_to_address( - &usk, + let min_confirmations = NonZeroU32::new(1).unwrap(); + let proposal = st.propose_standard_transfer( + account, + fee_rule, + min_confirmations, &to, NonNegativeAmount::const_from_u64(15000), None, - ovk_policy, - NonZeroU32::new(1).unwrap(), None, )?; + // Executing the proposal should succeed + let txid = + st.create_proposed_transaction(&usk, ovk_policy, proposal, min_confirmations)?; + // Fetch the transaction from the database let raw_tx: Vec<_> = st .wallet() @@ -1071,7 +1116,7 @@ pub(crate) mod tests { } #[test] - fn create_to_address_succeeds_to_t_addr_zero_change() { + fn spend_succeeds_to_t_addr_zero_change() { let mut st = TestBuilder::new() .with_block_cache() .with_test_account(AccountBirthday::from_sapling_activation) @@ -1089,24 +1134,40 @@ pub(crate) mod tests { assert_eq!(st.get_total_balance(account), value); assert_eq!(st.get_spendable_balance(account, 1), value); + // TODO: This test was originally written to use the pre-zip-313 fee rule + // and has not yet been updated. + #[allow(deprecated)] + let fee_rule = StandardFeeRule::PreZip313; + // TODO: generate_next_block_from_tx does not currently support transparent outputs. let to = TransparentAddress::PublicKey([7; 20]).into(); - assert_matches!( - st.create_spend_to_address( - &usk, + let min_confirmations = NonZeroU32::new(1).unwrap(); + let proposal = st + .propose_standard_transfer::( + account, + fee_rule, + min_confirmations, &to, NonNegativeAmount::const_from_u64(50000), None, + None, + ) + .unwrap(); + + // Executing the proposal should succeed + assert_matches!( + st.create_proposed_transaction::( + &usk, OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), - None + proposal, + min_confirmations ), Ok(_) ); } #[test] - fn create_to_address_spends_a_change_note() { + fn change_note_spends_succeed() { let mut st = TestBuilder::new() .with_block_cache() .with_test_account(AccountBirthday::from_sapling_activation) @@ -1131,17 +1192,33 @@ pub(crate) mod tests { NonNegativeAmount::ZERO ); + // TODO: This test was originally written to use the pre-zip-313 fee rule + // and has not yet been updated. + #[allow(deprecated)] + let fee_rule = StandardFeeRule::PreZip313; + // TODO: generate_next_block_from_tx does not currently support transparent outputs. let to = TransparentAddress::PublicKey([7; 20]).into(); - assert_matches!( - st.create_spend_to_address( - &usk, + let min_confirmations = NonZeroU32::new(1).unwrap(); + let proposal = st + .propose_standard_transfer::( + account, + fee_rule, + min_confirmations, &to, NonNegativeAmount::const_from_u64(50000), None, + None, + ) + .unwrap(); + + // Executing the proposal should succeed + assert_matches!( + st.create_proposed_transaction::( + &usk, OvkPolicy::Sender, - NonZeroU32::new(1).unwrap(), - None + proposal, + min_confirmations ), Ok(_) ); @@ -1205,6 +1282,7 @@ pub(crate) mod tests { ]) .unwrap(); + #[allow(deprecated)] let fee_rule = FixedFeeRule::standard(); let input_selector = GreedyInputSelector::new( fixed::SingleOutputChangeStrategy::new(fee_rule, None), @@ -1298,10 +1376,7 @@ pub(crate) mod tests { assert_eq!(st.get_total_balance(account), total); assert_eq!(st.get_spendable_balance(account, 1), total); - let input_selector = GreedyInputSelector::new( - zip317::SingleOutputChangeStrategy::new(Zip317FeeRule::standard(), None), - DustOutputPolicy::default(), - ); + let input_selector = input_selector(StandardFeeRule::Zip317, None); // This first request will fail due to insufficient non-dust funds let req = TransactionRequest::new(vec![Payment { From 570ea4858868fa9cb1a7dfe56044f3c40c48b56d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 26 Oct 2023 14:55:19 -0600 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Daira Emma Hopwood --- zcash_client_backend/src/data_api/wallet.rs | 4 ++-- zcash_client_backend/src/fees/standard.rs | 2 +- zcash_client_sqlite/src/testing.rs | 6 ++---- zcash_primitives/CHANGELOG.md | 1 + zcash_primitives/src/transaction/fees.rs | 8 ++++++-- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index f07b87194..c2de33e26 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -381,8 +381,8 @@ where /// Returns the proposal, which may then be executed using [`create_proposed_transaction`] /// /// Parameters: -/// * `wallet_db`: A read/write reference to the wallet database -/// * `params`: Consensus parameters +/// * `wallet_db`: A read/write reference to the wallet database. +/// * `params`: Consensus parameters. /// * `fee_rule`: The fee rule to use in creating the transaction. /// * `spend_from_account`: The unified account that controls the funds that will be spent /// in the resulting transaction. This procedure will return an error if the diff --git a/zcash_client_backend/src/fees/standard.rs b/zcash_client_backend/src/fees/standard.rs index c0443cd09..572bb814b 100644 --- a/zcash_client_backend/src/fees/standard.rs +++ b/zcash_client_backend/src/fees/standard.rs @@ -17,7 +17,7 @@ use zcash_primitives::{ use super::{fixed, zip317, ChangeError, ChangeStrategy, DustOutputPolicy, TransactionBalance}; -/// A change strategy that and proposes change as a single output to the most current supported +/// A change strategy that proposes change as a single output to the most current supported /// shielded pool and delegates fee calculation to the provided fee rule. pub struct SingleOutputChangeStrategy { fee_rule: StandardFeeRule, diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index acce89adf..c14b6d44f 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -549,7 +549,7 @@ impl TestState { >, > { let params = self.network(); - let result = propose_standard_transfer_to_address::<_, _, CommitmentTreeErrT>( + propose_standard_transfer_to_address::<_, _, CommitmentTreeErrT>( &mut self.db_data, ¶ms, fee_rule, @@ -559,9 +559,7 @@ impl TestState { amount, memo, change_memo, - ); - - result + ) } /// Invokes [`propose_shielding`] with the given arguments. diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index ea11f1e97..c13fd259c 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -80,6 +80,7 @@ and this library adheres to Rust's notion of ### Added - `transaction::fees::StandardFeeRule` + ## [0.13.0] - 2023-09-25 ### Added - `zcash_primitives::consensus::BlockHeight::saturating_sub` diff --git a/zcash_primitives/src/transaction/fees.rs b/zcash_primitives/src/transaction/fees.rs index a2c7a4501..fbdda8f7b 100644 --- a/zcash_primitives/src/transaction/fees.rs +++ b/zcash_primitives/src/transaction/fees.rs @@ -60,9 +60,13 @@ pub trait FutureFeeRule: FeeRule { /// An enumeration of the standard fee rules supported by the wallet. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum StandardFeeRule { - #[deprecated(note = "It is recommended to use `StandardFeeRule::Zip317` instead.")] + #[deprecated( + note = "Using this fee rule violates ZIP 317, and might cause transactions built with it to fail. Use `StandardFeeRule::Zip317` instead." + )] PreZip313, - #[deprecated(note = "It is recommended to use `StandardFeeRule::Zip317` instead.")] + #[deprecated( + note = "Using this fee rule violates ZIP 317, and might cause transactions built with it to fail. Use `StandardFeeRule::Zip317` instead." + )] Zip313, Zip317, }