fix(swift-sdk): isLocal = mine-or-tracked; promote wallet identities, fix observed-entry mislinking - #4375
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:
📝 WalkthroughWalkthroughThe Swift SDK now derives ChangesIdentity locality and restoration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant IdentityView
participant PlatformWalletPersistenceHandler
participant PersistentIdentity
participant SwiftData
IdentityView->>PlatformWalletPersistenceHandler: load wallet and identities
PlatformWalletPersistenceHandler->>PersistentIdentity: reconcile wallet linkage and isLocal
PlatformWalletPersistenceHandler->>SwiftData: save corrected identity state
SwiftData-->>PlatformWalletPersistenceHandler: return restored identities
SwiftData-->>IdentityView: provide local and observed identities
IdentityView->>IdentityView: render status and available actions
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🕓 Ready for review — next in queue (commit b9c7399) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/Views/IdentityDetailView.swift (1)
296-307: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire a loaded wallet before exposing signing actions.
isLocalonly denotes persisted wallet ownership. It does not prove thatPlatformWalletManagerhas a loaded wallet handle. The detail view already applies the correct loaded-wallet check to top-up, transfer, withdrawal, and marketplace actions.
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift#L296-L307: replaceidentity.isLocalwithhasLoadedWallet(for: identity)before showing DPNS registration.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift#L375-L375: keep profile display available for observed identities, but pass loaded-wallet eligibility intodashPayProfileCardand hide its edit/setup controls when no loaded wallet exists.🤖 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/IdentityDetailView.swift` around lines 296 - 307, In IdentityDetailView.swift, update the DPNS registration button near lines 296-307 to require hasLoadedWallet(for: identity) instead of identity.isLocal. At line 375, keep the profile visible for observed identities but pass the loaded-wallet eligibility into dashPayProfileCard so its edit/setup controls are hidden without a loaded wallet.
🤖 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 4660-4667: Remove the unverified future incident date from the
historical comments in PlatformWalletPersistenceHandler and
IdentityIsLocalPersistenceTests, or replace it with the verified date; keep the
wording consistent across both files.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- Line 894: Update the observed-identity token-balance persistence flow around
the identity guard and persistTokenBalances call to use the handler’s configured
network explicitly, rather than resolving it through WalletId.default(). Ensure
new balance rows retain the configured networkRaw, including for unlinked
identities.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- Around line 296-307: In IdentityDetailView.swift, update the DPNS registration
button near lines 296-307 to require hasLoadedWallet(for: identity) instead of
identity.isLocal. At line 375, keep the profile visible for observed identities
but pass the loaded-wallet eligibility into dashPayProfileCard so its edit/setup
controls are hidden without a loaded wallet.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ffd0757-448a-45ad-a0e6-336051d8c5c8
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift
| /// One-shot self-heal: re-derive `PersistentIdentity.isLocal` | ||
| /// from the wallet relationship for every row where the two | ||
| /// disagree. Historical stores need this twice over: the persister | ||
| /// used to write `isLocal: false` unconditionally (so the wallet's | ||
| /// own identities carried `false` — the 2026-08-12 mainnet field | ||
| /// bug), and before that nothing wrote the flag from the changeset | ||
| /// path at all. Idempotent — a store where every row already | ||
| /// matches is left untouched (no save). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the future incident date.
Today is August 11, 2026. Both comments describe August 12, 2026 as a past incident. Replace it with the verified incident date or remove the date.
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift#L4660-L4667: correct or remove the historical incident date.packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift#L8-L13: use the same corrected date or remove the historical incident date.
📍 Affects 2 files
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift#L4660-L4667(this comment)packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift#L8-L13
🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`
around lines 4660 - 4667, Remove the unverified future incident date from the
historical comments in PlatformWalletPersistenceHandler and
IdentityIsLocalPersistenceTests, or replace it with the verified date; keep the
wording consistent across both files.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The fresh-upsert derivation fixes the reported isLocal flag for ordinary wallet-owned and observed rows, but ownership reconciliation remains unsafe in two blocking cases: historical rows corrupted by the old fallback are trusted during restore, and manager-relative observed entries can erase another wallet's valid ownership. The PR also contains a future-dated incident reference that should be corrected.
Source: reviewer backends gpt-5.6-sol (general) and gpt-5.6-sol (ffi-engineer); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 💬 1 nitpick(s)
🤖 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/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1822-1824: Observed FFI entries can erase another local wallet's ownership
`walletId == nil` and `identityIndex == nil` describe an identity as out-of-wallet relative to the Rust identity manager that emitted this changeset; they do not prove that no other wallet in the shared SwiftData store owns the identity. `load_identity_by_dpns_name` can add wallet B's identity to wallet A through `add_out_of_wallet_identity`, while `PersistentIdentity.identityId` is globally unique, so this callback resolves both managers' entries to the same row. The unconditional nil assignment then removes the valid wallet-B relationship and marks the identity non-local, causing wallet B to lose the identity from relationship-based restore and signing/UI paths. Preserve a relationship to a different wallet and only clear a nil/nil entry when its current relationship points to the callback's scope wallet—the relationship the old fallback could have fabricated.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:4684-4686: Startup heal promotes historically mislinked observed identities to wallet-owned
The previous unconditional scope-wallet fallback linked every nil-wallet observed entry to the scope wallet, while the absent identity index remained at `PersistentIdentity.identityIndex`'s default value of 0. This startup pass treats that historically corrupted relationship as authoritative and sets `isLocal = true`; `loadWalletList()` then includes the row in `w.identities` and restores it into Rust as wallet-owned, so it no longer re-emits in the nil-wallet/nil-index form that could clear the bad link. This can also displace the genuine index-0 identity: Swift sorts same-index rows by identity ID, and `build_wallet_identity_bucket` uses `bucket.insert(identity_index, managed)`, so the last index-0 row wins. The migration must validate or repair historical wallet relationships before deriving `isLocal` and constructing the restore buffer, using wallet/key derivation evidence rather than the relationship alone. Add an upgrade test that seeds the old-store shape and executes `loadWalletList()` through Rust restore; the current stale-row test directly re-emits an observed snapshot and bypasses this startup failure.
- [NITPICK] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:4660-4667: Remove the future incident date
The source comment and `IdentityIsLocalPersistenceTests.swift` describe 2026-08-12 as a historical observation, but the exact head was authored on 2026-08-11 and the current date is 2026-08-11. Remove the unverified date or replace it with the verified incident date consistently in both files.
| let ownerWalletId: Data? = | ||
| entry.walletId ?? (entry.identityIndex != nil ? walletId : nil) | ||
| row.wallet = fetchWalletForLink(walletId: ownerWalletId) |
There was a problem hiding this comment.
🔴 Blocking: Observed FFI entries can erase another local wallet's ownership
walletId == nil and identityIndex == nil describe an identity as out-of-wallet relative to the Rust identity manager that emitted this changeset; they do not prove that no other wallet in the shared SwiftData store owns the identity. load_identity_by_dpns_name can add wallet B's identity to wallet A through add_out_of_wallet_identity, while PersistentIdentity.identityId is globally unique, so this callback resolves both managers' entries to the same row. The unconditional nil assignment then removes the valid wallet-B relationship and marks the identity non-local, causing wallet B to lose the identity from relationship-based restore and signing/UI paths. Preserve a relationship to a different wallet and only clear a nil/nil entry when its current relationship points to the callback's scope wallet—the relationship the old fallback could have fabricated.
| let ownerWalletId: Data? = | |
| entry.walletId ?? (entry.identityIndex != nil ? walletId : nil) | |
| row.wallet = fetchWalletForLink(walletId: ownerWalletId) | |
| let ownerWalletId: Data? = | |
| entry.walletId ?? (entry.identityIndex != nil ? walletId : nil) | |
| if let ownerWalletId { | |
| row.wallet = fetchWalletForLink(walletId: ownerWalletId) | |
| } else if row.wallet?.walletId == walletId { | |
| row.wallet = nil | |
| } |
source: ['codex']
There was a problem hiding this comment.
Fixed in 257a938. The out-of-wallet branch now unlinks only when the row's current relationship points at the changeset's scope wallet — the one the old unconditional fallback could have fabricated — and leaves a relationship to any other wallet untouched. Covered by the new testObservedEntryPreservesAnotherWalletsLinkage: wallet B's row keeps its linkage (and its isLocal) when wallet A's manager emits the identity as observed.
There was a problem hiding this comment.
Resolved in this update — Observed FFI entries can erase another local wallet's ownership no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let shouldBeLocal = row.wallet != nil | ||
| if row.isLocal != shouldBeLocal { | ||
| row.isLocal = shouldBeLocal |
There was a problem hiding this comment.
🔴 Blocking: Startup heal promotes historically mislinked observed identities to wallet-owned
The previous unconditional scope-wallet fallback linked every nil-wallet observed entry to the scope wallet, while the absent identity index remained at PersistentIdentity.identityIndex's default value of 0. This startup pass treats that historically corrupted relationship as authoritative and sets isLocal = true; loadWalletList() then includes the row in w.identities and restores it into Rust as wallet-owned, so it no longer re-emits in the nil-wallet/nil-index form that could clear the bad link. This can also displace the genuine index-0 identity: Swift sorts same-index rows by identity ID, and build_wallet_identity_bucket uses bucket.insert(identity_index, managed), so the last index-0 row wins. The migration must validate or repair historical wallet relationships before deriving isLocal and constructing the restore buffer, using wallet/key derivation evidence rather than the relationship alone. Add an upgrade test that seeds the old-store shape and executes loadWalletList() through Rust restore; the current stale-row test directly re-emits an observed snapshot and bypasses this startup failure.
source: ['codex']
There was a problem hiding this comment.
Addressed in 257a938, with a deliberately non-destructive design:
-
Restore-slice collision guard —
loadWalletList()now routes each wallet's rows throughrestorableIdentities(_:walletId:), which resolves per-walletidentityIndexcollisions before anything reaches Rust's per-index BTreeMap. On a collision, the row with wallet-derivation key evidence wins:PersistentPublicKey.walletIdis stamped bypersistIdentityKeysonly for keys Rust derived from that wallet's DIP-9 tree, and out-of-wallet entries never produce key rows at all. A legacy mislink (placeholder index 0, no key rows) therefore loses to the genuine index-0 identity instead of displacing it. Losers are only omitted from the slice — store rows are untouched — and skips are logged. Covered bytestRestorableIdentitiesPrefersKeyEvidenceOnIndexCollision, where the mislink's id deliberately sorts first so ordering alone would pick the wrong row. -
Why no destructive store repair: validating legacy relationships by key derivation at restore time isn't possible without the seed (Keychain-backed, and
ExternalSignablewallets can't derive in-process), and any heuristic unlink risks a false positive on a genuinely-owned row — which would strip the user's own identity, i.e. recreate the original bug with the sign flipped. The mislinked rows heal organically instead: the scope-only unlink inpersistIdentitiesclears the fabricated relationship the next time the identity re-emits, and until then the collision guard keeps them out of the wallet-owned Rust state at index-collision points.
On the heal: it only touches isLocal (upward), never the relationship, so it doesn't create wallet-owned Rust state by itself — the restore marshalling you describe is merge-base behavior, now bounded by (1).
There was a problem hiding this comment.
Resolved in this update — Startup heal promotes historically mislinked observed identities to wallet-owned no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// One-shot self-heal: re-derive `PersistentIdentity.isLocal` | ||
| /// from the wallet relationship for every row where the two | ||
| /// disagree. Historical stores need this twice over: the persister | ||
| /// used to write `isLocal: false` unconditionally (so the wallet's | ||
| /// own identities carried `false` — the 2026-08-12 mainnet field | ||
| /// bug), and before that nothing wrote the flag from the changeset | ||
| /// path at all. Idempotent — a store where every row already | ||
| /// matches is left untouched (no save). |
There was a problem hiding this comment.
💬 Nitpick: Remove the future incident date
The source comment and IdentityIsLocalPersistenceTests.swift describe 2026-08-12 as a historical observation, but the exact head was authored on 2026-08-11 and the current date is 2026-08-11. Remove the unverified date or replace it with the verified incident date consistently in both files.
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in eaaa952 — Remove the future incident date no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…v11) Same disease as the Swift port (see the prior two commits and #4375): the persister only ever wrote isLocal = 0, so a wallet's own identity carried "not local" and the flag could only mislead. Ownership travels on the walletId FK; remove the column instead of healing it. - IdentityEntity loses the field; the persistence handler stops writing it; example-app readers (home-list subtitle, detail-screen read-only banner) re-key on walletId. - Room v11 + MIGRATION_10_11: table rebuild (create/copy/swap/reindex — same pattern as the platform_addresses and token_balances rebuilds; SQLite DROP COLUMN needs 3.35+, which older Android bundles lack). Exported 11.json regenerated via KSP; the hand-written DDL matches it column-for-column. - New migrate10To11DropsIsLocalAndKeepsRows instrumented test seeds a wallet-owned row (isLocal mistakenly 0 — the exact field bug) plus an observed row and asserts both survive with walletId intact; the 4→11 and 1→11 chain tests extend to v11. All 11 DashDatabaseMigrationTest cases pass on an API 35 emulator (after a -wipe-data — a stale AVD fails ALL of them with 'Unable to lock file …db.lck', pre-existing and unrelated). The ContestedNamesOwnershipTest JVM suite passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Kotlin v10-to-v11 migration can delete or detach identity-dependent data, and two Swift reconciliation paths can still erase valid ownership or promote legacy mislinks into wallet-owned Rust state. These three blocking issues must be fixed; the future-dated test comment is also still present.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 💬 1 nitpick(s)
2 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.kt:613-614: Preserve identity-dependent rows during the v10-to-v11 rebuild
Room enables SQLite foreign-key enforcement, so dropping `identities` performs an implicit deletion of every old parent row before `_new_identities` is renamed. SQLite applies the existing child actions during that deletion: rows in `public_keys`, `dpns_names`, `dashpay_profiles`, contact requests/profiles, ignored senders, payments, and `documents` are removed through `ON DELETE CASCADE`, while `data_contracts.ownerIdentityId` and `token_balances.identityRef` are set to null. Copying the parent rows into a differently named table first does not preserve those references. Rework the migration so all dependent data and foreign keys survive the parent-table replacement, and extend the migration test with at least one cascading child and one `SET NULL` reference; the current test only seeds identities and cannot detect this loss.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityWalletLinkageTests.swift`:
- [NITPICK] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityWalletLinkageTests.swift:12-13: Remove the future incident date
The replacement regression-test comment still describes 2026-08-12 as a historical mainnet observation even though the exact head and current review date are 2026-08-11. Remove the unverified date while retaining the reason for the test.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1817-1819: Observed FFI entries can erase another local wallet's ownership
(existing thread: https://github.com/dashpay/platform/pull/4375#discussion_r3760295036)
`walletId == nil` and `identityIndex == nil` only classify an identity as out-of-wallet relative to the Rust identity manager emitting this changeset. For example, wallet A can resolve wallet B's identity through `load_identity_by_dpns_name`; `add_out_of_wallet_identity` then emits the nil/nil shape from A's manager. Because `PersistentIdentity.identityId` is globally unique, this callback resolves wallet B's existing row and the unconditional nil assignment removes its valid wallet-B relationship. Preserve relationships belonging to another wallet and only clear the scope-wallet relationship that the previous fallback could have fabricated.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:4881-4907: Startup heal promotes historically mislinked observed identities to wallet-owned
(existing thread: https://github.com/dashpay/platform/pull/4375#discussion_r3760295050)
Removing the interim `isLocal` heal does not repair wallet relationships written by the merge-base handler's unconditional scope-wallet fallback. The lightweight SwiftData migration preserves those relationships, so an historically observed row remains in `w.identities` and is immediately marshalled as wallet-owned during startup. Rust's `build_wallet_identity_bucket` then stamps `wallet_id = Some(entry.wallet_id)` and inserts it by `identity_index`. Because the observed row retained the default index 0, it can also replace the genuine index-0 identity when `BTreeMap::insert` processes the duplicate key. Validate and repair legacy relationships using wallet/key-derivation evidence before constructing the restore buffer, and add an upgrade test that seeds the old-store relationship and executes the complete `loadWalletList()`-to-Rust restore path.
| db.execSQL("DROP TABLE `identities`") | ||
| db.execSQL("ALTER TABLE `_new_identities` RENAME TO `identities`") |
There was a problem hiding this comment.
🔴 Blocking: Preserve identity-dependent rows during the v10-to-v11 rebuild
Room enables SQLite foreign-key enforcement, so dropping identities performs an implicit deletion of every old parent row before _new_identities is renamed. SQLite applies the existing child actions during that deletion: rows in public_keys, dpns_names, dashpay_profiles, contact requests/profiles, ignored senders, payments, and documents are removed through ON DELETE CASCADE, while data_contracts.ownerIdentityId and token_balances.identityRef are set to null. Copying the parent rows into a differently named table first does not preserve those references. Rework the migration so all dependent data and foreign keys survive the parent-table replacement, and extend the migration test with at least one cascading child and one SET NULL reference; the current test only seeds identities and cannot detect this loss.
source: ['codex']
There was a problem hiding this comment.
Out of scope as of the current head: the Kotlin v10→v11 migration was descoped from this PR along with the whole column-removal direction (the PR now keeps isLocal with no schema change on either platform). The finding is noted on the parked refactor/remove-islocal-column branch, which must not be revived as-is — both for this FK-cascade concern and because its derivation predates the imported-keys semantics this PR settled on.
There was a problem hiding this comment.
Resolved in this update — Preserve identity-dependent rows during the v10-to-v11 rebuild no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // persisted as "not local" (observed on a mainnet device 2026-08-12, | ||
| // where it hid the identity-key refresh affordances in dashwallet-ios). |
There was a problem hiding this comment.
💬 Nitpick: Remove the future incident date
The replacement regression-test comment still describes 2026-08-12 as a historical mainnet observation even though the exact head and current review date are 2026-08-11. Remove the unverified date while retaining the reason for the test.
| // persisted as "not local" (observed on a mainnet device 2026-08-12, | |
| // where it hid the identity-key refresh affordances in dashwallet-ios). | |
| // persisted as "not local" on a mainnet device, | |
| // where it hid the identity-key refresh affordances in dashwallet-ios). |
source: ['codex']
There was a problem hiding this comment.
Done in 257a938 — the calendar date is removed from the incident comments (the referenced IdentityWalletLinkageTests.swift no longer exists on this branch; the surviving IdentityIsLocalPersistenceTests.swift and the heal comment now describe the incident without a date).
There was a problem hiding this comment.
Resolved in this update — Remove the future incident date no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
0abbe3b to
571345b
Compare
QuantumExplorer
changed the title
571345b to
3d8b067
Compare
Field testing found the wallet's own identity persisted with isLocal == false, which hid the refresh button and silently no-opped the pull gesture (#981 moved the key-refresh gates to the wallet relationship as a workaround). The SDK now defines isLocal as "this device can act as the identity" — via the wallet linkage OR imported key material (masternode voting/owner/payout keys, pasted user keys) — promoted by its writers and healed at startup (dashpay/platform#4375). Update the remaining app-side readers, written against the dead "Local Only / On Network" badge reading: - Identities list badge: the orange "Local Only" badge (which would have appeared on the user's OWN identities post-fix) becomes an "Observed" badge on the rare non-local rows. - Identity detail sheet: the "Status: Local Only / On Network" row becomes "Access: Local / Observed" — "Local" rather than "In Wallet" because imported-key identities are local without a wallet; the separate Wallet row names the wallet when there is one. - refreshFromNetwork: drop the '!row.isLocal' filter — it would have skipped exactly the wallet's own identities; every persisted row is Platform-confirmed, so refresh them all. Note: until the platform-side fix is pulled into ../platform, rows still carry isLocal == false and every identity shows the "Observed" badge — land/pull dashpay/platform#4375 first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
changed the title
…or identity ownership Review follow-ups on #4375 (thepastaclaw blockers): - An out-of-wallet entry now unlinks ONLY a relationship to the changeset's scope wallet — the one the old unconditional fallback could have fabricated. "Out-of-wallet" is relative to the emitting Rust manager: wallet A resolving wallet B's identity via load_identity_by_dpns_name emits the nil/nil shape from A's manager, and the row is globally keyed by identityId, so the previous unconditional clear could strip wallet B's valid ownership. - loadWalletList()'s restore slice now resolves per-wallet identityIndex collisions before rows reach Rust's per-index BTreeMap (last insert wins there): a legacy row mislinked by the pre-fix fallback carries the placeholder index 0 and could displace the genuine index-0 identity. On a collision the row with wallet-derivation key evidence wins (PersistentPublicKey.walletId is stamped only for keys Rust derived from this wallet's DIP-9 tree; out-of-wallet entries never get key rows). Non-destructive: losers are only omitted from the slice, and the scope-unlink above repairs the mislink when the identity re-emits. No destructive store repair: distinguishing legacy mislinks from genuinely-owned rows by derivation requires the seed, which isn't available at restore time, and a false-positive unlink would break the user's own identity. - Drop the disputed calendar date from the incident comments. New tests: observed emission from wallet A preserves wallet B's linkage; index-0 collision resolves to the key-evidence row with unique indexes passing through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Persistence/Models/PersistentIdentity.swift`:
- Line 184: Update the fixture construction in ProvenTokenBalancePersistTests
to pass isLocal: true, preserving the test’s documented local-identity behavior
instead of relying on the new default.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1823-1850: Update the unlink condition in the owner-wallet
handling near fetchWalletForLink so row.wallet is cleared only when
ownerWalletId is nil and the existing scope-wallet match holds. Preserve wallet
assignment when the lookup succeeds, and leave links untouched when
ownerWalletId is non-nil but fetchWalletForLink returns nil.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d6e9b66-d2dd-4989-a013-9462a7a1cd1f
📒 Files selected for processing (6)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Review findings for legacy ownership migration and wallet-backed action gating.
| ) else { return } | ||
| var healed = 0 | ||
| for row in rows where row.wallet != nil && !row.isLocal { | ||
| row.isLocal = true |
There was a problem hiding this comment.
[P1] Do not promote unverified legacy wallet links
Legacy observed identities mislinked by the old fallback satisfy this condition, so they become local before restore. The collision guard only helps when another row shares index 0; a sole mislink or evidence-less tie is still restored as wallet-owned and displayed as signable. Ambiguous legacy rows need ownership validation or quarantine before promotion/restore, plus a full loadWalletList-to-Rust restore regression.
There was a problem hiding this comment.
Addressed in 4516c46, split by harm:
- "Displayed as signable" — the heal now promotes
isLocalonly when the linkage is corroborated by wallet-derivation key evidence (walletKeyEvidence: aPersistentPublicKeystamped with the wallet's id, whichpersistIdentityKeyswrites only for DIP-9-derived keys and out-of-wallet entries never produce). A legacy mislink has no key rows, so it stays non-local and never presents as signable.testLoadWalletListPromotesOnlyEvidencedRowsAndNeverDemotescovers the mislink, the evidenced row, and the walletless-local row. - Sole mislink restored as wallet-owned — I tried the full quarantine you suggested (exclude every evidence-less linked row from the restore slice) and it broke legitimate restores: the contact- and payment-restore round-trip tests failed because collision-free rows without key stamps stopped restoring, which in a real older-era store would silently skip contact/payment rehydration. So the guard stays collision-scoped (evidence wins the index fight — that's the destructive case), and a sole index-0 mislink is instead displaced organically the next time the wallet's genuine identity loads:
add_identityreplaces the bucket slot, and the evidence-gated heal keeps the mislink from presenting as signable in the interim. The rationale is spelled out inrestorableIdentities' doc.
On the full loadWalletList()-to-Rust restore regression: the Swift half (slice construction) is unit-covered; a true cross-FFI restore test needs the Rust wallet-manager harness and I'd suggest tracking it separately rather than blocking this fix.
| // same upward-only heal at startup for rows this path | ||
| // never touches again. | ||
| row.isLocal = true | ||
| } else if row.wallet?.walletId == walletId { |
There was a problem hiding this comment.
[P1] Preserve ownership when wallet lookup misses
A non-nil ownerWalletId can still produce a nil fetch result, for example when the matching network-scoped wallet row is unavailable. Control then enters the unlink branch and clears an existing scope-wallet relationship even though the entry explicitly declared wallet ownership. Branch on ownerWalletId first and only unlink for a genuine nil-owner entry.
There was a problem hiding this comment.
Fixed in 4516c46 — the unlink branch now also requires ownerWalletId == nil, so an entry that declared an owner whose wallet row merely failed to resolve (network-scoped fetch miss) never strips the existing link. Covered by testDeclaredOwnerFetchMissDoesNotUnlink.
| if !identity.isLocal { | ||
| // Register name button — registration signs with | ||
| // the identity's wallet keys, so wallet-owned only. | ||
| if identity.isLocal { |
There was a problem hiding this comment.
[P2] Gate name registration on a loaded wallet
Under the new semantics, isLocal also includes walletless identities with imported keys. RegisterNameView immediately requires identity.wallet and a loaded Platform wallet, so this button is guaranteed to fail for that newly-supported local case. Use hasLoadedWallet here unless registration is extended to support walletless managed identities.
There was a problem hiding this comment.
Fixed in 4516c46 — the register-name button now gates on hasLoadedWallet(for:), matching what RegisterNameView immediately requires, with a comment noting it's deliberately NOT isLocal until registration supports walletless managed identities.
| .font(.caption) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| dashPayProfileCard(identity: identity) |
There was a problem hiding this comment.
[P2] Keep DashPay mutations wallet-backed
Making this card unconditional exposes Edit and Set up actions for walletless identities. The editor receives walletId nil and falls back to firstWallet, which can be unrelated and does not manage this identity, leading to identityNotFound or submission through the wrong manager. The profile may remain readable, but mutation buttons should require hasLoadedWallet and the arbitrary firstWallet fallback should be removed.
There was a problem hiding this comment.
Fixed in 4516c46 — both the Edit and Set-up buttons gate on hasLoadedWallet(for:) (the card itself stays readable for walletless/observed identities), and the editor's firstWallet fallback is removed: an unresolvable walletId now surfaces an explicit error instead of submitting through an unrelated wallet's manager.
| || row.ownerPrivateKeyIdentifier != nil | ||
| || row.payoutPrivateKeyIdentifier != nil | ||
| if importedAnyKey { | ||
| row.isLocal = true |
There was a problem hiding this comment.
[P2] Demote after the last imported key is forgotten
This promotion is never reversed when KeyDetailView or KeysListView deletes the final imported private key. A walletless identity then displays both Local and No Keys and continues passing isLocal-based action gates despite no longer being signable. Explicit key-removal flows can safely recompute isLocal from wallet linkage plus remaining key material without introducing blind persister demotion.
There was a problem hiding this comment.
Fixed in 4516c46 — new PersistentIdentity.recomputeIsLocalAfterKeyRemoval() recomputes from wallet linkage + remaining imported key material (user-key identifiers and the voting/owner/payout slots), documented as the ONE sanctioned demotion path: the removal flow can see the keychain state change; the persister and heal can't. Both forget-key sites (KeyDetailView, KeysListView) call it after clearing the keychain reference, so a walletless identity can no longer show Local + No Keys.
…wallet-gated example-app mutations Addresses QuantumExplorer's P1/P2 review comments on #4375: - [P1] The out-of-wallet unlink now also requires ownerWalletId == nil: an entry that DECLARED an owner whose wallet row merely failed to resolve (network-scoped fetch miss) is not an observation and no longer strips the existing scope link. - [P1] The startup heal promotes isLocal only when the linkage is corroborated by wallet-derivation key evidence (PersistentPublicKey.walletId, stamped only for keys Rust derived from that wallet's DIP-9 tree), so legacy mislinks are never displayed as signable. The restore-slice guard keeps its collision-scoped shape deliberately: a blanket evidence quarantine broke legitimate evidence-less restores (contact/payment round-trip tests caught it), so a collision-free row still restores and a sole index-0 mislink is instead displaced organically when the wallet's genuine identity next loads (add_identity replaces the bucket slot). - [P2] Forgetting a private key now recomputes isLocal via the new PersistentIdentity.recomputeIsLocalAfterKeyRemoval() — the one sanctioned demotion path (the removal flow can see the keychain state change; the persister and heal can't) — so a walletless identity can't show Local + No Keys after its last key is forgotten. - [P2] Register-name and DashPay profile Edit/Set-up gate on hasLoadedWallet (their flows resolve identity.wallet + a loaded Platform wallet immediately); the profile editor's arbitrary firstWallet fallback is removed in favor of an explicit error. - ProvenTokenBalancePersistTests' local-identity fixture passes isLocal: true explicitly now that the initializer default is false. New tests: declared-owner fetch miss preserves the link; heal promotes only evidenced rows (mislink stays non-local, walletless-local untouched); collision guard keeps evidence-less collision-free rows restorable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Re-review findings for legacy ownership restoration and local-access recomputation.
| i += 1 | ||
| } | ||
| if group.count == 1 { | ||
| result.append(group[0]) |
There was a problem hiding this comment.
[P1] Do not restore an unverified sole mislink as wallet-owned
The single-row path still appends an evidence-less legacy mislink. Rust treats every identity in this restore slice as wallet-owned and explicitly sets managed.wallet_id to the wallet ID; Swift mutation gates and DashPay eligibility also trust the stale wallet relationship. A wallet with no genuine identity may never receive the later add_identity that is supposed to displace it. Keeping isLocal false only changes the badge—it does not quarantine the identity from wallet-owned behavior. Ambiguous rows need an out-of-wallet restore path or durable ownership classification.
There was a problem hiding this comment.
Fixed in 6388513 — with the era-spanning evidence in place (see the pre-breadcrumb thread), "no key rows at all" is now a clean durable classification for the old fallback's product, so the restore slice quarantines key-less linked rows even when sole: they are never marshalled, so Rust never stamps wallet_id = Some on them and the corruption can't become durable, while the store row stays untouched for the upsert path's scope-unlink to repair on re-emit. Genuinely-owned rows of every era keep restoring (the earlier objection — evidence-less legitimate rows — is resolved by the era arm, not by re-widening the restore). The contact/payment round-trip fixtures gain era-style key rows to match the real invariant that an owned identity always has persisted key rows. Covered by testRestorableIdentitiesQuarantinesSoleKeylessMislink plus the updated collision test.
| for row in rows { | ||
| guard let ownerWalletId = row.wallet?.walletId, | ||
| !row.isLocal, | ||
| Self.walletKeyEvidence(row, walletId: ownerWalletId) |
There was a problem hiding this comment.
[P1] The heal skips legitimate pre-breadcrumb identities
PersistentPublicKey.walletId is optional specifically for keys persisted before the derivation-breadcrumb columns existed, and this patch itself preserves evidence-less rows because they can be genuinely wallet-owned. Those same upgrade rows fail this guard and retain isLocal false, reproducing the original misclassification until an uncertain future re-emission. The Keychain-driven breadcrumb backfill runs separately and cannot cover every watch-only or unmaterialized-key row. Ownership evidence must be migrated before healing, or another authoritative signal is needed.
There was a problem hiding this comment.
Fixed in 6388513 — walletKeyEvidence now spans the eras: a key row stamped with THIS wallet's id (post-breadcrumb strong evidence) or a key row with no stamp at all (pre-breadcrumb rows, where PersistentPublicKey.walletId is optional exactly as you note). The discriminator that makes this safe is that a mislinked observed row satisfies neither arm — out-of-wallet entries never produce key rows at all (identity + keys ride one atomic changeset round for owned identities only), and a key stamped for a different wallet never counts. So pre-breadcrumb genuine rows — including watch-only / unmaterialized-key ones, whose PUBLIC key rows exist regardless of private material — heal immediately at startup rather than waiting on a re-emission. Covered by the new pre-breadcrumb case in testLoadWalletListPromotesOnlyEvidencedRowsAndNeverDemotes.
| /// "No Keys". | ||
| public func recomputeIsLocalAfterKeyRemoval() { | ||
| isLocal = wallet != nil | ||
| || publicKeys.contains { $0.privateKeyKeychainIdentifier != nil } |
There was a problem hiding this comment.
[P2] Check actual Keychain state when recomputing local access
The recomputation treats a non-nil identifier as proof that key material remains, but the identity-list code already documents that identifiers can outlive missing or wiped Keychain items and therefore checks the Keychain directly. After forgetting the last accessible key, any other stale identifier leaves isLocal true and recreates the Local plus No Keys state. Recompute from concrete Keychain availability for remaining public and special keys, or clear stale references before evaluating.
There was a problem hiding this comment.
Fixed in 6388513 — recomputeIsLocalAfterKeyRemoval() now verifies every stored reference against live Keychain state before evaluating: per-key identifiers via hasPrivateKey(identityId:keyIndex:) / hasIdentityPrivateKey(publicKeyHex:) (the same probes the identity list's "No Keys" badge uses) and the voting/owner/payout slots via hasSpecialKey, clearing stale references as it goes. A dangling identifier can no longer keep a row "Local" after its backing item was wiped. @MainActor to match the Keychain probes and the UI removal flows it serves.
…ine, keychain-verified key-removal recompute Review round 3 on #4375: - [P1] walletKeyEvidence now spans key-persistence eras: a key row stamped with THIS wallet's id (strong, post-breadcrumb) OR a key row with no stamp at all (pre-breadcrumb — PersistentPublicKey.walletId is optional precisely because those rows predate the derivation columns). A mislinked observed row satisfies neither arm: out-of- wallet entries never produce key rows (Rust emits identity + keys in one atomic round for owned identities only), and a key stamped for a DIFFERENT wallet never counts. The heal therefore promotes pre-breadcrumb genuine rows instead of skipping them. - [P1] With mislinks now cleanly distinguishable by having NO key rows, the restore slice quarantines them even when sole: restoring one would stamp it wallet_id = Some in Rust and make the corruption durable. Genuinely-owned rows of every era — including watch-only / unmaterialized-key ones, whose PUBLIC key rows exist regardless of private material — keep restoring; the contact/payment round-trip fixtures gain era-style key rows to match reality (an owned identity always has persisted key rows). - [P2] recomputeIsLocalAfterKeyRemoval verifies each stored identifier against LIVE Keychain state (hasPrivateKey / hasIdentityPrivateKey / hasSpecialKey), clearing stale references before evaluating — a dangling identifier can no longer keep a row Local after its backing item was wiped. @mainactor to match the Keychain probes and the UI removal flows it serves. New/updated tests: pre-breadcrumb row heals via the era arm and restores collision-free; sole key-less mislink quarantined; key-less mislink loses the index-0 collision to the stamped genuine row. 336 unit tests pass (the two IdentityResolverSignIntegrationTests keychain-entitlement failures pre-exist on the baseline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Persistence/Models/PersistentIdentity.swift`:
- Around line 341-345: Update the isLocal assignment in PersistentIdentity to
replace the raw wallet != nil check with the same verified wallet-evidence
contract used by restoration, while preserving the existing private-key and
identifier checks. Ensure a keyless or stale wallet relationship does not mark
the identity local, including after its last imported key is removed.
- Around line 318-328: Update the successful import flow in
KeyDetailView.validateAndStorePrivateKey() to persist the matching
PersistentPublicKey.privateKeyKeychainIdentifier and promote identity.isLocal
before PersistentIdentity’s recomputation loop runs. Preserve existing ownership
checks, and add coverage for importing two keys, removing the last referenced
key, and retaining local state while the other imported key remains in Keychain.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swift`:
- Around line 352-355: Update the private-key deletion flow containing
identity.recomputeIsLocalAfterKeyRemoval in
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swift#L352-L355
to handle modelContext.save() errors visibly and keep the view presented until
persistence succeeds; apply the same handling before dismissal in
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swift#L544-L547,
ensuring both flows do not silently discard save failures after the irreversible
Keychain deletion.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a9a2d46-4a6d-4c55-93ce-17b307c64ef8
📒 Files selected for processing (8)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/ProvenTokenBalancePersistTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Re-review finding for wallet-ownership evidence classification.
| _ row: PersistentIdentity, | ||
| walletId: Data | ||
| ) -> Bool { | ||
| row.publicKeys.contains { $0.walletId == walletId || $0.walletId == nil } |
There was a problem hiding this comment.
[P1] An unstamped key is not reliable wallet-ownership evidence
A nil walletId does not uniquely identify a pre-breadcrumb key. LoadIdentityView and IdentityKeyRefresher create PersistentPublicKey rows with walletId nil for arbitrary observed identities. If one of those rows was linked by the old fallback—or refreshed after being mislinked—this predicate promotes it to Local and restores it into Rust as wallet-owned. A row containing a key stamped for another wallet plus an unstamped key also passes. This needs explicit migration provenance or validated derivation evidence, plus a regression covering a legacy mislink with nil-stamped public keys.
There was a problem hiding this comment.
You're right, and this reverses my round-3 change — fixed in 6432754. IdentityKeyRefresher and LoadIdentityView do persist unstamped key rows for arbitrary observed identities, so the era-fallback arm was unsound (and a foreign-wallet stamp + unstamped key also slipped the predicate). The single evidence contract is now PersistentIdentity.hasWalletDerivationEvidence(for:) — a key row stamped with THIS wallet's id, which only persistIdentityKeys writes for DIP-9-derived keys — shared by the heal, the restore quarantine, and recomputeIsLocalAfterKeyRemoval.
On the provenance question: the migration provenance already exists as the Keychain breadcrumb backfill (backfillIdentityKeyBreadcrumbs), which stamps pre-breadcrumb rows from Keychain metadata; validated derivation evidence beyond that isn't computable at load time (needs the seed). So a genuinely-owned unstamped row defers — backfill or its next owned re-emit stamps it and promotion/restore follow — which is recoverable, unlike a durably restored mislink. This consciously supersedes the round-3 concern about deferred promotion: soundness wins over immediacy now that we know unstamped keys can sit on observed rows.
Regressions added as requested: testRestorableIdentitiesQuarantinesSoleMislinkWithForeignKeys (mislink carrying an unstamped AND a foreign-stamped key → quarantined) and the heal case where a linked row with only an unstamped key is NOT promoted.
…fied save handling on key removal Review round 4 on #4375: - [P1] Unstamped key rows are no longer accepted as ownership evidence: LoadIdentityView and IdentityKeyRefresher persist unstamped PersistentPublicKey rows for arbitrary observed identities, so 'has any key row' could promote/restore a mislinked row after a by-id import or refresh, and a foreign-wallet stamp plus an unstamped key also slipped the previous predicate. The single evidence contract is now PersistentIdentity.hasWalletDerivationEvidence(for:) — a key row stamped with THIS wallet's id, written only by persistIdentityKeys for DIP-9-derived keys — shared by the heal, the restore quarantine, and recomputeIsLocalAfterKeyRemoval (which no longer trusts the raw wallet relationship either, per the parallel CodeRabbit finding). A genuinely-owned pre-breadcrumb row defers until the Keychain breadcrumb backfill or its next owned re-emit stamps it — a recoverable state, unlike a durably restored mislink; this consciously supersedes the round-3 era-fallback arm. Regressions: sole mislink carrying an unstamped + foreign-stamped key is quarantined; a linked row with only an unstamped key is not promoted by the heal. The contact/payment restore fixtures use stamped keys, matching what the persister actually writes. - Forget-key flows surface a SwiftData save failure in a dedicated alert and keep the sheet open instead of dismissing: the Keychain delete is irreversible, so a silently dropped save would leave a stale reference + isLocal with no way to re-run the flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| /// meanwhile is a recoverable deferral — restoring a mislink as | ||
| /// wallet-owned is not. | ||
| public func hasWalletDerivationEvidence(for walletId: Data) -> Bool { | ||
| publicKeys.contains { $0.walletId == walletId } |
There was a problem hiding this comment.
[P1] Preserve wallet derivation evidence during refresh
This method is now the sole ownership-evidence contract, but LoadIdentityView and IdentityKeyRefresher replace the public-key rows without carrying walletId or identityDerivationPath forward. Refreshing a genuine wallet identity therefore erases the evidence required by restorableIdentities; on the next launch it can be quarantined from Rust restore, particularly offline or after stored scalars have been removed. Preserve both provenance fields when rebuilding matching keys, or route refreshes through the ownership-aware persister, and add a refresh-then-restore regression test.
There was a problem hiding this comment.
Verified and agreed — this was a real interaction bug introduced by round 4 making the stamps load-bearing. Fixed in a51c061: new model seam replacePublicKeysPreservingProvenance(with:) carries the keychain identifier AND the derivation breadcrumb (walletId + identityDerivationPath) forward when rebuilding key rows, gated on a full (keyId, publicKeyData) match so different key material can't inherit evidence. Both IdentityKeyRefresher and LoadIdentityView route through it (the wholesale removeAll() is gone). Regression test testKeyReplacePreservesProvenanceAndSurvivesRestore covers refresh-then-restore: the stamp survives, brand-new key material stays unstamped, and the refreshed identity still passes restorableIdentities.
| // Forgetting the last imported key can end the identity's | ||
| // local status — recompute from what remains (wallet | ||
| // linkage / other key material). | ||
| identity.recomputeIsLocalAfterKeyRemoval() |
There was a problem hiding this comment.
[P1] Delete the wallet-derived Keychain record
This view can reveal a key from either the legacy (identityId, keyIndex) scheme or the wallet-derived identity_privkey.* scheme, but forgetPrivateKey deletes only the legacy record. deletePrivateKey reports success when that legacy item is absent, so this recompute/save path clears the SwiftData reference while the wallet-derived secret remains retrievable and usable by the signer. Delete the actual storage scheme—using deleteIdentityPrivateKey(walletId:derivationPath:) when its breadcrumb exists—and apply the same correction to KeyDetailView.
There was a problem hiding this comment.
Verified and agreed — deletePrivateKey returns true on errSecItemNotFound and the reveal path falls back to retrieveIdentityPrivateKey(publicKeyHex:), so the forget was a no-op for wallet-derived keys exactly as described. Fixed in a51c061: both forget flows (KeysListView's PrivateKeyView and KeyDetailView) now also delete the wallet-derived identity_privkey.* item via the persisted breadcrumb (deleteIdentityPrivateKey(walletId:derivationPath:)), AND-ing the results so a failed delete keeps the SwiftData reference and surfaces through the existing error path. (Note: for a wallet-derived key the signer can still re-derive from the seed on demand — the forget removes the materialized secret, which is that flow's scope.)
… forget deletes both keychain schemes Review round 5 on #4375 — both findings verified before fixing: - [P1] Refresh flows were erasing ownership evidence: IdentityKeyRefresher and LoadIdentityView rebuilt the public-key rows from freshly fetched data carrying only the keychain identifier forward, dropping walletId + identityDerivationPath — which round 4 made load-bearing (the restore quarantine requires the stamp), so refreshing a genuine wallet identity could quarantine it from the next restore. New model seam replacePublicKeysPreservingProvenance(with:) carries the identifier AND the breadcrumb forward, but only on a full (keyId, publicKeyData) match — different key material must not inherit evidence. Both flows now use it; LoadIdentityView's wholesale removeAll() is gone. Refresh-then-restore regression: stamp survives, new key material stays unstamped, the refreshed identity still passes restorableIdentities. - [P1] forgetPrivateKey deleted only the legacy (identityId, keyIndex) keychain item, which reports success when absent — so forgetting a wallet-derived key cleared the SwiftData reference while the identity_privkey.* secret stayed retrievable via the reveal fallback. Both forget flows now also delete the wallet-derived item through the persisted breadcrumb (deleteIdentityPrivateKey(walletId:derivationPath:)) and AND the results, so a failed delete keeps the reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // secret stayed retrievable. | ||
| let km = KeychainManager.shared | ||
| var removed = km.deletePrivateKey(identityId: identity.identityId, keyIndex: Int32(keyId)) | ||
| if let persistedKey = identity.publicKeys.first(where: { $0.keyId == Int32(keyId) }), |
There was a problem hiding this comment.
[P1] Forget derived keys without requiring breadcrumbs
The derived-key deletion is conditional on both breadcrumb fields, but PersistentPublicKey explicitly supports pre-migration rows where these fields remain nil until backfill. getPrivateKey can still reveal such a key by public-key hex; Forget then deletes the nonexistent legacy item, skips this block, clears the SwiftData reference, and leaves the derived secret retrievable. Resolve the matching identity-key Keychain account using its stored identifier or metadata/public-key lookup, delete it independently of the SwiftData breadcrumb, and verify hasIdentityPrivateKey is false before clearing the reference. Apply the same correction in KeyDetailView.
There was a problem hiding this comment.
Verified and agreed — the round-5 fix had exactly that gap: gating the derived-key delete on the SwiftData breadcrumb leaves pre-migration rows (nil breadcrumbs until backfill) forgettable-in-name-only, while getPrivateKey still reveals them by public-key hex. Fixed in 88c410a:
New KeychainManager.forgetIdentityKeyMaterial(...) is the single forget seam for both views. It deletes the legacy (identityId, keyIndex) item, the breadcrumb-addressed derived item when one exists, and ALWAYS runs a breadcrumb-independent sweep resolving the identity_privkey.* account by metadata public-key lookup (looped, since several accounts can carry the same public key). It returns true only after verifying hasIdentityPrivateKey and hasPrivateKey are both false — and both views now guard on that result, surfacing a "nothing was changed" alert instead of clearing the SwiftData reference when anything remains retrievable (clearing would orphan the secret with no handle pointing at it). Applied identically in KeysListView's PrivateKeyView and KeyDetailView.
…hout breadcrumbs, verify before clearing Review round 6 on #4375, verified before fixing: the round-5 derived-key delete was gated on BOTH SwiftData breadcrumb fields, but pre-migration rows carry nil breadcrumbs until the backfill runs while their secret is still revealable by metadata public-key lookup — so Forget deleted the absent legacy item, skipped the derived one, cleared the reference, and left the secret retrievable. New KeychainManager.forgetIdentityKeyMaterial(identityId:keyIndex: publicKeyHex:breadcrumbWalletId:derivationPath:) is the single forget seam for both views: - deletes the legacy (identityId, keyIndex) item; - deletes the wallet-derived item via the breadcrumb when the caller has one, and ALWAYS runs a breadcrumb-independent metadata public-key sweep (looped — several accounts can carry the same public key; converges as each delete lands); - returns true only after verifying nothing remains retrievable under either scheme (hasIdentityPrivateKey + hasPrivateKey). Both forget flows guard on that result and surface a 'nothing was changed' alert on failure instead of clearing the SwiftData reference — clearing while the secret is retrievable would orphan the key with no handle pointing at it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // removes one, so the lookup converges to nil. A delete error | ||
| // breaks out — `ok` goes false and the verification below | ||
| // fails closed. | ||
| while let account = identityPrivateKeyAccount(publicKeyHex: publicKeyHex) { |
There was a problem hiding this comment.
[P1] Verify deletion with the same lookup used for retrieval
Both the sweep and its postcondition depend on decoding kSecAttrGeneric, but retrieveIdentityPrivateKey retrieves directly by kSecAttrLabel and does not decode metadata. An identity_privkey.* row with a matching label but missing or corrupt metadata is therefore still readable by the signer while identityPrivateKeyAccount skips it and hasIdentityPrivateKey reports false; this function returns success and the views clear their reference. The existing legacy-delete code explicitly recognizes this malformed-metadata state. Delete and verify using the same label-based selector as retrieval without loading the secret, or require retrieveIdentityPrivateKey itself to be nil before reporting success.
There was a problem hiding this comment.
Verified and agreed — the sweep and its postcondition matched by decoded metadata while retrieveIdentityPrivateKey serves by service + kSecAttrLabel without ever decoding, so a label-matching row with missing/corrupt metadata survived a 'successful' forget exactly as you describe. Fixed in c060338: forgetIdentityKeyMaterial now also sweeps with retrieval's own selector via a new attributes-only identityPrivateKeyAccounts(labeledWith:) (service + lowercased-label, mirroring the retrieval query byte-for-byte; kSecReturnAttributes only, so the secret never enters Swift memory — per the 'without loading the secret' option). The success postcondition now requires the label-selector lookup to come back empty in addition to hasIdentityPrivateKey and the legacy check, so nothing retrieval can serve survives a forget that reports true, and the views keep their reference otherwise.
… label selector Review round 7 on #4375, verified before fixing: the round-6 sweep and its postcondition both matched identity_privkey.* rows by decoding the kSecAttrGeneric metadata blob, while retrieveIdentityPrivateKey serves the signer by service + kSecAttrLabel alone and never decodes metadata. A row with a matching label but missing/corrupt metadata was therefore still readable after a 'successful' forget: the metadata sweep skipped it, hasIdentityPrivateKey reported false, and the views cleared their reference. forgetIdentityKeyMaterial now additionally sweeps and verifies with the retrieval selector itself — a new attributes-only identityPrivateKeyAccounts(labeledWith:) helper (service + label, exactly as retrieval queries; secret bytes never enter Swift memory). Success requires both the metadata check AND the label-selector check to come back empty, so nothing retrieval can serve survives a forget that reports true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift (1)
385-454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the different-key-material branch of the provenance match.
replacePublicKeysPreservingProvenancerequires a match on bothkeyIdandpublicKeyData, and its documentation states that a breadcrumb on different key material would fabricate evidence. This test only exercises a differentkeyIdthroughbrandNew, so thepublicKeyDatahalf of the condition is never exercised.Add a case where an incoming key reuses
keyId0 with differentpublicKeyData. Assert that it inherits neither the stamp nor the keychain identifier.💚 Proposed additional test
/// Rotated key material at the SAME keyId must not inherit the /// breadcrumb — the stamp describes one specific public key. func testKeyReplaceDoesNotCarryProvenanceToRotatedKeyMaterial() throws { try insertWalletRow() let context = ModelContext(container) let wallet = try XCTUnwrap( try context.fetch(FetchDescriptor<PersistentWallet>()).first ) let identity = PersistentIdentity( identityId: ownIdentityId, isLocal: true, network: .testnet ) identity.wallet = wallet context.insert(identity) attachKeyRow(to: identity, stampedWalletId: walletId) identity.publicKeys[0].privateKeyKeychainIdentifier = "kc-0" try context.save() // Same keyId, different bytes. let rotated = PersistentPublicKey( keyId: 0, purpose: .authentication, securityLevel: .master, keyType: .ecdsaSecp256k1, publicKeyData: Data(repeating: 0x33, count: 33), identityId: identity.identityIdString ) identity.replacePublicKeysPreservingProvenance(with: [rotated]) try context.save() let row = try XCTUnwrap(identity.publicKeys.first { $0.keyId == 0 }) XCTAssertNil(row.walletId, "different key material must not inherit the stamp") XCTAssertNil(row.privateKeyKeychainIdentifier) XCTAssertTrue( PlatformWalletPersistenceHandler.restorableIdentities( [identity], walletId: walletId ).isEmpty, "no surviving evidence means no wallet-owned restore" ) }🤖 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/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift` around lines 385 - 454, Add a separate test for replacePublicKeysPreservingProvenance where an incoming key reuses keyId 0 but has different publicKeyData. Assert the replacement has neither walletId nor privateKeyKeychainIdentifier, and verify restorableIdentities returns no wallet-owned identity when that is the only key.
🤖 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 945-958: Bound the metadata sweep loop in
forgetIdentityKeyMaterial by adding an iteration cap around
identityPrivateKeyAccount(publicKeyHex:) and deleteGenericPassword(account:).
Stop sweeping once the cap is reached, while preserving the existing catch
behavior that sets ok to false; allow the existing verification block to fail
closed when cleanup does not converge.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swift`:
- Around line 378-383: Update validateAndStorePrivateKey() to retain the account
identifier returned by KeychainManager.storePrivateKey and assign it to the
matching PersistentPublicKey.privateKeyKeychainIdentifier after a successful
import. Preserve the existing success state updates and save the model context,
then add coverage for importing two keys and removing one while the other
remains local.
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swift`:
- Around line 551-559: Update the key-removal flow around
forgetIdentityKeyMaterial so it fails closed when publicKeyHex cannot be
resolved, rather than passing an empty string. Require a non-empty resolved
public key before invoking the forget seam; preserve the existing removal and
privateKeyKeychainIdentifier-clearing path only when verification succeeds.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift`:
- Around line 385-454: Add a separate test for
replacePublicKeysPreservingProvenance where an incoming key reuses keyId 0 but
has different publicKeyData. Assert the replacement has neither walletId nor
privateKeyKeychainIdentifier, and verify restorableIdentities returns no
wallet-owned identity when that is the only key.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15958bfa-d87a-45b2-af7d-f247525e506c
📒 Files selected for processing (9)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityKeyRefresher.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
There was a problem hiding this comment.
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/Views/KeyDetailView.swift (1)
404-423: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecompute
isLocaleven when noPersistentPublicKeyrow matches.
hasPrivateKeyreads the Keychain, not the persisted row. A legacy identity can therefore hold a Keychain item with no matchingPersistentPublicKeyrow, which makespersistedKeynil. The flow then deletes the Keychain item and dismisses without callingrecomputeIsLocalAfterKeyRemoval(). The identity keepsisLocal == trueafter its last key is gone.Move the recompute and the save outside the
if let persistedKeyblock.🐛 Proposed fix
- if let persistedKey { - persistedKey.privateKeyKeychainIdentifier = nil - // Forgetting the last imported key can end the - // identity's local status — recompute from what - // remains (wallet linkage / other key material). - identity.recomputeIsLocalAfterKeyRemoval() - do { - try modelContext.save() - } catch { - // The Keychain item is already gone (that delete - // is irreversible), so a silent save failure - // would leave a stale reference + isLocal on the - // next launch with no way to re-run this flow. - // Surface it and keep the sheet open. - modelContext.rollback() - forgetError = "The key was removed from the Keychain, but saving the change failed: \(error.localizedDescription)" - return - } - } + persistedKey?.privateKeyKeychainIdentifier = nil + // Forgetting the last imported key can end the identity's + // local status — recompute from what remains (wallet + // linkage / other key material). Runs even when no row + // matched: the Keychain item is gone either way. + identity.recomputeIsLocalAfterKeyRemoval() + do { + try modelContext.save() + } catch { + // The Keychain item is already gone (that delete is + // irreversible), so a silent save failure would leave a + // stale reference + isLocal on the next launch with no + // way to re-run this flow. Surface it and keep the + // sheet open. + modelContext.rollback() + forgetError = "The key was removed from the Keychain, but saving the change failed: \(error.localizedDescription)" + return + } dismiss()🤖 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/KeyDetailView.swift` around lines 404 - 423, Move identity.recomputeIsLocalAfterKeyRemoval() and the modelContext.save() error-handling flow out of the if let persistedKey block in the key-removal action. Keep clearing persistedKey.privateKeyKeychainIdentifier conditional on the matching row, but always recompute isLocal and save—even when persistedKey is nil—while preserving rollback, forgetError assignment, and early return on save failure.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift (1)
296-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the watch-only case for the heal path.
This test covers the signing-material gate on the upsert path only.
healIdentityIsLocalFlagsapplies the same gate at startup, buttestLoadWalletListPromotesOnlyEvidencedRowsAndNeverDemotesruns with the default probe returningtrue. A regression that drops the probe check from the heal would promote a watch-only wallet's evidenced rows toLocalat launch and no test would fail.Add a test that sets
walletSigningMaterialProbetofalse, seeds a wallet-linked row with a stamped key row andisLocal == false, callsloadWalletList(), and asserts the row stays non-local.🤖 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/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift` around lines 296 - 309, Add a startup-heal test near testLoadWalletListPromotesOnlyEvidencedRowsAndNeverDemotes that sets walletSigningMaterialProbe to return false, seeds a wallet-linked identity with a stamped key row and isLocal false, calls loadWalletList(), and verifies the identity remains non-local. Reuse the existing setup and fetch helpers and preserve the existing promotion test unchanged.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (1)
1839-1854: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the signing-material probe across the upsert loop.
The default probe calls
WalletStorage().hasMnemonic(for:), which reads the Keychain. This branch calls it once per upsert entry, so a round carrying N identities performs N Keychain reads on the serial queue inside the Ruststore()bracket.healIdentityIsLocalFlagsalready caches the result per wallet in itssignabledictionary; the upsert path does not.Cache the result per wallet for the duration of the loop.
♻️ Proposed refactor
func persistIdentities( walletId: Data, upserts: [IdentityEntrySnapshot], removed: [Data] ) { onQueue { + // One Keychain probe per wallet per round — the default + // probe reads `WalletStorage`, and a round can carry many + // identities for the same wallet. + var signable: [Data: Bool] = [:] for entry in upserts {if let ownerWallet = fetchWalletForLink(walletId: ownerWalletId) { row.wallet = ownerWallet - if walletSigningMaterialProbe(ownerWallet.walletId) { + let ownerId = ownerWallet.walletId + if signable[ownerId] == nil { + signable[ownerId] = walletSigningMaterialProbe(ownerId) + } + if signable[ownerId] == true { row.isLocal = true } } else if let declaredOwnerId = ownerWalletId {🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift` around lines 1839 - 1854, Memoize wallet signing-material checks within the upsert loop containing wallet linkage and walletSigningMaterialProbe. Add a per-wallet cache for probe results, reuse the cached value for repeated wallet IDs, and invoke walletSigningMaterialProbe only on the first encounter of each wallet during that loop.
🤖 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.
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swift`:
- Around line 404-423: Move identity.recomputeIsLocalAfterKeyRemoval() and the
modelContext.save() error-handling flow out of the if let persistedKey block in
the key-removal action. Keep clearing persistedKey.privateKeyKeychainIdentifier
conditional on the matching row, but always recompute isLocal and save—even when
persistedKey is nil—while preserving rollback, forgetError assignment, and early
return on save failure.
---
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1839-1854: Memoize wallet signing-material checks within the
upsert loop containing wallet linkage and walletSigningMaterialProbe. Add a
per-wallet cache for probe results, reuse the cached value for repeated wallet
IDs, and invoke walletSigningMaterialProbe only on the first encounter of each
wallet during that loop.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift`:
- Around line 296-309: Add a startup-heal test near
testLoadWalletListPromotesOnlyEvidencedRowsAndNeverDemotes that sets
walletSigningMaterialProbe to return false, seeds a wallet-linked identity with
a stamped key row and isLocal false, calls loadWalletList(), and verifies the
identity remains non-local. Reuse the existing setup and fetch helpers and
preserve the existing promotion test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8468165-b2c3-4ac4-a6ce-3f9d3b759e04
📒 Files selected for processing (7)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeyDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LoadIdentityView.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityIsLocalPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/KeysListView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Consolidated review of the current head: two blocking restore-integrity findings and four additional signing-capability/key-lifecycle findings remain. Swift CI is green, but these are behavioral correctness issues that should be addressed before merge.
| walletId: Data | ||
| ) -> [PersistentIdentity] { | ||
| let restorable = identities.filter { row in | ||
| let hasEvidence = walletKeyEvidence(row, walletId: walletId) |
There was a problem hiding this comment.
[P1] Preserve legitimate pre-breadcrumb watch-only identities
This filter still permanently excludes a genuine legacy identity when its public-key rows predate the breadcrumb columns and no identity_privkey.* metadata exists. That is the normal shape for a watch-only/xpub wallet, and it can also occur when only the older direct-key scheme exists. backfillAllWalletBreadcrumbsForLoad cannot stamp those rows, and once this identity is omitted from the Rust restore bucket there is no guaranteed owned re-emission to repair it. A Keychain lookup error is also collapsed to an empty metadata list, producing the same quarantine. Please use a migration-safe ownership signal for legacy/watch-only rows, or preserve an unknown state and retry rather than treating absence of private-key-derived evidence as proof that the wallet link is invalid.
There was a problem hiding this comment.
Agreed, and taken with your second option — preserve unknown — in 98bf816. You're right that this population can NEVER earn a stamp (identity keys are hardened-path, underivable from an xpub, so no identity_privkey.* metadata can exist for a watch-only wallet) and that omission from the Rust bucket has no guaranteed repair emission; permanent exclusion is strictly worse than what it prevented. restorableIdentities now restores sole unstamped rows as ownership-unknown; only same-index collisions are resolved (stamped contender wins — the case where a fallback-era mislink at placeholder index 0 would displace the genuine identity). The safety story moves entirely to presentation: the heal and the persister never promote such rows to Local (tri-state probe, unknown defers), so a restored mislink shows Observed with no mutation controls, and the upsert path unlinks it on an out-of-wallet or differently-declared re-emit. testSoleUnstampedRowRestoresAsOwnershipUnknownButNeverLocal pins both halves.
| let byWallet = Dictionary(grouping: allMetadata) { $0.walletId.lowercased() } | ||
| for (walletIdHex, items) in byWallet { | ||
| guard let walletId = Data(hexString: walletIdHex) else { continue } | ||
| _ = backfillCore(walletId: walletId, items: items) |
There was a problem hiding this comment.
[P1] Scope the breadcrumb backfill to the metadata identity and key
The newly load-bearing call reaches backfillCore, where the target row is fetched by publicKeyData alone and .first is used. IdentityPrivateKeyMetadata already carries identityId and keyId; ignoring them means duplicate/reused public-key bytes can stamp an observed or stale row with another identity's wallet/path. That wrong row then passes restorableIdentities as owned while the genuine row can remain quarantined. Match the full identity/key tuple (plus public-key bytes and applicable network/wallet context) before writing the breadcrumb.
There was a problem hiding this comment.
Verified and agreed — backfillCore matched by publicKeyData alone with .first. Fixed in 98bf816: keyId rides the predicate and the identity is verified through the relationship (canonical 32-byte id) with a string-column fallback for legacy rows persisted without the relationship, so duplicate/reused key bytes can no longer stamp another identity's row with this wallet's breadcrumb.
| /// `serialQueue` like the rest of the handler's mutable state — | ||
| /// override before first use. | ||
| var walletSigningMaterialProbe: (Data) -> Bool = { walletId in | ||
| WalletStorage().hasMnemonic(for: walletId) |
There was a problem hiding this comment.
[P2] Probe actual identity signing capability, not mnemonic existence
hasMnemonic is neither necessary nor sufficient for this identity to sign. KeychainSigner can use a materialized identity scalar without a mnemonic, while a mnemonic item can exist without being seed-bound to this wallet/key set. It also collapses .unavailable and .absent, so a temporary Keychain failure is persisted as non-signability. This can produce both false Observed and false Local classifications. Please centralize a tri-state, identity-specific capability check that considers verified seed binding and live stored/special key material, preserving or retrying on unknown availability.
There was a problem hiding this comment.
Addressed in 98bf816 with a proportionate version of the centralization you asked for: a single WalletSigningCapability.probe (backed by WalletStorage.mnemonicAvailability) now feeds the persister gate, the heal, and the key-removal recompute, and it is tri-state — .present/.absent decide, .unavailable defers: since every writer promotes one-way, a transient Keychain failure now costs a delayed promotion, never a persisted non-signability. On necessity/sufficiency: imported scalars are deliberately covered by the key-material arms of isLocal rather than the wallet arm, and full seed-binding verification stays at unlock (verifySeedBinding is async manager machinery that gates actual signing) — the probe's doc spells out that it gates the persisted classification, not signing itself. If you want per-identity live-material resolution folded in as well, I'd suggest that as a follow-up on top of this seam rather than blocking here.
| guard !identity.isLocal, | ||
| let walletId = identity.wallet?.walletId else { return false } | ||
| guard let walletId = identity.wallet?.walletId else { return false } | ||
| return walletManager.wallet(for: walletId) != nil |
There was a problem hiding this comment.
[P2] Require signability as well as a loaded wallet for mutations
The PR now deliberately keeps a loaded watch-only wallet's identity at isLocal == false, but this eligibility check admits it based only on the wallet handle. The same loaded-wallet-only pattern remains on top-up/transfer/withdraw, DPNS marketplace/register, and DashPay profile edit/setup controls in IdentityDetailView. Those controls are exposed and then fail at signer/core-key resolution. Conversely, the token link checks only isLocal, admitting walletless imported-key identities into action views that require a managed wallet. Manager-backed mutations need both a loaded handle and operation-specific signing capability; read-only presentation can remain separate.
There was a problem hiding this comment.
Fixed in 98bf816 — one canMutate(as:) gate (isLocal && hasLoadedWallet) now fronts every manager-backed mutation: DashPay-tab eligibility, top-up/transfer/withdraw, DPNS register + marketplace, profile Edit/Set-up, and the token-actions link (which, as you note, previously admitted walletless imported-key identities with no manager handle). Read-only presentation — profile card, token rows, key list — stays ungated.
| let walletEvidence = wallet.map { | ||
| hasWalletDerivationEvidence(for: $0.walletId) | ||
| } ?? false | ||
| isLocal = walletEvidence |
There was a problem hiding this comment.
[P2] Do not treat derivation evidence as current signing material
This contradicts the persister's new mnemonic gate. A watch-only linked identity starts non-local, becomes local when the user imports one scalar, and should become non-local again when that scalar is forgotten. Its stamped public key makes walletEvidence stay true here, so it remains Local with no remaining signing material. Recompute with the same real capability resolver used by startup/upsert; a wallet relationship or derivation breadcrumb proves ownership, not present ability to sign.
There was a problem hiding this comment.
Verified and agreed — the recompute's wallet arm conflated ownership evidence with current capability. Fixed in 98bf816: it now uses the same WalletSigningCapability.probe as the persister/heal (current signing material, not derivation stamps), so your exact scenario — watch-only linked identity, one imported scalar forgotten — comes out non-local despite its stamps. When the Keychain can't answer, the recompute aborts wholesale and leaves the flag as it stands, since every arm of it depends on Keychain truth.
| // would leave a stale reference + isLocal on the | ||
| // next launch with no way to re-run this flow. | ||
| // Surface it and keep the sheet open. | ||
| modelContext.rollback() |
There was a problem hiding this comment.
[P2] Do not roll back the repair after irreversible Keychain deletion
The secret is already gone when save() fails, but this rollback restores the obsolete identifier and old isLocal value. Live Keychain probes then hide the Forget control, startup reconciliation is upward-only, and the alert offers no retry, so the stale persisted state can become permanent. The same issue exists in KeysListView. Also move recompute/save outside if let persistedKey: a legacy key can be deleted successfully without a matching SwiftData key row, and the current path then skips locality repair entirely. Keep or durably retry the corrective model mutation instead of rolling it back after the irreversible delete.
There was a problem hiding this comment.
Verified and agreed — the rollback recreated the orphaning it was meant to prevent, since the Keychain delete is irreversible. Fixed in 98bf816: both forget flows (and the import flow, which had the same defect) keep the corrective in-memory mutation on save failure — the main context autosaves, so persistence retries — and surface an alert saying the change will be re-saved; the reference/recompute can no longer be reverted against a dead secret. The recompute + save also moved outside the persistedKey conditional so a legacy-scheme key forgotten without a matching SwiftData row still gets its locality repaired.
…-state capability probe, no-rollback repairs Review round 9 on #4375, each finding verified first: - [P1] backfillCore matches the full metadata tuple: keyId rides the predicate and the identity is verified via the relationship (canonical id) with a string-column fallback for legacy rows — duplicate/reused public-key bytes can no longer stamp another identity's row with this wallet's breadcrumb. - [P1] restorableIdentities no longer quarantines sole unstamped rows (the reviewer's preserve-unknown option): watch-only/xpub wallets can NEVER earn a stamp (identity keys are hardened-path, underivable from an xpub) and an identity omitted from the Rust bucket has no guaranteed owned re-emission — permanent exclusion was worse than the mislink it prevented. Ownership-unknown rows restore; index collisions still prefer the stamped contender; and everything user-facing (Local, mutation controls) still demands proof, so a restored mislink presents as Observed only. - [P2] One tri-state capability probe (WalletSigningCapability, backed by WalletStorage.mnemonicAvailability) now feeds the persister gate, the heal, AND recomputeIsLocalAfterKeyRemoval: present/absent decide, unavailable defers (never persisted as non-signability). The recompute's wallet arm uses current signing material instead of derivation evidence — a watch-only identity whose one imported scalar is forgotten comes out non-local even though its stamps remain — and aborts wholesale when the Keychain can't answer. - [P2] Manager-backed mutations require BOTH signability and a loaded handle via one canMutate(as:) gate: DashPay-tab eligibility, top-up/transfer/withdraw, DPNS register + marketplace, profile Edit/Set-up, and the token-actions link (which previously admitted walletless imported-key identities with no manager handle). - [P2] The forget/import flows no longer roll back the corrective model mutation after irreversible Keychain writes — rolling back restored a reference to a nonexistent secret, hid the Forget control behind live probes, and left no repair path. The truthful in-memory state is kept (main-context autosave retries), the recompute/save moved outside the persistedKey conditional (a legacy-scheme key can be forgotten without a matching row), and KeysListView fails closed on an unresolvable public key (CodeRabbit critical) before touching anything. CodeRabbit's import-identifier and loop-bound findings were already addressed in 0994291 (identifier persisted + bounded sweep). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
PR-scoped re-review of the current head: three regressions introduced by this change plus one unresolved legacy-restore migration blocker. I excluded the two adjacent issues that are clearly unchanged from the merge base.
| group.append(sorted[i]) | ||
| i += 1 | ||
| } | ||
| if group.count == 1 { |
There was a problem hiding this comment.
[P1] Do not restore ownership-unknown rows as wallet-owned
This behavior predates the PR, but remains an in-scope migration blocker because this change is meant to repair legacy links created by the old unconditional fallback. A sole unstamped mislink is still appended here; Rust puts every restore entry into wallet_identities, sets managed.wallet_id = Some(entry.wallet_id), and startup then sees a local identity and skips the index-0 discovery that could find the genuine identity. A later snapshot also returns the foreign row as wallet-owned. In a same-index group, two unstamped rows are resolved lexicographically, so a genuine legacy watch-only identity can be discarded instead. isLocal == false is not a quarantine from Rust wallet behavior; this needs an explicit ownership-unknown/out-of-wallet restore representation or durable provenance.
There was a problem hiding this comment.
Agreed on the analysis — an ownership-unknown row restored into wallet_identities occupies the index slot, suppresses index-0 discovery, and re-emits as owned; isLocal == false gates presentation, not Rust wallet behavior. I did not fold the fix into this PR, and want to flag the scope decision explicitly rather than decide it unilaterally:
The correct fix is the one IdentityRestoreEntryFFI's own doc already reserves — a dedicated out-of-wallet/ownership-unknown restore channel so Rust can hold these identities in out_of_wallet_identities (index-0 discovery still runs, later discovery/add_identity graduates them, snapshots don't claim them as owned). That is a cross-FFI change: new restore field/array in rs-platform-wallet-ffi, routing in build_wallet_identity_bucket's caller, JNI struct parity, regenerated headers and xcframework, plus Swift slice logic — a scope this PR's owner explicitly trimmed once already.
What this PR already guarantees meanwhile: no NEW mislinks can form (gated fallback + declared-owner reconciliation), existing scope mislinks unlink on re-emit, index collisions resolve to the stamped contender (so the genuine index-0 identity is not displaced when both rows are present), and unknown-ownership rows never present as Local or reach mutation surfaces. The residual — a SOLE legacy mislink occupying an index slot and suppressing that slot's discovery — predates the PR and does not worsen under it.
Proposal: track the out-of-wallet restore channel as a follow-up issue against rs-platform-wallet-ffi, referencing this thread. If you'd rather block this PR on it, say so and I'll build the FFI channel here.
| // both sides carry a canonical id, falling back to the | ||
| // string column (whose historical encodings vary) for | ||
| // rows persisted without the relationship. | ||
| let metaIdentityId = Data.identifier(fromBase58: meta.identityId) |
There was a problem hiding this comment.
[P1] Normalize legacy identity-id formats before tuple matching
The new tuple scoping fixes the wrong-row problem, but rejects valid metadata written by existing registration paths. IdentityKeyPersister stores the literal identityId: "pending"; Data.identifier(fromBase58:) accepts it as non-nil short data, so linked candidates compare a 32-byte identity with that short value and never take the string fallback. Other writers use identity.identityIdString, which is hex, while persister-created PersistentPublicKey.identityId values are Base58. Those rows likewise fail this match and remain unstamped, undermining the heal and collision winner. Accept only canonical 32-byte Base58 or hex IDs; for placeholder metadata, match the wallet ID + identity index + key ID + public key/path tuple.
There was a problem hiding this comment.
Verified and agreed — IdentityKeyPersister does write identityId: "pending", Data.identifier(fromBase58:) has no length check so it decodes placeholders to short garbage, and the row column has both base58 and hex writers. Fixed in 15f8562: a canonicalIdentityId() normalizer accepts only 32-byte base58 or hex; canonical metadata ids compare via the relationship (or the canonicalized string column for unlinked rows), and placeholder metadata corroborates the linked identity's identityIndex as you suggested, with the (publicKey, keyId) predicate plus the existing derivation-path self-check carrying the binding otherwise.
| /// at unlock and gates actual signing — this probe only gates the | ||
| /// persisted classification. | ||
| public enum WalletSigningCapability { | ||
| public static func probe(walletId: Data) -> Bool? { |
There was a problem hiding this comment.
[P2] Do not equate mnemonic-item presence with verified capability
This newly introduced probe promotes isLocal when a Keychain item merely exists, even if that mnemonic does not bind to the restored wallet. loadWalletList() runs the heal before loadFromPersistor() calls verifySeedBinding; a later seed-mismatch rejection is logged and leaves the wallet external-signable, but nothing reverses the persisted isLocal = true. The example app therefore exposes mutation controls that actual signing rejects. Feed verified seed-binding/per-identity signer capability into locality, or ensure a failed binding cannot leave the identity classified as Local.
There was a problem hiding this comment.
Verified and agreed — presence isn't binding. Fixed in 15f8562 in both directions: (1) the probe now requires the wallet row's seedBindingVerifiedMarker — persisted only after verifySeedBinding's full check bound the mnemonic to the wallet's account-0 xpub — so a present-but-never-verified mnemonic DEFERS promotion until the first successful unlock rather than promoting on existence; (2) a binding MISMATCH now actively repairs: verifySeedBinding's mismatch branch calls the new handleSeedBindingMismatch(walletId:), which clears the stale marker (blocking future promotions) and demotes the wallet's identities to whatever their imported key material still proves — the second sanctioned demotion path. A failed binding can no longer leave an identity classified Local.
| // the context back and surface the failure — | ||
| // re-running the load is safe (Keychain | ||
| // writes are idempotent upserts). | ||
| modelContext.rollback() |
There was a problem hiding this comment.
[P2] Do not roll back references after irreversible Keychain writes
The private and special keys above have already been stored in Keychain. This newly added rollback removes their SwiftData identifiers and the isLocal promotion, leaving inaccessible/orphaned secrets if the user does not manually retry. It is the same two-store failure mode the latest changes fixed in KeyDetailView and KeysListView. Keep the truthful model mutation for autosave/explicit retry, or compensate by deleting exactly every newly written Keychain item before rolling the context back.
There was a problem hiding this comment.
Agreed — same defect class as the flows fixed last round, introduced by my round-9 change here. Fixed in 15f8562: no rollback after the Keychain writes; the truthful rows are kept for autosave retry and the failure is surfaced without claiming success.
…ismatch demotion, no-rollback load Review round 10 on #4375: - [P1] backfillCore normalizes identity ids before tuple matching: canonicalIdentityId() accepts only 32-byte base58 OR hex (the two historical row encodings) and rejects placeholders — the registration-time IdentityKeyPersister writes identityId: "pending", which Data.identifier(fromBase58:) happily decodes to short garbage that could never match. Canonical metadata ids compare via the relationship (or the canonicalized string column); placeholder metadata corroborates the linked identity's index, with the (publicKey, keyId) predicate plus the derivation-path self-check carrying the binding otherwise. - [P2] WalletSigningCapability now requires a VERIFIED binding, not mere item presence: the probe takes the wallet row's seedBindingVerifiedMarker (persisted only after verifySeedBinding's full check bound the mnemonic to the wallet's account-0 xpub). Present-but-never-verified defers promotion until the first successful unlock. And a binding MISMATCH now actively repairs: the new handleSeedBindingMismatch(walletId:) clears the stale marker and demotes the wallet's identities to whatever their imported key material still proves — the second sanctioned demotion path, wired into verifySeedBinding's mismatch branch, so a failed binding can no longer leave identities classified Local. - [P2] LoadIdentityView no longer rolls back after irreversible Keychain writes (same two-store defect fixed for the forget/import flows last round): the truthful rows are kept for autosave retry and the failure is surfaced without claiming success. Restore of ownership-unknown rows (the remaining P1) is acknowledged as real and pre-existing; it needs the out-of-wallet restore channel the IdentityRestoreEntryFFI doc already reserves, which is a cross-FFI change proposed for a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Re-review of current head 15f85625: four P1 restore/locality correctness issues and two P2 capability-revocation issues remain. Canonical Base58/hex identity matching and the LoadIdentityView no-rollback fix are resolved.
| ) -> Bool? { | ||
| switch WalletStorage().mnemonicAvailability(for: walletId) { | ||
| case .present: | ||
| return verifiedBindingMarker != nil ? true : nil |
There was a problem hiding this comment.
[P1] Successful verification does not promote identities in this session. The startup locality heal runs before PlatformWalletManager.loadFromPersistor() verifies the seed. Successful verification then only saves the marker, so an upgraded seed-backed identity with derivation evidence remains isLocal == false until an unrelated upsert or the next launch. Freshly created wallets likewise do not establish this marker before identity persistence. Re-run the evidence-gated heal when verification writes the marker, and establish equivalent verified capability for resident newly created wallets.
There was a problem hiding this comment.
Verified and agreed — the heal-before-verify ordering left a session-long gap, and fresh wallets had no marker at all. Fixed in 0f185e8: setSeedBindingMarker now runs the evidence-gated promotion for the wallet it just verified (in the same serialized write), and createWallet mints the marker immediately after creation via a best-effort verifySeedBinding, so identities registered in the first session promote within their own persist round rather than deferring to the next launch.
| } | ||
| // The stamped contender wins the slot; with no (or every) | ||
| // stamp present, deterministic sort order decides. | ||
| let winner = group.first { walletKeyEvidence($0, walletId: walletId) } |
There was a problem hiding this comment.
[P1] An unstamped index collision can discard the genuine legacy identity. A real pre-breadcrumb watch-only identity and a fallback-era observed mislink can both be unstamped at index 0. Falling back to lexicographic group[0] can omit the real identity from Rust restore indefinitely. The migration needs an ownership-safe collision policy rather than an arbitrary tie-break.
There was a problem hiding this comment.
Agreed that lexicographic tie-break between two unstamped same-index rows is not ownership-safe — but no ownership-safe policy EXISTS at this layer: with zero evidence on either row, Swift cannot rank them, and restoring neither permanently excludes the genuine identity (the round-9 conclusion). The ownership-safe policy is exactly the out-of-wallet restore channel: restore BOTH as ownership-unknown, let index-0 discovery redeem the genuine one (add_identity replaces nothing because neither occupies the bucket), and let the mislink stay observed. That's the cross-FFI change proposed on the earlier thread — new restore representation in rs-platform-wallet-ffi, routing into out_of_wallet_identities, JNI parity, regenerated artifacts. I've asked the PR owner whether to build it here or track it as a follow-up; pending that decision, 0f185e8's evidence-gated promotion ensures neither contender can present as Local or reach mutation surfaces.
| i += 1 | ||
| } | ||
| if group.count == 1 { | ||
| result.append(group[0]) |
There was a problem hiding this comment.
[P1] A sole historical mislink is restored as wallet-owned. This admits every sole unstamped row. Rust then unconditionally sets managed.wallet_id = Some(entry.wallet_id), and a later Swift snapshot can promote the row using wallet-level seed capability without checking identity-level walletKeyEvidence. A seed binding proves the wallet, not ownership of this identity, so an old observed-identity mislink can become durably Local and mutation-capable.
There was a problem hiding this comment.
The specific harm you identify — a snapshot later promoting the restored mislink using wallet-level capability without identity-level evidence — is real and is now closed Swift-side in 0f185e8: promotion is identity-evidence-gated at EVERY writer. The upsert requires walletKeyEvidence in addition to the capability probe (a bare wallet_id declaration no longer promotes, precisely because restore can launder one), a fresh registration promotes via a new hook in persistIdentityKeys the moment its derivation stamps are written in the same round, and the heal/marker-write paths were already evidence-gated. A restored mislink has no stamped key rows and can never acquire them (its keys aren't derivable from this wallet), so it cannot become Local or mutation-capable through any snapshot, whatever Rust believes about its ownership. The remaining effect — the row occupying a wallet_identities index slot in Rust and suppressing that slot's discovery — is the part that needs the out-of-wallet restore channel from the sibling thread, where I've asked whether to build it in this PR or as a follow-up.
| if let linked = candidate.identity { | ||
| return linked.identityIndex == meta.identityIndex | ||
| } | ||
| return true |
There was a problem hiding this comment.
[P1] Placeholder backfill can stamp an orphaned replacement row. For identityId == "pending", any unlinked candidate is accepted and fetch-order .first decides. replacePublicKeysPreservingProvenance removes old keys from the relationship without deleting their SwiftData rows, so an orphan and the live linked key can share (publicKeyData, keyId). The backfill may stamp the orphan while the live identity remains without evidence. Prefer an unambiguous linked wallet/index candidate and explicitly delete or reject orphaned candidates.
There was a problem hiding this comment.
Verified and agreed — replacePublicKeysPreservingProvenance detached rows without deleting them (collection removal only detaches; .cascade fires on parent deletion), so orphans sharing (publicKeyData, keyId) with the live key were real. Fixed in 0f185e8 on both ends: the replace seam now DELETES replaced rows (their provenance was already copied forward), and the placeholder backfill arm prefers an index-corroborated LINKED candidate and refuses to fall through to unlinked rows whenever linked candidates exist — unlinked acceptance survives only for legacy stores with no linked candidate at all.
| row.publicKeys.contains { $0.privateKeyKeychainIdentifier != nil } | ||
| || row.votingPrivateKeyIdentifier != nil | ||
| || row.ownerPrivateKeyIdentifier != nil | ||
| || row.payoutPrivateKeyIdentifier != nil |
There was a problem hiding this comment.
[P2] Seed-mismatch demotion treats stale Keychain references as live signing material. PersistentIdentity.recomputeIsLocalAfterKeyRemoval() explicitly documents and handles identifiers outliving deleted Keychain items by probing the live items first. This new path checks only nonnil strings, so a wrong seed plus stale references leaves isLocal == true even when neither the seed nor an imported scalar can sign. Reuse the live tri-state capability checks before retaining Local status.
There was a problem hiding this comment.
Verified and agreed — the demotion checked identifier strings, exactly the staleness the recompute guards against. Fixed in 0f185e8: handleSeedBindingMismatch verifies each claimed scalar/special key against LIVE Keychain state via new strict nonisolated tri-state probes (identityScalarConfirmedPresent covering both storage schemes, specialKeyConfirmedPresent), keeping Local only for confirmed-present material and preserving on unanswerable probes.
| // verification marker (blocking future promotions) | ||
| // and demote each identity to whatever its imported | ||
| // key material still proves. | ||
| persistenceHandler?.handleSeedBindingMismatch(walletId: walletId) |
There was a problem hiding this comment.
[P2] Capability is revoked only for SeedMismatch. The earlier missing-mnemonic branch returns .watchOnly without clearing the old marker or recomputing identity locality, and deterministic non-SeedMismatch verification failures also bypass this handler. Because hasMnemonic collapses Keychain-unavailable into false, first distinguish .absent from transient .unavailable, then clear/demote on definitive seed loss or invalid binding state while preserving state only for transient failures.
There was a problem hiding this comment.
Verified and agreed on the absent/unavailable collapse. Fixed in 0f185e8: verifySeedBinding's early branch now switches on mnemonicAvailability — .absent is a definitive capability revocation (clears the marker and demotes via handleSeedBindingMismatch, same as an invalid binding) while .unavailable preserves all state as a transient failure. Deterministic non-SeedMismatch FFI failures still rethrow without demotion deliberately: they don't prove anything about the seed, and the demotion path requires proof.
…tion, definitive-absence revocation, in-session marker promotion Review round 11 on #4375, all six verified first: - [P1] Promotion is now identity-evidence-gated at EVERY writer, which closes the restore-launder loop Swift-side: a mislink restored as wallet-owned re-emits with wallet_id set, and only the absence of derivation-stamped key rows distinguishes it from a genuine owned identity. The upsert requires walletKeyEvidence + capability; a fresh registration promotes moments later in the same round when persistIdentityKeys writes the stamps (new promotion hook there); the heal and marker-write promotion cover everything else. A laundered mislink can therefore never become Local or mutation-capable, whatever Rust believes about its ownership. - [P1] Placeholder ('pending') backfill prefers an index-corroborated LINKED candidate and refuses to fall through to unlinked rows when linked candidates exist — replacement-detached orphans sharing (publicKeyData, keyId) with the live key can no longer soak up the stamp. replacePublicKeysPreservingProvenance now DELETES replaced rows (collection removal only detaches; cascade fires on parent deletion), removing the orphan source entirely. - [P1] Successful verification now promotes in-session: setSeedBindingMarker runs the evidence-gated promotion for the wallet it just verified (the startup heal ran before verification), and createWallet mints the marker immediately so identities registered in the first session don't defer to the next launch. - [P2] handleSeedBindingMismatch verifies claimed key material against LIVE Keychain state via new strict tri-state probes (identityScalarConfirmedPresent / specialKeyConfirmedPresent) — stale identifiers no longer keep a row Local after a wrong seed; unanswerable probes preserve rather than conclude. - [P2] verifySeedBinding distinguishes definitive seed ABSENCE (revokes: clears marker + demotes, wired like a mismatch) from a transient Keychain failure (preserves everything), instead of collapsing both into watch-only with stale Local state. The two restore-channel P1s (sole mislink restored as owned; unstamped index-collision tie-break) remain tracked for the out-of-wallet restore channel — the Swift-side laundering hardening above removes their Local/mutation impact meanwhile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Re-review of current head 0f185e89: Swift CI is build-blocked, the fresh-wallet marker path still runs before mnemonic persistence, and four migration/capability issues remain. The restored-wallet marker promotion, live-probed demotion, confirmed-absence revocation, and identity-level promotion evidence are fixed.
| // their `isLocal` promotion until the next launch's unlock runs | ||
| // the verification. Best-effort: a failure only delays promotion, | ||
| // it can't mis-classify. | ||
| try? verifySeedBinding(w) |
There was a problem hiding this comment.
[P1] Swift CI does not compile. This try? expression and the duplicate at line 545 produce result of 'try?' is unused under the repository's warnings-as-errors policy. The current Swift job fails on these exact diagnostics. Explicitly consume or handle the result.
| } | ||
| let w = ManagedPlatformWallet(handle: walletHandle, walletId: idData) | ||
| self.wallets[idData] = w | ||
| // Establish verified signing capability for the fresh wallet now |
There was a problem hiding this comment.
[P1] Fresh-wallet verification still runs before the mnemonic exists in Keychain. CreateWalletView and WalletDetailView store the mnemonic only after createWallet returns, so this verification sees .absent and mints no marker. The paired key callback then leaves an identity registered during this session isLocal == false, hiding mutation UI until relaunch. Trigger verification after successful mnemonic storage (or make mnemonic persistence and verification part of this operation).
| // The stamped contender wins the slot; with no (or every) | ||
| // stamp present, deterministic sort order decides. | ||
| let winner = group.first { walletKeyEvidence($0, walletId: walletId) } | ||
| ?? group[0] |
There was a problem hiding this comment.
[P1] An unstamped index collision still chooses arbitrarily. A genuine pre-breadcrumb/watch-only identity and a fallback-era observed mislink can both be unstamped at index 0. Falling back to lexicographic group[0] can omit the genuine identity permanently from Rust restore.
| i += 1 | ||
| } | ||
| if group.count == 1 { | ||
| result.append(group[0]) |
There was a problem hiding this comment.
[P1] A sole historical index-0 mislink still suppresses discovery of the real identity. Restoring this ownership-unknown row inserts it into Rust's wallet bucket at index 0. highest_registration_index then makes discovery resume at 1, so the genuine wallet identity at index 0 is never probed and the mislink cannot self-heal. Unknown restored rows must not advance the discovery watermark, or index 0 must be explicitly reprobed.
| let candidates = (try? backgroundContext.fetch(descriptor)) ?? [] | ||
| let selected: PersistentPublicKey? | ||
| if let metaIdentityId { | ||
| selected = candidates.first { candidate in |
There was a problem hiding this comment.
[P1] Historical detached key rows remain unrecoverable. The placeholder branch now prefers linked rows, but canonical-ID matching can still choose an unlinked orphan before the live linked key, and the sole-unlinked fallback stamps an orphan without relinking it. persistIdentityKeys likewise takes an arbitrary existing (keyId, identityId) row and assigns the parent only on insertion, so later emissions can keep updating the orphan without ever giving the identity ownership evidence. Prefer the target-linked row, safely relink a sole orphan, and delete duplicates.
| // verification marker (blocking future promotions) | ||
| // and demote each identity to whatever its imported | ||
| // key material still proves. | ||
| persistenceHandler?.handleSeedBindingMismatch(walletId: walletId) |
There was a problem hiding this comment.
[P2] Deterministic invalid seed material still retains stale capability. Confirmed mnemonic absence now revokes correctly, but this catch revokes only .invalidParameter/SeedMismatch. Rust maps malformed UTF-8/BIP39, invalid length, and deterministic derivation failures to WalletOperation, so a corrupt or replaced mnemonic retains its old marker and Local UI indefinitely although signing cannot work. Distinguish definitive credential errors from transient Keychain/resolver failures and revoke the definitive class.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Preliminary Codex verification confirms five blocking defects and one suggestion. The current Swift build fails under the repository's warnings-as-errors Xcode invocation, while fresh-wallet capability initialization, legacy ownership restoration, key-row reconciliation, and Keychain-unavailability handling can still leave persisted identity state incorrect.
Source: Codex reviewers gpt-5.6-sol (general) and gpt-5.6-sol (ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 1 suggestion(s)
1 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/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:489: Consume the optional verification result so Swift CI compiles
`build_ios.sh` builds the example app with both `OTHER_SWIFT_FLAGS="-warnings-as-errors"` and `SWIFT_TREAT_WARNINGS_AS_ERRORS=YES`. Swift emits `result of 'try?' is unused` for this bare expression even though `verifySeedBinding` is `@discardableResult`, so the warning becomes a build error. Explicitly discard the optional result here and at the duplicate call on line 545.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:483-489: Verify fresh wallets only after their signing source is available
`CreateWalletView` and `WalletDetailView` call `storeMnemonic` only after `createWallet` returns. This verification therefore observes definitive mnemonic absence, clears any marker, and returns `.watchOnly`; storing the mnemonic afterward does not trigger another verification. Later identity-key persistence requires the missing verified-binding marker before promoting `isLocal`, so identities registered during the session remain Observed until a later unlock or relaunch. The raw-seed overload has the same structural problem because it probes mnemonic-backed capability even though its resident seed may never be stored as a mnemonic. Persist and verify the signing source atomically, or expose a post-persistence verification operation and require creation callers to invoke it before identity persistence.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:882-892: Revoke stale capability for definitive credential failures
This catch revokes capability only for `.invalidParameter`, the FFI code used for `SeedMismatch`. Deterministic credential failures—including invalid UTF-8, invalid BIP-39 words or checksum, invalid mnemonic length, and a restored wallet missing BIP44 account 0—are converted to `InvalidIdentityData` and flattened by `platform_wallet_verify_seed_binds_to_wallet_cached` into `ErrorWalletOperation`. The stale marker consequently survives. `WalletSigningCapability.probe` checks only mnemonic presence plus a non-nil marker, not whether the marker's stored Keychain stamp still matches, so the identity can remain Local although all signing attempts deterministically fail. Add a distinct FFI classification for definitive credential failures and invoke the existing revocation/demotion path for that class while preserving state for transient resolver or Keychain errors.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3066-3074: Prefer and relink the live key row during breadcrumb recovery
For canonical identity metadata, this selects the first matching candidate without preferring the key row currently linked to the target identity. Older refresh paths could detach a key row without deleting it, leaving an orphan and a live linked replacement with the same identity, key ID, and public-key bytes. If fetch order returns the orphan first, the backfill stamps the orphan while `PersistentIdentity.publicKeys` remains without the ownership evidence used by healing and restoration. `persistIdentityKeys` has the same recovery gap at lines 2200-2232: it fetches an arbitrary existing `(identityId, keyId)` row and assigns the parent only when inserting a new row. Prefer an already-linked target row, safely relink a sole orphan, and delete duplicate detached rows so both persistence paths update the live relationship.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5034-5041: Startup heal promotes historically mislinked observed identities to wallet-owned
(existing thread: https://github.com/dashpay/platform/pull/4375#discussion_r3760295050)
The evidence gate now prevents an unstamped legacy mislink from becoming `isLocal`, but `restorableIdentities` still appends every sole unstamped row and arbitrarily chooses `group[0]` when all same-index contenders are unstamped. Every emitted `IdentityRestoreEntryFFI` is defined as wallet-owned: Rust sets `managed.wallet_id = Some(entry.wallet_id)` and inserts it into `wallet_identities` at the persisted index. A sole fallback-era mislink at index 0 therefore makes `highest_registration_index` resume discovery at 1, preventing discovery of the genuine index-0 identity. If a genuine pre-breadcrumb/watch-only row collides with a mislink, the lexicographic tie-break can instead discard the genuine row. Ownership-unknown rows need a distinct restore representation routed to `out_of_wallet_identities`, where they neither claim wallet ownership nor advance discovery.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swift:388-437: Do not erase key references when Keychain availability is unknown
`hasPrivateKey`, `hasIdentityPrivateKey`, and `hasSpecialKey` collapse every non-success Keychain status into `false`. A temporarily locked or inaccessible Keychain therefore causes this method to clear still-valid public and special-key references. If the subsequent wallet capability probe returns `nil`, the method returns while claiming to preserve the prior classification, but those reference mutations have already happened and the caller can save them. For a walletless identity it also recomputes `isLocal` from the falsely cleared references. Use the existing strict tri-state probes, stage every result before mutating the model, and leave both references and `isLocal` unchanged if any required lookup is unanswerable.
| // their `isLocal` promotion until the next launch's unlock runs | ||
| // the verification. Best-effort: a failure only delays promotion, | ||
| // it can't mis-classify. | ||
| try? verifySeedBinding(w) |
There was a problem hiding this comment.
🔴 Blocking: Consume the optional verification result so Swift CI compiles
build_ios.sh builds the example app with both OTHER_SWIFT_FLAGS="-warnings-as-errors" and SWIFT_TREAT_WARNINGS_AS_ERRORS=YES. Swift emits result of 'try?' is unused for this bare expression even though verifySeedBinding is @discardableResult, so the warning becomes a build error. Explicitly discard the optional result here and at the duplicate call on line 545.
| try? verifySeedBinding(w) | |
| _ = try? verifySeedBinding(w) |
source: ['codex']
There was a problem hiding this comment.
Fixed in b27eec3 — both call sites now discard explicitly (_ = try?); the warnings-as-errors CI job compiles again.
🤖 Addressed by Claude Code
| // Establish verified signing capability for the fresh wallet now | ||
| // (minting the seed-binding marker when a Keychain mnemonic backs | ||
| // it) — identities registered this session would otherwise defer | ||
| // their `isLocal` promotion until the next launch's unlock runs | ||
| // the verification. Best-effort: a failure only delays promotion, | ||
| // it can't mis-classify. | ||
| try? verifySeedBinding(w) |
There was a problem hiding this comment.
🔴 Blocking: Verify fresh wallets only after their signing source is available
CreateWalletView and WalletDetailView call storeMnemonic only after createWallet returns. This verification therefore observes definitive mnemonic absence, clears any marker, and returns .watchOnly; storing the mnemonic afterward does not trigger another verification. Later identity-key persistence requires the missing verified-binding marker before promoting isLocal, so identities registered during the session remain Observed until a later unlock or relaunch. The raw-seed overload has the same structural problem because it probes mnemonic-backed capability even though its resident seed may never be stored as a mnemonic. Persist and verify the signing source atomically, or expose a post-persistence verification operation and require creation callers to invoke it before identity persistence.
source: ['codex']
There was a problem hiding this comment.
Addressed in 149f083 by REMOVING the premature verification rather than reordering it: the round-11 createWallet verify ran before creation callers store the mnemonic, so it observed definitive absence and — with the absence branch now revoking — wrongfully demoted brand-new wallets. Fresh wallets return to deferring first-session promotion to the first unlock's verification. A dedicated post-persistence verification hook (your 'persist and verify atomically' option) is deliberately NOT added in this round — the PR's remaining capability-lifecycle scope is under an explicit hold while the owner decides between trimming this PR back to the core isLocal fix or continuing the hardening series.
🤖 Addressed by Claude Code
| let candidates = (try? backgroundContext.fetch(descriptor)) ?? [] | ||
| let selected: PersistentPublicKey? | ||
| if let metaIdentityId { | ||
| selected = candidates.first { candidate in | ||
| if let linked = candidate.identity { | ||
| return linked.identityId == metaIdentityId | ||
| } | ||
| return Self.canonicalIdentityId(candidate.identityId) == metaIdentityId | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Prefer and relink the live key row during breadcrumb recovery
For canonical identity metadata, this selects the first matching candidate without preferring the key row currently linked to the target identity. Older refresh paths could detach a key row without deleting it, leaving an orphan and a live linked replacement with the same identity, key ID, and public-key bytes. If fetch order returns the orphan first, the backfill stamps the orphan while PersistentIdentity.publicKeys remains without the ownership evidence used by healing and restoration. persistIdentityKeys has the same recovery gap at lines 2200-2232: it fetches an arbitrary existing (identityId, keyId) row and assigns the parent only when inserting a new row. Prefer an already-linked target row, safely relink a sole orphan, and delete duplicate detached rows so both persistence paths update the live relationship.
source: ['codex']
| public func recomputeIsLocalAfterKeyRemoval() { | ||
| let keychain = KeychainManager.shared | ||
| for key in publicKeys where key.privateKeyKeychainIdentifier != nil { | ||
| let alive = keychain.hasPrivateKey( | ||
| identityId: identityId, | ||
| keyIndex: key.keyId | ||
| ) || keychain.hasIdentityPrivateKey( | ||
| publicKeyHex: key.publicKeyData.toHexString() | ||
| ) | ||
| if !alive { | ||
| key.privateKeyKeychainIdentifier = nil | ||
| } | ||
| } | ||
| if votingPrivateKeyIdentifier != nil, | ||
| !keychain.hasSpecialKey(identityId: identityId, keyType: .voting) { | ||
| votingPrivateKeyIdentifier = nil | ||
| } | ||
| if ownerPrivateKeyIdentifier != nil, | ||
| !keychain.hasSpecialKey(identityId: identityId, keyType: .owner) { | ||
| ownerPrivateKeyIdentifier = nil | ||
| } | ||
| if payoutPrivateKeyIdentifier != nil, | ||
| !keychain.hasSpecialKey(identityId: identityId, keyType: .payout) { | ||
| payoutPrivateKeyIdentifier = nil | ||
| } | ||
| // The wallet arm uses the SAME capability resolver as the | ||
| // persister's promotion gate — current signing material, not | ||
| // derivation evidence. A stamped key proves the wallet ONCE | ||
| // derived this identity's keys (ownership), not that it can | ||
| // sign today: a watch-only linked identity whose one imported | ||
| // scalar is being forgotten must come out non-local even | ||
| // though its stamps remain. | ||
| var walletArm = false | ||
| if let ownerWallet = wallet { | ||
| switch WalletSigningCapability.probe( | ||
| walletId: ownerWallet.walletId, | ||
| verifiedBindingMarker: ownerWallet.seedBindingVerifiedMarker | ||
| ) { | ||
| case .some(true): | ||
| walletArm = true | ||
| case .some(false): | ||
| walletArm = false | ||
| case .none: | ||
| // The Keychain couldn't answer (or the binding was | ||
| // never verified) — every arm of this recompute | ||
| // depends on Keychain truth, so deciding against | ||
| // unknowns risks persisting a wrong classification. | ||
| // Leave the flag as it stands; a later pass (or the | ||
| // startup heal) decides. | ||
| return |
There was a problem hiding this comment.
🔴 Blocking: Do not erase key references when Keychain availability is unknown
hasPrivateKey, hasIdentityPrivateKey, and hasSpecialKey collapse every non-success Keychain status into false. A temporarily locked or inaccessible Keychain therefore causes this method to clear still-valid public and special-key references. If the subsequent wallet capability probe returns nil, the method returns while claiming to preserve the prior classification, but those reference mutations have already happened and the caller can save them. For a walletless identity it also recomputes isLocal from the falsely cleared references. Use the existing strict tri-state probes, stage every result before mutating the model, and leave both references and isLocal unchanged if any required lookup is unanswerable.
source: ['codex']
| } catch let error as PlatformWalletError { | ||
| if case .invalidParameter = error { | ||
| setDashPaySeedMismatch(walletId, true) | ||
| // A failed binding must not leave this wallet's | ||
| // identities classified as Local: clear the stale | ||
| // verification marker (blocking future promotions) | ||
| // and demote each identity to whatever its imported | ||
| // key material still proves. | ||
| persistenceHandler?.handleSeedBindingMismatch(walletId: walletId) | ||
| } | ||
| throw error |
There was a problem hiding this comment.
🟡 Suggestion: Revoke stale capability for definitive credential failures
This catch revokes capability only for .invalidParameter, the FFI code used for SeedMismatch. Deterministic credential failures—including invalid UTF-8, invalid BIP-39 words or checksum, invalid mnemonic length, and a restored wallet missing BIP44 account 0—are converted to InvalidIdentityData and flattened by platform_wallet_verify_seed_binds_to_wallet_cached into ErrorWalletOperation. The stale marker consequently survives. WalletSigningCapability.probe checks only mnemonic presence plus a non-nil marker, not whether the marker's stored Keychain stamp still matches, so the identity can remain Local although all signing attempts deterministically fail. Add a distinct FFI classification for definitive credential failures and invoke the existing revocation/demotion path for that class while preserving state for transient resolver or Keychain errors.
source: ['codex']
… fix observed-entry mislinking
Owner-decided semantics for PersistentIdentity.isLocal: true for every
identity that is YOURS or deliberately tracked on this device —
wallet-derived identities always ('things from the wallet should
always be local'), manual adds (LoadIdentityView by id/name) always —
and false only for incidental rows (observed foreign identities
materialized by sync). Promote-only: no sync path ever writes false
over true.
The field bug: the persister wrote a constant isLocal: false for
every row it created and nothing promoted, so the wallet's own
identity (correct wallet relationship, identityIndex 0) showed as
not-local on a mainnet device, hiding the identity-key refresh
affordances in dashwallet-ios. LoadIdentityView additionally
clobbered its own manual adds with false.
- persistIdentities promotes isLocal when it attaches the wallet
relationship; observed rows stay false; existing rows are never
demoted (a manual mark survives sync flowing over the row).
- Wallet-relationship hygiene, fixed alongside because the same
audit exposed it: the scope-wallet fallback is gated on
identity_index != nil, so out-of-wallet (observed) entries no
longer get mislinked to whatever wallet's changeset carried them;
a fabricated scope link is cleared when the identity re-emits
observed; a link matching a declared-but-unresolvable owner
survives, while one contradicting the declaration is cleared;
another wallet's valid relationship always survives a foreign
manager's observed emission.
- loadWalletList() runs a one-shot promote-only heal (wallet-linked
&& !isLocal → true) for stores written by the constant-false era.
- LoadIdentityView marks its rows isLocal: true (manual add) instead
of erasing the very provenance the flag records.
- Example app: retire the dead 'Local Only / On Network' badge
reading; the rare incidental rows show 'Observed'; the
!isLocal network-feature gates (DPNS/tokens/profile/refresh) are
removed — under the real semantics they would have hidden those
features for every wallet identity; wallet-signing actions
(register name, marketplace) gate on hasLoadedWallet.
isLocal deliberately makes NO claim about signing capability;
consumers that need capability compute it live, and wallet-owned
filtering has walletOwnedIdentitiesPredicate. This supersedes the
capability-oriented hardening series, archived unmerged on
archive/islocal-review-hardening.
Covered by IdentityIsLocalPersistenceTests: promotion (direct +
index-fallback), observed rows stay incidental and unlinked,
sync never demotes manual adds, declared-owner keep/clear,
cross-wallet preservation, and the startup heal. 335 tests pass on
simulator; the two IdentityResolverSignIntegrationTests
keychain-entitlement failures pre-exist on the baseline.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
149f083 to
b9c7399
Compare
QuantumExplorer
changed the title
The SDK now defines isLocal as 'this identity is yours or deliberately tracked here': wallet-derived identities are always local, manual adds are local, and only incidental (observed) rows are false — with the persister promoting on wallet linkage and a startup heal for the constant-false era (dashpay/platform#4375). Update the three app-side readers written against the old dead 'Local Only / On Network' badge reading: - Identities list badge: the orange 'Local Only' badge (which would now appear on every wallet identity) becomes an 'Observed' badge on the rare incidental rows. - Identity detail sheet: the always-on Status row is replaced by an 'Observed' row shown only for incidental rows — the Wallet row already names the owner otherwise. - refreshFromNetwork: drop the '!row.isLocal' filter, which would have skipped exactly the wallet's own identities. The key-refresh gates stay on the wallet relationship (#981) — that operation needs the wallet's DIP-9 tree specifically, which isLocal deliberately does not claim. Land/pull dashpay/platform#4375 first: until then rows still carry the constant false and every identity shows the 'Observed' badge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#983) The SDK now defines isLocal as 'this identity is yours or deliberately tracked here': wallet-derived identities are always local, manual adds are local, and only incidental (observed) rows are false — with the persister promoting on wallet linkage and a startup heal for the constant-false era (dashpay/platform#4375). Update the three app-side readers written against the old dead 'Local Only / On Network' badge reading: - Identities list badge: the orange 'Local Only' badge (which would now appear on every wallet identity) becomes an 'Observed' badge on the rare incidental rows. - Identity detail sheet: the always-on Status row is replaced by an 'Observed' row shown only for incidental rows — the Wallet row already names the owner otherwise. - refreshFromNetwork: drop the '!row.isLocal' filter, which would have skipped exactly the wallet's own identities. The key-refresh gates stay on the wallet relationship (#981) — that operation needs the wallet's DIP-9 tree specifically, which isLocal deliberately does not claim. Land/pull dashpay/platform#4375 first: until then rows still carry the constant false and every identity shows the 'Observed' badge. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Semantics (owner-decided)
PersistentIdentity.isLocalmeans this identity is yours or deliberately tracked on this device:trueis also the initializer default: a directly-constructed row is a manual add);falseonly for incidental rows — observed foreign identities materialized by sync that nobody asked to track.Promote-only: no sync path ever writes
falseovertrue. The flag makes no claim about signing capability — consumers that need capability compute it live; wallet-owned filtering haswalletOwnedIdentitiesPredicate. Reviews proposing capability/evidence gating of this flag are out of scope by owner decision.Issue being fixed
The persister wrote a constant
isLocal: falsefor every row it created and nothing promoted, so the wallet's own identity (correct wallet relationship,identityIndex0) showed as not-local on a real mainnet device — hiding the identity-key refresh affordances in dashwallet-ios (see dashwallet-ios #981). LoadIdentityView additionally clobbered its own manual adds withfalse.What was done
One commit, no schema change:
persistIdentitiespromotesisLocalon wallet linkage; observed rows stayfalse; existing rows are never demoted.identity_index != nilso observed entries are never mislinked to the changeset's scope wallet; a fabricated scope link clears when its identity re-emits observed; a link matching a declared-but-unresolvable owner survives while one contradicting the declaration clears; another wallet's valid relationship always survives a foreign manager's observed emission.loadWalletList()runs a one-shot promote-only heal for constant-false-era stores.true.!isLocalnetwork-feature gates are removed (they'd hide DPNS/tokens/profile/refresh for every wallet identity under the real semantics); wallet-signing actions gate onhasLoadedWallet.History note
An extensive capability-oriented hardening series (evidence-gated promotion, seed-binding markers, tri-state keychain probes) was built here across 12 review rounds and then superseded by the owner's semantics decision — archived unmerged on
archive/islocal-review-hardening. Genuinely pre-existing defects discovered during that series (forget-key dual-scheme deletion, KeyDetail import dropping its identifier, the out-of-wallet restore channel reserved byIdentityRestoreEntryFFI's doc, FFI error taxonomy for credential failures) should be tracked as separate issues, not re-litigated on this PR.How Has This Been Tested
IdentityIsLocalPersistenceTests(in-memory store through the real persister bridge): promotion (direct + index-fallback), observed rows stay incidental and unlinked, sync never demotes manual adds, declared-owner keep/clear, cross-wallet preservation, startup heal. 335 tests pass on simulator; the twoIdentityResolverSignIntegrationTestskeychain-entitlement failures pre-exist on the baseline.SwiftExampleAppbuilds clean under warnings-as-errors.Companion dashwallet-ios PR #983 aligns the app's badge copy and balance-refresh loop.
🤖 Generated with Claude Code