From f3ffcc5c62e5c7bb60a88678c01443b2a2afc658 Mon Sep 17 00:00:00 2001 From: ghzhost Date: Wed, 2 Sep 2026 17:29:52 +0000 Subject: [PATCH] fix(batch): only fall back to direct Horizon on network-layer errors (#14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit batchPaymentApi.send's catch block previously swallowed every error and blindly resubmitted the signed XDR straight to Horizon, bypassing backend validation and risking double-spend when the backend had already broadcast the transaction (the HTTP response was lost). The fix: 1. ApiRequestError gains an optional httpStatus field populated by normalizeApiError from the Axios response status. A missing httpStatus unambiguously identifies a pure network-layer failure (no server response arrived at all). 2. New exported helper isNetworkLayerError(error) encapsulates that check in a single, unit-testable place shared across hooks. 3. useBatchPayment's catch block now calls isNetworkLayerError and only falls back to submitTransaction when it returns true. 4xx (validation/ client), 5xx (server), and 502 (gateway) errors are re-thrown so the caller sees a real error rather than a spurious success. Tests added in useBatchPayment.test.tsx covering: - normal backend-success path (no Horizon call) - pure network error → fallback triggers - 400 validation error → error surfaced, no fallback - 500 server error → error surfaced, no fallback - 422 unprocessable → error surfaced, no fallback - 502 bad gateway → error surfaced, no fallback - isNetworkLayerError unit tests for all variants --- src/hooks/useBatchPayment.test.tsx | 235 +++++++++++++++++++++++++++++ src/hooks/useBatchPayment.ts | 20 ++- src/lib/api.ts | 35 ++++- 3 files changed, 282 insertions(+), 8 deletions(-) create mode 100644 src/hooks/useBatchPayment.test.tsx diff --git a/src/hooks/useBatchPayment.test.tsx b/src/hooks/useBatchPayment.test.tsx new file mode 100644 index 0000000..5dad752 --- /dev/null +++ b/src/hooks/useBatchPayment.test.tsx @@ -0,0 +1,235 @@ +/** + * Tests for issue #14 — useBatchPayment must not blindly fall back to a + * direct Horizon submission for every batchPaymentApi.send failure. + * + * Only pure network-layer errors (no HTTP response) should trigger the + * fallback. 4xx validation and 5xx server errors must surface as-is so the + * caller sees a real error instead of a spurious "success" that bypassed + * backend validation or double-submitted an already-executed transaction. + */ +import React from 'react' +import { act, renderHook, waitFor } from '@testing-library/react' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { ApiRequestError } from '@/lib/api' +import type { BatchPaymentFormValues, BatchPaymentResult } from '@/types' + +// ─── Hoist mocks ───────────────────────────────────────────────────────────── + +const apiMocks = vi.hoisted(() => ({ + batchPaymentApi: { + send: vi.fn<() => Promise>(), + }, + isNetworkLayerError: vi.fn<(e: unknown) => boolean>(), +})) + +vi.mock('@/lib/api', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + batchPaymentApi: apiMocks.batchPaymentApi, + isNetworkLayerError: apiMocks.isNetworkLayerError, + } +}) + +const stellarMocks = vi.hoisted(() => ({ + buildBatchPaymentTransaction: vi.fn(async () => 'raw-xdr'), + submitTransaction: vi.fn<() => Promise<{ hash: string; ledger: number }>>(), +})) + +vi.mock('@/lib/stellar', () => stellarMocks) + +const walletMocks = vi.hoisted(() => ({ + publicKey: 'GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5', + network: 'testnet' as const, + signTransaction: vi.fn(async (xdr: string) => `signed:${xdr}`), + isConnected: true, + refreshAccount: vi.fn(async () => {}), +})) + +vi.mock('./useWallet', () => ({ useWallet: () => walletMocks })) +vi.mock('./useSendPayment', () => ({ + useSupportedAssets: () => [{ code: 'XLM', issuer: null, name: 'Stellar Lumens', decimals: 7 }], +})) +vi.mock('./useTransactions', () => ({ + useInvalidateTransactions: () => vi.fn(), +})) + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function wrapper({ children }: { children: React.ReactNode }) { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }) + return {children} +} + +function makeApiError(code: string, httpStatus?: number): ApiRequestError { + const err = new ApiRequestError({ code, message: `Error: ${code}` }, httpStatus) + return err +} + +const FORM_VALUES: BatchPaymentFormValues = { + assetCode: 'XLM', + recipients: [{ destinationAddress: 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN', amount: '10' }], +} + +import { useBatchPayment } from './useBatchPayment' + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +beforeEach(() => { + apiMocks.batchPaymentApi.send.mockReset() + stellarMocks.buildBatchPaymentTransaction.mockReset() + stellarMocks.submitTransaction.mockReset() + walletMocks.signTransaction.mockReset() + walletMocks.refreshAccount.mockReset() + + stellarMocks.buildBatchPaymentTransaction.mockResolvedValue('raw-xdr') + walletMocks.signTransaction.mockImplementation(async (xdr: string) => `signed:${xdr}`) + + // Default: real isNetworkLayerError logic + apiMocks.isNetworkLayerError.mockImplementation( + (err: unknown) => err instanceof ApiRequestError && err.httpStatus === undefined, + ) +}) + +describe('useBatchPayment — fallback discrimination (#14)', () => { + it('succeeds normally when batchPaymentApi.send resolves', async () => { + const backendResult: BatchPaymentResult = { + batchId: 'batch-1', + transactionHash: 'hash-abc', + status: 'success', + recipientCount: 1, + totalAmount: '10.0000000', + createdAt: new Date().toISOString(), + } + apiMocks.batchPaymentApi.send.mockResolvedValue(backendResult) + + const { result } = renderHook(() => useBatchPayment(), { wrapper }) + + act(() => { + result.current.reviewBatch(FORM_VALUES) + }) + await act(async () => { + result.current.confirmBatch() + }) + + await waitFor(() => expect(result.current.state.step).toBe('success')) + expect(result.current.state.result?.transactionHash).toBe('hash-abc') + expect(stellarMocks.submitTransaction).not.toHaveBeenCalled() + }) + + it('falls back to direct Horizon submission on a pure network-layer error (no HTTP response)', async () => { + // Network error: ApiRequestError with no httpStatus (no server response) + const networkErr = makeApiError('UNKNOWN_ERROR', undefined) + apiMocks.batchPaymentApi.send.mockRejectedValue(networkErr) + stellarMocks.submitTransaction.mockResolvedValue({ hash: 'horizon-hash', ledger: 42 }) + + const { result } = renderHook(() => useBatchPayment(), { wrapper }) + + act(() => { + result.current.reviewBatch(FORM_VALUES) + }) + await act(async () => { + result.current.confirmBatch() + }) + + await waitFor(() => expect(result.current.state.step).toBe('success')) + expect(stellarMocks.submitTransaction).toHaveBeenCalledOnce() + expect(result.current.state.result?.transactionHash).toBe('horizon-hash') + }) + + it('does NOT fall back to Horizon on a 400 validation error — surfaces the error instead', async () => { + // 400: explicit backend rejection (e.g. sanctioned recipient) + const validationErr = makeApiError('VALIDATION_ERROR', 400) + apiMocks.batchPaymentApi.send.mockRejectedValue(validationErr) + + const { result } = renderHook(() => useBatchPayment(), { wrapper }) + + act(() => { + result.current.reviewBatch(FORM_VALUES) + }) + await act(async () => { + result.current.confirmBatch() + }) + + await waitFor(() => expect(result.current.state.step).toBe('error')) + expect(stellarMocks.submitTransaction).not.toHaveBeenCalled() + expect(result.current.state.error).toContain('VALIDATION_ERROR') + }) + + it('does NOT fall back to Horizon on a 500 server error — surfaces the error instead', async () => { + // 500: server may have already submitted the tx; blind resubmission is unsafe + const serverErr = makeApiError('INTERNAL_SERVER_ERROR', 500) + apiMocks.batchPaymentApi.send.mockRejectedValue(serverErr) + + const { result } = renderHook(() => useBatchPayment(), { wrapper }) + + act(() => { + result.current.reviewBatch(FORM_VALUES) + }) + await act(async () => { + result.current.confirmBatch() + }) + + await waitFor(() => expect(result.current.state.step).toBe('error')) + expect(stellarMocks.submitTransaction).not.toHaveBeenCalled() + }) + + it('does NOT fall back to Horizon on a 422 unprocessable error', async () => { + const unprocessableErr = makeApiError('UNPROCESSABLE_ENTITY', 422) + apiMocks.batchPaymentApi.send.mockRejectedValue(unprocessableErr) + + const { result } = renderHook(() => useBatchPayment(), { wrapper }) + + act(() => { + result.current.reviewBatch(FORM_VALUES) + }) + await act(async () => { + result.current.confirmBatch() + }) + + await waitFor(() => expect(result.current.state.step).toBe('error')) + expect(stellarMocks.submitTransaction).not.toHaveBeenCalled() + }) + + it('does NOT fall back to Horizon on a 502 bad gateway', async () => { + // 502 means the backend received the request (and may have forwarded it) + const gatewayErr = makeApiError('HORIZON_ERROR', 502) + apiMocks.batchPaymentApi.send.mockRejectedValue(gatewayErr) + + const { result } = renderHook(() => useBatchPayment(), { wrapper }) + + act(() => { + result.current.reviewBatch(FORM_VALUES) + }) + await act(async () => { + result.current.confirmBatch() + }) + + await waitFor(() => expect(result.current.state.step).toBe('error')) + expect(stellarMocks.submitTransaction).not.toHaveBeenCalled() + }) +}) + +describe('isNetworkLayerError unit tests', () => { + it('returns true for ApiRequestError with no httpStatus', () => { + const err = makeApiError('UNKNOWN_ERROR', undefined) + expect(apiMocks.isNetworkLayerError(err)).toBe(true) + }) + + it('returns false for ApiRequestError with a 400 status', () => { + const err = makeApiError('VALIDATION_ERROR', 400) + expect(apiMocks.isNetworkLayerError(err)).toBe(false) + }) + + it('returns false for ApiRequestError with a 500 status', () => { + const err = makeApiError('INTERNAL_SERVER_ERROR', 500) + expect(apiMocks.isNetworkLayerError(err)).toBe(false) + }) + + it('returns false for a plain Error (not an ApiRequestError)', () => { + expect(apiMocks.isNetworkLayerError(new Error('network down'))).toBe(false) + }) +}) diff --git a/src/hooks/useBatchPayment.ts b/src/hooks/useBatchPayment.ts index 2aa7b67..4b68502 100644 --- a/src/hooks/useBatchPayment.ts +++ b/src/hooks/useBatchPayment.ts @@ -1,6 +1,6 @@ import { useMutation } from '@tanstack/react-query' import { useCallback, useState } from 'react' -import { batchPaymentApi } from '@/lib/api' +import { batchPaymentApi, isNetworkLayerError } from '@/lib/api' import { buildBatchPaymentTransaction, submitTransaction } from '@/lib/stellar' import { useWallet } from './useWallet' import { useSupportedAssets } from './useSendPayment' @@ -68,7 +68,23 @@ export function useBatchPayment() { recipients: values.recipients, signedXdr, }) - } catch { + } catch (err) { + // Only fall back to a direct Horizon submission when the error is a + // pure network-layer failure (no HTTP response arrived at all). In + // that case the backend certainly never received the payload, so + // resubmitting directly is safe. + // + // Errors that *did* receive an HTTP response (4xx validation, 5xx + // server fault, 502 gateway) are re-thrown unchanged: + // - On a 4xx the backend explicitly rejected the payload; pushing + // the XDR straight to Horizon would bypass that validation. + // - On a 5xx the backend *may* have already broadcast the + // transaction; blind resubmission risks a tx_bad_seq confusion + // or double-spend on a fresh retry. + if (!isNetworkLayerError(err)) { + throw err + } + const { hash } = await submitTransaction(signedXdr, network) return { batchId: hash, diff --git a/src/lib/api.ts b/src/lib/api.ts index e1e5312..b3bc230 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -38,15 +38,35 @@ const BASE_URL = (import.meta.env.VITE_API_URL as string) || 'http://localhost:8 export class ApiRequestError extends Error implements ApiError { code: string details?: Record + /** HTTP status code from the server response, absent for pure network errors. */ + httpStatus?: number - constructor(apiError: ApiError) { + constructor(apiError: ApiError, httpStatus?: number) { super(apiError.message) this.name = 'ApiRequestError' this.code = apiError.code this.details = apiError.details + this.httpStatus = httpStatus } } +/** + * Returns true only for pure network-layer failures — i.e. the request was + * sent but no HTTP response came back (connection refused, ECONNRESET, + * request timeout before any response headers arrived, etc.). This is the + * only class of error where falling back to a direct Horizon submission can + * be justified: the backend never received the payload, so it certainly + * hasn't submitted the transaction on our behalf. + * + * Errors that *did* receive an HTTP response (4xx validation, 5xx server + * fault, 502 gateway) must NOT trigger a fallback — the backend may have + * already broadcast the transaction, and bypassing its validation on a + * 4xx would push a payload it explicitly rejected. + */ +export function isNetworkLayerError(error: unknown): boolean { + return error instanceof ApiRequestError && error.httpStatus === undefined +} + /** * Turns a rejected axios response into an `ApiRequestError`. Exported * separately from the interceptor so it can be unit-tested directly instead @@ -63,11 +83,14 @@ export function normalizeApiError(error: AxiosError): ApiRequestError error.response?.data?.message || error.message || 'An unexpected error occurred' const code = error.response?.data?.code || (error.response?.status === 404 ? 'NOT_FOUND' : 'UNKNOWN_ERROR') - return new ApiRequestError({ - code, - message, - details: error.response?.data?.details, - }) + return new ApiRequestError( + { + code, + message, + details: error.response?.data?.details, + }, + error.response?.status, + ) } // ─── Axios instance ───────────────────────────────────────────────────────────