From c4fe5f2bb0767ecffd4ed779eca6a0e34885c8b9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 5 Mar 2024 19:44:29 -0700 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Daira-Emma Hopwood --- .../zcash_protocol/src/constants/mainnet.rs | 2 +- .../zcash_protocol/src/constants/testnet.rs | 2 +- components/zcash_protocol/src/value.rs | 16 +++++---- zcash_client_backend/CHANGELOG.md | 6 ++-- zcash_client_backend/src/zip321.rs | 36 +++++++++---------- 5 files changed, 32 insertions(+), 30 deletions(-) diff --git a/components/zcash_protocol/src/constants/mainnet.rs b/components/zcash_protocol/src/constants/mainnet.rs index b30babc721..25193fab88 100644 --- a/components/zcash_protocol/src/constants/mainnet.rs +++ b/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]. /// diff --git a/components/zcash_protocol/src/constants/testnet.rs b/components/zcash_protocol/src/constants/testnet.rs index a0935ad2e7..bbe74b6a55 100644 --- a/components/zcash_protocol/src/constants/testnet.rs +++ b/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]. /// diff --git a/components/zcash_protocol/src/value.rs b/components/zcash_protocol/src/value.rs index cfd3a7fc30..395ac8edc2 100644 --- a/components/zcash_protocol/src/value.rs +++ b/components/zcash_protocol/src/value.rs @@ -155,7 +155,7 @@ impl TryFrom for u64 { type Error = BalanceError; fn try_from(value: ZatBalance) -> Result { - 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 { - 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() diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 17f58112f5..e642f21f20 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/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 diff --git a/zcash_client_backend/src/zip321.rs b/zcash_client_backend/src/zip321.rs index e795b74af2..fbd9b3bb6d 100644 --- a/zcash_client_backend/src/zip321.rs +++ b/zcash_client_backend/src/zip321.rs @@ -245,7 +245,7 @@ impl TransactionRequest { payment_index: Option, ) -> impl IntoIterator + '_ { 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 { - 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) -> Option { - amount_str(amount).map(|s| format!("amount{}={}", param_index(idx), s)) + pub fn amount_param(amount: NonNegativeAmount, idx: Option) -> 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); }