Skip to content

refactor: codify paykit issuer contract - #1216

Merged
jvsena42 merged 10 commits into
masterfrom
codex/1208-paykit-interop
Sep 10, 2026
Merged

refactor: codify paykit issuer contract#1216
jvsena42 merged 10 commits into
masterfrom
codex/1208-paykit-interop

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Closes #1208

This PR:

  1. Codifies the Android Paykit issuer contract aligned with bitkit-ios#719 at 4d077e4c3e0b354adeee704b8752e6b567c48d64.
  2. Validates the exact cross-platform request and endpoint fixtures through Android's production filtering and parsing paths.
  3. Adds canonical automation identifiers and a regtest journey for the incoming Payment Request flow.

Description

  • Keeps Lightning identifiers chain-independent and covers network-specific P2TR, P2WPKH, P2SH, and P2PKH identifiers across Bitcoin, testnet, signet, and regtest.
  • Enforces exact lowercase btc and identifier casing, preserves issuer order, removes duplicates, and drops unsupported or wrong-network identifiers.
  • Requires endpoint payloads to be JSON objects with a non-empty string value, retaining optional string bounds and rejecting non-string bounds or wrong-network identifiers.
  • Uses the byte-identical iOS fixture with SHA-256 35f671bbc4c437dbd28cacb33ca09f8ba9d91c6d3a422a247abe0969ad7a2a5e.
  • Distinguishes third-party bitkit/server issuers from Bitkit-to-Bitkit bitkit/wallet issuers and documents raw UI Automator inspection for container test tags.

Preview

N/A — no user-visible changes.

QA Notes

Manual Tests

  • 1. Saved and linked regtest issuer on bitkit/server → publish a current btc-regtest-p2wpkh JSON endpoint → send the canonical 100,000-sat one-time request: Payment Request Confirm opens automatically with 100,000 sats.
  • 2. Dismiss the automatic confirmation with Android back → tap the pending-request bell → Payment Requests → Pay → Show details: the confirmation shows 100,000 sats and the saved issuer contact.

Automated Checks

  • PaykitIssuerInteropTest.kt: 3/3 tests passed on pre-merge head 731fb6b 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 pre-merge head 731fb6b using an isolated Pixel 9a API 36 AVD, including the canonical Row, Pay, and Dismiss identifiers.
  • Test coverage consolidated from PublicPaykitRepoTest.kt and PaykitPaymentRequestRepoTest.kt into the canonical fixture suite so one source exercises the real parser and request mapper.
  • Pre-merge compile, full unit suite, detekt, journey XML validation, and E2E Debug build passed on 731fb6b.
  • Current master e2fa21f was merged with the Paykit subscription architecture preserved; post-merge checks were not run at user direction.

@greptile-apps

This comment has been minimized.

