Merge rust-bitcoin#3728: units: Unify and flesh out ops impls

68f3c3e api: Run just check-api (Tobin C. Harding)
b956940 Add ops unit tests for amount types (Tobin C. Harding)
d88306f units: Implement ops for amount types (Tobin C. Harding)
4a3aa4a Add ops unit tests for Weight (Tobin C. Harding)
43ef9a6 units: Implement ops for Weight (Tobin C. Harding)
c5fbc7e units: Add assign macros (Tobin C. Harding)
20a79da units: Use one level of path for ops traits (Tobin C. Harding)
6ce5385 units: Add macros for Add and Sub (Tobin C. Harding)
b31ca72 units: Use rhs instead of other (Tobin C. Harding)
cd0fa2d Make FeeRate ops impls more terse (Tobin C. Harding)

Pull request description:

  Sort out `Add`, `AddAssign`, `Sub`, and `SubAssign` for the following types:

  - `Amount`
  - `SignedAmount`
  - `FeeRate`
  - `Weight`

  And clean up as we go.

  Note that `BlockInterval` isn't touched in this PR - it looks correct already.

  The unit tests in the two patches "Add ops tests ..." can be moved if you want to verify that they don't build without the patch they follow.

ACKs for top commit:
  apoelstra:
    ACK 68f3c3e; successfully ran local tests

Tree-SHA512: a82d3bf288f61b169b8cff498e81bd2cd123c8dcbf534413233ff03f06102a42508e09b2f7e5b268b21f82d4bf2b3612cd88dea1231b4d3e6455c7e99f82e729

units/src/amount/signed.rs

@@ -388,10 +388,8 @@ impl ops::Add for SignedAmount {
         self.checked_add(rhs).expect("SignedAmount addition error")
     }
 }
-
-impl ops::AddAssign for SignedAmount {
-    fn add_assign(&mut self, other: SignedAmount) { *self = *self + other }
-}
+crate::internal_macros::impl_add_for_references!(SignedAmount);
+crate::internal_macros::impl_add_assign!(SignedAmount);
 
 impl ops::Sub for SignedAmount {
     type Output = SignedAmount;
@@ -400,10 +398,8 @@ impl ops::Sub for SignedAmount {
         self.checked_sub(rhs).expect("SignedAmount subtraction error")
     }
 }
-
-impl ops::SubAssign for SignedAmount {
-    fn sub_assign(&mut self, other: SignedAmount) { *self = *self - other }
-}
+crate::internal_macros::impl_sub_for_references!(SignedAmount);
+crate::internal_macros::impl_sub_assign!(SignedAmount);
 
 impl ops::Rem<i64> for SignedAmount {
     type Output = SignedAmount;

units/src/amount/tests.rs

@@ -1008,3 +1008,99 @@ fn trailing_zeros_for_amount() {
     assert_eq!(format!("{}", Amount::from_sat(400_000_000_000_010)), "4000000.0000001 BTC");
     assert_eq!(format!("{}", Amount::from_sat(400_000_000_000_000)), "4000000 BTC");
 }
+
+#[test]
+#[allow(clippy::op_ref)]
+fn unsigned_addition() {
+    let one = Amount::from_sat(1);
+    let two = Amount::from_sat(2);
+    let three = Amount::from_sat(3);
+
+    assert!(one + two == three);
+    assert!(&one + two == three);
+    assert!(one + &two == three);
+    assert!(&one + &two == three);
+}
+
+#[test]
+#[allow(clippy::op_ref)]
+fn unsigned_subtract() {
+    let one = Amount::from_sat(1);
+    let two = Amount::from_sat(2);
+    let three = Amount::from_sat(3);
+
+    assert!(three - two == one);
+    assert!(&three - two == one);
+    assert!(three - &two == one);
+    assert!(&three - &two == one);
+}
+
+#[test]
+fn unsigned_add_assign() {
+    let mut f = Amount::from_sat(1);
+    f += Amount::from_sat(2);
+    assert_eq!(f, Amount::from_sat(3));
+
+    let mut f = Amount::from_sat(1);
+    f += &Amount::from_sat(2);
+    assert_eq!(f, Amount::from_sat(3));
+}
+
+#[test]
+fn unsigned_sub_assign() {
+    let mut f = Amount::from_sat(3);
+    f -= Amount::from_sat(2);
+    assert_eq!(f, Amount::from_sat(1));
+
+    let mut f = Amount::from_sat(3);
+    f -= &Amount::from_sat(2);
+    assert_eq!(f, Amount::from_sat(1));
+}
+
+#[test]
+#[allow(clippy::op_ref)]
+fn signed_addition() {
+    let one = SignedAmount::from_sat(1);
+    let two = SignedAmount::from_sat(2);
+    let three = SignedAmount::from_sat(3);
+
+    assert!(one + two == three);
+    assert!(&one + two == three);
+    assert!(one + &two == three);
+    assert!(&one + &two == three);
+}
+
+#[test]
+#[allow(clippy::op_ref)]
+fn signed_subtract() {
+    let one = SignedAmount::from_sat(1);
+    let two = SignedAmount::from_sat(2);
+    let three = SignedAmount::from_sat(3);
+
+    assert!(three - two == one);
+    assert!(&three - two == one);
+    assert!(three - &two == one);
+    assert!(&three - &two == one);
+}
+
+#[test]
+fn signed_add_assign() {
+    let mut f = SignedAmount::from_sat(1);
+    f += SignedAmount::from_sat(2);
+    assert_eq!(f, SignedAmount::from_sat(3));
+
+    let mut f = SignedAmount::from_sat(1);
+    f += &SignedAmount::from_sat(2);
+    assert_eq!(f, SignedAmount::from_sat(3));
+}
+
+#[test]
+fn signed_sub_assign() {
+    let mut f = SignedAmount::from_sat(3);
+    f -= SignedAmount::from_sat(2);
+    assert_eq!(f, SignedAmount::from_sat(1));
+
+    let mut f = SignedAmount::from_sat(3);
+    f -= &SignedAmount::from_sat(2);
+    assert_eq!(f, SignedAmount::from_sat(1));
+}

units/src/amount/unsigned.rs

@@ -457,10 +457,8 @@ impl ops::Add for Amount {
         self.checked_add(rhs).expect("Amount addition error")
     }
 }
