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

Splitting to Multiple files and Optimizing #43

Merged
merged 18 commits into from
Jan 30, 2025

Conversation

anupsv
Copy link
Collaborator

@anupsv anupsv commented Jan 23, 2025

Addresses:
#36
#35

@@ -15,15 +15,14 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 30
steps:
- uses: actions/checkout@v4
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just moving to hash here to make it more safer.

src/kzg.rs Outdated Show resolved Hide resolved
@anupsv anupsv marked this pull request as ready for review January 27, 2025 21:23
@bxue-l2
Copy link
Collaborator

bxue-l2 commented Jan 28, 2025

It looks there are unused dependency in the PR, I found out by cargo machete --with-metadata, more see

Copy link
Collaborator

@bxue-l2 bxue-l2 left a comment

Choose a reason for hiding this comment

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

overall, lgtm. But I will integrate hokulea with this library. And provide more feedbacks

primitives/Cargo.toml Outdated Show resolved Hide resolved
&self,
polynomial: &PolynomialEvalForm,
index: u64,
srs: &SRS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting, so you are passing srs as an argument to the function. As opposed to having prover to hold it.

One advantage here is that in the application code, we can just have one srs, and instantiate multiple provers. @samlaf wdyt

Copy link
Contributor

Choose a reason for hiding this comment

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

When you say "instantiate multiple provers", you mean "multiple KZG structs"? Problem seems to be that KZG struct holds a fixed sized expanded_roots_of_unity. Should we also pass that in then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, there is really not much difference, between storing a list of expanded_roots_of_unity, vs storing a list of kzg struct.

@bxue-l2
Copy link
Collaborator

bxue-l2 commented Jan 30, 2025

LGTM, hokulea is able to use the new interfaces, see
Layr-Labs/hokulea#29

Copy link
Collaborator

@bxue-l2 bxue-l2 left a comment

Choose a reason for hiding this comment

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

please address the test error

@anupsv anupsv merged commit bdd755e into master Jan 30, 2025
1 check passed
@anupsv anupsv deleted the splitting-to-multiple-files branch January 30, 2025 20:43
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.

3 participants