From 911f8cbd6a73199754becf17993a718f953d7f22 Mon Sep 17 00:00:00 2001
From: "Tobin C. Harding" <me@tobin.cc>
Date: Wed, 13 Mar 2024 12:59:53 +1100
Subject: [PATCH] fix: Manually implement serde traits

Currently we are deriving the serde traits for the `absolute::{Height,
Time}` types, this is incorrect because we maintain an invariant on
the inner `u32` of both types that it is above or below the threshold.

Manually implement the serde traits and pass the deserialized `u32` to
`from_consensus` to maintain the invariant.

Close: #2559
---
 units/src/lib.rs               |  4 +++
 units/src/locktime/absolute.rs | 61 +++++++++++++++++++++++++++++++---
 units/src/test_macros.rs       | 16 +++++++++
 3 files changed, 76 insertions(+), 5 deletions(-)
 create mode 100644 units/src/test_macros.rs

diff --git a/units/src/lib.rs b/units/src/lib.rs
index 3b42f1c2..1aeea076 100644
--- a/units/src/lib.rs
+++ b/units/src/lib.rs
@@ -33,6 +33,10 @@ 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 locktime;
diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs
index 0930f3cd..a85e4571 100644
--- a/units/src/locktime/absolute.rs
+++ b/units/src/locktime/absolute.rs
@@ -4,9 +4,6 @@
 
 use core::fmt;
 
-#[cfg(feature = "serde")]
-use serde::{Deserialize, Serialize};
-
 use internals::write_err;
 
 #[cfg(feature = "alloc")]
@@ -28,7 +25,6 @@ pub const LOCK_TIME_THRESHOLD: u32 = 500_000_000;
 
 /// An absolute block height, guaranteed to always contain a valid height value.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct Height(u32);
 
 impl Height {
@@ -100,13 +96,33 @@ impl From<ParseError> for ParseHeightError {
     fn from(value: ParseError) -> Self { Self(value) }
 }
 
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for Height {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let u = u32::deserialize(deserializer)?;
+        Ok(Height::from_consensus(u).map_err(serde::de::Error::custom)?)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl serde::Serialize for Height {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        self.to_consensus_u32().serialize(serializer)
+    }
+}
+
 /// A UNIX timestamp, seconds since epoch, guaranteed to always contain a valid time value.
 ///
 /// Note that there is no manipulation of the inner value during construction or when using
 /// `to_consensus_u32()`. Said another way, `Time(x)` means 'x seconds since epoch' _not_ '(x -
 /// threshold) seconds since epoch'.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct Time(u32);
 
 impl Time {
@@ -155,6 +171,27 @@ impl fmt::Display for Time {
 
 crate::impl_parse_str!(Time, ParseTimeError, parser(Time::from_consensus));
 
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for Time {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let u = u32::deserialize(deserializer)?;
+        Ok(Time::from_consensus(u).map_err(serde::de::Error::custom)?)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl serde::Serialize for Time {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        self.to_consensus_u32().serialize(serializer)
+    }
+}
+
 /// Error returned when parsing block time fails.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct ParseTimeError(ParseError);
@@ -364,4 +401,18 @@ mod tests {
         assert!(result.is_err());
     }
 
+    #[test]
+    #[cfg(feature = "serde")]
+    pub fn encode_decode_height() {
+        serde_round_trip!(Height::ZERO);
+        serde_round_trip!(Height::MIN);
+        serde_round_trip!(Height::MAX);
+    }
+
+    #[test]
+    #[cfg(feature = "serde")]
+    pub fn encode_decode_time() {
+        serde_round_trip!(Time::MIN);
+        serde_round_trip!(Time::MAX);
+    }
 }
diff --git a/units/src/test_macros.rs b/units/src/test_macros.rs
new file mode 100644
index 00000000..bc2d05e2
--- /dev/null
+++ b/units/src/test_macros.rs
@@ -0,0 +1,16 @@
+// 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);
+    })
+);