Skip to content
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

Deprecate generate_schnorrsig_keypair method #372

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jan 11, 2022

Recently we deprecated a bunch of functions/methods that used the term schnorrsig. Seems we left generate_schnorrsig_keypair in there, along with some stale docs on it.

  • Patch 1: Adds method KeyPair::public_key that calls through to XOnlyPublicKey::from_keypair.
  • Patch 2: Deprecates generate_schnorrsig_keypair and uses the newly defined pk.public_key() everywhere.

Note to reviewers

Please note, this PR has been totally re-written using the suggestions below by @apoelstra.

apoelstra
apoelstra previously approved these changes Jan 11, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@apoelstra
Copy link
Member

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 KeyPair::new and .public_key() to extract the xonly key from it.

@tcharding
Copy link
Member Author

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?

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
Copy link
Member Author

tcharding commented Jan 12, 2022

@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 public_key method as suggested and used it throughout, turned out cleaner, thanks.

Re-review at your leisure please.

@tcharding tcharding changed the title Rename schnorrsig method Deprecate generate_schnorrsig_keypair method Jan 12, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 97524b2

@apoelstra apoelstra merged commit 83e3372 into rust-bitcoin:master Jan 12, 2022
@dr-orlovsky dr-orlovsky added this to Done in Taproot Jan 13, 2022
@tcharding tcharding deleted the schnorr-rename branch January 20, 2022 01:03

/// Gets the [XOnlyPublicKey] for this [KeyPair].
#[inline]
pub fn public_key(&self) -> XOnlyPublicKey {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my bad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants