Repository: rust-bitcoin/rust-bitcoin
Pull request #3679 (2024-11-28 05:41:25)

Support serde serializing Amount as string
Sometimes JSON parsers may munge floats. Instead of using `f64` we can serialize BTC amounts as strings.

Close: #894

apoelstra: CI failure looks real.
tcharding: I don't know what is going on here, I can't repro locally. This build for me both in the repo root and in `units/`. ` cargo test --no-default-features --features=serde,arbitrary,std`
apoelstra: My local CI fails with `alloc`,`arbitrary`,`default`,`serde`,`std`. With Rust 1.83.
jamillambert: > My local CI fails with `alloc`,`arbitrary`,`default`,`serde`,`std`. With Rust 1.83.

@apoelstra Just out of interest are you running the CI workflow in `.github/workflows/` locally with for example act? Or do you have separate scripts?

I also have the issue that I can't reproduce it locally and had to run the CI workflow in github by changing the yml file to allow a manual trigger.  I also didn't find a way to fix the CI fail.
yancyribbens: > @apoelstra Just out of interest are you running the CI workflow in .github/workflows/ locally with for example act? Or do you have separate scripts?

@jamillambert did you try: maintainer-tools/ci/run_task.sh

apoelstra: I have completely separate scripts. I don't even use cargo. I use `buildRustCrate` from nixpkgs and crate2nix.
tcharding: Trust me @jamillambert you don't even want to know what all that shit is. You've heard of those horse hair shirts medieval Christians used to wear as penance? Well nix is like that for computer nerds.
tcharding: I am still unable to repro this locally. I added a few unit tests and `cargo test --no-default-features --features=serde,alloc can_serde_as_` passes all of them.
apoelstra: In the `units/` directory I can reproduce with `cargo --locked build --no-default-features '--features=std serde arbitrary'`

Your `features` invocation is wrong -- they should be space-separated not comma-separated.
jamillambert: > In the `units/` directory I can reproduce with `cargo --locked build --no-default-features '--features=std serde arbitrary'`

Interestingly if you run `just build` everything passes.  But if you run the command you gave, or `cargo build --all-features` in `units/` it gives the error ``Deserialize<'_>` is not implemented for `String` ``

i.e. 
`Cargo build --workspace --all-targets --all-features` passes
`Cargo build --all-features` fails

Which suggests there is some issue with dependencies
jamillambert: > I think the issue is that we are using `serde` with no default features. When we enable our own `alloc` feature we should also enable `serde/alloc`.

Yep that seems to fix it.  It all passes for me if I add that to `Cargo.toml`
tcharding: > Your features invocation is wrong -- they should be space-separated not comma-separated.

Unless I've gone mad; commas are an alternative to spaces and don't need the quote

`cargo build --features=foo,bar,baz` is identical to `cargo build --features='foo bar baz'

Thanks for the debugging help

> `cargo build --workspace --all-targets --all-features` passes
> `cargo build --all-features` fails

@jamillambert the first invocation builds test code as well where as the second one doesn't. `--workspace` is needed if running `cargo` from the crate root but in my experience it has some quirks. (see `Target` section of `cargo --help`.) 

Patch:
diff --git a/units/Cargo.toml b/units/Cargo.toml
index 61acb661..682307dd 100644
--- a/units/Cargo.toml
+++ b/units/Cargo.toml
@@ -15,7 +15,7 @@ exclude = ["tests", "contrib"]
 [features]
 default = ["std"]
 std = ["alloc", "internals/std"]
-alloc = ["internals/alloc"]
+alloc = ["internals/alloc","serde?/alloc"]
 
 [dependencies]
 internals = { package = "bitcoin-internals", version = "0.4.0" }
diff --git a/units/src/amount/serde.rs b/units/src/amount/serde.rs
index a485994b..2caa8c74 100644
--- a/units/src/amount/serde.rs
+++ b/units/src/amount/serde.rs
@@ -20,13 +20,16 @@
 //! }
 //! ```
 
