From 045e6f8d637571a9e1acff16e7c778e917626493 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:27:12 +0200 Subject: [PATCH 1/3] Add HTTP coverage for gs/db trigger rejection (#4477) --- .../gs/__tests__/gs.controller.e2e.spec.ts | 136 ++++++++++++++++-- .../gs/__tests__/gs.controller.spec.ts | 34 ++--- 2 files changed, 132 insertions(+), 38 deletions(-) diff --git a/src/subdomains/generic/gs/__tests__/gs.controller.e2e.spec.ts b/src/subdomains/generic/gs/__tests__/gs.controller.e2e.spec.ts index ffd365a6b7..4f0c14f246 100644 --- a/src/subdomains/generic/gs/__tests__/gs.controller.e2e.spec.ts +++ b/src/subdomains/generic/gs/__tests__/gs.controller.e2e.spec.ts @@ -1,6 +1,9 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { Body, + CanActivate, Controller, + ExecutionContext, INestApplication, MiddlewareConsumer, Module, @@ -9,12 +12,19 @@ import { ValidationPipe, VersioningType, } from '@nestjs/common'; +import { GUARDS_METADATA } from '@nestjs/common/constants'; import { Test } from '@nestjs/testing'; import * as bodyParser from 'body-parser'; import request from 'supertest'; import { GetConfig } from 'src/config/config'; +import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; +import { UserRole } from 'src/shared/auth/user-role.enum'; +import { DfxLogger } from 'src/shared/services/dfx-logger'; +import * as processServiceModule from 'src/shared/services/process.service'; import { DbQueryDto, DbReturnData } from 'src/subdomains/generic/gs/dto/db-query.dto'; import { GsTriggerType } from 'src/subdomains/generic/gs/dto/gs-trigger-type.enum'; +import { GsController } from 'src/subdomains/generic/gs/gs.controller'; +import { GsService } from 'src/subdomains/generic/gs/gs.service'; import { DebugQueryDto, DebugQueryResult } from '../dto/debug-query.dto'; import { DebugQueryTreeSizeMiddleware } from '../middleware/debug-query-tree-size.middleware'; @@ -67,20 +77,10 @@ class GsControllerTestModule { } } -// Test-only route for the `/gs/db` request pipeline (production file: `gs.controller.ts`). The -// production `GsController` is NOT bootstrapped here, for the same reason `GsDebugTestController` -// above isn't: `RoleGuard()` and `UserActiveGuard()` return already-instantiated guard objects -// baked into `@UseGuards()` at controller-decoration time in `gs.controller.ts`, so calling -// `RoleGuard()` / `UserActiveGuard()` again in this file creates different instances that -// `Test.overrideGuard()` cannot match. -// -// This controller deliberately does NOT reproduce the trigger-enforcement check. That check is -// exercised against the REAL `GsController` in the unit test `gs.controller.spec.ts`; duplicating it here -// would just be two tests for the same logic. This fixture covers the full DbQueryDto / -// ValidationPipe surface (not only the trigger field) — what only the full NestJS pipeline -// can prove: that the real `DbQueryDto` decorators (`@IsEnum(GsTriggerType)`, -// `@MaxLength(256)` on `table`/`identifier`, control-character rejection, etc.) are actually -// wired into the global `ValidationPipe`. +// Test-only route that isolates the `DbQueryDto` / ValidationPipe surface from controller +// behavior. It proves the real DTO decorators (`@IsEnum(GsTriggerType)`, `@MaxLength(256)` on +// `table`/`identifier`, control-character rejection, etc.) are wired into the global pipe and +// deliberately leaves trigger enforcement to the real-controller HTTP suite below. @Controller('gs') class GsDbQueryDtoTestController { @Post('db') @@ -291,3 +291,111 @@ describe('GsController e2e (db query DTO validation)', () => { .expect(201); }); }); + +describe('GsController e2e (missing trigger enforcement)', () => { + let app: INestApplication; + let service: DeepMocked; + let verboseSpy: jest.SpyInstance; + + const jwt: JwtPayload = { role: UserRole.ADMIN, ip: '1.2.3.4' }; + const allowAdminGuard: CanActivate = { + canActivate(context: ExecutionContext): boolean { + context.switchToHttp().getRequest<{ user?: JwtPayload }>().user = jwt; + return true; + }, + }; + + beforeAll(async () => { + service = createMock(); + verboseSpy = jest.spyOn(DfxLogger.prototype, 'verbose').mockImplementation(); + jest.spyOn(processServiceModule, 'DisabledProcess').mockReturnValue(false); + + const builder = Test.createTestingModule({ + controllers: [GsController], + providers: [{ provide: GsService, useValue: service }], + }); + const handlers = [GsController.prototype.getDbData, GsController.prototype.getExtendedData]; + const guards = handlers.flatMap((handler) => Reflect.getMetadata(GUARDS_METADATA, handler) as CanActivate[]); + + for (const guard of guards) { + if (typeof guard === 'function') { + builder.overrideGuard(guard).useValue(allowAdminGuard); + } else { + jest.spyOn(guard, 'canActivate').mockImplementation(allowAdminGuard.canActivate.bind(allowAdminGuard)); + } + } + + const moduleRef = await builder.compile(); + app = moduleRef.createNestApplication(); + app.enableVersioning({ type: VersioningType.URI, defaultVersion: [GetConfig().defaultVersion] }); + app.use(bodyParser.json({ limit: '20mb' })); + app.useGlobalPipes( + new ValidationPipe({ + whitelist: true, + transformOptions: { exposeUnsetFields: false }, + }), + ); + await app.init(); + }); + + afterAll(async () => { + try { + if (app) await app.close(); + } finally { + jest.restoreAllMocks(); + } + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it.each(['/v1/gs/db', '/v1/gs/db/custom'])( + 'rejects a missing trigger on %s before calling either GS service', + async (path) => { + const response = await request(app.getHttpServer()).post(path).send({ table: 'asset' }).expect(400); + + expect(response.body.message).toBe('Trigger type is required'); + expect(verboseSpy).toHaveBeenCalledTimes(1); + expect(verboseSpy).toHaveBeenCalledWith( + 'GS db call: table=asset, identifier=missing, trigger=missing, role=Admin', + ); + expect(service.getDbData).not.toHaveBeenCalled(); + expect(service.getExtendedDbData).not.toHaveBeenCalled(); + }, + ); + + it('routes a valid /gs/db request to getDbData only', async () => { + const result: DbReturnData = { keys: ['standard'], values: [{ id: 1 }] }; + service.getDbData.mockResolvedValue(result); + + const response = await request(app.getHttpServer()) + .post('/v1/gs/db') + .send({ table: 'asset', trigger: GsTriggerType.MANUAL }) + .expect(201); + + expect(response.body).toEqual(result); + expect(service.getDbData).toHaveBeenCalledWith( + expect.objectContaining({ table: 'asset', trigger: GsTriggerType.MANUAL }), + UserRole.ADMIN, + ); + expect(service.getExtendedDbData).not.toHaveBeenCalled(); + }); + + it('routes a valid /gs/db/custom request to getExtendedDbData only', async () => { + const result: DbReturnData = { keys: ['custom'], values: [{ id: 2 }] }; + service.getExtendedDbData.mockResolvedValue(result); + + const response = await request(app.getHttpServer()) + .post('/v1/gs/db/custom') + .send({ table: 'asset', trigger: GsTriggerType.AUTO }) + .expect(201); + + expect(response.body).toEqual(result); + expect(service.getExtendedDbData).toHaveBeenCalledWith( + expect.objectContaining({ table: 'asset', trigger: GsTriggerType.AUTO }), + UserRole.ADMIN, + ); + expect(service.getDbData).not.toHaveBeenCalled(); + }); +}); diff --git a/src/subdomains/generic/gs/__tests__/gs.controller.spec.ts b/src/subdomains/generic/gs/__tests__/gs.controller.spec.ts index 911211040d..a9fe37e0df 100644 --- a/src/subdomains/generic/gs/__tests__/gs.controller.spec.ts +++ b/src/subdomains/generic/gs/__tests__/gs.controller.spec.ts @@ -9,15 +9,10 @@ import { GsTriggerType } from 'src/subdomains/generic/gs/dto/gs-trigger-type.enu import { GsController } from 'src/subdomains/generic/gs/gs.controller'; import { GsService } from 'src/subdomains/generic/gs/gs.service'; -// Unit-level regression coverage for `GsController`'s private `logAndCheckTrigger` helper -// (called from both `getDbData` and `getExtendedData`), exercised against the REAL controller — -// unlike `gs.controller.e2e.spec.ts`, which cannot bootstrap the real `GsController` through -// NestJS' HTTP pipeline: `RoleGuard()` / `UserActiveGuard()` bake already-instantiated guard -// objects into `@UseGuards()` at controller-decoration time, so a fresh `Test.overrideGuard()` -// call from a test module can't target them. Guards are a framework layer wrapped around the -// controller, not part of the method itself, so a direct `new GsController(service)` plus -// a plain method call sidesteps that problem entirely and exercises the actual production code -// path this feature touches. +// Direct regression coverage for `GsController`'s private `logAndCheckTrigger` helper (called +// from both handlers). Calling the real controller without the NestJS wrapper lets this suite +// assert the synchronous audit/service side effects before the returned Promise settles. The +// E2E suite separately covers both handlers through NestJS routing and exception mapping. describe('GsController', () => { let service: DeepMocked; let controller: GsController; @@ -56,26 +51,17 @@ describe('GsController', () => { for (const { name, call, serviceCall } of handlers) { describe(name, () => { it('rejects a request without trigger, logs first, and never calls the GS service', async () => { - const started = performance.now(); - let caught: unknown; - try { - await call(query({})); - } catch (e) { - caught = e; - } - const elapsed = performance.now() - started; - - expect(caught).toBeInstanceOf(BadRequestException); - expect((caught as BadRequestException).message).toBe('Trigger type is required'); - // Structural invariant: audit line is emitted before rejection, service is never entered. + const promise = call(query({})); + + // Structural invariant: audit line is emitted and service is never entered before rejection settles. expect(verboseSpy).toHaveBeenCalledTimes(1); expect(verboseSpy).toHaveBeenCalledWith( 'GS db call: table=asset, identifier=missing, trigger=missing, role=Admin', ); expect(serviceCall()).not.toHaveBeenCalled(); - // Rejection path is synchronous (no SettingService/DB await). Keep a modest SLA so - // a regression that re-introduces awaited work fails the suite without relying on load. - expect(elapsed).toBeLessThan(1000); + + await expect(promise).rejects.toBeInstanceOf(BadRequestException); + await expect(promise).rejects.toThrow('Trigger type is required'); }); it('accepts trigger=Manual', async () => { From 9010318c2da661f940046e581246a365f2bd0cae Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 09:16:48 +0200 Subject: [PATCH 2/3] e710bc80 - Route EUR deposits to Bank Frick (#4470) * feat(bank): make the deposit-target bank an operational input The customer-facing deposit selector filtered Bank Frick out by name, so switching a currency to a different receiving bank required a deploy. Replace that hardcoded exclusion with a Bank.receivePriority column, mirroring the existing Bank.sendPriority tie-breaker: lower value wins, ties broken by ascending id. Deploying this changes nothing on its own - every row is backfilled to the neutral default, so the incumbent banks keep their currencies until the priority is deliberately lowered. The migration is schema-only and never touches bank rows, per the established convention for that table. * test(bank): build the selector fixtures as real Bank instances A plain spread of the shared mocks produces an object literal without the entity methods, so it does not satisfy the repository's Bank[] contract and fails the type check. Route the id-carrying copies through createCustomBank, which assigns onto a real Bank. * fix(bank): make deposit-target eligibility explicit instead of implied by rank Review found that ranking alone does not preserve today's behaviour. The removed filter excluded a bank categorically; a low rank only deprioritises it. Whenever the incumbent for a currency is missing or receive=false, or the sctInst lookup falls through to the general fallback, the previously excluded bank would be shown to a customer without anyone switching it on. Make receivePriority nullable with NULL meaning 'never offered as a deposit target', and filter on it before ranking. Eligibility is deliberately not derived from receive: a bank can need to accept and reconcile incoming money without being advertised. The migration now backfills exactly the banks the old name filter already allowed, so the deploy freezes today's routing instead of merely making a change unlikely. * refactor(bank): declare the selector's undefined result and correct a stale comment getBank and getMatchingBank returned Bank while both find() calls can yield undefined. That was already true, but explicit eligibility makes it a documented outcome: a bank left at NULL priority is skipped even as the last candidate, so no match is a state the callers must handle. Both of them already did. Also correct the payout comment that claimed to mirror a bank-name exclusion in the deposit selector - that exclusion no longer exists there. * fix(bank): give the new migration a fresh identity and pin sctInst eligibility TypeORM identifies a migration by its class name, not its file contents. This migration was edited in place after its first version already existed on the branch, so any database that had run the old NOT NULL DEFAULT 1000 version would never run the corrected one and would keep every bank eligible. Production has not run it (no such row in migrations), but a local database might have, so the migration gets a new timestamp and class name. If the old version did run somewhere, ADD COLUMN now fails loudly instead of leaving a silently wrong state. Also add the missing INSTANT case: a NULL-priority bank carrying sctInst must not win the instant branch, which pins that eligibility is filtered before the sctInst lookup rather than after it. * refactor(buy): state the non-null precondition of the bank response builder Widening getBank to Bank | undefined widened this helper's parameter with it, while the body dereferences the bank unconditionally. The caller already rejects the undefined case, so the signature should carry that precondition rather than claim to accept a value it cannot handle. * fix(seed): seed receivePriority so a fresh database has eligible deposit banks The selector skips any bank whose receivePriority is NULL. The migration backfills rows that already exist, but a fresh database is migrated while the bank table is still empty, so the backfill touches nothing and the seed then inserts every bank without the column - leaving local, CI and onboarding environments with no eligible deposit target at all. Seed the column explicitly: 1000 for the banks the selector already offered, empty (NULL) for the dormant Bank Frick rows, mirroring the split the migration applies to an existing database. * docs(bank): scope the receivePriority guarantee to the generic selector The column comment claimed a bank at NULL is never offered to a customer as a deposit target. That overstates it: the explicit personal-IBAN path resolves its own bank and is not gated by this column - it never was, and the bank-name filter this replaced did not cover it either. A comment that promises a guarantee the code does not hold is worse than none, so state the scope and note the cache delay that applies to receive just the same. * docs(bank): scope the remaining eligibility statements to the generic selector The entity comment was corrected, but the migration docstring and the seed comment still claimed a NULL leaves no eligible deposit target at all. Explicit personal-IBAN paths are not gated by this column, so both now name the selector they actually describe. * docs(bank): spell out that receive and receivePriority are ANDed The comment said a number makes a bank eligible, next to a line saying eligibility is not derived from receive. Read together that suggests the priority alone decides. It does not: the selector starts from receive=true rows, so a receive=false bank stays excluded whatever its priority. * refactor(bank): route EUR deposits to Bank Frick in code Replaces the data-driven receivePriority approach with the hardcoded rule the product decision calls for: an EUR bank transfer is routed to Bank Frick. The column, its migration, the seed entries and the debug allowlist entry are removed again. Other receiving banks stay a fallback: if the Frick EUR row is not receiving, EUR deposits keep working through the incumbents rather than failing. SEPA Instant is exempt because Bank Frick does not offer it, so an instant request still reaches a bank that can execute it. Switching this back now requires a release rather than a data change. * test(bank): drop the import left unused by the removed priority cases * test(bank): pin the currency check in the Frick EUR rule Removing `bank.currency === 'EUR'` from the rule left the whole suite green, so nothing guarded it. The rule matches on name and currency and find() takes the first hit, so without that check an EUR deposit could be handed the franc account's IBAN. The CHF row is listed first to make the case bite. * fix(bank): keep Bank Frick out of every path the EUR rule does not cover The removed bank-name filter excluded Bank Frick from the whole selector. Replacing it with an EUR-only rule dropped that exclusion everywhere else, so Frick became a candidate again in paths the rule never claimed: - a CHF request could return the Frick franc row, which is receive=true in production, depending on a database order that is not guaranteed - an EUR instant request with no sctInst bank available fell through to the generic fallback and could return Frick there - the exact case the instant exemption exists to prevent - the condition tested paymentMethod !== INSTANT, which is also true for CARD Scope the rule positively to BANK, and restore the categorical exclusion for everything after it. Resolve several qualifying Frick rows by highest id, the way getBankInternal already resolves an ambiguous (name, currency) pair. One test asserted the fallback behaviour that was the defect; it now asserts the incumbent wins, in both input orders. * fix(bank): resolve the Frick EUR row through the attribution mechanism The tie-breaker sorted by highest id while claiming to mirror getBankInternal. It did not: selectAttributionBank prefers the asset-linked row, because that binding is what isBankMatching and the booked bank_tx history are keyed on. With two active Frick EUR rows the customer would have been shown the newer row's IBAN while attribution stayed on the older one, so incoming payments would not match and book with pendingInputAmount 0 - the netting skew the code warns about at that very function. getReceiveBanks does not load the asset relation either, so the preference could never have applied there. Resolve through getBankInternal instead, which is cached and does load it, and keep the receive check since it does not filter on that. * test(bank): pin that no other Frick row substitutes the attributed one When the asset-linked Frick EUR row is not receiving, a second unbound Frick row must not stand in for it: attribution stays on the disabled row, so a payment into the unbound IBAN would book against a row nothing is keyed on. Also scope the comment above the rule. It aligns the selection rule with attribution, not the caches - ibanCache is loaded once at module init while this read goes through the repository cache, so a row edited at runtime can still be seen differently by the two until restart. That gap predates this rule and applies to every bank, and the comment should not read as a promise that they can never disagree. * test(bank): reject array filters in the findCached mock instead of casting The helper cast the where clause to the object variant, discarding the array form the signature allows. A future array filter would then have matched nothing and silently returned every bank instead of failing. Its sibling three lines up already rejects that case explicitly; do the same here. --- .../core/buy-crypto/routes/buy/buy.service.ts | 4 +- .../bank/bank/__tests__/bank.service.spec.ts | 236 ++++++++++++++++-- .../supporting/bank/bank/bank.service.ts | 38 ++- .../fiat-output/fiat-output.service.ts | 7 +- 4 files changed, 250 insertions(+), 35 deletions(-) diff --git a/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts b/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts index 72a78be438..cc610c8efc 100644 --- a/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts +++ b/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts @@ -554,8 +554,10 @@ export class BuyService { }; } + // getBank() can return undefined, but this builder dereferences the bank unconditionally - callers + // must resolve that before calling in, so the parameter states the precondition instead of widening. private buildBankResponse( - bank: Awaited>, + bank: NonNullable>>, reference?: string, ): BankInfoDto & { isPersonalIban: boolean; reference?: string } { return { diff --git a/src/subdomains/supporting/bank/bank/__tests__/bank.service.spec.ts b/src/subdomains/supporting/bank/bank/__tests__/bank.service.spec.ts index 8aa15e736b..0a640399ad 100644 --- a/src/subdomains/supporting/bank/bank/__tests__/bank.service.spec.ts +++ b/src/subdomains/supporting/bank/bank/__tests__/bank.service.spec.ts @@ -16,6 +16,7 @@ import { createCustomVirtualIban } from 'src/subdomains/supporting/bank/virtual- import { VirtualIban, VirtualIbanStatus } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban.entity'; import { VirtualIbanRepository } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban.repository'; import { FiatPaymentMethod } from 'src/subdomains/supporting/payment/dto/payment-method.enum'; +import { FindOneOptions, FindOptionsWhere } from 'typeorm'; import { createCustomBank, createDefaultBanks, @@ -24,7 +25,6 @@ import { yapealEUR, olkyEUR, frickEUR, - frickCHF, } from '../__mocks__/bank.entity.mock'; import { Bank } from '../bank.entity'; import { BankRepository } from '../bank.repository'; @@ -46,6 +46,36 @@ function createBankSelectorInput( }; } +function mockFindCachedByForBanks(bankRepo: BankRepository, banks: Bank[]): void { + jest + .spyOn(bankRepo, 'findCachedBy') + .mockImplementation(async (_key: number | string, where: FindOptionsWhere | FindOptionsWhere[]) => { + // getReceiveBanks always supplies one object; fail visibly if that contract changes. + if (Array.isArray(where)) throw new Error('mockFindCachedByForBanks does not support array filters'); + + const receive = where.receive; + if (typeof receive === 'boolean') return banks.filter((bank) => bank.receive === receive); + return banks; + }); +} + +function mockFindCachedForBanks(bankRepo: BankRepository, banks: Bank[]): void { + jest + .spyOn(bankRepo, 'findCached') + .mockImplementation(async (_key: number | string, options?: FindOneOptions) => { + const where = options?.where; + if (!where) return banks; + // getBankInternal always supplies one object; fail visibly if that contract changes, rather + // than casting the array variant away and silently matching nothing. + if (Array.isArray(where)) throw new Error('mockFindCachedForBanks does not support array filters'); + return banks.filter( + (bank) => + (where.name === undefined || bank.name === where.name) && + (where.currency === undefined || bank.currency === where.currency), + ); + }); +} + describe('BankService', () => { let service: BankService; @@ -88,12 +118,8 @@ describe('BankService', () => { .mockResolvedValue(createCustomCountry({ yapealEnable: yapealEnable })); const allBanks = disabledBank ? createDefaultDisabledBanks() : createDefaultBanks(); - jest.spyOn(bankRepo, 'findCachedBy').mockImplementation(async (_key: string, filter?: any) => { - if (filter?.receive !== undefined) { - return allBanks.filter((b) => b.receive === filter.receive); - } - return allBanks; - }); + mockFindCachedByForBanks(bankRepo, allBanks); + mockFindCachedForBanks(bankRepo, []); } it('should be defined', () => { @@ -153,23 +179,191 @@ describe('BankService', () => { expect(result.bic).toBe(yapealEUR.bic); }); - it('never offers Bank Frick as a deposit bank, even when it is the first receive bank for the currency', async () => { - // Frick is placed first so a missing exclusion guard would wrongly select it; the customer must still - // be shown the incumbent bank for each currency. - // A preceding test disables the shared olkyEUR mock in place (createDefaultDisabledBanks mutates it), - // so restore its natural receive state here to exercise Frick exclusion rather than that leaked state. - olkyEUR.receive = true; - const frickFirst = [frickEUR, frickCHF, olkyEUR, yapealEUR, yapealCHF]; - jest.spyOn(bankRepo, 'findCachedBy').mockImplementation(async (_key: string, filter?: any) => { - if (filter?.receive !== undefined) return frickFirst.filter((b) => b.receive === filter.receive); - return frickFirst; + it('routes BANK EUR deposits to Bank Frick regardless of bank order', async () => { + const incumbent = createCustomBank({ ...olkyEUR, receive: true }); + const frick = createCustomBank({ ...frickEUR, receive: true }); + mockFindCachedByForBanks(bankRepo, [incumbent, frick]); + mockFindCachedForBanks(bankRepo, [incumbent, frick]); + + const result = await service.getBank(createBankSelectorInput('EUR', undefined, FiatPaymentMethod.BANK)); + expect(result).toBe(frick); + }); + + it('prefers the older asset-linked Bank Frick EUR row for a BANK EUR deposit', async () => { + // Production shape: the older row owns the custody asset and its IBAN is used by isBankMatching + // and the Financial Log. Returning the newer unbound row would detach the customer IBAN from + // that attribution. The mock order mirrors `order: { id: 'DESC' }` from getBankInternal. + const assetLinkedFrick = Object.assign( + createCustomBank({ + ...frickEUR, + id: 101, + receive: true, + iban: 'LI75088110105923K0101', + }), + { asset: {} }, + ); + const unboundNewerFrick = createCustomBank({ + ...frickEUR, + id: 202, + receive: true, + iban: 'LI75088110105923K0202', + }); + const banks = [unboundNewerFrick, assetLinkedFrick]; + mockFindCachedByForBanks(bankRepo, banks); + mockFindCachedForBanks(bankRepo, banks); + + const result = await service.getBank(createBankSelectorInput('EUR', undefined, FiatPaymentMethod.BANK)); + expect(result).toBe(assetLinkedFrick); + expect(result.iban).toBe('LI75088110105923K0101'); + }); + + it('picks the EUR Bank Frick row, not its CHF row, for an EUR deposit', async () => { + // The CHF row is listed first on purpose: the getBankInternal query matches on bank name AND + // currency. Without the currency check a customer paying in EUR could be handed the franc + // account's IBAN, and no other test in this file would notice. + const frickChfRow = createCustomBank({ + name: IbanBankName.FRICK, + currency: 'CHF', + receive: true, + iban: 'FRICK-CHF-ROW', + bic: 'BFRILI22', + }); + const frickEurRow = createCustomBank({ ...frickEUR, receive: true }); + mockFindCachedByForBanks(bankRepo, [frickChfRow, frickEurRow]); + mockFindCachedForBanks(bankRepo, [frickChfRow, frickEurRow]); + + const result = await service.getBank(createBankSelectorInput('EUR')); + expect(result).toBe(frickEurRow); + expect(result.currency).toBe('EUR'); + }); + + it('falls back to the established EUR receiver when Bank Frick is not receiving', async () => { + const disabledFrick = createCustomBank({ ...frickEUR, receive: false }); + const incumbent = createCustomBank({ ...olkyEUR, receive: true }); + mockFindCachedByForBanks(bankRepo, [disabledFrick, incumbent]); + mockFindCachedForBanks(bankRepo, [disabledFrick, incumbent]); + + const result = await service.getBank(createBankSelectorInput('EUR')); + expect(result).toBe(incumbent); + }); + + it('does not substitute another Bank Frick row when the attributed one is not receiving', async () => { + // The attributed (asset-linked) row is disabled while a second, unbound Frick row still receives. + // The rule must not fall through to that one: attribution stays on the disabled row, so paying + // into the unbound IBAN would book against a row nothing is keyed on. The incumbent wins instead. + const attributedDisabled = createCustomBank({ + ...frickEUR, + id: 19, + receive: false, + asset: createCustomAsset({}), + iban: 'FRICK-ATTRIBUTED-DISABLED', + }); + const unboundReceiving = createCustomBank({ + ...frickEUR, + id: 77, + receive: true, + asset: null, + iban: 'FRICK-UNBOUND-RECEIVING', + }); + const incumbent = createCustomBank({ ...olkyEUR, receive: true }); + mockFindCachedByForBanks(bankRepo, [attributedDisabled, unboundReceiving, incumbent]); + mockFindCachedForBanks(bankRepo, [attributedDisabled, unboundReceiving]); + + const result = await service.getBank(createBankSelectorInput('EUR')); + expect(result).toBe(incumbent); + }); + + it('leaves CHF bank selection unaffected by the Bank Frick EUR rule', async () => { + const frick = createCustomBank({ ...frickEUR, receive: true }); + const chf = createCustomBank({ ...yapealCHF, receive: true }); + mockFindCachedByForBanks(bankRepo, [frick, chf]); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('CHF')); + expect(result).toBe(chf); + }); + + it('does not let a Bank Frick CHF row capture a CHF request', async () => { + const frickChf = createCustomBank({ + name: IbanBankName.FRICK, + currency: 'CHF', + receive: true, + iban: 'LI75088110105923K0CHF', + bic: 'BFRILI22', }); + const chf = createCustomBank({ ...yapealCHF, receive: true }); + mockFindCachedByForBanks(bankRepo, [frickChf, chf]); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('CHF')); + expect(result).toBe(chf); + }); + + it('uses an instant-capable EUR bank instead of Bank Frick for INSTANT payments', async () => { + const frick = createCustomBank({ ...frickEUR, receive: true, sctInst: false }); + const instantBank = createCustomBank({ ...olkyEUR, receive: true, sctInst: true }); + mockFindCachedByForBanks(bankRepo, [frick, instantBank]); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('EUR', undefined, FiatPaymentMethod.INSTANT)); + expect(result).toBe(instantBank); + }); + + it('falls back to an incumbent EUR bank when no EUR bank supports INSTANT', async () => { + const frick = createCustomBank({ ...frickEUR, receive: true, sctInst: false }); + const incumbent = createCustomBank({ ...olkyEUR, receive: true, sctInst: false }); + mockFindCachedByForBanks(bankRepo, [frick, incumbent]); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('EUR', undefined, FiatPaymentMethod.INSTANT)); + expect(result).toBe(incumbent); + expect(result).not.toBe(frick); + }); + + it('uses an incumbent EUR bank instead of Bank Frick for CARD payments', async () => { + const frick = createCustomBank({ ...frickEUR, receive: true }); + const incumbent = createCustomBank({ ...olkyEUR, receive: true }); + mockFindCachedByForBanks(bankRepo, [frick, incumbent]); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('EUR', undefined, FiatPaymentMethod.CARD)); + expect(result).toBe(incumbent); + expect(result).not.toBe(frick); + }); + + it.each([ + ['Bank Frick first', true], + ['incumbent first', false], + ])('uses the incumbent EUR fallback for unsupported currency with %s', async (_description, frickFirst) => { + const frick = createCustomBank({ ...frickEUR, receive: true }); + const incumbent = createCustomBank({ ...olkyEUR, receive: true }); + const banks = frickFirst ? [frick, incumbent] : [incumbent, frick]; + mockFindCachedByForBanks(bankRepo, banks); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('GBP')); + expect(result).toBe(incumbent); + expect(result).not.toBe(frick); + }); + + it('returns undefined when neither the requested currency nor the EUR fallback can receive', async () => { + const disabledGbp = createCustomBank({ currency: 'GBP', receive: false }); + const disabledEur = createCustomBank({ ...olkyEUR, receive: false }); + mockFindCachedByForBanks(bankRepo, [disabledGbp, disabledEur]); + mockFindCachedForBanks(bankRepo, []); + + const result = await service.getBank(createBankSelectorInput('GBP')); + expect(result).toBeUndefined(); + }); - const eur = await service.getBank(createBankSelectorInput('EUR')); - expect(eur.name).toBe(IbanBankName.OLKY); + it('uses an instant-capable EUR fallback when GBP has no instant account', async () => { + const gbpBank = createCustomBank({ currency: 'GBP', receive: true, sctInst: false }); + const eurInstantBank = createCustomBank({ ...olkyEUR, receive: true, sctInst: true }); + mockFindCachedByForBanks(bankRepo, [gbpBank, eurInstantBank]); + mockFindCachedForBanks(bankRepo, []); - const chf = await service.getBank(createBankSelectorInput('CHF', 10000)); - expect(chf.name).toBe(IbanBankName.YAPEAL); + const result = await service.getBank(createBankSelectorInput('GBP', undefined, FiatPaymentMethod.INSTANT)); + expect(result).toBe(eurInstantBank); }); }); diff --git a/src/subdomains/supporting/bank/bank/bank.service.ts b/src/subdomains/supporting/bank/bank/bank.service.ts index 50bb04a34d..a23a8b3d44 100644 --- a/src/subdomains/supporting/bank/bank/bank.service.ts +++ b/src/subdomains/supporting/bank/bank/bank.service.ts @@ -85,19 +85,37 @@ export class BankService implements OnModuleInit { } // --- BANK SELECTOR --- // - async getBank({ currency, paymentMethod }: BankSelectorInput): Promise { + // Returns undefined when no eligible receiving (receive=true) bank exists for either the requested + // currency or the EUR fallback. + async getBank({ currency, paymentMethod }: BankSelectorInput): Promise { const fallBackCurrency = 'EUR'; - // Bank Frick's rows are receive=true so money arriving on its accounts is fully processed, but it - // must never be offered to a customer as a deposit target - customers are always shown the incumbent - // banks (Olkypay/Yapeal). It is deliberately filtered out of this customer-facing selector here; - // inbound crediting runs via BankTxFrickService, not this path, and the outbound payout selector - // applies its own separate Frick handling, so the exclusion affects only the deposit IBAN shown to - // customers. - const banks = (await this.getReceiveBanks()).filter((b) => b.name !== IbanBankName.FRICK); + const receiveBanks = await this.getReceiveBanks(); + + // Product decision, deliberately hardcoded: an EUR bank transfer is routed to Bank Frick. + // Resolved through getBankInternal so this picks the row selectAttributionBank would pick - + // (name, currency) is not unique, and the asset-linked identity is the one isBankMatching and the + // booked bank_tx history are keyed on. Choosing by any other rule here, e.g. the newest row, would + // hand out an IBAN that attribution does not follow. + // This aligns the selection RULE, not the caches: ibanCache is loaded once at module init while + // this read goes through the repository cache, so a bank row edited at runtime can still be seen + // differently by the two until the process restarts. That gap predates this rule and applies to + // every bank; do not read this call as a guarantee that the two can never disagree. + // The rule is scoped to exactly EUR + BANK; receive must still hold, since getBankInternal does + // not filter on it - and if the attributed row is not receiving, no other Frick row stands in for + // it, because the exclusion below then applies to all of them. + if (currency === 'EUR' && paymentMethod === FiatPaymentMethod.BANK) { + const frickEur = await this.getBankInternal(IbanBankName.FRICK, 'EUR'); + if (frickEur?.receive) return frickEur; + } + + // Everything below keeps the categorical exclusion the removed bank-name filter provided: Bank + // Frick must not win a currency it was never routed to, and must not be reachable through the + // instant lookup or the EUR currency fallback either. Only the explicit rule above may return it. + const banks = receiveBanks.filter((bank) => bank.name !== IbanBankName.FRICK); // select the matching bank account - let account: Bank; + let account: Bank | undefined; // instant bank if (!account && paymentMethod === FiatPaymentMethod.INSTANT) { @@ -117,7 +135,7 @@ export class BankService implements OnModuleInit { currencyName: string, fallBackCurrencyName: string, selector?: (bank: Bank) => boolean, - ): Bank { + ): Bank | undefined { const matchingBanks = selector ? banks.filter(selector) : banks; return ( diff --git a/src/subdomains/supporting/fiat-output/fiat-output.service.ts b/src/subdomains/supporting/fiat-output/fiat-output.service.ts index 260147d99e..2c3bc47ceb 100644 --- a/src/subdomains/supporting/fiat-output/fiat-output.service.ts +++ b/src/subdomains/supporting/fiat-output/fiat-output.service.ts @@ -59,9 +59,10 @@ export class FiatOutputService { return { accountIban: virtualIban.iban, bank: virtualIban.bank }; } - // Automatic sender-bank selection is incumbent-banks-only. Bank Frick is payout-eligible exclusively - // through explicit per-output assignment (accountIban at creation or manual database assignment), - // mirroring the deliberate exclusion in BankService.getBank() for the customer-facing deposit selector. + // Automatic payout selection excludes Bank Frick by name; it is payout-eligible exclusively through + // explicit per-output assignment (accountIban at creation or manual database assignment). This is + // independent of the customer-facing deposit direction: BankService.getBank() deliberately routes + // EUR deposits to Bank Frick. The payout exclusion and deposit routing therefore do not conflict. const banks = (await this.bankService.getSenderBanks(currency)).filter( (candidate) => candidate.name !== IbanBankName.FRICK, ); From b2553d2bf6879af6cad5cf0e75723f6e6658d9d2 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Thu, 30 Jul 2026 09:18:02 +0200 Subject: [PATCH 3/3] 7392bb63 - Index the ledger content-change scan and project financial-log fields in SQL (#4480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(ledger): index the content-change scan and project financial-log fields in SQL Two independent sources of database load on dfxprd, both measured in production. 1. The nine ledger consumers each run a (updated, id) keyset content-change scan every minute, but none of their source tables had an index on `updated`. EXPLAIN (ANALYZE, BUFFERS) for trading_order showed a Parallel Seq Scan reading ~494 MB from disk and filtering 5,418,069 rows to return 4, at 151.765 ms per call. Add composite (updated, id) indexes on all nine source tables. 2. GET /v1/dashboard/financial/log selected every matching row including the ~42 KB `message` JSON: 1,316 MB of message payload across the 31,925 matching rows, requested twice per minute, with a 6.6 s median and 29.8 s maximum query time. The endpoint needs only four `balancesTotal` scalars, one BTC price and the `balancesByFinancialType` sub-tree — `assets` (1,346 MB) and `tradings` (43 MB) were never read. Project just those sub-trees in SQL. The response is unchanged for the same underlying data. Malformed `message` JSON now aborts the query instead of silently dropping the row: for a financial dashboard a visible failure is preferable to an unnoticed gap in the series. Verified that all 31,925 matching rows currently hold valid JSON. * style: apply prettier formatting to log repository spec * fix: use deterministic TypeORM index names in ledger scan migration Replace the nine self-invented index names in AddLedgerContentChangeScanIndexes1785460000000 with the deterministic names TypeORM's DefaultNamingStrategy would generate itself, per the "No custom naming for TypeORM indexes" rule. Also documents the derivation in the migration's docstring so the names can be recomputed. * fix(log): address financial-log projection review findings - Restore the falsy btcAssetId check (0/undefined/null all take the no-BTC-asset path) that extractBtcPrice used to have, instead of only excluding undefined. - Guard all five projected numbers (totalBalanceChf, plusBalanceChf, minusBalanceChf, btcPriceChf, fxPnlChf) with a jsonb_typeof check so one bad value nulls only that field instead of aborting the whole query, and align the fxPnlChf guard with the same pattern used by the neighboring priceChf projection. - Remove the now-unused extractBtcPrice helper. - Add coverage for the ORDER BY direction, the exact $N parameter position per SQL condition, and the projection's edge cases (btcAssetId 0/undefined, missing/null balancesTotal fields), and drop assertions that only read back their own test fixture. * docs(migration): correct the lock-behaviour analysis for the index migration The previous note claimed a CREATE INDEX SHARE lock is held only for the duration of that one index build, and justified the write-blocking risk for trading_order on that basis. That was wrong. All nine CREATE INDEX statements run inside a single transaction: TypeORM's migrationsTransactionMode defaults to "all" (DataSource.js:263-265) and config.ts never overrides it, and MigrationExecutor.js:206 opens one transaction for all pending migrations. PostgreSQL releases locks at COMMIT, not per statement. The write-blocking window for trading_order is therefore the sum of all nine builds, and once the transaction commits all nine tables are simultaneously write-blocked -- including bank_tx, buy_crypto, crypto_input and payout_order. Splitting this across several migration files would not change that. Also drop the redundant SET LOCAL lock_timeout calls. SET LOCAL is scoped to the transaction, so 18 of the 19 had no effect and implied a per-statement guard that does not exist; one call in up() and one in down() remain. * fix(log): stop defaulting SQL NULL to 0 in the financial-log projection totalBalanceChf/plusBalanceChf/minusBalanceChf now stay null when the SQL projection nulls them, matching the fxPnlChf handling already in place; the `?? 0` default remains solely at the mapSummaryToEntry call site. Also drop the Number() coercion in the balancesByType loop so a genuinely missing key passes through unconverted instead of becoming NaN/null, and fail loud with the log id and type key when a balancesByFinancialType entry is not an object instead of throwing an unhandled TypeError. Narrows the "byte-for-byte identical" docstring claim to its actual scope and documents down()'s ACCESS EXCLUSIVE lock behaviour and the per-statement scope of lock_timeout in the index migration. * fix(log): make balancesByType optional and revert the F11 throw FinancialLogSummary.balancesByType and FinancialLogEntryDto.balancesByType promised plain `number` fields, but a real production row has an entry missing one of the two keys, which the repository already passes through as `undefined` (proven by an existing test). Mark both fields optional and narrow the `as` cast accordingly instead of forcing a promise the code doesn't keep. Also drop the now-unreachable JSON-string branch when parsing balancesByFinancialType: pg-types always returns jsonb columns already parsed (OID 3802), so the string path had zero coverage and no path to it. Most importantly, revert the typeof/null throw added for a prior finding: it was too broad. Before that guard, a primitive balancesByFinancialType entry (number/string/boolean) never threw at all - JS auto-boxes primitives for property access, so e.g. `(1).plusBalanceChf` is simply `undefined`. Only `null` used to throw, and only inside the old mapper's per-row try/catch, silently dropping just that one row. The throw turned three previously-harmless cases into a 500 for the whole request, and widened null's blast radius from "one row" to "everything". Optional chaining covers all of it in one line: primitives resolve to undefined exactly as before, and null now keeps the row (with an empty balancesByType entry) instead of either silently vanishing or failing the whole request. * docs(log): describe what the financial-log projection actually does Two comments claimed more than the code delivers. mapSummaryToEntry still promised a byte-identical response. That stopped being true on purpose: a `balancesByFinancialType` entry holding `null` used to make the previous mapLogToEntry drop the entire log line via its per-row try/catch, whereas the row is now kept with an empty entry. A silent gap in a financial curve is worse than an empty partial entry. The same comment also claimed SQL "fails loud" without qualification, while individual scalar values are in fact tolerated by the jsonb_typeof guards. The balancesByType loop now records why it deliberately has no per-property typeof guard: the case is unproven in production (287,989 entries with both values numeric, one with plusBalanceChf missing, none with a string, boolean or null value), a guard would break response equivalence with the old mapLogToEntry which passed such values through unchanged, and unlike the five scalar fields there is no ::float8 cast here that could abort the query. * fix(log): keep only real numbers in the balancesByType projection The signature promises `plusBalanceChf?: number`, but a balancesByFinancialType entry whose property itself holds a string, boolean or null was passed through unchanged, so the endpoint could return values the DTO does not allow. The previous mapLogToEntry had the same hole; this closes it instead of carrying it forward. Only real numbers are kept now; every other value becomes undefined. On the current production data this changes nothing (287,989 entries with both values numeric, one with plusBalanceChf missing, none holding a string, boolean or null), so it exists to protect the contract for future data. A missing key still yields undefined, and a non-object entry still does not throw. This applies the same hardening the five scalar fields already get via jsonb_typeof in SQL, in TypeScript because balancesByFinancialType is passed through as a raw JSON object. --- ...00000-AddLedgerContentChangeScanIndexes.js | 164 ++++++ .../dashboard-financial.service.spec.ts | 209 +++++++- .../dashboard/dashboard-financial.service.ts | 70 +-- .../dashboard/dto/financial-log.dto.ts | 7 +- .../log/__tests__/log.repository.spec.ts | 469 +++++++++++++++++- .../log/__tests__/log.service.spec.ts | 26 +- .../supporting/log/log.repository.ts | 233 +++++++++ src/subdomains/supporting/log/log.service.ts | 13 +- 8 files changed, 1133 insertions(+), 58 deletions(-) create mode 100644 migration/1785460000000-AddLedgerContentChangeScanIndexes.js diff --git a/migration/1785460000000-AddLedgerContentChangeScanIndexes.js b/migration/1785460000000-AddLedgerContentChangeScanIndexes.js new file mode 100644 index 0000000000..d7c527e70a --- /dev/null +++ b/migration/1785460000000-AddLedgerContentChangeScanIndexes.js @@ -0,0 +1,164 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * Add composite indexes `(updated, id)` on the nine ledger consumer source tables so the + * per-minute content-change-scan (`runContentChangeScan` in + * `src/subdomains/core/accounting/services/consumers/ledger-watermark.helper.ts:210`) stops doing a + * full sequential scan on each of them. + * + * The scan does: + * `WHERE (updated > :scan OR (updated = :scan AND id > :scanId)) ORDER BY updated ASC, id ASC LIMIT 100`. + * None of the nine tables previously had an index on `updated`. + * + * Column order `(updated, id)` is intentional: the query orders by `updated, id`, so an index on + * `updated` alone would still need an explicit Sort step whenever several rows share the same + * `updated` value — same reasoning as AddFinancialLogQueryIndex1785400000000 for `(created, id)`. + * + * Measured evidence: production `EXPLAIN (ANALYZE, BUFFERS)` for `trading_order` (921 MB, the + * largest of the nine) without this index showed a Parallel Seq Scan, Rows Removed by Filter + * 1,806,023 (x3 workers = 5,418,069 rows), Buffers shared hit=26096 read=63188 (~494 MB from disk + * per call), returning only 4 matching rows, Execution Time 151.765 ms. A 45 s delta measurement + * showed 4,750 MB of disk reads and 32.5 million rows read for `trading_order` alone. + * + * 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 nine `CREATE INDEX` statements 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:263-265` (`migrationExecutor.transaction = + * options?.transaction || this.options?.migrationsTransactionMode || "all"` — default `"all"`); + * `src/config/config.ts:265` 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:348` — + * "no migrationsTransactionMode override → default 'all'"); + * `node_modules/typeorm/migration/MigrationExecutor.js:206` 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, the write-blocking window for `trading_order` (the first + * table built) is the sum of all nine index builds, not just its own, and by the time the + * transaction commits all nine tables — including central transaction tables `bank_tx`, + * `buy_crypto`, `crypto_input`, `payout_order` — are simultaneously write-blocked. If further + * migrations are pending at the same time, those run in the same transaction too and extend the + * window further. Splitting this into multiple separate migration files would NOT change this + * (the same transaction still applies across files run in the same batch). `SET LOCAL lock_timeout` caps only how long we WAIT to acquire a lock, not how long we hold it once + * acquired. That timeout is scoped to each individual lock-acquisition attempt — each of the nine + * `CREATE INDEX` statements (and, in `down()`, each of the nine `DROP INDEX` statements) gets its own + * wait budget, not a single global ceiling shared across the whole migration. An earlier statement can + * succeed well inside its 5s budget while a later one still times out and aborts the transaction. + * Production scan+sort for the biggest table's `(updated, id)` was measured at 1159 ms + * — but that number is a `work_mem`-bound External Merge sort spilling ~116 MB to disk, NOT the + * index build itself, which sorts in `maintenance_work_mem` (256 MB in this instance, in RAM) and + * should be faster, plus the time to write ~150 MB of index pages. That points to a low + * single-digit-second range as a realistic expectation for a single index build, but is NOT a + * measured index-build time and must NOT be asserted as a hard upper bound on total build time or + * on the cumulative lock window across all nine tables (none has been measured). 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 nine `DROP INDEX` statements 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 on each table, not writes alone. `down()` runs in + * its own migration transaction (same TypeORM default `migrationsTransactionMode: "all"`), so the same + * cumulative-window reasoning applies: all nine ACCESS EXCLUSIVE locks are held until COMMIT, and the + * first table dropped is blocked — for reads and writes — for the sum of all nine drops. Running + * `migration:revert` against a live table is therefore materially more disruptive than `up()`, not + * merely its mirror image. + * + * Tables and index names: + * trading_order → IDX_47e55a74022f04d725395b9648 + * crypto_input → IDX_37d5dbe4bda6e9e78b0ac08ba1 + * bank_tx → IDX_834c06e67196ac958afc5dccec + * buy_crypto → IDX_398573811cc39fb7ff740459a6 + * exchange_tx → IDX_82c40ae44b9968bf6d2c6acdd0 + * payout_order → IDX_44c2cf65b5554fb61eef1453c5 + * buy_fiat → IDX_934bb0a02ccf36e8ed04bb6bdd + * liquidity_management_order → IDX_6d47b5e8f3e480587a4e3da5a4 + * liquidity_order → IDX_617b110d76b02979c229fbc6be + * + * These are not arbitrary names but the deterministic names TypeORM's DefaultNamingStrategy would + * generate itself, since custom index naming is disallowed by CONTRIBUTING.md. Each name is + * `IDX_` followed by the first 26 hex characters of `sha1( + '_id_updated')` (column names + * `id` and `updated` sorted alphabetically and joined with `_`, per TypeORM's DefaultNamingStrategy). + * + * @class + * @implements {MigrationInterface} + */ +module.exports = class AddLedgerContentChangeScanIndexes1785460000000 { + name = 'AddLedgerContentChangeScanIndexes1785460000000'; + + /** + * @param {QueryRunner} queryRunner + */ + async up(queryRunner) { + // SET LOCAL is scoped to the whole transaction, so set once for all nine CREATE INDEX + // statements below. Bounds WAIT time to acquire the lock, not how long the lock is held. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query( + `CREATE INDEX "IDX_47e55a74022f04d725395b9648" ON "trading_order" ("updated", "id")`, + ); + + await queryRunner.query( + `CREATE INDEX "IDX_37d5dbe4bda6e9e78b0ac08ba1" ON "crypto_input" ("updated", "id")`, + ); + + await queryRunner.query(`CREATE INDEX "IDX_834c06e67196ac958afc5dccec" ON "bank_tx" ("updated", "id")`); + + await queryRunner.query( + `CREATE INDEX "IDX_398573811cc39fb7ff740459a6" ON "buy_crypto" ("updated", "id")`, + ); + + await queryRunner.query( + `CREATE INDEX "IDX_82c40ae44b9968bf6d2c6acdd0" ON "exchange_tx" ("updated", "id")`, + ); + + await queryRunner.query( + `CREATE INDEX "IDX_44c2cf65b5554fb61eef1453c5" ON "payout_order" ("updated", "id")`, + ); + + await queryRunner.query(`CREATE INDEX "IDX_934bb0a02ccf36e8ed04bb6bdd" ON "buy_fiat" ("updated", "id")`); + + await queryRunner.query( + `CREATE INDEX "IDX_6d47b5e8f3e480587a4e3da5a4" ON "liquidity_management_order" ("updated", "id")`, + ); + + await queryRunner.query( + `CREATE INDEX "IDX_617b110d76b02979c229fbc6be" ON "liquidity_order" ("updated", "id")`, + ); + } + + /** + * @param {QueryRunner} queryRunner + */ + async down(queryRunner) { + // SET LOCAL is scoped to the whole transaction, so set once for all nine DROP INDEX + // statements below. Bounds WAIT time to acquire the lock, not how long the lock is held. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query(`DROP INDEX "public"."IDX_617b110d76b02979c229fbc6be"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_6d47b5e8f3e480587a4e3da5a4"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_934bb0a02ccf36e8ed04bb6bdd"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_44c2cf65b5554fb61eef1453c5"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_82c40ae44b9968bf6d2c6acdd0"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_398573811cc39fb7ff740459a6"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_834c06e67196ac958afc5dccec"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_37d5dbe4bda6e9e78b0ac08ba1"`); + + await queryRunner.query(`DROP INDEX "public"."IDX_47e55a74022f04d725395b9648"`); + } +}; 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 b54ea2fe7e..b9f983e1cd 100644 --- a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts +++ b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts @@ -3,18 +3,24 @@ import { Test, TestingModule } from '@nestjs/testing'; import { AssetService } from 'src/shared/models/asset/asset.service'; import { RefRewardService } from 'src/subdomains/core/referral/reward/services/ref-reward.service'; import { Log } from '../../log/log.entity'; +import { FinancialLogSummary } from '../../log/log.repository'; import { LogService } from '../../log/log.service'; import { DashboardFinancialService } from '../dashboard-financial.service'; describe('DashboardFinancialService', () => { let service: DashboardFinancialService; + let logService: LogService; + let assetService: AssetService; beforeEach(async () => { + logService = createMock(); + assetService = createMock(); + const module: TestingModule = await Test.createTestingModule({ providers: [ DashboardFinancialService, - { provide: LogService, useValue: createMock() }, - { provide: AssetService, useValue: createMock() }, + { provide: LogService, useValue: logService }, + { provide: AssetService, useValue: assetService }, { provide: RefRewardService, useValue: createMock() }, ], }).compile(); @@ -53,24 +59,205 @@ describe('DashboardFinancialService', () => { expect(entry.minus.binance).toEqual({ total: 7, withdraw: 1, trading: 6 }); }); - describe('mapLogToEntry (fxPnlChf exposure)', () => { - const logWith = (balancesTotal: object): Log => - ({ created: new Date('2026-07-14T00:00:00Z'), message: JSON.stringify({ balancesTotal }) }) as Log; + describe('mapSummaryToEntry (fxPnlChf exposure)', () => { + const summaryWith = (overrides: Partial): FinancialLogSummary => ({ + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByType: {}, + ...overrides, + }); it('exposes the fxPnlChf written into the log entry, preserving a negative value', () => { - const entry = service['mapLogToEntry']( - logWith({ totalBalanceChf: 100, plusBalanceChf: 100, minusBalanceChf: 0, fxPnlChf: -3245 }), + const entry = service['mapSummaryToEntry']( + summaryWith({ totalBalanceChf: 100, plusBalanceChf: 100, minusBalanceChf: 0, fxPnlChf: -3245 }), ); - expect(entry?.fxPnlChf).toBe(-3245); + expect(entry.fxPnlChf).toBe(-3245); }); it('defaults historical entries logged before fxPnlChf existed to 0', () => { - const entry = service['mapLogToEntry']( - logWith({ totalBalanceChf: 100, plusBalanceChf: 100, minusBalanceChf: 0 }), + const entry = service['mapSummaryToEntry']( + summaryWith({ totalBalanceChf: 100, plusBalanceChf: 100, minusBalanceChf: 0, fxPnlChf: null }), + ); + + expect(entry.fxPnlChf).toBe(0); + }); + + it('defaults null totalBalanceChf/plusBalanceChf/minusBalanceChf (as the repository now returns for missing/null source data, F13) to 0 in the response — same as the old mapLogToEntry ?? 0 defaults', () => { + const entry = service['mapSummaryToEntry']( + summaryWith({ totalBalanceChf: null, plusBalanceChf: null, minusBalanceChf: null, fxPnlChf: null }), ); - expect(entry?.fxPnlChf).toBe(0); + expect(entry.totalBalanceChf).toBe(0); + expect(entry.plusBalanceChf).toBe(0); + expect(entry.minusBalanceChf).toBe(0); + expect(entry.fxPnlChf).toBe(0); + }); + + it('produces the same FinancialLogEntryDto the old mapLogToEntry would have for equivalent data', () => { + // Underlying FinanceLog.message JSON that the old mapper would have parsed: + // { + // balancesTotal: { totalBalanceChf: 1000, plusBalanceChf: 1500, minusBalanceChf: 500, fxPnlChf: -12.5 }, + // balancesByFinancialType: { + // Crypto: { plusBalance: 1, plusBalanceChf: 800, minusBalance: 0, minusBalanceChf: 200 }, + // Fiat: { plusBalance: 1, plusBalanceChf: 700, minusBalance: 0, minusBalanceChf: 300 }, + // }, + // assets: { "7": { priceChf: 65000.25 } }, + // } + // Old mapLogToEntry(log, 7) expected output (reconstructed byte-for-byte from that path): + const expectedFromOldMapper = { + timestamp: new Date('2026-07-14T12:00:00Z'), + totalBalanceChf: 1000, + plusBalanceChf: 1500, + minusBalanceChf: 500, + fxPnlChf: -12.5, + btcPriceChf: 65000.25, + balancesByType: { + Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 }, + Fiat: { plusBalanceChf: 700, minusBalanceChf: 300 }, + }, + }; + + const summary: FinancialLogSummary = { + created: expectedFromOldMapper.timestamp, + id: 42, + totalBalanceChf: 1000, + plusBalanceChf: 1500, + minusBalanceChf: 500, + fxPnlChf: -12.5, + btcPriceChf: 65000.25, + balancesByType: { + Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 }, + Fiat: { plusBalanceChf: 700, minusBalanceChf: 300 }, + }, + }; + + expect(service['mapSummaryToEntry'](summary)).toEqual(expectedFromOldMapper); + }); + + it('produces the same FinancialLogEntryDto the old mapLogToEntry/extractBtcPrice pathway would have when btcAssetId is undefined (no BTC asset resolved)', () => { + // Underlying FinanceLog.message JSON (assets present, but no BTC asset id was resolved this call): + // { + // balancesTotal: { totalBalanceChf: 1000, plusBalanceChf: 1500, minusBalanceChf: 500, fxPnlChf: -12.5 }, + // balancesByFinancialType: { Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 } }, + // assets: { "7": { priceChf: 65000.25 } }, + // } + // Old extractBtcPrice(financeLog, undefined): `!btcAssetId` is true for undefined => returns 0, + // regardless of `assets` content. Old mapLogToEntry expected output: + const expectedFromOldMapper = { + timestamp: new Date('2026-07-14T12:00:00Z'), + totalBalanceChf: 1000, + plusBalanceChf: 1500, + minusBalanceChf: 500, + fxPnlChf: -12.5, + btcPriceChf: 0, + balancesByType: { Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 } }, + }; + + // New path: getFinancialLogSummaries projects a SQL literal 0 when btcAssetId is undefined (no + // assets path parameter bound), so the summary already carries btcPriceChf: 0. + const summary: FinancialLogSummary = { + created: expectedFromOldMapper.timestamp, + id: 42, + totalBalanceChf: 1000, + plusBalanceChf: 1500, + minusBalanceChf: 500, + fxPnlChf: -12.5, + btcPriceChf: 0, + balancesByType: { Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 } }, + }; + + expect(service['mapSummaryToEntry'](summary)).toEqual(expectedFromOldMapper); + }); + + it('produces the same FinancialLogEntryDto the old mapLogToEntry/extractBtcPrice pathway would have when btcAssetId is 0 (falsy, same as undefined)', () => { + // Same underlying FinanceLog.message JSON as above, but btcAssetId = 0 this time. + // Old extractBtcPrice(financeLog, 0): `!btcAssetId` is true for 0 (falsy) => returns 0, exactly + // like the undefined case above — this is the behaviour F1 restores (id=0 is unreachable in this + // database today, but the falsy check keeps the two cases byte-identical, as before the projection + // was moved into SQL). + const expectedFromOldMapper = { + timestamp: new Date('2026-07-14T12:00:00Z'), + totalBalanceChf: 1000, + plusBalanceChf: 1500, + minusBalanceChf: 500, + fxPnlChf: -12.5, + btcPriceChf: 0, + balancesByType: { Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 } }, + }; + + // New path: getFinancialLogSummaries(0, ...) also takes the SQL-literal-0 branch (F1 falsy check), + // so the summary carries btcPriceChf: 0 here too — identical to the btcAssetId=undefined case. + const summary: FinancialLogSummary = { + created: expectedFromOldMapper.timestamp, + id: 43, + totalBalanceChf: 1000, + plusBalanceChf: 1500, + minusBalanceChf: 500, + fxPnlChf: -12.5, + btcPriceChf: 0, + balancesByType: { Crypto: { plusBalanceChf: 800, minusBalanceChf: 200 } }, + }; + + expect(service['mapSummaryToEntry'](summary)).toEqual(expectedFromOldMapper); + }); + }); + + describe('getFinancialLog', () => { + it('resolves getBtcCoin before getFinancialLogSummaries (ordering required for SQL btcAssetId)', 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, + balancesByType: { Crypto: { plusBalanceChf: 120, minusBalanceChf: 20 } }, + }, + ]; + + const getBtcCoinSpy = 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); + + expect(getBtcCoinSpy).toHaveBeenCalled(); + expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true); + // Ordering matters: btcAssetId is a SQL projection parameter, so getBtcCoin must finish first. + expect(getBtcCoinSpy.mock.invocationCallOrder[0]).toBeLessThan(getSummariesSpy.mock.invocationCallOrder[0]); + + expect(result.entries).toEqual([ + { + timestamp: summaries[0].created, + totalBalanceChf: 100, + plusBalanceChf: 120, + minusBalanceChf: 20, + fxPnlChf: 1.5, + btcPriceChf: 64000, + balancesByType: { Crypto: { plusBalanceChf: 120, minusBalanceChf: 20 } }, + }, + ]); + // Mutation guard: projected plus/minus and btc price must survive end-to-end. + expect(result.entries[0].plusBalanceChf).not.toBe(result.entries[0].minusBalanceChf); + expect(result.entries[0].btcPriceChf).toBe(64000); + }); + + it('passes undefined btcAssetId when getBtcCoin returns no asset', async () => { + jest.spyOn(assetService, 'getBtcCoin').mockResolvedValue(undefined as never); + const getSummariesSpy = jest.spyOn(logService, 'getFinancialLogSummaries').mockResolvedValue([]); + + await service.getFinancialLog(); + + expect(getSummariesSpy).toHaveBeenCalledWith(undefined, undefined, undefined); }); }); }); diff --git a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts index b3c3e4b8bf..42fa6b397d 100644 --- a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts +++ b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts @@ -2,6 +2,7 @@ import { Injectable } from '@nestjs/common'; import { AssetService } from 'src/shared/models/asset/asset.service'; import { RefRewardService } from '../../core/referral/reward/services/ref-reward.service'; import { Log } from '../log/log.entity'; +import { FinancialLogSummary } from '../log/log.repository'; import { LogService } from '../log/log.service'; import { FinanceLog } from '../log/dto/log.dto'; import { @@ -23,16 +24,13 @@ export class DashboardFinancialService { ) {} async getFinancialLog(from?: Date, dailySample?: boolean): Promise { - const [logs, btcAsset] = await Promise.all([ - this.logService.getFinancialLogs(from, dailySample), - this.assetService.getBtcCoin(), - ]); - - const btcAssetId = btcAsset?.id; - const entries = logs - .map((log) => this.mapLogToEntry(log, btcAssetId)) - .filter((e): e is FinancialLogEntryDto => e != null); + // 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 entries = summaries.map((summary) => this.mapSummaryToEntry(summary)); return { entries }; } @@ -234,39 +232,25 @@ export class DashboardFinancialService { return { timestamp: latest.created, byType, byBlockchain }; } - private mapLogToEntry(log: Log, btcAssetId?: number): FinancialLogEntryDto | undefined { - try { - const financeLog: FinanceLog = JSON.parse(log.message); - - const btcPriceChf = this.extractBtcPrice(financeLog, btcAssetId); - - const balancesByType: Record = {}; - if (financeLog.balancesByFinancialType) { - for (const [type, data] of Object.entries(financeLog.balancesByFinancialType)) { - balancesByType[type] = { - plusBalanceChf: data.plusBalanceChf, - minusBalanceChf: data.minusBalanceChf, - }; - } - } - - return { - timestamp: log.created, - totalBalanceChf: financeLog.balancesTotal?.totalBalanceChf ?? 0, - plusBalanceChf: financeLog.balancesTotal?.plusBalanceChf ?? 0, - minusBalanceChf: financeLog.balancesTotal?.minusBalanceChf ?? 0, - fxPnlChf: financeLog.balancesTotal?.fxPnlChf ?? 0, - btcPriceChf, - balancesByType, - }; - } catch { - return undefined; - } - } - - private extractBtcPrice(financeLog: FinanceLog, btcAssetId?: number): number { - if (!financeLog.assets || !btcAssetId) return 0; - - return financeLog.assets[btcAssetId]?.priceChf ?? 0; + // Pure mapping over the SQL projection: for well-formed data, the response matches the previous + // mapLogToEntry path exactly, including the `?? 0` field defaults. One case is intentionally + // different: if a `balancesByFinancialType` entry has the value `null` (e.g. `{"Crypto": null}`), + // the row is now kept (with an empty balancesByType entry) instead of the previous mapLogToEntry's + // per-row try/catch silently dropping the whole log line — a silent gap in a financial curve is + // worse than an empty partial entry, so this was changed on purpose. A malformed `message` document + // still fails loud: the `message::jsonb` cast in SQL throws for that. Individual scalar field + // values, though, are tolerated via `jsonb_typeof` guards (nulled rather than thrown), and + // non-numeric values inside `balancesByFinancialType` are passed through unchanged + // (see log.repository.ts). + private mapSummaryToEntry(summary: FinancialLogSummary): FinancialLogEntryDto { + return { + timestamp: summary.created, + totalBalanceChf: summary.totalBalanceChf ?? 0, + plusBalanceChf: summary.plusBalanceChf ?? 0, + minusBalanceChf: summary.minusBalanceChf ?? 0, + fxPnlChf: summary.fxPnlChf ?? 0, + btcPriceChf: summary.btcPriceChf, + 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 e344a9e69b..b53f4fb8c3 100644 --- a/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts +++ b/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts @@ -7,7 +7,12 @@ export class FinancialLogEntryDto { // logged before this field existed (see BalancesTotal.fxPnlChf). fxPnlChf: number; btcPriceChf: number; - balancesByType: Record; + /** + * 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. + */ + 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 6da4f4bbe2..98ba6df5d4 100644 --- a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts @@ -1,5 +1,5 @@ import { EntityManager, UpdateResult } from 'typeorm'; -import { FINANCIAL_LOG_VALIDITY_AUDIT_SUBSYSTEM } from '../log.entity'; +import { FINANCIAL_DATA_LOG_SUBSYSTEM, FINANCIAL_LOG_VALIDITY_AUDIT_SUBSYSTEM, LogSeverity } from '../log.entity'; import { LogRepository } from '../log.repository'; type UpdateQueryBuilderStub = { @@ -187,4 +187,471 @@ describe('LogRepository', () => { expect(stub.getExists).toHaveBeenCalled(); }); }); + + describe('getFinancialLogSummaries', () => { + it('maps the SQL projection into FinancialLogSummary (numbers, balancesByType, btc price)', 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: '42', + totalBalanceChf: '100.5', + plusBalanceChf: '200', + minusBalanceChf: '99.5', + fxPnlChf: '-3245', + btcPriceChf: '65000.25', + balancesByFinancialType: { + // Real jsonb-embedded numbers already arrive as JS numbers via the pg driver — no Number() + // coercion happens in this loop anymore (F10), so the mock must reflect that shape. + Crypto: { plusBalanceChf: 10, minusBalanceChf: 5, plusBalance: 1, minusBalance: 1 }, + Fiat: { plusBalanceChf: 90, minusBalanceChf: 40 }, + }, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(7); + + expect(rows).toEqual([ + { + created, + id: 42, + totalBalanceChf: 100.5, + plusBalanceChf: 200, + minusBalanceChf: 99.5, + fxPnlChf: -3245, + btcPriceChf: 65000.25, + balancesByType: { + Crypto: { plusBalanceChf: 10, minusBalanceChf: 5 }, + Fiat: { plusBalanceChf: 90, minusBalanceChf: 40 }, + }, + }, + ]); + // Projection must use distinct plus/minus columns (mutation: swap would fail this assertion). + expect(rows[0].plusBalanceChf).not.toBe(rows[0].minusBalanceChf); + expect(rows[0].balancesByType.Crypto.plusBalanceChf).toBe(10); + expect(rows[0].balancesByType.Crypto.minusBalanceChf).toBe(5); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + // Exact path→alias bindings: swapping plus/minus (or hard-coding btc) in the projection must fail. + // jsonb_typeof guards (F2/F3) wrap each cast; the path→alias pairing is still asserted. + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'totalBalanceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'totalBalanceChf')::float8`); + expect(sql).toContain(`END AS "totalBalanceChf"`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'plusBalanceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'plusBalanceChf')::float8`); + expect(sql).toContain(`END AS "plusBalanceChf"`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'minusBalanceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'minusBalanceChf')::float8`); + expect(sql).toContain(`END AS "minusBalanceChf"`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'assets' -> $5::text -> 'priceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'assets' -> $5::text ->> 'priceChf')::float8`); + expect(sql).toContain(`AS "btcPriceChf"`); + expect(sql).toContain(`message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType"`); + expect(sql).not.toContain("-> 'tradings'"); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true, '7']); + }); + + it('projects btcPriceChf as SQL literal 0 when btcAssetId is undefined (no assets path param)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 1, + plusBalanceChf: 1, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: null, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(undefined); + + expect(rows[0].btcPriceChf).toBe(0); + expect(rows[0].fxPnlChf).toBeNull(); + expect(rows[0].balancesByType).toEqual({}); + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('0::float8'); + expect(sql).not.toContain("-> 'assets'"); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true]); + }); + + it('defaults absent btc priceChf from SQL null to 0 in the mapping layer', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 1, + plusBalanceChf: 1, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: null, + balancesByFinancialType: null, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(99); + expect(rows[0].btcPriceChf).toBe(0); + }); + + it('uses the dailySample MAX(id)-per-day subquery when dailySample is true', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(undefined, undefined, true); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('MAX(id)'); + expect(sql).toContain('CAST(created AS DATE)'); + expect(sql).toContain('id IN ('); + }); + + it('fails loud when the keyset cursor id no longer exists (no silent empty main query)', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([]); + const stub = financialLogQueryBuilderStub(false); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(stub as never); + + await expect( + repo.getFinancialLogSummaries(undefined, undefined, false, undefined, undefined, 999), + ).rejects.toThrow('Financial log cursor row 999 no longer exists'); + + expect(stub.getExists).toHaveBeenCalled(); + }); + + it('returns empty when the cursor still exists (legitimate end-of-data)', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([]); + const stub = financialLogQueryBuilderStub(true); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(stub as never); + + await expect( + repo.getFinancialLogSummaries(undefined, undefined, false, undefined, undefined, 999), + ).resolves.toEqual([]); + + expect(stub.getExists).toHaveBeenCalled(); + }); + + it('skips the existence check when after is unset', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([]); + const stub = financialLogQueryBuilderStub(false); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(stub as never); + + await expect(repo.getFinancialLogSummaries()).resolves.toEqual([]); + + expect(stub.getExists).not.toHaveBeenCalled(); + }); + + it('binds from/to/limit/after as incremental parameters after the fixed filters', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + const stub = financialLogQueryBuilderStub(true); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(stub as never); + const from = new Date('2026-01-01T00:00:00Z'); + const to = new Date('2026-02-01T00:00:00Z'); + + await repo.getFinancialLogSummaries(3, from, false, to, 50, 10); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('created >='); + expect(sql).toContain('created <='); + expect(sql).toContain('(created, id) >'); + expect(sql).toContain('LIMIT'); + expect(params).toEqual([ + 'LogService', + FINANCIAL_DATA_LOG_SUBSYSTEM, + LogSeverity.INFO, + true, + '3', + from, + to, + 10, + 10, + 50, + ]); + }); + + it('orders by created ASC, id ASC (never DESC) so the chart stays chronological and keyset pagination advances forward', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('ORDER BY created ASC, id ASC'); + }); + + describe('parameter position ($N placeholders match their array index)', () => { + const from = new Date('2026-01-01T00:00:00Z'); + const to = new Date('2026-02-01T00:00:00Z'); + + it('only from, btcAssetId set', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7, from); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('$5::text'); + expect(sql).toContain('created >= $6'); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true, '7', from]); + }); + + it('from + to, btcAssetId set', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7, from, false, to); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('created >= $6'); + expect(sql).toContain('created <= $7'); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true, '7', from, to]); + }); + + it('from + limit, btcAssetId set, dailySample true', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7, from, true, undefined, 50); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('created >= $6'); + expect(sql).toContain('LIMIT $7'); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true, '7', from, 50]); + }); + + it('from + after + limit, btcAssetId set', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(financialLogQueryBuilderStub(true) as never); + + await repo.getFinancialLogSummaries(7, from, false, undefined, 50, 10); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('created >= $6'); + expect(sql).toContain('(created, id) > ((SELECT c.created FROM log c WHERE c.id = $7), $8)'); + expect(sql).toContain('LIMIT $9'); + expect(params).toEqual([ + 'LogService', + FINANCIAL_DATA_LOG_SUBSYSTEM, + LogSeverity.INFO, + true, + '7', + from, + 10, + 10, + 50, + ]); + }); + + it('none of from/to/after/limit, btcAssetId set', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('$5::text'); + expect(sql).not.toContain('created >='); + expect(sql).not.toContain('created <='); + expect(sql).not.toContain('(created, id) >'); + expect(sql).not.toContain('LIMIT'); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true, '7']); + }); + + it('btcAssetId undefined, from + after', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(financialLogQueryBuilderStub(true) as never); + + await repo.getFinancialLogSummaries(undefined, from, false, undefined, undefined, 10); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('0::float8'); + expect(sql).toContain('created >= $5'); + expect(sql).toContain('(created, id) > ((SELECT c.created FROM log c WHERE c.id = $6), $7)'); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true, from, 10, 10]); + }); + }); + + it('guards all five projected number fields with jsonb_typeof so one bad value nulls only that field (F2/F3)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'totalBalanceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'totalBalanceChf')::float8`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'plusBalanceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'plusBalanceChf')::float8`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'minusBalanceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'minusBalanceChf')::float8`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'fxPnlChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'balancesTotal' ->> 'fxPnlChf')::float8`); + expect(sql).toContain(`WHEN jsonb_typeof(message::jsonb -> 'assets' -> $5::text -> 'priceChf') = 'number'`); + expect(sql).toContain(`THEN (message::jsonb -> 'assets' -> $5::text ->> 'priceChf')::float8`); + }); + + it('projects btcPriceChf as SQL literal 0 when btcAssetId is 0 — same falsy branch as undefined (F1)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 1, + plusBalanceChf: 1, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: null, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(0); + + expect(rows[0].btcPriceChf).toBe(0); + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('0::float8'); + expect(sql).not.toContain(`-> 'assets'`); + expect(params).toEqual(['LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true]); + }); + + it('maps a raw row where balancesTotal is entirely missing (all guards fire) to null/null/null (not 0) and keeps fxPnlChf null — the ?? 0 default happens only in mapSummaryToEntry, never in the repository (F13)', async () => { + const repo = new LogRepository({} as EntityManager); + const created = new Date('2026-07-14T00:00:00Z'); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created, + id: 1, + totalBalanceChf: null, + plusBalanceChf: null, + minusBalanceChf: null, + fxPnlChf: null, + btcPriceChf: '65000.25', + balancesByFinancialType: null, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(7); + + expect(rows[0].totalBalanceChf).toBeNull(); + expect(rows[0].plusBalanceChf).toBeNull(); + expect(rows[0].minusBalanceChf).toBeNull(); + expect(rows[0].fxPnlChf).toBeNull(); + expect(rows[0].btcPriceChf).toBe(65000.25); + }); + + it('maps a raw row with JSON null for only totalBalanceChf/plusBalanceChf (minusBalanceChf/fxPnlChf unaffected) to null/null, not 0 (F13)', async () => { + const repo = new LogRepository({} as EntityManager); + const created = new Date('2026-07-14T00:00:00Z'); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created, + id: 2, + totalBalanceChf: null, + plusBalanceChf: null, + minusBalanceChf: '500', + fxPnlChf: '-12.5', + btcPriceChf: '65000.25', + balancesByFinancialType: null, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(7); + + expect(rows[0].totalBalanceChf).toBeNull(); + expect(rows[0].plusBalanceChf).toBeNull(); + expect(rows[0].minusBalanceChf).toBe(500); + expect(rows[0].fxPnlChf).toBe(-12.5); + expect(rows[0].btcPriceChf).toBe(65000.25); + }); + + it('passes plusBalanceChf through unconverted when the key is missing from one balancesByFinancialType entry, instead of Number(undefined) => NaN => null (F10, matches a real production row)', async () => { + const repo = new LogRepository({} as EntityManager); + const created = new Date('2026-07-14T00:00:00Z'); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created, + id: 99, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: { + Crypto: { minusBalanceChf: 5 }, // plusBalanceChf key missing, as observed in production + }, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(); + + expect(rows[0].balancesByType.Crypto.plusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Crypto.minusBalanceChf).toBe(5); + // The old mapLogToEntry omitted the key entirely for a missing value; JSON serialisation drops an + // undefined-valued key the same way — never NaN, never null. + expect(JSON.parse(JSON.stringify(rows[0].balancesByType.Crypto))).toEqual({ minusBalanceChf: 5 }); + }); + + it('keeps the row and yields undefined fields (not 0, not null, no error) when a balancesByFinancialType entry is null, a number, or a string (F16 reverts the F11 throw)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 77, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: { Crypto: null, Fiat: 1, Other: 'bad' }, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(); + + expect(querySpy).toHaveBeenCalledTimes(1); + expect(rows).toHaveLength(1); + expect(rows[0].balancesByType.Crypto.plusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Crypto.minusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Fiat.plusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Fiat.minusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Other.plusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Other.minusBalanceChf).toBeUndefined(); + // Same JSON-serialisation check as the F10 test: undefined values are omitted, never null/0. + expect(JSON.parse(JSON.stringify(rows[0].balancesByType))).toEqual({ Crypto: {}, Fiat: {}, Other: {} }); + }); + + it('yields undefined for both fields (no error, row kept) when a balancesByFinancialType entry is an object but its properties have the wrong type (string/boolean)', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 88, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: { + Crypto: { plusBalanceChf: 'bad', minusBalanceChf: true }, + }, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(); + + expect(rows).toHaveLength(1); + expect(rows[0].balancesByType.Crypto.plusBalanceChf).toBeUndefined(); + expect(rows[0].balancesByType.Crypto.minusBalanceChf).toBeUndefined(); + // 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({}); + }); + }); }); diff --git a/src/subdomains/supporting/log/__tests__/log.service.spec.ts b/src/subdomains/supporting/log/__tests__/log.service.spec.ts index 748cbd5487..8c06347f83 100644 --- a/src/subdomains/supporting/log/__tests__/log.service.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.service.spec.ts @@ -12,7 +12,7 @@ import { LogSeverity, MAX_VALIDITY_SWEEP_ROWS, } from '../log.entity'; -import { LogRepository } from '../log.repository'; +import { FinancialLogSummary, LogRepository } from '../log.repository'; import { LogService } from '../log.service'; jest.mock('../log.repository'); @@ -270,6 +270,30 @@ describe('LogService', () => { }); }); + describe('getFinancialLogSummaries', () => { + it('delegates to the repository with the same arguments and returns FinancialLogSummary-shaped rows', async () => { + const from = new Date('2026-07-01T00:00:00Z'); + const to = new Date('2026-07-15T00:00:00Z'); + const summaries: FinancialLogSummary[] = [ + { + created: from, + id: 11, + totalBalanceChf: 100, + plusBalanceChf: 150, + minusBalanceChf: 50, + fxPnlChf: -12, + btcPriceChf: 65000, + balancesByType: { Crypto: { plusBalanceChf: 150, minusBalanceChf: 50 } }, + }, + ]; + const spy = jest.spyOn(logRepo, 'getFinancialLogSummaries').mockResolvedValue(summaries); + + await expect(service.getFinancialLogSummaries(7, from, true, to, 25, 10)).resolves.toEqual(summaries); + + expect(spy).toHaveBeenCalledWith(7, from, true, to, 25, 10); + }); + }); + describe('create', () => { it('should reject fabricated financial log validity audit records', async () => { const saveSpy = jest.spyOn(logRepo, 'save'); diff --git a/src/subdomains/supporting/log/log.repository.ts b/src/subdomains/supporting/log/log.repository.ts index 6be2809ef7..6da5244332 100644 --- a/src/subdomains/supporting/log/log.repository.ts +++ b/src/subdomains/supporting/log/log.repository.ts @@ -40,6 +40,41 @@ export interface FinancialLogAssetPrice { logId: number; } +/** + * Dashboard financial-log chart fields projected from a FinancialDataLog snapshot (no full message JSON). + * Contains exactly what mapSummaryToEntry needs so it never touches log.message. + */ +export interface FinancialLogSummary { + created: Date; + id: number; + /** + * null when absent/non-numeric in the source JSON (missing key, JSON null, or a wrong JSON type) — + * mapSummaryToEntry keeps its existing `?? 0` default at the call site; this method must NOT default + * it itself. + */ + totalBalanceChf: number | null; + /** Same null semantics as totalBalanceChf. */ + plusBalanceChf: number | null; + /** Same null semantics as totalBalanceChf. */ + minusBalanceChf: number | null; + /** + * null when absent in the source JSON (first entry has no previous snapshot to diff against, see + * BalancesTotal.fxPnlChf) — mapSummaryToEntry keeps its existing `?? 0` default at the call site; + * this method must NOT default it itself. + */ + fxPnlChf: number | null; + /** + * 0 when btcAssetId is falsy (undefined or 0) or the asset key/price is unusable — computed in SQL + * only when btcAssetId is truthy. + */ + btcPriceChf: number; + /** + * plusBalanceChf/minusBalanceChf can be `undefined` per type: a real production row had a + * `balancesByFinancialType` entry missing one of the two keys (see getFinancialLogSummaries below). + */ + balancesByType: Record; +} + @Injectable() export class LogRepository extends BaseRepository { constructor(manager: EntityManager) { @@ -324,6 +359,204 @@ ORDER BY l.created ASC, l.id ASC`; return rows; } + /** + * SQL-side projection of the small FinancialDataLog sub-trees needed by the dashboard financial log chart + * (balancesTotal scalars, optional BTC priceChf, balancesByFinancialType). Callers avoid shipping/parsing + * the full ~42 KB `message` JSON per row — `assets` and `tradings` are never selected/transferred. + * + * balancesByFinancialType is selected as a single jsonb sub-object column (not LATERAL-expanded): it is much + * smaller than the parent message and excludes assets/tradings; reduced to plus/minus CHF per type in JS. + * + * Malformed `message` JSON fails loud: `message::jsonb` aborts the whole query — same fail-loud choice as + * getFinancialLogAssetPrices (volume-tested against all 31,925 matching rows in production, zero invalid + * JSON found; re-stated here, not re-verified). + * + * Optional `after` keyset cursor and `dailySample` match getFinancialLogs semantics (see that method). + * + * All five number fields below (totalBalanceChf, plusBalanceChf, minusBalanceChf, btcPriceChf and + * fxPnlChf) are guarded with `jsonb_typeof(...) = 'number'` — same pattern as the priceChf guard in + * getFinancialLogAssetPrices above. A non-numeric value (missing key, JSON null, or a wrong JSON type) + * nulls only that one field instead of aborting the whole query via a failing ::float8 cast; the outer + * message::jsonb cast itself stays fail-loud. SQL NULL is kept as `null` in the mapping below for + * totalBalanceChf/plusBalanceChf/minusBalanceChf/fxPnlChf (btcPriceChf is the only one of the five + * defaulted to 0 here, matching extractBtcPrice's `?.priceChf ?? 0`); mapSummaryToEntry's existing + * `?? 0` default at the call site turns a null field into 0 in the response for total/plus/minus/fxPnl, + * matching the old mapLogToEntry `?? 0` defaults for the same underlying data. That equality holds for + * a missing key or a JSON null value in the source — it does NOT hold for a value that is present but + * wrongly typed (e.g. the JSON string "100" in a number field): the old path passed such a string + * through unchanged, while the jsonb_typeof guard here turns it into SQL NULL and therefore 0 + * downstream. No such value exists in production today (verified: 31,952 of 31,956 balancesTotal rows + * have plusBalanceChf/minusBalanceChf as JSON 'number', the remaining 4 rows have a missing key or JSON + * null — no string observed); the scope limit above is deliberate, not a bug. + */ + async getFinancialLogSummaries( + btcAssetId?: number, + from?: Date, + dailySample?: boolean, + to?: Date, + limit?: number, + after?: number, // id of the last row of the previous page; NEVER a Date/created value + ): Promise { + const params: unknown[] = []; + let i = 1; + + // Fixed filter params shared by the main WHERE and (when dailySample) the MAX(id) subquery. + const systemParam = `$${i++}`; + const subsystemParam = `$${i++}`; + const severityParam = `$${i++}`; + const validParam = `$${i++}`; + params.push('LogService', FINANCIAL_DATA_LOG_SUBSYSTEM, LogSeverity.INFO, true); + + // BTC price: only bind a parameter when btcAssetId is truthy — mirrors the old extractBtcPrice's + // `!btcAssetId` falsy check (0/undefined/null all take the "no BTC asset" path), not merely + // `!== undefined`. btcAssetId=0 is unreachable in this database (asset ids start at 1), so the + // difference is not observable today, but the falsy check preserves byte-identical behaviour with + // extractBtcPrice for any future btcAssetId=0 — do not "fix" this back to `!== undefined`. + let btcPriceSelect: string; + if (btcAssetId) { + const assetPath = `message::jsonb -> 'assets' -> $${i}::text`; + // jsonb_typeof guard (same pattern as the balancesTotal fields below and getFinancialLogAssetPrices' + // priceChf guard above): a missing asset entry or a non-numeric priceChf value nulls only this + // field instead of aborting the whole query via a failing ::float8 cast. Number(null) => 0 in the + // mapping below, matching extractBtcPrice's `?.priceChf ?? 0` and its 0-return paths. + btcPriceSelect = `CASE WHEN jsonb_typeof(${assetPath} -> 'priceChf') = 'number' THEN (${assetPath} ->> 'priceChf')::float8 ELSE NULL END`; + params.push(String(btcAssetId)); + i++; + } else { + btcPriceSelect = '0::float8'; + } + + const conditions: string[] = []; + if (dailySample) { + // Same daily-sample shape as getFinancialLogs: restrict to MAX(id) per calendar day among valid INFO + // FinancialDataLog rows, then apply from/to/after/limit on the outer filtered set. + conditions.push( + `id IN (SELECT MAX(id) FROM log WHERE system = ${systemParam} AND subsystem = ${subsystemParam} AND severity = ${severityParam} AND valid = ${validParam} GROUP BY CAST(created AS DATE))`, + ); + } else { + conditions.push( + `system = ${systemParam}`, + `subsystem = ${subsystemParam}`, + `severity = ${severityParam}`, + `valid = ${validParam}`, + ); + } + + if (from) { + conditions.push(`created >= $${i++}`); + params.push(from); + } + if (to) { + conditions.push(`created <= $${i++}`); + params.push(to); + } + if (after != null) { + // Same row-value keyset as getFinancialLogs: created resolved in-DB at full precision. + conditions.push(`(created, id) > ((SELECT c.created FROM log c WHERE c.id = $${i}), $${i + 1})`); + params.push(after, after); + i += 2; + } + + let limitClause = ''; + if (limit != null) { + limitClause = `LIMIT $${i++}`; + params.push(limit); + } + + const sql = ` +SELECT 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 + WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'plusBalanceChf') = 'number' + THEN (message::jsonb -> 'balancesTotal' ->> 'plusBalanceChf')::float8 + ELSE NULL + END AS "plusBalanceChf", + CASE + WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'minusBalanceChf') = 'number' + THEN (message::jsonb -> 'balancesTotal' ->> 'minusBalanceChf')::float8 + ELSE NULL + 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" +FROM log +WHERE ${conditions.join(' AND ')} +ORDER BY created ASC, id ASC +${limitClause}`; + + const raw = (await this.query(sql, params)) as { + created: Date | string; + id: number | string; + totalBalanceChf: number | string | null; + plusBalanceChf: number | string | null; + minusBalanceChf: number | string | null; + fxPnlChf: number | string | null; + btcPriceChf: number | string | null; + balancesByFinancialType: unknown; + }[]; + + const rows: FinancialLogSummary[] = raw.map((r) => { + // pg may return numeric columns as strings; coerce with Number(...) like getFinancialLogAssetPrices. + // totalBalanceChf/plusBalanceChf/minusBalanceChf/fxPnlChf all stay null when the SQL projection + // above nulled them (do NOT default any of them to 0 here — that belongs to mapSummaryToEntry's + // `?? 0` at the call site). + // 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 } + >; + 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. + // 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. + const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); + balancesByType[type] = { + plusBalanceChf: asNumber(data?.plusBalanceChf), + minusBalanceChf: asNumber(data?.minusBalanceChf), + }; + } + } + + return { + created: r.created instanceof Date ? r.created : new Date(r.created), + id: Number(r.id), + totalBalanceChf: r.totalBalanceChf == null ? null : Number(r.totalBalanceChf), + plusBalanceChf: r.plusBalanceChf == null ? null : Number(r.plusBalanceChf), + minusBalanceChf: r.minusBalanceChf == null ? null : Number(r.minusBalanceChf), + fxPnlChf: r.fxPnlChf == null ? null : Number(r.fxPnlChf), + btcPriceChf, + balancesByType, + }; + }); + + if (!rows.length && after != null) await this.assertEmptyResultIsEndOfData(after); + return rows; + } + // After an empty main-query result with a keyset cursor, fail loud when the cursor id is gone: the row-value // subquery would return NULL and `(created, id) > (NULL, :afterId)` is NULL in Postgres → WHERE excludes every // row → silent empty result that callers misread as end-of-data. Only invoked when the main query already diff --git a/src/subdomains/supporting/log/log.service.ts b/src/subdomains/supporting/log/log.service.ts index d122a4708e..da29c6fa5f 100644 --- a/src/subdomains/supporting/log/log.service.ts +++ b/src/subdomains/supporting/log/log.service.ts @@ -13,7 +13,7 @@ import { LogSeverity, MAX_VALIDITY_SWEEP_ROWS, } from './log.entity'; -import { FinancialLogAssetPrice, LogRepository } from './log.repository'; +import { FinancialLogAssetPrice, FinancialLogSummary, LogRepository } from './log.repository'; @Injectable() export class LogService { @@ -154,6 +154,17 @@ export class LogService { return this.logRepo.getFinancialLogAssetPrices(from, to, limit, after); } + async getFinancialLogSummaries( + btcAssetId?: number, + from?: Date, + dailySample?: boolean, + to?: Date, + limit?: number, + after?: number, // id of the last row of the previous page; NEVER a Date/created value + ): Promise { + return this.logRepo.getFinancialLogSummaries(btcAssetId, from, dailySample, to, limit, after); + } + async getLatestFinancialLog(): Promise { return this.logRepo.getLatestFinancialLog(); }