Deprecate generate_schnorrsig_keypair method #372
Conversation
tcharding
force-pushed
the
schnorr-rename
branch
from
3645800 to
dc04e55
Compare
There was a problem hiding this comment.
ACK dc04e55
Good to be consistent in naming, though I'm a bit confused about what this method does or why it exists. A KeyPair already contains a secret key and a public key, which can be extracted using upstream functions secp256k1_keypair_pub and secp256k1_keypair_sec (although we don't expose these, presumably an historical accident).
Also the comment doesn't really make sense to me. What is "batch keypair generation" and why would calling those methods directly be more efficient than calling this one?
|
I've ACKed this because it makes sense as a cleanup PR, but I think a better approach would be to deprecate this method, expose accessors for the secret/public part of the keypair, and suggest people just use |
lol, I assumed this 'batching' was just another part of the Taproot code I had not fully groked yet, funny you pointed this out. I'll re-work this PR. |
Currently to get the `XOnlyPublicKey` from a `KeyPair` users must do `XOnlyPublicKey::from_keypair(&kp)`. While this does the job we can make the lib more ergonomic by providing a method directly on `KeyPair` that calls through to `XOnlyPublicKey::from_keypair`. Add method `KeyPair::public_key(&self)`.
We have deprecated all other functions that use the identifier 'schnorrsig' but we missed `generate_schnorrsig_keypair`. This function is purely a helper function and serves no real purpose other than to reduce two lines of code to a single line. Downstream users can write this function themselves if they need it. Also, we recently added a new public method to `KeyPair` to get the public key in a slightly more ergonomic fashion. Use `kp.public_key()` when replacing usage of now deprecated `generate_schnorrsig_keypair` function.
tcharding
force-pushed
the
schnorr-rename
branch
from
dc04e55 to
97524b2
Compare
|
@apoelstra I've not added functionality for exposing secret key as you suggest because I'm not fully aware of why it was not exposed before hand. Seems like a different PR anyways. I have added a Re-review at your leisure please. |
tcharding
changed the title
|
|
||
| /// Gets the [XOnlyPublicKey] for this [KeyPair]. | ||
| #[inline] | ||
| pub fn public_key(&self) -> XOnlyPublicKey { |
There was a problem hiding this comment.
@tcharding @apoelstra
Post merge comment: The name is somewhat confusing because there is already a PublicKey(different from XOnlyPublicKey) type that the impl shown below. I know that we have had the discussion of return type in function names several times, but I think based on confusion between XOnlyPublicKey and PublicKey, we should have named to x_only_public_key. Perhaps we should add another method public_key that actually returns a full PublicKey.
Because this is already released code, we can perhaps use the names pub_key and x_only_pub_key ? What do you think?
impl From<KeyPair> for PublicKey {
#[inline]
fn from(pair: KeyPair) -> Self {
PublicKey::from_keypair(&pair)
}
}There was a problem hiding this comment.
The deprecate thing is just to try and be kind to our users, right? No need to be dogmatic with it if we botch something up and there is no nice way to fix it nicely while using 'deprecated since'. Also trait impls can't be deprecated anyways so we cannot be dogmatic about it.
There was a problem hiding this comment.
Sorry, for the long unclear previous comment. I was talking about the first commit in this PR. Not the second one that deprecates another method. My basic objection was that .public_key should have returned the PublicKey type, but it returns the XonlyPublicKey type.
Recently we deprecated a bunch of functions/methods that used the term
schnorrsig. Seems we leftgenerate_schnorrsig_keypairin there, along with some stale docs on it.KeyPair::public_keythat calls through toXOnlyPublicKey::from_keypair.generate_schnorrsig_keypairand uses the newly definedpk.public_key()everywhere.Note to reviewers
Please note, this PR has been totally re-written using the suggestions below by @apoelstra.