fix: save contacts without repeated remote lookups - #851
Conversation
|
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 8 files.
Pair PR synonymdev/bitkit-android#1395: equivalent.
Findings:
2 inline (2 LOW)
QA:
Tests running: 2 of 4 passed.
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
jvsena42
left a comment
There was a problem hiding this comment.
One LOW inline (own key in the follow list).
Merge note: #830 adds a restorePrivateConnection || existing != nil guard to saveContact, and the two PRs conflict in ContactsManager.swift. Whichever lands second must make the new default saveContact closure in importContacts pass restorePrivateConnection: true, or every import of a new contact fails with profileNotFound.
Checked and clean:
- Receiver paths are re-discovered on every link/publication pass, and endpoints are fetched from the contact key at pay time.
- The label is only a fallback.
- The saved key is the preview key.
- Partial success is kept, and a retry saves only the missing contacts.
- The progress guards hold.
- The journey files are identical to the Android twin.
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 9 files.
New findings below; the rest is in the review.
Retest suggested: Tests 1-2, J1, J2 (Both journeys changed, as did ContactsManager and the self-follow expectations.)
Pair PR synonymdev/bitkit-android#1395: not compared; the prepared-save, retry and pending-control behavior remains aligned with Android PR #1395.
Findings: 2 fixed. No new findings.
The README guidance is wrapped, and both import views now have tests for pending-state UI. Self-follows are excluded before profile resolution, with tests for bare and prefixed keys and self-only lists.
QA:
Tests running: 0 of 4 passed.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 3 files.
New findings below; the rest is in the review.
Retest suggested: Tests 1-2, J1, J2 (No QA item finished.)
The prepared-save, retry, and pending-control behavior remains aligned with Android PR #1395; that PR was not compared here.
Findings: 2 previously fixed. No new findings.
Prepared contact saving now restores local private connections as required by the updated SDK. The DEBUG fixture initializer works with the updated manager, and the import regression tests remain intact.
QA:
Tests running: 4 of 4 passed.
Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
There was a problem hiding this comment.
Verdict: ✅ Approve
Tests for the review: Tests 1 and 2 and journeys J1 and J2 pass.
QA:
Tested on iOS 26.5 simulator (iPhone 17 Pro), regtest.
🟢 Tests 1, 2, J1, J2
Test 1
Import completed offline and all three contacts remained after reconnecting.
1.mp4 |
![]() | ![]() | ![]() | ![]() |
Test 2
The failed save kept import open and preserved two contacts; retry saved only the missing contact.
2.mp4 |
![]() | ![]() | ![]() | ![]() |
Test J1
Imported all 62 Pubky-only contacts; self-follow variants excluded the owner from saved contacts.
J1small.mp4 | J1large.mp4 | J1self.mp4 | J1selfonly.mp4 |
![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() |
Test J2
Selected contacts were saved and the deselected contact stayed absent, including with self-follow.
J2.mp4 | J2self.mp4 |
![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() | ![]() |
Note
I ticked Tests 1–2 and J1–J2 in the PR checklist.
Tip
Tests 1, 2 worth a journey
Test 1
- Open the populated contact import preview
- Disable connectivity after the preview is populated
- Verify the prepared contact list remains visible
- Tap Import All
- Verify Let Your Contacts Pay You appears
- Restore connectivity and open Contacts to verify the saved names
Test 2
- Open the profile import preview with three contacts
- Make one contact save fail in the debug build
- Tap Import All
- Verify the import screen stays open and two contacts were saved
- Clear the save fault and tap Import All again
- Verify only the missing contact was written and all three contacts appear
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete pull request diff against its merge base, at b7ce984.
No new actionable code findings.
Prepared import saves the contacts already shown in the preview, keeps successful saves when a later local save fails, and skips the signed-in profile before profile resolution. That matches #853. New contacts start on the wallet receiver path; the existing contact-sync link burst discovers further paths. The same prepared-save, retry, and self-follow behavior is in bitkit-android#1395 at faae845. Follow keys are re-encoded by Paykit before they reach this filter, so a padding-bit alias of the same key arrives as the canonical spelling.
Validation: inspected ContactsManagerTests and ContactImportUITests at this revision and did not run them here. Run Tests passed on this head.
Device testing: not performed in this review. ovi-reviewer reports manual tests 1–2 and journeys J1–J2 passed on an iOS 26.5 simulator at this revision.
Ready for device testing.
Suggested additional test cases
- Platform: iOS simulator, regtest, disposable Ring identity.
- Setup: the following list includes that identity under a z-base32 spelling that changes only the final character's padding bits, plus at least one other follow.
- Action: open the import preview and tap Import All.
- Expected result: the preview count excludes that identity, Import All finishes on Let your contacts pay you, and Contacts does not contain that profile.
jvsena42
left a comment
There was a problem hiding this comment.
No new findings. The self-follow thread is addressed at b7ce984.
Checked and clean:
ContactsManager.swift:548skips the signed-in key before profile resolution, so it never reachespendingImportContactsorimportContacts.- Padding-alias self-follows, which Android filters with
canonicalizedin synonymdev/bitkit-android#1395, cannot reachmatcheshere. Follow keys come back fromfetchPubkyFollowsre-encoded byPubkyPublicKey::new, and the session key is derived from the secret viapubkyPublicKeyFromSecret, so both inputs are canonical. - After the merge, the default
importContactsclosure passesrestorePrivateConnection: true. New contacts no longer throwprofileNotFound, and blocked peers are re-blocked if the save fails. - Partial failure keeps the saved subset.
contactsdrivesprepareSavedContactsand the link burst. A retry saves only the missing keys and rethrows the first error so the preview stays. - Both views guard against re-entry with
isImporting, and Select is disabled while an import runs. ContactImportUITestFixtureis only reachable under#if DEBUGwith-contact-import-ui-test, and the release path is unchanged.
Device gate: not run — the contact journeys need a disposable Ring identity with a known follow list including its own key, which the journey Capabilities table does not provide. The iOS run waits on the Android gate.
…bky-profile-load-lag Adopts #851's contact import: it saves the profiles the preview already shows to local SDK contact storage, without profile or receiver lookups, and the import screens guard against repeated taps. This drops the branch's own import save path, its unresolved follow tracking, the import running flag and the route check after an import, which #851 replaces. Follow lookups while preparing an import stay on the interactive read lane. # Conflicts: # Bitkit/Managers/ContactsManager.swift # Bitkit/Views/Contacts/ContactImportOverviewView.swift # Bitkit/Views/Contacts/ContactImportSelectView.swift
#851 adds journeys for importing all and selected contacts, which cover saving the prepared follows. contact-import.xml becomes contact-import-after-leaving.xml and keeps only what those do not: an import keeps saving after you leave the overview and, when it finishes, does not take you to Pay Contacts. The Contacts list journey now points at import-all-contacts.xml for its precondition.



























Closes #853
Twin: synonymdev/bitkit-android#1395
Importing an already prepared list of contacts repeated profile lookups and receiver discovery for every contact. Those requests were serialized by the SDK adapter, so large lists could remain busy for minutes. Partial failures were also allowed to advance past import.
Description
Out of Scope
Design
N/A — no design available. Existing import screens and loading controls are reused.
Preview
The reported recordings and logs were supplied privately and are not attached here. No post-fix device recording is claimed; the import journeys below remain for device QA.
QA Notes
Journeys
import-all-contacts.xml— imports a prepared list, including Pubky-only profiles; repeat with 62 contacts.import-selected-contacts.xml— saves selected contacts and leaves the deselected contact absent.Both journeys are ported between Android and iOS.
Manual Tests
Automated Checks
A storage-backed regression exercises the default SDK import path without a live session. It verifies canonical key/label persistence, the default wallet receiver, preservation of existing receiver paths, and reloading after the SDK runtime is discarded.
updated
ContactsManagerTests.swift— a 62-contact prepared batch saves without network resolution, duplicates are skipped, partial failure preserves successful saves, retry saves missing contacts, and cancellation stops further writes/results.ran 45 tests across
ContactsManagerTestsandContactPaymentsServiceTests: all passed, including existing publication/rollback behavior.ran XcodeBuildMCP simulator build/test with
DEBUG E2E_BUILD UNIT_TESTING, SwiftFormat andgit diff --check.Four new regression tests; no existing tests removed.
Independent final correctness and repository-fit review: clean.