-
Notifications
You must be signed in to change notification settings - Fork 10
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
expose snarky hashToGroup #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @Trivo25! I have a couple of comments.
In addition to those, I think this should have a unit test which demonstrates that the TS implementation equals the OCaml one.
You can find similar thngs being done in other unit tests like mina-signer/src/signature.unit-test.ts
.
Instead of hard-coded test inputs, it's best to use random field elements with our property testing framework, like here in the finite field unit test:
https://github.com/o1-labs/snarkyjs-bindings/blob/290d7b8fbcdc7ecba2f8981811ee958800bc4ad5/crypto/finite-field.unit-test.ts#L26-L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'm approving in advance but have two more comments about the test
Co-authored-by: Gregor Mitscha-Baude <gregor.mitscha-baude@gmx.at>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, LGTM.
Some improvements suggested in the OCaml code.
let input = Array.map (Js.to_array xs) ~f:of_js_field in | ||
if Js.to_bool is_checked then Random_oracle.Checked.hash input | ||
else Random_oracle.hash (Array.map ~f:to_unchecked input) |> Field.constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let input = Array.map (Js.to_array xs) ~f:of_js_field in | |
if Js.to_bool is_checked then Random_oracle.Checked.hash input | |
else Random_oracle.hash (Array.map ~f:to_unchecked input) |> Field.constant | |
let a = Js.to_array xs in | |
if Js.to_bool is_checked then Random_oracle.Checked.hash (Array.map a ~f:of_js_field) | |
else Random_oracle.hash (Array.map a ~f:(fun x -> to_unchecked (of_js_field x)) |> Field.constant |
One fewer iteration in the else case. Don't know if worth it ;-)
let digest = | ||
if Js.to_bool is_checked then Random_oracle.Checked.hash input | ||
if Js.to_bool is_checked then | ||
Snark_params.Group_map.Checked.to_group input | ||
else | ||
Random_oracle.hash (Array.map ~f:to_unchecked input) |> Field.constant | ||
let x, y = Snark_params.Group_map.to_group (to_unchecked input) in | ||
(Field.constant @@ x, Field.constant @@ y) | ||
in | ||
to_js_field digest | ||
to_js_group (fst digest) (snd digest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Js.to_bool is_checked then Snark_params.Group_map.Checked.to_group input
else
let x, y = Snark_params.Group_map.to_group (to_unchecked input) in
to_js_group (Field.constant x) (Field.constant y)
- the
@@
is unneeded - don't need to construct
digest
to immediately destruct it throughfst
andsnd
This reverts commit bd61c15.
note: the params needed for the curve operation are currently hard-coded (originally taken from OCaml) and not calculated in JS
TODO: push bindings
closes o1-labs/o1js#772 companion of o1-labs/o1js#887