Adding KeyPair::serialize_sec. Closes #298 #308
Conversation
dr-orlovsky
force-pushed
the
extrakeys/keypair-ser
branch
from
0ed9c4f
to
7e2f7fe
Compare
src/schnorrsig.rs
Outdated
| pub fn serialize_sec<V: Verification>(&self, secp: &Secp256k1<V>) -> [u8; constants::SECRET_KEY_SIZE] { | ||
| *SecretKey::from_keypair(secp, self).as_ref() |
There was a problem hiding this comment.
This doesn't really require a context, See #306 (review)
dr-orlovsky
force-pushed
the
extrakeys/keypair-ser
branch
from
c241c57
to
8ee4e05
Compare
| /// Serialize the key pair as a secret key byte value | ||
| #[inline] | ||
| pub fn serialize_secret(&self) -> [u8; constants::SECRET_KEY_SIZE] { | ||
| *SecretKey::from_keypair(self).as_ref() | ||
| } |
There was a problem hiding this comment.
I know that adding this function was the original idea of this PR.
I am wondering on whether or not this is still worth to have given the now very convenient functions of converting a KeyPair into a SecretKey. Working with From and Into can sometimes be annoying though - esp. when chaining methods together - because the compiler sometimes requires type annotations.
If serializing a KeyPair is the ultimate goal, what about the following API:
impl KeyPair {
pub fn secret_key(&self) -> SecretKey;
pub fn public_key(&self) -> PublicKey;
}These are easily accessible, will never require type hints but are more general than providing the secret-key bytes directly. As such, they compose better with the existing functionality. Serializing a KeyPair is then just a matter of:
let bytes = keypair.secret_key().as_ref();There was a problem hiding this comment.
I overall does not like the current API on getting byte representation of different key types, which uses very distinct names and approaches and not cohesive at all (serialize* in one cases, [..] or as_ref() in other)...
I do not have any preference here other then overhauling the whole key API, which is not a task of this PR. As regarding this PR I will be happy to update it to whatever API will be supported by @apoelstra, @elichai, you and other lib mantainers.
|
@apoelstra any other suggestions regarding #308 (comment) for this PR or it can be merged? |
There was a problem hiding this comment.
ACK 8ee4e05
I agree this contributes even further to the mess of serialize methods that have different names and different allocation behavior.
In general I'm okay with this library being a bit weird/inconsistent, because it has pretty strong requirements to be zero-allocation and has to follow the contours of the underlying cryptography, and in this case deal with conflicting viewpoints about how accessible raw secret data should be.
Also note this is a breaking change because of the removal of the context requirement from from_keypair.
No description provided.