chore: optimise crisp unit tests#1373
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes test setup/teardown and precomputes proofs, and introduces a cached, lazily-initialized Barretenberg API in the CRISP SDK with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as "Test Suite"
participant SDK as "crisp-sdk\ngetBBApi()/generateProof()/verifyProof()"
participant BB as "Barretenberg API\n(shared instance)"
participant Contract as "CRISP Contracts\n(On-chain verifier)"
rect rgba(200,220,255,0.5)
Test->>SDK: request proof generation or verification
SDK->>BB: getBBApi() (init if needed)
SDK->>BB: generateProof / verifyProof
BB-->>SDK: proof / verification result
SDK->>Contract: submit proof for on-chain verification (optional)
Contract-->>SDK: verification response
SDK-->>Test: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
18-21: Add suite teardown for cached proof engine resources.Since proofs are now generated once in
before(), add anafter()teardown to calldestroyBBApi()so this suite also releases cached BB resources explicitly.♻️ Suggested patch
import { hashLeaf, generateBFVKeys, SIGNATURE_MESSAGE, generateVoteProof, getAddressFromSignature, encodeSolidityProof, generateMerkleTree, SIGNATURE_MESSAGE_HASH, generateMaskVoteProof, + destroyBBApi, } from '@crisp-e3/sdk' @@ before(async function () { @@ maskProof = await generateMaskVoteProof({ publicKey, merkleLeaves: leaves, balance, slotAddress: address, numOptions: 2, }) }) + + after(() => { + destroyBBApi() + })Also applies to: 41-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts` around lines 18 - 21, The test suite caches proof engine resources during the before() setup; add an after() teardown to call destroyBBApi() to explicitly release those cached BB resources when the suite finishes. Locate the test file's suite where before() generates proofs (in crisp.contracts.test.ts) and add an after() hook that calls destroyBBApi() (or the exported cleanup function that tears down the BB API) so cached resources are cleaned up between test runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts`:
- Around line 103-107: The test currently hard-codes e3Id = 1n which relies on
prior tests consuming e3Id 0; instead capture the actual request id produced by
the request in this test. Replace the hard-coded e3Id by extracting the id from
the result/state/event returned by await mockEnclave.request(await
crispProgram.getAddress()) (or from any emitted event/state accessor on the
mockEnclave), assign that value to e3Id, and use that variable in subsequent
assertions so the test is order-independent (refer to symbols: e3Id,
mockEnclave.request, crispProgram.getAddress).
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 25-39: getBBApi can race when called concurrently because _bbApi
stays null while Barretenberg.new() is awaited; introduce an initialization
guard promise (e.g. _bbApiInitPromise) that is set before calling
Barretenberg.new() and awaited by other callers so only one
Barretenberg.new()/initSRSChonk(2 ** 21) runs; on success set _bbApi and clear
the init promise, on failure clear the init promise and rethrow so subsequent
calls can retry; also update destroyBBApi to clear both _bbApi and
_bbApiInitPromise and call _bbApi.destroy() if present.
---
Nitpick comments:
In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts`:
- Around line 18-21: The test suite caches proof engine resources during the
before() setup; add an after() teardown to call destroyBBApi() to explicitly
release those cached BB resources when the suite finishes. Locate the test
file's suite where before() generates proofs (in crisp.contracts.test.ts) and
add an after() hook that calls destroyBBApi() (or the exported cleanup function
that tears down the BB API) so cached resources are cleaned up between test
runs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/index.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
ctrlc03
force-pushed
the
chore/optimise-crisp-unit
branch
from
a0879d0 to
bb485ee
Compare
ctrlc03
force-pushed
the
chore/optimise-crisp-unit
branch
from
bb485ee to
38c60a9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
103-107:⚠️ Potential issue | 🟠 MajorRemove order-dependent
e3Idassumption.Line 104 hard-codes
e3Id = 1nbased on another test’s side effect. This breaks when the test runs alone or execution order changes. Capture the id produced by this test’srequest()call instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts` around lines 103 - 107, The test currently hard-codes e3Id = 1n relying on another test's side-effect; instead capture the id returned by this test's request call: call mockEnclave.request(await crispProgram.getAddress()) and assign its returned id to e3Id (replace the hard-coded e3Id) so the test uses the actual id produced by its own request; update any subsequent assertions or uses to reference this captured e3Id and remove the order-dependent assumption.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts`:
- Around line 41-71: Add an explicit after() teardown that releases the BB API
resources used by the shared proof generation in before(): after the
generateVoteProof and generateMaskVoteProof calls (referencing before(),
generateVoteProof, generateMaskVoteProof), call the BB API cleanup/shutdown
helper provided by the codebase (e.g., teardownBBApi(), shutdownBBApiClient(),
or the actual helper name available in your repo) and await it so cached BB
connections/resources are closed before test exit.
---
Duplicate comments:
In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts`:
- Around line 103-107: The test currently hard-codes e3Id = 1n relying on
another test's side-effect; instead capture the id returned by this test's
request call: call mockEnclave.request(await crispProgram.getAddress()) and
assign its returned id to e3Id (replace the hard-coded e3Id) so the test uses
the actual id produced by its own request; update any subsequent assertions or
uses to reference this captured e3Id and remove the order-dependent assumption.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/index.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
- examples/CRISP/packages/crisp-sdk/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (1)
41-71:⚠️ Potential issue | 🟠 MajorAdd suite teardown for cached BB API resources.
before()now generates proofs through SDK cached BB state, but this file still lacks anafter()cleanup. Add teardown to avoid cross-test resource leakage.♻️ Suggested fix
import { hashLeaf, generateBFVKeys, SIGNATURE_MESSAGE, generateVoteProof, getAddressFromSignature, encodeSolidityProof, generateMerkleTree, SIGNATURE_MESSAGE_HASH, generateMaskVoteProof, + destroyBBApi, } from '@crisp-e3/sdk' @@ before(async function () { @@ }) + + after(async function () { + await destroyBBApi() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts` around lines 41 - 71, Add an after() teardown in this test suite to clear the BB API cached state that generateVoteProof and generateMaskVoteProof rely on: implement after(async function() { /* call SDK teardown to avoid leaked resources */ await clearCachedBBState(); /* or the SDK's equivalent, e.g. resetBbApiCache() / closeBbClient() */ }); Place this after() alongside the existing before() so the cached BB resources used by generateVoteProof and generateMaskVoteProof are explicitly cleaned up between test runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 45-51: destroyBBApi can miss cleanup when getBBApi is still
initializing; add a teardown coordination flag and check it after init
completes. Introduce a boolean like _bbApiDestroyed (default false); in
destroyBBApi set _bbApiDestroyed = true and still clear/cleanup _bbApi and
_bbApiInitPromise; in getBBApi (the async initializer that resolves
_bbApiInitPromise) check _bbApiDestroyed after creating the instance and if true
immediately call instance.destroy(), set _bbApi = null, and return null so a
pending init cannot leave a live _bbApi.
---
Duplicate comments:
In `@examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts`:
- Around line 41-71: Add an after() teardown in this test suite to clear the BB
API cached state that generateVoteProof and generateMaskVoteProof rely on:
implement after(async function() { /* call SDK teardown to avoid leaked
resources */ await clearCachedBBState(); /* or the SDK's equivalent, e.g.
resetBbApiCache() / closeBbClient() */ }); Place this after() alongside the
existing before() so the cached BB resources used by generateVoteProof and
generateMaskVoteProof are explicitly cleaned up between test runs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/index.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/packages/crisp-sdk/src/index.ts
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
ctrlc03
force-pushed
the
chore/optimise-crisp-unit
branch
from
38c60a9 to
9fc9589
Compare
ctrlc03
force-pushed
the
chore/optimise-crisp-unit
branch
from
0910bea to
b6044fa
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
112-113: Broad scope of runner changes extends beyond PR objective.The PR title indicates "optimise crisp unit tests," but this file changes runners for 16 jobs, many unrelated to CRISP (e.g.,
build_ciphernode_image,test_contracts,contrib-readme-job). Consider:
- Updating the PR title/description to reflect the broader scope, or
- Splitting non-CRISP changes into a separate PR for clarity.
This is purely an organizational suggestion and doesn't block the changes.
Also applies to: 149-150, 189-190, 224-225, 237-238, 356-357, 573-574, 665-666, 694-695, 750-751, 871-872, 908-909
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 112 - 113, The CI workflow change updates the runner for many jobs (e.g., build_e3_support_risc0, build_ciphernode_image, test_contracts, contrib-readme-job) which is broader than the PR title "optimise crisp unit tests"; either update the PR title and description to accurately reflect that multiple jobs' runners were changed, or split the unrelated job runner changes into a separate PR (keep only CRISP-related job changes like the CRISP test job in this PR and move others such as build_ciphernode_image, test_contracts, contrib-readme-job, etc. into a new branch/PR). Ensure the commit message(s) and PR body list the specific job names changed so reviewers can easily see the scope.
36-39: Inconsistent runner selection remains across jobs.Three jobs still use the custom
enclave-cirunner:
rust_tests(lines 37-39)crisp_e2e(lines 461-463)template_integration(lines 798-800)If this is intentional (e.g., these jobs require more resources), consider adding a comment in the workflow explaining why certain jobs need custom runners. This helps future maintainers understand the decision.
Also applies to: 460-463, 797-800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 36 - 39, The workflow uses a custom runner for the jobs rust_tests, crisp_e2e, and template_integration which is inconsistent with other jobs; either switch these jobs to the standard runner configuration (remove group: enclave-ci and labels: [enclave-ci-runner] and use the shared runs-on value used elsewhere) or, if the custom runner is required, add a short explanatory comment immediately above each job (rust_tests, crisp_e2e, template_integration) stating why the enclave-ci runner is necessary (resource or hardware dependency) so future maintainers understand the exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 112-113: The CI workflow change updates the runner for many jobs
(e.g., build_e3_support_risc0, build_ciphernode_image, test_contracts,
contrib-readme-job) which is broader than the PR title "optimise crisp unit
tests"; either update the PR title and description to accurately reflect that
multiple jobs' runners were changed, or split the unrelated job runner changes
into a separate PR (keep only CRISP-related job changes like the CRISP test job
in this PR and move others such as build_ciphernode_image, test_contracts,
contrib-readme-job, etc. into a new branch/PR). Ensure the commit message(s) and
PR body list the specific job names changed so reviewers can easily see the
scope.
- Around line 36-39: The workflow uses a custom runner for the jobs rust_tests,
crisp_e2e, and template_integration which is inconsistent with other jobs;
either switch these jobs to the standard runner configuration (remove group:
enclave-ci and labels: [enclave-ci-runner] and use the shared runs-on value used
elsewhere) or, if the custom runner is required, add a short explanatory comment
immediately above each job (rust_tests, crisp_e2e, template_integration) stating
why the enclave-ci runner is necessary (resource or hardware dependency) so
future maintainers understand the exception.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.ymlexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/index.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/packages/crisp-sdk/src/index.ts
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
ctrlc03
force-pushed
the
chore/optimise-crisp-unit
branch
from
b6044fa to
e4e192e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
30-39:⚠️ Potential issue | 🟠 Major
destroyBBApi()can still leak a live instance when teardown races with initialization.If
destroyBBApi()runs while the initializer fromgetBBApi()is in-flight, teardown returns, but the pending initializer can still assign_bbApiafterward. That leaves resources alive after explicit cleanup and can cause flaky shutdown behavior.♻️ Suggested fix
let _bbApi: Barretenberg | null = null let _bbApiInitPromise: Promise<Barretenberg> | null = null +let _bbApiGeneration = 0 const getBBApi = async (): Promise<Barretenberg> => { if (_bbApi) return _bbApi if (_bbApiInitPromise) return _bbApiInitPromise + const generationAtStart = _bbApiGeneration _bbApiInitPromise = (async () => { try { const api = await Barretenberg.new() await api.initSRSChonk(2 ** 21) + if (generationAtStart !== _bbApiGeneration) { + api.destroy() + throw new Error('Barretenberg API was destroyed during initialization') + } _bbApi = api return api } finally { _bbApiInitPromise = null } })() return _bbApiInitPromise } export const destroyBBApi = (): void => { + _bbApiGeneration += 1 _bbApiInitPromise = null if (_bbApi) { _bbApi.destroy() _bbApi = null } }Also applies to: 45-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-sdk/src/vote.ts` around lines 30 - 39, The initializer can overwrite cleanup by assigning _bbApi after destroyBBApi clears it; fix by capturing the current init promise in getBBApi()/the IIFE (use a local const like initPromise = _bbApiInitPromise) and before assigning _bbApi/returning api verify that _bbApiInitPromise === initPromise (or check a dedicated destroyed flag set by destroyBBApi); update destroyBBApi to clear _bbApiInitPromise and _bbApi and close the instance as before so the initializer aborts its assignment when a teardown raced in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 30-39: The initializer can overwrite cleanup by assigning _bbApi
after destroyBBApi clears it; fix by capturing the current init promise in
getBBApi()/the IIFE (use a local const like initPromise = _bbApiInitPromise) and
before assigning _bbApi/returning api verify that _bbApiInitPromise ===
initPromise (or check a dedicated destroyed flag set by destroyBBApi); update
destroyBBApi to clear _bbApiInitPromise and _bbApi and close the instance as
before so the initializer aborts its assignment when a teardown raced in.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.ymlexamples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.tsexamples/CRISP/packages/crisp-sdk/src/index.tsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- examples/CRISP/packages/crisp-sdk/src/index.ts
ctrlc03
force-pushed
the
chore/optimise-crisp-unit
branch
from
e4e192e to
8cd44f7
Compare
0xjei
approved these changes
Mar 1, 2026
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
2 participants
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.
Use ubuntu-latest unless strictly necessary to use larger runner. Avoid re-generating proofs in crisp contract tests
Summary by CodeRabbit
New Features
Refactor
Tests
Chores