OpenZeppelin / OpenZeppelin/compact-contracts

dev: live-harness follow-ups (node-rejection constants, orchestrator tests, env-down pkill)

Open
#716 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

2-low CI enhancement
Dominant language
TypeScript
Stars
55
Forks
29
Avg merge
5d 7h
Merged PRs (30d)
25

Description

Follow-ups found while building the integration-live harness (branch
test/integration-live-harness). None of these block that branch; they were
deliberately left out to keep it scoped. Design notes live in
docs/design/integration-live-harness*.md.

Filed from the Codebase improvement template.

1. Centralize the node-rejection strings

Node error strings are hardcoded in 8 files, in three different roles:

Role Where
Live assertion contracts/test/integration/specs/confidentialFungibleToken.spec.ts (exact phrase)
Live assertion contracts/src/token/test/ConfidentialFungibleToken.test.ts:1188 (loose /block limits|exhaust the block/i)
Retry logic contracts/test-utils/harness/LiveSimulatorBackend.ts:170DETERMINISTIC_REJECTION = /1010: Invalid Transaction/
Prose only 6 doc comments in live.globalSetup.ts, scripts/live/LiveOrchestrator.ts, test-utils/fixtures/nativeShieldedToken.ts

The retry logic and the canary now encode different notions of the same
rejection: the canary demands the full phrase, isDeterministicRejection matches
only the 1010: Invalid Transaction prefix. That is how these drift apart.

Proposed contracts/test-utils/harness/nodeRejections.ts:

/** Verified against ledger-v8 8.1.0 / midnight-js 4.1.1. These are the NODE's
 *  words, not ours — a ledger bump can reword them. */
export const VERIFIED_AGAINST = { '@midnight-ntwrk/ledger-v8': '8.1.0' } as const;

export const RPC_INVALID_TRANSACTION = '1010: Invalid Transaction';

export const NODE_REJECTION = {
  /** Tx IR exceeds the per-tx block byte budget. */
  blockLimitExceeded: `${RPC_INVALID_TRANSACTION}: Transaction would exhaust the block limits`,
  /** Coin spent against stale node state (re-spend / stale UTXO). */
  staleCoinState: `${RPC_INVALID_TRANSACTION}: Custom error: 103`,
} as const;

On tying this to the ledger version: a Record<ledgerVersion, ...> map would be
speculative, since only one ledger version is pinned at a time and the unused
branches would rot. Prefer a VERIFIED_AGAINST constant plus a harness unit test
asserting the installed version still matches, so a ledger bump fails loudly and
forces someone to re-verify the strings rather than discovering it later as a
mystery red canary.

2. errorChainText — the shallow-walk bug

Both canaries collect the error text from message, one level of cause, and
String(error). The node's 1010 text can also sit:

  • more than one level deep in a cause chain,
  • in AggregateError.errors,
  • only in toString(), for an effect FiberFailure that keeps its cause behind a
    Symbol.

isDeterministicRejection already walks all of these, and
test-utils/harness/test/LiveSimulatorBackend.test.ts:177-199 documents the
two-level and FiberFailure shapes explicitly. The shallow version would miss
those and fail on the wrong assertion.

It works today for the block-limit rejection (verified across three live runs), so
this is latent rather than broken. Fix it once, in the shared module, and have both
canaries plus isDeterministicRejection use it.

3. Tighten the base spec's matcher

contracts/src/token/test/ConfidentialFungibleToken.test.ts:1188 still uses
/block limits|exhaust the block/i. A broad matcher can be satisfied by an
unrelated deploy failure (funding, proving, a submission bounce) and report a false
green, which silently voids the scope claim the canary exists to prove. The
integration canary was tightened to the exact phrase; this one should match.

4. make env-down kills its own recipe shell

Every env-up and env-down prints:

make: [Makefile:21: env-down] Terminated (ignored)

Makefile:21 is @-pkill -f "docker compose -f $(COMPOSE_FILE) logs". pkill
excludes its own PID but not its parent — and the recipe's sh -c command line
contains the pattern, so pkill matches and kills the shell running it. The @-
prefix hides the failure.

Effect is mild: the line meant to kill the backgrounded docker compose logs -f
streamers is unreliable, but the following docker compose down stops the
containers, so the streamers exit anyway. Still, the line does not do what it was
written to do.

Fix is the standard bracket trick, so the pattern cannot match the shell that
contains it:

@-pkill -f "docker compose -f $(COMPOSE_FILE) [l]ogs" 2>/dev/null || true

5. Missing tests for the live orchestrator

The scripts vitest project added on the harness branch covers resolvePlan,
classify, and report naming. Still untested:

  • assertSoleLiveProject (live.globalSetup.ts) — belongs to the harness
    project, not scripts, so the new project does not reach it. Already exported as
    a pure function for this purpose.
  • RunLock staleness — inject pidAlive the way lockHolderState already does.
  • The teardown policy — exactly-once, opt-out honoured, and teardown ordered before
    the lock release.

6. No CI job runs the scripts project

It is green locally in ~200 ms and gates the orchestrator's only unit coverage.

7. Confirm CI gating parity for the integration-live job

scripts/test-live.ts --list now emits integration, and the workflow on
ci/live-test-workflow (draft #681) feeds --list into
fromJSON(needs.plan.outputs.categories), so the job is spawned automatically.
Unconfirmed: whether that job inherits the same node-up / 6-hour-budget /
live-tests-label gating as the per-category unit jobs.

8. Compile integration artifacts into their own directory

Split out into its own issue: #758. Raised by @andrew-fleming on #717.

Noted separately — not part of this issue

The dry integration project is red on main as installed: 5 failures in
contracts/test/integration/specs/initStateIsolation.spec.ts, because
test/integration/fixtures/* are written against an older
@openzeppelin/compact-simulator API. The installed 0.2.0 constructor takes
(deps), so new ComposedTokensSimulator(args, {}) hits deps.backend.kind on
undefined. Verified by running the suite with all harness-branch changes stashed.
This may already be tracked; if not it deserves its own issue, since fixing it
means updating three fixtures rather than the harness.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with docs/design/integration-live-harness*.md, then inspect the shared harness code, LiveSimulatorBackend tests, ConfidentialFungibleToken.test.ts, Makefile, and the scripts and harness Vitest configurations. Trace the existing orchestrator and CI workflow before changing anything. Done means the rejection handling, orchestrator coverage, env-down behavior, CI coverage and gating parity are verified, with the separately tracked artifact work excluded.

Written by the indexing model from the issue text.

Assessment

Tech stack
docker-compose, typescript
Domain
build-system, ci-cd, devops, testing-qa
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.