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

Add a blanket Serialize, Deserialize impl to IBC domain types #7

Open
hdevalence opened this issue Apr 27, 2023 · 3 comments
Open

Add a blanket Serialize, Deserialize impl to IBC domain types #7

hdevalence opened this issue Apr 27, 2023 · 3 comments

Comments

@hdevalence
Copy link
Member

hdevalence commented Apr 27, 2023

The original ibc-rs code we forked from used #[derive(Serialize, Deserialize)] to derive serialization formats from the domain types. However, this is dangerous from an interoperability and protocol compatibility standpoint, because it means that:

  1. External, long-lived interfaces (serialization formats) are derived from internal implementation details, so that changing those implementations becomes a breaking change;
  2. Messages that already have defined serializations as part of the IBC specification now have a second, ad-hoc serialization only understood by Rust code -- which may happen to mostly align with the specified serialization, or may not, or may differ only in some subtle way.

To fix this, we need to ensure that:

  • All Serialize and Deserialize implementations for protobuf messages always serialize through the Proto-derived serialization format (by forwarding to the ibc-proto type, which is compiled with ProtoJSON support)
  • Any other Serialize and Deserialize implementations are audited and justified against some external format.

One way to do this could be to use a blanket impl on the ibc-types-domain-type DomainType crate, something that's not possible for the penumbra-proto DomainType (which has implementations on foreign types, some of which have Serialize impls already). This would ensure that any DomainType would be Serialize, and (by disallowing conflicting impls) that it was not serialized other than by conversion through a proto type.

The remaining Serialize/Deserialize impls would then be easily findable using ripgrep or some other search tool.

@erwanor erwanor moved this to Future in Testnets Apr 28, 2023
@plaidfinch plaidfinch moved this from Future to Next in Testnets May 5, 2023
@plaidfinch plaidfinch moved this from Next to Testnet 53: Himalia in Testnets May 12, 2023
@redshiftzero redshiftzero moved this from Testnet 53: Himalia to Future in Testnets May 26, 2023
@hdevalence
Copy link
Member Author

This is unblocked now that cosmos/ibc-proto-rs#95 landed.

@github-project-automation github-project-automation bot moved this from Future to Testnet 53: Himalia in Testnets May 31, 2023
@hdevalence hdevalence reopened this May 31, 2023
@redshiftzero redshiftzero moved this from Testnet 53: Himalia to Next in Testnets Jun 1, 2023
@conorsch
Copy link
Contributor

Much of this work has been completed via recent PR into ibc-types. Still a few more to go before we can close.

@conorsch conorsch moved this from Next (Steal from here) to In Progress (Already claimed) in Testnets Aug 7, 2023
@conorsch conorsch moved this from In Progress (Already claimed) to Next (Steal from here) in Testnets Aug 7, 2023
@hdevalence hdevalence changed the title Restore Serialize impls for all IBC domain types Add a blanket Serialize, Deserialize impl to IBC domain types Oct 3, 2023
@aubrika aubrika moved this from Next to Testnet 64: Titan in Testnets Oct 18, 2023
@aubrika aubrika moved this from Testnet 64: Titan to Next in Testnets Oct 19, 2023
@erwanor
Copy link
Member

erwanor commented Nov 29, 2023

This is at least partially done. In #69 and #71, we went through all (?) domain types and made sure that the serde derives forward to pbjson. What remains is removing the rest of the Serialize and Deserialize implementations in a second push. This will be done in concert with @grod220.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Next
Development

No branches or pull requests

3 participants