feat: compact-deployer tool - #86
Merged
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a deployer package with configuration loading, artifact resolution, wallet management, provider construction, deployment persistence, programmatic APIs, and CLI support. It also adds coverage configuration, documentation, and extensive unit tests. ChangesDeployer platform rollout
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔵 Low · up to This PR adds a new deployment tool and CLI, but the current head still has bounded merge-readiness issues including potentially unreadable keystores, unhandled wallet-checkpoint failures, permissive input parsing, temporary-file leakage, and misleading usage guidance. It is mergeable with explicit owner follow-up on these items. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 99 functions across 54 files. (6 skipped: 6 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
⚠️ 9 New Security Findings
The latest commit contains 9 new security findings.
| Findings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Dependency: yarn / brace-expansion@ 2.0.2 SUMMARY Direct Dependency: brace-expansion Location : yarn.lock OCCURRENCE
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / ip-address@ 10.0.1 SUMMARY Direct Dependency: ip-address Location : yarn.lock OCCURRENCE
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / minimatch@ 9.0.5 SUMMARY Direct Dependency: minimatch Location : yarn.lock OCCURRENCES
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / picomatch@ 4.0.3 SUMMARY Direct Dependency: picomatch Location : yarn.lock OCCURRENCES
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / postcss@ 8.5.6 SUMMARY Direct Dependency: postcss Location : yarn.lock OCCURRENCE
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / rollup@ 4.52.5 SUMMARY Direct Dependency: rollup Location : yarn.lock OCCURRENCE
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / tar@ 7.5.3 SUMMARY Direct Dependency: tar Location : yarn.lock OCCURRENCES
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / undici@ 5.29.0 SUMMARY Direct Dependency: undici Location : yarn.lock OCCURRENCES
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: yarn / vite@ 7.1.12 SUMMARY Direct Dependency: vite Location : yarn.lock OCCURRENCES
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
Not a finding? Ignore it by adding a comment on the line with just the word noboost.
Scanner: boostsecurity - Trivy (Filesystem scanning)
There was a problem hiding this comment.
⚠️ 10 New Security Findings
The latest commit contains 10 new security findings.
| Findings | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Dependency: npm / @substrate/connect@ 0.8.11 SUMMARY Dependency: @substrate/connect Transitive through:
Location : yarn.lock OCCURRENCE
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / glob@ 10.5.0 SUMMARY Dependency: glob Transitive through:
Location : yarn.lock OCCURRENCE
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / minimatch@ 9.0.5 SUMMARY Dependency: minimatch Transitive through:
Location : yarn.lock OCCURRENCES
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / node-domexception@ 1.0.0 SUMMARY Dependency: node-domexception Transitive through:
Location : yarn.lock OCCURRENCE
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / picomatch@ 4.0.3 SUMMARY Dependency: picomatch Transitive through:
Location : yarn.lock OCCURRENCE
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / rollup@ 4.52.5 SUMMARY Dependency: rollup Transitive through:
Location : yarn.lock OCCURRENCE
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / tar@ 7.5.3 SUMMARY Dependency: tar Transitive through:
Location : yarn.lock OCCURRENCES
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / undici@ 5.29.0 SUMMARY Dependency: undici Transitive through:
Location : yarn.lock OCCURRENCES
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / uuid@ 10.0.0 SUMMARY Dependency: uuid Transitive through:
Location : yarn.lock OCCURRENCE
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Dependency: npm / vite@ 7.1.12 SUMMARY Dependency: vite Transitive through:
Location : yarn.lock OCCURRENCES
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Remediation :
|
Not a finding? Ignore it by adding a comment on the line with just the word noboost.
Scanner: boostsecurity - BoostSecurity SCA
0xisk
force-pushed
the
feat/compact-deployer
branch
from
May 19, 2026 12:29
958d227 to
26aa42b
Compare
0xisk
added a commit
that referenced
this pull request
May 19, 2026
…ions Addresses the BoostSecurity findings on PR #86. Eight of the originally flagged packages auto-upgraded when yarn.lock regenerated post-rebase (brace-expansion, ip-address, minimatch, picomatch, postcss, rollup, tar, vite). The remaining three are forced via root resolutions: - undici ^6.24.0 (was 5.29.0 via testcontainers) Fixes CVE-2026-1525/1526/1527/22036/2229 — HTTP smuggling, WebSocket memory exhaustion, CRLF injection, decompression DoS. Major bump but testcontainers' usage stays within the v6 API. - glob ^11.0.0 (was 10.5.0 via archiver-utils, cacache) Replaces the EOL v10 line. - uuid ^13.0.0 (was 10.0.0 via dockerode) Replaces the EOL v10 line. Two findings left unaddressed: - @substrate/connect@0.8.11 (EOL warning). Pinned exactly by `@polkadot/rpc-provider@16.5.6` (constraint is `0.8.11`, not a range), so resolutions can't override it without breaking that transitive. Would need to wait for @PolkaDot to publish a release that drops the EOL dep. - node-domexception@1.0.0 (EOL warning). Pulled by `fetch-blob@3.2.0`. Modern Node has a native DOMException; the package only matters until fetch-blob/undici upstream drops the polyfill import. No safe override exists. Both are warning-level (EOL classification, no active CVE). Build + 50 unit tests + CLI typecheck still pass after resolutions.
0xisk
marked this pull request as ready for review
June 8, 2026 09:42
0xisk
marked this pull request as draft
June 8, 2026 09:49
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
packages/deployer/src/wallet/keystore.ts-196-201 (1)
196-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce seed length constraints at keystore creation time.
seedFromHexcurrently accepts any even-length hex, so users can create keystores that later failclassifySeed(which expects 64/128 hex chars). Validate length here to fail fast.Suggested fix
function seedFromHex(hex: string): Buffer { const stripped = hex.startsWith('0x') ? hex.slice(2) : hex; - if (!/^[0-9a-fA-F]+$/.test(stripped) || stripped.length % 2 !== 0) { - throw new WalletError('Seed must be hex-encoded'); + const isValidHex = /^[0-9a-fA-F]+$/.test(stripped); + const isSupportedLength = stripped.length === 64 || stripped.length === 128; + if (!isValidHex || !isSupportedLength) { + throw new WalletError('Seed must be a 64- or 128-char hex string'); } return Buffer.from(stripped, 'hex'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/src/wallet/keystore.ts` around lines 196 - 201, seedFromHex currently accepts any even-length hex and should instead enforce the expected seed lengths so keystores fail fast: in seedFromHex, after stripping the 0x prefix and validating hex characters, check that stripped.length is either 64 or 128 (matching classifySeed expectations) and throw a WalletError with a clear message if not; otherwise continue to return Buffer.from(stripped, 'hex'). Reference: seedFromHex and classifySeed.tests/integrations/README.md-7-25 (1)
7-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to the fenced layout block.
Line 7 opens a fenced code block without a language tag, which triggers markdownlint MD040.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/README.md` around lines 7 - 25, The README's fenced code block starting with ``` lacks a language tag (markdownlint MD040); update the opening fence from ``` to include a language specifier (for example ```text or ```bash) so the block is properly tagged—modify the README's fenced block that contains the directory tree (the ``` fence and its closing ``` fence) to add the chosen language tag.Source: Linters/SAST tools
tests/integrations/_harness/paths.ts-20-37 (1)
20-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen fixture artifact preflight checks beyond directory existence.
existsSync(...)alone accepts empty/stale directories, so tests can pass preflight and fail later with less clear errors. Validate that the artifact directory contains expected compiled outputs (or at least is non-empty).Suggested patch
-import { existsSync, rmSync } from 'node:fs'; +import { existsSync, readdirSync, rmSync } from 'node:fs'; @@ +function hasCompiledArtifact(dir: string): boolean { + return existsSync(dir) && readdirSync(dir).length > 0; +} + export function requireFixtureArtifact(): void { - if (existsSync(ARTIFACT_DIR)) return; + if (hasCompiledArtifact(ARTIFACT_DIR)) return; @@ export function requirePrivateCounterArtifact(): void { - if (existsSync(PRIVATE_COUNTER_ARTIFACT_DIR)) return; + if (hasCompiledArtifact(PRIVATE_COUNTER_ARTIFACT_DIR)) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/_harness/paths.ts` around lines 20 - 37, The current preflight helpers requireFixtureArtifact and requirePrivateCounterArtifact only call existsSync(ARTIFACT_DIR) / existsSync(PRIVATE_COUNTER_ARTIFACT_DIR), which permits empty/stale directories; update both functions to ensure the artifact directory is not only present but contains expected compiled outputs (e.g., check readdirSync(...) yields at least one file and/or assert presence of expected filenames/extensions such as .wasm/.json); if the directory is empty or missing expected files, throw the same Error with the helpful "Run `make -C tests/integrations compile` first." message so failures surface earlier and with clearer diagnostics.packages/deployer/src/loaders/constructor-meta.ts-34-46 (1)
34-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse own-property checks when validating named args.
Using
incan match prototype properties. UseObject.hasOwn/hasOwnPropertyto validate only explicit user-provided keys.Proposed fix
export function reorderNamedArgs( named: Record<string, unknown>, paramNames: readonly string[], ): unknown[] { - const missing = paramNames.filter((n) => !(n in named)); + const hasOwn = Object.prototype.hasOwnProperty; + const missing = paramNames.filter((n) => !hasOwn.call(named, n)); if (missing.length > 0) { throw new ConfigError( `args object is missing constructor parameter(s): ${missing.join(', ')}`, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/src/loaders/constructor-meta.ts` around lines 34 - 46, The validation uses the `in` operator which matches prototype properties; update the missing-parameter check in the constructor args validation to only consider own properties by replacing `(n in named)` with an own-property check such as `Object.hasOwn(named, n)` (or `Object.prototype.hasOwnProperty.call(named, n)` for older runtimes), keeping the rest of the logic (throwing ConfigError, computing extra via Object.keys(named), and returning `paramNames.map((n) => named[n])`) unchanged.packages/deployer/src/providers/private-state-password.test.ts-38-60 (1)
38-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure mocked
node:cryptois always restored.Cleanup at Line 58-59 won’t run if the test fails earlier, which can contaminate subsequent tests in the same worker. Wrap the mock scope in
try/finally.Suggested fix
it('should throw after 1024 rounds when every hash has 4+ identical chars', async () => { - // Force every round to produce a string that hits the `(.)\1{3,}` guard so - // the loop exhausts its retry budget and reaches the explicit throw. - vi.resetModules(); - vi.doMock('node:crypto', async () => { - const actual = - await vi.importActual<typeof import('node:crypto')>('node:crypto'); - return { - ...actual, - createHash: () => ({ - update: () => ({ - digest: () => 'aaaaBBBBccccDDDD', - }), - }), - }; - }); - const { derivePrivateStatePassword: derive } = await import( - './private-state-password.ts' - ); - expect(() => derive('anything')).toThrow(/unable to find a hash/); - vi.doUnmock('node:crypto'); - vi.resetModules(); + vi.resetModules(); + try { + vi.doMock('node:crypto', async () => { + const actual = + await vi.importActual<typeof import('node:crypto')>('node:crypto'); + return { + ...actual, + createHash: () => ({ + update: () => ({ + digest: () => 'aaaaBBBBccccDDDD', + }), + }), + }; + }); + const { derivePrivateStatePassword: derive } = await import( + './private-state-password.ts' + ); + expect(() => derive('anything')).toThrow(/unable to find a hash/); + } finally { + vi.doUnmock('node:crypto'); + vi.resetModules(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/src/providers/private-state-password.test.ts` around lines 38 - 60, The test mocks node:crypto but restores it after assertions, which won't run if the test throws; wrap the mock/require/assert/unmock sequence in a try/finally so node:crypto is always restored: call vi.resetModules(); vi.doMock('node:crypto', ...) then import { derivePrivateStatePassword as derive } and run the expect inside the try block, and in finally call vi.doUnmock('node:crypto') and vi.resetModules(); reference derivePrivateStatePassword / derive and the vi.doMock/vi.doUnmock calls so the mock is reliably cleaned up even when the test throws.packages/deployer/README.md-23-37 (1)
23-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd languages to fenced code blocks for lint compliance.
Lines 23 and 64 use unlabeled fenced blocks, which triggers MD040.
Suggested fix
-``` +```text compact-deploy <Contract> --network <name> required unless [profile].default_network is set --config <path> default: walk up from CWD for compact.toml @@ -v, --verbose pino debug logs to .compact/logs/<timestamp>.log -h, --help --version@@
-+bash
compact-deploy --network preprod
--seed-cache-from-dust /path/to/state.json
--seed-cache-from-shielded /path/to/shielded.json # optionalAlso applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/README.md` around lines 23 - 37, The README has unlabeled fenced code blocks causing MD040; update the two unlabeled blocks that show the CLI usage and example (the blocks beginning with "compact-deploy <Contract>" and the example "compact-deploy <Contract> --network preprod \") by adding appropriate language identifiers: use "text" for the CLI usage block and "bash" (or "sh") for the example block. Ensure both fenced blocks are updated so the first starts with ```text and the second with ```bash to satisfy the linter.Source: Linters/SAST tools
packages/deployer/README.md-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken roadmap anchor link.
Line 7 links to
#roadmap--todo, but this README has no matching section, so the link is dead.Suggested fix
-> **Status: developer-preview, testnet only.** Verified on local devnet + preview. Preprod blocked ([Known issues](`#known-issues-may-2026`)). Mainnet unsupported: unaudited, no hardware signer, no multisig, no tx retry, no upgrade tooling. See [Roadmap](`#roadmap--todo`). +> **Status: developer-preview, testnet only.** Verified on local devnet + preview. Preprod blocked ([Known issues](`#known-issues-may-2026`)). Mainnet unsupported: unaudited, no hardware signer, no multisig, no tx retry, no upgrade tooling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/README.md` at line 7, The README contains a dead anchor link `#roadmap--todo`; update the markdown to point to the actual Roadmap heading or create a matching section: either change the link target in the Status line from `#roadmap--todo` to the real anchor (e.g., `#roadmap--todo` -> `#roadmap` or `#roadmap--todo` -> `#roadmap-and-todo` depending on the existing heading text), or add a "Roadmap / TODO" heading that generates the `#roadmap--todo` anchor so the link resolves; ensure the visible link text ("Roadmap") and the heading slug match.Source: Linters/SAST tools
packages/deployer/src/config/compact-config.test.ts-48-50 (1)
48-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest can pass for the wrong failure path.
Line 49 creates a second
[profile]table, which can fail at TOML parsing before thedefault_networksemantic check runs. This weakens what this test is asserting.💡 Suggested fix
- const dir = tmpRepo(`${MIN_VALID}\n[profile]\ndefault_network = "ghost"\n`); + const invalid = MIN_VALID.replace( + 'default_network = "local"', + 'default_network = "ghost"', + ); + const dir = tmpRepo(invalid);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/src/config/compact-config.test.ts` around lines 48 - 50, The test is creating a second [profile] table which can trigger a TOML parse error before the semantic check; change the tmpRepo content so it does not duplicate the [profile] header but instead adds default_network = "ghost" inside the already-existing profile from MIN_VALID (i.e. append 'default_network = "ghost"' under the existing [profile] rather than adding a new '[profile]' line) so CompactConfig.load(...) exercises the semantic validation for default_network missing.packages/deployer/src/deployer.ts-580-583 (1)
580-583:⚠️ Potential issue | 🟡 MinorAbsolute sync timeout concern is overstated for this pipeline
timeout({ each: timeoutMs })does restart on every emission, but becauseRx.filter((s: FacadeState) => s.isSynced)is upstream, onlyisSynced=trueemissions can reset the timer—progress emissions whereisSyncedis false won’t extend it. Switching tofirstis only necessary if the code can emit multipleisSynced=truevalues without terminating (e.g., it does not stop after the first synced emission).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deployer/src/deployer.ts` around lines 580 - 583, The current pipeline uses Rx.timeout({ each: timeoutMs }) after Rx.filter((s: FacadeState) => s.isSynced) so only emissions with s.isSynced === true will reset the timer; if you want any progress emission to extend the timeout, move Rx.timeout before the Rx.filter so all FacadeState emissions reset the timer, otherwise if your intent is to bound time until the first synced emission (and not allow repeated isSynced=true to restart it) replace the timeout call with Rx.timeout({ first: timeoutMs }); update the pipeline around Rx.timeout and Rx.filter accordingly.examples/fungible-token/README.md-16-20 (1)
16-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConstructor type table conflicts with the Uint mapping.
The table lists
_decimals/_feeBpsasnumber, but later this README statesUint<N>maps tobigintand examples use bigint literals. Please make the table consistent.Suggested fix
-| `_decimals` | `Uint<8>` | `number` | +| `_decimals` | `Uint<8>` | `bigint` | ... -| `_feeBps` | `Uint<32>` | `number` | +| `_feeBps` | `Uint<32>` | `bigint` |Also applies to: 143-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/fungible-token/README.md` around lines 16 - 20, The constructor type table incorrectly lists `_decimals` and `_feeBps` as `number` while `Uint<N>` is mapped to `bigint` elsewhere; update the table rows for `_decimals` and `_feeBps` to use `BigInt`/`bigint` (consistent with `_maxSupply`/`_quorum` and the README examples), and apply the same correction to the duplicated entries at lines referenced (around 143-144). Ensure the table consistently reflects that `Uint<N>` maps to `bigint`.examples/README.md-16-16 (1)
16-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign artifact policy docs across example READMEs.
This says artifacts are committed, but the fungible-token README describes
artifacts/as gitignored and locally generated. Please make both docs consistent to avoid broken setup expectations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/README.md` at line 16, The README currently states "Compiled artifacts ship committed..." which conflicts with the fungible-token README that says artifacts/ is gitignored and generated locally; make the artifact policy consistent across both READMEs by choosing one policy and updating the sentence that references "artifacts/" (the line currently claiming compiled artifacts are committed) to match the chosen policy, and ensure the fungible-token README has the same wording and instructions for setup, generation, or deployment so both docs present the same expectations.packages/cli/src/runDeploy.ts-106-112 (1)
106-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast on unknown short flags and extra positional args.
Current parsing treats unknown single-dash args as positional, and silently ignores extra positionals after the first contract. That produces confusing downstream errors instead of actionable CLI feedback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/runDeploy.ts` around lines 106 - 112, The parser currently treats unknown single-dash tokens as positional and allows extra positionals; update the default branch in the argument loop in runDeploy.ts so that if arg startsWith('-') (single-dash short flag) you throw an Error(`Unknown flag: ${arg}`) instead of pushing to out.positional, and after parsing validate out.positional: if out.positional.length === 0 throw a missing contract error, and if out.positional.length > 1 throw an Error indicating extra positional args (mention the unexpected tokens) before assigning out.contract = out.positional[0]; reference the variables arg, out.positional, and out.contract when locating where to change behavior.packages/cli/src/runDeploy.ts-95-103 (1)
95-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden
--sync-timeoutparsing to reject malformed values.
parseInt()accepts strings like10abc. This should fail validation instead of being treated as10.Suggested fix
- const seconds = Number.parseInt(raw, 10); - if (!Number.isFinite(seconds) || seconds <= 0) { + if (!/^\d+$/.test(raw)) { + throw new Error( + `--sync-timeout requires a positive integer (seconds); got "${raw}"`, + ); + } + const seconds = Number(raw); + if (!Number.isSafeInteger(seconds) || seconds <= 0) { throw new Error( `--sync-timeout requires a positive integer (seconds); got "${raw}"`, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/runDeploy.ts` around lines 95 - 103, The --sync-timeout branch currently uses Number.parseInt(raw, 10) which permits inputs like "10abc"; update the parsing in the case '--sync-timeout' (around expectValue and out.syncTimeoutSec) to reject malformed values by ensuring the raw string is an exact integer representation before assigning out.syncTimeoutSec — e.g., parse the number and verify String(seconds) === raw (or use a /^\d+$/ check) and then confirm Number.isFinite(seconds) && seconds > 0, otherwise throw the existing error with the original raw value.examples/fungible-token/README.md-26-43 (1)
26-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced tree block.
The block is missing a fence language and triggers MD040.
Suggested fix
-``` +```text fungible-token/ contracts/ ... -``` +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/fungible-token/README.md` around lines 26 - 43, The fenced tree block starting with "fungible-token/" in examples/fungible-token/README.md is missing a language tag (triggers MD040); update the opening fence from ``` to ```text so the block becomes a text-fenced code block (i.e., change the opening triple-backticks that precede "fungible-token/" to include the "text" tag).Source: Linters/SAST tools
🧹 Nitpick comments (3)
tests/integrations/specs/deploy/historyIsolation.spec.ts (1)
26-38: Shared deployment-state wipes—race unlikely under current single-fork config, but shared state still couples specs.
tests/integrations/vitest.config.tsuses Vitest forks withpoolOptions.forks.singleFork: true, which should prevent concurrent execution across spec files, reducing the flakiness risk from wipingtests/integrations/deployments/compactinhistoryIsolation.spec.ts(beforeAll/afterAll).Still, all specs share the same
DEPLOYMENTS_DIRand rely on wipes; if parallelism settings change, this becomes order-dependent. Consider isolating the deployments directory per spec/worker to avoid future nondeterminism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/specs/deploy/historyIsolation.spec.ts` around lines 26 - 38, The test currently calls wipeDeployments() in beforeAll/afterAll and uses shared DEPLOYMENTS_DIR (via requireFixtureArtifact()/deployFixture()), coupling specs; change the test setup to use an isolated deployments directory per spec/worker by deriving a unique DEPLOYMENTS_DIR (e.g. from Vitest worker id or a temp UUID) and pass it into requireFixtureArtifact()/deployFixture() or make wipeDeployments() operate on that per-test directory; update historyIsolation.spec.ts to set the unique directory before calling requireFixtureArtifact(), deployFixture(), and wipeDeployments() so each spec uses its own deployments folder and no longer shares global state.tests/integrations/specs/deploy/deploy.spec.ts (1)
33-35: ⚡ Quick winTighten hex ID assertions to catch truncation regressions.
/^[0-9a-f]+$/iis permissive and accepts malformed short values. Use length-constrained patterns (matching the deployer contract for address/tx IDs) so persistence and return-shape regressions fail fast.Suggested patch
+const HEX_64 = /^[0-9a-f]{64}$/i; @@ - expect(result.address).toMatch(/^[0-9a-f]+$/i); - expect(result.txId).toMatch(/^[0-9a-f]+$/i); - expect(result.txHash).toMatch(/^[0-9a-f]+$/i); + expect(result.address).toMatch(HEX_64); + expect(result.txId).toMatch(HEX_64); + expect(result.txHash).toMatch(HEX_64); @@ - expect(head.Counter.address).toMatch(/^[0-9a-f]+$/i); - expect(head.Counter.txHash).toMatch(/^[0-9a-f]+$/i); + expect(head.Counter.address).toMatch(HEX_64); + expect(head.Counter.txHash).toMatch(HEX_64);Also applies to: 47-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/specs/deploy/deploy.spec.ts` around lines 33 - 35, The hex assertions are too permissive; update the regexes for result.address, result.txId and result.txHash to enforce exact lengths used by the deployer contract (e.g., address = 40 hex chars, txId/txHash = 64 hex chars) so truncated values fail; replace /^[0-9a-f]+$/i with length-constrained patterns (e.g., /^[0-9a-f]{40}$/i and /^[0-9a-f]{64}$/i as appropriate) and make the same change for the other duplicate assertions further down in the spec.packages/cli/test/prompt.test.ts (1)
79-149: ⚡ Quick winAdd regression tests for stdin
endanderrorpaths.Please add cases asserting promise settlement when the stream ends/errors, so prompt termination behavior stays stable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/prompt.test.ts` around lines 79 - 149, Add two regression tests to prompt.test.ts that exercise stdin 'end' and 'error' paths: use promptPassphrase(...) to start the promise, trigger mockStdin.emit('end') in one test and assert the promise settles (rejects) and raw mode is turned off and stdin.pause() called; in the other test emit mockStdin.emit('error', new Error('boom')) and assert the promise rejects with that error (or the propagated error) and cleanup (setRawMode(false) and pause) occurs; locate tests near the existing describe blocks and reuse mockStdin.emit, mockStdin.setRawMode, and mockStdin.pause checks already used in other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/logger.ts`:
- Around line 18-20: The logger currently returns pino() to STDOUT when
createLogger({ json: true }) is used, which can interleave with the single JSON
result and prompt output; update createLogger to route pino logs to STDERR (or
disable them) when opts.json is true by providing an explicit
destination/transport to STDERR so all logger.* output goes to stderr, and also
update promptPassphrase (or any prompt printing) to write its prompt text to
STDERR when json mode is enabled; locate createLogger in
packages/cli/src/logger.ts and promptPassphrase (or the passphrase prompt
function) and change their output targets accordingly so only the final JSON
result is written to STDOUT.
In `@packages/cli/src/prompt.ts`:
- Around line 9-47: readMaskedLine currently can hang if stdin closes without a
newline; add handlers for stdin 'end' and 'error' events so the Promise always
settles: attach stdin.once('end', ...) to cleanup and resolve with the current
buffer, attach stdin.once('error', ...) to cleanup and reject with the error,
and ensure cleanup removes these listeners (and still restores raw mode, pauses,
removes 'data' listener and writes newline) so readMaskedLine always resolves or
rejects when input closes or errors; update references in the function
(readMaskedLine, onData, cleanup) accordingly.
In `@packages/deployer/src/config/schema.ts`:
- Around line 43-49: The union allows ambiguous objects because z.object strips
unknown keys by default; update fileRefSchema and moduleRefSchema to be strict
(e.g., call .strict() on each z.object) so that an input containing both file
and module will fail validation and therefore fileOrModuleRefSchema will reject
ambiguous refs; modify the definitions of fileRefSchema and moduleRefSchema
(used by fileOrModuleRefSchema) accordingly.
In `@packages/deployer/src/deployments.ts`:
- Around line 111-113: The writeJson function currently writes directly to the
target file which can corrupt the JSON if interrupted; make it atomic by writing
to a temporary file and then renaming it into place: open a temp path (e.g.
`${path}.tmp-${process.pid}-${Date.now()}`) with fs.promises.open, write the
JSON payload, call handle.sync() (fsync) and close the handle, then fs.rename
the temp to the final path; ensure any errors clean up the temp file. Update
writeJson to use this temp-write+fsync+rename sequence to guarantee crash-safe
durable writes.
In `@packages/deployer/src/loaders/artifact.ts`:
- Around line 65-67: The code currently hardcodes contractDir =
resolve(artifactPath, 'contract') but then calls findEntry(artifactPath) which
may return a top-level entry; fix by computing the actual compiled-asset
directory from the resolved entry returned by findEntry instead of the hardcoded
'contract' path. Concretely, call findEntry(artifactPath) first, then derive
contractDir from the entry (e.g., the entry's parent/resolved directory) and use
that contractDir everywhere assets are bound (the same places where contractDir
is passed later around entry/asset binding), replacing any remaining uses of the
original hardcoded resolve(artifactPath, 'contract').
In `@packages/deployer/src/loaders/constructor-meta.ts`:
- Around line 21-24: The current check throws a ConfigError when names.length
=== 0 even if the caller passed an empty named-args object; modify the logic so
that when names.length === 0 you detect an empty plain object (e.g. args is an
object and Object.keys(args).length === 0) and treat it as an empty positional
args array (resolve to []), otherwise keep throwing the ConfigError referencing
artifactPath; update the branch that currently throws the ConfigError (the
symbols names, artifactPath, and ConfigError) to perform this empty-object check
and return [] when appropriate.
In `@packages/deployer/src/loaders/contract-resolve.ts`:
- Around line 88-91: The current absolute-path check in resolveUnderRoot uses
artifact.startsWith('/'), which fails on Windows; change it to use
path.isAbsolute(artifact) (i.e., import/qualify isAbsolute from the path module)
so Windows-style absolute paths like C:\... and UNC paths are detected
correctly; keep the rest of the logic (resolve(rootDir, artifact), existsSync
check, fallback to resolve(rootDir, artifactsDir, artifact)) unchanged and
reference resolveUnderRoot, artifact, rootDir, artifactsDir when making the
edit.
In `@packages/deployer/src/providers/network.ts`:
- Around line 12-20: KNOWN_NETWORK_IDS currently includes 'mainnet' which
contradicts the intended testnet-only scope; update the set named
KNOWN_NETWORK_IDS to remove 'mainnet' so the deployer will no longer whitelist
or accept mainnet. Locate the constant KNOWN_NETWORK_IDS in
packages/deployer/src/providers/network.ts and delete the 'mainnet' entry (or
replace the set with a new ReadonlySet that omits 'mainnet') and ensure any code
relying on KNOWN_NETWORK_IDS (e.g., validation logic elsewhere) will now reject
mainnet networks accordingly.
In `@packages/deployer/src/providers/proof-server.ts`:
- Around line 65-69: The PROOF_SERVER_PORT parsing currently uses
Number.parseInt which allows partial numeric strings and doesn't check bounds;
update the validation around where port is parsed (the block using
Number.parseInt/Number.isNaN) to first ensure the string fully matches /^\d+$/
(reject anything like "6300abc" or empty/whitespace), then parse with
Number.parseInt or Number() and verify the resulting integer is between 1 and
65535 inclusive; if validation fails throw a ConfigError with a clear message
including the invalid value.
In `@packages/deployer/src/wallet/handler.ts`:
- Around line 448-455: Replace the manual '/' parsing in pathDir and pathBase
with Node's path utilities to handle platform-specific separators: import the
Node 'path' module and implement pathDir using path.dirname and pathBase using
path.basename so Windows backslashes are handled correctly and cache
directory/file resolution works across OSes; update any callers if necessary to
expect the same string behavior returned by path.dirname/path.basename
(functions: pathDir, pathBase).
In `@packages/deployer/src/wallet/keystore.ts`:
- Around line 125-141: fromJSON currently assumes data.crypto exists and will
throw a raw TypeError for malformed JSON; instead validate the shape and types
before accessing nested properties and throw a WalletError for bad input. In the
Keystore.fromJSON method (handling MidnightKeystore), add defensive checks that
data is an object, data.version exists, data.crypto is a non-null object, and
that data.crypto.kdf and data.crypto.cipher are strings; if any check fails,
throw a WalletError with a clear message about malformed keystore JSON; then
continue to validate values (kdf === 'scrypt', cipher === 'aes-128-ctr') and
return new Keystore(data). Ensure this protects callers like readFromFile from
leaking raw TypeErrors.
In `@tests/integrations/_harness/walletPool.ts`:
- Around line 56-66: The in-flight promise is being cached via
this.cache.set(alias, built) before the async build completes, so if
WalletHandler.build(...) or owned.provider.start(true) rejects the rejected
promise remains cached and signerFor(alias) will keep failing; change the flow
so you only cache a successfully-resolved wallet: either await the built result
before calling this.cache.set(alias, ...) or attach a .then(...) that stores the
resolved owned value and a .catch(...) that removes the cache entry on failure
(referencing built, WalletHandler.build, owned.provider.start, this.cache.set
and signerFor(alias) to locate the code).
In `@tests/integrations/local-env.yml`:
- Line 4: Replace floating :latest tags with pinned, deterministic tags for the
container images referenced by the image: keys — specifically update
midnightntwrk/proof-server and midnightntwrk/indexer-standalone to explicit
version tags or digests (e.g., midnightntwrk/proof-server:<version> and
midnightntwrk/indexer-standalone:<version> or use an IMAGE_* variable with a
pinned default) instead of :latest so integration runs are reproducible; ensure
both image entries match the existing pinned approach used for the node image
(node:0.22.2) and update any CI docs or env variables that control these image
values.
---
Minor comments:
In `@examples/fungible-token/README.md`:
- Around line 16-20: The constructor type table incorrectly lists `_decimals`
and `_feeBps` as `number` while `Uint<N>` is mapped to `bigint` elsewhere;
update the table rows for `_decimals` and `_feeBps` to use `BigInt`/`bigint`
(consistent with `_maxSupply`/`_quorum` and the README examples), and apply the
same correction to the duplicated entries at lines referenced (around 143-144).
Ensure the table consistently reflects that `Uint<N>` maps to `bigint`.
- Around line 26-43: The fenced tree block starting with "fungible-token/" in
examples/fungible-token/README.md is missing a language tag (triggers MD040);
update the opening fence from ``` to ```text so the block becomes a text-fenced
code block (i.e., change the opening triple-backticks that precede
"fungible-token/" to include the "text" tag).
In `@examples/README.md`:
- Line 16: The README currently states "Compiled artifacts ship committed..."
which conflicts with the fungible-token README that says artifacts/ is
gitignored and generated locally; make the artifact policy consistent across
both READMEs by choosing one policy and updating the sentence that references
"artifacts/" (the line currently claiming compiled artifacts are committed) to
match the chosen policy, and ensure the fungible-token README has the same
wording and instructions for setup, generation, or deployment so both docs
present the same expectations.
In `@packages/cli/src/runDeploy.ts`:
- Around line 106-112: The parser currently treats unknown single-dash tokens as
positional and allows extra positionals; update the default branch in the
argument loop in runDeploy.ts so that if arg startsWith('-') (single-dash short
flag) you throw an Error(`Unknown flag: ${arg}`) instead of pushing to
out.positional, and after parsing validate out.positional: if
out.positional.length === 0 throw a missing contract error, and if
out.positional.length > 1 throw an Error indicating extra positional args
(mention the unexpected tokens) before assigning out.contract =
out.positional[0]; reference the variables arg, out.positional, and out.contract
when locating where to change behavior.
- Around line 95-103: The --sync-timeout branch currently uses
Number.parseInt(raw, 10) which permits inputs like "10abc"; update the parsing
in the case '--sync-timeout' (around expectValue and out.syncTimeoutSec) to
reject malformed values by ensuring the raw string is an exact integer
representation before assigning out.syncTimeoutSec — e.g., parse the number and
verify String(seconds) === raw (or use a /^\d+$/ check) and then confirm
Number.isFinite(seconds) && seconds > 0, otherwise throw the existing error with
the original raw value.
In `@packages/deployer/README.md`:
- Around line 23-37: The README has unlabeled fenced code blocks causing MD040;
update the two unlabeled blocks that show the CLI usage and example (the blocks
beginning with "compact-deploy <Contract>" and the example "compact-deploy
<Contract> --network preprod \") by adding appropriate language identifiers: use
"text" for the CLI usage block and "bash" (or "sh") for the example block.
Ensure both fenced blocks are updated so the first starts with ```text and the
second with ```bash to satisfy the linter.
- Line 7: The README contains a dead anchor link `#roadmap--todo`; update the
markdown to point to the actual Roadmap heading or create a matching section:
either change the link target in the Status line from `#roadmap--todo` to the
real anchor (e.g., `#roadmap--todo` -> `#roadmap` or `#roadmap--todo` ->
`#roadmap-and-todo` depending on the existing heading text), or add a "Roadmap /
TODO" heading that generates the `#roadmap--todo` anchor so the link resolves;
ensure the visible link text ("Roadmap") and the heading slug match.
In `@packages/deployer/src/config/compact-config.test.ts`:
- Around line 48-50: The test is creating a second [profile] table which can
trigger a TOML parse error before the semantic check; change the tmpRepo content
so it does not duplicate the [profile] header but instead adds default_network =
"ghost" inside the already-existing profile from MIN_VALID (i.e. append
'default_network = "ghost"' under the existing [profile] rather than adding a
new '[profile]' line) so CompactConfig.load(...) exercises the semantic
validation for default_network missing.
In `@packages/deployer/src/deployer.ts`:
- Around line 580-583: The current pipeline uses Rx.timeout({ each: timeoutMs })
after Rx.filter((s: FacadeState) => s.isSynced) so only emissions with
s.isSynced === true will reset the timer; if you want any progress emission to
extend the timeout, move Rx.timeout before the Rx.filter so all FacadeState
emissions reset the timer, otherwise if your intent is to bound time until the
first synced emission (and not allow repeated isSynced=true to restart it)
replace the timeout call with Rx.timeout({ first: timeoutMs }); update the
pipeline around Rx.timeout and Rx.filter accordingly.
In `@packages/deployer/src/loaders/constructor-meta.ts`:
- Around line 34-46: The validation uses the `in` operator which matches
prototype properties; update the missing-parameter check in the constructor args
validation to only consider own properties by replacing `(n in named)` with an
own-property check such as `Object.hasOwn(named, n)` (or
`Object.prototype.hasOwnProperty.call(named, n)` for older runtimes), keeping
the rest of the logic (throwing ConfigError, computing extra via
Object.keys(named), and returning `paramNames.map((n) => named[n])`) unchanged.
In `@packages/deployer/src/providers/private-state-password.test.ts`:
- Around line 38-60: The test mocks node:crypto but restores it after
assertions, which won't run if the test throws; wrap the
mock/require/assert/unmock sequence in a try/finally so node:crypto is always
restored: call vi.resetModules(); vi.doMock('node:crypto', ...) then import {
derivePrivateStatePassword as derive } and run the expect inside the try block,
and in finally call vi.doUnmock('node:crypto') and vi.resetModules(); reference
derivePrivateStatePassword / derive and the vi.doMock/vi.doUnmock calls so the
mock is reliably cleaned up even when the test throws.
In `@packages/deployer/src/wallet/keystore.ts`:
- Around line 196-201: seedFromHex currently accepts any even-length hex and
should instead enforce the expected seed lengths so keystores fail fast: in
seedFromHex, after stripping the 0x prefix and validating hex characters, check
that stripped.length is either 64 or 128 (matching classifySeed expectations)
and throw a WalletError with a clear message if not; otherwise continue to
return Buffer.from(stripped, 'hex'). Reference: seedFromHex and classifySeed.
In `@tests/integrations/_harness/paths.ts`:
- Around line 20-37: The current preflight helpers requireFixtureArtifact and
requirePrivateCounterArtifact only call existsSync(ARTIFACT_DIR) /
existsSync(PRIVATE_COUNTER_ARTIFACT_DIR), which permits empty/stale directories;
update both functions to ensure the artifact directory is not only present but
contains expected compiled outputs (e.g., check readdirSync(...) yields at least
one file and/or assert presence of expected filenames/extensions such as
.wasm/.json); if the directory is empty or missing expected files, throw the
same Error with the helpful "Run `make -C tests/integrations compile` first."
message so failures surface earlier and with clearer diagnostics.
In `@tests/integrations/README.md`:
- Around line 7-25: The README's fenced code block starting with ``` lacks a
language tag (markdownlint MD040); update the opening fence from ``` to include
a language specifier (for example ```text or ```bash) so the block is properly
tagged—modify the README's fenced block that contains the directory tree (the
``` fence and its closing ``` fence) to add the chosen language tag.
---
Nitpick comments:
In `@packages/cli/test/prompt.test.ts`:
- Around line 79-149: Add two regression tests to prompt.test.ts that exercise
stdin 'end' and 'error' paths: use promptPassphrase(...) to start the promise,
trigger mockStdin.emit('end') in one test and assert the promise settles
(rejects) and raw mode is turned off and stdin.pause() called; in the other test
emit mockStdin.emit('error', new Error('boom')) and assert the promise rejects
with that error (or the propagated error) and cleanup (setRawMode(false) and
pause) occurs; locate tests near the existing describe blocks and reuse
mockStdin.emit, mockStdin.setRawMode, and mockStdin.pause checks already used in
other cases.
In `@tests/integrations/specs/deploy/deploy.spec.ts`:
- Around line 33-35: The hex assertions are too permissive; update the regexes
for result.address, result.txId and result.txHash to enforce exact lengths used
by the deployer contract (e.g., address = 40 hex chars, txId/txHash = 64 hex
chars) so truncated values fail; replace /^[0-9a-f]+$/i with length-constrained
patterns (e.g., /^[0-9a-f]{40}$/i and /^[0-9a-f]{64}$/i as appropriate) and make
the same change for the other duplicate assertions further down in the spec.
In `@tests/integrations/specs/deploy/historyIsolation.spec.ts`:
- Around line 26-38: The test currently calls wipeDeployments() in
beforeAll/afterAll and uses shared DEPLOYMENTS_DIR (via
requireFixtureArtifact()/deployFixture()), coupling specs; change the test setup
to use an isolated deployments directory per spec/worker by deriving a unique
DEPLOYMENTS_DIR (e.g. from Vitest worker id or a temp UUID) and pass it into
requireFixtureArtifact()/deployFixture() or make wipeDeployments() operate on
that per-test directory; update historyIsolation.spec.ts to set the unique
directory before calling requireFixtureArtifact(), deployFixture(), and
wipeDeployments() so each spec uses its own deployments folder and no longer
shares global state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2121ac35-31dc-4364-ad61-5db4afc670a7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (102)
.gitignoreMakefilecompact.tomlexamples/README.mdexamples/fungible-token/README.mdexamples/fungible-token/compact.tomlexamples/fungible-token/contracts/TokenExample.compactexamples/fungible-token/contracts/security/Initializable.compactexamples/fungible-token/contracts/token/FungibleToken.compactexamples/fungible-token/contracts/utils/Utils.compactexamples/fungible-token/deploy/TokenExample.args.mjsexamples/fungible-token/deploy/deployTokenExample.tsexamples/fungible-token/package.jsonpackage.jsonpackages/cli/package.jsonpackages/cli/src/logger.tspackages/cli/src/prompt.tspackages/cli/src/runBuilder.tspackages/cli/src/runCompiler.tspackages/cli/src/runDeploy.tspackages/cli/test/logger.test.tspackages/cli/test/prompt.test.tspackages/cli/test/runBuilder.test.tspackages/cli/test/runDeploy.test.tspackages/cli/vitest.config.tspackages/deployer/README.mdpackages/deployer/package.jsonpackages/deployer/src/config/compact-config.test.tspackages/deployer/src/config/compact-config.tspackages/deployer/src/config/schema.test.tspackages/deployer/src/config/schema.tspackages/deployer/src/deployer.test.tspackages/deployer/src/deployer.tspackages/deployer/src/deployments.test.tspackages/deployer/src/deployments.tspackages/deployer/src/errors.test.tspackages/deployer/src/errors.tspackages/deployer/src/index.tspackages/deployer/src/loaders/args.test.tspackages/deployer/src/loaders/args.tspackages/deployer/src/loaders/artifact.test.tspackages/deployer/src/loaders/artifact.tspackages/deployer/src/loaders/constructor-meta.test.tspackages/deployer/src/loaders/constructor-meta.tspackages/deployer/src/loaders/context.test.tspackages/deployer/src/loaders/context.tspackages/deployer/src/loaders/contract-resolve.test.tspackages/deployer/src/loaders/contract-resolve.tspackages/deployer/src/loaders/init-state.test.tspackages/deployer/src/loaders/init-state.tspackages/deployer/src/loaders/ref-resolver.test.tspackages/deployer/src/loaders/ref-resolver.tspackages/deployer/src/loaders/signing-key.test.tspackages/deployer/src/loaders/signing-key.tspackages/deployer/src/providers/build.test.tspackages/deployer/src/providers/build.tspackages/deployer/src/providers/network.test.tspackages/deployer/src/providers/network.tspackages/deployer/src/providers/private-state-password.test.tspackages/deployer/src/providers/private-state-password.tspackages/deployer/src/providers/proof-server.test.tspackages/deployer/src/providers/proof-server.tspackages/deployer/src/runDeploy.test.tspackages/deployer/src/runDeploy.tspackages/deployer/src/wallet/handler.test.tspackages/deployer/src/wallet/handler.tspackages/deployer/src/wallet/keystore.test.tspackages/deployer/src/wallet/keystore.tspackages/deployer/src/wallet/seeds.test.tspackages/deployer/src/wallet/seeds.tspackages/deployer/tsconfig.jsonpackages/deployer/vitest.config.tstests/integrations/.gitignoretests/integrations/README.mdtests/integrations/_harness/deployer.tstests/integrations/_harness/logger.tstests/integrations/_harness/network.tstests/integrations/_harness/paths.tstests/integrations/_harness/teardown.tstests/integrations/_harness/walletPool.tstests/integrations/compact.tomltests/integrations/fixtures/Counter.compacttests/integrations/fixtures/PrivateCounter.compacttests/integrations/fixtures/initstates/PrivateCounter.jsontests/integrations/fixtures/signingkeys/Counter.signingkeytests/integrations/fixtures/signingkeys/PrivateCounter.signingkeytests/integrations/fixtures/signingkeys/SecondaryCounter.signingkeytests/integrations/fixtures/witnesses/PrivateCounter.witness.tstests/integrations/local-env.ymltests/integrations/specs/deploy/asyncDisposeCleanup.spec.tstests/integrations/specs/deploy/deploy.spec.tstests/integrations/specs/deploy/dryRun.spec.tstests/integrations/specs/deploy/historyIsolation.spec.tstests/integrations/specs/deploy/historyRotation.spec.tstests/integrations/specs/deploy/privateCounter.spec.tstests/integrations/specs/deploy/proofServerAuto.spec.tstests/integrations/specs/errors/errors.spec.tstests/integrations/specs/wallet/keystorePassphrase.spec.tstests/integrations/specs/wallet/walletLifecycle.spec.tstests/integrations/specs/wallet/walletPool.spec.tstests/integrations/vitest.config.tsturbo.json
This was referenced Jun 8, 2026
0xisk
force-pushed
the
feat/compact-deployer
branch
from
June 8, 2026 11:23
ff82d7b to
04760d1
Compare
5 tasks
compact-deployer tool
0xisk
added a commit
that referenced
this pull request
Jun 8, 2026
The deployer (#86) moved to the midnight-js 4.1 / ledger-v8 / compact-runtime 0.16 stack, but the simulator and the repo-wide compiler pin were left behind on three different versions. * simulator: bump @midnight-ntwrk/compact-runtime 0.14.0 -> 0.16.0 and replace @midnight-ntwrk/ledger-v7 with ledger-v8 8.1.0. The v7->v8 API is signature-identical for every symbol the simulator uses, so the only source change is one test-fixture import path. * CI: bump the compiler pin 0.28.0 -> 0.31.0 in the setup action so the freshly compiled fixtures target runtime 0.16.0. * README: bump the compiler badge 0.29.0 -> 0.31.0. Verified locally: fixtures recompile to runtime-version 0.16.0 / compiler-version 0.31.0, and build, types, lint, and all 56 simulator tests pass. Refs: #107
0xisk
added a commit
that referenced
this pull request
Jun 8, 2026
Harden the deployer tool and CLI against the issues raised in the #86 review. Each fix verified against current code and covered by tests. * cli/logger: route `--json` pino logs to STDERR (fd 2) so STDOUT carries only the single result object. * cli/prompt: write the passphrase prompt + newline to STDERR, and settle the promise on stdin `end`/`error` so non-interactive input (closed/newline-less pipe) can no longer hang the CLI. * config/schema: mark the file/module ref union members `.strict()` so an ambiguous `{ file, module }` is rejected instead of silently dropping a key. * deployments: write the ledger atomically (temp file + rename) so a crash mid-write can't truncate `*.json`. * loaders/artifact: bind compiled assets to `dirname(entry)` rather than a hardcoded `contract/`, fixing the top-level-`index.js` case. * loaders/constructor-meta: allow an empty named-args object for a no-arg constructor (resolves to `[]`); `reorderNamedArgs` still rejects unexpected keys. * loaders/contract-resolve: use `path.isAbsolute` instead of `startsWith('/')` for OS-correct absolute-path detection. * providers/network: drop `mainnet` from the allow-list while the deployer is testnet/preview-only. * providers/proof-server: reject partial-numeric and out-of-range `PROOF_SERVER_PORT` values (full `\d+` match + 1..65535 bounds). * wallet/handler: use `path.dirname`/`basename` instead of manual `/` splitting so cache paths work on Windows. * wallet/keystore: validate shape in `fromJSON` and throw `WalletError` (not a raw `TypeError`) on malformed keystore JSON. Refs: #86
0xisk
added a commit
that referenced
this pull request
Jun 8, 2026
* integrations/walletPool: evict a wallet build from the cache if it rejects, so a failed `WalletHandler.build`/`provider.start` no longer poisons every later `signerFor(alias)` call with the same rejection. * integrations/local-env: pin proof-server and indexer-standalone by digest (resolved from `:latest`) for deterministic runs, matching the already-pinned node image. Both digests are multi-arch (amd64 + arm64) manifests and identical to the current `:latest`, so behaviour is unchanged. Refs: #86
Introduce the `@openzeppelin/compact-deployer` package: a programmatic and CLI deployer for Compact contracts. Covers config loading, artifact and constructor-arg resolution, wallet/keystore handling, network and proof-server providers, and deployment-record bookkeeping. Wire the deployer into `@openzeppelin/compact-cli` via the new `compact-deploy` command (runDeploy), with passphrase prompting and JSON/verbose output modes. * Add root resolutions for the midnight stack and a `coverage` turbo task so the new package can report coverage. Part 1 of 2 splitting the original deployer PR. Examples and the integration-test suite follow in a stacked PR that builds on this one. Refs: #86
Harden the deployer tool and CLI against the issues raised in the #86 review. Each fix verified against current code and covered by tests. * cli/logger: route `--json` pino logs to STDERR (fd 2) so STDOUT carries only the single result object. * cli/prompt: write the passphrase prompt + newline to STDERR, and settle the promise on stdin `end`/`error` so non-interactive input (closed/newline-less pipe) can no longer hang the CLI. * config/schema: mark the file/module ref union members `.strict()` so an ambiguous `{ file, module }` is rejected instead of silently dropping a key. * deployments: write the ledger atomically (temp file + rename) so a crash mid-write can't truncate `*.json`. * loaders/artifact: bind compiled assets to `dirname(entry)` rather than a hardcoded `contract/`, fixing the top-level-`index.js` case. * loaders/constructor-meta: allow an empty named-args object for a no-arg constructor (resolves to `[]`); `reorderNamedArgs` still rejects unexpected keys. * loaders/contract-resolve: use `path.isAbsolute` instead of `startsWith('/')` for OS-correct absolute-path detection. * providers/network: drop `mainnet` from the allow-list while the deployer is testnet/preview-only. * providers/proof-server: reject partial-numeric and out-of-range `PROOF_SERVER_PORT` values (full `\d+` match + 1..65535 bounds). * wallet/handler: use `path.dirname`/`basename` instead of manual `/` splitting so cache paths work on Windows. * wallet/keystore: validate shape in `fromJSON` and throw `WalletError` (not a raw `TypeError`) on malformed keystore JSON. Refs: #86
0xisk
force-pushed
the
feat/compact-deployer
branch
from
June 8, 2026 12:52
475c4ac to
15619e6
Compare
Both the wallet cache and the LevelDB store now hang off the compact.toml directory, so the project is the unit of state and a second concurrent compact-deploy in the same project fails on the LevelDB lock.
pepebndc
requested changes
Sep 4, 2026
pepebndc
left a comment
There was a problem hiding this comment.
The package is in good shape: typed error hierarchy, scrypt/AES keystore with a timing-safe MAC check, 0o600 on secret files, atomic ledger writes, seed-hashed cache filenames, and no secret reaches logs or JSON output. I built and ran it locally on Node 24 (4/4 packages, 381 deployer tests, 74 cli tests, tsc and biome clean). No security finding is above Medium.
Four blockers, all in the deploy window and the release surface. Inline comments carry the details.
Blocker: a deploy can be lost. The txId is only visible after finalization and the signing-key write, with no log before the ledger write. Any failure in that window (indexer WS drop, sync stall, LevelDB lock, 10 s file-lock timeout, corrupt <network>.json) exits with an error while the contract is live, and nothing in any output mode carries the txId or address. There is no re-run guard, so the operator deploys twice. This is issue #96, which this PR closes on merge without implementing; the code does address #115 (commit 6b2f432), so relink to that.
Blocker: compact-cli cannot be released after merge. It depends on @openzeppelin/compact-deployer: workspace:^ (packs to ^0.0.1), but release.yml and release-publish.yml list only builder, cli, simulator, and RELEASING.md's first-release order has no deployer entry.
Blocker: Node engine bump on compact-cli. >=22 to >=24 ships in the same package as compact-compiler and compact-builder. compact-contracts pins compact-cli ^0.0.3 with engines.node >=22. Needs a minor bump with a **Breaking:** changelog entry, or compact-deploy as its own bin package.
Blocker: two Midnight stacks on one branch. The deployer pins midnight-js 4.1.1 / compact-runtime 0.16.0 / ledger-v8 8.1.2 while the simulator is on 5.0.0-beta.7 / 0.19.0 / ledger-v9, and both copies install. Your README says a deploy needs one physical compact-runtime shared with the compiled contract, so the tool cannot deploy contracts built on the compact-contracts stack today. The reason (compact-js 2.5.3 depends on an unpublished ledger-v9 alpha) lives in a commit message; put the supported stack and the migration plan in the README.
High, not blocking on their own: testcontainers ^10.28.0 and axios are runtime dependencies never imported in src/ (testkit-js brings its own testcontainers 12, so two dockerode majors install). The full report with the Medium items (undocumented undici/glob/uuid resolutions forcing testkit's testcontainers down two majors, the 100% coverage gate failing at error-format.ts:41 with the new coverage task not in CI, exit-4 errors that are never thrown, String(e) rendering tagged SDK errors as [object Object], args and init-state loaded after the ~37 min sync, LevelDB provider never closed, .gitignore comment contradicting deployments.ts) is available if useful. #109 also still modifies tool files and needs a rebase on this head before it reads as examples and tests only.
| privateStateProvider: s.opts.privateStateProvider, | ||
| privateStateSecret: s.privateStateSecret, | ||
| }); | ||
| const txResult = await executeDeploy({ |
There was a problem hiding this comment.
Blocking (High): executeDeploy wraps deployContract, which submits, blocks in watchForTxData (an unbounded poll), then calls setSigningKey, and only then returns the txId. A WebSocket drop or indexer stall here leaves the spinner on "proof gen + submit" forever with no identifier logged, and any throw in the window reports DeployTxFailedError for a contract that is already on chain. Use submitTxAsync (exported by midnight-js-contracts 4.1.1) to get the txId at submit, log it at info, write a pending record under the ledger lock, wait for finalization under a bounded timeout, promote the record on success, include the txId in the error on failure, and refuse to deploy while a pending record exists unless a flag overrides it. That is the concrete gap behind #96.
Member
Author
There was a problem hiding this comment.
Agree — fixed in 4d6e31c: createUnprovenDeployTx + submitTxAsync, pending ledger record before the wait, watchForTxData under --tx-timeout (default 600 s), txId and address in every post-submit error, --force to deploy over a pending record.
| deploymentsDir: s.config.deploymentsDir, | ||
| network: s.networkName, | ||
| }); | ||
| const persisted = await deployments.record(s.opts.contract, record); |
There was a problem hiding this comment.
Blocking (High): deployments.record runs after the tx is confirmed and nothing logs the record first. A lock timeout (file-lock.ts:36, 10 s), EACCES, or a corrupt <network>.json (raw SyntaxError from readJson at atomic-json.ts:10) rejects deploy(), both front-ends exit 1, and the address, txId, and txHash never reach the user. Log the full record at info before this call, wrap readJson failures in a typed error, and rethrow a DeployError that carries the address. Add a test that injects a lock timeout and asserts the address is still surfaced.
Member
Author
There was a problem hiding this comment.
Agree — fixed in 4d6e31c: the record is logged at info before the write, readJson wraps the SyntaxError with the file path, and a failed ledger write rethrows DeploymentsFileError (exit 6) carrying address, txId and txHash. Lock-timeout test added.
| ], | ||
| "engines": { | ||
| "node": ">=22" | ||
| "node": ">=24" |
There was a problem hiding this comment.
Blocking (High): >=24 (main has >=22) applies to compact-compiler and compact-builder users too, since they ship in this package. compact-contracts pins compact-cli ^0.0.3 and declares engines.node >=22. RELEASING.md requires a **Breaking:** changelog entry and a minor bump for this, and there is no packages/cli/CHANGELOG.md. Alternative: ship compact-deploy as its own bin package and keep the cli floor at 22.
Member
Author
There was a problem hiding this comment.
Agree — recorded as Breaking in packages/cli/CHANGELOG.md in 0603ec2. The floor is real: compact-deploy uses await using / AsyncDisposableStack, which Node 22 lacks.
| }, | ||
| "dependencies": { | ||
| "@openzeppelin/compact-builder": "workspace:^", | ||
| "@openzeppelin/compact-deployer": "workspace:^", |
There was a problem hiding this comment.
Blocking (High): workspace:^ rewrites to ^0.0.1 at pack time, but release.yml and release-publish.yml only know compact-builder, compact-cli, compact-simulator. A compact-cli release after merge points at an npm version that cannot be published. Add compact-deployer to both workflows, add repository and a CHANGELOG to its package.json, and update the first-release order in RELEASING.md (deployer before cli).
Member
Author
There was a problem hiding this comment.
Agree — fixed in 0603ec2: both workflows know compact-deployer, repository field added, CHANGELOG.md for cli and deployer (and in their files arrays), RELEASING.md order is builder, simulator, deployer, cli.
| "@midnight-ntwrk/compact-js": "2.5.1", | ||
| "@midnight-ntwrk/compact-runtime": "0.16.0", | ||
| "@midnight-ntwrk/ledger-v8": "8.1.2", | ||
| "@midnight-ntwrk/midnight-js-contracts": "4.1.1", |
There was a problem hiding this comment.
Blocking (High): midnight-js 4.1.1 and compact-runtime 0.16.0 here, versus 5.0.0-beta.7 and 0.19.0 in packages/simulator/package.json on this branch; both copies install (node_modules/@midnight-ntwrk/compact-runtime 0.16.0 and packages/simulator/node_modules/.../compact-runtime 0.19.0). README line 34 says a deploy needs one physical compact-runtime shared with the compiled contract, so contracts built on the compact-contracts stack (0.19.0, ledger-v9) cannot be deployed with this tool. Commit 360d7cf explains why (compact-js 2.5.3 depends on an unpublished ledger-v9 alpha); put the supported stack and the migration plan in the deployer README and root README so consumers see it.
Member
Author
There was a problem hiding this comment.
Agree — documented in f2304b2: a Supported stack section in the deployer README (pins, compact compile +0.31.1, the version-mismatch symptom, the compact-contracts main incompatibility and the ledger-v9 blocker) plus a deployer entry in the root README.
| "pino": "^9.7.0", | ||
| "rxjs": "^7.8.1", | ||
| "smol-toml": "^1.3.4", | ||
| "testcontainers": "^10.28.0", |
There was a problem hiding this comment.
High: testcontainers (and axios) are never imported in packages/deployer/src. The only container use goes through @midnight-ntwrk/testkit-js, which requires ^12 and installs its own nested 12.1.0, so this line adds an unused second copy with a second dockerode major. Remove both.
Neither is imported anywhere under packages/deployer/src. The only container use goes through @midnight-ntwrk/testkit-js, which brings its own testcontainers 12; the direct ^10 pin added a second copy with a second dockerode major.
compact-cli depends on @openzeppelin/compact-deployer via workspace:^, which yarn rewrites to a concrete version at pack time, but neither release workflow knew the package, so a cli release after merge would point at an npm version nothing can publish. The deployer joins the workflow choice lists, gets its repository field, and moves ahead of the cli in the first-release order. RELEASING.md requires a CHANGELOG.md per published package. Both new files start with an Unreleased section; the cli entry records the Node 24 floor as Breaking, since compact-deploy relies on explicit resource management (await using, AsyncDisposableStack) that Node 22 lacks and the bump lands on compact-compiler and compact-builder users too. The changelogs are added to the files arrays so they ship in the tarballs.
The deployer pins compact-runtime 0.16.0 and ledger-v8, and needs artifacts from compactc 0.31.1; the default 0.34 compiler targets runtime 0.19.0 and the deploy fails with a version mismatch. The README now says so up front, notes that compact-contracts main (runtime 0.19.0, ledger-v9) is not deployable with this tool yet, and names the blocker: compact-js 2.5.3 depends on an unpublished ledger-v9 alpha. The root README gains the deployer package and corrects the Node requirements per package.
The default LevelDB store is opened per deploy and holds its lock until the process exits, so a script that deploys several contracts in one process needs privateStateProvider next to walletProvider.
--config and --seed-cache-from-* already resolve against the shell CWD; --seed-file alone resolved against the compact.toml directory, so a relative path typed from a subdirectory read the wrong file. A relative [wallet].keystore keeps the rootDir anchor because it comes from the config file, not the shell.
The schema bounded n and r separately, but scryptSync also enforces a memory budget and an n < 2^(16r) ceiling, so a keystore with n=2^20 r=8 passed validation and then surfaced as a raw OpenSSL RangeError. One predicate now models both limits and runs in the schema and in encrypt. fromJSON also lost its hand-written pre-checks: they restated what the zod schema already enforces, and the schema error names the field.
A bare Error exited 1; errors.ts promises a stable code per failure mode, and a sync timeout is a wallet failure (exit 3).
Two waiters that both saw a stale lock could each unlink it: the first re-created the lock, the second removed that fresh file and acquired alongside the holder. The break now renames the lock aside (atomic, one winner), re-checks the parked file's age, and restores it if a waiter had already taken over. The lock timeout is a DeployError instead of a bare Error.
applyNetwork rejects mainnet, so the additionalFeeOverhead fallback to the testkit default could never run.
Both depend only on the config and the artifact, and a typo in either used to surface after a 30-60 min first preprod sync.
The library entrypoint still rendered a wallet-SDK tagged record as "[object Object]"; 298def0 fixed the CLI arm only.
deployContract fuses submission and the wait for finalization and yields no identifier until the tx has landed, so a WebSocket drop or indexer stall left the CLI spinning with nothing to reconcile, and any failure in that window reported a deploy that may already be on chain as failed. The three SDK steps now run explicitly: createUnprovenDeployTx, submitTxAsync, then watchForTxData raced against --tx-timeout (default 600 s), with the private-state and signing-key writes only after a SucceedEntirely status. The deployments ledger gains a pending state. A record with the address and txId is written as soon as the node accepts the tx and promoted to confirmed on finalization. A timeout or rejection leaves it pending and names both identifiers in the error; the next deploy of that contract refuses until --force, and the check runs before proving so a blocked deploy costs no fees. Every ledger write is logged at info first and a failing write (lock timeout, EACCES, malformed <network>.json) rethrows as DeploymentsFileError, exit 6, carrying address, txId and txHash. readJson wraps a SyntaxError with the file path instead of leaking it. IndexerUnreachableError and ProofServerUnreachableError had no call site and are gone; exit code 4 is unused.
Node 24 ships a native global WebSocket and midnight-js reads that global rather than importing ws. Verified on a local node: with the native class pinned, sync, deploy and watchForTxData all completed; with WebSocket forced to undefined the sync never connected. The shim and the ws / @types/ws dependencies go; ws stays in the lockfile only as a transitive dependency of the indexer provider and testkit-js.
testkit-js builds a pino logger at import and pretty-prints through it to stdout, ignoring the logger passed into every call, so `compact-deploy --json | jq` failed on an "Initializing wallet builder" line that landed before the result object. The logger is exported; setting its level to silent at module load stops the line. The empty `<cwd>/logs/tests/` file the same logger creates on import cannot be prevented from user code and needs an upstream change.
With the proof server down the CLI printed only "Failed to prove transaction". The wallet SDK does attach the connection error as the cause, but Effect.runPromise rejects with a FiberFailure that copies the message and leaves `cause` unset; the real failure hangs off the registered symbol effect/Runtime/FiberFailure/Cause. formatError now unwraps that symbol (no runtime dependency on effect) and collapses a wrapper whose cause repeats its message, so the line reads "Failed to prove transaction: Failed to connect to Proof Server: Transport error (POST http://127.0.0.1:6300/prove): fetch failed".
0xisk
added a commit
that referenced
this pull request
Sep 7, 2026
Add the fungible-token deploy walkthrough under examples/ and the end-to-end integration suite under tests/integrations/, both built on the compact-deployer tool. * examples/fungible-token: a TokenExample contract wrapping the OZ Compact FungibleToken module, with deploy scripts exercising the common constructor-arg types and `compact-deploy` CLI invocations. * tests/integrations: vitest specs covering deploy flows, dry-run, history isolation/rotation, wallet lifecycle, keystore and error paths, plus a docker local-stack harness driven by the top-level Makefile. * Re-add the `examples/*` workspace and the integration/env make shortcuts to the root package.json, and the root compact.toml used for real-network deploys. Both source trees are compiled with `compact compile +0.31.1`, the compiler that matches the deployer's pinned compact-runtime 0.16.0. An artifact from the default compactc fails at submit with a version mismatch, so the version is a Makefile variable rather than implicit. The root `types` script now also runs tsc over tests/integrations. That tree is not a workspace package, so nothing else would type-check it, and vitest only strips its types. The two specs that need `proof_server = "auto"` are skipped: testkit-js boots that container from a `proof-server.yml` in the process CWD, which this repo does not ship. Part 2 of 2 splitting the original deployer PR. Refs: #86
0xisk
added a commit
that referenced
this pull request
Sep 7, 2026
* integrations/walletPool: evict a wallet build from the cache if it rejects, so a failed `WalletHandler.build`/`provider.start` no longer poisons every later `signerFor(alias)` call with the same rejection. * integrations/local-env: pin proof-server and indexer-standalone to release tags (8.0.3 and 4.0.1) rather than `:latest`, so the stack matches the ledger-v8 versions the deployer supports and does not move under the suite. Drop the proof-server container healthcheck too: that image is distroless, so no probe can run in it and the container never reports healthy. `make env-up` polls /version from the host instead. Refs: #86
0xisk
added a commit
that referenced
this pull request
Sep 7, 2026
Add the fungible-token deploy walkthrough under examples/ and the end-to-end integration suite under tests/integrations/, both built on the compact-deployer tool. * examples/fungible-token: a TokenExample contract wrapping the OZ Compact FungibleToken module, with deploy scripts exercising the common constructor-arg types and `compact-deploy` CLI invocations. * tests/integrations: vitest specs covering deploy flows, dry-run, history isolation/rotation, wallet lifecycle, keystore and error paths, plus a docker local-stack harness driven by the top-level Makefile. * Re-add the `examples/*` workspace and the integration/env make shortcuts to the root package.json, and the root compact.toml used for real-network deploys. Both source trees are compiled with `compact compile +0.31.1`, the compiler that matches the deployer's pinned compact-runtime 0.16.0. An artifact from the default compactc fails at submit with a version mismatch, so the version is a Makefile variable rather than implicit. The root `types` script now also runs tsc over tests/integrations. That tree is not a workspace package, so nothing else would type-check it, and vitest only strips its types. The two specs that need `proof_server = "auto"` are skipped: testkit-js boots that container from a `proof-server.yml` in the process CWD, which this repo does not ship. Part 2 of 2 splitting the original deployer PR. Refs: #86
0xisk
added a commit
that referenced
this pull request
Sep 7, 2026
* integrations/walletPool: evict a wallet build from the cache if it rejects, so a failed `WalletHandler.build`/`provider.start` no longer poisons every later `signerFor(alias)` call with the same rejection. * integrations/local-env: pin proof-server and indexer-standalone to release tags (8.0.3 and 4.0.1) rather than `:latest`, so the stack matches the ledger-v8 versions the deployer supports and does not move under the suite. Drop the proof-server container healthcheck too: that image is distroless, so no probe can run in it and the container never reports healthy. `make env-up` polls /version from the host instead. Refs: #86
0xisk
added a commit
that referenced
this pull request
Sep 8, 2026
* test(deployer): add examples and integration-test suite Add the fungible-token deploy walkthrough under examples/ and the end-to-end integration suite under tests/integrations/, both built on the compact-deployer tool. * examples/fungible-token: a TokenExample contract wrapping the OZ Compact FungibleToken module, with deploy scripts exercising the common constructor-arg types and `compact-deploy` CLI invocations. * tests/integrations: vitest specs covering deploy flows, dry-run, history isolation/rotation, wallet lifecycle, keystore and error paths, plus a docker local-stack harness driven by the top-level Makefile. * Re-add the `examples/*` workspace and the integration/env make shortcuts to the root package.json, and the root compact.toml used for real-network deploys. Both source trees are compiled with `compact compile +0.31.1`, the compiler that matches the deployer's pinned compact-runtime 0.16.0. An artifact from the default compactc fails at submit with a version mismatch, so the version is a Makefile variable rather than implicit. The root `types` script now also runs tsc over tests/integrations. That tree is not a workspace package, so nothing else would type-check it, and vitest only strips its types. The two specs that need `proof_server = "auto"` are skipped: testkit-js boots that container from a `proof-server.yml` in the process CWD, which this repo does not ship. Part 2 of 2 splitting the original deployer PR. Refs: #86 * fix(tests): address CodeRabbit review feedback * integrations/walletPool: evict a wallet build from the cache if it rejects, so a failed `WalletHandler.build`/`provider.start` no longer poisons every later `signerFor(alias)` call with the same rejection. * integrations/local-env: pin proof-server and indexer-standalone to release tags (8.0.3 and 4.0.1) rather than `:latest`, so the stack matches the ledger-v8 versions the deployer supports and does not move under the suite. Drop the proof-server container healthcheck too: that image is distroless, so no probe can run in it and the container never reports healthy. `make env-up` polls /version from the host instead. Refs: #86 * fix(examples): point testnet proof_server at localhost "auto" boots a testkit container from a proof-server.yml in the working directory; nothing in this repo ships one, so the preview and preprod blocks could never resolve a proof server. * test(integrations): assert the seeded private state The PrivateCounter spec inferred both deploy-pipeline paths from a green result. It now owns the private-state provider and reads the seed back, and asserts whole shapes instead of probing fields. * docs(examples): name the vendored compact-contracts commit The three vendored modules claimed v0.0.1-alpha.1; they are byte-identical to compact-contracts 19b36a74. The header now names the commit so a refresh has a diff base, and TokenExample drops the unused Maybe export. * docs(examples): link the CLI flag list, drop sync timing The preview timing contradicted the null-route callout two lines below, and the hand-copied flag list was a second source of truth for the deployer README's. * refactor(integrations): one source for stack + seeds The MIDNIGHT_* overrides could point pool wallets at a different stack than the one compact.toml deploys against, and the seed table restated the deployer's LOCAL_PREFUNDED_SEEDS. Aliases now index into that export, and getSharedPool() drops the env argument it ignored after the first call. * test(integrations): assert error text, keep the pool warm walletPool.spec reset the shared pool in afterAll, forcing every later spec to re-sync its wallets; it now owns a pool. The error specs promised messages naming the offending key and asserted only the class. * build: make types and test-integration self-sufficient Both needed packages/deployer/dist to exist already. `types` now runs the build task first and `test-integration` takes build as a prerequisite. The Ctrl+C teardown ran twice because the EXIT handler fires after the INT one; trap EXIT alone. @tsconfig/node24 pinned exact like the other root dev deps. * chore: drop the root compact.toml Nothing consumed it: the integration suite and the example each carry their own config. Its `signing_key_file` pointed at a gitignored path that does not exist, and because `CompactConfig.load()` walks upward it silently became the config for any bare `compact-deploy` run inside `packages/*`. * test(integrations): un-skip the proof-server specs The `auto` path works now that the deployer ships its compose file, so both specs run against a real container instead of being skipped. Each asserts that no `proof-server_*` container is left running: the containers take a random host port, so a leak collides with nothing and a later deploy succeeding proves nothing about cleanup on its own. That assertion is also why the async-dispose spec no longer redeploys. As the suite's first transaction on a fresh chain, that deploy failed on dust the genesis wallet had not accrued yet. The example's testnet blocks go back to `"auto"`, so nothing there needs a proof server started by hand. * docs(integrations): correct the proof-server healthcheck note The image has a shell; it only lacks curl. Say that instead of calling it distroless. * docs(examples): say the vendored modules carry local headers * build: recompile fixtures when the compiler pin changes The artifact targets only depended on their .compact sources, so a COMPACTC_VERSION bump kept artifacts from the previous compiler. * test(integrations): assert prepare leaves an injected wallet unstarted * fix(integrations): await the deploy inside the await-using scope Returning the bare promise let Deployer's asyncDispose run before the deploy settled, tearing down the wallet and any auto proof-server container while proving was still in flight. * fix(integrations): publish the local stack on loopback only Every consumer targets 127.0.0.1, so binding on 0.0.0.0 only exposed the dev node and the insecure-credential indexer to the LAN. * build: make clean drop fixture artifacts and logs
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Part 1 of 2 (originally one large PR, now split for reviewability).
Introduces the
@openzeppelin/compact-deployerpackage: a programmatic and CLI deployer for Compact contracts.packages/deployer/— config loading (compact.toml+ schema), artifact and constructor-arg resolution, init-state and signing-key loaders, wallet/keystore handling, network + proof-server + build providers, deployment-record bookkeeping, and therunDeployentry point.packages/cli/— wires the deployer intocompact-clias thecompact-deploycommand, with passphrase prompting and JSON/verbose output.resolutionsand acoverageturbo task.Why split
The original PR was 103 files / +16.7k. This half is the tool; the consumers (examples + integration tests) live in the stacked PR below.
Stacked PR
➡️ #109 — examples + integration-test suite, based on this branch. Merge this first.
Verification
Build, types, and lint pass.
compact-deployer247 unit tests andcompact-cli67 unit tests green.Summary by CodeRabbit
compact-deploycommand and programmatic deployment APIs.compact.toml.