Repository navigation
feat: generic platform handling - #35
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR adds platform-aware handle/link verification (Twitter/X) by introducing platformName handling across entrypoints and verifiers, adds deployment orchestration and a Twitter-specific deploy script, provides new Noir circuits and generated Honk verifier artifacts, restructures test fixtures and utilities, and updates configs and ignore rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Entrypoint as LinkHandleEntrypoint
participant TextRecord as LinkTextRecordEntrypoint
participant Verifier as LinkHandleCommandVerifier
participant DB as State
User->>Entrypoint: submit(textRecord, recordName, platformName)
Entrypoint->>TextRecord: verifyTextRecord(textRecord, recordName, platformName)
TextRecord->>TextRecord: check recordNameHash
alt platformName configured mismatch
TextRecord-->>Entrypoint: revert InvalidCommand
Entrypoint-->>User: revert
else platformName ok
TextRecord->>Verifier: provide proof & public inputs
Verifier->>Verifier: extract PLATFORM_NAME and ENS_NAME from command
Verifier->>Verifier: validate textRecord.platformName matches
alt proof valid
Verifier-->>TextRecord: success
TextRecord->>DB: persist/mark verified
TextRecord-->>Entrypoint: success
Entrypoint-->>User: verified
else proof invalid
Verifier-->>TextRecord: revert
TextRecord-->>Entrypoint: revert
Entrypoint-->>User: revert
end
end
sequenceDiagram
autonumber
participant Dev
participant Script as compile.sh / prove.sh
participant Nargo
participant BB as bb
participant Target as target/
Dev->>Script: ./compile.sh twitter
Script->>Nargo: ensure nargo version, nargo compile
Nargo-->>Script: compiled
Script->>BB: bb write_vk, bb write_solidity_verifier
BB-->>Target: verification_key.vk, HonkVerifier.sol
Dev->>Script: ./prove.sh twitter
Script->>Nargo: nargo execute (witness)
Nargo-->>Script: witness
Script->>BB: bb prove (binary & fields)
BB-->>Target: proof.bin, proof_fields.json, public_inputs_fields.json
Script-->>Dev: artifacts ready in target/
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/fixtures/handleCommand/HandleCommandTestFixture.sol (1)
98-110:⚠️ Potential issue | 🟡 MinorComment/code mismatch: comment says
bytes32[154]but code usesbytes32[155].Line 102 comment says "Decode the blob into fixed bytes32[154]" but the actual decoding on line 103 and the array allocation on line 106 both correctly use 155. The comment is stale.
📝 Fix the comment
- // 2) Decode the blob into fixed bytes32[154] + // 2) Decode the blob into fixed bytes32[155]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/handleCommand/HandleCommandTestFixture.sol` around lines 98 - 110, The comment in _getPublicInputsFieldsFromBinary incorrectly states "bytes32[154]" while the code decodes and allocates 155 elements; update the inline comment to match the actual type/length (bytes32[155]) so it accurately reflects the decode operation and the subsequent allocation/loop over 155 elements in the function.
🧹 Nitpick comments (8)
test/fixtures/linkHandleCommand/prove.sh (1)
78-87: Silent skip on missing fields output could mask a broken Step 3.If
bb prove --output_format fieldssilently ignores the flag (e.g., wrongbbversion), Step 3 exits 0 but produces noproof_fields.json. Theif [ -f ... ]guards here then silently skip the move, and the script reports success while the JSON artifacts are absent. Consider failing loudly if the expected output files aren't present:♻️ Proposed improvement
-if [ -f "./target/fields_output/proof_fields.json" ]; then - mv ./target/fields_output/proof_fields.json ./target/proof_fields.json - log "Moved proof_fields.json to ./target/" -fi -if [ -f "./target/fields_output/public_inputs_fields.json" ]; then - mv ./target/fields_output/public_inputs_fields.json ./target/public_inputs_fields.json - log "Moved public_inputs_fields.json to ./target/" -fi +mv ./target/fields_output/proof_fields.json ./target/proof_fields.json \ + || { log "proof_fields.json not found after bb prove (fields)"; exit 1; } +log "Moved proof_fields.json to ./target/" +mv ./target/fields_output/public_inputs_fields.json ./target/public_inputs_fields.json \ + || { log "public_inputs_fields.json not found after bb prove (fields)"; exit 1; } +log "Moved public_inputs_fields.json to ./target/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/prove.sh` around lines 78 - 87, The current move block silently skips when proof_fields.json or public_inputs_fields.json are missing, masking a failed "bb prove --output_format fields" run; update the section around the mv checks to explicitly verify both ./target/fields_output/proof_fields.json and ./target/fields_output/public_inputs_fields.json exist and, if either is missing, call the existing log function to emit a clear error and exit non-zero (e.g., log "Missing proof_fields.json" and exit 1) rather than continuing, so the script fails loudly when Step 3 produced no artifacts.test/fixtures/linkHandleCommand/compile.sh (1)
43-49: Nargo version check could produce false positives.If
nargo --versionoutputs additional lines or whitespace, thegrepresult might differ from the literal comparison string even when the version matches. A more robust check:♻️ Suggested improvement
-if [ "$(nargo --version | grep "nargo version = $NARGO_VERSION")" != "nargo version = $NARGO_VERSION" ]; then +if ! nargo --version 2>/dev/null | grep -q "nargo version = $NARGO_VERSION"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/compile.sh` around lines 43 - 49, The current nargo version check compares the full grep output string which can fail if nargo --version prints extra lines/whitespace; change the check to extract and compare the version more robustly (e.g., capture nargo --version output, trim and extract the version token and compare that to NARGO_VERSION or use grep -qF "nargo version = $NARGO_VERSION" to test presence). Update the snippet that uses nargo --version and the conditional that triggers noirup --version $NARGO_VERSION (keeping LOG_FILE and exit behavior) so the test reliably detects matching versions even with extra output or whitespace.script/LinkHandleWithFixture.s.sol (1)
7-19: LGTM — clean migration to the Twitter-specific fixture.Note: if
LINK_X_HANDLE_VERIFIERat0x8cd219...was deployed with the old fixture's circuit, it should be redeployed using the new Twitter-specificHonkVerifierbefore running this script against Sepolia.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/LinkHandleWithFixture.s.sol` around lines 7 - 19, The constant LINK_X_HANDLE_VERIFIER in LinkHandleWithFixtureScript may point to a verifier deployed with the old fixture; before running, ensure the verifier contract at that address is redeployed with the new Twitter-specific HonkVerifier or make the script configurable: replace the hardcoded LINK_X_HANDLE_VERIFIER with an address loaded from env (vm.envAddress or vm.envUint converted) or update the constant to the new deployed verifier address so LinkHandleEntrypoint(LINK_X_HANDLE_VERIFIER) matches the HonkVerifier expected by LinkHandleCommandTestFixture.getTwitterFixture.test/src/verifiers/ClaimHandleCommandVerifier/Encode.t.sol (1)
5-5: Inconsistency: link-handleHonkVerifierpaired withClaimHandleCommandVerifier.These encoding tests don't invoke
verify(), so no failure occurs here. However, this is inconsistent with the claim-handle circuit: any future test in this suite that exercisesisValid()will fail at proof verification. Aligning the import with the claim-handleHonkVerifier(same fix as inDeployHandleRegistrar.s.sol) avoids latent breakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/verifiers/ClaimHandleCommandVerifier/Encode.t.sol` at line 5, The test imports the link-handle HonkVerifier while exercising ClaimHandleCommandVerifier, which will cause future isValid()/verify() checks to fail; change the import to the claim-handle variant of HonkVerifier so the verifier implementation matches ClaimHandleCommandVerifier (same fix applied in DeployHandleRegistrar.s.sol), then re-run tests that call isValid()/verify() to ensure proof verification succeeds.src/entrypoints/LinkTextRecordEntrypoint.sol (1)
39-44: Minor doc inconsistency:platformNameexample values differ across files.The NatDoc here says
platformNamee.g."x", but inLinkHandleEntrypoint.sol(line 17) the example is"Twitter". While these could be valid distinct examples, having different casing/values for the same concept across related files is confusing. Consider aligning the examples.📝 Suggested doc alignment
- /// `@param` platformName Platform name in the command (e.g. "x") + /// `@param` platformName Platform name in the command (e.g. "Twitter")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/LinkTextRecordEntrypoint.sol` around lines 39 - 44, The NatSpec for the constructor parameter platformName is inconsistent with LinkHandleEntrypoint.sol; update the doc comment for the constructor's platformName parameter to use the same example/casing as in LinkHandleEntrypoint.sol (e.g., "Twitter") so the docs are aligned — change the example in the constructor's `@param` platformName line to match LinkHandleEntrypoint.sol while leaving VERIFIER, _recordNameHash, and _platformNameHash logic unchanged.test/fixtures/linkHandleCommand/LinkHandleCommandTestFixture.sol (2)
69-87: Magic numbers 440 and 155 must stay synchronized with the verifier.These sizes correspond to
PROOF_SIZE(440) andNUMBER_OF_PUBLIC_INPUTS(155) in the HonkVerifier. If those constants change, these fixtures will silently produce incorrect data. Consider referencing the constants or adding a comment noting the dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/LinkHandleCommandTestFixture.sol` around lines 69 - 87, The functions _getProofFieldsFromBinary and _getPublicInputsFieldsFromBinary currently hardcode 440 and 155; replace these magic numbers by introducing named constants (e.g., PROOF_SIZE = 440 and NUMBER_OF_PUBLIC_INPUTS = 155) at the top of the fixture or, if possible, import the verifier constants, and add a runtime sanity check after reading the binary (validate abi.decode yields the expected length or that packed.length matches expected bytes) that reverts or fails the test with a clear message if they diverge so the fixture stays synchronized with the HonkVerifier.
42-51: Word index forplatformNameextraction is tightly coupled to command format.
getNthWord(expectedPublicInputs.command, 2)assumes the command is always"Link my <platform> ...". If the command template ever changes word ordering, this silently extracts the wrong field. A comment documenting the expected command format would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/LinkHandleCommandTestFixture.sol` around lines 42 - 51, The extraction of platformName in LinkHandleCommand uses TestStringUtils.getNthWord(expectedPublicInputs.command, 2) which assumes a fixed command phrase; add a short inline comment next to the LinkHandleCommand/TextRecord construction explaining the exact expected command format (e.g., which word index corresponds to platform and which to ensName) and why index 2 is used, or replace the magic index with a clearly named constant (e.g., PLATFORM_WORD_INDEX) and document that constant so future maintainers know the required command template; reference LinkHandleCommand, TextRecord, TestStringUtils.getNthWord, and expectedPublicInputs.command when adding the comment or constant.src/verifiers/LinkHandleCommandVerifier.sol (1)
16-19: Extract the sharedCommandParamIndexenum to a common utilities file.The
CommandParamIndexenum is defined identically in bothLinkHandleCommandVerifier.solandLinkEmailCommandVerifier.solat file scope. While these verifiers are not currently imported together, having two enums with the same name creates a latent naming collision risk if both files are imported in the same compilation unit (e.g., in a shared test or integration contract). Consider consolidating this enum into a shared utilities file or library.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/verifiers/LinkHandleCommandVerifier.sol` around lines 16 - 19, Extract the shared enum by moving CommandParamIndex out of LinkHandleCommandVerifier.sol into a new shared utilities file (e.g., a utils or types library) and remove the duplicate enum from LinkEmailCommandVerifier.sol; update both verifier contracts to import and reference the centralized CommandParamIndex type instead of redeclaring it, ensure the new utils file declares the enum at file scope and is accessible (public/internal as appropriate) and update any references in functions or mappings that use CommandParamIndex so compilation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 28-29: The current patterns "**/target/*" and "!**/target/*.sol"
leave .sol files nested in subdirectories under target ignored because git won't
traverse ignored directories; update the rules to un-ignore Solidity files
anywhere under target by replacing "!**/target/*.sol" with "!**/target/**/*.sol"
and also add a directory un-ignore pattern like "!**/target/**/" so git can
descend into those directories and pick up nested .sol files.
In `@script/DeployAllDiscord.s.sol`:
- Line 8: The DeployAllDiscord.s.sol script incorrectly imports the
Twitter-specific HonkVerifier; replace the import of HonkVerifier coming from
the twitter fixture with the platform-appropriate verifier (either the generic
handleCommand/HonkVerifier.sol or a Discord-specific verifier if available) so
DeployAllDiscord.s.sol uses the correct circuit; update the import line
referencing HonkVerifier and ensure any deployment code in
DeployAllDiscord.s.sol that constructs or references HonkVerifier continues to
work with the new verifier symbol.
In `@script/DeployHandleRegistrar.s.sol`:
- Line 7: The script imports the wrong HonkVerifier for deploying
ClaimHandleCommandVerifier; update the import so HonkVerifier used by
ClaimHandleCommandVerifier matches the noir-generated verifier for the
claim-handle circuit (replace the import that references
linkHandleCommand/twitter/target with the HonkVerifier from the handleCommand
fixture), ensuring the deployed ClaimHandleCommandVerifier uses the correct
verification key.
In `@script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol`:
- Around line 39-45: Fix the typo in the NatSpec comment inside
_DeployLinkHandleEntrypoint.s.sol: change "ideally ths would be" to "ideally
this would be" in the block that documents the HonkVerifier deployment (the
comment above the function that "Deploys the HonkVerifier and returns the
address"); ensure the rest of the doc comment remains unchanged.
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 42-46: The three error-handler blocks after the commands `nargo
execute "$WITNESS_NAME" >> "$LOG_FILE" 2>&1`, `bb prove "$WITNESS_NAME" -o
"$PROOF_FILE" >> "$LOG_FILE" 2>&1`, and `bb prove "$WITNESS_NAME" -o
"$PROOF_FIELDS_FILE" --as-fields >> "$LOG_FILE" 2>&1` are dead code under `set
-euo pipefail`; replace each `command; if [ $? -ne 0 ]; then log "..."; exit 1;
fi` pattern with the safe idiom `command >> "$LOG_FILE" 2>&1 || { log "Failed to
..."; exit 1; }` so the custom log runs on failure without being short-circuited
by `set -e`.
- Around line 22-26: The CIRCUIT_NAME extraction can produce multiple lines if
Nargo.toml contains more than one `name = ...`; update the extraction in
prove.sh so CIRCUIT_NAME is limited to the first match (e.g., use grep -m 1 or
pipe to head -n1) before the sed/tr -d processing to ensure a single value is
produced; modify the command that sets CIRCUIT_NAME (the line assigning
CIRCUIT_NAME) so downstream path arguments that use CIRCUIT_NAME are not
corrupted by newline-separated values.
- Around line 64-75: The script currently uses the non-existent flag
"--output_format fields" on the bb prove invocation; remove that flag and
generate fields-format output using one of the supported approaches: either run
bb prove without "--output_format" to produce the binary proof and then call bb
proof_as_fields -p ./target/proof -k ./target/vk -o ./target/fields_output to
convert to fields, or replace the bb prove call entirely with the single-step bb
prove_output_all command (e.g. bb prove_output_all --scheme ultra_honk -b
./target/$CIRCUIT_NAME.json -w ./target/$WITNESS_NAME.gz -o ./target) which
emits both binary and *_fields.json outputs; update the script to use one of
these flows and remove the invalid flag from the bb prove invocation.
In `@test/fixtures/linkHandleCommand/twitter/Nargo.toml`:
- Line 5: The compiler_version constraint in the Nargo manifest is too broad and
won't match the pinned pre-release; update the compiler_version entry (the
compiler_version key in test/fixtures/linkHandleCommand/twitter/Nargo.toml) to
explicitly include the beta pre-release used by compile.sh—either pin to
"1.0.0-beta.5" or use a range like ">=1.0.0-beta.5, <1.0.0" so the manifest will
match the Nargo 1.0.0-beta.5 compiler used by compile.sh.
In `@test/fixtures/redditHandleCommand/RedditHandleCommandTestFixture.sol`:
- Line 48: The platformName is extracted with the wrong 0-based index: update
the call to TestStringUtils.getNthWord so it uses index 1 instead of 2 (i.e.,
change the expression that sets platformName from
TestStringUtils.getNthWord(expectedPublicInputs.command, 2) to use 1) so
platformName correctly picks "reddit" from expectedPublicInputs.command; ensure
you only modify the platformName assignment referencing
TestStringUtils.getNthWord and leave other indices unchanged.
---
Outside diff comments:
In `@test/fixtures/handleCommand/HandleCommandTestFixture.sol`:
- Around line 98-110: The comment in _getPublicInputsFieldsFromBinary
incorrectly states "bytes32[154]" while the code decodes and allocates 155
elements; update the inline comment to match the actual type/length
(bytes32[155]) so it accurately reflects the decode operation and the subsequent
allocation/loop over 155 elements in the function.
---
Nitpick comments:
In `@script/LinkHandleWithFixture.s.sol`:
- Around line 7-19: The constant LINK_X_HANDLE_VERIFIER in
LinkHandleWithFixtureScript may point to a verifier deployed with the old
fixture; before running, ensure the verifier contract at that address is
redeployed with the new Twitter-specific HonkVerifier or make the script
configurable: replace the hardcoded LINK_X_HANDLE_VERIFIER with an address
loaded from env (vm.envAddress or vm.envUint converted) or update the constant
to the new deployed verifier address so
LinkHandleEntrypoint(LINK_X_HANDLE_VERIFIER) matches the HonkVerifier expected
by LinkHandleCommandTestFixture.getTwitterFixture.
In `@src/entrypoints/LinkTextRecordEntrypoint.sol`:
- Around line 39-44: The NatSpec for the constructor parameter platformName is
inconsistent with LinkHandleEntrypoint.sol; update the doc comment for the
constructor's platformName parameter to use the same example/casing as in
LinkHandleEntrypoint.sol (e.g., "Twitter") so the docs are aligned — change the
example in the constructor's `@param` platformName line to match
LinkHandleEntrypoint.sol while leaving VERIFIER, _recordNameHash, and
_platformNameHash logic unchanged.
In `@src/verifiers/LinkHandleCommandVerifier.sol`:
- Around line 16-19: Extract the shared enum by moving CommandParamIndex out of
LinkHandleCommandVerifier.sol into a new shared utilities file (e.g., a utils or
types library) and remove the duplicate enum from LinkEmailCommandVerifier.sol;
update both verifier contracts to import and reference the centralized
CommandParamIndex type instead of redeclaring it, ensure the new utils file
declares the enum at file scope and is accessible (public/internal as
appropriate) and update any references in functions or mappings that use
CommandParamIndex so compilation succeeds.
In `@test/fixtures/linkHandleCommand/compile.sh`:
- Around line 43-49: The current nargo version check compares the full grep
output string which can fail if nargo --version prints extra lines/whitespace;
change the check to extract and compare the version more robustly (e.g., capture
nargo --version output, trim and extract the version token and compare that to
NARGO_VERSION or use grep -qF "nargo version = $NARGO_VERSION" to test
presence). Update the snippet that uses nargo --version and the conditional that
triggers noirup --version $NARGO_VERSION (keeping LOG_FILE and exit behavior) so
the test reliably detects matching versions even with extra output or
whitespace.
In `@test/fixtures/linkHandleCommand/LinkHandleCommandTestFixture.sol`:
- Around line 69-87: The functions _getProofFieldsFromBinary and
_getPublicInputsFieldsFromBinary currently hardcode 440 and 155; replace these
magic numbers by introducing named constants (e.g., PROOF_SIZE = 440 and
NUMBER_OF_PUBLIC_INPUTS = 155) at the top of the fixture or, if possible, import
the verifier constants, and add a runtime sanity check after reading the binary
(validate abi.decode yields the expected length or that packed.length matches
expected bytes) that reverts or fails the test with a clear message if they
diverge so the fixture stays synchronized with the HonkVerifier.
- Around line 42-51: The extraction of platformName in LinkHandleCommand uses
TestStringUtils.getNthWord(expectedPublicInputs.command, 2) which assumes a
fixed command phrase; add a short inline comment next to the
LinkHandleCommand/TextRecord construction explaining the exact expected command
format (e.g., which word index corresponds to platform and which to ensName) and
why index 2 is used, or replace the magic index with a clearly named constant
(e.g., PLATFORM_WORD_INDEX) and document that constant so future maintainers
know the required command template; reference LinkHandleCommand, TextRecord,
TestStringUtils.getNthWord, and expectedPublicInputs.command when adding the
comment or constant.
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 78-87: The current move block silently skips when
proof_fields.json or public_inputs_fields.json are missing, masking a failed "bb
prove --output_format fields" run; update the section around the mv checks to
explicitly verify both ./target/fields_output/proof_fields.json and
./target/fields_output/public_inputs_fields.json exist and, if either is
missing, call the existing log function to emit a clear error and exit non-zero
(e.g., log "Missing proof_fields.json" and exit 1) rather than continuing, so
the script fails loudly when Step 3 produced no artifacts.
In `@test/src/verifiers/ClaimHandleCommandVerifier/Encode.t.sol`:
- Line 5: The test imports the link-handle HonkVerifier while exercising
ClaimHandleCommandVerifier, which will cause future isValid()/verify() checks to
fail; change the import to the claim-handle variant of HonkVerifier so the
verifier implementation matches ClaimHandleCommandVerifier (same fix applied in
DeployHandleRegistrar.s.sol), then re-run tests that call isValid()/verify() to
ensure proof verification succeeds.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/fixtures/linkHandleCommand/prove.sh (1)
47-49: Unquoted variables inbb provepath arguments.
$CIRCUIT_NAMEand$WITNESS_NAMEare expanded without double quotes on lines 47–49 and 58–60. Whiletr -d ' 'strips ASCII spaces, it leaves other IFS characters (tabs, newlines) intact, making word-splitting technically possible.♻️ Proposed fix (apply to both `bb prove` invocations)
- --bytecode_path ./target/$CIRCUIT_NAME.json \ - --witness_path ./target/$WITNESS_NAME.gz \ - --output_path ./target \ + --bytecode_path "./target/$CIRCUIT_NAME.json" \ + --witness_path "./target/$WITNESS_NAME.gz" \ + --output_path ./target \- --bytecode_path ./target/$CIRCUIT_NAME.json \ - --witness_path ./target/$WITNESS_NAME.gz \ - --output_path ./target/fields_output \ + --bytecode_path "./target/$CIRCUIT_NAME.json" \ + --witness_path "./target/$WITNESS_NAME.gz" \ + --output_path ./target/fields_output \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/prove.sh` around lines 47 - 49, The bb prove invocations use unquoted expansions of $CIRCUIT_NAME and $WITNESS_NAME in the --bytecode_path, --witness_path and --output_path arguments which can still be subject to word-splitting; fix both bb prove invocations by wrapping these expansions in double quotes (e.g. "--bytecode_path \"./target/${CIRCUIT_NAME}.json\"" and "--witness_path \"./target/${WITNESS_NAME}.gz\""), and ensure --output_path uses a quoted path as well so all path arguments are protected from IFS splitting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 66-75: The script silently succeeds when bb prove --output_format
fields exits 0 but does not produce proof_fields.json/public_inputs_fields.json;
update the proof-moving block (the checks that mv
./target/fields_output/proof_fields.json -> ./target/proof_fields.json and mv
public_inputs_fields.json -> ./target/public_inputs_fields.json) to assert that
at least ./target/proof_fields.json exists after the move: if proof_fields.json
is missing, log an error via the existing log function and exit non‑zero; only
emit the final success log (the messages that claim files were generated) when
./target/proof_fields.json is present (and optionally public_inputs_fields.json
if required). Ensure rm -rf ./target/fields_output still runs for cleanup but
perform the existence check before printing success or returning 0.
---
Duplicate comments:
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 22-26: The CIRCUIT_NAME extraction can return multiple lines if
Nargo.toml contains more than one "name = ..." entry; change the extraction
pipeline that sets CIRCUIT_NAME (the grep/sed/tr -d command) to limit grep to
the first match (e.g., use grep -m 1 '^name = ' Nargo.toml) so CIRCUIT_NAME is
always a single value, then keep the existing sed and tr trimming; ensure
downstream uses of CIRCUIT_NAME (the path arguments referenced later) receive
the single, trimmed string.
---
Nitpick comments:
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 47-49: The bb prove invocations use unquoted expansions of
$CIRCUIT_NAME and $WITNESS_NAME in the --bytecode_path, --witness_path and
--output_path arguments which can still be subject to word-splitting; fix both
bb prove invocations by wrapping these expansions in double quotes (e.g.
"--bytecode_path \"./target/${CIRCUIT_NAME}.json\"" and "--witness_path
\"./target/${WITNESS_NAME}.gz\""), and ensure --output_path uses a quoted path
as well so all path arguments are protected from IFS splitting.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
zkfriendly
approved these changes
Feb 24, 2026
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.
Summary by CodeRabbit
New Features
Tests
Chores