Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an enumeration of standard fee rules. #1018

Merged
merged 3 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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}`.

Expand Down
117 changes: 95 additions & 22 deletions zcash_client_backend/src/data_api/wallet.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
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,
Expand All @@ -13,9 +13,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},
};
Expand Down Expand Up @@ -204,38 +204,40 @@ pub fn create_spend_to_address<DbT, ParamsT>(
Error<
<DbT as WalletRead>::Error,
<DbT as WalletCommitmentTrees>::Error,
GreedyInputSelectorError<BalanceError, DbT::NoteRef>,
Infallible,
GreedyInputSelectorError<Zip317FeeError, DbT::NoteRef>,
Zip317FeeError,
>,
Comment on lines +207 to +208
Copy link
Contributor

@str4d str4d Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change to a public API that should be documented in the changelog.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #1030.

>
where
ParamsT: consensus::Parameters + Clone,
DbT: WalletWrite + WalletCommitmentTrees,
DbT::NoteRef: Copy + Eq + Ord,
{
let req = zip321::TransactionRequest::new(vec![Payment {
recipient_address: to.clone(),
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 proposal = propose_standard_transfer_to_address(
wallet_db,
params,
StandardFeeRule::PreZip313,
account,
min_confirmations,
to,
amount,
memo,
label: None,
message: None,
other_params: vec![],
}])
.expect(
"It should not be possible for this to violate ZIP 321 request construction invariants.",
);
change_memo,
)?;

#[allow(deprecated)]
let fee_rule = fixed::FeeRule::standard();
let change_strategy = fees::fixed::SingleOutputChangeStrategy::new(fee_rule, change_memo);
spend(
create_proposed_transaction(
wallet_db,
params,
prover,
&GreedyInputSelector::<DbT, _>::new(change_strategy, DustOutputPolicy::default()),
usk,
req,
ovk_policy,
proposal,
min_confirmations,
)
}
Expand Down Expand Up @@ -374,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.
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
/// * `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<DbT, ParamsT, CommitmentTreeErrT>(
wallet_db: &mut DbT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition should be documented in the changelog.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #1030.

params: &ParamsT,
fee_rule: StandardFeeRule,
spend_from_account: AccountId,
min_confirmations: NonZeroU32,
to: &RecipientAddress,
amount: NonNegativeAmount,
memo: Option<MemoBytes>,
change_memo: Option<MemoBytes>,
) -> Result<
Proposal<StandardFeeRule, DbT::NoteRef>,
Error<
DbT::Error,
CommitmentTreeErrT,
GreedyInputSelectorError<Zip317FeeError, DbT::NoteRef>,
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::<DbT, _>::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)]
Expand Down
23 changes: 23 additions & 0 deletions zcash_client_backend/src/fees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
};

pub mod fixed;
pub mod standard;
pub mod zip317;

/// A proposed change amount and output pool.
Expand Down Expand Up @@ -118,6 +119,28 @@
StrategyError(E),
}

impl<E, NoteRefT> ChangeError<E, NoteRefT> {
pub fn map<E0, F: FnOnce(E) -> E0>(self, f: F) -> ChangeError<E0, NoteRefT> {
Copy link
Contributor

@str4d str4d Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition should be documented in the changelog, or removed from the public API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #1030.

match self {
ChangeError::InsufficientFunds {

Check warning on line 125 in zcash_client_backend/src/fees.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees.rs#L125

Added line #L125 was not covered by tests
available,
required,
} => ChangeError::InsufficientFunds {
available,
required,
},
ChangeError::DustInputs {
transparent,
sapling,

Check warning on line 134 in zcash_client_backend/src/fees.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees.rs#L132-L134

Added lines #L132 - L134 were not covered by tests
} => ChangeError::DustInputs {
transparent,
sapling,
},
ChangeError::StrategyError(e) => ChangeError::StrategyError(f(e)),

Check warning on line 139 in zcash_client_backend/src/fees.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees.rs#L139

Added line #L139 was not covered by tests
}
}
}

impl<CE: fmt::Display, N: fmt::Display> fmt::Display for ChangeError<CE, N> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match &self {
Expand Down
101 changes: 101 additions & 0 deletions zcash_client_backend/src/fees/standard.rs
Original file line number Diff line number Diff line change
@@ -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
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
/// shielded pool and delegates fee calculation to the provided fee rule.
pub struct SingleOutputChangeStrategy {
fee_rule: StandardFeeRule,
change_memo: Option<MemoBytes>,
}

impl SingleOutputChangeStrategy {
/// Constructs a new [`SingleOutputChangeStrategy`] with the specified ZIP 317
/// fee parameters.
pub fn new(fee_rule: StandardFeeRule, change_memo: Option<MemoBytes>) -> 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<P: consensus::Parameters, NoteRefT: Clone>(
&self,
params: &P,
target_height: BlockHeight,
transparent_inputs: &[impl transparent::InputView],
transparent_outputs: &[impl transparent::OutputView],
sapling_inputs: &[impl sapling::InputView<NoteRefT>],
sapling_outputs: &[impl sapling::OutputView],
dust_output_policy: &DustOutputPolicy,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
#[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(),

Check warning on line 74 in zcash_client_backend/src/fees/standard.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/standard.rs#L73-L74

Added lines #L73 - L74 were not covered by tests
)
.compute_balance(
params,
target_height,
transparent_inputs,
transparent_outputs,
sapling_inputs,
sapling_outputs,
dust_output_policy,

Check warning on line 83 in zcash_client_backend/src/fees/standard.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/standard.rs#L77-L83

Added lines #L77 - L83 were not covered by tests
)
Comment on lines +76 to +84
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'll ever get used to the need for this kind of duplication in Rust :-/ I hope there's some language improvement that addresses it.

.map_err(|e| e.map(Zip317FeeError::Balance)),
StandardFeeRule::Zip317 => zip317::SingleOutputChangeStrategy::new(

Check warning on line 86 in zcash_client_backend/src/fees/standard.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/standard.rs#L85-L86

Added lines #L85 - L86 were not covered by tests
Zip317FeeRule::standard(),
self.change_memo.clone(),
)
.compute_balance(
params,
target_height,
transparent_inputs,
transparent_outputs,
sapling_inputs,
sapling_outputs,
dust_output_policy,
),
}
}
}
Loading
Loading