-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fit Nova+CycleFold into FoldingScheme
trait & Add examples/
for folding SHA256 circuit
#64
Conversation
- update lib.rs's `FoldingScheme` trait interface - fit Nova+CycleFold into the `FoldingScheme` trait - refactor `src/nova/*`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left some comments. But not strongly opinionated. So you can pass on them.
I'd like to rework the traits of the lib after #57 such that we can simplify it. The example made me realize we have a bit of too much complexity there. And might be complex to use.
} | ||
} | ||
|
||
/// cargo test --example simple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about having tests inside examples.
Examples should contain examples, so basically a way to show the user how to work with some par of the lib. But if we want tests, we should move this to an integration tests folder IMO.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that it is somehow weird, but here it is not testing any logic implemented by us, but just ensuring that the usage of Sha256 in-circuit and out-circuit matches for the example Sha256FCircuit
impl, which is only created for this example.
But I don't see were we could move it, an option is to directly remove it, but I find it useful to ensure that the step_native
(in-circuit) & generate_step_constraints
(outside-circuit) are compatible, which is what the test checks.
Not sure on if to remove it or not, how do you see it?
6213364
to
0b2675a
Compare
0b2675a
to
493d94d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! The trait changes also LGTM!
Changes:
FoldingScheme
trait interfaceFoldingScheme
traitsrc/nova/*
IVC
->Nova
(also movingivc.rs
intomod.rs
),DeciderCircuit
->DeciderEthCircuit
, and some other methodsexamples/
dir with aFoldingScheme
example folding the SHA256 circuitOne note on this PR nature:
This kind of PR is a bit 'opinionated' regarding the design of the interfaces. How I'm trying to approach it is to try to avoid premature abstractions, trying to fit the current needs in the interfaces and also the future ones that are clear and obvious, but for the ones that are not fully clear I prefer to wait to do those abstractions once we have other schemes integrated and we can see concretely what are the needs.
For example, the current interface will probably require changes for when we add ProtoGalaxy, so I'm trying to avoid trying to guess the abstractions now, and later when we integrate the actual ProtoGalaxy code do another refactor of the (premature) abstractions to make it really fit with the final needs of the code.
This does not mean that the current interface is only for Nova, in fact it is very close to the one that we defined at the beginning with @han0110 , where we defined an agnostic interface for the diverse folding schemes that are on the roadmap. But being aware that there might be unknown needs that we will find once we have the actual code for each scheme, just that we're not trying to over-guess them in advance.