Skip to content

docs: codify paykit issuer contract - #1216

Open
ovitrif wants to merge 5 commits into
masterfrom
codex/1208-paykit-interop
Open

docs: codify paykit issuer contract#1216
ovitrif wants to merge 5 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.

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 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.
  • Test coverage consolidated from PublicPaykitRepoTest.kt and PaykitPaymentRequestRepoTest.kt into the canonical fixture suite so one source exercises the real parser and request mapper.
  • Local compile, full unit suite, detekt, journey XML validation, and E2E Debug build passed.

@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.

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