8000 Move type definitions of pubkey/script hash types by tcharding · Pull Request #1914 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move type definitions of pubkey/script hash types #1914

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 2 commits into from
Aug 2, 2023
Merged

Move type definitions of pubkey/script hash types #1914

merged 2 commits into from
Aug 2, 2023

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented Jun 20, 2023 •
edited
Loading

Total re-write since review. Now this PR moves the hash type definitions out of hash_types. Please see #1909 (comment) for more.

No longer adds unit tests.

Fix: #1909

apoelstra
apoelstra previously approved these changes Jun 21, 2023
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 ee33eefe04ef9aa809904b4c42c41f9cf2caf647

Kixunil
Kixunil reviewed Jun 22, 2023
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.

Wouldn't this be better as doctest?

@yancyribbens
Copy link
Contributor

Also From is defined in key.rs so the test could go there? I don't really understand why the impl PubkeyHash and the struct are in different files along with the traits.

@tcharding tcharding mentioned this pull request Jun 23, 2023
Closed
@tcharding tcharding changed the title Add unit tests verifying From impls for hashes Move type definitions of pubkey/script hash types Jun 23, 2023
@tcharding
Copy link
Member Author

I don't really understand why the impl PubkeyHash and the struct are in different files along with the traits.

BOOM! Thanks man.

apoelstra
apoelstra reviewed Jun 23, 2023
View reviewed changes
bitcoin/src/crypto/key.rs Outdated
@@ -116,8 +116,7 @@ impl PublicKey {
hash160::Hash::hash(&self.inner.serialize()).to_byte_array(),
))
} else {
// We can't create witness pubkey hashes for an uncompressed
// public keys
// We can't create witness pubkey hash for an uncompressed pubkey.
Copy link
Member

Choose a reason for hiding this comment

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

In 76f9ab2df76d5a65ade52c339d51a2999b8446ab

Should be "a witness pubkey hash".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.

@apoelstra
Copy link
Member

You say new does not make sense for Script since it implies ownership, but both Path and PathBuf have new constructors (and neither have empty one).

I think we should use the name new for both.

@apoelstra
Copy link
Member

ack b798939fde1d9890a93d41abe31eaf23e4a5356e other than the new/empty thing. Also, I'm unconvinced that moving the hash types is a breaking change, but it's fine, since the Script constructor rename definitely will be.

@tcharding
Copy link
Member Author

Will fix emty/new thing as suggested.

I'm unconvinced that moving the hash types is a breaking change

Well after this one can no longer write use bitcoin::hash_types::PubkeyHash and before this they can. That is a breaking change, right? IIRC I mentioned it because I intentionally did not add a re-export in hash_types since we are moving loads of things out of there.

@tcharding
Copy link
Member Author
tcharding commented Jun 25, 2023 •
edited
Loading

I just realized the new/empty change should not even be in this PR, its totally unrelated to pubkey/script hash - will pull it out into a separate PR.

#1925

@tcharding tcharding force-pushed the 06-20-hash-froms branch 2 times, most recently from 921b2fe to 9d9fa62 Compare June 26, 2023 01:43
@tcharding
Copy link
Member Author

Added a patch 9216ab11 Move re-export of newtypes::* to stop rustfmt messing with the code layout.

apoelstra
apoelstra approved these changes Jun 27, 2023
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 9d9fa623e401b1e75c6720a71c6d874a5738cfbe

@tcharding
Copy link
Member Author

Rebased on master (to pick up #1927). No other changes.

apoelstra
apoelstra previously approved these changes Jul 11, 2023
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 dcfbafd75572801e1fd8582c165a3cf956b0d80b

@tcharding
Copy link
Member Author

Removed the first two patches (because we now have #1925) and rebased.

@tcharding
Copy link
Member Author

Pulled master and rebased, woops.

apoelstra
apoelstra previously approved these changes Jul 18, 2023
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 10a95a6cf98c97f397affedfb44f1bdaca1b2178

@tcharding tcharding mentioned this pull request Aug 1, 2023
Merged
@tcharding
Improve PubkeyHash and WPubkeyHash
2197f13 
Improve the pubkey hash types by doing:

- Define the types in the `crypto::key` module
- Add From<&PublicKey> impl for `PubkeyHash`

Keep the current crate level re-export so this does not impact the
public API _if_ people are using the re-export but is still a breaking
change.
@tcharding
Improve the ScriptHash and WScriptHash types
27b3c1e 
Improve the script hash types by doing:

- Define the types in the `crypto::script` module
- Put the From impls directly below the type definitions

Keep the current crate level re-export so this does not impact the
public API _if_ people are using the re-export but is still a breaking
change.
@tcharding
Copy link
Member Author

Rebase only, no other changes.

apoelstra
apoelstra approved these changes Aug 2, 2023
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 27b3c1e

@apoelstra
Copy link
Member

Merging based on #1945. The rebase made no changes.

@apoelstra apoelstra merged commit f994595 into rust-bitcoin:master Aug 2, 2023
@tcharding tcharding deleted the 06-20-hash-froms branch August 3, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hash newtypes that only hash one type of data should have a From impl
4 participants
@tcharding @yancyribbens @apoelstra @Kixunil
0