Repository navigation
fix: pause paykit work during profile deletion - #954
Conversation
 [Medium impact] The PR appears safe to merge; no actionable issue was found. SummaryPauses automatic Paykit work while profile deletion runs, then resumes it if the profile remains signed in.
Acknowledged scope: Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Begin profile deletion] --> B[Pause automatic Paykit work]
B --> C[Run explicit cleanup]
C --> D{Profile still signed in?}
D -->|Yes| E[Resume automatic work and retry retained contact link]
D -->|No| F[Keep automatic work stopped]
B --> G[Allow admitted work and payment proofs to finish]
Reviews (1) · Last reviewed commit: "fix: pause paykit work during profile de..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Gates automatic Paykit work (contact refresh, Shop import, request intake and presentation, endpoint publication, reconciliation) on a new automaticPaykitSession that is nil while a profile deletion runs. The same gates hold for awaited work rechecked through canRefresh. Explicit proof refresh and the deletion's own cleanup keep currentSession. Reviewed 1ae24c1, full tier.
What I checked, and 6 candidates I ruled out
Read: the full diff, plus PubkyProfileManager session and sign-out paths, PrivatePaykitService deletion and reconciliation, PublicPaykitService.syncCurrentPublishedEndpoints, PaykitPaymentProofRefreshState, MainNavView deep-link preparation, and the three new tests
Call sites traced: retryPendingPaykitEndpointRemoval, refreshIncomingPaykitPaymentRequests, prepareSavedPaykitContacts, handlePendingPaykitSubscriptionNotification, syncReceivePublicPaykitEndpoints
CI: validate and Greptile green. Run Tests, Run Integration Tests and build-local were still pending when I reviewed. iOS does not build on this reviewer's Linux host, so this review is reasoned from the code and CI.
Ruled out
- Resume burst after a successful deletion: the
isDeletingProfileonChangehandlers fire then too. Every path they call gates onautomaticPaykitSession, which is nil once sign-out has cleared the auth state. - A rejected refresh leaving a stale
refreshTask: the task now clears itself in adeferwhencanRefreshrefuses it.testAutomaticRefreshPausesWithoutCancellingAdmittedWorkAndResumescovers both admitted and queued work. - Polling republish using
pubkyProfile.publicKeyinstead ofsession.publicKey(AppScene.swift, full-mode round): the only call between the top-of-loopautomaticPaykitSessionguard and the republish is the synchronousstartShopContactImport(), so the identity is the same. - Proof refresh stalled by deletion:
automatic: falsecompares againstcurrentSession, which deletion keeps until sign-out's session mutation. - Endpoint reconciliation skipped at launch while session restore runs:
retryPendingPaykitEndpointRemovalnow returns until the profile is authenticated. TheauthStateonChangereruns it when the UI flag is off, and the UI-on paths run after foreground sync. - Deep-link retry loop after deletion:
retryPendingContactAfterDeletionis cleared whenhandlePendingDeepLinkstarts. It survives only when that call bailed on an in-flight preparation, so theisPreparingPendingContactDeepLinkhandler fires at most one retry.
Merge confidence: 4/5, no findings, but the unit-test and build jobs had not finished and the branch was not built here.
|
Historical The failing test and SendConfirmationView are unchanged by this PR. The exact cause is still unproven, and this specific failure has not been reproduced on the base. Update: Ben's attempt 2 passed on unchanged head Update on The test, Lightning/Blocktank services, workflow and dependency lock match actual master. That is not proof the failure is unrelated: current master The Current Update on unchanged The test and SendConfirmationView are byte-identical to actual master Ovi independently started unit attempt 2 at 22:20:18 UTC; its subsequent failure is investigated below. No additional retry was dispatched here. Actual E2E job 117191824764 and required e2e-status 117240386702 subsequently passed at 23:17 UTC. Build and translations passed. The current-head integration job passed on its second internal attempt: the first had 34 tests/one failure in Update on unchanged The complete 6,248-line log, annotations, exact retry artifact and exported diagnostics were inspected. Xcode confirmed launch completion at 22:46:11.913 and reported the app in the foreground. XCTest requested its assertion at 22:46:11.923; testmanagerd received it at 11.928. SwiftUI view-appearance events arrived at 19.338. XCTest's 10-second assertion wait expired at 21.937. The following launch-performance test explicitly terminated that process at 22.837; RunningBoard only logged the developer-testing assertion acquisition at 24.826, then rejected the now-absent process. That later rejection is not evidence of an app crash causing the original timeout. The next launch acquired its assertion in about 2ms, and the launch-performance test passed. This narrows the observed failure to delayed test-manager/RunningBoard assertion handling, but does not establish why it was delayed or rule out a shared-run contribution. The launch test and unit workflow are byte-identical to actual master |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 1ae24c1.
No new actionable code findings.
Automatic contact refresh, Shop import, request intake and presentation, and endpoint publication stay paused for the whole profile deletion, then resume only while that signed-in session remains. Admitted refreshes, explicit cleanup, and payment-proof completion keep running. A local receive invoice still commits when public publication is deferred. A contact link that arrives during deletion stays pending and is retried once preparation is free, including read-only routing after sign-out.
Compared with bitkit-android#1510 at bc00345 for the shared pause and resume contract. Android fails incoming request refresh closed for the entire deletion; this PR still allows explicit proof refresh and adds the contact-link retry. Journey actions match. Interrupted recovery and a maximum deletion duration remain out of scope, as stated on the PR.
Validation: the new tests in PubkyProfileManagerTests, PaykitPaymentRequestServiceTests, and ReceiveInvoiceRefreshTests were inspected and not executed here. On this revision, Run Tests, Run Integration Tests, e2e-tests, and Validate Translations passed. ovi-reviewer was still pending.
Device testing: not performed in this review.
Ready for device testing
Suggested additional test cases
- iOS, Paykit UI enabled, disposable signed-in profile. Receive a contact deep link and complete profile deletion successfully before that link is routed. Expected: onboarding is shown, the link is kept, and it then opens through read-only contact routing once preparation finishes, with no repeated error.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ⛔️ Request changes. A contact link opened during a deletion that succeeds ends in an error toast (Test 6).
Findings:
- MEDIUM
Bitkit/MainNavView.swift:437: when the deletion succeeds, this retry runs signed out.loadContactsIfNeeded(for: contactPublicKey)fails withPubky shared state requires an active session, andshowScanError()shows "Unable To Read QR" on the profile onboarding screen. The contact never opens, though the description promises read-only profile routing. On master the same link opened during a deletion shows the contact page with no error. The signed-out load fails on master for any contact link too; this retry is the new path into it. - LOW
Bitkit/ViewModels/WalletViewModel.swift:1590: a paid-invoice replacement skipped during deletion gets one later attempt and no retry. That attempt is the resumeTaskinAppScene.swift:519-526. It runs once, after a contacts refresh, and only logs a failure. When a deletion fails on a homeserver error and that attempt fails too, the paid invoice stays published until the next foreground, channel event or payment.
QA for 1ae24c1:
Tested on an iPhone 17 simulator (iOS 26.5) with regtest wallets and staging Pubky profiles.
Test 1: delete-profile journey, one contact ✅ Deletion showed progress, reached profile onboarding in 20 s, and the profile stayed gone after reopening Profile and after a relaunch (UI snapshot).
While deletion ran, the screen showed a spinner and had no tappable control. Afterwards the Home header read "Profile" with no name.
t1-delete-profile-journey.2x.mp4 |  |  |
Test 2: delete-profile journey, 62 contacts after preparation settled ✅ Deletion reached profile onboarding 18 s after the confirm, and reopening Profile did not bring the profile back.
The 62 contacts were saved by link: 61 keys with no homeserver plus one real staging profile, not a Ring import. Deletion was confirmed nearly 5 min after the last save. The spinner showed while it ran, and the app log has no safety-wait line.
t5-62-contacts-settled.2x.mp4 |  |  |
Test 3: delete-profile journey, 61 contacts right after saving ✅ Deletion reached profile onboarding 15 s after the confirm, and reopening Profile did not bring the profile back.
The contacts were saved the same way on a recreated profile; one of the 62 keys timed out in the profile lookup before Save. Deletion was confirmed 1 min 43 s after the last save, while contact preparation was still running, and the app log has no safety-wait line.
t3-62-contacts-immediate.2x.mp4 |  |  |
Test 4: automatic Paykit work paused during deletion ✅ While the deletion's first request was held, a background and foreground and a contact link started no new Paykit work; admitted work finished and deletion reached onboarding.
The homeserver was held at the network for the first 14 s of the deletion, with open connections closed first so any new request had to reconnect.
- No new homeserver connection opened (connection log).
- The app log shows no contact load until the deletion ended.
- A reconcile admitted before the confirm finished after the release.
On master, the same foreground during a held deletion started a contact load within a second, and a later foreground started a Shop import (master's app log).
t2-automatic-work-paused-during-deletion.2x.mp4 |
Test 5: failed deletion resumes work and retries the contact link ✅ After a failed deletion the identity stayed signed in, automatic work restarted at once, and the contact link was retried once and later opened the contact.
The contact link's preparation was held, deletion started, and the homeserver was then made unreachable.
- Cleanup waited for the admitted preparation and failed only after the preparation timed out at 30 s. So the order in this PR's manual test (cleanup fails first) did not occur.
- "Unable to Delete Profile" showed.
- Contact refresh, Shop import, endpoint refresh and request intake restarted within 110 ms (app log).
- The link was retried once, with one toast and no loop.
- With the network back, the kept link opened the contact at the next foreground (second video).
t3-failed-deletion-resumes.2x.mp4 |  | t3-retained-link-routes.2x.mp4 |
Test 6: contact link opened during a deletion that succeeds ❌ The link was kept, but after the deletion reached onboarding its retry showed "Unable To Read QR" and the contact never opened.
This is the case @piotr-iohk suggested in their QA review. The link was a saved contact's, opened 9 s into the deletion (Test 4's run, at its end). The log shows Failed to load contacts before routing contact link: ... Pubky shared state requires an active session. On master, the same link opened during a deletion showed the contact page with no error (second video), and the deletion then reached onboarding (UI snapshot).
 | t4-master-link-during-deletion.2x.mp4 |
