fix: streamline paykit foreground sync

[ben-kaufman]

Description

This PR makes incoming Paykit requests appear sooner while Bitkit is open, without running the full synchronization flow on every inbox check.

• Updates Paykit to 0.1.0-rc54 and uses its backup-content fingerprint so transient SDK operation leases do not trigger wallet backup uploads. Real changes, including partial failures and cancellation, still request a backup; unreadable fingerprints are handled conservatively.
• Checks incoming private messages after 5, 10, 15, then 30 seconds of inactivity. New requests reset that backoff.
• Keeps contact/path discovery, endpoint maintenance, proof reconciliation, and eligible-recipient refresh on a separate 30/60/120-second schedule.
• Preserves the existing bounded handshake burst, foreground lifecycle, 120-second unresolved-request presentation retry, and private-only resolution.

The shorter inbox interval trades extra request-check traffic for responsiveness. Repeated request changes can keep the foreground inbox at its five-second minimum delay. This is intentional for active exchanges and is not a claim of lower battery usage overall.

SDK changes: pubky/paykit-rs#157
Release: https://github.com/pubky/paykit-rs/releases/tag/v0.1.0-rc54

Design

N/A — no UI changes.

Preview

N/A — no UI changes.

QA Notes

Manual Tests

• 1. Linked Paykit contact → keep Wallet open for two minutes → send a new request from the contact: Payment Request opens automatically, normally within the next 30-second inbox check plus processing time.
• 2. regression: background Bitkit → send a request → return to Wallet: the foreground refresh discovers it; no recurring polling continues while backgrounded.
• 3. Payment Request → dismiss the sheet → open the bell → reopen the request: the request remains available and contact/amount are unchanged.
• 4. Add a contact on both peers → return to Wallet: the existing bounded handshake burst still establishes the private connection; no public payment fallback is introduced.

Automated Checks

• Extended the existing polling test in AppViewModelSendFlowTest.kt to verify the five-second reset when requests change, subsequent 10/15-second backoff, independent maintenance, and stopping polling.
• Added PaykitBackupStateTrackingTest.kt for equal, changed, and unreadable fingerprints, partial failures, and coroutine cancellation using the same tracking helper as the SDK service.
• All 2,479 unit tests passed across 166 suites against rc54 downloaded from GitHub Maven, with no failures or skipped tests.
• just compile, just test, just lint, and the dev debug APK build passed. No local SDK artifact was substituted.

[greptile-apps]

Confidence Score: 4/5

The implementation appears safe to merge, with non-blocking test gaps around backup fingerprint decisions and request-driven inbox backoff reset.

Findings

1.  Fingerprint branches lack tests ▶
2.  Backoff reset remains untested ▶

Summary

• Polls incoming requests after 5, 10, 15, and then 30 seconds, resetting the inbox cadence when requests change.
• Runs endpoint, proof, and eligible-recipient maintenance on a separate 30/60/120-second schedule.
• Preserves the 120-second long-term presentation retry independently of inbox polling.
• Uses Paykit rc54’s suspendable backup revision and checks it after successful, failed, or cancelled operations.
• Extends polling cadence tests, although reset behavior and backup fingerprint branches still need focused coverage.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[App enters foreground] --> B[Start initial bounded sync burst]
    A --> C[Start recurring inbox loop]
    C --> D[Wait 5 / 10 / 15 / 30 seconds]
    D --> E{Maintenance due?}
    E -- No --> F[Refresh incoming requests]
    E -- Yes --> G[Refresh private endpoints]
    G --> H[Reconcile payment proofs]
    H --> F
    F --> I{Pending requests changed?}
    I -- Yes --> J[Reset inbox delay to 5 seconds]
    I -- No --> K[Advance inbox backoff]
    E -- Yes --> L[Refresh eligible recipients]
    J --> D
    K --> D
    L --> I
    M[App leaves foreground] --> N[Cancel polling and retry jobs]

Loading

_{Reviews (1) · Last reviewed commit: "fix: streamline paykit foreground sync"}

[greptile-apps]

Fingerprint branches lack tests

The new backup-fingerprint path has no focused test covering equal, changed, or unreadable revisions, or cancellation after a partial mutation. Since null or failed reads request a backup while equal revisions suppress one, a regression could restore unnecessary uploads or silently omit required backups. Please add unit coverage for these branches and the finally behavior.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[ben-kaufman]

Addressed in f030603. Added table-driven coverage for equal, changed, and unreadable fingerprints, plus partial failure preserving the original error and actual coroutine cancellation after mutation. The cancellation test includes a suspending revision read, so it verifies that cleanup runs under NonCancellable. Tests exercise the same small tracking helper used by the SDK service. All 2,479 unit tests, compile, and lint pass.

[greptile-apps]

Backoff reset remains untested

