-
Notifications
You must be signed in to change notification settings - Fork 361
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
feat: [PLONK_AUDIT_4-11] fixes #735 #736
Conversation
---> |
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.
Looks good. Added a few comments.
@@ -422,7 +421,6 @@ contract PlonkVerifier { | |||
} | |||
|
|||
function compute_pi( | |||
bytes memory proof, |
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 don't really understand dropping proof
parameter from compute_pi()
, since load_wire_commitments_commit_api()
accepts the proof and uses it. I guess you're planning further changes?
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.
Ah nice catch, the last test that I generated was without the use of api.Commit() and the proof is not needed in that case I forgot about that (I followed the compiler that told me the parameter was unused). I'll add a condition in the template to not take the proof when commit is not used, but it's a mistake on my end
@@ -422,7 +421,6 @@ contract PlonkVerifier { | |||
} | |||
|
|||
function compute_pi( |
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.
You've lost some of the nice comments you wrote for the audit (e.g. for compute_pi()
but also in other places). Maybe you should copy these over if this is the master copy of the contract?
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 re added the comments
Added missing proof length check, fixes #735