From a266ae90d2eaf532e987b1e0d67f4e0b3521fc33 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 11:54:37 +0200 Subject: [PATCH 1/3] perf(payment): serve the asset list from the specification cache and widen the window to one hour (#4522) * perf(payment): serve the asset list from the specification cache The fiat endpoint was moved to the cached repository earlier; the asset list still issued the same query per request. This is the second call site the issue names. * perf(payment): widen the specification cache window to one hour The specifications are edited outside the application, so the cache does not have to follow a write path, and the cache lives in the process and is dropped on every restart. The window is passed explicitly, and two assertions pin it: one fails if the argument is dropped and the 5 minute default applies again, the other fails if the entry never expires. * docs(payment): narrow the specification cache comment to a checkable claim The sentence claimed that no write path exists anywhere in the codebase. That is an absolute statement a reader cannot check and that any future commit can silently invalidate. The class itself carries the same information: it declares read methods only. --- .../asset/__tests__/asset.controller.spec.ts | 76 ++++++++++++++ src/shared/models/asset/asset.controller.ts | 2 +- ...ansaction-specification.repository.spec.ts | 99 +++++++++++++++++++ .../transaction-specification.repository.ts | 7 +- 4 files changed, 180 insertions(+), 4 deletions(-) create mode 100644 src/shared/models/asset/__tests__/asset.controller.spec.ts create mode 100644 src/subdomains/supporting/payment/repositories/__tests__/transaction-specification.repository.spec.ts diff --git a/src/shared/models/asset/__tests__/asset.controller.spec.ts b/src/shared/models/asset/__tests__/asset.controller.spec.ts new file mode 100644 index 0000000000..5510dc072c --- /dev/null +++ b/src/shared/models/asset/__tests__/asset.controller.spec.ts @@ -0,0 +1,76 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { ConfigService } from 'src/config/config'; +import { Blockchain } from 'src/integration/blockchain/shared/enums/blockchain.enum'; +import { + TransactionDirection, + TransactionSpecification, +} from 'src/subdomains/supporting/payment/entities/transaction-specification.entity'; +import { TransactionSpecificationRepository } from 'src/subdomains/supporting/payment/repositories/transaction-specification.repository'; +import { EntityManager } from 'typeorm'; +import { RepositoryFactory } from '../../../repositories/repository.factory'; +import { createCustomAsset } from '../__mocks__/asset.entity.mock'; +import { AssetController } from '../asset.controller'; +import { AssetService } from '../asset.service'; + +function createSpec(values: Partial): TransactionSpecification { + return Object.assign(new TransactionSpecification(), { + system: Blockchain.ETHEREUM, + minVolume: 1, + minFee: 0, + ...values, + }); +} + +// The specifications must come from the cache: find() is stubbed to reject, so an uncached read fails the +// test instead of silently passing. +describe('AssetController.getAllAsset', () => { + const usdt = createCustomAsset({ dexName: 'USDT', sellable: true, approxPriceChf: 1 }); + const specs = [createSpec({ asset: 'USDT', direction: TransactionDirection.OUT, minVolume: 4 })]; + + let controller: AssetController; + let assetService: DeepMocked; + let specRepo: TransactionSpecificationRepository; + let find: jest.SpyInstance; + let findCached: jest.SpyInstance; + + beforeAll(() => new ConfigService()); // sets module-level Config (AssetDtoMapper reads tradingLimits) + + beforeEach(() => { + assetService = createMock(); + specRepo = new TransactionSpecificationRepository(createMock()); + + find = jest.spyOn(specRepo, 'find').mockRejectedValue(new Error('uncached specification query')); + findCached = jest.spyOn(specRepo, 'findCached').mockResolvedValue(specs); + + assetService.getAllBlockchainAssets.mockResolvedValue([usdt]); + + controller = new AssetController(assetService, { + transactionSpecification: specRepo, + } as unknown as RepositoryFactory); + }); + + it('reads the specifications from the cache', async () => { + await controller.getAllAsset(undefined, { includePrivate: 'false' }); + + expect(findCached).toHaveBeenCalledTimes(1); + expect(findCached).toHaveBeenCalledWith('all'); + expect(find).not.toHaveBeenCalled(); + }); + + it('applies the cached specification to the returned limits', async () => { + const [dto] = await controller.getAllAsset(undefined, { includePrivate: 'false' }); + + expect(dto.limits.minVolume).toBe(4); + }); + + it('picks the outgoing specification of the asset', async () => { + findCached.mockResolvedValue([ + createSpec({ asset: 'USDT', direction: TransactionDirection.IN, minVolume: 222 }), + createSpec({ asset: 'USDT', direction: TransactionDirection.OUT, minVolume: 8 }), + ]); + + const [dto] = await controller.getAllAsset(undefined, { includePrivate: 'false' }); + + expect(dto.limits.minVolume).toBe(8); + }); +}); diff --git a/src/shared/models/asset/asset.controller.ts b/src/shared/models/asset/asset.controller.ts index b83479c299..5758d60562 100644 --- a/src/shared/models/asset/asset.controller.ts +++ b/src/shared/models/asset/asset.controller.ts @@ -36,7 +36,7 @@ export class AssetController { const queryBlockchains = blockchains?.split(',').map((value) => value as Blockchain); const specRepo = this.repoFactory.transactionSpecification; - const specs = await specRepo.find(); + const specs = await specRepo.findCached('all'); return this.assetService .getAllBlockchainAssets(queryBlockchains ?? jwt?.blockchains ?? [], includePrivate === 'true') diff --git a/src/subdomains/supporting/payment/repositories/__tests__/transaction-specification.repository.spec.ts b/src/subdomains/supporting/payment/repositories/__tests__/transaction-specification.repository.spec.ts new file mode 100644 index 0000000000..e86e0bfb93 --- /dev/null +++ b/src/subdomains/supporting/payment/repositories/__tests__/transaction-specification.repository.spec.ts @@ -0,0 +1,99 @@ +import { createMock } from '@golevelup/ts-jest'; +import { EntityManager } from 'typeorm'; +import { TransactionDirection, TransactionSpecification } from '../../entities/transaction-specification.entity'; +import { TransactionSpecificationRepository } from '../transaction-specification.repository'; + +function createSpec(values: Partial): TransactionSpecification { + return Object.assign(new TransactionSpecification(), { system: 'Fiat', minVolume: 1, minFee: 0, ...values }); +} + +// The repository extends CachedRepository so that FiatController.getAllFiat and AssetController.getAllAsset +// stop issuing one query per request. These tests pin the caching behaviour itself, because the switch from +// find() to findCached() leaves the returned data unchanged. +describe('TransactionSpecificationRepository.findCached', () => { + const specs = [createSpec({ asset: 'EUR', direction: TransactionDirection.IN, minVolume: 5 })]; + + let repository: TransactionSpecificationRepository; + let find: jest.SpyInstance; + + beforeEach(() => { + repository = new TransactionSpecificationRepository(createMock()); + find = jest.spyOn(repository, 'find').mockResolvedValue(specs); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('queries the database once and serves the following calls from the cache', async () => { + await expect(repository.findCached('all')).resolves.toBe(specs); + await expect(repository.findCached('all')).resolves.toBe(specs); + await expect(repository.findCached('all')).resolves.toBe(specs); + + expect(find).toHaveBeenCalledTimes(1); + }); + + it('issues a single query for concurrent calls', async () => { + const [first, second] = await Promise.all([repository.findCached('all'), repository.findCached('all')]); + + expect(first).toBe(specs); + expect(second).toBe(specs); + expect(find).toHaveBeenCalledTimes(1); + }); + + it('queries again after invalidateCache', async () => { + await repository.findCached('all'); + + repository.invalidateCache(); + const updated = [createSpec({ asset: 'CHF', direction: TransactionDirection.IN, minVolume: 9 })]; + find.mockResolvedValue(updated); + + await expect(repository.findCached('all')).resolves.toBe(updated); + expect(find).toHaveBeenCalledTimes(2); + }); + + it('propagates a failing query instead of returning a stale or empty result', async () => { + find.mockRejectedValue(new Error('connection terminated')); + + await expect(repository.findCached('all')).rejects.toThrow('connection terminated'); + + find.mockResolvedValue(specs); + await expect(repository.findCached('all')).resolves.toBe(specs); + expect(find).toHaveBeenCalledTimes(2); + }); + + // The repository passes CacheItemResetPeriod.EVERY_HOUR explicitly. Dropping that argument would fall back + // to the CachedRepository default of 5 minutes without any other visible effect, so the window is asserted + // here: the first case fails on the default, the second one fails if the entry never expires. + it('serves the cached list well past the 5 minute default', async () => { + jest.useFakeTimers(); + + await repository.findCached('all'); + jest.advanceTimersByTime(59 * 60 * 1000); + await repository.findCached('all'); + + expect(find).toHaveBeenCalledTimes(1); + }); + + it('queries again once the hour has passed', async () => { + jest.useFakeTimers(); + + await repository.findCached('all'); + jest.advanceTimersByTime(61 * 60 * 1000); + await repository.findCached('all'); + + expect(find).toHaveBeenCalledTimes(2); + }); + + it('keeps a separate cache entry per key', async () => { + const inSpecs = [createSpec({ direction: TransactionDirection.IN })]; + const outSpecs = [createSpec({ direction: TransactionDirection.OUT })]; + find.mockResolvedValueOnce(inSpecs).mockResolvedValueOnce(outSpecs); + + await expect(repository.findCached('in')).resolves.toBe(inSpecs); + await expect(repository.findCached('out')).resolves.toBe(outSpecs); + await expect(repository.findCached('in')).resolves.toBe(inSpecs); + + expect(find).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts b/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts index d2ad59f615..b68dac7a33 100644 --- a/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts +++ b/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts @@ -1,15 +1,16 @@ import { Injectable } from '@nestjs/common'; import { Active, isAsset } from 'src/shared/models/active'; import { CachedRepository } from 'src/shared/repositories/cached.repository'; +import { CacheItemResetPeriod } from 'src/shared/utils/async-cache'; import { EntityManager } from 'typeorm'; import { TransactionDirection, TransactionSpecification } from '../entities/transaction-specification.entity'; -// Same cache duration as Fiat and Country (CachedRepository default EVERY_5_MINUTES). -// Pure reference/master data: no save/update/insert/delete path exists in the codebase. +// Cached for an hour, longer than the CachedRepository default of EVERY_5_MINUTES. +// This repository exposes read methods only, so nothing here has to invalidate the cache. @Injectable() export class TransactionSpecificationRepository extends CachedRepository { constructor(manager: EntityManager) { - super(TransactionSpecification, manager); + super(TransactionSpecification, manager, CacheItemResetPeriod.EVERY_HOUR); } getProps(param: Active): { system: string; asset: string } { From b1982b01213473e4b5f4019868e4bfc7fe9398f2 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 12:23:17 +0200 Subject: [PATCH 2/3] docs(kyc): document the staff KYC 403 and pin it in the tests (#4567) Follow-up to the two review points left open when the staff KYC error code was merged. The protected-file endpoint now documents the structured 403 it can answer with, the way the same controller already documents the analogous 2FA case. The tests asserted on ForbiddenException, which the new exception extends - a regression back to the generic answer would have stayed green. They now pin the concrete exception and its response body, and a new case pins the counterpart: a wrong role must keep answering generically, without the code, so the two situations stay distinguishable in both directions. --- .../generic/kyc/controllers/kyc.controller.ts | 4 +++ .../services/__tests__/kyc.service.spec.ts | 33 ++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/subdomains/generic/kyc/controllers/kyc.controller.ts b/src/subdomains/generic/kyc/controllers/kyc.controller.ts index 67acea4473..539d1d1002 100644 --- a/src/subdomains/generic/kyc/controllers/kyc.controller.ts +++ b/src/subdomains/generic/kyc/controllers/kyc.controller.ts @@ -80,6 +80,9 @@ const MergedResponse = { type: MergedDto, }; const TfaResponse = { description: '2FA is required' }; +// Staff reaching a protected file without a completed identification; the body carries +// code STAFF_KYC_REQUIRED so clients can branch on it rather than on the message text. +const StaffKycResponse = { description: 'Staff access requires a completed identification' }; @ApiTags('KYC') @Controller({ path: 'kyc', version: [GetConfig().kycVersion] }) @@ -163,6 +166,7 @@ export class KycController { @Get('file/:id') @ApiBearerAuth() + @ApiForbiddenResponse(StaffKycResponse) @UseGuards(OptionalJwtAuthGuard) async getFile( @GetJwt() jwt: JwtPayload | undefined, diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index 582086c6aa..c0e621cfa7 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -3,6 +3,7 @@ import { ForbiddenException } from '@nestjs/common'; import { Configuration, ConfigService } from 'src/config/config'; import { BlobContent } from 'src/integration/infrastructure/storage/storage.service'; import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; +import { StaffKycRequiredException } from 'src/shared/auth/exceptions/staff-kyc-required.exception'; import { SetStaffKycClearance } from 'src/shared/auth/staff-kyc-clearance'; import { UserRole } from 'src/shared/auth/user-role.enum'; import { createCustomCountry } from 'src/shared/models/country/__mocks__/country.entity.mock'; @@ -162,6 +163,19 @@ describe('KycService getFileByUid protected-file access', () => { expect(documentService.downloadFile).not.toHaveBeenCalled(); }); + // The counterpart: a wrong role is a different problem with a different fix, so it must NOT produce + // the KYC answer — otherwise the two cases are indistinguishable again, just in the other direction. + it('answers a non-privileged role generically, not with the KYC code', async () => { + SetStaffKycClearance([]); + kycFileService.getKycFile.mockResolvedValue(kycFile()); + + const error = await service.getFileByUid('FILE-UID', jwtFor(UserRole.USER), ip).catch((e) => e); + + expect(error).toBeInstanceOf(ForbiddenException); + expect(error).not.toBeInstanceOf(StaffKycRequiredException); + expect(error.getResponse()).not.toHaveProperty('code'); + }); + it('forbids an unauthenticated request (no JWT) from a protected file', async () => { kycFileService.getKycFile.mockResolvedValue(kycFile()); @@ -177,10 +191,27 @@ describe('KycService getFileByUid protected-file access', () => { SetStaffKycClearance([]); kycFileService.getKycFile.mockResolvedValue(kycFile()); - await expect(service.getFileByUid('FILE-UID', jwtFor(role), ip)).rejects.toBeInstanceOf(ForbiddenException); + // Pins the concrete exception, not just the ForbiddenException it extends: the point of this + // answer is that the caller can tell a missing identification apart from a missing role, and an + // assertion on the base type would stay green if it fell back to the generic 403. + await expect(service.getFileByUid('FILE-UID', jwtFor(role), ip)).rejects.toBeInstanceOf( + StaffKycRequiredException, + ); expect(documentService.downloadFile).not.toHaveBeenCalled(); }); + it('answers with the machine-readable code', async () => { + SetStaffKycClearance([]); + kycFileService.getKycFile.mockResolvedValue(kycFile()); + + const error = await service.getFileByUid('FILE-UID', jwtFor(role), ip).catch((e) => e); + + expect(error.getResponse()).toEqual({ + code: 'STAFF_KYC_REQUIRED', + message: expect.stringContaining('KYC level 50'), + }); + }); + it('still serves a non-protected file', async () => { SetStaffKycClearance([]); kycFileService.getKycFile.mockResolvedValue(kycFile({ protected: false })); From add2d077ac7e1d55f0afa9d2e7b1db7d86539095 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 12:41:32 +0200 Subject: [PATCH 3/3] docs(payment): correct the specification cache comment (#4570) * docs(payment): correct the specification cache comment The comment claimed the repository exposes read methods only. It does not: over CachedRepository and BaseRepository it inherits TypeORM's public save, update, insert and delete, none of which touch the cache. The sentence was meant to explain why no caller invalidates the cache today, but it stated something a reader can disprove by opening the base class. Replaced by the obligation itself, which stays true no matter what the class inherits or which callers exist. * docs(payment): drop the cache invalidation sentence instead of rewording it Third attempt at that sentence, third defect: after the unverifiable claim and the factually wrong one, the remaining version was misleading. invalidateCache() clears the caches of one instance, while RepositoryFactory builds its own alongside the DI provider, and TransactionHelper keeps a separate copy on its own cron. The instance question is not specific to this repository and does not belong in this file. What remains is the one statement a reader can check right here. --- .../payment/repositories/transaction-specification.repository.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts b/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts index b68dac7a33..c7ec06046f 100644 --- a/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts +++ b/src/subdomains/supporting/payment/repositories/transaction-specification.repository.ts @@ -6,7 +6,6 @@ import { EntityManager } from 'typeorm'; import { TransactionDirection, TransactionSpecification } from '../entities/transaction-specification.entity'; // Cached for an hour, longer than the CachedRepository default of EVERY_5_MINUTES. -// This repository exposes read methods only, so nothing here has to invalidate the cache. @Injectable() export class TransactionSpecificationRepository extends CachedRepository { constructor(manager: EntityManager) {