refactor(security): single-source lookupKnownSpender (#780) - #827
Merged
Conversation
permit2.ts carried a byte-for-byte functional duplicate of known-spenders.ts's lookupKnownSpender (same CONTRACTS-driven loop/switch/label logic) — two implementations of one security lookup guaranteed to drift, the failure class #765 D10 single-sourcing exists to prevent. Delete the local redefinition in permit2.ts, import the canonical export from src/security/known-spenders.js instead (matching the import already used by custom-call/actions.ts and execution/index.ts). Drop the now-unused CONTRACTS import from permit2.ts along with it. Add a regression guard (test/780-known-spender-single-source.test.ts) that walks src/ and asserts exactly one `function lookupKnownSpender(` definition exists, plus that permit2.ts imports rather than redefines it.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Closes #780
What changed and why
lookupKnownSpenderwas implemented twice: the canonical, exportedversion in
src/security/known-spenders.ts:35-51, and a local,non-exported redefinition in
src/modules/allowances/permit2.ts:195-222with byte-for-byte identical CONTRACTS-driven loop/switch/label logic.
Two implementations of one security lookup are guaranteed to drift —
a spender allowlisted (or removed) in one copy without a matching edit
to the other — the same failure class #765 D10's single-sourcing
pattern exists to prevent.
Fix: deleted the local redefinition in
permit2.tsand imported thecanonical function instead:
This mirrors the import already used by
src/modules/custom-call/actions.tsand
src/modules/execution/index.ts, both of which already imported thecanonical function rather than redefining it —
permit2.tswas the onlyholdout. The sole callsite (
permit2.ts:338pre-fix,lookupKnownSpender(chain, downstreamSpender)) type-checks unchangedagainst the shared signature (
chain: SupportedChain, spender:0x${string}``).The
CONTRACTSimport inpermit2.tsbecame unused once its onlyconsumer (the local
lookupKnownSpender) was removed, so it's droppedtoo — the doc comment on
Permit2SubAllowanceRow.downstreamSpenderLabelthat references "the canonical CONTRACTS table" is unchanged and still
accurate (the shared function still reads from
CONTRACTS, just fromone place now).
Falsifier test
test/780-known-spender-single-source.test.ts— two assertions:.tsfile undersrc/(same recursive-walk pattern astest/757-recipient-authorization.test.ts's "no fourth writer"guard) and counts
function lookupKnownSpender(definitions.Fails on unfixed code (finds two:
known-spenders.tsandpermit2.ts). Passes after this fix (finds exactly one, inknown-spenders.ts).permit2.tscontains an import statement forlookupKnownSpenderfrom../../security/known-spenders.js, so afuture contributor can't satisfy assertion 1 by just renaming the
local copy to something else while still not importing the shared
one.
Both assertions report the offending file:line on failure for
immediate actionability.
Tests were NOT run locally — Node/npm are not installed on the
authoring machine. CI (Build & Test on Node 20 + 22) is the
authoritative check for this PR; the new test was written by matching
test/757-recipient-authorization.test.ts's directory-walk patternand
test/presign-annotation-guard-735.test.ts's static-source-guardstyle exactly, so it should run as-is under the existing vitest setup.
Existing tests updated
None.
lookupKnownSpenderwas never exported frompermit2.tsand noexisting test in
test/allowances-permit2.test.ts(or elsewhere)imports or exercises it by name — the two implementations were
identical in behavior, so there's no existing assertion whose expected
value changes.
Blast radius
One file's internal function body deleted (
permit2.ts), one importline changed, one import line dropped, no other change. The public
behavior of
fetch_permit2_sub_allowances(the tool that calls thenow-shared function at the old line 338) is unchanged — the deleted
code was byte-for-byte identical to what it now calls. No schema,
export, or tool-surface change.
Residual concerns
("add a test/lint guard that a second definition of a security
lookup fails, per docs(design): consolidate #759 recipient-authorization seam design #765 D10's pattern") as a general rule covering
every security lookup — only a guard scoped to
lookupKnownSpenderitself. Generalizing it to catch drift on other security lookups
(allowlists, selector checks, etc.) would need enumerating which
functions count as "a security lookup" repo-wide, which reads as a
separate, larger decision than this point-fix; flagging it rather
than guessing scope.
npm run build,npm run lint, ornpm testlocally (no Node on this machine) — the diff iscorrectness-by-construction against the existing file's style and
the CI run is the first real execution of the new test.