Skip to content

e710bc80 - Keep buy-bound IBANs out of the user-level personal IBAN lookups - #4515

Open
TaprootFreak wants to merge 1 commit into
developfrom
fix/buy-bound-iban-user-lookup
Open

e710bc80 - Keep buy-bound IBANs out of the user-level personal IBAN lookups#4515
TaprootFreak wants to merge 1 commit into
developfrom
fix/buy-bound-iban-user-lookup

Conversation

@TaprootFreak

Copy link
Copy Markdown
Collaborator

Follow-up to #4511. A logic review there found that the user-level lookup does not filter on
buy IS NULL, so the generic deposit path can hand a customer an IBAN bound to a different purchase.
BankTxService then assigns incoming funds on that IBAN to its stored purchase:

if (virtualIban?.buy) {
  await this.updateInternal(tx, { type: BankTxType.BUY_CRYPTO, buyId: virtualIban.buy.id });
  continue;
}

The customer pays for purchase A and the money is booked against purchase B.

Why this is safe to change now

The risk is latent, not active. Checked against production:

  • Of 3122 virtual_iban rows ever created, none has a buyId.
  • wallet.buySpecificIbanEnabled is off on all 66 wallets; asset.personalIbanEnabled is off on all
    430 assets. Those two flags gate the only path that creates a buy-bound row, so it has never run.

No existing row changes meaning, no customer is affected, and no migration is needed. The moment
someone enables those flags there would be rows to reason about — this is the cheapest point to close
it.

Why this does not contradict #4384

The test this replaces was named retains merge-base behavior by reusing a buy-bound Yapeal IBAN in the generic lookup. Its commit message explains the intent: the Bank Frick work had changed Yapeal
behaviour as a side effect, and that was rolled back so every Yapeal-reachable path matched the
merge base again.

That was a statement about the scope of that PR, not a finding that buy-bound IBANs should satisfy a
user-level lookup. This PR makes the change deliberately, on its own merits, in isolation — which is
the route #4384 left open.

The change

buy: IsNull() on both user-level lookups: getActiveReceivingForUserAndCurrency and
getActiveSendingCandidatesForUserAndCurrency. The private findActiveForUserCurrencyAndBank already
carried it, so all three user-related lookups now agree on which rows count as generally assigned to
the customer. Buy-specific callers keep using getActiveForBuyAndCurrency.

Both directions are filtered on purpose. An asymmetric condition between deposits and payouts is
exactly the kind of inconsistency that produced the previous round of findings.

The test

The behavioural test stores the generally assigned row first, so it carries the lower id, and the
buy-bound row second. Under the existing newest-first ordering the buy-bound row would win, which
means only the new predicate can make the test return the free row. Removing buy: IsNull() fails it.

@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

One review pass, clean on both dimensions — this PR is itself the fix for a finding raised during the
review of #4511, so the substance had already been argued out before the branch existed.

Verified before opening: format, lint and type-check clean; the five affected suites pass (272 tests);
the Bank Frick coverage gate stays at 100% on all four metrics.

The behavioural test was checked by mutation rather than by inspection: with buy: IsNull() removed
from the lookup, excludes a buy-bound IBAN from the generic user lookup fails, because the fixture
stores the generally assigned row first and the buy-bound row second — under newest-first ordering the
buy-bound row would otherwise win. The predicate is therefore load-bearing for that test, not
incidentally satisfied by it.

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 30, 2026 16:35
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