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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Sep 13, 2024
Merged

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

merged 13 commits into from
Sep 13, 2024

Conversation

romac
Copy link
Member

@romac romac commented Jan 25, 2024 •
edited
Loading

No description provided.

@tony-iqlusion tony-iqlusion mentioned this pull request Jan 26, 2024
Closed
@romac romac mentioned this pull request Jan 30, 2024
Closed
8 tasks
@tony-iqlusion
Copy link
Member

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
Copy link
Member Author

romac commented Sep 4, 2024 •
edited
Loading

Sure, will try to get this done ASAP

@romac
Copy link
Member Author

romac commented Sep 4, 2024

Blocked by cosmos/cosmos-rust#499

@romac romac mentioned this pull request Sep 4, 2024
Closed
@tony-iqlusion
Copy link
Member

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

@romac romac marked this pull request as ready for review September 5, 2024 19:20
@romac
Copy link
Member Author

romac commented Sep 5, 2024

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

@romac
Copy link
Member Author

romac commented Sep 5, 2024

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
Copy link
Member Author

romac commented Sep 5, 2024

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

@tony-iqlusion
Copy link
Member

@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
Copy link
Member Author

romac commented Sep 6, 2024

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

@romac
Copy link
Member Author

romac commented Sep 6, 2024

@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
rnbguy reviewed Sep 6, 2024
View reviewed changes
Cargo.toml Show resolved Hide resolved
@rnbguy
Copy link
Collaborator

rnbguy commented Sep 6, 2024 •
edited
Loading

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
Copy link
Member

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
Copy link
Member

Farhad-Shabani commented Sep 6, 2024 •
edited
Loading

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
Copy link
Member Author

romac commented Sep 7, 2024

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

@romac
Copy link
Member Author

romac commented Sep 7, 2024 •
edited
Loading

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
Copy link
Member

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

@Farhad-Shabani
Copy link
Member

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 added a commit to cosmos/cosmos-rust that referenced this pull request Sep 9, 2024
@tony-iqlusion
cosmos-sdk-proto: remove ics23 dependency
55cc652 
As discussed in cosmos/ibc-proto-rs#187, removes this dependency so
`ics-proto` can import it directly and we don't need to worry about
keeping the two synchronized.
@tony-iqlusion tony-iqlusion mentioned this pull request Sep 9, 2024
Merged
@tony-iqlusion
Copy link
Member

Here's a PR to remove the ics23 dependency from cosmos-sdk-proto: cosmos/cosmos-rust#502

tony-iqlusion added a commit to cosmos/cosmos-rust that referenced this pull request Sep 9, 2024
@tony-iqlusion
cosmos-sdk-proto: remove ics23 dependency (#502)
5832165 
As discussed in cosmos/ibc-proto-rs#187, removes this dependency so
`ics-proto` can import it directly and we don't need to worry about
keeping the two synchronized.
@romac romac requested a review from rnbguy September 11, 2024 06:49
@romac romac changed the title Use Cosmos SDK and ICS23 protos via the cosmos-sdk-proto crate Use Cosmos SDK protos via the cosmos-sdk-proto crate instead of bundling them in ibc-proto Sep 13, 2024
@romac romac merged commit 2257bc6 into main Sep 13, 2024
10 checks passed
@romac romac deleted the romac/cosmos-sdk-proto branch September 13, 2024 07:15
@rnbguy rnbguy mentioned this pull request Nov 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tony-iqlusion tony-iqlusion tony-iqlusion approved these changes

@rnbguy rnbguy Awaiting requested review from rnbguy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@romac @tony-iqlusion @rnbguy @Farhad-Shabani