Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Optional Amount Serialization #552

Merged
merged 2 commits into from Mar 15, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
163 changes: 150 additions & 13 deletions src/util/amount.rs
Expand Up @@ -875,13 +875,30 @@ pub mod serde {

/// This trait is used only to avoid code duplication and naming collisions
/// of the different serde serialization crates.
///
/// TODO: Add the private::Sealed bound in next breaking release
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope someone will remember 😆

pub trait SerdeAmount: Copy + Sized {
fn ser_sat<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error>;
fn des_sat<'d, D: Deserializer<'d>>(d: D) -> Result<Self, D::Error>;
fn ser_btc<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error>;
fn des_btc<'d, D: Deserializer<'d>>(d: D) -> Result<Self, D::Error>;
}

mod private {
/// add this as a trait bound to traits which consumers of this library
/// should not be able to implement.
pub trait Sealed {}
impl Sealed for super::Amount {}
impl Sealed for super::SignedAmount {}
}

/// This trait is only for internal Amount type serialization/deserialization
pub trait SerdeAmountForOpt: Copy + Sized + SerdeAmount + private::Sealed {
fn type_prefix() -> &'static str;
fn ser_sat_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error>;
fn ser_btc_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error>;
}

impl SerdeAmount for Amount {
fn ser_sat<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error> {
u64::serialize(&self.as_sat(), s)
Expand All @@ -898,6 +915,18 @@ pub mod serde {
}
}

impl SerdeAmountForOpt for Amount {
fn type_prefix() -> &'static str {
"u"
}
fn ser_sat_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error> {
s.serialize_some(&self.as_sat())
}
fn ser_btc_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error> {
s.serialize_some(&self.as_btc())
}
}

impl SerdeAmount for SignedAmount {
fn ser_sat<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error> {
i64::serialize(&self.as_sat(), s)
Expand All @@ -914,6 +943,18 @@ pub mod serde {
}
}

impl SerdeAmountForOpt for SignedAmount {
fn type_prefix() -> &'static str {
"i"
}
fn ser_sat_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error> {
s.serialize_some(&self.as_sat())
}
fn ser_btc_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error> {
s.serialize_some(&self.as_btc())
}
}

pub mod as_sat {
//! Serialize and deserialize [Amount] as real numbers denominated in satoshi.
//! Use with `#[serde(with = "amount::serde::as_sat")]`.
Expand All @@ -933,23 +974,46 @@ pub mod serde {
//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but a typo nearby

Suggested change
//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.
//! Serialize and deserialize [Option<Amount>] as real numbers denominated in satoshi.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better

Suggested change
//! Serialize and deserialize [Optoin<Amount>] as real numbers denominated in satoshi.
//! Serialize and deserialize `Option<`[`Amount`]`>` as real numbers denominated in satoshi.

//! Use with `#[serde(default, with = "amount::serde::as_sat::opt")]`.

use serde::{Deserializer, Serializer};
use util::amount::serde::SerdeAmount;
use serde::{Deserializer, Serializer, de};
use util::amount::serde::SerdeAmountForOpt;
use std::fmt;
use std::marker::PhantomData;

pub fn serialize<A: SerdeAmount, S: Serializer>(
pub fn serialize<A: SerdeAmountForOpt, S: Serializer>(
a: &Option<A>,
s: S,
) -> Result<S::Ok, S::Error> {
match *a {
Some(a) => a.ser_sat(s),
Some(a) => a.ser_sat_opt(s),
None => s.serialize_none(),
}
}

pub fn deserialize<'d, A: SerdeAmount, D: Deserializer<'d>>(
pub fn deserialize<'d, A: SerdeAmountForOpt, D: Deserializer<'d>>(
d: D,
) -> Result<Option<A>, D::Error> {
Ok(Some(A::des_sat(d)?))
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<{}64>", X::type_prefix())
}

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_sat(d)?))
}
}
d.deserialize_option(VisitOptAmt::<A>(PhantomData))
}
}
}
Expand All @@ -973,23 +1037,46 @@ pub mod serde {
//! Serialize and deserialize [Option<Amount>] as JSON numbers denominated in BTC.
//! Use with `#[serde(default, with = "amount::serde::as_btc::opt")]`.

use serde::{Deserializer, Serializer};
use util::amount::serde::SerdeAmount;
use serde::{Deserializer, Serializer, de};
use util::amount::serde::SerdeAmountForOpt;
use std::fmt;
use std::marker::PhantomData;

pub fn serialize<A: SerdeAmount, S: Serializer>(
pub fn serialize<A: SerdeAmountForOpt, S: Serializer>(
a: &Option<A>,
s: S,
) -> Result<S::Ok, S::Error> {
match *a {
Some(a) => a.ser_btc(s),
Some(a) => a.ser_btc_opt(s),
None => s.serialize_none(),
}
}

pub fn deserialize<'d, A: SerdeAmount, D: Deserializer<'d>>(
pub fn deserialize<'d, A: SerdeAmountForOpt, D: Deserializer<'d>>(
d: D,
) -> Result<Option<A>, D::Error> {
Ok(Some(A::des_btc(d)?))
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<f64>")
}

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_btc(d)?))
}
}
d.deserialize_option(VisitOptAmt::<A>(PhantomData))
}
}
}
Expand Down Expand Up @@ -1345,7 +1432,7 @@ mod tests {
fn serde_as_btc_opt() {
use serde_json;

#[derive(Serialize, Deserialize, PartialEq, Debug)]
#[derive(Serialize, Deserialize, PartialEq, Debug, Eq)]
struct T {
#[serde(default, with = "::util::amount::serde::as_btc::opt")]
pub amt: Option<Amount>,
Expand All @@ -1362,6 +1449,13 @@ mod tests {
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\": 2.5, \"samt\": -2.5}").unwrap();
assert_eq!(t, with);

Expand All @@ -1375,4 +1469,47 @@ mod tests {
let value_without: serde_json::Value = serde_json::from_str("{}").unwrap();
assert_eq!(without, serde_json::from_value(value_without).unwrap());
}

#[cfg(feature = "serde")]
#[test]
fn serde_as_sat_opt() {
use serde_json;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is serde_test in case you don't want to rely on JSON-specific semantics when testing your implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serde_test for example makes it quite easy to test Visitor implementations against various scenarios like owned vs borrowed data.

We are currently fixing some of this retroactively in rust-secp: rust-bitcoin/rust-secp256k1#270


#[derive(Serialize, Deserialize, PartialEq, Debug, Eq)]
struct T {
#[serde(default, with = "::util::amount::serde::as_sat::opt")]
pub amt: Option<Amount>,
#[serde(default, with = "::util::amount::serde::as_sat::opt")]
pub samt: Option<SignedAmount>,
}

let with = T {
amt: Some(Amount::from_sat(2__500_000_00)),
samt: Some(SignedAmount::from_sat(-2__500_000_00)),
};
Comment on lines +1486 to +1489
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary, but I would like to test +/-21_000_000__000_000_00 as an edge case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from IRC; follow up work to this is likely going to reduce max_value to 21e6, which will require changing some of these things.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a certain reason to reduce the max value to anything below the actual maximum representation? IIRC both Amount types have safe math traits implemented on them.

Just to day that things like "total received" etc could exceed 21M and can still be denominated in sats/btc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, currently the max value is above the representation as a json number so serialization / deserialization can fail

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

This number is still plenty big, 4.2 times 21 million * 100 million sats. If you would like to support more revenue than that -- which perhaps we should! -- we'd need to look into string-ized numbers.

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\": 250000000, \"samt\": -250000000}").unwrap();
JeremyRubin marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(t, with);

let t: T = serde_json::from_str("{}").unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we try from_str(r#{"amt": null, "samt": null}#) here as well, to make sure it's covered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could try that explicitly, but that is already tested in the Roundtrip test code I added :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for trying explicitly, but nit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah given outstanding acks maybe merge and add a test when these tests get fixed up with the max value fixes.

assert_eq!(t, without);

let value_with: serde_json::Value =
serde_json::from_str("{\"amt\": 250000000, \"samt\": -250000000}").unwrap();
assert_eq!(with, serde_json::from_value(value_with).unwrap());

let value_without: serde_json::Value = serde_json::from_str("{}").unwrap();
JeremyRubin marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(without, serde_json::from_value(value_without).unwrap());
JeremyRubin marked this conversation as resolved.
Show resolved Hide resolved
}
}