From 7fa53440dc9c81d0e4b74eea21c9ca725a917ceb Mon Sep 17 00:00:00 2001
From: "Tobin C. Harding" <me@tobin.cc>
Date: Thu, 20 Jun 2024 14:08:06 +1000
Subject: [PATCH] Move serde_round_trip macro to internals

We currently duplicate the serde_round_trip macro in `units` and
`bitcoin`, this is unnecessary since it is a private test macro we can
just throw it in `internals`.

While we are at it lets improve the macro by testing a binary encoding
also, elect to use the `bincode` crate because we already have it in
our dependency graph.

Add `test-serde` feature to `internals` to feature gate the macro and
its usage (preventing the transient dependency on `bincode` and
`serde_json`).
---
 Cargo-minimal.lock                   |  2 ++
 Cargo-recent.lock                    |  2 ++
 bitcoin/Cargo.toml                   |  1 +
 bitcoin/src/bip32.rs                 |  2 ++
 bitcoin/src/blockdata/transaction.rs |  2 ++
 bitcoin/src/lib.rs                   |  3 ---
 bitcoin/src/test_macros.rs           | 16 ----------------
 internals/Cargo.toml                 |  6 ++++++
 internals/src/lib.rs                 |  7 +++++++
 internals/src/serde.rs               | 17 +++++++++++++++++
 units/Cargo.toml                     |  1 +
 units/src/lib.rs                     |  4 ----
 units/src/locktime/absolute.rs       |  2 ++
 units/src/test_macros.rs             | 16 ----------------
 14 files changed, 42 insertions(+), 39 deletions(-)
 delete mode 100644 bitcoin/src/test_macros.rs
 delete mode 100644 units/src/test_macros.rs

