test: migrate unit suites to async backend-aware simulator (dry + live)#620
test: migrate unit suites to async backend-aware simulator (dry + live)#6200xisk wants to merge 10 commits into
Conversation
Migrate every module's per-contract simulator and specs from the old
synchronous createSimulator to the new async, backend-aware API: the
factory now returns a class whose create() is awaited and whose circuits
return promises, so one spec file runs on both backends selected by
MIDNIGHT_BACKEND=dry|live.
* simulators: constructor -> static async create(...); delegating methods
return Promise<R>; private-state injection helpers use the async
getPrivateState/setPrivateState; artifactName added so the live harness
can locate compiled assets + ZK keys.
* specs: new X() -> await X.create(); every circuit/state call awaited;
failure paths use await expect(...).rejects.toThrow(...); caller identity
uses alias strings (as('OWNER')) in access + multisig.
* live: a test:live script, vitest.live.config.ts, a registerLiveBackend
harness (deploy-per-test against the local stack), and a label-gated
live-tests CI workflow; make env-up / local-env.yml bring up node +
indexer + proof server.
* deps: consume @openzeppelin/compact-simulator via portal: against the
sibling compact-tools checkout until the live backend is published.
Validated dry: 28 files, 1183 tests green. Live: proven end-to-end on the
security module against a local node.
Note: the live _harness/ (network/deploy/wallet/ownWallet/providers) is
shared infra that overlaps the in-flight native-shielded-token branch and
should be de-duplicated at merge.
WalkthroughThe PR converts simulator wrappers and their tests to async factory and Promise-based APIs across access, multisig, token, security, and utils. It also adds local Docker Compose, logging, and live Vitest configuration files. ChangesAccess simulator migration
Multisig simulator migration
Token simulator migration
Security and utility simulator migration
Local live environment tooling
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The async backend-aware simulator (OpenZeppelin/compact-tools#121) is now published as @openzeppelin/compact-simulator@0.2.0. Replace the placeholder `portal:` dependency on a sibling compact-tools checkout with the published package, so the default `test` CI can install the dependency and run the dry suite (the `portal:` link could not resolve in CI). Validated dry on the utils, access, token, and security modules (298 tests green) against the published API; the imported surface (createSimulator, registerLiveBackend, LiveBackendRequest, LiveContext, SimulatorOptions) is unchanged from the portal build.
The live backend is now consumed from the published @openzeppelin/compact-simulator, so the sibling compact-tools checkout and `yarn workspace ... build` step are no longer needed to resolve the dependency. Remove both and install against the committed lockfile with `--immutable` (was `--no-immutable`, required only while the `portal:` link had to be re-resolved). Behaviour-only cleanup of the label-gated workflow; the dry `test` CI is unaffected.
Rescope this PR to the dry async unit-test migration only. Remove the live-backend wiring (test/integration/_harness, the vitest.live.config.ts, the test:live script, Makefile + local-env.yml) and the label-gated live-tests workflow. The live workflow returns in a follow-up PR (as a sharded GitHub-hosted matrix, since a single job caps at 6h). Also drop the pre-existing integration suite (test/integration specs, fixtures, _mocks, vitest.integration.config.ts, the compact:integration and test:integration scripts): it uses the old synchronous simulator API and does not run against the async @openzeppelin/compact-simulator 0.2.0, and it is not wired into CI. It will be re-added, migrated, alongside the live workflow. What remains: the per-module simulator + spec async migration and the @openzeppelin/compact-simulator ^0.2.0 pin.
0xisk
changed the title
Restore the test/integration specs, fixtures, _mocks, vitest.integration.config.ts, and the compact:integration / test:integration scripts that the previous commit removed. They predate this PR (they live on main) and are not part of this migration, so the PR leaves them untouched. Only the live-backend wiring stays removed and deferred to a follow-up PR: test/integration/_harness, vitest.live.config.ts, the test:live script, Makefile, local-env.yml, and the live-tests workflow.
Bring back the local live-backend testing setup removed earlier and extend it to the integration suite. The live CI workflow stays out (it returns in a follow-up PR as a sharded matrix). * Restore the local stack (Makefile + local-env.yml), vitest.live.config.ts, the test/integration/_harness live wiring, and the test:live script — so `make env-up && yarn test:live` runs the unit specs on a real node. * Migrate the integration suite (composedTokens + sharedInitCollision fixtures, initStateIsolation spec) to the async backend-aware simulator, matching the unit migration: static create, awaited circuits, rejects/resolves assertions. Add artifactName so the live harness can deploy each composed contract. * Add integration live wiring: vitest.integration.live.config.ts, plus compact:integration:live (real ZK keys, no SKIP_ZK) and test:integration:live. The integration suite shares the unit live harness unchanged — the harness deploys any contract by artifactName, so no live-backend code is added. Validated dry: integration spec 5/5 green against the async API.
0xisk
changed the title
The integration suite is out of scope for this PR. Revert the integration fixtures (composedTokens, sharedInitCollision) and the initStateIsolation spec to their main versions, and remove the integration live wiring: vitest.integration.live.config.ts and the test:integration:live / compact:integration:live scripts. The integration tests are now untouched relative to main. Migrating them to the async simulator (and wiring them for live) is deferred to the follow-up PR alongside the live CI workflow. Kept: the unit async migration and the local unit live setup (test:live, vitest.live.config.ts, the _harness backend, Makefile, local-env.yml).
0xisk
changed the title
Reduce this PR to the migration itself: only the existing simulator-using files (per-module simulators + specs) move to the async backend-aware @openzeppelin/compact-simulator 0.2.0, plus the dependency bump. No new files. Remove the live scaffolding added earlier — the test/integration/_harness backend, vitest.live.config.ts, the test:live script, Makefile, and local-env.yml. The local live setup and the live CI workflow belong in a follow-up PR. The integration suite is untouched relative to main.
0xisk
changed the title
Bring back the local unit live-testing setup: the `test:live` script, vitest.live.config.ts, the test/integration/_harness live backend, and make env-up / local-env.yml. `make env-up && yarn test:live` runs the migrated unit specs against a real node (MIDNIGHT_BACKEND=live). The integration suite stays untouched relative to main, and the live CI workflow is still deferred to a follow-up PR.
0xisk
changed the title
Drop the live-backend harness directory. Default dry CI is unaffected (it uses vitest.config.ts, not the live setup).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
contracts/src/token/test/simulators/FungibleTokenSimulator.ts (1)
309-313: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAwait the private-state write before returning.
Line 313 starts the state update but
injectSecretKeyresolves immediately. In the async/live backend path, the next awaited circuit call can observe the previous key.Proposed fix
injectSecretKey: async ( newSK: Uint8Array, ): Promise<FungibleTokenPrivateState> => { const updatedState = FungibleTokenPrivateState.withSecretKey(newSK); - this.setPrivateState(updatedState); + await this.setPrivateState(updatedState); return updatedState; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/simulators/FungibleTokenSimulator.ts` around lines 309 - 313, The async injectSecretKey flow in FungibleTokenSimulator is returning before the private state write completes, which can let later circuit calls see stale data. Update injectSecretKey to await the private-state update triggered by setPrivateState on FungibleTokenPrivateState, and make sure the method only resolves after the new secret key has been persisted.contracts/src/token/test/simulators/NonFungibleTokenSimulator.ts (1)
465-469: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAwait the private-state write before returning.
Line 469 should not fire-and-forget the private-state update; callers rely on
await injectSecretKey(...)before authorization-sensitive circuit calls.Proposed fix
injectSecretKey: async ( newSK: Uint8Array, ): Promise<NonFungibleTokenPrivateState> => { const updatedState = NonFungibleTokenPrivateState.withSecretKey(newSK); - this.setPrivateState(updatedState); + await this.setPrivateState(updatedState); return updatedState; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/simulators/NonFungibleTokenSimulator.ts` around lines 465 - 469, The injectSecretKey method in NonFungibleTokenSimulator is updating private state without waiting for the write to complete, which breaks callers that rely on awaiting it before sensitive operations. Update injectSecretKey so the setPrivateState call is awaited before returning the updated NonFungibleTokenPrivateState, keeping the async contract consistent and ensuring the secret-key state is fully persisted first.contracts/src/token/test/simulators/MultiTokenSimulator.ts (1)
254-258: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAwait the private-state write before returning.
Line 258 should wait for the backend private-state update; otherwise
await injectSecretKey(...)does not guarantee the next circuit uses the injected key.Proposed fix
injectSecretKey: async ( newSK: Uint8Array, ): Promise<MultiTokenPrivateState> => { const updatedState = MultiTokenPrivateState.withSecretKey(newSK); - this.setPrivateState(updatedState); + await this.setPrivateState(updatedState); return updatedState; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/simulators/MultiTokenSimulator.ts` around lines 254 - 258, The injectSecretKey method in MultiTokenSimulator returns before the private-state update is committed, so awaiting it does not guarantee the injected key is available for the next circuit. Update injectSecretKey to await the backend write via setPrivateState before returning the updated MultiTokenPrivateState, and keep the flow anchored around MultiTokenPrivateState.withSecretKey and setPrivateState so the async contract is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/src/access/test/AccessControl.test.ts`:
- Around line 153-155: `AccessControlSimulator.assertOnlyRole()` returns a
resolved promise value rather than throwing, so the `AccessControl.test.ts`
assertion using `resolves.not.toThrow()` is the wrong matcher. Update the test
around `assertOnlyRole(OPERATOR_ROLE_1)` to use a direct `await` on the call, or
explicitly assert the resolved empty array, and apply the same correction to any
other `assertOnlyRole` checks in this file.
In `@contracts/src/access/test/Ownable.test.ts`:
- Around line 164-165: The assertions in Ownable.test.ts are using the wrong
matcher for assertOnlyOwner, since ownable.assertOnlyOwner() resolves to a value
rather than throwing; update the affected test cases to use a direct await or
assert the resolved value instead of resolves.not.toThrow(). Make this change in
the test block around assertOnlyOwner and apply the same correction to the
repeated pattern elsewhere in the Ownable.test suite.
In `@contracts/src/access/test/ShieldedAccessControl.test.ts`:
- Around line 159-166: The success-case assertions in
ShieldedAccessControl.test.ts are using resolves.not.toThrow() on
promise-returning circuit calls, which checks the wrong subject. Update the
repeated test cases in the ShieldedAccessControlSimulator test block to await
the circuit call directly, and if needed assert the returned value or resulting
state instead of using toThrow. Use the circuit invocation inside the "%s should
succeed" test as the reference point for the fix.
In `@contracts/src/access/test/ZOwnablePK.test.ts`:
- Around line 226-240: The transfer ownership tests are passing the commitment
into transferOwnership() instead of the new owner ID, so they do not validate
the intended id-to-commitment flow. Update the calls in ZOwnablePK.test.ts to
use the newOwnerId value when invoking
ownable.as('OWNER').transferOwnership(...) and keep the unauthorized case
aligned with the same API shape. This will ensure the transferOwnership and
_transferOwnership paths, including the counter bump assertion, are exercised
correctly.
- Around line 138-140: The assertions around ZOwnablePK._computeOwnerId and
assertOnlyOwner are using the wrong matcher because these methods return a
Uint8Array, not a thrown promise; update the tests to await the call directly or
assert on the returned bytes, using the ownable._computeOwnerId and
assertOnlyOwner symbols to locate the cases. Also fix transferOwnership to pass
the computed owner id/commitment value, so the newOwnerCommitment argument uses
the previously derived id value (such as newIdHash or repeatedId) instead of the
mismatched placeholder.
In `@contracts/src/token/test/MultiToken.test.ts`:
- Around line 183-185: The `balanceOf` assertion in `MultiToken.test.ts` is
using the wrong matcher because `token.balanceOf(OWNER.either, TOKEN_ID)`
returns a `bigint`, not a function that can throw. Update the expectation in the
`balanceOf` test to assert on the resolved value directly with a value matcher,
and use the `balanceOf` call itself as the unique location to replace
`resolves.not.toThrow()` with an assertion against the expected bigint result.
In `@contracts/src/token/test/nonFungibleToken.test.ts`:
- Around line 630-632: The test is asserting on the resolved promise value with
resolves.not.toThrow(), which does not validate the async circuit call
correctly. Update the affected cases in nonFungibleToken.test.ts around
token.transferFrom and the other matching circuit assertions to await the async
call directly instead of wrapping it in expect(...).resolves.not.toThrow(), then
keep the existing post-state checks to verify behavior after execution.
In `@local-env.yml`:
- Line 4: Pin the Docker image references in local-env.yml so the test:live
stack is reproducible and compatible with midnightntwrk/midnight-node:0.22.2;
replace the floating midnightntwrk/proof-server:latest and
midnightntwrk/indexer-standalone:latest entries with the documented compatible
tags midnightntwrk/proof-server:8.0.3 and
midnightntwrk/indexer-standalone:4.0.1, or equivalent digests, using the
existing service definitions as the location to update.
- Line 5: The container command is being passed as one argv token, so split the
`command` entry for the proof server into separate arguments using the existing
`command` field so it starts correctly. Also replace the floating
`midnightntwrk/proof-server:latest` and
`midnightntwrk/indexer-standalone:latest` image tags with fixed versioned tags
in the same local stack config to keep the environment reproducible.
In `@Makefile`:
- Around line 8-14: The env-up target currently returns immediately after docker
compose up -d, so it can finish before proof-server, indexer, and node are
actually ready. Update the env-up recipe to wait for the stack to become healthy
before exiting, using the existing SERVCES/COMPOSE_FILE flow and any
health-check or readiness mechanism already available in the compose setup. Keep
the log streaming behavior, but make sure the env-up target does not complete
until the key services are reported healthy and ready for test:live.
---
Nitpick comments:
In `@contracts/src/token/test/simulators/FungibleTokenSimulator.ts`:
- Around line 309-313: The async injectSecretKey flow in FungibleTokenSimulator
is returning before the private state write completes, which can let later
circuit calls see stale data. Update injectSecretKey to await the private-state
update triggered by setPrivateState on FungibleTokenPrivateState, and make sure
the method only resolves after the new secret key has been persisted.
In `@contracts/src/token/test/simulators/MultiTokenSimulator.ts`:
- Around line 254-258: The injectSecretKey method in MultiTokenSimulator returns
before the private-state update is committed, so awaiting it does not guarantee
the injected key is available for the next circuit. Update injectSecretKey to
await the backend write via setPrivateState before returning the updated
MultiTokenPrivateState, and keep the flow anchored around
MultiTokenPrivateState.withSecretKey and setPrivateState so the async contract
is honored.
In `@contracts/src/token/test/simulators/NonFungibleTokenSimulator.ts`:
- Around line 465-469: The injectSecretKey method in NonFungibleTokenSimulator
is updating private state without waiting for the write to complete, which
breaks callers that rely on awaiting it before sensitive operations. Update
injectSecretKey so the setPrivateState call is awaited before returning the
updated NonFungibleTokenPrivateState, keeping the async contract consistent and
ensuring the secret-key state is fully persisted first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e16b1f8f-99ff-41f0-a4dd-328c39e1e4d2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (51)
Makefilecontracts/package.jsoncontracts/src/access/test/AccessControl.test.tscontracts/src/access/test/Ownable.test.tscontracts/src/access/test/ShieldedAccessControl.test.tscontracts/src/access/test/ZOwnablePK.test.tscontracts/src/access/test/simulators/AccessControlSimulator.tscontracts/src/access/test/simulators/OwnableSimulator.tscontracts/src/access/test/simulators/ShieldedAccessControlSimulator.tscontracts/src/access/test/simulators/ZOwnablePKSimulator.tscontracts/src/archive/test/ShieldedToken.test.tscontracts/src/archive/test/simulators/ShieldedTokenSimulator.tscontracts/src/multisig/test/Forwarder.test.tscontracts/src/multisig/test/ForwarderPrivate.test.tscontracts/src/multisig/test/ProposalManager.test.tscontracts/src/multisig/test/ShieldedMultiSig.test.tscontracts/src/multisig/test/ShieldedMultiSigV2.test.tscontracts/src/multisig/test/ShieldedMultiSigV3.test.tscontracts/src/multisig/test/ShieldedTreasury.test.tscontracts/src/multisig/test/Signer.test.tscontracts/src/multisig/test/SignerManager.test.tscontracts/src/multisig/test/presets/ForwarderPrivate.test.tscontracts/src/multisig/test/presets/ForwarderShielded.test.tscontracts/src/multisig/test/presets/ForwarderUnshielded.test.tscontracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/MockForwarderShieldedSimulator.tscontracts/src/multisig/test/simulators/MockForwarderUnshieldedSimulator.tscontracts/src/multisig/test/simulators/ProposalManagerSimulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigSimulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigV2Simulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigV3Simulator.tscontracts/src/multisig/test/simulators/ShieldedTreasurySimulator.tscontracts/src/multisig/test/simulators/SignerManagerSimulator.tscontracts/src/multisig/test/simulators/SignerSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderShieldedSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderUnshieldedSimulator.tscontracts/src/security/test/Initializable.test.tscontracts/src/security/test/Pausable.test.tscontracts/src/security/test/simulators/InitializableSimulator.tscontracts/src/security/test/simulators/PausableSimulator.tscontracts/src/token/test/FungibleToken.test.tscontracts/src/token/test/MultiToken.test.tscontracts/src/token/test/nonFungibleToken.test.tscontracts/src/token/test/simulators/FungibleTokenSimulator.tscontracts/src/token/test/simulators/MultiTokenSimulator.tscontracts/src/token/test/simulators/NonFungibleTokenSimulator.tscontracts/src/utils/test/simulators/UtilsSimulator.tscontracts/src/utils/test/utils.test.tscontracts/vitest.live.config.tslocal-env.yml
| await expect( | ||
| accessControl.assertOnlyRole(OPERATOR_ROLE_1), | ||
| ).resolves.not.toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the test file and related symbols
ast-grep outline contracts/src/access/test/AccessControl.test.ts --view expanded || true
# Find all occurrences of the matcher pattern in the repo
rg -n "resolves\.not\.toThrow\(|resolves\.toThrow\(|not\.toThrow\(" contracts/src || true
# Locate the implementation/sig of assertOnlyRole and nearby helpers
rg -n "assertOnlyRole" contracts/src || trueRepository: OpenZeppelin/compact-contracts
Length of output: 10876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' contracts/src/access/test/simulators/AccessControlSimulator.ts
printf '\n---\n'
sed -n '140,220p' contracts/src/access/test/AccessControl.test.ts
printf '\n---\n'
sed -n '1,120p' contracts/src/access/AccessControl.compactRepository: OpenZeppelin/compact-contracts
Length of output: 13656
Replace resolves.not.toThrow() with a direct await
AccessControlSimulator.assertOnlyRole() returns Promise<[]>, so toThrow is the wrong matcher here. Use await accessControl.assertOnlyRole(OPERATOR_ROLE_1); or assert the resolved [] explicitly. This same pattern appears elsewhere in the file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/access/test/AccessControl.test.ts` around lines 153 - 155,
`AccessControlSimulator.assertOnlyRole()` returns a resolved promise value
rather than throwing, so the `AccessControl.test.ts` assertion using
`resolves.not.toThrow()` is the wrong matcher. Update the test around
`assertOnlyRole(OPERATOR_ROLE_1)` to use a direct `await` on the call, or
explicitly assert the resolved empty array, and apply the same correction to any
other `assertOnlyRole` checks in this file.
| await expect(ownable.assertOnlyOwner()).resolves.not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
set -e
git ls-files 'contracts/src/access/test/*' 'contracts/src/access/*' | sed -n '1,200p'
printf '\n--- Ownable.test.ts excerpt ---\n'
nl -ba contracts/src/access/test/Ownable.test.ts | sed -n '130,210p'
printf '\n--- search for assertOnlyOwner definition/usages ---\n'
rg -n "assertOnlyOwner\\(" contracts/src -S
printf '\n--- search for resolves.not.toThrow usages ---\n'
rg -n "resolves\.not\.toThrow\(" contracts/src -S
printf '\n--- search for toThrow matcher around promises ---\n'
rg -n "toThrow\(" contracts/src -SRepository: OpenZeppelin/compact-contracts
Length of output: 50386
🏁 Script executed:
set -e
git ls-files 'contracts/src/access/test/*' 'contracts/src/access/*' | sed -n '1,200p'
printf '\n--- Ownable.test.ts excerpt ---\n'
nl -ba contracts/src/access/test/Ownable.test.ts | sed -n '130,210p'
printf '\n--- search for assertOnlyOwner definition/usages ---\n'
rg -n "assertOnlyOwner\\(" contracts/src -S
printf '\n--- search for resolves.not.toThrow usages ---\n'
rg -n "resolves\.not\.toThrow\(" contracts/src -S
printf '\n--- search for toThrow matcher around promises ---\n'
rg -n "toThrow\(" contracts/src -SRepository: OpenZeppelin/compact-contracts
Length of output: 50386
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path('contracts/src/access/test/Ownable.test.ts')
for i, line in enumerate(path.read_text().splitlines(), 1):
if 155 <= i <= 170:
print(f"{i:4d}: {line}")
PYRepository: OpenZeppelin/compact-contracts
Length of output: 888
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path('contracts/src/access/test/Ownable.test.ts')
for i, line in enumerate(path.read_text().splitlines(), 1):
if 155 <= i <= 170:
print(f"{i:4d}: {line}")
PYRepository: OpenZeppelin/compact-contracts
Length of output: 888
🌐 Web query:
Vitest expect resolves toThrow matcher resolved promise value not a function docs
💡 Result:
In Vitest, the expect().resolves and expect().rejects modifiers expect a promise as their input [1][2]. If you pass a function instead of a promise, you will encounter a "TypeError: You must provide a Promise to expect when using.resolves (or.rejects), not 'function'" [3][2]. The toThrow matcher is designed to assert that a function throws an error when called [1]. Because it needs to execute a function to catch its thrown error, the usage patterns for synchronous and asynchronous code differ: 1. Synchronous Code: Wrap the code in a function so that toThrow can invoke it safely [1]. Example: expect( => mySyncFunction).toThrow [1] 2. Asynchronous Code: Use the rejects modifier, which is specifically designed to handle promises and works similarly to toThrow for async errors [1]. Example: await expect(myAsyncPromise).rejects.toThrow [1] Passing a function directly to expect().resolves is not supported because resolves is intended to wait for a promise to settle and then validate the resulting value, not to execute a function and catch its errors [4][2]. If you need to test that an asynchronous operation does not throw, you should await the promise normally and use not.toThrow() only if you are checking for synchronous errors within the resolved value, or simply ensure the promise resolves without rejection [1][5].
Citations:
- 1: https://vitest.dev/api/expect
- 2: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 3: expect().resolves does not accept a function vitest-dev/vitest#10281
- 4: fix:
.resolvesand.rejectsexpectations vitest-dev/vitest#1300 - 5: https://github.com/vitest-dev/vitest/blob/9423dc08/test/core/test/jest-expect.test.ts
Use an await or value assertion here instead of resolves.not.toThrow()
ownable.assertOnlyOwner() resolves to [], so toThrow() is the wrong matcher. Use await ownable.assertOnlyOwner() or assert the resolved value directly; the same pattern repeats in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/access/test/Ownable.test.ts` around lines 164 - 165, The
assertions in Ownable.test.ts are using the wrong matcher for assertOnlyOwner,
since ownable.assertOnlyOwner() resolves to a value rather than throwing; update
the affected test cases to use a direct await or assert the resolved value
instead of resolves.not.toThrow(). Make this change in the test block around
assertOnlyOwner and apply the same correction to the repeated pattern elsewhere
in the Ownable.test suite.
| )('%s should succeed', async (circuitName, args) => { | ||
| await expect( | ||
| ( | ||
| contract[circuitName as keyof ShieldedAccessControlSimulator] as ( | ||
| ...a: unknown[] | ||
| ) => unknown | ||
| )(...args); | ||
| }).not.toThrow(); | ||
| ) => Promise<unknown> | ||
| )(...args), | ||
| ).resolves.not.toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
git ls-files contracts/src/access/test/ShieldedAccessControl.test.ts
wc -l contracts/src/access/test/ShieldedAccessControl.test.ts
sed -n '140,185p' contracts/src/access/test/ShieldedAccessControl.test.ts
# Search for similar matcher usage in this file and nearby tests.
rg -n "\.resolves\.not\.toThrow\(\)|\.rejects\.toThrow\(\)|toThrow\(\)" contracts/src/access/test/ShieldedAccessControl.test.ts contracts/src/access/test -g '*.ts'Repository: OpenZeppelin/compact-contracts
Length of output: 9205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test file and surrounding matcher patterns.
git ls-files contracts/src/access/test/ShieldedAccessControl.test.ts
wc -l contracts/src/access/test/ShieldedAccessControl.test.ts
sed -n '140,185p' contracts/src/access/test/ShieldedAccessControl.test.ts
# Find all occurrences of the same matcher shape in this file and nearby tests.
rg -n "\.resolves\.not\.toThrow\(\)|\.rejects\.toThrow\(\)|toThrow\(\)" contracts/src/access/test/ShieldedAccessControl.test.ts contracts/src/access/test -g '*.ts'Repository: OpenZeppelin/compact-contracts
Length of output: 9205
🌐 Web query:
Vitest expect resolves not.toThrow promise matcher semantics
💡 Result:
In Vitest, using expect(...).resolves.not.toThrow carries specific semantic and technical constraints based on the type of input provided. 1. Expectation Semantics: The.resolves modifier is intended for use with Promises [1]. It unwraps the resolved value of a promise so that subsequent matchers can be applied [1]. Therefore, expect(promise).resolves.not.toThrow asserts that the promise successfully resolves without the underlying execution throwing an error [2]. 2. Input Requirements: Historically, Vitest required a Promise as the argument for expect when using.resolves [3]. Attempting to pass a function (e.g., expect(async =>...).resolves.not.toThrow) would typically result in a TypeError stating that a Promise must be provided [4][3]. 3. Recent Developments: As of mid-2026, there have been active community efforts and pull requests (such as #10284, #10342, and #10403) to generalize support for functions within.resolves, similar to how they are supported in.rejects [4]. While.rejects inherently requires a function to catch and inspect thrown errors [1][5],.resolves is primarily for handling resolved values. If you encounter issues, ensure you are testing a Promise. If you are testing an asynchronous function, common practice is to await the function execution first, or use.rejects to verify expected failures [1][2]. For verifying that code does not throw, ensuring the promise resolves successfully is the standard approach using.resolves [2].
Citations:
- 1: https://vitest.dev/api/expect
- 2: https://stackoverflow.com/questions/47144187/can-you-write-async-tests-that-expect-tothrow
- 3: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 4: expect().resolves does not accept a function vitest-dev/vitest#10281
- 5: https://github.com/vitest-dev/vitest/blob/v4.1.7/docs/api/expect.md
🌐 Web query:
Vitest expect resolves not.toThrow promise matcher semantics
💡 Result:
In Vitest, the .resolves matcher is designed to unwrap a successfully resolved promise and assert that the resulting value matches a given condition [1][2]. The .toThrow matcher, however, is specifically designed to assert that a function throws an error when executed [1][3]. When using expect(...).resolves.not.toThrow(), Vitest attempts to apply toThrow to the resolved value of the promise, rather than treating that value as a function to be executed [4]. Consequently, this matcher chain is generally inappropriate for testing that an asynchronous process does not throw, because toThrow expects a function as its input [3], not a resolved promise value [1][4]. Key technical points regarding these semantics include: 1. Promise Handling vs. Function Execution: .resolves unwraps the promise and passes the settled value to the next matcher [2][4]. If you pass a value that is not a function to .toThrow, the matcher will fail or behave unexpectedly because it cannot execute the value to catch an error [3][4]. 2. Alternative for Async Exceptions: To verify that an asynchronous operation does not throw an error, the standard approach is to simply await the promise [2]. If the promise rejects, the test will fail automatically unless you explicitly handle the rejection (e.g., with a try/catch block or by using .rejects.toThrow()) [5][2]. 3. Intended Use of.rejects: The .rejects matcher is the intended way to test for asynchronous exceptions [2]. It is specifically built to catch and assert against the rejection reason of a promise [1][3]. If you need to ensure an async operation finishes without error, awaiting the promise directly is sufficient, as any rejection will cause the test to fail [2]. If you specifically wish to verify that an async function does not throw, you should ensure that the function itself is awaited during the test execution, rather than attempting to chain not.toThrow() after a .resolves call [5].
Citations:
- 1: https://vitest.dev/api/expect
- 2: https://vitest.dev/guide/learn/async
- 3: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/expect.md
- 4: fix:
.resolvesand.rejectsexpectations vitest-dev/vitest#1300 - 5: https://stackoverflow.com/questions/47144187/can-you-write-async-tests-that-expect-tothrow
🌐 Web query:
Vitest expect resolves not.toThrow promise matcher semantics
💡 Result:
In Vitest, the .resolves and .rejects matchers are designed to handle asynchronous code by unwrapping promises [1][2]. Regarding not.toThrow, the semantics are as follows: 1. Primary Usage: The .resolves matcher is intended to be used with a promise [1][3]. When you use await expect(promise).resolves.not.toThrow(), Vitest waits for the promise to resolve successfully; if it resolves, the assertion passes because no error was thrown during the resolution of the promise [1][4]. 2. Function Parameter Support: Historically, passing a function to expect().resolves (e.g., expect(async () => { ... }).resolves.not.toThrow()) was not supported and would throw a TypeError stating that you must provide a Promise, not a function [5][3]. While there has been recent community discussion and development (as of mid-2026) regarding aligning this behavior with Jest to allow functions in .resolves, the standard and stable way to assert that an asynchronous operation does not throw is to execute the function and pass the resulting promise to expect [5][3]. 3. Contrast with .rejects: The .rejects matcher is explicitly designed to handle functions that might throw (or return rejected promises) [1][2]. This is why await expect(async () => { ... }).rejects.toThrow() works—rejects captures and unwraps the rejection [1][2]. If you need to verify that an asynchronous function does not throw, the standard practice is to await the function call and assert against the resulting promise: await expect(myAsyncFunction).resolves.not.toThrow; [4] If you use a function as an argument, it is generally recommended to await the execution of that function before asserting, or use the .rejects pattern if you expect an error [1][4].
Citations:
- 1: https://vitest.dev/api/expect
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/expect.md
- 3: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 4: https://stackoverflow.com/questions/47144187/can-you-write-async-tests-that-expect-tothrow
- 5: expect().resolves does not accept a function vitest-dev/vitest#10281
🌐 Web query:
Vitest expect resolves not.toThrow promise matcher semantics
💡 Result:
In Vitest, expect(...).resolves.not.toThrow() is a valid assertion, but it behaves differently from the standard toThrow matcher used on functions. 1. Semantics of.resolves: The .resolves modifier is designed to unwrap a promise [1]. When you use await expect(promise).resolves.not.toThrow(), Vitest awaits the promise. If the promise resolves successfully, the .not.toThrow() matcher then checks the resolved value (or the lack of a thrown error during the process) [1][2]. 2. Important Constraint: Unlike .rejects, which is specifically intended for testing that an operation throws an error (and therefore accepts a function to execute), .resolves is intended to assert the value a promise resolves to [1]. Historically, Vitest required a Promise as the input to expect() when using .resolves [3][4]. Passing a function to expect(async () => { ... }).resolves.not.toThrow() has been a source of technical inconsistency, often resulting in a TypeError (e.g., "You must provide a Promise to expect when using.resolves, not 'function'") [3][4]. 3. How to use correctly: If you want to ensure an asynchronous operation completes without throwing an error: - If the operation is a Promise, simply use: await expect(myPromise).resolves.not.toThrow() [5][2]. - If the operation is a function that returns a Promise, you should invoke it first to get the Promise: await expect(myFunction()).resolves.not.toThrow() [6][2]. If your goal is to test that a function does not throw an error synchronously, you should avoid .resolves entirely and use the standard: expect(() => myFunction()).not.toThrow() [7][8]. Current development in Vitest (as of mid-2026) has seen discussions and efforts to improve consistency, such as allowing functions in .resolves to match the behavior of .rejects, but the standard and safest approach remains passing the resolved Promise to expect() when using the .resolves modifier [3][4].
Citations:
- 1: https://vitest.dev/api/expect
- 2: fix:
.resolvesand.rejectsexpectations vitest-dev/vitest#1300 - 3: expect().resolves does not accept a function vitest-dev/vitest#10281
- 4: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 5: https://stackoverflow.com/questions/47144187/can-you-write-async-tests-that-expect-tothrow
- 6: https://github.com/vitest-dev/vitest/blob/9423dc08/test/core/test/jest-expect.test.ts
- 7: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/api/expect.md
- 8: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/expect.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the Vitest version used by this repository and inspect related test patterns.
git ls-files package.json pnpm-lock.yaml yarn.lock package-lock.json contracts/package.json contracts/pnpm-lock.yaml contracts/yarn.lock contracts/package-lock.json
rg -n '"vitest"|vitest' package.json contracts/package.json pnpm-lock.yaml yarn.lock package-lock.json contracts/pnpm-lock.yaml contracts/yarn.lock contracts/package-lock.json -g '!*node_modules*'
# Inspect a couple of neighboring assertions in the same file for comparison.
sed -n '150,175p' contracts/src/access/test/ShieldedAccessControl.test.ts
sed -n '320,360p' contracts/src/access/test/ShieldedAccessControl.test.tsRepository: OpenZeppelin/compact-contracts
Length of output: 3599
🌐 Web query:
site:vitest.dev/api/expect toThrow resolves function matcher
💡 Result:
The toThrow (and its alias toThrowError) matcher is not supported with the .resolves modifier in testing frameworks like Jest or Vitest [1]. The toThrow matcher is designed to assert that a function throws an error when executed synchronously [2][1]. Because .resolves is used to unwrap a promise and assert its resolved value, it is conceptually incompatible with testing for thrown errors [1]. To test that an asynchronous operation throws or rejects, you should use the .rejects modifier instead of .resolves [2][3][1]. Example of correct usage for async error handling: await expect(myAsyncFunction).rejects.toThrow; If you attempt to use expect(...).resolves.toThrow(), the test runner will typically indicate that the matcher is not supported or that the assertion is invalid [1].
Citations:
- 1: https://v3.vitest.dev/api/expect
- 2: https://vitest.dev/api/expect
- 3: https://v3.cn.vitest.dev/api/expect
Replace resolves.not.toThrow() in ShieldedAccessControl.test.ts
toThrow does not belong on these promise assertions: the circuits resolve to values, so this chain is checking the wrong subject. Use a plain await for success cases, or assert the returned value/state explicitly. Update the repeated occurrences in this file as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/access/test/ShieldedAccessControl.test.ts` around lines 159 -
166, The success-case assertions in ShieldedAccessControl.test.ts are using
resolves.not.toThrow() on promise-returning circuit calls, which checks the
wrong subject. Update the repeated test cases in the
ShieldedAccessControlSimulator test block to await the circuit call directly,
and if needed assert the returned value or resulting state instead of using
toThrow. Use the circuit invocation inside the "%s should succeed" test as the
reference point for the fix.
| await expect( | ||
| ownable._computeOwnerId(eitherOwner, randomByteArray), | ||
| ).resolves.not.toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File outline =="
ast-grep outline contracts/src/access/test/ZOwnablePK.test.ts --view expanded || true
echo
echo "== Relevant lines around 120-170 =="
sed -n '120,170p' contracts/src/access/test/ZOwnablePK.test.ts | cat -n
echo
echo "== Simulator signature =="
ast-grep outline contracts/src/access/test/simulators/ZOwnablePKSimulator.ts --view expanded || true
echo
echo "== transferOwnership occurrences =="
rg -n "transferOwnership\(" contracts/src/access/test -g '*.ts'Repository: OpenZeppelin/compact-contracts
Length of output: 7173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('contracts/src/access/test/ZOwnablePK.test.ts')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 130 <= i <= 150:
print(f"{i:4}: {line}")
PYRepository: OpenZeppelin/compact-contracts
Length of output: 942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ZOwnablePK.test.ts excerpt =="
nl -ba contracts/src/access/test/ZOwnablePK.test.ts | sed -n '130,155p'
echo
echo "== search for `.resolves.not.toThrow()` =="
rg -n "\.resolves\.not\.toThrow\(\)" contracts/src/access/test/ZOwnablePK.test.ts
echo
echo "== _computeOwnerId definition =="
rg -n "computeOwnerId|_computeOwnerId" contracts/src/access/test contracts/src/access -g '*.ts' -A 4 -B 4
echo
echo "== simulator transferOwnership signature =="
nl -ba contracts/src/access/test/simulators/ZOwnablePKSimulator.ts | sed -n '1,120p'Repository: OpenZeppelin/compact-contracts
Length of output: 241
🌐 Web query:
Vitest expect.resolves toThrow matcher semantics function only docs
💡 Result:
In Vitest, using expect.resolves.toThrow is not supported, and attempting to do so will result in a TypeError [1][2]. The matcher.resolves is designed to unwrap a promise and assert its resolved value; if you attempt to use it with.toThrow, Vitest will throw an error stating: "You must provide a Promise to expect when using.resolves, not 'function'" [1][2]. This is because.resolves expects an object with a.then method (a promise), whereas.toThrow is a synchronous matcher that expects a function to be passed to expect [3][2]. To test that an asynchronous function throws an error, you should use.rejects.toThrow instead [3][4]: await expect(asyncFunction).rejects.toThrow; If you have a function that returns a promise but you want to ensure it completes successfully without throwing, you can use.resolves.not.toThrow in some contexts [5], but you must ensure the subject passed to expect is a promise, not a function [1][2]. If your goal is to test a function that throws synchronously, use expect( => function).toThrow [3][4].
Citations:
- 1: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 2: https://github.com/vitest-dev/vitest/blob/206e8cff/packages/expect/src/jest-expect.ts
- 3: https://vitest.dev/api/expect
- 4: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/api/expect.md
- 5: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/test/core/test/jest-expect.test.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('contracts/src/access/test/ZOwnablePK.test.ts')
lines = p.read_text().splitlines()
for start,end in [(170,270),(1,80)]:
print(f"== lines {start}-{end} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
print()
PYRepository: OpenZeppelin/compact-contracts
Length of output: 7041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('contracts/src/access/test/ZOwnablePK.test.ts')
lines = p.read_text().splitlines()
for start, end in [(170, 270), (1, 80)]:
print(f"== lines {start}-{end} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
print()
PYRepository: OpenZeppelin/compact-contracts
Length of output: 7041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('contracts/src/access/test/ZOwnablePK.test.ts')
lines = p.read_text().splitlines()
for start, end in [(1, 70), (180, 250)]:
print(f"== lines {start}-{end} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
print()
PYRepository: OpenZeppelin/compact-contracts
Length of output: 5515
Replace the matcher and pass the owner id
await expect(ownable._computeOwnerId(...)).resolves.not.toThrow() is the wrong assertion here: _computeOwnerId() resolves to a Uint8Array, so assert on the bytes or just await the call. The same matcher pattern later on assertOnlyOwner() has the same problem.
transferOwnership(...) should receive the new owner id; the newOwnerCommitment argument later in the file should be newIdHash/repeatedId instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/access/test/ZOwnablePK.test.ts` around lines 138 - 140, The
assertions around ZOwnablePK._computeOwnerId and assertOnlyOwner are using the
wrong matcher because these methods return a Uint8Array, not a thrown promise;
update the tests to await the call directly or assert on the returned bytes,
using the ownable._computeOwnerId and assertOnlyOwner symbols to locate the
cases. Also fix transferOwnership to pass the computed owner id/commitment
value, so the newOwnerCommitment argument uses the previously derived id value
(such as newIdHash or repeatedId) instead of the mismatched placeholder.
| it('should fail when unauthorized transfers ownership', async () => { | ||
| await expect( | ||
| ownable.as('UNAUTHORIZED').transferOwnership(newOwnerCommitment), | ||
| ).rejects.toThrow('ZOwnablePK: caller is not the owner'); | ||
| }); | ||
|
|
||
| /** | ||
| * @description More thoroughly tested in `_transferOwnership` | ||
| * */ | ||
| it('should bump instance after transfer', () => { | ||
| const beforeInstance = ownable.getPublicState().ZOwnablePK__counter; | ||
| it('should bump instance after transfer', async () => { | ||
| const beforeInstance = (await ownable.getPublicState()) | ||
| .ZOwnablePK__counter; | ||
|
|
||
| // Transfer | ||
| ownable.as(OWNER).transferOwnership(newOwnerCommitment); | ||
| await ownable.as('OWNER').transferOwnership(newOwnerCommitment); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the new owner ID into transferOwnership(), not the commitment.
transferOwnership(...) takes newOwnerId, but Lines 228 and 240 pass newOwnerCommitment. That means these tests can still pass without exercising the intended id-to-commitment path, especially the counter-bump case.
Suggested fix
- ownable.as('UNAUTHORIZED').transferOwnership(newOwnerCommitment),
+ ownable.as('UNAUTHORIZED').transferOwnership(newIdHash),
@@
- await ownable.as('OWNER').transferOwnership(newOwnerCommitment);
+ await ownable.as('OWNER').transferOwnership(newIdHash);📝 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.
| it('should fail when unauthorized transfers ownership', async () => { | |
| await expect( | |
| ownable.as('UNAUTHORIZED').transferOwnership(newOwnerCommitment), | |
| ).rejects.toThrow('ZOwnablePK: caller is not the owner'); | |
| }); | |
| /** | |
| * @description More thoroughly tested in `_transferOwnership` | |
| * */ | |
| it('should bump instance after transfer', () => { | |
| const beforeInstance = ownable.getPublicState().ZOwnablePK__counter; | |
| it('should bump instance after transfer', async () => { | |
| const beforeInstance = (await ownable.getPublicState()) | |
| .ZOwnablePK__counter; | |
| // Transfer | |
| ownable.as(OWNER).transferOwnership(newOwnerCommitment); | |
| await ownable.as('OWNER').transferOwnership(newOwnerCommitment); | |
| it('should fail when unauthorized transfers ownership', async () => { | |
| await expect( | |
| ownable.as('UNAUTHORIZED').transferOwnership(newIdHash), | |
| ).rejects.toThrow('ZOwnablePK: caller is not the owner'); | |
| }); | |
| /** | |
| * `@description` More thoroughly tested in `_transferOwnership` | |
| * */ | |
| it('should bump instance after transfer', async () => { | |
| const beforeInstance = (await ownable.getPublicState()) | |
| .ZOwnablePK__counter; | |
| // Transfer | |
| await ownable.as('OWNER').transferOwnership(newIdHash); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/access/test/ZOwnablePK.test.ts` around lines 226 - 240, The
transfer ownership tests are passing the commitment into transferOwnership()
instead of the new owner ID, so they do not validate the intended
id-to-commitment flow. Update the calls in ZOwnablePK.test.ts to use the
newOwnerId value when invoking ownable.as('OWNER').transferOwnership(...) and
keep the unauthorized case aligned with the same API shape. This will ensure the
transferOwnership and _transferOwnership paths, including the counter bump
assertion, are exercised correctly.
| await expect( | ||
| token.balanceOf(OWNER.either, TOKEN_ID), | ||
| ).resolves.not.toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json vitest version =="
rg -n '"vitest"|resolves\.not\.toThrow|toThrow\(' package.json contracts/package.json . -g 'package.json' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.mjs' -g '*.cjs' | sed -n '1,120p'
echo
echo "== target file excerpt =="
cat -n contracts/src/token/test/MultiToken.test.ts | sed -n '160,200p'Repository: OpenZeppelin/compact-contracts
Length of output: 15798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^|/)package\.json$|MultiToken\.test\.ts$|vitest'
echo
echo "== vitest usage in repo =="
rg -n "resolves\.not\.toThrow|resolves\.toThrow|toThrow\(" contracts/src -g '*.ts' -g '*.tsx' | sed -n '1,160p'
echo
echo "== target file around lines 183-185 =="
cat -n contracts/src/token/test/MultiToken.test.ts | sed -n '175,190p'Repository: OpenZeppelin/compact-contracts
Length of output: 19679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' contracts/src/token/test/MultiToken.test.tsRepository: OpenZeppelin/compact-contracts
Length of output: 7406
🌐 Web query:
Vitest 4.1.9 expect resolves toThrow matcher resolved value function bigint
💡 Result:
In Vitest, the.resolves matcher is used to assert on the fulfilled value of a promise, while.toThrow is used to assert that a function throws an error [1][2]. These are distinct mechanisms:.toThrow requires a function to be passed to expect so it can safely catch and inspect the thrown error [1][3]. Regarding your query: 1. Resolves and toThrow: You cannot directly combine.resolves.toThrow because.resolves expects a promise that fulfills to a value, not a function that throws [1][4][3]. If you need to test that an asynchronous function (returning a promise) rejects with a specific error, you should use.rejects.toThrow instead [1][2][3]. 2. BigInt support: Vitest's expect matchers, such as.toBeGreaterThan or.toEqual, generally support BigInt where appropriate, as they often rely on deep equality or standard comparison operators that handle BigInt [5][6][7]. If you are asserting that a function throws a BigInt or an error containing a BigInt,.toThrow supports deep equality checks, which can handle BigInt values [1][7]. 3. Function parameter support: As of mid-2026, there have been ongoing updates to Vitest regarding the types of values accepted by.resolves [4]. Historically,.resolves strictly required a Promise, while.rejects was more permissive with functions for compatibility with Jest [4][8]. Example of correct usage for async error testing: // Correct way to test an async function that throws await expect(myAsyncFunction).rejects.toThrow; // If you need to check for a specific thrown value (including BigInt) await expect(myAsyncFunction).rejects.toThrow(100n); // 100n is a BigInt If you are attempting to test that a promise resolves to a function that, when called, throws an error, you must first await the promise to retrieve the function, and then wrap that function in another expectation [9]: const myFunc = await myAsyncFunctionReturningFunc; expect( => myFunc).toThrow;
Citations:
- 1: https://vitest.dev/api/expect
- 2: https://vitest.dev/guide/learn/async
- 3: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/api/expect.md
- 4: expect().resolves does not accept a function vitest-dev/vitest#10281
- 5: More Jest Expect compactible APIs vitest-dev/vitest#43
- 6: https://github.com/vitest-dev/vitest/blob/206e8cff/packages/expect/src/jest-expect.ts
- 7: vitest-dev/vitest@de215c19c
- 8: "You must provide a Promise to expect()" inconsistent between
resolvesandrejectsvitest-dev/vitest#6643 - 9: https://github.com/vitest-dev/vitest/blob/9423dc08/test/core/test/jest-expect.test.ts
🌐 Web query:
Vitest resolves.toThrow matcher on resolved promise value documentation
💡 Result:
In Vitest, you should not use.resolves.toThrow to test for thrown errors [1][2]. Instead, you must use.rejects.toThrow when asserting that a promise will reject [1][3]. The.resolves and.rejects helpers are designed to unwrap promises and apply subsequent matchers to the resolved value or the rejected reason, respectively [3]. Key points regarding this behavior: 1. Correct Usage for Errors: To assert that an asynchronous function or promise rejects with an error, use: await expect(promise).rejects.toThrow; [1][4] 2. Function Wrapping: Unlike synchronous toThrow assertions, which require wrapping the throwing code in a function (e.g., expect( => code).toThrow), async assertions using.rejects do not require this wrapping because the promise rejection itself is the signal that an error occurred [1][5]. 3. Misuse of.resolves: Using.resolves.toThrow does not work as expected because.resolves expects the promise to complete successfully, at which point toThrow will fail because no error was thrown [1][6]. While you may see code in internal test suites that uses.resolves.not.toThrow to verify that a promise resolves successfully without throwing, this is simply confirming the absence of an error during resolution [6][7]. Always remember to await the entire expect expression to ensure the assertion is properly executed and waited for by the test runner [8][3].
Citations:
- 1: https://vitest.dev/api/expect
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/expect.md
- 3: https://vitest.dev/guide/learn/async
- 4: https://github.com/antfu/skills/blob/main/skills/vitest/references/core-expect.md
- 5: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/api/expect.md
- 6: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/test/core/test/jest-expect.test.ts
- 7: https://github.com/vitest-dev/vitest/blob/9423dc08/test/core/test/jest-expect.test.ts
- 8: https://github.com/sapegin/vitest-cheat-sheet
Replace resolves.not.toThrow() with a value assertion. balanceOf(...) resolves to a bigint, so toThrow is the wrong matcher here.
Proposed fix
- await expect(
- token.balanceOf(OWNER.either, TOKEN_ID),
- ).resolves.not.toThrow();
+ expect(await token.balanceOf(OWNER.either, TOKEN_ID)).toEqual(0n);📝 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.
| await expect( | |
| token.balanceOf(OWNER.either, TOKEN_ID), | |
| ).resolves.not.toThrow(); | |
| expect(await token.balanceOf(OWNER.either, TOKEN_ID)).toEqual(0n); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/token/test/MultiToken.test.ts` around lines 183 - 185, The
`balanceOf` assertion in `MultiToken.test.ts` is using the wrong matcher because
`token.balanceOf(OWNER.either, TOKEN_ID)` returns a `bigint`, not a function
that can throw. Update the expectation in the `balanceOf` test to assert on the
resolved value directly with a value matcher, and use the `balanceOf` call
itself as the unique location to replace `resolves.not.toThrow()` with an
assertion against the expected bigint result.
| await expect( | ||
| token.transferFrom(OWNER.either, OWNER.either, TOKENID_1), | ||
| ).resolves.not.toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant test file and matcher usages.
git ls-files 'contracts/src/token/test/nonFungibleToken.test.ts'
rg -n "resolves\.not\.toThrow|toThrow\(" contracts/src/token/test/nonFungibleToken.test.tsRepository: OpenZeppelin/compact-contracts
Length of output: 4052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding test cases around the reported lines.
sed -n '600,700p' contracts/src/token/test/nonFungibleToken.test.ts
sed -n '1180,1335p' contracts/src/token/test/nonFungibleToken.test.tsRepository: OpenZeppelin/compact-contracts
Length of output: 10529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'contracts/src/token/test/nonFungibleToken.test.ts'
rg -n "resolves\.not\.toThrow|toThrow\(" contracts/src/token/test/nonFungibleToken.test.tsRepository: OpenZeppelin/compact-contracts
Length of output: 4052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '600,700p' contracts/src/token/test/nonFungibleToken.test.ts
sed -n '1180,1335p' contracts/src/token/test/nonFungibleToken.test.tsRepository: OpenZeppelin/compact-contracts
Length of output: 10529
Replace resolves.not.toThrow() on circuit promises
These assertions target the resolved value, not the async call. Use await token... and keep the post-state checks instead. Affects lines 632, 1194, 1235, and 1317.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/token/test/nonFungibleToken.test.ts` around lines 630 - 632,
The test is asserting on the resolved promise value with resolves.not.toThrow(),
which does not validate the async circuit call correctly. Update the affected
cases in nonFungibleToken.test.ts around token.transferFrom and the other
matching circuit assertions to await the async call directly instead of wrapping
it in expect(...).resolves.not.toThrow(), then keep the existing post-state
checks to verify behavior after execution.
| # WARNING: Insecure default credentials below. For local development only — do not use in production. | ||
| services: | ||
| proof-server: | ||
| image: 'midnightntwrk/proof-server:latest' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What image tags or digests are recommended for midnightntwrk/proof-serverandmidnightntwrk/indexer-standaloneto pair withmidnightntwrk/midnight-node:0.22.2 for local development/live testing?
💡 Result:
To pair with midnightntwrk/midnight-node:0.22.2 for local development and live testing, the recommended image tags for the proof server and standalone indexer are [1]: - midnightntwrk/proof-server:8.0.3 [1][2][3] - midnightntwrk/indexer-standalone:4.0.1 [1][2][3] These versions are part of the standard compatibility set used in Midnight's local development environments (e.g., in the standalone.yml configuration) to ensure inter-component functionality [1][2][3]. While newer versions of these components may be available on Docker Hub, using the versions listed in the official compatibility documentation is recommended to prevent issues during local development [1][4][5]. Always ensure that your local environment configuration correctly maps the ports and environment variables required for these services, such as setting the APPINFRASECRET for the indexer [2][6].
Citations:
- 1: https://github.com/midnightntwrk/midnight-sdk/blob/main/COMPATIBILITY.md
- 2: https://docs.midnight.network/guides/midnight-local-network
- 3: https://github.com/midnightntwrk/midnight-local-dev
- 4: https://docs.midnight.network/relnotes/support-matrix
- 5: https://docs.midnight.network/relnotes/overview
- 6: https://github.com/midnightntwrk/midnight-indexer
🏁 Script executed:
git ls-files | rg '(^|/)(local-env\.yml|standalone\.yml|COMPATIBILITY\.md|README\.md|docker-compose|compose)' && printf '\n---\n' && sed -n '1,120p' local-env.yml && printf '\n---\n' && rg -n 'proof-server|indexer-standalone|midnight-node|8\.0\.3|4\.0\.1|latest' -S .Repository: OpenZeppelin/compact-contracts
Length of output: 3705
Pin the Docker images to compatible releases. local-env.yml still floats midnightntwrk/proof-server and midnightntwrk/indexer-standalone on latest; pin them to the documented compatible tags for midnightntwrk/midnight-node:0.22.2 (midnightntwrk/proof-server:8.0.3 and midnightntwrk/indexer-standalone:4.0.1) or to digests so test:live stays reproducible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@local-env.yml` at line 4, Pin the Docker image references in local-env.yml so
the test:live stack is reproducible and compatible with
midnightntwrk/midnight-node:0.22.2; replace the floating
midnightntwrk/proof-server:latest and midnightntwrk/indexer-standalone:latest
entries with the documented compatible tags midnightntwrk/proof-server:8.0.3 and
midnightntwrk/indexer-standalone:4.0.1, or equivalent digests, using the
existing service definitions as the location to update.
| services: | ||
| proof-server: | ||
| image: 'midnightntwrk/proof-server:latest' | ||
| command: ['midnight-proof-server -v'] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In Docker Compose, does command: ['midnight-proof-server -v']pass one argv token or split it intomidnight-proof-serverand-v?
💡 Result:
In Docker Compose, when you provide the command as an array containing a single string—such as command: ['midnight-proof-server -v']—it is treated as a single argument token, not two [1]. Docker Compose parses the list ['midnight-proof-server -v'] literally [1]. It attempts to execute an executable named exactly midnight-proof-server -v (including the space and the hyphen) because that entire string is passed as the first element of the array [1]. Because Docker Compose's command uses exec form, there is no shell to interpret the space as a separator or to perform argument splitting [2][3]. If you intended to pass midnight-proof-server as the command and -v as an argument, you must split them into separate array elements [1]: command: ['midnight-proof-server', '-v'] This ensures the first element is recognized as the executable and the second as its flag [1]. While some documentation or third-party tutorials (such as certain Midnight Network guides) may incorrectly use the single-string array format, it is technically incorrect if the executable does not literally have a space in its filename [1][4][5].
Citations:
- 1: https://stackoverflow.com/questions/68768653/why-docker-does-not-recognize-bin-sh-c-as-a-valid-entrypoint
- 2: https://stackoverflow.com/questions/61333367/do-docker-composes-two-command-forms-behave-differently
- 3: https://www.dash0.com/faq/how-to-run-multiple-commands-in-docker-compose
- 4: https://github.com/midnightntwrk/example-counter/blob/main/MIGRATION_GUIDE.md
- 5: https://docs.midnight.network/guides/generating-dust-programmatically
🏁 Script executed:
sed -n '1,120p' local-env.ymlRepository: OpenZeppelin/compact-contracts
Length of output: 2036
🏁 Script executed:
sed -n '1,120p' local-env.ymlRepository: OpenZeppelin/compact-contracts
Length of output: 2036
Split the proof-server argv and pin the floating images.
command: ['midnight-proof-server -v']passes a single argv token; split it into['midnight-proof-server', '-v']so the container can start.midnightntwrk/proof-server:latestandmidnightntwrk/indexer-standalone:latestare floating tags; pin them to exact versions to keep the local stack reproducible.
Possible fix
- command: ['midnight-proof-server -v']
+ command: ['midnight-proof-server', '-v']📝 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.
| command: ['midnight-proof-server -v'] | |
| command: ['midnight-proof-server', '-v'] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@local-env.yml` at line 5, The container command is being passed as one argv
token, so split the `command` entry for the proof server into separate arguments
using the existing `command` field so it starts correctly. Also replace the
floating `midnightntwrk/proof-server:latest` and
`midnightntwrk/indexer-standalone:latest` image tags with fixed versioned tags
in the same local stack config to keep the environment reproducible.
| env-up: env-down | ||
| docker compose -f $(COMPOSE_FILE) up -d | ||
| @mkdir -p $(LOGS_DIR) | ||
| @for svc in $(SERVICES); do \ | ||
| docker compose -f $(COMPOSE_FILE) logs -f --no-log-prefix $$svc > $(LOGS_DIR)/$$svc.log 2>&1 & \ | ||
| done | ||
| @echo "Logs streaming to $(LOGS_DIR)/" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the stack to become healthy before env-up returns.
docker compose ... up -d exits once the containers start, not when proof-server, indexer, and node are actually ready. With the new test:live flow, running the suite immediately after make env-up can still fail intermittently during startup.
Possible fix
env-up: env-down
- docker compose -f $(COMPOSE_FILE) up -d
+ docker compose -f $(COMPOSE_FILE) up -d --wait
`@mkdir` -p $(LOGS_DIR)📝 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.
| env-up: env-down | |
| docker compose -f $(COMPOSE_FILE) up -d | |
| @mkdir -p $(LOGS_DIR) | |
| @for svc in $(SERVICES); do \ | |
| docker compose -f $(COMPOSE_FILE) logs -f --no-log-prefix $$svc > $(LOGS_DIR)/$$svc.log 2>&1 & \ | |
| done | |
| @echo "Logs streaming to $(LOGS_DIR)/" | |
| env-up: env-down | |
| docker compose -f $(COMPOSE_FILE) up -d --wait | |
| `@mkdir` -p $(LOGS_DIR) | |
| `@for` svc in $(SERVICES); do \ | |
| docker compose -f $(COMPOSE_FILE) logs -f --no-log-prefix $$svc > $(LOGS_DIR)/$$svc.log 2>&1 & \ | |
| done | |
| `@echo` "Logs streaming to $(LOGS_DIR)/" |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 8-8: Target body for "env-up" exceeds allowed length of 5 lines (6).
(maxbodylength)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 8 - 14, The env-up target currently returns
immediately after docker compose up -d, so it can finish before proof-server,
indexer, and node are actually ready. Update the env-up recipe to wait for the
stack to become healthy before exiting, using the existing SERVCES/COMPOSE_FILE
flow and any health-check or readiness mechanism already available in the
compose setup. Keep the log streaming behavior, but make sure the env-up target
does not complete until the key services are reported healthy and ready for
test:live.
1 participant
Types of changes
Migrates every module's per-contract simulator and unit specs from the old
synchronous
createSimulatorto the new async, backend-aware API published as@openzeppelin/compact-simulator@0.2.0(OpenZeppelin/compact-tools#121). Onefactory, two backends: a single spec file runs on both
MIDNIGHT_BACKEND=dryand=live.What changed
src/**/test/simulators/*.ts):constructor(...)→static async create(...); delegating methods returnPromise<R>; private-state helpers use the asyncgetPrivateState/setPrivateState;artifactNameadded so the live harness can locate compiled assets + ZK keys.src/**/test/*.test.ts):new X()→await X.create(); every circuit/state call awaited; failure paths useawait expect(...).rejects.toThrow(...); caller identity uses alias strings (as('OWNER')) inaccess+multisig.test:livescript,vitest.live.config.ts, thetest/integration/_harnessregisterLiveBackendharness (deploys a fresh contract per test), andmake env-up/local-env.yml(node + indexer + proof-server) — somake env-up && yarn test:liveruns the unit specs on a real node.@openzeppelin/compact-simulatorbumped^0.1.0→^0.2.0.Validation
vitest run); defaulttestCI is green.make env-up && yarn test:live); the automated CI run lands with the follow-up workflow PR.PR Checklist
Further comments
A single async factory (rather than a separate sync/async pair) keeps one simulator
file and one test file per module — no duplicate
*Backendtwins — at the cost of aone-time, mostly-mechanical async migration. The integration suite on main still uses
the old synchronous API and will be migrated in the follow-up.
Summary by CodeRabbit
New Features
Bug Fixes