fix: save contacts without repeated remote lookups - #1395
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
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
saveContactmerges 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.
isImportingresets infinally.restorePrivateConnection = trueunblocks a contact previously deleted, and re-blocks on failure.- The journey files are identical to the iOS twin.
piotr-iohk
left a comment
There was a problem hiding this comment.
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
yw5xyandyw5xgare 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.
piotr-iohk
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
No new findings. The own-key thread is fixed at faae845.
Checked and clean:
prepareImportdrops the own key viaPubkyPublicKeyFormat.canonicalized, covering exact, prefix-stripped and final-symbol padding aliases. Decoding the…yw5xy/…yw5xgfixtures gives the same 32 bytes, while…yw5xwdecodes to a different key, so the mask merges aliases without merging distinct keys.canonicalizedcannot throw.PaykitPublicKeys.normalizechecks length and alphabet without decoding and is wrapped inrunCatching, so a malformed follow falls through to the existing placeholder path.- The preview is the only source for Import All and Import Selected, so
saveContactnever receives the self identity. PublicPaykitReposync-failure logs carry only the endpoint identifier and error text.PayContactsViewModelunwraps a wrappedPublicPaykitError. The retry copy's "contacts unchanged" claim holds, sinceenable()androllbackEnablednever touch contacts.- Import buttons are guarded against re-entry,
isImportingresets infinally, and the pending import survives cancellation for retry. - iOS twin synonymdev/bitkit-ios#851 compares with
matchesinstead ofcanonicalized. 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 makescanonicalizedhere 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
left a comment
There was a problem hiding this comment.
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
CreateProfileViewout of scope for that wait.
Follow-up bugs for those two gaps are drafted and will be linked when filed.
|
Follow-ups from the 61-contact Ring import:
|
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.
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
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. Companion iOS change: synonymdev/bitkit-ios#851.
Manual Tests
Automated Checks
PubkyRepoTest.kt— prepared profiles save with remote lookups unavailable, duplicates are skipped, partial failure preserves successful saves, and retry saves missing contacts.ContactImportOverviewViewModelTest.kt— pending import prevents duplicate requests and cancellation clears progress.ContactImportSelectViewModelTest.kt— selected profiles are passed through without re-fetching.PayContactsViewModelTest.kt— wrapped session errors retain guidance and unknown failures offer retry without claiming payments are enabled.just testpassed all 3,044 tests with no failures or skips.just compile,just lintandjust buildpassed. 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.