refactor: replace enclave-sdk with crisp-sdk#908
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
cedoor
force-pushed
the
refactor/replace-enclave-sdk
branch
from
f74f390 to
8eb2c7b
Compare
WalkthroughWASM bindings and core zk-inputs were changed to accept vectorized votes and signed i64 JS-facing types; a new wasm export exposes BFV params. The CRISP SDK was refactored from an enclave path to using ZKInputsGenerator, types were updated to polynomial-shaped circuit inputs, and tests adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant SDK as Crisp SDK (TS)
participant ZK as ZKInputsGenerator (wasm)
Note over App,SDK: prepare encodedVote (string[]) and previousCiphertext (Uint8Array)
App->>SDK: encryptVoteAndGenerateCRISPInputs(encodedVote, publicKey, previousCiphertext, bfvParams?)
SDK->>ZK: ZKInputsGenerator.withDefaults()/with(bfvParams) -> generate_inputs(prev_ciphertext, public_key, vote: Vec<i64>)
alt success
ZK-->>SDK: CRISPCircuitInputs (polynomials, params, ct_add...)
SDK-->>App: CRISPCircuitInputs + metadata placeholders
else error
ZK-->>SDK: error
SDK-->>App: propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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
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)
59-85: Fix variable declarations in switch case.Lines 64-65 declare variables inside a switch case without a block, which can lead to scope issues. Wrap the case logic in braces.
Apply this diff:
switch (votingMode) { case VotingMode.GOVERNANCE: + { const voteArray = [] const length = bfvParams?.degree || DEFAULT_BFV_PARAMS.degree const halfLength = length / 2 const yesBinary = toBinary(vote.yes).split('') const noBinary = toBinary(vote.no).split('') // Fill first half with 'yes' binary representation (pad with leading 0s if needed) for (let i = 0; i < halfLength; i++) { const offset = halfLength - yesBinary.length voteArray.push(i < offset ? '0' : yesBinary[i - offset]) } // Fill second half with 'no' binary representation (pad with leading 0s if needed) for (let i = 0; i < length - halfLength; i++) { const offset = length - halfLength - noBinary.length voteArray.push(i < offset ? '0' : noBinary[i - offset]) } return voteArray + } default: throw new Error('Unsupported voting mode') }Based on static analysis hints.
🧹 Nitpick comments (4)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
20-24: Clarify the intended usage scope of DEFAULT_BFV_PARAMS.The comment states "for testing purposes only," but this constant is used in production code (line 65 and line 156 in
vote.ts). Either update the comment to reflect production usage or establish separate test and production constants.examples/CRISP/crates/zk-inputs/src/lib.rs (1)
166-169: Consider expanding test coverage beyond the alternating pattern.All tests use the same
create_vote_vectorhelper that produces an alternating 0-1 pattern. Consider adding tests with:
- All zeros
- All ones
- Random patterns
- Edge values near boundaries
This would improve confidence in the encoding and encryption logic.
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
89-110: Note potential precision loss when casting u64 to f64.Lines 102 and 106 cast
u64values tof64for JavaScript compatibility. While the current default parameters (plaintext_modulus=1032193, moduli=[0x3FFFFFFF000001]) are within the safe integer range (2^53 - 1), custom parameters with larger moduli could lose precision.Consider using
BigIntfor JavaScript representation if moduli values may exceed 2^53 in the future.examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
151-175: Implementation is work-in-progress.The TODO comment on line 165 indicates that several circuit input fields (
public_key_x,public_key_y,signature,hashed_message,balance,merkle_proof_*) are incomplete placeholders. Ensure these fields are populated before the feature is production-ready.Do you want me to generate an issue to track the completion of these TODO items?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs(4 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(12 hunks)examples/CRISP/packages/crisp-sdk/package.json(0 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/index.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(3 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/packages/crisp-sdk/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-03T23:02:41.732Z
Learnt from: ryardley
PR: gnosisguild/enclave#133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-03T23:02:41.732Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
examples/CRISP/crates/zk-inputs/src/lib.rs
🧬 Code graph analysis (6)
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
BFVParams(174-178)
examples/CRISP/crates/zk-inputs/src/lib.rs (1)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (4)
generate_inputs(50-69)encrypt_vote(82-87)create_vote_vector(128-130)with_defaults(42-46)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
CRISPCircuitInputs(13-13)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
examples/CRISP/crates/zk-inputs/src/lib.rs (5)
encrypt_vote(129-141)get_bfv_params(157-159)new(40-48)create_vote_vector(167-169)with_defaults(57-59)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
encodeVote(59-85)encryptVoteAndGenerateCRISPInputs(151-175)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)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (3)
IVote(79-88)BFVParams(174-178)CRISPCircuitInputs(146-169)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)
🪛 Biome (2.1.2)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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: crisp_e2e
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
🔇 Additional comments (11)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
13-13: LGTM! Public API updated to reflect new circuit input structure.The removal of
CRISPVoteAndInputsand retention ofCRISPCircuitInputsaligns with the refactoring to use polynomial-based representations.examples/CRISP/crates/zk-inputs/src/lib.rs (2)
50-59: LGTM! Documentation appropriately updated.The expanded documentation clearly indicates testing-only usage and provides helpful notes about parameter suitability.
70-82: LGTM! Vote vectorization implemented correctly.The signature change from scalar to
Vec<u64>and direct encoding of the vote vector aligns with the PR's vectorization objectives.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
7-12: LGTM! Imports updated to use new BFV-based types.The test imports correctly reference the new
BFVParams,DEFAULT_BFV_PARAMS, andMAXIMUM_VOTE_VALUEexports.
127-136: LGTM! Test assertions updated for new circuit input structure.The expectations correctly validate the new
CRISPCircuitInputsstructure withct_add,params, and polynomial-based field types.examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (2)
49-69: LGTM! WASM binding correctly updated for vector-based votes.The signature change aligns with the core library's vectorization.
127-130: LGTM! Test helper aligns with core library.The
create_vote_vectorhelper matches the pattern used in the corezk-inputscrate.examples/CRISP/packages/crisp-sdk/src/types.ts (3)
90-141: LGTM! Comprehensive type definitions for polynomial-based circuit inputs.The new interfaces (
Polynomial,GrecoCryptographicParams,GrecoBoundParams,CiphertextAdditionInputs,GrecoParams) provide clear structure for the refactored CRISP circuit input representation.
147-169: LGTM! CRISPCircuitInputs correctly restructured.The migration from
string[][]toPolynomial[]for circuit input fields and the addition ofct_addandparamsfields align with the PR's polynomial-based refactoring objectives.
173-177: LGTM! BFVParams interface appropriately defined.The interface clearly captures the BFV parameter structure with appropriate types (number for degree, bigint for moduli).
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
156-160: LGTM! Refactored to use ZKInputsGenerator workflow.The migration from EnclaveSDK to
ZKInputsGeneratorand the conversion ofencodedVotetoBigUint64Arraycorrectly implements the vector-based encryption flow.
There was a problem hiding this comment.
Actionable comments posted: 0
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-sdk/src/vote.ts (2)
51-59: Fix the parameter documentation.The doc comment at line 55 references
@param bfvDegreebut the actual parameter isbfvParams.Apply this diff to fix the documentation:
/** * Encode a vote based on the voting mode * @param vote The vote to encode * @param votingMode The voting mode to use for encoding - * @param bfvDegree The degree of the BFV polynomial to use for encoding * @param votingPower The voting power of the voter + * @param bfvParams Optional BFV parameters to use for encoding * @returns The encoded vote as a string */
62-84: Wrap switch case variable declarations in blocks.Variables declared directly in switch cases can be erroneously accessed by other cases. Wrap the declarations in a block to restrict their scope.
Apply this diff to fix the scope issue:
switch (votingMode) { - case VotingMode.GOVERNANCE: + case VotingMode.GOVERNANCE: { const voteArray = [] const length = bfvParams?.degree || DEFAULT_BFV_PARAMS.degree const halfLength = length / 2 const yesBinary = toBinary(vote.yes).split('') const noBinary = toBinary(vote.no).split('') // Fill first half with 'yes' binary representation (pad with leading 0s if needed) for (let i = 0; i < halfLength; i++) { const offset = halfLength - yesBinary.length voteArray.push(i < offset ? '0' : yesBinary[i - offset]) } // Fill second half with 'no' binary representation (pad with leading 0s if needed) for (let i = 0; i < length - halfLength; i++) { const offset = length - halfLength - noBinary.length voteArray.push(i < offset ? '0' : noBinary[i - offset]) } return voteArray + } default: throw new Error('Unsupported voting mode') }Based on static analysis.
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
170-181: Track the TODO for completing CRISP input fields.The placeholder fields (public_key_x, public_key_y, signature, hashed_message, balance, merkle_proof_*) need to be properly populated before the circuit can generate CRISP proofs.
Do you want me to open a new issue to track the completion of these CRISP input fields?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (3)
IVote(79-88)BFVParams(174-178)CRISPCircuitInputs(146-169)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)
CRISPCircuitInputs(13-13)
🪛 Biome (2.1.2)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: build_e3_support_dev
- GitHub Check: rust_unit
🔇 Additional comments (3)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
90-178: LGTM! Well-structured type definitions for the SDK refactoring.The new interfaces (Polynomial, GrecoCryptographicParams, GrecoBoundParams, CiphertextAdditionInputs, GrecoParams, BFVParams) provide clear structure for the migration from string-based to polynomial-based representations. The updated CRISPCircuitInputs interface appropriately combines the new structured types with existing string array fields for signature and Merkle proof data.
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
7-10: LGTM! Imports correctly updated for the SDK refactoring.The new imports align with the shift from EnclaveSDK to ZKInputsGenerator-based flow, introducing the necessary types and constants.
151-167: Verify external package return type or add explicit type checking.The type assertion at line 167 assumes
generateInputsfrom@enclave/crisp-zk-inputsreturns compatible fields, but without access to that package's implementation, the exact return structure cannot be verified in this repository. The @todo comment and the re-assignment of fields (public_key_x, public_key_y, signature, hashed_message, balance) to empty/default values suggest incomplete initialization. Confirm thatgenerateInputsreturns all required fields fromCRISPCircuitInputs(ct_add, params, ct0is, ct1is, pk0is, pk1is, r1is, r2is, p1is, p2is, u, e0, e1, k1, merkle_proof_*), or explicitly document which fields it provides vs. which are added later.
cedoor
force-pushed
the
refactor/replace-enclave-sdk
branch
from
321b98d to
1ef7e30
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
examples/CRISP/crates/zk-inputs/src/lib.rs (2)
212-230: Update test name and purpose.The test is named
test_inputs_generation_with_vote_0but now uses a generic alternating pattern rather than a specific vote value. Either update the test name to reflect its actual purpose or differentiate the test data.
286-304: Fix test to actually test different vote values.The test claims to test vote = 0 and vote = 1, but both test cases use the same
vote.clone(). This doesn't validate behavior with different vote values as intended.Apply this diff to fix the test:
#[test] fn test_vote_values() { let generator = ZKInputsGenerator::with_defaults().expect("failed to create generator"); let public_key = generator .generate_public_key() .expect("failed to generate public key"); let vote = create_vote_vector(); let prev_ciphertext = generator .encrypt_vote(&public_key, vote.clone()) .expect("failed to encrypt vote"); - // Test vote = 0. + // Test with alternating pattern. let result_0 = generator.generate_inputs(&prev_ciphertext, &public_key, vote.clone()); assert!(result_0.is_ok()); - // Test vote = 1. - let result_1 = generator.generate_inputs(&prev_ciphertext, &public_key, vote.clone()); + // Test with all ones. + let vote_ones: Vec<u64> = vec![1u64; DEFAULT_DEGREE]; + let result_1 = generator.generate_inputs(&prev_ciphertext, &public_key, vote_ones); assert!(result_1.is_ok()); }examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
62-85: Wrap switch case declarations in a block.The static analysis tool correctly identifies that variable declarations in the switch case can be accessed by other cases, which is error-prone.
Apply this diff to wrap the case in a block:
switch (votingMode) { - case VotingMode.GOVERNANCE: + case VotingMode.GOVERNANCE: { const voteArray = [] const length = bfvParams?.degree || DEFAULT_BFV_PARAMS.degree const halfLength = length / 2 const yesBinary = toBinary(vote.yes).split('') const noBinary = toBinary(vote.no).split('') // Fill first half with 'yes' binary representation (pad with leading 0s if needed) for (let i = 0; i < halfLength; i++) { const offset = halfLength - yesBinary.length voteArray.push(i < offset ? '0' : yesBinary[i - offset]) } // Fill second half with 'no' binary representation (pad with leading 0s if needed) for (let i = 0; i < length - halfLength; i++) { const offset = length - halfLength - noBinary.length voteArray.push(i < offset ? '0' : noBinary[i - offset]) } return voteArray + } default: throw new Error('Unsupported voting mode') }
🧹 Nitpick comments (2)
examples/CRISP/crates/zk-inputs/src/lib.rs (1)
166-169: Consider adding tests with varied vote patterns.The deterministic helper is good for consistent testing. However, consider adding tests with different vote patterns (all zeros, all ones, random values) to ensure robustness across different inputs.
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
169-181: TODO: Complete the remaining circuit inputs.The placeholder values for signature, merkle proof, etc., need to be populated before this can be used in production. The TODO comment correctly flags this incomplete implementation.
Do you want me to help implement the logic to populate these remaining fields, or should this be tracked in a separate issue?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs(4 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(12 hunks)examples/CRISP/packages/crisp-sdk/package.json(0 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/index.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(3 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/packages/crisp-sdk/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/CRISP/packages/crisp-sdk/src/constants.ts
- examples/CRISP/packages/crisp-sdk/src/index.ts
- examples/CRISP/packages/crisp-sdk/tests/vote.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-03T23:02:41.732Z
Learnt from: ryardley
PR: gnosisguild/enclave#133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-03T23:02:41.732Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
examples/CRISP/crates/zk-inputs/src/lib.rs
🧬 Code graph analysis (4)
examples/CRISP/crates/zk-inputs/src/lib.rs (2)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (4)
generate_inputs(50-69)encrypt_vote(82-87)create_vote_vector(128-130)with_defaults(42-46)templates/default/program/src/lib.rs (1)
test(37-72)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
examples/CRISP/crates/zk-inputs/src/lib.rs (5)
encrypt_vote(129-141)get_bfv_params(157-159)new(40-48)create_vote_vector(167-169)with_defaults(57-59)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (3)
IVote(79-88)BFVParams(174-178)CRISPCircuitInputs(146-169)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)
CRISPCircuitInputs(13-13)
🪛 Biome (2.1.2)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
🔇 Additional comments (10)
examples/CRISP/crates/zk-inputs/src/lib.rs (3)
29-32: LGTM!The comment clarification about testing purposes is helpful and appropriate.
50-59: LGTM!The expanded documentation with testing warnings is clear and helpful.
156-160: LGTM!The method correctly returns a cloned Arc reference to the BFV parameters.
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (3)
48-69: LGTM with upstream validation dependency.The wrapper correctly forwards the vote vector to the core generator. Once validation is added to the core
generate_inputsmethod (as suggested in the core review), this will automatically benefit from that check.
80-87: LGTM with upstream validation dependency.The wrapper correctly forwards the vote vector to the core generator. Once validation is added to the core
encrypt_votemethod (as suggested in the core review), this will automatically benefit from that check.
127-130: LGTM!The test helper is consistent with the core library's approach to deterministic testing.
examples/CRISP/packages/crisp-sdk/src/types.ts (3)
90-95: LGTM!The Polynomial interface provides a clear structure for coefficient vectors.
97-141: LGTM!The structured type definitions for cryptographic parameters and ciphertext operations are well-organized and comprehensive.
171-177: Verify moduli type compatibility with WASM layer.The
BFVParamsinterface declaresmoduli: BigUint64Array, which correctly preserves u64 precision. However, the WASM binding'sget_bfv_paramsmethod (inzk-inputs-wasm/src/lib.rs) currently casts moduli to f64, causing precision loss.Ensure that once the WASM layer is fixed to return moduli as strings (per the earlier review comment), the TypeScript side properly converts those strings to BigUint64Array.
Based on the WASM review finding, please verify that the conversion from WASM string moduli to BigUint64Array is implemented correctly when the WASM fix is applied.
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
7-10: LGTM!The import changes correctly reflect the migration from EnclaveSDK to ZKInputsGenerator.
ctrlc03
force-pushed
the
refactor/replace-enclave-sdk
branch
from
c5d0e2d to
0563f5a
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/crates/zk-inputs-wasm/src/lib.rs (1)
52-74: Validate vote coefficients are non-negative.The function accepts
Vec<i64>and casts toVec<u64>without validating that all values are non-negative. Negative vote coefficients would produce incorrect large unsigned values.Apply this diff to add validation:
pub fn generate_inputs( &self, prev_ciphertext: &[u8], public_key: &[u8], vote: Vec<i64>, ) -> Result<JsValue, JsValue> { + // Validate that all vote coefficients are non-negative. + if vote.iter().any(|&v| v < 0) { + return Err(JsValue::from_str("All vote coefficients must be non-negative")); + } + let vote_vec: Vec<u64> = vote.into_iter().map(|v| v as u64).collect();examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
59-85: Fix switch case variable declarations.Variables declared at lines 64-65 (
voteArray,length, etc.) can be erroneously accessed by other switch clauses. Wrap the GOVERNANCE case body in a block to restrict variable scope.Apply this diff to fix the issue:
switch (votingMode) { - case VotingMode.GOVERNANCE: + case VotingMode.GOVERNANCE: { const voteArray = [] const length = bfvParams?.degree || DEFAULT_BFV_PARAMS.degree const halfLength = length / 2 const yesBinary = toBinary(vote.yes).split('') const noBinary = toBinary(vote.no).split('') // Fill first half with 'yes' binary representation (pad with leading 0s if needed) for (let i = 0; i < halfLength; i++) { const offset = halfLength - yesBinary.length voteArray.push(i < offset ? '0' : yesBinary[i - offset]) } // Fill second half with 'no' binary representation (pad with leading 0s if needed) for (let i = 0; i < length - halfLength; i++) { const offset = length - halfLength - noBinary.length voteArray.push(i < offset ? '0' : noBinary[i - offset]) } return voteArray + } default: throw new Error('Unsupported voting mode') }
♻️ Duplicate comments (2)
examples/CRISP/crates/zk-inputs/src/lib.rs (2)
70-82: Add validation for vote vector length.The function accepts a
Vec<u64>vote parameter but doesn't validate that its length matches the BFV degree. If the vote vector length doesn't matchself.bfv_params.degree(), the plaintext encoding at line 81 may fail with an unclear error message.Apply this diff to add validation:
pub fn generate_inputs( &self, prev_ciphertext: &[u8], public_key: &[u8], vote: Vec<u64>, ) -> Result<String> { + // Validate vote vector length matches BFV degree. + if vote.len() != self.bfv_params.degree() { + return Err(eyre::eyre!( + "Vote vector length {} does not match BFV degree {}", + vote.len(), + self.bfv_params.degree() + )); + } + // Deserialize the provided public key. let pk = PublicKey::from_bytes(public_key, &self.bfv_params)
129-135: Add validation for vote vector length.Similar to
generate_inputs, this function should validate that the vote vector length matches the BFV degree before encoding.Apply this diff to add validation:
pub fn encrypt_vote(&self, public_key: &[u8], vote: Vec<u64>) -> Result<Vec<u8>> { + // Validate vote vector length matches BFV degree. + if vote.len() != self.bfv_params.degree() { + return Err(eyre::eyre!( + "Vote vector length {} does not match BFV degree {}", + vote.len(), + self.bfv_params.degree() + )); + } + let pk = PublicKey::from_bytes(public_key, &self.bfv_params)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs(5 hunks)examples/CRISP/crates/zk-inputs/src/lib.rs(12 hunks)examples/CRISP/packages/crisp-sdk/package.json(0 hunks)examples/CRISP/packages/crisp-sdk/src/constants.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/index.ts(1 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(2 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(3 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/packages/crisp-sdk/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/packages/crisp-sdk/src/constants.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-03T23:02:41.732Z
Learnt from: ryardley
PR: gnosisguild/enclave#133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-03T23:02:41.732Z
Learning: In `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, using the same RNG instance `rng_test` for generating multiple key shares without advancing its state is acceptable.
Applied to files:
examples/CRISP/crates/zk-inputs/src/lib.rs
🧬 Code graph analysis (5)
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
CRISPCircuitInputs(13-13)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
examples/CRISP/packages/crisp-sdk/src/types.ts (3)
IVote(79-88)BFVParams(174-178)CRISPCircuitInputs(146-169)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (2)
examples/CRISP/crates/zk-inputs/src/lib.rs (5)
new(40-48)encrypt_vote(129-141)get_bfv_params(157-159)create_vote_vector(167-169)with_defaults(57-59)crates/bfv-helpers/src/lib.rs (1)
moduli_array(169-177)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
encodeVote(59-85)encryptVoteAndGenerateCRISPInputs(151-180)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)
examples/CRISP/crates/zk-inputs/src/lib.rs (1)
examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (4)
generate_inputs(53-74)encrypt_vote(87-94)create_vote_vector(145-147)with_defaults(45-49)
🪛 Biome (2.1.2)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (10)
examples/CRISP/packages/crisp-sdk/src/index.ts (1)
13-13: LGTM! Clean export consolidation.The single-line export statement improves readability and correctly removes the deprecated
CRISPVoteAndInputstype from the public API surface.examples/CRISP/crates/zk-inputs/src/lib.rs (1)
166-169: Good test helper for deterministic vote vectors.The helper function provides a consistent way to generate test vote vectors matching the default degree, improving test maintainability.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (3)
7-12: LGTM! Imports correctly updated for new API.The imports now properly reference the new BFV-based types and constants, aligning with the SDK's transition from enclave-sdk to crisp-sdk.
19-30: Good test coverage for BFV parameter variants.The tests properly exercise both default BFV parameters and custom parameters, ensuring the encoding logic works correctly across different polynomial degrees.
121-136: LGTM! Test correctly uses the new API.The test now properly creates
previousCiphertextusingBigInt64Arrayand validates the structure of the returnedCRISPCircuitInputsobject, including the new fields likect_addandparams.examples/CRISP/packages/crisp-sdk/src/types.ts (3)
90-142: Excellent type system refactoring.The new interfaces (
Polynomial,GrecoCryptographicParams,GrecoBoundParams,CiphertextAdditionInputs,GrecoParams) provide clear, well-structured types that align with the BFV-based parameterization approach. ThePolynomialtype wraps coefficients in a clear, type-safe manner.
146-169: LGTM! CRISPCircuitInputs properly structured.The updated interface now uses
PolynomialandPolynomial[]types instead of raw string arrays, improving type safety and clarity. The new fieldsct_addandparamsintegrate well with the overall structure.
174-178: Clean BFVParams interface.The interface correctly captures the BFV parameters with appropriate types:
degreeas number,plaintextModulusas bigint, andmodulias BigInt64Array for precision preservation.examples/CRISP/crates/zk-inputs-wasm/src/lib.rs (1)
96-127: Excellent precision preservation with BigInt.Using
js_sys::BigIntforplaintextModulusandmodulicorrectly avoids precision loss for large u64 values that exceed JavaScript's safe integer range (2^53-1). This addresses the critical issue from the past review.examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
151-180: LGTM! Well-structured API with proper validation.The function correctly:
- Validates
encodedVotelength against BFV degree (line 157-159), addressing the past review comment- Creates
ZKInputsGeneratorwith appropriate BFV parameters- Converts vote to
BigInt64Arrayfor WASM compatibility- Returns properly structured
CRISPCircuitInputs
There was a problem hiding this comment.
Actionable comments posted: 0
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)
63-84: Wrap switch case declarations in a block.Variable declarations (
voteArray,length, etc.) inside the switch case can erroneously leak to other cases. Wrap the case body in braces to restrict scope.As per static analysis hints.
Apply this diff:
switch (votingMode) { - case VotingMode.GOVERNANCE: + case VotingMode.GOVERNANCE: { const voteArray = [] const length = bfvParams?.degree || DEFAULT_BFV_PARAMS.degree const halfLength = length / 2 const yesBinary = toBinary(vote.yes).split('') const noBinary = toBinary(vote.no).split('') // Fill first half with 'yes' binary representation (pad with leading 0s if needed) for (let i = 0; i < halfLength; i++) { const offset = halfLength - yesBinary.length voteArray.push(i < offset ? '0' : yesBinary[i - offset]) } // Fill second half with 'no' binary representation (pad with leading 0s if needed) for (let i = 0; i < length - halfLength; i++) { const offset = length - halfLength - noBinary.length voteArray.push(i < offset ? '0' : noBinary[i - offset]) } return voteArray + } default: throw new Error('Unsupported voting mode') }
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
170-181: Track completion of placeholder CRISP inputs.The TODO at line 172 and placeholder values (empty arrays for signature/merkle fields, zero values for balance) indicate incomplete implementation. Ensure these fields are populated before the function is used in production.
Do you want me to open an issue to track the completion of these required CRISP circuit inputs (public_key_x/y, signature, hashed_message, balance, merkle_proof fields)?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts(3 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 (3)
IVote(79-88)BFVParams(174-178)CRISPCircuitInputs(146-169)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
DEFAULT_BFV_PARAMS(24-24)
🪛 Biome (2.1.2)
examples/CRISP/packages/crisp-sdk/src/vote.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: test_contracts
🔇 Additional comments (3)
examples/CRISP/packages/crisp-sdk/src/vote.ts (3)
7-10: LGTM! Imports align with the SDK refactor.The updated imports correctly reflect the transition from enclave-sdk to the zk-inputs-based workflow, bringing in
ZKInputsGenerator, updated types, and the newDEFAULT_BFV_PARAMSconstant.
159-161: Validation correctly addresses previous review feedback.The encodedVote length validation against BFV degree properly prevents runtime errors from the Rust layer, as recommended in the earlier review.
165-165: BigInt64Array is correct. The type has been verified. The WASM binding expectsVec<i64>for the vote parameter, and BigInt64Array is the correct JavaScript type for signed i64 values. The WASM layer internally converts i64 to u64 via bit-casting, which is the intended behavior.
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores