Release: develop -> main

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(<table> + '_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"`);
+  }
+};

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<ReturnType<BankService['getBank']>>,
+    bank: NonNullable<Awaited<ReturnType<BankService['getBank']>>>,
     reference?: string,
   ): BankInfoDto & { isPersonalIban: boolean; reference?: string } {
     return {

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<GsService>;
+  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<GsService>();
+    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();
+  });
+});