Reorganize tendermint::abci to clean up module organization and conform to Rust RFC356

[hdevalence]

Currently, the ABCI types in tendermint_proto have the structural content filled in, but aren't nicely organized yet:

Rust's RFC356 recommends that type names should avoid having common prefixes, and instead use module paths to factor a name into components, for instance having abci::request::{Commit, CheckTx} instead of abci::{RequestCommit, RequestCheckTx}. This has the advantage that the user of the name can decide which parts of the name are required, writing any of

• abci::request::Commit,
• request::Commit,
• Commit,

depending on how much context is assumed at the use site. It also makes navigating the related types in the docs more clear, since the types are organized into categories.

What's the definition of "done" for this issue?

To tidy the module organization:

• rename and move RequestFoo to request::Foo;
• move the Request enum to request and pub use it in the abci module;
• rename and move ResponseFoo to response::Foo;
• move the Response enum to response and pub use it in the abci module;
• eliminate the response_apply_snapshot_chunk and response_offer_snapshot modules, by either:
  • option A: rename response_apply_snapshot_chunk::Result to response::ApplySnapshotChunkResult (least impact on code that currently uses the structure, it's a clean rename);
  • option B: change the data modeling of ApplySnapshotChunk and OfferSnapshot to hold enums internally (need to make non-mechanical changes to using code, but potentially cleaner in the long run);

I would pick option A above.

To fill in the documentation:

• Document all requests and responses and all fields, copying descriptions from the Methods and Types spec page.

To make it easier to model the four ABCI connections:

Create per-connection enums:

• ConsensusRequest/ConsensusResponse;
• MempoolRequest/MempoolResponse;
• InfoRequest/InfoResponse;
• SnapshotRequest/SnapshotRequest;

with conversions impl From<FooRequest> for Request (infallible, all variants of the per-connection enum are variants of the full enum) and impl TryFrom<Request> for FooRequest (fallible, not all variants of the full enum are variants of the per-connection enum).

[hdevalence]

I realized after writing this up that what I suggested doesn't actually make sense.

The code in question is proto-generated, so it can't be meaningfully edited without being clobbered later, and I missed this on first reading because I didn't realize that the generated code is checked in as part of the source tree.

@thanethomson pointed out that the tendermint crate has more Rustic "domain types" for common use, but although there's a tendermint::abci module there, it's not filled in with all of the ABCI message types, and the tendermint_abci crate uses the protos directly instead of the tendermint domain types.

The data modeling suggested above could instead be done on a set of domain types in the tendermint crate, with conversions to/from the proto types.

[thanethomson]

The data modeling suggested above could instead be done on a set of domain types in the tendermint crate, with conversions to/from the proto types.

Makes sense 👍

[hdevalence]

Cool, I'll instead work on a PR to produce those types as part of tendermint::abci?

[thanethomson]

Yes, I think that makes sense. I've been debating whether or not to implement domain types for the ABCI data structures for some time now, and can summarize the pros and cons I've thought of as follows:

Pros

• We can potentially shield ABCI users from breaking Protobuf changes (e.g. through the use of defaults for new fields, where applicable).
• We can structure them in a way that's more consistent with Rust's API guidelines (as per the core issue here in Reorganize tendermint::abci to clean up module organization and conform to Rust RFC356 #856).
• We can attach additional documentation to them, explaining them better.
• We have the freedom to implement conversions to/from other types quite easily.

Cons

• For significant breaking changes in the Protobuf definitions, we need to manually update these data structures.
• There's more code to maintain (structures, documentation and conversions).

Ultimately it comes down to the fact that it's more work to maintain them, but I think the benefits are worth the effort.

@romac, what do you think?

[romac]

I am personally a bit wary of exposing the Protobuf type directly, as to me they are a serialization concern only, and I would rather expose properly designed Rust domain types, for all the pros you list above.

At the same time I don't want to dismiss the cost of having to maintain this extra code, but if you ask me, I agree that the pros outweigh the cons for the benefit of the users of the library.

[hdevalence]

I don't think there's a tension between exposing domain types and exposing protobuf types. There are use-cases that require domain types and use-cases that require raw protos. It seems like the problem is that if there's only one set of types, those use cases get conflated.

Even if there are well-modeled domain types and the protos are only a serialization concern, they're still required for anything that needs to work with serialization. For example, I'm working on an alternative ABCI interface. I would like that interface to use well-modeled domain types, but internally it needs to know about the protos to be able to speak TSP.

I'm not sure exactly what was meant by "wary of exposing the Protobuf type directly", so my apologies if I misunderstood what you meant, but I don't think it makes sense to try to hide the protos completely, since they're part of the ecosystem. Keeping them in a separate crate means they can be versioned independently of the main API, and people don't need to use them by default. They can opt-in by using the tendermint_proto crate, which has a different level of API.

[romac]

@hdevalence I fully agree with you! I should have been clearer about what I meant by "wary of exposing the Protobuf type directly", which is that I would not want to expose the Protobuf types directly in the public API. We definitely want to export them somehow (either in the tendermint_abci or tendermint_proto crate) for others to use for serialization. Apologies for the misunderstanding, and thank you for your comment!

[hdevalence]

Closed by #1022