Remove to/from/as_raw_hash functions#2981
Conversation
bitcoin/src/bip152.rs
Outdated
There was a problem hiding this comment.
I don't think this helps anything. AsRef impl on Txid/Wtxid should stay but it's true that this method has weak API. I think it'd be more appropriate to have a trait for that that's only implemented for the two expected ids.
// please suggest a better name
pub trait AsTransactionIdentifier: sealed::Sealed {
fn as_identifier(&self) -> &[u8; 32];
}There was a problem hiding this comment.
If we add such a trait we should also add it as a bound on bitcoin::merkle_node::MerkleNode::Leaf since the MerkleNode trait is currently unsealed and has no bounds on its Leaf type, but it is also intended to only be about wtxid and txid.
Interesting that there are multiple places where we want to be generic over those two types..
There was a problem hiding this comment.
#2987, a sexy little PR if I don't say so myself. Good ideas crew.
hashes/src/util.rs
Outdated
There was a problem hiding this comment.
I agree with the change but not justification. I think casting raw hashes is perfectly fine. The reason to remove it is to help getting 1.0 out sooner. Hopefully one day we can add it back. At least getters.
There was a problem hiding this comment.
Personally I think that casting raw hashes should be done by to_byte_array and from_byte_array and we shouldn't have blessed "raw hash" versions.
But sure, let's default to not having them, and it's easy to add later if we agree that we should.
|
Note to self, wait till #2987 resolves then pick this back up. |
fa0eca8 to
b48d780
Compare
apoelstra
left a comment
There was a problem hiding this comment.
ACK b48d7806a963a696ca28552d819266a7c4775087
|
ACK the code but I'd prefer to modify the commit message to not be misleading. |
b48d780 to
a764d6f
Compare
I went to do this but I don't see which bit is you think is misleading sorry. A added EDIT: Oh did you mean the PR description? I changed it to match the commit log. |
|
Force push is changes to the commit log only, no code changes. |
|
What I found misleading is the claim that this has anything to do with hash newtypes not being general hashes. A The reason to still remove this is to not produce unstable API from the So the message should state this. |
|
I could be convinced either way (and it's fine to change the commit message to say "we're doing it to shrink the API to things we agree on", but I also had no idea what you meant about the original message being misleading. There is a comment thread above where you stated your case and I disagreed and there is no clear conclusion. And then the request to change the message was in a separate thread so it wasn't clear it was related. I'm often guilty of this too -- but it's worth the effort to slow down and make your position clear rather than saying things are "wrong" in an unspecified way. |
|
I guess the confusion often stems from GH not allowing commenting on commit messages. Another thing is that this PR is weird because we all want it for completely different reasons and simultaneously I think the reason stated in the message doesn't make sense. |
lol, now that is rough consensus |
In an effort to shrink the API of `hashes` remove the `from_raw_hash`, `to_raw_hash`, and `as_raw_hash` inherent functions from types created with the `hash_newtype` macro. There are a few reasons why this is favourable: - It allows stable crates to use the macro and not expose unstable `hashes` types in their API. - It makes types created with the macro less "general" in the sense that its more obscure to just hash any data into them. This allows us to write cleaner APIs in `rust-bitcoin`.
a764d6f to
2b56f76
Compare
|
Force push is commit log change (copied to PR description) only, no code changes. |
In an effort to shrink the API of
hashesremove thefrom_raw_hash,to_raw_hash, andas_raw_hashinherent functions from types created with thehash_newtypemacro.There are a few reasons why this is favourable:
hashestypes in their API.rust-bitcoin.