Skip to content

fix: save contacts without repeated remote lookups - #1395

Merged
piotr-iohk merged 5 commits into
masterfrom
fix/contact-import-progress
Oct 2, 2026
Merged

piotr-iohk merged 5 commits into
masterfrom
fix/contact-import-progress

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1397

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

  • Save the profiles already shown in the preview using local SDK contact storage. The import operation no longer performs profile or receiver network lookups.
  • Preserve existing receiver paths; new contacts start with the wallet path. Additional receivers are discovered through the existing contact refresh/private-link flow.
  • Preserve successful saves and keep the remaining import retryable when a local save fails. Retry skips contacts already saved.
  • Prevent repeated import actions while a save is pending.
  • Preserve master's explicit contact re-add/unblocking and synchronized contact-list updates.
  • Android payment-sharing errors retain guidance for wrapped session/wallet errors and offer a useful retry message instead of Unknown error. Public endpoint sync failures retain their individual diagnostic messages in logs.

Out of Scope

  • SDK lock and network timeout redesign; profile discovery before the preview still requires connectivity.
  • The exact cause of the later payment-sharing Continue failure is not established: the supplied log export ends before that attempt.
  • Profile restoration/deletion feedback is covered by separate changes.

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

  • new import-all-contacts.xml — imports a prepared list, including Pubky-only profiles; repeat with 62 contacts.
  • new import-selected-contacts.xml — saves selected contacts and leaves the deselected contact absent.

Both journeys are ported between Android and iOS. Companion iOS change: synonymdev/bitkit-ios#851.

Manual Tests

  • Disable connectivity after the preview is populated → prepared contacts can still be saved without remote discovery — network fault injection is outside journey capabilities.
  • Fail a local contact save → remain on import, preserve successful saves and retry missing contacts; do not report complete success — storage fault injection is outside journey capabilities.

Automated Checks

  • updated PubkyRepoTest.kt — prepared profiles save with remote lookups unavailable, duplicates are skipped, partial failure preserves successful saves, and retry saves missing contacts.
  • updated ContactImportOverviewViewModelTest.kt — pending import prevents duplicate requests and cancellation clears progress.
  • updated ContactImportSelectViewModelTest.kt — selected profiles are passed through without re-fetching.
  • updated PayContactsViewModelTest.kt — wrapped session errors retain guidance and unknown failures offer retry without claiming payments are enabled.
  • Focused repository, SDK, import and onboarding tests passed. After integrating current master, full just test passed all 3,044 tests with no failures or skips.
  • just compile, just lint and just build passed. Lint completes with existing repository warnings; none points to changed code.

Four new regression tests; no existing tests removed.

Independent final correctness and repository-fit review, including the contact-deletion integration: clean.

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Contact import flow refactored to skip redundant lookups.

The PR appears safe to merge based on the reviewed changes.

Summary

The PR saves prepared contact profiles locally instead of repeating remote lookups, retains successful saves when another save fails, guards against duplicate import actions, and improves payment-sharing error diagnostics.

  • Adds regression tests and contact-import QA journeys.
  • No actionable issue was established.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Prepared contact preview] --> B[Import selected profiles]
  B --> C[Save contacts locally]
  C --> D{Any save failed?}
  D -->|Yes| E[Keep preview available for retry]
  D -->|No| F[Continue to payment-sharing setup]
  C --> G[Contact synchronization]
  G --> H[Discover additional receiver paths]
Loading

Reviews (1) · Last reviewed commit: "fix: save prepared contacts without remo..."

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from faae845 (run).

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

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One LOW inline (own key in the follow list).

Checked and clean:

  • Saved receiver paths are re-discovered on every link/publication pass, and endpoints are fetched from the contact key at pay time, so skipping the lookup cannot misroute a payment. SDK saveContact merges paths.
  • The label is only a fallback, and the profile is re-resolved on load.
  • The saved key is the preview key.
  • Partial success is published, the pending import is kept, and a retry saves only the missing contacts.
  • isImporting resets in finally.
  • restorePrivateConnection = true unblocks a contact previously deleted, and re-blocks on failure.
  • The journey files are identical to the iOS twin.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Full review of the complete PR diff against its merge base, at a9ad5dff.

1 actionable finding — resolve or provide an evidence-backed rebuttal.

The exact and prefix-stripped self-follow from the open review thread is filtered at this revision. A follow that is the same 32-byte identity with a different final z-base32 padding character still stays in the preview, and Import All fails again on every retry. The iOS twin bitkit-ios#851 uses the same matches comparison. Android's import-all actions verify self-exclusion; the iOS journey describes that repeat in its description and README and does not use those action lines.

This review did not run unit tests or a device. The new PubkyRepoTest import cases were inspected and cover an exact key and a prefix-stripped key. The passing Appium jobs do not walk contact import; the new journeys are the device contract.

Device testing: not performed in this review.

Suggested additional test cases

  • Android. Setup: a disposable Ring identity whose follow list includes that identity spelled with a non-canonical final z-base32 character (same 32-byte key as the session; the repo fixtures ending yw5xy and yw5xg are this pair) plus one other follow. Action: open the import preview and tap Import All. Expected: the friend count excludes the self alias, Import All finishes on Let Your Contacts Pay You, and Contacts does not include your own profile.

Findings

  • [LOW] Exclude padding-equivalent self follows from import — inline at app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:966.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated
@ben-kaufman
ben-kaufman requested a review from piotr-iohk October 1, 2026 06:38

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Follow-up review since a9ad5dff, at faae845. The inherited baseline is the completed full review (prior QA review) of the diff against merge base 4605a078. This pass rechecked that coverage and the padding-alias change in faae845.

No new actionable code findings.

The self-follow failure in the open review and the padding-alias finding in the prior QA review are addressed at this revision. prepareImport drops a follow when PubkyPublicKeyFormat.canonicalized matches the session key, so the fixture spellings ending yw5xg and yw5xy stay out of the preview and cannot fail Import All on retry. A later local save failure still keeps contacts already saved, leaves the preview up, and a retry saves only missing keys. isImporting is set before the save and cleared in finally.

Compared only the self-follow filter on companion bitkit-ios#851 at b7ce984. That revision still uses matches, which keeps a padding-different spelling of the session key. The Android import journeys now include that repeat.

Validation: inspected PubkyRepoTest, ContactImportOverviewViewModelTest, ContactImportSelectViewModelTest, and PayContactsViewModelTest; they were not executed here. build and lint passed on this head. The passing local Appium jobs do not walk contact import. The device contract is import-all-contacts and import-selected-contacts. A check of the z-base32 padding mask maps the yw5xg / yw5xy fixture pair to one canonical key; that was not a run of PubkyPublicKeyFormatTest.

Device testing: not performed in this review.

Ready for device testing.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No new findings. The own-key thread is fixed at faae845.

Checked and clean:

  • prepareImport drops the own key via PubkyPublicKeyFormat.canonicalized, covering exact, prefix-stripped and final-symbol padding aliases. Decoding the …yw5xy / …yw5xg fixtures gives the same 32 bytes, while …yw5xw decodes to a different key, so the mask merges aliases without merging distinct keys.
  • canonicalized cannot throw. PaykitPublicKeys.normalize checks length and alphabet without decoding and is wrapped in runCatching, so a malformed follow falls through to the existing placeholder path.
  • The preview is the only source for Import All and Import Selected, so saveContact never receives the self identity.
  • PublicPaykitRepo sync-failure logs carry only the endpoint identifier and error text.
  • PayContactsViewModel unwraps a wrapped PublicPaykitError. The retry copy's "contacts unchanged" claim holds, since enable() and rollbackEnabled never touch contacts.
  • Import buttons are guarded against re-entry, isImporting resets in finally, and the pending import survives cancellation for retry.
  • iOS twin synonymdev/bitkit-ios#851 compares with matches instead of canonicalized. That is not reachable there: both the follow keys and the session key come out of the Paykit FFI re-encoded from bytes, so a padding alias never reaches the filter. The same holds on Android, which makes canonicalized here defensive.

Device gate: not run — the contact journeys need a disposable Ring identity whose follow list includes its own key and a padding-alias spelling of it, and the Capabilities table in journeys/README.md provides no such identity.

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Import of a prepared Ring follow list is faster on faae845c. That matches this change: saving the contacts already shown in the preview no longer repeats profile and receiver lookups.

Still slow, and already covered elsewhere:

  • Loading the contact list before import, and contact avatars, on this build and on merged synonymdev/bitkit-ios#851. Those reads still take the Paykit lock one at a time. Open #1399 and synonymdev/bitkit-ios#854 move public profile, follow, and avatar reads off that lock (up to six at once) and cache avatars. #854's 4.6 s figure is a spinner on the Ring row until Pay Contacts opens. It is not the Continue button.

Not covered here or by #1399 / #854, from a Ring import of 61 contacts:

  • Continue on "Let your contacts pay you" ran for a long time and did not finish, on Android and on merged iOS. This PR already lists that path as out of scope.
  • On iOS, creating a profile shows no loading indicator. #854 leaves CreateProfileView out of scope for that wait.

Follow-up bugs for those two gaps are drafted and will be linked when filed.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

Follow-ups from the 61-contact Ring import:

@piotr-iohk
piotr-iohk dismissed jvsena42’s stale review October 2, 2026 05:43

findings addressed

@piotr-iohk
piotr-iohk merged commit 6ba44a4 into master Oct 2, 2026
21 checks passed
@piotr-iohk
piotr-iohk deleted the fix/contact-import-progress branch October 2, 2026 05:44
Jasonvdb added a commit that referenced this pull request Oct 2, 2026
Adopts #1395's contact import: it saves the profiles the preview already
shows, one at a time and without profile lookups or receiver discovery,
skips contacts already saved, keeps the saved contacts and fails when a
save fails so a retry saves only the missing ones, leaves the identity's
own key out of the preview, and the import screens ignore repeated taps.
This drops the branch's own import save path, its unresolved follow
tracking and saved label lookup, and the import's bulk lane receiver
discovery.

Kept on top of it: the import runs in the repository scope and stops
saving once the identity changes, isImportingContacts disables Select,
Import All and Continue, a successful import clears the pending import so
the overview moves on, each save checks its owner under the Paykit lock,
imported profiles fill the session profile cache, and prepareImport's
follow lookups stay on the interactive read lane.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: importing contacts stays busy for minutes on large lists

3 participants