diff --git a/migration/1784885000000-AddLiquidityOrderNotSentRecheckDue.js b/migration/1784885000000-AddLiquidityOrderNotSentRecheckDue.js new file mode 100644 index 0000000000..7cf40061b3 --- /dev/null +++ b/migration/1784885000000-AddLiquidityOrderNotSentRecheckDue.js @@ -0,0 +1,49 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * Marks a liquidity management order somebody has released as never sent, until the venue has been asked + * once more. While the column is set the order stays quarantined: the release is accepted, not yet in effect. + * + * Concluding that a request never reached the venue is the one judgement nothing here can verify from the + * outside, and it can be made at the very moment reconciliation is watching the venue confirm that same + * order. Were the release to take effect at once, the order would be terminal — its rule free to plan + * against funds that are in fact committed — before anything could contradict it. Waiting for one machine + * answer costs a single reconciliation pass, normally seconds, and closes that window entirely. + * + * Two exceptions end the wait without an answer, both about liveness rather than evidence: an order no + * integration can look up any more, and a venue that has answered nothing for long enough. + * + * The column records work outstanding, not when the release was asked for: that goes into the order's own + * reason, which nothing clears. Indexed so the wait never turns into a scan. + * + * Purely additive and nullable, no backfill: existing rows read NULL, which is exactly right — they predate + * this path entirely and have no release awaiting confirmation. The index is the deterministic TypeORM name + * for a single-column index on this table, matching the entity's `@Index()`. + * + * @class + * @implements {MigrationInterface} + */ +module.exports = class AddLiquidityOrderNotSentRecheckDue1784885000000 { + name = 'AddLiquidityOrderNotSentRecheckDue1784885000000'; + + /** + * @param {QueryRunner} queryRunner + */ + async up(queryRunner) { + await queryRunner.query(`ALTER TABLE "liquidity_management_order" ADD "notSentRecheckDue" TIMESTAMP`); + await queryRunner.query( + `CREATE INDEX "IDX_cfc953689f0268e33cf14c1cc0" ON "liquidity_management_order" ("notSentRecheckDue")`, + ); + } + + /** + * @param {QueryRunner} queryRunner + */ + async down(queryRunner) { + await queryRunner.query(`DROP INDEX "public"."IDX_cfc953689f0268e33cf14c1cc0"`); + await queryRunner.query(`ALTER TABLE "liquidity_management_order" DROP COLUMN "notSentRecheckDue"`); + } +}; diff --git a/src/integration/exchange/services/__tests__/scrypt-websocket-connection.spec.ts b/src/integration/exchange/services/__tests__/scrypt-websocket-connection.spec.ts index 1e35fa95ab..33e8ea199d 100644 --- a/src/integration/exchange/services/__tests__/scrypt-websocket-connection.spec.ts +++ b/src/integration/exchange/services/__tests__/scrypt-websocket-connection.spec.ts @@ -1,6 +1,10 @@ import { EventEmitter as MockEventEmitter } from 'events'; import Ws from 'ws'; -import { ScryptMessageType, ScryptWebSocketConnection } from '../scrypt-websocket-connection'; +import { + ScryptMessageType, + ScryptRequestTimeoutError, + ScryptWebSocketConnection, +} from '../scrypt-websocket-connection'; type MockWebSocketInstance = MockEventEmitter & { url: string; @@ -999,4 +1003,57 @@ describe('ScryptWebSocketConnection', () => { expect(cb).toHaveBeenCalledTimes(1); }); + + describe('unanswered requests', () => { + const REQUEST_TIMEOUT_MS = 30000; + + function subscribeReqIds(ws: MockWebSocketInstance, streamName: ScryptMessageType): number[] { + return ws.send.mock.calls + .map(([payload]) => JSON.parse(payload as string)) + .filter((msg) => msg.type === 'subscribe' && msg.streams?.[0]?.name === streamName) + .map((msg) => msg.reqid as number); + } + + it('retries a read once when the venue never answers, instead of failing the caller', async () => { + const ws = await firstConnectWithStream(); + const streamName = ScryptMessageType.EXECUTION_REPORT; + + const fetchPromise = connection.fetch(streamName); + await flushPromises(); + expect(subscribeReqIds(ws, streamName)).toHaveLength(1); + + // silence for the whole deadline — the venue simply does not reply + jest.advanceTimersByTime(REQUEST_TIMEOUT_MS); + await flushPromises(); + + const reqIds = subscribeReqIds(ws, streamName); + expect(reqIds).toHaveLength(2); + + ws.emit( + 'message', + JSON.stringify({ reqid: reqIds[1], type: streamName, initial: true, data: [{ ClOrdID: 'ord-after-retry' }] }), + ); + + await expect(fetchPromise).resolves.toEqual([{ ClOrdID: 'ord-after-retry' }]); + expect(loggerWarn).toHaveBeenCalledWith(expect.stringContaining(`Retrying fetch ${streamName}`)); + }); + + it('surfaces an unanswered request as ScryptRequestTimeoutError, not a plain Error', async () => { + const ws = await firstConnectWithStream(); + const streamName = ScryptMessageType.EXECUTION_REPORT; + + const fetchPromise = connection.fetch(streamName); + const assertion = expect(fetchPromise).rejects.toBeInstanceOf(ScryptRequestTimeoutError); + await flushPromises(); + + // both the first attempt and its retry go unanswered + jest.advanceTimersByTime(REQUEST_TIMEOUT_MS); + await flushPromises(); + jest.advanceTimersByTime(REQUEST_TIMEOUT_MS); + await flushPromises(); + + await assertion; + expect(subscribeReqIds(ws, streamName)).toHaveLength(2); + }); + }); }); diff --git a/src/integration/exchange/services/__tests__/scrypt.service.spec.ts b/src/integration/exchange/services/__tests__/scrypt.service.spec.ts index fa37135c86..28ca6f26c7 100644 --- a/src/integration/exchange/services/__tests__/scrypt.service.spec.ts +++ b/src/integration/exchange/services/__tests__/scrypt.service.spec.ts @@ -4,7 +4,14 @@ import { ScryptTransactionStatus, ScryptTransactionType, } from '../../dto/scrypt.dto'; -import { ScryptMessageType, ScryptWebSocketConnection } from '../scrypt-websocket-connection'; +import { + ScryptAmendRejectedError, + ScryptMessageType, + ScryptRequestTimeoutError, + ScryptUnconfirmedWriteError, + ScryptVenueRejectionError, + ScryptWebSocketConnection, +} from '../scrypt-websocket-connection'; import { ScryptService } from '../scrypt.service'; jest.mock('src/config/config', () => { @@ -546,4 +553,97 @@ describe('ScryptService', () => { ]); }); }); + describe('checkTrade — the amend write boundary', () => { + function stubAmendPath(editOutcome: Error): void { + jest.spyOn(service as any, 'getOrderStatus').mockResolvedValue({ + id: 'dfx-lm-7', + status: ScryptOrderStatus.PARTIALLY_FILLED, + price: 1, + remainingQuantity: 5, + }); + jest.spyOn(service as any, 'getTradePrice').mockResolvedValue(2); + jest.spyOn(service as any, 'editOrder').mockRejectedValue(editOutcome); + jest.spyOn(service as any, 'cancelOrder').mockResolvedValue(undefined); + } + + it('propagates an unconfirmed amend instead of swallowing it', async () => { + // Regression guard: the amend used to be wrapped in a catch that cancelled and returned false, so the + // caller never learned that a replacement order might be live at the venue under the reserved id. + stubAmendPath(new ScryptRequestTimeoutError('Timeout waiting for ExecutionReport update after 60000ms')); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date(), 'dfx-lm-7-1')).rejects.toBeInstanceOf( + ScryptUnconfirmedWriteError, + ); + expect((service as any).cancelOrder).not.toHaveBeenCalled(); + }); + + it('carries the reserved replacement reference on the raised error', async () => { + stubAmendPath(new ScryptRequestTimeoutError('Timeout waiting for ExecutionReport update after 60000ms')); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date(), 'dfx-lm-7-1')).rejects.toMatchObject({ + reference: 'dfx-lm-7-1', + }); + }); + + it('forgets a cached open order when the follow-up cancel goes unconfirmed', async () => { + // the cancel is a write as well: unconfirmed, it may have taken effect while the cached report still + // shows the order open — and a non-terminal entry is never refreshed, so every later check would wait + // on a picture that cannot change + stubAmendPath(new ScryptVenueRejectionError('Scrypt order edit rejected: price out of band')); + jest.spyOn(service as any, 'cancelOrder').mockRejectedValue(new ScryptRequestTimeoutError('Request timeout')); + (service as any).executionReports.set('dfx-lm-7', { ClOrdID: 'dfx-lm-7', OrdStatus: ScryptOrderStatus.NEW }); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date(), 'dfx-lm-7-1')).rejects.toMatchObject({ + message: expect.stringContaining('cancel went unconfirmed'), + }); + + expect((service as any).executionReports.has('dfx-lm-7')).toBe(false); + }); + + it('keeps the cached order when the cancel is confirmed', async () => { + stubAmendPath(new ScryptVenueRejectionError('Scrypt order edit rejected: price out of band')); + (service as any).executionReports.set('dfx-lm-7', { ClOrdID: 'dfx-lm-7', OrdStatus: ScryptOrderStatus.NEW }); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date(), 'dfx-lm-7-1')).rejects.toBeInstanceOf( + ScryptAmendRejectedError, + ); + + expect((service as any).executionReports.has('dfx-lm-7')).toBe(true); + }); + + it('keeps waiting on a pending order however old it is — pending is observed, not unknown', async () => { + // quarantining it would make reconciliation find the reference, hand the order back, and the next + // completion check quarantine it again: a loop, not a resolution + jest.spyOn(service as any, 'getOrderStatus').mockResolvedValue({ + id: 'dfx-lm-7', + status: ScryptOrderStatus.PENDING_NEW, + remainingQuantity: 5, + }); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date(Date.now() - 120 * 60 * 1000))).resolves.toBe( + false, + ); + }); + + it('keeps waiting on a pending order that is still young', async () => { + jest.spyOn(service as any, 'getOrderStatus').mockResolvedValue({ + id: 'dfx-lm-7', + status: ScryptOrderStatus.PENDING_NEW, + remainingQuantity: 5, + }); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date())).resolves.toBe(false); + }); + + it('cancels on an explicit rejection, but reports the refusal and the spent reference', async () => { + // A rejection is a reply: nothing was created, so cancelling is safe. The caller still has to learn + // about it — the replacement reference is burnt at the venue and must not be derived again. + stubAmendPath(new ScryptVenueRejectionError('Scrypt order edit rejected: price out of band')); + + await expect(service.checkTrade('dfx-lm-7', 'EUR', 'USDT', new Date(), 'dfx-lm-7-1')).rejects.toMatchObject({ + spentReference: 'dfx-lm-7-1', + }); + expect((service as any).cancelOrder).toHaveBeenCalled(); + }); + }); }); diff --git a/src/integration/exchange/services/scrypt-websocket-connection.ts b/src/integration/exchange/services/scrypt-websocket-connection.ts index d93a6adb41..3e53d66438 100644 --- a/src/integration/exchange/services/scrypt-websocket-connection.ts +++ b/src/integration/exchange/services/scrypt-websocket-connection.ts @@ -50,6 +50,81 @@ export function isTransientWsError(e: Error): boolean { return TRANSIENT_WS_ERROR_MARKERS.some((m) => e.message?.toLowerCase().includes(m.toLowerCase())); } +/** + * A request was sent but no answer arrived within its deadline. + * + * Deliberately its own type rather than another entry in TRANSIENT_WS_ERROR_MARKERS: those markers describe + * a socket that demonstrably dropped the request, so retrying is safe for anything. A timeout describes + * silence — the venue may or may not have acted. Only idempotent reads may retry it; every write path must + * translate it into an unknown outcome. Matching on the message text instead would make that distinction + * impossible to enforce, because both kinds of timeout would read the same. + */ +export class ScryptRequestTimeoutError extends Error {} + +/** + * A write that may or may not have taken effect at the venue — raised where an order was created, amended or + * restarted and no reply confirmed the outcome. + * + * Distinct from {@link ScryptRequestTimeoutError}, which describes only *how* the call ended: the same + * dropped socket is harmless on a read and unresolved on a write, so the distinction that matters to the + * caller is the side effect, not the transport. Anything carrying this type must be quarantined and + * reconciled, never repeated. + */ +export class ScryptUnconfirmedWriteError extends Error { + constructor( + message: string, + readonly reference: string | undefined, + ) { + super(message); + } +} + +/** + * An order the venue once acknowledged can no longer be found in its state. + * + * Not a failure: the order may have completed or been cancelled outside our view, and we cannot tell which. + * Treating it as failed would release the rule to open a second position against the same funds. + */ +export class ScryptOrderNotFoundError extends Error {} + +/** + * An amend the venue refused. The replacement was never created, so the ORIGINAL order is still live — and + * its reference is spent, because the venue requires references to be unique. Carries it so the caller can + * record it and derive a fresh one next time instead of reusing a burnt reference forever. + */ +export class ScryptAmendRejectedError extends Error { + constructor( + message: string, + readonly spentReference: string | undefined, + ) { + super(message); + } +} + +/** + * The venue sent an explicit error in reply to one specific request. + * + * Deliberately NOT a rejection: `unknown reqid` arrives the same way and means the venue lost our request + * context, which for a mutation is as open as silence. Callers that can tell the two apart narrow it; callers + * that cannot must keep treating it as an unresolved outcome. + */ +export class ScryptErrorResponseError extends Error {} + +/** + * The venue replied and refused the request. This is the ONLY evidence that a write did not take effect — + * everything else leaves the outcome open. + * + * A type rather than a set of message patterns: a rejection is now impossible to miss by phrasing a message + * differently, and impossible to fake by a transport error that happens to contain the word. Every path that + * turns a venue refusal into an exception must use this type, or the caller will retry a settled outcome + * forever. + */ +export class ScryptVenueRejectionError extends Error {} + +export function isVenueRejection(e: Error): boolean { + return e instanceof ScryptVenueRejectionError; +} + interface ScryptRequest { reqid?: number; type: ScryptRequestType | ScryptMessageType; @@ -120,7 +195,7 @@ export class ScryptWebSocketConnection { } }; - return this.retryOnTransientWsError(doFetch, `fetch ${streamName}`); + return this.retryIdempotentRead(doFetch, `fetch ${streamName}`); } async fetchAll(streamName: ScryptMessageType, filters?: Record): Promise { @@ -155,7 +230,7 @@ export class ScryptWebSocketConnection { } }; - return this.retryOnTransientWsError(doFetch, `fetchAll ${streamName}`); + return this.retryIdempotentRead(doFetch, `fetchAll ${streamName}`); } // Register a callback fired after a successful RE-connect (not the first connect). Used to re-fetch state that @@ -164,11 +239,19 @@ export class ScryptWebSocketConnection { this.reconnectCallbacks.push(callback); } - private async retryOnTransientWsError(operation: () => Promise, label: string): Promise { + /** + * Retry wrapper for IDEMPOTENT READS ONLY — `fetch` and `fetchAll`. Never widen this to a call that can + * create, amend or cancel an order, or move funds: it retries on timeout, and a timed-out write may + * already have been executed by the venue. Write paths must surface the timeout so the caller can treat + * the outcome as unknown (see OrderOutcomeUnknownException in the liquidity-management subdomain). + */ + private async retryIdempotentRead(operation: () => Promise, label: string): Promise { try { return await operation(); } catch (error) { - if (isTransientWsError(error)) { + // A read that went unanswered is safe to repeat: re-subscribing to a snapshot stream has no side + // effect at the venue. Without this, a single silent 30s window ends the whole liquidity order. + if (isTransientWsError(error) || error instanceof ScryptRequestTimeoutError) { this.logger.warn(`Retrying ${label} after transient error: ${error.message}`); return operation(); } @@ -186,7 +269,7 @@ export class ScryptWebSocketConnection { return new Promise((resolve, reject) => { const timeoutId = setTimeout(() => { unsubscribe(); - reject(new Error(`Timeout waiting for ${streamName} update after ${timeoutMs}ms`)); + reject(new ScryptRequestTimeoutError(`Timeout waiting for ${streamName} update after ${timeoutMs}ms`)); }, timeoutMs); const unsubscribe = this.subscribe(streamName, (data) => { @@ -440,7 +523,7 @@ export class ScryptWebSocketConnection { return new Promise((resolve, reject) => { const timeout = setTimeout(() => { this.pendingRequests.delete(reqId); - reject(new Error(`Request timeout after ${timeoutMs}ms`)); + reject(new ScryptRequestTimeoutError(`Request timeout after ${timeoutMs}ms`)); }, timeoutMs); this.pendingRequests.set(reqId, { resolve, reject, timeout }); @@ -459,7 +542,11 @@ export class ScryptWebSocketConnection { if (message.type === ScryptMessageType.ERROR) { const errorMsg = typeof message.error === 'object' ? JSON.stringify(message.error) : message.error; - request.reject(new Error(`Scrypt error: ${errorMsg}`)); + // The venue answered this specific request negatively. Whether that settles the outcome depends on the + // reason — a malformed order is settled, a lost session is not — and Scrypt does not document its + // codes, so this stays a distinct type and the caller decides. Never silently a plain Error: that is + // what let a refusal look like a transport hiccup. + request.reject(new ScryptErrorResponseError(`Scrypt error: ${errorMsg}`)); } else { request.resolve(message); } diff --git a/src/integration/exchange/services/scrypt.service.ts b/src/integration/exchange/services/scrypt.service.ts index 8acdfcbadf..3d166a6789 100644 --- a/src/integration/exchange/services/scrypt.service.ts +++ b/src/integration/exchange/services/scrypt.service.ts @@ -27,7 +27,21 @@ import { ScryptWithdrawStatus, } from '../dto/scrypt.dto'; import { TradeChangedException } from '../exceptions/trade-changed.exception'; -import { ScryptMessageType, ScryptWebSocketConnection } from './scrypt-websocket-connection'; +import { + isVenueRejection, + ScryptAmendRejectedError, + ScryptMessageType, + ScryptOrderNotFoundError, + ScryptUnconfirmedWriteError, + ScryptVenueRejectionError, + ScryptWebSocketConnection, +} from './scrypt-websocket-connection'; + +/** + * After this long without a usable answer, an order the venue once acknowledged is treated as lost rather + * than merely slow. Shared by the "cannot be found" and the "stuck pending" paths so both give up together. + */ +const ORDER_LOST_AFTER_MINUTES = 60; @Injectable() export class ScryptService extends PricingProvider { @@ -122,6 +136,16 @@ export class ScryptService extends PricingProvider { return [ScryptOrderStatus.FILLED, ScryptOrderStatus.CANCELED, ScryptOrderStatus.REJECTED].includes(r.OrdStatus); } + /** + * Drop what we believe about an order, so the next lookup has to ask the venue. + * + * A non-terminal cached report is never replaced by a fetch, which is right while it is trustworthy and + * wrong the moment an unconfirmed write may have changed the order underneath it. + */ + private forgetExecutionReport(clOrdId: string): void { + this.executionReports.delete(clOrdId); + } + private cacheExecutionReport(r: ScryptExecutionReport): void { const existing = this.executionReports.get(r.ClOrdID); if (existing && this.isTerminalExecutionReport(existing) && !this.isTerminalExecutionReport(r)) return; @@ -215,8 +239,12 @@ export class ScryptService extends PricingProvider { amount: number, address: string, memo?: string, + // See placeOrder: the caller persists this before calling, so a timed-out withdrawal stays traceable. + // Two withdrawals (205'589.77 USDT on 15.07.2026, 553'823.67 USDT on 20.07.2026) executed at the venue + // while being recorded as failed here, because the generated id never left this stack frame. + reservedClReqId?: string, ): Promise { - const clReqId = randomUUID(); + const clReqId = reservedClReqId ?? randomUUID(); const withdrawData = { Quantity: amount.toString(), @@ -241,7 +269,7 @@ export class ScryptService extends PricingProvider { ); if (transaction.Status === ScryptTransactionStatus.REJECTED) { - throw new Error( + throw new ScryptVenueRejectionError( `Scrypt withdrawal rejected: ${transaction.RejectText ?? transaction.RejectReason ?? 'Unknown reason'}`, ); } @@ -341,7 +369,7 @@ export class ScryptService extends PricingProvider { return side === ScryptOrderSide.BUY ? price : 1 / price; } - async sell(from: string, to: string, amount: number): Promise { + async sell(from: string, to: string, amount: number, reservedClOrdId?: string): Promise { const { symbol, side } = await this.getTradePair(from, to); const price = await this.getOrderBookPrice(symbol, side); const sizeIncrement = await this.getSizeIncrement(symbol); @@ -352,10 +380,10 @@ export class ScryptService extends PricingProvider { const rawQty = side === ScryptOrderSide.SELL ? amount : amount / price; const orderQty = Util.floorToValue(rawQty, sizeIncrement); - return this.placeAndReturnId(symbol, side, orderQty, price); + return this.placeAndReturnId(symbol, side, orderQty, price, reservedClOrdId); } - async buy(from: string, to: string, amount: number): Promise { + async buy(from: string, to: string, amount: number, reservedClOrdId?: string): Promise { const { symbol, side } = await this.getTradePair(from, to); const price = await this.getOrderBookPrice(symbol, side); const sizeIncrement = await this.getSizeIncrement(symbol); @@ -366,7 +394,7 @@ export class ScryptService extends PricingProvider { const rawQty = side === ScryptOrderSide.BUY ? amount : amount / price; const orderQty = Util.floorToValue(rawQty, sizeIncrement); - return this.placeAndReturnId(symbol, side, orderQty, price); + return this.placeAndReturnId(symbol, side, orderQty, price, reservedClOrdId); } private async getSizeIncrement(symbol: string): Promise { @@ -379,6 +407,7 @@ export class ScryptService extends PricingProvider { side: ScryptOrderSide, orderQty: number, price: number, + reservedClOrdId?: string, ): Promise { const response = await this.placeOrder( symbol, @@ -387,10 +416,39 @@ export class ScryptService extends PricingProvider { ScryptOrderType.LIMIT, ScryptTimeInForce.GOOD_TILL_CANCEL, price, + reservedClOrdId, ); return response.id; } + /** + * Reconciliation lookup: does the venue know this withdrawal reference at all? + * + * Distinct from `getWithdrawalStatus`, which answers from the live push cache only. After a timeout that + * cache is exactly what cannot be trusted — the push may be what went missing — so this falls back to the + * venue's own history. A `null` result therefore means "the venue has no record", not "we have not seen it". + */ + async findWithdrawal(clReqId: string): Promise { + const cached = this.balanceTransactions.get(clReqId); + // A terminal record cannot change; a non-terminal one may be stale because the terminal push was the + // thing that went missing, so it must not shortcut the lookup. + if (cached && this.isTerminalBalanceTransaction(cached)) return cached; + + const transactions = await this.connection.fetchAll( + ScryptMessageType.BALANCE_TRANSACTION, + ); + + const found = transactions.find((t) => t.ClReqID === clReqId); + if (!found) return cached ?? null; + + // Feed the recovery back into the live cache. `getWithdrawalStatus` reads only from there, so an order + // that leaves quarantine on the strength of this lookup would otherwise poll a reference the cache still + // does not know and never complete. + this.cacheBalanceTransaction(found); + + return found; + } + async getOrderStatus(clOrdId: string): Promise { // Try in-memory cache first let report = this.executionReports.get(clOrdId); @@ -425,14 +483,28 @@ export class ScryptService extends PricingProvider { }; } - async checkTrade(clOrdId: string, from: string, to: string, orderCreated?: Date): Promise { + /** + * @param replacementClOrdId reference to use if this check has to amend or restart the order. Must be + * reproducible from the order row by the caller, so a timed-out replacement stays findable. + */ + async checkTrade( + clOrdId: string, + from: string, + to: string, + orderCreated?: Date, + replacementClOrdId?: string, + // Invoked immediately before a replacement is sent, so the caller can make the reference durable first. + // Without that, a replacement whose confirmation is lost is neither the current reference nor a spent + // one, and the next pass derives it a second time. + claimReplacement?: () => Promise, + ): Promise { const orderInfo = await this.getOrderStatus(clOrdId); if (!orderInfo) { // If the order is older than 1 hour and still not found, it's lost const ageMinutes = orderCreated ? Util.minutesDiff(orderCreated) : 0; - if (ageMinutes > 60) { - throw new Error( - `Order ${clOrdId} not found after ${Math.round(ageMinutes)} minutes — likely completed or cancelled outside of tracked state`, + if (ageMinutes > ORDER_LOST_AFTER_MINUTES) { + throw new ScryptOrderNotFoundError( + `Order ${clOrdId} not found after ${Math.round(ageMinutes)} minutes — it may have completed or been cancelled outside of tracked state`, ); } @@ -451,19 +523,52 @@ export class ScryptService extends PricingProvider { this.logger.verbose(`Order ${clOrdId}: price changed ${orderInfo.price} -> ${currentPrice}, updating order`); try { - const newId = await this.editOrder(clOrdId, from, to, orderInfo.remainingQuantity, currentPrice); + await claimReplacement?.(); + + const newId = await this.editOrder( + clOrdId, + from, + to, + orderInfo.remainingQuantity, + currentPrice, + replacementClOrdId, + ); this.logger.verbose(`Order ${clOrdId} changed to ${newId}`); throw new TradeChangedException(newId); } catch (e) { if (e instanceof TradeChangedException) throw e; - // If edit fails, try to cancel and let it restart + // The amend is a write. Unless the venue explicitly rejected it, we do not know whether a + // replacement order now exists under `replacementClOrdId` — cancelling and carrying on would + // leave it live and untracked, which is how an amend turns into a duplicate position. + if (!isVenueRejection(e)) + throw new ScryptUnconfirmedWriteError( + `Scrypt gave no confirmed outcome for the amend of order ${clOrdId}: ${e.message}`, + replacementClOrdId, + ); + + // Rejected by the venue: nothing was created, so the cancel-and-restart fallback is safe. this.logger.verbose(`Could not update order ${clOrdId}, attempting cancel: ${e.message}`); + let cancelConfirmed = true; try { await this.cancelOrder(clOrdId, from, to); } catch (cancelError) { - this.logger.verbose(`Cancel also failed: ${cancelError.message}`); + // The cancel is a write too. Unconfirmed, it may well have taken effect at the venue while the + // cached report still shows the order open — and a non-terminal entry is never refreshed, so + // every later check would keep waiting on a picture that cannot change. Drop it instead and + // let the next lookup ask the venue. + cancelConfirmed = false; + this.forgetExecutionReport(clOrdId); + this.logger.warn(`Cancel of order ${clOrdId} went unconfirmed: ${cancelError.message}`); } + + // Surface the refusal so the caller can note the spent reference. Without that the next tick + // derives the very same one, the venue refuses it as a duplicate, and the pair loops. + throw new ScryptAmendRejectedError( + `Scrypt refused the amend of order ${clOrdId}: ${e.message}` + + (cancelConfirmed ? '' : ' (the follow-up cancel went unconfirmed)'), + replacementClOrdId, + ); } } else { this.logger.verbose(`Order ${clOrdId} open, price is still ${currentPrice}`); @@ -489,6 +594,10 @@ export class ScryptService extends PricingProvider { this.logger.verbose(`Order ${clOrdId} cancelled, restarting with remaining ${remaining} (base currency)`); + await claimReplacement?.(); + + // Same write boundary as the amend above: an unconfirmed restart may have created a live order under + // `replacementClOrdId`, so the caller has to quarantine rather than see a retryable transport error. const response = await this.placeOrder( symbol, side, @@ -496,7 +605,15 @@ export class ScryptService extends PricingProvider { ScryptOrderType.LIMIT, ScryptTimeInForce.GOOD_TILL_CANCEL, price, - ); + replacementClOrdId, + ).catch((e) => { + if (isVenueRejection(e)) throw e; + + throw new ScryptUnconfirmedWriteError( + `Scrypt gave no confirmed outcome for the restart of order ${clOrdId}: ${e.message}`, + replacementClOrdId, + ); + }); this.logger.verbose(`Order ${clOrdId} changed to ${response.id}`); throw new TradeChangedException(response.id); @@ -507,11 +624,18 @@ export class ScryptService extends PricingProvider { return true; case ScryptOrderStatus.REJECTED: - throw new Error(`Order ${clOrdId} has been rejected: ${orderInfo.rejectReason ?? 'unknown reason'}`); + throw new ScryptVenueRejectionError( + `Order ${clOrdId} has been rejected: ${orderInfo.rejectReason ?? 'unknown reason'}`, + ); case ScryptOrderStatus.PENDING_NEW: case ScryptOrderStatus.PENDING_CANCEL: case ScryptOrderStatus.PENDING_REPLACE: + // Deliberately just waits, however old the order is. A pending report is an OBSERVATION — we know + // where the order stands — so it is not an unknown outcome and must not be quarantined: reconciliation + // would find the reference, hand the order straight back, and the next completion check would + // quarantine it again. An order that stays pending too long is a stuck order, which the monitoring + // counter surfaces; it is not an unresolved one. this.logger.verbose(`Order ${clOrdId} is pending (${orderInfo.status}), waiting...`); return false; } @@ -535,8 +659,12 @@ export class ScryptService extends PricingProvider { orderType: ScryptOrderType = ScryptOrderType.LIMIT, timeInForce: ScryptTimeInForce = ScryptTimeInForce.GOOD_TILL_CANCEL, price?: number, + // Caller-supplied reference, persisted by the caller BEFORE this call. Without it the id only exists on + // the stack and is lost on timeout — leaving a possibly live venue order nobody can look up. Falls back + // to a fresh id so ad-hoc callers keep working; the venue requires daily uniqueness and <36 chars. + reservedClOrdId?: string, ): Promise { - const clOrdId = randomUUID(); + const clOrdId = reservedClOrdId ?? randomUUID(); // Price is required for LIMIT orders if (orderType === ScryptOrderType.LIMIT && price === undefined) { @@ -565,7 +693,9 @@ export class ScryptService extends PricingProvider { ); if (report.OrdStatus === ScryptOrderStatus.REJECTED) { - throw new Error(`Scrypt order rejected: ${report.Text ?? report.OrdRejReason ?? 'Unknown reason'}`); + throw new ScryptVenueRejectionError( + `Scrypt order rejected: ${report.Text ?? report.OrdRejReason ?? 'Unknown reason'}`, + ); } return { @@ -601,9 +731,13 @@ export class ScryptService extends PricingProvider { to: string, newQuantity: number, newPrice: number, + // See placeOrder. A cancel-replace creates a NEW venue order, so its reference needs the same + // reproducibility as the initial one — otherwise an amend that times out leaves a live order that + // nothing can look up. + reservedClOrdId?: string, ): Promise { const { symbol } = await this.getTradePair(from, to); - const newClOrdId = randomUUID(); + const newClOrdId = reservedClOrdId ?? randomUUID(); const editData = { OrigClOrdID: clOrdId, @@ -622,7 +756,9 @@ export class ScryptService extends PricingProvider { ); if (report.OrdStatus === ScryptOrderStatus.REJECTED) { - throw new Error(`Scrypt order edit rejected: ${report.Text ?? report.OrdRejReason ?? 'Unknown reason'}`); + throw new ScryptVenueRejectionError( + `Scrypt order edit rejected: ${report.Text ?? report.OrdRejReason ?? 'Unknown reason'}`, + ); } return newClOrdId; diff --git a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts new file mode 100644 index 0000000000..27131b413a --- /dev/null +++ b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts @@ -0,0 +1,992 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; +import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; +import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; +import { EntityManager, FindManyOptions, FindOneOptions } from 'typeorm'; +import { CustodyAccountAccess } from '../../entities/custody-account-access.entity'; +import { CustodyAccount } from '../../entities/custody-account.entity'; +import { CustodyAccessLevel, CustodyAccountStatus } from '../../enums/custody'; +import { CustodyAccountAccessRepository } from '../../repositories/custody-account-access.repository'; +import { CustodyAccountRepository } from '../../repositories/custody-account.repository'; +import { CustodyAccountService } from '../custody-account.service'; + +describe('CustodyAccountService', () => { + let service: CustodyAccountService; + let custodyAccountRepo: DeepMocked; + let custodyAccountAccessRepo: DeepMocked; + let userDataService: DeepMocked; + + const ownerId = 100; + const strangerId = 200; + const ownAccountId = 1; + const foreignAccountId = 2; + + function ownerUserData(overrides: Partial = {}): UserData { + return Object.assign(new UserData(), { id: ownerId, users: [], custodyAccounts: [], ...overrides }); + } + + function strangerUserData(overrides: Partial = {}): UserData { + return Object.assign(new UserData(), { id: strangerId, users: [], custodyAccounts: [], ...overrides }); + } + + function ownCustodyAccount(overrides: Partial = {}): CustodyAccount { + return Object.assign(new CustodyAccount(), { + id: ownAccountId, + title: 'Own Safe', + description: 'Owner account', + owner: ownerUserData(), + requiredSignatures: 1, + status: CustodyAccountStatus.ACTIVE, + accessGrants: [], + ...overrides, + }); + } + + function foreignCustodyAccount(overrides: Partial = {}): CustodyAccount { + return Object.assign(new CustodyAccount(), { + id: foreignAccountId, + title: 'Foreign Safe', + description: 'Shared account', + owner: strangerUserData(), + requiredSignatures: 1, + status: CustodyAccountStatus.ACTIVE, + accessGrants: [], + ...overrides, + }); + } + + function accessGrant(params: { + id?: number; + account: CustodyAccount; + userData: UserData; + accessLevel: CustodyAccessLevel; + active: boolean; + }): CustodyAccountAccess { + return Object.assign(new CustodyAccountAccess(), { + id: params.id ?? 10, + account: params.account, + userData: params.userData, + accessLevel: params.accessLevel, + active: params.active, + }); + } + + /** + * Mirrors the repository where clause for requireOwner: `{ id }`. + * Status is deliberately absent there, so a non-matching status returns null if that filter + * is accidentally reintroduced. + */ + function mockFindOneAccountForOwnerCheck(account: CustodyAccount | undefined): void { + custodyAccountRepo.findOne.mockImplementation( + async (options: FindOneOptions): Promise => { + const where = options.where as { + id?: number; + status?: CustodyAccountStatus; + }; + + if (!account) { + return null; + } + + if (where.id !== undefined && account.id !== where.id) { + return null; + } + + if (where.status !== undefined && account.status !== where.status) { + return null; + } + + return account; + }, + ); + } + + /** + * Mirrors the repository where clause for checkAccess: + * `{ account: { id }, userData: { id }, active: true }`. + * Inactive grants resolve to undefined, matching Postgres behaviour. + */ + function mockFindOneActiveGrant(grant: CustodyAccountAccess | undefined): void { + custodyAccountAccessRepo.findOne.mockImplementation( + async (options: FindOneOptions): Promise => { + const where = options.where as { + account?: { id?: number }; + userData?: { id?: number }; + active?: boolean; + }; + + if (!grant) { + return null; + } + + if (where.active === true && !grant.active) { + return null; + } + + if (where.account?.id !== undefined && grant.account.id !== where.account.id) { + return null; + } + + if (where.userData?.id !== undefined && grant.userData.id !== where.userData.id) { + return null; + } + + return grant; + }, + ); + } + + /** + * Mirrors the repository where clause for getCustodyAccountsForUser: + * `{ userData: { id }, active: true, account: { status: ACTIVE } }`. + * Only grants that would pass the SQL filter are returned. + */ + function mockFindActiveGrants(grants: CustodyAccountAccess[]): void { + custodyAccountAccessRepo.find.mockImplementation( + async (options: FindManyOptions): Promise => { + const where = options.where as { + userData?: { id?: number }; + active?: boolean; + account?: { status?: CustodyAccountStatus }; + }; + + return grants.filter((grant) => { + if (where.active === true && !grant.active) { + return false; + } + if (where.userData?.id !== undefined && grant.userData.id !== where.userData.id) { + return false; + } + if (where.account?.status !== undefined && grant.account.status !== where.account.status) { + return false; + } + return true; + }); + }, + ); + } + + /** + * Mirrors the repository where clause for requireActingAllowed: + * `{ userData: { id }, account: { owner: { id } }, accessLevel: READ, active: true }`. + * Account status is deliberately absent there — blocking must not shed a narrowing — so the + * status branch below never fires for that query; it stays to keep the mock honest if the + * clause ever changes. + */ + function mockFindOneActingGrant(grant: CustodyAccountAccess | undefined): void { + custodyAccountAccessRepo.findOne.mockImplementation( + async (options: FindOneOptions): Promise => { + const where = options.where as { + userData?: { id?: number }; + account?: { owner?: { id?: number }; status?: CustodyAccountStatus }; + accessLevel?: CustodyAccessLevel; + active?: boolean; + }; + + if (!grant) { + return null; + } + + if (where.userData?.id !== undefined && grant.userData.id !== where.userData.id) { + return null; + } + + if (where.account?.owner?.id !== undefined && grant.account.owner.id !== where.account.owner.id) { + return null; + } + + if (where.account?.status !== undefined && grant.account.status !== where.account.status) { + return null; + } + + if (where.accessLevel !== undefined && grant.accessLevel !== where.accessLevel) { + return null; + } + + if (where.active === true && !grant.active) { + return null; + } + + return grant; + }, + ); + } + + beforeEach(() => { + custodyAccountRepo = createMock(); + custodyAccountAccessRepo = createMock(); + userDataService = createMock(); + + service = new CustodyAccountService(custodyAccountRepo, custodyAccountAccessRepo, userDataService); + }); + + describe('checkAccess', () => { + it('allows the owner to write when there is no grant on their own account', async () => { + const account = ownCustodyAccount(); + custodyAccountRepo.findOne.mockResolvedValue(account); + mockFindOneActiveGrant(undefined); + + await expect(service.checkAccess(ownAccountId, ownerId, CustodyAccessLevel.WRITE)).resolves.toEqual({ + custodyAccount: account, + isLegacy: false, + }); + }); + + it('rejects write when the owner has an active read grant on their own account', async () => { + const account = ownCustodyAccount(); + const grant = accessGrant({ + account, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + custodyAccountRepo.findOne.mockResolvedValue(account); + mockFindOneActiveGrant(grant); + + await expect(service.checkAccess(ownAccountId, ownerId, CustodyAccessLevel.WRITE)).rejects.toThrow( + ForbiddenException, + ); + }); + + it('allows read when the owner has an active read grant on their own account', async () => { + const account = ownCustodyAccount(); + const grant = accessGrant({ + account, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + custodyAccountRepo.findOne.mockResolvedValue(account); + mockFindOneActiveGrant(grant); + + await expect(service.checkAccess(ownAccountId, ownerId, CustodyAccessLevel.READ)).resolves.toEqual({ + custodyAccount: account, + isLegacy: false, + }); + }); + + it('allows write when the owner has an active write grant on their own account', async () => { + const account = ownCustodyAccount(); + const grant = accessGrant({ + account, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + custodyAccountRepo.findOne.mockResolvedValue(account); + mockFindOneActiveGrant(grant); + + await expect(service.checkAccess(ownAccountId, ownerId, CustodyAccessLevel.WRITE)).resolves.toEqual({ + custodyAccount: account, + isLegacy: false, + }); + }); + + it('rejects access when a stranger has no grant', async () => { + const account = ownCustodyAccount(); + custodyAccountRepo.findOne.mockResolvedValue(account); + mockFindOneActiveGrant(undefined); + + await expect(service.checkAccess(ownAccountId, strangerId, CustodyAccessLevel.READ)).rejects.toThrow( + ForbiddenException, + ); + }); + + it('allows read but rejects write when a stranger has a read grant', async () => { + const account = ownCustodyAccount(); + const grant = accessGrant({ + account, + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + custodyAccountRepo.findOne.mockResolvedValue(account); + mockFindOneActiveGrant(grant); + + await expect(service.checkAccess(ownAccountId, strangerId, CustodyAccessLevel.READ)).resolves.toEqual({ + custodyAccount: account, + isLegacy: false, + }); + + await expect(service.checkAccess(ownAccountId, strangerId, CustodyAccessLevel.WRITE)).rejects.toThrow( + ForbiddenException, + ); + }); + + it('does not narrow the owner when their grant on the own account is inactive', async () => { + const account = ownCustodyAccount(); + const inactiveGrant = accessGrant({ + account, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: false, + }); + custodyAccountRepo.findOne.mockResolvedValue(account); + // Mock honours active: true — inactive grant must resolve as no grant + mockFindOneActiveGrant(inactiveGrant); + + await expect(service.checkAccess(ownAccountId, ownerId, CustodyAccessLevel.WRITE)).resolves.toEqual({ + custodyAccount: account, + isLegacy: false, + }); + }); + }); + + describe('getCustodyAccountsForUser', () => { + it('lists an own account without a grant as write', async () => { + const account = ownCustodyAccount(); + userDataService.getUserData.mockResolvedValue(ownerUserData({ custodyAccounts: [account] })); + mockFindActiveGrants([]); + + const result = await service.getCustodyAccountsForUser(ownerId); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual( + expect.objectContaining({ + id: ownAccountId, + accessLevel: CustodyAccessLevel.WRITE, + isLegacy: false, + }), + ); + }); + + it('lists an own account with an active read grant as read', async () => { + const account = ownCustodyAccount(); + const grant = accessGrant({ + account, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + userDataService.getUserData.mockResolvedValue(ownerUserData({ custodyAccounts: [account] })); + mockFindActiveGrants([grant]); + + const result = await service.getCustodyAccountsForUser(ownerId); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual( + expect.objectContaining({ + id: ownAccountId, + accessLevel: CustodyAccessLevel.READ, + isLegacy: false, + }), + ); + }); + + it('includes a shared foreign account once and does not duplicate the own account', async () => { + const ownAccount = ownCustodyAccount(); + const foreignAccount = foreignCustodyAccount(); + const ownReadGrant = accessGrant({ + id: 10, + account: ownAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + const sharedReadGrant = accessGrant({ + id: 11, + account: foreignAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + + userDataService.getUserData.mockResolvedValue(ownerUserData({ custodyAccounts: [ownAccount] })); + // Both grants are active and on ACTIVE accounts — mock returns exactly what SQL would + mockFindActiveGrants([ownReadGrant, sharedReadGrant]); + + const result = await service.getCustodyAccountsForUser(ownerId); + + expect(result).toHaveLength(2); + + const ownEntry = result.find((dto) => dto.id === ownAccountId); + const sharedEntry = result.find((dto) => dto.id === foreignAccountId); + + expect(ownEntry).toEqual( + expect.objectContaining({ + id: ownAccountId, + accessLevel: CustodyAccessLevel.READ, + isLegacy: false, + }), + ); + expect(sharedEntry).toEqual( + expect.objectContaining({ + id: foreignAccountId, + accessLevel: CustodyAccessLevel.READ, + isLegacy: false, + }), + ); + + // Own account must not appear a second time as "shared" + const ownOccurrences = result.filter((dto) => dto.id === ownAccountId); + expect(ownOccurrences).toHaveLength(1); + }); + + it("filters out inactive grants, another user's grants, and grants on non-active accounts", async () => { + const account = ownCustodyAccount(); + const legitimateGrant = accessGrant({ + id: 10, + account, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + + // Noise: inactive grant for the caller on an otherwise qualifying foreign account + const inactiveNoiseAccount = foreignCustodyAccount({ id: 3 }); + const inactiveGrant = accessGrant({ + id: 11, + account: inactiveNoiseAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: false, + }); + + // Noise: active grant belonging to a different userData + const strangerGrantAccount = foreignCustodyAccount({ id: 4 }); + const strangerGrant = accessGrant({ + id: 12, + account: strangerGrantAccount, + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + + // Noise: active grant for the caller on a non-ACTIVE account + const blockedAccount = foreignCustodyAccount({ id: 5, status: CustodyAccountStatus.BLOCKED }); + const blockedGrant = accessGrant({ + id: 13, + account: blockedAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + + userDataService.getUserData.mockResolvedValue(ownerUserData({ custodyAccounts: [account] })); + mockFindActiveGrants([legitimateGrant, inactiveGrant, strangerGrant, blockedGrant]); + + const result = await service.getCustodyAccountsForUser(ownerId); + + expect(result).toHaveLength(1); + expect(result.map((dto) => dto.id).sort()).toEqual([ownAccountId]); + expect(result[0]).toEqual( + expect.objectContaining({ + id: ownAccountId, + accessLevel: CustodyAccessLevel.READ, + isLegacy: false, + }), + ); + }); + }); + + describe('getAccessList', () => { + it('lets the owner inspect active grants on their blocked account', async () => { + const blockedOwnAccount = ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }); + const grant = accessGrant({ + account: blockedOwnAccount, + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + mockFindOneAccountForOwnerCheck(blockedOwnAccount); + custodyAccountAccessRepo.find.mockResolvedValue([grant]); + + await expect(service.getAccessList(ownAccountId, ownerId)).resolves.toEqual([grant]); + expect(custodyAccountAccessRepo.find).toHaveBeenCalledWith({ + where: { account: { id: ownAccountId }, active: true }, + relations: { userData: true }, + }); + }); + + it('rejects a non-owner', async () => { + mockFindOneAccountForOwnerCheck(foreignCustodyAccount()); + + await expect(service.getAccessList(foreignAccountId, ownerId)).rejects.toThrow( + new ForbiddenException('Only the account owner can manage access grants'), + ); + expect(custodyAccountAccessRepo.find).not.toHaveBeenCalled(); + }); + }); + + describe('grantAccess', () => { + const mail = 'stranger@example.com'; + + let txManager: { + findOne: jest.Mock; + create: jest.Mock; + save: jest.Mock; + }; + + beforeEach(() => { + txManager = { + findOne: jest.fn().mockResolvedValue(null), + create: jest.fn( + (_entityClass: unknown, plain: Partial): CustodyAccountAccess => + Object.assign(new CustodyAccountAccess(), plain), + ), + save: jest.fn(async (entity: CustodyAccountAccess): Promise => entity), + }; + + Object.defineProperty(custodyAccountAccessRepo, 'manager', { + value: txManager as unknown as EntityManager, + configurable: true, + }); + }); + + it('creates a grant for a foreign e-mail address on an active own account', async () => { + const account = ownCustodyAccount(); + const target = strangerUserData(); + mockFindOneAccountForOwnerCheck(account); + userDataService.getUsersByMail.mockResolvedValue([target]); + + const result = await service.grantAccess(ownAccountId, ownerId, mail, CustodyAccessLevel.READ); + + expect(userDataService.getUsersByMail).toHaveBeenCalledWith(mail, true, {}); + expect(txManager.findOne).toHaveBeenCalledWith(CustodyAccountAccess, { + where: { account: { id: ownAccountId }, userData: { id: strangerId }, active: true }, + }); + expect(txManager.create).toHaveBeenCalledWith(CustodyAccountAccess, { + account, + userData: target, + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + expect(result).toEqual( + expect.objectContaining({ + account, + userData: target, + accessLevel: CustodyAccessLevel.READ, + active: true, + }), + ); + }); + + it('rejects granting on a blocked own account before resolving the e-mail address', async () => { + const blockedOwnAccount = ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }); + mockFindOneAccountForOwnerCheck(blockedOwnAccount); + + await expect(service.grantAccess(ownAccountId, ownerId, mail, CustodyAccessLevel.READ)).rejects.toThrow( + new BadRequestException('Cannot grant access on an account that is not active'), + ); + expect(userDataService.getUsersByMail).not.toHaveBeenCalled(); + expect(txManager.findOne).not.toHaveBeenCalled(); + }); + + it('rejects granting write on a blocked own account just the same', async () => { + // The refusal must not depend on the level asked for: inspection is no more grantable + // during a hold than acting is. + const blockedOwnAccount = ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }); + mockFindOneAccountForOwnerCheck(blockedOwnAccount); + + await expect(service.grantAccess(ownAccountId, ownerId, mail, CustodyAccessLevel.WRITE)).rejects.toThrow( + new BadRequestException('Cannot grant access on an account that is not active'), + ); + expect(userDataService.getUsersByMail).not.toHaveBeenCalled(); + }); + + it('rejects a non-owner', async () => { + mockFindOneAccountForOwnerCheck(foreignCustodyAccount()); + + await expect(service.grantAccess(foreignAccountId, ownerId, mail, CustodyAccessLevel.READ)).rejects.toThrow( + new ForbiddenException('Only the account owner can manage access grants'), + ); + expect(userDataService.getUsersByMail).not.toHaveBeenCalled(); + expect(txManager.findOne).not.toHaveBeenCalled(); + }); + + it("rejects granting access to the caller's own e-mail address", async () => { + const account = ownCustodyAccount(); + mockFindOneAccountForOwnerCheck(account); + userDataService.getUsersByMail.mockResolvedValue([ownerUserData()]); + + await expect(service.grantAccess(ownAccountId, ownerId, mail, CustodyAccessLevel.WRITE)).rejects.toThrow( + new BadRequestException('Cannot grant access to yourself'), + ); + expect(txManager.findOne).not.toHaveBeenCalled(); + }); + }); + + describe('updateAccess', () => { + const accessId = 10; + + let accessQuery: { + innerJoinAndSelect: jest.Mock; + where: jest.Mock; + andWhere: jest.Mock; + setLock: jest.Mock; + getOne: jest.Mock; + }; + let txManager: { + createQueryBuilder: jest.Mock; + update: jest.Mock; + create: jest.Mock; + save: jest.Mock; + }; + + beforeEach(() => { + accessQuery = { + innerJoinAndSelect: jest.fn(), + where: jest.fn(), + andWhere: jest.fn(), + setLock: jest.fn(), + getOne: jest.fn(), + }; + for (const method of ['innerJoinAndSelect', 'where', 'andWhere', 'setLock'] as const) { + accessQuery[method].mockReturnValue(accessQuery); + } + + txManager = { + createQueryBuilder: jest.fn().mockReturnValue(accessQuery), + update: jest.fn(), + create: jest.fn( + (_entityClass: unknown, plain: Partial): CustodyAccountAccess => + Object.assign(new CustodyAccountAccess(), plain), + ), + save: jest.fn(async (entity: CustodyAccountAccess): Promise => entity), + }; + + Object.defineProperty(custodyAccountAccessRepo, 'manager', { + value: { + transaction: jest.fn((cb: (manager: EntityManager) => Promise) => + cb(txManager as unknown as EntityManager), + ), + }, + configurable: true, + }); + + custodyAccountRepo.findOne.mockResolvedValue(ownCustodyAccount()); + }); + + it("narrows the owner's own grant from write to read", async () => { + const lockedGrant = accessGrant({ + id: accessId, + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + const result = await service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.READ); + + expect(custodyAccountRepo.findOne).toHaveBeenCalled(); + expect(txManager.update).toHaveBeenCalledTimes(1); + expect(txManager.create).toHaveBeenCalledTimes(1); + expect(txManager.create).toHaveBeenCalledWith( + CustodyAccountAccess, + expect.objectContaining({ + accessLevel: CustodyAccessLevel.READ, + active: true, + }), + ); + expect(txManager.save).toHaveBeenCalledTimes(1); + expect(txManager.save).toHaveBeenCalledWith( + expect.objectContaining({ + accessLevel: CustodyAccessLevel.READ, + active: true, + }), + ); + expect(result.accessLevel).toBe(CustodyAccessLevel.READ); + }); + + it('lets the owner lift a narrowing on a blocked account so one block cannot freeze their Safe', async () => { + // A narrowing blocks the owner's whole Safe (requireActingAllowed) and does not care about + // account status. If grant management required an ACTIVE account, blocking one account + // would freeze every other one of theirs with no way back. + const blockedOwnAccount = ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }); + mockFindOneAccountForOwnerCheck(blockedOwnAccount); + + const lockedGrant = accessGrant({ + id: accessId, + account: blockedOwnAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + const result = await service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.WRITE); + + expect(result.accessLevel).toBe(CustodyAccessLevel.WRITE); + expect(txManager.save).toHaveBeenCalledWith( + expect.objectContaining({ accessLevel: CustodyAccessLevel.WRITE, active: true }), + ); + }); + + it("restores the owner's own grant from read back to write", async () => { + const lockedGrant = accessGrant({ + id: accessId, + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + const result = await service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.WRITE); + + expect(txManager.update).toHaveBeenCalledTimes(1); + expect(txManager.create).toHaveBeenCalledTimes(1); + expect(txManager.save).toHaveBeenCalledTimes(1); + expect(result.accessLevel).toBe(CustodyAccessLevel.WRITE); + }); + + it('does nothing when the requested level already matches (short-circuit)', async () => { + const lockedGrant = accessGrant({ + id: accessId, + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + const result = await service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.READ); + + expect(result).toBe(lockedGrant); + expect(txManager.update).not.toHaveBeenCalled(); + expect(txManager.create).not.toHaveBeenCalled(); + expect(txManager.save).not.toHaveBeenCalled(); + }); + + it('rejects updateAccess from a non-owner', async () => { + await expect(service.updateAccess(ownAccountId, accessId, strangerId, CustodyAccessLevel.READ)).rejects.toThrow( + ForbiddenException, + ); + + expect(txManager.createQueryBuilder).not.toHaveBeenCalled(); + }); + + it("rejects raising a stranger's access on a blocked account and writes nothing", async () => { + const blockedOwnAccount = ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }); + mockFindOneAccountForOwnerCheck(blockedOwnAccount); + + const lockedGrant = accessGrant({ + id: accessId, + account: blockedOwnAccount, + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + await expect(service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.WRITE)).rejects.toThrow( + new BadRequestException('Cannot raise access on an account that is not active'), + ); + expect(txManager.update).not.toHaveBeenCalled(); + expect(txManager.create).not.toHaveBeenCalled(); + expect(txManager.save).not.toHaveBeenCalled(); + }); + + it("lets a stranger's access be lowered on a blocked account", async () => { + const blockedOwnAccount = ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }); + mockFindOneAccountForOwnerCheck(blockedOwnAccount); + + const lockedGrant = accessGrant({ + id: accessId, + account: blockedOwnAccount, + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + const result = await service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.READ); + + expect(result.accessLevel).toBe(CustodyAccessLevel.READ); + expect(txManager.update).toHaveBeenCalledTimes(1); + expect(txManager.create).toHaveBeenCalledTimes(1); + expect(txManager.create).toHaveBeenCalledWith( + CustodyAccountAccess, + expect.objectContaining({ + accessLevel: CustodyAccessLevel.READ, + active: true, + }), + ); + expect(txManager.save).toHaveBeenCalledTimes(1); + }); + + it("still lets a stranger's access be raised on an active account", async () => { + const activeOwnAccount = ownCustodyAccount(); + mockFindOneAccountForOwnerCheck(activeOwnAccount); + + const lockedGrant = accessGrant({ + id: accessId, + account: activeOwnAccount, + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + const result = await service.updateAccess(ownAccountId, accessId, ownerId, CustodyAccessLevel.WRITE); + + expect(result.accessLevel).toBe(CustodyAccessLevel.WRITE); + expect(txManager.update).toHaveBeenCalledTimes(1); + expect(txManager.create).toHaveBeenCalledTimes(1); + expect(txManager.save).toHaveBeenCalledTimes(1); + }); + }); + + describe('revokeAccess', () => { + const accessId = 10; + + let accessQuery: { + innerJoinAndSelect: jest.Mock; + where: jest.Mock; + andWhere: jest.Mock; + setLock: jest.Mock; + getOne: jest.Mock; + }; + let txManager: { + createQueryBuilder: jest.Mock; + update: jest.Mock; + create: jest.Mock; + save: jest.Mock; + }; + + beforeEach(() => { + accessQuery = { + innerJoinAndSelect: jest.fn(), + where: jest.fn(), + andWhere: jest.fn(), + setLock: jest.fn(), + getOne: jest.fn(), + }; + for (const method of ['innerJoinAndSelect', 'where', 'andWhere', 'setLock'] as const) { + accessQuery[method].mockReturnValue(accessQuery); + } + + txManager = { + createQueryBuilder: jest.fn().mockReturnValue(accessQuery), + update: jest.fn(), + create: jest.fn( + (_entityClass: unknown, plain: Partial): CustodyAccountAccess => + Object.assign(new CustodyAccountAccess(), plain), + ), + save: jest.fn(async (entity: CustodyAccountAccess): Promise => entity), + }; + + Object.defineProperty(custodyAccountAccessRepo, 'manager', { + value: { + transaction: jest.fn((cb: (manager: EntityManager) => Promise) => + cb(txManager as unknown as EntityManager), + ), + }, + configurable: true, + }); + + custodyAccountRepo.findOne.mockResolvedValue(ownCustodyAccount()); + }); + + it("rejects revoking the owner's own access grant", async () => { + const lockedGrant = accessGrant({ + id: accessId, + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + await expect(service.revokeAccess(ownAccountId, accessId, ownerId)).rejects.toThrow(BadRequestException); + expect(txManager.update).not.toHaveBeenCalled(); + }); + + it("revokes a foreign grantee's access", async () => { + const lockedGrant = accessGrant({ + id: accessId, + account: ownCustodyAccount(), + userData: strangerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + accessQuery.getOne.mockResolvedValue(lockedGrant); + + await expect(service.revokeAccess(ownAccountId, accessId, ownerId)).resolves.toBeUndefined(); + expect(txManager.update).toHaveBeenCalledTimes(1); + expect(txManager.update).toHaveBeenCalledWith( + CustodyAccountAccess, + accessId, + expect.objectContaining({ active: false }), + ); + }); + + it('rejects revokeAccess from a non-owner', async () => { + await expect(service.revokeAccess(ownAccountId, accessId, strangerId)).rejects.toThrow(ForbiddenException); + expect(txManager.createQueryBuilder).not.toHaveBeenCalled(); + }); + }); + + describe('requireActingAllowed', () => { + it('allows acting when no grant is configured', async () => { + mockFindOneActingGrant(undefined); + + await expect(service.requireActingAllowed(ownerId)).resolves.toBeUndefined(); + }); + + it('rejects acting when the owner has an active read grant on their own account', async () => { + const grant = accessGrant({ + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + mockFindOneActingGrant(grant); + + await expect(service.requireActingAllowed(ownerId)).rejects.toThrow( + new ForbiddenException('This Safe is limited to inspection, acting is not permitted'), + ); + }); + + it('allows acting when the owner has an active write grant on their own account', async () => { + const grant = accessGrant({ + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + mockFindOneActingGrant(grant); + + await expect(service.requireActingAllowed(ownerId)).resolves.toBeUndefined(); + }); + + it('allows acting on the own safe when the owner holds a read grant only on a foreign account', async () => { + const grant = accessGrant({ + account: foreignCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + mockFindOneActingGrant(grant); + + await expect(service.requireActingAllowed(ownerId)).resolves.toBeUndefined(); + }); + + it('allows acting when the owner read grant on their own account is inactive', async () => { + const grant = accessGrant({ + account: ownCustodyAccount(), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: false, + }); + mockFindOneActingGrant(grant); + + await expect(service.requireActingAllowed(ownerId)).resolves.toBeUndefined(); + }); + + it('still rejects acting when the narrowed own account is blocked', async () => { + // Blocking an account must not be a way to shed the restriction: elsewhere a non-active + // account grants nothing, here its absence would grant the right to act. + const grant = accessGrant({ + account: ownCustodyAccount({ status: CustodyAccountStatus.BLOCKED }), + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + mockFindOneActingGrant(grant); + + await expect(service.requireActingAllowed(ownerId)).rejects.toThrow(ForbiddenException); + }); + }); +}); diff --git a/src/subdomains/core/custody/services/__tests__/custody-order.service.spec.ts b/src/subdomains/core/custody/services/__tests__/custody-order.service.spec.ts new file mode 100644 index 0000000000..355bc98793 --- /dev/null +++ b/src/subdomains/core/custody/services/__tests__/custody-order.service.spec.ts @@ -0,0 +1,285 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { ForbiddenException } from '@nestjs/common'; +import { Blockchain } from 'src/integration/blockchain/shared/enums/blockchain.enum'; +import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; +import { UserRole } from 'src/shared/auth/user-role.enum'; +import { Asset } from 'src/shared/models/asset/asset.entity'; +import { AssetService } from 'src/shared/models/asset/asset.service'; +import { AssetDto } from 'src/shared/models/asset/dto/asset.dto'; +import { FiatService } from 'src/shared/models/fiat/fiat.service'; +import { BuyService } from 'src/subdomains/core/buy-crypto/routes/buy/buy.service'; +import { SwapPaymentInfoDto } from 'src/subdomains/core/buy-crypto/routes/swap/dto/swap-payment-info.dto'; +import { Swap } from 'src/subdomains/core/buy-crypto/routes/swap/swap.entity'; +import { SwapService } from 'src/subdomains/core/buy-crypto/routes/swap/swap.service'; +import { SellService } from 'src/subdomains/core/sell-crypto/route/sell.service'; +import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; +import { User } from 'src/subdomains/generic/user/models/user/user.entity'; +import { UserService } from 'src/subdomains/generic/user/models/user/user.service'; +import { FeeDto } from 'src/subdomains/supporting/payment/dto/fee.dto'; +import { FiatPaymentMethod } from 'src/subdomains/supporting/payment/dto/payment-method.enum'; +import { MinAmount } from 'src/subdomains/supporting/payment/dto/transaction-helper/min-amount.dto'; +import { GetCustodyInfoDto } from '../../dto/input/get-custody-info.dto'; +import { CustodyOrder } from '../../entities/custody-order.entity'; +import { CustodyOrderStatus, CustodyOrderType } from '../../enums/custody'; +import { CustodyOrderStepRepository } from '../../repositories/custody-order-step.repository'; +import { CustodyOrderRepository } from '../../repositories/custody-order.repository'; +import { CustodyAccountService } from '../custody-account.service'; +import { CustodyOrderService } from '../custody-order.service'; +import { CustodyService } from '../custody.service'; +import { EquityPairService } from '../equity-pair.service'; + +describe('CustodyOrderService', () => { + let service: CustodyOrderService; + let userService: DeepMocked; + let custodyOrderRepo: DeepMocked; + let custodyOrderStepRepo: DeepMocked; + let custodyService: DeepMocked; + let custodyAccountService: DeepMocked; + let sellService: DeepMocked; + let buyService: DeepMocked; + let swapService: DeepMocked; + let assetService: DeepMocked; + let fiatService: DeepMocked; + let equityPairService: DeepMocked; + + const walletUserId = 10; + const userDataId = 100; + const jwtAccountId = 999; + const orderId = 50; + const swapRouteId = 77; + const paymentInfoId = 88; + + function custodyUser(overrides: Partial = {}): User { + return Object.assign(new User(), { + id: walletUserId, + userData: Object.assign(new UserData(), { id: userDataId }), + custodyBalances: [], + ...overrides, + }); + } + + function jwtPayload(overrides: Partial = {}): JwtPayload { + return { + user: walletUserId, + account: jwtAccountId, + role: UserRole.USER, + ip: '127.0.0.1', + ...overrides, + }; + } + + function receiveOrderDto(overrides: Partial = {}): GetCustodyInfoDto { + return Object.assign(new GetCustodyInfoDto(), { + type: CustodyOrderType.RECEIVE, + sourceAsset: 'ETH', + targetAsset: 'ZCHF', + sourceAmount: 1, + paymentMethod: FiatPaymentMethod.BANK, + ...overrides, + }); + } + + function custodyAsset(name: string, id: number): Asset { + return Object.assign(new Asset(), { + id, + name, + blockchain: Blockchain.ETHEREUM, + }); + } + + function zeroFee(): FeeDto { + return { + min: 0, + rate: 0, + fixed: 0, + dfx: 0, + network: 0, + platform: 0, + bank: 0, + total: 0, + }; + } + + function assetDto(name: string): AssetDto { + return { name } as AssetDto; + } + + function swapPaymentInfo(overrides: Partial = {}): SwapPaymentInfoDto { + const minDeposit: MinAmount = { amount: 0, asset: 'ETH' }; + + return { + id: paymentInfoId, + uid: 'swap-uid', + timestamp: new Date('2024-01-01T00:00:00.000Z'), + routeId: swapRouteId, + depositAddress: '0xdeposit', + blockchain: Blockchain.ETHEREUM, + minDeposit, + fee: 0, + minFee: 0, + fees: zeroFee(), + minVolume: 0, + maxVolume: 0, + amount: 1, + sourceAsset: assetDto('ETH'), + minFeeTarget: 0, + feesTarget: zeroFee(), + minVolumeTarget: 0, + maxVolumeTarget: 0, + exchangeRate: 1, + rate: 1, + exactPrice: true, + priceSteps: [], + estimatedAmount: 1, + targetAsset: assetDto('ZCHF'), + paymentRequest: undefined, + isValid: true, + error: undefined, + ...overrides, + }; + } + + function custodyOrder(overrides: Partial = {}): CustodyOrder { + return Object.assign(new CustodyOrder(), { + id: orderId, + type: CustodyOrderType.RECEIVE, + status: CustodyOrderStatus.CREATED, + user: custodyUser(), + ...overrides, + }); + } + + function mockReceiveHappyPath(user: User): void { + userService.getUser.mockResolvedValue(user); + custodyAccountService.requireActingAllowed.mockResolvedValue(undefined); + + const sourceAsset = custodyAsset('ETH', 1); + const targetAsset = custodyAsset('ZCHF', 2); + assetService.getAssetsByName.mockImplementation(async (name: string): Promise => { + if (name === 'ETH') return [sourceAsset]; + if (name === 'ZCHF') return [targetAsset]; + return []; + }); + + const paymentInfo = swapPaymentInfo(); + swapService.createSwapPaymentInfo.mockResolvedValue(paymentInfo); + swapService.getById.mockResolvedValue(Object.assign(new Swap(), { id: swapRouteId })); + + custodyOrderRepo.create.mockImplementation((dto: object) => + Object.assign(new CustodyOrder(), { status: CustodyOrderStatus.CREATED }, dto), + ); + custodyOrderRepo.save.mockImplementation(async (order: CustodyOrder) => + Object.assign(order, { + id: orderId, + status: order.status, + type: order.type, + }), + ); + } + + beforeEach(() => { + userService = createMock(); + custodyOrderRepo = createMock(); + custodyOrderStepRepo = createMock(); + custodyService = createMock(); + custodyAccountService = createMock(); + sellService = createMock(); + buyService = createMock(); + swapService = createMock(); + assetService = createMock(); + fiatService = createMock(); + equityPairService = createMock(); + + service = new CustodyOrderService( + userService, + custodyOrderRepo, + custodyOrderStepRepo, + custodyService, + custodyAccountService, + sellService, + buyService, + swapService, + assetService, + fiatService, + equityPairService, + ); + }); + + describe('createOrder', () => { + it('rejects createOrder when acting is narrowed to inspection', async () => { + const user = custodyUser(); + userService.getUser.mockResolvedValue(user); + custodyAccountService.requireActingAllowed.mockRejectedValueOnce( + new ForbiddenException('This Safe is limited to inspection, acting is not permitted'), + ); + + await expect(service.createOrder(jwtPayload(), receiveOrderDto())).rejects.toThrow(ForbiddenException); + + expect(custodyOrderRepo.save).not.toHaveBeenCalled(); + expect(custodyOrderRepo.create).not.toHaveBeenCalled(); + expect(swapService.createSwapPaymentInfo).not.toHaveBeenCalled(); + }); + + it("calls requireActingAllowed with the loaded user's userData id, not the jwt ids", async () => { + const user = custodyUser(); + const jwt = jwtPayload({ user: walletUserId, account: jwtAccountId }); + + expect(jwt.user).not.toBe(user.userData.id); + expect(jwt.account).not.toBe(user.userData.id); + expect(jwt.user).not.toBe(jwt.account); + + mockReceiveHappyPath(user); + + const result = await service.createOrder(jwt, receiveOrderDto()); + + expect(custodyAccountService.requireActingAllowed).toHaveBeenCalledWith(user.userData.id); + expect(custodyAccountService.requireActingAllowed).not.toHaveBeenCalledWith(jwt.user); + expect(custodyAccountService.requireActingAllowed).not.toHaveBeenCalledWith(jwt.account); + expect(result.orderId).toBe(orderId); + expect(result.type).toBe(CustodyOrderType.RECEIVE); + }); + }); + + describe('confirmOrder', () => { + it('rejects confirmOrder when acting is narrowed to inspection and does not update the order', async () => { + const order = custodyOrder(); + custodyOrderRepo.findOne.mockResolvedValue(order); + custodyAccountService.requireActingAllowed.mockRejectedValueOnce( + new ForbiddenException('This Safe is limited to inspection, acting is not permitted'), + ); + + await expect(service.confirmOrder(walletUserId, orderId)).rejects.toThrow(ForbiddenException); + + expect(custodyOrderRepo.update).not.toHaveBeenCalled(); + }); + + it('rejects a stranger before requireActingAllowed so ownership short-circuits the narrowing check', async () => { + const order = custodyOrder(); + const strangerId = 9999; + expect(strangerId).not.toBe(order.user.id); + + custodyOrderRepo.findOne.mockResolvedValue(order); + + await expect(service.confirmOrder(strangerId, orderId)).rejects.toThrow( + new ForbiddenException('Order is not from current user'), + ); + + expect(custodyAccountService.requireActingAllowed).not.toHaveBeenCalled(); + }); + + it('calls requireActingAllowed with order.user.userData.id and confirms when acting is allowed', async () => { + const order = custodyOrder(); + custodyOrderRepo.findOne.mockResolvedValue(order); + custodyAccountService.requireActingAllowed.mockResolvedValue(undefined); + custodyOrderRepo.update.mockResolvedValue(undefined); + + await service.confirmOrder(walletUserId, orderId); + + expect(custodyAccountService.requireActingAllowed).toHaveBeenCalledWith(order.user.userData.id); + expect(custodyOrderRepo.update).toHaveBeenCalledWith( + order.id, + expect.objectContaining({ status: CustodyOrderStatus.CONFIRMED }), + ); + }); + }); +}); diff --git a/src/subdomains/core/custody/services/__tests__/custody.service.spec.ts b/src/subdomains/core/custody/services/__tests__/custody.service.spec.ts new file mode 100644 index 0000000000..cff2b007c6 --- /dev/null +++ b/src/subdomains/core/custody/services/__tests__/custody.service.spec.ts @@ -0,0 +1,151 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { NotFoundException } from '@nestjs/common'; +import { UserRole } from 'src/shared/auth/user-role.enum'; +import { createCustomAsset } from 'src/shared/models/asset/__mocks__/asset.entity.mock'; +import { AssetService } from 'src/shared/models/asset/asset.service'; +import { AuthService } from 'src/subdomains/generic/user/models/auth/auth.service'; +import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; +import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; +import { createCustomUser } from 'src/subdomains/generic/user/models/user/__mocks__/user.entity.mock'; +import { UserService } from 'src/subdomains/generic/user/models/user/user.service'; +import { WalletService } from 'src/subdomains/generic/user/models/wallet/wallet.service'; +import { AssetPrice } from 'src/subdomains/supporting/pricing/domain/entities/asset-price.entity'; +import { AssetPricesService } from 'src/subdomains/supporting/pricing/services/asset-prices.service'; +import { RefService } from '../../../referral/process/ref.service'; +import { CustodyOrder } from '../../entities/custody-order.entity'; +import { CustodyOrderStatus, CustodyOrderType } from '../../enums/custody'; +import { CustodyBalanceRepository } from '../../repositories/custody-balance.repository'; +import { CustodyOrderRepository } from '../../repositories/custody-order.repository'; +import { CustodyService } from '../custody.service'; + +describe('CustodyService', () => { + let service: CustodyService; + let userDataService: DeepMocked; + let custodyOrderRepo: DeepMocked; + let assetPricesService: DeepMocked; + + const asset = createCustomAsset({ id: 42, name: 'BTC' }); + const custodyUser = createCustomUser({ id: 7, role: UserRole.CUSTODY }); + const accountId = 100; + + beforeEach(() => { + userDataService = createMock(); + custodyOrderRepo = createMock(); + assetPricesService = createMock(); + + service = new CustodyService( + createMock(), + userDataService, + createMock(), + createMock(), + createMock(), + custodyOrderRepo, + createMock(), + assetPricesService, + createMock(), + ); + + userDataService.getUserData.mockResolvedValue( + Object.assign(new UserData(), { id: accountId, users: [custodyUser] }), + ); + }); + + function depositOrder(updated: Date, amount: number): CustodyOrder { + return Object.assign(new CustodyOrder(), { + id: 1, + type: CustodyOrderType.DEPOSIT, + status: CustodyOrderStatus.COMPLETED, + inputAmount: amount, + inputAsset: asset, + user: custodyUser, + updated, + }); + } + + function assetPrice(created: Date, priceChf: number, priceEur: number, priceUsd: number): AssetPrice { + return Object.assign(new AssetPrice(), { + id: created.getTime(), + asset, + created, + priceChf, + priceEur, + priceUsd, + }); + } + + describe('getUserCustodyHistory', () => { + it('throws when the account is missing', async () => { + userDataService.getUserData.mockResolvedValue(null); + + await expect(service.getUserCustodyHistory(accountId)).rejects.toThrow(NotFoundException); + }); + + it('returns an empty history when there are no completed orders', async () => { + custodyOrderRepo.find.mockResolvedValue([]); + + await expect(service.getUserCustodyHistory(accountId)).resolves.toEqual({ totalValue: [] }); + }); + + it('uses a single price row per asset unchanged (balance × price)', async () => { + const balance = 2; + const priceChf = 100; + const priceEur = 90; + const priceUsd = 110; + + custodyOrderRepo.find.mockResolvedValue([depositOrder(new Date('2025-11-01T08:00:00.000Z'), balance)]); + assetPricesService.getAssetPrices.mockResolvedValue([ + assetPrice(new Date('2025-11-01T09:00:00.000Z'), priceChf, priceEur, priceUsd), + ]); + + const result = await service.getUserCustodyHistory(accountId); + + expect(result.totalValue).toHaveLength(1); + expect(result.totalValue[0].value).toEqual({ + chf: balance * priceChf, + eur: balance * priceEur, + usd: balance * priceUsd, + }); + }); + + it('uses the latest price per asset when multiple price rows fall on the same UTC day', async () => { + const balance = 2; + // Later timestamp first in the array — selection must use created, not array order. + const laterPrice = assetPrice(new Date('2025-11-01T23:01:00.000Z'), 150, 140, 160); + const earlierPrice = assetPrice(new Date('2025-11-01T09:00:00.000Z'), 100, 90, 110); + + custodyOrderRepo.find.mockResolvedValue([depositOrder(new Date('2025-11-01T08:00:00.000Z'), balance)]); + assetPricesService.getAssetPrices.mockResolvedValue([laterPrice, earlierPrice]); + + const result = await service.getUserCustodyHistory(accountId); + + expect(result.totalValue).toHaveLength(1); + // Must be balance × later price (300), not the sum of both (500). + expect(result.totalValue[0].value).toEqual({ + chf: balance * laterPrice.priceChf, + eur: balance * laterPrice.priceEur, + usd: balance * laterPrice.priceUsd, + }); + }); + + it('uses the higher id when two price rows share the same created timestamp', async () => { + const balance = 2; + const created = new Date('2025-11-01T09:00:00.000Z'); + // Lower id first in the array — selection must use higher id, not array order. + const lowerIdPrice = Object.assign(assetPrice(created, 100, 90, 110), { id: 1 }); + const higherIdPrice = Object.assign(assetPrice(created, 150, 140, 160), { id: 2 }); + + custodyOrderRepo.find.mockResolvedValue([depositOrder(new Date('2025-11-01T08:00:00.000Z'), balance)]); + assetPricesService.getAssetPrices.mockResolvedValue([lowerIdPrice, higherIdPrice]); + + const result = await service.getUserCustodyHistory(accountId); + + expect(result.totalValue).toHaveLength(1); + // Must be balance × higher-id price (300), not the lower-id price (200). + expect(result.totalValue[0].value).toEqual({ + chf: balance * higherIdPrice.priceChf, + eur: balance * higherIdPrice.priceEur, + usd: balance * higherIdPrice.priceUsd, + }); + }); + }); +}); diff --git a/src/subdomains/core/custody/services/custody-account.service.ts b/src/subdomains/core/custody/services/custody-account.service.ts index 6cf3d8e102..8580fcc954 100644 --- a/src/subdomains/core/custody/services/custody-account.service.ts +++ b/src/subdomains/core/custody/services/custody-account.service.ts @@ -56,8 +56,9 @@ export class CustodyAccountService { const allOwnedAccounts = account.custodyAccounts ?? []; const ownedAccounts = allOwnedAccounts.filter((ca) => ca.status === CustodyAccountStatus.ACTIVE); - // shared accounts via active grants only (history filtered in SQL, not JS) - const activeSharedGrants = await this.custodyAccountAccessRepo.find({ + // active grants only (history filtered in SQL, not JS) — these cover both foreign accounts + // the caller may see and own accounts the caller narrowed for themselves + const activeGrants = await this.custodyAccountAccessRepo.find({ where: { userData: { id: accountId }, active: true, @@ -65,10 +66,21 @@ export class CustodyAccountService { }, relations: { account: { owner: true } }, }); - const sharedAccounts = activeSharedGrants.filter((a) => a.account.owner.id !== accountId); + const sharedAccounts = activeGrants.filter((a) => a.account.owner.id !== accountId); + + // A grant on an own account narrows the owner's level — see checkAccess. Without this the + // list would offer WRITE where the authorisation only grants inspection. + const ownLevelByAccount = new Map( + activeGrants.filter((a) => a.account.owner.id === accountId).map((a) => [a.account.id, a.accessLevel]), + ); const custodyAccounts: CustodyAccountDto[] = [ - ...ownedAccounts.map((ca) => CustodyAccountDtoMapper.toDto(ca, CustodyAccessLevel.WRITE)), + ...ownedAccounts.map((ca) => { + // No grant on an own account means the owner keeps full disposal — that is the rule, + // not a fallback for a missing value. + const level = ownLevelByAccount.get(ca.id) ?? CustodyAccessLevel.WRITE; + return CustodyAccountDtoMapper.toDto(ca, level); + }), ...sharedAccounts.map((a) => CustodyAccountDtoMapper.toDto(a.account, a.accessLevel)), ]; @@ -84,9 +96,10 @@ export class CustodyAccountService { } /** - * Resolves an account for authorisation. Only ACTIVE accounts are visible — + * Resolves an account for the data path. Only ACTIVE accounts are visible — * Blocked/Closed are treated as missing so status cannot be bypassed via id. - * Shared by checkAccess and requireOwner so every auth path is covered once. + * Grant management does not go through here: it depends on ownership alone, so blocking an + * account cannot strand the grants on it (see requireOwner). */ async getCustodyAccountById(custodyAccountId: number): Promise { const custodyAccount = await this.custodyAccountRepo.findOne({ @@ -120,11 +133,6 @@ export class CustodyAccountService { const custodyAccount = await this.getCustodyAccountById(custodyAccountId); - // Owner has WRITE access - if (custodyAccount.owner.id === accountId) { - return { custodyAccount, isLegacy: false }; - } - // Active grant only — inactive history must not participate in authorisation const access = await this.custodyAccountAccessRepo.findOne({ where: { @@ -133,6 +141,16 @@ export class CustodyAccountService { active: true, }, }); + + // Owner has WRITE access — unless they granted themselves a narrower level. A signed + // authorisation can reserve acting for someone else while the owner only inspects; the + // owner's own grant is the only way to express that, so it must not be overridden here. + // Managing grants stays with the owner regardless (requireOwner), so this cannot lock + // anyone out of their own account. + if (custodyAccount.owner.id === accountId && !access) { + return { custodyAccount, isLegacy: false }; + } + if (!access) { throw new ForbiddenException('No access to this custody account'); } @@ -155,7 +173,9 @@ export class CustodyAccountService { * disclose holdings outside the grant, so refuse with 409 instead of a fabricated subset * or an over-broad full Safe. The owner already authorises every one of those rows and * reaches them via the caller-scoped endpoints; their own authorisation is total, so the - * ambiguity check is skipped when the caller is the owner. + * ambiguity check is skipped when the caller is the owner. That still holds once an owner + * narrows themselves to READ: what a narrowed grant withdraws is acting, not sight — the + * holdings are the owner's either way, so there is nothing to disclose across a boundary. * * Once balances and orders carry an account, callers filter by it and this multi-account * refusal is no longer needed. Legacy is unaffected (caller has no accounts). @@ -255,6 +275,18 @@ export class CustodyAccountService { const account = await this.requireOwner(custodyAccountId, ownerAccountId); + // Grant management stays reachable on a blocked account so nobody gets stranded — but that + // covers taking rights away, not handing them out. Widening the circle of authorised people + // during a hold is exactly what a hold is meant to prevent, and it is no way out of one + // either. Withdrawing and re-levelling stay open. + // + // The status is read outside the write, so a hold placed in that instant would not be seen + // here. Nothing sets a non-active status yet; whatever introduces one must deactivate the + // account's grants as part of the same change, which closes this on its own. + if (account.status !== CustodyAccountStatus.ACTIVE) { + throw new BadRequestException('Cannot grant access on an account that is not active'); + } + const target = await this.resolveUserByMail(mail); if (target.id === ownerAccountId) { throw new BadRequestException('Cannot grant access to yourself'); @@ -269,18 +301,21 @@ export class CustodyAccountService { ownerAccountId: number, accessLevel: CustodyAccessLevel, ): Promise { + // Authorise before touching any grant row. The owner may re-level any grant including + // their own — see rejectOwnerGrantRevocation for why only revoking stays refused. const account = await this.requireOwner(custodyAccountId, ownerAccountId); // Read + deactivate + insert under one transaction with a row lock so concurrent // update/revoke cannot leave a superseded active grant behind a false revoke success. return this.custodyAccountAccessRepo.manager.transaction(async (manager) => { const access = await this.lockActiveAccessGrant(manager, custodyAccountId, accessId); - this.rejectOwnerGrantMutation(access, account, 'modify'); if (access.accessLevel === accessLevel) { return access; } + this.rejectElevationWhileNotActive(access, account, accessLevel); + await manager.update(CustodyAccountAccess, ...access.deactivate()); const grant = manager.create(CustodyAccountAccess, { @@ -299,7 +334,7 @@ export class CustodyAccountService { await this.custodyAccountAccessRepo.manager.transaction(async (manager) => { const access = await this.lockActiveAccessGrant(manager, custodyAccountId, accessId); - this.rejectOwnerGrantMutation(access, account, 'revoke'); + this.rejectOwnerGrantRevocation(access, account); await manager.update(CustodyAccountAccess, ...access.deactivate()); }); @@ -319,39 +354,105 @@ export class CustodyAccountService { } /** - * Owner-only authorisation for grant management. Missing, non-active and foreign - * accounts all yield the same Forbidden so callers cannot probe existence (403 vs 404). - * NotFound for missing grant rows stays downstream after ownership is established. + * Owner-only authorisation for grant management. Missing and foreign accounts yield the same + * Forbidden so callers cannot probe existence (403 vs 404). NotFound for missing grant rows + * stays downstream after ownership is established. + * + * Status is deliberately not required here, unlike on the data paths. Blocking an account + * governs what may be done with it, not who decides that. Requiring ACTIVE would strand every + * grant on a blocked account: the owner could neither lift a narrowing they placed on it nor + * withdraw a stranger's access, and since a narrowing blocks the owner's whole Safe + * (requireActingAllowed), one blocked account would freeze all their others with no way back. + * + * That reasoning covers taking rights away, not handing them out. Issuing a grant and raising + * a stranger's level both check the status themselves (grantAccess, + * rejectElevationWhileNotActive), so a hold cannot be used to widen anyone's authority. */ private async requireOwner(custodyAccountId: number, accountId: number): Promise { - let custodyAccount: CustodyAccount; - try { - custodyAccount = await this.getCustodyAccountById(custodyAccountId); - } catch (e) { - if (e instanceof NotFoundException) { - throw new ForbiddenException('Only the account owner can manage access grants'); - } - throw e; - } + const custodyAccount = await this.custodyAccountRepo.findOne({ + where: { id: custodyAccountId }, + relations: { owner: true }, + }); - if (custodyAccount.owner.id !== accountId) { + if (!custodyAccount || custodyAccount.owner.id !== accountId) { throw new ForbiddenException('Only the account owner can manage access grants'); } return custodyAccount; } - private rejectOwnerGrantMutation( + /** + * Refuses acting for an owner who limited themselves to inspection. + * + * Orders address a whole Safe, not a single account — balances and orders carry no account + * today. So any own account narrowed to READ blocks acting: the order could touch exactly + * those holdings, and serving it would act past the authorisation. Fail closed rather than + * guess which account an order belongs to. + * + * Account status is deliberately not filtered here, unlike everywhere else. Elsewhere a + * non-active account is treated as absent so it grants nothing; here absence would grant + * something — the right to act. Blocking or closing an account must never be a way to shed + * a restriction. + * + * Without a narrowing grant this passes, which is every account in production today. + * + * The check does not span a lock with the write that follows it, so a narrowing committed in + * that gap lets one order through. Accepted deliberately: only the owner manages grants and + * only the owner narrows themselves, so the sole party who could win that race is the one + * who may lift the restriction outright. There is no adversary to lock out, and holding a + * lock across order creation would slow every trade to guard against nobody. + */ + async requireActingAllowed(accountId: number): Promise { + const narrowed = await this.custodyAccountAccessRepo.findOne({ + where: { + userData: { id: accountId }, + account: { owner: { id: accountId } }, + accessLevel: CustodyAccessLevel.READ, + active: true, + }, + }); + + if (narrowed) { + throw new ForbiddenException('This Safe is limited to inspection, acting is not permitted'); + } + } + + /** + * On an account that is not active, rights may be taken away but not handed out — the same + * rule grantAccess applies to new grants. Raising a stranger from READ to WRITE arms a right + * that becomes effective the moment the account is unblocked, which is exactly what a hold is + * meant to prevent; lowering them stays open, as does anything on the owner's own row, which + * is their way out of a narrowing. + * + * Elevation is recognised by comparing the two levels there are. A third level would have to + * turn this into an ordering comparison — an equality check would let a raise slip past. + * + * The status comes from the account read before the write, so a hold placed in that instant + * would not be seen. Nothing sets a non-active status yet; whatever introduces one must + * deactivate the account's grants as part of the same change, which closes this on its own. + */ + private rejectElevationWhileNotActive( access: CustodyAccountAccess, account: CustodyAccount, - action: 'modify' | 'revoke', + newLevel: CustodyAccessLevel, ): void { + const isOwnGrant = access.userData.id === account.owner.id; + const isElevation = access.accessLevel === CustodyAccessLevel.READ && newLevel === CustodyAccessLevel.WRITE; + + if (!isOwnGrant && isElevation && account.status !== CustodyAccountStatus.ACTIVE) { + throw new BadRequestException('Cannot raise access on an account that is not active'); + } + } + + /** + * The owner's own grant may be re-levelled but never revoked. Re-levelling is how an owner + * limits themselves to inspection and hands acting to someone else — and how they take it + * back, since only the owner reaches this path (requireOwner). Revoking would leave the + * account without an owner row and make the level unrecordable, so it stays refused. + */ + private rejectOwnerGrantRevocation(access: CustodyAccountAccess, account: CustodyAccount): void { if (access.userData.id === account.owner.id) { - throw new BadRequestException( - action === 'revoke' - ? "Cannot revoke the account owner's access grant" - : "Cannot modify the account owner's access grant", - ); + throw new BadRequestException("Cannot revoke the account owner's access grant"); } } diff --git a/src/subdomains/core/custody/services/custody-order.service.ts b/src/subdomains/core/custody/services/custody-order.service.ts index 14b2814d55..a7e10fef77 100644 --- a/src/subdomains/core/custody/services/custody-order.service.ts +++ b/src/subdomains/core/custody/services/custody-order.service.ts @@ -42,6 +42,7 @@ import { CustodyOrderResponseDtoMapper } from '../mappers/custody-order-response import { GetCustodyOrderDtoMapper } from '../mappers/get-custody-order-dto.mapper'; import { CustodyOrderStepRepository } from '../repositories/custody-order-step.repository'; import { CustodyOrderRepository } from '../repositories/custody-order.repository'; +import { CustodyAccountService } from './custody-account.service'; import { CustodyService } from './custody.service'; @Injectable() @@ -53,6 +54,7 @@ export class CustodyOrderService { private readonly custodyOrderRepo: CustodyOrderRepository, private readonly custodyOrderStepRepo: CustodyOrderStepRepository, private readonly custodyService: CustodyService, + private readonly custodyAccountService: CustodyAccountService, @Inject(forwardRef(() => SellService)) private readonly sellService: SellService, @Inject(forwardRef(() => BuyService)) @@ -69,6 +71,10 @@ export class CustodyOrderService { const user = await this.userService.getUser(jwt.user, { userData: true, custodyBalances: true }); if (!user) throw new NotFoundException('User not found'); + // An owner who limited themselves to inspection must not act, and this route is how acting + // reaches the Safe — the account guards do not cover it. + await this.custodyAccountService.requireActingAllowed(user.userData.id); + const orderDto: CreateCustodyOrderInternalDto = { user, type: dto.type }; let paymentInfo: CustodyOrderResponseDto = null; @@ -262,12 +268,16 @@ export class CustodyOrderService { async confirmOrder(userId: number, orderId: number): Promise { const order = await this.custodyOrderRepo.findOne({ where: { id: orderId }, - relations: { user: true }, + relations: { user: { userData: true } }, }); if (!order) throw new NotFoundException('Order not found'); if (userId != order.user.id) throw new ForbiddenException('Order is not from current user'); + // Confirming is the step that releases an order, so it needs the same check as creating — + // otherwise a narrowing that took effect in between would not hold. + await this.custodyAccountService.requireActingAllowed(order.user.userData.id); + await this.custodyOrderRepo.update(...order.confirm()); } diff --git a/src/subdomains/core/custody/services/custody.service.ts b/src/subdomains/core/custody/services/custody.service.ts index 10f8cd0bff..77ff5cdde8 100644 --- a/src/subdomains/core/custody/services/custody.service.ts +++ b/src/subdomains/core/custody/services/custody.service.ts @@ -10,6 +10,7 @@ import { UserDataService } from 'src/subdomains/generic/user/models/user-data/us import { User } from 'src/subdomains/generic/user/models/user/user.entity'; import { UserService } from 'src/subdomains/generic/user/models/user/user.service'; import { WalletService } from 'src/subdomains/generic/user/models/wallet/wallet.service'; +import { AssetPrice } from 'src/subdomains/supporting/pricing/domain/entities/asset-price.entity'; import { AssetPricesService } from 'src/subdomains/supporting/pricing/services/asset-prices.service'; import { In } from 'typeorm'; import { RefService } from '../../referral/process/ref.service'; @@ -28,6 +29,12 @@ interface CustodyOrderSingle { amount: number; } +interface DailyFiatValue { + chf: number; + eur: number; + usd: number; +} + @Injectable() export class CustodyService { constructor( @@ -203,16 +210,7 @@ export class CustodyService { } // calculate daily portfolio value from current balances and available prices - const dailyValue = dayPrices.reduce( - (value, price) => { - const balance = assetBalancesMap.get(price.asset.id) ?? 0; - value.chf += balance * price.priceChf; - value.eur += balance * price.priceEur; - value.usd += balance * price.priceUsd; - return value; - }, - { chf: 0, eur: 0, usd: 0 }, - ); + const dailyValue = this.calculateDailyPortfolioValue(dayPrices, assetBalancesMap); totalValue.push({ date: new Date(day), @@ -227,6 +225,38 @@ export class CustodyService { return { totalValue }; } + /** + * Grouping uses UTC (`Util.isoDate`). `asset_price.created` is a local-time + * `timestamp without time zone`, so multiple snapshots can land on the same UTC day + * (e.g. local post-midnight). Use the latest price per asset for that day. + * On equal `created`, the higher `id` wins (later insert), independent of list order. + */ + private calculateDailyPortfolioValue(dayPrices: AssetPrice[], assetBalancesMap: Map): DailyFiatValue { + const latestPriceByAsset = new Map(); + + for (const price of dayPrices) { + const existing = latestPriceByAsset.get(price.asset.id); + if ( + !existing || + price.created.getTime() > existing.created.getTime() || + (price.created.getTime() === existing.created.getTime() && price.id > existing.id) + ) { + latestPriceByAsset.set(price.asset.id, price); + } + } + + return [...latestPriceByAsset.values()].reduce( + (value, price) => { + const balance = assetBalancesMap.get(price.asset.id) ?? 0; + value.chf += balance * price.priceChf; + value.eur += balance * price.priceEur; + value.usd += balance * price.priceUsd; + return value; + }, + { chf: 0, eur: 0, usd: 0 }, + ); + } + async getUserTotalBalancesChf(date: Date): Promise> { const balances = await this.getHistoricalBalances(date); if (!balances.length) return new Map(); diff --git a/src/subdomains/core/liquidity-management/adapters/actions/__tests__/scrypt.adapter.spec.ts b/src/subdomains/core/liquidity-management/adapters/actions/__tests__/scrypt.adapter.spec.ts index c1997ddf51..387976f068 100644 --- a/src/subdomains/core/liquidity-management/adapters/actions/__tests__/scrypt.adapter.spec.ts +++ b/src/subdomains/core/liquidity-management/adapters/actions/__tests__/scrypt.adapter.spec.ts @@ -1,12 +1,28 @@ import { createMock } from '@golevelup/ts-jest'; import { Blockchain } from 'src/integration/blockchain/shared/enums/blockchain.enum'; -import { ScryptTransactionStatus, ScryptWithdrawStatus } from 'src/integration/exchange/dto/scrypt.dto'; +import { + ScryptBalanceTransaction, + ScryptOrderInfo, + ScryptOrderStatus, + ScryptTransactionStatus, + ScryptWithdrawStatus, +} from 'src/integration/exchange/dto/scrypt.dto'; +import { + ScryptAmendRejectedError, + ScryptOrderNotFoundError, + ScryptRequestTimeoutError, + ScryptUnconfirmedWriteError, + ScryptVenueRejectionError, +} from 'src/integration/exchange/services/scrypt-websocket-connection'; import { ScryptService } from 'src/integration/exchange/services/scrypt.service'; import { AssetService } from 'src/shared/models/asset/asset.service'; import { DexService } from 'src/subdomains/supporting/dex/services/dex.service'; import { PricingService } from 'src/subdomains/supporting/pricing/services/pricing.service'; +import { LiquidityManagementAction } from '../../../entities/liquidity-management-action.entity'; import { LiquidityManagementOrder } from '../../../entities/liquidity-management-order.entity'; +import { UncertainOrderResolution } from '../../../enums'; import { OrderFailedException } from '../../../exceptions/order-failed.exception'; +import { OrderOutcomeUnknownException } from '../../../exceptions/order-outcome-unknown.exception'; import { LiquidityManagementOrderRepository } from '../../../repositories/liquidity-management-order.repository'; import { ScryptAdapter, ScryptAdapterCommands } from '../scrypt.adapter'; @@ -15,6 +31,8 @@ const DEST_ENV = 'TEST_SCRYPT_WITHDRAW_ADDR'; function createWithdrawOrder(overrides: Partial = {}): LiquidityManagementOrder { return Object.assign(new LiquidityManagementOrder(), { correlationId: 'corr-1', + // young enough that an unobservable withdrawal is still waited on rather than quarantined + created: new Date(), action: { command: ScryptAdapterCommands.WITHDRAW, paramMap: { @@ -27,6 +45,28 @@ function createWithdrawOrder(overrides: Partial = {}): }); } +function createUncertainSellOrder(overrides: Partial = {}): LiquidityManagementOrder { + return Object.assign(new LiquidityManagementOrder(), { + id: 4711, + correlationId: 'dfx-lm-4711', + // young enough that an unreadable order is still retried rather than quarantined + created: new Date(), + updated: new Date(Date.now() - 30 * 60 * 1000), + action: { command: ScryptAdapterCommands.SELL, paramMap: {} }, + ...overrides, + }); +} + +/** Minimal but fully typed venue order record, so the tests do not have to widen the return type. */ +function venueOrder(id: string, status = ScryptOrderStatus.NEW): ScryptOrderInfo { + return { id, symbol: 'EUR/USDT', side: 'Sell', status, quantity: 1, filledQuantity: 0, remainingQuantity: 1 }; +} + +/** Typed action stub — `paramMap` is a getter over `params`, so the raw field is what a fixture sets. */ +function withdrawAction(): LiquidityManagementAction { + return Object.assign(new LiquidityManagementAction(), { command: ScryptAdapterCommands.WITHDRAW, params: '{}' }); +} + describe('ScryptAdapter', () => { let adapter: ScryptAdapter; let scryptService: ScryptService; @@ -129,4 +169,418 @@ describe('ScryptAdapter', () => { expect(dexService.checkTransferCompletion).toHaveBeenCalledWith('0xsuccess', Blockchain.ETHEREUM); }); }); + + describe('checkWithdrawCompletion — unobservable withdrawals', () => { + it('quarantines an aged withdrawal the venue has no record of at all', async () => { + jest.spyOn(scryptService, 'getWithdrawalStatus').mockResolvedValue(null); + const old = createWithdrawOrder({ created: new Date(Date.now() - 120 * 60 * 1000) }); + + await expect(adapter.checkCompletion(old)).rejects.toBeInstanceOf(OrderOutcomeUnknownException); + }); + + it('keeps waiting on an aged withdrawal the venue DOES know but has not settled', async () => { + // a record without a hash is an observation, not an unknown outcome — quarantining it would only + // bounce the order between reconciliation and the completion check + jest.spyOn(scryptService, 'getWithdrawalStatus').mockResolvedValue({ + id: 'w-inflight', + status: ScryptTransactionStatus.COMPLETED, + }); + const old = createWithdrawOrder({ created: new Date(Date.now() - 120 * 60 * 1000) }); + + await expect(adapter.checkCompletion(old)).resolves.toBe(false); + }); + + it('still just waits while the withdrawal is young', async () => { + jest.spyOn(scryptService, 'getWithdrawalStatus').mockResolvedValue(null); + + await expect(adapter.checkCompletion(createWithdrawOrder({ created: new Date() }))).resolves.toBe(false); + }); + }); + + describe('reserveCorrelationId', () => { + it('derives a reproducible reference from the order id', () => { + const order = Object.assign(new LiquidityManagementOrder(), { id: 4711 }); + + const reference = adapter.reserveCorrelationId(order); + + expect(reference).toBe('dfx-lm-4711'); + // the venue requires uniqueness per day and fewer than 36 characters + expect(reference.length).toBeLessThan(36); + }); + }); + + describe('classifySendOutcome', () => { + it('turns a timeout into an unknown outcome, so the order is never silently repeated', () => { + const timeout = new ScryptRequestTimeoutError('Timeout waiting for ExecutionReport update after 60000ms'); + + const classified = adapter['classifySendOutcome'](timeout, 'sell of 1 EUR to USDT'); + + expect(classified).toBeInstanceOf(OrderOutcomeUnknownException); + }); + + it('also treats a dropped connection as unknown — the bytes may already have reached the venue', () => { + // requestWithId hands the payload to the socket before the pending entry exists; a later close rejects + // it with a generic message that says nothing about whether the venue acted on it. + const dropped = new Error('Connection closed'); + + const classified = adapter['classifySendOutcome'](dropped, 'withdrawal of 1 USDT to 0xabc'); + + expect(classified).toBeInstanceOf(OrderOutcomeUnknownException); + }); + + it('keeps a venue rejection an ordinary failure — the venue replied, so the outcome is known', () => { + const rejected = new ScryptVenueRejectionError('Scrypt withdrawal rejected: insufficient limit'); + + const classified = adapter['classifySendOutcome'](rejected, 'withdrawal of 1 USDT to 0xabc'); + + expect(classified).toBe(rejected); + expect(classified).not.toBeInstanceOf(OrderOutcomeUnknownException); + }); + }); + + describe('checkTradeCompletion — recovering a lost adoption', () => { + it('adopts a claimed replacement the venue is working before it may write again', async () => { + // the window: the venue accepted the replacement, the save that would have recorded it failed, and the + // row still names the predecessor the venue has since cancelled + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => (id === 'dfx-lm-4711-1' ? venueOrder(id) : null)); + jest.spyOn(scryptService, 'checkTrade').mockResolvedValue(false); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await adapter['checkTradeCompletion'](order, 'EUR', 'USDT'); + + expect(order.correlationId).toBe('dfx-lm-4711-1'); + expect(orderRepo.save).toHaveBeenCalled(); + }); + + it('does not adopt a replacement the venue rejected', async () => { + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => venueOrder(id, ScryptOrderStatus.REJECTED)); + jest.spyOn(scryptService, 'checkTrade').mockResolvedValue(false); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await adapter['checkTradeCompletion'](order, 'EUR', 'USDT'); + + expect(order.correlationId).toBe('dfx-lm-4711'); + }); + + it('never walks back: after an amend this row recorded, the cancelled original is left alone', async () => { + // a predecessor is not a replacement. Adopting it would restart the very quantity the replacement the + // row already names is working, which is the double execution this whole path exists to prevent. + const getOrderStatus = jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => + venueOrder(id, id === 'dfx-lm-4711' ? ScryptOrderStatus.CANCELED : ScryptOrderStatus.NEW), + ); + jest.spyOn(scryptService, 'checkTrade').mockResolvedValue(false); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + order.updateCorrelationId('dfx-lm-4711-1'); + + await adapter['checkTradeCompletion'](order, 'EUR', 'USDT'); + + expect(order.correlationId).toBe('dfx-lm-4711-1'); + expect(getOrderStatus).not.toHaveBeenCalledWith('dfx-lm-4711'); + }); + + it('writes nothing while a claimed replacement is absent, even with a cancelled predecessor in view', async () => { + // the reference is claimed BEFORE the request leaves, so one the venue does not show may be live there + // this second — and the cancelled predecessor is exactly the bait for sending a second one next to it + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => + id === 'dfx-lm-4711' ? venueOrder(id, ScryptOrderStatus.CANCELED) : null, + ); + const checkTrade = jest.spyOn(scryptService, 'checkTrade').mockResolvedValue(false); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter['checkTradeCompletion'](order, 'EUR', 'USDT')).resolves.toBe(false); + + expect(checkTrade).not.toHaveBeenCalled(); + expect(order.correlationId).toBe('dfx-lm-4711'); + }); + + it('writes nothing when the venue cannot be asked about a claimed replacement at all', async () => { + // an unreadable lookup is not a reply either, and only a reply can rule a claimed reference out + jest.spyOn(scryptService, 'getOrderStatus').mockRejectedValue(new Error('connection closed')); + const checkTrade = jest.spyOn(scryptService, 'checkTrade').mockResolvedValue(false); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter['checkTradeCompletion'](order, 'EUR', 'USDT')).resolves.toBe(false); + + expect(checkTrade).not.toHaveBeenCalled(); + }); + + it('quarantines an aged order whose claimed replacement nobody can account for', async () => { + // holding writes back is safe, but not for good — the manual path only accepts quarantined orders, + // so without this an order nobody can observe would have no way out at all + jest.spyOn(scryptService, 'getOrderStatus').mockResolvedValue(null); + // a check that would otherwise report cleanly, so the quarantine can only come from the barrier itself + const checkTrade = jest.spyOn(scryptService, 'checkTrade').mockResolvedValue(false); + const old = createUncertainSellOrder({ created: new Date(Date.now() - 120 * 60 * 1000) }); + old.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter['checkTradeCompletion'](old, 'EUR', 'USDT')).rejects.toThrow(OrderOutcomeUnknownException); + + expect(checkTrade).not.toHaveBeenCalled(); + }); + + it('does not quarantine an aged order the venue can still show us, whatever failed downstream', async () => { + // otherwise reconciliation hands it straight back and the next check quarantines it again + jest.spyOn(scryptService, 'getOrderStatus').mockImplementation(async (id: string) => venueOrder(id)); + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new Error('pricing service unavailable')); + const old = createUncertainSellOrder({ created: new Date(Date.now() - 120 * 60 * 1000) }); + + await expect(adapter['checkTradeCompletion'](old, 'EUR', 'USDT')).resolves.toBe(false); + }); + }); + + describe('checkTradeCompletion — the amend boundary', () => { + it('quarantines when an amend or restart went unconfirmed, instead of failing the order', async () => { + // The check can WRITE (cancel-replace, restart). An unconfirmed write there may have created a live + // order at the venue; failing would pause the rule, which auto-reactivates and reissues the trade. + jest + .spyOn(scryptService, 'checkTrade') + .mockRejectedValue(new ScryptUnconfirmedWriteError('no confirmed outcome for the amend', 'dfx-lm-4711-1')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).rejects.toBeInstanceOf( + OrderOutcomeUnknownException, + ); + }); + + it('carries the replacement reference into the quarantine reason, so it can be reconciled', async () => { + jest + .spyOn(scryptService, 'checkTrade') + .mockRejectedValue(new ScryptUnconfirmedWriteError('no confirmed outcome for the amend', 'dfx-lm-4711-1')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).rejects.toThrow( + /dfx-lm-4711-1/, + ); + }); + + it('still treats a plain dropped connection on the read path as retry-next-tick', async () => { + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new Error('Connection closed')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).resolves.toBe(false); + }); + + it('does not fail an acknowledged order just because it could not be read', async () => { + // Failing here would release the rule to open a second position while the first is live at the venue. + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new Error('malformed market data snapshot')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).resolves.toBe(false); + }); + + it('also stops retrying when the error is a transient transport one', async () => { + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new Error('Connection closed')); + jest.spyOn(scryptService, 'getOrderStatus').mockResolvedValue(null); + const old = createUncertainSellOrder({ created: new Date(Date.now() - 120 * 60 * 1000) }); + + await expect(adapter['checkTradeCompletion'](old, 'EUR', 'USDT')).rejects.toBeInstanceOf( + OrderOutcomeUnknownException, + ); + }); + + it('stops retrying an order it has been unable to observe for too long, and quarantines it', async () => { + // otherwise it polls for good: the manual path only accepts quarantined orders, so there would be no + // way out at all + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new Error('malformed market data snapshot')); + // and the venue cannot show us the order either — that is what makes it a blind spot rather than a + // downstream hiccup + jest.spyOn(scryptService, 'getOrderStatus').mockResolvedValue(null); + const old = createUncertainSellOrder({ created: new Date(Date.now() - 120 * 60 * 1000) }); + + await expect(adapter['checkTradeCompletion'](old, 'EUR', 'USDT')).rejects.toBeInstanceOf( + OrderOutcomeUnknownException, + ); + }); + + it('quarantines an order the venue acknowledged and can no longer find', async () => { + jest + .spyOn(scryptService, 'checkTrade') + .mockRejectedValue(new ScryptOrderNotFoundError('Order dfx-lm-4711 not found after 90 minutes')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).rejects.toBeInstanceOf( + OrderOutcomeUnknownException, + ); + }); + + it('keeps watching the original when the venue refuses an amend, and notes the spent reference', async () => { + jest + .spyOn(scryptService, 'checkTrade') + .mockRejectedValue(new ScryptAmendRejectedError('Scrypt refused the amend', 'dfx-lm-4711-1')); + const order = createUncertainSellOrder(); + + await expect(adapter['checkTradeCompletion'](order, 'EUR', 'USDT')).resolves.toBe(false); + // recorded, so the next derivation moves on instead of reusing a reference the venue already burnt + expect(adapter['nextCorrelationId'](order)).toBe('dfx-lm-4711-2'); + }); + + it('does not accept a mere message resembling a rejection as a verdict', async () => { + // the whole point of the type: a transport error quoting the phrase must not end the order + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new Error('Scrypt order rejected: bad price')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).resolves.toBe(false); + }); + + it('quarantines a read timeout rather than failing — every write here is already wrapped', async () => { + jest.spyOn(scryptService, 'checkTrade').mockRejectedValue(new ScryptRequestTimeoutError('Request timeout')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).resolves.toBe(false); + }); + + it('fails the order when the venue explicitly rejected it — that is a verdict, not silence', async () => { + jest + .spyOn(scryptService, 'checkTrade') + .mockRejectedValue(new ScryptVenueRejectionError('Scrypt order rejected: bad price')); + + await expect(adapter['checkTradeCompletion'](createUncertainSellOrder(), 'EUR', 'USDT')).rejects.toBeInstanceOf( + OrderFailedException, + ); + }); + }); + + describe('nextCorrelationId', () => { + it('names the replacement an amend or restart would create, reproducibly from the row', () => { + const order = Object.assign(new LiquidityManagementOrder(), { id: 4711, correlationId: 'dfx-lm-4711' }); + + expect(adapter['nextCorrelationId'](order)).toBe('dfx-lm-4711-1'); + + order.updateCorrelationId('dfx-lm-4711-1'); + expect(adapter['nextCorrelationId'](order)).toBe('dfx-lm-4711-2'); + }); + }); + + describe('resolveUncertainOrder', () => { + it('reports SENT when the venue knows the reference', async () => { + jest.spyOn(scryptService, 'getOrderStatus').mockResolvedValue(venueOrder('dfx-lm-4711')); + + await expect(adapter.resolveUncertainOrder(createUncertainSellOrder())).resolves.toBe( + UncertainOrderResolution.SENT, + ); + }); + + it('never concludes NOT_SENT from mere absence, however old the order is', async () => { + // Scrypt offers no terminal "this reference was never accepted" reply, so absence from a snapshot is + // not evidence. Releasing the rule on that basis is what would let a late-materialising request repeat. + jest.spyOn(scryptService, 'getOrderStatus').mockResolvedValue(null); + const ancient = createUncertainSellOrder({ updated: new Date(Date.now() - 24 * 60 * 60 * 1000) }); + + await expect(adapter.resolveUncertainOrder(ancient)).resolves.toBe(UncertainOrderResolution.UNRESOLVED); + }); + + it('reports UNAVAILABLE when the lookup itself fails — no question reached the venue', async () => { + jest.spyOn(scryptService, 'getOrderStatus').mockRejectedValue(new Error('Connection closed')); + + await expect(adapter.resolveUncertainOrder(createUncertainSellOrder())).resolves.toBe( + UncertainOrderResolution.UNAVAILABLE, + ); + }); + + it('stays UNRESOLVED when no reference was ever reserved', async () => { + const order = createUncertainSellOrder({ correlationId: undefined }); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.UNRESOLVED); + }); + + it('uses the withdrawal lookup for withdraw orders', async () => { + jest.spyOn(scryptService, 'findWithdrawal').mockResolvedValue(null); + const order = createUncertainSellOrder({ + action: withdrawAction(), + }); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.UNRESOLVED); + expect(scryptService.findWithdrawal).toHaveBeenCalledWith('dfx-lm-4711'); + }); + + it('reports SENT for a withdraw order the venue does know', async () => { + jest + .spyOn(scryptService, 'findWithdrawal') + .mockResolvedValue({ ClReqID: 'dfx-lm-4711' } as ScryptBalanceTransaction); + const order = createUncertainSellOrder({ + action: withdrawAction(), + }); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.SENT); + }); + + it('finds a claimed replacement the venue accepted but never confirmed, and tracks its reference', async () => { + // the amend boundary: the original is unknown to the venue, the claimed replacement is live + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => (id === 'dfx-lm-4711-1' ? venueOrder(id) : null)); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.SENT); + expect(order.correlationId).toBe('dfx-lm-4711-1'); + }); + + it('reports NOT_SENT when every reference this order sent was rejected — nothing is live', async () => { + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => venueOrder(id, ScryptOrderStatus.REJECTED)); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.NOT_SENT); + }); + + it('checks the reference that was actually sent, never a synthesised future one', async () => { + // regression guard: reconciling a freshly quarantined order used to start at an unsent reference, + // stop on its meaningless absence, and never look at the one that had really gone out + const seen: string[] = []; + jest.spyOn(scryptService, 'getOrderStatus').mockImplementation(async (id: string) => { + seen.push(id); + return venueOrder(id); + }); + + await expect(adapter.resolveUncertainOrder(createUncertainSellOrder())).resolves.toBe( + UncertainOrderResolution.SENT, + ); + expect(seen).toEqual(['dfx-lm-4711']); + }); + + it('stays quarantined when a claimed replacement is not (yet) visible, even if the predecessor is', async () => { + // an accepted replacement may lag in the venue's view; falling back to the order it replaced would + // report SENT on a superseded reference and leave the live replacement untracked + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => (id === 'dfx-lm-4711' ? venueOrder(id) : null)); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.UNRESOLVED); + }); + + it('falls back to the predecessor only after the replacement was explicitly rejected', async () => { + jest + .spyOn(scryptService, 'getOrderStatus') + .mockImplementation(async (id: string) => + venueOrder(id, id === 'dfx-lm-4711-1' ? ScryptOrderStatus.REJECTED : ScryptOrderStatus.NEW), + ); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.SENT); + expect(order.correlationId).toBe('dfx-lm-4711'); + }); + + it('prefers the replacement when BOTH it and the superseded original still exist', async () => { + // The replaced order lingers at the venue in a cancelled state. Matching it first would report SENT + // and leave the live replacement untracked, with the completion check polling a dead reference. + jest.spyOn(scryptService, 'getOrderStatus').mockImplementation(async (id: string) => venueOrder(id)); + const order = createUncertainSellOrder(); + order.recordSpentCorrelationId('dfx-lm-4711-1'); + + await expect(adapter.resolveUncertainOrder(order)).resolves.toBe(UncertainOrderResolution.SENT); + expect(order.correlationId).toBe('dfx-lm-4711-1'); + }); + }); }); diff --git a/src/subdomains/core/liquidity-management/adapters/actions/base/liquidity-action.adapter.ts b/src/subdomains/core/liquidity-management/adapters/actions/base/liquidity-action.adapter.ts index ef38ed1e9c..7caaa05072 100644 --- a/src/subdomains/core/liquidity-management/adapters/actions/base/liquidity-action.adapter.ts +++ b/src/subdomains/core/liquidity-management/adapters/actions/base/liquidity-action.adapter.ts @@ -4,6 +4,7 @@ import { LiquidityManagementSystem } from '../../../enums'; import { OrderFailedException } from '../../../exceptions/order-failed.exception'; import { OrderNotNecessaryException } from '../../../exceptions/order-not-necessary.exception'; import { OrderNotProcessableException } from '../../../exceptions/order-not-processable.exception'; +import { OrderOutcomeUnknownException } from '../../../exceptions/order-outcome-unknown.exception'; import { Command, CorrelationId, LiquidityActionIntegration } from '../../../interfaces'; export abstract class LiquidityActionAdapter implements LiquidityActionIntegration { @@ -30,7 +31,14 @@ export abstract class LiquidityActionAdapter implements LiquidityActionIntegrati try { return await this.commands.get(command)(order); } catch (e) { - if (e instanceof OrderNotProcessableException || e instanceof OrderNotNecessaryException) throw e; + if ( + e instanceof OrderNotProcessableException || + e instanceof OrderNotNecessaryException || + // must survive the catch-all below: collapsing an unknown outcome into OrderFailedException is + // what makes the pipeline retry a request that may already have executed + e instanceof OrderOutcomeUnknownException + ) + throw e; throw new OrderFailedException(e.message); } diff --git a/src/subdomains/core/liquidity-management/adapters/actions/scrypt.adapter.ts b/src/subdomains/core/liquidity-management/adapters/actions/scrypt.adapter.ts index f68de2cf2f..a248b471ce 100644 --- a/src/subdomains/core/liquidity-management/adapters/actions/scrypt.adapter.ts +++ b/src/subdomains/core/liquidity-management/adapters/actions/scrypt.adapter.ts @@ -1,8 +1,18 @@ import { Injectable } from '@nestjs/common'; import { Blockchain } from 'src/integration/blockchain/shared/enums/blockchain.enum'; -import { ScryptOrderInfo, ScryptOrderSide, ScryptTransactionStatus } from 'src/integration/exchange/dto/scrypt.dto'; +import { + ScryptOrderInfo, + ScryptOrderSide, + ScryptOrderStatus, + ScryptTransactionStatus, +} from 'src/integration/exchange/dto/scrypt.dto'; import { TradeChangedException } from 'src/integration/exchange/exceptions/trade-changed.exception'; -import { isTransientWsError } from 'src/integration/exchange/services/scrypt-websocket-connection'; +import { + isVenueRejection, + ScryptAmendRejectedError, + ScryptOrderNotFoundError, + ScryptUnconfirmedWriteError, +} from 'src/integration/exchange/services/scrypt-websocket-connection'; import { ScryptService } from 'src/integration/exchange/services/scrypt.service'; import { Asset } from 'src/shared/models/asset/asset.entity'; import { AssetService } from 'src/shared/models/asset/asset.service'; @@ -11,9 +21,10 @@ import { Util } from 'src/shared/utils/util'; import { DexService } from 'src/subdomains/supporting/dex/services/dex.service'; import { PriceValidity, PricingService } from 'src/subdomains/supporting/pricing/services/pricing.service'; import { LiquidityManagementOrder } from '../../entities/liquidity-management-order.entity'; -import { LiquidityManagementSystem } from '../../enums'; +import { LiquidityManagementSystem, UncertainOrderResolution } from '../../enums'; import { OrderFailedException } from '../../exceptions/order-failed.exception'; import { OrderNotProcessableException } from '../../exceptions/order-not-processable.exception'; +import { OrderOutcomeUnknownException } from '../../exceptions/order-outcome-unknown.exception'; import { Command, CorrelationId } from '../../interfaces'; import { LiquidityManagementOrderRepository } from '../../repositories/liquidity-management-order.repository'; import { LiquidityActionAdapter } from './base/liquidity-action.adapter'; @@ -24,6 +35,18 @@ export enum ScryptAdapterCommands { BUY = 'buy', } +/** Marks a reference as ours when reading Scrypt's own order/transaction history. */ +const SCRYPT_CORRELATION_PREFIX = 'dfx-lm-'; + +/** + * How long an acknowledged order may stay unobservable before it is quarantined rather than polled again. + * + * Matches the age at which the venue lookup itself gives up on finding an order, so both routes out of a + * silent order agree. Quarantine is not a verdict — the order is still not declared failed — it only moves it + * somewhere a human can act on. + */ +const SCRYPT_UNOBSERVABLE_QUARANTINE_MINUTES = 60; + @Injectable() export class ScryptAdapter extends LiquidityActionAdapter { private readonly logger = new DfxLogger(ScryptAdapter); @@ -95,7 +118,7 @@ export class ScryptAdapter extends LiquidityActionAdapter { order.outputAsset = token; try { - const response = await this.scryptService.withdrawFunds(token, amount, address); + const response = await this.scryptService.withdrawFunds(token, amount, address, undefined, order.correlationId); return response.id; } catch (e) { @@ -105,7 +128,7 @@ export class ScryptAdapter extends LiquidityActionAdapter { ); } - throw e; + throw this.classifySendOutcome(e, `withdrawal of ${amount} ${token} to ${address}`); } } @@ -175,7 +198,7 @@ export class ScryptAdapter extends LiquidityActionAdapter { order.outputAsset = targetAssetEntity.dexName; try { - return await this.scryptService.sell(tradeAsset, targetAssetEntity.dexName, amount); + return await this.scryptService.sell(tradeAsset, targetAssetEntity.dexName, amount, order.correlationId); } catch (e) { if (this.isBalanceTooLowError(e)) { throw new OrderNotProcessableException( @@ -183,7 +206,7 @@ export class ScryptAdapter extends LiquidityActionAdapter { ); } - throw e; + throw this.classifySendOutcome(e, `sell of ${amount} ${tradeAsset} to ${targetAssetEntity.dexName}`); } } @@ -204,6 +227,15 @@ export class ScryptAdapter extends LiquidityActionAdapter { } if (!withdrawal?.txHash) { + // No record at all, past the age at which the venue is considered to have lost it: we cannot tell + // whether this withdrawal happened, and the manual path only accepts quarantined orders, so leaving it + // here would mean no way out at all. A record WITHOUT a hash is different — that is an observation, the + // withdrawal is simply still in flight, and quarantining it would only bounce it back and forth. + if (!withdrawal && Util.minutesDiff(order.created) > SCRYPT_UNOBSERVABLE_QUARANTINE_MINUTES) + throw new OrderOutcomeUnknownException( + `Scrypt has no record of withdrawal ${correlationId} after more than ${SCRYPT_UNOBSERVABLE_QUARANTINE_MINUTES} minutes`, + ); + this.logger.verbose(`No withdrawal id for id ${correlationId} at ${this.scryptService.name} found`); return false; } @@ -229,8 +261,29 @@ export class ScryptAdapter extends LiquidityActionAdapter { } private async checkTradeCompletion(order: LiquidityManagementOrder, from: string, to: string): Promise { + // Before anything may write again: a previous pass may have had its replacement accepted and then failed + // to record it, leaving this row pointing at the predecessor the venue has already cancelled. Restarting + // from that predecessor would place a second order alongside the live replacement. + if (!(await this.adoptLiveReplacement(order))) + return this.waitOrQuarantine(order, 'has a claimed replacement that can be neither confirmed nor ruled out'); + + // The check may amend or restart the order, which creates a NEW venue order. Hand it a reference derived + // from the order row so that a replacement whose confirmation never arrives is still findable — without + // this, the reconciliation below could not cover the amend boundary even in principle. + const replacementClOrdId = this.nextCorrelationId(order); + try { - const isComplete = await this.scryptService.checkTrade(order.correlationId, from, to, order.created); + const isComplete = await this.scryptService.checkTrade( + order.correlationId, + from, + to, + order.created, + replacementClOrdId, + async () => { + order.recordSpentCorrelationId(replacementClOrdId); + await this.orderRepo.save(order); + }, + ); if (isComplete) { order.outputAmount = await this.aggregateTradeOutput(order); @@ -244,15 +297,148 @@ export class ScryptAdapter extends LiquidityActionAdapter { return false; } - if (isTransientWsError(e)) { - this.logger.warn(`Transient WS error checking order ${order.id}, will retry next tick: ${e.message}`); + // Write boundary FIRST. This check can amend or restart the order, and an unconfirmed write must + // quarantine — before the transient-error branch below, which is only ever safe for reads. Getting the + // order wrong here would let a dropped socket during an amend look like a harmless retry. + if (e instanceof ScryptUnconfirmedWriteError) { + throw new OrderOutcomeUnknownException( + `${e.message} (replacement reference ${e.reference ?? replacementClOrdId})`, + ); + } + + // The amend was refused, so nothing was created and the original order is still live. Note the spent + // reference — the venue will not accept it again — and carry on watching the original. + if (e instanceof ScryptAmendRejectedError) { + if (e.spentReference) { + order.recordSpentCorrelationId(e.spentReference); + await this.orderRepo.save(order); + } + this.logger.warn(`Scrypt refused the amend for order ${order.id}, continuing with the original`); return false; } - throw new OrderFailedException(e.message); + // The venue once acknowledged this order and now cannot find it. That is not a failure — it may have + // filled or been cancelled outside our view — so it goes to a human instead of releasing the rule. + if (e instanceof ScryptOrderNotFoundError) throw new OrderOutcomeUnknownException(e.message); + + // A rejection is a reply: the venue reached a verdict, so the order really did end. + if (isVenueRejection(e)) throw new OrderFailedException(e.message); + + // Anything else is a failure to OBSERVE an order that the venue has acknowledged and may still be + // working. Failing it here would let the rule open a second position against the same funds, so the + // order is kept and looked at again next tick. + // + // Hold it back only for a genuine blind spot, though. The failure may just as well have come from + // pricing or from aggregating the result — on an order the venue can still show us, and parking THAT + // would have reconciliation hand it straight back, only for the next check to park it again. + const stillObservable = await this.scryptService.getOrderStatus(order.correlationId).catch(() => null); + + if (!stillObservable) return this.waitOrQuarantine(order, `cannot be observed: ${e.message}`); + + this.logger.warn(`Could not check Scrypt order ${order.id}, will look again next tick: ${e.message}`); + return false; } } + /** + * Adopt a claimed replacement the venue has accepted but this row never recorded. + * + * The window is narrow — the venue accepted the replacement and the save that would have adopted it + * failed — but its consequence is not: the row still names the predecessor, the venue has cancelled that + * one, and the next check would happily restart from it while the replacement is live. + * + * Only references NEWER than the current one are candidates. A predecessor is not a replacement: after an + * amend that DID get recorded it sits in the list as cancelled, and adopting it would walk the row + * backwards and restart the very quantity the replacement is already working. + * + * Returns whether this order may be written to at all. A claim that can be neither confirmed nor ruled out + * is a barrier rather than something to step past — the reference is recorded BEFORE the request leaves, + * so one the venue does not show may still be live there, and carrying on with the predecessor would put a + * second request next to it. + */ + private async adoptLiveReplacement(order: LiquidityManagementOrder): Promise { + const currentAttempt = this.attemptNumber(order, order.correlationId); + const claimed = this.attemptedReferencesNewestFirst(order).filter( + (reference) => this.attemptNumber(order, reference) > currentAttempt, + ); + + for (const reference of claimed) { + // `null` means the venue does not show it, `undefined` that it could not be asked. Neither is a reply, + // and only a reply can establish that a claimed reference created nothing. + const info = await this.scryptService.getOrderStatus(reference).catch(() => undefined); + + if (info == null) { + this.logger.warn( + `Order ${order.id} claimed ${reference}, but the venue ${ + info === null ? 'does not show it' : 'could not be asked about it' + } — holding back every write against ${order.correlationId}`, + ); + + return false; + } + + // A rejection IS a reply: this claim created nothing, so an older one may still be the live order. + if (info.status === ScryptOrderStatus.REJECTED) continue; + + this.logger.warn( + `Order ${order.id} still named ${order.correlationId}, but the venue is working ${reference} — adopting it`, + ); + order.updateCorrelationId(reference); + await this.orderRepo.save(order); + + return true; + } + + return true; + } + + /** + * Hold an order back because something about it cannot be observed right now. + * + * Waiting is the safe answer — writing against an order whose true state is unknown is how a second + * request against the same funds happens. But not forever: the manual path only accepts quarantined + * orders, so an order nobody can ever observe would poll for good with no way out at all. Past the same + * age at which the venue itself is considered to have lost an order, it goes to a human instead — still + * not declared failed. + */ + private waitOrQuarantine(order: LiquidityManagementOrder, reason: string): boolean { + if (Util.minutesDiff(order.created) > SCRYPT_UNOBSERVABLE_QUARANTINE_MINUTES) + throw new OrderOutcomeUnknownException( + `Scrypt order ${order.id} ${reason}, and is over ${SCRYPT_UNOBSERVABLE_QUARANTINE_MINUTES} minutes old`, + ); + + this.logger.warn(`Scrypt order ${order.id} ${reason}, will look again next tick`); + + return false; + } + + /** + * Every reference this order has actually put on the wire, newest first. + * + * Ordered by the attempt suffix rather than by storage order, so it does not depend on how the list was + * assembled. Deliberately does NOT include the next reference: that one has not been sent, and looking for + * it would stop the search on an absence that means nothing — leaving the reference that WAS sent unchecked + * and the order quarantined for good. + */ + private attemptedReferencesNewestFirst(order: LiquidityManagementOrder): CorrelationId[] { + return [...order.allCorrelationIds].sort((a, b) => this.attemptNumber(order, b) - this.attemptNumber(order, a)); + } + + /** Which attempt a reference belongs to: the reserved one is 0, every replacement counts up from there. */ + private attemptNumber(order: LiquidityManagementOrder, reference: CorrelationId | undefined): number { + return Number(reference?.slice(`${SCRYPT_CORRELATION_PREFIX}${order.id}-`.length)) || 0; + } + + /** + * Reference for the next venue order this row may produce (an amend or a restart). + * + * Derived from the order id and the number of references already used, so it is reproducible from the row + * alone — no extra column, and no window in which a replacement exists that we cannot name. + */ + private nextCorrelationId(order: LiquidityManagementOrder): CorrelationId { + return `${SCRYPT_CORRELATION_PREFIX}${order.id}-${order.allCorrelationIds.length}`; + } + private async aggregateTradeOutput(order: LiquidityManagementOrder): Promise { const correlationIds = order.allCorrelationIds; @@ -357,14 +543,131 @@ export class ScryptAdapter extends LiquidityActionAdapter { order.outputAsset = toAsset; try { - return await this.scryptService.sell(fromAsset, toAsset, amount); + return await this.scryptService.sell(fromAsset, toAsset, amount, order.correlationId); } catch (e) { // No "(balance: ..., min. requested: ..., max. requested: ...)" suffix: balance/min/max are not in scope here. // The only production Scrypt 'sell' action has no onFail/onSuccess chain, so its error never reaches the liquidity-pipeline regex parser. if (this.isBalanceTooLowError(e)) { throw new OrderNotProcessableException(e.message); } - throw e; + throw this.classifySendOutcome(e, `sell of ${amount} ${fromAsset} to ${toAsset}`); + } + } + + /** + * Venue reference claimed before the request goes out. Scrypt is the one integration that lets us choose + * it (`ClOrdID`/`ClReqID`), so it is the one integration that can be reconciled after silence. + * + * Derived from the order id, which never repeats — this satisfies the venue's "unique daily, below 36 + * characters" requirement without a random component, so the reference is reproducible from the row alone. + */ + reserveCorrelationId(order: LiquidityManagementOrder): CorrelationId { + return `${SCRYPT_CORRELATION_PREFIX}${order.id}`; + } + + /** + * Decide whether a failed write demonstrably never reached the venue, or whether its outcome is unknown. + * + * Fail-closed, like `toBroadcastBoundaryError` in the payout subdomain: only silence-free evidence lets an + * error stay an ordinary failure. A timeout means the venue may already have executed, so it becomes an + * unknown outcome and the order is quarantined rather than repeated. + */ + private classifySendOutcome(e: Error, description: string): Error { + // Only a reply from the venue proves what happened to the request. A rejection means it was seen and + // refused — an ordinary failure, safe to let the rule plan again. + if (isVenueRejection(e)) return e; + + // Everything else is silence, and silence is not evidence. A timeout is obvious, but a dropped socket is + // just as ambiguous: once `ws.send` has run the bytes may already be on the wire, and the close that + // follows rejects the pending request with a generic message that says nothing about whether the venue + // acted on them. Both become unknown outcomes. + // + // Over-classifying costs an operator a look at the venue; under-classifying is what moved money without + // a record. Since absence at the venue is not proof, such an order waits for a human rather than + // resolving itself — deliberately the expensive direction, because the cheap one is the dangerous one. + return new OrderOutcomeUnknownException(`Scrypt gave no confirmed outcome for the ${description}: ${e.message}`); + } + + /** + * Ask Scrypt what happened to a quarantined order. Observes only — never re-sends. + * + * Can only ever confirm a positive: Scrypt has no terminal "this reference was never accepted" reply, so + * a missing record leaves the order quarantined for a human rather than releasing its rule. + */ + async resolveUncertainOrder(order: LiquidityManagementOrder): Promise { + const { correlationId } = order; + if (!correlationId) return UncertainOrderResolution.UNRESOLVED; + + let allAttemptsRejected = false; + + try { + if (order.action.command === ScryptAdapterCommands.WITHDRAW) { + const withdrawal = await this.scryptService.findWithdrawal(correlationId); + if (withdrawal) { + this.logger.info(`Scrypt confirmed reference ${correlationId} exists; order ${order.id} was sent`); + return UncertainOrderResolution.SENT; + } + } else { + // Newest first. A replacement supersedes the order it replaced, and the replaced one usually still + // exists at the venue in a cancelled state — checking oldest first would match that, report SENT and + // leave the live replacement untracked while the completion check polls a superseded reference. + const candidates = this.attemptedReferencesNewestFirst(order); + let rejectedCount = 0; + + for (const candidate of candidates) { + const info = await this.scryptService.getOrderStatus(candidate); + + // Absent, newest first: an accepted replacement may simply not be visible yet, while the order it + // replaced still is. Falling through to that predecessor would report SENT on a reference the venue + // has already superseded and leave the live replacement untracked, so stop here instead. + if (!info) { + this.logger.warn( + `Scrypt does not (yet) know reference ${candidate} for order ${order.id} — keeping it quarantined`, + ); + return UncertainOrderResolution.UNRESOLVED; + } + + // A refused replacement never took effect and leaves its predecessor live. This is the only case in + // which an older reference may be considered. + if (info.status === ScryptOrderStatus.REJECTED) { + order.recordSpentCorrelationId(candidate); + rejectedCount++; + continue; + } + + // Track the reference the venue actually knows. + if (candidate !== order.correlationId) order.updateCorrelationId(candidate); + + this.logger.info(`Scrypt confirmed reference ${candidate} exists; order ${order.id} was sent`); + return UncertainOrderResolution.SENT; + } + + allAttemptsRejected = candidates.length > 0 && rejectedCount === candidates.length; + } + + // Every reference this order put on the wire came back rejected. Nothing was ever created, so unlike + // mere absence this IS a definitive negative — and leaving it unresolved would query a settled outcome + // forever while the rule stays blocked. + if (allAttemptsRejected) { + this.logger.info(`Scrypt rejected every reference of order ${order.id}; nothing was executed`); + return UncertainOrderResolution.NOT_SENT; + } + + // Absence is NOT proof. A snapshot without the reference may simply predate the venue registering it, + // and Scrypt offers no terminal "this was never accepted" acknowledgement to rely on. Concluding + // otherwise is what would let the rule reissue a request that later materialises — so the order stays + // quarantined for a human, and the rule stays blocked, which is the safe direction. + this.logger.warn( + `Scrypt still has no record of reference ${correlationId} for order ${order.id} — keeping it quarantined`, + ); + return UncertainOrderResolution.UNRESOLVED; + } catch (e) { + // The lookup travels the same connection that just went silent. An unreachable venue is not evidence + // of anything — stay in quarantine rather than guess in either direction. Reported as UNAVAILABLE and + // not UNRESOLVED, because no question was actually put to the venue: the caller uses that difference + // to decide whether the order still owes a look. + this.logger.warn(`Could not resolve uncertain Scrypt order ${order.id}: ${e.message}`); + return UncertainOrderResolution.UNAVAILABLE; } } diff --git a/src/subdomains/core/liquidity-management/adapters/balances/custom.adapter.ts b/src/subdomains/core/liquidity-management/adapters/balances/custom.adapter.ts index d62038d0b3..c9308599ab 100644 --- a/src/subdomains/core/liquidity-management/adapters/balances/custom.adapter.ts +++ b/src/subdomains/core/liquidity-management/adapters/balances/custom.adapter.ts @@ -41,7 +41,14 @@ export class CustomAdapter implements LiquidityBalanceIntegration { this.exchangeRegistry.get('Binance').getAvailableBalance(asset.name), this.orderRepo.sum('inputAmount', { action: { system: In([LiquidityManagementSystem.KRAKEN, LiquidityManagementSystem.BINANCE]) }, - status: In([LiquidityManagementOrderStatus.CREATED, LiquidityManagementOrderStatus.IN_PROGRESS]), + // UNCERTAIN too: the generic quarantine in startNewOrders is not Scrypt-specific, so a + // Kraken/Binance order can land there — and neither adapter can resolve it, so it would stay + // put and silently drop its amount out of this balance for as long as it remains unresolved. + status: In([ + LiquidityManagementOrderStatus.CREATED, + LiquidityManagementOrderStatus.IN_PROGRESS, + LiquidityManagementOrderStatus.UNCERTAIN, + ]), inputAsset: asset.name, }), ]); diff --git a/src/subdomains/core/liquidity-management/adapters/balances/exchange.adapter.ts b/src/subdomains/core/liquidity-management/adapters/balances/exchange.adapter.ts index 10a86d9a68..f09db1605d 100644 --- a/src/subdomains/core/liquidity-management/adapters/balances/exchange.adapter.ts +++ b/src/subdomains/core/liquidity-management/adapters/balances/exchange.adapter.ts @@ -40,7 +40,14 @@ export class ExchangeAdapter implements LiquidityBalanceIntegration { const system = Object.values(LiquidityManagementSystem).find((s) => s.toString() === context.toString()); const query = { action: { system }, - status: In([LiquidityManagementOrderStatus.CREATED, LiquidityManagementOrderStatus.IN_PROGRESS]), + // UNCERTAIN counts as pending: the venue may be holding or may already have moved these funds. This + // gate is what stops a sibling rule on the same exchange from acting on a balance whose true state is + // still unknown, so an unresolved order has to block here just as a running one does. + status: In([ + LiquidityManagementOrderStatus.CREATED, + LiquidityManagementOrderStatus.IN_PROGRESS, + LiquidityManagementOrderStatus.UNCERTAIN, + ]), }; return system diff --git a/src/subdomains/core/liquidity-management/controllers/order.controller.ts b/src/subdomains/core/liquidity-management/controllers/order.controller.ts index 869c3524bd..f49acd3910 100644 --- a/src/subdomains/core/liquidity-management/controllers/order.controller.ts +++ b/src/subdomains/core/liquidity-management/controllers/order.controller.ts @@ -1,9 +1,12 @@ -import { Controller, Get, UseGuards } from '@nestjs/common'; +import { Body, Controller, Get, Param, ParseIntPipe, Put, UseGuards } from '@nestjs/common'; import { AuthGuard } from '@nestjs/passport'; import { ApiBearerAuth, ApiExcludeEndpoint, ApiTags } from '@nestjs/swagger'; +import { GetJwt } from 'src/shared/auth/get-jwt.decorator'; +import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; import { RoleGuard } from 'src/shared/auth/role.guard'; import { UserActiveGuard } from 'src/shared/auth/user-active.guard'; import { UserRole } from 'src/shared/auth/user-role.enum'; +import { ResolveUncertainOrderDto } from '../dto/resolve-uncertain-order.dto'; import { LiquidityManagementOrder } from '../entities/liquidity-management-order.entity'; import { LiquidityManagementPipelineService } from '../services/liquidity-management-pipeline.service'; @@ -19,4 +22,16 @@ export class LiquidityManagementOrderController { async getProcessingOrders(): Promise { return this.service.getProcessingOrders(); } + + @Put(':id/resolveUncertain') + @ApiBearerAuth() + @ApiExcludeEndpoint() + @UseGuards(AuthGuard(), RoleGuard(UserRole.ADMIN), UserActiveGuard()) + async resolveUncertainOrder( + @GetJwt() jwt: JwtPayload, + @Param('id', ParseIntPipe) id: number, + @Body() dto: ResolveUncertainOrderDto, + ): Promise { + return this.service.resolveUncertainOrderManually(id, dto, jwt.account); + } } diff --git a/src/subdomains/core/liquidity-management/dto/__tests__/resolve-uncertain-order.dto.spec.ts b/src/subdomains/core/liquidity-management/dto/__tests__/resolve-uncertain-order.dto.spec.ts new file mode 100644 index 0000000000..4dcd8045f5 --- /dev/null +++ b/src/subdomains/core/liquidity-management/dto/__tests__/resolve-uncertain-order.dto.spec.ts @@ -0,0 +1,40 @@ +import { plainToInstance } from 'class-transformer'; +import { validate } from 'class-validator'; +import { ResolveUncertainOrderDto } from '../resolve-uncertain-order.dto'; + +// This DTO carries the record of a decision to release an order whose outcome the venue could not confirm: +// an assertion that somebody checked, and where. Both are read later by whoever has to reconstruct why the +// release was allowed, so the input has to survive transformation as written. +describe('ResolveUncertainOrderDto', () => { + const validateDto = async (raw: Record) => validate(plainToInstance(ResolveUncertainOrderDto, raw)); + + it('keeps the words of a verification reference intact', async () => { + const instance = plainToInstance(ResolveUncertainOrderDto, { + noExecutionVerified: true, + verificationReference: ' venue console, ticket OPS-42 ', + }); + + // trimmed at the edges, untouched inside — a reference stripped of its spaces is no longer the evidence + expect(instance.verificationReference).toBe('venue console, ticket OPS-42'); + }); + + it('accepts a verified claim with a reference', async () => { + expect( + await validateDto({ noExecutionVerified: true, verificationReference: 'venue console, ticket OPS-42' }), + ).toEqual([]); + }); + + it('rejects a claim that does not assert the check was made', async () => { + const errors = await validateDto({ noExecutionVerified: false, verificationReference: 'checked' }); + + expect(errors).toHaveLength(1); + expect(errors[0].property).toBe('noExecutionVerified'); + }); + + it('rejects a reference that is only whitespace', async () => { + const errors = await validateDto({ noExecutionVerified: true, verificationReference: ' ' }); + + expect(errors).toHaveLength(1); + expect(errors[0].property).toBe('verificationReference'); + }); +}); diff --git a/src/subdomains/core/liquidity-management/dto/resolve-uncertain-order.dto.ts b/src/subdomains/core/liquidity-management/dto/resolve-uncertain-order.dto.ts new file mode 100644 index 0000000000..0387f39575 --- /dev/null +++ b/src/subdomains/core/liquidity-management/dto/resolve-uncertain-order.dto.ts @@ -0,0 +1,40 @@ +import { ApiProperty } from '@nestjs/swagger'; +import { Transform } from 'class-transformer'; +import { IsBoolean, IsIn, IsNotEmpty, IsString, MaxLength, MinLength } from 'class-validator'; +import { Util } from 'src/shared/utils/util'; + +/** + * Manual release of an order whose outcome the venue could not confirm. + * + * Accepted rather than executed: the order stays quarantined until reconciliation has had one answer from + * the venue, so a release can never end an order while a confirmation of it is still in flight. In the + * ordinary case that is the next pass, seconds later. Two exceptions, both about liveness: an order no + * integration can look up any more, and a venue that has answered nothing for long enough. There the release + * takes effect on this assertion alone — silence stops being a veto, it never becomes evidence. + * + * The automatic reconciliation can only ever prove the positive — that the venue knows the reference. It + * never concludes the negative, because no venue reply establishes "this was never accepted". Somebody has + * to look, and this is where that judgement is recorded. + * + * Modelled on the payout subdomain's guarded retry: an explicit assertion plus a verifiable reference, so + * the decision is deliberate and auditable rather than a status flip. + */ +export class ResolveUncertainOrderDto { + @ApiProperty({ + description: + 'Assertion that the venue was checked directly and the request demonstrably never took effect. ' + + 'Required, and required to be true — releasing an order whose outcome is still open risks executing it twice.', + }) + @IsNotEmpty() + @IsBoolean() + @IsIn([true], { message: 'noExecutionVerified must be true — an unverified order must stay quarantined' }) + noExecutionVerified: boolean; + + @ApiProperty({ description: 'Where that was checked, so the decision can be audited later.' }) + @IsNotEmpty() + @IsString() + @Transform(Util.trim) + @MinLength(3) + @MaxLength(1024) + verificationReference: string; +} diff --git a/src/subdomains/core/liquidity-management/entities/__tests__/liquidity-management-order.entity.spec.ts b/src/subdomains/core/liquidity-management/entities/__tests__/liquidity-management-order.entity.spec.ts new file mode 100644 index 0000000000..3e7995c837 --- /dev/null +++ b/src/subdomains/core/liquidity-management/entities/__tests__/liquidity-management-order.entity.spec.ts @@ -0,0 +1,59 @@ +import { LiquidityManagementOrderStatus } from '../../enums'; +import { LiquidityManagementOrder } from '../liquidity-management-order.entity'; + +const minutesAgo = (minutes: number): Date => new Date(Date.now() - minutes * 60 * 1000); + +describe('LiquidityManagementOrder', () => { + describe('releaseWaitedOutVenue', () => { + function released(at?: Date): LiquidityManagementOrder { + return Object.assign(new LiquidityManagementOrder(), { + status: LiquidityManagementOrderStatus.UNCERTAIN, + notSentRecheckDue: at, + }); + } + + it('is false while no release is pending at all', () => { + expect(released(undefined).releaseWaitedOutVenue()).toBe(false); + expect(released(null).releaseWaitedOutVenue()).toBe(false); + }); + + it('is false just inside the wait', () => { + expect(released(minutesAgo(59)).releaseWaitedOutVenue()).toBe(false); + }); + + it('is true once the wait has been exceeded', () => { + expect(released(minutesAgo(61)).releaseWaitedOutVenue()).toBe(true); + }); + }); + + describe('resolveAsSent / resolveAsNotSent / requestNotSentRelease', () => { + it('accepts a release without acting on it: the order keeps blocking', () => { + const order = Object.assign(new LiquidityManagementOrder(), { + status: LiquidityManagementOrderStatus.UNCERTAIN, + }).requestNotSentRelease('checked by hand'); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(order.notSentRecheckDue).toBeInstanceOf(Date); + }); + + it('drops the pending release when the order turns out to have been sent', () => { + const order = Object.assign(new LiquidityManagementOrder(), { + status: LiquidityManagementOrderStatus.UNCERTAIN, + notSentRecheckDue: minutesAgo(5), + }).resolveAsSent(); + + expect(order.status).toBe(LiquidityManagementOrderStatus.IN_PROGRESS); + expect(order.notSentRecheckDue).toBeNull(); + }); + + it('drops it when the release is put into effect, so nothing is owed afterwards', () => { + const order = Object.assign(new LiquidityManagementOrder(), { + status: LiquidityManagementOrderStatus.UNCERTAIN, + notSentRecheckDue: minutesAgo(5), + }).resolveAsNotSent('released'); + + expect(order.status).toBe(LiquidityManagementOrderStatus.FAILED); + expect(order.notSentRecheckDue).toBeNull(); + }); + }); +}); diff --git a/src/subdomains/core/liquidity-management/entities/liquidity-management-order.entity.ts b/src/subdomains/core/liquidity-management/entities/liquidity-management-order.entity.ts index 2b03d7a6c3..2c16e613dc 100644 --- a/src/subdomains/core/liquidity-management/entities/liquidity-management-order.entity.ts +++ b/src/subdomains/core/liquidity-management/entities/liquidity-management-order.entity.ts @@ -1,14 +1,24 @@ import { Active } from 'src/shared/models/active'; -import { IEntity } from 'src/shared/models/entity'; import { baseUnitsTransformer } from 'src/shared/models/base-units.transformer'; +import { IEntity } from 'src/shared/models/entity'; +import { Util } from 'src/shared/utils/util'; import { Price, PriceStep } from 'src/subdomains/supporting/pricing/domain/entities/price'; import { Column, Entity, Index, JoinTable, ManyToOne } from 'typeorm'; import { LiquidityManagementOrderStatus } from '../enums'; import { OrderFailedException } from '../exceptions/order-failed.exception'; import { OrderNotProcessableException } from '../exceptions/order-not-processable.exception'; +import { OrderOutcomeUnknownException } from '../exceptions/order-outcome-unknown.exception'; import { LiquidityManagementAction } from './liquidity-management-action.entity'; import { LiquidityManagementPipeline } from './liquidity-management-pipeline.entity'; +/** + * How long a release waits for a venue that cannot be reached before taking effect anyway. + * + * A liveness bound, not a safety one. Nothing is concluded from the silence: the person who released the + * order concluded it, and this only stops an unreachable venue from vetoing them forever. + */ +const RELEASE_WITHOUT_VENUE_MINUTES = 60; + @Entity() export class LiquidityManagementOrder extends IEntity { @Column({ length: 256, nullable: false }) @@ -56,6 +66,29 @@ export class LiquidityManagementOrder extends IEntity { @Column({ type: 'int', nullable: true }) previousOrderId?: number; + /** + * Set when somebody has released this order as never sent, and cleared once the venue has been asked once + * more. While it stands, the order STAYS QUARANTINED — the release is accepted but not yet in effect. + * + * A judgement that a request never left is the one conclusion nothing here can verify from the outside, + * and it is made at the same moment reconciliation may be watching the venue confirm that very order. If + * the release took effect immediately, that order would be terminal — its rule free to plan against funds + * that are in fact committed — before anything could contradict it. So it waits for one machine answer, + * which normally arrives on the next pass, seconds later. + * + * Two exceptions, both about liveness rather than safety, and neither concluding anything from silence: + * an order no integration can look up any more never gets an answer, and a venue that has been unreachable + * for `RELEASE_WITHOUT_VENUE_MINUTES` is not going to give one. There the release takes effect on the + * operator's assertion — which is what it was checked for. Silence stops being a veto; it never becomes + * evidence. + * + * A marker for work outstanding, NOT a record of when the release was asked for: that goes into the + * order's reason, which nothing clears. Indexed so that finding these few rows is never a scan. + */ + @Index() + @Column({ type: 'timestamp', nullable: true }) + notSentRecheckDue?: Date | null; + @Column({ type: 'text', nullable: true }) correlationId?: string; @@ -110,6 +143,36 @@ export class LiquidityManagementOrder extends IEntity { return [...new Set(ids)].filter((id) => id); } + /** + * Claim the venue-side reference BEFORE the request goes out, without advancing the status. + * + * The order stays CREATED — it has not been sent yet — but the reference is now durable, so an + * un-acknowledged request can still be looked up afterwards. Without this, an id generated inside the + * integration and only returned on success is lost exactly when it is needed. Mirrors the reservation + * that fiat-output performs against Bank Frick before transmitting a payment order. + */ + reserveCorrelationId(correlationId: string): this { + this.correlationId = correlationId; + + return this; + } + + /** + * Note a reference an attempt has consumed at the venue without adopting it as the current one. + * + * A rejected amend still burns its reference — the venue requires them to be unique — so the next attempt + * must pick a fresh one. Since the next reference is derived from how many this order has used, recording + * the spent one here is what makes that derivation advance instead of repeating itself. + */ + recordSpentCorrelationId(spent: string): this { + if (!this.allCorrelationIds.includes(spent)) + this.previousCorrelationIds = [...this.allCorrelationIds.filter((id) => id !== this.correlationId), spent].join( + ',', + ); + + return this; + } + inProgress(correlationId: string): this { this.correlationId = correlationId; this.status = LiquidityManagementOrderStatus.IN_PROGRESS; @@ -143,4 +206,52 @@ export class LiquidityManagementOrder extends IEntity { return this; } + + /** Quarantine an order whose request left our side without an observed outcome. */ + uncertain(error: OrderOutcomeUnknownException): this { + this.status = LiquidityManagementOrderStatus.UNCERTAIN; + this.errorMessage = error.message; + + return this; + } + + /** The venue confirmed it knows this order: leave quarantine and let the normal completion check take over. */ + resolveAsSent(): this { + this.status = LiquidityManagementOrderStatus.IN_PROGRESS; + this.notSentRecheckDue = null; + + return this; + } + + /** The venue demonstrably never received this order: nothing was executed, so it is a plain failure. */ + resolveAsNotSent(reason: string): this { + this.status = LiquidityManagementOrderStatus.FAILED; + this.errorMessage = reason; + this.notSentRecheckDue = null; + + return this; + } + + /** + * Whether a pending release has waited out a venue that answers nothing. + * + * The wait exists to catch a confirmation that is in flight right now. After this long there is none in + * flight, only an operator who checked and is being ignored — so silence stops vetoing them. + */ + releaseWaitedOutVenue(): boolean { + return this.notSentRecheckDue != null && Util.minutesDiff(this.notSentRecheckDue) > RELEASE_WITHOUT_VENUE_MINUTES; + } + + /** + * Accept somebody's judgement that this order never left — without acting on it yet. + * + * The order stays quarantined until the venue has been asked one more time, so a release can never make an + * order terminal while a confirmation of it is still in flight. + */ + requestNotSentRelease(reason: string): this { + this.errorMessage = reason; + this.notSentRecheckDue = new Date(); + + return this; + } } diff --git a/src/subdomains/core/liquidity-management/enums/index.ts b/src/subdomains/core/liquidity-management/enums/index.ts index 8a7b0af4bd..edddb69185 100644 --- a/src/subdomains/core/liquidity-management/enums/index.ts +++ b/src/subdomains/core/liquidity-management/enums/index.ts @@ -39,6 +39,28 @@ export enum LiquidityManagementOrderStatus { COMPLETE = 'Complete', NOT_PROCESSABLE = 'NotProcessable', FAILED = 'Failed', + // Quarantine for an order whose request left our side without an observed outcome. Terminal for the + // pipeline (it never resumes on its own) but not for the order: `resolveUncertainOrders` asks the venue + // what happened and moves it on to IN_PROGRESS or FAILED. See OrderOutcomeUnknownException. + UNCERTAIN = 'Uncertain', +} + +/** Outcome of asking a venue what happened to an order that ended in {@link LiquidityManagementOrderStatus.UNCERTAIN}. */ +export enum UncertainOrderResolution { + /** The venue knows the order — it was sent. Hand it back to the normal completion check. */ + SENT = 'Sent', + /** The venue demonstrably does not know the order — nothing was executed, the rule may plan anew. */ + NOT_SENT = 'NotSent', + /** The venue answered, and the answer settles nothing. Stay in quarantine and look again later. */ + UNRESOLVED = 'Unresolved', + /** + * The venue could not be asked at all. + * + * Deliberately not the same as UNRESOLVED: that one is an answer, this one is the absence of one, and a + * caller that retires an order's outstanding work on the strength of a completed lookup must not retire it + * on a failed one. + */ + UNAVAILABLE = 'Unavailable', } export enum LiquidityManagementPipelineStatus { diff --git a/src/subdomains/core/liquidity-management/exceptions/order-outcome-unknown.exception.ts b/src/subdomains/core/liquidity-management/exceptions/order-outcome-unknown.exception.ts new file mode 100644 index 0000000000..7c599c6094 --- /dev/null +++ b/src/subdomains/core/liquidity-management/exceptions/order-outcome-unknown.exception.ts @@ -0,0 +1,18 @@ +/** + * The request left our side but we never observed its outcome — the venue may or may not have acted on it. + * + * This is deliberately NOT `OrderFailedException`. `Failed` asserts knowledge ("it demonstrably did not + * happen") and, because a failed pipeline pauses its rule and the rule auto-reactivates after + * `reactivationTime`, it also means "try the same thing again in a few minutes". Applying that to an + * ambiguous outcome is how a single un-acknowledged request turns into a double execution. + * + * Orders that end here are quarantined in `LiquidityManagementOrderStatus.UNCERTAIN` and resolved by + * asking the venue what actually happened — never by repeating the request. Mirrors the payout + * subdomain's `PayoutBroadcastException` / `PAYOUT_UNCERTAIN` pair, which solves the same problem for + * blockchain broadcasts. + */ +export class OrderOutcomeUnknownException extends Error { + constructor(message: string) { + super(message); + } +} diff --git a/src/subdomains/core/liquidity-management/interfaces/index.ts b/src/subdomains/core/liquidity-management/interfaces/index.ts index 54e4cc28f2..fed3e41c52 100644 --- a/src/subdomains/core/liquidity-management/interfaces/index.ts +++ b/src/subdomains/core/liquidity-management/interfaces/index.ts @@ -2,7 +2,7 @@ import { Active } from 'src/shared/models/active'; import { Asset } from 'src/shared/models/asset/asset.entity'; import { LiquidityBalance } from '../entities/liquidity-balance.entity'; import { LiquidityManagementOrder } from '../entities/liquidity-management-order.entity'; -import { LiquidityManagementContext, LiquidityOptimizationType } from '../enums'; +import { LiquidityManagementContext, LiquidityOptimizationType, UncertainOrderResolution } from '../enums'; export type CorrelationId = string; export type PipelineId = number; @@ -19,6 +19,20 @@ export interface LiquidityActionIntegration { executeOrder(order: LiquidityManagementOrder): Promise; checkCompletion(order: LiquidityManagementOrder): Promise; validateParams(command: string, params: Record): boolean; + + /** + * Venue-side reference to claim before the request is sent, so an un-acknowledged request stays + * traceable. Optional: integrations that derive their correlationId from the venue's response (or encode + * their own state into it) omit this and keep the existing behaviour. + */ + reserveCorrelationId?(order: LiquidityManagementOrder): CorrelationId; + + /** + * Ask the venue what happened to an order quarantined as UNCERTAIN. Must never re-send the request — + * it may only observe. Integrations that cannot look an order up omit this; their orders stay in + * quarantine for a human to resolve. + */ + resolveUncertainOrder?(order: LiquidityManagementOrder): Promise; } export interface LiquidityState { diff --git a/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.spec.ts b/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.spec.ts index 9dc1f52c28..8eb67c322d 100644 --- a/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.spec.ts +++ b/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.spec.ts @@ -3,7 +3,14 @@ import { NotificationService } from 'src/subdomains/supporting/notification/serv import { LiquidityManagementOrder } from '../entities/liquidity-management-order.entity'; import { LiquidityManagementPipeline } from '../entities/liquidity-management-pipeline.entity'; import { LiquidityManagementRule } from '../entities/liquidity-management-rule.entity'; -import { LiquidityManagementPipelineStatus, LiquidityManagementRuleStatus, LiquidityOptimizationType } from '../enums'; +import { + LiquidityManagementOrderStatus, + LiquidityManagementPipelineStatus, + LiquidityManagementRuleStatus, + LiquidityOptimizationType, + UncertainOrderResolution, +} from '../enums'; +import { OrderOutcomeUnknownException } from '../exceptions/order-outcome-unknown.exception'; import { LiquidityActionIntegrationFactory } from '../factories/liquidity-action-integration.factory'; import { LiquidityManagementOrderRepository } from '../repositories/liquidity-management-order.repository'; import { LiquidityManagementPipelineRepository } from '../repositories/liquidity-management-pipeline.repository'; @@ -38,6 +45,664 @@ describe('LiquidityManagementPipelineService', () => { ); }); + describe('startNewOrders — unknown outcomes', () => { + function createdOrder(id = 7): LiquidityManagementOrder { + return Object.assign(new LiquidityManagementOrder(), { + id, + status: LiquidityManagementOrderStatus.CREATED, + action: { id: 233, system: 'Scrypt', command: 'sell' }, + }); + } + + it('quarantines an order as UNCERTAIN instead of failing it when the outcome is unknown', async () => { + const order = createdOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn().mockRejectedValue(new OrderOutcomeUnknownException('Scrypt did not answer')), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + }); + + await service['startNewOrders'](); + + // FAILED would pause the rule, and the rule auto-reactivates — i.e. it would repeat a request that + // may already have executed. That is the exact path that mis-booked two live withdrawals. + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(order.status).not.toBe(LiquidityManagementOrderStatus.FAILED); + expect(notificationService.sendMail).toHaveBeenCalled(); + }); + + it('fails — not quarantines — an unclassified error when no reference was ever reserved', async () => { + const order = createdOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + // no reserveCorrelationId, so nothing can have been transmitted + executeOrder: jest.fn().mockRejectedValue(new Error('no integration configured')), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + }); + + const anyChanged = await service['startNewOrders'](); + + // quarantining a provably-never-sent request would strand config errors in a human-only state + expect(order.status).toBe(LiquidityManagementOrderStatus.FAILED); + // and it must still leave CREATED, or the caller's `while (hasChanges)` loop cannot terminate + expect(anyChanged).toBe(true); + }); + + it('quarantines an unclassified error once a reference was reserved', async () => { + const order = createdOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + reserveCorrelationId: () => 'dfx-lm-7', + executeOrder: jest.fn().mockRejectedValue(new Error('socket exploded mid-send')), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + }); + + const anyChanged = await service['startNewOrders'](); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(anyChanged).toBe(true); + }); + + it('never re-sends a CREATED order that already carries a reserved reference', async () => { + // that combination means a previous pass reached the send boundary and died before recording the + // result — re-sending is the one thing that could duplicate a live request + const order = createdOrder(); + order.correlationId = 'dfx-lm-7'; + const executeOrder = jest.fn(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + reserveCorrelationId: () => 'dfx-lm-7', + executeOrder, + checkCompletion: jest.fn(), + validateParams: jest.fn(), + }); + + await service['startNewOrders'](); + + expect(executeOrder).not.toHaveBeenCalled(); + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(notificationService.sendMail).toHaveBeenCalled(); + }); + + it('persists the reserved correlation id BEFORE the request is sent', async () => { + const order = createdOrder(4711); + const saveOrder: string[] = []; + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(orderRepo, 'save').mockImplementation(async (o: LiquidityManagementOrder) => { + saveOrder.push(`save:${o.status}:${o.correlationId}`); + return o; + }); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + reserveCorrelationId: () => 'dfx-lm-4711', + executeOrder: jest.fn().mockImplementation(async () => { + saveOrder.push('send'); + return 'dfx-lm-4711'; + }), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + }); + + await service['startNewOrders'](); + + // the reference must be durable before the request leaves — otherwise a timeout loses it for good + expect(saveOrder).toEqual(['save:Created:dfx-lm-4711', 'send', 'save:InProgress:dfx-lm-4711']); + }); + }); + + describe('resolveUncertainOrders', () => { + function uncertainOrder(overrides: Partial = {}): LiquidityManagementOrder { + return Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + correlationId: 'dfx-lm-9', + errorMessage: 'Scrypt did not answer', + action: { id: 233, system: 'Scrypt', command: 'sell' }, + ...overrides, + }); + } + + /** An order somebody has released as never sent — accepted, but not yet in effect. */ + const RELEASED_AT = new Date(Date.now() - 5 * 60 * 1000); + + function releasePendingOrder(releasedAt = RELEASED_AT): LiquidityManagementOrder { + return uncertainOrder({ + errorMessage: 'Scrypt did not answer (released by account 42: venue checked — ticket OPS-42)', + notSentRecheckDue: releasedAt, + }); + } + + function stubIntegration(resolution: UncertainOrderResolution): void { + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn(), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + resolveUncertainOrder: jest.fn().mockResolvedValue(resolution), + }); + } + + it('only ever asks about quarantined orders', async () => { + const findBy = jest.spyOn(orderRepo, 'findBy').mockResolvedValue([]); + + await service['resolveUncertainOrders'](); + + expect(findBy).toHaveBeenCalledWith({ status: LiquidityManagementOrderStatus.UNCERTAIN }); + }); + + it.each([ + [UncertainOrderResolution.SENT, LiquidityManagementOrderStatus.IN_PROGRESS], + [UncertainOrderResolution.NOT_SENT, LiquidityManagementOrderStatus.FAILED], + [UncertainOrderResolution.UNRESOLVED, LiquidityManagementOrderStatus.UNCERTAIN], + [UncertainOrderResolution.UNAVAILABLE, LiquidityManagementOrderStatus.UNCERTAIN], + ])('moves an unreleased order on %s -> %s', async (resolution, expectedStatus) => { + const order = uncertainOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(resolution); + + await service['resolveUncertainOrders'](); + + expect(order.status).toBe(expectedStatus); + }); + + it('keeps a released order quarantined until the venue has actually answered', async () => { + // the release is a judgement, and while it is unconfirmed the order must not become terminal — + // a terminal order lets its rule plan again against funds that may well be committed + const order = releasePendingOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + stubIntegration(UncertainOrderResolution.UNAVAILABLE); + + await service['resolveUncertainOrders'](); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(orderRepo.update).not.toHaveBeenCalled(); + }); + + it('puts a release into effect once the venue confirms it has no record either', async () => { + const order = releasePendingOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + const update = jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.UNRESOLVED); + + await expect(service['resolveUncertainOrders']()).resolves.toBe(true); + + // the write is what counts, not the in-memory entity — and it is guarded on the exact release examined + expect(update).toHaveBeenCalledWith( + { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + notSentRecheckDue: RELEASED_AT, + }, + expect.objectContaining({ status: LiquidityManagementOrderStatus.FAILED, notSentRecheckDue: null }), + ); + // the operator and their reference survive, and the release is dated + expect(update.mock.calls[0][1].errorMessage).toContain('OPS-42'); + expect(update.mock.calls[0][1].errorMessage).toMatch(/released \d{4}-\d{2}-\d{2}T/); + }); + + it('cannot end an order on a release written after the one it examined', async () => { + // a newer release has a confirmation of its own outstanding; ending the order on the older reading + // would skip it, and ending an order is the one step nothing here can take back + const order = releasePendingOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + const update = jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 0, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.UNRESOLVED); + + await expect(service['resolveUncertainOrders']()).resolves.toBe(false); + + expect(update.mock.calls[0][0]).toMatchObject({ notSentRecheckDue: RELEASED_AT }); + }); + + it('does not release an unreleased order on the same inconclusive answer', async () => { + // absence is not proof; without somebody having checked independently there is only one negative + const order = uncertainOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + stubIntegration(UncertainOrderResolution.UNRESOLVED); + + await service['resolveUncertainOrders'](); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + }); + + it('lets a verified release through once the venue has been unreachable for an hour', async () => { + // a check nobody can perform must not hold an order somebody has verified by hand out of reach for + // good — nothing is concluded from the silence, the person who released it concluded it + const order = releasePendingOrder(new Date(Date.now() - 120 * 60 * 1000)); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.UNAVAILABLE); + + await expect(service['resolveUncertainOrders']()).resolves.toBe(true); + + expect(order.status).toBe(LiquidityManagementOrderStatus.FAILED); + expect(order.errorMessage).toContain('could not be reached'); + }); + + it('overrules a pending release the moment the venue confirms the order', async () => { + const order = releasePendingOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + + expect(order.status).toBe(LiquidityManagementOrderStatus.IN_PROGRESS); + expect(order.notSentRecheckDue).toBeNull(); + }); + + it('puts a release into effect when no integration can ever look the order up', async () => { + // the documented exception: waiting on an answer that can never come would quarantine it for good + const order = releasePendingOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + const update = jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue(null); + + // and the pass reports the change, so the caller's loop knows something moved + await expect(service['resolveUncertainOrders']()).resolves.toBe(true); + + expect(update).toHaveBeenCalledWith( + expect.objectContaining({ id: 9, notSentRecheckDue: RELEASED_AT }), + expect.objectContaining({ status: LiquidityManagementOrderStatus.FAILED }), + ); + }); + + it('leaves an unreleased order alone when its adapter is gone', async () => { + const order = uncertainOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue(null); + + await service['resolveUncertainOrders'](); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(orderRepo.update).not.toHaveBeenCalled(); + }); + + it('puts a confirmed order back into quarantine when the release does not land', async () => { + // an alert alone is read at human speed while the rule reactivates in minutes + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([uncertainOrder()]); + jest + .spyOn(orderRepo, 'findOneBy') + .mockResolvedValue(uncertainOrder({ status: LiquidityManagementOrderStatus.FAILED })); + const update = jest + .spyOn(orderRepo, 'update') + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }) + .mockResolvedValueOnce({ affected: 0, raw: [], generatedMaps: [] }) + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + + expect(update.mock.calls[2][1]).toMatchObject({ status: LiquidityManagementOrderStatus.UNCERTAIN }); + expect(notificationService.sendMail).toHaveBeenCalledWith( + expect.objectContaining({ correlationId: 'lm-observation-unapplied-9' }), + ); + }); + + it('does the same when the release throws instead of matching nothing', async () => { + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([uncertainOrder()]); + jest + .spyOn(orderRepo, 'findOneBy') + .mockResolvedValue(uncertainOrder({ status: LiquidityManagementOrderStatus.FAILED })); + const update = jest + .spyOn(orderRepo, 'update') + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }) + .mockRejectedValueOnce(new Error('connection lost')) + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + + expect(update.mock.calls[2][1]).toMatchObject({ status: LiquidityManagementOrderStatus.UNCERTAIN }); + expect(notificationService.sendMail).toHaveBeenCalled(); + }); + + it('keeps retrying a confirmed observation whose repair write failed, until it lands', async () => { + // one failed statement must not be the end of an observation: the order would stay terminal while the + // venue works it, and nothing selects a terminal row again + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([uncertainOrder()]); + jest + .spyOn(orderRepo, 'findOneBy') + .mockResolvedValue(uncertainOrder({ status: LiquidityManagementOrderStatus.FAILED })); + const update = jest + .spyOn(orderRepo, 'update') + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }) + .mockResolvedValueOnce({ affected: 0, raw: [], generatedMaps: [] }) // the release misses + .mockRejectedValueOnce(new Error('deadlock detected')) // and the repair fails + .mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + expect(service['unappliedObservations'].size).toBe(1); + + // the next pass repeats the write before it asks the venue anything + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([]); + await service['resolveUncertainOrders'](); + + expect(update).toHaveBeenLastCalledWith( + expect.anything(), + expect.objectContaining({ status: LiquidityManagementOrderStatus.UNCERTAIN }), + ); + expect(service['unappliedObservations'].size).toBe(0); + }); + + it('stops retrying once another path has put the order somewhere safe', async () => { + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([uncertainOrder()]); + jest + .spyOn(orderRepo, 'findOneBy') + .mockResolvedValueOnce(uncertainOrder({ status: LiquidityManagementOrderStatus.FAILED })) + .mockResolvedValue(uncertainOrder({ status: LiquidityManagementOrderStatus.IN_PROGRESS })); + jest + .spyOn(orderRepo, 'update') + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }) + .mockResolvedValueOnce({ affected: 0, raw: [], generatedMaps: [] }) + .mockRejectedValueOnce(new Error('deadlock detected')); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + expect(service['unappliedObservations'].size).toBe(1); + + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([]); + await service['resolveUncertainOrders'](); + + expect(service['unappliedObservations'].size).toBe(0); + }); + + it('makes a confirmed order safe with the very first write, whatever state it is in', async () => { + // until this lands, the only thing keeping a confirmed order from being treated as finished business + // is this process staying alive — so it comes before the substantial write, and covers an order a + // concurrent release has already ended + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([releasePendingOrder()]); + const update = jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + + const [where, payload] = update.mock.calls[0]; + expect(payload).toEqual({ + status: LiquidityManagementOrderStatus.UNCERTAIN, + notSentRecheckDue: null, + }); + expect(where).toMatchObject({ id: 9 }); + // an order already ended by a release is repaired by this same statement + const status = (where as unknown as { status: { _value: LiquidityManagementOrderStatus[] } }).status; + expect(status._value).toEqual(expect.arrayContaining([LiquidityManagementOrderStatus.FAILED])); + }); + + it('reports a confirmed order whose state it cannot even read', async () => { + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([uncertainOrder()]); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 0, raw: [], generatedMaps: [] }); + jest.spyOn(orderRepo, 'findOneBy').mockRejectedValue(new Error('connection lost')); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + + expect(notificationService.sendMail).toHaveBeenCalledWith( + expect.objectContaining({ correlationId: 'lm-observation-unapplied-9' }), + ); + }); + + it('stays quiet when something else had already released the order correctly', async () => { + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([uncertainOrder()]); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 0, raw: [], generatedMaps: [] }); + jest + .spyOn(orderRepo, 'findOneBy') + .mockResolvedValue(uncertainOrder({ status: LiquidityManagementOrderStatus.IN_PROGRESS })); + stubIntegration(UncertainOrderResolution.SENT); + + await service['resolveUncertainOrders'](); + + expect(notificationService.sendMail).not.toHaveBeenCalled(); + }); + + it('keeps the order quarantined when the lookup itself throws', async () => { + const order = uncertainOrder(); + jest.spyOn(orderRepo, 'findBy').mockResolvedValue([order]); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn(), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + resolveUncertainOrder: jest.fn().mockRejectedValue(new Error('venue unreachable')), + }); + + await service['resolveUncertainOrders'](); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + }); + }); + + describe('resolveUncertainOrderManually', () => { + const VERIFIED_DTO = { noExecutionVerified: true, verificationReference: 'venue console, ticket OPS-42' }; + + it('refuses an unverified claim, even if the edge validation were bypassed', async () => { + await expect( + service.resolveUncertainOrderManually(9, { noExecutionVerified: false, verificationReference: 'x' }, 42), + ).rejects.toThrow(/noExecutionVerified must be true/); + }); + + it('refuses a whitespace-only verification reference', async () => { + await expect( + service.resolveUncertainOrderManually(9, { noExecutionVerified: true, verificationReference: ' ' }, 42), + ).rejects.toThrow(/must name where the venue was checked/); + }); + + it('refuses to release an order the venue confirms is live, whoever would win the write', async () => { + // the race the compare-and-set alone cannot decide: writing first is not the same as being right + const order = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + errorMessage: 'unknown', + action: { id: 233, system: 'Scrypt', command: 'sell' }, + }); + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValue(order); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn(), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + resolveUncertainOrder: jest.fn().mockResolvedValue(UncertainOrderResolution.SENT), + }); + + await expect(service.resolveUncertainOrderManually(9, VERIFIED_DTO, 42)).rejects.toThrow( + /the venue confirms the request exists/, + ); + // and the observation is PERSISTED, not just refused — otherwise a later attempt made while the venue + // is unreachable could still release the order and undo what was seen here + expect(order.status).toBe(LiquidityManagementOrderStatus.IN_PROGRESS); + expect(orderRepo.update).toHaveBeenCalled(); + }); + + it('holds a confirmed order and reports it when the manual refusal cannot be written either', async () => { + // the manual path faces the same race as reconciliation, so it must end the same way: the order stays + // blocking and a person is told, rather than the observation being discarded with the exception + const order = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + errorMessage: 'unknown', + action: { id: 233, system: 'Scrypt', command: 'sell' }, + }); + const raced = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.FAILED, + errorMessage: 'unknown (released by account 7: venue checked — ticket OPS-99)', + action: { id: 233, system: 'Scrypt', command: 'sell' }, + }); + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValueOnce(order).mockResolvedValue(raced); + const update = jest + .spyOn(orderRepo, 'update') + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }) + .mockResolvedValueOnce({ affected: 0, raw: [], generatedMaps: [] }) + .mockResolvedValueOnce({ affected: 1, raw: [], generatedMaps: [] }); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn(), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + resolveUncertainOrder: jest.fn().mockResolvedValue(UncertainOrderResolution.SENT), + }); + + await expect(service.resolveUncertainOrderManually(9, VERIFIED_DTO, 42)).rejects.toThrow(/held as uncertain/); + + expect(update.mock.calls[2][1]).toMatchObject({ status: LiquidityManagementOrderStatus.UNCERTAIN }); + // the account and reference recorded by whoever released it are still there afterwards + expect(update.mock.calls[2][1].errorMessage).toContain('account 7'); + expect(update.mock.calls[2][1].errorMessage).toContain('OPS-99'); + expect(notificationService.sendMail).toHaveBeenCalledWith( + expect.objectContaining({ correlationId: 'lm-observation-unapplied-9' }), + ); + }); + + it('accepts a release for an order whose adapter is no longer registered', async () => { + // nothing can be asked here, so the request is recorded and reconciliation puts it into effect + const order = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + errorMessage: 'unknown', + action: { id: 233, system: 'Scrypt', command: 'sell' }, + }); + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValue(order); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue(null); + + await expect(service.resolveUncertainOrderManually(9, VERIFIED_DTO, 42)).resolves.toBeUndefined(); + + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(order.notSentRecheckDue).toBeInstanceOf(Date); + }); + + it('skips an order another path resolved first, instead of overwriting it', async () => { + const order = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + errorMessage: 'unknown', + }); + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValue(order); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 0, raw: [], generatedMaps: [] }); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn(), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + resolveUncertainOrder: jest.fn().mockResolvedValue(UncertainOrderResolution.UNRESOLVED), + }); + + await expect(service.resolveUncertainOrderManually(9, VERIFIED_DTO, 42)).rejects.toThrow(/resolved elsewhere/); + }); + + it('records a release and where the check happened, without ending the order yet', async () => { + const order = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.UNCERTAIN, + errorMessage: 'Scrypt gave no confirmed outcome', + }); + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValue(order); + jest.spyOn(orderRepo, 'update').mockResolvedValue({ affected: 1, raw: [], generatedMaps: [] }); + jest.spyOn(actionIntegrationFactory, 'getIntegration').mockReturnValue({ + supportedCommands: ['sell'], + executeOrder: jest.fn(), + checkCompletion: jest.fn(), + validateParams: jest.fn(), + resolveUncertainOrder: jest.fn().mockResolvedValue(UncertainOrderResolution.UNRESOLVED), + }); + + await service.resolveUncertainOrderManually(9, VERIFIED_DTO, 42); + + // accepted, but the order keeps blocking until reconciliation has had one answer from the venue + expect(order.status).toBe(LiquidityManagementOrderStatus.UNCERTAIN); + expect(order.notSentRecheckDue).toBeInstanceOf(Date); + expect(order.errorMessage).toContain('venue console, ticket OPS-42'); + expect(order.errorMessage).toContain('account 42'); + }); + + it('refuses to touch an order that is not quarantined', async () => { + const order = Object.assign(new LiquidityManagementOrder(), { + id: 9, + status: LiquidityManagementOrderStatus.IN_PROGRESS, + }); + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValue(order); + + await expect(service.resolveUncertainOrderManually(9, VERIFIED_DTO, 42)).rejects.toThrow( + /only an uncertain order/, + ); + expect(order.status).toBe(LiquidityManagementOrderStatus.IN_PROGRESS); + }); + + it('fails loudly for an unknown order', async () => { + jest.spyOn(orderRepo, 'findOneBy').mockResolvedValue(null); + + await expect(service.resolveUncertainOrderManually(404, VERIFIED_DTO, 42)).rejects.toThrow( + /No liquidity management order/, + ); + }); + }); + + describe('processPipelines — the observation barrier', () => { + it('advances nothing while a confirmed observation could not be recorded', async () => { + // an order may be live at the venue while its row says otherwise; starting or advancing anything on + // that picture is exactly how a second request goes out + service['unappliedObservations'].set(1, new LiquidityManagementOrder()); + jest.spyOn(service as any, 'resolveUncertainOrders').mockResolvedValue(false); + const startNewPipelines = jest.spyOn(service as any, 'startNewPipelines').mockResolvedValue(false); + const checkRunningOrders = jest.spyOn(service as any, 'checkRunningOrders').mockResolvedValue(false); + const startNewOrders = jest.spyOn(service as any, 'startNewOrders').mockResolvedValue(false); + + await service.processPipelines(); + + expect(startNewPipelines).not.toHaveBeenCalled(); + expect(checkRunningOrders).not.toHaveBeenCalled(); + expect(startNewOrders).not.toHaveBeenCalled(); + }); + + it('resumes once the observation has been recorded', async () => { + jest.spyOn(service as any, 'resolveUncertainOrders').mockResolvedValue(false); + const startNewPipelines = jest.spyOn(service as any, 'startNewPipelines').mockResolvedValue(false); + jest.spyOn(service as any, 'checkRunningOrders').mockResolvedValue(false); + jest.spyOn(service as any, 'checkRunningPipelines').mockResolvedValue(false); + jest.spyOn(service as any, 'startNewOrders').mockResolvedValue(false); + + await service.processPipelines(); + + expect(startNewPipelines).toHaveBeenCalled(); + }); + }); + + describe('checkRunningPipelines — quarantined orders', () => { + it('leaves a pipeline whose last order is UNCERTAIN completely alone', async () => { + const rule = Object.assign(new LiquidityManagementRule(), { id: 42, sendNotifications: true }); + const pipeline = Object.assign(new LiquidityManagementPipeline(), { + id: 1, + status: LiquidityManagementPipelineStatus.IN_PROGRESS, + currentAction: { id: 233 }, + rule, + }); + jest.spyOn(pipelineRepo, 'find').mockResolvedValue([pipeline]); + jest + .spyOn(orderRepo, 'findOne') + .mockResolvedValue( + Object.assign(new LiquidityManagementOrder(), { id: 9, status: LiquidityManagementOrderStatus.UNCERTAIN }), + ); + + const anyChanged = await service['checkRunningPipelines'](); + + // no advance, no fail, no new order — and crucially the rule is neither paused nor reactivated, + // which is what stops an unresolved order from being reissued + expect(anyChanged).toBe(false); + expect(pipeline.status).toBe(LiquidityManagementPipelineStatus.IN_PROGRESS); + expect(pipelineRepo.save).not.toHaveBeenCalled(); + expect(orderRepo.save).not.toHaveBeenCalled(); + expect(ruleRepo.save).not.toHaveBeenCalled(); + expect(notificationService.sendMail).not.toHaveBeenCalled(); + }); + }); + describe('handlePipelineFail', () => { it('resets the activation debounce timer when a rule is paused', async () => { const rule = Object.assign(new LiquidityManagementRule(), { diff --git a/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.ts b/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.ts index 0f108a73d4..3dbd887d94 100644 --- a/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.ts +++ b/src/subdomains/core/liquidity-management/services/liquidity-management-pipeline.service.ts @@ -1,18 +1,21 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { BadRequestException, ConflictException, Injectable, NotFoundException } from '@nestjs/common'; import { CronExpression } from '@nestjs/schedule'; import { DfxLogger } from 'src/shared/services/dfx-logger'; import { Process } from 'src/shared/services/process.service'; import { DfxCron } from 'src/shared/utils/cron'; +import { Util } from 'src/shared/utils/util'; import { MailContext, MailType } from 'src/subdomains/supporting/notification/enums'; import { MailRequest } from 'src/subdomains/supporting/notification/interfaces'; import { NotificationService } from 'src/subdomains/supporting/notification/services/notification.service'; import { In } from 'typeorm'; +import { ResolveUncertainOrderDto } from '../dto/resolve-uncertain-order.dto'; import { LiquidityManagementOrder } from '../entities/liquidity-management-order.entity'; import { LiquidityManagementPipeline } from '../entities/liquidity-management-pipeline.entity'; -import { LiquidityManagementOrderStatus, LiquidityManagementPipelineStatus } from '../enums'; +import { LiquidityManagementOrderStatus, LiquidityManagementPipelineStatus, UncertainOrderResolution } from '../enums'; import { OrderFailedException } from '../exceptions/order-failed.exception'; import { OrderNotNecessaryException } from '../exceptions/order-not-necessary.exception'; import { OrderNotProcessableException } from '../exceptions/order-not-processable.exception'; +import { OrderOutcomeUnknownException } from '../exceptions/order-outcome-unknown.exception'; import { LiquidityActionIntegrationFactory } from '../factories/liquidity-action-integration.factory'; import { LiquidityManagementOrderRepository } from '../repositories/liquidity-management-order.repository'; import { LiquidityManagementPipelineRepository } from '../repositories/liquidity-management-pipeline.repository'; @@ -23,6 +26,19 @@ import { LiquidityManagementService } from './liquidity-management.service'; export class LiquidityManagementPipelineService { private readonly logger = new DfxLogger(LiquidityManagementPipelineService); + /** + * Confirmed venue observations this process has not managed to write down yet. + * + * A statement that fails must not be the end of one. The order it belongs to would stay terminal while the + * venue works it, and nothing selects a terminal row again — so the write is kept and simply retried until + * it lands. A retry here asks the venue nothing; it only repeats what is already known. + * + * Held in memory on purpose. The alternative is a durable queue whose rows nothing ever drains, and the + * case this cannot cover — the process ending first — is the case the alert raised alongside it covers: + * somebody has been told, by name and reference, to treat the order as live. + */ + private readonly unappliedObservations = new Map(); + constructor( private readonly ruleRepo: LiquidityManagementRuleRepository, private readonly orderRepo: LiquidityManagementOrderRepository, @@ -38,12 +54,27 @@ export class LiquidityManagementPipelineService { async processPipelines(): Promise { let hasChanges = true; while (hasChanges) { + // reconcile before issuing anything new: an order whose outcome we could not observe must be + // accounted for against the venue before the same rule is allowed to act again + const uncertainResolved = await this.resolveUncertainOrders(); + + // A venue observation this process holds but could not write means at least one order may be live at + // the venue while its row says otherwise. Nothing downstream may run on that picture — starting or + // advancing anything now is exactly how a second request goes out — so the pass stops here and the + // next one retries the write first. + if (this.unappliedObservations.size) { + this.logger.error( + `Holding the liquidity pipeline: ${this.unappliedObservations.size} confirmed venue observation(s) could not be recorded`, + ); + return; + } + const newPipelinesStarted = await this.startNewPipelines(); const ordersChanged = await this.checkRunningOrders(); const pipelinesChanged = await this.checkRunningPipelines(); const newOrdersStarted = await this.startNewOrders(); - hasChanges = newPipelinesStarted || ordersChanged || pipelinesChanged || newOrdersStarted; + hasChanges = uncertainResolved || newPipelinesStarted || ordersChanged || pipelinesChanged || newOrdersStarted; } } @@ -63,12 +94,22 @@ export class LiquidityManagementPipelineService { async getProcessingOrders(): Promise { return this.orderRepo.findBy({ - status: In([LiquidityManagementOrderStatus.CREATED, LiquidityManagementOrderStatus.IN_PROGRESS]), + // a quarantined order is unfinished business, not a closed one — it belongs in this view + status: In([ + LiquidityManagementOrderStatus.CREATED, + LiquidityManagementOrderStatus.IN_PROGRESS, + LiquidityManagementOrderStatus.UNCERTAIN, + ]), }); } async getPendingTx(): Promise { return this.orderRepo.findBy({ + // Deliberately WITHOUT the quarantined status. The financial log adds a pending amount back to the + // balance and nets it against the venue's locked funds — which works for an order the venue really is + // holding. For a quarantined order there may be nothing locked, so counting it would inflate equity by + // its full amount. Overstating equity is the one error direction that can hide a real loss from the + // safety threshold, so an unresolved order is left out until reconciliation says it was sent. status: LiquidityManagementOrderStatus.IN_PROGRESS, action: { command: In(['withdraw', 'deposit', 'transfer']) }, }); @@ -174,10 +215,27 @@ export class LiquidityManagementPipelineService { private async startNewOrders(): Promise { const newOrders = await this.orderRepo.findBy({ status: LiquidityManagementOrderStatus.CREATED }); + let anyChanged = false; for (const order of newOrders) { + // A CREATED order that already carries a reference means a previous pass reached the send boundary and + // never recorded the result — the process died between transmitting and saving. Re-sending it is the + // one thing we must not do, so it goes straight into quarantine to be reconciled. + if (order.correlationId) { + order.uncertain( + new OrderOutcomeUnknownException( + `Reference ${order.correlationId} was reserved but the result was never recorded — the request may have been sent`, + ), + ); + await this.orderRepo.save(order); + await this.reportUncertainOrder(order); + anyChanged = true; + continue; + } + try { await this.executeOrder(order); + anyChanged = true; } catch (e) { if (e instanceof OrderNotNecessaryException) { order.complete(); @@ -188,24 +246,400 @@ export class LiquidityManagementPipelineService { } else if (e instanceof OrderFailedException) { order.fail(e); await this.orderRepo.save(order); + } else if (e instanceof OrderOutcomeUnknownException || order.correlationId) { + // Either the integration declared the outcome unknown, or a reference was reserved — meaning the + // send boundary was crossed and we cannot prove the request did not reach the venue. Quarantine + // rather than fail: failing pauses the rule, and the rule auto-reactivates, which would repeat a + // request that may already have executed. + const cause = e instanceof OrderOutcomeUnknownException ? e : new OrderOutcomeUnknownException(e.message); + order.uncertain(cause); + await this.orderRepo.save(order); + await this.reportUncertainOrder(order); + } else { + // No reference was ever reserved, so nothing can have been transmitted — this is an ordinary + // failure. Quarantining it would strand configuration and factory errors in a state only a human + // can clear, for a request that provably never happened. + order.fail(new OrderFailedException(e.message)); + await this.orderRepo.save(order); } + // every branch above persists a new status, so the order leaves the CREATED set either way — this is + // what keeps the caller's `while (hasChanges)` loop from spinning on an order it cannot advance + anyChanged = true; + this.logger.info(`Error in starting new liquidity order ${order.id}:`, e); } } - return newOrders.length > 0; + return anyChanged; } private async executeOrder(order: LiquidityManagementOrder): Promise { const actionIntegration = this.actionIntegrationFactory.getIntegration(order.action); + // Claim the venue-side reference before the request goes out. Integrations that can pin their own + // reference (Scrypt's ClOrdID) become traceable after an un-acknowledged send; the persisted id is also + // what makes a crash between send and save recoverable instead of orphaning a live venue order. + const reservedCorrelationId = actionIntegration.reserveCorrelationId?.(order); + if (reservedCorrelationId) { + order.reserveCorrelationId(reservedCorrelationId); + await this.orderRepo.save(order); + } + const correlationId = await actionIntegration.executeOrder(order); order.inProgress(correlationId); await this.orderRepo.save(order); } + /** + * Resolve orders quarantined as UNCERTAIN by asking the venue what actually happened. + * + * This only ever observes — it must not re-send anything. An order leaves quarantine when the venue + * either confirms it knows the reference (back to IN_PROGRESS, the normal completion check takes over) or + * demonstrably does not (FAILED, so the rule may plan anew from a fresh balance). Anything inconclusive + * stays put: an order nobody can account for is safer parked than retried. + */ + private async resolveUncertainOrders(): Promise { + // First: anything this process observed and could not write. Retried before new lookups, because an + // order the venue has confirmed sitting in a terminal state is the one thing here that cannot wait. + for (const unapplied of [...this.unappliedObservations.values()]) await this.blockConfirmedOrder(unapplied); + + const orders = await this.orderRepo.findBy({ status: LiquidityManagementOrderStatus.UNCERTAIN }); + let anyChanged = false; + + for (const order of orders) { + // Somebody has already judged this one never sent; the venue's answer is what puts that into effect. + const releasePending = Boolean(order.notSentRecheckDue); + + try { + // Null when the action's system or command is no longer registered at all — an order can outlive the + // adapter that made it, and dereferencing that would throw here on every pass, forever. + const actionIntegration = this.actionIntegrationFactory.getIntegration(order.action); + + if (!actionIntegration?.resolveUncertainOrder) { + // The one exception to "a release waits for the venue": there is no lookup for this order at all, + // so the answer it would wait for can never come, and waiting would quarantine it for good. The + // operator's judgement is all there is, which is why the assertion behind it is required. + if (releasePending && (await this.completeNotSentRelease(order, 'no integration can look it up'))) + anyChanged = true; + continue; + } + + const resolution = await actionIntegration.resolveUncertainOrder(order); + + if (resolution === UncertainOrderResolution.SENT) { + if (await this.applyConfirmedObservation(order)) { + anyChanged = true; + this.logger.info(`Uncertain liquidity order ${order.id} resolved: venue confirmed it was sent`); + } + } else if (resolution === UncertainOrderResolution.NOT_SENT) { + // Every reference came back refused — a verdict, not a judgement, so it needs no confirming. + if (await this.completeNotSentRelease(order, 'the venue confirmed the request never arrived')) + anyChanged = true; + } else if (releasePending && resolution === UncertainOrderResolution.UNRESOLVED) { + // The venue answered and has no record, which is not proof on its own — but somebody has already + // checked independently and released the order on that basis. Two negatives, one of them from a + // person who looked: that is what this release was waiting for. + if (await this.completeNotSentRelease(order, 'the venue has no record of it either')) anyChanged = true; + } else if (releasePending && order.releaseWaitedOutVenue()) { + // Nobody has been able to ask this venue anything for long enough. Waiting more does not make an + // answer likelier; it only keeps an order a person has verified by hand out of reach. + if (await this.completeNotSentRelease(order, 'the venue could not be reached for long enough')) + anyChanged = true; + } + // Otherwise — a venue that cannot be asked yet, or an inconclusive answer with nobody having + // released the order — nothing changes and it keeps blocking. + } catch (e) { + // a failing lookup must never promote the order out of quarantine + this.logger.error(`Error in resolving uncertain liquidity order ${order.id}:`, e); + } + } + + return anyChanged; + } + + /** + * Put a not-sent conclusion into effect: the order becomes an ordinary failure and its rule may plan anew. + * + * The only place an order leaves quarantine downwards. Everything that reaches here has either a venue + * verdict behind it, or a person who checked plus a venue that has no record — never a single judgement on + * its own. The two exceptions are about liveness, not evidence: a venue nothing can ask, and one that has + * answered nothing for long enough. Silence there stops vetoing the person who checked; it proves nothing. + */ + private async completeNotSentRelease(order: LiquidityManagementOrder, because: string): Promise { + // The release this pass looked at, captured before the entity is mutated. Ending an order is the one + // irreversible step here, so it may only be taken against exactly the release that was examined — never + // against one written since, whose own confirmation is still outstanding. + const examined = order.notSentRecheckDue ?? null; + + order.resolveAsNotSent(`${order.errorMessage} (released ${new Date().toISOString()}: ${because})`); + + if (!(await this.leaveQuarantine(order, examined))) return false; + + this.logger.info(`Uncertain liquidity order ${order.id} released as never sent: ${because}`); + + return true; + } + + /** + * Record that the venue holds this order — and make sure that fact lands somewhere durable. + * + * Both releases are conditional on the state they expect, so either can miss, and either can fail. What + * must never follow from that is a confirmed order sitting in a state its rule reads as finished: an alert + * is acted on at human speed, a rule reactivates in minutes. So anything short of a clean release puts the + * order back into quarantine, which is the state this subdomain already treats as "in flight, outcome + * open" — no rule plans against it, and the next reconciliation pass simply tries again. + * + * Returns whether the order was released; a re-quarantined one has not been. + */ + private async applyConfirmedObservation(order: LiquidityManagementOrder): Promise { + // First, and as its own smallest possible write: put the order where nothing acts on it. That covers + // both a pending release, which could otherwise end it on the next inconclusive lookup, and a release + // that has already ended it — repairing that here rather than afterwards is what stops this process + // from being the only thing standing between a live venue order and a second request. + await this.secureConfirmedOrder(order); + + order.resolveAsSent(); + + try { + if (await this.leaveQuarantine(order)) return true; + } catch (e) { + this.logger.error(`Could not record the venue observation for liquidity order ${order.id}:`, e); + } + + await this.blockConfirmedOrder(order); + + return false; + } + + /** + * Make an order the venue has confirmed safe in one statement, before anything else can fail. + * + * Quarantine is the state nothing acts on, so this both strips a pending release of its power to end the + * order and puts one that has already been ended back. Everything else about applying an observation can + * be retried; this cannot wait for a retry, because until it lands the only thing keeping a confirmed + * order from being treated as finished business is this process staying alive. + * + * Deliberately the narrowest write here: two columns, no appended text, no read it depends on. The reason + * why follows separately, and if that never lands the order is at least still blocking. + */ + private async secureConfirmedOrder(order: LiquidityManagementOrder): Promise { + await this.orderRepo + .update( + { + id: order.id, + status: In([ + LiquidityManagementOrderStatus.UNCERTAIN, + LiquidityManagementOrderStatus.FAILED, + LiquidityManagementOrderStatus.NOT_PROCESSABLE, + ]), + }, + { status: LiquidityManagementOrderStatus.UNCERTAIN, notSentRecheckDue: null }, + ) + .catch((e) => this.logger.error(`Could not secure confirmed liquidity order ${order.id}:`, e)); + } + + /** + * Put an order the venue has confirmed back where nothing can act on it, and say so. + * + * Left alone only where the outcome is already safe: in progress or complete means somebody released it + * correctly first, still quarantined means it never stopped blocking and the next pass will retry. Every + * other state — including one this pass could not even read — is reported, because it is the case where a + * live venue order was about to be treated as finished business. + */ + private async blockConfirmedOrder(order: LiquidityManagementOrder): Promise { + const current = await this.orderRepo.findOneBy({ id: order.id }).catch(() => null); + + // Already safe: in progress or complete means another path released it correctly. Still quarantined + // counts too, but only with no release pending on it — a quarantined order somebody has released is one + // inconclusive lookup away from being ended, which is precisely what the observation contradicts. + const settled = [LiquidityManagementOrderStatus.IN_PROGRESS, LiquidityManagementOrderStatus.COMPLETE]; + const safe = + current && + (settled.includes(current.status) || + (current.status === LiquidityManagementOrderStatus.UNCERTAIN && !current.notSentRecheckDue)); + + if (safe) { + this.unappliedObservations.delete(order.id); + return; + } + + const message = + `Liquidity order ${order.id}: the venue confirms reference ${order.correlationId} exists, but that could ` + + `not be recorded as usual. It is held as uncertain — treat it as live at the venue and resolve it by ` + + `hand; no rule may plan against these funds until somebody has.`; + + const blocked = await this.orderRepo + .update( + // Guarded on the exact reason just read: between that read and this write another path can have + // replaced it, and appending to the older copy would erase whatever it now says. + { + id: order.id, + status: In([LiquidityManagementOrderStatus.FAILED, LiquidityManagementOrderStatus.NOT_PROCESSABLE]), + ...(current ? { errorMessage: current.errorMessage } : {}), + }, + // Append to the reason already on the row, and where it could not be read, change only the status: + // whatever it says may be the operator account and verification reference behind the resolution + // being overruled, and that has to survive. + current + ? { + status: LiquidityManagementOrderStatus.UNCERTAIN, + errorMessage: [current.errorMessage, message].filter((part) => part).join(' — '), + notSentRecheckDue: null, + } + : { status: LiquidityManagementOrderStatus.UNCERTAIN }, + ) + .then((result) => Boolean(result.affected)) + .catch(() => false); + + // Kept for the next pass if it did not land. Dropping it here is what would let a confirmed order stay + // terminal on the strength of one failed statement. + if (blocked) this.unappliedObservations.delete(order.id); + else this.unappliedObservations.set(order.id, order); + + this.logger.error(message); + + await this.notificationService.sendMail({ + type: MailType.ERROR_MONITORING, + context: MailContext.LIQUIDITY_MANAGEMENT, + correlationId: `lm-observation-unapplied-${order.id}`, + options: { debounce: 3600000 }, + input: { subject: 'Liquidity management order CONFIRMED BUT NOT RECORDED', errors: [message] }, + }); + } + + /** + * Write a resolved order back, but only if it is still quarantined. + * + * Automatic reconciliation and an operator can be looking at the same order at the same time; an + * unconditional save would let whoever finishes last win. Losing that race the wrong way would release a + * rule for an order the venue had just confirmed as live, which is the double execution this all exists to + * prevent — so the status is part of the WHERE clause and a lost race is simply skipped. + */ + private async leaveQuarantine(order: LiquidityManagementOrder, expectedRecheckDue?: Date | null): Promise { + const result = await this.orderRepo.update( + { + id: order.id, + status: LiquidityManagementOrderStatus.UNCERTAIN, + // narrowed by the caller when the outcome depends on WHICH pending release was examined + ...(expectedRecheckDue !== undefined ? { notSentRecheckDue: expectedRecheckDue } : {}), + }, + { + status: order.status, + errorMessage: order.errorMessage, + correlationId: order.correlationId, + previousCorrelationIds: order.previousCorrelationIds, + // carries the pending-release marker: set when somebody releases the order, cleared once the venue + // has answered, and written here because this is where either becomes durable + notSentRecheckDue: order.notSentRecheckDue ?? null, + }, + ); + + if (!result.affected) { + this.logger.info(`Uncertain liquidity order ${order.id} was already resolved elsewhere, skipping`); + return false; + } + + return true; + } + + private async reportUncertainOrder(order: LiquidityManagementOrder): Promise { + const message = + `Liquidity order ${order.id} (action ${order.action.id}, ${order.action.system}/${order.action.command}) ` + + `has an unknown outcome: ${order.errorMessage}. Reference: ${order.correlationId ?? 'none reserved'}. ` + + `The order is quarantined and will NOT be retried automatically — it is resolved against the venue.`; + + this.logger.error(message); + + await this.notificationService.sendMail({ + type: MailType.ERROR_MONITORING, + context: MailContext.LIQUIDITY_MANAGEMENT, + // pinned per order so repeated reports collapse instead of one mail per pass; debounce rather than + // suppressRecurring, so a still-unresolved order keeps reminding us once an hour + correlationId: `lm-order-uncertain-${order.id}`, + options: { debounce: 3600000 }, + input: { + subject: 'Liquidity management order outcome UNKNOWN', + errors: [message], + }, + }); + } + + /** + * Release a quarantined order by hand, after somebody checked the venue directly. + * + * Reconciliation can only ever confirm that a reference exists; it never concludes the opposite, because + * no venue reply proves "this was never accepted". Without this path a genuinely unsent request would + * block its rule forever. Guarded like the payout subdomain's retry: the caller must assert the check and + * name where it happened, and the assertion is recorded on the order. + */ + async resolveUncertainOrderManually( + orderId: number, + dto: ResolveUncertainOrderDto, + resolvedBy: number, + ): Promise { + // Re-asserted here, not only at the edge: this is the one call that can release a possibly-executed + // request, so the claim behind it must hold at the point where it takes effect. + if (dto.noExecutionVerified !== true) + throw new BadRequestException('noExecutionVerified must be true — an unverified order stays quarantined'); + + const verificationReference = dto.verificationReference?.trim(); + if (!verificationReference) + throw new BadRequestException('verificationReference must name where the venue was checked'); + + const order = await this.orderRepo.findOneBy({ id: orderId }); + if (!order) throw new NotFoundException(`No liquidity management order found for id ${orderId}`); + + if (order.status !== LiquidityManagementOrderStatus.UNCERTAIN) + throw new BadRequestException( + `Liquidity management order ${orderId} is ${order.status}, only an uncertain order can be resolved manually`, + ); + + // Ask the venue one more time, right here. A compare-and-set alone only decides who writes first, and + // "first" is not the same as "right": an operator releasing the order in the same moment reconciliation + // confirms it is live would win the write and release the rule against a live position. A positive + // observation therefore outranks the operator's judgement. + // + // A lookup that cannot be performed does not refuse the request outright — but it does not put it into + // effect either. The order stays quarantined until reconciliation has had one answer, so a release can + // never end an order while a confirmation of it is still in flight. + const actionIntegration = this.actionIntegrationFactory.getIntegration(order.action); + if (actionIntegration?.resolveUncertainOrder) { + const resolution = await actionIntegration.resolveUncertainOrder(order); + + if (resolution === UncertainOrderResolution.SENT) { + // Record the observation before refusing. Merely throwing would leave the row quarantined, so a + // later attempt — made while the venue happens to be unreachable — could still release it and undo + // what we just saw. Once observed live, the order is no longer a candidate for manual release at all. + // Same path as automatic reconciliation, including what happens when that write does not land. + const released = await this.applyConfirmedObservation(order); + + throw new ConflictException( + `Liquidity management order ${orderId} cannot be released: the venue confirms the request exists. ` + + (released + ? 'It has been returned to in progress and the normal completion check now tracks it.' + : 'It could not be returned to in progress and is held as uncertain — it has been reported.'), + ); + } + } + + order.requestNotSentRelease( + `${order.errorMessage} (released by account ${resolvedBy} at ${new Date().toISOString()}: ` + + `venue checked, no execution found — ${verificationReference})`, + ); + + if (!(await this.leaveQuarantine(order))) + throw new ConflictException( + `Liquidity management order ${orderId} was resolved elsewhere while this request was in flight`, + ); + + // only after the write actually landed — a log line for a save that failed is worse than none + this.logger.info( + `Uncertain liquidity order ${orderId} manually resolved as not executed by account ${resolvedBy}, verified via ${verificationReference}`, + ); + } + private async checkRunningOrders(): Promise { const runningOrders = await this.orderRepo.findBy({ status: LiquidityManagementOrderStatus.IN_PROGRESS }); let anyChanged = false; @@ -227,6 +661,15 @@ export class LiquidityManagementPipelineService { anyChanged = true; continue; } + if (e instanceof OrderOutcomeUnknownException) { + // The completion check can amend or restart an order, so it has a send boundary of its own. An + // unconfirmed outcome here must quarantine rather than fail, for the same reason as on first send. + order.uncertain(e); + await this.orderRepo.save(order); + await this.reportUncertainOrder(order); + anyChanged = true; + continue; + } this.logger.error(`Error in checking running liquidity order ${order.id}:`, e); } @@ -253,11 +696,10 @@ export class LiquidityManagementPipelineService { await this.ruleRepo.save(rule); - const [successMessage, mailRequest] = this.generateSuccessMessage(pipeline); - - if (rule.sendNotifications) await this.notificationService.sendMail(mailRequest); - - this.logger.verbose(successMessage); + // No mail on success. Over the week to 27.07.2026 this path produced 211 of 255 liquidity mails, none of + // which carried information or asked for an action — and that volume is what made the mails that DO + // matter (see reportUncertainOrder) unreadable. The completion stays in the log. + this.logger.verbose(this.generateSuccessMessage(pipeline)); } private async handlePipelineFail( @@ -278,20 +720,20 @@ export class LiquidityManagementPipelineService { if (rule.sendNotifications) await this.notificationService.sendMail(mailRequest); } - private generateSuccessMessage(pipeline: LiquidityManagementPipeline): [string, MailRequest] { - const { id, type, maxAmount, rule } = pipeline; - const successMessage = `${type} pipeline for max. ${maxAmount} ${rule.targetName} (rule ${rule.id}) completed. Pipeline ID: ${id}`; + /** + * Stable short key for "same cause", used to scope alert debouncing. Digits are stripped so that amounts, + * ids and balances in the message do not make every repeat of one recurring problem look like a new one. + */ + private causeKey(errorMessage?: string): string { + const normalized = (errorMessage ?? 'unknown').toLowerCase().replace(/\d+/g, '#'); - const mailRequest: MailRequest = { - type: MailType.ERROR_MONITORING, - context: MailContext.LIQUIDITY_MANAGEMENT, - input: { - subject: 'Liquidity management pipeline SUCCESS', - errors: [successMessage], - }, - }; + return Util.createHash(normalized).slice(0, 12); + } + + private generateSuccessMessage(pipeline: LiquidityManagementPipeline): string { + const { id, type, maxAmount, rule } = pipeline; - return [successMessage, mailRequest]; + return `${type} pipeline for max. ${maxAmount} ${rule.targetName} (rule ${rule.id}) completed. Pipeline ID: ${id}`; } private generateFailMessage( @@ -306,6 +748,16 @@ export class LiquidityManagementPipelineService { const mailRequest: MailRequest = { type: MailType.ERROR_MONITORING, context: MailContext.LIQUIDITY_MANAGEMENT, + // Pinned per rule and debounced: a single incident retries every few minutes, and each attempt used to + // mail. On 20.07.2026 two rules produced 18 mails in two hours for one underlying problem. Debounce + // rather than suppressRecurring, so a rule that keeps failing keeps reminding us once an hour instead + // of going quiet forever after the first mail. + // Keyed by rule AND cause. Suppression compares only correlationId and context, never the body, so a + // rule-only key would swallow a genuinely different failure of the same rule inside the window. + // Keying by pipeline would defeat the purpose instead — every retry is a new pipeline, which is how + // one incident produced 18 mails in two hours. + correlationId: `lm-pipeline-fail-${rule.id}-${this.causeKey(order.errorMessage)}`, + options: { debounce: 3600000 }, input: { subject: 'Liquidity management pipeline FAIL', errors: [ diff --git a/src/subdomains/core/monitoring/observers/liquidity.observer.ts b/src/subdomains/core/monitoring/observers/liquidity.observer.ts index d28e82fa29..7799862b6e 100644 --- a/src/subdomains/core/monitoring/observers/liquidity.observer.ts +++ b/src/subdomains/core/monitoring/observers/liquidity.observer.ts @@ -21,6 +21,9 @@ interface LiquidityData { stuckTradingRuleCount: number; stuckLmOrderCount: number; stuckLmRuleCount: number; + // Orders whose request left our side without an observed outcome. Unlike the stuck counters this has no + // age threshold: one such order is already worth surfacing, because money may have moved without a record. + uncertainLmOrderCount: number; safetyModeActive: boolean; krakenSyncDelay: number; // min binanceSyncDelay: number; // min @@ -88,6 +91,9 @@ export class LiquidityObserver extends MetricObserver { status: In([LiquidityManagementRuleStatus.PAUSED, LiquidityManagementRuleStatus.PROCESSING]), updated: LessThan(Util.minutesBefore(60)), }), + uncertainLmOrderCount: await this.repos.lmOrder.countBy({ + status: LiquidityManagementOrderStatus.UNCERTAIN, + }), safetyModeActive: this.processService.isSafetyModeActive(), binanceSyncDelay: Math.abs(Util.minutesDiff(lastBinanceTx?.externalCreated, binance?.updated)), krakenSyncDelay: Math.abs(Util.minutesDiff(lastKrakenTx?.externalCreated, kraken?.updated)), diff --git a/src/subdomains/supporting/bank/bank/__tests__/bank.controller.spec.ts b/src/subdomains/supporting/bank/bank/__tests__/bank.controller.spec.ts new file mode 100644 index 0000000000..59db5b0fef --- /dev/null +++ b/src/subdomains/supporting/bank/bank/__tests__/bank.controller.spec.ts @@ -0,0 +1,110 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { BadRequestException, ValidationPipe } from '@nestjs/common'; +import { GUARDS_METADATA, METHOD_METADATA, PATH_METADATA } from '@nestjs/common/constants'; +import { RequestMethod } from '@nestjs/common/enums'; +import { THROTTLER_LIMIT, THROTTLER_TTL } from '@nestjs/throttler/dist/throttler.constants'; +import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; +import { OptionalJwtAuthGuard } from 'src/shared/auth/optional.guard'; +import { RateLimitGuard } from 'src/shared/auth/rate-limit.guard'; +import { UserRole } from 'src/shared/auth/user-role.enum'; +import { olkyEUR } from '../__mocks__/bank.entity.mock'; +import { BankController } from '../bank.controller'; +import { BankService } from '../bank.service'; +import { CheckReceiveIbanDto } from '../dto/receive-iban.dto'; +import { ReceiveIbanStatus } from '../dto/receive-iban.enum'; + +// The receiveIban check runs behind an optional guard, so the controller must forward the account of a +// present JWT and undefined otherwise - that distinction is what makes the service answer LoginRequired. +describe('BankController.checkReceiveIban', () => { + let controller: BankController; + let service: DeepMocked; + + const dto: CheckReceiveIbanDto = { iban: olkyEUR.iban }; + + beforeEach(() => { + service = createMock(); + controller = new BankController(service); + }); + + it('forwards the account of the authenticated customer and wraps the status', async () => { + service.getReceiveIbanStatus.mockResolvedValue(ReceiveIbanStatus.DFX_IBAN); + + const result = await controller.checkReceiveIban({ account: 42, role: UserRole.USER } as JwtPayload, dto); + + expect(service.getReceiveIbanStatus).toHaveBeenCalledWith(dto.iban, 42); + expect(result).toEqual({ status: ReceiveIbanStatus.DFX_IBAN }); + }); + + it('forwards undefined without a JWT', async () => { + service.getReceiveIbanStatus.mockResolvedValue(ReceiveIbanStatus.LOGIN_REQUIRED); + + const result = await controller.checkReceiveIban(undefined, dto); + + expect(service.getReceiveIbanStatus).toHaveBeenCalledWith(dto.iban, undefined); + expect(result).toEqual({ status: ReceiveIbanStatus.LOGIN_REQUIRED }); + }); +}); + +// The two tests above pass the JWT in directly, so they cannot see the decorators that decide whether a JWT +// is ever attached. Removing @UseGuards entirely leaves them green while every logged-in customer would get +// LoginRequired (req.user never set), and swapping in a hard AuthGuard() would 401 every anonymous customer. +describe('BankController.checkReceiveIban routing & security metadata', () => { + const handler = BankController.prototype.checkReceiveIban; + + it('is mounted as PUT bank/receiveIban', () => { + expect(Reflect.getMetadata(PATH_METADATA, BankController)).toBe('bank'); + expect(Reflect.getMetadata(PATH_METADATA, handler)).toBe('receiveIban'); + expect(Reflect.getMetadata(METHOD_METADATA, handler)).toBe(RequestMethod.PUT); + }); + + it('guards the route with RateLimitGuard before OptionalJwtAuthGuard', () => { + const guards = Reflect.getMetadata(GUARDS_METADATA, handler) as unknown[]; + + // Order matters as documented on the route; the optional guard must be the auth guard, because a hard + // AuthGuard() would reject exactly the anonymous callers this endpoint exists to serve. + expect(guards).toEqual([RateLimitGuard, OptionalJwtAuthGuard]); + }); + + it('carries a route-level throttle, which is what gives the guard a limit at all', () => { + // RateLimitGuard resolves `routeOrClassLimit || this.options.limit`, and ThrottlerModule.forRoot() is + // registered without options - so without this decorator nothing would be throttled. + expect(Reflect.getMetadata(THROTTLER_LIMIT, handler)).toBe(60); + expect(Reflect.getMetadata(THROTTLER_TTL, handler)).toBe(60); + }); +}); + +// CheckReceiveIbanDto deliberately carries no @Transform. The Util transform helpers call string methods on +// the raw value - sanitizeString does value.trim() behind a bare truthiness check, trimAll does +// value?.replace(...) - and @Transform runs before @IsString, so a non-string body throws a TypeError that +// the exception filter turns into a 500 on a route reachable without a login. Adding a transform here would +// look like tidying up; these cases pin the boundary behaviour rather than the absence of a decorator. +describe('CheckReceiveIbanDto validation boundary', () => { + // Same configuration as the global pipe in main.ts. + const pipe = new ValidationPipe({ whitelist: true, transformOptions: { exposeUnsetFields: false } }); + const metadata = { type: 'body' as const, metatype: CheckReceiveIbanDto }; + + it.each([123, [], {}, null, undefined, ''])( + 'rejects %p with a BadRequestException, never a TypeError', + async (iban) => { + await expect(pipe.transform({ iban }, metadata)).rejects.toBeInstanceOf(BadRequestException); + }, + ); + + it('passes a plain string through unchanged, leaving normalization to the service', async () => { + await expect(pipe.transform({ iban: ' LI75-0881-1010-5923-K000E ' }, metadata)).resolves.toEqual({ + iban: ' LI75-0881-1010-5923-K000E ', + }); + }); +}); + +// These four strings are the wire contract: the client library carries its own copy of this enum, and the +// support form derives its wording from it. Comparing enum members would let a renamed value pass here and +// break at runtime in the browser instead, so the literals are asserted. +describe('ReceiveIbanStatus wire values', () => { + it('serializes to the strings the consumers expect', () => { + expect(ReceiveIbanStatus.DFX_IBAN).toBe('DfxIban'); + expect(ReceiveIbanStatus.NOT_MATCHED).toBe('NotMatched'); + expect(ReceiveIbanStatus.INVALID_IBAN).toBe('InvalidIban'); + expect(ReceiveIbanStatus.LOGIN_REQUIRED).toBe('LoginRequired'); + }); +}); 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 64667c5b93..53d11e3571 100644 --- a/src/subdomains/supporting/bank/bank/__tests__/bank.service.spec.ts +++ b/src/subdomains/supporting/bank/bank/__tests__/bank.service.spec.ts @@ -12,8 +12,12 @@ import { createDefaultUserData } from 'src/subdomains/generic/user/models/user-d import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; import { UserService } from 'src/subdomains/generic/user/models/user/user.service'; import { BankAccountService } from 'src/subdomains/supporting/bank/bank-account/bank-account.service'; +import { createCustomVirtualIban } from 'src/subdomains/supporting/bank/virtual-iban/__mocks__/virtual-iban.entity.mock'; +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 { + createCustomBank, createDefaultBanks, createDefaultDisabledBanks, yapealCHF, @@ -26,6 +30,7 @@ import { Bank } from '../bank.entity'; import { BankRepository } from '../bank.repository'; import { BankSelectorInput, BankService } from '../bank.service'; import { IbanBankName } from '../dto/bank.dto'; +import { ReceiveIbanStatus } from '../dto/receive-iban.enum'; function createBankSelectorInput( currency = 'EUR', @@ -69,6 +74,7 @@ describe('BankService', () => { { provide: FiatService, useValue: fiatService }, { provide: CountryService, useValue: countryService }, { provide: BankAccountService, useValue: bankAccountService }, + { provide: VirtualIbanRepository, useValue: createMock() }, TestUtil.provideConfig(), ], }).compile(); @@ -225,6 +231,7 @@ describe('Bank (name, currency) collision tie-break', () => { { provide: FiatService, useValue: createMock() }, { provide: CountryService, useValue: createMock() }, { provide: BankAccountService, useValue: createMock() }, + { provide: VirtualIbanRepository, useValue: createMock() }, TestUtil.provideConfig(), ], }).compile(); @@ -369,3 +376,235 @@ describe('Bank (name, currency) collision tie-break', () => { expect(BankService.isBankMatching(asset, 'YAPEAL-UNBOUND-NEWER-IBAN')).toBe(false); }); }); + +describe('BankService.getReceiveIbanStatus', () => { + const accountId = 42; + const otherAccountId = 43; + + // A retired collective account: same IBAN a customer may have transferred to years ago, but receive=false today. + const retiredCollectiveAccount = createCustomBank({ iban: 'CH5604835012345678009', receive: false, send: false }); + const personalIban = 'DE89370400440532013000'; + const expiredPersonalIban = 'AT483200000012345864'; + const foreignPersonalIban = 'CH4431999123000889012'; + + let service: BankService; + let bankRepo: BankRepository; + let virtualIbanRepo: VirtualIbanRepository; + + beforeEach(async () => { + bankRepo = createMock(); + virtualIbanRepo = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + imports: [TestSharedModule], + providers: [ + BankService, + { provide: BankRepository, useValue: bankRepo }, + { provide: VirtualIbanRepository, useValue: virtualIbanRepo }, + TestUtil.provideConfig(), + ], + }).compile(); + + service = module.get(BankService); + }); + + // The only shape getReceiveIbanStatus passes to findCachedBy; keeps the mock typed without `any`. + type AccountScopedWhere = { userData: { id: number } }; + + function setup(banks: Bank[], virtualIbansByAccount: Map = new Map()): void { + jest.spyOn(bankRepo, 'findCached').mockResolvedValue(banks); + jest + .spyOn(virtualIbanRepo, 'findCachedBy') + .mockImplementation( + async (_key: string | number, where: AccountScopedWhere) => virtualIbansByAccount.get(where.userData.id) ?? [], + ); + } + + it('reports a collective account IBAN as a DFX IBAN, without asking for personal IBANs', async () => { + // A collective account hit short-circuits for a logged-in caller too - no account-scoped lookup happens. + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus(olkyEUR.iban, accountId)).resolves.toBe(ReceiveIbanStatus.DFX_IBAN); + expect(virtualIbanRepo.findCachedBy).not.toHaveBeenCalled(); + }); + + it('reports a collective account IBAN as a DFX IBAN without a login, before ever asking for personal IBANs', async () => { + // The bank check must run before the login check, otherwise a logged-out customer gets LoginRequired for + // an IBAN we can already confirm. + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus(olkyEUR.iban)).resolves.toBe(ReceiveIbanStatus.DFX_IBAN); + expect(virtualIbanRepo.findCachedBy).not.toHaveBeenCalled(); + }); + + it('reports a collective account IBAN stored in paper format as a DFX IBAN', async () => { + // The stored side is normalized too, so a row that carries a grouped value still matches. + setup([createCustomBank({ iban: 'LU11 6060 0020 0000 5040' })]); + + await expect(service.getReceiveIbanStatus('LU116060002000005040', accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + }); + + it('reports a personal IBAN stored in paper format as a DFX IBAN', async () => { + // The comparison normalizes stored virtual_iban values as well, so their format need not be guaranteed. + setup( + createDefaultBanks(), + new Map([[accountId, [createCustomVirtualIban({ iban: 'de89 3704 0044 0532 0130 00' })]]]), + ); + + await expect(service.getReceiveIbanStatus(personalIban, accountId)).resolves.toBe(ReceiveIbanStatus.DFX_IBAN); + }); + + it('reports a collective account IBAN with receive=false as a DFX IBAN', async () => { + // A retired or closed account still received DFX money, and a missing transfer can predate it being stood down. + setup([retiredCollectiveAccount]); + + await expect(service.getReceiveIbanStatus(retiredCollectiveAccount.iban, accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + }); + + it('reports a personal IBAN of the requesting account as a DFX IBAN', async () => { + setup(createDefaultBanks(), new Map([[accountId, [createCustomVirtualIban({ iban: personalIban })]]])); + + await expect(service.getReceiveIbanStatus(personalIban, accountId)).resolves.toBe(ReceiveIbanStatus.DFX_IBAN); + expect(virtualIbanRepo.findCachedBy).toHaveBeenCalledWith(`user-${accountId}`, { userData: { id: accountId } }); + }); + + it.each([VirtualIbanStatus.EXPIRED, VirtualIbanStatus.DEACTIVATED, VirtualIbanStatus.RESERVED])( + 'reports a personal IBAN with status %s as a DFX IBAN', + async (status) => { + // An expired personal IBAN was still a real receiving IBAN, so no lifecycle state may be filtered out. + setup( + createDefaultBanks(), + new Map([[accountId, [createCustomVirtualIban({ iban: expiredPersonalIban, active: false, status })]]]), + ); + + await expect(service.getReceiveIbanStatus(expiredPersonalIban, accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + }, + ); + + it('reports a formally invalid IBAN as invalid, without querying any IBAN', async () => { + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus('DE123456', accountId)).resolves.toBe(ReceiveIbanStatus.INVALID_IBAN); + expect(bankRepo.findCached).not.toHaveBeenCalled(); + expect(virtualIbanRepo.findCachedBy).not.toHaveBeenCalled(); + }); + + it('reports a correctly shaped IBAN with a wrong checksum as invalid, not as unmatched', async () => { + // A changed digit keeps the country and length intact, so only the checksum catches it. Answering + // NotMatched here would send a customer looking for a transfer that never left with a typo in the IBAN. + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus('DE89370400440532013001', accountId)).resolves.toBe( + ReceiveIbanStatus.INVALID_IBAN, + ); + }); + + it.each([undefined, null, '', ' '])( + 'reports an unusable input (%p) as invalid instead of throwing', + async (input) => { + // Defensive only: @IsString/@IsNotEmpty reject undefined, null and '' with a 400 before the service is + // reached, so of these only ' ' can actually arrive. The typeof guard in normalizeIban short-circuits + // the non-string cases, and an all-separator string normalizes to '' and is returned as null. + // The cast stays because getReceiveIbanStatus itself declares `iban: string`; it is what lets the test + // reach the guard from outside the type system, which is exactly the situation the guard exists for. + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus(input as string, accountId)).resolves.toBe( + ReceiveIbanStatus.INVALID_IBAN, + ); + }, + ); + + it('reports a valid IBAN that matched neither list as not matched when the customer is logged in', async () => { + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus(foreignPersonalIban, accountId)).resolves.toBe( + ReceiveIbanStatus.NOT_MATCHED, + ); + }); + + it('requires a login for a valid unmatched IBAN, because personal IBANs stay unchecked without one', async () => { + setup(createDefaultBanks()); + + await expect(service.getReceiveIbanStatus(foreignPersonalIban)).resolves.toBe(ReceiveIbanStatus.LOGIN_REQUIRED); + expect(virtualIbanRepo.findCachedBy).not.toHaveBeenCalled(); + }); + + it('recognizes the same IBAN written with grouping spaces and in lower case', async () => { + setup(createDefaultBanks(), new Map([[accountId, [createCustomVirtualIban({ iban: personalIban })]]])); + + await expect(service.getReceiveIbanStatus('lu11 6060 0020 0000 5040', accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + await expect(service.getReceiveIbanStatus('de89 3704 0044 0532 0130 00', accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + }); + + // The invisible separators are written as escape sequences on purpose: it makes them visible in review and + // lowers the risk of an edit or a copy-paste quietly normalizing them into ordinary spaces, which would + // void exactly those cases. + it.each([ + ['an ASCII space', ' '], + ['a hyphen', '-'], + ['a dot', '.'], + ['a slash', '/'], + ['a non-breaking space', '\u00a0'], + ['a narrow non-breaking space', '\u202f'], + ['a zero-width space', '\u200b'], + ['a soft hyphen', '\u00ad'], + ['a tab', '\t'], + ['a line break', '\n'], + ])('recognizes an IBAN grouped with %s', async (_name, separator) => { + setup([frickEUR], new Map([[accountId, [createCustomVirtualIban({ iban: personalIban })]]])); + + const group = (iban: string): string => (iban.match(/.{1,4}/g) ?? []).join(separator); + + await expect(service.getReceiveIbanStatus(group(frickEUR.iban), accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + await expect(service.getReceiveIbanStatus(group(personalIban), accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + }); + + it('recognizes an IBAN pasted with surrounding quotes', async () => { + setup([frickEUR]); + + await expect(service.getReceiveIbanStatus('"LI75 0881 1010 5923 K000E"', accountId)).resolves.toBe( + ReceiveIbanStatus.DFX_IBAN, + ); + }); + + it('does not extract an IBAN out of surrounding ASCII words', async () => { + // Separators are stripped, an ASCII label is not: it survives normalization and makes the value invalid, + // which is what we want - a prefix is indistinguishable from extra characters that corrupt the IBAN. + // The guarantee is ASCII-only by construction: a label in a non-Latin script is stripped like a + // separator and the IBAN is accepted. Harmless, but the reason this test says "ASCII". + setup([frickEUR]); + + await expect(service.getReceiveIbanStatus('IBAN: LI75 0881 1010 5923 K000E', accountId)).resolves.toBe( + ReceiveIbanStatus.INVALID_IBAN, + ); + }); + + it('never reports a personal IBAN of another account as a DFX IBAN', async () => { + setup( + createDefaultBanks(), + new Map([ + [accountId, [createCustomVirtualIban({ iban: personalIban })]], + [otherAccountId, [createCustomVirtualIban({ iban: foreignPersonalIban })]], + ]), + ); + + await expect(service.getReceiveIbanStatus(foreignPersonalIban, accountId)).resolves.toBe( + ReceiveIbanStatus.NOT_MATCHED, + ); + }); +}); diff --git a/src/subdomains/supporting/bank/bank/bank.controller.ts b/src/subdomains/supporting/bank/bank/bank.controller.ts index 2e974f332c..ab4b35682a 100644 --- a/src/subdomains/supporting/bank/bank/bank.controller.ts +++ b/src/subdomains/supporting/bank/bank/bank.controller.ts @@ -1,8 +1,14 @@ -import { Controller, Get } from '@nestjs/common'; -import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; +import { Body, Controller, Get, Put, UseGuards } from '@nestjs/common'; +import { ApiBearerAuth, ApiOkResponse, ApiTags } from '@nestjs/swagger'; +import { Throttle } from '@nestjs/throttler'; +import { GetJwt } from 'src/shared/auth/get-jwt.decorator'; +import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; +import { OptionalJwtAuthGuard } from 'src/shared/auth/optional.guard'; +import { RateLimitGuard } from 'src/shared/auth/rate-limit.guard'; import { BankService } from './bank.service'; import { BankDto } from './dto/bank.dto'; import { BankMapper } from './dto/bank.mapper'; +import { CheckReceiveIbanDto, ReceiveIbanDto } from './dto/receive-iban.dto'; @ApiTags('Bank') @Controller('bank') @@ -16,4 +22,21 @@ export class BankController { return banks.map(BankMapper.toDto); } + + // PUT because the IBAN to check belongs in the body, never in the URL - this is a read, it changes nothing. + @Put('receiveIban') + @ApiBearerAuth() + // RateLimitGuard first; the route-level @Throttle below is what sets the limit. Deliberately more generous + // than the 10/60 on the one-shot endpoints (kyc 2fa/verify, auth mail login): RateLimitGuard buckets IPv4 + // callers by /24, so everyone behind one company NAT shares this counter, and the intended consumer is an + // input field meant to be re-checked while a customer corrects a typo. + @UseGuards(RateLimitGuard, OptionalJwtAuthGuard) + @Throttle(60, 60) + @ApiOkResponse({ type: ReceiveIbanDto }) + async checkReceiveIban( + @GetJwt() jwt: JwtPayload | undefined, + @Body() dto: CheckReceiveIbanDto, + ): Promise { + return { status: await this.bankService.getReceiveIbanStatus(dto.iban, jwt?.account) }; + } } diff --git a/src/subdomains/supporting/bank/bank/bank.service.ts b/src/subdomains/supporting/bank/bank/bank.service.ts index ae5215727d..f93cd8379d 100644 --- a/src/subdomains/supporting/bank/bank/bank.service.ts +++ b/src/subdomains/supporting/bank/bank/bank.service.ts @@ -1,13 +1,16 @@ import { Injectable, OnModuleInit } from '@nestjs/common'; +import * as IbanTools from 'ibantools'; import { Blockchain } from 'src/integration/blockchain/shared/enums/blockchain.enum'; import { Asset } from 'src/shared/models/asset/asset.entity'; import { DfxLogger } from 'src/shared/services/dfx-logger'; import { Util } from 'src/shared/utils/util'; import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; +import { VirtualIbanRepository } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban.repository'; import { FiatPaymentMethod } from '../../payment/dto/payment-method.enum'; import { Bank } from './bank.entity'; import { BankRepository } from './bank.repository'; import { IbanBankName } from './dto/bank.dto'; +import { ReceiveIbanStatus } from './dto/receive-iban.enum'; export interface BankSelectorInput { amount?: number; @@ -21,7 +24,12 @@ export class BankService implements OnModuleInit { private readonly logger = new DfxLogger(BankService); private static ibanCache: Map = new Map(); // key: "bankName-currency", value: iban - constructor(private bankRepo: BankRepository) {} + // The VirtualIbanRepository is injected instead of the VirtualIbanService: that service depends on this + // one, and both live in BankModule, so the service-level dependency would close a provider cycle. + constructor( + private readonly bankRepo: BankRepository, + private readonly virtualIbanRepo: VirtualIbanRepository, + ) {} onModuleInit() { void this.loadIbanCache(); @@ -118,8 +126,59 @@ export class BankService implements OnModuleInit { return expectedIban === accountIban; } + // --- RECEIVE IBAN CHECK --- // + + // Tells the client whether an IBAN typed in by a customer is one that belongs to DFX - not whether it still + // accepts money. Pure input aid for the support form: it enforces nothing, it only lets the frontend phrase + // a helpful hint. + async getReceiveIbanStatus(iban: string, userDataId?: number): Promise { + // normalizeIban strips separator characters and yields null for input that cannot hold an IBAN at all; + // it does not rescue every conceivable input (a `IBAN:` prefix stays invalid, correctly). Both sides of + // every comparison run through it, so a stored value in paper format matches too. + const normalizedIban = BankService.normalizeIban(iban); + if (!normalizedIban || !IbanTools.validateIBAN(normalizedIban).valid) return ReceiveIbanStatus.INVALID_IBAN; + + // Deliberately not filtered by `receive`: a hit on a retired or closed account is still money that went + // to DFX, and a missing transfer can predate the account being stood down. A receive=true filter would + // tell a real customer that their IBAN does not belong to DFX. + const banks = await this.getAllBanks(); + if (banks.some((b) => BankService.normalizeIban(b.iban) === normalizedIban)) return ReceiveIbanStatus.DFX_IBAN; + + // Personal IBANs are only ever checked for the requesting account. The guard is optional, so a global + // lookup would turn this endpoint into an unauthenticated oracle over customer-bound IBANs. Without a + // login the personal IBANs stay unchecked, hence the answer must never be NOT_MATCHED here. + if (!userDataId) return ReceiveIbanStatus.LOGIN_REQUIRED; + + // No lifecycle filter either (active=false, status Expired/Deactivated/Reserved all count): an expired + // personal IBAN was still a real receiving IBAN. Same cache key and filter as + // VirtualIbanService.getVirtualIbansForAccount, so both paths share the cached list. + const virtualIbans = await this.virtualIbanRepo.findCachedBy(`user-${userDataId}`, { + userData: { id: userDataId }, + }); + if (virtualIbans.some((v) => BankService.normalizeIban(v.iban) === normalizedIban)) + return ReceiveIbanStatus.DFX_IBAN; + + return ReceiveIbanStatus.NOT_MATCHED; + } + // --- HELPER METHODS --- // + // An IBAN is ASCII alphanumeric only, so everything else is separator noise: grouping spaces of any kind, + // hyphens, dots, slashes, quotes, and the invisible formatting characters that come along when a value + // is pasted out of a statement PDF or an HTML mail. Removing everything that is not ASCII alphanumeric + // covers every separator by construction, where chasing a deny-list did not - ibantools' own + // electronicFormatIBAN only removes ASCII spaces and hyphens, and \s misses the zero-width family. + // Note this also drops non-ASCII letters and digits, so a label in a non-Latin script is silently + // stripped rather than making the value invalid. Harmless (it can never produce a *different* valid + // IBAN), but it means only ASCII surroundings are reliably rejected. + // The parameter is widened past the callers' types on purpose: this sits on the trust boundary between a + // request body and the comparison, so it answers for anything the type system cannot actually guarantee. + private static normalizeIban(iban: string | null | undefined): string | null { + if (typeof iban !== 'string') return null; + + return iban.replace(/[^A-Za-z0-9]/g, '').toUpperCase() || null; + } + // Picks the bank row that owns attribution for a single (name, currency) key. `banks` must already // be sorted by id descending (newest first). Prefer a row linked to an asset: that binding is the // basis of every per-asset match (isBankMatching) and of the IBAN already present on booked diff --git a/src/subdomains/supporting/bank/bank/dto/receive-iban.dto.ts b/src/subdomains/supporting/bank/bank/dto/receive-iban.dto.ts new file mode 100644 index 0000000000..2d557f5941 --- /dev/null +++ b/src/subdomains/supporting/bank/bank/dto/receive-iban.dto.ts @@ -0,0 +1,20 @@ +import { ApiProperty } from '@nestjs/swagger'; +import { IsNotEmpty, IsString } from 'class-validator'; +import { ReceiveIbanStatus } from './receive-iban.enum'; + +// No @Transform(Util.sanitize) here: Util.sanitizeString calls value.trim() behind a mere truthiness check, +// and @Transform runs before @IsString - so a non-string body would throw a TypeError that the exception +// filter reports as a 500 instead of a 400, on an endpoint reachable without a JWT. The same defect class +// (an unguarded string method on an untyped transform value) sits in Util.trim and Util.trimAll too. +// Validation alone rejects a non-string cleanly; the service normalizes the string afterwards. +export class CheckReceiveIbanDto { + @ApiProperty() + @IsNotEmpty() + @IsString() + iban: string; +} + +export class ReceiveIbanDto { + @ApiProperty({ enum: ReceiveIbanStatus }) + status: ReceiveIbanStatus; +} diff --git a/src/subdomains/supporting/bank/bank/dto/receive-iban.enum.ts b/src/subdomains/supporting/bank/bank/dto/receive-iban.enum.ts new file mode 100644 index 0000000000..c4ceb81a22 --- /dev/null +++ b/src/subdomains/supporting/bank/bank/dto/receive-iban.enum.ts @@ -0,0 +1,21 @@ +export enum ReceiveIbanStatus { + // An IBAN that belongs to DFX: either a collective account from the bank table, or a personal deposit IBAN + // of the requesting account. It does not say the account still accepts money - getReceiveIbanStatus ignores + // the bank `receive` flag and every virtual_iban lifecycle state, so a long-closed account matches just as + // well. Phrase the hint as "belongs to us", never as "pay in here". + DFX_IBAN = 'DfxIban', + + // The IBAN could not be attributed for this caller. This does NOT claim that the IBAN does not belong to + // DFX: personal IBANs of other accounts are deliberately never checked, and mergeUserData does not move + // virtual_iban rows to the master, so a customer's own older personal IBAN can land here as well. + NOT_MATCHED = 'NotMatched', + + // The input is not a structurally valid IBAN (country, length or checksum), so there is nothing to look up. + INVALID_IBAN = 'InvalidIban', + + // No collective account matched, and personal IBANs are only ever checked for the authenticated account, so + // without a login the check stays incomplete. Never answered as NOT_MATCHED, which would overstate it. + // Tokens that carry no `account` claim get this too even though they are authenticated - company tokens + // (generateCompanyToken) are wallet-scoped, not account-scoped. Not a case the support form produces. + LOGIN_REQUIRED = 'LoginRequired', +}