Repository navigation
fix: import Shop contacts and refresh on return - #1485
Conversation
ben-kaufman
changed the title
|
@ovitrif Could you or the Shop/mobile reviewers run the remaining checkout acceptance checks on this draft? Ben has no disposable buyer/seller session available and asked that reviewers test the flow. The importer is now on Use an isolated buyer/seller fixture and record the app SHA, SDK version, device, request ID and observed transitions, without secrets. For the actual Shop request, stop at payment review: no funding, mining, approval or broadcast is requested. The payment-priority case needs a controlled non-broadcast fixture; leave it explicitly unrun if that capability is unavailable. The forced published-artifact run passed all 4,005 unit tests; full lint and compiler diagnostics match baseline. That does not establish checkout acceptance. Please leave this PR draft until the relevant real checkout evidence is recorded, with any remaining cases clearly identified. iOS twin and matching QA request: synonymdev/bitkit-ios#934. |
|
@ben-kaufman Android results for 3a0630f (paykit-android 0.1.0-rc76), a One bug blocks the scan: Test 1: First-time seller on cold open, open app and resume ✅A new seller with a valid storefront and marker was imported and linked without opening Contacts on all three paths: cold open (import 60 s after a launch that waited for the debugger, link at 21:02:59 UTC), app already open (marker at 21:03:39, import on the same poll, link at 21:03:58) and resume (nothing while backgrounded, import 5 s after resume, link 25 s after resume).
Test 2: Shop marker and seller Payment Request ✅The staging Shop wrote the buyer's marker for seller
Test 3: More than 100 markers ❌With 117 markers (100 for keys with no homeserver, 7 valid sellers), only the valid sellers near the start of the list were imported, and the three at positions 28, 54 and 116 were still missing 23 minutes later, longer than a full pass took on iOS, because of the reset described above.
Test 4: Label, removal and block preservation ✅An edited label and two removed sellers (removal blocks the peer; there is no separate block action) stayed as left through six polls that re-sent all of them to the SDK import and through a relaunch; the in-flight edit variant wasn't run.
Test 5: Invalid data ✅A malformed marker, a marker naming another seller, the buyer's own key, a key with no homeserver, and storefronts with the wrong owner or record type imported nothing over 75 s of polls, and after the wrong-owner storefront was fixed the seller was imported and linked 5 s later.
Test 6: Interrupted discovery and refresh ⏭️Not run: the retry path keeps its state in the cursor, Test 7: Stale-read and identity fencing ⏭️Not run for the same reason as Test 6; it needs a rerun after the reset fix. Test 8: Payment-priority handling ⏭️Not run: it needs a payment submission in flight, and this fixture can't submit one without funding and broadcast. |
BitcoinErrorLog
left a comment
BitcoinErrorLog
left a comment
There was a problem hiding this comment.
Reviewed at 3a0630fb against what the Shop writes (pubky/pubky-marketplace#220, on master). One bug blocks it. Everything else covers what the Shop needs.
Blocking: the importer still restarts on every poll (Ovi's Case 3). ShopContactImporter.importPage resets cursor, handled and refreshPending when owner !== signIn. PubkyRepo.currentSignIn() returns a new PubkySignIn(publicKey, generation) on every call, so the reset runs on every poll. As a result, sellers past the first ten markers are never imported, the same sellers go to importContacts and trigger a contact reload every 10 s, and the retry state that Cases 6 and 7 depend on is wiped each time. Suggested fix: compare by value, not by reference:
if (owner?.publicKey != signIn.publicKey || owner?.generation != signIn.generation) {ShopContactImporterTest misses this because it reuses one signIn val. Please add a test that passes two separate but equal PubkySignIn instances (the way currentSignIn() does), then asserts that the second call lists from the first page's cursor and doesn't send already handled sellers to the import again. After the fix, please ask Ovi to rerun Cases 3, 6 and 7.
Matches the Shop
- Path and format: lists
/pub/pubky.app/migrations/seller-auto-follow/v1/, accepts only<z32>.json, and requiressellerto match the file name andcompletedAt > 0. This is exactly what the Shop writes ({"seller":"<z32>","completedAt":<ms>}, seller without apubkyprefix).ignoreUnknownKeys = truemeans a future extra field won't break parsing. - When it runs: on foreground (when polling starts, the
combineemits), on reconnect (connectivity is in thecombine), and on each existing payment-request poll. There's no new timer, and it stops when polling stops. - Idempotent and respects removals: the SDK
importContactsskips existing contacts, the buyer's own key andBlockedpeers in one transaction. Bitkit'sremoveContactcallsblockPeerbeforeremoveContact, so a seller the user removed stays removed. - No clobbering: new contacts get
label = null, and existing records, labels, tags and overrides aren't touched.
Not blocking
- A seller is imported only when they have a
shop.json. Shop #222 (open) writes a default one for every seller who takes bitcoin, so this needs no Bitkit change. - Unreachable sellers aren't cached, so every pass fetches them again (Ovi measured 70–80 s per page of unreachable keys on iOS). Real buyers have few markers, so this can wait. A short in-memory negative cache would make a pass cheaper.
- Optional test: a marker with an extra unknown key still imports.
|
@BitcoinErrorLog @ovitrif Fixed in The existing paging and interrupted-refresh tests now allocate fresh sign-ins and both fail on the preceding production code. Added one table for real identity/generation changes. Final Please rerun Cases 3, 6 and 7 on this head with the existing isolated staging fixture: deep-page progress and wraparound without duplicate imports; interrupted commit/refresh recovery; stale-read, background and identity fencing. Record the exact head and transitions. These emulator checks have not been claimed from the unit tests. No funding, mining or payment approval/broadcast is requested. Case 8 stays explicitly unrun without a non-broadcast fixture, and the PR stays draft. |
|
Resolved the conflict with current master in c7accdf. Kept master's Bitkit Core 0.8.0-rc6 and this PR's Paykit 0.1.0-rc76; the existing Shop importer/polling fix and merged USDT changes are preserved. Normal signed merge, no history rewrite; all five PR commits are GitHub-verified. Validation: Still draft: the requested current-head Cases 3/6/7 and remaining device acceptance checks are not replaced by these unit tests. No new checkout or latency claim. |
@BitcoinErrorLog @ben-kaufman Cases 3, 6 and 7 pass on Android. Case 3 ran on 4c1c28c and on the current head c7accdf. Cases 6 and 7 ran on 4c1c28c; the merge leaves Test 3: More than 100 markers ✅On 4c1c28c, a buyer with 117 markers (110 keys with no homeserver, plus 7 sellers at positions 24 to 117) got all 7 sellers in one 11-minute pass. The cursor moved ten markers per poll, and each seller reached the SDK import exactly once. A seller added before the cursor was imported as soon as the scan restarted. The next full pass sent no handled seller to the import again, and the reset branch never ran between polls. On c7accdf, a second buyer got all 7 the same way, including two sellers whose markers landed before the cursor.
Test 6: Interrupted discovery and refresh ✅Discovery cut offline before its listing imported nothing, and the seller came in on the first poll after reconnecting. With the refresh cut offline right after the SDK saved a seller, the importer kept
Test 7: Stale-read and identity fencing ✅A contact read held from before an import showed 11 contacts; after it was released, the next read showed all 12. An import held before its save while the wallet was reset ended in
Test 8: Payment-priority handling ⏭️Not run: it needs a payment submission in flight, and no non-broadcast fixture exists. |
BitcoinErrorLog
left a comment
BitcoinErrorLog
left a comment
There was a problem hiding this comment.
Re-reviewed at c7accdf1. My blocker is fixed, so I'm approving.
Fixed: the importer no longer restarts on every poll. In 4c1c28c2, ShopContactImporter.importPage compares publicKey and generation instead of object identity. The fresh PubkySignIn that currentSignIn() returns on each poll now keeps the cursor, handled and refreshPending, and a real account switch or a new sign-in generation still starts a fresh scan. The master merge in c7accdf1 doesn't touch ShopContactImporter, PubkyRepo or the importer tests.
The tests cover both directions:
- The paging test now passes a new but equal
PubkySignInon each call. It checks that the first page is listed only twice in four polls (once at the start and once on the rescan after the empty last page), and that each seller goes toimportShopContactsexactly once. The old!==check fails this. - The interrupted-refresh test retries with a new equal instance, so a pending refresh now survives across polls, which is what Cases 6 and 7 rely on.
- The new
identity or generation change starts a fresh scantest covers both a new generation for the same key and a different key. It checks that the second scan starts again from the first page, never lists from the old cursor, and fetches the already handled seller's storefront again.
Ovi's emulator runs confirm it: Cases 3, 6 and 7 pass. Case 3 passed on 4c1c28c2 and on c7accdf1 (117 markers, all 7 sellers imported once each, including sellers inserted before the cursor, with no reset between polls). Cases 6 and 7 passed on 4c1c28c2, and the importer and PubkyRepo haven't changed since.
Not blocking: as on iOS, Case 8 (import deferred while a payment is being submitted) hasn't been run because there's no fixture for it. The negative-cache suggestion from my earlier review still applies.
This PR is still a draft. @ben-kaufman, please take it out of draft when you're ready, since we need it for RC2.
|
Thanks, I've recorded the Cases 3/6/7 retest and current-head approval in the PR body. I also checked that Case 8 is still unrun, and I haven't waived it or counted unit tests as an emulator pass. Keeping the draft until the remaining acceptance checks are settled. The negative-cache suggestion can stay a separate performance follow-up with bounded expiry, so temporarily unavailable sellers are not skipped indefinitely. |
 [Medium impact] The changes affect the Paykit contact and payment-request workflow. Fix both cancellation bugs before merging. FindingsSummaryAdds Shop seller discovery and refreshes shared contacts on foreground entry and contact links.
Acknowledged scope from DiagramsequenceDiagram
participant App
participant Importer
participant Pubky
participant SDK
participant Contacts
App->>Importer: Foreground or existing poll
Importer->>Pubky: Read marker page and storefronts
Pubky-->>Importer: Valid sellers
Importer->>SDK: Import missing, unblocked contacts
Note over SDK: Wait while payment submission is active
SDK-->>Importer: Imported records
Importer->>Contacts: Start a fresh contact read
App->>Contacts: Refresh before contact-link routing
Reviews (1) · Last reviewed commit: "chore: merge current master into shop co..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
@ben-kaufman @BitcoinErrorLog Case 8 on Android at c7accdf: the import waits correctly while the payment sheet stays open (8a passes). But leaving the payment sheet ends the deferral while the payment is still being submitted (8b fails). That also blocks the identity-change branch (8c not run). Nothing was broadcast. Bug:
Also, from the code (not a device run): Android defers only while an incoming request is being paid ( Setup:
Hold and abandon:
Each video shows the phone next to a timeline of debugger breakpoints and logcat lines, in UTC, at 2x. Test 8a: Import waits while a Payment Request is being paid, then runs ✅With request
Test 8b: Leaving the payment sheet keeps the import waiting ❌With the same request held from 14:25:08, seller s2's import waited at the gate (14:25:23). Pressing Back on the payment sheet (14:25:30) cleared the submission flag. The import wrote s2 160 ms later, while the payment was still held before its transaction was built.
Test 8c: Identity change during the wait ⏭️Blocked by 8b. Every identity change I found (Delete Profile) means leaving the payment sheet, which already ends the wait. After the profile was deleted (14:26:56) and the payment released (14:27:50), no import ran for the old identity. Rerun after the 8b fix. The video is 8b's.
|
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P1). Imports Shop seller markers into the buyer's Paykit contacts and refreshes contacts on foreground and contact deep links. Reviewed c7accdf1, full tier.
What I checked, and the candidates I ruled out
Read in full: ShopContactImporter.kt, PubkyRepo.kt (contact region and signOut), the AppViewModel.kt hunks with startPaykitPaymentRequestPolling, awaitPubkyDeeplinkInitialization and the scan job, PaykitSdkService.importShopContacts, PubkyService additions, ContentView.kt lifecycle observer
Call sites traced: refreshContacts (foreground collector, deep link), importShopContacts (poll loop, foreground collector), stopPaykitPaymentRequestPolling (ON_STOP, dispose)
CI: build, lint, detekt, build-local green; e2e shards still running when posted
Not built here: the branch's Gradle build is left to CI on an auto review.
Ruled out
- Cursor advanced past an unsaved page: the cursor and
handledonly move whenrefreshPendingis cleared, and a thrownSignInChangedleaves both untouched. - Import write during payment submission:
importShopContactsre-checksisPaymentSubmissionActiveinside the operation lock and loops. - Stale import after identity change:
isStillCurrentis checked inside the lock right before the SDK call, and the SDK import is bound toexpectedIdentity. - Refresh outliving sign-out:
signOutcancels and clearscontactRefresh. - Cold reader: it did not return within this run, so the finding is unchallenged by a second reader.
Merge confidence: 3/5, the P1 on a contact link opened while connectivity changes.
This comment has been minimized.
This comment has been minimized.
|
Updated in 2f07d8c. The shared-refresh and queued-import cancellation findings are fixed, including the real PubkyService wrapper. A queued cancelled caller cannot start an SDK write, while an already-admitted write still finishes safely. The reproduced payment-sheet issue in #issuecomment-6195035283 is also fixed: the import gate follows the running software submission, not sheet visibility. Incoming requests, contact payments and plain sends keep imports paused after Back until the operation returns or is actually cancelled. A held-submit table covers all three; duplicate confirmation stays blocked. The existing USDT cancellation test also checks dismissal without releasing the gate. No payment was broadcast.
For the bounded device follow-up, please rerun 8b/8c and queued-import cancellation through the actual foreground/background or disable caller, then Case 9. The interim Test 8 hook cancels a Job inside PaykitSdkService; that does not by itself prove cancellation propagates through the old PubkyService wrapper. Stop before broadcasting. Existing active QA need not be duplicated. The Test 5 report shows invalid data rejected and corrected data discovered after 21.5 s. That delay remains performance evidence to investigate, not proof that validation/retry failed. Please retain the timing separately from the functional result; this PR adds no progress UI or checkout-latency deadline. It does not claim that performance is fixed. |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
Re-review of 2f07d8c8 after c7accdf1. 1 earlier finding: 1 fixed. 0 new findings. Full tier on the diff since the last review.
- One cancelled caller cancels the shared contact refresh: fixed.
refreshContacts()now counts waiters undercontactsLockagainst the exactDeferred, and only the last departing waiter cancels and clears it;cancelling one contact refresh does not cancel another waitercovers the foreground-plus-contact-link case.
What I checked, and 4 candidates I ruled out
Read: git diff c7accdf..2f07d8c, plus refreshContacts, signOut's refresh reset, onConfirmPay, sendPaykitUsdt, hideSheet, cancelHardwarePaymentRequestIfNeeded, onHardwarePaymentSubmissionChanged and its only caller HwSendSignScreen
Not built here: the branch's Gradle build is left to CI on this track.
Ruled out
- Waiter count inherited by a newer refresh: a new
Deferredresets the count to 0 before its own caller increments it, and an older caller'sfinallyonly decrements whencontactRefresh?.second === refresh, so a stale completion cannot clear or cancel the newer read. - Software submission flag stuck on: it is set in
onConfirmPayandsendPaykitUsdtimmediately before atrywhosefinallyclears it; theUNDISPATCHEDlaunch closes the gap where a secondPayConfirmedcould slip in, andviewModelScopecancellation still runs thefinally. - Dismissing the sheet ending the pause (Case 8b):
clearActiveContactPaymentContextstill setsisSubmittingPaymentRequest = false, but the setter now ORs inisSoftwarePaymentSubmissionActive, so the repo flag stays up until the payment coroutine ends. - Cancelled queued import still writing:
importShopContactsnow drops the queue'sJob(queueContext.minusKey(Job), the same form as the existing read wrapper), so a caller cancelled before admission never reachesimportContacts; an admitted write still completes undercompleteSdkCall, covered by the two newPaykitSdkServiceTestcases.
Merge confidence: 4/5, 0 open findings, but build and the e2e shards were still running when this was posted.
|
@ben-kaufman @BitcoinErrorLog Following up on the 2f07d8c fix: Case 8 passes on Android at 2f07d8c, which is still the head. Leaving the payment sheet no longer releases a waiting Shop import. The import now waits until the payment itself ends (8b). With the sheet open, it still writes once after the payment (8a). A profile delete during the wait ends the import with no write (8c). Nothing was broadcast. Setup:
Hold and abandon:
Each video shows the phone next to a timeline of steps, debugger breakpoints and logcat lines, in CEST, at 2x. Test 8a: Import waits while a payment is held, then runs once ✅With the payment held from 18:30:33, s1's import stopped at the payment gate (18:30:48) and made no write or link call for 109 s. After the abandon, the submission ended (18:32:37.9), and the import wrote s1 once (18:32:38.2) and linked it (18:32:43.8).
Test 8b: Leaving the payment sheet keeps the import waiting ✅With the payment held from 18:34:26, s2's import waited at the gate (18:34:55). The first Back stepped the sheet from Confirm to Amount, and a second Back closed the sheet (18:35:14). The import then kept waiting, with no write, link call or new poll, for 95 s. It wrote s2 only after the submission ended (18:36:49.9 end, 18:36:50.1 write, 18:36:59.6 link).
Test 8c: Identity reset while the import waits on a payment ✅With the payment held from 18:37:54, s3's import was waiting at the gate (18:38:15). I closed the sheet and deleted the buyer's profile (confirmed 18:38:53, contacts deleted 18:39:01, sign-out 18:39:05). The waiting import job was cancelled at 18:39:19.6 without writing s3. After the abandon (18:40:21.6), no import, SDK write or link call ran in the next 64 s. One more thing showed up at 18:39:19.8: the sign-out's Paykit teardown (
|
BitcoinErrorLog
left a comment
BitcoinErrorLog
left a comment
There was a problem hiding this comment.
Approval stands at 2f07d8c8.
- Case 8b: fixed. The import gate now follows the running submission, not whether the sheet is visible. Pressing Back on the payment sheet keeps a waiting Shop import paused until the payment itself ends. Ovi's run on
2f07d8c8passes 8a, 8b and 8c, and nothing was broadcast. - Cancellation: fixed.
importShopContactskeeps the caller's Job, so an import cancelled before admission never writes. Only the last waiter cancels the shared contact refresh, so a contact link survives a foreground cancel. Each fix has a regression test. - CI is green on
2f07d8c8.
ovitrif
left a comment
ovitrif
left a comment
There was a problem hiding this comment.
Approving at 2f07d8c on the device evidence:
- Cases 1, 2, 4, 5 and 3, 6, 7 after the importer fix
- Case 8a/8b/8c on this head: Back no longer releases the import mid-payment, and nothing was broadcast
5 participants
This PR imports Shop seller markers into the buyer's shared Paykit contacts and refreshes contacts on foreground entry and contact deep links.
Closes #1482. Twin: synonymdev/bitkit-ios#934. SDK: pubky/paykit-rs#194 and RC76.
Description
Ready for review. Remaining real Shop-to-Bitkit checkout validation is still open before merge. Large histories, unavailable data and existing link safety waits can require multiple polls; no fixed checkout-latency or complete checkout claim.
Out of Scope
No Shop implementation, new UI, automatic public follows, peer unblocking, payment submission, protocol/schema change, shortened safety wait or deployment. Import is not payment-acceptance evidence.
Design
N/A - no UI changes.
Preview
Reviewer staging checkout and scan recordings.
QA Notes
Historical staging results on
3a0630fb/ RC76: Cases 1, 2, 4 and 5 passed with the qualifications in the linked report; Case 3 exposed the per-poll reset.4c1c28c2acorrects that reset with red-before/green-after tests.Reviewer emulator retests now report Cases 3, 6 and 7 passing on a Pixel 7 / Android 15 devDebug regtest build against staging, using debugger breakpoints to inspect transitions:
4c1c28c2and currentc7accdf1: 117 markers, seven valid sellers imported once each, ten-marker page progress without per-poll reset, wraparound and discovery of markers inserted before the cursor. The first pass took 11 minutes on4c1c28c2; this is correctness evidence, not fast checkout or a current-head latency guarantee. A forced short intermediate page remains unverified.4c1c28c2: offline discovery recovered on the next poll; a refresh interrupted after save retained its cursor and pending refresh, refreshed after reconnect and ended with one row per seller.4c1c28c2: an older contact read was followed by a fresh read, and a held pre-save import rejected wallet reset withSignInChangedand no SDK write. The new identity saw no old contacts. Restoring the first identity reset its scan once and imported the held seller. These importer and PubkyRepo sources are unchanged by thec7accdf1master merge; this is not a claim that all payment code is unchanged.The Shop reviewer approved
c7accdf1after these results. The subsequent non-broadcast Case 8 run passed while the payment sheet remained open, but reproduced import resuming after Back while payment submission was still running; 8c was not reached. The running-operation gate now fixes that boundary with regressions for plain/contact/request sends. Device 8b/8c and caller-level queued cancellation must be retested on the new head. The later hook-based cancellation pass cancels inside PaykitSdkService, not the parent PubkyService caller. Remaining acceptance stays open.Repeated lookups of unreachable sellers remain a non-blocking performance follow-up. A negative cache would need bounded expiry and recovery checks so a previously unavailable seller can still be imported promptly; no cache or polling change is included here.
Journeys
pubky-contact.xml: saved/unsaved/self routing, PIN protection and warm background-to-contact-link return.Manual Tests
The shared marketplace journey fixture starts with linked contacts and does not supply marker/history seeding or controlled interruption. The reviewer used separate staging fixtures and debugger breakpoints. The checklist below retains unrun variants rather than treating every historical case label as a full current-head pass. Matching full steps are in
journeys/pubky-marketplace/shop-contact-import.mdon both platforms.Automated Checks
ShopContactImporterTest.kt: paging/rescan with freshly allocated equal sign-ins, marker/storefront validation, retry, coalescing, cancellation/identity changes and post-commit refresh recovery. The paging and pending-refresh cases fail before the fix; a separate table checks reset on a different key or generation.just compile,just test,just lintpass; 4,200 tests across 239 suites, zero failures/errors/skips. Four tests added, none removed: shared-reader cancellation, queued caller cancellation, admitted-write completion, and held software submission across dismissal. All 1,399 Detekt SARIF findings match the verified master baseline after normalizing only roots/line offsets. Compiler/format diagnostics introduce no findings. Suspend stubs follow the repository lambda convention. No dependency, safety-wait or payment-authorization change.5df9b5e63df1f8b825003f07d612377d7c9e14f7d1ef74da11b5dd4673292e70, matching the independently downloaded GitHub Packages artifact and its checksum. Generated Kotlin matches the validated SDK binding, all four native ABIs are present, and no local Maven override remains. RC76 source651695fcc07633f6df9d392ac761c71a412ed865contains corrected directory cursor encoding/order, atomic import and SDK master throughb0115c947df7c41d34f422b149cff23fbd882cd8. Dependency pins are unchanged by the polling fix. RC74 and RC75 remain untouched.