fix: move payment settlement from requirePayment() to middleware

packages/atxp-express/src/atxpExpress.ts

@@ -12,14 +12,14 @@
   sendProtectedResourceMetadataNode,
   sendOAuthMetadataNode,
   detectProtocol,
-  setDetectedCredential,
   getPendingPaymentChallenge,
   type PaymentProtocol,
   type ATXPConfig,
   type TokenCheck,
   type PendingPaymentChallenge,
   verifyOpaqueIdentity,
   parseCredentialBase64,
+  ProtocolSettlement,
 } from "@atxp/server";
 
 export function atxpExpress(args: ATXPArgs): Router {
@@ -105,36 +105,62 @@
         return;
       }
 
-      // Set up ATXP context, then store detected credential if present.
-      // requirePayment() will find it via getDetectedCredential() and settle
-      // before charging, using the pricing context it has (amount, options).
-      return withATXPContext(config, resource, tokenCheck, () => {
+      // Set up ATXP context, settle any payment credential, then run route.
+      // Settlement happens HERE (in middleware) rather than in requirePayment()
+      // so the ledger is credited before any route code runs. This avoids
+      // footguns where tool handlers call requirePayment() multiple times
+      // (e.g., pre-flight balance check + post-generation charge) and the
+      // first call consumes the credential, leaving nothing for the second.
+      return withATXPContext(config, resource, tokenCheck, async () => {
         if (detected) {
           // Resolve identity for the settlement ledger credit.
-          // The settle must use the same sourceAccountId as the charge
-          // (atxpAccountId() = OAuth sub) so the ledger entries match.
-          // For MPP: prefer the OAuth user (recovered from opaque) over the
-          // wallet address from the credential's `source` field — the ledger
-          // is keyed by OAuth identity, not wallet address.
-          // For ATXP: use the sourceAccountId embedded in the credential.
-          // For X402: falls back to OAuth sub (credential has no identity).
           const sourceAccountId = (detected.protocol === 'mpp' && user)
             ? user
             : resolveIdentitySync(config, req, detected.protocol, detected.credential) || user || undefined;
-          setDetectedCredential({
-            protocol: detected.protocol,
-            credential: detected.credential,
-            sourceAccountId,
-          });
-          logger.info(`Stored ${detected.protocol} credential in context for requirePayment (sourceAccountId=${sourceAccountId})`);
+
+          // Settle the credential immediately — credits the auth server's
+          // ledger so subsequent charge() calls in requirePayment() succeed.
+          const destinationAccountId = await config.destination.getAccountId();
+          const settlement = new ProtocolSettlement(
+            config.server,
+            logger,
+            fetch.bind(globalThis),
+            destinationAccountId,
+          );
+
+          // For X402: the credential's parsed payload contains `accepted` — the
+          // exact payment requirement the client signed off on. Pass it directly
+          // as paymentRequirements instead of regenerating from server config.
+          // For MPP/ATXP: credentials are self-contained, no extra context needed.
+          const context: Record<string, unknown> = {
+            ...(sourceAccountId && { sourceAccountId }),
+            destinationAccountId,
+          };
+
+          if (detected.protocol === 'x402') {
+            const parsed = parseCredentialBase64(detected.credential);
+            if (parsed?.accepted) {
+              context.paymentRequirements = parsed.accepted;
+            }
+          }
+
+          try {
+            const result = await settlement.settle(
+              detected.protocol,
+              detected.credential,
+              context as Parameters<typeof settlement.settle>[2],
+            );
+            logger.info(`Settled ${detected.protocol} in middleware: txHash=${result.txHash}, amount=${result.settledAmount}`);
+          } catch (error) {
+            logger.error(`Middleware settlement failed for ${detected.protocol}: ${error instanceof Error ? error.message : String(error)}`);
+            // Don't store the credential — it's already consumed/invalid.
+            // requirePayment() will see no credential, charge will fail,
+            // and a fresh payment challenge will be issued.
+          }
         }
 
         // Intercept the response to rewrite McpServer's wrapped payment errors
         // back into proper JSON-RPC errors with full challenge data.
-        // McpServer catches McpError(-30402) and wraps it into a CallToolResult,
-        // discarding error.data (which contains x402/mpp challenge data).
-        // We detect the wrapped error and reconstruct the JSON-RPC error using
-        // challenge data stored in AsyncLocalStorage by buildOmniError.
         installPaymentResponseRewriter(res, logger);
 
         return next();
@@ -197,11 +223,11 @@
   res.writeHead = function writeHeadDeferred(this: Response, ...args: any[]): any {
     deferredWriteHead = args;
     return this;
   } as any;
 
   function flushWriteHead(self: Response): void {
     if (!deferredWriteHead) return;
     (origWriteHead as any).apply(self, deferredWriteHead);
     deferredWriteHead = null;
   }
 
@@ -210,8 +236,8 @@
   res.write = function writeWithPaymentRewrite(this: Response, ...args: any[]): any {
     flushWriteHead(this);
     args[0] = rewriteChunk(args[0]);
     return (origWrite as any).apply(this, args);
   } as any;
 
   // Hook res.end for non-SSE (enableJsonResponse) responses.
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -239,12 +265,12 @@
           }
         }
       }
       (origWriteHead as any).apply(this, deferredWriteHead);
       deferredWriteHead = null;
     }
 
     return (origEnd as any).apply(this, args);
   } as any;
 }
 
 /**

packages/atxp-express/src/omniChallenge.test.ts

@@ -268,10 +268,12 @@ describe('credential detection Express middleware', () => {
       expect(storedCredential).toBeNull();
     });
 
-    it('should store ATXP credential with sourceAccountId from raw JSON', async () => {
+    // Settlement now happens in the middleware, not in requirePayment().
+    // The credential is NOT stored — it's settled immediately, so
+    // getDetectedCredential() returns null in route handlers.
+    it('should settle ATXP credential in middleware (not store for later)', async () => {
       let storedCredential: DetectedCredential | null = null;
 
-      // Raw JSON (not base64-encoded)
       const atxpCredential = JSON.stringify({
         sourceAccountId: 'atxp_acct_raw123',
         sourceAccountToken: 'tok_raw',
@@ -297,12 +299,11 @@ describe('credential detection Express middleware', () => {
         .send(TH.mcpToolRequest());
 
       expect(response.status).toBe(200);
-      expect(storedCredential).not.toBeNull();
-      expect(storedCredential!.protocol).toBe('atxp');
-      expect(storedCredential!.sourceAccountId).toBe('atxp_acct_raw123');
+      // Credential was settled in middleware, not stored for requirePayment()
+      expect(storedCredential).toBeNull();
     });
 
-    it('should store X402 credential with sourceAccountId from OAuth sub (fallback)', async () => {
+    it('should settle X402 credential in middleware (not store for later)', async () => {
       let storedCredential: DetectedCredential | null = null;
 
       const router = atxpExpress(TH.config({
@@ -325,18 +326,12 @@ describe('credential detection Express middleware', () => {
         .send(TH.mcpToolRequest());
 
       expect(response.status).toBe(200);
-      expect(storedCredential).not.toBeNull();
-      expect(storedCredential!.protocol).toBe('x402');
-      // X402 credentials don't contain identity, so sourceAccountId falls back
-      // to the OAuth sub. This ensures the settle credits the same account that
-      // the charge deducts from (atxpAccountId() = OAuth sub).
-      expect(storedCredential!.sourceAccountId).toBe('atxp:atxp_acct_x402user');
+      expect(storedCredential).toBeNull();
     });
 
-    it('should store ATXP credential with sourceAccountId from base64-encoded JSON', async () => {
+    it('should settle base64-encoded ATXP credential in middleware (not store for later)', async () => {
       let storedCredential: DetectedCredential | null = null;
 
-      // Base64-encoded JSON
       const atxpCredential = Buffer.from(JSON.stringify({
         sourceAccountId: 'atxp_acct_b64_456',
         sourceAccountToken: 'tok_b64',
@@ -362,9 +357,7 @@ describe('credential detection Express middleware', () => {
         .send(TH.mcpToolRequest());
 
       expect(response.status).toBe(200);
-      expect(storedCredential).not.toBeNull();
-      expect(storedCredential!.protocol).toBe('atxp');
-      expect(storedCredential!.sourceAccountId).toBe('atxp_acct_b64_456');
+      expect(storedCredential).toBeNull();
     });
   });
 });

packages/atxp-server/src/requirePayment.test.ts

@@ -2,9 +2,8 @@ import { describe, it, expect, vi } from 'vitest';
 import { requirePayment } from './index.js';
 import * as TH from './serverTestHelpers.js';
 import { BigNumber } from 'bignumber.js';
-import { withATXPContext, setDetectedCredential } from './atxpContext.js';
+import { withATXPContext } from './atxpContext.js';
 import { PAYMENT_REQUIRED_ERROR_CODE } from '@atxp/common';
-import { McpError } from '@modelcontextprotocol/sdk/types.js';
 import { ProtocolSettlement } from './protocol.js';
 
 describe('requirePayment', () => {
@@ -341,62 +340,17 @@ describe('requirePayment', () => {
     });
   });
 
-  describe('settlement of detected credentials', () => {
-    it('should settle credential and succeed when charge passes after settlement', async () => {
-      const mockSettle = vi.fn().mockResolvedValue({ txHash: '0xabc', settledAmount: '10000' });
-      vi.spyOn(ProtocolSettlement.prototype, 'settle').mockImplementation(mockSettle);
-
-      const paymentServer = TH.paymentServer({ charge: vi.fn().mockResolvedValue(true) });
-      const config = TH.config({ paymentServer });
-
-      await withATXPContext(config, new URL('https://example.com'), TH.tokenCheck(), async () => {
-        setDetectedCredential({ protocol: 'x402', credential: 'dGVzdA==', sourceAccountId: 'base:0x123' });
-        await expect(requirePayment({ price: BigNumber(0.01) })).resolves.not.toThrow();
-        expect(mockSettle).toHaveBeenCalledWith('x402', 'dGVzdA==', expect.objectContaining({
-          destinationAccountId: `base:${TH.DESTINATION}`,
-        }));
-        expect(paymentServer.charge).toHaveBeenCalled();
-      });
-
-      vi.restoreAllMocks();
-    });
-
-    it('should throw McpError when settlement fails instead of falling through silently', async () => {
-      const mockSettle = vi.fn().mockRejectedValue(new Error('on-chain tx reverted'));
-      vi.spyOn(ProtocolSettlement.prototype, 'settle').mockImplementation(mockSettle);
-
-      const paymentServer = TH.paymentServer({ charge: vi.fn().mockResolvedValue(false) });
-      const config = TH.config({ paymentServer });
-
-      await withATXPContext(config, new URL('https://example.com'), TH.tokenCheck(), async () => {
-        setDetectedCredential({ protocol: 'x402', credential: 'dGVzdA==', sourceAccountId: 'base:0x123' });
-        try {
-          await requirePayment({ price: BigNumber(0.01) });
-          expect.fail('should have thrown');
-        } catch (err: unknown) {
-          // Should be an McpError with settlement failure details, NOT an omni-challenge
-          expect(err).toBeInstanceOf(McpError);
-          const mcpErr = err as McpError;
-          expect(mcpErr.code).toBe(-32000);
-          expect(mcpErr.message).toContain('Payment settlement failed for x402');
-          expect((mcpErr.data as Record<string, unknown>)?.reason).toBe('on-chain tx reverted');
-          // charge() should NOT have been called — we threw before reaching it
-          expect(paymentServer.charge).not.toHaveBeenCalled();
-        }
-      });
-
-      vi.restoreAllMocks();
-    });
-
-    it('should skip settlement and proceed to normal charge when no credential detected', async () => {
+  // Settlement is now handled by the middleware (atxpExpress), not requirePayment().
+  // See atxpExpress.test.ts for settlement tests.
+  describe('requirePayment does not settle (settlement moved to middleware)', () => {
+    it('should charge directly without settling — middleware handles settlement before route code runs', async () => {
       const mockSettle = vi.fn();
       vi.spyOn(ProtocolSettlement.prototype, 'settle').mockImplementation(mockSettle);
 
       const paymentServer = TH.paymentServer({ charge: vi.fn().mockResolvedValue(true) });
       const config = TH.config({ paymentServer });
 
       await withATXPContext(config, new URL('https://example.com'), TH.tokenCheck(), async () => {
-        // No setDetectedCredential call — no credential on this request
         await expect(requirePayment({ price: BigNumber(0.01) })).resolves.not.toThrow();
         expect(mockSettle).not.toHaveBeenCalled();
         expect(paymentServer.charge).toHaveBeenCalled();

packages/atxp-server/src/requirePayment.ts

@@ -1,10 +1,8 @@
 import { RequirePaymentConfig, extractNetworkFromAccountId, extractAddressFromAccountId, Network, AuthorizationServerUrl } from "@atxp/common";
-import { McpError } from "@modelcontextprotocol/sdk/types.js";
 import { BigNumber } from "bignumber.js";
-import { getATXPConfig, atxpAccountId, atxpToken, getDetectedCredential, setPendingPaymentChallenge } from "./atxpContext.js";
+import { getATXPConfig, atxpAccountId, atxpToken, setPendingPaymentChallenge } from "./atxpContext.js";
 import { buildPaymentOptions, omniChallengeMcpError } from "./omniChallenge.js";
 import { getATXPResource } from "./atxpContext.js";
-import { ProtocolSettlement, type SettlementContext } from "./protocol.js";
 import { signOpaqueIdentity } from "./opaqueIdentity.js";
 
 export async function requirePayment(paymentConfig: RequirePaymentConfig): Promise<void> {
@@ -45,14 +43,8 @@ export async function requirePayment(paymentConfig: RequirePaymentConfig): Promi
     ...(token && { sourceAccountToken: token }),
   };
 
-  // If a payment credential was detected on this request (retry after challenge),
-  // settle it now. We have the full pricing context to generate requirements.
-  const detectedCredential = getDetectedCredential();
-  if (detectedCredential) {
-    await settleDetectedCredential(config, detectedCredential, charge, destinationAccountId, paymentAmount);
-    // After settlement, the ledger should be credited. Fall through to charge below.
-  }
-
+  // Settlement is handled by the middleware (atxpExpress) before route code runs.
+  // The ledger is already credited by the time we get here on a retry request.
   config.logger.debug(`Charging ${paymentConfig.price} to ${charge.options.length} options for source ${user}`);
 
   const chargeSucceeded = await config.paymentServer.charge(charge);
@@ -121,78 +113,6 @@ async function fetchAllSources(
   return sources;
 }
 
-/**
- * Settle a payment credential that was detected on this retry request.
- *
- * This runs inside requirePayment because it has the pricing context needed
- * to generate protocol-specific settlement data:
- * - X402: regenerates paymentRequirements from charge options (same as the challenge)
- * - ATXP: passes sourceAccountToken and payment options
- * - MPP: passes credential directly (self-contained)
- *
- * After settlement, the auth service credits the local ledger, so the
- * subsequent charge() call will succeed.
- */
-async function settleDetectedCredential(
-  config: NonNullable<ReturnType<typeof getATXPConfig>>,
-  detected: NonNullable<ReturnType<typeof getDetectedCredential>>,
-  charge: { options: Array<{ network: string; currency: string; address: string; amount: BigNumber }>; sourceAccountId: string; destinationAccountId: string },
-  destinationAccountId: string,
-  paymentAmount: BigNumber,
-): Promise<void> {
-  const { protocol, credential, sourceAccountId } = detected;
-  config.logger.info(`Settling ${protocol} credential in requirePayment (has pricing context)`);
-
-  // ProtocolSettlement is instantiated per-request. This is intentional — the class
-  // is lightweight (stores config references only, no connections or heavy init).
-  // Caching would require threading persistent state through requirePayment's
-  // stateless call chain, for negligible benefit.
-  const settlement = new ProtocolSettlement(
-    config.server,
-    config.logger,
-    fetch.bind(globalThis),
-    destinationAccountId,
-  );
-
-  // Build settlement context with identity and protocol-specific data
-  const context: SettlementContext = {
-    ...(sourceAccountId && { sourceAccountId }),
-    destinationAccountId,
-    options: charge.options,
-  };
-
-  // For X402, regenerate the paymentRequirements from the destination's
-  // real chain addresses (not the ATXP account ID). This is the standard X402
-  // pattern — the server generates requirements from its own config.
-  if (protocol === 'x402') {
-    const resource = getATXPResource()?.toString() ?? '';
-    let sources: Array<{ chain: string; address: string }> = charge.options.map(o => ({ chain: o.network, address: o.address }));
-    try {
-      const fetchedSources = await config.destination.getSources();
-      sources = fetchedSources.map(s => ({ chain: s.chain, address: s.address }));
-    } catch (err) {
-      config.logger.warn(`Failed to fetch destination sources for X402 settle: ${err}`);
-    }
-    const payment = buildPaymentOptions({ amount: paymentAmount, sources, resource, payeeName: config.payeeName });
-    if (payment.x402.accepts.length === 0) {
-      config.logger.warn('X402 settle: no compatible payment options after filtering');
-    }
-    context.paymentRequirements = payment.x402;
-  }
-
-  try {
-    const result = await settlement.settle(protocol, credential, context);
-    config.logger.info(`${protocol} settlement succeeded: txHash=${result.txHash}, amount=${result.settledAmount}`);
-  } catch (error) {
-    // Settlement failed — the credential was invalid or the on-chain tx failed.
-    // Throw an explicit error so the client knows its credential was rejected,
-    // rather than silently falling through to charge (which would fail with a
-    // confusing insufficient_balance + new challenge).
-    const reason = error instanceof Error ? error.message : String(error);
-    config.logger.error(`${protocol} settlement failed: ${reason}`);
-    throw new McpError(-32000, `Payment settlement failed for ${protocol}`, { protocol, reason });
-  }
-}
 
 /**
  * Build an omni-challenge MCP error that includes ATXP-MCP + X402 + MPP data.