-
Notifications
You must be signed in to change notification settings - Fork 6
Circuits naming convention #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
|
|
You're right that we would not have Also semantically I would claim that the gadget is the code in the |
Right, agree then, happy with it ^^ |
Been thinking a bit more on this, and I find it more natural to use
So with this, I'm more inclined towards having the
|
My concern about having a type called I've checked other repositories built by other developers and they also use
Those repositories don't have a For this reason (consistency with the conventions that plonky2 developers and others are using), I'd prefer to keep the type that holds Here's another proposal: instead of having a type that has the build method, we could have just functions. Like this: fn merkle_proof_build(builder: &mut CircuitBuilder<F, D>, max_depth: usize) -> Result<MerkleClaimAndProofTarget>
fn merkle_proof_existence_build(builder: &mut CircuitBuilder<F, D>, max_depth: usize) -> Result<MerkleProofExistenceTarget>
fn signature_verify_build(builder: &mut CircuitBuilder<F, D>) -> Result<SignatureVerifyTarget>
fn signed_pod_verify_build(builder: &mut CircuitBuilder<F, D>, params: &Params) -> Result<SignedPodVerifyTarget> |
True, agree. Although on the same time, the codebase of plonky2 is not much active and there is not much community around it using those terms, so we would not be diverging from an active community. Still I agree that better if we can keep using those conventions. But if we follow the plonky2 convention, then we would not have the So with this, I think that we might not follow 100% the plonky2 convention. Once we assume we're not completely consistent with plonky2 convention, the matter is how far or close we are from that 😺 From all the options we've explored, I think that the proposal of #181 (comment) together with what you say in #181 (comment) (2d point) on replacing struct MerkleProofTargets {
max_depth: usize, /* or Params if needed more */
root: HashOutTarget,
//... (rest of Targets)
}
impl MerkleProofTargets {
fn build(builder: &mut CircuitBuilder<F, D>, max_depth: usize /* or Params if needed more */)
-> Result<Self> {
// logic of the circuit
return Self{...} // returns `MerkleProofTargets`
}
fn set_targets(&self, pw: &mut PartialWitness<F>, inputs: ...) -> Result<()> {
// assign inputs to the targets
}
} Probably we will still refer (informally in meetings and also in the code inline comments) to the |
*I used |
Currently we have (using the merkletree gadget as example):
Then to use it we have to do:
I find the
eval
naming confusing, since it makes it look like the method is "evaluating", when actually it is setting the targets and the gadget logic (ie. building the gadget/circuit logic).Since the other method is
set_targets
, maybe an option would be usingadd_targets
instead ofeval
, but not convinced; maybe something else likebuild_circuit
(although in some cases will not be a 'circuit' but a 'gadget'). Then maybe something likebuild_logic
ortargets_logic
or similar. Or, since we're already in theSomethingGadget
context, can be directlySomethingGadget::build()
.Also we can group the two structs to simplify a bit the interfaces, something like:
Then we would use it as
Opening this issue to have a separate thread to discuss it, hoping to include it
in the refactor (#171).
The text was updated successfully, but these errors were encountered: