-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: main
Are you sure you want to change the base?
Conversation
...and rename to `test_contracts`
The regression in the |
There was a problem hiding this 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.
There was a problem hiding this 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" } |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this 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?
Detected VM performance changes
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. |
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
zk_supervisor fmt
andzk_supervisor lint
.