docs: 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.
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.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 against 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 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.