Add reputation functions + fork tests (Steps 3-5)

[vraspar]

Summary

• 9 reputation functions: giveFeedback, revokeFeedback, appendResponse, getSummary, getClients, readAllFeedback, readFeedback, getLastIndex, getResponseCount
• readAllFeedback transforms contract's 7 parallel arrays into structured FeedbackEntry[] objects
• Fork test infrastructure: prool-managed Anvil instances forking Base Sepolia (adapted from x402r-sdk)
• 17 fork tests: 8 identity + 9 reputation — full lifecycle (register → feedback → response → revoke)
• 15 reputation unit tests + shared mock helpers extracted from identity tests
• CI: foundry-toolchain for Anvil binary, scoped test scripts (--project unit / --project fork)

Test plan

• pnpm test — 37 unit tests pass (22 identity + 15 reputation)
• pnpm test:fork — 17 fork tests pass against Anvil fork of Base Sepolia
• pnpm build && pnpm typecheck — compiles clean
• pnpm check — biome lint/format clean
• pnpm knip — no unused exports
• pnpm size-limit — all entries within limits

🤖 Generated with Claude Code

[vraspar]

SDK Review

Found 7 issues (reviewed: tests, conventions, dead code, CI/coverage, SDK design):

---

1. [CI/Coverage] Coverage config nested inside project — may be silently ignored by vitest

vitest.config.ts puts coverage: inside test.projects[0].test.coverage. In vitest's projects mode, coverage is a root-level config, not per-project. Running vitest run --project unit --coverage may look for root-level coverage config, find none, and use defaults — meaning the reporter, include, and exclude filters never apply. The lcov.info file may not be generated with the expected reporters, and the codecov upload step uses fail_ci_if_error: false, silently masking any missing file.

erc8004/vitest.config.ts

Lines 9 to 16 in 3d2946c

	include: ['tests/**/*.test.ts'],
	exclude: ['tests/fork/**'],
	coverage: {
	provider: 'v8',
	reporter: [process.env.CI ? 'lcov' : 'text', 'json', 'html'],
	include: ['src/**/*.ts'],
	exclude: ['src/**/index.ts', 'src/**/types.ts'],
	},

Fix: Move coverage: to the root test: level, outside projects::

defineConfig({
  test: {
    coverage: {
      provider: 'v8',
      reporter: [process.env.CI ? 'lcov' : 'text', 'json', 'html'],
      include: ['src/**/*.ts'],
      exclude: ['src/**/index.ts', 'src/**/types.ts', 'src/abis/**'],
    },
    projects: [ ... ],
  },
})

---

2. [CI/Coverage] Coverage include/exclude doesn't match codecov ignore — ABI files inflate local numbers

