feat: add assemblycreate for warning suppression for zksolc 1.5.10

[dutterbutter]

What 💻

• Adds assemblycreate error suppression

Why ✋

• Breaking change in 1.5.9 of zksolc

Evidence 📷

Include screenshots, screen recordings, or console output here demonstrating that your changes work as intended

Documentation 📚

Please ensure the following before submitting your PR:

• Check if these changes affect any documented features or workflows.
• Update the book if these changes affect any documented features or workflows.

[elfedy]

Looks good. Maybe we should wait until linking is merged to add this change and then formally bump zksolc to version 1.5.9 along with it though.

[Karrq]

I think it's ok as long as we don't change the default version here. But we should add this flag by default for >= 1.5.9 I believe since the affected contract is part of forge-std

[elfedy]

I think it's ok as long as we don't change the default version here. But we should add this flag by default for >= 1.5.9 I believe since the affected contract is part of forge-std

It's fine in terms of functionality. It's just tidier history wise to have an isolated PR bumping the compiler version and include the changes related to the version bump in that same PR.

[Karrq]

Hey @dutterbutter I need 1.5.9 for the linking due to the unlinked factory deps, I think it's best if we merge your PR first, bumping to 1.5.9 with it, and then my linking PR would be based on top of 1.5.9, wdyt?
/cc @elfedy

[dutterbutter]

@elfedy bumped this to 1.5.10 and made it default. One question I have, when I log out the compiler output from the tests I see the following difference between TxOrigin warning and AssemblyCreate warning:

AssemblyCreate

compiled: ProjectCompileOutput {
    compiler_output: AggregatedCompilerOutput {
        errors: [
            Error {
                component: "general",
                error_code: None,
                formatted_message: Some(
                    "Warning: `suppressedWarnings` at the root of standard JSON input is deprecated. Please move them to `settings`.\n",
                ),
                message: "`suppressedWarnings` at the root of standard JSON input is deprecated. Please move them to `settings`.",
                severity: Warning,
                source_location: None,
                type: "Warning",
            },
        ],

TxOrigin:

compiled: ProjectCompileOutput {
    compiler_output: AggregatedCompilerOutput {
        errors: [],

I would expect the error field to be similar? I dont think it has any effect on this PR as the test passes but wanted to flag nonetheless.

[dutterbutter]

Is it correct the test failures related to:

failed compiling zksync project: Missing libraries detected:

Would be resolved with #800 ?

[elfedy]

Is it correct the test failures related to:

failed compiling zksync project: Missing libraries detected:

Would be resolved with #800 ?

@dutterbutter I don't think those failures are expected. @Karrq can confirm though, maybe something changed on 1.5.10 about the missing libs/linking output?

[elfedy]

when I log out the compiler output from the tests I see the following difference between TxOrigin warning and AssemblyCreate warning

@dutterbutter will investigate this, it is strange as it should not change at all and should always have the warning. We are just deserializing the error json so not sure how this can happen by just changing the suppressedWarnings field value.

[Karrq]

Hmm I suspect it could be because in a recent compiler release (I think 1.5.8) the detect-missing-libraries flag has been changed a bit, and instead of being exclusive with the bytecode it's now included in the output.
I think the presence of the field (and/or) values triggers our check and we end compilation by showing this message to the user.
I don't particularly think we should get rid of it entirely (for backwards compatiblity), but we could reduce this to a "warning" for 1.5.8, and with #800 I can get rid of the warning for >= 1.5.9

[elfedy]

I don't particularly think we should get rid of it entirely (for backwards compatiblity), but we could reduce this to a "warning" for 1.5.8, and with #800 I can get rid of the warning for >= 1.5.9

Since we removed the detect-missing-libraries flag in #822, we can probably remove that missing-libraries error flow here instead of #800, if this is in fact the cause of the failing tests. Not sure it's worth keeping just for the 1.5.8 use case

[dutterbutter]

@elfedy @Karrq do I understand correctly that we want to remove this condition entirely in this PR?

[Karrq]

I'm in favor of retaining it wrt backwards compatibility & compiler versioning, but if that's not a concern then I'm in favor of removing it as well

[elfedy]

@dutterbutter yes we would remove all missing libs checks from handle_output, they were just there to print a pretty warning about missing libs with the old flow. I think right know that text would only be relevant for 1.5.8 and it's just not worth it to keep that warning and flow to support a warning for that version alone (And they can get missing libs in that version by using --json flag anyways)

[dutterbutter]

@elfedy updated to remove the logic for processing missing libraries in zksync_handle_output here. This has fixed the Missing libraries detected test failures previously mentioned.

Are tests, zksolc_can_compile_with_remapped_links and zksolc_can_compile_with_remapped_links here, still relevant with this change? They are currently failing from the mentioned change?

[elfedy]

Are tests, zksolc_can_compile_with_remapped_links and zksolc_can_compile_with_remapped_links here, still relevant with this change? They are currently failing from the mentioned change?

I think we should keep the tests. As far as I understand they should pass as the libraries are being specified in the input and bytecode should be linked, right @Karrq ?

[elfedy]

@dutterbutter @Karrq seems we will need to update our is_unlinked() method. we currently check for empty hash or missing_libraries having some entry. Testing some compilations with 1.5.10, it seems that when you pass libraries, the contract will be linked and compiled but still output its libraries on the missing_libraries field (@hedgar2017 is this correct? Is it expected?). So we will need to distinguish it in some other way. Maybe checking for empty hash is enough or we could make use of the objectFormat field ("raw" = linked, "elf" = unlinked) ?

[hedgar2017]

LGtM!

[dutterbutter]

@hedgar2017 any insight on @elfedy question:

Testing some compilations with 1.5.10, it seems that when you pass libraries, the contract will be linked and compiled but still output its libraries on the missing_libraries field (@hedgar2017 is this correct? Is it expected?

[hedgar2017]

@dutterbutter @Karrq seems we will need to update our is_unlinked() method. we currently check for empty hash or missing_libraries having some entry. Testing some compilations with 1.5.10, it seems that when you pass libraries, the contract will be linked and compiled but still output its libraries on the missing_libraries field (@hedgar2017 is this correct? Is it expected?). So we will need to distinguish it in some other way. Maybe checking for empty hash is enough or we could make use of the objectFormat field ("raw" = linked, "elf" = unlinked) ?

Must be a bug. I can fix it in v1.5.11.
For checking the bytecode status, using objectFormat as you said is the cleanest way. I added it exactly for this purpose.

[Karrq]

I actually had a different false positive where interfaces (that didn't have a hash) would be detected as unlinked - so I removed that and added if the number of unlinked factory deps (which afaik contains all factory deps) is higher than the list of linked factory deps, in addition to the missing libraries, but if we have another field to determine that then that's even better

[Karrq]

For checking the bytecode status, using objectFormat as you said is the cleanest way. I added it exactly for this purpose.

Sounds like we should switch is_unlinked to that then - I originally made it from my understanding of the documentation

[hedgar2017]

For checking the bytecode status, using objectFormat as you said is the cleanest way. I added it exactly for this purpose.

Sounds like we should switch is_unlinked to that then - I originally made it from my understanding of the documentation

Sure, but please provide a bug report for the issue above. Is it very important to fix anyway.

[Karrq]

Sure, but please provide a bug report for the issue above.

So @dutterbutter if I understand correctly you are observing a populated missing_libraries field in a fully linked contract?

[Karrq]

Other than the discussion above this looks good