Remove user_enum macro
#1291
Remove user_enum macro
#1291
Conversation
In preparation for patching the `Network` serde impls; add a roundtrip test for `serde` (de)serializing the `Network` type.
80586bf to
de38a46
Compare
bitcoin/src/network/constants.rs
Outdated
| #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] | ||
| #[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))] | ||
| pub enum Network { | ||
| /// Classic Bitcoin (mainnet Bitcoin network). |
There was a problem hiding this comment.
Would it be enough to just say "Mainnet Bitcoin"?
There was a problem hiding this comment.
That's good with me, @apoelstra?
There was a problem hiding this comment.
Lol, yeah. Since I wrote this a fork of bitcoin core called "bitcoin classic" came out so the exsting text is needlessly confusing..
bitcoin/src/network/constants.rs
Outdated
| Signet => "signet", | ||
| Regtest => "regtest", | ||
| }; | ||
| f.pad(s) |
There was a problem hiding this comment.
I'm afraid pad will cut off letters if someone specifies too low precision.
There was a problem hiding this comment.
So I should have thought harder and used write!() like normal, right?
There was a problem hiding this comment.
For now, yes. Long term we will need some helper fn to format non-integers without losing precision.
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] | ||
| #[cfg_attr(feature = "serde", serde(rename_all = "lowercase"))] | ||
| pub enum Network { |
There was a problem hiding this comment.
🤷 "Remove user_enum macro" is the MR and commit message..
There was a problem hiding this comment.
Heads up that the "complaining about #[non_exhaustive] on Network" issue is here #2225 and we basically agreed to eliminate any direct use of the enum in the library, and then move the enum itself to another crate where it won't cause breakage (primarily because nobody will use it..) and then we can remove #[non_exhaustive].
Instead this library will receive network params by taking generic Into<Params> or AsRef<Params> arguments, and you can make your own network enum that implements this and describes exactly the set of networks that you support.
There was a problem hiding this comment.
To be fair, after all the drama introducing this change has caused I was surprised to see it come in so nonchalantly. I will admit that this should have been in a stand alone PR, the repercussions discussed, and a mention in the changelog. I accept responsibility for the lack of all three.
There was a problem hiding this comment.
I think this is one of those cases where nobody could predict how much impact it would have. It seemed totally obvious that the enum should be non_exhaustive. Well, it kinda still is obvious to me but I see how also having other shared enums helps people not write a few lines of code. Though now that I think about it, it's super ironic that people here go as far as to NIH something just to avoid dependency and they refuse to write an enum...
To be honest, there were multiple occasions where I was tempted to say "if you don't want strict API, don't use Rust". But I'm pretty sure it'd come off wrong even though I honestly believe Rust is not that useful if you want to write a quick dirty thing and great if you write something security-critical.
There was a problem hiding this comment.
Rust's lack of dependent types makes it basically impossible to write many of the APIs that we want (in particular APIs surrounding Network, e.g. addresses whose network is tied to them but which can still be parsed as the same type). Its coherence rules are constantly giving us trouble when we try to break crates apart. Rust's lack of inheritance or monkeypatching or anything also makes it impossible to NIH certain things, especially traits but sometimes enums whose job it is to provide interop. So "just write an enum" is not a solution to a lack of a usable Network enum. (But IMHO it is a solution if library crates avoid referring directly to the enum and instead use their interop with Params, since this does enable people to write their own enums.)
So if I wanted a strict API I'd use Idris or something, but Rust is definitely a language for getting work done.
Philosophy aside -- I was also caught off-guard by what a pain in the ass #[non_exhaustive] turned out to be, since it seemed unlikely that people were supporting the existing full set of networks and that they were doing so by exhaustively matching. I think my own code pretty consistently just panics on some networks.
There was a problem hiding this comment.
Rust's lack of dependent types makes it basically impossible to write many of the APIs that we want
True, unfortunately.
Its coherence rules are constantly giving us trouble when we try to break crates apart.
Sadly, this may be unsolvable. The rules protect against other problems. I was thinking about it many times but couldn't come up with a reasonable solution.
Rust's lack of inheritance
Thank God for it! IME inheritance is just a mess. Though it's true that Rust would benefit from a delegation syntax. There were multiple attempts at RFC but sadly it doesn't seem like it's going anywhere anytime soon.
So if I wanted a strict API I'd use Idris or something
I'd love to but last time I checked Idris was experimental and IDK how good performance of it is.
b9d3278 to
d4ff254
Compare
The `user_enum` macro is only used a single time. The macro includes custom serde logic which can be trivially derived instead. Remove the `user_enum` macro and just implement `Network` the old fashioned way. Doing so simplifies the code.
d4ff254 to
f429c22
Compare
|
Implemented suggestions from @Kixunil (incl. "Mainnet Bitcoin" rustdoc) |
| #[cfg(feature = "std")] | ||
| let message = format!("Unknown network (type {})", s); | ||
| #[cfg(not(feature = "std"))] | ||
| let message = "Unknown network"; |
There was a problem hiding this comment.
FTR, I plan to add some helpers to deal with this more cleanly.
f6467ac Minimize usage of Network in public API (Tobin C. Harding) 3ec5eff Add Magic::from_params (Tobin C. Harding) Pull request description: Minimize usage of the `Network` enum in the public API. See #2225 for context, and #1291 (comment) for an interpretation of that long discussion. Close: #2169 ACKs for top commit: sanket1729: reACK f6467ac. apoelstra: ACK f6467ac Tree-SHA512: f12ecd9578371b3162382a9181f7f982e4d0661915af3cfdc21516192cc4abb745e1ff452649a0862445e91232f74287f98eb7e9fc68ed1581ff1a97b7216b6a
Remove the
user_enummacro because it is only used once and is unnecessarily complicated.Patch 1: Add a preparatory unit test to make sure patch 2 maintains the current serde logic.
Patch 2: Simplify
Networkby removinguser_enum