feat: identity registration with asset-lock proofs

[shumkov]

Issue being fixed or feature implemented

SwiftExampleApp could only create a Platform identity from an existing asset-lock proof — there was no path to register an identity directly from a Core (SPV) wallet balance. This PR adds that flow end-to-end, with ExternalSignable wallets (no root xpriv on the Rust side) and an automatic IS-lock → ChainLock fallback when InstantSend doesn't show up.

Status: validated end-to-end on testnet through Phase 2. Identity successfully registered from Core funds with both IS-lock (3.5 s after broadcast) and ChainLock paths confirmed working. Phase 3 (stuck-asset-lock catch-up, CL-height retry hardening, and 23 post-review fixes) builds cleanly but the latest review-fix sweep has not yet been re-validated live on testnet.

What was done?

End-to-end Core-funded identity registration with no private-key material crossing the FFI boundary, plus follow-on fixes that took it from "compiles" to "works on testnet" and then "recovers from every edge case we found."

Phase 1 — core feature (initial 5 commits):

• rs-dpp + rs-sdk + rs-sdk-ffi (992be090): StateTransition::sign_with_signer<S: key_wallet::signer::Signer>; renamed try_from_identity_with_signer → try_from_identity_with_signer_and_private_key + new try_from_identity_with_signers<IS, AS> sibling; mirror split for top-up; ProtocolError::ExternalSignerError(String); put_to_platform_with_signer / broadcast_request_for_new_identity_with_signer / top_up_identity_with_signer on the SDK side. Rename ripple across rs-sdk-ffi, wasm-sdk, rs-drive-abci, strategy-tests.
• rs-platform-wallet-ffi: MnemonicResolverCoreSigner trampoline (203067259): implements key_wallet::signer::Signer by deferring to a Swift-side MnemonicResolver vtable. Each Core ECDSA call atomically fetches mnemonic → derives key → signs digest → zeroes buffers. No private-key bytes ever leave Swift. Typed MnemonicResolverSignerError.
• rs-platform-wallet refactor (f1a7d1c2): IdentityFunding enum (FromWalletBalance, FromExistingAssetLock, UseAssetLock) replaces the dual IdentityFundingMethod/TopUpFundingMethod pair. L1/L2 merge: single register_identity_with_signer (signer + proof + path + keys_map) + single register_identity_with_funding (funding-source orchestration). IS→CL fallback (180 s) and H3 cleanup-on-success centralized. build_asset_lock_transaction<S: Signer> / create_funded_asset_lock_proof<S: Signer> return (_, DerivationPath) — credit-output privkey no longer leaves the wallet.
• swift-sdk wrappers (9d5e506a): registerIdentityWithFunding(amountDuffs:identityIndex:identityPubkeys:signer:) with internal MnemonicResolver() and withExtendedLifetime so ARC can't release the resolver mid-FFI. ManagedAssetLockManager.buildTransaction / createFundedProof take an external MnemonicResolver. KeychainManager delete-query fix (was using kSecValueData as a filter → errSecDuplicateItem).
• SwiftExampleApp (8a57e882): CreateIdentityView Core-account branch + plan doc.

Phase 2 — end-to-end unblock (after Phase 1 testnet attempt):

• 885a1be3 — fix(platform-wallet-ffi): always enable masternode sync for SPV
  Real root cause for the "tx never IS-locks" symptom. In trusted-SDK mode the app set masternodeSyncEnabled=false, which disabled dash-spv's ChainLockManager + InstantSendManager. The SPV client connected to masternode peers and received CLSig/ISLock P2P messages, but with no manager subscribed, MessageDispatcher dropped them. Removed the FFI knob entirely; hardcoded enable_masternodes = true. Asset-lock proofs are a published platform-wallet feature; the IS/CL subscription is a non-optional dependency.
• 4184a425 — chore: bump rust-dashcore to 5297d61a for chainlock wallet handling
  Picks up feat(key-wallet): add chainlock handling to the wallet rust-dashcore#756 which adds WalletInterface::process_chain_lock, promotes records InBlock → InChainLockedBlock on chainlock arrival, and emits a new WalletEvent::TransactionsChainlocked variant. Without this the chainlock fallback branch of wait_for_proof could never resolve — records were stuck at InBlock(_) forever. Match-arm coverage added in core_bridge + balance_handler for the new variant.
• 3d16a31a — fix(SwiftExampleApp): bump identity funding floor to v1 minimum for 3 keys
  Platform rejected v1 identity-creates funded at the v0 floor (200_000 duffs) because v1's calculate_min_required_fee_v1 adds per-key creation cost. With defaultKeyCount = 3 the real floor is 221_500 duffs (2_000_000 base + 3 × 6_500_000 per-key, all in credits, ÷ 1000 credits/duff). Bumped minIdentityFundingDuffs to 221_500 and defaultCoreFundingDuffs to 250_000.
• 34d702d3 — docs(swift-sdk): mark SPV event-routing follow-up resolved — plan doc resolution note.

Phase 3 — stuck-asset-lock catch-up + retry hardening + post-review cleanup:

The Phase 2 testnet run worked for fresh registrations but exposed a recoverability gap: an asset lock that was broadcast before the app was killed (or that received its IS-lock / chainlock during a session the app was not running) sat at Broadcast forever after relaunch. Resolving that surfaced a chain of issues, each of which became a commit:

• f65e2e4351 fix(platform-wallet): persist chain-lock context promotions to Swift via bridge — Rust-side WalletEvent::TransactionsChainlocked was emitted but the changeset bridge wasn't projecting the per-record InBlock → InChainLockedBlock flip into the PlatformWalletChangeSet Swift consumes. Added chain_lock_promotions to the core changeset and wired the projection.
• d404cd0caf feat(platform-wallet-ffi): restore tx records for unresolved asset locks at load + 5aa9e9ad5f feat(swift-sdk): project tx records for unresolved asset locks into restore entry — at app launch, Swift now hands the funding-tx record (with persisted BlockInfo context) back to Rust as part of the wallet restore entry, so the in-memory transactions() map has something for the chainlock cascade to promote. Without this, restored asset locks at Broadcast had no in-memory record for apply_chain_lock to find, so the cascade silently no-op'd.
• 67f5962012 feat(platform-wallet): background catch-up for stuck asset locks — new fire-and-forget FFI asset_lock_manager_catch_up_blocking + Swift PlatformWalletManager.catchUpStuckAssetLocks that walks every PersistentAssetLock with statusRaw < 2 and spawns Task.detached calls into resume_asset_lock, parked on wait_for_proof. The chain-lock cascade then fires on the next CLSig without any user interaction.

Phase 3.5 — what the in-flight working tree adds on top (this session, not yet split into commits):

After Phase 3's four commits a second round of testnet inspection turned up several deeper issues. Captured here for context; full commit will follow review:

