refactor: remove packages instead use compact-tools as submodules#295
refactor: remove packages instead use compact-tools as submodules#2950xisk wants to merge 11 commits into
compact-tools as submodules#295Conversation
WalkthroughThe PR integrates Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Good initiative on this @0xisk! I talked offline with @emnul about this and I don't think it's a good idea to have a private repo as a submodule because outside contributors won't be able to clone it. I think it'd be better to duplicate the packages in both repos (which is obnoxious, I know) and instead set up compact-tools to be public and release it properly as a package. IMO that's the ideal scenario. Thoughts?
andrew-fleming
changed the title
compact-toolscompact-tools
Thanks @andrew-fleming Yes that was the intention to wait on this draft PR on either |
0xisk
force-pushed
the
fix/compact-tools
branch
from
bdacd6c to
bd654d7
Compare
![boostsecurity-io[bot]](https://avatars.githubusercontent.com/in/192984?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 3 New Security Findings
The latest commit contains 3 new security findings.
| Findings | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Dependency: npm / babel-traverse@ 6.26.0 SUMMARY Dependency: babel-traverse Transitive through:
Location : yarn.lock OCCURRENCE
|
||||||||||||||||||
Remediation :
|
||||||||||||||||||
| Dependency: npm / cross-spawn@ 5.1.0 SUMMARY Dependency: cross-spawn Transitive through:
Location : yarn.lock OCCURRENCE
|
||||||||||||||||||
Remediation :
|
||||||||||||||||||
| Dependency: npm / json5@ 0.5.1 SUMMARY Dependency: json5 Transitive through:
Location : yarn.lock OCCURRENCE
|
||||||||||||||||||
Remediation :
|
Not a finding? Ignore it by adding a comment on the line with just the word noboost.
Scanner: boostsecurity - BoostSecurity SCA
![semgrep-code-openzeppelin[bot]](https://avatars.githubusercontent.com/u/20820676?s=60&v=4){kind=small}
| "ansi-styles@npm:^4.0.0": | ||
| version: 4.3.0 | ||
| resolution: "ansi-styles@npm:4.3.0" | ||
| "babel-traverse@npm:^6.24.1, babel-traverse@npm:^6.26.0": |
There was a problem hiding this comment.
Risk: Affected versions of @babel/traverse and babel-traverse are vulnerable to Incomplete List of Disallowed Inputs / Incorrect Comparison. Compiling untrusted code with Babel using plugins that invoke the internal path.evaluate() or path.evaluateTruthy() methods (for example @babel/plugin-transform-runtime, @babel/preset-env with useBuiltIns, or any polyfill‐provider plugin) allows a maliciously crafted AST to execute arbitrary code on the build machine during compilation.
Manual Review Advice: A vulnerability from this advisory is reachable if you use Babel to compile untrusted JavaScript
Fix: There are no safe versions of this library available for upgrade. Library included at compact-contracts/yarn.lock:1622.
Reference(s): GHSA-67hx-6x53-jw92, CVE-2023-45133
⭐ Fixed in commit e996465 ⭐
…use config var in package.json
0xisk
changed the title
compact-toolscompact-tools
0xisk
changed the title
compact-toolscompact-tools as submodules
| "fmt-and-lint": "cd contracts && biome check . --changed", | ||
| "fmt-and-lint:fix": "cd contracts && biome check . --changed --write", | ||
| "fmt-and-lint:ci": "cd contracts && biome ci . --changed --no-errors-on-unmatched", |
There was a problem hiding this comment.
I had to add this because I couldn't find any workaround to ignore the biome.json config inside the root of the compact-tools submodule.
yarn fmt-and-lint 1 ✘ 22.18.0
~/compact-contracts/compact-tools/biome.json configuration ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
✖ Found a nested root configuration, but there's already a root configuration.
ℹ The other configuration was found in ~/compact-contracts.
ℹ Use the migration command from the root of the project to update the configuration.
$ biome migrate --write
There was a problem hiding this comment.
biome does not play well with submodules 🥲
|
This PR is ready but Just need to fix the CI for testing; https://github.com/OpenZeppelin/compact-contracts/actions/runs/21521884236/job/62015059025?pr=295#step:7:1 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
70-70:⚠️ Potential issue | 🟠 MajorRemove or correct the invalid import path on line 70.
The documented import path
compact-contracts/node_modules/@openzeppelin/compact-contracts/...does not exist in the repository structure. The@openzeppelin/compact-contractspackage is neither listed as a dependency in package.json nor present as a submodule. The note on line 73 confirms the package is not yet available as an NPM package. Either update the documentation to show the correct import mechanism once the package is available, or remove this example until the setup is functional.
🤖 Fix all issues with AI agents
In `@biome.json`:
- Around line 10-12: Move the experimentalScannerIgnores entry from the root to
the files section by creating files.experimentalScannerIgnores and removing the
root-level experimentalScannerIgnores; replace the glob "compact-tools/**" with
a literal basename "compact-tools" because experimentalScannerIgnores only
accepts basenames/path segments, or alternatively remove it and add a
files.includes entry using the !! force-ignore syntax (e.g., files.includes with
an !! pattern) if you need glob-like behavior. Ensure you update the key name
exactly as files.experimentalScannerIgnores and/or use files.includes for
force-ignore patterns.
In `@CHANGELOG.md`:
- Around line 14-29: Fix the malformed package scopes in CHANGELOG.md by
replacing occurrences of the incorrect token "@openzeppelin/compact-/" with the
correct package scopes (e.g., "@openzeppelin/compact" or the intended scoped
package names) so entries like "@openzeppelin/compact-/contracts",
"@openzeppelin/compact-/compact", and
"@openzeppelin/compact-/contracts-simulator" are corrected to their proper
scoped package identifiers; ensure all similar entries (including the three
instances shown and any other "@openzeppelin/compact-/" matches) are updated
consistently and run a quick grep/search to verify no remaining malformed scopes
exist.
In `@compact-tools`:
- Line 1: The repository includes a git submodule named compact-tools that
points to a private repository, which prevents external contributors from
cloning; to fix this, either make the compact-tools repository public and
publish it as an npm package (then replace the submodule dependency with a
proper package.json dependency) or remove the submodule from the repo and add a
documented placeholder/installation step until compact-tools is public; update
any references to compact-tools in the codebase (imports, build scripts, and
README) so they point to the npm package name or the new public repo location
and keep this PR as draft until the submodule is made public or replaced.
In `@contracts/package.json`:
- Around line 40-41: The package.json currently depends on local submodule
packages "@openzeppelin/compact-tools-cli" and
"@openzeppelin/compact-tools-simulator" which fail if the compact-tools
submodule isn't initialized; add a preinstall script in contracts' package.json
that checks whether the submodule is initialized (e.g., verify existence of
../compact-tools/packages or run a lightweight git submodule status check) and
either runs git submodule update --init --recursive or fails with a clear
message instructing the developer to run that command; update the "scripts"
section to include this "preinstall" hook so npm/yarn will validate/init the
submodule before resolving the file:../compact-tools/* dependencies.
- Around line 24-32: The npm scripts "compact", "compact:fast",
"compact:access", "compact:archive", "compact:security", "compact:token", and
"compact:utils" include the unsupported --hierarchical flag; update each script
in package.json to remove --hierarchical (preserve the compiler version
"+0.26.0" and any other valid flags like --skip-zk or --dir), leaving
"compact:fast" and the "test" script's --skip-zk intact; verify the "build" and
"test" scripts remain unchanged aside from this removal so compact-compiler
invocations use only supported flags.
🧹 Nitpick comments (6)
package.json (1)
11-16: Confirm lint scope aftercd contracts.These scripts now only lint files under
contracts/. If you still want lint coverage for root-level files (workflows/scripts/configs), consider running Biome at the repo root as well.🔧 Optional tweak to cover both scopes
- "fmt-and-lint": "cd contracts && biome check . --changed", - "fmt-and-lint:fix": "cd contracts && biome check . --changed --write", - "fmt-and-lint:ci": "cd contracts && biome ci . --changed --no-errors-on-unmatched", + "fmt-and-lint": "biome check . --changed && (cd contracts && biome check . --changed)", + "fmt-and-lint:fix": "biome check . --changed --write && (cd contracts && biome check . --changed --write)", + "fmt-and-lint:ci": "biome ci . --changed --no-errors-on-unmatched && (cd contracts && biome ci . --changed --no-errors-on-unmatched)",.github/actions/setup/action.yml (1)
31-38: Add a guard for missing submodule checkout.If a workflow or local run uses this action without submodules initialized, the
cd compact-toolswill fail with a generic error. Consider adding a clear check and message.✅ Suggested guard
- name: Build compact-tools submodule shell: bash run: | echo "📦 Building compact-tools submodule..." + if [ ! -d "compact-tools" ]; then + echo "::error::compact-tools submodule not found. Ensure checkout uses submodules: true." + exit 1 + fi cd compact-tools yarn install --immutable yarn build echo "✅ compact-tools built successfully"Makefile (2)
10-17: Consider immutable installs for deterministic setup.Local setup currently allows lockfile drift. If you want parity with CI, switch to
--immutable(and optionally add a separate target for intentional lockfile updates).🔧 Suggested change
install: ## Install dependencies for the main project `@echo` "📦 Installing dependencies..." - `@yarn` install + `@yarn` install --immutable build-submodule: ## Build the compact-tools submodule `@echo` "🔨 Building compact-tools submodule..." - `@cd` compact-tools && yarn && yarn build + `@cd` compact-tools && yarn install --immutable && yarn build
1-8: Optional: add standard phony targets to satisfy checkmake.
checkmakewarns about missingall,clean, andtest. If you want lint-clean Makefiles, consider adding light stubs.🧹 Optional additions
-.PHONY: help setup install build-submodule submodule-init submodule-update +.PHONY: help setup install build-submodule submodule-init submodule-update all clean test + +all: setup ## Default target + +clean: ## Cleanup artifacts (define as needed) + `@echo` "No clean steps defined." + +test: ## Run tests (define as needed) + `@echo` "No test steps defined."README.md (2)
52-55: Add troubleshooting guidance for submodule initialization failures.If users don't have access to the
compact-toolssubmodule (assuming it's private), these setup commands will fail with authentication errors. Consider adding a troubleshooting section that explains:
- How to verify submodule access
- What to do if
git submodule update --init --recursivefails with authentication errors- Prerequisites for accessing the
compact-toolsrepository📝 Suggested addition to Troubleshooting section
Add to the Troubleshooting section (around line 238):
- **Submodule initialization fails?** Ensure you have access to the `compact-tools` repository. If you're an external contributor and encounter authentication errors, please wait until `compact-tools` is published as a public package or repository.Also applies to: 60-64
150-155: Document submodule access requirements consistently.While the instructions correctly mention the
compact-toolssubmodule and use the--recursiveflag, there's no warning or prerequisite about needing access to the submodule repository. This information should be consistent with the Installation section and mentioned early in the Development setup section.📋 Suggested prerequisite addition
Add before line 150:
> ### ⚠️ Submodule Access > > This repository uses the [`compact-tools`](https://github.com/OpenZeppelin/compact-tools) submodule. Ensure you have access to this repository before proceeding with setup.
| "ignoreUnknown": true, | ||
| "experimentalScannerIgnores": ["compact-tools/**"], | ||
| "includes": [ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Biome 2.3.8 experimentalScannerIgnores configuration documentation
💡 Result:
In Biome v2.3.8, experimentalScannerIgnores is documented under the configuration reference as:
- Config key:
files.experimentalScannerIgnores - Type: array of literal path segments / basenames
- Behavior: tells Biome’s scanner to skip indexing those paths during crawling (so they won’t be part of the module graph and won’t contribute to type inference)
- Limitations: no glob patterns; only basenames are matched; recommended only as a last resort for performance. [1]
Example:
{
"files": {
"experimentalScannerIgnores": ["lodash", "dist", "RedisCommander.d.ts"]
}
}
``` [1]
Note: In the newer docs (post‑2.3.8), this option is marked **deprecated**, with guidance to use the **force-ignore syntax via `files.includes`** (notably `!!` to fully exclude from indexing). [2][3]
**Sources:** Biome configuration reference (includes `files.experimentalScannerIgnores`) [1][2], Biome v2.3 blog (explains `!` vs `!!` and the deprecation) [3].
---
</details>
**Move `experimentalScannerIgnores` under `files` and use basename matching instead of glob patterns.**
The correct config path is `files.experimentalScannerIgnores`, not at the root level. Additionally, this option only accepts literal basenames or path segments—not glob patterns. `"compact-tools/**"` will not work as intended. Either use `"compact-tools"` as a basename, or switch to `files.includes` with the `!!` force-ignore syntax (the recommended approach in current Biome versions).
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @biome.json around lines 10 - 12, Move the experimentalScannerIgnores entry
from the root to the files section by creating files.experimentalScannerIgnores
and removing the root-level experimentalScannerIgnores; replace the glob
"compact-tools/**" with a literal basename "compact-tools" because
experimentalScannerIgnores only accepts basenames/path segments, or
alternatively remove it and add a files.includes entry using the !! force-ignore
syntax (e.g., files.includes with an !! pattern) if you need glob-like behavior.
Ensure you update the key name exactly as files.experimentalScannerIgnores
and/or use files.includes for force-ignore patterns.
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| - @tsconfig/node24 to @openzeppelin/compact-/contracts, @openzeppelin/compact-/compact, @openzeppelin/compact-/contracts-simulator (#278) | ||
| - OpenZeppelin Compact Simulator (#247) | ||
|
|
||
| ### Changed | ||
|
|
||
| - Bump compact compiler to v0.25.0 (#233) | ||
| - Bump .nvmrc to v24.9.0 (#278) | ||
| - Upgrade @types/node 22.18.0 -> 24.9.0 in openzeppelin-compact, @openzeppelin-compact/contracts, @openzeppelin-compact/compact, @openzeppelin-compact/contracts-simulator (#278) | ||
| - Bump node version requirement to >=22 in @openzeppelin-compact/contracts and @openzeppelin-compact/contracts-simulator (#278) | ||
| - Upgrade @types/node 22.18.0 -> 24.9.0 in openzeppelin/compact-, @openzeppelin/compact-/contracts, @openzeppelin/compact-/compact, @openzeppelin/compact-/contracts-simulator (#278) | ||
| - Bump node version requirement to >=22 in @openzeppelin/compact-/contracts and @openzeppelin/compact-/contracts-simulator (#278) | ||
|
|
||
| ### Removed | ||
|
|
||
| - @tsconfig/node22 from @openzeppelin-compact/contracts, @openzeppelin-compact/compact, @openzeppelin-compact/contracts-simulator (#278) | ||
| - @tsconfig/node22 from @openzeppelin/compact-/contracts, @openzeppelin/compact-/compact, @openzeppelin/compact-/contracts-simulator (#278) | ||
| - Bump compact compiler to v0.26.0 (#279) | ||
| - Upgrade @midnight-ntwrk/compact-runtime ^0.8.1 -> ^0.9.0 (#279) | ||
| - Move @openzeppelin-compact/compact to its own package in the package/compact dir (#247) | ||
| - Move @openzeppelin/compact-/compact to its own package in the package/compact dir (#247) |
There was a problem hiding this comment.
Fix malformed package scope in changelog entries.
Several entries read @openzeppelin/compact-/..., which appears to be a typo and could confuse users.
✍️ Suggested corrections (example)
-- `@tsconfig/node24` to `@openzeppelin/compact-/contracts`, `@openzeppelin/compact-/compact`, `@openzeppelin/compact-/contracts-simulator` (`#278`)
+- `@tsconfig/node24` to `@openzeppelin/compact-contracts`, `@openzeppelin/compact-compact`, `@openzeppelin/compact-contracts-simulator` (`#278`)
-- Upgrade `@types/node` 22.18.0 -> 24.9.0 in openzeppelin/compact-, `@openzeppelin/compact-/contracts`, `@openzeppelin/compact-/compact`, `@openzeppelin/compact-/contracts-simulator` (`#278`)
+- Upgrade `@types/node` 22.18.0 -> 24.9.0 in `@openzeppelin/compact-contracts`, `@openzeppelin/compact-compact`, `@openzeppelin/compact-contracts-simulator` (`#278`)
-- Bump node version requirement to >=22 in `@openzeppelin/compact-/contracts` and `@openzeppelin/compact-/contracts-simulator` (`#278`)
+- Bump node version requirement to >=22 in `@openzeppelin/compact-contracts` and `@openzeppelin/compact-contracts-simulator` (`#278`)
-- `@tsconfig/node22` from `@openzeppelin/compact-/contracts`, `@openzeppelin/compact-/compact`, `@openzeppelin/compact-/contracts-simulator` (`#278`)
+- `@tsconfig/node22` from `@openzeppelin/compact-contracts`, `@openzeppelin/compact-compact`, `@openzeppelin/compact-contracts-simulator` (`#278`)
-- Move `@openzeppelin/compact-/compact` to its own package in the package/compact dir (`#247`)
+- Move `@openzeppelin/compact-compact` to its own package in the package/compact dir (`#247`)🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 14 - 29, Fix the malformed package scopes in
CHANGELOG.md by replacing occurrences of the incorrect token
"@openzeppelin/compact-/" with the correct package scopes (e.g.,
"@openzeppelin/compact" or the intended scoped package names) so entries like
"@openzeppelin/compact-/contracts", "@openzeppelin/compact-/compact", and
"@openzeppelin/compact-/contracts-simulator" are corrected to their proper
scoped package identifiers; ensure all similar entries (including the three
instances shown and any other "@openzeppelin/compact-/" matches) are updated
consistently and run a quick grep/search to verify no remaining malformed scopes
exist.
| @@ -0,0 +1 @@ | |||
| Subproject commit 1301ca6f4d2003b6ab968685dd65e440c9340655 | |||
There was a problem hiding this comment.
Major: Private repository submodule blocks external contributors.
The compact-tools submodule references a private repository, which will prevent external contributors from cloning this repository until compact-tools is made public. This is a known blocker (acknowledged in PR comments and blocked by compact-tools PR #5), but it fundamentally breaks the open-source nature of this project in its current state.
Recommendation: As noted by reviewer andrew-fleming, the preferred path is to make compact-tools public and release it as a proper npm package. Until then, this PR should remain in draft status and should not be merged.
🧰 Tools
🪛 GitHub Actions: CodeQL Analysis
[error] 1-1: Missing required file: compact-tools/packages/cli/dist/runCompiler.js
[error] 1-1: Missing required file: compact-tools/packages/cli/dist/runBuilder.js
🪛 GitHub Actions: Compact Contracts Test Suite
[error] 1-1: Missing required file: compact-tools/packages/cli/dist/runCompiler.js
[error] 1-1: Missing required file: compact-tools/packages/cli/dist/runBuilder.js
🪛 GitHub Actions: Format and Lint
[error] 1-1: Missing required file: compact-tools/packages/cli/dist/runCompiler.js
[error] 1-1: Missing required file: compact-tools/packages/cli/dist/runBuilder.js
🤖 Prompt for AI Agents
In `@compact-tools` at line 1, The repository includes a git submodule named
compact-tools that points to a private repository, which prevents external
contributors from cloning; to fix this, either make the compact-tools repository
public and publish it as an npm package (then replace the submodule dependency
with a proper package.json dependency) or remove the submodule from the repo and
add a documented placeholder/installation step until compact-tools is public;
update any references to compact-tools in the codebase (imports, build scripts,
and README) so they point to the npm package name or the new public repo
location and keep this PR as draft until the submodule is made public or
replaced.
| "compact": "compact-compiler +0.26.0 --hierarchical", | ||
| "compact:fast": "compact-compiler +0.26.0 --hierarchical --skip-zk", | ||
| "compact:access": "compact-compiler +0.26.0 --dir access --hierarchical", | ||
| "compact:archive": "compact-compiler +0.26.0 --dir archive --hierarchical", | ||
| "compact:security": "compact-compiler +0.26.0 --dir security --hierarchical", | ||
| "compact:token": "compact-compiler +0.26.0 --dir token --hierarchical", | ||
| "compact:utils": "compact-compiler +0.26.0 --dir utils --hierarchical", | ||
| "build": "compact-builder", | ||
| "test": "compact-compiler --skip-zk && vitest run", | ||
| "test": "compact-compiler +0.26.0 --skip-zk && vitest run", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
compact-compiler 0.26.0 --hierarchical --skip-zk flags documentation
💡 Result:
For Compact compiler (compactc) / toolchain 0.26.0, the official flag docs list --skip-zk but do not list any --hierarchical flag.
--skip-zk: “causes the compiler to skip the generation of proving keys” (useful because proving key generation can be time-consuming; it’s also skipped ifzkircan’t be found). (docs.midnight.network)- The same manual page enumerates the supported flags (
--help,--version,--language-version,--vscode,--skip-zk,--no-communications-commitment,--sourceRoot ...,--trace-passes) and--hierarchicalis not among them. (docs.midnight.network)
If you’re seeing --hierarchical accepted somewhere, it’s likely coming from a different command/binary (or an older/internal build). The docs recommend checking the tool’s built-in help for the exact options your installed version supports (e.g., compact compile --help). (docs.midnight.network)
Citations:
- 1: https://docs.midnight.network/develop/reference/tools/compiler-usage
- 2: https://docs.midnight.network/develop/reference/tools/compiler-usage
- 3: https://docs.midnight.network/develop/tutorial/building?utm_source=openai
Remove unsupported --hierarchical flag from compact-compiler commands.
The flag is not documented in the official compact-compiler 0.26.0 documentation. The supported flags are: --help, --version, --language-version, --vscode, --skip-zk, --no-communications-commitment, --sourceRoot, and --trace-passes. Lines 24–31 use --hierarchical across seven npm scripts, which will cause these commands to fail at runtime. The --skip-zk flag (line 32) is valid and properly used.
🤖 Prompt for AI Agents
In `@contracts/package.json` around lines 24 - 32, The npm scripts "compact",
"compact:fast", "compact:access", "compact:archive", "compact:security",
"compact:token", and "compact:utils" include the unsupported --hierarchical
flag; update each script in package.json to remove --hierarchical (preserve the
compiler version "+0.26.0" and any other valid flags like --skip-zk or --dir),
leaving "compact:fast" and the "test" script's --skip-zk intact; verify the
"build" and "test" scripts remain unchanged aside from this removal so
compact-compiler invocations use only supported flags.
| "@openzeppelin/compact-tools-cli": "file:../compact-tools/packages/cli", | ||
| "@openzeppelin/compact-tools-simulator": "file:../compact-tools/packages/simulator", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if .gitmodules exists and shows submodule configuration
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"Repository: OpenZeppelin/compact-contracts
Length of output: 176
🏁 Script executed:
# Check the current state of compact-tools directory
ls -la compact-tools 2>/dev/null || echo "compact-tools directory not found"Repository: OpenZeppelin/compact-contracts
Length of output: 182
🏁 Script executed:
# Check git config for submodule configuration
git config --file=.gitmodules --get-regexp path 2>/dev/null || echo "No submodule paths in config"Repository: OpenZeppelin/compact-contracts
Length of output: 116
🏁 Script executed:
# Check if the paths referenced in package.json actually exist
echo "Checking cli package..."
test -f compact-tools/packages/cli/package.json && echo "✓ cli/package.json exists" || echo "✗ cli/package.json not found"
echo "Checking simulator package..."
test -f compact-tools/packages/simulator/package.json && echo "✓ simulator/package.json exists" || echo "✗ simulator/package.json not found"Repository: OpenZeppelin/compact-contracts
Length of output: 191
🏁 Script executed:
# Look for any documentation about setup/initialization
find . -maxdepth 2 -type f \( -name "README*" -o -name "SETUP*" -o -name ".github" \) -exec echo "Found: {}" \;Repository: OpenZeppelin/compact-contracts
Length of output: 92
🏁 Script executed:
# Check contracts/package.json to see the exact dependencies
head -50 contracts/package.json | tail -15Repository: OpenZeppelin/compact-contracts
Length of output: 485
🏁 Script executed:
# Check README for setup instructions
cat README.md | head -100Repository: OpenZeppelin/compact-contracts
Length of output: 3034
Submodule initialization is required before install; already documented in README but worth reinforcing.
The file:../compact-tools/packages/... dependencies require the compact-tools submodule to be initialized first. The README correctly documents the sequence (git submodule update --init --recursive before yarn), but developers following a standard npm install or yarn without the submodule step will encounter failures when resolving these local package references.
Consider adding a setup validation script or preinstall hook to ensure the submodule is initialized before dependency resolution.
🤖 Prompt for AI Agents
In `@contracts/package.json` around lines 40 - 41, The package.json currently
depends on local submodule packages "@openzeppelin/compact-tools-cli" and
"@openzeppelin/compact-tools-simulator" which fail if the compact-tools
submodule isn't initialized; add a preinstall script in contracts' package.json
that checks whether the submodule is initialized (e.g., verify existence of
../compact-tools/packages or run a lightweight git submodule status check) and
either runs git submodule update --init --recursive or fails with a clear
message instructing the developer to run that command; update the "scripts"
section to include this "preinstall" hook so npm/yarn will validate/init the
submodule before resolving the file:../compact-tools/* dependencies.
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
@0xisk the changes look mostly good! I left some comments. There's also an issue with the types script—we need to remove a dep from the turbo schema (https://github.com/OpenZeppelin/compact-contracts/blob/fix/compact-tools/turbo.json#L78)
× Could not find package "@openzeppelin-compact/compact" referenced by task "@openzeppelin-compact/compact#build" in project
╭─[turbo.json:78:9]
77 │ "dependsOn": [
78 │ "@openzeppelin-compact/compact#build",
· ─────────────────────────────────────
79 │ "@openzeppelin/compact-contracts#compact"
╰────
A more meta review: I think introducing submodules for a helper package here adds unnecessary complexity and is ultimately more trouble than it's worth. AFAICT this isn't really blocking anything on the apps PR. I found one import for the isKeyOrAddressZero circuit which can be tmp c/p
Another way to get this to 0.28.0, taking a pessimistic view on when the tools package will be published, is to just update the existing subpackages and imports directly, no? It's not sexy, but this avoids refactoring the CI, docs, manifests, etc which has double the value bc if we merge this, we're adding technical debt that we'll have to change back
If you prefer we continue with submodules and @emnul is on board, let's address the comments, get the CI passing, and we should be good to roll 👍
| "compact": "compact-compiler +0.26.0 --hierarchical", | ||
| "compact:fast": "compact-compiler +0.26.0 --hierarchical --skip-zk", | ||
| "compact:access": "compact-compiler +0.26.0 --dir access --hierarchical", | ||
| "compact:archive": "compact-compiler +0.26.0 --dir archive --hierarchical", | ||
| "compact:security": "compact-compiler +0.26.0 --dir security --hierarchical", | ||
| "compact:token": "compact-compiler +0.26.0 --dir token --hierarchical", | ||
| "compact:utils": "compact-compiler +0.26.0 --dir utils --hierarchical", |
There was a problem hiding this comment.
All of the imports expect a flattened structure
There was a problem hiding this comment.
Oh scratch that, the test script compiles without the flag. Nit: would consistency be better with compiling vs compile + testing? Right now, my artifacts dir is including both flattened + hierarchical
| }, | ||
| "devDependencies": { | ||
| "@openzeppelin-compact/contracts-simulator": "workspace:^", | ||
| "@openzeppelin/compact-tools-cli": "file:../compact-tools/packages/cli", |
There was a problem hiding this comment.
The CLI is required to build the package for the compiled artifacts. I think this should be a regular dep
| "fmt-and-lint": "cd contracts && biome check . --changed", | ||
| "fmt-and-lint:fix": "cd contracts && biome check . --changed --write", | ||
| "fmt-and-lint:ci": "cd contracts && biome ci . --changed --no-errors-on-unmatched", |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
AFAICT this isn't really blocking anything on the apps PR. I found one import for the isKeyOrAddressZero circuit which can be tmp c/p
And the import here:
import { isKeyOrAddressZero } from "../../../../compact-contracts/contracts/src/utils/Utils";
isn't even needed since there's this in the adjacent Utils.compact in the oz directory within shielded-token/:
I'm with @andrew-fleming on this one. We're adding a lot of tech debit for something that we will ultimately have to remove again in a couple months. |
|
Thank you @andrew-fleming and @emnul that's what I like about being in an odd numbered team, it makes it easier to decide 😄 I don't disagree with you and will close this PR in favor of #291 |
3 participants
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applycompact-toolsas a submodule (until it is published then we can use the npm version"@openzeppelin-compact/contractsto@openzeppelin/compact-contractsFixes: #362
Blocked by: OpenZeppelin/compact-tools#5
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.