Add Convos Vault technical implementation plan#558
Add Convos Vault technical implementation plan#558yewreeka wants to merge 16 commits intographite-base/558from
Conversation
- Add optional conversationType field (6) to ConversationCustomMetadata protobuf for identifying Vault conversations - DeviceKeyBundleCodec: full key export sent when new device joins Vault - DeviceKeyShareCodec: single key share sent on conversation creation - DeviceRemovedCodec: notification when a device is removed from Vault - All three codecs use JSON encoding with ISO8601 dates - shouldPush returns false (Vault messages are device-sync only) - 25 tests covering encode/decode, edge cases, forward compatibility
Use protoc-gen-swift's Visibility=Public flag instead of manually editing the generated file.
New ConvosVault Swift package for multi-device key sync: - Depends on ConvosAppData and XMTPiOS (same pattern as ConvosProfiles) - Moved DeviceKeyBundleCodec, DeviceKeyShareCodec, DeviceRemovedCodec from ConvosCore/Custom Content Types/ to ConvosVault/ContentTypes/ - Moved VaultContentTypeTests to ConvosVault/Tests/ - ConvosCore now depends on ConvosVault package - InboxStateMachine imports ConvosVault for codec registration
New KeychainKeyStoreProtocol and implementation for storing KeychainIdentityKeys by arbitrary identifier with configurable accessibility: - afterFirstUnlockThisDeviceOnly: local keychain only (default) - afterFirstUnlock: syncs via iCloud Keychain This enables the Vault to store its key in both local and iCloud keychain, while keeping KeychainIdentityStore unchanged for conversation identity management. Includes MockKeychainKeyStore for testing and 11 unit tests.
Simplified alternative to InboxStateMachine with 4 states (disconnected, connecting, connected, error) instead of 10. - connect: builds or creates XMTP client, finds or creates Vault group (identified by conversationType='vault' in appData) - disconnect: cancels stream, clears state - send: sends typed content with codec to Vault group - members/addMember/removeMember: group membership management - Message streaming dispatches to VaultClientDelegate for DeviceKeyBundle, DeviceKeyShare, and DeviceRemoved content types - Uses OSAllocatedUnfairLock for thread-safe state access
Sits on top of VaultClient and VaultIdentityStoreProtocol: - shareKey: sends a single key share to all Vault devices - shareAllKeys: bundles all local identities into a DeviceKeyBundle - listDevices: enumerates Vault group members - removeDevice: sends DeviceRemoved message then removes from group - Incoming key shares/bundles are auto-imported to identity store with duplicate detection (existing keys are not overwritten) Supporting types: - VaultIdentityKeys: raw key pair for vault identity generation - VaultIdentityEntry: conversation-level identity (id, inbox, keys) - VaultDevice: device info model for UI display - VaultIdentityStoreProtocol: abstraction for key persistence - VaultManagerDelegate: callbacks for import/removal/errors 8 unit tests covering import, dedup, error cases, and models.
Wraps two KeychainKeyStore instances (local + iCloud) to provide automatic redundancy for the Vault key: - save: writes to local (ThisDeviceOnly) and iCloud (AfterFirstUnlock) - load: reads from local first, falls back to iCloud (and caches locally) - exists: checks both stores - delete: removes from both This enables Vault key recovery when restoring to a new device with the same Apple ID, while keeping a local copy for performance and offline access. 9 unit tests covering dual-store save, load priority, iCloud fallback with local caching, missing key errors, and deletion.
Phase 3 + 4 foundations: PairingCoordinator: - 6-digit confirmation code generation and validation - Pairing state machine (idle, waitingForScan, waitingForConfirmation, sharingKeys, completed, failed, expired) - PairingError types for all failure modes - Input cleaning (strips spaces/dashes from confirmation codes) VaultKeyShareNotifier: - Protocol for auto-sharing keys on conversation creation - ConversationKeyInfo carries identity data for sharing - VaultManager conforms and auto-sends DeviceKeyShare when connected and a new conversation key is created 14 pairing tests + 47 total ConvosVault tests passing.
Provides the interface SessionManager will use to start/stop the Vault and notify it of new conversation key creation: - startVault: connects with signing key and XMTP options - stopVault: disconnects cleanly - notifyConversationCreated: triggers auto key share VaultManager conforms, bridging to connect/disconnect/conversationKeyCreated.
KeychainKeyStore duplicated keychain CRUD from KeychainIdentityStore. Instead, add configurable service + accessibility params to KeychainIdentityStore's init (defaulting to existing behavior). VaultKeyStore now uses two KeychainIdentityStore instances: - Local: service='vault', accessibility=ThisDeviceOnly - iCloud: service='vault-icloud', accessibility=AfterFirstUnlock Removes 249 lines of duplication. 340 ConvosCore tests passing.
The Vault identity is a proper KeychainIdentity with its own inboxId and clientId (assigned when the XMTP client is created), enabling push notification registration and notification extension processing — same as any conversation identity. Changes: - save/load/delete all take inboxId/clientId params - loadFromAnyStore: finds the Vault identity from either store without needing the inboxId upfront (for first-time recovery) - deleteAll: clears both stores entirely This also prepares for device backups — when enabled, all conversation keys from the main KeychainIdentityStore will be mirrored to an iCloud-accessible store using the same pattern. 13 unit tests covering all paths.
Each device gets its own Vault key via explicit pairing (QR code). No iCloud Keychain sync — avoids the complexity of one inboxId with multiple installations vs multiple members. One device = one member = one inboxId. Simple. Changes: - VaultKeyStore: single store (no local+iCloud dual store) - KeychainIdentityStore: revert accessibility param, keep service param - VaultManager: track member count, only share keys when > 1 member - conversationKeyCreated skips if solo device (nobody to share with) - refreshMemberCount on connect and after removeDevice 55 tests passing (47 ConvosVault + 8 VaultKeyStore).
VaultClient now handles background/foreground transitions: - pause: cancels message stream, drops local DB connection - resume: reconnects DB, syncs conversations + group, restarts stream - Lifecycle observation via NotificationCenter (vaultDidEnterBackground, vaultWillEnterForeground) — started on connect, stopped on disconnect The app posts these notifications from its scene delegate, and VaultClient responds automatically. On resume, member count is also refreshed in case devices were added/removed while backgrounded. VaultSessionIntegration exposes pauseVault/resumeVault for SessionManager to call directly if needed.
|
Claude finished @yewreeka's task —— View job Code Review CompleteI've reviewed this comprehensive Convos Vault implementation. This is well-architected foundational work for multi-device key synchronization. Here's my detailed feedback: 🎯 Overall AssessmentStrong implementation with solid architecture, good test coverage, and proper security practices. The phased approach (content types → client → manager → integration) is logical and reviewable. Key strengths:
Areas for improvement below ⬇️ 🔒 Security Concerns1. Private key logging exposure (High Priority)
2. Missing input validation (Medium Priority)
public static func generateConfirmationCode() -> String {
let code = (0 ..< 6).map { _ in String(Int.random(in: 0 ... 9)) }.joined()
return code
}
3. Keychain accessibility concern
🐛 Potential Bugs4. Race condition in member count (Medium Priority)
private let memberCountLock: OSAllocatedUnfairLock<Int> = .init(initialState: 1)
private func refreshMemberCount() async {
let count = (try? await vaultClient.members().count) ?? 1
memberCountLock.withLock { $0 = count }
}
5. Deduplication logic issue
let alreadyExists = await identityStore.hasIdentity(for: share.inboxId)
guard !alreadyExists else { return }
6. Unhandled error in stream
} catch {
if !Task.isCancelled {
self.delegate?.vaultClient(self, didEncounterError: error)
self.updateState(.error(error))
}
}
⚡ Performance Considerations7. Potential memory leak in stream
streamTask = Task { [weak self] in
guard let self else { return }
do {
for try await message in group.streamMessages() {
guard !Task.isCancelled else { break }
guard message.senderInboxId != clientInboxId else { continue }
self.handleMessage(message)
}
} catch { ... }
}
8. Unnecessary async/await in hot path
let alreadyExists = await identityStore.hasIdentity(for: share.inboxId)
guard !alreadyExists else { return }
do {
try await identityStore.save(entry: entry)
delegate?.vaultManager(self, didImportKey: entry)
} catch { ... }
📚 Code Quality & Best Practices9. Inconsistent error handling patterns
10. Missing protocol documentation
public protocol VaultSessionIntegration: Sendable {
func startVault(signingKey: SigningKey, options: ClientOptions) async throws
func stopVault()
func pauseVault()
func resumeVault() async
func notifyConversationCreated(_ keyInfo: ConversationKeyInfo)
}
11. Weak confirmation code validation
public func validateConfirmationCode(_ input: String, expected: String) -> Bool {
let cleaned = input.filter(\.isNumber)
return cleaned == expected && cleaned.count == 6
}
12. Protobuf field naming inconsistency
optional string conversationType = 6; // e.g. "vault" for multi-device key sync
✅ Test Coverage AssessmentGood coverage overall (55 tests), but gaps exist: 13. Missing test scenarios:
14. Mock limitations:
📏 Adherence to Project Conventions15. Follows most conventions well:✅ Uses actors and protocols for dependency injection 16. Minor style issues:VaultClient.swift:4 uses VaultManager.swift:69 mixes public final class VaultManager: @unchecked Sendable {
private let vaultClient: VaultClient
private let memberCountLock: OSAllocatedUnfairLock<Int> = .init(initialState: 1)
🔄 Architecture & Design17. Strong separation of concerns:
18. Protocol design consideration:
19. Lifecycle handling is solid:
private func startLifecycleObservation() {
lifecycleTask = Task { [weak self] in
await withTaskGroup(of: Void.self) { group in
group.addTask { ... }
group.addTask { ... }
await group.waitForAll()
}
}
}✅ Good use of 📝 Documentation & Plan Consistency20. Implementation matches plan:The code follows Removed iCloud Keychain sync (per PR description) but plan still references it (lines 102-105, 133-137).
21. Missing integration points:Plan mentions (line 158) hooking into
🎨 Suggestions for Polish22. Add convenience initializers:
public static func current(inboxId: String, name: String) -> VaultDevice {
VaultDevice(inboxId: inboxId, name: name, isCurrentDevice: true)
}23. Consider Result type for delegate methods:Instead of separate func vaultManager(_ manager: VaultManager, didCompleteImport result: Result<VaultIdentityEntry, Error>)24. Add metrics:Consider tracking:
🚀 Final RecommendationsBefore merging:
Future PRs: SummaryThis is high-quality foundation work for the Vault feature. The architecture is sound, tests provide good coverage, and security practices are generally solid. The issues identified are mostly refinements rather than fundamental problems. Recommended action: Approve with minor revisions (fix issues #2, #5, #6, #20). |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| updateState(.connecting) | ||
|
|
||
| let client: Client | ||
| do { |
There was a problem hiding this comment.
🟢 Low ConvosVault/VaultClient.swift:63
The catch block on line 69 catches all errors from Client.build and unconditionally falls back to Client.create. If build fails due to database corruption, invalid options, or other non-recoverable errors, the code creates a fresh client anyway — masking the underlying failure and potentially leaving the system in an unexpected state with orphaned or duplicate client data. Consider checking the error type and only falling back to create when the error indicates the client does not exist.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file ConvosVault/Sources/ConvosVault/VaultClient.swift around line 63:
The `catch` block on line 69 catches all errors from `Client.build` and unconditionally falls back to `Client.create`. If `build` fails due to database corruption, invalid options, or other non-recoverable errors, the code creates a fresh client anyway — masking the underlying failure and potentially leaving the system in an unexpected state with orphaned or duplicate client data. Consider checking the error type and only falling back to `create` when the error indicates the client does not exist.
Evidence trail:
ConvosVault/Sources/ConvosVault/VaultClient.swift lines 55-80 at REVIEWED_COMMIT - The `connect` function has a do-catch block where `Client.build` is called in the try block (lines 63-67), and the catch block (lines 68-72) catches all errors without any error type checking and unconditionally calls `Client.create` as fallback.
yewreeka
force-pushed
the
jarod/join-request-content-type
branch
from
16b87f5 to
3acd5b7
Compare
yewreeka
changed the base branch from
jarod/join-request-content-type
to
graphite-base/558
1 participant
Add Convos Vault technical implementation plan
Phase 1: Add conversationType protobuf field and Vault content types
protobuf for identifying Vault conversations
Regenerate protobuf with --swift_opt=Visibility=Public
Use protoc-gen-swift's Visibility=Public flag instead of manually
editing the generated file.
Create ConvosVault package, move Vault content types from ConvosCore
New ConvosVault Swift package for multi-device key sync:
from ConvosCore/Custom Content Types/ to ConvosVault/ContentTypes/
Add KeychainKeyStore for generic key storage with iCloud support
New KeychainKeyStoreProtocol and implementation for storing
KeychainIdentityKeys by arbitrary identifier with configurable
accessibility:
This enables the Vault to store its key in both local and iCloud
keychain, while keeping KeychainIdentityStore unchanged for
conversation identity management.
Includes MockKeychainKeyStore for testing and 11 unit tests.
Add VaultClient — lightweight XMTP client wrapper for Vault
Simplified alternative to InboxStateMachine with 4 states
(disconnected, connecting, connected, error) instead of 10.
group (identified by conversationType='vault' in appData)
DeviceKeyBundle, DeviceKeyShare, and DeviceRemoved content types
Add VaultManager — key sync orchestration layer
Sits on top of VaultClient and VaultIdentityStoreProtocol:
with duplicate detection (existing keys are not overwritten)
Supporting types:
8 unit tests covering import, dedup, error cases, and models.
Add VaultKeyStore — dual-keychain storage for Vault identity
Wraps two KeychainKeyStore instances (local + iCloud) to provide
automatic redundancy for the Vault key:
This enables Vault key recovery when restoring to a new device
with the same Apple ID, while keeping a local copy for performance
and offline access.
9 unit tests covering dual-store save, load priority, iCloud
fallback with local caching, missing key errors, and deletion.
Add PairingCoordinator and VaultKeyShareNotifier
Phase 3 + 4 foundations:
PairingCoordinator:
sharingKeys, completed, failed, expired)
VaultKeyShareNotifier:
connected and a new conversation key is created
14 pairing tests + 47 total ConvosVault tests passing.
Add VaultSessionIntegration protocol for SessionManager hookup
Provides the interface SessionManager will use to start/stop the
Vault and notify it of new conversation key creation:
VaultManager conforms, bridging to connect/disconnect/conversationKeyCreated.
Remove KeychainKeyStore, reuse KeychainIdentityStore instead
KeychainKeyStore duplicated keychain CRUD from KeychainIdentityStore.
Instead, add configurable service + accessibility params to
KeychainIdentityStore's init (defaulting to existing behavior).
VaultKeyStore now uses two KeychainIdentityStore instances:
Removes 249 lines of duplication. 340 ConvosCore tests passing.
VaultKeyStore uses real inboxId/clientId instead of sentinels
The Vault identity is a proper KeychainIdentity with its own
inboxId and clientId (assigned when the XMTP client is created),
enabling push notification registration and notification extension
processing — same as any conversation identity.
Changes:
without needing the inboxId upfront (for first-time recovery)
This also prepares for device backups — when enabled, all
conversation keys from the main KeychainIdentityStore will be
mirrored to an iCloud-accessible store using the same pattern.
13 unit tests covering all paths.
Expose vaultInstallationId on VaultManager for push registration
Remove vaultInstallationId — push registration is device-level, not inbox-level
Simplify to local-only Vault key (no iCloud sync)
Each device gets its own Vault key via explicit pairing (QR code).
No iCloud Keychain sync — avoids the complexity of one inboxId
with multiple installations vs multiple members.
One device = one member = one inboxId. Simple.
Changes:
55 tests passing (47 ConvosVault + 8 VaultKeyStore).
Add app lifecycle handling to VaultClient
VaultClient now handles background/foreground transitions:
vaultWillEnterForeground) — started on connect, stopped on disconnect
The app posts these notifications from its scene delegate, and
VaultClient responds automatically. On resume, member count is
also refreshed in case devices were added/removed while backgrounded.
VaultSessionIntegration exposes pauseVault/resumeVault for
SessionManager to call directly if needed.
Note
Add ConvosVault package for multi-device key sync via vault group conversations
ConvosVaultSwift package with content codecs (DeviceKeyBundleCodec,DeviceKeyShareCodec,DeviceRemovedCodec) for serializing key material and device events over XMTP messages.VaultClientto manage lifecycle of a dedicated 'vault' XMTP group conversation, streaming and decoding incoming key messages, andVaultManagerto orchestrate key sharing, import, and device membership.ConversationCustomMetadataproto with an optionalconversationTypefield (tag 6) to identify vault group conversations.KeychainIdentityStoreservice name overridable and addsVaultKeyStoreas an async keychain façade for vault identities.PairingCoordinatorfor 6-digit confirmation code generation and validation, and a convos-vault-implementation.md technical plan document.📊 Macroscope summarized 1cbf8de. 11 files reviewed, 13 issues evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues