Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds a new workspace crate Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (CRISP / callers)
participant Packer as Packer (e3-zk-helpers::packing)
participant Conv as Converter (e3-zk-helpers::utils::bigint_to_field)
participant Safe as SAFE (e3-safe / compute_safe)
Caller->>Packer: flatten(polynomial arrays, bit) -> Vec<Field>
Packer-->>Caller: flattened payload (Vec<Field))
Caller->>Safe: compute_safe(DS_CIPHERTEXT, payload, io_pattern)
Safe-->>Caller: squeezed fields (Vec<Field))
Caller->>Conv: convert first Field -> BigInt (commitment)
Conv-->>Caller: commitment BigInt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
0xjei
changed the title
zk-helpers cratezk-helpers crate [skip-line-limit]
0xjei
changed the title
zk-helpers crate [skip-line-limit]![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
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/commitments.rs`:
- Around line 296-300: Before indexing coeff_y[mod_idx][party_idx + 1], add
explicit bounds validation: verify mod_idx < coeff_y.len() and party_idx + 1 <
coeff_y[mod_idx].len(); if the check fails, return a descriptive error
(propagate Result) from the surrounding function or, if you cannot change the
signature, early-return or panic with a clear message indicating invalid
mod_idx/party_idx and the offending values; after the guards, continue pushing
crate::utils::bigint_to_field(share_value) into payload as before.
In `@crates/zk-helpers/src/utils.rs`:
- Around line 125-138: The calculate_bit_width function currently overcounts
bits by adding an extra +1 to bound.bits(); fix it so bit_width is computed from
(bound - 1).bits() + 1 for positive bounds: for calculate_bit_width, after
parsing bound and handling bound <= 0, compute a new BigInt bound_minus_one =
&bound - BigInt::from(1) and set bit_width = (bound_minus_one.bits() as u32) + 1
(instead of using bound.bits() and the extra +1); this aligns packing logic used
by compute_ciphertext_commitment/flatten and fixes off-by-one for powers of two.
In `@templates/default/program/src/lib.rs`:
- Around line 51-55: The inline comment "// 10" is stale relative to the
encrypted value [3u64]; update the code so the comment and variable name match
the value—either change the comment to "// 3" or rename the variable ten to
three and update all subsequent uses (e.g., the variable declared at let ten =
public_key.try_encrypt(...) and any later reference on line 66) to keep
identifiers and comments consistent with Plaintext::try_encode(&[3u64], ...).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@templates/default/program/src/lib.rs`:
- Around line 51-55: The test vector is mislabeled: the variable ten is
encrypting the plaintext [3] via
public_key.try_encrypt(Plaintext::try_encode(&[3u64], Encoding::poly(),
¶ms)...) while the inline comment "// 10" is incorrect (and the expected sum
is 5). Fix by either renaming the variable ten to three (or a more descriptive
name like encrypted_three) wherever it's declared/used, or update the comment to
reflect the actual value (e.g., "// 3"); ensure the same fix is applied to the
second occurrence around the other instance (line ~75) so
Plaintext::try_encode(&[3u64], ...) and any uses of ten are consistent.
🧹 Nitpick comments (7)
crates/polynomial/Cargo.toml (1)
2-7: Repository URL may need updating for consistency.The package was renamed to
e3-polynomial, but the repository URL still references the old pathcrates/polynomial. Consider updating to reflect the new naming convention if the directory will be renamed, or verify this is intentional.examples/CRISP/circuits/src/main.nr (1)
173-175: Use config constants for commitment generics.
Hard‑coding512, 2, 36risks drift if GRECO configs change; usingN,L, andGRECO_BIT_CTkeeps commitments aligned.♻️ Suggested update
- let ct_commitment = compute_ciphertext_commitment::<512, 2, 36>(ct0is, ct1is); + let ct_commitment = compute_ciphertext_commitment::<N, L, GRECO_BIT_CT>(ct0is, ct1is); ... - let _prev_ct_commitment = - compute_ciphertext_commitment::<512, 2, 36>(prev_ct0is, prev_ct1is); - let sum_ct_commitment = - compute_ciphertext_commitment::<512, 2, 36>(sum_ct0is, sum_ct1is); + let _prev_ct_commitment = + compute_ciphertext_commitment::<N, L, GRECO_BIT_CT>(prev_ct0is, prev_ct1is); + let sum_ct_commitment = + compute_ciphertext_commitment::<N, L, GRECO_BIT_CT>(sum_ct0is, sum_ct1is);Also applies to: 190-193
Cargo.toml (1)
117-118: Pin arkworks versions to match workspace pinning policy.
External dependencies in the workspace use exact-pinned versions (e.g.,actix = "=0.13.5"), butark-ffandark-bn254use semver ranges ("0.5"). For consistency and reproducibility, consider pinning both to specific patch versions.crates/zk-helpers/src/utils.rs (3)
16-17: Consider consolidating the redundant type aliases.Both
FieldandFieldElementalias the same typeark_bn254::Fr. If both names are intentionally used for semantic distinction in different contexts, consider adding a brief comment explaining their purpose. Otherwise, consolidate to a single alias.♻️ Suggested simplification
-use ark_bn254::Fr as Field; -use ark_bn254::Fr as FieldElement; +use ark_bn254::Fr as Field; + +// Re-export for compatibility if needed elsewhere +pub type FieldElement = Field;
101-113: The negative value handling inbigint_to_fieldhas redundant fallback logic.Line 104 computes
(value % zkp_modulus) + &zkp_modulusfor negative values, which should always produce a value in[0, modulus). Theunwrap_or_elsefallback at lines 108-110 handles the case whereto_biguint()fails, but after the modular reduction this should never happen.Consider simplifying:
♻️ Suggested simplification
pub fn bigint_to_field(value: &BigInt) -> FieldElement { let zkp_modulus = get_zkp_modulus(); - let reduced = if value < &BigInt::zero() { - (value % &zkp_modulus) + &zkp_modulus - } else { - value % &zkp_modulus - }; - let biguint = reduced - .to_biguint() - .unwrap_or_else(|| (&zkp_modulus + reduced).to_biguint().unwrap()); + let reduced = ((value % &zkp_modulus) + &zkp_modulus) % &zkp_modulus; + let biguint = reduced + .to_biguint() + .expect("reduced value should be non-negative after modular reduction"); let bytes = biguint.to_bytes_le(); FieldElement::from_le_bytes_mod_order(&bytes) }
131-139: Clarify the bit-width calculation formula in the docstring.The docstring states the formula is
ceil(log₂(bound)) + 1, butBigInt::bits()already returnsfloor(log₂(n)) + 1for positiven. The actual computation is therefore approximatelyceil(floor(log₂(bound)) + 1) + 1, which may yield more bits than expected for exact powers of 2.If this extra margin is intentional for the ZK circuit requirements, a clarifying comment would help future maintainers understand the rationale.
crates/zk-helpers/src/commitments.rs (1)
472-487: Refactor to returnResultinstead of panicking on PK commitment mismatch.
This is a public API function that should gracefully handle invalid inputs through error types rather than panic. The function currently panics whenpk_commitmentdoesn't match the computed commitment; returningResult<Vec<BigInt>, E>would allow callers to decide how to handle this validation failure. The crate already usesthiserrorfor error handling, supporting this pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Around line 118-119: Update the ark-* crate entries in Cargo.toml to use exact
patch pins like the rest of the workspace: change the ark-ff and ark-bn254
dependency specifications to use the exact version string (e.g., "=0.5.0") so
they follow the same pinning pattern as other crates; update the entries named
ark-ff and ark-bn254 accordingly.
2 participants
This PR adds the
zk-helperscrate in order to get rid of most of the utilities for zkp fromzkfhe-shared.Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.