p2p: replace prost-amino with prost
#979
Conversation
p2p/Cargo.toml
Outdated
| [lib] | ||
| test = false |
There was a problem hiding this comment.
Not sure why this was here, but I added a test so I had to remove this to make the test work
There was a problem hiding this comment.
The reason it is declared is that all tests are in a separate crate over at https://github.com/informalsystems/tendermint-rs/tree/master/test
There was a problem hiding this comment.
Aah, ok.
My test was mostly useful for ensuring message equivalence during development. Since I'm fairly confident of that at this point (and hopefully all of this can be removed soon) I can remove it and restore these lines.
There was a problem hiding this comment.
Or keep the test and add it to the test crate?
There was a problem hiding this comment.
@thanethomson the API the test is written against isn't public.
I went ahead and removed it for now.
The only message that `prost-amino` was still used for was `AuthSigMessage`, and the only way an amino-encoded version of that message differs from a proto is the amino type prefix on the `pub_key` field, i.e. `tendermint/PubKeyEd25519`. This commit adds a test vector for a serialized amino message, and then changes `AuthSigMessage` to use `prost-derive`: - An `AuthSigMessage::new` function takes care of adding the Amino prefix to the public key - A `TryFrom` impl for `proto::p2p::AuthSigMessage` takes care of verifying and removing it With that, `prost-amino` is no longer necessary and this commit therefore removes the last dependency on it.
tony-iqlusion
force-pushed
the
p2p/remove-amino-rs
branch
from
cc79374
to
beed538
Compare
Codecov Report
@@ Coverage Diff @@
## master #979 +/- ##
========================================
+ Coverage 72.3% 72.5% +0.2%
========================================
Files 204 203 -1
Lines 16587 16597 +10
========================================
+ Hits 12005 12046 +41
+ Misses 4582 4551 -31
Continue to review full report at Codecov.
|
p2p/Cargo.toml
Outdated
| [lib] | ||
| test = false |
There was a problem hiding this comment.
The reason it is declared is that all tests are in a separate crate over at https://github.com/informalsystems/tendermint-rs/tree/master/test
|
Could you also add a changelog entry. |
It seems tests are now placed in a `test` crate, however this is a private API that cannot be called from a separate crate. Hopefully all Amino support can be removed soon, so now that compatibility has been achieved it shouldn't really matter.
|
Changelog entry added, and looks like all of the tests are passing now except a docs failure happening internally within Not sure what to do about that but I would hope #978 can now address it. |
| prost-amino = { version = "0.6", optional = true } | ||
| prost-amino-derive = { version = "0.6", optional = true } |
|
Awesome thanks @tony-iqlusion! Yes, #978 should address that. Once I've merged this I'll pull these changes into #978 to make 100% sure. |
The only message that
prost-aminowas still used for wasAuthSigMessage, and the only way an amino-encoded version of that message differs from a proto is the amino type prefix on thepub_keyfield, i.e.tendermint/PubKeyEd25519.This commit adds a test vector for a serialized amino message, and then changes
AuthSigMessageto useprost-derive:AuthSigMessage::newfunction takes care of adding the Amino prefix to the public keyTryFromimpl forproto::p2p::AuthSigMessagetakes care of verifying and removing itWith that,
prost-aminois no longer necessary and this commit therefore removes the last dependency on it.