cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
181 stars 73 forks source link

Fix inconsistencies when de/serializing `CommitmentPrefix` #1229

Closed seanchen1991 closed 1 month ago

seanchen1991 commented 1 month ago

Feature Summary

Raised by @soareschen:

It looks like there are some inconsistencies between the JSON serialization and deserialization of ConnectionEnd in ibc-rs. I tried to query for the connection end from sov-ibc, and when try to parse the response using QueryConnectionResponse, I got the error:

Error: ParseError(Error("invalid type: string \"ibc\", expected struct CommitmentPrefix", line: 1, column: 139))

So it seems like the commitment_prefix field is serialized into string during serialization, but is expected to be wrapped inside a CommitmentPrefix wrapper field during JSON deserialization.

I think the culprit is probably here:


#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
pub struct CommitmentPrefix {
bytes: Vec<u8>,
}

[cfg(feature = "serde")]

impl serde::Serialize for CommitmentPrefix { fn serialize(&self, serializer: S) -> Result<S::Ok, S::Error> where S: serde::Serializer, { format!("{self:?}").serialize(serializer) } }


> The Deserialize instance is auto derived using serde, but the Serialize instance is manually implemented to serialize it as string

## Proposal

<!-- Provide a full description and requirements of the feature -->

Either auto-derive `serde::Serialize` as well for `CommitmentPrefix`, or manually deserialize it so that parsing is consistent.
soareschen commented 1 month ago

Note that the current manually derived Serialize instance for CommitmentPrefix is also incorrectly implemented. It uses the Debug instance of CommitmentPrefix, but the Debug implementation serializes a commitment prefix with invalid UTF-8 as "<not valid UTF8: {:?}>".

impl fmt::Debug for CommitmentPrefix {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let converted = core::str::from_utf8(self.as_bytes());
        match converted {
            Ok(s) => write!(f, "{s}"),
            Err(_e) => write!(f, "<not valid UTF8: {:?}>", self.as_bytes()),
        }
    }
}

For safe and correct serialization, we should revert the behavior and serialize it as bytes, not string.