feat: safe refactoring#1079
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new Rust SAFE sponge crate under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Caller (API/User)
participant Safe as SafeSponge::<L>
participant Keccak as keccak256
participant Poseidon as Poseidon2
Client->>Safe: start(io_pattern, domain_separator)
Note right of Safe: serialize IO pattern into fixed buffer\nperform runtime bounds checks
Safe->>Keccak: keccak256(buffer[..byte_count])
Keccak-->>Safe: 32-byte digest
Safe->>Safe: truncate digest → 128-bit Field tag\ninitialize state with tag
Client->>Safe: absorb(input_fields)
Safe->>Safe: validate pattern, write into state
Safe->>Poseidon: permute when rate full
Poseidon-->>Safe: permuted state
Client->>Safe: squeeze()
Safe->>Safe: validate pattern, read outputs
Safe->>Poseidon: permute as needed between squeezes
Poseidon-->>Safe: permuted state
Client->>Safe: finish()
Safe->>Safe: assert IO pattern complete, clear state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 2
🧹 Nitpick comments (6)
crates/safe/Cargo.toml (2)
7-7: Repository URL should point to the main repository, not a subdirectory.The
repositoryfield typically points to the root repository URL. Cargo/crates.io convention is to use the main repo URL.-repository = "https://github.com/gnosisguild/enclave/crates/safe" +repository = "https://github.com/gnosisguild/enclave"
10-13: Pin exact versions for cryptographic dependencies to match workspace strategy.The
ark-ffandark-bn254dependencies use semver ranges (0.5,0.2) which allow patch variations, inconsistent withsha3 = "0.10.8"and the workspace's explicit pinning strategy. For cryptographic libraries, pin exact versions:ark-ff = "0.5.0",ark-bn254 = "0.5.0", andtaceo-poseidon2 = "0.2.0".crates/safe/src/lib.rs (4)
73-74: Clarify why thetagfield is unused.The
tagfield is stored but only used during initialization. If it's intentionally kept for future use or debugging, a brief comment would help. Otherwise, consider removing it or using it infinish()for verification.
124-124: Consider accepting&[Field]instead ofVec<Field>for the absorb input.Taking
Vec<Field>by value forces callers to allocate and transfer ownership. Accepting&[Field]would be more flexible and efficient, especially for callers who want to reuse the input buffer.- pub fn absorb(&mut self, input: Vec<Field>) { - let length = input.len() as u32; + pub fn absorb(&mut self, input: &[Field]) { + let length = input.len() as u32; // ... - for input in &input { + for elem in input { // ... - self.state[self.absorb_pos + CAPACITY] += input; + self.state[self.absorb_pos + CAPACITY] += elem;This would require updating the test calls from
sponge.absorb(elements.clone())tosponge.absorb(&elements).
219-223: Consider usingzeroizefor secure memory erasure.The state is reset to zero but standard assignment doesn't guarantee secure erasure—the compiler may optimize it away or old values may remain in memory. For cryptographic code,
zeroizeprovides guaranteed secure erasure. The workspace already includeszeroize = "=1.8.1".+use zeroize::Zeroize; // In finish(): - self.state = [Field::zero(); STATE_SIZE]; + self.state.zeroize();Note: This requires that
Field(ark-bn254'sFr) implementsZeroize. If it doesn't, you may need to usezeroize::Zeroizingwrapper or manually zero the bytes.
259-259: Redundant.take(L)on fixed-size array.The array
io_patternalready has exactlyLelements, so.take(L)is redundant.- for item in io_pattern.iter().take(L) { + for item in io_pattern.iter() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(1 hunks)circuits/crates/libs/safe/Nargo.toml(1 hunks)circuits/crates/libs/safe/src/lib.nr(4 hunks)crates/safe/Cargo.toml(1 hunks)crates/safe/src/lib.rs(1 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:
Cargo.tomlcrates/safe/Cargo.toml
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
circuits/crates/libs/safe/src/lib.nr
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
circuits/crates/libs/safe/src/lib.nr
📚 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:
circuits/crates/libs/safe/Nargo.toml
⏰ 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). (9)
- GitHub Check: build_sdk
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: build_e3_support_dev
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_unit
🔇 Additional comments (7)
Cargo.toml (1)
26-26: LGTM!The new
crates/safeworkspace member is correctly added in alphabetical order, consistent with the existing member list.circuits/crates/libs/safe/src/lib.nr (3)
292-305: Buffer overflow fix looks correct.The serialization loop now properly uses the compile-time bound
Lwith the runtime guard(i < word_count) & (byte_count + 4 <= 256). This correctly addresses the buffer overflow issue mentioned in the PR description while respecting Noir's compile-time loop constraint.
308-313: Domain separator bounds checking is correct.The byte-by-byte appending with
byte_count < 256check prevents buffer overflow when the serialized IO pattern words consume most of the 256-byte buffer.
691-712: Good boundary test for large IO patterns.This test validates the 256-byte buffer limit with 64 alternating ABSORB/SQUEEZE operations. The test correctly exercises the buffer overflow fix.
crates/safe/src/lib.rs (1)
350-643: Comprehensive test coverage.The tests thoroughly cover hashing, Merkle nodes, commitment schemes, domain separation, multiple squeezes, zero-length operations, tag computation, and consecutive operation aggregation. Good coverage of edge cases.
circuits/crates/libs/safe/Nargo.toml (1)
9-10: Dependency migration from SHA256 to Keccak256 is correct. Bothkeccak256andposeidonat versionv0.1.1are available in their respective repositories and ready to use.crates/safe/Cargo.toml (1)
14-14: Thehexdependency is properly defined in the root workspace dependencies ashex = "=0.4.3". The configuration incrates/safe/Cargo.tomlusingworkspace = trueis correct and will not cause a build error.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/safe/src/lib.rs (1)
124-138: Consider taking&[Field]instead ofVec<Field>inabsorbto avoid unnecessary cloning.The docstring describes
inputas a “slice of field elements”, but the signature takes ownership of aVec<Field>:pub fn absorb(&mut self, input: Vec<Field>) { let length = input.len() as u32; ... for input in &input { ... } }Callers in this module have to clone their data (
elements.clone(),values.clone(), etc.) just to satisfy the API, even thoughabsorbonly reads from the input. You can make this more flexible and allocation‑friendly by accepting a slice:- pub fn absorb(&mut self, input: Vec<Field>) { - let length = input.len() as u32; + pub fn absorb(&mut self, input: &[Field]) { + let length = input.len() as u32; @@ - for input in &input { + for input in input {Call sites would then pass
&elements/&values, avoiding extra allocations while matching the documentation wording.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/safe/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:349-349
Timestamp: 2024-10-12T10:24:07.572Z
Learning: When reviewing `PlaintextAggregated` struct instantiations in the project, note that the `src_chain_id` field may not always need to be included. Avoid flagging its absence unless it is required in the specific context.
Applied to files:
crates/safe/src/lib.rs
⏰ 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_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: crisp_rust_unit
🔇 Additional comments (1)
crates/safe/src/lib.rs (1)
350-643: Test coverage for SAFE + Keccak256 tag behavior looks strong.The tests exercise:
- Basic hashing and determinism (
test_safe_hashing,test_merkle_node,test_commitment_scheme).- Domain separation influence (
test_domain_separation).- Multiple squeezes and zero‑length operations.
- Tag aggregation correctness for ABSORB and SQUEEZE runs, including mixed patterns.
- A maximal‑size IO pattern hitting the former “word_count” / buffer edge case.
Given the core implementation changes, this level of coverage is solid and should catch most regressions around IO pattern handling and tag computation behavior.
|
ready @cedoor |
- Updated @crisp-e3/sdk to 0.4.2 - Updated @crisp-e3/contracts to 0.4.2 - Updated @crisp-e3/zk-inputs to 0.4.2 - Published to npm
cedoor
force-pushed
the
feat/safe-rust
branch
from
a319f80 to
ae85ffe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol (1)
11-41: VK hash and VK points look consistent; please confirm they’re regenerated from the new circuitThe updated
VK_HASHand all G1 points inloadVerificationKey()look like a fresh VK dump, and they are numerically consistent with the top‑level constants (N,LOG_N,NUMBER_OF_PUBLIC_INPUTS) and theBaseZKHonkVerifierconstructor wiring.Given these are opaque constants, the main risk is a mismatch between:
- the Noir / Rust SAFE + Keccak circuit and proving key, and
- this on‑chain verification key +
VK_HASHdomain separator.Please double‑check that:
- this VK was generated from the exact circuit/proving key that the SAFE/Keccak refactor now uses, and
VK_HASHis recomputed from the same preimage as whatever the off‑circuit tooling passes into proof generation.If that’s already covered by your build scripts/tests (e.g. end‑to‑end proof verification against this contract), then this change looks good as‑is. Otherwise, consider adding a small regeneration/check script or CI step so future VK updates can’t drift silently.
Also applies to: 43-73, 75-89, 91-105, 107-121, 127-128
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
Cargo.toml(1 hunks)circuits/crates/libs/safe/Nargo.toml(1 hunks)circuits/crates/libs/safe/src/lib.nr(5 hunks)crates/Dockerfile(1 hunks)crates/safe/Cargo.toml(1 hunks)crates/safe/src/lib.rs(1 hunks)examples/CRISP/client/package.json(1 hunks)examples/CRISP/packages/crisp-contracts/contracts/CRISPVerifier.sol(1 hunks)examples/CRISP/packages/crisp-contracts/package.json(1 hunks)examples/CRISP/packages/crisp-sdk/package.json(1 hunks)examples/CRISP/packages/crisp-zk-inputs/package.json(1 hunks)examples/CRISP/scripts/publish.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/CRISP/client/package.json
- examples/CRISP/packages/crisp-zk-inputs/package.json
- examples/CRISP/scripts/publish.ts
- circuits/crates/libs/safe/Nargo.toml
- crates/Dockerfile
- examples/CRISP/packages/crisp-sdk/package.json
🧰 Additional context used
🧠 Learnings (10)
📚 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:
crates/safe/Cargo.tomlCargo.toml
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
circuits/crates/libs/safe/src/lib.nrcrates/safe/src/lib.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:137-139
Timestamp: 2024-10-08T19:45:18.209Z
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:
circuits/crates/libs/safe/src/lib.nrcrates/safe/src/lib.rs
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
circuits/crates/libs/safe/src/lib.nr
📚 Learning: 2024-10-08T01:48:49.778Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/aggregator/src/publickey_aggregator.rs:46-46
Timestamp: 2024-10-08T01:48:49.778Z
Learning: In `packages/ciphernode/router/src/hooks.rs`, the `src_chain_id` parameter in `PublicKeyAggregator::new` is correctly handled, even if not explicitly provided during instantiation.
Applied to files:
circuits/crates/libs/safe/src/lib.nr
📚 Learning: 2024-10-12T10:24:07.572Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 139
File: packages/ciphernode/core/src/events.rs:349-349
Timestamp: 2024-10-12T10:24:07.572Z
Learning: When reviewing `PlaintextAggregated` struct instantiations in the project, note that the `src_chain_id` field may not always need to be included. Avoid flagging its absence unless it is required in the specific context.
Applied to files:
crates/safe/src/lib.rs
📚 Learning: 2025-09-19T11:16:53.825Z
Learnt from: cedoor
Repo: gnosisguild/enclave PR: 752
File: packages/enclave-contracts/contracts/Enclave.sol:15-17
Timestamp: 2025-09-19T11:16:53.825Z
Learning: The Enclave contract in the gnosisguild/enclave repository has not been deployed yet as of September 2025, so storage layout considerations for upgradeable contracts don't apply to current changes.
Applied to files:
crates/safe/src/lib.rs
📚 Learning: 2025-10-27T15:37:59.138Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 904
File: crates/net/src/bin/p2p_test.rs:22-45
Timestamp: 2025-10-27T15:37:59.138Z
Learning: In test files (especially integration test binaries like p2p_test.rs), suggesting correlation ID filtering for DHT events may be unnecessarily complex since test environments are typically more controlled and false positives are less of a concern.
Applied to files:
crates/safe/src/lib.rs
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 133
File: packages/ciphernode/enclave/src/main.rs:5-18
Timestamp: 2024-10-08T19:45:18.209Z
Learning: Including the OWO ASCII art in `packages/ciphernode/enclave/src/main.rs` is acceptable, even if it increases the binary size.
Applied to files:
crates/safe/src/lib.rs
📚 Learning: 2024-10-22T03:39:29.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89
Timestamp: 2024-10-22T03:39:29.448Z
Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.
Applied to files:
crates/safe/src/lib.rs
⏰ 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). (9)
- GitHub Check: build_sdk
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
- GitHub Check: crisp_unit
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: build_ciphernode_image
- GitHub Check: test_contracts
- GitHub Check: rust_unit
🔇 Additional comments (15)
examples/CRISP/packages/crisp-contracts/package.json (1)
3-3: LGTM: Version bump aligns with new SAFE crate.The patch version increment from 0.4.1 to 0.4.2 is appropriate for this PR's additions.
Cargo.toml (1)
26-26: LGTM: Workspace member addition is correct.The new
crates/safemember is properly integrated into the workspace and positioned alphabetically.circuits/crates/libs/safe/src/lib.nr (4)
159-160: LGTM: IO pattern bounds check prevents index out of bounds.The validation
assert(self.io_count < L)correctly prevents accessingio_pattern[self.io_count]beyond array bounds and provides a clear error message.
290-322: LGTM: Buffer management correctly prevents domain separator truncation.The fixed 256-byte buffer with explicit limits ensures:
- Maximum 48 aggregated words (192 bytes) for IO patterns
- Full 64-byte domain separator always included
- Fail-fast assertion for oversized patterns
This addresses the previous domain separator truncation concern while working within Noir's fixed-size array constraints.
324-327: LGTM: Keccak-256 usage is correct and well-documented.The switch to
keccak256with clarifying comments about the distinction from SHA3-256 ensures cross-implementation consistency between Noir and Rust.
701-762: LGTM: Excellent edge case coverage.The new tests validate critical security properties:
test_large_io_pattern: Verifies 48-word limit works correctlytest_domain_separator_not_truncated: Regression test ensuring domain separators affect tags even at maximum pattern sizeThese directly test the fixes for the domain separator truncation issue.
crates/safe/src/lib.rs (8)
24-51: LGTM: Type definitions and constants are correct.The BN254 field type and sponge parameters (RATE=3, CAPACITY=1) are properly defined and match the Noir implementation.
68-83: LGTM: SafeSponge struct correctly implements SAFE specification.The structure properly tracks sponge state per SAFE spec 2.2, with all required fields for domain-separated operation.
95-112: LGTM:startmethod correctly initializes sponge state.Tag computation and capacity initialization follow SAFE spec 2.4 exactly.
124-162: LGTM:absorbimplementation follows SAFE spec correctly.Proper validation, permutation handling, and state management per specification 2.4.
173-208: LGTM:squeezeimplementation is correct and symmetric withabsorb.The IO pattern exhaustion check at Line 175 mirrors
absorb's validation, providing consistent error handling.
218-227: LGTM:finishproperly validates and clears state.Completion check and state erasure follow SAFE spec 2.4 security requirements.
254-356: LGTM:compute_tagimplementation is secure and correct.The function properly:
- Aggregates consecutive operations per SAFE spec 2.3
- Uses fixed 256-byte buffer with 48-word limit
- Always includes full 64-byte domain separator
- Applies Keccak-256 and truncates to 128 bits
The implementation matches the Noir version and addresses previous domain separator truncation concerns.
358-717: LGTM: Excellent test coverage validates correctness and security.The test suite comprehensively covers:
- Basic functionality and determinism
- Multiple operation patterns
- Domain separation security
- Edge cases (zero length, maximum size)
- Regression tests for previous bugs
- IO pattern exhaustion error handling
This provides high confidence in implementation correctness.
crates/safe/Cargo.toml (1)
9-14: Consider updating sha3 to a stable 0.11.x version when released.The dependencies are generally current. However, sha3 is at 0.10.8 while version 0.11.0-rc.3 is available (released September 2025). The arkworks libraries (ark-ff, ark-bn254) are at stable 0.5.0 from October 2024, and taceo-poseidon2 is at 0.2.0 from November 2025. When sha3 0.11.x reaches stable release, plan an upgrade to ensure access to the latest cryptographic improvements.
3 participants
word_countloop.keccak256instead ofsha256(sha3 over sha2 family).Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Closes #1064