Skip to content

Rename inner key field in PrivateKey and PublicKey#762

Merged
sanket1729 merged 1 commit intorust-bitcoin:masterfrom
BP-WG:key/inner
Jan 11, 2022
Merged

Rename inner key field in PrivateKey and PublicKey#762
sanket1729 merged 1 commit intorust-bitcoin:masterfrom
BP-WG:key/inner

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky commented Jan 9, 2022

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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Rebased

Kixunil
Kixunil previously approved these changes Jan 10, 2022
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

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

@apoelstra
Copy link
Copy Markdown
Member

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

@sanket1729
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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
sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 6, 2022
sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 6, 2022
sander2 added a commit to sander2/polkabtc-clients that referenced this pull request Oct 7, 2022
moonman889 added a commit to moonman889/interbtc-clients that referenced this pull request Sep 15, 2025
neon-rider578 added a commit to neon-rider578/interbtc-clients that referenced this pull request Sep 30, 2025
stack-sage7291 added a commit to stack-sage7291/interbtc-clients that referenced this pull request Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

PSBT key vs bitcoin key naming confusion

4 participants