Conversation
SwenSchaeferjohann
requested review from
ananas-block and
sergeytimoshin
as code owners
WalkthroughThis update introduces protocol versioning to the JavaScript packages and CLI, enabling builds and tests for both V1 and V2. Build scripts, workflows, and test configurations now support explicit version selection. Selective component installation is added for CI, with documentation updates and enhanced error handling and logging for prover processes. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant SetupAction
participant InstallScript
participant Builder
participant Tester
Workflow->>SetupAction: Run setup-and-build (with components)
SetupAction->>InstallScript: ./install.sh --components=[list]
InstallScript-->>SetupAction: Installs selected components
SetupAction-->>Workflow: Setup complete
Workflow->>Builder: Build stateless.js/compressed-token (V1 or V2)
Builder-->>Workflow: Build artifacts
Workflow->>Tester: Run tests (V1 or V2)
Tester-->>Workflow: Test results
alt Prover process fails
Tester->>InstallScript: Find prover logs
InstallScript-->>Tester: Output prover log contents
end
sequenceDiagram
participant Rollup
participant SourceCode
participant Env
Env-->>Rollup: Set LIGHT_PROTOCOL_VERSION
Rollup->>SourceCode: Replace __BUILD_VERSION__ with version
SourceCode-->>Rollup: Compiled output with embedded version
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
* stateless.js add treeinfos * wip * add getTokenPoolInfos * wip * wip * wip - storageoptions * wip * wip * wip * wip - known bug in rpc-interop.test.ts if using random trees * debugged test-rpc in ctoken * all ctoken tests working * rm logs * clean * ctxs to infos, removed redundant getMintProgramId calls * rm deadcode, storageoptions * fix cli getMindProgramId use * fix tokenpool * fix test stateless.js add treeinfos wip add getTokenPoolInfos wip wip wip - storageoptions wip wip wip wip - known bug in rpc-interop.test.ts if using random trees debugged test-rpc in ctoken all ctoken tests working rm logs clean ctxs to infos, removed redundant getMintProgramId calls rm deadcode, storageoptions fix cli getMindProgramId use fix tokenpool fix test update CHANGELOG.md files update changelog update CHANGELOG.md export get-token-pool-infos.ts wip clean wip refactor merklecontext refactor StateTreeInfo wip wip debug test-rpc getCompressedTokenAccountsByOwner fix unit test wip wip wip debug .readUIntLE wip wip wip fix buffer conversion at test-rpc decode wip tests working compressedProof -> validityProof update changelog wip refactor: do not allow output trees for decompress, transfer fmt wip rename getCachedStateTreeInfos upd changelog use with getCachedStateTreeInfos getStateTreeInfos wip wip delegate test wip added transfer-delegated.test.ts added transfer-delegated test cases. all green add tests for decompress-delegated fix rm logs from program update changelog update err msg fix state-tree-luts wip getActiveStateTreeInfos -> getAllStateTreeInfos wip fix sigs for nullifyStateTree wip wip cleanup add getCachedActiveStateTreeInfos mock revert to compressedProof add tokenpools tests update comment update CHANGELOG.md update changelog 0.21.0 update changelog fix fix event parsing post rebase stateless js refactor dedupe types link to computebudgetprogram wip fix DX for instructions: add docstrings to call signatures wip dont break mergeTokenAccounts wip wip add v2 trees to test-rpc v1 mergeable test-rpc tests working rpc-interop tests working with v2 all stateless.js tests working with v2 compressed-token tests working rebase to main fixup cargo lock wip wip (#1794) wip (#1781) chore: backport breaking api changes to v1 js SDKs (#1661) * stateless.js add treeinfos * wip * add getTokenPoolInfos * wip * wip * wip - storageoptions * wip * wip * wip * wip - known bug in rpc-interop.test.ts if using random trees * debugged test-rpc in ctoken * all ctoken tests working * rm logs * clean * ctxs to infos, removed redundant getMintProgramId calls * rm deadcode, storageoptions * fix cli getMindProgramId use * fix tokenpool * fix test stateless.js add treeinfos wip add getTokenPoolInfos wip wip wip - storageoptions wip wip wip wip - known bug in rpc-interop.test.ts if using random trees debugged test-rpc in ctoken all ctoken tests working rm logs clean ctxs to infos, removed redundant getMintProgramId calls rm deadcode, storageoptions fix cli getMindProgramId use fix tokenpool fix test update CHANGELOG.md files update changelog update CHANGELOG.md export get-token-pool-infos.ts wip clean wip refactor merklecontext refactor StateTreeInfo wip wip debug test-rpc getCompressedTokenAccountsByOwner fix unit test wip wip wip debug .readUIntLE wip wip wip fix buffer conversion at test-rpc decode wip tests working compressedProof -> validityProof update changelog wip refactor: do not allow output trees for decompress, transfer fmt wip rename getCachedStateTreeInfos upd changelog use with getCachedStateTreeInfos getStateTreeInfos wip wip delegate test wip added transfer-delegated.test.ts added transfer-delegated test cases. all green add tests for decompress-delegated fix rm logs from program update changelog update err msg fix state-tree-luts wip getActiveStateTreeInfos -> getAllStateTreeInfos wip fix sigs for nullifyStateTree wip wip cleanup add getCachedActiveStateTreeInfos mock revert to compressedProof add tokenpools tests update comment update CHANGELOG.md update changelog 0.21.0 update changelog fix fix event parsing post rebase stateless js refactor dedupe types link to computebudgetprogram wip fix DX for instructions: add docstrings to call signatures wip dont break mergeTokenAccounts wip wip add v2 trees to test-rpc v1 mergeable test-rpc tests working rpc-interop tests working with v2 all stateless.js tests working with v2 compressed-token tests working rebase to main fixup cargo lock wip chore: backport breaking api changes to v1 js SDKs (#1661) * stateless.js add treeinfos * wip * add getTokenPoolInfos * wip * wip * wip - storageoptions * wip * wip * wip * wip - known bug in rpc-interop.test.ts if using random trees * debugged test-rpc in ctoken * all ctoken tests working * rm logs * clean * ctxs to infos, removed redundant getMintProgramId calls * rm deadcode, storageoptions * fix cli getMindProgramId use * fix tokenpool * fix test stateless.js add treeinfos wip add getTokenPoolInfos wip wip wip - storageoptions wip wip wip wip - known bug in rpc-interop.test.ts if using random trees debugged test-rpc in ctoken all ctoken tests working rm logs clean ctxs to infos, removed redundant getMintProgramId calls rm deadcode, storageoptions fix cli getMindProgramId use fix tokenpool fix test update CHANGELOG.md files update changelog update CHANGELOG.md export get-token-pool-infos.ts wip clean wip refactor merklecontext refactor StateTreeInfo wip wip debug test-rpc getCompressedTokenAccountsByOwner fix unit test wip wip wip debug .readUIntLE wip wip wip fix buffer conversion at test-rpc decode wip tests working compressedProof -> validityProof update changelog wip refactor: do not allow output trees for decompress, transfer fmt wip rename getCachedStateTreeInfos upd changelog use with getCachedStateTreeInfos getStateTreeInfos wip wip delegate test wip added transfer-delegated.test.ts added transfer-delegated test cases. all green add tests for decompress-delegated fix rm logs from program update changelog update err msg fix state-tree-luts wip getActiveStateTreeInfos -> getAllStateTreeInfos wip fix sigs for nullifyStateTree wip wip cleanup add getCachedActiveStateTreeInfos mock revert to compressedProof add tokenpools tests update comment update CHANGELOG.md update changelog 0.21.0 update changelog fix fix event parsing post rebase stateless js refactor dedupe types link to computebudgetprogram wip fix DX for instructions: add docstrings to call signatures wip dont break mergeTokenAccounts wip wip add v2 trees to test-rpc v1 mergeable test-rpc tests working rpc-interop tests working with v2 all stateless.js tests working with v2 compressed-token tests working rebase to main fixup cargo lock wip wip rename statetreeinfo -> treeinfo wip wip wip cli test works wip wip wip wip wip wip wip wip wip wip wip wip rm logs fix lint clean upd comment chore: backport breaking api changes to v1 js SDKs (#1661) (#1790) * stateless.js add treeinfos * wip * add getTokenPoolInfos * wip * wip * wip - storageoptions * wip * wip * wip * wip - known bug in rpc-interop.test.ts if using random trees * debugged test-rpc in ctoken * all ctoken tests working * rm logs * clean * ctxs to infos, removed redundant getMintProgramId calls * rm deadcode, storageoptions * fix cli getMindProgramId use * fix tokenpool * fix test stateless.js add treeinfos wip add getTokenPoolInfos wip wip wip - storageoptions wip wip wip wip - known bug in rpc-interop.test.ts if using random trees debugged test-rpc in ctoken all ctoken tests working rm logs clean ctxs to infos, removed redundant getMintProgramId calls rm deadcode, storageoptions fix cli getMindProgramId use fix tokenpool fix test update CHANGELOG.md files update changelog update CHANGELOG.md export get-token-pool-infos.ts wip clean wip refactor merklecontext refactor StateTreeInfo wip wip debug test-rpc getCompressedTokenAccountsByOwner fix unit test wip wip wip debug .readUIntLE wip wip wip fix buffer conversion at test-rpc decode wip tests working compressedProof -> validityProof update changelog wip refactor: do not allow output trees for decompress, transfer fmt wip rename getCachedStateTreeInfos upd changelog use with getCachedStateTreeInfos getStateTreeInfos wip wip delegate test wip added transfer-delegated.test.ts added transfer-delegated test cases. all green add tests for decompress-delegated fix rm logs from program update changelog update err msg fix state-tree-luts wip getActiveStateTreeInfos -> getAllStateTreeInfos wip fix sigs for nullifyStateTree wip wip cleanup add getCachedActiveStateTreeInfos mock revert to compressedProof add tokenpools tests update comment update CHANGELOG.md update changelog 0.21.0 update changelog fix fix event parsing post rebase stateless js refactor dedupe types link to computebudgetprogram wip fix DX for instructions: add docstrings to call signatures wip dont break mergeTokenAccounts wip wip add v2 trees to test-rpc v1 mergeable test-rpc tests working rpc-interop tests working with v2 all stateless.js tests working with v2 compressed-token tests working rebase to main fixup cargo lock wip chore: backport breaking api changes to v1 js SDKs (#1661) * stateless.js add treeinfos * wip * add getTokenPoolInfos * wip * wip * wip - storageoptions * wip * wip * wip * wip - known bug in rpc-interop.test.ts if using random trees * debugged test-rpc in ctoken * all ctoken tests working * rm logs * clean * ctxs to infos, removed redundant getMintProgramId calls * rm deadcode, storageoptions * fix cli getMindProgramId use * fix tokenpool * fix test stateless.js add treeinfos wip add getTokenPoolInfos wip wip wip - storageoptions wip wip wip wip - known bug in rpc-interop.test.ts if using random trees debugged test-rpc in ctoken all ctoken tests working rm logs clean ctxs to infos, removed redundant getMintProgramId calls rm deadcode, storageoptions fix cli getMindProgramId use fix tokenpool fix test update CHANGELOG.md files update changelog update CHANGELOG.md export get-token-pool-infos.ts wip clean wip refactor merklecontext refactor StateTreeInfo wip wip debug test-rpc getCompressedTokenAccountsByOwner fix unit test wip wip wip debug .readUIntLE wip wip wip fix buffer conversion at test-rpc decode wip tests working compressedProof -> validityProof update changelog wip refactor: do not allow output trees for decompress, transfer fmt wip rename getCachedStateTreeInfos upd changelog use with getCachedStateTreeInfos getStateTreeInfos wip wip delegate test wip added transfer-delegated.test.ts added transfer-delegated test cases. all green add tests for decompress-delegated fix rm logs from program update changelog update err msg fix state-tree-luts wip getActiveStateTreeInfos -> getAllStateTreeInfos wip fix sigs for nullifyStateTree wip wip cleanup add getCachedActiveStateTreeInfos mock revert to compressedProof add tokenpools tests update comment update CHANGELOG.md update changelog 0.21.0 update changelog fix fix event parsing post rebase stateless js refactor dedupe types link to computebudgetprogram wip fix DX for instructions: add docstrings to call signatures wip dont break mergeTokenAccounts wip wip add v2 trees to test-rpc v1 mergeable test-rpc tests working rpc-interop tests working with v2 all stateless.js tests working with v2 compressed-token tests working rebase to main fixup cargo lock wip wip rename statetreeinfo -> treeinfo wip wip wip cli test works wip wip wip wip wip wip wip wip wip wip wip wip rm logs fix lint clean upd comment wip stateless.js tests working ctoken tests working lint address comments selective installs fmt v1 and v2 builds and tests working ctoken v1,v2 working rm logs ci
There was a problem hiding this comment.
any particular reason why this is this in the scripts folder?
I feel like it would be more visible at project root.
There was a problem hiding this comment.
we dont need it anymore actually. removing in #1806
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
.github/workflows/js.yml (1)
1-13: 🛠️ Refactor suggestionAdd explicit
permissionsblock to satisfy GitHub security scanner
CodeQL repeatedly flags the absence of apermissionssection. Supplying the minimal read-only scope prevents the defaultwrite-alltoken and silences the alert.name: js-tests-v1 +permissions: + contents: read + pull-requests: read
♻️ Duplicate comments (5)
.github/workflows/cli-v2.yml (2)
1-13: Same missingpermissionsblock as other workflows
See earlier comment injs.yml.
22-24: Job condition suppresses workflow on pushes
Same logic issue described forjs.yml.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 22-87: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}.github/workflows/cli-v1.yml (2)
1-13: Addpermissionsblock
Repeats the issue highlighted injs.yml.
22-24: Condition mistakenly disables job on pushes
Same fix as proposed forjs.yml.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 22-87: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}.github/workflows/js-v2.yml (1)
1-14:⚠️ Potential issueAdd explicit permissions block for GITHUB_TOKEN
This workflow should limitGITHUB_TOKENpermissions to follow best practices and satisfy security checks.on: push: branches: - main pull_request: branches: - "*" types: - opened - synchronize - reopened - ready_for_review +permissions: + contents: read + name: js-tests-v2
🧹 Nitpick comments (20)
cli/src/utils/process.ts (1)
13-49: Risk of dumping huge log files – consider streaming / truncation
logProverFileContents()currently loads each prover log fully into memory and prints it.
If the prover produces multi-MB logs (very common on failure), CI logs will explode and the Node
process may OOM.A minimal improvement:
-const contents = await readFile(filePath, "utf8"); -console.log(contents); +const contents = (await readFile(filePath, "utf8")) + .split("\n") + .slice(-1000) # last 1 000 lines – tweak as desired + .join("\n"); +console.log(contents);Or pipe through
tail -n, but sticking to JS keeps it platform-neutral.scripts/install.sh (1)
369-389: Component names are hard-coded – expose them via help or validation
should_installsilently ignores unknown component names, which can hide typos
(e.g.--components=redeis).
Consider validating the user-supplied list against the known set and failing fast on unknown
entries.scripts/devenv.sh (1)
69-70: Comment wording doesn’t match behaviourThe new comment says “start prover without redis” yet
REDIS_URLis still exported.
If the intention is to make Redis optional, clarify:# Set REDIS_URL; unset it to run the prover without redis support export REDIS_URL="redis://localhost:6379"or remove the comment.
.github/workflows/release.yml (1)
17-18: Hard-coded component list couples workflow to install script – consider using a reusable matrixThe long comma-separated string makes future tweaks error-prone (missing/extra commas, no linting). Since the underlying
install.shsupports selective components, consider expressing them as a YAML sequence and join in the action, e.g.:with: components: ${{ join(fromJSON('["rust","node","pnpm","solana","anchor","jq","keys","dependencies"]'),',') }}Easier to diff, reorder, or toggle.
js/stateless.js/BUILD.md (1)
12-16: Minor nit: emphasise import origin to avoid confusionThe code snippet references
featureFlags.isV2()but doesn’t show the import; readers new to the repo might search for it.```typescript import { featureFlags } from './constants';A quick addition improves self-containment of the doc.
js/stateless.js/test-version.js (2)
6-9: Trim noisy console output in CI
The threeconsole.logstatements add noise to job logs but do not aid assertions. Consider removing or gating them behindDEBUGto keep CI output succinct.
10-12: Compare versions case-insensitively to avoid fragile failures
If callers passexpectedVersion=v2(lowercase) the comparison fails. Normalise both sides once:-const expectedVersion = process.env.EXPECTED_VERSION || 'V1'; -const actualVersion = featureFlags.version.replace(/['"]/g, ''); +const expectedVersion = (process.env.EXPECTED_VERSION || 'V1').toUpperCase(); +const actualVersion = featureFlags.version.replace(/['"]/g, '').toUpperCase();js/stateless.js/tests/unit/version.test.ts (1)
6-11: Drop diagnosticconsole.logcalls from unit tests
Unit-test output should stay minimal; these logs don’t participate in assertions and slow Vitest when captured. Remove them or wrap inif (process.env.DEBUG)..github/actions/setup-and-build/action.yml (2)
33-40: Two steps share the name “Install dependencies” – rename for clarity
Having identical step names makes troubleshooting difficult in the Actions UI. Rename the second step to something like “Run component installer”.
43-47: Quote the components argument to preserve commas and spaces
When the input contains spaces the current bash test works, but the value expansion inside the command can split on spaces. Safe-guard with quotes:./scripts/install.sh --components "${{ inputs.components }}"You already quote the RHS, but also quote inside the
-ntest:if [ -n "${{ inputs.components }}" ]; thenThis avoids word-splitting edge-cases.
js/stateless.js/rollup.config.js (1)
24-27: Avoid double-quoting__BUILD_VERSION__
JSON.stringifyinjects extra quotes which you later strip at runtime. Injecting the raw value simplifies downstream code:-__BUILD_VERSION__: JSON.stringify(process.env.LIGHT_PROTOCOL_VERSION || 'V1') +__BUILD_VERSION__: `'${process.env.LIGHT_PROTOCOL_VERSION || 'V1'}'`This removes the need for
.replace(/['"]/g, '')elsewhere and keeps the constant a plain string literal.scripts/INSTALL.md (1)
7-16: Minor doc nit – add quotes around multi-value examplesSome shells will split on
,before the script gets them. Wrapping the value in single quotes avoids surprises:-./scripts/install.sh --components "node,pnpm,dependencies" +./scripts/install.sh --components 'node,pnpm,dependencies'js/compressed-token/tests/unit/version.test.ts (2)
5-22: Throwing insidebeforeAllaborts the whole suite – prefer an explicit assertionHalting the entire test run masks other potential failures. Consider asserting inside a test instead, which will still fail fast but keep the runner’s reporting consistent:
-beforeAll(() => { - const expectedVersion = - process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1; - const actualVersion = featureFlags.version.replace(/['"]/g, ''); - - if (actualVersion !== expectedVersion) { - throw new Error(/* … */); - } -}); +it('build and runtime versions must match', () => { + const expectedVersion = + process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1; + const actualVersion = featureFlags.version.replace(/['"]/g, ''); + expect(actualVersion).toBe( + expectedVersion, + 'stateless.js must be rebuilt for the requested LIGHT_PROTOCOL_VERSION', + ); +});
24-31: Remove noisy console output in unit testsThe three
console.logstatements add >1 KB to every CI log. Unless they assert something, drop them or gate behindprocess.env.DEBUG.js/stateless.js/src/rpc.ts (1)
342-343: Unused parameter can be removed (or documented as deprecated)
_publicInputHashis not utilised. Keeping dead parameters widens the public API surface and misleads users into thinking the feature exists. Either:
- Remove it from the signature and all call-sites, or
- Mark it
/** @deprecated */so consumers know it’s intentionally unsupported.js/stateless.js/src/constants.ts (1)
18-33: Consider validating env input before casting to theVERSIONenum
process.env.LIGHT_PROTOCOL_VERSIONis blindly asserted asVERSION, so an invalid string (e.g."foo") bypasses the type-safety the enum was meant to provide and will propagate throughfeatureFlags.Not critical, but a guard like:
const envVersion = process.env.LIGHT_PROTOCOL_VERSION?.toUpperCase() as VERSION; if (envVersion === VERSION.V1 || envVersion === VERSION.V2) { return envVersion; }would avoid undefined behaviour.
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Unexpected constant condition.
(lint/correctness/noConstantCondition)
.github/workflows/js-v2.yml (1)
58-61: Enable selective component installation in setup-and-build
Leverage the new opt-in feature to skip unnecessary installs. For example, only install the JS packages and CLI in this job:- name: Setup and build uses: ./.github/actions/setup-and-build + with: + components: stateless.js,compressed-token,zk-compression-clijs/stateless.js/package.json (3)
90-94: Use cross-platform env var handling in package scripts
InlineVAR=…assignments won’t work on Windows. Consider addingcross-envand updating scripts, e.g.:- "test:v1": "LIGHT_PROTOCOL_VERSION=V1 pnpm test", - "test:v2": "LIGHT_PROTOCOL_VERSION=V2 pnpm test", - "test:unit:all:v1": "LIGHT_PROTOCOL_VERSION=V1 vitest run tests/unit --reporter=verbose", - "test:unit:all:v2": "LIGHT_PROTOCOL_VERSION=V2 vitest run tests/unit --reporter=verbose", + "test:v1": "cross-env LIGHT_PROTOCOL_VERSION=V1 pnpm test", + "test:v2": "cross-env LIGHT_PROTOCOL_VERSION=V2 pnpm test", + "test:unit:all:v1": "cross-env LIGHT_PROTOCOL_VERSION=V1 vitest run tests/unit --reporter=verbose", + "test:unit:all:v2": "cross-env LIGHT_PROTOCOL_VERSION=V2 vitest run tests/unit --reporter=verbose",And add
"cross-env": "^7.0.3"todevDependencies.
103-103: Align--bailusage across e2e scripts
Only some tests use--bail=1. For consistent fast feedback, either add--bail=1to all long-running e2e commands or remove it where it’s not needed.
112-115: Ensure cross-platform support for build scripts
The mainbuildscript uses a Bashifand inline env vars, which will fail on Windows. Consider a cross-env approach or a Node-based helper, e.g.:- "build": "if [ \"$LIGHT_PROTOCOL_VERSION\" = \"V2\" ]; then LIGHT_PROTOCOL_VERSION=V2 pnpm build:bundle; else LIGHT_PROTOCOL_VERSION=V1 pnpm build:bundle; fi", + "build": "cross-env-shell \"LIGHT_PROTOCOL_VERSION=${LIGHT_PROTOCOL_VERSION:-V1}\" pnpm build:bundle",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/actions/setup-and-build/action.yml(2 hunks).github/workflows/cli-v1.yml(1 hunks).github/workflows/cli-v2.yml(1 hunks).github/workflows/js-v2.yml(1 hunks).github/workflows/js.yml(2 hunks).github/workflows/release.yml(1 hunks)cli/src/utils/initTestEnv.ts(1 hunks)cli/src/utils/process.ts(2 hunks)cli/src/utils/processProverServer.ts(1 hunks)js/compressed-token/package.json(2 hunks)js/compressed-token/rollup.config.js(3 hunks)js/compressed-token/src/types.ts(0 hunks)js/compressed-token/src/utils/version-check.ts(1 hunks)js/compressed-token/tests/setup/version-check.ts(1 hunks)js/compressed-token/tests/unit/version.test.ts(1 hunks)js/compressed-token/vitest.config.ts(1 hunks)js/stateless.js/.eslintignore(1 hunks)js/stateless.js/BUILD.md(1 hunks)js/stateless.js/package.json(3 hunks)js/stateless.js/rollup.config.js(2 hunks)js/stateless.js/src/constants.ts(2 hunks)js/stateless.js/src/globals.d.ts(1 hunks)js/stateless.js/src/rpc.ts(3 hunks)js/stateless.js/test-version.js(1 hunks)js/stateless.js/tests/unit/version.test.ts(1 hunks)scripts/INSTALL.md(1 hunks)scripts/devenv.sh(1 hunks)scripts/install.sh(3 hunks)
💤 Files with no reviewable changes (1)
- js/compressed-token/src/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
cli/src/utils/initTestEnv.ts (1)
cli/src/utils/processProverServer.ts (1)
startProver(88-145)
js/stateless.js/test-version.js (1)
js/stateless.js/src/constants.ts (1)
featureFlags(16-35)
cli/src/utils/processProverServer.ts (1)
cli/src/utils/process.ts (2)
spawnBinary(198-236)waitForServers(238-265)
js/stateless.js/src/rpc.ts (1)
js/stateless.js/src/constants.ts (1)
featureFlags(16-35)
js/compressed-token/src/utils/version-check.ts (1)
js/stateless.js/src/constants.ts (1)
featureFlags(16-35)
🪛 GitHub Check: CodeQL
.github/workflows/cli-v1.yml
[warning] 22-87: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
.github/workflows/cli-v2.yml
[warning] 22-87: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
.github/workflows/js-v2.yml
[warning] 22-92: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 Biome (1.9.4)
js/stateless.js/src/constants.ts
[error] 20-20: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (9)
js/stateless.js/.eslintignore (1)
4-4:test-version.jslooks fine to ignore – confirm it is only executed via Node, not the linterAdding
test-version.jsto.eslintignoreis sensible if the file is a quick-and-dirty Node entry point.
Just double-check that:
- The file is not supposed to be type-checked / linted (e.g. it’s pure JS with
require()).- All quality gates you still care about (unit tests, type checks) invoke it elsewhere.
If both are true, no further action needed.
scripts/install.sh (1)
393-393: 👍 Forwarding"$@"intomainis the correct fixPassing the script’s CLI args down to
maincloses a long-standing foot-gun.js/compressed-token/vitest.config.ts (1)
15-15: Verify path resolution forglobalSetupVitest resolves
globalSetuprelative to the config file.
'./tests/setup/version-check.ts'looks correct, but a misplaced./vs../will make the test
runner silently skip the script.
Runpnpm vitest --runlocally to ensure the setup file is executed.js/stateless.js/src/globals.d.ts (1)
1-1: Global constant is declared but not surfaced viatypes– make sure consumers can pick it upDeclaring the ambient constant here is fine, but remember that the declaration file must be referenced from every compilation unit that relies on it (e.g. via the
types,typeRoots, orfilesproperty intsconfig.json). Otherwise downstream packages may fail to find__BUILD_VERSION__and fall back toany.Confirm that the stateless.js
tsconfig.jsonalready includessrcin itsincludelist or explicitly lists this.d.tsfile infiles.js/compressed-token/rollup.config.js (1)
27-34: Build-time version injection is implemented correctly
@rollup/plugin-replaceis injected early in the chain, before the TypeScript transform, so the__BUILD_VERSION__constant will be substituted in both TS and emitted JS paths. No further action needed.js/stateless.js/src/rpc.ts (3)
162-164: Correctly gatesproveByIndexon V2 – looks goodReturning
falsefor V1 avoids accessing a non-existent field and keeps legacy behaviour intact.
1068-1068: ConsistentproveByIndexhandling in V1 pathMirrors the earlier fix; no issues spotted.
1893-1896: Explicitfalsefor address proofs is appropriateMaintains the array length parity while signalling the invariant clearly.
.github/workflows/cli-v2.yml (1)
26-36:Details
✅ Verification successful
Verify Redis tag
8.0.1exists
At the time of writing the latest stable image on Docker Hub is 7.x;redis:8.0.1may fail to pull.
🏁 Script executed:
#!/bin/bash # List available Redis tags and grep for 8.0.1 curl -s https://registry.hub.docker.com/v2/repositories/library/redis/tags?page_size=100 | jq -r '.results[].name' | grep -E '^8\.0\.1$' || echo "Redis 8.0.1 tag not found"Length of output: 146
Redis tag verified
The Docker image tagredis:8.0.1exists on Docker Hub as confirmed by the API call—no changes required.
| spawnedProcess.on("close", async (code) => { | ||
| console.log(`${binaryName} process exited with code ${code}`); | ||
| // Log prover file contents if prover exits with non-zero code | ||
| if (code !== 0 && binaryName.includes("prover")) { | ||
| console.error(`Prover process failed with exit code ${code}`); | ||
| await logProverFileContents(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unhandled rejection possible – wrap await logProverFileContents() in try/catch
If logProverFileContents() itself throws, the promise returned by the async listener is
ignored by EventEmitter, leading to an unhandled-rejection warning in Node ≥ 18.
- if (code !== 0 && binaryName.includes("prover")) {
- console.error(`Prover process failed with exit code ${code}`);
- await logProverFileContents();
- }
+ if (code !== 0 && binaryName.includes("prover")) {
+ console.error(`Prover process failed with exit code ${code}`);
+ try {
+ await logProverFileContents();
+ } catch (err) {
+ console.error("Failed to print prover logs:", err);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spawnedProcess.on("close", async (code) => { | |
| console.log(`${binaryName} process exited with code ${code}`); | |
| // Log prover file contents if prover exits with non-zero code | |
| if (code !== 0 && binaryName.includes("prover")) { | |
| console.error(`Prover process failed with exit code ${code}`); | |
| await logProverFileContents(); | |
| } | |
| spawnedProcess.on("close", async (code) => { | |
| console.log(`${binaryName} process exited with code ${code}`); | |
| // Log prover file contents if prover exits with non-zero code | |
| if (code !== 0 && binaryName.includes("prover")) { | |
| console.error(`Prover process failed with exit code ${code}`); | |
| try { | |
| await logProverFileContents(); | |
| } catch (err) { | |
| console.error("Failed to print prover logs:", err); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
In cli/src/utils/process.ts around lines 218 to 224, the await call to
logProverFileContents() inside the "close" event listener is not wrapped in a
try/catch block, which can cause unhandled promise rejections if the function
throws. To fix this, wrap the await logProverFileContents() call in a try/catch
block and handle or log any errors inside the catch to prevent unhandled
rejections.
| --components) | ||
| components="$2" | ||
| install_all=false | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing value check for --components – could shift the wrong arg
Passing --components without a following list will make $2 empty and shift 2 will swallow the
next flag, producing confusing errors.
--components)
- components="$2"
- install_all=false
- shift 2
+ if [[ -z ${2:-} ]]; then
+ echo "--components requires a comma-separated list" >&2
+ exit 1
+ fi
+ components="$2"
+ install_all=false
+ shift 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --components) | |
| components="$2" | |
| install_all=false | |
| shift 2 | |
| ;; | |
| --components) | |
| if [[ -z ${2:-} ]]; then | |
| echo "--components requires a comma-separated list" >&2 | |
| exit 1 | |
| fi | |
| components="$2" | |
| install_all=false | |
| shift 2 | |
| ;; |
🤖 Prompt for AI Agents
In scripts/install.sh around lines 353 to 357, the --components option assigns
$2 to components and shifts 2 without checking if $2 is empty. To fix this, add
a check to verify that $2 is not empty before assigning and shifting. If $2 is
empty, print an error message and exit or handle the missing value appropriately
to avoid swallowing the next argument.
| try { | ||
| // TODO: check if using redisUrl is better here. | ||
| await startProver(proverPort, proverRunMode, circuits); | ||
| } catch (error) { | ||
| console.error("Failed to start prover:", error); | ||
| // Prover logs will be automatically displayed by spawnBinary in process.ts | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nested logging & re-throw leads to duplicated noise
startProver already wraps its own awaitable with a try/catch that logs and re-throws.
Catching again here produces the same stack trace twice:
try {
await startProver(...);
} catch (error) {
console.error("Failed to start prover:", error); // duplicate
throw error;
}Either:
- Remove this outer try/catch, or
- Catch here but omit the second
console.error.
Example minimal change:
- try {
- await startProver(proverPort, proverRunMode, circuits);
- } catch (error) {
- console.error("Failed to start prover:", error);
- throw error;
- }
+ await startProver(proverPort, proverRunMode, circuits);Less log-spam, same failure semantics.
🤖 Prompt for AI Agents
In cli/src/utils/initTestEnv.ts around lines 152 to 159, the outer try/catch
block around startProver duplicates error logging because startProver already
logs and re-throws errors. To fix this, remove the outer try/catch entirely to
avoid redundant logs while preserving the error propagation behavior.
| try { | ||
| validateVersionConsistency(); | ||
| const expectedVersion = process.env.LIGHT_PROTOCOL_VERSION || 'V1'; | ||
| console.log(`✅ Version check passed: Using ${expectedVersion}`); | ||
| } catch (error) { | ||
| console.error('❌ Version check failed:'); | ||
| console.error(error.message); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
catch (error) assumes error.message exists – unsafe with unknown
Under --strict, the caught variable is unknown; accessing message without narrowing is a type error.
- } catch (error) {
- console.error('❌ Version check failed:');
- console.error(error.message);
+ } catch (error) {
+ console.error('❌ Version check failed:');
+ if (error instanceof Error) {
+ console.error(error.message);
+ } else {
+ console.error(error);
+ }Also returns clearer output for non-Error throws (e.g. strings).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| validateVersionConsistency(); | |
| const expectedVersion = process.env.LIGHT_PROTOCOL_VERSION || 'V1'; | |
| console.log(`✅ Version check passed: Using ${expectedVersion}`); | |
| } catch (error) { | |
| console.error('❌ Version check failed:'); | |
| console.error(error.message); | |
| process.exit(1); | |
| } | |
| try { | |
| validateVersionConsistency(); | |
| const expectedVersion = process.env.LIGHT_PROTOCOL_VERSION || 'V1'; | |
| console.log(`✅ Version check passed: Using ${expectedVersion}`); | |
| } catch (error) { | |
| console.error('❌ Version check failed:'); | |
| if (error instanceof Error) { | |
| console.error(error.message); | |
| } else { | |
| console.error(error); | |
| } | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In js/compressed-token/tests/setup/version-check.ts around lines 7 to 15, the
catch block assumes the caught error has a message property, which is unsafe
under strict TypeScript settings where the error is of type unknown. Fix this by
narrowing the error type before accessing error.message, for example, check if
error is an instance of Error and then log error.message; otherwise, log the
error directly to handle non-Error throws safely and provide clearer output.
| #!/usr/bin/env node | ||
|
|
||
| // Test script to verify the compiled version is correctly set | ||
| import { featureFlags } from './dist/cjs/node/index.cjs'; |
There was a problem hiding this comment.
import in a CommonJS-default context will throw – fall back to require or mark the package as ESM
With #!/usr/bin/env node Node assumes CommonJS when type is not set to "module" at the repository root. A top-level import will cause a runtime syntax error in that scenario, breaking CI.
-import { featureFlags } from './dist/cjs/node/index.cjs';
+/* eslint-disable @typescript-eslint/no-var-requires */
+const { featureFlags } = require('./dist/cjs/node/index.cjs');Alternatively, add "type": "module" in the nearest package.json and rename to .mjs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { featureFlags } from './dist/cjs/node/index.cjs'; | |
| /* eslint-disable @typescript-eslint/no-var-requires */ | |
| const { featureFlags } = require('./dist/cjs/node/index.cjs'); |
🤖 Prompt for AI Agents
In js/stateless.js/test-version.js at line 4, the use of a top-level `import`
statement in a CommonJS context causes a runtime syntax error because Node
defaults to CommonJS without `"type": "module"` in package.json. To fix this,
replace the `import` statement with a `require` call to load the module, or
alternatively, add `"type": "module"` to the nearest package.json and rename the
file extension to `.mjs` to enable ES module support.
| const proverProcess = spawnBinary(getProverPathByArch(), args); | ||
|
|
||
| try { | ||
| await waitForServers([{ port: proverPort, path: "/" }]); | ||
| console.log(`Prover started successfully!`); | ||
| } catch (error) { | ||
| console.error( | ||
| "Failed to start prover - prover logs will be displayed above", | ||
| ); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
proverProcess is unused and may be orphaned on failure
The variable is declared but never referenced, triggering no-unused-vars and—more importantly—leaks the child process if waitForServers throws. Capture and clean up:
-const proverProcess = spawnBinary(getProverPathByArch(), args);
+const proverProcess = spawnBinary(getProverPathByArch(), args);- } catch (error) {
+ } catch (error) {
+ try {
+ killProcess(proverProcess.pid);
+ } catch (_) {
+ /* ignore */
+ }
console.error(
"Failed to start prover - prover logs will be displayed above",
);
throw error;
}Add a return of proverProcess if callers need a handle.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/src/utils/processProverServer.ts around lines 134 to 144, the variable
proverProcess is declared but never used, causing a potential child process leak
if waitForServers throws an error. To fix this, ensure you handle proverProcess
properly by adding cleanup logic to terminate the process in the catch block if
waitForServers fails, and return proverProcess at the end of the function so
callers can manage the process handle.
| "test": "pnpm test:e2e:all", | ||
| "test:v1": "LIGHT_PROTOCOL_VERSION=V1 pnpm test", | ||
| "test:v2": "LIGHT_PROTOCOL_VERSION=V2 pnpm test", | ||
| "test-all": "vitest run", | ||
| "test:unit:all": "EXCLUDE_E2E=true vitest run", | ||
| "test:unit:all:v1": "LIGHT_PROTOCOL_VERSION=V1 vitest run tests/unit --reporter=verbose", | ||
| "test:unit:all:v2": "LIGHT_PROTOCOL_VERSION=V2 vitest run tests/unit --reporter=verbose", | ||
| "test-all:verbose": "vitest run --reporter=verbose", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Version-scoped test scripts are recursive – may cause infinite loops
"test:v1": "LIGHT_PROTOCOL_VERSION=V1 pnpm test" calls the default test script, which itself invokes test:e2e:all, eventually executing all scripts again under test:v1|v2 when run via CI matrix. Consider targeting the underlying vitest commands directly to avoid redundant recursion and timeouts.
🤖 Prompt for AI Agents
In js/compressed-token/package.json around lines 80 to 87, the version-scoped
test scripts like "test:v1" call the default "test" script, causing recursive
execution and potential infinite loops. To fix this, modify "test:v1" and
"test:v2" to directly invoke the underlying vitest commands with the appropriate
environment variables instead of calling "pnpm test". This avoids recursion and
redundant test runs.
| stateless-js-v1: | ||
| name: stateless-js-v1 | ||
| if: github.event.pull_request.draft == false | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
if: expression skips the job on push events
github.event.pull_request is undefined for push, so the comparison is falsy and the job is never executed on a direct push to main, despite being configured in the trigger block.
- if: github.event.pull_request.draft == false
+ # Run for all pushes and for non-draft PRs
+ if: github.event_name != 'pull_request' || github.event.pull_request.draft == false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stateless-js-v1: | |
| name: stateless-js-v1 | |
| if: github.event.pull_request.draft == false | |
| runs-on: ubuntu-latest | |
| stateless-js-v1: | |
| name: stateless-js-v1 | |
| # Run for all pushes and for non-draft PRs | |
| if: github.event_name != 'pull_request' || github.event.pull_request.draft == false | |
| runs-on: ubuntu-latest |
🤖 Prompt for AI Agents
In .github/workflows/js.yml around lines 21 to 24, the if condition uses
github.event.pull_request which is undefined for push events, causing the job to
be skipped on direct pushes. Modify the if expression to safely check for
pull_request existence before accessing draft, or adjust the workflow triggers
and conditions to ensure the job runs on both pull_request and push events as
intended.
| export function validateVersionConsistency(expectedVersion?: string): void { | ||
| const expected = | ||
| expectedVersion || process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1; | ||
| const actual = featureFlags.version.replace(/['"]/g, ''); | ||
|
|
||
| if (actual !== expected) { | ||
| throw new Error( | ||
| `Version mismatch detected!\n` + | ||
| `Expected: ${expected} (from ${expectedVersion ? 'parameter' : 'LIGHT_PROTOCOL_VERSION env var'})\n` + | ||
| `Actual: ${actual} (from built stateless.js)\n\n` + | ||
| `This means stateless.js was built with ${actual} but you're trying to use ${expected}.\n` + | ||
| `Please rebuild both packages with the same version:\n` + | ||
| ` pnpm build:${expected.toLowerCase()}\n` + | ||
| `This command will automatically build both stateless.js and compressed-token with ${expected}.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make comparison case-insensitive & strip quotes on both operands
A mismatch such as expected='v2' vs actual='V2' currently throws erroneously. Normalise both strings before comparison.
- const expected =
- expectedVersion || process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1;
- const actual = featureFlags.version.replace(/['"]/g, '');
+ const expectedRaw =
+ expectedVersion || process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1;
+ const expected = expectedRaw.replace(/['"]/g, '').toUpperCase();
+ const actual = featureFlags.version.replace(/['"]/g, '').toUpperCase();
- if (actual !== expected) {
+ if (actual !== expected) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function validateVersionConsistency(expectedVersion?: string): void { | |
| const expected = | |
| expectedVersion || process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1; | |
| const actual = featureFlags.version.replace(/['"]/g, ''); | |
| if (actual !== expected) { | |
| throw new Error( | |
| `Version mismatch detected!\n` + | |
| `Expected: ${expected} (from ${expectedVersion ? 'parameter' : 'LIGHT_PROTOCOL_VERSION env var'})\n` + | |
| `Actual: ${actual} (from built stateless.js)\n\n` + | |
| `This means stateless.js was built with ${actual} but you're trying to use ${expected}.\n` + | |
| `Please rebuild both packages with the same version:\n` + | |
| ` pnpm build:${expected.toLowerCase()}\n` + | |
| `This command will automatically build both stateless.js and compressed-token with ${expected}.`, | |
| ); | |
| } | |
| export function validateVersionConsistency(expectedVersion?: string): void { | |
| const expectedRaw = | |
| expectedVersion || process.env.LIGHT_PROTOCOL_VERSION || VERSION.V1; | |
| const expected = expectedRaw.replace(/['"]/g, '').toUpperCase(); | |
| const actual = featureFlags.version.replace(/['"]/g, '').toUpperCase(); | |
| if (actual !== expected) { | |
| throw new Error( | |
| `Version mismatch detected!\n` + | |
| `Expected: ${expected} (from ${expectedVersion ? 'parameter' : 'LIGHT_PROTOCOL_VERSION env var'})\n` + | |
| `Actual: ${actual} (from built stateless.js)\n\n` + | |
| `This means stateless.js was built with ${actual} but you're trying to use ${expected}.\n` + | |
| `Please rebuild both packages with the same version:\n` + | |
| ` pnpm build:${expected.toLowerCase()}\n` + | |
| `This command will automatically build both stateless.js and compressed-token with ${expected}.`, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In js/compressed-token/src/utils/version-check.ts around lines 10 to 25, the
version comparison between expected and actual is case-sensitive and only strips
quotes from actual. To fix this, normalize both expected and actual by stripping
quotes and converting to the same case (e.g., lowercase) before comparing them.
This ensures the comparison is case-insensitive and avoids erroneous mismatches
like 'v2' vs 'V2'.
| proveByIndices: value.accounts | ||
| .map((r: any) => r.rootIndex.proveByIndex) | ||
| .concat(value.addresses.map((r: any) => false)), | ||
| .concat(value.addresses.map((r: any) => false)), // addresses.proveByIndex is always false. |
There was a problem hiding this comment.
do we have a getValidityProof in ts that resembles the get_validity_proof_v2 response?
Issues with the current response are:
- addresses.proveByIndex is always false
- leaves -> the address in an address proof is not a leaf
- leafIndices -> is not a public input for address proofs
There was a problem hiding this comment.
all V2 endpoints in TS are fully non-breaking for existing clients by maintaining the v1 ts return types
I can add a getValidityProofV2 in a different PR
|
closing in favor of #1806 |
2 participants
install.shallows opt-in selective installs (some workflows do not need redis for example)Summary by CodeRabbit
New Features
Documentation
Tests
Style
Chores