ben-kaufman
ben-kaufman previously approved these changes Sep 2, 2026
@ovitrif ovitrif changed the title docs: codify paykit issuer interoperability docs: codify paykit issuer contract Sep 3, 2026

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the issuer contract against the iOS port (#719). The shared fixture is good, but it cannot currently catch the two places where the platforms actually disagree — one low comment with the cases to add.

Comment thread app/src/test/resources/paykit-issuer-interoperability.json
ben-kaufman
ben-kaufman previously approved these changes Sep 6, 2026

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 privateinternal, 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.

Comment thread app/src/main/java/to/bitkit/repositories/PaykitIssuerInterop.kt
@ovitrif
ovitrif force-pushed the codex/1208-paykit-interop branch from 2998631 to 41ac764 Compare September 7, 2026 15:58
@ovitrif ovitrif self-assigned this Sep 7, 2026

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Network gate at parse — same point, same shape. parseEndpoint(methodId, endpointData, network = Env.network)MethodId.fromRawValue(methodId, network), so btc-bitcoin-p2wpkh on regtest is dropped before the payload is decoded, and isPayable re-checks the decoded address network afterwards. Not a behaviour change from master — fromRawValue already compared against rawValueForNetwork(Env.network); this only parameterises it, and every call site still passes the default.
  2. Strict non-string min/max — genuinely Android behaviour, so iOS was matched to something real rather than to a fiction. Json(appJson) { isLenient = false } overrides JsonModule's isLenient = true, so kotlinx requires quotes for a String? and 1000/true throw into the runCatchingnull. ignoreUnknownKeys is inherited, and explicit "min": null is still accepted.
  3. Display == execute — untouched. The only send-confirm change is the root testTag; shown and paid amounts both come from _sendUiState.value.amount, with acceptsPaymentAmount/acceptsLightningInvoice re-checked at confirm time and AppViewModel.kt:4324-4329 refusing to proceed if the request context vanished.
  4. No issuer string reaches UI or logsPaykitIssuerInterop.kt has no Logger calls at all; every request-path log passes counterparties through PubkyPublicKeyFormat.redacted(...) and errors through redactedContext(). The only issuer text reaching UI is the trimmed note, same as iOS.
  5. 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.

@ovitrif
ovitrif force-pushed the codex/1208-paykit-interop branch from 41ac764 to 47d1895 Compare September 7, 2026 23:03
@ovitrif
ovitrif requested a review from jvsena42 September 7, 2026 23:03
@ovitrif
ovitrif force-pushed the codex/1208-paykit-interop branch from 47d1895 to cff08e9 Compare September 8, 2026 12:58
@ovitrif
ovitrif force-pushed the codex/1208-paykit-interop branch from cff08e9 to 1867ae3 Compare September 8, 2026 15:13

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) inheriting ignoreUnknownKeys = true. Non-object top level, raw string, malformed JSON, missing/null/numeric value, numeric min, boolean max all throw into runCatching -> null; empty/whitespace value rejected explicitly. There is no default for value, so no fail-open on the recipient. Nothing in the payload is logged — the file has no Logger call — and no issuer string reaches a path, URL or network call. An unbounded-length value is accepted by shape but must then decode as a real address/invoice/LNURL in isPayable, which also enforces invoice expiry and network, so it can't be smuggled into the UI.
  • Identifier filter. Exact-string match against rawValueForNetwork(network) then distinct() — 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, > 0uL rejects zero, overflow throws, then the ULong.MAX_VALUE / 1000uL cap. No ULong arithmetic is performed on the issuer value (comparison only), so the USat wrapping rule isn't implicated here. Display and execution both read _sendUiState.value.amount, and the confirm-time acceptsPaymentAmount / acceptsLightningInvoice re-checks are unchanged from master.
  • The doc matches what the code enforces. Every claim in docs/paykit-issuer-interoperability.md maps to a specific guard — asset, the 18,446,744,073,709,551 bound, 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: onSheetVisible marks presented and nulls requestedPaymentRequestId, so the back-path defer condition is false and the request isn't auto-re-presented, but stays in pendingRequests for the bell. "Saved as a contact" was needed because the recipient row otherwise falls back to PubkyProfile.placeholder(publicKey).
  • Behaviour-change audit. fromRawValue(value, network = Env.network) and parseEndpoint(..., network = Env.network) default to the same value master compared against; every production call site passes the default. toPaykitPaymentRequest only widened private -> internal. The three "btc" -> BITCOIN_ASSET substitutions are the only occurrences. serializePayload still emits {"value":"..."} with encodeDefaults = 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.

@ovitrif ovitrif changed the title docs: codify paykit issuer contract refactor: codify paykit issuer contract Sep 9, 2026
@ovitrif
ovitrif requested a review from jvsena42 September 9, 2026 16:02
@jvsena42

jvsena42 commented Sep 9, 2026

Copy link
Copy Markdown
Member

Cross-platform journey run — Android side

Ran journeys/payment-requests/issuer-interoperability.xml on a live Pixel_9 emulator against the iOS twin (bitkit-ios#719) on an iPhone 17 simulator, plus the shared fixture suite. All 12 actions pass.

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 → ContactPay → Request → amount → PaymentRequestSend), so the iOS build acted as the issuer and Android as the receiver. Identities: Android pubkya83fojuspu61rp5no8u9mro99tgmii6gh9j9opby6yu8x79jxjhy ("aaa"), issuer pubky19nuszfku3aqde9s66uihyxx3m8zqudxosk1nyemciwe53pkcwiy ("Paykit QA"), each saved as the other's contact.

