Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces OPRF-raw mode (server-side OPRF computation), integrates the Stwo ZK engine, migrates to Ethers v6 APIs throughout the codebase, upgrades ESLint to v9, and updates proto definitions and generated contract types to support these features across client, server, and browser bundling layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DB as OPRF Operator
participant ZK as ZK Engine (Stwo)
Note over Client,ZK: OPRF-Raw Claim Creation & Server Processing
Client->>Client: getBlocksToReveal()
Note right of Client: Mark slices with hash: 'oprf-raw'
Client->>Client: Track oprfRawMarkers & cross-block overshooting
Client->>Server: createClaimOnAttestor(claim, zkEngine: 'stwo')
Server->>Server: decryptTranscript()
activate Server
Server->>Server: Process packets with oprf-raw markers
Note right of Server: Accumulate plaintext spans across blocks
Server->>DB: computeOPRFRaw(plaintext, markers)
activate DB
DB->>DB: For each marker: validateBounds, slice plaintext
DB->>DB: generateOPRFRequestData + evaluateOPRF + finaliseOPRF
DB-->>Server: OPRFRawResult[] (with nullifiers)
deactivate DB
Server->>Server: Apply string replacements to info.parameters
Note right of Server: Store originalText → nullifierText mappings
deactivate Server
Server->>ZK: verifyZkPacket(proofs, oprfRawMarkers)
Note right of ZK: Verify Stwo proofs alongside markers
ZK-->>Server: { redactedPlaintext, oprfRawMarkers }
Server->>Server: assertValidProviderTranscript()
Note right of Server: Apply stored replacements to canonicalized JSON params
Server-->>Client: ClaimCreated + OprfRawReplacements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/tests/zk.test.ts (1)
275-354: Test doesn't actually verify the oprf-raw server-side path.The "OPRF-Raw Nullifier Consistency" test runs the same client-side OPRF flow twice (both using
generateOPRFRequestData→toprf→finaliseOPRF) and compares results. While this validates OPRF determinism, it doesn't test the actualcomputeOPRFRawfunction fromsrc/server/utils/oprf-raw.tsthat the server uses in oprf-raw mode.Consider adding a test that imports and calls
computeOPRFRawdirectly to verify end-to-end consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/zk.test.ts` around lines 275 - 354, The test currently duplicates the client-side flow instead of exercising the server-side helper; update the test to import and call computeOPRFRaw from oprf-raw.ts and compare its output to the existing ZK OPRF finaliser results: generate a request with oprfOperator.generateOPRFRequestData(testData, TOPRF_DOMAIN_SEPARATOR, logger), call toprf(...) to get the server evaluation, then call computeOPRFRaw(...) with the same maskedData (and any required server key/context) and ensure the nullifier produced by computeOPRFRaw matches oprfOperator.finaliseOPRF(...) result (and also assert determinism by repeating); use the existing symbols computeOPRFRaw, generateOPRFRequestData, finaliseOPRF, toprf, ZKProofEngine.ZK_ENGINE_GNARK and TOPRF_DOMAIN_SEPARATOR to locate code and wire inputs/outputs accordingly.src/scripts/build-browser.sh (1)
8-14: Consider making stwo resource checks fail the build for production.The script warns but doesn't fail when
s2circuits.jsors2circuits_bg.wasmare missing. If stwo is used at runtime without these files, users will encounter cryptic errors.If stwo is a required feature in this PR, consider making this a build failure. If it's experimental/optional, the current warning approach is acceptable but you may want to add a comment clarifying the expected behavior.
♻️ Option: Fail build if stwo resources are missing
# ensure stwo resources exist (s2circuits.js + s2circuits_bg.wasm) +STWO_MISSING=0 if [ ! -f ./browser/resources/stwo/s2circuits.js ]; then - echo "Warning: stwo/s2circuits.js not found in resources" + echo "Error: stwo/s2circuits.js not found in resources" + STWO_MISSING=1 fi if [ ! -f ./browser/resources/stwo/s2circuits_bg.wasm ]; then - echo "Warning: stwo/s2circuits_bg.wasm not found in resources" + echo "Error: stwo/s2circuits_bg.wasm not found in resources" + STWO_MISSING=1 fi +if [ $STWO_MISSING -eq 1 ]; then + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/build-browser.sh` around lines 8 - 14, The current build-browser.sh only prints warnings when ./browser/resources/stwo/s2circuits.js or ./browser/resources/stwo/s2circuits_bg.wasm are missing; change it to fail the build in production by checking an environment flag (e.g., PRODUCTION or REQUIRE_STWO) and calling exit 1 if either file is absent, or alternatively add a clear comment documenting that the checks are intentionally non-fatal for optional/experimental builds; update the s2circuits.js and s2circuits_bg.wasm checks to branch on that flag and exit with a non-zero status when required.src/utils/zk.ts (1)
710-718: Consider documenting stwo's OPRF limitation or clarifying the error message.The
operatorMakersmap includesstwofor symmetric cipher proofs, butOPRF_OPERATOR_MAKERScorrectly excludes it—stwo does not support OPRF/TOPRF operations. Only gnark implements Reclaim's threshold vOPRF circuits. While the current code structure is correct, if a user selectsstwoand attempts TOPRF operations,makeDefaultOPRFOperatorwill throw an unclear error. Either document this limitation in comments or provide a more descriptive error message (e.g., "OPRF operations are only supported with the 'gnark' engine").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/zk.ts` around lines 710 - 718, The code allows selecting the 'stwo' engine in operatorMakers but OPRF_OPERATOR_MAKERS rightly omits it, which leads makeDefaultOPRFOperator to throw an unclear error if a user requests OPRF/TOPRF with 'stwo'; update makeDefaultOPRFOperator to detect when the selected engine (from operatorMakers or the engine parameter) is 'stwo' and throw a clear, descriptive error like "OPRF operations are only supported with the 'gnark' engine", and/or add a brief comment above operatorMakers/OPRF_OPERATOR_MAKERS documenting that stwo does not support OPRF/TOPRF and only gnark implements Reclaim's threshold vOPRF circuits so maintainers and users see the limitation immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/create-claim.ts`:
- Around line 515-524: The loop over serverPacketsToReveal builds the ZK reveal
payload but omits overshotOprfRawFromPrevBlock; update the destructuring to also
pull overshotOprfRawFromPrevBlock from each server packet and pass it into
setRevealOfMessage so the ZKRevealInfo includes that field. Locate the loop that
calls setRevealOfMessage (the for...of over serverPacketsToReveal) and add
overshotOprfRawFromPrevBlock to the destructured variables and to the object
literal passed to setRevealOfMessage, consistent with the shape produced by
src/utils/redactions.ts and the ZKRevealInfo type.
In `@src/scripts/fallbacks/stwo.ts`:
- Around line 186-193: The code indexes publicSignals.noncesAndCounters[0]
before optional chaining which can throw if noncesAndCounters is undefined;
update the assignments for expectedNonce and expectedCounter to use optional
chaining with array access (publicSignals.noncesAndCounters?.[0]?.nonce and
?.counter) and adjust the subsequent guard (the if that checks
expectedNonce/expectedCounter) to rely on these safe values; also consider using
optional chaining for expectedCiphertext/expectedPlaintext (publicSignals?.in,
publicSignals?.out) to avoid similar TypeErrors and ensure logger?.warn path
executes instead of throwing.
In `@src/scripts/generate-receipt.ts`:
- Line 60: The code currently maps any --zk value not exactly 'gnark' to 'stwo',
silently converting valid 'snarkjs' or mistyped values; change the logic around
getCliArgument('zk') and the zkEngine assignment to explicitly accept only
'gnark' or 'snarkjs' (e.g., if val === 'gnark' || val === 'snarkjs' then
zkEngine = val) and otherwise report an error and exit (or throw) so invalid
values are rejected; update any downstream uses of zkEngine ('zkEngine'
variable) to expect either 'gnark' or 'snarkjs'.
In `@src/server/utils/assert-valid-claim-request.ts`:
- Around line 247-255: The current pendingOprfRaw map stores a single object per
nextPktIdx so later spillover markers overwrite earlier ones; change
pendingOprfRaw to map each nextPktIdx to an array of marker objects (same shape)
and update separateOprfRawMarkers() and any code that reads pendingOprfRaw to
push new entries instead of replacing, then when processing a packet drain and
iterate that array to reconstruct/apply all nullifiers (not just the first
pendingMarker); make equivalent fixes for the other occurrences you noted
(around the separateOprfRawMarkers usage and packet processing logic).
- Around line 356-448: Packets decrypted via directReveal are skipping the
oprf-raw handling because the current logic runs only after verifyZkPacket; move
the oprf-raw/overshoot processing out of the verifyZkPacket-only path by
extracting the block that handles pendingOprfRaw, separateOprfRawMarkers,
computeOPRFRaw, and the plaintext replacements into a helper (e.g.
processOprfRawForPacket) and call it for every decrypted packet regardless of
verifyZkPacket/directReveal/zkReveal mode, and ensure it also runs for zkReveal
packets that have markers but no proofs by using zkReveal?.overshotOprfRawLength
and result.oprfRawMarkers checks inside that helper; reference symbols:
verifyZkPacket, directReveal, zkReveal, oprfRawMarkers, pendingOprfRaw,
separateOprfRawMarkers, computeOPRFRaw.
- Around line 140-154: The current replaceAll over canonicalStringify(params) is
unsafe; instead parse params into an object, and for each entry in
oprfRawReplacements locate the exact structural location (add/expect a location
field such as a JSON Pointer or dot-path on OPRFRawReplacement) and replace only
that node's value with nullifierText, then re-stringify via canonicalStringify
and assign to params and info.parameters; keep logger.debug({ replacements:
oprfRawReplacements.length }, 'applied oprf-raw parameter replacements')
unchanged. Ensure you replace the string-based replaceAll usage around
canonicalStringify(params) with a traversal/resolution step that updates the
parsed object and only then JSON.stringify/canonicalStringify the modified
object.
In `@src/server/utils/oprf-raw.ts`:
- Around line 97-100: The log currently emits a truncated OPRF-derived
identifier via nullifierHex in the logger.debug call in oprf-raw.ts; remove any
logging of nullifierHex (or any derivative of the nullifier) and instead log
only non-identifying metadata (e.g., fromIndex, length, and a boolean or fixed
string like "nullifier_redacted": true) in the logger.debug call to avoid
creating stable join keys; update the logger.debug invocation in the same
location (the logger.debug that currently references nullifierHex and the
message 'computed oprf-raw nullifier') to omit
Buffer.from(nullifier).toString('hex') and replace it with a redacted indicator.
---
Nitpick comments:
In `@src/scripts/build-browser.sh`:
- Around line 8-14: The current build-browser.sh only prints warnings when
./browser/resources/stwo/s2circuits.js or
./browser/resources/stwo/s2circuits_bg.wasm are missing; change it to fail the
build in production by checking an environment flag (e.g., PRODUCTION or
REQUIRE_STWO) and calling exit 1 if either file is absent, or alternatively add
a clear comment documenting that the checks are intentionally non-fatal for
optional/experimental builds; update the s2circuits.js and s2circuits_bg.wasm
checks to branch on that flag and exit with a non-zero status when required.
In `@src/tests/zk.test.ts`:
- Around line 275-354: The test currently duplicates the client-side flow
instead of exercising the server-side helper; update the test to import and call
computeOPRFRaw from oprf-raw.ts and compare its output to the existing ZK OPRF
finaliser results: generate a request with
oprfOperator.generateOPRFRequestData(testData, TOPRF_DOMAIN_SEPARATOR, logger),
call toprf(...) to get the server evaluation, then call computeOPRFRaw(...) with
the same maskedData (and any required server key/context) and ensure the
nullifier produced by computeOPRFRaw matches oprfOperator.finaliseOPRF(...)
result (and also assert determinism by repeating); use the existing symbols
computeOPRFRaw, generateOPRFRequestData, finaliseOPRF, toprf,
ZKProofEngine.ZK_ENGINE_GNARK and TOPRF_DOMAIN_SEPARATOR to locate code and wire
inputs/outputs accordingly.
In `@src/utils/zk.ts`:
- Around line 710-718: The code allows selecting the 'stwo' engine in
operatorMakers but OPRF_OPERATOR_MAKERS rightly omits it, which leads
makeDefaultOPRFOperator to throw an unclear error if a user requests OPRF/TOPRF
with 'stwo'; update makeDefaultOPRFOperator to detect when the selected engine
(from operatorMakers or the engine parameter) is 'stwo' and throw a clear,
descriptive error like "OPRF operations are only supported with the 'gnark'
engine", and/or add a brief comment above operatorMakers/OPRF_OPERATOR_MAKERS
documenting that stwo does not support OPRF/TOPRF and only gnark implements
Reclaim's threshold vOPRF circuits so maintainers and users see the limitation
immediately.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b36b8dc9-2923-4089-8cdc-90aa086fa192
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
browser/index.htmlpackage.jsonproto/api.protoprovider-schemas/http/parameters.yamlsrc/client/create-claim.tssrc/proto/api.tssrc/scripts/build-browser.shsrc/scripts/build-browser.tssrc/scripts/build-jsc.tssrc/scripts/fallbacks/stwo.tssrc/scripts/generate-receipt.tssrc/server/utils/assert-valid-claim-request.tssrc/server/utils/oprf-raw.tssrc/tests/zk.test.tssrc/types/general.tssrc/types/providers.gen.tssrc/types/zk.tssrc/utils/prepare-packets.tssrc/utils/redactions.tssrc/utils/zk.ts
adiwajshing
approved these changes
Mar 31, 2026
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/server/utils/assert-valid-claim-request.ts (3)
356-448:⚠️ Potential issue | 🟠 Major
oprf-rawrewriting still only runs inside the proof-backed ZK branch.Because this block only executes after
verifyZkPacket(), adirectRevealpacket—or azkRevealthat only carriesoprfRawMarkers/overshotOprfRawLength—still skips the replacement logic entirely. That leaves raw plaintext unreplaced on those paths and can strand pending spillovers until the final incomplete-marker error. Please extract this into a helper and run it after any decryption path onceplaintextis available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/assert-valid-claim-request.ts` around lines 356 - 448, The oprf-raw replacement logic (the block using separateOprfRawMarkers, computeOPRFRaw, pendingOprfRaw, oprfRawReplacements and updating decryptedTranscript/plaintext) is only executed inside the zk-proof branch; extract that whole behavior into a helper (e.g., processOprfRawMarkers(plaintext, i, sender, transcript, decryptedTranscript, pendingOprfRaw, oprfRawReplacements, logger)) that performs the cross-block pending handling, marker separation, computeOPRFRaw calls, and in-place plaintext/prev-packet updates, then invoke this helper immediately after any decryption path once plaintext is available (both directReveal and zkReveal flows) so replacements and pending spillovers are handled uniformly. Ensure the helper uses existing functions computeOPRFRaw and separateOprfRawMarkers and updates pendingOprfRaw and decryptedTranscript exactly as the current inline code does.
140-149:⚠️ Potential issue | 🟠 MajorStill location-unsafe:
replaceAll()can rewrite unrelated params.This loop rewrites every matching substring in the canonical JSON. If the same value appears twice, or also appears in another field/key/escaped form, provider validation can run against params that no longer match the bytes actually nullified from the transcript. Please carry a structural location through the
oprf-rawflow and update only that node before re-stringifying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/assert-valid-claim-request.ts` around lines 140 - 149, The current loop uses replaceAll on the canonical JSON string which is location-unsafe; instead parse and mutate the params object at the exact node(s) indicated by the oprfRawReplacements, then re-stringify. Update oprfRawReplacements (or expect it) to carry a structural locator (e.g., JSON Pointer / path) alongside originalText/nullifierText, walk into the parsed params object and replace only that node's value, then call canonicalStringify(params) and assign it to info.parameters; keep references to canonicalStringify, params, oprfRawReplacements and info.parameters when implementing this change.
249-255:⚠️ Potential issue | 🟠 MajorStill only one spillover
oprf-rawmarker survives per next packet.
pendingOprfRaw[nextPktIdx]stores a single object andseparateOprfRawMarkers()returns only onependingMarker. If two markers spill into the same next packet, the later one overwrites the earlier one, so only one nullifier gets reconstructed/applied. This needs to buffer an array pernextPktIdxand drain all of them when that packet is processed.Also applies to: 417-424, 481-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/utils/assert-valid-claim-request.ts` around lines 249 - 255, pendingOprfRaw currently maps nextPktIdx to a single object so later spillover markers overwrite earlier ones; change pendingOprfRaw to map each nextPktIdx to an array of marker objects (e.g., Array<{partialData: Uint8Array, dataLocation: {...}, originPktIdx:number}>), update the logic in separateOprfRawMarkers() to push markers into that array instead of assigning a single pendingMarker and return all buffered markers for a nextPktIdx, and update any code that consumes pendingOprfRaw/pendingMarker so it iterates and drains the array (apply all reconstructed nullifiers) rather than processing only one entry. Ensure similar fixes are applied to the other occurrences referenced (the other separateOprfRawMarkers/pendingOprfRaw usages).
🧹 Nitpick comments (1)
src/avs/client/create-claim-on-avs.ts (1)
169-175: Consider adding validation for empty event logs.If
taskCompletedsucceeds but noEventLogentries are found (edge case),evwould beundefinedandev?.argswould returnundefined, which gets cast toTaskCompletedEvent.OutputObject. This could mask issues where the expected event wasn't emitted.The pattern is consistent with similar code in
src/server/handlers/completeClaimOnChain.ts, so this may be intentional, but a defensive check could prevent silent failures.🛡️ Optional defensive check
const logs = rslt?.logs ?? [] const eventLogs = logs.filter((log): log is EventLog => log instanceof EventLog) const ev = eventLogs[0] +if(!ev) { + throw new Error('INTERNAL: TaskCompleted event not emitted') +} return { - object: ev?.args as unknown as TaskCompletedEvent.OutputObject, + object: ev.args as unknown as TaskCompletedEvent.OutputObject, txHash: rslt!.hash }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/avs/client/create-claim-on-avs.ts` around lines 169 - 175, The code currently assumes an EventLog exists after calling taskCompleted; add a defensive check after computing eventLogs (from rslt?.logs) to handle the empty case: if eventLogs.length === 0, throw or return a clear error (e.g., throw new Error("taskCompleted returned no EventLog for TaskCompletedEvent")) instead of casting undefined to TaskCompletedEvent.OutputObject; update the return to only cast ev.args when ev is present (use ev!.args after the guard) and reference variables rslt, logs, EventLog, eventLogs, ev, and TaskCompletedEvent.OutputObject when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/avs/utils/tasks.ts`:
- Around line 40-44: The code currently assumes eventLogs[0] is a
NewTaskCreatedEvent; instead, locate the correct NewTaskCreatedEvent by
filtering/parsing eventLogs rather than taking the first entry: iterate
eventLogs (from rslt?.logs), identify entries whose topic/name matches
NewTaskCreatedEvent (or use the contract parser for NewTaskCreatedEvent), pick
that matched EventLog (not eventLogs[0]) and then cast its args to
NewTaskCreatedEvent.OutputObject (update references to ev and arg accordingly).
Ensure you use the contract event identifier (NewTaskCreatedEvent) when
selecting/parsing so other emitted events (Initialized, OwnershipTransferred,
Paused, etc.) are ignored.
In `@src/server/handlers/completeClaimOnChain.ts`:
- Around line 29-31: The code in completeClaimOnChain.ts assumes eventLogs[0] is
a TaskCompletedEvent and casts its args to TaskCompletedEvent.OutputObject;
instead filter or find the correct EventLog by its event name (e.g., match
against "TaskCompleted" or TaskCompletedEvent.name) before casting, then extract
args from that matched event; update the handling around the symbols eventLogs,
EventLog, TaskCompletedEvent and obj to use the foundEvent (or return/throw if
not found) to avoid invalid casts.
In `@src/server/handlers/createClaimOnChain.ts`:
- Around line 35-36: The code converts task.taskIndex (a bigint from
NewTaskCreatedEvent.OutputObject) using Number(), risking precision loss;
instead preserve it as a safe representation: replace Number(task.taskIndex)
with a non-lossy form such as task.taskIndex.toString() (or keep it as a bigint
if response typing allows) and update the surrounding response/DTO typing in
createClaimOnChain.ts (and any callers of the object containing txHash and
taskIndex) to accept a string or bigint accordingly so large indexes are not
truncated.
In `@src/tests/zk.test.ts`:
- Around line 310-328: The test currently re-runs the client masked flow
(generateOPRFRequestData -> toprf -> finaliseOPRF) instead of invoking the
server-side flow; import and call computeOPRFRaw and use its return as
oprfRawNullifier in place of the repeated
generateOPRFRequestData/toprf/finaliseOPRF sequence in the "Compute via oprf-raw
flow" section (i.e., replace the block that builds
oprfRawRequest/oprfRawResponse/oprfRawNullifier with a single call to
computeOPRFRaw(testData, TOPRF_DOMAIN_SEPARATOR, logger) or the function's
required args), and remove the duplicated steps so the test actually exercises
computeOPRFRaw.
In `@src/utils/socket-base.ts`:
- Around line 31-35: The error handler on socket.addEventListener currently
types the callback as ErrorEvent and constructs an Error from event.message
which is undefined in browser Event objects; change the handler to accept a
permissive type (e.g., Event | ErrorEvent | any), then normalize the payload by
checking event.error first, then event.message, then a sensible fallback string
like 'Unknown network error' and pass a constructed Error or the existing error
to AttestorError.fromError; apply the same normalization logic to the other
handler referenced (lines around the second addEventListener usage) to ensure
both Node.js ws and browser WebSocket produce meaningful error messages.
---
Duplicate comments:
In `@src/server/utils/assert-valid-claim-request.ts`:
- Around line 356-448: The oprf-raw replacement logic (the block using
separateOprfRawMarkers, computeOPRFRaw, pendingOprfRaw, oprfRawReplacements and
updating decryptedTranscript/plaintext) is only executed inside the zk-proof
branch; extract that whole behavior into a helper (e.g.,
processOprfRawMarkers(plaintext, i, sender, transcript, decryptedTranscript,
pendingOprfRaw, oprfRawReplacements, logger)) that performs the cross-block
pending handling, marker separation, computeOPRFRaw calls, and in-place
plaintext/prev-packet updates, then invoke this helper immediately after any
decryption path once plaintext is available (both directReveal and zkReveal
flows) so replacements and pending spillovers are handled uniformly. Ensure the
helper uses existing functions computeOPRFRaw and separateOprfRawMarkers and
updates pendingOprfRaw and decryptedTranscript exactly as the current inline
code does.
- Around line 140-149: The current loop uses replaceAll on the canonical JSON
string which is location-unsafe; instead parse and mutate the params object at
the exact node(s) indicated by the oprfRawReplacements, then re-stringify.
Update oprfRawReplacements (or expect it) to carry a structural locator (e.g.,
JSON Pointer / path) alongside originalText/nullifierText, walk into the parsed
params object and replace only that node's value, then call
canonicalStringify(params) and assign it to info.parameters; keep references to
canonicalStringify, params, oprfRawReplacements and info.parameters when
implementing this change.
- Around line 249-255: pendingOprfRaw currently maps nextPktIdx to a single
object so later spillover markers overwrite earlier ones; change pendingOprfRaw
to map each nextPktIdx to an array of marker objects (e.g., Array<{partialData:
Uint8Array, dataLocation: {...}, originPktIdx:number}>), update the logic in
separateOprfRawMarkers() to push markers into that array instead of assigning a
single pendingMarker and return all buffered markers for a nextPktIdx, and
update any code that consumes pendingOprfRaw/pendingMarker so it iterates and
drains the array (apply all reconstructed nullifiers) rather than processing
only one entry. Ensure similar fixes are applied to the other occurrences
referenced (the other separateOprfRawMarkers/pendingOprfRaw usages).
---
Nitpick comments:
In `@src/avs/client/create-claim-on-avs.ts`:
- Around line 169-175: The code currently assumes an EventLog exists after
calling taskCompleted; add a defensive check after computing eventLogs (from
rslt?.logs) to handle the empty case: if eventLogs.length === 0, throw or return
a clear error (e.g., throw new Error("taskCompleted returned no EventLog for
TaskCompletedEvent")) instead of casting undefined to
TaskCompletedEvent.OutputObject; update the return to only cast ev.args when ev
is present (use ev!.args after the guard) and reference variables rslt, logs,
EventLog, eventLogs, ev, and TaskCompletedEvent.OutputObject when making the
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e817296-7018-4b88-b116-1a46fa9d8688
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.eslintignore.eslintrc.yamleslint.config.jspackage.jsonsrc/avs/client/create-claim-on-avs.tssrc/avs/contracts/ReclaimServiceManager.tssrc/avs/contracts/common.tssrc/avs/contracts/factories/ReclaimServiceManager__factory.tssrc/avs/tests/test.operator.tssrc/avs/types/index.tssrc/avs/utils/contracts.tssrc/avs/utils/register.tssrc/avs/utils/tasks.tssrc/client/create-claim.tssrc/client/utils/client-socket.tssrc/external-rpc/handle-incoming-msg.tssrc/external-rpc/types.tssrc/providers/http/index.tssrc/providers/http/patch-parse5-tree.tssrc/providers/http/utils.tssrc/scripts/generate-receipt.tssrc/scripts/generate-toprf-keys.tssrc/scripts/jsc-cli-rpc.tssrc/scripts/start-server.tssrc/server/handlers/completeClaimOnChain.tssrc/server/handlers/createClaimOnChain.tssrc/server/handlers/createTaskOnMechain.tssrc/server/handlers/init.tssrc/server/handlers/toprf.tssrc/server/utils/assert-valid-claim-request.tssrc/server/utils/nitro-attestation.tssrc/server/utils/oprf-raw.tssrc/tests/auth.test.tssrc/tests/claim-creation.test.tssrc/tests/describe-with-server.tssrc/tests/gcp-attestation.test.tssrc/tests/http-provider-utils.test.tssrc/tests/jsc.test_mac.tssrc/tests/mock-provider-server.tssrc/tests/mocks.tssrc/tests/rpc-tunnel.test.tssrc/tests/signatures.test.tssrc/tests/tee-bundle.test.tssrc/tests/utils.tssrc/tests/zk.test.tssrc/types/rpc.tssrc/utils/auth.tssrc/utils/b64-json.tssrc/utils/claims.tssrc/utils/redactions.tssrc/utils/signatures/eth.tssrc/utils/socket-base.ts
💤 Files with no reviewable changes (2)
- .eslintignore
- .eslintrc.yaml
✅ Files skipped from review due to trivial changes (24)
- src/providers/http/patch-parse5-tree.ts
- src/external-rpc/handle-incoming-msg.ts
- src/providers/http/utils.ts
- src/server/utils/nitro-attestation.ts
- src/tests/rpc-tunnel.test.ts
- src/tests/describe-with-server.ts
- src/tests/mock-provider-server.ts
- src/tests/utils.ts
- src/types/rpc.ts
- src/scripts/jsc-cli-rpc.ts
- src/avs/tests/test.operator.ts
- src/utils/b64-json.ts
- src/scripts/generate-toprf-keys.ts
- src/tests/gcp-attestation.test.ts
- src/tests/mocks.ts
- src/client/utils/client-socket.ts
- src/tests/tee-bundle.test.ts
- src/scripts/start-server.ts
- src/tests/auth.test.ts
- src/avs/utils/contracts.ts
- src/tests/claim-creation.test.ts
- src/utils/claims.ts
- src/tests/jsc.test_mac.ts
- src/tests/http-provider-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/scripts/generate-receipt.ts
- src/client/create-claim.ts
- package.json
- src/utils/redactions.ts
- src/server/utils/oprf-raw.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
package.json (1)
24-24: Use a cross-platform clean step inbuild.Line 24 uses
rm -rf, which is not portable across environments. Prefer a cross-platform cleaner (rimraf).💡 Suggested fix
- "build": "rm -rf lib && tspc -p tsconfig.build.json", + "build": "rimraf lib && tspc -p tsconfig.build.json",{ "devDependencies": { "rimraf": "^6.0.1" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 24, The build script currently uses the non-portable shell command "rm -rf" in the "build" npm script; replace it with a cross-platform cleaner by adding rimraf as a devDependency and updating the "build" script to call rimraf (or use npx rimraf) to remove the lib directory before running tspc -p tsconfig.build.json; update package.json devDependencies to include "rimraf" (e.g., ^6.x) and change the "build" script string referenced as "build" to use rimraf instead of rm -rf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/providers.ts`:
- Line 144: The return type for the provider function was narrowed from allowing
void to only undefined, breaking backward compatibility; update the signature
that currently reads Awaitable<{ extractedParameters: { [key: string]: string }
} | undefined> to allow void as well (e.g. Awaitable<{ extractedParameters:
Record<string, string> } | void>) so implementations that return void or
Promise<void> remain valid; ensure you update the type alias or function/method
signature that declares this return type (the one returning extractedParameters)
and keep the extractedParameters shape as Record<string, string> for clarity.
In `@src/utils/claims.ts`:
- Line 3: Replace the relative import "import { SIGNATURES, strToUint8Array }
from './index.ts'" with the project alias import used elsewhere (e.g., import {
SIGNATURES, strToUint8Array } from '#src/...') so the module resolution is
consistent; update the import to reference the aliased path that exports
SIGNATURES and strToUint8Array (matching other imports in this file) and run
type-check/static analysis to confirm the alias resolves correctly.
In `@tsconfig.json`:
- Around line 17-19: The path mapping for the TypeScript compiler is incorrect:
change the "paths" entry that maps "#src/*" (the unique module alias used across
the repo) from pointing at the compiled output "./lib/*" to point at the source
files "./src/*" in tsconfig.json so TypeScript can resolve imports during
compilation; update the value for "#src/*" to ["./src/*"] and keep the rest of
the tsconfig paths intact.
---
Nitpick comments:
In `@package.json`:
- Line 24: The build script currently uses the non-portable shell command "rm
-rf" in the "build" npm script; replace it with a cross-platform cleaner by
adding rimraf as a devDependency and updating the "build" script to call rimraf
(or use npx rimraf) to remove the lib directory before running tspc -p
tsconfig.build.json; update package.json devDependencies to include "rimraf"
(e.g., ^6.x) and change the "build" script string referenced as "build" to use
rimraf instead of rm -rf.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e0b9046-4ea2-43b0-ac32-3735b7240f1a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
package.jsonsrc/external-rpc/benchmark.tssrc/external-rpc/types.tssrc/scripts/build-browser.tssrc/types/providers.tssrc/types/signatures.tssrc/utils/claims.tstsconfig.build.jsontsconfig.json
✅ Files skipped from review due to trivial changes (2)
- tsconfig.build.json
- src/external-rpc/benchmark.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scripts/build-browser.ts
- src/external-rpc/types.ts
| assertValidProviderReceipt( | ||
| opts: AssertValidProviderReceipt<Params> | ||
| ): void | Promise<void> | { extractedParameters: { [key: string]: string } } | ||
| ): Awaitable<{ extractedParameters: { [key: string]: string } } | undefined> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate all interface implementations/call sites
rg -nP --type=ts -C3 '\bassertValidProviderReceipt\s*\('
# 2) Heuristic scan for implementations that may not explicitly return a value
# (manual inspection required from context output)
rg -nP --type=ts -C8 'assertValidProviderReceipt\s*\([^)]*\)\s*\{'Repository: reclaimprotocol/attestor-core
Length of output: 8869
🏁 Script executed:
# View the http provider implementation of assertValidProviderReceipt
cat -n src/providers/http/index.ts | sed -n '257,350p'Repository: reclaimprotocol/attestor-core
Length of output: 3964
🏁 Script executed:
# Check for any other provider implementations
rg -l 'assertValidProviderReceipt\s*\(' --type=ts | xargs -I {} sh -c 'echo "=== {} ===" && grep -n "assertValidProviderReceipt" {}'Repository: reclaimprotocol/attestor-core
Length of output: 1038
🏁 Script executed:
# Look for the prior type definition to understand what changed
git log --oneline -n 20 -- src/types/providers.tsRepository: reclaimprotocol/attestor-core
Length of output: 132
🏁 Script executed:
# Continue reading the http provider implementation to see the return statement
cat -n src/providers/http/index.ts | sed -n '350,400p'Repository: reclaimprotocol/attestor-core
Length of output: 1720
🏁 Script executed:
# Check git diff to see what the prior signature was
git diff HEAD~1 HEAD -- src/types/providers.tsRepository: reclaimprotocol/attestor-core
Length of output: 103
🏁 Script executed:
# Also check if there's a return statement after line 350
rg -n 'return.*extractedParams' src/providers/http/index.ts -A2 -B2Repository: reclaimprotocol/attestor-core
Length of output: 177
🏁 Script executed:
# Search for all provider implementations (not just http)
find src/providers -name "*.ts" -type f | head -20Repository: reclaimprotocol/attestor-core
Length of output: 191
🏁 Script executed:
# Check if there are multiple provider implementations with assertValidProviderReceipt
rg -l 'assertValidProviderReceipt' src/providers --type=tsRepository: reclaimprotocol/attestor-core
Length of output: 100
🏁 Script executed:
# Check git history for the file to understand prior signature
git log --all --oneline src/types/providers.tsRepository: reclaimprotocol/attestor-core
Length of output: 1546
🏁 Script executed:
# Check the specific commit that changed providers.ts
git show c8cf7d3:src/types/providers.ts | grep -A10 "assertValidProviderReceipt"Repository: reclaimprotocol/attestor-core
Length of output: 369
🏁 Script executed:
# Check a few commits back to see prior signature
git show 1133d75:src/types/providers.ts 2>/dev/null | grep -A10 "assertValidProviderReceipt" || echo "File may not exist at that commit"Repository: reclaimprotocol/attestor-core
Length of output: 369
🏁 Script executed:
# Look at the full interface definition in providers.ts to see context
cat -n src/types/providers.ts | sed -n '130,150p'Repository: reclaimprotocol/attestor-core
Length of output: 1046
🏁 Script executed:
# Find the definition of Awaitable type
rg -n 'type Awaitable' src/types --type=ts -A2Repository: reclaimprotocol/attestor-core
Length of output: 234
🏁 Script executed:
# Check call site in assert-valid-claim-request.ts to see how result is used
cat -n src/server/utils/assert-valid-claim-request.ts | sed -n '155,165p'Repository: reclaimprotocol/attestor-core
Length of output: 373
🏁 Script executed:
# Check how rslt is used after the call
cat -n src/server/utils/assert-valid-claim-request.ts | sed -n '158,175p'Repository: reclaimprotocol/attestor-core
Length of output: 591
🏁 Script executed:
# Also check the client create-claim.ts call site
cat -n src/client/create-claim.ts | sed -n '545,560p'Repository: reclaimprotocol/attestor-core
Length of output: 546
Type narrowing from void to undefined removes prior compatibility.
The signature changed from void | Promise<void> | { extractedParameters: { [key: string]: string } } to Awaitable<{ extractedParameters: { [key: string]: string } } | undefined>, removing void as a valid return type. While the current http provider implementation always explicitly returns the extracted parameters object, any future provider implementations that return void (implicitly or explicitly) would violate the new contract.
Consider preserving backward compatibility:
Awaitable<{ extractedParameters: Record<string, string> } | void>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/providers.ts` at line 144, The return type for the provider
function was narrowed from allowing void to only undefined, breaking backward
compatibility; update the signature that currently reads Awaitable<{
extractedParameters: { [key: string]: string } } | undefined> to allow void as
well (e.g. Awaitable<{ extractedParameters: Record<string, string> } | void>) so
implementations that return void or Promise<void> remain valid; ensure you
update the type alias or function/method signature that declares this return
type (the one returning extractedParameters) and keep the extractedParameters
shape as Record<string, string> for clarity.
Comment on lines
+17
to
+19
| "paths": { | ||
| "#src/*": ["src/*"] | ||
| } | ||
| "#src/*": ["./lib/*"] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tsconfig.json `#src` path target:"
jq -r '.compilerOptions.paths["#src/*"]' tsconfig.json
echo
echo "tsconfig.build.json `#src` path target (if present):"
fd -a '^tsconfig\.build\.json$' -x sh -c 'echo "--- {}"; jq -r ".compilerOptions.paths[\"#src/*\"] // \"<not-set>\"" "{}"'
echo
echo "TS files importing `#src/`*:"
rg -nP --type=ts '(from\s+["'\'']#src/|import\(\s*["'\'']#src/)' src || trueRepository: reclaimprotocol/attestor-core
Length of output: 46393
Map #src/* to source (./src/*) instead of compiled output for proper TypeScript resolution.
Line 18 currently maps #src/* to ./lib/* (the compiled output directory), but 400+ source files throughout the codebase import from #src/* during compilation. This creates a broken dependency: TypeScript needs to resolve these paths before lib/ is generated, causing resolution to fail or depend on stale artifacts from prior builds.
💡 Suggested fix
"paths": {
- "#src/*": ["./lib/*"]
+ "#src/*": ["./src/*"]
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tsconfig.json` around lines 17 - 19, The path mapping for the TypeScript
compiler is incorrect: change the "paths" entry that maps "#src/*" (the unique
module alias used across the repo) from pointing at the compiled output
"./lib/*" to point at the source files "./src/*" in tsconfig.json so TypeScript
can resolve imports during compilation; update the value for "#src/*" to
["./src/*"] and keep the rest of the tsconfig paths intact.
Scratch-net
force-pushed
the
combined-s2-oprf
branch
from
c8cf7d3 to
1df746a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/external-rpc/benchmark.ts`:
- Around line 8-11: The file imports strToUint8Array twice which causes a
duplicate identifier error; remove the redundant import by keeping only one
import statement for strToUint8Array (the existing import that also brings in
other types or utilities as needed) and delete the duplicate line that
re-imports strToUint8Array, ensuring the import for CompleteTLSPacket remains
unchanged if present.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6066ae1d-203a-45ff-9f43-5bf61f95b31e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/external-rpc/benchmark.tssrc/scripts/build-browser.tssrc/utils/claims.tstsconfig.build.json
✅ Files skipped from review due to trivial changes (1)
- src/utils/claims.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tsconfig.build.json
- src/scripts/build-browser.ts
|
Promptless prepared a documentation update related to this change. Triggered by PR #68 Updated the OPRF-RAW documentation to accurately explain that it runs OPRF computation on the attestor instead of the client, where the attestor sees the plaintext value but still generates a cryptographic nullifier for verification. Also added clarification to the changelog. Review at https://app.gopromptless.ai/suggestions/e70c7544-b705-4dec-a281-b1835b64d1a6 |
Merged
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.
Combines PR #66 and PR #67 for testing
Summary by CodeRabbit
Release Notes
New Features
oprf-rawredaction mode for server-side OPRF processing with plaintext reveal to attestor.Dependencies
Chores