Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ERC‑8004 agent identity support: new types and constants, ownership‑proof generation, on‑chain registry interactions (verify owner, fetch tokenURI), lifecycle methods (add/remove/get/verify/getCard), integrates "agent" into identity inference/removal and GCR generation, and resolves tokenURI via base64/IPFS/HTTP. Changes
Sequence DiagramsequenceDiagram
participant Client
participant DemosSDK as Demos (Identities)
participant Signer as Crypto/Signer
participant Registry as ERC8004_Registry
participant Chain as EVM_RPC
Client->>DemosSDK: generateAgentOwnershipMessage(pubKey, agentId, evmAddr)
DemosSDK->>Signer: format/prepare message
Signer-->>DemosSDK: message,timestamp,nonce
DemosSDK-->>Client: {message,timestamp,nonce}
Client->>DemosSDK: createAgentOwnershipProof(agentId, evmAddr)
DemosSDK->>Signer: sign(message)
Signer-->>DemosSDK: signature
DemosSDK-->>Client: DemosOwnershipProof
Client->>DemosSDK: addAgentIdentity(payload, referralCode?)
DemosSDK->>Registry: submit payload/proof (via RPC)
Registry->>Chain: persist/verify ownership
Chain-->>Registry: tx receipt
Registry-->>DemosSDK: RPCResponse
DemosSDK-->>Client: RPCResponseWithValidityData
Client->>DemosSDK: verifyAgentOwnership(agentId, expectedOwner)
DemosSDK->>Registry: ownerOf(agentId)
Registry-->>DemosSDK: ownerAddress
DemosSDK-->>Client: boolean
Client->>DemosSDK: getAgentCard(agentId)
DemosSDK->>Registry: tokenURI(agentId)
Registry-->>DemosSDK: tokenURI
DemosSDK->>Chain: resolve tokenURI (base64/ipfs/http)
Chain-->>DemosSDK: metadata JSON
DemosSDK-->>Client: ERC8004AgentCard | null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/abstraction/Identities.ts:
- Around line 1146-1152: generateAgentOwnershipMessage currently creates its own
timestamp which diverges from the timestamp generated in
createAgentOwnershipProof, causing mismatch during verification; update
generateAgentOwnershipMessage to accept a timestamp argument (or return the
timestamp alongside the string) and have createAgentOwnershipProof call
generateAgentOwnershipMessage with the same timestamp it uses for the proof so
both the message string and proof.timestamp are identical, or if
generateAgentOwnershipMessage is unused, remove it and build the message only
inside createAgentOwnershipProof to guarantee a single source of truth.
🧹 Nitpick comments (4)
src/websdk/GCRGeneration.ts (1)
366-372: Avoidas anytype assertion foredit.data.The
as anyon line 370 bypasses TypeScript's type checking. SinceAgentGCRDatais now a valid type in theGCREditIdentity.dataunion (persrc/types/blockchain/GCREdit.ts), you can remove this assertion or use a proper type.♻️ Suggested improvement
case "agent_identity_assign": { edit.data = { ...identityPayload.payload, timestamp: tx.content.timestamp, - } as any + } break }src/abstraction/Identities.ts (3)
1304-1328: Consider distinguishing between "not owner" and "network/contract error" cases.The current implementation returns
falsefor any error, making it impossible for callers to distinguish between "user doesn't own the agent" vs "network error / contract issue". This could lead to confusing user experiences.♻️ Suggested improvement
async verifyAgentOwnership( agentId: string, expectedOwner: string, - ): Promise<boolean> { + ): Promise<{ owned: boolean; error?: string }> { const registryAbi = [ "function ownerOf(uint256 tokenId) external view returns (address)", ] try { const provider = new ethers.JsonRpcProvider( Identities.AGENT_CHAIN_CONFIG.rpc, ) const contract = new ethers.Contract( Identities.AGENT_REGISTRY_ADDRESS, registryAbi, provider, ) const owner = await contract.ownerOf(agentId) - return owner.toLowerCase() === expectedOwner.toLowerCase() - } catch (error: any) { - // Token doesn't exist or other error - return false + return { owned: owner.toLowerCase() === expectedOwner.toLowerCase() } + } catch (error) { + // Check if token doesn't exist vs network error + const errorMessage = error instanceof Error ? error.message : String(error) + if (errorMessage.includes("nonexistent token") || errorMessage.includes("ERC721")) { + return { owned: false, error: "Token does not exist" } + } + return { owned: false, error: `Verification failed: ${errorMessage}` } } }
1362-1368: Consider IPFS gateway reliability and adding request timeout.The public
ipfs.iogateway may be unreliable or rate-limited. Consider making the gateway configurable or adding a timeout to prevent hanging requests.♻️ Suggested improvement
} else if (tokenUri.startsWith("ipfs://")) { // IPFS URI - fetch via gateway const ipfsHash = tokenUri.replace("ipfs://", "") const response = await axios.get( `https://ipfs.io/ipfs/${ipfsHash}`, + { timeout: 10000 } ) return response.data } else if (tokenUri.startsWith("http")) { // HTTP(S) URI - const response = await axios.get(tokenUri) + const response = await axios.get(tokenUri, { timeout: 10000 }) return response.data }
1120-1134: Consider environment-based configuration for registry address and RPC endpoint.The hardcoded registry address and RPC endpoint are currently scoped to Base Sepolia testnet. The codebase already uses
process.env.NODE_ENVfor environment-based switching in other parts (e.g., nativeBridge.ts). Adopting a similar pattern here—or using a configuration object passed at initialization—would simplify future deployments to Base mainnet or other chains once registry contracts are deployed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/abstraction/Identities.tssrc/abstraction/index.tssrc/types/abstraction/index.tssrc/types/blockchain/GCREdit.tssrc/websdk/GCRGeneration.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Ensure full TypeScript type coverage
Run type checks after changes
Use@/path aliases instead of relative imports
Use JSDoc format for all new methods and functions
Add// REVIEW:comments before newly added features
Provide clear, actionable error messages
Use descriptive names that clearly express intent
Files:
src/abstraction/index.tssrc/websdk/GCRGeneration.tssrc/types/blockchain/GCREdit.tssrc/abstraction/Identities.tssrc/types/abstraction/index.ts
🧬 Code graph analysis (3)
src/types/blockchain/GCREdit.ts (2)
src/abstraction/index.ts (1)
AgentIdentityPayload(65-65)src/types/abstraction/index.ts (1)
AgentIdentityPayload(315-330)
src/abstraction/Identities.ts (4)
src/websdk/demosclass.ts (1)
Demos(48-977)src/types/abstraction/index.ts (2)
DemosOwnershipProof(285-292)AgentIdentityPayload(315-330)src/encryption/index.ts (1)
uint8ArrayToHex(14-14)src/types/index.ts (1)
RPCResponseWithValidityData(152-152)
src/types/abstraction/index.ts (1)
src/abstraction/index.ts (8)
ERC8004Endpoint(71-71)DemosOwnershipProof(69-69)ERC8004AgentCard(70-70)AgentIdentityPayload(65-65)AgentIdentityAssignPayload(66-66)AgentIdentityRemovePayload(67-67)AgentIdentityPayloadType(68-68)IdentityPayload(53-53)
🔇 Additional comments (9)
src/websdk/GCRGeneration.ts (1)
374-382: LGTM!The
agent_identity_removecase correctly extends the existing removal pattern and follows the established passthrough behavior for identity removal payloads.src/abstraction/index.ts (1)
31-39: LGTM!The new agent identity types are properly imported and re-exported, following the established pattern for other identity types. The public API surface expansion is consistent with the ERC-8004 agent identity support.
Also applies to: 64-72
src/types/blockchain/GCREdit.ts (1)
84-85: LGTM!The type additions for agent identity support are well-structured:
AgentGCRDataalias provides a clear naming convention consistent with other GCR data types- The
contextunion extension anddataunion properly cover both add and remove operations- Types align correctly with the
AgentIdentityPayloaddefinitionAlso applies to: 90-90, 100-101
src/types/abstraction/index.ts (3)
285-292: Verify thesignaturefield flexibility is intentional.The
signaturefield inDemosOwnershipProofaccepts bothstringand{ type: string; data: string }. Looking atcreateAgentOwnershipProofinIdentities.ts(line 1177-1180), it creates the object form. Ensure consumers handle both formats consistently.
315-330: LGTM! Well-documented agent identity payload.The
AgentIdentityPayloadinterface has comprehensive JSDoc documentation for each field and captures all necessary data for ERC-8004 agent identity binding. The structure aligns well with the ownership proof workflow.
370-370: LGTM!The
UserPointsextensions for agent tracking follow the established pattern used for UD domains, with proper optional fields for backward compatibility and appropriate data structures for the agent metadata.Also applies to: 379-386
src/abstraction/Identities.ts (3)
1226-1264: LGTM! Good client-side validation.The validation logic properly checks for required fields, validates EVM address format, and ensures the proof's EVM address matches the payload. The case-insensitive comparison for address matching is correct for EVM addresses.
77-80: LGTM!The context union extension to include
"agent"in bothinferIdentityandremoveIdentitymethods follows the established pattern for other identity types and enables the agent identity workflow.Also applies to: 135-138
1187-1225: LGTM! Excellent documentation.The new agent identity methods have comprehensive JSDoc documentation with clear descriptions, parameter documentation, and practical usage examples. This follows the coding guidelines for JSDoc format on new methods.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/abstraction/Identities.ts:
- Around line 1118-1135: Add a `// REVIEW:` marker comment immediately before
the "SECTION: ERC-8004 Agent Identities" block so the new ERC-8004 feature is
flagged per guidelines; locate the block containing the static symbols
AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG (and the "SECTION: ERC-8004 Agent
Identities" comment) and insert the `// REVIEW:` line directly above it.
- Around line 1345-1387: getAgentCard currently uses Buffer.from (breaks in
browsers), performs axios.get without timeouts/size limits, and accepts
unvalidated http:// URIs (SSRF risk). Fix by: (1) replace Buffer.from(...) with
a runtime-safe base64 decode fallback used in getAgentCard (e.g., use Buffer
when available via typeof Buffer !== 'undefined' else use atob + Uint8Array/
TextDecoder to produce a UTF-8 string) for the data: branch; (2) add axios
options { timeout: 10000, responseType: 'json', maxContentLength: 1024 * 100,
maxBodyLength: 1024 * 100 } to all axios.get calls in getAgentCard; and (3)
reject or normalize URIs so only data:application/json;base64:, ipfs://, and
https:// schemes are allowed (do not accept plain http://); return null or throw
a clear error when an unsupported scheme is encountered.
🧹 Nitpick comments (5)
src/abstraction/Identities.ts (5)
23-25: Tighten agent-related types instead of widening withany.You import
AgentIdentityPayload/DemosOwnershipProof, butgetAgentCard()returnsany | null. IfERC8004AgentCardexists (per PR summary), import and return it to keep TS coverage strong. As per coding guidelines, ensure full TypeScript type coverage.Proposed diff
import { @@ UDIdentityPayload, AgentIdentityPayload, DemosOwnershipProof, + ERC8004AgentCard, } from "@/types/abstraction" @@ - async getAgentCard(agentId: string): Promise<any | null> { + async getAgentCard(agentId: string): Promise<ERC8004AgentCard | null> {
77-101:inferIdentity()payload typing is getting riskier as contexts expand.Now that
"agent"is supported,payload: any+method: (context + "_identity_assign") as anyincreases the chance of shipping an invalid payload/method pairing without compile-time feedback. Consider a context→payload mapping type (or overloads) so each context can only accept its correct payload type. As per coding guidelines, ensure full TypeScript type coverage.
135-165: Same typing concern inremoveIdentity()asinferIdentity()(now includes"agent").
payload: anymakes it easy to send malformed remove payloads (e.g.,{ agentId }vs{ agentId, chain }) with no TS help. Suggest mirroring the same context→payload typing approach here too. As per coding guidelines, ensure full TypeScript type coverage.
1235-1273:addAgentIdentity()should validate more proof/payload invariants.Right now it checks
agentId,evmAddress,proof, EVM address format, and thatproof.evmAddressmatchespayload.evmAddress. Consider also checking:
payload.proof.agentId === payload.agentIdpayload.proof.demosPublicKey === demos.getEd25519Address()(or at least non-empty + hex format)agentIdis numeric / BigInt-safe (to avoid accidental garbage values)payload.chainis present and matches supported chains (or restrict the type)As per coding guidelines, ensure full TypeScript type coverage + actionable errors.
1313-1337:verifyAgentOwnership()should not silently swallow all errors.Returning
falsefor any RPC/ABI/chain error makes debugging hard (rate limits, misconfigured RPC, wrong registry address, etc.). Suggest at least logging (behind a debug flag) or returning{ ok: boolean; reason?: string }for SDK consumers. Also validateexpectedOwnerformat before calling the chain.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/abstraction/Identities.tssrc/types/abstraction/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types/abstraction/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Ensure full TypeScript type coverage
Run type checks after changes
Use@/path aliases instead of relative imports
Use JSDoc format for all new methods and functions
Add// REVIEW:comments before newly added features
Provide clear, actionable error messages
Use descriptive names that clearly express intent
Files:
src/abstraction/Identities.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
src/abstraction/Identities.ts (2)
94-99: Nice: actionable web2 proof-format error message.The supported-formats list is a good UX improvement and should reduce support churn.
944-963: DetectSignatureType error string improvement is good.Clear “expected formats” messaging will help upstream callers quickly fix inputs.
…re/erc-8004-agent-identity # Conflicts: # src/abstraction/Identities.ts # src/abstraction/index.ts # src/types/abstraction/index.ts # src/types/blockchain/GCREdit.ts # src/websdk/GCRGeneration.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/types/abstraction/index.ts (2)
465-467:AgentIdentityPayloadTypebreaks the established payload-union naming convention.Every other payload union type follows
<Context>IdentityPayload(e.g.,XmIdentityPayload,Web2IdentityPayload,UdIdentityPayload). TheTypesuffix is unique to this entry. The name clash withAgentIdentityPayload(the inner data interface) is the root cause; consider renaming the inner data interface toAgentIdentityDatato free upAgentIdentityPayloadfor the union, keeping it consistent with the rest.♻️ Sketch of the rename
-export interface AgentIdentityPayload { … } // data +export interface AgentIdentityData { … } // data -export interface AgentIdentityAssignPayload … { - payload: AgentIdentityPayload +export interface AgentIdentityAssignPayload … { + payload: AgentIdentityData -export type AgentIdentityPayloadType = - | AgentIdentityAssignPayload - | AgentIdentityRemovePayload +export type AgentIdentityPayload = + | AgentIdentityAssignPayload + | AgentIdentityRemovePayload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/abstraction/index.ts` around lines 465 - 467, The union type name AgentIdentityPayloadType is inconsistent; rename the inner interface AgentIdentityPayload to AgentIdentityData, then change the union type name from AgentIdentityPayloadType to AgentIdentityPayload and update all references accordingly (e.g., adjust any usage sites that referenced AgentIdentityPayload to now use AgentIdentityData, and ensure AgentIdentityAssignPayload and AgentIdentityRemovePayload remain in the union). Also update any exports/imports that mention these symbols so the project compiles with the new AgentIdentityPayload (union) and AgentIdentityData (inner) names.
403-411:signature: string | { type: string; data: string }— document or remove the unusedstringvariant.
createAgentOwnershipProofalways produces the object form. Thestringvariant is never emitted, so either document the scenario in which a rawstringsignature is valid, or narrow the type to only{ type: string; data: string }to avoid misleading callers.♻️ Proposed narrowing
export interface DemosOwnershipProof { type: "demos-signature" message: string - signature: string | { type: string; data: string } + signature: { type: string; data: string } demosPublicKey: string agentId: string evmAddress: string timestamp: number }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/abstraction/index.ts` around lines 403 - 411, The DemosOwnershipProof.signature union includes an unused string variant; update the interface DemosOwnershipProof to narrow signature to only the object form { type: string; data: string } (since createAgentOwnershipProof only emits that shape), and adjust any dependent types or usages expecting a raw string; if you instead intend to support raw string signatures, add documentation and ensure createAgentOwnershipProof can emit that variant. Update references to DemosOwnershipProof and createAgentOwnershipProof accordingly so callers and type checks remain consistent.src/websdk/GCRGeneration.ts (1)
382-388:as anycast foragent_identity_assigndata signals a missingAgentGCRDatatype.The
ud_identity_assigncase (Lines 374–380) spreads its payload directly with no cast. Theas anyhere indicates thatGCREditIdentity.datainGCREdit.tsdoesn't yet include a typed shape for agent identity edits. Adding anAgentGCRDatatype (analogous toWeb2GCRData/Web2TLSNGCRData) would close this type-coverage gap, keep the data field typed end-to-end, and satisfy the full TypeScript type-coverage guideline.♻️ Sketch of the fix (in GCREdit.ts + here)
In
src/types/blockchain/GCREdit.ts, add:// REVIEW: AgentGCRData — ERC-8004 agent identity GCR data shape export interface AgentGCRData { agentId: string evmAddress: string chain: string txHash: string tokenUri: string proof: DemosOwnershipProof resolverUrl?: string timestamp: number }Then in
GCRGeneration.ts:case "agent_identity_assign": { edit.data = { ...identityPayload.payload, timestamp: tx.content.timestamp, - } as any + } as AgentGCRData break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/websdk/GCRGeneration.ts` around lines 382 - 388, Add a concrete AgentGCRData type and wire it into the GCREdit identity union, then replace the unsafe as any cast: declare an interface AgentGCRData (fields: agentId, evmAddress, chain, txHash, tokenUri, proof: DemosOwnershipProof, optional resolverUrl, timestamp: number) in the GCREdit type file and update the GCREditIdentity.data union to include AgentGCRData, then in the agent_identity_assign case replace the as any cast with the AgentGCRData type so edit.data is strongly typed.src/abstraction/Identities.ts (1)
1410-1410: Return typeany | nullshould beERC8004AgentCard | null.
any | nullcollapses toany, defeating TypeScript type coverage.ERC8004AgentCardis already defined in this codebase and reflects the correct return structure. This is required by the full TypeScript type coverage guideline.♻️ Proposed fix
- async getAgentCard(agentId: string): Promise<any | null> { + async getAgentCard(agentId: string): Promise<ERC8004AgentCard | null> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abstraction/Identities.ts` at line 1410, Change the getAgentCard signature to return ERC8004AgentCard | null instead of any | null and update any internal variables/return points to match that type; ensure ERC8004AgentCard is imported or referenced in this file and adapt the function's returned object shape (or cast only where unavoidable) so the TypeScript compiler recognizes the correct ERC8004AgentCard structure for getAgentCard.
🤖 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/abstraction/Identities.ts`:
- Around line 1282-1298: The JSDoc example for addAgentIdentity shows an
incorrect call to createAgentOwnershipProof missing the agentId parameter;
update the example so createAgentOwnershipProof is invoked with (demos, agentId,
evmAddress) to match the actual signature on the Identities class, and ensure
the example uses the same agentId value later when calling addAgentIdentity so
the sample is consistent with the createAgentOwnershipProof and addAgentIdentity
usages.
In `@src/types/abstraction/index.ts`:
- Around line 385-388: Add a `// REVIEW:` comment immediately before the new
ERC-8004 section header so it follows the project's coding guidelines;
specifically insert `// REVIEW:` directly above the `// SECTION ERC-8004 Agent
Identities` line in the file where the `BaseAgentIdentityPayload` interface is
declared to mark this new feature for review.
---
Duplicate comments:
In `@src/abstraction/Identities.ts`:
- Around line 1212-1220: The ownership message generated by
generateAgentOwnershipMessage currently only includes a timestamp and is
vulnerable to replay; update generateAgentOwnershipMessage(demosPublicKey,
agentId, evmAddress) to include a cryptographic nonce and domain-binding info
(chainId and registryAddress) in the human-readable message and return object.
Generate a secure random nonce (e.g., crypto.randomBytes or equivalent), accept
or fetch chainId and registryAddress (add params if needed), embed "chainId:
<chainId>, registry: <registryAddress>, nonce: <nonce>" in the message string
alongside the timestamp, and return { message, timestamp, nonce, chainId,
registryAddress } so callers can verify one-time, domain-scoped proofs. Ensure
existing callers are updated to handle the new return fields.
- Around line 1183-1184: The new section comment "// SECTION: ERC-8004 Agent
Identities" needs the required review marker; update the comment to prepend "//
REVIEW:" so it reads "// REVIEW: // SECTION: ERC-8004 Agent Identities" (or "//
REVIEW: SECTION: ERC-8004 Agent Identities" per project style) to comply with
coding guidelines and indicate this is a new feature area.
- Around line 1428-1446: The tokenUri parsing currently uses Node-only
Buffer.from, makes axios requests without timeouts, and accepts plain http which
violates ERC-8004; update the tokenUri handling block so: replace
Buffer.from(base64, "base64") with a cross-platform base64 decode that uses
Buffer when available and falls back to browser-safe APIs (e.g., globalThis.atob
+ TextDecoder) to produce UTF‑8 JSON; add a sensible request timeout option to
all axios.get calls that fetch IPFS/remote metadata (e.g., pass { timeout: <ms>
} when calling axios.get in the ipfs and http(s) branches); and tighten the URI
check to only allow "https://" (change tokenUri.startsWith("http") to
tokenUri.startsWith("https://")) and otherwise throw or return an error so only
data:application/json;base64, ipfs:// and https:// are accepted. Ensure these
changes are applied to the same tokenUri handling code paths where Buffer.from
and axios.get are referenced.
---
Nitpick comments:
In `@src/abstraction/Identities.ts`:
- Line 1410: Change the getAgentCard signature to return ERC8004AgentCard | null
instead of any | null and update any internal variables/return points to match
that type; ensure ERC8004AgentCard is imported or referenced in this file and
adapt the function's returned object shape (or cast only where unavoidable) so
the TypeScript compiler recognizes the correct ERC8004AgentCard structure for
getAgentCard.
In `@src/types/abstraction/index.ts`:
- Around line 465-467: The union type name AgentIdentityPayloadType is
inconsistent; rename the inner interface AgentIdentityPayload to
AgentIdentityData, then change the union type name from AgentIdentityPayloadType
to AgentIdentityPayload and update all references accordingly (e.g., adjust any
usage sites that referenced AgentIdentityPayload to now use AgentIdentityData,
and ensure AgentIdentityAssignPayload and AgentIdentityRemovePayload remain in
the union). Also update any exports/imports that mention these symbols so the
project compiles with the new AgentIdentityPayload (union) and AgentIdentityData
(inner) names.
- Around line 403-411: The DemosOwnershipProof.signature union includes an
unused string variant; update the interface DemosOwnershipProof to narrow
signature to only the object form { type: string; data: string } (since
createAgentOwnershipProof only emits that shape), and adjust any dependent types
or usages expecting a raw string; if you instead intend to support raw string
signatures, add documentation and ensure createAgentOwnershipProof can emit that
variant. Update references to DemosOwnershipProof and createAgentOwnershipProof
accordingly so callers and type checks remain consistent.
In `@src/websdk/GCRGeneration.ts`:
- Around line 382-388: Add a concrete AgentGCRData type and wire it into the
GCREdit identity union, then replace the unsafe as any cast: declare an
interface AgentGCRData (fields: agentId, evmAddress, chain, txHash, tokenUri,
proof: DemosOwnershipProof, optional resolverUrl, timestamp: number) in the
GCREdit type file and update the GCREditIdentity.data union to include
AgentGCRData, then in the agent_identity_assign case replace the as any cast
with the AgentGCRData type so edit.data is strongly typed.
Critical: - getAgentCard: cross-platform base64 decoding, axios timeouts, https-only - Ownership message: add nonce + chainId + registry for replay protection High: - getAgentCard return type: any|null → ERC8004AgentCard|null - Fix JSDoc example missing agentId param - Remove unnecessary `as any` cast in GCR generation - Add // REVIEW: markers per coding guidelines Type changes: - DemosOwnershipProof.signature narrowed to object form only - DemosOwnershipProof now includes nonce field
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/types/abstraction/index.ts (1)
467-469: Naming convention differs from other identity payload unions.Other identity unions follow the pattern
XmIdentityPayload,Web2IdentityPayload,PqcIdentityPayload, etc., but this usesAgentIdentityPayloadType. The inner data interfaceAgentIdentityPayload(line 436) occupies the typical union name.Consider renaming for consistency in a future refactor:
AgentIdentityPayload→AgentIdentityDataAgentIdentityPayloadType→AgentIdentityPayloadNot blocking for this PR since it's functional and changing it would require updates across multiple files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/abstraction/index.ts` around lines 467 - 469, Rename the inconsistent union and inner interface to match existing conventions by changing AgentIdentityPayload (the inner data interface) to AgentIdentityData and AgentIdentityPayloadType (the union) to AgentIdentityPayload; update all references to AgentIdentityPayloadType -> AgentIdentityPayload and AgentIdentityPayload -> AgentIdentityData, and ensure the union still composes AgentIdentityAssignPayload and AgentIdentityRemovePayload so code using those types continues to compile.src/abstraction/Identities.ts (2)
1187-1201: Hardcoded testnet configuration is acceptable for now.The registry address and chain config are currently scoped to Base Sepolia. For mainnet deployment, consider making these configurable (e.g., via constructor options or environment variables). Not blocking for testnet phase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abstraction/Identities.ts` around lines 1187 - 1201, The AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG constants in Identities.ts are hardcoded to Base Sepolia; make them configurable so mainnet values can be injected at runtime by replacing the static constants with configurable options: add constructor or factory overloads that accept registryAddress and chainConfig (or read from environment vars) and default to the existing AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG when not provided; update any references that use AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG to use the instance properties (e.g., this.agentRegistryAddress, this.agentChainConfig) so tests keep current defaults but deployments can supply mainnet values.
1412-1415: Consider distinguishing ownership failure from errors.The catch block returns
falsefor any error (network issues, rate limits, RPC failures), making it indistinguishable from "address doesn't own token". This complicates debugging.Since this is a client-side convenience helper and the node also verifies ownership, this isn't blocking. However, for better developer experience, consider either logging the error or returning a richer result:
💡 Optional improvement
- } catch (error: any) { - // Token doesn't exist or other error - return false + } catch (error: any) { + // Log for debugging - token may not exist, or network/RPC error occurred + console.warn(`verifyAgentOwnership failed for agentId ${agentId}:`, error?.message || error) + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abstraction/Identities.ts` around lines 1412 - 1415, The catch in src/abstraction/Identities.ts currently swallows all errors and returns false, conflating "doesn't own token" with runtime failures; update the helper (the ownership-check function in Identities.ts) to either log the caught error (e.g., console.error or your logger) before returning false or change the function's return shape to a richer union (e.g., { owns: boolean, error?: Error } or throw the error) so callers can distinguish ownership=false from transient errors; adjust the function signature and call sites accordingly to handle the new return/throw behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PR_REVIEW.md`:
- Around line 7-28: The PR_REVIEW.md is stale — it lists "NOT FIXED" for issues
that are already addressed; update or remove those entries: mark the
getAgentCard-related items as fixed (reference decodeBase64ToUtf8 replacing
Buffer.from, axios calls updated with { timeout: 10_000 }, and URLs validated
with startsWith("https://")), and mark the ownership-proof item as fixed
(reference generateAgentOwnershipMessage now including nonce via
ethers.randomBytes(16) and embedding chainId + registry, and updates to
DemosOwnershipProof and createAgentOwnershipProof), or delete the outdated
"CRITICAL — Must Fix" block entirely to avoid confusion.
- Around line 32-68: The PR_REVIEW.md HIGH-items list is stale: update or remove
the entries that are already fixed in code (getAgentCard return type now
Promise<ERC8004AgentCard | null> — ref: getAgentCard, JSDoc example corrected to
include agentId — ref: createAgentOwnershipProof, and missing REVIEW markers
added — ref: the // REVIEW: ERC-8004 Agent Identity markers); also confirm the
GCRGeneration cast issue was handled (remove unnecessary "as any" or cast to
AgentGCRData in GCRGeneration.ts if still present) and then either delete the
resolved rows or mark them as fixed so the document matches the actual code
state before merging.
---
Nitpick comments:
In `@src/abstraction/Identities.ts`:
- Around line 1187-1201: The AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG
constants in Identities.ts are hardcoded to Base Sepolia; make them configurable
so mainnet values can be injected at runtime by replacing the static constants
with configurable options: add constructor or factory overloads that accept
registryAddress and chainConfig (or read from environment vars) and default to
the existing AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG when not provided;
update any references that use AGENT_REGISTRY_ADDRESS and AGENT_CHAIN_CONFIG to
use the instance properties (e.g., this.agentRegistryAddress,
this.agentChainConfig) so tests keep current defaults but deployments can supply
mainnet values.
- Around line 1412-1415: The catch in src/abstraction/Identities.ts currently
swallows all errors and returns false, conflating "doesn't own token" with
runtime failures; update the helper (the ownership-check function in
Identities.ts) to either log the caught error (e.g., console.error or your
logger) before returning false or change the function's return shape to a richer
union (e.g., { owns: boolean, error?: Error } or throw the error) so callers can
distinguish ownership=false from transient errors; adjust the function signature
and call sites accordingly to handle the new return/throw behavior.
In `@src/types/abstraction/index.ts`:
- Around line 467-469: Rename the inconsistent union and inner interface to
match existing conventions by changing AgentIdentityPayload (the inner data
interface) to AgentIdentityData and AgentIdentityPayloadType (the union) to
AgentIdentityPayload; update all references to AgentIdentityPayloadType ->
AgentIdentityPayload and AgentIdentityPayload -> AgentIdentityData, and ensure
the union still composes AgentIdentityAssignPayload and
AgentIdentityRemovePayload so code using those types continues to compile.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
PR_REVIEW.mdsrc/abstraction/Identities.tssrc/types/abstraction/index.tssrc/websdk/GCRGeneration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/websdk/GCRGeneration.ts
| ## 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. |
There was a problem hiding this comment.
This review document appears stale — critical issues listed as "NOT FIXED" have been addressed in the code.
Cross-referencing with the actual code in src/abstraction/Identities.ts:
| Claimed Issue | Document Status | Actual Code State |
|---|---|---|
| Buffer.from breaks in browsers | NOT FIXED | ✅ Fixed: decodeBase64ToUtf8 helper at lines 1427-1437 |
| No axios timeouts | NOT FIXED | ✅ Fixed: { timeout: 10_000 } at lines 1470, 1476 |
| Accepts plain http:// | NOT FIXED | ✅ Fixed: startsWith("https://") at line 1473 |
| No replay protection/nonce | NOT FIXED | ✅ Fixed: nonce via ethers.randomBytes(16) + chainId + registry at lines 1218-1230 |
Either update this document to reflect the current code state or remove it to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PR_REVIEW.md` around lines 7 - 28, The PR_REVIEW.md is stale — it lists "NOT
FIXED" for issues that are already addressed; update or remove those entries:
mark the getAgentCard-related items as fixed (reference decodeBase64ToUtf8
replacing Buffer.from, axios calls updated with { timeout: 10_000 }, and URLs
validated with startsWith("https://")), and mark the ownership-proof item as
fixed (reference generateAgentOwnershipMessage now including nonce via
ethers.randomBytes(16) and embedding chainId + registry, and updates to
DemosOwnershipProof and createAgentOwnershipProof), or delete the outdated
"CRITICAL — Must Fix" block entirely to avoid confusion.
| ## 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. |
There was a problem hiding this comment.
HIGH priority items also appear to be resolved in the code.
| Claimed Issue | Document Status | Actual Code State |
|---|---|---|
| JSDoc example wrong signature | NOT FIXED | ✅ Fixed: Line 1301 shows createAgentOwnershipProof(demos, "123", "0x...") with 3 params |
Missing // REVIEW: markers |
NOT FIXED | ✅ Fixed: Present at Identities.ts:1184 and types/abstraction/index.ts:385 |
getAgentCard return type |
NOT FIXED | ✅ Fixed: Return type is Promise<ERC8004AgentCard | null> at line 1439 |
This document should be updated or removed before merge to avoid misleading future reviewers.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~66-~66: The official name of this software platform is spelled with a capital “H”.
Context: ...T FIXED Per project coding guidelines (.github/copilot-instructions.md), new features...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PR_REVIEW.md` around lines 32 - 68, The PR_REVIEW.md HIGH-items list is
stale: update or remove the entries that are already fixed in code (getAgentCard
return type now Promise<ERC8004AgentCard | null> — ref: getAgentCard, JSDoc
example corrected to include agentId — ref: createAgentOwnershipProof, and
missing REVIEW markers added — ref: the // REVIEW: ERC-8004 Agent Identity
markers); also confirm the GCRGeneration cast issue was handled (remove
unnecessary "as any" or cast to AgentGCRData in GCRGeneration.ts if still
present) and then either delete the resolved rows or mark them as fixed so the
document matches the actual code state before merging.
…re/erc-8004-agent-identity
…kynesyslabs/sdks into feature/erc-8004-agent-identity
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/types/abstraction/index.ts (1)
446-450: TightenDemosOwnershipProof.signature.typeto the algorithm union.Using
stringhere weakens type safety. PreferSigningAlgorithmto align with the rest of the abstraction types.Suggested fix
export interface DemosOwnershipProof { @@ - signature: { type: string; data: string } + signature: { type: SigningAlgorithm; data: string } @@ }As per coding guidelines "Ensure full TypeScript type coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/abstraction/index.ts` around lines 446 - 450, The DemosOwnershipProof interface currently types signature.type as string which weakens safety; change signature.type to the SigningAlgorithm union used elsewhere so the interface DemosOwnershipProof (and its signature field) references SigningAlgorithm instead of string, updating the signature property type in DemosOwnershipProof to { type: SigningAlgorithm; data: string } to align with other abstraction types.src/abstraction/Identities.ts (1)
1420-1441: JSDoc forgetAgentCardis currently orphaned.The block at Line 1420-Line 1425 is separated from
getAgentCardbydecodeBase64ToUtf8, so the new method lacks attached JSDoc.Suggested fix
- /** - * Fetch the agent card metadata from the token URI. - * - * `@param` agentId The ERC-8004 token ID - * `@returns` The agent card metadata or null if not found - */ /** * Decode a base64 string to UTF-8, with cross-platform support. */ private decodeBase64ToUtf8(base64: string): string { @@ } + /** + * Fetch the agent card metadata from the token URI. + * + * `@param` agentId The ERC-8004 token ID + * `@returns` The agent card metadata or null if not found + */ async getAgentCard(agentId: string): Promise<ERC8004AgentCard | null> {As per coding guidelines "Use JSDoc format for all new methods and functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abstraction/Identities.ts` around lines 1420 - 1441, The JSDoc intended for getAgentCard is orphaned because decodeBase64ToUtf8 sits between the comment block and the method; move or duplicate the JSDoc so it directly precedes the async getAgentCard(agentId: string): Promise<ERC8004AgentCard | null> declaration (remove the intermediate decodeBase64ToUtf8 placement or place its own JSDoc above decodeBase64ToUtf8) so the getAgentCard JSDoc is attached to getAgentCard; ensure the JSDoc describes parameters and return type consistent with ERC8004AgentCard and matches project JSDoc style.
🤖 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/abstraction/Identities.ts`:
- Around line 1402-1417: The current try/catch around creating the
JsonRpcProvider and calling contract.ownerOf(agentId) swallows all errors and
returns false for transient RPC/network issues; instead, narrow the catch: call
new ethers.JsonRpcProvider(...) and contract.ownerOf(agentId) as before, but in
the catch inspect the error (e.g., error.code or error.message) and only return
false for the specific ERC-721 "nonexistent token" error (or a deterministic
owner-query-not-found message), while rethrowing other errors so callers can
handle RPC/network failures; reference the symbols provider, contract, ownerOf,
Identities.AGENT_REGISTRY_ADDRESS and Identities.AGENT_CHAIN_CONFIG.rpc when
updating the logic.
- Around line 1467-1473: The IPFS gateway handling in the tokenUri branch should
normalize various ipfs:// forms (e.g., ipfs://ipfs/<cid>, ipfs:///<cid>) before
building the gateway URL; instead of using tokenUri.replace("ipfs://", ""),
parse tokenUri to strip the leading scheme and any leading "ipfs/" segment or
extra slashes to produce a clean ipfsHash/path, then use that normalized value
when calling axios.get; update the logic around tokenUri and the ipfsHash
variable in the Identites.ts block handling tokenUri.startsWith("ipfs://") so
the gateway URL is always valid for common IPFS URI variants.
- Around line 1321-1348: The addAgentIdentity payload validation currently only
checks evmAddress; update addAgentIdentity to also assert that
payload.proof.agentId === payload.agentId (throw a clear Error if mismatched)
and, if available, validate payload.proof.demosPublicKey against the connected
identity's public key (e.g. compare payload.proof.demosPublicKey to
connectedIdentity.publicKey or payload.demosPublicKey) to prevent mismatched
proof/payload combos; place these checks alongside the existing
proof/evenAddress validation and include descriptive error messages referencing
agentId and demosPublicKey.
In `@src/types/abstraction/index.ts`:
- Around line 478-493: The AgentIdentityPayload interface does not include the
runtime timestamp added by the agent GCR add path; update the
AgentIdentityPayload type to include an optional timestamp (e.g., timestamp?:
number) so it matches the shape produced by GCRGeneration when adding agents, or
alternatively introduce two types (e.g., AgentIdentityRequest vs
AgentIdentityStored) and use the request type in GCRGeneration to keep persisted
types unchanged; ensure references to AgentIdentityPayload (and usages in
GCRGeneration) are updated to the chosen type so TypeScript reflects the runtime
shape.
In `@src/websdk/GCRGeneration.ts`:
- Around line 382-388: In the GCRGeneration code inside the case
"agent_identity_assign", the generated edit object doesn't carry over optional
referral data; set edit.referralCode to the referral value from the identity
payload (e.g., edit.referralCode = identityPayload.payload.referralCode or
assign conditionally if present) so the referral code is propagated along with
the existing fields like timestamp.
---
Nitpick comments:
In `@src/abstraction/Identities.ts`:
- Around line 1420-1441: The JSDoc intended for getAgentCard is orphaned because
decodeBase64ToUtf8 sits between the comment block and the method; move or
duplicate the JSDoc so it directly precedes the async getAgentCard(agentId:
string): Promise<ERC8004AgentCard | null> declaration (remove the intermediate
decodeBase64ToUtf8 placement or place its own JSDoc above decodeBase64ToUtf8) so
the getAgentCard JSDoc is attached to getAgentCard; ensure the JSDoc describes
parameters and return type consistent with ERC8004AgentCard and matches project
JSDoc style.
In `@src/types/abstraction/index.ts`:
- Around line 446-450: The DemosOwnershipProof interface currently types
signature.type as string which weakens safety; change signature.type to the
SigningAlgorithm union used elsewhere so the interface DemosOwnershipProof (and
its signature field) references SigningAlgorithm instead of string, updating the
signature property type in DemosOwnershipProof to { type: SigningAlgorithm;
data: string } to align with other abstraction types.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3455162e-8686-43ed-ac69-1c0cedf906f5
📒 Files selected for processing (5)
src/abstraction/Identities.tssrc/abstraction/index.tssrc/types/abstraction/index.tssrc/types/blockchain/GCREdit.tssrc/websdk/GCRGeneration.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/abstraction/index.ts
- src/types/blockchain/GCREdit.ts
| // Validate the payload | ||
| if (!payload.agentId) { | ||
| throw new Error("Agent ID is required") | ||
| } | ||
| if (!payload.evmAddress) { | ||
| throw new Error("EVM address is required") | ||
| } | ||
| if (!payload.proof) { | ||
| throw new Error("Ownership proof is required") | ||
| } | ||
|
|
||
| // Verify the EVM address format | ||
| const evmPattern = /^0x[0-9a-fA-F]{40}$/ | ||
| if (!evmPattern.test(payload.evmAddress)) { | ||
| throw new Error( | ||
| `Invalid EVM address format: ${payload.evmAddress}`, | ||
| ) | ||
| } | ||
|
|
||
| // Verify the proof contains the correct EVM address | ||
| if ( | ||
| payload.proof.evmAddress.toLowerCase() !== | ||
| payload.evmAddress.toLowerCase() | ||
| ) { | ||
| throw new Error( | ||
| "Ownership proof EVM address doesn't match payload EVM address", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Validate full proof-to-payload binding in addAgentIdentity.
Current checks only bind evmAddress. Add at least proof.agentId === payload.agentId (and ideally proof.demosPublicKey against the connected identity) to prevent inconsistent proof/payload combinations from passing local validation.
Suggested fix
// Verify the proof contains the correct EVM address
if (
payload.proof.evmAddress.toLowerCase() !==
payload.evmAddress.toLowerCase()
) {
throw new Error(
"Ownership proof EVM address doesn't match payload EVM address",
)
}
+
+ if (payload.proof.agentId !== payload.agentId) {
+ throw new Error(
+ "Ownership proof agentId doesn't match payload agentId",
+ )
+ }
+
+ const connectedDemosKey = await demos.getEd25519Address()
+ if (payload.proof.demosPublicKey !== connectedDemosKey) {
+ throw new Error(
+ "Ownership proof demosPublicKey doesn't match connected identity",
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/abstraction/Identities.ts` around lines 1321 - 1348, The addAgentIdentity
payload validation currently only checks evmAddress; update addAgentIdentity to
also assert that payload.proof.agentId === payload.agentId (throw a clear Error
if mismatched) and, if available, validate payload.proof.demosPublicKey against
the connected identity's public key (e.g. compare payload.proof.demosPublicKey
to connectedIdentity.publicKey or payload.demosPublicKey) to prevent mismatched
proof/payload combos; place these checks alongside the existing
proof/evenAddress validation and include descriptive error messages referencing
agentId and demosPublicKey.
| try { | ||
| const provider = new ethers.JsonRpcProvider( | ||
| Identities.AGENT_CHAIN_CONFIG.rpc, | ||
| ) | ||
| const contract = new ethers.Contract( | ||
| Identities.AGENT_REGISTRY_ADDRESS, | ||
| registryAbi, | ||
| provider, | ||
| ) | ||
|
|
||
| const owner = await contract.ownerOf(agentId) | ||
| return owner.toLowerCase() === expectedOwner.toLowerCase() | ||
| } catch (error: any) { | ||
| // Token doesn't exist or other error | ||
| return false | ||
| } |
There was a problem hiding this comment.
Do not collapse all verification errors into false.
Returning false for every exception makes transient RPC/network issues indistinguishable from real non-ownership, which can mislead callers and UI flows.
Suggested fix
try {
@@
const owner = await contract.ownerOf(agentId)
return owner.toLowerCase() === expectedOwner.toLowerCase()
} catch (error: any) {
- // Token doesn't exist or other error
- return false
+ const message = String(error?.message || "")
+ if (
+ message.toLowerCase().includes("nonexistent token") ||
+ message.toLowerCase().includes("erc721")
+ ) {
+ return false
+ }
+ throw new Error(`Failed to verify agent ownership: ${message}`)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/abstraction/Identities.ts` around lines 1402 - 1417, The current
try/catch around creating the JsonRpcProvider and calling
contract.ownerOf(agentId) swallows all errors and returns false for transient
RPC/network issues; instead, narrow the catch: call new
ethers.JsonRpcProvider(...) and contract.ownerOf(agentId) as before, but in the
catch inspect the error (e.g., error.code or error.message) and only return
false for the specific ERC-721 "nonexistent token" error (or a deterministic
owner-query-not-found message), while rethrowing other errors so callers can
handle RPC/network failures; reference the symbols provider, contract, ownerOf,
Identities.AGENT_REGISTRY_ADDRESS and Identities.AGENT_CHAIN_CONFIG.rpc when
updating the logic.
| } else if (tokenUri.startsWith("ipfs://")) { | ||
| // IPFS URI - fetch via gateway | ||
| const ipfsHash = tokenUri.replace("ipfs://", "") | ||
| const response = await axios.get( | ||
| `https://ipfs.io/ipfs/${ipfsHash}`, | ||
| { timeout: 10_000 }, | ||
| ) |
There was a problem hiding this comment.
Normalize IPFS URIs before gateway resolution.
replace("ipfs://", "") fails for common ipfs://ipfs/<cid> URIs and can produce invalid gateway URLs.
Suggested fix
} else if (tokenUri.startsWith("ipfs://")) {
// IPFS URI - fetch via gateway
- const ipfsHash = tokenUri.replace("ipfs://", "")
+ const ipfsPath = tokenUri.replace(/^ipfs:\/\//, "")
+ const normalizedIpfsPath = ipfsPath.replace(/^ipfs\//, "")
const response = await axios.get(
- `https://ipfs.io/ipfs/${ipfsHash}`,
+ `https://ipfs.io/ipfs/${normalizedIpfsPath}`,
{ timeout: 10_000 },
)
return response.data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/abstraction/Identities.ts` around lines 1467 - 1473, The IPFS gateway
handling in the tokenUri branch should normalize various ipfs:// forms (e.g.,
ipfs://ipfs/<cid>, ipfs:///<cid>) before building the gateway URL; instead of
using tokenUri.replace("ipfs://", ""), parse tokenUri to strip the leading
scheme and any leading "ipfs/" segment or extra slashes to produce a clean
ipfsHash/path, then use that normalized value when calling axios.get; update the
logic around tokenUri and the ipfsHash variable in the Identites.ts block
handling tokenUri.startsWith("ipfs://") so the gateway URL is always valid for
common IPFS URI variants.
| export interface AgentIdentityPayload { | ||
| /** ERC-8004 agent ID (NFT token ID) */ | ||
| agentId: string | ||
| /** EVM address that owns the agent NFT */ | ||
| evmAddress: string | ||
| /** The chain where the agent is registered (e.g., "base.sepolia") */ | ||
| chain: string | ||
| /** Transaction hash of the agent registration */ | ||
| txHash: string | ||
| /** Token URI pointing to the agent card metadata */ | ||
| tokenUri: string | ||
| /** Ownership proof signed by Demos wallet */ | ||
| proof: DemosOwnershipProof | ||
| /** Optional resolver URL for the agent */ | ||
| resolverUrl?: string | ||
| } |
There was a problem hiding this comment.
AgentIdentityPayload is missing the timestamp field used by the agent GCR add path.
src/websdk/GCRGeneration.ts (Line 382-Line 386) adds timestamp to agent identity data, but this interface does not represent that runtime shape. Please add an optional timestamp (or split request vs persisted payload types) to keep typing accurate.
Suggested fix
export interface AgentIdentityPayload {
@@
/** Optional resolver URL for the agent */
resolverUrl?: string
+ /** Auto-populated during GCR generation */
+ timestamp?: number
}As per coding guidelines "Ensure full TypeScript type coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/abstraction/index.ts` around lines 478 - 493, The
AgentIdentityPayload interface does not include the runtime timestamp added by
the agent GCR add path; update the AgentIdentityPayload type to include an
optional timestamp (e.g., timestamp?: number) so it matches the shape produced
by GCRGeneration when adding agents, or alternatively introduce two types (e.g.,
AgentIdentityRequest vs AgentIdentityStored) and use the request type in
GCRGeneration to keep persisted types unchanged; ensure references to
AgentIdentityPayload (and usages in GCRGeneration) are updated to the chosen
type so TypeScript reflects the runtime shape.
| case "agent_identity_assign": { | ||
| edit.data = { | ||
| ...identityPayload.payload, | ||
| timestamp: tx.content.timestamp, | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
Propagate agent referral codes into the generated identity edit.
In the agent_identity_assign branch, edit.referralCode is never set, so optional referral data is dropped for this flow.
Suggested fix
case "agent_identity_assign": {
edit.data = {
...identityPayload.payload,
timestamp: tx.content.timestamp,
}
+ edit.referralCode = identityPayload.referralCode ?? null
break
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/websdk/GCRGeneration.ts` around lines 382 - 388, In the GCRGeneration
code inside the case "agent_identity_assign", the generated edit object doesn't
carry over optional referral data; set edit.referralCode to the referral value
from the identity payload (e.g., edit.referralCode =
identityPayload.payload.referralCode or assign conditionally if present) so the
referral code is propagated along with the existing fields like timestamp.
2 participants
PR Type
Enhancement
Description
Added comprehensive ERC-8004 agent identity support to Demos SDK
Implemented agent ownership proof creation and verification methods
Added methods to manage agent identities (add, remove, get)
Extended type definitions and GCR handling for agent identities
Added client-side helpers for NFT ownership verification and metadata retrieval
Diagram Walkthrough
File Walkthrough
Identities.ts
Implement ERC-8004 agent identity operationssrc/abstraction/Identities.ts
"agent"context support toinferIdentity()andremoveIdentity()methods
createAgentOwnershipProof()to generate signed ownershipproofs
addAgentIdentity()method with payload validation and EVMaddress verification
removeAgentIdentity()andgetAgentIdentities()methods for agentlifecycle management
verifyAgentOwnership()to check NFT ownership on BaseSepolia registry
getAgentCard()to fetch agent metadata from various URIformats (data URI, IPFS, HTTP)
AGENT_REGISTRY_ADDRESSandAGENT_CHAIN_CONFIGindex.ts
Export agent identity types and interfacessrc/abstraction/index.ts
AgentIdentityPayload,AgentIdentityAssignPayload,AgentIdentityRemovePayload,AgentIdentityPayloadTypeDemosOwnershipProof,ERC8004AgentCard,ERC8004Endpointindex.ts
Define ERC-8004 agent identity type structuressrc/types/abstraction/index.ts
ERC8004Endpointinterface for agent endpoint configurationDemosOwnershipProofinterface for signature-based ownershipverification
ERC8004AgentCardinterface based on EIP-8004 specificationAgentIdentityPayloadinterface with agent ID, EVM address,chain, transaction hash, and token URI
AgentIdentityAssignPayloadandAgentIdentityRemovePayloadinterfaces
AgentIdentityPayloadTypeunion typeIdentityPayloadunion to include agent identity typesUserPointsbreakdown to includeagentsfieldUserPointsto includelinkedAgentsfield with agent metadataGCREdit.ts
Extend GCR edit types for agent identitiessrc/types/blockchain/GCREdit.ts
AgentGCRDatatype alias for agent identity GCR dataGCREditIdentitycontext type to include"agent"operations
agentIdandchainfieldsGCRGeneration.ts
Handle agent identity GCR generationsrc/websdk/GCRGeneration.ts
"agent_identity_assign"to process agentidentity assignments
"agent_identity_remove"to process agentidentity removals
generation
Summary by CodeRabbit
New Features
Documentation