fix: clean up deleted private contacts by ben-kaufman · Pull Request #830 · synonymdev/bitkit-ios

ben-kaufman · 2026-09-29T14:29:34Z

Closes #838

This PR fixes private payments remaining available after a contact is deleted and contacts reappearing after a successful deletion.

Android companion: synonymdev/bitkit-android#1372

Description

Revokes every known private receiver when deleting a contact so existing links cannot continue delivering payment requests.
Restores private connections before saving an explicit re-add or import, so a failed restoration can be retried.
Prevents background receiver refreshes from recreating deleted contacts and retries stale list snapshots so unrelated saved contacts remain visible.
Attempts to withdraw shared private endpoints while the link is still usable, then blocks all known receivers even if delivery fails. Post-block cleanup skips network delivery.
Rechecks peer authorization for cached approvals and before the first payment dispatch, cleaning up prepared proofs if the peer was blocked.
Requires active subscriptions with the contact to end before deletion and explains this in the delete error toast.

Out of Scope

Wallet Activity remains unchanged; Payment Requests visibility follows the SDK's existing blocked-peer behavior.
SDK protocols and cancellation of payments already dispatched are unchanged.
Paykit has not launched yet, so no backward compatibility or migration is needed.

Design

N/A — no design available. Adds an error message to the existing contact-deletion toast.

Preview

VIDEO_1

QA Notes

Journeys

new delete-and-readd-contact.xml — deletion survives refresh/restart, private requests stop, and explicit readd reconnects.
new delete-contact-with-active-subscription.xml — deletion requires the subscription to end, and re-add does not revive a canceled subscription.

Both journeys are mirrored on iOS and Android and have not been run locally.

Manual Tests

N/A

Automated Checks

added PaykitContactLifecycleTests.swift — receiver revocation, withdrawal before block including delivery failure, failed re-add retry, and subscription deletion guards for payer/payee, cancellation and expiry.
updated ContactsManagerTests.swift — invalidated initial loads preserve unrelated contacts and reset stops a superseded load.
updated PaykitPaymentRequestServiceTests.swift — blocked requests are not payable, cached approvals are rechecked without repeating acceptance, and subsequent history refresh matches SDK visibility.
ran 204 focused simulator tests covering contacts, requests, private endpoint cleanup, proofs and send confirmation. Re-ran lifecycle tests after extending subscription coverage to both roles. Ad-hoc simulator signing and isolated checkouts use the existing pinned dependency versions.

Deletion can still wait for existing SDK operations or endpoint-withdrawal network timeouts. No end-to-end deletion latency claim is made.

greptile-apps · 2026-09-29T14:33:28Z

Confidence Score: 3/5

[Critical risk]

The PR should not merge until failed re-adds remain recoverable and already-approved requests cannot be dispatched after their peer is blocked.

Findings

Failed re-add can strand contact ▶
Approved requests bypass blocking ▶
Skipped cleanup loses retry ▶

Summary

The PR blocks known receiver paths before contact deletion, limits private-link restoration to explicit add/import, guards stale contact loads, and filters blocked peers from actionable payment requests. It also adds lifecycle tests and a two-wallet journey.

Re-add needs to recover from a record saved before peer unblocking fails.
An already-approved payment needs a block check before dispatch.
Skipped private-list delivery should retain any outstanding remote-cleanup obligation.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  D[Delete contact] --> B[Block known receiver paths]
  B --> R[Remove SDK contact record]
  R --> C[Clean private endpoint state]
  A[Explicit add or import] --> S[Save contact record]
  S --> U[Unblock peer paths]
  P[Payment request] --> F[Filter blocked peers on refresh]
  P --> G[Check block state during preparation]

_{Reviews (1) · Last reviewed commit: "fix: clean up deleted private contacts"}

greptile-apps · 2026-09-29T14:33:31Z

