You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to #1689 / PR #1971. Raised by otReviewAgentfour times across four review rounds, in four different framings — unify the signer admission model, move the policy out of the base adapter, extract an admission evaluator, centralize the partial helpers. Deferred each time in that PR for reasons specific to it; filing here so it is tracked rather than re-litigated per round.
The ask
packages/chain/src/evm-adapter-base.ts gained the #1689 publish-admission policy as a set of protected helpers on an already ~4k-line class: version capability detection, a version memo, _cgAdmitsAttestedAuthor (author-only), _cgPublishAdmits (payer+author), _publishAuthorizationRejection, plus two pool call sites with deliberately different orchestration.
Extract a focused admission component — constructed with the chain read dependencies plus contextGraphId and an optional attested author — exposing something like evaluatePinned() and eligiblePayers() / hasFundablePayer(). EVMChainAdapterBase would then orchestrate signer and funding selection and delegate the policy, instead of callers deciding when to pass or omit the author.
Why it was deferred in #1971, and what has to be preserved
Not disagreement with the shape — these are reasons the timing was wrong:
No CI on that branch. PRs based on testnet-canary run neither ci.yml (branch-filtered) nor the Solidity/Node matrices; packages/evm-module/** is not even in the one path-triggered workflow. A no-behaviour-change restructuring of an authorization path is exactly what you do not want unverified.
The code was mutation-proven as it stood — 8 chain-side mutations over 7 distinct properties plus 7 publisher-side, each killing exactly its intended test. Restructuring trades a verified arrangement for a better-shaped unverified one.
The no-author signer-pool rejection keeps its byte-identical legacy message (pinned by a source-text assertion).
Author-admitted pool returned unnarrowed.
Rejection carries the structured details it was actually evaluated from — never re-derived.
The non-obvious hazard
The refactor would take the deployed-version memo with it. That memo is keyed on the resolved lifecycle address, and the key choice is load-bearing: a Hub rotation changes the address, so the memo is invalidated for free and the gate self-heals with no listener and no extra wiring. Relocating it is therefore not a mechanical move — the key choice and its rationale must be carried across deliberately.
Suggested sequencing
Land on a branch where CI actually runs. Confirm the existing admission and NO_FUNDED enrichment tests pass unchanged (they are the behaviour contract), and re-run the mutation set afterwards rather than assuming a pure refactor preserved it.
Also worth folding in: a known perf item from the same PR — poolHasFundableSigner issues up to 2N isAuthorizedPublisher reads because those reads are not single-flighted (no CG-authorization read is a ReadThroughTtlCache consumer). The one-line fix is hoisting the author branch out of the per-wallet loop, which an extraction would make natural.
Follow-up to #1689 / PR #1971. Raised by
otReviewAgentfour times across four review rounds, in four different framings — unify the signer admission model, move the policy out of the base adapter, extract an admission evaluator, centralize the partial helpers. Deferred each time in that PR for reasons specific to it; filing here so it is tracked rather than re-litigated per round.The ask
packages/chain/src/evm-adapter-base.tsgained the #1689 publish-admission policy as a set of protected helpers on an already ~4k-line class: version capability detection, a version memo,_cgAdmitsAttestedAuthor(author-only),_cgPublishAdmits(payer+author),_publishAuthorizationRejection, plus two pool call sites with deliberately different orchestration.Extract a focused admission component — constructed with the chain read dependencies plus
contextGraphIdand an optional attested author — exposing something likeevaluatePinned()andeligiblePayers()/hasFundablePayer().EVMChainAdapterBasewould then orchestrate signer and funding selection and delegate the policy, instead of callers deciding when to pass or omit the author.Why it was deferred in #1971, and what has to be preserved
Not disagreement with the shape — these are reasons the timing was wrong:
testnet-canaryrun neitherci.yml(branch-filtered) nor the Solidity/Node matrices;packages/evm-module/**is not even in the one path-triggered workflow. A no-behaviour-change restructuring of an authorization path is exactly what you do not want unverified._authorizedPublisherSignershoists the author check to one read for the whole pool to avoid per-wallet read amplification (the Idle-RPC regression from RS wallet-rotation: uncached IdentityStorage resolution + per-selection revalidation (~+32k RPC req/day/node) #1583/store pressure from listGraphs and shared-memory slice SPARQL during sync/finalization #1549 class).poolHasFundableSignerroutes each wallet through the full predicate because it is a cold path — once per already-failed publish, 1-5 wallets — where uniformity was judged worth more than reads. Any extraction must keep both profiles rather than imposing one shape on both.Invariants any refactor must preserve
KnowledgeAssetsLifecycle.version() >= 10.1.7, failing closed on unreadable/unparseable/absent.ContextGraphsis absent from the Hub (pre-bug(protocol): curated publish authorization checks the transaction payer instead of the attested agent #1689 behaviour, deliberately retained).detailsit was actually evaluated from — never re-derived.The non-obvious hazard
The refactor would take the deployed-version memo with it. That memo is keyed on the resolved lifecycle address, and the key choice is load-bearing: a Hub rotation changes the address, so the memo is invalidated for free and the gate self-heals with no listener and no extra wiring. Relocating it is therefore not a mechanical move — the key choice and its rationale must be carried across deliberately.
Suggested sequencing
Land on a branch where CI actually runs. Confirm the existing admission and NO_FUNDED enrichment tests pass unchanged (they are the behaviour contract), and re-run the mutation set afterwards rather than assuming a pure refactor preserved it.
Also worth folding in: a known perf item from the same PR —
poolHasFundableSignerissues up to 2NisAuthorizedPublisherreads because those reads are not single-flighted (no CG-authorization read is aReadThroughTtlCacheconsumer). The one-line fix is hoisting the author branch out of the per-wallet loop, which an extraction would make natural.