Conversation
# Conflicts: # Bitkit/ViewModels/AppViewModel.swift
|
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 24 files.
Counterpart synonymdev/bitkit-android#1329: equivalent.
Findings:
2 inline (non-blocking)
Audit:
Audited - no findings.
Coverage:
QA: journeys and manual tests await all reviewers to approve, author can run it now via comment: @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
| } | ||
| ) async throws { | ||
| if isProfileSetupPending, let publicKey { | ||
| if let publicKey, isProfileSetupPending || AdoptedPubkyReference.current != nil { |
There was a problem hiding this comment.
Identities created through a signup link take this pending-profile branch and return before publishOwnSharedRecord(). Completing the profile therefore leaves it unavailable in Ring until Bitkit initializes again. Could we publish the owned key after successful signup activation or when this branch completes?
There was a problem hiding this comment.
Fixed in 337ea2f: approveSignupAuth calls publishOwnSharedRecord() once signup activation succeeds, so Ring sees the key right away, even before the profile is finished. Android needed no change because its provider serves the stored key live.
jvsena42
left a comment
There was a problem hiding this comment.
Three LOWs inline, all gated behind the Paykit opt-in, all checked by a second pass.
Checked and clean:
- The access group is team-prefixed with no wildcard, and the record is
WhenUnlockedThisDeviceOnlyand non-synchronizable, so nothing reaches iCloud or a device-to-device restore. One thing worth a thought: the shared record isWhenUnlockedwhile the private store isAfterFirstUnlock, so an adopted identity's secret is unreadable from a locked background context — I did not trace whether any background path needs it. - What is written is the seed-derived pubky secret as hex, never the mnemonic, and it is the point of the feature.
- Identity binding.
loadSecretrequires 64 lowercase hex that derives back to the requested pubky, and adoption re-derives the public key from the secret it actually read, so a stale or hostile record cannot supply a key for another identity. - Wipe.
getAllKeyChainStorageKeysnow filters on Bitkit's own group, so Ring's items are never enumerated or deleted, and both wipe entry points remove Bitkit's shared records first. - Scheme surface. The
bitkit://pubky-authcallback parsing is gone with the relay flow. - Migration from the v2.5.0 relay flow: those users keep their session until expiry, legacy
externalSessionbackups decode to no identity, and the same pubky can be re-adopted from an updated Ring.
Parity with synonymdev/bitkit-android#1329: android's boundary is an exported provider behind a signature permission plus a per-call UID signature check, and it copies nothing at rest; here the secret sits in the shared group by design. Both reduce to "same signing identity".
| try sessionProvider.persistReceiverNoiseSecretKey(access.exportReceiverNoiseSecretKey()) | ||
|
|
||
| guard shouldStoreLocalSecret, let localSecret = access.exportLocalSecretKey() else { | ||
| guard AdoptedPubkyReference.current == nil, let localSecret = access.exportLocalSecretKey() else { |
There was a problem hiding this comment.
A signup link approved while an adopted Ring reference is still set leaves the identity self-inconsistent. LOW as shipped, because it is behind the Paykit opt-in.
approveSignupAuth gates only on publicKey == nil and hasStoredIdentity(), neither of which looks at AdoptedPubkyReference. Reaching that state is possible: initializePersistedSession now feeds the adopted Ring secret into the re-sign-in, and when both importSession and signIn(R) fail the saved session is deleted and publicKey goes nil, while the reference survives — checkAdoptedSource clears it only on errSecItemNotFound, and the Ring record is still there.
A signup link then registers the wallet identity W, and this guard refuses to persist W's secret because the reference exists. The signup itself is signed with W correctly, but afterwards activeSecretKeyHex() returns R: ordinary approvals in PubkyAuthApprovalSheet display W and sign with R, the SDK is handed R as the local secret for W's session, and snapshotSessionBackupState returns nil so W is never backed up. Worse, the next launch can flip identity — if importSession fails, signIn(R) restores R and W is orphaned on the homeserver.
Fix, one line: clear AdoptedPubkyReference.current just before registerIdentity (a signup link means "use my wallet identity", and this guard then persists W), or throw already-signed-in while a reference exists so the user re-picks the Ring key on Pubky Choice.
Parity note: synonymdev/bitkit-android#1329 avoids the precondition structurally — its resolveSignedInSession never re-signs in with the Ring credential and does not delete the session on failure, so hasIdentity() stays true and a signup link throws. Its SDK provider has the same Ring fallback, so the divergence is in these two iOS behaviours, not in the fallback itself.
There was a problem hiding this comment.
Fixed in 6451020: hasStoredIdentity() in PubkyProfileManager.swift now counts an AdoptedPubkyReference, because the next launch signs in with that Ring key again. After a failed restore a signup link now gets "Already signed in", the same as Android. I went with your second option: clearing the reference at signup would swap the picked Ring identity for the wallet key after one offline launch.
| } | ||
|
|
||
| private static func clearLocalAppState() async { | ||
| SharedPubkyKeychain.removeAllOwn() |
There was a problem hiding this comment.
A failed forgetSessionAccess() leaves the homeserver session behind, so a deleted Ring identity comes back with no key. LOW.
The catch only logs, and clearLocalAppState never deletes .paykitSession. The realistic throw is SDK construction failing before the provider's clear callback runs. On the next launch the saved session is still there, activeSecretKeyHex() is nil now that the reference is gone, and importSession with no local secret restores R as authenticated — an identity the user was just told was disconnected, which can still do session-only writes but cannot sign. If the import fails instead, it is the "session expired" toast on every launch until sign-out.
Fix: delete .paykitSession (and .pubkySecretKey) on this path regardless of the SDK call's outcome, as restoreSessionBackupState already does.
There was a problem hiding this comment.
Fixed in ed779b1: when forgetSessionAccess() fails, clearLocalState() in PubkyProfileManager.swift now deletes .paykitSession and .pubkySecretKey through PubkySessionAccessTeardown.clear, the same as Android's clearLocalState.
| /// Under test, records live under their own service so a suite never touches the records Pubky Ring reads. | ||
| static let service = Env.isUnitTest ? "unit-tests.pubky.shared.v1" : "pubky.shared.v1" | ||
| static let ringSourceApp = "app.pubkyring" | ||
| static let ownSourceApp = "to.bitkit" |
There was a problem hiding this comment.
Nit: ownSourceApp has no network component while Env.keychainGroup does, so all flavors share one to.bitkit: namespace in the shared group. The bundle id is to.bitkit in every configuration, so this needs a Debug or E2E build installed over the mainnet one rather than a co-install: that build's removeAllOwn() then deletes the mainnet record, and it publishes a regtest key that Ring shows as a Bitkit key. The mainnet record heals on its next launch. Worth noting that the prefix is a cross-app contract with Ring and the android twin, so it cannot be suffixed unilaterally.
There was a problem hiding this comment.
Fixed in 7208b47: ownSourceApp in SharedPubkyKeychain.swift now follows the network with the Android app IDs: to.bitkit on mainnet, to.bitkit.tnet on testnet and signet, to.bitkit.dev on regtest. Pubky Ring already lists all three, so it needs no change, and mainnet keeps to.bitkit.
|
On the |
jvsena42
left a comment
There was a problem hiding this comment.
Delta since b9a8d8045 (64510209, ed779b11, 7208b470, 55abbe3a): no findings. All three are addressed.
- Signup while a Ring key is adopted.
hasStoredIdentity()now counts an adopted reference as an identity, so the signup link is refused as already signed in and the user re-picks the key on Pubky Choice. That closes the path where W was registered whileactiveSecretKeyHex()still returned R. - Source loss. The catch now clears the session access keychain entries directly when
forgetSessionAccess()throws, so the deleted Ring identity is not resurrected on the next launch. - Shared record name.
ownSourceAppis per network and matches android's applicationIds, so a test build installed over the mainnet one no longer removes or shadows its record. - The new
activeSecretKeyHex() != nilguard gives an unreadable key its own toast instead of "no identity", matching synonymdev/bitkit-android#1329.
|
@Jasonvdb conflict |
This PR:
pubky.sharedkeychain groupDescription
pubky.sharedkeychain group so Pubky Ring can use the pubky Bitkit created, including one created from a signup link, which is shared as soon as signup completes.to.bitkit,to.bitkit.tnet,to.bitkit.dev), so a test build can't touch the mainnet entry.Release gate
pubky.sharedkeychain group must be enabled on theto.bitkitApp ID before device builds work.Out of Scope
PubkyProfileManager: the BIP39 passphrase difference with Android is pre-existing and unchanged.Reviewer Notes
externalSession, written since v2.3.0 behind the same toggle) still decode, but restore without a Pubky identity. They only held a homeserver session, never the key: the key stays in Pubky Ring and can be picked again on Pubky Choice with a Pubky Ring build that includes feat: share pubky keys with Bitkit pubky/pubky-ring#377. Covered bytestRestoreSessionBackupStateForLegacyExternalSessionClearsCredentials.WhenUnlockedThisDeviceOnly, like Pubky Ring's, while Bitkit's own keychain usesAfterFirstUnlock. Nothing reads a picked pubky's key in the background, because Bitkit has no background modes, and a failed read keeps the saved session.Design
Pubky Choice follows the "Profile use existing" and "Profile Create" mockups; the matching Figma frames aren't confirmed, so no link is added.
Preview
QA Notes
Journeys
N/A — no journeys yet; see Manual Tests. Most flows need Pubky Ring running alongside Bitkit, which the Capabilities table doesn't list; the few that don't are marked "no journey yet".
Manual Tests
pubky_secret_keyis written to the keychain — Pubky Ring alongside Bitkit not in Capabilitiespubkyauth://request into the scanner → the session is restored and the request is authorized with the Ring key read just in time — Pubky Ring alongside Bitkit not in Capabilitiespubkyauth://direct_signuplink → "Already signed in" shows and no new identity is created — Pubky Ring alongside Bitkit not in Capabilitiespubkyauth://direct_signuplink → Authorize → Pubky Ring shows the Bitkit pubky as a dashed card without relaunching Bitkit — Pubky Ring alongside Bitkit not in Capabilitiespubkyauth://request with the adopted Bitkit pubky → the grant succeeds, so the foreign key is read just in time — Pubky Ring alongside Bitkit not in Capabilitiesregression:Pubky Ring: deep-link authorize and encrypted backup export for an owned pubky → both still work after the relay removal — Pubky Ring alongside Bitkit not in Capabilitiesregression:Bitkit: scanner → paste apubkyauth://request → the approval sheet opens and completes, and Bitkit no longer registers thepubkyauthscheme — no journey yetAutomated Checks
SharedPubkyKeychainTests.swift— a shared record's secret must be 64 lowercase hex characters that derive the requested pubkyPubkyChoiceViewTests.swift— Pubky Choice picks its description, and shows the Create card only, when Pubky Ring lists no pubkysPubkyProfileManagerTests.swift— the signing key comes from Bitkit's own secret or, just in time, the picked pubky's; a picked pubky counts as a stored identity and has no backup snapshot; legacy relay backups restore without an identity; restore tests inject the shared-record cleanup, and unit tests use their ownunit-tests.pubky.shared.v1keychain service, so they never touch Pubky Ring's recordsShopPaymentRequestTests.swift— the signup scanner test clears any picked pubky, so it doesn't depend on simulator statePubkyProfileManagerTests.swift— relay auth URL, callback, nonce and completion cases, and the external-session backup cases, because the relay flow and that session kind no longer existPaykitSdkClientConfigTests.swift— relay auth activation and cleanup cases, because that flow no longer existsPubkyAuthURLSchemeTests.swift— the Pubky Ring outbound scheme query, because Bitkit no longer opens Pubky Ringswiftformat --linton the changed files — no formatting changes