-
Notifications
You must be signed in to change notification settings - Fork 224
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
Use ed25519-consensus instead of ed25519-dalek #1046
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ | |
|
||
use std::convert::TryInto; | ||
|
||
use ed25519_dalek as ed25519; | ||
use prost::Message as _; | ||
|
||
use x25519_dalek::PublicKey as EphemeralPublic; | ||
|
@@ -113,8 +112,8 @@ impl Version { | |
#[must_use] | ||
pub fn encode_auth_signature( | ||
self, | ||
pub_key: &ed25519::PublicKey, | ||
signature: &ed25519::Signature, | ||
pub_key: &ed25519_consensus::VerificationKey, | ||
signature: &ed25519_consensus::Signature, | ||
) -> Vec<u8> { | ||
if self.is_protobuf() { | ||
// Protobuf `AuthSigMessage` | ||
|
@@ -126,7 +125,7 @@ impl Version { | |
|
||
let msg = proto::p2p::AuthSigMessage { | ||
pub_key: Some(pub_key), | ||
sig: signature.as_ref().to_vec(), | ||
sig: signature.to_bytes().to_vec(), | ||
}; | ||
|
||
let mut buf = Vec::new(); | ||
|
@@ -168,8 +167,8 @@ impl Version { | |
#[cfg(feature = "amino")] | ||
fn encode_auth_signature_amino( | ||
self, | ||
pub_key: &ed25519::PublicKey, | ||
signature: &ed25519::Signature, | ||
pub_key: &ed25519_consensus::VerificationKey, | ||
signature: &ed25519_consensus::Signature, | ||
) -> Vec<u8> { | ||
// Legacy Amino encoded `AuthSigMessage` | ||
let msg = amino_types::AuthSigMessage::new(pub_key, signature); | ||
|
@@ -184,8 +183,8 @@ impl Version { | |
#[cfg(not(feature = "amino"))] | ||
fn encode_auth_signature_amino( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this method's even necessary. Why would one want to convert a compile-time error to a runtime error like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, I tried to leave the rest of the code untouched as a drop-in change. |
||
self, | ||
_: &ed25519::PublicKey, | ||
_: &ed25519::Signature, | ||
_: &ed25519_consensus::VerificationKey, | ||
_: &ed25519_consensus::Signature, | ||
) -> Vec<u8> { | ||
panic!("attempted to encode auth signature using amino, but 'amino' feature is not present") | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -208,8 +208,7 @@ define_error! { | |
|_| { format_args!("subtle encoding error") }, | ||
|
||
Signature | ||
[ DisplayOnly<signature::Error> ] | ||
|_| { format_args!("signature error") }, | ||
|_| { "signature error" }, | ||
Comment on lines
210
to
+211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to my other comment: should we not find a way here to bubble up the original error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. xref #1046 (comment) |
||
|
||
TrustThresholdTooLarge | ||
|_| { "trust threshold is too large (must be <= 1)" }, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be worthwhile to bubble up the original error here? Unfortunately it seems like the
ed25519_consensus::Error
type doesn't implementstd::fmt::Display
unless you turn on thestd
feature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation for removing the original error was that the code this is replacing used an opaque error type, so there's no additional information conveyed by the original error, and was easier to just remove it rather than patch the
ed25519_consensus::Error
type to have an extraDisplay
impl. But I could add thatDisplay
impl in another release.