+#[cfg(feature = "alloc")]
 use core::fmt;
 
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
 #[cfg(feature = "alloc")] // This is because `to_float_in` uses `to_string`.
 use super::Denomination;
-use super::{Amount, ParseAmountError, SignedAmount};
+#[cfg(feature = "alloc")]
+use super::ParseAmountError;
+use super::{Amount, SignedAmount};
 
 /// This trait is used only to avoid code duplication and naming collisions
 /// of the different serde serialization crates.
@@ -37,6 +40,10 @@ pub trait SerdeAmount: Copy + Sized {
     fn ser_btc<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error>;
     #[cfg(feature = "alloc")]
     fn des_btc<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::Error>;
+    #[cfg(feature = "alloc")]
+    fn ser_str<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error>;
+    #[cfg(feature = "alloc")]
+    fn des_str<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::Error>;
 }
 
 mod private {
@@ -50,8 +57,11 @@ pub trait SerdeAmountForOpt: Copy + Sized + SerdeAmount {
     fn ser_sat_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error>;
     #[cfg(feature = "alloc")]
     fn ser_btc_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error>;
+    #[cfg(feature = "alloc")]
+    fn ser_str_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error>;
 }
 
+#[cfg(feature = "alloc")]
 struct DisplayFullError(ParseAmountError);
 
 #[cfg(feature = "std")]
@@ -70,6 +80,7 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 }
 
 #[cfg(not(feature = "std"))]
+#[cfg(feature = "alloc")]
 impl fmt::Display for DisplayFullError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) }
 }
@@ -90,6 +101,18 @@ fn des_btc<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::
         use serde::de::Error;
         Amount::from_btc(f64::deserialize(d)?).map_err(DisplayFullError).map_err(D::Error::custom)
     }
+    #[cfg(feature = "alloc")]
+    fn ser_str<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
+        s.serialize_str(&self.to_string_in(Denomination::Bitcoin))
+    }
+    #[cfg(feature = "alloc")]
+    fn des_str<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::Error> {
+        use serde::de::Error;
+        let s: alloc::string::String = Deserialize::deserialize(d)?;
+        Amount::from_str_in(&s, Denomination::Bitcoin)
+            .map_err(DisplayFullError)
+            .map_err(D::Error::custom)
+    }
 }
 
 impl SerdeAmountForOpt for Amount {
@@ -101,6 +124,10 @@ fn ser_sat_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S:
     fn ser_btc_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
         s.serialize_some(&self.to_btc())
     }
+    #[cfg(feature = "alloc")]
+    fn ser_str_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
+        s.serialize_some(&self.to_string_in(Denomination::Bitcoin))
+    }
 }
 
 impl SerdeAmount for SignedAmount {
@@ -121,6 +148,18 @@ fn des_btc<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::
             .map_err(DisplayFullError)
             .map_err(D::Error::custom)
     }
+    #[cfg(feature = "alloc")]
+    fn ser_str<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
+        s.serialize_str(self.to_string_in(Denomination::Bitcoin).as_str())
+    }
+    #[cfg(feature = "alloc")]
+    fn des_str<'d, D: Deserializer<'d>>(d: D, _: private::Token) -> Result<Self, D::Error> {
+        use serde::de::Error;
+        let s: alloc::string::String = Deserialize::deserialize(d)?;
+        SignedAmount::from_str_in(&s, Denomination::Bitcoin)
+            .map_err(DisplayFullError)
+            .map_err(D::Error::custom)
+    }
 }
 
 impl SerdeAmountForOpt for SignedAmount {
@@ -132,6 +171,10 @@ fn ser_sat_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S:
     fn ser_btc_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
         s.serialize_some(&self.to_btc())
     }
