feat: support Pubky signup - #1224
Conversation
d02c926 to
09dff52
Compare
Greptile SummaryThis PR adds Pubky Ring signup URL parsing and scanner routing, registers the wallet-derived identity with the requested Homeserver, activates Paykit, and resumes profile setup through durable local state.
Confidence Score: 3/5This PR should not merge until signup activation can recover from the second network operation failing and pending profile setup can be exited without an immediate navigation loop. The new signup sequence can complete remote registration and authorization while leaving Bitkit without a local session, and the successful path's durable pending marker makes the CreateProfile back action ineffective. Files Needing Attention: app/src/main/java/to/bitkit/services/PaykitSdkService.kt, app/src/main/java/to/bitkit/repositories/PubkyRepo.kt, app/src/main/java/to/bitkit/ui/ContentView.kt
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/models/PubkyAuthRequest.kt | Adds strict Ring signup parsing, query validation, and conversion into the existing Pubky authorization request model. |
| app/src/main/java/to/bitkit/repositories/PubkyRepo.kt | Coordinates signup registration, authorization, activation, and pending profile state, but the multi-step flow can strand remotely completed signup without a local session. |
| app/src/main/java/to/bitkit/services/PaykitSdkService.kt | Adds registration without activation, discarding an activatable signup result and requiring a failure-prone second sign-in. |
| app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt | Routes Ring signup through the normal scanner while explicitly rejecting Pubky requests in payment-only contexts. |
| app/src/main/java/to/bitkit/ui/ContentView.kt | Resumes pending profile setup automatically, but conflicts with the unchanged dismissible CreateProfile back action. |
| app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalViewModel.kt | Dispatches Ring signup approval, handles existing identities, and dismisses the approval sheet before profile setup. |
Sequence Diagram
sequenceDiagram
participant Scanner
participant Approval as Approval UI
participant Repo as PubkyRepo
participant Server as Homeserver
participant App as Requesting app
participant Paykit
Scanner->>Approval: Pubky Ring signup request
Approval->>Repo: Approve signup
Repo->>Server: Register derived identity
Repo->>App: Approve authorization
Repo->>Paykit: Sign in and activate session
Paykit-->>Repo: Active local session
Repo-->>Approval: Dismiss approval
Repo->>Repo: Mark profile setup pending
Repo-->>Scanner: Navigate to profile setup
Reviews (1): Last reviewed commit: d02c926 | Re-trigger Greptile
|
Regtest device QA, home Scan, QR from staging.pubky.app. Staging e2e doesn’t finish — is that expected? No spinner after scan. Already signed in toast works and the scanner closes (iOS leaves it up — noted on #724). bitkit_logs_2026-09-03_09-44-43-android.zip Screen_Recording_20260903_113712_Bitkit.Regtest-android.mp4 |
|
@piotr-iohk Thanks for the device QA and logs. You found a real signup interop bug: Bitkit treated every The ordinary sign-in QR rejection is separate. These PRs use Paykit rc50’s app-scoped grant auth model, while staging Pubky App is still generating the older auth request format. Pubky App needs to update its sign-in flow to the new grant model for ordinary sign-in to work with Bitkit. Could you please recheck the staging signup path on this head? |
447e781 to
36bfd02
Compare
174e4c8 to
504ae04
Compare
|
Retested the rebased head on a physical Samsung S22 using the regtest build and a signup QR from staging.pubky.app. Signup now completes end to end: authorization progress is visible, Bitkit creates the Pubky identity and opens profile setup, and the staging website continues successfully. The logs confirm the authorization completed and the local identity/session were created. Ordinary sign-in still returns Authorization failed because staging currently generates the older non-grant authorization request. As clarified, that is outside the scope of this signup PR. Non-blocking UI parity note: the approval screen differs between platforms. iOS (left) always shows the placeholder profile card, while Android (right) omits it because no profile exists yet. It would be good to align the intended design, but I don’t think this should block the signup fix.
bitkit_logs_2026-09-03_14-48-43.zip Screen_Recording_20260903_164721_Bitkit.Regtest.mp4 |
|
needs conflict resolution @ben-kaufman |
504ae04 to
94bc950
Compare
ea18746 to
f331ad8
Compare
21d5a06 to
a681dfc
Compare
f331ad8 to
ef49aa1
Compare
|
I checked the failing |
ef49aa1 to
ff24be9
Compare
|
Restacked onto the updated #1200 head ( |
ff24be9 to
f54f1f3
Compare
73328c7 to
6df1707
Compare
There was a problem hiding this comment.
utACK, left a few inline nits.
Coverage
Total: 54%
- Journeys: 83% - Five of the six QA journeys are directly exercised by scoped regression tests; website continuation is supported only by prior device QA
- Unit tests: 80% - Nine changed behavior surfaces have focused tests; the approval-sheet composition and PubkyService delegates lack direct changed tests
- QA: 0% - Manual Tests not run
Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
addressed - reaudit confirmed
jvsena42
left a comment
There was a problem hiding this comment.
One LOW inline, diagnostics-only. Not blocking.
Gating: every entry point needs isPaykitEnabled (DataStore default false, written only from Dev Settings) and PaykitFeatureFlags.isUiAvailable. Nothing in .github or the justfile sets PAYKIT_UI_DISABLED, so the UI is available in a mainnet release build but off until the Dev Settings toggle. Both manifest aliases ship android:enabled="false" and are flipped only by PubkyAuthHandlerRegistrar. So: reachable only by someone who has opened Dev Settings and enabled Paykit.
Delta since my last pass: 0ff1a0129 merges master with no conflict-resolution edits, and 6df1707e1 "separate pubky signup and authorization routes" answers the thread on PubkyAuthHandlerRegistrar.kt:95. Over-correction check passed — the auth-alias predicate is now byte-identical to the pre-PR merge-base (isPaykitUiEnabled && hasIdentity && hasSecretKey), so session-only Ring users are advertised exactly as before, and the new signup alias claims only pubkyauth://signup, pubkyauth://direct_signup and pubkyring://signup. Nothing else was introduced. None of my threads were pending.
Checked and clean:
- Manifest split.
MainActivityPubkySignupisenabled="false",exported="true", with filter 1 = schemepubkyauthx hosts {signup,direct_signup} and filter 2 =pubkyring+signup. Since Android merges<data>attributes per filter, that cross-product is exactly what's intended.PubkyAuthManifestTestresolves against the merged manifest via Robolectric in all three states and pins that with the auth alias enabled,pubkyauth://signup*goes to the auth alias (wherehandlePubkyAuthanswers "Already signed in") andpubkyring://signupgoes nowhere. - Key material. The identity is seed-derived, never generated. The hex secret is passed in-process to
bootstrap().signUpand to bitkitcoreapprovePubkyAuth, and persisted only bypersistSessionAccessinto the AndroidKeyStore-backedKeychain. On logging:scanLogIdhashes any non-SamRock QR,sanitizedDeeplinkLogValuedrops query and fragment, andLogger.errorrenders only[Class='message']. Every new parse error message inPubkyAuthRequest.ktis value-free, and aURISyntaxException— whose message embeds the whole input — can't reach the signup path becauseisSignupUrlalready requiresURI(rawUrl)to succeed. One limitation worth stating: my local bitkit-core checkout predates the pubky module, so I could not readapprovePubkyAuth's Rust error strings. That sink is shared with the pre-existing non-signupapproveAuthpath, so it isn't new exposure, but it is unverified rather than cleared. - Authorization / TOCTOU. Display state is built from
parseAuthUrl(authUrl); approval re-parses the same immutable string with pure functions, so thehomeserverPublicKeyrendered on the sheet is the same field handed toregisterIdentity.requestAuthorizepinsstate.authUrl == authUrl,transitionToAuthorizingandinFlightAuthorizationare both CAS, andPubkyAuthorizationLocalAuthconfirms only thependingAuthUrlit was asked to authenticate. - Caps shown equal caps signed. Kotlin
parseCapabilitiesdrops only segments pubky-common'sCapability::try_fromalso rejects, and any non-r/waction makes Rust reject the whole capability — so displayed is always a superset of granted.validateSignupRequestruns bitkitcore's parser on the constructed URL first. - Trust boundary.
hsis format-checked only, with no allowlist — you declined that after my earlier thread and mitigated by rendering the homeserver plus the trust warning, so I'm not reopening it.relay/secretare never rendered and only reach the network via the constructedpubkyauth:///?relay=…handed to bitkitcore, which is pre-existing for ordinary auth URLs.x-bitkit-claimon a signup is rejected, andpubkyauth://signup?hs=Xwith arelaybut nosecretfailsrequiredSingle("secret")rather than silently downgrading to direct signup. - Manifest exposure. Both aliases are exported, so any installed app can fire the intent while an alias is enabled — but the most it gets is the sheet.
processDeeplinkrequires Paykit on and a wallet to exist, routes throughlaunchScan(allowPubkyAuth = true)which defers while locked, andapproveSignupAuthis reachable only via Authorize ->RequestLocalAuth-> PIN/biometric (or the tap alone when neither is configured, consistent with the rest of the app). Both are pinned by tests. - Lifecycle and partial failure.
approveRingAuthfailing writes nothing locally and the remote account is retried via rc51's 409 path; activation failure runsclearRegisteredIdentityActivationLockedunderNonCancellable; a failed pending-flag write runsforgetSessionAccess+clearLocalState; process death between activation and the pending write leaves the session restorable on next launch with the profile creatable from the Profile tab.createIdentitywith a stored key goes straight tosignIn, never Homegate. - Cancellation. The ViewModel is
hiltViewModel()in the sheet host, a sibling ofRootNavHostundersetContent, so it's Activity-scoped — dismissing the sheet cannot cancelapproveSignupAuth. All rollbacks areNonCancellable._effectsis aSharedFlowwithextraBufferCapacity = 1, so aDismisswith no collector is dropped, which is harmless because CreateProfile navigation is driven by the persisted pending flag rather than the effect. - Cross-identity.
hasIdentity()checks in-memory key plus both keychain entries, so a session-only Ring identity blocks a second seed-derived signup.activateBootstrapResultdeletesPAYKIT_SDK_STATEwhen the key changes, andsharesPrivatePaykitEndpointsis reset before registration. - Scanner containment. Contact-payment, shop and webview scans pass
allowPubkyAuth = falseand are rejected before touching payment state. - Migration limit clause. The
createIdentityrestructure movedshouldRevokeSessionOnFailure = truefrom the shared path into the Homegate branch only. I checked what else that assignment carried — nothing but revoke-on-failure for a freshly created Homegate session, which is exactly what it was meant to confine. No migration finding.
One observation, not a finding: the registrar keys on the in-memory _publicKey while the runtime check uses the keychain, so after a RestorationFailed the signup alias is advertised and the app answers "Already signed in". The outcome is correct and it predates this delta.
Cross-repo: the alias split is Android-only by construction — iOS registers URL schemes statically in Info.plist and can't runtime-gate them, so the "advertised for links it can't handle" concern is structural there rather than a bug. Worth a note on synonymdev/bitkit-ios#722 rather than a fix. The LOW below should also be checked against synonymdev/bitkit-ios#724 approval-failure handler, which I haven't re-reviewed this round.
|
This reuses the existing authorization sheet and Create Profile screen, with signup details and a homeserver card added to the sheet. |
There was a problem hiding this comment.
utACK
Coverage
Total: 67%
- Journeys: 100% - The changed behavior has a direct regression for rejection followed by a later incoming request
- Unit tests: 100% - Both prior findings are covered by focused unit-test changes in the scoped delta
- QA: 0% - Manual Tests not run
Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill
Confirmed: no Figma source exists for these reused screens, and this delta changes no UI layout, copy, or resource. The design-reference note is settled. |
There was a problem hiding this comment.
utACK, left a few inline nits.
Coverage
Total: 0%
- Journeys: 0% - The diagnostics-only superseded-request path is not exercised by a test in this delta
- Unit tests: 0% - No unit test observes the relocated error log for a superseded approval failure
- QA: 0% - Manual Tests not run
Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill
jvsena42
left a comment
There was a problem hiding this comment.
The consent bypass I raised last round is fixed. Verified independently at head rather than taking the diff's word for it:
handleDirectPubkySignup,isDirectSignupUrlandisCompletingPubkySignupno longer exist anywhere inapp/src/main.approveSignupAuthhas exactly one production caller —PubkyAuthApprovalViewModel:189insideapproveRequest, reachable only fromauthorize():173←confirmAuthorize():159.registerIdentity,approveRingAuthandactivateRegisteredIdentitylive inside it with no other callers. There is no remaining path from a scanned, pasted or deeplinked URL to registration that skips the sheet or local auth.- The wallet-exists guard is now inside the
isProtocolUrlbranch (AppViewModel.kt:4668-4677), so the old ordering bug is gone, andlaunchScan:1929-1932enqueues while!_isAuthenticatedwithflushDeferredScan:2048refusing until authenticated — the PIN bypass is closed. handlePubkyAuth:4696-4717has no approval branch left;pubkyauth://signup?hs=with no relay/secret/caps now takes the same sheet path, sinceauthorizesApponly affects parsing.- The consent chain is pinned end to end:
requestAuthorizeCASes onstate.authUrl == authUrl && state == Authorize, local auth routes throughAuthCheckView/BiometricsView,confirmAuthorizeCASes oninFlightAuthorization, andauthorize()re-parses the same immutable URL.PubkyAuthApprovalViewModelTest:193-229runs all three formats and assertsnever { approveSignupAuth }before consent and after cancelling local auth. homeserverPublicKeyis rendered (PubkyAuthApprovalSheet.kt:411-418,testTag("PubkySignupHomeserver")).- The signup hosts moved to a
MainActivityPubkySignupalias withandroid:enabled="false", flipped only byPubkyAuthHandlerRegistrar.kt:67(isPaykitUiEnabled && !hasIdentity).
One new LOW inline, dev/QA-facing.
| if (!approveRequest(request, authUrl)) return | ||
|
|
||
| Logger.info("Auth approved for '${request.serviceNames.firstOrNull().orEmpty()}'", context = TAG) | ||
| if (request.isSignup) { |
There was a problem hiding this comment.
Low, and dev/QA-facing today (isPaykitEnabled defaults false) — but this early return leaves the VM parked in Authorizing for that URL.
The ordinary path two lines below sets ApprovalState.Success; the signup branch returns without touching _uiState, so it stays {authUrl = url, state = Authorizing} from transitionToAuthorizing:255-265. The PubkyAlreadySignedInError exit at :304 does the same, returning before the _uiState.update { Authorize } at :307.
That matters because resetForLoad:267-278 returns false when the URL matches and the state is Authenticating/Authorizing, so load:63 returns without parsing. inFlightAuthorization was already cleared in the finally at :161, so the in-flight restore at :55-62 doesn't apply either. Result: AuthorizingContent:360-374 renders a buttonless spinner. Not a lockout — back still dismisses via approvalBackAction:264 → dismiss():321-323 — but dismiss() never resets state, so every re-open of that URL is the same spinner for the life of the Activity. The VM is Activity-scoped (bare hiltViewModel() in ContentView.kt:571-575, a sibling of RootNavHost), so it survives sheet close/reopen and rotation.
Reachable because direct-signup URLs carry no nonce: parseSignup only requires hs, and relay/secret/caps are needed only when authorizesApp is true. So pubkyauth://direct_signup?hs=<key> from a printed or static QR is byte-identical on every scan — the VM's own test at :194-200 uses exactly those static strings. The gate re-opens after Sign out (ProfileScreen.kt:82 → clearLocalState:1397-1404 clears both keychain keys, so hasIdentity() is false and the already-signed-in toast no longer intercepts). pubkyring://signup is unaffected — it carries a per-request secret.
Settle the state before dismissing on both paths. Emit Dismiss first and update after, so the signup path doesn't flash SuccessContent for a frame on its way into Create Profile. Alternatively — and this covers any future terminal path — scope resetForLoad's Authorizing refusal to a genuinely live authorization: inFlightAuthorization.get()?.authUrl == authUrl. load:55-62 already handles the truly in-flight case.
There was a problem hiding this comment.
Changed the reload guard so a completed signup URL can return to consent instead of reopening the spinner. Local authentication and an authorization that is still in flight remain protected from duplicate loads.
ovitrif
left a comment
There was a problem hiding this comment.
manual review of delta, re-approved
There was a problem hiding this comment.
QA Notes
QA_1 and QA_2 Pixel 10 Pro emulators on Android 16/API 36; exact-head dev build 2.4.1 (188).
-
✅ passed: Scanning the live staging Pubky App QR registered the identity, advanced the website to profile setup, and completed the Bitkit profile.
-
✅ passed: Scanning a direct-signup QR showed the homeserver; Cancel left the wallet unsigned in, and Authorize required the local PIN before profile setup.
-
✅ passed: A signup request while signed in showed “Already signed in” and preserved the existing profile.
-
✅ passed: The payment-only scanner rejected the Pubky QR and retained the Bitcoin send flow.
-
✅ passed: A locked wallet queued the signup deep link until unlock, then showed the approval sheet without prior registration.
-
⛔️ failed: Offline cold-start session recovery reproducibly terminated Bitkit with a
NullPointerExceptioninAppViewModel.toastatAppViewModel.kt:4519. Restoring connectivity recovered the same Pubky and non-default homeserver, and the profile-save retry completed without another signup call.
Tip
The app must remain running after a recovery failure so the user can restore connectivity and retry.
Coverage
Total: 94% (delta since 3dac138, 5 files)
- Journeys: 100% - PubkyAuthApprovalViewModelTest.kt covers terminal success, already-signed-in dismissal, duplicate local authentication, and active authorization reload.
- Unit tests: 100% - The changed reload predicate has focused assertions for both terminal branches and its active-operation guard.
- QA: 83% - 5 of 6 Manual Tests passed on Pixel 10 Pro Android 16 emulators.
Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill
jvsena42
left a comment
There was a problem hiding this comment.
15aca5075 closes it. You took the second option — scoping the Authorizing refusal to a live inFlightAuthorization rather than settling the state on the terminal paths — and it works for both branches at once, since the guard keys on the in-flight record instead of on which branch was taken.
Trace: after either terminal path returns, the finally at :161 CASes inFlightAuthorization back to null. Re-opening the same URL then hits load:55 (in-flight restore skipped) → resetForLoad:63 → state is Authorizing but inFlightAuthorization.get()?.authUrl == authUrl is null == authUrl, so isAuthorizing is false → CAS to {authUrl, Loading} at :278 → parseAuthUrl → Authorize with real buttons. No SuccessContent flash either, since neither path touches _uiState.
No new door opened: resetForLoad(A) is unreachable while A is genuinely in flight because load:55-62 returns early on the same condition, and a second approveSignupAuth is independently blocked by the untouched inFlightAuthorization.compareAndSet(null, …) at :149 plus initializeMutex and the hasIdentity() re-check in PubkyRepo.approveSignupAuth:1006-1013. A UI-state reset can't bypass those.
terminal signup outcomes allow the same URL to reload for consent (PubkyAuthApprovalViewModelTest.kt:240-270) is a real regression test — revert the :270-271/:274 hunk and it fails on both the assertEquals(Authorize, …) at :261 and the parseAuthUrl verification at :262, and it loops both Result.success and PubkyAlreadySignedInError.
One sibling case inline.
| if ( | ||
| currentState.authUrl == authUrl && | ||
| currentState.state in setOf(ApprovalState.Authenticating, ApprovalState.Authorizing) | ||
| (currentState.state == ApprovalState.Authenticating || isAuthorizing) |
There was a problem hiding this comment.
Low, dev/QA-facing — the Authorizing half is fixed, but Authenticating can strand a URL the same way, via a dismissal the VM never hears about.
With PIN enabled: Authorize → requestAuthorize:104-111 sets Authenticating → AuthCheckView overlays inside the sheet's Box (PubkyAuthApprovalSheet.kt:80-98, :149-164). If the user then taps the scrim, swipes down, or presses system back, none of PubkyAuthApprovalSheet.kt, AuthCheckView.kt or BiometricsView.kt declares a BackHandler or DisposableEffect, so the event goes straight to SheetHost.kt:181-195 → onDismiss → ContentView.kt:499 appViewModel.hideSheet(), bypassing the VM entirely. cancelLocalAuth is only wired to AuthCheckView.onBack / BiometricsView.onFailure (:160, :175), neither of which fires on host-driven dismissal.
So the VM stays {authUrl, Authenticating} with no pending prompt, and unlike the Authorizing branch there is no in-flight record to invalidate it — this guard returns false on the flag alone. Re-scanning the same static direct-signup URL renders AuthorizingContent: spinner, no PIN pad, no buttons, for the life of the Activity. Same symptom you just fixed, reached from the sibling state. Biometrics is mostly immune, since the system prompt's cancel drives onFailure → cancelLocalAuth.
The new test at :217-222 pins the Authenticating refusal as intended but doesn't cover host-driven dismissal, so it passes either way.
Smallest fix keeps the guard and releases the state when the overlay goes away — in PubkyAuthorizationLocalAuth:
DisposableEffect(Unit) { onDispose { pendingAuthUrl?.let(viewModel::cancelLocalAuth) } }Alternatively collapse Authenticating → Authorize from the sheet host's onDismiss.
There was a problem hiding this comment.
Dismissing the approval sheet now clears its pending authentication state. Reopening the same URL returns to consent and requires local authentication again.
|
Moved toast initialization before the startup collectors, so an immediate session-recovery error can show its toast without crashing the app. |





















Description
pubkyring://signupand auth-bearingpubkyauth://signuprequests, plus directpubkyauth://direct_signupand parameter-only legacypubkyauth://signup, through the normal scanner and deep-link flow.This PR is stacked on #1200 and uses its Paykit rc51 authorization model. Ordinary Pubky App sign-in must use that grant-auth model; compatibility with the older sign-in request is intentionally outside this signup PR.
Preview
Not included; this reuses the existing scanner, authorization approval sheet, loading treatment, and profile setup UI.
QA Notes
Validation:
testDevDebugUnitTest: 2,346 tests passed on the restacked branch.compileDevDebugKotlinanddetektpassed.PubkyRepoTest.kt: 97 tests passed, including existing-key recovery, secure-storage errors, sign-in/profile failure retries, cancellation, and no-key signup. Detekt reports no changed-file findings.