From 94148fb5d28b959405b37d205b783e2602f6beb5 Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Fri, 14 Aug 2026 04:58:38 +0100 Subject: [PATCH 1/2] fix(api): check confirmTransaction results in the seven routes that discarded them (GH#2517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Connection.confirmTransaction()` can RESOLVE with a `SignatureResult` whose `err` records an on-chain execution failure. Awaiting it proves the RPC call completed, not that the transaction succeeded — which is why the surrounding try/catch blocks were useful but insufficient: depending on which confirmation path wins, an execution failure may throw OR be returned as data. Seven call sites discarded that result and advanced success-only state anyway: devnet-airdrop:371 mirror-mint address upserted for a mint that was never created devnet-airdrop:760 24h claim stays reserved; response reports an amount devnet-mint-token:313 optional airdrop reported successful devnet-mint-token:402 nonexistent mint becomes the stored mapping and is returned as status: "created" devnet-mirror-mint:362 address persisted/returned for a failed creation playground/faucet:235 claim recorded, caller told 10,000 Sim-USDC arrived playground/faucet:271 SOL success advertised; RPC fallback loop stops The fix is to use what the repo already had. `assertSuccessfulConfirmation()` (lib/transaction-confirmation.ts) accepts only an explicit `value.err === null`, so it also fails closed on a malformed or incomplete result, and two routes were already using it correctly. No new abstraction, no new convention. Checked the four call sites the issue did NOT list, to see whether it undercounted: faucet:194, faucet:425 and auto-fund:213 already use the helper, and auto-fund:141 does the same check by hand. So all four were fine and the report's count of seven is exactly right. Left auto-fund:141's hand-rolled `airdropResult.value.err` check alone. It is correct, and converting a working site would be churn on a route this PR otherwise does not touch — noted rather than changed. The test is a source scan, not per-route cases, because the property is "no call site anywhere is bare" — a test per known route says nothing about the eighth one somebody adds later. It strips line comments first (this commit's own comments mention `confirmTransaction()` in prose), and asserts the scan still finds at least ten sites so it cannot start passing vacuously if a refactor moves the calls behind a wrapper. Mutation-tested: reverting devnet-mirror-mint to a bare call fails the guard, which names the offending `file:line` in the assertion message. Control passes either side. Verified: 305 files, 3073 passed, 17 skipped; tsc --noEmit exit 0. The four pre-existing transaction-confirmation unit tests still pass unchanged. Co-Authored-By: Claude Opus 5 --- .../api/confirm-transaction-checked.test.ts | 102 ++++++++++++++++++ app/app/api/devnet-airdrop/route.ts | 17 ++- app/app/api/devnet-mint-token/route.ts | 15 ++- app/app/api/devnet-mirror-mint/route.ts | 8 +- app/app/api/playground/faucet/route.ts | 20 +++- 5 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 app/__tests__/api/confirm-transaction-checked.test.ts diff --git a/app/__tests__/api/confirm-transaction-checked.test.ts b/app/__tests__/api/confirm-transaction-checked.test.ts new file mode 100644 index 000000000..fb7608c58 --- /dev/null +++ b/app/__tests__/api/confirm-transaction-checked.test.ts @@ -0,0 +1,102 @@ +/** + * GH#2517 — every `confirmTransaction()` in an API route must have its result + * checked. + * + * `Connection.confirmTransaction()` can RESOLVE with a `SignatureResult` whose + * `err` records an on-chain execution failure — awaiting it proves the RPC call + * completed, not that the transaction succeeded. Seven routes discarded that + * result and advanced success-only state anyway: persisting a mint address for + * a mint that was never created, consuming a 24-hour faucet claim, and telling + * callers that tokens were delivered. + * + * The repo already had the right answer — `assertSuccessfulConfirmation()`, + * which accepts only an explicit `value.err === null` and so also fails closed + * on a malformed or incomplete result. Two routes already used it. This is the + * guard that stops the next call site being written without it. + * + * Deliberately a source scan rather than per-route tests: the property is "no + * call site anywhere is bare", which is a statement about the whole surface. A + * test per known route would say nothing about the eighth one somebody adds. + */ + +import { describe, it, expect } from "vitest"; +import * as fs from "fs"; +import * as path from "path"; + +const API_DIR = path.resolve(__dirname, "../../app/api"); + +function routeFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...routeFiles(full)); + else if (entry.name === "route.ts") out.push(full); + } + return out; +} + +/** Strip line comments so prose mentioning confirmTransaction() isn't counted. */ +function stripComments(src: string): string { + return src + .split("\n") + .map((l) => l.replace(/\/\/.*$/, "")) + .join("\n"); +} + +interface Site { + file: string; + line: number; + checked: boolean; +} + +function callSites(): Site[] { + const sites: Site[] = []; + for (const file of routeFiles(API_DIR)) { + const code = stripComments(fs.readFileSync(file, "utf8")); + for (const m of code.matchAll(/\.confirmTransaction\(/g)) { + const idx = m.index ?? 0; + const window = code.slice(Math.max(0, idx - 500), idx + 500); + sites.push({ + file: path.relative(API_DIR, file), + line: code.slice(0, idx).split("\n").length, + // Either the shared helper, or auto-fund's older hand-rolled + // `airdropResult.value.err` check — both enforce the invariant. + checked: + window.includes("assertSuccessfulConfirmation") || + /\.value\.err/.test(window), + }); + } + } + return sites; +} + +describe("GH#2517: no API route discards a confirmTransaction result", () => { + it("finds the call sites at all (guards the scan itself)", () => { + // If a refactor moves these calls behind a wrapper, this test starts + // passing vacuously — so assert the scan still sees something. + expect(callSites().length).toBeGreaterThanOrEqual(10); + }); + + it("checks every one of them", () => { + const bare = callSites().filter((s) => !s.checked); + expect( + bare.map((s) => `${s.file}:${s.line}`), + "confirmTransaction() result discarded — wrap it in assertSuccessfulConfirmation()", + ).toEqual([]); + }); + + it("the seven routes GH#2517 named import the shared helper", () => { + const named = [ + "devnet-airdrop/route.ts", + "devnet-mint-token/route.ts", + "devnet-mirror-mint/route.ts", + "playground/faucet/route.ts", + ]; + for (const rel of named) { + const src = fs.readFileSync(path.join(API_DIR, rel), "utf8"); + expect(src, rel).toContain( + 'from "@/lib/transaction-confirmation"', + ); + } + }); +}); diff --git a/app/app/api/devnet-airdrop/route.ts b/app/app/api/devnet-airdrop/route.ts index 7c8a5cfca..9f5ba17b6 100644 --- a/app/app/api/devnet-airdrop/route.ts +++ b/app/app/api/devnet-airdrop/route.ts @@ -53,6 +53,7 @@ import { getConfig } from "@/lib/config"; import { getDevnetMintSigner } from "@/lib/devnet-signer"; import type { getServiceClient as _GetServiceClient } from "@/lib/supabase"; import * as Sentry from "@sentry/nextjs"; +import { assertSuccessfulConfirmation } from "@/lib/transaction-confirmation"; type SupabaseClient = ReturnType; @@ -368,7 +369,14 @@ async function resolveServerOwnedDevnetMint( try { const createSig = await connection.sendRawTransaction(signedCreateTx.serialize()); - await connection.confirmTransaction(createSig, "confirmed"); + // GH#2517: confirmTransaction() can RESOLVE with a SignatureResult whose + // `err` records an on-chain failure, so awaiting it is not proof of + // success. Without this, a mint that failed on chain still reached the + // `devnet_mints` upsert below and became the stored mapping. + assertSuccessfulConfirmation( + await connection.confirmTransaction(createSig, "confirmed"), + "Devnet mirror-mint creation", + ); } catch (e) { Sentry.captureException(e, { tags: { endpoint: "/api/devnet-airdrop", step: "resolveServerOwnedDevnetMint.createMint" }, @@ -757,7 +765,12 @@ export async function POST(req: NextRequest) { const txSig = await connection.sendRawTransaction( (signedTx as Transaction).serialize(), ); - await connection.confirmTransaction(txSig, "confirmed"); + // GH#2517: without this the 24h claim stays reserved and the + // response reports an amount even though nothing was minted. + assertSuccessfulConfirmation( + await connection.confirmTransaction(txSig, "confirmed"), + "Devnet airdrop mint", + ); return txSig; })(), 30_000, diff --git a/app/app/api/devnet-mint-token/route.ts b/app/app/api/devnet-mint-token/route.ts index 4feb48bbd..7be0e12de 100644 --- a/app/app/api/devnet-mint-token/route.ts +++ b/app/app/api/devnet-mint-token/route.ts @@ -37,6 +37,7 @@ import * as Sentry from "@sentry/nextjs"; import { getClientIp } from "@/lib/get-client-ip"; import { checkMintRateLimit } from "@/lib/devnet-mirror-mint-rate-limit"; import { tryFaucetGate, releaseFaucetClaim } from "@/lib/faucet-rate-gate"; +import { assertSuccessfulConfirmation } from "@/lib/transaction-confirmation"; export const dynamic = "force-dynamic"; @@ -310,7 +311,12 @@ export async function POST(req: NextRequest) { const airdropTxSig = await connection.sendRawTransaction( (signedAirdropTx as Transaction).serialize(), ); - await connection.confirmTransaction(airdropTxSig, "confirmed"); + // GH#2517: the thrown-error branch omits the airdrop fields, so an + // unchecked result here reports the optional airdrop as successful. + assertSuccessfulConfirmation( + await connection.confirmTransaction(airdropTxSig, "confirmed"), + "Devnet token airdrop", + ); mintSucceeded = true; return NextResponse.json({ @@ -399,7 +405,12 @@ export async function POST(req: NextRequest) { (tx as Transaction).partialSign(mintKeypair); tx = mintSigner.signTransaction(tx); const sig = await connection.sendRawTransaction((tx as Transaction).serialize()); - await connection.confirmTransaction(sig, "confirmed"); + // GH#2517: a nonexistent mint must not become the stored mapping or be + // returned as `status: "created"` to later callers. + assertSuccessfulConfirmation( + await connection.confirmTransaction(sig, "confirmed"), + "Devnet mint creation", + ); const devnetMint = mintKeypair.publicKey.toBase58(); diff --git a/app/app/api/devnet-mirror-mint/route.ts b/app/app/api/devnet-mirror-mint/route.ts index 9dc4a6f1f..85e573203 100644 --- a/app/app/api/devnet-mirror-mint/route.ts +++ b/app/app/api/devnet-mirror-mint/route.ts @@ -45,6 +45,7 @@ import { getServiceClient } from "@/lib/supabase"; import { getDevnetMintSigner } from "@/lib/devnet-signer"; import { validateTokenMetadata, validateDexScreenerResponse, validateJupiterTokenResponse } from "@/lib/token-metadata-validators"; import * as Sentry from "@sentry/nextjs"; +import { assertSuccessfulConfirmation } from "@/lib/transaction-confirmation"; export const dynamic = "force-dynamic"; @@ -359,7 +360,12 @@ export async function POST(req: NextRequest) { let sig: string; try { sig = await connection.sendRawTransaction(tx.serialize()); - await connection.confirmTransaction(sig, "confirmed"); + // GH#2517: with no pre-existing canonical row this route would otherwise + // persist or return an address whose mint creation failed. + assertSuccessfulConfirmation( + await connection.confirmTransaction(sig, "confirmed"), + "Devnet mirror-mint creation", + ); } catch (e) { Sentry.captureException(e, { tags: { endpoint: "/api/devnet-mirror-mint", step: "sendAndConfirm" }, diff --git a/app/app/api/playground/faucet/route.ts b/app/app/api/playground/faucet/route.ts index bddb008a9..80edc944a 100644 --- a/app/app/api/playground/faucet/route.ts +++ b/app/app/api/playground/faucet/route.ts @@ -45,6 +45,7 @@ import { } from "@solana/spl-token"; import { getDevnetMintSigner } from "@/lib/devnet-signer"; import * as Sentry from "@sentry/nextjs"; +import { assertSuccessfulConfirmation } from "@/lib/transaction-confirmation"; export const dynamic = "force-dynamic"; @@ -232,9 +233,15 @@ export async function POST(req: NextRequest) { (signedTx as Transaction).serialize(), { skipPreflight: false }, ); - await connection.confirmTransaction( - { signature: usdcSig, blockhash, lastValidBlockHeight }, - "confirmed", + // GH#2517: the catch below releases the durable claim slot, so throwing + // here is what stops a failed mint from recording the claim and telling + // the caller 10,000 Sim-USDC was delivered. + assertSuccessfulConfirmation( + await connection.confirmTransaction( + { signature: usdcSig, blockhash, lastValidBlockHeight }, + "confirmed", + ), + "Playground USDC faucet mint", ); } catch (mintErr) { // Release the durable claim slot so a mint failure doesn't lock the wallet. @@ -268,7 +275,12 @@ export async function POST(req: NextRequest) { const airdropSig: string = await Promise.race([ (async () => { const s = await pubConn.requestAirdrop(walletPk, SOL_AIRDROP_AMOUNT); - await pubConn.confirmTransaction(s, "confirmed"); + // GH#2517: an unchecked result advertises SOL success and stops the + // RPC fallback loop from trying the next endpoint. + assertSuccessfulConfirmation( + await pubConn.confirmTransaction(s, "confirmed"), + "Playground SOL airdrop", + ); return s; })(), new Promise((_, reject) => From 5f80d4c9b00a124a5d8b1e289ecc0ef617af67fd Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Fri, 14 Aug 2026 05:52:33 +0100 Subject: [PATCH 2/2] test(2517): bind each confirmation check to its own call, not to a window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's review on #2519 was right: the scan marked a call "checked" if `assertSuccessfulConfirmation` or `.value.err` appeared anywhere within ±500 characters. A bare confirmation sitting beside a guarded one therefore passed — which defeats the point of a guard whose whole job is catching the NEXT call site somebody adds. Demonstrated rather than assumed. Adding await connection.confirmTransaction(sig, "finalized"); immediately after the guarded call in devnet-mirror-mint passes the old scan and fails the new one. Every call in the tree is one of two shapes, so both are now recognised exactly: A assertSuccessfulConfirmation(await conn.confirmTransaction(...), "...") B const result = await conn.confirmTransaction(...) // result asserted, or // result.value.err read Shape B is keyed to the BINDING NAME, so an unrelated guarded call elsewhere in the same file cannot vouch for it. Mutation-tested, control either side: control 3 passed bare call adjacent to a guarded one 1 failed result bound but never asserted 1 failed control 3 passed Verified: 305 files, 3073 passed, 17 skipped; tsc --noEmit exit 0. Co-Authored-By: Claude Opus 5 --- .../api/confirm-transaction-checked.test.ts | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/app/__tests__/api/confirm-transaction-checked.test.ts b/app/__tests__/api/confirm-transaction-checked.test.ts index fb7608c58..10de58328 100644 --- a/app/__tests__/api/confirm-transaction-checked.test.ts +++ b/app/__tests__/api/confirm-transaction-checked.test.ts @@ -49,21 +49,60 @@ interface Site { checked: boolean; } +/** + * Decide whether ONE call's result is checked, bound to that call rather than + * to its neighbourhood. + * + * An earlier version of this scan looked for `assertSuccessfulConfirmation` + * anywhere within ±500 characters, which CodeRabbit correctly flagged on #2519: + * a bare confirmation sitting next to a guarded one would pass. That defeats the + * point of a guard whose whole job is catching the NEXT call site. + * + * Every call in the tree is one of two shapes, so both are recognised exactly: + * + * A assertSuccessfulConfirmation(await conn.confirmTransaction(...), "...") + * B const result = await conn.confirmTransaction(...) // then result is + * // asserted or + * // result.value.err + * // is inspected + * + * Shape B is tied to the BINDING NAME, so an unrelated guarded call elsewhere + * in the file cannot vouch for it. + */ +function isChecked(code: string, callIdx: number): boolean { + const before = code.slice(0, callIdx); + + // A: the call is the argument to the assertion. Allow the receiver + // expression and an `await` between the paren and the call. + if (/assertSuccessfulConfirmation\(\s*(?:await\s+)?[\w$.]*$/.test(before)) { + return true; + } + + // B: the call's result is bound; require that binding to be checked later. + const bound = before.match(/(?:const|let|var)\s+([\w$]+)\s*(?::[^=]+)?=\s*(?:await\s+)?[\w$.]*$/); + if (bound) { + const name = bound[1]; + const after = code.slice(callIdx); + const asserted = new RegExp( + `assertSuccessfulConfirmation\\(\\s*${name}\\b`, + ).test(after); + const inspected = new RegExp(`\\b${name}\\.value\\.err\\b`).test(after); + return asserted || inspected; + } + + return false; +} + function callSites(): Site[] { const sites: Site[] = []; for (const file of routeFiles(API_DIR)) { const code = stripComments(fs.readFileSync(file, "utf8")); for (const m of code.matchAll(/\.confirmTransaction\(/g)) { const idx = m.index ?? 0; - const window = code.slice(Math.max(0, idx - 500), idx + 500); sites.push({ file: path.relative(API_DIR, file), line: code.slice(0, idx).split("\n").length, - // Either the shared helper, or auto-fund's older hand-rolled - // `airdropResult.value.err` check — both enforce the invariant. - checked: - window.includes("assertSuccessfulConfirmation") || - /\.value\.err/.test(window), + checked: isChecked(code, idx), }); } }