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

Restore no_std compatibility for JSON serialization impls #98

Closed
romac opened this issue May 1, 2023 · 8 comments · Fixed by #118
Closed

Restore no_std compatibility for JSON serialization impls #98

romac opened this issue May 1, 2023 · 8 comments · Fixed by #118

Comments

@romac
Copy link
Member

romac commented May 1, 2023

See #92 and #95 for more details,
@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented May 2, 2023 •
edited
Loading

FYI: Reached out to the ComposableFi, they mentioned that they plan to upgrade their ibc-proto-rs fork from the upstream, but not earlier than in a month or two

@Farhad-Shabani
Copy link
Member

Just noticed upon upgrading ibc-proto-rs to v0.31.0 in ibc-rs (this PR), the CI can’t pass no-std-check in which we make the serde feature on to check no-std compatibility for the Substrate users. (The ics23/serde works for std environments)

Sounds like we either should (1) upstream no_std compliance for pbjson or alternatively (2) define some domain types representing ics23 protos so can derive no-std serde on them.

Also, @DaviRain-Su to check with you: are you dependent on any of the derived serde within the ic23_commitment of ibc-rs (e.g. here) at the moment?

@Farhad-Shabani Farhad-Shabani mentioned this issue Jun 2, 2023
Merged
7 tasks
@DaviRain-Su
Copy link
Contributor

Also, @DaviRain-Su to check with you: are you dependent on any of the derived serde within the ic23_commitment of ibc-rs (e.g. here) at the moment?

I have not any dependent for this.

@romac
Copy link
Member Author

romac commented Jun 4, 2023

Glad to hear! Then let's close this if no one objects.

Should the need for pbjson support on no-std arise in the future we may look into adding that (I actually have a WIP branch doing just that but more work is needed and I have more pressing things to take care of at the moment).

@romac romac closed this as not planned Won't fix, can't repro, duplicate, stale Jun 6, 2023
@Farhad-Shabani Farhad-Shabani mentioned this issue Jul 5, 2023
Closed
@Farhad-Shabani
Copy link
Member

Given the need brought up in cosmos/ibc-rs#741, this issue is reopened.

@dzmitry-lahoda
Copy link

Hm, actually all was working until very specific commit recently. May be 3 weeks ago.

@dzmitry-lahoda
Copy link

dzmitry-lahoda commented Jul 27, 2023 •
edited
Loading

In Composable there are 2 forks, one is 0.15 version (super old, on of reason was no_std issue i recall), and other one on last version before serde in no_std was broken (so we again no_std issue).

We are trying to be first chain to be Cosmos like chain (CW and IBC) not written in Go, but that is really hard. Issue is that when CW lib or IBC-rs break no_std, it would go deep several libs down the way, and unrealistically to drive fixes of no_std in so many libs.

No sure how other no_stds handle that. In current case no_std broken in some dep which depens on some generator.

@Farhad-Shabani Farhad-Shabani mentioned this issue Jul 27, 2023
Merged
@Farhad-Shabani
Copy link
Member

We are trying to be first chain to be Cosmos like chain (CW and IBC) not written in Go, but that is really hard. Issue is that when CW lib or IBC-rs break no_std, it would go deep several libs down the way, and unrealistically to drive fixes of no_std in so many libs.

Sorry hearing that you are experiencing such trouble.
Hopefully, #118 should fix the issue. Let us know if any other adjustments needed regarding no_std

@Farhad-Shabani Farhad-Shabani mentioned this issue Jul 28, 2023
Merged
romac pushed a commit to cosmos/ics23 that referenced this issue Aug 16, 2023
@Farhad-Shabani
feat: enable no_std support for pbjson (#158)
864b821 
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
soareschen added a commit that referenced this issue Sep 25, 2023
@soareschen
Merge tag 'v0.34.0' into luca_joss/solomachine-lightclient-v3-from-v0.32
f0bd7b4 
ibc-proto-rs v0.34.0

August 17th, 2023

This release updates the Cosmos SDK protos to v0.47.3 and IBC-Go protos to v7.2.0.

Additionally, it restore `no_std` support for JSON serialization via `serde`.
Previously, `Serialize` and `Deserialize` instances were only derived when
the `std` feature was enabled, but that is no longer required.

As such, they now require the `serde` feature to be enabled, independently of
whether or not the `std` feature is enabled.

BUG FIXES:

- Restore `no_std` support for JSON serialization
  #98

FEATURES:

- Update Cosmos SDK protos to v0.47.3 and IBC-Go protos to v7.2.0
  #129
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: restore no_std support for JSON serialization cosmos/ibc-proto-rs
4 participants
@romac @dzmitry-lahoda @DaviRain-Su @Farhad-Shabani