Enhance Vite configuration and improve memory usage checks#621
Open
Kolezhniuk wants to merge 8 commits into
Open
Enhance Vite configuration and improve memory usage checks#621Kolezhniuk wants to merge 8 commits into
Kolezhniuk wants to merge 8 commits into
Conversation
refactor: change logger parameter to unused in exportFFlonkVk function fix: initialize variables pB and pS in zkey_new.js style: format code for consistency in witness_calculator.cjs test: update test timeouts to use beforeAll and afterAll hooks feat: add Groth16 test suite for zkSNARK verification feat: implement smart contract verifiers for Groth16, PLONK, and FFlonk chore: add Vite configuration for building and testing
- Updated memory usage functions in `groth16_prove.js` and `zkey_new.js` to handle cases where the process object is undefined, ensuring compatibility in non-Node environments. - Modified `vite.config.js` to exclude certain Node-only files from browser builds, preventing unnecessary dependencies from being included in the bundle. - Added a plugin to stub Node-only modules, allowing for cleaner builds and avoiding errors related to missing modules in the browser context. - Disabled minification in the browser build for easier debugging and added rollup options to manage external dependencies more effectively.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the snarkjs toolchain by migrating builds to Vite and tests to Vitest, while also updating key dependencies for ESM compatibility and consolidating/removing legacy subprojects (Rollup configs, Mocha-based test runners, standalone browser/smart-contract test packages).
Changes:
- Replace Rollup-based build pipeline with a multi-mode Vite config (node, CLI, browser ESM, browser IIFE).
- Replace Mocha with Vitest (including Playwright-driven browser tests) and simplify CI to a single Ubuntu job.
- Update various modules for ESM-friendly deep imports (e.g.,
@noble/hashes/*.js), improve memory-usage guards, and remove obsolete test/build subprojects.
Reviewed changes
Copilot reviewed 54 out of 64 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.js | Introduces Vite build modes and Vitest multi-project setup (node, browser, smart-contracts). |
| test/smart_contracts.test.js | Adds Hardhat-based verifier deployment/verification tests under Vitest. |
| test/polynomial.test.js | Updates lifecycle hooks for Vitest (beforeAll/afterAll). |
| test/keypar_test.js | Updates lifecycle hooks for Vitest (beforeAll/afterAll). |
| test/groth16.test.js | Adds a Groth16 test intended to run in both Node and browser contexts. |
| test/fullprocess.js | Updates lifecycle hooks for Vitest (beforeAll/afterAll). |
| test/fflonk.test.js | Updates lifecycle hooks for Vitest and adjusts unused vars naming. |
| test/circuit2/circuit_js/witness_calculator.cjs | Formatting/cleanup adjustments for generated witness calculator helper. |
| test/circuit2/circuit_js/generate_witness.cjs | Formatting/cleanup adjustments for witness generator script. |
| src/zkey_verify_frominit.js | Switches noble-hashes import path for ESM compatibility. |
| src/zkey_new.js | Switches noble-hashes import path; adds safer memory-usage checks. |
| src/zkey_export_verificationkey.js | Renames unused logger parameter to satisfy lint conventions. |
| src/zkey_contribute.js | Switches noble-hashes import path for ESM compatibility. |
| src/zkey_bellman_contribute.js | Switches noble-hashes import path for ESM compatibility. |
| src/zkey_beacon.js | Switches noble-hashes import path for ESM compatibility. |
| src/taskmanager.js | Removes legacy task manager implementation. |
| src/powersoftau_verify.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_utils.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_preparephase2.js | Minor cleanup (unused vars/comment formatting). |
| src/powersoftau_new.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_import.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_export_challenge.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_convert.js | Minor cleanup (remove unused import/comment formatting). |
| src/powersoftau_contribute.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_challenge_contribute.js | Switches noble-hashes import path for ESM compatibility. |
| src/powersoftau_beacon.js | Switches noble-hashes import path for ESM compatibility. |
| src/polynomial/polynomial.js | Comment formatting cleanup + fixes incorrect curve reference in expX. |
| src/plonk_setup.js | Removes unused reads / minor cleanup. |
| src/plonk_prove.js | Import formatting cleanup. |
| src/plonk_exportsoliditycalldata.js | Removes unused helpers/vars (keeps curve load for validation). |
| src/plonk_equation.js | Renames unused parameter to satisfy lint conventions. |
| src/misc.js | Switches noble-hashes import paths for ESM compatibility. |
| src/keypair.js | Switches noble-hashes import path for ESM compatibility. |
| src/Keccak256Transcript.js | Switches noble-hashes import path for ESM compatibility. |
| src/groth16_prove.js | Adds safer memory-usage checks + explicit GC hints. |
| src/fflonk_export_calldata.js | Removes unused helpers/vars (keeps curve load for validation). |
| src/clprocessor.js | Avoids unused err in fallback package.json read logic. |
| smart_contract_tests/test/smart_contracts.test.js | Removes legacy standalone smart-contract test suite. |
| smart_contract_tests/package.json | Removes legacy standalone smart-contract test package definition. |
| smart_contract_tests/hardhat.config.cjs | Removes legacy Hardhat config (CJS) from standalone subproject. |
| smart_contract_tests/.gitignore | Removes legacy ignore rules from standalone subproject. |
| package.json | Updates scripts/deps for Vite+Vitest+Hardhat v3 and adjusts exports/version. |
| hardhat.config.js | Adds ESM Hardhat configuration for consolidated smart-contract tests. |
| eslint.config.js | Migrates ESLint to flat config with Vitest globals and updated ignores. |
| config/rollup.iife.config.js | Removes legacy Rollup IIFE build config. |
| config/rollup.iife_min.config.js | Removes legacy Rollup minified IIFE build config. |
| config/rollup.cli.config.js | Removes legacy Rollup CLI build config. |
| config/rollup.cjs.config.js | Removes legacy Rollup CJS build config. |
| config/rollup.browser.esm.config.js | Removes legacy Rollup browser ESM build config. |
| cli.js | Removes file-level eslint disable (now governed by flat config). |
| browser_tests/test/launch-groth16.js | Removes legacy Puppeteer-based browser test runner. |
| browser_tests/test/groth16.html | Removes legacy HTML-based browser test harness. |
| browser_tests/package.json | Removes legacy browser_tests subproject package definition. |
| browser_tests/package-lock.json | Removes legacy browser_tests lockfile. |
| .gitignore | Ignores Hardhat build outputs and additional local artifacts. |
| .github/workflows/tests.yml | Simplifies CI to Ubuntu + Playwright install + npm test. |
| .eslintrc.cjs | Removes legacy ESLint config (replaced by flat config). |
Files not reviewed (1)
- browser_tests/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+20
| // Deps bundled in browser build (everything except these externals) | ||
| const browserExternal = ["ffjavascript", "@iden3/binfileutils", "r1csfile"]; | ||
| const isBrowserExternal = (id) => | ||
| browserExternal.includes(id) || browserExternal.some((e) => id.startsWith(e + "/")); |
Contributor
Author
There was a problem hiding this comment.
The ESM file is the browser package export, consumed via bundlers that resolve these peer-deps from node_modules — no import map needed. ffjavascript was already external in master, so this isn't new behavior, just two more iden3-owned ESM packages added to that same requirement. Bundling them would risk duplicate copies / version skew when consumers also depend on them. The plain <script type="module"> standalone case is served by the IIFE build (snarkjs.min.js) with globals, not this ESM entry. Keeping them external by design.
Comment on lines
+95
to
+110
| if (mode === "browser-iife") { | ||
| return { | ||
| plugins: [stubNodeOnlyModules], | ||
| build: { | ||
| lib: { | ||
| entry: "./main.js", | ||
| name: "snarkjs", | ||
| formats: ["iife"], | ||
| fileName: () => "snarkjs.min.js", | ||
| }, | ||
| outDir: "build", | ||
| emptyOutDir: false, | ||
| minify: false, | ||
| rollupOptions: { | ||
| external: isBrowserExternal, | ||
| output: { |
Contributor
Author
There was a problem hiding this comment.
Good catch — the name was misleading. Minification had been disabled for debugging; re-enabled minify: true for the IIFE build so snarkjs.min.js is actually minified again (matching prior master behavior).
- Added new benchmarking scripts in package.json for various operations including prove, verify, and compare. - Updated smart contract test file to reference the correct paths for Groth16, Plonk, and Fflonk verifiers. - Enabled minification in vite.config.js for production builds.
…for cleaner output
Member
|
For comparison: I re-applied this migration on top of our optimization stack rather than replaying it, so the MSM/CIOS/memory work is preserved and a couple of bugs are fixed. See #629 (based on our optimization branch, so its diff is the migration alone). Migration-vs-migration diff: feat/esm-migration...feature/esm-tooling Deviations are called out in #629's description. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request makes significant changes to the project's build system, testing setup, and configuration files, focusing on modernizing the toolchain, removing legacy and redundant files, and improving developer experience. The most important changes are the migration from Rollup to Vite for builds, switching from Mocha to Vitest for testing, updating ESLint configuration, and cleaning up old test and config files.
Build system modernization:
package.jsonto use Vite commands for building Node, CLI, browser, and IIFE bundles. The old Rollup config files were removed. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]Testing and CI improvements:
browser_testsandsmart_contract_testssubprojects, including their configs, scripts, and dependencies, consolidating all tests under the main project. [1] [2] [3] [4] [5]Linting and configuration updates:
eslint.config.js, using the latest ESLint and@eslint/js, and updated ignored paths and rules. The old.eslintrc.cjswas removed. [1] [2]@noble/hashes,ffjavascript, and others.General project cleanup:
0.7.6-pocand adjusted theexportsfield inpackage.jsonfor better module resolution. [1] [2]Smart contract testing update:
These updates collectively modernize the development workflow, improve maintainability, and streamline the project's configuration and testing infrastructure.