Full review of 0ea3fcc..1ae24c1. No device case drove a payment during deletion, so the receive-invoice publish deferral (the LOW finding) was checked in code only.
Pair PR synonymdev/bitkit-android#1510 matches on the pause and resume: both defer automatic work during deletion, invalidate a paid public invoice locally and publish its replacement on resume. It differs in two places. Android also refuses incoming-request refresh for the whole deletion, where this PR still allows explicit proof refresh, as @piotr-iohk's review notes. The contact-link retry is iOS-only.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at f6cc033, descendant of 1ae24c1, with the same comparison base and merge base 0ea3fcc. Inherited the completed baseline review for deletion pause and begin/end pairing, Shop import, polling, request intake and presentation, payment-proof refresh and receive-invoice deferral, unchanged tests and journey notes, and Android pause/resume parity from bc00345. This pass reviewed the follow-up delta: signed-out contact routing, cleanup-pending acknowledgement, resume ordering, the new tests and notes, and Android publication fences at eacdc265.
No new actionable code findings.
The two concerns in the changes-requested review are addressed in this source. After a successful deletion, a retained contact link skips the private contact read and opens Add Contact. A paid public invoice cleared during deletion stays pending, resumed publication runs before contact refresh, and a failed foreground publish remains pending for the existing maintenance poll. An admitted write leaves that flag set when its Lightning invoice is no longer current. That is source analysis; the simulator timing from the earlier review was not repeated on this commit.
Device testing: not performed in this review. Simulator evidence on the changes-requested review is for 1ae24c1 only.
Recommended before device testing: Run Tests and Run Integration Tests were still pending on this commit. validate had passed. This review did not execute the Swift tests. The author reports a local pass on the fix thread; that report is not execution by this review.
Android #1510 at eacdc265 uses the same acknowledgement rule, and both journey notes treat contact-link retry as iOS-only. Android also stops an in-flight public publish inside its mutex once deletion has started. iOS foreground refresh and reconciliation pass a session fence; receive and channel refresh still check deletion before the call. That pre-call check predates this follow-up.
|
Cross-platform follow-up in b368a3b: receive and both channel-publication routes now retain the starting session and recheck identity, deletion, cancellation and sharing after preparation, inside the existing endpoint lock. This closes the pre-lock gap where deletion could remove endpoints before delayed preparation republished them. Already-admitted writes still finish before cleanup; proof/bookkeeping and safety waits are unchanged. iOS already serializes SDK-queued publication with removal through that outer lock. Three table-driven regressions reproduced the stale-publication/admission failures before the fence. Final 2,627 native tests and 39 focused tests pass. Full formatting matches the 57-finding master baseline; translations are 0 warnings/errors; no introduced compiler diagnostics. Independent review is clean, and all four PR commits are signed and GitHub-verified. Current master, including the merged backup fix, is included. Fresh CI is pending. No new device/payment acceptance or general recovery-latency result is claimed; the controlled manual cases remain unchecked in the body. |
There was a problem hiding this comment.
Verdict: ⛔️ Request changes. A saved contact's link opened while the session restore has failed opens Add Contact, and Save then fails (Test 1).
The PR moved to b368a3b after this run; these results are for f6cc033.
Findings:
- MEDIUM
Bitkit/Utilities/MainNavRouting.swift:20:preparePubkyContactLinkandroutePublicContactIfSignedOut(:41) treat!isAuthenticatedas signed out. A stored identity whose session restore failed or was deferred also has nopublicKey.- Its saved contact's link then opens Add Contact as a new contact. The link is already consumed, so nothing re-routes after the restore.
- Save fails while offline, and after the restore it shows "Invalid pubky key format".
- On master the same link shows "Unable To Read QR", stays pending and opens the contact page once the session restores.
testSignedOutContactLinkDoesNotWaitForPrivateContactsasserts.addContactwhile a stored identity restores.- b368a3b leaves this routing and the session restore unchanged, so this still applies there.
Both findings of the earlier review are fixed in code, as @piotr-iohk's review also reads them:
- After a successful deletion, the kept contact link skips the private contact load and opens Add Contact.
- A paid public invoice sets the pending flag, and the maintenance round publishes a new invoice and acknowledges only a current write.
QA for f6cc033:
Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet and a staging Pubky profile.
Test 1: saved contact's link while the session restore has failed ❌ The link opened Add Contact for a contact already saved, and Save failed before and after the session restored.
The wallet had one saved contact. The app was cold-launched with the homeserver unreachable, so the restore failed (app log) while Home still showed the profile name (second video, same state on master).
- Opening the contact's link showed Add Contact with Save.
- Save showed "Failed to add contact" while offline.
- With the network back, the session restored and contacts loaded (app log). The screen stayed on Add Contact, and Save showed "Invalid pubky key format".
- On master, installed over the same wallet, the same launch and link showed "Unable To Read QR". The link then opened the contact's page once the session restored (second video).
t4-head.2x.mp4 | t4-master.2x.mp4 |
Not driven at f6cc033, because the PR moved to b368a3b first:
- The contact link kept during a deletion that succeeds (the earlier Test 6).
- A failed deletion resuming work and retrying the kept link (the earlier Test 5). This push changed the resume order and the signed-in link preparation.
- A payment on the public invoice during a deletion that fails, and its republish by maintenance (the PR's manual tests 1 and 3).
Tests 1-4 of the earlier review passed at 1ae24c1: the delete-profile journey with one and with 62 contacts, and the pause of automatic work during deletion. This push leaves the deletion and its pause gates unchanged.
Delta review: diff 9 files. No device case drove the publication acknowledgement in PublicPaykitService.swift or the paid-invoice pending flag in WalletViewModel.swift.
Pair PR synonymdev/bitkit-android#1510 at e8ade7d keeps a paid public invoice's replacement pending the same way. It has no contact-link retry or signed-out contact route, so the finding is iOS-only.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at b368a3b: two LOWs inline. I found no HIGH or MEDIUM.
I reviewed this as a funds, auth and deep-link change (paid-invoice rotation, publication fences, contact link routing).
Checked and clean:
- Paid invoices:
rotatePublicPaykitInvoiceIfNeededclears the local invoice and sets cleanup pending whatever the deletion state (WalletViewModel.swift:1502-1509). Only a write whose desired bolt11 equals the currentpublicPaykitBolt11, with the session still current, clears pending. Receipt attribution is not gated onautomaticPaykitSession. Incoming requests cannot be presented mid-deletion: three checks require the session. - Identity: publication fences compare
SignedInSession(key + revision). Sign-out bumps the revision before taking the endpoint lock. Signed-out routing only pushes.addContactwith a validated key.saveContactand the endpoint reads need an SDK session, so nothing private is read or granted. - Lifecycle:
performProfileDeletionresets withdeferand callsendProfileDeletion()on both branches. The deletionTaskis unstructured, so Back or dismiss cannot cancel it. Try again and Disconnect are fenced by the session revision. A link retained during deletion retries at most once. - Concurrency: every check, then await, then recheck site rechecks after the await. The
PaykitPaymentRequestManager.refreshgeneration guard is correct. No nested endpoint-lock acquisition was added.
Performance-relevant: pause and resume of the poll and publication around the shared endpoint lock. To measure: confirm → onboarding with 1 and 62 contacts against master, SDK admissions during deletion, and the resume burst after a failed deletion.
Device gate (partial): b368a3b — not run yet. Pending: journeys/profile/delete-profile.xml (1 and 62 contacts) timed against master, the failed-deletion resume count, and green runs of both suggested fixes. The device result will follow.
Each LOW can be fixed here or moved to a follow-up issue. Reply on the thread with which one.
|
Device gate (partial): iPhone 17 Pro simulator (iOS 26.5), Debug builds of master
* Master sample 1 idled ~40 s instead of 60 s and was the wallet's first profile. The PR is about 3 s faster than master. Most of the remaining time is the ~23 s between "Deleted all contacts" and sign-out, where the app logs nothing. That is the request count tracked in pubky/paykit-rs#197. Automatic Paykit work lines (publication, refresh, intake, endpoint, maintenance) between the tap and onboarding: 0 on all four runs. Loading state, same on both builds:
The only WARN/ERROR in each window, on both builds: |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR as a full reassessment at d7e0e1f1 because the comparison base changed since the saved assessment. No earlier coverage was inherited. The merge base is eb3156ab.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
The current source still pauses automatic Paykit intake and publication during profile deletion and resumes it for a retained identity. The resolved contact-link and paid-invoice threads match that source (signed-out routing, paid-invoice pending, stored-identity routing). The open channel-refresh thread still matches this revision: discussion. The shared resume contract was compared with Android #1510 at 71d9ef7.
Run Tests succeeded on this commit. The new unit tests were inspected and not re-run here. They do not cover a forced channel refresh skipped during a failed deletion. Device testing was not performed. The author's manual checklist remains unchecked.
Suggested additional test cases
- iOS unit: public sharing is on and a reusable public bolt11 is already published. Begin profile deletion, call
refreshPaykitEndpointsAfterChannelAvailabilityChangedwithforceRefreshLightning: true, then fail deletion so the same session remains. Resume publication withrefreshIfNeeded: false. Expected: the published Lightning invoice is a new invoice, and the cached bolt11 is left unpublished.
Device testing: not performed in this review.
Findings
- [LOW] Retain a forced public Lightning refresh skipped during failed deletion — inline at
Bitkit/ViewModels/WalletViewModel.swift:1452.
|
Device gate (partial):
Both runs show the same overlay with spinner throughout and reach profile onboarding. On reopening Profile, the deleted profile does not reappear. Both runs leave one contact's private endpoint withdrawal pending, and public cleanup is refused by policy at sign-out: Log lines (UTC, idle run)The run right after import logs the same three warnings, plus Master logs the same three warnings in both 62-contact runs, so they predate this PR. With 62 contacts the PR is ~11 s faster than master; most of the gain is in private endpoint removal (PR fails it at +28–30 s, master at +33.5–37 s). Further deletion timing is tracked in synonymdev/bitkit-android#1522 and pubky/paykit-rs#197. |
There was a problem hiding this comment.
Verdict: ✅ Approve. One LOW in this push, and all seven device cases pass at aac9d0b.
Findings:
- LOW
Bitkit/ViewModels/WalletViewModel.swift:1421: the new invoice revision check reports a replaced invoice as a lost session.- Turning on "Enable payments with contacts" runs
refreshPublicPaykitEndpoints. If the public invoice is invalidated while it runs (a payment lands on the cached public invoice, or a channel becomes usable or ready),:1421or:1521throwsPubkyServiceError.sessionNotActive. ContactPaymentsService.setEnabledrethrows it because the change is still current. Settings shows "Error: No active Pubky session" and the setting turns back off, though the session is fine. The rollback also removes the endpoints the payment had just republished.- The foreground, maintenance and channel callers return quietly on
sessionNotActive; the enable path does not. - Reproduced with a payment on the cached public invoice and a local hold that only widens the window between the revision capture and the check (video and toast below). The channel trigger was not driven. Retrying the preparation once when only the invoice revision changed would avoid it.
- Turning on "Enable payments with contacts" runs
t8.2x.mp4 |  |
The forced-refresh LOWs from @jvsena42 and @piotr-iohk at d7e0e1f are fixed at this head: a forced refresh during a failed deletion and one before sign-in both publish a new invoice (Tests 1 and 2).
QA for aac9d0b:
Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet, a staging Pubky profile and channels bought from staging Blocktank. The head build adds local hooks that are not part of this PR. They hold or fail the deletion, hold endpoint preparation, a snapshot before the endpoint lock, a write inside the lock and the session revocation, and log which path consumed a kept refresh. Invoices are named by a short hash of the bolt11 read from the homeserver.
Test 1: forced channel refresh during a deletion that fails ✅ A channel that became ready during the deletion cleared the public invoice, and after the deletion failed the homeserver got a new invoice with no further channel event.
- The homeserver served invoice 7ff43b48, and the deletion was held.
- A new zero-conf channel from Blocktank became ready during the hold. After the delayed forced refresh, the app's stored invoice was empty and cleanup was pending, while the homeserver still served 7ff43b48.
- The deletion then failed ("Unable to Delete Profile") and the profile stayed signed in.
- Within 10 s the homeserver served a new invoice 3113f569, and cleanup pending cleared.
- Checked by reading the homeserver endpoint, the app's stored invoice and flag, and the app log.
t1.2x.mp4 |
Test 2: forced channel refresh before sign-in ✅ The channel became usable while the session restore was failing, and after the restore the publication replaced invoice bdb0d6e5 instead of reusing it.
- Cold launch with the homeserver unreachable: the restore failed three times while the channels reconnected. The app logged that it kept the refresh for the stored identity.
- With the network back, the session restored. The publication that reuses a valid invoice consumed the kept refresh, and the homeserver then served a new invoice 1c106302 with cleanup pending false.
- No channel event happened after the launch. A run without the log lines showed the same replacement.
t2.2x.mp4 |
Test 3: an older endpoint snapshot landing after a paid-invoice replacement ✅ The paid invoice came back on the homeserver for about 24 s, then maintenance republished the replacement with no new payment or foreground.
- A foreground publication's snapshot with invoice eca3ff3d was held before the endpoint lock.
- The LSP paid eca3ff3d, and the replacement 7fbc5152 was published.
- Releasing the snapshot wrote the paid eca3ff3d back, and cleanup became pending again.
- 24 s later the homeserver served 7fbc5152 again and pending cleared. That window is the transient stale write the description of this PR allows for.
t3.2x.mp4 |
Test 4: paid public invoice during a deletion that fails ✅ The first resumed publication failed, and maintenance published a new invoice 58 s after the network returned.
- The LSP paid the published invoice 7fbc5152 while the deletion was held. The app cleared its stored invoice and set cleanup pending, and the homeserver kept 7fbc5152 while the deletion ran.
- With the homeserver unreachable, the deletion failed, and so did the first resumed publication and the next maintenance round.
- With the network back and no foreground or payment, the homeserver served a new invoice 4c6ac3ea and pending cleared.
t4.2x.mp4 |
Test 5: contact payments turned off while publications wait, then on again ✅ Once the change applied, the endpoints were removed and nothing republished them. Turning it back on published new endpoints with cleanup pending false.
- A payment left the receive publication and a maintenance round held before admission. Contact payments were then turned off, and the setting waited for the held maintenance round.
- After the release, the held receive publication, admitted while sharing was still on, published first. The change then removed the endpoints and turned sharing off, and nothing republished them over the next 80 s.
- Turning the setting on again published new endpoints, and cleanup pending stayed false.
t5.2x.mp4 |
Test 6: delayed preparation cannot republish endpoints a deletion removed ✅ The held publication was refused after the deletion removed the endpoints, and the homeserver stayed empty.
- A payment's receive publication was held in preparation. The deletion removed the endpoints, and its session revocation was held.
- After the release, the publication was refused with
sessionNotActive(app log). - The homeserver had no endpoints 14 s later, and still none after the deletion reached profile onboarding.
t6.2x.mp4 |
Test 7: a write already inside the endpoint lock finishes before deletion removes it ✅ The deletion waited 42 s for the held write, then removed the endpoints and finished.
- A payment's publication was admitted and held inside the endpoint lock, and the deletion was then confirmed.
- While the write was held, the homeserver kept its endpoints and the deletion did not finish.
- After the release, the write finished with no publication failure. The deletion then removed the endpoints and reached profile onboarding.
t7.2x.mp4 |
Not driven:
- A late snapshot while contact payments are being enabled, and during a failed enable's rollback (the second half of the seventh manual test). Both go through the same acknowledgement Test 3 drove on the maintenance path; checked in code only.
- The held-restore variant of the sixth manual test: the contact payments setting is hidden until the session restores (
GeneralSettingsView.swift:136).
Carried, not re-driven:
- Tests 1, 2 and 4 of our review of d7e0e1f: the contact-link routing and the kept link's retry, which this push leaves unchanged.
- @jvsena42's delete-profile journey at aac9d0b.
Delta review of d7e0e1f..aac9d0b: diff, 4 production files and 2 test files. No device case drove:
- A kept refresh dropped after sign-out, identity replacement, a new session for the same key or cancellation.
testStartupChannelRefreshCannotTransferToAnotherIdentityOrIneligibleCallercovers it, and it passed on the test simulator. - A successful enable clearing a cleanup flag that was already set, and a rollback that restores sharing on. Checked in code only.
Pair PR synonymdev/bitkit-android#1510 at d46bd6c keeps the forced refresh as a request that the next current publication consumes, for a deletion and for the first sign-in, matching this PR. I found no invoice revision check in its endpoint preparation that would fail an enable.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-approving at b8a3078 ("retry superseded public invoice preparation"), with one LOW inline.
- ovi-reviewer's LOW (
WalletViewModel.swift:1421, the enable failing with "No active Pubky session" on invoice invalidation) is fixed. The retry is bounded (for attempt in 0 ..< 2,:1415) and retries only an invoice-revision mismatch. Session, sharing, UI and cancel changes still throwsessionNotActive, and attempt 1 rejects both deletion flags (:1419,:1422). - No paid invoice is re-advertised. The retry re-reads
hasReusablePublicPaykitInvoiceandcanCommitInvoiceafter every suspension, and the acknowledgement still re-arms pending on a revision or bolt11 mismatch. The cross-identity fences are unchanged.
Device gate: b8a3078 — no re-run needed. This delta changes publication logic only, with no UI or journey change. delete-profile.xml passed 7/7 at aac9d0b and d7e0e1f. perf: at most one extra invoice creation per publication (bounded retry), plus the redundant republish in the LOW.
The LOW can be fixed here or moved to a follow-up issue. Reply on the thread with which one.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at 21a8f59f, covering changes since aac9d0bd against the prior QA review. Comparison base and merge base are unchanged at eb3156ab. This pass reviewed the one-shot public-invoice retry, its rejection for deletion, cancellation, identity or session changes, and disabled sharing or Paykit UI, contact-payment enable and restore, and acknowledgement of the revision captured with the Bitcoin/Lightning snapshot before USDT preparation. Inherited coverage is profile-deletion gating, contact-link retention, payment-request gating, forced public-refresh retention, and the startup comparison with Android PR 1510 at 9fddab92.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
A revision-only supersession retries once inside a single refreshPublicPaykitEndpoints call and no longer fails enable as a lost session. Both publishers acknowledge the revision taken after that snapshot returns and before USDT preparation, so a successful retry does not schedule a redundant republish, and a newer invalidation stays pending, including an empty Lightning invoice. That is the capture point requested on b8a30780.
Android PR 1510 was compared only at 98cab12b for this flow. It keeps a pending Lightning refresh across publication attempts and does not retry invoice creation on a revision change. That is not an iOS defect, and it is not a review of that PR.
Validation is source inspection at this pin. I did not run tests or a device. Hosted Run Tests passed on this commit. Integration was still running and end-to-end was queued. Device testing: not performed in this review. Simulator results cited on the PR remain historical for aac9d0b and earlier commits.
Findings
- [LOW] Enable accepts a second invoice supersession — inline at
Bitkit/ViewModels/WalletViewModel.swift:1415.
There was a problem hiding this comment.
Re-approving at 417bcca. No findings. This round was reviewed on Opus because Fable credits ran out.
- My LOW is fixed in
21a8f59. Thread replied. 417bccamakes onerefreshPublicPaykitEndpointscall perbuildWalletEndpoints, forced whenrefreshIfNeeded && includeLightning(:582-583), instead of a forced call followed by a second snapshot call with its own retry budget. That fixes @piotr-iohk's "enable accepts a second supersession": a second invalidation inside the single retry throwspublicationFailedwith bolt11 cleared and pending set, and enable rolls back (testForcedPublicationUsesOneInvoiceRetryBudget,testEnablingContactPaymentsRetriesInvoiceSupersessionOnce).- No paid invoice is reused. Attempt 1 still forces a new one, and a payment after the commit bumps the revision so the ack keeps pending. No shared state across sessions was added, the
canCommitInvoiceguards are unchanged, and no locks were added. - Parity: Android #1510 also prepares once per publication. It acknowledges by comparing bolt11 rather than by revision.
Device gate: 417bcca — no re-run needed for this delta (publication logic only, no UI or journey change). delete-profile.xml passed at aac9d0b and d7e0e1f. perf: one fewer refreshPublicPaykitEndpoints per forced publication (read from code).
CI: Run Tests is red on testPendingWalletSwitchResumesAfterTags (SendConfirmationLifecycleTests.swift), a file this PR does not touch. The same test also fails on #954 and #973, which share no code, and master's unit tests are red on another test. It needs a re-run before merge.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of changes since 21a8f59f, at 417bccab, against the completed baseline review. Comparison base and merge base remain eb3156ab. This pass covered the single public-endpoint snapshot, its one invoice-revision retry, failure when that same preparation changes again, rejection for deletion, cancellation, identity or session replacement, and disabled sharing or Paykit UI, contact-payment enable and restore, revision acknowledgement before USDT preparation, the updated ReceiveInvoiceRefreshTests, and the unchanged profile journey paragraph. Android preparation was compared at 98cab12b. Inherited baseline coverage is profile-deletion pause and resume, contact deep-link retention, payment-request gating, forced public-refresh retention, and the startup comparison with Android PR 1510 at 9fddab92.
No new actionable code findings.
buildWalletEndpoints now prepares one wallet snapshot and forces a Lightning refresh on that call when a refresh is requested and Lightning publication is enabled. A revision change during address preparation or invoice creation retries once. A further change in that call fails publication, keeps cleanup pending, and leaves the session in place. Enabling contact payments stays enabled after one successful retry and restores the previous preference when publication fails. The acknowledged revision is the one taken with that snapshot before USDT preparation, so a newer invalidation stays pending, including when the Lightning invoice is empty. Android PR 1510 at 98cab12b also prepares the address and Lightning invoice once per publication. iOS still retries once when only the invoice revision changes.
Validation: source inspection at 417bcca. The updated tests were read and not executed. Run Tests, Run Integration Tests, and build-local were still in progress on this commit.
Device testing: not performed in this review.
Recommended before device testing: wait for Run Tests and Run Integration Tests on 417bcca.
There was a problem hiding this comment.
Verdict: ✅ Approve. No findings in this push, and all nine device cases pass at 417bcca.
Findings: none.
The three LOWs raised since aac9d0b are fixed at this head:
- Ours (thread): an invalidation during "Enable payments with contacts" no longer fails as a lost session. One invalidation is retried and the setting stays on (Test 1).
- @jvsena42's (thread): the retried invoice is acknowledged as current, so no maintenance round republishes it (Test 1). An invalidation after the snapshot still leaves cleanup pending (Test 4).
- @piotr-iohk's (thread): a second invalidation inside the one retry fails the enable and rolls it back (Test 2).
QA for 417bcca:
Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet, a staging Pubky profile and channels bought from staging Blocktank. The head build adds local hooks that are not part of this PR:
- They hold the public invoice preparation in each attempt, the snapshot right after its revision capture, new preparations, a write inside the endpoint lock, the deletion and the session revocation.
- They also log each attempt, the snapshot revision and each acknowledgement.
Every invalidation comes from a real event: the LSP paying the cached public invoice, or a new channel becoming ready. Invoices are named by a short hash of the bolt11. The homeserver endpoint, the app's stored invoice and its cleanup flag were read directly.
Test 1: one invoice invalidation while contact payments are being enabled ✅ The preparation retried once, published the new invoice and acknowledged it with cleanup pending false; the toggle stayed on with no error (app log, homeserver reads and recording).
- With contact payments off, the homeserver had no endpoints and the app kept its cached invoice a4d9d5b9.
- Turning the setting on held the forced preparation in attempt 0. The LSP then paid a4d9d5b9, and the app invalidated it.
- After the release, the app logged "invoice superseded, retrying". Attempt 1 committed a10484fc, and the homeserver served it.
- The acknowledgement compared the same revision and left cleanup pending false.
- No other publication started in the next 70 s (app log). The toggle read on in the UI tree, and no error toast showed over the payment sheet.
t1.2x.mp4 |
Test 2: a second invalidation inside the one retry ✅ The enable failed with "Bitkit could not publish Paykit payment endpoints.", the setting turned back off and the homeserver was left with no endpoints (recording, still and homeserver reads).
- As in Test 1, a payment on the cached invoice moved the enable to attempt 1, which was held.
- A new zero-conf channel became ready during that hold, and its delayed forced refresh invalidated the invoice again.
- After the release, the app logged "invoice superseded again, no further retry" and
publicationFailed. The enable created no third invoice. - The error toast showed about 8 s later. The rollback turned sharing off and removed the endpoints. The channel refresh's own publication had written once before the rollback.
- The payment sheet covered Settings during the run, so the toggle state was read from the UI tree; the still shows it off afterwards.
t2.2x.mp4 |  |
Test 3: deletion starts while the retry is held, then fails ✅ The held retry committed no invoice and published nothing during the deletion, and after the deletion failed the homeserver got a new invoice with cleanup pending false (app log and homeserver reads).
- A foreground publication was held in attempt 0, and the LSP paid the published invoice 1a2efcb2. After the release, the publication moved to attempt 1, which was held.
- Delete Profile was confirmed and held. Releasing attempt 1 ended it quietly, with no commit.
- The app's stored invoice stayed empty, and the homeserver kept 1a2efcb2 while the deletion ran.
- The deletion then failed ("Unable to Delete Profile"), and the identity was kept. The resumed publications replaced 1a2efcb2: the last write, e98ebe64, was acknowledged as current, and cleanup pending ended false.
t3.2x.mp4 |
Test 4: an invalidation after the Bitcoin snapshot ✅ A paid invoice written by a late snapshot left cleanup pending, and the next publication restored the current invoice about 42 s later with no payment or foreground (app log and homeserver reads).
- A foreground publication's snapshot of e0563e70 was held right after its revision capture. The LSP paid e0563e70.
- The payment's own publication then published a4d9d5b9 and acknowledged it.
- Releasing the held snapshot wrote the paid e0563e70 back. Its acknowledgement compared the older snapshot revision with the current one and left cleanup pending.
- The next publication reused a4d9d5b9, and pending cleared. The 42 s with the paid invoice served is the transient stale write the description of this PR allows for.
t4.2x.mp4 |
Test 5: forced channel refresh during a deletion that fails ✅ A channel that became ready during the held deletion cleared the app's invoice, and after the deletion failed the homeserver got a new invoice within 20 s with no further channel event (app log and homeserver reads).
- The homeserver served 129c56b4. A new zero-conf channel became ready while the deletion was held.
- Its delayed forced refresh cleared the app's stored invoice and set cleanup pending. The homeserver kept 129c56b4, and nothing published.
- The deletion then failed ("Unable to Delete Profile"). The homeserver served 85fdbeb8, and pending cleared.
t5.2x.mp4 |
Test 6: delayed preparation cannot republish endpoints a deletion removed ✅ The held publication was refused with sessionNotActive, and the homeserver stayed empty through the end of the deletion (app log and homeserver reads).
- The LSP paid the published invoice, and the payment's publication was held before preparation.
- The deletion removed the endpoints, and its session revocation was held.
- After the release, the publication was refused (app log). The homeserver had no endpoints 15 s later, and still none after the deletion reached profile onboarding.
t6.2x.mp4 |
Test 7: a write already inside the endpoint lock finishes before the deletion removes it ✅ The deletion waited 45 s for the held write, then removed the endpoints and reached profile onboarding (recording and homeserver reads).
- A payment's publication was admitted and held inside the endpoint lock, and the deletion was then confirmed.
- While the write was held, the homeserver kept its endpoints and the deletion stayed on its progress overlay.
- After the release, the write published 10411a21 with no publication failure. The deletion then removed the endpoints and reached profile onboarding, and the homeserver stayed empty.
t7.2x.mp4 |
Test 8: contact payments turned off while publications wait, then on again ✅ Once the change applied, the endpoints were removed and nothing republished them for over 70 s. Turning the setting back on published new endpoints with cleanup pending false (homeserver reads).
- A payment's publication and a maintenance round were held before preparation, and contact payments were then turned off. The change waited for the held work.
- After the release, both held publications, admitted while sharing was on, wrote first. The change then removed the endpoints and turned sharing off.
- Turning the setting on again published ddfbe697, acknowledged as current.
t8.2x.mp4 |
Test 9: forced channel refresh before sign-in ✅ The channel became usable while the session restore was failing, and after the restore the first publication consumed the kept refresh and replaced ddfbe697 (app log and homeserver reads).
- Cold launch with the homeserver unreachable: the restore failed twice while the channels reconnected. The app logged that it kept the refresh for the stored identity.
- With the network back, the session restored. The publication that reuses a valid invoice logged that it consumed the kept refresh and published d67d9826, with cleanup pending false.
- No channel event happened after the launch.
t9.2x.mp4 |
Carried, not re-driven:
- Tests 1, 2 and 4 of our review of d7e0e1f: the contact-link routing and the kept link's retry. These are unchanged since d7e0e1f.
- @jvsena42's delete-profile journey at aac9d0b. The deletion path is unchanged since aac9d0b.
Delta review of aac9d0b..417bcca: diff, 2 production files, 1 test file and the journey notes. USDT publishing was off. Test 4 held the snapshot at the revision capture rather than inside USDT preparation, and no case drove the routeHintsUnavailable path.
Pair PR synonymdev/bitkit-android#1510 at 98cab12 also prepares one Lightning invoice per publication, under an invoice lock and with no revision retry. The one-retry rule is iOS-only, as the reviews of @piotr-iohk (review) and @jvsena42 (review) note.
|
This PR is approved and CI passed at its head when I posted this comment: Its unit test failure is practically the old master unit test failure, which was fixed by a PR merged later into master I am fixing the merge conflict, letting CI unit tests pass, won't wait for e2e, then admin-merge it. |
ovitrif
left a comment
ovitrif
left a comment
There was a problem hiding this comment.
approving merge resolution
There was a problem hiding this comment.
Verdict: ⛔️ Request changes. Since the merge of master, a contact link that this PR holds through a failed restore is routed as soon as a sign-in link is cancelled: it opens Add Contact for a contact that is already saved (Test 1).
Findings:
- MEDIUM
Bitkit/MainNavView.swift:902: a contact link moved behind a sign-in link skips the deletion and restore hold.loadContactsForPendingDeepLinkIfNeededreads onlyapp.pendingDeepLinkURL. Master'sretainDeepLink(AppViewModel.swift:261-264) moves a pending contact link intodeepLinkAfterPubkyAuthwhen a sign-in link arrives, including a link that is pending only because this PR holds it.- When the sign-in sheet closes,
handlePendingDeepLink()finds nothing pending, so the preparation returnstruewithout its guards.routePendingDeepLinkIfReadythen promotes the held link (AppViewModel.swift:279) and routes it in the same call.isContactDeepLinkReadyis already true then: during a failed restoreisAuthenticatedis false, and during a deletion the contacts are loaded. - With a saved identity whose restore failed,
routePublicContactIfSignedOutreturns false, andscannerManager.handleScanopens Add Contact for the saved contact. During a deletion the same path opens the contact page and startsrefreshContactLink(read from the code, not driven). The link is consumed, so it does not open again once the restore or deletion ends. - Fix: in the call that promotes the held link, return without routing, so
onChange(of: pendingDeepLinkURL)runs the preparation for it. Or let the preparation read the held link as well.
QA for 92e554c:
Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet and a staging Pubky profile (Alice954e) with one saved contact (Bob954). Per case, the homeserver was made unreachable to fail the restore or the deletion, or held to stall them. The sign-in link is the dummy request from journeys/pubky-auth/signin-with-second-link.xml.
Test 1: a sign-in link opened while a saved contact's link waits through a failed restore ❌ After Cancel on the sign-in sheet, Add Contact opened for the saved Bob954 while the restore was still failing, and Bob's contact page never opened after the restore (recording and app log).
- Cold launch with the homeserver unreachable: the restore failed four times. Bob's link was held, and Home stayed for 25 s.
- The sign-in link opened the Authorize sheet. 0.7 s after Cancel, the app routed Bob's link and opened Add Contact with Save. The restore was still failing then.
- With the network back, the session restored and one contact loaded. The screen stayed on Add Contact: the link had been consumed.
t1.2x.mp4 |
Test 2: a saved contact's link alone while the restore has failed ✅ The link opened nothing during the failed restore and opened Bob954's contact page once after the restore (recording and app log).
- Cold launch with the homeserver unreachable: the restore failed four times. Bob's link left Home in place for 63 s, with no Add Contact, no toast and no route in the log.
- With the network back, the session restored, one contact loaded and the link opened Bob954's contact page once. Back returned Home.
t2.2x.mp4 |
Test 3: a failed deletion retries the kept contact link once ✅ The deletion failed with the profile still signed in, the link was retried once with no error loop, and the next foreground opened Bob954's contact page once (recording and app log).
- Bob's link was opened with the homeserver held, so its preparation waited. Delete Profile was then confirmed, and the homeserver refused.
- The deletion failed ("Unable to Delete Profile"). The kept link was retried once and failed to load contacts once, and the public endpoint refresh resumed right after.
- With the network back, the next foreground opened Bob954's contact page once.
t3.2x.mp4 |
Test 4: a contact link opened during a deletion that succeeds ✅ The link waited for the deletion, then opened Bob954's public profile (Add Contact) once, signed out, with no toast (recording and app log).
- Bob's link was opened 5 s after Delete Profile was confirmed. Nothing routed while the deletion ran.
- After the sign-out, the link opened Add Contact with Pay and Save once, and the log has no routing error. Back showed profile onboarding.
t4.2x.mp4 |
Carried, not re-driven:
- Tests 1–9 of our review of 417bcca: invoice retry, rollback, deletion during a retry, the late snapshot, forced refresh, the publication fence, the admitted write, the sharing toggle and refresh before sign-in.
WalletViewModel,PublicPaykitService,PubkyProfileManagerand the Paykit request and contact payment services are unchanged since 417bcca. - @jvsena42's delete-profile journey at aac9d0b. The deletion path is unchanged since aac9d0b, and Test 4 ends in a successful deletion.
Not driven, on code unchanged since 417bcca. The finding needs a fix first, and the next delta can test these:
- an empty Lightning invoice after the Bitcoin snapshot
- who owns the kept refresh after an identity replacement, a sign-out or a cancellation
- a hold inside USDT preparation, and
routeHintsUnavailable - turning sharing off while the restore is held: the toggle is hidden until the session restores
Delta review of 417bcca..92e554c: diff, the merge of master. The one conflict resolution keeps both onChange handlers in MainNavView, and both run (Tests 1 and 3). No case drove the finding's deletion variant, or both links queued on the PIN screen.
Pair PR synonymdev/bitkit-android#1510 merged at 98cab12. On Android, a link held behind a sign-in link is replayed through launchScan, the same contact-link wait as a first delivery (read, not driven).
|
Admin merge as planned above: master merged in at |
|
@ben-kaufman two commits were added to this PR after your approval; could you review them before it merges?
It merges for 3.0.0 RC3 once you approve and the unit tests pass. |
There was a problem hiding this comment.
Verdict: 💬 Comment. The fix for our finding holds (Test 1). One LOW: a failed Disconnect drops the forced Lightning refresh kept during a failed restore (Test 6). Two cases from the PR's notes cannot be set up from the app, so they stay unrun (Tests 9 and 10).
Findings:
- LOW
Bitkit/Managers/PubkyProfileManager.swift:1664: a failed Disconnect drops the kept forced refresh, although the identity stays.storedIdentityGuardbinds the refresh kept during a failed restore tosessionRevision.signOut(performSessionCleanup:)changes that revision inbeginSessionMutation()before its cleanup runs, and keeps it when the cleanup throws.- Disconnect needs the homeserver, so it fails while the restore fails. When the same identity then restores,
preparePendingPublicPaykitInvoiceRefreshdrops the refresh, and the publication reuses the cached invoice. That invoice's route hints can predate the channel that became usable or ready. - Fix: compare the stored identity rather than the session revision, or drop the kept refresh when a sign-out completes.
Our MEDIUM at 92e554c (thread) is fixed. A link held behind a sign-in link now goes through the contact-link preparation before it opens: through a failed restore (Test 1), during a deletion (Test 2) and from the PIN screen (Test 3).
QA for 4c026d2:
Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet, staging Pubky profiles (Alice954g with Bob954 saved, later Carol954g), a channel bought from staging Blocktank and the local USDT fixture. Where a case says so, the homeserver was made unreachable to fail the restore. The sign-in and payment links are the ones from journeys/pubky-auth/signin-with-second-link.xml.
Tests 2 and 5–8 use this head plus local hooks that are not part of this PR:
- They hold the deletion, a publication right after its snapshot's revision capture, a publication inside USDT endpoint preparation, and new preparations.
- One makes a new public invoice count as having no route hints.
- They log each preparation, snapshot revision and acknowledgement. Invoices are named by a short hash of the bolt11. The homeserver's endpoints and the app's stored invoice and cleanup flag were read directly.
Test 1: a sign-in link opened while a saved contact's link waits through a failed restore ✅ After Cancel nothing opened while the restore kept failing, and once the session restored Bob954's contact page opened once (recording and app log).
- Cold launch with the homeserver unreachable: the restore failed four times. Bob's link opened nothing.
- The sign-in link opened the Authorize sheet. For 36 s after Cancel, Home stayed, with no Add Contact, no toast and no contact route in the log.
- With the network back, the session restored and one contact loaded. Bob's link then opened his saved-contact page once. Back returned Home.
t1.2x.mp4 |
Test 2: a sign-in link cancelled while a contact link waits for a deletion ✅ Nothing opened while the deletion ran, and after it signed out Bob954's public profile opened once, with no toast (recording and app log).
- Delete Profile was confirmed, and a hook held the deletion.
- Bob's link, then the sign-in link, were opened during the deletion. The Authorize sheet showed over it. For 26 s after Cancel nothing opened, with no contact route or contact refresh in the log.
- Once released, the deletion removed the contacts and signed out. Bob's link then opened his public profile (Add Contact with Pay and Save) once. Back showed profile onboarding.
t2.2x.mp4 |
Test 3: a contact link and a sign-in link opened on the PIN screen ✅ After the PIN only the Authorize sheet opened, and after Cancel the contacts refreshed and Bob954's contact page opened once (recording and app log).
- PIN on, profile signed in. With the app in the background, Bob's link and then the sign-in link were opened, and the PIN screen showed.
- After the PIN, the Authorize sheet opened and no contact page.
- After Cancel, the contacts refreshed and Bob's link routed 3 ms later to his contact page. The log has one sign-in route, one contact route and no routing error.
t3.2x.mp4 |
Test 4: signin-with-second-link.xml ✅ In all three orders the Authorize sheet showed first with no Send screen, and after Cancel the Send flow opened at 10 000 sats once (recording and app log).
- Sign-in link, then payment link, on the PIN screen.
- Payment link, then sign-in link, on the PIN screen: the order this push changes, where the payment link waits behind the sign-in link.
- Payment link while the Authorize sheet is open.
t4.2x.mp4 |
Test 5: an empty Lightning invoice in the snapshot, then a channel becomes ready ✅ The held write left cleanup pending on the revision alone, and the next publication wrote the current Lightning invoice and cleared it (app log, homeserver reads and recording).
- With no channel, the public invoice was empty, and the homeserver had only the on-chain endpoint.
- A foreground publication was held right after its snapshot (no Lightning invoice, revision 8493EF9D). New preparations were parked.
- A zero-conf channel became ready. Its forced refresh invalidated the invoice (revision 675C8049).
- After the release, the on-chain-only write was acknowledged with "published current invoice false, cleanup pending true". The empty invoice matched the app's invoice, which was still empty, so only the revision kept the refresh pending.
- The parked publications then wrote a Lightning invoice, acknowledged as current with cleanup pending false. The app and the homeserver ended on the same invoice.
t5.2x.mp4 |
Test 6: a Disconnect attempted while the restore fails, with a forced refresh kept ✅ The endpoints ended consistent with no error, but the restored identity published without the kept refresh, which the failed Disconnect had dropped (the LOW; app log, homeserver reads and recording).
- Cold launch with the homeserver unreachable: the restore failed, and the channel event kept the forced refresh for the stored identity.
- Profile showed Try Again and Disconnect. Disconnect failed: it restores the session from the homeserver to remove the private endpoints first.
- With the network back, Try Again restored the same identity. Its publication logged "kept refresh dropped: identity changed" and ran without a forced refresh: the failed sign-out had changed the session revision.
- It reused the cached invoice. Here that invoice already had this channel's route hint, so the app and the homeserver stayed consistent and cleanup ended false.
t6.2x.mp4 |
Test 7: an invoice paid while a publication waits inside USDT preparation ✅ The acknowledgement used the revision captured with the Bitcoin snapshot and left cleanup pending, and the next publication repaired it (app log, homeserver reads and recording).
- USDT was on, against the local USDT fixture. The homeserver served the Lightning invoice, the on-chain address and the USDT address.
- A foreground publication captured its snapshot (revision A20E5C2A) and was held inside USDT endpoint preparation. New preparations were parked.
- The LSP paid the cached invoice, and the app invalidated it.
- After the release, the USDT step saw the new revision 5B7D2959. The acknowledgement used the snapshot's: "published current invoice false, cleanup pending true".
- The parked publication then wrote a new invoice with the USDT address, acknowledged as current with cleanup pending false.
t7.2x.mp4 |
Test 8: routeHintsUnavailable while contact payments are being enabled ✅ The enable published the on-chain endpoint only, acknowledged it as current and kept the setting on with no error (app log, homeserver reads and recording).
- A hook made the new public invoice count as having no route hints.
- Turning "Enable payments with contacts" off and on again logged "publishing on-chain endpoint only". It acknowledged the on-chain endpoint as current with cleanup pending false.
t8.2x.mp4 |
Test 9: turning contact payments off while the restore is held ⏭️ Not run: the app shows "Enable payments with contacts" only for a signed-in profile (GeneralSettingsView.swift:136), and during a failed restore General settings has no such row (recording).
t9.2x.mp4 |
Test 10: a completed sign-out, an identity replacement or a cancellation while a forced refresh is kept ⏭️ Not run: the app cannot reach these states (read from the code and Test 6).
- Disconnect restores the session from the homeserver before it can complete, so it fails while the restore fails (Test 6).
- Once the session restores, the kept refresh is consumed or dropped. A replacement identity needs that sign-out first.
- Nothing in the UI cancels a publication task.
Carried, not re-driven:
- Tests 2–4 of our review of 92e554c: a contact link alone through a failed restore, the retry after a failed deletion, and a successful deletion. This push changes
routePendingDeepLinkIfReadyonly when a link waits behind a sign-in link, and none of those cases has one. - Tests 1–9 of our review of 417bcca: the publication cases.
WalletViewModel,PublicPaykitService,PubkyProfileManagerand the Paykit services are unchanged since 417bcca. - @jvsena42's delete-profile journey at aac9d0b. Test 2 also ends in a successful deletion.
Delta review of 92e554c..4c026d2: diff, 2 files. Tests 1–4 drove the changed promotion path for contact and payment links. Tests 5–10 cover what our earlier reviews left open.
Pair PR synonymdev/bitkit-android#1510 is merged. It replays a link held behind a sign-in link through the same contact-link wait as a first delivery, which iOS now matches (read, not driven).
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 4c026d2dd.
Delta since my last review:
92e554c0a(master merge): the only conflict is inMainNavView.swift, and the resolution keeps all three handlers:onChange(isDeletingProfile)(:445),onChange(isPreparingPendingContactDeepLink)(:452), and master'sonChange(sheets.isPubkyAuthApprovalShownOrPending)(:456) with its replacement delay. Each merged file's diff from417bccamatches master's own diff byte for byte. Nothing was dropped or duplicated.4c026d2dd:routePendingDeepLinkIfReady(AppViewModel.swift:282) now only promotes a held contact link and returns. ThependingDeepLinkURLonChangethen runs a freshhandlePendingDeepLink(), so the deletion guard (:905), the restore and unauthenticated guards inpreparePubkyContactLink(MainNavRouting.swift:19-21) and the signed-out check all apply to it. No suspension sits between preparation and routing when nothing else is pending. The held link is cleared before it is promoted, so it cannot be consumed twice. The lock-screen bookkeeping keeps the held URL throughformIntersection(:302).
My four threads stay resolved, and this delta reopens none of them. ovi-reviewer's open LOW on a failed Disconnect at PubkyProfileManager.swift:1664 predates this delta and is not re-raised here.
CI is green at this head.
Device gate: 4c026d2 — no new run.
Skipped — already driven this round: a contact link held through a cancelled sign-in during a failed restore, during a deletion, and with both links queued on the PIN screen, plus signin-with-second-link.xml in all three orders (iOS, by ovi-reviewer at 4c026d2, passed: #954 (review)).
Skipped — covered by CI at 4c026d2: unit and integration tests, local e2e (all green).
delete-profile.xml passed 7/7 at aac9d0b and d7e0e1f, and the deletion path has not changed since. perf: not relevant (one extra routing hop after the sign-in sheet closes); l10n: no UI text changed.
5 participants
Closes #955
Related to #908 and #861; recovery and broader latency investigation remain open. Android counterpart: synonymdev/bitkit-android#1510.
This PR pauses automatic Paykit refresh and publication during profile deletion so they do not compete with explicit cleanup.
Description
Defers automatic contact refresh, Shop imports, request intake/presentation and endpoint publication during deletion, then resumes for a retained identity.
Receive/channel publication captures the starting session before preparation and rechecks cancellation, identity, deletion and sharing inside the existing shared endpoint lock. A delayed preparation cannot republish endpoints after deletion removes them. Already-admitted writes finish before removal; explicit cleanup is unchanged.
Matching paid-invoice invalidation and payment-proof bookkeeping remain enabled. Replacement stays pending after transient errors, and an older admitted write cannot acknowledge newer invalidation. Public refresh resumes without waiting for contact refresh.
Retains contact deep links during deletion and retries once preparation is available. Only confirmed absence of a stored identity permits signed-out public contact navigation. Running, deferred or failed restoration and unreadable credentials keep the URL pending without private reads or a scan error; restoration completion re-enters existing preparation. Signed-in contact refresh retains identity and cancellation checks.
Incorporates the merged backup fix and translations without rewriting history.
Retains a forced public Lightning refresh before channel work can suspend, including during deletion. Before authentication, the request is bound to the stored-identity revision and profile object; existing publication consumes it only after that same identity restores. Sign-out and identity replacement invalidate it, without a new recovery loop or authentication grant. An invoice revision plus session/sharing/cancellation checks prevent older preparation from restoring the invalidated cache.
Both wallet-backed publication paths re-arm pending reconciliation if an older public snapshot lands after an acknowledged replacement. Enabling sharing and rollback preserve that publisher-owned result; explicit removal acknowledgement is unchanged. This repairs eventual endpoint state; it does not prevent every transient stale write.
Each public publication prepares one wallet endpoint snapshot. Public invoice preparation retries once when only the invoice revision changes under the same eligible session. Repeated invalidation ends with a publication failure rather than false session loss. Session changes, disabled sharing/UI, cancellation, and deletion before or during the retry remain rejected; creation and decoding errors still propagate. Existing admitted writes and conservative publication acknowledgement are unchanged.
Both wallet-backed publishers acknowledge the invoice revision captured with the final Bitcoin endpoint snapshot, before later USDT preparation. A successful retry no longer schedules a redundant republish; newer invalidation still stays pending, including when the invoice is empty.
Out of Scope
Design
N/A - no UI changes.
Preview
N/A - no UI changes.
QA Notes
Journeys
delete-profile.xml: reviewer 5549389182 reported 7/7 actions passed on precedingaac9d0bwith zero contacts, persistent progress feedback, and no deleted profile returning on reopen. That pass and earlierd7e0e1f62-contact runs do not exercise the current retry correction.Manual Tests
Hold an admitted SDK operation, begin deletion and trigger contact/payment/maintenance events. Release it and verify admitted work and explicit cleanup finish without new automatic intake/publication. Hold receive/channel endpoint preparation, remove endpoints during deletion while revocation is held, then release preparation: endpoints must remain absent. A write already inside the shared endpoint lock must finish before cleanup removes it. These gates are outside journey Capabilities.
Hold invoice preparation or identity lookup before deletion and verify automatic public/private rotation stays deferred while matching invoice invalidation and receipt attribution complete. Native lock tests are not a payment/device pass.
Hold contact deep-link preparation and make cleanup fail before preparation returns. Verify the retained identity resumes automatic work and the link retries once without an error loop. Also verify successful deletion opens a retained public contact signed out without a QR error or private-contact read.
After deferred invoice replacement, fail deletion and the first resumed publication. Verify maintenance retries without another scene activation/payment. Older writes must not acknowledge newer invalidation; stale, cancelled or disabled-sharing failures must not mark new work pending.
With a saved identity, hold or fail restoration and open a saved-contact link: no Add Contact navigation, private-contact read or scan error; the URL remains pending. Complete recovery and verify existing-contact navigation once. A genuinely absent identity, including successful deletion, must still open the public contact. This requires controlled restoration beyond journey Capabilities.
Hold initial restoration or SDK admission with public contact payments enabled, disable sharing, then release: no new automatic publication. An admitted write may finish without consuming pending cleanup. No new device execution is claimed for this counterpart case.
Hold deletion, make a channel usable/ready, then fail deletion retaining the identity: the public invoice refresh survives, including an older held invoice creation. Skipped/failed publication stays pending; newer invalidation is not consumed by an older write, and cancelled/replaced/disabled-sharing work cannot refill the cache. Release old public snapshot X after replacement Y is acknowledged: normal reconciliation restores Y. Repeat through enabling sharing and rollback, including successful current publication clearing earlier pending work. Controlled gates are outside journey Capabilities; private channel-refresh behavior is unchanged.
Hold initial restoration before authentication, make a channel usable, then let restoration fail or publication remain blocked. Restore the same saved identity and retry normal publication without another channel event: the old invoice must be refreshed. Replacement identity, sign-out, cancellation and disabled sharing must not authorize the pending refresh. Controlled restore/publication gates are outside journey Capabilities.
Enable contact payments while the public invoice is invalidated during address preparation or invoice creation. One invalidation should re-prepare successfully without rollback or a redundant maintenance republish; repeated invalidation should stop after one retry. Invalidate again after the Bitcoin snapshot while later endpoint preparation is held, including an empty Lightning invoice: the newer refresh must remain pending. Hold the invoice-only retry, begin deletion, then release: no replacement publication or cache refill. Identity/session changes, cancellation, disabled sharing/UI and genuine creation/decode errors must still reject. These require controlled gates beyond journey Capabilities.
Automated Checks
PubkyProfileManagerTests.swift: deletion lifecycle and session/identity checks.PaykitPaymentRequestServiceTests.swift: queued automatic intake versus explicit proof completion.ReceiveInvoiceRefreshTests.swift: paid invalidation, deferred publication, retries, acknowledgement ownership, and receive/channel preparation racing deletion, cancellation, sharing and session replacement. The existing production endpoint lock orders admitted writes before cleanup.WalletViewModelReceiveTests.swift: local receive details continue without authentication while automatic public publication stays blocked.PubkyContactLinkTests.swift: retained URLs and real navigation-model routing without private reads when signed out, plus stale/cancelled preparation.Final author validation at
417bccab: 2,643 native tests passed with exactly the six existing CI live/integration-suite exclusions, plus 314 focused tests. xcresult independently confirms all unique passing cases. Three full-native CLI per-test listings were omitted by interleaved output; xcresult and complete raw totals reconcile them. One test added, one strengthened, two existing snapshot gates adapted, none removed. The unchanged21a8f59fproduction source failed both new boundary regressions. The endpoint builder now prepares one snapshot, retaining forced Lightning refresh and a single invoice-only retry budget. Repeated invalidation during that sole retry still rejects publication, retains pending work and preserves enable rollback. The second address-time invalidation from the old duplicate-call reproducer is no longer reached; it is not claimed as a post-fix executed rejection.Full SwiftFormat lint retains 57 exact baseline findings against actual master
bd099662; translations are byte-identical at zero warnings/errors. The complete compiler audit verified all 16 production-rebuild warnings and the one final-focused warning against the existing 77-warning reference: all 22 referenced source files are unchanged on actual master. This is verified reference reuse, not a fresh master compiler run. The incremental native run emitted no diagnostics. Independent correctness/native/polish review is clean on the exact committed patch. The full eleven-commit PR range is cryptographically signed and verified locally and on GitHub. No dependency, binding, persisted format, SDK operation lock or safety wait changed.Current
417bccabhosted unit attempt 1 failed the unchanged tag-to-coin-picker visibility assertion. Ten unchanged local executions on exact master and ten on this head passed on iOS 26.5; CI used iOS 26.2, and the failure is not base-reproduced. Ovi's unit attempt 2 also failed, this time in the unchanged basic app-launch UI test while acquiring XCTest's background assertion; all 2,651 native executions passed, including the earlier navigation case. Exported diagnostics show the developer-testing assertion was handled only after XCTest timed out and the next test terminated the app. The underlying delay is unexplained and not base-reproduced; no code/test change or further retry was made. Actual E2E job 117191824764 and required e2e-status 117240386702 passed at 23:17 UTC; the unit/UI job is still failed. Integration passed its second internal attempt after a channel-purchase service-unavailable failure, while refund lifecycle passed both attempts. Build/translations passed. See the complete CI investigation and retained failure history. Previous21a8f59funit, build and translations passed; the unit job reported 2,645 native passes and 39 UI passes with four UI skips on its GitHub merge checkout, not this local branch's test inventory. Its E2E job117150149762 was queued before the push. Its integration job117141829556 was cancelled at the one-hour job limit after two real internal attempts each failed one of 34 tests withLDKNode.WalletOperationTimeout; the third attempt has no final result. Both annotations identify refund-lifecycle test line138,lightning.sync()after VSS restore/start and before refund deposit/mining. The exact operation inside sync and root cause remain unknown. Test/service/workflow/dependency sources match actual master, but this does not establish an unrelated/base failure. Actual masterbd099662passed all 34 integration tests on its first attempt, refund lifecycle64.273s; previous masterafdc534balso passed34/34, refund49.697s. The timeout is not base-reproduced. No blind rerun, timeout/assertion/exclusion change or funded local live test was made.Earlier
aac9d0bpassed all required CI, including actual E2E117022271919 and e2e-status117090359909; its integration passed34 tests first attempt, refund46.819s. Earlierd7e0e1f1had two same-named refund-lifecycle timeouts before cancellation. Later passes do not establish a root-cause fix. Preceding21a8f59fauthor validation remains historical:2,642 native/313 focused,57 baseline format findings and zero translation errors/warnings.Current-head reviewer evidence: Ovi review 5554309476 approves exact
417bccaband reports nine passing cases on an iPhone 17/iOS 26.5 simulator, regtest wallet and staging services, with disclosed uncommitted hold/logging hooks. The cases cover successful invoice-only retry without redundant maintenance publication; second invalidation rejecting and rolling enable back; deletion during a retry followed by recovery after failed deletion; late-snapshot invalidation retaining repair; forced refresh during failed deletion; refusal of delayed preparation after endpoint removal; admitted-write completion before removal; sharing-off/on ordering; and retained forced refresh across failed initial sign-in. Payments and channel events were performed by the reviewer, not this author pass.The late-snapshot case served the paid invoice transiently for about 42 seconds before reconciliation restored the current invoice. That supports eventual repair, not prevention of every stale write or a latency guarantee. The sharing-off case allowed already-admitted publications to finish before removal applied. USDT publication was disabled; the snapshot was held immediately after revision capture rather than inside USDT preparation, and
routeHintsUnavailablewas not exercised. Retained-link routing and the ordinary deletion journey were carried from older heads, not rerun. Identity replacement, new same-key sessions, cancellation ownership, empty-Lightning variants, private/payment-proof combinations and the 62-contact repeat are not established by these nine cases. The compound manual checkboxes remain open; this is attributed reviewer evidence, not author replication or a benchmark.No new author-driven UI/device/payment journey was run for this correction. Reviewer5551080932 approved preceding
aac9d0band reported seven controlled simulator cases using local hold/failure/logging hooks: force during failed deletion, force before failed restoration, eventual stale-snapshot repair, paid-invoice replacement after failed deletion/publication, sharing-toggle admission, refusal of preparation after deletion removal, and finish-before-removal of admitted writes. It also reproduced the enable/invoice LOW fixed here. Those results are historical for417bccab, not a current device pass. Late-snapshot enable/rollback, held-restore sharing-toggle, replacement/sign-out/cancellation device ownership, and some pending-marker enable/rollback cases were not driven.Reviewer5549389182's ordinary zero-contact journey was on
aac9d0b; retained-link cases were carried from earlierd7e0e1freviews, not re-driven here. Current manual checkboxes remain open. Native fixtures are not physical-device/mainnet or controlled remote acceptance. Private channel-refresh implementation is unchanged from master. Existing routing tests do not mount MainNavView or establish restoration-observer timing. Prioraac9d0bauthor evidence (2,638 native and 309 focused) remains historical. Initial fixture/toolchain failures and the superseded overbroad test command, interrupted after unit tests while entering extra UI configurations, are not counted as final passes.The pre-USDT revision capture was source-inspected, not separately tested with a suspended USDT operation. Native tests hold the admitted endpoint write; neither those tests nor the code review establish device timing, remote acceptance or a measured speedup. The preceding
b8a30780correction passed 2,641 native and 312 focused tests and remains historical evidence.The earlier publication-admission follow-up passed 2,627 native tests and focused receive/service checks. That evidence predates the current follow-ups and is not substituted for final validation above.
Historical simulator evidence at
1ae24c1b: reviewer 5544014702 completed ordinary deletion with one and 62 contacts and held-operation admission. Successful-deletion retained-link routing failed and was subsequently fixed. The failed-deletion run did not achieve cleanup failing before preparation returned, and no payment during deletion was tested. Earlier five Profile/contact-import UI smoke passes predate the follow-ups.Earlier author timing-only samples on a preserved unfunded one-contact simulator recorded deletion at 33.13s versus 43.02/43.16s in two baselines, with zero new saved-contact-observer SDK admissions versus eight before; an admitted read completed. These are historical samples, not a final-head benchmark. Recovery still took 73.00s and recreation 43.58s. Timing hooks are absent. No payment, funding, broadcast or wallet reset was performed.
An earlier hosted navigation assertion passed on an unchanged rerun. Its original cause remains unproven; that is not evidence of a root-cause fix or complete manual acceptance.
The startup regression reproduced reuse of the old invoice across nine restoration/preparation/publication combinations on the preceding production source. Failure cases also lost pending repair. The final tests exercise real gated profile initialization with an adopted-reference fixture and narrow endpoint/preparation fakes; they do not establish mounted SwiftUI lifecycle timing or remote device behavior.
Reviewer comment 6216622041 reports historical
d7e0e1f62-contact deletions in 58.9 and 59.6 seconds versus 70.8 and 70.0 seconds on mastereb3156a. Both builds left one private withdrawal pending and logged a capability-disable policy refusal, so these warnings predate this PR. Review5549389182 additionally reports historicalaac9d0bzero-contact deletion at 30.4-31.4 seconds versus master 33.4-34.6 seconds. These are bounded simulator samples, not a general speedup or maximum-delay guarantee. No safety check or wait was bypassed.