Release: develop -> main - #4479
Merged
Merged
Conversation
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
July 29, 2026 21:27
* feat(bank): make the deposit-target bank an operational input The customer-facing deposit selector filtered Bank Frick out by name, so switching a currency to a different receiving bank required a deploy. Replace that hardcoded exclusion with a Bank.receivePriority column, mirroring the existing Bank.sendPriority tie-breaker: lower value wins, ties broken by ascending id. Deploying this changes nothing on its own - every row is backfilled to the neutral default, so the incumbent banks keep their currencies until the priority is deliberately lowered. The migration is schema-only and never touches bank rows, per the established convention for that table. * test(bank): build the selector fixtures as real Bank instances A plain spread of the shared mocks produces an object literal without the entity methods, so it does not satisfy the repository's Bank[] contract and fails the type check. Route the id-carrying copies through createCustomBank, which assigns onto a real Bank. * fix(bank): make deposit-target eligibility explicit instead of implied by rank Review found that ranking alone does not preserve today's behaviour. The removed filter excluded a bank categorically; a low rank only deprioritises it. Whenever the incumbent for a currency is missing or receive=false, or the sctInst lookup falls through to the general fallback, the previously excluded bank would be shown to a customer without anyone switching it on. Make receivePriority nullable with NULL meaning 'never offered as a deposit target', and filter on it before ranking. Eligibility is deliberately not derived from receive: a bank can need to accept and reconcile incoming money without being advertised. The migration now backfills exactly the banks the old name filter already allowed, so the deploy freezes today's routing instead of merely making a change unlikely. * refactor(bank): declare the selector's undefined result and correct a stale comment getBank and getMatchingBank returned Bank while both find() calls can yield undefined. That was already true, but explicit eligibility makes it a documented outcome: a bank left at NULL priority is skipped even as the last candidate, so no match is a state the callers must handle. Both of them already did. Also correct the payout comment that claimed to mirror a bank-name exclusion in the deposit selector - that exclusion no longer exists there. * fix(bank): give the new migration a fresh identity and pin sctInst eligibility TypeORM identifies a migration by its class name, not its file contents. This migration was edited in place after its first version already existed on the branch, so any database that had run the old NOT NULL DEFAULT 1000 version would never run the corrected one and would keep every bank eligible. Production has not run it (no such row in migrations), but a local database might have, so the migration gets a new timestamp and class name. If the old version did run somewhere, ADD COLUMN now fails loudly instead of leaving a silently wrong state. Also add the missing INSTANT case: a NULL-priority bank carrying sctInst must not win the instant branch, which pins that eligibility is filtered before the sctInst lookup rather than after it. * refactor(buy): state the non-null precondition of the bank response builder Widening getBank to Bank | undefined widened this helper's parameter with it, while the body dereferences the bank unconditionally. The caller already rejects the undefined case, so the signature should carry that precondition rather than claim to accept a value it cannot handle. * fix(seed): seed receivePriority so a fresh database has eligible deposit banks The selector skips any bank whose receivePriority is NULL. The migration backfills rows that already exist, but a fresh database is migrated while the bank table is still empty, so the backfill touches nothing and the seed then inserts every bank without the column - leaving local, CI and onboarding environments with no eligible deposit target at all. Seed the column explicitly: 1000 for the banks the selector already offered, empty (NULL) for the dormant Bank Frick rows, mirroring the split the migration applies to an existing database. * docs(bank): scope the receivePriority guarantee to the generic selector The column comment claimed a bank at NULL is never offered to a customer as a deposit target. That overstates it: the explicit personal-IBAN path resolves its own bank and is not gated by this column - it never was, and the bank-name filter this replaced did not cover it either. A comment that promises a guarantee the code does not hold is worse than none, so state the scope and note the cache delay that applies to receive just the same. * docs(bank): scope the remaining eligibility statements to the generic selector The entity comment was corrected, but the migration docstring and the seed comment still claimed a NULL leaves no eligible deposit target at all. Explicit personal-IBAN paths are not gated by this column, so both now name the selector they actually describe. * docs(bank): spell out that receive and receivePriority are ANDed The comment said a number makes a bank eligible, next to a line saying eligibility is not derived from receive. Read together that suggests the priority alone decides. It does not: the selector starts from receive=true rows, so a receive=false bank stays excluded whatever its priority. * refactor(bank): route EUR deposits to Bank Frick in code Replaces the data-driven receivePriority approach with the hardcoded rule the product decision calls for: an EUR bank transfer is routed to Bank Frick. The column, its migration, the seed entries and the debug allowlist entry are removed again. Other receiving banks stay a fallback: if the Frick EUR row is not receiving, EUR deposits keep working through the incumbents rather than failing. SEPA Instant is exempt because Bank Frick does not offer it, so an instant request still reaches a bank that can execute it. Switching this back now requires a release rather than a data change. * test(bank): drop the import left unused by the removed priority cases * test(bank): pin the currency check in the Frick EUR rule Removing `bank.currency === 'EUR'` from the rule left the whole suite green, so nothing guarded it. The rule matches on name and currency and find() takes the first hit, so without that check an EUR deposit could be handed the franc account's IBAN. The CHF row is listed first to make the case bite. * fix(bank): keep Bank Frick out of every path the EUR rule does not cover The removed bank-name filter excluded Bank Frick from the whole selector. Replacing it with an EUR-only rule dropped that exclusion everywhere else, so Frick became a candidate again in paths the rule never claimed: - a CHF request could return the Frick franc row, which is receive=true in production, depending on a database order that is not guaranteed - an EUR instant request with no sctInst bank available fell through to the generic fallback and could return Frick there - the exact case the instant exemption exists to prevent - the condition tested paymentMethod !== INSTANT, which is also true for CARD Scope the rule positively to BANK, and restore the categorical exclusion for everything after it. Resolve several qualifying Frick rows by highest id, the way getBankInternal already resolves an ambiguous (name, currency) pair. One test asserted the fallback behaviour that was the defect; it now asserts the incumbent wins, in both input orders. * fix(bank): resolve the Frick EUR row through the attribution mechanism The tie-breaker sorted by highest id while claiming to mirror getBankInternal. It did not: selectAttributionBank prefers the asset-linked row, because that binding is what isBankMatching and the booked bank_tx history are keyed on. With two active Frick EUR rows the customer would have been shown the newer row's IBAN while attribution stayed on the older one, so incoming payments would not match and book with pendingInputAmount 0 - the netting skew the code warns about at that very function. getReceiveBanks does not load the asset relation either, so the preference could never have applied there. Resolve through getBankInternal instead, which is cached and does load it, and keep the receive check since it does not filter on that. * test(bank): pin that no other Frick row substitutes the attributed one When the asset-linked Frick EUR row is not receiving, a second unbound Frick row must not stand in for it: attribution stays on the disabled row, so a payment into the unbound IBAN would book against a row nothing is keyed on. Also scope the comment above the rule. It aligns the selection rule with attribution, not the caches - ibanCache is loaded once at module init while this read goes through the repository cache, so a row edited at runtime can still be seen differently by the two until restart. That gap predates this rule and applies to every bank, and the comment should not read as a promise that they can never disagree. * test(bank): reject array filters in the findCached mock instead of casting The helper cast the where clause to the object variant, discarding the array form the signature allows. A future array filter would then have matched nothing and silently returned every bank instead of failing. Its sibling three lines up already rejects that case explicitly; do the same here.
…-log fields in SQL (#4480) * perf(ledger): index the content-change scan and project financial-log fields in SQL Two independent sources of database load on dfxprd, both measured in production. 1. The nine ledger consumers each run a (updated, id) keyset content-change scan every minute, but none of their source tables had an index on `updated`. EXPLAIN (ANALYZE, BUFFERS) for trading_order showed a Parallel Seq Scan reading ~494 MB from disk and filtering 5,418,069 rows to return 4, at 151.765 ms per call. Add composite (updated, id) indexes on all nine source tables. 2. GET /v1/dashboard/financial/log selected every matching row including the ~42 KB `message` JSON: 1,316 MB of message payload across the 31,925 matching rows, requested twice per minute, with a 6.6 s median and 29.8 s maximum query time. The endpoint needs only four `balancesTotal` scalars, one BTC price and the `balancesByFinancialType` sub-tree — `assets` (1,346 MB) and `tradings` (43 MB) were never read. Project just those sub-trees in SQL. The response is unchanged for the same underlying data. Malformed `message` JSON now aborts the query instead of silently dropping the row: for a financial dashboard a visible failure is preferable to an unnoticed gap in the series. Verified that all 31,925 matching rows currently hold valid JSON. * style: apply prettier formatting to log repository spec * fix: use deterministic TypeORM index names in ledger scan migration Replace the nine self-invented index names in AddLedgerContentChangeScanIndexes1785460000000 with the deterministic names TypeORM's DefaultNamingStrategy would generate itself, per the "No custom naming for TypeORM indexes" rule. Also documents the derivation in the migration's docstring so the names can be recomputed. * fix(log): address financial-log projection review findings - Restore the falsy btcAssetId check (0/undefined/null all take the no-BTC-asset path) that extractBtcPrice used to have, instead of only excluding undefined. - Guard all five projected numbers (totalBalanceChf, plusBalanceChf, minusBalanceChf, btcPriceChf, fxPnlChf) with a jsonb_typeof check so one bad value nulls only that field instead of aborting the whole query, and align the fxPnlChf guard with the same pattern used by the neighboring priceChf projection. - Remove the now-unused extractBtcPrice helper. - Add coverage for the ORDER BY direction, the exact $N parameter position per SQL condition, and the projection's edge cases (btcAssetId 0/undefined, missing/null balancesTotal fields), and drop assertions that only read back their own test fixture. * docs(migration): correct the lock-behaviour analysis for the index migration The previous note claimed a CREATE INDEX SHARE lock is held only for the duration of that one index build, and justified the write-blocking risk for trading_order on that basis. That was wrong. All nine CREATE INDEX statements run inside a single transaction: TypeORM's migrationsTransactionMode defaults to "all" (DataSource.js:263-265) and config.ts never overrides it, and MigrationExecutor.js:206 opens one transaction for all pending migrations. PostgreSQL releases locks at COMMIT, not per statement. The write-blocking window for trading_order is therefore the sum of all nine builds, and once the transaction commits all nine tables are simultaneously write-blocked -- including bank_tx, buy_crypto, crypto_input and payout_order. Splitting this across several migration files would not change that. Also drop the redundant SET LOCAL lock_timeout calls. SET LOCAL is scoped to the transaction, so 18 of the 19 had no effect and implied a per-statement guard that does not exist; one call in up() and one in down() remain. * fix(log): stop defaulting SQL NULL to 0 in the financial-log projection totalBalanceChf/plusBalanceChf/minusBalanceChf now stay null when the SQL projection nulls them, matching the fxPnlChf handling already in place; the `?? 0` default remains solely at the mapSummaryToEntry call site. Also drop the Number() coercion in the balancesByType loop so a genuinely missing key passes through unconverted instead of becoming NaN/null, and fail loud with the log id and type key when a balancesByFinancialType entry is not an object instead of throwing an unhandled TypeError. Narrows the "byte-for-byte identical" docstring claim to its actual scope and documents down()'s ACCESS EXCLUSIVE lock behaviour and the per-statement scope of lock_timeout in the index migration. * fix(log): make balancesByType optional and revert the F11 throw FinancialLogSummary.balancesByType and FinancialLogEntryDto.balancesByType promised plain `number` fields, but a real production row has an entry missing one of the two keys, which the repository already passes through as `undefined` (proven by an existing test). Mark both fields optional and narrow the `as` cast accordingly instead of forcing a promise the code doesn't keep. Also drop the now-unreachable JSON-string branch when parsing balancesByFinancialType: pg-types always returns jsonb columns already parsed (OID 3802), so the string path had zero coverage and no path to it. Most importantly, revert the typeof/null throw added for a prior finding: it was too broad. Before that guard, a primitive balancesByFinancialType entry (number/string/boolean) never threw at all - JS auto-boxes primitives for property access, so e.g. `(1).plusBalanceChf` is simply `undefined`. Only `null` used to throw, and only inside the old mapper's per-row try/catch, silently dropping just that one row. The throw turned three previously-harmless cases into a 500 for the whole request, and widened null's blast radius from "one row" to "everything". Optional chaining covers all of it in one line: primitives resolve to undefined exactly as before, and null now keeps the row (with an empty balancesByType entry) instead of either silently vanishing or failing the whole request. * docs(log): describe what the financial-log projection actually does Two comments claimed more than the code delivers. mapSummaryToEntry still promised a byte-identical response. That stopped being true on purpose: a `balancesByFinancialType` entry holding `null` used to make the previous mapLogToEntry drop the entire log line via its per-row try/catch, whereas the row is now kept with an empty entry. A silent gap in a financial curve is worse than an empty partial entry. The same comment also claimed SQL "fails loud" without qualification, while individual scalar values are in fact tolerated by the jsonb_typeof guards. The balancesByType loop now records why it deliberately has no per-property typeof guard: the case is unproven in production (287,989 entries with both values numeric, one with plusBalanceChf missing, none with a string, boolean or null value), a guard would break response equivalence with the old mapLogToEntry which passed such values through unchanged, and unlike the five scalar fields there is no ::float8 cast here that could abort the query. * fix(log): keep only real numbers in the balancesByType projection The signature promises `plusBalanceChf?: number`, but a balancesByFinancialType entry whose property itself holds a string, boolean or null was passed through unchanged, so the endpoint could return values the DTO does not allow. The previous mapLogToEntry had the same hole; this closes it instead of carrying it forward. Only real numbers are kept now; every other value becomes undefined. On the current production data this changes nothing (287,989 entries with both values numeric, one with plusBalanceChf missing, none holding a string, boolean or null), so it exists to protect the contract for future data. A missing key still yields undefined, and a non-object entry still does not throw. This applies the same hardening the five scalar fields already get via jsonb_typeof in SQL, in TypeScript because balancesByFinancialType is passed through as a raw JSON object.
TaprootFreak
approved these changes
Jul 30, 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