make dependency secp256k1 optional, fix #391 #441
Conversation
yihuang
force-pushed
the
optional-secp256k1
branch
from
e250426 to
48d60e2
Compare
Codecov Report
@@ Coverage Diff @@
## master #441 +/- ##
========================================
- Coverage 30.9% 30.9% -0.1%
========================================
Files 132 132
Lines 5502 5504 +2
Branches 1710 1711 +1
========================================
Hits 1701 1701
- Misses 2605 2607 +2
Partials 1196 1196
Continue to review full report at Codecov.
|
| @@ -3,7 +3,9 @@ | |||
| use crate::error::{Error, Kind}; | |||
| use anomaly::fail; | |||
| use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer}; | |||
| use signatory::{ecdsa::curve::secp256k1, ed25519}; | |||
| #[cfg(feature = "secp256k1")] | |||
There was a problem hiding this comment.
If we moved everything secp-related into a separate file/module, we can likely just annotate it on the mod level instead? Not blocking this PR but probably worth separating this out (if possible)?
There was a problem hiding this comment.
Good stuff!
I think this is fine as an initial step towards not only improving the dependency tree for the main use case (ie. using only Ed25519 keys), but also towards improving the API in general for this main use case. Perhaps, as @liamsi suggested, by moving stuff around if possible, or by exposing a slightly different API when the secp256k1 feature is enabled.
As mentioned inline, another thing that we should imho look into is the presence of supposedly unreachable cases (_ => unreachable!()), which may not be when the secp256k1 feature is enabled. But since this PR improves on the previous behavior by making those actually unreachable when the feature is disabled, it's not a blocker.
👍 from me in the meantime, we can discuss further improvements in a follow-up issue.
| @@ -49,6 +53,7 @@ impl PublicKey { | |||
|
|
|||
| /// Get Ed25519 public key | |||
| pub fn ed25519(self) -> Option<ed25519::PublicKey> { | |||
| #[allow(unreachable_patterns)] | |||
There was a problem hiding this comment.
I understand that those _ => unreachable!() patterns were here previously to this PR, but just wanted to mention that those seem inherently unsafe to me (as in: may lead to panics). This PR improves the state of things by making them actually unreachable if the secp265k1 feature is not enabled (hence the need for the annotation), but we should perhaps take a closer look at them in a follow-up issue.
fix #391