From 6d70c77cf90a373041c93ac6e3a746f85a51b900 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 28 Feb 2025 09:54:50 +1100 Subject: [PATCH] Enforce newtype sanity rules for amount types The unchecked-should-be-unsafe conversation is out of scope for this patch. We want to bite off small chunks so the constructors are left as they currently are - we are just doing the encapsulation here. This is in preparation for enforcing the MAX_MONEY invariant which is not currently enforced. As per the sanity rules policy outline in: https://github.com/rust-bitcoin/rust-bitcoin/discussions/4090 For both amount types create a private `encapsulate` module that consists of exactly the type and a single constructor and a single getter. --- units/src/amount/signed.rs | 156 +++++++++++++++++++---------------- units/src/amount/unsigned.rs | 126 ++++++++++++++++------------ 2 files changed, 156 insertions(+), 126 deletions(-) diff --git a/units/src/amount/signed.rs b/units/src/amount/signed.rs index d4c2d0227..5e5b4c5a2 100644 --- a/units/src/amount/signed.rs +++ b/units/src/amount/signed.rs @@ -16,37 +16,60 @@ use super::{ DisplayStyle, OutOfRangeError, ParseAmountError, ParseError, }; -/// A signed amount. -/// -/// The [`SignedAmount`] type can be used to express Bitcoin amounts that support arithmetic and -/// conversion to various denominations. The [`SignedAmount`] type does not implement [`serde`] -/// traits but we do provide modules for serializing as satoshis or bitcoin. -/// -/// Warning! -/// -/// This type implements several arithmetic operations from [`core::ops`]. -/// To prevent errors due to an overflow when using these operations, -/// it is advised to instead use the checked arithmetic methods whose names -/// start with `checked_`. The operations from [`core::ops`] that [`SignedAmount`] -/// implements will panic when an overflow occurs. -/// -/// # Examples -/// -/// ``` -/// # #[cfg(feature = "serde")] { -/// use serde::{Serialize, Deserialize}; -/// use bitcoin_units::SignedAmount; -/// -/// #[derive(Serialize, Deserialize)] -/// struct Foo { -/// // If you are using `rust-bitcoin` then `bitcoin::amount::serde::as_sat` also works. -/// #[serde(with = "bitcoin_units::amount::serde::as_sat")] // Also `serde::as_btc`. -/// amount: SignedAmount, -/// } -/// # } -/// ``` -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct SignedAmount(i64); +mod encapsulate { + /// A signed amount. + /// + /// The [`SignedAmount`] type can be used to express Bitcoin amounts that support arithmetic and + /// conversion to various denominations. The [`SignedAmount`] type does not implement [`serde`] + /// traits but we do provide modules for serializing as satoshis or bitcoin. + /// + /// Warning! + /// + /// This type implements several arithmetic operations from [`core::ops`]. + /// To prevent errors due to an overflow when using these operations, + /// it is advised to instead use the checked arithmetic methods whose names + /// start with `checked_`. The operations from [`core::ops`] that [`SignedAmount`] + /// implements will panic when an overflow occurs. + /// + /// # Examples + /// + /// ``` + /// # #[cfg(feature = "serde")] { + /// use serde::{Serialize, Deserialize}; + /// use bitcoin_units::SignedAmount; + /// + /// #[derive(Serialize, Deserialize)] + /// struct Foo { + /// // If you are using `rust-bitcoin` then `bitcoin::amount::serde::as_sat` also works. + /// #[serde(with = "bitcoin_units::amount::serde::as_sat")] // Also `serde::as_btc`. + /// amount: SignedAmount, + /// } + /// # } + /// ``` + #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] + pub struct SignedAmount(i64); + + impl SignedAmount { + /// Constructs a new [`SignedAmount`] with satoshi precision and the given number of satoshis. + /// + /// Caller to guarantee that `satoshi` is within valid range. + /// + /// See [`Self::MIN`] and [`Self::MAX_MONEY`]. + pub const fn from_sat_unchecked(satoshi: i64) -> SignedAmount { SignedAmount(satoshi) } + + /// Gets the number of satoshis in this [`SignedAmount`]. + /// + /// # Examples + /// + /// ``` + /// # use bitcoin_units::SignedAmount; + /// assert_eq!(SignedAmount::ONE_BTC.to_sat(), 100_000_000); + /// ``` + pub const fn to_sat(self) -> i64 { self.0 } + } +} +#[doc(inline)] +pub use encapsulate::SignedAmount; impl SignedAmount { /// The zero amount. @@ -73,24 +96,9 @@ impl SignedAmount { /// let amount = SignedAmount::from_sat(-100_000); /// assert_eq!(amount.to_sat(), -100_000); /// ``` - pub const fn from_sat(satoshi: i64) -> SignedAmount { SignedAmount(satoshi) } - - /// Gets the number of satoshis in this [`SignedAmount`]. - /// - /// # Examples - /// - /// ``` - /// # use bitcoin_units::SignedAmount; - /// assert_eq!(SignedAmount::ONE_BTC.to_sat(), 100_000_000); - /// ``` - pub const fn to_sat(self) -> i64 { self.0 } - - /// Constructs a new [`SignedAmount`] with satoshi precision and the given number of satoshis. - /// - /// Caller to guarantee that `satoshi` is within valid range. - /// - /// See [`Self::MIN`] and [`Self::MAX_MONEY`]. - pub const fn from_sat_unchecked(satoshi: i64) -> SignedAmount { SignedAmount(satoshi) } + pub const fn from_sat(satoshi: i64) -> SignedAmount { + SignedAmount::from_sat_unchecked(satoshi) + } /// Converts from a value expressing a decimal number of bitcoin to a [`SignedAmount`]. /// @@ -153,11 +161,11 @@ impl SignedAmount { (false, sat) if sat > SignedAmount::MAX.to_sat() as u64 => Err(ParseAmountError( ParseAmountErrorInner::OutOfRange(OutOfRangeError::too_big(true)), )), - (false, sat) => Ok(SignedAmount(sat as i64)), // Cast ok, value in this arm does not wrap. + (false, sat) => Ok(SignedAmount::from_sat(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(-(sat as i64))), // Cast ok, value in this arm does not wrap. + (true, sat) => Ok(SignedAmount::from_sat(-(sat as i64))), // Cast ok, value in this arm does not wrap. } } @@ -300,11 +308,11 @@ impl SignedAmount { /// Gets the absolute value of this [`SignedAmount`]. #[must_use] - pub fn abs(self) -> SignedAmount { SignedAmount(self.0.abs()) } + pub fn abs(self) -> SignedAmount { SignedAmount::from_sat(self.to_sat().abs()) } /// Gets the absolute value of this [`SignedAmount`] returning [`Amount`]. #[must_use] - pub fn unsigned_abs(self) -> Amount { Amount::from_sat(self.0.unsigned_abs()) } + pub fn unsigned_abs(self) -> Amount { Amount::from_sat(self.to_sat().unsigned_abs()) } /// Returns a number representing sign of this [`SignedAmount`]. /// @@ -312,19 +320,19 @@ impl SignedAmount { /// - `1` if the amount is positive /// - `-1` if the amount is negative #[must_use] - pub fn signum(self) -> i64 { self.0.signum() } + pub fn signum(self) -> i64 { self.to_sat().signum() } /// Checks if this [`SignedAmount`] is positive. /// /// Returns `true` if this [`SignedAmount`] is positive and `false` if /// this [`SignedAmount`] is zero or negative. - pub fn is_positive(self) -> bool { self.0.is_positive() } + pub fn is_positive(self) -> bool { self.to_sat().is_positive() } /// Checks if this [`SignedAmount`] is negative. /// /// Returns `true` if this [`SignedAmount`] is negative and `false` if /// this [`SignedAmount`] is zero or positive. - pub fn is_negative(self) -> bool { self.0.is_negative() } + pub fn is_negative(self) -> bool { self.to_sat().is_negative() } /// Returns the absolute value of this [`SignedAmount`]. /// @@ -334,8 +342,8 @@ impl SignedAmount { #[must_use] pub const fn checked_abs(self) -> Option { // No `map()` in const context. - match self.0.checked_abs() { - Some(res) => Some(SignedAmount(res)), + match self.to_sat().checked_abs() { + Some(res) => Some(SignedAmount::from_sat(res)), None => None, } } @@ -346,8 +354,8 @@ impl SignedAmount { #[must_use] pub const fn checked_add(self, rhs: SignedAmount) -> Option { // No `map()` in const context. - match self.0.checked_add(rhs.0) { - Some(res) => SignedAmount(res).check_min_max(), + match self.to_sat().checked_add(rhs.to_sat()) { + Some(res) => SignedAmount::from_sat(res).check_min_max(), None => None, } } @@ -359,8 +367,8 @@ impl SignedAmount { #[must_use] pub const fn checked_sub(self, rhs: SignedAmount) -> Option { // No `map()` in const context. - match self.0.checked_sub(rhs.0) { - Some(res) => SignedAmount(res).check_min_max(), + match self.to_sat().checked_sub(rhs.to_sat()) { + Some(res) => SignedAmount::from_sat(res).check_min_max(), None => None, } } @@ -372,8 +380,8 @@ impl SignedAmount { #[must_use] pub const fn checked_mul(self, rhs: i64) -> Option { // No `map()` in const context. - match self.0.checked_mul(rhs) { - Some(res) => SignedAmount(res).check_min_max(), + match self.to_sat().checked_mul(rhs) { + Some(res) => SignedAmount::from_sat(res).check_min_max(), None => None, } } @@ -386,8 +394,8 @@ impl SignedAmount { #[must_use] pub const fn checked_div(self, rhs: i64) -> Option { // No `map()` in const context. - match self.0.checked_div(rhs) { - Some(res) => Some(SignedAmount(res)), + match self.to_sat().checked_div(rhs) { + Some(res) => Some(SignedAmount::from_sat(res)), None => None, } } @@ -398,8 +406,8 @@ impl SignedAmount { #[must_use] pub const fn checked_rem(self, rhs: i64) -> Option { // No `map()` in const context. - match self.0.checked_rem(rhs) { - Some(res) => Some(SignedAmount(res)), + match self.to_sat().checked_rem(rhs) { + Some(res) => Some(SignedAmount::from_sat(res)), None => None, } } @@ -413,7 +421,9 @@ impl SignedAmount { /// On overflow, panics in debug mode, wraps in release mode. #[must_use] #[deprecated(since = "TBD", note = "consider converting to u64 using `to_sat`")] - pub fn unchecked_add(self, rhs: SignedAmount) -> SignedAmount { Self(self.0 + rhs.0) } + pub fn unchecked_add(self, rhs: SignedAmount) -> SignedAmount { + Self::from_sat(self.to_sat() + rhs.to_sat()) + } /// Unchecked subtraction. /// @@ -424,7 +434,9 @@ impl SignedAmount { /// On overflow, panics in debug mode, wraps in release mode. #[must_use] #[deprecated(since = "TBD", note = "consider converting to u64 using `to_sat`")] - pub fn unchecked_sub(self, rhs: SignedAmount) -> SignedAmount { Self(self.0 - rhs.0) } + pub fn unchecked_sub(self, rhs: SignedAmount) -> SignedAmount { + Self::from_sat(self.to_sat() - rhs.to_sat()) + } /// Subtraction that doesn't allow negative [`SignedAmount`]s. /// @@ -453,7 +465,7 @@ impl SignedAmount { /// Checks the amount is within the allowed range. const fn check_min_max(self) -> Option { - if self.0 < Self::MIN.0 || self.0 > Self::MAX.0 { + if self.to_sat() < Self::MIN.to_sat() || self.to_sat() > Self::MAX.to_sat() { None } else { Some(self) @@ -514,6 +526,6 @@ impl From for SignedAmount { impl<'a> Arbitrary<'a> for SignedAmount { fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result { let s = i64::arbitrary(u)?; - Ok(Self(s)) + Ok(Self::from_sat(s)) } } diff --git a/units/src/amount/unsigned.rs b/units/src/amount/unsigned.rs index 71ee58de4..ae9c3f5c2 100644 --- a/units/src/amount/unsigned.rs +++ b/units/src/amount/unsigned.rs @@ -16,29 +16,58 @@ use super::{ OutOfRangeError, ParseAmountError, ParseError, SignedAmount, }; -/// An amount. -/// -/// The [`Amount`] type can be used to express Bitcoin amounts that support arithmetic and -/// conversion to various denominations. The [`Amount`] type does not implement [`serde`] traits -/// but we do provide modules for serializing as satoshis or bitcoin. -/// -/// # Examples -/// -/// ``` -/// # #[cfg(feature = "serde")] { -/// use serde::{Serialize, Deserialize}; -/// use bitcoin_units::Amount; -/// -/// #[derive(Serialize, Deserialize)] -/// struct Foo { -/// // If you are using `rust-bitcoin` then `bitcoin::amount::serde::as_sat` also works. -/// #[serde(with = "bitcoin_units::amount::serde::as_sat")] // Also `serde::as_btc`. -/// amount: Amount, -/// } -/// # } -/// ``` -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct Amount(u64); +mod encapsulate { + /// An amount. + /// + /// The [`Amount`] type can be used to express Bitcoin amounts that support arithmetic and + /// conversion to various denominations. The [`Amount`] type does not implement [`serde`] traits + /// but we do provide modules for serializing as satoshis or bitcoin. + /// + /// Warning! + /// + /// This type implements several arithmetic operations from [`core::ops`]. + /// To prevent errors due to an overflow when using these operations, + /// it is advised to instead use the checked arithmetic methods whose names + /// start with `checked_`. The operations from [`core::ops`] that [`Amount`] + /// implements will panic when an overflow occurs. + /// + /// # Examples + /// + /// ``` + /// # #[cfg(feature = "serde")] { + /// use serde::{Serialize, Deserialize}; + /// use bitcoin_units::Amount; + /// + /// #[derive(Serialize, Deserialize)] + /// struct Foo { + /// // If you are using `rust-bitcoin` then `bitcoin::amount::serde::as_sat` also works. + /// #[serde(with = "bitcoin_units::amount::serde::as_sat")] // Also `serde::as_btc`. + /// amount: Amount, + /// } + /// # } + /// ``` + #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] + pub struct Amount(u64); + + impl Amount { + /// Constructs a new [`Amount`] with satoshi precision and the given number of satoshis. + /// + /// Caller to guarantee that `satoshi` is within valid range. See [`Self::MAX_MONEY`]. + pub const fn from_sat_unchecked(satoshi: u64) -> Amount { Self(satoshi) } + + /// Gets the number of satoshis in this [`Amount`]. + /// + /// # Examples + /// + /// ``` + /// # use bitcoin_units::Amount; + /// assert_eq!(Amount::ONE_BTC.to_sat(), 100_000_000); + /// ``` + pub const fn to_sat(self) -> u64 { self.0 } + } +} +#[doc(inline)] +pub use encapsulate::Amount; impl Amount { /// The zero amount. @@ -67,22 +96,7 @@ impl Amount { /// let amount = Amount::from_sat(100_000); /// assert_eq!(amount.to_sat(), 100_000); /// ``` - pub const fn from_sat(satoshi: u64) -> Amount { Amount(satoshi) } - - /// Gets the number of satoshis in this [`Amount`]. - /// - /// # Examples - /// - /// ``` - /// # use bitcoin_units::Amount; - /// assert_eq!(Amount::ONE_BTC.to_sat(), 100_000_000); - /// ``` - pub const fn to_sat(self) -> u64 { self.0 } - - /// Constructs a new [`Amount`] with satoshi precision and the given number of satoshis. - /// - /// Caller to guarantee that `satoshi` is within valid range. See [`Self::MAX_MONEY`]. - pub const fn from_sat_unchecked(satoshi: u64) -> Amount { Self(satoshi) } + pub const fn from_sat(satoshi: u64) -> Amount { Amount::from_sat_unchecked(satoshi) } /// Converts from a value expressing a decimal number of bitcoin to an [`Amount`]. /// @@ -148,7 +162,7 @@ impl Amount { OutOfRangeError::negative(), ))); } - if sats > Self::MAX.0 { + if sats > Self::MAX.to_sat() { return Err(ParseAmountError(ParseAmountErrorInner::OutOfRange( OutOfRangeError::too_big(false), ))); @@ -299,8 +313,8 @@ impl Amount { #[must_use] pub const fn checked_add(self, rhs: Amount) -> Option { // No `map()` in const context. - match self.0.checked_add(rhs.0) { - Some(res) => Amount(res).check_max(), + match self.to_sat().checked_add(rhs.to_sat()) { + Some(res) => Amount::from_sat(res).check_max(), None => None, } } @@ -311,8 +325,8 @@ impl Amount { #[must_use] pub const fn checked_sub(self, rhs: Amount) -> Option { // No `map()` in const context. - match self.0.checked_sub(rhs.0) { - Some(res) => Some(Amount(res)), + match self.to_sat().checked_sub(rhs.to_sat()) { + Some(res) => Some(Amount::from_sat(res)), None => None, } } @@ -323,8 +337,8 @@ impl Amount { #[must_use] pub const fn checked_mul(self, rhs: u64) -> Option { // No `map()` in const context. - match self.0.checked_mul(rhs) { - Some(res) => Amount(res).check_max(), + match self.to_sat().checked_mul(rhs) { + Some(res) => Amount::from_sat(res).check_max(), None => None, } } @@ -337,8 +351,8 @@ impl Amount { #[must_use] pub const fn checked_div(self, rhs: u64) -> Option { // No `map()` in const context. - match self.0.checked_div(rhs) { - Some(res) => Some(Amount(res)), + match self.to_sat().checked_div(rhs) { + Some(res) => Some(Amount::from_sat(res)), None => None, } } @@ -349,8 +363,8 @@ impl Amount { #[must_use] pub const fn checked_rem(self, rhs: u64) -> Option { // No `map()` in const context. - match self.0.checked_rem(rhs) { - Some(res) => Some(Amount(res)), + match self.to_sat().checked_rem(rhs) { + Some(res) => Some(Amount::from_sat(res)), None => None, } } @@ -364,7 +378,9 @@ impl Amount { /// On overflow, panics in debug mode, wraps in release mode. #[must_use] #[deprecated(since = "TBD", note = "consider converting to u64 using `to_sat`")] - pub fn unchecked_add(self, rhs: Amount) -> Amount { Self(self.0 + rhs.0) } + pub fn unchecked_add(self, rhs: Amount) -> Amount { + Self::from_sat(self.to_sat() + rhs.to_sat()) + } /// Unchecked subtraction. /// @@ -375,7 +391,9 @@ impl Amount { /// On overflow, panics in debug mode, wraps in release mode. #[must_use] #[deprecated(since = "TBD", note = "consider converting to u64 using `to_sat`")] - pub fn unchecked_sub(self, rhs: Amount) -> Amount { Self(self.0 - rhs.0) } + pub fn unchecked_sub(self, rhs: Amount) -> Amount { + Self::from_sat(self.to_sat() - rhs.to_sat()) + } /// Converts to a signed amount. #[rustfmt::skip] // Moves code comments to the wrong line. @@ -385,7 +403,7 @@ impl Amount { /// Checks if the amount is below the maximum value. Returns `None` if it is above. const fn check_max(self) -> Option { - if self.0 > Self::MAX.0 { + if self.to_sat() > Self::MAX.to_sat() { None } else { Some(self) @@ -445,6 +463,6 @@ impl TryFrom for Amount { impl<'a> Arbitrary<'a> for Amount { fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result { let a = u64::arbitrary(u)?; - Ok(Self(a)) + Ok(Self::from_sat(a)) } }