From da62304470364b17923b9677c5b7749f1649c1ab Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:49:33 +0200 Subject: [PATCH 1/5] a5f7d881 - Document the `wait` naming convention for long-polling endpoints (#4498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Document the `wait` naming convention for long-polling endpoints An endpoint that blocks on purpose must carry `wait` as its own path segment, and an endpoint expected to answer quickly must not use the word anywhere in its path. Monitoring depends on it: the "Slowest requests" panel of the dfx-api-traces dashboard excludes routes matching `^.*/wait(/.*)?$` on http.route, because a long poll's duration measures how long a customer took to act rather than how long the API computed. Naming a blocking endpoint without `wait` turns it into a permanent latency outlier that masks real regressions; naming a fast endpoint with `wait` silently removes it from the latency table. * Tighten the wait-naming section after review Four corrections: - The rule said a fast endpoint must not use `wait` "anywhere in its path", while the same section states that only a complete `wait` segment matches. Reworded to "as a path segment" so the rule and its matching semantics agree. - "cancelled" -> "canceled": CONTRIBUTING.md requires American English spelling, and this was the only occurrence in the file. - Dropped the internal dashboard name, the concrete row cap and the reference to the infrastructure repository. This is a public repo; the rule stands on its own without naming internal monitoring assets. - Added a cross-reference under Naming Conventions, where URL routes are already covered, so the rule is discoverable from there. * List the blocking endpoints that carry no wait segment Review found the endpoint table incomplete: three further routes block on an external event without a `wait` segment. - `GET /v1/lnurlp/:id` polls for a pending payment via Util.poll, default 10 s and extendable by the caller through `params.timeout`. It shows up in latency monitoring with peaks around 20 s. - `GET /v1/node/:node/tx/:txId` and its `:mode` variant poll for a confirmation for up to 600 s. None of them can reasonably be renamed: the LNURL pay-request path is encoded into LNURLs already in circulation, and the node routes are admin-only and excluded from the API docs. They are now listed as explicit exemptions rather than left unmentioned, so the rule describes the codebase as it is instead of claiming a clean slate. Also corrects the resolution condition: for MULTIPLE-mode payment links the waiter resolves once a quote reaches the completion threshold, while the payment itself can stay Pending. * Make the wait segment the default and exemptions explicit The previous wording bound only new endpoints and closed the list of exemptions outright. That is the wrong shape: the segment is the standard, and an exemption stays possible — but only as an explicit, justified entry in the table. A blocking route that neither carries the segment nor appears in the table is now stated to be a defect, with the two acceptable fixes named (rename, or document). This keeps the rule enforceable without pretending the three existing exemptions can be legislated away. * Separate passive waiting from waiting on your own operation Review showed the definition was too broad. Five further routes match "response time governed by an external event" without carrying a wait segment: - GET /v1/lnurlp/tx/:id (waits for an on-chain confirmation) - PUT /v1/sell/paymentInfos/:id/confirm, authorization branch - PUT /v1/swap/paymentInfos/:id/confirm, authorization branch - PUT /v1/realunit/sell/:id/confirm, eip7702 branch - PUT /v1/realunit/transfer/:id/confirm Listing them as exemptions would have been the wrong fix. They differ in kind from a long poll: each starts an operation and then waits for that operation to complete, so the duration measures work the API set in motion. That is exactly the signal latency monitoring should surface — PUT /v1/realunit/sell/:id/confirm has been observed at 47 s. Hiding it would defeat the purpose. The rule now says so explicitly: `wait` is for endpoints that do nothing but wait for another actor, and endpoints awaiting their own operation must not carry the segment. * Scope the defect wording to passive waiting only Review found an internal contradiction: the section first states that endpoints awaiting their own operation must not be named `wait`, then declares unconditionally that "a blocking route that is neither named `wait` nor listed here is a defect". Read literally, that reclassified the five routes just legitimized two paragraphs earlier as undocumented defects. Both the rule sentence and the monitoring bullet now say "passively waiting" instead of "blocking", and the rule states explicitly that routes awaiting their own operation need no table entry. * Reclassify GET /v1/lnurlp/tx/:id as a passive waiter Review caught a misclassification. The route was listed as "waits for its own operation", but the payment request tells the payer to broadcast the transaction themselves and send back only the hash (payment-request.mapper.ts): "Broadcast the signed transaction to the blockchain and send the transaction hash back via the endpoint". The API then polls for a confirmation of a transaction it never sent — passive waiting on an external actor, the same class as the other exemptions. Its `hex` branch does broadcast, but returns right after txInMempool without awaiting confirmation, so it is not a long poll at all. The route therefore moves into the exemption table. Like the LNURL pay-request path, it cannot be renamed: the URL is handed to the payer inside the payment request. * Correct the hex-branch note for the ICP case The exemption row claimed the `hex` branch "broadcasts and returns immediately — it does not wait". That holds for the EVM and Firo paths, but the ICP sub-branch waits for the payer's allowance first (payment-quote.service.ts:711, Util.retry with 3 attempts at 2 s) before broadcasting. The classification of the route is unaffected — it stays a passive waiter and an exemption — but the stated reason was inaccurate. --- CONTRIBUTING.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 862168563f..617add0f93 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,6 +69,7 @@ Missing any of these = changes requested. - **Boolean flags: positive naming**: `safetyModeActive` not `safetyModuleInactive` - **Short, descriptive names — no redundant prefixes**: `uid` not `transactionRequestUid`, `balances` not `assetBalances`, `txId` not `transactionId` - **Variable names must precisely reflect the data**: `priceChf` not `amountChf` for a price +- **`wait` is reserved for long-polling routes**: see [Long-Polling Endpoints Must Be Named `wait`](#long-polling-endpoints-must-be-named-wait) — the segment is load-bearing for latency monitoring ### Methods @@ -965,6 +966,36 @@ Keep old endpoints for backward compatibility but annotate: @ApiOperation({ deprecated: true }) ``` +### Long-Polling Endpoints Must Be Named `wait` + +An endpoint that does nothing but wait for someone else to act — its response time is determined solely by when another request or process triggers the event, and it performs no work of its own meanwhile — **must** carry `wait` as its own path segment, unless it is listed as an explicit exemption below. Conversely, an endpoint expected to answer quickly **must not** use `wait` as a path segment. + +This does **not** cover an endpoint that starts an operation and then waits for it to finish — broadcasting a transaction and awaiting its confirmation, for example. That duration reflects work the API itself set in motion, which makes it a legitimate monitoring signal, so those endpoints stay visible and must **not** be named `wait`. Current examples: `PUT /v1/sell/paymentInfos/:id/confirm` and `PUT /v1/swap/paymentInfos/:id/confirm` (both in their `authorization` branch), `PUT /v1/realunit/sell/:id/confirm` (`eip7702` branch) and `PUT /v1/realunit/transfer/:id/confirm`. + +Endpoints that block by design: + +| Path | Blocks until | `wait` segment | +| ------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------- | +| `GET /v1/lnurlp/wait/:id` | the payment resolves: completed, canceled, expired — or, for `MULTIPLE`-mode links, when a quote reaches the configured completion threshold (the payment itself may stay `Pending`) | yes | +| `GET /v1/paymentLink/payment/wait` | the same, for the authenticated payment-link flow | yes | +| `GET /v1/lnurlp/:id` | a pending payment appears; bounded by `timeout` (default 10 s, caller-controllable) | no — exempt | +| `GET /v1/lnurlp/tx/:id` | the payer's own broadcast reaches one confirmation (`tx` branch); 15 polls at 1 s. The `hex` branch broadcasts without awaiting confirmation, except on ICP, where it first waits for the payer's allowance (3 retries at 2 s) | no — exempt | +| `GET /v1/node/:node/tx/:txId` | the transaction reaches one confirmation; bounded at 600 s | no — exempt | +| `GET /v1/node/:node/:mode/tx/:txId` | the same | no — exempt | + +**The `wait` segment is the default; exemptions must be explicit.** A *passively* waiting route without one is acceptable only if it is listed in the table above together with the reason it cannot carry the segment. A passively waiting route that is neither named `wait` nor listed here is a defect — fix it by renaming the route or by adding an entry, never by leaving it undocumented. Routes of the second kind above — those awaiting an operation they started themselves — need no entry; they are outside this rule by design. + +The four current exemptions keep their paths because those are fixed from outside: `/v1/lnurlp/:id` is the LNURL pay-request path encoded into LNURLs already in circulation, `/v1/lnurlp/tx/:id` is handed to the payer inside the payment request itself, and the two node routes are admin-only and `@ApiExcludeEndpoint()`. Because they carry no `wait` segment they stay visible in latency monitoring — read their duration as expected behavior, not as a regression. + +This is not cosmetic. Latency monitoring excludes routes matching `^.*/wait(/.*)?$` from its slowest-requests view. A long poll's duration measures how long a *customer* took to act, not how long the API computed — leaving it in that view pushes the genuine outliers out of a list with a fixed row cap. + +Getting the name wrong breaks monitoring in one of two directions: + +- **A passively waiting endpoint without a `wait` segment** appears as a permanent latency outlier and masks real regressions — that is exactly what the exemptions above cost us today, which is why the list must stay short and justified. +- **A fast endpoint with a `wait` segment** is silently dropped from the latency view — if it ever becomes slow, nobody notices. + +The pattern is segment-anchored, so `/waitlist`, `/waitTime`, `/awaiting` and `/waiting/:id` are unaffected; only a complete `wait` segment matches. Matching runs on the server-side route template (`http.route`), never on the raw request path, which is caller-controlled. + ### RealUnit: `/quote/*` vs `/brokerbot/*` The RealUnit purchase and sale flows historically lived under `/v1/realunit/brokerbot/*`. That naming is misleading: most of those endpoints never touch the on-chain Brokerbot smart contract. Treat them as two distinct subsystems: From 213d4b5b476b0e4714c3c74e2dcd9f763e297ba0 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:19:48 +0200 Subject: [PATCH 2/5] Say attempts, not retries, for the ICP allowance wait (#4507) `Util.retry(action, 3, 2000)` runs three attempts in total, not three retries after a first failure: the parameter is named `tryCount` and the recursion guard is `tryCount > 1` (util.ts:685). The wait therefore spans at most two delays of 2 s, not three. Follow-up to #4498, where the wording said "3 retries at 2 s". --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 617add0f93..1a7212d758 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -979,7 +979,7 @@ Endpoints that block by design: | `GET /v1/lnurlp/wait/:id` | the payment resolves: completed, canceled, expired — or, for `MULTIPLE`-mode links, when a quote reaches the configured completion threshold (the payment itself may stay `Pending`) | yes | | `GET /v1/paymentLink/payment/wait` | the same, for the authenticated payment-link flow | yes | | `GET /v1/lnurlp/:id` | a pending payment appears; bounded by `timeout` (default 10 s, caller-controllable) | no — exempt | -| `GET /v1/lnurlp/tx/:id` | the payer's own broadcast reaches one confirmation (`tx` branch); 15 polls at 1 s. The `hex` branch broadcasts without awaiting confirmation, except on ICP, where it first waits for the payer's allowance (3 retries at 2 s) | no — exempt | +| `GET /v1/lnurlp/tx/:id` | the payer's own broadcast reaches one confirmation (`tx` branch); 15 polls at 1 s. The `hex` branch broadcasts without awaiting confirmation, except on ICP, where it first waits for the payer's allowance (up to 3 attempts, 2 s apart) | no — exempt | | `GET /v1/node/:node/tx/:txId` | the transaction reaches one confirmation; bounded at 600 s | no — exempt | | `GET /v1/node/:node/:mode/tx/:txId` | the same | no — exempt | From ec0cfb1c031b35c0fed0c2b4e9309c72ce66d7f5 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:04:21 +0200 Subject: [PATCH 3/5] 7392bb63 - Make balancesByType opt-out on the financial log endpoint (#4506) * perf: skip balancesByFinancialType in financial log query on demand The financial-overview screen calls GET /v1/dashboard/financial/log every minute but never reads balancesByType; that sub-tree still made up 82% of the response and roughly half the DB time (measured 2030ms vs 1078ms for the same query). Add an opt-out query parameter that drops the jsonb sub-tree from the SELECT list instead of just discarding it after loading. Existing callers that don't pass the parameter keep getting the exact same response as before. * style: apply prettier line width to the new service signature * test(dashboard): pin the byType query parameter at the controller The endpoint had no controller test at all, so a transposed argument or an inverted string check would have passed every existing service and repository test. Six inputs are pinned; only the exact string false disables the subtree. * test(dashboard): pin dailySample and the from edge cases too dailySample is evaluated by the same rule as byType but was only ever tested with true and false, so inverting its check would have passed. It now gets the same six inputs, each holding byType at the opposite value so a transposition of the two arguments cannot hide. Adds the two untested from branches: omitted yields undefined, and an unparseable value throws without reaching the service. * test(dashboard): cover the empty-string case for from An empty from takes the same early-return branch as a missing one, but only this case catches a mutation of that branch to an undefined check: an empty string would then reach Date parsing and throw instead of yielding undefined. --- .../dashboard-financial.controller.spec.ts | 95 +++++++++++++++ .../dashboard-financial.service.spec.ts | 37 +++++- .../dashboard-financial.controller.ts | 4 +- .../dashboard/dashboard-financial.service.ts | 14 ++- .../dashboard/dto/financial-log.dto.ts | 7 +- .../log/__tests__/log.repository.spec.ts | 91 ++++++++++++++ .../log/__tests__/log.service.spec.ts | 12 +- .../supporting/log/log.repository.ts | 114 +++++++++++------- src/subdomains/supporting/log/log.service.ts | 3 +- 9 files changed, 322 insertions(+), 55 deletions(-) create mode 100644 src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts diff --git a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts new file mode 100644 index 0000000000..2e01f396c1 --- /dev/null +++ b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts @@ -0,0 +1,95 @@ +import { createMock } from '@golevelup/ts-jest'; +import { BadRequestException } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { DashboardFinancialController } from '../dashboard-financial.controller'; +import { DashboardFinancialService } from '../dashboard-financial.service'; +import { FinancialLogResponseDto } from '../dto/financial-log.dto'; + +describe('DashboardFinancialController', () => { + let controller: DashboardFinancialController; + let dashboardFinancialService: DashboardFinancialService; + + beforeEach(async () => { + dashboardFinancialService = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + controllers: [DashboardFinancialController], + providers: [{ provide: DashboardFinancialService, useValue: dashboardFinancialService }], + }).compile(); + + controller = module.get(DashboardFinancialController); + }); + + describe('getFinancialLog', () => { + const from = '2026-07-01T00:00:00.000Z'; + const emptyResponse: FinancialLogResponseDto = { entries: [] }; + + it.each([ + { byType: undefined as string | undefined, expected: true, label: 'omitted' }, + { byType: '', expected: true, label: "empty string ''" }, + { byType: 'true', expected: true, label: "'true'" }, + { byType: '0', expected: true, label: "'0'" }, + { byType: 'False', expected: true, label: "'False'" }, + { byType: 'false', expected: false, label: "'false'" }, + ])( + 'forwards includeByType=$expected when byType is $label (and from/dailySample unchanged)', + async ({ byType, expected }) => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + if (byType === undefined) { + await controller.getFinancialLog(from, 'true'); + } else { + await controller.getFinancialLog(from, 'true', byType); + } + + expect(spy).toHaveBeenCalledWith(new Date(from), true, expected); + }, + ); + + it.each([ + { dailySample: undefined as string | undefined, expected: true, label: 'omitted' }, + { dailySample: '', expected: true, label: "empty string ''" }, + { dailySample: 'true', expected: true, label: "'true'" }, + { dailySample: '0', expected: true, label: "'0'" }, + { dailySample: 'False', expected: true, label: "'False'" }, + { dailySample: 'false', expected: false, label: "'false'" }, + ])( + 'forwards dailySample=$expected when dailySample is $label (byType held at the opposite value)', + async ({ dailySample, expected }) => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + const byType = expected ? 'false' : 'true'; + + await controller.getFinancialLog(from, dailySample, byType); + + expect(spy).toHaveBeenCalledWith(new Date(from), expected, !expected); + }, + ); + + it('passes from, dailySample and includeByType through in order without transposition', async () => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + await controller.getFinancialLog('2026-06-15T00:00:00.000Z', 'false', 'true'); + + expect(spy).toHaveBeenCalledWith(new Date('2026-06-15T00:00:00.000Z'), false, true); + }); + + it.each([ + { from: undefined as string | undefined, label: 'omitted' }, + { from: '', label: "empty string ''" }, + ])('passes undefined to the service when from is $label', async ({ from }) => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + await controller.getFinancialLog(from, 'true', 'true'); + + expect(spy).toHaveBeenCalledWith(undefined, true, true); + }); + + it('throws BadRequestException and does not call the service when from is not a valid date', async () => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + await expect(controller.getFinancialLog('not-a-date', 'true', 'true')).rejects.toThrow(BadRequestException); + + expect(spy).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts index b9f983e1cd..13e7cb95bb 100644 --- a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts +++ b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts @@ -231,7 +231,7 @@ describe('DashboardFinancialService', () => { const result = await service.getFinancialLog(from, true); expect(getBtcCoinSpy).toHaveBeenCalled(); - expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true); + expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true, undefined, undefined, undefined, undefined); // Ordering matters: btcAssetId is a SQL projection parameter, so getBtcCoin must finish first. expect(getBtcCoinSpy.mock.invocationCallOrder[0]).toBeLessThan(getSummariesSpy.mock.invocationCallOrder[0]); @@ -257,7 +257,40 @@ describe('DashboardFinancialService', () => { await service.getFinancialLog(); - expect(getSummariesSpy).toHaveBeenCalledWith(undefined, undefined, undefined); + expect(getSummariesSpy).toHaveBeenCalledWith( + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + ); + }); + + it('forwards includeByType=false through to getFinancialLogSummaries and the response omits balancesByType entirely (not an empty object, not null)', async () => { + const btcAsset = { id: 7 } as Awaited>; + const summaries: FinancialLogSummary[] = [ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 120, + minusBalanceChf: 20, + fxPnlChf: 1.5, + btcPriceChf: 64000, + // no balancesByType key: the repository omits it entirely when includeByType is false + }, + ]; + jest.spyOn(assetService, 'getBtcCoin').mockResolvedValue(btcAsset); + const getSummariesSpy = jest.spyOn(logService, 'getFinancialLogSummaries').mockResolvedValue(summaries); + + const from = new Date('2026-07-01T00:00:00Z'); + const result = await service.getFinancialLog(from, true, false); + + expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true, undefined, undefined, undefined, false); + expect('balancesByType' in result.entries[0]).toBe(false); + expect(JSON.parse(JSON.stringify(result.entries[0]))).not.toHaveProperty('balancesByType'); }); }); }); diff --git a/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts b/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts index 550afd1341..8034236e05 100644 --- a/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts +++ b/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts @@ -25,11 +25,13 @@ export class DashboardFinancialController { async getFinancialLog( @Query('from') from?: string, @Query('dailySample') dailySample?: string, + @Query('byType') byType?: string, ): Promise { const fromDate = this.parseDate(from); const sample = dailySample !== 'false'; + const includeByType = byType !== 'false'; - return this.dashboardFinancialService.getFinancialLog(fromDate, sample); + return this.dashboardFinancialService.getFinancialLog(fromDate, sample, includeByType); } @Get('latest') diff --git a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts index e7f46a22fa..37432322ce 100644 --- a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts +++ b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts @@ -23,12 +23,20 @@ export class DashboardFinancialService { private readonly refRewardService: RefRewardService, ) {} - async getFinancialLog(from?: Date, dailySample?: boolean): Promise { + async getFinancialLog(from?: Date, dailySample?: boolean, includeByType?: boolean): Promise { // BTC price is projected in SQL and needs btcAssetId as a parameter, so resolve getBtcCoin first. // One extra sequential roundtrip vs the previous Promise.all, judged negligible against the // eliminated transfer volume of the full message JSON. const btcAsset = await this.assetService.getBtcCoin(); - const summaries = await this.logService.getFinancialLogSummaries(btcAsset?.id, from, dailySample); + const summaries = await this.logService.getFinancialLogSummaries( + btcAsset?.id, + from, + dailySample, + undefined, + undefined, + undefined, + includeByType, + ); const entries = summaries.map((summary) => this.mapSummaryToEntry(summary)); return { entries }; @@ -255,7 +263,7 @@ export class DashboardFinancialService { minusBalanceChf: summary.minusBalanceChf ?? 0, fxPnlChf: summary.fxPnlChf ?? 0, btcPriceChf: summary.btcPriceChf, - balancesByType: summary.balancesByType, + ...(summary.balancesByType !== undefined ? { balancesByType: summary.balancesByType } : {}), }; } } diff --git a/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts b/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts index b53f4fb8c3..8ea95f8fa7 100644 --- a/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts +++ b/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts @@ -11,8 +11,13 @@ export class FinancialLogEntryDto { * plusBalanceChf/minusBalanceChf can be missing per type when the source FinancialDataLog * snapshot omitted one of the two keys (see FinancialLogSummary.balancesByType); a missing * value is left out of the JSON response the same way it always was, never defaulted to 0. + * + * The whole field is only present when the caller did not opt out via the `byType` query + * parameter on GET /v1/dashboard/financial/log (`byType=false`); when opted out it is omitted + * entirely from the response — never an empty object, never null — because it makes up 82% of + * the payload and the Overview screen that calls this endpoint on every refresh never reads it. */ - balancesByType: Record; + balancesByType?: Record; } export class FinancialLogResponseDto { diff --git a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts index 61c6ea1496..f962ddb2b0 100644 --- a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts @@ -691,5 +691,96 @@ describe('LogRepository', () => { // Wrong-typed values become undefined and are dropped on serialisation — never null/0/string/boolean. expect(JSON.parse(JSON.stringify(rows[0].balancesByType.Crypto))).toEqual({}); }); + + it('omits balancesByFinancialType from the SELECT list when includeByType is explicitly false (the actual DB-time and payload-size saving)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7, undefined, undefined, undefined, undefined, undefined, false); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).not.toContain('balancesByFinancialType'); + }); + + it('still selects and returns balancesByType when includeByType is not passed at all (backward compatibility)', async () => { + const repo = new LogRepository({} as EntityManager); + const created = new Date('2026-07-14T00:00:00Z'); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([ + { + created, + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: { Crypto: { plusBalanceChf: 10, minusBalanceChf: 5 } }, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(7); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain(`message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType"`); + expect(rows[0].balancesByType).toEqual({ Crypto: { plusBalanceChf: 10, minusBalanceChf: 5 } }); + }); + + it('omits the balancesByType key entirely (not undefined-valued, not an empty object) from the mapped summary when includeByType is false', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + }, + ]); + + const rows = await repo.getFinancialLogSummaries( + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + false, + ); + + expect(rows).toHaveLength(1); + expect('balancesByType' in rows[0]).toBe(false); + }); + + it('keeps the same $N placeholder positions when includeByType=false is combined with every other optional parameter (btcAssetId, from, to, after, limit)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(financialLogQueryBuilderStub(true) as never); + const from = new Date('2026-01-01T00:00:00Z'); + const to = new Date('2026-02-01T00:00:00Z'); + + await repo.getFinancialLogSummaries(7, from, false, to, 50, 10, false); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('$5::text'); + expect(sql).toContain('created >= $6'); + expect(sql).toContain('created <= $7'); + expect(sql).toContain('(created, id) > ((SELECT c.created FROM log c WHERE c.id = $8), $9)'); + expect(sql).toContain('LIMIT $10'); + expect(sql).not.toContain('balancesByFinancialType'); + expect(params).toEqual([ + 'LogService', + FINANCIAL_DATA_LOG_SUBSYSTEM, + LogSeverity.INFO, + true, + '7', + from, + to, + 10, + 10, + 50, + ]); + }); }); }); diff --git a/src/subdomains/supporting/log/__tests__/log.service.spec.ts b/src/subdomains/supporting/log/__tests__/log.service.spec.ts index 8c06347f83..366850d949 100644 --- a/src/subdomains/supporting/log/__tests__/log.service.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.service.spec.ts @@ -288,9 +288,17 @@ describe('LogService', () => { ]; const spy = jest.spyOn(logRepo, 'getFinancialLogSummaries').mockResolvedValue(summaries); - await expect(service.getFinancialLogSummaries(7, from, true, to, 25, 10)).resolves.toEqual(summaries); + await expect(service.getFinancialLogSummaries(7, from, true, to, 25, 10, false)).resolves.toEqual(summaries); - expect(spy).toHaveBeenCalledWith(7, from, true, to, 25, 10); + expect(spy).toHaveBeenCalledWith(7, from, true, to, 25, 10, false); + }); + + it('forwards includeByType as undefined when the caller omits it, so the repository default (true) applies', async () => { + const spy = jest.spyOn(logRepo, 'getFinancialLogSummaries').mockResolvedValue([]); + + await service.getFinancialLogSummaries(7); + + expect(spy).toHaveBeenCalledWith(7, undefined, undefined, undefined, undefined, undefined, undefined); }); }); diff --git a/src/subdomains/supporting/log/log.repository.ts b/src/subdomains/supporting/log/log.repository.ts index 79f1becb03..b927ade16b 100644 --- a/src/subdomains/supporting/log/log.repository.ts +++ b/src/subdomains/supporting/log/log.repository.ts @@ -71,8 +71,14 @@ export interface FinancialLogSummary { /** * plusBalanceChf/minusBalanceChf can be `undefined` per type: a real production row had a * `balancesByFinancialType` entry missing one of the two keys (see getFinancialLogSummaries below). + * + * The whole property is only present when getFinancialLogSummaries' `includeByType` parameter is + * true (the default, for backward compatibility); when explicitly false the key is absent (not an + * empty object) because the underlying `balancesByFinancialType` jsonb sub-tree was never selected + * from the database in the first place — that omission from the SELECT list, not a post-hoc + * discard, is the actual DB-time/payload saving. */ - balancesByType: Record; + balancesByType?: Record; } @Injectable() @@ -396,6 +402,10 @@ ORDER BY l.created ASC, l.id ASC`; to?: Date, limit?: number, after?: number, // id of the last row of the previous page; NEVER a Date/created value + includeByType = true, // selects/omits the balancesByFinancialType sub-tree from the SELECT list; + // true (the default) reproduces the exact pre-existing response for every caller that does not + // pass this parameter; only an explicit false skips the sub-tree. This default is intentional and + // required by this spec's backward-compatibility guarantee — it is not masking an error case. ): Promise { const params: unknown[] = []; let i = 1; @@ -463,31 +473,39 @@ ORDER BY l.created ASC, l.id ASC`; params.push(limit); } - const sql = ` -SELECT created AS "created", - id AS "id", - CASE + const selectColumns = [ + `created AS "created"`, + `id AS "id"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'totalBalanceChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'totalBalanceChf')::float8 ELSE NULL - END AS "totalBalanceChf", - CASE + END AS "totalBalanceChf"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'plusBalanceChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'plusBalanceChf')::float8 ELSE NULL - END AS "plusBalanceChf", - CASE + END AS "plusBalanceChf"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'minusBalanceChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'minusBalanceChf')::float8 ELSE NULL - END AS "minusBalanceChf", - CASE + END AS "minusBalanceChf"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'fxPnlChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'fxPnlChf')::float8 ELSE NULL - END AS "fxPnlChf", - ${btcPriceSelect} AS "btcPriceChf", - message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType" + END AS "fxPnlChf"`, + `${btcPriceSelect} AS "btcPriceChf"`, + ]; + // The actual DB-time/payload saving: when not requested, this sub-tree is never in the SELECT + // list at all (not selected and then discarded after the fact). + if (includeByType) { + selectColumns.push(`message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType"`); + } + + const sql = ` +SELECT ${selectColumns.join(',\n ')} FROM log WHERE ${conditions.join(' AND ')} ORDER BY created ASC, id ASC @@ -501,7 +519,7 @@ ${limitClause}`; minusBalanceChf: number | string | null; fxPnlChf: number | string | null; btcPriceChf: number | string | null; - balancesByFinancialType: unknown; + balancesByFinancialType?: unknown; }[]; const rows: FinancialLogSummary[] = raw.map((r) => { @@ -512,35 +530,41 @@ ${limitClause}`; // btcPriceChf: absent/unusable path → 0, matching extractBtcPrice's `?.priceChf ?? 0`. const btcPriceChf = r.btcPriceChf == null ? 0 : Number(r.btcPriceChf); - const balancesByType: Record = {}; - if (r.balancesByFinancialType != null) { - // Always an already-parsed object/array here, never a JSON string: pg-types registers JSON.parse - // as the type parser for jsonb (OID 3802) and this repo configures no custom type parser, so the - // driver never hands back a raw string for this column. - const byType = r.balancesByFinancialType as Record< - string, - { plusBalanceChf?: number; minusBalanceChf?: number } - >; - // Only real numbers are kept for plusBalanceChf / minusBalanceChf; any non-number value - // (string, boolean, null, nested object, or missing key) becomes undefined so the result - // matches the number | undefined contract. On current production data this is a no-op - // (287,989 entries both numbers, one missing plusBalanceChf key — no string/boolean/null), - // and exists only to protect the contract for future/other data. The previous mapLogToEntry - // passed contract-breaking values through unchanged; this closes that hole. Same hardening - // idea as the five scalar fields above (jsonb_typeof = 'number' in SQL), applied in - // TypeScript because balancesByFinancialType is passed through as a raw JSON object. Note: - // this is a type check, not a finiteness check — it also lets `Infinity` through (e.g. from - // a JSON number like `1e999`, which `JSON.parse` turns into `Infinity`); `NaN` cannot occur - // in valid jsonb. - const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); - for (const [type, data] of Object.entries(byType)) { - // Optional chaining keeps non-object entries (null / number / string / boolean) from throwing: - // property access yields undefined and the row is retained with empty fields, rather than - // failing the whole request. - balancesByType[type] = { - plusBalanceChf: asNumber(data?.plusBalanceChf), - minusBalanceChf: asNumber(data?.minusBalanceChf), - }; + // Only computed/present at all when includeByType is true (see the SELECT-list construction + // above): the key is entirely absent on the returned summary otherwise (conditional spread + // below), not an empty object and not null. + let balancesByType: Record | undefined; + if (includeByType) { + balancesByType = {}; + if (r.balancesByFinancialType != null) { + // Always an already-parsed object/array here, never a JSON string: pg-types registers JSON.parse + // as the type parser for jsonb (OID 3802) and this repo configures no custom type parser, so the + // driver never hands back a raw string for this column. + const byType = r.balancesByFinancialType as Record< + string, + { plusBalanceChf?: number; minusBalanceChf?: number } + >; + // Only real numbers are kept for plusBalanceChf / minusBalanceChf; any non-number value + // (string, boolean, null, nested object, or missing key) becomes undefined so the result + // matches the number | undefined contract. On current production data this is a no-op + // (287,989 entries both numbers, one missing plusBalanceChf key — no string/boolean/null), + // and exists only to protect the contract for future/other data. The previous mapLogToEntry + // passed contract-breaking values through unchanged; this closes that hole. Same hardening + // idea as the five scalar fields above (jsonb_typeof = 'number' in SQL), applied in + // TypeScript because balancesByFinancialType is passed through as a raw JSON object. Note: + // this is a type check, not a finiteness check — it also lets `Infinity` through (e.g. from + // a JSON number like `1e999`, which `JSON.parse` turns into `Infinity`); `NaN` cannot occur + // in valid jsonb. + const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); + for (const [type, data] of Object.entries(byType)) { + // Optional chaining keeps non-object entries (null / number / string / boolean) from throwing: + // property access yields undefined and the row is retained with empty fields, rather than + // failing the whole request. + balancesByType[type] = { + plusBalanceChf: asNumber(data?.plusBalanceChf), + minusBalanceChf: asNumber(data?.minusBalanceChf), + }; + } } } @@ -552,7 +576,7 @@ ${limitClause}`; minusBalanceChf: r.minusBalanceChf == null ? null : Number(r.minusBalanceChf), fxPnlChf: r.fxPnlChf == null ? null : Number(r.fxPnlChf), btcPriceChf, - balancesByType, + ...(includeByType ? { balancesByType } : {}), }; }); diff --git a/src/subdomains/supporting/log/log.service.ts b/src/subdomains/supporting/log/log.service.ts index da29c6fa5f..de207e4126 100644 --- a/src/subdomains/supporting/log/log.service.ts +++ b/src/subdomains/supporting/log/log.service.ts @@ -161,8 +161,9 @@ export class LogService { to?: Date, limit?: number, after?: number, // id of the last row of the previous page; NEVER a Date/created value + includeByType?: boolean, ): Promise { - return this.logRepo.getFinancialLogSummaries(btcAssetId, from, dailySample, to, limit, after); + return this.logRepo.getFinancialLogSummaries(btcAssetId, from, dailySample, to, limit, after, includeByType); } async getLatestFinancialLog(): Promise { From b02504c94bdba438c3699bfed9c3b6c6f1f074c3 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:27:54 +0200 Subject: [PATCH 4/5] 7392bb63 - Add the (tradingRuleId, id) index for the per-rule max-order aggregate (#4497) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(trading): index trading_order by (tradingRuleId, id) and rewrite the per-rule max-order lookup TradingRuleService.getCurrentTradingOrders ran a table-wide GROUP BY MAX(id) aggregate over trading_order once per minute (LogJobService), scanning 5.4M rows to compute 17 maxima (474 ms, ~680 MB read per call). The obvious per-rule rewrite is not safe on its own: measured in production against the current index set, a correlated MAX(id) subquery per rule takes 4,329 ms — 9x slower than today, because Postgres has no index to jump straight to a rule's row range and PostgreSQL 17.10 has no index skip scan. So this ships the composite index and the query rewrite together, never apart: migration/1785480000000-AddTradingOrderRuleIdIndex.js adds ("tradingRuleId", "id"), and getCurrentTradingOrders now looks up the max id per rule directly, letting Postgres use that index (17 cheap lookups instead of one full scan). The existing single-column index on tradingRuleId is left in place; the migration docstring notes the resulting redundancy. Added trading-rule.service.pg.spec.ts (pg-mem, real generated SQL) to pin the rewrite: highest id per rule, rules without orders produce no entry, an order with no matching rule is excluded, and the result is checked against the pre-rewrite aggregate on the same seeded data. * fix(trading): resolve migration ordering collision, document N+1 trade-off AddTradingOrderRuleIdIndex1785480000000 sorted before the already-merged ClearDevUserSignatures1785500000000, so it would run before an already-executed migration. Renamed to 1785510000000 (class name and name property updated to match); the generated index name IDX_710fd49e19d248643cb2afa70f is unaffected, since TypeORM derives it from the table and column names, not the timestamp. Also documents in getCurrentTradingOrders that the per-rule MAX(id) lookups are an N+1 pattern that scales linearly with the rule count (currently 17 rules -> 19 queries), and why a single correlated-subquery alternative was skipped: it would give up testability against the lightweight mirror entities used in trading-rule.service.pg.spec.ts. * revert(trading): keep the single-statement aggregate, ship the index alone The per-rule rewrite is withdrawn. It traded one READ COMMITTED snapshot for N, so two rules could report maxima from different points in time — not acceptable where LogJobService writes the FinanceLog from the result. The coherent single-statement alternative is a correlated subquery, which pg-mem cannot execute at all, so it could not be covered by a test that runs. The index stays: measured against a Postgres 17.10 rebuild with production planner settings, the unchanged aggregate moves from a Parallel Seq Scan over 93,486 blocks to a Parallel Index Only Scan over 15,020 with zero heap fetches. The docstring drops the claim that index and rewrite must ship together. The test loses its comparison against a re-created copy of the same aggregate, which could no longer fail, and gains an empty-rule-table case plus an explicit assertion that no null reaches the In(...) list. * docs(migration): drop an unmeasured table size from the docstring intro The opening sentence claimed ~920 MB, which matches none of the measurements this migration cites — production reports 698 MB for the heap and the rebuild 730 MB. The intro now carries no figures at all; the measured ones stay further down, each with its source named. * fix(test): give the pg-mem mirror entity the relation the join needs The suite was written for a query shape that took no join, so the mirror entity carried only the tradingRuleId column. Restoring the single-statement aggregate brought back innerJoin on a relation path, which TypeORM resolves through entity metadata — all three tests aborted with 'Relation with property path tradingRule in entity was not found'. Foreign key creation stays off so the deliberately orphaned fixture remains insertable. * fix(test): narrow the findBy spy argument instead of casting to a made-up shape The cast asserted a hand-written shape onto findBy's argument type, which also allows an array — TypeScript rejected it as insufficiently overlapping. The argument is now narrowed: an array condition throws, then instanceof FindOperator establishes the real TypeORM type. The assertion itself is unchanged, and every unexpected shape fails loudly rather than skipping. * docs(test): say what the operator guard actually checks The comment claimed a different operator would fail loudly, but the guard tests the FindOperator base class — Any, Not and Between all pass it. The guard's real job is rejecting a bare value that would slip past the array check, and the operator choice is not what this test is about. --- ...785510000000-AddTradingOrderRuleIdIndex.js | 145 +++++++++++++++++ .../__tests__/trading-rule.service.pg.spec.ts | 147 ++++++++++++++++++ .../trading/services/trading-rule.service.ts | 9 ++ 3 files changed, 301 insertions(+) create mode 100644 migration/1785510000000-AddTradingOrderRuleIdIndex.js create mode 100644 src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts diff --git a/migration/1785510000000-AddTradingOrderRuleIdIndex.js b/migration/1785510000000-AddTradingOrderRuleIdIndex.js new file mode 100644 index 0000000000..ef5e0fd4d9 --- /dev/null +++ b/migration/1785510000000-AddTradingOrderRuleIdIndex.js @@ -0,0 +1,145 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * Add a composite index on `trading_order ("tradingRuleId", "id")` so the per-minute + * "latest trading order per rule" lookup stops doing a full sequential scan of the table. + * Row counts and sizes are given further down, together with where each was measured. + * + * The query this targets is: + * `SELECT MAX("tradingOrder"."id") AS "tradingOrderId" FROM "trading_order" "tradingOrder" + * INNER JOIN "trading_rule" "tradingRule" ON "tradingRule"."id" = "tradingOrder"."tradingRuleId" + * GROUP BY "tradingOrder"."tradingRuleId"`. + * Source: `TradingRuleService.getCurrentTradingOrders`, called from `LogJobService` once per + * minute as part of the financial-log job. + * + * Production `EXPLAIN (ANALYZE, BUFFERS)` for this exact aggregate query (measured externally + * against production; not reproducible from this repository) showed Execution Time 489 ms, + * Buffers: shared hit=3734 read=85614 — only 4.2% of blocks came from the 1 GB `shared_buffers` + * cache, the rest was freshly read from disk on every one-minute run. Table size at the time of + * measurement in production: 698 MB. Result: 17 rows (there are exactly 17 `trading_rule` rows). + * + * Column order `("tradingRuleId", "id")` is intentional: equality on `tradingRuleId` first so + * Postgres can jump straight to one rule's leaf range, then `id` so a backward index scan finds + * the maximum id for that rule without a sort or a table-wide aggregate. The same physical order + * is what enables the Index Only Scan plan change for the unchanged aggregate described below; + * it would also be what a correlated per-rule lookup needs, if this repository's test + * infrastructure allowed one (see the comment on the service method for that trade-off). + * + * This migration ships ONLY the index; the query above is not rewritten. Measured externally + * against a Postgres 17.10 rebuild with production planner settings, loaded with 5,421,152 rows + * (730 MB) and the 17 `trading_rule` rows in real production distribution (not reproducible + * from this repository): + * - Without this index: Parallel Seq Scan, 93,486 buffer blocks, 8,181 ms (warm). + * - With this index: Parallel Index Only Scan, Heap Fetches: 0, 15,020 buffer blocks, 5,676 ms + * (warm). + * The planner picks this index for exactly the query above with no code change — this migration + * is therefore not a no-op. The index itself is 116 MB. That matches the fresh production + * measurement above (489 ms, shared hit=3734 read=85614, only 4.2% cache hit rate, 698 MB + * table): a 116 MB index has a realistic chance of staying resident in the 1 GB + * `shared_buffers`; the substantially larger table demonstrably does not. A correlated per-rule + * lookup (`SELECT r.id, (SELECT MAX(o.id) FROM trading_order o WHERE o."tradingRuleId" = r.id) + * FROM trading_rule r;`) would be faster still with this index — Index Only Scan Backward, 52 + * buffer blocks, 1.5 ms (warm), measured on the same rebuild (same external measurement + * disclaimer: not reproducible from this repository) — but is NOT shipped here: it would need a + * correlated subquery that this repository's pg-mem-based test suite (pg-mem 3.0.14) cannot + * execute. See the comment on `TradingRuleService.getCurrentTradingOrders` for that trade-off. + * + * The existing single-column index `IDX_f862025cb7ca5a2d66d14fb89a` on + * `trading_order ("tradingRuleId")` is NOT removed by this migration. It was created by + * `AddForeignKeyIndexes1779802432879` (`CREATE INDEX "IDX_f862025cb7ca5a2d66d14fb89a" ON + * "trading_order" ("tradingRuleId")`). The new composite index makes that single-column index + * functionally redundant for most purposes (any query that can use the single-column index on + * `tradingRuleId` can equally use the new composite, since `tradingRuleId` is its leading + * column), but dropping the old index is out of scope for this change and is left for a + * separate, later migration. + * + * CREATE INDEX CONCURRENTLY is not used: migrations in this codebase run transactionally and + * boot-blockingly (see `src/config/config.ts`, `migrationsRun` gated by the `SQL_MIGRATE` env + * var). CREATE INDEX CONCURRENTLY is not allowed inside a transaction and would crash the + * migration. + * + * Lock behaviour, stated precisely: all pending migrations run inside a single database + * transaction (TypeORM default `migrationsTransactionMode: "all"`), and PostgreSQL only + * releases locks at COMMIT, not at the end of each statement. Evidence: + * `node_modules/typeorm/data-source/DataSource.js` (`migrationExecutor.transaction = + * options?.transaction || this.options?.migrationsTransactionMode || "all"` — default `"all"`); + * `src/config/config.ts` only sets `migrationsRun` and never overrides + * `migrationsTransactionMode` (corroborated by the comment in + * `src/shared/models/asset/__tests__/add-binance-custody-assets-ondo-ada.migration.spec.ts` — + * "no migrationsTransactionMode override → default 'all'"); + * `node_modules/typeorm/migration/MigrationExecutor.js` starts one transaction for pending + * migrations and commits only at the end. A plain CREATE INDEX holds a SHARE lock for the + * entire build; reads continue throughout, but writes to the table are blocked while that lock + * is held. Because locks are held until COMMIT, if other migrations are pending in the same + * batch, this index's SHARE lock is held until all of them commit together, not just until this + * statement finishes. `SET LOCAL lock_timeout` bounds only how long we wait to ACQUIRE the lock, + * not how long we hold it once acquired, and it is scoped to the whole transaction — that is + * exactly why it is set only once at the top of `up()` and once at the top of `down()`, not per + * statement (there is only one `CREATE INDEX` / one `DROP INDEX` in this migration). + * + * Risk framing: this migration runs boot-blockingly at app startup (`migrationsRun`, gated by + * the `SQL_MIGRATE` env var), so the starting instance itself is not yet serving requests and is + * not itself a writer. Concurrent writers would be a still-running predecessor instance during a + * rolling deploy, or external consumers. If a lock conflict occurs, the migration aborts after + * `lock_timeout` and so does the app start — that is fail-closed and intentional, but it is a + * deploy abort and must be named as such. + * + * `down()` reverses this with `DROP INDEX` and is subject to a stricter lock: PostgreSQL takes + * an ACCESS EXCLUSIVE lock for `DROP INDEX` (vs. the SHARE lock `CREATE INDEX` takes above), and + * ACCESS EXCLUSIVE conflicts with every other lock mode, including the AccessShareLock a plain + * `SELECT` takes — so `down()` blocks reads as well as writes, not writes alone. Because + * `down()` also runs inside the single-batch transaction (same TypeORM default + * `migrationsTransactionMode: "all"`), the same hold-until-COMMIT reasoning applies. + * + * Honest disclaimer: whether the Postgres planner will actually pick this new index for the + * unchanged aggregate query above has NOT been verified in production itself, because the index + * does not exist there yet. The rebuild measurement above already shows that the plan changes + * from Parallel Seq Scan to Parallel Index Only Scan when this index exists — but that is still + * not confirmation on production itself. As a point of reference (not a guarantee), the same + * style of prediction was made for the `created` index in + * `AddTradingOrderCreatedIndex1785470000000` (selectivity + cost-model reasoning only, no prior + * plan-change observation) and was confirmed after that deploy (measured externally against + * production; not reproducible from this repository): the query's plan changed from a Seq Scan + * to an Index Scan, execution time dropped from 141.283 ms to 30.3 ms, and buffer reads dropped + * from 89,282 to 2,716. + * + * Index name: `IDX_710fd49e19d248643cb2afa70f` on `trading_order ("tradingRuleId", "id")`. + * This is not an arbitrary name but the deterministic name TypeORM's DefaultNamingStrategy would + * generate itself, since custom index naming is disallowed by CONTRIBUTING.md. The name is + * `IDX_` followed by the first 26 hex characters of `sha1('trading_order_id_tradingRuleId')` + * (table name + `_` + the two column names `id` and `tradingRuleId` sorted alphabetically and + * joined with `_`, per TypeORM's DefaultNamingStrategy — `id` sorts before `tradingRuleId`). + * The physical index column order remains `("tradingRuleId", "id")` as required for the equality + * + max-id access path above; only the name derivation sorts the column names. + * + * @class + * @implements {MigrationInterface} + */ +module.exports = class AddTradingOrderRuleIdIndex1785510000000 { + name = 'AddTradingOrderRuleIdIndex1785510000000'; + + /** + * @param {QueryRunner} queryRunner + */ + async up(queryRunner) { + // SET LOCAL is scoped to the whole transaction. Bounds WAIT time to acquire the lock, not how + // long the lock is held. Set once: this migration has a single CREATE INDEX statement. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query( + `CREATE INDEX "IDX_710fd49e19d248643cb2afa70f" ON "trading_order" ("tradingRuleId", "id")`, + ); + } + + /** + * @param {QueryRunner} queryRunner + */ + async down(queryRunner) { + // SET LOCAL is scoped to the whole transaction. Bounds WAIT time to acquire the lock, not how + // long the lock is held. Set once: this migration has a single DROP INDEX statement. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query(`DROP INDEX "public"."IDX_710fd49e19d248643cb2afa70f"`); + } +}; diff --git a/src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts b/src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts new file mode 100644 index 0000000000..f11d939706 --- /dev/null +++ b/src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts @@ -0,0 +1,147 @@ +import { createMock } from '@golevelup/ts-jest'; +import { DataType, newDb } from 'pg-mem'; +import { Column, DataSource, Entity, FindOperator, JoinColumn, ManyToOne, PrimaryColumn } from 'typeorm'; +import { TradingRuleService } from '../trading-rule.service'; +import { TradingService } from '../trading.service'; + +// the real TradingOrder / TradingRule entities cannot be registered standalone (relations pull +// in the whole entity graph), so these tables mirror only the columns getCurrentTradingOrders +// actually touches — under the real table names +@Entity({ name: 'trading_rule' }) +class TradingRuleTable { + @PrimaryColumn() + id: number; +} + +@Entity({ name: 'trading_order' }) +class TradingOrderTable { + @PrimaryColumn() + id: number; + + @Column({ type: 'int' }) + tradingRuleId: number; + + // Relation path for `.innerJoin('tradingOrder.tradingRule', ...)`; createForeignKeyConstraints + // is false so the intentionally orphaned fixture row (tradingRuleId with no matching rule) stays insertable. + @ManyToOne(() => TradingRuleTable, { nullable: false, createForeignKeyConstraints: false }) + @JoinColumn({ name: 'tradingRuleId' }) + tradingRule: TradingRuleTable; +} + +// runs getCurrentTradingOrders against a Postgres-semantics engine (pg-mem) to verify the +// aggregation semantics, because a mocked query builder never executes SQL and a wrong shape +// (e.g. MAX swapped for MIN, or the INNER JOIN removed) would otherwise go unnoticed +describe('TradingRuleService.getCurrentTradingOrders (postgres semantics)', () => { + let dataSource: DataSource; + let service: TradingRuleService; + + beforeAll(async () => { + const db = newDb(); + // TypeORM runs SELECT version() / current_database() on connect; pg-mem does not ship them + db.public.registerFunction({ name: 'version', returns: DataType.text, implementation: () => 'PostgreSQL 15.0' }); + db.public.registerFunction({ name: 'current_database', returns: DataType.text, implementation: () => 'test' }); + + dataSource = (await db.adapters.createTypeormDataSource({ + type: 'postgres', + entities: [TradingRuleTable, TradingOrderTable], + synchronize: true, + })) as DataSource; + await dataSource.initialize(); + }); + + afterAll(async () => { + if (dataSource?.isInitialized) await dataSource.destroy(); + }); + + beforeEach(async () => { + await dataSource.getRepository(TradingOrderTable).clear(); + await dataSource.getRepository(TradingRuleTable).clear(); + + const tradingService = createMock(); + service = new TradingRuleService(tradingService); + (service as any).orderRepo = dataSource.getRepository(TradingOrderTable); + (service as any).ruleRepo = dataSource.getRepository(TradingRuleTable); + }); + + async function seedFixture(): Promise { + const ruleRepo = dataSource.getRepository(TradingRuleTable); + const orderRepo = dataSource.getRepository(TradingOrderTable); + + // rule 1: several orders → expect max id 30 + // rule 2: exactly one order → expect id 40 + // rule 3: no orders → must not appear + // orphan order 99: tradingRuleId matches no rule → must not appear (INNER JOIN exclusion) + await ruleRepo.save([{ id: 1 }, { id: 2 }, { id: 3 }]); + await orderRepo.save([ + { id: 10, tradingRuleId: 1 }, + { id: 20, tradingRuleId: 1 }, + { id: 30, tradingRuleId: 1 }, + { id: 40, tradingRuleId: 2 }, + { id: 99, tradingRuleId: 999 }, + ]); + } + + it('returns the highest-id order per rule, skips empty rules and orphans', async () => { + await seedFixture(); + + const result = await service.getCurrentTradingOrders(); + const resultIds = result.map((order) => order.id).sort((a, b) => a - b); + + // concrete ids per rule (fails if MAX is swapped for MIN or any wrong pick) + expect(resultIds).toEqual([30, 40]); + expect(result).toHaveLength(2); + expect(result.some((order) => order.id === 10 || order.id === 20)).toBe(false); + expect(result.some((order) => order.id === 99)).toBe(false); + }); + + it('returns an empty array when trading_rule is empty', async () => { + const result = await service.getCurrentTradingOrders(); + + expect(result).toEqual([]); + }); + + it('never passes null or undefined order ids into findBy In(...)', async () => { + await dataSource.getRepository(TradingRuleTable).save([{ id: 1 }, { id: 2 }]); + await dataSource.getRepository(TradingOrderTable).save([ + { id: 10, tradingRuleId: 1 }, + { id: 20, tradingRuleId: 1 }, + ]); + + const findBySpy = jest.spyOn(service['orderRepo'], 'findBy'); + try { + await service.getCurrentTradingOrders(); + + expect(findBySpy).toHaveBeenCalled(); + const findByArg = findBySpy.mock.calls[0][0]; + + // findBy accepts a single condition or an array of them; getCurrentTradingOrders only ever + // passes a single { id: In(...) } condition, so an array here would itself be a regression. + if (Array.isArray(findByArg)) { + throw new Error('expected findBy to receive a single FindOptionsWhere condition, not an array of them'); + } + + const idCondition = findByArg.id; + + // In(...) produces a real TypeORM FindOperator instance. A bare value would slip past the + // array check below, so reject it here. This does not pin the operator to In specifically: + // Any(), Not() and friends are FindOperator instances too, and swapping In for Any would + // carry the same list of ids -- which is what this test is actually about. + if (!(idCondition instanceof FindOperator)) { + throw new Error(`expected findBy id condition to be a FindOperator, got: ${String(idCondition)}`); + } + + // FindOperator.value is typed against the entity's own field type (number here), but + // In(...) stores the full array as the operator's underlying value. Widen through the real + // FindOperator class -- not an invented shape -- to read it without lying about its declared + // element type; unknown is the one legitimate single-step escape hatch for this widening. + const idValues = (idCondition as FindOperator).value; + if (!Array.isArray(idValues)) { + throw new Error('expected the FindOperator to carry an array of ids'); + } + + expect(idValues.every((id) => id !== null && id !== undefined)).toBe(true); + } finally { + findBySpy.mockRestore(); + } + }); +}); diff --git a/src/subdomains/core/trading/services/trading-rule.service.ts b/src/subdomains/core/trading/services/trading-rule.service.ts index 837f1aa657..1368a767cd 100644 --- a/src/subdomains/core/trading/services/trading-rule.service.ts +++ b/src/subdomains/core/trading/services/trading-rule.service.ts @@ -21,6 +21,15 @@ export class TradingRuleService { // --- PUBLIC API --- // + // One statement, not a per-rule loop: all rules' maxima must come from the same + // READ-COMMITTED snapshot, because LogJobService writes the FinanceLog from this result. + // Separate statements per rule could observe an insert into trading_order mid-loop and mix + // maxima from different points in time — a single GROUP BY aggregate cannot do that. + // The composite index on trading_order ("tradingRuleId", "id") (see the + // AddTradingOrderRuleIdIndex migration) lets Postgres answer this with an Index Only Scan. + // A correlated per-rule lookup would be faster still, but pg-mem (this repo's test engine for + // this query, see trading-rule.service.pg.spec.ts) cannot execute a correlated subquery — + // don't "optimize" this into one without first solving that. async getCurrentTradingOrders(): Promise { const lastTradingOrderIds = await this.orderRepo .createQueryBuilder('tradingOrder') From 3f4a0bf8c3ce476cdda6bb23ef4d9e6391f47b40 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:34:27 +0200 Subject: [PATCH 5/5] acaf0515 - perf(statistic): load only status settings for the status endpoint (#4512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(statistic): load only status settings for the status endpoint GET /v1/statistic/status is polled about six times a minute and returns 32 bytes. It went through SettingService.getAll(), which reads the whole setting table and filtered for the Status suffix in JS afterwards. Measured in production: the table holds 76 rows totalling 1586 kB of values, of which the two Status keys are 14 bytes. A single unrelated entry (ledgerCutoverBoundary.trading_order) accounts for 1534 kB and was transferred and materialised into entities on every poll. Filter in the database instead, per CONTRIBUTING ("filter in SQL not JS"). getAll() stays as it is — the settings endpoint still needs every row. The response is unchanged. * refactor(setting): use find with Like and a deterministic order Review follow-ups: - Prefer the repository method over the query builder, per CONTRIBUTING; every other suffix filter in this codebase uses Like in find options. - Order by id so the projected-name reduce in StatisticService has a defined winner. Two distinct keys can map to the same projected name (StatusfooStatus and fooStatusStatus both become fooStatus); the old find() path did not guarantee an order either, so this pins behaviour that was previously left to the query plan. - Add a direct repository test asserting the exact find options, so a swapped pattern (Status% instead of %Status) or a dropped order fails. --- .../__tests__/setting.repository.spec.ts | 29 ++++++++++ .../models/setting/setting.repository.ts | 6 +- src/shared/models/setting/setting.service.ts | 6 ++ .../__tests__/statistic.service.spec.ts | 55 +++++++++++++++++++ .../core/statistic/statistic.service.ts | 6 +- 5 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 src/subdomains/core/statistic/__tests__/statistic.service.spec.ts diff --git a/src/shared/models/setting/__tests__/setting.repository.spec.ts b/src/shared/models/setting/__tests__/setting.repository.spec.ts index 3a93abc107..e343023697 100644 --- a/src/shared/models/setting/__tests__/setting.repository.spec.ts +++ b/src/shared/models/setting/__tests__/setting.repository.spec.ts @@ -1,6 +1,35 @@ +import { Like } from 'typeorm'; import { Setting } from '../setting.entity'; import { SettingRepository } from '../setting.repository'; +describe('SettingRepository.getStatusSettings', () => { + function repositoryWithFind(result: Setting[]) { + const repository = Object.create(SettingRepository.prototype) as SettingRepository; + const find = jest.fn().mockResolvedValue(result); + Object.defineProperty(repository, 'find', { value: find }); + return { repository, find }; + } + + it('finds status settings in ascending id order', async () => { + const settings = [Object.assign(new Setting(), { id: 1, key: 'paymentStatus', value: 'active' })]; + const { repository, find } = repositoryWithFind(settings); + + await expect(repository.getStatusSettings()).resolves.toBe(settings); + + expect(find).toHaveBeenCalledWith({ + where: { key: Like('%Status') }, + order: { id: 'ASC' }, + }); + }); + + it('passes through an empty result', async () => { + const settings: Setting[] = []; + const { repository } = repositoryWithFind(settings); + + await expect(repository.getStatusSettings()).resolves.toBe(settings); + }); +}); + describe('SettingRepository.setDateMax', () => { function repositoryWithTransaction(transactionManager: Record) { const repository = Object.create(SettingRepository.prototype) as SettingRepository; diff --git a/src/shared/models/setting/setting.repository.ts b/src/shared/models/setting/setting.repository.ts index 25666f97eb..107d5eb7fb 100644 --- a/src/shared/models/setting/setting.repository.ts +++ b/src/shared/models/setting/setting.repository.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import { CachedRepository } from 'src/shared/repositories/cached.repository'; -import { EntityManager } from 'typeorm'; +import { EntityManager, Like } from 'typeorm'; import { Setting } from './setting.entity'; @Injectable() @@ -31,4 +31,8 @@ export class SettingRepository extends CachedRepository { this.invalidateCache(); } + + async getStatusSettings(): Promise { + return this.find({ where: { key: Like('%Status') }, order: { id: 'ASC' } }); + } } diff --git a/src/shared/models/setting/setting.service.ts b/src/shared/models/setting/setting.service.ts index a0b17f0fef..680ae01222 100644 --- a/src/shared/models/setting/setting.service.ts +++ b/src/shared/models/setting/setting.service.ts @@ -16,6 +16,12 @@ export class SettingService { return this.settingRepo.find(); } + // Loads only settings whose key ends with "Status" instead of transferring the entire table: in production, + // getAll() transfers about 1.5 MB, including one entry of about 1.5 MB, to return only a few bytes of status data. + async getStatusSettings(): Promise { + return this.settingRepo.getStatusSettings(); + } + async get(key: string, defaultValue?: string): Promise { return this.settingRepo.findOneBy({ key }).then((d) => d?.value ?? defaultValue); } diff --git a/src/subdomains/core/statistic/__tests__/statistic.service.spec.ts b/src/subdomains/core/statistic/__tests__/statistic.service.spec.ts new file mode 100644 index 0000000000..02efff7a79 --- /dev/null +++ b/src/subdomains/core/statistic/__tests__/statistic.service.spec.ts @@ -0,0 +1,55 @@ +import { createMock } from '@golevelup/ts-jest'; +import { Test, TestingModule } from '@nestjs/testing'; +import { Setting } from 'src/shared/models/setting/setting.entity'; +import { SettingService } from 'src/shared/models/setting/setting.service'; +import { BuyService } from 'src/subdomains/core/buy-crypto/routes/buy/buy.service'; +import { SellService } from 'src/subdomains/core/sell-crypto/route/sell.service'; +import { StatisticService } from 'src/subdomains/core/statistic/statistic.service'; +import { UserService } from 'src/subdomains/generic/user/models/user/user.service'; + +describe('StatisticService', () => { + let service: StatisticService; + let settingService: jest.Mocked; + + beforeEach(async () => { + settingService = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + StatisticService, + { provide: BuyService, useValue: createMock() }, + { provide: SellService, useValue: createMock() }, + { provide: SettingService, useValue: settingService }, + { provide: UserService, useValue: createMock() }, + ], + }).compile(); + + service = module.get(StatisticService); + }); + + describe('getStatus', () => { + it('loads only status settings', async () => { + settingService.getStatusSettings.mockResolvedValue([]); + + await service.getStatus(); + + expect(settingService.getStatusSettings).toHaveBeenCalled(); + expect(settingService.getAll).not.toHaveBeenCalled(); + }); + + it('maps status settings to status keys and values', async () => { + settingService.getStatusSettings.mockResolvedValue([ + Object.assign(new Setting(), { key: 'buyStatus', value: 'Available' }), + Object.assign(new Setting(), { key: 'sellStatus', value: 'Limited' }), + ]); + + await expect(service.getStatus()).resolves.toEqual({ buy: 'Available', sell: 'Limited' }); + }); + + it('returns an empty object when there are no status settings', async () => { + settingService.getStatusSettings.mockResolvedValue([]); + + await expect(service.getStatus()).resolves.toEqual({}); + }); + }); +}); diff --git a/src/subdomains/core/statistic/statistic.service.ts b/src/subdomains/core/statistic/statistic.service.ts index d8ba363623..889efd607e 100644 --- a/src/subdomains/core/statistic/statistic.service.ts +++ b/src/subdomains/core/statistic/statistic.service.ts @@ -41,10 +41,8 @@ export class StatisticService implements OnModuleInit { } async getStatus(): Promise { - const settings = await this.settingService.getAll(); - return settings - .filter((s) => s.key.endsWith('Status')) - .reduce((prev, curr) => ({ ...prev, [curr.key.replace('Status', '')]: curr.value }), {}); + const settings = await this.settingService.getStatusSettings(); + return settings.reduce((prev, curr) => ({ ...prev, [curr.key.replace('Status', '')]: curr.value }), {}); } getAll(): StatisticDto {