-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add support for ZoKrates with curly brackets (wip) #211
base: master
Are you sure you want to change the base?
Conversation
We jsut copy pasted the existing frontend and updated the configurations and scripts to support both Zokrates frontends
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 is a very nice patch, Stefanos. I have a few questions about it, but I think they should be easy to address.
Also adds missing tests for tuples
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 looks good to me now.
If you're happy with it, then I'll merge it.
} | ||
} | ||
|
||
fn assembly_assign_impl_<const IS_CNST: bool>( |
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.
@alex-ozdemir, can you review this code?
} | ||
} | ||
|
||
fn assembly_constraint_<const IS_CNST: bool>( |
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.
Here I just add the assembly constraints like it was an assertion.
I think that if we make sure that assembly is handled correctly, then we can proceed. ref: https://zokrates.github.io/language/assembly.html |
Is the omission of |
} | ||
} | ||
|
||
pub fn field_to_bool_unsafe(f: T) -> Result<T, String> { |
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.
@alex-ozdemir, can you review this one?
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 think that you actually want this operator using it will maintain the connection between the input and the output of the operator in the final constraint system, it will just assume that the value is 0 or 1. See this part of R1CS lowering.
What do you think?
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.
Thanks, that makes sense! Should we still wrap it in a new_witness Op? If we do so, in the test, should we also pass the new_witness value?
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.
Thanks, that makes sense! Should we still wrap it in a new_witness Op?
No, you should not.
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.
Fixed it
Sorry for the delay. It was not intentional; I tried to implement it (without changing the IR), but the test failed. |
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.
(A change may not be necessary---see my comment and let me know what you think.)
} | ||
} | ||
|
||
pub fn field_to_bool_unsafe(f: T) -> Result<T, String> { |
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 think that you actually want this operator using it will maintain the connection between the input and the output of the operator in the final constraint system, it will just assume that the value is 0 or 1. See this part of R1CS lowering.
What do you think?
@alex-ozdemir fixed the issues, now the mpc tests fail. Seems like we miss libcrypto:
|
Hi @StefanosChaliasos , We've dodged the issue with ABY's testing infrastructure. (#221) |
This PR tries to address #208.
Some of the features that are not supported by the grammar are:
Furthermore, we don't currently support MPC.
Finally, the implementation of assembly has not been tested and shouldn't be complete.
All the tests pass. To run them, you can use the following commands:
The changes to support ZoKrates with curly brackets are the following:
third_party/ZoKratesCurly
.src/front/zsharpcurly
.zokc
.circ
executable to support zsharpcurly and implementedzcxi
.TODOs:
Note that you can check the changes vs the original implementation in the first 4 commits of this PR.