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

Move RpoRandomCoin to crate #237

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Move RpoRandomCoin to crate #237

merged 1 commit into from
Dec 19, 2023

Conversation

Al-Kindi-0
Copy link
Collaborator

Describe your changes

This moves the RpoRandomCoin to the crypto crate and defines a trait for generating random Felt and Word on it.

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@Al-Kindi-0 Al-Kindi-0 requested a review from bobbinth December 18, 2023 11:35
@bobbinth bobbinth changed the base branch from next to al-new-padding-rule December 19, 2023 01:27
Copy link
Contributor

@bobbinth bobbinth left a 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
Comment on lines 50 to 51
let digest = Rpo256::hash_elements(seed);
let digest_elem = digest.as_elements();
Copy link
Contributor

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();

Copy link
Collaborator Author

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
Comment on lines 9 to 12
/// 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 {
Copy link
Contributor

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."

Copy link
Collaborator Author

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.

@bobbinth
Copy link
Contributor

Two other things for this PR:

  • Could we add a small section to the main README file about the rand module?
  • Let's update the CHANGELOG.

Copy link
Contributor

@bobbinth bobbinth 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!

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
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@Al-Kindi-0
Copy link
Collaborator Author

Makes sense. Done!

@Al-Kindi-0 Al-Kindi-0 changed the base branch from al-new-padding-rule to next December 19, 2023 17:19
@bobbinth
Copy link
Contributor

Thank you!

@bobbinth bobbinth merged commit 35c2e86 into next Dec 19, 2023
@bobbinth bobbinth deleted the al-rporandomcoin branch December 19, 2023 17:45
@bobbinth bobbinth mentioned this pull request Jan 6, 2024
12 tasks
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.

2 participants