+            let record = try await sdk.saveContact(update: Paykit.ContactUpdate(publicKey: publicKey, receiverPaths: contactPaths, label: label))
+            if restorePrivateConnection {
+                for peer in try await sdk.linkedPeers() where peer.state == .blocked && PubkyPublicKeyFormat.matches(peer.counterparty, publicKey) {
+                    _ = try await sdk.unblockPeer(counterparty: peer.counterparty, counterpartyReceiverPath: peer.counterpartyReceiverPath)


Failed re-add can strand contact If linkedPeers() or unblockPeer() throws during an explicit re-add, this code has already saved the contact record but reports that adding it failed. After Contacts reloads that record, another add is rejected as a duplicate, so the user cannot retry restoring the blocked private connection through the add flow.

Knowledge Base Used: Contacts and Pubky identity

Fixed by restoring blocked receivers before saving the contact. If peer lookup or an unblock fails, the contact is not persisted, so retrying the explicit add works. The lifecycle regression covers lookup failure and partial unblock failure.

Commit: 75e8c9c.

greptile-apps · 2026-09-29T14:33:31Z

                markApprovedForPayment: true,
                preservePending: !request.requiresAcceptance
            ) {
+                try await service.ensurePaymentAllowed($0)


Approved requests bypass blocking If a request is approved and its peer is then blocked while the send sheet remains open, the send flow skips prepareForPayment, which is the only place this new check runs. Confirmation can therefore dispatch payment to the blocked contact without checking again.

Knowledge Base Used: Payment request management

Fixed the cached-approval path: every preparation checks current peer blocking without repeating acceptance or private-list consumption. Lightning, LNURL, onchain and hardware flows also check after preparation before their first send, and remove the prepared proof when authorization is denied. Added cached-approval regression coverage.

Commit: 75e8c9c.

greptile-apps · 2026-09-29T14:33:31Z

+                    guard let report = try await PaykitSdkService.shared.clearPrivatePaymentList(to: publicKey, receiverPath: receiverPath)
+                    else { continue }


Skipped cleanup loses retry If blocking leaves a previously published private list in place, this continue treats its skipped remote clear as successful cleanup. The app then erases the local publication state and retry marker, so it has no cleanup work left to perform before an explicit re-add unblocks the peer.

Knowledge Base Used: Paykit payment modes

This is not conditional. PaykitSdkService.removeContact blocks every known receiver path before ContactsManager calls removeSavedContact, so clearPrivatePaymentList returns nil on every deletion, and the deleted contact keeps the last private payment list we sent them. Before this PR the clear was delivered. Keeping the cleanup pending would not help either: blockPeer drops the encrypted link, and the peer stays blocked until an explicit re-add. The clear needs to run before the paths are blocked.

You’re right — I missed the ordering across the two services. ContactsManager.removeContact invokes PaykitSdkService.removeContact before PrivatePaykitService.removeSavedContact, and the former blocks every known receiver path. Therefore, clearPrivatePaymentList necessarily returns nil during this cleanup; retaining the retry marker would not make it deliverable while the peer remains blocked.

The cleanup must occur before the SDK contact removal/blocking (or otherwise explicitly clear the list before blocking). Given that ordering, the continue here is intentional, and my “skipped cleanup loses retry” comment does not apply. Retracting it.

Correct about the ordering. Deletion now attempts empty-list delivery to linked receivers inside the existing SDK operation lock before blocking them, so another publication cannot run between withdrawal and revocation. A delivery failure still allows local blocking and contact removal. Post-block cleanup continues to skip network delivery. Tests cover the ordering and failed withdrawal. This preserves the withdrawal attempt, but it cannot retract invoices already shared and can still wait on the SDK network timeout.

Commit: 75e8c9c.

Resolved. The best-effort clear now runs before blockPeer under operationLock. When it cannot be delivered, the contact keeps the last list, which is the accepted limitation.

jvsena42

Two MEDIUM and one LOW inline, all shared with synonymdev/bitkit-android#1372. I also replied on the cleanup thread: the remote private-list clear is skipped on every deletion. Paykit is on by default on master since #818 (unreleased).

Checked and clean:

removeContact/saveContact run under operationLock, so block → remove → unblock cannot interleave.
SDK handshakes reject blocked peers, so a racing link burst cannot re-link.
The detached delete task is not cancelled with the view.
contactsRevision discards a stale load.
The restorePrivateConnection || existing != nil guard stops background refreshes recreating a deleted contact, and every explicit add/import passes true.
Public markers cannot fire under .localOnly.
Blocked state is scoped per identity.
A partial block failure is recoverable by retrying the delete, as the new test asserts.

jvsena42 · 2026-09-29T14:51:00Z

        }
    }

    func removeContact(publicKey: String) async throws -> Paykit.ContactRecord? {


Contacts deleted before this change stay linked and keep presenting payment requests after upgrade.

On v2.5.0, removeContact only called sdk.removeContact, which deletes the contact record (contacts.rs:71) and leaves the peer Linked. receive_private_messages_from_linked_peers pulls from every linked peer whether or not a contact exists, and synchronize() hides only .blocked peers (PaykitPaymentRequestService.swift:533). Nothing on the load path (ContactsManager.loadContacts → pruneUnsavedContactState) blocks them. The changelog claim does not hold for anyone who deleted a linked contact before this release. Affects opted-in users on released builds.

Fix: after a successful full load, run a one-time reconciliation that blocks every linkedPeers() entry whose counterparty has no contact record.

Same in synonymdev/bitkit-android#1372.

Paykit has not launched yet, so no backward compatibility is needed. We are not adding an orphan-peer reconciliation, migration flag, or upgrade cleanup for pre-launch state.

jvsena42 · 2026-09-29T14:51:00Z

            PaykitPaymentRequest(historyRecord: $0, now: synchronizationDate)
        }
