Release: develop -> main - #4564
Merged
Merged
Conversation
* feat(auth): require staff KYC clearance on elevated endpoints
Access to endpoints gated on Admin, Debug, Compliance, Support or RealUnit now
additionally requires an identified natural person behind the calling account:
kycLevel >= 50 and a non-empty verifiedName.
The check lives in RoleGuard, so it covers all 235 elevated gates at once. It is a
property of the endpoint, not of the caller: it applies only when every entry role of a
gate is elevated, so an admin using ordinary customer functionality is unaffected.
StaffKycClearanceService derives the cleared account ids from the DB every minute,
ProcessService primes an in-memory Set from the resulting setting, and RoleGuard reads
it synchronously. This mirrors the existing JWT denylists: the requirement takes effect
on already-issued tokens within one refresh interval, with no DB lookup per request.
The allowlist is deliberately fail-closed - an empty or not-yet-primed Set denies every
elevated endpoint, so a DB or cron outage cannot silently re-open admin access.
Three privilege checks that live in business logic rather than a gate are covered too,
via the new hasStaffAccess helper. All three sit on routes that are not role-gated
(OptionalJwtAuthGuard), so the requirement would otherwise be bypassable: protected KYC
file downloads, ownership-independent access to any customer's transaction history, and
posting official support replies. Privilege checks behind already-gated endpoints (the
isAdmin distinctions in the support note services) are left unchanged - the gate has run
by then.
The three files deciding elevated access are pinned at 100% coverage on all four metrics
via a dedicated CI step, following the existing Frick coverage gate.
* fix(auth): correct import order and a stale service reference
Restores the alphabetical import order in user.module.ts and fixes a comment in
setting.service.ts that referenced a class name that does not exist.
* refactor(auth): filter the verified name in SQL and fix import order
Moves the verifiedName condition out of JS and into the query, per the
"filter in SQL, not JS" rule: TRIM(...) <> '' covers empty and whitespace-only names and
drops NULL on its own, since the comparison yields NULL.
The raw fragment interpolates the alias TypeORM passes in, which is already quoted - an
unquoted camelCase identifier would be folded to lowercase by Postgres and fail at
runtime. Verified against a real Postgres instance: the emitted SQL is
TRIM("..."."verifiedName") <> '' and only genuinely verified staff rows survive, with
NULL, empty and whitespace-only names all filtered out. The unit test pins the rendered
predicate, since mocked repositories never execute the fragment.
Also restores the alphabetical import order in process.service.ts.
* fix(auth): close two gaps in the clearance path
Two holes found in review, both in the path that decides elevated access.
Blank names: Postgres TRIM() with no character argument strips ASCII space only, while
the JS trim() it replaced also strips tabs, newlines and Unicode spaces. A verifiedName
of a single tab or a non-breaking space therefore passed the filter and cleared an
account carrying no identification. The predicate now uses BTRIM with the explicit
character set that trim() strips, spelled out rather than left to [[:space:]], whose
meaning depends on the database locale.
A mocked repository never executes raw SQL, so a new suite runs the predicate against a
real Postgres and pins it to the semantics it replaced: every blank variant is excluded,
padded real names are kept, and the result is asserted to equal the JS filter on every
input. It follows the existing MIGRATION_TEST_PG pattern, so CI runs it against its
throwaway database.
Manual override: the generic PUT /setting/:key route accepted staffKycClearance, which
contradicted this branch's own comment and let a cleared admin write uncleared accounts
into the allowlist, live until the next sync overwrote it. Sync-owned settings are now
rejected there; the sync path writes through setObj and is unaffected.
* refactor(setting): enforce system-managed keys in the service
Moves the system-managed check from the controller into SettingService.set, where it
covers every caller rather than only the HTTP route, and keeps the controller thin as the
contributing guide requires. The sync path writes through setObj, which goes to the
repository directly, so it stays unaffected - now pinned by a test.
Widens the Postgres suite so the character set itself is guarded: the blank fixtures are
derived from the JS runtime rather than from BlankChars, because fixtures generated from
the constant under test would shrink along with it and could never catch a character
dropped from it. Verified by removing one character from BlankChars, which turns the
suite red.
* test(setting): pin the key and value written by the generic setter
Pins key and value on the generic setter instead of only asserting that something was
written - a swapped argument pair would otherwise pass, and this is the only place the
pair is checked since the controller test was folded into the service test.
Records why U+200B is absent from BlankChars: trim() does not strip it either, so adding
it would make the predicate stricter than the check it replaced. Whether a name made of
invisible characters counts as identification is a question about how verifiedName is
written, not about this gate.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
August 1, 2026 08:34
… without a person (#4440) * fix(liquidity-management): bound the uncertain-order quarantine An order the venue keeps having no record of stayed UNCERTAIN indefinitely: the only way out was a manual release through the admin endpoint. Where nobody performs that release, the order never resolves and the rule behind it stays blocked, so the venue silently stops being served at all. Abandon such an order to FAILED once it has been unresolvable for longer than ABANDON_UNRESOLVED_MINUTES. What makes that safe is not a conclusion about the order but that a rule replans from the venue's CURRENT balance, never from the abandoned order: an execution that did happen has already moved that balance, so the replan sizes itself down rather than duplicating the request. Two guards, both fail-closed: - UNAVAILABLE never abandons, however old the order. It is the absence of an answer, not an answer, and waiting cannot turn it into one. - An order without a creation date never abandons. Util.minutesDiff reads a missing date as the epoch and would otherwise abandon instantly. * fix(liquidity-management): scale the abandon bound to the request kind One bound for every command was either useless or unsafe. Measured over the 30 days to 2026-07-29 on completed Scrypt orders in prod: trades (n=55): median 9.6s p95 19.8s max 57.1s withdrawals (n=49): median 7.7min p95 82min max 5.6h A trade is done inside a minute, so holding one for hours strands its rule for no reason — the incident order was a trade and sat for nine. A withdrawal at 30 minutes is entirely ordinary, so a short bound there would abandon live transfers and reissue them. Trades now abandon after 5 minutes (~5x the slowest observed), everything else after 12 hours (~2x the slowest observed withdrawal). Balances refresh every minute and the pipeline runs every 10 seconds, so neither bound is limited by detection speed, only by how long the request may still be alive. Venue-internal commands are an allowlist (buy, sell, purchase): an unrecognised or unloaded command falls to the long bound, because being slow to abandon costs minutes while being fast to abandon a live transfer duplicates it. * fix(liquidity-management): bound orders no integration can look up The bound was applied only after a lookup had actually been made, so it missed the one case where waiting is provably pointless: an order whose adapter is no longer registered. No lookup can ever arrive for it, and without a manual release nothing in this system moves it again — the same permanent quarantine, reached sooner. Apply the same bound there. Also corrects a comment that said "for hours", which stopped being true once trades got a five-minute bound. * fix(liquidity-management): correct the abandon bound after review Three defects, all in the abandon path introduced by this PR. Clock ran from the wrong instant. `unresolvableTooLong` measured from `created`, a @CreateDateColumn that is never updated. An order can run normally for a long time and only become UNCERTAIN late, when a completion check amends or restarts it and that write goes unconfirmed — its `created` is old by then, so the bound expired on the very first pass while the replacement request was seconds old and plausibly still arriving. Measures from `updated` now, which moves with the transition into quarantine. Allowlist keyed on the command name alone. `sell` on an exchange is a book match settled in seconds; `sell` on the DEX adapter is an on-chain swap with a confirmation time, and it was getting the five-minute bound. Keyed on `system/command` pairs now, listing only the exchange trades actually measured. Weakest write had the weakest guard. `abandonUnresolvableOrder` called `leaveQuarantine` without `expectedRecheckDue`, so an operator release written between this pass's read and the write was overwritten together with its audited reason — the one write resting on no evidence outranking the one resting on a person. Narrowed on the examined release, as `completeNotSentRelease` already does. Also withdraws the automatic abandon for orders no integration can look up. That path never asks a venue, so only the clock would be left, and the safety argument requires a balance that reflects an execution: a chain balance omits unconfirmed transactions and a bank balance comes from the last imported batch. Abandoning there would be guessing with the one class of order nothing can observe, so it keeps waiting for the manual release instead. * fix(liquidity-management): match SQL NULL when narrowing on an absent release The narrowed update never matched a row. TypeORM renders a raw null in a where object as `= NULL` — invalidWhereValuesBehavior.null defaults to "ignore", which falls through to an equality — and `x = NULL` is UNKNOWN in SQL, so it matches nothing, not even the row whose column really is NULL. That disabled this PR's entire purpose. The abandon path is only ever reached for an order nobody had released, so the examined value there is always null: every abandon would have updated zero rows and logged itself as a lost race while the order stayed quarantined. `completeNotSentRelease` carried the same defect on its NOT_SENT branch, which reaches it with no release pending. Uses IsNull() for the null case, matching how the rest of the repo expresses this. The mocked repo in the unit tests returns a fixed affected count and cannot see criteria semantics, so the test asserts on the operator itself. Also corrects the UNCERTAIN status comment, which claimed quarantine is always time-bounded. Since the previous commit withdrew the automatic abandon for orders no integration can look up, two cases legitimately wait for a human: those orders, and a venue that stays unreachable. * docs(liquidity-management): align the remaining comments with the new bound Three places still described the pre-change behaviour, one of them contradicting a comment corrected earlier in the same method. - resolveUncertainOrders said anything inconclusive "stays put"; it now stays put only until the bound for its kind of request runs out. - The UNRESOLVED enum value said "look again later", implying indefinitely. - ScryptAdapter.resolveUncertainOrder's docstring said a missing record leaves the order quarantined for a human, while its own inline comment already said the caller bounds that wait. Two weaker statements in the same adapter are softened for the same reason: they describe entry into quarantine, which is unchanged, but read as if quarantine were the end state. Also adds the missing regression test for the NOT_SENT caller of the IsNull narrowing. Asserting on status alone cannot catch a regression there, because resolveAsNotSent sets FAILED synchronously before the write is attempted, and the mocked repository returns a fixed affected count. * docs(liquidity-management): finish aligning comments, cover both allowlist entries Remaining statements that the abandon bound made untrue, plus two test gaps. Comments: the abandon docstring said "for hours", which is wrong for the five-minute trade bound; a stray line break left "The" stranded mid-sentence; the manual-release docstring claimed an unsent request would block its rule forever, which now only holds for the cases nothing can observe; three adapter comments described quarantine as ending only with a human. The resolveUncertainOrder docstring also said it can only ever confirm a positive, which its own NOT_SENT branch contradicts — that one predates this PR but sits in the paragraph already being corrected. Tests: the bound tests only ever used `sell`, so dropping or mistyping `scrypt/buy` from the allowlist would have gone unnoticed; they now run over both entries. And nothing covered a pending release together with an expired bound — the branch order makes the audited release win, but only a test stops a later reordering from silently replacing an operator's recorded verdict with an anonymous clock. * fix(liquidity-management): report an unaskable order as unavailable, not unresolved An order with no correlation id was reported as UNRESOLVED, which means the venue answered and had no record. Nothing was asked: there is no reference to ask about. The distinction now decides an order's fate — the caller may abandon an UNRESOLVED order once its bound expires, and would have recorded "the venue has had no record of it" for a lookup that never ran, against this PR's own rule that a row must not claim an observation nobody made. UNAVAILABLE is exactly this case by its own definition: the venue could not be asked at all, and such an order is never abandoned automatically. Only orders predating the reserve-before-send guarantee can reach it, and those are the ones that most need a person to look. Also fixes a docstring this PR made worse two commits ago: "an absence can only ever confirm a positive" states the opposite of what the code does. An absence confirms nothing — that is the whole point of the paragraph. * test(liquidity-management): expect an unaskable order to be unavailable The existing case asserted UNRESOLVED for an order with no reference, without saying why. It was written when the distinction had no consequence: nothing acted on UNRESOLVED, so either value behaved the same. This PR gives it one — UNRESOLVED now permits an automatic abandon — so the old expectation locks in the behaviour the previous commit corrected. Replaced by the documented case, covering both an undefined and a null reference, and asserting that no lookup is attempted at all. * fix(liquidity-management): do not abandon an order with references left unasked The lookup stops at the newest reference when the venue cannot see it, and deliberately does not fall through to the one it replaced — a hit there would report SENT on a superseded reference. But it reported that stop as UNRESOLVED, an answer, and the bound may abandon an order on an answer. The predecessor of an invisible replacement is very often the live one, so an order with an open attempt at the venue could have been failed after five minutes and its rule sent to plan again. An existing test documents exactly this shape: predecessor still NEW at the venue. It now reports UNAVAILABLE whenever references were left unasked, which the caller never abandons on. A single unknown reference still reports UNRESOLVED — nothing was skipped there, so the answer is complete. Also repairs a side effect of the previous commit: with an unaskable order reporting UNAVAILABLE, a hand-verified release stopped taking effect immediately and instead waited out the full unreachable-venue hour. Nothing went out under a reference, so no lookup can ever answer — the same dead end as an order with no integration, and released the same way. Two smaller review points: the it.each table left its parameter implicitly any, and the allowlist hardcoded the system name that already exists as an enum value. The command halves stay literal because their enums live in the adapters, which import this entity. * fix(liquidity-management): count only references that can still be live as unasked The previous commit counted every remaining reference as unasked, including ones already superseded. That creates the permanent quarantine this PR exists to end: once a replacement has been adopted, the reference it replaced stays in the list forever, so if the venue later loses sight of the adopted one, the dead predecessor makes every lookup incomplete and the order can never be abandoned — worse than before this PR, where the bound would have applied. Only references at least as current as the adopted one count now, matching how adoptLiveReplacement already separates claimed replacements from established ones. Also completes two comments: UNAVAILABLE has three causes, not one, and only two of them were named. A complete answer may time out; an incomplete one never does, whatever caused it. * feat(liquidity-management): bound the unanswered outcomes too Waiting for an operator was still the end state whenever the venue could not be asked, or not completely: no reference to ask with, an unreachable venue, a lookup that stopped with a reference left unasked. Where nobody performs the manual release, that is not caution — it is the same permanent quarantine this PR set out to remove, reached by a different door, and the rule behind the order dies with it just as surely. These outcomes now expire as well, on their own much longer clock: 24 hours, over four times the slowest order ever observed at this venue. By then nothing can still be in flight, so the balance the rule replans from already reflects whatever really happened — the same argument that makes the short bound safe, with far more room. The recorded reason says the venue was never heard from, never that nothing was sent, so the row still claims only what was observed. One case still waits for a person, and deliberately: an order no integration can look up at all. There the venue is never asked and the balance a replan would read is not live either, so neither half of the safety argument holds. * fix(liquidity-management): finish a lookup that would otherwise never complete The 24-hour bound did not hold for one of the three cases it covered. When a claimed replacement never reached the venue, it is absent on every pass — so the lookup stopped at it every time and never reached the reference it replaced. Waiting could not fix that: the absence is structural, not transient. Meanwhile these are GTC limit orders with no expiry, so the unchecked predecessor can sit open in the book indefinitely, and abandoning after 24 hours would have planned those funds a second time. The measurements behind the bounds cannot see this either. They describe orders that completed, so an order that never becomes observable is by construction absent from the sample — the numbers are a floor, not a ceiling. Past the age at which this venue already treats an order as lost, the invisible replacement is now recorded as spent and the lookup carries on to the reference that may actually be live. That turns a question which never completed into one that does, which is the only way such an order ever gets a real answer rather than a guess. Review points from the same round: the two abandon paths were identical except for their reason, so they are one method taking it as an argument; the timeout map and the entity mutator no longer name only the case they started with; a JSDoc had come to sit above the wrong method; and the new predicates now have direct entity tests covering both bounds and the missing-timestamp guard. * fix(liquidity-management): apply the same age rule to the write path The previous commit repaired the reconciliation lookup but not the completion check, and the two disagreed. Reconciliation would walk past a claimed replacement the venue never showed and resolve the order to its predecessor; the very next completion check blocked on that same invisible claim, refused every write, and quarantined the order again. The order oscillated between the two states indefinitely — and because each return to quarantine refreshes `updated`, both abandon bounds were reset every cycle, so the safety net meant to end exactly this was disabled by the loop itself. adoptLiveReplacement now steps past a claim under the same condition the lookup uses: the venue ANSWERED and still does not show it, past the age at which this integration already treats an order as lost. A claim the venue could not be asked about keeps blocking however old it is — silence is not an answer, and no clock turns it into one. Also drops a call to recordSpentCorrelationId that could never do anything: its argument always came from the order's own reference list, which is the one case the entity guards against. The commit message claiming otherwise was wrong; the correctness came from continuing the loop, not from that line. The "keeping it quarantined" warning now only logs on the path that actually keeps it quarantined, and the comment claiming an explicit rejection is the only way to reach an older reference names the timeout path too. * feat(liquidity-management): cancel before giving up, instead of timing it out Everything about abandoning a quarantined order was an estimate of when its request could no longer execute. That estimate is what three rounds of review kept breaking: the measurements behind it only describe orders that finished, Scrypt trades are GTC and expire never, and the clock the two code paths shared was a whole-row timestamp that any unrelated write reset — so the order oscillated instead of ending. None of that is necessary. What makes abandoning dangerous is not the passage of time but the possibility of a live request, and that possibility can be removed rather than waited out: cancel every reference the order ever sent, and only give up once the venue confirms none of them can execute. A cancel is the opposite of a re-send and cannot create anything, which makes it the one write that is always safe against an outcome nobody observed. The order is abandoned when the venue settles all of its references, and keeps waiting when it will not — unreachable, or reporting one in some other state. That is the same conservative answer as before, but reached for a stated reason instead of an assumed one, and it no longer depends on any clock. What this removes: the 24-hour unanswered bound, the reference-left-unasked classification, the age exception in adoptLiveReplacement, and the tests built on them. The two measured bounds stay — they still decide when it is worth trying to clean up at all, not whether cleaning up is safe. Scrypt only. Adapters that cannot cancel omit the new hook and their orders keep waiting for a person, unchanged. * fix(liquidity-management): read what a cancellation actually established The cancellation had two outcomes where the venue has three, and each gap was a way to lose money or to stall forever. A partially filled order cancels with a terminal Canceled status AND a non-zero fill — the protocol says so explicitly. Reading only the status called that "nothing can execute" and abandoned the order, dropping a real fill and handing the rule back funds that were already spent. A fill is now recognised whatever the final status says, and the order is pointed at the reference that executed so the ordinary completion path books it instead. A refused cancellation arrives as an execution report, not as an error — this venue sends no separate reject message. So the branch meant to catch "there is no such order" was unreachable, and the incident case this PR exists for would never have been settled at all. It is now read off ExecType and CxlRejReason, and only UnknownOrder counts: every other reason settles nothing. An order with no reference ran the loop zero times and fell through to "all settled" — abandoned on a confirmation nobody gave. It now refuses without asking anything, since there is nothing to ask about. Also adds the tests this contract had none of, and corrects the comments left describing the two-tier bound that no longer exists: age only decides when cleaning up is worth attempting, and the venue decides whether giving up is safe. * test(liquidity-management): fix fixtures for the new cancellation contract The cancel now returns the venue's execution report rather than a boolean, so a mock that resolved undefined made the amend path read a field off nothing and take the unconfirmed branch. And cancelOutstanding derives the trade pair from the rule, which the shared uncertain-order fixture does not carry. * fix(liquidity-management): file a cancellation under the order it settles The fill was still unbookable, one layer further down. This venue tags a cancel confirmation with the CANCEL request's id and names the cancelled order only in OrigClOrdID — while every lookup here is keyed on ClOrdID alone. So the terminal state was filed under an id nothing ever asks about, and the cancelled order kept answering with whatever non-terminal report preceded it. Pointing the row at a reference that had executed therefore led nowhere: reconciliation saw the stale report, handed the order back, and the completion check found the same stale report again. The confirmation is now filed under the order it settles. Even then the fill was unreachable: the reconciliation lookup stops at the first reference the venue does not show, newest first, and a claimed replacement that never arrived is exactly such a reference. It would stop there on every pass and never reach the older one the cancellation had just identified as executed. The stop now applies only to references NEWER than the one the row names — the current reference is authoritative, and a cancellation that found a fill points at it deliberately. An unreadable filled size no longer counts as zero; it settles nothing, since concluding "nothing filled" from a value that could not be parsed is how a real fill gets dropped. The rejection branch in the catch is gone — this venue answers a refused cancel with a report, not an exception, so it was unreachable and contradicted the branch that does the work. Adds seven direct tests for the classification itself, which until now was only ever mocked, and completes the fixture that the type checker could not see through. * fix(exchange): file only a real cancellation under the cancelled reference The previous commit filed every response tagged with the cancelled order's id, refusals included. A refusal comes back on the same channel carrying the order's last known state, and for a reference the venue never had that reads as a live New order — so caching it invented one. The next lookup found that phantom, called the order sent against a reference that had never executed, and pointed the row away from the one that actually filled. Same lost fill as before, one layer further down. Only a terminal Canceled says anything about the order being cancelled, and that covers both outcomes the caller distinguishes: nothing filled, and partially filled. Refusals stay uncached, so a reference the venue never had keeps reporting nothing — which is what lets the newer-reference guard in the reconciliation lookup do its job. Adds the two tests that would have caught it: a refusal must leave the cache untouched, and a cancellation that finds a fill must still name the executed reference after the next reconciliation pass. * feat(liquidity-management): treat an executed reference as settled too The whole tangle came from one wrong assumption: that a reference which filled is a reason to hold on to the order. It is not. The question this path asks is whether a reference can still execute, and one that already ran answers it just as finally as one that was called off. Holding on was also pointless, because the fill is in the venue's balance and that balance is what the rule replans from — it plans for what is actually left, not for what this row believed. So there is nothing to rescue and nothing to guess: cancel everything, give up, replan. That removes the part which produced a critical finding in five consecutive review rounds — rerouting the row onto the executed reference, and with it the need to remember which references the venue never had. Gone with it: the reroute, the newer-than-current guard in the lookup, and the chained test that only ever existed to cover them. Two findings from the same round, both real: - The cancel waiter accepted the first report mentioning the order, including a non-terminal PendingCancel. Since it unsubscribes on its first match, the real terminal report that follows would never have been seen. PendingCancel is now skipped. - resolveUncertainOrders described itself as only ever observing, which stopped being true when this PR gave it a cancel. * fix(exchange): require a terminal state before calling a reference executed The fill was read before the status, so a REFUSED cancel could be classified as executed. A refusal carries the order's last known state — a partially filled order that could not be cancelled reports a fill while staying wide open — and since the caller now treats executed like settled, that would have let it walk away from a reference still able to trade. The same double execution this path exists to prevent, re-entered through the door the previous simplification opened. A fill on its own says nothing; only a terminal state says nothing more is coming. Executed now requires Canceled or Filled, and a non-terminal report with a fill settles nothing. Also records which reference filled on the order before abandoning it. The money side is carried independently by the venue's own transaction sync, but an abandoned order books no output and cannot say for itself what happened — naming the reference is what lets the two be tied together afterwards. Documents the one assumption that stays an assumption: the replan reads a pushed balance, so a just-landed fill may briefly not be in it. Seconds in practice, but a window rather than a guarantee, and the comment said otherwise. Corrects the enum, the docstring and the log line that still described the executed case as one that must be completed rather than abandoned. * fix(exchange): decide what counts as terminal in one place The cancellation check restated which states are final and left one out. A rejected order is as unable to trade as a cancelled one, but it fell through both branches and settled nothing — so an order that provably could not execute would have stayed quarantined for want of being recognised. Exactly the class of permanent hang this PR removes, reintroduced beside it. This venue already answers that question in one place, and that answer includes rejected. Reusing it means a second list cannot drift from the first. Adds the two cases that were missing: a rejected reference with nothing filled settles, one with a fill counts as executed — same as cancelled and filled. * docs(liquidity-management): align three statements with what the code now does All three were true when written and were made false by changes in this PR. The entity header still claimed abandoning is safe because the replan reads the CURRENT balance — stated as settled fact, while the adapter's own cancellation now honestly calls that a window rather than a guarantee. It names both things that actually carry it now: the venue's confirmation, and a replan against the balance, with the freshness caveat pointed at where it is described. The no-reference comment said such orders resolve on a bound rather than a verdict. They do not: with no reference there is nothing to cancel either, so the automatic route cannot confirm anything about them and they wait for a person. And abandoning is no longer reserved for orders the venue never accounted for — it is reached just as well when a reference executed and nothing is left outstanding. * docs(exchange): state the UnknownOrder reading as the inference it is The comment asserted that UnknownOrder means there is no such order, so nothing under that reference can execute. The protocol spec lists the reason without defining it, so that cannot be read off the value: "never existed" and "not processed yet" are indistinguishable from it alone. The reading itself stays — it is what lets the incident case resolve at all, and every alternative leaves the order waiting forever. But it now says what it rests on instead of claiming certainty: the caller only cancels a reference it has already failed to find through a separate status lookup, and only after the order has outlived the window in which its request could still be live. Two independent negative answers plus that age are the strongest evidence this protocol offers, which is a different claim from proof. * fix(exchange): treat a missing filled size as missing, not as zero Number('') is 0, not NaN, so an empty or whitespace CumQty passed the finite check and read as an untouched order — settling it. The comment two lines above promises the opposite: that an unreadable quantity settles nothing. Emptiness is now caught separately, with tests for both forms. Also corrects five statements that this PR had made untrue: The entity's abandon mutator still said "an order the venue never accounted for" — the fourth instance of a sentence corrected three times elsewhere, and wrong for the same reason: abandoning now also happens when a reference executed. The abandon docstring claimed to rest on nothing but the clock, four lines above stating correctly that it is reached only after the venue confirmed nothing can execute. The clock decides when cleaning up is worth attempting; the confirmation is what permits it. Same wording corrected in the sibling docstring and in the test comment that echoed it. The UNRESOLVED enum value said the bound alone gives an order up. It does not — without the cancellation's confirmation the order stays quarantined. And "every reference the row ever put on the wire" overstates the source: a reference is reserved before it is sent, so the list can contain one that never left. * fix(exchange): a refusal that contradicts itself settles nothing The UnknownOrder branch returned settled unconditionally, while the terminal branch right above it checks the filled quantity before deciding. That inconsistency mattered: a refused cancel carries the order's last known state, and the neighbouring test proves such a refusal really can arrive with a filled quantity attached. A report claiming the venue has no record of an order while reporting a fill on it disagrees with itself. Nothing may be concluded from that — least of all that nothing can execute, which is the one conclusion that lets the caller walk away. It now settles nothing and the order keeps waiting. * docs: say what the cancellation evidence actually covers The UnknownOrder comment claimed two independent negative answers per reference. It gets one: the status lookup stops at the first reference the venue does not show, while the cancellation asks about every reference of the order — so for all but one of them the refusal is the only negative answer there is. Age plus one refusal is still the strongest evidence this protocol offers, but that is a weaker claim than the one that stood there. Four statements corrected in the same class, all of them versions of sentences already fixed elsewhere in this PR and missed at these spots: - The reference-list helper still said "actually put on the wire", one line from the caller where exactly that wording was corrected — a reference is reserved before it is sent. - The interface contract for cancelOutstanding had the same wording, making it narrower than what its only implementation documents. - A comment still had the bound abandoning an order by itself; it triggers a cancellation attempt, and only that attempt's confirmation abandons. - An inline comment still called the abandon write "resting on no evidence at all", four lines below a docstring that had already been corrected to say it rests on the venue's confirmation. * fix(exchange): check the contradiction before the status decides The self-contradiction guard added last commit sat behind the terminal-state check, so it only caught the non-terminal shape. A refusal claiming the venue has no record while reporting a fill is contradictory whatever status rides along — with a terminal one attached it went straight through as executed, which is exactly the permission to give the order up that the guard exists to withhold. It now runs first. The cancellation is also no longer cached when its filled quantity is unreadable. Readers derive that value with `parseFloat(...) || 0`, so such an entry would quietly claim nothing was filled on every later lookup — the same reading the classification itself was changed to avoid. And three statements the previous commit claimed to have corrected but did not: its edit script aborted partway and I only chased the one failure it reported instead of checking whether the rest had run. The interface contract still said "put on the wire", and two comments still had the bound giving an order up by itself rather than triggering a cancellation whose confirmation does. Corrected now, with a grep over the whole diff to confirm no further instances remain. * test(exchange): cover the matcher and a non-numeric filled size The PendingCancel skip added earlier had no test at all: every cancellation test mocks the transport directly and hands back a finished report, which routes around the matcher entirely. This one captures the matcher and drives it, asserting it ignores the interim state and takes the terminal report that follows — the waiter resolves on its first match and stops listening, so accepting PendingCancel would freeze it as the answer. Also adds a genuinely non-numeric filled size. Empty and whitespace were covered, 'abc' was not. * fix(liquidity-management): ask every reference before deciding cancelOutstanding promised to cancel everything the order could still have live, then left the loop at the first reference the venue would not settle. Those it skipped are precisely the ones that can sit open in the book while the newest keeps refusing — so the method did the opposite of what it says whenever it mattered most. It now asks about every reference and decides afterwards; one refusal still keeps the whole order quarantined. The existing test hid this: its unconfirmed reference happened to be last in iteration order, so both got called anyway. The new one puts it first. Also sharpens the interface contract on what "confirmed" covers. An accepted cancellation or a terminal order settles the question outright; a refusal saying the venue has no such order is an inference from its own words, not a statement about execution. The contract said CONFIRMED for all three, which overstates the weakest of them. * docs: correct the bound's role everywhere, and say where it does not reach The previous commit claimed a grep over the whole diff had confirmed no further instances of "the bound gives the order up". It had not — the grep matched the exact wording I had just changed, not the pattern. Five more places said it, in three different phrasings. Searched by pattern this time, with the result verified as empty afterwards. More useful than the wording: the transfer bound does not reach Scrypt withdrawals at all. It was derived from measured withdrawal runtimes, but reaching it only triggers a cancellation attempt, and the one venue that implements cancellation refuses it for withdrawals outright — there is no such thing as cancelling one there. So those still wait for a person, and the value governs the other transfer kinds and any venue that gains a cancellable withdrawal later. Said so at the constant, and renamed the test that asserted this to stop implying a venue can do what Scrypt cannot. Also: the abandon write does rest on evidence — the venue's cancellation — just not on evidence about whether the request was ever sent. And the cache test now covers all four unreadable shapes instead of only the empty string. * fix(liquidity-management): stop calling an inference a confirmation The reason written into the order and the log said the venue "confirmed" that nothing can execute. It does not always confirm that: a refusal saying it has no such order settles the question by inference, which the code says plainly where that inference is made. The interface contract was corrected for exactly this distinction two commits ago and the downstream message was not, so an operator reading the reason later would lose the difference between an accepted cancellation and something concluded from the venue's wording. Both now say "answered", which covers all three outcomes honestly. Four smaller consistency fixes in the same class: two docstrings, an enum comment and a test title said "every reference the order ever sent", excluding the reserved-but-never-sent case that cancelOutstanding explicitly covers; a trade test title still credited the bound alone with abandoning, while its transfer twin had been corrected for that; and a test comment still said the caller may abandon once the bound expires, without the settled cancellation that actually permits it. * fix(exchange): never file a cleanup cancellation under the order it cancelled A confirmed cancellation was cached under the cancelled order's own id. That made a later status lookup read the order as terminally cancelled — and a cleanup cancellation says nothing about the order as a whole: its sibling references may still be unsettled and live. The lookup would then report the order as known, take it out of quarantine, and the completion check would open a replacement beside a reference that can still trade. The double execution this whole path exists to prevent, reachable in a plain multi-reference order. The aliasing only ever existed to make the EXECUTED reroute work, and that reroute is gone — cancelling now settles a reference rather than adopting it. So this is dead code with a live hazard, and removing it restores what the method did before this PR: report the outcome, cache nothing. The test that demanded the aliasing now demands its absence. * test(exchange): restore the amend-propagation guard lost to a regex My delete regex in the previous commit took more than the one test it was aimed at. I restored the block brace and the helper it had swallowed, but not this test — it guards that an unconfirmed amend is propagated as such and that nothing is cancelled along the way, behaviour that is unchanged and still thrown in two places. Its loss also orphaned an import, which eslint reports as a warning; `npm run lint` exits zero on warnings, so my own gate check called that green while CONTRIBUTING requires none. Restored verbatim from the parent commit. Verified by diffing every test title against that commit: only the one intentional reversal differs, and eslint with --max-warnings 0 is clean. * style(exchange): keep these two files on the CRLF endings this repository uses Restores what my Python edit scripts had silently normalised to LF. It is not cosmetic: with every line counted as changed the real diff was unreadable and the branch could not take a thirteen-line change to the same region. * fix(liquidity-management): keep the venue-lookup cooldown inside the abandon bound #4438 landed on develop while this branch was open and throttles the very loop this branch extends: a quarantined order's venue lookup now waits a tenth of the order's age, capped at thirty minutes, and skips the pass entirely while that wait runs. The skip happens before the branch that gives an expired order up, so the throttle silently governed the deadline too — a trade quarantined when it was already hours old would draw the full thirty-minute interval and be abandoned six times its own five-minute bound too late, with nothing anywhere saying the bound had moved. The interval is now also capped at what is left of the order's own bound, held at the cooldown floor once that has passed. A wait reaching past the deadline it has to keep cannot do its job; below the floor, a cancellation the venue will not confirm would retry on every ten-second tick, which is what #4438 exists to prevent. Both bounds keep their intent and the tighter one wins. The remaining time comes from the entity, from the same private accessor as the deadline itself, so the two can never disagree about which bound applies. Only the direction carrying the safety is asserted: an order past its bound reports nothing left. The converse cannot hold at the single instant where elapsed equals the bound, and a non-negative duration cannot express that — it costs one cooldown floor, never a missed abandonment. Verified by mutation: removing the new cap leaves the abandonment sitting for the full thirty minutes and turns the test red. #4438's own cap tests are untouched — their fixture sets no `updated`, so no deadline constrains them — and a transfer, whose bound is twelve hours, still waits the full cap. * fix(exchange): refuse a negative filled size instead of reading it as untouched A cumulative filled size below zero is finite and parses cleanly, so it passed the unreadable-value guard and then lost to `filled > 0` — reported as an order nothing had ever traded, which settles the cancellation and lets the caller walk away from the reference. Same class as the `Number('') === 0` case the guard above it already names: a value that cannot mean what it says must not be compared as though it could. Fails closed as UNCONFIRMED. Verified by mutation: dropping the check turns the two new cases green again, which is what makes them worth having. * test(liquidity-management): type the new cooldown fixtures through the shared helper `tsc` rejected what jest had happily transpiled: an inline action literal is checked against LiquidityManagementAction inside Partial<LiquidityManagementOrder> and lacks paramMap, created and updated. The file already has agedOrder(minutes, command) for exactly this, with the cast the rest of the suite uses, so both new tests now go through it instead of building their own fixture. My local gate had missed it because I ran jest and eslint but not tsc. Re-verified by mutation after the rewrite: removing the cap still turns the bound test red. * fix(liquidity-management): freeze the deadline cap at the last lookup The cap was read fresh on every tick, so it shrank while the wait it bounds grew — and the two met halfway. A five-minute bound was therefore re-asked at 2.5 minutes, then 3.75, then 4.4: a geometric series of full history fetches before a deadline that had not moved, which is the cost #4438 exists to avoid. The abandonment also landed late, around 5.75 minutes, because each of those lookups restamped the cooldown. Now measured as the time that was left when the last lookup finished. Adding the elapsed wait back to what is left now yields exactly that figure — bound − (attempt − updated) — so the cap holds still between lookups and needs nothing stored beyond the stamp already kept. One lookup, and the abandonment lands on the bound rather than past it. Verified by mutation, both directions: re-reading the cap on each tick turns the new test red, and removing the cap altogether turns two red. * fix(liquidity-management): stop reporting a stuck quarantine write as a race A missed compare-and-set was logged as "already resolved elsewhere, skipping", which is one of two possible causes and the harmless one. The other is a narrowing that matched no row and never will: then the same line appears on every pass while the order does not move at all, and the wording makes a permanent block read like routine concurrency. Found the hard way in production. A release timestamp written by hand in SQL carried microsecond precision; a JavaScript Date carries milliseconds, so the value the code loaded could not match the row it came from. The order sat behind this message, once per tick, until the precision was corrected — and the log said it had been resolved elsewhere the whole time. The line now names both causes, says that repetition means the second one, and is a warning: a race is rare and self-correcting, while the other case is a silent permanent block and this is the only trace it leaves. Which is the exact failure mode this branch exists to end, so it should not be introduced by the branch's own bookkeeping. No extra query — re-reading the row to tell the cases apart consumed a call the surrounding tests sequence their mocks on, which is too much machinery for a log line. Verified by mutation: restoring the old wording turns the new test red. * fix(liquidity-management): give the order up at its bound, not a cooldown later Two review lanes found the same defect independently, and between them the whole of it: the quarantine bound was reliably overshot by up to a minute. Half of it was an off-by-one between the two halves of one clock. The deadline having arrived is what lets a reconciliation pass run at all; being *past* the bound is what lets that pass give the order up. With a strict `>` the instant the bound was exactly met satisfied the first and not the second, so the pass ran, abandoned nothing, and re-stamped its cooldown — a five-minute bound gave up at six. Now `>=`, and the two turn over together. Asserted with the clock held still, because anything built from a live `Date.now()` is already a fraction past the boundary, where `>` and `>=` agree and the bug is invisible. The other half was the cap's own floor. A lookup running shortly before the deadline has less than a floor's worth of time left, and imposing a full interval on it scheduled the next pass after the deadline — the overshoot the cap exists to prevent, caused by the cap. The floor now applies only once the deadline has passed, where there is nothing left to protect and an unconfirmable cancellation must still not retry every tick. Making that distinction possible is why `msUntilAbandonable()` became `abandonableAt()`. A remaining duration has to be clamped at zero, and that clamp erases precisely what a throttle needs after the deadline: whether the wait it is about to impose began before it. An absolute instant carries the sign, so the lookup's headroom is a subtraction rather than a reconstruction — and the comment explaining the reconstruction is gone with it. Verified by mutation, three ways: restoring the floor on an open deadline turns the new boundary test red, `>` turns three red, and dropping the cap turns three red. My first attempt at the floor half was not caught by its own test — the scenario never produced a lookup close to the deadline — so the test now reaches it the way production does, via an order that enters the pass already near its bound with no cooldown recorded. * refactor(liquidity-management): follow the method naming rule, fix a stale doc link CONTRIBUTING requires methods to be verbs, with the documented exception being boolean getters on `is`/`has`. `abandonableAt()` is neither, and it returns a Date — so `getAbandonableAt()`. The entity's other additions here are already verbs or read as statements; this was the one value getter, and the only method of its shape in any entity, so there was no house pattern arguing the other way. The JSDoc above `unresolvableTooLong()` still linked `{@link msUntilAbandonable}`, a method the previous commit removed. A dead link in the one comment explaining why the two halves of that clock must agree is worse than no link. * refactor(liquidity-management): apply the naming rule to the helper too `abandonBoundMinutes()` is a noun phrase, exactly what the previous commit renamed `abandonableAt()` for. Applying the rule to the public getter and not to the private helper beside it would have left the file arguing with itself. The entity spec's describe block also still carried the old name. The local variable holding the returned instant keeps its noun name: the rule governs methods, and `abandonableAt` is what that value is. * docs: stop promising a bound the code deliberately does not give Three statements claimed more than the code does. The bound was described as "how long before it is abandoned", the quarantine as ending "not indefinitely", and every outcome at an askable venue as "bounded". None of that holds: reaching the bound starts a cancellation, and the order is given up only if the venue settles every reference it claimed. Where an integration cannot cancel, or the venue will not answer, the order stays quarantined past the bound — which the same comments then said correctly a few lines further down, so they contradicted themselves. The claim these bounds actually support is narrower and worth stating exactly: they end the assumption that somebody will eventually look, not the quarantine. The catch in `cancelOrder` said anything reaching it is a transport failure. It starts with a trade-pair lookup, so a missing pair or configuration lands there too, having sent nothing. The comment now says what the handler really knows — that it cannot tell which failures got as far as writing, which is precisely why it drops the cached report for all of them. Comments only; no statement, condition or value changed. * docs: finish the pass, and check the whole class rather than the reports Three more statements of the same kind the previous commit corrected, each contradicted by a comment within a few lines of it: - The UNCERTAIN enum said an order with no venue record "is abandoned to FAILED" once past the window, four lines above its own correct note that only the venue's answer permits FAILED. - `completeNotSentRelease` said the other exit means an unreleased order "cannot block its rule forever", against the corrected text 150 lines up in the same file saying it can, and that this release is then the only way out. - SETTLED was documented as "the certainty the caller needs" in both the enum and the service, while the constant it rests on says the unknown-order reading is an inference and not a documented guarantee. Both now separate the two qualities of answer instead of flattening them. Fixing only what was reported is what left these behind twice, so this time the whole diff was swept for the pattern — every new comment claiming a guarantee. The three above were real. The remaining matches are sound: one is asserted by the test it appears in, one is hedged with "in practice" and exists precisely to explain why the code does not rely on it, and the rest describe the failure this branch fixes rather than promising anything about it. Comments only; no statement, condition or value changed. * docs: correct three claims, including one where the review was right and I was not The review's first two rounds on this pattern each left something behind, and one of the leftovers was a call I had made myself: - The UNCERTAIN enum said an order past the window "has its references cancelled". Still too much: a Scrypt withdrawal returns false from cancelOutstanding without asking anything, and an adapter that cannot cancel omits the method entirely. It now says an attempt is made where the adapter supports one. - The entity said an askable venue leaves no outcome waiting for an operator. The attempt is a cancellation; a venue that will not confirm one holds the order indefinitely, and a request that cannot be cancelled is never attempted. Stated as what it is: those orders get an attempt, not a bound. - `abandonUncertainOrder` claimed its release marker is "always null in practice, because a pending release routes to an earlier branch". I had reviewed that claim in the previous commit's sweep and passed it. It is wrong: this branch has no release condition of its own, so an order released while the venue was unreachable reaches it with the marker set. The compare-and-set was already correct — it narrows on the value actually read — but the comment explained it with a premise that does not hold. Checked what the third one implied and it is fine: the abandonment prefixes the existing message rather than replacing it, so an operator's audited reason and ticket survive. Said so in the comment, since the question now arises there. Comments only; no statement, condition or value changed. * docs(liquidity-management): correct what this test says the abandon would do The comment said the abandon would overwrite the operator's audited reason with an anonymous one. It would not — both exits prefix the existing message rather than replacing it, which I established while correcting the production comment in the previous commit and then failed to carry over to here, leaving the two saying opposite things about the same code. What actually differs is the verdict each exit adds: the release states the venue confirmed the request never arrived, the abandon only that nothing is left to execute, and just one of them rests on somebody having checked. That is what the branch order protects and what the assertion is worth having for. * fix(liquidity-management): give quarantined Scrypt withdrawals an automatic exit A quarantined withdrawal had no way out but an operator: cancelOutstanding returned false for WITHDRAW without asking anything, because Scrypt has no cancel operation for withdrawals. Trades resolved themselves, withdrawals waited for a person who may never come — and withdrawals are roughly half of what this venue does. The exit cannot rest on a cancellation, so it rests on confirmed absence: the venue returns its full transaction history and this reference is not in it. That is weaker than "the request never arrived" and is worded as such — the order is abandoned, never released as not-sent, because nobody observed a send. What carries the safety is the consistency gate, not the age bound. A fresh history counts only when it is non-empty and every already-known transaction inside the order's window reappears in it. Without that, a truncated reply that merely omits the reference would read as "does not exist", the rule would replan, and withdraw() would run again — it only checks minAmount > balance and then takes min(maxAmount, balance), so with enough balance left a second payout goes through. A failed gate warns with the missing references named, so a permanently broken fetch is visible instead of becoming a silent wait. cancelOutstanding now returns the reason to record instead of a boolean, so an abandoned withdrawal does not carry the trade wording claiming every reference was answered for. Reaching quarantine without a reference is an invariant break since reserve-before-send, not a planned wait: logged as an error rather than silently held. Verified against production — no Scrypt order is in Uncertain, and every one without a reference is Failed or NotProcessable, so there is no legacy population to migrate. Trades whose references the venue will not settle keep waiting, deliberately: that wait is on the venue, not on a person. While Scrypt is silent nothing can be planned against it anyway, and the next pass abandons the order within seconds of it answering. Releasing earlier would be guessing, and an onFail chain could then buy the same funds on a second venue while a zombie order still sits in the book. * fix(liquidity-management): close the remaining paths where Scrypt waits for a person Review found three more states a quarantined Scrypt order could only leave by hand, plus a gate that did not hold what it promised. The consistency gate was only as good as the local cache it compared against. With that cache empty — process restart, failed warm-up, catch-up not through yet — it passed without checking anything, so a truncated history that merely omitted the reference would have read as confirmed absence. It now requires an anchor: at least one known reference inside the window, plus the newest known transaction overall, so an empty time window cannot hollow it out either. Missing anchor means no exit and a warning, which only defers — warm-up refills the cache within seconds. The test that accepted the unsafe state now demands the safe one. An order whose command is no longer registered was skipped on every pass: the factory hands out an integration only for a supported command, and Scrypt/sell-if-deficit still exists in production as an action the code dropped. Reconciliation now resolves by system alone, because what matters for resolving a quarantined order is who can ask the venue, not whether the command is still executable. Executing still requires a registered command. Cancellation for such an order sends nothing — it asks after every reference and gives up only where the venue reports all of them terminal. An order without a reference was a dead end, needlessly: the reference is derived from the order id alone, so it can be reconstructed and the venue asked as usual. The invariant-break error stays, it is just no longer the only response. A missing `updated` switched the deadline off entirely, which turned a defensive guard into a permanent wait. It falls back to `created` — always older, so the bound expires earlier, which is safe because abandoning still needs the venue's confirmation; the clock only decides when a cleanup attempt is worth making. Without either timestamp the order still stays put, or the bound would run from the epoch and give up at once. A test asserted the old behaviour by name ("never abandons an order whose quarantine timestamp is missing") and was guarding the very defect; its claim is inverted rather than dropped. Two comments promised automation the code does not deliver for systems other than Scrypt. * fix(exchange): make the absence proof survive a venue that answers incompletely The consistency gate compared the fresh history against the local cache — but both come from the same client, and fetchAll treats a missing `next` as end-of-history without checking cursor or sequence. A truncated pagination therefore looks complete, and the same missing slice is absent from cache and reply alike, so the gate passed on a common-mode failure. The newest-known anchor did not help: it guards recency, while the failure drops an older row. An old anchor is added alongside it. The oldest cached reference stamped before the order's own window must reappear in the fresh reply, which forces the answer to span that window and exposes a suffix cut in either direction. What this cannot see is stated where the check lives rather than glossed over: a hole in the middle of history, missing from cache and reply alike, has nothing local to be compared against. The cache snapshot was taken before the fetch and never re-read, so a withdrawal arriving as a live push while the bulk fetch was open was invisible to both snapshot and reply — and would have been reported absent. The live map is now re-read after the await. The anchor requirement introduced a way to wait forever: the cache is filled by the boot warm-up and by reconnects only, and a failed warm-up had no retry. On a stable socket the cache would then stay empty, every reconciliation would defer, and the withdrawal would sit there — the forbidden combination, reintroduced by its own fix. A warm-up that fails now triggers the existing catch-up instead of waiting for a reconnect, and only a real rejection counts: an empty history is a successful warm-up. Cancelling for a command that is no longer registered was passive, waiting for a terminal status that may never come. The premise was wrong: getTradePair matches a pair in either direction and returns the same symbol, and a cancel reads only that symbol, never the side. With a tradeAsset in the params such an order takes the ordinary cancel path and gains the UnknownOrder inference with it. Only without a determinable symbol does the passive branch remain. * fix(exchange): drop the absence gate, it bought a non-risk with a forbidden wait The gate guarded against a second withdrawal. That is not a risk worth guarding: Scrypt withdrawal destinations are exclusively DFX-owned addresses, so a repeated payout moves funds between our own accounts — an internal rebooking, not a loss and not a compliance incident. What it cost instead was the one thing that is not allowed. Every anchor it required can be structurally absent: no cached row carrying a ClReqID, no row older than the order, nothing inside the order's window, or simply an order older than the 365-day cache bound. In each of those cases the check returned false on every pass, forever, and the withdrawal waited for someone to refill a cache by hand. Review found two such paths; both came from the gate itself. So the gate goes, and with it roughly 120 lines. What remains reacts to an absent answer rather than to a missing local anchor — a fetch that throws, or an empty history — plus the one positive observation that costs nothing: if the reference turns up in the live cache while the bulk fetch is open, absence is not confirmed. A truncated venue reply can now cause a second withdrawal, which is written where the check lives, because it is a decision and not an oversight. Two defects surfaced underneath and are fixed on their own merits. Balance transactions without a readable timestamp were dropped from the cache unconditionally — `new Date(undefined) >= x` is always false — while the sibling function five lines above guards exactly that case; the rows discarded were withdrawals we later look for. And the passive path for an unregistered command treated "the venue does not know this reference" like "the venue could not be asked", so it never settled; the active path infers precisely that. Only a lookup that could not be performed keeps the order waiting now. * fix(exchange): close the last two states a Scrypt order could not leave alone An empty transaction history was treated as an answer we could not use. It is the opposite: on a fresh or long-idle account — the stream carries only deposits and withdrawals, never trades — the venue answers successfully with zero rows, and a reference cannot be in a list that has none. The branch turned that into a permanent wait, and the warm-up retry cannot break it, because an empty result counts as a *successful* warm-up. Two of my own comments contradicted each other on exactly this point; the one at the check claimed a total integration failure that recovery would repair, three lines from the constructor saying an empty array triggers nothing. A real outage throws and is caught above, so the two similar-looking cases are handled differently on purpose. That is now written where the check is. The second state was a trade of a command the code no longer has. Cancelling needs a symbol, the order carried none that could be trusted, and Scrypt orders run until cancelled — so nothing would ever end it. The premise was wrong: ScryptOrderInfo already names the symbol a reference lives under, and a cancel reads nothing but that symbol. The venue therefore tells us how to cancel its own order, and every reference it can still show is cancellable. So the symbol derivation goes. It was not only unnecessary but unsound: a tradeAsset left in a stale paramMap is no evidence of what the command once did, "SELL versus everything else" is not a side inference for a command that by definition is not SELL, and the pair lookup reads live security configuration that a delisted pair no longer matches — while the order sits open at the venue under its original symbol. One path now covers every unsupported command: ask, then cancel under the symbol the venue hands back, evaluated exactly as the known-command path evaluates a cancel. `null` still means the venue has no record; only `undefined` — could not be asked — keeps the order waiting, and waiting on the venue is allowed. cancelIfOutstanding keeps its behaviour byte for byte; it and the new symbol-taking variant share one core rather than a second copy of the guards. * fix(liquidity-management): do not advance an order past the check that would finish it Making reconciliation resolve by system alone let an unregistered command be observed again — that part was right and stays. What it also did was let such an order back into IN_PROGRESS on a venue-confirmed SENT, and that was a trap of my own making. The normal completion path uses the strict lookup, which returns nothing for an unregistered command, and the result was dereferenced unchecked: every pass threw a TypeError that the surrounding catch merely logged. The order sat in IN_PROGRESS, where the automatic abandon path does not reach and the manual endpoint refuses it for no longer being quarantined. Worse than the state it replaced, because quarantine at least has an exit. Taking the loose lookup into the completion path instead would not have helped: checkCompletion answers `false` for a command it does not know, so the order would hang just as long without even an error to show for it. The exit has to run through quarantine, which is where the cancel path now works. So the rule is split by what a caller can actually do. Observing is allowed for anyone who can ask the venue — reconciliation stays command-independent. Advancing out of quarantine requires whoever can also finish the job, so a venue-side SENT for a vanished command stays quarantined and leaves by the ordinary cancel/abandon route. Second layer, for any system rather than this one: a running order whose adapter is gone now raises an unknown-outcome error instead of a TypeError, so the existing handler quarantines it. That turns a silent permanent loop into the state from which both the automatic and the manual exit apply. The docstring claimed a confirmed order goes back to IN_PROGRESS and the normal check takes over, three lines above a comment describing the removed-command case as covered. Both were true separately and wrong togeth…
TaprootFreak
previously approved these changes
Aug 1, 2026
* Name the caller in the log line of a rejected request A rejected request is logged with its reason but with nothing about who sent it. On an endpoint that runs without authentication there is then no way to tell a partner integration, one of our own apps and a third-party script apart, which leaves a recurring rejection without an owner who could fix it. The WARN line now carries the X-Client value, the requesting site (Origin, or the origin of Referer - never its path or query, which can hold personal data or tokens) and the user agent. All three are client-supplied and unauthenticated, so they are a diagnostic hint, never an identity, and nothing is gated on them. Each part is masked to the same standard as the rest of the logs, capped per header, and stripped of anything that could break the line, so a crafted header cannot forge a log entry of its own. * Log the values a rejected request sent A validation message names the field and the values it accepts, not the value that arrived. A client sending one wrong constant is therefore visible as a steady rejection count and stays unidentifiable: the same line is produced whether the field was missing, misspelled or set to a value from a neighbouring enum. The global ValidationPipe now raises a ValidationFailedException that carries the raw ValidationError[] alongside the response, and the exception filter renders the rejected values into its WARN line. The response is unchanged - the exception is built by the base factory and only re-raised, which a test pins field by field against the stock pipe. The rendering treats every value as untrusted input: redacted by field name, masked by value pattern, bounded in count, depth and length, and reduced to a summary for structured or oversized values, so a request body cannot be turned into a log dump through a validation failure. * Render a rejected value only where the field declares what it accepts The first pass rendered any scalar that failed validation and relied on a field-name denylist to hold back the sensitive ones. Review found three fields it does not hold back: `apiKey` and `masterKey` are not matched by it, `accountNumber` is not either because the list anchors `number` exactly, and a rejected `webhookUrl` or `redirectUri` would have carried its query string - the place a webhook credential lives - into the log verbatim. Widening the list would have been the wrong repair. It has to grow with every DTO that ever carries a credential, and the field it has not heard of yet is the one that leaks. The value is now rendered only where the constraint that failed declares a closed set of literals (`isEnum`, `isIn`). That is the case the log line cannot be read without, because one client sending one wrong constant looks exactly like another sending a different one, and it is bounded by what the field declares rather than by what happened to arrive. Every other field keeps its shape and loses its content: `<string(43)>`, `<number>`, `<array(2)>`. Missing, null and empty stay visible for every field - there is nothing to disclose, and it is what separates a client that never sent the field from one that sent it wrong. The name-based redaction stays as a second layer over the fields it does cover. Also moves the oversize rule into `maskLogValue`, so a caller cannot pay for masking a string it is about to cut away; types the exception body as `Record<string, unknown>` rather than `any`; and points the neighbouring payment-info spec at the pipe `main.ts` now installs, whose comment this change had made inaccurate. * Pin the two files this change completes in the coverage ratchet The gate's advisory step named them on this branch and not on the base: the new DTO spec is what carries `get-buy-quote.dto.ts` and `xor.validator.ts` to 100% on all four metrics. Unpinned they would stay unguarded, so a later change could erode the coverage this pull request created without turning the gate red. * Say what the literal-domain rule bounds, and what it does not The comment read as if declaring a closed set of values also bounded the value that arrives there. It does not: the declaration decides which fields may show their content at all, and what is then shown is still untrusted input, held only by the masking and the length cap. * Say what holds a rendered value, and cover the case that showed it did not The comment claimed that what gets rendered "stays masked and capped". That is true of a string, and only of a string: a number or a boolean is written out as it is - which is harmless, because being one is already the bound, but the sentence did not say so. The gap was possible because no test covered a non-string under a closed set of values, although the repository has such a field: `noExecutionVerified` is `@IsBoolean() @isin([true])`. Adds one. * Note the field name in the rule the previous commit already applied to it That commit put the field name through the same rendering as the value - a precaution, since no DTO today validates through a client-keyed object - but said only what it did to the value. Names it, and closes the paragraph it had left mid-line. * Reduce the Origin header to its origin as well Only the referring URL was cut down to its origin. `Origin` was passed through as it arrived - on the reasoning that it carries nothing else - and that is the reasoning this whole line is built to distrust: the header comes from the client, and a value that is not what it is supposed to be is exactly the one that must not reach the log with a query string on it. A crafted `Origin: https://example.com/x?token=…` went into the WARN line as sent, which is what the comment above it says cannot happen. Both headers now go through the same reduction, and an unparsable value is dropped either way. Two comments were left behind by the previous commit and are corrected with it: the summary over `describeRejectedValues` and the one in the exception filter both still claimed that everything rendered is masked and capped, which holds for a string and not for a number or a boolean. And the field name goes through `maskLogValue`, not through the whole of `renderValue` - "the same rendering as the value" overstated it. * Pick the first header that yields an origin, not the first one that is there Reducing `Origin` to its origin cost two things that were not meant to go: `Origin: null` - what a browser sends for an opaque origin, a sandboxed frame or a redirect across sites - is not a URL, so it fell into the catch and vanished, although having no origin to name is itself worth the line. And an `Origin` the client filled with something unparsable now suppressed the `Referer` too, because the choice was made on which header was present rather than on which one produced something: a caller that could have been named ended up as no caller at all. The headers are now tried in order and the first one that yields an origin wins, with the opaque literal kept as it is. Both cases are pinned by a test. Also drops a completeness claim from the neighbouring DTO spec: it demonstrates two rejections, it does not enumerate the ones that DTO can produce. * Pin that an opaque origin beats a referer, like any other origin does The rule is that the first header to yield an origin wins, and `null` is one - so a request that carries both an opaque `Origin` and a usable `Referer` is logged as opaque. That is intended and now written down in the comment, but nothing held it: the test covered the opaque header alone. * Make the origin cap test reach the cap The oversized origin it sent was not a URL, so `callerOrigin` dropped it before the cap could apply: the assertion that nothing floods the line held for a reason the test did not intend, and the origin cap itself was never exercised. It now sends a parsable URL that is too long and checks where it gets cut. * Cut a capped value between characters, not between code units `slice` counts UTF-16 units, so a value whose character straddles the cap lost half of a surrogate pair: the stray half went into the log right before the ellipsis and reaches a UTF-8 transport as a replacement character. Every caller inherits it - both caller headers and every rendered field value - since none of them filter what a client can send. The cut now walks characters. Pinned by a test that puts an emoji on the boundary. * Name the unit of a reported length, and the factory's return type `maskLogValue` reports an oversized value as `<N chars>`, but N is `String.length` - UTF-16 code units, not characters. 257 astral characters are reported as 514. That is the very distinction the previous commit drew for the cut, so the label is renamed rather than the count converted: the number is the one `MAX_STRING` is compared against, and counting characters would mean walking the oversized string this branch exists to avoid. The same inaccuracy exists in `redact` and `format` (`<... N chars ...>`, `...(N chars)`), which are untouched here: those are shipped log formats and outside what this branch changes. `DetailedValidationPipe.createExceptionFactory` gets the explicit return type CONTRIBUTING.md asks for, matching the base signature. * Keep the whole warning on one line, and mask an IBAN by value Two gaps the caller markers and the rejected values were already closed against, but the line they join was not: The reason is masked, but not collapsed. Ninety-nine of the exceptions that produce it interpolate a value into their message, and some of those values come from the request (`Invalid address for ${field}: ${address}`), so a line break in one of them ends the log line and starts a second one that looks like a log entry of its own. It now goes through the same collapse and the same character-safe cut as everything else on the line - `slice` would have halved a surrogate pair at the cap the same way it did before the previous commit. Neither the name-based nor the value-based redaction covered an IBAN that is not under a field named for one. The rejected value of a field with a closed set of literals is rendered as it arrived, so a client putting an account number into such a field put it in the log; in the request trace the same value under any key the name list does not know reached it too. `maskValue` masks it by shape now, in one run or in the groups of four it is printed in, bounded so a transaction hash is still left intact. `singleLine` and `capCharacters` come out of `maskLogValue` for this, which leaves one definition of "cannot break a log line" in the file. `format` gets it as well: `JSON.stringify` escapes the control characters but leaves U+2028 and U+2029, which is the one way the request trace could still be split. * Drop the IBAN pattern, and bound the work the cap and the scan do The IBAN pattern is withdrawn. Matching an account number by shape needs the country to know where it ends: without it the grouped form either stops short of a 32-character IBAN and leaves the last group readable, or runs on and swallows the words behind it - both were measured, and the way out of both is a country-length table inside a log formatter. A value-shape list is also open by construction: an account number is one shape, a document number and a card are others, and the one that is not in the list yet is the one that leaks. What is left is the rule the branch already had - the value is only rendered where the field declares a closed set of literals, and it is masked by field name there. `capCharacters` walks to the cap instead of materializing the value first. It is reached with an exception message now, and one of those can be as large as the body it interpolated a value from; spreading that to render 500 characters cost a multiple of the message in heap. The reason is cut to a scan length before it is masked for the same reason, wide enough past the cap that a pattern starting inside it is still seen whole. `format` cuts on a character boundary too - it was the one section left that could halve a surrogate pair - and reports the section length in the unit it counts, as the per-string cap already does. Two comments claimed more than the code does: not every string on a trace line goes through the collapse, only the free-form values do. * Close the three ways the log line still let something through The client header was the one value on a trace line that was interpolated as it arrived. A header can carry U+0085, which `LINE_BREAKING` covers and no other step did, so it is rendered like every other value from the caller now, capped to a name rather than a payload. The reason's scan cut could expose what it split. Masking only shortens, so the part of a message that survives to the log can come from well past the visible cap - and a pattern that straddled the cut arrives halved, is no longer recognized, and its head is then among the characters that survive. A pattern length is read past what is kept and dropped again, which leaves only patterns that ended before the cut, and those were seen whole. The length comes from the patterns themselves, next to where they are declared. Reading the message can throw: the response body is whatever the thrower put there, and an array element that cannot be turned into a string takes `join` with it - before the response is sent. The line loses its reason instead. `format` cuts to its own budget again. That budget is in code units and `capCharacters` counts characters, so an astral section came out at twice it; the cut moves off a surrogate pair rather than counting past it. * Mask before collapsing, and stop trying to bound the scan Masking ran after the collapse, so a control character sitting inside a pattern took the pattern with it: the collapse turned it into a space, the pattern no longer matched, and an address was logged in full. It was measured on both the reason and the client header. Masking runs first now, on the value as it arrived. The scan margin is withdrawn. It cut the message before masking so the work was bounded, and every version of it cut somewhere a pattern could be open - a pattern crossing the cut arrives halved, is no longer recognized, and masking what precedes it shortens the text enough to pull its head into view. Two attempts at a margin produced two ways for that to happen. The message is masked whole again, as it was before this branch; what is left is the cap, which is what this branch actually needed to bound. The request target was the last free-form value that reached a line as it arrived. `maskUrl` collapses it too, which is one line where the query is already being dropped, and makes the sentence about this file true. The response now goes out before the line is written. Everything the line renders comes from the request or from the thrower, and reading either can throw - which left the caller with no response rather than with a line missing a detail. An unreadable exception body gets the generic one instead of none. Two comments claimed more than they had to: a message *can* interpolate a request value, and what the field-name masking covers is stated without a claim about every DTO in the repository. * Let the field say its rejected value may be logged Rendering the value wherever the constraint declares a closed set of literals was the wrong rule, and it is the one thing this branch does that `develop` does not: `develop` logs no rejected values at all. A constraint bounds what is accepted, not what a client sends, and a rejected value is by definition outside it - so `paymentMethod` rendered an account number, a card number, a passport number and an API key just as readily as it rendered a wrong constant. Every attempt to catch those by shape has failed in a new way, because that list is open. `@LogRejectedValue()` closes it from the other side. The field declares that its wrong values are constants of the program, and only a field that says so has its value rendered; everything else keeps its shape and loses its content, including every field that exists today. The two payment-method fields and the personal IBAN provider carry the marker, which is what the endpoint this started from needs. An error without the object it came from renders nothing, so a value cannot arrive through a `ValidationError` that never passed a DTO. `maskLogValue` masks a value whole when removing the control characters is what reveals a pattern in it. A control character splits the pattern so it no longer matches, and the collapse then puts the halves next to each other in the line; a single value is not a sentence, so masking more of it than the pattern costs nothing. Two comments still said more than they had to: the response is already sent by the time the reason is read, and what is masked by value are the three patterns declared above, not personal data in general. * Render a declared constant, never the string that arrived The marker said a field's value may be shown; it did not say which values, so a client could still put an account number, a card number or an API key into a field that carries it and have it logged in full. The marker now carries the set those values may come from, and only a value in that set is rendered - matched without regard to case, and written out of the set rather than out of the request, so what reaches the line is a constant of this program either way. For a field taking the fiat payment methods that set is the full payment-method union: its crypto member is exactly what a client sends there by mistake, which is the case these lines exist for. The personal IBAN provider loses the marker again - there is no wider set its wrong values come from, so it would never render anything. With that, nothing client-composed reaches the line and the masking is no longer what protects it, which is what every attempt so far had rested on. `singleLine` removes what could break a line instead of replacing it, and runs before the masking everywhere: a character placed inside a pattern used to leave the pattern split around whatever replaced it, so the masking no longer saw it - in the reason, in the request target and in the trace bodies, not only where the per-value guard reached. Removing it puts the pattern back together first. That guard is gone with it. * Give the provider its values back, and keep the reading honest The personal IBAN provider was dropped from the declarations on the grounds that no wider set exists for it. That was wrong: the matching ignores case, so the field's own values are exactly what makes `frick` readable as `Frick` - the mistake this line is for. It declares them again. A numeric enum object carries its reverse mapping, so `Object.values` on one returns the member names alongside the values and a request sending a name would have matched. Only the values are taken now. A declared constant is written by this code rather than by the request, but it goes through the same rendering as everything else on the line - there is no reason for the one value on it that is not checked to be the one a hand wrote. The status is resolved before anything else is read and falls back to a server error when the exception cannot answer or answers with something Express will not send; the request is read after the response has gone out, since the response never needed it. Two comments described the replaced behaviour rather than the removal that took its place. * Let a declared value past the field name, and mask on both sides of the removal The provider declaration was inert. `personalIbanProvider` carries `iban` in its name, so the name-based redaction answered first and the value never reached the declaration - the fix landed and changed nothing. A declared match now comes first: what it renders is a constant of this program, and the field's name says nothing about a value that was never the client's. Removing what breaks a line breaks a pattern in the other direction. It joins what stood on either side, so an address followed by a control character and one more digit comes out as one run, where it no longer ends on a word boundary and the masking no longer sees it - the mirror image of the case the removal was introduced for. Both passes run now, around the removal rather than on one side of it, in the one place that renders free-form text for a line. A second pass cannot invent a match: what the first leaves behind carries no alphanumerics. The response body is the generic one whenever the status being sent is not the one the exception names, so a replaced status no longer ships a body that contradicts it, and a message that cannot be read falls back to the status rather than out of the method. The line is written inside its own guard. The response is already out by then, so a failure to describe it - the logger included, which is the one thing that could not report it - ends there instead of travelling back to the caller. 1xx leaves the accepted range: it is not a final response, so it is not one this can send.
A staff member who holds the role but is not KYC-cleared got the generic "Forbidden resource", which is indistinguishable from a removed role. Neither the person nor any tooling in front of the API could tell that the fix is to complete an identification. RoleGuard now throws StaffKycRequiredException: HTTP 403 with a machine-readable code STAFF_KYC_REQUIRED and a message naming the actual requirement. This follows the existing TFA_REQUIRED pattern, so clients branch on the code instead of matching prose. A wrong ROLE still yields the generic 403 - the two cases need different actions and must stay distinguishable. The protected KYC file route checked both conditions through one predicate and reported "Requires admin or compliance role" even when the role was fine; it now reports each case separately. JwtUserActiveGuard calls the guard programmatically, but only with UserRole.USER, which is never elevated - so it still receives a boolean.
TaprootFreak
approved these changes
Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist