Upgrade Repo to Latest Package / Compact Versions#364
Draft
Conversation
WalkthroughThis PR updates compiler toolchain versions (0.26.0 → 0.28.0), refactors Compiler and Builder APIs to accept options objects instead of positional parameters, updates runtime dependencies ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
emnul
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 21-25: Check whether the dependency
"@midnight-ntwrk/ledger-v7"@7.0.0 actually exists by running npm view
`@midnight-ntwrk/ledger-v7`@7.0.0 (or checking the upstream repo/tags); if the
version is not published, update package.json to a published, supported version
(e.g., the documented Ledger 4.0.0) or remove/replace the dependency accordingly
and ensure the string "@midnight-ntwrk/ledger-v7" in package.json is changed to
the verified version; commit the corrected package.json and update lockfiles if
applicable.
In `@packages/compact/README.md`:
- Around line 9-15: Update the local-path examples in the README so they point
to the actual package directory (packages/compact) instead of packages/cli:
change the install example that uses
"@openzeppelin/compact-tools-cli@file:./compact-tools/packages/cli" to use the
correct local file path for the compact package, and update the direct
invocation example that calls "node
compact-tools/packages/cli/dist/runCompiler.js" to reference the correct
runCompiler.js location under packages/compact (or the correct relative path to
dist/runCompiler.js); ensure the package name `@openzeppelin/compact-tools-cli`
and the runCompiler.js invocation remain consistent with the package's actual
build output.
- Line 132: Docs claim --skip-zk is not accepted but CompactBuilder.fromArgs
delegates to CompactCompiler.parseArgs which still accepts it; update code to
match docs by rejecting --skip-zk: add validation in CompactBuilder.fromArgs to
detect the --skip-zk flag (or the parsed option from CompactCompiler.parseArgs)
and throw or exit with a clear error, or alternatively strip/ignore the flag
before calling CompactCompiler.parseArgs so the build always includes ZK proofs;
reference CompactBuilder.fromArgs, CompactCompiler.parseArgs and the --skip-zk
flag when making the change.
In `@packages/compact/src/Builder.ts`:
- Around line 13-18: The constructor for class Builder currently accepts
CompilerOptions (constructor(options: CompilerOptions = {})) which allows a
runtime flags field to be passed through even though BuilderOptions omits it;
fix by sanitizing the incoming options before storing/forwarding: in the Builder
constructor destructure or create a new object that explicitly omits the flags
property (e.g., const { flags, ...safeOptions } = options) and assign
this.options = safeOptions (type BuilderOptions) and pass safeOptions into
CompactCompiler, or alternatively change the constructor signature to accept
BuilderOptions instead of CompilerOptions so flags can never be provided.
- Around line 101-107: The constructor for CompactBuilder currently accepts
CompilerOptions allowing disallowed flags; change the CompactBuilder constructor
signature to accept options: BuilderOptions = {} (using the BuilderOptions type
that excludes flags) and update fromArgs to call CompactCompiler.parseArgs(args,
env) then strip or ignore any flags (e.g., remove a flags property or filter out
--skip-zk/SKIP_ZK) before passing the sanitized options into new
CompactBuilder(...); ensure references to CompactBuilder, BuilderOptions,
CompactCompiler.parseArgs, and fromArgs are used so the constructor and factory
path enforce the BuilderOptions constraint.
In `@packages/simulator/test/setup.ts`:
- Around line 26-85: The catch in compileContract currently uses isSpawnSyncRet
(which checks SpawnSyncReturns) but execAsync/exec errors use an ExecException
with a code property; update the catch in compileContract to detect exec errors
by checking err is an object and has a code (e.g. (err as any).code === 'ENOENT'
or numeric 127) instead of relying on isSpawnSyncRet, and when detected throw
the friendly '`compact` not found...' Error; leave other errors re-thrown.
Reference: compileContract, execAsync, isSpawnSyncRet, and use the error.code
check for ExecException handling.
🧹 Nitpick comments (1)
packages/compact/src/Compiler.ts (1)
609-647: Consider adding validation for+version flag without a value.The argument parsing handles
--dir,--src, and--outflags with value validation (throwing if no value follows), but the version flag (+<version>) at line 639 doesn't validate that there's actually a version string after the+. If a user passes just+with nothing after,versionwould be set to an empty string.This is a minor edge case, but for consistency with other flag validation:
💡 Optional: Add validation for empty version string
} else if (args[i].startsWith('+')) { - options.version = args[i].slice(1); + const ver = args[i].slice(1); + if (!ver) { + throw new Error('+<version> flag requires a version number (e.g., +0.28.0)'); + } + options.version = ver; } else {
Comment on lines
+9
to
+15
| ```bash | ||
| # As a local dependency | ||
| yarn add @openzeppelin/compact-tools-cli@file:./compact-tools/packages/cli | ||
|
|
||
| # Or invoke directly after building | ||
| node compact-tools/packages/cli/dist/runCompiler.js | ||
| ``` |
There was a problem hiding this comment.
Fix local-path examples if the package lives under packages/compact.
The README resides in packages/compact, but the local install and dev instructions point to packages/cli. If the package directory is packages/compact, these paths will mislead users.
✏️ Suggested doc fix
- yarn add `@openzeppelin/compact-tools-cli`@file:./compact-tools/packages/cli
+ yarn add `@openzeppelin/compact-tools-cli`@file:./compact-tools/packages/compact
- node compact-tools/packages/cli/dist/runCompiler.js
+ node compact-tools/packages/compact/dist/runCompiler.js
- cd packages/cli
+ cd packages/compactAlso applies to: 218-231
🤖 Prompt for AI Agents
In `@packages/compact/README.md` around lines 9 - 15, Update the local-path
examples in the README so they point to the actual package directory
(packages/compact) instead of packages/cli: change the install example that uses
"@openzeppelin/compact-tools-cli@file:./compact-tools/packages/cli" to use the
correct local file path for the compact package, and update the direct
invocation example that calls "node
compact-tools/packages/cli/dist/runCompiler.js" to reference the correct
runCompiler.js location under packages/compact (or the correct relative path to
dist/runCompiler.js); ensure the package name `@openzeppelin/compact-tools-cli`
and the runCompiler.js invocation remain consistent with the package's actual
build output.
| compact-builder [options] | ||
| ``` | ||
|
|
||
| Accepts all compiler options except `--skip-zk` (builds always include ZK proofs). |
There was a problem hiding this comment.
Builder CLI docs conflict with current parsing behavior.
CompactBuilder.fromArgs delegates to CompactCompiler.parseArgs, so --skip-zk is still accepted. Either disallow it in code or update the docs to match current behavior.
✏️ Suggested doc fix
-Accepts all compiler options except `--skip-zk` (builds always include ZK proofs).
+Accepts all compiler options (including `--skip-zk`), since the builder delegates to the compiler.📝 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.
Suggested change
| Accepts all compiler options except `--skip-zk` (builds always include ZK proofs). | |
| Accepts all compiler options (including `--skip-zk`), since the builder delegates to the compiler. |
🤖 Prompt for AI Agents
In `@packages/compact/README.md` at line 132, Docs claim --skip-zk is not accepted
but CompactBuilder.fromArgs delegates to CompactCompiler.parseArgs which still
accepts it; update code to match docs by rejecting --skip-zk: add validation in
CompactBuilder.fromArgs to detect the --skip-zk flag (or the parsed option from
CompactCompiler.parseArgs) and throw or exit with a clear error, or
alternatively strip/ignore the flag before calling CompactCompiler.parseArgs so
the build always includes ZK proofs; reference CompactBuilder.fromArgs,
CompactCompiler.parseArgs and the --skip-zk flag when making the change.
Comment on lines
+13
to
+18
| /** | ||
| * Configuration options for the Builder CLI. | ||
| * Inherits from CompilerOptions but excludes `flags` (which would allow --skip-zk). | ||
| * Builds should always include ZK proofs. | ||
| */ | ||
| export type BuilderOptions = Omit<CompilerOptions, 'flags'>; |
There was a problem hiding this comment.
Type mismatch allows flags to leak through despite BuilderOptions excluding them.
The documentation states that BuilderOptions omits flags because "Builds should always include ZK proofs." However, there's an inconsistency:
- Line 66:
private readonly options: BuilderOptions(excludesflags) - Line 89:
constructor(options: CompilerOptions = {})(includesflags)
Since the constructor accepts CompilerOptions, users can pass flags: '--skip-zk', which would then be passed to CompactCompiler at line 118. The assignment this.options = options implicitly drops the flags field due to the BuilderOptions type, but the object still contains it at runtime.
Consider either:
- Explicitly omitting
flagswhen storing options, or - Changing the constructor parameter type to
BuilderOptions
🛡️ Option 1: Explicitly omit flags
- constructor(options: CompilerOptions = {}) {
- this.options = options;
+ constructor(options: CompilerOptions = {}) {
+ const { flags: _ignored, ...builderOptions } = options;
+ this.options = builderOptions;
}🛡️ Option 2: Change parameter type
- constructor(options: CompilerOptions = {}) {
+ constructor(options: BuilderOptions = {}) {
this.options = options;
}Also applies to: 66-66, 89-91
🤖 Prompt for AI Agents
In `@packages/compact/src/Builder.ts` around lines 13 - 18, The constructor for
class Builder currently accepts CompilerOptions (constructor(options:
CompilerOptions = {})) which allows a runtime flags field to be passed through
even though BuilderOptions omits it; fix by sanitizing the incoming options
before storing/forwarding: in the Builder constructor destructure or create a
new object that explicitly omits the flags property (e.g., const { flags,
...safeOptions } = options) and assign this.options = safeOptions (type
BuilderOptions) and pass safeOptions into CompactCompiler, or alternatively
change the constructor signature to accept BuilderOptions instead of
CompilerOptions so flags can never be provided.
Comment on lines
+101
to
107
| static fromArgs( | ||
| args: string[], | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ): CompactBuilder { | ||
| const options = CompactCompiler.parseArgs(args, env); | ||
| return new CompactBuilder(options); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usage of fromArgs and CompactBuilder initialization
rg -n "CompactBuilder\.fromArgs|new CompactBuilder" --type ts -B2 -A5Repository: OpenZeppelin/compact-contracts
Length of output: 2204
🏁 Script executed:
#!/bin/bash
# Find the CompactCompiler.parseArgs implementation
fd "CompactCompiler" --type ts | head -20Repository: OpenZeppelin/compact-contracts
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check for skip-zk handling
rg -n "skip-zk|SKIP_ZK" --type ts -B2 -A2Repository: OpenZeppelin/compact-contracts
Length of output: 37816
🏁 Script executed:
#!/bin/bash
# Find BuilderOptions interface definition
rg -n "BuilderOptions|interface.*Options" packages/compact/src/Builder.ts -B2 -A5Repository: OpenZeppelin/compact-contracts
Length of output: 671
🏁 Script executed:
#!/bin/bash
# Check CompactBuilder constructor in Builder.ts
sed -n '85,95p' packages/compact/src/Builder.tsRepository: OpenZeppelin/compact-contracts
Length of output: 448
Constructor type signature violates BuilderOptions design.
The BuilderOptions type (line 18) explicitly excludes flags to prevent --skip-zk, but the constructor (line 89) accepts CompilerOptions, allowing flags through. The fromArgs method passes CompactCompiler.parseArgs() output directly without filtering, meaning SKIP_ZK=true or --skip-zk arguments can bypass the intended constraint.
Fix: Change constructor parameter to options: BuilderOptions = {} and filter flags in fromArgs before instantiation.
🤖 Prompt for AI Agents
In `@packages/compact/src/Builder.ts` around lines 101 - 107, The constructor for
CompactBuilder currently accepts CompilerOptions allowing disallowed flags;
change the CompactBuilder constructor signature to accept options:
BuilderOptions = {} (using the BuilderOptions type that excludes flags) and
update fromArgs to call CompactCompiler.parseArgs(args, env) then strip or
ignore any flags (e.g., remove a flags property or filter out --skip-zk/SKIP_ZK)
before passing the sanitized options into new CompactBuilder(...); ensure
references to CompactBuilder, BuilderOptions, CompactCompiler.parseArgs, and
fromArgs are used so the constructor and factory path enforce the BuilderOptions
constraint.
Comment on lines
+26
to
+85
| function isSpawnSyncRet( | ||
| err: unknown, | ||
| ): err is SpawnSyncReturns<string | Buffer> { | ||
| if (typeof err !== 'object' || err === null) { | ||
| return false; | ||
| } | ||
|
|
||
| const typedErr = err as Partial<SpawnSyncReturns<string | Buffer>> & | ||
| Record<string, unknown>; | ||
|
|
||
| const okErr = typedErr.error instanceof Error; | ||
| const okStdout = | ||
| typeof typedErr.stdout === 'string' || Buffer.isBuffer(typedErr.stdout); | ||
| const okStderr = | ||
| typeof typedErr.stderr === 'string' || Buffer.isBuffer(typedErr.stderr); | ||
| const okStatus = | ||
| typeof typedErr.status === 'number' || typedErr.status === null; | ||
|
|
||
| return okErr && okStdout && okStderr && okStatus; | ||
| } | ||
|
|
||
| async function compileContract(contractFile: string): Promise<void> { | ||
| const inputPath = join(SAMPLE_CONTRACTS_DIR, contractFile); | ||
| const contractName = contractFile.replace('.compact', ''); | ||
| const outputDir = join(ARTIFACTS_DIR, contractName); | ||
| const contractArtifact = join(outputDir, 'contract', 'index.js'); | ||
|
|
||
| // Skip if artifact already exists and is newer than source | ||
| if (existsSync(contractArtifact) && existsSync(inputPath)) { | ||
| const artifactTime = statSync(contractArtifact).mtime; | ||
| const sourceTime = statSync(inputPath).mtime; | ||
| if (artifactTime >= sourceTime) { | ||
| console.log(`✓ ${contractFile} (already compiled)`); | ||
| return; // Already compiled and up to date | ||
| } | ||
| } | ||
|
|
||
| if (!existsSync(inputPath)) { | ||
| throw new Error(`Contract file not found: ${inputPath}`); | ||
| } | ||
|
|
||
| // Ensure output directory and keys subdirectory exist | ||
| mkdirSync(outputDir, { recursive: true }); | ||
| mkdirSync(join(outputDir, 'keys'), { recursive: true }); | ||
|
|
||
| const command = `compact compile --skip-zk "${inputPath}" "${outputDir}"`; | ||
| try { | ||
| await execAsync(command); | ||
| } catch (err: unknown) { | ||
| if (!isSpawnSyncRet(err)) { | ||
| throw err; | ||
| } | ||
|
|
||
| if (err.status === 127) { | ||
| throw new Error( | ||
| '`compact` not found (exit code 127). Is it installed and on PATH?', | ||
| ); | ||
| } | ||
|
|
||
| throw err; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/simulator/test/setup.ts | head -100Repository: OpenZeppelin/compact-contracts
Length of output: 3831
🌐 Web query:
Node.js child_process exec error handling ExecException code status command not found
💡 Result:
In child_process.exec(cmd, cb), “command not found” is usually not a Node error—it’s the shell returning a non‑zero exit code, and Node surfaces that as an Error (often referred to as an ExecException) in the callback. The key fields to inspect are:
error.code: process exit code (non‑zero means failure)stderr: shell’s error text (often contains “command not found”)error.signal: if it was terminated by a signal (otherwise typicallynull) (r2.nodejs.org)
Typical cases
1) POSIX (macOS/Linux): command not found → exit code 127
const { exec } = require('node:child_process');
exec('does-not-exist', (error, stdout, stderr) => {
if (error) {
console.log('exit code:', error.code); // often 127
console.log('stderr:', stderr); // "... command not found"
return;
}
});Exit code 127 for “command not found” is commonly observed when running via a POSIX shell. (codestudy.net)
2) spawn / execFile: missing executable → ENOENT (not a numeric exit code)
If you use spawn()/execFile() and the binary can’t be located, Node can fail before a child process even starts, producing:
error.code === 'ENOENT'error.syscall === 'spawn'
Common causes: binary not on PATH, wrong working directory, or on Windows calling something that exists only as a .bat/.cmd without going through cmd.exe. (nodejs.org)
3) Windows gotchas (.bat/.cmd and PATH)
.bat/.cmdfiles generally need a shell (exec()), or explicitlycmd.exe /c your-script.cmd. (nodejs.org)- If standard commands (like
ipconfig) “aren’t found”, it can be a brokenPATH(e.g., missingSystem32). (stackoverflow.com) - Some commands resolve in
cmd.exeinteractively (likejekyll→jekyll.bat), but Nodespawn/execFilewon’t auto-resolve that; you may need to call the.batdirectly (or useexec). (matthew-jackson.com)
Practical error-handling pattern (exec)
exec(command, (error, stdout, stderr) => {
if (!error) return;
// Shell-level failure (non-zero exit code)
console.error({ code: error.code, signal: error.signal });
console.error('stderr:', stderr);
// If you expected "command not found":
// - POSIX: code often 127
// - Windows: message often "'xxx' is not recognized..."
});If you share your OS + the exact error object (all fields) + the command string, I can tell which of the above cases you’re hitting and the most direct fix.
Citations:
- 1: https://r2.nodejs.org/docs/v24.0.1/api/child_process.html?utm_source=openai
- 2: https://www.codestudy.net/blog/use-child-process-execsync-but-keep-output-in-console/?utm_source=openai
- 3: https://nodejs.org/download/release/v0.10.38/docs/api/all.html?utm_source=openai
- 4: https://nodejs.org/download/release/v5.6.0/docs/api/child_process.html?utm_source=openai
- 5: https://stackoverflow.com/questions/19858385/nodejs-child-process-doesnt-find-commands?utm_source=openai
- 6: https://matthew-jackson.com/blog/node-child_process-enoent-error-windows/?utm_source=openai
🌐 Web query:
Node.js exec() function what error object properties code exit
💡 Result:
For child_process.exec(command, cb), the callback is cb(error, stdout, stderr).
error (when non-null) commonly used properties
error.code: the process exit code (number). By convention, anything other than0indicates an error. [1]error.signal: the signal name (string) that terminated the process (if it was killed by a signal). [1]
Also, since it’s an Error, it has the usual error.message, error.name, error.stack, etc.
If you want “exit code” without using exec’s error
Listen to the returned ChildProcess:
'exit'event handler receives(code, signal):codeis the exit code (ornullif terminated by signal)signalis the signal (ornullif exited normally) [2]
Promisified exec
If you use util.promisify(exec), a rejection gives the same error plus extra error.stdout and error.stderr. [1]
Sources: Node.js child_process docs (exec + promisified exec) [1], Node.js ChildProcess 'exit' event docs [2].
Fix exec error handling to detect "command not found."
The current code uses isSpawnSyncRet() to check for SpawnSyncReturns (from spawnSync()), but exec() errors have a different structure. Promisified exec() errors have an error.code property, not error.status, and don't have a nested error.error property. The guard will fail for all exec() errors, causing them to be re-thrown before the status === 127 check executes.
🐛 Proposed fix (use error.code instead of error.status)
-import { exec, type SpawnSyncReturns } from 'node:child_process';
+import { exec } from 'node:child_process';
@@
-function isSpawnSyncRet(
- err: unknown,
-): err is SpawnSyncReturns<string | Buffer> {
+function isExecError(
+ err: unknown,
+): err is Error & { code?: number | string; stdout?: string | Buffer; stderr?: string | Buffer } {
if (typeof err !== 'object' || err === null) {
return false;
}
-
- const typedErr = err as Partial<SpawnSyncReturns<string | Buffer>> &
- Record<string, unknown>;
-
- const okErr = typedErr.error instanceof Error;
- const okStdout =
- typeof typedErr.stdout === 'string' || Buffer.isBuffer(typedErr.stdout);
- const okStderr =
- typeof typedErr.stderr === 'string' || Buffer.isBuffer(typedErr.stderr);
- const okStatus =
- typeof typedErr.status === 'number' || typedErr.status === null;
-
- return okErr && okStdout && okStderr && okStatus;
+ return 'code' in err;
}
@@
} catch (err: unknown) {
- if (!isSpawnSyncRet(err)) {
+ if (!isExecError(err)) {
throw err;
}
- if (err.status === 127) {
+ if (err.code === 127) {
throw new Error(
'`compact` not found (exit code 127). Is it installed and on PATH?',
);
}🤖 Prompt for AI Agents
In `@packages/simulator/test/setup.ts` around lines 26 - 85, The catch in
compileContract currently uses isSpawnSyncRet (which checks SpawnSyncReturns)
but execAsync/exec errors use an ExecException with a code property; update the
catch in compileContract to detect exec errors by checking err is an object and
has a code (e.g. (err as any).code === 'ENOENT' or numeric 127) instead of
relying on isSpawnSyncRet, and when detected throw the friendly '`compact` not
found...' Error; leave other errors re-thrown. Reference: compileContract,
execAsync, isSpawnSyncRet, and use the error.code check for ExecException
handling.
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
1 participant
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.
Summary by CodeRabbit
New Features
Dependency Updates
API Improvements