feat: mask vote utilities#924
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughRefactors CRISP SDK vote APIs to accept a consolidated parameter object and adds generateMaskVote. ECDSA circuit verify_signature now returns Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant SDK as SDK (vote.ts)
participant ZK as ZK Input Generator
participant Circuit as CRISP Circuit
Note right of Test: Normal flow — encrypt vote
Test->>SDK: encryptVoteAndGenerateCRISPInputs({encodedVote, publicKey, previousCiphertext, signature, message, merkleData, balance})
SDK->>ZK: generate initial CRISP inputs (vote encoding)
ZK-->>SDK: CRISPCircuitInputs
SDK->>SDK: extract hashed_message, public_key_x/y, signature\npopulate merkle_proof_* and balance
SDK-->>Test: CRISPCircuitInputs (complete)
Note right of Test: Masking flow — create zero vote
Test->>SDK: generateMaskVote(publicKey, previousCiphertext, bfvParams?, merkleRoot)
SDK->>ZK: generate CRISP inputs for zero vote
ZK-->>SDK: CRISPCircuitInputs
SDK->>SDK: fill zero-values for signature/pubkey/hashed_message\nset merkle_root to provided value
SDK-->>Test: CRISPCircuitInputs (masked)
Note right of Test: Circuit verification
Test->>Circuit: verify_signature(hashed_message, pub_key_x, pub_key_y, signature)
Circuit-->>Test: bool (is_signature_valid)
Test->>Circuit: (if is_signature_valid) proceed with merkle_root == merkle_root_calculated check
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/CRISP/circuits/src/ecdsa.nr(4 hunks)examples/CRISP/circuits/src/main.nr(2 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(4 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/utils.test.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
IMerkleProof(19-19)
examples/CRISP/packages/crisp-sdk/tests/utils.test.ts (2)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleTree(27-29)examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
LEAVES(16-27)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleProof(39-80)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (7)
votingPowerLeaf(31-31)merkleProof(32-32)LEAVES(16-27)MAX_DEPTH(29-29)VOTE(14-14)SIGNATURE(12-13)MESSAGE(11-11)examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleProof(39-80)examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
encodeVote(60-86)encryptVoteAndGenerateCRISPInputs(154-188)generateMaskVote(197-228)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)examples/CRISP/packages/crisp-sdk/src/types.ts (3)
EncryptVoteAndGenerateCRISPInputsParams(208-217)CRISPCircuitInputs(148-172)IVote(81-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: rust_unit
🔇 Additional comments (16)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
205-217: LGTM! Well-structured parameter interface.The new
EncryptVoteAndGenerateCRISPInputsParamsinterface provides a clean, object-based API that improves the function signature's maintainability and extensibility compared to multiple positional parameters.examples/CRISP/circuits/src/main.nr (2)
42-43: LGTM! Captures signature verification result.Storing the boolean result from
verify_signatureenables the subsequent check at line 60, improving the circuit's security guarantees.
60-62: Excellent security improvement.The updated condition now requires both a valid Merkle proof AND a valid signature to classify as a voter, preventing unauthorized votes from attackers who might obtain valid Merkle proofs without corresponding signature authority.
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
26-44: LGTM! Centralized default cryptographic values.These constants provide standardized default values for testing and align with the test data used in the circuit's
test_verify_signature_sdk_inputtest case inexamples/CRISP/circuits/src/ecdsa.nr.examples/CRISP/packages/crisp-sdk/tests/constants.ts (2)
7-7: LGTM! Import added for Merkle proof generation.
31-32: LGTM! Precomputed Merkle proof for test reuse.Centralizing the
merkleProofconstant improves test maintainability by avoiding duplicate computation across test files.examples/CRISP/circuits/src/ecdsa.nr (4)
10-17: LGTM! Improved API design.Returning a boolean instead of asserting internally provides better flexibility, allowing callers to handle verification failures according to their context (as demonstrated in
main.nrwhere it's used in a conditional).
90-90: LGTM! Test correctly updated for boolean return.
115-115: LGTM! Test correctly updated for boolean return.
138-139: LGTM! Negative test case correctly updated.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
127-135: LGTM! Updated to use new object-based API.The refactored function signature with a single parameter object improves readability and maintainability.
146-154: LGTM! Extended assertions for new CRISP input fields.The test now properly validates the expanded circuit inputs including signature, public key, Merkle proof, and balance fields.
158-181: LGTM! New test for mask vote generation.The test properly validates that
generateMaskVoteproduces well-formed CRISP inputs with placeholder values for signature and Merkle proof data.examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
154-163: LGTM! Improved function signature with object parameter.The refactored API using destructured object parameters enhances maintainability and makes the function call sites more readable.
176-187: LGTM! Properly spreads and extends CRISP inputs.The function now correctly returns the complete CRISP circuit inputs including signature verification and Merkle proof fields.
197-228: Verify that hardcoded Merkle proof array lengths match circuit expectations for mask votes.The
generateMaskVotefunction creates placeholder circuit inputs with hardcodedmerkle_proof_indicesandmerkle_proof_siblingsarrays of length 4, whileMAX_DEPTHis 20. Themerkle_proof_lengthis hardcoded to'1'. These values appear intentional (the function generates all dummy/zero-filled data), but without circuit specifications or protocol documentation, their correctness cannot be confirmed. Tests only verify field presence, not array lengths.Please confirm:
- Is length 4 correct for mask vote Merkle proofs, or should these match
MAX_DEPTH(20)?- Should
merkle_proof_lengthbe'1'for mask votes, or should it reflect the array length?- Are there circuit constraints that define the expected structure for mask votes?
ctrlc03
force-pushed
the
feat/mask-vote
branch
from
db52260 to
41b4d7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
162-184: Strengthen balance assertion for mask vote.The test validates the
generateMaskVotefunction well, but the balance assertion on line 183 could be more specific. Since mask votes should have a balance of'0'(as per the implementation in vote.ts line 226), consider asserting the exact value instead of just checking that it's defined.Apply this diff to strengthen the assertion:
- expect(crispInputs.balance).toBeDefined() + expect(crispInputs.balance).toBe('0')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/CRISP/circuits/src/ecdsa.nr(4 hunks)examples/CRISP/circuits/src/main.nr(2 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(4 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/utils.test.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/CRISP/packages/crisp-sdk/tests/constants.ts
- examples/CRISP/circuits/src/main.nr
- examples/CRISP/packages/crisp-sdk/tests/utils.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
encryptVoteAndGenerateCRISPInputs(154-188)generateMaskVote(197-228)examples/CRISP/packages/crisp-sdk/tests/constants.ts (4)
SIGNATURE(12-13)MESSAGE(11-11)merkleProof(32-32)votingPowerLeaf(31-31)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
IMerkleProof(19-19)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)examples/CRISP/packages/crisp-sdk/src/types.ts (3)
EncryptVoteAndGenerateCRISPInputsParams(208-217)CRISPCircuitInputs(148-172)IVote(81-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: rust_unit
🔇 Additional comments (6)
examples/CRISP/circuits/src/ecdsa.nr (2)
15-17: Excellent refactor to return bool instead of asserting internally.This change improves API flexibility by allowing callers to explicitly handle signature verification failures. The approach aligns with best practices for verification functions and makes the code more composable.
90-90: Test assertions correctly updated for boolean return.All test cases properly validate the new boolean return value, covering both successful verification (lines 90, 115) and failed verification (line 139).
Also applies to: 115-115, 139-139
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
205-217: Well-designed parameter consolidation.The new
EncryptVoteAndGenerateCRISPInputsParamsinterface effectively consolidates function parameters into a single object, improving API ergonomics and maintainability. The use of typed hex strings forsignatureand optionalbfvParamsdemonstrates good TypeScript practices.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
131-159: Test correctly updated for new parameter object API.The test properly validates the refactored
encryptVoteAndGenerateCRISPInputsfunction with comprehensive assertions for all new fields including signature components, merkle proof data, and balance.examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
154-188: Clean refactor with proper parameter validation.The refactored function effectively uses the parameter object pattern and maintains proper validation. The extraction and transformation of signature and merkle proof data is correct, and all fields are appropriately converted to the required string format for circuit inputs.
197-214: Good implementation of mask vote generation.The function correctly generates a zero vote for masking purposes, properly encoding it with zero voting power and generating the corresponding ZK inputs.
ctrlc03
force-pushed
the
feat/mask-vote
branch
from
41b4d7a to
5abb27b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
218-226: Fix merkle_proof_length inconsistency and consider extracting magic numbers.Two issues with the placeholder field initialization:
Line 224:
merkle_proof_lengthis set to'1'whilemerkle_proof_indicesandmerkle_proof_siblingsare arrays of length 4. This inconsistency was flagged in a previous review and remains unresolved.Lines 218-223: The hardcoded array lengths (32, 64, 4) are magic numbers that should ideally be extracted as named constants for maintainability.
Apply this diff to fix the inconsistency:
public_key_x: Array.from({ length: 32 }, () => '0'), public_key_y: Array.from({ length: 32 }, () => '0'), signature: Array.from({ length: 64 }, () => '0'), hashed_message: Array.from({ length: 32 }, () => '0'), merkle_proof_indices: Array.from({ length: 4 }, () => '0'), merkle_proof_siblings: Array.from({ length: 4 }, () => '0'), - merkle_proof_length: '1', + merkle_proof_length: '4', merkle_root: merkleRoot.toString(), balance: '0',Consider also extracting the array lengths as constants:
const ECDSA_PUBKEY_COMPONENT_LENGTH = 32 const ECDSA_SIGNATURE_LENGTH = 64 const MERKLE_PROOF_PLACEHOLDER_LENGTH = 4
🧹 Nitpick comments (3)
examples/CRISP/packages/crisp-sdk/tests/utils.test.ts (1)
38-41: Unpad indices alongside siblings to keep proof arrays alignedSlice indices to the same length as siblings to avoid verifier assumptions about equal lengths.
Apply:
- const unpaddedProof = { - ...proof.proof, - siblings: proof.proof.siblings.slice(0, proof.length), - } + const unpaddedProof = { + ...proof.proof, + indices: proof.proof.indices.slice(0, proof.length), + siblings: proof.proof.siblings.slice(0, proof.length), + }Optionally assert lengths before verify:
expect(unpaddedProof.siblings.length).toBe(proof.length) expect(unpaddedProof.indices.length).toBe(proof.length)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
162-185: Consider asserting specific placeholder values in mask vote test.The test validates that all fields are defined but doesn't check the specific placeholder values. For better test coverage, consider asserting that placeholder fields contain expected values (e.g.,
balanceshould be'0', signature fields should be arrays of'0').Apply this diff to add more specific assertions:
expect(crispInputs.merkle_proof_siblings).toBeDefined() expect(crispInputs.merkle_proof_length).toBeDefined() expect(crispInputs.merkle_root).toBeDefined() - expect(crispInputs.balance).toBeDefined() + expect(crispInputs.balance).toBe('0') + expect(crispInputs.public_key_x).toEqual(Array.from({ length: 32 }, () => '0')) + expect(crispInputs.public_key_y).toEqual(Array.from({ length: 32 }, () => '0')) + expect(crispInputs.signature).toEqual(Array.from({ length: 64 }, () => '0')) + expect(crispInputs.hashed_message).toEqual(Array.from({ length: 32 }, () => '0'))examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
197-228: Consider adding inline comments explaining the masking strategy.The
generateMaskVotefunction creates a vote with zero values and placeholder signature/merkle fields, but it's not immediately clear from the code why these specific placeholder values are used or how mask votes integrate with the voting protocol. Adding a brief inline comment explaining the masking strategy would improve code comprehension.For example:
export const generateMaskVote = async ( publicKey: Uint8Array, previousCiphertext: Uint8Array, bfvParams = DEFAULT_BFV_PARAMS, merkleRoot: bigint, ): Promise<CRISPCircuitInputs> => { // Create a zero vote for masking purposes. Mask votes allow users to // participate in the voting protocol without revealing their actual vote, // using placeholder values for signature verification fields. const plaintextVote: IVote = { yes: 0n, no: 0n, } // ... rest of implementation }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/CRISP/circuits/src/ecdsa.nr(4 hunks)examples/CRISP/circuits/src/main.nr(2 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(4 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/utils.test.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/CRISP/circuits/src/ecdsa.nr
- examples/CRISP/circuits/src/main.nr
- examples/CRISP/packages/crisp-sdk/tests/constants.ts
- examples/CRISP/packages/crisp-sdk/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (3)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
encryptVoteAndGenerateCRISPInputs(154-188)generateMaskVote(197-228)examples/CRISP/packages/crisp-sdk/tests/constants.ts (4)
SIGNATURE(12-13)MESSAGE(11-11)merkleProof(32-32)votingPowerLeaf(31-31)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)
examples/CRISP/packages/crisp-sdk/tests/utils.test.ts (2)
examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
generateMerkleTree(27-29)examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
LEAVES(16-27)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)examples/CRISP/packages/crisp-sdk/src/types.ts (3)
EncryptVoteAndGenerateCRISPInputsParams(208-217)CRISPCircuitInputs(148-172)IVote(81-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: test_contracts
🔇 Additional comments (5)
examples/CRISP/packages/crisp-sdk/tests/utils.test.ts (2)
30-30: LGTM: deterministic tree setupTree generation from fixed LEAVES looks good for a stable test fixture.
43-43: LGTM: explicit boolean verificationAssert against true is precise here; no change needed.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
15-15: LGTM: Import updates align with API refactoring.The import changes correctly reflect the updated API surface, importing
generateMaskVoteand consolidating test constants from a shared location.Also applies to: 19-19, 21-21
131-158: LGTM: Test correctly validates updated API and expanded output shape.The test properly exercises the refactored
encryptVoteAndGenerateCRISPInputswith the new parameter object and validates all expanded public input fields including signature, merkle proof, and balance.examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
154-188: LGTM: Breaking API change properly handled.The refactoring to accept a single parameter object has been completed successfully. There is only one caller in the codebase (the test at
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts:131), and it has been correctly updated to use the new signature with all required fields:encodedVote,publicKey,previousCiphertext,signature,message,merkleData, andbalance.
2 participants
fix #923
Summary by CodeRabbit
New Features
Refactor
Tests