feat(core,react): add the unauthenticated invoice payment call - #206
feat(core,react): add the unauthenticated invoice payment call#206joshuakrueger-dfx wants to merge 5 commits into
Conversation
|
@marassteiner please review This is a PR, not an issue: 8 files against its base branch, head |
|
Sorry — I've reached my hourly job limit and can't take this on right now. I'll pick it up automatically once the limit resets. |
marassteiner
left a comment
There was a problem hiding this comment.
Review — approve
Recommendation: approve. No merge-blockers for the change set itself. PR remains correctly draft and stacked on #201 until that lands and this is retargeted to develop.
Gate A — AI diff review (two independent lanes)
Lane A — Konformität/Qualität
- Source-only diff (8 files, +146): core client method, query type, exports, tests, thin React hook re-export — matches CONTRIBUTING (no version/changelog/lockfile bumps; conventional
feat(core,react)commit; tests for core). - Dual-layer placement matches existing
getRecipient/ neighbour patterns (hook does not go through the core client). - Hygiene clean (no
console.log/ TODOs / debug). - Stacking on #201 instead of
developis documented and intentional for a stacked PR; not a defect in the code.
Lane B — Logik/Korrektheit
GET+PaymentLinksUrl.payment+token: falseis correct for the unauthenticated invoice pay endpoint.Utils.buildQueryencoding (spaces,/,Date, enum) covered by tests; optional omission covered; bothPaymentLinkPayResponsebranches (quoted + terminal) covered.- Core and React request shapes match for the happy path.
Gate B — Local software run
On this review host (Node 24.19.0; CI pins Node 16):
| Command | Result |
|---|---|
npx lerna run lint |
exit 0 (8 pre-existing warnings in bip322-multisig only; core/react clean) |
npx lerna run format:check |
exit 0 |
npx lerna run build |
exit 0 (all 4 packages) |
npx lerna run test |
exit 0 — core 7 suites / 82 tests, bip322 4 / 46 |
DONE review PR 206 LOCAL_RUN_OK
Note: no CI checks appear on this PR because pr.yaml only triggers against main/develop; that is expected for the stacked base, as the PR body states. Local gates green on Node 24; Node-16 parity remains for when this is retargeted.
Follow-up findings (not merge-blockers)
-
Query type does not encode either-or identity rules —
PaymentLinkInvoicePaymentQueryrequires onlyamount;routeId|routeandexternalId|messagestay optional at the type level though the API validators require them. Documented intentional trade-off in the PR body; runtime API still rejects incomplete queries.
→ #221 -
React
useApi401 retry can attach a session token aftertoken: false— first request is correctly unauthenticated; on 401 with an existing session,api.hook.tsretries withgetAuthToken(). Pre-existinguseApibehaviour; this PR is the firstusePaymentRoutescaller withtoken: false. Fix belongs inapi.hook.ts, not in this method.
→ #222
Open process items (from PR body — not code defects)
- Merge #201 first, then retarget this PR to
developand mark ready so CI runs. - Publish
@dfx.swiss/react/ consume in DFXswiss/app#1274 after release.
No merge from this review.
|
@marassteiner please review This is a PR, not an issue: 8 files against |
|
Sorry — I've reached my hourly job limit and can't take this on right now. I'll pick it up automatically once the limit resets. |
|
EN: Working on this now — job |
marassteiner
left a comment
There was a problem hiding this comment.
Review — approve
Recommendation: approve. No merge-blockers in this change set. The PR correctly remains draft and stacked on #201 until that lands and this is retargeted to develop.
Gate A — AI diff review (two independent lanes)
Lane A — Konformität/Qualität
- 0 findings. Full pass confirmed.
- Source-only (+146, 8 files): query type, core client method, public exports, core tests, thin React hook — matches CONTRIBUTING (no version/CHANGELOG/lockfile bumps; conventional
feat(core,react)commit; tests extended in@dfx.swiss/core). - End-to-end wiring:
PaymentLinkInvoicePaymentQuery→PaymentLinksApi.getInvoicePayment→ core/react exports →usePaymentRoutes.getInvoicePayment(interface +useCallback+useMemodeps). - Reuses
PaymentLinkPayResponse/hasPaymentQuotefrom #201; long-form query only; pattern matches existing public endpoints (token: false,Utils.buildQuery).
Lane B — Logik/Korrektheit
- 1 non-blocking finding (already tracked).
- Correct:
GET+PaymentLinksUrl.payment+token: falseon both core and react;Utils.buildQueryencoding (ISO dates, omitted optionals, enum string) covered by tests; both quoted and terminal response branches asserted; no semantic merge risk (pure additive).
| Severity | Finding | Evidence | Disposition |
|---|---|---|---|
| medium (follow-up) | PaymentLinkInvoicePaymentQuery documents either-or route/payment identity, but only amount is type-required — { amount: '5' } typechecks and fails only at the API |
packages/core/src/definitions/route.ts:382-396 |
Intentional per PR body; same looseness pattern as PaymentLinkPaymentQuery. Tracked in #221 |
Pre-existing / out-of-diff note (not re-raised as a new defect): React useApi 401-retry can re-attach a session token when the original call set token: false (packages/react/src/hooks/api.hook.ts:89-107). #206 is the first token: false consumer on usePaymentRoutes. Primary fix is in api.hook.ts, not this method. Tracked in #222.
Gate B — Local software run
Repository is a Lerna monorepo (SDK packages), not a long-running app. Documented path from CONTRIBUTING.md was executed on this checkout (6a5abfc):
| Command | Result |
|---|---|
npm ci |
exit 0 |
npx lerna run lint |
exit 0 (8 pre-existing warnings in bip322-multisig only; core/react clean) |
npx lerna run format:check |
exit 0 (all four packages) |
npx lerna run build |
exit 0 (all four packages) |
npx lerna run test |
exit 0 — core 7 suites / 82 tests, bip322-multisig 4 / 46 |
Host Node: v24.19.0 (CI pins Node 16; PR body already notes that CI does not run on stacked base branches — pr.yaml only targets main/develop). This is environmental for the missing GH checks, not a defect in the diff.
Follow-up issues (linked)
- #221 — tighten either-or identity rules on
PaymentLinkInvoicePaymentQuery - #222 —
useApimust not attach session token on 401 retry whentoken: false
Merge checklist (outside this review)
- Merge #201 first (base +
PaymentLinkPayResponse) - Retarget this PR to
developand mark ready so CI runs - Publish
@dfx.swiss/react/@dfx.swiss/core, then consume in DFXswiss/app#1274
No merge from this review.
34839fd to
ebf649b
Compare
`GET paymentLink/payment` is the call a payer makes after scanning an invoice QR code. It was the one payment-link endpoint with no SDK method, so its consumer built the URL by hand and fired it with a raw fetch — verb, query shape and response type living in the consuming repository, where they go stale without anything failing at build time. The URL constant was already here and already used for POST and DELETE; only the GET was missing. The response type needs no new modelling either: the endpoint returns the quoted pay request or the terminal error payload, which is exactly the PaymentLinkPayResponse union from the pay-flow contract this branch builds on. The query carries the long-form parameters only. The short forms the API also accepts (r, e, m, l, n, a, c, d, s, w) are a url-length device for printed codes and the handler maps them onto the long forms before anything else, so putting them in a typed contract would offer two names for one field with no gain. Tests cover the built query, omitted optionals staying out of it entirely, the verb and the suppressed session token, and both branches of the response.
The type says amount is required and everything else optional, and until now nothing held that: the mutation from required to optional stayed green because Jest does not see types. ts-jest does type-check, so three calls now sit under @ts-expect-error. Relaxing amount to optional, widening it to string | number, or adding an index signature each makes a directive unused (TS2578) and fails the suite - measured 1, 1 and 2 suites red.
f6be1b0 to
e40964a
Compare
|
EN: DE: DetailsWhy this and not
|
| Mutation | Result |
|---|---|
amount: string → amount?: string |
Test Suites: 1 failed, 9 passed / Tests: 119 (from 10 passed / 120), TS2578 |
amount: string → amount: string | number |
1 suite red, TS2578 |
[key: string]: unknown added to the interface |
2 suites red, TS2578 — the second is collateral damage, so this is the weakest of the three |
Worth noting how that failure reads: a type error kills the whole suite, so its tests are not counted. Tests: 119 passed beside Test Suites: 1 failed is the red state — looking only at the test line would show 119 green and hide it.
Gates on this head
lerna run lint, format:check, build and test all exit 0 locally on Node 20 — core 10 suites / 120 tests, react 2 / 41, bip322 4 / 46 (1 todo). CI on Node 16 is green on this head with the same numbers, and the new suite is in its PASS list.
What is still open
Unchanged: this waits on #201, then the publish, then the consumer pull request in DFXswiss/services. The two either-or rules (routeId | route, externalId | message) remain carried by the doc comment — expressing them means a union of four admissible shapes, which is a readability trade-off rather than an oversight. Say the word and it can be added the same way.
|
EN: DE: DetailsThe five findings and why none of them changed the code1 — 2 — 3 — 4 — 5 — see 1. What did change since the approvalsBoth approvals sit on Gates on
|
|
@marassteiner please review This is a PR, not an issue: 10 files against Your two approvals sit on Please note the merge order: #201 has to land before this one — this branch returns One point is deliberately left open and flagged in the description rather than fixed: |
|
EN: Working on this now — job |
|
EN: The required independent review of head DetailsHead under review: The mandatory two-lane review did not run: both lanes aborted on a usage limit before reading the diff. No substitute review was used. Local software run on this checkout (Node v24.19.0; CI pins Node 16) completed with exit 0:
CI on this head is already green ( This is not a code verdict. Merge order remains as stated in the pull request body: #201 has to land first. |
|
@marassteiner please review This is a PR, not an issue: 10 files against Your last completed review was on |
Base is now
develop, sopr.yamlcan run. Rebased onto currentdevelopagain on 19 Aug (it was four commits behind, which is why a local run showed 9 suites where CI — testing the merge — showed 10).Open items — what this pull request is waiting on
PaymentLinkPayResponse, the return type of the new method; merging this first would put a method ondevelopwhose type is not there. It is also why this review currently is the whole stack.develop.ebf649b(13 Aug) and again on the current head after the rebase — core 10 suites / 120 tests, react 2 / 41, bip322 4 / 46.@dfx.swiss/react.fetchininvoice.screen.tsxfor this method.Not symptom-driven: No incident and no error report. This exists because the review on
DFXswiss/services#1274(finding 8) named a rule the consuming repository cannot satisfy at the call site: API access goes through the SDK, and this endpoint has no SDK method.Scale:
GET paymentLink/paymentis the call every OpenCryptoPay payer triggers after scanning an invoice QR code — the merchant that prompted #1274 bills more than 100 invoices a month, each one sending a payer through it. Today its verb, query shape and response type live ininvoice.screen.tsx, where they can drift from the API without anything failing at build time.Smaller fix considered: Leaving the hand-built
fetchin place and only typing its response insideDFXswiss/services— no change in this repository at all. Insufficient because it puts endpoint knowledge in the consumer, which is the specific thing the guideline forbids, and it would have to be repeated in every further consumer; the whole point of #198 is that the contract lives once, here.Why
GET paymentLink/paymentis the call a payer makes after scanning an invoice QR code. It is the one payment-link endpoint with no SDK method, so its consumer builds the URL by hand and fires it with a rawfetch—DFXswiss/services,src/screens/invoice.screen.tsx:36,127. Verb, query shape and response type therefore live in the consuming repository, where they go stale without anything failing at build time.This came out of the review on DFXswiss/app#1274 (finding 8). That pull request cannot fix it at the call site without moving endpoint knowledge into the wrong repository, so the fix is here.
What is already there
Little of this is new:
PaymentLinksUrl.paymentalready exists and is already used — forPOST(createPayment) andDELETE(cancelPayment). Only theGETon that same URL was missing, so no new constant.PaymentLinkPayResponseunion feat(core,react): add the payment link pay flow contract #201 introduces. Field-by-field against the API (payment-link.controller.ts:215,dto/payment-link.dto.ts:55,69,shared/dto/error.dto.ts):PaymentLinkRequestBasematchesPaymentLinkRequestDto,PaymentLinkPayRequestmatchesPaymentLinkPayRequestDto, andPaymentLinkPayTerminalmatchesPaymentLinkPaymentErrorResponseDto.@UseGuards, so it is called withtoken: false— the patterngetStandardsin feat(core,react): add the payment link pay flow contract #201 already uses.Shape of the query
Long-form parameters only. The API also accepts short forms (
r,e,m,l,n,a,c,d,s,w), but they are a url-length device for printed codes and the handler maps them onto the long forms before anything else runs. Putting both in a typed contract would offer two names for one field and no caller would gain from it.Which fields are required was read off the validators rather than guessed: a route identity (
routeIdorroute) and a payment identity (externalIdormessage) are each required through mutually alternative@ValidateIfchains,amountis required outright, and the rest carry@IsOptional().Of those three rules, the type carries only one.
amountis required inPaymentLinkInvoicePaymentQuery;routeId,route,externalIdandmessageare all optional there, sogetInvoicePayment({ amount: '5' })compiles and is rejected by the API at runtime. The two either-or rules are expressible — a union of the four admissible shapes would do it — so this is a choice rather than a limit of the language: four overlapping shapes read badly at the call site and in the editor's hover, and the rules are documented on the interface instead. Worth knowing when reading the section above, which describes what the endpoint enforces, not what the compiler does.Placement
Both layers, because that is what this repository already does:
getRecipientlives on the core client andgetPaymentRecipientin the react hook, independently — the hook does not use the core client anywhere. So the query type, the client method and the tests go into@dfx.swiss/core, and the thin hook method sits next to its neighbour in@dfx.swiss/react. Purely additive; no existing signature or behaviour changes.Verification
All four commands from
CONTRIBUTING.md, run with the repository's own binaries on Node 20 (Studio). CI pins Node 16.lerna run lintcore/reactclean; nine pre-existing warnings inbip322-multisigonly)lerna run format:checklerna run buildlerna run testCoverage on the touched runtime files, collected on that same run:
PaymentLinksApi.ts,definitions/route.tsandpayment-routes.hook.tsare each 100 % statements / branches / functions / lines.Mutations, each asserted to hit exactly one occurrence, each with the diff checked and the working tree proven clean afterwards:
token: falseremovedgetInvoicePaymenttests failGETbecomesPOSTgetRecipientURL becomeshistoryamountloosened from required to optionalThat last row used to read stays green, and it was the honest admission that the only rule the type carries was held by review rather than by a test. It is closed now, without a new dependency and without touching the configuration:
packages/coreruns Jest through ts-jest, which type-checks, sosrc/__tests__/invoice-payment-query-type.test.tsputs three calls that must not compile under@ts-expect-error. Relax the rule and the expected error disappears, the directive becomes unused, and TypeScript reports TS2578 — the suite fails.Measured on head
e40964af, Node 20, each mutation asserted to hit exactly one occurrence and the file verified restored afterwards:amount: string→amount?: string10 passed / 120 testsbecomes1 failed + 9 passed, 119 testsamount: string→amount: string | number[key: string]: unknownaddedNote the shape of that failure: a type error kills the suite, so its tests are not counted at all.
Tests: 101 passednext toTest Suites: 1 failedis the red state, not a partial one — reading only the test line would show 101 green and hide it.The two either-or rules (
routeId | route,externalId | message) are still carried by the doc comment alone; expressing them means a union of four admissible shapes, which is a readability trade-off rather than an oversight.Not verified
The four commands above and the type mutations were run on Node 20; CI pins Node 16. TypeScript comes from the repository either way, so the directives should behave the same — but the Node 16 run is the one that counts, and it is the check on this head.
An earlier version of this section said the first retarget started no workflow because
pull_requestdoes not run onedited. That reason was wrong:editedis in the workflow's trigger list. It did not run because the base at that moment wasfeat/payment-link-pay-contract, andpr.yamlfilters onmainanddevelop.Nothing holds the route-identity and payment-identity rules but the doc comment on the interface — not the compiler and not a test (see Shape of the query).
amountis now held by the compiler through the type test above.No live call against the API; the tests use a mocked HTTP client. No consumption in
DFXswiss/servicesyet — that follows the release, as the rule requires. The react hook is exercised through a Jest harness that stubsuseCallback/useMemoas identity; it does not render with Testing Library.The two approvals on this pull request were given on
6a5abfc6. Since then came the test commit, the type test and two rebases — at the current head this is formally unreviewed, and a re-review is due before it goes anywhere near a merge.Final pass (e40964a):
Coherent: Every file serves the one title — the query type, the client method, the thin react method beside its neighbour, their tests, and the type test that holds the one rule the type carries. Nothing here belongs to another topic; what looks larger than that is #201, whose two commits this branch still carries because it is not merged.
Nothing extra: No new dependency, no configuration change, no new URL constant (
PaymentLinksUrl.paymentalready existed forPOSTandDELETE), and the short-form query parameters (r,e,m, …) are deliberately left out — they are a url-length device for printed codes and the handler maps them onto the long forms before anything else runs. Deliberately not built: the two either-or rules as a union of four admissible shapes (readability trade-off, documented under Shape of the query), and any consumption inDFXswiss/services— that follows the release and gets its own pull request, as #1239 to #1243 already do.Sources closed:
DFXswiss/services#1274, review finding 8 — the SDK method exists here; the swap at the call site follows the publish and is the last row of the table above. #198 (umbrella), step 3 — this is the invoice-payment slice on top of #201. Commits — each says what its diff does. Reviews — marassteiner approved twice, both on6a5abfc6, so those approvals do not cover this head; no other review exists. Open — #201 must land first, then the release, then the consumer pull request; and this pass expires with the next commit.