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

Re-export the ics23.cosmos.v1 Protobuf definitions from the ics23 crate #92

Merged
merged 9 commits into from
May 1, 2023

Conversation

romac
Copy link
Member

@romac romac commented Apr 19, 2023

Closes: #10

The proto definitions are exported both under the ibc_proto::cosmos::ics23::v1 module and under the ibc_proto::ics23 module for backward source compatiblity.

This is nonetheless a breaking change as it may break compilation or trigger warnings in code which relied on these definitions being different than the ones in ics23.

Warning
Note that because the code generated by pbjson-build is not no_std compatible, the serde annotations on the generated protos are only emitted when the std feature of ibc-proto is enabled, which is unfortunate but I didn't find a way around that.

@romac romac marked this pull request as ready for review April 27, 2023 07:50
@romac romac requested review from hu55a1n1 and mzabaluev and removed request for hu55a1n1 April 27, 2023 14:51
@erwanor
Copy link
Collaborator

erwanor commented Apr 28, 2023

Thanks for your work on this @romac.

Re: pbjson, that seems like a necessary tradeoff, and we can help with getting upstream to get no_std compliance. It would be great to merge this soon because our ICS23 support hinges on this + a release, and we're already a little behind schedule 🙏

Copy link
Contributor

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Not familiar with the changes in .github/workflows/rust.yml, otherwise the changes look good as far as I understand.

Thanks Romain!

@romac
Copy link
Member Author

romac commented Apr 28, 2023

Before merging this and doing a release, I would just like to get some feedback from @plafer or @Farhad-Shabani on whether or not they use the JSON impls on no_std or are aware of anyone using those. Either way this shouldn't prevent us from doing a release on Monday, but maybe then as an alpha to give some time to find an alternative solution (eg. no_std support in pbjson) before the final release.

@plafer
Copy link
Contributor

plafer commented Apr 28, 2023

I don't have time to do a thorough analysis of this right now, but we do not use the JSON impls internally. Off the top of my head, I do not know of anyone that does.

@romac
Copy link
Member Author

romac commented May 1, 2023

I am told we have a potential user of JSON serialization in no_std context, so let's do an alpha release and then try to find a way to restore no_std compat for the generated code.

@romac romac merged commit 0caffd8 into main May 1, 2023
@romac romac deleted the extern-ics23 branch May 1, 2023 07:45
@romac
Copy link
Member Author

romac commented May 1, 2023

romac pushed a commit to cosmos/ics23 that referenced this pull request Aug 16, 2023
Closes: #156

Addresses the `no_std` compatibility issue with `serde` feature in `ibc-rs`. This is caused by the recent implementation of ProtoJSON serialization and deserialization [0] for the `ics23` Protobuf definitions using `pbjson`, and then re-exporting the ics23 type [1] in `ibc-proto-rs`. Some of our users by then (starting from IBC-rs v0.41.0) are experiencing compilation errors. [2]

To meet this immediate need [3] and the lack of activity in the `pbjson` crate for months, we have taken the initiative to feature `no_std` support in the `informalsystems-pbjson` crate and have it published.

[0] #146
[1] cosmos/ibc-proto-rs#92
[2] cosmos/ibc-rs#741
[3] cosmos/ibc-proto-rs#98 (comment)

---

* feat: enable no_std support for pbjson

* fix: get serde feature work with no-std

* deps: use informalsystems-pbjson v0.6.0

* deps: use informalsystems-pbjson v0.6.0
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.

Duplicate proto files for ICS23
4 participants