Skip to content

fix: schema hygiene and SQL parameterization (COW-997, COW-998)#81

Merged
lgahdl merged 9 commits into
developfrom
luizhatem/cow-997-998-schema-and-sql-hygiene
Jun 4, 2026
Merged

fix: schema hygiene and SQL parameterization (COW-997, COW-998)#81
lgahdl merged 9 commits into
developfrom
luizhatem/cow-997-998-schema-and-sql-hygiene

Conversation

@lgahdl
Copy link
Copy Markdown
Contributor

@lgahdl lgahdl commented Jun 1, 2026

Summary

Two small hygiene fixes from the grant review targeting dead schema and inconsistent SQL patterns.

Changes

  • COW-997 [F4] src/application/handlers/setup.ts — remove the cow_cache.orderbook_cache CREATE TABLE block. The table had 0 rows and 0 reads/writes anywhere in the codebase; the live persistent cache is cow_cache.order_uid_cache. Also updated the file-level doc comment which referenced the removed table.
  • COW-998 [F6] src/application/helpers/orderbookClient.ts — replace manual string interpolation ('${uid.replace(...)}' ) with sql.join(batch.map(uid => sql\${uid}`), sql`, `)ingetCachedUidStatuses`. Now consistent with the parameterized pattern used for cache writes in the same file.

How to Test

  1. pnpm typecheck — no errors
  2. pnpm test — all tests pass

Checklist

  • Tests pass locally
  • Linting passes
  • Documentation updated (if needed)
  • Breaking changes documented (if any)

Breaking Changes

None — orderbook_cache had no rows and no consumers. The sql.join produces semantically identical queries with bound parameters instead of interpolated strings.

Related Issues

Closes COW-997
Closes COW-998

- COW-997 [F4]: remove dead cow_cache.orderbook_cache table creation from
  setup.ts; table had 0 rows and 0 reads/writes — live cache is order_uid_cache
- COW-998 [F6]: replace string interpolation in getCachedUidStatuses with
  sql.join() parameterized query, consistent with cache write pattern in same file

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

COW-997

COW-998

@lgahdl lgahdl changed the base branch from main to develop June 2, 2026 13:52
@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

The SQL parameterization change in getCachedUidStatuses is the right fix. Replacing sql.raw(...) string-interpolation with sql.join(batch.map(uid => sql${uid}), sql, ) correctly delegates value binding to Drizzle/the driver so UIDs are passed as parameters, not interpolated literals — closing the injection risk that existed whenever a UID contained a single quote or other SQL metacharacter.

sql.join is part of Drizzle ORM's core tagged-template utilities and is available in the version Ponder 0.16.x bundles, so no compatibility concern there. The resulting SQL fragment slots directly into the WHERE ... IN (...) position in the surrounding sql...`` template, which is the supported usage pattern.

The legacy orderbook_cache table drop in setup.ts is a nice cleanup alongside this — good to remove dead DDL rather than let it accumulate. Approving.

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Review: SQL parameterization + schema hygiene

sql.join compatibility — confirmed safe

Checked drizzle-orm 0.41.0 (the installed version via pnpm, pinned transitively by ponder 0.16.2). sql.join exists on the sql tag function, and ponder re-exports sql directly from drizzle-orm (export { sql, … } from "drizzle-orm"). The generated query is WHERE chain_id = $1 AND order_uid IN ($2, $3, …) — fully parameterized, no string interpolation. The pattern is safe and correct.

setup.ts — removal of cow_cache.orderbook_cache

The legacy table creation is removed. Since this is inside CREATE TABLE IF NOT EXISTS, existing deployments where the table already exists are unaffected. New deployments won't create the table. Make sure there are no remaining references to cow_cache.orderbook_cache anywhere in the codebase before merging — a stale query against a non-existent table would throw at runtime with no warning.

# Quick check
grep -r "orderbook_cache" src/

Everything else in the diff is clean. No other concerns.

Comment on lines +486 to +490
const uidList = sql.join(batch.map((uid) => sql`${uid}`), sql`, `);
const rows = (await context.db.sql.execute(
sql.raw(
`SELECT order_uid, status, executed_sell_amount, executed_buy_amount
FROM cow_cache.order_uid_cache
WHERE chain_id = ${chainId} AND order_uid IN (${placeholders})`,
),
sql`SELECT order_uid, status, executed_sell_amount, executed_buy_amount
FROM cow_cache.order_uid_cache
WHERE chain_id = ${chainId} AND order_uid IN (${uidList})`,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would prefer something like:

const rows = await context.db.sql
  .select({
    orderUid: orderUidCache.orderUid,
    status: orderUidCache.status,
    executedSellAmount: orderUidCache.executedSellAmount,
    executedBuyAmount: orderUidCache.executedBuyAmount,
  })
  .from(orderUidCache)
  .where(
    and(
      eq(orderUidCache.chainId, chainId),
      inArray(orderUidCache.orderUid, batch),
    ),
  );

lgahdl and others added 8 commits June 4, 2026 10:48
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e is not a direct dep

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… add drizzle-orm as direct dep

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the remaining raw sql\` template in orderbookClient.ts with
.insert().onConflictDoUpdate() using the already-defined orderUidCacheTable.
Also drops the now-unused `sql` named import from ponder.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- blockHandler.ts: 3× sql\`${col} IS NULL\` → isNull(col); adds isNull import
- execution-summary.ts: sql\`SELECT … GROUP BY\` → .select().from().where().groupBy()
  using discreteOrder table and count() operator; drops Number() cast

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…able suffix)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Handler was converted from db.execute(sql\`GROUP BY\`) to
db.select().from().where().groupBy() with count() from ponder.
Update mocks accordingly: ponder:api now exposes db.select, ponder
exports and/eq/count, and StatusRow.count is number not string.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lgahdl lgahdl merged commit 1b2125b into develop Jun 4, 2026
4 checks passed
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.

2 participants