feat(identity): ERC-8004 identity + reputation MCP tools

[arcayne]

Summary

• Adds 8 ERC-8004 identity MCP tools: agent_register, agent_update, agent_status, agent_list, agent_reputation, agent_feedback_list, agent_give_feedback, agent_revoke_feedback
• Integrates agent card generation and IPFS (Pinata) upload; supports metadata fields (description, image, services, actions, supportedTrust, tags, version, license, sourceCode, documentation, x402, active)
• Converges onto @injective/agent-sdk for identity/reputation registry interaction; adds viem client factory, EIP-712 wallet-link helper, and error classes

Test plan

• pnpm typecheck clean
• pnpm build clean
• pnpm test — 321 passed / 6 skipped (integration suite runs via test:integration)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • On-chain ERC-8004 agent identity: register/update agents, wallet linking, status/listing, reputation queries, submit/revoke feedback

• Documentation

  • README updated to advertise identity features, pnpm setup, required environment variables, and an integration guide

• Tests

  • Extensive unit, demo, integration, and end-to-end tests covering identity flows and validation

• Chores

  • Added runtime dependencies and updated .gitignore/package manager metadata

[coderabbitai]

Warning

Rate limit exceeded

@arcayne has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 31 minutes and 30 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 82aaeaf8-8666-49eb-960a-ff8b8474e4a4

📥 Commits

Reviewing files that changed from the base of the PR and between b13f153 and 1176f47.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (4)

• package.json
• src/identity/identity.test.ts
• src/identity/index.ts
• src/identity/read.test.ts

📝 Walkthrough

Walkthrough

Adds an ERC-8004 identity subsystem: new identity read/write adapters backed by @injective/agent-sdk, MCP identity tools and validation, Pinata/IPFS integration, two identity error types, extensive unit/integration/demo tests, documentation, and package/README/.gitignore updates.

Changes

