Added ERC-8004 agent identity support

PR_REVIEW.md

@@ -0,0 +1,139 @@
+# PR #70 Review — ERC-8004 Agent Identity
+
+Reviewed against current code on branch `feature/erc-8004-agent-identity`.
+
+---
+
+## CRITICAL — Must Fix
+
+### 1. `getAgentCard()`: Buffer.from breaks in browsers + no timeouts + accepts `http://`
+**File:** `src/abstraction/Identities.ts:1428-1447`
+**Status:** NOT FIXED
+
+Three issues in one method:
+- **`Buffer.from(base64, "base64")`** (line 1434) — crashes in browser environments. This SDK exports from `src/abstraction/` which has no Buffer polyfill.
+- **No timeout on `axios.get`** — IPFS gateway (line 1439) and HTTP (line 1445) calls can hang indefinitely.
+- **Accepts plain `http://`** (line 1443: `startsWith("http")`) — SSRF risk in server contexts; should restrict to `https://` only.
+
+**Fix:** Add a `decodeBase64ToUtf8` helper with Buffer/atob fallback, add `{ timeout: 10_000 }` to all axios calls, change `"http"` check to `"https://"`.
+
+### 2. Ownership message lacks replay protection / domain separation
+**File:** `src/abstraction/Identities.ts:1212-1219`
+**Status:** NOT FIXED
+
+`generateAgentOwnershipMessage` only includes a timestamp. No nonce, no chainId, no registry address in the signed payload. Signatures are more reusable than they should be.
+
+**Fix:** Add a cryptographic nonce (e.g. `ethers.hexlify(ethers.randomBytes(16))`), embed chainId + registry address in the message, return `{ message, timestamp, nonce }`. Update `DemosOwnershipProof` type and `createAgentOwnershipProof` accordingly.
+
+> **Note:** If the node enforces strict TTL + one-time nonce server-side, this is lower priority. Confirm with backend team whether the node handles replay prevention. If yes, demote to MEDIUM.
+
+---
+
+## HIGH — Should Fix
+
+### 3. `getAgentCard` return type is `any | null`
+**File:** `src/abstraction/Identities.ts:1410`
+**Status:** NOT FIXED
+
+`any | null` collapses to `any`. `ERC8004AgentCard` is already defined in `src/types/abstraction/index.ts:417-424` and is the correct return type. Also not imported in Identities.ts.
+
+**Fix:** Import `ERC8004AgentCard`, change return to `Promise<ERC8004AgentCard | null>`.
+
+### 4. JSDoc example for `addAgentIdentity` has wrong signature
+**File:** `src/abstraction/Identities.ts:1287`
+**Status:** NOT FIXED
+
+Example shows:
+```ts
+const proof = await identities.createAgentOwnershipProof(demos, evmAddress)
+```
+But actual signature (line 1230) is `createAgentOwnershipProof(demos, agentId, evmAddress)` — `agentId` is missing.
+
+**Fix:** Update example to `createAgentOwnershipProof(demos, "123", "0x...")`.
+
+### 5. `as any` cast in GCR generation for agent_identity_assign
+**File:** `src/websdk/GCRGeneration.ts:386`
+**Status:** NOT FIXED
+
+The `as any` is unnecessary — `AgentGCRData` (alias for `AgentIdentityPayload`) already exists in `GCREdit.ts:106` and is included in the `GCREditIdentity.data` union at line 123.
+
+**Fix:** Remove `as any` — the spread + timestamp should be assignable directly, or use `as AgentGCRData` if needed.
+
+### 6. Missing `// REVIEW:` markers on new feature sections
+**Files:** `src/abstraction/Identities.ts:1183`, `src/types/abstraction/index.ts:385`
+**Status:** NOT FIXED
+
+Per project coding guidelines (`.github/copilot-instructions.md`), new features need `// REVIEW:` markers.
+
+**Fix:** Add `// REVIEW: ERC-8004 Agent Identity — new feature` above both section headers.
+
+---
+
+## MEDIUM — Nice to Have
+
+### 7. `verifyAgentOwnership` silently swallows all errors
+**File:** `src/abstraction/Identities.ts:1398-1401`
+
+Returns `false` for any error (network, rate limit, wrong RPC, etc.) — indistinguishable from "not owner". Consider at minimum a `console.error` or returning `{ owned: boolean; error?: string }`.
+
+**Decision needed:** Changing return type is breaking. If keeping `boolean`, at least add `console.error` for debugging. If this is a client-side convenience helper (node also verifies), lower priority.
+
+### 8. `DemosOwnershipProof.signature` union has unused `string` variant
+**File:** `src/types/abstraction/index.ts:406`
+
+Type is `string | { type: string; data: string }` but `createAgentOwnershipProof` (line 1250) always produces the object form. The `string` variant is never emitted.
+
+**Fix:** Narrow to `{ type: string; data: string }` only, OR document when string variant is valid.
+
+### 9. `AgentIdentityPayloadType` naming inconsistency
+**File:** `src/types/abstraction/index.ts:465-467`
+
+Every other identity union follows the pattern `XmIdentityPayload`, `Web2IdentityPayload`, etc. This one uses `AgentIdentityPayloadType` with a unique `Type` suffix, clashing with `AgentIdentityPayload` (the inner data interface).
+
+**Ideal fix:** Rename inner `AgentIdentityPayload` → `AgentIdentityData`, then rename `AgentIdentityPayloadType` → `AgentIdentityPayload`. This is a larger refactor touching imports across files.
+
+**Pragmatic fix:** Leave as-is if not worth the churn, but note the inconsistency.
+
+---
+
+## LOW / DEFERRED — Won't fix now
+
+### 10. Hardcoded registry address and RPC
+**File:** `src/abstraction/Identities.ts:1188-1199`
+
+Currently scoped to Base Sepolia testnet. Should eventually be configurable for mainnet deployment. The codebase already uses `process.env.NODE_ENV` elsewhere. Not blocking for testnet phase.
+
+### 11. `inferIdentity` / `removeIdentity` use `payload: any`
+**File:** `src/abstraction/Identities.ts:84, 142`
+
+Expanding `context` union to include `"agent"` further weakens type safety with `any` payloads. A context→payload discriminated union map would be ideal but is a cross-cutting refactor affecting all identity types, not just agent.
+
+### 12. Additional validation in `addAgentIdentity`
+Could also verify `payload.proof.agentId === payload.agentId`, check `demosPublicKey` format, validate `agentId` is numeric/BigInt-safe. Current validation is sufficient for MVP; node validates server-side.
+
+---
+
+## FALSE POSITIVES — Already Fixed or Not Applicable
+
+| Review Comment | Status | Reason |
+|---|---|---|
+| Timestamp inconsistency between `generateAgentOwnershipMessage` and `createAgentOwnershipProof` | **FIXED** in commit 59ae089 | `createAgentOwnershipProof` now calls `generateAgentOwnershipMessage` and shares timestamp (line 1236) |
+| Refactor to remove duplicated message code | **FIXED** in commit 59ae089 | Same fix as above — single source of truth now |
+| `AgentGCRData` type missing from `GCREdit.ts` | **NOT AN ISSUE** | `AgentGCRData` already exists at `GCREdit.ts:106` and is in the data union at line 123 |
+| `atob()` → `Buffer.from()` suggestion (qodo) | **ALREADY APPLIED** | Code already uses `Buffer.from` (line 1434). The real issue is Buffer itself not being browser-safe |
+| No audit logging | **NOT APPLICABLE** | SDK is a client library; audit logging belongs in the node/server, not the SDK |
+| No structured logging | **NOT APPLICABLE** | Same — SDK consumers handle their own logging |
+| Ticket compliance (no ticket provided) | **NOISE** | Automated check, not a code issue |
+| Codebase duplication compliance | **NOISE** | Not configured, automated check |
+
+---
+
+## Summary
+
+| Priority | Count | Items |
+|---|---|---|
+| CRITICAL | 2 | getAgentCard safety (#1), replay protection (#2) |
+| HIGH | 4 | Return type (#3), JSDoc (#4), as any (#5), REVIEW markers (#6) |
+| MEDIUM | 3 | Error swallowing (#7), signature type (#8), naming (#9) |
+| LOW/DEFERRED | 3 | Config (#10), payload typing (#11), extra validation (#12) |
+| FALSE POSITIVE | 8 | Already fixed or not applicable |