o1-labs/o1js-bindings

Bindings for o1js to lower layers of the proof system and the Mina transaction logic

o1js-bindings's Issues

Add a check in the CI verifying the code typechecks

          > Is it possible to add in the CI the code typechecks?

that would be very nice indeed, will think about it. probably just means an extra tsconfig file here and make CI install TS

Originally posted by @mitschabaude in #140 (comment)

Optimisation: avoid destructure the array in function parameters.

          you call the function with the full array. this statement here happens inside the function. it destructures the full array to drop the first element. so we create a copy which has 1 element less.

again, I want to avoid the error-prone inline index-shifting

Originally posted by @mitschabaude in #140 (comment)

Avoid copying arrays

          Yes we could slightly optimize the code, but it comes at the cost that we have to always deal with the index translation by 1. This has led to bugs in the previous code base, so I'd like to keep it simple for now and always to the conversion to/from an ml array as a separate step.

In theory we could use array.unshift(0) at the end to get the same result without copying the array. The only reason I don't do that is because TS doesn't understand it

Originally posted by @mitschabaude in #140 (comment)

Documentation: Update README's to reflect that building uses o1js instead of Mina

There is stale documentation such as this file, that needs to be updated to make mention of o1js being the root project to build from now.

Refactor: Change `__snarkyTsBindings` -> `__o1jsTsBindings`

While refactoring all references of snarkyjs to o1js, we need to make a change to this part where an object is imported from the Mina repo.

There needs to be a set of accompanying PRs that makes the changes in this PR and in the Mina PR

Comparison in toBase

Should the line in toBase() be

for (let power = base; power <= x; power **= 2n) {

instead of

for (let power = base; power < x; power **= 2n) {

to properly handle the case when x is equal to base?
https://github.com/o1-labs/o1js-bindings/blob/main/crypto/bigint-helpers.ts#L191

CamlLookupSelectors: xor comes first

          xor comes first

Originally posted by @dannywillems in #160 (comment)

Don't use `.unwrap()`

All uses should be replaced by something that reports a useful error.
In many cases the panic should probably be turned into a Result::Err instead and passed back up.
This is especially problematic because panics don't contain any useful debugging information when compiled to WASM.
See rustwasm/wasm-bindgen#1289

Drop `promise` lib?

You don't need it.
A thread pool is needed anyway, you can run the async_kernel scheduler in there too.
You can await on a Deferred.t by using the Promise constructor and calling the resolve function passed in from within the Ivar's handler.
You can implement from_deferred : 'a Deferred.t -> a Promise.t too using this simple trick,
but I'm not sure the library has any point at that point. All you really want is the to_js and of_js functions.

`npm run make` broken on `berkeley`

It doesn't put the compiled OCaml output in the dist directory

Investigate if there are memory leaks due to missing `.free()` calls

See for example #142 (comment)

Interdependency between this and monorepo

MinaProtocol/mina depends on this, but this also depends on the monorepo.
Very little is gained from this setup AFAICT compared to having it be part of the monorepo properly rather than as a submodule.
Alternatively you could break the cycle and remove the submodule from the monorepo.

Adjust efficient num workers for Apple M1 Max

I was investigating proving performance on Apple M1 Max, and managed to double the proving speed by adjusting the number of threads/workers used in this config:

o1js-bindings/js/node/node-backend.js

Line 142 in c5aa5f1

'Apple M1 Max': 3,

The associated issue has apparently been fixed in newer chromium (?) and/or nodejs. So i've upgraded to nodejs v21.4.0 and set the num workers to 7, which double my proving speed for 2^16 constraints from ~50s to ~24s.

I'm unsure of all implications this has for the bindings or o1js in general, but its a significant improvement from my sparse testing.

joo_global_object is deprecated

globalThis should be used.

https://github.com/ocsigen/js_of_ocaml/blob/master/CHANGES.md?plain=1#L159

Rethink use of macros in wasm bindings

          Mmh, I see. I understand your point, and I do agree (I hate macros...). However, IMO, it is good to stay with one convention, and maybe initiate a refactorization after.

Not a blocking comment for this PR, only an opinion.

Originally posted by @dannywillems in #192 (comment)

Would be nice to have at the root of the mina directory a script loading all environment variables related to the project, like the `_build` directory used by dune. I don't see any env var for it.

          Would be nice to have at the root of the mina directory a script loading all environment variables related to the project, like the `_build` directory used by dune. I don't see any env var for it.

Originally posted by @dannywillems in #192 (comment)

Fix compiling with `separate` jsoo mode

It's much faster, but when I try to use it I get this error with a snarkyjs only modified at parts unrelated to this (AFAICT, might be wrong):

[user@nixos:/work/mina/src/lib/snarkyjs/dist/node]$ node
Welcome to Node.js v19.8.1.
Type ".help" for more information.
> snarkyjs = await import("./snarky.js");
/work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:8603
         throw err;
         ^

Error
    at caml_raise_with_string (/work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:849:63)
    at caml_failwith (/work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:1326:7)
    at caml_int_of_string (/work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:6325:29)
    at /work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:802660:24
    at /work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:802666:8
    at Object.<anonymous> (/work/mina/src/lib/snarkyjs/dist/node/_node_bindings/snarky_js_node.bc.cjs:804154:3)
    at Module._compile (node:internal/modules/cjs/loader:1275:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1329:10)
    at Module.load (node:internal/modules/cjs/loader:1133:32)
    at Module._load (node:internal/modules/cjs/loader:972:12)

Node.js v19.8.1

Apple M2 cpus not optimized

Fix here doesn't catch all of the M2 cpus, like Apple M2 Pro etc. I would fix it myself, but I don't know where did those numbers come from.

Can't we get rid of this? Why wasm-bindgen type it as any?

          Can't we get rid of this? Why wasm-bindgen type it as any?

Originally posted by @dannywillems in #160 (comment)

(out-of-scope for this PR): I wish we have this 3 as a constant in a top level file like mod.rs or smth!

          (out-of-scope for this PR): I wish we have this 3 as a constant in a top level file like mod.rs or smth!

Originally posted by @dannywillems in #187 (comment)

bindings.js: use helper for caml/rust options

          I think it would be clearer if we could write

if (is_none(x[4)) { ...} else { ... = f(get_option(x[4]) }

Maybe for another issue ?

Originally posted by @rbonichon in #110 (comment)

Move `kimchi` library to Mina repo under `src/lib/crypto`

Summary

In order to get MinaProtocol/mina#14466 working (and more generally o1-labs/o1js#1096), we need to move the kimchi code that generates the WASM dependencies for o1js into the Mina repo. Part of building o1js depends on the OCaml code which the o1js-stub module covers, but an additional part is the Rust code that generates the WASM needed for Kimchi.

Since Mina will be a submodule of o1js, generating the WASM won't be an issue for o1js, however, since the stub code will live in Mina, there is no way to get the WASM dependencies as of right now.

Thus, we should probably move the Kimchi code into the Mina repo and out of the bindings code.

bindings.js: be more es6 friendly?

          Will be available with a new version of jsoo. Could be changed with https://github.com/MinaProtocol/mina/pull/13777 as jsoo seems now to be more es6 friendly, see https://github.com/ocsigen/js_of_ocaml/pull/1391

Originally posted by @dannywillems in #110 (comment)

Having trouble regenerating

.