• The actual root cause for "stuck asset lock chore(wallet-lib): upgrade webpack to v.5 #10": PlatformWalletInfo is a wrapper around upstream's ManagedWalletInfo and delegated ~20 WalletInfoInterface methods (network, wallet_id, balance, sync heights, etc.) but silently missed apply_chain_lock and last_applied_chain_lock. Both fell through to the trait's default no-op. Upstream's spawn_chainlock_wallet_dispatch task was calling wallet.write().await.apply_chain_lock(...) for every validated CL, but it was hitting the no-op — metadata.last_applied_chain_lock stayed None, records never got promoted, the cascade never reached wait_for_proof. Two-line fix: delegate both methods.
• CL-height-too-low retry path on identity registration. A wallet that observes a fresh ChainLock locally can race Platform's view of the same height; submitting too early returns InvalidAssetLockProofCoreChainHeightError (consensus code 10506). New submit_with_cl_height_retry helper in registration.rs retries every 15s for 210s (matching mainnet's create-empty-blocks-interval = 3m), bumping PutSettings::user_fee_increase per retry. Tenderdash's mempool caches rejected ST hashes for ~24h on mainnet/testnet (keep-invalid-txs-in-cache = true in dashmate's tenderdash/config.toml.dot), so retries must produce distinct signable bytes to bypass the cache. Critical follow-up: the original wiring was a no-op because broadcast_request_for_new_identity_with_signer hardcoded user_fee_increase = 0 (packages/rs-sdk/src/platform/transition/broadcast_identity.rs:171) — the threading was missing through the trait method. Symmetric fix to both private-key and signer variants now propagates PutSettings::user_fee_increase end-to-end.
• Wallet trust hierarchy made explicit. The retry path no longer queries Platform's self-reported core_chain_locked_height anywhere. That metadata is unproven and a malicious DAPI node could stall us indefinitely. The wallet's last_applied_chain_lock (SPV-verified BLS signature, cryptographic) is the only signal trusted. Removed get_platform_core_chain_locked_height and its call sites; submission is now optimistic and reacts to Platform's deterministic 10506 rejection.
• Wallet-side CL fallback in wait_for_proof — if the per-record context didn't promote (race between SPV's apply_chain_lock and the catch-up task entering wait_for_proof), the fallback uses wallet.last_applied_chain_lock.block_height >= record.height() to build a Chain proof directly. Gated on a chain-id match (wallet.network() == sdk.network) so a misconfigured wallet (network drift / restore from wrong-network backup) can't synthesize a proof on the wrong chain.
• AssetLockStatus::Consumed terminal variant. The previous remove_asset_lock deleted the row from both Rust's in-memory tracked_asset_locks and Swift's PersistentAssetLock table. The deletion broke historical UI lookups: the "Transactions" list couldn't map a consumed funding tx back to its locked amount. New semantics: Rust drops from memory (terminal — no more proof work to do); Swift retains the row at statusRaw = 4 "Consumed" for the lifetime of the wallet. Catch-up scanner and "ready to fund" UI continue to filter < InstantSendLocked so Consumed entries don't generate noise. Renamed the function remove_asset_lock → consume_asset_lock to match.
• PlatformWalletError::FinalityTimeout(OutPoint) — was FinalityTimeout(Txid). The full outpoint now flows from wait_for_proof through resolve_funding_with_is_timeout_fallback directly, eliminating a BTreeMap-iteration-order-dependent find_tracked_unproven_lock helper that could pick the wrong unproven lock when multiple were tracked at the same (funding_type, identity_index).
• Wallet's last-applied chain-lock exposed via FFI. New has_last_applied_chain_lock / last_applied_chain_lock_height / last_applied_chain_lock_block_hash[32] fields on CoreWalletStateFFI; Swift CoreWalletStateSnapshot.lastAppliedChainLockHeight: UInt32? and lastAppliedChainLockBlockHash: Data?. Currently in-memory only — not persisted across app restarts.
• Transaction-list UI for asset-lock rows. Asset-lock txs were reading as "Internal transaction to myself" with +0.00000000 DASH in red. The Rust classifier already labeled them TransactionType::AssetLock but the Swift row picked an icon from direction (which was Internal) and an amount from transaction.netAmount (which is ~0 because the credit output is a self-owned address). Fixed: purple lock icon (lock.fill), displayDirection returns "Asset Lock" / "Asset Unlock" instead of "Internal", row joins PersistentAssetLock.amountDuffs by txid (summing across vouts) and renders the actual L1 DASH burned; falls back to "Asset Lock (amount unknown)" if the linked row is missing. Same treatment in TransactionDetailView.
• Multi-reviewer code audit + 20 of 23 findings addressed. Spawned ce-correctness-reviewer / ce-adversarial-reviewer / ce-maintainability-reviewer / rust-quality-engineer / silent-failure-hunter against the working tree. Findings ranged from "load-bearing CRITICAL" (the user_fee_increase no-op) down to LOW cleanups (stray TODO comment). The 3 deliberately skipped are documented in code comments (FundingResolution refactor: out of scope; saturating_add(1): defensive only inside the budget; #[non_exhaustive] on AssetLockStatus: would force wildcard arms and lose the compile-time signal for new variants).
• Fix for Found-008 (Found-008: LockNotifyHandler::notify_waiters() drops lock events arriving in wait_for_proof's check/await gap (concurrent asset-lock builds stall on FinalityTimeout) #3641): LockNotifyHandler missed-wakeup race in wait_for_proof. The tokio::sync::Notify API has a well-known foot-gun: notify_waiters() only wakes currently-registered waiters and does NOT store a permit. The wait_for_proof loop checks state, then calls lock_notify.notified().await — an IS/CL event arriving in that gap is silently dropped, and the next event never comes for that specific txid, so the wait stalls until FinalityTimeout. Fix uses the canonical Notified::enable() pattern (Option C in my comment on Found-008: LockNotifyHandler::notify_waiters() drops lock events arriving in wait_for_proof's check/await gap (concurrent asset-lock builds stall on FinalityTimeout) #3641 — different from the issue body's Option A/B): arm the future BEFORE the state check, so any subsequent notify_waiters() is captured by the pinned future. ~10 lines per loop site (applied to both wait_for_proof and wait_for_chain_lock), preserves the multi-waiter semantics that notify_one would break, no API change to LockNotifyHandler. Closes Found-008: LockNotifyHandler::notify_waiters() drops lock events arriving in wait_for_proof's check/await gap (concurrent asset-lock builds stall on FinalityTimeout) #3641 (Found-008 / AL-001). The AL-001 e2e regression test in PR test(platform-wallet): e2e framework + 60-test suite, 5 upstream bug pins #3549 will pin the fix once that PR merges.

How Has This Been Tested?

• cargo test --workspace green for rs-dpp / rs-sdk / rs-platform-wallet (124/124 in the lib tests).
• Testnet end-to-end identity registration confirmed working (Phase 2 validation): IS-lock arrived 3.5 s after broadcast; live event tracing showed every wiring channel firing (PlatformEventManager::on_sync_event → LockNotifyHandler → wait_for_proof poll → context flip → InstantAssetLockProof emitted → Platform IdentityCreateTransition accepted).
• Stuck-asset-lock catch-up confirmed working in the working-tree session: slot chore(wallet-lib): upgrade webpack to v.5 #10 (broadcast pre-restart, on-chain txlock=true, 538 confirmations) advanced from statusRaw=1 to statusRaw=3 with a populated proofBytes after the apply_chain_lock delegation fix landed — exactly the cascade described above.
• iOS sim build green after every commit and after every review fix (./build_ios.sh --target sim --profile dev).
• The Phase 3.5 review-fix sweep (CL-height retry path actually threading user_fee_increase, Consumed status persisting, the new FinalityTimeout(OutPoint) shape) has not been re-exercised on testnet yet — the build is green but a follow-up identity registration on a fresh wallet is needed to confirm the retry path now bypasses Tenderdash's hash cache as intended.

Breaking Changes

• Removed masternode_sync_enabled parameter from the platform_wallet_manager_spv_start FFI signature and the corresponding masternodeSyncEnabled field on PlatformSpvStartConfig. Callers must drop the argument. Rationale: asset-lock proof acquisition requires it, so the flag was unsafe to expose. Internal-FFI ABI; the Swift SDK in this repo is the only consumer.
• At the Rust public-API level, the prior try_from_identity_with_signer / try_from_identity methods were renamed to _and_private_key / _with_private_key variants alongside new _with_signer(s) siblings (all internal callers updated in the same commit).
• BroadcastRequestForNewIdentity trait signature change: both broadcast_request_for_new_identity_with_private_key and broadcast_request_for_new_identity_with_signer gained a user_fee_increase: UserFeeIncrease parameter. This is the critical fix that makes the retry path actually function — the old hardcoded 0 produced identical ST hashes across retries and got dedup'd at Tenderdash. External implementors of the trait need to thread the parameter through to IdentityCreateTransition::try_from_identity_with_signer{,s}{,_and_private_key}.
• AssetLockStatus gained a Consumed variant. Exhaustive matches in downstream consumers must add an arm. We intentionally did NOT mark the enum #[non_exhaustive] — every cross-crate match site uses exhaustive arms by design so the compiler catches the next variant addition the same way it caught this one.
• PlatformWalletError::FinalityTimeout(Txid) → FinalityTimeout(OutPoint). The variant payload type changed; consumers pattern-matching the variant need to destructure an OutPoint instead of a Txid. .txid field still accessible via the outpoint.
• tracked_asset_locks map semantics changed. Was: removed on consumption. Now: terminal entries are dropped from the in-memory map but the Swift PersistentAssetLock row is retained with statusRaw=4. Load-path filters Consumed entries so they're never re-added to memory. Consumers reading the in-memory map see only still-actionable locks.
• rust-dashcore rev bump (53130869 → 5297d61a) introduces WalletEvent::TransactionsChainlocked which forces match-arm coverage in downstream consumers of the WalletEvent enum.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
• I have made corresponding changes to the documentation if needed

Summary by CodeRabbit

Release Notes

• New Features

  • Added asset lock persistence and resumption for identity registration recovery
  • Added Chain Lock upgrade fallback for instant send timeout scenarios
  • Added registration progress tracking and visibility in the app

• Improvements

  • Simplified SPV startup by making masternode sync mandatory for asset lock proof acquisition
  • Enhanced Chain Lock support throughout Platform wallet operations
  • Improved asset lock state tracking and display in Storage Explorer

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d00ccf70-2ba0-4788-bc6e-a8e62f25c93d

📥 Commits

Reviewing files that changed from the base of the PR and between e01fc9d and 710de74.

📒 Files selected for processing (4)

• packages/rs-platform-wallet-ffi/Cargo.toml
• packages/rs-platform-wallet-ffi/src/core_wallet_types.rs
• packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift

 ________________
< CSI: Git Diff. >
 ----------------
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

📝 Walkthrough

Walkthrough

This PR refactors identity registration and top-up to external-signer flows by establishing a shared mnemonic-resolver FFI layer in rs-sdk-ffi, implementing a core-signer bridge that derives BIP32 keys and signs digests without exposing private keys to Swift, splitting identity-related SDK traits into separate private-key and signer variants with explicit user-fee-increase threading, and updating all trait call sites to use the new method signatures.

Changes

Mnemonic Resolver FFI and Core Signer

Layer / File(s)	Summary
Mnemonic resolver FFI infrastructure packages/rs-sdk-ffi/src/mnemonic_resolver.rs	Defines C-callback contract, result-code enum, vtable and handle types for Swift to provide mnemonic-resolver context; exports dash_sdk_mnemonic_resolver_create and dash_sdk_mnemonic_resolver_destroy to manage handle lifecycle.
Core signer ECDSA implementation packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs	Implements Signer trait by resolving BIP39 mnemonic via the handle, deriving BIP32 private key for a given path, signing 32-byte digests, computing public keys, and wrapping all key material in Zeroizing; includes typed error enum and comprehensive tests for round-trip signing and error propagation.
Module organization packages/rs-sdk-ffi/src/lib.rs, packages/rs-platform-wallet-ffi/src/lib.rs	Declare mnemonic_resolver and mnemonic_resolver_core_signer modules in rs-sdk-ffi and re-export their public APIs; add platform_wallet and platform_wallet_ffi to logging filter; declare asset_lock_persistence module in platform-wallet-ffi.
Migrate resolver definitions packages/rs-platform-wallet-ffi/src/derive_and_persist_callbacks.rs	Remove mnemonic-resolver C-ABI exports (buffer constant, result codes, callback/vtable/handle types, create/destroy functions) from this module; keep identity-key persister as the sole export; update tests to remove resolver coverage and focus on persister-only paths.
Update import sources packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs, packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs	Redirect mnemonic-resolver type/constant imports from crate::derive_and_persist_callbacks to rs_sdk_ffi; update test helper callback type annotation accordingly.
Split broadcast identity traits packages/rs-sdk/src/platform/transition/broadcast_identity.rs	Remove single broadcast_request_for_new_identity method; add broadcast_request_for_new_identity_with_private_key (accepts settings, derives user_fee_increase); add broadcast_request_for_new_identity_with_signer (core_key_wallet-gated, accepts asset_lock_proof_path and two signers); implement both for Identity.
Split top-up identity traits packages/rs-sdk/src/platform/transition/top_up_identity.rs	Rename top_up_identity to top_up_identity_with_private_key (derives user_fee_increase from settings); add top_up_identity_with_signer (core_key_wallet-gated, accepts asset_lock_proof_path and external signer); implement both for Identity.
Update FFI call sites packages/rs-sdk-ffi/src/identity/topup.rs	Update instant-lock and instant-lock-and-wait FFI entry points to call top_up_identity_with_private_key(...) instead of top_up_identity(...), passing settings directly and removing separate user_fee_increase argument extraction.

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• refactor(wallet): unify all asset lock paths and add transaction recovery dash-evo-tool#799: Implements unified, signer-driven asset-lock flows with persistent tracked-asset-lock records and derivation-path-based recovery — directly addressing code-level asset-lock recovery objectives.

Possibly related PRs

• dashpay/platform#3542: Introduced the resolver/persister callback-handle and derive+sign FFI pipeline that this PR builds upon and refactors into rs-sdk-ffi.

Poem

🐰 A resolver in Rust, a signer so true,
Keys stay in Swift while we sign on through,
No private keys flowing, just digests and paths,
Identity registration follows the Math! 🔐

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/swift/funding-with-asset-lock

[thepastaclaw]

Review Gate

Commit: 710de74a

• ⏳ Debounce: 3m ago (need 30m)

• ✅ CI checks: builds passed, 0/0 tests passed

• ✅ CodeRabbit review: comment found

• ✅ Off-peak hours: off-peak (01:33 AM PT Friday)

• Run review now (check to override)

[coderabbitai]

Actionable comments posted: 9

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/Sources/SwiftDashSDK/Security/KeychainManager.swift (1)

165-165: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same delete-then-add bug still present in storePrivateKeyNonisolated and storeKeyData.

The fix at lines 627-645 acknowledges that including kSecValueData/kSecAttrGeneric in a SecItemDelete query causes stale rows to survive when the new payload differs, ultimately leading to errSecDuplicateItem on the subsequent SecItemAdd. The same pattern is still present in two sibling functions and will fail identically under the same conditions:

• storePrivateKeyNonisolated (line 165): SecItemDelete(query as CFDictionary) is called with the full query that includes kSecValueData (line 143) and kSecAttrGeneric (line 156). This path is reachable via storePrivateKey (line 108) and is still the legacy identity-id+keyIndex writer.
• storeKeyData (line 381): SecItemDelete(query as CFDictionary) is called with a query that includes kSecValueData (line 372). This is the backing store for storeSpecialKey (line 287).

Apply the same attribute-only delete query pattern in both functions so the fix is consistent across the writer surface.

🛡️ Proposed fix for both sibling functions

@@ storePrivateKeyNonisolated @@
-        // Delete any existing item first
-        SecItemDelete(query as CFDictionary)
+        // Delete any existing item first. Use an attribute-only
+        // query so kSecValueData/kSecAttrGeneric don't act as
+        // value filters and leave stale rows behind on update.
+        var deleteQuery: [String: Any] = [
+            kSecClass as String: kSecClassGenericPassword,
+            kSecAttrService as String: serviceName,
+            kSecAttrAccount as String: keyIdentifier
+        ]
+        if let accessGroup = accessGroup {
+            deleteQuery[kSecAttrAccessGroup as String] = accessGroup
+        }
+        SecItemDelete(deleteQuery as CFDictionary)

@@ storeKeyData @@
-        SecItemDelete(query as CFDictionary)
+        var deleteQuery: [String: Any] = [
+            kSecClass as String: kSecClassGenericPassword,
+            kSecAttrService as String: serviceName,
+            kSecAttrAccount as String: identifier
+        ]
+        if let accessGroup = accessGroup {
+            deleteQuery[kSecAttrAccessGroup as String] = accessGroup
+        }
+        SecItemDelete(deleteQuery as CFDictionary)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift` at
line 165, The SecItemDelete call in storePrivateKeyNonisolated and storeKeyData
currently uses the full query that contains kSecValueData (and kSecAttrGeneric
in storePrivateKeyNonisolated), which can leave stale rows and cause
errSecDuplicateItem on SecItemAdd; change both deletes to use an attribute-only
delete query (include kSecClass and the identifying attributes such as
kSecAttrAccount, kSecAttrService, kSecAttrLabel, kSecAttrAccessible, etc., but
omit kSecValueData and kSecAttrGeneric) so the existing item is removed based on
its attributes before calling SecItemAdd, matching the pattern used in the
earlier fix for the other writer.

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift (1)

199-235: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Platform-payment success currently has no terminal UI path.

submitPlatformPayment sets createdIdentityId, but the view only renders completion/failure UI through activeController (terminalSection). For Platform-payment, activeController is never set, so a successful create returns users to the same form with no success confirmation or completion action.

Suggested minimal fix

@@
                 try await MainActor.run {
                     try persistCreatedIdentity(
                         identityId: created.identityId,
                         network: network
                     )
@@
                     try modelContext.save()
                     self.createdIdentityId = created.identityId
                     self.isCreating = false
+                    dismiss()
                 }

Also applies to: 624-629, 844-906

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`
around lines 199 - 235, The view never shows a terminal UI when
submitPlatformPayment succeeds because successful platform payments set
createdIdentityId but never set activeController; update the view logic (around
the conditional rendering that checks activeController / preselectedAssetLock)
to also treat a non-nil createdIdentityId as a terminal state by rendering
terminalSection(for:) (or a small completion-only section) when
createdIdentityId != nil, or alternatively set activeController to a
completion-state controller inside submitPlatformPayment after success;
reference submitPlatformPayment, createdIdentityId, activeController,
RegistrationProgressSection, and terminalSection(for:) to locate code to change
and ensure the submit button path for platform payments either shows the same
success banner and actions or dismisses the sheet appropriately.

🧹 Nitpick comments (12)

packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md (4)

1056-1071: ⚡ Quick win

Update open questions with implementation outcomes.

Several questions in this section were resolved during implementation:

• Line 1058 asks about 100,000 duffs default, but line 1054 documents that defaultCoreFundingDuffs was set to 250,000 duffs in commit 3d16a31a.
• Line 1059-1060 asks about asset-lock minimum, but line 1054 shows minIdentityFundingDuffs was set to 221,500 duffs.

📝 Suggested update

 ## Open questions
 
-- **Default funding amount**: 100,000 duffs (0.001 DASH)?
-- **Asset-lock minimum constant**: name + value, verify <
-  testnet faucet typical payout (per adversarial review W8).
+- **Default funding amount**: ✅ Resolved - set to 250,000 duffs in commit 3d16a31a (iter SPV follow-up).
+- **Asset-lock minimum constant**: ✅ Resolved - `minIdentityFundingDuffs` set to 221,500 duffs (identity_create_base_cost + asset_lock_base * CREDITS_PER_DUFF + identity_key_in_creation_cost * 3 for defaultKeyCount=3).
 - **Key count**: stick with `defaultKeyCount = 3` (1 master + 2
   high), or expose a picker?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md`
around lines 1056 - 1071, Update the "Open questions" section to reflect
resolved decisions: replace the question about default funding amount with a
statement that defaultCoreFundingDuffs was set to 250,000 duffs (commit
3d16a31a), and replace the asset-lock minimum question with a note that
minIdentityFundingDuffs was set to 221,500 duffs; remove or mark as resolved the
items referencing those values (lines mentioning 100,000 duffs and asset-lock
minimum) and keep the remaining open questions (key count and AssetLockStatus
extension) unchanged.

---

147-149: ⚡ Quick win

Update iteration number reference for clarity.

Line 147 states "Resolved in iter 2" but the SwiftData mirror work was actually delivered in iteration 3 (which was renumbered from the original iter 2 after the mid-iter-1 signer discovery). Line 583 confirms this: "### Iter 3 — SwiftData mirror + persister callback (was iter 2)".

📝 Suggested clarification

-**Resolved in iter 2.** From iter 2 onward, a `PersistentAssetLock`
+**Resolved in iter 3 (originally planned as iter 2).** From iter 3 onward, a `PersistentAssetLock`
 SwiftData model mirrors `TrackedAssetLock` via a new FFI
 persister callback.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md`
around lines 147 - 149, Update the iteration reference from "iter 2" to "iter 3"
in the sentence that currently reads "Resolved in iter 2" so it matches the
later note "### Iter 3 — SwiftData mirror + persister callback (was iter 2)";
ensure the wording still indicates that the PersistentAssetLock SwiftData model
mirrors TrackedAssetLock via the FFI persister callback and that this work was
delivered in iteration 3.

---

992-992: 💤 Low value

Clarify "bug repro" scenario intent.

Scenario 2 is labeled "🔴 bug repro" but describes the correct expected behavior ("must NOT appear in Resumable Registrations"). Is this testing a regression (preventing a past bug from returning), or was this a bug discovered during UAT?

📝 Suggested clarification

Consider either:

• If it's a regression test: "Regression guard — Start a fresh registration..."
• If it's a discovered bug: Add a note indicating whether the bug was fixed: "🔴 bug fixed — ..." or "🔴 known issue — ..."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md`
at line 992, Update the wording for Scenario 2 (currently labeled "🔴 bug
repro") to clearly state intent: if this is preventing a previously fixed
regression, rename to "Regression guard — Start a fresh registration..." ; if it
documents a discovered/fixed issue, use "🔴 bug fixed — ..." or "🔴 known issue
— ..." and add a short note whether the bug was fixed; ensure the description
still states that the in-flight slot must not appear in Resumable Registrations
and reference the related code check (Phase.isActive + identity-slot union in
ResumableRegistrationsList, commit 02a15497c6) so readers know the behavioral
contract being validated.

---

417-417: ⚡ Quick win

Update cross-reference to SPV follow-up section.

The reference "See § Iter 5 / SPV event-routing follow-up" points to a section that is actually titled "### SPV event-routing follow-up — RESOLVED (2026-05-13)" at lines 1048-1054, not within iter 5.

📝 Suggested fix

-Diagnosed as either testnet masternode silence or a regression in dash-spv → wallet integration (likely from recent rust-dashcore bumps). **Iter 4's auto IS→CL fallback was triggered but ALSO timed out** because the chain-lock event never propagated to our poll either. See § Iter 5 / SPV event-routing follow-up.
+Diagnosed as either testnet masternode silence or a regression in dash-spv → wallet integration (likely from recent rust-dashcore bumps). **Iter 4's auto IS→CL fallback was triggered but ALSO timed out** because the chain-lock event never propagated to our poll either. See § SPV event-routing follow-up.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md`
at line 417, Update the cross-reference text "See § Iter 5 / SPV event-routing
follow-up" so it points to the actual section heading "SPV event-routing
follow-up — RESOLVED (2026-05-13)" used later in the document; locate the string
in the Step 13 paragraph and replace it with a clear reference matching the
exact heading text ("See § SPV event-routing follow-up — RESOLVED
(2026-05-13)"), or alternatively change the reference to "See § SPV
event-routing follow-up" if you prefer a shorter match that exactly equals the
section title "SPV event-routing follow-up — RESOLVED (2026-05-13)" to ensure
the cross-reference resolves correctly.

.claude/skills/simulator-control/SKILL.md (2)

66-81: ⚡ Quick win

Consider filtering stderr to prevent JSON parse failures.

The current implementation pipes both stdout and stderr (2>&1) into Python's json.loads(). If idb ui describe-all emits any warnings or errors to stderr, the JSON parsing will fail.

🛡️ Suggested hardening

 tap_label() {
     local label="$1"
     local udid=$(xcrun simctl list devices booted | awk -F'[()]' '/Booted/ {print $2}')
-    LABEL="$label" UDID="$udid" "$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>&1 \
+    LABEL="$label" UDID="$udid" "$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>/dev/null \
         | python3 -c "
 import json, os, subprocess, sys
 items = json.loads(sys.stdin.read())

Or capture stderr separately if diagnostics are needed:

local output=$("$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>/tmp/idb_err.log)
LABEL="$label" UDID="$udid" echo "$output" | python3 -c "..."

🤖 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 @.claude/skills/simulator-control/SKILL.md around lines 66 - 81, The piped
command currently redirects stderr into stdout (the "2>&1" around the call to
"$HOME/.local/bin/idb" ui describe-all) so the inline Python (json.loads on
sys.stdin) can receive non-JSON diagnostics and fail; change the shell pipeline
to separate or filter stderr before passing JSON to the python snippet: remove
or modify the "2>&1" redirection on the idb ui describe-all call and instead
capture only its stdout (or redirect stderr to a log/pipe) so the python3 -c
block that calls json.loads(...) receives clean JSON; update references around
LABEL/UDID and the subprocess.run(...) invocation accordingly to still use the
UDID env var and perform the tap.

---

150-152: 💤 Low value

Consider disambiguating when multiple "Resume" buttons exist.

Line 152 taps the first "Resume" button in the accessibility tree. If the UI contains multiple resumable registrations, this may tap an unintended row.

The comment acknowledges this limitation. For more precise automation, consider filtering by both label and frame.y coordinate range, or waiting for the specific outpoint substring to appear in the accessibility tree before tapping its associated Resume button (using the pattern from Workflow C, lines 157-163).

🤖 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 @.claude/skills/simulator-control/SKILL.md around lines 150 - 152, The
tap_label "Resume" call is ambiguous when multiple Resume buttons exist; instead
locate the specific Resume button associated with the intended registration by
first waiting for the unique outpoint substring to appear in the accessibility
tree (as done in Workflow C lines 157-163) or by filtering candidate nodes by
their frame.y coordinate range, then tap that specific node rather than calling
tap_label "Resume" blindly; update the sequence around tap_label "Identities" /
tap_label "Resume" to perform the wait-and-find (or y-range filter) and then
invoke the tap on the resolved node.

packages/rs-platform-wallet/src/wallet/asset_lock/build.rs (1)

110-134: 💤 Low value

Consider validating exactly one credit-output key is returned.

The code handles the zero-keys case with an error but silently drops any extra keys beyond the first via drain().next(). While the builder contract should guarantee a single key per funding output, making this invariant explicit would improve clarity.

💡 Optional validation

 let path = match result.keys {
     AssetLockCreditKeys::Public(mut keys) => {
+        if keys.len() != 1 {
+            return Err(PlatformWalletError::AssetLockTransaction(
+                format!("Builder returned {} credit-output keys; expected exactly 1", keys.len())
+            ));
+        }
         let (_pubkey, path) = keys.drain(..).next().ok_or_else(|| {

🤖 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/src/wallet/asset_lock/build.rs` around lines 110
- 134, The code currently takes the first key from result.keys
(AssetLockCreditKeys::Public) and ignores any extras; change the match arm to
explicitly validate there is exactly one credit-output key: inspect the
collection (e.g., check keys.len() or pattern-match on a single-element Vec) and
return a PlatformWalletError::AssetLockTransaction if it is empty or contains >1
entries, then return the single path; keep the existing error for the Private
variant and still return Ok((result.transaction, path)) on success.

packages/rs-platform-wallet/src/wallet/core/broadcast.rs (1)

39-41: 💤 Low value

Doc link target looks wrong.

The intra-doc link points at crate::wallet::asset_lock::build for MnemonicResolverCoreSigner, but per the PR description that type lives in platform-wallet-ffi, not in this crate's asset_lock::build module. Consider dropping the path link (since the type isn't reachable from this crate) or rewording to plain prose referring to platform-wallet-ffi.

🤖 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/src/wallet/core/broadcast.rs` around lines 39 -
41, The intra-doc link for MnemonicResolverCoreSigner is incorrect: it points to
crate::wallet::asset_lock::build but the type lives in platform-wallet-ffi;
update the doc comment (the lines referencing `MnemonicResolverCoreSigner` and
`crate::wallet::asset_lock::build`) to remove or change the intra-doc path —
either drop the link and refer to the type in plain prose
("MnemonicResolverCoreSigner from platform-wallet-ffi") or point users to the
external crate; ensure you do not keep the unreachable
`crate::wallet::asset_lock::build` link.

packages/rs-platform-wallet/src/wallet/identity/network/registration.rs (3)

419-440: 💤 Low value

Two sequential 180s waits on the IS→CL fallback path may double the user-visible delay.

After upgrade_to_chain_lock_proof(&out_point, CL_FALLBACK_TIMEOUT) succeeds, resume_asset_lock(&out_point, CL_FALLBACK_TIMEOUT) is called purely to re-derive the credit-output path. The inline comment correctly notes this should "short-circuit to the existing-proof branch" — but the CL_FALLBACK_TIMEOUT (180 s) is passed as the wait budget anyway, so if the short-circuit assumption ever breaks (e.g. a regression in resume_asset_lock's state machine) users could wait up to 360 s total before observing the failure. Consider passing Duration::ZERO/a small bound here to make the "no wait expected" invariant load-bearing, or split out a path-only helper.

🤖 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/src/wallet/identity/network/registration.rs`
around lines 419 - 440, The code calls
asset_locks.upgrade_to_chain_lock_proof(&out_point, CL_FALLBACK_TIMEOUT) and
then immediately calls asset_locks.resume_asset_lock(&out_point,
CL_FALLBACK_TIMEOUT) just to re-derive the credit-output path, which can double
user-visible wait time if the short-circuit assumption breaks; change
resume_asset_lock usage so it does not wait: either call a new "path-only"
helper on asset_locks that derives the path without any SPV wait, or pass a
zero/very-small timeout (e.g. Duration::ZERO) instead of CL_FALLBACK_TIMEOUT
when calling resume_asset_lock to make the "no wait expected" invariant
load-bearing, leaving upgrade_to_chain_lock_proof and ResolvedFunding
construction unchanged.

---

224-275: ⚡ Quick win

L1 doc claims a defensive check that isn't actually here.

The doc says "The first key (id=0) MUST be a MASTER + AUTHENTICATION key — this is enforced here defensively so a malformed map fails fast", but the body only constructs IdentityV0 from keys_map and submits. The actual MASTER/AUTHENTICATION enforcement lives in the L2 orchestrator (register_identity_with_funding, lines 374-393), so callers that invoke the L1 primitive directly (the doc explicitly advertises this for evo-tool/integration tests) get no validation. Either move the check down into the L1 primitive (recommended, since the comment at lines 442-448 says L2 deliberately bypasses L1 only to avoid the by-value keys_map) or reword the doc to make clear the check is L2-only.

🤖 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/src/wallet/identity/network/registration.rs`
around lines 224 - 275, The docstring incorrectly claims a defensive check for
the first key but none exists; update register_identity_with_signer to enforce
that keys_map contains an entry with id 0 and that that IdentityPublicKey has
the required MASTER key type and AUTHENTICATION purpose (e.g., check
keys_map.get(&0) and validate its key_type/purpose fields), returning an
appropriate dash_sdk::Error if missing/invalid; alternatively, if you prefer not
to add logic here, change the docstring to state that validation is performed
only by register_identity_with_funding so callers of
register_identity_with_signer must ensure keys_map is well-formed.

---

288-310: ⚡ Quick win

Clarify the Copy derive on PutSettings and consider extracting user_fee_increase for defensive clarity.

settings.and_then(|s| s.user_fee_increase) moves settings: Option<PutSettings>, which works here only because PutSettings explicitly derives Copy. The second argument then passes settings again. While this compiles correctly, using settings.as_ref().and_then(|s| s.user_fee_increase) avoids relying on implicit copies and is more defensive to future changes:

🛠️ Suggested fix

-        identity
-            .top_up_identity_with_signer(
-                &self.sdk,
-                asset_lock_proof,
-                asset_lock_proof_path,
-                asset_lock_signer,
-                settings.and_then(|s| s.user_fee_increase),
-                settings,
-            )
-            .await
+        let user_fee_increase = settings.as_ref().and_then(|s| s.user_fee_increase);
+        identity
+            .top_up_identity_with_signer(
+                &self.sdk,
+                asset_lock_proof,
+                asset_lock_proof_path,
+                asset_lock_signer,
+                user_fee_increase,
+                settings,
+            )
+            .await

🤖 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/src/wallet/identity/network/registration.rs`
around lines 288 - 310, The current call in top_up_identity_with_signer relies
on PutSettings being Copy by using settings.and_then(|s| s.user_fee_increase),
which is fragile; change the second argument to use a reference so we don't move
settings: pass settings.as_ref().and_then(|s| s.user_fee_increase) when calling
identity.top_up_identity_with_signer, keeping the original settings variable for
the later settings parameter and avoiding depending on PutSettings::Copy.

packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (1)

337-369: 💤 Low value

Consistency: add unsafe impl Send + Sync for UnresolvedAssetLockTxRecordFFI and tighten tx_bytes mutability.

Two small consistency items on the new struct:

1. Every other restore FFI type in this file (AccountSpecFFI, IdentityKeyRestoreFFI, IdentityRestoreEntryFFI, UtxoRestoreEntryFFI, WalletRestoreEntryFFI) carries an explicit unsafe impl Send + Sync. UnresolvedAssetLockTxRecordFFI is the only new addition without it — because it holds *mut u8, it falls back to !Send + !Sync. It compiles today only because the slice is consumed inline in the load callback and never crosses a thread boundary; a future caller that wants to ship the slice through a Send future or thread pool will hit a compile error inconsistent with sibling types.

2. tx_bytes: *mut u8 is read-only on the Rust side (decoded via consensus::deserialize). Sibling read-only buffers use *const u8 (e.g. UtxoRestoreEntryFFI.script_pubkey, IdentityKeyRestoreFFI.data). *const u8 better documents the intended access pattern.

♻️ Proposed consistency fixes

 pub struct UnresolvedAssetLockTxRecordFFI {
     pub account_index: u32,
-    pub tx_bytes: *mut u8,
+    pub tx_bytes: *const u8,
     pub tx_bytes_len: usize,
@@
 unsafe impl Send for UtxoRestoreEntryFFI {}
 unsafe impl Sync for UtxoRestoreEntryFFI {}
+unsafe impl Send for UnresolvedAssetLockTxRecordFFI {}
+unsafe impl Sync for UnresolvedAssetLockTxRecordFFI {}

Swift call sites that allocate tx_bytes via UnsafeMutablePointer<UInt8> will need to convert/UnsafePointer(...) at construction; the matching change in persistence.rs::restore_unresolved_asset_lock_tx_records is implicit since slice_from_raw already takes *const u8 (the current *mut u8 is coerced).

Also applies to: 440-452

🤖 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/wallet_restore_types.rs` around lines 337
- 369, UnresolvedAssetLockTxRecordFFI currently uses tx_bytes: *mut u8 and lacks
thread-safety markers; change tx_bytes to *const u8 (to reflect read-only access
during consensus::deserialize) and add an explicit unsafe impl Send + Sync for
UnresolvedAssetLockTxRecordFFI so it matches sibling FFI types (e.g.,
AccountSpecFFI, UtxoRestoreEntryFFI) and allows the struct to be sent across
threads; update any construction sites to cast MutablePointer to const as
needed.

🤖 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-dpp/src/state_transition/mod.rs`:
- Around line 1293-1369: The method sign_with_signer currently allows signing
any transition with a signature field; restrict it to only the asset-lock
transition variants by adding an early guard that inspects self's enum variant
(the transition enum that implements sign_with_signer) and returns a
ProtocolError::Generic (or other appropriate ProtocolError) if self is not an
asset-lock variant; place this check at the top of sign_with_signer before
computing signable_bytes, and keep the rest of the flow (including calling
signer.sign_ecdsa, constructing RecoverableSignature, and calling set_signature)
unchanged so only asset-lock transitions can reach set_signature and update
signature_public_key_id/state.

In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- Around line 742-774: The current find_tracked_unproven_lock implementation
uses iter().find(...) which returns an arbitrary match instead of the
most-recently-tracked entry promised by the docs; update the function to collect
matching entries from info.tracked_asset_locks (filtering by funding_type,
identity_index, status and proof.is_none()) and then choose the entry with the
latest timestamp (e.g., lock.tracked_at or lock.created_at) using max_by_key (or
fold) before mapping to OutPoint, or alternatively update the doc comment to
explicitly state it returns any matching unproven lock; change the logic in
find_tracked_unproven_lock (and reference tracked_asset_locks and
AssetLockStatus::Built | AssetLockStatus::Broadcast) accordingly.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift`:
- Around line 144-146: The code in PersistentAssetLock uses raw.load(as:
UInt32.self) on voutBytes which can crash on platforms without alignment
guarantees; replace that load with an unaligned-safe byte-copy decode (same
pattern used in ManagedAssetLockManager/PlatformWalletManager) by copying the 4
voutBytes into a local UInt32 via withUnsafeBytes or Data.copyBytes into a
UInt32 buffer and then calling littleEndian to obtain vout.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift`:
- Around line 135-136: The code currently maps pathPtr to an empty string and
returns BuildResult(transaction: txData, derivationPath: path) even when the C
string is missing; update ManagedAssetLockManager (the functions constructing
BuildResult) to treat a nil pathPtr as an immediate failure rather than
defaulting to "", e.g. detect pathPtr == nil and return/throw an error (or a
failing BuildResult) with a clear message before creating
BuildResult(transaction:txData,...); apply the same change to the other
occurrences noted (the analogous blocks around the 168-172 and 216-220 diffs) so
callers don’t receive empty derivationPath values.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 2417-2421: The code currently returns ManagedIdentity(handle:
outManagedHandle) even when the FFI succeeded but the out handle is NULL_HANDLE;
update the post-FFI logic in the closure that builds the (identityId,
ManagedIdentity) tuple to validate outManagedHandle (compare against
NULL_HANDLE) and throw a descriptive error if it is NULL before constructing
ManagedIdentity; apply the same validation to the other occurrence that builds
ManagedIdentity at the later block around the second spot noted (the one around
lines 2527-2530), so neither path can return a ManagedIdentity wrapping
NULL_HANDLE.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Around line 360-366: The current logic clears retainedAssetLockManagers at the
start of the batch which can drop wrappers while their attached catch-up tasks
(asset_lock_manager_catch_up_blocking) are still running; instead, change to
track each started task (e.g., store a Task/Promise or DispatchGroup per wallet
using the retainedAssetLockManagers map) and only remove the corresponding
manager entry when that task completes or a per-task joined timeout expires.
Update the code paths around the loop that iterates wallets (the block that
currently calls retainedAssetLockManagers.removeAll(keepingCapacity: true) and
then launches catch-up work) to register the task, await or observe its
completion, and then remove that specific manager key rather than clearing the
whole collection; ensure the cleanup logic used elsewhere (the block around
lines ~405-428) follows the same per-task join-and-remove pattern.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 2855-2859: The code currently uses UInt8(clamping:) for
entry.funding_type and entry.status which can silently map out-of-range or
negative persisted integers into valid enum values; instead perform an
exact/validated conversion: check record.fundingTypeRaw and record.statusRaw are
within 0...255 (or use UInt8(exactly:) and test for nil), and if either
conversion fails, log the error (including record identifiers) and skip/return
the invalid row rather than clamping; update the assignment of
entry.funding_type and entry.status to use the validated UInt8 values
(references: entry.funding_type, entry.status, record.fundingTypeRaw,
record.statusRaw in PlatformWalletPersistenceHandler.swift).

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingRegistrationsList.swift`:
- Around line 17-19: The ForEach is currently keyed by identityIndex which is
not globally unique across wallets; change the row identity to a composite key
of (walletId, identityIndex). Add or use a unique identifier on the registration
controller (e.g., a computed property like rowId or walletAndIndex that returns
"\(walletId)-\(identityIndex)"), then update the ForEach to use id: \.rowId (or
the new property) so PendingRegistrationRow(controller: controller) is keyed by
the combined walletId and identityIndex.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegistrationProgressView.swift`:
- Around line 49-55: The instantLockTimeout constant in RegistrationProgressView
is set to 300.0 but the registration flow contract uses a 180-second InstantSend
timeout; update the static let instantLockTimeout: TimeInterval in
RegistrationProgressView to 180.0 so the UI transitions from InstantSend wait to
ChainLock fallback at the correct 180s threshold.

---

Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- Line 165: The SecItemDelete call in storePrivateKeyNonisolated and
storeKeyData currently uses the full query that contains kSecValueData (and
kSecAttrGeneric in storePrivateKeyNonisolated), which can leave stale rows and
cause errSecDuplicateItem on SecItemAdd; change both deletes to use an
attribute-only delete query (include kSecClass and the identifying attributes
such as kSecAttrAccount, kSecAttrService, kSecAttrLabel, kSecAttrAccessible,
etc., but omit kSecValueData and kSecAttrGeneric) so the existing item is
removed based on its attributes before calling SecItemAdd, matching the pattern
used in the earlier fix for the other writer.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- Around line 199-235: The view never shows a terminal UI when
submitPlatformPayment succeeds because successful platform payments set
createdIdentityId but never set activeController; update the view logic (around
the conditional rendering that checks activeController / preselectedAssetLock)
to also treat a non-nil createdIdentityId as a terminal state by rendering
terminalSection(for:) (or a small completion-only section) when
createdIdentityId != nil, or alternatively set activeController to a
completion-state controller inside submitPlatformPayment after success;
reference submitPlatformPayment, createdIdentityId, activeController,
RegistrationProgressSection, and terminalSection(for:) to locate code to change
and ensure the submit button path for platform payments either shows the same
success banner and actions or dismisses the sheet appropriately.

---

Nitpick comments:
In @.claude/skills/simulator-control/SKILL.md:
- Around line 66-81: The piped command currently redirects stderr into stdout
(the "2>&1" around the call to "$HOME/.local/bin/idb" ui describe-all) so the
inline Python (json.loads on sys.stdin) can receive non-JSON diagnostics and
fail; change the shell pipeline to separate or filter stderr before passing JSON
to the python snippet: remove or modify the "2>&1" redirection on the idb ui
describe-all call and instead capture only its stdout (or redirect stderr to a
log/pipe) so the python3 -c block that calls json.loads(...) receives clean
JSON; update references around LABEL/UDID and the subprocess.run(...) invocation
accordingly to still use the UDID env var and perform the tap.
- Around line 150-152: The tap_label "Resume" call is ambiguous when multiple
Resume buttons exist; instead locate the specific Resume button associated with
the intended registration by first waiting for the unique outpoint substring to
appear in the accessibility tree (as done in Workflow C lines 157-163) or by
filtering candidate nodes by their frame.y coordinate range, then tap that
specific node rather than calling tap_label "Resume" blindly; update the
sequence around tap_label "Identities" / tap_label "Resume" to perform the
wait-and-find (or y-range filter) and then invoke the tap on the resolved node.

In `@packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- Around line 337-369: UnresolvedAssetLockTxRecordFFI currently uses tx_bytes:
*mut u8 and lacks thread-safety markers; change tx_bytes to *const u8 (to
reflect read-only access during consensus::deserialize) and add an explicit
unsafe impl Send + Sync for UnresolvedAssetLockTxRecordFFI so it matches sibling
FFI types (e.g., AccountSpecFFI, UtxoRestoreEntryFFI) and allows the struct to
be sent across threads; update any construction sites to cast MutablePointer to
const as needed.

In `@packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- Around line 110-134: The code currently takes the first key from result.keys
(AssetLockCreditKeys::Public) and ignores any extras; change the match arm to
explicitly validate there is exactly one credit-output key: inspect the
collection (e.g., check keys.len() or pattern-match on a single-element Vec) and
return a PlatformWalletError::AssetLockTransaction if it is empty or contains >1
entries, then return the single path; keep the existing error for the Private
variant and still return Ok((result.transaction, path)) on success.

In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 39-41: The intra-doc link for MnemonicResolverCoreSigner is
incorrect: it points to crate::wallet::asset_lock::build but the type lives in
platform-wallet-ffi; update the doc comment (the lines referencing
`MnemonicResolverCoreSigner` and `crate::wallet::asset_lock::build`) to remove
or change the intra-doc path — either drop the link and refer to the type in
plain prose ("MnemonicResolverCoreSigner from platform-wallet-ffi") or point
users to the external crate; ensure you do not keep the unreachable
`crate::wallet::asset_lock::build` link.

In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- Around line 419-440: The code calls
asset_locks.upgrade_to_chain_lock_proof(&out_point, CL_FALLBACK_TIMEOUT) and
then immediately calls asset_locks.resume_asset_lock(&out_point,
CL_FALLBACK_TIMEOUT) just to re-derive the credit-output path, which can double
user-visible wait time if the short-circuit assumption breaks; change
resume_asset_lock usage so it does not wait: either call a new "path-only"
helper on asset_locks that derives the path without any SPV wait, or pass a
zero/very-small timeout (e.g. Duration::ZERO) instead of CL_FALLBACK_TIMEOUT
when calling resume_asset_lock to make the "no wait expected" invariant
load-bearing, leaving upgrade_to_chain_lock_proof and ResolvedFunding
construction unchanged.
- Around line 224-275: The docstring incorrectly claims a defensive check for
the first key but none exists; update register_identity_with_signer to enforce
that keys_map contains an entry with id 0 and that that IdentityPublicKey has
the required MASTER key type and AUTHENTICATION purpose (e.g., check
keys_map.get(&0) and validate its key_type/purpose fields), returning an
appropriate dash_sdk::Error if missing/invalid; alternatively, if you prefer not
to add logic here, change the docstring to state that validation is performed
only by register_identity_with_funding so callers of
register_identity_with_signer must ensure keys_map is well-formed.
- Around line 288-310: The current call in top_up_identity_with_signer relies on
PutSettings being Copy by using settings.and_then(|s| s.user_fee_increase),
which is fragile; change the second argument to use a reference so we don't move
settings: pass settings.as_ref().and_then(|s| s.user_fee_increase) when calling
identity.top_up_identity_with_signer, keeping the original settings variable for
the later settings parameter and avoiding depending on PutSettings::Copy.

In `@packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md`:
- Around line 1056-1071: Update the "Open questions" section to reflect resolved
decisions: replace the question about default funding amount with a statement
that defaultCoreFundingDuffs was set to 250,000 duffs (commit 3d16a31a), and
replace the asset-lock minimum question with a note that minIdentityFundingDuffs
was set to 221,500 duffs; remove or mark as resolved the items referencing those
values (lines mentioning 100,000 duffs and asset-lock minimum) and keep the
remaining open questions (key count and AssetLockStatus extension) unchanged.
- Around line 147-149: Update the iteration reference from "iter 2" to "iter 3"
in the sentence that currently reads "Resolved in iter 2" so it matches the
later note "### Iter 3 — SwiftData mirror + persister callback (was iter 2)";
ensure the wording still indicates that the PersistentAssetLock SwiftData model
mirrors TrackedAssetLock via the FFI persister callback and that this work was
delivered in iteration 3.
- Line 992: Update the wording for Scenario 2 (currently labeled "🔴 bug repro")
to clearly state intent: if this is preventing a previously fixed regression,
rename to "Regression guard — Start a fresh registration..." ; if it documents a
discovered/fixed issue, use "🔴 bug fixed — ..." or "🔴 known issue — ..." and
add a short note whether the bug was fixed; ensure the description still states
that the in-flight slot must not appear in Resumable Registrations and reference
the related code check (Phase.isActive + identity-slot union in
ResumableRegistrationsList, commit 02a15497c6) so readers know the behavioral
contract being validated.
- Line 417: Update the cross-reference text "See § Iter 5 / SPV event-routing
follow-up" so it points to the actual section heading "SPV event-routing
follow-up — RESOLVED (2026-05-13)" used later in the document; locate the string
in the Step 13 paragraph and replace it with a clear reference matching the
exact heading text ("See § SPV event-routing follow-up — RESOLVED
(2026-05-13)"), or alternatively change the reference to "See § SPV
event-routing follow-up" if you prefer a shorter match that exactly equals the
section title "SPV event-routing follow-up — RESOLVED (2026-05-13)" to ensure
the cross-reference resolves correctly.

🪄 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: 7da94f0d-3eec-4969-bbcf-bb7966120fea

📥 Commits

Reviewing files that changed from the base of the PR and between fa1e492 and 67f5962.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (76)

• .claude/skills/simulator-control/SKILL.md
• Cargo.toml
• packages/rs-dpp/src/errors/protocol_error.rs
• packages/rs-dpp/src/state_transition/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/methods/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/methods/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/methods/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/methods/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/v0_methods.rs
• packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/mod.rs
• packages/rs-drive-abci/tests/strategy_tests/strategy.rs
• packages/rs-platform-wallet-ffi/Cargo.toml
• packages/rs-platform-wallet-ffi/src/asset_lock/build.rs
• packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs
• packages/rs-platform-wallet-ffi/src/asset_lock_persistence.rs
• packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
• packages/rs-platform-wallet-ffi/src/core_wallet_types.rs
• packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs
• packages/rs-platform-wallet-ffi/src/lib.rs
• packages/rs-platform-wallet-ffi/src/mnemonic_resolver_core_signer.rs
• packages/rs-platform-wallet-ffi/src/persistence.rs
• packages/rs-platform-wallet-ffi/src/spv.rs
• packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs
• packages/rs-platform-wallet/Cargo.toml
• packages/rs-platform-wallet/src/changeset/changeset.rs
• packages/rs-platform-wallet/src/changeset/core_bridge.rs
• packages/rs-platform-wallet/src/error.rs
• packages/rs-platform-wallet/src/lib.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
• packages/rs-platform-wallet/src/wallet/core/balance_handler.rs
• packages/rs-platform-wallet/src/wallet/core/broadcast.rs
• packages/rs-platform-wallet/src/wallet/core/wallet.rs
• packages/rs-platform-wallet/src/wallet/identity/mod.rs
• packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
• packages/rs-platform-wallet/src/wallet/identity/network/top_up.rs
• packages/rs-platform-wallet/src/wallet/identity/types/funding.rs
• packages/rs-platform-wallet/src/wallet/identity/types/mod.rs
• packages/rs-sdk-ffi/src/identity/put.rs
• packages/rs-sdk-ffi/src/identity/topup.rs
• packages/rs-sdk/src/platform/transition/broadcast_identity.rs
• packages/rs-sdk/src/platform/transition/put_identity.rs
• packages/rs-sdk/src/platform/transition/top_up_identity.rs
• packages/strategy-tests/src/transitions.rs
• packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift
• packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletFFI.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
• packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift
• packages/swift-sdk/SwiftExampleApp/CREATE_IDENTITY_FROM_CORE_FUNDS_PLAN.md
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/IdentityRegistrationController.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/PlatformWalletManager+RegistrationCoordinator.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/RegistrationCoordinator.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/OptionsView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingRegistrationsList.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegistrationProgressView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageExplorerView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/KeyManagerTests.swift
• packages/wasm-sdk/src/state_transitions/identity.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restrict sign_with_signer to asset-lock transitions.

This public method currently succeeds for any variant that has a signature field. On identity-signed transitions like DataContractCreate, Batch, or IdentityCreditTransfer, that skips the normal key-purpose/security checks and never updates signature_public_key_id, so callers can manufacture an invalid signed transition and still get Ok(()). Please hard-fail here unless self is one of the asset-lock variants this API is meant for.

Suggested guard

 pub async fn sign_with_signer<S: ::key_wallet::signer::Signer>(
     &mut self,
     path: &::key_wallet::bip32::DerivationPath,
     signer: &S,
 ) -> Result<(), ProtocolError> {
+    match self {
+        StateTransition::IdentityCreate(_)
+        | StateTransition::IdentityTopUp(_)
+        | StateTransition::AddressFundingFromAssetLock(_)
+        | StateTransition::ShieldFromAssetLock(_) => {}
+        other => {
+            return Err(ProtocolError::CorruptedCodeExecution(format!(
+                "{} cannot be signed via the asset-lock signer path",
+                other.name()
+            )));
+        }
+    }
+
     let data = self.signable_bytes()?;

🤖 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-dpp/src/state_transition/mod.rs` around lines 1293 - 1369, The
method sign_with_signer currently allows signing any transition with a signature
field; restrict it to only the asset-lock transition variants by adding an early
guard that inspects self's enum variant (the transition enum that implements
sign_with_signer) and returns a ProtocolError::Generic (or other appropriate
ProtocolError) if self is not an asset-lock variant; place this check at the top
of sign_with_signer before computing signable_bytes, and keep the rest of the
flow (including calling signer.sign_ecdsa, constructing RecoverableSignature,
and calling set_signature) unchanged so only asset-lock transitions can reach
set_signature and update signature_public_key_id/state.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

cat -n packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift | sed -n '130,160p'

Repository: dashpay/platform

Length of output: 1114

---

🏁 Script executed:

cat -n packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift | head -50

Repository: dashpay/platform

Length of output: 2836

---

🏁 Script executed:

rg -A 5 -B 5 "voutBytes" packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift

Repository: dashpay/platform

Length of output: 623

---

🏁 Script executed:

find packages/swift-sdk -name "Package.swift" -o -name "package.swift" | head -5

Repository: dashpay/platform

Length of output: 137

---

🏁 Script executed:

cat packages/swift-sdk/Package.swift | grep -A 20 "swiftLanguageVersions\|platforms"

Repository: dashpay/platform

Length of output: 631

---

🏁 Script executed:

rg "loadUnaligned" packages/swift-sdk --type swift -c

Repository: dashpay/platform

Length of output: 42

---

🏁 Script executed:

rg -B 3 -A 3 "withUnsafeBytes.*load\(as:" packages/swift-sdk --type swift | head -40

Repository: dashpay/platform

Length of output: 42

---

🏁 Script executed:

rg -B 2 -A 3 "prefix\(|suffix\(" packages/swift-sdk/Sources/SwiftDashSDK --type swift -A 2 | grep -A 2 "withUnsafeBytes\|\.load\|UInt" | head -50

Repository: dashpay/platform

Length of output: 2495

---

🏁 Script executed:

rg "\.copyMemory\|\.copyBytes" packages/swift-sdk --type swift -B 2 -A 2

Repository: dashpay/platform

Length of output: 42

---

🏁 Script executed:

python3 << 'EOF'
# Check Swift's Data and UnsafeRawBufferPointer alignment behavior
# Swift Data.suffix() may not guarantee alignment for load(as:)

print("""
Swift Memory Alignment Analysis:
================================

1. Data.suffix(4) returns a Data slice - a VIEW into the original Data.
   When you call withUnsafeBytes on a Data suffix, the closure receives
   an UnsafeRawBufferPointer whose baseAddress is offset into the original
   Data storage.

2. UnsafeRawBufferPointer.load(as: T) assumes the baseAddress is properly
   aligned for T. For UInt32, that means the address must be 4-byte aligned.

3. If rawBytes is at offset 32 (odd multiple of 4), then suffix(4) gives
   a pointer at offset 32. A 4-byte load from offset 32 is aligned.
   
   BUT if the original Data object itself is not aligned to 4 bytes in
   storage (common for Data created from arbitrary byte sources), the
   pointer at ANY offset may be unaligned.

4. Swift Data does NOT guarantee alignment in its internal storage.
   A Data object's baseAddress may be at any byte boundary.

5. Therefore, using raw.load(as: UInt32.self) on a Data-derived
   UnsafeRawBufferPointer is unsafe and can trigger a trap on ARM64
   (which requires aligned loads).

6. iOS 18+ and macOS 15+ both support Swift 6 and have loadUnaligned
   available (since Swift 5.3), but the safest portable approach is
   byte copying to a local variable then loading that.

Conclusion: The review comment is CORRECT. The code is unsafe.
""")
EOF

Repository: dashpay/platform

Length of output: 1401

---

🏁 Script executed:

rg "littleEndian\|bigEndian" packages/swift-sdk/Sources/SwiftDashSDK --type swift -B 2 -A 2 | head -80

Repository: dashpay/platform

Length of output: 42

---

Use an unaligned-safe decode for vout to avoid runtime traps.

Line 145 uses raw.load(as: UInt32.self) on Data bytes, which lacks alignment guarantees and can crash on ARM64. The codebase uses safe byte-copy decoding consistently elsewhere (e.g., ManagedAssetLockManager, PlatformWalletManager); apply the same pattern here.

Suggested fix

-        let vout = voutBytes.withUnsafeBytes { raw in
-            raw.load(as: UInt32.self).littleEndian
-        }
+        let vout = voutBytes.withUnsafeBytes { raw -> UInt32 in
+            var value: UInt32 = 0
+            withUnsafeMutableBytes(of: &value) { dst in
+                dst.copyBytes(from: raw.prefix(MemoryLayout<UInt32>.size))
+            }
+            return UInt32(littleEndian: value)
+        }

📝 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.

Suggested change

	let vout = voutBytes.withUnsafeBytes { raw in
	raw.load(as: UInt32.self).littleEndian
	}
	let vout = voutBytes.withUnsafeBytes { raw -> UInt32 in
	var value: UInt32 = 0
	withUnsafeMutableBytes(of: &value) { dst in
	dst.copyBytes(from: raw.prefix(MemoryLayout<UInt32>.size))
	}
	return UInt32(littleEndian: value)
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift`
around lines 144 - 146, The code in PersistentAssetLock uses raw.load(as:
UInt32.self) on voutBytes which can crash on platforms without alignment
guarantees; replace that load with an unaligned-safe byte-copy decode (same
pattern used in ManagedAssetLockManager/PlatformWalletManager) by copying the 4
voutBytes into a local UInt32 via withUnsafeBytes or Data.copyBytes into a
UInt32 buffer and then calling littleEndian to obtain vout.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast when derivation path is missing instead of defaulting to "".

These methods currently treat a missing pathPtr as success and return an empty derivation path, which can cause later signing failures with less actionable errors.

Suggested fix

-        let path = pathPtr.map { String(cString: $0) } ?? ""
+        guard let pathPtr, pathPtr.pointee != 0 else {
+            throw PlatformWalletError.unknown("FFI returned success but derivation path was empty")
+        }
+        let path = String(cString: pathPtr)
         return BuildResult(transaction: txData, derivationPath: path)

-        let path = pathPtr.map { String(cString: $0) } ?? ""
+        guard let pathPtr, pathPtr.pointee != 0 else {
+            throw PlatformWalletError.unknown("FFI returned success but derivation path was empty")
+        }
+        let path = String(cString: pathPtr)
         return FundedProofResult(

-        let path = pathPtr.map { String(cString: $0) } ?? ""
+        guard let pathPtr, pathPtr.pointee != 0 else {
+            throw PlatformWalletError.unknown("FFI returned success but derivation path was empty")
+        }
+        let path = String(cString: pathPtr)
         return ResumeResult(

Also applies to: 168-172, 216-220

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/AssetLock/ManagedAssetLockManager.swift`
around lines 135 - 136, The code currently maps pathPtr to an empty string and
returns BuildResult(transaction: txData, derivationPath: path) even when the C
string is missing; update ManagedAssetLockManager (the functions constructing
BuildResult) to treat a nil pathPtr as an immediate failure rather than
defaulting to "", e.g. detect pathPtr == nil and return/throw an error (or a
failing BuildResult) with a clear message before creating
BuildResult(transaction:txData,...); apply the same change to the other
occurrences noted (the analogous blocks around the 168-172 and 216-220 diffs) so
callers don’t receive empty derivationPath values.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate outManagedHandle before constructing ManagedIdentity.

Both paths can currently return a ManagedIdentity wrapping NULL_HANDLE if FFI succeeds but omits the out handle, which pushes failure to a later, harder-to-debug point.

Suggested fix

             try result.check()
+            guard outManagedHandle != NULL_HANDLE else {
+                throw PlatformWalletError.walletOperation("identity handle not returned")
+            }
             let identityId = Swift.withUnsafeBytes(of: idTuple) { Data($0) }
             return (identityId, ManagedIdentity(handle: outManagedHandle))

Also applies to: 2527-2530

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`
around lines 2417 - 2421, The code currently returns ManagedIdentity(handle:
outManagedHandle) even when the FFI succeeded but the out handle is NULL_HANDLE;
update the post-FFI logic in the closure that builds the (identityId,
ManagedIdentity) tuple to validate outManagedHandle (compare against
NULL_HANDLE) and throw a descriptive error if it is NULL before constructing
ManagedIdentity; apply the same validation to the other occurrence that builds
ManagedIdentity at the later block around the second spot noted (the one around
lines 2527-2530), so neither path can return a ManagedIdentity wrapping
NULL_HANDLE.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Don’t release retained managers by batch replacement while catch-up tasks may still run.

retainedAssetLockManagers.removeAll(...) can drop wrappers from a prior invocation even if their detached tasks are still in-flight, which can invalidate handle during asset_lock_manager_catch_up_blocking.

Track per-task completion and release each retained manager only after its corresponding task finishes (or after an explicit joined timeout), rather than clearing the whole previous batch on method entry.

Also applies to: 405-428

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`
around lines 360 - 366, The current logic clears retainedAssetLockManagers at
the start of the batch which can drop wrappers while their attached catch-up
tasks (asset_lock_manager_catch_up_blocking) are still running; instead, change
to track each started task (e.g., store a Task/Promise or DispatchGroup per
wallet using the retainedAssetLockManagers map) and only remove the
corresponding manager entry when that task completes or a per-task joined
timeout expires. Update the code paths around the loop that iterates wallets
(the block that currently calls
retainedAssetLockManagers.removeAll(keepingCapacity: true) and then launches
catch-up work) to register the task, await or observe its completion, and then
remove that specific manager key rather than clearing the whole collection;
ensure the cleanup logic used elsewhere (the block around lines ~405-428)
follows the same per-task join-and-remove pattern.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t clamp funding_type / status during restore marshalling.

UInt8(clamping:) can silently change corrupt/out-of-range persisted values into valid-looking enums (e.g., negative → 0), which can restore the wrong asset-lock state. Prefer exact conversion and skip/log invalid rows.

Suggested fix

-            entry.funding_type = UInt8(clamping: record.fundingTypeRaw)
+            guard let fundingType = UInt8(exactly: record.fundingTypeRaw) else {
+                NSLog("[persistor-load:swift] dropping asset-lock row with invalid fundingTypeRaw: %d", record.fundingTypeRaw)
+                continue
+            }
+            entry.funding_type = fundingType
             entry.identity_index = UInt32(bitPattern: record.identityIndexRaw)
             entry.amount_duffs = UInt64(bitPattern: record.amountDuffs)
-            entry.status = UInt8(clamping: record.statusRaw)
+            guard let status = UInt8(exactly: record.statusRaw) else {
+                NSLog("[persistor-load:swift] dropping asset-lock row with invalid statusRaw: %d", record.statusRaw)
+                continue
+            }
+            entry.status = status

📝 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.

Suggested change

	entry.funding_type = UInt8(clamping: record.fundingTypeRaw)
	entry.identity_index = UInt32(bitPattern: record.identityIndexRaw)
	entry.amount_duffs = UInt64(bitPattern: record.amountDuffs)
	entry.status = UInt8(clamping: record.statusRaw)
	entry.proof_bytes = proofPtr
	guard let fundingType = UInt8(exactly: record.fundingTypeRaw) else {
	NSLog("[persistor-load:swift] dropping asset-lock row with invalid fundingTypeRaw: %d", record.fundingTypeRaw)
	continue
	}
	entry.funding_type = fundingType
	entry.identity_index = UInt32(bitPattern: record.identityIndexRaw)
	entry.amount_duffs = UInt64(bitPattern: record.amountDuffs)
	guard let status = UInt8(exactly: record.statusRaw) else {
	NSLog("[persistor-load:swift] dropping asset-lock row with invalid statusRaw: %d", record.statusRaw)
	continue
	}
	entry.status = status
	entry.proof_bytes = proofPtr

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`
around lines 2855 - 2859, The code currently uses UInt8(clamping:) for
entry.funding_type and entry.status which can silently map out-of-range or
negative persisted integers into valid enum values; instead perform an
exact/validated conversion: check record.fundingTypeRaw and record.statusRaw are
within 0...255 (or use UInt8(exactly:) and test for nil), and if either
conversion fails, log the error (including record identifiers) and skip/return
the invalid row rather than clamping; update the assignment of
entry.funding_type and entry.status to use the validated UInt8 values
(references: entry.funding_type, entry.status, record.fundingTypeRaw,
record.statusRaw in PlatformWalletPersistenceHandler.swift).

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use a globally unique row id in ForEach.

Rows are keyed by identityIndex only, but registrations are keyed by (walletId, identityIndex). Two wallets can share the same index, which can collapse/replace rows in the list.

Suggested fix

-                ForEach(active, id: \.identityIndex) { controller in
+                ForEach(
+                    active,
+                    id: \.walletId
+                ) { controller in
                     PendingRegistrationRow(controller: controller)
                 }

+// Better: explicit composite id to avoid relying on walletId-only uniqueness.
+ForEach(active, id: \.self) { controller in ... } // if controller is Hashable

+// Or add a computed stable key:
+// "\(controller.walletId.toHexString())-\(controller.identityIndex)"

📝 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.

Suggested change

	ForEach(active, id: \.identityIndex) { controller in
	PendingRegistrationRow(controller: controller)
	}
	ForEach(
	active,
	id: \.walletId
	) { controller in
	PendingRegistrationRow(controller: controller)
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingRegistrationsList.swift`
around lines 17 - 19, The ForEach is currently keyed by identityIndex which is
not globally unique across wallets; change the row identity to a composite key
of (walletId, identityIndex). Add or use a unique identifier on the registration
controller (e.g., a computed property like rowId or walletAndIndex that returns
"\(walletId)-\(identityIndex)"), then update the ForEach to use id: \.rowId (or
the new property) so PendingRegistrationRow(controller: controller) is keyed by
the combined walletId and identityIndex.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align InstantSend timeout heuristic with the registration flow contract.

Line 55 sets instantLockTimeout to 300.0, but this registration flow is defined with a 180-second InstantSend timeout before ChainLock fallback. Using 300s will keep the UI in the IS-wait step longer than actual behavior.

Suggested fix

-    private static let instantLockTimeout: TimeInterval = 300.0
+    private static let instantLockTimeout: TimeInterval = 180.0

📝 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.

Suggested change

	/// Cutoff (seconds since `Broadcast`) where the Rust side falls
	/// back from InstantSend to ChainLock. Mirrors
	/// `AssetLockManager`'s 300 s IS wait. If `statusRaw == 1` is
	/// still the state after this window, the wallet is in the CL
	/// fallback window (180 s); we mark step 4 done and step 5
	/// active to communicate the shift.
	private static let instantLockTimeout: TimeInterval = 300.0
	/// Cutoff (seconds since `Broadcast`) where the Rust side falls
	/// back from InstantSend to ChainLock. Mirrors
	/// `AssetLockManager`'s 300 s IS wait. If `statusRaw == 1` is
	/// still the state after this window, the wallet is in the CL
	/// fallback window (180 s); we mark step 4 done and step 5
	/// active to communicate the shift.
	private static let instantLockTimeout: TimeInterval = 180.0

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegistrationProgressView.swift`
around lines 49 - 55, The instantLockTimeout constant in
RegistrationProgressView is set to 300.0 but the registration flow contract uses
a 180-second InstantSend timeout; update the static let instantLockTimeout:
TimeInterval in RegistrationProgressView to 180.0 so the UI transitions from
InstantSend wait to ChainLock fallback at the correct 180s threshold.

[thepastaclaw]

Code Review

The PR fixes the main stuck-asset-lock flow, but three issues in the new recovery/signer paths still need attention. Two are runtime bugs in the asset-lock lifecycle, and one is a private-key memory-handling bug in the new FFI signer. I also dropped one interop report that does not reproduce on the current code path because consumed locks are removed from the in-memory list before Swift can decode them.

Reviewed commit: 97417be

🔴 3 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 358-397: Repeated catch-up passes can invalidate live asset-lock handles and block the main actor
  `catchUpStuckAssetLocks` clears `retainedAssetLockManagers` at the start of every invocation, but the previous invocation has already launched detached tasks that keep using the raw `handle` through `asset_lock_manager_catch_up_blocking`. Releasing the wrapper runs `asset_lock_manager_destroy` in `ManagedAssetLockManager.deinit`, so a foreground or reconnect-triggered second pass can destroy handles that the first batch is still using. On the Rust side, `asset_lock_manager_catch_up_blocking` holds a read lock on the handle table for the full `runtime().block_on(manager.resume_asset_lock(...))` call, while destroy takes the write lock to remove the handle. That means the second pass can both turn the first batch into `errorInvalidHandle` and synchronously block the main actor until the earlier proof wait finishes or times out.

In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- [BLOCKING] lines 60-79: `consume_asset_lock` computes a consumed-state changeset but never persists it
  `consume_asset_lock()` removes the entry from `tracked_asset_locks`, changes its status to `Consumed`, and returns an `AssetLockChangeSet`, but it never calls `queue_asset_lock_changeset()`. Both current callers in `identity/network/registration.rs` only check for `Err` and discard the successful return value, so the consumed state never reaches the Swift persister. That leaves the on-disk `PersistentAssetLock` row at its pre-consumption status, which contradicts this method's own contract and causes restart/history paths to reload the lock as still actionable instead of terminally consumed.

In `packages/rs-platform-wallet-ffi/src/mnemonic_resolver_core_signer.rs`:
- [BLOCKING] lines 301-334: The mnemonic-backed signer leaves a second copy of the private scalar in `SecretKey` memory
  The zeroization comment here is wrong. `derive_priv()` returns `Zeroizing<[u8; 32]>`, but `secp256k1::SecretKey::from_slice(secret_bytes.as_ref())` copies those bytes into a separate `SecretKey` value. In the vendored `secp256k1 0.30.0` crate, `SecretKey` is a `Copy` wrapper over its own `[u8; 32]` buffer and exposes `non_secure_erase()` for manual wiping. As written, dropping `secret_bytes` only clears the original buffer; the copied scalar inside `secret` remains in process memory until reuse. That defeats the explicit goal of keeping the derived private key out of long-lived Rust memory in both `sign_ecdsa()` and `public_key()`.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2816-2821: The asset-lock restore format hardcodes BIP44 account 0 even though the public API accepts other account indices
  Both restore builders write `entry.account_index = 0` instead of persisting the real funding account. The comments acknowledge this as a temporary limitation, but the SDK already exposes public asset-lock APIs with an `accountIndex` parameter, so this is not just dead code. On reload, Rust restores unresolved asset-lock transactions into `standard_bip44_accounts.get_mut(&rec.account_index)` and drops records whose account is missing, so any asset lock funded from a nonzero BIP44 account will fail to recover after restart. The same hardcoded value is repeated in `buildUnresolvedAssetLockTxRecordBuffer` at lines 2912-2921, so both the tracked-lock snapshot and the unresolved-tx replay path are affected.

[thepastaclaw]

🔴 Blocking: Repeated catch-up passes can invalidate live asset-lock handles and block the main actor

catchUpStuckAssetLocks clears retainedAssetLockManagers at the start of every invocation, but the previous invocation has already launched detached tasks that keep using the raw handle through asset_lock_manager_catch_up_blocking. Releasing the wrapper runs asset_lock_manager_destroy in ManagedAssetLockManager.deinit, so a foreground or reconnect-triggered second pass can destroy handles that the first batch is still using. On the Rust side, asset_lock_manager_catch_up_blocking holds a read lock on the handle table for the full runtime().block_on(manager.resume_asset_lock(...)) call, while destroy takes the write lock to remove the handle. That means the second pass can both turn the first batch into errorInvalidHandle and synchronously block the main actor until the earlier proof wait finishes or times out.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 358-397: Repeated catch-up passes can invalidate live asset-lock handles and block the main actor
  `catchUpStuckAssetLocks` clears `retainedAssetLockManagers` at the start of every invocation, but the previous invocation has already launched detached tasks that keep using the raw `handle` through `asset_lock_manager_catch_up_blocking`. Releasing the wrapper runs `asset_lock_manager_destroy` in `ManagedAssetLockManager.deinit`, so a foreground or reconnect-triggered second pass can destroy handles that the first batch is still using. On the Rust side, `asset_lock_manager_catch_up_blocking` holds a read lock on the handle table for the full `runtime().block_on(manager.resume_asset_lock(...))` call, while destroy takes the write lock to remove the handle. That means the second pass can both turn the first batch into `errorInvalidHandle` and synchronously block the main actor until the earlier proof wait finishes or times out.

[thepastaclaw]

🔴 Blocking: consume_asset_lock computes a consumed-state changeset but never persists it

consume_asset_lock() removes the entry from tracked_asset_locks, changes its status to Consumed, and returns an AssetLockChangeSet, but it never calls queue_asset_lock_changeset(). Both current callers in identity/network/registration.rs only check for Err and discard the successful return value, so the consumed state never reaches the Swift persister. That leaves the on-disk PersistentAssetLock row at its pre-consumption status, which contradicts this method's own contract and causes restart/history paths to reload the lock as still actionable instead of terminally consumed.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- [BLOCKING] lines 60-79: `consume_asset_lock` computes a consumed-state changeset but never persists it
  `consume_asset_lock()` removes the entry from `tracked_asset_locks`, changes its status to `Consumed`, and returns an `AssetLockChangeSet`, but it never calls `queue_asset_lock_changeset()`. Both current callers in `identity/network/registration.rs` only check for `Err` and discard the successful return value, so the consumed state never reaches the Swift persister. That leaves the on-disk `PersistentAssetLock` row at its pre-consumption status, which contradicts this method's own contract and causes restart/history paths to reload the lock as still actionable instead of terminally consumed.

[thepastaclaw]

🔴 Blocking: The mnemonic-backed signer leaves a second copy of the private scalar in SecretKey memory

The zeroization comment here is wrong. derive_priv() returns Zeroizing<[u8; 32]>, but secp256k1::SecretKey::from_slice(secret_bytes.as_ref()) copies those bytes into a separate SecretKey value. In the vendored secp256k1 0.30.0 crate, SecretKey is a Copy wrapper over its own [u8; 32] buffer and exposes non_secure_erase() for manual wiping. As written, dropping secret_bytes only clears the original buffer; the copied scalar inside secret remains in process memory until reuse. That defeats the explicit goal of keeping the derived private key out of long-lived Rust memory in both sign_ecdsa() and public_key().

💡 Suggested change

Suggested change

	async fn sign_ecdsa(
	&self,
	path: &DerivationPath,
	sighash: [u8; 32],
	) -> Result<(secp256k1::ecdsa::Signature, secp256k1::PublicKey), Self::Error> {
	let secret_bytes = self.derive_priv(path)?;
	let secp = Secp256k1::new();
	// `SecretKey::from_slice` validates the 32-byte scalar is a
	// legitimate field element. The slice borrow is dropped at
	// the end of this block; `secret_bytes` is then zeroed when
	// it falls out of scope.
	let secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
	.map_err(|e| MnemonicResolverSignerError::InvalidScalar(e.to_string()))?;
	let msg = secp256k1::Message::from_digest(sighash);
	let signature = secp.sign_ecdsa(&msg, &secret);
	let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
	// `secp256k1::SecretKey` is `Copy` (a thin wrapper over a
	// 32-byte buffer) and doesn't itself zero on drop — but the
	// backing buffer here came from `secret_bytes`
	// (a `Zeroizing<[u8; 32]>`), which IS wiped when it falls
	// out of scope below. The `secret` binding is forgotten by
	// letting it go out of scope; no explicit `drop` needed.
	let _ = secret;
	Ok((signature, pubkey))
	}
	async fn public_key(&self, path: &DerivationPath) -> Result<secp256k1::PublicKey, Self::Error> {
	let secret_bytes = self.derive_priv(path)?;
	let secp = Secp256k1::new();
	let secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
	.map_err(|e| MnemonicResolverSignerError::InvalidScalar(e.to_string()))?;
	let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
	let _ = secret;
	Ok(pubkey)
	async fn sign_ecdsa(
	&self,
	path: &DerivationPath,
	sighash: [u8; 32],
	) -> Result<(secp256k1::ecdsa::Signature, secp256k1::PublicKey), Self::Error> {
	let secret_bytes = self.derive_priv(path)?;
	let secp = Secp256k1::new();
	let mut secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
	.map_err(|e| MnemonicResolverSignerError::InvalidScalar(e.to_string()))?;
	let msg = secp256k1::Message::from_digest(sighash);
	let signature = secp.sign_ecdsa(&msg, &secret);
	let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
	secret.non_secure_erase();
	Ok((signature, pubkey))
	}
	async fn public_key(&self, path: &DerivationPath) -> Result<secp256k1::PublicKey, Self::Error> {
	let secret_bytes = self.derive_priv(path)?;
	let secp = Secp256k1::new();
	let mut secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
	.map_err(|e| MnemonicResolverSignerError::InvalidScalar(e.to_string()))?;
	let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
	secret.non_secure_erase();
	Ok(pubkey)
	}

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/mnemonic_resolver_core_signer.rs`:
- [BLOCKING] lines 301-334: The mnemonic-backed signer leaves a second copy of the private scalar in `SecretKey` memory
  The zeroization comment here is wrong. `derive_priv()` returns `Zeroizing<[u8; 32]>`, but `secp256k1::SecretKey::from_slice(secret_bytes.as_ref())` copies those bytes into a separate `SecretKey` value. In the vendored `secp256k1 0.30.0` crate, `SecretKey` is a `Copy` wrapper over its own `[u8; 32]` buffer and exposes `non_secure_erase()` for manual wiping. As written, dropping `secret_bytes` only clears the original buffer; the copied scalar inside `secret` remains in process memory until reuse. That defeats the explicit goal of keeping the derived private key out of long-lived Rust memory in both `sign_ecdsa()` and `public_key()`.

[thepastaclaw]

🟡 Suggestion: The asset-lock restore format hardcodes BIP44 account 0 even though the public API accepts other account indices

Both restore builders write entry.account_index = 0 instead of persisting the real funding account. The comments acknowledge this as a temporary limitation, but the SDK already exposes public asset-lock APIs with an accountIndex parameter, so this is not just dead code. On reload, Rust restores unresolved asset-lock transactions into standard_bip44_accounts.get_mut(&rec.account_index) and drops records whose account is missing, so any asset lock funded from a nonzero BIP44 account will fail to recover after restart. The same hardcoded value is repeated in buildUnresolvedAssetLockTxRecordBuffer at lines 2912-2921, so both the tracked-lock snapshot and the unresolved-tx replay path are affected.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2816-2821: The asset-lock restore format hardcodes BIP44 account 0 even though the public API accepts other account indices
  Both restore builders write `entry.account_index = 0` instead of persisting the real funding account. The comments acknowledge this as a temporary limitation, but the SDK already exposes public asset-lock APIs with an `accountIndex` parameter, so this is not just dead code. On reload, Rust restores unresolved asset-lock transactions into `standard_bip44_accounts.get_mut(&rec.account_index)` and drops records whose account is missing, so any asset lock funded from a nonzero BIP44 account will fail to recover after restart. The same hardcoded value is repeated in `buildUnresolvedAssetLockTxRecordBuffer` at lines 2912-2921, so both the tracked-lock snapshot and the unresolved-tx replay path are affected.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

packages/rs-platform-wallet/src/wallet/apply.rs (1)

293-309: ⚡ Quick win

Add a regression test for the Consumed branch.

This new branch is core to restart/recovery semantics, but current tests still only cover insert/tombstone flow. Please add a focused test that seeds a tracked lock, applies status = Consumed, and asserts the in-memory map entry is removed.

Suggested test shape

+    #[test]
+    fn apply_asset_lock_consumed_removes_from_in_memory_map() {
+        let mut wallet = build_test_wallet();
+        let mut info = empty_info(&wallet);
+        let out_point = OutPoint::default();
+
+        // Seed as Built.
+        let mut seed = AssetLockChangeSet::default();
+        seed.asset_locks.insert(
+            out_point,
+            AssetLockEntry {
+                out_point,
+                transaction: dashcore::Transaction {
+                    version: 3,
+                    lock_time: 0,
+                    input: vec![],
+                    output: vec![],
+                    special_transaction_payload: None,
+                },
+                account_index: 0,
+                funding_type: AssetLockFundingType::IdentityRegistration,
+                identity_index: 0,
+                amount_duffs: 5_000,
+                status: AssetLockStatus::Built,
+                proof: None,
+            },
+        );
+        let mut cs = PlatformWalletChangeSet::default();
+        cs.asset_locks = Some(seed);
+        info.apply_changeset(&mut wallet, cs).expect("seed");
+        assert!(info.tracked_asset_locks.contains_key(&out_point));
+
+        // Apply Consumed transition.
+        let mut consumed = AssetLockChangeSet::default();
+        consumed.asset_locks.insert(
+            out_point,
+            AssetLockEntry {
+                out_point,
+                transaction: dashcore::Transaction {
+                    version: 3,
+                    lock_time: 0,
+                    input: vec![],
+                    output: vec![],
+                    special_transaction_payload: None,
+                },
+                account_index: 0,
+                funding_type: AssetLockFundingType::IdentityRegistration,
+                identity_index: 0,
+                amount_duffs: 5_000,
+                status: AssetLockStatus::Consumed,
+                proof: None,
+            },
+        );
+        let mut cs = PlatformWalletChangeSet::default();
+        cs.asset_locks = Some(consumed);
+        info.apply_changeset(&mut wallet, cs).expect("consume");
+        assert!(!info.tracked_asset_locks.contains_key(&out_point));
+    }

🤖 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/src/wallet/apply.rs` around lines 293 - 309, Add
a regression test that seeds the in-memory tracked_asset_locks map with a
TrackedAssetLock (keyed by out_point), then constructs an entry whose status is
AssetLockStatus::Consumed and invokes the code path that processes that entry
(the function that contains the shown branch handling entry.status). After
invoking it assert that tracked_asset_locks does not contain the out_point (i.e.
the entry was removed). Use the exact symbols TrackedAssetLock,
tracked_asset_locks, AssetLockStatus::Consumed, out_point and entry to locate
and exercise the branch.

🤖 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 @.claude/skills/simulator-control/SKILL.md:
- Around line 63-82: The tap_label function exports LABEL and UDID inline before
invoking a python -c string which can fail to receive those env vars in non-bash
shells; update tap_label to pass LABEL and UDID into the Python interpreter via
a heredoc (invoke python3 with a here-doc block) or explicitly export them in
the shell environment before calling idb so the Python code can read
os.environ['LABEL'] and os.environ['UDID'] reliably; locate the tap_label
function and the inline python3 -c invocation and replace it with a heredoc form
(or an explicit export of LABEL/UDID) so environment propagation is portable
across shells.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift`:
- Line 26: PersistentAssetLockDisplay's canFundIdentity currently returns true
for Consumed (statusRaw == 4); update the predicate to exclude Consumed so spent
locks can't fund identities. Locate the canFundIdentity computed property in
PersistentAssetLockDisplay and change its logic to require statusRaw >= 2 AND
statusRaw != 4 (i.e., allow ChainLocked and other non-consumed finalized states
but explicitly disallow Consumed); reference the canFundIdentity property and
statusRaw field when making the change.

---

Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/apply.rs`:
- Around line 293-309: Add a regression test that seeds the in-memory
tracked_asset_locks map with a TrackedAssetLock (keyed by out_point), then
constructs an entry whose status is AssetLockStatus::Consumed and invokes the
code path that processes that entry (the function that contains the shown branch
handling entry.status). After invoking it assert that tracked_asset_locks does
not contain the out_point (i.e. the entry was removed). Use the exact symbols
TrackedAssetLock, tracked_asset_locks, AssetLockStatus::Consumed, out_point and
entry to locate and exercise the branch.

🪄 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: a2891db7-1533-4806-b424-3a03b004291e

📥 Commits

Reviewing files that changed from the base of the PR and between 67f5962 and 65e417d.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (36)

• .claude/skills/simulator-control/SKILL.md
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs
• packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs
• packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs
• packages/rs-platform-wallet-ffi/src/asset_lock_persistence.rs
• packages/rs-platform-wallet-ffi/src/core_wallet_types.rs
• packages/rs-platform-wallet-ffi/src/persistence.rs
• packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs
• packages/rs-platform-wallet/src/changeset/changeset.rs
• packages/rs-platform-wallet/src/changeset/core_bridge.rs
• packages/rs-platform-wallet/src/error.rs
• packages/rs-platform-wallet/src/manager/accessors.rs
• packages/rs-platform-wallet/src/wallet/apply.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/lock_notify_handler.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs
• packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
• packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
• packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs
• packages/rs-sdk-ffi/src/lib.rs
• packages/rs-sdk/src/platform/transition/broadcast_identity.rs
• packages/rs-sdk/src/platform/transition/put_identity.rs
• packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift
• packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegistrationProgressView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageModelListViews.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WalletMemoryExplorerView.swift

✅ Files skipped from review due to trivial changes (2)

• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WalletMemoryExplorerView.swift
• packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs

🚧 Files skipped from review as they are similar to previous changes (10)

• packages/rs-platform-wallet-ffi/src/asset_lock_persistence.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_create_transition/v0/v0_methods.rs
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
• packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
• packages/rs-platform-wallet-ffi/src/persistence.rs
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
• packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/RegistrationProgressView.swift
• packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Environment variable propagation inconsistency with pitfall warning.

The tap_label function sets environment variables using LABEL="$label" UDID="$udid" command | python3 -c "..." (lines 66-81), but lines 258-264 warn that "Env vars don't propagate into python3 -c '...' the way they do into bash" and recommend using a heredoc form instead.

While this pattern works in bash, it may fail in other shells or configurations, causing the Python script to see empty environment variables and the function to fail silently.

🛡️ Proposed fix for shell portability

 tap_label() {
     local label="$1"
     local udid=$(xcrun simctl list devices booted | awk -F'[()]' '/Booted/ {print $2}')
-    LABEL="$label" UDID="$udid" "$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>&1 \
-        | python3 -c "
+    export LABEL="$label"
+    export UDID="$udid"
+    "$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>&1 | python3 -c "
 import json, os, subprocess, sys
 items = json.loads(sys.stdin.read())
 label = os.environ['LABEL']

Or use the heredoc pattern recommended in the pitfalls section for maximum portability.

📝 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.

Suggested change

	tap_label() {
	local label="$1"
	local udid=$(xcrun simctl list devices booted | awk -F'[()]' '/Booted/ {print $2}')
	LABEL="$label" UDID="$udid" "$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>&1 \
	| python3 -c "
	import json, os, subprocess, sys
	items = json.loads(sys.stdin.read())
	label = os.environ['LABEL']
	# Exact match first, fall back to substring
	match = next((it for it in items if it.get('AXLabel') == label and it.get('enabled')), None)
	if not match:
	match = next((it for it in items if label in (it.get('AXLabel') or '') and it.get('enabled')), None)
	if not match:
	print(f'no enabled element matching {label!r}', file=sys.stderr); sys.exit(1)
	f = match['frame']
	x, y = int(f['x'] + f['width']/2), int(f['y'] + f['height']/2)
	subprocess.run([os.path.expanduser('~/.local/bin/idb'), 'ui', 'tap', '--udid', os.environ['UDID'], str(x), str(y)], check=True)
	print(f'tapped {match.get(\"AXLabel\")!r} ({match.get(\"type\")}) at ({x},{y})')
	"
	}
	tap_label() {
	local label="$1"
	local udid=$(xcrun simctl list devices booted | awk -F'[()]' '/Booted/ {print $2}')
	export LABEL="$label"
	export UDID="$udid"
	"$HOME/.local/bin/idb" ui describe-all --udid "$udid" 2>&1 | python3 -c "
	import json, os, subprocess, sys
	items = json.loads(sys.stdin.read())
	label = os.environ['LABEL']
	# Exact match first, fall back to substring
	match = next((it for it in items if it.get('AXLabel') == label and it.get('enabled')), None)
	if not match:
	match = next((it for it in items if label in (it.get('AXLabel') or '') and it.get('enabled')), None)
	if not match:
	print(f'no enabled element matching {label!r}', file=sys.stderr); sys.exit(1)
	f = match['frame']
	x, y = int(f['x'] + f['width']/2), int(f['y'] + f['height']/2)
	subprocess.run([os.path.expanduser('~/.local/bin/idb'), 'ui', 'tap', '--udid', os.environ['UDID'], str(x), str(y)], check=True)
	print(f'tapped {match.get(\"AXLabel\")!r} ({match.get(\"type\")}) at ({x},{y})')
	"
	}

🤖 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 @.claude/skills/simulator-control/SKILL.md around lines 63 - 82, The
tap_label function exports LABEL and UDID inline before invoking a python -c
string which can fail to receive those env vars in non-bash shells; update
tap_label to pass LABEL and UDID into the Python interpreter via a heredoc
(invoke python3 with a here-doc block) or explicitly export them in the shell
environment before calling idb so the Python code can read os.environ['LABEL']
and os.environ['UDID'] reliably; locate the tap_label function and the inline
python3 -c invocation and replace it with a heredoc form (or an explicit export
of LABEL/UDID) so environment propagation is portable across shells.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix predicate to exclude Consumed status.

canFundIdentity currently returns true for Consumed locks (statusRaw == 4), but consumed locks have already been spent and cannot fund identities. The Resume button should not be enabled for these.

🐛 Proposed fix to exclude Consumed status

-    var canFundIdentity: Bool { statusRaw >= 2 }
+    var canFundIdentity: Bool { statusRaw == 2 || statusRaw == 3 }

Alternatively, if additional finalized states are added between ChainLocked and Consumed in the future:

-    var canFundIdentity: Bool { statusRaw >= 2 }
+    var canFundIdentity: Bool { statusRaw >= 2 && statusRaw < 4 }

📝 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.

Suggested change

	var canFundIdentity: Bool { statusRaw >= 2 }
	var canFundIdentity: Bool { statusRaw == 2 || statusRaw == 3 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift`
at line 26, PersistentAssetLockDisplay's canFundIdentity currently returns true
for Consumed (statusRaw == 4); update the predicate to exclude Consumed so spent
locks can't fund identities. Locate the canFundIdentity computed property in
PersistentAssetLockDisplay and change its logic to require statusRaw >= 2 AND
statusRaw != 4 (i.e., allow ChainLocked and other non-consumed finalized states
but explicitly disallow Consumed); reference the canFundIdentity property and
statusRaw field when making the change.

[thepastaclaw]

Code Review

The current head fixes the new chain-lock watermark persistence path, but it does not resolve the three blocking issues carried forward from the earlier review. One additional restore-path limitation remains live as a non-blocking recovery gap: it is now documented in comments, but the runtime behavior is unchanged.

Reviewed commit: 65e417d

🔴 2 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 358-425: Repeated catch-up passes can destroy or stall asset-lock managers that detached tasks are still using
  `catchUpStuckAssetLocks()` clears `retainedAssetLockManagers` at line 365 before it launches the next batch, but the previous batch's detached tasks still hold only the raw `handle` captured at lines 396-425. Releasing the wrapper runs `asset_lock_manager_destroy()` in `ManagedAssetLockManager.deinit`, and the Rust FFI side resolves that handle through `ASSET_LOCK_MANAGER_STORAGE.with_item(...)` for the full synchronous `runtime().block_on(manager.resume_asset_lock(...))` call. That creates two bad outcomes on a second foreground/reconnect pass: `deinit` can synchronously block the main actor waiting for the handle-table write lock, and any later tasks from the first batch can lose the handle once destroy finally removes it. The comments claiming prior tasks have already completed or timed out are not enforced anywhere in this method.

In `packages/rs-platform-wallet-ffi/src/mnemonic_resolver_core_signer.rs`:
- [BLOCKING] lines 301-334: The mnemonic-backed signer still leaves a second private-key copy in `SecretKey` memory
  `derive_priv()` returns `Zeroizing<[u8; 32]>`, but both `sign_ecdsa()` and `public_key()` immediately copy those bytes into a separate `secp256k1::SecretKey` with `SecretKey::from_slice(...)`. Zeroizing `secret_bytes` only wipes the original buffer; it does not wipe the copied scalar owned by `secret`. The current comment at lines 317-322 is therefore incorrect, and the implementation still violates the stated FFI security goal that mnemonic-derived private key material not remain resident on the Rust side after the call.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2859-2964: The restore path still hardcodes asset-lock account index 0
  Both restore builders still write `account_index = 0`: `buildAssetLockRestoreBuffer()` at line 2864 and `buildUnresolvedAssetLockTxRecordBuffer()` at line 2964. That value is load-bearing on the Rust side: tracked locks are keyed under `map.entry(spec.account_index)`, and unresolved funding transactions are restored through `standard_bip44_accounts.get_mut(&rec.account_index)` and dropped when that account does not exist. The new comments document this as a known limitation, but the public Swift SDK surface still exposes nonzero `accountIndex` on asset-lock creation APIs, so restart recovery for locks funded from other BIP44 accounts remains incorrect.

[thepastaclaw]

🔴 Blocking: Repeated catch-up passes can destroy or stall asset-lock managers that detached tasks are still using

catchUpStuckAssetLocks() clears retainedAssetLockManagers at line 365 before it launches the next batch, but the previous batch's detached tasks still hold only the raw handle captured at lines 396-425. Releasing the wrapper runs asset_lock_manager_destroy() in ManagedAssetLockManager.deinit, and the Rust FFI side resolves that handle through ASSET_LOCK_MANAGER_STORAGE.with_item(...) for the full synchronous runtime().block_on(manager.resume_asset_lock(...)) call. That creates two bad outcomes on a second foreground/reconnect pass: deinit can synchronously block the main actor waiting for the handle-table write lock, and any later tasks from the first batch can lose the handle once destroy finally removes it. The comments claiming prior tasks have already completed or timed out are not enforced anywhere in this method.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 358-425: Repeated catch-up passes can destroy or stall asset-lock managers that detached tasks are still using
  `catchUpStuckAssetLocks()` clears `retainedAssetLockManagers` at line 365 before it launches the next batch, but the previous batch's detached tasks still hold only the raw `handle` captured at lines 396-425. Releasing the wrapper runs `asset_lock_manager_destroy()` in `ManagedAssetLockManager.deinit`, and the Rust FFI side resolves that handle through `ASSET_LOCK_MANAGER_STORAGE.with_item(...)` for the full synchronous `runtime().block_on(manager.resume_asset_lock(...))` call. That creates two bad outcomes on a second foreground/reconnect pass: `deinit` can synchronously block the main actor waiting for the handle-table write lock, and any later tasks from the first batch can lose the handle once destroy finally removes it. The comments claiming prior tasks have already completed or timed out are not enforced anywhere in this method.

[thepastaclaw]

🔴 Blocking: The mnemonic-backed signer still leaves a second private-key copy in SecretKey memory

derive_priv() returns Zeroizing<[u8; 32]>, but both sign_ecdsa() and public_key() immediately copy those bytes into a separate secp256k1::SecretKey with SecretKey::from_slice(...). Zeroizing secret_bytes only wipes the original buffer; it does not wipe the copied scalar owned by secret. The current comment at lines 317-322 is therefore incorrect, and the implementation still violates the stated FFI security goal that mnemonic-derived private key material not remain resident on the Rust side after the call.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-ffi/src/mnemonic_resolver_core_signer.rs`:
- [BLOCKING] lines 301-334: The mnemonic-backed signer still leaves a second private-key copy in `SecretKey` memory
  `derive_priv()` returns `Zeroizing<[u8; 32]>`, but both `sign_ecdsa()` and `public_key()` immediately copy those bytes into a separate `secp256k1::SecretKey` with `SecretKey::from_slice(...)`. Zeroizing `secret_bytes` only wipes the original buffer; it does not wipe the copied scalar owned by `secret`. The current comment at lines 317-322 is therefore incorrect, and the implementation still violates the stated FFI security goal that mnemonic-derived private key material not remain resident on the Rust side after the call.

[thepastaclaw]

🟡 Suggestion: The restore path still hardcodes asset-lock account index 0

Both restore builders still write account_index = 0: buildAssetLockRestoreBuffer() at line 2864 and buildUnresolvedAssetLockTxRecordBuffer() at line 2964. That value is load-bearing on the Rust side: tracked locks are keyed under map.entry(spec.account_index), and unresolved funding transactions are restored through standard_bip44_accounts.get_mut(&rec.account_index) and dropped when that account does not exist. The new comments document this as a known limitation, but the public Swift SDK surface still exposes nonzero accountIndex on asset-lock creation APIs, so restart recovery for locks funded from other BIP44 accounts remains incorrect.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2859-2964: The restore path still hardcodes asset-lock account index 0
  Both restore builders still write `account_index = 0`: `buildAssetLockRestoreBuffer()` at line 2864 and `buildUnresolvedAssetLockTxRecordBuffer()` at line 2964. That value is load-bearing on the Rust side: tracked locks are keyed under `map.entry(spec.account_index)`, and unresolved funding transactions are restored through `standard_bip44_accounts.get_mut(&rec.account_index)` and dropped when that account does not exist. The new comments document this as a known limitation, but the public Swift SDK surface still exposes nonzero `accountIndex` on asset-lock creation APIs, so restart recovery for locks funded from other BIP44 accounts remains incorrect.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs (1)

240-242: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix broken test imports and stale doc links that reference removed symbols.

The test module imports dash_sdk_mnemonic_resolver_create and dash_sdk_mnemonic_resolver_destroy from crate::derive_and_persist_callbacks (lines 240-242), but these functions were removed from that module and are now in rs_sdk_ffi. This breaks compilation of the test suite. The make_resolver() helper at line 281 uses these broken imports directly.

Additionally, the module documentation contains four broken intra-doc links:

• Lines 4, 30: [MnemonicResolverHandle](crate::MnemonicResolverHandle) — the type is imported from rs_sdk_ffi
• Line 23: [MnemonicResolver](crate::MnemonicResolverHandle) — wrong crate path
• Line 94: [crate::dash_sdk_mnemonic_resolver_create] — function is in rs_sdk_ffi, not crate

Proposed fixes

 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::derive_and_persist_callbacks::{
-        dash_sdk_mnemonic_resolver_create, dash_sdk_mnemonic_resolver_destroy,
-    };
+    use rs_sdk_ffi::{dash_sdk_mnemonic_resolver_create, dash_sdk_mnemonic_resolver_destroy};
     use std::ffi::CString;

Update doc link paths from crate:: to rs_sdk_ffi:: for MnemonicResolverHandle (lines 4, 30) and dash_sdk_mnemonic_resolver_create (line 94). Fix line 23 link text to match the target type.

🤖 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/sign_with_mnemonic_resolver.rs` around
lines 240 - 242, The test imports and intra-doc links reference removed symbols
in crate::derive_and_persist_callbacks and crate:: paths; change the use
declarations to import dash_sdk_mnemonic_resolver_create and
dash_sdk_mnemonic_resolver_destroy from rs_sdk_ffi (so make_resolver() uses the
correct functions), and update the doc links to point to
rs_sdk_ffi::MnemonicResolverHandle (both occurrences), correct the link text for
MnemonicResolver to the actual type name, and change the
dash_sdk_mnemonic_resolver_create doc link to
rs_sdk_ffi::dash_sdk_mnemonic_resolver_create so all references resolve to the
relocated symbols.

♻️ Duplicate comments (1)

packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs (1)

303-337: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Mnemonic-derived private scalar still leaks into SecretKey memory after signing.

SecretKey::from_slice(secret_bytes.as_ref()) copies the 32 scalar bytes into a fresh SecretKey([u8; 32]). Dropping secret_bytes (Zeroizing) only wipes the original buffer; the copy owned by secret is left intact (no Drop zeroizes it), so the comment on lines 319-324 is not correct and the module-level claim that "no private key bytes survive past the trait-method boundary" is violated for both sign_ecdsa and public_key. secp256k1::SecretKey exposes non_secure_erase() specifically for this — call it before returning.

🔒 Proposed fix

     async fn sign_ecdsa(
         &self,
         path: &DerivationPath,
         sighash: [u8; 32],
     ) -> Result<(secp256k1::ecdsa::Signature, secp256k1::PublicKey), Self::Error> {
         let secret_bytes = self.derive_priv(path)?;
         let secp = Secp256k1::new();
-        let secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
+        let mut secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
             .map_err(|e| MnemonicResolverSignerError::InvalidScalar(e.to_string()))?;
         let msg = secp256k1::Message::from_digest(sighash);
         let signature = secp.sign_ecdsa(&msg, &secret);
         let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
-        // `secp256k1::SecretKey` is `Copy` (a thin wrapper over a
-        // 32-byte buffer) and doesn't itself zero on drop — but the
-        // backing buffer here came from `secret_bytes`
-        // (a `Zeroizing<[u8; 32]>`), which IS wiped when it falls
-        // out of scope below. The `secret` binding is forgotten by
-        // letting it go out of scope; no explicit `drop` needed.
-        let _ = secret;
+        // `secp256k1::SecretKey` owns its own [u8; 32] copy of the
+        // scalar; `secret_bytes` zeroization does not reach it. Wipe
+        // the copy explicitly before returning.
+        secret.non_secure_erase();
         Ok((signature, pubkey))
     }
 
     async fn public_key(&self, path: &DerivationPath) -> Result<secp256k1::PublicKey, Self::Error> {
         let secret_bytes = self.derive_priv(path)?;
         let secp = Secp256k1::new();
-        let secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
+        let mut secret = secp256k1::SecretKey::from_slice(secret_bytes.as_ref())
             .map_err(|e| MnemonicResolverSignerError::InvalidScalar(e.to_string()))?;
         let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &secret);
-        let _ = secret;
+        secret.non_secure_erase();
         Ok(pubkey)
     }

Confirm the non_secure_erase() API is available on the SecretKey re-exported by key_wallet::dashcore::secp256k1 in the version pinned by this workspace:

#!/bin/bash
# Find the secp256k1 version key-wallet/dashcore pull in, and confirm
# SecretKey::non_secure_erase exists in that version's source.
fd -t f Cargo.lock | head -n 5
rg -nP '^name = "secp256k1"' -A2 Cargo.lock | head -n 60
rg -nP '\bnon_secure_erase\b' -C2

🤖 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-sdk-ffi/src/mnemonic_resolver_core_signer.rs` around lines 303 -
337, The secret scalar copy created by SecretKey::from_slice in the functions
sign_ecdsa and public_key remains in memory because secp256k1::SecretKey does
not zero on drop; call SecretKey::non_secure_erase() on the secret binding (the
variable named secret) just before returning from both sign_ecdsa and public_key
(i.e., after creating pubkey/signature and before Ok(...)) so the copied scalar
is wiped; keep the existing error mapping from derive_priv and from_slice
unchanged and ensure secret_bytes (Zeroizing) still drops as before. Also verify
that SecretKey::non_secure_erase exists in the re-exported secp256k1 version
before committing.

🧹 Nitpick comments (1)

packages/rs-sdk/src/platform/transition/top_up_identity.rs (1)

59-69: 💤 Low value

Avoid silently relying on PutSettings: Copy when threading settings twice.

Both impls do settings.and_then(|s| s.user_fee_increase).unwrap_or_default() at line 59 and line 88, then pass settings again to broadcast_and_wait at line 69 and line 100. This only compiles because Option<PutSettings> is currently Copy. The moment any non-Copy field is added to PutSettings (or if RequestSettings ever loses Copy), both call sites break in a non-obvious way. Borrowing via as_ref() makes the intent explicit and decouples this code from PutSettings's Copy derivation.

♻️ Proposed defensive refactor

     async fn top_up_identity_with_private_key(
         &self,
         sdk: &Sdk,
         asset_lock_proof: AssetLockProof,
         asset_lock_proof_private_key: &PrivateKey,
         settings: Option<PutSettings>,
     ) -> Result<u64, Error> {
-        let user_fee_increase = settings.and_then(|s| s.user_fee_increase).unwrap_or_default();
+        let user_fee_increase = settings
+            .as_ref()
+            .and_then(|s| s.user_fee_increase)
+            .unwrap_or_default();

     async fn top_up_identity_with_signer<AS>(
         ...
     ) -> Result<u64, Error>
     where
         AS: dpp::key_wallet::signer::Signer + Send + Sync,
     {
-        let user_fee_increase = settings.and_then(|s| s.user_fee_increase).unwrap_or_default();
+        let user_fee_increase = settings
+            .as_ref()
+            .and_then(|s| s.user_fee_increase)
+            .unwrap_or_default();

Also applies to: 88-100

🤖 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-sdk/src/platform/transition/top_up_identity.rs` around lines 59 -
69, The code currently relies on Option<PutSettings> being Copy by calling
settings.and_then(...) twice and passing settings by value to
broadcast_and_wait; change the extraction to borrow the option when reading
user_fee_increase (use settings.as_ref().and_then(|s|
s.user_fee_increase).unwrap_or_default()) and then pass an owned Option to
broadcast_and_wait by cloning when needed (e.g., settings.clone() or
settings.as_ref().cloned()) so that
IdentityTopUpTransition::try_from_identity_with_private_key and
broadcast_and_wait calls no longer depend on PutSettings being Copy; apply the
same fix to both occurrences around the user_fee_increase extraction and the
broadcast_and_wait call.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-platform-wallet-ffi/src/lib.rs`:
- Line 13: The module asset_lock_persistence is declared but not re-exported
like the other sibling modules; either add a re-export (pub use
asset_lock_persistence::*;) to the existing re-export block that contains the
other pub use ...::* entries so the crate API remains consistent, or if you
intentionally want to keep its symbols namespaced, add a brief comment next to
the pub mod asset_lock_persistence declaration explaining that omission (e.g.,
“intentionally not re-exported to preserve namespace”) so reviewers understand
it is deliberate.

---

Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs`:
- Around line 240-242: The test imports and intra-doc links reference removed
symbols in crate::derive_and_persist_callbacks and crate:: paths; change the use
declarations to import dash_sdk_mnemonic_resolver_create and
dash_sdk_mnemonic_resolver_destroy from rs_sdk_ffi (so make_resolver() uses the
correct functions), and update the doc links to point to
rs_sdk_ffi::MnemonicResolverHandle (both occurrences), correct the link text for
MnemonicResolver to the actual type name, and change the
dash_sdk_mnemonic_resolver_create doc link to
rs_sdk_ffi::dash_sdk_mnemonic_resolver_create so all references resolve to the
relocated symbols.

---

Duplicate comments:
In `@packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- Around line 303-337: The secret scalar copy created by SecretKey::from_slice
in the functions sign_ecdsa and public_key remains in memory because
secp256k1::SecretKey does not zero on drop; call SecretKey::non_secure_erase()
on the secret binding (the variable named secret) just before returning from
both sign_ecdsa and public_key (i.e., after creating pubkey/signature and before
Ok(...)) so the copied scalar is wiped; keep the existing error mapping from
derive_priv and from_slice unchanged and ensure secret_bytes (Zeroizing) still
drops as before. Also verify that SecretKey::non_secure_erase exists in the
re-exported secp256k1 version before committing.

---

Nitpick comments:
In `@packages/rs-sdk/src/platform/transition/top_up_identity.rs`:
- Around line 59-69: The code currently relies on Option<PutSettings> being Copy
by calling settings.and_then(...) twice and passing settings by value to
broadcast_and_wait; change the extraction to borrow the option when reading
user_fee_increase (use settings.as_ref().and_then(|s|
s.user_fee_increase).unwrap_or_default()) and then pass an owned Option to
broadcast_and_wait by cloning when needed (e.g., settings.clone() or
settings.as_ref().cloned()) so that
IdentityTopUpTransition::try_from_identity_with_private_key and
broadcast_and_wait calls no longer depend on PutSettings being Copy; apply the
same fix to both occurrences around the user_fee_increase extraction and the
broadcast_and_wait call.

🪄 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: f995f992-27c3-43c9-ae8d-20ce0d575fd5

📥 Commits

Reviewing files that changed from the base of the PR and between 65e417d and e01fc9d.

📒 Files selected for processing (18)

• packages/rs-platform-wallet-ffi/src/asset_lock/build.rs
• packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
• packages/rs-platform-wallet-ffi/src/derive_and_persist_callbacks.rs
• packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs
• packages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rs
• packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs
• packages/rs-platform-wallet-ffi/src/lib.rs
• packages/rs-platform-wallet-ffi/src/shielded_sync.rs
• packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs
• packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
• packages/rs-sdk-ffi/src/identity/topup.rs
• packages/rs-sdk-ffi/src/lib.rs
• packages/rs-sdk-ffi/src/mnemonic_resolver.rs
• packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs
• packages/rs-sdk/src/platform/transition/broadcast_identity.rs
• packages/rs-sdk/src/platform/transition/put_identity.rs
• packages/rs-sdk/src/platform/transition/top_up_identity.rs
• packages/wasm-sdk/src/state_transitions/identity.rs

✅ Files skipped from review due to trivial changes (3)

• packages/rs-platform-wallet-ffi/src/shielded_sync.rs
• packages/rs-platform-wallet-ffi/src/derive_identity_key_at_slot.rs
• packages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rs

🚧 Files skipped from review as they are similar to previous changes (6)

• packages/wasm-sdk/src/state_transitions/identity.rs
• packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
• packages/rs-sdk/src/platform/transition/put_identity.rs
• packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs
• packages/rs-platform-wallet-ffi/src/asset_lock/build.rs
• packages/rs-platform-wallet/src/wallet/identity/network/registration.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 1. Inspect what asset_lock_persistence exports.
fd -t f asset_lock_persistence.rs packages/rs-platform-wallet-ffi
rg -nP '^\s*(pub\s+(unsafe\s+)?(extern\s+"C"\s+)?(fn|struct|enum|type|const|static)|pub\s+use)\b' packages/rs-platform-wallet-ffi/src/asset_lock_persistence.rs 2>/dev/null

# 2. Look for symbol-name collisions with asset_lock.
rg -nP '^\s*pub\s+(unsafe\s+)?(extern\s+"C"\s+)?fn\s+(\w+)' packages/rs-platform-wallet-ffi/src/asset_lock.rs \
  packages/rs-platform-wallet-ffi/src/asset_lock_persistence.rs 2>/dev/null | sort

# 3. See whether downstream callers reach the module through the flat namespace.
rg -nP '\basset_lock_persistence::' packages/rs-platform-wallet-ffi/src

Repository: dashpay/platform

Length of output: 931

---

🏁 Script executed:

# Read the module declarations and re-export block
cat -n packages/rs-platform-wallet-ffi/src/lib.rs | sed -n '1,130p'

Repository: dashpay/platform

Length of output: 4638

---

Add missing pub use asset_lock_persistence::*; or document why it's intentionally namespaced.

asset_lock_persistence is declared on line 13 but is not re-exported in the re-export block (lines 70–122), unlike every other public sibling module. If intentional (e.g., to keep symbols namespaced), add a comment explaining the rationale. Otherwise, add pub use asset_lock_persistence::*; to maintain consistency with the crate's pattern and flatten the namespace for downstream consumers.

🤖 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/lib.rs` at line 13, The module
asset_lock_persistence is declared but not re-exported like the other sibling
modules; either add a re-export (pub use asset_lock_persistence::*;) to the
existing re-export block that contains the other pub use ...::* entries so the
crate API remains consistent, or if you intentionally want to keep its symbols
namespaced, add a brief comment next to the pub mod asset_lock_persistence
declaration explaining that omission (e.g., “intentionally not re-exported to
preserve namespace”) so reviewers understand it is deliberate.

[thepastaclaw]

Code Review

I verified the cited paths against SHA 67e053296e5b311fd95fe3cbb4db5e819d7bb802. Four findings are real on this head: three blockers in the Swift/Rust asset-lock and signer paths, plus one restore limitation for nonzero BIP44 accounts. The fee-bump retry path also trusts an unverified server rejection before increasing user_fee_increase, so this PR still needs changes before merge.

Reviewed commit: 67e0532

🔴 3 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 360-425: `catchUpStuckAssetLocks` can destroy handles that detached catch-up tasks still need
  `catchUpStuckAssetLocks()` releases every previously retained `ManagedAssetLockManager` at line 365, but the detached work only keeps the raw `handle` captured at line 397. `ManagedAssetLockManager.deinit` calls `asset_lock_manager_destroy()`, and the Rust FFI resolves `asset_lock_manager_catch_up_blocking()` through `HandleStorage::with_item(...)`, which keeps the handle-table read lock for the entire synchronous `runtime().block_on(manager.resume_asset_lock(...))` call. A second catch-up pass can therefore block the main actor while `destroy()` waits for that read lock, then remove the handle before later tasks from the first batch run, causing them to fail with `ErrorInvalidHandle`. The comments claim the prior batch has already completed or timed out, but the method never tracks or enforces that invariant.

In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- [BLOCKING] lines 60-79: `consume_asset_lock()` computes the consumed snapshot but never persists it
  The method documentation says successful consumption must remove the lock from the in-memory map and upsert the persisted row with `status = Consumed`, but the implementation only builds an `AssetLockChangeSet` and returns it. Unlike the other asset-lock mutation paths, it never calls `queue_asset_lock_changeset()`, and both registration/top-up callers ignore the successful return value and only log `Err`. The result is that the new `Consumed` state exists only in process memory: after restart, Swift reloads the stale pre-consumption row and Rust treats the lock as actionable again.

In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] lines 137-186: The ChainLock-height retry loop trusts unverified 10506 responses before increasing user fees
  `submit_with_cl_height_retry()` treats any `InvalidAssetLockProofCoreChainHeightError` extracted by `as_asset_lock_proof_cl_height_too_low()` as authoritative and immediately bumps `PutSettings.user_fee_increase` before re-signing. That signal is not locally verified. `wait_for_response()` in `rs-sdk` accepts `wait_for_state_transition_result` error payloads directly from the DAPI server, deserializes the embedded consensus error, and returns it without any proof or independent check. A malicious or compromised endpoint can therefore fabricate a 10506 rejection once, force the wallet onto a higher-fee retry, and let a later attempt succeed with inflated fees. The retry budget is bounded, but the trust boundary is still wrong because an untrusted network peer is allowed to trigger fee escalation.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2859-2964: Asset-lock restore still hardcodes `account_index = 0` even though Rust uses that field to route recovery
  Both restore builders write `account_index = 0`: the tracked-lock snapshot at lines 2859-2864 and the unresolved funding-transaction buffer at lines 2955-2964. On the Rust side that field is load-bearing. `build_unused_asset_locks()` stores tracked locks under `spec.account_index`, `recover_asset_lock_blocking()` looks up the funding transaction in `standard_bip44_accounts.get(&account_index)`, and `restore_unresolved_asset_lock_tx_records()` drops a row if the matching account does not exist. The public Swift asset-lock APIs already accept arbitrary `accountIndex`, so a lock funded from a nonzero BIP44 account will not recover correctly after restart.

[thepastaclaw]

🔴 Blocking: catchUpStuckAssetLocks can destroy handles that detached catch-up tasks still need

catchUpStuckAssetLocks() releases every previously retained ManagedAssetLockManager at line 365, but the detached work only keeps the raw handle captured at line 397. ManagedAssetLockManager.deinit calls asset_lock_manager_destroy(), and the Rust FFI resolves asset_lock_manager_catch_up_blocking() through HandleStorage::with_item(...), which keeps the handle-table read lock for the entire synchronous runtime().block_on(manager.resume_asset_lock(...)) call. A second catch-up pass can therefore block the main actor while destroy() waits for that read lock, then remove the handle before later tasks from the first batch run, causing them to fail with ErrorInvalidHandle. The comments claim the prior batch has already completed or timed out, but the method never tracks or enforces that invariant.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 360-425: `catchUpStuckAssetLocks` can destroy handles that detached catch-up tasks still need
  `catchUpStuckAssetLocks()` releases every previously retained `ManagedAssetLockManager` at line 365, but the detached work only keeps the raw `handle` captured at line 397. `ManagedAssetLockManager.deinit` calls `asset_lock_manager_destroy()`, and the Rust FFI resolves `asset_lock_manager_catch_up_blocking()` through `HandleStorage::with_item(...)`, which keeps the handle-table read lock for the entire synchronous `runtime().block_on(manager.resume_asset_lock(...))` call. A second catch-up pass can therefore block the main actor while `destroy()` waits for that read lock, then remove the handle before later tasks from the first batch run, causing them to fail with `ErrorInvalidHandle`. The comments claim the prior batch has already completed or timed out, but the method never tracks or enforces that invariant.

[thepastaclaw]

🔴 Blocking: consume_asset_lock() computes the consumed snapshot but never persists it

The method documentation says successful consumption must remove the lock from the in-memory map and upsert the persisted row with status = Consumed, but the implementation only builds an AssetLockChangeSet and returns it. Unlike the other asset-lock mutation paths, it never calls queue_asset_lock_changeset(), and both registration/top-up callers ignore the successful return value and only log Err. The result is that the new Consumed state exists only in process memory: after restart, Swift reloads the stale pre-consumption row and Rust treats the lock as actionable again.

💡 Suggested change

Suggested change

	pub(crate) async fn consume_asset_lock(
	&self,
	out_point: &OutPoint,
	) -> Result<AssetLockChangeSet, PlatformWalletError> {
	let mut wm = self.wallet_manager.write().await;
	let info = wm
	.get_wallet_info_mut(&self.wallet_id)
	.ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?;
	let mut cs = AssetLockChangeSet::default();
	if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) {
	if info.tracked_asset_locks.remove(out_point).is_some() {
	cs.removed.insert(*out_point);
	}
	if let Some(mut entry) = info.tracked_asset_locks.remove(out_point) {
	entry.status = AssetLockStatus::Consumed;
	entry.proof = None; // one-shot — never relevant after consumption
	cs.asset_locks.insert(*out_point, (&entry).into());
	} else {
	tracing::warn!(
	outpoint = %out_point,
	"consume_asset_lock: outpoint not tracked — already consumed or never present"
	);
	}
	cs
	Ok(cs)
	pub(crate) async fn consume_asset_lock(
	&self,
	out_point: &OutPoint,
	) -> Result<AssetLockChangeSet, PlatformWalletError> {
	let cs = {
	let mut wm = self.wallet_manager.write().await;
	let info = wm
	.get_wallet_info_mut(&self.wallet_id)
	.ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?;
	let mut cs = AssetLockChangeSet::default();
	if let Some(mut entry) = info.tracked_asset_locks.remove(out_point) {
	entry.status = AssetLockStatus::Consumed;
	entry.proof = None;
	cs.asset_locks.insert(*out_point, (&entry).into());
	} else {
	tracing::warn!(
	outpoint = %out_point,
	"consume_asset_lock: outpoint not tracked — already consumed or never present"
	);
	}
	cs
	};
	self.queue_asset_lock_changeset(cs.clone());
	Ok(cs)
	}

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs`:
- [BLOCKING] lines 60-79: `consume_asset_lock()` computes the consumed snapshot but never persists it
  The method documentation says successful consumption must remove the lock from the in-memory map and upsert the persisted row with `status = Consumed`, but the implementation only builds an `AssetLockChangeSet` and returns it. Unlike the other asset-lock mutation paths, it never calls `queue_asset_lock_changeset()`, and both registration/top-up callers ignore the successful return value and only log `Err`. The result is that the new `Consumed` state exists only in process memory: after restart, Swift reloads the stale pre-consumption row and Rust treats the lock as actionable again.

[thepastaclaw]

🔴 Blocking: The ChainLock-height retry loop trusts unverified 10506 responses before increasing user fees

submit_with_cl_height_retry() treats any InvalidAssetLockProofCoreChainHeightError extracted by as_asset_lock_proof_cl_height_too_low() as authoritative and immediately bumps PutSettings.user_fee_increase before re-signing. That signal is not locally verified. wait_for_response() in rs-sdk accepts wait_for_state_transition_result error payloads directly from the DAPI server, deserializes the embedded consensus error, and returns it without any proof or independent check. A malicious or compromised endpoint can therefore fabricate a 10506 rejection once, force the wallet onto a higher-fee retry, and let a later attempt succeed with inflated fees. The retry budget is bounded, but the trust boundary is still wrong because an untrusted network peer is allowed to trigger fee escalation.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] lines 137-186: The ChainLock-height retry loop trusts unverified 10506 responses before increasing user fees
  `submit_with_cl_height_retry()` treats any `InvalidAssetLockProofCoreChainHeightError` extracted by `as_asset_lock_proof_cl_height_too_low()` as authoritative and immediately bumps `PutSettings.user_fee_increase` before re-signing. That signal is not locally verified. `wait_for_response()` in `rs-sdk` accepts `wait_for_state_transition_result` error payloads directly from the DAPI server, deserializes the embedded consensus error, and returns it without any proof or independent check. A malicious or compromised endpoint can therefore fabricate a 10506 rejection once, force the wallet onto a higher-fee retry, and let a later attempt succeed with inflated fees. The retry budget is bounded, but the trust boundary is still wrong because an untrusted network peer is allowed to trigger fee escalation.

[thepastaclaw]

🟡 Suggestion: Asset-lock restore still hardcodes account_index = 0 even though Rust uses that field to route recovery

Both restore builders write account_index = 0: the tracked-lock snapshot at lines 2859-2864 and the unresolved funding-transaction buffer at lines 2955-2964. On the Rust side that field is load-bearing. build_unused_asset_locks() stores tracked locks under spec.account_index, recover_asset_lock_blocking() looks up the funding transaction in standard_bip44_accounts.get(&account_index), and restore_unresolved_asset_lock_tx_records() drops a row if the matching account does not exist. The public Swift asset-lock APIs already accept arbitrary accountIndex, so a lock funded from a nonzero BIP44 account will not recover correctly after restart.

_{source: ['codex']}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2859-2964: Asset-lock restore still hardcodes `account_index = 0` even though Rust uses that field to route recovery
  Both restore builders write `account_index = 0`: the tracked-lock snapshot at lines 2859-2864 and the unresolved funding-transaction buffer at lines 2955-2964. On the Rust side that field is load-bearing. `build_unused_asset_locks()` stores tracked locks under `spec.account_index`, `recover_asset_lock_blocking()` looks up the funding transaction in `standard_bip44_accounts.get(&account_index)`, and `restore_unresolved_asset_lock_tx_records()` drops a row if the matching account does not exist. The public Swift asset-lock APIs already accept arbitrary `accountIndex`, so a lock funded from a nonzero BIP44 account will not recover correctly after restart.