Cohort / File(s)	Summary
Repository metadata \.gitignore, README.md, package.json	Ignore package-lock.json, Vitest build cache *.timestamp-*.mjs, and scripts//docs/; README documents ERC-8004 identity tools and env vars; adds @injective/agent-sdk (git dep), viem, and pnpm metadata.
Error types & tests src/errors/index.ts, src/errors/errors.test.ts	Add IdentityNotFound and IdentityTxFailed error classes and tests asserting codes, names, and message contents; include new errors in the instanceof-Error suite.
Identity read adapter & tests src/identity/read.ts, src/identity/read.test.ts	New identityRead adapter with cached AgentReadClient per network; implements status/list/reputation/feedbackList with owner address conversion, type over-fetch+filtering, bigint/decimal normalization, and IdentityNotFound translation; tests validate mappings and error behavior.
Identity write adapter & tests src/identity/index.ts, src/identity/identity.test.ts, src/identity/demo.test.ts	New identity adapter exposing register, update, giveFeedback, revokeFeedback; handles optional Pinata upload, keystore unlock, AgentClient delegation, wallet-link logic, SDK error wrapping to IdentityTxFailed; unit/demo tests with SDK/wallet mocks.
Integration & E2E tests + docs src/identity/integration.test.ts, src/integration/identity.integration.test.ts, src/identity/INTEGRATION.md	Add conditional E2E integration tests that exercise full agent lifecycle on testnet + Pinata and a detailed INTEGRATION.md describing setup, env vars, execution modes, and troubleshooting.
MCP server tools & validation src/mcp/server.ts, src/mcp/server.test.ts	Register identity MCP tools (agent_register, agent_update, agent_status, agent_list, agent_reputation, agent_feedback_list, agent_give_feedback, agent_revoke_feedback), add Zod schemas for agent metadata/services/actions/addresses, and tests for schema rules.
Tests only (added suites) src/identity/*.test.ts, src/integration/*, src/mcp/server.test.ts	Multiple new unit, demo, integration, and E2E test suites covering identity adapters, read/write flows, schema validation, and end-to-end interactions with real testnet/IPFS (conditional).

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant MCP_Server
  participant IdentityModule
  participant Keystore
  participant Pinata
  participant AgentSDK
  participant Chain

  Client->>MCP_Server: call agent_register(params, address, password)
  MCP_Server->>IdentityModule: validate & forward params
  IdentityModule->>Keystore: unlock(address, password)
  Keystore-->>IdentityModule: signer
  alt upload metadata to IPFS
    IdentityModule->>Pinata: upload metadata
    Pinata-->>IdentityModule: cardUri
  end
  IdentityModule->>AgentSDK: register(agentMeta, signer)
  AgentSDK->>Chain: submit tx
  Chain-->>AgentSDK: txHash / receipt
  AgentSDK-->>IdentityModule: agentId, txHash
  IdentityModule-->>MCP_Server: formatted result (agentId, txHash, cardUri)
  MCP_Server-->>Client: JSON-stringified response

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I hopped a patch across the dev field,

cards on IPFS and contracts revealed,
register, update, feedback in tune,
tests and docs beneath the moon,
a little rabbit cheers — identities bloom!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(identity): ERC-8004 identity + reputation MCP tools' clearly and concisely summarizes the main change: adding ERC-8004 identity and reputation functionality via new MCP tools.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/erc-8004-identity-tools

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​injectivelabs/​core-proto-ts-v2@​1.17.3 ⏵ 1.18.0			+1	+1
	@​injectivelabs/​ts-types@​1.17.8 ⏵ 1.18.24	+1		+1	+1
	@​injectivelabs/​networks@​1.17.8 ⏵ 1.18.24				+1
	@​injectivelabs/​utils@​1.17.8 ⏵ 1.18.24	+1		+1	+1
	@​injectivelabs/​sdk-ts@​1.17.8 ⏵ 1.18.24	-2		+1	+1
	@​modelcontextprotocol/​sdk@​1.26.0 ⏵ 1.29.0
	viem@​2.46.2 ⏵ 2.48.4	+1		+1	+1

View full report

[coderabbitai]

Actionable comments posted: 8

♻️ Duplicate comments (1)

src/identity/index.ts (1)

271-291: ⚠️ Potential issue | 🟡 Minor

Root cause for the value integer issue flagged in server.ts.

BigInt(params.value) throws RangeError for any non-integer number. The schema fix in agent_give_feedback (z.number().int()) is the right place to enforce this; see the comment there. Noting here so it's clear why: this line is the source that makes the schema constraint necessary. Consider also defensively coercing with Math.trunc(params.value) if you want to tolerate rounding, but schema-level rejection is cleaner.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 271 - 291, The BigInt(params.value) call
in giveFeedback is throwing RangeError for non-integer numbers; update the
giveFeedback implementation to either assume schema-level validation or
defensively coerce to an integer before BigInt: replace BigInt(params.value)
with BigInt(Math.trunc(params.value)) (or otherwise assert
Number.isInteger(params.value) and throw a clear error) in the giveFeedback
method that calls createClient so non-integer inputs no longer cause RangeError
at runtime.

🧹 Nitpick comments (5)

src/identity/integration.test.ts (1)

38-38: Nit: confirm describe.skipIf semantics with a string.

SKIP_REASON is string | null. describe.skipIf skips on truthy values, so this works in practice — but passing the reason string as the condition obscures intent. Consider passing a boolean and logging the reason separately, e.g.:

const shouldSkip = !TEST_ADDRESS || !TEST_PASSWORD || !PINATA_JWT
if (shouldSkip) console.log(`Skipping identity integration tests: ${SKIP_REASON}`)
describe.skipIf(shouldSkip)('Identity Integration Tests (Testnet)', ...)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/integration.test.ts` at line 38, The test currently passes
SKIP_REASON (a string|null) directly to describe.skipIf which relies on
truthiness and obscures intent; compute an explicit boolean (e.g., shouldSkip)
from the required checks (TEST_ADDRESS, TEST_PASSWORD, PINATA_JWT) and pass that
to describe.skipIf, and separately log SKIP_REASON when shouldSkip is true to
preserve the human-readable reason; update the test declaration using shouldSkip
instead of SKIP_REASON and add a console.log or test logger for SKIP_REASON when
skipping.

src/identity/demo.test.ts (1)

74-145: Heavy console.log output in a standard unit test will clutter every npm test run.

This file is picked up by the default vitest run (not gated behind an env var the way integration.test.ts is), so the emoji-rich log lines will print on every developer/CI run. Consider either:

• Gating the logs behind if (process.env.DEBUG) / process.env.VITEST_VERBOSE, or
• Moving this into the integration suite if it's intended as a walkthrough rather than a regression test, or
• Dropping the logs entirely and relying on the assertions (they fully cover the flow).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/demo.test.ts` around lines 74 - 145, The test emits many
emoji-rich console.log calls (e.g., lines around identity.register and
identityRead.status printing registerResult and statusResult) which will clutter
every vitest run; either remove those console.log statements, or wrap them with
a runtime guard such as if (process.env.VITEST_VERBOSE || process.env.DEBUG)
before the blocks that log the registration and status outputs, or move this
entire test into the integration suite; update references to the mocked call
assertions (mockRegister) to remain unchanged and ensure the test still asserts
registerResult and statusResult values after removing/gating logs.

README.md (1)

156-166: Optional: architecture tree omits the new identity/ module.

Consider adding identity/ ERC-8004 agent identity + reputation (wraps @injective/agent-sdk) to the tree so the diagram stays in sync with the added surface.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 156 - 166, Update the architecture tree in the README
to include the new identity module: add a line like "├── identity/    ERC-8004
agent identity + reputation (wraps `@injective/agent-sdk`)" in the same list where
other modules (config, keystore, wallets, markets, accounts, trading,
evm/eip712, orders, transfers, bridges, evm) are declared so the diagram and
module surface reflect the added identity/ module.

src/identity/read.ts (1)

92-113: Optional: 3× over-fetch for type filtering is fragile when the requested type is rare.

If only ~5% of agents on the registry match type='analytics' and the caller asks for limit: 20, fetchLimit = 60 will likely yield fewer than 20 matches and the caller has no way to know whether they got a truncated page. total here is post-filter count from the over-fetched slice, not the true total matching the filter on-chain.

Given the SDK doesn't index by type, this is a pragmatic approach, but consider either:

• Documenting the caveat in the tool description (results are best-effort when type is set), or
• Paginating until limit is reached or the SDK returns fewer than fetchLimit.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/read.ts` around lines 92 - 113, The current 3x over-fetch using
fetchLimit when params.type is set can underdeliver when the type is rare;
change the logic in the read handler that calls sdk.getAgentsByOwner /
sdk.listAgents to page and accumulate results until either you have at least
limit matching agents (filtering by params.type) or the SDK returns fewer items
than the page size (no more data), then slice to limit and set filteredTotal to
the total matching count you accumulated (not just the sliced length); ensure
you preserve the existing agents mapping (agentId, name, agentType, owner) and
stop paginating when no more pages are returned to avoid infinite loops.

src/mcp/server.ts (1)

985-985: Minor: owner schema accepts arbitrary strings.

identityRead.list only handles two shapes: a string starting with inj (converted via evm.injAddressToEth) or anything else passed through as a 0x address. Any other input (e.g., a typo, an ENS name, empty string) is forwarded to sdk.getAgentsByOwner as a malformed hex address and surfaces as a cryptic SDK error.

Recommend validating at the schema layer to match the tool description (inj1... or 0x...):

🛡️ Proposed fix

-    owner: z.string().optional().describe('Filter by owner — accepts inj1... or 0x... address.'),
+    owner: z.string()
+      .regex(/^(inj1[a-z0-9]{38}|0x[a-fA-F0-9]{40})$/, 'Must be inj1... or 0x... address')
+      .optional()
+      .describe('Filter by owner — accepts inj1... or 0x... address.'),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp/server.ts` at line 985, The owner schema currently allows arbitrary
strings; restrict it to only the two accepted shapes so identityRead.list won't
forward invalid values to sdk.getAgentsByOwner. Replace z.string().optional()
for the owner field with a union/refinement that accepts either an inj-prefixed
address (string starting with "inj") or a 0x hex Ethereum address (e.g., regex
/^0x[0-9a-fA-F]{40}$/), and keep it optional; mention the same symbols in the
code (the owner schema field and the identityRead.list flow) so callers are
validated at the schema layer before evm.injAddressToEth or sdk.getAgentsByOwner
are invoked.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@package.json`:
- Line 22: The package.json currently uses an unresolved local file dependency
"@injective/agent-sdk": "file:../injective-agent-cli/packages/sdk" which breaks
CI and npm installs; replace that entry with a machine-independent reference —
e.g. if you move both repos into a monorepo, change to "workspace:*" and add a
pnpm-workspace.yaml; or publish `@injective/agent-sdk` and pin a published version
(e.g. "0.x.y"); or point to the git URL with path
(git+https://github.com/<org>/injective-agent-cli.git#<tag>&path=/packages/sdk)
— update the "@injective/agent-sdk" value accordingly and ensure CI workflows
reflect the chosen approach.

In `@README.md`:
- Around line 59-67: Add the three missing identity tools to the Identity
(ERC-8004) table and add a blank line after the heading to satisfy MD058: insert
rows for `agent_reputation` (read-only: returns aggregated reputation — score,
count, clients), `agent_feedback_list` (read-only: lists individual feedback
entries with value, tags, revocation status; no gas), and `agent_give_feedback`
(on-chain feedback submission; costs gas), and ensure there is an empty line
between the "### Identity (ERC-8004)" heading and the table header so
markdownlint MD058 is resolved.

In `@src/identity/demo.test.ts`:
- Around line 1-8: Update the top-of-file doc comment to reference the correct
test filename: change the run command "npm test -- src/identity/demo.ts" to "npm
test -- src/identity/demo.test.ts" (edit the comment block at the top of
demo.test.ts). Ensure the example command matches the actual test filename so
the run pattern works as expected.

In `@src/identity/INTEGRATION.md`:
- Around line 91-97: Update the "Increase Timeout" section so the shown npm test
command actually increases Vitest's per-test timeout: replace or augment the
suggested flags with Vitest's timeout flag (for example use
--testTimeout=300000) or explicitly instruct readers to edit the timeout in the
test file src/identity/integration.test.ts; mention the exact test file name and
the --testTimeout=300000 flag so consumers know where to change it or how to run
tests with a longer timeout.

In `@src/identity/integration.test.ts`:
- Around line 38-89: The tests leak state and rely on cross-test shared variable
agentId set inside the "it('registers agent on testnet with full metadata')"
test; move registration into a deterministic setup and add cleanup: register the
agent in beforeAll (call identity.register with TEST_ADDRESS/TEST_PASSWORD to
produce agentId and keep the existing assertions as separate it()s if you want
reporting) or collapse into a single e2e lifecycle it(); then add an afterAll
that calls identity.deregister(agentId, { confirm: true }) (or the equivalent
deregister function) as a best-effort cleanup to remove the on-chain agent;
update imports to include afterAll from vitest if needed and reference the
agentId variable produced in setup for all subsequent tests.

In `@src/identity/read.ts`:
- Around line 115-159: Both reputation and feedbackList currently swallow all
errors; update the catch blocks in the reputation and feedbackList methods to
log the error (include agentId and error.message) and only return the zero/empty
result for the SDK's "no entries"/"not found" condition; for all other
exceptions rethrow. Locate the reputation method (calls getReputation) and
feedbackList method (calls getFeedbackEntries) and replace the broad catch(_err)
with logic that (1) captures the thrown error, (2) checks the
SDK/registry-specific "not found" or "no entries" error sentinel/class/string
and if matched returns the current zeroed result, otherwise call
processLogger.error or similar with a structured message including agentId and
the error, then rethrow the error.

In `@src/integration/identity.integration.test.ts`:
- Around line 95-103: The test is flaky because identityRead.list(config, {
limit: 50 }) can miss the newly created agent; update the call to scope results
to the test wallet by passing the owner filter (e.g. identityRead.list(config, {
limit: 50, owner: <wallet address> })), using the existing config wallet/address
field so the search reliably returns agents for this wallet; then assert found
by agentId as before (references: identityRead.list, agentId, config).

In `@src/mcp/server.ts`:
- Around line 1038-1039: The schema currently allows fractional numbers for the
rating `value` which then causes `identity.giveFeedback` to throw when it calls
BigInt(params.value); change the schema entry named `value` in the relevant Zod
object (the lines defining `value: z.number()...`) to enforce integers (e.g.,
use `.int()` like `valueDecimals` does) so non-integer inputs are rejected early
and produce a validation error before `giveFeedback` runs.

---

Duplicate comments:
In `@src/identity/index.ts`:
- Around line 271-291: The BigInt(params.value) call in giveFeedback is throwing
RangeError for non-integer numbers; update the giveFeedback implementation to
either assume schema-level validation or defensively coerce to an integer before
BigInt: replace BigInt(params.value) with BigInt(Math.trunc(params.value)) (or
otherwise assert Number.isInteger(params.value) and throw a clear error) in the
giveFeedback method that calls createClient so non-integer inputs no longer
cause RangeError at runtime.

---

Nitpick comments:
In `@README.md`:
- Around line 156-166: Update the architecture tree in the README to include the
new identity module: add a line like "├── identity/    ERC-8004 agent identity +
reputation (wraps `@injective/agent-sdk`)" in the same list where other modules
(config, keystore, wallets, markets, accounts, trading, evm/eip712, orders,
transfers, bridges, evm) are declared so the diagram and module surface reflect
the added identity/ module.

In `@src/identity/demo.test.ts`:
- Around line 74-145: The test emits many emoji-rich console.log calls (e.g.,
lines around identity.register and identityRead.status printing registerResult
and statusResult) which will clutter every vitest run; either remove those
console.log statements, or wrap them with a runtime guard such as if
(process.env.VITEST_VERBOSE || process.env.DEBUG) before the blocks that log the
registration and status outputs, or move this entire test into the integration
suite; update references to the mocked call assertions (mockRegister) to remain
unchanged and ensure the test still asserts registerResult and statusResult
values after removing/gating logs.

In `@src/identity/integration.test.ts`:
- Line 38: The test currently passes SKIP_REASON (a string|null) directly to
describe.skipIf which relies on truthiness and obscures intent; compute an
explicit boolean (e.g., shouldSkip) from the required checks (TEST_ADDRESS,
TEST_PASSWORD, PINATA_JWT) and pass that to describe.skipIf, and separately log
SKIP_REASON when shouldSkip is true to preserve the human-readable reason;
update the test declaration using shouldSkip instead of SKIP_REASON and add a
console.log or test logger for SKIP_REASON when skipping.

In `@src/identity/read.ts`:
- Around line 92-113: The current 3x over-fetch using fetchLimit when
params.type is set can underdeliver when the type is rare; change the logic in
the read handler that calls sdk.getAgentsByOwner / sdk.listAgents to page and
accumulate results until either you have at least limit matching agents
(filtering by params.type) or the SDK returns fewer items than the page size (no
more data), then slice to limit and set filteredTotal to the total matching
count you accumulated (not just the sliced length); ensure you preserve the
existing agents mapping (agentId, name, agentType, owner) and stop paginating
when no more pages are returned to avoid infinite loops.

In `@src/mcp/server.ts`:
- Line 985: The owner schema currently allows arbitrary strings; restrict it to
only the two accepted shapes so identityRead.list won't forward invalid values
to sdk.getAgentsByOwner. Replace z.string().optional() for the owner field with
a union/refinement that accepts either an inj-prefixed address (string starting
with "inj") or a 0x hex Ethereum address (e.g., regex /^0x[0-9a-fA-F]{40}$/),
and keep it optional; mention the same symbols in the code (the owner schema
field and the identityRead.list flow) so callers are validated at the schema
layer before evm.injAddressToEth or sdk.getAgentsByOwner are invoked.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 37fc7396-fe94-46bb-8aa5-9cbaa09b468a

📥 Commits

Reviewing files that changed from the base of the PR and between 747441e and 032030a.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (15)

• .gitignore
• README.md
• package.json
• src/errors/errors.test.ts
• src/errors/index.ts
• src/identity/INTEGRATION.md
• src/identity/demo.test.ts
• src/identity/identity.test.ts
• src/identity/index.ts
• src/identity/integration.test.ts
• src/identity/read.test.ts
• src/identity/read.ts
• src/integration/identity.integration.test.ts
• src/mcp/server.test.ts
• src/mcp/server.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Doc comment has a stale filename.

Line 7 says npm test -- src/identity/demo.ts but the file is demo.test.ts. The pattern won't match.

✏️ Fix

- *   npm test -- src/identity/demo.ts
+ *   npm test -- src/identity/demo.test.ts

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/**
	* End-to-end demo: register an agent with full metadata, then fetch its status with reputation.
	*
	* This is a Vitest test file demonstrating the full identity workflow with mocked SDK.
	*
	* Run with:
	* npm test -- src/identity/demo.ts
	*/
	/**
	* End-to-end demo: register an agent with full metadata, then fetch its status with reputation.
	*
	* This is a Vitest test file demonstrating the full identity workflow with mocked SDK.
	*
	* Run with:
	* npm test -- src/identity/demo.test.ts
	*/

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/demo.test.ts` around lines 1 - 8, Update the top-of-file doc
comment to reference the correct test filename: change the run command "npm test
-- src/identity/demo.ts" to "npm test -- src/identity/demo.test.ts" (edit the
comment block at the top of demo.test.ts). Ensure the example command matches
the actual test filename so the run pattern works as expected.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Minor: "Increase Timeout" section shows a command that doesn't increase timeout.

--bail 1 --reporter=verbose controls failure-bail and reporter verbosity — it does not change the per-test timeout. Consider replacing with the Vitest timeout flag (e.g., --testTimeout=300000) or clarify that the timeout must be edited in the test file itself.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/INTEGRATION.md` around lines 91 - 97, Update the "Increase
Timeout" section so the shown npm test command actually increases Vitest's
per-test timeout: replace or augment the suggested flags with Vitest's timeout
flag (for example use --testTimeout=300000) or explicitly instruct readers to
edit the timeout in the test file src/identity/integration.test.ts; mention the
exact test file name and the --testTimeout=300000 flag so consumers know where
to change it or how to run tests with a longer timeout.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Integration suite leaks testnet state and is fragile to test filtering / reordering.

