Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Support error / warning suppression in zksolc #33
feat: Support error / warning suppression in zksolc #33
Changes from all commits
afe2718
92c9908
186c458
ba3cf92
c30a264
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Alternatively these fields could go on
ZkSolcSettings
vs here in order to remove duplicating the fields in bothZkSettings
andZkSolcInput
(since one struct contains the other this may not be ideal). Then when building the input we pass them directly, we have this flow forCliSettings
alreadyThere 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.
Could you explain how suppressed warnings / errors would be passed in this case? AFAIU,
ZkSolcInput
covers the entire standard JSON input forzksolc
andzksolc
requires these fields on the top level (not inside thesettings
object), so wouldn't it need to have these fields present in any case?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.
They need to be in
ZkSolcInput
but not inZkSettings
neccesarily.So there are a couple of structs involved in building the compilation input:
ZkSolcSettings
: which are the actual settings that are passed by foundry to theProject
. This for now contains two fieldsZkSettings
: which aims to be the literalsettings
object on JSON input.CliSettings
: which are cli arguments when compiling (e.g: base_path).ZkSolcVersionedInput
: Which is built when resolving solc versions. This has all the information needed to compile.ZkSolcInput
, the input actually passed as jsonSo lets say they are in
ZkSolcSettings
, when building theZkSolcVersionedInput
we would have:Hope that makes sense
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.
IIUC, I should not have put any new settings outside of the
settings
field.Would it help if I deprecated them where they currently are and moved them inside
settings
?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.
@hedgar2017 that does seem cleaner
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.
@elfedy please try 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.
@slowli @hedgar2017 tested the new release here: b04a920 (Replace
<PATH_TO_RELEASE>
with the local path to zksolc test release). Seems to be working correctly (Tests pass with it and fail with previous releases)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.
Maybe we can wait until those changes are part of a stable release then bump the version here and incorporate the changes in that test commit.
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.
@elfedy great!
We're going to make a release in the next few days.
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.
@elfedy released with zksolc v1.5.7!
Also supported by zksolc v1.5.6 and older, but one level above the
settings
object.