Skip to content
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

Updates ABI Encoder #201 #359

Closed
wants to merge 1 commit into from
Closed

Updates ABI Encoder #201 #359

wants to merge 1 commit into from

Conversation

petscheit
Copy link
Contributor

Updates the ABIEncoder to v2, which enables us to pass structs as parameters to functions. Simplifies the Verifier.sol a bit and is also needed for application tests (takes a lot of complexity out of the testing script because I can simply pass the proof object from the proof.json instead of parsing every value separately when calling verifyTx)

@Schaeff
Copy link
Member

Schaeff commented May 30, 2019

Would be super awesome to have this!
A few thoughts as I've tried this in the past.

  • Are you able to test a contract on Remix with this? I tried in the past an there was a missing piece, can't remember if it was on the Remix encoding side or if ABIV2 didn't support a certain type of recursive struct. Maybe the issue was with the verifying key, is this why you didn't change it? If not why?
  • Could you share a gas cost comparison?
  • ABIV2 is likely to stay experimental for a little while now, maybe it would make sense to have two versions as I agree it's great for testing and UX but people may still want the "stable" one

@petscheit
Copy link
Contributor Author

Regarding Remix:
-currently structs containing structs can't be passed with remix, as far as I understand. Wasn't able to make that work, so thats actually quite a downside.

Gas comparison:
image

I'm currently having some issue with the gm17 contract, for some reason the compiler wont link the imported libraries properly so can't test the gas usage for that one atm, still investigating. Overall it seems to have a positive effect on deployment and execution cost. I used the root code for the comparison, with the values given in the getting started.

Regarding having two versions of the contract, I agree. Should I implement that?

@Schaeff
Copy link
Member

Schaeff commented Jun 3, 2019

@petscheit I checked with @yann300 from Remix and passing structs works fine with the new interface which is being rolled out. So with this and ABIv2 being still experimental, I guess for this PR it would make sense to have a --abiv2 flag in zokrates export-verifier which is off by default (until things get stable) and which creates an ABIv2 contract for testing. @JacobEberhardt thoughts welcome!

@JacobEberhardt
Copy link
Member

I agree. This ABIv2 contract should be covered by the tests @petscheit is currently adding to the CI pipeline as well.

@petscheit
Copy link
Contributor Author

OK, sounds good to me too. I"ll get going on this now and implement it into the testing pipeline aswell. @Schaeff Should I change the proof.json format so it works for the different ABI's? I'm thinking the proof.json could have two sections, one for abiv1 and one for abiv2 and they contain the data needed for the proof in copy/paste friendly format. Would also make the circleci integration easier

@JacobEberhardt
Copy link
Member

If i may chime in here: The goal is to have the proof.json as an independent minimal format and then offer a cli-option to convert into copy-past friendly representations for various target platforms/IDEs.

@Schaeff
Copy link
Member

Schaeff commented Jul 5, 2019

Hey @petscheit , closing this as we merged #414 please shout if I got this wrong!

@Schaeff Schaeff closed this Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants