From f7527e14c9763a6786055e38dca37b32ad4bcaaa Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 9 Oct 2023 18:05:48 -0600 Subject: [PATCH 1/2] Use `NonNegativeAmount` for note and utxo value fields --- zcash_client_backend/CHANGELOG.md | 16 ++- zcash_client_backend/src/data_api.rs | 6 +- zcash_client_backend/src/data_api/error.rs | 15 +-- zcash_client_backend/src/data_api/wallet.rs | 6 +- .../src/data_api/wallet/input_selection.rs | 37 +++--- zcash_client_backend/src/fees.rs | 16 +-- zcash_client_backend/src/fees/fixed.rs | 109 ++++++++-------- zcash_client_backend/src/fees/zip317.rs | 117 +++++++++--------- zcash_client_backend/src/wallet.rs | 12 +- zcash_client_backend/src/zip321.rs | 49 +++++--- zcash_client_sqlite/src/chain.rs | 9 +- zcash_client_sqlite/src/lib.rs | 7 +- zcash_client_sqlite/src/wallet.rs | 23 ++-- .../init/migrations/add_transaction_views.rs | 4 +- zcash_client_sqlite/src/wallet/sapling.rs | 39 +++--- zcash_extensions/src/transparent/demo.rs | 12 +- zcash_primitives/CHANGELOG.md | 17 +++ zcash_primitives/src/transaction/builder.rs | 70 ++--------- .../src/transaction/components/amount.rs | 19 ++- .../transaction/components/sapling/builder.rs | 10 +- .../transaction/components/sapling/fees.rs | 6 +- .../src/transaction/components/transparent.rs | 8 +- .../components/transparent/builder.rs | 28 ++--- .../components/transparent/fees.rs | 6 +- .../src/transaction/components/tze.rs | 2 +- zcash_primitives/src/transaction/sighash.rs | 5 +- zcash_primitives/src/transaction/tests.rs | 15 +-- 27 files changed, 337 insertions(+), 326 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 9b81117e5..9c1da9ada 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -27,8 +27,12 @@ and this library adheres to Rust's notion of - The `NoteMismatch` variant of `data_api::error::Error` now wraps a `data_api::NoteId` instead of a backend-specific note identifier. The related `NoteRef` type parameter has been removed from `data_api::error::Error`. - - `wallet::create_spend_to_address` now takes a `NonNegativeAmount` rather than - an `Amount`. + - `SentTransactionOutput::value` is now represented as `NonNegativeAmount` instead of + `Amount`, and constructors and accessors have been updated accordingly. + - The `available` and `required` fields of `data_api::error::Error::InsufficientFunds` + are now represented as `NonNegativeAmount` instead of `Amount`. + - `data_api::wallet::create_spend_to_address` now takes its `amount` argument as + as `NonNegativeAmount` instead of `Amount`. - All uses of `Amount` in `data_api::wallet::input_selection` have been replaced with `NonNegativeAmount`. - `wallet::shield_transparent_funds` no longer @@ -50,6 +54,14 @@ and this library adheres to Rust's notion of accept an additional `change_memo` argument. - All uses of `Amount` in `zcash_client_backend::fees` have been replaced with `NonNegativeAmount`. +- `zcash_client_backend::wallet::WalletTransparentOutput::value` is now represented + as `NonNegativeAmount` instead of `Amount`, and constructors and accessors have been + updated accordingly. +- `zcash_client_backend::wallet::ReceivedSaplingNote::value` is now represented + as `NonNegativeAmount` instead of `Amount`, and constructors and accessors have been + updated accordingly. +- Almost all uses of `Amount` in `zcash_client_backend::zip321` have been replaced + with `NonNegativeAmount`. ## [0.10.0] - 2023-09-25 diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index c710da193..1253dbf98 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -630,7 +630,7 @@ pub enum Recipient { pub struct SentTransactionOutput { output_index: usize, recipient: Recipient, - value: Amount, + value: NonNegativeAmount, memo: Option, sapling_change_to: Option<(AccountId, sapling::Note)>, } @@ -639,7 +639,7 @@ impl SentTransactionOutput { pub fn from_parts( output_index: usize, recipient: Recipient, - value: Amount, + value: NonNegativeAmount, memo: Option, sapling_change_to: Option<(AccountId, sapling::Note)>, ) -> Self { @@ -667,7 +667,7 @@ impl SentTransactionOutput { &self.recipient } /// Returns the value of the newly created output. - pub fn value(&self) -> Amount { + pub fn value(&self) -> NonNegativeAmount { self.value } /// Returns the memo that was attached to the output, if any. This will only be `None` diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index e2b1c5bcf..0fcde032f 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -4,13 +4,11 @@ use std::error; use std::fmt::{self, Debug, Display}; use shardtree::error::ShardTreeError; +use zcash_primitives::transaction::components::amount::NonNegativeAmount; use zcash_primitives::{ transaction::{ builder, - components::{ - amount::{Amount, BalanceError}, - sapling, transparent, - }, + components::{amount::BalanceError, sapling, transparent}, }, zip32::AccountId, }; @@ -45,7 +43,10 @@ pub enum Error { BalanceError(BalanceError), /// Unable to create a new spend because the wallet balance is not sufficient. - InsufficientFunds { available: Amount, required: Amount }, + InsufficientFunds { + available: NonNegativeAmount, + required: NonNegativeAmount, + }, /// The wallet must first perform a scan of the blockchain before other /// operations can be performed. @@ -110,8 +111,8 @@ where Error::InsufficientFunds { available, required } => write!( f, "Insufficient balance (have {}, need {} including fee)", - i64::from(*available), - i64::from(*required) + u64::from(*available), + u64::from(*required) ), Error::ScanRequired => write!(f, "Must scan blocks first"), Error::Builder(e) => write!(f, "An error occurred building the transaction: {}", e), diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 43d72e748..98454d355 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -215,7 +215,7 @@ where { let req = zip321::TransactionRequest::new(vec![Payment { recipient_address: to.clone(), - amount: amount.into(), + amount, memo, label: None, message: None, @@ -591,7 +591,7 @@ where builder.add_sapling_output( internal_ovk(), dfvk.change_address().1, - value.into(), + *value, memo.clone(), )?; sapling_output_meta.push(( @@ -599,7 +599,7 @@ where account, PoolType::Shielded(ShieldedProtocol::Sapling), ), - value.into(), + *value, Some(memo), )) } diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs index 4bdbede17..1c3685ed0 100644 --- a/zcash_client_backend/src/data_api/wallet/input_selection.rs +++ b/zcash_client_backend/src/data_api/wallet/input_selection.rs @@ -10,7 +10,7 @@ use zcash_primitives::{ legacy::TransparentAddress, transaction::{ components::{ - amount::{Amount, BalanceError, NonNegativeAmount}, + amount::{BalanceError, NonNegativeAmount}, sapling::fees as sapling, OutPoint, TxOut, }, @@ -35,7 +35,10 @@ pub enum InputSelectorError { Selection(SelectorErrT), /// Insufficient funds were available to satisfy the payment request that inputs were being /// selected to attempt to satisfy. - InsufficientFunds { available: Amount, required: Amount }, + InsufficientFunds { + available: NonNegativeAmount, + required: NonNegativeAmount, + }, /// The data source does not have enough information to choose an expiry height /// for the transaction. SyncRequired, @@ -60,8 +63,8 @@ impl fmt::Display for InputSelectorError write!( f, "Insufficient balance (have {}, need {} including fee)", - i64::from(*available), - i64::from(*required) + u64::from(*available), + u64::from(*required) ), InputSelectorError::SyncRequired => { write!(f, "Insufficient chain data is available, sync required.") @@ -270,17 +273,17 @@ impl From } } -pub(crate) struct SaplingPayment(Amount); +pub(crate) struct SaplingPayment(NonNegativeAmount); #[cfg(test)] impl SaplingPayment { - pub(crate) fn new(amount: Amount) -> Self { + pub(crate) fn new(amount: NonNegativeAmount) -> Self { SaplingPayment(amount) } } impl sapling::OutputView for SaplingPayment { - fn value(&self) -> Amount { + fn value(&self) -> NonNegativeAmount { self.0 } } @@ -338,10 +341,7 @@ where let mut transparent_outputs = vec![]; let mut sapling_outputs = vec![]; - let mut output_total = Amount::zero(); for payment in transaction_request.payments() { - output_total = (output_total + payment.amount).ok_or(BalanceError::Overflow)?; - let mut push_transparent = |taddr: TransparentAddress| { transparent_outputs.push(TxOut { value: payment.amount, @@ -374,8 +374,8 @@ where } let mut sapling_inputs: Vec> = vec![]; - let mut prior_available = Amount::zero(); - let mut amount_required = Amount::zero(); + let mut prior_available = NonNegativeAmount::ZERO; + let mut amount_required = NonNegativeAmount::ZERO; let mut exclude: Vec = vec![]; // This loop is guaranteed to terminate because on each iteration we check that the amount // of funds selected is strictly increasing. The loop will either return a successful @@ -414,13 +414,18 @@ where } sapling_inputs = wallet_db - .select_spendable_sapling_notes(account, amount_required, anchor_height, &exclude) + .select_spendable_sapling_notes( + account, + amount_required.into(), + anchor_height, + &exclude, + ) .map_err(InputSelectorError::DataSource)?; let new_available = sapling_inputs .iter() .map(|n| n.value()) - .sum::>() + .sum::>() .ok_or(BalanceError::Overflow)?; if new_available <= prior_available { @@ -506,8 +511,8 @@ where }) } else { Err(InputSelectorError::InsufficientFunds { - available: balance.total().into(), - required: shielding_threshold.into(), + available: balance.total(), + required: shielding_threshold, }) } } diff --git a/zcash_client_backend/src/fees.rs b/zcash_client_backend/src/fees.rs index 22987c8f1..5ddcffc1b 100644 --- a/zcash_client_backend/src/fees.rs +++ b/zcash_client_backend/src/fees.rs @@ -5,7 +5,7 @@ use zcash_primitives::{ memo::MemoBytes, transaction::{ components::{ - amount::{Amount, BalanceError, NonNegativeAmount}, + amount::{BalanceError, NonNegativeAmount}, sapling::fees as sapling, transparent::fees as transparent, OutPoint, @@ -100,10 +100,10 @@ pub enum ChangeError { /// required outputs and fees. InsufficientFunds { /// The total of the inputs provided to change selection - available: Amount, + available: NonNegativeAmount, /// The total amount of input value required to fund the requested outputs, /// including the required fees. - required: Amount, + required: NonNegativeAmount, }, /// Some of the inputs provided to the transaction were determined to currently have no /// economic value (i.e. their inclusion in a transaction causes fees to rise in an amount @@ -127,8 +127,8 @@ impl fmt::Display for ChangeError { } => write!( f, "Insufficient funds: required {} zatoshis, but only {} zatoshis were available.", - i64::from(required), - i64::from(available) + u64::from(*required), + u64::from(*available) ), ChangeError::DustInputs { transparent, @@ -235,7 +235,7 @@ pub trait ChangeStrategy { #[cfg(test)] pub(crate) mod tests { use zcash_primitives::transaction::components::{ - amount::Amount, + amount::NonNegativeAmount, sapling::fees as sapling, transparent::{fees as transparent, OutPoint, TxOut}, }; @@ -257,14 +257,14 @@ pub(crate) mod tests { pub(crate) struct TestSaplingInput { pub note_id: u32, - pub value: Amount, + pub value: NonNegativeAmount, } impl sapling::InputView for TestSaplingInput { fn note_id(&self) -> &u32 { &self.note_id } - fn value(&self) -> Amount { + fn value(&self) -> NonNegativeAmount { self.value } } diff --git a/zcash_client_backend/src/fees/fixed.rs b/zcash_client_backend/src/fees/fixed.rs index 1ab4240ff..773f1d8df 100644 --- a/zcash_client_backend/src/fees/fixed.rs +++ b/zcash_client_backend/src/fees/fixed.rs @@ -1,12 +1,11 @@ //! Change strategies designed for use with a fixed fee. -use std::cmp::Ordering; use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, transaction::{ components::{ - amount::{Amount, BalanceError, NonNegativeAmount}, + amount::{BalanceError, NonNegativeAmount}, sapling::fees as sapling, transparent::fees as transparent, }, @@ -89,9 +88,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { ) .unwrap(); // fixed::FeeRule::fee_required is infallible. - let total_in = (t_in + sapling_in) - .and_then(|v| NonNegativeAmount::try_from(v).ok()) - .ok_or(BalanceError::Overflow)?; + let total_in = (t_in + sapling_in).ok_or(BalanceError::Overflow)?; if (!transparent_inputs.is_empty() || !sapling_inputs.is_empty()) && fee_amount > total_in { // For the fixed-fee selection rule, the only time we consider inputs dust is when the fee @@ -109,62 +106,57 @@ impl ChangeStrategy for SingleOutputChangeStrategy { .collect(), }) } else { - let total_out = [t_out, sapling_out, fee_amount.into()] + let total_out = [t_out, sapling_out, fee_amount] .iter() - .sum::>() + .sum::>() .ok_or(BalanceError::Overflow)?; let overflow = |_| ChangeError::StrategyError(BalanceError::Overflow); - let proposed_change = - (Amount::from(total_in) - total_out).ok_or(BalanceError::Underflow)?; - match proposed_change.cmp(&Amount::zero()) { - Ordering::Less => Err(ChangeError::InsufficientFunds { - available: total_in.into(), - required: total_out, - }), - Ordering::Equal => TransactionBalance::new(vec![], fee_amount).map_err(overflow), - Ordering::Greater => { - let proposed_change = NonNegativeAmount::try_from(proposed_change).unwrap(); - let dust_threshold = dust_output_policy - .dust_threshold() - .unwrap_or_else(|| self.fee_rule.fixed_fee()); - if dust_threshold > proposed_change { - match dust_output_policy.action() { - DustAction::Reject => { - let shortfall = (dust_threshold - proposed_change) - .ok_or(BalanceError::Underflow)?; - Err(ChangeError::InsufficientFunds { - available: total_in.into(), - required: (total_in + shortfall) - .ok_or(BalanceError::Overflow)? - .into(), - }) - } - DustAction::AllowDustChange => TransactionBalance::new( - vec![ChangeValue::sapling( - proposed_change, - self.change_memo.clone(), - )], - fee_amount, - ) - .map_err(overflow), - DustAction::AddDustToFee => TransactionBalance::new( - vec![], - (fee_amount + proposed_change).ok_or(BalanceError::Overflow)?, - ) - .map_err(overflow), + let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds { + available: total_in, + required: total_out, + })?; + if proposed_change == NonNegativeAmount::ZERO { + TransactionBalance::new(vec![], fee_amount).map_err(overflow) + } else { + let dust_threshold = dust_output_policy + .dust_threshold() + .unwrap_or_else(|| self.fee_rule.fixed_fee()); + + if dust_threshold > proposed_change { + match dust_output_policy.action() { + DustAction::Reject => { + let shortfall = (dust_threshold - proposed_change) + .ok_or(BalanceError::Underflow)?; + Err(ChangeError::InsufficientFunds { + available: total_in, + required: (total_in + shortfall).ok_or(BalanceError::Overflow)?, + }) } - } else { - TransactionBalance::new( + DustAction::AllowDustChange => TransactionBalance::new( vec![ChangeValue::sapling( proposed_change, self.change_memo.clone(), )], fee_amount, ) - .map_err(overflow) + .map_err(overflow), + DustAction::AddDustToFee => TransactionBalance::new( + vec![], + (fee_amount + proposed_change).ok_or(BalanceError::Overflow)?, + ) + .map_err(overflow), } + } else { + TransactionBalance::new( + vec![ChangeValue::sapling( + proposed_change, + self.change_memo.clone(), + )], + fee_amount, + ) + .map_err(overflow) } } } @@ -176,10 +168,7 @@ mod tests { use zcash_primitives::{ consensus::{Network, NetworkUpgrade, Parameters}, transaction::{ - components::{ - amount::{Amount, NonNegativeAmount}, - transparent::TxOut, - }, + components::{amount::NonNegativeAmount, transparent::TxOut}, fees::fixed::FeeRule as FixedFeeRule, }, }; @@ -209,9 +198,11 @@ mod tests { &Vec::::new(), &[TestSaplingInput { note_id: 0, - value: Amount::from_u64(60000).unwrap(), + value: NonNegativeAmount::const_from_u64(60000), }], - &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))], &DustOutputPolicy::default(), ); @@ -240,22 +231,24 @@ mod tests { &[ TestSaplingInput { note_id: 0, - value: Amount::from_u64(40000).unwrap(), + value: NonNegativeAmount::const_from_u64(40000), }, // enough to pay a fee, plus dust TestSaplingInput { note_id: 0, - value: Amount::from_u64(10100).unwrap(), + value: NonNegativeAmount::const_from_u64(10100), }, ], - &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))], &DustOutputPolicy::default(), ); assert_matches!( result, Err(ChangeError::InsufficientFunds { available, required }) - if available == Amount::from_u64(50100).unwrap() && required == Amount::from_u64(60000).unwrap() + if available == NonNegativeAmount::const_from_u64(50100) && required == NonNegativeAmount::const_from_u64(60000) ); } } diff --git a/zcash_client_backend/src/fees/zip317.rs b/zcash_client_backend/src/fees/zip317.rs index 5b95e0845..4063041ce 100644 --- a/zcash_client_backend/src/fees/zip317.rs +++ b/zcash_client_backend/src/fees/zip317.rs @@ -3,14 +3,13 @@ //! Change selection in ZIP 317 requires careful handling of low-valued inputs //! to ensure that inputs added to a transaction do not cause fees to rise by //! an amount greater than their value. -use core::cmp::Ordering; use zcash_primitives::{ consensus::{self, BlockHeight}, memo::MemoBytes, transaction::{ components::{ - amount::{Amount, BalanceError, NonNegativeAmount}, + amount::{BalanceError, NonNegativeAmount}, sapling::fees as sapling, transparent::fees as transparent, }, @@ -66,7 +65,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { .filter_map(|i| { // for now, we're just assuming p2pkh inputs, so we don't check the size of the input // script - if i.coin().value < self.fee_rule.marginal_fee().into() { + if i.coin().value < self.fee_rule.marginal_fee() { Some(i.outpoint().clone()) } else { None @@ -77,7 +76,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy { let mut sapling_dust: Vec<_> = sapling_inputs .iter() .filter_map(|i| { - if i.value() < self.fee_rule.marginal_fee().into() { + if i.value() < self.fee_rule.marginal_fee() { Some(i.note_id().clone()) } else { None @@ -179,59 +178,56 @@ impl ChangeStrategy for SingleOutputChangeStrategy { let total_in = (t_in + sapling_in).ok_or_else(overflow)?; - let total_out = [t_out, sapling_out, fee_amount.into()] + let total_out = [t_out, sapling_out, fee_amount] .iter() - .sum::>() + .sum::>() .ok_or_else(overflow)?; - let proposed_change = (total_in - total_out).ok_or_else(underflow)?; - match proposed_change.cmp(&Amount::zero()) { - Ordering::Less => Err(ChangeError::InsufficientFunds { - available: total_in, - required: total_out, - }), - Ordering::Equal => TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow()), - Ordering::Greater => { - let proposed_change = NonNegativeAmount::try_from(proposed_change).unwrap(); - let dust_threshold = dust_output_policy - .dust_threshold() - .unwrap_or_else(|| self.fee_rule.marginal_fee()); + let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds { + available: total_in, + required: total_out, + })?; - if dust_threshold > proposed_change { - match dust_output_policy.action() { - DustAction::Reject => { - let shortfall = - (dust_threshold - proposed_change).ok_or_else(underflow)?; + if proposed_change == NonNegativeAmount::ZERO { + TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow()) + } else { + let dust_threshold = dust_output_policy + .dust_threshold() + .unwrap_or_else(|| self.fee_rule.marginal_fee()); - Err(ChangeError::InsufficientFunds { - available: total_in, - required: (total_in + shortfall.into()).ok_or_else(overflow)?, - }) - } - DustAction::AllowDustChange => TransactionBalance::new( - vec![ChangeValue::sapling( - proposed_change, - self.change_memo.clone(), - )], - fee_amount, - ) - .map_err(|_| overflow()), - DustAction::AddDustToFee => TransactionBalance::new( - vec![], - (fee_amount + proposed_change).ok_or_else(overflow)?, - ) - .map_err(|_| overflow()), + if dust_threshold > proposed_change { + match dust_output_policy.action() { + DustAction::Reject => { + let shortfall = (dust_threshold - proposed_change).ok_or_else(underflow)?; + + Err(ChangeError::InsufficientFunds { + available: total_in, + required: (total_in + shortfall).ok_or_else(overflow)?, + }) } - } else { - TransactionBalance::new( + DustAction::AllowDustChange => TransactionBalance::new( vec![ChangeValue::sapling( proposed_change, self.change_memo.clone(), )], fee_amount, ) - .map_err(|_| overflow()) + .map_err(|_| overflow()), + DustAction::AddDustToFee => TransactionBalance::new( + vec![], + (fee_amount + proposed_change).ok_or_else(overflow)?, + ) + .map_err(|_| overflow()), } + } else { + TransactionBalance::new( + vec![ChangeValue::sapling( + proposed_change, + self.change_memo.clone(), + )], + fee_amount, + ) + .map_err(|_| overflow()) } } } @@ -244,10 +240,7 @@ mod tests { consensus::{Network, NetworkUpgrade, Parameters}, legacy::Script, transaction::{ - components::{ - amount::{Amount, NonNegativeAmount}, - transparent::TxOut, - }, + components::{amount::NonNegativeAmount, transparent::TxOut}, fees::zip317::FeeRule as Zip317FeeRule, }, }; @@ -275,9 +268,11 @@ mod tests { &Vec::::new(), &[TestSaplingInput { note_id: 0, - value: Amount::from_u64(55000).unwrap(), + value: NonNegativeAmount::const_from_u64(55000), }], - &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))], &DustOutputPolicy::default(), ); @@ -301,12 +296,12 @@ mod tests { .unwrap(), &Vec::::new(), &[TxOut { - value: Amount::from_u64(40000).unwrap(), + value: NonNegativeAmount::const_from_u64(40000), script_pubkey: Script(vec![]), }], &[TestSaplingInput { note_id: 0, - value: Amount::from_u64(55000).unwrap(), + value: NonNegativeAmount::const_from_u64(55000), }], &Vec::::new(), &DustOutputPolicy::default(), @@ -334,14 +329,16 @@ mod tests { &[ TestSaplingInput { note_id: 0, - value: Amount::from_u64(49000).unwrap(), + value: NonNegativeAmount::const_from_u64(49000), }, TestSaplingInput { note_id: 1, - value: Amount::from_u64(1000).unwrap(), + value: NonNegativeAmount::const_from_u64(1000), }, ], - &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))], &DustOutputPolicy::default(), ); @@ -367,18 +364,20 @@ mod tests { &[ TestSaplingInput { note_id: 0, - value: Amount::from_u64(29000).unwrap(), + value: NonNegativeAmount::const_from_u64(29000), }, TestSaplingInput { note_id: 1, - value: Amount::from_u64(20000).unwrap(), + value: NonNegativeAmount::const_from_u64(20000), }, TestSaplingInput { note_id: 2, - value: Amount::from_u64(1000).unwrap(), + value: NonNegativeAmount::const_from_u64(1000), }, ], - &[SaplingPayment::new(Amount::from_u64(40000).unwrap())], + &[SaplingPayment::new(NonNegativeAmount::const_from_u64( + 40000, + ))], &DustOutputPolicy::default(), ); diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index c7467c8a6..63863f2e0 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -10,9 +10,9 @@ use zcash_primitives::{ sapling, transaction::{ components::{ + amount::NonNegativeAmount, sapling::fees as sapling_fees, transparent::{self, OutPoint, TxOut}, - Amount, }, TxId, }, @@ -69,7 +69,7 @@ impl WalletTransparentOutput { &self.recipient_address } - pub fn value(&self) -> Amount { + pub fn value(&self) -> NonNegativeAmount { self.txout.value } } @@ -181,7 +181,7 @@ pub struct ReceivedSaplingNote { txid: TxId, output_index: u16, diversifier: sapling::Diversifier, - note_value: Amount, + note_value: NonNegativeAmount, rseed: sapling::Rseed, note_commitment_tree_position: Position, } @@ -192,7 +192,7 @@ impl ReceivedSaplingNote { txid: TxId, output_index: u16, diversifier: sapling::Diversifier, - note_value: Amount, + note_value: NonNegativeAmount, rseed: sapling::Rseed, note_commitment_tree_position: Position, ) -> Self { @@ -220,7 +220,7 @@ impl ReceivedSaplingNote { pub fn diversifier(&self) -> sapling::Diversifier { self.diversifier } - pub fn value(&self) -> Amount { + pub fn value(&self) -> NonNegativeAmount { self.note_value } pub fn rseed(&self) -> sapling::Rseed { @@ -236,7 +236,7 @@ impl sapling_fees::InputView for ReceivedSaplingNote &self.note_id } - fn value(&self) -> Amount { + fn value(&self) -> NonNegativeAmount { self.note_value } } diff --git a/zcash_client_backend/src/zip321.rs b/zcash_client_backend/src/zip321.rs index a26c3cdf4..28523f677 100644 --- a/zcash_client_backend/src/zip321.rs +++ b/zcash_client_backend/src/zip321.rs @@ -15,7 +15,7 @@ use nom::{ use zcash_primitives::{ consensus, memo::{self, MemoBytes}, - transaction::components::Amount, + transaction::components::amount::NonNegativeAmount, }; #[cfg(any(test, feature = "test-dependencies"))] @@ -68,7 +68,7 @@ pub struct Payment { /// The payment address to which the payment should be sent. pub recipient_address: RecipientAddress, /// The amount of the payment that is being requested. - pub amount: Amount, + pub amount: NonNegativeAmount, /// A memo that, if included, must be provided with the payment. /// If a memo is present and [`recipient_address`] is not a shielded /// address, the wallet should report an error. @@ -298,7 +298,9 @@ mod render { use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS}; use zcash_primitives::{ - consensus, transaction::components::amount::COIN, transaction::components::Amount, + consensus, + transaction::components::amount::COIN, + transaction::components::{amount::NonNegativeAmount, Amount}, }; use super::{memo_to_base64, MemoBytes, RecipientAddress}; @@ -369,8 +371,8 @@ mod render { /// Constructs an "amount" key/value pair containing the encoded ZEC amount /// at the specified parameter index. - pub fn amount_param(amount: Amount, idx: Option) -> Option { - amount_str(amount).map(|s| format!("amount{}={}", param_index(idx), s)) + pub fn amount_param(amount: NonNegativeAmount, idx: Option) -> Option { + amount_str(amount.into()).map(|s| format!("amount{}={}", param_index(idx), s)) } /// Constructs a "memo" key/value pair containing the base64URI-encoded memo @@ -403,7 +405,9 @@ mod parse { }; use percent_encoding::percent_decode; use zcash_primitives::{ - consensus, transaction::components::amount::COIN, transaction::components::Amount, + consensus, + transaction::components::amount::COIN, + transaction::components::{amount::NonNegativeAmount, Amount}, }; use crate::address::RecipientAddress; @@ -415,7 +419,7 @@ mod parse { #[derive(Debug, PartialEq, Eq)] pub enum Param { Addr(Box), - Amount(Amount), + Amount(NonNegativeAmount), Memo(MemoBytes), Label(String), Message(String), @@ -462,7 +466,7 @@ mod parse { let mut payment = Payment { recipient_address: *addr.ok_or(Zip321Error::RecipientMissing(i))?, - amount: Amount::zero(), + amount: NonNegativeAmount::ZERO, memo: None, label: None, message: None, @@ -618,8 +622,12 @@ mod parse { )), "amount" => parse_amount(value) - .map(|(_, a)| Param::Amount(a)) - .map_err(|e| e.to_string()), + .map_err(|e| e.to_string()) + .and_then(|(_, a)| { + NonNegativeAmount::try_from(a) + .map_err(|_| "Payment amount must be nonnegative.".to_owned()) + }) + .map(Param::Amount), "label" => percent_decode(value.as_bytes()) .decode_utf8() @@ -753,7 +761,7 @@ mod tests { use zcash_primitives::{ consensus::{Parameters, TEST_NETWORK}, memo::Memo, - transaction::components::Amount, + transaction::components::{amount::NonNegativeAmount, Amount}, }; use crate::address::RecipientAddress; @@ -815,7 +823,7 @@ mod tests { payments: vec![ Payment { recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), - amount: Amount::from_u64(376876902796286).unwrap(), + amount: NonNegativeAmount::const_from_u64(376876902796286), memo: None, label: None, message: Some("".to_string()), @@ -836,7 +844,7 @@ mod tests { payments: vec![ Payment { recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), - amount: Amount::from_u64(0).unwrap(), + amount: NonNegativeAmount::ZERO, memo: None, label: None, message: None, @@ -854,7 +862,7 @@ mod tests { payments: vec![ Payment { recipient_address: RecipientAddress::Shielded(decode_payment_address(TEST_NETWORK.hrp_sapling_payment_address(), "ztestsapling1n65uaftvs2g7075q2x2a04shfk066u3lldzxsrprfrqtzxnhc9ps73v4lhx4l9yfxj46sl0q90k").unwrap()), - amount: Amount::from_u64(0).unwrap(), + amount: NonNegativeAmount::ZERO, memo: None, label: None, message: Some("".to_string()), @@ -891,7 +899,7 @@ mod tests { let v1r = TransactionRequest::from_uri(&TEST_NETWORK, valid_1).unwrap(); assert_eq!( v1r.payments.get(0).map(|p| p.amount), - Some(Amount::from_u64(100000000).unwrap()) + Some(NonNegativeAmount::const_from_u64(100000000)) ); let valid_2 = "zcash:?address=tmEZhbWHTpdKMw5it8YDspUXSMGQyFwovpU&amount=123.456&address.1=ztestsapling10yy2ex5dcqkclhc7z7yrnjq2z6feyjad56ptwlfgmy77dmaqqrl9gyhprdx59qgmsnyfska2kez&amount.1=0.789&memo.1=VGhpcyBpcyBhIHVuaWNvZGUgbWVtbyDinKjwn6aE8J-PhvCfjok"; @@ -899,11 +907,11 @@ mod tests { v2r.normalize(&TEST_NETWORK); assert_eq!( v2r.payments.get(0).map(|p| p.amount), - Some(Amount::from_u64(12345600000).unwrap()) + Some(NonNegativeAmount::const_from_u64(12345600000)) ); assert_eq!( v2r.payments.get(1).map(|p| p.amount), - Some(Amount::from_u64(78900000).unwrap()) + Some(NonNegativeAmount::const_from_u64(78900000)) ); // valid; amount just less than MAX_MONEY @@ -912,7 +920,7 @@ mod tests { let v3r = TransactionRequest::from_uri(&TEST_NETWORK, valid_3).unwrap(); assert_eq!( v3r.payments.get(0).map(|p| p.amount), - Some(Amount::from_u64(2099999999999999u64).unwrap()) + Some(NonNegativeAmount::const_from_u64(2099999999999999u64)) ); // valid; MAX_MONEY @@ -921,7 +929,7 @@ mod tests { let v4r = TransactionRequest::from_uri(&TEST_NETWORK, valid_4).unwrap(); assert_eq!( v4r.payments.get(0).map(|p| p.amount), - Some(Amount::from_u64(2100000000000000u64).unwrap()) + Some(NonNegativeAmount::const_from_u64(2100000000000000u64)) ); } @@ -1004,7 +1012,8 @@ mod tests { } #[test] - fn prop_zip321_roundtrip_amount(amt in arb_nonnegative_amount()) { + fn prop_zip321_roundtrip_amount(nn_amt in arb_nonnegative_amount()) { + let amt = Amount::from(nn_amt); let amt_str = amount_str(amt).unwrap(); assert_eq!(amt, parse_amount(&amt_str).unwrap().1); } diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 975674565..011713ca0 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -326,10 +326,7 @@ mod tests { use zcash_primitives::{ block::BlockHash, - transaction::{ - components::{amount::NonNegativeAmount, Amount}, - fees::zip317::FeeRule, - }, + transaction::{components::amount::NonNegativeAmount, fees::zip317::FeeRule}, zip32::ExtendedSpendingKey, }; @@ -518,13 +515,13 @@ mod tests { st.scan_cached_blocks(h2, 1); assert_eq!( st.get_total_balance(AccountId::from(0)), - NonNegativeAmount::from_u64(150_000).unwrap() + NonNegativeAmount::const_from_u64(150_000) ); // We can spend the received notes let req = TransactionRequest::new(vec![Payment { recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), - amount: Amount::from_u64(110_000).unwrap(), + amount: NonNegativeAmount::const_from_u64(110_000), memo: None, label: None, message: None, diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 6025838b5..c35a42d18 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -55,7 +55,10 @@ use zcash_primitives::{ memo::{Memo, MemoBytes}, sapling, transaction::{ - components::{amount::Amount, OutPoint}, + components::{ + amount::{Amount, NonNegativeAmount}, + OutPoint, + }, Transaction, TxId, }, zip32::{AccountId, DiversifierIndex, ExtendedFullViewingKey}, @@ -596,7 +599,7 @@ impl WalletWrite for WalletDb tx_ref, output.index, &recipient, - Amount::from_u64(output.note.value().inner()).map_err(|_| { + NonNegativeAmount::from_u64(output.note.value().inner()).map_err(|_| { SqliteClientError::CorruptedData( "Note value is not a valid Zcash amount.".to_string(), ) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index cb29419b8..a74ee67f0 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1345,7 +1345,10 @@ pub(crate) fn get_unspent_transparent_outputs( let index: u32 = row.get(1)?; let script_pubkey = Script(row.get(2)?); - let value = Amount::from_i64(row.get(3)?).unwrap(); + let value_raw: i64 = row.get(3)?; + let value = NonNegativeAmount::from_nonnegative_i64(value_raw).map_err(|_| { + SqliteClientError::CorruptedData(format!("Negative utxo value: {}", value_raw)) + })?; let height: u32 = row.get(4)?; let outpoint = OutPoint::new(txid_bytes, index); @@ -1642,7 +1645,7 @@ pub(crate) fn put_legacy_transparent_utxo( ":received_by_account": &u32::from(received_by_account), ":address": &output.recipient_address().encode(params), ":script": &output.txout().script_pubkey.0, - ":value_zat": &i64::from(output.txout().value), + ":value_zat": &i64::from(Amount::from(output.txout().value)), ":height": &u32::from(output.height()), ]; @@ -1708,7 +1711,7 @@ pub(crate) fn insert_sent_output( ":from_account": &u32::from(from_account), ":to_address": &to_address, ":to_account": &to_account, - ":value": &i64::from(output.value()), + ":value": &i64::from(Amount::from(output.value())), ":memo": memo_repr(output.memo()) ]; @@ -1736,7 +1739,7 @@ pub(crate) fn put_sent_output( tx_ref: i64, output_index: usize, recipient: &Recipient, - value: Amount, + value: NonNegativeAmount, memo: Option<&MemoBytes>, ) -> Result<(), SqliteClientError> { let mut stmt_upsert_sent_output = conn.prepare_cached( @@ -1762,7 +1765,7 @@ pub(crate) fn put_sent_output( ":from_account": &u32::from(from_account), ":to_address": &to_address, ":to_account": &to_account, - ":value": &i64::from(value), + ":value": &i64::from(Amount::from(value)), ":memo": memo_repr(memo) ]; @@ -2016,7 +2019,7 @@ mod tests { assert!(bal_absent.is_empty()); // Create a fake transparent output. - let value = Amount::from_u64(100000).unwrap(); + let value = NonNegativeAmount::const_from_u64(100000); let outpoint = OutPoint::new([1u8; 32], 1); let txout = TxOut { value, @@ -2064,7 +2067,7 @@ mod tests { assert_matches!( st.wallet().get_transparent_balances(account_id, height_2), - Ok(h) if h.get(taddr) == Some(&value) + Ok(h) if h.get(taddr) == Some(&value.into()) ); // Artificially delete the address from the addresses table so that @@ -2134,8 +2137,8 @@ mod tests { .unwrap() .into_iter() .map(|utxo| utxo.value()) - .sum::>(), - Some(Amount::from(expected)), + .sum::>(), + Some(expected), ); }; @@ -2147,7 +2150,7 @@ mod tests { let value = NonNegativeAmount::from_u64(100000).unwrap(); let outpoint = OutPoint::new([1u8; 32], 1); let txout = TxOut { - value: value.into(), + value, script_pubkey: taddr.script(), }; diff --git a/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs b/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs index d0b422a02..5d838aae2 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/add_transaction_views.rs @@ -396,6 +396,8 @@ mod tests { #[test] #[cfg(feature = "transparent-inputs")] fn migrate_from_wm2() { + use zcash_primitives::transaction::components::amount::NonNegativeAmount; + let network = Network::TestNetwork; let data_file = NamedTempFile::new().unwrap(); let mut db_data = WalletDb::for_path(data_file.path(), network).unwrap(); @@ -419,7 +421,7 @@ mod tests { sequence: 0, }], vout: vec![TxOut { - value: Amount::from_i64(1100000000).unwrap(), + value: NonNegativeAmount::const_from_u64(1100000000), script_pubkey: Script(vec![]), }], authorization: Authorized, diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 6072bb48c..c41a91463 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -9,7 +9,10 @@ use zcash_primitives::{ consensus::BlockHeight, memo::MemoBytes, sapling::{self, Diversifier, Note, Nullifier, Rseed}, - transaction::{components::Amount, TxId}, + transaction::{ + components::{amount::NonNegativeAmount, Amount}, + TxId, + }, zip32::AccountId, }; @@ -97,7 +100,9 @@ fn to_spendable_note(row: &Row) -> Result, S Diversifier(tmp) }; - let note_value = Amount::from_i64(row.get(4)?).unwrap(); + let note_value = NonNegativeAmount::from_nonnegative_i64(row.get(4)?).map_err(|_e| { + SqliteClientError::CorruptedData("Note values must be nonnegative".to_string()) + })?; let rseed = { let rcm_bytes: Vec<_> = row.get(5)?; @@ -534,7 +539,7 @@ pub(crate) mod tests { let to: RecipientAddress = to_extsk.default_address().1.into(); let request = zip321::TransactionRequest::new(vec![Payment { recipient_address: to, - amount: Amount::const_from_i64(10000), + amount: NonNegativeAmount::const_from_u64(10000), memo: None, // this should result in the creation of an empty memo label: None, message: None, @@ -771,8 +776,8 @@ pub(crate) mod tests { available, required }) - if available == Amount::const_from_i64(50000) - && required == Amount::const_from_i64(80000) + if available == NonNegativeAmount::const_from_u64(50000) + && required == NonNegativeAmount::const_from_u64(80000) ); // Mine blocks SAPLING_ACTIVATION_HEIGHT + 2 to 9 until just before the second @@ -800,8 +805,8 @@ pub(crate) mod tests { available, required }) - if available == Amount::const_from_i64(50000) - && required == Amount::const_from_i64(80000) + if available == NonNegativeAmount::const_from_u64(50000) + && required == NonNegativeAmount::const_from_u64(80000) ); // Mine block 11 so that the second note becomes verified @@ -893,7 +898,7 @@ pub(crate) mod tests { available, required }) - if available == Amount::zero() && required == Amount::const_from_i64(12000) + if available == NonNegativeAmount::ZERO && required == NonNegativeAmount::const_from_u64(12000) ); // Mine blocks SAPLING_ACTIVATION_HEIGHT + 1 to 41 (that don't send us funds) @@ -922,7 +927,7 @@ pub(crate) mod tests { available, required }) - if available == Amount::zero() && required == Amount::const_from_i64(12000) + if available == NonNegativeAmount::ZERO && required == NonNegativeAmount::const_from_u64(12000) ); // Mine block SAPLING_ACTIVATION_HEIGHT + 42 so that the first transaction expires @@ -1180,7 +1185,7 @@ pub(crate) mod tests { // payment to an external recipient Payment { recipient_address: RecipientAddress::Shielded(addr2), - amount: amount_sent.into(), + amount: amount_sent, memo: None, label: None, message: None, @@ -1189,7 +1194,7 @@ pub(crate) mod tests { // payment back to the originating wallet, simulating legacy change Payment { recipient_address: RecipientAddress::Shielded(addr), - amount: amount_legacy_change.into(), + amount: amount_legacy_change, memo: None, label: None, message: None, @@ -1299,7 +1304,7 @@ pub(crate) mod tests { // This first request will fail due to insufficient non-dust funds let req = TransactionRequest::new(vec![Payment { recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), - amount: Amount::const_from_i64(50000), + amount: NonNegativeAmount::const_from_u64(50000), memo: None, label: None, message: None, @@ -1316,15 +1321,15 @@ pub(crate) mod tests { NonZeroU32::new(1).unwrap(), ), Err(Error::InsufficientFunds { available, required }) - if available == Amount::const_from_i64(51000) - && required == Amount::const_from_i64(60000) + if available == NonNegativeAmount::const_from_u64(51000) + && required == NonNegativeAmount::const_from_u64(60000) ); // This request will succeed, spending a single dust input to pay the 10000 // ZAT fee in addition to the 41000 ZAT output to the recipient let req = TransactionRequest::new(vec![Payment { recipient_address: RecipientAddress::Shielded(dfvk.default_address().1), - amount: Amount::const_from_i64(41000), + amount: NonNegativeAmount::const_from_u64(41000), memo: None, label: None, message: None, @@ -1350,7 +1355,7 @@ pub(crate) mod tests { // in the total balance. assert_eq!( st.get_total_balance(account), - (total - NonNegativeAmount::from_u64(10000).unwrap()).unwrap() + (total - NonNegativeAmount::const_from_u64(10000)).unwrap() ); } @@ -1383,7 +1388,7 @@ pub(crate) mod tests { let utxo = WalletTransparentOutput::from_parts( OutPoint::new([1u8; 32], 1), TxOut { - value: Amount::const_from_i64(10000), + value: NonNegativeAmount::const_from_u64(10000), script_pubkey: taddr.script(), }, h, diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 6935c8b72..8dbe46f6b 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -489,7 +489,7 @@ mod tests { transaction::{ builder::Builder, components::{ - amount::Amount, + amount::{Amount, NonNegativeAmount}, tze::{Authorized, Bundle, OutPoint, TzeIn, TzeOut}, }, fees::fixed, @@ -828,10 +828,10 @@ mod tests { .add_sapling_spend(extsk, *to.diversifier(), note1, witness1.path().unwrap()) .unwrap(); - let value = Amount::from_u64(100000).unwrap(); + let value = NonNegativeAmount::const_from_u64(100000); let (h1, h2) = demo_hashes(&preimage_1, &preimage_2); builder_a - .demo_open(value, h1) + .demo_open(value.into(), h1) .map_err(|e| format!("open failure: {:?}", e)) .unwrap(); let (tx_a, _) = builder_a @@ -847,9 +847,9 @@ mod tests { let mut builder_b = demo_builder(tx_height + 1); let prevout_a = (OutPoint::new(tx_a.txid(), 0), tze_a.vout[0].clone()); - let value_xfr = (value - fee_rule.fixed_fee().into()).unwrap(); + let value_xfr = (value - fee_rule.fixed_fee()).unwrap(); builder_b - .demo_transfer_to_close(prevout_a, value_xfr, preimage_1, h2) + .demo_transfer_to_close(prevout_a, value_xfr.into(), preimage_1, h2) .map_err(|e| format!("transfer failure: {:?}", e)) .unwrap(); let (tx_b, _) = builder_b @@ -873,7 +873,7 @@ mod tests { builder_c .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - (value_xfr - fee_rule.fixed_fee().into()).unwrap(), + (value_xfr - fee_rule.fixed_fee()).unwrap(), ) .unwrap(); diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 213f77093..4397c220c 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -49,8 +49,25 @@ and this library adheres to Rust's notion of - `fees::fixed::FeeRule::fixed_fee` now wraps a `NonNegativeAmount` instead of an `Amount` - `fees::zip317::FeeRule::marginal_fee` is now represented and exposed as a `NonNegativeAmount` instead of an `Amount` +- `zcash_primitives::transaction::sighash::TransparentAuthorizingContext::input_amounts` now + returns the input values as `NonNegativeAmount` instead of as `Amount` - `zcash_primitives::transaction::components::sapling`: - `MapAuth` trait methods now take `&mut self` instead of `&self`. + - `sapling::fees::InputView::value` now returns a `NonNegativeAmount` instead of an `Amount` + - `sapling::fees::OutputView::value` now returns a `NonNegativeAmount` instead of an `Amount` +- `zcash_primitives::transaction::components::transparent`: + - `transparent::TxOut::value` now has type `NonNegativeAmount` instead of `Amount` + - `transparent::builder::TransparentBuilder::add_output` now takes its `value` + parameter as a `NonNegativeAmount` instead of as an `Amount`. + - `transparent::fees::InputView::value` now returns a `NonNegativeAmount` instead of an `Amount` + - `transparent::fees::OutputView::value` now returns a `NonNegativeAmount` instead of an `Amount` +- The following `zcash_primitives::transaction::builder::Builder` methods + have changed to take a `NonNegativeAmount` for their `value` arguments, + instead of an `Amount`. + - `Builder::add_sapling_output` + - `Builder::add_transparent_output` +- `zcash_primitives::transaction::components::amount::testing::arb_nonnegative_amount` + now returns a `NonNegativeAmount` instead of an `Amount` ### Removed - `zcash_primitives::constants`: diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 24e09f084..fba0bb0fb 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -337,21 +337,14 @@ impl<'a, P: consensus::Parameters, R: RngCore + CryptoRng> Builder<'a, P, R> { &mut self, ovk: Option, to: PaymentAddress, - value: Amount, + value: NonNegativeAmount, memo: MemoBytes, ) -> Result<(), sapling_builder::Error> { - if value.is_negative() { - return Err(sapling_builder::Error::InvalidAmount); - } self.sapling_builder.add_output( &mut self.rng, ovk, to, - NoteValue::from_raw( - value - .try_into() - .expect("Cannot create Sapling outputs with negative note values."), - ), + NoteValue::from_raw(value.into()), memo, ) } @@ -372,7 +365,7 @@ impl<'a, P: consensus::Parameters, R: RngCore + CryptoRng> Builder<'a, P, R> { pub fn add_transparent_output( &mut self, to: &TransparentAddress, - value: Amount, + value: NonNegativeAmount, ) -> Result<(), transparent::builder::Error> { self.transparent_builder.add_output(to, value) } @@ -720,7 +713,6 @@ mod tests { transaction::components::{ amount::{Amount, NonNegativeAmount}, sapling::builder::{self as sapling_builder}, - transparent::builder::{self as transparent_builder}, }, zip32::ExtendedSpendingKey, }; @@ -741,29 +733,6 @@ mod tests { zip32::AccountId, }; - #[test] - fn fails_on_negative_output() { - let extsk = ExtendedSpendingKey::master(&[]); - let dfvk = extsk.to_diversifiable_full_viewing_key(); - let ovk = dfvk.fvk().ovk; - let to = dfvk.default_address().1; - - let sapling_activation_height = TEST_NETWORK - .activation_height(NetworkUpgrade::Sapling) - .unwrap(); - - let mut builder = Builder::new(TEST_NETWORK, sapling_activation_height, None); - assert_eq!( - builder.add_sapling_output( - Some(ovk), - to, - Amount::from_i64(-1).unwrap(), - MemoBytes::empty() - ), - Err(sapling_builder::Error::InvalidAmount) - ); - } - // This test only works with the transparent_inputs feature because we have to // be able to create a tx with a valid balance, without using Sapling inputs. #[test] @@ -795,7 +764,7 @@ mod tests { let tsk = AccountPrivKey::from_seed(&TEST_NETWORK, &[0u8; 32], AccountId::from(0)).unwrap(); let prev_coin = TxOut { - value: Amount::from_u64(50000).unwrap(), + value: NonNegativeAmount::const_from_u64(50000), script_pubkey: tsk .to_account_pubkey() .derive_external_ivk() @@ -816,7 +785,7 @@ mod tests { builder .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - Amount::from_u64(40000).unwrap(), + NonNegativeAmount::const_from_u64(40000), ) .unwrap(); @@ -852,7 +821,7 @@ mod tests { builder .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - Amount::from_u64(40000).unwrap(), + NonNegativeAmount::const_from_u64(40000), ) .unwrap(); @@ -864,21 +833,6 @@ mod tests { ); } - #[test] - fn fails_on_negative_transparent_output() { - let tx_height = TEST_NETWORK - .activation_height(NetworkUpgrade::Sapling) - .unwrap(); - let mut builder = Builder::new(TEST_NETWORK, tx_height, None); - assert_eq!( - builder.add_transparent_output( - &TransparentAddress::PublicKey([0; 20]), - Amount::from_i64(-1).unwrap(), - ), - Err(transparent_builder::Error::InvalidAmount) - ); - } - #[test] fn fails_on_negative_change() { use crate::transaction::fees::zip317::MINIMUM_FEE; @@ -913,7 +867,7 @@ mod tests { .add_sapling_output( ovk, to, - Amount::from_u64(50000).unwrap(), + NonNegativeAmount::const_from_u64(50000), MemoBytes::empty(), ) .unwrap(); @@ -931,7 +885,7 @@ mod tests { builder .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - Amount::from_u64(50000).unwrap(), + NonNegativeAmount::const_from_u64(50000), ) .unwrap(); assert_matches!( @@ -963,14 +917,14 @@ mod tests { .add_sapling_output( ovk, to, - Amount::from_u64(30000).unwrap(), + NonNegativeAmount::const_from_u64(30000), MemoBytes::empty(), ) .unwrap(); builder .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - Amount::from_u64(20000).unwrap(), + NonNegativeAmount::const_from_u64(20000), ) .unwrap(); assert_matches!( @@ -1007,14 +961,14 @@ mod tests { .add_sapling_output( ovk, to, - Amount::from_u64(30000).unwrap(), + NonNegativeAmount::const_from_u64(30000), MemoBytes::empty(), ) .unwrap(); builder .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - Amount::from_u64(20000).unwrap(), + NonNegativeAmount::const_from_u64(20000), ) .unwrap(); assert_matches!( diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index 996b2e7bc..8c4d6fc64 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -281,6 +281,19 @@ impl NonNegativeAmount { let amount = u64::from_le_bytes(bytes); Self::from_u64(amount) } + + /// Reads a NonNegativeAmount from a signed 64-bit little-endian integer. + /// + /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`. + pub fn from_nonnegative_i64_le_bytes(bytes: [u8; 8]) -> Result { + let amount = i64::from_le_bytes(bytes); + Self::from_nonnegative_i64(amount) + } + + /// Returns this NonNegativeAmount encoded as a signed 64-bit little-endian integer. + pub fn to_i64_le_bytes(self) -> [u8; 8] { + self.0.to_i64_le_bytes() + } } impl From for Amount { @@ -400,7 +413,7 @@ impl std::fmt::Display for BalanceError { pub mod testing { use proptest::prelude::prop_compose; - use super::{Amount, MAX_MONEY}; + use super::{Amount, NonNegativeAmount, MAX_MONEY}; prop_compose! { pub fn arb_amount()(amt in -MAX_MONEY..MAX_MONEY) -> Amount { @@ -409,8 +422,8 @@ pub mod testing { } prop_compose! { - pub fn arb_nonnegative_amount()(amt in 0i64..MAX_MONEY) -> Amount { - Amount::from_i64(amt).unwrap() + pub fn arb_nonnegative_amount()(amt in 0i64..MAX_MONEY) -> NonNegativeAmount { + NonNegativeAmount::from_u64(amt as u64).unwrap() } } diff --git a/zcash_primitives/src/transaction/components/sapling/builder.rs b/zcash_primitives/src/transaction/components/sapling/builder.rs index 7890d5143..8687ccb2b 100644 --- a/zcash_primitives/src/transaction/components/sapling/builder.rs +++ b/zcash_primitives/src/transaction/components/sapling/builder.rs @@ -23,7 +23,7 @@ use crate::{ transaction::{ builder::Progress, components::{ - amount::Amount, + amount::{Amount, NonNegativeAmount}, sapling::{ fees, Authorization, Authorized, Bundle, GrothProofBytes, OutputDescription, SpendDescription, @@ -75,9 +75,9 @@ impl fees::InputView<()> for SpendDescriptionInfo { &() } - fn value(&self) -> Amount { + fn value(&self) -> NonNegativeAmount { // An existing note to be spent must have a valid amount value. - Amount::from_u64(self.note.value().inner()).unwrap() + NonNegativeAmount::from_u64(self.note.value().inner()).unwrap() } } @@ -144,8 +144,8 @@ impl SaplingOutputInfo { } impl fees::OutputView for SaplingOutputInfo { - fn value(&self) -> Amount { - Amount::from_u64(self.note.value().inner()) + fn value(&self) -> NonNegativeAmount { + NonNegativeAmount::from_u64(self.note.value().inner()) .expect("Note values should be checked at construction.") } } diff --git a/zcash_primitives/src/transaction/components/sapling/fees.rs b/zcash_primitives/src/transaction/components/sapling/fees.rs index 10d72adb6..61800b1e8 100644 --- a/zcash_primitives/src/transaction/components/sapling/fees.rs +++ b/zcash_primitives/src/transaction/components/sapling/fees.rs @@ -1,7 +1,7 @@ //! Types related to computation of fees and change related to the Sapling components //! of a transaction. -use crate::transaction::components::amount::Amount; +use crate::transaction::components::amount::NonNegativeAmount; /// A trait that provides a minimized view of a Sapling input suitable for use in /// fee and change calculation. @@ -9,12 +9,12 @@ pub trait InputView { /// An identifier for the input being spent. fn note_id(&self) -> &NoteRef; /// The value of the input being spent. - fn value(&self) -> Amount; + fn value(&self) -> NonNegativeAmount; } /// A trait that provides a minimized view of a Sapling output suitable for use in /// fee and change calculation. pub trait OutputView { /// The value of the output being produced. - fn value(&self) -> Amount; + fn value(&self) -> NonNegativeAmount; } diff --git a/zcash_primitives/src/transaction/components/transparent.rs b/zcash_primitives/src/transaction/components/transparent.rs index 988174e2f..e580f4050 100644 --- a/zcash_primitives/src/transaction/components/transparent.rs +++ b/zcash_primitives/src/transaction/components/transparent.rs @@ -7,7 +7,7 @@ use std::io::{self, Read, Write}; use crate::legacy::{Script, TransparentAddress}; -use super::amount::{Amount, BalanceError}; +use super::amount::{Amount, BalanceError, NonNegativeAmount}; pub mod builder; pub mod fees; @@ -82,7 +82,7 @@ impl Bundle { let output_sum = self .vout .iter() - .map(|p| p.value) + .map(|p| Amount::from(p.value)) .sum::>() .ok_or(BalanceError::Overflow)?; @@ -159,7 +159,7 @@ impl TxIn { #[derive(Clone, Debug, PartialEq, Eq)] pub struct TxOut { - pub value: Amount, + pub value: NonNegativeAmount, pub script_pubkey: Script, } @@ -168,7 +168,7 @@ impl TxOut { let value = { let mut tmp = [0u8; 8]; reader.read_exact(&mut tmp)?; - Amount::from_nonnegative_i64_le_bytes(tmp) + NonNegativeAmount::from_nonnegative_i64_le_bytes(tmp) } .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "value out of range"))?; let script_pubkey = Script::read(&mut reader)?; diff --git a/zcash_primitives/src/transaction/components/transparent/builder.rs b/zcash_primitives/src/transaction/components/transparent/builder.rs index 95a8b8949..757f8c8c4 100644 --- a/zcash_primitives/src/transaction/components/transparent/builder.rs +++ b/zcash_primitives/src/transaction/components/transparent/builder.rs @@ -6,7 +6,7 @@ use crate::{ legacy::{Script, TransparentAddress}, transaction::{ components::{ - amount::{Amount, BalanceError}, + amount::{Amount, BalanceError, NonNegativeAmount}, transparent::{self, fees, Authorization, Authorized, Bundle, TxIn, TxOut}, }, sighash::TransparentAuthorizingContext, @@ -134,10 +134,6 @@ impl TransparentBuilder { utxo: OutPoint, coin: TxOut, ) -> Result<(), Error> { - if coin.value.is_negative() { - return Err(Error::InvalidAmount); - } - // Ensure that the RIPEMD-160 digest of the public key associated with the // provided secret key matches that of the address to which the provided // output may be spent. @@ -164,11 +160,11 @@ impl TransparentBuilder { Ok(()) } - pub fn add_output(&mut self, to: &TransparentAddress, value: Amount) -> Result<(), Error> { - if value.is_negative() { - return Err(Error::InvalidAmount); - } - + pub fn add_output( + &mut self, + to: &TransparentAddress, + value: NonNegativeAmount, + ) -> Result<(), Error> { self.vout.push(TxOut { value, script_pubkey: to.script(), @@ -183,20 +179,20 @@ impl TransparentBuilder { .inputs .iter() .map(|input| input.coin.value) - .sum::>() + .sum::>() .ok_or(BalanceError::Overflow)?; #[cfg(not(feature = "transparent-inputs"))] - let input_sum = Amount::zero(); + let input_sum = NonNegativeAmount::ZERO; let output_sum = self .vout .iter() .map(|vo| vo.value) - .sum::>() + .sum::>() .ok_or(BalanceError::Overflow)?; - (input_sum - output_sum).ok_or(BalanceError::Underflow) + (Amount::from(input_sum) - Amount::from(output_sum)).ok_or(BalanceError::Underflow) } pub fn build(self) -> Option> { @@ -241,7 +237,7 @@ impl TxIn { #[cfg(not(feature = "transparent-inputs"))] impl TransparentAuthorizingContext for Unauthorized { - fn input_amounts(&self) -> Vec { + fn input_amounts(&self) -> Vec { vec![] } @@ -252,7 +248,7 @@ impl TransparentAuthorizingContext for Unauthorized { #[cfg(feature = "transparent-inputs")] impl TransparentAuthorizingContext for Unauthorized { - fn input_amounts(&self) -> Vec { + fn input_amounts(&self) -> Vec { return self.inputs.iter().map(|txin| txin.coin.value).collect(); } diff --git a/zcash_primitives/src/transaction/components/transparent/fees.rs b/zcash_primitives/src/transaction/components/transparent/fees.rs index 4b3f4ddb0..12ac0d616 100644 --- a/zcash_primitives/src/transaction/components/transparent/fees.rs +++ b/zcash_primitives/src/transaction/components/transparent/fees.rs @@ -4,7 +4,7 @@ use super::TxOut; use crate::{ legacy::Script, - transaction::{components::amount::Amount, OutPoint}, + transaction::{components::amount::NonNegativeAmount, OutPoint}, }; /// This trait provides a minimized view of a transparent input suitable for use in @@ -20,13 +20,13 @@ pub trait InputView: std::fmt::Debug { /// fee and change computation. pub trait OutputView: std::fmt::Debug { /// Returns the value of the output being created. - fn value(&self) -> Amount; + fn value(&self) -> NonNegativeAmount; /// Returns the script corresponding to the newly created output. fn script_pubkey(&self) -> &Script; } impl OutputView for TxOut { - fn value(&self) -> Amount { + fn value(&self) -> NonNegativeAmount { self.value } diff --git a/zcash_primitives/src/transaction/components/tze.rs b/zcash_primitives/src/transaction/components/tze.rs index e766d2f9f..e038d3eec 100644 --- a/zcash_primitives/src/transaction/components/tze.rs +++ b/zcash_primitives/src/transaction/components/tze.rs @@ -253,7 +253,7 @@ pub mod testing { prop_compose! { pub fn arb_tzeout()(value in arb_nonnegative_amount(), precondition in arb_precondition()) -> TzeOut { - TzeOut { value, precondition } + TzeOut { value: value.into(), precondition } } } diff --git a/zcash_primitives/src/transaction/sighash.rs b/zcash_primitives/src/transaction/sighash.rs index 3fd9b8b2e..9ab8bf409 100644 --- a/zcash_primitives/src/transaction/sighash.rs +++ b/zcash_primitives/src/transaction/sighash.rs @@ -3,6 +3,7 @@ use blake2b_simd::Hash as Blake2bHash; use super::{ components::{ + amount::NonNegativeAmount, sapling::{self, GrothProofBytes}, transparent, Amount, }, @@ -27,7 +28,7 @@ pub enum SignableInput<'a> { index: usize, script_code: &'a Script, script_pubkey: &'a Script, - value: Amount, + value: NonNegativeAmount, }, #[cfg(feature = "zfuture")] Tze { @@ -63,7 +64,7 @@ pub trait TransparentAuthorizingContext: transparent::Authorization { /// so that wallets can commit to the transparent input breakdown /// without requiring the full data of the previous transactions /// providing these inputs. - fn input_amounts(&self) -> Vec; + fn input_amounts(&self) -> Vec; /// Returns the list of all transparent input scriptPubKeys, provided /// so that wallets can commit to the transparent input breakdown /// without requiring the full data of the previous transactions diff --git a/zcash_primitives/src/transaction/tests.rs b/zcash_primitives/src/transaction/tests.rs index 9335d8984..44c0534bc 100644 --- a/zcash_primitives/src/transaction/tests.rs +++ b/zcash_primitives/src/transaction/tests.rs @@ -3,10 +3,11 @@ use std::ops::Deref; use proptest::prelude::*; -use crate::{consensus::BranchId, legacy::Script}; +use crate::{ + consensus::BranchId, legacy::Script, transaction::components::amount::NonNegativeAmount, +}; use super::{ - components::Amount, sapling, sighash::{ SignableInput, TransparentAuthorizingContext, SIGHASH_ALL, SIGHASH_ANYONECANPAY, @@ -134,7 +135,7 @@ fn zip_0143() { index: n as usize, script_code: &tv.script_code, script_pubkey: &tv.script_code, - value: Amount::from_nonnegative_i64(tv.amount).unwrap(), + value: NonNegativeAmount::from_nonnegative_i64(tv.amount).unwrap(), }, _ => SignableInput::Shielded, }; @@ -156,7 +157,7 @@ fn zip_0243() { index: n as usize, script_code: &tv.script_code, script_pubkey: &tv.script_code, - value: Amount::from_nonnegative_i64(tv.amount).unwrap(), + value: NonNegativeAmount::from_nonnegative_i64(tv.amount).unwrap(), }, _ => SignableInput::Shielded, }; @@ -170,7 +171,7 @@ fn zip_0243() { #[derive(Debug)] struct TestTransparentAuth { - input_amounts: Vec, + input_amounts: Vec, input_scriptpubkeys: Vec