Merge pull request #1014 from nuttycom/wallet/nonnegative_txos

Use `NonNegativeAmount` for note and UTXO value fields

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
+  - `SentTransactionOutput::value` is now represented as `NonNegativeAmount` instead of
-    an `Amount`.
+    `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
 

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<MemoBytes>,
     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<MemoBytes>,
         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`

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::{
+        components::{amount::BalanceError, sapling, transparent},
-            amount::{Amount, BalanceError},
-            sapling, transparent,
-        },
     },
     zip32::AccountId,
 };
@@ -45,7 +43,10 @@ pub enum Error<DataSourceError, CommitmentTreeError, SelectionError, FeeError> {
     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),
+                u64::from(*available),
-                i64::from(*required)
+                u64::from(*required)
             ),
             Error::ScanRequired => write!(f, "Must scan blocks first"),
             Error::Builder(e) => write!(f, "An error occurred building the transaction: {}", e),

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),
                 ))
             }

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<DbErrT, SelectorErrT> {
     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<DE: fmt::Display, SE: fmt::Display> fmt::Display for InputSelectorError<DE,
             } => write!(
                 f,
                 "Insufficient balance (have {}, need {} including fee)",
-                i64::from(*available),
+                u64::from(*available),
-                i64::from(*required)
+                u64::from(*required)
             ),
             InputSelectorError::SyncRequired => {
                 write!(f, "Insufficient chain data is available, sync required.")
@@ -270,17 +273,17 @@ impl<DbErrT, ChangeStrategyErrT, NoteRefT> From<BalanceError>
     }
 }
 
-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<ReceivedSaplingNote<DbT::NoteRef>> = vec![];
-        let mut prior_available = Amount::zero();
+        let mut prior_available = NonNegativeAmount::ZERO;
-        let mut amount_required = Amount::zero();
+        let mut amount_required = NonNegativeAmount::ZERO;
         let mut exclude: Vec<DbT::NoteRef> = 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::<Option<Amount>>()
+                .sum::<Option<NonNegativeAmount>>()
                 .ok_or(BalanceError::Overflow)?;
 
             if new_available <= prior_available {
@@ -506,8 +511,8 @@ where
             })
         } else {
             Err(InputSelectorError::InsufficientFunds {
-                available: balance.total().into(),
+                available: balance.total(),
-                required: shielding_threshold.into(),
+                required: shielding_threshold,
             })
         }
     }

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<E, NoteRefT> {
     /// 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<CE: fmt::Display, N: fmt::Display> fmt::Display for ChangeError<CE, N> {
             } => write!(
                 f,
                 "Insufficient funds: required {} zatoshis, but only {} zatoshis were available.",
-                i64::from(required),
+                u64::from(*required),
-                i64::from(available)
+                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<u32> for TestSaplingInput {
         fn note_id(&self) -> &u32 {
             &self.note_id
         }
-        fn value(&self) -> Amount {
+        fn value(&self) -> NonNegativeAmount {
             self.value
         }
     }

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)
+        let total_in = (t_in + sapling_in).ok_or(BalanceError::Overflow)?;
-            .and_then(|v| NonNegativeAmount::try_from(v).ok())
-            .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::<Option<Amount>>()
+                .sum::<Option<NonNegativeAmount>>()
                 .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 {
+            let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds {
-                        match dust_output_policy.action() {
+                available: total_in,
-                            DustAction::Reject => {
+                required: total_out,
-                                let shortfall = (dust_threshold - proposed_change)
+            })?;
-                                    .ok_or(BalanceError::Underflow)?;
+            if proposed_change == NonNegativeAmount::ZERO {
-                                Err(ChangeError::InsufficientFunds {
+                TransactionBalance::new(vec![], fee_amount).map_err(overflow)
-                                    available: total_in.into(),
+            } else {
-                                    required: (total_in + shortfall)
+                let dust_threshold = dust_output_policy
-                                        .ok_or(BalanceError::Overflow)?
+                    .dust_threshold()
-                                        .into(),
+                    .unwrap_or_else(|| self.fee_rule.fixed_fee());
-                                })
+
-                            }
+                if dust_threshold > proposed_change {
-                            DustAction::AllowDustChange => TransactionBalance::new(
+                    match dust_output_policy.action() {
-                                vec![ChangeValue::sapling(
+                        DustAction::Reject => {
-                                    proposed_change,
+                            let shortfall = (dust_threshold - proposed_change)
-                                    self.change_memo.clone(),
+                                .ok_or(BalanceError::Underflow)?;
-                                )],
+                            Err(ChangeError::InsufficientFunds {
-                                fee_amount,
+                                available: total_in,
-                            )
+                                required: (total_in + shortfall).ok_or(BalanceError::Overflow)?,
-                            .map_err(overflow),
+                            })
-                            DustAction::AddDustToFee => TransactionBalance::new(
-                                vec![],
-                                (fee_amount + proposed_change).ok_or(BalanceError::Overflow)?,
-                            )
-                            .map_err(overflow),
                         }
-                    } else {
+                        DustAction::AllowDustChange => TransactionBalance::new(
-                        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::{
+            components::{amount::NonNegativeAmount, transparent::TxOut},
-                amount::{Amount, NonNegativeAmount},
-                transparent::TxOut,
-            },
             fees::fixed::FeeRule as FixedFeeRule,
         },
     };
@@ -209,9 +198,11 @@ mod tests {
             &Vec::<TxOut>::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)
         );
     }
 }

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::<Option<Amount>>()
+            .sum::<Option<NonNegativeAmount>>()
             .ok_or_else(overflow)?;
 
-        let proposed_change = (total_in - total_out).ok_or_else(underflow)?;
+        let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds {
-        match proposed_change.cmp(&Amount::zero()) {
+            available: total_in,
-            Ordering::Less => Err(ChangeError::InsufficientFunds {
+            required: total_out,
-                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());
 
-                if dust_threshold > proposed_change {
+        if proposed_change == NonNegativeAmount::ZERO {
-                    match dust_output_policy.action() {
+            TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow())
-                        DustAction::Reject => {
+        } else {
-                            let shortfall =
+            let dust_threshold = dust_output_policy
-                                (dust_threshold - proposed_change).ok_or_else(underflow)?;
+                .dust_threshold()
+                .unwrap_or_else(|| self.fee_rule.marginal_fee());
 
-                            Err(ChangeError::InsufficientFunds {
+            if dust_threshold > proposed_change {
-                                available: total_in,
+                match dust_output_policy.action() {
-                                required: (total_in + shortfall.into()).ok_or_else(overflow)?,
+                    DustAction::Reject => {
-                            })
+                        let shortfall = (dust_threshold - proposed_change).ok_or_else(underflow)?;
-                        }
+
-                        DustAction::AllowDustChange => TransactionBalance::new(
+                        Err(ChangeError::InsufficientFunds {
-                            vec![ChangeValue::sapling(
+                            available: total_in,
-                                proposed_change,
+                            required: (total_in + shortfall).ok_or_else(overflow)?,
-                                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()),
                     }
-                } else {
+                    DustAction::AllowDustChange => TransactionBalance::new(
-                    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::{
+            components::{amount::NonNegativeAmount, transparent::TxOut},
-                amount::{Amount, NonNegativeAmount},
-                transparent::TxOut,
-            },
             fees::zip317::FeeRule as Zip317FeeRule,
         },
     };
@@ -275,9 +268,11 @@ mod tests {
             &Vec::<TxOut>::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::<TestTransparentInput>::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::<SaplingPayment>::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(),
         );
 

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<NoteRef> {
     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<NoteRef> ReceivedSaplingNote<NoteRef> {
         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<NoteRef> ReceivedSaplingNote<NoteRef> {
     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<NoteRef> sapling_fees::InputView<NoteRef> for ReceivedSaplingNote<NoteRef>
         &self.note_id
     }
 
-    fn value(&self) -> Amount {
+    fn value(&self) -> NonNegativeAmount {
         self.note_value
     }
 }

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<usize>) -> Option<String> {
+    pub fn amount_param(amount: NonNegativeAmount, idx: Option<usize>) -> Option<String> {
-        amount_str(amount).map(|s| format!("amount{}={}", param_index(idx), s))
+        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<RecipientAddress>),
-        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);
         }

zcash_client_sqlite/src/chain.rs

@@ -326,10 +326,7 @@ mod tests {
 
     use zcash_primitives::{
         block::BlockHash,
-        transaction::{
+        transaction::{components::amount::NonNegativeAmount, fees::zip317::FeeRule},
-            components::{amount::NonNegativeAmount, Amount},
-            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,

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<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
                             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(),
                                 )

zcash_client_sqlite/src/wallet.rs

@@ -1345,7 +1345,10 @@ pub(crate) fn get_unspent_transparent_outputs<P: consensus::Parameters>(
 
         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<P: consensus::Parameters>(
         ":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<P: consensus::Parameters>(
         ":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<P: consensus::Parameters>(
     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<P: consensus::Parameters>(
         ":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::<Option<Amount>>(),
+                    .sum::<Option<NonNegativeAmount>>(),
-                Some(Amount::from(expected)),
+                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(),
         };
 

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,

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<ReceivedSaplingNote<ReceivedNoteId>, 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)
+            if available == NonNegativeAmount::const_from_u64(50000)
-                && required == Amount::const_from_i64(80000)
+                && 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)
+            if available == NonNegativeAmount::const_from_u64(50000)
-                && required == Amount::const_from_i64(80000)
+                && 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)
+                if available == NonNegativeAmount::const_from_u64(51000)
-                && required == Amount::const_from_i64(60000)
+                && 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,

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();
 

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`:

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<OutgoingViewingKey>,
         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(
+            NoteValue::from_raw(value.into()),
-                value
-                    .try_into()
-                    .expect("Cannot create Sapling outputs with negative note values."),
-            ),
             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!(

zcash_primitives/src/transaction/components/amount.rs

@@ -281,6 +281,21 @@ impl NonNegativeAmount {
         let amount = u64::from_le_bytes(bytes);
         Self::from_u64(amount)
     }
+
+    /// Reads a NonNegativeAmount from a signed integer represented as a two's
+    /// complement 64-bit little-endian value.
+    ///
+    /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`.
+    pub fn from_nonnegative_i64_le_bytes(bytes: [u8; 8]) -> Result<Self, ()> {
+        let amount = i64::from_le_bytes(bytes);
+        Self::from_nonnegative_i64(amount)
+    }
+
+    /// Returns this NonNegativeAmount encoded as a signed two's complement 64-bit
+    /// little-endian value.
+    pub fn to_i64_le_bytes(self) -> [u8; 8] {
+        self.0.to_i64_le_bytes()
+    }
 }
 
 impl From<NonNegativeAmount> for Amount {
@@ -400,7 +415,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 +424,8 @@ pub mod testing {
     }
 
     prop_compose! {
-        pub fn arb_nonnegative_amount()(amt in 0i64..MAX_MONEY) -> Amount {
+        pub fn arb_nonnegative_amount()(amt in 0i64..MAX_MONEY) -> NonNegativeAmount {
-            Amount::from_i64(amt).unwrap()
+            NonNegativeAmount::from_u64(amt as u64).unwrap()
         }
     }
 

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 {
+    fn value(&self) -> NonNegativeAmount {
-        Amount::from_u64(self.note.value().inner())
+        NonNegativeAmount::from_u64(self.note.value().inner())
             .expect("Note values should be checked at construction.")
     }
 }

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<NoteRef> {
     /// 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;
 }

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<A: Authorization> Bundle<A> {
         let output_sum = self
             .vout
             .iter()
-            .map(|p| p.value)
+            .map(|p| Amount::from(p.value))
             .sum::<Option<Amount>>()
             .ok_or(BalanceError::Overflow)?;
 
@@ -159,7 +159,7 @@ impl TxIn<Authorized> {
 
 #[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)?;

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> {
+    pub fn add_output(
-        if value.is_negative() {
+        &mut self,
-            return Err(Error::InvalidAmount);
+        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::<Option<Amount>>()
+            .sum::<Option<NonNegativeAmount>>()
             .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::<Option<Amount>>()
+            .sum::<Option<NonNegativeAmount>>()
             .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<transparent::Bundle<Unauthorized>> {
@@ -241,7 +237,7 @@ impl TxIn<Unauthorized> {
 
 #[cfg(not(feature = "transparent-inputs"))]
 impl TransparentAuthorizingContext for Unauthorized {
-    fn input_amounts(&self) -> Vec<Amount> {
+    fn input_amounts(&self) -> Vec<NonNegativeAmount> {
         vec![]
     }
 
@@ -252,7 +248,7 @@ impl TransparentAuthorizingContext for Unauthorized {
 
 #[cfg(feature = "transparent-inputs")]
 impl TransparentAuthorizingContext for Unauthorized {
-    fn input_amounts(&self) -> Vec<Amount> {
+    fn input_amounts(&self) -> Vec<NonNegativeAmount> {
         return self.inputs.iter().map(|txin| txin.coin.value).collect();
     }
 

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
     }
 

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 }
         }
     }
 

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<Amount>;
+    fn input_amounts(&self) -> Vec<NonNegativeAmount>;
     /// 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

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<Amount>,
+    input_amounts: Vec<NonNegativeAmount>,
     input_scriptpubkeys: Vec<Script>,
 }
 
@@ -179,7 +180,7 @@ impl transparent::Authorization for TestTransparentAuth {
 }
 
 impl TransparentAuthorizingContext for TestTransparentAuth {
-    fn input_amounts(&self) -> Vec<Amount> {
+    fn input_amounts(&self) -> Vec<NonNegativeAmount> {
         self.input_amounts.clone()
     }
 
@@ -214,7 +215,7 @@ fn zip_0244() {
         let input_amounts = tv
             .amounts
             .iter()
-            .map(|amount| Amount::from_nonnegative_i64(*amount).unwrap())
+            .map(|amount| NonNegativeAmount::from_nonnegative_i64(*amount).unwrap())
             .collect();
         let input_scriptpubkeys = tv
             .script_pubkeys