feat: allow to encode multiple options in CRISP votes [skip-line-limit]#1262
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughGeneralizes CRISP from binary to multi-option voting: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as SDK/Client
participant Worker as SDK Worker
participant Circuit as Noir Circuit
participant Contract as CRISP Contract
participant Tally as Tally Decoder
User->>Client: select option(s) & balance
Client->>Worker: generateMaskVoteProof / generateVoteProof (vote: bigint[], numOptions)
Worker->>Circuit: prove(encoded_vote, num_options)
Circuit->>Circuit: validate per-option segments & bounds
Circuit-->>Worker: proof + ciphertext
Worker->>Contract: submit proof + ciphertext + numOptions
Contract->>Tally: decodeTally(e3Id)
Tally->>Tally: split plaintext into segments by numOptions and MAX_VOTE_BITS
Tally-->>Contract: per-option counts array
Contract-->>User: result/tally
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
369-372:⚠️ Potential issue | 🔴 CriticalUpdate ciphertext commitment index after adding
num_options.
Withnum_optionsnow a public input in the circuit, the public inputs array structure has shifted. Currently,publicInputs[5]points tonum_options(a u32); the actual ciphertext commitment (the circuit's return value) is now atpublicInputs[6].Fix
- const encryptedVoteCommitment = publicInputs[5] as `0x${string}` + const encryptedVoteCommitment = publicInputs[6] as `0x${string}`
🤖 Fix all issues with AI agents
In `@examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol`:
- Around line 175-178: Add a defensive validation to ensure numOptions is not
zero before computing segmentSize: check the value retrieved from
e3Data[e3Id].numOptions (and optionally that tally.length is divisible by
numOptions) and revert with a clear message if invalid; place this guard just
after reading numOptions and decoding tally in the same scope (where numOptions,
e3Data, e3Id, tally, and segmentSize are used) so the division by zero cannot
occur.
In `@examples/CRISP/packages/crisp-sdk/src/utils.ts`:
- Around line 161-166: The getMaxVoteValue function does not validate numChoices
and can produce NaN/Infinity when numChoices <= 0; update getMaxVoteValue to
validate that numChoices is a positive integer (e.g., numChoices > 0 and
Number.isInteger(numChoices)) before calling
ZKInputsGenerator.withDefaults().getBFVParams() and computing segmentSize, and
if invalid throw a clear RangeError (or return a safe default) so segmentSize,
effectiveBits, and the bigint calculation cannot produce invalid results.
🧹 Nitpick comments (3)
examples/CRISP/server/src/cli/commands.rs (1)
102-105: Hardcodednum_optionslimits configurability.The
num_optionsis hardcoded to2, which doesn't allow CLI users to configure the number of voting options. The PR objective mentions "configurable number of options" - consider adding it as a function parameter or CLI input toinitialize_crisp_round.If hardcoding is intentional for demo purposes, this is acceptable, but consider adding a comment explaining the choice.
💡 Suggested approach to make num_options configurable
pub async fn initialize_crisp_round( token_address: &str, balance_threshold: &str, + num_options: u64, ) -> Result<u64, Box<dyn std::error::Error + Send + Sync>> { // ... let token_address: Address = token_address.parse()?; let balance_threshold = U256::from_str_radix(&balance_threshold, 10)?; - let num_options = U256::from(2); + let num_options = U256::from(num_options);examples/CRISP/packages/crisp-sdk/src/utils.ts (1)
173-175: Add input validation fornumChoices.
getZeroVote(0)returns an empty array, and negative values would throw. Consider adding validation for consistency withgetMaxVoteValue:♻️ Proposed validation
export const getZeroVote = (numChoices: number): bigint[] => { + if (numChoices <= 0) { + throw new Error('numChoices must be a positive integer') + } return Array(numChoices).fill(0n) }examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
52-71:getE3()ignorese3Idand doesn't use thee3smapping.The function always returns a hardcoded E3 struct instead of reading from the
e3smapping populated byrequest(). This may cause issues if tests rely on retrieving specific E3 data (e.g.,plaintextOutput,committeePublicKeyset via other setters).For a more accurate mock:
♻️ Proposed fix to use the mapping
- function getE3(uint256) external view returns (E3 memory) { - return - E3({ - seed: 0, - threshold: [uint32(1), uint32(2)], - requestBlock: 0, - startWindow: [uint256(0), uint256(0)], - duration: 0, - expiration: 0, - encryptionSchemeId: bytes32(0), - e3Program: IE3Program(address(0)), - e3ProgramParams: bytes(""), - customParams: abi.encode(address(0), 0, 2), - decryptionVerifier: IDecryptionVerifier(address(0)), - committeePublicKey: committeePublicKey, - ciphertextOutput: bytes32(0), - plaintextOutput: plaintextOutput, - requester: address(0) - }); + function getE3(uint256 e3Id) external view returns (E3 memory) { + E3 storage stored = e3s[e3Id]; + // Return stored E3 with dynamic fields from contract state + return E3({ + seed: stored.seed, + threshold: stored.threshold, + requestBlock: stored.requestBlock, + startWindow: stored.startWindow, + duration: stored.duration, + expiration: stored.expiration, + encryptionSchemeId: stored.encryptionSchemeId, + e3Program: stored.e3Program, + e3ProgramParams: stored.e3ProgramParams, + customParams: stored.customParams, + decryptionVerifier: stored.decryptionVerifier, + committeePublicKey: committeePublicKey, // Use current state + ciphertextOutput: stored.ciphertextOutput, + plaintextOutput: plaintextOutput, // Use current state + requester: stored.requester + }); }
ctrlc03
force-pushed
the
feat/generic-encoding-crisp
branch
from
eb62397 to
5f7d8f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
52-71:⚠️ Potential issue | 🟡 Minor
getE3ignores the stored E3 data.The
requestfunction stores E3 data in thee3smapping (line 21-37), butgetE3ignores this stored data and always returns a hardcoded struct. This means tests usingrequest()to set up state won't get the expected values back fromgetE3().🐛 Proposed fix to use stored data
- function getE3(uint256) external view returns (E3 memory) { - return - E3({ - seed: 0, - threshold: [uint32(1), uint32(2)], - requestBlock: 0, - startWindow: [uint256(0), uint256(0)], - duration: 0, - expiration: 0, - encryptionSchemeId: bytes32(0), - e3Program: IE3Program(address(0)), - e3ProgramParams: bytes(""), - customParams: abi.encode(address(0), 0, 2), - decryptionVerifier: IDecryptionVerifier(address(0)), - committeePublicKey: committeePublicKey, - ciphertextOutput: bytes32(0), - plaintextOutput: plaintextOutput, - requester: address(0) - }); + function getE3(uint256 e3Id) external view returns (E3 memory) { + E3 storage stored = e3s[e3Id]; + // Return stored data if it exists, otherwise return defaults + if (address(stored.e3Program) != address(0)) { + return stored; + } + return E3({ + seed: 0, + threshold: [uint32(1), uint32(2)], + requestBlock: 0, + startWindow: [uint256(0), uint256(0)], + duration: 0, + expiration: 0, + encryptionSchemeId: bytes32(0), + e3Program: IE3Program(address(0)), + e3ProgramParams: bytes(""), + customParams: abi.encode(address(0), 0, 2), + decryptionVerifier: IDecryptionVerifier(address(0)), + committeePublicKey: committeePublicKey, + ciphertextOutput: bytes32(0), + plaintextOutput: plaintextOutput, + requester: address(0) + }); }examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
81-103:⚠️ Potential issue | 🔴 CriticalJavaScript bitwise operations are limited to 32-bit integers.
The left shift
byteValue << (j * 8)wraps modulo 32 in JavaScript. Whenj >= 4, shifts of 32, 40, 48, 56 bits become 0, 8, 16, 24 bits respectively, causing bytes 4-7 to overwrite bytes 0-3. Based on learnings, the decoded u64 values can be any 64-bit value, so this will produce incorrect results.🔧 Fix using BigInt for 64-bit arithmetic
-const decodeBytesToNumbers = (data: Uint8Array): number[] => { +const decodeBytesToNumbers = (data: Uint8Array): bigint[] => { if (data.length % 8 !== 0) { throw new Error('Data length must be multiple of 8') } const arrayLength = data.length / 8 - const result: number[] = [] + const result: bigint[] = [] for (let i = 0; i < arrayLength; i++) { const offset = i * 8 - let value = 0 + let value = 0n // Read 8 bytes in little-endian order for (let j = 0; j < 8; j++) { - const byteValue = data[offset + j] - value |= byteValue << (j * 8) + const byteValue = BigInt(data[offset + j]) + value |= byteValue << BigInt(j * 8) } result.push(value) } return result }Note: This change will require updating
decodeTallyto work withbigint[]instead ofnumber[](line 131-133 already uses BigInt conversions, so the impact is minimal).
🤖 Fix all issues with AI agents
In `@examples/CRISP/client/src/hooks/voting/useVoteCasting.ts`:
- Around line 132-142: The code hardcodes 2-element vote vectors which breaks
multi-option polls; update the VoteStateLite type to include num_options
(matching the SDK), update the Poll interface to expose either num_options or an
options array, and then replace every fixed [0n,0n], [balance,0n], and
[0n,balance] construction inside useVoteCasting (and its catch returns) with a
dynamic builder: create const numOptions = voteState.num_options ||
poll.num_options || poll.options.length and use Array(numOptions).fill(0n) for
the base vector, and when setting a selected choice assign
vote[pollSelected.value] = balance (or similar) so the vote vector length
matches the poll option count; ensure all places that returned [0n,0n]
(including initial returns and error branches) use this dynamic construction.
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 266-272: The check inside validateVote is tautological because
numChoices is set to vote.length, so the condition vote.length !== numChoices
will never be true; either remove that dead check or change the function
signature to accept an expected count (e.g., expectedNumChoices) and compare
that to vote.length. If you choose the latter, update the function
validateVote(vote: Vote, balance: bigint, expectedNumChoices: number) and
replace the current comparison with if (vote.length !== expectedNumChoices)
throw a descriptive Error; also ensure getMaxVoteValue(numChoices) uses the
intended count (use expectedNumChoices instead of numChoices) or adjust logic
accordingly.
🧹 Nitpick comments (5)
examples/CRISP/server/src/server/indexer.rs (1)
77-81: Consider validatingnum_optionsbefore use.While the field is correctly extracted from the decoded tuple, there's no validation that
num_optionsis a sensible value (e.g., > 0 and within the expectedMAX_OPTIONSlimit). If the contract enforces this, it may be acceptable, but defensive validation here would catch malformed data early.examples/CRISP/server/src/cli/commands.rs (1)
102-105: Consider makingnum_optionsconfigurable.The value is hardcoded to
2, which works for binary voting but limits the flexibility that this PR aims to introduce. Consider acceptingnum_optionsas a function parameter or reading it from the config, similar totoken_addressandbalance_threshold.pub async fn initialize_crisp_round( token_address: &str, balance_threshold: &str, + num_options: u64, ) -> Result<u64, Box<dyn std::error::Error + Send + Sync>> { // ... - let num_options = U256::from(2); + let num_options = U256::from(num_options);examples/CRISP/client/src/model/vote.model.ts (1)
59-59: Type definition aligns with SDK but creates duplication.The
Votetype is now defined identically in both this file and@crisp-e3/sdk(examples/CRISP/packages/crisp-sdk/src/types.tsline 69). Consider importing the type from the SDK instead of redefining it to maintain a single source of truth.+import { Vote } from '@crisp-e3/sdk' + // ... other type definitions ... -export type Vote = bigint[] +export { Vote }examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
52-54: Redundant conditional check.Since
choiceIdxiterates from0ton-1andn = vote.length, the conditionchoiceIdx < vote.lengthis always true. The fallback0nis unreachable.🧹 Simplify to remove dead code
for (let choiceIdx = 0; choiceIdx < n; choiceIdx += 1) { - const value = choiceIdx < vote.length ? vote[choiceIdx] : 0n + const value = vote[choiceIdx] const binary = toBinary(value).split('')
130-134: Update to align withdecodeBytesToNumbersfix.Once
decodeBytesToNumbersreturnsbigint[](per the fix above), theBigInt()conversion on line 133 becomes unnecessary.🧹 After fixing decodeBytesToNumbers
let value = 0n for (let i = 0; i < segment.length; i++) { const weight = 2n ** BigInt(segment.length - 1 - i) - value += BigInt(segment[i]) * weight + value += segment[i] * weight }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enclave-contracts/contracts/Enclave.sol (1)
262-284:⚠️ Potential issue | 🔴 CriticalPersist encryptionSchemeId/decryptionVerifier to storage.
e3s[e3Id]is written beforevalidate, butencryptionSchemeIdanddecryptionVerifierare only updated on the memory copy afterward. This leaves storage with zeroed fields and breaks later verification flows.🛠️ Suggested fix
e3.encryptionSchemeId = encryptionSchemeId; e3.decryptionVerifier = decryptionVerifier; + + e3s[e3Id].encryptionSchemeId = encryptionSchemeId; + e3s[e3Id].decryptionVerifier = decryptionVerifier;
🤖 Fix all issues with AI agents
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 111-140: The decodeTally function currently relies on
decodeBytesToNumbers which returns number[] and can lose precision for u64
coefficients; update decodeBytesToNumbers to decode into BigInt[] using
DataView.getBigUint64 (choose consistent endianness) so 64-bit values are
preserved, then update decodeTally to accept BigInt[] (adjust variables like
numbers, segment values, and the accumulation to use BigInt arithmetic) and
ensure MAX_VOTE_BITS and any comparisons use BigInt-safe logic; keep function
names decodeBytesToNumbers and decodeTally so callers remain valid.
- Around line 43-71: The encodeVote function must validate and reject empty vote
arrays to avoid division by zero: inside encodeVote (and since encryptVote calls
it), check if vote.length === 0 and throw a clear Error (e.g., "vote must
contain at least one choice") before computing segmentSize/degree; this mirrors
the guard in decodeTally and prevents Infinity/NaN propagation in segmentSize,
remainder, and the returned encoding.
🧹 Nitpick comments (2)
examples/CRISP/client/src/utils/constants.ts (1)
12-13: Centralize NUM_OPTIONS to avoid drift.
Consider sharing this constant with the worker/config (instead of duplicating) so a future option-count change doesn’t silently desync masking proofs.examples/CRISP/client/libs/crispSDKWorker.js (1)
9-10: Avoid hardcoding numOptions in the worker.
If the app’s option count changes, masking proofs will be generated with the wrong value. Consider passingnumOptionsin the message payload (or importing a shared constant) to keep it in sync.♻️ Suggested approach
-const NUM_OPTIONS = 2 +const DEFAULT_NUM_OPTIONS = 2 self.onmessage = async function (event) { const { type, data } = event.data switch (type) { case 'generate_proof': try { - const { e3Id, vote, publicKey, balance, address: slotAddress, signature, messageHash, isMasking, crispServer, merkleLeaves } = data + const { + e3Id, + vote, + publicKey, + balance, + address: slotAddress, + signature, + messageHash, + isMasking, + crispServer, + merkleLeaves, + numOptions = DEFAULT_NUM_OPTIONS, + } = data ... - numOptions: NUM_OPTIONS, + numOptions,Also applies to: 23-30
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
52-70:⚠️ Potential issue | 🟡 MinorReturn stored E3s to keep request/getE3 consistent.
request()persists toe3s, butgetE3()ignores the mapping and always returns a static struct, which will be incorrect for non‑zero E3 IDs or multiple requests.🛠️ Suggested fix
- function getE3(uint256) external view returns (E3 memory) { - return - E3({ + function getE3(uint256 e3Id) external view returns (E3 memory) { + E3 memory stored = e3s[e3Id]; + if (address(stored.e3Program) != address(0)) { + return stored; + } + return E3({ seed: 0, threshold: [uint32(1), uint32(2)], requestBlock: 0, startWindow: [uint256(0), uint256(0)], duration: 0, expiration: 0, encryptionSchemeId: bytes32(0), e3Program: IE3Program(address(0)), e3ProgramParams: bytes(""), - customParams: abi.encode(address(0), 0, 2), + customParams: abi.encode(address(0), e3Id, 2), decryptionVerifier: IDecryptionVerifier(address(0)), committeePublicKey: committeePublicKey, ciphertextOutput: bytes32(0), plaintextOutput: plaintextOutput, requester: address(0) - }); + }); }
🤖 Fix all issues with AI agents
In `@examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol`:
- Around line 181-193: The code can produce segmentSize == 0 (and thus silent
zero tallies) when numOptions (from e3Data[e3Id].numOptions) is greater than
tally.length; add a guard after decoding tally (from
_decodeBytesToUint64Array(e3.plaintextOutput)) to detect segmentSize == 0 (i.e.,
tally.length < numOptions) and return an empty uint256[] (or revert with a clear
message) instead of continuing; update logic around segmentSize, effectiveSize
and any downstream loops that assume non‑zero segments to early-exit on this
condition to avoid producing all-zero results.
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 52-68: The encodeVote logic allows each choice to use up to
segmentSize bits which can exceed the total MAX_VOTE_BITS that decodeTally
reads; update encodeVote (the loop using segmentSize, voteArray, toBinary) to
enforce a per-choice bit cap derived from MAX_VOTE_BITS (e.g., maxSegment =
Math.floor(MAX_VOTE_BITS / n)) and validate that binary.length <= maxSegment (or
reduce segmentSize to min(segmentSize, maxSegment)) and throw a clear Error
referencing MAX_VOTE_BITS and the choice index when exceeded so
encryptVote/encodeVote cannot produce values that decodeTally will truncate.
🧹 Nitpick comments (1)
templates/default/contracts/MyProgram.sol (1)
54-65: Interface signature updated correctly forcustomParamssupport.The
validatefunction now includes the additionalbytes calldataparameter to comply with the updatedIE3Programinterface. The parameter is intentionally unused in this template, which is appropriate.Consider naming the parameter
customParamsand adding a brief NatSpec note for template users who may want to use it (asCRISPProgram.soldoes fornumOptionsdecoding):📝 Suggested documentation improvement
/// `@notice` Validate the E3 program parameters /// `@param` e3Id The E3 program ID /// `@param` e3ProgramParams The E3 program parameters - function validate(uint256 e3Id, uint256, bytes calldata e3ProgramParams, bytes calldata, bytes calldata) external returns (bytes32) { + /// `@param` customParams Optional custom parameters for program-specific configuration + function validate(uint256 e3Id, uint256, bytes calldata e3ProgramParams, bytes calldata, bytes calldata customParams) external returns (bytes32) { require(authorizedContracts[msg.sender] || msg.sender == owner(), CallerNotAuthorized()); require(paramsHashes[e3Id] == bytes32(0), E3AlreadyInitialized()); paramsHashes[e3Id] = keccak256(e3ProgramParams);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@examples/CRISP/packages/crisp-sdk/src/vote.ts`:
- Around line 262-292: validateVote currently omits the minimum choices check
that encodeVote enforces, so add a guard at the start of validateVote (which
accepts vote: Vote and balance: bigint) to throw an Error if vote.length < 2;
keep this check before computing numChoices or maxValue (which uses
getMaxVoteValue) so the binary vs multi-choice logic only runs for 2+ options
and prevents incorrect "3+ options" handling for single-choice votes.
- Updated @crisp-e3/sdk to 0.5.9 - Updated @crisp-e3/contracts to 0.5.9 - Updated @crisp-e3/zk-inputs to 0.5.9 - Published to npm
2 participants
fix #1255
Summary by CodeRabbit
New Features
Improvements
Tests