Skip to content

Conversation

@tarcieri
Copy link
Member

@tarcieri tarcieri commented Feb 1, 2023

Adds tests for the TWO_INV, ROOT_OF_UNITY, and ROOT_OF_UNITY_INV constants, based on the tests generated via ff_derive.

Some of them are currently failing and have been annotated with #[ignore] and a TODO comment.

Adds tests for the `TWO_INV`, `ROOT_OF_UNITY`, and `ROOT_OF_UNITY_INV`
constants, based on the tests generated via `ff_derive`.

Some of them are currently failing and have been annotated with
`#[ignore]` and a TODO comment.
@tarcieri tarcieri requested review from fjarri and str4d February 1, 2023 22:33
@tarcieri
Copy link
Member Author

tarcieri commented Feb 1, 2023

Note: they're copy-pasted for now, but once I get these all working (including DELTA) I can extract them into elliptic-curve::dev as a macro or something

@fjarri
Copy link
Contributor

fjarri commented Feb 1, 2023

I think there are a few tests missing here:

  • MULTIPLICATIVE_GENERATOR ^ ((modulus - 1) >> S) == ROOT_OF_UNITY
  • MULTIPLICATIVE_GENERATOR ^ (2^S) == DELTA

@tarcieri
Copy link
Member Author

tarcieri commented Feb 2, 2023

Yeah as implied in the comment above yours the DELTA constants aren't defined yet

@tarcieri
Copy link
Member Author

tarcieri commented Feb 2, 2023

Whoa, just hit an ICE on stable Rust (looks like while compiling the dependencies):

https://github.com/RustCrypto/elliptic-curves/actions/runs/4078778175/jobs/7029397528

Edit: filed an issue rust-lang/rust#107613

@tarcieri
Copy link
Member Author

tarcieri commented Feb 3, 2023

Okay, after an ICE and failures possibly related to GitHub rewriting OS package tarballs I'm going to land this while it's green.

Will backfill a few additional tests and try to consolidate the definitions of the test into a reusable macro.

@tarcieri tarcieri merged commit 231fcaf into master Feb 3, 2023
@tarcieri tarcieri deleted the k256+p256+p384/add-constant-tests branch February 3, 2023 00:11
This was referenced Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants