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

Commenting pickles #13386

Merged
merged 100 commits into from
Sep 26, 2023
Merged

Commenting pickles #13386

merged 100 commits into from
Sep 26, 2023

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented Jun 12, 2023 •
edited
Loading

This MR aims to commenting pickles.

Pickles makes abstraction of multiple cryptographic and type theory concepts. I took the liberty to redirect the readers to online documentations and research papers instead of explaining with new words as I feel the explanations would be better by external resources.

For the reviewers: there is a bunch of TODO for which I do not know the answer. Feel free to comment.

For another MR(?):

  • plonk_checks
  • gen_scalars

@dannywillems dannywillems requested a review from a team as a code owner June 12, 2023 13:00
@dannywillems dannywillems mentioned this pull request Jun 12, 2023
Closed
7 tasks
@dannywillems dannywillems force-pushed the dannywillems/commenting-pickles branch 4 times, most recently from d321e42 to daebfbc Compare June 15, 2023 08:08
@dannywillems dannywillems changed the title Draft: commenting pickles Commenting pickles Jun 15, 2023
@dannywillems dannywillems force-pushed the dannywillems/commenting-pickles branch from daebfbc to e79dc0b Compare June 15, 2023 21:09
@dannywillems dannywillems requested a review from a team as a code owner June 15, 2023 21:09
mrmr1993
mrmr1993 reviewed Jun 15, 2023
View reviewed changes
src/lib/pickles/README.md Outdated Show resolved Hide resolved
src/lib/pickles/README.md Outdated Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Outdated Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/endo.mli Outdated Show resolved Hide resolved
@dannywillems dannywillems force-pushed the dannywillems/commenting-pickles branch 5 times, most recently from f596688 to a0ba2af Compare June 20, 2023 12:17
@dannywillems
Copy link
Member Author

Considering this MR as ready to be reviewed. More changes will come in another MR.

mrmr1993
mrmr1993 reviewed Jun 20, 2023
View reviewed changes
src/lib/pickles_types/shifted_value.mli Outdated Show resolved Hide resolved
src/lib/pickles_types/shifted_value.mli Outdated Show resolved Hide resolved
src/lib/pickles_types/shifted_value.mli Outdated Show resolved Hide resolved
src/lib/pickles_types/shifted_value.ml Outdated Show resolved Hide resolved
src/lib/pickles_types/shifted_value.ml Outdated Show resolved Hide resolved
@dannywillems dannywillems force-pushed the dannywillems/commenting-pickles branch 2 times, most recently from a83f887 to 8b48768 Compare June 20, 2023 17:02
@rbonichon
Copy link
Contributor

!ci-build-me

1 similar comment
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems mentioned this pull request Jun 21, 2023
Closed
mrmr1993
mrmr1993 reviewed Jun 27, 2023
View reviewed changes
src/lib/pickles/composition_types/spec.mli Outdated Show resolved Hide resolved
src/lib/pickles/step_main.ml Outdated Show resolved Hide resolved
src/lib/pickles/step_verifier.ml Outdated Show resolved Hide resolved
src/lib/pickles/step_verifier.mli Outdated Show resolved Hide resolved
src/lib/pickles/type.mli Outdated Show resolved Hide resolved
src/lib/pickles_base/proofs_verified.mli Outdated Show resolved Hide resolved
src/lib/pickles_base/side_loaded_verification_key.mli Outdated Show resolved Hide resolved
src/lib/pickles_types/at_most.ml Outdated Show resolved Hide resolved
src/lib/pickles_types/at_most.mli Outdated Show resolved Hide resolved
src/lib/pickles_types/vector.mli Outdated Show resolved Hide resolved
jspada
jspada approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@jspada jspada left a comment

Choose a reason for hiding this comment

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

Awesome work! So helpful to have these comments in the code! Well done!
Spotted a few typos, made some suggestions and asked some questions.

src/lib/pickles/README.md Outdated Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Show resolved Hide resolved
src/lib/pickles/README.md Outdated Show resolved Hide resolved
src/lib/pickles/dummy.mli Show resolved Hide resolved
src/lib/pickles/step_verifier.mli Show resolved Hide resolved
src/lib/pickles_base/domain.mli Outdated Show resolved Hide resolved
src/lib/pickles_types/nat.mli Show resolved Hide resolved
src/lib/pickles_types/plonk_types.mli Show resolved Hide resolved
src/lib/pickles_types/vector.mli Show resolved Hide resolved
@dannywillems dannywillems marked this pull request as draft August 15, 2023 08:39
@dannywillems
Copy link
Member Author

On hold for later. Converting to draft.

dannywillems and others added 9 commits September 25, 2023 21:05
@dannywillems
Update src/lib/pickles/fix_domains.mli
f55dcf0 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles_types/plonk_types.mli
3d9d219 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/limb_vector/make.mli
c9644f0 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/one_hot_vector/one_hot_vector.mli
04e6cc3 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/per_proof_witness.mli
9dcc828 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/pseudo/pseudo.mli
04e2a0f 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles_types/vector.mli
ca7b74a 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/pseudo/pseudo.mli
3f52b0d 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/side_loaded_verification_key.mli
4d20ff4 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
dannywillems
dannywillems commented Sep 25, 2023
View reviewed changes
src/lib/pickles/ro.mli Outdated Show resolved Hide resolved
dannywillems and others added 2 commits September 25, 2023 21:12
@dannywillems
Update src/lib/pickles/ro.mli
e625edb
@dannywillems
Update src/lib/pickles/tock_field_sponge.mli
6ae6b98 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
dannywillems and others added 6 commits September 25, 2023 21:15
@dannywillems
Update src/lib/pickles/requests.mli
7d06d5d
@dannywillems
Update src/lib/pickles/timer.mli
7acd5df 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/sponge_inputs.mli
39d4963 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Update src/lib/pickles/step_verifier.mli
69ec08f 
Co-authored-by: Richard Bonichon <richard.bonichon@o1labs.org>
@dannywillems
Pickles_types: hlist - remove syntax changed
53dae2e 
Not supposed to be in this PR
@dannywillems
Pickles_types: shifted_value - be more strict on the requirement
80e5904 
It is not "supposed", it is.
This was referenced Sep 25, 2023
Closed
Closed
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems force-pushed the dannywillems/commenting-pickles branch from f8645ed to 3ef0146 Compare September 25, 2023 20:05
@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems
Copy link
Member Author

!ci-build-me

@dannywillems dannywillems merged commit ad7c810 into develop Sep 26, 2023
@dannywillems dannywillems deleted the dannywillems/commenting-pickles branch September 26, 2023 09:35
@dannywillems dannywillems self-assigned this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrmr1993 mrmr1993 mrmr1993 left review comments

@rbonichon rbonichon rbonichon approved these changes

@jspada jspada jspada approved these changes

Assignees

@dannywillems dannywillems

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dannywillems @rbonichon @mrmr1993 @jspada