-
Notifications
You must be signed in to change notification settings - Fork 264
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
Fix SecretKey FromStr bug #296
Conversation
src/key.rs
Outdated
/// Converts a `SECRET_KEY_SIZE`- array to a secret key | ||
#[inline] | ||
pub fn from_inner(data: [u8; 32])-> Result<SecretKey, Error> { | ||
match data.len() { |
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.
you're matching on the data len when it's already 32 bytes.
maybe just call SecretKey::from_slice
instead? (altough I do like using the type system to restrict the size of the slice, especially when you can use TryFrom
to convert a slice to an array)
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.
you're matching on the data len when it's already 32 bytes.
Copy-paste error 🤦
I may be misunderstanding this, but I think there was an unnecessary copy_from_slice
in SecretKey::from_slice
if we actually own a [u8; 32]
. The current FromStr
implementation provides an owned [u8; 32]
from hex which we convert to a secret key, so I added this method.
Damn, that's a good find, Thanks! |
cb05055
to
a6430aa
Compare
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.
ACK
(idk about the name of the function, but I'll leave bikeshedding to others)
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.
impl FromStr for PublicKey
in the library uses from_slice
method, which performs checks and returns error if the key is not correct. I assume we need to use the similar approach for impl FromStr for SecretKey
, since there is already a SecretKey::from_slice
method doing all necessary checks. Introduction of new key construction function which duplicate code of already existing method + will exist for SecretKey
type only (and not other) probably is a bad API (or, we need it to be defined for PublicKey
as well and move the shared code to yet another private method)
Secret::from_str did not check if the secret key was a valid one or not.
@dr-orlovsky, It's not possible for The code duplication is one method call 🤷 . I don't care that much about the slight inefficiencies of avoiding a 32-byte copy. I can just use the |
a6430aa
to
6265b25
Compare
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.
ack 6265b25
There is an argument that this is a breaking change, but I consider it a bugfix so it's not. |
SecretKey::from_str
did not check if the secret key was a valid one or not