diff --git a/Cargo-minimal.lock b/Cargo-minimal.lock
index 49b2d61a5..5987430c5 100644
--- a/Cargo-minimal.lock
+++ b/Cargo-minimal.lock
@@ -86,7 +86,9 @@ dependencies = [
 name = "bitcoin-internals"
 version = "0.3.0"
 dependencies = [
+ "bincode",
  "serde",
+ "serde_json",
 ]
 
 [[package]]
diff --git a/Cargo-recent.lock b/Cargo-recent.lock
index 91c38c420..18bd0a6ad 100644
--- a/Cargo-recent.lock
+++ b/Cargo-recent.lock
@@ -85,7 +85,9 @@ dependencies = [
 name = "bitcoin-internals"
 version = "0.3.0"
 dependencies = [
+ "bincode",
  "serde",
+ "serde_json",
 ]
 
 [[package]]
diff --git a/bitcoin/Cargo.toml b/bitcoin/Cargo.toml
index 314829cda..9b53b8406 100644
--- a/bitcoin/Cargo.toml
+++ b/bitcoin/Cargo.toml
@@ -42,6 +42,7 @@ bitcoinconsensus = { version = "0.106.0+26", default-features = false, optional
 actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = [ "derive", "alloc" ], optional = true }
 
 [dev-dependencies]
+internals = { package = "bitcoin-internals", version = "0.3.0", features = ["test-serde"] }
 serde_json = "1.0.0"
 serde_test = "1.0.19"
 bincode = "1.3.1"
diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs
index 07033a05f..f30ea74ae 100644
--- a/bitcoin/src/bip32.rs
+++ b/bitcoin/src/bip32.rs
@@ -917,6 +917,8 @@ impl std::error::Error for InvalidBase58PayloadLengthError {}
 #[cfg(test)]
 mod tests {
     use hex::test_hex_unwrap as hex;
+    #[cfg(feature = "serde")]
+    use internals::serde_round_trip;
 
     use super::ChildNumber::{Hardened, Normal};
     use super::*;
diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs
index 0b4c93fae..7b716108a 100644
--- a/bitcoin/src/blockdata/transaction.rs
+++ b/bitcoin/src/blockdata/transaction.rs
@@ -1633,6 +1633,8 @@ mod tests {
     use core::str::FromStr;
 
     use hex::{test_hex_unwrap as hex, FromHex};
+    #[cfg(feature = "serde")]
+    use internals::serde_round_trip;
 
     use super::*;
     use crate::consensus::encode::{deserialize, serialize};
diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs
index eb2295551..6f2c74965 100644
--- a/bitcoin/src/lib.rs
+++ b/bitcoin/src/lib.rs
@@ -83,9 +83,6 @@ pub extern crate secp256k1;
 #[macro_use]
 extern crate actual_serde as serde;
 
-#[cfg(test)]
-#[macro_use]
-mod test_macros;
 mod internal_macros;
 #[cfg(feature = "serde")]
 mod serde_utils;
diff --git a/bitcoin/src/test_macros.rs b/bitcoin/src/test_macros.rs
deleted file mode 100644
index bc2d05e26..000000000
--- a/bitcoin/src/test_macros.rs
+++ /dev/null
@@ -1,16 +0,0 @@
-// SPDX-License-Identifier: CC0-1.0
-
-//! Bitcoin serde macros.
-//!
-//! This module provides internal macros used for unit tests.
-
-#[cfg(feature = "serde")]
-macro_rules! serde_round_trip (
-    ($var:expr) => ({
-        use serde_json;
-
-        let encoded = serde_json::to_value(&$var).unwrap();
-        let decoded = serde_json::from_value(encoded).unwrap();
-        assert_eq!($var, decoded);
-    })
-);
diff --git a/internals/Cargo.toml b/internals/Cargo.toml
index 20cdaadd8..83396806d 100644
--- a/internals/Cargo.toml
+++ b/internals/Cargo.toml
@@ -18,9 +18,15 @@ default = []
 std = ["alloc"]
 alloc = []
 
+test-serde = ["serde/derive", "serde_json", "bincode"]
+
 [dependencies]
 serde = { version = "1.0.103", default-features = false, optional = true }
 
+# Don't enable these directly, use `test-serde` feature instead.
+serde_json = { version = "1.0.68", optional = true }
+bincode = { version = "1.3.1", optional = true }
+
 [dev-dependencies]
 
 [package.metadata.docs.rs]
diff --git a/internals/src/lib.rs b/internals/src/lib.rs
index d86a1c3e8..9de80dfc7 100644
--- a/internals/src/lib.rs
+++ b/internals/src/lib.rs
@@ -21,10 +21,17 @@ extern crate alloc;
 #[cfg(feature = "std")]
 extern crate std;
 
+#[cfg(feature = "test-serde")]
+pub extern crate serde_json;
+
+#[cfg(feature = "test-serde")]
+pub extern crate bincode;
+
 pub mod array_vec;
 pub mod const_tools;
 pub mod error;
 pub mod macros;
 mod parse;
 #[cfg(feature = "serde")]
+#[macro_use]
 pub mod serde;
diff --git a/internals/src/serde.rs b/internals/src/serde.rs
index c7afa0d37..3e4e64656 100644
--- a/internals/src/serde.rs
+++ b/internals/src/serde.rs
@@ -293,3 +293,20 @@ macro_rules! serde_struct_human_string_impl {
         }
     )
 }
+
+/// Does round trip test to/from serde value.
+#[cfg(feature = "test-serde")]
+#[macro_export]
+macro_rules! serde_round_trip (
+    ($var:expr) => ({
+        use serde_json;
+
+        let encoded = $crate::serde_json::to_value(&$var).expect("serde_json failed to encode");
+        let decoded = $crate::serde_json::from_value(encoded).expect("serde_json failed to decode");
+        assert_eq!($var, decoded);
+
+        let encoded = $crate::bincode::serialize(&$var).expect("bincode failed to encode");
+        let decoded = $crate::bincode::deserialize(&encoded).expect("bincode failed to decode");
+        assert_eq!($var, decoded);
+    })
+);
diff --git a/units/Cargo.toml b/units/Cargo.toml
index 54143dcb3..02d2931ae 100644
--- a/units/Cargo.toml
+++ b/units/Cargo.toml
@@ -23,6 +23,7 @@ internals = { package = "bitcoin-internals", version = "0.3.0" }
 serde = { version = "1.0.103", default-features = false, features = ["derive"], optional = true }
 
 [dev-dependencies]
+internals = { package = "bitcoin-internals", version = "0.3.0", features = ["test-serde"] }
 serde_test = "1.0"
 serde_json = "1.0"
 
diff --git a/units/src/lib.rs b/units/src/lib.rs
index e3fbdf581..b533575af 100644
--- a/units/src/lib.rs
+++ b/units/src/lib.rs
@@ -31,10 +31,6 @@ extern crate std;
 #[cfg(feature = "serde")]
 pub extern crate serde;
 
-#[cfg(test)]
-#[macro_use]
-mod test_macros;
-
 pub mod amount;
 #[cfg(feature = "alloc")]
 pub mod block;
diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs
index 64a5094ce..10b5fac7d 100644
--- a/units/src/locktime/absolute.rs
+++ b/units/src/locktime/absolute.rs
@@ -373,6 +373,8 @@ impl From<ConversionError> for ParseError {
 #[cfg(test)]
 mod tests {
     use super::*;
+    #[cfg(feature = "serde")]
+    use internals::serde_round_trip;
 
     #[test]
     fn time_from_str_hex_happy_path() {
diff --git a/units/src/test_macros.rs b/units/src/test_macros.rs
deleted file mode 100644
index bc2d05e26..000000000
--- a/units/src/test_macros.rs
+++ /dev/null
@@ -1,16 +0,0 @@
-// SPDX-License-Identifier: CC0-1.0
-
-//! Bitcoin serde macros.
-//!
-//! This module provides internal macros used for unit tests.
-
-#[cfg(feature = "serde")]
-macro_rules! serde_round_trip (
-    ($var:expr) => ({
-        use serde_json;
-
-        let encoded = serde_json::to_value(&$var).unwrap();
-        let decoded = serde_json::from_value(encoded).unwrap();
-        assert_eq!($var, decoded);
-    })
-);