-
Notifications
You must be signed in to change notification settings - Fork 35
fix(api): check confirmTransaction results in the seven routes that discarded them (GH#2517) #2519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| /** | ||
| * 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; | ||
| } | ||
|
|
||
| /** | ||
| * 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; | ||
| sites.push({ | ||
| file: path.relative(API_DIR, file), | ||
| line: code.slice(0, idx).split("\n").length, | ||
| checked: isChecked(code, idx), | ||
| }); | ||
| } | ||
| } | ||
| 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"', | ||
| ); | ||
| } | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<typeof _GetServiceClient>; | ||
|
|
||
|
|
@@ -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", | ||
| ); | ||
|
Comment on lines
+372
to
+379
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🌐 Web query:
💡 Result: For Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
for f in \
app/app/api/devnet-airdrop/route.ts \
app/app/api/devnet-mint-token/route.ts \
app/app/api/devnet-mirror-mint/route.ts \
app/app/api/playground/faucet/route.ts
do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
rg -n -C 18 'confirmTransaction|sendRawTransaction|requestAirdrop|getLatestBlockhash|getLatestBlockhashAndContext|lastValidBlockHeight|assertSuccessfulConfirmation' "$f"
fi
done
printf '%s\n' '--- package/version references ---'
rg -n -S '`@solana/web3`\.js|solana-web3|web3\.js' --glob 'package.json' --glob 'package-lock.json' --glob 'yarn.lock' --glob 'pnpm-lock.yaml' --glob 'bun.lockb' .
printf '%s\n' '--- helper definitions and uses ---'
rg -n -S 'assertSuccessfulConfirmation' .Repository: dcccrypto/percolator-launch Length of output: 50383 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- confirmation helper ---'
helper="$(fd -t f 'transaction-confirmation' . | head -n 1 || true)"
if [ -n "$helper" ]; then
echo "### $helper"
cat -n "$helper"
fi
printf '%s\n' '--- exact imports and relevant source slices ---'
for f in \
app/app/api/devnet-airdrop/route.ts \
app/app/api/devnet-mint-token/route.ts \
app/app/api/devnet-mirror-mint/route.ts \
app/app/api/playground/faucet/route.ts
do
echo "### $f"
sed -n '1,45p' "$f"
rg -n -C 5 'getLatestBlockhash|sendRawTransaction|requestAirdrop|confirmTransaction' "$f" \
| sed -n '1,260p'
done
printf '%s\n' '--- package-lock direct dependency ---'
sed -n '1,25p;895,915p' package-lock.json
printf '%s\n' '--- web3.js v1.98.4 confirmation implementation ---'
curl -fsSL https://raw.githubusercontent.com/solana-foundation/solana-web3.js/v1.98.4/src/connection.ts \
| rg -n -C 18 'confirmTransaction\\(|requestAirdrop\\(' \
| sed -n '1,300p'Repository: dcccrypto/percolator-launch Length of output: 19854 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- confirmation helper definition ---'
fd -t f -i 'transaction-confirmation' . | while read -r f; do
case "$f" in
*/lib/transaction-confirmation.ts|*/lib/transaction-confirmation.tsx)
echo "### $f"
cat -n "$f"
;;
esac
done
printf '%s\n' '--- web3.js v1.98.4 source excerpts ---'
python3 - <<'PY'
from urllib.request import urlopen
urls = [
"https://raw.githubusercontent.com/solana-foundation/solana-web3.js/v1.98.4/src/connection.ts",
"https://raw.githubusercontent.com/solana-labs/solana-web3.js/v1.98.4/src/connection.ts",
]
text = None
for url in urls:
try:
text = urlopen(url, timeout=20).read().decode()
print("SOURCE:", url)
break
except Exception as exc:
print("FETCH_FAILED:", url, type(exc).__name__, str(exc))
if text is None:
raise SystemExit("Could not fetch connection.ts")
lines = text.splitlines()
needles = ("confirmTransaction(", "requestAirdrop(")
starts = [i for i, line in enumerate(lines) if any(n in line for n in needles)]
for start in starts:
lo = max(0, start - 8)
hi = min(len(lines), start + 75)
print(f"\n--- lines {lo + 1}-{hi} ---")
for i in range(lo, hi):
print(f"{i + 1}: {lines[i]}")
PYRepository: dcccrypto/percolator-launch Length of output: 2253 🏁 Script executed: #!/bin/bash
set -eu
curl -kfsSL https://registry.npmjs.org/@solana/web3.js/-/web3.js-1.98.4.tgz \
| tar -xzOf - package/src/connection.ts \
| python3 -c '
import sys
lines = sys.stdin.read().splitlines()
needles = ("confirmTransaction(", "requestAirdrop(")
starts = [i for i, line in enumerate(lines) if any(n in line for n in needles)]
for start in starts:
lo = max(0, start - 10)
hi = min(len(lines), start + 90)
print(f"--- lines {lo + 1}-{hi} ---")
for i in range(lo, hi):
print(f"{i + 1}: {lines[i]}")
'Repository: dcccrypto/percolator-launch Length of output: 22377 🏁 Script executed: #!/bin/bash
set -eu
curl -kfsSL https://registry.npmjs.org/@solana/web3.js/-/web3.js-1.98.4.tgz \
| tar -xzOf - package/src/connection.ts \
| python3 -c '
import sys
lines = sys.stdin.read().splitlines()
needles = (
"confirmTransactionUsingLegacyTimeoutStrategy",
"confirmTransactionUsingBlockHeightExceedanceStrategy",
"BLOCKHEIGHT_EXCEEDED",
"lastValidBlockHeight",
"TRANSACTION being confirmed",
)
for i, line in enumerate(lines):
if any(n in line for n in needles):
lo = max(0, i - 12)
hi = min(len(lines), i + 100)
print(f"--- lines {lo + 1}-{hi} ---")
for j in range(lo, hi):
print(f"{j + 1}: {lines[j]}")
'Repository: dcccrypto/percolator-launch Length of output: 50383 Use blockheight-aware confirmation for the five raw transactions and bounded status polling for the SOL airdrop.
📍 Affects 4 files
🤖 Prompt for AI Agents |
||
| } 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, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.