Skip to content

feat: add Agent Registration File utilities#6

Merged
vraspar merged 7 commits into
mainfrom
vraspar/registration-file
Apr 9, 2026
Merged

feat: add Agent Registration File utilities#6
vraspar merged 7 commits into
mainfrom
vraspar/registration-file

Conversation

@vraspar
Copy link
Copy Markdown
Contributor

@vraspar vraspar commented Apr 7, 2026

Summary

  • Adds src/registration/ module implementing the ERC-8004 Agent Registration File spec
  • parseRegistrationFile() — validates JSON against the spec schema (type URI, required fields, service entries)
  • createRegistrationFile() — builder with auto-set type URI
  • findService() / findServices() — lookup services by name in a registration file
  • fetchRegistrationFile() — fetches and validates from HTTPS URLs
  • resolveServiceEndpoint() — full pipeline: resolveAgent(agentId) → fetch registration file → find service → return endpoint
  • New export path: @x402r/erc8004/registration
  • Bumps version to 0.1.0-alpha.1

All code is generic ERC-8004 spec-aligned — zero x402r references in src/.

Test plan

  • pnpm build && pnpm typecheck && pnpm test — 75 tests pass
  • grep -r "x402r" src/ returns zero matches
  • New registration tests cover: parse validation (valid/invalid), build (auto type), findService (hit/miss), fetch (mock valid/invalid), resolveServiceEndpoint (mock pipeline)

🤖 Generated with Claude Code

Add src/registration/ module with:
- types.ts: AgentRegistrationFile, ServiceEntry, RegistrationBinding
- parse.ts: parseRegistrationFile() with full schema validation
- build.ts: createRegistrationFile() builder with auto-set type URI
- services.ts: findService() and findServices() lookup helpers
- fetch.ts: fetchRegistrationFile() for HTTPS-only remote fetching
- resolve.ts: resolveServiceEndpoint() full pipeline (on-chain -> fetch -> lookup)

New export: @x402r/erc8004/registration
Bumps version to 0.1.0-alpha.1.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented Apr 8, 2026

SDK Review

Found 12 issues (reviewed: tests, conventions, dead code, SDK design):


Critical (100)

1. [Type Bug] RegistrationBinding.agentId is number — must be bigint

