feat: PostgreSQL migration, leaderboard page, security hardening#5
Open
feat: PostgreSQL migration, leaderboard page, security hardening#5
Conversation
Replace better-sqlite3 with pg Pool for multi-replica support. Lazy schema init avoids build-time ECONNREFUSED. Advisory locks replace in-memory address mutex. Nonces and rate limits moved to PostgreSQL. Add /leaderboard route with rankings, hall of fame, fleet stats. Externalize SESSION_SECRET, add HSTS, tighten CSP and API input validation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrate server from SQLite/better-sqlite3 to PostgreSQL/pg with async DB helpers; replace in-memory nonces, locks and rate-limits with DB-backed implementations; add paginated leaderboard UI and server loader; harden headers/CSP and defensive JSON parsing; remove better-sqlite3 from build/runtime and Docker libstdc++ install. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant Postgres
participant VendingMachine
Client->>Server: POST /api/game/deliver-reward (payload)
Server->>Postgres: TRY acquire advisory lock for stake_address
alt lock acquired
Server->>Postgres: SELECT session/ip/recent counts, game session
Postgres-->>Server: counts / session
Server->>Postgres: UPDATE game_sessions ... WHERE ... (atomic)
Postgres-->>Server: rowCount
alt deliver to vending
Server->>VendingMachine: GET treasury (cached)
VendingMachine-->>Server: supply data
end
Server->>Postgres: INSERT reward_log / upsert leaderboard / update counters
Postgres-->>Server: OK
Server-->>Client: 200 { delivered: true/false, reason? }
else lock not acquired
Server-->>Client: 429 "Request in progress"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/lib/server/session.js (1)
4-5: Silent fallback to random secret may cause subtle production failures.If
SESSION_SECRETis unset in a multi-replica deployment, each pod generates a different random secret, causing tokens issued by one replica to be rejected by others. Consider logging a warning (or failing startup in production) when the env var is missing, to make misconfiguration immediately visible.🛡️ Suggested improvement
+import { dev } from '$app/environment'; + // Stable secret from env (multi-replica safe), random fallback for dev -const SECRET = env.SESSION_SECRET || randomBytes(32).toString('hex'); +const SECRET = env.SESSION_SECRET || (() => { + if (!dev) { + console.warn('[session] SESSION_SECRET not set — tokens will not survive restarts or work across replicas'); + } + return randomBytes(32).toString('hex'); +})();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/session.js` around lines 4 - 5, The current SECRET assignment silently falls back to randomBytes(32) when env.SESSION_SECRET is unset, which breaks multi-replica token validation; update the startup logic around SECRET (the env.SESSION_SECRET check and use of randomBytes) to detect a missing SESSION_SECRET and either (a) log a clear warning with context and include process.env.NODE_ENV so operators can see misconfiguration, or (b) in non-development environments exit the process with an error; ensure the message identifies SESSION_SECRET and the app's environment so the issue is visible in logs and does not silently produce per-replica secrets.src/lib/server/nonce-store.js (1)
14-15: Consider probabilistic cleanup to reduce write amplification under load.Every
createNoncecall deletes all expired rows, which may cause write amplification under heavy traffic. A probabilistic approach (e.g., 1-5% chance) or a separate background job would be gentler on the database.♻️ Optional: probabilistic cleanup
export async function createNonce(stakeAddress) { const now = Date.now(); - // Opportunistic cleanup of expired nonces - await query('DELETE FROM nonces WHERE expires_at < $1', [now]); + // Probabilistic cleanup (~5% of requests) + if (Math.random() < 0.05) { + await query('DELETE FROM nonces WHERE expires_at < $1', [now]); + } const nonce = randomBytes(32).toString('hex');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/nonce-store.js` around lines 14 - 15, The current createNonce implementation runs an opportunistic cleanup on every call by executing query('DELETE FROM nonces WHERE expires_at < $1', [now]) which can cause heavy write amplification; change this to a probabilistic cleanup (e.g., only run the DELETE with a small random chance like 1–5%) or move the deletion into a separate background task and remove it from createNonce. Locate the DELETE call in nonce-store.js (inside createNonce) and either wrap it in a random chance guard before invoking query, or extract the deletion into a new cleanupExpiredNonces function and schedule it with a periodic timer or external job, ensuring createNonce only inserts the new nonce and does not always perform the DELETE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server/db.js`:
- Around line 168-174: activeSessionCount is counting all sessions started in
the time window including those already ended; update the SQL in
activeSessionCount to only count sessions where ended is false/0 (i.e., add an
AND ended = 0 or AND ended = FALSE condition) so it only returns currently
active sessions — modify the query call that selects from game_sessions and keep
using the same stakeAddress and cutoff parameters.
- Around line 146-154: The UPDATE in recordForgeAtomic currently omits the
session's ended flag so late/replayed forge requests can increment closed
sessions; modify the WHERE clause of the UPDATE in recordForgeAtomic to include
"AND ended = 0" (or the equivalent check against the ended column) alongside the
existing id, blocks_forged, and last_forge_at conditions so the increment only
occurs for active (not-ended) sessions.
- Around line 95-101: The aggregate query results (SUM/COUNT/BIGINT) are
returned as strings which leads to silent type-coercion bugs; update the
functions getSessionRewards, getIpRewards, getDeliveredNight, and getServerStats
to normalize those aggregate fields to numeric types before returning (e.g.,
wrap the returned aggregate values with Number(...) or parseInt(..., 10)),
ensuring fields like total (from getSessionRewards/getIpRewards),
delivered/night counts (getDeliveredNight), and total_blocks/max_session_blocks
(getServerStats) are converted to JS Numbers so downstream
arithmetic/comparisons work correctly.
In `@src/lib/server/leaderboard.js`:
- Around line 38-48: The current queries in src/lib/server/leaderboard.js use
INNER JOIN and GROUP BY l.display_name which collapses different wallets sharing
a display_name and drops game_sessions with no leaderboard row; update the two
query(...) calls to use LEFT JOIN between game_sessions (alias g) and
leaderboard (alias l), group by g.stake_address (not l.display_name) for the
games_played aggregation, and select the display_name via
COALESCE(l.display_name, g.stake_address) or similar for presentation so
sessions without a leaderboard entry are preserved and aggregation is by wallet.
- Around line 7-16: getRankedLeaderboard currently passes page/perPage directly
to LIMIT/OFFSET and orders only by score; validate and clamp page and perPage
(coerce to integers, default to 1 and 25 on NaN/<=0, and cap perPage at a
reasonable max like 100) before computing offset, and use those safe values in
the query; also make the ORDER BY stable by adding a deterministic tiebreaker
(e.g., ORDER BY score DESC, id ASC) so tied scores return a consistent row order
across requests; update usages of page, perPage, offset, and the SQL passed to
query(...) accordingly.
In `@src/routes/api/block_count/`+server.js:
- Around line 10-14: The code dereferences body._epoch_no without ensuring body
is a non-null object, which can throw for valid JSON like null; update the
request handling in the +server.js handler to validate the parsed body before
accessing _epoch_no: after await request.json() check that body !== null and
typeof body === "object" (and that "_epoch_no" in body or body._epoch_no !==
undefined), and if the shape is invalid return json({ error: 'Invalid payload'
}, { status: 400 }); only then assign const epochNo = body._epoch_no (and
optionally coerce/validate its type).
In `@src/routes/api/game/auth-nonce/`+server.js:
- Line 17: Wrap the await createNonce(stakeAddress) call in a try/catch inside
the route handler (the function that reads stakeAddress and calls createNonce)
so DB errors are handled explicitly; on catch, log the error and return a
structured HTTP response (e.g., JSON error body) with an appropriate status like
503 Service Unavailable (instead of letting an unstructured 500 bubble up), and
only proceed with normal response when createNonce resolves successfully. Ensure
you reference the createNonce call and the stakeAddress variable in the handler
when implementing the try/catch and response paths.
- Around line 7-11: The code destructures { stakeAddress } from body without
ensuring body is a non-null object, so valid JSON like null will cause a runtime
error; after parsing request.json() (where body is assigned) add a guard that
verifies typeof body === 'object' && body !== null and if not return json({
error: 'Invalid JSON' }, { status: 400 }) before performing const { stakeAddress
} = body so destructuring is only done on a safe object.
In `@src/routes/api/pool_history/`+server.js:
- Line 7: ALLOWED_PARAMS currently lists 'epoch_no' without the underscore,
causing inconsistency with Koios param naming and other callers that use
'_pool_bech32'; update the whitelist to use '_epoch_no' instead of 'epoch_no' in
the ALLOWED_PARAMS Set (refer to the ALLOWED_PARAMS constant and the existing
'_pool_bech32' entry) so incoming requests and Koios calls use the same
underscore-prefixed param name.
In `@src/routes/api/prometheus/`+server.js:
- Line 35: The current allowlist check uses query.startsWith(pattern) which
allows bypasses like "upstream_metric" for pattern "up"; change the predicate to
perform a token-boundary match instead by testing against an escaped-regex
anchored at start and requiring a boundary or end after the pattern (e.g. new
RegExp(`^${escapeRegex(pattern)}(?=$|\\W|\\.)`). Implement or reuse an
escapeRegex helper to escape pattern values, replace the arrow callback inside
ALLOWED_METRIC_PATTERNS.some(...) with this regex test, and ensure the logic
accepts exact matches or pattern followed by a valid token separator rather than
any prefix match.
In `@src/routes/leaderboard/`+page.server.js:
- Around line 5-6: The current load function uses
parseInt(url.searchParams.get('page') || '1') which yields NaN for non-numeric
input and lets Math.max(1, NaN) produce NaN; update load to sanitize the page
query by parsing with a radix (parseInt(..., 10)), validating the result with
Number.isFinite or Number.isInteger and falling back to 1 when the parsed value
is NaN or <= 0 so downstream code (e.g., getRankedLeaderboard offset
calculation) always receives a valid positive integer page value.
---
Nitpick comments:
In `@src/lib/server/nonce-store.js`:
- Around line 14-15: The current createNonce implementation runs an
opportunistic cleanup on every call by executing query('DELETE FROM nonces WHERE
expires_at < $1', [now]) which can cause heavy write amplification; change this
to a probabilistic cleanup (e.g., only run the DELETE with a small random chance
like 1–5%) or move the deletion into a separate background task and remove it
from createNonce. Locate the DELETE call in nonce-store.js (inside createNonce)
and either wrap it in a random chance guard before invoking query, or extract
the deletion into a new cleanupExpiredNonces function and schedule it with a
periodic timer or external job, ensuring createNonce only inserts the new nonce
and does not always perform the DELETE.
In `@src/lib/server/session.js`:
- Around line 4-5: The current SECRET assignment silently falls back to
randomBytes(32) when env.SESSION_SECRET is unset, which breaks multi-replica
token validation; update the startup logic around SECRET (the env.SESSION_SECRET
check and use of randomBytes) to detect a missing SESSION_SECRET and either (a)
log a clear warning with context and include process.env.NODE_ENV so operators
can see misconfiguration, or (b) in non-development environments exit the
process with an error; ensure the message identifies SESSION_SECRET and the
app's environment so the issue is visible in logs and does not silently produce
per-replica secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbfea6e9-8cb4-4554-9717-c489feb468d5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
Dockerfilepackage.jsonsrc/hooks.server.jssrc/lib/server/db.jssrc/lib/server/leaderboard.jssrc/lib/server/nonce-store.jssrc/lib/server/session.jssrc/routes/+layout.sveltesrc/routes/api/block_count/+server.jssrc/routes/api/game/auth-nonce/+server.jssrc/routes/api/game/check-wallet/+server.jssrc/routes/api/game/deliver-reward/+server.jssrc/routes/api/game/leaderboard/+server.jssrc/routes/api/game/start-session/+server.jssrc/routes/api/koios/[...path]/+server.jssrc/routes/api/pool_history/+server.jssrc/routes/api/prometheus/+server.jssrc/routes/health/+server.jssrc/routes/leaderboard/+page.server.jssrc/routes/leaderboard/+page.sveltevite.config.js
💤 Files with no reviewable changes (2)
- src/routes/api/koios/[...path]/+server.js
- vite.config.js
- Wrap pg aggregate returns (SUM/BIGINT) with Number() to prevent string coercion bugs in arithmetic - Add ended = 0 guard to recordForgeAtomic and activeSessionCount - Use LEFT JOIN + GROUP BY stake_address in hall of fame queries - Clamp pagination inputs, add stable ORDER BY tiebreaker - Fix prometheus allowlist bypass (startsWith → regex boundary) - Fix NaN propagation in leaderboard page param parsing - Add null/non-object JSON body guards on auth-nonce, block_count - Fix pool_history param name epoch_no → _epoch_no
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/routes/api/game/auth-nonce/+server.js (1)
20-20:⚠️ Potential issue | 🟠 MajorHandle DB failures from
createNonce()explicitly.The
createNoncecall is now async and backed by PostgreSQL. If the database is unavailable, this will throw and result in an unstructured 500 response. Wrap in try/catch to return a structured 503 error.🛡️ Proposed fix
- const nonce = await createNonce(stakeAddress); + let nonce; + try { + nonce = await createNonce(stakeAddress); + } catch { + return json({ error: 'Service temporarily unavailable' }, { status: 503 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/api/game/auth-nonce/`+server.js at line 20, The call to createNonce in the POST handler for the auth-nonce route must be wrapped in a try/catch so database failures produce a structured 503 response instead of an unstructured 500; update the code around createNonce(stakeAddress) in the +server.js handler to catch errors from the async createNonce call, log the error (including the error object), and return a Response/JSON with a 503 status and a clear error payload (e.g., { error: "Service unavailable" }) so callers receive a structured service-unavailable response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/routes/api/game/auth-nonce/`+server.js:
- Line 20: The call to createNonce in the POST handler for the auth-nonce route
must be wrapped in a try/catch so database failures produce a structured 503
response instead of an unstructured 500; update the code around
createNonce(stakeAddress) in the +server.js handler to catch errors from the
async createNonce call, log the error (including the error object), and return a
Response/JSON with a 503 status and a clear error payload (e.g., { error:
"Service unavailable" }) so callers receive a structured service-unavailable
response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1f83461-0a46-4f27-b408-7ae7779ce346
📒 Files selected for processing (7)
src/lib/server/db.jssrc/lib/server/leaderboard.jssrc/routes/api/block_count/+server.jssrc/routes/api/game/auth-nonce/+server.jssrc/routes/api/pool_history/+server.jssrc/routes/api/prometheus/+server.jssrc/routes/leaderboard/+page.server.js
✅ Files skipped from review due to trivial changes (1)
- src/lib/server/leaderboard.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/api/block_count/+server.js
- src/routes/api/prometheus/+server.js
- src/routes/leaderboard/+page.server.js
- src/routes/api/pool_history/+server.js
…atus Query vending machine treasury_read API (cached 60s) to check NIGHT balance. Early-exit deliver-reward when supply depleted. Show status banners on title screen, in-game HUD, and game-over screen. Real-time update if supply_exhausted returned mid-game.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/component/game-wrapper.svelte (1)
53-61:⚠️ Potential issue | 🟠 MajorSupply status not included in
walletState, breaking title screen display.The
supplyDepletedandsupplyLowflags are set directly on thegameobject (lines 60-61), butgame.walletState(lines 53-58) does not include these properties. Insrc/lib/game/hud.jsat lines 453-462,drawTitleScreencheckswalletState?.supplyDepletedandwalletState?.supplyLow—these will always beundefined, so the incentive banner will never show the "NIGHT SUPPLY DEPLETED" or "LOW NIGHT SUPPLY" messages on the title screen.Proposed fix — include supply status in walletState
// Sync wallet state into game for canvas rendering game.walletState = { phase: walletPhase, wallets, error: walletError, leaderboard, + supplyDepleted, + supplyLow, }; game.guestMode = guestMode; game.supplyDepleted = supplyDepleted; game.supplyLow = supplyLow;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/component/game-wrapper.svelte` around lines 53 - 61, The walletState object is missing the supply flags the HUD expects; update the assignment to include supplyDepleted and supplyLow in game.walletState (in addition to or instead of setting game.supplyDepleted/game.supplyLow) so drawTitleScreen can read walletState?.supplyDepleted and walletState?.supplyLow; locate the walletState assignment in game-wrapper.svelte (game.walletState = { ... }) and add supplyDepleted and supplyLow properties sourced from the existing supplyDepleted and supplyLow variables.
🧹 Nitpick comments (2)
src/lib/game/hud.js (1)
453-462: Inconsistent indentation in conditional block.The
if/else if/elseblock has inconsistent indentation—theifand firstelse ifuse tabs differently than the rest of the function. This appears to be a formatting issue.Additionally, as noted in
game-wrapper.svelte,walletState?.supplyDepletedandwalletState?.supplyLowwill always beundefinedbecause these properties are not included ingame.walletState. The fix should be applied ingame-wrapper.svelte.Suggested indentation fix (assuming walletState is fixed upstream)
- if (walletState?.supplyDepleted) { - drawGlowText(ctx, 'NIGHT SUPPLY DEPLETED — FORGE FOR SCORE', cx, bannerY, - 'bold 11px monospace', COLOR.red, COLOR.red, 4); - } else if (walletState?.supplyLow) { - drawGlowText(ctx, 'LOW NIGHT SUPPLY — FORGE WHILE YOU CAN', cx, bannerY, - 'bold 11px monospace', COLOR.amber, COLOR.amber, 4); - } else { + if (walletState?.supplyDepleted) { + drawGlowText(ctx, 'NIGHT SUPPLY DEPLETED — FORGE FOR SCORE', cx, bannerY, + 'bold 11px monospace', COLOR.red, COLOR.red, 4); + } else if (walletState?.supplyLow) { + drawGlowText(ctx, 'LOW NIGHT SUPPLY — FORGE WHILE YOU CAN', cx, bannerY, + 'bold 11px monospace', COLOR.amber, COLOR.amber, 4); + } else { drawGlowText(ctx, 'DELEGATORS EARN 10x NIGHT PER BLOCK', cx, bannerY, 'bold 11px monospace', COLOR.amber, COLOR.amber, 4); - } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/game/hud.js` around lines 453 - 462, Fix the inconsistent indentation in the conditional block around drawGlowText and correct the missing properties on walletState: normalize the indentation for the if / else if / else that reference walletState?.supplyDepleted and walletState?.supplyLow so the block matches surrounding code style, and then add the missing supplyDepleted and supplyLow properties to the game.walletState initialization in game-wrapper.svelte (or ensure they are derived where walletState is constructed) so walletState?.supplyDepleted and walletState?.supplyLow are not always undefined; keep the conditional logic in hud.js (around drawGlowText) intact but properly indented and rely on the fixed walletState source to provide those flags.src/routes/api/game/deliver-reward/+server.js (1)
131-136: Supply check doesn't handle fetch failures gracefully.When
getNightSupply()returnsnull(due to network errors or missing config),supply?.depletedevaluates toundefined(falsy), allowing the request to proceed to the VM call. This is arguably correct behavior (fail-open), but consider whether you want rewards to continue when supply status is unknown.If fail-closed behavior is preferred for safety:
Optional: fail-closed on unknown supply status
// Check NIGHT supply before wasting a VM call const supply = await getNightSupply(); + if (!supply) { + await logReward(stakeAddress, amount, 'supply_unknown', null, now); + return json({ delivered: false, reason: 'supply_check_failed' }); + } if (supply?.depleted) { await logReward(stakeAddress, amount, 'supply_exhausted', null, now); return json({ delivered: false, reason: 'supply_exhausted' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/api/game/deliver-reward/`+server.js around lines 131 - 136, The supply check currently treats a null/failed getNightSupply() as unknown and proceeds; change it to fail-closed by explicitly handling a null/undefined supply: after calling getNightSupply(), if supply is null/undefined or supply.depleted is true, call logReward(stakeAddress, amount, 'supply_check_failed' /* or 'supply_unknown' */ , null, now) and return json({ delivered: false, reason: 'supply_check_failed' }); update the branch that references supply?.depleted to instead test (supply == null || supply.depleted) so unknown fetch failures are treated as exhausted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server/night-supply.js`:
- Around line 13-19: The fetch response is parsed without checking HTTP status,
so replace the direct res.json() usage in the block that calls fetch with a
guard on res.ok (using vmUrl, vmKey and NIGHT_TOKEN_ID to locate the code): if
!res.ok, read the response body (text or json) and surface/throw an error
(including res.status and the body) instead of continuing to access
data[NIGHT_TOKEN_ID]; only call res.json() and proceed to inspect
data[NIGHT_TOKEN_ID] when res.ok is true so real API errors are not treated as a
depleted supply.
- Around line 21-26: Validate night.balance before computing thresholds: parse
Number(night.balance) into a variable (e.g., rawBalance) and check
Number.isFinite(rawBalance); only divide by 1_000_000 and compare to
LOW_THRESHOLD / DEPLETED_THRESHOLD when finite. If not finite, set available to
null (or a sentinel) and set lowSupply and depleted to true (or another explicit
"unknown" handling) so malformed balances don't silently report false; update
the return in the function that reads night.balance to use these checks and the
constants LOW_THRESHOLD and DEPLETED_THRESHOLD.
In `@src/routes/api/game/night-supply/`+server.js:
- Around line 6-8: The handler currently treats a null/failed getNightSupply()
as "not depleted" which is misleading; update the code path where supply is
falsy (the block returning json({ available: 0, lowSupply: false, depleted:
false })) to instead return an explicit unknown flag (e.g., json({ available: 0,
lowSupply: false, depleted: false, unknown: true })) or replace depleted with
unknown: true so clients can detect uncertainty; reference the getNightSupply
call and the endpoint response object so you adjust the fallback response
returned by the API handler accordingly.
---
Outside diff comments:
In `@src/lib/component/game-wrapper.svelte`:
- Around line 53-61: The walletState object is missing the supply flags the HUD
expects; update the assignment to include supplyDepleted and supplyLow in
game.walletState (in addition to or instead of setting
game.supplyDepleted/game.supplyLow) so drawTitleScreen can read
walletState?.supplyDepleted and walletState?.supplyLow; locate the walletState
assignment in game-wrapper.svelte (game.walletState = { ... }) and add
supplyDepleted and supplyLow properties sourced from the existing supplyDepleted
and supplyLow variables.
---
Nitpick comments:
In `@src/lib/game/hud.js`:
- Around line 453-462: Fix the inconsistent indentation in the conditional block
around drawGlowText and correct the missing properties on walletState: normalize
the indentation for the if / else if / else that reference
walletState?.supplyDepleted and walletState?.supplyLow so the block matches
surrounding code style, and then add the missing supplyDepleted and supplyLow
properties to the game.walletState initialization in game-wrapper.svelte (or
ensure they are derived where walletState is constructed) so
walletState?.supplyDepleted and walletState?.supplyLow are not always undefined;
keep the conditional logic in hud.js (around drawGlowText) intact but properly
indented and rely on the fixed walletState source to provide those flags.
In `@src/routes/api/game/deliver-reward/`+server.js:
- Around line 131-136: The supply check currently treats a null/failed
getNightSupply() as unknown and proceeds; change it to fail-closed by explicitly
handling a null/undefined supply: after calling getNightSupply(), if supply is
null/undefined or supply.depleted is true, call logReward(stakeAddress, amount,
'supply_check_failed' /* or 'supply_unknown' */ , null, now) and return json({
delivered: false, reason: 'supply_check_failed' }); update the branch that
references supply?.depleted to instead test (supply == null || supply.depleted)
so unknown fetch failures are treated as exhausted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12469583-a2be-4e67-ba38-d384136a8fa2
📒 Files selected for processing (6)
src/lib/component/game-wrapper.sveltesrc/lib/game/hud.jssrc/lib/server/night-supply.jssrc/routes/api/game/deliver-reward/+server.jssrc/routes/api/game/night-supply/+server.jssrc/routes/leaderboard/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/leaderboard/+page.svelte
Comment on lines
+13
to
+19
| const res = await fetch(`${vmUrl}?action=treasury_read`, { | ||
| headers: { 'X-API-Token': vmKey }, | ||
| signal: AbortSignal.timeout(10000), | ||
| }); | ||
| const data = await res.json(); | ||
| const night = data[NIGHT_TOKEN_ID]; | ||
| if (!night) return { available: 0, lowSupply: true, depleted: true }; |
There was a problem hiding this comment.
Missing HTTP status check before parsing JSON.
The code calls res.json() without first checking res.ok. If the vending machine returns a 4xx/5xx error with a JSON body (e.g., {"error": "unauthorized"}), the code will proceed and likely hit the !night branch on line 18-19, returning depleted: true for what is actually an API error.
Proposed fix
const res = await fetch(`${vmUrl}?action=treasury_read`, {
headers: { 'X-API-Token': vmKey },
signal: AbortSignal.timeout(10000),
});
+ if (!res.ok) {
+ throw new Error(`Treasury API error: ${res.status}`);
+ }
const data = await res.json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await fetch(`${vmUrl}?action=treasury_read`, { | |
| headers: { 'X-API-Token': vmKey }, | |
| signal: AbortSignal.timeout(10000), | |
| }); | |
| const data = await res.json(); | |
| const night = data[NIGHT_TOKEN_ID]; | |
| if (!night) return { available: 0, lowSupply: true, depleted: true }; | |
| const res = await fetch(`${vmUrl}?action=treasury_read`, { | |
| headers: { 'X-API-Token': vmKey }, | |
| signal: AbortSignal.timeout(10000), | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`Treasury API error: ${res.status}`); | |
| } | |
| const data = await res.json(); | |
| const night = data[NIGHT_TOKEN_ID]; | |
| if (!night) return { available: 0, lowSupply: true, depleted: true }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/night-supply.js` around lines 13 - 19, The fetch response is
parsed without checking HTTP status, so replace the direct res.json() usage in
the block that calls fetch with a guard on res.ok (using vmUrl, vmKey and
NIGHT_TOKEN_ID to locate the code): if !res.ok, read the response body (text or
json) and surface/throw an error (including res.status and the body) instead of
continuing to access data[NIGHT_TOKEN_ID]; only call res.json() and proceed to
inspect data[NIGHT_TOKEN_ID] when res.ok is true so real API errors are not
treated as a depleted supply.
Comment on lines
+21
to
+26
| const balance = Number(night.balance) / 1_000_000; | ||
| return { | ||
| available: balance, | ||
| lowSupply: balance < LOW_THRESHOLD, | ||
| depleted: balance < DEPLETED_THRESHOLD, | ||
| }; |
There was a problem hiding this comment.
NaN propagation from malformed balance produces incorrect supply status.
If night.balance is undefined, null, or a non-numeric string, Number(night.balance) returns NaN. Then NaN < LOW_THRESHOLD and NaN < DEPLETED_THRESHOLD both evaluate to false, causing the function to report lowSupply: false, depleted: false when the actual state is unknown.
Proposed fix
const balance = Number(night.balance) / 1_000_000;
+ if (Number.isNaN(balance)) {
+ return { available: 0, lowSupply: true, depleted: true };
+ }
return {
available: balance,
lowSupply: balance < LOW_THRESHOLD,
depleted: balance < DEPLETED_THRESHOLD,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const balance = Number(night.balance) / 1_000_000; | |
| return { | |
| available: balance, | |
| lowSupply: balance < LOW_THRESHOLD, | |
| depleted: balance < DEPLETED_THRESHOLD, | |
| }; | |
| const balance = Number(night.balance) / 1_000_000; | |
| if (Number.isNaN(balance)) { | |
| return { available: 0, lowSupply: true, depleted: true }; | |
| } | |
| return { | |
| available: balance, | |
| lowSupply: balance < LOW_THRESHOLD, | |
| depleted: balance < DEPLETED_THRESHOLD, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/night-supply.js` around lines 21 - 26, Validate night.balance
before computing thresholds: parse Number(night.balance) into a variable (e.g.,
rawBalance) and check Number.isFinite(rawBalance); only divide by 1_000_000 and
compare to LOW_THRESHOLD / DEPLETED_THRESHOLD when finite. If not finite, set
available to null (or a sentinel) and set lowSupply and depleted to true (or
another explicit "unknown" handling) so malformed balances don't silently report
false; update the return in the function that reads night.balance to use these
checks and the constants LOW_THRESHOLD and DEPLETED_THRESHOLD.
Comment on lines
+6
to
+8
| if (!supply) { | ||
| return json({ available: 0, lowSupply: false, depleted: false }); | ||
| } |
There was a problem hiding this comment.
Inconsistent default when supply fetch fails.
When getNightSupply() returns null (due to fetch errors or missing config), the endpoint returns depleted: false. This is misleading—a failure to determine supply status should not be interpreted as "supply is available." The client may continue attempting rewards when the supply state is actually unknown.
Consider returning an explicit unknown: true flag or propagating the error so the client can handle uncertainty appropriately.
Proposed fix
export async function GET() {
const supply = await getNightSupply();
if (!supply) {
- return json({ available: 0, lowSupply: false, depleted: false });
+ return json({ available: 0, lowSupply: false, depleted: false, unknown: true });
}
return json(supply, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/api/game/night-supply/`+server.js around lines 6 - 8, The handler
currently treats a null/failed getNightSupply() as "not depleted" which is
misleading; update the code path where supply is falsy (the block returning
json({ available: 0, lowSupply: false, depleted: false })) to instead return an
explicit unknown flag (e.g., json({ available: 0, lowSupply: false, depleted:
false, unknown: true })) or replace depleted with unknown: true so clients can
detect uncertainty; reference the getNightSupply call and the endpoint response
object so you adjust the fallback response returned by the API handler
accordingly.
After delegation TX is submitted, immediately grant 10x multiplier and re-issue session token via /api/game/refresh-session. Removes the awaitTx delay and the redundant connectWallet call that forced a second CIP-8 signature popup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/api/game/refresh-session/+server.js (1)
49-49: Avoid silent Koios failures; add structured logging.Line 49 swallows provider/network errors, which makes incident diagnosis and alerting difficult.
🛠️ Proposed fix
- } catch {} + } catch (error) { + console.warn('refresh-session: koios account_info failed', { + stakeAddress, + error: String(error) + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/api/game/refresh-session/`+server.js at line 49, The empty catch block after the Koios provider/network call should not silently swallow errors; replace the empty catch {} with structured logging and appropriate error handling: capture the caught error (e) and call your logger (e.g., processLogger.error or console.error) with a descriptive message including context (e.g., "Koios provider call failed", provider/network name, sessionId or userId if available) and the error object, then either rethrow the error or return a controlled error response from the route handler so failures are observable and alertable; update the catch located directly after the Koios call in the route handler (the empty catch {} in +server.js) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/api/game/refresh-session/`+server.js:
- Around line 29-31: The code currently trusts the client-supplied delegated
boolean (variable delegated and derived isDelegated) and uses it when minting a
delegated token; replace this by deriving delegation server-side only: stop
using !!delegated, instead validate delegation by checking server-side state or
verifying the delegation transaction (e.g., call your session lookup / DB flag
or a verifyDelegationTx(wallet, tx) helper) and set isDelegated from that
trusted check, and ensure the code path that mints or issues delegated tokens
(the mint/create session token code that currently reads isDelegated) uses this
server-validated result only.
---
Nitpick comments:
In `@src/routes/api/game/refresh-session/`+server.js:
- Line 49: The empty catch block after the Koios provider/network call should
not silently swallow errors; replace the empty catch {} with structured logging
and appropriate error handling: capture the caught error (e) and call your
logger (e.g., processLogger.error or console.error) with a descriptive message
including context (e.g., "Koios provider call failed", provider/network name,
sessionId or userId if available) and the error object, then either rethrow the
error or return a controlled error response from the route handler so failures
are observable and alertable; update the catch located directly after the Koios
call in the route handler (the empty catch {} in +server.js) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4b17fbb-16b4-4a65-8ac9-42cb06fa24e4
📒 Files selected for processing (2)
src/lib/component/game-wrapper.sveltesrc/routes/api/game/refresh-session/+server.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/component/game-wrapper.svelte
Comment on lines
+29
to
+31
| // If client reports delegation TX was just submitted, trust it | ||
| // (they already have a valid session — worst case they get 10x for one session) | ||
| let isDelegated = !!delegated; |
There was a problem hiding this comment.
Client-controlled delegation flag enables privilege escalation.
Line 31 trusts a client-supplied delegated value and Line 52 mints a delegated token from it. Any valid non-delegated session can self-upgrade by posting {"delegated": true}.
🔐 Proposed fix (derive delegation server-side only)
- // If client reports delegation TX was just submitted, trust it
- // (they already have a valid session — worst case they get 10x for one session)
- let isDelegated = !!delegated;
+ // Never trust client-supplied privilege flags
+ let isDelegated = session.isDelegated;
- // Otherwise verify via Koios
- if (!isDelegated) {
+ // Verify via Koios only when not already delegated
+ if (!isDelegated) {
const koios = getKoiosUrl();
try {
const res = await fetch(`${koios}/account_info`, {Also applies to: 34-35, 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/api/game/refresh-session/`+server.js around lines 29 - 31, The
code currently trusts the client-supplied delegated boolean (variable delegated
and derived isDelegated) and uses it when minting a delegated token; replace
this by deriving delegation server-side only: stop using !!delegated, instead
validate delegation by checking server-side state or verifying the delegation
transaction (e.g., call your session lookup / DB flag or a
verifyDelegationTx(wallet, tx) helper) and set isDelegated from that trusted
check, and ensure the code path that mints or issues delegated tokens (the
mint/create session token code that currently reads isDelegated) uses this
server-validated result only.
CIP-8 signature already proves wallet ownership. If Turnstile widget can't load (ad blocker, browser privacy), allow auth to proceed rather than hard-blocking with 403.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/leaderboardroute with ranked table, tier badges, hall of fame, fleet stats, and pagination (SSR)worker-src 'self'CSP directive, API input validation (query param whitelist,startsWithfor Prometheus, JSON parse try/catch)ssr.externalfrom vite configTest plan
npm run buildpasses (confirmed locally)starforgedb)/healthreturns 200 when PG is reachable, 503 when not/leaderboardrenders rankings, hall of fame, fleet statsSummary by CodeRabbit
New Features
Security
Bug Fixes
Infrastructure
Dependencies