feat: enable DashPay iOS flow + key health tooling#3765
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds wallet-scoped key storage and cleanup, a WalletKeyHealth diagnostic sheet (check/rederive/delete), purpose-driven key constraints and unified contract-bounds picker, optional DashPay key registration during identity creation, phased SwiftData wallet deletion, Rust FFI + model contract-bounds propagation, DPNS availability correctness, SDK platform-version resolution, and UI guards for invalidated SwiftData models. ChangesKey Health Diagnostics and DashPay Integration
SwiftData Resilience, FFI, and SDK Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift (2)
222-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEstablished contact metadata (alias, note, isHidden) is not being read from the SDK.
The code maps
establishedIdsdirectly toDashPayContactstructs using only the DashPay profile's display name, but never callsmanaged.getEstablishedContact(contactId:)to fetch theEstablishedContactobject that holds per-contact metadata.Impact:
- The
.filter { !$0.isHidden }at Line 94 is broken—all contacts default toisHidden = false.- The note display (Lines 372-377) never renders—
noteis alwaysnil.- The contact
alias(distinct from the profile'sdisplayName) is ignored.🔧 Proposed fix to fetch established contact metadata
contacts = establishedIds.map { contactId in + // Fetch the EstablishedContact to get alias, note, isHidden + let established = try? managed.getEstablishedContact(contactId: contactId) + let alias = try? established?.getAlias() + let note = try? established?.getNote() + let isHidden = (try? established?.isHidden()) ?? false + let profile = (try? wallet.getDashPayProfile(identityId: contactId)) ?? nil let trimmedName = profile?.displayName? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let displayName = trimmedName.isEmpty - ? (String(contactId.toHexString().prefix(12)) + "…") - : trimmedName + // Prefer alias, then profile displayName, then hex fallback + let displayName: String + if let alias = alias, !alias.isEmpty { + displayName = alias + } else if !trimmedName.isEmpty { + displayName = trimmedName + } else { + displayName = String(contactId.toHexString().prefix(12)) + "…" + } + return DashPayContact( id: contactId, displayName: displayName, - identityId: contactId + identityId: contactId, + note: note, + isHidden: isHidden ) }Based on learnings: Context snippet 1 shows that
EstablishedContactprovidesgetAlias(),getNote(), andisHidden()methods that return the contact-specific metadata stored by the SDK.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` around lines 222 - 234, When building contacts from establishedIds inside the mapping (currently using wallet.getDashPayProfile), also call managed.getEstablishedContact(contactId:) for each contactId, read EstablishedContact.getAlias(), getNote(), and isHidden() and use those values to populate the DashPayContact alias, note, and isHidden fields (falling back to profile.displayName or the hex short id when alias is empty); ensure you still set identityId and id as before and handle any nil/throwing calls safely (try? for both wallet.getDashPayProfile and managed.getEstablishedContact) so filters and note/alias rendering work correctly.
202-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContact request creation timestamps are not being read from the SDK.
The code maps
incomingIdsandsentIdsdirectly toDashPayContactRequeststructs using only the identity IDs, but never callsmanaged.getIncomingContactRequest(senderId:)ormanaged.getSentContactRequest(recipientId:)to fetch the actualContactRequestobjects that hold the realcreatedAttimestamp.Impact:
- Line 409 displays
request.createdAt, style: .relative, butcreatedAtdefaults toDate()(current time) at Line 330.- All requests incorrectly show "just now" instead of the actual creation time.
- Request age information is lost.
🔧 Proposed fix to fetch request timestamps
incomingRequests = incomingIds.map { senderId in + let request = try? managed.getIncomingContactRequest(senderId: senderId) + let timestamp = (try? request?.getCreatedAt()) ?? 0 + let createdAt = timestamp > 0 ? Date(timeIntervalSince1970: TimeInterval(timestamp) / 1000) : Date() + DashPayContactRequest( id: "incoming-\(senderId.toHexString())", senderId: senderId, - recipientId: identity.identityId + recipientId: identity.identityId, + createdAt: createdAt ) } sentRequests = sentIds.map { recipientId in + let request = try? managed.getSentContactRequest(recipientId: recipientId) + let timestamp = (try? request?.getCreatedAt()) ?? 0 + let createdAt = timestamp > 0 ? Date(timeIntervalSince1970: TimeInterval(timestamp) / 1000) : Date() + DashPayContactRequest( id: "sent-\(recipientId.toHexString())", senderId: identity.identityId, - recipientId: recipientId + recipientId: recipientId, + createdAt: createdAt ) }Based on learnings: Context snippet 2 shows that
ContactRequest.getCreatedAt()returns aUInt64timestamp (milliseconds since epoch) that must be converted to aDatefor display.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` around lines 202 - 215, The mapping that builds DashPayContactRequest in FriendsView currently uses only IDs and thus loses real timestamps; update the incomingIds and sentIds transforms to fetch the actual ContactRequest objects via managed.getIncomingContactRequest(senderId:) and managed.getSentContactRequest(recipientId:), read the ContactRequest.getCreatedAt() (a UInt64 milliseconds-since-epoch), convert it to a Date (Date(timeIntervalSince1970: Double(ms) / 1000.0)), and assign that Date to DashPayContactRequest.createdAt so the UI shows the real request age; keep existing id/senderId/recipientId assignments and handle optional/missing ContactRequest by falling back to Date() or nil as appropriate.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift (1)
290-339: Note: Several struct fields are defined but never populated.The
DashPayContactandDashPayContactRequeststructs define fields that are not currently set during initialization inloadFriends():
DashPayContact.note— alwaysnil(never fetched fromEstablishedContact.getNote())DashPayContact.dpnsName— alwaysnil(DPNS lookup not implemented)DashPayContact.isHidden— alwaysfalse(never fetched fromEstablishedContact.isHidden())DashPayContactRequest.createdAt— always defaults toDate()(never fetched fromContactRequest.getCreatedAt())DashPayContactRequest.senderDisplayName— alwaysnil(sender profile lookup not implemented)These fields render correctly when populated (Lines 362-377, Line 409), but the initialization logic doesn't call the SDK accessors to retrieve the values. This is addressed by the fixes proposed in the earlier comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` around lines 290 - 339, The issue is that several fields on DashPayContact and DashPayContactRequest are never populated; update the loadFriends() logic where EstablishedContact and ContactRequest objects are mapped into DashPayContact and DashPayContactRequest to call the SDK accessors and fill these fields: for DashPayContact call EstablishedContact.getNote() to set note, EstablishedContact.isHidden() to set isHidden, and perform the DPNS name lookup (same logic used elsewhere) to set dpnsName before constructing DashPayContact; for DashPayContactRequest call ContactRequest.getCreatedAt() to set createdAt and fetch the sender’s profile/display name to set senderDisplayName before constructing DashPayContactRequest. Ensure you use the DashPayContact and DashPayContactRequest initializers (id, displayName, identityId, dpnsName, note, isHidden) and (id, senderId, recipientId, createdAt, senderDisplayName) respectively when replacing the current mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- Around line 618-643: The delete path still only removes the legacy account
name but writes now use account =
"identity_privkey.\(metadata.walletId).\(derivationPath)"; update the single-key
delete API (the function that deletes by Keychain identifier / the delete method
that currently expects the legacy name) to attempt deletion using both the new
namespaced account string ("identity_privkey.<walletId>.<derivationPath>") and
the legacy form ("identity_privkey.<derivationPath>"), or fall back to the
metadata-scan used by retrieveIdentityPrivateKey(publicKeyHex:) /
retrieveKeyData(identifier:), and ensure
PersistentPublicKey.privateKeyKeychainIdentifier values remain consistent after
upserts.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swift`:
- Around line 140-160: pickerEntries currently builds system entries by
filtering out any IDs present in contractsForNetwork, causing DashPay's system
metadata to be dropped when a saved DashPay exists; update the logic in the
pickerEntries computed property so systemContractsAllowingKeyBounds entries are
kept authoritative (i.e., prefer the system entry when IDs collide) — build
system entries first (from Self.systemContractsAllowingKeyBounds) and then
append only saved entries whose id is not present in the system set (derived
from systemContractsAllowingKeyBounds) so BoundsPickerEntry for DashPay retains
the system allowsContractScope and documentTypesAllowingBounds values rather
than being overwritten by the saved contract.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- Around line 1379-1449: The makeDashpayKeyPair helper writes derived private
keys to Keychain and registers pubkeys without verifying the private key
actually matches preview.publicKeyHex; add a validation step after deriving the
key and before calling KeychainManager.shared.storeIdentityPrivateKey by
invoking KeyValidation.validatePrivateKeyForPublicKey(preview.privateKeyData,
preview.publicKeyHex) (or equivalent) and throw a
PlatformWalletError.walletOperation if validation fails—mirror the check used in
AddIdentityKeyView.submit() so deriveIdentityAuthKeyAtSlot,
preview.publicKeyHex, preview.privateKeyData,
KeyValidation.validatePrivateKeyForPublicKey, and
KeychainManager.shared.storeIdentityPrivateKey are used to locate and implement
the guard.
- Around line 862-877: The submit() path currently appends DashPay keys whenever
addDashPayKeys is true, even for resume/unusedAssetLock flows where
dashpayKeysSection is hidden; update submit() to use the same gate used by the
UI (the condition that controls dashpayKeysSection visibility) before calling
makeDashpayKeyPair and appending keys so you only add keys for the flows that
show the dashpay UI; specifically, guard the block that calls
makeDashpayKeyPair/append to identityPubkeys (and uses Self.defaultKeyCount,
managedWallet, walletId, identityIndex, platformState.currentNetwork) with the
same resume/unusedAssetLock check that prevents dashpayKeysSection from showing,
ensuring resumeIdentityWithAssetLock(...) won’t see an altered key count.
---
Outside diff comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift`:
- Around line 222-234: When building contacts from establishedIds inside the
mapping (currently using wallet.getDashPayProfile), also call
managed.getEstablishedContact(contactId:) for each contactId, read
EstablishedContact.getAlias(), getNote(), and isHidden() and use those values to
populate the DashPayContact alias, note, and isHidden fields (falling back to
profile.displayName or the hex short id when alias is empty); ensure you still
set identityId and id as before and handle any nil/throwing calls safely (try?
for both wallet.getDashPayProfile and managed.getEstablishedContact) so filters
and note/alias rendering work correctly.
- Around line 202-215: The mapping that builds DashPayContactRequest in
FriendsView currently uses only IDs and thus loses real timestamps; update the
incomingIds and sentIds transforms to fetch the actual ContactRequest objects
via managed.getIncomingContactRequest(senderId:) and
managed.getSentContactRequest(recipientId:), read the
ContactRequest.getCreatedAt() (a UInt64 milliseconds-since-epoch), convert it to
a Date (Date(timeIntervalSince1970: Double(ms) / 1000.0)), and assign that Date
to DashPayContactRequest.createdAt so the UI shows the real request age; keep
existing id/senderId/recipientId assignments and handle optional/missing
ContactRequest by falling back to Date() or nil as appropriate.
---
Nitpick comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift`:
- Around line 290-339: The issue is that several fields on DashPayContact and
DashPayContactRequest are never populated; update the loadFriends() logic where
EstablishedContact and ContactRequest objects are mapped into DashPayContact and
DashPayContactRequest to call the SDK accessors and fill these fields: for
DashPayContact call EstablishedContact.getNote() to set note,
EstablishedContact.isHidden() to set isHidden, and perform the DPNS name lookup
(same logic used elsewhere) to set dpnsName before constructing DashPayContact;
for DashPayContactRequest call ContactRequest.getCreatedAt() to set createdAt
and fetch the sender’s profile/display name to set senderDisplayName before
constructing DashPayContactRequest. Ensure you use the DashPayContact and
DashPayContactRequest initializers (id, displayName, identityId, dpnsName, note,
isHidden) and (id, senderId, recipientId, createdAt, senderDisplayName)
respectively when replacing the current mappings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6d84bc6-d0e5-4a71-8558-f04615575f46
📒 Files selected for processing (24)
packages/rs-sdk/src/platform/dpns_usernames/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDPNSName.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayProfile.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsViewStubs.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundFromAssetLockPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ObservableDashPayService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageExplorerView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpIdentityView.swift
💤 Files with no reviewable changes (3)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ObservableDashPayService.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsViewStubs.swift
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3765 +/- ##
============================================
+ Coverage 87.17% 87.33% +0.15%
============================================
Files 2607 2590 -17
Lines 319589 317082 -2507
============================================
- Hits 278602 276911 -1691
+ Misses 40987 40171 -816
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR enables the DashPay iOS flow with focused DPNS, Keychain, and SwiftData fixes. Verified one blocking regression introduced by the new key-health tooling — ECDSA_HASH160 keys are misclassified as orphans because the check compares the 33-byte derived pubkey against the 20-byte HASH160 stored in PersistentPublicKey.publicKeyData (confirmed by AddIdentityKeyView's own HASH160 persistence path and the model's storage shape). Several suggestions follow on the new DashPay key path (lossy ContractBounds round-trip, raw-private-key/WIF surfacing) and on Keychain hygiene (stale deleteIdentityPrivateKey format, orphan delete leaves Keychain bytes).
🔴 1 blocking | 🟡 8 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift:194-231: Key-health check misclassifies ECDSA_HASH160 identity keys as orphans
`runCheck` compares `preview.publicKeyData` (the 33-byte compressed secp256k1 pubkey returned by `deriveIdentityAuthKeyAtSlot`) directly against `row.publicKeyData`. For `ECDSA_HASH160` keys those two values are never equal: `AddIdentityKeyView.swift:582-592` deliberately persists the 20-byte HASH160 as `pubkeyBytesForFFI`, and that 20-byte payload is what flows back through the persister callback into `PersistentPublicKey.publicKeyData` (`PlatformWalletPersistenceHandler.swift:1394`). The check therefore takes the `else` branch and emits `.orphan(...)`, which rolls the entire identity up to `.orphan` and exposes the destructive `Delete this identity` button (`WalletKeyHealthSheet.swift:509-515`) for an identity whose mnemonic-derived key is in fact correct. Because the sheet is reachable from Wallet Info as a repair tool, any wallet containing a HASH160 identity key will see this deterministic false positive. Branch on `keyType`: for `.ecdsaHash160`, compare against `KeychainManager.computePublicKeyHashHex(preview.publicKeyData)` (or its raw 20-byte form) instead of the 33-byte pubkey.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift:333-340: `deleteOrphan` leaves the orphan identity's private-key bytes in Keychain
`WalletKeyHealthChecker.deleteOrphan` cascades the SwiftData rows but does not delete the corresponding `identity_privkey.<walletIdHex>.<derivationPath>` (or legacy `identity_privkey.<derivationPath>`) Keychain entries. The user-facing alert (`WalletKeyHealthSheet.swift:424-432`) tells the user "all associated keys" are removed, which sets the wrong expectation. Items are `kSecAttrAccessibleWhenUnlockedThisDeviceOnly`, not biometric-gated. Snapshot `identity.publicKeys` before delete, then `SecItemDelete` rows at both the namespaced and legacy account names, gated on the same `metadata.walletId` check used by `cleanupLegacyKeychainEntry` to avoid clobbering another wallet's bytes.
In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift:794-807: `deleteIdentityPrivateKey(derivationPath:)` was not migrated to the walletId-namespaced account
This PR migrates `storeIdentityPrivateKey` to `identity_privkey.<walletIdHex>.<derivationPath>` (line 642) but leaves the sibling deleter constructing the legacy `identity_privkey.<derivationPath>` account string. `SecItemDelete` therefore never matches any row written by the new path; the helper returns success on `errSecItemNotFound` and silently leaks the secret bytes. Confirmed by grep: this helper has no current callers, so the defect is latent — but the API is still public and is an obvious next stop for any cleanup code that doesn't go through the new `deleteAllIdentityPrivateKeys(forWalletId:)`. Either require the same `(walletId, derivationPath)` pair the writer uses, or remove the helper.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1369-1396: DashPay keys' `ContractBounds` are dropped on persist round-trip
`makeDashpayKeyPair` registers the two DashPay keys with `ContractBounds.singleContractDocumentType(id: dashpayContractId, documentTypeName: "contactRequest")` (`CreateIdentityView.swift:1390-1393`), but the persister callback constructs `PersistentPublicKey` without touching `contractBoundsData` and never updates it on subsequent upserts. As a result, every persisted DashPay key resurfaces with `contractBounds == nil` via `toIdentityPublicKey()`. `TransitionDetailView` builds local DPP projections from `identity.identityPublicKeys` across many state transitions, so any local lookup of these keys will see them as unbounded rather than scoped to the contactRequest document type. Even when bounds *are* persisted, `PersistentPublicKey.from` / `toIdentityPublicKey()` only round-trip `[contractId]` → `.singleContract(id:)`, so the `documentTypeName` would still be lost. Carry the contract-bounds payload through the persister callback and extend `PersistentPublicKey` to preserve the document-type variant.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift:1406-1432: Default DashPay key path materializes raw private keys + WIFs across the FFI boundary
`makeDashpayKeyPair` derives the encryption/decryption pair via `ManagedPlatformWallet.deriveIdentityAuthKeyAtSlot(...)`, which crosses the FFI returning both `private_key_bytes` and `private_key_wif` into ARC-managed Swift objects. That is materially weaker than the existing callback-based `IdentityKeyPersister.persist` flow, which keeps the secret short-lived and never serializes it as a WIF string. Because `addDashPayKeys` is now default-on, every wallet-backed identity registration materializes two extra private scalars and two WIF strings on the Swift side. Per `packages/swift-sdk/CLAUDE.md`, this is the kind of "derive + write" pipeline that should live in `platform-wallet` behind a single FFI entry point that returns to Swift only what's needed for the Keychain write.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift:1395-1436: Pre-registration DashPay key writes leave Keychain orphans with empty `identityId` on failed submit
`makeDashpayKeyPair` writes private bytes to Keychain with `IdentityPrivateKeyMetadata.identityId = ""` and relies on the post-on-chain persister callback to overwrite the field. If registration aborts between this write and a successful land (network failure, app kill, asset-lock rejection), the two 32-byte secrets stay in Keychain at the namespaced account with an empty `identityId` and no anchoring `PersistentPublicKey` row. They won't show up as orphans in the health sheet either, since the sheet walks identities. Two options: defer the Keychain write until the registration call succeeds (and let the persister callback own it), or sweep the pre-derived entries on submit error.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift:1406-1411: DashPay encryption/decryption keys derived via the IDENTITY_AUTHENTICATION DIP-9 sub-feature
`deriveIdentityAuthKeyAtSlot(identityIndex:, keyId:)` hardcodes the base path to `IDENTITY_AUTHENTICATION_PATH_{MAINNET|TESTNET}` regardless of caller purpose; this PR makes that the default DashPay encryption/decryption derivation. Two consequences: (1) the derived bytes are a pure function of `(identity_index, key_index)` with no purpose component, so if any future flow ever derives an *authentication* key at the same `keyId` it produces the same 32-byte secret — ECDH and ECDSA sharing a private key is a textbook reuse hazard, and the `nextKeyId = max + 1` allocator is the only thing keeping the invariant. (2) A standards-compliant DIP-9 wallet restoring from the same mnemonic will look for DashPay keys under the encryption sub-feature path, not under `5'/0'`, and will silently miss them. Per `packages/swift-sdk/CLAUDE.md`, the right fix is in Rust: extend `platform-wallet` so callers can request an encryption-purpose keypair via the proper sub-feature, and call it here.
In `packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift:252-263: Hardcoded `platform_version = 11` for mainnet/testnet will silently break at the next protocol activation
When mainnet or testnet activates v12+, every SDK consumer using this default will keep emitting v11 `getDocuments` wire shape and start hitting the very `"could not decode data contracts query"` failure this branch is fixing. There is no compile-time link between this constant and `PLATFORM_VERSIONS` in `rs-platform-version`, so a protocol bump won't trigger any indicator. Best fix: probe the active protocol version on init (the auto-discover masternode path is precedent), or expose a Rust-side helper `dash_sdk_default_platform_version_for_network` so the mapping lives next to the version tables. At minimum, add a tracked TODO with a clear trigger.
In `packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/dpns_usernames/mod.rs:406-413: `is_dpns_name_available` correctness fix has no automated test
The semantic change from `documents.is_empty()` to `documents.values().all(|d| d.is_none())` is the central correctness fix in the PR and drives user-visible DPNS registration UX, but is exercised only manually via the iOS app. Commit 2c9e9af692 just relocated DPNS network tests under `tests/`, so there is now a natural home for a positive case (proven-non-existence map → `Ok(true)`) and a negative case (`Some(Document)` entry → `Ok(false)`). Add both so a future change to `Document::fetch_many`'s return shape can't regress the semantics silently.
| for (keyId, purpose) in purposes { | ||
| let preview = try managedWallet.deriveIdentityAuthKeyAtSlot( | ||
| identityIndex: identityIndex, | ||
| keyId: keyId, | ||
| network: network | ||
| ) | ||
| let pubKeyHashHex = SwiftDashSDK.KeychainManager.computePublicKeyHashHex( | ||
| preview.publicKeyData | ||
| ) | ||
| let metadata = IdentityPrivateKeyMetadata( | ||
| identityId: identityIdPlaceholder, | ||
| keyId: keyId, | ||
| walletId: walletIdHex, | ||
| identityIndex: identityIndex, | ||
| keyIndex: keyId, | ||
| derivationPath: preview.derivationPath, | ||
| publicKey: preview.publicKeyHex, | ||
| publicKeyHash: pubKeyHashHex, | ||
| keyType: KeyType.ecdsaSecp256k1.rawValue, | ||
| purpose: purpose.rawValue, | ||
| securityLevel: SecurityLevel.medium.rawValue | ||
| ) | ||
| guard KeychainManager.shared.storeIdentityPrivateKey( | ||
| preview.privateKeyData, | ||
| derivationPath: preview.derivationPath, | ||
| metadata: metadata | ||
| ) != nil else { |
There was a problem hiding this comment.
🟡 Suggestion: Default DashPay key path materializes raw private keys + WIFs across the FFI boundary
makeDashpayKeyPair derives the encryption/decryption pair via ManagedPlatformWallet.deriveIdentityAuthKeyAtSlot(...), which crosses the FFI returning both private_key_bytes and private_key_wif into ARC-managed Swift objects. That is materially weaker than the existing callback-based IdentityKeyPersister.persist flow, which keeps the secret short-lived and never serializes it as a WIF string. Because addDashPayKeys is now default-on, every wallet-backed identity registration materializes two extra private scalars and two WIF strings on the Swift side. Per packages/swift-sdk/CLAUDE.md, this is the kind of "derive + write" pipeline that should live in platform-wallet behind a single FFI entry point that returns to Swift only what's needed for the Keychain write.
source: ['codex']
There was a problem hiding this comment.
Acknowledged, not addressed in this commit. Migrating makeDashpayKeyPair to the callback-based persister flow (no raw private_key_bytes / private_key_wif across FFI) requires an analog of prePersistIdentityKeysForRegistration that accepts a custom (keyId, purpose, securityLevel, contractBounds) schedule — the existing helper only knows the default auth-key policy. Tracked as a follow-up so addDashPayKeys: true defaults don't materialize secrets ARC-managed on the Swift side. Mitigation in this commit: KeyValidation.validatePrivateKeyForPublicKey runs before the Keychain write so a mis-derived key fails fast.
| let identityIdPlaceholder = "" // overwritten by persister callback | ||
| // when the identity actually | ||
| // lands on-chain; the metadata | ||
| // we write here only needs to | ||
| // satisfy the keychain | ||
| // round-trip lookup by pubkey | ||
| // hex. | ||
|
|
||
| var rows: [ManagedPlatformWallet.IdentityPubkey] = [] | ||
| rows.reserveCapacity(purposes.count) | ||
|
|
||
| for (keyId, purpose) in purposes { | ||
| let preview = try managedWallet.deriveIdentityAuthKeyAtSlot( | ||
| identityIndex: identityIndex, | ||
| keyId: keyId, | ||
| network: network | ||
| ) | ||
| let pubKeyHashHex = SwiftDashSDK.KeychainManager.computePublicKeyHashHex( | ||
| preview.publicKeyData | ||
| ) | ||
| let metadata = IdentityPrivateKeyMetadata( | ||
| identityId: identityIdPlaceholder, | ||
| keyId: keyId, | ||
| walletId: walletIdHex, | ||
| identityIndex: identityIndex, | ||
| keyIndex: keyId, | ||
| derivationPath: preview.derivationPath, | ||
| publicKey: preview.publicKeyHex, | ||
| publicKeyHash: pubKeyHashHex, | ||
| keyType: KeyType.ecdsaSecp256k1.rawValue, | ||
| purpose: purpose.rawValue, | ||
| securityLevel: SecurityLevel.medium.rawValue | ||
| ) | ||
| guard KeychainManager.shared.storeIdentityPrivateKey( | ||
| preview.privateKeyData, | ||
| derivationPath: preview.derivationPath, | ||
| metadata: metadata | ||
| ) != nil else { | ||
| throw PlatformWalletError.walletOperation( | ||
| "Could not persist DashPay key (kid \(keyId), purpose \(purpose.name)) to Keychain" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Pre-registration DashPay key writes leave Keychain orphans with empty identityId on failed submit
makeDashpayKeyPair writes private bytes to Keychain with IdentityPrivateKeyMetadata.identityId = "" and relies on the post-on-chain persister callback to overwrite the field. If registration aborts between this write and a successful land (network failure, app kill, asset-lock rejection), the two 32-byte secrets stay in Keychain at the namespaced account with an empty identityId and no anchoring PersistentPublicKey row. They won't show up as orphans in the health sheet either, since the sheet walks identities. Two options: defer the Keychain write until the registration call succeeds (and let the persister callback own it), or sweep the pre-derived entries on submit error.
source: ['claude']
There was a problem hiding this comment.
Acknowledged, not addressed in this commit. Pre-registration Keychain writes do leave entries with identityId = "" if submit aborts, and the health sheet doesn't currently iterate them (it walks identities). The cleaner fix is to defer the Keychain write into the persister-callback path so it only lands after on-chain success — same end state as the raw-private-keys-across-FFI item above and they're naturally addressed together. Tracked as a follow-up; in the meantime the entries are recoverable via the (existing) wallet-delete flow's deleteAllIdentityPrivateKeys(forWalletId:) sweep.
| for (keyId, purpose) in purposes { | ||
| let preview = try managedWallet.deriveIdentityAuthKeyAtSlot( | ||
| identityIndex: identityIndex, | ||
| keyId: keyId, | ||
| network: network | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: DashPay encryption/decryption keys derived via the IDENTITY_AUTHENTICATION DIP-9 sub-feature
deriveIdentityAuthKeyAtSlot(identityIndex:, keyId:) hardcodes the base path to IDENTITY_AUTHENTICATION_PATH_{MAINNET|TESTNET} regardless of caller purpose; this PR makes that the default DashPay encryption/decryption derivation. Two consequences: (1) the derived bytes are a pure function of (identity_index, key_index) with no purpose component, so if any future flow ever derives an authentication key at the same keyId it produces the same 32-byte secret — ECDH and ECDSA sharing a private key is a textbook reuse hazard, and the nextKeyId = max + 1 allocator is the only thing keeping the invariant. (2) A standards-compliant DIP-9 wallet restoring from the same mnemonic will look for DashPay keys under the encryption sub-feature path, not under 5'/0', and will silently miss them. Per packages/swift-sdk/CLAUDE.md, the right fix is in Rust: extend platform-wallet so callers can request an encryption-purpose keypair via the proper sub-feature, and call it here.
source: ['claude']
There was a problem hiding this comment.
Acknowledged, not addressed in this commit. The fix the reviewer describes (extend platform-wallet with an encryption-purpose sub-feature derivation under the proper DIP-9 path, then route Swift through a single FFI entry point) is the right shape and belongs in Rust per packages/swift-sdk/CLAUDE.md. Filed as a follow-up. The collision hazard the comment flags is real but bounded by nextKeyId = max + 1's strict-monotonic invariant today; landing a DIP-9-compliant path before any external wallet recovers from the same mnemonic is the long-term remediation.
| let documents = Document::fetch_many(self, query).await?; | ||
|
|
||
| // If no documents found, the name is available | ||
| Ok(documents.is_empty()) | ||
| // `Document::fetch_many` returns `BTreeMap<Identifier, Option<Document>>` | ||
| // — a non-existence proof comes back as a non-empty map whose values are | ||
| // all `None`. Checking `documents.is_empty()` would treat a proven | ||
| // non-existence as "taken". The name is available iff no entry in the | ||
| // map carries an actual document. | ||
| Ok(documents.values().all(|d| d.is_none())) |
There was a problem hiding this comment.
🟡 Suggestion: is_dpns_name_available correctness fix has no automated test
The semantic change from documents.is_empty() to documents.values().all(|d| d.is_none()) is the central correctness fix in the PR and drives user-visible DPNS registration UX, but is exercised only manually via the iOS app. Commit 2c9e9af just relocated DPNS network tests under tests/, so there is now a natural home for a positive case (proven-non-existence map → Ok(true)) and a negative case (Some(Document) entry → Ok(false)). Add both so a future change to Document::fetch_many's return shape can't regress the semantics silently.
source: ['claude']
There was a problem hiding this comment.
Acknowledged, not addressed in this commit. An automated test for is_dpns_name_available (positive Ok(true) on proven-non-existence Some(None) map + negative Ok(false) on Some(Document)) is the right gate against silent regressions in Document::fetch_many semantics. Filed as a follow-up — slotting it into the existing tests/ directory is cheap once the iOS PR is in; doing it here would balloon the diff with test-only infra unrelated to the DashPay flow.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "75e21716cbb2d3b3e37786c6dc56148f33e41c12950061d3644a336a8431ed80"
)Xcode manual integration:
|
- rs-sdk: fix DPNS availability false-positive — non-existence proofs come back as an IndexMap with `None` values, not an empty map, so `documents.is_empty()` was reporting available names as taken. Switched to `values().all(is_none)`. - swift-sdk SDK: default `platformVersion` is now network-aware (11 for mainnet/testnet, 12 for devnet/regtest) so we don't emit the V1 `getDocuments` wire format against pre-v3.1 nodes that reject it with "could not decode data contracts query". - swift-sdk PlatformWalletPersistenceHandler.deleteWalletData: switched to a four-phase save (identity-children → identities → accounts → wallet) so SwiftData's save-time inverse cleanup never has to touch a non-optional inverse on a row in the same batch as the parent's delete. Avoids the `Cannot remove PersistentX from relationship Y on PersistentZ` fatal at the cost of atomicity across phases (acceptable for a user-initiated wipe). All model relationships stay non-Optional. - swift-sdk keychain: namespace identity-private-key keychain accounts by walletId (`identity_privkey.<walletIdHex>.<path>`). The old per-path scheme collided whenever two wallets had an identity at the same `identity_index`, leaving prior rows pointing at another wallet's private bytes. - swift-example-app AddIdentityKeyView: surface DashPay as a "(System)" entry in the contract-bounds picker; lock KeyType to ECDSA secp256k1 + SecurityLevel to Medium when purpose is Encryption/Decryption (DPP enforces both); require the document type binding when the contract only declares the bounded-key requirement at document-type level (DashPay → contactRequest), with auto-fill. - swift-example-app CreateIdentityView: new default-on toggle "Register DashPay keys" that registers 2 extra keys (Encryption + Decryption, MEDIUM, ECDSA secp256k1, bound to DashPay → contactRequest) alongside the default auth keys. Asset-lock minimum auto-scales for the extra `identity_key_in_creation_cost`. - swift-example-app WalletKeyHealthSheet: new diagnostic in Wallet Info — re-derives each `PersistentPublicKey` from the wallet mnemonic, classifies healthy / needsRederive / orphan, and offers Re-derive (writes the new-format keychain entry + sweeps the legacy one) or Delete Identity. - swift-example-app WalletRowView.platformBalance + BalanceCardView.platformBalance: skip identities whose `modelContext` is nil to avoid reading invalidated rows during a cascade delete. - Dead code removed: stale `DashPayService` (Swift SDK; hardcoded key indices, didn't go through the new FFI), `ObservableDashPayService` (empty wrapper, never used), `FriendsViewStubs` (folded real types into FriendsView). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/thirsty-williams-6e9d18
branch
from
c05ca3e to
2bf062c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (2)
1406-1436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate each derived DashPay key before persisting.
makeDashpayKeyPairwrites private keys to Keychain without verifying they match the returned public key.AddIdentityKeyView.submit()performs this validation (lines 506-518), but this helper skips it. A mismatch would register an identity with unusable DashPay keys.Suggested fix
for (keyId, purpose) in purposes { let preview = try managedWallet.deriveIdentityAuthKeyAtSlot( identityIndex: identityIndex, keyId: keyId, network: network ) + guard KeyValidation.validatePrivateKeyForPublicKey( + privateKeyHex: preview.privateKeyData.toHexString(), + publicKeyHex: preview.publicKeyHex, + keyType: .ecdsaSecp256k1, + network: network + ) else { + throw PlatformWalletError.walletOperation( + "Derived DashPay key didn't match its public key (kid \(keyId), purpose \(purpose.name))" + ) + } let pubKeyHashHex = SwiftDashSDK.KeychainManager.computePublicKeyHashHex( preview.publicKeyData )As per coding guidelines, "Always validate private keys match their public keys using
KeyValidation.validatePrivateKeyForPublicKey."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift` around lines 1406 - 1436, In the for-loop inside CreateIdentityView (where deriveIdentityAuthKeyAtSlot returns `preview`) validate that the derived private key matches the returned public key before calling `KeychainManager.shared.storeIdentityPrivateKey`; specifically call `KeyValidation.validatePrivateKeyForPublicKey` (the same check used in `AddIdentityKeyView.submit`) with `preview.privateKeyData` and `preview.publicKeyHex` and throw a `PlatformWalletError.walletOperation` if validation fails, then proceed to store only when validation succeeds.
862-877:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate
addDashPayKeyscheck to match UI visibility condition.The UI hides
dashpayKeysSectionfor.unusedAssetLock(resume) flows, butsubmit()still checksaddDashPayKeysunconditionally. If a user toggles DashPay keys on, then switches to resume an existing asset lock, the extra keys are still appended—potentially causingresumeIdentityWithAssetLockto fail against an outpoint funded for the original 3-key registration.Suggested fix
Add a computed property and use it in both
plannedIdentityKeyCountandsubmit():+ /// Whether to register DashPay keys for this submission. Matches + /// the UI gate so resume/walletless paths never silently add keys. + private var shouldRegisterDashPayKeys: Bool { + guard case .wallet = walletSelection, + fundingSelection != .unusedAssetLock else { + return false + } + return addDashPayKeys + } private var plannedIdentityKeyCount: UInt32 { - Self.defaultKeyCount + (addDashPayKeys ? Self.dashPayExtraKeyCount : 0) + Self.defaultKeyCount + (shouldRegisterDashPayKeys ? Self.dashPayExtraKeyCount : 0) } // In submit(): - if addDashPayKeys { + if shouldRegisterDashPayKeys { do { identityPubkeys.append(contentsOf: try makeDashpayKeyPair(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift` around lines 862 - 877, The submit flow is appending DashPay keys even when the UI hides dashpayKeysSection for the .unusedAssetLock (resume) flow; add a computed Bool (e.g., isDashPayVisible or canAddDashPayKeys) that encodes the same visibility condition used by dashpayKeysSection, then use that property instead of directly checking addDashPayKeys in both plannedIdentityKeyCount and submit() so DashPay keys are only added when the UI actually allows them (affecting makeDashpayKeyPair, addDashPayKeys, plannedIdentityKeyCount, and resumeIdentityWithAssetLock code paths).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 2631-2637: The current flow deletes Core Data identity rows
(backgroundContext.delete(identity) + try backgroundContext.save()) before the
caller runs PlatformWalletManager.deleteWallet(...) keychain cleanup, which can
break retries because identityIdsForWallet becomes empty; fix by ensuring
keychain items are removed before committing identity deletion: either iterate
identityIdsForWallet and call deleteAllKeychainItems(forIdentityId:) for each id
prior to performing backgroundContext.delete(identity) and save(), or
retain/persist the identityIdsForWallet across retries (e.g., return or store
the IDs so deleteAllKeychainItems(forIdentityId:) can be retried safely) so the
caller’s cleanup can always find the identity IDs.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`:
- Around line 1367-1370: Replace the direct property access in FieldRow so
AccountStorageDetailView uses the shared helper: instead of reading
record.wallet.name or record.wallet.walletId directly, call
walletLabel(record.wallet) (the same approach used in IdentityStorageDetailView)
to build the display string; update the FieldRow with label "Wallet" to use
walletLabel(record.wallet) to centralize formatting and avoid crashes when
record.wallet is nil or invalidated.
---
Duplicate comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- Around line 1406-1436: In the for-loop inside CreateIdentityView (where
deriveIdentityAuthKeyAtSlot returns `preview`) validate that the derived private
key matches the returned public key before calling
`KeychainManager.shared.storeIdentityPrivateKey`; specifically call
`KeyValidation.validatePrivateKeyForPublicKey` (the same check used in
`AddIdentityKeyView.submit`) with `preview.privateKeyData` and
`preview.publicKeyHex` and throw a `PlatformWalletError.walletOperation` if
validation fails, then proceed to store only when validation succeeds.
- Around line 862-877: The submit flow is appending DashPay keys even when the
UI hides dashpayKeysSection for the .unusedAssetLock (resume) flow; add a
computed Bool (e.g., isDashPayVisible or canAddDashPayKeys) that encodes the
same visibility condition used by dashpayKeysSection, then use that property
instead of directly checking addDashPayKeys in both plannedIdentityKeyCount and
submit() so DashPay keys are only added when the UI actually allows them
(affecting makeDashpayKeyPair, addDashPayKeys, plannedIdentityKeyCount, and
resumeIdentityWithAssetLock code paths).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fe33e52-67aa-41d6-804d-4da9e6fd9dd1
📒 Files selected for processing (15)
packages/rs-sdk/src/platform/dpns_usernames/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsViewStubs.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ObservableDashPayService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
💤 Files with no reviewable changes (3)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsViewStubs.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ObservableDashPayService.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayService.swift
✅ Files skipped from review due to trivial changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-sdk/src/platform/dpns_usernames/mod.rs
- packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at head 2bf062c. The latest delta (c05ca3e..2bf062c) restores non-Optional SwiftData inverses on PersistentAccount/DPNSName/DashpayProfile/DashpayContactRequest and introduces a four-phase save() in PlatformWalletPersistenceHandler.deleteWalletData to avoid the SwiftData inverse-nullify fatal. The four-phase delete in deleteWalletData is internally consistent, but the sibling WalletKeyHealthSheet.deleteOrphan was not given the same treatment AND its doc comment now lies about the relationships being Optional — that is the one new blocking issue in the latest delta. All 10 prior findings from the c05ca3e review remain STILL VALID; the latest delta did not touch any of those code regions. Status reconciliation: 10 prior findings STILL VALID (all carried forward), 0 FIXED, 0 OUTDATED, 0 INTENTIONALLY DEFERRED. New in latest delta: 1 blocking (deleteOrphan stale comment + non-Optional inverse fatal risk) and 1 nitpick (SendTransactionView.platformBalance missing the same mid-cascade-delete guard added to BalanceCardView). Overflow: 2 lower-confidence nitpicks (SendTransactionView guard asymmetry and rederive silently swallowing Keychain failures) dropped from the report to fit the 10-finding budget.
🔴 1 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
7 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift:327-340: [NEW in latest delta] `deleteOrphan` doc-comment is stale and the single-save path will fatal under the now non-Optional inverses
The latest delta restores `PersistentDPNSName.identity` (PersistentDPNSName.swift:84) and `PersistentDashpayProfile.identity` (PersistentDashpayProfile.swift:86) to non-Optional, and adds the explicit four-phase delete in `PlatformWalletPersistenceHandler.deleteWalletData` precisely to avoid the SwiftData `Cannot remove PersistentX from relationship Y on PersistentZ because an appropriate default value is not configured` fatal that fires when a save would null out a non-Optional inverse. `deleteOrphan` at 333-340 was not given the same treatment: it still calls `modelContext.delete(identity)` followed by a single `try modelContext.save()`. If the orphan identity has any attached DPNS name, DashPay profile, or contact-request row, that single save will reproduce the exact fatal the PR author called out elsewhere. The path is reachable from the `Delete this identity` button in this sheet — an orphan identity reached on a wallet re-imported from a different mnemonic can plausibly still own DPNS labels or a DashPay profile from the previous wallet. Compounding the risk, the doc comment on lines 328-332 still claims `PersistentDPNSName.identity`, `PersistentDashpayProfile.identity`, and `PersistentDashpayContactRequest.owner` 'are all Optional', which is no longer true — the comment is now actively misleading anyone who reads it to verify the safety claim. Apply the same per-layer-then-save sequencing as `deleteWalletData` (delete `identity.dpnsNames`, `identity.dashpayProfile`, `identity.contactRequests` first, save, then delete the identity and save again) and update the doc comment to reflect the new relationship shape.
In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift:794-807: [CARRIED FORWARD — STILL VALID] `deleteIdentityPrivateKey(derivationPath:)` not migrated to the walletId-namespaced account
Verified at head 2bf062c0 — line 796 still constructs `identity_privkey.\(derivationPath)`, while `storeIdentityPrivateKey` now writes `identity_privkey.\(walletIdHex).\(derivationPath)`. `SecItemDelete` therefore never matches any row written by the new path; the helper returns success on `errSecItemNotFound` and silently leaves the secret bytes in Keychain. No current in-tree callers, so the defect is latent — but the API is still public and is the obvious next stop for any future cleanup that doesn't go through `deleteAllIdentityPrivateKeys(forWalletId:)`. Either require the same `(walletId, derivationPath)` pair the writer uses, or remove the helper.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1369-1409: [CARRIED FORWARD — STILL VALID] DashPay keys' `ContractBounds` are dropped on persist round-trip
Still valid. `makeDashpayKeyPair` registers the two DashPay keys with `ContractBounds.singleContractDocumentType(id: dashpayContractId, documentTypeName: "contactRequest")` (CreateIdentityView.swift:1390-1393), but `PersistentPublicKey` serializes bounds as a bare `[contractId]` list and reconstructs them with `.singleContract(id:)` — the `documentTypeName` is dropped entirely. Confirming grep: no reference to `contractBoundsData` or `contract_bounds` in the persister callback or `PersistentPublicKey`. After one persistence round-trip the local `IdentityPublicKey` projection no longer matches the key actually registered on-chain; any local DPP code reading `identity.identityPublicKeys` sees a weaker, semantically different bound than what the PR introduces. Carry the contract-bounds payload through the persister callback and extend `PersistentPublicKey` to preserve the `documentTypeName` variant.
- WalletKeyHealthSheet: branch HASH160 keys against pubkey-hash hex instead of comparing the 33-byte derived pubkey to the 20-byte stored hash; report unsupported key types as orphan with a clear reason. Rederive returns per-key failures so the sheet can show WHICH key is stuck. deleteOrphan also wipes the identity's Keychain entries. - KeychainManager: deleteIdentityPrivateKey now takes walletId (symmetric with store) and also sweeps the legacy account. Added deleteAllIdentityPrivateKeys(forIdentityIdBase58:). - AddIdentityKeyView: system contract entries always win over user-saved rows that share an ID, so DashPay's bounded document- type metadata isn't masked by a parallel saved-contract entry. - CreateIdentityView: shouldRegisterDashPayKeys computed gate mirrors the UI section visibility; submit() and the funding-min calc both route through it. KeyValidation.validatePrivateKeyForPublicKey added to makeDashpayKeyPair before persisting. - PlatformWalletManager.deleteWallet: keychain cleanup moved BEFORE deleteWalletData so a partial-failure retry can still find the identityIds to purge. - StorageRecordDetailViews: AccountStorageDetailView now uses the walletLabel(record.wallet) helper. - SDK.swift: platform_version=11 pin has an explicit TODO + trigger describing when to bump it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fatal `PersistentDPNSName.identity`, `PersistentDashpayProfile.identity`, and `PersistentDashpayContactRequest.owner` are non-Optional, which makes a single-save `modelContext.delete(identity)` fatal whenever SwiftData has to null out their inverses in the same save batch. `WalletKeyHealthChecker.deleteOrphan` now mirrors `PlatformWalletPersistenceHandler.deleteWalletData`'s per-layer approach: pre-delete DPNS names + DashPay profile + contact requests + save, then delete the identity + save. Updated the stale doc comment that claimed the inverses are Optional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Self reviewed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- Around line 808-825: The loop that unconditionally deletes legacyAccount risks
removing another wallet's key; change the logic so that before calling
SecItemDelete for legacyAccount you perform a SecItemCopyMatching query for the
legacyAccount (using kSecClassGenericPassword, kSecAttrService, kSecAttrAccount
and kSecReturnData) retrieve and decode the stored IdentityPrivateKeyMetadata,
and only call SecItemDelete when the decoded IdentityPrivateKeyMetadata.walletId
equals walletIdHex (mirror the guarded cleanup used in WalletKeyHealthSheet);
keep the unconditional delete for newAccount but gate deletion of legacyAccount
behind this metadata walletId check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcb0d9a1-e2d9-4f4f-a252-d5a3ecae7599
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddIdentityKeyView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift
|
Actionable comments posted: 0 |
…tity-key persister
`PersistentPublicKey` was serializing ContractBounds as a bare
`[contractId]` list and reconstructing them as `.singleContract(id:)`,
silently dropping the `documentTypeName` qualifier. DashPay's
encryption/decryption keys register with
`.singleContractDocumentType(id: dashpay, documentTypeName:
"contactRequest")`; after one SwiftData round-trip the local DPP
projection on `identity.identityPublicKeys` was a weaker
`.singleContract` bound that didn't match the on-chain key.
Extended the FFI struct + Swift persistence shape to round-trip
both ContractBounds variants verbatim:
Rust (rs-platform-wallet-ffi):
- IdentityKeyEntryFFI gains contract_bounds_{kind, id, document_type}.
Kind tag mirrors DPP's `ContractBounds` (0=none, 1=SingleContract,
2=SingleContractDocumentType); doc-type is a heap CString released
via free_identity_key_entry_ffi.
- Layout assert bumped (136 → 184 bytes).
- 2 new tests covering both bound variants + idempotent free.
Swift (swift-sdk):
- IdentityKeyEntrySnapshot carries ManagedPlatformWallet.ContractBounds.
- persistIdentityKeysCallback projects the FFI trio into the enum.
- persistIdentityKeys writes both columns on insert AND update.
- PersistentPublicKey grows a contractBoundsDocumentTypeName column;
toIdentityPublicKey()/from(IdentityPublicKey) round-trip both
variants. Legacy stores without the new column load cleanly
(per repo policy — dev stores rebuild on schema changes).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed both carry-forward findings flagged in the latest review:
DashPay ContractBounds round-trip drops
|
`deleteIdentityPrivateKey(walletId:derivationPath:)` was deleting `identity_privkey.<path>` (the legacy, non-namespaced account) unconditionally. The legacy format collided across wallets — that is the bug we are fixing — so an unconditional delete could clobber a different wallet's row at the same DIP-9 path. Promoted `WalletKeyHealthSheet.cleanupLegacyKeychainEntry` (which already did the right thing — lookup metadata, decode, match walletId, delete) into a public `KeychainManager` helper `deleteLegacyKeychainEntryIfOwnedByWallet(walletIdHex:derivationPath:)`. Both `deleteIdentityPrivateKey` and the post-rederive cleanup in `WalletKeyHealthSheet` now route through it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at head ea4c16a. The latest delta (2bf062c..ea4c16a) resolves 4 prior findings: ECDSA_HASH160 misclassification is FIXED via the new keyType switch; deleteIdentityPrivateKey is FIXED with the new (walletId, derivationPath) signature; deleteOrphan Keychain wipe is FIXED via the new deleteAllIdentityPrivateKeys(forIdentityIdBase58:) helper; the platform_version hardcode is FIXED at the minimum bar with a tracked TODO. Status of remaining prior findings: 6 STILL VALID and carried forward (Swift-side DashPay derive pipeline materializing raw scalars, encryption keys via auth DIP-9 sub-feature, pre-registration empty-identityId placeholder, ContractBounds documentTypeName drop on round-trip, is_dpns_name_available missing tests, deleteOrphan single-save cascade hazard). New in latest delta: 1 blocking — the new deleteOrphan adopted the Keychain wipe but did NOT adopt the per-layer-save pattern that deleteWalletData (PlatformWalletPersistenceHandler.swift:2594-2650) had to introduce for non-optional inverses, and because the Keychain wipe now runs FIRST, a SwiftData fatal on save leaves the user without private keys AND with the identity row still present. Two new latent suggestions: the legacy-account branch of the new deleteIdentityPrivateKey is not gated on walletId metadata (cross-wallet collision the PR aimed to retire), and the new deleteAllIdentityPrivateKeys(forIdentityIdBase58:) silently skips items with undecodable metadata via try?. Review action: REQUEST_CHANGES on the one blocking concern.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3765 ea4c16a
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 597, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._
Reviewed commit: ea4c16a
🔴 1 blocking | 🟡 7 suggestion(s)
Verified findings
blocking: [CARRIED-FORWARD — partially addressed in latest delta] deleteOrphan still single-saves; will fatal when the orphan owns DPNS / DashPay rows, and the Keychain wipe runs FIRST so the user is stranded
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift (line 397)
The latest delta added KeychainManager.shared.deleteAllIdentityPrivateKeys(forIdentityIdBase58:) at line 407 and updated the doc comment, but the SwiftData side still does a single modelContext.delete(identity) + try modelContext.save() (lines 413-414). The relationship inverse PersistentDPNSName.identity is non-optional (PersistentDPNSName.swift:84, with an explicit comment stating SwiftData's non-optional contract is honored), and the same applies to PersistentDashpayProfile.identity and PersistentDashpayContactRequest.owner. The codebase's own PlatformWalletPersistenceHandler.deleteWalletData (PlatformWalletPersistenceHandler.swift:2594-2649) carries a long PHASE 1/2/3 comment explaining that SwiftData fatals with Cannot remove PersistentX from relationship Y on PersistentZ because an appropriate default value is not configured whenever a non-optional inverse is nulled in the same save batch, and works around this with per-layer saves (delete dpnsNames + dashpayProfile + contactRequests, save; delete identity, save). deleteOrphan did not adopt that pattern. Any orphan identity that still owns even one DPNS name, DashPay profile, or contact request will fatal at line 414. Worse, because the Keychain wipe at line 407 now runs FIRST, the fatal leaves the user with the private-key bytes already gone AND the SwiftData identity row still in place — the diagnostic sheet has effectively destroyed the user's keys without finishing the cleanup. An orphan reached by re-importing a different mnemonic is precisely the case this sheet exists to repair, and is plausibly the case where leftover DPNS labels or a DashPay profile from the previous wallet are still present. Apply the same per-layer-then-save sequencing as deleteWalletData, and move the Keychain wipe AFTER the SwiftData saves succeed so a mid-flow failure doesn't strand the user without keys.
suggestion: [NEW in latest delta] deleteIdentityPrivateKey(walletId:derivationPath:) legacy-account branch is not gated on walletId metadata — can clobber a sibling wallet's legacy key
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (line 804)
The new (walletId, derivationPath) overload deletes both identity_privkey.<walletIdHex>.<derivationPath> AND identity_privkey.<derivationPath> (legacy), keyed only on the account name for the legacy entry — no kSecAttrGeneric metadata.walletId check. Compare with deleteAllIdentityPrivateKeys(forIdentityIdBase58:) at line 877 and deleteAllIdentityPrivateKeys(forWalletId:) which gate on metadata. The legacy identity_privkey.<path> account is precisely the cross-wallet-colliding name this PR is retiring (PR description: two wallets at identity_index = 0 keep distinct keys in Keychain). If wallet A still has a partially-migrated legacy entry at path X, and any caller invokes deleteIdentityPrivateKey(walletId: B, derivationPath: X), wallet A's legacy bytes are silently deleted. No in-tree callers today (verified), so the defect is latent — but the function is public, the comment positions it as the symmetric counterpart to storeIdentityPrivateKey, and the obvious next step (per-key cleanup outside the wholesale forWalletId: sweep) goes through here. Gate the legacy delete on metadata.walletId == thisWallet (matching cleanupLegacyKeychainEntry's safety property), or drop the legacy branch and require legacy cleanup to go through the metadata-gated helpers.
suggestion: [NEW in latest delta] deleteAllIdentityPrivateKeys(forIdentityIdBase58:) silently skips items with undecodable metadata, leaving orphan private bytes behind
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (line 862)
The new helper iterates every identity_privkey.* keychain item, decodes the kSecAttrGeneric metadata blob with try?, and skips items whose decode fails (lines 869-875). It is invoked by the sheet's Delete this identity button (the fix for the prior deleteOrphan finding). Any keychain row whose IdentityPrivateKeyMetadata JSON cannot be decoded — corruption, a partial/aborted write (the pre-registration DashPay-keys path explicitly writes identityId: "" placeholders), or a future schema rename — is silently dropped from the scan. The 32-byte private bytes remain in Keychain after the user is told the orphan was cleaned up. The most likely producers of malformed metadata are exactly the failure modes this PR is repairing, so at minimum log undecodable rows so the developer/user is aware, or treat metadata-decode-failure on an identity_privkey.* row as itself an orphan signal (no matching PersistentPublicKey.pkid == account anywhere) and remove it. The try? swallows both signals today.
suggestion: [CARRIED-FORWARD — STILL VALID] Default DashPay key path materializes raw private keys + WIFs across the FFI, violating the Swift SDK rule
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (line 1394)
Re-verified at head ea4c16a. makeDashpayKeyPair (CreateIdentityView.swift:1394-1485) still loops (encryption, decryption) and calls managedWallet.deriveIdentityAuthKeyAtSlot(...) per slot, copying preview.privateKeyData (32 raw scalar bytes) plus preview.privateKeyWif into ARC-managed Swift values, then Swift orchestrates the validate-then-Keychain-write pipeline. The latest delta added a KeyValidation.validatePrivateKeyForPublicKey cross-check on the hex'd privkey + pubkey (lines 1435-1446) — sensible defense against marshalling drift, but it cements the FFI contract that delivers raw secrets into Swift for orchestration. The Swift SDK's CLAUDE.md names this exact shape ("orchestrating multi-step derivation pipelines in Swift (mnemonic → seed → path → key → store)") as the anti-precedent and points at platform_wallet_preview_identity_registration_keys as the correct precedent: one FFI entry point that owns the per-purpose policy table, validates internally, and returns the minimum payload Swift needs to write to Keychain. Add a platform_wallet_derive_dashpay_keypair (or extend the existing identity-registration preview to include DashPay-purpose slots) so the cross-check happens on the Rust side and the WIF never crosses the FFI. With this toggle defaulting on, every wallet-backed registration amplifies the surface where raw secrets sit in Swift heap.
suggestion: [CARRIED-FORWARD — STILL VALID] DashPay encryption/decryption keys derived via the IDENTITY_AUTHENTICATION DIP-9 sub-feature path
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (line 1421)
Re-verified at head ea4c16a. deriveIdentityAuthKeyAtSlot(identityIndex:keyId:network:) resolves on the Rust side via dash_sdk_derive_identity_key_at_slot_with_resolver → derive_ecdsa_identity_auth_keypair_from_master → identity_auth_derivation_path_for_type, whose base is hardcoded to IDENTITY_AUTHENTICATION_PATH_{MAINNET|TESTNET} (DIP-9 m/9'/<coin>'/5'/0'/...). makeDashpayKeyPair calls it for both .encryption (kid firstKeyId) and .decryption (kid firstKeyId + 1). Two consequences: (1) the 32-byte scalar is a pure function of (identity_index, key_index) with no purpose component — any future authentication-key derivation at the same (identity_index, key_id) produces the same private key, and ECDH + ECDSA sharing a scalar is textbook key reuse across cryptosystems. The nextKeyId = max + 1 allocator is the only invariant preventing collision today. The new validatePrivateKeyForPublicKey check at lines 1435-1446 verifies internal pub↔priv consistency, not that the base path was right. (2) A DIP-9-conformant restore tool looking under the encryption sub-feature path will silently miss these keys. Fix on the Rust side: extend platform-wallet with an encryption-purpose keypair derivation under the correct DIP-9 sub-feature, expose it through rs-platform-wallet-ffi, call it here.
suggestion: [CARRIED-FORWARD — STILL VALID] Pre-registration DashPay key writes leave Keychain orphans with empty identityId on failed submit
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (line 1410)
Re-verified at head ea4c16a. makeDashpayKeyPair writes the two DashPay private bytes to Keychain with IdentityPrivateKeyMetadata.identityId = "" (the explicit identityIdPlaceholder at line 1410) and relies on the post-on-chain persister callback to overwrite the field. If registration aborts between this write and a successful landing (network failure, app kill, asset-lock rejection, user force-close), the two 32-byte secrets stay in Keychain at the namespaced account with empty identityId and no anchoring PersistentPublicKey row. The new deleteAllIdentityPrivateKeys(forIdentityIdBase58:) cleanup added in this delta keys on metadata.identityId, so these placeholder rows are NOT swept by the per-identity cleanup path — they are also invisible to the orphan-scan in the health sheet (which iterates PersistentPublicKey, not Keychain). Defer the Keychain write until registration succeeds, or sweep the pre-derived entries from the submit-error path.
suggestion: [CARRIED-FORWARD — STILL VALID] DashPay keys' ContractBounds.singleContractDocumentType is dropped on persist round-trip
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 1369)
Re-verified at head ea4c16a. makeDashpayKeyPair registers both DashPay keys with ContractBounds.singleContractDocumentType(id: dashpayContractId, documentTypeName: "contactRequest") (CreateIdentityView.swift:1405-1408). PersistentPublicKey serializes contract bounds as a bare [contractId] list and reconstructs with .singleContract(id:) — the documentTypeName variant is dropped entirely. After one persistence round-trip the local IdentityPublicKey projection no longer matches the key actually registered on-chain; any local DPP code reading identity.identityPublicKeys sees a weaker, semantically different bound than what this PR introduces. Carry the contract-bounds payload through the persister callback and extend PersistentPublicKey to preserve the documentTypeName variant.
suggestion: [CARRIED-FORWARD — STILL VALID] is_dpns_name_available correctness fix has no automated test
packages/rs-sdk/src/platform/dpns_usernames/mod.rs (line 406)
Re-verified at head ea4c16a. The semantic change from documents.is_empty() to documents.values().all(|d| d.is_none()) is the central Rust correctness fix in this PR and drives user-visible DPNS registration UX, but it is exercised only manually via the iOS app. packages/rs-sdk/tests/dpns_usernames.rs and tests/dpns_unit_tests.rs exist (commit 2c9e9af relocated DPNS tests there) but neither covers is_dpns_name_available. Add: positive (proven-non-existence map → Ok(true)), negative (Some(Document) → Ok(false)), and explicit empty-map cases so a future change to Document::fetch_many's return shape can't regress the semantics silently.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift`:397-418: [CARRIED-FORWARD — partially addressed in latest delta] deleteOrphan still single-saves; will fatal when the orphan owns DPNS / DashPay rows, and the Keychain wipe runs FIRST so the user is stranded
The latest delta added `KeychainManager.shared.deleteAllIdentityPrivateKeys(forIdentityIdBase58:)` at line 407 and updated the doc comment, but the SwiftData side still does a single `modelContext.delete(identity)` + `try modelContext.save()` (lines 413-414). The relationship inverse `PersistentDPNSName.identity` is non-optional (PersistentDPNSName.swift:84, with an explicit comment stating SwiftData's non-optional contract is honored), and the same applies to `PersistentDashpayProfile.identity` and `PersistentDashpayContactRequest.owner`. The codebase's own `PlatformWalletPersistenceHandler.deleteWalletData` (PlatformWalletPersistenceHandler.swift:2594-2649) carries a long PHASE 1/2/3 comment explaining that SwiftData fatals with `Cannot remove PersistentX from relationship Y on PersistentZ because an appropriate default value is not configured` whenever a non-optional inverse is nulled in the same save batch, and works around this with per-layer saves (delete dpnsNames + dashpayProfile + contactRequests, save; delete identity, save). `deleteOrphan` did not adopt that pattern. Any orphan identity that still owns even one DPNS name, DashPay profile, or contact request will fatal at line 414. Worse, because the Keychain wipe at line 407 now runs FIRST, the fatal leaves the user with the private-key bytes already gone AND the SwiftData identity row still in place — the diagnostic sheet has effectively destroyed the user's keys without finishing the cleanup. An orphan reached by re-importing a different mnemonic is precisely the case this sheet exists to repair, and is plausibly the case where leftover DPNS labels or a DashPay profile from the previous wallet are still present. Apply the same per-layer-then-save sequencing as `deleteWalletData`, and move the Keychain wipe AFTER the SwiftData saves succeed so a mid-flow failure doesn't strand the user without keys.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:804-827: [NEW in latest delta] deleteIdentityPrivateKey(walletId:derivationPath:) legacy-account branch is not gated on walletId metadata — can clobber a sibling wallet's legacy key
The new `(walletId, derivationPath)` overload deletes both `identity_privkey.<walletIdHex>.<derivationPath>` AND `identity_privkey.<derivationPath>` (legacy), keyed only on the account name for the legacy entry — no `kSecAttrGeneric` metadata.walletId check. Compare with `deleteAllIdentityPrivateKeys(forIdentityIdBase58:)` at line 877 and `deleteAllIdentityPrivateKeys(forWalletId:)` which gate on metadata. The legacy `identity_privkey.<path>` account is precisely the cross-wallet-colliding name this PR is retiring (PR description: two wallets at identity_index = 0 keep distinct keys in Keychain). If wallet A still has a partially-migrated legacy entry at path X, and any caller invokes `deleteIdentityPrivateKey(walletId: B, derivationPath: X)`, wallet A's legacy bytes are silently deleted. No in-tree callers today (verified), so the defect is latent — but the function is `public`, the comment positions it as the symmetric counterpart to `storeIdentityPrivateKey`, and the obvious next step (per-key cleanup outside the wholesale `forWalletId:` sweep) goes through here. Gate the legacy delete on `metadata.walletId == thisWallet` (matching `cleanupLegacyKeychainEntry`'s safety property), or drop the legacy branch and require legacy cleanup to go through the metadata-gated helpers.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:862-881: [NEW in latest delta] deleteAllIdentityPrivateKeys(forIdentityIdBase58:) silently skips items with undecodable metadata, leaving orphan private bytes behind
The new helper iterates every `identity_privkey.*` keychain item, decodes the `kSecAttrGeneric` metadata blob with `try?`, and skips items whose decode fails (lines 869-875). It is invoked by the sheet's `Delete this identity` button (the fix for the prior `deleteOrphan` finding). Any keychain row whose `IdentityPrivateKeyMetadata` JSON cannot be decoded — corruption, a partial/aborted write (the pre-registration DashPay-keys path explicitly writes `identityId: ""` placeholders), or a future schema rename — is silently dropped from the scan. The 32-byte private bytes remain in Keychain after the user is told the orphan was cleaned up. The most likely producers of malformed metadata are exactly the failure modes this PR is repairing, so at minimum log undecodable rows so the developer/user is aware, or treat metadata-decode-failure on an `identity_privkey.*` row as itself an orphan signal (no matching `PersistentPublicKey.pkid == account` anywhere) and remove it. The `try?` swallows both signals today.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:1394-1485: [CARRIED-FORWARD — STILL VALID] Default DashPay key path materializes raw private keys + WIFs across the FFI, violating the Swift SDK rule
Re-verified at head ea4c16a08. `makeDashpayKeyPair` (CreateIdentityView.swift:1394-1485) still loops `(encryption, decryption)` and calls `managedWallet.deriveIdentityAuthKeyAtSlot(...)` per slot, copying `preview.privateKeyData` (32 raw scalar bytes) plus `preview.privateKeyWif` into ARC-managed Swift values, then Swift orchestrates the validate-then-Keychain-write pipeline. The latest delta added a `KeyValidation.validatePrivateKeyForPublicKey` cross-check on the hex'd privkey + pubkey (lines 1435-1446) — sensible defense against marshalling drift, but it cements the FFI contract that delivers raw secrets into Swift for orchestration. The Swift SDK's `CLAUDE.md` names this exact shape ("orchestrating multi-step derivation pipelines in Swift (mnemonic → seed → path → key → store)") as the anti-precedent and points at `platform_wallet_preview_identity_registration_keys` as the correct precedent: one FFI entry point that owns the per-purpose policy table, validates internally, and returns the minimum payload Swift needs to write to Keychain. Add a `platform_wallet_derive_dashpay_keypair` (or extend the existing identity-registration preview to include DashPay-purpose slots) so the cross-check happens on the Rust side and the WIF never crosses the FFI. With this toggle defaulting on, every wallet-backed registration amplifies the surface where raw secrets sit in Swift heap.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:1421-1426: [CARRIED-FORWARD — STILL VALID] DashPay encryption/decryption keys derived via the IDENTITY_AUTHENTICATION DIP-9 sub-feature path
Re-verified at head ea4c16a08. `deriveIdentityAuthKeyAtSlot(identityIndex:keyId:network:)` resolves on the Rust side via `dash_sdk_derive_identity_key_at_slot_with_resolver` → `derive_ecdsa_identity_auth_keypair_from_master` → `identity_auth_derivation_path_for_type`, whose base is hardcoded to `IDENTITY_AUTHENTICATION_PATH_{MAINNET|TESTNET}` (DIP-9 `m/9'/<coin>'/5'/0'/...`). `makeDashpayKeyPair` calls it for both `.encryption` (kid `firstKeyId`) and `.decryption` (kid `firstKeyId + 1`). Two consequences: (1) the 32-byte scalar is a pure function of `(identity_index, key_index)` with no purpose component — any future authentication-key derivation at the same `(identity_index, key_id)` produces the same private key, and ECDH + ECDSA sharing a scalar is textbook key reuse across cryptosystems. The `nextKeyId = max + 1` allocator is the only invariant preventing collision today. The new `validatePrivateKeyForPublicKey` check at lines 1435-1446 verifies internal pub↔priv consistency, not that the base path was right. (2) A DIP-9-conformant restore tool looking under the encryption sub-feature path will silently miss these keys. Fix on the Rust side: extend `platform-wallet` with an encryption-purpose keypair derivation under the correct DIP-9 sub-feature, expose it through `rs-platform-wallet-ffi`, call it here.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:1410-1472: [CARRIED-FORWARD — STILL VALID] Pre-registration DashPay key writes leave Keychain orphans with empty identityId on failed submit
Re-verified at head ea4c16a08. `makeDashpayKeyPair` writes the two DashPay private bytes to Keychain with `IdentityPrivateKeyMetadata.identityId = ""` (the explicit `identityIdPlaceholder` at line 1410) and relies on the post-on-chain persister callback to overwrite the field. If registration aborts between this write and a successful landing (network failure, app kill, asset-lock rejection, user force-close), the two 32-byte secrets stay in Keychain at the namespaced account with empty `identityId` and no anchoring `PersistentPublicKey` row. The new `deleteAllIdentityPrivateKeys(forIdentityIdBase58:)` cleanup added in this delta keys on `metadata.identityId`, so these placeholder rows are NOT swept by the per-identity cleanup path — they are also invisible to the orphan-scan in the health sheet (which iterates `PersistentPublicKey`, not Keychain). Defer the Keychain write until registration succeeds, or sweep the pre-derived entries from the submit-error path.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:1369-1409: [CARRIED-FORWARD — STILL VALID] DashPay keys' ContractBounds.singleContractDocumentType is dropped on persist round-trip
Re-verified at head ea4c16a08. `makeDashpayKeyPair` registers both DashPay keys with `ContractBounds.singleContractDocumentType(id: dashpayContractId, documentTypeName: "contactRequest")` (CreateIdentityView.swift:1405-1408). `PersistentPublicKey` serializes contract bounds as a bare `[contractId]` list and reconstructs with `.singleContract(id:)` — the `documentTypeName` variant is dropped entirely. After one persistence round-trip the local `IdentityPublicKey` projection no longer matches the key actually registered on-chain; any local DPP code reading `identity.identityPublicKeys` sees a weaker, semantically different bound than what this PR introduces. Carry the contract-bounds payload through the persister callback and extend `PersistentPublicKey` to preserve the `documentTypeName` variant.
- [SUGGESTION] In `packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:406-413: [CARRIED-FORWARD — STILL VALID] is_dpns_name_available correctness fix has no automated test
Re-verified at head ea4c16a08. The semantic change from `documents.is_empty()` to `documents.values().all(|d| d.is_none())` is the central Rust correctness fix in this PR and drives user-visible DPNS registration UX, but it is exercised only manually via the iOS app. `packages/rs-sdk/tests/dpns_usernames.rs` and `tests/dpns_unit_tests.rs` exist (commit 2c9e9af692 relocated DPNS tests there) but neither covers `is_dpns_name_available`. Add: positive (proven-non-existence map → `Ok(true)`), negative (`Some(Document)` → `Ok(false)`), and explicit empty-map cases so a future change to `Document::fetch_many`'s return shape can't regress the semantics silently.
deleteAllIdentityPrivateKeys(forIdentityIdBase58:) and the sibling forWalletId sweep used `try?` to skip rows whose metadata blob failed to decode, leaving orphan private bytes behind without telling anyone. Replaced both with explicit do/catch that surface the skip via os_log under subsystem=dashpay.SwiftDashSDK category=Keychain — still conservatively NOT deleting rows we can't prove we own, but the developer/user can now see them in Console.app instead of inferring silence. Also applied cargo fmt --all to the ContractBounds FFI extension to clear the macOS Tests workflow's `cargo fmt --check --all` gate that failed on commit 7ae00b8. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Latest review was scanned against head
Newly landed in 03f088f27a: 🟡 deleteAllIdentityPrivateKeys silently skips undecodable-metadata rows — both Also clears a Carry-forward items deferred as previously discussed: raw private keys + WIFs across FFI, DashPay keys via IDENTITY_AUTHENTICATION DIP-9 path, pre-registration empty-identityId placeholder cleanup, |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at head b83f935. The latest delta (single commit: ea4c16a..b83f935) reshapes WalletKeyHealthSheet.deleteOrphan into a four-phase delete (Keychain wipe → PHASE 1 delete DPNS/profile/contact-request children + save → PHASE 2 delete identity + save) that mirrors PlatformWalletPersistenceHandler.deleteWalletData. This FIXES the prior blocking concern about SwiftData fataling on non-Optional inverses during cascade. No new defects introduced by the latest delta. Of 8 prior findings: 1 FIXED (deleteOrphan cascade-fatal), 7 STILL VALID and carried forward unchanged because the latest delta touched only one file. Two of the carried-forward findings sit on the DashPay key-derivation FFI (cross-cryptosystem key reuse via the IDENTITY_AUTHENTICATION DIP-9 path, and pre-registration Keychain writes that can strand secrets with empty identityId on a failed submit); they are the most important remaining items. All findings are suggestions or nitpicks — no in-scope blockers remain after the four-phase fix.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3765 b83f935
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 597, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._
Reviewed commit: b83f935
🟡 6 suggestion(s) | 💬 1 nitpick(s)
Verified findings
suggestion: [CARRIED-FORWARD — STILL VALID] DashPay encryption/decryption keys derived via the IDENTITY_AUTHENTICATION DIP-9 sub-feature path
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (line 1421)
Verified at head b83f935 — unchanged in the latest delta. makeDashpayKeyPair calls managedWallet.deriveIdentityAuthKeyAtSlot(...) for both .encryption (kid firstKeyId) and .decryption (kid firstKeyId + 1). That helper resolves on the Rust side via dash_sdk_derive_identity_key_at_slot_with_resolver → derive_ecdsa_identity_auth_keypair_from_master → identity_auth_derivation_path_for_type, whose base is hardcoded to IDENTITY_AUTHENTICATION_PATH_{MAINNET|TESTNET} (DIP-9 m/9'/<coin>'/5'/0'/...).
Two consequences: (1) the 32-byte scalar is a pure function of (identity_index, key_index) with no purpose component — any future authentication-key derivation at the same (identity_index, key_id) produces the same private scalar; ECDH (DashPay) + ECDSA (auth signing) sharing a scalar is key reuse across cryptosystems. The nextKeyId = max + 1 allocator is the only invariant preventing collision today, and the new validatePrivateKeyForPublicKey cross-check at lines 1435-1446 verifies internal pub↔priv consistency, not that the base path was right. (2) A DIP-9-conformant restore tool looking under the encryption sub-feature path will silently miss these keys — recovery in any other wallet will not find the DashPay secrets.
Fix on the Rust side per packages/swift-sdk/CLAUDE.md: extend platform-wallet with an encryption-purpose keypair derivation under the correct DIP-9 sub-feature, expose it through rs-platform-wallet-ffi, then call it here.
suggestion: [CARRIED-FORWARD — STILL VALID] DashPay key registration orchestrates raw private keys + WIFs in Swift, violating the Swift SDK boundary rule
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (line 1394)
Verified at head b83f935 — unchanged. makeDashpayKeyPair loops (encryption, decryption), calls managedWallet.deriveIdentityAuthKeyAtSlot(...) per slot, copies preview.privateKeyData (32 raw scalar bytes) plus preview.privateKeyWif into ARC-managed Swift values, then Swift orchestrates the validate→Keychain-write→FFI-payload pipeline. The added KeyValidation.validatePrivateKeyForPublicKey cross-check (lines 1435-1446) is sensible defense against marshalling drift but cements the FFI contract that delivers raw secrets into Swift for orchestration.
packages/swift-sdk/CLAUDE.md names this exact shape ("orchestrating multi-step derivation pipelines in Swift (mnemonic → seed → path → key → store)") as the anti-precedent, and points at platform_wallet_preview_identity_registration_keys as the correct precedent: one FFI entry point that owns the per-purpose policy table, validates internally, and returns the minimum payload Swift needs to write to Keychain. Add a platform_wallet_derive_dashpay_keypair (or extend the existing identity-registration preview to include DashPay-purpose slots) so the cross-check happens on the Rust side and the WIF never crosses the FFI. With DashPay enabled by default, every wallet-backed registration amplifies the surface where raw secrets sit in the Swift heap.
suggestion: [CARRIED-FORWARD — STILL VALID] Pre-registration DashPay Keychain writes leave orphans with empty identityId on failed submit
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (line 1410)
Verified at head b83f935 — unchanged. makeDashpayKeyPair writes both DashPay private bytes to Keychain with IdentityPrivateKeyMetadata.identityId = "" (the explicit identityIdPlaceholder at line 1410) and relies on the post-on-chain persister callback to overwrite that field. If registration aborts between the Keychain write and a successful landing (network failure, app kill, asset-lock rejection, force-close), the two 32-byte secrets stay in Keychain at the namespaced account with empty identityId and no anchoring PersistentPublicKey row.
Interaction with finding #4 below: deleteAllIdentityPrivateKeys(forIdentityIdBase58:) keys on metadata.identityId, so these placeholder rows will NOT be swept by the per-identity cleanup path; they are also invisible to WalletKeyHealthSheet's orphan scan (which iterates PersistentPublicKey, not Keychain). Only the wallet-wide deleteAllIdentityPrivateKeys(forWalletId:) reaches them. Defer the Keychain write until registration succeeds, or scrub by walletId in the submit-error path.
suggestion: [CARRIED-FORWARD — STILL VALID] deleteAllIdentityPrivateKeys(forIdentityIdBase58:) silently skips items with undecodable metadata, leaving orphan secret bytes behind
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (line 862)
Verified at head b83f935 — unchanged. The helper iterates every identity_privkey.* keychain item, decodes the kSecAttrGeneric metadata blob with try? (lines 869-873), and skips items whose decode fails. It is invoked from WalletKeyHealthChecker.deleteOrphan as PHASE 0 of the new four-phase delete and presented to the user as a definitive cleanup.
Any keychain row whose IdentityPrivateKeyMetadata JSON cannot be decoded — corruption, a partial/aborted write (the pre-registration DashPay-keys path explicitly writes identityId: "" placeholders, see finding above), or a future schema rename — is silently dropped from the scan, and its 32-byte private bytes remain in Keychain after the user is told the orphan was cleaned up. The most likely producers of malformed metadata are exactly the failure modes this PR repairs. At minimum log undecodable rows; better, treat metadata-decode-failure on an identity_privkey.* row as itself an orphan signal (no PersistentPublicKey.pkid == account anywhere in SwiftData) and remove it.
suggestion: [CARRIED-FORWARD — STILL VALID] deleteIdentityPrivateKey(walletId:derivationPath:) legacy-account branch is not gated on walletId metadata — can clobber a sibling wallet's legacy key
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (line 804)
Verified at head b83f935 — unchanged. The (walletId, derivationPath) overload deletes both identity_privkey.<walletIdHex>.<derivationPath> AND identity_privkey.<derivationPath> (legacy) keyed only on account name for the legacy entry — no kSecAttrGeneric metadata.walletId check. Compare with deleteAllIdentityPrivateKeys(forIdentityIdBase58:) (line 842+) and deleteAllIdentityPrivateKeys(forWalletId:) (line 886+), which both gate on the decoded metadata.
The legacy identity_privkey.<path> account is precisely the cross-wallet-colliding name this PR retires (PR description: two wallets at identity_index = 0 keep distinct keys in Keychain). If wallet A still holds a partially-migrated legacy entry at path X, and any caller invokes deleteIdentityPrivateKey(walletId: B, derivationPath: X), wallet A's legacy bytes are silently deleted. No in-tree callers today (the defect is latent), but the function is public and positioned as the symmetric counterpart to storeIdentityPrivateKey — the obvious next step (per-key cleanup outside the wholesale forWalletId: sweep) goes through here. Gate the legacy delete on metadata.walletId == walletIdHex, or drop the legacy branch and require legacy cleanup to go through the metadata-gated helpers.
suggestion: [CARRIED-FORWARD — STILL VALID] DashPay keys' ContractBounds.singleContractDocumentType is dropped on persist round-trip
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift (line 107)
Verified at head b83f935 — unchanged. makeDashpayKeyPair registers both DashPay keys with ContractBounds.singleContractDocumentType(id: dashpayContractId, documentTypeName: "contactRequest") (CreateIdentityView.swift:1405-1408). PersistentPublicKey serializes contract bounds as a bare [contractId] JSON list (line 53, line 138 reads publicKey.contractBounds!.contractId) and reconstructs with .singleContract(id: $0) in toIdentityPublicKey() at line 120 — the documentTypeName variant is dropped entirely.
After one persistence round-trip the local IdentityPublicKey projection no longer matches the key actually registered on-chain; any local DPP code reading identity.identityPublicKeys sees a weaker, semantically different bound than what this PR introduces. Carry the contract-bounds payload through the persister callback and extend PersistentPublicKey to preserve the documentTypeName variant.
nitpick: [CARRIED-FORWARD — STILL VALID] is_dpns_name_available correctness fix has no automated test
packages/rs-sdk/src/platform/dpns_usernames/mod.rs (line 406)
Verified at head b83f935 — unchanged. The semantic change from documents.is_empty() to documents.values().all(|d| d.is_none()) is the central Rust correctness fix in this PR and drives user-visible DPNS registration UX through the FFI, but it is exercised only manually via the iOS app. packages/rs-sdk/tests/dpns_usernames.rs and tests/dpns_unit_tests.rs exist (commit 2c9e9af relocated DPNS tests there) but neither covers is_dpns_name_available — grep confirms only src/platform/dpns_usernames/mod.rs and queries.rs reference the symbol.
Add three cases: positive (proven-non-existence map → Ok(true)), negative (Some(Document) → Ok(false)), and empty-map (defines the semantics explicitly) so a future change to Document::fetch_many's return shape can't regress the semantics silently.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:1421-1426: [CARRIED-FORWARD — STILL VALID] DashPay encryption/decryption keys derived via the IDENTITY_AUTHENTICATION DIP-9 sub-feature path
Verified at head b83f9352 — unchanged in the latest delta. `makeDashpayKeyPair` calls `managedWallet.deriveIdentityAuthKeyAtSlot(...)` for both `.encryption` (kid `firstKeyId`) and `.decryption` (kid `firstKeyId + 1`). That helper resolves on the Rust side via `dash_sdk_derive_identity_key_at_slot_with_resolver` → `derive_ecdsa_identity_auth_keypair_from_master` → `identity_auth_derivation_path_for_type`, whose base is hardcoded to `IDENTITY_AUTHENTICATION_PATH_{MAINNET|TESTNET}` (DIP-9 `m/9'/<coin>'/5'/0'/...`).
Two consequences: (1) the 32-byte scalar is a pure function of `(identity_index, key_index)` with no purpose component — any future authentication-key derivation at the same `(identity_index, key_id)` produces the same private scalar; ECDH (DashPay) + ECDSA (auth signing) sharing a scalar is key reuse across cryptosystems. The `nextKeyId = max + 1` allocator is the only invariant preventing collision today, and the new `validatePrivateKeyForPublicKey` cross-check at lines 1435-1446 verifies internal pub↔priv consistency, not that the base path was right. (2) A DIP-9-conformant restore tool looking under the encryption sub-feature path will silently miss these keys — recovery in any other wallet will not find the DashPay secrets.
Fix on the Rust side per `packages/swift-sdk/CLAUDE.md`: extend `platform-wallet` with an encryption-purpose keypair derivation under the correct DIP-9 sub-feature, expose it through `rs-platform-wallet-ffi`, then call it here.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:1394-1485: [CARRIED-FORWARD — STILL VALID] DashPay key registration orchestrates raw private keys + WIFs in Swift, violating the Swift SDK boundary rule
Verified at head b83f9352 — unchanged. `makeDashpayKeyPair` loops `(encryption, decryption)`, calls `managedWallet.deriveIdentityAuthKeyAtSlot(...)` per slot, copies `preview.privateKeyData` (32 raw scalar bytes) plus `preview.privateKeyWif` into ARC-managed Swift values, then Swift orchestrates the validate→Keychain-write→FFI-payload pipeline. The added `KeyValidation.validatePrivateKeyForPublicKey` cross-check (lines 1435-1446) is sensible defense against marshalling drift but cements the FFI contract that delivers raw secrets into Swift for orchestration.
`packages/swift-sdk/CLAUDE.md` names this exact shape ("orchestrating multi-step derivation pipelines in Swift (mnemonic → seed → path → key → store)") as the anti-precedent, and points at `platform_wallet_preview_identity_registration_keys` as the correct precedent: one FFI entry point that owns the per-purpose policy table, validates internally, and returns the minimum payload Swift needs to write to Keychain. Add a `platform_wallet_derive_dashpay_keypair` (or extend the existing identity-registration preview to include DashPay-purpose slots) so the cross-check happens on the Rust side and the WIF never crosses the FFI. With DashPay enabled by default, every wallet-backed registration amplifies the surface where raw secrets sit in the Swift heap.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:1410-1472: [CARRIED-FORWARD — STILL VALID] Pre-registration DashPay Keychain writes leave orphans with empty identityId on failed submit
Verified at head b83f9352 — unchanged. `makeDashpayKeyPair` writes both DashPay private bytes to Keychain with `IdentityPrivateKeyMetadata.identityId = ""` (the explicit `identityIdPlaceholder` at line 1410) and relies on the post-on-chain persister callback to overwrite that field. If registration aborts between the Keychain write and a successful landing (network failure, app kill, asset-lock rejection, force-close), the two 32-byte secrets stay in Keychain at the namespaced account with empty `identityId` and no anchoring `PersistentPublicKey` row.
Interaction with finding #4 below: `deleteAllIdentityPrivateKeys(forIdentityIdBase58:)` keys on `metadata.identityId`, so these placeholder rows will NOT be swept by the per-identity cleanup path; they are also invisible to `WalletKeyHealthSheet`'s orphan scan (which iterates `PersistentPublicKey`, not Keychain). Only the wallet-wide `deleteAllIdentityPrivateKeys(forWalletId:)` reaches them. Defer the Keychain write until registration succeeds, or scrub by walletId in the submit-error path.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:862-881: [CARRIED-FORWARD — STILL VALID] deleteAllIdentityPrivateKeys(forIdentityIdBase58:) silently skips items with undecodable metadata, leaving orphan secret bytes behind
Verified at head b83f9352 — unchanged. The helper iterates every `identity_privkey.*` keychain item, decodes the `kSecAttrGeneric` metadata blob with `try?` (lines 869-873), and skips items whose decode fails. It is invoked from `WalletKeyHealthChecker.deleteOrphan` as PHASE 0 of the new four-phase delete and presented to the user as a definitive cleanup.
Any keychain row whose `IdentityPrivateKeyMetadata` JSON cannot be decoded — corruption, a partial/aborted write (the pre-registration DashPay-keys path explicitly writes `identityId: ""` placeholders, see finding above), or a future schema rename — is silently dropped from the scan, and its 32-byte private bytes remain in Keychain after the user is told the orphan was cleaned up. The most likely producers of malformed metadata are exactly the failure modes this PR repairs. At minimum log undecodable rows; better, treat metadata-decode-failure on an `identity_privkey.*` row as itself an orphan signal (no `PersistentPublicKey.pkid == account` anywhere in SwiftData) and remove it.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:804-827: [CARRIED-FORWARD — STILL VALID] deleteIdentityPrivateKey(walletId:derivationPath:) legacy-account branch is not gated on walletId metadata — can clobber a sibling wallet's legacy key
Verified at head b83f9352 — unchanged. The `(walletId, derivationPath)` overload deletes both `identity_privkey.<walletIdHex>.<derivationPath>` AND `identity_privkey.<derivationPath>` (legacy) keyed only on account name for the legacy entry — no `kSecAttrGeneric` metadata.walletId check. Compare with `deleteAllIdentityPrivateKeys(forIdentityIdBase58:)` (line 842+) and `deleteAllIdentityPrivateKeys(forWalletId:)` (line 886+), which both gate on the decoded metadata.
The legacy `identity_privkey.<path>` account is precisely the cross-wallet-colliding name this PR retires (PR description: two wallets at `identity_index = 0` keep distinct keys in Keychain). If wallet A still holds a partially-migrated legacy entry at path X, and any caller invokes `deleteIdentityPrivateKey(walletId: B, derivationPath: X)`, wallet A's legacy bytes are silently deleted. No in-tree callers today (the defect is latent), but the function is `public` and positioned as the symmetric counterpart to `storeIdentityPrivateKey` — the obvious next step (per-key cleanup outside the wholesale `forWalletId:` sweep) goes through here. Gate the legacy delete on `metadata.walletId == walletIdHex`, or drop the legacy branch and require legacy cleanup to go through the metadata-gated helpers.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift`:107-140: [CARRIED-FORWARD — STILL VALID] DashPay keys' ContractBounds.singleContractDocumentType is dropped on persist round-trip
Verified at head b83f9352 — unchanged. `makeDashpayKeyPair` registers both DashPay keys with `ContractBounds.singleContractDocumentType(id: dashpayContractId, documentTypeName: "contactRequest")` (CreateIdentityView.swift:1405-1408). `PersistentPublicKey` serializes contract bounds as a bare `[contractId]` JSON list (line 53, line 138 reads `publicKey.contractBounds!.contractId`) and reconstructs with `.singleContract(id: $0)` in `toIdentityPublicKey()` at line 120 — the `documentTypeName` variant is dropped entirely.
After one persistence round-trip the local `IdentityPublicKey` projection no longer matches the key actually registered on-chain; any local DPP code reading `identity.identityPublicKeys` sees a weaker, semantically different bound than what this PR introduces. Carry the contract-bounds payload through the persister callback and extend `PersistentPublicKey` to preserve the `documentTypeName` variant.
- [NITPICK] In `packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:406-413: [CARRIED-FORWARD — STILL VALID] is_dpns_name_available correctness fix has no automated test
Verified at head b83f9352 — unchanged. The semantic change from `documents.is_empty()` to `documents.values().all(|d| d.is_none())` is the central Rust correctness fix in this PR and drives user-visible DPNS registration UX through the FFI, but it is exercised only manually via the iOS app. `packages/rs-sdk/tests/dpns_usernames.rs` and `tests/dpns_unit_tests.rs` exist (commit 2c9e9af6 relocated DPNS tests there) but neither covers `is_dpns_name_available` — grep confirms only `src/platform/dpns_usernames/mod.rs` and `queries.rs` reference the symbol.
Add three cases: positive (proven-non-existence map → `Ok(true)`), negative (`Some(Document)` → `Ok(false)`), and empty-map (defines the semantics explicitly) so a future change to `Document::fetch_many`'s return shape can't regress the semantics silently.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift (1)
414-416:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle partial-success orphan deletion without leaving stale UI state.
At Line 414,
deleteOrphanthrowskeychainErroreven after SwiftData deletion succeeds. Then at Line 667, the caller treats it as a full failure and keeps the deleted identity inreports, leaving stale UI/actions.Suggested localized fix
private func deleteIdentity(_ report: WalletIdentityKeyHealthReport) { do { try WalletKeyHealthChecker.deleteOrphan( identity: report.identityRow, modelContext: modelContext ) actionMessage = "Deleted orphan identity \(report.identityIdBase58.prefix(12))…" errorMessage = nil // Drop the now-deleted row from the report list so the // sheet doesn't try to render it again. reports.removeAll { $0.id == report.id } } catch { - errorMessage = "Delete failed: \(error.localizedDescription)" + if report.identityRow.modelContext == nil { + reports.removeAll { $0.id == report.id } + actionMessage = "Deleted orphan identity \(report.identityIdBase58.prefix(12))…" + errorMessage = "Keychain cleanup warning: \(error.localizedDescription)" + } else { + errorMessage = "Delete failed: \(error.localizedDescription)" + } } }Also applies to: 656-669
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift` around lines 414 - 416, The deleteOrphan flow currently throws keychainError even when the SwiftData deletion succeeded, causing the caller (which updates the reports array) to treat the whole operation as a failure and leave stale UI state; change deleteOrphan so that if SwiftData deletion succeeded but keychain deletion failed it does not throw away the success—either return a result/flag or clear the orphan from the in-memory reports and surface the keychain error separately (e.g., return .partialSuccess(with: keychainError) or set keychainError on a Result and not throw), update the caller to remove the deleted identity from reports when SwiftData deletion succeeded and then present a non-blocking error message for the keychainError instead of preventing report removal; reference: deleteOrphan, keychainError, and the reports collection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:
- Around line 514-526: The branch mapping
ContractBounds::SingleContractDocumentType must not emit contract_bounds_kind =
2 while providing a null contract_bounds_document_type; change the match arm in
the code handling entry.public_key.contract_bounds() so that you attempt
CString::new(document_type_name.as_str()) and only set contract_bounds_kind = 2
and contract_bounds_document_type to the CString pointer on Ok(c). On Err(_)
fall back to treating it as the SingleContract case (set contract_bounds_kind =
1, contract_bounds_id = id.to_buffer(), and contract_bounds_document_type =
ptr::null()) so the discriminant and payload remain consistent; reference the
ContractBounds::SingleContractDocumentType match arm, CString::new, and the
contract_bounds_kind/contract_bounds_document_type variables when making the
change.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift`:
- Around line 29-35: The contractBoundsDocumentTypeName field can become stale
because the contractBounds setter only updates contractBoundsData; update the
contractBounds setter (the property/method named contractBounds) to also set
contractBoundsDocumentTypeName: when the new value is
.singleContractDocumentType(...) assign the document type name into
contractBoundsDocumentTypeName and set contractBoundsData accordingly, and when
the new value is .singleContract(...) or .unbounded set
contractBoundsDocumentTypeName to nil (and update contractBoundsData as before)
so toIdentityPublicKey() reconstructs the correct variant.
- Around line 143-152: The code that builds ContractBounds from
contractBounds?.first must validate that the Data is exactly 32 bytes before
constructing a ContractBounds variant; update the block in PersistentPublicKey
(where ContractBounds is set using contractBounds and
contractBoundsDocumentTypeName) to check id.count == 32 and only create
.singleContract(id:) or .singleContractDocumentType(id:documentTypeName:) when
that condition holds, otherwise treat the value as malformed (e.g., set bounds =
nil or propagate an error) so invalid-length ids are rejected before any FFI
marshalling.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1350-1371: The persistence code writes contract-bounds into the
new columns but the restore path in buildIdentityRestoreBuffer (and the nearby
logic around lines referenced: 1388-1389, 1410-1416) never marshals those fields
back into the IdentityKeyRestoreFFI, so scoped keys are restored as unbounded;
update buildIdentityRestoreBuffer to read the persisted contractBoundsIds and
contractBoundsDocumentTypeName for a given PersistentPublicKey (the same fields
set where snapshotBoundsIds/snapshotBoundsDocType are computed) and populate the
corresponding contract-bounds field on the IdentityKeyRestoreFFI (building the
same enum/variant shape you used when writing: none, singleContract(id), or
singleContractDocumentType(id,name)) before serializing the restore buffer so
the Rust side receives the restored bounds.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift`:
- Around line 414-416: The deleteOrphan flow currently throws keychainError even
when the SwiftData deletion succeeded, causing the caller (which updates the
reports array) to treat the whole operation as a failure and leave stale UI
state; change deleteOrphan so that if SwiftData deletion succeeded but keychain
deletion failed it does not throw away the success—either return a result/flag
or clear the orphan from the in-memory reports and surface the keychain error
separately (e.g., return .partialSuccess(with: keychainError) or set
keychainError on a Result and not throw), update the caller to remove the
deleted identity from reports when SwiftData deletion succeeded and then present
a non-blocking error message for the keychainError instead of preventing report
removal; reference: deleteOrphan, keychainError, and the reports collection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edb644b5-b2d5-4283-b9fa-ea0b1c7d4b08
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/identity_persistence.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletKeyHealthSheet.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift
|
Reviewed at head
Carry-forwards remain deferred as previously discussed:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (b83f935 -> 03f088f) is focused hardening that resolves three prior findings: legacy-keychain delete is now gated on metadata.walletId via a shared helper, ContractBounds.singleContractDocumentType now round-trips through both the FFI and SwiftData via a new doc-type column, and both wholesale Keychain sweeps now os_log undecodable rows instead of silently skipping. Four prior findings remain STILL VALID against current head: (1-3) the DashPay registration path still derives encryption/decryption keys from the DIP-9 identity-authentication branch in Rust while Swift orchestrates the validation/Keychain/FFI pipeline with raw 32-byte secrets + WIFs in ARC memory, and still pre-writes those secrets to Keychain with empty identityId before registration finalizes (leaving orphans on aborted submit); (7) the central is_dpns_name_available correctness fix still has no rs-sdk test. Two minor new observations from the delta: the per-key legacy helper silently no-ops on malformed metadata (asymmetric with the wholesale-sweep logging the same delta added) and the FFI CString interior-NUL fallback drops both id and doc-type. No new blocking defects.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift:1394-1485: [CARRIED-FORWARD — STILL VALID] DashPay key registration orchestrates raw private keys + WIFs in Swift, violating the Swift SDK boundary rule
Re-verified at HEAD 03f088f27 — unchanged. `makeDashpayKeyPair` loops `(encryption, decryption)`, calls `managedWallet.deriveIdentityAuthKeyAtSlot(...)` per slot, copies `preview.privateKeyData` (32 raw scalar bytes) plus `preview.privateKeyWif` into ARC-managed Swift values, then Swift orchestrates the validate->Keychain-write->FFI-payload pipeline. The `KeyValidation.validatePrivateKeyForPublicKey` cross-check is sensible defense against marshalling drift but cements the FFI contract that delivers raw secrets into Swift for orchestration.
`packages/swift-sdk/CLAUDE.md` names this exact shape ("orchestrating multi-step derivation pipelines in Swift (mnemonic -> seed -> path -> key -> store)") as the anti-precedent, and points at `platform_wallet_preview_identity_registration_keys` as the correct precedent: one FFI entry point that owns the per-purpose policy table, validates internally, and returns the minimum payload Swift needs to write to Keychain. Add a `platform_wallet_derive_dashpay_keypair` (or extend the existing identity-registration preview to include DashPay-purpose slots) so the cross-check happens on the Rust side and the WIF never crosses the FFI. With DashPay enabled by default, every wallet-backed registration amplifies the surface where raw secrets sit in the Swift heap.
In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift:947-980: [CARRIED-FORWARD — partially addressed] Identity-scoped Keychain cleanup logs but still leaves undecodable metadata rows behind
The delta added `Self.log.error(...)` reporting (subsystem `dashpay.SwiftDashSDK`, category `Keychain`) for both the missing-`kSecAttrGeneric`-blob branch and the JSON-decode-failure branch in both `deleteAllIdentityPrivateKeys(forIdentityIdBase58:)` (lines 950-975) and `deleteAllIdentityPrivateKeys(forWalletId:)` — the original "silent skip" half of the finding is now visible to developers in Console.app.
What remains: the bytes still survive the sweep. The most likely producers of malformed metadata are exactly the failure modes this PR creates — `identityId: ""` placeholder rows from the pre-registration DashPay path (finding #3 above), partial/aborted writes, or a future schema rename. The wholesale identity-scoped wipe presents to the user as definitive (`WalletKeyHealthChecker.deleteOrphan` PHASE 0), but undecodable rows remain. The conservative "don't delete data we can't prove we own" stance is reasonable, but for the `forIdentityIdBase58:` path specifically, undecodable `identity_privkey.*` rows that no `PersistentPublicKey.privateKeyKeychainIdentifier` points at anywhere in SwiftData are safe to treat as orphan signals and delete.
Reduced severity to `suggestion` since the developer is no longer blind to the leftovers.
| private func makeDashpayKeyPair( | ||
| managedWallet: ManagedPlatformWallet, | ||
| walletId: Data, | ||
| identityIndex: UInt32, | ||
| firstKeyId: UInt32, | ||
| network: Network | ||
| ) throws -> [ManagedPlatformWallet.IdentityPubkey] { | ||
| let purposes: [(keyId: UInt32, purpose: KeyPurpose)] = [ | ||
| (firstKeyId, .encryption), | ||
| (firstKeyId + 1, .decryption), | ||
| ] | ||
| let bounds: ManagedPlatformWallet.ContractBounds = .singleContractDocumentType( | ||
| id: Self.dashpayContractId, | ||
| documentTypeName: Self.dashpayContactRequestDocumentType | ||
| ) | ||
| let walletIdHex = walletId.toHexString() | ||
| let identityIdPlaceholder = "" // overwritten by persister callback | ||
| // when the identity actually | ||
| // lands on-chain; the metadata | ||
| // we write here only needs to | ||
| // satisfy the keychain | ||
| // round-trip lookup by pubkey | ||
| // hex. | ||
|
|
||
| var rows: [ManagedPlatformWallet.IdentityPubkey] = [] | ||
| rows.reserveCapacity(purposes.count) | ||
|
|
||
| for (keyId, purpose) in purposes { | ||
| let preview = try managedWallet.deriveIdentityAuthKeyAtSlot( | ||
| identityIndex: identityIndex, | ||
| keyId: keyId, | ||
| network: network | ||
| ) | ||
|
|
||
| // Defence against derivation drift / FFI marshalling | ||
| // bugs — mirrors `AddIdentityKeyView.submit`'s cross- | ||
| // check. A mismatched DashPay key lands on Platform as | ||
| // a key the trampoline can't sign with and surfaces as | ||
| // an opaque "encrypted xpub" failure on the first | ||
| // contact-request flow, which is much harder to debug | ||
| // after the fact than failing fast here. | ||
| guard | ||
| KeyValidation.validatePrivateKeyForPublicKey( | ||
| privateKeyHex: preview.privateKeyData.toHexString(), | ||
| publicKeyHex: preview.publicKeyHex, | ||
| keyType: .ecdsaSecp256k1, | ||
| network: network | ||
| ) | ||
| else { | ||
| throw PlatformWalletError.walletOperation( | ||
| "Derived DashPay key (kid \(keyId), purpose \(purpose.name)) didn't match its public key — refusing to persist" | ||
| ) | ||
| } | ||
|
|
||
| let pubKeyHashHex = SwiftDashSDK.KeychainManager.computePublicKeyHashHex( | ||
| preview.publicKeyData | ||
| ) | ||
| let metadata = IdentityPrivateKeyMetadata( | ||
| identityId: identityIdPlaceholder, | ||
| keyId: keyId, | ||
| walletId: walletIdHex, | ||
| identityIndex: identityIndex, | ||
| keyIndex: keyId, | ||
| derivationPath: preview.derivationPath, | ||
| publicKey: preview.publicKeyHex, | ||
| publicKeyHash: pubKeyHashHex, | ||
| keyType: KeyType.ecdsaSecp256k1.rawValue, | ||
| purpose: purpose.rawValue, | ||
| securityLevel: SecurityLevel.medium.rawValue | ||
| ) | ||
| guard KeychainManager.shared.storeIdentityPrivateKey( | ||
| preview.privateKeyData, | ||
| derivationPath: preview.derivationPath, | ||
| metadata: metadata | ||
| ) != nil else { | ||
| throw PlatformWalletError.walletOperation( | ||
| "Could not persist DashPay key (kid \(keyId), purpose \(purpose.name)) to Keychain" | ||
| ) | ||
| } | ||
| rows.append( | ||
| ManagedPlatformWallet.IdentityPubkey( | ||
| keyId: keyId, | ||
| keyType: .ecdsaSecp256k1, | ||
| purpose: purpose, | ||
| securityLevel: .medium, | ||
| pubkeyBytes: preview.publicKeyData, | ||
| contractBounds: bounds | ||
| ) | ||
| ) | ||
| } | ||
| return rows | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [CARRIED-FORWARD — STILL VALID] DashPay key registration orchestrates raw private keys + WIFs in Swift, violating the Swift SDK boundary rule
Re-verified at HEAD 03f088f — unchanged. makeDashpayKeyPair loops (encryption, decryption), calls managedWallet.deriveIdentityAuthKeyAtSlot(...) per slot, copies preview.privateKeyData (32 raw scalar bytes) plus preview.privateKeyWif into ARC-managed Swift values, then Swift orchestrates the validate->Keychain-write->FFI-payload pipeline. The KeyValidation.validatePrivateKeyForPublicKey cross-check is sensible defense against marshalling drift but cements the FFI contract that delivers raw secrets into Swift for orchestration.
packages/swift-sdk/CLAUDE.md names this exact shape ("orchestrating multi-step derivation pipelines in Swift (mnemonic -> seed -> path -> key -> store)") as the anti-precedent, and points at platform_wallet_preview_identity_registration_keys as the correct precedent: one FFI entry point that owns the per-purpose policy table, validates internally, and returns the minimum payload Swift needs to write to Keychain. Add a platform_wallet_derive_dashpay_keypair (or extend the existing identity-registration preview to include DashPay-purpose slots) so the cross-check happens on the Rust side and the WIF never crosses the FFI. With DashPay enabled by default, every wallet-backed registration amplifies the surface where raw secrets sit in the Swift heap.
source: ['claude', 'codex']
| else { | ||
| continue | ||
| } | ||
| // Surface undecodable-metadata rows via os_log rather | ||
| // than silently skipping them. The caller has asked | ||
| // for a wholesale identity-scoped wipe, so the safest | ||
| // action is still to NOT delete a row we can't prove | ||
| // we own — but the developer needs to know that an | ||
| // `identity_privkey.*` row exists whose metadata we | ||
| // can't read (likely a partial / aborted write, a | ||
| // legacy pre-metadata row, or a future schema rename), | ||
| // because the user is being told the orphan was | ||
| // cleaned up. Visible in Console.app under the | ||
| // `dashpay.SwiftDashSDK` subsystem. | ||
| guard let metadataData = item[kSecAttrGeneric as String] as? Data else { | ||
| Self.log.error( | ||
| "Skipping identity_privkey row at account \(account, privacy: .public) — missing kSecAttrGeneric metadata blob; private bytes remain" | ||
| ) | ||
| continue | ||
| } | ||
| let metadata: IdentityPrivateKeyMetadata | ||
| do { | ||
| metadata = try decoder.decode(IdentityPrivateKeyMetadata.self, from: metadataData) | ||
| } catch { | ||
| Self.log.error( | ||
| "Skipping identity_privkey row at account \(account, privacy: .public) — metadata JSON decode failed (\(String(describing: error), privacy: .public)); private bytes remain" | ||
| ) | ||
| continue | ||
| } | ||
| guard metadata.identityId == identityIdBase58 else { | ||
| continue | ||
| } | ||
| try deleteGenericPassword(account: account) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [CARRIED-FORWARD — partially addressed] Identity-scoped Keychain cleanup logs but still leaves undecodable metadata rows behind
The delta added Self.log.error(...) reporting (subsystem dashpay.SwiftDashSDK, category Keychain) for both the missing-kSecAttrGeneric-blob branch and the JSON-decode-failure branch in both deleteAllIdentityPrivateKeys(forIdentityIdBase58:) (lines 950-975) and deleteAllIdentityPrivateKeys(forWalletId:) — the original "silent skip" half of the finding is now visible to developers in Console.app.
What remains: the bytes still survive the sweep. The most likely producers of malformed metadata are exactly the failure modes this PR creates — identityId: "" placeholder rows from the pre-registration DashPay path (finding #3 above), partial/aborted writes, or a future schema rename. The wholesale identity-scoped wipe presents to the user as definitive (WalletKeyHealthChecker.deleteOrphan PHASE 0), but undecodable rows remain. The conservative "don't delete data we can't prove we own" stance is reasonable, but for the forIdentityIdBase58: path specifically, undecodable identity_privkey.* rows that no PersistentPublicKey.privateKeyKeychainIdentifier points at anywhere in SwiftData are safe to treat as orphan signals and delete.
Reduced severity to suggestion since the developer is no longer blind to the leftovers.
source: ['claude', 'codex']
There was a problem hiding this comment.
Partially: stays with the conservative "don't delete data we can't prove we own" stance for now — the os_log surfacing landed in 03f088f so the residue isn't silent anymore. Your suggested refinement (delete undecodable rows in the forIdentityIdBase58: path when no PersistentPublicKey.privateKeyKeychainIdentifier references the account anywhere in SwiftData) is sound but requires the helper to take a ModelContext, which tightens KeychainManager's coupling to the persistence layer. Tracking as a follow-up; the most likely producer (the pre-registration identityId: "" placeholder rows) is itself a carry-forward whose root-cause fix would eliminate the leftover class entirely.
…sub-fixes PR #3765 follow-up review found five issues from the prior round. Addressed in this commit: 1. Rust FFI `IdentityKeyEntryFFI::from_entry` was emitting kind=2 with a null doc-type pointer on `CString::new` failure (interior-NUL — DPP rejects them upstream so unreachable today, but the discriminant + payload were inconsistent). Now demotes to kind=1 (`SingleContract { id }`) so the contract id is preserved and the Swift consumer sees a coherent variant. 2. PersistentPublicKey.contractBounds setter cleared only `contractBoundsData`, leaving the new `contractBoundsDocumentTypeName` column stale across variant changes. Setter now also nils the doc-type column; the persister continues to write it explicitly after the setter, atomically. 3. `toIdentityPublicKey()` could construct a `ContractBounds` from a corrupt short / over-long id; downstream FFI marshalling hard-asserts 32 bytes and would crash on the NEXT call. Added an `id.count == 32` gate; a wrong-length id drops the bounds projection but keeps the key. 4. Persist↔restore round-trip closure. `IdentityKeyRestoreFFI` was missing contract-bounds fields, so cold-restart loaded scoped DashPay keys as unbounded. Extended the Rust struct with `contract_bounds_{kind,id, document_type}` (mirrors the persist-side encoding); `build_identity_public_keys` reconstructs the variant; Swift's `buildIdentityRestoreBuffer` reads from the persisted columns and emits the same trio (CString allocation reuses `cStringBuffers` for release). 5. `WalletKeyHealthChecker.deleteOrphan` was throwing `keychainError` even when SwiftData succeeded, so the caller (`deleteIdentity` in the sheet) treated it as a full failure and kept the deleted row in `reports` — stale UI. Returns `OrphanDeleteOutcome { swiftDataDeleted, keychainError }` instead; caller drops the row on SwiftData success and surfaces the keychain side as a non-blocking warning. Plus a follow-on consistency fix found in the same review: `KeychainManager.deleteLegacyKeychainEntryIfOwnedByWallet` was silently no-op'ing on malformed metadata while the wholesale sweeps log via `os_log`. Added matching log lines so all three "skip, can't verify ownership" branches surface in Console.app uniformly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…act_bounds_document_type Per coderabbit feedback on 3319620994 — `free_identity_key_entry_ffi` is idempotent, but a copied-then-double-freed struct value would still trigger UB. Spell out the supported consumption pattern (Swift binding copies the doc-type into an owned String inside the callback) so the constraint is explicit for future maintainers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta 03f088f→03c54053 is a focused hardening pass that closes the persist↔restore ContractBounds round-trip and resolves both prior NEW findings (per-key legacy keychain helper now logs malformed-metadata skips symmetric to the wholesale sweeps; CString interior-NUL fallback now demotes to SingleContract kind=1 preserving the contract id rather than dropping it). Five prior findings remain STILL VALID at HEAD and are unchanged in this delta: (#1) DashPay encryption/decryption keys derived via the DIP-9 IDENTITY_AUTHENTICATION sub-feature path, (#2) Swift orchestrates raw 32-byte scalars + WIFs across the FFI for DashPay registration, (#3) pre-registration Keychain writes with placeholder identityId leak on aborted submit, (#4) identity-scoped Keychain sweep now logs but still leaves undecodable rows behind (partially addressed → suggestion), (#5) is_dpns_name_available correctness fix has no rs-sdk test. Two new latest-delta observations: PersistentPublicKey.contractBounds setter unconditionally nulls contractBoundsDocumentTypeName (statement-ordering footgun confirmed at PlatformWalletPersistenceHandler.swift:1415-1416), and the new restore-path contract_bounds projection silently demotes scoped keys to 'no bounds' on a wrong-length id rather than refusing the row. No blocking issues; recommending COMMENT.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift:89-107: [NEW in latest delta] `contractBounds` setter unconditionally nulls `contractBoundsDocumentTypeName` — statement-ordering footgun
Confirmed at HEAD: the setter at lines 89-107 always clears `contractBoundsDocumentTypeName` whenever the ids change through it. The doc comment correctly explains why (ids and doc-type are paired), but the contract this creates is asymmetric and fragile:
* `from(_:identityId:)` is fine because it uses the designated initializer (line 70), bypassing the setter.
* `PlatformWalletPersistenceHandler.upsertPublicKeyRow` at lines 1415-1416 writes `row.contractBounds = snapshotBoundsIds` (clears the doc-type) immediately followed by `row.contractBoundsDocumentTypeName = snapshotBoundsDocType` (restores it). This works ONLY because of strict statement ordering. Reordering, or any new caller that sets `contractBoundsDocumentTypeName` first and `contractBounds` second, silently degrades a `.singleContractDocumentType` row to `.singleContract` on the next `toIdentityPublicKey()` call — exactly the DashPay-specific failure mode this PR exists to fix.
Less foot-gunny shapes: (a) make the setter accept `(ids, docType)` atomically (or expose only `from(_:identityId:)` for external writes); (b) drop the side-effect from the setter and have the persister handler null the doc-type explicitly when it wants to.
| if let id = pk.contractBounds?.first, id.count == 32 { | ||
| withUnsafeMutableBytes(of: &row.contract_bounds_id) { dst in | ||
| id.copyBytes(to: dst.bindMemory(to: UInt8.self).baseAddress!, count: 32) | ||
| } | ||
| if let docType = pk.contractBoundsDocumentTypeName, !docType.isEmpty { | ||
| row.contract_bounds_kind = 2 | ||
| row.contract_bounds_document_type = UnsafePointer( | ||
| duplicateCString(docType, allocation: allocation) | ||
| ) | ||
| } else { | ||
| row.contract_bounds_kind = 1 | ||
| row.contract_bounds_document_type = nil | ||
| } | ||
| } else { | ||
| row.contract_bounds_kind = 0 | ||
| row.contract_bounds_document_type = nil | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: [NEW in latest delta] Restore-path silently demotes scoped DashPay keys to 'no bounds' on wrong-length id
The new contract_bounds mirror in the restore-row builder gates on pk.contractBounds?.first?.count == 32 and falls through to row.contract_bounds_kind = 0 (unbounded) when the id is malformed/wrong-length. The Rust receiver in build_identity_public_keys then constructs IdentityPublicKeyV0 with contract_bounds: None. This is internally self-consistent with the persist-side comment ('rather than crashing FFI marshalling on the Rust side'), but has a scope-broadening effect: a DashPay encryption key persisted as .singleContractDocumentType(dashpay, contactRequest) that becomes corrupted on disk would, after cold restart, be presented to the local signer/UI as an unbounded key. The on-chain key still carries the bound (Drive will reject any out-of-scope state transition), so the only attack surface is wasted asset-lock / processing fees on a guaranteed-reject transition — not a consensus break.
Prefer either: (a) propagate the length error to the caller and drop the entire key from the restore set; (b) at minimum os_log the wrong-length-id case via the same dashpay.SwiftDashSDK subsystem. The parallel branch in PersistentPublicKey.toIdentityPublicKey() (line 164-171) swallows the length mismatch silently as well.
source: ['codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #3765 at HEAD 8f65db2. The latest delta (03c5405→8f65db24) is documentation-only: a 10-line ownership/lifetime comment on IdentityKeyEntryFFI::contract_bounds_document_type clarifying the single-owner invariant for Swift consumers. No new defects introduced. Prior findings status: (1) DashPay encryption keys derived via IDENTITY_AUTHENTICATION DIP-9 sub-feature path — STILL VALID; (2) DashPay key registration orchestrates raw private keys + WIFs in Swift — STILL VALID; (3) Pre-registration DashPay Keychain writes leave orphans with empty identityId on failed submit — STILL VALID; (4) Identity-scoped Keychain cleanup logs but still leaves undecodable rows behind — STILL VALID (partially addressed); (5) is_dpns_name_available correctness fix has no automated test — STILL VALID; (6) contractBounds setter unconditionally nulls contractBoundsDocumentTypeName — STILL VALID; (7) Restore-path silently demotes scoped DashPay keys to no-bounds on wrong-length id — STILL VALID; (8) deleteLegacyKeychainEntryIfOwnedByWallet silent skip on malformed metadata — FIXED at 03c5405; (9) CString interior-NUL fallback dropped contract id — FIXED at 03c5405. No new latest-delta findings. Recommending COMMENT.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift:89-107: [CARRIED-FORWARD — STILL VALID] `contractBounds` setter unconditionally nulls `contractBoundsDocumentTypeName` — statement-ordering footgun
Re-verified at HEAD 8f65db24 — unchanged. The setter at lines 89-107 always clears `contractBoundsDocumentTypeName` whenever the ids change through it. The doc comment correctly explains why (ids and doc-type are paired), but the contract this creates is asymmetric and fragile: `from(_:identityId:)` is fine because it uses the designated initializer, bypassing the setter; `PlatformWalletPersistenceHandler.upsertPublicKeyRow` writes `row.contractBounds = snapshotBoundsIds` (clears the doc-type) immediately followed by `row.contractBoundsDocumentTypeName = snapshotBoundsDocType` (restores it). This works ONLY because of strict statement ordering. Reordering, or any new caller that sets `contractBoundsDocumentTypeName` first and `contractBounds` second, silently degrades a `.singleContractDocumentType` row to `.singleContract` on the next `toIdentityPublicKey()` call — exactly the DashPay-specific failure mode this PR exists to fix. Less foot-gunny shapes: (a) make the setter accept `(ids, docType)` atomically (or expose only `from(_:identityId:)` for external writes); (b) drop the side-effect from the setter and have the persister handler null the doc-type explicitly when it wants to.
2 participants
Issue being fixed or feature implemented
Enable the DashPay end-to-end flow on the iOS SwiftExampleApp (search by DPNS name → send contact request → accept → send payment) and fix several blockers discovered while standing it up.
What was done?
rs-sdkis_dpns_name_availablewas treating proven non-existence (a non-emptyIndexMapwithNonevalues) as "name taken". Fixed todocuments.values().all(is_none).swift-sdkSDK.initnow picks a network-awareplatformVersiondefault: PV 11 formainnet/testnet(V0getDocumentswire) and PV 12 fordevnet/regtest. Without this, every Platform document query against testnet failed with"could not decode data contracts query"becausePlatformVersion::latest()emits the V1 wire format.KeychainManager.storeIdentityPrivateKeynamespaces accounts by walletId (identity_privkey.<walletIdHex>.<derivationPath>). The old per-path scheme collided whenever two wallets had an identity at the sameidentity_index.PlatformWalletPersistenceHandler.deleteWalletDatanow runs in four save phases — identity-children (DPNS names, DashPay profile, DashPay contact requests) → identities → accounts → wallet. SwiftData fatals during save() if it has to null out a non-optional inverse on a row in the same batch as the parent's delete; splitting the cascade across saves means each parent's children are already gone from the store by the time the parent is processed. Trades atomicity for keeping every model's inverse non-Optional (the schema stays honest about which references are always present). Costs four saves instead of one for a user-initiated wipe.swift-example-appAddIdentityKeyView: surfaces DashPay as a "(System)" entry in the contract-bounds picker since it's required for DashPay flows but isn't pre-saved locally. Locks KeyType to ECDSA secp256k1 + SecurityLevel to Medium when purpose is Encryption/Decryption (DPP enforces both — seevalidate_identity_public_keys_structure/v0/mod.rs). Forces the document-type binding when the contract only declaresrequiresIdentityEncryptionBoundedKeyat document-type level (DashPay →contactRequest), with auto-fill.CreateIdentityView: new default-on Register DashPay keys toggle that registers 2 extra keys (Encryption + Decryption, MEDIUM, ECDSA secp, bound to DashPay →contactRequest) alongside the default auth keys. Asset-lock minimum auto-scales for the extra per-key cost (+~13K duffs).WalletKeyHealthSheet(new): diagnostic in Wallet Info. Re-derives eachPersistentPublicKeyfrom the wallet mnemonic, classifies as healthy / needsRederive / orphan, offers per-identity Re-derive (writes a fresh new-format keychain entry + sweeps the legacy one) or Delete Identity.WalletRowView.platformBalance+BalanceCardView.platformBalanceskip identities whosemodelContextis nil (mid-cascade-delete window —BackingData.swift:866fatals otherwise).Sources/.../DashPayService.swift(hardcoded key indices, didn't go through the new FFI),ObservableDashPayService(empty wrapper, never used),FriendsViewStubs(folded value types intoFriendsView).How Has This Been Tested?
Built + driven in the iOS Simulator end-to-end against testnet:
rs-sdkfix).identity_index = 0keep distinct private keys in Keychain after recovery.Not exercised on hardware or against mainnet.
Breaking Changes
None.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor