Skip to content

e710bc80 - Route EUR deposits to Bank Frick - #4470

Merged
TaprootFreak merged 17 commits into
developfrom
feature/frick-eur-deposit-selector
Jul 30, 2026
Merged

e710bc80 - Route EUR deposits to Bank Frick#4470
TaprootFreak merged 17 commits into
developfrom
feature/frick-eur-deposit-selector

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Change

EUR bank transfers are routed to Bank Frick, hardcoded in the deposit selector.

BankService.getBank() previously excluded Bank Frick by name, so Olkypay was the only EUR deposit
target it could return. That exclusion is replaced by the opposite rule:

if (currency === 'EUR' && paymentMethod === FiatPaymentMethod.BANK) {
  const frickEur = receiveBanks
    .filter((bank) => bank.name === IbanBankName.FRICK && bank.currency === 'EUR')
    .sort((a, b) => b.id - a.id)[0];
  if (frickEur) return frickEur;
}

// every path below keeps the categorical exclusion the name filter provided before
const banks = receiveBanks.filter((bank) => bank.name !== IbanBankName.FRICK);

The rule is narrow on purpose

Only EUR bank transfers. The condition is positive (paymentMethod === BANK), so instant and card
requests never enter it. SEPA Instant matters here: Bank Frick does not offer it (sctInst=false), so
an instant request has to reach a bank that can actually execute it.

Every other path keeps the old exclusion. The removed name filter kept Bank Frick out of the whole
selector, not just one branch. Replacing it with an EUR-only rule would have made Frick a candidate
again wherever the rule does not reach — a CHF request could have hit the Frick franc row, and an EUR
instant request with no sctInst bank available would have fallen through to the generic fallback and
landed there too. So the exclusion is restored for everything after the rule; only the explicit rule
may return Frick.

Other banks stay a fallback. The rule applies only when a receiving Frick EUR row exists. If it is
receive=false, the request falls through to the existing selection instead of failing.

Ambiguity is resolved deterministically. (name, currency) is not unique in the schema, so if
several Frick EUR rows were ever active at once, the highest id wins — the same way
getBankInternal already resolves an ambiguous pair. Otherwise the displayed IBAN could change with
the repository's row order.

Scope

The outbound path is untouched: FiatOutputService keeps excluding Bank Frick from automatic sender
selection, so payouts still require explicit assignment. Deposit routing and payout selection are
independent directions.

Two changes unrelated to the routing rule are kept because they are correct either way:
getBank()/getMatchingBank() now declare the Bank | undefined both find() calls can produce,
and buildBankResponse states the non-null precondition it already relied on.

Trade-off

This is a code constant, not configuration: switching the EUR deposit target back, or to another
bank, requires a release. That is the explicit product decision behind this PR — an earlier revision
implemented it as a Bank.receivePriority column so the switch would be a data change, and that
approach was dropped in favour of the hardcoded rule.

Tests

bank.service.spec.ts covers the rule and each of its limits: an EUR transfer reaching Frick even
when the incumbent is listed first; the fallback when the Frick row is receive=false; the EUR row
winning over the Frick CHF row; the highest-id row winning when two Frick EUR rows are active; a CHF
request not being captured by a Frick franc row listed first; an EUR instant request with no
sctInst bank available still not reaching Frick; a card request not reaching it either; and the
unsupported-currency fallback landing on the incumbent in both input orders.

Every case lists the expected winner where input order alone cannot explain a pass. The rule's three
guards were additionally checked by breaking them one at a time: removing the categorical exclusion
fails exactly the four path tests, weakening === BANK fails the card test, and dropping the id sort
fails the tie-break test.

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.
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.
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

❌ ESLint: 1 errors, 2 warnings

…d 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.
… 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.
…igibility

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.
…uilder

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.
…sit 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.
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.
… 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.
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.
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Ready after 6 review passes (2 independent reviews per pass: conformance and logic).

