Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Least authority remediation #40

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

trbritt
Copy link
Collaborator

@trbritt trbritt commented Nov 5, 2024

This request addresses a few key concerns highlighted by the audit. These involve test coverage of edge cases in the higher level field extensions, an improved method for generating random elements in $\mathbb{G}_1$ and $\mathbb{G}_2$, as well as clarifying miscallaneous typos, incorrect references in the comments, etc. Additionally, suggested improvements have been included. See the official response to the audit for more details.

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 94.89362% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/secrets.rs 93.20% 17 Missing ⚠️
src/fields/fp6.rs 92.59% 2 Missing ⚠️
src/groups/mod.rs 96.96% 2 Missing ⚠️
src/lib.rs 94.11% 2 Missing ⚠️
src/fields/fp12.rs 95.65% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@trbritt trbritt marked this pull request as ready for review November 11, 2024 18:36
Copy link
Contributor

@merolish merolish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as various corrections and changes go, nothing stands out to me as an issue.

Comment on lines +190 to +192
let mut retval = [F::zero(); N];
retval[0] = F::default();
Self::new(&retval)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the change here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +609 to +610
let a = Fp12::rand(&mut OsRng);
let b = Fp12::rand(&mut OsRng);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They'd prefer nondeterministic unit tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they want fuzz/property tests. This is correct IMO.

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