informalsystems / tendermint-rs

Client libraries for Tendermint/CometBFT in Rust!
Apache License 2.0
609 stars 224 forks source link

Split out `tendermint-proto` into separate repository #1462

Open soareschen opened 1 month ago

soareschen commented 1 month ago

Description

Downstream projects such as ibc-proto and ibc-rs depends only on tendermint-proto, but not the other tendermint crates. By having its own repository and release cycle, we won’t have to depend on the tendermint libraries directly, making it easier to keep tendermint-proto, ibc-proto-rs, and ibc-rs in sync. Plus, we’re aiming to fully decouple the ibc-rs libraries from tendermint (except for the ICS-07 implementation). These together would give us more flexibility, even allowing us to apply upgrades asynchronously.

romac commented 1 month ago

@tony-iqlusion From the cosmos-rust side, any objections with doing this or does it sound good to you?

tony-iqlusion commented 1 month ago

That sounds fine to me. In the past I’ve suggested getting all of the proto crates into a single repo so cross-cutting changes are easier

romac commented 1 month ago

I may be wrong, but I don't expect there should be many such cross-cutting changes when a new version of tendermint-proto is released. It's gonna be a whole other story when cometbft-proto v1 is released, but it'll only be a one-time cost I think.

That said I am not opposed the idea of having a single cosmos/rust-proto repository, it's just that it's a lot more initial work to get everything lined up etc.

@Farhad-Shabani @soareschen What do you think?

Farhad-Shabani commented 1 month ago

From ibc-rs perspective, putting all the proto crates into a single repo, if with a shared workspace where version bumps happen together, is not desirable. The proto level version upgrades are breaking changes for ibc-rs. Therefore, changes in such a repo that don’t directly relate to ibc-rs still impacts the project. Specially, we'd like to decouple ibc-rs (except for ICS-07) from any tendermint dependencies.

At the very least, I recommend keeping tendermint-proto and ibc-proto-rs workspaces and versions separate. Though, to me, generally keeping each workspace in separate repo feels like a simpler way to handle changelog, releases, and maintenance.

soareschen commented 1 month ago

To me, the main considerations for whether to put code in the same repository are as follows:

If we choose to go with monorepo, then there is a secondary concern of whether to put the code in the same workspace:

tony-iqlusion commented 1 month ago

Right now cosmos-sdk-proto and ibc-proto both need a release after tendermint-proto. Soon it will be tendermint-proto -> cosmos-sdk-proto -> ibc-proto.

They all share the same dependencies (e.g. prost and tonic) and each have their own artisanal tool for ETLing protobuf schemas into Rust code.

Just the latter alone seems like a lot of duplicated effort.

tony-iqlusion commented 3 weeks ago

The other argument for consolidating into a single repo is they could share the "proto build" ETL logic for transforming protos into Rust source code, so we can actually share what cool tricks we've developed between them.

Here's one example: https://github.com/cosmos/ibc-proto-rs/pull/240

dakom commented 2 weeks ago

from the outside, building application/tooling that uses these crates, I'm very +1 having it all in one repo. It's been painful with some version conflicts and not knowing where to look, so punting with functions like any_to_any() and rpc_to_grpc_but_it's_really_the_same_type() if y'all catch my drift.

Tbh what I'd really like is one crate focused only on the tonic_build generation, with features so it's all there and kept in sync and I don't even need to think of which Coin or Any or whatever type is the one to use, since there would only be one of those types exported.

Also agree with the benefit of having the build logic in one place for things like that transport feature

Ofc that's just a view from the outside of wanting to import it all, not necessarily maintain it all, and I don't know the whole scope involved with such a request - please take my vote with a grain of salt :)

tony-iqlusion commented 2 weeks ago

@dakom as of the latest releases of tendermint-proto, cosmos-sdk-proto, and the forthcoming release of ibc-proto there should (hopefully) no longer be duplication between types.

tendermint-proto now contains its own versions of the google.protobuf well-known types which cosmos-sdk-proto and ibc-proto now use, and duplication between cosmos-sdk-proto and ibc-proto has been eliminated.

If you see duplicate types at this point, it's a bug.

dakom commented 2 weeks ago

thanks @tony-iqlusion - sorry I missed the notification. most likely I need to update some dependency, or updated and didn't notice that this had been fixed. thanks for the heads up!