-
-impl ops::AddAssign for Amount {
-    fn add_assign(&mut self, other: Amount) { *self = *self + other }
-}
+crate::internal_macros::impl_add_for_references!(Amount);
+crate::internal_macros::impl_add_assign!(Amount);
 
 impl ops::Sub for Amount {
     type Output = Amount;
@@ -469,10 +467,8 @@ impl ops::Sub for Amount {
         self.checked_sub(rhs).expect("Amount subtraction error")
     }
 }
-
-impl ops::SubAssign for Amount {
-    fn sub_assign(&mut self, other: Amount) { *self = *self - other }
-}
+crate::internal_macros::impl_sub_for_references!(Amount);
+crate::internal_macros::impl_sub_assign!(Amount);
 
 impl ops::Rem<u64> for Amount {
     type Output = Amount;

units/src/fee_rate.rs

@@ -2,8 +2,7 @@
 
 //! Implements `FeeRate` and assoctiated features.
 
-use core::fmt;
-use core::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign};
+use core::{fmt, ops};
 
 #[cfg(feature = "arbitrary")]
 use arbitrary::{Arbitrary, Unstructured};
@@ -185,70 +184,38 @@ impl From<FeeRate> for u64 {
     fn from(value: FeeRate) -> Self { value.to_sat_per_kwu() }
 }
 
-impl Add for FeeRate {
+impl ops::Add for FeeRate {
     type Output = FeeRate;
 
     fn add(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 + rhs.0) }
 }
+crate::internal_macros::impl_add_for_references!(FeeRate);
+crate::internal_macros::impl_add_assign!(FeeRate);
 
-impl Add<FeeRate> for &FeeRate {
-    type Output = FeeRate;
-
-    fn add(self, other: FeeRate) -> <FeeRate as Add>::Output { FeeRate(self.0 + other.0) }
-}
-
-impl Add<&FeeRate> for FeeRate {
-    type Output = FeeRate;
-
-    fn add(self, other: &FeeRate) -> <FeeRate as Add>::Output { FeeRate(self.0 + other.0) }
-}
-
-impl<'a> Add<&'a FeeRate> for &FeeRate {
-    type Output = FeeRate;
-
-    fn add(self, other: &'a FeeRate) -> <FeeRate as Add>::Output { FeeRate(self.0 + other.0) }
-}
-
-impl Sub for FeeRate {
+impl ops::Sub for FeeRate {
     type Output = FeeRate;
 
     fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 - rhs.0) }
 }
