feat: add subscription requests - #1239
Conversation
 The PR should not merge until payment history for deleted creator subscriptions remains reachable. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed for fund draining specifically. No critical or high findings.
This adds the creator/payee side and re-tags the existing payer logic with isPayer guards. Every path that moves sats is untouched and still needs a fresh user action per period (acceptSubscriptionAndStartPayment → send flow; due periods → openIncomingPaymentRequest → send flow). Nothing auto-pays, nothing pays a cancelled or dismissed period, and the persisted-before-cleared dismissal ordering from the earlier fix is intact (dismissSubscriptionPayment, :289-322).
I specifically checked for the three bugs confirmed on the earlier subscriptions work — none repeat: creation uses a callback rather than a StateFlow, and the proof/dismissal code is unchanged, so neither the proof-kept-forever wedge nor the queue-cleared-before-persist ordering is reintroduced.
Also verified clean: amount shown vs paid is still enforced by acceptsPaymentAmount; cross-identity is guarded by expectedIdentity/generation under operationMutex with the SDK re-checking identity in uploadProfileAvatar and proposePaymentRequest, and publishCreatedSubscription gating on isCurrentState; payee records can't leak into one-off history (toPaykitPaymentRequest:1188 rejects recurrence != null); the notification scheduler, monthly-cost, proposals and accept paths are all isPayer-filtered; runSuspendCatching throughout the new suspend paths; creationMutex.tryLock guards double-submit.
Three LOW notes inline, all dev/QA-facing today (isPaykitEnabled default false). Two of them also apply to the iOS twin (synonymdev/bitkit-ios#736).
|
conflicts |
There was a problem hiding this comment.
Re-reviewed at 70629e09. No new findings.
Attribution: the delta since my last pass (29cecb8a) is exactly one commit — 70629e095 chore: merge master into subscription requests — touching 34 files, of which 30 are byte-identical to master (#1216, #1235, #1236 landing) and 4 differ only on master's side. Net: this PR authored zero new lines since I last looked, so I re-verified the PR's own subscription code at head against the fund-safety list rather than reviewing a delta.
One merge interaction I checked because it's the kind of thing that slips through: master's #1216 made MethodId.fromRawValue(value, network = Env.network) network-aware, but PaykitSubscription.kt:313 still calls the one-arg form. The default parameter makes it behaviourally identical in production, and creator-side acceptedPaymentEndpointIdentifiers uses MethodId.rawValue = rawValueForNetwork(Env.network), so proposer and payer agree on identifiers. Not a finding, but worth a glance if the default ever changes.
Fund safety — the reason this PR gets the scrutiny it does. The creator-side role split only narrows the payer paths rather than adding new ones: requestsThrough returns empty for payee (:240), accept() requires it == subscription && it.isPayer && isProposalActionable and pins the displayed value (repo:690), the scheduler is gated on isPayer (scheduler:70), and restoredAcceptances is filtered on isPayer (repo:767). No auto-start charge path exists on the creator side. Period arithmetic holds: every == 0u rejected at :296, every.toLong() * index stays inside Long, and the plusX calls are wrapped in non-suspend runCatching — correct usage. No new ULong arithmetic was introduced.
Also clean: nothing seed-derived written, sent or logged; uploadProfileAvatar(expectedIdentity) and proposePaymentRequest(expectedIdentity) both check live identity, and publishCreatedSubscription gates on isCurrentState(generation, expectedIdentity); inbound proposal fields are all bounded (description 1024, benefits 8×160, note 256, icon_uri 512); every suspend path uses runSuspendCatching, with runCatching only on non-suspend helpers; dismissSubscriptionPayment persists before mutating in-memory state.
Recorded, not filed — the creator-side ledger counts payer-asserted proofs. PaykitSubscription.kt:345-354 reads only billingPeriod and paymentEndpointIdentifier; the SDK's proof payload is never inspected and nothing correlates against LDK or on-chain receipts, so a subscriber can make "Payments: N" and N +amount rows appear on the creator's detail screen without paying. Both platforms apply the same billing-grid filter before counting a proof (Android periodsThrough at PaykitSubscription.kt:352, iOS recurrence.contains(billingPeriod) at PaykitSubscription.swift:547-557), and Android derives the rows on the fly rather than persisting them. One Android-specific consequence worth knowing: isCreatedVisible (:216) includes || paidPeriods.isNotEmpty(), so a single bogus proof pins a cancelled subscription in the Created list.
This is the same pre-existing Paykit-wide trust model I recorded on the iOS twin (synonymdev/bitkit-ios#736) rather than filing against it — the SDK does stateless correlation only and delegates settlement to the caller. Not this PR's defect; this is just the first Android code to render the number. If it's ever addressed, the narrow interim is to keep paidPeriods.isNotEmpty() out of the delete-visibility rule, or label those rows as reported rather than settled.
Status of my open threads: the short-first-billing-period one (PaykitPaymentRequestRepo.kt:576) is still unanswered — head still has startsAt = timestamp, anchor = timestamp with timestamp = proposal time. The icon-namespace and orphaned-icon-blob threads you replied to today are both deferred with reasons I accept; I've left a note on the first with input from the iOS side for whenever the coordinated follow-up happens.
Gating unchanged: dev/QA-facing today.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Sent proposal sheet his not respecting top padding
Cross-platform test with iOS #736Android Verdict: the Android side behaved correctly everywhere I could test it. The test IDs don't match iOS, which I'm treating as blocking, and the top-padding issue from my earlier review is still there. Results against the QA list
🔴 Blocking: IDs that don't match iOSThese have to match, per the shared journey vocabulary. AGENTS.md makes Android the reference, so most of the fixes may land on iOS (#736). Please settle each one with that PR so both land together.
With the amount step reusing the Still open from my earlier review: top paddingIt still reproduces at Other findings
Emulator noteThe icon picker on the emulator is Google Photos' picker: tapping a tile only selects it, and it isn't applied until Done is tapped. The app correctly launches a single, image-only |
|
Applied the standard sheet height to both the Create Subscription and Sent proposal states, so their titles keep the same top clearance as the other creation steps. Android's existing test tags remain unchanged as the shared journey reference. Sender identity and encrypted-link recovery remain separate from this layout fix. |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Both blockers are resolved and verified, so this no longer applies. Top padding: fixed at 8f1e6f7 and confirmed on a Pixel 9 emulator running the PR-head CI build — the Create Subscription drag handle and title now clear the status bar and the Subscriptions top bar. Test IDs: iOS aligned to Android's tags in synonymdev/bitkit-ios#736 at 1833d00a (Tab-day/week/month/year from rawValue, so also locale-independent; PaymentRequest prefix on the amount step; contact-row hyphen dropped). Details in my new review.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at 8f1e6f7eb. Both of my blockers are resolved — I've dismissed the stale CHANGES_REQUESTED. One new MEDIUM inline, gated so not blocking.
Top padding: fixed, and I verified it on device rather than from the diff. Installed the PR-head CI build (bitkit-dev-debug-188 from 8f1e6f7eb) on a Pixel 9 emulator: the Create Subscription drag handle and title now sit clear of both the status bar and the Subscriptions top bar. The fix is also complete — all four creation steps get the standard height now: Details (:150), Amount (:165, pre-existing), Recipient (:384, pre-existing), Sent (:204). SubscriptionProposalSent correctly takes modifier: Modifier = Modifier and applies it first in the chain (:433, :436), matching CreateSubscriptionDetails (:224, :241).
One honest gap: I could not reach the Sent proposal sheet on device this run. The recipient step showed SubscriptionRecipientUnavailable with Propose disabled — no privately-linked contact available, which is the stuck encrypted-link state I documented on synonymdev/bitkit-ios#736. So the Sent sheet is verified by code reading only: identical sheetHeight() application, and the layout cannot clip because it uses BoxWithConstraints(weight(1f)) -> verticalScroll + heightIn(min = maxHeight) (:452-456), with fillMaxSize() after a fixed height() being a no-op.
Test IDs: resolved on the iOS side, which is where the fixes belonged since AGENTS.md makes Android the reference. Verified at 1833d00a against the tags I read off the live emulator:
| Element | Android (on device) | iOS at 1833d00a |
|---|---|---|
| Frequency tabs | Tab-day / -week / -month / -year |
TabItem($0, accessibilityIdentifier: "Tab-\($0.rawValue)") with case day/week/month/year — matches, and now locale-independent since it keys off rawValue rather than the localized description |
| Amount step | PaymentRequestAmount, PaymentRequestAmountContinue, PaymentRequestAmountField, PaymentRequestNumberPad |
testIdentifierPrefix: "PaymentRequest" — matches |
| Recipient contact row | SubscriptionContact<pubky>, no separator |
hyphen dropped — matches |
The new finding closes row 7 of my own QA table, which I had logged as "Keyboard — not applicable on the emulator run". It was never exercised; it is now, and there is a real gap. Inline at :244.
Checked and clean, at head:
- Cancel and Back paths of the creation flow, enumerated. Details -> system Back hides the sheet and only
rememberstate is lost; nothing remote or local exists yet. Amount -> Back and Recipient -> Back both return to Details with the draft intact (:160,:174). While a propose is in flight, scrim, drag and system Back are all blocked byContentView.kt:502dismissEnabled = !isCreatingPaymentRequest && …(honoured bySheetHostat:182and:191), andcreationMutex.tryLock(repo:528) blocks double-submit. Sent -> OK/Back just hides. An identity change or generation mismatch mid-proposal giveswasPublishedToActiveState=false-> sheet hidden plus a "queued" toast, with no local record created for a foreign identity (AppViewModel:5061-5071,repo:583,:677). An in-progress icon load gates both Choose Recipient and Propose (:330,:379). - Creator cancel/delete is remote-first:
repo:1069 operation(current)then:1072 applySubscriptionRecordLocked, insiderunSuspendCatching. ACancellationExceptionlanding between them leaves only a stale cache — the SDK record store, not local JSON, is the source of truth and is re-read bysynchronizeLocked(:919). Delete goes through theSubscriptionCancelconfirmation route (SubscriptionsScreen.kt:653-661). There is no "crash resurrects a cancelled subscription" path here. - Upgrade from v2.5.0.
PaykitSubscriptionis derived, not persisted; the only persisted local state issubscriptionAcceptedAt/dismissedSubscriptionPaymentIds, and the repo diff touches no persistence code. The new model fields all default (role = Payer,iconUri = null,deliveryStatus = null), and role comes from the SDK'slocalRolewithUNKNOWN/null mapping to null — the record is dropped rather than mis-assigned a role. No SDK bump: paykit-android0.1.0-rc51, same asv2.5.0. - Fund safety, key material and trust boundaries are unchanged since my
70629e09pass — this commit is layout-only and introduces no new state, suspend calls or arithmetic.
I also corrected a line in my 2026-09-10 review: I'd written that Android was stricter than iOS on the billing-grid check. Both platforms apply the same filter — Android periodsThrough at PaykitSubscription.kt:352, iOS recurrence.contains(billingPeriod) at PaykitSubscription.swift:547-557.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at b62e0d402. The keyboard fix works — verified on device — but it covers only the Details step. The Recipient step has the same exposure and still reproduces. One MEDIUM inline, gated so not blocking.
Details step: fixed. Installed the PR-head CI build on a Pixel 9 and repeated the measurement from my earlier thread. With mInputShown=true on the Description field, SubscriptionChooseRecipient moved from [42,2172][1038,2319] (under the keyboard, top edge ~y1540) to [42,1352][1038,1499], and SubscriptionDescription from [42,1317][1038,1554] to [42,1115][1038,1352] — fully visible instead of clipped. The commit is exactly two lines, the import at :20 and .imePadding() at :246, placed inside the Column's chain as suggested.
Recipient step: same bug, still there. Same build, same device: tap SubscriptionRecipientSearch, keyboard opens, and SubscriptionPropose does not move — [42,2172][1038,2319] both before and after, entirely under the keyboard. Details inline on CreatePaymentRequestScreen.kt:415.
I checked the other two steps and they are genuinely not exposed, so this is the last one:
- Amount (
:153-167) uses the customNumberPad(CreatePaymentRequestScreen.kt:185);NumberPadTextField.ktimports nothing fromfoundation.textormaterial3and contains noTextField, so it never raises the IME. - Sent (
:198-207) has no text input, and its chain at:437-444is untouched.
Checked and clean:
- The fix's ordering is correct and does not break the closed-keyboard case.
modifieris the head of the chain at:242and the call site at:151passesModifier.sheetHeight(), so the effective order issheetHeight() -> fillMaxSize() -> … -> navigationBarsPadding() -> imePadding() -> padding()— fixed height outside the IME padding, which is the safe direction.imePadding()is inset-driven, so it pads by zero when the keyboard is hidden; nothing is pushed off the sheet.navigationBarsPadding()thenimePadding()does not double-count, because the outer consumes the nav inset and the inner applies only the excess.SheetHost(BottomSheetScaffold,sheetPeekHeight = 0.dp, nowindowInsets) supplies nothing, so there is no third application. This is byte-identical to the in-repo twinHwPassphrasePromptSheet.kt:121-127. - No scope creep. The diff since
8f1e6f7ebis two files; the.github/workflows/ci.ymlhunk is master's #1241 arriving through the merge commit, not the author's change (git diff master refs/remotes/pr/1239 -- .github/workflows/ci.ymlis empty). The propose path,creationMutex.tryLock(repo:528), thedismissEnabled = !isCreatingPaymentRequestguard (ContentView.kt:502) and draft preservation on Back (:161,:175) are all zero-line diffs. - Conventions on the touched code:
modifieris the first optional parameter (:225,:435) and passed last with no trailing comma (:151,:166,:205,:386,:556). - CI is green on this head.
No test was added, and I am not asking for one — nothing in the repo's androidTest asserts IME layout, and a Compose-rule test cannot drive a real IME. This stays a device check.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at c0caca6a9. Fixed, verified on device, and the shared-host regression risk checked rather than assumed. Nothing further from me on this PR.
The commit is the one-line .imePadding() at CreatePaymentRequestScreen.kt:417 plus its import — exactly the shape I suggested, in the shared content composable so it covers both hosts.
Measured on the PR-head CI build (Pixel 9, mInputShown=true): SubscriptionPropose moved from [42,2172][1038,2319] to [42,1352][1038,1499], i.e. from fully under the keyboard to above it. That was the last of the four creation steps — Details was fixed in the previous commit, and Amount and Sent have no IME exposure (the Amount step uses the custom NumberPad, and Sent has no text input).
The nested-imePadding() case is clean. Worth stating explicitly because I asserted it was safe when I proposed the fix, and an assertion is not evidence: PaymentRequestRecipientContent is also hosted inside ReceiveSheet, which already applies .imePadding() at ReceiveSheet.kt:130, so the new line now sits inside an existing one. Going Receive → Request from contact with the keyboard up, ReceiveScreen bottom goes 2424 → 1541 and PaymentRequestRecipient goes 2361 → 1541 — the inner container lands exactly on the keyboard edge. Double-counting would have shrunk it twice, to roughly y657. windowInsetsPadding consumes the inset for descendants, so the inner call contributes zero excess, and the bounds confirm it.
Also checked: the diff is that one file and two lines, so nothing else moved.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
✅ reviewed and cross tested with iOS PR
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@ben-kaufman you have some commit with unverified signature |
92e836e to
2c96198
Compare
2 participants
Description
This PR adds subscription proposals to contacts, building on the payer flow merged in #1186.
Discover, autopay and renewal UI are intentionally excluded. Icon hosting is public by design. Base is
master; no unmerged payer dependency remains.iOS counterpart: synonymdev/bitkit-ios#736.
Design
Pubky file safety
This branch uses paykit-rs v0.1.0-rc54. Every displayed Pubky URI, including a descriptor's Pubky
src, uses the bounded Kotlin fetch with a 1 MiB successful-body limit before bytes cross the FFI boundary. Existing fixed display sizes, the 15% memory-cache limit and Coil's bounded dedicated disk cache remain in place. Custom subscription icons and public icon hosting remain unchanged.The 1 MiB value caps each successful encoded body. It is not a total-memory or decoded-pixel limit. Paykit rejects oversized successful bodies while reading and detects declared-length truncation. HTTP error bodies may still be buffered inside the Pubky client before Paykit regains control. The request timeout limits duration rather than that error-body memory, transport and current-chunk buffers add overhead, and a close-delimited body can still return partial bytes if the connection closes early. Shared-avatar handling and ambiguous proposal-publication outcomes remain outside this dependency update.
Preview
Create → recipient → sent → overview using test wallets, silent at 4× speed. The first three seconds of the accelerated source clip were trimmed to exclude the system photo picker. All attached media was inspected and shows only Bitkit, including its keyboard; no device home screen or other app is included.
The walkthrough predates the final recipient corrections. The first screenshot below shows the final timer, Contacts spacing, Paste inset and 52dp field height.
android-pr-bitkit-only-4x.mp4
Final recipient, sent confirmation and overview screenshots
QA Notes
Manual Tests
Live regtest creation/delivery/acceptance/manual on-chain payments, custom icons and paid cancellation passed in both directions with iOS. Offline error/draft retention/retry and pending deletion also passed. Live transport-queued flushing, Lightning, mainnet and production push were not tested.
Automated Checks
ScreensMapTest.ktand sevenPubkyImageFetcherTest.ktcases.PubkyImageFetcherTest.ktverifies direct and descriptor-target fetches forward the 1 MiB limit and preserves coroutine cancellation. Native HTTP streaming behavior remains covered upstream in Paykit.PaykitSubscriptionProposalTest.ktcovers UTF-8/wire-size boundaries.PaykitPaymentRequestRepoSubscriptionTest.ktcovers creator terms, queued delivery, oversize rejection before icon upload/enqueue, pending cancellation and duplicate/off-schedule proofs. No automated coverage was removed.CreateSubscriptionScreenTest.kt,SubscriptionsScreenTest.ktand existingCreatePaymentRequestScreenTest.ktregression coverage. Includes all four frequency tabs, no-Discover empty state, compact 520dp confirmation layouts, loading state, single recipient, expiry, and truthful sent/queued copy.