Deduplicate code between evm ConstraintBuilder and BaseConstraintBuilder#1318
Conversation
github-actions
Bot
added
the
crate-zkevm-circuits
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
I added one note about removing a function that I think is duplicated, but otherwise looks very good!
|
@ed255 check now, I solved lots of conflicts with main basically |
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
Just one last small comment :P
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
LGTM! This looks nicer now :)
| condition: None, | ||
| } | ||
| } | ||
| pub(crate) trait ConstrainBuilderCommon<F: Field> { |
There was a problem hiding this comment.
| pub(crate) trait ConstrainBuilderCommon<F: Field> { | |
| pub(crate) trait ConstraintBuilderCommon<F: Field> { |
There was a problem hiding this comment.
I also think that maybe it's time to name ConstraintBuilder and BaseConstraintBuilder in another way. Tbh, I'm not really sure about the best naming, but what about:
ConstraintBuilder -> EvmConstraintBuilder
BaseConstraintBuilder -> BasicConstraintBulder
There was a problem hiding this comment.
Oh, I did not saw your comment about the renaming @leolara!
There was a problem hiding this comment.
Overall LGTM, nice refactor :) only the typo to be fixed
There was a problem hiding this comment.
I have done the ConstraintBuilder -> EvmConstraintBuilder rename
4 participants
Description
evm ConstraintBuilder and BaseConstraintBuilder have:
require_zero
require_equal
require_boolean
require_in_set
condition
add_constraints
add_constraint
validate_degree
all with very similar implementation. Deduplicate them
Issue Link
#1202
Type of change
Contents
Rationale
DRY
How Has This Been Tested?
Run tests.
Notes (TODO?)