fix(platform): load identity by index through the wallet signer#3861
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>
… claude/loving-poitras-d9b2e2
`IdentityWallet::load_identity_by_index` derived the DIP-9 identity-auth key hash from the in-memory key_wallet Wallet, which fails for app wallets loaded as `WalletType::ExternalSignable` (seed in iOS Keychain) with "External signable wallet has no private key" — the same bug PR #3860 fixed for the discovery scan. Apply the same fix pattern: - Add `load_identity_by_index_from_master` deriving the probe hash from a caller-resolved master xpriv via `derive_identity_auth_key_hash_from_master`; both entry points share one body, and the probe slot now uses `MASTER_KEY_INDEX` explicitly. - Expose `platform_wallet_load_identity_at_index` in platform-wallet-ffi with a nullable `MnemonicResolverHandle` (null = historical resident-wallet derive), reusing the shared `resolve_master_from_resolver` helper with the same zeroization hygiene as discovery. - Add `ManagedPlatformWallet.loadIdentity(atIndex:storage:)` in the Swift SDK mirroring `discoverIdentities` (MnemonicResolver + withExtendedLifetime). - Unit tests pin that the ExternalSignable resident derive fails at the loading slot while the master derive reproduces the key-resident hash byte-for-byte on both networks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds resolver-backed identity loading for non-resident wallets. A new library method ChangesResolver-backed identity loading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 55ff3de) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/identity_discovery.rs (1)
171-197: Avoid the “pre-poll dispatch error” concern; consider scrubbing on panic via a Drop guard
block_on_workeralways spawns the future on the shared tokio runtime and only fails via panic (rt.spawn(future).await.expect(...)), so there’s no outer error path that would dropmasterbefore the in-asyncmaster.private_key.non_secure_erase()runs (for both Ok/Err results). The existing pattern inidentity_loading.rsmirrors this as well.Optional hardening: if
discover_from_master/load_identity_by_index_from_mastercan panic, the current wipe won’t run; a smallDropguard aroundmasterwould ensure scrubbing during unwinding.🤖 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/rs-platform-wallet-ffi/src/identity_discovery.rs` around lines 171 - 197, The current code wipes master.private_key only after discover_from_master returns, so if that function panics the scrub won't run; wrap the moved `master` in a small Drop guard that calls `master.private_key.non_secure_erase()` in its Drop impl (e.g., create a ScrubOnDrop or MasterGuard that owns the ExtendedPrivKey and erases in Drop), then move that guard into the async closure passed to `block_on_worker` (used alongside `discover_from_master` / `load_identity_by_index_from_master`) so the private key is scrubbed both on normal completion and on panic/unwinding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs`:
- Around line 26-32: The loop in discover_inner consumes the by-value parameter
source: KeyHashSource<'_> when you match on it inside the gap-limit while loop,
causing a move on first iteration; fix by making KeyHashSource copyable or by
borrowing it in the loop. Either derive Copy and Clone on enum KeyHashSource
(add #[derive(Copy, Clone)] above KeyHashSource) so matching by value is
allowed, or change the loop to match on &source and adjust pattern arms to use
references (match &source { KeyHashSource::ResidentWallet => ...,
KeyHashSource::Master(x) => ... } ) so source is not moved. Ensure references to
ExtendedPrivKey in the Master arm are used accordingly.
---
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/identity_discovery.rs`:
- Around line 171-197: The current code wipes master.private_key only after
discover_from_master returns, so if that function panics the scrub won't run;
wrap the moved `master` in a small Drop guard that calls
`master.private_key.non_secure_erase()` in its Drop impl (e.g., create a
ScrubOnDrop or MasterGuard that owns the ExtendedPrivKey and erases in Drop),
then move that guard into the async closure passed to `block_on_worker` (used
alongside `discover_from_master` / `load_identity_by_index_from_master`) so the
private key is scrubbed both on normal completion and on panic/unwinding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ba91cce-08a9-44aa-8e80-28c673c08809
📒 Files selected for processing (10)
packages/rs-platform-wallet-ffi/src/identity_discovery.rspackages/rs-platform-wallet-ffi/src/identity_key_preview.rspackages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rspackages/rs-platform-wallet-ffi/src/identity_loading.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/network/discovery.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/loading.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Focused fix that mirrors the established #3860 resolver pattern onto single-index identity loading. New load_identity_by_index_from_master, the FFI entry point pinned on the wallet handle's own wallet_id, and the Swift wrapper (which correctly uses withExtendedLifetime(resolver)) are all consistent with the merged discovery path. No in-scope defects; the two codex 'blocking' findings either describe a pre-existing accepted pattern shared with discovery (Copy-semantics on ExtendedPrivKey) or target code this PR does not modify (preview wrapper).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3861 +/- ##
============================================
+ Coverage 70.73% 70.82% +0.09%
============================================
Files 20 20
Lines 2788 2797 +9
============================================
+ Hits 1972 1981 +9
Misses 816 816
🚀 New features to boost your workflow:
|
…tras-d9b2e2 # Conflicts: # packages/rs-platform-wallet-ffi/src/identity_discovery.rs # packages/rs-platform-wallet-ffi/src/identity_key_preview.rs # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior review at 6b70ddf had no findings, so nothing to carry forward. The only substantive new agent finding (codex-general's identity-promotion gap at loading.rs:240) is a pre-existing pattern that exists identically in already-merged discovery.rs from PR #3860, which this PR explicitly mirrors; it is not introduced or relied upon by this PR's stated goal (External-signable resolver-backed derive). No in-scope blocking issues, suggestions, or nitpicks.
|
✅ 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: "d95b669d8256f7386f031261690aa9d0b120f9edc78c052dc7ef2eb525aa7a18"
)Xcode manual integration:
|
3 participants
Issue being fixed or feature implemented
IdentityWallet::load_identity_by_indexderives the DIP-9 identity-auth key hash from the in-memorykey_wallet::Wallet. App wallets are loaded asWalletType::ExternalSignable(seed in iOS Keychain), so the derive fails withExternal signable wallet has no private key— the exact bug #3860 fixed for the discovery scan. Loading a single identity at a known HD index was also unreachable from the app: no FFI entry point or Swift wrapper existed for it.Depends on #3860 (its commit is included in this branch; the diff shrinks to the loading-only changes once it merges).
What was done?
Applied the #3860 fix pattern to the single-index loading path:
rs-platform-wallet(wallet/identity/network/loading.rs): addedIdentityWallet::load_identity_by_index_from_master, deriving the probe hash from a caller-resolved master xpriv viaderive_identity_auth_key_hash_from_master. Both entry points share one body (load_identity_by_index_inner); the source branch lives in a pure, unit-testablederive_load_probe_hashhelper. The probe slot and the stored derivation breadcrumb now useMASTER_KEY_INDEXinstead of a bare0.rs-platform-wallet-ffi(newidentity_loading.rs): exposedplatform_wallet_load_identity_at_index(wallet_handle, mnemonic_resolver_handle, identity_index, out_found, out_identity_id). A non-null resolver resolves the wallet's mnemonic on demand (self-pinned to the handle's ownwallet_id, via the sharedresolve_master_from_resolver) and drives the_from_masterpath with the same zeroization hygiene as discovery; null falls back to the resident-wallet derive.ManagedPlatformWallet.swift): addedloadIdentity(atIndex:storage:)mirroringdiscoverIdentities—MnemonicResolver+withExtendedLifetime, thin bridge perswift-sdk/CLAUDE.md. Returns the discovered identity'sIdentifierornil; the identity itself reaches SwiftData via the existing persister callback.How Has This Been Tested?
loading.rs(mirroring the fix(platform): derive identity-rescan keys through the wallet signer #3860identity_handle.rstests):load_probe_master_matches_resident_across_networks_and_indicesandexternal_signable_load_probe_errors_but_master_succeeds— pin that the resident derive fails forExternalSignablewallets at the loading slot while the master derive reproduces the key-resident hash byte-for-byte (Mainnet + Testnet, several indices), and that loading and discovery probe the same slot.cargo test -p platform-wallet --lib(new tests + fix(platform): derive identity-rescan keys through the wallet signer #3860'sidentity_handletests pass),cargo check -p platform-wallet --all-features --tests,cargo check -p platform-wallet-ffi --all-features --tests,cargo clippy,cargo fmt --all../build_ios.sh --target simregenerates the cbindgen header with the new symbol and builds SwiftExampleApp successfully (** BUILD SUCCEEDED **).Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests