chore: update bfv-helpers dependencies [skip-line-limit]#883
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUpdate bfv-helpers: swap workspace-based greco/fhe deps for explicit git dependencies, bump several crate versions, and adapt client code to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant BFVHelpers as "bfv-helpers::client"
participant Greco as "GrecoVectors"
participant BFV as "BFV params / ciphertext"
rect #E8F5E9
Client->>BFVHelpers: request prepare_greco_inputs(...)
BFVHelpers->>Greco: GrecoVectors::compute(ciphertext, pub_key, bfv_params)
note right of Greco: compute produces internal vectors
Greco-->>BFVHelpers: GrecoVectors
BFVHelpers->>Greco: standard_form()
Greco-->>BFVHelpers: circuit_inputs
BFVHelpers-->>Client: circuit_inputs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
cedoor
force-pushed
the
chore/bfv-helpers
branch
from
aca8667 to
231c2d9
Compare
cedoor
force-pushed
the
chore/bfv-helpers
branch
from
a93c90c to
09d318b
Compare
cedoor
changed the title
cedoor
force-pushed
the
chore/bfv-helpers
branch
from
6c154ab to
c046f37
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/bfv-helpers/src/client.rs (2)
24-33: Docstring inaccuracies for bfv_encrypt.
- Mentions “Input validation vector computation fails” though this function doesn’t compute them.
- Says “Vector of moduli” but type is [u64; 1].
Suggested fix:
/// # Errors /// Returns error string if: /// - Public key deserialization fails /// - Plaintext encoding fails -/// - Encryption fails -/// - Input validation vector computation fails +/// - Encryption failsAnd update the arg doc where “Vector of moduli” appears to “Array of moduli (RNS)”.
75-84: Return type mismatch in bfv_verifiable_encrypt docs.Docs say Result<VerifiableEncryptionResult, String>, but function returns anyhow::Result<...>.
Apply:
-/// * `Result<VerifiableEncryptionResult, String>` - Contains encrypted u64 and circuit inputs for ZKP +/// * `Result<VerifiableEncryptionResult>` - Contains ciphertext bytes and circuit inputs for ZKPAlso, mirror the terminology used (“ciphertext bytes”) for clarity.
🧹 Nitpick comments (4)
crates/bfv-helpers/src/client.rs (2)
118-118: Avoid stringify/parse churn: return JSON Value instead of String.Prefer serde_json::Value in VerifiableEncryptionResult to avoid extra allocation and future double-parsing.
Apply:
@@ #[derive(Debug, Clone)] pub struct VerifiableEncryptionResult { pub encrypted_data: Vec<u8>, - pub circuit_inputs: String, + pub circuit_inputs: serde_json::Value, } @@ Ok(VerifiableEncryptionResult { encrypted_data: cipher_text.to_bytes(), - circuit_inputs: standard_input_val.to_json().to_string(), + circuit_inputs: standard_input_val.to_json(), })
181-200: Tests don’t exercise circuit_inputs; assert it’s valid JSON.Add minimal checks that circuit_inputs parses and is non-empty. This guards regressions in Greco standard_form/to_json.
Apply:
@@ fn test_bfv_verifiable_encrypt_a64() { use fhe::bfv::{Ciphertext, PublicKey, SecretKey}; use fhe_traits::{DeserializeParametrized, FheDecrypter, Serialize}; + use serde_json as json; @@ - let encrypted_data = - bfv_verifiable_encrypt(num, pk.to_bytes(), degree, plaintext_modulus, moduli).unwrap(); + let encrypted_data = + bfv_verifiable_encrypt(num, pk.to_bytes(), degree, plaintext_modulus, moduli).unwrap(); + + // Validate circuit_inputs JSON + let v: json::Value = json::from_str( + encrypted_data.circuit_inputs.as_str().unwrap_or_default() + ).unwrap(); + assert!(v.is_object()); @@ fn test_bfv_verifiable_encrypt_v64() { use fhe::bfv::{Ciphertext, PublicKey, SecretKey}; use fhe_traits::{DeserializeParametrized, FheDecrypter, Serialize}; + use serde_json as json; @@ - let encrypted_data = bfv_verifiable_encrypt( + let encrypted_data = bfv_verifiable_encrypt( num.clone(), pk.to_bytes(), degree, plaintext_modulus, moduli, ) .unwrap(); + + // Validate circuit_inputs JSON + let v: json::Value = json::from_str( + encrypted_data.circuit_inputs.as_str().unwrap_or_default() + ).unwrap(); + assert!(v.is_object());If you adopt the Value-returning API above, drop the as_str() and parsing, and assert!(encrypted_data.circuit_inputs.is_object()) directly.
Also applies to: 202-229
Cargo.toml (1)
99-104: Unify alloy features at workspace level to avoid drift.examples/CRISP/server requires "rpc-types-eth". Consider enabling it here so dependents can use alloy.workspace = true instead of pinning their own version.
Apply:
-alloy = { version = "=1.0.41", features = ["node-bindings", "full"] } +alloy = { version = "=1.0.41", features = ["node-bindings", "full", "rpc-types-eth"] }examples/CRISP/server/Cargo.toml (1)
30-30: Prefer using the workspace alloy to prevent version drift.Once the root adds "rpc-types-eth", switch to alloy.workspace = true here.
Proposed change (after root update):
-alloy = { version = "=1.0.41", features = ["full", "rpc-types-eth"] } +alloy.workspace = truePlease confirm that alloy’s "rpc-types-eth" feature API remained stable between 1.0.23 → 1.0.41 for your usages.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(2 hunks)README.md(1 hunks)crates/bfv-helpers/Cargo.toml(1 hunks)crates/bfv-helpers/src/client.rs(3 hunks)examples/CRISP/Cargo.toml(1 hunks)examples/CRISP/server/Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/bfv-helpers/Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
PR: gnosisguild/enclave#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:
Cargo.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). (10)
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (4)
crates/bfv-helpers/src/client.rs (2)
12-12: Greco import path change looks right—please confirm crate module path.If the e3-greco-generator crate re-exports as greco::vectors::GrecoVectors in the referenced git ref, this is good. Otherwise, adjust the path.
Run a quick compile or check the crate docs to confirm the module path is greco::vectors::GrecoVectors and not a renamed namespace.
107-116: Verify GrecoVectors::compute parameter order and RNS base expectations.The code calls
GrecoVectors::compute()with parameters:(plaintext, u_rns, e0_rns, e1_rns, cipher_text, pk, params). However, I cannot access the currentGrecoVectors::computesignature from the greco crate (https://github.com/gnosisguild/greco, main branch) to verify:
- Whether the parameter order matches the current API.
- Whether u_rns, e0_rns, e1_rns are in the base expected by Greco.
PRs #9 and #10 have been merged to main (Oct 20 & 23, 2025), but without direct access to the greco repository or a pinned commit/ref in Cargo.toml, the exact API cannot be confirmed.
examples/CRISP/Cargo.toml (1)
33-33: LGTM on serde bump.Version and features align with the root; explicit "std" is fine.
Cargo.toml (1)
153-153: ✅ Verified: No duplicate serde or alloy versions in the workspace build graph.The cargo tree output shows only serde v1.0.228 in the resolved dependency graph—no duplicates. The note about
crates/supportv1.0.219 is not a concern: it's explicitly excluded from the workspace with the comment "client needs to be able to build crates/support independently," so it builds outside the workspace independently. The serde bump in the workspace is clean.
3 participants
Closes #865
Depends on: gnosisguild/zkfhe-generator#9 and gnosisguild/zkfhe-generator#10
Summary by CodeRabbit
Chores
Refactor
Documentation