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(zk): refactor circuit polynomial builder #384

Merged
merged 10 commits into from
Apr 11, 2024

Conversation

chokobole
Copy link
Contributor

Description

This PR refactors(mostly) and optimizes CircuitPolynomialBulider

Copy link
Contributor

@dongchangYoo dongchangYoo 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.

44e9625
refac(zk): change set_id to j for consistency
->
refac(zk): change set_idx to j for consistency

7bdf930
refac(zk): sort member variable in a order of usage
->
refac(zk): sort member variables in a order of usage

7bdf930
I think it should be in the order theta, gamma, beta if the order is based on usage; however, why don’t we just squash this with 6c7e174 since we over write part of this commit?
& If this is changed, I think EvaluationInput constructor should also be changed (aka use in line 267)

Previously, the progress was displayed as (0 / 0), and now it's changed to (1 / 1).
@chokobole
Copy link
Contributor Author

44e9625 refac(zk): change set_id to j for consistency -> refac(zk): change set_idx to j for consistency

7bdf930 refac(zk): sort member variable in a order of usage -> refac(zk): sort member variables in a order of usage

7bdf930 I think it should be in the order theta, gamma, beta if the order is based on usage; however, why don’t we just squash this with 6c7e174 since we over write part of this commit? & If this is changed, I think EvaluationInput constructor should also be changed (aka use in line 267)

I agree with the first and second points. But For the third point, yeap, I also noticed that I should've changed EvaluationInput constructor, but I don't agree with squashing both commits since I believe both commits handle different parts.

@chokobole chokobole force-pushed the refac/refactor-circuit-polynomial-builder branch from 5e6b971 to d742867 Compare April 9, 2024 14:43
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

@chokobole chokobole force-pushed the refac/refactor-circuit-polynomial-builder branch from d742867 to d661376 Compare April 11, 2024 04:22
@chokobole chokobole force-pushed the refac/refactor-circuit-polynomial-builder branch from d661376 to f179bbb Compare April 11, 2024 04:46
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.

LGTM

@chokobole chokobole merged commit 9889d5f into main Apr 11, 2024
3 checks passed
@chokobole chokobole deleted the refac/refactor-circuit-polynomial-builder branch April 11, 2024 05:29
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

5 participants