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.

Sign up for GitHub

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

Jump to bottom

implement From<PublicKey> for PubkeyHash #1248

Merged
merged 1 commit into from Sep 9, 2022
Merged

implement From<PublicKey> for PubkeyHash #1248

merged 1 commit into from Sep 9, 2022

Conversation

nlanson
Copy link
Contributor

@nlanson nlanson commented Sep 8, 2022 •
edited

Part of #1245.

Do we also want a method on PubkeyHash or is the From impl sufficient?

@tcharding
Copy link
Member

tcharding commented Sep 8, 2022 •
edited

Do we also want a method on PubkeyHash or is the From impl sufficient?

IMO the From impl is sufficient. I would personally have left the logic in pubkey_hash and called key.pubkey_hash() from the From impl. It is more uniform with wpubkey_hash.

Kixunil
Kixunil previously approved these changes Sep 8, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 509a036

But would prefer calling pubkey_hash from the From impl rather than the other way around.

@Kixunil Kixunil added minor API Change This PR should get a minor version bump trivial Obvious, easy and quick to review (few lines or doc-only...) one ack PRs that have one ACK, so one more can progress them labels Sep 8, 2022
@apoelstra
Copy link
Member

Agree with the above comments, but I'm happy with this. Thanks!

apoelstra
apoelstra previously approved these changes Sep 8, 2022
View reviewed changes
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 509a036

@apoelstra
Copy link
Member

Holding off merging pending discussion in #1245.

@nlanson nlanson dismissed stale reviews from apoelstra and Kixunil via 81f81ca September 9, 2022 01:01
@nlanson
Copy link
Contributor Author

nlanson commented Sep 9, 2022

Moved logic back into pubkey_hash()

tcharding
tcharding approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 81f81ca

Kixunil
Kixunil approved these changes Sep 9, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 81f81ca

sanket1729
sanket1729 approved these changes Sep 9, 2022
View reviewed changes
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 81f81ca

apoelstra
apoelstra approved these changes Sep 9, 2022
View reviewed changes
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 81f81ca

@apoelstra apoelstra merged commit 4b662f8 into rust-bitcoin:master Sep 9, 2022
@tcharding tcharding added this to the 0.30.0 milestone Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
minor API Change This PR should get a minor version bump one ack PRs that have one ACK, so one more can progress them trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nlanson @tcharding @apoelstra @Kixunil @sanket1729