ibc-rs

ibc-rs copied to clipboard

Feature Summary

We introduced ToProto trait in #995. It did streamline a lot of code, but we can streamline even more.

Proposal

1. Extend the ToProto trait with try_from_any() method and replace the generic with an associated type.

trait ToProto {
    type ProtoType;
    type Error;

    fn type_url(&self) -> String;

    fn to_any(&self) -> Any;

    fn try_from_any(any: Any) -> Result<Self, Self::Error>;
}

By removing the generic P from ToProto, we can access the corresponding protobuf type of DomainType at <DomainType as ToProto>::ProtoType.

Side-note: we may want to rename ToProto to something like ProtoLike.

2. For a DomainType with a corresponding ProtoType, we/users only maintain the following code. Note the derive-macro ToProto(ProtoType) for DomainType.

#[derive(ToProto(ProtoType))]
struct DomainType {
    hello: String,
}

impl TryFrom<Self::ProtoType> for DomainType {
    type Error = Error;

    fn try_from(proto: Self::ProtoType) -> Result<Self, Self::Error> {
        Ok(Self { hello: proto.hello })
    }
}

impl Into<Self::ProtoType> for DomainType {
    fn into(self) -> Self::ProtoType {
        Self::ProtoType {
            hello: self.hello,
        }
    }
}

3. The derive-macro will generate the following code block for the above example.

impl Protobuf<ProtoType> for DomainType {}

impl ToProto for DomainType {
    type ProtoType = ProtoType;
    type Error = Error;

    fn type_url(&self) -> String {
        ProtoType::type_url()
    }

    fn to_any(self) -> Any {
        Any {
            type_url: ProtoType::type_url(),
            value: ProtoType::from(self).encode_to_vec(),
        }   
    }

    fn try_from_any(any: Any) -> Result<Self, Self::Error> {
        if &any.type_url == ProtoType::type_url() {
            ProtoType::decode(any.value.as_slice()).and_then(Self::try_from)
        } else {
            Err(DecodeError::new("Invalid type_url"))
        }
    }
}

impl Protobuf<Any> for DomainType {}

impl TryFrom<Any> for DomainType {
    type Error = Error;

    fn try_from(any: Any) -> Result<Self, Self::Error> {
        let proto = ProtoType::try_from_any(any)?;
        Self::try_from(proto)
    }
}

impl From<DomainType> for Any {
    fn from(domain: DomainType) -> Self {
        let proto = ProtoType::from(domain);
        proto.to_any()
    }
}

With this, we can simplify the implementation of most of the current domain types. For example, the following codes can be removed from the Tendermint client state domain type.

https://github.com/cosmos/ibc-rs/blob/4a0990f35e2a9338b7205445b9d21a044784104c/ibc-clients/ics07-tendermint/src/client_state.rs#L65

https://github.com/cosmos/ibc-rs/blob/4a0990f35e2a9338b7205445b9d21a044784104c/ibc-clients/ics07-tendermint/src/client_state.rs#L81-L95

This raises a question to me, do we really need to hardcode constants such as this,

https://github.com/cosmos/ibc-rs/blob/4a0990f35e2a9338b7205445b9d21a044784104c/ibc-clients/ics07-tendermint/types/src/client_state.rs#L27

as they can be retrieved by ToProto::type_url() even in current codebase. For example, the above type URL is present as

<TmClientState as ToProto<RawTmClientState>>::type_url()

If we want to make sure that these types of URLs are the ones we expect, we can add unit tests.

Actually, we don't need

impl Protobuf<Any> for DomainType {}

The compilation error source, after removing these

https://github.com/cosmos/ibc-rs/blob/4a0990f35e2a9338b7205445b9d21a044784104c/ibc-clients/ics07-tendermint/src/consensus_state.rs#L96

https://github.com/cosmos/ibc-rs/blob/4a0990f35e2a9338b7205445b9d21a044784104c/ibc-testkit/src/testapp/ibc/clients/mock/consensus_state.rs#L103

can be refactored as

use ibc::primitives::ToVec;
Any::from(self).to_vec()

While the usability with the associated type is better, we do have a use for the parameter on the Protobuf trait: a single domain type can be used to encode to/decode from different versions of protobuf-generated types. Currently it's useful in supporting the CometBFT protocol versions used by 0.34.x, 0.37.x, and 0.38.x nodes, but also in post-v1 future, some messages might evolve to have different versions, and those will be published side by side to .v1 in .v2 packages and so on. The intent is to not proliferate versioned domain types unless there is a clear need for that, instead the versioning complexity is represented at the cometbft-proto level.

It'd be good if we have a trait to enforce a validate method for these domain types - as we don't perform data validation for the protobuf types.

trait ProtoValidate {
  fn validate(&self) -> bool;
}

trait ToProto: ProtoValidate { ... }

I recently learned about associated consts for traits and structs. So ToProto trait may look like this

trait ToProto {
    type ProtoType;
    type Error;
    const TYPE_URL: &'static str;

    fn to_any(&self) -> Any;
    fn try_from_any(any: Any) -> Result<Self, Self::Error>;
}

So we can rewrite the Any conversion as following:

    fn to_any(self) -> Any {
        Any {
            type_url: Self::TYPE_URL.to_owned(),
            value: ProtoType::from(self).encode_to_vec(),
        }   
    }

    fn try_from_any(any: Any) -> Result<Self, Self::Error> {
        match any.type_url.as_str() {
            Self::TYPE_URL => ProtoType::decode(any.value.as_slice()).and_then(Self::try_from),
            _ => Err(DecodeError::new("Invalid type_url"))
        }
    }