diff --git a/docs/http-status-codes.md b/docs/http-status-codes.md new file mode 100644 index 00000000..d3dbccd8 --- /dev/null +++ b/docs/http-status-codes.md @@ -0,0 +1,63 @@ +# HTTP Status Code Conventions + +This document defines the consistent rules for HTTP status codes across all Lens API routes. + +## The Rule + +**404 Not Found** is returned when: +- A **resource does not exist** (the requested entity is not configured, not registered, or fundamentally absent) +- Examples: pair not watched, API key not found, webhook subscription ID not found + +**200 OK with empty result** is returned when: +- A **query matched nothing** (the resource exists and the query was valid, but no data satisfied the filters/time range) +- Examples: no price data in the requested time window, no trades in the last 24h, no pools for the given asset + +**400 Bad Request** is returned when: +- Request parameters are malformed, out of range, or violate validation rules +- Examples: invalid date format, unsupported window value, negative amount + +## Application per Route + +### Routes returning 404 for non-existent resources + +| Route | 404 Condition | +|---|---| +| `GET /price/:assetA/:assetB` | Pair not watched on the requested network | +| `GET /price/:assetA/:assetB/route` | Pair not watched on the requested network | +| `GET /price/:assetA/:assetB/depth` | Pair not watched on the requested network | +| `GET /benchmark/:asset` | Pair not watched on the requested network | +| `DELETE /admin/keys/:id` | API key ID not found | +| `DELETE /webhooks/:id` | Already 204 (idempotent deletion) | + +### Routes returning 200 with empty result + reason + +| Route | Empty Result Condition | Response Shape | +|---|---|---| +| `GET /basket` | One or more assets have no price data | `{ error: "No price data found for: ASSET1, ASSET2" }` **→ Changed to 200** | +| `GET /compare/:asset` | No Lens or Reflector price available | `{ lens: null, reflector: null, status: "no_data", ... }` **→ Changed to 200** | +| `GET /candles/:assetA/:assetB` | No trades in the requested time range | `{ candles: [], pairKey, interval, from, to }` | +| `GET /price/:assetA/:assetB/history` | No aggregated buckets for the window | `{ buckets: [], pairKey, window }` | +| `GET /spreads/:asset` | No venue quoted the asset in the window | `{ venues: [], tightest: null, ... }` | +| `GET /volumes/:asset` | No trades for the asset in the window | `{ totalVolume: 0, byVenue: {}, venues: [], tradeCount: 0, ... }` | +| `GET /screener` | No pairs matched the filters | `{ data: [], nextCursor: null, hasMore: false, total: null }` | +| `GET /pools` | No pools for the network | `{ pools: [] }` | +| `GET /pairs` | No watched pairs (configuration issue) | `{ pairs: [] }` | + +### Rationale + +The distinction preserves semantic information that a single status code cannot carry: + +- **404** tells the client "you asked for something that cannot exist — check your configuration or endpoint" +- **200 + empty** tells the client "your request was valid but there is no data right now — try again later or widen your filters" + +A client that retries a 404 without changing its request is wasting resources. A client that retries a 200-empty after waiting or adjusting filters is behaving correctly. + +### Migration notes + +Routes that **changed** from 404 to 200-empty: +- `/basket` — was 404 when an asset had no price; now 200 with `{ error: ... }` listing the missing assets +- `/compare/:asset` — was 404 when both sources were null; now 200 with `status: "no_data"` + +Routes that were **already correct** and unchanged: +- `/candles`, `/price/*/history`, `/spreads`, `/volumes`, `/screener`, `/pools`, `/pairs` — all return 200 with empty arrays +- `/price`, `/price/*/route`, `/price/*/depth`, `/benchmark` — all return 404 for unwatched pairs diff --git a/openapi.json b/openapi.json index b889418f..93921d14 100644 --- a/openapi.json +++ b/openapi.json @@ -1374,6 +1374,7 @@ "get": { "operationId": "compareAsset", "summary": "Compare Lens prices with every configured external oracle", + "description": "Returns 200 even when both Lens and all configured oracles have no data for the asset. The `status` field distinguishes \"no_data\" from \"aligned\", \"minor_deviation\", \"major_deviation\" and \"partial\" (one source only). The response includes both the legacy `reflector` field for backward compatibility and an `oracles` array listing every configured source.", "security": [], "parameters": [ { @@ -1391,7 +1392,7 @@ ], "responses": { "200": { - "description": "Price comparison", + "description": "Price comparison (includes status:\"no_data\" when both sources are null)", "headers": { "Cache-Control": { "$ref": "#/components/headers/CacheControl" @@ -1405,9 +1406,6 @@ } } }, - "404": { - "description": "No price data found for the asset" - }, "429": { "$ref": "#/components/responses/TooManyRequests" } @@ -1605,6 +1603,7 @@ "get": { "operationId": "getBasket", "summary": "Compute the weighted price of a basket of assets", + "description": "Returns 200 even when one or more assets have no price data. The response body will contain an `error` field listing the missing assets and `reason: \"missing_price_data\"` rather than computing a partial basket price. A 404 would imply the assets themselves are invalid.", "security": [], "parameters": [ { @@ -1646,7 +1645,7 @@ ], "responses": { "200": { - "description": "Basket price", + "description": "Basket price or error with reason when price data is missing", "headers": { "Cache-Control": { "$ref": "#/components/headers/CacheControl" @@ -1663,9 +1662,6 @@ "400": { "description": "Missing assets or mismatched/invalid weights" }, - "404": { - "description": "No price data for one or more assets" - }, "429": { "$ref": "#/components/responses/TooManyRequests" } diff --git a/openapi.yaml b/openapi.yaml index c96dd10a..5f45a023 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -896,6 +896,12 @@ paths: get: operationId: compareAsset summary: Compare Lens prices with every configured external oracle + description: >- + Returns 200 even when both Lens and all configured oracles have no data + for the asset. The `status` field distinguishes "no_data" from "aligned", + "minor_deviation", "major_deviation" and "partial" (one source only). + The response includes both the legacy `reflector` field for backward + compatibility and an `oracles` array listing every configured source. security: [] parameters: - name: asset @@ -907,7 +913,7 @@ paths: - $ref: '#/components/parameters/NetworkQuery' responses: '200': - description: Price comparison + description: Price comparison (includes status:"no_data" when both sources are null) headers: Cache-Control: $ref: '#/components/headers/CacheControl' @@ -915,8 +921,6 @@ paths: application/json: schema: $ref: '#/components/schemas/CompareResponse' - '404': - description: No price data found for the asset '429': $ref: '#/components/responses/TooManyRequests' @@ -1041,6 +1045,11 @@ paths: get: operationId: getBasket summary: Compute the weighted price of a basket of assets + description: >- + Returns 200 even when one or more assets have no price data. The + response body will contain an `error` field listing the missing assets + and `reason: "missing_price_data"` rather than computing a partial + basket price. A 404 would imply the assets themselves are invalid. security: [] parameters: - name: asset @@ -1067,7 +1076,7 @@ paths: example: [0.6, 0.4] responses: '200': - description: Basket price + description: Basket price or error with reason when price data is missing headers: Cache-Control: $ref: '#/components/headers/CacheControl' @@ -1077,8 +1086,6 @@ paths: $ref: '#/components/schemas/BasketResponse' '400': description: Missing assets or mismatched/invalid weights - '404': - description: No price data for one or more assets '429': $ref: '#/components/responses/TooManyRequests' diff --git a/src/__tests__/basket.test.ts b/src/__tests__/basket.test.ts index cc81161f..e7576403 100644 --- a/src/__tests__/basket.test.ts +++ b/src/__tests__/basket.test.ts @@ -131,7 +131,7 @@ describe('GET /basket', () => { await app.close() }) - it('returns 404 when an asset has no price data', async () => { + it('returns 200 with error and reason when an asset has no price data', async () => { mockQuery.mockImplementation(async (_sql: string, params: string[]) => { const asset = params[0] if (asset === 'USDC') return { rows: [{ vwap: '1.0' }] } @@ -143,7 +143,12 @@ describe('GET /basket', () => { method: 'GET', url: '/basket?asset=USDC&weight=1&asset=UNKNOWN&weight=1', }) - expect(res.statusCode).toBe(404) + // Changed from 404 to 200 per issue #262: the asset code is valid, the + // query succeeded, there is just no data. A 404 would imply the endpoint + // or the asset itself doesn't exist. + expect(res.statusCode).toBe(200) expect(res.json().error).toMatch(/UNKNOWN/) + expect(res.json().reason).toBe('missing_price_data') + expect(res.json().missingAssets).toContain('UNKNOWN') }) }) diff --git a/src/api/rest.ts b/src/api/rest.ts index 2f5fbdce..ad66fca9 100644 --- a/src/api/rest.ts +++ b/src/api/rest.ts @@ -146,10 +146,12 @@ export async function registerRESTRoutes(app: FastifyInstance) { async (req, reply) => { const { assetA, assetB } = req.params const amount = parseFloat(req.query.amount ?? '1000') + // Validate amount before the pair check: a malformed request parameter + // is a 400 regardless of whether the pair is watched. + if (isNaN(amount) || amount <= 0) return reply.status(400).send({ error: 'amount must be a positive number' }) const network = req.network ?? activeNetwork const pair = findPair(assetA, assetB, network) if (!pair) return reply.status(404).send({ error: `Pair ${assetA}/${assetB} not watched on ${network}` }) - if (isNaN(amount) || amount <= 0) return reply.status(400).send({ error: 'amount must be a positive number' }) return getBestRoute(pair.assetA, pair.assetB, pair.pairKey, amount, network) } diff --git a/src/routes/basket.ts b/src/routes/basket.ts index 4304326f..27616cbd 100644 --- a/src/routes/basket.ts +++ b/src/routes/basket.ts @@ -87,7 +87,14 @@ export async function registerBasketRoutes(app: FastifyInstance) { const missingAssets = components.filter((c) => c.price === null).map((c) => c.asset) if (missingAssets.length > 0) { - return reply.status(404).send({ error: `No price data found for: ${missingAssets.join(', ')}` }) + // 200 with reason: query was valid but no price data exists (yet). + // A 404 would imply the assets themselves are invalid/unwatched. + return { + error: `No price data found for: ${missingAssets.join(', ')}`, + reason: 'missing_price_data', + missingAssets, + requestedAt: new Date().toISOString(), + } } const basketPrice = components.reduce( diff --git a/src/routes/oracle.ts b/src/routes/oracle.ts index 5dfda971..416a660e 100644 --- a/src/routes/oracle.ts +++ b/src/routes/oracle.ts @@ -21,8 +21,21 @@ export async function registerOracleRoutes( const network: NetworkName = requestedNetwork ?? activeNetwork const comparison = await compareOracles(assetCode, network, adapters) + // CHANGE FROM UPSTREAM (#262): Return 200 with status:"no_data" instead of 404 + // when both Lens and all oracles have no data. The asset code itself is valid, + // the query succeeded — there is just no data yet. A 404 would imply the asset + // is invalid or the endpoint does not exist. if (!comparison) { - return reply.status(404).send({ error: `No price data found for asset ${assetCode}` }) + return { + asset: assetCode, + network, + lens: null, + reflector: null, + deviationPct: null, + status: 'no_data', + fetchedAt: new Date().toISOString(), + oracles: [], + } } return comparison } diff --git a/tests/httpStatusCodes.test.ts b/tests/httpStatusCodes.test.ts new file mode 100644 index 00000000..bde26d7c --- /dev/null +++ b/tests/httpStatusCodes.test.ts @@ -0,0 +1,376 @@ +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest' +import type { FastifyInstance } from 'fastify' + +// --------------------------------------------------------------------------- +// Mock DB / Redis / Oracle before any app code is imported. +// The full buildApp() wires real routes that hit pgPool / prisma / Redis — +// none of which are available in CI. Each mock returns "empty but valid" data +// so every route that should return 200-with-empty-result does exactly that, +// and routes whose 4xx behavior is driven by input validation (not DB) still +// return the correct code regardless of these stubs. +// --------------------------------------------------------------------------- + +const { mockPgQuery, mockPrismaWebhookDelete, mockPrismaApiKeyFindUnique } = vi.hoisted(() => ({ + mockPgQuery: vi.fn(), + mockPrismaWebhookDelete: vi.fn(), + mockPrismaApiKeyFindUnique: vi.fn(), +})) + +vi.mock('../src/db', () => ({ + pgPool: { query: mockPgQuery }, + prisma: { + webhook: { + create: vi.fn(), + delete: mockPrismaWebhookDelete, + }, + apiKey: { + create: vi.fn(), + findUnique: mockPrismaApiKeyFindUnique, + update: vi.fn(), + }, + }, +})) + +vi.mock('../src/redis', () => ({ + redis: { + get: vi.fn().mockResolvedValue(null), + set: vi.fn().mockResolvedValue('OK'), + del: vi.fn().mockResolvedValue(1), + on: vi.fn(), + subscribe: vi.fn(), + publish: vi.fn(), + duplicate: vi.fn().mockReturnValue({ + subscribe: vi.fn(), + on: vi.fn(), + }), + }, + getCachedPrice: vi.fn().mockResolvedValue(null), + setCachedPrice: vi.fn(), +})) + +vi.mock('../src/aggregator/vwap', () => ({ + getAggregatedPrice: vi.fn().mockResolvedValue({ + price: 0, + ammPrice: null, + sdexPrice: null, + vwap1m: null, + vwap1h: null, + volume24h: 0, + priceChange24h: null, + }), +})) + +vi.mock('../src/aggregator/bestRoute', () => ({ + getBestRoute: vi.fn().mockResolvedValue({ + route: 'sdex', + sdexPrice: null, + ammPrice: null, + estimatedOutput: 0, + slippagePct: 0, + recommendation: 'sdex', + }), +})) + +vi.mock('../src/pricing/depth', () => ({ + getDepth: vi.fn().mockResolvedValue({ + levels: [], + totalLiquidity: 0, + slippage: 0, + }), +})) + +// Reflector oracle: mark as not configured so /compare returns no_data +vi.mock('../src/ingest/oracles/reflector', () => ({ + reflectorOracleAdapter: { + name: 'reflector', + isConfigured: () => false, + getPrice: vi.fn(), + }, +})) + +import { buildApp } from '../src/app' + +/** + * Tests for issue #262: standardized HTTP status code behavior across all routes. + * + * The rule: + * - 404 when a resource does not exist (pair not watched, key not found) + * - 200 with empty result when a query matched nothing (no data in time window) + */ + +describe('HTTP status code standardization (#262)', () => { + let app: FastifyInstance + + beforeAll(async () => { + process.env.STELLAR_NETWORK = 'testnet' + process.env.WATCHED_PAIRS = 'XLM:native,USDC:GBBD47IF6LWK7P7MDEVSCWR7DPUWV3NY3DTQEVFL4NAT4AQH3ZLLFLA5' + process.env.REQUIRE_API_KEY = 'false' // Disable API key auth for tests + // Set ADMIN_TOKEN so admin routes don't reject with 401 before reaching 404 + process.env.ADMIN_TOKEN = 'test-admin-token' + + // Default: all DB queries return empty result sets + mockPgQuery.mockResolvedValue({ rows: [] }) + // Default: webhook delete does nothing (P2025 = record not found, idempotent) + mockPrismaWebhookDelete.mockRejectedValue({ code: 'P2025' }) + // Default: API key lookup returns null (not found) + mockPrismaApiKeyFindUnique.mockResolvedValue(null) + + app = await buildApp() + await app.ready() + }) + + afterAll(async () => { + await app?.close() + }) + + describe('404 for non-existent resources', () => { + it('GET /price/:assetA/:assetB returns 404 when pair is not watched', async () => { + const res = await app.inject({ + method: 'GET', + url: '/price/XLM/NONEXISTENT', + }) + expect(res.statusCode).toBe(404) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body.error).toMatch(/not watched/) + }) + + it('GET /price/:assetA/:assetB/route returns 404 when pair is not watched', async () => { + const res = await app.inject({ + method: 'GET', + url: '/price/XLM/NONEXISTENT/route?amount=100', + }) + expect(res.statusCode).toBe(404) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body.error).toMatch(/not watched/) + }) + + it('GET /price/:assetA/:assetB/depth returns 404 when pair is not watched', async () => { + const res = await app.inject({ + method: 'GET', + url: '/price/XLM/NONEXISTENT/depth?amount=100', + }) + expect(res.statusCode).toBe(404) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body.error).toMatch(/not watched/) + }) + + it('GET /benchmark/:asset returns 404 when pair is not watched', async () => { + const res = await app.inject({ + method: 'GET', + url: '/benchmark/NONEXISTENT?target=USD', + }) + expect(res.statusCode).toBe(404) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body.error).toMatch(/not watched/) + }) + + it('DELETE /admin/keys/:id returns 404 when API key not found', async () => { + const res = await app.inject({ + method: 'DELETE', + url: '/admin/keys/00000000-0000-0000-0000-000000000000', + headers: { + 'x-admin-token': 'test-admin-token', + }, + }) + expect(res.statusCode).toBe(404) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body.error).toMatch(/not found/) + }) + }) + + describe('200 with empty result when query matched nothing', () => { + it('GET /basket returns 200 with error and reason when assets have no price data', async () => { + // basket route uses pgPool to get VWAP — return null vwap (no data) + mockPgQuery.mockResolvedValue({ rows: [{ vwap: null }] }) + const res = await app.inject({ + method: 'GET', + url: '/basket?asset=FAKEASSET1&asset=FAKEASSET2&weight=0.5&weight=0.5', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body).toHaveProperty('reason', 'missing_price_data') + expect(body).toHaveProperty('missingAssets') + expect(Array.isArray(body.missingAssets)).toBe(true) + }) + + it('GET /compare/:asset returns 200 with status:"no_data" when both sources are null', async () => { + // pgPool returns no vwap rows; reflector oracle is not configured (mocked above) + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/compare/NONEXISTENTASSET', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('status', 'no_data') + expect(body).toHaveProperty('lens', null) + expect(body).toHaveProperty('reflector', null) + }) + + it('GET /candles/:assetA/:assetB returns 200 with empty candles array when no data exists', async () => { + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/candles/XLM/USDC?from=2020-01-01T00:00:00Z&to=2020-01-02T00:00:00Z', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('candles') + expect(Array.isArray(body.candles)).toBe(true) + // May be empty depending on whether fixture data exists in that range + }) + + it('GET /price/:assetA/:assetB/history returns 200 with empty buckets array when no data exists', async () => { + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/price/XLM/USDC/history?window=1m&limit=1', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('buckets') + expect(Array.isArray(body.buckets)).toBe(true) + }) + + it('GET /spreads/:asset returns 200 with empty venues array when no venue quoted the asset', async () => { + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/spreads/NONEXISTENTASSET?window=5m', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('venues') + expect(Array.isArray(body.venues)).toBe(true) + expect(body.venues).toHaveLength(0) + expect(body).toHaveProperty('tightest', null) + }) + + it('GET /volumes/:asset returns 200 with totalVolume:0 when no trades exist', async () => { + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/volumes/NONEXISTENTASSET?window=24h', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('totalVolume', 0) + expect(body).toHaveProperty('byVenue') + expect(Object.keys(body.byVenue)).toHaveLength(0) + expect(body).toHaveProperty('tradeCount', 0) + }) + + it('GET /screener returns 200 with empty data array when no pairs match filters', async () => { + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/screener?volume=9999999999999', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('data') + expect(Array.isArray(body.data)).toBe(true) + expect(body.data).toHaveLength(0) + expect(body).toHaveProperty('hasMore', false) + }) + + it('GET /pools returns 200 with empty pools array when no pools exist for the network', async () => { + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/pools?network=testnet', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('pools') + expect(Array.isArray(body.pools)).toBe(true) + // May be empty or populated depending on fixture data + }) + + it('GET /pairs returns 200 with empty pairs array when no pairs are watched', async () => { + // This scenario is unlikely in production but can occur during initial setup + mockPgQuery.mockResolvedValue({ rows: [] }) + const res = await app.inject({ + method: 'GET', + url: '/pairs', + }) + expect(res.statusCode).toBe(200) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('pairs') + expect(Array.isArray(body.pairs)).toBe(true) + }) + }) + + describe('400 for malformed requests (unchanged behavior)', () => { + it('GET /candles/:assetA/:assetB returns 400 for invalid interval', async () => { + const res = await app.inject({ + method: 'GET', + url: '/candles/XLM/USDC?interval=invalid', + }) + expect(res.statusCode).toBe(400) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + }) + + it('GET /basket returns 400 when assets and weights count mismatch', async () => { + const res = await app.inject({ + method: 'GET', + url: '/basket?asset=XLM&asset=USDC&weight=0.5', + }) + expect(res.statusCode).toBe(400) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + expect(body.error).toMatch(/must match/) + }) + + it('GET /spreads/:asset returns 400 for invalid window', async () => { + const res = await app.inject({ + method: 'GET', + url: '/spreads/XLM?window=invalid', + }) + expect(res.statusCode).toBe(400) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + }) + + it('GET /volumes/:asset returns 400 for invalid window', async () => { + const res = await app.inject({ + method: 'GET', + url: '/volumes/XLM?window=invalid', + }) + expect(res.statusCode).toBe(400) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + }) + + it('GET /price/:assetA/:assetB/route returns 400 for negative amount', async () => { + const res = await app.inject({ + method: 'GET', + url: '/price/XLM/USDC/route?amount=-100', + }) + expect(res.statusCode).toBe(400) + const body = JSON.parse(res.body) + expect(body).toHaveProperty('error') + }) + }) + + describe('DELETE /webhooks/:id is idempotent (204 even when not found)', () => { + it('returns 204 for non-existent webhook ID', async () => { + // prisma.webhook.delete throws P2025 (record not found) — should be treated + // as a success (idempotent delete) + mockPrismaWebhookDelete.mockRejectedValue({ code: 'P2025' }) + const res = await app.inject({ + method: 'DELETE', + url: '/webhooks/00000000-0000-0000-0000-000000000000', + }) + expect(res.statusCode).toBe(204) + expect(res.body).toBe('') + }) + }) +})