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.

Sign up for GitHub

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

Jump to bottom

Integrate with sponge & Add multivariate challenge strategy #82

Merged
merged 25 commits into from
Jul 22, 2021
Merged

Integrate with sponge & Add multivariate challenge strategy #82

merged 25 commits into from
Jul 22, 2021

Conversation

tsunrise
Copy link
Member

@tsunrise tsunrise commented Jun 18, 2021 •
edited

Description

  • Integrate with ark::sponge API
  • Add multivariate challenge strategy

closes: #81

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@tsunrise tsunrise changed the title Integrate with sponge & Add multivariate challenge strategy Integrate with sponge & Add multivariate challenge strategy [WIP] Jun 18, 2021
@tsunrise tsunrise changed the title Integrate with sponge & Add multivariate challenge strategy [WIP] Integrate with sponge & Add multivariate challenge strategy Jun 18, 2021
weikengchen
weikengchen reviewed Jun 18, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/data_structures.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 6, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

Pratyush commented Jul 6, 2021 •
edited

We should add a next_challenge_of_size API that allows specifying the challenge size via FieldElementSize.

@tsunrise
Copy link
Member Author

tsunrise commented Jul 7, 2021

We should add a next_challenge_of_size API that allows specifying the challenge size via FieldElementSize.

fixed

@tsunrise tsunrise marked this pull request as ready for review July 9, 2021 05:18
@tsunrise
Copy link
Member Author

tsunrise commented Jul 9, 2021

not sure why no_std fails in CI though. cargo.toml looks good to me...

Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/ipa_pc/mod.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/data_structures.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 13, 2021
View reviewed changes
src/lib.rs Show resolved Hide resolved
@Pratyush
Copy link
Member

This looks mostly good, barring the questions above. Great job!

Quick question: to reduce the churn wrt tests, instead of passing in the generation strategy as an argument, inside the test template can we simply add a loop over both kinds of strategies?

@tsunrise
Copy link
Member Author

This looks mostly good, barring the questions above. Great job!

Quick question: to reduce the churn wrt tests, instead of passing in the generation strategy as an argument, inside the test template can we simply add a loop over both kinds of strategies?

Sure. I will do that

@Pratyush
Copy link
Member

I think the no_std issue should be fixed now.

@tsunrise
Copy link
Member Author

I think the no_std issue should be fixed now.

Thanks so much! (I should really consider adding a spelling checker for my editor lol)

Pratyush
Pratyush reviewed Jul 19, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 19, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jul 19, 2021
View reviewed changes
src/challenge.rs Outdated Show resolved Hide resolved
@Pratyush Pratyush merged commit 982e1a1 into arkworks-rs:master Jul 22, 2021
@Pratyush Pratyush mentioned this pull request Jul 22, 2021
Closed
nmohnblatt added a commit to geometryxyz/poly-commit that referenced this pull request Jun 29, 2022
@nmohnblatt
Revert "Integrate with sponge & Add multivariate challenge strategy (a…
d79ead1 
…rkworks-rs#82)"

This reverts commit 982e1a1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pratyush Pratyush Pratyush left review comments

@weikengchen weikengchen weikengchen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate with ark-sponge
3 participants
@tsunrise @Pratyush @weikengchen