-
Notifications
You must be signed in to change notification settings - Fork 45
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
Move RpoRandomCoin to crate #237
Conversation
4345ab1
to
6cd6877
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few small comments inline. After these are addressed, we can merge.
src/rand/rpo.rs
Outdated
let digest = Rpo256::hash_elements(seed); | ||
let digest_elem = digest.as_elements(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we use the same approach was we use on line 68 below. For example:
let digest: Word = Rpo256::hash_elements(seed).into();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
src/rand/mod.rs
Outdated
/// Pseudo-random basefield elements generator. | ||
/// | ||
/// An instance can be used to draw, uniformly at random, basefield elements as well as `Word`s. | ||
pub trait RandFeltsGen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I like RandFeltsGen
better than Rng
. Should we just go with Rng
? Or maybe FeltRng
?
Also minor nit: on line 9 I'd probably say just: "Pseudo-random element generator."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Updated to FeltRng
and changed the comment.
Two other things for this PR:
|
There was a problem hiding this 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!
Should we rebase it from next
and merge? (this way we don't have to wait until #236 is done.
nits: minor chore: update log and readme
7bcc972
to
9ee3d87
Compare
|
Makes sense. Done! |
Thank you! |
Describe your changes
This moves the
RpoRandomCoin
to thecrypto
crate and defines a trait for generating randomFelt
andWord
on it.Checklist before requesting a review
next
according to naming convention.