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

Duplicate proto files for ICS23 #10

Closed
5 tasks done
adizere opened this issue Apr 26, 2021 · 0 comments · Fixed by #92
Closed
5 tasks done

Duplicate proto files for ICS23 #10

adizere opened this issue Apr 26, 2021 · 0 comments · Fixed by #92

Comments

@adizere
Copy link
Contributor

adizere commented Apr 26, 2021 •
edited
Loading

Crate

ibc and ibc-proto

Problem Definition

Currently, the canonical place for the .proto file of ICS23 is confio/ics23. The Cosmos-SDK and IBC-go repos both copy the proofs.proto file from confio/ics23 as a "third party proto". The ibc-proto crate similarly copies the same proofs.proto file from Cosmos-SDK. In general, the state of .proto files is quite fractured, but ICS23 is even more so.

This fracturing creates the problem that there are two version of the ICS23 specs:

  • in ibc_proto we have this
  • in the "canonical" confio/ics23 there is this

The two files are not identical, since the confio/ics23 version has been recently changed, and the updates did not propagate downstream into CosmosSDK nor into ibc-proto (yet). Even if the files were on par, there would still be two legit definitions of the same data structures, for every structure in ics23.rs.

A specific instance where this is problematic is that we need awkward code to convert from ics23::ProofSpec to the identical data structure ibc_proto::ics23::ProfSpec.

Proposal

Ideally, crate ibc should depend on a single .proto definition of the ICS23 files.

Acceptance Criteria

  • The fix is to avoid generating ics23.rs in ibc-proto crate and rely entirely on confio/ics23. This means that ibc_proto::ics23 should not exist anymore.

The long-term fix is to have all .proto files related to IBC stored in a single unified place, e.g., repo cosmos/ibc. This is a larger eco-system problem, however, which we don't need to track with this issue.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere self-assigned this Apr 26, 2021
@adizere adizere mentioned this issue Apr 27, 2021
Merged
5 tasks
@hu55a1n1 hu55a1n1 mentioned this issue Nov 19, 2021
Merged
5 tasks
@adizere adizere removed their assignment Feb 24, 2022
@hu55a1n1 hu55a1n1 transferred this issue from informalsystems/hermes Sep 29, 2022
@kevinji kevinji mentioned this issue Apr 19, 2023
Closed
This was referenced Apr 19, 2023
Closed
Merged
@romac romac closed this as completed in #92 May 1, 2023
romac added a commit that referenced this issue May 1, 2023
@romac
Re-export the ics23.cosmos.v1 Protobuf definitions from the ics23…
0caffd8 
… crate (#92)

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`.

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.

* Use `ics23` crate Protobuf definitions for `ics23.cosmos.v1` protos

* Run Clippy on all possible features combinations using `cargo-hack` on CI

* Add changelog entry

* Use `pbjson`-based protos

* Use master branch

* Use latest published version of `ics23`

* Only emit serde annotations when `std` feature is enabled

* Fix compilation with std enabled
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.

Re-export the ics23.cosmos.v1 Protobuf definitions from the ics23 crate cosmos/ibc-proto-rs
1 participant
@adizere