Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

Refactor: evm-circuit util/chips to "gadgets" crate#611

Merged
roynalnaruto merged 7 commits into
privacy-ethereum:mainfrom
scroll-tech:refactor/gadgets-util
Jul 12, 2022
Merged

Refactor: evm-circuit util/chips to "gadgets" crate#611
roynalnaruto merged 7 commits into
privacy-ethereum:mainfrom
scroll-tech:refactor/gadgets-util

Conversation

@roynalnaruto
Copy link
Copy Markdown
Collaborator

@roynalnaruto roynalnaruto commented Jul 10, 2022 •
edited
Loading

Closes #463

  • Migrates most of util module (Expr trait and implementations for common types) to gadgets member crate
  • Fix compilation in zkevm-circuits member crate wherever appropriate
  • Adds LessThanChip to the gadgets member crate, to be used in Copy Circuit #584
  • Migrate binary_number module from zkevm_circuits::state_circuit to gadgets crate
  • Migrate helper modules like and, sum, or, not and so on to gadgets crate

@github-actions github-actions Bot added crate-gadgets Issues related to the gadgets workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jul 10, 2022
@roynalnaruto roynalnaruto marked this pull request as ready for review July 10, 2022 11:44
@roynalnaruto roynalnaruto requested a review from CPerezz July 10, 2022 11:44
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Jul 10, 2022

@CPerezz added this PR to address #463 . Will rebase the #584 PR once this is merged. Would be good to get a review on this.

@roynalnaruto roynalnaruto mentioned this pull request Jul 10, 2022
Merged
8 tasks
@lispc
Copy link
Copy Markdown
Collaborator

lispc commented Jul 11, 2022

i have another idea: reimport (pub use ...) gadgets::util::Expr as crate::util::Expr. So we don't have to modify lots of files, so the change will be much less, and some pending PR will be easier to merge conflicts. what do you think? @roynalnaruto

@roynalnaruto roynalnaruto force-pushed the refactor/gadgets-util branch from b5e1526 to f8dbcf5 Compare July 11, 2022 03:43
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Jul 11, 2022

@lispc great idea, yes it will make the changes minimal. I've made that change.

@roynalnaruto roynalnaruto changed the title refactor Expr trait into gadgets, add LessThan to gadgets Refactor: evm-circuit util/chips to "gadgets" crate Jul 11, 2022
han0110
han0110 approved these changes Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM. Nice refactor!

Comment thread gadgets/src/less_than.rs Outdated
Comment thread gadgets/src/less_than.rs
ed255
ed255 reviewed Jul 12, 2022
View reviewed changes
Comment thread gadgets/src/binary_number.rs Outdated
Comment thread gadgets/src/util.rs Outdated
Comment thread gadgets/src/less_than.rs
@roynalnaruto roynalnaruto requested a review from ed255 July 12, 2022 15:22
ed255
ed255 approved these changes Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this refactor :)

z2trillion
z2trillion approved these changes Jul 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@z2trillion z2trillion 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

@roynalnaruto roynalnaruto merged commit 159a073 into privacy-ethereum:main Jul 12, 2022
@roynalnaruto roynalnaruto deleted the refactor/gadgets-util branch July 12, 2022 16:26
lightsing referenced this pull request in scroll-tech/zkevm-circuits Jul 25, 2022
@roynalnaruto @lispc @lightsing
Refactor: evm-circuit util/chips to "gadgets" crate (#611)
c71a99e 
* refactor Expr trait into gadgets, add LessThan to gadgets

* chore: refactor binary number mod and helper mods to gadgets crate

* chore: document the gadgets crate, minor changes as per review

* tests: Lt chip tests under gadgets crate

Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
jonathanpwang pushed a commit to axiom-crypto/zkevm-circuits that referenced this pull request Aug 1, 2023
@lispc
feat(zktrie state): fix state rebuilding (privacy-ethereum#611)
e6a6e33 
* capacity checking works well

* lint

* fix state wit
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@miha-stopar miha-stopar Awaiting requested review from miha-stopar

@CPerezz CPerezz Awaiting requested review from CPerezz

4 more reviewers

@lispc lispc lispc approved these changes

@z2trillion z2trillion z2trillion approved these changes

@han0110 han0110 han0110 approved these changes

@ed255 ed255 ed255 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crate-gadgets Issues related to the gadgets workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate util module gadgets to the gadgets workspace member

5 participants

@roynalnaruto @lispc @z2trillion @han0110 @ed255