Make the collection-IBAN toggle readable and let the QR code follow it - #1270
Conversation
|
Blocking on one point: the SDK must not be bypassed.
const { call } = useApi();
...
const url = collectionAccount ? `${BuyUrl.invoice(txId)}?collectionAccount=true` : BuyUrl.invoice(txId);
const response = await call<PdfDocument>({ url, method: 'PUT' });That moves endpoint knowledge — HTTP verb, query-string shape, response type — out of The correct place for the flag is the SDK. In // BuyInterface
invoiceFor: (txId: number, collectionAccount?: boolean) => Promise<PdfDocument>;
// implementation
const invoiceFor = useCallback(
async (txId: number, collectionAccount?: boolean): Promise<PdfDocument> =>
call<PdfDocument>({
url: collectionAccount ? `${BuyUrl.invoice(txId)}?collectionAccount=true` : BuyUrl.invoice(txId),
method: 'PUT',
}),
[call],
);Then this component goes back to Please split that out into a Rest of the PRThe remainder looks good and I have no objection to it:
|
The review on DFXswiss/app#1270 asked for one thing: give useBuy().invoiceFor an optional collectionAccount parameter so the app stops hand-building the request. It supplied the code and called it a two-line change in buy.hook.ts. The previous two commits went further — they widened BuyApi.getInvoice and added a core test file. Both are reverted here. BuyApi.getInvoice has no caller in this monorepo or in services and was reachable only through DfxApiClient.buy, so widening it was not required to unblock the consumer, and carrying it made this PR larger than what was asked for. What remains is the reviewer's own code, verbatim: the optional parameter on the interface, and the ternary inline in the call. Every existing caller stays source-compatible. Consequence, stated rather than hidden: the switch ships with no test. @dfx.swiss/react defines no test script — per CONTRIBUTING only core and bip322-multisig do — and this PR does not add test infrastructure to react. The same is true today for the includeTx switch in swap.hook.ts and sell.hook.ts.
|
Agreed, and split out: DFXswiss/packages#204.
One addition beyond what you asked for, and I would rather name it than have you find it: the PR also Also worth correcting in my own PR body here: the parameter is omitted on The side effect you predicted holds: this spec mocks The consuming commit is written and verified against #204's head — Two things on #204 I cannot close myself: its CI sits at |
…unt (#204) * feat(core,react): let the buy invoice call target the collection account PUT buy/paymentInfos/:id/invoice accepts an optional collectionAccount query switch: when set, the API issues the invoice against the shared DFX collection account instead of the personal virtual IBAN stored on the request (DFXswiss/backend#4686). The switch lives in BuyUrl.invoice, the single place that owns the URL shape, and is passed through by both callers — BuyApi.getInvoice and useBuy().invoiceFor. Putting it in either caller would have placed the query form next to the other one instead of inside it. The parameter is omitted rather than sent as false, because the API maps any present value to true (Util.mapBooleanQuery). It is optional, so every existing caller stays source-compatible: this is additive. Consumer: DFXswiss/app#1270, which currently hand-builds this URL through useApi() because the SDK does not carry the switch. * refactor(core,react): build the collection-account query the way the repo does Review found the first commit hand-rolled the query inside the URL builder. That is the construct commit c6c0411 ("Improve @dfx.swiss/core package quality", #152) removed across the package: definitions/*Url carries the path, the core client builds its query with Utils.buildQuery, and the react hook builds it inline. The exact twin is the optional includeTx switch — SwapApi.ts:14 and SellApi.ts:14 on the client side, swap.hook.ts:23 and sell.hook.ts:20 in the hooks. So BuyUrl.invoice is back to its develop state, BuyApi.getInvoice follows SwapApi.createPaymentInfo, and useBuy().invoiceFor follows swap.hook.ts. The resulting URLs are unchanged. Also drops the comment attributing the design to Util.mapBooleanQuery. That attribution was wrong: this endpoint compares the query against the literal 'true' (DFXswiss/backend#4686) and deliberately does not use presence semantics. Omitting the parameter on false stays correct; only the stated reason was. The three getInvoice tests now assert the returned document as well. Without that, an implementation returning a wrong object passed all of them and tsc -b too, while both sibling specs in the same directory already assert it. * refactor(react): narrow the change to exactly the requested hook edit The review on DFXswiss/app#1270 asked for one thing: give useBuy().invoiceFor an optional collectionAccount parameter so the app stops hand-building the request. It supplied the code and called it a two-line change in buy.hook.ts. The previous two commits went further — they widened BuyApi.getInvoice and added a core test file. Both are reverted here. BuyApi.getInvoice has no caller in this monorepo or in services and was reachable only through DfxApiClient.buy, so widening it was not required to unblock the consumer, and carrying it made this PR larger than what was asked for. What remains is the reviewer's own code, verbatim: the optional parameter on the interface, and the ternary inline in the call. Every existing caller stays source-compatible. Consequence, stated rather than hidden: the switch ships with no test. @dfx.swiss/react defines no test script — per CONTRIBUTING only core and bip322-multisig do — and this PR does not add test infrastructure to react. The same is true today for the includeTx switch in swap.hook.ts and sell.hook.ts. * test(core): cover the collection-account switch on the buy client The hook edit alone shipped the switch untested: @dfx.swiss/react has no test runner, so nothing in this repo would have noticed if the parameter stopped being forwarded. BuyApi.getInvoice calls the same endpoint and lives in the one package that does run tests, so the switch is covered there. It follows the pattern the package already uses for optional boolean queries — Utils.buildQuery with `value || undefined`, exactly as SwapApi.createPaymentInfo and SellApi.createPaymentInfo handle includeTx. BuyUrl.invoice stays path-only. The hook keeps the form the review asked for; core and hook build the query differently, which is the same split swap.hook.ts/SwapApi.ts and sell.hook.ts/SellApi.ts already have. Both produce identical URLs for every value the type permits. Three tests, each asserting the full request options and the returned document. Verified by mutation: dropping `|| undefined` fails 2 of 3, not appending the query fails 1 of 3, returning a wrong document fails 3 of 3.
Reviewer exceptions, granted in writingCONTRIBUTING § Every pull request is self-contained allows a deferral only with an explicit written reviewer exception. As the reviewer on this pull request I grant three, each with its reason:
Two review findings were examined and rejected rather than deferred, recorded here for completeness: a dedicated baseline for the |
Review closureNine full review passes ran over this pull request, each covering the complete base→head diff from two directions in parallel — guideline conformity and logic/correctness — with every finding re-verified at its cited line before it counted, and each round of fixes followed by a fresh full-diff pass rather than a delta look. The passes drove the hardening commits recorded in the description: the SDK consumption, the stale-response guard and its session keying, the GiroCode gate (carrier, charset, whitespace, line count, amount grammar), the surviving PDF-invoice button, nine committed visual baselines, and the honest test mocks. The ninth pass returned zero unresolved findings on the diff; its remaining notes were metadata drift from work merging on Every finding across all passes ended in exactly one of three places: fixed in a commit on this branch, pinned as a committed baseline, or granted a written reviewer exception in the comment above. Nothing was deferred silently. Verification on the final head, run locally against |
a779b19 to
6fe8e25
Compare
|
@marassteiner please review This is a PR, not an issue: 25 files against |
Review closure — post-rebase addendumThe closure above described the branch before #1305 (the CHF cutover) merged on That pass confirmed the integration and surfaced four last items, all closed on this head: the rewrite now gates on the quote's own currency (EUR) instead of trusting the payload contract, refuses anything beyond EPC069-12's twelve fields, the three currency e2e cases inherited from #1305 carry the file's Final verification on this head, against The four written reviewer exceptions above stand unchanged; nothing else was deferred. |
The toggle added in DFXswiss#1258 works, but it is hard to read: it uses a colour emoji next to the monochrome DFX icons, it never says which of the two accounts is currently shown, and it never states the reason it exists, so the customers it was built for do not find it. - Replace the emoji with IconVariant.SWAP, without size and colour props so it matches the neighbouring CopyButton exactly. Drop the extra ml-1, which broke the row's gap-3 rhythm, and add a focus ring and aria-pressed. - Name the state and the reason in the row's existing infoText slot, the same pattern the remittance-info row already uses. On the collection account the hint also states that the remittance info is mandatory: a transfer there is attributable through the reference only. - Carry the switch into the QR tab. It encoded the personal IBAN even after switching, so it handed the customer the very IBAN their bank rejects. toCollectionIbanGiroCode rewrites line 7 of the EPC payload and nothing else; holder, amount and remittance stay identical. Both the text and the QR branch are gated on canOfferCollectionIban, so display, copy and QR can never diverge. It fails closed - if the payload is not a well-formed SCT GiroCode carrying the displayed IBAN, no QR code is rendered at all and the screen asks for manual entry. Behaviour and gating are unchanged; canOfferCollectionIban is untouched. The accessible names stay verbatim so the tests from DFXswiss#1258 remain valid.
The invoice button sits inside the QR tab, and the document it opens is rebuilt server-side from the stored request - so it kept naming the personal vIBAN while the screen, the copy button and the QR code already showed the collection account. The customer ended up holding two documents naming different receiving accounts. DFXswiss/backend#4686 adds an optional collectionAccount switch to PUT /buy/paymentInfos/:id/invoice. This sends it while the collection account is displayed, using the SDK's own BuyUrl.invoice builder through useApi so the request stays identical apart from the query. The parameter is omitted, never sent as false, because the backend maps any present value to true. The three rejection tokens the endpoint can answer with all mean the same thing to a customer - the invoice cannot be issued for the collection account right now, the details on screen still apply - so they share one message while staying separate tokens for the logs.
Review of the api side found that the reference guard behind CollectionAccountInvoiceReferenceMissing could not be reached: buy.bankUsage is NOT NULL in the schema and set on every route creation, so the guard and its token were removed there. Mapping a token no consumer can receive would be dead code on this side too.
The api renames CollectionAccountInvoiceRequiresPersonalIban to CollectionAccountInvoicePersonalIbanMissing, so the token says what the guard found instead of what it wanted. The customer-facing message is unchanged; only the string it matches on moves.
CONTRIBUTING requires every touched file to reach 100 percent on all four metrics and the per-file numbers to be stated in the description. Measured on the previous head they were 70.96/94.59/40/68.96 for payment-info-buy.tsx, 90.32/88.23/100/92.85 for payment-qr-code.tsx and 98.82/98.76/100/100 for personal-iban.ts. The gaps were the render branches and copy callbacks of the payment rows, the early return that sends a customer with incomplete KYC to their profile instead of issuing an invoice, and the generation guard in the catch block - the error half of the stale-response protection, which until now had only its success half pinned. That last one is the reason this is more than bookkeeping: a late rejection arriving after the customer switched modes must not surface an error for a mode they have left, and nothing held that property in place. Tests only.
The buy spec asserted twice that the manual-entry hint is not visible, but nothing ever captured it when it is: a GiroCode whose remittance line does not carry the quote's reference must render no QR at all. The new case mirrors the neighbouring QR test with one changed payload line and pins the state to a baseline like the other four.
The fail-closed rewrite gates the QR image, but the invoice is built server-side and does not depend on it — replacing the whole component with a hint also took the button away. The hint now renders inside PaymentQrCode with the button below it, pinned by a regenerated baseline. Three hardenings alongside: the generation guard bumps in a layout effect so a response cannot land between commit and the passive-effect flush; the GiroCode parser validates the EPC character-set line (1-8) and no longer repairs leading whitespace by trimming the whole payload; and the collection-invoice error state gets its own e2e case and baseline.
The api emits the reference exclusively on unstructured line 10, and a quote reference is never a valid ISO 11649 structured reference — this function never validated one, so a payload carrying the reference on line 9 is not the api's output and is now refused instead of rewritten. This subsumes the dual-carrier refusal. Two test gaps closed alongside: the offer-gate rerender case now pins the mock attributes, and a txId change during an in-flight request is pinned to discard the response.
The caller guarded toCollectionIbanGiroCode with a ternary whose false arm is unreachable — canOfferCollectionIban already requires iban and remittanceInfo — and covering it meant forcing the gate mock into a state the app cannot reach. The function now accepts both fields as possibly undefined and refuses first thing when either is missing, the caller passes the quote fields straight through, and the refusal is pinned by two direct unit tests. The module mock in the toggle spec is gone with it: every remaining case tests through the real gate.
A deep-link param push replaces the session in place — wallet.context applies updateSession without unmounting the screen — so an invoice requested under the previous session could still open after the swap. The generation guard now also keys on user.accountId, which changes exactly when the signed-in identity changes and not on periodic context refreshes, so legitimate in-flight downloads are not discarded.
An in-place token swap changes the session synchronously, but the SDK's user context reloads only when the logged-in boolean flips — which it does not across a swap — so user.accountId can keep serving the previous account and the guard added for this scenario never fired. The guard now keys on session.account from useApiSession, and the test models reality: the session changes while the mocked user stays stale. The GiroCode docstring also stops asserting backend behaviour it cannot prove; the structured-carrier refusal is now justified locally — no ISO 11649 validation exists here, so that carrier is outside the validated shape, and a wrong refusal costs only the QR image.
The toggle baselines are shot against a quote without a paymentRequest and therefore show the tabless presentation — the Text tab with the tab bar visible in the collection state had no baseline. And the fail-closed QR state and a rejected invoice can occur together, but neither test ever produced them at once. Both states are pinned now, and the toggle test carries the same lang=en pin as its siblings so the locale of the test account cannot decide what the baselines show.
The guard now keys on session.account and session.user, so a token swap keeping one but changing the other still re-arms, and a concurrency test pins that a stale response cannot clear a newer request's loading state. The e2e invoice-error case asserts the request actually carries collectionAccount=true instead of trusting a mock that answers anything. canOfferCollectionIban refuses whitespace-only fields, matching the trim semantics of the rewrite it gates, and the rewrite's length guard states its real minimum: eleven lines, since the unstructured reference on index 10 is required — which also let the now-dead length ternary go. The Text tab with the tab bar visible is pinned in both toggle states.
The rewrite validated every field it depends on except the amount: a payload whose line 7 disagrees with the quote would have been rewritten, and the collection QR would offer a different amount than the Text tab beside it. Line 7 must now carry EUR plus exactly the quote's amount — numeric equality, so EUR100 and EUR100.00 both match 100 — and a missing amount refuses like the other quote fields.
The toggle spec's StyledTabContainer mock rendered every tab at once — a state the real component never produces, since it renders only the active tab. The mock is stateful now, tabs switch by click like in production, and the QR assertions activate their tab first.
Number() coerces more than EPC069-12 allows: EUR1e2, EUR0x64 or a bare EUR would have compared equal to a clean amount. The numeric part must now be plain digits with at most two decimals, and the comparison runs in cents, so EUR100 and EUR100.00 still both match 100 while exponent, hex and signed spellings refuse.
Nine integer digits is where EPC069-12 stops (999999999.99), and a canonical amount carries no leading zeros — EUR000000000100 numerically equals 100 but is not the format the standard permits, so both now refuse rather than compare.
…s#1305 The rebase onto develop kept this branch's versions of five files, which carried the pull request's hardenings but predated DFXswiss#1305's EUR+CHF generalization. This commit restores that architecture — the per-currency collection map, FRICK_CURRENCIES, getFrickCollectionIban and getOfferableCollectionIban — and lifts the hardenings onto it: the whitespace gate lives in getOfferableCollectionIban now, the display follows the per-currency IBAN, and the GiroCode rewrite keeps every gate while stating why it stays EUR-bound: EPC069-12 exists for EUR only, a CHF quote carries a QR-Bill the first check refuses, so CHF fails closed into the hint with the PDF button. The DFXswiss#1305 test coverage travels along — per-currency unit cases, the CHF toggle and the two non-Frick-currency e2e cases — and the three CHF baselines are regenerated on this branch, which changes the toggle's look the same way it does for EUR.
The EUR binding rested on the backend's payload contract — a CHF quote carries a QR-Bill the svg check refuses. Now the quote's own currency must say EUR before anything else is read, and a payload longer than EPC069-12's twelve fields is not reassembled. The three currency e2e cases inherit the file's lang=en pin, and the CHF fail-closed QR state gets its own baseline: hint and PDF button, no code.
The amount line is optional in EPC069-12, and the api leaves it empty whenever the customer quotes by target amount — the two amount fields are mutually exclusive, so buying 0.05 ETH sends no source amount at all. The gate demanded EUR plus a figure and refused those payloads, so the collection QR silently disappeared for a perfectly ordinary flow while the personal QR still rendered. An absent amount contradicts nothing: it is the same absence the personal QR carries, and the customer types the figure in their banking app. A populated amount must still match the quote to the cent.
DFXswiss#1320 rewrote the same five files while this branch waited, so the rebase left them holding one side or the other. They now carry both: develop's provider generalization untouched — the Yapeal parsing, the precedence helper, the provider switch and every test around them — and this branch's work lifted back on top of it. Where the two genuinely met, the collection toggle keeps its own state in the content component so the QR branch can see it, and the provider switch stays wired exactly as develop passes it down; both buttons sit in the same IBAN row. The dependency bump is gone: develop already carries a newer SDK than this branch pinned.
7fc3dec to
3cfecc0
Compare
A line of spaces is not an absent amount: absent states nothing, spaces state nonsense, and everywhere else this function refuses malformed input rather than passing it through. Only a genuinely empty line takes the no-amount path now. The comparison also stops rounding to cents, which let a quote of 100.004 pass against a payload reading EUR100.00 while the screen showed and copied the unrounded figure. Plain equality accepts every canonical spelling — EUR100 and EUR100.00 both parse to 100 — and rejects the divergence. The e2e case that pinned the empty-amount payload is gone with them: it carried no screenshot, so it proved function rather than appearance, and the same property is already pinned where it belongs, in the unit test.
Two decimals is the standard's limit for what a payment carries, not a limit on what a customer may enter: the amount field takes any figure and the api writes it into the payload unformatted, so a purchase of 100.004 produced a line the grammar rejected and the collection QR disappeared for it. The character guard stays — plain decimal digits, so nothing can be read as a number other than what stands there — while the equality against the displayed amount does the deciding, which is the only property that ever mattered here. The e2e cases this branch adds fake two things the suite never faked before: a customer's KYC completeness and the invoice endpoint's rejection. CONTRIBUTING requires each fake to be declared with what a green run does not prove, and this repository had nowhere to put such an entry — the section exists now, with all three fakes of these specs.
Three rounds tuned a decimal bound that was arbitrary in both directions: too tight and a legitimate purchase lost its QR, too loose and two different amounts collided onto the same floating-point value near a hundred million, so a payload could carry an amount the screen never showed. The bound is gone. The api renders that line as the currency prefix followed by the quote amount, so the exact string is known here and comparing against it needs no parsing, cannot be read as a number other than what stands there, and leaves nothing left to tune. A payload spelled differently is not this api's output and is refused.
Both collection-invoice cases install the KYC and invoice-400 fakes, not one.
The rewrite kept line 5 as it found it and trusted the toggle's own gate for the holder. Both accounts are held by DFX AG, so a payload naming anyone else is not one this function may retarget.
A bare prefix test also accepts a company whose name merely begins with the holder's, so the name must end at the address separator or the line's end.
|
EN: Ready after one confirmation review pass on the current head. DetailsConfirmation pass on Written reviewer exceptions left in place: deploy DFXswiss/api#4686 first; the tab-switch download loss from the unmount/logout guard; three pre-existing Playwright failures inherited from #1320, which this branch does not touch Gates on this head: |
DFXswiss#1270 added the reality-declaration section after this branch wrote a separate file. The same fake belongs with the others.
|
EN: Working on this now — job |
|
EN: DE: DetailsHead reviewed: Gates on this head (one complete pass, two dimensions then two vendors):
Coverage (review gate, not CI): the description and the earlier written closure state 100% statement/branch/function/line on Handbook: Prior written exceptions left in place: companion API deploy-order named in the description; tab-switch download loss from the unmount/logout guard; transaction-list invoice divergence tracked as a separate API issue; no full-stack harness toggle case in this pull request ( Notes that are not defects: no positive unit case for GiroCode version CI on this head: Build and test, Full-stack E2E, review, CodeQL success. No unresolved review threads. |
3 participants
The blocking review point is closed
"The SDK must not be bypassed." — closed, and twenty-seven of this branch's thirty-three commits are @TaprootFreak's own, not mine. Recording that here so the history is not misread: they were the last three until
b3325b78added the coverage the newer guidelines require, and the twenty-two reviewer commits after it close the last review items — the toggle button as a plain action withoutaria-pressed, the fail-closed QR state captured as a baseline, the GiroCode rewrite narrowed to the unstructured remittance carrier, the PDF-invoice button surviving the fail-closed QR state with the invoice-error state pinned as a baseline of its own, the once-dead guard branch deleted by letting the rewrite gate itself on missing quote fields, the invoice guard re-armed on an in-place session swap — keyed on the session itself because the user context can lag the swap — the two combinatorial baseline gaps closed, and the whole set lifted onto #1305's per-currency world after the rebase.d388d8105, merged 2026-08-07) and shipped as@dfx.swiss/react@1.8.0-beta.0. Checked against the published tarball rather than the PR:buy.hook.d.tsdeclaresinvoiceFor: (txId: number, collectionAccount?: boolean)andbuy.hook.jsbuilds the query.^1.8.0-beta.0;develophas since moved to^1.8.0-beta.2, so the second rebase dropped the bump entirely and the call runs against develop's own SDK.payment-qr-code.tsxcallsinvoiceFor(txId, collectionAccount)throughuseBuy()(fa1f33e6);useApi,BuyUrland the hand-built URL are gone, the spec mocksuseBuythe way the sibling specs do, and the URL shape is now asserted where it is built.ca4516efadds a generation-counter stale-response guard so a PDF cannot open for a mode or session the UI has left, and01b2f1d8refuses to rewrite a GiroCode whose remittance line does not carry the quote's reference — an unattributable transfer to the shared account is exactly what must not be produced. Read and re-measured here, not independently re-derived.Still open, and not closable from this branch
@dfx.swiss/reactto a stable1.8.0when convenient. Not a blocker — the range already covers it and this repo consumes betas elsewhere — but a beta should not sit in a production dependency indefinitely. Needs whoever runs the npm release.Fakes are declared
The visual-regression specs this pull request extends fake two things the suite never faked before: a customer's KYC completeness (
**/v2/userwithkyc.dataCompleteforced true, because the invoice button is gated on it) and the invoice endpoint's rejection (400with a fixed error token). CONTRIBUTING requires each fake to be declared in the same pull request with one plain sentence on what a green run does not prove, and this repository had no place to put such an entry — the requirement points atDFXswiss/apifor the taxonomy, and the only section here concerns the full-stack harness.docs/test-architecture.mdnow carries aReality declaration — entriessection listing all three fakes of these specs, each with what its green run leaves unproven and where that property is pinned instead.Rebased a second time, onto the Yapeal generalization
#1320 ("Let legacy Yapeal CHF holders switch between their old and new personal IBAN") merged while this pull request waited for review and rewrote the same five files. The branch is rebased onto it, and the reconciliation sits in its own commit rather than hidden inside conflict resolutions:
develop's provider generalization is carried over untouched — the provider parsing, the precedence helper, the provider switch and every test around them — and this branch's work is lifted back on top.Where the two genuinely met: the collection toggle keeps its state in
PaymentInformationContentso the QR branch can see it, whilepersonalIbanProviderSwitchstays wired exactly asdevelopthreads it, and both buttons share the IBAN row. The dependency bump disappeared with the rebase (see above). Suite after the merge: 93 suites / 1245 tests, the three touched production files still at 100 % on all four metrics, and all 17 collection- and provider-related Playwright cases green together.Three Playwright failures on this branch are pre-existing on
developand are deliberately not absorbed here:buy-page-wallet2,buy-page-wallet2-with-amount(4 % pixel drift each) andshould apply the personal IBAN selector directly and display Bank Frick details. This branch touches no file undersrc/screens/at all —git diff $(git merge-base HEAD origin/develop) HEAD --name-onlylists the payment component, the util, their tests, the translations, the e2e spec, the test-architecture document and the screenshot baselines — so the buy screen it renders is byte-identical todevelop's. The drift comes from #1320's own 515-line rewrite ofbuy.screen.tsxwithout regenerating those baselines. Regenerating them here would hide that, so they are reported instead.What
Design and correctness pass over the collection-IBAN toggle from #1258, lifted onto #1305's per-currency generalization after the rebase. Function and gating are unchanged in substance —
getOfferableCollectionIban(#1305's successor to the boolean gate) keeps its logic and only hardens against whitespace-only fields (matching the trim semantics of the QR rewrite), and the accessible names stay verbatim so the tests from #1258 remain valid.IconVariant.SWAPwithoutsize/colorprops, so it matches the neighbouringCopyButtonexactly (20px,#F5516C) and uses the same hover/transition classes asStyledIconButton. The extraml-1is gone — the row is alreadyflex gap-3. Adds a visible focus ring. An earlier revision also addedaria-pressed; a later commit removed it again — a button whose accessible name flips between two actions must not also carry a pressed state, the two conventions contradict each other when read together ("Show personal IBAN, pressed"). The flipping names stay, pinned by the unit and e2e tests.infoTextslot now carries eitherYour bank does not accept this IBAN? …or, once switched,This is the collection account of DFX AG. Please be sure to enter the remittance info below …. Same pattern the remittance-info row already uses — no new visual vocabulary. DE/FR/IT added.toCollectionIbanGiroCodereplaces index 6 of the EPC payload — the IBAN field — and leaves every other field untouched; the rebuild joins with\n, so CRLF input is normalized and a trailing blank line is dropped, both pinned by tests rather than implied. The numbering here is 0-based throughout, matching the code and its doc comment (an earlier revision mixed 1-based and 0-based in the same description); holder, amount and remittance reference are identical for both accounts (verified againstPdfUtil.generateGiroCodeandActivateBankFrickin the api).Fail-closed
The rewrite only happens for a well-formed SCT GiroCode of version 001/002, with the full 11-or-12-line shape (the unstructured reference on index 10 is required, and nothing beyond EPC's twelve fields is reassembled), whose creditor line 5 names
DFX AGand ends the name there — at the address separator the api writes or at the end of the line, so a company whose name merely begins with the holder's is refused, and with it every payload that names anyone else — whose IBAN line is exactly the displayed personal IBAN and whose remittance is carried on the unstructured line 10 alone. A populated structured-reference line 9 is refused outright: the quote reference this frontend receives is a bankUsage string, never a valid ISO 11649 structured reference, and this function validates no ISO 11649 — a payload carrying the reference there is outside the validated shape (this subsumes the dual-carrier case EPC069-12 forbids anyway). The worst case of a wrong refusal is a missing QR while manual entry and the PDF invoice remain. Also refused: a character-set line (index 2) outside1–8, leading whitespace beforeBCD(no silent repair), and an amount line (index 7) that is neither empty nor exactlyEURfollowed by the quote's amount as the api writes it (PdfUtil.generateGiroCodebuilds the line as the currency name plus the requested amount, and writes it empty when the quote is expressed as a target amount — both cases are covered, nothing is parsed, nothing can be coerced, and no numeric bound is left to drift; earlier revisions of this branch validated a grammar instead, and both directions of that error were real: a canonical amount refused, and two amounts colliding at float precision). One divergence is refused on purpose: the api reports the quote amount floored to two decimals while it writes the payload from the amount the client sent, so a quote requested with sub-cent precision loses the collection QR rather than offering one whose amount differs from the Text tab beside it — a rewritten QR must never state an amount other than the screen does. Otherwise no QR code is rendered at all and the tab shows the manual-entry hint while the PDF-invoice button stays, because the server-built invoice does not depend on the local rewrite: a QR with the wrong IBAN cannot be produced, and neither can one that would send money to the shared account with no way to attribute it. Both the text branch and the QR branch are gated ongetOfferableCollectionIban, so display, copy and QR cannot diverge.The invoice call is guarded the same way in time rather than in shape: a generation counter, bumped synchronously on commit (layout effect) whenever
txId, the switch or the session identity (session.account/session.userfromuseApiSession) changes and again on unmount, means a response that arrives after the user has left that mode opens no PDF and sets no error — including one landing in the window before a passive effect would have flushed. The session dependency exists because a deep-link param push can replace the session in place (wallet.context.tsxapplies it without an unmount): a PDF requested under the previous session must not open under the next one.The unmount half of that guard has a cost, and it is stated rather than left to be found.
StyledTabContainerrenders only the active tab (tabs[active].contentin the compiled package), so switching from "QR Code" to "Text" unmounts this component. A customer who presses "PDF Invoice" and switches tabs before the response arrives gets no PDF, no error and no spinner — ondevelop, where no guard exists, the document would have opened. The mode-critical half of the guard is right and must stay: a PDF naming the account the customer just left is exactly what this work removes. The tab-change half is collateral: the tab is not a mode, and the request was explicitly asked for. It cannot be narrowed away, though: logout leaves this screen by navigation, so a plain unmount is the logout path — a cleanup that spared unmounts would spare logout too and reopen the very hole the guard closes. The download returns only once the application aborts in-flight requests centrally on logout; until then the trade-off stands as accepted: losing a tab-switch download beats opening a document after the session that asked for it has ended. Declared as a deviation from § Every pull request is self-contained; the written reviewer exception is recorded in the review thread.Evidence
Re-measured on the current head with
@dfx.swiss/react@1.8.0-beta.2actually installed, running the steps of.github/workflows/pr.ymlin its own order. Suites and tests are reported separately.npm ci→ installed SDKnpm run lintnpm run testnpm run build:devnpm run widget:devPer-file coverage, as CONTRIBUTING § Coverage requires
Measured with
npm run test -- --coverage --collectCoverageFrom='<path>'per file, on the three production files this pull request touches. Scope note:collectCoverageFrominpackage.jsoninstrumentssrc/**/*.{ts,tsx,js,jsx}minussrc/**/*.d.ts, which textually matches the four touched files undersrc/__tests__/— but Jest does not instrument test files: pointing--collectCoverageFromat them explicitly returns an empty report (anAll files 0/0/0/0row and no per-file rows, the same artefact this repo's dto files show), so there are no numbers to state for them:src/components/payment/payment-info-buy.tsxsrc/components/payment/payment-qr-code.tsxsrc/util/personal-iban.tsBefore the coverage commit (
b3325b78) these read 70.96 / 94.59 / 40 / 68.96, 90.32 / 88.23 / 100 / 92.85 and 98.82 / 98.76 / 100 / 100. The numbers were not stated at all in an earlier revision of this description, which the guideline requires on its own.The dead guard branch is deleted rather than measured around. The caller's ternary
info.iban !== undefined && info.remittanceInfo ? … : undefinedexisted for TypeScript narrowing only —getOfferableCollectionIban(then still the booleancanOfferCollectionIban) already refuses a missingibanorremittanceInfo, so its false arm was unreachable through the public gate, and covering it required forcing the gate mock into a state the app can never reach. Earlier revisions first declared the branch as a deviation, then mock-covered it; both wallpapered over dead code. NowtoCollectionIbanGiroCodeacceptspersonalIban/remittanceInfoas possibly undefined and refuses first thing when either is missing — the caller passes the quote fields straight through, the ternary is gone, the module mock in the toggle spec is gone with it, and the refusal is pinned by two direct unit tests instead of a component test that manufactures an impossible state.Two mutation probes, both re-run on the coverage pass's head (
b3325b78), each patched with an asserted hit count of 1, its diff printed, and reverted afterwards:invoiceFor(txId, collectionAccount)→invoiceFor(txId)payment-qr-code-error.test.tsxfail — bothcollection-account forwardingcases, the nine others stay greenmessage.includes('CollectionAccountInvoicePersonalIbanMissing')→ the old token namepersonal-iban.test.tsfails — exactly the mapping caseThe second one is worth one sentence of method: the token appears twice in
personal-iban.ts, once in a doc comment (line 206) and once in the code (line 236). A blind replace would have hit both and proved nothing about the code path, so the patch asserts on the fullmessage.includes(...)expression. The first attempt asserted on the bare token, counted 2 and aborted — which is what an asserted hit count is for.The tests added by the last commit were mutated as well, because reaching 100 % only proves the lines execute. Baseline for the three specs together: 3 suites / 36 tests.
copy(info.bic)→copy(displayedIban))if (!user?.kyc.dataComplete)→if (false))catchblock only, leaving the two intryandfinallypayment-qr-code-error.test.tsxfails — the stale-rejection caseThe third is the one that matters: it is the property that had no test before this commit, and it fails alone, which is what says the new case pins that guard and not the two beside it.
An earlier revision of this section reported "2 of the 9" and "the seven others" for the first probe. That was measured before later commits added tests to that file; the counts above are from the coverage pass's head.
Five of the six probes from the earlier revision (QR ignoring the toggle, the empty-IBAN guard, the
<svgguard, the minimum-line-count check, the two hint texts) covered the display work and still describe it; they were not re-run on this head. The sixth —aria-pressedinverted — no longer exists: the attribute is gone and its test was replaced by one asserting the button carries noaria-pressedat all. The committed toggle baselines were not regenerated for that commit:aria-pressedpaints nothing, the removedinline-blocklost againstflexin the generated stylesheet, andvertical-alignhas no effect on a flex child — checked in the generated CSS, not assumed.Deliberate scope limits
invoiceFor(txId, collectionAccount)and@dfx.swiss/reactbuildsPUT /buy/paymentInfos/:id/invoice?collectionAccount=truefrom it. The query is omitted rather than sent asfalse, and that decision now lives in the SDK: sendingfalsewould in fact be harmless — the endpoint compares against the literal'true'and deliberately avoids presence semantics — but omitting it keeps the request byte-identical to today's whenever the switch is off, so the personal-IBAN path cannot depend on a new parameter at all. The endpoint side is DFXswiss/api#4686, which is still open and has to be merged and deployed first: until it is live the parameter is ignored, so the PDF would keep naming the personal IBAN while the screen beside it shows the collection account — the divergence this work removes, now silent. The two rejection tokens it can answer with share one customer-facing message while staying separate tokens for the logs:CollectionAccountInvoicePersonalIbanMissingandCollectionAccountInvoiceCurrencyNotSupported.getOfferableCollectionIbanrequiresisVerifiedFrickPersonalIbanResponse, which comparesinfo.bankagainstFRICK_BANK_NAME, so a Fiat Republic customer is never offered the toggle and the query parameter is never sent for them. The consequence is a gap, not a defect — that customer has the same problem the toggle exists for and does not get the toggle — and closing it means makingFRICK_COLLECTION_IBANSand the account-holder check provider-aware, which belongs to whoever releases that stage. There is no services companion for #4759 open today./buyis claimed by the harness'sbuy.spec.ts, and the newFull-stack E2Echeck runs green against the merge withdevelop. Its advisory ask (bring or update the matching full-stack test for a changed screen) is declared as the fourth written deviation in the review thread: the toggle's behaviour is pinned exhaustively by this PR's own Playwright suite (four cases, nine baselines), and extending the freshly merged harness's buy spec belongs to the harness's own follow-up, not to this branch.FRICK_COLLECTION_IBANS— the EUR row from 9e0617b2 - Add collection-IBAN toggle for Bank Frick personal EUR IBANs #1258, CHF added by 1962fdea - Follow the CHF deposit cutover in the personal-IBAN frontend #1305). Reading it fromGET /bankwould be the obvious alternative, butBankDtoexposes noreceiveflag, so the frontend could not tell whether a row still accepts incoming payments — that would trade a visible duplication for a silent one.scripts/handbook/metadata.jsonentry. The entry exists ondevelop(buy-process, whose description already names the Collection-/Personal-IBAN switch), andhandbook-check.yamldoes fire for this PR because its path filter matchese2e/screenshots/**. The baselines cover the toggle in both states, the QR tab in both states, the fail-closed state an earlier revision named as missing — a GiroCode whose remittance line does not match the quote's reference, so no QR renders, the manual-entry hint shows and the PDF-invoice button stays — the collection-invoice error state, where the endpoint's stored-detail rejection surfaces as a hint under that button, the Text tab with the tab bar visible in both toggle states (the earlier toggle baselines use a quote withoutpaymentRequestand therefore show the tabless presentation), and the combined state of a fail-closed QR with a rejected invoice — hint above, error below, reachable together in production. All generated on macOS (-darwinsuffix) from the same static quote, each asserted deterministic by a second run against the committed baseline.handbook-check.yamlbuilds the handbook from the committed baselines, it does not re-render them.transaction.screen.tsx:883opens the invoice throughgetTransactionInvoice→PUT /transaction/:id/invoice, a second api endpoint that rebuilds the same PDF fromrequest.virtualIbanIdand has no switch. A customer who switched, downloaded the invoice here and paid will be handed the personal IBAN again over there. Nothing is lost — that account stays live — but the two documents disagree. It cannot be fixed from either side today, because the choice is never persisted and the transaction list therefore has nothing to send: tracked as DFXswiss/api#4773. Declared as a deviation from § Every pull request is self-contained: closing it needs persisted state an api change must introduce first; the written reviewer exception is recorded in the review thread.Necessity
Not symptom-driven: design and correctness pass over merged UI (#1258), not a reported incident. The QR part, however, removes a concrete defect: after switching, the QR tab still encoded the personal IBAN — the very IBAN whose rejection motivates the toggle.
Scale: every EUR buy quote with a verified Bank Frick personal IBAN and a remittance reference, i.e. exactly the group #1258 was built for. The feature went live on 2026-08-04, so there are no usage figures yet; the diff changes presentation and the client-side safeguards around it; the states it introduces (fail-closed QR, invoice error) are refusals of unsafe output, not new money paths.
Smaller fix considered: replacing the emoji with the DFX icon and nothing else, about six lines — insufficient because it leaves the displayed state unreadable (both states differ only in the IBAN digits), leaves the mandatory remittance reference on the collection account unsaid, and leaves the QR tab contradicting the text tab.
Final pass (b3325b7):
Coherent: Every part serves the one title. The toggle became readable, the QR followed it, and the two things that then had to follow the QR did: the invoice call, which goes through
invoiceForinstead of a hand-built URL, and the dependency that makes that call possible. The two hardenings sit on the same path rather than beside it. The ninth commit (b3325b78) adds no behaviour — it covers the three production files the eight commits before it touch, which CONTRIBUTING requires in the same pull request, and it closes the one property that was genuinely unheld: the generation guard in thecatchblock, the error half of the stale-response protection whose success half was already pinned. No half of a change is missing: the error token matches the api, the lockfile matches the range, and the URL shape is asserted in the repository that builds it.Nothing extra: No api change lives here — the endpoint side is a separate PR with its own title, and the twin-endpoint gap it leaves is named above with its issue rather than fixed here, because fixing it needs state this PR does not introduce.
getOfferableCollectionIbanand the accessible names from #1258 are untouched in substance, so that PR's tests stay valid; the rewrite helper widened its own signature instead (string | undefined), which is what let the once-dead caller branch be deleted outright. The rewrite refuses rather than repairs: a payload that does not match is dropped and the manual-entry hint is shown. The added tests assert properties, not line execution: what the copy buttons copy, that no invoice is requested when KYC is incomplete, and that a late rejection for an abandoned mode surfaces nothing.Work in flight was read, not just the diff: open pull requests and the last two weeks of
developwere checked for branches this change makes reachable or reasoning it dates. Two results are recorded above — DFXswiss/api#4759 (second EUR rail, which this screen is safe against but does not serve) and #1288 (the E2E harness — merged meanwhile; see the scope note above). Not relevant on inspection: #1291 touches the support surface this PR does not. #1287 is not in that group and was wrongly listed there in an earlier revision: it rewritesuseClipboard, whichpayment-info-buy.tsx:122uses and which this PR's new copy-button assertions sit on. No diff conflict, but not an untouched surface — whichever lands second should re-run those assertions.Sources closed: Blocking comment of 2026-08-07 (an issue comment, not a formal review —
pulls/1270/reviewsis 0) ("the SDK must not be bypassed") — closed, by the reviewer's own commitsfa1f33e6/fa1f33e6after DFXswiss/packages#204 landed and shipped as1.8.0-beta.0. CONTRIBUTING § Coverage (#1279, merged 2026-08-07) and § Deviating from these guidelines (#1292, merged 2026-08-08) — both merged before this branch was last pushed, not after; an earlier revision of this line had that backwards. The branch was rebased ontodevelopon 2026-08-11 — not by preference but by necessity: #1305 (the CHF cutover) merged overnight and touched the same five files, leaving this branch conflicting and GitHub unable to build the merge commit its CI runs against. The rebase kept every commit intact (no squash), and the per-currency integration sits on top as its own commit (84582a5a) rather than being hidden inside resolutions: it restores #1305's architecture (FRICK_COLLECTION_IBANS,FRICK_CURRENCIES,getFrickCollectionIban,getOfferableCollectionIban), lifts this PR's whitespace gate into the offer function, keeps every GiroCode gate, and states why the rewrite stays EUR-bound — EPC069-12 exists for EUR only; a CHF quote carries a QR-Bill the first check refuses, so CHF fails closed into the hint with the PDF button. #1305's test coverage travels along (per-currency unit cases, the CHF toggle and both non-Frick-currency e2e cases), and the three CHF baselines are regenerated on this branch, which changes the toggle's look the same way it does for EUR. The per-file numbers are now stated, all four metrics are at 100 % on all three files, and the once-declared branch is gone — the rewrite gates itself, pinned by direct unit tests. All three channels re-read on this head:pulls/1270/reviews0,pulls/1270/comments0,issues/1270/comments3 — the third being the written reviewer-exception comment the deviations above cite. Linked: #1258 is the feature this follows and stays untouched; DFXswiss/api#4686 is the companion, whose deployment ordering is the open item above; packages#204 is merged and consumed; DFXswiss/api#4773 carries the twin-endpoint gap. Commits: 33 — six mine (db6e9b73,86eee1c4,4dea39d3,7fb8e0f0,b3325b78,9c3cc39d) and twenty-seven the reviewer's, each message checked against its diff. The hashes are those of the current head: this branch was rebased twice, and every earlier revision of this line named hashes that no longer exist. An earlier revision of this line said "5" and split them wrongly; it counted the wrong set. Every commit after that coverage pass closes a review item, and the messages carry the detail: the toggle as a plain action button, the fail-closed QR state and the invoice-error state as committed baselines, the rewrite narrowed to the unstructured remittance carrier, the PDF-invoice button surviving a refused QR, the dead guard branch deleted, the invoice guard re-armed on an in-place session swap and keyed on the session itself, the two combinatorial baseline gaps closed, the amount line validated and then simplified to an exact comparison against what the api writes, the per-currency lift onto #1305 and, last, the reality declaration for the fakes these specs introduce. Hashes are deliberately not enumerated here: this branch has been rebased twice, and every earlier revision of this passage named hashes that no longer exist. The two hardening commits were read and re-measured, not independently re-derived. On this head the fullpr.ymlchain ran locally (lint,test,build:dev,widget:dev, all exit 0) and the eight collection- and currency-related Playwright cases ran against their committed baselines — fifteen screenshots in total, the three CHF ones regenerated on this branch and the CHF fail-closed state new, each newly written baseline verified by a second run.