Skip to content

chore: upgrade paykit to rc74 - #1470

Merged
ben-kaufman merged 1 commit into
masterfrom
codex/paykit-rc74
Oct 13, 2026
Merged

ben-kaufman merged 1 commit into
masterfrom
codex/paykit-rc74

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1471

This PR upgrades Bitkit to Paykit rc74 and keeps payment approval bound to the proposal the user reviewed.

Description

  • Use the published com.synonym:paykit-android:0.1.0-rc74 package from release commit 03d4f05721e92d3944cb4d5af4b2c98c4390a5a8.
  • Carry the reviewed proposal ID through one-time and subscription acceptance so a missing or replaced proposal cannot be accepted from an outdated review.
  • Reject a refreshed request with a different proposal ID before recording local acceptance ownership.

Design

N/A — no UI changes.

Preview

N/A — no layout changes.

QA Notes

Journeys

N/A — not drivable; see Manual Tests. The changed rejection cases require controlled replacement or omission of a proposal stream-item ID.

Manual Tests

  • 1 regression: review a one-time or subscription proposal → replace it with the same terms under the same request ID before accepting → the original review cannot accept the replacement — controlled proposal identity replacement is not in Capabilities.
  • 2 regression: open a request without its proposal stream-item ID → attempt acceptance → the request is unavailable — malformed SDK record injection is not in Capabilities.

Automated Checks

  • added PaykitPaymentRequestRepoTest.kt — missing proposal IDs and refreshed replacements fail before SDK acceptance or local acceptance ownership is recorded.

  • added PaykitPaymentRequestRepoSubscriptionTest.kt — missing and replaced subscription proposal IDs fail closed.

  • updated PaykitPaymentRequestRepoTest.kt, PaykitPaymentRequestRepoSubscriptionTest.kt and PaykitSdkServiceTest.kt — positive acceptance and operation-lock coverage passes the reviewed proposal ID to the required SDK argument.

  • ran a complete Detekt report comparison against current master (f3bb3d20a): all 1,980 findings are unchanged, with zero introduced findings after formatting.

  • ran a fresh final review of the migration on current master, including the newer subscription first-payment paths: no material findings. Four regression cases added; no tests removed.

  • ran just compile, just test, just lint and just format locally against the published rc74 package: 3,989 tests passed, zero failures or skips. The 13 main-source and 209 test-source compiler diagnostics match current master exactly; no new compiler or Detekt findings.

  • ran independent Maven artifact validation: the registry and Gradle cache agree on AAR SHA256 ee4284a9ffcc8f959fa4cf7ef2f2aa9b7f64c7caac4c065d890e65e16400ef70; generated sources match the release bindings, with all four native architectures and all 231 expected UniFFI checksum exports per architecture.

  • ran a separate-client grant-signup probe against the released rc74 SDK and an isolated Pubky testnet: three fresh identities registered, then approved the requester on the first attempt from a new client. Each requester received the registered identity with exactly the requested capabilities. This is SDK integration evidence, not a device UI run.

Live payment, mixed rc73/rc74 payment and wallet recovery flows were not run during this follow-up.

@greptile-apps

greptile-apps Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High impact] The PR appears safe to merge with its existing prerequisite, #1458.

Summary

Upgrades Paykit from 0.1.0-rc73 to 0.1.0-rc74 and carries the reviewed proposal ID through acceptance.

  • Payment approvals stay tied to the proposal you reviewed.

No actionable findings. Tests were inspected, not run during this review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[User approves reviewed proposal] --> B{Proposal ID present?}
  B -->|No| X[Request unavailable]
  B -->|Yes| C{Current proposal matches review?}
  C -->|No| X
  C -->|Yes| D[Pass reviewed ID to SDK acceptance]
  D --> E[Update local accepted state]
Loading

Reviews (1) · Last reviewed commit: "chore: upgrade paykit to rc74" · Reviewed by Greptile

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No findings. Bumps Paykit to rc74 and passes the reviewed proposal's stream item id to claimAndAcceptPaymentRequest, so a proposal replaced under the same Payment Request ID cannot be accepted from an older review. Reviewed d5280c0, full tier.

What I checked, and 5 candidates I ruled out

