From d25e7172598cf8917744eb60a4677bba95e864e5 Mon Sep 17 00:00:00 2001 From: Johnson Oyemade Date: Sun, 30 Aug 2026 17:17:35 +0100 Subject: [PATCH 1/2] fix(tokens): distinguish RPC failure from insufficient allowance in checkAllowance checkAllowance previously swallowed getAllowance RPC/network errors and returned { hasAllowance: false }. The create-stream flow keys off hasAllowance to decide whether to prompt an SEP-41 approve(), so a transient hiccup incorrectly triggered an extra signed transaction and fee even when allowance was already sufficient. Return { hasAllowance: undefined, error } on failure so callers can distinguish 'checked, insufficient' (hasAllowance === false) from 'couldn't check' (hasAllowance === undefined) and retry the read instead of defaulting to needs-approval. Update AllowanceResult docs accordingly. --- lib/tokens.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/tokens.ts b/lib/tokens.ts index c33baa2..3d6d690 100644 --- a/lib/tokens.ts +++ b/lib/tokens.ts @@ -92,7 +92,16 @@ import { } from './token-allowance-gateway'; export interface AllowanceResult { - hasAllowance: boolean; + /** + * Whether the spender has sufficient allowance. + * - `true` → allowance >= requiredAmount (checked, sufficient) + * - `false` → allowance < requiredAmount (checked, insufficient — needs approve) + * - `undefined` → allowance could not be checked (e.g. transient RPC failure). + * Callers MUST check `error` first and retry the read rather than treating + * `undefined` as "needs approval", otherwise a hiccup triggers an + * unnecessary SEP-41 approve() transaction and fee. + */ + hasAllowance: boolean | undefined; currentAllowance: bigint; error?: string; } @@ -139,6 +148,12 @@ export async function getAllowance( /** * Check whether the source address has sufficient token allowance for spender. * Throws an explicit error on missing arguments (#348), returns structured status on check completion. + * + * IMPORTANT: On a successful RPC read, `hasAllowance` is `true`|`false` and + * `error` is absent. On a transient RPC/network failure, `hasAllowance` is + * `undefined` and `error` is set — callers must surface the error and retry + * the read rather than treating it as "insufficient allowance" (which would + * trigger an unnecessary approve() transaction and fee). */ export async function checkAllowance( source: string, @@ -162,7 +177,7 @@ export async function checkAllowance( } catch (err: unknown) { const message = err instanceof Error ? err.message : 'Failed to check token allowance'; return { - hasAllowance: false, + hasAllowance: undefined, currentAllowance: 0n, error: message, }; From 51f1ad87041dd7048a2f33b5cd22f340498f1998 Mon Sep 17 00:00:00 2001 From: Johnson Oyemade Date: Sun, 30 Aug 2026 18:15:22 +0100 Subject: [PATCH 2/2] fix(soroban,tests): restore invokeContract returnValue plumbing and isolate test state soroban.ts was broken since #379: duplicated poll logic with a stray status check left the file syntactically invalid and pollForConfirmation still returned string instead of InvokeContractResult. Make poll return { hash, returnValue } on SUCCESS and { hash } on pending timeout so create_stream's stream_id surfaces correctly, and clean up the stray block. Also clear the inclusion-fee cache on resetServer / via __clearFeeStatsCache so fee-stats tests don't leak cached p70 across cases. Tests: update soroban-pipeline mocks to expect object shape, fix tokens.test mock type, add checkAllowance RPC-failure distinguishing test, fix stream.test missing-field order, and mock checkRecipientExists in create/page tests with debounce-aware waits. --- app/create/__tests__/page.test.tsx | 16 ++++++++++++++ lib/soroban-pipeline.test.ts | 10 ++++++--- lib/soroban.ts | 35 +++++++++++------------------- lib/stream.test.ts | 4 +++- lib/tokens.test.ts | 17 +++++++++++++-- 5 files changed, 54 insertions(+), 28 deletions(-) diff --git a/app/create/__tests__/page.test.tsx b/app/create/__tests__/page.test.tsx index 01f7aa3..307c211 100644 --- a/app/create/__tests__/page.test.tsx +++ b/app/create/__tests__/page.test.tsx @@ -51,6 +51,15 @@ vi.mock('@/lib/token-allowance-gateway', () => ({ }), })); +const mockCheckRecipientExists = vi.fn().mockResolvedValue(true); +vi.mock('@/lib/soroban', async () => { + const actual = await vi.importActual('@/lib/soroban'); + return { + ...actual, + checkRecipientExists: (...args: unknown[]) => mockCheckRecipientExists(...args), + }; +}); + vi.mock('lucide-react', () => ({ ArrowRight: () => React.createElement('span', null, '→'), Info: () => React.createElement('span', null, 'i'), @@ -96,6 +105,11 @@ async function fillRecipient(container: HTMLElement) { await act(async () => { setFieldValue(recipientInput, TEST_RECIPIENT); }); + // Recipient existence check is debounced 600ms + RPC; wait for it to settle + // so the form isn't blocked by `recipientStatus === 'checking'`. + await act(async () => { + await new Promise((r) => setTimeout(r, 700)); + }); } async function fillDeposit(container: HTMLElement, amount: string) { @@ -110,6 +124,7 @@ async function fillDeposit(container: HTMLElement, amount: string) { describe('CreatePage — zero-rate guard (issue #243)', () => { beforeEach(() => { vi.clearAllMocks(); + mockCheckRecipientExists.mockResolvedValue(true); mockCreateStream.mockResolvedValue({ hash: 'tx_hash_abc', streamId: 7n }); mockRefreshStreamData.mockResolvedValue(undefined); }); @@ -179,6 +194,7 @@ describe('CreatePage — SEP-41 allowance check before deposit (issue #218)', () beforeEach(() => { vi.clearAllMocks(); + mockCheckRecipientExists.mockResolvedValue(true); mockIsMock.mockReturnValue(false); mockCreateStream.mockResolvedValue({ hash: 'tx_hash_abc', streamId: 7n }); mockRefreshStreamData.mockResolvedValue(undefined); diff --git a/lib/soroban-pipeline.test.ts b/lib/soroban-pipeline.test.ts index bc86024..1dead0d 100644 --- a/lib/soroban-pipeline.test.ts +++ b/lib/soroban-pipeline.test.ts @@ -82,7 +82,7 @@ function simError(message: string) { return { error: message }; } -beforeEach(() => { +beforeEach(async () => { vi.useFakeTimers(); mockGetAccount.mockReset().mockResolvedValue({ accountId: () => SOURCE, sequenceNumber: () => '1' }); mockSimulate.mockReset(); @@ -93,6 +93,10 @@ beforeEach(() => { mockAssemble.mockReset().mockReturnValue({ build: () => ({ toEnvelope: () => ({ toXDR: () => 'assembled-envelope-b64' }) }), }); + // Clear the inclusion-fee cache so the "falls back to BASE_FEE" test + // doesn't see a cached p70 from the previous test's successful fetch. + const { __clearFeeStatsCache } = await import('./soroban.js'); + __clearFeeStatsCache(); }); afterEach(() => { @@ -212,7 +216,7 @@ describe('invokeContract', () => { promise.catch(() => {}); await vi.advanceTimersByTimeAsync(2000); - expect(await promise).toBe('deadbeef'); + expect(await promise).toEqual(expect.objectContaining({ hash: 'deadbeef' })); expect(signTx).toHaveBeenCalledTimes(1); expect(mockSend).toHaveBeenCalledTimes(1); }); @@ -227,7 +231,7 @@ describe('invokeContract', () => { promise.catch(() => {}); await vi.advanceTimersByTimeAsync(31_000); - expect(await promise).toBe('deadbeef'); + expect(await promise).toEqual(expect.objectContaining({ hash: 'deadbeef' })); expect(signTx).toHaveBeenCalledTimes(1); expect(mockSend).toHaveBeenCalledTimes(1); }); diff --git a/lib/soroban.ts b/lib/soroban.ts index 0f141ab..bfe74d6 100644 --- a/lib/soroban.ts +++ b/lib/soroban.ts @@ -121,6 +121,12 @@ function getServer(): SorobanRpc.Server { */ export function resetServer(): void { serverInstance = undefined; + feeStatsCache = undefined; +} + +/** Test-only: clear the inclusion-fee cache so fee-stats tests are isolated. */ +export function __clearFeeStatsCache(): void { + feeStatsCache = undefined; } // ── Retry / Backoff ─────────────────────────────────────────────────────────── @@ -426,25 +432,6 @@ export async function invokeContract( throw new Error('Submission returned no transaction hash'); } - if (status.status === SorobanRpc.Api.GetTransactionStatus.SUCCESS) { - // #362 — surface the confirmed transaction's return value instead - // of discarding it. Contract functions like DripFactory::create_stream - // return data (the assigned stream_id) that callers otherwise have - // no way to obtain without a separate re-query. - return { hash, returnValue: status.returnValue }; - } - if (status.status === SorobanRpc.Api.GetTransactionStatus.FAILED) { - // Non-retryable — transaction executed and failed on-chain - recordFailure(); - throw new Error(`Transaction failed: ${hash}`); - } - // status === 'NOT_FOUND' — keep polling - } - throw new Error(`Transaction timed out after ${MAX_POLL_ATTEMPTS}s: ${hash}`); - }, { - context: `invokeContract(${method})`, - signal, - }); return pollForConfirmation(hash, timeoutMs, signal); }; @@ -461,12 +448,16 @@ export async function invokeContract( * reach a verdict (transient RPC errors, or the confirmation window elapsing) * the hash is returned as *pending* rather than replaying the transaction: * it is already on-chain and the caller can look it up (see #358). + * + * On SUCCESS the confirmed transaction's return value is surfaced so callers + * like DripFactory::create_stream can obtain the assigned stream_id without + * a separate re-query (see #362). */ async function pollForConfirmation( hash: string, timeoutMs: number, signal?: AbortSignal, -): Promise { +): Promise { for (let i = 0; i < MAX_POLL_ATTEMPTS; i++) { if (signal?.aborted) throw new OperationAbortedError(); @@ -492,7 +483,7 @@ async function pollForConfirmation( if (status.status === SorobanRpc.Api.GetTransactionStatus.SUCCESS) { // The RPC round-trip completed — the network is healthy. resetCircuitBreaker(); - return hash; + return { hash, returnValue: status.returnValue }; } if (status.status === SorobanRpc.Api.GetTransactionStatus.FAILED) { // The transaction executed and the contract reverted. The RPC worked @@ -504,7 +495,7 @@ async function pollForConfirmation( } // Submitted but unconfirmed within the window — pending, not failed. - return hash; + return { hash }; } /** diff --git a/lib/stream.test.ts b/lib/stream.test.ts index 90f765e..bd18766 100644 --- a/lib/stream.test.ts +++ b/lib/stream.test.ts @@ -116,7 +116,9 @@ describe('getStreamInfo', () => { it('throws a clear error when a field is missing rather than silently defaulting', async () => { mockSimulateReadOnly.mockResolvedValue(scvMap({ sender: new Address(SENDER).toScVal(), - // recipient deliberately omitted + // recipient deliberately omitted — flags included so the missing-recipient + // path is exercised first (flags is now read before sender/recipient). + flags: xdr.ScVal.scvU32(0), })); const { getStreamInfo } = await import('./stream.js'); await expect(getStreamInfo(SENDER, STREAM_ADDRESS)).rejects.toThrow(/Missing field: recipient/); diff --git a/lib/tokens.test.ts b/lib/tokens.test.ts index e23799e..3026357 100644 --- a/lib/tokens.test.ts +++ b/lib/tokens.test.ts @@ -102,8 +102,21 @@ describe('SEP-41 Token Allowance Helpers (#347, #348)', () => { expect(result.currentAllowance).toBe(1000n); }); + it('checkAllowance distinguishes RPC failure from insufficient allowance (hasAllowance undefined, not false)', async () => { + vi.mocked(soroban.simulateReadOnly).mockRejectedValueOnce(new Error('Network request timed out')); + const result = await checkAllowance(VALID_SOURCE, VALID_TOKEN, VALID_SPENDER, 500n); + // Must NOT be `false` — false means "checked, insufficient" and would + // trigger an unnecessary approve() and fee. Undefined means "couldn't check". + expect(result.hasAllowance).toBeUndefined(); + expect(result.currentAllowance).toBe(0n); + expect(result.error).toMatch(/Network request timed out/); + // Callers must check error first; a falsy check `!hasAllowance` would + // still be true for undefined, so the correct guard is `hasAllowance === false`. + expect(result.hasAllowance === false).toBe(false); + }); + it('approveAllowance calls SAC with 4 arguments and succeeds (#347)', async () => { - vi.mocked(soroban.invokeContract).mockResolvedValueOnce('tx_hash_123'); + vi.mocked(soroban.invokeContract).mockResolvedValueOnce({ hash: 'tx_hash_123' } as any); const mockSignTx = vi.fn().mockResolvedValue('signed'); const result = await approveAllowance( @@ -127,7 +140,7 @@ describe('SEP-41 Token Allowance Helpers (#347, #348)', () => { ); // Verify 4 arguments were passed to invokeContract - const passedArgs = vi.mocked(soroban.invokeContract).mock.calls[0][3]; + const passedArgs = vi.mocked(soroban.invokeContract).mock.calls[0]![3] as unknown[]; expect(passedArgs).toHaveLength(4); }); });