search

cosmos / ibc-proto-rs

Rust Protobuf definitions and gRPC clients for interacting with Cosmos SDK, IBC and Interchain Security
Apache License 2.0
47 stars 29 forks source link

Use Cosmos SDK protos via the `cosmos-sdk-proto` crate instead of bundling them in `ibc-proto` #187

Closed romac closed 3 weeks ago

tony-iqlusion commented 1 month ago

Now that cosmos-sdk-proto v0.25.0-pre.0 is out, can this be revived?

Happy to make any upstream changes necessary to get this to work before a final v0.25.0 release.

romac commented 1 month ago

Sure, will try to get this done ASAP

romac commented 1 month ago

Blocked by https://github.com/cosmos/cosmos-rust/issues/499

tony-iqlusion commented 1 month ago

@romac I have published cosmos-sdk-proto v0.25.0-pre.1 which should fix the serde + no_std issue

romac commented 1 month ago

@tony-iqlusion Works perfectly, thanks! Ready for review

romac commented 1 month ago

Let's not merge it quite yet as I would first like to do a release with the current pending changes before releasing this.

romac commented 1 month ago

ibc-proto v0.48.0 is out, so we can merge this whenever :)

tony-iqlusion commented 1 month ago

@romac if everything looks good I can cut a final v0.25.0 release of cosmos-sdk-proto, though perhaps you'd like to validate it elsewhere first

romac commented 1 month ago

@Farhad-Shabani @rnbguy Can you please take a look at this PR when you have time, and see if it works for you?

romac commented 1 month ago

@tony-iqlusion @Farhad-Shabani @rnbguy Right now we are re-exporting the Cosmos SDK proto from cosmos_sdk_proto as ibc_proto::cosmos, but I wonder if we are not better off just removing that export and have people use cosmos_sdk_proto directly? Or at least make it an optional dependency behind a feature flag? What do you think?

rnbguy commented 1 month ago

if we are not better off just removing that export and have people use cosmos_sdk_proto directly

Very good point @romac ! If there is no dependency to this crate (which is the case, I believe), I prefer to import it by myself directly.

tony-iqlusion commented 1 month ago

If you can completely remove the dependency on cosmos-sdk-proto and use the two crates independently of each other, I think that would make sense. It would simplify updates as well.

Farhad-Shabani commented 1 month ago

It makes sense to me to remove the re-exports and completely drop the dependency on cosmos-sdk-proto. I just noticed ics23 is getting imported from cosmos-sdk-proto. Was there a specific reason for that? I'd prefer ibc-proto to directly be dependent to and re-export the necessary artifacts from ics23 like before, so we can have more control over updates and streamline the process.

romac commented 1 month ago

Ah unfortunately IBC protos do depend on some Cosmos SDK protos, as can be seen here :(

romac commented 1 month ago

I just noticed ics23 is getting imported from cosmos-sdk-proto. Was there a specific reason for that?

If ibc-proto depends on cosmos-sdk-proto and therefore transitively depends in ics23, to me it made more sense to re-export ics23 from the latter rather than depending on it directly.

Now if we do away with the dependency on cosmos-sdk-proto (which I don't believe is possible), see my comment above, then it would indeed make more sense to re-export ics23 directly.

But even if we keep the cosmos-sdk-proto dependency, it could be argued that, even if the ICS23 protos are namespaced under cosmos., since those are IBC-specific protos, that they would not be exposed in cosmos-sdk-proto (and therefore remove the ics23 dependency from cosmos-sdk-proto, if possible) and we would instead re-export them directly from ibc-proto, therefore fulfilling your wish.

Does that make sense?

tony-iqlusion commented 1 month ago

I would be fine with removing the ics23 dependency from cosmos-sdk-proto and having you import it directly

Farhad-Shabani commented 4 weeks ago

Ah unfortunately IBC protos do depend on some Cosmos SDK protos, as can be seen here :(

Oh, I get the dependency now. Thanks for clearing that up, @romac.

But even if we keep the cosmos-sdk-proto dependency, it could be argued that, even if the ICS23 protos are namespaced under cosmos., since those are IBC-specific protos, that they would not be exposed in cosmos-sdk-proto (and therefore remove the ics23 dependency from cosmos-sdk-proto, if possible) and we would instead re-export them directly from ibc-proto, therefore fulfilling your wish.

This fits my view exactly. I see ICS23 as more of IBC-specific protos. Since cosmos-sdk-proto is fine with it, let’s re-export directly from ibc-proto if that works for you as well.

tony-iqlusion commented 3 weeks ago

Here's a PR to remove the ics23 dependency from cosmos-sdk-proto: https://github.com/cosmos/cosmos-rust/pull/502