feat(bank): add a receive-IBAN check to the bank client and hook - #192
Conversation
The support form asks a customer which account their missing transfer went to. Today that field is a required dropdown filled from GET /bank, which lists only the collective accounts - a customer who deposits through a personal IBAN can never find their own receiving IBAN there and has to pick an account they never transferred to. The field is becoming free text, and the API gained PUT /bank/receive-iban so the frontend can tell the customer straight away whether what they typed is an IBAN DFX receives money on. This exposes that call: BankUrl.receiveIban, the ReceiveIbanStatus enum with its four wire values, the ReceiveIbanCheck response type, BankApi.checkReceiveIban on the client, and checkReceiveIban on the useBank hook. NotMatched deliberately does not claim the IBAN is unknown to DFX - personal IBANs of other accounts are never checked, and an account merge leaves personal IBANs behind on the former account, so a customer's own older IBAN can land there too. The comment on the enum says so, because the wording a frontend shows depends on it. Unlike the neighbouring list(), the new client method does not pass token: false. The endpoint distinguishes an authenticated check from LoginRequired, so the bearer token has to travel with the request whenever one exists. No existing API changes: getBanks, BankUrl.get and the Bank type are untouched.
Review found two gaps in the previous commit. The client method had no test, although CONTRIBUTING asks for tests when functionality is added to core, and the pattern was there all along - auth-api.test.ts asserts the exact call shape of a client method, token flag included. The relevant invariant here is the absence of token: false, since dropping the bearer token would make LoginRequired unresolvable and silently disable the personal-IBAN half of the check. bank-api.test.ts now pins the URL, method and body, asserts that no token key is present, and keeps list() asserted with token: false so the deliberate asymmetry between the two is written down. The enum comments were line comments. tsc strips those from the declaration output, and a consumer only ever sees dist/ - so the warning that NotMatched makes no claim about DFX ownership would never have reached the developer writing the customer-facing wording, who is exactly who it was written for. All four members are JSDoc now and reach dist/definitions/bank.d.ts, verified on the built artifact rather than the source. DfxIban is phrased as ownership rather than active receiving, because most rows in the bank registry are decommissioned accounts and a present-tense claim would be untrue.
…ban branches Review found the test title stating the inverse of the invariant it guards. The API answers LoginRequired precisely when no token arrives, so naming the token-preserving test after that status said the opposite of what it pins, and mocking LoginRequired made the wrong reading look deliberate. Title and mock now agree with the assertion and its comment; the assertion itself is unchanged. The DfxIban documentation named only the collective-account branch. The other one - the requesting account's own personal deposit IBAN - is the case the endpoint exists for, and with the enum documented as the source of truth for the customer-facing wording, leaving the positive case unstated was the worst half to omit.
The API repository documents camelCase for URL routes, and the closest sibling for this very concept - GET /v1/buy/personalIban - follows it, as do the two read-with-a-body precedents this endpoint was modelled on. The hyphenated form was an unjustified deviation. Doing it now is free: the endpoint is unreleased and has no consumer in production. Once this package ships the call, the same rename would break everyone using it. Only the route value changes. The BankUrl key, the enum values, the method and type names and the documentation all stay as they are; BankApi and the hook reference the constant symbolically and follow automatically. The test keeps a literal rather than the constant, so an unintended change to the route still fails a test.
…ree status claims Two independent reviews found that the enum documentation had been copied from the API repository without the corrections made there afterwards. Three statements were therefore still wrong here. LoginRequired described itself as "the caller is not logged in". That excludes the case it also covers: an authenticated token that carries no account-scoped identity. It now names the cause instead of the login state. DfxIban claimed that many bank rows are decommissioned. That quantifies over the contents of the production table, cannot be checked from this repository, and was never the point - what a consumer needs is that a match does not imply the account still accepts funds. NotMatched explained itself by naming a function in the API repository. A reader of this package cannot open it, and the statement can go stale there without anything failing here. The consequence a consumer actually needs survives without the mechanism: an IBAN the same customer used earlier can land in this state, so it must never be phrased as "this IBAN does not belong to DFX". The concurrency and throttling requirements lived only in the pull request description, which nobody sees who installs the package. They are now JSDoc on both public methods and verified present in the built declarations. For the React side that meant annotating the interface member rather than the function inside the hook: useBank emits as returning BankInterface, so a comment on the implementation would have produced no declaration at all and the note would have been invisible for the third time in this repository.
|
This diff went through four review passes before two independent reviews came back with no findings. What they caught: The status documentation had been copied from the API repository without the corrections made there afterwards. Three statements were consequently wrong here: The concurrency and throttling requirements were only in this description, which nobody sees who installs the package. They are now JSDoc on both public methods, verified present in the built declarations rather than in the source. On the React side that meant annotating the interface member and not the function inside the hook: A test was missing for the one deliberate asymmetry in the client: unlike Two further points were rejected with evidence rather than applied: the commit type of the route rename, since amending pushed commits is not done here and the branch already carries the commit that drives the correct bump; and a demand to release a major version because the hook interface gained a required member —
This PR stays in draft on purpose. Merging it publishes a beta immediately, and the endpoint it binds to is not merged yet — see the ordering section above. |
…erify This documentation ships as .d.ts to consumers who have only the installed package in front of them. Anything it asserts about the service behind the call is unverifiable there and goes stale without a single test turning red. Removed accordingly: that the endpoint is throttled and what HTTP 429 means, that many DFX accounts are decommissioned, that an IBAN the same customer used earlier can end up unmatched, that another customer's personal IBAN is never checked, and which steps the check skips when no identity is present. What replaces them is either verifiable in this repository or phrased as guidance rather than fact: any non-2xx response is raised as an ApiException by the client in this package, and a rejected call means the IBAN could not be checked rather than that it was rejected. The wording rule for NotMatched stays - it is an instruction to the consumer, not a claim about the service. The test comment loses its assertion about how the service distinguishes two statuses and states what the assertion actually pins: unlike list(), this call does not opt out of the token.
…the test bite The previous wording said any non-2xx response is raised. That holds for the core client, but not for the hook: on HTTP 401 the shared api hook retries once with the current token when the stored token changed while the call was in flight, and a successful retry resolves normally. Both places now say what is true in either package - a failure never arrives as a status, and a rejection is an ApiException - and keep the instruction to read that as "could not check". The endpoint test asserted the sent IBAN against the same constant it passed in, so an implementation that always sent one hardcoded IBAN passed. It now runs over two distinct example IBANs, which fails that implementation on the second case. The second test's title claimed what the service can answer. It now states what the assertion pins: unlike list(), this call does not set token: false.
|
Three review passes were needed before both independent reviews came back clean. What changed along the way: Documentation that a consumer cannot verify. This package ships its docs as A failure statement that was true for only one of the two packages. The replacement wording said any non-2xx response is raised. That holds for the core client, but the React hook retries once on HTTP 401 when the stored token changed while the call was in flight, and a successful retry resolves normally. Both places now say what is true in either package: an HTTP failure never arrives as a status, and a rejection is an A test that did not prove what its name claimed. The endpoint test asserted the sent IBAN against the same constant it passed in, so an implementation that always sent one hardcoded IBAN passed it. It now runs over two distinct example IBANs and fails such an implementation on the second row. Its sibling test also lost a title that described what the service can answer, in favour of what the assertion actually pins. Two reported points were rejected with reasons rather than applied: retrying anonymously on HTTP 401, which cannot occur here because the endpoint's guard never rejects an unusable token but treats the request as anonymous; and reducing the The PR description was corrected twice as well: it still carried the ordering note from before the API side landed, and it named the wrong version for the precedent it cites. |
1 participant
What
Adds
PUT /v1/bank/receiveIbanto@dfx.swiss/coreand@dfx.swiss/react: theBankUrl.receiveIbanroute, theReceiveIbanStatusenum (DfxIban,NotMatched,InvalidIban,LoginRequired), theReceiveIbanCheckresponse type,useBank().checkReceiveIban(iban)in the React hook, andDfxApiClient.bank.checkReceiveIban(iban)in the API client.getBanks/bank.list()and the value ofBankUrl.getare unchanged.Why
The support form's "missing transfer" reason requires picking a receiver IBAN from a dropdown fed by
GET /bank, which lists only the collective accounts. Customers with a personal deposit IBAN cannot find their actual receiver there and are forced to select an account that is not theirs. The dropdown is being replaced by a free-text field; this endpoint lets the entered IBAN be validated server-side so the customer gets a meaningful response.Ordering — the API side has landed
DFXswiss/api#4391 was merged on 27 July. The wire contract in this PR was re-checked against
the merged state of
developin the api repo and matches exactly: path, method, request body,response shape and all four enum values.
Merging here publishes to npm, and that release is what unblocks the downstream consumer,
DFXswiss/services#1191 — its CI installs from the lockfile and therefore cannot go green until
a published version carries
checkReceiveIban.Consuming this
ReceiveIbanStatusand bothcheckReceiveIbanmethods carry JSDoc that reaches the published.d.ts, so the details show up in the IDE. In short: there is no abort signal, so a consumer driving this from a text field must debounce and discard responses that do not belong to the most recent request. An HTTP failure never arrives as aReceiveIbanStatus; the call rejects with anApiException, and that means the IBAN could not be checked, not that it was rejected.Also note that
NotMatchedis deliberately not a statement that DFX does not own the IBAN; the JSDoc on the enum is the source of truth for the user-facing wording.Version pins for downstream
@dfx.swiss/coreis not a direct dependency ofservices; it arrives transitively through@dfx.swiss/react, the only package here that depends on it. Bumpingreacttherefore pulls the new core version along automatically and yields a single copy — there is no separate core pin to raise.Worth knowing for anyone who does pin core directly (nobody today): core is on
0.x, where lerna'sfeatbump raises the minor and a^0.4.1range does not accept0.5.0, so such a pin would have to be raised explicitly.One reviewer-facing nuance:
checkReceiveIbanis a required member ofBankInterface, so anyone who constructs the return type ofuseBank()by hand — a test mock, for instance — has to add the new method. This follows the established convention here —045ab68(#181) added three required members to a hook interface while@dfx.swiss/reactstood at1.4.0-beta.1, and the next release went to1.4.0-beta.2, with no major bump — so it is intentionally not released as a breaking change, but it is a real source-compatibility detail and should be an explicit decision rather than an oversight.Testing
packages/core/src/__tests__/bank-api.test.tscovers the call shape ofcheckReceiveIbanover two distinct example IBANs — so an implementation that sent a hardcoded one would fail the second row — and pins that notoken: falseis passed — unlikelist(), this endpoint must send the bearer token when one exists, otherwise the API can never distinguish an authenticatedNotMatchedfromLoginRequired. The same file keepslist()asserted withtoken: false, so the deliberate asymmetry between the two methods is recorded in one place.The status documentation is JSDoc rather than line comments, because
tscstrips line comments from the declaration output and a consumer only ever seesdist/. Verified on the built artifact:packages/core/dist/definitions/bank.d.tscarries all four blocks.Run locally from the repo root, the same chain as CI (
.github/workflows/pr.yaml):lerna run lintlerna run format:checklerna run buildlerna run test