Bump deps to prerelease, add ecdsa tests by andrew-fleming · Pull Request #145 · OpenZeppelin/compact-tools

andrew-fleming · 2026-08-19T18:37:42Z

Targeting beta branch with unstable prerelease deps and necessary changes. This PR also includes ecdsa tests from the new deps

coderabbitai · 2026-08-19T18:37:56Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3150722-82d6-4008-9a99-f4295088de22

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

0xisk · 2026-08-20T10:23:10Z

      // Query ctx
      const modifiedTxCtx: QueryContext = {
-        ...ctx.currentQueryContext,
+        ...ctx.callContext.currentQueryContext,


🔴 blocking: the newCtx literal just below (lines 142-149) is still the pre-0.18 flat shape. Under 0.18 CircuitContext requires callContext, queryContexts, gasCosts, callProofDataTrace and events, so that object is a type error (TS2353: 'currentQueryContext' does not exist in type 'CircuitContext<…>', reproduced by compiling the same shape under src/). It passes at runtime only because toEqual(newCtx) compares two plain objects, so the only test covering setContext now asserts nothing about a context the runtime would accept.

Unblocks: rebuild newCtx via createCircuitContext (or copyCircuitContext(ctx) plus the override) so the assertion runs against a real 0.18 context.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:11Z

      // Query ctx
      const modifiedTxCtx: QueryContext = {
-        ...ctx.currentQueryContext,
+        ...ctx.callContext.currentQueryContext,


🔴 blocking: test/** is never typechecked, which is why the broken literal above ships green. packages/simulator/tsconfig.types.json sets include: ["src/**/*"], and its exclude lists "tests" while the directory is actually test. This PR hand-migrates five test files to a new context shape with no type verification, and one of them is already wrong.

Unblocks: prove test/** compiles under 0.18. Cheapest route is a tsconfig.test.json wired into the types turbo task.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:13Z

+    // (`callContext` + per-contract `queryContexts`/`gasCosts`). Build it via
+    // the runtime's `createCircuitContext` factory rather than a hand-rolled
+    // literal so every required field is populated correctly.
+    this.context = createCircuitContext<P>(


🔴 blocking: this drops the optional time arg, so simulated block time changes from a fixed 0 to wall-clock. Upstream defaults it to Math.floor(Date.now() / 1000) and stamps it into initialQueryContext.block.secondsSinceEpoch; under 0.16 the hand-rolled new QueryContext(...) left it at 0n (verified against the installed 0.16). Every downstream suite reading kernel.blockTime() flips to a non-reproducible clock, with no option, no test and no note in the PR body. useCircuitContextSender compounds it: it drops the existing callContext.time and re-stamps mid-flow.

Unblocks, any one of:

pass time explicitly (0 preserves today's behaviour),

add SimulatorOptions.time defaulting to 0, thread it through init() and useCircuitContextSender, and pin it with a test,

or confirm wall-clock is intended and document it as a breaking behaviour change for consumers.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:14Z

-  public context: CircuitContext<P>;
+  // Assigned by the async `init()`; the manager is always constructed and then
+  // awaited (`init`) before any circuit call reads the context.
+  public context!: CircuitContext<P>;


🔴 blocking: two-phase construction with no guard. CircuitContextManager is a barrel export, so an external caller who skips init() gets Cannot read properties of undefined (reading 'callContext') instead of a useful message. Pre-PR the constructor always returned a usable object; this PR is what makes it constructible-but-unusable.

Unblocks: back context with a getter that throws "CircuitContextManager: call init() before use". Same applies to contractAddress! in createDrySimulator.ts.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:15Z

 * which includes private state, public (ledger) state, zswap local state, and transaction context.
 */
+/** Shape of a compiled contract's constructor result (sync or async in 0.18). */
+type InitialStateResult<P> = {


🔴 blocking: this local InitialStateResult exists because IMinimalContract.initialState (src/types/Contract.ts:22-32) still declares the sync-only return type. The canonical interface now lies about 0.18, and anything else typed against it gets the wrong shape.

Unblocks: move the T | Promise<T> union onto IMinimalContract and have this file reuse it rather than redeclaring.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:16Z

+      if: ${{ inputs.skip-compact != 'true' }}
+      shell: bash
+      env:
+        VER: ${{ inputs.compact-prerelease }}


🔴 blocking: the compact-prerelease input is non-functional as written. VER is scoped to this step, and test/setup.ts:55 reads process.env.COMPACTC_VERSION, which nothing sets. Override the input and CI installs one toolchain while the tests compile with the hardcoded +0.33.0-rc.2.

Unblocks: export it, e.g. echo "COMPACTC_VERSION=$VER" >> "$GITHUB_ENV" in this step.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:17Z

+        [ -z "$PLAT" ] && PLAT="$(uname -m)-unknown-linux-musl"
+        DEST="$HOME/.compact/versions/$VER/$PLAT"
+        mkdir -p "$DEST"
+        curl -fsSL -o /tmp/compactc.zip \


🔴 blocking (downgrade if the team disagrees): the pre-release compiler is downloaded, chmod +x'd and executed with no integrity check. Every other action in this file is pinned by commit SHA and the repo runs OpenSSF scorecard, so this is a step down in posture on the one artifact that runs arbitrary code in CI.

Unblocks: pin a sha256 next to compact-prerelease and sha256sum -c before unzip. If the release publishes no checksum, say so and I'll drop this to a followup.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:18Z

+  // (compactc 0.31.x) emits code expecting compact-runtime 0.16.0, which the
+  // 0.18.0-rc.1 runtime this package now depends on rejects at load time.
+  // Override via COMPACTC_VERSION if a newer pinned toolchain is installed.
+  const compilerVersion = process.env.COMPACTC_VERSION ?? '0.33.0-rc.2';


🔴 blocking: the compiler version is now part of what the artifacts depend on, but not part of the cache key. The staleness check above (lines 33-41) compares artifact mtime against source mtime only, so anyone with an existing clone keeps their 0.31-compiled artifacts and hits a load-time rejection against the 0.18 runtime. CI is unaffected (the test turbo task is cache: false and the checkout is clean), so this lands on every local dev instead.

Unblocks: write compilerVersion (and the zkir flag) into outputDir and recompile on mismatch.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:19Z

-          const result = fn(context(), ...args).result;
+          ) => { result: unknown } | Promise<{ result: unknown }>;
+          // 0.18 circuits are async; `await` also tolerates the older sync shape.
+          const { result } = await fn(context(), ...args);


❔ question: does 0.33-generated circuit code copy the context before executing? The pure proxy is wired with () => this.circuitContext (createDrySimulator.ts:105), so circuits receive the live base context, and 0.18's finalizeCallProofData pushes into callProofDataTrace in place. If generated code does not copyCircuitContext first, every circuits.pure.* call permanently appends trace, events and gas to the persistent context even though its result context is discarded. I could not get a 0.33-compiled artifact to check this.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:20Z


-    this.context = {
-      currentPrivateState,
+    // compact-runtime 0.18 restructured `CircuitContext` into a call-tree


❔ question (non-blocking): heads-up on an upstream quirk this comment glosses. createCircuitContext calls createCallContext twice and seeds queryContexts[addr] / gasCosts[addr] from the first while callContext is the second, so context.queryContexts[addr] !== context.callContext.currentQueryContext from birth. Nothing here reads the former today. Worth confirming that still holds if the simulator grows a cross-contract path.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:21Z

+} from '../fixtures/sample-contracts/witnesses/EcdsaWitnesses';
+
+/** Runtime shape the generated circuits accept for an ECDSA signature. */
+export type Secp256k1EcdsaSignature = { r: bigint; s: bigint };


❔ question (non-blocking): Ecdsa.compact:9 re-exports Secp256k1EcdsaSignature and Secp256k1Point "so the generated TS exposes them", but this file hand-rolls { r: bigint; s: bigint } and imports Secp256k1Point from @midnight-ntwrk/compact-runtime. Is the re-export dead, or should the simulator consume the generated types?

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:22Z

+    // compact-runtime 0.18: the caller-scoped fields live on `callContext`.
+    // Copy (shallow-clones `callContext`) before overriding the Zswap local
+    // state so the base context is left untouched.
+    const ctx = copyCircuitContext(baseCtx) as CircuitContext<P>;


🔵 followup (non-blocking): copyCircuitContext is marked @internal in the runtime's typings, so a patch release could drop it without notice. Nothing better to do here (hand-rolling the copy is worse). Worth a note in the comment above and an upstream ask to make it public.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:23Z

+  // compact-runtime 0.18: `createCircuitContext` populates the full call-tree
+  // shape (callContext + queryContexts/gasCosts) and derives an empty Zswap
+  // local state from `sender`.
+  return createCircuitContext<P>(


🔵 followup (non-blocking): neither useCircuitContext nor useCircuitContextSender is referenced anywhere in packages/, and neither is exported from src/index.ts. Checked origin/beta: already dead and unexported before this PR, so deleting it is pre-existing cleanup rather than yours. Flagging only because it got hand-migrated to the call-tree shape untested. Delete it, or export and cover it.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:24Z

   */
  public getPrivateState(): P {
-    return this.circuitContext.currentPrivateState;
+    return this.circuitContext.callContext.currentPrivateState as P;


⚪ nitpick: as P casts over PS | undefined. Harmless once init() has run, but it is the same hole as the missing init guard: before init() this returns undefined typed as P.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:25Z

+     * after construction, before any circuit call. Split out from the
+     * constructor because compact-runtime 0.18 made `initialState` async.
+     */
+    async init(): Promise<this> {


⚪ nitpick: docs went stale with the async migration. Line 15 still calls this the "Internal synchronous simulator primitive"; DryBackend.ts:38-42 still says it "wraps the synchronous result in a resolved promise" and that dry behaviour is preserved "byte-for-byte"; the exported SyncSimulator type name and its (...args) => unknown circuit signature are both misleading now.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:26Z

+  const compilerVersion = process.env.COMPACTC_VERSION ?? '0.33.0-rc.2';
+  // secp256k1 primitives (e.g. secp256k1EcdsaVerify) exist only in the ZKIR v3
+  // backend, so contracts that use them must opt in; others stay on the default.
+  const usesSecp256k1 = /secp256k1/i.test(readFileSync(inputPath, 'utf8'));


⚪ nitpick: this sniffs the raw file, so a contract that mentions secp256k1 only in a comment gets --feature-zkir-v3, and one that pulls it in through an import does not. An explicit per-fixture flag beside CONTRACT_FILES would be less surprising as the fixture set grows.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:27Z

+  for (let i = 0; i < length; i++) {
+    out[i] = Number(v & 0xffn);
+    v >>= 8n;
+  }


⚪ nitpick: byte order matches 0.16 (little-endian, checked against the installed 0.16 casts.js), but the original threw range error … does not fit into n bytes while this silently truncates. Only small counters go through it today, so nothing is wrong now, but a truncating helper in a test can green-light a wrong value later. if (v !== 0n) throw … after the loop restores parity.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-08-20T10:23:28Z

+// A zero-filled byte array of `length`. (Previously `convertFieldToBytes(length,
+// 0n, '')` from compact-runtime 0.16, which was removed in 0.18; a `new
+// Uint8Array` is zero-initialized and behaves identically here.)
+export const zeroUint8Array = (length = 32) => new Uint8Array(length);


⚪ nitpick: third near-identical zero/field-byte helper in the tree, alongside src/signers/Signers.ts:41 and the local convertFieldToBytes in test/integration/SampleZOwnable.test.ts. Worth collapsing into one shared test util.

added by claude (dev3-midnight-basic-review)

0xisk

Thank you @andrew-fleming, looking good! left comments and questions.

Also marked the beta branch as a protected branch.

andrew-fleming added 6 commits July 24, 2026 22:43

add tmp runtime dep

001fc87

update to 0.18 runtime

aa7be1f

wire sim to 0.18-rc/ledger-9 stack

78f2cbb

fix fmt

c61be9d

add ecdsa tests

1f8929f

Merge branch 'main' into simulator-ecdsa-stack

8dca627

andrew-fleming requested review from a team as code owners August 19, 2026 18:37

andrew-fleming requested a review from 0xisk August 19, 2026 18:38

add pre-release compact install

7d179ad

0xisk reviewed Aug 20, 2026

View reviewed changes

Uh oh!

Conversation

andrew-fleming commented Aug 19, 2026

Uh oh!

coderabbitai Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xisk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 19, 2026 •

edited

Loading

0xisk left a comment •

edited

Loading