Add non_exhaustive compiler directive to AddressType
#1011
Conversation
| @@ -139,6 +139,7 @@ impl From<bech32::Error> for Error { | |||
|
|
|||
| /// The different types of addresses. | |||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| #[non_exhaustive] | |||
There was a problem hiding this comment.
Hmm, I'm a little dubious of this - I can imagine matching on this and doing...something, eg maybe I want to know if an address is a witness and map every type to a bool or something, but with non_exhaustive now you have to _ => ...panic, I guess?. There are definitely reasons to want forward compat without breaking downstream, but for "we added a new address type" kinda things is so rare that I'm not sure we need to worry about that breakage, IMO.
There was a problem hiding this comment.
Thanks for the review. I'm not totally sure of the merit of this PR myself, whether we should use non_exhaustive, or whether we should use it but on some different set of types. There has been multiple folks speak up in the past positively about non_exhaustive, the intention of this PR is to bring out all the opinions and then I'll just try to shepherd them in. As such, I'll leave this sitting open for a while and see what comes of it.
There was a problem hiding this comment.
There are much better ways to detect whether an address is witness (or something else) than matching each type. I think bumping the major version just because there's a new soft fork is annoying. The issue with this library is that pretty much any Bitcoin application written in Rust uses it, so it's a shitton of breakage.
IMO any public enum and struct that could be possibly extended in the future should be #[non_exhaustive]
There was a problem hiding this comment.
I mean philosophically I agree with you, it'd be cool to have the library not have ballot changes for soft forks. In practice, though, that just makes the downstream code that would have broken have panics that they may hit instead, which isn't an improvement. More generally, Bitcoin does soft forks really really rarely, I'm not sure we need to worry too much about changing a few enums when a new soft fork happens - we probably have to change a ton of fields to add support for it anyway.
There was a problem hiding this comment.
I tend to agree with @Kixunil here ... my feeling is that matching on these enums is a bit of a weird thing to do, and if you are doing this, you ought to have an error path for the case that some unsupported softfork has happened. (Or in some cases perhaps you could do some sane default.)
On the other hand, soft forks in Bitcoin are really rare, so this is something of an academic discussion. Maybe the time to discuss this is whenever the next fork actually seems to be happening? (My guess is that the next fork will be addition of some covenant opcodes, which won't even bump any version numbers, though it could also be APO..)
There was a problem hiding this comment.
The problem is it's not "just updating a few enums". Due to breaking change there needs to be a coordinated major version bump across all ecosystem. This is a huge library and it is the only (rich) Bitcoin library available in Rust today, so pretty much every Bitcoin project written in Rust depends on it. Perhaps if Address was a separate crate it'd be less painful but I guess it'd still be significant.
This is my main motivation behind trying to make the library stable as you might have noticed from my efforts.
Maybe the time to discuss this is whenever the next fork actually seems to be happening?
I don't think so, if at that time we have 1.0 it's already too late. At the latest we need to decide before making it 1.0.
There was a problem hiding this comment.
As others have mentioned, I agree with #[non-exhuastive] for AddressType. There are known issues with many exchanges not allowing to send coins to future addresses. Adding [non-exhuastive] generally should help in these cases.
but the other two are not needed as they cannot be extended in the future.
We have to carefully reason about each case if they are really needed or not.
src/util/address.rs
Outdated
| @@ -358,6 +359,7 @@ impl From<WitnessVersion> for opcodes::All { | |||
|
|
|||
| /// The method used to produce an address. | |||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| #[non_exhaustive] | |||
There was a problem hiding this comment.
This, I am not sure is required. This should cover all the cases of interest that could possibly be extended in the future.
src/util/taproot.rs
Outdated
| @@ -870,6 +870,7 @@ impl fmt::UpperHex for FutureLeafVersion { | |||
|
|
|||
| /// The leaf version for tapleafs. | |||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| #[non_exhaustive] | |||
There was a problem hiding this comment.
This is not required. We have already modelled the leaf versions using FutureLeafVersion
Currently adding variants to enums is a breaking change. In an effort to reduce the upgrade burden on users we can use the `non_exhaustive` compiler directive so that adding a new variant does not cause downstream code to break. Add `non_exhaustive` to the `AddressType` since it may be extended in the future.
tcharding
force-pushed
the
05-26-non-exhaustive
branch
from
ecca766
to
43b684b
Compare
tcharding
changed the title
AddressType
|
Changes in force push:
|
|
Glad to see we all agree! |
…ective to `AddressType` 43b684b Add non_exhaustive compiler directive to AddressType (Tobin C. Harding) Pull request description: Add non_exhaustive compiler directive to AddressType Currently adding variants to enums is a breaking change. In an effort to reduce the upgrade burden on users we can use the `non_exhaustive` compiler directive so that adding a new variant does not cause downstream code to break. Add `non_exhaustive` to the `AddressType` since it may be extended in the future. ACKs for top commit: sanket1729: ACK 43b684b Kixunil: ACK 43b684b apoelstra: ACK 43b684b Tree-SHA512: 2b2a15fb501d23058acca94318776ffcccedf463d43d07afa290fba46a7bd58b3a730f6e1f25605ef399afcfdb5de4c7ad67eaa0adff0ba39b0096cbcec10f57
Add non_exhaustive compiler directive to AddressType
Currently adding variants to enums is a breaking change. In an effort to
reduce the upgrade burden on users we can use the
non_exhaustivecompiler directive so that adding a new variant does not cause
downstream code to break.
Add
non_exhaustiveto theAddressTypesince it may be extended inthe future.