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

Merged
merged 54 commits into from
Nov 15, 2024
Merged

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.

perekopskiy
perekopskiy previously approved these changes Nov 12, 2024
contracts Outdated Show resolved Hide resolved
@slowli slowli added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@popzxc popzxc added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit c02098b Nov 15, 2024
42 checks passed
@popzxc popzxc deleted the aov-pla-1041-brush-up-test-contracts branch November 15, 2024 18:12
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