Skip to content

fix: Phase-0 review follow-up for merged PRs #306 + #307#308

Merged
irfndi merged 3 commits into
mainfrom
fix/phase0-review-followup
Jul 23, 2026
Merged

fix: Phase-0 review follow-up for merged PRs #306 + #307#308
irfndi merged 3 commits into
mainfrom
fix/phase0-review-followup

Conversation

@irfndi

@irfndi irfndi commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Follow-up PR addressing the review findings on the merged PR #306 (G2.5 TWAP oracle) and PR #307 (Phase-0 apps G1–G4).

#306 contracts

Finding Fix
P2 — negative TWAP avg ticks truncated toward zero (AetherHook.sol:325) Floor division toward −∞ in getTwapTick (e.g. -70000/90 → -778); tests updated to floor expectations incl. the -778 vector
P2 — inverted quote computed from the rounded direct price (AetherHook.sol:504) Full-precision reciprocal Q96²·1e18/sqrtP² straight from sqrtPriceX96; representable + accurate even when the direct price floors to zero (test rewritten accordingly)
Docstring contradicts test body (AetherHookTwap.t.sol:135) NatSpec aligned with the implemented schedule
Invariant window vs ring overflow (AetherHookInvariants.t.sol:224) Handler tracks the oldest retained observation; invariant clamps the window to it

#307 apps

Finding Fix
Non-numeric limit → empty result NaN guard in route (+ defensive Number.isFinite clamp in the service)
🟠 Major — token cache/schema not chain-scoped Migration 0003: tokens PK (chain_id, address); chain_id in INSERT/ON CONFLICT; WHERE chain_id on D1 fallback + cron refresh; TokenSchema.chainId
Fabricated timestamps on every cached read asOf stamped once per refresh, stored in the KV envelope, reused on reads (regression test added)
ws-routing listener ordering (ws-routing.test.ts:52) Promises created before sends
Token address validation (chain-state-reader.ts:130) isAddress + getAddress before Token; constructor failures map to OnChainReadError("invalid_pool_key")
P1 — tick probes not aligned to tick-spacing (chain-state-reader.ts:177) Scan anchored to boundaries (floor(tick/spacing)·spacing ± k·spacing, anchor included)
P1 — legacy fallback on any V4 error (swap.service.ts:312) Rollback only when the reader reports not_configured; authoritative failures propagate
P1 — prices never published to the WS hub (index.ts:129) Price-refresh queue publishes POST /price after each KV write
P2 — price WS payload mismatched its consumer (index.ts:129) Per-token {tokenAddress, price, updatedAt} envelope; PriceTicker unwraps + filters by token (no undefined.toFixed crash)
P2 — quotes leaving the scanned tick window (swap.ts:65) verifiedTickWindow in state; engine rejects out-of-window simulations (price_out_of_range)
P2 — default token list never retries (TokenSearch.tsx:104) loaded flag set only on retained success
Canonicalize poolId casing (index.ts:141) toLowerCase() before DO selection + forwarded key (+ regression test)
Dependency pins (package.json) jsbi@3/ethers@5 documented as explicit test-pool exceptions in apps/api/README.md

Verification (all exit 0)

  • contracts: forge build, forge test — 134 passing
  • root: bun run typecheck, bun run lint, bun run test — 88 passing
  • api: typecheck, test — 44 passing, build (wrangler dry-run)
  • web: build, typecheck

Summary by CodeRabbit

  • New Features

    • Token data, caching, and search are now chain-scoped for more reliable multi-network support.
    • Live price streaming is now delivered per token (token-address subscriptions).
    • Verified-tick-window constraints can cause quotes to reject out-of-range prices.
  • Bug Fixes

    • Improved limit parsing to prevent NaN/negative pagination issues.
    • Canonicalized pool IDs so mixed-case and lowercase map to the same orderbook snapshot.
    • Safer quote/swap error handling now includes “price out of range”.
  • Documentation

    • Added notes documenting intentional dependency version exceptions.
  • Tests

    • Updated/added coverage for token caching, WebSocket updates, and TWAP edge cases.

Contracts (PR #306 review):
- AetherHook.getTwapTick: floor negative, non-divisible avg ticks toward
  negative infinity instead of truncating toward zero (-70000/90 = -778,
  not -777), removing the upward TWAP bias that could spuriously cross
  TP/SL thresholds
- AetherHook._tickToPriceX18: compute the reciprocal at full precision
  directly from sqrtPriceX96 (Q96^2*1e18/sqrtP^2) instead of inverting the
  already-rounded direct quote; reverse-direction keeper reads stay
  accurate and representable even when the direct price rounds to zero
- Tests: floor expectations, full-precision reciprocal helper, reciprocal
  readable at extreme negative ticks, corrected stale docstring
- Invariant fuzz handler tracks the oldest RETAINED ring observation and
  clamps the TWAP window to it, so ring overflow no longer fails the
  invariant on buffer bounds instead of TWAP correctness

Apps (PR #307 review):
- api tokens route: non-numeric limit falls back to the default (100)
  instead of NaN -> slice(0,0) -> empty result
- api ws routing: canonicalize poolId (toLowerCase) before DO selection
  and snapshot key so mixed-case ids map to one order book
- api token-list: chain-scoped tokens table (migration 0003, composite PK
  (chain_id, address)) + chain_id in INSERT/ON CONFLICT, D1 read filter,
  TokenSchema.chainId, chain-scoped cron refresh
- api token-list: stamp timestamps once per refresh and reuse on cached
  reads (no fabricated per-request Date.now())
- api chain-state-reader: validate/EIP-55-normalize pool addresses before
  Token construction (invalid_pool_key), anchor the tick scan to
  tickSpacing boundaries and expose the verified tick window
- api quote-engine: reject simulations that exit the verified tick window
  (price_out_of_range) instead of quoting unverified territory
- api swap.service: legacy rollback only when the reader is explicitly
  not_configured, never after an authoritative V4 quote failure
- api websocket hub: per-token price_update payload {tokenAddress, price,
  updatedAt} published by the price-refresh queue pipeline (was a
  handshake-only route); web PriceTicker unwraps the envelope and filters
  by token (no more undefined.toFixed crash)
- web TokenSearch: mark default list loaded only on a retained success so
  failed/cancelled loads retry on reopen
- api: document jsbi@3 + ethers@5 test-pool dependency exceptions
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@irfndi, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c5872667-54c5-4e39-9cca-ea7af8fe3978

📥 Commits

Reviewing files that changed from the base of the PR and between fd3088a and 1ab2116.

📒 Files selected for processing (2)
  • apps/api/migrations/0003_chain_scoped_tokens.sql
  • apps/api/src/durable-objects/websocket-hub-do.ts

Walkthrough

This change scopes token data by chain, adds token-based price WebSocket updates, constrains V4 quotes to verified tick windows, normalizes orderbook keys, and revises contract TWAP rounding, reciprocal pricing, and invariant modeling.

Changes

API data and quote flows

Layer / File(s) Summary
Chain-scoped token data
apps/api/migrations/..., apps/api/src/db/*, apps/api/src/services/token*.ts, apps/api/src/workers/*, packages/shared/src/schema.ts, apps/api/test/services/*
Token persistence, caching, refresh queries, schemas, timestamps, pagination, and tests now include chain identifiers and shared refresh timestamps.
Verified quote bounds
apps/api/src/services/chain-state-reader.ts, apps/api/src/services/quote-engine.ts, apps/api/src/services/swap.service.ts
Pool keys are validated, tick scans align to spacing boundaries, and quotes fail when simulated ticks leave the verified window; legacy fallback is limited to unconfigured readers in auto mode.
Token price WebSocket flow
apps/api/src/durable-objects/websocket-hub-do.ts, apps/api/src/workers/queue-handler.ts, apps/api/src/index.ts, apps/api/test/ws-routing.test.ts
Price refreshes publish token updates to the hub, which routes token envelopes to subscribers; orderbook IDs are lowercased consistently.
Frontend token loading
apps/web/src/components/PriceTicker.tsx, apps/web/src/components/TokenSearch.tsx, apps/web/test/components/TokenSearch.test.tsx
PriceTicker unwraps and filters token envelopes, while default token loading marks completion only after a successful request.

Contract TWAP precision

Layer / File(s) Summary
TWAP arithmetic and reciprocal pricing
packages/contracts/src/hook/AetherHook.sol
TWAP tick division now floors toward negative infinity, and inverted prices use full-precision square-root-price calculations.
TWAP validation models
packages/contracts/test/fuzz/AetherHookInvariants.t.sol, packages/contracts/test/unit/AetherHook*.t.sol
Tests and fuzz models reflect floor rounding, precise inversion, and the retained observation ring.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant QueueHandler
  participant WebSocketHubDO
  participant PriceTicker
  QueueHandler->>WebSocketHubDO: POST token price update
  WebSocketHubDO->>PriceTicker: price_update envelope
  PriceTicker->>PriceTicker: filter matching token address
Loading

Possibly related PRs

  • irfndi/AetherDEX#307: Earlier V4 quote-path refactoring directly related to verified tick windows and quote error handling.

Poem

A rabbit hops where token prices gleam,
Chain-scoped carrots line the stream.
Ticks floor softly, quotes stay true,
Precise TWAPs sparkle anew.
Hop, hop, review the change tonight!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is related, but it's generic and only references review follow-up and prior PRs rather than the actual code changes. Use a concise title that names the main change, such as chain-scoped tokens, websocket routing fixes, and TWAP/quote behavior updates.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/phase0-review-followup

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @irfndi, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9eb47a05f4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

(chain_id, address, symbol, name, decimals, logo_url, is_verified, is_native, total_supply, created_at, updated_at)
SELECT 1, address, symbol, name, decimals, logo_url, is_verified, is_native, total_supply, created_at, updated_at
FROM tokens;
DROP TABLE tokens;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Rebuild foreign-key dependents before dropping tokens

When this migration runs against a database containing any pool row, pools.token0_address and token1_address still reference tokens(address), so with foreign-key enforcement enabled this DROP TABLE tokens fails immediately with FOREIGN KEY constraint failed; the later pools_v2 rebuild is never reached. The migration therefore cannot upgrade a non-empty D1 database and needs a foreign-key-safe/deferred rebuild of the dependency graph.

Useful? React with 👍 / 👎.

Comment on lines +144 to +146
INSERT INTO tokens (chain_id, address, symbol, name, decimals, logo_url, is_verified, is_native, total_supply, created_at, updated_at)
VALUES (${token.chainId}, ${token.address}, ${token.symbol}, ${token.name}, ${token.decimals}, ${token.logoUrl}, ${token.isVerified ? 1 : 0}, ${token.isNative ? 1 : 0}, ${token.totalSupply}, ${Date.now()}, ${Date.now()})
ON CONFLICT(chain_id, address) DO UPDATE SET

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add chain filters to every TokenService read

Once the new composite key stores the same address for two chains, this chain-scoped upsert can create both rows, but getToken, listTokens, searchTokens, and getVerifiedTokens still query without chain_id and the service has no current-chain dependency. Consequently getToken(address) can select either chain nondeterministically and list/search calls mix chains; scope every read by an explicit chain ID just as the write is scoped.

AGENTS.md reference: AGENTS.md:L24-L24

Useful? React with 👍 / 👎.

Comment thread apps/api/src/workers/queue-handler.ts Outdated
await hub.fetch(
new Request("http://price-hub/price", {
method: "POST",
body: JSON.stringify({ tokenAddress: token, price: priceUsd, updatedAt }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject failed price lookups before broadcasting

When CoinGecko returns a non-2xx response, lacks the token, or throws—and for the configured Sepolia environment the helper still queries the Ethereum endpoint—fetchTokenPrice returns 0 rather than failing. This new broadcast therefore replaces a connected ticker's last valid value with $0.00000000; validate priceUsd > 0 or propagate the fetch failure before publishing the update.

AGENTS.md reference: AGENTS.md:L18-L18

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
apps/api/src/workers/queue-handler.ts (1)

74-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Sequential, un-timed-out hub notification adds latency risk per token.

Each token iteration now does 3 sequential network calls (price fetch, KV put, hub.fetch), and the new hub.fetch call has no timeout/AbortSignal. If the Durable Object stalls, this loop blocks the whole queue message for msg.tokens.length iterations instead of failing fast.

♻️ Suggested guard
       await hub.fetch(
         new Request("http://price-hub/price", {
           method: "POST",
           body: JSON.stringify({ tokenAddress: token, price: priceUsd, updatedAt }),
+          signal: AbortSignal.timeout(5_000),
         }),
       )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/src/workers/queue-handler.ts` around lines 74 - 97, Update the
token-processing loop around hub.fetch so the Durable Object notification uses a
bounded timeout via an AbortSignal, failing fast if the hub stalls. Preserve the
existing per-token error handling and price/KV updates, while ensuring a
timed-out notification does not block subsequent tokens for the full queue
message.
apps/api/src/durable-objects/websocket-hub-do.ts (1)

111-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pool-flavored naming leftover for token subscriptions.

watchedKeys/knownTokens were renamed to reflect the token model, but the wire protocol still uses msg.poolId, "list_pools", and "pool_list" — confusing for anyone subscribing to this price hub with a token address under a field/message literally named "pool".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/src/durable-objects/websocket-hub-do.ts` around lines 111 - 125,
Update the websocket subscription protocol in the subscribe/unsubscribe and
list_pools handlers to use token-oriented naming consistently: replace
msg.poolId and poolId payload fields with the established token identifier name,
and rename the list request/response message types from pool-based names to
token-based names. Keep watchedKeys and knownTokens behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/api/migrations/0003_chain_scoped_tokens.sql`:
- Around line 27-28: Update the migration’s table-rebuild sequence around
tokens_chain_scoped and the dependent pools, transactions, and
liquidity_positions tables so foreign-key dependencies are handled before
dropping referenced tables. Either rebuild dependent tables first or explicitly
disable enforcement for the atomic migration, then run foreign_key_check before
committing and preserve the intended final schema.

In `@apps/api/src/routes/tokens.ts`:
- Around line 53-56: Reject or default negative token limits before they reach
slicing: update the parsed-limit handling in tokens.ts to treat values below
zero like invalid input, and add the same non-negative lower-bound enforcement
in the token-list service limit handling for non-HTTP callers. Apply the change
at apps/api/src/routes/tokens.ts lines 53-56 and
apps/api/src/services/token-list.service.ts lines 199-204.

In `@apps/api/src/workers/cron-handler.ts`:
- Around line 79-86: Move the database and queue operations in refreshTopPools()
and enqueuePriceRefresh() behind the existing Db and WorkerEnv Effect services,
replacing direct env.DB.prepare(...) and env.PRICE_QUEUE.send(...) usage. Run
these Effect workflows with Layer.provide, and preserve failures through
structured Effect error handling rather than unhandled raw promise errors.

In `@apps/api/test/services/token-list.test.ts`:
- Around line 164-174: Update the “stamps timestamps once per refresh” test to
assert fetcher.calls() equals 1 after both listTokens reads, proving the second
read used the KV cache; keep the existing timestamp assertions unchanged.

---

Nitpick comments:
In `@apps/api/src/durable-objects/websocket-hub-do.ts`:
- Around line 111-125: Update the websocket subscription protocol in the
subscribe/unsubscribe and list_pools handlers to use token-oriented naming
consistently: replace msg.poolId and poolId payload fields with the established
token identifier name, and rename the list request/response message types from
pool-based names to token-based names. Keep watchedKeys and knownTokens behavior
unchanged.

In `@apps/api/src/workers/queue-handler.ts`:
- Around line 74-97: Update the token-processing loop around hub.fetch so the
Durable Object notification uses a bounded timeout via an AbortSignal, failing
fast if the hub stalls. Preserve the existing per-token error handling and
price/KV updates, while ensuring a timed-out notification does not block
subsequent tokens for the full queue message.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4f6db915-50ac-457a-8a1d-57b74840bd7a

📥 Commits

Reviewing files that changed from the base of the PR and between 5e8d65a and 9eb47a0.

📒 Files selected for processing (24)
  • apps/api/README.md
  • apps/api/migrations/0003_chain_scoped_tokens.sql
  • apps/api/src/db/queries.ts
  • apps/api/src/db/schema.ts
  • apps/api/src/durable-objects/websocket-hub-do.ts
  • apps/api/src/index.ts
  • apps/api/src/routes/tokens.ts
  • apps/api/src/services/chain-state-reader.ts
  • apps/api/src/services/quote-engine.ts
  • apps/api/src/services/swap.service.ts
  • apps/api/src/services/token-list.service.ts
  • apps/api/src/services/token.service.ts
  • apps/api/src/workers/cron-handler.ts
  • apps/api/src/workers/queue-handler.ts
  • apps/api/test/services/token-list.test.ts
  • apps/api/test/ws-routing.test.ts
  • apps/web/src/components/PriceTicker.tsx
  • apps/web/src/components/TokenSearch.tsx
  • apps/web/test/components/TokenSearch.test.tsx
  • packages/contracts/src/hook/AetherHook.sol
  • packages/contracts/test/fuzz/AetherHookInvariants.t.sol
  • packages/contracts/test/unit/AetherHook.t.sol
  • packages/contracts/test/unit/AetherHookTwap.t.sol
  • packages/shared/src/schema.ts

Comment thread apps/api/migrations/0003_chain_scoped_tokens.sql Outdated
Comment thread apps/api/src/routes/tokens.ts Outdated
Comment on lines +79 to +86
// Refresh verified tokens every 5 minutes — scoped to this chain: tokens are keyed
// by (chain_id, address) and another chain's tokens must not be refreshed here.
const chainId = Number.parseInt(env.CHAIN_ID, 10)
const tokens = await env.DB.prepare("SELECT address FROM tokens WHERE is_verified = 1 AND chain_id = ? LIMIT 200")
.bind(Number.isNaN(chainId) ? 1 : chainId)
.all<{
address: string
}>()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file outline =="
ast-grep outline apps/api/src/workers/cron-handler.ts --view expanded || true

echo "== relevant lines =="
cat -n apps/api/src/workers/cron-handler.ts | sed -n '1,180p'

echo "== git diff stat =="
git diff --stat || true

echo "== diff for file =="
git diff -- apps/api/src/workers/cron-handler.ts | sed -n '1,260p' || true

echo "== Effect imports/services in workers/api layer =="
rg -n "Effect\.catch|Class\.Service|Context\.Service|Layer\.effect|`@effect`|env\.DB\.prepare|`@hono/effect`" apps/api/src/workers apps/api/src -g '*.ts' | head -n 200 || true

Repository: irfndi/AetherDEX

Length of output: 11067


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== SQL client/service examples =="
for f in apps/api/src/db/client.ts apps/api/src/services/pool.service.ts apps/api/src/services/token.service.ts apps/api/src/services/position.service.ts apps/api/src/services/swap.service.ts; do
  echo "--- $f"
  wc -l "$f"
  sed -n '1,180p' "$f"
done

echo "== route imports and environment binding =="
rg -n "CronEnv|handleScheduled|WorkerEnv|Db|Layer.effect|Context.Service<.*Db>|Service<.*Db>" apps/api/src -g '*.ts' | sed -n '1,260p'

Repository: irfndi/AetherDEX

Length of output: 24938


Move the cron refresh queries into Effect services.

refreshTopPools() and enqueuePriceRefresh() still use raw env.DB.prepare(...)/env.PRICE_QUEUE.send(...) in a non-Effect worker implementation. These are backend data operations, so move them behind Db/WorkerEnv Effect services via Layer.provide and structured error handling.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/src/workers/cron-handler.ts` around lines 79 - 86, Move the database
and queue operations in refreshTopPools() and enqueuePriceRefresh() behind the
existing Db and WorkerEnv Effect services, replacing direct env.DB.prepare(...)
and env.PRICE_QUEUE.send(...) usage. Run these Effect workflows with
Layer.provide, and preserve failures through structured Effect error handling
rather than unhandled raw promise errors.

Source: Coding guidelines

Comment thread apps/api/test/services/token-list.test.ts
- migration 0003: rebuild FK dependents children-first (transactions,
  liquidity_positions, pools) before dropping tokens, so the table swap
  cannot fail under SQLite foreign-key enforcement; PRAGMA guards +
  foreign_key_check; verified with d1 migrations apply --local
- token.service: scope every tokens-table read to the current chain via
  TokenServiceDeps { chainId } (getToken can no longer pick another
  chain's row nondeterministically)
- queue-handler: only broadcast prices > 0 — a failed CoinGecko lookup
  (0 sentinel) must not overwrite a ticker's last good value with $0
- tokens route + token-list service: treat negative limit as invalid
  (default 100) — slice(0, -n) would silently drop trailing entries
- tests: timestamp-stability test proves the cache path via fetcher.calls;
  negative-limit regression; TokenServiceDeps provided in storage tests

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/api/migrations/0003_chain_scoped_tokens.sql`:
- Around line 18-20: Update the rebuilt transactions and liquidity_positions
schemas to restore foreign keys from pool_id to pools(pool_id), while retaining
the existing users(address) foreign keys. If necessary, stage child data during
the pools rebuild, then ensure the final table materialization enforces both
relationships and passes foreign_key_check.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ac73d74c-2353-4fd1-a5c4-f81a87aa6d88

📥 Commits

Reviewing files that changed from the base of the PR and between 9eb47a0 and fd3088a.

📒 Files selected for processing (7)
  • apps/api/migrations/0003_chain_scoped_tokens.sql
  • apps/api/src/routes/tokens.ts
  • apps/api/src/services/token-list.service.ts
  • apps/api/src/services/token.service.ts
  • apps/api/src/workers/queue-handler.ts
  • apps/api/test/services/storage-errors.test.ts
  • apps/api/test/services/token-list.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • apps/api/src/routes/tokens.ts
  • apps/api/src/workers/queue-handler.ts
  • apps/api/test/services/token-list.test.ts
  • apps/api/src/services/token-list.service.ts

Comment thread apps/api/migrations/0003_chain_scoped_tokens.sql

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd3088aa88

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +91 to +96
await hub.fetch(
new Request("http://price-hub/price", {
method: "POST",
body: JSON.stringify({ tokenAddress: token, price: priceUsd, updatedAt }),
}),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Rehydrate hibernating WebSocket subscribers before broadcasting

When the hub hibernates between the five-minute refreshes, this POST wakes a new Durable Object instance whose in-memory subscribers map is empty, although the sockets accepted through ctx.acceptWebSocket remain connected. The broadcast is therefore sent to nobody and existing PriceTicker clients stop receiving updates after hibernation; reconstruct subscribers from ctx.getWebSockets() (and persisted attachments for watched keys) before fan-out.

AGENTS.md reference: AGENTS.md:L104-L105

Useful? React with 👍 / 👎.

block_timestamp INTEGER NOT NULL,
status TEXT NOT NULL DEFAULT 'pending', -- 'pending' | 'confirmed' | 'failed'
created_at INTEGER NOT NULL,
FOREIGN KEY (user_address) REFERENCES users(address)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve pool foreign keys after rebuilding dependents

After this migration, transactions.pool_id and liquidity_positions.pool_id permanently lose the foreign keys present in 0001_initial_schema.sql; only the user foreign key is recreated. Consequently, future inserts can reference nonexistent pools and deleting a pool can leave orphaned transactions and positions, while the final foreign_key_check cannot detect these future violations. Rebuild the dependent tables again with FOREIGN KEY (pool_id) REFERENCES pools(pool_id) after the parent rebuild is complete.

Useful? React with 👍 / 👎.

…an-out

- migration 0003 (R2 follow-up): second pass rebuilds transactions and
  liquidity_positions AFTER pools exists again, restoring their
  FOREIGN KEY (pool_id) REFERENCES pools(pool_id) — the FKs are only
  dropped mid-swap, not permanently; final schema keeps the full
  integrity graph minus the intentionally retired pools->tokens(address)
  FK (tokens PK is now (chain_id, address))
- websocket-hub-do: rehydrate the in-memory subscriber map from
  ctx.getWebSockets() before broadcasts and message handling, so price
  fan-out survives DO hibernation between refreshes (sockets stay
  connected but the map does not); watched keys are persisted in the
  socket attachment and restored on hydration

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ab2116b61

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


// The hub broadcasts ALL token prices (it is a single shared instance); keep the
// latest payload for THIS ticker's token, unwrapped out of the frame envelope.
const [data, setData] = useState<PricePayload | null>(null)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear the retained price when the token changes

When the chart route is reused with a different tokenAddress, this local state retains the previous token's payload; the effect ignores the old envelope because it no longer matches, but never resets data. The ticker therefore displays the old token's price until the new token receives its next refresh, potentially several minutes later. Reset the retained payload when tokenAddress or chainId changes.

AGENTS.md reference: AGENTS.md:L18-L18

Useful? React with 👍 / 👎.

);
INSERT OR IGNORE INTO tokens_chain_scoped
(chain_id, address, symbol, name, decimals, logo_url, is_verified, is_native, total_supply, created_at, updated_at)
SELECT 1, address, symbol, name, decimals, logo_url, is_verified, is_native, total_supply, created_at, updated_at

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the deployed chain when migrating token rows

This rewrites every existing token as chain 1, although the repository's default and staging deployments use Sepolia (CHAIN_ID=11155111). When upgrading a Sepolia D1 database with cached rows, those rows become invisible to the newly chain-filtered Sepolia reads, so the D1 fallback fails during an upstream/KV outage; if the database is later read as mainnet, the mislabeled metadata can also be served for the wrong chain. The migration must assign the source database's actual chain or deliberately discard and repopulate this cache rather than hard-code mainnet.

AGENTS.md reference: AGENTS.md:L24-L24

Useful? React with 👍 / 👎.

@irfndi
irfndi merged commit fa19dc6 into main Jul 23, 2026
9 of 16 checks passed
@irfndi
irfndi deleted the fix/phase0-review-followup branch July 23, 2026 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant