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

refac(halo2): change concrete struct type to trait type for rng #394

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

k-gunjan
Copy link

Description

This PR makes rng parameter of create_proof function generic over RngCore and RngState.
Defined trait RngState and implemented for XORShiftRng

@k-gunjan k-gunjan marked this pull request as draft April 19, 2024 06:41
@k-gunjan k-gunjan marked this pull request as ready for review April 23, 2024 04:27
@chokobole chokobole changed the title Refac/change concrete struct type to trait type for rng refac: change concrete struct type to trait type for rng Apr 23, 2024
@chokobole
Copy link
Contributor

Thanks for your contribution! 🎉

Firstly, could you please follow the commit convention outlined here?

Additionally, it's essential that every commit is buildable and testable, but unfortunately, your PR doesn't meet these criteria. To address this, I suggest squashing all the commits into a single one to ensure they're buildable and testable.

Let me know if you have any questions or need further clarification. Thanks again for your effort! 👍

@k-gunjan k-gunjan force-pushed the refac/change-concrete-struct-type-to-trait-type-for-rng branch from 1ded7a6 to fc029f6 Compare April 23, 2024 15:06
@k-gunjan k-gunjan requested a review from chokobole April 23, 2024 15:08
…iftRng with generic R which implements RngState
@k-gunjan k-gunjan force-pushed the refac/change-concrete-struct-type-to-trait-type-for-rng branch from fc029f6 to 28a22d5 Compare April 23, 2024 15:16
@chokobole chokobole changed the title refac: change concrete struct type to trait type for rng refac(halo2): change concrete struct type to trait type for rng Apr 24, 2024
@dongchangYoo
Copy link
Contributor

dongchangYoo commented Apr 24, 2024

@k-gunjan
How about creating a unit test that shows that the results (proofS) of passing XORShiftRng and XorShiftRng to the create_proof function are the same?

Copy link
Contributor

@chokobole chokobole left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ashjeong ashjeong left a comment

Choose a reason for hiding this comment

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

Could you change your commit's scope to match the PR's scope (Halo2)? Thanks again for the contribution~ 🥰

Copy link
Contributor

@Insun35 Insun35 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@TomTaehoonKim TomTaehoonKim left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

6 participants