Skip to content

Move type definitions of pubkey/script hash types#1914

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
tcharding:06-20-hash-froms
Aug 2, 2023
Merged

Move type definitions of pubkey/script hash types#1914
apoelstra merged 2 commits intorust-bitcoin:masterfrom
tcharding:06-20-hash-froms

Conversation

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

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

Wouldn't this be better as doctest?

@yancyribbens
Copy link
Copy Markdown
Contributor

yancyribbens commented Jun 22, 2023

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
Copy Markdown
Member Author

tcharding commented Jun 23, 2023

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

@apoelstra apoelstra Jun 23, 2023

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
Copy Markdown
Member Author

@tcharding tcharding Jun 25, 2023

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

apoelstra commented Jun 23, 2023

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

apoelstra commented Jun 23, 2023

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
Copy Markdown
Member Author

tcharding commented Jun 25, 2023

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
Copy Markdown
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
Copy Markdown
Member Author

tcharding commented Jun 26, 2023

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
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 9d9fa623e401b1e75c6720a71c6d874a5738cfbe

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jul 8, 2023

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

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

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jul 18, 2023

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

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jul 18, 2023

Pulled master and rebased, woops.

apoelstra
apoelstra previously approved these changes Jul 18, 2023
View reviewed changes
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 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
Copy Markdown
Member Author

tcharding commented Aug 2, 2023

Rebase only, no other changes.

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

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Aug 2, 2023

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