WitnessVersion type #562
WitnessVersion type #562
Conversation
dr-orlovsky
added
the
API break
src/util/address.rs
Outdated
| impl From<WitnessVersion> for ::bech32::u5 { | ||
| /// Converts `WitnessVersion` instance into corresponding Bech32 u5-value | ||
| fn from(ver: WitnessVersion) -> Self { | ||
| ::bech32::u5::try_from_u8(ver.into_version_no()).expect("WitnessVersion is broken") |
There was a problem hiding this comment.
Maybe this error text could be more descriptive, e.g. "WitnessVersion is always in 0..=16"
src/util/address.rs
Outdated
| @@ -168,22 +337,19 @@ impl Payload { | |||
| /// Get a [Payload] from an output script (scriptPubkey). | |||
| pub fn from_script(script: &script::Script) -> Option<Payload> { | |||
| Some(if script.is_p2pkh() { | |||
| Payload::PubkeyHash(PubkeyHash::from_slice(&script.as_bytes()[3..23]).unwrap()) | |||
| Payload::PubkeyHash(PubkeyHash::from_slice(&script.as_bytes()[3..23]).expect("hash engine broken")) | |||
There was a problem hiding this comment.
The failure has nothing to do with the hash engine but with supplying a byte slice of a wrong length.
There was a problem hiding this comment.
Removed expect at all by using infallible from_inner
| } | ||
|
|
||
|
|
||
| impl FromStr for WitnessVersion { |
There was a problem hiding this comment.
Is this actually useful? Where do we directly (de)serialize a witness version? Isn't it always part of some other data structure with its own encoding?
There was a problem hiding this comment.
I could imagine it being useful somewhere, and it's not much extra code.
There was a problem hiding this comment.
Yeah, it's not too bad now that I noticed even without it the UnparsableWitnessVersion error variant can't be made more expressive.
src/util/address.rs
Outdated
| } else if script.is_witness_program() { | ||
| // We can unwrap the u5 check and assume script length | ||
| // We can unwrap assume script length |
There was a problem hiding this comment.
I think the and has to stay in the comment for it to make any sense.
|
Fixed @sgeisler suggestions & force-pushed. Ready for re-review |
|
Does this really still have to be its own PR? Afaik it's included in #563 (idk if based on an up to date version) and fits in there quite well. It's a breaking change anyway. |
|
I have left some feedback on #563 for this commit. |
|
Updated with review suggestions from #563 |
|
Again, would anyone be opposed to closing this in favor of merging it with #563? It's just so related and reviewing #563 together with the #562 changes seemed easy enough. #563 is also close to merge ready imo, maybe a third review would be nice and we need to decide if we want to release 0.26.1 before that, but there's nothing fundamentally blocking just merging #563 all at once after that. |
|
Agree, closing this now |
|
Reopened as #617 |
…gates to bitcoin_hashes 1d6a46e change bitcoin-hashes feature gates to bitcoin_hashes (Andrew Poelstra) Pull request description: This leaves `bitcoin-hashes` in place everywhere in the documentation, but changes it to `bitcoin_hashes` on the actual feature gates, to ensure that both flags work. It's unfortunately a bit hard to test this because the library will be self-consistent pretty-much no matter what we do ... the issue is if the user enables the wrong flag we want to make sure that all the APIs we intend to be visible are actually visible. Fixes rust-bitcoin#562. ACKs for top commit: tcharding: ACK 1d6a46e Tree-SHA512: 1e060aeb2810ef1e23cf5c956023aca586550ba0287bbf7bc1108dc14091e17d7601aac3f057d0313fafd21351cdda1b08b4250f34ecde917be686d0b739e65a
Part of Taproot epic (should be listed in #503)