chore: make scripts more consistent#1033
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUpdated CI and npm scripts to replace legacy ciphernode/circuit targets with language-specific targets (Rust/Noir/EVM); added Testing documentation to README; adjusted package prebuild to use pnpm -C invocations. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant PNPM as pnpm
participant EVM as packages/enclave-contracts
participant RUST as crates/... (Rust)
participant NOIR as noir-projects
rect `#f3f4f6`
Dev->>PNPM: trigger CI job or run `pnpm test` / `pnpm rust:lint`
end
alt Top-level test flow
PNPM->>PNPM: resolve `test` -> run per-target scripts
PNPM->>EVM: `pnpm -C packages/enclave-contracts test`
PNPM->>RUST: `pnpm rust:test` / `pnpm -C crates/... build`
PNPM->>NOIR: `pnpm noir:test`
else CI lint update
PNPM->>RUST: `pnpm rust:lint` -- replaces `pnpm ciphernode:lint`
end
Note over PNPM,Dev: Results reported back to CI / Developer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
49-49: Remove extraneous blank line.Line 49 introduces an unnecessary blank line after the solc installation step. This appears to be unintended formatting.
- name: Install solc run: | sudo add-apt-repository ppa:ethereum/ethereum \ && sudo apt-get update -y \ && sudo apt-get install -y solc - - name: Run CRISP Unit Tests
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(3 hunks)README.md(1 hunks)package.json(1 hunks)packages/enclave-sdk/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
README.md
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
.github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_e3_support_dev
- GitHub Check: test_contracts
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_risc0
- GitHub Check: build_sdk
- GitHub Check: rust_unit
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_integration
🔇 Additional comments (8)
README.md (1)
46-91: Documentation is comprehensive and well-structured.The new Testing section clearly documents the available test commands and provides helpful examples. The warning about using pnpm scripts is appropriate and the --no-prebuild flag for integration tests adds practical guidance.
Verify that all documented scripts (lines 54–87) are correctly defined in the root
package.jsonand that the test:integration script actually supports the--no-prebuildflag.package.json (4)
55-57: Verify build and wasm:build script order and existence.Line 55 defines
build: "pnpm compile"and line 57 addswasm:build: "pnpm -C ./crates/wasm build". The mainbuildscript referencescompile(line 14), which runspnpm build:ts && pnpm rust:build. However,wasm:buildis not directly invoked by the rootbuildscript. Verify whether:
- The WASM build is intentionally separate from the root build process
- Or if
wasm:buildshould be integrated into thebuild:tsor main build chainClarify the relationship between
build,compile,build:ts,rust:build, andwasm:build.
25-29: Fix brokenevm:installscript delegation in package.json.The verification found that
evm:install(line 37) targetspackages/enclave-contractsand attempts to delegate to aninstallscript, but that script does not exist in the target package's package.json. All other pnpm delegations in lines 25–56 are valid.Correct the script target for
evm:installor add the missinginstallscript topackages/enclave-contracts/package.json.⛔ Skipped due to learnings
Learnt from: ryardley Repo: gnosisguild/enclave PR: 145 File: packages/ciphernode/data/Cargo.toml:0-0 Timestamp: 2024-10-22T02:36:01.448Z Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.Learnt from: ryardley Repo: gnosisguild/enclave PR: 156 File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274 Timestamp: 2024-10-23T01:59:42.967Z Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
14-20: Script delegation and paths are correctly configured.All verification checks passed:
tests/integration/test.shexistsscripts/test-circuits.shandscripts/lint-circuits.shexist- All
pnpm -Cdelegation targets (packages/enclave-contracts,packages/enclave-sdk,packages/enclave-react,crates/wasm,packages/enclave-config) have valid package.json files with corresponding scripts- The
prerust:buildlifecycle hook is properly configured to runpnpm evm:buildbeforerust:build- No old ciphernode script references remain in package.json
- Root lint command (
pnpm evm:lint && pnpm rust:lint && pnpm noir:lint) references only existing, properly delegated targetsThe refactoring successfully modularizes the build system without breaking any script dependencies.
30-31: No build dependency issues found—review comment is incorrect.The prerust:build lifecycle hook is working as intended. Verification confirms:
prerust:buildcorrectly executespnpm evm:buildbeforerust:buildevm:buildhas no Rust artifact dependencies (Solidity + TypeScript compilation only)rust:buildhas no dependency on EVM outputs (Cargo.toml contains only Rust crates)- No circular dependencies exist—the build systems are independent and properly orchestrated
Likely an incorrect or invalid review comment.
.github/workflows/ci.yml (3)
91-92: Lint command naming and implementation are now consistent with language-specific scripts.The step label and command have been updated from ciphernode to rust (lines 91–92), aligning with the new
pnpm rust:lintscript defined in the root package.json (line 24). This change correctly reflects the shift to language-specific tooling.
134-135: Lint command naming and implementation are now consistent with language-specific scripts.The step label and command have been updated from ciphernode to rust (lines 134–135), consistent with the rust_unit job (lines 91–92) and the new
pnpm rust:lintscript. This ensures uniformity across Rust-related CI jobs.
1-750: No remainingciphernode:lintreferences found—PR changes verified.Verification confirms that:
- No
ciphernode:lintcommand references exist in the CI workflow- Lint commands in both
rust_unitandrust_integrationjobs are properly usingpnpm rust:lint- The only
ciphernodemention is the job nameciphernode_integration_test:on line 305, which is unrelated to lint commandsThe PR has successfully removed all legacy lint command references.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
49-94: Documentation clearly addresses PR objectives and provides helpful guidance.The new Testing section properly emphasizes the requirement to use pnpm scripts over direct tool invocation, and comprehensively documents the available test scripts. The note on line 51 about required setup steps aligns well with the PR's stated objective to "require all crate operations to run via pnpm."
However, a few minor clarifications would improve clarity:
- Line 64: Could specify what types of tests
rust:testruns (e.g., "unit, integration, and doc tests")- Line 51: Could briefly mention examples of setup (e.g., "e.g., compiling smart contracts, building binaries")
Consider expanding the setup explanation in the warning to make it more concrete and actionable for contributors unfamiliar with the build process.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(2 hunks)README.md(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-22T02:36:01.448Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/data/Cargo.toml:0-0
Timestamp: 2024-10-22T02:36:01.448Z
Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
Applied to files:
.github/workflows/ci.ymlREADME.md
📚 Learning: 2024-10-23T01:59:42.967Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 156
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:274-274
Timestamp: 2024-10-23T01:59:42.967Z
Learning: In the `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs` file and other test files within this project, hardcoding `CIPHERNODE_SECRET` is acceptable for testing purposes.
Applied to files:
README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: test_contracts
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_unit
🔇 Additional comments (5)
.github/workflows/ci.yml (1)
91-92: Confirm rust:lint script is correctly defined in package.json.The CI workflow has been updated to call
pnpm rust:lintin bothrust_unitandrust_integrationjobs. Ensure this script is properly defined and tested before merging, as CI failures from missing scripts could be costly.The dependency on the package.json script definition should be verified via the earlier review comment's verification script.
Also applies to: 134-135
README.md (1)
90-93: No issues found—--no-prebuildflag is properly supported and documented.Verification confirms that
tests/integration/test.shaccepts the--no-prebuildflag (line 7), and the README documentation on lines 92-93 accurately reflects the supported implementation. The flag is parsed by the script and documentation is consistent.package.json (3)
14-15: Verify ecosystem coverage in lint and compile commands.Analysis of the orchestrator scripts reveals:
compileordering is correct: Thebuild:tsscript (pnpm evm:build && pnpm sdk:build && pnpm react:build) runs beforerust:buildusing sequential execution (&&), ensuring proper dependency ordering. ✓
lintcommand has gaps: The lint command omitssdk:lint—whilesdkhassdk:buildandsdk:testdefined, there is no correspondingsdk:lintscript. Additionally,wasmecosystem (which haswasm:buildandwasm:release) has nowasm:lintorwasm:testdefined or invoked.
testcommand: All referenced test scripts (evm:test,rust:test,sdk:test,noir:test) are properly defined. Cannot verify from current state whethernoir:testwas previously omitted.Action needed: Confirm whether
sdk:lintshould be added to the lint orchestrator, and whetherwasmecosystem should have lint/test scripts and corresponding orchestrator entries, or if those ecosystems intentionally lack testing/linting.
58-58: No breaking changes—aligns with existing patterns.The path change from
"./crates/wasm"to"crates/wasm"is safe and establishes consistency with existing patterns already in the codebase. CI workflows (.github/workflows/ci.yml) and documentation (scripts/README.md) both reference the workspace using thecrates/wasmformat without./prefix, confirming this change improves uniformity. Thepnpm -Cpattern operates identically with or without the./prefix, and no conflicts arise.
26-26: Style inconsistency in rust scripts:rust:testusescd crates &&pattern whilerust:buildandrust:lintinvoke cargo directly.All referenced script paths exist and the scripts function correctly. However, there's a minor pattern inconsistency:
rust:testusescd crates && ./scripts/test.shwhilerust:buildandrust:lintruncargocommands directly from the root directory. Sinceevm:*andsdk:*scripts follow thecd <path> && pnpm <task>pattern consistently, consider aligningrust:buildandrust:lintwithcd crates &&for uniformity—e.g.,cd crates && cargo build --locked --releaseandcd crates && cargo fmt -- --check.Note: The scripts function identically in either form since the root
Cargo.tomldefines the workspace scope; this is a style consistency suggestion, not a functional issue.⛔ Skipped due to learnings
Learnt from: ryardley Repo: gnosisguild/enclave PR: 184 File: packages/ciphernode/net/tests/entrypoint.sh:4-8 Timestamp: 2024-11-25T09:47:48.863Z Learning: When reviewing test scripts like `packages/ciphernode/net/tests/entrypoint.sh`, avoid suggesting additional error handling and cleanup for `iptables` commands, as it may not be necessary.Learnt from: ryardley Repo: gnosisguild/enclave PR: 145 File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83 Timestamp: 2024-10-22T02:10:34.864Z Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.Learnt from: ryardley Repo: gnosisguild/enclave PR: 145 File: packages/ciphernode/evm/src/enclave_sol_reader.rs:87-89 Timestamp: 2024-10-22T03:39:29.448Z Learning: In the `ciphernode` project, specifically in `packages/ciphernode/evm/src/enclave_sol_reader.rs`, the method `EnclaveSolReader::attach` should be retained even if it directly calls `EvmEventReader::attach` without additional processing. Avoid suggesting its removal in future reviews.Learnt from: ryardley Repo: gnosisguild/enclave PR: 145 File: packages/ciphernode/data/Cargo.toml:0-0 Timestamp: 2024-10-22T02:36:01.448Z Learning: In `packages/ciphernode/data/Cargo.toml`, `actix-rt` is only used in tests and should remain in `[dev-dependencies]`.
2 participants
Closes #809
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.