Repository navigation
feat: generic platform handling - #33
Closed
benceharomi wants to merge 11 commits into
Closed
benceharomi wants to merge 11 commits into
benceharomi wants to merge 11 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces platform-configurable link-handle support (recordName and platformName) across deployment scripts, entrypoints, verifiers, tests, and fixtures; adds Twitter-specific Noir fixtures and a HonkVerifier implementation; introduces TestStringUtils for word extraction; and updates imports and tooling config. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 2
🧹 Nitpick comments (5)
script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol (1)
42-42:_getHonkVerifierAddress()mutability should be documentedThe TODO on line 37 captures the intent well. One nit: the function's
virtual(non-pure) visibility is the root cause of the double-deployment issue above. Once the registry takes over HonkVerifier deployment and this can becomepure, the double-call hazard disappears entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol` at line 42, Document the mutability and intent of _getHonkVerifierAddress(): add a NatSpec `@dev` above the function explaining why it is currently non-pure/virtual (causes double-deploy hazard) and include the existing TODO that once the registry owns HonkVerifier this should become pure; if you can confirm the registry now provides a static address, change the signature of _getHonkVerifierAddress() to be pure and update callers accordingly to eliminate the double-call hazard.script/DeployLinkHandleEntrypoint/DeployLinkHandleEntrypointTwitter.s.sol (1)
5-5:HonkVerifierimported from test fixturesThis deployment script imports
HonkVerifierfrom../../test/fixtures/, meaning the test verifier artifact is used for production deployment. This is acknowledged by theTODOin the base script. Ensure this is intentional for the current deployment target (e.g., Sepolia testnet only) and is replaced before any mainnet deployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/DeployLinkHandleEntrypoint/DeployLinkHandleEntrypointTwitter.s.sol` at line 5, The deployment script currently imports the test-only verifier artifact HonkVerifier from the test fixtures (the import of HonkVerifier), which is unsafe for mainnet; confirm this is intentional for the current Sepolia-only deployment and, if not, replace the import with the production verifier contract (or the correct artifact path) and update any references to HonkVerifier in DeployLinkHandleEntrypointTwitter.s.sol to use the production verifier contract name and ABI before merging for mainnet.test/fixtures/redditHandleCommand/RedditHandleCommandTestFixture.sol (1)
46-55: Document the hardcoded word-position index2forplatformNameextraction
getNthWord(expectedPublicInputs.command, 2)relies on the command format "Link my handle to ". If the command template in expected_public_inputs.json changes, this silently breaks. Add a named constant (e.g.,PLATFORM_WORD_INDEX = 2) with a comment documenting the assumed format, so the coupling is explicit and changes to the command template are caught during test review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/redditHandleCommand/RedditHandleCommandTestFixture.sol` around lines 46 - 55, Replace the magic index 2 used in TestStringUtils.getNthWord(expectedPublicInputs.command, 2) with a clearly named constant (e.g., PLATFORM_WORD_INDEX = 2) and add a comment near the constant documenting the expected command format "Link my <platform> handle to <ensName>" so the coupling is explicit; update the LinkHandleCommand construction (TextRecord.platformName) to call TestStringUtils.getNthWord(expectedPublicInputs.command, PLATFORM_WORD_INDEX) and ensure the constant is visible to the test fixture for future reviewers.test/utils/TestStringUtils.t.sol (1)
1-47: Good test coverage for the core cases.Consider adding edge-case tests for strings with multiple consecutive spaces, leading/trailing spaces, and single-word input to ensure
getNthWordhandles them correctly. These are the scenarios most likely to produce off-by-one issues in the word-counting logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/TestStringUtils.t.sol` around lines 1 - 47, Add unit tests to TestStringUtilsTest invoking TestStringUtilsHelper.callGetNthWord to cover edge cases: (1) single-word input (e.g., "word") with index 0 and negative index -1, (2) inputs with multiple consecutive spaces (e.g., "a b c") verifying indexes 0..2 and negatives, and (3) inputs with leading/trailing spaces (e.g., " a b c ") verifying that leading/trailing spaces are ignored and indexes map to "a","b","c"; reuse existing vm.expectRevert(TestStringUtils.WordIndexOutOfBounds.selector) checks for out-of-range indices and add appropriate assertEq checks for valid returns using callGetNthWord.src/verifiers/LinkHandleCommandVerifier.sol (1)
16-19: Multiple verifier files define identical file-levelCommandParamIndexenums.
LinkHandleCommandVerifier.sol,LinkEmailCommandVerifier.sol,ProveAndClaimCommandVerifier.sol, andClaimHandleCommandVerifier.soleach define a file-levelenum CommandParamIndex. While the current architecture avoids collisions by importing each verifier independently, this pattern creates maintenance risk and reduces clarity. If any central contract ever needs to import multiple verifiers, a naming collision would occur.Consider consolidating shared enums (like
PLATFORM_NAME/ENS_NAMEused by both Link verifiers) into a common definitions file, or scoping each enum inside its respective contract.🤖 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, Multiple verifier files declare the same top-level enum CommandParamIndex (values PLATFORM_NAME, ENS_NAME), risking name collisions; move the shared enum into a single common definitions file (e.g., a new VerifierTypes.sol or Definitions.sol) and have LinkHandleCommandVerifier, LinkEmailCommandVerifier, ProveAndClaimCommandVerifier, and ClaimHandleCommandVerifier import that file, or alternatively scope the enum inside each contract (e.g., contract LinkHandleCommandVerifier { enum CommandParamIndex { ... } }) so the identifier is not global; update all references to CommandParamIndex in those contracts to use the new imported symbol or the scoped enum name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol`:
- Around line 9-33: The run function calls _getHonkVerifierAddress() twice which
deploys two HonkVerifier instances; capture the verifier address once and reuse
it for both construction and logging to avoid deploying a second unused
verifier. Specifically, call _getHonkVerifierAddress() a single time (either
assign uint256/ address verifierAddr = _getHonkVerifierAddress() before
vm.startBroadcast or call it once during broadcast and store it), pass that
stored verifierAddr into the LinkHandleCommandVerifier constructor when creating
commandVerifier, and use the same verifierAddr in the final log for
HONK_VERIFIER= instead of calling _getHonkVerifierAddress() again. Ensure the
rest of the logs still reference address(commandVerifier) and
address(entrypoint) as before.
In `@script/DeployLinkHandleEntrypoint/DeployLinkHandleEntrypointDiscord.s.sol`:
- Around line 1-20: This file is a stale Twitter copy-paste and fails to
compile; replace the contract with a Discord-specific implementation that
matches the abstract base by implementing the exact abstract methods
_getHonkVerifierAddress(), _getDkimRegistryAddress(), _getPlatformName(), and
_getRecordName() (remove/stop using the non-existent overrides
_deployHonkVerifier(), _dkimRegistry(), and _keyName()); ensure the contract
name is unique (e.g., DeployLinkHandleEntrypointDiscordScript) so it does not
collide with the Twitter script, return address(new HonkVerifier()) from
_getHonkVerifierAddress(), provide the correct sepolia DKIM address from the
file for _getDkimRegistryAddress(), and return Discord-specific strings for
_getPlatformName() (e.g., "com.discord") and _getRecordName() (e.g., "discord")
so the contract fully implements the base interface and compiles.
---
Duplicate comments:
In `@src/verifiers/LinkEmailCommandVerifier.sol`:
- Around line 14-20: The enum CommandParamIndex is duplicated; replace the local
enum in LinkEmailCommandVerifier (and the counterpart in the other verifier)
with a single shared definition: move CommandParamIndex to a common location
(e.g., a shared enums or types contract/library) and have
LinkEmailCommandVerifier import and use that shared CommandParamIndex instead of
declaring its own; update any references in functions/methods that use
CommandParamIndex to refer to the imported type and remove the duplicate enum
declarations.
---
Nitpick comments:
In `@script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol`:
- Line 42: Document the mutability and intent of _getHonkVerifierAddress(): add
a NatSpec `@dev` above the function explaining why it is currently
non-pure/virtual (causes double-deploy hazard) and include the existing TODO
that once the registry owns HonkVerifier this should become pure; if you can
confirm the registry now provides a static address, change the signature of
_getHonkVerifierAddress() to be pure and update callers accordingly to eliminate
the double-call hazard.
In `@script/DeployLinkHandleEntrypoint/DeployLinkHandleEntrypointTwitter.s.sol`:
- Line 5: The deployment script currently imports the test-only verifier
artifact HonkVerifier from the test fixtures (the import of HonkVerifier), which
is unsafe for mainnet; confirm this is intentional for the current Sepolia-only
deployment and, if not, replace the import with the production verifier contract
(or the correct artifact path) and update any references to HonkVerifier in
DeployLinkHandleEntrypointTwitter.s.sol to use the production verifier contract
name and ABI before merging for mainnet.
In `@src/verifiers/LinkHandleCommandVerifier.sol`:
- Around line 16-19: Multiple verifier files declare the same top-level enum
CommandParamIndex (values PLATFORM_NAME, ENS_NAME), risking name collisions;
move the shared enum into a single common definitions file (e.g., a new
VerifierTypes.sol or Definitions.sol) and have LinkHandleCommandVerifier,
LinkEmailCommandVerifier, ProveAndClaimCommandVerifier, and
ClaimHandleCommandVerifier import that file, or alternatively scope the enum
inside each contract (e.g., contract LinkHandleCommandVerifier { enum
CommandParamIndex { ... } }) so the identifier is not global; update all
references to CommandParamIndex in those contracts to use the new imported
symbol or the scoped enum name.
In `@test/fixtures/redditHandleCommand/RedditHandleCommandTestFixture.sol`:
- Around line 46-55: Replace the magic index 2 used in
TestStringUtils.getNthWord(expectedPublicInputs.command, 2) with a clearly named
constant (e.g., PLATFORM_WORD_INDEX = 2) and add a comment near the constant
documenting the expected command format "Link my <platform> handle to <ensName>"
so the coupling is explicit; update the LinkHandleCommand construction
(TextRecord.platformName) to call
TestStringUtils.getNthWord(expectedPublicInputs.command, PLATFORM_WORD_INDEX)
and ensure the constant is visible to the test fixture for future reviewers.
In `@test/utils/TestStringUtils.t.sol`:
- Around line 1-47: Add unit tests to TestStringUtilsTest invoking
TestStringUtilsHelper.callGetNthWord to cover edge cases: (1) single-word input
(e.g., "word") with index 0 and negative index -1, (2) inputs with multiple
consecutive spaces (e.g., "a b c") verifying indexes 0..2 and negatives, and
(3) inputs with leading/trailing spaces (e.g., " a b c ") verifying that
leading/trailing spaces are ignored and indexes map to "a","b","c"; reuse
existing vm.expectRevert(TestStringUtils.WordIndexOutOfBounds.selector) checks
for out-of-range indices and add appropriate assertEq checks for valid returns
using callGetNthWord.
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)
test/fixtures/handleCommand/HandleCommandTestFixture.sol (1)
102-103:⚠️ Potential issue | 🟡 MinorStale 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 uses
bytes32[155].📝 Fix 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 102 - 103, The inline comment above the decode is stale—update the comment to match the actual decode size: change the text that says "bytes32[154]" to "bytes32[155]" so it reflects the decoding into (bytes32[155] memory publicInputsFixed) from publicInputsFieldsData (referencing publicInputsFixed and abi.decode(publicInputsFieldsData, (bytes32[155]))).
🧹 Nitpick comments (6)
test/fixtures/linkHandleCommand/prove.sh (1)
1-26: Shared preamble withcompile.shandclean.shcould be extracted.The argument parsing,
SCRIPT_DIR/WORKDIRresolution, andCIRCUIT_NAMEextraction (Lines 1-26) are duplicated across all three scripts. Consider extracting a shared_common.shthat each script sources.🤖 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 1 - 26, Extract the duplicated argument parsing and environment setup from prove.sh/compile.sh/clean.sh into a new sourced script (e.g., _common.sh) that exports PLATFORM, SCRIPT_DIR, WORKDIR and sets CIRCUIT_NAME by reading Nargo.toml; then replace the top-of-file blocks in prove.sh, compile.sh and clean.sh with a single source line (source "_common.sh" or ". _common.sh") so each script reuses the same parsing/resolution logic and error handling for missing args, missing WORKDIR, and empty CIRCUIT_NAME.test/fixtures/linkHandleCommand/compile.sh (1)
43-46: Fragile version comparison fornargo --version.The comparison
"$(nargo --version | grep ...)" != "nargo version = $NARGO_VERSION"depends on exact output format. Ifnargo --versionoutput changes slightly (e.g., extra metadata), this will always triggernoirup. Consider a simpler substring check:💡 Suggested alternative
-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 - 46, The current exact-string comparison of nargo --version output is fragile; change the check in compile.sh to test for the presence of the expected version substring instead of matching the entire line: run nargo --version and use grep -q (or grep -F) to check for "$NARGO_VERSION" (or a stable prefix like "nargo version") and branch on its exit status so noirup --version is only invoked when the version substring is missing.test/fixtures/linkHandleCommand/LinkHandleCommandTestFixture.sol (1)
69-87: Hardcoded array sizes (440, 155) are tightly coupled to the HonkVerifier constants.These sizes match
PROOF_SIZE = 440andNUMBER_OF_PUBLIC_INPUTS = 155in the HonkVerifier. Since this is a test fixture, the coupling is acceptable, but if the verifier changes, these will silently produce incorrect decodes. Consider adding a brief 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 two helper functions _getProofFieldsFromBinary and _getPublicInputsFieldsFromBinary currently use hardcoded sizes (440 and 155) that mirror HonkVerifier's PROOF_SIZE and NUMBER_OF_PUBLIC_INPUTS; add a concise comment above each function noting this dependency (e.g., "Depends on HonkVerifier.PROOF_SIZE = 440" and "Depends on HonkVerifier.NUMBER_OF_PUBLIC_INPUTS = 155") so future maintainers know the decode sizes must be updated if the verifier constants change.test/fixtures/linkHandleCommand/twitter/target/HonkVerifier.sol (1)
1-3: Dual pragma statements in a single file.Line 3 declares
pragma solidity >=0.8.21;and line 127 declarespragma solidity ^0.8.27;. The compiler uses the intersection (>=0.8.27 <0.9.0), which is valid but unusual. Since this is an auto-generated verifier, this is likely an artifact of concatenating multiple source files.Also applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/twitter/target/HonkVerifier.sol` around lines 1 - 3, This file contains two conflicting pragma directives ("pragma solidity >=0.8.21" at the top and "pragma solidity ^0.8.27" later), so remove the duplicate and consolidate to a single pragma; locate the second occurrence ("pragma solidity ^0.8.27") and delete it (or replace the top pragma with the desired single directive) so the file has one consistent pragma statement (e.g., keep "pragma solidity ^0.8.27;" or a single "pragma solidity >=0.8.21;" according to project standard).test/fixtures/linkHandleCommand/twitter/src/x_handle_regex.nr (1)
134-139: Potential underflow ifmatch_lengthis 0.Line 136:
match_length - 1on au32will underflow/wrap whenmatch_length == 0. In Noir, unchecked u32 arithmetic wraps, which would makein_rangetrue for all iterations, causing the assertion on Line 138 to evaluate against uninitialized or zero state data. While a zero-length match is unlikely in practice (and the accept-state check on Line 156 provides a backstop), consider adding a guard or documenting the invariant thatmatch_length > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/twitter/src/x_handle_regex.nr` around lines 134 - 139, The loop computing transitions can underflow when match_length == 0 because it uses match_length - 1; to fix, add an explicit guard that ensures the loop body runs only when match_length > 0 (or early-return/skip the transition loop when match_length == 0) so the in_range calculation and the assert involving current_states and next_states are never evaluated with a wrapped value; modify the code around the for-loop that references match_length, in_range, current_states, next_states, and the assert to check match_length > 0 before entering the loop (or document and enforce the invariant that match_length is always > 0).test/fixtures/linkHandleCommand/twitter/src/main.nr (1)
4-4: Unused import:pedersen_hashis never referenced in this module.
pedersen_hashis imported but not used anywhere inmain. Remove it to keep imports clean.Proposed fix
-use std::{collections::bounded_vec::BoundedVec, hash::pedersen_hash}; +use std::collections::bounded_vec::BoundedVec;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/linkHandleCommand/twitter/src/main.nr` at line 4, The import list includes an unused symbol pedersen_hash; remove pedersen_hash from the use statement so only the actually used BoundedVec remains (i.e. change the use std::{collections::bounded_vec::BoundedVec, hash::pedersen_hash}; line to only import BoundedVec), ensuring no other references to pedersen_hash exist in this module before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/DeployAllDiscord.s.sol`:
- Line 8: The DeployAllDiscord.s.sol script imports the Twitter-specific
HonkVerifier (HonkVerifier) but should use a Discord-specific verifier for
deploying discord.zkemail.eth; update the import in DeployAllDiscord.s.sol to
point to a Discord verifier (e.g., from a new or existing
test/fixtures/discordHandleCommand/HonkVerifier.sol) or create the missing
discordHandleCommand verifier circuit and target/HonkVerifier.sol before
importing it; follow the pattern used in DeployAllReddit.s.sol to locate the
correct file and ensure the deployed verifier symbol HonkVerifier matches the
one referenced by the deploy script.
In `@test/fixtures/linkHandleCommand/compile.sh`:
- Around line 54-58: The script uses set -e which causes commands like nargo
compile >> "$LOG_FILE" 2>&1 to exit the shell immediately on failure so the
subsequent if [ $? -ne 0 ] checks never run; change those blocks to use the form
if ! nargo compile >> "$LOG_FILE" 2>&1; then ... fi (or temporarily disable set
-e around the command with set +e / set -e) so the custom log "Failed to compile
circuit" (and the similar checks around the other nargo/compile steps referenced
in the comment) are reached; update the three occurrences (the nargo compile
invocation and the two analogous steps at the later blocks) replacing the $?
pattern with if ! command constructs or local set +e around the command.
In `@test/fixtures/linkHandleCommand/twitter/src/sender_domain_regex.nr`:
- Around line 140-166: Fix the two comment typos inside fn check_accept_state:
change "aceppt state" to "accept state" in the function docblock and change
"asserted_match)length" to "asserted_match_length" in the inline comment that
describes haystack_index vs asserted_match_length; leave all code and variable
names (next_state, haystack_index, asserted_match_length, accept_state_reached,
accept_state_reached_bool, asserted_path_traversed) unchanged.
In `@test/fixtures/linkHandleCommand/twitter/src/x_handle_regex.nr`:
- Around line 99-101: Fix the typo in the inline comment near the calculation of
asserted_path_traversed: change the stray "asserted_match)length" to
"asserted_match_length" so the comment correctly references the
asserted_match_length variable used in the expression (asserted_match_length -
haystack_index == 1); ensure the comment reads "should equal 1 since
haystack_index should be 1 less than asserted_match_length".
- Line 79: Fix the typo in the comment string in x_handle_regex.nr where "aceppt
state" should read "accept state"; update the comment near the phrase
"Constrains the recognition of accept_state being reached" (search for "aceppt
state" or "accept_state" in that file) so the word is spelled "accept" and keep
the surrounding wording unchanged.
---
Outside diff comments:
In `@test/fixtures/handleCommand/HandleCommandTestFixture.sol`:
- Around line 102-103: The inline comment above the decode is stale—update the
comment to match the actual decode size: change the text that says
"bytes32[154]" to "bytes32[155]" so it reflects the decoding into (bytes32[155]
memory publicInputsFixed) from publicInputsFieldsData (referencing
publicInputsFixed and abi.decode(publicInputsFieldsData, (bytes32[155]))).
---
Duplicate comments:
In `@script/DeployHandleRegistrar.s.sol`:
- Line 7: The deployment script imports a test fixture (HonkVerifier) from
test/fixtures which is inappropriate for production deployment; replace that
import by referencing the real contract source or an interface located in the
contracts/ or src/ directory (e.g., add a HonkVerifier contract or IVerifier
interface under your contracts/ folder and import that instead), or load the
deployed artifact/ABI at runtime rather than importing from test/fixtures;
update the import statement referencing HonkVerifier in
DeployHandleRegistrar.s.sol and adjust any constructor/typing usage to the new
production contract/interface symbol.
In `@script/DeployLinkHandleEntrypoint/DeployLinkHandleEntrypointTwitter.s.sol`:
- Line 5: This deployment script imports a test fixture (HonkVerifier from
test/fixtures) which should not be used in production deployment scripts;
replace the test-fixture import with the proper production contract or artifact
(e.g., the canonical HonkVerifier contract from the contracts/src or built
artifact path used by other deploy scripts) and update any references in
DeployLinkHandleEntrypointTwitter.s.sol that use HonkVerifier so they reference
the production contract name/location instead of the test fixture.
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 42-46: The current pattern in prove.sh uses `set -euo pipefail`
and then checks `$?` after running `nargo execute "$WITNESS_NAME" >> "$LOG_FILE"
2>&1`, which is dead code; replace each occurrence (the `nargo execute` block
and the similar blocks at the other two spots) with the explicit conditional
form `if ! nargo execute "$WITNESS_NAME" >> "$LOG_FILE" 2>&1; then` followed by
the existing `log "Failed to execute circuit"` and `exit 1` handling (same fix
applied in compile.sh), so the command failure is detected without relying on
`$?` after `set -e`.
In `@test/src/entrypoints/HandleRegistrar/_HandleRegistrarTest.sol`:
- Line 7: The test imports and uses the Twitter link-handle verifier
HonkVerifier for the ClaimHandleCommandVerifier test (HonkVerifier is referenced
in this test and triggers the same root-cause as in
ClaimHandleCommandVerifier/IsValid.t.sol); replace the HonkVerifier import and
any instantiation in _HandleRegistrarTest.sol with the correct verifier or a
focused mock that implements the same interface expected by
ClaimHandleCommandVerifier (e.g., import the proper LinkHandle verifier used by
ClaimHandleCommandVerifier or add a test-only mock that replicates its verify
behavior), and update the test references where HonkVerifier is used so the test
exercises ClaimHandleCommandVerifier rather than reusing the Honk-specific
implementation.
In `@test/src/resolvers/HandleResolver/ResolverRegistrarIntegration.t.sol`:
- Line 14: The test imports a verifier that likely doesn't match the circuit
artifacts (HonkVerifier in ResolverRegistrarIntegration.t.sol), duplicating the
same circuit-compatibility problem noted in
ClaimHandleCommandVerifier/IsValid.t.sol; fix it by replacing the import with
the correct, compiled verifier artifact that matches the circuit used in this
test (or recompile/regenerate the circuit artifacts so HonkVerifier matches),
ensuring the symbol HonkVerifier used in the test refers to the verifier
contract produced by the current circuit build; verify the import path points to
the up-to-date compiled target and run the circuit build before re-running the
test.
---
Nitpick comments:
In `@test/fixtures/linkHandleCommand/compile.sh`:
- Around line 43-46: The current exact-string comparison of nargo --version
output is fragile; change the check in compile.sh to test for the presence of
the expected version substring instead of matching the entire line: run nargo
--version and use grep -q (or grep -F) to check for "$NARGO_VERSION" (or a
stable prefix like "nargo version") and branch on its exit status so noirup
--version is only invoked when the version substring is missing.
In `@test/fixtures/linkHandleCommand/LinkHandleCommandTestFixture.sol`:
- Around line 69-87: The two helper functions _getProofFieldsFromBinary and
_getPublicInputsFieldsFromBinary currently use hardcoded sizes (440 and 155)
that mirror HonkVerifier's PROOF_SIZE and NUMBER_OF_PUBLIC_INPUTS; add a concise
comment above each function noting this dependency (e.g., "Depends on
HonkVerifier.PROOF_SIZE = 440" and "Depends on
HonkVerifier.NUMBER_OF_PUBLIC_INPUTS = 155") so future maintainers know the
decode sizes must be updated if the verifier constants change.
In `@test/fixtures/linkHandleCommand/prove.sh`:
- Around line 1-26: Extract the duplicated argument parsing and environment
setup from prove.sh/compile.sh/clean.sh into a new sourced script (e.g.,
_common.sh) that exports PLATFORM, SCRIPT_DIR, WORKDIR and sets CIRCUIT_NAME by
reading Nargo.toml; then replace the top-of-file blocks in prove.sh, compile.sh
and clean.sh with a single source line (source "_common.sh" or ". _common.sh")
so each script reuses the same parsing/resolution logic and error handling for
missing args, missing WORKDIR, and empty CIRCUIT_NAME.
In `@test/fixtures/linkHandleCommand/twitter/src/main.nr`:
- Line 4: The import list includes an unused symbol pedersen_hash; remove
pedersen_hash from the use statement so only the actually used BoundedVec
remains (i.e. change the use std::{collections::bounded_vec::BoundedVec,
hash::pedersen_hash}; line to only import BoundedVec), ensuring no other
references to pedersen_hash exist in this module before committing.
In `@test/fixtures/linkHandleCommand/twitter/src/x_handle_regex.nr`:
- Around line 134-139: The loop computing transitions can underflow when
match_length == 0 because it uses match_length - 1; to fix, add an explicit
guard that ensures the loop body runs only when match_length > 0 (or
early-return/skip the transition loop when match_length == 0) so the in_range
calculation and the assert involving current_states and next_states are never
evaluated with a wrapped value; modify the code around the for-loop that
references match_length, in_range, current_states, next_states, and the assert
to check match_length > 0 before entering the loop (or document and enforce the
invariant that match_length is always > 0).
In `@test/fixtures/linkHandleCommand/twitter/target/HonkVerifier.sol`:
- Around line 1-3: This file contains two conflicting pragma directives ("pragma
solidity >=0.8.21" at the top and "pragma solidity ^0.8.27" later), so remove
the duplicate and consolidate to a single pragma; locate the second occurrence
("pragma solidity ^0.8.27") and delete it (or replace the top pragma with the
desired single directive) so the file has one consistent pragma statement (e.g.,
keep "pragma solidity ^0.8.27;" or a single "pragma solidity >=0.8.21;"
according to project standard).
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol (1)
9-37: The previous double-deploy bug is fully resolved — LGTM.
honkVerifierAddressis now captured once inside the broadcast (Line 15) and reused both forLinkHandleCommandVerifierconstruction (Line 20) and the finalHONK_VERIFIER=log (Line 31). The three pure virtual hooks (_getDkimRegistryAddress,_getRecordName,_getPlatformName) are called twice each — once inside the broadcast and once in the post-broadcast log block. Because they arepure, this is functionally harmless in a Forge script context; consider caching them as locals if you want consistency with howhonkVerifierAddressis handled.♻️ Optional: cache pure-virtual return values once
vm.startBroadcast(deployerPrivateKey); + address dkimRegistryAddress = _getDkimRegistryAddress(); + string memory recordName = _getRecordName(); + string memory platformName = _getPlatformName(); + console.log("\n=== Step 0: Deploy HonkVerifier ==="); address honkVerifierAddress = _deployHonkVerifier(); console.log("HonkVerifier deployed at:", honkVerifierAddress); console.log("\n=== Step 1: Deploy LinkHandleCommandVerifier ==="); LinkHandleCommandVerifier commandVerifier = - new LinkHandleCommandVerifier(honkVerifierAddress, _getDkimRegistryAddress()); + new LinkHandleCommandVerifier(honkVerifierAddress, dkimRegistryAddress); console.log("LinkHandleCommandVerifier deployed at:", address(commandVerifier)); console.log("\n=== Step 2: Deploy LinkHandleEntrypoint ==="); LinkHandleEntrypoint entrypoint = - new LinkHandleEntrypoint(address(commandVerifier), _getRecordName(), _getPlatformName()); + new LinkHandleEntrypoint(address(commandVerifier), recordName, platformName); console.log("LinkHandleEntrypoint deployed at:", address(entrypoint)); vm.stopBroadcast(); console.log("\n=== Deployment Complete ==="); console.log("HONK_VERIFIER=", honkVerifierAddress); - console.log("DKIM_REGISTRY=", _getDkimRegistryAddress()); + console.log("DKIM_REGISTRY=", dkimRegistryAddress); console.log("LINK_HANDLE_COMMAND_VERIFIER=", address(commandVerifier)); - console.log("RECORD_NAME=", _getRecordName()); - console.log("PLATFORM_NAME=", _getPlatformName()); + console.log("RECORD_NAME=", recordName); + console.log("PLATFORM_NAME=", platformName); console.log("LINK_HANDLE_ENTRYPOINT=", address(entrypoint));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol` around lines 9 - 37, Cache the pure-virtual return values so they are called once and reused: call _getDkimRegistryAddress(), _getRecordName(), and _getPlatformName() and store their results in locals (e.g., dkimRegistryAddr, recordName, platformName) before vm.startBroadcast, then use those locals when constructing LinkHandleCommandVerifier and LinkHandleEntrypoint and when logging after vm.stopBroadcast; keep honkVerifierAddress behavior as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@script/DeployLinkHandleEntrypoint/_DeployLinkHandleEntrypoint.s.sol`:
- Around line 9-37: Cache the pure-virtual return values so they are called once
and reused: call _getDkimRegistryAddress(), _getRecordName(), and
_getPlatformName() and store their results in locals (e.g., dkimRegistryAddr,
recordName, platformName) before vm.startBroadcast, then use those locals when
constructing LinkHandleCommandVerifier and LinkHandleEntrypoint and when logging
after vm.stopBroadcast; keep honkVerifierAddress behavior as-is.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/fixtures/linkHandleCommand/twitter/src/sender_domain_regex.nr`:
- Line 52: The generated fixture contains a typo "asserted_match)length"; update
the generator that emits
test/fixtures/linkHandleCommand/twitter/src/sender_domain_regex.nr to output
"asserted_match_length" instead: find the generator/template code (e.g., the
function generate_sender_domain_regex, the template variable
SENDER_DOMAIN_REGEX_TEMPLATE, or any code that writes sender_domain_regex.nr)
and replace the incorrect token "asserted_match)length" with the correct
"asserted_match_length" so regenerated fixtures no longer include the typo.
- Line 31: The comment in the generated file contains a typo ("aceppt" should be
"accept") in the string "Constrains the recognition of accept_state being
reached. If an aceppt state is reached,"; fix this in the generator/template
that emits sender_domain_regex.nr by replacing "aceppt" with "accept" in the
template source that produces that comment, then re-run the generator to
regenerate the fixture so the generated file is corrected.
In `@test/fixtures/linkHandleCommand/twitter/src/x_handle_regex.nr`:
- Around line 51-53: Comment contains a typo "asserted_match)length" — update
the comment to "asserted_match_length" where it documents the equality check for
asserted_path_traversed; specifically edit the comment above the expression that
computes asserted_path_traversed (the variables asserted_match_length and
haystack_index are referenced) in the generator that emits x_handle_regex.nr so
future generated files include the corrected comment.
- Line 31: Fix the typo "aceppt" → "accept" in the generator that produces
x_handle_regex.nr so the generated comment reads "Constrains the recognition of
accept_state being reached." Locate the generator/template that emits the file
x_handle_regex.nr and update the string containing "Constrains the recognition
of accept_state being reached. If an aceppt state is reached," to correct
"aceppt" to "accept" (ensure any surrounding templates or i18n keys are updated
accordingly).
benceharomi
force-pushed
the
generic-platform-handling
branch
from
February 20, 2026 19:33
41d0c20 to
87ec168
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/fixtures/linkHandleCommand/compile.sh (1)
43-43: Usegrep -F(literal match) for the version string.
$NARGO_VERSION(1.0.0-beta.5) contains dots, which are regex metacharacters ingrepand will match any character in that position. While a false positive is impractical here, using-Fmakes the intent explicit.♻️ Proposed fix
-if [ "$(nargo --version | grep "nargo version = $NARGO_VERSION")" != "nargo version = $NARGO_VERSION" ]; then +if [ "$(nargo --version | grep -F "nargo version = $NARGO_VERSION")" != "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` at line 43, The if-condition uses grep with a regex, which treats dots in $NARGO_VERSION as metacharacters; change the call in the compile.sh snippet that runs nargo --version | grep "nargo version = $NARGO_VERSION" to use fixed-string matching (grep -F) so the literal version string is matched; update the command in the conditional that references nargo --version and $NARGO_VERSION accordingly.test/src/entrypoints/LinkHandleEntrypoint/LinkHandleEntrypoint.t.sol (1)
60-69: Consider adding a parallel test forRECORD_NAMEmismatch in theentrypoint()path.
test_entrypoint_revertsWhenPlatformNameMismatchcovers thePLATFORM_NAMEaxis well. If the implementation also validatesRECORD_NAMEinsideentrypoint()(not justverifyTextRecord), a matching test would be valuable to prevent regressions on that guard.Also,
discordEntrypoint.encode(...)on line 66 is fine (encode is platform-agnostic), butlinkHandle.encode(...)would be equally correct and might be slightly clearer about intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/entrypoints/LinkHandleEntrypoint/LinkHandleEntrypoint.t.sol` around lines 60 - 69, Add a new test mirroring test_entrypoint_revertsWhenPlatformNameMismatch that asserts entrypoint() reverts when the RECORD_NAME in the entrypoint helper does not match the command's recordName: construct a command via LinkHandleCommandTestFixture.getTwitterFixture(), deploy a LinkHandleEntrypointHelper with a different RECORD_NAME (e.g., "bio" vs fixture's value) and same PLATFORM_NAME, call encode(...) on that helper (or use linkHandle.encode(...) if you prefer clarity), vm.expectRevert(abi.encodeWithSelector(LinkTextRecordEntrypoint.InvalidCommand.selector)), then call entrypoint(encodedCommand) to verify the guard; place the new test alongside the existing test_entrypoint_revertsWhenPlatformNameMismatch.
🤖 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/compile.sh`:
- Around line 45-50: The script currently runs noirup and bbup directly (noirup
--version $NARGO_VERSION >> "$LOG_FILE" 2>&1 and bbup --version $BB_VERSION >>
"$LOG_FILE" 2>&1) which can fail silently; change each to follow the existing
pattern used in Steps 1–3 by wrapping the call in an if ! ...; then call log
with a clear failure message including the tool and version (e.g., "Failed to
run noirup --version $NARGO_VERSION") and exit 1 on failure. Use the same
variables ($NARGO_VERSION, $BB_VERSION, $LOG_FILE) and the script's log function
to keep behavior consistent with the other checks.
---
Nitpick comments:
In `@test/fixtures/linkHandleCommand/compile.sh`:
- Line 43: The if-condition uses grep with a regex, which treats dots in
$NARGO_VERSION as metacharacters; change the call in the compile.sh snippet that
runs nargo --version | grep "nargo version = $NARGO_VERSION" to use fixed-string
matching (grep -F) so the literal version string is matched; update the command
in the conditional that references nargo --version and $NARGO_VERSION
accordingly.
In `@test/src/entrypoints/LinkHandleEntrypoint/LinkHandleEntrypoint.t.sol`:
- Around line 60-69: Add a new test mirroring
test_entrypoint_revertsWhenPlatformNameMismatch that asserts entrypoint()
reverts when the RECORD_NAME in the entrypoint helper does not match the
command's recordName: construct a command via
LinkHandleCommandTestFixture.getTwitterFixture(), deploy a
LinkHandleEntrypointHelper with a different RECORD_NAME (e.g., "bio" vs
fixture's value) and same PLATFORM_NAME, call encode(...) on that helper (or use
linkHandle.encode(...) if you prefer clarity),
vm.expectRevert(abi.encodeWithSelector(LinkTextRecordEntrypoint.InvalidCommand.selector)),
then call entrypoint(encodedCommand) to verify the guard; place the new test
alongside the existing test_entrypoint_revertsWhenPlatformNameMismatch.
benceharomi
force-pushed
the
generic-platform-handling
branch
from
February 20, 2026 19:54
053d3a5 to
53db751
Compare
Member
Author
|
codecov is messed up, needed to open a new pr |
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
1 participant
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