Read in full: the 8 changed files of the diff against codex/paykit-observation-proof-handoff; PaykitPaymentRequestRepo.accept, updateRequest, ensurePaymentAllowed; the subscription branch of accept; AppViewModel.acceptIncomingPaymentRequestIfNeeded and acceptSubscriptionAndStartPayment.
SDK contract: paykit-rs v0.1.0-rc74 (03d4f05): claim_and_accept_payment_request in paykit-sdk/src/runtime/payment_requests.rs and require_current_payment_request_action in paykit-sdk/src/domain/payment_requests/mod.rs. The FFI surface diff rc73..rc74 adds the id to accept_payment_request, claim_and_accept_payment_request and accept_allowance, plus forget_peer; the app calls only the second.
CI: lint, detekt and Greptile green; build and build-local still running at review time. The branch was not built here: on bitkit-android this review reasons from the code and CI.

Ruled out

  • Id read at accept time instead of the displayed one: the SDK doc asks for "the id that was displayed, not one read just before this call". The one-time path captures request.proposalStreamItemId from the request the UI holds, then compares it with current from _pendingRequests inside updateRequest's lock; the subscription path requires current == subscription, and data class equality now includes the id. Both pass the displayed value.
  • Check-then-act window: the SDK repeats the comparison in the same storage transaction that claims and queues the Acceptance, and a mismatch is a Policy error. That is not in the uncertain set, so the locally recorded accepted id is rolled back as before.
  • A request built without the id: every incoming PaykitPaymentRequest comes from PaymentRequestRecord.toPaykitPaymentRequest or from PaykitSubscription, which both carry the field now. The other constructors are outgoing (toCreatedPaykitPaymentRequest) or previews. Billing-period requests carry it but never take the requiresAcceptance path.
  • Proposal id moving under a normal refresh: the reducer sets proposal_stream_item_id once, from the proposal's stream item, and a second proposal under the same ID marks the record invalid. A routine refresh cannot change it and lock out a legitimate accept.
  • Unwired rc74 surface: accept_allowance also gained the parameter, but the app has no allowance acceptance call site, so nothing else fails to compile against the new bindings.

Merge confidence: 4/5, build and build-local were still running and the branch was not built here.

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 3d01f12 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

jvsena42
jvsena42 previously approved these changes Oct 12, 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.

Approving at d5280c0: no findings. I reviewed this as a payment-approval change, not a chore: besides the version bump it adds a fail-closed check to acceptance and ships a security changelog fragment.

Checked:

  • One-time accept: the request captured when the review is presented (AppViewModel.kt:1392, :3451) is the object passed to accept; its proposalStreamItemId is compared with the pending entry (PaykitPaymentRequestRepo.kt:994) and sent as the reviewed value (:1008). A missing id throws RequestUnavailable (:992); there is no fallback to the latest proposal.
  • Subscription accept: the view model requires the stored subscription to equal the displayed one (AppViewModel.kt:6360-6362), the repo requires it again (:1071), and equality now includes the proposal id (PaykitSubscription.kt:190). A missing id throws before the SDK call (:1073-1074). The first payment is a billing-period request and does not go through a second accept.
  • Auto-presented requests and the hardware path use the same captured request; retry refreshes and re-presents, so the fresh terms are reviewed; no review is persisted across a relaunch.
  • Ownership: the local mismatch check runs before addAcceptedOneTimeId (:1001), and an SDK Policy rejection removes the id unless it was already saved (:1020-1022). On rejection the request stays pending with fresh data and nothing is paid, since accept precedes any send.
  • rc73 to rc74: proposal_stream_item_id is derived from stream items on read and not persisted, so there is no stored-format change; the other changed SDK entry points (restoreBackupState, acceptAllowance, forgetPeer) are not called by the app.
  • Tests pin replaced and missing ids for one-time and subscription acceptance at the repo level, with no SDK call and no ownership write.

Two notes, no change requested:

  • rc74 changes signup approval: approve_auth now treats an existing homeserver account as an error where rc73 tolerated it, and the app registers and then approves through two different clients (PubkyRepo.kt:1528-1535). A first-time grant signup may fail once and succeed on retry. This is from reading the SDK; I have no grant-signup requester to drive it. The same concern is raised on synonymdev/bitkit-ios#921.
  • #1432 still calls the two-argument accept; whichever merges second has to add the proposal id.

