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

Draft
wants to merge 28 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

Detected VM performance changes

Benchmark name change in estimated runtime change in number of opcodes executed
deploy_simple_contract +8.7% +0 (NaN%)
deploy_simple_contract_legacy +7.4% N/A

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.

1 participant