feat: signature generation and parsing#914
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds signature extraction and wiring into the CRISP SDK and client: new signature utilities and types, a CRISP input generator that embeds signature fields, client-side message signing integration during vote casting, tests for extraction and input generation, one Noir test, and a minor formatting tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientHook as useVoteCasting
participant Signer as SignMessage (wagmi)
participant SDK_Sign as extractSignature()
participant SDK_Gen as generateCRISPInputs()
participant Encrypt as encryptVoteAndGenerateCRISPInputs
participant Network as Broadcast
User->>ClientHook: start castVote(...)
ClientHook->>Signer: signMessageAsync(message)
Signer-->>ClientHook: signedMessage (0x...)
ClientHook->>SDK_Sign: extractSignature(message, signedMessage)
SDK_Sign-->>ClientHook: NoirSignatureInputs (hashed_message, pub_key_x, pub_key_y, signature)
ClientHook->>SDK_Gen: generateCRISPInputs(partialInputs, signedMessage, message)
SDK_Gen-->>ClientHook: CRISPCircuitInputs (includes signature fields)
ClientHook->>Encrypt: encryptVoteAndGenerateCRISPInputs(inputs)
Encrypt-->>ClientHook: encryptedVote
ClientHook->>Network: broadcast(encryptedVote)
Network-->>User: confirmation
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 |
ctrlc03
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
examples/CRISP/packages/crisp-sdk/tests/signature.test.ts (1)
12-23: Consider enhancing test coverage.The test validates type correctness but not extraction logic. Consider adding assertions for:
- Byte array lengths (32 bytes for keys/hash, 64 for signature)
- Specific extracted values matching expected outputs from the test signature
- Edge cases (invalid signatures, malformed inputs)
Example enhancement:
expect(hashed_message).toHaveLength(32) expect(pub_key_x).toHaveLength(32) expect(pub_key_y).toHaveLength(32) expect(extractedSignature).toHaveLength(64) // Validate against known values expect(Array.from(hashed_message)).toEqual([ 200, 232, 98, 162, 80, 131, 242, 57, 252, 76, 226, 45, 127, 206, 207, 39, 206, 44, 211, 171, 113, 67, 121, 68, 78, 253, 202, 79, 29, 128, 130, 76 ])examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
149-169: Consider more comprehensive assertions.The test validates structure but not content. Consider adding assertions to verify:
- Arrays are non-empty
- Signature-related fields contain expected data types (strings representing numbers)
- Arrays have reasonable lengths (e.g.,
hashed_messageshould have 32 bytes = 32 elements)Example enhancement:
expect(crispInputs.hashed_message).toBeInstanceOf(Array) +expect(crispInputs.hashed_message.length).toBeGreaterThan(0) +expect(crispInputs.hashed_message.every(item => typeof item === 'string')).toBe(true)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/CRISP/circuits/src/ecdsa.nr(1 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts(3 hunks)examples/CRISP/client/src/utils/vote.ts(0 hunks)examples/CRISP/packages/crisp-sdk/src/index.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/signature.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(3 hunks)examples/CRISP/packages/crisp-sdk/tests/signature.test.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(3 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/client/src/utils/vote.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:49:48.617Z
Learnt from: 0xjei
PR: gnosisguild/enclave#648
File: examples/CRISP/circuits/src/main.nr:40-44
Timestamp: 2025-08-27T13:49:48.617Z
Learning: In CRISP circuits using Greco library, the binary check `assert(0 == b * (qmt - b))` for polynomial k1 should not be applied to all coefficients - only specific coefficients require this constraint, such as `k1.coefficients[2048 - 1]`.
Applied to files:
examples/CRISP/circuits/src/utils.nr
🧬 Code graph analysis (5)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
NoirSignatureInputs(15-15)
examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
NoirSignatureInputs(183-200)
examples/CRISP/packages/crisp-sdk/tests/signature.test.ts (1)
examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
CRISPCircuitInputs(146-169)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
encodeVote(60-86)encryptVoteAndGenerateCRISPInputs(156-185)generateCRISPInputs(195-209)
🔇 Additional comments (8)
examples/CRISP/circuits/src/ecdsa.nr (1)
95-118: LGTM!The new test validates signature verification with SDK-generated inputs and follows the same pattern as the existing
test_verify_signature.examples/CRISP/circuits/src/utils.nr (1)
10-31: LGTM!The formatting changes improve readability without altering the logic.
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
180-200: LGTM!The
NoirSignatureInputsinterface is well-defined and clearly documents the signature components needed for Noir circuit verification.examples/CRISP/packages/crisp-sdk/src/index.ts (1)
11-15: LGTM!The exports correctly expose the new signature functionality and types through the public API.
examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
17-44: LGTM!The signature extraction logic correctly:
- Hashes the message using Ethereum's message hashing convention
- Recovers the uncompressed public key and extracts x/y coordinates (skipping the 0x04 prefix)
- Extracts r and s components from the signature (excluding the v recovery byte)
- Returns properly typed
NoirSignatureInputsfor Noir circuit verificationexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
10-17: LGTM!The import additions correctly include
generateCRISPInputs, which is needed for the new test coverage.
44-44: LGTM!Good type consistency improvement. Using
BigInt64Arrayaligns with the type expectations and usage elsewhere in the test file.
22-31: Original concerns are partially addressed; test isolation is a future-proofing suggestion rather than current issue.
Signature validity: The hardcoded signature is validated in
signature.test.ts(lines 13-17) via theextractSignature()function, which verifies the signature components.Test isolation: While the current tests don't modify the mutable
zkInputsGeneratorfixture (all operations are read-only), usingbeforeEachwould be better practice to prevent accidental state pollution if future tests modify these objects. This is not a current issue but a preventive measure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/tests/constants.ts (1)
9-12: LGTM! Test constants are well-structured.The new test constants follow appropriate formats:
SIGNATUREmatches the Ethereum signature format (65 bytes)VOTEcorrectly uses BigInt literalsMESSAGEprovides a clear test stringConsider adding JSDoc comments to document the purpose and relationship of these constants:
+/** + * Test message for signature verification + */ export const MESSAGE = 'Vote for round 0' +/** + * Valid ECDSA signature for MESSAGE + */ export const SIGNATURE = '0x1641431d0ed3fd86814f026da62e11434b53c6a85162fea7f99218bf3c307dec7f361c235f07b658780afd91a5c9d68d6a4b14d5eb0511f6688d3e91140eec121b' +/** + * Test vote tally + */ export const VOTE = { yes: 10n, no: 0n }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/packages/crisp-sdk/src/index.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/signature.test.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
- examples/CRISP/packages/crisp-sdk/tests/signature.test.ts
⏰ 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_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: test_contracts
🔇 Additional comments (2)
examples/CRISP/packages/crisp-sdk/src/index.ts (2)
11-12: LGTM! New module exports follow established patterns.The barrel exports for
voteandsignaturemodules maintain consistency with the existing export structure in the SDK.
15-23: LGTM! Type exports properly extended.The addition of
NoirSignatureInputsto the type exports is correct, and the multi-line formatting improves readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
193-198: Consider clearer parameter names to matchextractSignatureconvention.The parameter
signature: \0x${string}`actually holds the hex-encoded signed message (whatextractSignaturecallssignedMessage`), which can confuse readers. While the function call on line 198 is logically correct, renaming would improve clarity.Consider this diff:
export const generateCRISPInputs = async ( partialInputs: CRISPCircuitInputs, - signature: `0x${string}`, + signedMessage: `0x${string}`, message: string, ): Promise<CRISPCircuitInputs> => { - const { hashed_message, pub_key_x, pub_key_y, signature: extractedSignature } = await extractSignature(message, signature) + const { hashed_message, pub_key_x, pub_key_y, signature: extractedSignature } = await extractSignature(message, signedMessage)Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts(4 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
CRISPCircuitInputs(146-169)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
⏰ 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_unit
- GitHub Check: rust_integration
🔇 Additional comments (2)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (1)
9-9: LGTM! Signature scaffolding correctly integrated.The wagmi
useSignMessagehook is properly imported, extracted, and included in the dependency array. The commented-out signature generation on lines 54-55 is appropriately marked as scaffolding for future integration.Also applies to: 26-26, 54-55, 123-124
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
200-207: LGTM! Signature extraction and type conversion implemented correctly.The function properly converts the extracted signature components from byte arrays to string arrays and merges them into the CRISP circuit inputs. The spread operator usage and type conversions are appropriate.
ctrlc03
force-pushed
the
feat/signature-generation-and-parsing
branch
from
7d46bb3 to
a52573c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
185-207: Consider adding error handling for signature extraction failures.The function calls
extractSignaturewithout handling potential failures during public key recovery or signature parsing. Consider wrapping the extraction in a try-catch block or documenting the expected error behavior for invalid signatures.Example:
export const generateCRISPInputs = async ( partialInputs: CRISPCircuitInputs, signature: `0x${string}`, message: string, ): Promise<CRISPCircuitInputs> => { + try { const { hashed_message, pub_key_x, pub_key_y, signature: extractedSignature } = await extractSignature(message, signature) return { ...partialInputs, hashed_message: Array.from(hashed_message).map((b) => b.toString()), public_key_x: Array.from(pub_key_x).map((b) => b.toString()), public_key_y: Array.from(pub_key_y).map((b) => b.toString()), signature: Array.from(extractedSignature).map((b) => b.toString()), } + } catch (error) { + throw new Error(`Failed to extract signature: ${error instanceof Error ? error.message : 'Unknown error'}`) + } }examples/CRISP/packages/crisp-sdk/src/signature.ts (2)
17-44: Consider renaming the parameter for clarity.The parameter
signedMessageis misleading—it contains the hex-encoded signature, not a "signed message" (which typically means message + signature together). Consider renaming it tosignatureorsignatureHexfor clarity.-export const extractSignature = async (message: string, signedMessage: `0x${string}`): Promise<NoirSignatureInputs> => { +export const extractSignature = async (message: string, signature: `0x${string}`): Promise<NoirSignatureInputs> => { const messageHash = hashMessage(message) const messageBytes = hexToBytes(messageHash) const publicKey = await recoverPublicKey({ hash: messageHash, - signature: signedMessage, + signature: signature, }) const publicKeyBytes = hexToBytes(publicKey) const publicKeyX = publicKeyBytes.slice(1, 33) const publicKeyY = publicKeyBytes.slice(33, 65) // Extract r and s from signature (remove v) - const sigBytes = hexToBytes(signedMessage) + const sigBytes = hexToBytes(signature) const r = sigBytes.slice(0, 32) // First 32 bytes const s = sigBytes.slice(32, 64) // Next 32 bytes const signatureBytes = new Uint8Array(64) signatureBytes.set(r, 0) signatureBytes.set(s, 32) return { hashed_message: messageBytes, pub_key_x: publicKeyX, pub_key_y: publicKeyY, signature: signatureBytes, } }
17-44: Add input validation for signature and public key lengths.The function assumes the signature is 65 bytes and the recovered public key is 65 bytes (uncompressed format). Invalid inputs could cause silent failures or incorrect slice operations.
export const extractSignature = async (message: string, signedMessage: `0x${string}`): Promise<NoirSignatureInputs> => { const messageHash = hashMessage(message) const messageBytes = hexToBytes(messageHash) const publicKey = await recoverPublicKey({ hash: messageHash, signature: signedMessage, }) const publicKeyBytes = hexToBytes(publicKey) + if (publicKeyBytes.length !== 65) { + throw new Error(`Invalid public key length: expected 65 bytes, got ${publicKeyBytes.length}`) + } + const publicKeyX = publicKeyBytes.slice(1, 33) const publicKeyY = publicKeyBytes.slice(33, 65) // Extract r and s from signature (remove v) const sigBytes = hexToBytes(signedMessage) + if (sigBytes.length !== 65) { + throw new Error(`Invalid signature length: expected 65 bytes, got ${sigBytes.length}`) + } + const r = sigBytes.slice(0, 32) // First 32 bytes const s = sigBytes.slice(32, 64) // Next 32 bytes const signatureBytes = new Uint8Array(64) signatureBytes.set(r, 0) signatureBytes.set(s, 32) return { hashed_message: messageBytes, pub_key_x: publicKeyX, pub_key_y: publicKeyY, signature: signatureBytes, } }examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
145-165: Consider adding more detailed assertions.The test only verifies that the fields are arrays/objects but doesn't check their actual contents or lengths. Consider adding assertions to verify:
- Array lengths match expected signature component sizes (32 bytes for coordinates, 64 for signature, 32 for hash)
- Arrays contain valid numeric strings
- The signature fields are non-empty
Example additions:
const crispInputs = await generateCRISPInputs(partialInputs, SIGNATURE, MESSAGE) expect(crispInputs.ct_add).toBeInstanceOf(Object) expect(crispInputs.params).toBeInstanceOf(Object) expect(crispInputs.ct0is).toBeInstanceOf(Array) expect(crispInputs.ct1is).toBeInstanceOf(Array) expect(crispInputs.pk0is).toBeInstanceOf(Array) expect(crispInputs.pk1is).toBeInstanceOf(Array) expect(crispInputs.r1is).toBeInstanceOf(Array) expect(crispInputs.r2is).toBeInstanceOf(Array) expect(crispInputs.p1is).toBeInstanceOf(Array) expect(crispInputs.hashed_message).toBeInstanceOf(Array) + expect(crispInputs.hashed_message).toHaveLength(32) + expect(crispInputs.hashed_message.every(b => typeof b === 'string')).toBe(true) expect(crispInputs.public_key_x).toBeInstanceOf(Array) + expect(crispInputs.public_key_x).toHaveLength(32) expect(crispInputs.public_key_y).toBeInstanceOf(Array) + expect(crispInputs.public_key_y).toHaveLength(32) expect(crispInputs.signature).toBeInstanceOf(Array) + expect(crispInputs.signature).toHaveLength(64) })
145-165: Consider adding error case testing.The test suite doesn't cover error scenarios such as invalid signatures or mismatched message/signature pairs. Adding negative test cases would improve robustness.
Example test:
it('should throw an error for invalid signature', async () => { const encodedVote = encodeVote(VOTE, VotingMode.GOVERNANCE, votingPower) const partialInputs = await encryptVoteAndGenerateCRISPInputs(encodedVote, publicKey, previousCiphertext) const invalidSignature = '0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' await expect( generateCRISPInputs(partialInputs, invalidSignature, MESSAGE) ).rejects.toThrow() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/CRISP/circuits/src/ecdsa.nr(1 hunks)examples/CRISP/circuits/src/utils.nr(2 hunks)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts(4 hunks)examples/CRISP/client/src/utils/vote.ts(0 hunks)examples/CRISP/packages/crisp-sdk/src/index.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/signature.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(2 hunks)examples/CRISP/packages/crisp-sdk/tests/constants.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/signature.test.ts(1 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(3 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/client/src/utils/vote.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/CRISP/packages/crisp-sdk/tests/constants.ts
- examples/CRISP/packages/crisp-sdk/src/index.ts
- examples/CRISP/client/src/hooks/voting/useVoteCasting.ts
- examples/CRISP/packages/crisp-sdk/src/types.ts
- examples/CRISP/packages/crisp-sdk/tests/signature.test.ts
- examples/CRISP/circuits/src/ecdsa.nr
- examples/CRISP/circuits/src/utils.nr
🧰 Additional context used
🧬 Code graph analysis (3)
examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
NoirSignatureInputs(183-200)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
CRISPCircuitInputs(146-169)examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
extractSignature(17-45)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (4)
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
encodeVote(60-86)encryptVoteAndGenerateCRISPInputs(154-183)generateCRISPInputs(193-207)examples/CRISP/packages/crisp-sdk/tests/constants.ts (3)
VOTE(12-12)SIGNATURE(10-11)MESSAGE(9-9)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)examples/CRISP/packages/crisp-sdk/src/types.ts (1)
BFVParams(174-178)
⏰ 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: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: rust_unit
🔇 Additional comments (4)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
11-11: LGTM!The import is correctly added to support the new signature extraction functionality.
examples/CRISP/packages/crisp-sdk/src/signature.ts (1)
7-9: LGTM!The imports are appropriate for signature extraction functionality. Using well-established viem utilities for cryptographic operations is a good practice.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
10-21: LGTM!Good practice to extract test constants into a separate file. This improves maintainability and reduces duplication across tests.
23-28: LGTM!Good refactoring to move shared test setup to the top level. The use of
BigInt64Arrayis correct and matches the expected API signature.
2 participants
fix #913
Summary by CodeRabbit
New Features
Tests
Chores