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

test: Brush up test contracts #3035

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

slowli
Copy link
Contributor

@slowli slowli commented Oct 8, 2024

What ❔

Extracts test contracts to a separate crate with a reasonable build pipeline.

Why ❔

For now, test contracts are distributed across multiple crates (e.g., loaded using hardcoded paths in the workspace). This is not maintainable and makes the codebase harder to publish and use.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk_supervisor fmt and zk_supervisor lint.

@slowli
Copy link
Contributor Author

slowli commented Oct 8, 2024

The regression in the iai benchmark is entirely explained by the increased time to read system contracts (more specifically, most additional instructions are in serde_json::read::next_or_eof function, so it looks like parsing Solidity contracts in zksync_contracts). Not entirely sure which changes in zksync_contracts slowed it down; logically, the only change is removal of the load test contract.

Copy link
Contributor Author

@slowli slowli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR isn't 100% brushed up, but I think it should give a good general understanding of the approach. Essentially, building test contracts is now moved to a build script (which still relies on yarn; I haven't explored options like foundry-compilers yet), so that the building process is fully encapsulated inside the corresponding crate. IMO, this approach has significantly better in terms of DevEx than the previously used one, but there may be significant downsides I'm missing.

Copy link
Contributor Author

@slowli slowli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly ready for review (at least the approach is ready), with the exception of a couple of tests disabled because it's currently impossible to suppress zksolc errors in foundry-compilers (see FIXMEs).

Cargo.toml Outdated
@@ -201,6 +201,8 @@ trybuild = "1.0"
vise = "0.2.0"
vise-exporter = "0.2.0"

foundry-compilers = { version = "0.11.1", git = "https://github.com/Moonsong-Labs/compilers.git", rev = "b2aca7f87e0484d1e202d77a4ada8b46b059da6d" }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to rely on a git repo here? (Honestly, can't think of other options as long as the overall approach is retained.)

@@ -34,6 +32,7 @@ allow = [
"OpenSSL",
"Apache-2.0 WITH LLVM-exception",
"0BSD",
"BSL-1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this license addition OK?

@slowli slowli marked this pull request as ready for review October 11, 2024 13:48
@slowli slowli requested a review from a team as a code owner October 11, 2024 13:48
Copy link
Contributor

@perekopskiy perekopskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks nice! Is zksolc error suppressing in work?

Copy link
Contributor

Detected VM performance changes

Benchmark name change in estimated runtime change in number of opcodes executed
deploy_simple_contract_legacy -4.8% +0 (+0.0%)
deploy_simple_contract -5.0% +0 (+0.0%)

Changes in number of opcodes executed indicate that the gas price of the benchmark has changed, which causes it run out of gas at a different time. Or that it is behaving completely differently.

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