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

Refactor RandomCoin trait #214

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

Al-Kindi-0
Copy link
Contributor

Addresses #206

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

Look good! Thank you! I left a few comments inline. Most of them are pretty small. The only more significant one is about using nonce from an instance variable rather than passing it via parameters. But if this significantly complicates FRI verifier interface, we can keep it as is.

prover/src/channel.rs Outdated Show resolved Hide resolved
prover/src/channel.rs Outdated Show resolved Hide resolved
prover/src/lib.rs Outdated Show resolved Hide resolved
crypto/src/random/mod.rs Outdated Show resolved Hide resolved
crypto/src/random/default.rs Outdated Show resolved Hide resolved
@Nashtare
Copy link
Contributor

Nashtare commented Sep 19, 2023

The clippy error seems similar to the one we hit on plonky2. Setting manually CARGO_INCREMENTAL: 0 for the clippy run may do the trick.

Copy link
Collaborator

@irakliyk irakliyk left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

@irakliyk irakliyk merged commit de69cb6 into facebook:main Sep 20, 2023
8 of 9 checks passed
@irakliyk
Copy link
Collaborator

The clippy error seems similar to the one we hit on plonky2. Setting manually CARGO_INCREMENTAL: 0 for the clippy run may do the trick.

Thank you! This seems to have fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants