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

Find and replace all amino encoding with protobuf #504

Closed
greg-szabo opened this issue Aug 4, 2020 · 2 comments · Fixed by #527
Closed

Find and replace all amino encoding with protobuf #504

greg-szabo opened this issue Aug 4, 2020 · 2 comments · Fixed by #527
Assignees
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request serialization
Milestone

Comments

@greg-szabo
Copy link
Member

for Tendermint 0.34 compatibility. Maybe it's worth identifying all places first and opening smaller issues to updates parts separately.

#305

@xla xla added dependencies Pull requests that update a dependency file enhancement New feature or request serialization labels Aug 4, 2020
@greg-szabo greg-szabo mentioned this issue Aug 5, 2020
5 tasks
@greg-szabo greg-szabo self-assigned this Aug 5, 2020
@greg-szabo
Copy link
Member Author

I think we have our work cut out for us here. There are 11 files in the amino_types folder, so at least 11 types have to be overhauled. I've gone through the tendermint/src/amino_types/block_id.rs file to see how it's done now. That file implements BlockId, CanonicalBlockId, PartsSetHeader, CanonicalPartSetHeader.

These structs are implemented in proto, currently in the tendermint_proto crate, under tendermint_proto::types.

Note, that only the struct definitions are implemented using proto. The implementation details (traits and additional functions) are implemented on the amino struct directly. This cannot be done on a proto struct if it's defined in a separate crate.

One option is to copy the auto-generated proto Rust structs into the Tendermint source code and implement functionality on them directly. This has the short-term benefit of getting the overhaul done slightly quicker.
The disadvantage is that the auto-generated proto files become part of the manually generated source code and it's going to be hard to maintain them automatically. Keeping them in a separate crate has the benefit of easily rebuilding them when a new version of the proto files are created.

Another option is keeping them in the separate crate and using the pattern we started with CommitSig in this repo and with the ibc_proto package implementation in the ibc-rs repo:
Use the auto-generated proto Rust structs as the raw types for the data coming from the wire and implement a domain type that is the native Rust implementation of that data. (For example using enums instead of ints, etc.) Then we can implement the TryFrom trait on this domain type and use that as conversion and validation in one go from the "raw" type defined by proto.
This has the benefit of keeping the proto files separate and also allowing for other serialization implementations easily.
It has the disadvantage that we have to separate the currently integrated struct and implementations and add the validation using TryFrom.
We can choose to not do too much conversion right now and keep the currently implemented amino structs as domain structs (without the amino-encoding annotations) while implementing a TryFrom trait on them to be able to convert from the proto-defined types. This has the benefit that possibly the code stays compatible with any application written on top of this library. (Except for the amino-conversion parts.)

I believe I should implement one such struct to show how it would look like. I'm going to try to do that with BlockId.

I'm taking all these notes because I'm interested in feedback for other methods. Maybe I'm overcomplicating this and there's an Occam's razor somewhere that I'm missing. Feel free to add notes to this issue.

@xla
Copy link
Contributor

xla commented Aug 6, 2020

Thanks for the exhaustive write-up, this is great to onboard someone on the context and the challenges. In essence we can't really fight Rusts orphan rules and going down the past where we manually copy code over and slab things on it sounds like a recipe for disaster.

While it's not the most effective option having safe conversions into properly typed domain types sounds appealing as the serialisation artifacts are pushed to the edge and we can with greater care craft our native entities. If we go down such a route we have to start thinking about the conract we provide through these shapes, i.e. what would it mean changing those and advancing them. Will they be subject to versioning? How can a user safely integrate against them? etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request serialization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants