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

Remove IBC protos in favour of the ibc-proto crate #459

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

romac
Copy link
Member

@romac romac commented Jan 30, 2024

Closes: #458
Closes: #442
Closes: #83

As promised, here is my follow-up to #458 and its corresponding issue in the ibc-proto-rs repository.

Here is the corresponding PR in ibc-proto which removes the generated Cosmos SDK protos and uses them via cosmos-sdk-proto instead: cosmos/ibc-proto-rs#187

Changes

  • Remove ibc-go submodule and the corresponding generated proto definitions
  • Update Cosmos SDK protos to v0.47.0
  • Re-export ICS 23 proto definitions from the ics23 crate instead of bundling them in cosmos-sdk-proto
  • Add no-std compatible ProtoJSON (de)serialization to all Protobuf definitions, via Informal's fork of pbjson.

TODO

Questions

  • Do we want to update the Cosmos SDK protos further, eg. to v0.50.x?
  • What's the best name for the feature guarding the ProtoJSON serialization code? Right now it's serde to match the corresponding feature in ibc-proto, but perhaps pbjson or protojson is a better name?

This PR is probably best reviewed commit by commit.

@tony-iqlusion This is by no mean the definitive way to go about deduplicating the work, so I am eager to hear what you think about this and to discuss further with you how best to proceeded.

prost = "0.12"
prost-types = "0.12"
prost-wkt-types = "0.5"
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this crate requires protoc?

error: failed to run custom build command for `prost-wkt-types v0.5.0`

Caused by:
  process didn't exit successfully: `/home/runner/work/cosmos-rust/cosmos-rust/target/release/build/prost-wkt-types-d2782fb67e78a862/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not find `protoc` installation and this build crate cannot proceed without
      this knowledge. If `protoc` is installed and this crate had trouble finding
      it, you can set the `PROTOC` environment variable with the specific path to your
      installed `protoc` binary.If you're on debian, try `apt-get install protobuf-compiler` or download it from https://github.com/protocolbuffers/protobuf/releases

As things currently stand, both tendermint-proto and cosmos-sdk-proto pregenerate all the protos so downstream users don't need protoc installed in order to be able to use the crates.

So changing this would make protoc a hard dependency where it wasn't before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah my bad, I should have realized that and asked if it was okay. I actually don't see a good reason for moving to prost-wkt-types right now so I will revert that change so that we can keep pre-generating all the protos.

Copy link
Member Author

@romac romac Mar 12, 2024

Choose a reason for hiding this comment

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

Oh now I remember: Because some Protobuf messages contain Any, Duration or Timestamp, we need those so-called "well-known" types to derive Serialize in order to derive Serialize for the enclosing message via pbjson. Unfortunately, prost-types does not provide such an impl.

Potential solutions I see:

  • a) Use prost-wkt-types instead of prost-types and incur the cost of downstream users having to install protoc
  • b) Use pbjson-types instead of prost-types
  • c) Switch between prost-types and pbjson-types at compile time based on whether the serde feature is enabled or not

Would one of those be acceptable or do you see another solution?

Copy link
Member Author

@romac romac Mar 12, 2024

Choose a reason for hiding this comment

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

  • d) Instruct prost-build to generate Rust definitions for the google.protobuf messages (ie. those "well-known types") instead of forwarding them to prost-types or another crate and let pbjson-build derive the serde impls for those.

Copy link
Member

Choose a reason for hiding this comment

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

Do any of those have an integration with the prost::Name trait like prost_types::Any::{from_msg, to_msg} do? We use that a lot in cosmrs: https://docs.rs/prost-types/latest/prost_types/struct.Any.html#impl-Any

Copy link
Member

Choose a reason for hiding this comment

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

I suppose if we generated our own Rust definitions for the google.protobuf messages, we could add from_msg/to_msg just like prost-types does.

Copy link
Member

Choose a reason for hiding this comment

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

@romac perhaps to break this PR up into more manageable chunks, you could PR just the change to generate our own google.protobuf.* protos which can be augmented with pbjson-build serializers? (and potentially other changes to proto-build as needed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ibc-go types adding support for json Abandon cosmos-sdk-proto in favor of ibc-proto?
2 participants