refactor: rename circuit_input to circuit_data [skip-line-limit]#1304
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRenames public circuit input types from Changes
🎯 4 (Complex) | ⏱️ ~60 minutes Sequence Diagram(s)Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/zk-helpers/src/circuits/threshold/pk_generation/circuit.rs (1)
23-31:⚠️ Potential issue | 🟡 MinorUpdate documentation reference in mod.rs — struct was renamed from
PkGenerationCircuitInputtoPkGenerationCircuitData.All code construction sites correctly use the new struct name with the three new
CrtPolynomialfields (eek,e_sm,sk). However, the documentation comment incrates/zk-helpers/src/circuits/threshold/pk_generation/mod.rs:11still references the old type name and should be updated toPkGenerationCircuitData.crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs (1)
22-22:⚠️ Potential issue | 🟡 MinorStale reference to
ShareComputationCircuitInputin comment.Line 22 still references the old type name
ShareComputationCircuitInput::dkg_input_type. Should be updated toShareComputationCircuitData.Proposed fix
- /// None: circuit accepts runtime-varying input type (SecretKey or SmudgingNoise via `ShareComputationCircuitInput::dkg_input_type`). + /// None: circuit accepts runtime-varying input type (SecretKey or SmudgingNoise via `ShareComputationCircuitData::dkg_input_type`).
🤖 Fix all issues with AI agents
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Around line 23-26: The import alias re-introduces the old "Input" suffix;
change the alias from ShareDecryptionCircuitData as
DkgShareDecryptionCircuitInput to ShareDecryptionCircuitData as
DkgShareDecryptionCircuitData, then update all usages of
DkgShareDecryptionCircuitInput (e.g., the construction/typing site where the
share decryption circuit data is used) to the new DkgShareDecryptionCircuitData
identifier so names are consistent with the PR rename.
- Around line 35-38: The import alias incorrectly preserves the old "Input"
naming: change the import alias from "ShareDecryptionCircuitData as
ThresholdShareDecryptionCircuitInput" to "ShareDecryptionCircuitData as
ThresholdShareDecryptionCircuitData", and then update any usages of the old
alias (e.g., the variable/type referenced as
ThresholdShareDecryptionCircuitInput around the code that constructs or reads
the share decryption input) to use ThresholdShareDecryptionCircuitData instead
so references to the circuit data type match the renamed import.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 45-48: The CircuitComputation::compute method currently uses the
parameter name `input` but should be `data` to match other Computation::compute
implementations; rename the parameter from `input` to `data` in the compute
signature and update all usages inside the function (calls to Bounds::compute,
Bits::compute, Inputs::compute and any local references) so they pass `data`
instead of `input`, keeping the rest of the method body unchanged.
🧹 Nitpick comments (13)
crates/zk-helpers/src/circuits/dkg/share_encryption/circuit.rs (1)
30-31: Nit: stale doc comment still says "Input to the share-encryption circuit".Should read "Data for the share-encryption circuit" (or similar) to match the renamed struct.
Suggested fix
-/// Input to the share-encryption circuit: plaintext, ciphertext, keys, and encryption randomness. +/// Data for the share-encryption circuit: plaintext, ciphertext, keys, and encryption randomness.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/circuit.rs (1)
24-27: Nit: doc comment on Line 24 still references "input" twice.Consider updating to align with the
*Datanaming convention used throughout this PR.Suggested fix
-/// Raw input for circuit input computation: decryption share polynomials from T+1 parties, +/// Raw data for circuit computation: decryption share polynomials from T+1 parties,crates/zk-helpers/src/circuits/threshold/share_decryption/circuit.rs (1)
7-7: Nit: doc comment still references "input" instead of "data".Line 7:
//! Circuit type and input for threshold share decryption.— consider updating to match the rename convention.Proposed fix
-//! Circuit type and input for threshold share decryption. +//! Circuit type and data for threshold share decryption.crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs (1)
27-27: Slightly awkward phrasing in doc comment.
"Which secret type this data is for (determines which branch to use in data)"— the trailing "in data" reads oddly. Consider rephrasing.Proposed fix
- /// Which secret type this data is for (determines which branch to use in data). + /// Which secret type this data is for (determines which branch the circuit uses).crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
53-55: Stale "input" wording in error message.Line 54 still says
"input json is not an object". Consider updating to"data json is not an object"for consistency with the rename.Suggested fix
let obj = json.as_object_mut().ok_or(CircuitsErrors::Other( - "input json is not an object".to_string(), + "data json is not an object".to_string(), ))?;crates/zk-helpers/src/circuits/computation.rs (1)
9-10: Doc comments still reference "input" in multiple places.Lines 9, 22, 28, 41, and 48 still mention "input" in doc comments (e.g., "parameters and input", "parameters and input produce bounds"). Consider updating these to say "data" for consistency with the rename.
Also applies to: 22-22, 28-28, 41-41, 48-48
crates/zk-helpers/src/circuits/dkg/share_encryption/sample.rs (1)
68-81: Pre-existing: double.map_erron smudging error generation.Lines 70-81 chain two identical
.map_err(...)calls ongenerate_smudging_error. The second one wraps the already-mappedCircuitsErrorsin anotherCircuitsErrors::Sample(...), producing a nested debug-format string. Not introduced by this PR, but worth a cleanup.Suggested fix — remove the duplicate `.map_err`
let esi_coeffs = trbfv .generate_smudging_error(num_ciphertexts as usize, lambda as usize, &mut rng) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate smudging error: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate smudging error: {:?}", - e - )) })?;crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs (2)
86-91: Pre-existing: duplicate.map_errchain on smudging error generation.Lines 80–91 chain two identical
.map_err(|e| CircuitsErrors::Sample(...))calls. The first one already converts the error, so the second is a no-op identity mapping. Not introduced by this PR, but worth cleaning up.♻️ Remove redundant `.map_err`
let esi_coeffs = trbfv .generate_smudging_error(sd.z as usize, sd.lambda as usize, &mut rng) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate smudging error: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate smudging error: {:?}", - e - )) })?;
98-111: Pre-existing: same duplicate.map_errpattern on error shares generation.Lines 98–111 also chain two identical
.map_errcalls. Same cleanup opportunity as above.♻️ Remove redundant `.map_err`
let esi_sss_u64 = share_manager .generate_secret_shares_from_poly(esi_poly.clone(), &mut rng.clone()) .map_err(|e| { CircuitsErrors::Sample(format!( "Failed to generate error shares: {:?}", e )) - }) - .map_err(|e| { - CircuitsErrors::Sample(format!( - "Failed to generate error shares: {:?}", - e - )) })?;crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/codegen.rs (1)
115-116: Nit: local variable still namedinput.The test variable on line 115 is still named
inputwhile the rest of the PR consistently usesdataorsamplefor this concept. Consider renaming for consistency with the broader refactor.♻️ Rename local variable
- let input = + let data = DecryptedSharesAggregationCircuitData::generate_sample(preset, committee).unwrap(); let circuit = DecryptedSharesAggregationCircuit; - let artifacts = circuit.codegen(preset, &input).unwrap(); + let artifacts = circuit.codegen(preset, &data).unwrap();crates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rs (1)
43-46: Unnecessary double-reference&dataon line 46.
datais already&Self::Data(i.e.,&PkAggregationCircuitData), so&datacreates&&PkAggregationCircuitData. Rust auto-derefs, so this compiles, but it's inconsistent with all otherCircuitComputation::computeimplementations which passdatadirectly.Proposed fix
- let inputs = Inputs::compute(preset, &data)?; + let inputs = Inputs::compute(preset, data)?;crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (2)
7-11: Stale doc references toInputafter the rename.Lines 10–11 still mention "Input coefficients" and
[`Input::standard_form`]. Since the struct isInputs(and the PR is specifically about this rename), consider updating these doc strings to stay consistent.Suggested diff
-//! Bounds, configs, bits, and input computation for the Decryption Share Aggregation TRBFV circuit. +//! Bounds, configs, bits, and data computation for the Decryption Share Aggregation TRBFV circuit. //! -//! Uses [`crate::threshold::decrypted_shares_aggregation::utils`] for Q/delta, modular inverses, -//! Lagrange-at-zero recovery, and scalar CRT reconstruction. Input coefficients are normalized -//! with [`e3_polynomial::reduce`] in [`Input::standard_form`], consistent with other circuits. +//! Uses [`crate::threshold::decrypted_shares_aggregation::utils`] for Q/delta, modular inverses, +//! Lagrange-at-zero recovery, and scalar CRT reconstruction. Coefficients are normalized +//! with [`e3_polynomial::reduce`] in [`Inputs::standard_form`], consistent with other circuits.
413-417: Local variable still namedinputafter the rename.The compute signatures now use
data, but this test variable is still calledinput. Consider renaming it for consistency with the refactor.- let input = + let data = DecryptedSharesAggregationCircuitData::generate_sample(preset, committee.clone()) .unwrap(); - let out = DecryptedSharesAggregationCircuit::compute(preset, &input).unwrap(); + let out = DecryptedSharesAggregationCircuit::compute(preset, &data).unwrap();
cedoor
force-pushed
the
refactor/input-to-data
branch
from
51f75f9 to
4ce699c
Compare
2 participants
Summary by CodeRabbit