Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

[Discussion Needed] Allow constructing Hashes from underlying byte array#20

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
dongcarl:2018-12-from-inner
May 1, 2019
Merged

[Discussion Needed] Allow constructing Hashes from underlying byte array#20
apoelstra merged 1 commit intorust-bitcoin:masterfrom
dongcarl:2018-12-from-inner

Conversation

@dongcarl
Copy link
Copy Markdown
Member

With #14 merged, there is no longer a direct way to construct Hashes from an underlying byte array. This reintroduces a way to do so. This also allows for a more efficient implementation of impl<D: Decoder> Decodable<D> for sha256d::Hash.

Needs much discussion because of concerns brought up in #10.

@TheBlueMatt
Copy link
Copy Markdown
Member

I'd personally vote for removing *::Hash::from_slice and skipping this, but that implies that rust-bitcoin will need to keep its Sha256dHash (at least) type, but I think most of the places rust-bitcoin would need to use that would never care about, eg, converting it to a secp private key, and maybe we can rename the type to something more descriptive given it may literally just be used for Bitcoin object hashes (ie block hash, txid...).

@apoelstra
Copy link
Copy Markdown
Member

I think with associated constants we can make this more generic now.

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.

I don't know what I meant by "more generic" above. This is fine.

@apoelstra
Copy link
Copy Markdown
Member

I also think that having a dedicated method should make this reasonably hard to use by accident/incorrectly, unlike From which is supposed to be for borderline-free conversions and kinda encourages YOLO usage.

@apoelstra apoelstra merged commit dcc64b3 into rust-bitcoin:master May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants