-
Notifications
You must be signed in to change notification settings - Fork 583
feat: native prover build #18012
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
base: native/napi
Are you sure you want to change the base?
feat: native prover build #18012
Conversation
9b18de9 to
cef9291
Compare
cef9291 to
1769fde
Compare
…neration-functions feat: proof generation functions (native prover)
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.
Approving already, but please can you solve the question regarding work in progress / local builds? Thanks!
|
|
||
| // Provides: caml_pasta_fq_plonk_proof_create | ||
| // Requires: plonk_wasm, tsRustConversion | ||
| var caml_pasta_fq_plonk_proof_create = function ( |
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.
Okay, this is indeed a strange location for these functions... However why are you only moving this one?
|
|
||
| rm -f "${CURRENT_DIRECTORY}/plonk_napi.node" | ||
| cp "${ARTIFACT}" "${CURRENT_DIRECTORY}/plonk_napi.node" | ||
| napi build \ |
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.
how clean!
| // This file gets auto-included in the generated plonk-napi types to supplement | ||
| // external pointer types. | ||
|
|
||
| type WasmPastaFpPlonkIndex = {}; |
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.
This means every time we have any External<T> we need to include a type T = {} here? Just confirming, looks like what you shared with us during this week call.
| var plonk_napi = require('./plonk_napi.node'); | ||
|
|
||
| module.exports = plonk_napi; | ||
| var plonk_napi = require('@o1js/native-' + process.platform + '-' + process.arch) |
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.
Will this still work for local debugging / work in progress ? Say we are still working on this and the current version is not published anywhere.
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "napi": { | |||
| "binaryName": "plonk_napi", | |||
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.
Actually, this is a TODO: we wanted to rename plonk-wasm into kimchi-wasm, and the same for plonk-napi to kimchi-napi. But let's do that once we have it working to avoid a renaming craziness.
| try { | ||
| var native = require('@o1js/native-' + process.platform + '-' + process.arch) | ||
|
|
||
| function override(functionName) { |
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.
Don't we prefer a helper here to avoid repeating this pattern over and over again as we have more overrides?
| var plonk_wasm = (function() { | ||
| var wasm = require('./plonk_wasm.js'); | ||
|
|
||
| function snakeToCamel(name) { |
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.
Makes sense to delete this already given that napi does this automatically (unlike neon did).
| var plonk_wasm = globalThis.plonk_wasm; | ||
|
|
||
| // Provides: plonk_napi | ||
| var plonk_napi = globalThis.plonk_wasm |
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.
oh sure, in web we still will have wasm
reset to native/napi
adds build and package.json dependency injection for native assets