Skip to content

feat: support Pubky signup - #1224

Open
ben-kaufman wants to merge 16 commits into
masterfrom
codex/pubky-ring-signup
Open

feat: support Pubky signup#1224
ben-kaufman wants to merge 16 commits into
masterfrom
codex/pubky-ring-signup

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

  • Accept app-authorized pubkyring://signup and auth-bearing pubkyauth://signup requests, plus direct pubkyauth://direct_signup and parameter-only legacy pubkyauth://signup, through the normal scanner and deep-link flow.
  • Register the wallet-derived Pubky identity with the requested Homeserver and optional signup token.
  • Require explicit approval and the existing PIN/biometric check for every signup format; show the requested homeserver before registration.
  • Restore locally owned identities by signing in on their existing homeserver, keeping credentials retryable after failures instead of registering at Homegate.
  • Activate the returned Paykit session and continue into the existing profile setup flow.
  • Reuse the approval sheet’s loading state while signup completes, report Already signed in when a local identity exists, and keep payment-only scanner state intact when rejecting Pubky requests.

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

  1. With no Pubky identity, scan a signup QR from staging Pubky App; approve it, confirm the website continues, and complete profile setup.
  2. With no Pubky identity, scan either direct signup format; confirm the homeserver is visible, Cancel performs no registration, and Authorize requires local authentication before profile setup opens.
  3. Scan a signup request while signed in and confirm the Already signed in state.
  4. Confirm payment-only scanners reject Pubky requests without clearing the current payment flow.
  5. With the wallet locked, open a signup deep link; confirm nothing registers or opens until unlocking, then the approval sheet appears.
  6. After signup on a non-default homeserver, make session recovery fail, restore connectivity, and retry Create/Restore Profile. Confirm it keeps the same homeserver. Repeat with a failed profile save and confirm retry never calls signup.

Validation:

  • Full testDevDebugUnitTest: 2,346 tests passed on the restacked branch.
  • compileDevDebugKotlin and detekt passed.
  • Regression coverage includes locked signup deep links, consent/local-auth ordering, real SDK activation and partial-persistence cleanup, stale pending state, and one-time profile navigation.
  • 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.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from d02c926 to 09dff52 Compare September 2, 2026 22:23
@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Greptile Summary

This 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.

  • Adds parsing and validation for pubkyring://signup requests.
  • Adds registration, requester authorization, session activation, and profile-setup continuation.
  • Preserves payment scanner state when Pubky requests are rejected and reports existing local identities.

Confidence Score: 3/5

This 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

Important Files Changed

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
Loading

Reviews (1): Last reviewed commit: d02c926 | Re-trigger Greptile

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
Comment thread app/src/main/java/to/bitkit/ui/ContentView.kt
@ben-kaufman ben-kaufman changed the title feat: support Pubky Ring signup feat: support Pubky signup Sep 2, 2026
@piotr-iohk

piotr-iohk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Regtest device QA, home Scan, QR from staging.pubky.app.

Staging e2e doesn’t finish — is that expected?
Signup QR does create a Bitkit identity and Create Profile (ring2 / pubky9m…). Staging stays on the Ring / QR step after Authorize. Sign-in QR from the same flow is rejected (only Pubky grant auth URLs are supported). So this isn’t e2e with the Pubky staging app — Bitkit gets a local profile, the website never continues. If approveRingAuth is supposed to complete that session, it’s a bug. If the site only listens for Ring, say so; Ben’s “QR → sheet → profile” case is then only the Bitkit half.

No spinner after scan.
Scanner closes, home looks unchanged for ~18s while register/activate runs. Feels like the scan did nothing, worse when staging also doesn’t move. “Deriving your keys…” on Create Profile is too late. Need a loader from scan until the profile screen.

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

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

@piotr-iohk Thanks for the device QA and logs. You found a real signup interop bug: Bitkit treated every pubkyauth://signup request as direct signup, even when Pubky App included relay, secret, and caps. I pushed a fix so auth-bearing signup requests now register the identity and then complete the relay authorization; parameter-only/direct signup still skips app authorization. Direct signup also shows progress while registration and activation run.

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?

@piotr-iohk

piotr-iohk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

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.

Screenshot 2026-09-03 at 16 50 53

bitkit_logs_2026-09-03_14-48-43.zip

Screen_Recording_20260903_164721_Bitkit.Regtest.mp4

piotr-iohk
piotr-iohk previously approved these changes Sep 3, 2026
@ovitrif

ovitrif commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

needs conflict resolution @ben-kaufman

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from 504ae04 to 94bc950 Compare September 3, 2026 19:03
ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ea18746 to f331ad8 Compare September 3, 2026 22:16
@ben-kaufman
ben-kaufman requested a review from ovitrif September 3, 2026 22:18
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-rc50-auth branch from 21d5a06 to a681dfc Compare September 4, 2026 12:24
@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from f331ad8 to ef49aa1 Compare September 4, 2026 13:19

