-
Notifications
You must be signed in to change notification settings - Fork 672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use KnowHrp instead of Network #2387
Conversation
We have a bunch of functions that take `Network` when what they really want is something that can be converted to a `KnownHrp`. Make `KnownHrp` public and accept `impl Into<KnownHrp>`.
Pull Request Test Coverage Report for Build 7621632788
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 20a5f1f
@@ -874,7 +875,7 @@ mod tests { | |||
let key = "033bc8c83c52df5712229a2f72206d90192366c36428cb0c12b6af98324d97bfbc" | |||
.parse::<CompressedPublicKey>() | |||
.unwrap(); | |||
let addr = Address::p2wpkh(&key, Bitcoin); | |||
let addr = Address::p2wpkh(&key, KnownHrp::Mainnet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO these tests could've stayed but whatever...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you are right. I did the change before I used impl Into
.
Note that I've just realized we'll have to make |
HRPs are an integral part of bech32. Where else could they belong to? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 20a5f1f
Something like |
As discussed rather extensively on #2225 I don't think we should do this. Users being forced to move between two different |
This has nothing to do with #2225 but funnily enough it adds argument for application-specific enums.
|
That, rather, implies |
Yes, it's a bug in signet but we're already screwed and have to live with what we have. The other possibilities are remove |
We have a bunch of functions that take
Network
when what they really want is something that can be converted to aKnownHrp
.Make
KnownHrp
public and acceptimpl Into<KnownHrp>
.