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

Gp/first impl #1

Merged
merged 7 commits into from
Jun 4, 2024
Merged

Gp/first impl #1

merged 7 commits into from
Jun 4, 2024

Conversation

JackPiri
Copy link
Contributor

@JackPiri JackPiri commented Jun 3, 2024

First implementation for the verifier:

  • data structures
  • deserialization
  • verification

Copy link
Collaborator

@DanieleDiBenedetto DanieleDiBenedetto left a comment

Choose a reason for hiding this comment

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

Perhaps, more expressive than posting elf files, it would be nice to directly post the examples (in the same way Risc0 does it) and the way you generated the resources starting from them. I guess it would turn out to be useful also for documentation purposes.

Please also be sure to document public functions/structs/types

Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Also included minor refactoring
HEADER-APACHE2 Outdated Show resolved Hide resolved
@JackPiri
Copy link
Contributor Author

JackPiri commented Jun 3, 2024

Perhaps, more expressive than posting elf files, it would be nice to directly post the examples (in the same way Risc0 does it) and the way you generated the resources starting from them. I guess it would turn out to be useful also for documentation purposes.

Please also be sure to document public functions/structs/types

Perhaps, more expressive than posting elf files, it would be nice to directly post the examples (in the same way Risc0 does it) and the way you generated the resources starting from them. I guess it would turn out to be useful also for documentation purposes.

Please also be sure to document public functions/structs/types

Replaced elf/binary files with source code.
Public code is now documented.

Copy link
Collaborator

@drgora drgora left a comment

Choose a reason for hiding this comment

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

I left few small comments; I believe the one on the test should be fixed before merging.

tests/integration.rs Outdated Show resolved Hide resolved
tests/integration.rs Outdated Show resolved Hide resolved
src/key.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Please fix rust-version, add the checks to Makefile.toml and include Readme.md in docs.

The comments to the code are mainly less important.

.github/workflows/CI-rustdoc.yml Show resolved Hide resolved
Makefile.toml Show resolved Hide resolved
src/proof.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Makefile.toml Outdated Show resolved Hide resolved
Makefile.toml Show resolved Hide resolved
@la10736 la10736 self-requested a review June 4, 2024 16:08
Copy link
Collaborator

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

LGTM!!!

Great job!

Copy link
Collaborator

@drgora drgora 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 to me now, thanks!

@JackPiri JackPiri merged commit b912231 into master Jun 4, 2024
2 checks passed
@JackPiri JackPiri deleted the gp/first_impl branch June 4, 2024 19:35
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.

4 participants