Device gate: no device check needed — no user-visible change on the accept path and no journey files; the two manual tests need a controlled proposal replacement, which the app itself cannot produce (it never calls forgetPeer). perf: not relevant.

⚠️ Not run: a plain accept-and-pay between two rc74 builds, and between an rc74 and an rc73 build.

Skipped — covered by CI at d5280c0: unit tests, detekt, lint, e2e (all green).

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Full review of the complete pull request diff against merge base afbf14d (stacked base codex/paykit-observation-proof-handoff), at d5280c0.

No new actionable code findings.

One-time and subscription approval stay tied to the proposal that was reviewed. accept reads proposalStreamItemId from the request or subscription held by the UI, refuses a missing id, and passes that same id to claimAndAcceptPaymentRequest. A one-time accept compares it with the current pending record inside updateRequest before local acceptance ownership is stored. A subscription accept requires the stored subscription to equal the displayed one, and that equality includes the proposal id. A replaced proposal fails with RequestUnavailable and does not call the SDK. The same binding is in bitkit-ios#921 at c1ae1a2; Android also rejects the mismatch in the repository before the SDK call.

Paykit v0.1.0-rc74 (03d4f05721e92d3944cb4d5af4b2c98c4390a5a8) also changes signup approval: when no homeserver record is visible, an existing account fails approve_auth. Grant signup still calls registerIdentity and then approveAuth on a separate client. That sequence succeeds when signUp has already published the matching homeserver record. This review did not execute that flow.

Validation: the new repository tests were inspected. On this revision, CI ran testDevDebugUnitTest in build, plus lint, detekt, and the local end-to-end jobs in run 38298976984. Those jobs do not exercise a replaced proposal id. Device testing: not performed in this review.

Ready for device testing.

Suggested additional test cases

  • Android, Paykit UI enabled, two regtest wallets on this build. The payer opens a one-time request, confirms the amount shown, and pays. The payment completes for that amount and the request leaves the pending list. Repeat for a subscription: accepting it starts the first period at the reviewed amount.
  • Android, fresh wallet with no Pubky identity. Approve a grant-signup auth URL for the app's homeserver. The first attempt finishes signup and authorization.

piotr-iohk
piotr-iohk previously approved these changes Oct 13, 2026
Base automatically changed from codex/paykit-observation-proof-handoff to master October 13, 2026 10:43
@piotr-iohk
piotr-iohk dismissed stale reviews from jvsena42 and themself October 13, 2026 10:43

The base branch was changed.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (f3bb3d20a) now that #1458 has merged. The rc74 patch is unchanged.

I checked the grant-signup concern against the released rc74 SDK on a local Pubky testnet. In three fresh-identity runs, registration completed before a new approval client processed the signup URL. All three succeeded on the first attempt, and the requester received the same identity with exactly the requested capabilities. sign_up awaits publication of the homeserver record before returning. I could not reproduce the suggested first-attempt failure in this setup.

The #1432 compatibility note is also addressed: its current head (7e7fae47b) already uses rc74 and passes the reviewed proposal ID.

Validation on this rebase: 3,989 unit tests passed, compilation passed, and complete compiler/Detekt comparisons found no new diagnostics versus master. Formatting and a fresh review are complete. Device UI and mixed rc73/rc74 payment checks were not rerun here.

@ben-kaufman
ben-kaufman enabled auto-merge October 13, 2026 12:31

@ovi-reviewer ovi-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: 👍 Approve

Review: diff 8 files.
Pair PR synonymdev/bitkit-ios#921 at c1ae1a2 ties acceptance to the reviewed proposal. Android rejects a refreshed local mismatch before recording acceptance ownership.

QA:
Tests queued.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: 👍 Approve

QA for 3d01f12:
Interim test report for the review; the rest resumes on its own.

Tested on three Android 15 emulators, regtest.

🟠 Test 1
Test 1

Incomplete: lane CLI process exited 255 before proposal replacement.


🟢 Test 2
Test 2

On 3d01f12 + test hook: acceptance with a missing reviewed proposal ID showed unavailable and left the request pending without payment.

2.mp4
test hook

Not pushed. Omits the proposal ID from a real incoming request before review and logs the unchanged acceptance result.

diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
index 9803c2bb3..3e2569a2e 100644
--- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
+++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
@@ -3451,7 +3451,12 @@ class AppViewModel @Inject constructor(
         val context = ContactPaymentContext(
             publicKey = publicKey,
             privatePaymentContext = privatePaymentContext,
-            incomingPaymentRequest = incomingPaymentRequest,
+            incomingPaymentRequest = incomingPaymentRequest?.let { request -> // test hook
+                if (Env.isDebug && request.requiresAcceptance) { // test hook
+                    Logger.info("Omitted proposal id '${request.proposalStreamItemId}' for '${request.id}' before review", context = "TestHook2") // test hook
+                    request.copy(proposalStreamItemId = null) // test hook
+                } else request // test hook
+            }, // test hook
             isInitialSubscriptionPayment = isInitialSubscriptionPayment,
             selectedTags = selectedTags,
         )
@@ -6375,7 +6380,10 @@ class AppViewModel @Inject constructor(
 
     private suspend fun acceptIncomingPaymentRequestIfNeeded(context: ContactPaymentContext?): Result<Unit> {
         val request = context?.incomingPaymentRequest ?: return Result.success(Unit)
-        return paykitPaymentRequestRepo.accept(request)
+        if (Env.isDebug) Logger.info("Submitted request '${request.id}' requiresAcceptance '${request.requiresAcceptance}' proposal '${request.proposalStreamItemId}'", context = "TestHook2") // test hook
+        return paykitPaymentRequestRepo.accept(request).also { result -> // test hook
+            if (Env.isDebug) Logger.info("Observed acceptance success '${result.isSuccess}' error '${result.exceptionOrNull()?.javaClass?.simpleName}'", context = "TestHook2") // test hook
+        } // test hook
     }
 
     fun showPaymentRequests() {

Tip

Test 2 worth a journey
Test 2
  • Create fresh payer and requester profiles on separate staging wallets.
  • Add the two profiles as contacts on both wallets and wait for private linking.
  • Fund the payer with 100000 sats in Savings.
  • Send a one-time 1000-sat request from the requester.
  • Open its normal payment review with the debug hook omitting the displayed proposal ID.
  • Swipe once to pay and verify the request-unavailable error.
  • Verify the request remains pending and the payer balance is unchanged.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Full reassessment of the entire pull request at 3d01f12 after the comparison base changed and no inherited baseline was available. Coverage is the full diff from merge base f3bb3d2.

No new actionable code findings.

One-time and subscription acceptance require the proposal stream item id from the request the user reviewed. A missing id, or a refreshed pending record whose id differs, fails with request unavailable before claimAndAcceptPaymentRequest and before one-time acceptance ownership is stored. The id passed to the SDK is that reviewed value. In paykit-rs v0.1.0-rc74, claim_and_accept_payment_request checks it inside the accept transaction. A second proposal for the same payment-request id does not replace terms in place, and a proposal sent again after forget_peer gets a new stream item id. restoreBackupState, acceptAllowance, and forgetPeer have no call site in this app, so the new restore-link policy has nothing to migrate here.

The accept binding matches bitkit-ios 66082fa: both platforms pass the displayed id and reject a missing id. Android also rejects a refreshed in-memory replacement before the SDK call and uses claim-and-accept. That comparison is source-only and is not a review of the iOS app.

Validation: Inspected PaykitPaymentRequestRepoTest.kt, PaykitPaymentRequestRepoSubscriptionTest.kt, and PaykitSdkServiceTest.kt. Those tests were not executed in this review. The CI build for this commit succeeded.

Device testing: not performed in this review.

Ready for device testing

Suggested additional test cases

  • Android, two linked contacts on this build, payer funded. Requester sends a one-time request. Payer reviews the amount and pays. The payer balance drops by the reviewed amount and the request leaves the pending list.
  • Android, same setup. Requester sends a subscription. Payer reviews the amount, subscribes, and pays the first period when prompted. The subscription becomes active and the payment matches the reviewed amount.

@ben-kaufman
ben-kaufman merged commit 08d02b5 into master Oct 13, 2026
22 checks passed
@ben-kaufman
ben-kaufman deleted the codex/paykit-rc74 branch October 13, 2026 15:48
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.

bug: paykit can accept a proposal other than the one the user reviewed

4 participants