Fix SecretKey FromStr bug

[sanket1729]

SecretKey::from_str did not check if the secret key was a valid one or not

[elichai]

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)

[sanket1729]

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.

[elichai]

Damn, that's a good find, Thanks!

[elichai]

ACK
(idk about the name of the function, but I'll leave bikeshedding to others)

[dr-orlovsky]

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)

[sanket1729]

@dr-orlovsky, It's not possible for PublicKey because we don't know the size of PublicKey (33/65). So, it's a bit more involved and would require more discussion for two different methods. Something like from_inner_compresesed and from_inner_uncompressed might lead away from the simple bug fix.

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 from_slice method to avoid more discussion :P

[apoelstra]

ack 6265b25

[apoelstra]

There is an argument that this is a breaking change, but I consider it a bugfix so it's not.