10000 Rename inner key field in PrivateKey and PublicKey by dr-orlovsky · Pull Request #762 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rename inner key field in PrivateKey and PublicKey #762

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
Merged
merged 1 commit into from
Jan 11, 2022
Merged

Rename inner key field in PrivateKey and PublicKey #762

merged 1 commit into from
Jan 11, 2022

Conversation

dr-orlovsky
Copy link
Collaborator
@dr-orlovsky dr-orlovsky commented Jan 9, 2022 •
edited
Loading

Since we already broke all possible key-related APIs with this release, I think this one is good to have with 0.28.

Closes #532

@dr-orlovsky dr-orlovsky added API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...) labels Jan 9, 2022
@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Jan 9, 2022
sanket1729
sanket1729 previously approved these changes Jan 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.

ffe7ecc47ef6145e11c166cff155afc2acb81bfb. The error is not as bad downstream. Should not be hard for people to debug.

   |
61 |     println!("{:?}", public_key.key);
   |                                 ^^^ unknown field
   |
   = note: available fields are: `compressed`, `inner`

Kixunil
Kixunil previously approved these changes Jan 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.

I'm not certain doing this is a good idea (was avoiding inner word lately) but

ACK ffe7ecc47ef6145e11c166cff155afc2acb81bfb

the code itself. Do as you wish.

@dr-orlovsky dr-orlovsky dismissed stale reviews from Kixunil and sanket1729 via b7ed666 January 10, 2022 08:20
@dr-orlovsky
Copy link
Collaborator Author

Rebased

Kixunil
Kixunil previously approved these changes Jan 10, 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 b7ed6669eba733750a4c8b4ec75f22aa1cdacebf

@dr-orlovsky dr-orlovsky requested a review from apoelstra January 10, 2022 10:11
apoelstra
apoelstra previously approved these changes Jan 10, 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 b7ed6669eba733750a4c8b4ec75f22aa1cdacebf

I like this. Typing key.key everywhere was pretty confusing.

@dr-orlovsky
Copy link
Collaborator Author

Hm, is this ready for the merge? Do not want to merge my own PRs by myself.

@apoelstra
Copy link
Member

Let's wait for one more ACK since @Kixunil was so hesitant.

@sanket1729
Copy link
Member

This will conflict with many of the open PRs. Should we first merge this or merge this as the last PR before the release? What do you think @dr-orlovsky

sanket1729
sanket1729 previously approved these changes Jan 11, 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 ffe7ecc

@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 lets merge them first, they were much harder and longer to review

@dr-orlovsky dr-orlovsky dismissed stale reviews from sanket1729, apoelstra, and Kixunil via eb09019 January 11, 2022 07:40
@dr-orlovsky
Copy link
Collaborator Author

Now, let's merge this one, after which I will re-base both #696 and #757, which have just a single ACK and have to be re-based anyway now.

@apoelstra @sanket1729 @Kixunil pls re-ack

@dr-orlovsky dr-orlovsky mentioned this pull request Jan 11, 2022
Merged
sanket1729
sanket1729 approved these changes Jan 11, 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.

utACK eb09019

Kixunil
Kixunil approved these changes Jan 11, 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 eb09019

@sanket1729 sanket1729 merged commit 06234a8 into rust-bitcoin:master Jan 11, 2022
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 15, 2022
Closed
sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 6, 2022
@sander2
chore: migration: private_key.key -> private_key.inner
8ec53d3 
see rust-bitcoin/rust-bitcoin#762
sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 6, 2022
@sander2
chore: migration: private_key.key -> private_key.inner
1a5eb57 
see rust-bitcoin/rust-bitcoin#762
sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 7, 2022
@sander2
chore: migration: private_key.key -> private_key.inner
7c6b592 
see rust-bitcoin/rust-bitcoin#762
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

@sanket1729 sanket1729 sanket1729 approved these changes

@apoelstra apoelstra apoelstra left review comments

Assignees
No one assigned
Labels
API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.

PSBT key vs bitcoin key naming confusion
4 participants
@dr-orlovsky @apoelstra @sanket1729 @Kixunil
0