-        let subscriptions = records.compactMap { PaykitSubscription(record: $0) }
+        let subscriptions = availableRecords.compactMap { PaykitSubscription(record: $0) }


Deleting a contact hides an accepted subscription instead of ending it, and re-adding the contact re-presents every missed period.

Once the peer is blocked, the subscription drops out of subscriptions, so cancel() throws requestUnavailable (1447). The SDK also refuses cancel on a blocked peer, so this cannot be fixed by cancelling afterwards. subscriptionAcceptedAt (1738-1743) is never pruned, while dismissedSubscriptionPaymentIds.formIntersection(activeRecurringRequestIds) (1755) drops the hidden subscription's dismissals.

After an explicit re-add unblocks the peer, requests(through:acceptedAt:) regenerates every unpaid period since the original acceptance, including dismissed ones, and each is presented. Nothing pays without a confirm. Before this change the subscription stayed visible and cancellable.

Fix: cancel (or warn about) the counterparty's active payer subscriptions before blockPeer, and move the acceptance anchor to the re-add date on unblock.

Same in synonymdev/bitkit-android#1372.

Deletion now refuses to proceed while this contact has an active subscription, whether we are payer or payee, and shows "End active subscriptions before deleting this contact." The check runs before endpoint withdrawal or blocking. Canceled or naturally ended subscriptions allow deletion. This keeps the existing cancellation and in-flight proof safeguards intact. Covered in PaykitContactLifecycleTests.swift and the mirrored subscription-deletion journey.

Commit: 75e8c9c.

Works. One LOW refinement: the guard counts fixed-term subscriptions whose endsAt is in the future (PubkyService.swift:659), but canCancel(at:) returns false whenever recurrence.endsAt != nil (PaykitSubscription.swift:478). A payer who accepted a fixed-term request can neither cancel it nor delete the contact until the term ends, while the refusal tells them to end the subscription. Bitkit creates requests with endsAt: nil, so this needs a proposal from another Paykit client, and isProposalActionable(at:) accepts those. Either allow cancelling fixed-term subscriptions or name the end date in the refusal. Same in synonymdev/bitkit-android#1372.

Added the fixed-term end date to the deletion refusal. Open-ended subscriptions keep the cancel-first message.

jvsena42 · 2026-09-29T14:51:00Z

+        )
+        await manager.refresh()
+        XCTAssertTrue(manager.pendingRequests.isEmpty)
+        XCTAssertEqual(manager.historyRequests.map(\.paymentRequestId), [record.paymentRequestId])


This asserts history keeps the blocked contact's request, which production cannot do.

SDK paymentRequests() is "across non-blocked counterparties" (payment_request_counterparties, payment_requests.rs:203-210), so deleting a contact removes its requests from history too. This passes only because the mock does not apply that filter. The journey step "verify 'Before deletion' has no available Pay action" (delete-and-readd-contact.xml:11) passes trivially because the row is gone.

Fix: make the mock drop blocked counterparties and assert history no longer contains the request, and reword the journey step to "is no longer listed". Or keep history for deleted contacts on purpose. Same in synonymdev/bitkit-android#1372.

Correct about normal SDK visibility. I updated the test to cover both timings: an earlier snapshot cannot leave a blocked request payable, and the next SDK snapshot removes it from history. The journey now waits for refresh and asserts the request is no longer listed. No separate history storage was added.

