fix(platform): derive identity-rescan keys through the wallet signer#3860
Conversation
App wallets load into the in-process WalletManager as key_wallet::WalletType::ExternalSignable (the seed lives in iOS Keychain, not in process). The "Re-scan for Identities" flow derived DIP-9 identity-auth keys directly from that in-memory wallet, which has no private key, so the scan and its key preview both failed immediately with "External signable wallet has no private key" and discovered nothing. Mirror the mechanism identity registration already uses: resolve the wallet's mnemonic on demand through a Swift-owned MnemonicResolver (Keychain-backed), build a master ExtendedPrivKey, and derive the scan / preview keys from that master. platform-wallet: - identity_handle.rs: add derive_identity_auth_key_hash_from_master, a master-xpriv sibling of derive_identity_auth_key_hash that goes through derive_ecdsa_identity_auth_keypair_from_master + ripemd160_sha256, so the rescan and registration derivations can never drift. - discovery.rs: factor the gap-limit scan into a shared discover_inner with a pluggable KeyHashSource (resident wallet vs. supplied master) and add the public discover_from_master; discover() behavior for key-resident wallets is unchanged. The per-index read lock is now only taken on the wallet-internal derive path. - unit tests pin the fix: master-based hash equals the resident-wallet hash on a Mnemonic wallet, the resident derive errors on an ExternalSignable wallet while the master derive succeeds, and the master hash matches the registration keypair's pubkey hash. platform-wallet-ffi: - platform_wallet_discover_identities and platform_wallet_preview_identity_registration_keys take a MnemonicResolverHandle. When non-null they resolve the mnemonic keyed by the wallet handle's own wallet_id, build the master, and derive via discover_from_master / derive_ecdsa_identity_auth_keypair_from_master; when null they keep the resident-wallet behavior. Resolver failures map to distinct NOT_FOUND / BUFFER_TOO_SMALL / other messages, and the resolve→master sequence is shared in resolve_master_from_resolver so both entry points stay in lockstep. Mnemonic/seed live in Zeroizing buffers; the master's scalar is non_secure_erase'd before return. swift-sdk: - ManagedPlatformWallet.discoverIdentities / previewIdentityRegistrationKeys construct a MnemonicResolver (default WalletStorage, overridable for tests) and pass its handle to the updated FFI. discoverIdentities runs the FFI inside Task.detached, so the resolver is captured and wrapped in withExtendedLifetime to keep ARC holding it for the call. Public signatures stay source-compatible (defaulted params only); the example app's SearchWalletsForIdentitiesView compiles unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 3 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR enables identity discovery and registration-key preview for watch-only and external-signable wallets by adding a mnemonic-resolver-based master-key derivation path that branches at the FFI boundary and core wallet layer, supporting both resident-key and resolver-backed wallet capabilities. ChangesIdentity Discovery and Key Preview for External-Signable Wallets
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related Issues
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 74eeb25) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR is well-scoped: it routes identity rescan/preview key derivation through the existing Swift-owned mnemonic resolver so iOS Keychain-backed ExternalSignable wallets can derive DIP-9 identity-auth keys without an in-process seed. One blocking issue: the new synchronous preview Swift wrapper does not extend the MnemonicResolver's lifetime across the FFI call (the discovery path does), creating a real use-after-free window because the Rust ctx is passUnretained. Two convergent suggestions worth addressing: the preview's resident-wallet branch now takes the wallet_manager read lock per row instead of once, and the Swift wrapper unconditionally forces every wallet through the resolver — a regression for resident-Mnemonic wallets created with createWallet(seed:) or without a Keychain-stored mnemonic.
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:791-798: Keep mnemonic resolver alive across the preview FFI call
`MnemonicResolver` builds its Rust callback context with `Unmanaged.passUnretained(self)` and frees the underlying handle in `deinit` — Rust only ever sees a raw pointer back to the Swift object, so ARC must keep the resolver alive for the whole synchronous FFI call. In this new preview path, the last Swift-level use of `resolver` is just evaluating `resolver.handle` as a parameter, after which ARC is free to end the object's lifetime. If the resolver is released before or during the call, Rust dereferences a freed `MnemonicResolverHandle` (or invokes the destroyed callback context inside `resolve_master_from_resolver`), causing FFI use-after-free / crash / potential memory corruption on a user-triggered preview. The async `discoverIdentities` path in this same PR already wraps the equivalent call in `withExtendedLifetime(resolver)` for exactly this reason; the synchronous preview needs the same guard.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:1199-1210: Swift wrapper forces every wallet through the resolver, regressing resident-Mnemonic flows
The Rust FFI deliberately keeps `mnemonic_resolver_handle` nullable so key-resident wallets keep the historical resident-wallet derivation. Both Swift wrappers (`discoverIdentities` and `previewIdentityRegistrationKeys`) unconditionally construct a `MnemonicResolver` and pass `resolver.handle`, so the null-handle fallback is unreachable from Swift. Concrete regressions: `PlatformWalletManager.createWallet(seed:)` (no BIP-39 mnemonic exists to resolve) and `createWallet(mnemonic:)` callers that do not separately persist the mnemonic via `WalletStorage` — discovery used to derive from the in-process wallet and now fails with resolver `NOT_FOUND`. If the design intent really is "iOS Keychain-backed only," surface that intent by either documenting the new precondition, falling back to a null resolver when no mnemonic is stored, or offering an explicit resident-wallet variant. Otherwise the public API silently loses a previously-working path.
In `packages/rs-platform-wallet-ffi/src/identity_key_preview.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_key_preview.rs:288-312: Resident-wallet preview path acquires WalletManager read lock per row
Before this PR, `platform_wallet_preview_identity_registration_keys` took `wallet.wallet_manager().blocking_read()` and resolved `key_wallet` once at the top of the closure, then reused them for every preview row. After the refactor, the resident-wallet branch of `derive_material` runs per iteration and re-acquires `blocking_read()` plus re-looks-up `get_wallet(&wallet.wallet_id())` on every row. The resolver path correctly hoists the master xpriv once outside the loop, so the resident path is now strictly worse than the original with no upside on the resolver side. Hoist the resident-path lock acquisition (and the `get_wallet` lookup) before the loop — matching how `master_opt` is resolved once — and let `derive_material` consume a borrowed `&Wallet`/`KeyHashSource`-shaped enum per row. Bonus: keeps the two paths symmetric in shape.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3860 +/- ##
=============================================
- Coverage 87.15% 70.73% -16.43%
=============================================
Files 2641 20 -2621
Lines 327793 2788 -325005
=============================================
- Hits 285701 1972 -283729
+ Misses 42092 816 -41276
🚀 New features to boost your workflow:
|
Follow-up review fixes for the identity rescan/preview DIP-9 key derivation in PR #3860: - Swift preview wrapper: pin the MnemonicResolver across the synchronous FFI call with withExtendedLifetime. Its Rust ctx is passUnretained and the handle is freed in deinit, so without pinning ARC could deallocate it at the resolver.handle argument (last Swift use) while Rust is mid-call — a use-after-free. Mirrors discoverIdentities. - Preview FFI: hoist the resident-path wallet-manager read guard and get_wallet lookup out of the per-row loop (they were re-acquired every row). The guard is now taken once and held across the pure-compute loop, matching pre-PR behavior; the per-row derive consumes a borrowed key source so the resolver and resident paths are symmetric in shape. - Both FFI entry points: select the derivation source by wallet capability instead of forcing the resolver. If the in-process wallet holds resident private keys (not external-signable, not watch-only), use the historical in-process derive and never touch the resolver (also skipping a pointless iOS Keychain read); otherwise consult the resolver, returning an enriched error when it's null for an external-signable/watch-only wallet. This restores raw-seed / createWallet(seed:) wallets, whose mnemonic may never have been persisted to WalletStorage. Module/function and Swift doc comments updated to describe the capability-based selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All three prior findings (preview FFI resolver lifetime, per-row WalletManager lock in resident preview, Swift forcing the resolver path on resident wallets) are FIXED on head 2a7d096. The capability-based dispatch in Rust (is_external_signable || is_watch_only) is the correct invariant, and resident Mnemonic/Seed/ExtendedPrivKey wallets now skip the resolver entirely. One new minor finding: the preview path holds the wallet-manager read guard across the Swift mnemonic-resolver callback (and the full row loop), unlike the discovery path which deliberately scopes the guard to the capability check.
🟡 1 suggestion(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/rs-platform-wallet-ffi/src/identity_key_preview.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/identity_key_preview.rs:353-447: Preview path holds WalletManager read guard across the Swift resolver callback
The preview closure acquires `wm = wallet.wallet_manager().blocking_read()` at line 353 and only releases it at the closure's end (line 446). For the external-signable branch this means the guard is held across `resolve_master_from_resolver(...)` at line 391 — a synchronous re-entry into Swift that performs an iOS Keychain read (and can stall on biometric unlock) — and across the entire per-row derive loop. The sibling discovery path in `identity_discovery.rs:165-178` deliberately scopes the read guard to a tight block that only inspects the wallet's shape (`is_external_signable` / `is_watch_only`), drops it before calling the resolver, and never crosses `block_on_worker`. Only the resident branch genuinely needs the guard for the loop's duration (it borrows `&Wallet`); the master branch does not need the guard past the capability check and the `wallet_id` / `network` reads. Today no deadlock occurs because the Swift resolver does not touch the wallet manager, but the wider hold window blocks concurrent wallet-manager writers behind a synchronous Keychain round-trip and introduces a non-obvious cross-FFI reentrancy invariant that the discovery path already avoids. Mirror the discovery shape: capture `wallet_has_resident_keys`, `network`, and `wallet_id` under the short guard, drop `wm`, then resolve the master and either re-acquire the guard for the resident branch's loop or hand the master to the loop directly.
…heck `platform_wallet_preview_identity_registration_keys` held the wallet-manager read guard across the synchronous Swift resolver callback (`resolve_master_from_resolver`, an iOS Keychain read that can stall on biometric unlock) and the whole derive loop, because the `DeriveSource::Resident` borrow kept it alive. Restructure to two-phase locking, mirroring the discovery path: a short guard scoped to the capability check is dropped before any resolver interaction; only the resident branch re-acquires the guard, and only for the loop's duration. The master branch holds no guard past the capability check. Zeroization is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental + cumulative verification at head 74eeb25. The prior locking concern is FIXED: the preview path now implements two-phase locking — a short capability-check read guard (lines 370-383) is dropped before the Swift mnemonic resolver call (lines 408-410), and the read guard is only re-acquired for the resident-derive loop (lines 425-439), matching the discovery-path invariant. No new in-scope defects identified across the FFI, Rust quality, security, and FFI-engineer perspectives. The codex-security-auditor finding about an empty SetPrices schedule panic touches an unmodified file (rs-drive token direct-purchase transformer) and is unrelated to this PR's identity-rescan/preview scope, so it is dropped.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "4f0099d7859e85299c34d6292310c4075a2087392e23d9ae19e31f7f9105a48c"
)Xcode manual integration:
|
2 participants
Issue being fixed or feature implemented
SwiftExampleApp → Identities → Add → "Re-scan for Identities" → pick a mnemonic-restored wallet → "Re-scan Wallet" failed immediately with:
and discovered 0 identities.
App wallets are loaded into the in-process
WalletManageraskey_wallet::WalletType::ExternalSignable— the seed lives in iOS Keychain, not in process. Both the discovery scan (IdentityWallet::discover) and its key preview (platform_wallet_preview_identity_registration_keys) derived DIP-9 identity-auth keys directly from that in-memory wallet, which holds no private key, so they bottomed out inroot_extended_priv_key()returningExternal signable wallet has no private key.Identity registration on the same wallets already works because its key derivation never touches the in-memory wallet — it resolves the mnemonic on demand through a Swift-owned
MnemonicResolver(Keychain-backed), builds a masterExtendedPrivKey, and derives from it. This PR routes the rescan / preview through that same mechanism.What was done?
packages/rs-platform-walletidentity_handle.rs: addedderive_identity_auth_key_hash_from_master, a master-xpriv sibling ofderive_identity_auth_key_hashthat goes throughderive_ecdsa_identity_auth_keypair_from_master+ripemd160_sha256, so the rescan and registration derivations can never drift.discovery.rs: factored the gap-limit scan into one shareddiscover_innerwith a pluggableKeyHashSource(resident wallet vs. supplied master) and added the publicdiscover_from_master.discover()behavior for key-resident wallets is unchanged; the per-index read lock is now only taken on the wallet-internal derive path.load_identity_by_index(same latent flaw) was intentionally left untouched.packages/rs-platform-wallet-ffi(signatures changed in place — pre-release)platform_wallet_discover_identitiesandplatform_wallet_preview_identity_registration_keysnow take aMnemonicResolverHandle. When non-null they resolve the mnemonic keyed by the wallet handle's ownwallet_id(self-pinned, same rationale asdash_sdk_derive_identity_key_at_slot_with_resolver), build the master, and derive viadiscover_from_master/derive_ecdsa_identity_auth_keypair_from_master; when null they keep the resident-wallet behavior for key-resident consumers.resolve_master_from_resolverhelper so both entry points stay in lockstep. Mnemonic/seed live inZeroizingbuffers; the master's scalar isnon_secure_erased before return (ExtendedPrivKeyhas noDrop).packages/swift-sdkManagedPlatformWallet.discoverIdentities/previewIdentityRegistrationKeysconstruct aMnemonicResolver(defaultWalletStorage, overridable for tests) and pass its handle to the updated FFI.discoverIdentitiesruns the FFI insideTask.detached, so the resolver is captured in the closure and the FFI call is wrapped inwithExtendedLifetimeto keep ARC holding it for the call's duration. Public signatures stay source-compatible (defaulted params only) — the example app'sSearchWalletsForIdentitiesViewcompiles unchanged. Swift stays a thin bridge; no derivation/policy moved into Swift (perswift-sdk/CLAUDE.md).How Has This Been Tested?
cargo fmt --all;cargo clippyclean for bothplatform-walletandplatform-wallet-ffi.cargo test -p platform-wallet --all-features: 223 passed, 0 failed (220 prior + 3 new). Newidentity_handle::testspin the fix for the "abandon … about" vector:master_hash_matches_resident_wallet_hash— master-based hash equals the resident-walletderive_identity_auth_key_hashon aMnemonicwallet across slots/networks.external_signable_errors_but_master_succeeds— the resident derive errors on anExternalSignablewallet while the master derive succeeds and reproduces the key-resident hash.master_hash_matches_registration_keypair_pubkey_hash— parity with registration's in-creation key #0.cargo test -p platform-wallet-ffi --all-features: all passing (100 + 26 + 5/9, 4 ignored).cargo check --workspace --all-targets: clean.cd packages/swift-sdk && ./build_ios.sh --target sim --profile release: succeeded; the regeneratedplatform-wallet-ffi.hshows the newMnemonicResolverHandle *mnemonic_resolver_handleparameter on both functions.xcodebuild -project SwiftExampleApp/SwiftExampleApp.xcodeproj -scheme SwiftExampleApp -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 16': BUILD SUCCEEDED withSearchWalletsForIdentitiesViewunchanged.Breaking Changes
None that warrant a
!type. The two FFI symbols gained a resolver parameter (pre-release SDK/FFI churn, not consensus-breaking); the only callers are the two Swift SDK methods, whose public signatures remain source-compatible via defaulted parameters.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests