Apply suggestions from code review

Co-authored-by: Daira-Emma Hopwood <[email protected]>

components/zcash_protocol/src/constants/mainnet.rs

@@ -29,7 +29,7 @@ pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviews";
 /// [Zcash Protocol Specification]: https://github.com/zcash/zips/blob/master/protocol/protocol.pdf
 pub const HRP_SAPLING_PAYMENT_ADDRESS: &str = "zs";
 
-/// The prefix for a Base58Check-encoded mainnet Sprout address
+/// The prefix for a Base58Check-encoded mainnet Sprout address.
 ///
 /// Defined in the [Zcash Protocol Specification section 5.6.3][sproutpaymentaddrencoding].
 ///

components/zcash_protocol/src/constants/testnet.rs

@@ -29,7 +29,7 @@ pub const HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY: &str = "zxviewtestsapling";
 /// [Zcash Protocol Specification]: https://github.com/zcash/zips/blob/master/protocol/protocol.pdf
 pub const HRP_SAPLING_PAYMENT_ADDRESS: &str = "ztestsapling";
 
-/// The prefix for a Base58Check-encoded testnet Sprout addresses.
+/// The prefix for a Base58Check-encoded testnet Sprout address.
 ///
 /// Defined in the [Zcash Protocol Specification section 5.6.3][sproutpaymentaddrencoding].
 ///

components/zcash_protocol/src/value.rs

@@ -155,7 +155,7 @@ impl TryFrom<ZatBalance> for u64 {
     type Error = BalanceError;
 
     fn try_from(value: ZatBalance) -> Result<Self, Self::Error> {
-        value.0.try_into().map_err(|_| BalanceError::Overflow)
+        value.0.try_into().map_err(|_| BalanceError::Underflow)
     }
 }
 
@@ -261,11 +261,9 @@ impl Zatoshis {
     ///
     /// Returns an error if the amount is outside the range `{0..MAX_MONEY}`.
     pub fn from_nonnegative_i64(amount: i64) -> Result<Self, BalanceError> {
-        if amount >= 0 {
-            Self::from_u64(u64::try_from(amount).unwrap())
-        } else {
-            Err(BalanceError::Underflow)
-        }
+        u64::try_from(amount)
+            .map_err(|_| BalanceError::Underflow)
+            .and_then(Self::from_u64)
     }
 
     /// Reads an Zatoshis from an unsigned 64-bit little-endian integer.
@@ -439,6 +437,12 @@ pub mod testing {
         }
     }
 
+    prop_compose! {
+        pub fn arb_nonnegative_zat_balance()(amt in 0i64..MAX_BALANCE) -> ZatBalance {
+            ZatBalance::from_i64(amt).unwrap()
+        }
+    }
+
     prop_compose! {
         pub fn arb_zatoshis()(amt in 0u64..MAX_MONEY) -> Zatoshis {
             Zatoshis::from_u64(amt).unwrap()

zcash_client_backend/CHANGELOG.md

@@ -56,10 +56,12 @@ and this library adheres to Rust's notion of
 - `zcash_client_backend::zip321::render::amount_str` now takes a
   `NonNegativeAmount` rather than a signed `Amount` as its argument.
 - `zcash_client_backend::zip321::parse::parse_amount` now parses a
-  `NonNegativeAmount` rather than a signed `Amount`.
+  `NonNegativeAmount` rather than a signed `Amount`. Also, it
+- `zcash_client_backend::zip321::TransactionRequest::total` now
+  returns `Result<_, BalanceError>` instead of `Result<_, ()>`.
 
 ### Removed
-- `zcash_client_backend::PoolType::is_receiver` use 
+- `zcash_client_backend::PoolType::is_receiver`: use
   `zcash_keys::Address::has_receiver` instead.
 
 ## [0.11.0] - 2024-03-01

zcash_client_backend/src/zip321.rs

@@ -245,7 +245,7 @@ impl TransactionRequest {
             payment_index: Option<usize>,
         ) -> impl IntoIterator<Item = String> + '_ {
             std::iter::empty()
-                .chain(render::amount_param(payment.amount, payment_index))
+                .chain(Some(render::amount_param(payment.amount, payment_index)))
                 .chain(
                     payment
                         .memo
@@ -414,28 +414,24 @@ mod render {
         format!("address{}={}", param_index(idx), addr.encode(params))
     }
 
-    /// Converts an [`NonNegativeAmount`] value to a correctly formatted decimal ZEC
+    /// Converts a [`NonNegativeAmount`] value to a correctly formatted decimal ZEC string.
     /// value for inclusion in a ZIP 321 URI.
-    pub fn amount_str(amount: NonNegativeAmount) -> Option<String> {
-        if amount.is_positive() {
-            let coins = u64::from(amount) / COIN;
-            let zats = u64::from(amount) % COIN;
-            Some(if zats == 0 {
-                format!("{}", coins)
-            } else {
-                format!("{}.{:0>8}", coins, zats)
-                    .trim_end_matches('0')
-                    .to_string()
-            })
+    pub fn amount_str(amount: NonNegativeAmount) -> String {
+        let coins = u64::from(amount) / COIN;
+        let zats = u64::from(amount) % COIN;
+        if zats == 0 {
+            format!("{}", coins)
         } else {
-            None
+            format!("{}.{:0>8}", coins, zats)
+                .trim_end_matches('0')
+                .to_string()
         }
     }
 
     /// Constructs an "amount" key/value pair containing the encoded ZEC amount
     /// at the specified parameter index.
-    pub fn amount_param(amount: NonNegativeAmount, idx: Option<usize>) -> Option<String> {
-        amount_str(amount).map(|s| format!("amount{}={}", param_index(idx), s))
+    pub fn amount_param(amount: NonNegativeAmount, idx: Option<usize>) -> String {
+        format!("amount{}={}", param_index(idx), amount_str(amount))
     }
 
     /// Constructs a "memo" key/value pair containing the base64URI-encoded memo
@@ -650,7 +646,7 @@ mod parse {
                 digit1,
                 opt(preceded(
                     char('.'),
-                    map_opt(digit0, |s: &str| if s.len() > 8 { None } else { Some(s) }),
+                    map_opt(digit1, |s: &str| if s.len() > 8 { None } else { Some(s) }),
                 )),
             )),
             |(whole_s, decimal_s): (&str, Option<&str>)| {
@@ -671,7 +667,7 @@ mod parse {
                     .and_then(|coin_zats| coin_zats.checked_add(zats))
                     .ok_or(BalanceError::Overflow)
                     .and_then(NonNegativeAmount::from_u64)
-                    .map_err(|_| format!("Not a valid zat amount: {}.{}", coins, zats))
+                    .map_err(|_| format!("Not a valid amount: {}.{:0>8} ZEC", coins, zats))
             },
         )(input)
     }
@@ -854,7 +850,7 @@ mod tests {
 
         for amt_u64 in amounts {
             let amt = NonNegativeAmount::from_u64(amt_u64).unwrap();
-            let amt_str = amount_str(amt).unwrap();
+            let amt_str = amount_str(amt);
             assert_eq!(amt, parse_amount(&amt_str).unwrap().1);
         }
     }
@@ -1099,7 +1095,7 @@ mod tests {
 
         #[test]
         fn prop_zip321_roundtrip_amount(amt in arb_nonnegative_amount()) {
-            let amt_str = amount_str(amt).unwrap();
+            let amt_str = amount_str(amt);
             assert_eq!(amt, parse_amount(&amt_str).unwrap().1);
         }