WitnessVersion type#562
WitnessVersion type#562dr-orlovsky wants to merge 2 commits intorust-bitcoin:masterfrom BP-WG:feat/witness-version
Conversation
sgeisler
left a comment
There was a problem hiding this comment.
Looks good and improves the API quite a bit imo. I left a few nits regarding some expects and comments.
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
| 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
| Payload::ScriptHash(ScriptHash::from_slice(&script.as_bytes()[2..22]).expect("hash engine broken")) | ||
| } 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 |
Part of Taproot epic (should be listed in #503)