fix: add real encoded tally#1122
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds a new Rust crate Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as TS Tests / Frontend
participant SDK as Crisp SDK (TS)
participant Contract as CRISPProgram (Solidity)
participant Server as CRISP Server (Rust)
participant Utils as crisp-utils (Rust crate)
participant Repo as Server Repo (DB)
Client->>SDK: submit/verify hex tally
SDK->>SDK: hexToBytes → decodeBytesToNumbers → split halves → compute yes/no
Contract->>Server: expose plaintextOutput (bytes)
Server->>Utils: decode_tally(bytes)
Utils-->>Server: VoteCounts { yes, no }
Server->>Repo: set_votes(BigUint yes, BigUint no)
Repo-->>Server: persist counts as strings
Server-->>Client: return WebResultRequest (tallies as strings)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
cedoor
force-pushed
the
refactor/new-decoding
branch
from
b835472 to
fe99f60
Compare
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
examples/CRISP/Cargo.toml(2 hunks)examples/CRISP/crates/crisp-utils/Cargo.toml(1 hunks)examples/CRISP/crates/crisp-utils/src/lib.rs(1 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(6 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(4 hunks)examples/CRISP/server/Cargo.toml(1 hunks)examples/CRISP/server/src/server/indexer.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
examples/CRISP/server/Cargo.tomlexamples/CRISP/Cargo.tomlexamples/CRISP/crates/crisp-utils/Cargo.toml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
examples/CRISP/server/Cargo.tomlexamples/CRISP/Cargo.tomlexamples/CRISP/crates/crisp-utils/Cargo.toml
📚 Learning: 2025-11-05T14:12:57.814Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 963
File: examples/CRISP/client/package.json:25-25
Timestamp: 2025-11-05T14:12:57.814Z
Learning: In the Enclave/CRISP codebase, `enclave-e3/sdk` and `crisp-e3/sdk` are different packages: `enclave-e3/sdk` is the general Enclave SDK, while `crisp-e3/sdk` is the CRISP-specific SDK. The CRISP client (`examples/CRISP/client`) intentionally depends on `enclave-e3/sdk`, not `crisp-e3/sdk`.
Applied to files:
examples/CRISP/server/Cargo.tomlexamples/CRISP/Cargo.tomlexamples/CRISP/crates/crisp-utils/Cargo.toml
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/Cargo.toml
🧬 Code graph analysis (2)
examples/CRISP/crates/crisp-utils/src/lib.rs (1)
crates/bfv-helpers/src/lib.rs (1)
decode_bytes_to_vec_u64(453-462)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
decodeTally(83-117)encodeVote(26-47)
⏰ 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_unit
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: Build & Push Image
🔇 Additional comments (15)
examples/CRISP/server/Cargo.toml (1)
49-49: LGTM!The workspace dependency addition follows the established pattern used by
crisp-constantsand correctly integrates the newcrisp-utilscrate. Based on learnings, this aligns with the self-contained CRISP workspace configuration.examples/CRISP/crates/crisp-utils/Cargo.toml (1)
1-13: LGTM!The new crate configuration is well-structured, follows the workspace inheritance pattern, and declares minimal dependencies appropriate for FHE tally decoding utilities.
examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
24-24: LGTM! The conversion fromBigInttonumberis safe here.
2^50 - 1is well within JavaScript'sNumber.MAX_SAFE_INTEGER(2^53 - 1), so no precision loss occurs. Note that ifHALF_LARGEST_MINIMUM_DEGREEwere ever increased beyond 52, this would need to revert toBigInt.examples/CRISP/packages/crisp-sdk/src/vote.ts (5)
31-32: LGTM!Converting
numbertoBigIntbefore binary encoding maintains compatibility with thetoBinaryhelper while allowing theVotetype to usenumberfields.
83-117: Verify decoded values are binary (0 or 1).The conversion logic at lines 102-111 assumes each element in
yesBinary/noBinaryis either 0 or 1. IfdecodeBytesToNumberscan return values other than 0/1 (which seems likely given it reads full 8-byte integers), the weighted sum will produce incorrect results.Please clarify: Are the decoded byte chunks guaranteed to represent single-bit values (0 or 1), or can they be arbitrary integers? If the latter, the binary-to-decimal conversion logic would need adjustment.
136-136: LGTM!Consistent with the
Votetype change to usenumberfields.
187-189: LGTM!The negative value check correctly uses number comparison, consistent with the
Votetype change.
211-211: LGTM!Consistent with the
Votetype change to usenumberfields.examples/CRISP/Cargo.toml (2)
9-10: LGTM!The new
crisp-utilscrate is correctly added to the workspace members. Based on learnings, this follows the established pattern for the self-contained CRISP workspace.
24-24: LGTM!The workspace dependency declaration follows the same pattern as
crisp-constantsand correctly references the local crate path.examples/CRISP/server/src/server/indexer.rs (2)
18-18: LGTM: Import statement is correct.The import of
decode_tallyfrom the newcrisp_utilscrate is properly declared.
293-301: LGTM: Clean integration of tally decoding utility.The replacement of manual decoding logic with the dedicated
decode_tallyfunction improves code clarity and maintainability. Error handling via the?operator correctly propagates decoding failures, and the decoded values are properly extracted and logged before being persisted.examples/CRISP/crates/crisp-utils/src/lib.rs (1)
60-71: LGTM: Test validates real FHE output decoding.The test uses actual FHE computation output and correctly validates the expected decoded values (10 billion yes, 30 billion no). This provides good coverage for the decoding logic with realistic data.
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
37-37: LGTM: Test updates align with type changes.The migration from
biginttonumberliterals is consistent throughout the test suite. ThedecodeHalfhelper function correctly returnsnumberand usesparseIntfor base-2 conversion, aligning with the updatedVotetype definition.Also applies to: 55-56, 61-79
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
74-78: No action needed. Thenumbertype is appropriate for the Vote fields.The system architecture explicitly constrains vote values to
MAXIMUM_VOTE_VALUE = 2^50 - 1(approximately 1.1 quadrillion), which is well belowNumber.MAX_SAFE_INTEGERof2^53 - 1(approximately 9 quadrillion). Vote values are validated against this maximum before use, preventing any precision loss. Thenumbertype is suitable for this domain.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (1)
54-76: Critical: Bitwise operations truncate values beyond 32 bits.JavaScript bitwise operators (
<<,|=) operate on 32-bit signed integers. Whenj >= 4, the shift amount exceeds 31 bits, causing the upper 4 bytes to be lost. This means 64-bit little-endian values cannot be correctly reconstructed.Apply this diff to fix the truncation using
BigIntfor reconstruction:const decodeBytesToNumbers = (data: Uint8Array): number[] => { if (data.length % 8 !== 0) { throw new Error('Data length must be multiple of 8') } const arrayLength = data.length / 8 const result: number[] = [] 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) + value |= BigInt(byteValue) << BigInt(j * 8) } - result.push(value) + result.push(Number(value)) } return result }Note: If values can exceed
Number.MAX_SAFE_INTEGER(2^53 - 1), the function should returnbigint[]instead to avoid precision loss.
🧹 Nitpick comments (2)
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (1)
242-244: Consider usinguint256for the loop counter.While functionally correct, using
uint64for the loop counterjis unconventional in Solidity. The standard practice is to useuint256for loop counters, which is also the default type for integer literals.Apply this diff:
// Read 8 bytes in little-endian order - for (uint64 j = 0; j < 8; j++) { + for (uint256 j = 0; j < 8; j++) { value |= uint64(uint8(data[offset + j])) << (j * 8); }examples/CRISP/crates/crisp-utils/src/lib.rs (1)
31-54: Consider adding basic validation for empty input.While the BigUint approach correctly prevents overflow, the function doesn't handle the edge case of empty input. If
tally_bytesis empty,decode_bytes_to_vec_u64would return an empty vector, leading tohalf_d = 0and empty slices, which would produceyes = 0andno = 0. This might be acceptable behavior, but consider documenting it or adding explicit validation.Consider adding a comment or validation:
pub fn decode_tally(tally_bytes: &[u8]) -> Result<VoteCounts> { // Decode bytes to numbers array (little-endian, 8 bytes per value) let decoded = decode_bytes_to_vec_u64(tally_bytes)?; + + // Empty input results in zero votes for both options + if decoded.is_empty() { + return Ok(VoteCounts { yes: BigUint::from(0u64), no: BigUint::from(0u64) }); + } // Votes are right-aligned with leading zeros, so we can use the entire halves let half_d = decoded.len() / 2;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
examples/CRISP/crates/crisp-utils/Cargo.toml(1 hunks)examples/CRISP/crates/crisp-utils/src/lib.rs(1 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol(2 hunks)examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol(1 hunks)examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts(3 hunks)examples/CRISP/packages/crisp-sdk/src/vote.ts(3 hunks)examples/CRISP/packages/crisp-sdk/tests/vote.test.ts(1 hunks)examples/CRISP/server/src/server/indexer.rs(2 hunks)examples/CRISP/server/src/server/models.rs(3 hunks)examples/CRISP/server/src/server/repo.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/CRISP/crates/crisp-utils/Cargo.toml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1122
File: examples/CRISP/crates/crisp-utils/src/lib.rs:30-54
Timestamp: 2025-12-17T16:25:05.292Z
Learning: In the CRISP project's `decode_tally` function (examples/CRISP/crates/crisp-utils/src/lib.rs), the decoded u64 values from `decode_bytes_to_vec_u64` are not necessarily binary (0 or 1). They can be any u64 value and are used as coefficients in a weighted sum with powers of 2.
📚 Learning: 2025-12-17T16:25:05.292Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1122
File: examples/CRISP/crates/crisp-utils/src/lib.rs:30-54
Timestamp: 2025-12-17T16:25:05.292Z
Learning: In the CRISP project's `decode_tally` function (examples/CRISP/crates/crisp-utils/src/lib.rs), the decoded u64 values from `decode_bytes_to_vec_u64` are not necessarily binary (0 or 1). They can be any u64 value and are used as coefficients in a weighted sum with powers of 2.
Applied to files:
examples/CRISP/server/src/server/indexer.rsexamples/CRISP/server/src/server/repo.rsexamples/CRISP/packages/crisp-sdk/src/vote.tsexamples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.solexamples/CRISP/packages/crisp-sdk/tests/vote.test.ts
📚 Learning: 2025-08-28T08:58:17.434Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 665
File: packages/enclave-sdk/src/contract-client.ts:110-118
Timestamp: 2025-08-28T08:58:17.434Z
Learning: viem's TypeScript ABI inference maps uint32[2] to [number, number], not [bigint, bigint]. The library uses number type for uint32 values to align with traditional database patterns and avoid unnecessary BigInt conversion.
Applied to files:
examples/CRISP/packages/crisp-sdk/src/vote.ts
📚 Learning: 2025-12-17T16:25:05.292Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1122
File: examples/CRISP/crates/crisp-utils/src/lib.rs:30-54
Timestamp: 2025-12-17T16:25:05.292Z
Learning: In examples/CRISP/crates/crisp-utils/src/lib.rs, when reviewing decode_tally and decode_bytes_to_vec_u64, do not assume decoded values are binary; treat each value as a coefficient (any u64) in a sum using powers of 2. Verify that arithmetic uses a weighted sum with 2^i, and that non-boolean values are handled correctly. Update tests to cover arbitrary coefficients and edge cases.
Applied to files:
examples/CRISP/crates/crisp-utils/src/lib.rs
📚 Learning: 2025-11-12T10:08:30.720Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 996
File: examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol:144-169
Timestamp: 2025-11-12T10:08:30.720Z
Learning: In the CRISP program (examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol), there are plans to move the merkle tree from the Enclave contract to the program contract itself in the future, which would allow direct calls to validateInput to be stored in the merkle tree.
Applied to files:
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol
🧬 Code graph analysis (3)
examples/CRISP/crates/crisp-utils/src/lib.rs (1)
crates/bfv-helpers/src/lib.rs (1)
decode_bytes_to_vec_u64(453-462)
examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (2)
examples/CRISP/packages/crisp-contracts/tests/utils.ts (2)
deployMockEnclave(43-47)deployCRISPProgram(69-87)examples/CRISP/client/libs/crispWorker.js (1)
vote(19-19)
examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (1)
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
decodeTally(83-117)encodeVote(26-47)
⏰ 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). (8)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_crisp_sdk
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
🔇 Additional comments (16)
examples/CRISP/packages/crisp-contracts/contracts/CRISPProgram.sol (2)
166-169: LGTM!The yes votes calculation correctly applies descending powers of 2 as weights (2^(halfD-1) down to 2^0), which aligns with the weighted coefficient scheme. Based on learnings, the decoded u64 values can be any value and are properly used as coefficients here.
172-174: LGTM!The no votes calculation correctly mirrors the yes votes logic with descending powers of 2 (2^(halfD-1) down to 2^0), ensuring consistent decoding of both vote tallies.
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (1)
15-17: LGTM!The simplified signature directly stores the bytes representation, aligning with the new tally decoding approach introduced across the PR.
examples/CRISP/server/src/server/indexer.rs (2)
18-18: LGTM!The import correctly integrates the new tally decoding utility from the crisp-utils crate.
293-305: LGTM!The decoding logic is cleanly refactored to use the centralized
decode_tallyutility. Error handling is preserved, and the vote counts are correctly passed to the repository layer.examples/CRISP/packages/crisp-contracts/tests/crisp.contracts.test.ts (2)
25-38: LGTM!The test correctly validates the new single-tally decoding path with appropriate large integer expectations (10 billion and 30 billion).
49-49: Fix vote inputs to use bigint notation to match the Vote type definition.The test uses plain numbers
{ yes: 10, no: 0 }at lines 49 and 78, but the Vote type explicitly requires bigint values. Update to{ yes: 10n, no: 0n }to align with the type signature and existing documentation.examples/CRISP/server/src/server/repo.rs (1)
169-183: LGTM!The type change from
u64toBigUintwith string storage correctly supports arbitrary-precision vote counts that can exceed 64-bit integer limits. The conversion to string viato_string()is appropriate for persistence.examples/CRISP/packages/crisp-sdk/tests/vote.test.ts (2)
48-56: LGTM!The test correctly validates the new tally decoding with expected values of 10 billion and 30 billion, matching the crisp-utils implementation.
60-79: LGTM!The test helper
decodeHalfcorrectly usesparseIntwith base 2 for internal test validation. The return type change tonumberis appropriate for the test assertions.examples/CRISP/crates/crisp-utils/src/lib.rs (1)
62-71: LGTM!The test correctly validates the decoding logic with realistic FHE output data and expected large integer counts.
examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
83-117: LGTM!The decodeTally function correctly converts hex strings to bytes and processes them in two halves using BigInt arithmetic for the weighted sum. The logic aligns with the crisp-utils Rust implementation.
183-186: LGTM!The vote validation correctly converts
MAXIMUM_VOTE_VALUEto BigInt for comparison, ensuring type consistency.examples/CRISP/server/src/server/models.rs (3)
129-136: LGTM!The type change from
u64toStringfor tally fields correctly supports arbitrary-precision vote counts. The removal oftotal_votesaligns with the refactored data model.
160-192: LGTM!The model fields consistently use
Stringfor vote tallies across E3 and E3Crisp structs, maintaining type consistency throughout the codebase.
208-219: LGTM!The conversion implementation correctly maps the updated String-based tally fields without the removed
total_votesfield.
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/server/src/server/repo.rs (1)
169-183: Fix the misleading error message.The error message on line 181 incorrectly references "ciphertext_input" when this method is setting votes.
}) }) .await - .map_err(|_| eyre::eyre!("Could not append ciphertext_input for '{key}'"))?; + .map_err(|_| eyre::eyre!("Could not set votes for '{key}'"))?; Ok(()) }
🧹 Nitpick comments (1)
examples/CRISP/server/src/server/repo.rs (1)
195-207: Minor inefficiency: redundantget_crisp()call.
get_crisp()is called on line 197, and thenget_vote_count()on line 205 internally callsget_crisp()again. Consider reusing the already-fetchede3_crispto avoid the duplicate store access.pub async fn get_web_result_request(&self) -> Result<WebResultRequest> { let e3 = self.get_e3().await?; let e3_crisp = self.get_crisp().await?; Ok(WebResultRequest { round_id: e3.id, option_1_tally: e3_crisp.votes_option_1, option_2_tally: e3_crisp.votes_option_2, option_1_emoji: e3_crisp.emojis[0].clone(), option_2_emoji: e3_crisp.emojis[1].clone(), end_time: e3.expiration, - total_votes: self.get_vote_count().await?, + total_votes: u64::try_from(e3_crisp.has_voted.len())?, }) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/CRISP/server/src/server/models.rs(4 hunks)examples/CRISP/server/src/server/repo.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1122
File: examples/CRISP/crates/crisp-utils/src/lib.rs:30-54
Timestamp: 2025-12-17T16:25:05.292Z
Learning: In the CRISP project's `decode_tally` function (examples/CRISP/crates/crisp-utils/src/lib.rs), the decoded u64 values from `decode_bytes_to_vec_u64` are not necessarily binary (0 or 1). They can be any u64 value and are used as coefficients in a weighted sum with powers of 2.
📚 Learning: 2025-12-17T16:25:05.292Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1122
File: examples/CRISP/crates/crisp-utils/src/lib.rs:30-54
Timestamp: 2025-12-17T16:25:05.292Z
Learning: In the CRISP project's `decode_tally` function (examples/CRISP/crates/crisp-utils/src/lib.rs), the decoded u64 values from `decode_bytes_to_vec_u64` are not necessarily binary (0 or 1). They can be any u64 value and are used as coefficients in a weighted sum with powers of 2.
Applied to files:
examples/CRISP/server/src/server/repo.rs
🧬 Code graph analysis (1)
examples/CRISP/server/src/server/models.rs (1)
examples/CRISP/server/src/server/indexer.rs (1)
e3(332-332)
⏰ 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). (8)
- GitHub Check: build_crisp_sdk
- GitHub Check: rust_integration
- GitHub Check: integration_prebuild
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
examples/CRISP/server/src/server/repo.rs (2)
14-14: LGTM!The
BigUintimport is correctly added to support large integer vote counts that may exceed standard integer limits.
129-130: LGTM!The initialization now uses string literals to align with the updated
E3Crispmodel wherevotes_option_1andvotes_option_2areStringtypes.examples/CRISP/server/src/server/models.rs (4)
128-137: LGTM!The
WebResultRequeststruct correctly usesStringfor tally fields to accommodate large integers serialized fromBigUint, while keepingtotal_votesasu64for the actual voter count.
171-172: LGTM!Type change to
Stringfor vote tallies is consistent with the broader refactoring to supportBigUint-derived values.
201-202: LGTM!
E3Crispmodel correctly updated to useStringfor vote fields, matching the storage format inrepo.rs.
209-220: LGTM!The
From<E3>implementation correctly maps theStringtally fields and sourcestotal_votesfromvote_countinstead of summing the tally values (which are now strings representing potentially large integers).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/CRISP/server/.env.example (1)
22-26: Reorder configuration keys alphabetically.The configuration changes look good semantically (E3_WINDOW_SIZE for activation window, E3_DURATION reduced from 160 to 60 for input submission interval). However, static analysis flags that
E3_DURATIONshould be ordered beforeE3_WINDOW_SIZEto maintain alphabetical consistency with other environment variables.🔎 Apply this diff to reorder the keys:
-# Defines the time window during which an e3 can be activated -E3_WINDOW_SIZE=40 # Defines the time interval during which users can submit their inputs # After this interval, the computation phase starts automatically E3_DURATION=60 +# Defines the time window during which an e3 can be activated +E3_WINDOW_SIZE=40
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/CRISP/server/.env.example(1 hunks)examples/CRISP/test/crisp.spec.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 1122
File: examples/CRISP/crates/crisp-utils/src/lib.rs:30-54
Timestamp: 2025-12-17T16:25:05.292Z
Learning: In the CRISP project's `decode_tally` function (examples/CRISP/crates/crisp-utils/src/lib.rs), the decoded u64 values from `decode_bytes_to_vec_u64` are not necessarily binary (0 or 1). They can be any u64 value and are used as coefficients in a weighted sum with powers of 2.
🪛 dotenv-linter (4.0.0)
examples/CRISP/server/.env.example
[warning] 26-26: [UnorderedKey] The E3_DURATION key should go before the E3_WINDOW_SIZE key
(UnorderedKey)
⏰ 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). (8)
- GitHub Check: build_crisp_sdk
- 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: Build & Push Image
🔇 Additional comments (1)
examples/CRISP/test/crisp.spec.ts (1)
47-47: Document the rationale for timeout reductions and verify CI stability.The E3 activation timeout was intentionally reduced from 300000ms to 60000ms (5x reduction), and the post-signature wait from 310000ms to 100000ms (~3x reduction). While these changes indicate performance improvements, they should be verified to ensure test reliability across CI environments, which may have different performance characteristics than local development.
Consider:
- Running this test multiple times in CI to confirm consistency
- Adding a comment explaining why these specific timeout values are sufficient
- Documenting any performance improvements that justified these reductions
2 participants
Fixes #1121
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.