refactor: move pvss and pvss-cli as zk-helpers mods [skip-line-limit]#1258
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR migrates PVSS-specific functionality from dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
0xjei
changed the title
pvss and pvss-cli as zk-helpers submodulespvss and pvss-cli as zk-helpers submodules
0xjei
changed the title
pvss and pvss-cli as zk-helpers submodules![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
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/Cargo.toml`:
- Line 28: The Cargo.toml currently declares tempfile as a regular dependency
but it’s only used in test-only modules (seen in codegen.rs and
pk_bfv/codegen.rs); move the tempfile entry from the main [dependencies] section
into the [dev-dependencies] section of Cargo.toml so it’s only enabled for
tests.
🧹 Nitpick comments (3)
crates/zk-helpers/src/utils.rs (1)
160-160: Prefer&[Vec<BigInt>]over&Vec<Vec<BigInt>>.Using a slice reference is more idiomatic in Rust and allows callers to pass both
Vecand slices without explicit conversion.♻️ Suggested change
-pub fn map_witness_2d_vector_to_json(values: &Vec<Vec<BigInt>>) -> Vec<serde_json::Value> { +pub fn map_witness_2d_vector_to_json(values: &[Vec<BigInt>]) -> Vec<serde_json::Value> {crates/zk-helpers/src/registry/registry.rs (1)
19-26: Consider adding common derives toDkgInputType.Adding
Debug,Copy, andPartialEqwould improve usability for testing, logging, and comparisons.♻️ Suggested change
-#[derive(Clone)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum DkgInputType {crates/zk-helpers/src/bin/zk_cli.rs (1)
100-113: Defer parameter/sample generation until circuit dispatch.Avoids unnecessary work when the circuit is not implemented or future circuits use different parameter types.
♻️ Suggested refactor
- let params = BfvParamSet::from(preset).build_arc(); - let sample = Sample::generate(¶ms); let circuit_name = circuit_meta.name(); let artifacts = match circuit_name { name if name == <PkBfvCircuit as Circuit>::NAME => { let circuit = PkBfvCircuit; + let params = BfvParamSet::from(preset).build_arc(); + let sample = Sample::generate(¶ms); circuit.codegen(PkBfvCodegenInput { preset, public_key: sample.public_key, })? }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/registry/registry.rs`:
- Around line 92-93: Update the doc comment for the struct CircuitRegistry to
remove the term "PVSS" (and any variant like "pvss") so it no longer references
PVSS terminology; find the docstring immediately above pub struct
CircuitRegistry and replace wording that says "Registry for PVSS circuits." with
a neutral description (e.g., "Registry for circuits." or "Registry for
threshold-recovery circuits.") that matches the new naming convention.
🧹 Nitpick comments (4)
crates/zk-helpers/src/circuits/sample.rs (2)
25-32: Consider usingOsRngfor cryptographic key generation.The code uses
thread_rng()for generating cryptographic keys. Whilethread_rng()is cryptographically secure, the codebase pattern intemplates/default/program/src/lib.rs(lines 48-49) usesOsRngforSecretKey::random. For consistency and to source entropy directly from the OS for key material, consider switching toOsRng.♻️ Suggested change
-use rand::thread_rng; +use rand::rngs::OsRng; use std::sync::Arc; // ... pub fn generate(params: &Arc<BfvParameters>) -> Self { - let mut rng = thread_rng(); - - let secret_key = SecretKey::random(¶ms, &mut rng); - let public_key = PublicKey::new(&secret_key, &mut rng); + let secret_key = SecretKey::random(¶ms, &mut OsRng); + let public_key = PublicKey::new(&secret_key, &mut OsRng); Self { public_key } }
41-47: Test assertion accesses internal structure.The assertion
sample.public_key.c.c.len() == 2accesses the internalcfield structure ofPublicKey. This creates coupling to thefhelibrary's internal representation. If this is intentional to validate the BFV public key structure (two polynomials), consider adding a brief comment explaining what's being verified.crates/zk-helpers/src/utils.rs (1)
160-169: Prefer slice reference over&Vec.The parameter type
&Vec<Vec<BigInt>>can be simplified to&[Vec<BigInt>]. This is more idiomatic Rust and allows the function to accept bothVecreferences and slices.♻️ Suggested change
-pub fn map_witness_2d_vector_to_json(values: &Vec<Vec<BigInt>>) -> Vec<serde_json::Value> { +pub fn map_witness_2d_vector_to_json(values: &[Vec<BigInt>]) -> Vec<serde_json::Value> { values .iter() .map(|value| { serde_json::json!({ "coefficients": to_string_1d_vec(value) }) }) .collect() }crates/zk-helpers/src/bin/zk_cli.rs (1)
104-113: Consider simplifying the match pattern.The current match uses
name if name == <PkBfvCircuit as Circuit>::NAMEwhich is verbose. Since there's currently only one circuit, a simple string literal match would be cleaner. As more circuits are added, consider using a dispatch table or the registry itself for codegen dispatch.♻️ Suggested simplification
let artifacts = match circuit_name { - name if name == <PkBfvCircuit as Circuit>::NAME => { + "pk-bfv" => { let circuit = PkBfvCircuit; circuit.codegen(PkBfvCodegenInput { preset, public_key: sample.public_key, })? } name => return Err(anyhow!("circuit {} not yet implemented", name)), };Alternatively, if the intent is to avoid hardcoding the string, the constant could be used directly in a cleaner way or the registry could be extended to dispatch codegen.
0xjei
changed the title
0xjei
changed the title
2 participants
closes #1248
Summary by CodeRabbit
New Features
Chores