feat: add MCP server for ACK-ID and ACK-Pay operations#72
feat: add MCP server for ACK-ID and ACK-Pay operations#72ak68a wants to merge 3 commits intoagentcommercekit:mainfrom
Conversation
Expose 9 tools via stdio MCP transport: identity (create/sign/verify credentials, resolve DIDs), payments (create/verify payment requests and receipts), and keypair generation. Signing tools accept JWK to prevent curve mismatches. MCP SDK pinned to 1.21.2 for zod 3 compatibility.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a new MCP server package at tools/mcp-server with an ES-module CLI, server entrypoint, tool registrations for identity, payment requests/receipts, utility key generation, shared utilities/resolver, TypeScript and Vitest configs, and test suites covering core flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tools/mcp-server/package.json (1)
17-19: Consider addingtsxto devDependencies.The
bin.ack-mcpentry (line 18) points to a TypeScript file, and thestartscript (line 25) explicitly usestsx. Whiletsxis available from the workspace root, adding it explicitly to this package's devDependencies would make the dependency clear and the package more self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mcp-server/package.json` around lines 17 - 19, The package.json currently exposes a TypeScript entrypoint ("bin"."ack-mcp": "./src/index.ts") and the start script relies on tsx; add "tsx" to this package's devDependencies (matching your workspace version or a compatible semver) so the package is self-contained—update package.json devDependencies to include tsx and run the package install (or add via npm/yarn pnpm) to lock it in.tools/mcp-server/src/tools/payment-requests.ts (2)
77-81: Condition always truthy due to default value.Since
expiresInSecondsdefaults to3600(line 51), the conditionif (expiresInSeconds)will always be true for normal usage. However, if a caller explicitly passes0to disable expiration, the falsy check would skip settingexpiresAt—which may be intentional.If
0should explicitly mean "no expiration," consider documenting this behavior in the schema description, or use an explicitundefinedcheck:if (expiresInSeconds !== undefined && expiresInSeconds > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mcp-server/src/tools/payment-requests.ts` around lines 77 - 81, The conditional using expiresInSeconds currently always evaluates truthy because a default of 3600 is applied; update the check around setting init.expiresAt so callers can explicitly pass 0 to mean "no expiration" — replace the truthy check with an explicit undefined and positive-value check (e.g., check expiresInSeconds !== undefined && expiresInSeconds > 0) in the block that assigns init.expiresAt; alternatively, document in the input schema or parameter docs that 0 means no expiration if you prefer to keep the current behavior.
43-64: Consider using Zod's.min(1)instead of manual validation.The manual check on lines 62-64 is redundant with Zod's built-in array constraints. Using
.min(1)would validate at schema level and provide consistent error handling.♻️ Suggested refactor
paymentOptions: z .array(paymentOptionSchema) + .min(1, "At least one payment option is required") .describe( "Array of payment options (amount, currency, recipient, network)", ),Then remove the manual check:
async ({ description, paymentOptions, expiresInSeconds, jwk, did }) => { try { - if (paymentOptions.length === 0) { - throw new Error("At least one payment option is required") - } - const keypair = keypairFromJwk(jwk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mcp-server/src/tools/payment-requests.ts` around lines 43 - 64, Add Zod array constraint to validate at schema level by changing the paymentOptions schema definition (the z.array(paymentOptionSchema) in the handler/schema block) to use .min(1) so empty arrays are rejected by Zod; then remove the manual runtime check that throws "At least one payment option is required" inside the async handler (the if (paymentOptions.length === 0) { throw new Error(...) } block) to avoid redundant validation and rely on Zod's consistent error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/mcp-server/package.json`:
- Around line 17-19: The package.json currently exposes a TypeScript entrypoint
("bin"."ack-mcp": "./src/index.ts") and the start script relies on tsx; add
"tsx" to this package's devDependencies (matching your workspace version or a
compatible semver) so the package is self-contained—update package.json
devDependencies to include tsx and run the package install (or add via npm/yarn
pnpm) to lock it in.
In `@tools/mcp-server/src/tools/payment-requests.ts`:
- Around line 77-81: The conditional using expiresInSeconds currently always
evaluates truthy because a default of 3600 is applied; update the check around
setting init.expiresAt so callers can explicitly pass 0 to mean "no expiration"
— replace the truthy check with an explicit undefined and positive-value check
(e.g., check expiresInSeconds !== undefined && expiresInSeconds > 0) in the
block that assigns init.expiresAt; alternatively, document in the input schema
or parameter docs that 0 means no expiration if you prefer to keep the current
behavior.
- Around line 43-64: Add Zod array constraint to validate at schema level by
changing the paymentOptions schema definition (the z.array(paymentOptionSchema)
in the handler/schema block) to use .min(1) so empty arrays are rejected by Zod;
then remove the manual runtime check that throws "At least one payment option is
required" inside the async handler (the if (paymentOptions.length === 0) { throw
new Error(...) } block) to avoid redundant validation and rely on Zod's
consistent error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08757648-7bf6-4ba2-aa7d-59eadde0d8c2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
tools/mcp-server/package.jsontools/mcp-server/src/index.tstools/mcp-server/src/tools/identity.test.tstools/mcp-server/src/tools/identity.tstools/mcp-server/src/tools/payment-receipts.tstools/mcp-server/src/tools/payment-requests.test.tstools/mcp-server/src/tools/payment-requests.tstools/mcp-server/src/tools/utility.tstools/mcp-server/src/util.test.tstools/mcp-server/src/util.tstools/mcp-server/tsconfig.jsontools/mcp-server/vitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/mcp-server/src/tools/workflow.test.ts (1)
33-130: Test currently validates SDK workflow, not MCP tool contract.This covers core
agentcommercekitflows well, but it does not execute MCP tool handlers (e.g.,ack_verify_payment_request) and therefore misses wrapper behavior like structuredverification(false, { reason })responses vs thrown errors. Add at least one integration path through registered MCP tools to validate real tool I/O contracts.Also applies to: 157-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mcp-server/src/tools/workflow.test.ts` around lines 33 - 130, The test exercises SDK flows directly (createSignedPaymentRequest, verifyPaymentRequestToken, createPaymentReceipt, verifyPaymentReceipt) but must also assert real MCP tool I/O by invoking the registered MCP handler (e.g., ack_verify_payment_request) so wrapper behavior (structured verification(false, { reason }) vs thrown errors) is validated; update the test to register or use the existing MCP tools/dispatcher, call the ack_verify_payment_request handler with the produced paymentRequestToken (instead of directly calling verifyPaymentRequestToken) and assert the handler returns the expected structured verification response, then mirror this integration approach for the second similar test block that covers receipt/payment verification to ensure tool-level contracts are asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/mcp-server/src/tools/workflow.test.ts`:
- Around line 34-35: Rename the non-assertive test titles to follow the
project's assertive naming pattern (it("creates...", it("throws...",
it("requires...", it("returns...")). For the test currently titled "completes an
identity + payment cycle end-to-end" (and the other tests flagged at the same
file), update the string to an assertive form such as "creates an identity and
processes payment end-to-end"; similarly replace any "completes..." with
"creates..." and "rejects..." with "throws..." (or other appropriate assertive
verbs) so that the test descriptions in the workflow.test.ts suite conform to
the required pattern and remain semantically equivalent.
---
Nitpick comments:
In `@tools/mcp-server/src/tools/workflow.test.ts`:
- Around line 33-130: The test exercises SDK flows directly
(createSignedPaymentRequest, verifyPaymentRequestToken, createPaymentReceipt,
verifyPaymentReceipt) but must also assert real MCP tool I/O by invoking the
registered MCP handler (e.g., ack_verify_payment_request) so wrapper behavior
(structured verification(false, { reason }) vs thrown errors) is validated;
update the test to register or use the existing MCP tools/dispatcher, call the
ack_verify_payment_request handler with the produced paymentRequestToken
(instead of directly calling verifyPaymentRequestToken) and assert the handler
returns the expected structured verification response, then mirror this
integration approach for the second similar test block that covers
receipt/payment verification to ensure tool-level contracts are asserted.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89e9b0c7-84f6-49d9-8d53-69caec8d0b2c
📒 Files selected for processing (1)
tools/mcp-server/src/tools/workflow.test.ts
| it("completes an identity + payment cycle end-to-end", async () => { | ||
| // 1. Generate keypairs for owner and agent |
There was a problem hiding this comment.
Rename test cases to required assertive naming pattern.
These titles use “completes/rejects”; guideline requires patterns like creates..., throws..., requires..., returns....
Proposed rename diff
- it("completes an identity + payment cycle end-to-end", async () => {
+ it("creates an identity + payment cycle end-to-end", async () => {
- it("rejects a credential signed by the wrong key", async () => {
+ it("throws when a credential is signed by the wrong key", async () => {
- it("rejects a payment request from an untrusted issuer", async () => {
+ it("throws when a payment request comes from an untrusted issuer", async () => {As per coding guidelines "Use assertive test names with patterns: "it(\"creates...\")" , "it(\"throws...\")" , "it(\"requires...\")" , "it(\"returns...\")"".
Also applies to: 132-133, 157-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/mcp-server/src/tools/workflow.test.ts` around lines 34 - 35, Rename the
non-assertive test titles to follow the project's assertive naming pattern
(it("creates...", it("throws...", it("requires...", it("returns...")). For the
test currently titled "completes an identity + payment cycle end-to-end" (and
the other tests flagged at the same file), update the string to an assertive
form such as "creates an identity and processes payment end-to-end"; similarly
replace any "completes..." with "creates..." and "rejects..." with "throws..."
(or other appropriate assertive verbs) so that the test descriptions in the
workflow.test.ts suite conform to the required pattern and remain semantically
equivalent.
1 participant
Adds an MCP server that exposes ACK identity and payment operations as tools. Any MCP-compatible agent can generate keypairs, create and verify credentials, issue payment requests, and verify receipts.
Tools
Identity (ACK-ID)
ack_create_controller_credential— create ownership proofack_sign_credential— sign a credential, returns JWTack_verify_credential— verify signature, expiry, trusted issuersack_resolve_did— resolve did:key, did:web, did:pkhPayments (ACK-Pay)
ack_create_payment_request— create signed payment request tokenack_verify_payment_request— verify and parse a payment request JWTack_create_payment_receipt— issue a receipt as a Verifiable Credentialack_verify_payment_receipt— verify a payment receiptUtility
ack_generate_keypair— generate secp256k1/secp256r1/Ed25519 keypair with DIDSigning tools accept JWK (returned by
ack_generate_keypair) rather than raw private key + curve to prevent curve mismatches.Uses stdio transport. MCP SDK pinned to 1.21.2 for zod 3 compatibility with the workspace.
Also added a workflow eval that exercises the full pipeline end-to-end:
Figured if we're shipping MCP tools we should prove the whole cycle actually works.
Test plan
pnpm run checkcleanSummary by CodeRabbit