-
-impl Sub<FeeRate> for &FeeRate {
-    type Output = FeeRate;
-
-    fn sub(self, other: FeeRate) -> <FeeRate as Add>::Output { FeeRate(self.0 - other.0) }
-}
-
-impl Sub<&FeeRate> for FeeRate {
-    type Output = FeeRate;
-
-    fn sub(self, other: &FeeRate) -> <FeeRate as Add>::Output { FeeRate(self.0 - other.0) }
-}
-
-impl<'a> Sub<&'a FeeRate> for &FeeRate {
-    type Output = FeeRate;
-
-    fn sub(self, other: &'a FeeRate) -> <FeeRate as Add>::Output { FeeRate(self.0 - other.0) }
-}
+crate::internal_macros::impl_sub_for_references!(FeeRate);
+crate::internal_macros::impl_sub_assign!(FeeRate);
 
 /// Computes the ceiling so that the fee computation is conservative.
-impl Mul<FeeRate> for Weight {
+impl ops::Mul<FeeRate> for Weight {
     type Output = Amount;
 
     fn mul(self, rhs: FeeRate) -> Self::Output {
         Amount::from_sat((rhs.to_sat_per_kwu() * self.to_wu() + 999) / 1000)
     }
 }
 
-impl Mul<Weight> for FeeRate {
+impl ops::Mul<Weight> for FeeRate {
     type Output = Amount;
 
     fn mul(self, rhs: Weight) -> Self::Output { rhs * self }
 }
 
-impl Div<Weight> for Amount {
+impl ops::Div<Weight> for Amount {
     type Output = FeeRate;
 
     /// Truncating integer division.
@@ -258,22 +225,6 @@ impl Div<Weight> for Amount {
     fn div(self, rhs: Weight) -> Self::Output { FeeRate(self.to_sat() * 1000 / rhs.to_wu()) }
 }
 
-impl AddAssign for FeeRate {
-    fn add_assign(&mut self, rhs: Self) { self.0 += rhs.0 }
-}
-
-impl AddAssign<&FeeRate> for FeeRate {
-    fn add_assign(&mut self, rhs: &FeeRate) { self.0 += rhs.0 }
-}
-
-impl SubAssign for FeeRate {
-    fn sub_assign(&mut self, rhs: Self) { self.0 -= rhs.0 }
-}
-
-impl SubAssign<&FeeRate> for FeeRate {
-    fn sub_assign(&mut self, rhs: &FeeRate) { self.0 -= rhs.0 }
-}
-
 impl core::iter::Sum for FeeRate {
     fn sum<I>(iter: I) -> Self
     where

units/src/internal_macros.rs

@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: CC0-1.0
+
+//! Internal macros.
+//!
+//! Macros meant to be used inside the `bitcoin-units` library.
+
+/// Implements `ops::Add` for various references.
+///
+/// Requires `$ty` it implement `Add` e.g. 'impl Add<T> for T'. Adds impls of:
+///
+/// - Add<T> for &T
+/// - Add<&T> for T
+/// - Add<&T> for &T
+macro_rules! impl_add_for_references {
+    ($ty:ident) => {
+        impl core::ops::Add<$ty> for &$ty {
+            type Output = $ty;
+
+            fn add(self, rhs: $ty) -> Self::Output { *self + rhs }
+        }
+
+        impl core::ops::Add<&$ty> for $ty {
+            type Output = $ty;
+
+            fn add(self, rhs: &$ty) -> Self::Output { self + *rhs }
+        }
+
+        impl<'a> core::ops::Add<&'a $ty> for &$ty {
+            type Output = $ty;
+
+            fn add(self, rhs: &'a $ty) -> Self::Output { *self + *rhs }
+        }
+    }
+}
+pub(crate) use impl_add_for_references;
+
+/// Implement `ops::AddAssign` for `$ty` and `&$ty`.
+macro_rules! impl_add_assign {
+    ($ty:ident) => {
+         impl core::ops::AddAssign<$ty> for $ty {
+            fn add_assign(&mut self, rhs: $ty) { *self = *self + rhs }
+        }
+
+        impl core::ops::AddAssign<&$ty> for $ty {
+            fn add_assign(&mut self, rhs: &$ty) { *self = *self + *rhs }
+        }
+    }
+}
+pub(crate) use impl_add_assign;
+
+/// Implement `ops::Sub` for various references.
+///
+/// Requires `$ty` it implement `Sub` e.g. 'impl Sub<T> for T'. Adds impls of:
+///
+/// - Sub<T> for &T
+/// - Sub<&T> for T
+/// - Sub<&T> for &T
+macro_rules! impl_sub_for_references {
+    ($ty:ident) => {
+        impl core::ops::Sub<$ty> for &$ty {
+            type Output = $ty;
+
+            fn sub(self, rhs: $ty) -> Self::Output { *self - rhs }
+        }
+
+        impl core::ops::Sub<&$ty> for $ty {
+            type Output = $ty;
+
+            fn sub(self, rhs: &$ty) -> Self::Output { self - *rhs }
+        }
+
+        impl<'a> core::ops::Sub<&'a $ty> for &$ty {
+            type Output = $ty;
+
+            fn sub(self, rhs: &'a $ty) -> Self::Output { *self - *rhs }
+        }
+    }
+}
+pub(crate) use impl_sub_for_references;
+
+/// Implement `ops::SubAssign` for `$ty` and `&$ty`.
+macro_rules! impl_sub_assign {
+    ($ty:ident) => {
+         impl core::ops::SubAssign<$ty> for $ty {
+            fn sub_assign(&mut self, rhs: $ty) { *self = *self - rhs }
+        }
+
+        impl core::ops::SubAssign<&$ty> for $ty {
+            fn sub_assign(&mut self, rhs: &$ty) { *self = *self - *rhs }
+        }
+   }
+}
+pub(crate) use impl_sub_assign;

units/src/lib.rs

@@ -23,6 +23,8 @@ extern crate alloc;
 #[cfg(feature = "std")]
 extern crate std;
 
+mod internal_macros;
+
 #[doc(hidden)]
 pub mod _export {
     /// A re-export of core::*