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

feat: Support error / warning suppression in zksolc #33

Merged

Conversation

slowli
Copy link

@slowli slowli commented Oct 18, 2024

  • Allows to suppress zksolc errors and/or warnings by adding relevant fields to ZkSettings and passing them to standard JSON input for the compiler.

@slowli slowli marked this pull request as ready for review October 18, 2024 06:24
@slowli slowli requested a review from hedgar2017 as a code owner October 18, 2024 06:24
@@ -66,6 +85,12 @@ pub struct ZkSettings {
/// Whether to compile via EVM assembly.
#[serde(default, rename = "forceEVMLA")]
pub force_evmla: bool,
/// Suppressed `zksolc` warnings.
#[serde(default, skip_serializing_if = "HashSet::is_empty")]
pub suppressed_warnings: HashSet<ZkSolcWarning>,
Copy link

@elfedy elfedy Oct 18, 2024

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 both ZkSettings and ZkSolcInput (since one struct contains the other this may not be ideal). Then when building the input we pass them directly, we have this flow for CliSettings already

Copy link
Author

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 for zksolc and zksolc requires these fields on the top level (not inside the settings object), so wouldn't it need to have these fields present in any case?

Copy link

@elfedy elfedy Oct 18, 2024

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 in ZkSettings 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 the Project. This for now contains two fields
    • ZkSettings: which aims to be the literal settings 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 json

So lets say they are in ZkSolcSettings , when building the ZkSolcVersionedInput we would have:

impl CompilerInput for ZkSolcVersionedInput {
     type Settings = ZkSolcSettings;
     ...
    fn build(
        sources: Sources,
        settings: Self::Settings,
        language: Self::Language,
        version: Version,
    ) -> Self {

        let ZkSolcSettings { settings, cli_settings, suppressed_warnings, suppressed_errors } = settings;
        let input = ZkSolcInput { language, sources, settings, suppressed_warnings, suppressed_errors }.sanitized(&version);

        Self { solc_version: version, input, cli_settings }
    }
...
}

Hope that makes sense

Copy link
Collaborator

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?

Copy link

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

Copy link
Collaborator

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!

Copy link

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)

Copy link

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Karrq
Karrq previously requested changes Nov 1, 2024
@Karrq Karrq dismissed their stale review November 1, 2024 16:53

Change meant for future version

@elfedy elfedy merged commit 8f1613e into Moonsong-Labs:zksync-v0.11.4 Nov 4, 2024
4 checks passed
Karrq pushed a commit that referenced this pull request Nov 26, 2024
feat: add FromStr to ZkSolcWarnings and ZkSolcErrors (#37)
Karrq added a commit that referenced this pull request Nov 26, 2024
* feat: add zksolc support

* fix: codeowner change (#32,#34)

* feat: zksolc v1.5.7 support (#35)

* feat: zksolc 1.5.7 support

* chore: update version constants

* feat: Support error / warning suppression in zksolc (#33)

feat: add FromStr to ZkSolcWarnings and ZkSolcErrors (#37)

* fix: add back skip filter over avoid-contracts

* feat(zk): properly mark unlinked contract (#40,#41,#43,#44)

feat(zk): mark bytecode as unlinked
refactor(zk): get output from `eravm` object
feat: retain pre-1.5.7 zksolc compatibility
test(artifact:zk): unprefixed bytecode test

* chore: bump version

* chore: merge conflicts 11.6->12.3

feat(zk): per-source profile & settings
feat(zk): stub our zksolc settings restrictions

* fix(ci): update rust toolchain version

* fix(ci): proper rust toolchain name

* chore: clippy

---------

Co-authored-by: Federico Rodríguez <[email protected]>
Co-authored-by: Tomi Moreno <[email protected]>
Co-authored-by: Alex Ostrovski <[email protected]>
Co-authored-by: Dustin Brickwood <[email protected]>
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.

5 participants