Two related concerns that will bite you after a few CI runs:

1. No cleanup. Every successful run registers a fresh agent on testnet and never deregisters it. Gas is wasted and the registry accumulates garbage indefinitely. The PR description says deregister is covered with confirm: true, but there’s no corresponding it(...) here. Add a final step (either an afterAll best-effort cleanup, or an it('deregisters the agent', ...) that asserts the tx and closes the lifecycle).

2. Cross-test shared state. agentId is assigned inside the first it and read by every subsequent it. If a user runs a single test via -t 'fetches registered agent', or the register test is skipped/fails, the downstream tests will silently use undefined as agentId, producing misleading failures (e.g. "newAgent is undefined" rather than "register failed"). Prefer either:

   • Doing the one-time setup in beforeAll (register there, keep the assertions as separate its for reporting), or
   • Consolidating the full lifecycle into a single it('e2e lifecycle', ...) so all steps share a deterministic scope.

🧹 Sketch: cleanup via afterAll

-  beforeAll(async () => {
+  beforeAll(async () => {
     console.log(`\n🔐 Verifying wallet at ${TEST_ADDRESS}...`)
     wallets.unlock(TEST_ADDRESS!, TEST_PASSWORD!)
     evmAddress = evm.injAddressToEth(TEST_ADDRESS!)
     console.log('✅ Wallet unlocked successfully\n')
   })
+
+  afterAll(async () => {
+    if (!agentId) return
+    try {
+      await identity.deregister(config, {
+        address: TEST_ADDRESS!,
+        password: TEST_PASSWORD!,
+        agentId,
+        confirm: true,
+      })
+    } catch (err) {
+      console.warn(`⚠️  Failed to deregister test agent ${agentId}:`, err)
+    }
+  })

(Remember to add afterAll to the vitest import.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/integration.test.ts` around lines 38 - 89, The tests leak state
and rely on cross-test shared variable agentId set inside the "it('registers
agent on testnet with full metadata')" test; move registration into a
deterministic setup and add cleanup: register the agent in beforeAll (call
identity.register with TEST_ADDRESS/TEST_PASSWORD to produce agentId and keep
the existing assertions as separate it()s if you want reporting) or collapse
into a single e2e lifecycle it(); then add an afterAll that calls
identity.deregister(agentId, { confirm: true }) (or the equivalent deregister
function) as a best-effort cleanup to remove the on-chain agent; update imports
to include afterAll from vitest if needed and reference the agentId variable
produced in setup for all subsequent tests.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Minor: silent catch in reputation and feedbackList hides operational failures.

Both handlers swallow every error and return zeroed/empty results. That conflates three distinct states into one:

1. Agent genuinely has no feedback yet (expected happy path).
2. RPC endpoint is down or rate-limited.
3. Contract address misconfigured / not deployed for this network.

Callers and operators can't distinguish these. At minimum, log the error (structured, with agentId and error message) so broken-registry incidents aren't invisible. Better: narrow the swallow to the SDK's "no entries"/"not found" error class and rethrow the rest.

♻️ Suggested shape

-    } catch (_err) {
-      // Reputation is best-effort — return zeros if the agent has no feedback or registry errors
-      return { agentId: params.agentId, score: 0, count: 0, clients: [] }
-    }
+    } catch (err) {
+      const msg = err instanceof Error ? err.message : String(err)
+      // Only treat "not found" / "no entries" as the zero-feedback case
+      if (/nonexistent|not found|no entries|invalid token/i.test(msg)) {
+        return { agentId: params.agentId, score: 0, count: 0, clients: [] }
+      }
+      throw err
+    }

Same pattern applies to feedbackList at lines 155-158.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/read.ts` around lines 115 - 159, Both reputation and
feedbackList currently swallow all errors; update the catch blocks in the
reputation and feedbackList methods to log the error (include agentId and
error.message) and only return the zero/empty result for the SDK's "no
entries"/"not found" condition; for all other exceptions rethrow. Locate the
reputation method (calls getReputation) and feedbackList method (calls
getFeedbackEntries) and replace the broad catch(_err) with logic that (1)
captures the thrown error, (2) checks the SDK/registry-specific "not found" or
"no entries" error sentinel/class/string and if matched returns the current
zeroed result, otherwise call processLogger.error or similar with a structured
message including agentId and the error, then rethrow the error.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm how sdk.listAgents orders results — if asc by agentId, the flakiness above is real.
fd -t f -e ts -e js 'listAgents' | head -n 20
rg -nP --type=ts -C4 '\blistAgents\s*\(' | head -n 200

Repository: InjectiveLabs/mcp-server

Length of output: 1205

---

🏁 Script executed:

# Read the integration test to see the exact context and full test
head -n 110 src/integration/identity.integration.test.ts | tail -n 25

Repository: InjectiveLabs/mcp-server

Length of output: 893

---

🏁 Script executed:

# Check the list() method signature in identity/read.ts
rg -nP --type=ts 'list\s*\(' src/identity/read.ts -A 10 | head -n 50

Repository: InjectiveLabs/mcp-server

Length of output: 506

---

🏁 Script executed:

# Look for type definitions or interface for list() method
rg -nP --type=ts 'interface.*List|type.*List' src/identity/read.ts -B 2 -A 5 | head -n 60

Repository: InjectiveLabs/mcp-server

Length of output: 1148

---

🏁 Script executed:

# Search for SDK agent ordering or sorting behavior
rg -nP --type=ts 'getAgentsByOwner|listAgents' src/ -A 5 | grep -iE 'sort|order|asc|desc' | head -n 20

Repository: InjectiveLabs/mcp-server

Length of output: 50

---

🏁 Script executed:

# Look for other integration tests that might use owner filtering
grep -rn 'identityRead.list' src/integration/ --include="*.ts"

Repository: InjectiveLabs/mcp-server

Length of output: 181

---

🏁 Script executed:

# Check the SDK client type for listAgents or getAgentsByOwner signatures
rg -nP 'interface.*AgentReadClient|type.*AgentReadClient' src/ -A 30 | head -n 80

Repository: InjectiveLabs/mcp-server

Length of output: 50

---

🏁 Script executed:

# Find SDK package dependency
rg -nP 'injective|agent' package.json

Repository: InjectiveLabs/mcp-server

Length of output: 394

---

🏁 Script executed:

# Find imports of AgentReadClient or SDK types
rg -nP "import.*from.*['\"]" src/identity/read.ts | head -n 10

Repository: InjectiveLabs/mcp-server

Length of output: 272

---

🏁 Script executed:

# Check if there's any SDK documentation or type hints in the repo
find . -name "*.d.ts" -o -name "*types*" | grep -i agent | head -n 10

Repository: InjectiveLabs/mcp-server

Length of output: 50

---

🏁 Script executed:

# Check test setup and see if there are any comments about testnet state or flakiness
head -n 50 src/integration/identity.integration.test.ts

Repository: InjectiveLabs/mcp-server

Length of output: 1878

---

🏁 Script executed:

# Search for retry mechanisms or flakiness handling in integration tests
rg -nP 'retry|flaky|timeout|maxRetries' src/integration/ --type=ts

Repository: InjectiveLabs/mcp-server

Length of output: 247

---

🏁 Script executed:

# Look at the beforeAll/beforeEach setup to understand test isolation
rg -nP 'beforeAll|beforeEach|describe.*identity' src/integration/identity.integration.test.ts -A 5 | head -n 40

Repository: InjectiveLabs/mcp-server

Length of output: 260

---

Minor: unfiltered listing may become flaky if testnet accumulates agents.

The test uses identityRead.list(config, { limit: 50 }) without filtering. If the SDK orders agents by ID ascending (or does not guarantee newest-first), the just-registered agent can fall outside the first 50 as testnet grows, causing expect(found).toBeDefined() to fail.

Scope the result to this wallet using the owner parameter, which is already supported:

🧪 Proposed fix

-    const result = await identityRead.list(config, { limit: 50 })
+    const result = await identityRead.list(config, { owner: testEvmAddress, limit: 50 })

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	it('lists agents and finds ours', async () => {
	const result = await identityRead.list(config, { limit: 50 })
	expect(result.agents.length).toBeGreaterThan(0)
	const found = result.agents.find(a => a.agentId === agentId)
	expect(found).toBeDefined()
	expect(found!.name).toBe('UpdatedTestBot')
	expect(found!.agentType).toBe('trading')
	}, 60_000)
	it('lists agents and finds ours', async () => {
	const result = await identityRead.list(config, { owner: testEvmAddress, limit: 50 })
	expect(result.agents.length).toBeGreaterThan(0)
	const found = result.agents.find(a => a.agentId === agentId)
	expect(found).toBeDefined()
	expect(found!.name).toBe('UpdatedTestBot')
	expect(found!.agentType).toBe('trading')
	}, 60_000)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/integration/identity.integration.test.ts` around lines 95 - 103, The test
is flaky because identityRead.list(config, { limit: 50 }) can miss the newly
created agent; update the call to scope results to the test wallet by passing
the owner filter (e.g. identityRead.list(config, { limit: 50, owner: <wallet
address> })), using the existing config wallet/address field so the search
reliably returns agents for this wallet; then assert found by agentId as before
(references: identityRead.list, agentId, config).

[ckhbtc]

Review focused on the same funds-safety / irreversibility / LLM-trust-boundary lens as PR #10. Two blocking items, four tighten-before-merge items. No secret-leakage issues found; self-link wallet restriction and error-taxonomy additions look good.

Blocking

1. Optimistic tx-hash returns on every write path — no receipt confirmation.
2. confirm: boolean is LLM-supplied and not a real safety barrier for an irreversible NFT burn.

Should land in the same revision

3. agentId Zod schema accepts any non-empty string; BigInt() throws surface as misleading IdentityTxFailed.
4. Read paths silently swallow network/RPC errors as score: 0.
5. URL-typed fields accept javascript: / data: / file://.
6. feedbackHash cast to 0x${string} without hex validation.

[ckhbtc]

Blocking — optimistic tx-hash return on all write paths.

register, update (L258), deregister (L267), giveFeedback (L290), and revokeFeedback (L302) all return whatever txHash/txHashes the SDK hands back, with no on-chain confirmation. This is the same class of bug that PR #10 fixed for bridge/close flows.

agent_deregister is particularly risky — the tool is documented as "permanently burn an NFT" but the returned txHash could be from a broadcast that was never included. An LLM caller that sees { agentId, txHash } will treat the burn as complete; if the tx is actually dropped, a subsequent agent_register could collide, or the user could believe the identity is gone when it isn't.

Suggested fix: after each write, either poll client.getStatus(id) (or confirm the NFT is gone, for deregister) before returning, or wait for the receipt and check status === 'success' + the expected event topic (the repo already has fix(identity): add event topic check to giveFeedback log extraction in history, so the pattern is available). Accepting unconfirmed SDK results as success is inconsistent with the project's stance from #10.

[arcayne]

Pushed back on the framing in 4cf1a97 — the SDK already calls publicClient.waitForTransactionReceipt on every write path (register client.ts:202+282, update :494, deregister :549, giveFeedback :619, revokeFeedback :689), and register/giveFeedback additionally extract the expected event topic from the receipt (line 205-209 / 622-626). So "the txHash could be from a broadcast that was never included" doesn't hold for this stack — the SDK blocks until the receipt is mined.

The narrower legitimate concern is that the SDK doesn't check receipt.status === 'success', so a tx that reverts post-simulation would still return a hash. For the irreversible deregister path I added a post-burn client.getStatus(id) re-read that throws DeregisterNotApplied if the agent still exists — that catches both "broadcast not applied" and "tx reverted post-sim". For update/revokeFeedback (recoverable on retry) I left it to the SDK's existing receipt wait. Happy to extend if you want belt-and-suspenders there too.

Filed as a follow-up: SDK should add a receipt.status === 'success' assertion at the source so all consumers benefit.

[arcayne]

Follow-up on this thread — extended the receipt-status check to every write path via SDK 0.2.1 (commit b13f153 here, SDK commit 381a2708).

The SDK now exposes assertReceiptSuccess(receipt, methodName, hash) and calls it after every waitForTransactionReceipt:

• register (initial register tx)
• register follow-up loop: setUri revert → onWarning (matches the existing two-phase URI semantics — registration already succeeded), walletLink revert → throw
• update (per planned write, named by function)
• giveFeedback, revokeFeedback

So the gap that was left after the first round (only deregister had a defense-in-depth post-check, and the SDK silently passed reverted-receipt hashes through) is now closed at the source. Consumers of any version ≥ 0.2.1 get this automatically — no MCP-side post-conditions needed.

Concrete behavior change: if a tx simulates clean but reverts post-broadcast (rare — usually only happens under racing state changes), the SDK now throws ContractError instead of returning the reverted hash as success.

[ckhbtc]

Blocking — confirm: boolean is LLM-controlled and not a meaningful guard.

The confirm flag is supplied by the same LLM context that decided to call agent_deregister in the first place. An adversarial or hallucinating prompt can trivially pass confirm: true; there is no out-of-band human confirmation, no ownership check, and no second factor before the SDK call.

Other destructive tools in this repo (bridge withdrawals, position close) don't take a runtime confirm parameter — they rely on explicit user phrasing + a strong tool description. A runtime confirm: boolean forwarded from the LLM looks like a safety barrier but is weaker than it appears.

Suggested fix: at minimum, do a server-side ownership check before calling client.deregister — fetch status, verify params.address (or the signer derived from it) is the actual NFT owner, and throw a typed error if not. That turns the confirm guard into defense-in-depth rather than the only line of defense. Consider also removing the confirm param entirely and letting tool description + signer-ownership do the work, matching the rest of the codebase.

[arcayne]

Partial accept in 4cf1a97 — added a server-side ownership pre-check before invoking client.deregister: fetches getStatus(id) and throws a typed NotAgentOwner if status.owner !== client.address (case-insensitive). The contract enforces this too, but failing here yields a clean typed error rather than a contract revert wrapped as IdentityTxFailed, and turns confirm: boolean into defense-in-depth instead of the only barrier.

Kept the confirm flag — it matches the --yes/--confirm pattern used by the agent-cli and gives the LLM a clear "this is destructive" signal in the schema. Removing it would lose that signal without adding safety, since the ownership check is the actual gate now.

Edge case from your prior review (PR #10): even the ownership pre-check could be defeated by a hallucinated address that happens to match a key in the keystore. The keystore-unlock requires the password, so an LLM still can't deregister without the user supplying both address and password in the same call.

[ckhbtc]

agentId: z.string().min(1) accepts any non-empty string, but downstream every use is BigInt(params.agentId). Values like "abc", "12n", "1.5" pass Zod validation then throw SyntaxError inside the try block in src/identity/index.ts, which wrapSdkError re-throws as IdentityTxFailed: Cannot convert abc to a BigInt — a misleading "tx failed" error for what is really input validation.

Same issue on every agentId schema in this file (agent_update, agent_deregister, agent_status, agent_list, agent_reputation, agent_feedback_list, agent_give_feedback, agent_revoke_feedback).

Fix: z.string().regex(/^\d+$/, 'agentId must be a non-negative integer string'). Same treatment for feedbackIndex, value (if string-typed), etc.

[arcayne]

Fixed in 4cf1a97. Added shared agentIdString = z.string().regex(/^\d+$/, 'agentId must be a non-negative integer string') at server.ts:867 and applied to all 8 identity tools (agent_update, agent_deregister, agent_status, agent_list is owner-side, agent_reputation, agent_feedback_list, agent_give_feedback, agent_revoke_feedback).

Also tightened value in agent_give_feedback to z.number().int() to fix the same class of bug for the rating field — that was the root cause of BigInt(params.value) throwing RangeError on fractional inputs (CodeRabbit caught it on the same line). Non-integer ratings now fail at the schema layer.

feedbackIndex was already z.number().int().min(0) so no change needed there.

[ckhbtc]

reputation and feedbackList (L158) both swallow _err unconditionally and return zeros / empty arrays. This masks network outages, RPC errors, and contract reverts as "no data" — an LLM consumer gets { score: 0, count: 0 } even when the registry is temporarily unreachable.

When an agent-to-agent trust decision (or a human deciding whether to trust an agent) is based on reputation, the difference between "this agent has no reputation" and "we couldn't reach the chain" is load-bearing.

Suggested fix: narrow the catch to only handle the specific not-found / empty-results case (e.g., check error type or message), and re-throw network/RPC errors so the caller can distinguish. If the SDK doesn't surface a typed "not found" error, consider a try { getStatus } catch { IdentityNotFound } pre-check and let other errors propagate.

[arcayne]

Fixed in 4cf1a97. Removed the broad catch (_err) { return zeros } in both reputation and feedbackList. RPC errors and contract reverts now propagate as-is — an LLM consumer (or human) trying to make a trust decision will see "execution reverted" / "fetch failed" instead of a confidently-wrong score: 0.

The "no feedback" case is still represented cleanly: the SDK's getReputation returns { score: 0, count: 0, clients } when active.length === 0 (read-client.ts:248) without throwing, so genuinely-empty reputation still surfaces as zeros rather than as an error. The catch was only ever swallowing real errors.

Updated the two unit tests that asserted on the swallow behavior to assert re-throw instead.

Did not pre-check getStatus → IdentityNotFound for nonexistent agents — the SDK contract calls return cleanly for that case, no exception path. If that turns out not to hold in practice we can add the pre-check.

[ckhbtc]

URL-typed fields are plain z.string().optional() — image (L877), uri (L881, L918), sourceCode (L886, L925), documentation (L887, L926), and the update-side image (L914) all pass javascript:, data:, file://, or any other URI scheme. These strings flow unmodified into IPFS upload and end up in a persistent on-chain record funded by the user's gas.

The descriptions say "https://, http://, or ipfs://" but that's not enforced. The service-entry schema already uses z.string().url() correctly for endpoint, so the pattern exists in this file.

Fix: z.string().url() on each. If ipfs:// needs to be accepted (URL parser rejects it in some Node versions), add an explicit allowlist regex: z.string().regex(/^(https?|ipfs):\/\//).

[arcayne]

Fixed in 4cf1a97. Added shared metadataUrl = z.string().regex(/^(https?|ipfs):\/\//, ...) at server.ts:874 and applied to every URL-typed field across both agent_register and agent_update:

• image
• uri
• sourceCode
• documentation
• feedbackURI (in agent_give_feedback — same class of issue you flagged, also tightened)

Used the regex form rather than z.string().url() because Node's URL parser rejects ipfs:// on some versions and we genuinely need that scheme. The regex enforces exactly the three schemes that the IPFS upload + on-chain card storage paths can handle: https://, http://, ipfs://. javascript:, data:, file:// and bare strings now fail at the schema layer before any network or chain interaction.

[ckhbtc]

feedbackHash is z.string().optional() but is cast to `0x${string}` in src/identity/index.ts:282 with no validation. An LLM passing arbitrary text gets a silent type cast and then a cryptic SDK-level failure when the contract rejects a non-32-byte value.

Fix: z.string().regex(/^0x[0-9a-fA-F]{64}$/, 'feedbackHash must be a 32-byte hex string').optional().

[arcayne]

Fixed in 4cf1a97. Added feedbackHashHex = z.string().regex(/^0x[0-9a-fA-F]{64}$/, 'feedbackHash must be a 32-byte hex string (0x + 64 hex chars)') at server.ts:881 and applied to feedbackHash in agent_give_feedback.

Now any non-32-byte input (wrong length, missing 0x, non-hex chars) fails at the Zod layer with a clear message rather than getting silently cast to `0x${string}` and surfacing as a cryptic SDK / contract revert. Pairs with the value: z.number().int() tightening that fixed the related BigInt() RangeError issue.

[coderabbitai]

🧹 Nitpick comments (2)

src/identity/index.ts (2)

283-289: Map "agent not found" to IdentityNotFound in the ownership pre-check.

When getStatus(id) throws because the token doesn't exist, the catch wraps it as a generic IdentityTxFailed: ERC721 .... The read adapter (src/identity/read.ts:72-75) already translates this case to the typed IdentityNotFound using the shared isAgentNotFoundError helper — deregister should do the same so callers get a consistent, actionable error instead of a transaction-failed wrapper for what is really a missing-agent case.

♻️ Proposed fix

 import {
   IdentityTxFailed,
+  IdentityNotFound,
   DeregisterNotConfirmed,
   NotAgentOwner,
   DeregisterNotApplied,
 } from '../errors/index.js'

     let status
     try {
       status = await client.getStatus(id)
-    } catch (err) { wrapSdkError(err) }
+    } catch (err) {
+      if (isAgentNotFoundError(err)) throw new IdentityNotFound(params.agentId)
+      wrapSdkError(err)
+    }
     if (status.owner.toLowerCase() !== client.address.toLowerCase()) {
       throw new NotAgentOwner(params.agentId, client.address, status.owner)
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 283 - 289, The ownership pre-check in
deregister uses client.getStatus(id) and currently swallows a missing-token
error by calling wrapSdkError(err); change this to detect the "agent not found"
case using the shared isAgentNotFoundError helper and throw
IdentityNotFound(params.agentId) when true, otherwise call wrapSdkError(err).
Update the catch around client.getStatus(id) in the deregister flow (the block
that assigns status and then checks status.owner) to perform this conditional
error mapping so callers see IdentityNotFound instead of a generic wrapped SDK
error.

---

291-295: Dead DeregisterNotConfirmed passthrough.

DeregisterNotConfirmed is a local precondition error thrown at line 275 — the SDK's client.deregister() cannot raise it, so passing it as a passthrough class to wrapSdkError is unreachable. Either drop it or replace with a class the SDK actually surfaces (e.g., NotAgentOwner if the contract revert can be typed).

♻️ Proposed fix

     let txHash: `0x${string}`
     try {
       const r = await client.deregister(id)
       txHash = r.txHash
-    } catch (err) { wrapSdkError(err, DeregisterNotConfirmed) }
+    } catch (err) { wrapSdkError(err) }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 291 - 295, The catch block after calling
client.deregister(id) uses the local precondition error class
DeregisterNotConfirmed as the passthrough to wrapSdkError even though
client.deregister cannot throw that; update the handler to either remove the
unreachable DeregisterNotConfirmed passthrough or replace it with an SDK-visible
error class (for example NotAgentOwner) that the contract revert can actually
produce, so change the wrapSdkError call in the client.deregister error handler
to use an appropriate SDK error class (or no passthrough) and keep the rest of
the logic in the same catch block referencing client.deregister, wrapSdkError,
and DeregisterNotConfirmed/NotAgentOwner as needed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/identity/index.ts`:
- Around line 283-289: The ownership pre-check in deregister uses
client.getStatus(id) and currently swallows a missing-token error by calling
wrapSdkError(err); change this to detect the "agent not found" case using the
shared isAgentNotFoundError helper and throw IdentityNotFound(params.agentId)
when true, otherwise call wrapSdkError(err). Update the catch around
client.getStatus(id) in the deregister flow (the block that assigns status and
then checks status.owner) to perform this conditional error mapping so callers
see IdentityNotFound instead of a generic wrapped SDK error.
- Around line 291-295: The catch block after calling client.deregister(id) uses
the local precondition error class DeregisterNotConfirmed as the passthrough to
wrapSdkError even though client.deregister cannot throw that; update the handler
to either remove the unreachable DeregisterNotConfirmed passthrough or replace
it with an SDK-visible error class (for example NotAgentOwner) that the contract
revert can actually produce, so change the wrapSdkError call in the
client.deregister error handler to use an appropriate SDK error class (or no
passthrough) and keep the rest of the logic in the same catch block referencing
client.deregister, wrapSdkError, and DeregisterNotConfirmed/NotAgentOwner as
needed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 72c33a27-3b76-42bc-b380-91124adf36f2

📥 Commits

Reviewing files that changed from the base of the PR and between 032030a and 4cf1a97.

📒 Files selected for processing (6)

• src/errors/index.ts
• src/identity/identity.test.ts
• src/identity/index.ts
• src/identity/read.test.ts
• src/identity/read.ts
• src/mcp/server.ts

✅ Files skipped from review due to trivial changes (1)

• src/identity/identity.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/mcp/server.ts
• src/identity/read.ts

[ckhbtc]

Re-review on top of 4cf1a97. Tightening from the previous round (agentId regex, metadataUrl scheme allowlist, feedbackHashHex, read-path error propagation, deregister ownership pre-check + post-burn DeregisterNotApplied) all look good. New blocker introduced by the SDK convergence, plus a few should-land-with-it items.

🔴 Blocking

1. @injective/agent-sdk is file:../injective-agent-cli/packages/sdk — not installable. The package isn't on npm (npm view @injective/agent-sdk → 404), and a fresh npm install against this PR's package.json silently leaves node_modules/@injective/agent-sdk empty — npm prints added 236 packages but the directory doesn't exist. tsc --noEmit then fails with 4 × TS2307: Cannot find module '@injective/agent-sdk' on src/identity/index.ts:9-19 and src/identity/read.ts:7.

   The README's setup is npm install && npm run build — that's currently broken for anyone without a sibling injective-agent-cli/ checkout (i.e. everyone except the author). Whatever the long-term plan is — publish the SDK to npm, vendor it, monorepo it — this needs to land before merge or the PR ships an unbuildable package. The PR's own test-plan claim of "326 passed / 6 skipped" can't be reproduced from a clean clone.

🟡 Should land in the same revision

2. README identity table is missing 4 of 9 tools. server.ts:889-1089 registers agent_register, agent_update, agent_deregister, agent_status, agent_list, agent_reputation, agent_feedback_list, agent_give_feedback, agent_revoke_feedback — but README.md:60-66 only lists the first five. The PR description also says "7 ERC-8004 identity MCP tools" which doesn't match the code. Update both to reflect all 9.

3. PINATA_JWT not documented. identity/index.ts:133-141 makes it required for register (no uri) and any card-affecting update, but the README setup section and the env table at README.md:212-219 don't mention it. The error message at register-time is clear, but users hitting this for the first time should see it in setup docs too — at minimum next to INJECTIVE_NETWORK.

4. Test plan smoke script isn't in the repo. PR description points reviewers at scripts/create-x402-agent but .gitignore now excludes scripts/. Either commit a sanitized version of the script (or move it into a test) or drop that line from the test plan — as written, no reviewer can run it.

🟢 Nits (non-blocking)

• Optimistic tx-hash returns on update / giveFeedback / revokeFeedback. deregister got DeregisterNotApplied (good), but the other writes still return r.txHash straight from the SDK without verifying the receipt didn't revert. register is implicitly safe because r.agentId only populates from a successful Transfer event; the others have no analogous signal. Lower-stakes than burn (writes are idempotent / re-runnable), but the same SDK gap noted in the deregister comment applies.
• walletLinkInfo silently returns {} when wallet === signer but txHashes.length <= 1 (index.ts:177-181) — the caller can't distinguish "link was already in place" from "SDK skipped the link tx for some other reason". A walletLinkSkipped reason here would help LLM consumers.
• list() over-fetches 3x with type filter (read.ts:91) and reports the filtered count as total. If a registry has > limit×3 agents of an unfiltered type, the LLM sees a misleadingly small total. Consider documenting this in the tool description, or paginating until exhausted.
• isAgentNotFoundError substring matching (index.ts:164-167) — comment acknowledges fragility. Worth a follow-up to ask the SDK to surface a typed code (e.g. ContractError.code === 'NONEXISTENT_TOKEN').

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (3)

README.md (1)

59-60: ⚠️ Potential issue | 🟡 Minor

Add the missing blank line before the Identity table.

MD058 is still firing here because the table starts immediately after the ### Identity (ERC-8004) heading.

📝 Proposed fix

 ### Identity (ERC-8004)
+
 | Tool | Description |
 |---|---|

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 59 - 60, Add a single blank line between the "###
Identity (ERC-8004)" heading and the following table (the line starting with "|
Tool | Description |") so the Markdown heading is separated from the table and
MD058 stops firing; update the README.md by inserting one empty line immediately
after the "### Identity (ERC-8004)" heading.

src/mcp/server.ts (1)

64-83: ⚠️ Potential issue | 🟠 Major

Validate action.url before persisting it in agent metadata.

Unlike image, uri, sourceCode, documentation, and feedbackURI, this field still accepts any string. That reopens the same validation gap for persisted URLs here: garbage values and unsafe schemes can pass straight into the on-chain card payload.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp/server.ts` around lines 64 - 83, The actionSchema currently allows
any string for the url field; update actionSchema to validate URLs the same way
other fields (image, uri, sourceCode, documentation, feedbackURI) are validated
— either replace z.string().optional() for url with the shared safe URL schema
(e.g., safeUrlSchema or uri/url validator used elsewhere) or use
z.string().url().optional() with allowed schemes, and keep the .describe(...)
call; ensure this validation is applied before persisting agent metadata so
garbage or unsafe schemes cannot be stored.

src/integration/identity.integration.test.ts (1)

95-96: ⚠️ Potential issue | 🟡 Minor

Filter the list call to this wallet to avoid testnet-size flakiness.

Once testnet accumulates enough agents, identityRead.list(config, { limit: 50 }) can miss the newly created one and make this assertion nondeterministic.

🧪 Proposed fix

-    const result = await identityRead.list(config, { limit: 50 })
+    const result = await identityRead.list(config, { owner: testEvmAddress, limit: 50 })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/integration/identity.integration.test.ts` around lines 95 - 96, The list
call is too broad and can miss the newly created agent on busy testnet; narrow
it to this test's wallet by adding the wallet filter to the identityRead.list
call. Replace identityRead.list(config, { limit: 50 }) with
identityRead.list(config, { limit: 50, wallet: <the test wallet identifier> })
(e.g., wallet.address or createdWallet.address depending on the variable used
earlier in this test) so the query only returns agents owned by this wallet.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/identity/index.ts`:
- Around line 11-12: The try blocks that call createClient() are catching
keystore errors (e.g., WalletNotFound, WrongPassword) and rethrowing them as
IdentityTxFailed; instead use wrapSdkError's passthrough behavior so keystore
errors are preserved. Update the catch handlers around createClient() in this
file (and the other affected ranges) to call wrapSdkError(err, { passthrough:
true }) or rethrow the original keystore errors rather than wrapping them in
IdentityTxFailed; keep IdentityTxFailed for genuine transaction failures after a
chain write, and reference the existing symbols createClient, wrapSdkError,
WalletNotFound, WrongPassword, and IdentityTxFailed when making the change.
- Around line 151-163: walletLinkInfo currently only annotates the response but
the SDK call in register() and update() still forwards params.wallet (or
client.address) causing writes to differ from the returned JSON; before calling
the adapter/client, decide whether to attempt a wallet link and omit the wallet
from the request when it should be skipped (i.e., when wallet is undefined or
wallet.toLowerCase() !== signerAddress.toLowerCase()), update callers of
walletLinkInfo (register, update) to call a pre-check (or extend walletLinkInfo
to return a decision flag like shouldLink) and branch to not include the wallet
in the SDK request when shouldLink is false, and keep walletLinkInfo only for
post-response metadata (including fixing tx hash selection if needed by using
the correct txHashes index). Ensure you reference walletLinkInfo, register, and
update when making the changes.

In `@src/mcp/server.ts`:
- Around line 927-948: The agent_update endpoint description currently says card
rebuilds happen only for description, image, and services, but identity.update()
also treats actions, active, supportedTrust, tags, version, license, sourceCode,
documentation (and other card-affecting fields such as x402) as triggers; update
the descriptive string for the agent_update handler to enumerate all fields that
cause a card rebuild (include description, image, services, actions, active,
supportedTrust, tags, version, license, sourceCode, documentation, and x402) and
note that PINATA_JWT is required when these fields change so callers won’t
inadvertently hit PINATA_JWT failures (look for the agent_update docstring / API
description and the identity.update() references to align wording).

---

Duplicate comments:
In `@README.md`:
- Around line 59-60: Add a single blank line between the "### Identity
(ERC-8004)" heading and the following table (the line starting with "| Tool |
Description |") so the Markdown heading is separated from the table and MD058
stops firing; update the README.md by inserting one empty line immediately after
the "### Identity (ERC-8004)" heading.

In `@src/integration/identity.integration.test.ts`:
- Around line 95-96: The list call is too broad and can miss the newly created
agent on busy testnet; narrow it to this test's wallet by adding the wallet
filter to the identityRead.list call. Replace identityRead.list(config, { limit:
50 }) with identityRead.list(config, { limit: 50, wallet: <the test wallet
identifier> }) (e.g., wallet.address or createdWallet.address depending on the
variable used earlier in this test) so the query only returns agents owned by
this wallet.

In `@src/mcp/server.ts`:
- Around line 64-83: The actionSchema currently allows any string for the url
field; update actionSchema to validate URLs the same way other fields (image,
uri, sourceCode, documentation, feedbackURI) are validated — either replace
z.string().optional() for url with the shared safe URL schema (e.g.,
safeUrlSchema or uri/url validator used elsewhere) or use
z.string().url().optional() with allowed schemes, and keep the .describe(...)
call; ensure this validation is applied before persisting agent metadata so
garbage or unsafe schemes cannot be stored.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 52e00f79-df9c-460c-b837-8d587a5c4ac6

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf1a97 and 77063b2.

⛔ Files ignored due to path filters (2)

• package-lock.json is excluded by !**/package-lock.json
• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (9)

• .gitignore
• README.md
• package.json
• src/errors/errors.test.ts
• src/errors/index.ts
• src/identity/identity.test.ts
• src/identity/index.ts
• src/integration/identity.integration.test.ts
• src/mcp/server.ts

✅ Files skipped from review due to trivial changes (1)

• package.json

🚧 Files skipped from review as they are similar to previous changes (3)

• src/errors/errors.test.ts
• .gitignore
• src/errors/index.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Preserve keystore errors instead of wrapping them as IdentityTxFailed.

createClient() runs inside these try blocks, so WalletNotFound and WrongPassword get relabeled as transaction failures even though no chain write happened. wrapSdkError() already has passthrough support; these callers should use it.

🛠️ Proposed fix

-import { IdentityTxFailed } from '../errors/index.js'
+import { IdentityTxFailed, WalletNotFound, WrongPassword } from '../errors/index.js'
@@
-    } catch (err) { wrapSdkError(err) }
+    } catch (err) { wrapSdkError(err, WalletNotFound, WrongPassword) }
@@
-    } catch (err) { wrapSdkError(err) }
+    } catch (err) { wrapSdkError(err, WalletNotFound, WrongPassword) }
@@
-    } catch (err) { wrapSdkError(err) }
+    } catch (err) { wrapSdkError(err, WalletNotFound, WrongPassword) }
@@
-    } catch (err) { wrapSdkError(err) }
+    } catch (err) { wrapSdkError(err, WalletNotFound, WrongPassword) }

Also applies to: 138-142, 168-201, 204-288

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 11 - 12, The try blocks that call
createClient() are catching keystore errors (e.g., WalletNotFound,
WrongPassword) and rethrowing them as IdentityTxFailed; instead use
wrapSdkError's passthrough behavior so keystore errors are preserved. Update the
catch handlers around createClient() in this file (and the other affected
ranges) to call wrapSdkError(err, { passthrough: true }) or rethrow the original
keystore errors rather than wrapping them in IdentityTxFailed; keep
IdentityTxFailed for genuine transaction failures after a chain write, and
reference the existing symbols createClient, wrapSdkError, WalletNotFound,
WrongPassword, and IdentityTxFailed when making the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Decide wallet linking before the SDK call, not only in the returned JSON.

walletLinkInfo() only post-processes the response. register() always forwards a wallet (params.wallet ?? client.address), so omitting wallet still self-links, and update() still forwards a mismatched wallet even when the result says linking was skipped. That makes the adapter's output inconsistent with the write it actually requested.

🛠️ Proposed fix

 function walletLinkInfo(wallet: string | undefined, signerAddress: string, txHashes: `0x${string}`[]): WalletLinkInfo {
   if (!wallet) return {}
   if (wallet.toLowerCase() !== signerAddress.toLowerCase()) {
     return {
       walletLinkSkipped: true,
       walletLinkReason: `Wallet ${wallet} does not match signer ${signerAddress} — only self-links supported`,
@@
   async register(config: Config, params: RegisterParams): Promise<RegisterResult> {
@@
     try {
       const client = createClient(config, params.address, params.password, storage)
+      const wallet = params.wallet?.toLowerCase() === client.address.toLowerCase()
+        ? params.wallet as `0x${string}`
+        : undefined
       const r = await client.register({
         name: params.name,
         type: params.type as AgentType,
         builderCode: params.builderCode,
-        wallet: (params.wallet ?? client.address) as `0x${string}`,
+        wallet,
         uri: params.uri,
@@
   async update(config: Config, params: UpdateParams): Promise<UpdateResult> {
@@
     try {
       const client = createClient(config, params.address, params.password, storage)
       const id = BigInt(params.agentId)
+      const wallet = params.wallet?.toLowerCase() === client.address.toLowerCase()
+        ? params.wallet as `0x${string}`
+        : undefined
       const r = await client.update(id, {
@@
-        wallet: params.wallet as `0x${string}` | undefined,
+        wallet,

Also applies to: 168-200, 204-253

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 151 - 163, walletLinkInfo currently only
annotates the response but the SDK call in register() and update() still
forwards params.wallet (or client.address) causing writes to differ from the
returned JSON; before calling the adapter/client, decide whether to attempt a
wallet link and omit the wallet from the request when it should be skipped
(i.e., when wallet is undefined or wallet.toLowerCase() !==
signerAddress.toLowerCase()), update callers of walletLinkInfo (register,
update) to call a pre-check (or extend walletLinkInfo to return a decision flag
like shouldLink) and branch to not include the wallet in the SDK request when
shouldLink is false, and keep walletLinkInfo only for post-response metadata
(including fixing tx hash selection if needed by using the correct txHashes
index). Ensure you reference walletLinkInfo, register, and update when making
the changes.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Broaden the agent_update description to match the real card-rebuild triggers.

This prompt says card rebuilds are only for description, image, and services, but identity.update() also treats actions, x402, active, supportedTrust, tags, version, license, sourceCode, and documentation as card-affecting. Right now the tool description will lead callers into avoidable PINATA_JWT failures.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mcp/server.ts` around lines 927 - 948, The agent_update endpoint
description currently says card rebuilds happen only for description, image, and
services, but identity.update() also treats actions, active, supportedTrust,
tags, version, license, sourceCode, documentation (and other card-affecting
fields such as x402) as triggers; update the descriptive string for the
agent_update handler to enumerate all fields that cause a card rebuild (include
description, image, services, actions, active, supportedTrust, tags, version,
license, sourceCode, documentation, and x402) and note that PINATA_JWT is
required when these fields change so callers won’t inadvertently hit PINATA_JWT
failures (look for the agent_update docstring / API description and the
identity.update() references to align wording).

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/identity/index.ts`:
- Around line 148-152: The write paths update, giveFeedback, and revokeFeedback
are not using isAgentNotFoundError(err), so a nonexistent agent surfaces as
IDENTITY_TX_FAILED; modify each of the three functions' catch blocks to detect
agent-not-found by calling isAgentNotFoundError(err) and rethrow or return the
specific IDENTITY_NOT_FOUND error/result instead of mapping every failure to
IDENTITY_TX_FAILED (match the same catch/throw pattern used elsewhere in this
file that checks isAgentNotFoundError and returns IDENTITY_NOT_FOUND). Ensure
you reference the same error constant (IDENTITY_NOT_FOUND) and the helper
isAgentNotFoundError when updating the catch logic in functions update,
giveFeedback, and revokeFeedback.
- Around line 84-89: The GiveFeedbackParams.value and
RevokeFeedbackParams.feedbackIndex use unsafe JS number types and are later
converted to BigInt (in the methods that perform the BigInt conversions),
risking precision loss for integers > 2^53-1; update the parameter types to
string (or bigint) in the GiveFeedbackParams and RevokeFeedbackParams
interfaces, and adjust the call sites and conversion logic to parse BigInt from
the string, or alternatively add a runtime guard using Number.isSafeInteger()
before converting the incoming number to BigInt and throw a clear error if the
check fails so values cannot silently corrupt on-chain data.

In `@src/identity/read.test.ts`:
- Around line 108-134: Add a test that explicitly exercises the ContractError
path used by isAgentNotFoundError: have mockGetEnrichedAgent reject with an
instance of ContractError whose revertReason === 'ERC721NonexistentToken' and
then assert that identityRead.status(config, { agentId: '###' }) rejects with
IdentityNotFound (and optional message check). This ensures the code path in
isAgentNotFoundError (the instanceof ContractError + revertReason check) is
covered alongside the existing string-substring error tests.
- Around line 361-376: Add a new test in src/identity/read.test.ts that mocks
mockGetFeedbackEntries returning an entry whose value is larger than
Number.MAX_SAFE_INTEGER (e.g., 9007199254740992n or 10n ** 18n + 1n) with an
appropriate decimals field, call identityRead.feedbackList(config, { agentId:
AGENT_ID }) and compute the exact expected normalized decimal string using
BigInt arithmetic (divide and format using BigInt to produce the exact decimal
representation); then assert that result.entries[0]!.value matches that exact
representation (or fail if it loses precision) so this boundary case will catch
silent precision loss in the normalization logic (see conversion logic in
feedbackList / Number(e.value) / 10 ** e.decimals).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 911ddd69-80fa-4b32-88e0-fa4555882913

📥 Commits

Reviewing files that changed from the base of the PR and between 77063b2 and b13f153.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (5)

• package.json
• src/identity/identity.test.ts
• src/identity/index.ts
• src/identity/read.test.ts
• src/mcp/server.ts

✅ Files skipped from review due to trivial changes (2)

• package.json
• src/mcp/server.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/identity/identity.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

What is JavaScript's Number.MAX_SAFE_INTEGER, and why can converting a Number larger than that to BigInt still produce the wrong integer?

💡 Result:

JavaScript's Number.MAX_SAFE_INTEGER is 9007199254740991 (2^53 - 1), the largest integer that can be exactly represented and safely used for arithmetic in the Number type, which uses IEEE 754 double-precision floating-point format with a 53-bit mantissa. Converting a Number larger than this to BigInt produces the wrong integer because Numbers larger than Number.MAX_SAFE_INTEGER cannot represent all integers exactly due to limited precision. The floating-point representation rounds or loses lower-order bits, so BigInt(n) where n > Number.MAX_SAFE_INTEGER captures only the imprecise approximation stored in the Number, not the original exact integer value. For example: let n = 90071992547409946; // Intended value, but stored imprecisely BigInt(n); // Returns 90071992547409952n or similar rounded value To preserve exact values, use BigInt from strings or literals with 'n' suffix instead of converting from imprecise Numbers.

Citations:

• 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
• 2: https://stackoverflow.com/questions/60982404/convert-number-to-bigint-or-string-when-its-value-exceeds-number-max-safe-integ
• 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/bigint

---

🏁 Script executed:

cat -n src/identity/index.ts | head -100

Repository: InjectiveLabs/mcp-server

Length of output: 3121

---

🏁 Script executed:

cat -n src/identity/index.ts | sed -n '100,115p'

Repository: InjectiveLabs/mcp-server

Length of output: 486

---

🏁 Script executed:

cat -n src/identity/index.ts | sed -n '265,295p'

Repository: InjectiveLabs/mcp-server

Length of output: 1364

---

🏁 Script executed:

cat -n src/identity/index.ts | sed -n '260,285p'

Repository: InjectiveLabs/mcp-server

Length of output: 1043

---

🏁 Script executed:

rg 'BigInt\(' src/identity/index.ts -B 2 -A 2

Repository: InjectiveLabs/mcp-server

Length of output: 771

---

Unsafe number type for on-chain integers causes silent data corruption.

value (GiveFeedbackParams, line 88) and feedbackIndex (RevokeFeedbackParams, line 107) are typed as number but converted to BigInt in the respective methods (lines 269 and 291). Any integer larger than 2^53 - 1 (9007199254740991) loses precision in JavaScript's floating-point representation before the BigInt conversion, causing wrong reputation values or feedback entries to be submitted on-chain.

Change parameter types to string or add Number.isSafeInteger() validation before conversion:

🛠️ Minimal guard if you keep the current API shape

+function assertSafeInteger(value: number, field: string): void {
+  if (!Number.isSafeInteger(value) || value < 0) {
+    throw new IdentityTxFailed(`${field} must be a non-negative safe integer`)
+  }
+}
@@
   async giveFeedback(config: Config, params: GiveFeedbackParams): Promise<GiveFeedbackResult> {
     try {
       const client = createClient(config, params.address, params.password)
+      assertSafeInteger(params.value, 'value')
       const r = await client.giveFeedback({
@@
   async revokeFeedback(config: Config, params: RevokeFeedbackParams): Promise<RevokeFeedbackResult> {
     try {
       const client = createClient(config, params.address, params.password)
+      assertSafeInteger(params.feedbackIndex, 'feedbackIndex')
       const r = await client.revokeFeedback({

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export interface GiveFeedbackParams {
	address: string
	password: string
	agentId: string
	value: number
	valueDecimals?: number
	export interface GiveFeedbackParams {
	address: string
	password: string
	agentId: string
	value: number
	valueDecimals?: number

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 84 - 89, The GiveFeedbackParams.value and
RevokeFeedbackParams.feedbackIndex use unsafe JS number types and are later
converted to BigInt (in the methods that perform the BigInt conversions),
risking precision loss for integers > 2^53-1; update the parameter types to
string (or bigint) in the GiveFeedbackParams and RevokeFeedbackParams
interfaces, and adjust the call sites and conversion logic to parse BigInt from
the string, or alternatively add a runtime guard using Number.isSafeInteger()
before converting the incoming number to BigInt and throw a clear error if the
check fails so values cannot silently corrupt on-chain data.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Preserve IDENTITY_NOT_FOUND on the write paths.

isAgentNotFoundError() is defined, but update, giveFeedback, and revokeFeedback never use it. A nonexistent agent will therefore surface as IDENTITY_TX_FAILED, which makes a bad agentId indistinguishable from a retryable transaction failure.

🛠️ Proposed fix

-import { IdentityTxFailed } from '../errors/index.js'
+import { IdentityNotFound, IdentityTxFailed } from '../errors/index.js'
@@
-    } catch (err) { wrapSdkError(err) }
+    } catch (err) {
+      if (isAgentNotFoundError(err)) throw new IdentityNotFound(params.agentId)
+      wrapSdkError(err)
+    }

Apply the same catch pattern to update, giveFeedback, and revokeFeedback.

Also applies to: 223-261, 264-295

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/index.ts` around lines 148 - 152, The write paths update,
giveFeedback, and revokeFeedback are not using isAgentNotFoundError(err), so a
nonexistent agent surfaces as IDENTITY_TX_FAILED; modify each of the three
functions' catch blocks to detect agent-not-found by calling
isAgentNotFoundError(err) and rethrow or return the specific IDENTITY_NOT_FOUND
error/result instead of mapping every failure to IDENTITY_TX_FAILED (match the
same catch/throw pattern used elsewhere in this file that checks
isAgentNotFoundError and returns IDENTITY_NOT_FOUND). Ensure you reference the
same error constant (IDENTITY_NOT_FOUND) and the helper isAgentNotFoundError
when updating the catch logic in functions update, giveFeedback, and
revokeFeedback.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm conversion strategy and current test coverage for precision boundaries.
rg -n "feedbackList|Number\\(|10 \\*\\*|decimals" src/identity/read.ts
rg -n "MAX_SAFE_INTEGER|9007199254740991|toBeCloseTo|bigint" src/identity/read.test.ts

Repository: InjectiveLabs/mcp-server

Length of output: 425

---

🏁 Script executed:

# Search for other tests involving feedbackList or value normalization
rg -B2 -A5 "feedbackList" src/identity/read.test.ts | head -100

# Check for any comments or safeguards around large values
rg -B3 -A3 "feedbackIndex|feedbackList.*value" src/identity/read.ts

# Look for any tests with larger bigint values
rg -n "value.*[0-9]{6,}n|MAX_SAFE" src/identity/read.test.ts

Repository: InjectiveLabs/mcp-server

Length of output: 1841

---

🏁 Script executed:

# Check the full feedbackList implementation for any comments about precision
rg -B5 -A10 "async feedbackList" src/identity/read.ts

# Look for any comments in the file about bigint, precision, or MAX_SAFE_INTEGER
rg -i "precision|safe|overflow|bigint.*number|number.*conversion" src/identity/read.ts

Repository: InjectiveLabs/mcp-server

Length of output: 700

---

🏁 Script executed:

# Get complete entries mapping to see the full context
sed -n '131,150p' src/identity/read.ts

Repository: InjectiveLabs/mcp-server

Length of output: 779

---

Add a test for bigint normalization precision boundaries.

The current test at lines 361–376 validates decimal normalization with a small value (12345n), but doesn't test behavior when value exceeds Number.MAX_SAFE_INTEGER (9007199254740991). The conversion at line 144 of src/identity/read.ts (Number(e.value) / 10 ** e.decimals) will silently lose precision with large bigints. A boundary test would prevent regression.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/identity/read.test.ts` around lines 361 - 376, Add a new test in
src/identity/read.test.ts that mocks mockGetFeedbackEntries returning an entry
whose value is larger than Number.MAX_SAFE_INTEGER (e.g., 9007199254740992n or
10n ** 18n + 1n) with an appropriate decimals field, call
identityRead.feedbackList(config, { agentId: AGENT_ID }) and compute the exact
expected normalized decimal string using BigInt arithmetic (divide and format
using BigInt to produce the exact decimal representation); then assert that
result.entries[0]!.value matches that exact representation (or fail if it loses
precision) so this boundary case will catch silent precision loss in the
normalization logic (see conversion logic in feedbackList / Number(e.value) / 10
** e.decimals).

[arcayne]

@ckhbtc — pushed b13f153 as a follow-up addressing the looser nits from my own re-read + the simplify pass. SDK side bumped to 0.2.1 (381a2708), additive/non-breaking; mcp-server repinned.

SDK 0.2.1 (381a2708):

• assertReceiptSuccess() helper — closes the optimistic-txHash gap on every write path. See follow-up on the receipt-confirmation thread.
• RegisterResult.walletTxHash / setUriTxHash and UpdateResult.walletTxHash named fields — consumers no longer have to identify the wallet-link tx by index in txHashes[] (which was unstable: position depended on whether the two-phase URI re-upload ran).
• formatContractError decodes ERC721NonexistentToken and ERC721IncorrectOwner into ContractError.revertReason, so consumers can stop substring-matching error messages.
• update() finds the wallet planned-write by its field discriminator instead of the ABI function-name string, so a future setAgentWallet rename won't silently break the mapping.
• Register followup loop: setUri revert → onWarning (matches existing two-phase URI semantics — registration already succeeded), wallet-link revert → throw.

mcp-server (b13f153):

• `walletLinkInfo()` now reads `r.walletTxHash` directly. The previously-silent `{}` branch (wallet === signer but SDK emitted no link tx) now returns `walletLinkSkipped` with a clear reason — no more "did the link happen?" ambiguity.
• `isAgentNotFoundError()` checks `ContractError.revertReason === 'ERC721NonexistentToken'` first; substring fallback retained for safety.
• `agent_list` tool description documents the over-fetch caveat: when `type` is set, the SDK has no on-chain index by type, so it over-fetches `limit*3` and post-filters. Both `agents` and `total` undercount when matching agents exist beyond the window. Suggests filtering by `owner` first.

Tests: 322 passed / 6 skipped (was 321; +1 new test for the SDK-emitted-no-walletTxHash case).

Ready for re-review.

[ckhbtc]

Three blocking items below — mostly concrete bugs/docs errors. Architecture, Zod validation, and error wrapping all look solid; this is close.

Blocking

1. npm install fails cold — @injective/agent-sdk uses a pnpm-only specifier

package.json line 23:

"github:InjectiveLabs/injective-agent-sdk#<sha>&path:/packages/sdk"

npm's github: shorthand does not support &path: — only #ref. pnpm resolves this via codeload tarballs (visible in pnpm-lock.yaml), so pnpm works, but any contributor who reflexively runs npm install (the repo's prior workflow) gets a broken install. The packageManager field only emits a warning, it doesn't hard-block npm.

Fix options:

• Publish @injective/agent-sdk to npm with a real version, or
• Add a preinstall script that exits with a clear message if npm_config_user_agent doesn't start with pnpm, and make the README's "pnpm only" note prominent (currently buried).

2. agent_register silently triggers a wallet-link tx when wallet is omitted — contradicts its own tool description

src/identity/index.ts line 181:

wallet: (params.wallet ?? client.address) as \`0x\${string}\`,

The agent_register tool description says "Omit to skip" wallet linking, but the code substitutes client.address and the SDK then submits a self-link transaction. agent_update does not apply this default — it passes params.wallet through as undefined — so the two write tools behave inconsistently for the same input.

This is an LLM foot-gun: a model following the documented "omit to skip" semantics will unknowingly send a second on-chain tx every time it registers without an explicit wallet.

Fix: pass params.wallet through as \0x${string}` | undefined(matchingagent_update`), or change the description and schema to make the auto-link behavior explicit.

3. README documents the wrong default network

README line 78:

INJECTIVE_NETWORK ... mainnet (default) or testnet.

But src/mcp/server.ts line 92:

const NETWORK = validateNetwork(process.env['INJECTIVE_NETWORK'] ?? 'testnet')

Default is testnet. A user who trusts the README, skips the env var, and runs agent_register will register on testnet then look for their agent on mainnet and find nothing. Either change the README to testnet (default) or change the code default — but they need to agree.

Non-blocking

• type is free-form z.string().min(1) in both agent_register and agent_update. The ERC-8004 registry presumably has a fixed set of valid types — should be z.enum([...]), like serviceEntrySchema already is. Otherwise an LLM can register type: \"my-custom-type\" and either get a confusing on-chain rejection or pollute the registry with garbage types.
• agent_list total is misleading when a type filter is active. It reflects matches inside the over-fetch window, not the registry total. The tool description documents the over-fetch caveat but the field name doesn't. Rename to matchedInWindow or add mayBeIncomplete: true when a type filter narrows the window — otherwise a caller seeing total: 3 will conclude only 3 such agents exist.

What looks good

• wrapSdkError / isAgentNotFoundError split is clean: IdentityNotFound for ERC-721 nonexistent token, everything else wrapped as IdentityTxFailed. No silent catches in read.ts — RPC errors propagate.
• Zod schemas are otherwise tight: agentIdString rejects non-integer strings before BigInt(), metadataUrl blocks javascript: / data:, feedbackHashHex enforces 32-byte length, agentOwnerFilter handles both inj1... and 0x....
• Pinata flow fails cleanly (IdentityTxFailed with a clear message) before any on-chain tx if neither PINATA_JWT nor a pre-uploaded uri is provided — no half-success state.
• pnpm lockfile pins the SDK to a specific commit, so reproducible for pnpm users.