-
Notifications
You must be signed in to change notification settings - Fork 112
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
Arrabbiata: move challenge semantic into its own module #3024
base: dw/use-challenges-structure
Are you sure you want to change the base?
Conversation
Simple move, only as the code is larger than initially, and should not be related to the column module.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dw/use-challenges-structure #3024 +/- ##
============================================================
Coverage 76.87% 76.87%
============================================================
Files 262 263 +1
Lines 62165 62165
============================================================
+ Hits 47787 47791 +4
+ Misses 14378 14374 -4 ☔ View full report in Codecov by Sentry. |
pub relation_randomiser: F, | ||
} | ||
|
||
impl<F> Index<usize> for Challenges<F> { |
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.
I know it's outside the scope of the PR, but I don't like having Index, only Index should be used
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.
I don't understand the comment.
impl<F: Zero> Default for Challenges<F> { | ||
fn default() -> Self { | ||
Self { | ||
constraint_randomiser: F::zero(), |
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
Also outside of the scope, but the alpha field would better be called contraint_combiner.
The term constraint_randomiser indicates a prover coined random to achieve some zk property
same remark for relation_randomiser
Given that the field are documented it's nonentheless understandable
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.
Agreed. Let me change this.
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.
See #3031
Simple move, only as the code is larger than initially, and should not be related to the column module.