perf(buy): resolve the user and the active vIBAN once per payment-info request - #4543
Closed
Danswar wants to merge 4 commits into
Closed
perf(buy): resolve the user and the active vIBAN once per payment-info request#4543Danswar wants to merge 4 commits into
Danswar wants to merge 4 commits into
Conversation
…o request
PUT /v1/buy/paymentInfos issued the same two lookups twice. Measured against production
traces of typical requests (~106 ms, 13 DB queries, 74% of the request spent in the
database): the user accounted for 4 of those queries and the vIBAN for 3.5.
- The user was loaded in createBuyPaymentInfo and again in toPaymentInfoDto. Each load
costs two queries, because TypeORM emits its DISTINCT-id pattern for findOne with
relations. toPaymentInfoDto now accepts the already-loaded user; the standalone
callers keep their previous behaviour through the optional parameter.
- getTxDetails already resolves the user's active vIBAN to pick the receiving bank for
the fee. It now returns that result so the deposit-destination step reuses it instead
of repeating the query. null distinguishes "looked up, none found" from undefined
("no lookup ran"), so a caller that never triggered one still resolves it itself. The
lookup still happens inside getTxDetails, so the ordering that keeps a failed quote
from creating an external account is unchanged.
Sharing one relation set between both loads also fixes a latent defect: createBuyPaymentInfo
requested userData.wallet, while buyCheck reads user.wallet — which was therefore always
undefined there. Both escape hatches guarded by it were inert: SKIP_AML_CHECK, and
autoTradeApproval, which is meant to let a wallet without tradeApprovalDate through rather
than failing it with RecommendationRequired. The equivalent check in getTxDetails does have
the wallet loaded and only emits a soft QuoteError, so the two disagreed. The resulting
behaviour change is gated by DisabledProcess(TRADE_APPROVAL_DATE).
userData.users is dropped: nothing reachable from this endpoint reads it, and it fanned the
user query out by the number of users on the account. userData.organization is kept, because
UserData.address reads organization.country and TypeORM joins the eager relations of a
requested relation one level only.
Review follow-up on the payment-info deduplication. Reusing the vIBAN lookup was wrong for the negative case. A "none found" answer is a whole getTxDetails (pricing, fees, limits) old by the time resolveBankInfo sees it, and the branch that follows issues an IBAN. If a concurrent request issued one in that window, the stale negative caused createForUser to hit a duplicate, swallow the ConflictException and fall into the fail-closed throw — a 400 PersonalIbanIssuanceFailed where the previous fresh read returned the IBAN. Bank 15 issued 460 user-level vIBANs in the last 90 days and several accounts hold multiple active ones, so concurrent requests demonstrably reach this path. Only a positive result is reused now; the negative path re-reads, which costs one SELECT before an external issuance call anyway. toPaymentInfoDto now rejects a preloadedUser whose id does not match userId — the transaction request is attributed to userId, so a mismatch would book it against another account — and documents that the user must carry PAYMENT_INFO_USER_RELATIONS, since getTxErrors dereferences user.wallet without optional chaining. Test gaps that let the change revert unnoticed are closed: getTxDetails actually returning the resolved vIBAN, createBuyPaymentInfo handing its user down, and the relation set itself (getUser is mocked everywhere, so deleting wallet: true was invisible). The trade-approval gate that the shared relation set restores now has direct coverage in a new payment-info.service spec.
…f documented Review follow-up. The previous commit fixed the stale-negative race with a runtime check, but kept a null/undefined tri-state that nothing consumed and whose doc still told callers a negative result was reusable — the exact reasoning that produces the fail-closed PersonalIbanIssuanceFailed. getBankIn now normalises "none found" to undefined and the type is narrowed to VirtualIban | undefined, so a negative cannot be represented at all and reuse is only ever a positive hit. The doc says why, rather than describing a distinction that no longer exists. Also from review: the negative-path test asserted only a call count, so a mutation that re-read and then discarded the result still passed. Its fresh read now returns a vIBAN — standing in for one issued concurrently in that window — and the test asserts that IBAN reaches the response. Smaller conformance fixes: sorted and absolutised the new spec's imports, added the missing return type on its user() helper and used the KycLevel enum rather than a bare 50, and gave the preloaded-user guard a capitalised message without the methodName prefix, which appears nowhere else in src.
Review follow-up, no behaviour change.
The negative-path test moved from CARD to BANK when its fresh read started returning a vIBAN,
but kept the comment explaining why CARD was necessary. That rationale belongs to the following
test, which is still CARD, and was actively wrong where it stood.
The preloaded-user JSDoc pointed at a module-private const via {@link}, so a cross-module caller
could not reach the contract it was told to satisfy. The relation set is spelled out instead,
which keeps the const private and the guidance usable.
Also sorts the new spec's process.service import into its alphabetical position, which the
previous commit claimed to have done.
Collaborator
Author
|
Superseded by #4545, which carries the same reviewed change on a fresh branch off the current develop. |
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.
Removes duplicated work from
PUT /v1/buy/paymentInfos, and fixes a latent defect that falls out of it.Why
This endpoint is the heaviest database consumer among the customer-facing routes, which is what makes it the first to suffer whenever the database is under load. Measured against production traces of typical requests (60–250 ms band): ~106 ms, 13.3 DB queries, 74% of the request spent waiting on the database. Of those queries,
Useraccounted for 4.0 andVirtualIbanfor 3.5 — both because the same lookup ran twice. For comparison,PUT /v1/buy/quoteruns the same fee and price calculation without these lookups and sits at a 2 ms median.What changed
The user was loaded twice.
createBuyPaymentInfoloaded it, thentoPaymentInfoDtoloaded the same user again by id. Each load costs two queries, because TypeORM emits itsDISTINCT-id pattern forfindOnewith relations.toPaymentInfoDtonow takes the already-loaded user; the standalone caller (realunit.service) keeps its previous behaviour through the optional parameter, and a mismatched(userId, preloadedUser)pair is rejected rather than booking the transaction request against another account.The active vIBAN was looked up twice.
getTxDetailsalready resolves it to pick the receiving bank for the fee, andresolveBankInforesolved it again for the deposit destination.getTxDetailsnow returns what it found so the second step reuses it.Only a positive result is reused. A "none found" answer is deliberately re-read. By the time
resolveBankInfosees it, it is a wholegetTxDetails(pricing, fees, limits) old, and the branch that follows issues an IBAN — so a vIBAN issued concurrently in that window would be missed,createForUserwould hit a duplicate, swallow theConflictException, and the request would fall into the fail-closed throw. That is a 400PersonalIbanIssuanceFailedwhere a fresh read returns the customer's IBAN. To keep that from being re-introduced as a future "optimisation", a negative result is normalised toundefinedat the single point that produces it (getBankIn), and the type isVirtualIban | undefined. Note this is enforced by tests, not by the compiler — the repo sets neitherstrictnorstrictNullChecks, sonullstays assignable. TwogetBankInspecs fail if the normalisation is dropped.Net query count: BANK/INSTANT with a personal IBAN 1 (was 2); BANK/INSTANT without 2 (was 2); CARD 1 (was 1); Frick selector 0 (was 0). Never more than before.
The lookup still happens inside
getTxDetails, in the original order. Resolving it earlier would run vIBAN machinery before the quote succeeds, which the existing tests correctly reject.Latent defect fixed
Unifying the two loads onto one relation set fixes a real bug.
createBuyPaymentInforequesteduserData.wallet, butbuyCheckreadsuser.wallet— a different relation, never loaded, so it was alwaysundefined. Both escape hatches guarded by it were inert:SKIP_AML_CHECKautoTradeApproval, which is meant to let a wallet without atradeApprovalDatethrough rather than rejecting it withRecommendationRequiredThe equivalent check in
getTxDetailsdoes have the wallet loaded and only emits a softQuoteError, so the two disagreed on the same condition.autoTradeApprovalis set on roughly half of the configured partner wallets — this is not a marginal case. Their users without atradeApprovalDatewill now be let through where they previously received a 400. That is what the flag is for, but it should be confirmed rather than shipped silently — the affected wallet list and the live flag state are in the internal review thread. The change is strictly loosening: in production the AML term of the condition is always true, so loading the wallet can only admit more, never reject more.Relations
userData.usersis dropped — nothing reachable from this endpoint reads it (checked the service call tree, the entity getters that dereferencethis.users, and the absence of any@AfterLoad/subscriber), and it fanned the user query out by the number of users on the account.userData.organizationis kept, and looks equally removable but is not:UserData.addressreadsorganization.country, and TypeORM joins the eager relations of a requested relation one level only — soorganization.countryis joined only whenorganizationis requested explicitly. Dropping it would silently blank the recipient country on CHF personal IBANs and drop the QR debtor for organization accounts.Verification
tsc --noEmit,prettier --checkandeslintclean.getTxDetailsreturning the resolved vIBAN,createBuyPaymentInfohanding its user down, the relation set itself (getUseris mocked everywhere, so a droppedwallet: truewould otherwise be invisible), and the id guard.payment-info.servicespec.