refactor: codify paykit issuer contract - #1216
Conversation
This comment has been minimized.
This comment has been minimized.
a6b3b52 to
2998631
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Ran the new suite in a throwaway worktree — PaykitIssuerInteropTest passes, so the fixtures pin real current behaviour rather than aspirational behaviour. Behaviour-change audit is clean: MethodId.fromRawValue(value, network) and parseEndpoint(..., network) are byte-identical to the old rawValue matching at every existing call site, toPaykitPaymentRequest only went private→internal, and the three "btc" literals → BITCOIN_ASSET are the only three in app/src/main. Every endpoint fixture is load-bearing (accepted is asserted against endpoint != null, not just the expected value), and regtest-filters-and-deduplicates would fail if either the filter or the distinct() were dropped.
One non-blocking question inline.
2998631 to
41ac764
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at 41ac764. No HIGH/MEDIUM, nothing to file. Reviewed as a funds-and-trust-boundary change rather than a docs one — the files add an issuer-payload parser, and the iOS twin under the same title turned out the same way.
The two commits since my last pass (38f477a, 41ac764) touch only the test, the fixture and the doc — no production source — and they're correct rather than merely present. The new request fixtures cover every documented identifier test builds 4 networks x 4 scripts = 16 expected on-chain keys and uses expectedIdentifiers.singleOrNull() plus a btc-<network>- prefix check, which correctly excludes the multi-identifier regtest-filters-and-deduplicates case and the Lightning cases; the Bolt11/LNURL assertions are exact-list matches, so dropping any of the 16 on-chain or 8 Lightning accepted fixtures fails the suite. The 18,446,744,073,709,551 sat bound in the doc matches ULong.MAX_VALUE / 1000uL at PaykitPaymentRequestRepo.kt:630.
Cross-checked all five properties I confirmed on the iOS twin (#719) — all present here:
- Network gate at parse — same point, same shape.
parseEndpoint(methodId, endpointData, network = Env.network)→MethodId.fromRawValue(methodId, network), sobtc-bitcoin-p2wpkhon regtest is dropped before the payload is decoded, andisPayablere-checks the decoded address network afterwards. Not a behaviour change from master —fromRawValuealready compared againstrawValueForNetwork(Env.network); this only parameterises it, and every call site still passes the default. - Strict non-string min/max — genuinely Android behaviour, so iOS was matched to something real rather than to a fiction.
Json(appJson) { isLenient = false }overridesJsonModule'sisLenient = true, so kotlinx requires quotes for aString?and1000/truethrow into therunCatching→null.ignoreUnknownKeysis inherited, and explicit"min": nullis still accepted. - Display == execute — untouched. The only send-confirm change is the root
testTag; shown and paid amounts both come from_sendUiState.value.amount, withacceptsPaymentAmount/acceptsLightningInvoicere-checked at confirm time andAppViewModel.kt:4324-4329refusing to proceed if the request context vanished. - No issuer string reaches UI or logs —
PaykitIssuerInterop.kthas noLoggercalls at all; every request-path log passes counterparties throughPubkyPublicKeyFormat.redacted(...)and errors throughredactedContext(). The only issuer text reaching UI is the trimmednote, same as iOS. - min/max never read for a payment decision — confirmed, see the resolved thread.
Also clean: published endpoints and the StoredPaymentEntry cache stay byte-identical to the shipped version (serializeEndpointPayload still emits {"value":"…"} with encodeDefaults = false), so there's no migration concern. The runCatching at PaykitIssuerInterop.kt:28 wraps a synchronous decode, which is the correct choice there. Gating verified — isUiEnabled(settingsStore.isPaykitEnabled) defaults false; the ordinary non-Paykit send path gains only a "SendConfirm" tag, and that string had no prior reference anywhere in app/ or journeys/, so no collision.
41ac764 to
47d1895
Compare
47d1895 to
cff08e9
Compare
cff08e9 to
1867ae3
Compare
jvsena42
left a comment
There was a problem hiding this comment.
No findings. Clean at the HIGH/MEDIUM bar.
I reviewed this as a trust-boundary and funds-path change despite the docs: label. The diff adds PaykitIssuerInterop.kt — a parser for counterparty-supplied endpoint payloads that feeds the pay path — and touches PaykitPaymentRequestRepo.kt, PublicPaykitRepo.kt, PrivatePaykitRepo.kt, PaykitSdkService.kt and SendConfirmScreen.kt. Production behaviour turns out byte-equivalent to master at every call site, so the actual risk is low, but anyone skimming the queue by commit type would file this under docs and skip it. Same shape on the iOS twin under the same title. Flagging the labelling, not the commit message.
Gating: PaykitFeatureFlags.isUiEnabled(settingsStore.isPaykitEnabled), default false, toggled only from Dev Settings; the home bell additionally requires isPaykitEnabled && pendingPaymentRequests.isNotEmpty(). Nothing in this PR changes gating. Dev/QA-facing today.
Delta since my last pass: the branch was rebased (all four original commits re-dated, contents identical) and 731fb6b84 "fix: align payment request journey" landed. That commit is test tags only — PaymentRequestRow${id} -> PaymentRequestRow-${id}, PaymentRequestReject -> PaymentRequestDismiss-, PaymentRequestPay -> PaymentRequestPay- — plus matching androidTest assertions and the journey README/XML. No parser, repo, ViewModel or amount code moved since I last passed it. My three earlier threads are resolved and were not reverted.
Checked and clean:
- Parser fails closed on every hostile shape. Strict decoder (
isLenient = false) inheritingignoreUnknownKeys = true. Non-object top level, raw string, malformed JSON, missing/null/numericvalue, numericmin, booleanmaxall throw intorunCatching-> null; empty/whitespacevaluerejected explicitly. There is no default forvalue, so no fail-open on the recipient. Nothing in the payload is logged — the file has noLoggercall — and no issuer string reaches a path, URL or network call. An unbounded-lengthvalueis accepted by shape but must then decode as a real address/invoice/LNURL inisPayable, which also enforces invoice expiry and network, so it can't be smuggled into the UI. - Identifier filter. Exact-string match against
rawValueForNetwork(network)thendistinct()— first-occurrence order preserved, wrong-network/uppercase/unknown dropped. Matches the doc. - Amount is one value from parse to pay.
toSats()is regex-gated,BigDecimal.movePointRight(8).toBigIntegerExact()rejects more than 8 significant fractional digits,> 0uLrejects zero, overflow throws, then theULong.MAX_VALUE / 1000uLcap. NoULongarithmetic is performed on the issuer value (comparison only), so theUSatwrapping rule isn't implicated here. Display and execution both read_sendUiState.value.amount, and the confirm-timeacceptsPaymentAmount/acceptsLightningInvoicere-checks are unchanged from master. - The doc matches what the code enforces. Every claim in
docs/paykit-issuer-interoperability.mdmaps to a specific guard — asset, the18,446,744,073,709,551bound, the eight-fractional-digit rule, expiry, identifier filtering, post-shape usability. It doesn't promise validation the code skips, which is the failure mode that would matter for an integrator. - Journey matches real behaviour. Every identifier the new journey names exists at head and is reachable on the described path. The "automatic confirmation -> back -> bell -> Pay" sequence is consistent with the state machine:
onSheetVisiblemarks presented and nullsrequestedPaymentRequestId, so the back-path defer condition is false and the request isn't auto-re-presented, but stays inpendingRequestsfor the bell. "Saved as a contact" was needed because the recipient row otherwise falls back toPubkyProfile.placeholder(publicKey). - Behaviour-change audit.
fromRawValue(value, network = Env.network)andparseEndpoint(..., network = Env.network)default to the same value master compared against; every production call site passes the default.toPaykitPaymentRequestonly widenedprivate->internal. The three"btc"->BITCOIN_ASSETsubstitutions are the only occurrences.serializePayloadstill emits{"value":"..."}withencodeDefaults = false, so published endpoints and cached entries are byte-identical to master. The only send-path change for non-Paykit users is the"SendConfirm"root tag.
Cross-repo parity with synonymdev/bitkit-ios#719: the shared fixture is byte-identical on both heads (sha256 35f671bb…7a2a5e), and both suites evaluate it against production parsers rather than test doubles. Network gate at parse, strict non-string bounds rejection, non-object/raw-string/missing-value rejection, unknown-key tolerance, identifier filtering and the request gate are all present and same-shaped. The amount parsers are differently shaped — iOS hand-rolls a digit parser, Android uses regex + BigDecimal — but agree on every input traced, including .5, 5., 00.001, nine decimals, 1e3, ., "" and a 30-digit whole part.
One immaterial divergence, recorded rather than filed: iOS trims the payload with .whitespacesAndNewlines, which covers Unicode spaces (U+00A0, U+2028, VT/FF); Android relies on kotlinx, which skips only ASCII whitespace. So a payload led by U+00A0 parses on iOS and is rejected on Android. It can't alter an amount or a recipient and no issuer emits it — worth a fixture case pinning ASCII-only whitespace if you want the two sides provably identical.
Cross-platform journey run — Android sideRan The journey nominally needs an external fixture issuer, and no such harness exists in either repo. It turns out one isn't needed: Bitkit can issue to itself (Contacts → contact → Request
The documented divergences held exactly as written: the row testTag is the bare Two things worth a look1. The receiver path is I deliberately did not change the journey: 2. adb shell uiautomator dump /sdcard/ui.xml >/dev/null
adb shell cat /sdcard/ui.xml | grep -oE 'resource-id="[^"]*"' | sort -u
# → PaymentRequestConfirm, SendSheet, PaymentRequestRow-b78df292-…iOS has the same trap with Notes, not problems
|
jvsena42
left a comment
There was a problem hiding this comment.
Verified the new receiver-path claims against the code, and they're accurate: journeys/payment-requests/README.md:9 "use bitkit/wallet when another Bitkit instance is the issuer" matches PaykitSdkService.kt:1002 (defaultConfig(PaykitReceiverPaths.WALLET)) and PaykitPaymentRequestRepo.kt:729, which picks PaykitReceiverPaths.ordered.firstOrNull { it in linked && it in capable } with ordered = [WALLET, SERVER] — so a Bitkit peer negotiates bitkit/wallet, while bitkit/server stays in supported for the third-party fixture. The android layout container-tag note matches what I saw on the live run.
Putting it in the suite README rather than a corpus-level journeys/README.md is right given the tree — this repo has only per-suite READMEs.
One parity note inline.
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed as a funds + trust-boundary change, not a refactor: the diff adds PaykitIssuerInterop.kt — a parser for counterparty-supplied issuer payloads that feeds the pay path — and touches SendConfirmScreen.kt. Flagging that because reviewers skim by type, and a payload parser under a refactor: heading is what gets waved through. It held up.
Production hunks are byte-identical to the ones I reviewed last pass; the three commits since (9e41e849, dc41c3ee, e47542cd) touch only the journey files and the xml. The chore: sync master merge brought in one master commit (b7cd6e9e docs: add paykit screens to screens map) and zero changes under app/src/main on any Paykit or send path — so no new caller or reader of the parser entered through it.
Trust boundary, field by field on PaykitEndpointPayload:
value— required, trimmed, empty rejected (PaykitIssuerInterop.kt:30-32). Consumed only byisPayable(PublicPaykitRepo.kt:395-410: bolt11 unexpired + network match; LNURL must decode asScanner.LnurlPay; on-chain must decode and network-match), then displayed. Never used as a path, a URL, or a log string.min/max— still write-only; no readers anywhere in the three repos.- Unknown keys tolerated, but
isLenient = falsemeans an unquoted or numeric value where a string is expected throws →runCatching→null→ endpoint dropped. Non-object top level, raw string, array and malformed JSON all →null. Fails closed on an unrecognised identifier:MethodId.fromRawValuereturnsnullbefore the payload is decoded at all (PublicPaykitRepo.kt:104). - No host allowlist, and none is needed here — the counterparty is a user-saved, user-linked contact and
valueis never fetched.
Ungated path is untouched. SendConfirmScreen.kt:222-226 is the only change there, a root testTag; the sole caller at :174 passes no modifier, so the ordinary send path gains exactly the string "SendConfirm", which has no other reference in app/ or journeys/. Nothing on the release-reachable path changes behaviour.
Amount pinning: one regex-gated toPaykitSats() value with the ULong.MAX_VALUE / 1000uL cap (PaykitPaymentRequestRepo.kt:991-993), read once into amountSats; confirm and pay both read _sendUiState.value.amount. No re-resolution of the payee after display — presentIncomingPaymentRequestOrStop (AppViewModel.kt:996-1020) opens with publicKey = request.counterparty from the same request object, and updateRequest (:766-767) refuses at pay time if the request is no longer pending or has expired.
Docs vs code: every guarantee in docs/paykit-issuer-interoperability.md maps to an enforced predicate. One imprecision I am explicitly not filing: L14 says "lifecycle state is proposed" while the gate at :982-986 also admits ACCEPTED — but ACCEPTED is written only locally at :453 after the user taps Pay, an issuer cannot produce it, and iOS has the identical .proposed || .accepted gate. Nobody acting on the doc gets anything wrong.
Recorded, not filed: a valid JSON object with a huge unknown field parses, and rawPayload is persisted verbatim into contactState.remoteEndpoints (PrivatePaykitRepo.kt:1285-1289) before isPayable, with no size guard. This is pre-existing (master used the identical Json config; this PR only moved it), gated, and requires a contact the user explicitly linked. I could not verify whether paykit-rs or the homeserver caps the published file — that is the thing that would settle it.
No HIGH, no MEDIUM. One LOW as a reply on the existing receiver-path thread rather than a new one.
Gating unchanged: PaykitFeatureFlags.isUiEnabled(settingsStore.isPaykitEnabled) defaults false, Dev Settings only — dev/QA-facing today.
jvsena42
left a comment
There was a problem hiding this comment.
Manually reviewed and cross-tested with synonymdev/bitkit-ios#719
jvsena42
left a comment
There was a problem hiding this comment.
Parity restored at 134c9eca. I byte-compared the two <description> blocks:
diff <(git show <ios#719 head>:journeys/payment-requests/issuer-interoperability.xml | sed -n '3p') \
<(git show <this head>:journeys/payment-requests/issuer-interoperability.xml | sed -n '3p')
→ identical
You moved the receiver-path caveat ahead of the rejected-shapes sentence and switched to "linked server fixture", which now matches ios#719 exactly. That was the property the thread was about, so it's closed.
Nothing else in the commit; production hunks unchanged. Clean from my side.
Closes #1208
This PR:
4d077e4c3e0b354adeee704b8752e6b567c48d64.Description
btcand identifier casing, preserves issuer order, removes duplicates, and drops unsupported or wrong-network identifiers.value, retaining optional string bounds and rejecting non-string bounds or wrong-network identifiers.35f671bbc4c437dbd28cacb33ca09f8ba9d91c6d3a422a247abe0969ad7a2a5e.bitkit/serverissuers from Bitkit-to-Bitkitbitkit/walletissuers and documents raw UI Automator inspection for container test tags.Preview
N/A — no user-visible changes.
QA Notes
Manual Tests
bitkit/server→ publish a currentbtc-regtest-p2wpkhJSON endpoint → send the canonical 100,000-sat one-time request: Payment Request Confirm opens automatically with 100,000 sats.Automated Checks
PaykitIssuerInteropTest.kt: 3/3 tests passed on pre-merge head731fb6bagainst the byte-identical shared fixture, covering accepted and rejected requests and endpoint payloads across all four networks and four documented on-chain scripts.PaymentRequestsScreenTest.kt: 5/5 connected tests passed on pre-merge head731fb6busing an isolated Pixel 9a API 36 AVD, including the canonical Row, Pay, and Dismiss identifiers.PublicPaykitRepoTest.ktandPaykitPaymentRequestRepoTest.ktinto the canonical fixture suite so one source exercises the real parser and request mapper.731fb6b.mastere2fa21fwas merged with the Paykit subscription architecture preserved; post-merge checks were not run at user direction.