From 004d07318465c73da9f302e13cbd50cfbe6fda35 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 18 Mar 2025 14:26:20 +0000 Subject: [PATCH] amount: return i64 from parse_signed_to_satoshi This private function is used for string-parsing an amount. It returns a sign boolean and a u64, but its surrounding logic can be simplified if it just returns a i64 (which is OK now since the range of `Amount` fits into the range of i64). Along the way we eliminate some instances of from_sat_unchecked. Annoyingly we still need the bool to distinguish -0 from +0; when parsing Amount we reject -0 (and we have tests for this). This causes a couple error constructors to no longer be used outside of unit tests. May be worth looking at whether we need them, or whether we should be using them in parse_signed_to_satoshi. --- units/src/amount/error.rs | 2 ++ units/src/amount/mod.rs | 21 ++++++++++++++------- units/src/amount/signed.rs | 16 ++++------------ units/src/amount/unsigned.rs | 11 +++-------- 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/units/src/amount/error.rs b/units/src/amount/error.rs index 62e03a306..992d7e543 100644 --- a/units/src/amount/error.rs +++ b/units/src/amount/error.rs @@ -182,8 +182,10 @@ impl OutOfRangeError { /// Returns true if the input value was smaller than the minimum allowed value. pub fn is_below_min(self) -> bool { !self.is_greater_than_max } + #[cfg(test)] pub(crate) fn too_big(is_signed: bool) -> Self { Self { is_signed, is_greater_than_max: true } } + #[cfg(test)] pub(crate) fn too_small() -> Self { Self { // implied - negative() is used for the other diff --git a/units/src/amount/mod.rs b/units/src/amount/mod.rs index 98e2137a8..8928cea9d 100644 --- a/units/src/amount/mod.rs +++ b/units/src/amount/mod.rs @@ -202,10 +202,12 @@ const INPUT_STRING_LEN_LIMIT: usize = 50; /// Parses a decimal string in the given denomination into a satoshi value and a /// [`bool`] indicator for a negative amount. +/// +/// The `bool` is only needed to distinguish -0 from 0. fn parse_signed_to_satoshi( mut s: &str, denom: Denomination, -) -> Result<(bool, u64), InnerParseError> { +) -> Result<(bool, SignedAmount), InnerParseError> { if s.is_empty() { return Err(InnerParseError::MissingDigits(MissingDigitsError { kind: MissingDigitsKind::Empty, @@ -237,7 +239,7 @@ fn parse_signed_to_satoshi( let last_n = precision_diff.unsigned_abs().into(); if let Some(position) = is_too_precise(s, last_n) { match s.parse::() { - Ok(0) => return Ok((is_negative, 0)), + Ok(0) => return Ok((is_negative, SignedAmount::ZERO)), _ => return Err(InnerParseError::TooPrecise(TooPreciseError { position: position + usize::from(is_negative), @@ -252,14 +254,14 @@ fn parse_signed_to_satoshi( }; let mut decimals = None; - let mut value: u64 = 0; // as satoshis + let mut value: i64 = 0; // as satoshis for (i, c) in s.char_indices() { match c { '0'..='9' => { // Do `value = 10 * value + digit`, catching overflows. - match 10_u64.checked_mul(value) { + match 10_i64.checked_mul(value) { None => return Err(InnerParseError::Overflow { is_negative }), - Some(val) => match val.checked_add(u64::from(c as u8 - b'0')) { + Some(val) => match val.checked_add(i64::from(c as u8 - b'0')) { None => return Err(InnerParseError::Overflow { is_negative }), Some(val) => value = val, }, @@ -295,13 +297,18 @@ fn parse_signed_to_satoshi( // Decimally shift left by `max_decimals - decimals`. let scale_factor = max_decimals - decimals.unwrap_or(0); for _ in 0..scale_factor { - value = match 10_u64.checked_mul(value) { + value = match 10_i64.checked_mul(value) { Some(v) => v, None => return Err(InnerParseError::Overflow { is_negative }), }; } - Ok((is_negative, value)) + let mut ret = + SignedAmount::from_sat(value).map_err(|_| InnerParseError::Overflow { is_negative })?; + if is_negative { + ret = -ret; + } + Ok((is_negative, ret)) } #[derive(Debug)] diff --git a/units/src/amount/signed.rs b/units/src/amount/signed.rs index cf633d4a4..262106673 100644 --- a/units/src/amount/signed.rs +++ b/units/src/amount/signed.rs @@ -10,7 +10,7 @@ use core::{default, fmt}; #[cfg(feature = "arbitrary")] use arbitrary::{Arbitrary, Unstructured}; -use super::error::{ParseAmountErrorInner, ParseErrorInner}; +use super::error::ParseErrorInner; use super::{ parse_signed_to_satoshi, split_amount_and_denomination, Amount, Denomination, Display, DisplayStyle, OutOfRangeError, ParseAmountError, ParseError, @@ -169,17 +169,9 @@ impl SignedAmount { /// /// If the amount is too big (positive or negative) or too precise. pub fn from_str_in(s: &str, denom: Denomination) -> Result { - match parse_signed_to_satoshi(s, denom).map_err(|error| error.convert(true))? { - // (negative, amount) - (false, sat) if sat > SignedAmount::MAX.to_sat() as u64 => Err(ParseAmountError( - ParseAmountErrorInner::OutOfRange(OutOfRangeError::too_big(true)), - )), - (false, sat) => Ok(SignedAmount::from_sat_unchecked(sat as i64)), // Cast ok, value in this arm does not wrap. - (true, sat) if sat > SignedAmount::MIN.to_sat().unsigned_abs() => Err( - ParseAmountError(ParseAmountErrorInner::OutOfRange(OutOfRangeError::too_small())), - ), - (true, sat) => Ok(SignedAmount::from_sat_unchecked(-(sat as i64))), // Cast ok, value in this arm does not wrap. - } + parse_signed_to_satoshi(s, denom) + .map(|(_, amount)| amount) + .map_err(|error| error.convert(true)) } /// Parses amounts with denomination suffix as produced by [`Self::to_string_with_denomination`] diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs index b4191fe89..25533675f 100644 --- a/units/src/amount/unsigned.rs +++ b/units/src/amount/unsigned.rs @@ -167,19 +167,14 @@ impl Amount { /// /// If the amount is too precise, negative, or greater than 21,000,000. pub fn from_str_in(s: &str, denom: Denomination) -> Result { - let (negative, sats) = + let (is_neg, amount) = parse_signed_to_satoshi(s, denom).map_err(|error| error.convert(false))?; - if negative { + if is_neg { return Err(ParseAmountError(ParseAmountErrorInner::OutOfRange( OutOfRangeError::negative(), ))); } - if sats > Self::MAX.to_sat() { - return Err(ParseAmountError(ParseAmountErrorInner::OutOfRange( - OutOfRangeError::too_big(false), - ))); - } - Ok(Self::from_sat_unchecked(sats)) + Self::try_from(amount).map_err(|e| ParseAmountError(ParseAmountErrorInner::OutOfRange(e))) } /// Parses amounts with denomination suffix as produced by [`Self::to_string_with_denomination`]