Add secp256r1 curve
#34
Conversation
There was a problem hiding this comment.
LGTM! Thanks for this work and apologies for the delay on the review..
Could @davidnevadoc or @kilic take another look to this prior to merging?
src/bn256/fq.rs
Outdated
| // TODO: Can we simply put 0 here:: | ||
| const ROOT_OF_UNITY: Fq = Fq::zero(); |
There was a problem hiding this comment.
I don't think this is correct. We should actually look for the correct root. (I belive we already saw in Vietnam there's only 1).
There was a problem hiding this comment.
Updated! @CPerezz can we try re-running the tests? They all pass locally.
|
Hey @rrrliu have you made sure the PR is up-to-date with upstream?? Also, there seems to be some issues related to the version of the |
|
Sure! I'll have a look once the failing checks are fixed :) |
|
Just rebased main and fixed tests! |
|
Some tests are still failing @rrrliu. I'll review once those are fixed 👍 |
|
I accidentally merged this. And somehow I can't seem to bring it back.. I probably screwed something up while editing the PR. |
Adds new directory
secp256r1that adds the NIST P-256 curve and its relevant arithmetic operations.To do this, also modified the macros in
src/derive/curve.rsto account for curves wherea ≠ 0. Becausebn256andsecp256k1both havea = 0, the elliptic curve operations (add(),double(),is_on_curve(), etc) used calculations that made this assumption. Sincesecp256r1hasa = 0xfff...fffc, these operations needed to be refactored.Tests mirror those of the
secp256k1curve. Additionally added a test that checks the ZETA value of the curve.Thank you to @CPerezz for helping me debug the tests and @enricobottazzi for investigating this library!