[o1js-main] Add o1js-stub to Mina repo in place of SnarkyJS #14461

MartinMinkov · 2023-10-30T22:41:53Z

Summary

Addresses #14466

🔗 o1js: o1-labs/o1js#1133
🔗 o1js-bindings: o1-labs/o1js-bindings#159

This PR aims to add support for building mina as a submodule of o1js. To do so, the following was done:

Removed the snarkyjs submodule
Added a o1js_stub module, that has the same dependencies as o1js. The goal with this module is to have a way for Mina CI to check that o1js can still be successfully compiled after changes are made. This gives a safety net to not totally break things if changes to the dependencies are made.
Adds the kimchi library from o1js-bindings into src/lib/crypto. This is to make sure that we can compile o1js_stub since we need the WASM dependencies generated from the library, as well as to support a longer term plan to create a o1 crypto repo that recombines the wasm bindings with o1caml-rust stubs.
Removes snarkyjs from nix and updates the Rust part of nix to reference the new kimchi library location
Removed codeowners entry for snarkyjs

This reverts commit 75f49e0c09faed96c13978db6b390acedcdb3756.

… checks

…pt).

…hanges-o1jsmain Feat/port rosetta zkapp changes o1jsmain

MartinMinkov · 2023-12-11T18:43:08Z

!ci-build-me

mitschabaude · 2023-12-12T13:27:55Z

!ci-build-me

…to feat/add-o1js-stubs

MartinMinkov · 2023-12-13T20:29:00Z

!ci-build-me

mitschabaude

Great work!

bkase