vitest include: ['src/**/*.ts'] counts src/abis/erc8004.ts (~300 lines of pure ABI data, zero logic). codecov.yml has ignore: ['src/abis/**'] which strips it. But vitest exclude only lists index.ts and types.ts — not src/abis/**. Local coverage will be inflated by the ABI file (which has no branches), while codecov shows different numbers.

Fix: Add 'src/abis/**' to vitest coverage exclude to match codecov's view.

---

3. [Dead Code] TEST_PRIVATE_KEY secret referenced in CI but never used

The fork test CI step sets TEST_PRIVATE_KEY: ${{ secrets.TEST_PRIVATE_KEY }} as an env var. No code reads process.env.TEST_PRIVATE_KEY — all fork test accounts use Anvil's hardcoded deterministic keys from tests/setup/constants.ts. This dead config implies fork tests need a funded wallet (they don't) and will confuse contributors setting up secrets.

erc8004/.github/workflows/ci.yml

Lines 58 to 60 in 3d2946c

	uses: nick-fields/retry@v3
	env:
	FORK_URL: ${{ secrets.FORK_URL }}

Fix: Remove TEST_PRIVATE_KEY from the CI env block.

---

4. [CI] dist/identity/index.js missing from size-limit

tsup builds 4 entrypoints (index, identity/index, reputation/index, abis/index). The exports map has 4 entries. But size-limit only checks 3 — dist/index.js, dist/reputation/index.js, dist/abis/index.js. The identity entry was never added (oversight from Step 1), and this PR adds the reputation entry without catching the gap.

Fix: Add { "path": "dist/identity/index.js", "limit": "5 kB" } to the size-limit array.

---

5. [Tests] Reputation unit tests use identity registry address

REGISTRY in tests/setup/mocks.ts is 0x8004A169FB4a3325136EB29fA0ceB6D2e539a432 — the identity registry. All reputation tests pass this as registryAddress. Since tests mock everything, the wrong address doesn't cause failures, but test assertions verify writeContract/readContract was called with the identity registry address — not the reputation registry. A reader would incorrectly conclude identity and reputation share an address.

Fix: Add REPUTATION_REGISTRY constant to mocks.ts using 0x8004BAa17C55a88189AE136b182e5fdA19dE9b63 (or a synthetic 0xCC... address), and use it in reputation tests.

---

6. [Tests] readAllFeedback missing includeRevoked: true test case

This is the most transformation-heavy function (7 parallel arrays → structured objects) with a boolean filter. Both unit test cases use includeRevoked: false. No test verifies the true path is forwarded to the contract or that entries with isRevoked: true appear in results.

Fix: Add a test with includeRevoked: true that asserts the flag is passed to readContract and that revoked entries appear in the output.

---

7. [Tests] Fork test readAllFeedback missing per-field assertions

The fork test for readAllFeedback only checks entries.length > 0, entries[0].value, and entries[0].tag1. Missing assertions for: client, feedbackIndex, valueDecimals, tag2, isRevoked. Fork tests are the ABI verification layer — the 7-array-to-object transformation is the highest-risk code path, and 5 of 7 decoded fields go completely unchecked.

Fix: Assert all fields of entries[0] in the fork test to verify the zip transformation against real contract output.

---

Generated with Claude Code using review-sdk skill

[vraspar]

All 7 issues addressed in 7399134. Quick assessment:

All valid — no pushback needed this time.

1. Coverage config — Correct. vitest's projects mode reads coverage from root test.coverage, not per-project. Was silently ignored. Moved to root + added src/abis/** exclusion to match codecov.
2. ABI coverage inflate — Rolled into fix Add ABIs + registerAgent + isRegistered (Step 1) #1.
3. Dead TEST_PRIVATE_KEY — Leftover from the pre-prool plan. Removed.
4. Missing identity size-limit — Oversight from Step 1. Added.
5. Wrong registry in reputation tests — Functionally harmless (mocks don't check address), but confusing for readers. Added REPUTATION_REGISTRY constant.
6. includeRevoked: true test — Borderline (it's just a passthrough bool), but reasonable for the most complex function. Added with assertion that true reaches the contract and revoked entries appear.
7. Fork test per-field assertions — Good catch. Fork tests are the ABI verification layer — checking 2/7 fields was insufficient. Now asserts all 7 fields of entries[0].

Tests: 38 unit + 17 fork, all passing. Build/typecheck/biome/knip/size-limit clean.

[A1igator]

SDK Review

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

Tests

1. [Tests] giveFeedback and appendResponse fork tests only check receipt status

Both write-function fork tests verify receipt.status === 'success' but never read back state to confirm the correct values were stored. The giveFeedback test is particularly important as it establishes all downstream test state.

• giveFeedback:

  erc8004/tests/fork/reputation.fork.test.ts

  Lines 42 to 55 in 7399134

  	it('giveFeedback submits feedback', async () => {
  	const hash = await giveFeedback(feedbackGiverClient, {
  	agentId,
  	value: 85n,
  	valueDecimals: 0,
  	tag1: 'x402r.resolution',
  	tag2: 'quality',
  	endpoint: '',
  	feedbackURI: '',
  	feedbackHash: zeroHash,
  	})
  	const receipt = await publicClient.waitForTransactionReceipt({ hash })
  	expect(receipt.status).toBe('success')
  	})

• appendResponse:

  erc8004/tests/fork/reputation.fork.test.ts

  Lines 123 to 138 in 7399134

  	it('appendResponse adds a response to feedback', async () => {
  	const lastIndex = await getLastIndex(publicClient, {
  	agentId,
  	clientAddress: accounts[1].address,
  	})
  	const hash = await appendResponse(agentOwnerClient, {
  	agentId,
  	clientAddress: accounts[1].address,
  	feedbackIndex: lastIndex,
  	responseURI: 'https://response.example.com',
  	responseHash: zeroHash,
  	})
  	const receipt = await publicClient.waitForTransactionReceipt({ hash })
  	expect(receipt.status).toBe('success')
  	})

Fix: After each write, read back the stored data and assert field values. For giveFeedback, call readFeedback and check value === 85n; for appendResponse, check response count incremented.

2. [Tests] getLastIndex and getResponseCount use toBeGreaterThan(0n) when exact values are deterministic

The lifecycle runs with exactly 1 feedback + 1 response, so both should return exactly 1n. Lower-bound checks would pass even with counter bugs.

erc8004/tests/fork/reputation.fork.test.ts

Lines 64 to 70 in 7399134

	it('getLastIndex returns the feedback index', async () => {
	const index = await getLastIndex(publicClient, {
	agentId,
	clientAddress: accounts[1].address,
	})
	expect(index).toBeGreaterThan(0n)
	})

erc8004/tests/fork/reputation.fork.test.ts

Lines 140 to 154 in 7399134

	it('getResponseCount returns the response count', async () => {
	const lastIndex = await getLastIndex(publicClient, {
	agentId,
	clientAddress: accounts[1].address,
	})
	const count = await getResponseCount(publicClient, {
	agentId,
	clientAddress: accounts[1].address,
	feedbackIndex: lastIndex,
	responders: [accounts[0].address],
	})
	expect(count).toBeGreaterThan(0n)
	})

Fix: expect(index).toBe(1n) and expect(count).toBe(1n)

3. [Tests] readAllFeedback includeRevoked filtering untested on-chain

Fork tests call readAllFeedback only with includeRevoked: false before revocation. After revokeFeedback, the test verifies via readFeedback but never calls readAllFeedback again to verify the contract's actual filtering logic.

erc8004/tests/fork/reputation.fork.test.ts

Lines 102 to 121 in 7399134

	it('readAllFeedback returns structured entries', async () => {
	const entries = await readAllFeedback(publicClient, {
	agentId,
	clientAddresses: [accounts[1].address],
	tag1: 'x402r.resolution',
	tag2: 'quality',
	includeRevoked: false,
	})
	expect(entries).toHaveLength(1)
	expect(entries[0].client.toLowerCase()).toBe(
	accounts[1].address.toLowerCase(),
	)
	expect(entries[0].feedbackIndex).toBeGreaterThan(0n)
	expect(entries[0].value).toBe(85n)
	expect(entries[0].valueDecimals).toBe(0)
	expect(entries[0].tag1).toBe('x402r.resolution')
	expect(entries[0].tag2).toBe('quality')
	expect(entries[0].isRevoked).toBe(false)
	})

Fix: After revocation, call readAllFeedback with includeRevoked: false (expect empty) and includeRevoked: true (expect 1 entry with isRevoked: true).

Conventions

4. [Conventions] ZERO_HASH manually defined instead of viem's zeroHash

The unit test defines ZERO_HASH as a manual hex string while the fork test correctly imports zeroHash from viem. Inconsistent and reinvents a viem export.

erc8004/tests/reputation.test.ts

Lines 4 to 5 in 7399134

	import { getClients } from '../src/reputation/getClients.js'
	import { getLastIndex } from '../src/reputation/getLastIndex.js'

Fix: import { zeroHash } from 'viem' and replace all ZERO_HASH references.

5. [Conventions] Unnecessary spread of readonly Address[] arrays in 3 files

getSummary.ts, readAllFeedback.ts, and getResponseCount.ts spread readonly arrays into mutable copies ([...parameters.clientAddresses]). viem's ContractFunctionArgs already accepts readonly Address[] for address[] ABI params -- no spread needed.

• erc8004/src/reputation/getSummary.ts

  Line 26 in 7399134

  [...parameters.clientAddresses],

• erc8004/src/reputation/readAllFeedback.ts

  Line 36 in 7399134

  [...parameters.clientAddresses],

• erc8004/src/reputation/getResponseCount.ts

  Line 27 in 7399134

  [...parameters.responders],

Fix: Pass arrays directly without spreading.

Dead Code

6. [Dead Code] accounts[2] (third test account) never accessed

Only accounts[0] and accounts[1] are used across all tests. The third account is dead weight.

erc8004/tests/setup/constants.ts

Lines 24 to 29 in 7399134

	{
	address: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
	privateKey:
	'0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a',
	},
	] as const

7. [Dead Code] privateKey fields on all test accounts never read

After switching to anvilBaseSepolia.getWalletClient(address), no code reads .privateKey. The TestAccount interface and all privateKey values are dead.

erc8004/tests/setup/constants.ts

Lines 7 to 29 in 7399134

	interface TestAccount {
	address: Address
	privateKey: Hex
	}
	export const accounts: readonly TestAccount[] = [
	{
	address: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266',
	privateKey:
	'0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80',
	},
	{
	address: '0x70997970C51812dc3A010C7d01b50e0d17dc79C8',
	privateKey:
	'0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d',
	},
	{
	address: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
	privateKey:
	'0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a',
	},
	] as const

Fix: Simplify to export const accounts = ['0xf39F...', '0x7099...'] as const.

8. [Dead Code] VITEST_SHARD_ID multiplier in poolId always resolves to 1

VITEST_SHARD_ID is not a Vitest environment variable and is never set in CI or scripts. The multiplication is always x * 1.

erc8004/tests/setup/constants.ts

Lines 35 to 37 in 7399134

	export const poolId =
	Number(process.env.VITEST_POOL_ID ?? 1) *
	Number(process.env.VITEST_SHARD_ID ?? 1)

Fix: export const poolId = Number(process.env.VITEST_POOL_ID ?? 1)

SDK Design

9. [SDK Design] Missing ReturnType export aliases diverges from viem convention

All functions have Parameters types (GiveFeedbackParameters, etc.) but return types (ReputationSummary, Feedback, FeedbackEntry) use ad-hoc names without ReturnType suffixes. viem exports GetEnsAddressReturnType, ReadContractReturnType, etc. for every action.

https://github.com/BackTrackCo/erc8004/blob/7399134b6c81339005bf91ef0ffa7b56a0a19961/src/reputation/types.ts

Fix: Add ReturnType aliases alongside named types (e.g., export type GetSummaryReturnType = ReputationSummary).

10. [SDK Design] readAllFeedback assumes all 7 contract arrays are equal length without validation

The .map() indexes into 6 other arrays by position. If the contract returns mismatched lengths (devnet bug, mock error), fields silently become undefined while TypeScript still types them as bigint/string.

erc8004/src/reputation/readAllFeedback.ts

Lines 28 to 51 in 7399134

	tag2s,
	revokedStatuses,
	] = await publicClient.readContract({
	address: registry,
	abi: reputationRegistryAbi,
	functionName: 'readAllFeedback',
	args: [
	parameters.agentId,
	[...parameters.clientAddresses],
	parameters.tag1,
	parameters.tag2,
	parameters.includeRevoked,
	],
	})
	return clients.map((client, i) => ({
	client,
	feedbackIndex: feedbackIndexes[i],
	value: values[i],
	valueDecimals: valueDecimals[i],
	tag1: tag1s[i],
	tag2: tag2s[i],
	isRevoked: revokedStatuses[i],
	}))

Fix: Add a length invariant check before the .map().

---

What passes cleanly:

• No ethers.js imports
• All functions import ABI from ../abis/index.js (no inline ABIs)
• Consistent function signatures: (WalletClient, params) => Hash / (PublicClient, params) => T
• requireAccount pattern used correctly in all write functions
• Operation-based architecture with clean identity/reputation boundary
• Stateless functions throughout (no class state, no closures)
• All 9 functions + 11 types properly exported through barrel files
• Mock extraction from identity.test.ts to setup/mocks.ts is complete

---

Generated with Claude Code using review-sdk skill

[vraspar]

Addressed in 413db4f. Accepted 7, pushed back on 3:

Accepted:

• Add remaining identity functions (Step 2) #2 Exact values: toBe(1n) instead of toBeGreaterThan(0n) — lifecycle is deterministic
• Add reputation functions + fork tests (Steps 3-5) #3 Post-revocation readAllFeedback: added 2 fork tests (includeRevoked: false → empty, true → 1 revoked entry). Now 19 fork tests.
• Fork test setup: pollingInterval + FORK_URL warning #4 zeroHash: replaced manual hex with viem import
• Prepare v0.1.0 for publish #5 Unnecessary spread: confirmed readonly Address[] compiles fine without spread. Removed from all 3 files.
• feat: add Agent Registration File utilities #6 Removed unused accounts[2]
• feat: add data: and ipfs:// URI support in fetchRegistrationFile #8 Simplified poolId — removed dead VITEST_SHARD_ID multiplier

Pushed back:

• Add ABIs + registerAgent + isRegistered (Step 1) #1 (write tests only check receipt): The fork tests are a sequential lifecycle — giveFeedback is immediately followed by getClients, getLastIndex, readFeedback, getSummary, and readAllFeedback which all read back the written state. Adding reads inside the write test would duplicate these assertions.
• revert: undo direct push to main #7 (dead privateKey fields): Simplifying to string[] requires updating every accounts[n].address call site across all tests. The private keys are forward-compatible — needed if we ever use privateKeyToAccount instead of Anvil. This is test infrastructure, not shipped code.
• #9 (ReturnType aliases): Our return types (ReputationSummary, Feedback, FeedbackEntry) already have meaningful names. viem needs ReturnType aliases because its generics do type-level ABI decoding — ours are concrete types. Adding GetSummaryReturnType = ReputationSummary is boilerplate with no benefit. Identity module uses ResolvedAgent without a ResolveAgentReturnType — consistent.
• #10 (array length validation): The contract guarantees equal-length parallel arrays — it's a fundamental invariant. Adding a runtime check here but not in resolveAgent (3 parallel calls), readFeedback (5 return values), etc. would be inconsistent. Per our conventions: "don't add validation for scenarios that can't happen."

Tests: 38 unit + 19 fork, all passing.

[A1igator]

SDK Review (Round 3)

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

Dead Code

1. [Dead Code] getTestClient() in anvil.ts is never called

getTestClient(), its AnvilInstance interface signature, and the createTestClient/TestClient viem imports exist solely for this method -- but no test file ever calls it. 9 dead lines + 2 dead imports.

erc8004/tests/setup/anvil.ts

Line 26 in 413db4f

getTestClient(): TestClient

2. [Dead Code] forkBlockNumber option in defineAnvil never passed

defineAnvil accepts forkBlockNumber?: bigint but the only call site (anvilBaseSepolia) never passes it. The parameter, its destructure, and its forwarding to prool are dead code today.

erc8004/tests/setup/anvil.ts

Line 37 in 413db4f

forkBlockNumber?: bigint

SDK Design

3. [SDK Design] GiveFeedbackParameters requires endpoint, feedbackURI, feedbackHash but the common case is empty/zero

The fork test passes endpoint: '', feedbackURI: '', feedbackHash: zeroHash -- the common usage. Forcing callers to always supply three empty/zero fields is unnecessary friction. The identity module already has precedent: metadata? in RegisterAgentParameters is optional.

erc8004/src/reputation/types.ts

Lines 6 to 16 in 413db4f

	export interface GiveFeedbackParameters {
	registryAddress?: Address
	agentId: bigint
	value: bigint
	valueDecimals: number
	tag1: string
	tag2: string
	endpoint: string
	feedbackURI: string
	feedbackHash: Hex

Fix: Make these endpoint?: string, feedbackURI?: string, feedbackHash?: Hex and default to ''/zeroHash in giveFeedback.ts.

4. [SDK Design] appendResponse JSDoc says "typically" but hides a hard revert

The doc says "Typically called by the agent owner" -- the word "typically" hides that the contract enforces this with a revert. Callers have no SDK-level signal this will fail if they're not the owner.

erc8004/src/reputation/appendResponse.ts

Lines 7 to 9 in 413db4f

	/**
	* Append a response to existing feedback. Typically called by the agent
	* owner to respond to a reviewer's feedback.

Fix: Change to "Reverts if msg.sender is not the owner of agentId."

5. [SDK Design] ReadAllFeedbackParameters.includeRevoked is required but the safe default is false

Every caller must write includeRevoked: false explicitly. The overwhelmingly common case is active feedback. viem uses optional?: boolean with defaults for boolean flags (e.g. strict in decodeAbiParameters).

erc8004/src/reputation/types.ts

Line 62 in 413db4f

tag2: string

Fix: includeRevoked?: boolean defaulting to false.

Conventions

6. [Conventions] .toLowerCase() for address comparison instead of viem's isAddressEqual

Fork tests manually lowercase addresses for comparison while the identity fork tests in the same PR use plain toBe with checksummed addresses. Inconsistent and bypasses viem's address normalization.

erc8004/tests/fork/reputation.fork.test.ts

Lines 59 to 61 in 413db4f

	expect(clients.map((c) => c.toLowerCase())).toContain(
	accounts[1].address.toLowerCase(),
	)

erc8004/tests/fork/reputation.fork.test.ts

Lines 112 to 114 in 413db4f

	expect(entries[0].client.toLowerCase()).toBe(
	accounts[1].address.toLowerCase(),
	)

Fix: import { isAddressEqual } from 'viem' and use expect(isAddressEqual(a, b)).toBe(true).

7. [Conventions] readAllFeedback returns mutable FeedbackEntry[] but should be readonly

getClients returns Promise<readonly Address[]> matching viem's read return convention. readAllFeedback breaks this by returning mutable FeedbackEntry[].

erc8004/src/reputation/readAllFeedback.ts

Line 14 in 413db4f

publicClient: PublicClient,

Fix: Promise<readonly FeedbackEntry[]> (no implementation change needed).

Tests

8. [Tests] includeRevoked: false unit test never asserts the flag reaches readContract

The includeRevoked: true test asserts expect(client.readContract).toHaveBeenCalledWith(expect.objectContaining({ args: expect.arrayContaining([true]) })). The includeRevoked: false test has no equivalent assertion -- if the implementation hardcoded true, this test would still pass.

erc8004/tests/reputation.test.ts

Lines 175 to 225 in 413db4f

	})
	// --- readFeedback ---
	describe('readFeedback', () => {
	it('returns structured feedback', async () => {
	const client = mockPublic({
	readFeedback: [85n, 0, 'x402r.resolution', 'quality', false],
	})
	const result = await readFeedback(client, {
	registryAddress: REPUTATION_REGISTRY,
	agentId: 42n,
	clientAddress: ADDR_A,
	feedbackIndex: 1n,
	})
	expect(result).toEqual({
	value: 85n,
	valueDecimals: 0,
	tag1: 'x402r.resolution',
	tag2: 'quality',
	isRevoked: false,
	})
	})
	})
	// --- readAllFeedback ---
	describe('readAllFeedback', () => {
	it('transforms parallel arrays to structured objects', async () => {
	const client = mockPublic({
	readAllFeedback: [
	[ADDR_A, ADDR_B], // clients
	[1n, 1n], // feedbackIndexes
	[85n, 90n], // values
	[0, 0], // valueDecimals
	['x402r.resolution', 'x402r.resolution'], // tag1s
	['quality', 'quality'], // tag2s
	[false, false], // revokedStatuses
	],
	})
	const result = await readAllFeedback(client, {
	registryAddress: REPUTATION_REGISTRY,
	agentId: 42n,
	clientAddresses: [ADDR_A, ADDR_B],
	tag1: 'x402r.resolution',
	tag2: 'quality',
	includeRevoked: false,
	})

Fix: Add matching expect.arrayContaining([false]) assertion.

9. [Tests] No negative test for unauthorized revokeFeedback

The fork suite only tests the happy path (feedback giver revokes their own feedback). No test verifies that a non-author calling revokeFeedback gets a revert. This is the single most meaningful missing negative test.

erc8004/tests/fork/reputation.fork.test.ts

Lines 156 to 175 in 413db4f

	it('revokeFeedback marks feedback as revoked', async () => {
	const lastIndex = await getLastIndex(publicClient, {
	agentId,
	clientAddress: accounts[1].address,
	})
	const hash = await revokeFeedback(feedbackGiverClient, {
	agentId,
	feedbackIndex: lastIndex,
	})
	const receipt = await publicClient.waitForTransactionReceipt({ hash })
	expect(receipt.status).toBe('success')
	const feedback = await readFeedback(publicClient, {
	agentId,
	clientAddress: accounts[1].address,
	feedbackIndex: lastIndex,
	})
	expect(feedback.isRevoked).toBe(true)
	})

Fix: Add a one-liner test that agentOwnerClient cannot revoke feedbackGiverClient's feedback.

10. [Tests] getSummary fork test omits summaryValueDecimals assertion

Feedback was submitted with valueDecimals: 0, but getSummary only asserts count and summaryValue. The third field summaryValueDecimals goes unchecked -- a contract bug corrupting this field would be invisible.

erc8004/tests/fork/reputation.fork.test.ts

Lines 90 to 100 in 413db4f

	it('getSummary aggregates feedback', async () => {
	const summary = await getSummary(publicClient, {
	agentId,
	clientAddresses: [accounts[1].address],
	tag1: 'x402r.resolution',
	tag2: 'quality',
	})
	expect(summary.count).toBe(1n)
	expect(summary.summaryValue).toBe(85n)
	})

Fix: Add expect(summary.summaryValueDecimals).toBe(0).

---

Generated with Claude Code using review-sdk skill

[vraspar]

Addressed in 8594113. Accepted 8, pushed back on 2 (plus #9 turned out to be wrong).

Accepted:

• Add reputation functions + fork tests (Steps 3-5) #3 Optional endpoint/feedbackURI/feedbackHash — default to ''/''/zeroHash
• Fork test setup: pollingInterval + FORK_URL warning #4 JSDoc fix — "Reverts if msg.sender is not the owner"
• Prepare v0.1.0 for publish #5 Optional includeRevoked — defaults to false
• feat: add Agent Registration File utilities #6 isAddressEqual instead of toLowerCase
• revert: undo direct push to main #7 readonly FeedbackEntry[] return type
• feat: add data: and ipfs:// URI support in fetchRegistrationFile #8 Added includeRevoked: false flag assertion to unit test
• #10 Added summaryValueDecimals assertion to fork test

Pushed back:

• Add ABIs + registerAgent + isRegistered (Step 1) #1 (getTestClient() dead code): Test infrastructure pattern from x402r-sdk. Used for time manipulation (increaseTime, mine) — will be needed for time-dependent tests. 9 lines of test infra, not shipped.
• Add remaining identity functions (Step 2) #2 (forkBlockNumber option): It's an optional param on a factory function. Not passing an optional param isn't dead code — we may pin to a specific block later for deterministic fork state.

#9 — Wrong assumption: Attempted the unauthorized revoke test — the contract does NOT revert when a non-author calls revokeFeedback. The tx succeeds (likely a no-op scoped to msg.sender's own feedback, or the contract allows the agent owner to revoke). Either way, the reviewer's "hard revert" assumption was incorrect, so the test was removed.

Tests: 38 unit + 19 fork, all passing.

[vraspar]

Nitpicks (non-blocking)

Code looks good — consistent with the identity module, all conventions followed. A few minor things for consideration:

1. Fork FORK_URL silently falls back to public RPC

anvil.ts has process.env.FORK_URL ?? 'https://sepolia.base.org'. The design plan says fork tests should skip when FORK_URL is unset. Running pnpm test:fork locally without it will silently hit the public Base Sepolia RPC (rate-limited, flaky). A console.warn('FORK_URL not set, using public RPC') would make the fallback visible.

2. No pollingInterval on fork public client

anvil.ts creates the public client with cacheTime: 0 but default pollingInterval (4s). Every waitForTransactionReceipt waits up to 4 seconds. Adding pollingInterval: 100 would speed up fork tests — Anvil mines instantly.

3. Fork test agentId dependency chain

Both fork suites set agentId in the first it(...) block — all subsequent tests depend on it. If registration fails, every following test errors on undefined. Moving registration into beforeAll would make failures clearer.

4. giveFeedback optional defaults not visible in types

endpoint, feedbackURI, feedbackHash default to '', '', zeroHash inside the function, but the type just shows endpoint?: string. A consumer reading only the type doesn't know the defaults. JSDoc @default annotations would close the gap.

---

Generated with Claude Code using review-sdk skill