This repository was archived by the owner on Jun 3, 2020. It is now read-only.
Sign integration test & catch up with latest tm-core develop#55
Merged
Sign integration test & catch up with latest tm-core develop#55
Conversation
- fix decoding issue - remove json cannonicalization
- catch up with latest changes on master - fix decoding issue - remove json cannonicalization & refactor code towards amino encoding instead - add sign_bytes to TendermintSign trait; equivalent to tendermint's Signable interface - add TendermintResponse trait (extending TendermintSign) to build a response from a request
- implement sign_bytes and set_signature for Heartbeat
- verify heartbeat response in test_handle_sign_request
- catch up with changes from master
- catch up with changes from master - minor clean up
- make the test work: don't include the signature, when calling sign_bytes - refactor code to be more idiomatic
- remove some debug output
- sign using the "first" signer
- test via integration test (decoding problem) - add amino roundtrip encode/decode test for SignVoteRequest (fails)
- update CanonicalVote to new field order & fixed size
b1616cd to
18d49a5
Compare
18d49a5 to
6f51dc9
Compare
- move extracting length into separate function - silence some warning about imports
6d6aed1 to
0eb39de
Compare
0dfcdd3 to
7fe38a7
Compare
7fe38a7 to
90790ef
Compare
- rudimentary `get_public_key` method that returns the *only* pub key or an error - add method get_only_signing_pubkey to achive aboveMichel - add PingRequest to integration test - some minor cleanup
90790ef to
44e48eb
Compare
tarcieri
suggested changes
Oct 19, 2018
Contributor
tarcieri
left a comment
There was a problem hiding this comment.
Generally looks good. I left some optional suggested improvements as line notes.
src/ed25519/keyring.rs
Outdated
|
|
||
| /// Sign a message using the secret key associated with the given public key | ||
| /// (if it is in our keyring) | ||
| pub fn sign(public_key: &PublicKey, msg: &[u8]) -> Result<Ed25519Signature, Error> { |
Contributor
There was a problem hiding this comment.
An alternative signature for this which can handle both cases without splitting them into separate functions:
pub fn sign(public_key: Option<&PublicKey>, msg: &[u8]) -> Result<Ed25519Signature, Error>
src/rpc.rs
Outdated
| PingRequest, PingResponse, PoisonPillMsg, PubKeyMsg, SignHeartbeatRequest, SignProposalRequest, | ||
| SignVoteRequest, SignedHeartbeatResponse, SignedProposalResponse, SignedVoteResponse, | ||
| TendermintSignable, HEARTBEAT_AMINO_NAME, PING_AMINO_NAME, POISON_PILL_AMINO_NAME, | ||
| PROPOSAL_AMINO_NAME, PUBKEY_AMINO_NAME, VOTE_AMINO_NAME, |
Contributor
There was a problem hiding this comment.
Can probably just use types::* here
src/rpc.rs
Outdated
| let rem: Vec<u8> = buff.clone().into_inner(); | ||
| let rem: Vec<u8> = | ||
| buff.clone().into_inner()[..(len.checked_add(1).unwrap() as usize)].to_vec(); | ||
| if amino_pre == *PP_PREFIX { |
Contributor
There was a problem hiding this comment.
I guess I missed this in previous review, but all of these if amino_pres look like a good case for a match expression
Contributor
Author
There was a problem hiding this comment.
The match wouldn't be simply
match amino_pre {
PP_PREFIX => return Ok(Request::PoisonPill(PoisonPillMsg {})),
...but something slightly more clumsy like
match amino_pre {
ref pp if *pp == *PP_PREFIX => return Ok(Request::PoisonPill(PoisonPillMsg {})),
...but I guess this is still more idiomatic.
Contributor
There was a problem hiding this comment.
Why can't you do:
match amino_pre.as_ref() {
PP_PREFIX => Ok(Request::PoisonPill(PoisonPillMsg {})),
......or thereabouts?
Contributor
Author
There was a problem hiding this comment.
Thanks! I've tried this (and variants of this), it yields:
- a static `PP_PREFIX` is defined here
...
95 | PP_PREFIX => Ok(Request::PoisonPill(PoisonPillMsg {})),
| ^^^^^^^^^ cannot be named the same as a static
|
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error[E0530]: match bindings cannot shadow statics
--> src/rpc.rs:96:13
a018b4c to
54338fe
Compare
tarcieri
approved these changes
Oct 19, 2018
Contributor
tarcieri
left a comment
There was a problem hiding this comment.
Looks good now, thanks!
Contributor
Author
|
Thanks a lot @tarcieri! I'll merge this and open issues for the left TODOs (#55 (comment)) |
tarcieri
pushed a commit
that referenced
this pull request
Oct 21, 2018
Now that #55 is landed, we can re-enable `#![deny(warnings)]` which is a best practice. This commit does that, and peppers remaining unused code with `#![allow(dead_code)]`. Additionally, to ensure `#![deny(missing_docs)]` is enforced and to follow (evolving) Abscissa conventions, this refactors the app as a library with a small `main.rs` stub.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ready for a first review
resolves #44
Todos:
sign_bytesand messages according toWhyto be on par with privval: Switch to amino encoding in SignBytes tendermint#2459Vote.SignBytesis a JSON string? tendermint#1622 (comment)get_public_key(resolves Implement get_public_key #72)