refactor: enclave-sdk modularity [skip-line-limit]#1388
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRefactors Enclave SDK into modular exports (crypto, contracts, events) with a factory method, replaces chainId with viem Chain, introduces new methods (getE3Quote, getE3Stage, getFailureReason), updates requestE3 parameter structure, adds feeToken to contracts, restructures event system, and reorganizes internal implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EnclaveSDK
participant ContractClient
participant EventListener
participant Blockchain
Client->>EnclaveSDK: create(options)
activate EnclaveSDK
EnclaveSDK->>ContractClient: new ContractClient(config)
activate ContractClient
ContractClient->>Blockchain: validate addresses
ContractClient-->>EnclaveSDK: initialized
deactivate ContractClient
EnclaveSDK->>EventListener: new EventListener(options)
activate EventListener
EventListener-->>EnclaveSDK: initialized
deactivate EventListener
EnclaveSDK-->>Client: sdk instance
deactivate EnclaveSDK
Client->>EnclaveSDK: requestE3(inputWindow, ...)
EnclaveSDK->>ContractClient: requestE3(params)
ContractClient->>Blockchain: simulate & execute contract call
Blockchain-->>ContractClient: transaction hash
ContractClient-->>EnclaveSDK: hash
EnclaveSDK-->>Client: hash
EnclaveSDK->>EventListener: onEnclaveEvent(E3_REQUESTED, callback)
activate EventListener
EventListener->>Blockchain: watch contract events
Blockchain-->>EventListener: E3_REQUESTED event emitted
EventListener->>Client: invoke callback(event)
deactivate EventListener
sequenceDiagram
participant Client
participant EnclaveSDK
participant CryptoModule
participant WASMModule
Client->>EnclaveSDK: generatePublicKey()
EnclaveSDK->>CryptoModule: generatePublicKey(presetName)
activate CryptoModule
CryptoModule->>WASMModule: resolveParams(presetName)
activate WASMModule
WASMModule-->>CryptoModule: BfvParams
deactivate WASMModule
CryptoModule->>WASMModule: bfv_generate_public_key(...)
WASMModule-->>CryptoModule: publicKey (Uint8Array)
deactivate CryptoModule
CryptoModule-->>EnclaveSDK: publicKey
EnclaveSDK-->>Client: publicKey
Client->>EnclaveSDK: encryptNumberAndGenProof(data, publicKey)
EnclaveSDK->>CryptoModule: encryptNumberAndGenProof(...)
activate CryptoModule
CryptoModule->>WASMModule: bfv_verifiable_encrypt_number(...)
WASMModule-->>CryptoModule: encryptedData
CryptoModule->>WASMModule: generateProof(circuitInputs)
WASMModule-->>CryptoModule: proof
CryptoModule-->>EnclaveSDK: VerifiableEncryptionResult
deactivate CryptoModule
EnclaveSDK-->>Client: VerifiableEncryptionResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This refactoring involves significant architectural changes across multiple new modules, reorganized exports, updated public API signatures, and subtle semantic shifts (chainId→chain, event renaming). The heterogeneity of changes (new module structure, type reorganization, delegation patterns, documentation updates) requires careful verification of internal consistency, import path correctness, and delegation logic. Possibly related PRs
Suggested labels
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 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: 6
🧹 Nitpick comments (5)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
38-38: Consider consistent formatting for both mock functions.The
mockPreviousCiphertextNotFoundResponsefunction now uses a single-line format, whilemockGetPreviousCiphertextResponse(lines 31-36) uses a multi-line format. For consistency and readability, consider using the same formatting style for both mock functions.♻️ Proposed fix for consistency
Either expand this function to multi-line to match the other mock:
- const mockPreviousCiphertextNotFoundResponse = () => ({ ok: false, status: 404 }) as Response + const mockPreviousCiphertextNotFoundResponse = () => + ({ + ok: false, + status: 404, + }) as ResponseOr condense the other mock to single-line:
- const mockGetPreviousCiphertextResponse = () => - ({ - ok: true, - status: 200, - json: async () => ({ ciphertext: previousCiphertext }), - }) as Response + const mockGetPreviousCiphertextResponse = () => ({ ok: true, status: 200, json: async () => ({ ciphertext: previousCiphertext }) }) as ResponseNote: The first option (expanding to multi-line) is generally more readable for mock functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/packages/crisp-sdk/tests/vote.test.ts` at line 38, The two mock helpers are formatted inconsistently: mockGetPreviousCiphertextResponse is multi-line while mockPreviousCiphertextNotFoundResponse is single-line; pick one style and make both match (prefer expanding mockPreviousCiphertextNotFoundResponse to a multi-line function for readability). Update the mockPreviousCiphertextNotFoundResponse function definition to mirror the shape and style of mockGetPreviousCiphertextResponse (same return casting to Response, explicit property lines) so both helpers use the same multi-line formatting.docs/pages/sdk.mdx (1)
143-159: Add a note that current encryption helpers are BFV-only.A short caveat here will prevent users from assuming TRBFV encryption is fully available in this SDK surface today.
Based on learnings: In
packages/enclave-sdk/src/enclave-sdk.ts, TRBFV parameter setup is intentionally incomplete and encryption methods intentionally support BFV only, throwing “Protocol not supported” for TRBFV.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pages/sdk.mdx` around lines 143 - 159, Add a short caveat to the "Encryption functions" section explaining that the SDK's current encryption helpers support BFV only and TRBFV is not implemented; update the mdx text near the examples to include one sentence noting "BFV-only (TRBFV not yet supported)" so users aren't misled, and cross-reference the implementation location (packages/enclave-sdk/src/enclave-sdk.ts) which currently throws "Protocol not supported" for TRBFV and contains incomplete TRBFV parameter setup; ensure the note mentions both SDK-instance helpers (generatePublicKey, encryptNumber, encryptNumberAndGenProof) and standalone imports (generatePublicKey, encryptNumber) so it’s clear across both usage patterns.packages/enclave-sdk/src/encryption/encrypt.ts (1)
20-29: Cache resolved preset params to avoid repeated setup overhead.
resolveParamsruns for every encrypt/commit call, which repeatedly does wasm-init checks and param materialization. Memoizing by preset will reduce hot-path latency/allocation churn.♻️ Suggested refactor
+const wasmInitPromise = initializeWasm() +const paramsCache = new Map<ThresholdBfvParamsPresetName, BfvParams>() + async function resolveParams(presetName: ThresholdBfvParamsPresetName): Promise<BfvParams> { - await initializeWasm() - const params = get_bfv_params(presetName) - return { + await wasmInitPromise + + const cached = paramsCache.get(presetName) + if (cached) return cached + + const params = get_bfv_params(presetName) + const resolved: BfvParams = { degree: Number(params.degree), plaintextModulus: params.plaintext_modulus as bigint, moduli: params.moduli as bigint[], error1Variance: params.error1_variance, } + + paramsCache.set(presetName, resolved) + return resolved }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/encryption/encrypt.ts` around lines 20 - 29, resolveParams currently calls initializeWasm and materializes BfvParams on every invocation; add memoization keyed by presetName to avoid repeated setup: create a module-level Map<ThresholdBfvParamsPresetName, BfvParams>, then in resolveParams check the map first and return the cached BfvParams if present; if missing, call initializeWasm(), call get_bfv_params(presetName), build the BfvParams object (degree, plaintextModulus, moduli, error1Variance) and store it in the map under presetName before returning it. Ensure keys use the ThresholdBfvParamsPresetName and keep the same returned shape as currently produced.packages/enclave-sdk/src/enclave-sdk.ts (2)
140-149: ReuseE3RequestParamsinrequestE3to prevent type drift.Line 140 duplicates the request shape already represented by
E3RequestParams. Using one shared type keepsrequestE3andgetE3Quotealigned.♻️ Proposed refactor
- public async requestE3(params: { - threshold: [number, number] - inputWindow: [bigint, bigint] - e3Program: `0x${string}` - e3ProgramParams: `0x${string}` - computeProviderParams: `0x${string}` - customParams?: `0x${string}` - gasLimit?: bigint - }): Promise<Hash> { + public async requestE3(params: E3RequestParams): Promise<Hash> { return this.contractClient.requestE3(params) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/enclave-sdk.ts` around lines 140 - 149, The requestE3 method duplicates the parameter shape; update its signature to accept the existing E3RequestParams type (e.g., change requestE3(params: { ... }) to requestE3(params: E3RequestParams): Promise<Hash>) so it reuses the shared type used by getE3Quote, and keep the body returning this.contractClient.requestE3(params) unchanged; ensure any needed import or export of E3RequestParams is present so the method compiles.
213-215: Await the async startPolling() call to propagate startup errors.Line 214 uses
voidto drop thestartPolling()result. SincestartPolling()is async and throwsSDKErroron failure, the error won't propagate to callers ofstartEventPolling(), preventing proper error handling.🛠️ Proposed fix
public async startEventPolling(): Promise<void> { - void this.eventListener.startPolling() + await this.eventListener.startPolling() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/enclave-sdk.ts` around lines 213 - 215, The startEventPolling method drops the promise from eventListener.startPolling() with void so any SDKError thrown there won't reach callers; change the call in startEventPolling to await eventListener.startPolling() (or return its Promise) so errors propagate to callers of startEventPolling and can be handled upstream; update any callers/tests if they relied on suppressed errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pages/sdk.mdx`:
- Line 88: The example for inputWindow uses Date.now() (milliseconds) which is
incorrect for on-chain seconds-based timestamps; update the inputWindow example
to use seconds by dividing Date.now() by 1000 (e.g., Math.floor(Date.now() /
1000)) and likewise convert the offset (5 * 60 * 1000) to seconds (5 * 60) so
inputWindow uses correct second-based values; modify the expression that sets
inputWindow to use these second conversions near the inputWindow example line.
In `@packages/enclave-sdk/src/events/event-listener.ts`:
- Around line 54-57: Change onEnclaveEvent to return Promise<void> instead of
void and propagate the promise from watchContractEvent so callers can observe
setup failures (watchContractEvent currently can throw SDKError); specifically,
update the signature of onEnclaveEvent<T extends AllEventTypes>(...) to return
Promise<void> and replace the fire-and-forget call with a returned/awaited call
to this.watchContractEvent(address, eventType, abi, callback). Then update all
callers (including once() and references in enclave-sdk.ts) to either await the
returned promise or explicitly handle/ignore it (e.g., try/catch or void-ignore)
so unhandled rejections are eliminated.
- Around line 168-169: The fallback logic for block ranges in event-listener.ts
uses logical OR which treats 0n as falsy and drops intentional block 0 values;
update the assignments that set fromBlock and toBlock (currently using fromBlock
|| this.config.fromBlock and toBlock || this.config.toBlock) to use nullish
coalescing (fromBlock ?? this.config.fromBlock and toBlock ??
this.config.toBlock) so explicit 0n values are preserved when calling the
methods that consume these ranges.
In `@templates/default/server/index.ts`:
- Line 44: The SDK initialization hardcodes chain: hardhat in
templates/default/server/index.ts which causes mismatched chain metadata; update
the SDK creation (where chain is set) to accept a configurable value instead of
the literal 'hardhat' — e.g., read from an environment variable like CHAIN or
infer from RPC_URL (or omit the chain field so the RPC provider determines it)
and pass that value into the SDK initialization (refer to the property `chain`
used during SDK creation and the surrounding initialization function where
RPC_URL/PRIVATE_KEY are read). Ensure the new behavior defaults safely (e.g., to
mainnet or provider-inferred) when CHAIN is not provided.
In `@templates/default/tests/integration.spec.ts`:
- Around line 203-205: The test is using a hardcoded e3Id (0n) when calling
getE3Stage which can fail if prior requests exist; instead capture the runtime
e3Id emitted by the request/state event (the value returned or emitted when
calling requestE3 or from the event listener earlier in the test) and pass that
variable to sdk.getE3Stage, replacing the literal 0n (also update the similar
checks at the other occurrence around the E3Stage assertions); reference
getE3Stage and the requestE3/event emission to locate where to read and reuse
the dynamic e3Id.
- Around line 179-190: The test currently approves a fixed token amount before
fetching the fee quote; change the flow to first call
sdk.getE3Quote(requestParams) (using the existing requestParams and quote
variable) and then use that returned quote to drive the token approval call
(e.g., pass quote to the token approval method you use in the test such as
token.approve(...) or the helper that mints/approves) so the approval amount
matches the quoted fee; keep the assertion assert(quote >= 0n) and replace the
prior fixed-amount approval with an approval that uses the quote value.
---
Nitpick comments:
In `@docs/pages/sdk.mdx`:
- Around line 143-159: Add a short caveat to the "Encryption functions" section
explaining that the SDK's current encryption helpers support BFV only and TRBFV
is not implemented; update the mdx text near the examples to include one
sentence noting "BFV-only (TRBFV not yet supported)" so users aren't misled, and
cross-reference the implementation location
(packages/enclave-sdk/src/enclave-sdk.ts) which currently throws "Protocol not
supported" for TRBFV and contains incomplete TRBFV parameter setup; ensure the
note mentions both SDK-instance helpers (generatePublicKey, encryptNumber,
encryptNumberAndGenProof) and standalone imports (generatePublicKey,
encryptNumber) so it’s clear across both usage patterns.
In `@examples/CRISP/packages/crisp-sdk/tests/vote.test.ts`:
- Line 38: The two mock helpers are formatted inconsistently:
mockGetPreviousCiphertextResponse is multi-line while
mockPreviousCiphertextNotFoundResponse is single-line; pick one style and make
both match (prefer expanding mockPreviousCiphertextNotFoundResponse to a
multi-line function for readability). Update the
mockPreviousCiphertextNotFoundResponse function definition to mirror the shape
and style of mockGetPreviousCiphertextResponse (same return casting to Response,
explicit property lines) so both helpers use the same multi-line formatting.
In `@packages/enclave-sdk/src/enclave-sdk.ts`:
- Around line 140-149: The requestE3 method duplicates the parameter shape;
update its signature to accept the existing E3RequestParams type (e.g., change
requestE3(params: { ... }) to requestE3(params: E3RequestParams): Promise<Hash>)
so it reuses the shared type used by getE3Quote, and keep the body returning
this.contractClient.requestE3(params) unchanged; ensure any needed import or
export of E3RequestParams is present so the method compiles.
- Around line 213-215: The startEventPolling method drops the promise from
eventListener.startPolling() with void so any SDKError thrown there won't reach
callers; change the call in startEventPolling to await
eventListener.startPolling() (or return its Promise) so errors propagate to
callers of startEventPolling and can be handled upstream; update any
callers/tests if they relied on suppressed errors.
In `@packages/enclave-sdk/src/encryption/encrypt.ts`:
- Around line 20-29: resolveParams currently calls initializeWasm and
materializes BfvParams on every invocation; add memoization keyed by presetName
to avoid repeated setup: create a module-level Map<ThresholdBfvParamsPresetName,
BfvParams>, then in resolveParams check the map first and return the cached
BfvParams if present; if missing, call initializeWasm(), call
get_bfv_params(presetName), build the BfvParams object (degree,
plaintextModulus, moduli, error1Variance) and store it in the map under
presetName before returning it. Ensure keys use the ThresholdBfvParamsPresetName
and keep the same returned shape as currently produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d21244a3-ec03-453a-a01a-58d7327636a9
📒 Files selected for processing (23)
docs/pages/sdk.mdxexamples/CRISP/packages/crisp-sdk/tests/vote.test.tspackages/enclave-react/src/useEnclaveSDK.tspackages/enclave-sdk/README.mdpackages/enclave-sdk/package.jsonpackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/contracts/contract-client.tspackages/enclave-sdk/src/contracts/index.tspackages/enclave-sdk/src/contracts/types.tspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/encryption/encrypt.tspackages/enclave-sdk/src/encryption/index.tspackages/enclave-sdk/src/encryption/types.tspackages/enclave-sdk/src/events/event-listener.tspackages/enclave-sdk/src/events/index.tspackages/enclave-sdk/src/events/types.tspackages/enclave-sdk/src/index.tspackages/enclave-sdk/src/types.tspackages/enclave-sdk/tests/sdk.test.tspackages/enclave-sdk/tsup.config.jstemplates/default/server/index.tstemplates/default/server/types.tstemplates/default/tests/integration.spec.ts
💤 Files with no reviewable changes (1)
- packages/enclave-sdk/src/contract-client.ts
ctrlc03
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
templates/default/server/index.ts (1)
36-45:⚠️ Potential issue | 🟠 MajorMake chain selection configurable instead of forcing
hardhat.Line 44 hard-codes
chain: hardhat, which can mismatchRPC_URLand cause incorrect chain context for writes/signing outside local dev.#!/bin/bash # Verify chain is hard-coded and no dynamic chain selection is used here. rg -nP "rpcUrl|chain:\s*hardhat|CHAIN_ID|CHAIN" templates/default/server/index.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/server/index.ts` around lines 36 - 45, The EnclaveSDK.create call hardcodes chain: hardhat which can mismatch RPC_URL; update the initialization in EnclaveSDK.create to accept a configurable chain value (e.g., read from an env var like CHAIN or derive from RPC_URL/CHAIN_ID) instead of the literal hardhat so runtime chain context matches RPC_URL; locate the EnclaveSDK.create invocation in templates/default/server/index.ts and replace the fixed chain: hardhat with a variable (validated/defaulted) that is used when constructing the SDK (ensure PRIVATE_KEY, RPC_URL and the new chain variable are validated together).templates/default/tests/integration.spec.ts (1)
186-190:⚠️ Potential issue | 🟡 MinorWait for the approval receipt instead of sleeping.
After
approveFeeToken(quote), relying on a fixed 1s delay is flaky; confirm the tx withwaitForTransaction(hash)beforerequestE3.Suggested fix
// Approve fee token console.log('Approving fee token...') const hash = await sdk.approveFeeToken(quote) + await sdk.waitForTransaction(hash) console.log('Fee token approved:', hash) - - await new Promise((resolve) => setTimeout(resolve, 1000))#!/bin/bash # Verify approval path and current synchronization strategy. rg -nP "approveFeeToken|waitForTransaction|setTimeout" templates/default/tests/integration.spec.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/tests/integration.spec.ts` around lines 186 - 190, The test uses a fixed 1s sleep after calling approveFeeToken(quote) which is flaky; replace the sleep with an explicit transaction confirmation by awaiting the transaction receipt for the returned hash (use waitForTransaction on the provider or SDK, e.g., provider.waitForTransaction(hash) or sdk.waitForTransaction(hash)) and verify the receipt/confirmation before proceeding to requestE3 so the approval is truly mined; update the block around approveFeeToken, the removed setTimeout, and any subsequent logic that assumes the approval is finalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pages/sdk.mdx`:
- Around line 88-91: The doc example sets inputWindow start to current time
which can expire before on-chain execution; update the example for inputWindow
so the start is a short buffered future time (e.g., now + bufferSeconds) and the
end is start + desiredDuration to avoid mempool/mining delays; reference the
inputWindow array and the requestE3 call so readers know to forward this
buffered window to requestE3 and consider making bufferSeconds a small constant
(30–60s) or configurable value.
- Around line 222-223: Add a short inline example demonstrating how to derive
the fee amount via sdk.getE3Quote(params) and then pass that derived amount into
sdk.approveFeeToken(amount) before calling sdk.requestE3 with the same request
parameters; reference the exact methods sdk.getE3Quote, sdk.approveFeeToken, and
sdk.requestE3 so readers see the intended sequence (getE3Quote → approveFeeToken
→ requestE3) and that the quote is produced from the same params used for
requestE3.
In `@packages/enclave-sdk/src/enclave-sdk.ts`:
- Around line 213-215: The startEventPolling() method currently discards the
promise from this.eventListener.startPolling() (using void) so callers cannot
observe SDKError; change startEventPolling to return the underlying promise
instead by removing the async modifier and returning
this.eventListener.startPolling() directly (no void) so callers of
startEventPolling() can await and handle startup errors thrown by
eventListener.startPolling().
In `@packages/enclave-sdk/src/events/event-listener.ts`:
- Around line 46-50: resolveContract misroutes events because it checks
membership against EnclaveEventType by string values, causing collisions with
identical strings in RegistryEventType; update resolveContract to disambiguate
enums by explicitly checking RegistryEventType first (e.g., use
Object.values(RegistryEventType).includes(eventType as RegistryEventType)) or
otherwise use a direct mapping/switch on AllEventTypes so that
RegistryEventType.OWNERSHIP_TRANSFERRED and .INITIALIZED map to
contracts.ciphernodeRegistry with CiphernodeRegistryOwnable__factory.abi while
EnclaveEventType values map to contracts.enclave with Enclave__factory.abi;
ensure the check uses the enum identity (RegistryEventType / EnclaveEventType)
rather than string value equality to choose the correct contract and ABI.
---
Duplicate comments:
In `@templates/default/server/index.ts`:
- Around line 36-45: The EnclaveSDK.create call hardcodes chain: hardhat which
can mismatch RPC_URL; update the initialization in EnclaveSDK.create to accept a
configurable chain value (e.g., read from an env var like CHAIN or derive from
RPC_URL/CHAIN_ID) instead of the literal hardhat so runtime chain context
matches RPC_URL; locate the EnclaveSDK.create invocation in
templates/default/server/index.ts and replace the fixed chain: hardhat with a
variable (validated/defaulted) that is used when constructing the SDK (ensure
PRIVATE_KEY, RPC_URL and the new chain variable are validated together).
In `@templates/default/tests/integration.spec.ts`:
- Around line 186-190: The test uses a fixed 1s sleep after calling
approveFeeToken(quote) which is flaky; replace the sleep with an explicit
transaction confirmation by awaiting the transaction receipt for the returned
hash (use waitForTransaction on the provider or SDK, e.g.,
provider.waitForTransaction(hash) or sdk.waitForTransaction(hash)) and verify
the receipt/confirmation before proceeding to requestE3 so the approval is truly
mined; update the block around approveFeeToken, the removed setTimeout, and any
subsequent logic that assumes the approval is finalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5d020df-431c-43d2-840e-9a44f95ae112
📒 Files selected for processing (5)
docs/pages/sdk.mdxpackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/events/event-listener.tstemplates/default/server/index.tstemplates/default/tests/integration.spec.ts
cedoor
suggested changes
Mar 6, 2026
Contributor
There was a problem hiding this comment.
We should alse rename greco.ts with user-data-encryption.ts, perhaps renaming the encryption module with crypto and adding zk stuff there.
Also, if we want to be consistent with the other code in the project, thresholdBfvParamsPresetName should be optional and we shoud have a default set (insecure now).
ctrlc03
force-pushed
the
refactor/enclave-sdk
branch
from
b9e10b9 to
4c4e0f2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-sdk/README.md (1)
44-74: 🛠️ Refactor suggestion | 🟠 MajorAdd migration guide for breaking changes.
The Quick Start example shows several breaking changes from the previous SDK version:
chainId→chain(viem Chain object)feeTokennow required in contracts configthresholdBfvParamsPresetNamenow requiredrequestE3parameters changed (duration→inputWindow)Consider adding a dedicated "Migration Guide" section to help existing users upgrade, documenting what changed and how to update their code.
📝 Suggested migration guide structure
Add a section before or after "Quick Start":
+## Migration Guide + +### Upgrading from v1.x to v2.x + +#### Configuration Changes + +```typescript +// Before +const sdk = new EnclaveSDK({ + // ... + chainId: 11155111, + contracts: { + enclave: '0x...', + ciphernodeRegistry: '0x...', + }, +}) + +// After +import { sepolia } from 'viem/chains' + +const sdk = new EnclaveSDK({ + // ... + chain: sepolia, + contracts: { + enclave: '0x...', + ciphernodeRegistry: '0x...', + feeToken: '0x...', // Now required + }, + thresholdBfvParamsPresetName: 'INSECURE_THRESHOLD_512', // Now required +}) +``` + +#### Event Type Changes + +The following events have been removed: +- `EnclaveEventType.E3_ACTIVATED` - Use `EnclaveEventType.E3_REQUESTED` instead +- `EnclaveEventType.INPUT_PUBLISHED` - [Document replacement event] + +#### API Changes + +```typescript +// Before +await sdk.requestE3({ + duration: [BigInt(0), BigInt(100)], + // ... +}) + +// After +await sdk.requestE3({ + inputWindow: [BigInt(0), BigInt(100)], + // ... +}) +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/README.md` around lines 44 - 74, Add a "Migration Guide" section to README describing breaking changes and how to update code: explain that EnclaveSDK now accepts viem Chain objects via the chain option (replace chainId), that contracts.enclave + contracts.ciphernodeRegistry must now include contracts.feeToken, that thresholdBfvParamsPresetName is required, and that requestE3's duration parameter was renamed to inputWindow; include before/after snippets showing EnclaveSDK construction (reference EnclaveSDK, chain, feeToken, thresholdBfvParamsPresetName) and requestE3 usage (reference requestE3 and inputWindow), and list removed/renamed events (mention EnclaveEventType.E3_ACTIVATED → EnclaveEventType.E3_REQUESTED and any INPUT_PUBLISHED replacement) — place this section adjacent to the Quick Start for discoverability.
♻️ Duplicate comments (4)
templates/default/tests/integration.spec.ts (2)
184-190:⚠️ Potential issue | 🟡 MinorWait for approval mining instead of sleeping.
The fixed one-second delay is still flaky;
requestE3can race the allowance update. Usesdk.waitForTransaction(...)here, as you already do for published inputs later in the test.🧪 Suggested test update
- const hash = await sdk.approveFeeToken(quote) - console.log('Fee token approved:', hash) - - await new Promise((resolve) => setTimeout(resolve, 1000)) + const approvalHash = await sdk.approveFeeToken(quote) + await sdk.waitForTransaction(approvalHash) + console.log('Fee token approved:', approvalHash)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/tests/integration.spec.ts` around lines 184 - 190, Replace the fixed 1s sleep after calling sdk.approveFeeToken(quote) with a proper wait for the approval transaction to be mined: capture the returned hash from approveFeeToken (variable hash) and call sdk.waitForTransaction(hash) (same sdk instance used elsewhere) instead of setTimeout so the test only proceeds once the allowance update is confirmed.
192-200:⚠️ Potential issue | 🟡 MinorStop assuming the new request ID is
0n.This still breaks on non-empty chains. Reuse the
E3_REQUESTEDevent returned bywaitForEventand key the store lookups/assertions off that runtimee3Id.🧪 Suggested test update
- await waitForEvent(EnclaveEventType.E3_REQUESTED, async () => { + const requestedEvent = await waitForEvent(EnclaveEventType.E3_REQUESTED, async () => { console.log('Requested E3...') await sdk.requestE3(requestParams) }) - state = store.get(0n) + state = store.get(requestedEvent.data.e3Id) assert(state, 'store should have E3State but it was falsey') - assert.strictEqual(state.e3Id, 0n) + assert.strictEqual(state.e3Id, requestedEvent.data.e3Id) assert.strictEqual(state.type, 'requested')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/tests/integration.spec.ts` around lines 192 - 200, The test incorrectly assumes the new request ID is 0n; instead capture the E3_REQUESTED event returned by waitForEvent and use its runtime e3Id for assertions. Change the waitForEvent call to assign its result (from waitForEvent(EnclaveEventType.E3_REQUESTED, ...)), extract the e3Id from that event, then call store.get(e3Id) and assert that state.e3Id === e3Id and state.type === 'requested' (references: waitForEvent, EnclaveEventType.E3_REQUESTED, sdk.requestE3, store.get, state.e3Id).docs/pages/sdk.mdx (1)
85-97:⚠️ Potential issue | 🟠 MajorMake the request example runnable as written.
Right now the snippet starts
inputWindowat the current second and skips thegetE3Quote→approveFeeTokenstep, so a copied example can fail before or duringrequestE3. Show a buffered future window and derive approval from the samerequestParamsobject.📝 Suggested doc update
-const hash = await sdk.requestE3({ +const now = BigInt(Math.floor(Date.now() / 1000)) +const requestParams = { threshold: [2, 3], - inputWindow: [ - BigInt(Math.floor(Date.now() / 1000)), - BigInt(Math.floor(Date.now() / 1000) + 5 * 60), - ], + inputWindow: [now + 30n, now + 30n + 5n * 60n], e3Program: '0x...', e3ProgramParams: '0x', computeProviderParams: '0x', customParams: '0x', -}) +} + +const quote = await sdk.getE3Quote(requestParams) +const approvalHash = await sdk.approveFeeToken(quote) +await sdk.waitForTransaction(approvalHash) +const hash = await sdk.requestE3(requestParams)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pages/sdk.mdx` around lines 85 - 97, Update the example so inputWindow is a buffered future window and show the full flow: build a requestParams object (containing threshold, inputWindow, e3Program, e3ProgramParams, computeProviderParams, customParams), call sdk.getE3Quote(requestParams) and then sdk.approveFeeToken(quote) to approve fees, and only after approval call sdk.requestE3(requestParams); reference the sdk.requestE3, sdk.getE3Quote, sdk.approveFeeToken and requestParams symbols and ensure inputWindow uses Date.now() plus a buffer (e.g., +60s or more) so the request is valid when executed.templates/default/server/index.ts (1)
36-45:⚠️ Potential issue | 🟠 MajorMake the template chain configurable.
This still hard-codes
hardhatwhen building the SDK, so any non-localRPC_URLgets the wrong chain metadata for reads/writes. The template should derive the chain from deployment config instead of baking in a single network.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/default/server/index.ts` around lines 36 - 45, The template currently hard-codes the chain as the literal hardhat in the EnclaveSDK.create call; change the chain property to read the chain/network from your deployment configuration (or derive it from the RPC_URL) instead of the string 'hardhat' so non-local RPC_URLs use the correct chain metadata—update the chain field in EnclaveSDK.create (referenced as the chain property in EnclaveSDK.create) to use the deployment config value (e.g., DEPLOYMENT.chain or a getChainFromRpc(RPC_URL) helper) rather than the hardcoded hardhat.
🧹 Nitpick comments (4)
packages/enclave-sdk/src/events/event-listener.ts (1)
46-63: Clarify behavior for registry-specificOwnershipTransferredandInitializedevents.The comment at lines 46-49 acknowledges that
OwnershipTransferredandInitializedexist in both enums with identical string values and default to the Enclave contract. However, if a caller needs to watch these events specifically on the CiphernodeRegistry contract, there's currently no way to do so.Consider whether this limitation should be documented in the public API or if an alternative mechanism (e.g., explicit contract parameter) should be provided for these edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/events/event-listener.ts` around lines 46 - 63, The current resolveContract logic uses REGISTRY_ONLY_EVENTS and will always route ambiguous names like "OwnershipTransferred" and "Initialized" to the Enclave (see REGISTRY_ONLY_EVENTS, resolveContract, AllEventTypes, contracts.ciphernodeRegistry, contracts.enclave, CiphernodeRegistryOwnable__factory, Enclave__factory); update the API to allow callers to explicitly target a contract for ambiguous event names (e.g., add an optional parameter like targetContract or forceRegistry to the public watch/subscribe method that resolveContract reads) and/or add clear public docs stating that shared event names default to the Enclave and how to opt into the registry-specific behavior so callers can watch those events on CiphernodeRegistry when needed.packages/enclave-sdk/src/crypto/encrypt.ts (3)
61-68: Add error handling forJSON.parseon circuit inputs.If the WASM function returns malformed JSON for
circuitInputs,JSON.parsewill throw a genericSyntaxError. Consider wrapping this in a try-catch to provide a more descriptive SDK error.Suggested improvement
- return { encryptedData, circuitInputs: JSON.parse(circuitInputs) } + try { + return { encryptedData, circuitInputs: JSON.parse(circuitInputs) } + } catch (e) { + throw new Error(`Failed to parse circuit inputs: ${e}`) + }Note: Import
SDKErrorfrom../utilsif a consistent error type is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/crypto/encrypt.ts` around lines 61 - 68, Wrap the JSON.parse call for circuitInputs (from bfv_verifiable_encrypt_number) in a try-catch and convert parse failures into a descriptive SDKError: capture the raw circuitInputs string, attempt JSON.parse, and on catch throw new SDKError with a message like "Failed to parse circuitInputs from bfv_verifiable_encrypt_number" including the original error and the raw string; also import SDKError from ../utils and return { encryptedData, circuitInputs: parsed } on success.
87-94: Same JSON.parse error handling needed for vector encryption.Apply the same defensive error handling for
JSON.parse(circuitInputs)inencryptVectorAndGenInputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/crypto/encrypt.ts` around lines 87 - 94, The return currently does JSON.parse(circuitInputs) directly in encryptVectorAndGenInputs after calling bfv_verifiable_encrypt_vector which can throw on invalid JSON; wrap the JSON.parse(circuitInputs) in a try/catch, and on error throw a new descriptive Error that includes context (e.g., "failed to parse circuitInputs from bfv_verifiable_encrypt_vector") and the original error message or circuitInputs value for debugging, preserving the existing return shape { encryptedData, circuitInputs: ... } when parsing succeeds.
20-29: Consider caching WASM initialization or BFV params.
resolveParamscallsinitializeWasm()on every invocation. While WASM initialization is typically idempotent, repeatedly calling it may have minor overhead. If performance is a concern for high-frequency encryption operations, consider caching the initialization state or the resolved params per preset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-sdk/src/crypto/encrypt.ts` around lines 20 - 29, The resolveParams function repeatedly calls initializeWasm and recomputes params on every invocation; cache the WASM initialization state and/or cache resolved BFV params per preset to avoid repeated work. Modify resolveParams (and/or module scope) to ensure initializeWasm is awaited only once (e.g., an initialized Promise or boolean guard) and store the resulting BfvParams in a Map keyed by presetName so get_bfv_params and the object construction run only on first access; subsequent calls should return the cached BfvParams for the given presetName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/pages/sdk.mdx`:
- Around line 164-173: Update the docs example to use the correct submodule name
`crypto` instead of `encryption`: change the import line that currently reads
`import { generatePublicKey, encryptNumber } from '@enclave-e3/sdk/encryption'`
to `import { generatePublicKey, encryptNumber } from '@enclave-e3/sdk/crypto' so
the snippet with `generatePublicKey`, `encryptNumber`, `ContractClient`, and
`EventListener` matches the actual exposed module names.
In `@packages/enclave-sdk/README.md`:
- Around line 306-326: Add the three missing read-operation docs to the README
API Reference under the "Read operations" section: document getE3Quote(params:
E3RequestParams): Promise<bigint> (explaining it returns the fee quote to know
what to approve), getE3Stage(e3Id: bigint): Promise<E3Stage> (explaining it
returns the lifecycle status of an E3), and getFailureReason(e3Id: bigint):
Promise<FailureReason> (explaining it returns failure details for a failed E3);
include short parameter and return descriptions and example usage consistent
with the surrounding read-operation entries so users can find and use these SDK
methods.
- Around line 132-166: The templates reference a missing event enum:
ActivateE3.tsx uses EnclaveEventType.E3_ACTIVATED (while the SDK defines
E3ActivatedData but no E3_ACTIVATED value), and
templates/default/server/index.ts uses a local ProgramEventType.INPUT_PUBLISHED
instead of SDK enums; fix by either adding E3_ACTIVATED to the SDK
EnclaveEventType enum (so E3ActivatedData has a matching enum value) or update
ActivateE3.tsx to reference an existing SDK event (e.g.,
EnclaveEventType.E3_REQUESTED or the appropriate published output event) and
update templates/default/server/index.ts to use the corresponding SDK enum
instead of ProgramEventType.INPUT_PUBLISHED so all event references match the
SDK definitions.
In `@packages/enclave-sdk/src/contracts/contract-client.ts`:
- Around line 216-236: The getE3Quote call uses E3RequestParams fields with
incorrect typing and a fallback that masks a required Solidity field; update the
E3RequestParams type so e3Program is an address/contract reference (use Address
or the IE3Program address type used across the codebase) instead of
`0x${string}`, make customParams required (remove the optional flag), and stop
defaulting to '0x' in getE3Quote — pass requestParams.customParams directly;
also add a small validation in getE3Quote to ensure e3Program is a valid address
before calling readContract.
In `@packages/enclave-sdk/src/enclave-sdk.ts`:
- Around line 69-71: The check uses
Object.values(ThresholdBfvParamsPresetNames).includes(...) but
ThresholdBfvParamsPresetNames is already an array; replace the
Object.values(...) call and use
ThresholdBfvParamsPresetNames.includes(presetName) to validate presetName,
keeping the same SDKError throw (class SDKError and message
'INVALID_THRESHOLD_BFV_PARAMS_PRESET_NAME') when the include check fails.
---
Outside diff comments:
In `@packages/enclave-sdk/README.md`:
- Around line 44-74: Add a "Migration Guide" section to README describing
breaking changes and how to update code: explain that EnclaveSDK now accepts
viem Chain objects via the chain option (replace chainId), that
contracts.enclave + contracts.ciphernodeRegistry must now include
contracts.feeToken, that thresholdBfvParamsPresetName is required, and that
requestE3's duration parameter was renamed to inputWindow; include before/after
snippets showing EnclaveSDK construction (reference EnclaveSDK, chain, feeToken,
thresholdBfvParamsPresetName) and requestE3 usage (reference requestE3 and
inputWindow), and list removed/renamed events (mention
EnclaveEventType.E3_ACTIVATED → EnclaveEventType.E3_REQUESTED and any
INPUT_PUBLISHED replacement) — place this section adjacent to the Quick Start
for discoverability.
---
Duplicate comments:
In `@docs/pages/sdk.mdx`:
- Around line 85-97: Update the example so inputWindow is a buffered future
window and show the full flow: build a requestParams object (containing
threshold, inputWindow, e3Program, e3ProgramParams, computeProviderParams,
customParams), call sdk.getE3Quote(requestParams) and then
sdk.approveFeeToken(quote) to approve fees, and only after approval call
sdk.requestE3(requestParams); reference the sdk.requestE3, sdk.getE3Quote,
sdk.approveFeeToken and requestParams symbols and ensure inputWindow uses
Date.now() plus a buffer (e.g., +60s or more) so the request is valid when
executed.
In `@templates/default/server/index.ts`:
- Around line 36-45: The template currently hard-codes the chain as the literal
hardhat in the EnclaveSDK.create call; change the chain property to read the
chain/network from your deployment configuration (or derive it from the RPC_URL)
instead of the string 'hardhat' so non-local RPC_URLs use the correct chain
metadata—update the chain field in EnclaveSDK.create (referenced as the chain
property in EnclaveSDK.create) to use the deployment config value (e.g.,
DEPLOYMENT.chain or a getChainFromRpc(RPC_URL) helper) rather than the hardcoded
hardhat.
In `@templates/default/tests/integration.spec.ts`:
- Around line 184-190: Replace the fixed 1s sleep after calling
sdk.approveFeeToken(quote) with a proper wait for the approval transaction to be
mined: capture the returned hash from approveFeeToken (variable hash) and call
sdk.waitForTransaction(hash) (same sdk instance used elsewhere) instead of
setTimeout so the test only proceeds once the allowance update is confirmed.
- Around line 192-200: The test incorrectly assumes the new request ID is 0n;
instead capture the E3_REQUESTED event returned by waitForEvent and use its
runtime e3Id for assertions. Change the waitForEvent call to assign its result
(from waitForEvent(EnclaveEventType.E3_REQUESTED, ...)), extract the e3Id from
that event, then call store.get(e3Id) and assert that state.e3Id === e3Id and
state.type === 'requested' (references: waitForEvent,
EnclaveEventType.E3_REQUESTED, sdk.requestE3, store.get, state.e3Id).
---
Nitpick comments:
In `@packages/enclave-sdk/src/crypto/encrypt.ts`:
- Around line 61-68: Wrap the JSON.parse call for circuitInputs (from
bfv_verifiable_encrypt_number) in a try-catch and convert parse failures into a
descriptive SDKError: capture the raw circuitInputs string, attempt JSON.parse,
and on catch throw new SDKError with a message like "Failed to parse
circuitInputs from bfv_verifiable_encrypt_number" including the original error
and the raw string; also import SDKError from ../utils and return {
encryptedData, circuitInputs: parsed } on success.
- Around line 87-94: The return currently does JSON.parse(circuitInputs)
directly in encryptVectorAndGenInputs after calling
bfv_verifiable_encrypt_vector which can throw on invalid JSON; wrap the
JSON.parse(circuitInputs) in a try/catch, and on error throw a new descriptive
Error that includes context (e.g., "failed to parse circuitInputs from
bfv_verifiable_encrypt_vector") and the original error message or circuitInputs
value for debugging, preserving the existing return shape { encryptedData,
circuitInputs: ... } when parsing succeeds.
- Around line 20-29: The resolveParams function repeatedly calls initializeWasm
and recomputes params on every invocation; cache the WASM initialization state
and/or cache resolved BFV params per preset to avoid repeated work. Modify
resolveParams (and/or module scope) to ensure initializeWasm is awaited only
once (e.g., an initialized Promise or boolean guard) and store the resulting
BfvParams in a Map keyed by presetName so get_bfv_params and the object
construction run only on first access; subsequent calls should return the cached
BfvParams for the given presetName.
In `@packages/enclave-sdk/src/events/event-listener.ts`:
- Around line 46-63: The current resolveContract logic uses REGISTRY_ONLY_EVENTS
and will always route ambiguous names like "OwnershipTransferred" and
"Initialized" to the Enclave (see REGISTRY_ONLY_EVENTS, resolveContract,
AllEventTypes, contracts.ciphernodeRegistry, contracts.enclave,
CiphernodeRegistryOwnable__factory, Enclave__factory); update the API to allow
callers to explicitly target a contract for ambiguous event names (e.g., add an
optional parameter like targetContract or forceRegistry to the public
watch/subscribe method that resolveContract reads) and/or add clear public docs
stating that shared event names default to the Enclave and how to opt into the
registry-specific behavior so callers can watch those events on
CiphernodeRegistry when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9becb913-5393-47e0-8104-f6649bae0fc8
📒 Files selected for processing (26)
docs/pages/sdk.mdxexamples/CRISP/packages/crisp-sdk/tests/vote.test.tspackages/enclave-react/src/useEnclaveSDK.tspackages/enclave-sdk/README.mdpackages/enclave-sdk/package.jsonpackages/enclave-sdk/src/constants.tspackages/enclave-sdk/src/contract-client.tspackages/enclave-sdk/src/contracts/contract-client.tspackages/enclave-sdk/src/contracts/index.tspackages/enclave-sdk/src/contracts/types.tspackages/enclave-sdk/src/crypto/encrypt.tspackages/enclave-sdk/src/crypto/index.tspackages/enclave-sdk/src/crypto/types.tspackages/enclave-sdk/src/crypto/user-data-encryption.tspackages/enclave-sdk/src/enclave-sdk.tspackages/enclave-sdk/src/events/event-listener.tspackages/enclave-sdk/src/events/index.tspackages/enclave-sdk/src/events/types.tspackages/enclave-sdk/src/index.tspackages/enclave-sdk/src/types.tspackages/enclave-sdk/tests/sdk.test.tspackages/enclave-sdk/tsup.config.jsscripts/build-circuits.tstemplates/default/server/index.tstemplates/default/server/types.tstemplates/default/tests/integration.spec.ts
💤 Files with no reviewable changes (1)
- packages/enclave-sdk/src/contract-client.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/enclave-sdk/src/contracts/index.ts
- packages/enclave-sdk/src/events/types.ts
- packages/enclave-sdk/src/contracts/types.ts
- packages/enclave-sdk/src/events/index.ts
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
- packages/enclave-react/src/useEnclaveSDK.ts
- packages/enclave-sdk/tests/sdk.test.ts
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.
fix #918 fix #1002 fix #1139
Summary by CodeRabbit
Release Notes
New Features
offcallback to React hook for event cleanupENCRYPTION_SCHEME_ENABLED,ENCRYPTION_SCHEME_DISABLED,OWNERSHIP_TRANSFERRED,INITIALIZEDBreaking Changes
chainIdwithchainparameter in configurationrequestE3to useinputWindowinstead ofstartWindow/durationfeeTokento contracts configurationE3_ACTIVATEDtoE3_REQUESTED