test(chain): adapter-level coverage for V10 publish/update approval gate (#720 follow-up)#735
Merged
Merged
Conversation
…ate (#720 follow-up) PR #720 shipped the `effectivePublishAllowance` / `computeApprovalAction` helpers with thorough pure-function tests, but the actual `publishV10` / `updateV10` call sites that wire `token.allowance()` → policy dispatch → `token.approve()` had no adapter-level coverage. Codex flagged this in the #716 review-consolidation audit: the helper tests cannot catch mistakes in the call-site wiring (wrong signer, wrong KA address, swapped labels, swallowed approve failures, missed `1n` floor at the boundary between helper and adapter, etc.). This PR closes that gap. ## Refactor `publishV10` and `updateV10` had two near-identical inline blocks (`evm-adapter.ts:2362-2382` and `:2796-2816`) that read the allowance, called `computeApprovalAction`, and conditionally broadcast `approve`. Extracted into a single private helper: `EVMChainAdapter.ensureV10ApproveTrac(signer, kav10Address, tokenAmount, txLabel)` Both call sites now delegate. Pure literal extraction — no behaviour change (verified by the existing helper tests plus the new ones below). The extraction also gives the tests a single seam to mock around (`(a as any).contracts.token` + spy on `sendContractTransaction`), without dragging the broadcast / failover / signing machinery into the unit-test surface. ## New tests (`evm-adapter.unit.test.ts`, +33 cases) `ensureV10ApproveTrac — per-publish (default) approval gate` - zero-cost publish on fresh wallet → approve(1n) ← the #720 mainnet revert scenario, now asserted end-to-end at the adapter layer - metadata-only update with existing 1n allowance → NO approve - zero-cost publish with comfortable leftover allowance → NO approve - positive tokenAmount with empty allowance → approve(tokenAmount) - positive tokenAmount with partial allowance → approve(tokenAmount) - positive tokenAmount fully covered → NO approve - boundary: allowance exactly equals tokenAmount → NO approve - read-only adapter (no token contract bound) → no-op `ensureV10ApproveTrac — replenishing policy` - fresh wallet → approve default 1000 TRAC ceiling - allowance comfortably above 10% threshold → NO approve - allowance below threshold → refill to target - custom targetAllowance + refillBelowFraction honoured both sides of the threshold boundary `ensureV10ApproveTrac — unlimited policy` - fresh wallet → approve MaxUint256 - wallet with MaxUint256 live → NO approve - wallet with partial residual ≥ publish floor → NO approve (defensive policy-switch case) - external revoke (allowance=0) → re-approves MaxUint256 `ensureV10ApproveTrac — call-site invariants` - publish label passed through verbatim (on-chain tracing) - update label passed through verbatim - token contract connected to operational signer (not admin) - allowance read against the passed-in KA address (no cache leak) - approve broadcast failures propagate (publish/update aborts cleanly) - 2^200 allowance handled without Number coercion (bigint safety) All 126 unit tests pass; `tsc` build clean. No production-code semantics changed. Closes the #720 follow-up requested in #716's review audit. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced May 27, 2026
matic031
pushed a commit
to KilianTrunk/dkg
that referenced
this pull request
Jun 2, 2026
…dler + canonical CG keying (OriginTrail#729 Bug 4) Adds `packages/publisher/test/v10-ack-v2-chunked.test.ts` — 8 cases exercising the LU-11 / OT-RFC-39 V2 chunked ACK path landed by PR OriginTrail#715/OriginTrail#717 and the canonical-CG-keying fix in PR OriginTrail#729 Bug 4. Background ========== OriginTrail#716 review-consolidation audit flagged this as the closest analogue of the gap PR OriginTrail#735 closed for OriginTrail#720: helper-level primitives (chunked AEAD, ciphertext Merkle tree, proto wire format, on-chain commitment fields) are well tested, but the StorageACKHandler that wires them together at the ACK boundary has zero direct coverage. The existing `v10-ack-edge-cases.test.ts` thoroughly covers the V1 (inline staging quads / inline encrypted blob) paths but skips V2 entirely. Worse, PR OriginTrail#729 Bug 4 shipped a fix to the V2 ACK `loadChunk` graph canonicalisation (`normalizeContextGraphIdForChunkStore` returning null → wildcard `GRAPH ?g` fallback) without a regression test — any future refactor of that hook could silently re-introduce the "keccak the decimal string '42'" miss. Scope ===== Eight cases — happy paths first, then the regression and decline shapes: 1. **Happy path** — cleartext CG, canonicalising normalizer, chunks persisted under `ciphertextChunkStoreGraph(canonical(swmGraphId))`, ACK signed. Pins the production canonicalisation path. 2. **OriginTrail#729 Bug 4 regression** — V2 intent omits `swmGraphId`, normalizer returns `null` on every input → handler MUST widen to `GRAPH ?g` and still find the chunks. The pre-fix code keccak'd the decimal `cgId` ('42') and missed every persisted chunk; the test now locks the fix in. 3. **Legacy no-normalizer shim** — `normalizeContextGraphIdForChunkStore` absent → uses raw `swmGraphId` literally, preserving pre-fix behaviour for callers that haven't wired the hook. 4. **Multi-CG isolation (PR OriginTrail#715 / ciphertext-chunk-store.ts:28-44)** — two CGs publish identical V10 KCs (same batchId, since it's plaintext-derived) but persist chunks under different canonical named graphs. ACK for CG-A must only see CG-A's chunks; an intent claiming CG-B's root for the same batchId is correctly declined with `CIPHERTEXT_ROOT_MISMATCH`. This pins the Codex finding that drove the per-CG named-graph design. 5. **`MISSING_CIPHERTEXT_CHUNKS` decline** — claim count=4, persist only indexes 0 and 2 → declines after the 10-second retry window with the missing indexes in the message ("missing 2/4 ... 1,3"). 6. **`CIPHERTEXT_ROOT_MISMATCH` decline** — all chunks present but publisher lies about the root → declines fast. 7. **Curated-only gate** — `isCgCurated` returns false → declines with `SIGNER_NOT_REGISTERED` (the V2-curated-only gate at `storage-ack-handler.ts:296-310`). Closes the bypass concern called out in the inline comment above the V2 branch. 8. **stagingQuads forbidden on V2 wire** — V2 intent with non-empty `stagingQuads` is rejected (the chunked path never carries inline ciphertext). Test harness ============ Real `OxigraphStore` (mirrors `v10-ack-edge-cases.test.ts` pattern), real `encodePublishIntent` / `decodeStorageACK` proto round-trips, real `buildCiphertextChunksRoot` Merkle tree construction. The new `buildV2IntentBytes(opts)` helper derives `ciphertextChunksRoot`, `ciphertextChunkCount`, and `publicByteSize` from the chunks list unless explicitly overridden — production wire shape, not a fake. The `seedChunks(store, opts)` helper inserts chunk literals under the same `(graph, subject, predicate)` layout that `ingestSwmCiphertextChunkEnvelope` in `dkg-agent.ts` writes. Verification ============ - `pnpm --filter @origintrail-official/dkg-publisher build` — clean - `vitest run test/v10-ack-v2-chunked.test.ts` — 8/8 pass - `vitest run test/v10-ack-v2-chunked.test.ts test/v10-ack-edge-cases.test.ts` — 53/53 pass (8 new + 45 existing); no regressions to the V1 suite Closes audit finding flagged by the OriginTrail#716 review-consolidation deep dive. Companion to PR OriginTrail#735 (OriginTrail#720 adapter coverage) and OriginTrail#737 (OriginTrail#700 drain bug). Co-authored-by: Cursor <cursoragent@cursor.com>
matic031
pushed a commit
to KilianTrunk/dkg
that referenced
this pull request
Jun 2, 2026
…onder + OriginTrail#729 Bug 5 canonical CG keying Adds `packages/agent/test/lu11-handle-get-ciphertext-chunk.test.ts` — 6 cases exercising the LU-11 / OT-RFC-39 `get-ciphertext-chunk` sync verb responder shipped by PR OriginTrail#717 and the canonical-CG-keying fix in PR OriginTrail#729 Bug 5. Background ========== OriginTrail#716 review-consolidation audit flagged this as one of the critical adapter-wiring gaps in the same class as OriginTrail#720/OriginTrail#735: helper-level signing primitives (`mintSignedCiphertextChunkCatchupRequest`, `verifySignedCiphertextChunkCatchupRequest`, replay guard) are present, but the responder that wires them together with the 5-layer authority stack and the SPARQL chunk lookup is completely untested. Worse, PR OriginTrail#729 Bug 5 shipped a fix to the responder's chunk-graph lookup without a regression test: - Pre-fix: `gossipWireIdFor(req.contextGraphId)` was called unconditionally. For a numeric on-chain id like "42" this keccak'd the literal decimal string and produced a graph URI nothing was ever persisted under — every late-joining core's backfill request silently returned "chunk not found" even when the bytes were on disk under the curator's nameHash. - Fix: routes through `canonicalChunkStoreCgIdOrNull` which returns `null` for inputs it can't safely canonicalise; the responder then widens to a `GRAPH ?g` wildcard scan. Scope ===== Six cases — happy path, the Bug 5 regression, structured-deny shapes, and the replay-guard boundary: 1. **Happy path** — subscribed cleartext CG → keccak wire hash → scoped lookup → chunk returned. Pins the production canonicalisation path the responder relies on. 2. **OriginTrail#729 Bug 5 regression** — numeric `contextGraphId = "42"` with no local CG mapping → `canonicalChunkStoreCgIdOrNull` returns `null` → handler widens to `GRAPH ?g` → finds the chunk persisted under the curator's real nameHash graph. Locks the fix in so a future refactor can't silently re-introduce the decimal-string keccak miss. 3. **`chunk not found` decline** — authorised requester + canonicalisable CG but no chunk persisted → structured `denied: 'chunk not found'` with echoed `(contextGraphId, batchIdHex, chunkIndex)` for requester correlation. 4. **Unauthorised requester** — full 5-layer auth fall-through: none of `resolveOnChainParticipantAgents`, `resolveBeaconPinnedCuratorEoa`, `getContextGraphAgentGateAddresses`, `getContextGraphAllowedPeers`, `chain.getIdentityIdForAddress` admit the requester → structured `denied: 'requester EOA not in any of: ...'` (or `'no authority source available'` when every probe returns null/undefined on MockChainAdapter). 5. **Malformed request bytes** — decoder throws → handler maps to `denied: 'malformed request: ...'` with defensive defaults on the echo fields (empty `contextGraphId`, empty `batchIdHex`, `-1` chunkIndex) so an attacker-controlled garbage payload can't leak back through the response envelope. 6. **Replay-guard boundary** — same wire bytes twice (same nonce, same issuedAtMs, same signature) → first attempt succeeds, second is rejected with `denied: 'replayed chunk-catchup nonce'`. Pins `ciphertextChunkCatchupReplayGuard.recordIfFresh()` so the defensive boundary against signed-envelope replay holds. Test harness ============ Real `DKGAgent` instance booted on `MockChainAdapter`. The responder method is reached through an `(agent as unknown as ResponderInternals)` cast — same pattern as the existing `swm-sender-key-pending-by-agent.test.ts`. Real ciphertext-catchup proto round-trip (`mintSignedCiphertextChunkCatchupRequest` → `encodeCiphertextChunkCatchupRequest` → `handleGetCiphertextChunk` → `decodeCiphertextChunkCatchupResponse`), real EIP-191 personal-sign via `ethers.Wallet.signMessage`. The OT-RFC-39 fifth authority is exercised by monkey-patching `chain.getIdentityIdForAddress` on the agent's chain adapter — the other four authorities answer null/undefined naturally on MockChainAdapter. Verification ============ - `pnpm --filter @origintrail-official/dkg-agent build` — clean - `vitest run test/lu11-handle-get-ciphertext-chunk.test.ts` — 6/6 pass No production code changes. Test-only PR. Closes audit finding flagged by the OriginTrail#716 review-consolidation deep dive. Companion to PR OriginTrail#735 (OriginTrail#720), PR OriginTrail#737 (OriginTrail#700 drain bug), PR OriginTrail#738 (OriginTrail#729 Bug 4 V2 ACK loadChunk canonical keying). Co-authored-by: Cursor <cursoragent@cursor.com>
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
1 participant
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.
Why
PR #720 shipped the
effectivePublishAllowance/computeApprovalActionhelpers (with thorough pure-function tests), and #716's review-consolidation audit flagged that we did not have adapter-level tests asserting the actualpublishV10/updateV10call sites correctly wire:token.allowance()→computeApprovalAction(policy, …)→token.approve()The pure-function tests can't catch mistakes in the call-site wiring — wrong signer, wrong KA address, swapped labels, swallowed approve failures, missed
1nfloor at the boundary between helper and adapter, etc. The original plan was to defer this torc.12.1; the user explicitly overrode that. This PR closes the gap inrc.12so we ship the #720 fix with the call-path coverage it deserves.What
1. Small refactor (pure literal extraction, no behaviour change)
publishV10andupdateV10had two near-identical inline approve blocks (evm-adapter.ts:2362-2382and:2796-2816). Extracted into one private helper:Both call sites now delegate to it. This also gives the tests a single seam to mock around (
(a as any).contracts.token+ spy onsendContractTransaction), so we can assert the wiring without dragging the broadcast / failover / signing machinery into the unit-test surface.2. +33 adapter-level test cases in
evm-adapter.unit.test.tsensureV10ApproveTrac — per-publish (default)(the #720 policy)approve(1n)← the fix(chain): close TRAC auto-approve gap (1n floor) + configurable allowance policy #720 mainnet revert scenario, now asserted end-to-endapprove(tokenAmount)approve(tokenAmount)(bounded, no widening)ensureV10ApproveTrac — replenishing(recommended for mainnet operators)targetAllowance+refillBelowFractionhonoured both sides of the threshold boundaryensureV10ApproveTrac — unlimited(V9 pattern)MaxUint256MaxUint256live → NO approveMaxUint256ensureV10ApproveTrac — call-site invariantsNumbercoercion (bigint safety)Verification
pnpm --filter @origintrail-official/dkg-chain test:unit→ 126 passed, 0 failed (33 new + 93 existing)pnpm --filter @origintrail-official/dkg-chain build→ clean (tscno errors)effectivePublishAllowance,computeApprovalAction × {per-publish, replenishing, unlimited}).Risk
Low. The only production change is a 4-line call-site swap + 1 new private method that is byte-for-byte equivalent to the two blocks it replaces. No public API surface changed.
Linked
Closes the #720 follow-up tracked in #716's review audit.
Made with Cursor