Commit: 75e8c9c.

Resolved in 75e8c9c.

jvsena42

Re-checked 75e8c9c and 171d449.

Open: one LOW, posted as a reply on the subscription thread. Fixed-term subscriptions block deletion but cannot be cancelled.

Resolved: the test/journey history thread and the private-list clear ordering.

Checked and clean:

The activeSubscription refusal propagates unwrapped, so the toast shows. (On Android it does not; see synonymdev/bitkit-android#1372.)
The re-add rollback re-blocks every originally blocked path on any failure, including cancellation, and rethrows the original error.
The cached-approval path re-checks through prepareForPayment for Lightning, LNURL, on-chain and HW, and cleans up the proof on denial.
The ContactsManager load loop prunes only after a successful load, and the generation guard stops a stale load from clearing isLoading.

ovi-reviewer

Advice: ✅ Approve

Review: diff 17 files.
The paired Android PR differs; see the parity finding.

Findings:
3 inline (2 MEDIUM, 1 LOW)

QA:
Tests queued.

Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)

ovi-reviewer

Verdict: ⛔️ Request Changes

Reaudit: diff 5 files.
New findings: 1 inline (1 MEDIUM); the rest is in the review.
Android PR #1372 diverges; see the parity finding.

Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)

ovi-reviewer · 2026-09-30T02:11:00Z


-            isBroadcastUnresolved = true
            do {
+                try await beforeBroadcastAttempt()


MEDIUM: Android hardware retries still skip the peer authorization check.

This per-attempt check closes the iOS retry gap. The paired Android PR still calls beforeBroadcast only when its cached payment is not prepared, and that callback holds its peer check. After an Electrum failure, Android can reuse the signed transaction and broadcast after the contact is blocked. Could we move the Android peer check before every hardware broadcast attempt and add the same blocked-retry regression there?

The iOS retry path is fixed here. The Android hardware retry change belongs in synonymdev/bitkit-android#1372, so I left this iOS branch unchanged.

ovi-reviewer

Advice: ✅ Approve

Reaudit: diff 5 files.
No new findings; the rest is in the review.

QA:
Tests queued.

Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)

fix: clean up deleted private contacts

67362eb

ben-kaufman mentioned this pull request Sep 29, 2026

fix: clean up deleted private contacts synonymdev/bitkit-android#1372

Open

2 tasks

greptile-apps Bot reviewed Sep 29, 2026

View reviewed changes

jvsena42 requested changes Sep 29, 2026

View reviewed changes

ben-kaufman added 2 commits September 29, 2026 10:16

fix: harden contact deletion and payment authorization

75e8c9c

fix: roll back failed contact re-add

171d449

ovitrif mentioned this pull request Sep 29, 2026

bug: deleted contacts keep private payments and can reappear #838

Open

jvsena42 requested changes Sep 29, 2026

View reviewed changes

fix: show fixed subscription end date

8d62210

ben-kaufman requested review from jvsena42 and ovitrif September 29, 2026 21:00

ovi-reviewer Bot reviewed Sep 30, 2026

View reviewed changes

Comment thread Bitkit/Views/Wallets/Send/SendSheet.swift Outdated

Comment thread Bitkit/Views/Wallets/Send/SendConfirmationView.swift Outdated

Comment thread Bitkit/Views/Contacts/ContactDetailView.swift

fix: recheck hardware send authorization on retry

a182bae

ovi-reviewer Bot suggested changes Sep 30, 2026

View reviewed changes

Merge master into fix/delete-private-contact

42a55f1

ovi-reviewer Bot reviewed Sep 30, 2026

View reviewed changes

		guard let report = try await PaykitSdkService.shared.clearPrivatePaymentList(to: publicKey, receiverPath: receiverPath)
		else { continue }

Conversation

ben-kaufman commented Sep 29, 2026 • edited by ovitrif Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Out of Scope

Design

Preview

QA Notes

Journeys

Manual Tests

Automated Checks

Uh oh!

greptile-apps Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 3/5

Findings

Summary

Diagram

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvsena42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvsena42 left a comment

Choose a reason for hiding this comment

Uh oh!

ovi-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ovi-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ovi-reviewer Bot Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovi-reviewer Bot left a comment

ben-kaufman commented Sep 29, 2026 •

edited by ovitrif

Loading

greptile-apps Bot commented Sep 29, 2026 •

edited

Loading