test: add marketplace wallet journey - #1220
Conversation
This comment has been minimized.
This comment has been minimized.
b296556 to
496b457
Compare
496b457 to
0a0f7c8
Compare
piotr-iohk
left a comment
There was a problem hiding this comment.
QA LGTM on the identifiers + journey contract. Did not re-run the isolated marketplace fixture.
Checked latest (0a0f7c8b):
- Cited IDs exist on this branch:
PubkyAuthWatchOnlyConsent/Approve/Authorize/OK,ContactPaymentsToggle,PaymentRequestsSheet,PaymentRequestRow-<id>,PaymentRequestPay-<id>,ReviewAmount,ReviewContactRecipient,GRAB,SendSuccess,ActivityAmount,ActivityTxDetails. xmllint --noout journeys/pubky-marketplace/wallet-leg.xmlpasses.- Compose identifier tests in
PaymentRequestsScreenTestmatch the hyphenated tags. - CI green, including
e2e-tests-staging - pubky_paykit.
Notes, not blockers:
- Merge #1217 first. This PR’s
PaymentRequestRow-/PaymentRequestPay-tags are a subset of #1217 (that one also addsPaymentRequestDismiss-<id>). - PR body QA notes cite request
5db852f6-…/ txa3a427ca…. README records different accepted runs (ad1a8463-…, then selector replayb7f67854-…). Fine if those are separate replays — worth one line so we do not treat it as a mismatch later. - README says #1209 “preserves rejected incoming requests as visible history.” On #1217, parse rejects stay log-only; the visible-history / retry path is the explicit Pay terminal toast. Wording is slightly off.
I am not blocking on not replaying the Locks/Paykit Server fixture here. The journey is documentation of an accepted 2026-09-02 run, and the app-side selectors it needs are present.
0a0f7c8 to
9552ab5
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Selector-parity pass looks right — I checked every tag the journey uses against bitkit-ios and all of them resolve there except one. Two notes below.
Also: the README pins Paykit Server 867fc883, the adb port maps and the homeserver key, but not the marketplace driver / Locks harness, so nobody else can replay this end to end yet. Is there a ref you can pin, like hardware-wallet/README.md does with bitkit-docker?
9552ab5 to
419503d
Compare
|
@jvsena42 I pinned the marketplace driver and Locks harness to |
6df11ed to
04d0720
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Two findings inline, both on the precondition block. Doc-only, so neither blocks.
Scope note: the title says test: but three production files change. I checked each against master rather than against my previously reviewed head, because the rebase makes the raw diff misleading:
PubkyAuthApprovalSheet.kt— only twotestTagadditions (PubkyAuthAuthorize,PubkyAuthOK). The extra ~10 lines versus what I last reviewed are the requester-label row from master'secb0499e9/75e1b4805, carried in by the rebase, not by this PR.git range-diffshows all six previously reviewed commits patch-equivalent.PaymentRequestsScreen.kt— hyphenated row tag plus a newPaymentRequestPay-<id>;onClickandenabledunchanged.SettingsScreen.kt—switchTestTagrename only, still inside the sameisPaykitEnabled && isPubkyAuthenticatedguard.
So it is genuinely test-hook work and the consent gate is untouched. Worth stating explicitly since a consent-sheet change under a test: heading is exactly what gets waved through.
Delta since my last pass (ba75c85e8, 04d0720f8): XML only — "Return the buyer wallet to Home" and moving the bell check after the auto-review is dismissed. Both address ben-kaufman's threads exactly without over-correcting, and the code agrees: auto-presentation is blocked while currentSheet != null but not by the current route, and the bell exists only on Home, so being on Home before the purchase is what makes it reachable after back.
Checked and clean:
- Consent gate unchanged.
onAuthorizestill guarded byuiState.authUrl == authUrl;ContentrendersLoadingunlessisCurrentRequest; Authorize ->requestAuthorize->RequestLocalAuth-> PIN/biometrics ->confirmAuthorize, withLocalAuthMode.Noneonly when neither PIN nor biometrics is enabled. The requesterclientIdand the full permission list still render, nothing truncated. NoBuildConfig, flag or injected fake anywhere in the diff — only tag strings. No auto-approve or auto-dismiss path:Dismissfires only fromviewModel.dismiss()on user Cancel/OK/back. - Scope pinning.
matchesWatchOnlyAccountCapabilitiesrejects any capability set other than exactly the two watch-only paths, withInvalidBitkitClaimCapabilitiesthrown before the Authorize screen renders — so step 15's vaguer wording can't wave through an over-scoped grant even though the text is loose. - Back on the auto-review behaves as the journey says.
canGoBackis false atConfirm, so back routes tohideSheet()->clearActiveContactPaymentContext(retry = true), whose defer condition is already false becauseonSheetVisiblecalledmarkPresentedon show. Nothing re-presents ahead of the bell tap.dismissEnabledis false only during HW signing or an unresolved broadcast, neither of which applies pre-swipe. Dismissal never rejects —rejectis called only from the Dismiss button,acceptonly after send — so the request stays in_pendingRequestsand the bell, row and Pay all render. - Pay tag placement matches the README's "paid request remained as history without Pay or Dismiss": the button row renders only when
onPay != null || onReject != null, and history rows pass neither. - Every tag in
wallet-leg.xmlresolves at head — the fourPubkyAuth*ids,ContactPaymentsToggle,Contact_<pk>,PaymentRequestsBell,PaymentRequestsSheet,PaymentRequestRow-/PaymentRequestPay-,ReviewAmount,ReviewContactRecipient(insideOnChainDetails, behind the show-details toggle, so step 34 is accurate),GRAB,SendSuccess,ActivityAmount/ActivityTxDetails. A sweep found no un-hyphenatedPaymentRequestRowreference left anywhere. - Step 19 is accurate: a clean wallet completing in-app profile creation ends with
setEnabled(true)on contact payments, so "verify enabled" holds for the standard flow. - Trust boundaries: nothing newly rendered from counterparty data; the requester row that arrived from master is bounded with
maxLines = 1and ellipsis.
Cross-repo with synonymdev/bitkit-ios#720: 30/30 actions in identical order once id->testTag is normalised. The remaining divergences are the platform-appropriate back wording, and action 4 — iOS now enumerates /pub/paykit/v0/bitkit/server and /pub/paykit/v0/private/bitkit/server with READ, WRITE, where this file still says "only the requested Paykit capabilities". The behaviour is identical on both platforms, so this is now the weaker file and the enumeration is worth porting here rather than weakening iOS.
Also stale here: the README still says PaymentRequestPay-<payment-request-id> is Android-only and that an iOS buyer must find its Pay control by accessible label. synonymdev/bitkit-ios#721 supersedes that.
On why this PR needs production changes when iOS needs none — iOS master already carried every identifier the journey uses and Android didn't, so this is pure instrumentation catch-up. That asymmetry is expected, not a smell.
jvsena42
left a comment
There was a problem hiding this comment.
Clean — no findings.
Reviewed as an authorization change, not a test change: the test: label understates it, since three production files change and one of them is the auth consent sheet. Worth stating so it isn't waved through on the label — but also so it isn't over-escalated, because the verdict is that only testTag strings changed.
Consent sheet (PubkyAuthApprovalSheet.kt) — two tag additions, no behaviour change:
:355.testTag("PubkyAuthAuthorize")on the Authorize button,:434on the success button. NoonClick, state, effect, flag,BuildConfigor injected-fake path changed.- Requester pinning intact end to end:
onAuthorizeguardsuiState.authUrl == authUrl(:85);ContentrendersLoadingunlessisCurrentRequest(:208);requestAuthorizerejects unlessstate.authUrl == authUrl && state == Authorize(VM:104);transitionToAuthorizingre-checks the pin under CAS (VM:237-243);authorize()re-parses the same pinned string and bails if it moved (VM:170);validatedApprovalClientIdthrowsRequesterChangedif the approved clientId differs from the displayed one (PaykitSdkService.kt:1021-1026). - Local auth still sits between the tap and the grant (
RequestLocalAuth→ PIN/biometrics →confirmAuthorize);Noneonly when neither PIN nor biometrics is configured (:192-196). Disclosure unchanged: requester row:388, full permission list:465,TrustWarning:398. Nothing truncated or reordered.
SettingsScreen.kt:346 — switchTestTag = "ContactPaymentsToggle", still inside if (state.isPaykitEnabled && state.isPubkyAuthenticated) (:339). Nothing on the ungated settings path changed.
Journey verified against the merged code, not the pre-merge trace. The two merge commits brought a 214-line master delta into PaymentRequestsScreen.kt (#1217/#1186: card-level onClick, title now displayContact.name, onReject→onDismiss, subscriptions) and rewrote AppViewModel — which is exactly where steps 17-21 could have silently broken. They didn't:
- All 18 tags resolve at head, PascalCase and exact.
- Step 4's wording is what actually renders:
displayPathstrips the trailing slash (PubkyAuthRequest.kt:54),rw→"READ, WRITE"(:57-63), andmatchesWatchOnlyAccountCapabilities(:26) pins the set to the twobitkit/serverpaths, so over-scope fails before the Authorize screen. - Step 8 holds: profile creation routes to
PayContacts, whosecontinueToProfilecallssetEnabled(true)(PayContactsViewModel:43). - Step 18 holds post-merge: system back →
SheetHostBackHandler→hideSheet()→clearActiveContactPaymentContext(retry = true), which defers only ifrequestedPaymentRequestId == id(null for auto-presented) or the id is inautomaticPendingRequests()(which excludes presented ids). No re-presentation, and the bell still reads raw_pendingRequests, which stays populated because dismissal never rejects. - Step 20 holds: subtitle is
subscriptions.nameFor ?: note ?: date(:502), so "note when present" is exact.
Parity with synonymdev/bitkit-ios#720: 30/30 actions, identical order and selector names; remaining divergences are platform-appropriate (testTag vs id, "Press Android back" vs "Tap back"). iOS touches only journey files because its master already carried most identifiers, whereas Android lacked PubkyAuthAuthorize, PubkyAuthOK, PaymentRequestPay-<id>, the hyphenated row tag and the ContactPaymentsToggle name. So the production edits here are instrumentation catch-up, and the asymmetry is expected rather than suspicious.
Gating: SettingsStore.kt:42 defaults isPaykitEnabled false, written only by the Dev Settings toggle — dev/QA-facing today.
|
@ovitrif conflicts |
Closes #1212
This PR:
Acceptance evidence covers the full controlled journey: seller watch-only claim, reciprocal linked-contact setup, fresh buyer funding, canonical Paykit delivery, payment approval, zero-confirmation detection, broadcast, one-block confirmation, paid request history, and Locks completion. This branch merges Android master
e2fa21f86; the approved original recording and acceptance evidence were captured at0a0f7c8b. A later replay at pre-rebase headcbc88a40reached two fresh Android identities, a funded buyer, and seller authorization, where Paykit Android rc51 rejected the pinned fixture's legacypubkyauth://signinrequest because rc51 requires grant auth. Pre-merge head04d0720f8corrects the reviewed request-bell sequence. Merge headb122caa6fpreserves the request-row navigation from merged #1186 while retaining the reviewed hyphenated row and Pay-action identifiers. Current head031314514incorporates mastere2fa21f86, including the #1242 screens-map update, without further conflicts. The current-head manual replay remains unchecked.Description
PaymentRequestRow-<id>and addsPaymentRequestPay-<id>.PubkyAuthAuthorize,PubkyAuthOK, andContactPaymentsToggleidentifiers.ed03a32e, cites merged upstream Paykit Server POC wake2pay from notification #2 for canonical request behavior, and cites merged Android feat: send paykit payment proofs #1178 for incoming-request swipe behavior.Preview
bitkit-android-1212-journey.mp4
QA Notes
Manual Tests
5db852f6-d47a-43c4-8b17-fb7a654ac4dd→ review the accepted fallback seller identitypubky…apfiy, 15,000 sats, and 141-sat fee → swipe once → verify transactiona3a427ca69a3f1d3f26fb47f38182c8543fdbd997227ddb8a2dec1ef0e927166at zero confirmations → mine one regtest block → verify confirmed Android activity, paid request history without a Pay action, confirmed Paykit delivery, and completed Locks purchase.cbc88a40created sellerpubky86…gfhdz4yand buyerpubkyr7…tu75yby, and the buyer received 1,000,000 sats in transaction21709c35a60fe081b1de0faeaae1ca403ec67cee76587262ccc6e4b37be6e840, confirmed at fixture height 16,424. At seller authorization, Android resolved and delivered the pinned server'spubkyauth://signinrequest, then Paykit Android0.1.0-rc51rejected it withcode=protocol_error, context=only Pubky grant auth URLs are supported. The pinned server at867fc883uses Paykit source6b241878/ Pubky0.8.0and has nosignin_grantsetup path, while rc51 embeds Pubky0.11.0grant auth. The request, payment, and confirmation boundaries require a compatible marketplace fixture.The checked replay is separate from the initial and selector-specific acceptance runs in
README.md; its request and transaction identifiers are intentionally distinct. The Preview remains historical evidence until manual test 2 replaces it with a current-head recording.Automated Checks
PaymentRequestsScreenTest.kt: all five focused Compose tests passed at the original reviewed head, including the incoming row and Pay identifiers.just compile,just test, andjust lint: passed for pre-merge head04d0720f87d2a11b0cbbb5f12426016baa6c80b2; detekt reported only existing baseline findings outside this PR.git diff --checkandxmllint --noout journeys/pubky-marketplace/wallet-leg.xml: passed at pre-merge head04d0720f87d2a11b0cbbb5f12426016baa6c80b2.b122caa6fand031314514bring in master throughe2fa21f86and resolve the request-row conflict.04d0720f87d2a11b0cbbb5f12426016baa6c80b2with the isolated local-backend routing; APK SHA-256:8180be69e01c93c6e41f0f54a3e9f3a042e3565cd77c7ad358e0a39c47667fa0.just e2eat the pre-merge head reachedvalidateSigningDevReleaseand stopped because no release keystore was configured in that worktree. The debug-signed E2E variant above is the device-test artifact.b122caa6fand031314514, at user direction.e21ce4b0bea14e42c0bb437cd8d443deebe6e2ba40488c0b24dbaaad54ec05ec.