This test never changes pendingRequests, so requestsChanged remains false and the new behavior that resets the inbox backoff to five seconds is not exercised. Make the mocked refresh update the pending-request flow and verify the next refresh after five seconds; otherwise an incorrect reset branch could pass while delaying later requests by thirty seconds.

[ben-kaufman]

Addressed in f030603. The existing polling test now changes pendingRequests during a settled refresh, checks that the next refresh occurs after five seconds, then verifies the following 10/15-second delays. It also checks that maintenance still runs on its independent cadence and that stopping polling stops refreshes. The targeted test and full unit suite pass.

[github-actions]

Regtest APK

Built from f030603 (run).

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

[jvsena42]

Reviewed as a first pass, with the backup change as the thing that mattered. No HIGH or MEDIUM. Two LOW notes inline, neither blocking.

The backup-skip logic is correct, and the conservative-on-error claim holds. This was the risk worth the time: making backups conditional on a fingerprint invites a false negative where a real state change stops being backed up and is lost on restore. I enumerated every path that bumped backupStateVersion on master against head:

Case	master	head
Success, content changed	bump	bump	same
Success, only a lease/counter changed	bump (spurious)	no bump	the intended fix
Block throws (partial failure)	bump if revision differs	finally -> bump if differs	same
CancellationException mid-block	bump if differs	finally + NonCancellable -> bump if differs	same, and the post-read can no longer itself be cancelled
Previous read unreadable	skip (null != null is false)	bump	head is stricter than master — this fixes a latent false negative
Next read unreadable	skip if previous also null	bump	stricter
Unconditional notifyBackupStateChanged() callers (importSession, signUp, signIn, completeAuth, publishPaykitProfile, uploadProfileAvatar, deletePaykitProfile, removeContact, clearStateLocked)	bump	bump	untouched

:927 reads previousRevision == null || nextRevision == null || previousRevision != nextRevision, so it skips only when both reads succeed and match. The author's "unreadable fingerprints are handled conservatively" is accurate, and the pre-read case is actually an improvement over master.

What the fingerprint covers, and why a false negative needs a hash collision. It is SHA-256 over the same export_backup_state() payload the wallet backup itself ships (exportBackupString = hex(export_backup_state().export_bytes()), consumed at PrivatePaykitRepo.kt:511 into WalletBackupV1.paykitSdkBackupState). So the only state that can change without moving the fingerprint is state that is not in the backup — operation leases, and rc54's in-memory private_payment_list_publications. Concurrency cannot manufacture a mismatch either: every tracked call holds operationMutex, so the before/after reads are serialized per op.

The rc51 -> rc54 bump, checked against the actual SDK diff rather than the release notes alone (gh pr diff 157 --repo pubky/paykit-rs, plus files at the tag):

• New backupStateRevision() — adopted here.
• PrivateStreamIntakeReport.receive_batch_id became Option<u64>. The app never reads receiveBatchId — verified two ways (git grep at head and a working-tree grep -rn); only .counterparty and .error are read, at PaykitPaymentRequestRepo.kt:961-969. No adaptation needed.
• Empty receives no longer allocate a batch or checkpoint Noise, and refresh_active_identity returns the previous state instead of rewriting initialized_at. Behaviour, not schema.
• Unchanged reservation-backed lists are reused in memory scoped to the link id, so report.queued is empty on reuse. applyPrivatePaykitRepo.applyPrivatePaymentListDeliveryReport only records cache entries for queued/cleared and derives retry keys from them, so an empty queued on an already-cached publish is a no-op. Not broken.
• rc53's fetchPubkyFileBounded is opt-in and unused here.
• No wire/path/schema changes, consistent with the diff — no field removals in storage/records.rs, only additions.

Gating trace, since it decides severity: ContentView.kt:296 starts the polling loop unconditionally on ON_START, so the timer runs for everyone. The work inside is gated — inbox refresh returns early at AppViewModel.kt:807 unless isPaykitEnabled (default false, SettingsStore.kt:42), a public key exists and a wallet exists; the maintenance call is gated inside canPublishPrivateEndpoints() on sharesPrivatePaykitEndpoints (default false), which is pre-existing and whose 30/60/120s cadence is unchanged. So: affects opted-in users on released builds.

Upgrade path from the shipped releases. v2.4.1 (published Latest) pins 0.1.0-rc31 and v2.5.0 pins 0.1.0-rc51, verified with git cat-file -p "<tag>^{commit}:gradle/libs.versions.toml". export_backup_state is the same function those releases already ran for every wallet backup and tolerates identity_state: None; rc54 makes no persisted-schema change. If the first post-upgrade read fails anyway, head backs up. Restore-time bumps stay suppressed by shouldSkipBackup() (BackupRepo.kt:134,360), and polling cannot start before state is loaded (walletExists() / isAvailable() / isSetup.await()).

