Conversation
|
@codex review |
|
cursor review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ed8253cfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .optional() | ||
| .describe('Override API base URL for registration relay; defaults to https://x402-worldchain.vercel.app'), | ||
| }), | ||
| description: 'Register this agent with a World ID proof.', |
There was a problem hiding this comment.
Update the root quick start for addressless registration
Update the repository's primary registration instructions alongside this removal of the address argument. README.md:34-48 still directs users to run register <your-wallet-address> and describes Base, manual mode, custom relays, and Base Sepolia, but this command now accepts no address or options and creates its own World Chain identity. Users following the top-level quick start will either receive an unexpected-argument error or, depending on argument handling, register a different generated identity than the wallet they supplied.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| export type AgentkitSignatureVerificationConfig = string | AgentkitSignatureVerificationOptions | ||
| const AGENTKIT_HEADER = 'X-AgentKit' |
There was a problem hiding this comment.
| const AGENTKIT_HEADER = 'X-AgentKit' | |
| const AGENTKIT_HEADER = 'AgentKit' |
the X- convention is no longer recommended
| return options?.rpcUrls?.[chainId] ?? options?.rpcUrl | ||
| type VerifyRequestDependencies = { | ||
| recoverAddress?: (body: Uint8Array, signature: Hex) => Promise<string> | ||
| lookupNullifierHash?: (address: string) => Promise<string | null> |
There was a problem hiding this comment.
nit. let's start moving away from the confusing nullifier hash terminology, maybe lookupId?
| export async function verifyRequest(request: Request, dependencies: VerifyRequestDependencies = {}): Promise<string> { | ||
| const signature = request.headers.get(AGENTKIT_HEADER)?.trim() | ||
| if (!signature) { | ||
| throw verificationError('Missing X-AgentKit header', 'MISSING_HEADER') |
There was a problem hiding this comment.
| throw verificationError('Missing X-AgentKit header', 'MISSING_HEADER') | |
| throw verificationError(`Missing ${AGENTKIT_HEADER} header`, 'MISSING_HEADER') |
| if (!signature) { | ||
| throw verificationError('Missing X-AgentKit header', 'MISSING_HEADER') | ||
| } | ||
| if (!isHex(signature) || !/^0x[0-9a-fA-F]{130}$/.test(signature)) { |
There was a problem hiding this comment.
seems redundant to do isHex and this regex
| valid: false, | ||
| error: `Signature verification error: ${reason}. The SIWE message the server reconstructed from your payload:\n\n${message}`, | ||
| } | ||
| body = new Uint8Array(await request.clone().arrayBuffer()) |
There was a problem hiding this comment.
nit. I wonder if there's a way to absorb the request body into the sha input blocks without an additional copy in memory
| ```bash | ||
| API_URL=https://your-api.example.com agentkit register 0x1234567890abcdef1234567890abcdef12345678 --network base-sepolia --auto | ||
| ``` | ||
| ## Sign an x402 request body |
There was a problem hiding this comment.
I don't think this particularly applies to x402
| */ | ||
| tryIncrementUsage(endpoint: string, humanId: string, limit: number): Promise<boolean> | ||
|
|
||
| hasUsedNonce?(nonce: string): Promise<boolean> |
There was a problem hiding this comment.
nonce usage should remain
| type VerifyFunction = (request: Request) => Promise<string> | ||
| type RecoverAddressFunction = (body: Uint8Array, signature: Hex) => Promise<string> | ||
|
|
||
| export function createAgentkitHooksInternal( |
There was a problem hiding this comment.
nit. internal function exported?
| ## Use AgentKit | ||
|
|
||
| Register your wallet in AgentBook so servers can verify you are human-backed. Registration is gasless by default (uses a hosted relay on Base mainnet). | ||
| Add the AgentKit x402 skill: |
There was a problem hiding this comment.
high we have mentions to x402 everywhere, it should be focused on the new version
| - **Smart wallet support**: EVM verification automatically supports both smart contract wallets (ERC-1271) and EOA wallets via RPC calls to the chain. | ||
| - **On-chain verification**: AgentBook lookups happen at request time, so revoked registrations take effect immediately. | ||
| - **Per-human tracking**: Usage limits are tracked by anonymous human identifier, not by wallet address. Multiple agents controlled by one person share a single counter. | ||
| - The signature authenticates only the normalized request body. It does not automatically bind the method, URL, host, audience, timestamp, or nonce. Put any required context in the signed body and validate it in the application. |
There was a problem hiding this comment.
this needs to be fixed
| return join(configHome, 'agentkit', 'key') | ||
| } | ||
|
|
||
| export async function loadOrCreateAgentIdentity(keyPath: string = getAgentkitKeyPath()): Promise<AgentIdentity> { |
There was a problem hiding this comment.
This is hiding the env, configured home etc. within the key.ts CLI file. Feel like this stuff should be configured in the index.ts and injected in, not critical though
| } | ||
|
|
||
| export function getAgentkitKeyPath( | ||
| env: NodeJS.ProcessEnv = process.env, |
There was a problem hiding this comment.
Why pass the whole env into the function? Why not just specify the path in the index.ts and inject in?
You could construct a config type object in the index.ts cleanly and inject it into the CLI
| args: [signer.address], | ||
| }) | ||
| } catch (err) { | ||
| return c.error({ |
There was a problem hiding this comment.
All of this error handling feels to me like it should be handled within the ClientContract class
|
|
||
| const temporaryDirectories: string[] = [] | ||
|
|
||
| afterEach(async () => { |
There was a problem hiding this comment.
Have done a fresh install of this branch and run bun test. Getting some test failures around
# Unhandled error between tests
-------------------------------
error: Cannot find module 'ajv/dist/core' from '/Users/thomas.waite/Documents/tfh/agentkit/node_modules/ajv-draft-04/dist/index.js'
Simplify the AgentKit SDK & CLI
Context (how things worked before)
CLI
Previously, the CLI provided a single "register" command, which accepted an ETH address and would register it with the AgentBook contract on the provided chain. It either printed out the transaction details, or used an API endpoint to register. The API address was customizable as a flag.
Core
When building the Shopify integration, I did a quick refactor to split the AgentKit SDK into two packages:
coreandx402. This was done pretty quickly to get the integration done in time, and mostly just exposed an escape hatch to verify x402-shaped signatures without having to pull in their entire SDK.x402
The
x402package contained all the x402-specific parts remaining after the core split. It implements a middleware that checks requests for agentkit data and, if present, applies a configurable discount or free trial.How things work now
CLI
The CLI now manages your agent keys for you. It gets saved in your config directory and used for all calls.
There are now two commands. The first one,
register, requests a World ID proof from the user and registers the agent's pKey on the AgentBook. Future calls do nothing.The
prove {req}command receives a request body, and returns the signed body back to the caller. It errors if the pKey is not registered in the AgentBook, prompting the agent to verify it.Both of these commands are intended to be called by agents, although humans can also use them.
Core
The core package has been pretty much completely rewritten. It now exposes a
verify(request)function that checks that the given request:If those checks succeed, it returns the nullifier hash of the human that the agent acts on behalf of. Otherwise it throws.
x402
The x402 package remains mostly unchanged, just switched to using the new signing implementation in
core.