+    #[cfg(feature = "alloc")]
+    fn ser_str_opt<S: Serializer>(self, s: S, _: private::Token) -> Result<S::Ok, S::Error> {
+        s.serialize_some(&self.to_string_in(Denomination::Bitcoin))
+    }
 }
 
 pub mod as_sat {
@@ -272,3 +315,140 @@ fn visit_some<D>(self, d: D) -> Result<Self::Value, D::Error>
         }
     }
 }
+
+#[cfg(feature = "alloc")]
+pub mod as_str {
+    //! Serialize and deserialize [`Amount`](crate::Amount) as a JSON string denominated in BTC.
+    //! Use with `#[serde(with = "amount::serde::as_str")]`.
+
+    use serde::{Deserializer, Serializer};
+
+    use super::private;
+    use crate::amount::serde::SerdeAmount;
+
+    pub fn serialize<A: SerdeAmount, S: Serializer>(a: &A, s: S) -> Result<S::Ok, S::Error> {
+        a.ser_str(s, private::Token)
+    }
+
+    pub fn deserialize<'d, A: SerdeAmount, D: Deserializer<'d>>(d: D) -> Result<A, D::Error> {
+        A::des_str(d, private::Token)
+    }
+
+    pub mod opt {
+        //! Serialize and deserialize `Option<Amount>` as a JSON string denominated in BTC.
+        //! Use with `#[serde(default, with = "amount::serde::as_str::opt")]`.
+
+        use core::fmt;
+        use core::marker::PhantomData;
+
+        use serde::{de, Deserializer, Serializer};
+
+        use super::private;
+        use crate::amount::serde::SerdeAmountForOpt;
+
+        pub fn serialize<A: SerdeAmountForOpt, S: Serializer>(
+            a: &Option<A>,
+            s: S,
+        ) -> Result<S::Ok, S::Error> {
+            match *a {
+                Some(a) => a.ser_str_opt(s, private::Token),
+                None => s.serialize_none(),
+            }
+        }
+
+        pub fn deserialize<'d, A: SerdeAmountForOpt, D: Deserializer<'d>>(
+            d: D,
+        ) -> Result<Option<A>, D::Error> {
+            struct VisitOptAmt<X>(PhantomData<X>);
+
+            impl<'de, X: SerdeAmountForOpt> de::Visitor<'de> for VisitOptAmt<X> {
+                type Value = Option<X>;
+
+                fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+                    write!(formatter, "An Option<String>")
+                }
+
+                fn visit_none<E>(self) -> Result<Self::Value, E>
+                where
+                    E: de::Error,
+                {
+                    Ok(None)
+                }
+                fn visit_some<D>(self, d: D) -> Result<Self::Value, D::Error>
+                where
+                    D: Deserializer<'de>,
+                {
+                    Ok(Some(X::des_str(d, private::Token)?))
+                }
+            }
+            d.deserialize_option(VisitOptAmt::<A>(PhantomData))
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn can_serde_as_sat() {
+        #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
+        pub struct HasAmount {
+            #[serde(with = "crate::amount::serde::as_sat")]
+            pub amount: Amount,
+        }
+
+        let orig = HasAmount {
+            amount: Amount::ONE_BTC,
+        };
+
+        let json = serde_json::to_string(&orig).expect("failed to ser");
+        let want = "{\"amount\":100000000}";
+        assert_eq!(json, want);
+
+        let rinsed: HasAmount = serde_json::from_str(&json).expect("failed to deser");
+        assert_eq!(rinsed, orig)
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn can_serde_as_btc() {
+        #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
+        pub struct HasAmount {
+            #[serde(with = "crate::amount::serde::as_btc")]
+            pub amount: Amount,
+        }
+
+        let orig = HasAmount {
+            amount: Amount::ONE_BTC,
+        };
+
+        let json = serde_json::to_string(&orig).expect("failed to ser");
+        let want = "{\"amount\":1.0}";
+        assert_eq!(json, want);
+
+        let rinsed: HasAmount = serde_json::from_str(&json).expect("failed to deser");
+        assert_eq!(rinsed, orig)
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn can_serde_as_str() {
+        #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
+        pub struct HasAmount {
+            #[serde(with = "crate::amount::serde::as_str")]
+            pub amount: Amount,
+        }
+
+        let orig = HasAmount {
+            amount: Amount::ONE_BTC,
+        };
+
+        let json = serde_json::to_string(&orig).expect("failed to ser");
+        let want = "{\"amount\":\"1\"}";
+        assert_eq!(json, want);
+
+        let rinsed: HasAmount = serde_json::from_str(&json).expect("failed to deser");
+        assert_eq!(rinsed, orig);
+    }
+}


Test patch:
diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs
index 0b22ea2f..1213e888 100644
--- a/units/src/amount/tests.rs
+++ b/units/src/amount/tests.rs
@@ -741,6 +741,31 @@ struct T {
     assert!(t.unwrap_err().to_string().contains(&OutOfRangeError::negative().to_string()));
 }
 
+#[cfg(feature = "serde")]
+#[cfg(feature = "alloc")]
+#[test]
+fn serde_as_str() {
+    #[derive(Serialize, Deserialize, PartialEq, Debug)]
+    struct T {
+        #[serde(with = "crate::amount::serde::as_str")]
+        pub amt: Amount,
+        #[serde(with = "crate::amount::serde::as_str")]
+        pub samt: SignedAmount,
+    }
+
+    serde_test::assert_tokens(
+        &T { amt: Amount::from_sat(123456789), samt: SignedAmount::from_sat(-123456789) },
+        &[
+            serde_test::Token::Struct { name: "T", len: 2 },
+            serde_test::Token::String("amt"),
+            serde_test::Token::String("1.23456789"),
+            serde_test::Token::String("samt"),
+            serde_test::Token::String("-1.23456789"),
+            serde_test::Token::StructEnd,
+        ],
+    );
+}
+
 #[cfg(feature = "serde")]
 #[cfg(feature = "alloc")]
 #[test]
@@ -825,6 +850,49 @@ struct T {
     assert_eq!(without, serde_json::from_value(value_without).unwrap());
 }
 
+#[cfg(feature = "serde")]
+#[cfg(feature = "alloc")]
+#[test]
+#[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin.
+fn serde_as_str_opt() {
+    use serde_json;
+
+    #[derive(Serialize, Deserialize, PartialEq, Debug, Eq)]
+    struct T {
+        #[serde(default, with = "crate::amount::serde::as_str::opt")]
+        pub amt: Option<Amount>,
+        #[serde(default, with = "crate::amount::serde::as_str::opt")]
+        pub samt: Option<SignedAmount>,
+    }
+
+    let with = T {
+        amt: Some(Amount::from_sat(123456789)),
+        samt: Some(SignedAmount::from_sat(-123456789)),
+    };
+    let without = T { amt: None, samt: None };
+
+    // Test Roundtripping
+    for s in [&with, &without].iter() {
+        let v = serde_json::to_string(s).unwrap();
+        let w: T = serde_json::from_str(&v).unwrap();
+        assert_eq!(w, **s);
+    }
+
+    let t: T =
+        serde_json::from_str("{\"amt\": \"1.23456789\", \"samt\": \"-1.23456789\"}").unwrap();
+    assert_eq!(t, with);
+
+    let t: T = serde_json::from_str("{}").unwrap();
+    assert_eq!(t, without);
+
+    let value_with: serde_json::Value =
+        serde_json::from_str("{\"amt\": \"1.23456789\", \"samt\": \"-1.23456789\"}").unwrap();
+    assert_eq!(with, serde_json::from_value(value_with).unwrap());
+
+    let value_without: serde_json::Value = serde_json::from_str("{}").unwrap();
+    assert_eq!(without, serde_json::from_value(value_without).unwrap());
+}
+
 #[test]
 fn sum_amounts() {
     assert_eq!(Amount::from_sat(0), [].iter().sum::<Amount>());