-
Notifications
You must be signed in to change notification settings - Fork 36
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
NOVA Scotia folding scheme test done #227 #240
base: main
Are you sure you want to change the base?
Conversation
csiejimmyliu
commented
Sep 21, 2024
- Nova Scotia folding test in morpo ffi
- Using toy r1cs example of Fibonacci sequence
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.
It looks good! Thank you for your contribution
there are several points that can be changed
- rename the circuit (
toy
-> e.g.adder
orfibonacci
) - remove temporary files
jna-5.13.0.jar
,toy_js/
( we just need ther1cs
andwasm
- try to run test with
cargo test --features nova_scotia
and include the CI script like
mopro/.github/workflows/build-and-test.yml
Lines 35 to 36 in b625093
- name: Run ffi halo2 tests run: cd mopro-ffi && cargo test --features halo2 --no-default-features
and please use |
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.
Thank you for fixing the errors
Can you fix the feature's name to nova-scotia
Thank we can merge this PR 🙏🏻
@@ -46,6 +48,18 @@ jobs: | |||
override: true | |||
- name: Run ffi circom tests | |||
run: cd mopro-ffi && cargo test --features circom --no-default-features | |||
test-ffi-nova_scotia: |
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.
can we align the usage of -
and _
I prefer the feature flag be nova-scotia
e.g. --features nova-scotia
(but in rust code some place needs to be nova_scotia
then just keep them nova_scotia
)
nova_scotia = [ | ||
"nova-scotia", |
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.
this can be changed to
nova-scotia = [
"dep:nova-scotia",
to activate the dependency with the same name
# Nova Scotia dependencies | ||
nova-scotia = { version = "0.5.0" } | ||
nova-snark = { version="0.23.0" } | ||
serde_json = { version="1.0.85" } | ||
|
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.
we should remove this (for now)
since we don't activate the nova-scotia
feature
(and it needs to work without these dependencies)
but once you update the type for udl
(serialization)
it should be solved easily
it can be done in another PR