Correct and optimize custom serializers for stringized values, hashes, nullable and optional fields

.changelog/unreleased/breaking-changes/1351-serializer-fixes.md

@@ -0,0 +1,16 @@
+- Changed the serde schema produced by `serialize` functions in these
+  helper modules ([\#1351](https://github.com/informalsystems/tendermint-
+  rs/pull/1351)):
+
+  * In `tendermint-proto`:
+    - `serializers::nullable`
+    - `serializers::optional`
+  * In `tendermint`:
+    - `serializers::apphash`
+    - `serializers::hash`
+    - `serializers::option_hash`
+
+  If `serde_json` is used for serialization, the output schema does not change.
+  But since serde is a generic framework, the changes may be breaking for
+  other users. Overall, these changes should make the serialized data
+  acceptable by the corresponding deserializer agnostically of the format.

.changelog/unreleased/improvements/1351-serializer-improvements.md

@@ -0,0 +1,3 @@
+- Corrected custom serializer helpers to consistently produce the format
+  accepted by the deserializer. Improved performance of deserializers.
+  ([\#1351](https://github.com/informalsystems/tendermint-rs/pull/1351))

proto/src/serializers/from_str.rs

@@ -1,6 +1,11 @@
-//! Serialize and deserialize any `T` that implements [[core::str::FromStr]]
-//! and [[core::fmt::Display]] from or into string. Note this can be used for
+//! Serialize and deserialize any `T` that implements [`FromStr`]
+//! and [`Display`] to convert from or into string. Note this can be used for
 //! all primitive data types.
+
+use alloc::borrow::Cow;
+use core::fmt::Display;
+use core::str::FromStr;
+
 use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
 
 use crate::prelude::*;
@@ -9,19 +14,50 @@ use crate::prelude::*;
 pub fn deserialize<'de, D, T>(deserializer: D) -> Result<T, D::Error>
 where
     D: Deserializer<'de>,
-    T: core::str::FromStr,
-    <T as core::str::FromStr>::Err: core::fmt::Display,
+    T: FromStr,
+    <T as FromStr>::Err: Display,
 {
-    String::deserialize(deserializer)?
+    <Cow<'_, str>>::deserialize(deserializer)?
         .parse::<T>()
-        .map_err(|e| D::Error::custom(format!("{e}")))
+        .map_err(D::Error::custom)
 }
 
 /// Serialize from T into string
 pub fn serialize<S, T>(value: &T, serializer: S) -> Result<S::Ok, S::Error>
 where
     S: Serializer,
-    T: core::fmt::Display,
+    T: Display,
 {
     value.to_string().serialize(serializer)
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::prelude::*;
+    use core::convert::Infallible;
+    use core::str::FromStr;
+    use serde::Deserialize;
+
+    struct ParsedStr(String);
+
+    impl FromStr for ParsedStr {
+        type Err = Infallible;
+
+        fn from_str(s: &str) -> Result<Self, Self::Err> {
+            Ok(Self(s.to_owned()))
+        }
+    }
+
+    #[derive(Deserialize)]
+    struct Foo {
+        #[serde(with = "super")]
+        msg: ParsedStr,
+    }
+
+    #[test]
+    fn can_deserialize_owned() {
+        const TEST_JSON: &str = r#"{ "msg": "\"Hello\"" }"#;
+        let v = serde_json::from_str::<Foo>(TEST_JSON).unwrap();
+        assert_eq!(v.msg.0, "\"Hello\"");
+    }
+}

proto/src/serializers/nullable.rs

@@ -17,7 +17,8 @@ where
     T: Default + PartialEq + Serialize,
 {
     if value == &T::default() {
-        return serializer.serialize_none();
+        serializer.serialize_none()
+    } else {
+        serializer.serialize_some(value)
     }
-    value.serialize(serializer)
 }

proto/src/serializers/optional.rs

@@ -1,6 +1,9 @@
 //! Serialize/deserialize `Option<T>` type where `T` has a serializer/deserializer.
-//! Use `null` if the received value equals the `Default` implementation.
-// Todo: Rename this serializer to something like "default_eq_none" to mirror its purpose better.
+//! Deserialize to `None` if the received value equals the `Default` value.
+//! Serialize `None` as `Some` with the `Default` value for `T`.
+
+// TODO: Rename this serializer to something like "default_eq_none" to mirror its purpose better.
+
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
 /// Deserialize `Option<T>`
@@ -19,7 +22,7 @@ where
     T: Default + Serialize,
 {
     match value {
-        Some(v) => v.serialize(serializer),
-        None => T::default().serialize(serializer),
+        Some(v) => serializer.serialize_some(v),
+        None => serializer.serialize_some(&T::default()),
     }
 }

proto/src/serializers/optional_from_str.rs

@@ -1,5 +1,6 @@
 //! De/serialize an optional type that must be converted from/to a string.
 
+use alloc::borrow::Cow;
 use core::{fmt::Display, str::FromStr};
 
 use serde::{de::Error, Deserialize, Deserializer, Serializer};
@@ -23,11 +24,41 @@ where
     T: FromStr,
     T::Err: Display,
 {
-    let s = match Option::<String>::deserialize(deserializer)? {
+    let s = match Option::<Cow<'_, str>>::deserialize(deserializer)? {
         Some(s) => s,
         None => return Ok(None),
     };
-    Ok(Some(s.parse().map_err(|e: <T as FromStr>::Err| {
-        D::Error::custom(format!("{e}"))
-    })?))
+    Ok(Some(s.parse().map_err(D::Error::custom)?))
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::prelude::*;
+    use core::convert::Infallible;
+    use core::str::FromStr;
+    use serde::Deserialize;
+
+    #[derive(Debug, PartialEq)]
+    struct ParsedStr(String);
+
+    impl FromStr for ParsedStr {
+        type Err = Infallible;
+
+        fn from_str(s: &str) -> Result<Self, Self::Err> {
+            Ok(Self(s.to_owned()))
+        }
+    }
+
+    #[derive(Deserialize)]
+    struct Foo {
+        #[serde(with = "super")]
+        msg: Option<ParsedStr>,
+    }
+
+    #[test]
+    fn can_deserialize_owned() {
+        const TEST_JSON: &str = r#"{ "msg": "\"Hello\"" }"#;
+        let v = serde_json::from_str::<Foo>(TEST_JSON).unwrap();
+        assert_eq!(v.msg, Some(ParsedStr("\"Hello\"".into())));
+    }
 }

tendermint/src/serializers/apphash.rs

@@ -1,6 +1,8 @@
 //! AppHash serialization with validation
 
-use serde::{Deserialize, Deserializer, Serializer};
+use alloc::borrow::Cow;
+
+use serde::{de, ser, Deserialize, Deserializer, Serializer};
 use subtle_encoding::hex;
 
 use crate::{prelude::*, AppHash};
@@ -10,8 +12,8 @@ pub fn deserialize<'de, D>(deserializer: D) -> Result<AppHash, D::Error>
 where
     D: Deserializer<'de>,
 {
-    let hexstring: String = Option::<String>::deserialize(deserializer)?.unwrap_or_default();
-    AppHash::from_hex_upper(hexstring.as_str()).map_err(serde::de::Error::custom)
+    let hexstring = Option::<Cow<'_, str>>::deserialize(deserializer)?.unwrap_or(Cow::Borrowed(""));
+    AppHash::from_hex_upper(&hexstring).map_err(de::Error::custom)
 }
 
 /// Serialize from AppHash into hexstring
@@ -20,6 +22,7 @@ where
     S: Serializer,
 {
     let hex_bytes = hex::encode_upper(value.as_ref());
-    let hex_string = String::from_utf8(hex_bytes).map_err(serde::ser::Error::custom)?;
-    serializer.serialize_str(&hex_string)
+    let hex_string = String::from_utf8(hex_bytes).map_err(ser::Error::custom)?;
+    // Serialize as Option<String> for symmetry with deserialize
+    serializer.serialize_some(&hex_string)
 }

tendermint/src/serializers/apphash_base64.rs

@@ -1,6 +1,8 @@
 //! AppHash serialization with validation
 
-use serde::{Deserialize, Deserializer, Serializer};
+use alloc::borrow::Cow;
+
+use serde::{de, Deserialize, Deserializer, Serializer};
 use subtle_encoding::base64;
 
 use crate::{prelude::*, AppHash};
@@ -10,9 +12,11 @@ pub fn deserialize<'de, D>(deserializer: D) -> Result<AppHash, D::Error>
 where
     D: Deserializer<'de>,
 {
-    let s = Option::<String>::deserialize(deserializer)?.unwrap_or_default();
-    let decoded = base64::decode(s).map_err(serde::de::Error::custom)?;
-    decoded.try_into().map_err(serde::de::Error::custom)
+    let decoded = match Option::<Cow<'_, str>>::deserialize(deserializer)? {
+        Some(s) => base64::decode(s.as_bytes()).map_err(de::Error::custom)?,
+        None => vec![],
+    };
+    decoded.try_into().map_err(de::Error::custom)
 }
 
 /// Serialize from [`AppHash`] into a base64-encoded string.

tendermint/src/serializers/hash.rs

@@ -1,6 +1,8 @@
 //! Hash serialization with validation
 
-use serde::{Deserialize, Deserializer, Serializer};
+use alloc::borrow::Cow;
+
+use serde::{de, ser, Deserialize, Deserializer, Serializer};
 use subtle_encoding::hex;
 
 use crate::{hash::Algorithm, prelude::*, Hash};
@@ -10,8 +12,8 @@ pub fn deserialize<'de, D>(deserializer: D) -> Result<Hash, D::Error>
 where
     D: Deserializer<'de>,
 {
-    let hexstring: String = Option::<String>::deserialize(deserializer)?.unwrap_or_default();
-    Hash::from_hex_upper(Algorithm::Sha256, hexstring.as_str()).map_err(serde::de::Error::custom)
+    let hexstring = Option::<Cow<'_, str>>::deserialize(deserializer)?.unwrap_or(Cow::Borrowed(""));
+    Hash::from_hex_upper(Algorithm::Sha256, &hexstring).map_err(de::Error::custom)
 }
 
 /// Serialize from Hash into hexstring
@@ -20,6 +22,7 @@ where
     S: Serializer,
 {
     let hex_bytes = hex::encode_upper(value.as_bytes());
-    let hex_string = String::from_utf8(hex_bytes).map_err(serde::ser::Error::custom)?;
-    serializer.serialize_str(&hex_string)
+    let hex_string = String::from_utf8(hex_bytes).map_err(ser::Error::custom)?;
+    // Serialize as Option<String> for symmetry with deserialize
+    serializer.serialize_some(&hex_string)
 }

tendermint/src/serializers/option_hash.rs

@@ -1,26 +1,49 @@
 //! `Option<Hash>` serialization with validation
 
-use serde::{Deserializer, Serializer};
+use alloc::borrow::Cow;
+
+use serde::{de, Deserialize, Deserializer, Serializer};
 
 use super::hash;
-use crate::Hash;
+use crate::{hash::Algorithm, Hash};
 
-/// Deserialize hexstring into `Option<Hash>`
+/// Deserialize a nullable hexstring into `Option<Hash>`.
+/// A null value is deserialized as `None`.
 pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<Hash>, D::Error>
 where
     D: Deserializer<'de>,
 {
-    hash::deserialize(deserializer).map(Some)
+    match Option::<Cow<'_, str>>::deserialize(deserializer)? {
+        Some(s) => Hash::from_hex_upper(Algorithm::Sha256, &s)
+            .map(Some)
+            .map_err(de::Error::custom),
+        None => Ok(None),
+    }
 }
 
-/// Serialize from `Option<Hash>` into hexstring
+/// Serialize from `Option<Hash>` into a nullable hexstring. None is serialized as null.
 pub fn serialize<S>(value: &Option<Hash>, serializer: S) -> Result<S::Ok, S::Error>
 where
     S: Serializer,
 {
     if value.is_none() {
         serializer.serialize_none()
     } else {
-        hash::serialize(&value.unwrap(), serializer)
+        // hash::serialize serializes as Option<String>, so this is consistent
+        // with the other branch.
+        hash::serialize(value.as_ref().unwrap(), serializer)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn none_round_trip() {
+        let v: Option<Hash> = None;
+        let json = serde_json::to_string(&v).unwrap();
+        let parsed: Option<Hash> = serde_json::from_str(&json).unwrap();
+        assert!(parsed.is_none());
     }
 }