From c90559de8e96172abf0ef1abf8ba55eefcc46f66 Mon Sep 17 00:00:00 2001
From: "Tobin C. Harding" <me@tobin.cc>
Date: Mon, 17 Feb 2025 14:25:21 +1100
Subject: [PATCH 1/9] Derive Copy for NumOpResult

The `NumOpResult` type is way more ergonomic to use if it derives
`Copy`. This restricts the `NumOpResult` to being `Copy` as well.

This does restrict what we can include in the error type in the future.

Derive Copy for `NumOpResult` and `NumOpResult`.
---
 units/src/amount/result.rs |  4 +--
 units/src/amount/tests.rs  | 50 +++++++++++++++++++-------------------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index 645b3edde..df26be7c7 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -12,7 +12,7 @@ use super::{Amount, SignedAmount};
 /// Result of an operation on [`Amount`] or [`SignedAmount`].
 ///
 /// The type parameter `T` should be normally `Amout` or `SignedAmount`.
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
 #[must_use]
 pub enum NumOpResult<T> {
     /// Result of a successful mathematical operation.
@@ -584,7 +584,7 @@ impl OptionExt<SignedAmount> for Option<SignedAmount> {
 }
 
 /// An error occurred while doing a mathematical operation.
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
 #[non_exhaustive]
 pub struct NumOpError;
 
diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs
index f5b450fdb..fa2db4874 100644
--- a/units/src/amount/tests.rs
+++ b/units/src/amount/tests.rs
@@ -1329,44 +1329,44 @@ fn amount_op_result_all_ops() {
     // let sres: NumOpResult<SignedAmount> = ssat + ssat;
 
     // Operations that where RHS is the result of another operation.
-    let _ = sat + res.clone();
-    let _ = &sat + res.clone();
-    // let _ = sat + &res.clone();
-    // let _ = &sat + &res.clone();
+    let _ = sat + res;
+    let _ = &sat + res;
+    // let _ = sat + &res;
+    // let _ = &sat + &res;
 
-    let _ = sat - res.clone();
-    let _ = &sat - res.clone();
-    // let _ = sat - &res.clone();
-    // let _ = &sat - &res.clone();
+    let _ = sat - res;
+    let _ = &sat - res;
+    // let _ = sat - &res;
+    // let _ = &sat - &res;
 
     // Operations that where LHS is the result of another operation.
-    let _ = res.clone() + sat;
-    // let _ = &res.clone() + sat;
-    let _ = res.clone() + &sat;
-    // let _ = &res.clone() + &sat;
+    let _ = res + sat;
+    // let _ = &res + sat;
+    let _ = res + &sat;
+    // let _ = &res + &sat;
 
-    let _ = res.clone() - sat;
-    // let _ = &res.clone() - sat;
-    let _ = res.clone() - &sat;
-    // let _ = &res.clone() - &sat;
+    let _ = res - sat;
+    // let _ = &res - sat;
+    let _ = res - &sat;
+    // let _ = &res - &sat;
 
     // Operations that where both sides are the result of another operation.
-    let _ = res.clone() + res.clone();
-    // let _ = &res.clone() + res.clone();
-    // let _ = res.clone() + &res.clone();
-    // let _ = &res.clone() + &res.clone();
+    let _ = res + res;
+    // let _ = &res + res;
+    // let _ = res + &res;
+    // let _ = &res + &res;
 
-    let _ = res.clone() - res.clone();
-    // let _ = &res.clone() - res.clone();
-    // let _ = res.clone() - &res.clone();
-    // let _ = &res.clone() - &res.clone();
+    let _ = res - res;
+    // let _ = &res - res;
+    // let _ = res - &res;
+    // let _ = &res - &res;
 }
 
 // Verify we have implemented all `Sum` for the `NumOpResult` type.
 #[test]
 fn amount_op_result_sum() {
     let res = Amount::from_sat(1) + Amount::from_sat(1);
-    let amounts = [res.clone(), res.clone()];
+    let amounts = [res, res];
     let amount_refs = [&res, &res];
 
     // Sum iterators.

From 2da332e04a7092aa8430f2383b7162775fc27924 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 18 Feb 2025 15:15:49 +0000
Subject: [PATCH 2/9] units: introduce impl_op_for_references and use it in
 three places

This macro can generally handle a lot of different cases where we
implement "the same trait but on references". We introduce it here and
use it in two places. We will use it in many more, but I wanted to make
the diff small on this commit, which introduces the actual macro code
and might take a bit of reading to understand.

You may want to use --color-moved-ws=allow-indentation-change to review
this, and the next commit.

The next set of changes will mechanically delete other macros that are
made redundant by this.
---
 units/src/amount/result.rs   | 31 +++++++++++----------
 units/src/internal_macros.rs | 53 ++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index df26be7c7..a8582a5e1 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -123,23 +123,23 @@ impl From<&SignedAmount> for NumOpResult<SignedAmount> {
     fn from(a: &SignedAmount) -> Self { Self::Valid(*a) }
 }
 
-impl ops::Add for Amount {
-    type Output = NumOpResult<Amount>;
+crate::internal_macros::impl_op_for_references! {
+    impl ops::Add<Amount> for Amount {
+        type Output = NumOpResult<Amount>;
 
-    fn add(self, rhs: Amount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
+        fn add(self, rhs: Amount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
+    }
 }
-crate::internal_macros::impl_add_for_amount_references!(Amount);
 
-impl ops::Add<NumOpResult<Amount>> for Amount {
-    type Output = NumOpResult<Amount>;
+crate::internal_macros::impl_op_for_references! {
+    impl ops::Add<NumOpResult<Amount>> for Amount {
+        type Output = NumOpResult<Amount>;
 
-    fn add(self, rhs: NumOpResult<Amount>) -> Self::Output { rhs.and_then(|a| a + self) }
+        fn add(self, rhs: NumOpResult<Amount>) -> Self::Output { rhs.and_then(|a| a + self) }
+    }
 }
-impl ops::Add<NumOpResult<Amount>> for &Amount {
-    type Output = NumOpResult<Amount>;
 
-    fn add(self, rhs: NumOpResult<Amount>) -> Self::Output { rhs.and_then(|a| a + self) }
-}
+// FIXME these two should be covered by generic impls below
 impl ops::Add<Amount> for NumOpResult<Amount> {
     type Output = NumOpResult<Amount>;
 
@@ -151,12 +151,13 @@ impl ops::Add<&Amount> for NumOpResult<Amount> {
     fn add(self, rhs: &Amount) -> Self::Output { rhs + self }
 }
 
-impl ops::Sub for Amount {
-    type Output = NumOpResult<Amount>;
+crate::internal_macros::impl_op_for_references! {
+    impl ops::Sub<Amount> for Amount {
+        type Output = NumOpResult<Amount>;
 
-    fn sub(self, rhs: Amount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
+        fn sub(self, rhs: Amount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
+    }
 }
-crate::internal_macros::impl_sub_for_amount_references!(Amount);
 
 impl ops::Sub<NumOpResult<Amount>> for Amount {
     type Output = NumOpResult<Amount>;
diff --git a/units/src/internal_macros.rs b/units/src/internal_macros.rs
index 1bae68986..d65167518 100644
--- a/units/src/internal_macros.rs
+++ b/units/src/internal_macros.rs
@@ -4,6 +4,59 @@
 //!
 //! Macros meant to be used inside the `bitcoin-units` library.
 
+/// Implements an opcode for various reference combinations.
+///
+/// Given `$ty`, assumes the `$op_trait<$other_ty>` trait is implemented on it,
+/// and implements the same trait with the full matrix of `&$ty` and `&$other_ty`:
+///
+/// - `Add<$other_ty> for &$ty`
+/// - `Add<&$other_ty> for $ty`
+/// - `Add<&$other_ty> for &$ty`
+///
+/// # Limitations
+///
+/// You must specify `$other_ty` and you may not use `Self`. So e.g. you need
+/// to write `impl ops::Add<Amount> for Amount { ... }` when calling this macro.
+macro_rules! impl_op_for_references {
+    (
+        impl $($op_trait:ident)::+<$other_ty:ty> for $ty:ty {
+            type Output = $($main_output:ty)*;
+            fn $op:ident($($main_args:tt)*) -> Self::Output {
+                $($main_impl:tt)*
+            }
+        }
+    ) => {
+        impl $($op_trait)::+<$other_ty> for $ty {
+            type Output = $($main_output)*;
+            fn $op($($main_args)*) -> Self::Output {
+                $($main_impl)*
+            }
+        }
+
+        impl $($op_trait)::+<$other_ty> for &$ty {
+            type Output = <$ty as $($op_trait)::+<$other_ty>>::Output;
+            fn $op(self, rhs: $other_ty) -> Self::Output {
+                (*self).$op(rhs)
+            }
+        }
+
+        impl $($op_trait)::+<&$other_ty> for $ty {
+            type Output = <$ty as $($op_trait)::+<$other_ty>>::Output;
+            fn $op(self, rhs: &$other_ty) -> Self::Output {
+                self.$op(*rhs)
+            }
+        }
+
+        impl<'a> $($op_trait)::+<&'a $other_ty> for &$ty {
+            type Output = <$ty as $($op_trait)::+<$other_ty>>::Output;
+            fn $op(self, rhs: &$other_ty) -> Self::Output {
+                (*self).$op(*rhs)
+            }
+        }
+    };
+}
+pub(crate) use impl_op_for_references;
+
 /// Implements `ops::Add` for various references.
 ///
 /// Requires `$ty` it implement `Add` e.g. 'impl Add<T> for T'. Adds impls of:

From a358e79a85b0677fc5a51de980add87c4f28cf41 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 18 Feb 2025 15:55:14 +0000
Subject: [PATCH 3/9] units: allow multiple invocations in
 impl_op_for_references macro

This is not too complicated a change to support and it will reduce the
noise in the following commits a fair bit.
---
 units/src/amount/result.rs   | 2 --
 units/src/internal_macros.rs | 6 +++---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index a8582a5e1..4d052ff6b 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -129,9 +129,7 @@ crate::internal_macros::impl_op_for_references! {
 
         fn add(self, rhs: Amount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
     }
-}
 
-crate::internal_macros::impl_op_for_references! {
     impl ops::Add<NumOpResult<Amount>> for Amount {
         type Output = NumOpResult<Amount>;
 
diff --git a/units/src/internal_macros.rs b/units/src/internal_macros.rs
index d65167518..d5d0f14d1 100644
--- a/units/src/internal_macros.rs
+++ b/units/src/internal_macros.rs
@@ -18,14 +18,14 @@
 /// You must specify `$other_ty` and you may not use `Self`. So e.g. you need
 /// to write `impl ops::Add<Amount> for Amount { ... }` when calling this macro.
 macro_rules! impl_op_for_references {
-    (
+    ($(
         impl $($op_trait:ident)::+<$other_ty:ty> for $ty:ty {
             type Output = $($main_output:ty)*;
             fn $op:ident($($main_args:tt)*) -> Self::Output {
                 $($main_impl:tt)*
             }
         }
-    ) => {
+    )+) => {$(
         impl $($op_trait)::+<$other_ty> for $ty {
             type Output = $($main_output)*;
             fn $op($($main_args)*) -> Self::Output {
@@ -53,7 +53,7 @@ macro_rules! impl_op_for_references {
                 (*self).$op(*rhs)
             }
         }
-    };
+    )+};
 }
 pub(crate) use impl_op_for_references;
 

From 0dc7f6cebdf92e14b73b1e74639f9cce531f5847 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 18 Feb 2025 16:20:17 +0000
Subject: [PATCH 4/9] units: rearrange a bit of code to prep the next commit

The next commit changes a lot of code, but almost entirely by moving and
indenting it. We try to do the moves here ahead of time, so it the diff
for the next commit will be just deletions and indentations.
---
 units/src/amount/result.rs | 28 ++++++++++++++--------------
 units/src/fee_rate/mod.rs  |  6 +++---
 units/src/weight.rs        |  6 +++---
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index 4d052ff6b..98ad8df4f 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -137,18 +137,6 @@ crate::internal_macros::impl_op_for_references! {
     }
 }
 
-// FIXME these two should be covered by generic impls below
-impl ops::Add<Amount> for NumOpResult<Amount> {
-    type Output = NumOpResult<Amount>;
-
-    fn add(self, rhs: Amount) -> Self::Output { rhs + self }
-}
-impl ops::Add<&Amount> for NumOpResult<Amount> {
-    type Output = NumOpResult<Amount>;
-
-    fn add(self, rhs: &Amount) -> Self::Output { rhs + self }
-}
-
 crate::internal_macros::impl_op_for_references! {
     impl ops::Sub<Amount> for Amount {
         type Output = NumOpResult<Amount>;
@@ -261,7 +249,19 @@ impl ops::Rem<&u64> for &Amount {
     fn rem(self, modulus: &u64) -> Self::Output { (*self).rem(*modulus) }
 }
 
-impl ops::Add for SignedAmount {
+// FIXME these two should be covered by generic impls below
+impl ops::Add<Amount> for NumOpResult<Amount> {
+    type Output = NumOpResult<Amount>;
+
+    fn add(self, rhs: Amount) -> Self::Output { rhs + self }
+}
+impl ops::Add<&Amount> for NumOpResult<Amount> {
+    type Output = NumOpResult<Amount>;
+
+    fn add(self, rhs: &Amount) -> Self::Output { rhs + self }
+}
+
+impl ops::Add<SignedAmount> for SignedAmount {
     type Output = NumOpResult<SignedAmount>;
 
     fn add(self, rhs: SignedAmount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
@@ -289,7 +289,7 @@ impl ops::Add<&SignedAmount> for NumOpResult<SignedAmount> {
     fn add(self, rhs: &SignedAmount) -> Self::Output { rhs + self }
 }
 
-impl ops::Sub for SignedAmount {
+impl ops::Sub<SignedAmount> for SignedAmount {
     type Output = NumOpResult<SignedAmount>;
 
     fn sub(self, rhs: SignedAmount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs
index f9dca7b87..1579578cc 100644
--- a/units/src/fee_rate/mod.rs
+++ b/units/src/fee_rate/mod.rs
@@ -135,20 +135,20 @@ impl From<FeeRate> for u64 {
     fn from(value: FeeRate) -> Self { value.to_sat_per_kwu() }
 }
 
-impl ops::Add for FeeRate {
+impl ops::Add<FeeRate> 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 ops::Sub for FeeRate {
+impl ops::Sub<FeeRate> for FeeRate {
     type Output = FeeRate;
 
     fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 - rhs.0) }
 }
 crate::internal_macros::impl_sub_for_references!(FeeRate);
+crate::internal_macros::impl_add_assign!(FeeRate);
 crate::internal_macros::impl_sub_assign!(FeeRate);
 
 impl core::iter::Sum for FeeRate {
diff --git a/units/src/weight.rs b/units/src/weight.rs
index df654e9d4..a1ea4fa80 100644
--- a/units/src/weight.rs
+++ b/units/src/weight.rs
@@ -166,20 +166,20 @@ impl From<Weight> for u64 {
     fn from(value: Weight) -> Self { value.to_wu() }
 }
 
-impl ops::Add for Weight {
+impl ops::Add<Weight> for Weight {
     type Output = Weight;
 
     fn add(self, rhs: Weight) -> Self::Output { Weight(self.0 + rhs.0) }
 }
 crate::internal_macros::impl_add_for_references!(Weight);
-crate::internal_macros::impl_add_assign!(Weight);
 
-impl ops::Sub for Weight {
+impl ops::Sub<Weight> for Weight {
     type Output = Weight;
 
     fn sub(self, rhs: Weight) -> Self::Output { Weight(self.0 - rhs.0) }
 }
 crate::internal_macros::impl_sub_for_references!(Weight);
+crate::internal_macros::impl_add_assign!(Weight);
 crate::internal_macros::impl_sub_assign!(Weight);
 
 impl ops::Mul<u64> for Weight {

From ad9564895b2ada7bd0ff510a91986b9b02a45ca8 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 18 Feb 2025 16:24:05 +0000
Subject: [PATCH 5/9] units: replace a gazillion more macro calls with new
 macro

Looks like a large diff but if you run

    git show --color-moved-ws=allow-indentation-change

you will see that it's 100% moves (though moves of code into the
reference macro). May be easier to just look at src/amount/result.rs
after this; it's pretty short now.
---
 units/src/amount/result.rs   | 322 +++++++++--------------------------
 units/src/fee_rate/mod.rs    |  22 +--
 units/src/internal_macros.rs | 120 -------------
 units/src/weight.rs          |  66 ++++---
 4 files changed, 127 insertions(+), 403 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index 98ad8df4f..67f76a6c2 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -135,270 +135,118 @@ crate::internal_macros::impl_op_for_references! {
 
         fn add(self, rhs: NumOpResult<Amount>) -> Self::Output { rhs.and_then(|a| a + self) }
     }
-}
 
-crate::internal_macros::impl_op_for_references! {
     impl ops::Sub<Amount> for Amount {
         type Output = NumOpResult<Amount>;
 
         fn sub(self, rhs: Amount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
     }
-}
 
-impl ops::Sub<NumOpResult<Amount>> for Amount {
-    type Output = NumOpResult<Amount>;
+    impl ops::Sub<NumOpResult<Amount>> for Amount {
+        type Output = NumOpResult<Amount>;
 
-    fn sub(self, rhs: NumOpResult<Amount>) -> Self::Output {
-        match rhs {
-            R::Valid(amount) => self - amount,
-            R::Error(_) => rhs,
+        fn sub(self, rhs: NumOpResult<Amount>) -> Self::Output {
+            match rhs {
+                R::Valid(amount) => self - amount,
+                R::Error(_) => rhs,
+            }
         }
     }
-}
-impl ops::Sub<NumOpResult<Amount>> for &Amount {
-    type Output = NumOpResult<Amount>;
 
-    fn sub(self, rhs: NumOpResult<Amount>) -> Self::Output {
-        match rhs {
-            R::Valid(amount) => self - amount,
-            R::Error(_) => rhs,
+    impl ops::Mul<u64> for Amount {
+        type Output = NumOpResult<Amount>;
+
+        fn mul(self, rhs: u64) -> Self::Output { self.checked_mul(rhs).valid_or_error() }
+    }
+    impl ops::Div<u64> for Amount {
+        type Output = NumOpResult<Amount>;
+
+        fn div(self, rhs: u64) -> Self::Output { self.checked_div(rhs).valid_or_error() }
+    }
+    impl ops::Rem<u64> for Amount {
+        type Output = NumOpResult<Amount>;
+
+        fn rem(self, modulus: u64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
+    }
+
+    // FIXME these two should be covered by generic impls below
+    impl ops::Add<Amount> for NumOpResult<Amount> {
+        type Output = NumOpResult<Amount>;
+
+        fn add(self, rhs: Amount) -> Self::Output { rhs + self }
+    }
+    impl ops::Sub<Amount> for NumOpResult<Amount> {
+        type Output = NumOpResult<Amount>;
+
+        fn sub(self, rhs: Amount) -> Self::Output {
+            match self {
+                R::Valid(amount) => amount - rhs,
+                R::Error(_) => self,
+            }
         }
     }
-}
-impl ops::Sub<Amount> for NumOpResult<Amount> {
-    type Output = NumOpResult<Amount>;
 
-    fn sub(self, rhs: Amount) -> Self::Output {
-        match self {
-            R::Valid(amount) => amount - rhs,
-            R::Error(_) => self,
+    impl ops::Add<SignedAmount> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn add(self, rhs: SignedAmount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
+    }
+
+    impl ops::Add<NumOpResult<SignedAmount>> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn add(self, rhs: NumOpResult<SignedAmount>) -> Self::Output { rhs.and_then(|a| a + self) }
+    }
+
+    impl ops::Sub<SignedAmount> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn sub(self, rhs: SignedAmount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
+    }
+    impl ops::Sub<NumOpResult<SignedAmount>> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn sub(self, rhs: NumOpResult<SignedAmount>) -> Self::Output {
+            match rhs {
+                R::Valid(amount) => amount - rhs,
+                R::Error(_) => rhs,
+            }
         }
     }
-}
-impl ops::Sub<&Amount> for NumOpResult<Amount> {
-    type Output = NumOpResult<Amount>;
 
-    fn sub(self, rhs: &Amount) -> Self::Output {
-        match self {
-            R::Valid(amount) => amount - (*rhs),
-            R::Error(_) => self,
+    impl ops::Add<SignedAmount> for NumOpResult<SignedAmount> {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn add(self, rhs: SignedAmount) -> Self::Output { rhs + self }
+    }
+
+    impl ops::Sub<SignedAmount> for NumOpResult<SignedAmount> {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn sub(self, rhs: SignedAmount) -> Self::Output {
+            match self {
+                R::Valid(amount) => amount - rhs,
+                R::Error(_) => self,
+            }
         }
     }
-}
 
-impl ops::Mul<u64> for Amount {
-    type Output = NumOpResult<Amount>;
+    impl ops::Mul<i64> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
 
-    fn mul(self, rhs: u64) -> Self::Output { self.checked_mul(rhs).valid_or_error() }
-}
-impl ops::Mul<&u64> for Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn mul(self, rhs: &u64) -> Self::Output { self.mul(*rhs) }
-}
-impl ops::Mul<u64> for &Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn mul(self, rhs: u64) -> Self::Output { (*self).mul(rhs) }
-}
-impl ops::Mul<&u64> for &Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn mul(self, rhs: &u64) -> Self::Output { self.mul(*rhs) }
-}
-
-impl ops::Div<u64> for Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn div(self, rhs: u64) -> Self::Output { self.checked_div(rhs).valid_or_error() }
-}
-impl ops::Div<&u64> for Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn div(self, rhs: &u64) -> Self::Output { self.div(*rhs) }
-}
-impl ops::Div<u64> for &Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn div(self, rhs: u64) -> Self::Output { (*self).div(rhs) }
-}
-impl ops::Div<&u64> for &Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn div(self, rhs: &u64) -> Self::Output { (*self).div(*rhs) }
-}
-
-impl ops::Rem<u64> for Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn rem(self, modulus: u64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
-}
-impl ops::Rem<&u64> for Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn rem(self, modulus: &u64) -> Self::Output { self.rem(*modulus) }
-}
-impl ops::Rem<u64> for &Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn rem(self, modulus: u64) -> Self::Output { (*self).rem(modulus) }
-}
-impl ops::Rem<&u64> for &Amount {
-    type Output = NumOpResult<Amount>;
-
-    fn rem(self, modulus: &u64) -> Self::Output { (*self).rem(*modulus) }
-}
-
-// FIXME these two should be covered by generic impls below
-impl ops::Add<Amount> for NumOpResult<Amount> {
-    type Output = NumOpResult<Amount>;
-
-    fn add(self, rhs: Amount) -> Self::Output { rhs + self }
-}
-impl ops::Add<&Amount> for NumOpResult<Amount> {
-    type Output = NumOpResult<Amount>;
-
-    fn add(self, rhs: &Amount) -> Self::Output { rhs + self }
-}
-
-impl ops::Add<SignedAmount> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn add(self, rhs: SignedAmount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
-}
-crate::internal_macros::impl_add_for_amount_references!(SignedAmount);
-
-impl ops::Add<NumOpResult<SignedAmount>> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn add(self, rhs: NumOpResult<SignedAmount>) -> Self::Output { rhs.and_then(|a| a + self) }
-}
-impl ops::Add<NumOpResult<SignedAmount>> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn add(self, rhs: NumOpResult<SignedAmount>) -> Self::Output { rhs.and_then(|a| a + self) }
-}
-impl ops::Add<SignedAmount> for NumOpResult<SignedAmount> {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn add(self, rhs: SignedAmount) -> Self::Output { rhs + self }
-}
-impl ops::Add<&SignedAmount> for NumOpResult<SignedAmount> {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn add(self, rhs: &SignedAmount) -> Self::Output { rhs + self }
-}
-
-impl ops::Sub<SignedAmount> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn sub(self, rhs: SignedAmount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
-}
-crate::internal_macros::impl_sub_for_amount_references!(SignedAmount);
-
-impl ops::Sub<NumOpResult<SignedAmount>> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn sub(self, rhs: NumOpResult<SignedAmount>) -> Self::Output {
-        match rhs {
-            R::Valid(amount) => amount - rhs,
-            R::Error(_) => rhs,
-        }
+        fn mul(self, rhs: i64) -> Self::Output { self.checked_mul(rhs).valid_or_error() }
     }
-}
-impl ops::Sub<NumOpResult<SignedAmount>> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
+    impl ops::Div<i64> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
 
-    fn sub(self, rhs: NumOpResult<SignedAmount>) -> Self::Output {
-        match rhs {
-            R::Valid(amount) => amount - rhs,
-            R::Error(_) => rhs,
-        }
+        fn div(self, rhs: i64) -> Self::Output { self.checked_div(rhs).valid_or_error() }
     }
-}
-impl ops::Sub<SignedAmount> for NumOpResult<SignedAmount> {
-    type Output = NumOpResult<SignedAmount>;
+    impl ops::Rem<i64> for SignedAmount {
+        type Output = NumOpResult<SignedAmount>;
 
-    fn sub(self, rhs: SignedAmount) -> Self::Output {
-        match self {
-            R::Valid(amount) => amount - rhs,
-            R::Error(_) => self,
-        }
+        fn rem(self, modulus: i64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
     }
 }
-impl ops::Sub<&SignedAmount> for NumOpResult<SignedAmount> {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn sub(self, rhs: &SignedAmount) -> Self::Output {
-        match self {
-            R::Valid(amount) => amount - *rhs,
-            R::Error(_) => self,
-        }
-    }
-}
-
-impl ops::Mul<i64> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn mul(self, rhs: i64) -> Self::Output { self.checked_mul(rhs).valid_or_error() }
-}
-impl ops::Mul<&i64> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn mul(self, rhs: &i64) -> Self::Output { self.mul(*rhs) }
-}
-impl ops::Mul<i64> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn mul(self, rhs: i64) -> Self::Output { (*self).mul(rhs) }
-}
-impl ops::Mul<&i64> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn mul(self, rhs: &i64) -> Self::Output { self.mul(*rhs) }
-}
-
-impl ops::Div<i64> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn div(self, rhs: i64) -> Self::Output { self.checked_div(rhs).valid_or_error() }
-}
-impl ops::Div<&i64> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn div(self, rhs: &i64) -> Self::Output { self.div(*rhs) }
-}
-impl ops::Div<i64> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn div(self, rhs: i64) -> Self::Output { (*self).div(rhs) }
-}
-impl ops::Div<&i64> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn div(self, rhs: &i64) -> Self::Output { (*self).div(*rhs) }
-}
-
-impl ops::Rem<i64> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn rem(self, modulus: i64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
-}
-impl ops::Rem<&i64> for SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn rem(self, modulus: &i64) -> Self::Output { self.rem(*modulus) }
-}
-impl ops::Rem<i64> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn rem(self, modulus: i64) -> Self::Output { (*self).rem(modulus) }
-}
-impl ops::Rem<&i64> for &SignedAmount {
-    type Output = NumOpResult<SignedAmount>;
-
-    fn rem(self, modulus: &i64) -> Self::Output { (*self).rem(*modulus) }
-}
 
 impl ops::Neg for SignedAmount {
     type Output = Self;
diff --git a/units/src/fee_rate/mod.rs b/units/src/fee_rate/mod.rs
index 1579578cc..f3ec8fd7d 100644
--- a/units/src/fee_rate/mod.rs
+++ b/units/src/fee_rate/mod.rs
@@ -135,19 +135,19 @@ impl From<FeeRate> for u64 {
     fn from(value: FeeRate) -> Self { value.to_sat_per_kwu() }
 }
 
-impl ops::Add<FeeRate> for FeeRate {
-    type Output = FeeRate;
+crate::internal_macros::impl_op_for_references! {
+    impl ops::Add<FeeRate> for FeeRate {
+        type Output = FeeRate;
 
-    fn add(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 + rhs.0) }
+        fn add(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 + rhs.0) }
+    }
+
+    impl ops::Sub<FeeRate> for FeeRate {
+        type Output = FeeRate;
+
+        fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 - rhs.0) }
+    }
 }
-crate::internal_macros::impl_add_for_references!(FeeRate);
-
-impl ops::Sub<FeeRate> for FeeRate {
-    type Output = FeeRate;
-
-    fn sub(self, rhs: FeeRate) -> Self::Output { FeeRate(self.0 - rhs.0) }
-}
-crate::internal_macros::impl_sub_for_references!(FeeRate);
 crate::internal_macros::impl_add_assign!(FeeRate);
 crate::internal_macros::impl_sub_assign!(FeeRate);
 
diff --git a/units/src/internal_macros.rs b/units/src/internal_macros.rs
index d5d0f14d1..668272aeb 100644
--- a/units/src/internal_macros.rs
+++ b/units/src/internal_macros.rs
@@ -57,66 +57,6 @@ macro_rules! impl_op_for_references {
 }
 pub(crate) use impl_op_for_references;
 
-/// 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;
-
-/// Implements `ops::Add` for various amount 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_amount_references {
-    ($ty:ident) => {
-        impl core::ops::Add<$ty> for &$ty {
-            type Output = NumOpResult<$ty>;
-
-            fn add(self, rhs: $ty) -> Self::Output { *self + rhs }
-        }
-
-        impl core::ops::Add<&$ty> for $ty {
-            type Output = NumOpResult<$ty>;
-
-            fn add(self, rhs: &$ty) -> Self::Output { self + *rhs }
-        }
-
-        impl<'a> core::ops::Add<&'a $ty> for &$ty {
-            type Output = NumOpResult<$ty>;
-
-            fn add(self, rhs: &'a $ty) -> Self::Output { *self + *rhs }
-        }
-    };
-}
-pub(crate) use impl_add_for_amount_references;
-
 /// Implement `ops::AddAssign` for `$ty` and `&$ty`.
 macro_rules! impl_add_assign {
     ($ty:ident) => {
@@ -131,66 +71,6 @@ macro_rules! impl_add_assign {
 }
 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::Sub` for various amount 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_amount_references {
-    ($ty:ident) => {
-        impl core::ops::Sub<$ty> for &$ty {
-            type Output = NumOpResult<$ty>;
-
-            fn sub(self, rhs: $ty) -> Self::Output { *self - rhs }
-        }
-
-        impl core::ops::Sub<&$ty> for $ty {
-            type Output = NumOpResult<$ty>;
-
-            fn sub(self, rhs: &$ty) -> Self::Output { self - *rhs }
-        }
-
-        impl<'a> core::ops::Sub<&'a $ty> for &$ty {
-            type Output = NumOpResult<$ty>;
-
-            fn sub(self, rhs: &'a $ty) -> Self::Output { *self - *rhs }
-        }
-    };
-}
-pub(crate) use impl_sub_for_amount_references;
-
 /// Implement `ops::SubAssign` for `$ty` and `&$ty`.
 macro_rules! impl_sub_assign {
     ($ty:ident) => {
diff --git a/units/src/weight.rs b/units/src/weight.rs
index a1ea4fa80..720dfd385 100644
--- a/units/src/weight.rs
+++ b/units/src/weight.rs
@@ -166,50 +166,46 @@ impl From<Weight> for u64 {
     fn from(value: Weight) -> Self { value.to_wu() }
 }
 
-impl ops::Add<Weight> for Weight {
-    type Output = Weight;
+crate::internal_macros::impl_op_for_references! {
+    impl ops::Add<Weight> for Weight {
+        type Output = Weight;
 
-    fn add(self, rhs: Weight) -> Self::Output { Weight(self.0 + rhs.0) }
+        fn add(self, rhs: Weight) -> Self::Output { Weight(self.0 + rhs.0) }
+    }
+    impl ops::Sub<Weight> for Weight {
+        type Output = Weight;
+
+        fn sub(self, rhs: Weight) -> Self::Output { Weight(self.0 - rhs.0) }
+    }
+
+    impl ops::Mul<u64> for Weight {
+        type Output = Weight;
+
+        fn mul(self, rhs: u64) -> Self::Output { Weight(self.0 * rhs) }
+    }
+    impl ops::Mul<Weight> for u64 {
+        type Output = Weight;
+
+        fn mul(self, rhs: Weight) -> Self::Output { Weight(self * rhs.0) }
+    }
+    impl ops::Div<u64> for Weight {
+        type Output = Weight;
+
+        fn div(self, rhs: u64) -> Self::Output { Weight(self.0 / rhs) }
+    }
+    impl ops::Div<Weight> for Weight {
+        type Output = u64;
+
+        fn div(self, rhs: Weight) -> Self::Output { self.to_wu() / rhs.to_wu() }
+    }
 }
-crate::internal_macros::impl_add_for_references!(Weight);
-
-impl ops::Sub<Weight> for Weight {
-    type Output = Weight;
-
-    fn sub(self, rhs: Weight) -> Self::Output { Weight(self.0 - rhs.0) }
-}
-crate::internal_macros::impl_sub_for_references!(Weight);
 crate::internal_macros::impl_add_assign!(Weight);
 crate::internal_macros::impl_sub_assign!(Weight);
 
-impl ops::Mul<u64> for Weight {
-    type Output = Weight;
-
-    fn mul(self, rhs: u64) -> Self::Output { Weight(self.0 * rhs) }
-}
-
-impl ops::Mul<Weight> for u64 {
-    type Output = Weight;
-
-    fn mul(self, rhs: Weight) -> Self::Output { Weight(self * rhs.0) }
-}
-
 impl ops::MulAssign<u64> for Weight {
     fn mul_assign(&mut self, rhs: u64) { self.0 *= rhs }
 }
 
-impl ops::Div<u64> for Weight {
-    type Output = Weight;
-
-    fn div(self, rhs: u64) -> Self::Output { Weight(self.0 / rhs) }
-}
-
-impl ops::Div<Weight> for Weight {
-    type Output = u64;
-
-    fn div(self, rhs: Weight) -> Self::Output { self.to_wu() / rhs.to_wu() }
-}
-
 impl ops::DivAssign<u64> for Weight {
     fn div_assign(&mut self, rhs: u64) { self.0 /= rhs }
 }

From 21ac5aefe0afb6863545b94b79cec46a09857cd4 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 18 Feb 2025 19:58:15 +0000
Subject: [PATCH 6/9] units: extend op reference macro to handle generics and
 where clauses

This is a bit ugly and requires that we put our where-clauses in
parentheses because the macro_rules parser sucks, but it allows us to
move the blanket-impls on NumOpResult into the macro.

This commit moves one instance and updates the macro; the next commits
will change the rest.
---
 units/src/amount/result.rs   | 67 ++++++++----------------------------
 units/src/internal_macros.rs | 22 +++++++++---
 2 files changed, 31 insertions(+), 58 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index 67f76a6c2..a526fbd0b 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -246,6 +246,20 @@ crate::internal_macros::impl_op_for_references! {
 
         fn rem(self, modulus: i64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
     }
+
+    impl<T> ops::Add<NumOpResult<T>> for NumOpResult<T>
+    where
+        (T: Copy + ops::Add<Output = NumOpResult<T>>)
+    {
+        type Output = NumOpResult<T>;
+
+        fn add(self, rhs: Self) -> Self::Output {
+            match (self, rhs) {
+                (R::Valid(lhs), R::Valid(rhs)) => lhs + rhs,
+                (_, _) => R::Error(NumOpError {}),
+            }
+        }
+    }
 }
 
 impl ops::Neg for SignedAmount {
@@ -254,59 +268,6 @@ impl ops::Neg for SignedAmount {
     fn neg(self) -> Self::Output { Self::from_sat(self.to_sat().neg()) }
 }
 
-impl<T> ops::Add for NumOpResult<T>
-where
-    T: ops::Add<Output = NumOpResult<T>>,
-{
-    type Output = NumOpResult<T>;
-
-    fn add(self, rhs: Self) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => lhs + rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-impl<T> ops::Add<NumOpResult<T>> for &NumOpResult<T>
-where
-    T: ops::Add<Output = NumOpResult<T>> + Copy,
-{
-    type Output = NumOpResult<T>;
-
-    fn add(self, rhs: NumOpResult<T>) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => *lhs + rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-impl<T> ops::Add<&NumOpResult<T>> for NumOpResult<T>
-where
-    T: ops::Add<Output = NumOpResult<T>> + Copy,
-{
-    type Output = NumOpResult<T>;
-
-    fn add(self, rhs: &NumOpResult<T>) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => lhs + *rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-impl<T> ops::Add for &NumOpResult<T>
-where
-    T: ops::Add<Output = NumOpResult<T>> + Copy,
-{
-    type Output = NumOpResult<T>;
-
-    fn add(self, rhs: &NumOpResult<T>) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => *lhs + *rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-
 impl<T> ops::Sub for NumOpResult<T>
 where
     T: ops::Sub<Output = NumOpResult<T>>,
diff --git a/units/src/internal_macros.rs b/units/src/internal_macros.rs
index 668272aeb..c5f0ac6fc 100644
--- a/units/src/internal_macros.rs
+++ b/units/src/internal_macros.rs
@@ -17,37 +17,49 @@
 ///
 /// You must specify `$other_ty` and you may not use `Self`. So e.g. you need
 /// to write `impl ops::Add<Amount> for Amount { ... }` when calling this macro.
+///
+/// Your where clause must include extra parenthesis, like `where (T: Copy)`.
 macro_rules! impl_op_for_references {
     ($(
-        impl $($op_trait:ident)::+<$other_ty:ty> for $ty:ty {
+        impl$(<$gen:ident>)? $($op_trait:ident)::+<$other_ty:ty> for $ty:ty
+        $(where ($($bounds:tt)*))?
+        {
             type Output = $($main_output:ty)*;
             fn $op:ident($($main_args:tt)*) -> Self::Output {
                 $($main_impl:tt)*
             }
         }
     )+) => {$(
-        impl $($op_trait)::+<$other_ty> for $ty {
+        impl$(<$gen>)?  $($op_trait)::+<$other_ty> for $ty
+        $(where $($bounds)*)?
+        {
             type Output = $($main_output)*;
             fn $op($($main_args)*) -> Self::Output {
                 $($main_impl)*
             }
         }
 
-        impl $($op_trait)::+<$other_ty> for &$ty {
+        impl$(<$gen>)?  $($op_trait)::+<$other_ty> for &$ty
+        $(where $($bounds)*)?
+        {
             type Output = <$ty as $($op_trait)::+<$other_ty>>::Output;
             fn $op(self, rhs: $other_ty) -> Self::Output {
                 (*self).$op(rhs)
             }
         }
 
-        impl $($op_trait)::+<&$other_ty> for $ty {
+        impl$(<$gen>)?  $($op_trait)::+<&$other_ty> for $ty
+        $(where $($bounds)*)?
+        {
             type Output = <$ty as $($op_trait)::+<$other_ty>>::Output;
             fn $op(self, rhs: &$other_ty) -> Self::Output {
                 self.$op(*rhs)
             }
         }
 
-        impl<'a> $($op_trait)::+<&'a $other_ty> for &$ty {
+        impl<'a, $($gen)?> $($op_trait)::+<&'a $other_ty> for &$ty
+        $(where $($bounds)*)?
+        {
             type Output = <$ty as $($op_trait)::+<$other_ty>>::Output;
             fn $op(self, rhs: &$other_ty) -> Self::Output {
                 (*self).$op(*rhs)

From 353c23fa0146bf01c1f4ca9af4b8fdc4a8f092ec Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 18 Feb 2025 18:29:02 +0000
Subject: [PATCH 7/9] units: pull generic op impls on NumOpResult into macro

---
 units/src/amount/result.rs | 126 +++++++++++--------------------------
 1 file changed, 38 insertions(+), 88 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index a526fbd0b..b2091a1b5 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -169,23 +169,6 @@ crate::internal_macros::impl_op_for_references! {
         fn rem(self, modulus: u64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
     }
 
-    // FIXME these two should be covered by generic impls below
-    impl ops::Add<Amount> for NumOpResult<Amount> {
-        type Output = NumOpResult<Amount>;
-
-        fn add(self, rhs: Amount) -> Self::Output { rhs + self }
-    }
-    impl ops::Sub<Amount> for NumOpResult<Amount> {
-        type Output = NumOpResult<Amount>;
-
-        fn sub(self, rhs: Amount) -> Self::Output {
-            match self {
-                R::Valid(amount) => amount - rhs,
-                R::Error(_) => self,
-            }
-        }
-    }
-
     impl ops::Add<SignedAmount> for SignedAmount {
         type Output = NumOpResult<SignedAmount>;
 
@@ -208,29 +191,12 @@ crate::internal_macros::impl_op_for_references! {
 
         fn sub(self, rhs: NumOpResult<SignedAmount>) -> Self::Output {
             match rhs {
-                R::Valid(amount) => amount - rhs,
+                R::Valid(amount) => self - amount,
                 R::Error(_) => rhs,
             }
         }
     }
 
-    impl ops::Add<SignedAmount> for NumOpResult<SignedAmount> {
-        type Output = NumOpResult<SignedAmount>;
-
-        fn add(self, rhs: SignedAmount) -> Self::Output { rhs + self }
-    }
-
-    impl ops::Sub<SignedAmount> for NumOpResult<SignedAmount> {
-        type Output = NumOpResult<SignedAmount>;
-
-        fn sub(self, rhs: SignedAmount) -> Self::Output {
-            match self {
-                R::Valid(amount) => amount - rhs,
-                R::Error(_) => self,
-            }
-        }
-    }
-
     impl ops::Mul<i64> for SignedAmount {
         type Output = NumOpResult<SignedAmount>;
 
@@ -260,6 +226,43 @@ crate::internal_macros::impl_op_for_references! {
             }
         }
     }
+
+    impl<T> ops::Add<T> for NumOpResult<T>
+    where
+        (T: Copy + ops::Add<NumOpResult<T>, Output = NumOpResult<T>>)
+    {
+        type Output = NumOpResult<T>;
+
+        fn add(self, rhs: T) -> Self::Output { rhs + self }
+    }
+
+    impl<T> ops::Sub<NumOpResult<T>> for NumOpResult<T>
+    where
+        (T: Copy + ops::Sub<Output = NumOpResult<T>>)
+    {
+        type Output = NumOpResult<T>;
+
+        fn sub(self, rhs: Self) -> Self::Output {
+            match (self, rhs) {
+                (R::Valid(lhs), R::Valid(rhs)) => lhs - rhs,
+                (_, _) => R::Error(NumOpError {}),
+            }
+        }
+    }
+
+    impl<T> ops::Sub<T> for NumOpResult<T>
+    where
+        (T: Copy + ops::Sub<Output = NumOpResult<T>>)
+    {
+        type Output = NumOpResult<T>;
+
+        fn sub(self, rhs: T) -> Self::Output {
+            match self {
+                R::Valid(amount) => amount - rhs,
+                R::Error(_) => self,
+            }
+        }
+    }
 }
 
 impl ops::Neg for SignedAmount {
@@ -268,59 +271,6 @@ impl ops::Neg for SignedAmount {
     fn neg(self) -> Self::Output { Self::from_sat(self.to_sat().neg()) }
 }
 
-impl<T> ops::Sub for NumOpResult<T>
-where
-    T: ops::Sub<Output = NumOpResult<T>>,
-{
-    type Output = NumOpResult<T>;
-
-    fn sub(self, rhs: Self) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => lhs - rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-impl<T> ops::Sub<NumOpResult<T>> for &NumOpResult<T>
-where
-    T: ops::Sub<Output = NumOpResult<T>> + Copy,
-{
-    type Output = NumOpResult<T>;
-
-    fn sub(self, rhs: NumOpResult<T>) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => *lhs - rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-impl<T> ops::Sub<&NumOpResult<T>> for NumOpResult<T>
-where
-    T: ops::Sub<Output = NumOpResult<T>> + Copy,
-{
-    type Output = NumOpResult<T>;
-
-    fn sub(self, rhs: &NumOpResult<T>) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => lhs - *rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-impl<T> ops::Sub for &NumOpResult<T>
-where
-    T: ops::Sub<Output = NumOpResult<T>> + Copy,
-{
-    type Output = NumOpResult<T>;
-
-    fn sub(self, rhs: Self) -> Self::Output {
-        match (self, rhs) {
-            (R::Valid(lhs), R::Valid(rhs)) => *lhs - *rhs,
-            (_, _) => R::Error(NumOpError {}),
-        }
-    }
-}
-
 impl core::iter::Sum<NumOpResult<Amount>> for NumOpResult<Amount> {
     fn sum<I>(iter: I) -> Self
     where

From a44a9d31f6ee728e8325e380e1c5500e00227a55 Mon Sep 17 00:00:00 2001
From: Andrew Poelstra <apoelstra@wpsoftware.net>
Date: Tue, 25 Feb 2025 20:45:56 +0000
Subject: [PATCH 8/9] Add a few impls to the result macro

Add a few missing impls to the `impl_op_for_references` macro.

Includes a minor whitespace change so that traits are grouped together.
---
 units/src/amount/result.rs | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/units/src/amount/result.rs b/units/src/amount/result.rs
index b2091a1b5..724230833 100644
--- a/units/src/amount/result.rs
+++ b/units/src/amount/result.rs
@@ -129,7 +129,6 @@ crate::internal_macros::impl_op_for_references! {
 
         fn add(self, rhs: Amount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
     }
-
     impl ops::Add<NumOpResult<Amount>> for Amount {
         type Output = NumOpResult<Amount>;
 
@@ -141,7 +140,6 @@ crate::internal_macros::impl_op_for_references! {
 
         fn sub(self, rhs: Amount) -> Self::Output { self.checked_sub(rhs).valid_or_error() }
     }
-
     impl ops::Sub<NumOpResult<Amount>> for Amount {
         type Output = NumOpResult<Amount>;
 
@@ -158,23 +156,39 @@ crate::internal_macros::impl_op_for_references! {
 
         fn mul(self, rhs: u64) -> Self::Output { self.checked_mul(rhs).valid_or_error() }
     }
+    impl ops::Mul<u64> for NumOpResult<Amount> {
+        type Output = NumOpResult<Amount>;
+
+        fn mul(self, rhs: u64) -> Self::Output { self.and_then(|lhs| lhs * rhs) }
+    }
+
     impl ops::Div<u64> for Amount {
         type Output = NumOpResult<Amount>;
 
         fn div(self, rhs: u64) -> Self::Output { self.checked_div(rhs).valid_or_error() }
     }
+    impl ops::Div<u64> for NumOpResult<Amount> {
+        type Output = NumOpResult<Amount>;
+
+        fn div(self, rhs: u64) -> Self::Output { self.and_then(|lhs| lhs / rhs) }
+    }
+
     impl ops::Rem<u64> for Amount {
         type Output = NumOpResult<Amount>;
 
         fn rem(self, modulus: u64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
     }
+    impl ops::Rem<u64> for NumOpResult<Amount> {
+        type Output = NumOpResult<Amount>;
+
+        fn rem(self, modulus: u64) -> Self::Output { self.and_then(|lhs| lhs % modulus) }
+    }
 
     impl ops::Add<SignedAmount> for SignedAmount {
         type Output = NumOpResult<SignedAmount>;
 
         fn add(self, rhs: SignedAmount) -> Self::Output { self.checked_add(rhs).valid_or_error() }
     }
-
     impl ops::Add<NumOpResult<SignedAmount>> for SignedAmount {
         type Output = NumOpResult<SignedAmount>;
 
@@ -202,16 +216,33 @@ crate::internal_macros::impl_op_for_references! {
 
         fn mul(self, rhs: i64) -> Self::Output { self.checked_mul(rhs).valid_or_error() }
     }
+    impl ops::Mul<i64> for NumOpResult<SignedAmount> {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn mul(self, rhs: i64) -> Self::Output { self.and_then(|lhs| lhs * rhs) }
+    }
+
     impl ops::Div<i64> for SignedAmount {
         type Output = NumOpResult<SignedAmount>;
 
         fn div(self, rhs: i64) -> Self::Output { self.checked_div(rhs).valid_or_error() }
     }
+    impl ops::Div<i64> for NumOpResult<SignedAmount> {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn div(self, rhs: i64) -> Self::Output { self.and_then(|lhs| lhs / rhs) }
+    }
+
     impl ops::Rem<i64> for SignedAmount {
         type Output = NumOpResult<SignedAmount>;
 
         fn rem(self, modulus: i64) -> Self::Output { self.checked_rem(modulus).valid_or_error() }
     }
+    impl ops::Rem<i64> for NumOpResult<SignedAmount> {
+        type Output = NumOpResult<SignedAmount>;
+
+        fn rem(self, modulus: i64) -> Self::Output { self.and_then(|lhs| lhs % modulus) }
+    }
 
     impl<T> ops::Add<NumOpResult<T>> for NumOpResult<T>
     where

From 814685e551f00a896448b46b195f7dde69c2d32b Mon Sep 17 00:00:00 2001
From: "Tobin C. Harding" <me@tobin.cc>
Date: Wed, 19 Feb 2025 15:31:32 +1100
Subject: [PATCH 9/9] Macroise the NumOpResult tests

Macroise the tests improving coverage along the way.

Includes a TODO to remind us to do `Neg` more fully.
---
 units/src/amount/tests.rs | 231 +++++++++++++++-----------------------
 1 file changed, 90 insertions(+), 141 deletions(-)

diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs
index fa2db4874..d3463660f 100644
--- a/units/src/amount/tests.rs
+++ b/units/src/amount/tests.rs
@@ -1196,170 +1196,122 @@ fn check_const() {
     assert_eq!(Amount::MAX_MONEY.to_sat() as i64, SignedAmount::MAX_MONEY.to_sat());
 }
 
-// Verify we have implemented all combinations of ops for `Amount` and `SignedAmount`.
-// It's easier to read this test that check the code.
+// Sanity check than stdlib supports the set of reference combinations for the ops we want.
 #[test]
 #[allow(clippy::op_ref)] // We are explicitly testing the references work with ops.
-fn amount_tyes_all_ops() {
-    // Sanity check than stdlib supports the set of reference combinations for the ops we want.
-    {
-        let x = 127;
+fn sanity_all_ops() {
+    let x = 127;
 
-        let _ = x + x;
-        let _ = &x + x;
-        let _ = x + &x;
-        let _ = &x + &x;
+    let _ = x + x;
+    let _ = &x + x;
+    let _ = x + &x;
+    let _ = &x + &x;
 
-        let _ = x - x;
-        let _ = &x - x;
-        let _ = x - &x;
-        let _ = &x - &x;
+    let _ = x - x;
+    let _ = &x - x;
+    let _ = x - &x;
+    let _ = &x - &x;
 
-        let _ = -x;
-    }
+    let _ = -x;
+}
 
+// Verify we have implemented all combinations of ops for the amount types and `NumOpResult` type.
+// It's easier to read this test than check the code.
+#[test]
+#[allow(clippy::op_ref)] // We are explicitly testing the references work with ops.
+fn num_op_result_ops() {
     let sat = Amount::from_sat(1);
     let ssat = SignedAmount::from_sat(1);
 
-    // Add
-    let _ = sat + sat;
-    let _ = &sat + sat;
-    let _ = sat + &sat;
-    let _ = &sat + &sat;
+    // Explicit type as sanity check.
+    let res: NumOpResult<Amount> = sat + sat;
+    let sres: NumOpResult<SignedAmount> = ssat + ssat;
 
-    // let _ = ssat + sat;
-    // let _ = &ssat + sat;
-    // let _ = ssat + &sat;
-    // let _ = &ssat + &sat;
+    macro_rules! check_op {
+        ($(let _ = $lhs:ident $op:tt $rhs:ident);* $(;)?) => {
+            $(
+                let _ = $lhs $op $rhs;
+                let _ = &$lhs $op $rhs;
+                let _ = $lhs $op &$rhs;
+                let _ = &$lhs $op &$rhs;
+            )*
+        }
+    }
 
-    // let _ = sat + ssat;
-    // let _ = &sat + ssat;
-    // let _ = sat + &ssat;
-    // let _ = &sat + &ssat;
+    // We do not currently support division involving `NumOpResult` and an amount type.
+    check_op! {
+        // Operations where RHS is the result of another operation.
+        let _ = sat + res;
+        let _ = sat - res;
+        // let _ = sat / res;
+        let _ = ssat + sres;
+        let _ = ssat - sres;
+        // let _ = ssat / sres;
 
-    let _ = ssat + ssat;
-    let _ = &ssat + ssat;
-    let _ = ssat + &ssat;
-    let _ = &ssat + &ssat;
+        // Operations where LHS is the result of another operation.
+        let _ = res + sat;
+        let _ = res - sat;
+        // let _ = res / sat;
+        let _ = sres + ssat;
+        let _ = sres - ssat;
+        // let _ = sres / ssat;
 
-    // Sub
-    let _ = sat - sat;
-    let _ = &sat - sat;
-    let _ = sat - &sat;
-    let _ = &sat - &sat;
-
-    // let _ = ssat - sat;
-    // let _ = &ssat - sat;
-    // let _ = ssat - &sat;
-    // let _ = &ssat - &sat;
-
-    // let _ = sat - ssat;
-    // let _ = &sat - ssat;
-    // let _ = sat - &ssat;
-    // let _ = &sat - &ssat;
-
-    let _ = ssat - ssat;
-    let _ = &ssat - ssat;
-    let _ = ssat - &ssat;
-    let _ = &ssat - &ssat;
-
-    // let _ = sat * sat;  // Intentionally not supported.
-
-    // Mul
-    let _ = sat * 3;
-    let _ = sat * &3;
-    let _ = &sat * 3;
-    let _ = &sat * &3;
-
-    let _ = ssat * 3_i64; // Explicit type for the benefit of the reader.
-    let _ = ssat * &3;
-    let _ = &ssat * 3;
-    let _ = &ssat * &3;
-
-    // Div
-    let _ = sat / 3;
-    let _ = &sat / 3;
-    let _ = sat / &3;
-    let _ = &sat / &3;
-
-    let _ = ssat / 3_i64; // Explicit type for the benefit of the reader.
-    let _ = &ssat / 3;
-    let _ = ssat / &3;
-    let _ = &ssat / &3;
-
-    // Rem
-    let _ = sat % 3;
-    let _ = &sat % 3;
-    let _ = sat % &3;
-    let _ = &sat % &3;
-
-    let _ = ssat % 3;
-    let _ = &ssat % 3;
-    let _ = ssat % &3;
-    let _ = &ssat % &3;
-
-    // FIXME: Do we want to support this?
-    // let _ = sat / sat;
-    //
-    // "How many times does this amount go into that amount?" - seems
-    // like a reasonable question to ask.
-
-    // FIXME: Do we want to support these?
-    // let _ = -sat;
-    // let _ = -ssat;
+        // Operations that where both sides are the result of another operation.
+        let _ = res + res;
+        let _ = res - res;
+        // let _ = res / res;
+        let _ = sres + sres;
+        let _ = sres - sres;
+        // let _ = sres / sres;
+    };
 }
 
-// FIXME: Should we support this sort of thing?
-// It will be a lot more code for possibly not that much benefit.
-#[test]
-fn can_ops_on_amount_and_signed_amount() {
-    // let res: NumOpResult<SignedAmount> = sat + ssat;
-}
-
-// Verify we have implemented all combinations of ops for the `NumOpResult` type.
-// It's easier to read this test that check the code.
+// Verify we have implemented all combinations of ops for the `NumOpResult` type and an integer.
+// It's easier to read this test than check the code.
 #[test]
 #[allow(clippy::op_ref)] // We are explicitly testing the references work with ops.
-fn amount_op_result_all_ops() {
+fn num_op_result_ops_integer() {
     let sat = Amount::from_sat(1);
-    // let ssat = SignedAmount::from_sat(1);
+    let ssat = SignedAmount::from_sat(1);
 
     // Explicit type as sanity check.
     let res: NumOpResult<Amount> = sat + sat;
-    // let sres: NumOpResult<SignedAmount> = ssat + ssat;
+    let sres: NumOpResult<SignedAmount> = ssat + ssat;
 
-    // Operations that where RHS is the result of another operation.
-    let _ = sat + res;
-    let _ = &sat + res;
-    // let _ = sat + &res;
-    // let _ = &sat + &res;
+    macro_rules! check_op {
+        ($(let _ = $lhs:ident $op:tt $rhs:literal);* $(;)?) => {
+            $(
+                let _ = $lhs $op $rhs;
+                let _ = &$lhs $op $rhs;
+                let _ = $lhs $op &$rhs;
+                let _ = &$lhs $op &$rhs;
+            )*
+        }
+    }
+    check_op! {
+        // Operations on a `NumOpResult` and integer.
+        let _ = res * 3_u64; // Explicit type for the benefit of the reader.
+        let _ = res / 3;
+        let _ = res % 3;
 
-    let _ = sat - res;
-    let _ = &sat - res;
-    // let _ = sat - &res;
-    // let _ = &sat - &res;
+        let _ = sres * 3_i64; // Explicit type for the benefit of the reader.
+        let _ = sres / 3;
+        let _ = sres % 3;
+    };
+}
 
-    // Operations that where LHS is the result of another operation.
-    let _ = res + sat;
-    // let _ = &res + sat;
-    let _ = res + &sat;
-    // let _ = &res + &sat;
+// Verify we have implemented all `Neg` for the amount types.
+#[test]
+fn amount_op_result_neg() {
+    // TODO: Implement Neg all round.
 
-    let _ = res - sat;
-    // let _ = &res - sat;
-    let _ = res - &sat;
-    // let _ = &res - &sat;
+    // let sat = Amount::from_sat(1);
+    let ssat = SignedAmount::from_sat(1);
 
-    // Operations that where both sides are the result of another operation.
-    let _ = res + res;
-    // let _ = &res + res;
-    // let _ = res + &res;
-    // let _ = &res + &res;
-
-    let _ = res - res;
-    // let _ = &res - res;
-    // let _ = res - &res;
-    // let _ = &res - &res;
+    // let _ = -sat;
+    let _ = -ssat;
+    // let _ = -res;
+    // let _ = -sres;
 }
 
 // Verify we have implemented all `Sum` for the `NumOpResult` type.
@@ -1373,7 +1325,4 @@ fn amount_op_result_sum() {
     let _ = amounts.iter().sum::<NumOpResult<Amount>>();
     let _ = amount_refs.iter().copied().sum::<NumOpResult<Amount>>();
     let _ = amount_refs.into_iter().sum::<NumOpResult<Amount>>();
-
-    // FIXME: Should we support this? I don't think so (Tobin).
-    // let _ = amount_refs.iter().sum::<NumOpResult<&Amount>>();
 }