Also checked and clean: the diff adds zero Logger.* calls and touches nothing seed-derived. Faster polling cannot re-present a dismissed request — dismissal marks presentedRequestIds and persists per identity (PaykitPaymentRequestRepo.kt:324-328), automaticPendingRequests() filters on it, and subscription dismissals are re-intersected on every sync (:680). Identity pinning is unchanged: refresh() captures stateGeneration and activeIdentity before taking the lock (:385-386). No reentrancy — the loop is sequential, refresh() holds operationMutex, there is a single job with an isActive check at :828, cancelled on ON_STOP. The 120s presentation retry is preserved via the new constant at :5444. Conventions are right: runSuspendCatching on both suspend reads, Duration arithmetic, no runBlocking, no System.currentTimeMillis().

Not re-raising greptile's point about the fingerprint branches being untested — I agree with it given the stakes, and it is already on record.

Could not verify: whether uniffi-Kotlin cancellation actually drops the Rust future rather than detaching it onto an internal runtime. If it detaches, a cancelled op could write after the finally read. The Rust storage layer uses synchronous transactions so this is unlikely, but only an instrumented test would settle it. I also did not inspect the binary AAR, so the exact Kotlin signature of backupStateRevision() is inferred from the committed Swift binding plus the fact that this compiles.

[jvsena42]

Low, performance not correctness: this turns each tracked SDK op into two full backup exports instead of two cheap revision reads.

rc51's state_revision() was load_state_blob().map(|s| s.revision). rc54's backup_state_revision() is export_backup_state() -> serde_json::to_value -> sort_all_objects -> to_vec -> Sha256 (paykit-ffi/src/sdk.rs:187-198 at v0.1.0-rc54). Since withStateRevisionTracking reads it before and after the block, every tracked op pays two of those, and each 5s inbox tick makes two tracked calls — so 4 full exports every 5 seconds while foregrounded, at a cost proportional to total state size (all private_stream_items, receipt_records, and so on).

Correctness is fine and the PR body already disclaims battery, so this is a note rather than a request. If it ever shows up in profiling, the cheap fix is a state_revision pre-check and only falling through to the fingerprint when the storage revision actually moved.

[ben-kaufman]

Agreed, this is a real CPU tradeoff, not a battery-saving guarantee. A raw revision pre-check could help some unchanged operations, but linked-peer receives still claim and release operation leases, so their raw revision moves even with no incoming message. Those receives still need the content fingerprint to distinguish real backup changes.

Keeping this PR focused on correct backup decisions. If large-state profiling shows this cost is significant, SDK-side fingerprint caching is worth evaluating separately, while preserving partial-failure and cancellation coverage.

[jvsena42]

Accepted — and your reason kills my suggested optimisation, which is the useful part. A raw state_revision pre-check would not help the case that actually dominates here: linked-peer receives claim and release operation leases, so the raw revision moves on every tick even with an empty inbox, and only the content fingerprint can tell that apart from a real change. So the pre-check would add a branch that almost never short-circuits on the hot path. Withdrawn; nothing to do here.

[jvsena42]

Addendum — f030603a3 landed while I was posting the review above. It closes the test gap, no new findings. My two LOW notes still stand as written; both are about behaviour this commit did not change.

The refactor extracts withPaykitBackupStateTracking as a top-level suspend function over readRevision/onChange/operation, which is what makes it testable without a live SDK. The predicate is carried over unchanged — previousRevision == null || nextRevision == null || previousRevision != nextRevision — and withContext(NonCancellable) is preserved around the post-read, so the enumeration in my review still holds line for line.

PaykitBackupStateTrackingTest covers the decision in both directions: all four (before, after) combinations including both unreadable variants, partial failure preserving the original error via assertSame, and cancellation after mutation.

The cancellation test is the one worth calling out — it is stronger than the iOS twin's. It launches a real coroutine, mutates, then cancelAndJoin()s, with a yield() inside readRevision. That yield() is the load-bearing part: in a cancelled coroutine it would throw if the post-read were not inside withContext(NonCancellable), so the test actually proves the NonCancellable wrapper does its job rather than just asserting the happy path. That is the property I most wanted covered here, since it is what keeps a cancelled-mid-write operation from silently skipping its backup.

For cross-repo reference, the iOS twin landed the equivalent suite in the same window (synonymdev/bitkit-ios#748, PaykitBackupStateTrackingTests.swift), with the same four-case table and the same partial-failure case. The two platforms now express the same rule in the same shape, which is the right outcome for a pair of PRs that have to agree about when a wallet backup is owed.

[ben-kaufman]

The staging pubky_paykit job failed before starting the emulator or running Bitkit. sdkmanager could not parse/download the Android repository metadata and then reported that system-images;android-33;default;x86_64 could not be found. This is runner/SDK setup, not a failing app assertion. Build, lint and detekt passed.

I tried rerunning just that job, but GitHub rejects the retry while the parent workflow is still running. It needs a retry once the remaining matrix jobs finish.