export interface RegistrationBinding {
agentId: number

Every other agentId in this codebase is bigint (ResolveAgentParameters, GiveFeedbackParameters, GetSummaryParameters, etc.) because ERC-721 token IDs are uint256 on-chain. Using number silently truncates any tokenId above 2^53 - 1. The upstream x402 spec (PR #1024) also explicitly says: "agentId is always a JSON string, even for EVM tokenIds that are numeric. This avoids precision loss for uint256 values."

A consumer who reads an agentId from resolveAgent() (returns bigint) and tries to populate a RegistrationBinding will get a type mismatch.

Fix: agentId: numberagentId: bigint


Important (75)

2. [Conventions] Duplicated REGISTRATION_TYPE constant

Same 'https://eips.ethereum.org/EIPS/eip-8004#registration-v1' string defined in both files. Should live once in types.ts and be imported.

3. [Conventions] Parameters interfaces defined outside types.ts

  • CreateRegistrationFileParameters in build.ts:10-19
  • ResolveServiceEndpointParameters in resolve.ts:6-10

In identity/ and reputation/, all parameter types live in types.ts. This PR breaks that convention.

4. [Conventions] as unknown as AgentRegistrationFile double cast

return obj as unknown as AgentRegistrationFile

The as unknown as T bypass is unnecessary after validation. Either use a single as AgentRegistrationFile cast or construct the object explicitly from validated fields.

5. [Conventions] agentRegistry: string loses CAIP-10 structure

agentRegistry: string // "eip155:{chainId}:{registryAddress}"

Could be a template literal type like `eip155:${number}:0x${string}` following viem's pattern of branded string types (e.g., Hex = `0x${string}`).

6. [SDK Design] CreateRegistrationFileParameters duplicates AgentRegistrationFile

export interface CreateRegistrationFileParameters {
name: string
description: string
image: string
services: ServiceEntry[]
x402Support?: boolean
active?: boolean
registrations?: RegistrationBinding[]
supportedTrust?: string[]
}

Near-duplicate of the interface minus type. Should be Omit<AgentRegistrationFile, 'type'>. The conditional spread logic (...(params.x !== undefined && { ... })) can then be replaced with { type: REGISTRATION_TYPE, ...params }.

7. [SDK Design] resolveServiceEndpoint returns bare string instead of a rich result

export async function resolveServiceEndpoint(
publicClient: PublicClient,
parameters: ResolveServiceEndpointParameters,
): Promise<string> {
const agent = await resolveAgent(publicClient, {
agentId: parameters.agentId,
registryAddress: parameters.registryAddress,
})
if (!agent.agentURI) {
throw new Error(`Agent ${parameters.agentId} has no URI set`)
}
const file = await fetchRegistrationFile(agent.agentURI)
const service = findService(file, parameters.serviceName)
if (!service) {
throw new Error(
`Service "${parameters.serviceName}" not found in agent ${parameters.agentId} registration file`,
)
}
return service.endpoint
}

Inconsistent with resolveAgent which returns a full ResolvedAgent object. Callers needing the ServiceEntry.version or the agentURI must re-implement the entire pipeline. Should return { endpoint, service, agentURI } at minimum. Changing the return type after release is breaking.

8. [SDK Design] Missing size-limit entry for registration subpath

package.json has size-limit entries for identity and reputation subpaths but not registration. Leaves a gap in CI bundle-size protection.

9. [Tests] No test for empty agentURI guard in resolveServiceEndpoint

it('resolves an endpoint through the full pipeline', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.resolve(registrationPayload),
}),
)
const publicClient = {
chain: { id: 8453 },
readContract: vi.fn(({ functionName }: { functionName: string }) => {
if (functionName === 'ownerOf')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'getAgentWallet')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'tokenURI')
return Promise.resolve('https://example.com/agent.json')
return Promise.reject(new Error(`unexpected: ${functionName}`))
}),
} as unknown as PublicClient
const endpoint = await resolveServiceEndpoint(publicClient, {
agentId: 42n,
serviceName: 'web',
registryAddress: REGISTRY,
})
expect(endpoint).toBe('https://example.com')
vi.unstubAllGlobals()
})
it('throws when the service is not found', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.resolve(registrationPayload),
}),
)
const publicClient = {
chain: { id: 8453 },
readContract: vi.fn(({ functionName }: { functionName: string }) => {
if (functionName === 'ownerOf')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'getAgentWallet')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'tokenURI')
return Promise.resolve('https://example.com/agent.json')
return Promise.reject(new Error(`unexpected: ${functionName}`))
}),
} as unknown as PublicClient
await expect(
resolveServiceEndpoint(publicClient, {
agentId: 42n,
serviceName: 'nonexistent',
registryAddress: REGISTRY,
}),
).rejects.toThrow('Service "nonexistent" not found')
vi.unstubAllGlobals()
})

resolve.ts:29-31 throws when agentURI is empty — a normal on-chain state after registration without a URI. This path is never exercised. Mock tokenURI returning '' to cover it.

10. [Tests] vi.stubGlobal cleanup leaks on assertion failure

