feat(platform)!: external KeychainSigner end-to-end + identity flow sweep#3541
Conversation
…weep
Migrates every iOS-exercised identity flow off the wallet-internal
`IdentitySigner` and onto the external `KeychainSigner` (vtable-backed
`SignerHandle`) wired through the FFI. The seed stops crossing the FFI
for any signing path; mnemonic stays in iOS Keychain; private keys are
derived on demand for one-shot signs.
## KeychainSigner + signer-handle FFI
- Reshaped `SignAsyncCallback` in `rs-sdk-ffi` to pass raw pubkey bytes
+ key-type tag instead of bincode-encoded `IdentityPublicKey`. Swift
trampoline does a SwiftData lookup on `PersistentPublicKey.publicKeyData`
-> `privateKeyKeychainIdentifier` -> Keychain fetch -> sign.
- New `KeychainSigner` Swift class wraps a `dash_sdk_signer_create_with_ctx`
handle. Implements both `Signer<IdentityPublicKey>` and
`Signer<PlatformAddress>` via `VTableSigner` projection - same handle,
two trait views, dispatched on the trampoline's `key_type` byte
(0..4 -> identity-key lookup; `0xFF` -> platform-address-hash lookup).
- New `dash_sdk_sign_with_mnemonic_and_path(mnemonic, passphrase, path,
data, key_type, network, ...)` FFI for on-demand derive+sign of
platform-address keys. Both seed and derived-key buffers `Zeroizing`-
wrapped Rust-side. BIP-39 wordlist auto-detected.
## Identity flows migrated to external signers
- Identity registration (address-funded) - `register_from_addresses`
takes both an identity signer and an address signer. Pubkeys
threaded in from caller-supplied `IdentityPubkeyFFI` array. Result
identity's `public_keys` populated from the placeholder so in-memory
`ManagedIdentity` has its keys immediately.
- DPNS name registration - `register_name_with_external_signer`,
`platform_wallet_register_dpns_name_with_signer`. Tier-2
architectural fix; Tier-1 async patch on
`IdentitySigner::derive_private_key_bytes` kept for the remaining
flows that haven't migrated yet.
- DashPay profile create / update + contact request send / accept -
paired Rust core + FFI + Swift wrappers. Example-app callers
migrated (`IdentityDetailView.DashPayProfileEditorView.save()`,
`FriendsView.AddFriendSheet.sendRequest()`).
- Identity transfer / transfer-to-addresses / withdrawal / update +
asset-lock-funded registration - all Rust + FFI + Swift wrappers
added. Example-app call sites unchanged for now (those flows still
route through `sdk.*` rs-sdk-ffi paths; future sweep).
## Identity-restore wiring + DPP key-security-level rule
- `WalletRestoreEntryFFI` carries `[IdentityKeyRestoreFFI]` rows per
identity. Cold-start populates `Identity.public_keys` directly - no
more empty-keys placeholders waiting on a sync round.
- Document state transitions (DPNS / DashPay profile / contact
request) now require HIGH-or-CRITICAL keys - MASTER explicitly
rejected. Six site fixes across `dpns.rs`, `profile.rs` (x4),
`contact_requests.rs` (x2). Identity update keeps MASTER (it's the
self-modification operation MASTER is for).
## DashPay contract auto-loading
- Added `dashpay-contract`, `withdrawals-contract`,
`wallet-utils-contract`, `token-history-contract`, `keywords-contract`
features to `rs-sdk-trusted-context-provider` in `rs-sdk-ffi`. All
system contracts now auto-load when the proof verifier asks for
them. Fixes "unknown contract" verification failures for DashPay.
## Keychain metadata + on-demand platform-address signing
- `IdentityPrivateKeyMetadata` gained `keyType`, `purpose`,
`securityLevel` fields. `publicKeyHash` now actually computed
(RIPEMD160(SHA256) via new `platform_wallet_hash160` FFI). Codable
migration uses `decodeIfPresent` defaults so older rows decode
cleanly.
- Platform-address signing rewritten to derive on demand from the
mnemonic - no per-address private keys persisted. Deleted
`prePersistPlatformAddressPrivateKeys*`,
`KeychainManager.{store,retrieve,delete}PlatformAddressPrivateKey`,
the `IdentityRestoreFFI.is_watched` field, the
`platform_address_privkey.<hash>` keychain account convention.
## CreateIdentityView UX
- Identity registration index picker reshaped from a fixed gap-limit
pool to a Stepper that defaults to `max(used identity_index) + 1`.
HD derivation is unbounded - there's no pool to deplete. Collision
detection on the input renders red and disables submit.
- After successful registration the form collapses to just the success
card.
- `RegisterNameView` gained an `onRegistered` callback so the parent's
`dpnsNames` `@State` updates immediately without leave-come-back.
## Verification
- `cargo check -p platform-wallet -p platform-wallet-ffi -p rs-sdk-ffi`
clean.
- `cargo test -p platform-wallet --lib` 110 passed;
`-p platform-wallet-ffi --lib` 48 passed;
`-p rs-sdk-ffi --lib` 252 passed.
- `./build_ios.sh --target sim` and `xcodebuild ... iPhone 17 sim`
both BUILD SUCCEEDED.
- End-to-end manual verification on iPhone 17 Pro / iOS 26.3
simulator: Create Identity -> Register DPNS Name -> Set DashPay
Profile all complete on a watch-only-restored wallet.
## Remaining anti-pattern (not blocking)
`IdentitySigner` and `signer_for_identity` still exist; legacy methods
marked Superseded. `TokenWallet` still uses `IdentitySigner::new` (no
FFI today; migrate when tokens get FFI exposure).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis change introduces comprehensive external-signer support to the platform wallet, replacing seed-based signing with externally-provided signer handles. New FFI entry points for identity registration, DashPay profile operations, credit transfers, and DPNS registration now accept signer references. Mnemonic language auto-detection is added, platform-address private-key derivation is removed, and SwiftData persistent models are refactored to use primitive types for predicate compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Swift as Swift App
participant Keychain as KeychainSigner
participant FFI as Platform Wallet FFI
participant Wallet as Platform Wallet
participant SDK as SDK Client
participant Signer as VTableSigner
Swift->>Keychain: init(modelContainer)
Keychain->>FFI: dash_sdk_signer_create_with_ctx(sign_cb, ctx=self)
FFI-->>Keychain: SignerHandle
Swift->>Keychain: sign(identityPublicKey, data)
Keychain->>Keychain: lookup key in SwiftData
Keychain->>Keychain: retrieve from iOS Keychain
Keychain->>FFI: dash_sdk_sign_with_mnemonic_and_path(mnemonic, path)
FFI-->>Keychain: signature
Keychain-->>Swift: signature
Swift->>FFI: platform_wallet_register_identity_with_signer(pubkeys, signer)
FFI->>FFI: parse identity pubkeys
FFI->>Wallet: register_from_addresses(pubkeys, signer)
Wallet->>SDK: create identity transition
SDK->>Signer: sign(identity_public_key, data)
Signer->>FFI: sign_async_callback(pubkey_bytes, key_type)
FFI->>Keychain: retrieve signing material
Keychain-->>Signer: signature via completion_cb
Signer-->>SDK: signature
SDK-->>Wallet: signed transition
Wallet-->>FFI: identity_id + handle
FFI-->>Swift: Success + identity_handle
sequenceDiagram
participant Swift as Swift App
participant FFI as Platform Wallet FFI
participant Wallet as Platform Wallet
participant Keychain as KeychainManager
participant Rust as Rust Native
Swift->>Swift: prePersistIdentityKeysForRegistration(index, count)
Swift->>FFI: dash_sdk_derive_identity_keys_from_mnemonic(mnemonic)
FFI->>Rust: derive_identity_keys (per key_index)
Rust-->>FFI: [IdentityKeyPreviewFFI] (pubkey + path + wif + privkey_bytes)
FFI-->>Swift: keys array
Swift->>Keychain: persist identity keys to iOS Keychain
Keychain->>Keychain: store privkey_bytes with metadata
Swift->>Swift: build identityPubkeys from derived keys
Swift->>FFI: platform_wallet_register_identity_with_signer(identityPubkeys, signer)
FFI->>Wallet: register_from_addresses
Wallet->>Swift: identity registered
Swift->>Keychain: retrieveIdentityPrivateKey(publicKeyHex)
Keychain-->>Swift: privkey_bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Review GateCommit:
|
…ses identity_count Two CI clippy errors after the async-Signer cascade and the two-bucket IdentityManager restructure: - shielded/operations.rs: build_shield_transition is now async (cascade from upstream Signer trait being made async); add the missing .await before .map_err. - examples/basic_usage.rs: IdentityManager.identities() was removed when the manager was split into out_of_wallet_identities and wallet_identities buckets; switch the example to the identity_count() helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (1)
613-629:⚠️ Potential issue | 🟡 MinorUpdate the
markIdentitySlotUseddocstring or remove the function — theisUsedflag is not read by identity-registration logic.The docstring claims this function exists "so the next call to
unusedIdentityIndicesskips it", but that API no longer exists and has been replaced byusedIdentityIndices(for:)(line 740), which reads truth exclusively fromallIdentities, not from theisUsedflag. The code at lines 737–739 explicitly documentsisUsedas a "deprecated" and "denormalized cache that drifted".The
slot.isUsed = truewrite at line 627 is unread by any functional code path for identity-registration addresses. If this is truly cosmetic bookkeeping, update the docstring to say so; otherwise, remove the function and its call site (line 504–505).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift` around lines 613 - 629, The markIdentitySlotUsed(walletId:identityIndex:) function currently writes slot.isUsed = true but that flag is not read by the identity-registration logic (usedIdentityIndices(for:) reads only allIdentities and the code comments mark isUsed as a denormalized/deprecated cache), so either (A) update the function docstring to explicitly state this is purely cosmetic bookkeeping and will not affect identity-registration behavior, referencing markIdentitySlotUsed, isUsed, usedIdentityIndices(for:), and allIdentities; or (B) remove the markIdentitySlotUsed function and its call site so dead writes are eliminated—choose one approach and apply it consistently.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityRegistrationFFI.swift (1)
65-133:⚠️ Potential issue | 🟡 MinorRemove the optional (
?) from thepubkey_bytesfield inIdentityPubkeyFFI.The Rust struct defines
pubkey_bytes: *const u8(non-nullable), and its safety documentation requires "each row'spubkey_bytesmust be a valid[u8; pubkey_len]buffer for the duration of the call." The Swift declaration usesUnsafePointer<UInt8>?(nullable), which contradicts the Rust requirement. Change it toUnsafePointer<UInt8>without the optional.Field order, types, and the function signature parameter sequence all match correctly with the Rust definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/IdentityRegistrationFFI.swift` around lines 65 - 133, The IdentityPubkeyFFI struct's pubkey_bytes is declared nullable but Rust requires a non-null pointer; change IdentityPubkeyFFI.pubkey_bytes from UnsafePointer<UInt8>? to UnsafePointer<UInt8> so it mirrors Rust's non-null *const u8, and audit any callers of platform_wallet_register_identity_with_signer to ensure they pass a non-null buffer for each identity_pubkeys row (maintaining the documented requirement that pubkey_bytes points to a valid [u8; pubkey_len] for the duration of the call).
🧹 Nitpick comments (29)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDocument.swift (1)
45-48: Silent fallback to.testnetmasks data corruption — log it.
AppNetwork(rawValue: networkRaw) ?? .testnetwill quietly swallow any unexpected integer (e.g., a new network added in a future build, a partially-migrated row from a schema change, or a write-side bug that leavesnetworkRawat0when no AppNetwork case maps to that value). A row whosenetworkRawdecodes to nothing is far more useful as a logged anomaly than as an implicit testnet record — particularly for documents tied to identities/wallets, where the wrong network tag misroutes lookups and predicate filters.♻️ Suggested fix: log unexpected raw values
public var network: AppNetwork { - get { AppNetwork(rawValue: networkRaw) ?? .testnet } + get { + if let n = AppNetwork(rawValue: networkRaw) { return n } + // Don't crash, but don't pretend this is normal either. + assertionFailure("PersistentDocument.network: unknown networkRaw=\(networkRaw); defaulting to .testnet") + return .testnet + } set { networkRaw = newValue.rawValue } }The same fallback appears in the other
Persistent*models per the AI summary; consider applying the same treatment uniformly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDocument.swift` around lines 45 - 48, The getter for `network` in `PersistentDocument` silently falls back to `.testnet` when `AppNetwork(rawValue: networkRaw)` is nil; update the `network` getter to detect that nil case, log the unexpected `networkRaw` value (including the raw integer and context like document id or name if available) using the project logger or `os_log`, and then return `.testnet` to preserve behavior; apply the same change to the other `Persistent*` models that use `AppNetwork(rawValue: ...) ?? .testnet` (reference symbols: `network`, `networkRaw`, `AppNetwork`, and the other `Persistent*` model getters).packages/rs-sdk-ffi/src/signer_simple.rs (1)
29-52: De-duplicateparse_mnemonic_any_language— currently three identical copies across crates.The same helper with the same 10-language array exists in:
packages/rs-sdk-ffi/src/signer_simple.rs(this file)packages/rs-platform-wallet-ffi/src/derivation.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rsYour own doc comment notes upstream
key_wallet::Mnemonicdoesn't exposeMnemonic::parse(phrase)even though the underlyingbip39crate does. The cleanest fix is to add a thinMnemonic::parse(orMnemonic::detect_language) tokey-walletand have all three callers use it; that also prevents the language list from silently drifting between crates (e.g., a future addition like Czech-mnemonic-only support landing in one place but not the others, which would make the same phrase succeed in one entry point and fail in another).Until then, at minimum consider extracting it once in
key-wallet(or a small workspace utility) and re-exporting, rather than maintaining three line-for-line copies.#!/bin/bash # Confirm there are no other copies we should fold in rg -nP '\bparse_mnemonic_any_language\b' --type=rust -C2 # Confirm key_wallet (or its bip39 backing) doesn't already expose an auto-detect helper fd -t f Cargo.toml --exec rg -nP '^\s*key[-_]wallet\s*=' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/signer_simple.rs` around lines 29 - 52, There are three identical parse_mnemonic_any_language implementations across crates; extract and centralize it in key-wallet (or a shared workspace crate) by adding a thin helper on key_wallet::mnemonic::Mnemonic such as Mnemonic::parse or Mnemonic::detect_language that encapsulates the LANGUAGES array and the loop currently in parse_mnemonic_any_language, then update callers (parse_mnemonic_any_language in this crate and the two other files) to call the new Mnemonic::parse/detect_language re-export instead of duplicating the language list; ensure the helper returns the same Result<key_wallet::mnemonic::Mnemonic, &'static str> signature so existing call sites need minimal change.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift (1)
599-611: External-signer plumbing reads cleanly; documented watch-only caveat is the right call-out.
KeychainSigner(modelContainer: modelContext.container)per call is fine (it just wraps a signer handle), and the inline note flags that contact-request encryption still pulls the sender's ECDH key from the seed Rust-side — so watch-only restores will hit a Rust error here rather than a Swift one. Worth confirming the failure surfaces with a specific enough message that the UI can distinguish it from a generic network/sign error in the catch on Line 614 (today it just showserror.localizedDescription).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift` around lines 599 - 611, Catch the error thrown by wallet.sendContactRequest(senderIdentityId:recipientIdentityId:signer:) and distinguish the Rust-side watch-only key derivation failure from generic errors: in the catch block that currently shows error.localizedDescription, unwrap the underlying error/bridge error (inspect error's underlying error, domain/code or message text for the Rust watch-only indicator such as "watch-only" or the crate-specific message), map that specific case to a clear UI message like "Action not allowed for watch-only restores" and surface that to the user, otherwise fall back to the generic error.localizedDescription; keep creating KeychainSigner(modelContainer:) as-is and make the change only in the catch/error-handling path around sendContactRequest.packages/rs-platform-wallet-ffi/src/identity_key_preview.rs (1)
264-271: Zeroize private-key material before reclaiming preview rows.
private_key_bytes: [u8; 32](and the existingprivate_key_wif, which encodes the same scalar) are released byrelease_row/free_rows/platform_wallet_preview_identity_registration_keys_freewithout scrubbing the buffers — the bytes survive in freed heap regions until the allocator overwrites them. Given the PR's stated goal of keeping seed/key material out of Rust process memory, the preview path is the one spot where ECDSA scalars do touch Rust heap, so wiping on drop is worth the small change.Suggest using
zeroize(or a hand-rolledvolatile_set_memory) insiderelease_rowbefore each allocation is reclaimed:🔒 Proposed hardening
unsafe fn release_row(row: &IdentityKeyPreviewFFI) { if !row.derivation_path.is_null() { unsafe { drop(CString::from_raw(row.derivation_path)); } } if !row.private_key_wif.is_null() { unsafe { - drop(CString::from_raw(row.private_key_wif)); + let wif = CString::from_raw(row.private_key_wif); + // WIF encodes the same scalar as `private_key_bytes`; + // scrub the bytes before the allocator reclaims them. + let bytes = wif.into_bytes_with_nul(); + zeroize::Zeroize::zeroize(&mut bytes.into_iter().collect::<Vec<u8>>()); } } if !row.public_key.is_null() && row.public_key_len > 0 { unsafe { drop(Vec::from_raw_parts( row.public_key, row.public_key_len, row.public_key_len, )); } } + // Wipe the inline scalar in-place. Casting through a mutable + // pointer is required because `release_row` borrows immutably. + unsafe { + let p = row as *const _ as *mut IdentityKeyPreviewFFI; + zeroize::Zeroize::zeroize(&mut (*p).private_key_bytes); + } }A cleaner alternative is to wrap the row in
ZeroizeOnDropand letDrophandle this implicitly — that also covers the partial-failure path inside the build loop for free.Also applies to: 361-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_key_preview.rs` around lines 264 - 271, The preview rows expose raw private key material (IdentityKeyPreviewFFI.private_key_bytes and private_key_wif) that is freed without scrubbing; fix by zeroizing the private_key_bytes (and the WIF buffer if held in Rust-owned memory) before reclaiming each row in the cleanup paths (release_row, free_rows, and platform_wallet_preview_identity_registration_keys_free). Use the zeroize crate (or a volatile_set_memory helper) to securely overwrite the 32-byte array and any Rust-owned CString contents, or wrap the row in a ZeroizeOnDrop wrapper to ensure automatic wiping on drop (also covers partial-failure during the build loop); ensure the zeroize call runs before calling into into_raw/free to avoid leaving secrets in the heap.packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs (1)
35-50: Tighten signer parameter to*const SignerHandlefor consistency withrs-sdk-ffi.The handle is cast to
*const VTableSigneron line 71 and used as an immutable reference (&VTableSigner), making the*mutparameter unnecessarily permissive.rs-sdk-ffiuses*const SignerHandleconsistently across token and document operations for equivalent immutable borrows. Callers can still pass the*mutpointer returned bydash_sdk_signer_create_with_ctxvia implicit coercion, so this is a non-breaking tightening.Suggested change
- signer_address_handle: *mut SignerHandle, + signer_address_handle: *const SignerHandle,This applies to
platform_address_wallet_transfer(line 33) andplatform_address_wallet_fund_from_asset_lock(line 45) as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs` around lines 35 - 50, Change the signer handle parameters from a mutable pointer to an immutable pointer to match rs-sdk-ffi: update the function signatures for platform_address_wallet_transfer and platform_address_wallet_fund_from_asset_lock so their signer_address_handle parameter is *const SignerHandle (not *mut), keep the existing null checks (is_null) and the cast that converts the pointer to *const VTableSigner/uses &VTableSigner, and ensure any internal uses treat the signer as an immutable borrow; callers can still pass *mut SignerHandle implicitly so no call sites need changes.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (1)
1437-1447: Fallback0forSecurityLeveldoesn't match the "safest DPP default" comment.
SecurityLevel(rawValue: 0)isMASTERon the Rust side — i.e., the most privileged level, not the safest. If aPersistentPublicKey.securityLevelString ever fails to parse (storage corruption only — write path always emitsString(UInt8)), the round-trip silently elevates the key to MASTER, which is the opposite of safe and bypasses the new "MASTER rejected except for identity self-update" gate from this PR.Realistically the row should be skipped on parse failure (mirroring the Rust-side
try_fromskip inbuild_identity_public_keys) so corruption can't synthesize a fake MASTER key locally. Failing safely also keeps Swift and Rust skip behavior symmetric.♻️ Proposed: skip the row on any discriminant parse failure
- row.key_type = UInt8(pk.keyType) ?? 0 - row.purpose = UInt8(pk.purpose) ?? 0 - row.security_level = UInt8(pk.securityLevel) ?? 0 - row.read_only = pk.readOnly + guard + let keyTypeByte = UInt8(pk.keyType), + let purposeByte = UInt8(pk.purpose), + let levelByte = UInt8(pk.securityLevel) + else { + // Storage corruption — let next sync refresh + // the key from chain rather than synthesizing + // bogus discriminants locally. + continue + } + row.key_type = keyTypeByte + row.purpose = purposeByte + row.security_level = levelByte + row.read_only = pk.readOnly🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift` around lines 1437 - 1447, The code in PlatformWalletPersistenceHandler that converts PersistentPublicKey discriminant strings to UInt8 uses `?? 0`, which silently maps parse failures to 0 (SecurityLevel MASTER); instead, mirror the Rust `build_identity_public_keys` behavior and skip the row if any discriminant fails to parse. Update the conversion for `pk.keyType`, `pk.purpose`, and `pk.securityLevel` to safely parse (e.g., guard/if-let using UInt8(...) initializers) and bail out/continue without inserting the row when any parse fails so corrupted strings cannot synthesize a MASTER SecurityLevel.packages/rs-platform-wallet-ffi/src/identity_update.rs (2)
154-163: No duplicate-key-id guard acrossadd_public_keysrows.
row.key_idis taken at face value for each row, so a caller passing two rows with the samekey_idwill produce twoIdentityPublicKey::V0 { id: N, .. }entries with the sameid. Whether this is fatal depends on whatupdate_identity_with_external_signerdoes with theVecdownstream — if it eventually feeds aBTreeMap<KeyID, ..>keyed onidone entry will silently win, and if it feedsIdentityUpdateTransition::add_public_keysthe protocol-side validator may reject the whole transition with a less-actionable error.A
HashSet<KeyID>insertion check during the parse loop, returningErrorInvalidParameteron collision, would catch this at the FFI boundary where the caller can react.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_update.rs` around lines 154 - 163, During parsing of add_public_keys rows before pushing IdentityPublicKey::V0 entries, track seen key IDs (row.key_id) with a HashSet<KeyID> and on insertion collision return ErrorInvalidParameter so duplicate key_id values are rejected at the FFI boundary; update the loop that constructs IdentityPublicKey::V0 (the code that builds keys.push(...)) to check/set the HashSet and return the error early (this prevents downstream silent wins or protocol rejections when update_identity_with_external_signer or IdentityUpdateTransition::add_public_keys is applied).
89-166: Consider validatingpubkey_byteslength againstkey_type.The discriminant validation for
key_type/purpose/security_levelis good, but the per-row pubkey blob is accepted as long as it's non-empty — wrong-length payloads (e.g., a 32-byte uncompressed-x point handed in forECDSA_SECP256K1which expects 33 compressed bytes, or a 48-byte BLS12_381 key for an ECDSA slot, etc.) will sail through here and only fail later inside DPP validation, with a much harder-to-attribute error path back across the FFI.A small per-
KeyTypelength switch at this validation point gives the caller (SwiftKeychainSigner) a clear, immediateErrorInvalidParameterinstead of a wallet-operation failure several layers in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_update.rs` around lines 89 - 166, Validation currently only checks pubkey_bytes is non-empty; add a KeyType-specific length check inside the add_public_keys loop (after reading row.pubkey_len / pubkey_bytes and after resolving key_type) to reject wrong-sized blobs immediately. For each KeyType variant (e.g., ECDSA_SECP256K1, BLS12_381, ED25519, etc.) compare pubkey_bytes.len() (or row.pubkey_len) against the expected size(s) and if mismatched set *out_error = PlatformWalletFFIError::new(PlatformWalletFFIResult::ErrorInvalidParameter, format!("add_public_keys[{}].pubkey_bytes length {} is invalid for key_type {:?}", i, pubkey_bytes.len(), key_type)) and return PlatformWalletFFIResult::ErrorInvalidParameter; do this before constructing IdentityPublicKey::V0 to ensure callers get a clear immediate error.packages/rs-platform-wallet/src/wallet/identity/network/dpns.rs (1)
355-385: Duplicated DPNS-name bookkeeping withregister_name.Lines 361–385 are character-for-character the same as lines 206–233 of
register_name(wall-clockacquired_at, label clone, write-lock, idempotency check,add_dpns_namewith the sameDpnsNameInfo). A small private helper onIdentityWalletlike:async fn record_dpns_name_locally( &self, identity_id: &Identifier, label: String, ) { /* …existing 25 lines… */ }would let both
register_nameandregister_name_with_external_signercollapse to a single call site, and any future refinement to the persistence flow (e.g., capturing the contract-side$createdAtonce the SDK surfaces it) only needs to land once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/dpns.rs` around lines 355 - 385, Duplicate DPNS bookkeeping appears in register_name and register_name_with_external_signer; extract the repeated logic into a private async helper on IdentityWallet (e.g., async fn record_dpns_name_locally(&self, identity_id: &Identifier, label: String)) that computes acquired_at, clones/receives the label, acquires the write lock on self.wallet_manager, checks idempotency against managed.dpns_names, and calls managed.add_dpns_name(DpnsNameInfo { label, acquired_at }, &self.persister); then replace the duplicated blocks in register_name and register_name_with_external_signer with a single await call to record_dpns_name_locally so future changes to persistence only need one edit.packages/rs-platform-wallet/src/wallet/identity/network/registration.rs (1)
411-463: IS→CL retry fallback duplicates the legacy code path verbatim.Lines 411–463 are essentially a copy of lines 236–289 from
register_identity_with_funding, modulo thesignerargument shape (&signervssigner) and a slightly different log message. Sameout_point_from_proofextraction, same 180-secondupgrade_to_chain_lock_proofwait, sameInvalidIdentityDataerror mapping in both arms, sameproof_out_point.is_none()early-out branch.A small private helper that takes the proof + retry callback and returns the resolved
Identitywould let both registration variants share the IS→CL recovery logic. Not blocking — both copies are correct — but if the timeout, log message, or error-classification rule ever changes, having two copies is the place where one will get missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs` around lines 411 - 463, Extract the IS→CL retry logic duplicated in this match into a small private helper (e.g., handle_instant_lock_retry) that accepts the initial proof, a closure to call put_to_platform_and_wait_for_response (so it can accommodate the different signer argument shapes), and references to self.asset_locks and self.out_point_from_proof; the helper should: 1) compute proof_out_point via out_point_from_proof, 2) on instant-lock rejection call upgrade_to_chain_lock_proof with Duration::from_secs(180), 3) retry the provided closure with the chain proof and map errors into PlatformWalletError::InvalidIdentityData with the same messages used today, and 4) return Result<Identity, PlatformWalletError>; then replace the duplicated match arms in both registration functions to call this helper, preserving existing log text and behavior.packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs (1)
21-55: Consider deduplicatingSignerRefinto a shared internal module.Identical
SignerRef<'a, S>adapters now live in at leasttransfer.rs,transfer_to_addresses.rs, anddpns.rs(and the AI summary mentionswithdrawal.rsand a contact-request module too). Each copy is the same struct, the sameDebugimpl, and the same#[async_trait] impl<'a, K, S> Signer<K>delegation.A single
pub(crate) struct SignerRef<'a, S: ?Sized>under e.g.packages/rs-platform-wallet/src/wallet/identity/network/signer_ref.rs(orcrate::wallet::signer::SignerRef) re-exported via the existingsuper::*would let every external-signer flow drop ~30 lines and stay in lockstep if the signer-trait surface ever grows (e.g., a new method onSigner<K>would otherwise need to be added in every copy).Not blocking — local copies compile and behave identically — but worth doing before yet another
_with_external_signervariant lands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/transfer.rs` around lines 21 - 55, Duplicate SignerRef<'a, S> adapters exist across modules; extract the struct, its Debug impl, and the #[async_trait] impl<'a, K, S> Signer<K> into a single shared internal module (e.g., wallet/identity/network/signer_ref.rs) as pub(crate) struct SignerRef<'a, S: ?Sized> and keep the same trait bounds and async_trait usage; then replace local definitions in transfer.rs, transfer_to_addresses.rs, dpns.rs (and withdrawal/contact-request modules) with imports from the new module (or re-export via the existing super::*), ensuring all call sites use the centralized SignerRef and preserving behavior when the Signer<K> trait expands.packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (1)
270-284: Free-callback contract is getting deep — consider helper free APIs.
LoadWalletListFreeFnis now responsible for releasing:
- the entries array
- per-wallet accounts arrays
- per-wallet platform-address balance arrays
- every xpub byte buffer
- per-wallet identity arrays
- every nested c-string + c-string pointer array on identity entries
- every per-identity
IdentityKeyRestoreFFIarray- every public-key byte buffer per row
Each Swift implementer has to keep all of that in sync with what they allocated. A single missed level leaks; a single double-free crashes. Consider exposing small per-shape free helpers (e.g.,
platform_wallet_identity_keys_free(ptr, count)) so Swift can call them in any order without re-implementing the unwind tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs` around lines 270 - 284, The free callback LoadWalletListFreeFn is doing too much and is brittle; add granular helper free functions for each nested shape so Swift can free pieces safely (e.g., platform_wallet_identity_keys_free(ptr: *mut IdentityKeyRestoreFFI, count: usize), wallet_accounts_free(ptr: *mut AccountFFI, count: usize), wallet_balances_free(ptr: *mut BalanceFFI, count: usize), wallet_identities_free(ptr: *mut WalletIdentityFFI, count: usize), xpub_buffer_free(ptr: *mut u8, len: usize), and entries_array_free(ptr: *mut *const WalletRestoreEntryFFI, count: usize)), update the API surface (export these helpers alongside LoadWalletListFreeFn) and document the exact ownership for WalletRestoreEntryFFI, IdentityKeyRestoreFFI, AccountFFI and nested c-string arrays so Swift calls the appropriate helper instead of a single monolithic free.packages/rs-platform-wallet-ffi/src/dashpay_profile.rs (2)
615-750: Significant duplication withcreate_or_update_profile.This function's null-check, identifier validation, three-string UTF-8 decode, and avatar-bytes copy logic (lines 630–697) is a near-verbatim copy of the legacy
create_or_update_profilehelper above (lines 414–505). Only the innerblock_on_workerbody differs.Consider extracting the input-parsing prelude into a private helper that returns a
Result<(Identifier, ProfileUpdate), PlatformWalletFFIResult>, then having both entry points call it. Reduces drift risk when the next field is added (e.g., new error code, new optional string).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/dashpay_profile.rs` around lines 615 - 750, The code duplication between platform_wallet_create_or_update_dashpay_profile_with_signer and the existing create_or_update_profile can be removed by extracting the input-parsing prelude into a private helper (e.g., parse_profile_input) that performs null/out_error checks, calls read_identifier, decode_opt_c_str for display_name/public_message/avatar_url, builds avatar_bytes Vec, and returns Result<(Identifier, ProfileUpdate), PlatformWalletFFIResult> (and sets out_error on Err). Replace the duplicated block in platform_wallet_create_or_update_dashpay_profile_with_signer and create_or_update_profile to call parse_profile_input(...) and early-return on Err, then keep only the differing block_on_worker signer/create vs update logic in each function while preserving use of out_profile and out_error.
711-722: Consider wrapping thesignerpointer dereference in an explicitunsafe { }block with a SAFETY comment.For consistency with
platform_wallet_register_dpns_name_with_signerindpns.rs, wrap the pointer dereference (line 712) in an explicitunsafe { }block and add a SAFETY comment documenting the invariants. While the bare dereference is legal in Rust 2021 edition within anunsafe extern "C" fn, wrapping it explicitly documents the safety contract and eases migration to Rust 2024 (whereunsafe_op_in_unsafe_fnbecomes warn-by-default).Refer to dpns.rs lines 267–271 for the pattern used elsewhere in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/dashpay_profile.rs` around lines 711 - 722, Wrap the raw pointer dereference of signer_addr into an explicit unsafe block and add a SAFETY comment describing the invariants (that signer_addr is a valid non-null pointer to a VTableSigner and lives for the duration of the call) similar to the pattern in platform_wallet_register_dpns_name_with_signer; locate the dereference occurring in the async closure inside block_on_worker where you create let signer: &VTableSigner = &*(signer_addr as *const VTableSigner) and replace it with an explicit unsafe { ... } block and a preceding SAFETY comment documenting the required guarantees.packages/rs-platform-wallet/src/wallet/identity/network/update.rs (1)
27-56: ExtractSignerRefto a shared module to eliminate duplication.This
SignerRef<'a, S>adapter is duplicated acrossdpns.rs,profile.rs,contact_requests.rs,transfer.rs,transfer_to_addresses.rs,withdrawal.rs, andupdate.rs. Since all implementations are identical, extracting to a single internal module (e.g.,crate::wallet::identity::network::signer_ref) would remove duplication and ensure future trait-bound or method changes only need to land once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/update.rs` around lines 27 - 56, The SignerRef<'a, S> adapter is duplicated across multiple files; extract it to a single internal module and re-export it for reuse: create a new module (e.g., wallet::identity::network::signer_ref) containing the SignerRef<'a, S> struct, its Debug impl, and the async_trait impl for Signer<K> (preserving method names sign, sign_create_witness, can_sign_with and trait bounds S: Signer<K> + ?Sized + Send + Sync), then replace the local definitions in dpns.rs, profile.rs, contact_requests.rs, transfer.rs, transfer_to_addresses.rs, withdrawal.rs, and update.rs with a use/import of the new SignerRef to eliminate duplication.packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs (2)
471-629: Tests cover the right shape contract.Coin-type-by-network,
key_count == 0no-op, and invalid-mnemonic rejection are good fixtures. Worth adding a test that derives the same(mnemonic, network, identity_index, key_index)twice and asserts byte-for-byte determinism onprivate_key_bytes/public_key— that's the property a future regression in path construction orderive_privwould silently break.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs` around lines 471 - 629, Add a new test that verifies determinism by calling dash_sdk_derive_identity_keys_from_mnemonic twice with the same mnemonic, DashSDKNetwork, identity_index and key_count, using IdentityRegistrationKeyDerivationsFFI to capture outputs, and assert the two results have equal count and that for each index the public_key bytes and private_key_bytes arrays are byte-for-byte identical; remember to call dash_sdk_derive_identity_keys_from_mnemonic_free(&mut out) after each invocation and compare items after cloning/accessing the rows (use CStr for derivation_path if you want to assert paths too).
380-395: Nit: stale comment on the WIF-failure cleanup branch.The comment "Path cstring + pubkey buffer were already detached" is misleading — at this point
path_cstringis still an ownedCString(the.into_raw()call doesn't happen until line 399). Thedrop(path_cstring)call is correct (it just runsCString::Drop), but the comment should be reworded so a future reader doesn't try to "fix" what looks like a double-free.- // Path cstring + pubkey buffer were already detached. + // Pubkey buffer was already detached via `mem::forget(pub_box)` — + // reclaim it through a Vec. `path_cstring` is still owned; + // dropping it here just frees the CString. drop(Vec::from_raw_parts(pub_ptr, pub_len, pub_len)); drop(path_cstring);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rs` around lines 380 - 395, The comment in the WIF-failure branch for the CString::new(dash_private.to_wif()) error is misleading — update it to state that path_cstring is still an owned CString that is being dropped here (not already detached with into_raw), and clarify that we are explicitly freeing the pubkey buffer via drop(Vec::from_raw_parts(pub_ptr, pub_len, pub_len)), dropping path_cstring, and calling cleanup(rows) before returning PlatformWalletFFIResult::ErrorUtf8Conversion and setting out_error (use the symbols wif_cstring, path_cstring, pub_ptr/pub_len, cleanup, out_error, PlatformWalletFFIError/PlatformWalletFFIResult in the comment to make the intent explicit).packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (1)
638-1002: LGTM, with optional dedup opportunity.Logic in
create_profile_with_external_signer/update_profile_with_external_signermatches the legacy variants and correctly drops theidentity_indexlookup since the signer is external. The HIGH/CRITICAL key-constraint rationale is well documented.Optional refactor: across the four profile paths (create/update × legacy/external) the contract load, property-map build, signing-key resolution, and post-publish caching are nearly identical. Extracting helpers like
load_dashpay_contract(),build_profile_properties(&input, avatar_hash, avatar_fingerprint),resolve_profile_signing_key(&self, identity_id), andcache_profile(...)would cut roughly half the body and make future protocol-level changes (e.g. adding new profile fields) require one edit instead of four.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/profile.rs` around lines 638 - 1002, The two methods duplicate logic present in their legacy counterparts — extract shared functionality into helpers: implement load_dashpay_contract() to return the Arc<DataContract>, build_profile_properties(input, avatar_hash, avatar_fingerprint) -> BTreeMap<String, Value>, resolve_profile_signing_key(&self, identity_id) -> Result<IdentityPublicKey, PlatformWalletError> (used by both legacy and _with_external_signer variants when signer is external), and cache_profile(&self, identity_id, profile) to update managed identity via self.persister; then replace the contract-loading, property-map construction, signing-key lookup, and post-publish cache update blocks inside create_profile_with_external_signer, update_profile_with_external_signer and the legacy create_profile/update_profile to call these helpers (preserve current error types and signatures and pass SignerRef where needed).packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs (1)
105-174: Optional: collapse the three discriminant try-from blocks.The three
TryFrom::try_from(...)arms forkey_type,purpose,security_levelare structurally identical (~15 lines × 3 = ~45 lines that all fail withErrorInvalidParameter). A small generic helper could reduce noise:♻️ Sketch
unsafe fn map_discriminant<T, F>( raw: u8, field: &str, row_index: usize, out_error: *mut PlatformWalletFFIError, ) -> Result<T, PlatformWalletFFIResult> where T: TryFrom<u8, Error = F>, F: std::fmt::Display, { T::try_from(raw).map_err(|_| { if !out_error.is_null() { *out_error = PlatformWalletFFIError::new( PlatformWalletFFIResult::ErrorInvalidParameter, format!( "identity_pubkeys[{row_index}].{field} = {raw} is not valid" ), ); } PlatformWalletFFIResult::ErrorInvalidParameter }) }Then each call collapses to:
let key_type: KeyType = match map_discriminant(row.key_type, "key_type", i, out_error) { Ok(v) => v, Err(code) => return code, };Not blocking — the current shape is fine, just verbose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs` around lines 105 - 174, The three identical TryFrom error-handling blocks for row.key_type, row.purpose and row.security_level should be collapsed into a small helper (e.g., unsafe fn map_discriminant<T, E>(raw: u8, field: &str, row_index: usize, out_error: *mut PlatformWalletFFIError) -> Result<T, PlatformWalletFFIResult> where T: TryFrom<u8, Error = E>, E: std::fmt::Display) that calls T::try_from(raw) and on Err sets *out_error = PlatformWalletFFIError::new(PlatformWalletFFIResult::ErrorInvalidParameter, format!("identity_pubkeys[{row_index}].{field} = {raw} is not valid")) if out_error is not null, then returns PlatformWalletFFIResult::ErrorInvalidParameter; replace the three match blocks with calls like let key_type: KeyType = map_discriminant(row.key_type, "key_type", i, out_error)?; and similarly for purpose and security_level before constructing IdentityPublicKeyV0 so behavior and error codes (PlatformWalletFFIResult::ErrorInvalidParameter) remain identical.packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift (2)
414-428:memset_ssecond argument should be the buffer's byte length, which here equalscount(UInt8 elements).
sigBufis[UInt8], soptr.countis the byte length and the call is correct. Leaving this note only because the same pattern inffiSignusesbuf.countidentically — please double-check all fourmemset_ssites if the buffer element type ever changes (e.g.[UInt32]), sinceptr.countis in elements rather than bytes for non-byte types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift` around lines 414 - 428, The defer scrub using sigBuf.withUnsafeMutableBufferPointer calls memset_s with ptr.count which is correct for [UInt8] but will be wrong if the buffer element type changes; update the memset_s second argument to use the buffer's byte length explicitly (e.g., ptr.count * MemoryLayout.elementStride/size) or compute bytes via ptr.baseAddress?.withMemoryRebound(to: UInt8.self, capacity: ptr.count) so the call always passes byte count, and audit the other three memset_s sites (including ffiSign) to make the same change so the scrub uses byte-length not element-count.
590-590: Early-return whencompletionis nil silently leaks the Rust completion slot.If
completionis null butcompletionCtxis non-null, we return without invoking it — leaving Rust'sCompletionSlotto time out after 5 minutes (and leak theBox<oneshot::Sender>). In practice Rust always passes a non-null completion, so this is defensive code only, but it's worth eitherprecondition-ing oncompletion != nilor documenting the expectation rather than silently returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift` at line 590, The guard currently does "guard let ctx, let completion else { return }" which silently drops when completion is nil and leaks Rust's CompletionSlot; change this to keep the defensive check but make the nil-completion case explicit: either assert/precondition that completion != nil (e.g. precondition(completion != nil, "Expected non-nil completion from Rust") ) or, if you prefer to handle it, call completionCtx with a failure before returning so the CompletionSlot is fulfilled; locate the variables completion, completionCtx and ctx in KeychainSigner.swift and replace the silent return with a precondition or an explicit invocation of completionCtx to avoid leaking Rust's Box<oneshot::Sender>.packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs (3)
451-462: Superseded entry point still exported — consider#[deprecated]for compiler-visible signal.The doc clearly steers new callers to
dash_sdk_derive_identity_keys_from_mnemonic, butplatform_wallet_derive_identity_keys_for_indexremains an unattributed public symbol. Adding#[deprecated(note = "use dash_sdk_derive_identity_keys_from_mnemonic; this path fails for watch-only wallets")](or annotating the linked Swift binding) gives downstream consumers a build-time nudge instead of relying on the rustdoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs` around lines 451 - 462, The exported function platform_wallet_derive_identity_keys_for_index is marked in docs as superseded but still a public symbol; add a compiler-visible deprecation attribute by annotating the public function (platform_wallet_derive_identity_keys_for_index) with #[deprecated(note = "use dash_sdk_derive_identity_keys_from_mnemonic; this path fails for watch-only wallets")] placed immediately above its declaration so downstream builds get a warning; ensure the attribute text references dash_sdk_derive_identity_keys_from_mnemonic and keeps the existing doc comment intact.
264-335: Duplicatekey_idrows inidentity_pubkeyssilently overwrite earlier entries.
keys_map.insert(row.key_id, ...)accepts the last-write-wins semantics ofBTreeMap, so a malformed input array with two rows sharing the samekey_idproduces a placeholder identity with the second key only — no error is surfaced to the caller. Worth either checkingkeys_map.insert(...).is_some()and erroring withErrorInvalidParameter, or documenting the dedup behavior so callers know the rust side won't validate uniqueness for them.♻️ Sketch
- keys_map.insert( - row.key_id, - IdentityPublicKey::V0(IdentityPublicKeyV0 { ... }), - ); + if keys_map + .insert(row.key_id, IdentityPublicKey::V0(IdentityPublicKeyV0 { ... })) + .is_some() + { + if !out_error.is_null() { + *out_error = PlatformWalletFFIError::new( + PlatformWalletFFIResult::ErrorInvalidParameter, + format!("identity_pubkeys contains duplicate key_id {}", row.key_id), + ); + } + return PlatformWalletFFIResult::ErrorInvalidParameter; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs` around lines 264 - 335, The loop silently overwrites duplicate row.key_id values when inserting into keys_map; update the insertion logic in identity_registration_with_signer.rs (the for loop that builds keys_map using keys_map.insert) to detect duplicates by checking if keys_map.insert(row.key_id, ...) returns Some(_), and when it does set *out_error (using PlatformWalletFFIError with PlatformWalletFFIResult::ErrorInvalidParameter) to a clear message referencing the duplicate row.key_id and return ErrorInvalidParameter instead of overwriting; ensure you use the same error-null check (!out_error.is_null()) pattern used elsewhere.
532-550:derive_identity_auth_keypairfailure leakspath_cstringof the failing iteration only viacleanup(rows).On lines 532–550, when derivation fails for the current iteration,
cleanup(rows)reclaims allocations from previously-pushed rows but the current iteration hasn't allocated anything yet — so this is correct. However, the matching paths at lines 552–564 (CString NUL on path) and 577–593 (CString NUL on WIF) handle in-flight allocations differently and inconsistently:
- Path NUL (552–564): only
cleanup(rows)— no current-iteration allocations to reclaim. ✓- WIF NUL (577–593): manually drops
pub_ptr(Vec),path_cstring(already owned, drops automatically), thencleanup(rows). ✓ but verbose.Consider extracting the per-row build into
Result<IdentityKeyPreviewFFI, PlatformWalletFFIResult>so each iteration owns its in-flight allocations under RAII and the outer loop only has to handle the rows already pushed torows. Pure refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs` around lines 532 - 550, The loop currently leaks or inconsistently drops per-iteration allocations when derive_identity_auth_keypair or subsequent CString NUL checks fail; refactor the per-row construction into a function that returns Result<IdentityKeyPreviewFFI, PlatformWalletFFIResult> (or a per-row builder Result) so all in-flight allocations (path_cstring, pub_ptr Vec, etc.) are owned inside the function and automatically dropped on error, and have the outer loop only call cleanup(rows) for already-pushed items; update calls around derive_identity_auth_keypair, the path CString creation, and the WIF CString creation to use this new per-row builder and return the appropriate PlatformWalletFFIResult on error.packages/rs-platform-wallet-ffi/src/identity_transfer.rs (1)
207-224: Use of*const Treinterpreted viausizeforSendcapture is fine; document thatoutputs_countmust fitisize::MAX.
slice::from_raw_parts(outputs, outputs_count)carries the standard safety requirement thatoutputs_count * size_of::<PlatformAddressCreditOutputFFI>()not exceedisize::MAX. With ~32 bytes/row that's astronomically unlikely in practice, but the unsafe-doc on this function says only "valid array for the duration of the call" — adding a one-liner about the size cap brings it in line with the standard FFI safety pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet-ffi/src/identity_transfer.rs` around lines 207 - 224, Add a short safety note next to the unsafe slice::from_raw_parts usage stating that outputs_count must be <= isize::MAX (so outputs_count * size_of::<PlatformAddressCreditOutputFFI>() cannot exceed isize::MAX) and that the caller must ensure the pointer and length form a valid array for the duration of the call; reference the variables/constructs involved (outputs, outputs_count, slice::from_raw_parts, PlatformAddressCreditOutputFFI, entries, and the surrounding loop that builds output_map) so future readers see the standard FFI size cap requirement.packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (2)
643-680: Pre-registration fallback scans every identity-privkey row on each signing call.
retrieveIdentityPrivateKey(publicKeyHex:)performs an unboundedkSecMatchLimitAllquery and JSON-decodes every metadata blob until a hex match is found. This runs on the FFI signer hot path during the (brief) registration window when SwiftData rows haven't landed yet. For a wallet with many identity keys it's still small, but worth being aware that:
- The scan is O(n) per signing call where n = total identity privkey rows for the service.
- There's no early break on
kSecAttrAccountmismatch before the JSON decode (the prefix check is cheap; OK).- If two rows somehow shared the same
publicKeyhex, the iteration order is undefined.If the fallback path is hit frequently in practice, consider adding a secondary lookup index (e.g. a
kSecAttrLabelset to the pubkey hex) so the query can be a direct match instead of a full scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift` around lines 643 - 680, retrieveIdentityPrivateKey(publicKeyHex:) currently does a kSecMatchLimitAll scan and JSON-decodes every item until a match, causing O(n) work on the signing hot path; change the retrieval to perform a direct keyed lookup instead of scanning: when storing identity private keys add a secondary index field (e.g. set kSecAttrLabel to the lowercased publicKey hex or store account as "identity_privkey.<pubhex>") and then in retrieveIdentityPrivateKey build a query that includes that attribute (kSecAttrLabel == publicKeyHex or kSecAttrAccount == "identity_privkey.\(publicKeyHex)") with kSecMatchLimit set to kSecMatchLimitOne and kSecReturnData true so you can avoid JSON-decoding all rows and return the kSecValueData directly; if you cannot change storage now, at minimum limit the query to match kSecAttrAccount prefix before decoding or add kSecMatchLimitOne and a deterministic tiebreak to ensure predictable behavior.
467-479:computePublicKeyHashHexswallows FFI failures as"".Returning
""onrc != 0makes the failure indistinguishable from "empty input". Any caller writingmetadata.publicKeyHash = computePublicKeyHashHex(pk)will end up with a row whosepublicKeyHash == ""for both legitimate-empty and unexpected-FFI-failure cases — and the explorer can't tell them apart. Consider logging the FFI failure (or returningnil) so a regression inplatform_wallet_hash160doesn't silently drop hash metadata across every newly-stored identity key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift` around lines 467 - 479, computePublicKeyHashHex currently swallows FFI failures by returning an empty string, conflating empty-input with real errors; change public static nonisolated func computePublicKeyHashHex(_ publicKey: Data) -> String to return an optional String? (or add a throwing variant) and return nil when publicKey.isEmpty or when platform_wallet_hash160 returns rc != 0, and log the failure including rc and publicKey.count (and any error text) so callers can distinguish empty input from FFI failure; update all callers that assign metadata.publicKeyHash to handle the optional (or try/catch the throw).packages/rs-sdk-ffi/src/signer.rs (1)
469-572:Signer<PlatformAddress>impl duplicates the oneshot-completion plumbing from theSigner<IdentityPublicKey>impl.The
oneshot::channelsetup,CompletionSlotboxing, vtable invocation, andtokio::time::timeoutarms are essentially copy-pasted from the identity-keysignimplementation (lines 342–394). Consider extracting anasync fn callback_sign(&self, signer_ptr, vtable, key_type: u8, key_bytes: &[u8], data: &[u8]) -> Result<BinaryData, ProtocolError>helper onVTableSignerand having bothSigner<K>impls delegate to it. Pure refactor, no behavior change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/signer.rs` around lines 469 - 572, Duplicate oneshot completion and FFI-invocation logic in Signer<PlatformAddress>::sign should be extracted into a shared async helper on VTableSigner to avoid copy-paste with Signer<IdentityPublicKey>. Add an async method on VTableSigner, e.g. callback_sign(&self, signer_ptr: *mut c_void, vtable: *const VTableType, key_type: u8, key_bytes: &[u8], data: &[u8]) -> Result<BinaryData, ProtocolError>, which performs the oneshot::channel creation, CompletionSlot boxing, unsafe ((*(*vtable)).sign_async) call, timeout with SIGN_ASYNC_COMPLETION_TIMEOUT and the same match arms returning BinaryData/ProtocolError (using SignResult, CompletionSlot, dash_sdk_sign_async_completion), then replace the duplicated block in Signer<PlatformAddress>::sign with a call into this helper (passing signer_ptr, vtable, SIGNER_KEY_TYPE_PLATFORM_ADDRESS_HASH and the 20-byte hash) and similarly have the Signer<IdentityPublicKey> impl delegate to it with its key type/bytes; preserve all existing semantics and error messages.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
776-795: Hardcoded DPP discriminants — surface these from the FFI /KeyType/SecurityLevelenums.
keyType: 0,purpose: 0, andsecurityLevel: secLevelByte(withsecLevelBytederived from a0=MASTER / 2=HIGHmirror in Swift) duplicate DPP protocol constants the rest of this file already encapsulates:IdentityPubkeyitself goes throughKeyType.ffiValue/SecurityLevel.ffiValue(see lines 467–469). Mirroring them again here makes a future DPP discriminant change a silent breakage on this code path only.Prefer routing through the existing enums (
KeyType.ecdsaSecp256k1.ffiValue,KeyPurpose.authentication.ffiValue,SecurityLevel.master/.high.ffiValue), or — better — have the Rust derivation FFI returnkey_type/purpose/security_levelper row so Swift just forwards them.As per coding guidelines: "Do not re-implement protocol constants (gap limit, key indices, path shapes) as Swift mirrors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 776 - 795, Replace the hardcoded discriminants in the Keychain row metadata by routing them through the existing enum ffi values instead of numeric literals: use KeyType.ecdsaSecp256k1.ffiValue for keyType, KeyPurpose.authentication.ffiValue for purpose, and SecurityLevel.master/.high.ffiValue for securityLevel (compute securityLevel using SecurityLevel.master when i == 0 else SecurityLevel.high) rather than the current secLevelByte/0/2 constants; update the KeychainManager.IdentityPrivateKeyMetadata construction to use those enum ffiValue properties (or alternatively pull key_type/purpose/security_level from the Rust FFI derivation result and forward them) so the metadata stays in sync with IdentityPubkey/FFI enums.
| public func prePersistIdentityKeysForRegistration( | ||
| identityIndex: UInt32, | ||
| keyCount: UInt32, | ||
| network: DashSDKNetwork, | ||
| walletIdHex: String, | ||
| keychain: KeychainManager = .shared, | ||
| storage: WalletStorage = WalletStorage() | ||
| ) throws -> [IdentityRegistrationKeyPreview] { | ||
| guard keyCount > 0 else { return [] } | ||
|
|
||
| // Pull the BIP-39 mnemonic out of Keychain. `String` can't be | ||
| // truly zeroed in Swift, but we keep the local lifetime as | ||
| // narrow as possible — only this function holds a reference, | ||
| // and the FFI call inside copies the bytes onto the Rust side | ||
| // (where they ARE held in `Zeroizing` and scrubbed at function | ||
| // exit). | ||
| let mnemonic = try storage.retrieveMnemonic(for: self.walletId) | ||
|
|
||
| var out = identityRegistrationKeyDerivationsFFIEmpty() | ||
| var error = PlatformWalletFFIError() | ||
| let result = mnemonic.withCString { mPtr -> PlatformWalletFFIResult in | ||
| // No BIP-39 passphrase is supported by the rest of the | ||
| // SDK yet (mirrors `KeychainSigner.swift`'s own | ||
| // `dash_sdk_sign_with_mnemonic_and_path` call site). | ||
| // Pass `nil` to mean "empty passphrase". | ||
| dash_sdk_derive_identity_keys_from_mnemonic( | ||
| mPtr, | ||
| nil, | ||
| network, | ||
| identityIndex, | ||
| keyCount, | ||
| &out, | ||
| &error | ||
| ) | ||
| } | ||
| defer { dash_sdk_derive_identity_keys_from_mnemonic_free(&out) } | ||
|
|
||
| guard result == Success else { | ||
| throw PlatformWalletError(result: result, error: error) | ||
| } | ||
|
|
||
| guard let base = out.items, out.count > 0 else { return [] } | ||
|
|
||
| var persisted: [IdentityRegistrationKeyPreview] = [] | ||
| persisted.reserveCapacity(out.count) | ||
|
|
||
| for i in 0..<out.count { | ||
| let row = base[i] | ||
| let path: String = row.derivation_path.map { String(cString: $0) } ?? "" | ||
| let wif: String = row.private_key_wif.map { String(cString: $0) } ?? "" | ||
|
|
||
| let pubData: Data | ||
| let pubHex: String | ||
| if let pubPtr = row.public_key, row.public_key_len > 0 { | ||
| pubData = Data(bytes: pubPtr, count: row.public_key_len) | ||
| pubHex = pubData.map { String(format: "%02x", $0) }.joined() | ||
| } else { | ||
| pubData = Data() | ||
| pubHex = "" | ||
| } | ||
|
|
||
| var pkTuple = row.private_key_bytes | ||
| let pkData = withUnsafeBytes(of: &pkTuple) { Data($0) } | ||
|
|
||
| // Identity-id is unknown pre-registration (Rust | ||
| // recomputes it from the input addresses). Use the | ||
| // marker `pending` so the keychain explorer makes it | ||
| // obvious which rows are pre-registered slots. | ||
| // | ||
| // Discriminant convention matches `CreateIdentityView`'s | ||
| // `IdentityPubkey` mapping (and the upstream Rust-side | ||
| // policy that lived inside `register_identity` before it | ||
| // moved to Swift): | ||
| // - keyId 0 → MASTER, AUTHENTICATION, ECDSA_SECP256K1 | ||
| // - keyId > 0 → HIGH, AUTHENTICATION, ECDSA_SECP256K1 | ||
| // The bytes used here (`0` / `1` / `0`) are the | ||
| // canonical DPP `repr(u8)` discriminants, the same ones | ||
| // every other FFI surface in this SDK speaks. | ||
| let pubHashHex = KeychainManager.computePublicKeyHashHex(pubData) | ||
| // CreateIdentityView's enum mapping uses .high for | ||
| // non-master rows; the DPP `SecurityLevel` discriminant | ||
| // for HIGH is 2 (0=MASTER, 1=CRITICAL, 2=HIGH, 3=MEDIUM). | ||
| // Mirror that here so the Keychain row stamps the same | ||
| // security level the pubkey row actually submitted to | ||
| // the network carries. | ||
| let secLevelByte: UInt8 = i == 0 ? 0 /* MASTER */ : 2 /* HIGH */ | ||
| let metadata = KeychainManager.IdentityPrivateKeyMetadata( | ||
| identityId: "pending", | ||
| keyId: UInt32(i), | ||
| walletId: walletIdHex, | ||
| identityIndex: identityIndex, | ||
| keyIndex: UInt32(i), | ||
| derivationPath: path, | ||
| publicKey: pubHex, | ||
| publicKeyHash: pubHashHex, | ||
| keyType: 0, // ECDSA_SECP256K1 | ||
| purpose: 0, // AUTHENTICATION | ||
| securityLevel: secLevelByte | ||
| ) | ||
| _ = keychain.storeIdentityPrivateKey( | ||
| pkData, | ||
| derivationPath: path, | ||
| metadata: metadata | ||
| ) | ||
|
|
||
| persisted.append( | ||
| IdentityRegistrationKeyPreview( | ||
| identityIndex: identityIndex, | ||
| derivationPath: path, | ||
| publicKeyData: pubData, | ||
| publicKeyHex: pubHex, | ||
| privateKeyWIF: wif, | ||
| privateKeyData: pkData | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| return persisted | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Violates the "no mnemonic round-tripping" guideline — orchestrate this in Rust.
prePersistIdentityKeysForRegistration does exactly the flow the coding guidelines forbid: it pulls the BIP-39 mnemonic out of Keychain (line 713), hands it to Rust via dash_sdk_derive_identity_keys_from_mnemonic (lines 717–731), waits for derived keys, and writes them back to Keychain (lines 796–800). The comment on lines 654–658 makes this explicit.
The justification ("watch-only wallets fail because Rust has no in-process xpriv") is real, but the architectural fix is a single Rust FFI entry point that owns the whole pipeline — e.g. invoking a Swift mnemonic_resolver callback for walletId so the seed never lives in Swift String memory (which, as the comment on lines 707–712 notes, can't be zeroed) and never crosses the FFI as a derivation input. Swift's role should be limited to the final (path_string, 32_private_key_bytes) → Keychain write, per the guideline.
This same shape would also let registerIdentityFromAddresses(...signer:) stop requiring the caller to hand-roll IdentityPubkey rows up front: Rust can derive + persist + register in one shot.
As per coding guidelines: "Do not fetch the mnemonic from Keychain, hand it back to Rust, wait for derived bytes, and write those to Keychain—orchestrate the entire pipeline as a single FFI entry point in Rust instead" and "Do not pull mnemonics or seeds across the FFI boundary for Swift to complete an operation Rust already knows how to complete".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`
around lines 697 - 815, prePersistIdentityKeysForRegistration currently reads
the mnemonic into Swift and passes it into
dash_sdk_derive_identity_keys_from_mnemonic then writes derived private keys
back to Keychain, which violates the "no mnemonic round-tripping" guideline;
instead implement a single Rust FFI entry (e.g.
dash_sdk_derive_and_persist_identity_keys) that performs derivation and
persistence inside Rust and accepts a Swift-provided mnemonic_resolver callback
(invoked by Rust with walletId to fetch mnemonic securely without returning it
to Swift as a String) so Swift no longer calls
dash_sdk_derive_identity_keys_from_mnemonic or reads the mnemonic in
prePersistIdentityKeysForRegistration and instead only invokes the new FFI to
request derivation/persistence; update prePersistIdentityKeysForRegistration and
registerIdentityFromAddresses to call the new FFI and remove direct uses of
mnemonic.withCString, dash_sdk_derive_identity_keys_from_mnemonic, and
KeychainManager.storeIdentityPrivateKey for the derived private bytes.
| public init(from decoder: Decoder) throws { | ||
| let c = try decoder.container(keyedBy: CodingKeys.self) | ||
| self.identityId = try c.decode(String.self, forKey: .identityId) | ||
| self.keyId = try c.decode(UInt32.self, forKey: .keyId) | ||
| self.walletId = try c.decode(String.self, forKey: .walletId) | ||
| self.identityIndex = try c.decode(UInt32.self, forKey: .identityIndex) | ||
| self.keyIndex = try c.decode(UInt32.self, forKey: .keyIndex) | ||
| self.derivationPath = try c.decode(String.self, forKey: .derivationPath) | ||
| self.publicKey = try c.decode(String.self, forKey: .publicKey) | ||
| self.publicKeyHash = try c.decodeIfPresent(String.self, forKey: .publicKeyHash) ?? "" | ||
| self.keyType = try c.decodeIfPresent(UInt8.self, forKey: .keyType) ?? 0 | ||
| self.purpose = try c.decodeIfPresent(UInt8.self, forKey: .purpose) ?? 0 | ||
| self.securityLevel = try c.decodeIfPresent(UInt8.self, forKey: .securityLevel) ?? 0 | ||
| } |
There was a problem hiding this comment.
Default-to-zero on missing descriptor fields silently mislabels old rows as MASTER/AUTHENTICATION/ECDSA.
For pre-migration rows that lack keyType/purpose/securityLevel, we now fill them with 0 — which the rest of the SDK reads back as MASTER security level + AUTHENTICATION purpose + ECDSA_SECP256K1 key type. The doc comment acknowledges this ("a sane 'I don't actually know' fallback"), but if any consumer (e.g. the diagnostic explorer or a future predicate) trusts these fields, it will display incorrect descriptors for legacy rows rather than rendering "unknown".
If the fields are purely diagnostic, consider either: making them Optional<UInt8> on the struct surface (so missing == nil) and only defaulting in display code; or stamping a sentinel like UInt8.max to signal "not recorded". Otherwise, this is a tradeoff worth confirming with the team.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`
around lines 557 - 570, The decoder is defaulting missing
keyType/purpose/securityLevel to 0 which mislabels legacy rows; update the model
so keyType, purpose, and securityLevel are Optional<UInt8> (change their
property types) and in init(from decoder:) remove the "?? 0" defaults and use
decodeIfPresent(...) so missing values stay nil; then update any consumers of
Keychain descriptor fields (references to keyType, purpose, securityLevel) to
handle optional values or map nil to a display sentinel (e.g., "unknown") where
appropriate.
| private func nextUnusedIdentityIndex(for walletId: Data) -> UInt32 { | ||
| let used = usedIdentityIndices(for: walletId) | ||
| guard let highest = used.max() else { return 0 } | ||
| return highest &+ 1 | ||
| } |
There was a problem hiding this comment.
&+ 1 silently wraps UInt32.max to 0, which is always a collision.
If usedIdentityIndices ever returns UInt32.max as the highest value, the wrapping-add lands on 0, the picker pre-fills index 0, and canSubmit will hard-disable the button (collision). The user is left with an unexplained "submit doesn't work" state — the only escape is manual stepper increments past the wrap.
Realistically unreachable, but a checked_add-equivalent that surfaces a user-visible "no free slots" hint would degrade more gracefully than a silent wrap into a collision. At minimum, prefer + 1 (trapping in debug) or guard:
🛡️ Proposed defensive fix
private func nextUnusedIdentityIndex(for walletId: Data) -> UInt32 {
let used = usedIdentityIndices(for: walletId)
guard let highest = used.max() else { return 0 }
- return highest &+ 1
+ return highest == UInt32.max ? UInt32.max : highest + 1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func nextUnusedIdentityIndex(for walletId: Data) -> UInt32 { | |
| let used = usedIdentityIndices(for: walletId) | |
| guard let highest = used.max() else { return 0 } | |
| return highest &+ 1 | |
| } | |
| private func nextUnusedIdentityIndex(for walletId: Data) -> UInt32 { | |
| let used = usedIdentityIndices(for: walletId) | |
| guard let highest = used.max() else { return 0 } | |
| return highest == UInt32.max ? UInt32.max : highest + 1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`
around lines 753 - 757, The current nextUnusedIdentityIndex(for:) uses wrapping
add (&+ 1) which wraps UInt32.max to 0 causing silent collisions; change
nextUnusedIdentityIndex(for:) to return an optional UInt32 (or propagate an
error) by computing used.max() and if highest == UInt32.max return nil (no free
slots), otherwise return highest + 1 using a checked add (+). Update all callers
(e.g., the picker prefill and canSubmit logic) to handle the nil case by showing
a user-visible "no free slots" state and disabling submission rather than
letting a silent wrap cause a collision; reference
nextUnusedIdentityIndex(for:), usedIdentityIndices(for:), and canSubmit when
updating call sites.
CI fix: - IdentityManager: replace manual Default impl with #[derive(Default)] (clippy::derivable_impls). Tighten an existing test assertion that also tripped clippy::unnecessary_get_then_check. Security / robustness (Major review hits): - platform-wallet-ffi: zeroize the inline private_key_bytes scalar AND scrub the WIF C-string buffer in place before each IdentityKeyPreviewFFI row is freed, in both platform_wallet_derive_identity_keys_for_index_free and dash_sdk_derive_identity_keys_from_mnemonic_free. Previously the freed allocation could be reused by the allocator with the cryptographic key material still recoverable. - rs-sdk-ffi signer: Signer<PlatformAddress>::sign_create_witness now rejects P2SH addresses with a clear ProtocolError instead of silently mislabeling them as P2pkh witnesses. The sign / can_sign_with paths still accept P2SH at the discriminant layer, but witness construction is the one place mislabeling would produce a structurally invalid transition. - ManagedPlatformWallet.swift: replace the two `precondition(...)` process-aborts on empty inputs in transferCreditsToAddresses and registerIdentityWithFunding with `guard ... throw PlatformWalletError.invalidParameter` so a UI caller can recover. Defensive guards (Minor review hits): - registration.rs: register_identity_with_funding_external_signer now defensively checks that keys_map[0] is a MASTER / AUTHENTICATION key before submitting; doc rewritten to explain that the IdentityCreate transition signature itself requires MASTER (the asset-lock-spend signature is keyed off asset_lock_private_key, not keys_map[0]). - identity_withdrawal.rs: validate the recipient DashAddress against wallet.platform().network() inside the with_item callback (via Address::require_network) so a mainnet-vs-testnet mismatch fails fast at the FFI boundary with ErrorInvalidParameter instead of surfacing as an opaque downstream error. Documentation cleanup (Minor review hits): - identity_keys_from_mnemonic.rs: rewrite both module and function docs so the zeroization story matches reality — only the 64-byte seed is wrapped in Zeroizing; intermediate ExtendedPrivKeys rely on secp256k1::SecretKey's drop path; the _free path additionally zeroizes the inline private_key_bytes and the WIF buffer. - update.rs: document the cache-stale-after-broadcast invariant on update_identity_with_external_signer so callers know to invoke refresh_identity() before a second update on the same identity. Frontend safety (Minor review hits): - CreateIdentityView.swift nextUnusedIdentityIndex: use checked addition instead of `&+ 1` so a wallet with UInt32.max as its highest used index doesn't silently wrap to 0 (which would always collide with the existing slot). - DashModelContainer.swift: thread DashMigrationPlan.self into both the persistent and in-memory ModelContainer constructors so a future schema bump only needs to add a stage to the plan, not remember to wire the plan into every container construction call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CI failures from the previous push:
1. Verify explorer covers all SwiftData models — the script greps the
whole DashModelContainer.swift file for `Foo.self` to extract model
types, and the new `migrationPlan: DashMigrationPlan.self` argument
I added to the ModelContainer constructor was being treated as a
model type. Scope the extraction to inside the `var modelTypes`
computed property body via awk so other `.self` references
elsewhere in the file (migration plan, version schema) don't trip
it. Verified locally: 18/18 models reported covered.
2. Find unused dependencies (cargo-machete) — `indexmap` was a
leftover dep in both `platform-wallet` and `platform-wallet-ffi`
from the pre-restructure `IdentityManagerStartState.{identities,
watched_identities}` IndexMaps. The two-bucket restructure
replaced those with BTreeMaps; no source code references
`indexmap` anymore (only stale README/PLAN docs). Drop both deps.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3541 +/- ##
============================================
+ Coverage 88.19% 88.29% +0.10%
============================================
Files 2474 2474
Lines 298726 300927 +2201
============================================
+ Hits 263447 265707 +2260
+ Misses 35279 35220 -59
🚀 New features to boost your workflow:
|
|
✅ 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: "3f03c45bd7cd5ff5596620aba2c77d860653ff1689e89f0acaf109e5a8c04988"
)Xcode manual integration:
|
2 participants
Issue being fixed or feature implemented
Migrates every iOS-exercised identity flow off the wallet-internal
IdentitySigner(which requires the seed in Rust process memory and panics inside async runtimes viablocking_read) and onto the externalKeychainSigner— a Swift class that owns adash_sdk_signer_create_with_ctxhandle and routes every signature through the iOS Keychain. The seed stops crossing the FFI for any signing path.Also fixes a long string of cascading bugs that surfaced once the architecture realigned: identity public keys not loading at startup, key-security-level rule violations on document state transitions, the DashPay system contract not being available to the proof verifier, registration index picker treating identity slots like a fixed-size address pool, post-registration UI not refreshing.
Built and verified end-to-end on iPhone 17 Pro / iOS 26.3 simulator: Create Identity → Register DPNS Name → Set DashPay Profile all complete on a watch-only-restored wallet.
What was done?
KeychainSigner + signer-handle FFI
SignAsyncCallbackinrs-sdk-ffito pass raw pubkey bytes + key-type tag instead of a bincode-encodedIdentityPublicKey. Swift trampoline does a SwiftData lookup onPersistentPublicKey.publicKeyData→privateKeyKeychainIdentifier→ Keychain fetch → sign.KeychainSignerSwift class wraps adash_sdk_signer_create_with_ctxhandle. Implements bothSigner<IdentityPublicKey>andSigner<PlatformAddress>viaVTableSignerprojection — same handle, two trait views, dispatched on the trampoline'skey_typebyte (0..4→ identity-key lookup;0xFF→ platform-address-hash lookup).dash_sdk_sign_with_mnemonic_and_pathFFI for on-demand derive+sign of platform-address keys. Both seed and derived-key buffersZeroizing-wrapped Rust-side. BIP-39 wordlist auto-detected.Identity flows migrated to external signers
register_from_addressestakes both signers. Pubkeys threaded in from caller-suppliedIdentityPubkeyFFIarray. Result identity'spublic_keyspopulated from the placeholder so in-memoryManagedIdentityhas its keys immediately after registration.platform_wallet_register_dpns_name_with_signer.sdk.*(rs-sdk-ffi); future sweep when consolidating onto the platform-wallet path.Identity-restore wiring + DPP key-security-level rule
WalletRestoreEntryFFIcarries[IdentityKeyRestoreFFI]rows per identity. Cold-start populatesIdentity.public_keysdirectly — no more empty-keys placeholders waiting on a sync round.DashPay contract auto-loading
dashpay-contract,withdrawals-contract,wallet-utils-contract,token-history-contract,keywords-contractfeatures tors-sdk-trusted-context-providerinrs-sdk-ffi/Cargo.toml. All system contracts now auto-load when the proof verifier asks for them. Fixes "unknown contract idBwr4...NS1C7in document verification" for DashPay flows.Keychain metadata + on-demand platform-address signing
IdentityPrivateKeyMetadatagainedkeyType,purpose,securityLevelfields.publicKeyHashnow actually computed (RIPEMD160(SHA256) via newplatform_wallet_hash160FFI). Codable migration usesdecodeIfPresentdefaults so older rows decode cleanly.prePersistPlatformAddressPrivateKeys*,KeychainManager.{store,retrieve,delete}PlatformAddressPrivateKey, theIdentityRestoreFFI.is_watchedfield, theplatform_address_privkey.<hash>keychain account convention.CreateIdentityView UX
max(used identity_index) + 1. HD derivation is unbounded; there's no pool to deplete. Collision detection on the input renders red and disables submit.RegisterNameViewgained anonRegisteredcallback so the parent'sdpnsNames@Stateupdates immediately without leave-come-back.How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-ffi -p rs-sdk-ffi→ clean.cargo test -p platform-wallet --lib→ 110 passed.cargo test -p platform-wallet-ffi --lib→ 48 passed.cargo test -p rs-sdk-ffi --lib→ 252 passed.cd packages/swift-sdk && ./build_ios.sh --target sim→ BUILD SUCCEEDED.xcodebuild -project SwiftExampleApp/SwiftExampleApp.xcodeproj -scheme SwiftExampleApp -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 17' -quiet build→ BUILD SUCCEEDED.Breaking Changes
Yes. Breaking for downstream consumers of
platform-wallet,platform-wallet-ffi, andrs-sdk-ffi:SignAsyncCallback/CanSignCallbacksignatures inrs-sdk-ffichanged shape — pass raw pubkey + key-type instead of bincodedIdentityPublicKey.dash_sdk_signer_creategot a_with_ctxsibling for context-attached signers.key_typeparameter ondash_sdk_signer_create_from_private_keyanddash_sdk_signer_sign.WalletRestoreEntryFFIgainedkeys: *const IdentityKeyRestoreFFI+keys_count. Older callers that allocate this struct must zero the new fields.platform_wallet_register_identity_with_signerswappedkey_count: u32foridentity_pubkeys: *const IdentityPubkeyFFI+identity_pubkeys_count. Two signer handles instead of one.IdentityPrivateKeyMetadataCodable shape gained 3 fields (defaulted on decode).Persistent*models internally storenetworkasIntshadow + computedAppNetworkaccessor (predicate-engine workaround); the public API shape ofnetworkasAppNetworkis unchanged.platform_wallet_register_identity_from_addresseswas deleted in the previous PR;register_from_addressesis now the canonical name._with_signervariants of every flow added; legacy IdentitySigner-based methods marked Superseded but kept for now.Signer<PlatformAddress> for PlatformAddressWalletimpl deleted entirely (was the "compiles, ships, breaks at runtime" trap).Checklist:
swift-sdk/CLAUDE.mdrules expanded)For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation