Repository navigation
feat: add lens_list_pairs, lens_get_pools and lens_screen_pairs tools - #482
Conversation
…bs#409) Agents could ask for a price but had no way to discover what is quotable, what liquidity sits behind a pair, or what is moving — REST answered all three while MCP answered none, leaving agents to guess asset codes. lens_list_pairs, lens_get_pools and lens_screen_pairs call the same listPairs/listPools/screenPairs the REST routes now delegate to, so the two transports cannot drift apart. Each result is bounded by a limit (max 100) and carries nextCursor/hasMore, assets always include the full issuer, and the tool descriptions spell out which question each one answers.
|
@Ugasutun Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Coordination note — ten PRs are editing one 179-line fileIf your MCP PR has just gone red with a conflict, this is why, and it is my fault rather than yours. I published 22 MCP issues (#404–#425) as if they were independent. They are not: almost all of them land in Already merged:
Order I intend to take the rest in, so you can rebase once rather than repeatedly:
Please do not all rebase at once. If you are below the one currently in flight, wait until the one above you lands — otherwise you rebase, I merge someone else, and you rebase again. I will comment on your PR when it is your turn. One thing to settle between you: Nothing here reflects on the quality of the work. Several of these are among the best PRs this repo has had. |
tsc --noEmit clean vitest 97 files, 1127 passing (up from 93 / 1068) dropped-tests none #325 GET /movers for market gainers and losers #327 GET /correlation for basket construction #485 net arbitrage spread against fees #494 declare the resources capability and expose pages over MCP Two fixes on merge. #325 shipped the route file and the OpenAPI entry but never registered it. The repo's own guard caught it: AssertionError: Documented but unregistered: ["GET /movers"] So the endpoint was documented, generated into clients, and did not exist. registerMoversRoutes already followed the house signature, so wiring it was an import and a call next to registerScreenerRoutes. That guard is worth its weight -- a documented route that 404s is worse than an undocumented one, because a client is generated against it. #327 added a console.error to src/routes/correlation.ts, which the logging hygiene test forbids. Now createLogger, the same change #388 and #318 needed in the previous batch. Three PRs in two batches have tripped that test, which suggests the rule is not discoverable enough from the outside -- worth a line in CONTRIBUTING rather than catching it per-PR. NOT merged, each failing on its own branch rather than from merge order: #460 Phoenix AMM adapter -- 5 failures alone, including its own networkVenueConfig and venues/registry tests #353 1 tsc error #354 1 tsc error #482 4 tsc errors #393 #431 #434 #472 conflict Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656
left a comment
There was a problem hiding this comment.
Screened against current main and this does not pass on its own branch, so a rebase alone will not fix it.
Merged alone it gives type errors against main. Run npx tsc --noEmit on a fresh rebase and you will see them.
Please rebase, get npx tsc --noEmit and npx vitest run both clean locally, and re-request. I would rather take this than not — it is held on correctness, not on wanting it.
Miracle656
left a comment
There was a problem hiding this comment.
The design is right and I want this in. It needs a rebase, because the screener was extracted on main in the week since you branched — differently from how you extracted it here, and in the same file.
The principle is the one I'd have asked for
* `lens_screen_pairs` — over the exact implementations the REST routes use
* (`listPairs`, `listPools`, `screenPairs`), so a page from a tool is a page
That is exactly right, and it is the thing MCP tool PRs usually get wrong: re-querying the database from the tool layer, so lens_screen_pairs and GET /screener slowly stop agreeing about what sortBy=liquidity or limit=500 means. Routing the tools through the same functions means there is one answer and one place to fix it.
Pulling /pools out of src/api/rest.ts into src/routes/pools.ts is also a genuine improvement independent of the tools — rest.ts has been a grab-bag for a while.
What changed under you
main now has src/services/screener.ts, exporting runScreener. It arrived with the GraphQL screener query (#609), which needed the same thing you need: one implementation behind more than one surface. src/routes/screener.ts is now a thin handler that validates with a Zod schema and calls it:
return await runScreener({ network, filters, sortBy, order, cursor, limit })Your branch extracts the same logic as screenPairs inside src/routes/screener.ts. So we have two extractions of one query, and git cannot reconcile them — merging main gives 266 conflicted lines across three regions in that file alone.
I stopped there rather than picking a winner by hand. This is the query behind published volume and ranking numbers, and a merge I resolve by choosing between two restructurings is exactly where a subtle wrong answer hides. You know what you intended; I would be guessing.
What the rebase needs
- Drop the
screenPairsextraction.src/routes/screener.tsonmainis already thin — leave it alone. - Point
lensTools.tsat the service:import { runScreener } from '../services/screener'. Note it issrc/services/, notsrc/routes/. - Check the argument shape. You currently go through
stringifyScreenerArgsbecause the route parsed strings.runScreenertakes typed values —{ network, filters: { volume, change_24h_min, ... }, sortBy, order, cursor, limit }— so the MCP tool can pass numbers straight through and that helper can go. The route still stringifies only because query parameters arrive as strings; a tool's arguments do not. - Keep
listPairsandlistPools. Neither exists onmain—/poolsis still inline inrest.tsandpairs.tsexports nolistPairs— so those extractions are yours and conflict with nothing conceptually. src/mcp/server.tsandsrc/api/rest.tshave one conflict region each, both small.
Two things worth knowing that landed while you were out, because they affect the tool surface:
- Limit clamping lives in
runScreener, not the route:Math.min(SCREENER_MAX_LIMIT, Math.max(1, limit ?? SCREENER_DEFAULT_LIMIT)). Passlimitthrough and the tool is clamped identically to REST and GraphQL for free. Do not re-clamp in the tool — one clamp or they drift. sortByandorderare allow-listed inside the service before being interpolated into the SQL, since a column name cannot be a bind parameter. That protection is inrunScreener, so going through it is also what keeps the tool from becoming an injection surface. Worth a test assertinglens_screen_pairswithsortBy: "price; DROP TABLE"is refused rather than passed along.
After the rebase: npx tsc --noEmit, npm test, and if you touch openapi.yaml at all, npm run openapi:gen plus npx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.ts — both are byte-compared in CI now.
Separately: #423 is closed and credited to you. You were right that the work was already on main — it went in with #487 (94bd8ea), which is why #490's branch was empty after the rebase. I checked it rather than taking it on trust, and the sigValidOnMainnet: false assertion in that test is the best thing in it; details on the issue.
Miracle656
left a comment
There was a problem hiding this comment.
Scanned clean. The design is right and better than I expected from the last round — I want to lead with that because the blocker here is entirely the base.
What you got right
screenPairs is a shared function that the route and lens_screen_pairs both call, so the MCP tool cannot drift from the HTTP surface. And it validates inside itself:
const sortBy = q.sortBy ?? 'volume'
if (!allowedSortFields.includes(sortBy)) {
return { ok: false, error: `sortBy must be one of: ${allowedSortFields.join(', ')}` }
}That is the correct place for it. A column name cannot be a bind parameter, so the allow-list is the only thing standing between sortBy and the SQL — and putting it in the shared function rather than in the route's zod schema is what stops the MCP path from being an injection surface the HTTP path is not. That was my main worry last time and you closed it properly.
The blocker: the branch is 306 commits behind main
$ git diff --shortstat origin/main L482 -- src
184 files changed, 1918 insertions(+), 29327 deletions(-)
Twenty-nine thousand lines of deletions. Not conflicts — deletions, because the branch predates the files. Among them:
| Deleted | Lines |
|---|---|
src/__tests__/auth.test.ts |
741 |
src/ingest/venues/blend.ts |
661 |
src/__tests__/phoenixIngester.test.ts |
657 |
src/ingest/venues/phoenix.ts |
581 |
src/__tests__/vwap.test.ts |
483 |
Two entire venue ingesters, the auth test suite, and much more. Merging this would undo months of work.
A rebase across 306 commits is genuinely unpleasant, and I would not blame you for starting a fresh branch from current main and moving your four files onto it — src/mcp/lensTools.ts, src/routes/pools.ts, and your changes to src/routes/pairs.ts and src/routes/screener.ts. The diff would be a tenth the size and far easier for both of us.
One thing to reconcile when you do
main has src/services/screener.ts with runScreener, which the REST route and the GraphQL screener query both use, and which carries its own ALLOWED_SORT_FIELDS and limit clamping. Your screenPairs is a second implementation of the same thing. Two screeners that both work is worse than one, because they drift silently — a filter fixed in one is wrong in the other for months before anyone notices.
Please fold the MCP tool onto runScreener rather than keeping screenPairs. You lose nothing: it already validates sortBy, it already clamps limit to [1, SCREENER_MAX_LIMIT], and going through it means your tool, the REST route and GraphQL answer identically by construction. If runScreener is missing something your tool needs, add it there and all three consumers get it.
A test asserting lens_screen_pairs with sortBy: "price; DROP TABLE users" is refused would be worth keeping either way — you have the guard, and that is the test that proves it stays.
main is green right now (tsc clean, 1,755 tests), so anything tsc or vitest reports after the rebase is yours.
|
Correction to my earlier review. I said this branch would delete 29,327 lines including both venue ingesters and the auth test suite. That was wrong, and the method was the problem: Measured properly:
What I said about the code stands and is the part worth your time: |
Main moved the MCP server on since this branch started: tools are advertised only where their data source exists, argument shapes are zod schemas, and every result passes a byte budget. The Divinity-Labs#409 tools now sit inside that architecture rather than beside it — lensTools.ts stays database-free and takes its reads from an injected adapter, page sizes come from the operator's MCP_* limits and are clamped instead of refused, and a page fragment no longer carries a cursor that would skip the rows it dropped. On the REST side /pools moved out of the api/rest aggregate into routes/pools.ts, next to /pairs and /screener, so GET /pools and lens_get_pools run one listPools and keep the decimal-string contract; decimalStrings.test.ts mounts the moved route, and the screener tool runs the service the route and GraphQL share.
main moved again while this branch was being rebased onto it, and this time it took the pagination question out of Divinity-Labs#409's hands: src/pagination.ts is now the shared keyset-cursor convention, and GET /pairs and GET /pools paginate on it — /pairs keyset on pairKey, /pools on the skip scan's seed, both default 50 and capped at 200, and /pairs rows gained the freshness fields (Divinity-Labs#243). The branch follows that shape instead of competing with it. listPairs and listPools are now the extracted bodies of those two handlers — same SQL, same cursor codec, same 400s — and the REST routes are thin wrappers that turn a query string into a page request. Each outcome reports the page size it actually honoured, which is what lets the MCP tools echo a truthful `limit` to an agent that asked for 500, and the screener adapter does the same for the tighter ceiling the screener keeps for itself. Harnesses that used to reach /pools through the api/rest aggregate now register the route module the way app.ts does.
The pushed branch had a merge of main on it whose conflicts were resolved by keeping the pre-pagination drafts of /pairs, /pools, the screener route and the MCP server — which is the tree that made CI fail with TS1005. Those four files are already rebuilt here against the pagination convention main settled on, so this merge takes this side and keeps the other side's history intact.
|
Merged as-is — no changes needed. Verified locally on a real merge into main: 0 files deleted, What made this easy to accept is that the three tools are built from the same functions the REST routes call — Sorry again for the bogus "deletes thousands of lines" note earlier in this PR — that was my two-dot diff mistake, not anything in your branch. |
…d net LP returns Ported rather than merged: #482 moved `/pools` out of `src/api/rest.ts` into `src/routes/pools.ts` and gave it keyset pagination, so the branch's edits landed on a handler that no longer exists. - The two window probes are now part of the paginated skip-scan query, as `LEFT JOIN LATERAL` so a pool with no snapshot and no trade in the window stays in the listing with null estimates instead of dropping off the page. - The computation moved to `src/pricing/lpReturns.ts`, a module with no `pg` import, because `src/mcp/lensTools.ts` is deliberately DB-free and needs the same function. `tests/mcp-lens.test.ts` asserts the MCP tool returns exactly the REST fields, so adding these to the route alone broke parity — they are now reported by `lens_get_pools` too, from the one implementation. - Estimates are plain numbers, not routed through `formatDecimal`. They come from a square root and an annualisation, so offering them as exact decimal strings would overstate what they are; `estimate: true` says so in the body. - Unknown inputs yield null, never zero: a pool with no trades in the window has an unmeasurable fee return, not a 0% one, and they rank differently. - `openapi.yaml` is the source, so the fields were added there and the json and TypeScript client regenerated. The sign convention is the subtle part and is now covered: `impermanentLossPct` is already negative, so `netReturn24hPct` adds it. A new test checks a pool whose price doubled (-5.72% divergence, 0.75% fees over 24h) and fails if that addition becomes a subtraction. Verified on the merge: tsc clean, readme:check clean, 1913 tests passed. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Closes #409
Summary
MCP exposed prices (L213) but nothing for discovery. An agent couldn't list
which pairs exist, see the liquidity behind them, or find what's moving, so it
had to guess asset codes. This PR adds three MCP tools that wrap the existing
/pairs,/poolsand/screenerlogic.Changes
lens_list_pairs,lens_get_poolsandlens_screen_pairsMCP tools./pairs,/poolsand/screenerintoshared service functions. The REST routes and MCP tools both call them, so
there is a single implementation and no duplicated queries.
with a
next_cursor/offsetfield for fetching the next page.lens_list_pairs: enumerate which pairs exist, e.g. to discover validasset codes before asking for a price.
lens_get_pools: inspect the liquidity behind a specific pair.lens_screen_pairs: find pairs by movement or criteria, e.g. top movers.Pagination
Pass the
next_cursorfrom a previous response ascursorto get the nextpage. When
next_cursorisnull, there are no more results. The parametersare documented in each tool's input schema and in
docs/mcp.md.Testing
assert the payloads match.
cursor yields the next page with no overlap.
GABC…XYZ).Checklist