feat(midnight-js): add ledger-8 engine core with envelope, down-convert and instance guards - #1164
feat(midnight-js): add ledger-8 engine core with envelope, down-convert and instance guards#1164sp-io wants to merge 44 commits into
Conversation
Needed by the OQ9 hard-fork fixture generators (task 0.2) to mint and verify contract-state fixtures across the v8/v9 protocol boundary.
Add testkit-js/testkit-js/src/fixtures/hf/: nine named contract-state hex fixtures spanning the ledger v8/v9 protocol boundary, a v9-era compiled twin of the spike's counter contract, generator scripts to mint/derive the fixtures, and a README documenting provenance and the mint-path decisions per fixture. Two fixtures (state-v8-v6-envelope.hex, state-migrated-v9.hex) are golden copies ported verbatim from spike-dapp-hf/island-3 (identical across islands 1-3 and ts-downcast/); the rest are minted or derived from those goldens via @midnightntwrk/ledger-v8 and ledger-v9's public APIs, with every deserialize outcome verified against the real packages before being documented.
Asserts every fixture exists and parses as hex, the v8/v9 goldens and the synthetic merkle fixture deserialize with their intended ledger, and every tampered/foreign fixture is rejected by both ledger-v8 and ledger-v9.
Adds a narrow no-restricted-imports exception for the hard-fork fixture smoke test (it must verify both raw ledger-v8 and ledger-v9 directly, which the protocol package's ledger re-export cannot do since it only re-exports v9), and declares the Node globals the .mjs fixture generator scripts use (they are plain scripts, not covered by the existing testkit-js *.ts lint block).
state-co-v2-only-foreign.hex was a bare ContractOperation, which fails at the envelope-tag check before reaching the mis-dispatch path it was meant to test. Now a well-formed ContractState (deserializes cleanly on ledger-v9) whose increment slot carries the golden migrated state's real, foreign post operation, so the failure only surfaces at execution.
Adds explicit protocolVersion assertions per fixture (incl. the null on state-both-keys.hex) and strict disk/manifest set equality, so drift between fixtures.json and the files is caught. Also updates the state-co-v2-only-foreign.hex assertions for its reworked shape: it must now deserialize cleanly on ledger-v9 and expose a foreign key, not throw.
… (D11) Adds the first ledger-8 engine modules to packages/protocol: version-aware contract-state envelope extraction (v8/v6 and migrated-v9) and the v9->v8 state down-convert with contract-agnostic bounded-Merkle-tree rehashing, productionizing the HF spike's downcast/rehash algorithm against @midnight-ntwrk/onchain-runtime-v3 directly. Adds DownConvertFailedError and MerkleNotRehashedError to errors.ts for the previously-registered DOWN_CONVERT_FAILED / MERKLE_NOT_REHASHED codes. The new engine modules stay internal (not wired into index.ts/v8.ts); the root entry point remains v9-static.
The array/map "rehash recursion" tests routed nested trees through an encode()/decode() round trip before calling downConvertForExecution, on the mistaken premise that the round trip strips a tree's cached hashes. On the pinned onchain-runtime-v3/ledger-v9 versions it doesn't: encode() already materializes node hashes regardless of prior rehash state, so checkRoot passed even with a broken (non-recursing) array/map case in rehashStateValue. Exports rehashStateValue for direct testing and rewrites both tests to assert a genuine before/after transition (checkRoot throws pre-call, passes post-call) against trees that have never been serialized, so a broken recursion is caught regardless of runtime-version hash-materialization behaviour. Verified via mutation: reverting the array/map cases to a passthrough fails exactly these two tests.
Add assertSharedLedger8Instances (constructor-reference-equality probes on the 0.16 runtime and ledger-v9 axes) and assertLedger8RuntimePresent (probes the retained v8 stack resolves before any fetch/proving starts) to the engine module, plus Ledger8InstanceMismatchError. Dual-instance negatives use aliased devDependencies (onchain-runtime-v3-alt, ledger-v9-alt) that are real second physical copies at the pinned versions. Adds per-glob coverage thresholds for version.ts and the engine files.
… gaps Reject a nullish value on either side of each dual-instantiation axis in assertSharedLedger8Instances before the reference-equality check, closing a vacuous-pass on undefined/null probes. Also drops a stale workspace-file reference from a test comment, covers the documented wrong-version-envelope DownConvertFailedError behavior with the purpose-built fixtures, and adds the missing construction-contract test for Ledger8InstanceMismatchError.
|
…olchain version CompactRuntime016 -> Ledger8CompactRuntime (+StateValue), runtime016 -> ledger8Runtime. The retained toolchain versions (compact-runtime 0.16.x, onchain-runtime-v3) can bump within the ledger-8 era; the era tag is the stable key, matching the existing Ledger8* error family. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
E2E Tests Resultse2e_tests_reports: Run #4902
🎉 All tests passed!Suites140 passed, 0 failed, and 5 other
Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
# Conflicts: # packages/protocol/src/test/protocol-acl.test.ts
… preflight loadLedger8 already wraps every acquisition failure in Ledger8RuntimeMissingError, so a separate preflight assert adds no guarantee a caller could not get from awaiting loadLedger8 directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e onchain-runtime-v3 axis The 4-argument assertSharedLedger8Instances compared the ledger-v9 axis against the same static import on both sides, so that comparison could never fail; only the retained 0.16 runtime axis has two genuine acquisition paths (this package's own dependency vs the copy the 0.16 glue resolves). The guard is now assertSharedLedger8Instance(axis, expected, actual), Ledger8InstanceAxis names only the axis production code asserts, and the now-unused ledger-v9-alt test alias is dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… rebuilding the state downConvertForExecution rebuilt the whole decoded StateValue tree (rehashing every bounded Merkle tree) on every call, even though the encode/decode round trip every input crosses already materializes the hashes on the pinned versions — a full deep copy per execution prep with no effect. The walk is now read-only: assertMerkleTreesRehashed checks each tree's root via checkRoot and throws MerkleNotRehashedError on a rootless tree, surfacing an upstream programming error loudly instead of silently repairing it. Ledger8CompactRuntimeStateValue shrinks to the decode seam actually used. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # packages/protocol/src/test/errors.test.ts
Extends the code-based `Symbol.hasInstance` to the three classes this PR adds (instance mismatch, down-convert failure, merkle not rehashed) and gives each a recognition row, so `instanceof` still holds for an error raised by a second physical copy of the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The structural check on a decoded state only caught a wholesale collapse to null, so a shortened array, a dropped map entry or a substituted subtree passed through as a valid execution state. It now compares a re-encode of the decoded value against its source, which catches every loss mode at every depth. The Merkle walk and ChargedState construction moved inside the coded-error handling, so a dual-instantiation or a walk failure can no longer escape as a bare WASM TypeError past a seam whose whole contract is code-based discrimination. envelope.ts no longer imports onchain-runtime-v3 as a value: the pre-fork ContractState is injected the way down-convert.ts already injects its runtime, so no bundle reaching these modules statically links the retained pre-fork WASM. Its version ternary became a Record keyed by LedgerVersion, so a future era added to LEDGER_VERSIONS fails to compile here instead of silently decoding with the pre-fork codec. assertMerkleTreesRehashed now handles every StateValue variant explicitly and is backed by a compile-time exhaustiveness guard in the style of version.ts, so a vendor bump that adds a container variant cannot silently skip the Merkle trees nested inside it. Error corrections: Ledger8RuntimeMissingError no longer names compact-runtime@0.16.0, which this repo does not install and never did; Ledger8InstanceMismatchError no longer claims a dual-instantiation corrupts results silently, because wasm-bindgen throws on every cross-copy handoff, and its yarn why hint now names the real package instead of a placeholder; DownConvertFailedError.stage is a closed union, which keeps the never-render- state-contents guarantee a property of the class rather than of every caller. The down-convert stage no longer claims a v9 source it cannot know.
The hard-fork goldens belong to #1184, which lands them on main together with a typed accessor. Carrying a copy here duplicated 30 files that PR already owns, inherited from the closed and unmerged #1159. The engine suite now serializes its own contract-state envelopes through the two runtimes it bridges, so it depends on neither the fixtures nor testkit-js. That dependency was not available anyway: testkit-js depends on midnight-js-protocol, so consuming its fixture accessor from here would close a workspace cycle. Coverage stays 100/100/100/100. What the goldens prove and this cannot - that a real migrated on-chain state down-converts byte-identically to its pre-migration form, and that the tampered envelopes fail closed - moves to its own PR alongside them. The protocol turbo.json goes with it, since nothing here reads a fixture path any more, as does the correction to the mint-migrated-v9-merkle generator comment.
The engine suite in #1164 builds its envelopes in-process, so it proves the bridge is correct but not that it is correct on real data. These tests close that gap: a genuinely migrated on-chain state must down-convert to data byte-identical with its pre-migration form, the pre-fork tag-v6 envelope must read to the same state as the post-fork one, a golden Merkle state must come out with a readable root, and every tampered golden must fail closed without leaking bytes. Fixtures are read by path rather than through testkit-js's typed accessor because testkit-js depends on midnight-js-protocol; a devDependency back would close a workspace cycle. The protocol turbo.json therefore declares the fixture directory as a test input, so editing a golden invalidates this package's test cache instead of replaying a stale pass. Verified locally against the fixtures from #1184: 7/7 pass. CI stays red here until both #1164 and #1184 have landed.
…tch hint The dual-publish rewrites dependency scopes at pack time, so the two published copies of midnight-js-protocol depend on two differently named copies of onchain-runtime-v3 at the same version -- an install combination no resolver can dedupe, and therefore a live cause of the very dual-instantiation that Ledger8InstanceMismatchError reports. rewriteScope touches package.json only, never a string in compiled code, so the single hardcoded package name sent every consumer installed from the @midnightntwrk scope to `yarn why` on a package absent from their tree. AXIS_PACKAGE_NAMES now carries every published name per axis and the message offers a `yarn why` for each. The errors.ts assertion is two-directional: it extracts all yarn why targets and compares the exact pair, so a missing name and a leaked extra name both fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ger agnostic packages/protocol is published, and consumers install with npm, pnpm or bun as well as yarn, so a `yarn why` hint is unrunnable for most of them. Name the packages once and every `why` equivalent once, avoiding a names-by-tools cross-product. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ifier rewriteScope renames this package in package.json only, never in a compiled string, so the subpath alone identifies the import under either scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…kflow The pre-push hook already runs yarn typecheck:tests; keep the compile-time drift detector comment in sync with that.
…-core Folds PR #1186 in: asserts the down-convert engine against the committed hard-fork goldens, which the in-process engine suite cannot cover.
…e publish rewrite The public-npm publish (.github/scripts/publish-public-npm.mjs) rewrites the old npm scope to the new one inside built .js/.d.ts files, not only in package.json. AXIS_PACKAGE_NAMES held both scoped names as literals, so the published artifact named one scope twice instead of naming both -- and no test could see it, since the suite runs against unrewritten source. Hold the scopes apart from the '/' and join them at use, so the rewrite has nothing to match, and pin that with a test asserting the module carries no scoped literal at all. Also narrow the instance-guard contract to a shared class binding: a re-export produces a fresh namespace object over one physical copy, so comparing namespaces would report a mismatch on a healthy install -- the very case the error message names first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…raction ENVELOPE_DECODERS was a plain object literal indexed by `version`, so an unexpected key resolved through Object.prototype instead of failing: 'constructor' handed the caller's own raw bytes back as an EncodedStateValue and 'toString' returned the string '[object Object]', neither throwing. Both are silent wrong answers from a function documented to fail closed, and both are reachable from the untyped JavaScript consumers this published package also serves. Build the table on a null prototype, freeze it, and validate `version` before dispatch with a new coded UnknownLedgerVersionError. Validating first also keeps `stage` inside its closed union -- it is derived from `version`, so an unvalidated string would otherwise reach a field whose contract is that consumers can switch on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a fallible root() Two silent-skip paths in the down-convert guards: The Merkle walk's switch had no default arm and returns void, so a variant outside the five it names fell through having checked nothing and recursed into nothing -- every tree nested inside such a container was skipped without a word. The compile-time exhaustiveness guard cannot cover this: the StateValue comes from a caller-injected runtime whose WASM can emit a tag the pinned .d.ts does not declare, and these declarations are already known to be unfaithful. Fold the guard into the switch as a `never` default that also throws at runtime. checkRoot treated only `undefined` as not-rehashed, but the wasm-bindgen shim for root() rethrows a Rust Err rather than always resolving to a value. That throw escaped uncoded and was relabelled DOWN_CONVERT_FAILED, telling the caller to check its envelope bytes when the fix is to rehash the tree. Treat a throw and a null alike, carrying the cause. structurallyEqual compared key counts rather than key sets, so two objects sharing no key compared equal whenever the diverging key's value was undefined. Unreachable in today's algebra; guarded so it stays that way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every container test built both sides of the re-encode comparison with onchain-runtime-v3, so it could not fail for ordering reasons; the only genuinely cross-codec case used a bare cell. The production comparison is a ledger-v9 encoding against an onchain-runtime-v3 re-encoding, and the map iteration order that makes it exact is a cross-runtime property. Build the v9 side natively over a multi-entry map inserted out of key order and over a nested array. Also cover the v8 decoder's truncated and trailing-bytes paths (only v9 had them), assert the v9 extraction stage rather than v8 alone, and pin the Map order-sensitivity that a get()-based rewrite would silently drop. Correct two comments that claimed more than they delivered: the dist-laziness onchain-runtime-v3 case passes today because no build entry reaches the engine at all, and the vendor drift detectors are checked by the pre-push hook only, never by CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every other vendor runtime the protocol package depends on is pinned there; this one was not, so a transitive copy at another version would resolve alongside it -- producing exactly the dual-instantiation the engine's instance guard exists to report. yarn.lock is unchanged: the pin matches what already resolved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ledger8CompactRuntime was satisfied by every post-fork runtime. StateValue.decode and `new ChargedState(...)` are structurally identical across the fork - the wire-shape drift detectors in this suite assert exactly that - so neither member could tell a pre-fork runtime from a post-fork one. onchain-runtime-v4 and compact-runtime, which re-exports it, both qualified; both are public barrel exports of this very package, so the wrong argument was one autocomplete away. It also failed silently: decode and re-encode then used the same post-fork codec, so the structural comparison, the Merkle walk and ChargedState construction all passed, and a v4 ChargedState came back typed as a v3 one - to surface later as an opaque wasm-bindgen rejection deep inside execution. ContractState now pins the era, through the GatherResult divergence in its query(). That divergence is incidental to this bridge rather than something it relies on, so a compile-time negative assertion holds the line: the pre-fix interface shape fails it, the fixed shape passes. An omitted or incomplete runtime was separately reported as DOWN_CONVERT_FAILED at an envelope-extraction stage, telling the caller to audit input bytes that were fine. The binding is now checked before any decoding and raises the new Ledger8RuntimeInvalidError, whose remediation points at the acquisition path. Asserts what the errors already promised: `cause` pass-through on all three DownConvertFailedError sites, by identity where the cause is injected, and the 'state down-convert' stage that separates a mid-conversion data loss from bad input bytes. Dropping either was previously invisible to the suite. Corrects comments that claimed more than the code delivers: - the prototype-chain 'toString' hazard yields '[object Undefined]', not '[object Object]' - the decoder is read into a local and called bare - dist-laziness does not yet cover lib/engine, since no build entry reaches it - wasm-bindgen checks wasm-class arguments but not method receivers, so a duplicate install can return plausible wrong bytes with no error at all; the instance guard is therefore load-bearing for correctness, not just diagnostics - the algebra does hold one undefined-valued slot, in a boundedMerkleTree leaf tuple, reached through the array branch - map iteration order is a canonical hash order, not ascending by key - the cross-codec block covers two containers and no Merkle tree - AXIS_PACKAGE_NAMES was renamed to axisPackageNames Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
VanessaPC
left a comment
VanessaPC
left a comment
There was a problem hiding this comment.
Reviewed the three new engine modules, the error classes, and the test suites against the base (feat/1004-v8-subpath-loadv8).
The core is solid, and I checked the parts that would be easy to get wrong rather than taking the docblocks' word for it: structurallyEqual's key-set guard and its Map/Uint8Array/BigInt/empty-container edges, the null-prototype decoder table against constructor / toString / __proto__, the exhaustiveness never plus its non-redundant runtime throw, DownConvertStage staying inside its union, readHexFixture's whole-hex length check, the golden-fixture decode matrix (all four golden assertions match the recorded outcomes, all five referenced fixtures exist), the split-scope defence in errors.ts against the pack-time rewrite (which does match on the scope with its slash, so it holds), and the onchain-runtime-v3-alt alias genuinely producing a second physical copy under nodeLinker: node-modules.
Eight comments inline. Two I'd like resolved before merge:
down-convert.ts:276—downConvertForExecutiondoesn't validateledger8Runtime, so a runtime fault surfaces asDownConvertFailedErrortelling the caller to audit input bytes that are fine. That's the mis-diagnosisLedger8RuntimeInvalidErrorwas added to prevent, at the one seam that skipped the check.errors.ts:283— the remediation tells callers to pass theContractStatethatloadLedger8()exposes. I pulled the published@midnightntwrk/ledger-v8@8.1.1typings: it exports its ownContractStatewith astatic deserialize, and has zero dependencies, so it cannot be onchain-runtime-v3's. Following the message yields a class that passes thetypeof deserialize === 'function'guard and decodes on a second WASM instance — the dual-instantiationinstance-guard.tsexists to catch.
The rest are low: three one-to-three-line fixes (errors.ts:130 prototype hole, instance-guard.ts:75 false dual-instantiation claim, errors.ts:242 dropped JSDoc), plus the ! assertions at down-convert.ts:212/215/219 on exported functions, the v9 path requiring an unused runtime, and an inverted comment premise in vitest.config.ts.
| * across the boundary needs the source-side tree, which this function never | ||
| * sees — it belongs at the envelope seam. | ||
| */ | ||
| export const downConvertForExecution = ( |
There was a problem hiding this comment.
medium — unguarded runtime seam
This validates nothing about ledger8Runtime, while the sibling seam validates precisely so that a runtime fault isn't reported as a data fault:
// envelope.ts:115
if (typeof ledger8ContractState?.deserialize !== 'function') {
throw new Ledger8RuntimeInvalidError('ContractState.deserialize');
}A runtime handed over without StateValue (the hand-assembled case Ledger8RuntimeInvalidError's own message calls out) produces TypeError: Cannot read properties of undefined (reading 'decode') at line 281, which the catch at line 292 re-throws as DownConvertFailedError('state down-convert', cause). That error's message then tells the caller to read the cause because it "distinguishes a tag mismatch ... from truncated, trailing, or empty input bytes" — sending them to audit input bytes that are fine. That's exactly the mis-diagnosis the new error code was added to prevent, at the one seam left unguarded.
A typeof ledger8Runtime?.StateValue?.decode !== 'function' / ChargedState check before the try would close it.
There was a problem hiding this comment.
Fixed in fc8f22d. downConvertForExecution now validates before the try:
if (typeof ledger8Runtime?.StateValue?.decode !== 'function') {
throw new Ledger8RuntimeInvalidError('StateValue.decode');
}
if (typeof ledger8Runtime.ChargedState !== 'function') {
throw new Ledger8RuntimeInvalidError('ChargedState');
}Only the two members this function actually dereferences are checked. ContractState is on Ledger8CompactRuntime to pin the era and is never called here, so guarding it would reject a runtime that works.
Five cases red first, each pinning the code and the missingMember: no runtime, null, a runtime with no StateValue, a StateValue whose decode is not callable, and a runtime with no ChargedState.
| constructor(readonly missingMember: string) { | ||
| super( | ||
| 'The ledger-8 runtime handed to the down-convert engine cannot be used: the binding it needs is missing. ' + | ||
| 'Acquire the runtime with loadLedger8() and pass the pre-fork ContractState class it exposes instead of ' + |
There was a problem hiding this comment.
medium — this remediation points callers into a dual-instantiation, and the guard won't catch it
Ledger8ContractState.deserialize (envelope.ts:43) is typed to return @midnight-ntwrk/onchain-runtime-v3's ContractState, and every real call site passes ocrt3.ContractState. But loadLedger8() (src/lib/load-v8.ts) dynamically imports src/v8.ts → @midnightntwrk/ledger-v8. From the published 8.1.1 tarball:
package/ledger-v8.d.ts:765 export class ContractState {
...
static deserialize(raw: Uint8Array): ContractState;
package/package.json dependencies: none, peerDependencies: none
So ledger-v8 exports its own ContractState, and with zero dependencies it cannot be re-exporting onchain-runtime-v3's. A caller who follows this message hands over a class that satisfies the typeof deserialize === 'function' guard above and then decodes envelopes on a second, unrelated WASM instance — the exact dual-instantiation instance-guard.ts exists to detect. So this isn't only a misleading string: it routes past the check meant to catch the failure it causes.
Related: the package exposes no sanctioned lazy accessor for onchain-runtime-v3 at all (which is why it had to become an unconditional runtime dependency). Worth saying that plainly here instead of pointing at loadLedger8().
There was a problem hiding this comment.
Verified the factual half independently rather than taking the quote. npm pack @midnightntwrk/ledger-v8@8.1.1 gives ledger-v8.d.ts:765 export class ContractState { ... static deserialize(raw: Uint8Array): ContractState } with data: ChargedState, and a package.json carrying neither dependencies nor peerDependencies. So the message did point callers at a different package's class running on its own WASM instance.
Fixed in fc8f22d: the remediation now names onchain-runtime-v3, says plainly that this package exposes no accessor for it, and mentions loadLedger8() only to rule it out. The test that pinned /loadLedger8/ was replaced by one asserting the message names onchain-runtime-v3 and the absent accessor.
One part I read differently, and it is why the fix is the string and not new machinery: "routes past the check meant to catch the failure it causes" overstates the exposure. Nothing crossing this seam is a WASM handle. deserialize(raw).data.state.encode() returns a POJO, and that POJO is then decoded by onchain-runtime-v3 inside downConvertForExecution. The failure instance-guard.ts exists for - a wasm-bindgen _assertClass rejection, or a __wbg_ptr read against the wrong linear memory - is not reachable this way. What is left is narrower: a schema divergence between the two encoders' POJOs, which a guard on class identity would not catch either.
| return; | ||
| case 'map': { | ||
| const map = sv.asMap()!; | ||
| map.keys().forEach((key) => assertMerkleTreesRehashed(map.get(key)!)); |
There was a problem hiding this comment.
low/medium — the ! assertions are unsound on this docblock's own terms
The docblock at line 202 says the vendor typings "are not authoritative about definedness here (asCell() is declared non-optional yet returns undefined for a null state value)", and the default arm at line 225 exists because a caller-injected runtime "can emit a tag the pinned .d.ts does not declare". Granting both premises, the assertions can't be relied on:
- a runtime whose
type()reports'map'whileget()fails to resolve a key marshalled out bykeys()→assertMerkleTreesRehashed(undefined)→ bareTypeErroronundefined.type() - same shape for
sv.asArray()!(line 215) andsv.asBoundedMerkleTree()!(line 212) →checkRoot(undefined)
Inside downConvertForExecution that's wrapped, but assertMerkleTreesRehashed and checkRoot are both exported and documented as throwing only MerkleNotRehashedError / DownConvertFailedError. A direct caller therefore gets an uncoded error out of a seam whose whole contract is code-based discrimination. An explicit undefined check throwing DownConvertFailedError is one line each.
There was a problem hiding this comment.
The inconsistency is real and I am not disputing it: the docblock says the vendor typings are not authoritative about definedness, the default arm exists precisely because a caller-injected runtime can emit a tag the pinned .d.ts does not declare, and then three ! trust those same typings.
Not in this round, and not because it is wrong. The package holds a per-file 100% branch floor, so each guard needs a fake-runtime test forcing asBoundedMerkleTree() / asArray() / map.get() to return undefined - three branches plus three tests, rather than one line each. Recorded as an explicit follow-up in the PR body instead of being folded into a round that was otherwise message and doc fixes.
| * this same name under a different scope, so naming only one scope would point | ||
| * every consumer installed from the other at a package not in their tree. | ||
| */ | ||
| const AXIS_BARE_PACKAGE_NAMES: Readonly<Record<Ledger8InstanceAxis, string>> = Object.freeze({ |
There was a problem hiding this comment.
low — fail-open prototype hole, the same one this PR deliberately closed for ENVELOPE_DECODERS
This table is Object.freezed but keeps its prototype, and axis is never validated before axisPackageNames indexes it at line 135. For the untyped-JS consumers these errors are explicitly written for:
assertSharedLedger8Instance('constructor', a, b)→ message readsTrace @midnight-ntwrk/function Object() { [native code] } and @midnightntwrk/function Object() { [native code] } with your package manager's ``why`` command'__proto__'or any other non-axis string →@midnight-ntwrk/undefined
envelope.ts:71 gets exactly this right (Object.create(null) + validate-before-dispatch, with a documented rationale). Same treatment here — Object.create(null) plus a guard in assertSharedLedger8Instance — closes it.
There was a problem hiding this comment.
Fixed in fc8f22d. AXIS_BARE_PACKAGE_NAMES is now Object.freeze(Object.assign(Object.create(null), { ... } satisfies Record<Ledger8InstanceAxis, string>)), and assertSharedLedger8Instance validates axis against a closed set before either probe is looked at, throwing a new UnknownLedger8AxisError (MIDNIGHT_JS_P_UNKNOWN_LEDGER8_AXIS) that keeps the offending string in a field rather than the message.
The axis set lives in instance-guard.ts, not in errors.ts. errors.ts is a build entry, so a predicate exported from it becomes public API that nothing outside the engine needs - and version.ts sets the precedent of exporting the tuple but no predicate. The one duplicated literal is checked, not trusted: satisfies Record<Ledger8InstanceAxis, true> fails the build if the union gains a member.
Covered by constructor / __proto__ / toString / valueOf / bogus, a non-string axis, and a direct pin on the table asserting no [native code] or [object reaches the message.
One distinction worth recording next to the ENVELOPE_DECODERS comparison: that hole was fail-open - it returned a value and threw nothing. This one only garbled the text of an error that was already being thrown, and only when a bogus axis coincided with a genuine probe failure. Same fix, different severity.
| * why `'onchain-runtime-v3'` is the only member today. | ||
| */ | ||
| export const assertSharedLedger8Instance = (axis: Ledger8InstanceAxis, probeA: unknown, probeB: unknown): void => { | ||
| if (probeA == null || probeB == null || probeA !== probeB) { |
There was a problem hiding this comment.
low — nullish probes are reported with a factually false diagnosis
Rejecting nullish probes before the === is right, and the reasoning in the docblock is sound. The problem is the error they get: Ledger8InstanceMismatchError's message asserts "Detected two physically distinct copies of onchain-runtime-v3 loaded into the same process (a dual-instantiation)" and sends the reader to npm why to hunt a duplicate install.
A caller who optional-chained an export that moved, or simply omitted a probe, is told something untrue about their dependency tree and pointed at a hunt that will find nothing. This PR adds Ledger8RuntimeInvalidError for precisely the "handed over incomplete" fault at the envelope seam; as it stands the two seams diagnose the same class of caller error differently.
There was a problem hiding this comment.
Fixed in fc8f22d. Nullish probes now throw Ledger8RuntimeInvalidError with missingMember: 'onchain-runtime-v3 instance probe', and Ledger8InstanceMismatchError is reserved for an actual reference-equality failure, so the two seams diagnose the same class of caller error the same way.
The guard is now three ordered checks - axis, then nullish, then !== - which also means axis is known to be one of this package's own literals before it reaches missingMember, keeping that field's "never caller-supplied text" property intact.
The three existing nullish cases were flipped to assert the new code and to assert the message does not contain dual-instantiation, so a regression that reintroduced the false diagnosis fails rather than passing on a matching axis.
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
low — this doc block is dropped, and UnknownLedgerVersionError ships undocumented
Two JSDoc blocks sit back to back: this one (lines 242–257) documents UnknownLedgerVersionError — it names version, LEDGER_VERSIONS and requestedVersion — and line 258 opens a second block documenting Ledger8RuntimeInvalidError.
A doc comment immediately followed by another doc comment is discarded, so the class actually declared at line 290 (UnknownLedgerVersionError) ships with no TypeDoc output and no IDE hover, while the block that does survive describes a different class. Looks like residue from the #1168 conflict resolution described in the PR body. Moving this block down to line 289 fixes both halves.
There was a problem hiding this comment.
Fixed in fc8f22d. The block moved down to the class it documents; confirmed in the emitted typings rather than assumed - dist/errors.d.ts now carries it directly above declare class UnknownLedgerVersionError.
Also swept the rest of the file for the same shape: no other back-to-back doc blocks. UnknownLedger8AxisError was added in the same round and sits above UnknownLedgerVersionError, so it has its own block and does not re-orphan this one - which it did on the first attempt, before the sweep caught it.
| if (typeof decoder !== 'function') { | ||
| throw new UnknownLedgerVersionError(String(version)); | ||
| } | ||
| if (typeof ledger8ContractState?.deserialize !== 'function') { |
There was a problem hiding this comment.
low — the v9 path requires a runtime it provably never uses
The v9 decoder (line 75) never reads ledger8ContractState, yet this check rejects its absence for every version. The docblock defends that as keeping "a v9 caller from drifting into a v8 call that has no runtime to reach for", but the cost runs against the goal this same file states twice — that "a v9-only consumer would pay for a runtime it never calls".
Concretely: a node-2.x-only consumer must acquire the retained pre-fork onchain-runtime-v3 runtime purely to extract a post-fork envelope, and extractEncodedStateValue(raw, 'v9', undefined) throws Ledger8RuntimeInvalidError for an argument that is dead on that path.
An optional third parameter, checked inside the 'v8' decoder (or right after the version validation, only when version === 'v8'), gives the same accurate diagnosis without the forced acquisition.
There was a problem hiding this comment.
Kept as it is; the docblock is corrected in 64a5068.
Two things came out of checking this one. First, the stated cost does not exist on this stack. extractEncodedStateValue has exactly one call site anywhere in it - createLedger8Engine() in #1168 / #1165 - and that factory has already awaited onchain-runtime-v3 two lines earlier to assemble the runtime it hands to downConvertForExecution. The v9 decoder is there so the bridge can read a post-fork envelope before down-converting it, not to serve a caller holding no pre-fork runtime; such a caller reads ledger-v9 directly and never reaches this function. So no consumer is currently forced to acquire anything.
Second, the shape of the alternative matters. That call site is (raw, version) => extractEncodedStateValue(raw, version, ocrt3.ContractState), where version is a LedgerVersion union value rather than a literal - so overloads, or a discriminated parameter object, would not compile against it. The only workable version of your suggestion is the optional third parameter, which trades a compile-time error on the one path that genuinely needs the argument for a runtime one.
What was actually wrong was the justification. The comment defended the requirement by the cost to a v9-only consumer while the same file argues laziness on a different axis (the erased import type, which is about bundling, not about the call contract). The docblock now separates those two, gives the call-graph reason, and records the condition that flips the decision: if this seam is ever surfaced beyond the engine, make the parameter optional and move the check into the 'v8' decoder.
| // a glob matching no file is ignored silently, so renaming or moving | ||
| // one of these files deletes its floor without any warning; and while | ||
| // the global thresholds above are also 100, these entries are | ||
| // redundant. They exist so that a future lowering of the global floor |
There was a problem hiding this comment.
low — the comment's premise is inverted, which matters exactly when someone edits this
In Vitest, files matched by a thresholds glob key are removed from the global threshold aggregate and judged only against their own entry. So these four entries aren't redundant with the global 100s — they change which files the global gate covers.
No behavioural difference while every number here is 100. But this comment is what a future editor would rely on when lowering one of these floors, and at that moment the file also silently stops contributing to the package-wide gate — the opposite of the protection the comment promises ("a future lowering of the global floor cannot quietly take these files down with it").
The neighbouring warning about a glob matching no file being silently ignored is accurate and worth keeping.
There was a problem hiding this comment.
Not changing this one - the comment is correct for the pinned toolchain, and I think the premise here is the inverted one.
Vitest 4.1.7, node_modules/vitest/dist/chunks/coverage.*.js, resolveThresholds:
// Global threshold is for all files, even if they are included by glob patterns
for (const file of files) {
const fileCoverage = coverageMap.fileCoverageFor(file);
globalCoverageMap.addFileCoverage(fileCoverage);
}The loop runs over every file with no exclusion of glob-matched ones; the per-glob entries are pushed as additional threshold sets alongside it. So these four entries are an extra gate on top of the global 100s, not a replacement for them, and lowering one of these floors does not drop that file out of the package-wide gate.
The exclusion behaviour you describe was real in earlier majors and is still what a lot of the circulating documentation says, so this is worth having on the record rather than left to be re-raised. The neighbouring warning about a glob matching no file being silently ignored stands as you say.
Review of #1164 found five seams whose failure reporting was wrong or whose docs did not ship. All are caller-facing diagnoses, none change a success path. - downConvertForExecution validated nothing about the runtime it dereferences, so a missing binding surfaced as a TypeError that the catch relabelled DOWN_CONVERT_FAILED - an error telling the caller to audit input bytes that are fine. It now fails with LEDGER8_RUNTIME_INVALID naming the absent member, matching the sibling check on the envelope seam. - Ledger8RuntimeInvalidError told callers to pass the ContractState that loadLedger8() exposes. loadLedger8() resolves the v8 ledger package, which exports its own same-named class with a matching static deserialize on a separate WASM instance, so following the message passed the duck-typed guard and decoded on the wrong copy. The message now names onchain-runtime-v3 and says plainly that this package exposes no accessor for it. - The UnknownLedgerVersionError doc block sat immediately before a second doc block, so it was discarded and the class shipped with no typings comment. Moved down to the class it documents. - A nullish instance probe was reported as a dual-instantiation, telling the caller two physical copies exist and sending them to npm why after a duplicate that is not there. It is now LEDGER8_RUNTIME_INVALID, the same code the envelope seam already uses for a binding handed over incomplete. - The axis package-name table kept its prototype and the axis was never validated, so a non-axis string rendered an Object.prototype member as the npm package to trace. The table is null-prototype and frozen, and the axis is validated against a closed, satisfies-checked set before any probe is compared, throwing the new UnknownLedger8AxisError. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…real call graph The docblock defended requiring ledger8ContractState on the v9 path by the cost to a v9-only consumer, and that consumer does not exist: extractEncodedStateValue is reached only from the ledger-8 engine's factory, which has already awaited onchain-runtime-v3 to assemble the runtime it hands to downConvertForExecution. The v9 decoder is there so the bridge can read a post-fork envelope before down-converting it, not to serve a caller holding no pre-fork runtime. Says that instead, keeps the two arguments apart - the call-graph claim here and the bundling claim behind the import type on Ledger8ContractState are unrelated - and records the condition that would flip the decision, so a later PR surfacing this seam beyond the engine knows to make the parameter optional and move the check into the v8 decoder. Docs only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o feat/1004-engine-core # Conflicts: # packages/protocol/src/test/dist-laziness.test.ts
3 participants
Overview
PR 1004-D of the MJS-01 series (#1004) - adds the ledger-8 engine core to
@midnight-ntwrk/midnight-js-protocol. Stacked on #1159 (HF fixtures), which stacks on #1156 <- #1155. Merge order: #1155 -> #1156 -> #1159 -> this.Now also carries #1186 (golden-fixture assertions), merged into this branch rather than landing as a separate PR.
Summary
src/lib/engine/envelope.ts- version-awareextractEncodedStateValue(raw, version, ledger8ContractState): decodes v8-era envelopes (pre-forkcontract-state[v6]) and post-fork v9 envelopes into the sharedEncodedStateValuePOJO.versionis validated against the decoder table before dispatch; an unknown value throws the codedUnknownLedgerVersionErrorrather than resolving through the prototype chain. Any decode failure is a typedDownConvertFailedError(MIDNIGHT_JS_P_DOWN_CONVERT_FAILED), never a silently empty state.src/lib/engine/down-convert.ts-downConvertForExecution(state, ledger8Runtime): v9->v8 state bridge over the WASM packages' ownencode()/decode()POJO round-trip. Structural integrity is checked by re-encoding and deep-comparing against the source.assertMerkleTreesRehashedwalks the state and asserts every bounded Merkle tree has a readable root viacheckRoot, throwingMerkleNotRehashedError(MIDNIGHT_JS_P_MERKLE_NOT_REHASHED); an unrecognisedStateValuevariant is a hard failure, not a silent skip.src/lib/engine/instance-guard.ts- dual-WASM fail-fast:assertSharedLedger8Instance(axis, probeA, probeB)compares two class bindings obtained from two different acquisition paths (a namespace object is not a valid probe - a re-export produces a fresh one over a single physical copy). Rejects nullish probes; throwsLedger8InstanceMismatchError. The single genuine axis isonchain-runtime-v3.src/errors.ts- four new error classes on the registry:Ledger8InstanceMismatchError,DownConvertFailedError,MerkleNotRehashedError,UnknownLedgerVersionError. Each error'scodefield is typed as its own literal, soinstanceofandcodecannot drift and consumers canswitchexhaustively.src/test/engine-golden-fixtures.test.tsasserts the bridge against the committed hard-fork goldens: a real migrated on-chain state down-converts byte-identically to its pre-migration v8 form, a golden Merkle state comes out with a readable root, and every tampered golden fails closed without leaking bytes.packages/protocol/turbo.jsondeclares the fixture directory as a test input so editing a golden invalidates the test cache.src/version.tsand all engine modules; package totals stay 100 and always-on.@midnight-ntwrk/onchain-runtime-v3@3.1.0as a protocol dependency, now also pinned in the rootresolutionsalongside every other vendor runtime. The devDep npm aliasonchain-runtime-v3-altgives the dual-instance negatives a real second physical WASM copy.Engine modules are intentionally not exported from the root entry or
./v8- the root stays v9-static; export surfacing lands with the execution legs (PR 1004-E) as a single lazy facade,loadLedger8Engine().Review fixes applied on this branch
A multi-agent review of the original diff surfaced these; all are fixed here.
.github/scripts/publish-public-npm.mjs) rewrites the old scope to the new one inside built.js/.d.tsfiles, not only inpackage.json.AXIS_PACKAGE_NAMESheld both scoped names as literals, so the public-npm artifact collapsed them into one - and no test could see it, since the suite runs against unrewritten source. Scopes are now held apart from the/and joined at use; a test asserts the module carries no scoped literal at all.ENVELOPE_DECODERSwas fail-open. A plain object literal indexed byversionresolved unexpected keys throughObject.prototype:'constructor'handed the caller's own raw bytes back as anEncodedStateValue,'toString'returned'[object Object]'- neither throwing, both reachable from untyped JS. The table is now null-prototype and frozen, andversionis validated before dispatch. Validating first also keepsstageinside its closed union.switchhad nodefaultand returnsvoid, so a variant outside the five it names fell through having checked nothing and recursed into nothing. The compile-time exhaustiveness guard is now folded into theswitchas aneverdefault that also throws at runtime - the pinned.d.tscannot vouch for a caller-injected runtime's WASM.checkRootmishandled a fallible vendor binding. The wasm-bindgen shim forroot()rethrows a RustErr; that throw escaped uncoded and was relabelledDOWN_CONVERT_FAILED, pointing the caller at its envelope bytes when the fix is to rehash. A throw and anullare now both reported asMERKLE_NOT_REHASHED, carrying the cause.structurallyEqualcompared key counts, not key sets - two objects sharing no key compared equal whenever the diverging key's value wasundefined. Unreachable in today's algebra; guarded so it stays that way.dist-lazinessonchain-runtime-v3 case passes today because no build entry reaches the engine at all, and the vendor drift detectors are checked by the pre-push hook only, never by CI. Both now say so.Second review round (VanessaPC), fixes applied
Five of the eight inline comments are fixed in
fc8f22d; 20 tests red first, all green after. Package stays 239/239, coverage 100/100/100/100, lint clean.downConvertForExecutionvalidated nothing aboutledger8Runtime. A runtime missing a binding this function dereferences raised a bareTypeErrorthat thecatchrelabelledDOWN_CONVERT_FAILED- an error whose message sends the caller to audit input bytes that are fine. It now throwsLEDGER8_RUNTIME_INVALIDnaming the absent member, matching the check the envelope seam already had. Only the two members it actually calls are checked;ContractStateis on the interface to pin the era and is never dereferenced here.Ledger8RuntimeInvalidError's remediation pointed at the wrong package. It told callers to pass theContractStatethatloadLedger8()exposes.loadLedger8()resolves@midnightntwrk/ledger-v8, which (verified against the published 8.1.1 tarball: zero dependencies,ledger-v8.d.ts:765) exports its ownContractStatewith a matchingstatic deserializeon a separate WASM instance. Following the message therefore yielded a class that passes the duck-typed guard and decodes on the wrong copy. The message now names onchain-runtime-v3 and says plainly that this package exposes no accessor for it.UnknownLedgerVersionErrorshipped undocumented. Its doc block sat immediately before a second doc block, so it was discarded and the class emitted with no typings comment. Moved down to the class it documents; confirmed indist/errors.d.ts.npm whyafter a duplicate that is not there. Nullish probes now throwLEDGER8_RUNTIME_INVALID- the same code the envelope seam uses for a binding handed over incomplete - and the mismatch error is reserved for an actual reference-equality failure.axiswas never validated, so a non-axis string rendered anObject.prototypemember as the npm package to trace (@scope/function Object() { [native code] }). The table is now null-prototype and frozen, andaxisis validated against a closed,satisfies-checked set ininstance-guard.tsbefore any probe is compared, throwing the newUnknownLedger8AxisError(MIDNIGHT_JS_P_UNKNOWN_LEDGER8_AXIS). The axis set lives in the engine module rather than inerrors.ts, which is a build entry - nothing outside needs to test an axis, andsatisfiesfails the build if the union grows.Not applied:
vitest.config.ts:52- the comment is correct as written. The claim that a glob-matched file leaves the global aggregate does not hold for the pinned Vitest 4.1.7:resolveThresholdsadds every file to the global map, under its own comment// Global threshold is for all files, even if they are included by glob patterns. That was the behaviour in older majors, not this one, so the per-glob entries are an extra gate rather than a replacement - which is what the comment says.down-convert.ts:212/215/219(the!assertions) - the consistency argument holds, but each new guard needs a fake-runtime test to keep the per-file 100% branch floor, so it is a follow-up rather than part of this round.envelope.ts:115(v9 requiring the pre-fork runtime) - kept as it is, docblock corrected in64a5068. The comment defended the unconditional requirement by the cost to a v9-only consumer; there is no such consumer.extractEncodedStateValuehas one call site in the whole stack -createLedger8Engine()in feat(midnight-js): add ledger-8 circuit execution and keep-state wrap behind the loadLedger8Engine facade #1168/feat(midnight-js): add v8-native tx composition incl. deploy machinery #1165 - and it has already awaited onchain-runtime-v3 two lines earlier to assemble the runtime fordownConvertForExecution. The v9 decoder is there so the bridge can read a post-fork envelope before down-converting it; a caller with no pre-fork runtime reads ledger-v9 directly and never reaches this function. Note also that overloads (or a discriminated parameter object) would not compile against that call site, which passesversionas aLedgerVersionunion value rather than a literal. The docblock now gives the real reason and records the condition that flips it: if this seam is ever surfaced beyond the engine, make the parameter optional and move the check into the'v8'decoder.Downstream:
assertSharedLedger8InstanceanddownConvertForExecutionchange only which coded error a misuse produces, and no signature moved, so #1168 and #1165 need a merge-forward but no code change.Dependent PRs: already synced
The stack has been merged forward, so #1168 and #1165 already carry these fixes.
feat/1004-engine-execution) - 270/270, lint clean, coverage 100/100/100/100. Two conflicts needed a decision rather than a union. Both branches had hardened the same prototype-chain hole inextractEncodedStateValue; this branch's resolution won, because a distinctUNKNOWN_LEDGER_VERSIONcode lets a consumer tell a bad version from bad bytes, and the offending string stays in a field instead of being interpolated into the message. The'envelope extraction'member ofDownConvertStageis unreachable under that resolution and was dropped. feat(midnight-js): add ledger-8 circuit execution and keep-state wrap behind the loadLedger8Engine facade #1168's prototype-member test was superseded by the wider one here, so the duplicate went and its two unique contributions were folded into the survivor: the'v7'case, and the note thatLedger8Engine.extractStateis the reachable path.feat/1004-engine-legs) - 301/301, lint clean, coverage 100/100/100/100, no test changes needed. Itsdist-lazinessversion was kept wholesale: the comment here says no build entry reacheslib/engine, which is true on this branch and false there, since feat(midnight-js): add v8-native tx composition incl. deploy machinery #1165 has the./engineentry.Both call
extractEncodedStateValue(raw, version, ocrt3.ContractState)andassertSharedLedger8Instance('onchain-runtime-v3', ocrt3.ChargedState, glue.ChargedState)- class bindings, matching the narrowed contract - so no signature changes were needed downstream.Also worth knowing for 1004-E:
onchain-runtime-v3stays a runtimedependency(not a devDependency), because #1168 imports it dynamically in production.Test plan
Submission Checklist
Links