Request b78df292-84a7-4d42-86ad-73464647aa77, 100,000 sats / $66.59. The confirmation auto-presented ~50s after send, back-press dismissed it as the journey says, the bell appeared, the sheet listed the row as "Paykit QA · ₿ 100 000 · $ 66.59", Pay reopened the confirmation, Show Details resolved ReviewContactRecipient to "Paykit QA". No payment was completed — the journey stops at the recipient check.

PaykitIssuerInteropTest: tests=3 failures=0 errors=0. The fixture JSON is byte-identical to the iOS copy (sha256 35f671bb…), and both suites are data-driven over all 48 cases (33 request + 15 endpoint) with matching test names.

The documented divergences held exactly as written: the row testTag is the bare PaymentRequestRow-b78df292-… (no counterparty/path/period suffix, unlike iOS), and dismissal is a back press rather than a swipe.

Two things worth a look

1. The receiver path is bitkit/wallet, not bitkit/server. journeys/payment-requests/README.md says to link the issuer on receiver path bitkit/server. Bitkit-to-Bitkit actually links on bitkit/wallet — this build's own paykit logs are the clearest evidence:

paykit_lib::pubky_routing: fetch_paykit_receiver_marker;
  receiver_path=PaykitReceiverPath("bitkit/wallet") owner=pubky19nusz… receiver=bitkit/wallet

I deliberately did not change the journey: bitkit/server may well be correct for the third-party fixture issuer the journey actually describes, which I never ran. Worth a word from whoever owns that fixture — if it is also bitkit/wallet, both READMEs need the fix.

2. android layout silently omits container testTags. PaymentRequestConfirm, SendSheet and PaymentRequestRow-* are applied to plain Box/Column nodes (SendConfirmScreen.kt:224, PaymentRequestsScreen.kt:525) and are absent from android layout --pretty output while being present in the app. I nearly filed a false "missing testTag" bug off that. The raw hierarchy shows them:

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 snapshot-ui. Probably worth a line in journeys/README.md on both sides, since every container assertion in this suite hits it.

Notes, not problems

  • Incoming requests poll on a 30s→60s→120s backoff (AppViewModel.kt:5244) — identical constant to iOS. Arrival took ~50s here; journeys shouldn't assume prompt arrival.
  • This branch is on paykit-android 0.1.0-rc51 while iOS is still on paykit-rs 0.1.0-rc46, so the contract is currently verified across a five-RC gap. bitkit-ios#697 closes it from the iOS side.
  • Expiry tags are PaymentRequestExpiryHour/Day/… here vs iOS's localized Tab-1 hour/… Not journey-relevant today, but the two won't stay diffable if a journey ever asserts on them.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread journeys/payment-requests/issuer-interoperability.xml Outdated
@ovitrif
ovitrif requested a review from jvsena42 September 9, 2026 22:55

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 by isPayable (PublicPaykitRepo.kt:395-410: bolt11 unexpired + network match; LNURL must decode as Scanner.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 = false means an unquoted or numeric value where a string is expected throws → runCatchingnull → endpoint dropped. Non-object top level, raw string, array and malformed JSON all → null. Fails closed on an unrecognised identifier: MethodId.fromRawValue returns null before 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 value is 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.

@ovitrif
ovitrif requested a review from jvsena42 September 10, 2026 09:55
Comment thread app/src/main/java/to/bitkit/repositories/PublicPaykitRepo.kt

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually reviewed and cross-tested with synonymdev/bitkit-ios#719

@jvsena42
jvsena42 enabled auto-merge September 10, 2026 10:39

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jvsena42
jvsena42 merged commit b83d7ae into master Sep 10, 2026
42 of 48 checks passed
@jvsena42
jvsena42 deleted the codex/1208-paykit-interop branch September 10, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: codify paykit issuer interoperability

3 participants