it('fetches and parses a valid registration file', async () => {
const payload = validPayload()
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.resolve(payload),
}),
)
const result = await fetchRegistrationFile('https://example.com/agent.json')
expect(result.name).toBe('My Agent')
vi.unstubAllGlobals()
})
it('throws on non-HTTPS URL', async () => {
await expect(
fetchRegistrationFile('http://example.com/agent.json'),
).rejects.toThrow('Only HTTPS URIs are supported')
})
it('throws on non-200 response', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({ ok: false, status: 404 }),
)
await expect(
fetchRegistrationFile('https://example.com/agent.json'),
).rejects.toThrow('HTTP 404')
vi.unstubAllGlobals()
})
it('throws on invalid JSON response', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.reject(new Error('Unexpected token')),
}),
)
await expect(
fetchRegistrationFile('https://example.com/agent.json'),
).rejects.toThrow('not valid JSON')
vi.unstubAllGlobals()
})
})
// --- resolveServiceEndpoint ---
describe('resolveServiceEndpoint', () => {
const registrationPayload = validPayload()
it('resolves an endpoint through the full pipeline', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.resolve(registrationPayload),
}),
)
const publicClient = {
chain: { id: 8453 },
readContract: vi.fn(({ functionName }: { functionName: string }) => {
if (functionName === 'ownerOf')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'getAgentWallet')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'tokenURI')
return Promise.resolve('https://example.com/agent.json')
return Promise.reject(new Error(`unexpected: ${functionName}`))
}),
} as unknown as PublicClient
const endpoint = await resolveServiceEndpoint(publicClient, {
agentId: 42n,
serviceName: 'web',
registryAddress: REGISTRY,
})
expect(endpoint).toBe('https://example.com')
vi.unstubAllGlobals()
})
it('throws when the service is not found', async () => {
vi.stubGlobal(
'fetch',
vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.resolve(registrationPayload),
}),
)
const publicClient = {
chain: { id: 8453 },
readContract: vi.fn(({ functionName }: { functionName: string }) => {
if (functionName === 'ownerOf')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'getAgentWallet')
return Promise.resolve('0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')
if (functionName === 'tokenURI')
return Promise.resolve('https://example.com/agent.json')
return Promise.reject(new Error(`unexpected: ${functionName}`))
}),
} as unknown as PublicClient
await expect(
resolveServiceEndpoint(publicClient, {
agentId: 42n,
serviceName: 'nonexistent',
registryAddress: REGISTRY,
}),
).rejects.toThrow('Service "nonexistent" not found')
vi.unstubAllGlobals()

vi.unstubAllGlobals() is called at the end of each it body. If the assertion throws first, the stub leaks into subsequent tests. Move to afterEach(() => vi.unstubAllGlobals()) at the describe level. Affects all 6 tests that stub fetch.

11. [Tests] services[i] must be an object branch untested

it('throws when a service entry lacks name', () => {
const payload = validPayload()
payload.services = [{ endpoint: 'https://example.com' }]
expect(() => parseRegistrationFile(payload)).toThrow(
'services[0].name must be a string',
)
})
it('throws when a service entry lacks endpoint', () => {
const payload = validPayload()
payload.services = [{ name: 'web' }]
expect(() => parseRegistrationFile(payload)).toThrow(
'services[0].endpoint must be a string',
)
})
it('throws on non-object input', () => {
expect(() => parseRegistrationFile('string')).toThrow(
'must be a JSON object',
)
expect(() => parseRegistrationFile(null)).toThrow('must be a JSON object')
expect(() => parseRegistrationFile([1])).toThrow('must be a JSON object')
})
it('throws when required string fields are missing', () => {
for (const field of ['name', 'description', 'image']) {
const payload = validPayload()
delete payload[field]
expect(() => parseRegistrationFile(payload)).toThrow(
`"${field}" must be a string`,
)
}
})

parse.ts:48-51 guards against service entries that are primitives/null/arrays. No test covers services: ['https://example.com'] — only missing name/endpoint within valid objects.

12. [Tests] No fork test for resolveServiceEndpoint

resolveServiceEndpoint calls resolveAgent (3 readContract calls). Both identity/ and reputation/ have fork tests (identity.fork.test.ts, reputation.fork.test.ts). No registration.fork.test.ts exists — mock tests can't catch ABI encoding mismatches.


# Category Issue Severity
1 Type Bug agentId: numberbigint 100
2 Conventions Duplicated REGISTRATION_TYPE constant 75
3 Conventions Parameters interfaces outside types.ts 75
4 Conventions as unknown as double cast 75
5 Conventions agentRegistry untyped string 75
6 SDK Design CreateRegistrationFileParameters duplicates interface 75
7 SDK Design resolveServiceEndpoint returns bare string 75
8 SDK Design Missing size-limit entry 75
9 Tests Empty agentURI guard untested 75
10 Tests vi.stubGlobal leak pattern 75
11 Tests Service entry non-object branch untested 75
12 Tests No fork test for resolveServiceEndpoint 75

Generated with Claude Code using review-sdk skill

…verage

