Fix broken serde::Deserialize and FromStr impl of keyPair
#492
Conversation
elsirion
force-pushed
the
2022-10-serde-fix
branch
from
6210a93 to
9ca7362
Compare
|
When Maybe we don't want to do that -- but disabling this at compile-time is a breaking change, so I'd rather first merge the non-breaking version (and backport it to several other versions) first, so that we can do a minor release. We can later remove the inefficient version if that's what we decide to do. |
elsirion
force-pushed
the
2022-10-serde-fix
branch
from
9ca7362 to
f6bfdf9
Compare
|
I also just noticed that my PR #379 (adding a serde module for |
elsirion
force-pushed
the
2022-10-serde-fix
branch
from
f6bfdf9 to
670a48b
Compare
src/key.rs
Outdated
| #[cfg(feature = "global-context")] | ||
| let ctx = SECP256K1; | ||
|
|
||
| #[cfg(not(feature = "global-context"))] | ||
| let ctx = Secp256k1::signing_only(); |
There was a problem hiding this comment.
I wish I could deduplicate that code, but the global context is of type Secp256k1<All> while we apparently only need a signing context, so I can't easily build a function returning Cow<'static, Secp256k1<_>> (it would change its signature depending on the feature flags which is bad imo).
How much slower is creating an All context?
There was a problem hiding this comment.
I don't understand the question. What code needs to be deduplicated?
There was a problem hiding this comment.
The code highlighted with the comment which initializes cfg depending on the feature set. At first I had a get_context() -> Cow<'static, Secp256k1<Signing>> function for that till I noticed the global context type is incompatible.
It's not a big deal, I just noticed that I always had to fix little problems in both FromStr and Deserialize.
elsirion
force-pushed
the
2022-10-serde-fix
branch
2 times, most recently
from
e5cc609 to
9d5e477
Compare
|
Could you add a unit test for the serde impl as well? |
elsirion
force-pushed
the
2022-10-serde-fix
branch
2 times, most recently
from
8064ed5 to
0451abf
Compare
src/key.rs
Outdated
| @@ -1053,13 +1053,18 @@ impl<'a> From<&'a KeyPair> for PublicKey { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(feature = "alloc")] | |||
There was a problem hiding this comment.
The alloc requirement due to calling Secp256k1::signing_only() is a bit annoying, but better than the global-context one
There was a problem hiding this comment.
I wonder if we should have a non-alloc non-global-context version that just returns an error 100% of the time.
My goal here is really just that we don't break compilation for anybody, while replacing the current "always aborts the process" code with something else...I don't care too much what else since the current behavior is so bad :)
There was a problem hiding this comment.
The code has gotten a bit hairy, but I think it should have the desired behavior now.
elsirion
force-pushed
the
2022-10-serde-fix
branch
2 times, most recently
from
a7186ca to
73990f2
Compare
|
Lol! Ok, I'll accept this. I think we should return an actual error rather than panicking, but a panic is certainly better than an abort. |
|
Actually, could you change the panic message to include "please enable the |
elsirion
force-pushed
the
2022-10-serde-fix
branch
from
73990f2 to
53c1354
Compare
|
Added the hint to turn on |
Panicking re-creates the existing behavior. If you actually want to have a proper error I think that should work too (with some more |
|
@elsirion sorry, one more ask -- can you add a commit which bumps the minor version number of this crate and adds a CHANGELOG entry for this? |
We have fixed a bug in `KeyPair` decoding and otherwise didn't change the API.
Fixes #491