Copy link
Copy Markdown
Contributor Author

I checked the failing pubky_paykit CI shard. It repeatedly fails the profile/contact-edit scenario with concatenated old and new text (ALICE WALLET AALICE WALLET A, Wallet A original notesEdited from wallet B), rather than failing Ring signup. The shared E2E helper calls clearValue() then setValue() without checking the clear took effect; the app's edit handlers replace the name/bio state directly and do not append it. This points to the Android input-clearing/automation path, but I have not reproduced the cause locally yet, so I am not calling this CI failure resolved or a proven harmless flake. The previously discussed signup-session preservation fix and its regression coverage are present; I resolved that answered review thread.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ef49aa1 to ff24be9 Compare September 4, 2026 22:15

Copy link
Copy Markdown
Contributor Author

Restacked onto the updated #1200 head (580b042fd) and pushed as ff24be924. All five signup commits are signed; range-diff confirms their patches are unchanged. The only resulting tree changes are the parent PR's SDK-version/changelog update. The previously reported profile-edit E2E issue is not being marked fixed by this restack; the new head still needs CI verification.

ovitrif

This comment was marked as resolved.

ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ff24be9 to f54f1f3 Compare September 6, 2026 15:04
@ben-kaufman ben-kaufman mentioned this pull request Sep 7, 2026
5 tasks

@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.

Re-reviewed the delta at dc72c36b5. The homeserver re-homing is fixed — trace on the resolved thread. No HIGH/MEDIUM.

Scoping note so it isn't misread: the canDeferStaleSession narrowing in this range is not part of this fix — it comes from the restacked parent (d8882611f refactor: remove paykit migration handling). I'm reviewing that separately on #1200 and will report there.

The test coverage here is stronger than the iOS twin's: these drive the real repo with the keychain mock, so they pin the actual wiring rather than an injected coordinator. never loadString(BIP39_MNEMONIC) in the restore test is a nice touch — it asserts the seed path isn't even touched when a stored key exists. One coverage note only: pubkyService is mocked, so the SDK-level persistSessionAccess deletion isn't exercised; I checked by reading that a signIn failure can't delete the key, so it's a gap in the net rather than a hole in the behaviour.

ovi-reviewer[bot]

This comment was marked as resolved.

@ovitrif
ovitrif dismissed their stale review September 7, 2026 20:20

already addressed

ovi-reviewer[bot]

This comment was marked as resolved.

@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.

Reviewed the delta at 540c6a455. No HIGH/MEDIUM — clean. I treated this as a remote-input-surface change since it registers a URL scheme.

The new route is runtime-toggled, not statically enabled — that was the distinction I most wanted to settle, because a manifest entry is normally live at install regardless of any runtime flag. It isn't here: the new pubkyring://signup filter sits inside the existing .ui.MainActivityPubkyAuth alias, which is android:enabled="false" in the manifest, and the only thing that flips it is PubkyAuthHandlerRegistrar.setAliasEnabled behind isPaykitUiEnabled && (!hasIdentity || hasSecretKey). So the new scheme is delivered under exactly the conditions pubkyauth:// already was. No new component, no change to exported, no change to MainActivity's own filters or to <queries>.

No lock or approval bypass. isProtocolUrl returns true for pubkyring + host signup, so the link takes the same branch as pubkyauth://: the Paykit flag and walletRepo.walletExists() gate, then launchScanenqueueDeferredScan when locked, and after unlock flushDeferredScanhandleScanhandlePubkyAuthshowSheet(Sheet.PubkyAuth). There's no path from this route to approveSignupAuth that skips the sheet.

Redaction holds for the new scheme. A pubkyring://signup?hs=..&relay=..&secret=.. link is logged twice, and both are safe: sanitizedDeeplinkLogValue drops query and fragment so the line is literally pubkyring://signup, and scanLogId falls through to sanitizedQrLogValue giving redacted#<8-byte sha256>. Nothing in this diff adds a new sink.

On collisions — claiming only the signup host is the right call. pubkyring://auth stays unclaimed, so Ring's own callback is untouched, and if both apps are installed Android shows the chooser, which is the intended "either app can perform signup" behaviour rather than a hijack.

The new PubkyAuthManifestTest is not trivially true. It resolves against the merged manifest via Robolectric's PackageManager, so it would catch a flavour override, and it pins both directions: the route must resolve while the alias is enabled and must resolve to nothing while disabled. Deleting the new filter or moving it onto a statically-enabled component both break it, which is exactly the pair of regressions worth guarding here.

Base automatically changed from codex/paykit-rc50-auth to master September 8, 2026 11:08
@jvsena42
jvsena42 dismissed piotr-iohk’s stale review September 8, 2026 11:08

The base branch was changed.

ovi-reviewer[bot]

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from 73328c7 to 6df1707 Compare September 8, 2026 13:30
ovi-reviewer[bot]

This comment was marked as resolved.

@ovi-reviewer

This comment was marked as resolved.

@ovi-reviewer

This comment was marked as resolved.

@ovitrif
ovitrif dismissed stale reviews from ovi-reviewer[bot], ovi-reviewer[bot], and themself September 8, 2026 18:16

addressed - reaudit confirmed

@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, 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. MainActivityPubkySignup is enabled="false", exported="true", with filter 1 = scheme pubkyauth x hosts {signup, direct_signup} and filter 2 = pubkyring+signup. Since Android merges <data> attributes per filter, that cross-product is exactly what's intended. PubkyAuthManifestTest resolves 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 (where handlePubkyAuth answers "Already signed in") and pubkyring://signup goes nowhere.
  • Key material. The identity is seed-derived, never generated. The hex secret is passed in-process to bootstrap().signUp and to bitkitcore approvePubkyAuth, and persisted only by persistSessionAccess into the AndroidKeyStore-backed Keychain. On logging: scanLogId hashes any non-SamRock QR, sanitizedDeeplinkLogValue drops query and fragment, and Logger.error renders only [Class='message']. Every new parse error message in PubkyAuthRequest.kt is value-free, and a URISyntaxException — whose message embeds the whole input — can't reach the signup path because isSignupUrl already requires URI(rawUrl) to succeed. One limitation worth stating: my local bitkit-core checkout predates the pubky module, so I could not read approvePubkyAuth's Rust error strings. That sink is shared with the pre-existing non-signup approveAuth path, 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 the homeserverPublicKey rendered on the sheet is the same field handed to registerIdentity. requestAuthorize pins state.authUrl == authUrl, transitionToAuthorizing and inFlightAuthorization are both CAS, and PubkyAuthorizationLocalAuth confirms only the pendingAuthUrl it was asked to authenticate.
  • Caps shown equal caps signed. Kotlin parseCapabilities drops only segments pubky-common's Capability::try_from also rejects, and any non-r/w action makes Rust reject the whole capability — so displayed is always a superset of granted. validateSignupRequest runs bitkitcore's parser on the constructed URL first.
  • Trust boundary. hs is 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/secret are never rendered and only reach the network via the constructed pubkyauth:///?relay=… handed to bitkitcore, which is pre-existing for ordinary auth URLs. x-bitkit-claim on a signup is rejected, and pubkyauth://signup?hs=X with a relay but no secret fails requiredSingle("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. processDeeplink requires Paykit on and a wallet to exist, routes through launchScan(allowPubkyAuth = true) which defers while locked, and approveSignupAuth is 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. approveRingAuth failing writes nothing locally and the remote account is retried via rc51's 409 path; activation failure runs clearRegisteredIdentityActivationLocked under NonCancellable; a failed pending-flag write runs forgetSessionAccess + clearLocalState; process death between activation and the pending write leaves the session restorable on next launch with the profile creatable from the Profile tab. createIdentity with a stored key goes straight to signIn, never Homegate.
  • Cancellation. The ViewModel is hiltViewModel() in the sheet host, a sibling of RootNavHost under setContent, so it's Activity-scoped — dismissing the sheet cannot cancel approveSignupAuth. All rollbacks are NonCancellable. _effects is a SharedFlow with extraBufferCapacity = 1, so a Dismiss with 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. activateBootstrapResult deletes PAYKIT_SDK_STATE when the key changes, and sharesPrivatePaykitEndpoints is reset before registration.
  • Scanner containment. Contact-payment, shop and webview scans pass allowPubkyAuth = false and are rejected before touching payment state.
  • Migration limit clause. The createIdentity restructure moved shouldRevokeSessionOnFailure = true from 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.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

This reuses the existing authorization sheet and Create Profile screen, with signup details and a homeserver card added to the sheet. ContentView adds navigation after signup. I could not find a Figma frame linked in the PR or repository, so I do not have a verified design reference to add.

ovi-reviewer[bot]

This comment was marked as resolved.

@ovi-reviewer

ovi-reviewer Bot commented Sep 8, 2026

Copy link
Copy Markdown

ben-kaufman: This reuses the existing authorization sheet and Create Profile screen, with signup details and a homeserver card added to the sheet. ContentView adds… (comment)

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.

ovi-reviewer[bot]

This comment was marked as resolved.

ovi-reviewer[bot]

This comment was marked as resolved.

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.

5 participants