ci(l1): add ZisK and OpenVM block executions to the CI #52
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the CI infrastructure to support ZisK and OpenVM zkVM backends for L1 block execution, in addition to the existing SP1 and Risc0 backends.
Key changes:
- Added new execution targets (
execute-zisk-ciandexecute-openvm-ci) and check targets for both ZisK and OpenVM in the Makefile - Extended the GitHub Actions test matrix to include
ziskandopenvmbackends - Reorganized Makefile sections for better clarity (Execution, Proving, Checks)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Makefile | Added execution targets for ZisK and OpenVM backends, added comprehensive check targets for all feature combinations, reorganized into logical sections, fixed update-ethrex target naming and syntax |
| .github/workflows/pr-main.yaml | Extended test matrix to include ZisK and OpenVM backends, consolidated build steps, added install steps for new backends |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Install OpenVM | ||
| if: matrix.backend == 'openvm' | ||
| uses: ./.github/actions/install-openvm |
There was a problem hiding this comment.
The install-openvm action does not exist in .github/actions/. The workflow will fail when trying to execute with the openvm backend because this action cannot be found. Either create the missing action directory and files, or remove openvm from the matrix if it's not ready yet.
.github/workflows/pr-main.yaml
Outdated
| matrix: | ||
| features: ["", "l2", "l2,sp1", "l2,risc0", "l2,zisk", "sp1", "risc0", "zisk"] | ||
| features: | ||
| ["", "l2", "l2,sp1", "l2,risc0", "l2,zisk", "sp1", "risc0", "zisk"] |
There was a problem hiding this comment.
The lint job's feature matrix is missing "openvm" and "l2,openvm" combinations. These feature combinations are tested in the Makefile's check target (lines 47 and 53) and in the test job (line 101), but they're not being linted here. Consider adding "openvm" and "l2,openvm" to maintain consistency across CI jobs.
| ["", "l2", "l2,sp1", "l2,risc0", "l2,zisk", "sp1", "risc0", "zisk"] | |
| ["", "l2", "l2,sp1", "l2,risc0", "l2,zisk", "sp1", "risc0", "zisk", "openvm", "l2,openvm"] |
| run: | | ||
| cargo b -r | ||
| if [ "${{ matrix.backend }}" = "exec" ]; then | ||
| cargo b -r |
There was a problem hiding this comment.
The consolidated build step doesn't handle the risc0 backend correctly. According to the Makefile (line 22), risc0 requires --no-default-features flag in addition to --features risc0. The current implementation will build risc0 with default features enabled, which is inconsistent with the expected behavior. Consider adding special handling for risc0:
if [ "${{ matrix.backend }}" = "exec" ]; then
cargo b -r
elif [ "${{ matrix.backend }}" = "risc0" ]; then
cargo b -r --no-default-features --features "${{ matrix.backend }}"
else
cargo b -r --features "${{ matrix.backend }}"
fi| cargo b -r | |
| cargo b -r | |
| elif [ "${{ matrix.backend }}" = "risc0" ]; then | |
| cargo b -r --no-default-features --features "${{ matrix.backend }}" |
No description provided.