Skip to content

feat: wire V10 Publishing Conviction NFT through SDK + daemon#527

Merged
zsculac merged 48 commits into
feat/archive-non-v10-contractsfrom
feat/wire-v10-pca-nft
May 15, 2026
Merged

feat: wire V10 Publishing Conviction NFT through SDK + daemon#527
zsculac merged 48 commits into
feat/archive-non-v10-contractsfrom
feat/wire-v10-pca-nft

Conversation

@zsculac
Copy link
Copy Markdown
Contributor

@zsculac zsculac commented May 15, 2026

Summary

Wires the V10 DKGPublishingConvictionNFT write+read surface end-to-end through the SDK so operators can create, fund, and manage Publishing Conviction Accounts via the chain-adapter, the DKGAgent facade, and the daemon HTTP API. POST /api/pca and the funds/agent/settle routes now return 200 with real tx hashes instead of 503.

Closes #519. Sibling cleanup tracked in #520.

Stacked on #500 (feat/archive-non-v10-contracts) — base of this PR is that branch, not main. #519's "drop the V9 null stubs" only makes sense on top of #500's archive. #500 is still open (base main); rebase this onto main once #500 merges.

Why

The V10 PCA NFT contract is deployed and invoked by KnowledgeAssetsV10.publish() on-chain, but the SDK exposed only read shims — all PCA write methods returned null, so the daemon /api/pca/* routes returned HTTP 503. The V9 PublishingConvictionAccount predecessor was archived in #500. This PR closes the SDK gap (the follow-up ADR-0001 explicitly named).

What changed (transitive — every consumer in one PR, the #500 lesson)

  • chain-adapter (packages/chain/src): new V10 interface surface — createConvictionAccount(committedTRAC), topUpConvictionAccount, registerConvictionAgent, deregisterConvictionAgent, isConvictionAgent, settleConvictionAccount, V10 12-tuple getConvictionAccountInfo. EVM impl + mock-adapter / no-chain-adapter parity. Dead V9 publishingConvictionAccount cache slot removed.
  • DKGAgent facade (packages/agent/src/dkg-agent.ts): five return null PCA stubs deleted, delegate to the new adapter methods, lockEpochs dropped, key→agent terminology, callers fixed.
  • daemon (packages/cli/src/daemon/routes/pca.ts): V10-shaped serializeAccountInfo; POST /api/pca body drops lockEpochs; :id/authorize replaced by POST :id/agent + DELETE :id/agent/:address; POST :id/settle added; :id/fundstopUp; owner revert → 403.
  • api-client / CLI (packages/cli/src/api-client.ts): V10 request/response shapes; registerPcaAgent/deregisterPcaAgent/settlePca methods + commands.
  • tests: packages/evm-module/test/v10-pca-lifecycle.test.ts (create → topUp → registerAgent → discounted publish via real KnowledgeAssetsV10.publish() → expiry revert); chain adapter + mock↔EVM parity; daemon route round-trip; devnet HTTP smoke.

V9 → V10 semantic break (DTOs changed shape — not a rename)

Concern V9 (archived) V10 (wired)
Lock duration per-account lockEpochs arg global protocol param; no caller arg
Authorization authorizedKeys + admin registerAgent + agentToAccountId (1 account/agent)
Ownership admin field ERC-721 ownerOf(accountId)
Funding addFunds → balance topUp → persistent topUpBalance
getAccountInfo 6-tuple 12-tuple (owner, committedTRAC, topUpBuffer, expiresAtEpoch, agentCount, …)

Breaking HTTP contract is acceptable: the V9 routes only ever returned 503 on V10 deployments — no working client to break. The same reasoning covers the ApiClient TypeScript surface (createPca drops lockEpochs; PCA probe field authorizedregistered; method renames): an intentional, coordinated break for the 10.0.0-rc major — V9 is archived in stacked base #500, so a deprecated shim would only call a removed contract. Broader V9 SDK excision is tracked by #520.

Owner-gating (curation trust model)

createConvictionAccount mints to the signer; topUp/register/deregister are owner-only on chain (msg.sender == ownerOf(accountId)). The SDK surfaces the NotAccountOwner revert (not swallowed); the daemon maps it to 403 (distinct from 503 = no-chain). Agents publish only.

Verification

Built via 8 tracer-bullet slices (one commit-set each, TDD red→green, independent reviewer gating each slice — 37 commits). Reviewer caught and forced fixes for real defects, notably:

  • mock↔EVM parity gaps (InvalidAmount/ZeroAgentAddress/baseEpochAllowance)
  • api-client/CLI callers still V9-shaped (lockEpochs leak)
  • lifecycle test used a Hub-rewired shortcut instead of real KnowledgeAssetsV10.publish()
  • devnet smoke could pass on stale chain state — now force-boots a clean devnet and asserts 0 < discountedCost < baseCost on chain (guards the silent-demotion risk: KAv10 takes the discount branch only when publishEpochs == lockDurationEpochs)

Gates executed in-run (per slice, reviewer-gated): pnpm -r build; pnpm --filter @origintrail-official/dkg-chain test; pnpm --filter @origintrail-official/dkg-evm-module exec hardhat test; daemon route tests; clean-devnet HTTP /api/pca round-trip with on-chain discount assertion. CI (Tornado lanes) is the remaining gate this PR triggers.

Out of scope (followups)

  • Excise V9 SDK scaffolding from chain-adapter / dkg-agent / daemon routes #520 — broader V9 SDK scaffolding excision (legacy ABIs, V9-touching evm-adapter methods, non-PCA interface cleanup).
  • DKGStakingConvictionNFT SDK surface (createConviction/claim) — separate feature.
  • On-chain Hub deregistration of V9 keys on existing deployments — ops task.
  • V10-only reward-flywheel coverage test — separate add.

🤖 Generated with Claude Code


Post-review cleanup (2026-05-15)

Addressed reviewer feedback:

  • Stripped non-feature artifacts (25ca4530): removed .devnet/run.mjs + pca-smoke-lib* and .scratch/issue-519/verify.md from the PR, reverted the .gitignore negation hack that force-tracked them, and added .scratch/ to .gitignore. The devnet smoke is verification scaffolding, not feature code — it runs locally; evidence lives here in the PR body. (Root cause: the orchestrator test_command was cd .devnet && node run.mjs, which forced the harness into the tree.)
  • Trimmed oversized comments (1bd98146): pca.ts header 23→4 lines; chain-adapter.ts + dkg-agent.ts JSDoc condensed (−43 net comment lines). Kept the load-bearing why (owner-revert→403, no-chain→503); prose moved to ARCHITECTURE.md § #519.

Net PR diff is now packages/* (feature) + .gitignore (policy) + ARCHITECTURE.md only. pnpm -r build green.

Independent devnet runtime verification

Booted a clean 2-node devnet and drove the full operator path through the live daemon (not the in-run subagent — independently re-run end-to-end):

Step Status Detail
POST /api/pca 200 accountId=1 (V10 conviction NFT minted to daemon EOA)
POST /api/pca/:id/agent 200 3/3 publisher wallets registered
publish KC as agent ok kcId=1
publish signer bound ok publish signer is a registered agent of account 1
GET /api/pca/:id 200 agentCount=3, discountBps=5000, topUpBuffer=0
on-chain discount ok CostCovered emitted for account 1; discountedCost == baseCost·(10000−discountBps)/10000 (exact on-chain tier formula)

The CostCovered event firing from DKGPublishingConvictionNFT for the account proves the publish routed through coverPublishingCost (a demoted/direct-spend publish emits none). Devnet zeroes token economics (base cost = 1 wei), so the discounted value floors to 0 — the assertion verifies the discount formula was applied exactly, robust to dust-sized base costs.

Known blocker — deferred to #500

CI is green except one job: Tornado: Solidity coverage (push safety net) — ratchet failure (lines 54.84% < 60%, branches 45.38% < 48%, functions 54.85% < 65%).

This is not introduced by #519. It is inherited from the base branch: #500 (feat/archive-non-v10-contracts) archived ~14 kLOC of V8/V9 Solidity tests, dropping evm-module coverage below the repo ratchet. #519 only adds tests (it cannot raise coverage above what #500 removed). Evidence the wiring itself is sound:

  • EVM-Integration matrix: success (3/3)
  • 22/24 CI jobs: success
  • Independent devnet runtime: 6/6 PASS (create PCA → register agent → publish via PCA → CostCovered on chain → discount per exact tier formula → GET round-trip)

Resolution owner: #500. The coverage ratchet (lower thresholds to the post-archive reality, or add V10 contract tests) belongs in the archive PR's scope, not this feature PR. #519 rebases onto main once #500 lands with coverage resolved. Tracked via a comment on #500.

Post-review round 2 (2026-05-15)

  • Naming disambiguation (c7283678): the V10 SDK methods were renamed *ConvictionAccount/*ConvictionAgent*PublishingConviction* (createPublishingConvictionAccount, topUpPublishingConvictionAccount, registerPublishingConvictionAgent, deregisterPublishingConvictionAgent, isPublishingConvictionAgent, settlePublishingConvictionAccount, getPublishingConvictionAccountInfo, type V10PublishingConvictionAccountInfo). "Conviction account" is ambiguous in this codebase — there are two: publishing (DKGPublishingConvictionNFT, this PR) and staking (DKGStakingConvictionNFT). The V9 names carried the Publishing qualifier for exactly this reason; the bare names were a regression I introduced in the spec. Applied transitively (interface, evm/mock/no-chain impls, facade, daemon routes, api-client, tests, ARCHITECTURE.md); bare Conviction names returned to the v8-v9-archive guard as V9-only.
  • Comments capped at ≤2 lines (c7283678): every multi-line V10 PCA comment block trimmed; load-bearing why kept, prose in ARCHITECTURE.md § SDK + daemon PCA write surface is wired to archived V9 contract; V10 PCA NFT has no SDK path #519.

Verification after rename: pnpm -r build green; chain (352✓) / agent (461✓) / cli (1117✓) suites green; devnet HTTP round-trip re-verified 6/6 PASS (HTTP API surface unchanged — rename is internal). Note: the two pre-existing read methods getConvictionAgentAccountId / getConvictionAccountLockDurationEpochs are on the base branch (not introduced here) and left as-is to avoid scope creep — minor pre-existing naming debt.

Post-review round 3 — codex findings addressed (c5762e2d)

Codex finding Fix
🔴 NFT-undeployed → HTTP 500 (write) / 404 (read) instead of 503 A — typed PcaUnavailableError (code PCA_UNAVAILABLE); evm-adapter throws it on write (requireConvictionNFT) and read (getPublishingConvictionAccountInfo) when the NFT is absent; daemon maps → 503. null now means account-missing only → 404. No more 500/404 on pre-V10 hubs.
🔴 Known PCA reverts → opaque 500 CInvalidAmount→400, Agent{Already,Not}Registered→409, AccountExpired→409 across all 5 write handlers.
🟡 api-client probe field authorized (V9 wording) D — renamed authorizedregistered (route + client type + tests + a stale cli.ts ref).
🟡 Mock hard-codes discountBps=0 / lifecycle fields BMockChainAdapter mirrors the contract's exact getDiscountBps tier ladder; persists createdAtEpoch, derives expiresAtEpoch. Settlement cursor / fullySwept deliberately not modeled in the mock (that is contract accounting — covered by the evm-module hardhat suite + the devnet smoke against the real NFT); documented in-code.

Verification at c5762e2d: pnpm -r build ✓; chain 360✓ / cli 1123✓ suites green; devnet HTTP round-trip re-verified 6/6 PASS (happy path unaffected — A/C/D are error-path/serialization, B is mock-only).

Post-review round 4 — codex round-2 findings addressed (351c2614)

Codex finding Fix
🔴 classifyPcaRevert incomplete (ZeroAgentAddress / AgentCapReached → 500) Completed the mapping from the contract's full error set (DKGPublishingConvictionNFT.sol:164-179): ZeroAgentAddress→400, TokenTransferFailed→400, AgentCapReached→409, AccountAlreadyFullySettled→409. Also a fast zero-address reject (400, pre-RPC) in POST + DELETE /api/pca/:id/agent.
🟡 Mock doesn't enforce maxAgentsPerAccount MockChainAdapter enforces the contract default cap 100 (mirrors :208/:711-712), throws AgentCapReached past the cap so mock-backed tests catch the regression.

Enumerated the contract's full custom-error set to end the incremental review loop (covered 4 reachable errors, not just the 2 flagged). TDD: each behavior driven red→green (6 cli route tests, 3 mock tests). Verification at 351c2614: pnpm -r build ✓; chain 363 / cli 1129 suites green; devnet HTTP round-trip re-verified 6/6 PASS.

Zvonimir and others added 30 commits May 15, 2026 10:54
Add V10ConvictionAccountInfo + the seven optional V10 PCA signatures to
the ChainAdapter interface (replacing the V9 ConvictionAccountInfo DTO),
and implement createConvictionAccount + getConvictionAccountInfo on
EVMChainAdapter against DKGPublishingConvictionNFT.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ConvictionAgent

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner-gated topUp/register revert propagates (not swallowed) so the
daemon can map NotAccountOwner to HTTP 403.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… reconcile guards

- Remove the unused legacy V9 `publishingConvictionAccount` Contract
  cache slot + its init resolve (DKGPublishingConvictionNFT is the only
  PCA contract on a V10 deploy); existing V10 read methods preserved.
- v8-v9-archive guard: stop guarding `createConvictionAccount` and
  `getConvictionAccountInfo` — those names are reclaimed by the V10
  DKGPublishingConvictionNFT surface (PRD §6).
- mock-adapter-parity: exempt the seven V10 PCA methods + the private
  requireConvictionNFT helper; TB-0002 mirrors them on the mock and
  removes the exemptions.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
In-memory account map with incrementing id and owner = signer; the V10
read shape mirrors DKGPublishingConvictionNFT.getAccountInfo.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nt/isConvictionAgent

Bidirectional agent map mirrors agentToAccountId; re-registration throws
AgentAlreadyRegistered for N28 parity.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
topUp accumulates the persistent buffer; settle is a permissionless
no-op sweep on the mock.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Registered)

topUp/register/deregister are owner-gated; the owner revert is surfaced,
never swallowed (daemon maps it to 403). settle stays permissionless.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
All seven DKGPublishingConvictionNFT methods reject via the shared
noChain() helper instead of returning a half-set 0n/null.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop the seven TB-0001 exemptions now that MockChainAdapter mirrors the
DKGPublishingConvictionNFT write+read surface; full chain suite green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ress/baseEpochAllowance)

createConvictionAccount/topUp reject zero, negative and uint96-overflow
amounts (InvalidAmount); registerConvictionAgent rejects the zero
address (ZeroAgentAddress); getAccountInfo.baseEpochAllowance is
committedTRAC / lockDurationEpochs — matching DKGPublishingConvictionNFT.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Delete the five return-null V9 PCA stubs (createPublishingConvictionAccount,
addPublishingConvictionAccountFunds, addPCAAuthorizedKey, isPCAAuthorizedKey,
getPublishingConvictionAccountInfo) and replace them with thin delegating
wrappers over the V10 chain-adapter surface: createConvictionAccount (no
lockEpochs), topUpConvictionAccount, registerConvictionAgent,
deregisterConvictionAgent, isConvictionAgent, settleConvictionAccount,
getConvictionAccountInfo. Owner reverts propagate (not swallowed); a null
return means the adapter has no V10 PCA surface.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Update the only live caller of the deleted V9 PCA facade names so
`pnpm -r build` stays green: createConvictionAccount/topUpConvictionAccount/
registerConvictionAgent/isConvictionAgent/getConvictionAccountInfo, TxResult
.hash field, and a V10 serializeAccountInfo shape. Route paths/semantics
(authorize, settle, owner-revert 403) are rewritten in TB-0004.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address review blockers:
- POST /api/pca no longer requires/echoes the removed V9 lockEpochs
  field; a V10 body with only { tokens } now returns 200, not 400.
- Owner-gated register/top-up reverts (NotAccountOwner from the V10
  contract and MockChainAdapter) now map to HTTP 403 via a shared
  isOwnerRevert() helper instead of falling through as HTTP 500.

Adds packages/cli/test/daemon-pca-routes.test.ts covering both.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
NoChainAdapter implements the V10 PCA methods but throws noChain()
rather than returning null, so no-chain/pre-chain daemons were
regressing to HTTP 500 instead of the documented 503 unavailable
contract. Add isNoChain() and short-circuit every PCA route catch
(create/authorize/funds/info) to 503 before the generic 500.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The /api/pca rewire dropped lockEpochs and now returns the V10
getAccountInfo shape. Update the operator caller surface to match:
- createPca request/response drops lockEpochs; `pca create` no longer
  takes --epochs or prints lockEpochs.
- getPcaInfo response and `pca info` print the V10 fields (owner,
  committedTRAC, topUpBuffer, baseEpochAllowance, epoch window,
  agentCount, lastSettledWindow, fullySwept, discountBps).
- admin→owner / authorizedKeys→conviction-agent wording.

Calibrated via tsc: api-client V10 types first (build RED on the old
cli.ts field reads), then cli.ts updated (build GREEN).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
POST /api/pca/:id/agent registers a publishing agent against the V10
DKGPublishingConvictionNFT, owner-gated (NotAccountOwner -> 403).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner-gated V10 agent deregistration; NotAccountOwner -> 403,
no-chain -> 503.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Permissionless V10 lazy-settlement sweep; no-chain -> 503.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the V9 PublishingConvictionAccount narrative with the V10
DKGPublishingConvictionNFT contract description and the full owner-gating
/ permissionless-settle route contract; drop stale authorizedKeys and
V10.1 wording.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Locks acceptance criterion #2/#4: a stateful agent round-trip proves
GET /api/pca/:id surfaces agentCount and the updated topUpBuffer in the
V10 serializeAccountInfo shape.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
api-client.ts authorizePcaKey (POST /api/pca/:id/authorize, { key })
is a V9 PCA DTO. Replace with registerPcaAgent (POST /api/pca/:id/agent,
{ agent }) matching the V10 daemon route contract, and rewire the
`dkg pca` command (authorize -> register-agent).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Maps to DELETE /api/pca/:id/agent/:address. Adds a private del()
helper (no DELETE verb existed) and the `dkg pca deregister-agent`
command.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Maps to the permissionless POST /api/pca/:id/settle route and the
`dkg pca settle` command.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e lifecycle

Standalone on-chain assertions for the DKGPublishingConvictionNFT
lifecycle: createAccount mints + records the discount tier, topUp is
owner-gated and leaves committedTRAC/expiry untouched, register/
deregister maintain the agent reverse map + agentCount, and settle
advances the lazy-settlement cursor then marks the account fully
swept post-expiry.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ish()

Registers an agent, publishes through the real KAV10.publish() with
p.epochs == lockDurationEpochs, and asserts on chain that the staker-
pool distribution equals the DISCOUNTED cost (tokenAmount * (1 -
discountBps/1e4)) while the KC still records the full tokenAmount —
proving the conviction discount branch executed, not the direct-spend
fallthrough.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
publish() gates the discount off and falls through to direct spend on
an expired PCA (no brick). The expiry revert lives in the conviction
funding entrypoint coverPublishingCost; drive it via an EOA standing
in for KnowledgeAssetsV10 and assert AccountExpired once block.timestamp
passes expiresAtTimestamp.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Review blocker: the expired-account criterion requires a real
publish() attempt, not a direct coverPublishingCost() call against a
Hub-rewired EOA. Rewrite test 3 to advance past expiresAtTimestamp
and call the real KAV10.publish(): the conviction discount is gated
off, publish() falls through to the direct-spend branch, _addTokens
reverts TooLowAllowance (the registered agent was only funded for the
up-front committedTRAC, never approved KAV10 for a direct spend), and
no KC is minted (atomic rollback). The same unfunded agent publishing
the same params pre-expiry succeeds via the NFT-funded discount branch
(test 2), so the revert is expiry-driven. Shared publisher/agent/CG
setup extracted into setupRegisteredAgentPublish().

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Issue #519 TB-0007 gate 5. The PCA HTTP smoke must assert the
discounted cost on chain, not just an HTTP 200 — KnowledgeAssetsV10
silently demotes to the no-discount branch when the publishing wallet
is not a registered agent / epochs != lockDurationEpochs. Add the pure
assertion helper + un-ignore the .devnet smoke entrypoint trio so the
literal test_command can reach .devnet/run.mjs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Issue #519 TB-0007 acceptance criterion 3 — the scripted PCA flow must
write round-trip evidence to .scratch/issue-519/verify.md. Pure table
builder, one row per step, PASS/FAIL verdict.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Zvonimir and others added 4 commits May 15, 2026 13:24
Review blocker 2: the smoke registered only publisher-wallets[0], but
the daemon publisher rotates operational wallets, so the wallet that
actually signed the publish tx may not be a registered agent —
KnowledgeAssetsV10 then silently demotes to the no-discount branch and
the failure surfaces as an opaque discount-math error. This pure helper
checks the publish tx signer's on-chain agentToAccountId against the
smoke's account and reports a distinct, actionable failure.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses both review blockers:

1. ensureDevnetLive no longer reuses an already-green devnet — it
   ALWAYS stop+wipe+boots a clean devnet, so the on-chain
   agentToAccountId map and the daemon's publish-signer rotation start
   from a deterministic empty state (no AgentAlreadyRegistered on
   re-run, no drifted signer).
2. readNode1 now returns the full operational+publisher wallet union;
   the smoke registers every candidate idempotently via
   classifyAgentRegistration (skips wallets already bound to this
   account, hard-fails on a foreign binding) and, after publish, binds
   the actual tx signer (receipt.from) to the account via
   assertPublishSignerBound BEFORE the discount math — a silent
   demotion now fails with an actionable message instead of an opaque
   discount-math error.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reflect the forced clean restart, idempotent multi-wallet registration,
and publish-signer binding; unit suite now 12/12.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Post-wiring architecture section: call path, ChainAdapter V10 PCA
surface, the V9->V10 semantic break, owner-gating, the daemon HTTP
contract, and the clean-devnet on-chain discount assertion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread packages/cli/src/daemon/routes/pca.ts
Comment thread packages/chain/src/mock-adapter.ts Outdated
Zvonimir and others added 2 commits May 15, 2026 14:06
The PCA devnet smoke (.devnet/run.mjs + pca-smoke-lib*) and the
.scratch/issue-519 verify note are verification scaffolding, not feature
code — they do not belong in the PR. Reverts the .gitignore negation
hack that force-tracked them. Runtime verification is done locally and
the evidence lives in the PR description instead. Also gitignores
.scratch/ so local scratch can't leak into a PR again.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pca.ts header 23→4 lines, chain-adapter + dkg-agent JSDoc condensed.
Keeps the load-bearing why (owner-revert→403, no-chain→503); drops
prose now covered by ARCHITECTURE.md § #519. Comment-only; build green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread packages/chain/src/mock-adapter.ts
Comment thread packages/cli/src/daemon/routes/pca.ts Outdated
Comment thread packages/cli/src/api-client.ts Outdated
Review feedback:

1. Naming: `*ConvictionAccount`/`*ConvictionAgent` were ambiguous with
   the Staking Conviction NFT (both are "conviction accounts"). Restore
   the explicit `*PublishingConviction*` qualifier the V9 names carried
   for exactly this disambiguation, across the chain-adapter interface,
   evm/mock/no-chain impls, the DKGAgent facade, daemon routes,
   api-client, tests, and ARCHITECTURE.md. The bare V9 `Conviction`
   names go back into the v8-v9-archive guard (V9-only again).
2. Comments: every multi-line V10 PCA comment block trimmed to <=2
   lines (load-bearing why kept; prose lives in ARCHITECTURE.md § #519).

Build green; chain/agent/cli suites green; devnet HTTP round-trip
re-verified 6/6 PASS post-rename (HTTP API unchanged).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread packages/chain/src/evm-adapter.ts Outdated
Comment thread packages/chain/src/evm-adapter.ts Outdated
Comment thread packages/chain/src/mock-adapter.ts Outdated
…rity

Addresses codex review on #527:

- A: EVMChainAdapter throws typed PcaUnavailableError when
  DKGPublishingConvictionNFT is undeployed (write + read paths);
  daemon maps it → 503. getPublishingConvictionAccountInfo throws
  for undeployed, null only for account-missing (→ 404), removing
  the old 500/404 ambiguity on pre-V10 hubs.
- C: deterministic PCA reverts now map to 4xx — InvalidAmount→400,
  Agent{Already,Not}Registered→409, AccountExpired→409 — across all
  write handlers (was opaque 500).
- D: api-client probe field authorized→registered (V10 agent
  terminology; drops retired V9 "authorized key" wording).
- B (mock parity): MockChainAdapter computes the exact contract
  getDiscountBps tier ladder + persists createdAtEpoch / derives
  expiresAtEpoch. Settlement cursor / fullySwept deliberately not
  modeled (covered by hardhat + devnet against the real NFT).

Build green; chain 360 / cli 1123 suites green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zsculac
Copy link
Copy Markdown
Contributor Author

zsculac commented May 15, 2026

Codex review findings addressed in c5762e2d (dedup: 8 comments → 4 issues):

  • NFT-undeployed → 500/404 instead of 503 (the pca.ts + evm-adapter.ts comments, both passes): fixed via a typed PcaUnavailableError thrown by the adapter on write+read paths and mapped to 503 by the daemon; null is now reserved for genuine account-missing (→404). Resolves the pre-V10-hub footgun.
  • Known PCA reverts → opaque 500: InvalidAmount→400, Agent{Already,Not}Registered→409, AccountExpired→409 across all write handlers.
  • api-client authorized: renamed to registered (V10 agent terminology).
  • Mock discountBps/lifecycle hard-coded: mock now mirrors the contract's exact getDiscountBps tier ladder + persists creation/expiry metadata. Lazy-settlement cursor / fullySwept are intentionally not re-implemented in the mock — that is on-chain accounting, verified by the evm-module hardhat suite and the devnet smoke against the real DKGPublishingConvictionNFT; faithfully duplicating it in a mock would be reimplementing the contract.

Verified: build + chain (360) / cli (1123) suites green; devnet HTTP /api/pca round-trip re-verified 6/6 PASS. The remaining red CI lane is the Solidity coverage ratchet, which is inherited from #500's V8/V9 test archival (out of scope here — flagged on #500).

Comment thread packages/cli/src/daemon/routes/pca.ts
Comment thread packages/chain/src/mock-adapter.ts
- classifyPcaRevert now covers all route-reachable deterministic PCA
  errors: ZeroAgentAddress→400, TokenTransferFailed→400,
  AgentCapReached→409, AccountAlreadyFullySettled→409 (was 500).
  Enumerated from DKGPublishingConvictionNFT.sol:164-179 to end the
  incremental review loop.
- Fast zero-address reject (400) in POST and DELETE /api/pca/:id/agent
  before any RPC (ethers.isAddress accepts 0x00..0).
- MockChainAdapter enforces maxAgentsPerAccount (default 100, mirrors
  contract :208/:711-712), throws AgentCapReached past the cap so
  mock-backed tests catch the regression.

TDD: each behavior driven red→green (6 cli route tests, 3 mock tests).
Build green; chain 363 / cli 1129 suites green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zsculac
Copy link
Copy Markdown
Contributor Author

zsculac commented May 15, 2026

Codex round-2 findings addressed in 351c2614:

  • classifyPcaRevert incomplete: rather than only the 2 flagged errors, I enumerated the contract's full custom-error set (DKGPublishingConvictionNFT.sol:164-179) and mapped every route-reachable deterministic one — ZeroAgentAddress→400, TokenTransferFailed→400, AgentCapReached→409, AccountAlreadyFullySettled→409 (was 500). Also added a fast zero-address reject (400, before any RPC) in both POST and DELETE /api/pca/:id/agent, since ethers.isAddress('0x0..0') is true.
  • Mock agent cap: MockChainAdapter now enforces the contract default maxAgentsPerAccount = 100 (mirrors :208 / revert at :711-712) and throws an AgentCapReached-shaped error past the cap, so mock-backed tests catch the regression. (Settlement accounting remains intentionally out of the mock — that is on-chain logic verified by the hardhat suite + devnet smoke.)

Built test-first (TDD, red→green per behavior): 6 new daemon-route tests, 3 new mock tests. Verified: pnpm -r build green; chain (363) / cli (1129) suites green; devnet /api/pca round-trip re-verified 6/6 PASS. Remaining red CI lane is still only the #500-inherited Solidity coverage ratchet (out of scope here; flagged on #500).

Comment thread packages/chain/src/no-chain-adapter.ts Outdated
Comment thread packages/chain/src/mock-adapter.ts
codex round-3:
- NoChainAdapter no longer defines the 7 optional #519 PCA methods, so
  the DKGAgent facade's `typeof` guard returns the documented `null`
  (was: throwing noChain() to direct SDK callers in no-chain mode).
  Daemon still 503 (facade null → existing path); GET capability probe
  yields 503 not 404. Interface marks them optional so omission is valid.
- Mock: lastSettledWindow/fullySwept marked STATIC STUBS and
  settlePublishingConvictionAccount a DELIBERATE NO-OP. Settlement is
  intentionally out of mock-parity scope (contract accounting, verified
  on-chain by hardhat + devnet) — honest scoping, not emulation. Pinning
  test locks the intentional stub so it can't silently half-model.

TDD red→green. Build green; chain 365 / cli 1130 suites green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zsculac
Copy link
Copy Markdown
Contributor Author

zsculac commented May 15, 2026

Codex round-3 addressed in f2ac30fb:

  • NoChainAdapter broke the facade null-contract: the 7 SDK + daemon PCA write surface is wired to archived V9 contract; V10 PCA NFT has no SDK path #519 PCA methods are optional on ChainAdapter, but NoChainAdapter defined them (throwing noChain()), so the facade's typeof guard passed and a direct SDK caller in no-chain mode got an exception instead of the documented null. Fixed by removing those 7 stubs from NoChainAdapter (omission is valid for optional iface members) → facade returns null (SDK contract restored), daemon write routes still 503 via the existing result===null path, and the GET capability probe yields 503 (not 404). getPublishingConvictionAccountOwner is pre-existing, left as-is.
  • Mock settlement parity overclaim: taking the reviewer's second option — lastSettledWindow/fullySwept are now explicitly marked STATIC STUBS and settlePublishingConvictionAccount a DELIBERATE NO-OP, with a pinning test locking the intentional scope. Settlement is contract accounting, verified on-chain (hardhat + devnet smoke); deliberately not emulated in a mock.

TDD red→green (facade-null contract test + no-chain GET 503 test + mock stub-pin test). Build green; chain 365 / cli 1130 suites green. Skipped a devnet re-run by design — these changes touch only the no-chain/mock adapters + comments; the devnet path uses the EVM adapter (unchanged, verified at the prior SHA). Only remaining red CI lane is still the #500-inherited coverage ratchet.

Comment thread packages/chain/src/mock-adapter.ts
Comment thread packages/cli/src/daemon/routes/pca.ts
…round 4)

- classifyPcaRevert: map OZ v5 ERC721NonexistentToken (+ legacy string
  fallback) to 404 UnknownAccount, so topUp/register/deregister/settle
  return a client error for an unminted PCA id instead of HTTP 500
  (GET already 404s — writes are now consistent). +3 route tests.
- mock-adapter: correct two comments that overclaimed contract parity;
  the mock is boundary-aligned (no wall clock) so it does not model the
  contract's mid-epoch expiry round-up. Comment-only, zero logic change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zsculac
Copy link
Copy Markdown
Contributor Author

zsculac commented May 15, 2026

Codex review round 4 — addressed in c41c4c7c

1. 🔴 mock expiresAtEpoch = createdAtEpoch + lockDurationEpochs bakes in the wrong invariant
The mock has no wall clock — createdAtEpoch is a synthetic monotonic counter, so every mock account is boundary-aligned by construction. At an epoch boundary the contract (expiresAtEpoch = epochAtTimestamp(expiresAtTimestamp-1)+1) also yields exactly createdAtEpoch+lockDurationEpochs; the +1 divergence the bot describes only occurs for mid-epoch creation, which the mock cannot represent (timestamps are deliberately 0). Modeling the round-up would require inventing a fake mid-epoch offset — pure fiction with no on-chain backing. The arithmetic is correct for the mock's model; the defect was two comments that overclaimed contract parity. Both rewritten to state the boundary-aligned simplification honestly and explicitly disclaim the contract's mid-epoch round-up. Zero logic change; the parity test (line 41) is already correctly named "internally consistent" and is untouched. (Same honest-scoping principle accepted in round 3.)

2. 🟡 write routes return 500 for an unknown accountId
Correct and worth fixing — topUp/register/deregister/settle route through OZ v5 _requireOwned/ownerOf, which reverts ERC721NonexistentToken for an unminted id; that was unclassified and fell through to 500, while GET already returns 404. Added one branch to classifyPcaRevert mapping ERC721NonexistentToken (plus a legacy string-revert fallback) to 404 UnknownAccount — one change fixes all four write routes since they share the helper. +3 route tests pin the consistency.

Verification: pnpm -r build clean; cli suite 1133 passed (+3 new), chain suite 365 passed (unchanged — comment-only), per-package tsc --noEmit clean. CI + EVM Integration re-dispatched on c41c4c7c (heavy matrix is dormant on a feature-branch base, so it runs via workflow_dispatch). Devnet smoke not re-run: this round touches only the cli error-mapping helper and mock comments — the EVM adapter / on-chain path is byte-identical to the SHA already verified 6/6.

Comment thread packages/cli/src/daemon/routes/pca.ts Outdated
Comment thread packages/cli/src/api-client.ts
Comment thread packages/cli/src/daemon/routes/pca.ts Outdated
Zvonimir and others added 4 commits May 15, 2026 17:37
…l (codex round 5)

- register route: wrap the post-write isPublishingConvictionAgent probe in
  its own try/catch so a probe failure can never turn a mined registration
  tx into HTTP 500 (caller would retry → AgentAlreadyRegistered). Response
  is now 200 + txHash whenever the tx mined; registered/adapterSupported
  is tri-state, matching the GET route's probedKey shape.
- DKGAgent: add public supportsPublishingConvictionNft getter; GET route
  uses it for 404-vs-503 instead of casting into the private chain field.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ound 6)

PCA write methods rethrew raw ethers CALL_EXCEPTIONs; providers report
custom errors as opaque "unknown custom error"+data, so the daemon's
message-based classifier never saw NotAccountOwner/InvalidAmount/ERC721-
NonexistentToken and returned 500 instead of 403/4xx. Wrap the 5 writes
in a pcaWrite() helper that runs enrichEvmError() on throw then rethrows,
mirroring isContractMissingRevert/translateRandomSamplingError.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zsculac zsculac merged commit 5ea5440 into feat/archive-non-v10-contracts May 15, 2026
27 of 28 checks passed
@zsculac
Copy link
Copy Markdown
Contributor Author

zsculac commented May 15, 2026

Codex review rounds 5 & 6 — addressed (pushed: 62f9e5eb158fce9f)

Round 5 #1 🔴 — register read-after-write could turn a mined tx into 500/registered:false (62f9e5eb)
The verification probe (isPublishingConvictionAgent) is now wrapped in its own try/catch, so a probe throw can no longer escape to the handler's outer catch and 500 a registration that already mined (which made callers retry → AgentAlreadyRegistered). Once the tx mines the response is an authoritative 200 + txHash; registered/adapterSupported is tri-state, mirroring the GET route's probedKey shape.

Round 5 #2 🔴 — api-client drops lockEpochs / renames auth methods → breaks SDK consumersintentional, documented, no code shim
This is a deliberate coordinated break for the 10.0.0-rc major. lockEpochs is now a global protocol param with no contract path; V9 is archived in stacked base #500, so a deprecated alias would only call a removed contract (dead code). No CHANGELOG file exists in the repo. The PR body's "V9 → V10 semantic break" section now explicitly covers the ApiClient TS surface as well, and the broader V9 SDK excision is tracked by #520. Adding non-functional shims was rejected as worse than the documented break.

Round 5 #3 🟡 — GET 404-vs-503 reached into (agent as any).chain (private internal) (62f9e5eb)
Added a public get supportsPublishingConvictionNft() on DKGAgent (mirrors the existing in-facade typeof this.chain.X === 'function' idiom used ~20×). The route now uses it; the single (agent as any) cast is gone, so a future facade refactor can't silently misclassify 503→404.

Round 6 🔴 — PCA EVM writes rethrew raw CALL_EXCEPTION, so custom errors fell through to 500 (5ffb379e)
Correct and impactful: providers surface custom-error reverts as opaque unknown custom error+data=0x…, so err.message carried no name and the daemon's message-based classifier never matched NotAccountOwner/InvalidAmount/ERC721NonexistentToken → HTTP 500 instead of 403/4xx. The owner-happy-path devnet smoke never reverted, and the unit tests fed synthetic message strings, so neither caught it. Fix: a pcaWrite() helper wraps all 5 PCA writes and runs enrichEvmError() on throw before rethrow — mirroring the file's existing isContractMissingRevert/translateRandomSamplingError pattern (DKGPublishingConvictionNFT is already in ERROR_ABI_CONTRACTS, so every PCA + inherited OZ error decodes). This also makes the round-4 ERC721NonexistentToken → 404 mapping actually work on the live chain.

Verification: pnpm -r build clean; chain 368 passed (+3 TDD), cli 1136 passed (+3 TDD), agent 463 passed; per-package tsc --noEmit clean. CI + EVM Integration re-dispatched on 158fce9f (heavy matrix is dormant on a feature-branch base → workflow_dispatch). The only red CI job is the pre-existing Tornado: Solidity coverage (push safety net) ratchet inherited from #500 (it archived ~14k LOC of V8/V9 tests) — no Solidity changed in this PR; deferred to #500 as previously agreed and flagged there.

// committedTRAC) — the signer must allow the NFT to pull the TRAC.
if (this.contracts.token) {
const allowance: bigint = await this.contracts.token.allowance(this.signer.address, nftAddress);
if (allowance < committedTRAC) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: approve() happens before we validate that the requested amount fits the contract's uint96 parameter. If a caller passes something larger than 2^96-1, the approval tx will still go through and then createAccount/topUp reverts with InvalidAmount, leaving an unintended unlimited allowance behind. Please reject out-of-range values before the allowance branch here and in topUpPublishingConvictionAccount() below.

authorized: verified === true,
txHash: result.txHash,
agent: agentAddr,
registered: verified === true,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: this reports registered: false whenever the post-write probe is unavailable or transiently fails, even though the tx already mined successfully. That makes the HTTP contract contradict the actual write result and can trigger pointless retries that then hit AgentAlreadyRegistered. The success field should stay true after a successful tx, with probe/verification exposed separately.

async topUpPublishingConvictionAccount(accountId: bigint, amount: bigint): Promise<TxResult> {
const acct = this.requireConvictionOwner(accountId);
this.requireValidConvictionAmount(amount);
acct.topUpBuffer += amount;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: the mock only validates each individual top-up amount, not the accumulated topUpBuffer. On chain topUpBalance is uint96, so repeated top-ups past 2^96-1 revert; here they keep succeeding and diverge from EVM behavior. Add a cumulative overflow check before += and cover it with a parity test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant