Merge prost-fork, delete previous amino_rs code#12
Conversation
- proposal / SignProposalMsg deserialization is working again 🎉 - annotated & updated other structs - tests need to be updated for other structs - commented out a few distractions (json cannonicalization)
- aminoName -> amino_name
- currently only (scalar) bytes work (used in tendermint/tmkms#35)
- necessary for tendermint/tmkms#35 to work - no other (scalar) types are currently supported (only bytes)
…ge_prost_fork - this should give us the whole git history of amino_rs, prost, and the commits in our fork - additionally, this renames the crate to prost-amino instead of prost (or previously amino) # Conflicts: # Cargo.toml
zmanian
left a comment
There was a problem hiding this comment.
Good starting point. We probable incrementally improve this over time.
Cargo.toml
Outdated
| version = "0.4.0" | ||
| authors = ["Dan Burkert <dan@danburkert.com>", "zaki", "Ismail Khoffi <ismail@tendermint.com"] | ||
| license = "Apache-2.0" | ||
| repository = "https://github.com/danburkert/prost" |
There was a problem hiding this comment.
Change this to https://github.com/tendermint/amino_rs
Cargo.toml
Outdated
| repository = "https://github.com/danburkert/prost" | ||
| documentation = "https://docs.rs/prost" | ||
| readme = "README.md" | ||
| description = "A Protocol Buffers implementation for the Rust Language." |
There was a problem hiding this comment.
"An implementation of the Amino serialization for Tendermint/Cosmos in the Rust Language.See https://github.com/tendermint/go-amino for details"
| members = [ | ||
| "benchmarks", | ||
| "conformance", | ||
| "prost-build", |
There was a problem hiding this comment.
Pros vs cons on renaming the sub crates?
There was a problem hiding this comment.
Hmm, I would only change names of sub-crates that I've modified (prost-derive and prost). The less we deviate from vanilla prost, the easier it will be to maintain this repo.
There was a problem hiding this comment.
Pros:
- better branding
- signal that we deviate from the original crate here
Cons:
- slightly more difficult to maintain changes from prost!
I think the single Con outweighs the Pros by far though.
README.md
Outdated
| [](https://crates.io/crates/prost) | ||
|
|
||
| Rust implementation of the Amino encoding standard. | ||
| # *PROST!* |
There was a problem hiding this comment.
We need rewrite the whole ReadMe. I can help
There was a problem hiding this comment.
I'll address all other comments you've made (Thanks a lot for the review!) and provide a minimalistic Readme. If you have suggestions, or, want to write it, let me know.
There was a problem hiding this comment.
Updated the Readme. PTAL.
- also update Readme to contain a link instead
|
Do you see any action items left on this @zmanian? |
|
@liamsi merge if you think you are ready |
|
There is still plenty of work to be done but I think it's reasonable to merge this. It is more complete to what we have in amino_rs now. I'll open a bunch issues for what still needs to be done. |
Merge prost-fork, delete previous amino_rs code
This should bring in the whole commit history of prost! (incl. the commits in my fork) and deletes the previous implementation, while still retaining our old history.
Additionally, it renames the crate to prost-amino. I think it better describes what this aims to be (but I haven't thought long about this). Previously, it was called amino for our amino_rs implementation (or prost, for @danburkert's implementation).
We should update and shorten the Readme to explain the state of this repo, point to go-amino and to prost, too.
I think circleci only fails because prost didn't run rustfmt as we do (on circelci). I want to deviate from prost as little as possible. Hence, I propose to turn of rustfmt here :-/ (removed via 7719a49) Additionally, it doesn't run clippy (removed via 4e4e910). Tests still fail on circelci (but pass locally).
After this gets merged we need to update the Cargo.toml in the kms to point to this crate instead of my fork.
related to tendermint/tmkms#38
closes #11