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

Constructors for compressed and uncompressed ECDSA keys #592

Merged
merged 3 commits into from May 6, 2021
Merged

Constructors for compressed and uncompressed ECDSA keys #592

merged 3 commits into from May 6, 2021

Conversation

dr-orlovsky
Copy link
Collaborator

No description provided.

@dr-orlovsky dr-orlovsky added the minor API Change This PR should get a minor version bump label Apr 12, 2021
@dr-orlovsky dr-orlovsky added this to the 0.26.1 milestone Apr 12, 2021
src/util/key.rs Outdated
@@ -78,6 +78,23 @@ pub struct PublicKey {
}

impl PublicKey {
/// Constructs compressed ECDSA PublicKey from the provided generic Secp256k1 public key
pub fn compressed(pk: secp256k1::PublicKey) -> PublicKey {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer just new for a compressed key (same for private)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. May be also legacy for uncompressed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For uncompressed I personally haven't a preference both uncompressed or legacy are ok

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer new and new_uncompressed

@dr-orlovsky
Copy link
Collaborator Author

@apoelstra done

@dr-orlovsky dr-orlovsky added this to In review in Taproot Apr 30, 2021
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

@dr-orlovsky, I think you probably forgot to push the changes 😛

@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 yeah, also needed rebase baeause of #589 merge

sgeisler
sgeisler previously approved these changes May 1, 2021
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

utACK 6e9d549

I think this can be merged if you have a good use case for it as-is. I wonder though if we shouldn't take a look at where these new functions could already be used in our library.

@@ -37,6 +37,23 @@ pub struct PublicKey {
}

impl PublicKey {
/// Constructs compressed ECDSA PublicKey from the provided generic Secp256k1 public key
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor nit: if it's not code, i.e. a type, I wouldn't write public key in camel case. If it's code/a type it should be formatted as such.

pub fn new(pk: secp256k1::PublicKey) -> PublicKey {
PublicKey {
compressed: true,
key: pk,
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, pk could be renamed to key which would allow to write key, instead of key: pk,.

src/util/ecdsa.rs Show resolved Hide resolved
@dr-orlovsky
Copy link
Collaborator Author

@sgeisler I checked the library, updated all places (including tests and docs) where the new API can be used. Next, as a separate commit 187eae8 I added PrivateKey::from_slice convenience method, since there were multiple use of SecretKey::from_slice with a boilerplate code around it. Additionally, I removed unnecessary map_err, which simplified readability of BIP32-related methods.

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

utACK 187eae8

Taproot automation moved this from In review to Ready for merge May 4, 2021
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACk 187eae8

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 187eae8

@apoelstra apoelstra merged commit d0fb626 into rust-bitcoin:master May 6, 2021
Taproot automation moved this from Ready for merge to Done May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor API Change This PR should get a minor version bump
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants