feat: support Pubky signup - #724
Conversation
Greptile SummaryThis PR adds Pubky Ring signup URL parsing, wallet-derived identity registration, authorization approval, session activation, and resumable profile setup through the existing scanner and approval UI.
Confidence Score: 3/5The PR should not merge until Ring signup can recover from intermediate failures and users can leave pending profile setup without being immediately redirected back. The new flow can strand a remotely registered identity when approval or sign-in fails, and its navigation observer creates a repeatable Create Profile trap while setup remains pending. Files Needing Attention: Bitkit/Managers/PubkyProfileManager.swift, Bitkit/MainNavView.swift
|
| Filename | Overview |
|---|---|
| Bitkit/Managers/PubkyProfileManager.swift | Adds Ring signup and pending-profile state, but the signup sequence is not recoverable after an intermediate failure. |
| Bitkit/MainNavView.swift | Adds automatic profile-setup resumption, but route-driven reevaluation prevents users from leaving the setup screen. |
| Bitkit/Models/PubkyAuthRequest.swift | Adds strict parsing and validation for Ring signup parameters and reconstructs the corresponding authorization URL. |
| Bitkit/ViewModels/AppViewModel.swift | Routes Ring signup requests through the existing scanner while preserving restricted payment-flow state. |
| Bitkit/Views/Sheets/PubkyAuthApproval/PubkyAuthApprovalSheet.swift | Extends the approval sheet to execute signup and transition into profile setup, with partial-failure recovery delegated to the manager. |
| Bitkit/Services/PubkyService.swift | Adds low-level registration and Ring authorization operations used by the new signup sequence. |
Sequence Diagram
sequenceDiagram
participant R as Pubky Ring request
participant B as Bitkit scanner
participant H as Homeserver
participant A as Auth relay
participant P as Profile setup
R->>B: pubkyring://signup
B->>B: Parse and show approval
B->>H: Register wallet-derived identity
H-->>B: Registration complete
B->>A: Approve authorization
A-->>B: Authorization complete
B->>H: Sign in
H-->>B: Active session
B->>P: Navigate to Create Profile
Reviews (1): Last reviewed commit: "feat: support Pubky Ring signup" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ben-kaufman
changed the title
|
Regtest device QA, home Scan, QR from staging.pubky.app. Staging e2e doesn’t finish — is that expected? No spinner after scan. Scanner stays up after already-signed-in / invalid auth. bitkit_logs_2026-09-03_09-45-08-ios.zip ScreenRecording_09-03-2026_11-38-24_ios-compressed.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? |
91555ae to
436ee06
Compare
|
Retested the rebased head on a physical iPhone 13 using the regtest build and a signup QR from staging.pubky.app. Signup now completes end to end: Bitkit shows progress while processing the request, creates the Pubky identity, opens profile setup, and the staging website continues successfully. Ordinary sign-in still fails because staging currently generates the older non-grant authorization request. As clarified, that is outside the scope of this signup PR. The original signup interoperability and missing-progress issues are resolved for me. bitkit_logs_2026-09-03_14-36-23-ios.zip ScreenRecording_09-03-2026.16-31-36_1-ios.MP4 |
436ee06 to
4eedebb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
692eccc to
0ae29a6
Compare
|
@jvsena42 On sheet replacement, both the authorization button and PIN callback start unstructured |
The base branch was changed.
|
Needs conflict resolution |
|
Merged |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Both fixes reviewed at 16f31299. Redaction landed cleanly; the wrapped-scheme guard closes the two forms I reported but is narrower than bitkit-core's own unwrapping — refinement on the existing thread, still LOW and gated.
Attribution, because the diffstat is misleading. 143 files differ from e509cb34, but 133 are byte-identical to origin/master (the 5ea3cf3d merge bringing in #697/#685/#686/#711/#735 and a swiftformat pass). The actual new authorship since my last review is git diff e509cb34 13dbd5d6 --stat = 3 files, +37/−1. That was the whole review surface.
Fix (b) landed. PubkyAuthApprovalSheet.swift:449-453 logs the fixed string on the signup branch; the interpolating line survives only for non-signup. I walked the rest of the Ring-signup path for other frames that might re-interpolate — approveSignupAuth, completeSignupAuthentication, approveSignupWithTimeout, PubkyService.registerIdentity/activateRegisteredIdentity — none do. The :307 interpolation is on the legacy createIdentity path and predates the PR. Note the toast at :454 still shows error.localizedDescription on screen; that wasn't part of the finding (UI, not logs), flagging it only so it's a decision rather than an oversight.
Fix (a) is partial — details on the thread. Short version: the guard compares the app stripper's output to the raw input, but decode is bitkit-core, which unwraps lightning: and bitkit:// itself. I verified this at the exact pinned revision (Package.resolved → 890502f2 = v0.5.14) rather than from memory. Two wrappers still reach handlePubkyAuthApproval: lnurl:lightning:pubkyauth://… (because removingLightningSchemes is a single ordered pass that tests lightning: first and never revisits it) and bitkit://pubkyauth://…. Suggested refinement is to check the decode output at :737 instead of trying to out-guess the stripper — no change to removingLightningSchemes or onOpenURL, which I still think should stay as they are.
The #697 regression check came back clean, which was the thing I most wanted to rule out now that #697 has landed on master:
Env.keychainGroup(Env.swift:123-127) is still network-suffixed;git diff e509cb34 head -- Env.swiftis empty.cf0b8c03did remove the"Pubky grant client ID"prefix check fromshouldDeferStaleSession— but that predicate was already identical ate509cb34, so nothing changed on this branch.- More importantly the precedent scenario can't fire here:
clientID(:1207-1212) keys onEnv.network, andEnv.network(:131-142) is regtest for Debug and bitcoin for Release. Different network means a different keychain group and a different client ID; same network means an identical client ID. So there's no same-entry mismatch producible across a Debug-over-TestFlight install at this head — the thing that bit #697 is structurally absent here.
Merge fidelity: faithful. git show --cc 16f31299 shows one conflict hunk (AppViewModel.swift ~:949), and comparing against git merge-tree --write-tree 13dbd5d6 5ea3cf3d the head differs only there, where the resolution is the union of both sides (releaseContactPaymentContext from the PR, hasSendPaymentTarget from master). No line differs from both parents anywhere else.
Everything I cleared at e509cb34 — fund paths, key material, authorization/TOCTOU, trust boundaries, lifecycle, cancellation, cross-identity — is untouched by both the fix and the merge.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Fix confirmed at 38c01421 — both wrappers are closed, and you took the decode-output approach rather than trying to out-guess the stripper.
AppViewModel.swift:738-743 guards case let .pubkyAuth on PubkyAuthRequest.isProtocolURL(rawUri) and throws otherwise, which catches anything bitkit-core unwrapped regardless of which layer did it. Traced the four cases:
- bare
pubkyauth://signup?…→isProtocolURL(rawUri)true → passes. Legit path intact. lightning:pubkyauth://…→ still caught earlier at:489.lnurl:lightning:pubkyauth://…→:489skipped as before (the stripper's single ordered pass leaveslightning:in place), decode yields.pubkyAuth, new guard sees a raw URI with schemelnurl→ throws.bitkit://pubkyauth://…→:489skipped, Rust'sreplace+ recursion yields.pubkyAuth, new guard throws.
Good catch releasing the contact payment context in the guard — that matches what :485-488 does and avoids leaking a claimed context on the throw path.
I checked it doesn't over-correct, which was the main risk with a guard this broad. The legitimate bitkit://pubky-auth/... Ring callback never reaches here: MainNavView.swift:386 intercepts it via PubkyRingAuthCallback.parse and returns before handleScannedData, and that parser requires scheme == "bitkit" && host == "pubky-auth" (PubkyProfileManager.swift:43), which isProtocolURL would reject. Different entry point, unaffected.
The test list covers more orderings than I named — lnurl:lightning:, lnurlp:lightning:, lnurlw:lightning:, lnurlc:lightning:, lightning:lightning:, bitkit://, and bitkit://bitkit://lightning: — plus a case asserting the contact payment claim is released on rejection.
That closes everything I had open on this PR. Clean from my side.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-checked at 96fd5058. Master merge only, no new authored commit — the merge brought in #720, #732 and #734.
Both halves of the wrapped-scheme fix survived intact, which is what I wanted to confirm on a merge this size:
AppViewModel.swift:483let rawUri = uriand:489the raw-vs-stripped guard:738the decode-output guard oncase let .pubkyAuth, with the contact-context release beneath it
git show --cc 96fd5058 produces no combined-diff lines, so nothing was resolved by hand and there's no evil-merge surface to explain.
Nothing to re-review; my confirmation stands.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reaudit e509cb3..96fd505: a merge of master into the branch. Comparing the PR's net patch before and after the merge, 3 of its 17 files differ and the other 14 are byte-identical, so the conflict resolution is the whole of the reviewable change. AppViewModel.swift keeps master's hasSendPaymentTarget alongside this branch's resetSendState(preservingContactPaymentContext:), and adds a raw-input guard on the decoded .pubkyAuth branch so a bitkit://-wrapped request is rejected: removingLightningSchemes() does not strip that prefix, so the earlier guard cannot fire on it. PubkyAuthApprovalSheet.swift keeps the authorizing-state dismissal lock together with the signup-specific error logging that omits the raw error. ShopPaymentRequestTests.swift keeps both the payment-scope and the wrapped/decoded Pubky rejection suites. The Debug simulator build succeeds. No findings.
QA Notes
- ⏭️ skipped: tested on e509cb3, recording
- ⏭️ skipped: tested on e509cb3, recording
- ⏭️ skipped: tested on e509cb3, recording
- ⏭️ skipped: tested on 51100a3, screenshot
- ⏭️ skipped: tested on 51100a3, screenshot
- ⏭️ skipped: tested on e509cb3, recording
Approve.
Replies
ben-kaufman: @jvsena42 On sheet replacement, both the authorization button and PIN callback start unstructured
Taskinstances… (comment)
That reading matches the code at this head, and it does not need a change here. performAuthorization re-checks sheets.pubkyAuthApprovalSheetItem?.request.rawUrl == config.request.rawUrl before it calls sheets.hideSheet(), so an in-flight leg belonging to a replaced sheet cannot dismiss or activate over a newer request, and approveSignupWithTimeout bounds that leg at 30 seconds. The uncancelled Task therefore runs to completion against a stale request and its result is discarded rather than applied.
ben-kaufman: Merged
masterand resolved the conflict, preserving the signup and payment changes. (comment)
Confirmed. Comparing the PR's net patch before and after the merge, 3 of the 17 files differ and the other 14 are byte-identical. AppViewModel.swift keeps master's hasSendPaymentTarget alongside this branch's resetSendState(preservingContactPaymentContext:), PubkyAuthApprovalSheet.swift keeps the authorizing-state dismissal lock alongside the signup-specific error logging, and ShopPaymentRequestTests.swift keeps both the payment-scope and the wrapped Pubky rejection suites. The Debug simulator build succeeds.
Coverage
Total: 67% (delta diff since e509cb3, 3 files)
- Journeys: 0% - The delta is a merge-conflict resolution with no user-visible behaviour change, so no journey was added or exercised.
- Unit tests: 100% -
ShopPaymentRequestTests.swiftcovers both merged halves: payment-scope rejection and wrapped/decoded Pubky rejection including the contact-claim release. - QA: 100% - 6 of 6 Manual Tests passed in the QA phase
Reviewed by Claude Code (claude-opus-5-high) via gh-pr-review-loop skill
feat: open pubky auth links
There was a problem hiding this comment.
Reaudit 96fd505..902ff39, 19 files. External deep-link delivery moves off MainNavView.onOpenURL onto a SceneDelegate + DeepLinkRouter retain-and-replay path, and the outbound Ring handoff switches to pubkyring. The routing body moved verbatim into handleDeepLink(_:), and the new .task(id:) re-fires when the node reaches .running, so a retained node-requiring URL still routes. normalizedProtocolURL slices the raw query instead of round-tripping URLComponents, preserving percent-encoded relay and secret bytes, and isBitkitSetupHandoff pins the wrapper to bitkit://pubky-auth/setup. The scheme switch is safe: Pubky Ring declares both inbound. 2 notes below. Retest suggested: items 1, 2 (they enter through an opened Pubky request and continue into the Ring app).
QA Notes
- pending: re-driven on this head in the Test phase — the delta moves external link delivery onto the scene delegate and switches the outbound handoff to
pubkyring. - pending: re-driven on this head in the Test phase — the delta moves external link delivery onto the scene delegate and switches the outbound handoff to
pubkyring. - ⏭️ skipped: already tested on e509cb3, report
- ⏭️ skipped: already tested on 51100a3, report
- ⏭️ skipped: already tested on 51100a3, report
- ⏭️ skipped: already tested on e509cb3, report
The approval follows the device run, which starts once jvsena42, piotr-iohk approve or on a @ovi-reviewer[bot] test comment.
Replies
ben-kaufman: @jvsena42 On sheet replacement, both the authorization button and PIN callback start unstructured
Taskinstances… (comment)
My answer from the 96fd505 review still stands, and this delta does not disturb it: the scoped diff 96fd505..902ff39 touches neither the authorization button, the PIN callback, nor sheet teardown. performAuthorization still re-checks the sheet item's rawUrl before dismissing, and approveSignupWithTimeout still bounds the leg at 30 seconds, so an uncancelled Task belonging to a replaced sheet runs to completion and has its result discarded rather than applied. Nothing to re-review here.
ben-kaufman: Merged
masterand resolved the conflict, preserving the signup and payment changes. (comment)
Confirmed, and still true at this head: 96fd505 is the merge commit itself, and everything added since then is the Pubky deep-link work on top of it rather than any further conflict resolution. The signup and payment changes you preserved are both still present at 902ff39 — AppViewModel.swift keeps hasSendPaymentTarget alongside resetSendState(preservingContactPaymentContext:), and the wrapped-request guard on the decoded .pubkyAuth branch is intact.
Notes
- Could we add the Figma frame link for the changed screens (
MainNavView,PubkyAuthApprovalSheet)? It lets the review compare the implementation against the design. - The description is missing the template's
Designheading, and this delta changes user-visible UI (the relay-origin row and the extended watch-only consent copy). Could we add a### Designsection carrying either the Figma frame link orN/A — no design available.?
Coverage
Total: 55% (delta diff since 96fd505, 19 files)
- Journeys: 75% - The new
journeys/pubky-auth/open-watch-only-link.xmldrives the cold-start setup link into watch-only consent, but not the warm-start or node-retention replay. - Unit tests: 90% - Every changed production path has a test that would fail if it broke; the
AppDelegateURL override and thecanHandleDeepLinks == falsegate are uncovered. - QA: 0% - Manual Tests not run; 4 of 6 carry, items 1 and 2 re-drive in the Test phase
Reviewed by Claude Code (claude-opus-5-high) via gh-pr-review-loop skill
|
Added the |
There was a problem hiding this comment.
✅ Approved with some observations
- ✅ On Loopky -> Click on sign in with ring -> selected Bitkit (with a Pubky identity) -> Authorize
⚠️ Couldn't test signing with ring because of pubky/pubky-ring#375
4 participants
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 #697 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.
Linked Issues/Tasks
Depends on #697.
Design
N/A — no design available.
Screenshot / Video
Not included; this reuses the existing scanner, authorization approval sheet, loading treatment, and profile setup UI.
QA Notes
Validation:
PubkyProfileManagerTests.swiftcovers stored-key sign-in, credential-read/sign-in/profile failures and retries, cancellation-error propagation, and unchanged no-key signup/cleanup. Existing consent, activation and scanner regressions also pass.