- agentId: number → bigint in RegistrationBinding (type bug)
- Deduplicate REGISTRATION_TYPE constant into types.ts
- Move param interfaces to types.ts (convention alignment)
- CreateRegistrationFileParameters uses Omit<AgentRegistrationFile, 'type'>
- agentRegistry typed as CAIP-10 template literal
- resolveServiceEndpoint returns { endpoint, service, agentURI }
- parseRegistrationFile validates optional fields when present
- fetchRegistrationFile adds 10s timeout + 1MB size guard
- resolveServiceEndpoint wraps fetch errors with agent context
- Add size-limit entry for registration subpath
- Fix vi.stubGlobal leak with afterEach cleanup
- Add 10 new tests covering all changes

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented Apr 9, 2026

SDK Review — Re-review (post e9577f4)

10 of 12 original issues fixed. 1 new regression found, 2 carried over.


New Regression (85)

[Type Bug] validateRegistrationBinding rejects string agentId — the spec-mandated format

typeof binding.agentId !== 'bigint' &&
typeof binding.agentId !== 'number'
) {
throw new Error(
`registrations[${index}].agentId must be a number or bigint`,

The upstream spec (x402-foundation/x402 PR #1024) says: "agentId is always a JSON string, even for EVM tokenIds that are numeric." A spec-compliant registration file has "agentId": "42" (string). JSON.parse produces { agentId: "42" } — a string, which this validator rejects because it only checks for bigint | number.

The coercion on line 79 (BigInt(binding.agentId)) would handle strings fine (BigInt("42") = 42n), but the validation gate blocks it.

Fix:

if (
  typeof binding.agentId !== 'bigint' &&
  typeof binding.agentId !== 'number' &&
  typeof binding.agentId !== 'string'
) {
  throw new Error(
    `registrations[${index}].agentId must be a string, number, or bigint`,
  )
}

Carried Over

[Conventions] as unknown as AgentRegistrationFile double cast (50)

return obj as unknown as AgentRegistrationFile

Still present. Lower severity now with thorough validation — the cast is structurally necessary given Record<string, unknown> → literal type. Not blocking.

[Tests] No fork test for resolveServiceEndpoint (75)

Still no tests/fork/registration.fork.test.ts. Both identity/ and reputation/ have fork tests for their readContract-backed functions. Not blocking for alpha but should exist before stable.


Resolved ✓

# Original Issue Fix
1 agentId: numberbigint types.ts:25
2 Duplicated REGISTRATION_TYPE Single source in types.ts:3-4
3 Params outside types.ts Both moved to types.ts
5 agentRegistry untyped string Template literal `eip155:${number}:0x${string}`
6 CreateRegistrationFileParameters duplication Omit<AgentRegistrationFile, 'type'>
7 resolveServiceEndpoint bare string Returns ResolvedServiceEndpoint
8 Missing size-limit entry Added at 5 kB
9 Empty agentURI guard untested Test at line 389
10 vi.stubGlobal leak afterEach cleanup
11 Service entry primitive untested Test at line 89

New additions (timeout, 1MB guard, error wrapping, optional field validation, bigint coercion) all look good.


Generated with Claude Code using review-sdk skill

The ERC-8004 spec mandates agentId as a JSON string. JSON.parse
produces { agentId: "42" } which the validator was rejecting.
Accept string, number, and bigint — all coerced to bigint.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented Apr 9, 2026

SDK Review — Genericity Check

The PR claims "zero x402r references in src/" — true at the string level, but x402-ecosystem conventions leak in two places.


[Genericity] x402Support and supportedTrust hardcoded in AgentRegistrationFile (75)

export interface AgentRegistrationFile {
type: typeof REGISTRATION_TYPE
name: string
description: string
image: string
services: ServiceEntry[]
x402Support?: boolean
active?: boolean
registrations?: RegistrationBinding[]
supportedTrust?: string[]
}

The ERC-8004 spec defines: type, name, description, image, services, active, registrations. The x402Support and supportedTrust fields are x402/agent0 ecosystem conventions, not part of the core spec. A non-x402 consumer (e.g., an A2A agent using ERC-8004 for identity only) gets x402-specific fields baked into their type.

Fix — use an index signature for non-spec fields:

export interface AgentRegistrationFile {
  type: typeof REGISTRATION_TYPE
  name: string
  description: string
  image: string
  services: ServiceEntry[]
  active?: boolean
  registrations?: RegistrationBinding[]
  [key: string]: unknown  // allow ecosystem-specific fields without hardcoding them
}

x402-specific consumers access those fields with typed narrowing in their own layer (SDK plugin), not in this generic library.


[Genericity] parseRegistrationFile validates non-spec fields (75)

if (obj.active !== undefined && typeof obj.active !== 'boolean') {
throw new Error('"active" must be a boolean when present')
}
if (obj.registrations !== undefined) {
if (!Array.isArray(obj.registrations)) {
throw new Error('"registrations" must be an array when present')
}
for (let i = 0; i < obj.registrations.length; i++) {
validateRegistrationBinding(obj.registrations[i], i)
}
}
if (obj.supportedTrust !== undefined) {
if (!Array.isArray(obj.supportedTrust)) {
throw new Error('"supportedTrust" must be an array when present')
}
for (let i = 0; i < obj.supportedTrust.length; i++) {
if (typeof obj.supportedTrust[i] !== 'string') {
throw new Error(`supportedTrust[${i}] must be a string`)
}
}
}
// Coerce registrations[].agentId to bigint (JSON.parse produces number)
if (obj.registrations) {
for (const binding of obj.registrations as Array<Record<string, unknown>>) {

The parser rejects files where x402Support is not a boolean or supportedTrust contains non-strings. A generic ERC-8004 parser should only validate spec-mandated fields and pass through everything else. Currently "x402Support": 1 causes a parse failure — this library is imposing x402 conventions on all consumers.

Fix — remove the x402Support and supportedTrust validation blocks (lines 46–73). Only validate: type, name, description, image, services, active, registrations.


Both fixes align with the plan's own rule: "@x402r/erc8004 stays generic. No x402 extension logic."


Generated with Claude Code using review-sdk skill

x402Support and supportedTrust are ecosystem conventions, not ERC-8004
spec fields. Replace with [key: string]: unknown index signature so
non-spec consumers aren't forced into x402-specific types. Validation
now only covers spec-mandated fields; extension fields pass through.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@A1igator
Copy link
Copy Markdown

A1igator commented Apr 9, 2026

SDK Review

Found 4 issues (reviewed: tests, conventions, dead code, SDK design):

  1. [Conventions] parseRegistrationFile mutates the caller's input object in-place. The obj reference is just a cast of the original json parameter -- coercing agentId to bigint (lines 49-53) silently modifies whatever the caller passed in. A function named "parse" should be a read-only validator, not a transformer with side effects.

    throw new Error('"registrations" must be an array when present')
    }
    for (let i = 0; i < obj.registrations.length; i++) {
    validateRegistrationBinding(obj.registrations[i], i)
    }

    Fix: Reconstruct the bindings array with coerced values instead of mutating:

    if (obj.registrations) {
      obj.registrations = (obj.registrations as Array<Record<string, unknown>>).map(
        (b) => ({ ...b, agentId: BigInt(b.agentId as string | number | bigint) })
      )
    }
  2. [SDK Design] createRegistrationFile is a trivial object-spread with a type-unsafe cast. The as AgentRegistrationFile defeats TypeScript's structural check on the spread -- callers can pass invalid data and the compiler won't catch it. A builder that does zero validation adds API surface without behavioral value.

    export function createRegistrationFile(
    params: CreateRegistrationFileParameters,
    ): AgentRegistrationFile {
    return { type: REGISTRATION_TYPE, ...params } as AgentRegistrationFile
    }

    Fix: Either delete the builder and let callers construct { type: REGISTRATION_TYPE, ...params } directly (adding the constant to the public API is sufficient), or have it call parseRegistrationFile internally so the builder actually guarantees validity.

  3. [Tests] All three error branches in validateRegistrationBinding are untested: primitive entry (registrations: [42]), missing agentRegistry, and invalid agentId type (agentId: true). These validation paths are the security boundary for malformed registration files from untrusted sources.

    for (const binding of obj.registrations as Array<Record<string, unknown>>) {
    binding.agentId = BigInt(binding.agentId as number)
    }
    }
    return obj as unknown as AgentRegistrationFile
    }
    function validateRegistrationBinding(entry: unknown, index: number): void {
    if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) {
    throw new Error(`registrations[${index}] must be an object`)
    }
    const binding = entry as Record<string, unknown>
    if (
    typeof binding.agentId !== 'bigint' &&
    typeof binding.agentId !== 'number' &&
    typeof binding.agentId !== 'string'

    Fix: Add three tests:

    it('throws when a registrations entry is a primitive', () => {
      const p = validPayload(); p.registrations = [42]
      expect(() => parseRegistrationFile(p)).toThrow('registrations[0] must be an object')
    })
    it('throws when agentRegistry is missing', () => {
      const p = validPayload(); p.registrations = [{ agentId: 1 }]
      expect(() => parseRegistrationFile(p)).toThrow('agentRegistry must be a string')
    })
    it('throws when agentId has invalid type', () => {
      const p = validPayload(); p.registrations = [{ agentId: true, agentRegistry: 'eip155:8453:0xabc' }]
      expect(() => parseRegistrationFile(p)).toThrow('agentId must be a string, number, or bigint')
    })
  4. [SDK Design] resolveServiceEndpoint in registration/ imports from identity/, creating a hidden one-way dependency between sibling modules. This is a cross-resource pipeline (identity resolution + registration fetch + service lookup) that belongs either at the call site or in a top-level orchestration layer, not nested inside one of the two modules it bridges.

    import { resolveAgent } from '../identity/index.js'

    Fix: Consider moving resolveServiceEndpoint to src/identity/ (where resolveAgent lives) or to a top-level src/actions/ directory. Either option makes the dependency direction explicit.


Generated with Claude Code using review-sdk skill

parseRegistrationFile was coercing registrations[].agentId to bigint
in-place, mutating the caller's object. Now reconstructs the bindings
array via .map() instead. Also adds 4 tests: primitive binding entry,
missing agentRegistry, invalid agentId type, and no-mutation check.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented Apr 9, 2026

Response to review #4

#1 (parse mutates input) — Fixed in eb95130. parseRegistrationFile now reconstructs the bindings array via .map() instead of coercing agentId in-place. Added a no-mutation test to verify.

#2 (builder is trivial) — Keeping as-is. The builder auto-sets a 60-char type URI that callers shouldn't need to remember or copy-paste. The as AgentRegistrationFile cast is structurally required because the [key: string]: unknown index signature prevents TypeScript from verifying the spread. Callers who want validation can call parseRegistrationFile separately — baking it into the builder adds runtime cost for typed inputs that are already correct at compile time.

#3 (validateRegistrationBinding untested) — Fixed in eb95130. Added 3 tests: primitive binding entry, missing agentRegistry, invalid agentId type.

#4 (cross-module dependency) — Keeping as-is. The dependency is one-way (registration → identity) and semantically correct: the function resolves a service endpoint from a registration file, using resolveAgent as a pipeline step. Moving it to identity/ would be wrong — identity has no concept of registration files or services. A top-level src/actions/ for a single function is premature indirection.

@vraspar
Copy link
Copy Markdown
Contributor Author

vraspar commented Apr 9, 2026

SDK Review — Final (commit 4bdc529)

Found 9 issues (reviewed: tests, conventions, dead code, SDK design):


1. [Correctness] Non-numeric string agentId bypasses validation, throws raw SyntaxError (82)

https://github.com/BackTrackCo/erc8004/blob/4bdc529e8b6e/src/registration/parse.ts#L56-L67

validateRegistrationBinding accepts any string for agentId, but BigInt("abc") throws an unhandled SyntaxError during coercion. Validation says "ok", coercion crashes with a raw engine error and no field context. No test covers this path.

Fix — validate numeric strings in the validator, or wrap the BigInt() call:

if (typeof binding.agentId === 'string' && !/^\d+$/.test(binding.agentId)) {
  throw new Error(
    `registrations[${index}].agentId string must be a non-negative integer, got "${binding.agentId}"`
  )
}

2. [Conventions] params vs parameters naming inconsistency (82)

https://github.com/BackTrackCo/erc8004/blob/4bdc529e8b6e/src/registration/build.ts#L11

Uses params as the argument name. Every other function across identity/ and reputation/ (30+ call sites) uses parameters.

3. [Security] Content-length size guard is bypassable (80)

https://github.com/BackTrackCo/erc8004/blob/4bdc529e8b6e/src/registration/fetch.ts#L28-L30

The 1 MB guard only fires when the server sends a content-length header. Any server omitting it (standard for chunked responses) bypasses the check entirely, and response.json() buffers the full body regardless. The documented throw condition "exceeds 1 MB" is a false guarantee.

Fix — enforce on actual bytes, keep content-length as early-exit optimization:

const text = await response.text()
if (text.length > 1_048_576) {
  throw new Error('Registration file exceeds 1 MB size limit')
}
let json: unknown
try { json = JSON.parse(text) } catch { throw new Error('Response is not valid JSON') }

4. [Conventions] SPEC_TYPE in tests duplicates exported REGISTRATION_TYPE (78)

https://github.com/BackTrackCo/erc8004/blob/4bdc529e8b6e/tests/registration.test.ts#L11

Hardcodes the URI string instead of importing REGISTRATION_TYPE. If the constant's value changes, tests silently pass against the stale string. Every other test file imports constants rather than duplicating.

5. [Design] Cross-module coupling in resolveServiceEndpoint (77)

https://github.com/BackTrackCo/erc8004/blob/4bdc529e8b6e/src/registration/resolve.ts#L2

The only file in registration/ that imports from identity/. The rest of the module is pure off-chain file-format logic. This is the only cross-module import in the entire src/ tree. Consider moving resolveServiceEndpoint to a separate pipeline.ts file to signal it's a composed convenience, not a primitive — or lifting it to src/ level.

6. [Conventions] mockReadContract duplicates shared mockPublic utility (76)

https://github.com/BackTrackCo/erc8004/blob/4bdc529e8b6e/tests/registration.test.ts#L371-L391

Structurally identical to mockPublic from tests/setup/mocks.ts, which identity.test.ts and reputation.test.ts already use. Import the shared helper instead.

7. [Tests] Assertion inside mock body — silently passes if fetch never called (75)

The "passes abort signal" test has expect(init?.signal).toBeDefined() inside the mock callback. If fetch is never invoked (e.g., future refactor), the test passes green with zero assertions evaluated. Move assertion to top level and verify with toBeInstanceOf(AbortSignal).

8. [Tests] Missing registryAddress auto-resolve tests (75)

Every public function in identity.test.ts and reputation.test.ts has a dedicated registryAddress auto-resolve describe block. resolveServiceEndpoint is the only PublicClient-accepting function in the new module and has no such coverage.

9. [Tests] No test for agentId input already a bigint (75)

Two coercion tests exist (number→bigint, string→bigint) but the bigint→bigint path — which the validator explicitly accepts — has no test.


# Category Issue Severity
1 Correctness Non-numeric string agentId → raw SyntaxError 82
2 Conventions params vs parameters naming 82
3 Security Content-length guard bypassable without header 80
4 Conventions Test hardcodes constant instead of importing 78
5 Design Cross-module coupling in resolve 77
6 Conventions Test duplicates shared mock helper 76
7 Tests Assertion buried inside mock body 75
8 Tests Missing registryAddress auto-resolve coverage 75
9 Tests No test for bigint→bigint coercion path 75

Generated with Claude Code using review-sdk skill

vraspar and others added 2 commits April 8, 2026 23:21
…cleanup

- Reject non-numeric string agentId (e.g. "abc") with descriptive error
  instead of raw SyntaxError from BigInt()
- Rename params → parameters in build.ts (codebase convention)
- Use response.text() + length check instead of content-length header
  for reliable size enforcement on chunked responses
- Import REGISTRATION_TYPE in tests instead of hardcoding
- Use shared mockPublic helper instead of local duplicate
- Move abort signal assertion to top level (not inside mock body)
- Add bigint→bigint coercion path test

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@vraspar vraspar merged commit 164d848 into main Apr 9, 2026
2 checks passed
@vraspar vraspar deleted the vraspar/registration-file branch April 9, 2026 06:28
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.

2 participants