Trust-Machines / p256k1

Wrappers around secp256k1 to expose the underlying math, specifically unwrapped points and scalars with multiexponentiation
Apache License 2.0
12 stars 6 forks source link

feat: Implement missing common traits for public types #79

Closed netrome closed 7 months ago

netrome commented 7 months ago

This PR implements common traits for public types following the recommendations in https://rust-lang.github.io/api-guidelines/

Note that many of the underlying operations are quite inefficient. The alternative would be to have varying behavior on different platforms, which wouldn't be an issue for PartialEq, Eq and Hash, but for PartialOrd and Ord this could lead to bugs in client code expecting ordering to be consistent.

I chose to be slow and consistent for the trait implementations in line with how it's done in secp256k1_sys. Interestingly, that crate provides helper functions like PublicKey::cmp_fast_unstable for the more performant implementations - which is something we also could do if the need arises.

cloudflare-workers-and-pages[bot] commented 7 months ago

Deploying p256k1 with  Cloudflare Pages  Cloudflare Pages

Latest commit: ea90682
Status: ✅  Deploy successful!
Preview URL: https://d3749482.p256k1.pages.dev
Branch Preview URL: https://78-implement-equality-operat.p256k1.pages.dev

View logs

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 80.32787% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 62.62%. Comparing base (b471b7d) to head (ea90682).

Files Patch % Lines
p256k1/src/keys.rs 88.57% 4 Missing :warning:
p256k1/src/ecdsa.rs 75.00% 2 Missing :warning:
p256k1/src/errors.rs 0.00% 2 Missing :warning:
p256k1/src/field.rs 75.00% 2 Missing :warning:
p256k1/src/schnorr.rs 75.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #79 +/- ## ========================================== + Coverage 61.40% 62.62% +1.21% ========================================== Files 13 13 Lines 938 998 +60 ========================================== + Hits 576 625 +49 - Misses 362 373 +11 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

netrome commented 7 months ago

LGTM. Was there a reason why you only implemented the ordering traits for field::Element, and not Scalar?

Yes, field elements have an upstream cmp function in libsecp256k1 (secp256k1_fe_cmp_var), but I did not find an equivalent for Scalar. Although writing this I realize that perhaps Scalar has a consistent representation across platforms, in which case we could write a safe ordering function or just use secp256k1_memcmp_var for the comparison.

But I'm not entirely sure that's correct, so let's save it as a follow-up if the need arises. The ordering traits are less important than Hash and Eq imo.