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

Cosmetics/Refactoring #14870

Merged
merged 8 commits into from
Jan 17, 2024
Merged

Cosmetics/Refactoring #14870

merged 8 commits into from
Jan 17, 2024

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Jan 16, 2024

This PR is purely cosmetic refactoring not intended to have any logical changes.

Dependency:

@volhovm volhovm changed the title Volhovm/refactoring 15 01 2024 Cosmetics/Refactoring Jan 16, 2024
Copy link
Contributor

@rbonichon rbonichon left a comment

Choose a reason for hiding this comment

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

Left some minor comments. Thanks!

@@ -147,7 +147,7 @@ let
MINA_COMMIT_DATE = "<unknown>";
MINA_BRANCH = "<unknown>";

DUNE_PROFILE = "devnet";
DUNE_PROFILE = "dev";
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it might need to depend on the concrete type of building one's doing, but that's a task for the future.


let create_async (pk : Keypair.t) ~primary ~auxiliary ~prev_chals
~prev_comms =
create_aux pk ~primary ~auxiliary ~prev_chals ~prev_comms
~f:(fun pk auxiliary_input runtime_tables prev_challenges prev_sgs ->
~f:(fun index witness runtime_tables prev_chals prev_sgs ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine the prev_chals change is related to homogeneity, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's just more homogenous w.r.t. the other call in the same file.

Comment on lines 15 to 19
let _one_hot_vector_to_num (type n) (v : n Per_proof_witness.One_hot_vector.t) :
Field.t =
let n = Vector.length (v :> (Boolean.var, n) Vector.t) in
let open Step_verifier in
Pseudo.choose (v, Vector.init n ~f:Field.of_int) ~f:Fn.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have removed the function altogether, maybe ;-)

@@ -2,7 +2,6 @@ open Pickles_types
open Hlist
open Import
open Impls.Step
open Step_verifier
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. Vicious ;-) Some people will not like that. I'm in favor, though

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm very much a fan of explicit imports, everything else is often really unreadable without some external tooling like IDE...

in
Boolean.Assert.all vs ; chalss )
in
[%log internal] "Step_compute_bulletproof_challenges_done" ;
let messages_for_next_step_proof =
let challenge_polynomial_commitments =
let module M =
H3.Map (Per_proof_witness) (E03 (Inner_curve))
H3.Map (Per_proof_witness) (E03 (Step_verifier.Inner_curve))
Copy link
Contributor

Choose a reason for hiding this comment

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

This here actually helps. We have so many Inner_curve identifiers

@volhovm volhovm marked this pull request as ready for review January 17, 2024 13:51
@volhovm volhovm requested review from a team as code owners January 17, 2024 13:51
This reverts commit 7f4c46e.
@volhovm
Copy link
Member Author

volhovm commented Jan 17, 2024

!ci-build-me

@volhovm
Copy link
Member Author

volhovm commented Jan 17, 2024

!ci-build-me

@volhovm
Copy link
Member Author

volhovm commented Jan 17, 2024

The dependency cosmetic PR o1-labs/snarky#833 cannot be merged right now because their CI is broken. So I removed the snarky repo update and will merge only the local changes.

@volhovm
Copy link
Member Author

volhovm commented Jan 17, 2024

!ci-build-me

@volhovm volhovm merged commit 0aa7f5e into develop Jan 17, 2024
39 checks passed
@volhovm volhovm deleted the volhovm/refactoring-15-01-2024 branch January 17, 2024 16:04
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.

5 participants