I approve the CODEOWNERS change -- what tags me as a blocker on (I delegate the protocol and infra review to others even those GitHub will say it's ✅ )

dannywillems

A first pass. Some comments:

That would be nice to provide a way to check the version of o1js-bindings has been imported. I cannot be sure that the code has not been modified.
It would be nice that dune build builds plonk_wasm. cargo doesn't seem to be run when we use dune build src/lib. I would have expected it does.
I'm not sure kimchi is the best name for the wasm/js deps.
I think it is already in o1js-bindings, but I see, when running dune build src/lib/o1js_stub:

warning: free variables in primitive code "tsSrs" (/home/soc/codes/o1-labs/mina/_build/default/src/lib/crypto/kimchi/js/bindings.js:13)
vars: tsBindings
code providing tsSrs (/home/soc/codes/o1-labs/mina/_build/default/src/lib/crypto/kimchi/js/bindings.js:13) may miss dependencies: tsBindings

Nix build is not happy. But I think it is already on o1js-main
Some commit messages could be squashed to clear the history, like d850951, 16716c9, f07fa96, e59e805, 3403057, 1ba5689, 645a768
I am a bit lost with some git merge, not sure why it is there.

Questions:

Do we run cargo fmt and clippy for plonk_wasm?
I do see some commits remove crypto/kimchi/js/js_backend, but it is still in my tree. Any reason?
Why don't we run prettier on JS files?

dannywillems · 2023-12-15T20:54:34Z

src/lib/crypto/kimchi/js/README.md

+need to run
+
+```
+dune build src/lib/marlin_plonk_bindings/js/test/nodejs/nodejs_test.bc.js


Can be changed to the right repo.

dannywillems · 2023-12-15T20:54:40Z

src/lib/crypto/kimchi/js/README.md

+
+```
+dune build src/lib/marlin_plonk_bindings/js/test/nodejs/nodejs_test.bc.js
+src/lib/marlin_plonk_bindings/js/test/nodejs/copy_over.sh


dannywillems · 2023-12-15T20:54:45Z

src/lib/crypto/kimchi/js/README.md

+Similarly, to run the web tests in `test/web`, you can run
+
+```
+dune build src/lib/marlin_plonk_bindings/js/test/web/web_test.bc.js


dannywillems · 2023-12-15T20:54:50Z

src/lib/crypto/kimchi/js/README.md

+
+```
+dune build src/lib/marlin_plonk_bindings/js/test/web/web_test.bc.js
+src/lib/marlin_plonk_bindings/js/test/web/copy_over.sh


mitschabaude · 2023-12-18T07:16:16Z

Hey @dannywillems I want to answer to some of your comments!

That would be nice to provide a way to check the version of o1js-bindings has been imported. I cannot be sure that the code has not been modified.

I just did this locally and verified that there is no change introduced. I cloned the version of o1js which has this Mina PR checked out (o1-labs/o1js#1133) and redid what Martin did: copy the /wasm folder from bindings/kimchi/wasm to mina/src/lib/crypto/kimchi/wasm. The only change is to the file paths that reference proof-systems, which is correct (the relative location of proof-systems is now ../.., not ../../../../..)

Feel free to do the same if you want to verify it:

gregor@p340:~$ rm -rf o1/o1js/src/mina/src/lib/crypto/kimchi/wasm
gregor@p340:~$ cp -r o1/mina/src/lib/snarkyjs/src/bindings/kimchi/wasm o1/o1js/src/mina/src/lib/crypto/kimchi/wasm
gregor@p340:~$ cd o1/o1js/src/mina
gregor@p340:~/o1/o1js/src/mina$ git diff
diff --git a/src/lib/crypto/kimchi/wasm/Cargo.toml b/src/lib/crypto/kimchi/wasm/Cargo.toml
index 65cde66006..18f9721adb 100644
--- a/src/lib/crypto/kimchi/wasm/Cargo.toml
+++ b/src/lib/crypto/kimchi/wasm/Cargo.toml
@@ -30,12 +30,12 @@ ark-ec = { version = "0.3.0", features = ["parallel"] }
 ark-poly = { version = "0.3.0", features = ["parallel"] }
 
 # proof-systems
-poly-commitment = { path = "../../proof-systems/poly-commitment" }
-groupmap = { path = "../../proof-systems/groupmap" }
-mina-curves = { path = "../../proof-systems/curves" }
-o1-utils = { path = "../../proof-systems/utils" }
-mina-poseidon = { path = "../../proof-systems/poseidon" }
-kimchi = { path = "../../proof-systems/kimchi", features = ["wasm_types"] }
+poly-commitment = { path = "../../../../../crypto/proof-systems/poly-commitment" }
+groupmap = { path = "../../../../../crypto/proof-systems/groupmap" }
+mina-curves = { path = "../../../../../crypto/proof-systems/curves" }
+o1-utils = { path = "../../../../../crypto/proof-systems/utils" }
+mina-poseidon = { path = "../../../../../crypto/proof-systems/poseidon" }
+kimchi = { path = "../../../../../crypto/proof-systems/kimchi", features = ["wasm_types"] }
 
 array-init = "2.0.0"
 base64 = "0.13.0"

mitschabaude · 2023-12-18T07:34:01Z

I'm not sure kimchi is the best name for the wasm/js deps.

@MartinMinkov I agree with @dannywillems. The original location in the Mina repo was in src/lib/crypto/kimchi_bindings, where the wasm and js folders sat next to stubs. I would suggest putting it there again instead of src/lib/crypto/kimchi.

I think it is already in o1js-bindings, but I see, when running dune build src/lib/o1js_stub:

warning: free variables in primitive code "tsSrs" (/home/soc/codes/o1-labs/mina/_build/default/src/lib/crypto/kimchi/js/bindings.js:13)
vars: tsBindings
code providing tsSrs (/home/soc/codes/o1-labs/mina/_build/default/src/lib/crypto/kimchi/js/bindings.js:13) may miss dependencies: tsBindings

Yeah this is already in o1js-bindings. Not really in the scope of this PR but I'm happy to push a small commit that fixes it.

Nix build is not happy. But I think it is already on o1js-main

Already in o1js-main, not in scope for this PR

I am a bit lost with some git merge, not sure why it is there.

@dannywillems I suspect you mean #14683 which was merged into this PR. It was necessary to fix the rosetta integration test in the Mina repo, which depended on o1js. Since o1js is no longer accessible from the mina repo, the test had to be refactored.

I do see some commits remove crypto/kimchi/js/js_backend, but it is still in my tree. Any reason?

I requested it on this PR: #14461 (comment)

Why don't we run prettier on JS files?

I assume you mean why don't we run it in CI? All of our JS developers will probably run prettier in their editors when auto-formatting on save.

In o1js-bindings we also had the GH action that you added which ran prettier in CI. I think adding a similar CI job here is out of scope of this PR, we don't have any JS CI now in Mina. But it could be a useful follow-up, when we also add the o1js-stubs compilation test.

mitschabaude · 2023-12-18T08:06:44Z

I'm not sure kimchi is the best name for the wasm/js deps.

Addressed here: b6a14e0

warning: free variables in primitive code "tsSrs"

Addressed here: f7a0abc

dannywillems · 2023-12-18T09:35:11Z

I assume you mean why don't we run it in CI? All of our JS developers will probably run prettier in their editors when auto-formatting on save.
In o1js-bindings we also had the GH action that you added which ran prettier in CI. I think adding a similar CI job here is out of scope of this PR, we don't have any JS CI now in Mina. But it could be a useful follow-up, when we also add the o1js-stubs compilation test.

LGTM for the follow-up.

joaosreis

Changes in Rosetta here come from commits outside the scope of this work. Would it be best to merge those changes separately (if you mean to merge them) so this PR only changes related files? Regardless, I'm approving since changes come from an already approved branch.

dannywillems · 2023-12-19T09:29:38Z

!ci-build-me

dannywillems · 2023-12-19T09:29:47Z

I opened the follow-up issue: #14741 listing my comments above.

dannywillems · 2023-12-19T09:33:12Z

Regarding #14461 (review), I totally do agree we should not include changes that are unrelated to the PR.
Another PR should have been opened, and this one could have been rebased (or whatever you like which do not pollute the commits) to include these changes. This will ease the review, and avoid bugs that can be introduced by mistakes.

In addition to that, we must enforce cleaning commits before merging. Seeing in the git history multiple commits with the same message and with the same goal, resulting from multiple attempts, should be forbidden. Before review, the history should be cleaned, and when a review starts, the OP should not modify previous commits, but only write on top of it, and request a new review after the OP cleaned the additional commits.

dkijania and others added 30 commits September 27, 2023 12:53

run only fast checks in rosetta int tests

5dfb734

add fast script for rosetta

6118445

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

1d47673

correct path

1d89857

Update rosetta-integration-tests-fast.sh

287fdc4

more validations

2109210

update counte

591b5bf

Revert "Auxiliary commit to revert individual files from 591b5bf"

536d242

This reverts commit 75f49e0c09faed96c13978db6b390acedcdb3756.

add quick test with minimal checks and no zkapps use for pull request…

4066dc1

… checks

compilation fixes

32e266f

remove leftovers

356d7da

remove comment

e4ba43a

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

6b55520

remove zkapps completely

528e0bf

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

439ec8f

Use zkapp_test_transaction for Rosetta integration tests (debug attem…

daeaf73

…pt).

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

43dec12

Typo.

d850951

Typo.

16716c9

Sending zkapp txns agaisnt Daemon's GraphQL endpoint.

47e96fd

Let's try with Python.

f07fa96

python3 -> python

e59e805

Install python.

3403057

Install python.

1ba5689

Install python and deps when needed (debugging attempt).

645a768

Escaping quotes with jq.

b6a842a

Tested network comms locally.

2d7a523

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

b3f59d4

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

e0286bd

Merge branch 'berkeley' into dkijania/tailor_rosetta_int_tests

d45ab3e

Merge pull request #14683 from MinaProtocol/feat/port-rosetta-zkapp-c…

8438840

…hanges-o1jsmain Feat/port rosetta zkapp changes o1jsmain

MartinMinkov requested a review from a team as a code owner December 11, 2023 18:41

delete js_backend from crypto/kimchi

07ba705

[rosetta] Fix broken SQL quesry for zkapp account updates.

3180007

Merge branch 'feat/add-o1js-stubs' of github.com:MinaProtocol/mina in…

d4314b0

…to feat/add-o1js-stubs

MartinMinkov mentioned this pull request Dec 13, 2023

[develop] Add mina as a git submodule for o1js o1-labs/o1js#1318

Merged

mitschabaude approved these changes Dec 14, 2023

View reviewed changes

bkase approved these changes Dec 15, 2023

View reviewed changes

dannywillems reviewed Dec 15, 2023

View reviewed changes

dannywillems mentioned this pull request Dec 15, 2023

[develop] Add o1js-stub to Mina repo in place of SnarkyJS #14649

Merged

mitschabaude added 3 commits December 18, 2023 08:58

move /kimchi/{wasm,js} to kimchi_bindings

b6a14e0

fix jsoo complaint about free variable tsBindings

f7a0abc

add back newline

cb151f6

joaosreis reviewed Dec 18, 2023

View reviewed changes

joaosreis approved these changes Dec 18, 2023

View reviewed changes

georgeee approved these changes Dec 18, 2023

View reviewed changes

dannywillems mentioned this pull request Dec 19, 2023

Follow-up on !14461 #14741

Open

5 tasks

dannywillems approved these changes Dec 19, 2023

View reviewed changes

MartinMinkov merged commit 5d2ea7a into o1js-main Dec 19, 2023
52 of 53 checks passed

MartinMinkov deleted the feat/add-o1js-stubs branch December 19, 2023 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[o1js-main] Add o1js-stub to Mina repo in place of SnarkyJS #14461

[o1js-main] Add o1js-stub to Mina repo in place of SnarkyJS #14461

MartinMinkov commented Oct 30, 2023 •

edited

Loading

MartinMinkov commented Dec 11, 2023

mitschabaude commented Dec 12, 2023

MartinMinkov commented Dec 13, 2023

mitschabaude left a comment

bkase left a comment •

edited

Loading

dannywillems left a comment

dannywillems Dec 15, 2023

dannywillems Dec 15, 2023

dannywillems Dec 15, 2023

dannywillems Dec 15, 2023

mitschabaude commented Dec 18, 2023 •

edited

Loading

mitschabaude commented Dec 18, 2023 •

edited

Loading

mitschabaude commented Dec 18, 2023

dannywillems commented Dec 18, 2023

joaosreis left a comment •

edited

Loading

dannywillems commented Dec 19, 2023

dannywillems commented Dec 19, 2023 •

edited

Loading

dannywillems commented Dec 19, 2023 •

edited

Loading

[o1js-main] Add o1js-stub to Mina repo in place of SnarkyJS #14461

[o1js-main] Add o1js-stub to Mina repo in place of SnarkyJS #14461

Conversation

MartinMinkov commented Oct 30, 2023 • edited Loading

Summary

MartinMinkov commented Dec 11, 2023

mitschabaude commented Dec 12, 2023

MartinMinkov commented Dec 13, 2023

mitschabaude left a comment

Choose a reason for hiding this comment

bkase left a comment • edited Loading

Choose a reason for hiding this comment

dannywillems left a comment

Choose a reason for hiding this comment

dannywillems Dec 15, 2023

Choose a reason for hiding this comment

dannywillems Dec 15, 2023

Choose a reason for hiding this comment

dannywillems Dec 15, 2023

Choose a reason for hiding this comment

dannywillems Dec 15, 2023

Choose a reason for hiding this comment

mitschabaude commented Dec 18, 2023 • edited Loading

mitschabaude commented Dec 18, 2023 • edited Loading

mitschabaude commented Dec 18, 2023

dannywillems commented Dec 18, 2023

joaosreis left a comment • edited Loading

Choose a reason for hiding this comment

dannywillems commented Dec 19, 2023

dannywillems commented Dec 19, 2023 • edited Loading

dannywillems commented Dec 19, 2023 • edited Loading

MartinMinkov commented Oct 30, 2023 •

edited

Loading

bkase left a comment •

edited

Loading

mitschabaude commented Dec 18, 2023 •

edited

Loading

mitschabaude commented Dec 18, 2023 •

edited

Loading

joaosreis left a comment •

edited

Loading

dannywillems commented Dec 19, 2023 •

edited

Loading

dannywillems commented Dec 19, 2023 •

edited

Loading