From ac32fd9b39a4aab456e7464b77820c61b21eed63 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:45:33 +0200 Subject: [PATCH 1/4] feat(bank): add a receive-IBAN check for the support form (#4391) * feat(bank): add a receive-IBAN check for the support form The support form's "Receiver IBAN" field is a required dropdown filled from GET /bank, which lists only the shared bank accounts. Customers who deposit through a personal IBAN (virtual_iban) can never find their own receiving IBAN there, so today they are forced to pick a shared account they never transferred to - the ticket then carries a wrong IBAN that reads like a statement from the customer. The field is being replaced by free text, and this endpoint is what lets the frontend tell the customer straight away whether the IBAN they typed is one DFX receives money on. PUT /bank/receive-iban takes the IBAN in the body - never in the URL, so it stays out of access logs - and answers with one of four states: DfxIban, InvalidIban, UnknownIban, or LoginRequired. Two filters are deliberately absent. The bank lookup ignores `receive`, because a missing transfer is by nature an old one and a hit on a retired account is still money that reached DFX; filtering would tell a real customer their IBAN does not belong to DFX. The personal-IBAN lookup ignores the lifecycle state for the same reason - an expired or deactivated personal IBAN was still a real receiving IBAN. Personal IBANs are matched only against the requesting account. The guard is optional, so a global lookup would turn this into an unauthenticated oracle over customer-bound IBANs. That is why LoginRequired exists as its own state: without a login the personal IBANs stay unchecked, and answering UnknownIban there would be a false statement to a customer whose IBAN does exist. The endpoint is an input aid only - it enforces nothing, and issue creation still accepts any free text. Comparison runs on the normalised electronic format, since both the stored values and customer input carry arbitrary grouping spaces and casing. BankService takes the VirtualIbanRepository rather than the VirtualIbanService, because that service already depends on BankService and both live in the same module. * fix(bank): correct IBAN normalization and input handling in the receive-IBAN check Review turned up four real defects in the first commit. Normalization stripped only whitespace, so a customer pasting an IBAN with hyphen grouping got InvalidIban for a perfectly valid IBAN. ibantools ships electronicFormatIBAN, which strips both spaces and hyphens and uppercases, so the hand-rolled helper is gone in favour of the library. It also returns null for a non-string, which is now treated like any other unusable input - and it makes the stored side null-safe, where the old regex would have thrown. The DTO no longer carries @Transform(Util.sanitize). That transform runs before @IsString, and Util.sanitizeString calls value.trim() unguarded, so a body such as {"iban": 123} threw a TypeError that the exception filter turned into a 500 - on an endpoint that is reachable without a login. HTML sanitizing is pointless for an IBAN that is structurally validated and normalized anyway. @IsString now rejects a non-string with a clean 400. A comment records why the transform must not come back. UnknownIban was renamed to NotMatched, because the old name asserted more than the check knows. For an authenticated caller the state means "could not attribute this", not "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 there too. Every state is now documented in the enum, including that NotMatched makes no claim about DFX ownership. The endpoint is reachable unauthenticated, so it now runs behind RateLimitGuard, placed first as in the existing public endpoints. Two tests were added: hyphen-grouped input resolves to DfxIban, and unusable input yields InvalidIban instead of throwing. * fix(bank): accept every whitespace style when normalizing an IBAN Switching to electronicFormatIBAN in the previous commit traded one gap for another. The library strips only ASCII spaces and hyphens; the hand-rolled helper it replaced stripped every kind of whitespace but no hyphens. Measured against ibantools 4.5.1 with a valid IBAN in different groupings, the library alone rejects a non-breaking space, a narrow non-breaking space, a tab and a line break, while accepting ASCII spaces and hyphens. That is not an edge case for this endpoint. An IBAN pasted out of a PDF statement or off a web page very often carries a non-breaking space as its grouping character, and the whole point of the free-text field is that customers paste what they have. Such a customer would have been told their perfectly valid IBAN is invalid. The normalization helper is back, now doing both: it strips all whitespace itself and then hands the value to electronicFormatIBAN, which removes hyphens, uppercases, and returns null for anything unusable. A typeof guard keeps that null-safety for stored values as well. All three comparison sites - the input, bank.iban and virtualIban.iban - run through it. The comment that implied the library normalized completely is corrected. Four cases were added covering non-breaking space, narrow non-breaking space, tab and line break, for a collective account and for a personal IBAN. The separators are written as escape sequences so no invisible character can be lost while editing. The guard was checked by reverting the helper to the library-only form: exactly those four cases fail, and pass again once it is restored. * fix(bank): normalize an IBAN by allow-list and close the untested branches Three safeguards of the receive-IBAN check turned out to be unpinned: inverting them left every test green. The branch order that answers a logged-out customer with DfxIban on a collective account - the single most common path through this endpoint - could be swapped for LoginRequired unnoticed. Checksum validation could be reduced to a shape check, which would answer a customer who mistyped a digit with NotMatched, telling them their IBAN is not ours instead of that it has a typo. And normalization could be dropped from the stored side of either comparison, which matters for personal IBANs because those values are persisted straight from the provider response without validation. One test each now pins these, and each mutation is caught by exactly its own test. Normalization changes approach rather than gaining another character. Stripping whitespace missed the zero-width family (U+200B, U+200D, U+2060, the direction marks) and the soft hyphen, none of which JavaScript counts as whitespace, plus dots, slashes and quotes. That was the third defect in the same function, so the deny-list of separators is replaced by an allow-list of what an IBAN may contain: letters and digits, nothing else. That is complete by construction. An IBAN: prefix stays invalid, which is correct - it is not a separator problem. Three comments overstated what they knew and are corrected. The normalization comment no longer promises to absorb every pasted form. The rate-limit comment no longer promises protection: ThrottlerModule.forRoot() is called without options, so limit is undefined and the guard's comparison is always false - the guard is inert until that is fixed separately, and the comment now only explains the ordering. The DTO comment now names the actual cause of the 500 it avoids, an unguarded value.trim() in Util.sanitizeString, rather than arguing that HTML sanitizing is pointless for an IBAN. Also moved the section header so it no longer encloses the unrelated isBankMatching, and made the constructor uniform by adding the missing readonly. * test(bank): pin the guards, the throttle and the validation boundary Mutation testing found three safeguards that could be removed without a single test noticing. Deleting @UseGuards entirely, or swapping OptionalJwtAuthGuard for a hard AuthGuard, left all tests green. Both are severe: without the optional guard req.user is never populated and every authenticated customer is told LoginRequired, while a hard guard turns anonymous callers away with a 401. The controller spec now asserts the route metadata the way ledger.controller.spec.ts does - path, method, and the guard list including its order - so both failure modes are caught. Removing the DTO validators, or "harmonizing" them with the @Transform(Util.trimAll) that every other IBAN DTO in the project carries, also went unnoticed. The second one is the realistic edit, and it would turn the deliberate 400 back into a 500 on a route reachable without a login. The DTO is now driven through a ValidationPipe built from the exact options in main.ts, asserting a BadRequestException for a number, an array, an object, null, undefined and an empty string, plus a positive case proving a plain string arrives unchanged - which is what catches a transform being added. The throttle is raised from 10 to 60 per minute. RateLimitGuard buckets IPv4 callers by /24, so a whole company network shares one counter, and unlike the one-shot precedents it was copied from - 2FA verification, mail login - an IBAN field gets re-checked while a customer corrects a typo. Ten would have meant two colleagues in one office locking each other out of the form they opened because something already went wrong. Two comments claimed more than the code delivers. The normalization is complete only for ASCII: non-ASCII letters and digits are stripped as separators too, so a label in a non-Latin script can be dropped where an ASCII one is not. That cannot produce a different valid IBAN, but the comment and the test name now say what actually holds. And DfxIban is phrased as belonging rather than as an invitation to pay in, because most matching rows are retired accounts. * refactor(bank): use a camelCase route and correct two overstated comments CONTRIBUTING documents camelCase for URL routes, and the closest sibling for this concept - GET /buy/personalIban - follows it, as do the two read-with-a-body precedents this endpoint was modelled on. The hyphenated route was an unjustified deviation, and renaming it is free right now: the endpoint is unreleased and has no consumer in production. Once the client library ships the call, the same rename would break everyone using it. Only the route literal and the two metadata assertions change; filenames, the enum, the DTO and the method names stay. Two comments claimed more than had been checked. The first said every other IBAN DTO in the project carries @Transform(Util.trimAll); three admin DTOs do not - update-bank-tx and create/update-fiat-output. Narrowed to customer-facing IBAN input DTOs, which is both true and the sharper form of the argument, since those are what someone would align against. The second said the bank table holds grouped IBAN values. Production stores all eighteen rows compact and uppercase; the only grouped value is a test fixture. What actually holds is the invariant behind it: no service writes bank.iban - rows arrive through migrations or by hand - and nothing normalizes the column on write, so a row can carry a grouped value at any time. The test that covers it was right and is unchanged. * docs(bank): argue the missing transform from mechanism, not from a census Three attempts at one comment, each narrowing a claim about what every other IBAN DTO carries, and each still wrong - the last counterexample being create-support-issue, which is customer-facing, login-optional and uses Util.sanitize rather than trimAll. The lesson is not a fourth narrowing. Any claim of the form "every other DTO does X" is either already false or becomes false with the next DTO, and it was never load-bearing: the argument works from mechanism alone. The Util helpers call string methods on the raw value, @Transform runs before @IsString, and a non-string body therefore becomes a TypeError that the exception filter turns into a 500 on a route reachable without a login. That transforms exist on other IBAN fields is enough to explain why adding one here would look like tidying up; how many and which is irrelevant. I swept the remaining comments in the diff for the same shape. Everything else states either this code's own behaviour or a fact measured directly, so no claim now depends on an inventory that can drift. Also names the seed CSV as a third way rows reach the bank table, alongside migrations and manual inserts. * docs(bank): drop every comment claim that depends on an unnamed inventory A comment may describe this code, or name a location a reader can open. It must not quantify over an unnamed set of other files or over production data, because such a claim goes stale invisibly. Three statements still did, and the pattern across the previous rounds was to weaken the quantifier rather than remove the dependency. Removed: that transforms sit on other IBAN fields, which was the fourth variant of the same sentence and carried nothing the mechanism argument had not already established; that no service writes bank.iban, a universal negative over present and future services whose load-bearing half was only ever the column; and that the guard order matches the existing public endpoints. The 60/60 rationale stays, because it names the two endpoints it compares against. The enum lost "most bank rows are retired" for the same reason - it quantifies over the contents of the production table and cannot be checked from the repository. The warning it carried is now grounded in the method instead: the check ignores the receive flag and every lifecycle state, so a long-closed account matches just as well. The merge caveat stays, rewritten to name mergeUserData, since a named function is re-checkable in one grep and the caveat is one of the two reasons NotMatched must not be read as "not a DFX IBAN". The audit also caught the method summary still describing the endpoint in the present tense as reporting an IBAN DFX receives money on, the same framing corrected in the enum a round earlier, and one stale route spelling in a test comment. * docs(bank): drop the last comment clause that quantifies over write paths The previous commit removed one half of this sentence and kept the other, which was the same shape with the verb swapped: "nothing normalizes bank.iban on write" is a universal negative over an unnamed set of write paths, and it goes stale the moment one appears. It was also never the point of the comment - what the test demonstrates is that the comparison normalizes the stored side, which is a statement about this code and needs no inventory at all. * test(bank): type the spec helpers and pin the short-circuit for a logged-in caller Two independent review lanes on the final state turned up eight items. The ordering of the checks was only pinned for an anonymous caller. A logged-in caller whose IBAN matches a collective account must also skip the account-scoped lookup, and nothing asserted that - running the personal lookup eagerly would have passed. That test now asserts it, and inverting the order fails exactly it. The spec helpers carried an untyped mock argument and no return types. The filter shape is now a named local type, so the mock is typed without any. Five comments claimed more than was checked. Two stated frequencies over customer behaviour - that a missing transfer is often old, and that the logged-out collective case is the most common by far. Neither is knowable from here, and both rationales stand without the quantifier. One claimed personal IBANs arrive unvalidated from the provider, which is false for one of the two providers; it now speaks only about this comparison normalizing stored values. One called a fixture a transposed digit where it substitutes one. One asserted that editors convert invisible separators, which is not generally true - the honest reason for escape sequences is that they make the characters visible in review, and that writing them literally went wrong twice here. And the throttle rationale described a consumer re-checking the field in the present tense, for a consumer that does not exist yet. * test(bank): pin the four status strings as the wire contract The enum values are what three repositories agree on: the client library carries its own copy, and the support form derives its wording from them. Every test so far compared enum members, so renaming a value would have passed here and broken at runtime in a customer's browser instead. The literals are now asserted; changing one fails exactly that test. One comment also went out claiming the separator cases had been written literally twice by mistake. That history is not in the repository - the cases arrived escaped and stayed escaped - so for any reader it is unverifiable. The verifiable half of the rationale stands on its own: escape sequences make the characters visible in review and lower the risk of an edit normalizing them away. --- .../bank/__tests__/bank.controller.spec.ts | 110 ++++++++ .../bank/bank/__tests__/bank.service.spec.ts | 239 ++++++++++++++++++ .../supporting/bank/bank/bank.controller.ts | 27 +- .../supporting/bank/bank/bank.service.ts | 61 ++++- .../bank/bank/dto/receive-iban.dto.ts | 20 ++ .../bank/bank/dto/receive-iban.enum.ts | 21 ++ 6 files changed, 475 insertions(+), 3 deletions(-) create mode 100644 src/subdomains/supporting/bank/bank/__tests__/bank.controller.spec.ts create mode 100644 src/subdomains/supporting/bank/bank/dto/receive-iban.dto.ts create mode 100644 src/subdomains/supporting/bank/bank/dto/receive-iban.enum.ts 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', +} From e26f73928185d69717a153d4209b30d607c0bdd0 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:52:12 +0200 Subject: [PATCH 2/4] fix(custody): do not double a day's portfolio value on duplicate price rows (#4412) * fix(custody): do not double a day's portfolio value on duplicate price rows The daily value summed balance x price over every price row of that day, assuming at most one row per asset and day. That assumption does not hold: asset_price.created is a local-time timestamp while the grouping uses UTC, so a snapshot taken shortly after local midnight lands in the previous UTC day. That day then carries two rows for the same asset and its value is reported at double. Measured against a real 455-day price series of one asset: six days carried two rows, and the value chart spiked to twice the correct amount on exactly those six days. Every customer with a Safe sees that chart. The daily value now takes the latest price per asset, decided by timestamp rather than by query order. Days with a single row are unaffected, which the added test pins down alongside the duplicate case. * fix(custody): break ties on equal price timestamps by id Two price rows with the same created timestamp kept whichever came first in the list, and the query orders only by created - so identical data could produce different chart values. The higher id is the later insert and now wins, decided from the data rather than the list order. --- .../__tests__/custody.service.spec.ts | 151 ++++++++++++++++++ .../core/custody/services/custody.service.ts | 50 ++++-- 2 files changed, 191 insertions(+), 10 deletions(-) create mode 100644 src/subdomains/core/custody/services/__tests__/custody.service.spec.ts 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.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(); From a6016683a0087ad7444897a858909fe35ab68e46 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Tue, 28 Jul 2026 07:48:21 +0200 Subject: [PATCH 3/4] feat(custody): let an owner's own grant narrow their access level (#4417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(custody): let an owner's own grant narrow their access level Until now the owner of a custody account always received WRITE, and the authorisation actually granted on that account was never consulted. That made one arrangement impossible to express: an authorisation in which the owner keeps inspection only and reserves acting for someone else. An active grant an owner holds on their own account now decides their level. Without such a grant nothing changes — the owner keeps full disposal, which is every account in production today. Managing grants stays tied to ownership rather than to the level, so an owner who narrows themselves can still hand the mandate back at any time and cannot lock themselves out. Reading is unaffected as well: a narrowed grant withdraws acting, not sight, and the holdings are the owner's either way. Adds the first test suite for CustodyAccountService, covering both paths and in particular that an inactive grant never narrows anything — deactivated history must not take part in authorisation. * feat(custody): enforce the access level where acting actually happens Two review findings, both real. First, the narrowing was unreachable. Self-grants are refused and the owner's grant row could not be modified, so no API path led to the state the previous commit reacts to. An owner may now re-level their own grant — limiting themselves to inspection and taking the mandate back. Revoking that row stays refused: it would leave the account without an owner row and make the level unrecordable. Second, and worse: order creation never consulted the access level at all. It runs under the custody role and reaches the Safe on its own, so an owner limited to inspection could still trade. Hiding buttons in the frontend would have been decoration — the API accepted the order regardless. createOrder and confirmOrder now refuse when an own account is limited to inspection. Orders address a whole Safe rather than a single account, since balances and orders carry no account today. Any own account narrowed to READ therefore blocks acting: the order could touch exactly those holdings. Fail closed rather than guess which account an order belongs to. Verified against a running instance in both directions: with the narrowing in place the order is refused with 403, and with the mandate restored the very same request succeeds. Accounts without a narrowing grant — every account in production today — are unaffected. * fix(custody): keep a narrowing in force on a blocked account, cover the order paths Two review findings. The acting check only looked at active accounts. Blocking or closing an account would therefore have lifted the restriction — exactly when caution matters most. Everywhere else a non-active account counts as absent and grants nothing; here absence would have granted something, namely the right to act. The status filter is gone and the test now insists a blocked account stays restricted. Not exploitable today, since no code path ever sets that status, but it would have been a trap for the first account-blocking feature. CustodyOrderService had no test suite either, so the two new call sites rested on manual verification alone. It now has one: both paths refuse when acting is narrowed, both pass the right identity rather than one of the two JWT ids, and a stranger is turned away on ownership before the narrowing check runs — so nobody can learn from the response whether an account is restricted. Also documents why the gap between check and write is left unlocked: 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. * fix(custody): keep grant management reachable on a blocked account A narrowing blocks the owner's whole Safe and deliberately ignores account status. Grant management, however, went through requireOwner, which demanded an active account. Blocking a single account would therefore have stranded the grant on it: the owner could neither lift a narrowing they had placed there nor withdraw a stranger's access, and since one narrowing blocks every account they hold, a single block would have frozen the whole Safe with no way back. Grant management now depends on ownership alone. Blocking an account governs what may be done with it, not who decides that. Missing and foreign accounts still yield the same Forbidden, so existence stays unprobeable. Also corrects a comment that still described the status filter removed in the previous commit, and sorts an import. * docs(custody): correct the docstring on getCustodyAccountById It still claimed to be shared by checkAccess and requireOwner. Since the last commit requireOwner resolves the account itself, without the status filter, so that blocking an account cannot strand the grants on it. Only the data path goes through here now. * fix(custody): refuse to issue new grants on an account that is not active Making grant management independent of account status went one step too far. It was meant to keep an owner from being stranded — able to lift a narrowing they placed on a blocked account, or withdraw a stranger's access. Issuing a new grant is neither. 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. grantAccess now refuses on a non-active account, and does so before resolving the address, so the response cannot reveal whether a mail address is registered. Withdrawing and re-levelling stay open. grantAccess and getAccessList had no tests at all; both are covered now, including that an owner can still inspect grants on a blocked account. The blocked-account test for updateAccess used an unconditional stub that would have stayed green if a status filter crept back into requireOwner — it now evaluates the where clause, verified by reintroducing the filter and watching the test turn red. * fix(custody): refuse to raise a stranger's level while an account is held Refusing new grants on a held account left the same door open one step over: raising an existing grant from inspection to acting widens someone's authority just as much, only through an existing row instead of a new one. It takes effect the moment the account is released, without anyone looking again. The rule is now uniform for a held account: rights may be taken away, not handed out. Lowering a stranger stays open, as does anything on the owner's own row, which is their way out of a narrowing. Uses BadRequestException rather than ConflictException, matching six existing places that refuse an action on an inactive or blocked resource; Conflict is reserved for duplicates. * chore(custody): tidy up after the review rounds Removes an import left unused when the exception type changed, uses the nullish operator the repo prefers, and records two assumptions that were only in my head: elevation is recognised by comparing the two levels that exist, so a third would have to turn it into an ordering comparison; and the status is read before the write, so whatever introduces a hold must deactivate that account's grants in the same change. Also covers granting write on a held account, not just read — the refusal must not depend on the level asked for. --- .../__tests__/custody-account.service.spec.ts | 992 ++++++++++++++++++ .../__tests__/custody-order.service.spec.ts | 285 +++++ .../services/custody-account.service.ts | 169 ++- .../custody/services/custody-order.service.ts | 12 +- 4 files changed, 1423 insertions(+), 35 deletions(-) create mode 100644 src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts create mode 100644 src/subdomains/core/custody/services/__tests__/custody-order.service.spec.ts 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/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()); } From aa79ebc38bd928d9c9d0abf6a85ec892135014a6 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Tue, 28 Jul 2026 08:17:22 +0200 Subject: [PATCH 4/4] Treat an unobserved order outcome as unknown instead of failed (#4405) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Treat an unobserved order outcome as unknown instead of failed A liquidity order whose request left our side without an observed answer was recorded as Failed. That asserts knowledge we do not have, and it is not inert: a failed pipeline pauses its rule, and the rule auto-reactivates after reactivationTime, so the same request is issued again a few minutes later. Two Scrypt withdrawals hit exactly this path and were mis-recorded. Both requests timed out waiting for the BalanceTransaction update; both were stored as Failed with no correlationId, and both had in fact executed at the venue - the exchange transaction and the balance drop match the order amount to the cent. No double withdrawal resulted, but only because the balance had already fallen by the time the rule retried. That balance is served from a push cache with no freshness guarantee, so the safeguard cannot be relied on in the very situation that produces the timeout. Four changes, following the payout subdomain, which already solves this problem for blockchain broadcasts: 1. Reserve the venue reference before sending. Scrypt is the one integration that lets us choose it (ClOrdID / ClReqID), but it was generated inside the service and only returned on success, so a timeout lost it for good and left nothing to look up. Integrations may now supply a reference through reserveCorrelationId, which the pipeline persists before the request goes out; the id is derived from the order id, satisfying the venue's daily uniqueness requirement without a random component. 2. Add LiquidityManagementOrderStatus.UNCERTAIN, the counterpart to PAYOUT_UNCERTAIN. It is terminal for the pipeline - checkRunningPipelines already leaves such an order alone, so no rule is paused and nothing auto-reactivates - but not for the order. 3. Classify the send boundary fail-closed. Request timeouts now carry their own error type rather than a message string, so they can be told apart from a dropped socket, which proves the request never completed. Only the latter stays an ordinary failure. As a consequence idempotent reads - fetch and fetchAll, which cannot affect venue state - retry on timeout instead of ending the whole order; that alone covers 47 of the 49 timeouts observed over two weeks. 4. Reconcile instead of repeat. resolveUncertainOrders asks the venue what happened and never re-sends. Absence only counts as proof after a grace window, an unreachable venue leaves the order in quarantine, and reconciliation runs before any new order is issued. Observability, so a quarantined order is not silently parked: uncertainLmOrderCount is exposed on the liquidity observer, the quarantine mail is pinned per order and debounced, and the pipeline failure mail is pinned per rule and debounced - one incident used to send a mail per retry. The success mail is dropped; it accounted for 211 of 255 liquidity mails in a week and carried no information, which is what made the mails that matter unreadable. Also fixes an unrelated spin: an error outside the three known exception types left the order in Created, and startNewOrders reported a change regardless, so the caller's while loop could not terminate. * Close the remaining unconfirmed-outcome paths found in review Three gaps in the first pass, all of the same shape as the bug it fixes. The amend boundary was still open. checkTrade may cancel-replace or restart an order from inside the completion check, and both created a fresh reference that was never recorded, so a replacement whose confirmation never arrived could not be looked up even in principle - and the error fell through to OrderFailedException, which pauses the rule and reissues the trade. Callers now supply the replacement reference, derived from the order row as -, so it is reproducible without an extra column. An unconfirmed outcome there quarantines the order, and reconciliation enumerates the replacement candidates rather than only the current reference. "Connection closed" was treated as proof that nothing was sent. It is not: requestWithId hands the payload to the socket before registering the pending entry, and a later close rejects that entry with a generic message that says nothing about whether the venue acted. The classification is now fail-closed - only an explicit rejection from the venue, which proves the request was seen and refused, keeps an error an ordinary failure; everything else is an unknown outcome. Over-classifying is self-correcting, because an error that truly happened before the send leaves no trace at the venue and reconciliation settles the order as failed after the grace window. hasPendingOrders did not count quarantined orders. That gate is what stops a second rule on the same exchange from acting while funds are unaccounted for, so an unresolved order has to block there. Same reasoning applied to getProcessingOrders, and to getPendingTx, where omitting the status would have dropped the amount in question out of the financial log. Alert debouncing was keyed by rule alone. Suppression compares only the key, never the body, so a genuinely different failure of the same rule was swallowed within the window. The key now includes a digit-insensitive hash of the cause: true repeats still collapse, a new cause still reports. Keying by pipeline instead would have restored the flood, since every retry is a new pipeline. Tests: the amend and reconciliation paths, quarantine on socket close, a venue rejection staying an ordinary failure, replacement-reference derivation, a pipeline left untouched while its last order is quarantined, and the timeout type and read retry in the connection itself. * Make the amend boundary actually reachable, and widen the pending checks Follow-up to the previous commit, from a second review pass. The amend fix did not fire. checkTrade's inner catch swallowed every error that was not TradeChangedException, attempted a best-effort cancel and returned false, so a timeout or a dropped socket during editOrder never reached the classification added for it - the order kept polling the old reference while a replacement created under the reserved one stayed invisible, and reconciliation never ran because the order never entered quarantine. Unless the venue explicitly rejected the amend, that path now raises a dedicated unconfirmed-write error, and the same guard wraps the restart in the cancelled branch. That error type is what the check path was missing. Message matching cannot separate a dropped socket on a read from one on a write, yet the first is harmless and the second is unresolved; the completion check now tests for the write boundary before the transient-error branch, which previously classified a connection drop during a restart as retry-next-tick. The rejection markers move next to the error types so the send path and the check path share one definition, extended with the edit rejection. The custom-asset balance sum was left out of the previous pass on the grounds that Scrypt is not among its systems. That reasoning was wrong: the generic quarantine in startNewOrders is not system-specific, so a Kraken or Binance order can reach the status too, and neither adapter can resolve it - its amount would silently drop out of the balance for as long as it stayed there. Tests cover the amend boundary end to end: an unconfirmed write surfaces as an unknown outcome, carries the replacement reference into the quarantine reason, and a plain dropped connection on the read path still resolves as retry-next-tick. The PR description has been rewritten to the final state; it still described the first iteration and, on the dropped-socket classification, asserted the opposite of what the code now does. * Cover the amend write boundary at the level where it used to be swallowed The previous commit made an unconfirmed amend propagate out of checkTrade, but the guard was only exercised through the adapter. The defect lived one layer below, in the service, and the service had no checkTrade tests at all - which is why the first attempt at this fix looked right and did nothing. Three tests at that level: an unconfirmed amend leaves checkTrade as an unconfirmed-write error instead of being cancelled away and reported as "not complete"; the raised error carries the reserved replacement reference, so the order can be reconciled against it; and an amend the venue explicitly rejected still falls back to cancel-and-continue, since a rejection proves nothing was created. * Stop inferring outcomes that were never observed, in four more places A cross-vendor review of the previous state found four more spots where the code still concluded something it had not seen. Each is the same mistake the PR set out to remove. A CREATED order that already carries a reserved reference is no longer re-sent. That combination can only arise when a previous pass reached the send boundary and died before recording the result, so sending again is precisely the duplicate we are trying to avoid; it now goes straight into quarantine. The catch-all no longer quarantines everything. Whether a reference was reserved tells us whether the send boundary was crossed at all: without one, nothing can have been transmitted and an ordinary failure is correct. Quarantining those stranded configuration and factory errors in a state only a human can clear - and, since reconciliation needs the same integration that just failed to load, they could never clear themselves. Reconciliation checked the oldest reference first. A replaced order usually still exists at the venue in a cancelled state, so the original matched, the order was reported as sent, and the live replacement stayed untracked while the completion check polled a dead reference. Candidates are now tried newest first. Absence is no longer treated as proof of non-arrival. The venue offers no terminal "this reference was never accepted" reply, so a missing record after any amount of time is not evidence; concluding otherwise is what would let a rule reissue a request that later materialises. Such an order stays quarantined for a human, and its rule stays blocked. The grace window existed only to make that inference safer and is gone with it. Two more, from the same principle. A failure to READ an order the venue has acknowledged no longer fails it - that would release the rule to open a second position against the same funds - but is retried, with a dedicated type for "acknowledged, then vanished" that quarantines instead of failing. And the quarantined status is taken back out of the financial log's pending set: the log adds a pending amount back to the balance and nets it against the venue's locked funds, but an unsent order locks nothing, so counting it would inflate equity by its full amount - the one error direction that can hide a real loss from the safety threshold. * Give a venue rejection its own type, and a quarantined order a way out A follow-up review found four more places where the code still had to guess. Rejections were recognised by message text, and one real terminal path phrased its message differently ("Order has been rejected"). It matched nothing, so after the previous commit made unmatched errors retry instead of fail, a genuinely rejected order would have been retried forever. Rejections now carry their own type, used by every path that turns a venue refusal into an exception: it cannot be missed by rephrasing, and a transport error that happens to quote the phrase can no longer masquerade as a settled outcome. Reconciliation recovered a withdrawal from the venue's history but never cached it, while getWithdrawalStatus reads only the cache. An order leaving quarantine on the strength of that lookup would have polled a reference the cache still did not know, and never completed. The match is now fed back through the same terminal-aware cache write as a live push. A bare request timeout in the completion check can only come from a read, because every write there is already wrapped. Quarantining it stranded an order that nothing at the venue had touched, so it is retried like any other read problem. And the gap left by the previous commit: with absence no longer proving non-arrival, nothing released a genuinely unsent order, so its rule would stay blocked indefinitely with no operator action available. There is now a guarded admin endpoint, modelled on the payout subdomain's retry guard - the caller must assert that the venue was checked and name where, and the assertion is recorded on the order. Also corrected: a comment claimed the socket send precedes registering the pending request, where the code does the reverse; the conclusion it supported is unaffected, since what makes a close ambiguous is that the bytes may already be on the wire. Stale grace-window wording removed from code and description, and the description now matches the final behaviour, including the deliberate exclusion of the quarantined status from the financial log's pending set. * Settle the last paths where a refused write could still repeat Seventh pass. Six findings taken, one declined. A refused amend now reports itself. checkTrade swallowed the refusal and returned "not complete", so the caller never learned that the replacement reference was spent - the venue requires them to be unique, so the next tick derived the very same one and the pair could loop. The refusal carries the spent reference, the order records it, and the derivation moves on while tracking stays on the original, which a refused amend leaves live. Reconciliation no longer adopts a rejected replacement. Any returned record counted as proof of sending, so a replacement the venue had refused would be adopted, immediately fail the order, and release the rule although the original was still standing. Both resolution paths now write conditionally. Automatic reconciliation and an operator can hold the same order at once, and an unconditional save let whoever finished last win - including failing an order the venue had just confirmed as live. The status is part of the WHERE clause and a lost race is skipped, or reported as a conflict on the manual path. A venue error reply is no longer a plain error. It carries its own type, deliberately distinct from 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. A cached withdrawal record that is not terminal no longer short-circuits the history lookup - the missing terminal push is exactly what such a record would be hiding. The manual endpoint was too thin: the asserted flag and the caller were dropped at the edge, a whitespace-only reference passed validation, and the log line preceded the write. The service now re-asserts the claim, normalises and caps the reference, records who authorised it, persists before logging, and returns nothing rather than the entity. DECLINED: extending this to the ccxt venues. They share the gap, but they have no client order id today and no reconciliation lookup, so propagating the unknown outcome would quarantine orders across every exchange with no exit but a human - a worse position than today, on venues where nothing has gone wrong. Recorded in the description as its own piece of work. * Bound the audit reference and record the consumer impact Conformity pass. The manual-resolution reference was unbounded while both analogous DTOs in the repository cap it at 1024 characters, and it was written to the log untruncated; the cap now lives in the DTO, so the service records the validated value instead of silently shortening it. Two update mocks in the tests dropped their types for no reason and are typed again. On the contract question: the new order status appears in no DTO, and both endpoints that expose these orders are admin-guarded and excluded from the published surface, so there is no public contract change. Whether the internal admin view enumerates statuses exhaustively cannot be determined from this repository, so the description asks the reviewer to confirm it rather than leaving the question unasked. * Let an observation outrank a judgement when both resolve the same order The compare-and-set added earlier decides who writes first, and first is not the same as right. If an operator releases a quarantined order in the same moment reconciliation confirms the request exists at the venue, the operator's write lands, the order is marked failed, the pipeline releases and the rule can issue a second order against a live position. The manual path now asks the venue itself before releasing, and refuses when the reference is found - a positive observation outranks the operator's judgement, and the order is left for the next reconciliation pass to move on. A lookup that cannot be performed does not block the release: the operator has asserted an independent check, and a reference the venue can no longer be asked about must not turn into an order nobody can ever clear. * Make a replacement reference durable before it is sent, and bound the unobservable case Three findings from the sixth pass, all of them consequences of the previous round's fixes. A replacement reference was only recorded once it was accepted or explicitly refused. In between it was neither the current reference nor a spent one, so a replacement whose confirmation was lost could be derived a second time. The reference is now claimed and persisted immediately before the request goes out, for both the amend and the restart. Reconciliation could still fall through to a superseded predecessor. An accepted replacement may simply not be visible at the venue yet, while the order it replaced still is; matching the older one reported the request as sent and left the live replacement untracked. An older reference is now considered only after the newer one was explicitly rejected - anything else keeps the order quarantined. A positive observation on the manual path was refused but not recorded. The row stayed quarantined, so a later attempt made while the venue happened to be unreachable could still release it and undo what had been seen. The observation is now persisted first, which takes the order out of manual reach entirely. And an acknowledged order that cannot be observed no longer polls forever. It was kept deliberately - failing it would release the rule against a live position - but the manual path only accepts quarantined orders, so there was no way out at all. Past the same age at which the venue itself is considered to have lost an order, it is quarantined: still not declared failed, but now reachable for a human. * Reconcile only references that were really sent The seventh pass found that the previous commit had broken the very case this work exists for. Reconciliation built its candidate list by deriving the NEXT reference and checking that first. For a freshly quarantined order that reference has never been sent, so it was absent — and since the previous commit made an absent newest reference stop the search, the reference that had actually gone out was never looked at. Every quarantined order would have stayed quarantined until somebody released it by hand. The tests hid it by answering every queried reference. Reconciliation now walks exactly the references this order has put on the wire, newest first, ordered by attempt number rather than by how the list happened to be stored. Nothing is synthesised. Two more from the same pass. The age bound that stops an unobservable order from polling forever sat behind the transient-transport branch, so an old order whose socket kept dropping never reached it — a dropped socket that keeps dropping is, from here, indistinguishable from one that will never answer, so the bound now applies to both. And a positive observation could still lose the write race: if somebody released the order as not executed while reconciliation was watching the venue confirm it, the release landed and the observation was discarded. An observation outranks a judgement, so it is now taken back — the order returns to in progress, with an alert, because whoever released it needs to know their check missed something. * Stop three silent waits, and keep the reclaim from overruling the venue Five findings from the eighth pass. The reclaim introduced last time was too broad. It matched any failed row by id, so an order that had ended for an entirely unrelated reason could be resurrected by a late positive observation. A not-sent resolution now stamps its failures, and only a failure carrying that stamp can be taken back — the reclaim exists to overrule a judgement, never the venue. The manual path ignored whether its own positive observation actually landed. If a negative resolution won the write, the observation was discarded; it now reclaims in exactly the same way as automatic reconciliation. Three paths could wait forever. An acknowledged withdrawal whose terminal update is never seen, an order left in a pending status whose update is missed, and - already bounded for trades - the unobservable case, all answered "not complete" indefinitely while the manual path accepts only quarantined orders, so there was no way out at all. All three now fall under the same age bound and become unknown outcomes rather than failures. And reconciliation can conclude a negative again. When every reference an order ever put on the wire comes back rejected, nothing was created — unlike mere absence that is a real negative, so the order is resolved instead of being asked about forever while its rule stays blocked. * Fold the pending-order age bound into the branch that already existed The previous commit added a second case block for the pending statuses instead of extending the one already there, which is a duplicate-case lint error. The bound now lives in the existing branch, and the sixty minutes it shares with the "order cannot be found" path is a named constant rather than a literal repeated in two places. This should have been caught before pushing: the lint run had already reported it and the commit went out anyway. * Tell an observed wait apart from an unknown outcome The ninth pass found the two drifting together again. The previous commit quarantined an order that had been pending too long, and a withdrawal the venue knew about but had not settled. Both are OBSERVATIONS - we know exactly where the order stands - so quarantine was the wrong shelf: reconciliation found the reference, handed the order straight back as sent, and the next completion check quarantined it again. Every transition reported a change, so the pipeline loop kept going round, querying the venue each time, while manual release refused because the reference existed. Quarantine now means only what it says: we cannot tell whether the request took effect. A pending order simply waits, however old it is, and a withdrawal the venue has a record for waits too. Only the genuine blind spot - no record at all, past the age at which the venue is considered to have lost an order - is still quarantined. An order that waits too long is a stuck order, which the monitoring counter already surfaces; it is not an unresolved one. The same pass found no route to an automatic duplicate execution across the initial send, amend, restart, rejection, reference-claim and reclaim paths. * Recover a lost adoption before writing again, and quarantine only a real blind spot Two findings from the tenth pass, plus three conformity points. A replacement the venue had accepted could be lost. The claim persists the new reference before sending, but adopting it afterwards is an ordinary save - and if that save failed, the row still named the predecessor while the venue had already cancelled it. The next check restarted from that predecessor and placed a second order alongside the live replacement. Before anything may write again, a claimed reference the venue is working is now adopted; a rejected one is skipped. The age bound was too broad. It fired for any failure after the check began, so an order the venue could still show us was quarantined because pricing or aggregation had failed - and reconciliation, finding the reference, handed it straight back for the next check to quarantine again. The bound now applies only when the order itself cannot be observed. From the conformity pass: the route follows the repository's camelCase majority (70 to 32); the audit reference is trimmed before validation, as eleven other string DTOs do; and the new tests use typed venue fixtures instead of widened literals. Not changed, with reason: the demand to remove every `any` from tests contradicts the verified house convention - the pattern appears 1441 times across this repository's specs. The cases where typing was straightforward are typed; the convention is not overturned in passing. * Adopt only forwards, and never write past a claim nobody can account for Three sequences found in review could still end in a second request against the same funds. A predecessor is not a replacement. After an amend this row DID record, the superseded original sits in the reference list as cancelled — and adoption, which only skipped the current reference, would take it back and restart the very quantity the replacement was working. Adoption now considers references newer than the current one only. A claimed reference the venue does not show, or cannot be asked about, is not a refusal. It is recorded before the request leaves, so it may be live at the venue right now; the check may not step past it and carry on with the predecessor. Such a claim now blocks every write on the order, and the wait ends the same way any other blind spot does: past the age at which the venue itself is considered to have lost an order, it goes to a human. Applying a positive observation takes two writes — release the quarantine, or take back a negative resolution that got there first. A crash or a failed write in between left the order in a state nothing ever read again, while its rule was free to plan anew. Failures stamped as not-sent are now re-examined alongside the quarantined ones for an hour, so the second write is simply retried. Six of the seven added tests fail without the corresponding change; the seventh pins the existing two-step reclaim. * Let a recorded not-sent failure stay reclaimable, and use PUT for the release An observation that the venue does know the order becomes durable in exactly one write. If that write does not land — the process ends, the statement fails — the observation is gone, and the order sits failed while the venue is working it. The hour-long window that covered this was itself a way to lose it, so there is no window any more: a failure stamped as not-sent stays eligible for reconciliation without an age limit. What that would otherwise cost is a venue lookup per settled failure on every tick, for the rest of its life. So each such order is asked about once per process instead. That is the same coverage — the observation can only be lost by this process ending or its write failing, and neither survives into the next one — without the standing load. The release endpoint updates an order, so it is a PUT, per the REST section of CONTRIBUTING. One test also went back to the typed venue-order helper instead of casting a literal. * Say it out loud when a confirmed observation cannot be written back Two review passes chased the same thing from opposite sides: re-examining terminal failures kept a lost observation recoverable, but it also meant a leading-wildcard scan over every historical failed row on each pipeline run, and the memo that bounded that cost treated an inconclusive lookup as settled — so a delayed confirmation was suppressed for the life of the process. Both go away with the mechanism. Reconciliation is back to quarantined orders only. The reclaim stays: it is one conditional statement and it still catches a negative resolution that lands mid-pass. What was missing was never the scan, it was the admission of failure. When the venue has confirmed an order and neither release matches — or the write throws — that observation is the one fact worth having, and it was being dropped as a log line. It now raises an alert naming the order and the reference, telling whoever reads it to treat the order as live at the venue. Consistent with the rest of this change: where we cannot be sure, a person decides, and no rule plans against those funds in the meantime. A release someone else performed correctly is recognised and stays quiet. * Hold a confirmed order blocking, rather than only reporting it An alert is read at human speed; a rule reactivates in minutes. So reporting that the venue confirms an order — while the row stays failed — still leaves the scheduler free to plan against funds that are already committed. Reporting was the wrong half of the answer. Anything short of a clean release now puts the order back into quarantine, which is the state this subdomain already treats as in flight with an open outcome: no rule plans against it, and the next reconciliation pass simply tries again. The alert stays, but on top of a state that holds by itself. The manual release had the same shape and now takes the same path: its refusal no longer depends on a write it never checked. Its message says which of the two happened. The report also no longer depends on a second read succeeding — that read is only what lets a genuinely safe outcome stay quiet, and a state that cannot be read is not one of them. * Bound the second look by observability, and stop overwriting what a row already says A not-sent failure is eligible for reconciliation again, so an observation whose writes never landed — including the re-quarantine that normally catches that — is still applied afterwards. The bound on it is the venue's own reach: Scrypt's execution history goes back thirty days, and past that nobody can make an observation to apply. That is deliberately not a deadline for applying one, which is how the previous attempt could lose it; it is also what keeps the query off the entire failure history. Second: overruling a resolution was erasing the record of it. Both write-backs built their message from the copy of the row this pass started with, which predates the resolution being overruled — so a reclaim replaced the account that released the order and the reference they checked with a stale string. Both now read the row and append to what it actually says, and where it cannot be read, only the status changes. * Mark a not-sent resolution on the row instead of hunting for it later Finding the failures that still owe reconciliation a second look was done with a wildcard match over every failure ever recorded, on a job that runs every ten seconds, and the attempts to bound that cost kept trading one hole for another: an in-memory memo lost the obligation on restart, a time window turned into a deadline for applying an observation. The obligation now lives on the row. A not-sent resolution stamps notSentResolvedAt; the first reconciliation pass that looks at the order again clears it. That is the coverage that was wanted — the pass writing such a resolution may be racing one that has just watched the venue confirm the same order, and an observation that could not be written would otherwise be gone — and it survives a restart, because it is a column and not a set in memory. Cleared after that one look, so settled failures are not put to the venue every ten seconds for good. Indexed, so finding them is an equality predicate. Two smaller things, both about not destroying evidence: The reclaim and the re-quarantine now guard their write on the exact reason they just read. Another resolution can land in between, and appending to the older copy would erase the newer operator and reference. And the verification reference is trimmed at the edges only. Util.trimAll removes every space — it is for identifiers like IBANs — and it was turning "venue console, ticket OPS-42" into one run of characters. That field is evidence somebody has to read. The migration is additive and nullable, with no backfill. It was not run against a Postgres instance: this machine has none. * Guard the recheck marker against a newer resolution, and keep the moment in the reason Clearing the marker was guarded by order and status alone, so a pass that had been looking at one resolution could clear the marker of a newer one written in the meantime — dropping the further look that newer resolution was owed, which is the whole point of the marker. The clear is now conditional on the exact marker the pass started from. A marked order whose integration has no lookup at all could never have its recheck happen, so the marker stood forever and every pass selected the row only to skip it. Such a row is released instead. And the column is named for what it is: work still owed, not a record of when the resolution happened. That moment now goes into the order's own reason, which nothing clears — so releasing the marker cannot make it unrecoverable. * Remove the superseded migration file left behind by the rename The rename to notSentRecheckDue added the new migration but left the old one in place, so both would have run and the table would have gained two columns instead of one. Only the new file belongs here; neither has been merged, so there is nothing deployed to correct. * Tell an unanswered lookup apart from an unanswerable one A venue that cannot be reached was reported the same way as a venue that answered and had no record. Both arrived as UNRESOLVED, so the recheck an order still owed was retired on the strength of a lookup that never happened — and with it the observation it was being kept for. A failed lookup is now UNAVAILABLE: no question reached the venue, so nothing was looked at, and the order stays marked until something actually is. Separately, an order can outlive the adapter that made it. The integration factory returns null for a system or command that is no longer registered, and both the reconciliation loop and the manual release dereferenced it: the loop would have thrown on every pass forever, and the manual release refused an order that nothing could ever look up again — leaving it quarantined with no way out. Both handle the absence now, and a marked failure nobody can ask about is released rather than selected and skipped for good. * Make a not-sent release take effect only once the venue has answered A release said the request never left, and the order became a plain failure on the spot. That is terminal: the pipeline finishes, the rule reactivates, and it may issue the request again — all while a confirmation that the order IS live could still have been in flight and simply lost its race to the write. Keeping such an order reconcilable afterwards did not help, because between the release and any later correction the rule was already free. So the release no longer ends the order. It is recorded, the order stays quarantined, and reconciliation puts it into effect on the next pass — normally seconds later — once the venue has answered that it has no record either. Two negatives, one of them from a person who looked. A venue that cannot be reached answers nothing, so nothing happens and the order keeps blocking; a venue that confirms the order overrules the release outright. An all-references-refused verdict still fails the order directly. That is the venue's own answer, not a judgement, and it needs no confirming. This removes the reclaim path entirely: nothing writes a failure behind a pending release any more, so there is no negative resolution left to take back. * End an order only against the release that was actually examined Ending an order is the one step here that cannot be taken back, so it now compares and sets on the exact pending release the pass looked at. A release written since has a confirmation of its own outstanding, and completing the older reading would skip it. The one case where a release does not wait for the venue — an order no integration can look up any more, where no answer can ever come — is now stated in the same places the rule is: the column, the DTO, the migration and the pull request text. It was only in the code. The release tests asserted against the in-memory entity, which the reconciliation loop would have left looking correct even if nothing had been stored. They now assert the conditional write itself, its predicate and its payload, and that the pass reports a change. One comment still described the reclaim path that was removed. * Retry an observation the database refused, instead of dropping it When the venue confirms an order and the repair that holds it blocking cannot be written, the failure was swallowed. The order stayed a plain failure — nothing selects one of those again — while the venue worked it, and after the rule reactivated the request could go out a second time. The write is kept and repeated at the start of each pass, ahead of any new lookup. It asks the venue nothing; it only repeats what is already known, and it stops as soon as it lands or another path has put the order somewhere safe. Held in memory deliberately: the alternative is a durable queue nothing ever drains, and the one case this cannot cover — the process ending first — is covered by the alert that goes out at the same moment, which names the order and tells whoever reads it to treat it as live. Also: the branch that completes a release for an order nothing can look up now reports the change like every other branch, so the caller's loop sees it. * Make a confirmed order safe with one narrow write, and stop the pass until it is Two ways a confirmed venue order could still end up terminal. The first: applying an observation is a substantial write, and everything about it can fail. So the very first thing now is the narrowest write in the file — one column on a row that is still quarantined, cancelling any pending release. Once that lands, no judgement can end the order, whatever fails afterwards and even if this process stops. It is also what makes a quarantined row genuinely safe to stop retrying on: quarantined WITH a release pending is one inconclusive lookup away from being ended, and that is exactly what the observation contradicts. The second: holding an unwritten observation in memory kept it alive, but the rest of the pass carried on regardless — starting pipelines, advancing them, issuing orders — on a picture in which an order says failed while the venue is working it. The pass now stops after reconciliation for as long as any observation is unrecorded, and the next one retries the write before asking the venue anything. * Repair a confirmed order with the first write, and do not trust a cache after an unconfirmed cancel The first write for a confirmed order only stripped a pending release of its power. If a release had already ended the order, that write matched nothing and the repair came later — so until it did, the only thing between a live venue order and a second request was this process staying alive. The same statement now also puts an ended order back into quarantine. Two columns, no appended text, nothing it depends on; the reason why still follows separately, and if that never lands the order is at least still blocking. Second: when an amend is refused, the fallback cancel is a write as well. Its failure was noted and dropped. Unconfirmed, that cancel may have taken effect at the venue while the cached report still shows the order open — and a non-terminal cached report is never replaced by a later fetch, so every check afterwards would keep waiting on a picture that cannot change, with the order stuck in progress and out of reach of both reconciliation and the manual path. The cached report is dropped instead, so the next lookup has to ask the venue, and the refusal says the cancel went unconfirmed. * Do not let an unreachable venue veto a verified release forever A release waits for the venue to answer, which is right — but a venue that answers nothing at all would have held a verified order for good. The wait exists to catch a confirmation that is in flight right now; after an hour of silence there is none in flight, only an operator who checked and is being ignored. This is a liveness bound, not a safety one, and nothing is concluded from the silence: the person who released the order concluded it. Silence merely stops being a veto. Also: an import out of alphabetical order. * Put the release rule where it belongs, and state both its exceptions everywhere Whether a pending release has waited out an unreachable venue is something the order knows about itself, so it is answered by the order, with the bound alongside it, and the service is left with the orchestration. And the promise that a release waits for the venue was written in six places, all of which still named only one way out of it. Both are now stated wherever the rule is: an order no integration can look up, and a venue that has answered nothing for long enough. Neither concludes anything from silence — silence stops being a veto on the person who checked, it never becomes evidence. The entity gains its own tests, including the boundary either side of the wait. * State both release exceptions in the DTO and the migration too The same promise is written in several places and two of them still named only one way the wait can end. No behaviour change. * Sort the entity imports by path The new import went in at the top instead of in order, and the two around it were already out of order. No behaviour change. --- ...0000-AddLiquidityOrderNotSentRecheckDue.js | 49 ++ .../scrypt-websocket-connection.spec.ts | 59 +- .../services/__tests__/scrypt.service.spec.ts | 102 ++- .../services/scrypt-websocket-connection.ts | 101 ++- .../exchange/services/scrypt.service.ts | 176 ++++- .../actions/__tests__/scrypt.adapter.spec.ts | 456 +++++++++++- .../actions/base/liquidity-action.adapter.ts | 10 +- .../adapters/actions/scrypt.adapter.ts | 329 ++++++++- .../adapters/balances/custom.adapter.ts | 9 +- .../adapters/balances/exchange.adapter.ts | 9 +- .../controllers/order.controller.ts | 17 +- .../resolve-uncertain-order.dto.spec.ts | 40 ++ .../dto/resolve-uncertain-order.dto.ts | 40 ++ .../liquidity-management-order.entity.spec.ts | 59 ++ .../liquidity-management-order.entity.ts | 113 ++- .../core/liquidity-management/enums/index.ts | 22 + .../order-outcome-unknown.exception.ts | 18 + .../liquidity-management/interfaces/index.ts | 16 +- ...uidity-management-pipeline.service.spec.ts | 667 +++++++++++++++++- .../liquidity-management-pipeline.service.ts | 496 ++++++++++++- .../observers/liquidity.observer.ts | 6 + 21 files changed, 2722 insertions(+), 72 deletions(-) create mode 100644 migration/1784885000000-AddLiquidityOrderNotSentRecheckDue.js create mode 100644 src/subdomains/core/liquidity-management/dto/__tests__/resolve-uncertain-order.dto.spec.ts create mode 100644 src/subdomains/core/liquidity-management/dto/resolve-uncertain-order.dto.ts create mode 100644 src/subdomains/core/liquidity-management/entities/__tests__/liquidity-management-order.entity.spec.ts create mode 100644 src/subdomains/core/liquidity-management/exceptions/order-outcome-unknown.exception.ts 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/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)),