Substantive changes made during review, in the order they were found:

  1. Ranking alone did not preserve the previous behaviour. The removed filter excluded a bank
    categorically; a low rank merely deprioritises it. Whenever the incumbent for a currency was
    missing or receive=false, or the sctInst lookup fell through to the general fallback, the
    previously excluded bank would have been shown to a customer without anyone switching it on.
    receivePriority is now nullable — NULL means not eligible — and eligibility is filtered
    before ranking.
  2. The migration had been edited in place while keeping its class name. TypeORM identifies a
    migration by name, so a database that had already run the earlier version would never have run
    the corrected one. It now carries a fresh timestamp and class name; if the old version did run
    somewhere, ADD COLUMN fails loudly instead of leaving a silently wrong state.
  3. The seed data did not carry the new column. A fresh database is migrated while bank is
    still empty, so the backfill touches nothing and the seed then inserts every bank without the
    column — leaving local, CI and onboarding environments without a single eligible deposit bank.
  4. Return types. getBank() and getMatchingBank() now declare Bank | undefined, which both
    find() calls can produce, and buildBankResponse states its non-null precondition instead of
    accepting a value it dereferences unconditionally.
  5. Scope of the eligibility guarantee. The entity comment, migration docstring, seed comment and
    this description claimed a NULL bank is never offered to a customer at all. It is scoped to the
    generic selector: explicit personal-IBAN paths resolve their own bank and are not gated by this
    column — they never were.

Beyond the test suite, the ordering and eligibility rules were checked by deliberately breaking them
and confirming that exactly the responsible tests fail: removing the id tie-breaker fails only the
two deploy-safety cases, removing the eligibility filter fails only the three NULL cases, and
replacing != null with a truthiness check fails only the receivePriority: 0 case.

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 29, 2026 19:45
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.
@TaprootFreak
TaprootFreak marked this pull request as draft July 29, 2026 20:26
@TaprootFreak TaprootFreak changed the title e710bc80 - Make the deposit-target bank an operational input e710bc80 - Route EUR deposits to Bank Frick Jul 29, 2026
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.
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.
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.
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.
…sting

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.
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Ready. The approach changed mid-review: an earlier revision made the deposit target a
Bank.receivePriority column so the switch would be a data change. That was dropped in favour of the
hardcoded rule now in the diff, and the column, its migration and the seed entries were removed again.

3 review passes on the final approach (2 independent reviews per pass: conformance and logic).
What they found, in order:

  1. The removed name filter was categorical, the new rule was not. Excluding Bank Frick by name
    kept it out of the whole selector. Replacing that with an EUR-only rule silently dropped the
    exclusion everywhere the rule does not reach: a CHF request could return the Frick franc row
    (receive=true in production, in an order the query does not guarantee), an EUR instant request
    with no sctInst bank available fell through to the generic fallback and could land there too, and
    the condition tested !== INSTANT, which is also true for CARD. The rule is now scoped positively
    to BANK, and the categorical exclusion is restored for everything after it.
  2. The tie-breaker contradicted attribution. Resolving several qualifying rows by highest id
    conflicted with selectAttributionBank, which prefers the asset-linked row because that binding is
    what isBankMatching and the booked bank_tx history are keyed on — and getReceiveBanks() does
    not even load the asset relation, so the preference could never have applied. The rule now resolves
    through getBankInternal, which is cached and does load it.
  3. Test and comment corrections. A missing case (attributed row not receiving while another Frick
    row does — no substitution may happen), and an unsafe type assertion in a new mock.

Three comments in this PR promised more than the code holds and were tightened: the guarantee is the
selection rule, not cache coherence between ibanCache and the repository cache.

Each guard was additionally checked by breaking it: removing the categorical exclusion fails exactly
the four path tests, weakening === BANK fails the card test, dropping the attribution resolution
fails the asset-preference test, and removing the receive check fails the fallback test.

Two pre-existing weaknesses were found but deliberately left alone, as they concern code this diff
does not touch: ibanCache is loaded once at module init while bank rows are otherwise read through a
5-minute cache, so the two can disagree after a runtime edit until restart; and
getBankInternal() declares Promise<Bank> while returning undefined for an empty result, with
three existing callers dereferencing it unguarded.

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 29, 2026 22:19
@TaprootFreak
TaprootFreak merged commit 9010318 into develop Jul 30, 2026
12 checks passed
@TaprootFreak
TaprootFreak deleted the feature/frick-eur-deposit-selector branch July 30, 2026 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant