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(rs-platform-wallet): e2e suite, Found-025 fix + triage 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

• New Features

  • Resumable identity registrations with persisted asset-locks, background catch‑up, progress UI, and single‑flight registration coordinator
  • Support for external signer flows (host-backed signing) for transactions, asset-locks, and identity operations

• Improvements

  • ChainLock fallback for InstantSend timeouts and broader ChainLock handling (more reliable finality)
  • SPV startup now enables masternode sync by default for correct asset‑lock proofs
  • Improved asset‑lock/transaction display, “Consumed” state visibility, and storage views

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

<review_stack_artifact>

</review_stack_artifact>

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

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

[shumkov]

Fixed in 3abd581a01. ManagedAssetLockManager is now final + @unchecked Sendable (immutable let handle, deinit runs exactly once via ARC) and each catch-up task captures the wrapper directly instead of the raw Handle. The wrapper's deinit (which destroys the FFI handle) cannot fire until the task drops its retain. The retainedAssetLockManagers workaround property is gone. Concurrent invocations of catchUpStuckAssetLocks no longer race on each other's handles.

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

[shumkov]

Fixed in 8a8545c945. consume_asset_lock now queues the Consumed changeset internally (write lock released before queueing). Doc-comment explains why this method queues internally while sibling mutators defer to in-module callers (queue_asset_lock_changeset is pub(super)-scoped; registration lives outside the asset_lock module). Test gap surfaced in the commit message — full AssetLockManager scaffolding doesn't exist yet; end-to-end registration flow exercises this path.

[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()`.

[shumkov]

Fixed in 13531ef5d2. secret.non_secure_erase() is now called on the secp256k1::SecretKey before drop in both sign_ecdsa and public_key; the misleading "backing buffer aliasing" comment is replaced. Zeroizing<[u8;32]> still covers the source buffer; the new wipe covers the SecretKey-owned scalar copy.

[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

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

[shumkov]

Fixed in 3abd581a01. ManagedAssetLockManager is now final + @unchecked Sendable (immutable let handle, deinit runs exactly once via ARC) and each catch-up task captures the wrapper directly instead of the raw Handle. The wrapper's deinit (which destroys the FFI handle) cannot fire until the task drops its retain. The retainedAssetLockManagers workaround property is gone. Concurrent invocations of catchUpStuckAssetLocks no longer race on each other's handles.

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

[shumkov]

Fixed in 13531ef5d2. secret.non_secure_erase() is now called on the secp256k1::SecretKey before drop in both sign_ecdsa and public_key; the misleading "backing buffer aliasing" comment is replaced. Zeroizing<[u8;32]> still covers the source buffer; the new wipe covers the SecretKey-owned scalar copy.

[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

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

[shumkov]

Fixed in 3abd581a01. ManagedAssetLockManager is now final + @unchecked Sendable (immutable let handle, deinit runs exactly once via ARC) and each catch-up task captures the wrapper directly instead of the raw Handle. The wrapper's deinit (which destroys the FFI handle) cannot fire until the task drops its retain. The retainedAssetLockManagers workaround property is gone. Concurrent invocations of catchUpStuckAssetLocks no longer race on each other's handles.

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

[shumkov]

Fixed in 8a8545c945. consume_asset_lock now queues the Consumed changeset internally (write lock released before queueing). Doc-comment explains why this method queues internally while sibling mutators defer to in-module callers (queue_asset_lock_changeset is pub(super)-scoped; registration lives outside the asset_lock module). Test gap surfaced in the commit message — full AssetLockManager scaffolding doesn't exist yet; end-to-end registration flow exercises this path.

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

[shumkov]

Accepting as a known trust boundary, not a blocker. The retry loop treats the 10506 as authoritative because a node that fabricates rejections is a malicious DAPI node — the right defense is DAPI-client-level (don't submit to it again), not retry-loop-level. Bounded retry budget caps grief impact (~14 bumps max in 210s), and the attack is unprofitable because identity fees flow to Platform validators, not DAPI nodes. Verifiable consensus errors (quorum signature or validator attestation) would be the principled fix — tracked as future work; doing it in-place here would either re-implement DAPI client trust or require an SDK API change neither of which belong in this PR.

Doc comment sharpened in 3b5d841503 to spell this out for future readers.

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

[thepastaclaw]

Code Review

I verified the flagged areas against commit 710de74adc5031e67e35ac4cff55b40ed9f592f2. Three findings are blocking and reproducible from the current source: the Swift catch-up path can invalidate live FFI handles, the new chain-lock metadata persistence still misses a restart case the proof fallback depends on, and successful registration/top-up never persists the terminal Consumed asset-lock state. Two additional FFI mismatches are real but lower severity.

Reviewed commit: 710de74

🔴 3 blocking | 🟡 2 suggestion(s)

1 additional finding

🟡 suggestion: Funded registration FFI path discards `IdentityPubkeyFFI` contract bounds

packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs (lines 86-95)

Swift populates contract_bounds_kind, contract_bounds_id, and contract_bounds_document_type when it builds IdentityPubkeyFFI, and the older signer registration path decodes those fields with decode_contract_bounds(...) before constructing IdentityPublicKeyV0. This newer funded-with-signer decoder hard-codes contract_bounds: None, so the same Swift public key is interpreted differently depending on which FFI entrypoint is used. Any bounded key is silently converted into an unbounded one on this path.

💡 Suggested change

        let pubkey_bytes: Vec<u8> =
            slice::from_raw_parts(row.pubkey_bytes, row.pubkey_len).to_vec();
        let contract_bounds = crate::identity_registration_with_signer::decode_contract_bounds(
            row,
            purpose,
            i,
            "identity_pubkeys",
        )?;
        keys_map.insert(
            row.key_id,
            IdentityPublicKey::V0(IdentityPublicKeyV0 {
                id: row.key_id,
                purpose,
                security_level,
                contract_bounds,
                key_type,
                read_only: row.read_only,
                data: BinaryData::new(pubkey_bytes),
                disabled_at: None,
            }),
        );

🤖 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 365-439: Repeated catch-up calls can free asset-lock manager handles while detached tasks still use them
  `catchUpStuckAssetLocks` drops `retainedAssetLockManagers` at the start of every invocation, but the actual work runs in `Task.detached` closures that capture only the raw `handle`. `ManagedAssetLockManager.deinit` destroys that handle, so a second call to `catchUpStuckAssetLocks` within the 300-second catch-up window can deinitialize the previous batch's wrappers while its detached tasks are still inside `asset_lock_manager_catch_up_blocking`. The method comment explicitly advertises foreground/reconnect retries, so this is not a theoretical path: it creates a real use-after-free / invalid-handle race during recovery.

In `packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- [BLOCKING] lines 198-224: `last_applied_chain_lock` is not persisted when a chain lock advances metadata without promoting records
  The new proof fallback in `wallet/asset_lock/sync/proof.rs` builds a ChainLock proof from persisted `metadata.last_applied_chain_lock` when a restarted wallet reloads an `InBlock` record after the promotion event already passed. This bridge only persists `last_applied_chain_lock` from `WalletEvent::TransactionsChainlocked`, and the code comment confirms upstream only emits that event when `per_account` is non-empty. In the no-promotion case, the wallet's in-memory metadata advances but nothing is persisted, so after a restart the fallback still sees stale or missing chain-lock metadata and the asset lock can remain stuck until some later chain lock happens to promote another record.

In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] lines 567-579: Successful registration/top-up drops the consumed asset-lock changeset instead of persisting it
  `consume_asset_lock` now returns an `AssetLockChangeSet` whose documented purpose is to upsert the persisted row with `status = Consumed` while removing the in-memory entry. Both success paths ignore that returned changeset and only log errors, so Swift never receives the `Consumed` upsert. The result is that a successful registration or top-up leaves persistence at the prior status (`Built`/`Broadcast`/`InstantSendLocked`/`ChainLocked`), which breaks the historical lookup behavior described in `tracking.rs` and allows already-spent locks to come back on reload because only persisted `Consumed` rows are filtered out.

In `packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs`:
- [SUGGESTION] lines 86-95: Funded registration FFI path discards `IdentityPubkeyFFI` contract bounds
  Swift populates `contract_bounds_kind`, `contract_bounds_id`, and `contract_bounds_document_type` when it builds `IdentityPubkeyFFI`, and the older signer registration path decodes those fields with `decode_contract_bounds(...)` before constructing `IdentityPublicKeyV0`. This newer funded-with-signer decoder hard-codes `contract_bounds: None`, so the same Swift public key is interpreted differently depending on which FFI entrypoint is used. Any bounded key is silently converted into an unbounded one on this path.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2955-2965: Restore path hard-codes unresolved asset-lock transactions to BIP44 account 0
  The restore payload now includes `account_index`, and the Rust loader uses that field to reinsert each unresolved funding transaction into `standard_bip44_accounts[account_index]`. Swift always serializes `entry.account_index = 0`, and the inline comment acknowledges that the wrong value causes the Rust side to drop the record. The SDK surface already allows asset locks to be funded from arbitrary BIP44 account indexes, so restarting a wallet with unresolved asset locks from a non-zero funding account will route those transactions to the wrong account or drop them outright, breaking the intended restart recovery path for those wallets.

[thepastaclaw]

🔴 Blocking: Repeated catch-up calls can free asset-lock manager handles while detached tasks still use them

catchUpStuckAssetLocks drops retainedAssetLockManagers at the start of every invocation, but the actual work runs in Task.detached closures that capture only the raw handle. ManagedAssetLockManager.deinit destroys that handle, so a second call to catchUpStuckAssetLocks within the 300-second catch-up window can deinitialize the previous batch's wrappers while its detached tasks are still inside asset_lock_manager_catch_up_blocking. The method comment explicitly advertises foreground/reconnect retries, so this is not a theoretical path: it creates a real use-after-free / invalid-handle race during recovery.

_{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 365-439: Repeated catch-up calls can free asset-lock manager handles while detached tasks still use them
  `catchUpStuckAssetLocks` drops `retainedAssetLockManagers` at the start of every invocation, but the actual work runs in `Task.detached` closures that capture only the raw `handle`. `ManagedAssetLockManager.deinit` destroys that handle, so a second call to `catchUpStuckAssetLocks` within the 300-second catch-up window can deinitialize the previous batch's wrappers while its detached tasks are still inside `asset_lock_manager_catch_up_blocking`. The method comment explicitly advertises foreground/reconnect retries, so this is not a theoretical path: it creates a real use-after-free / invalid-handle race during recovery.

[shumkov]

Fixed in 3abd581a01. ManagedAssetLockManager is now final + @unchecked Sendable (immutable let handle, deinit runs exactly once via ARC) and each catch-up task captures the wrapper directly instead of the raw Handle. The wrapper's deinit (which destroys the FFI handle) cannot fire until the task drops its retain. The retainedAssetLockManagers workaround property is gone. Concurrent invocations of catchUpStuckAssetLocks no longer race on each other's handles.

[thepastaclaw]

🔴 Blocking: last_applied_chain_lock is not persisted when a chain lock advances metadata without promoting records

The new proof fallback in wallet/asset_lock/sync/proof.rs builds a ChainLock proof from persisted metadata.last_applied_chain_lock when a restarted wallet reloads an InBlock record after the promotion event already passed. This bridge only persists last_applied_chain_lock from WalletEvent::TransactionsChainlocked, and the code comment confirms upstream only emits that event when per_account is non-empty. In the no-promotion case, the wallet's in-memory metadata advances but nothing is persisted, so after a restart the fallback still sees stale or missing chain-lock metadata and the asset lock can remain stuck until some later chain lock happens to promote another record.

_{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/changeset/core_bridge.rs`:
- [BLOCKING] lines 198-224: `last_applied_chain_lock` is not persisted when a chain lock advances metadata without promoting records
  The new proof fallback in `wallet/asset_lock/sync/proof.rs` builds a ChainLock proof from persisted `metadata.last_applied_chain_lock` when a restarted wallet reloads an `InBlock` record after the promotion event already passed. This bridge only persists `last_applied_chain_lock` from `WalletEvent::TransactionsChainlocked`, and the code comment confirms upstream only emits that event when `per_account` is non-empty. In the no-promotion case, the wallet's in-memory metadata advances but nothing is persisted, so after a restart the fallback still sees stale or missing chain-lock metadata and the asset lock can remain stuck until some later chain lock happens to promote another record.

[shumkov]

Fixed end-to-end in dfb8caeff4 plus upstream dashpay/rust-dashcore#769. Upstream now emits a single WalletEvent::ChainLockProcessed { chain_lock, locked_transactions } every time last_applied_chain_lock advances, regardless of whether any record was promoted — closing the "metadata-advanced but per-account empty" gap. Platform's core_bridge.rs consumes the renamed event and projects the chainlock into CoreChangeSet::last_applied_chain_lock unconditionally. Same commit folds in two related upstream API changes (the new ApplyChainLockOutcome return type and DerivedAddress::public_key: PublicKey).

[thepastaclaw]

🔴 Blocking: Successful registration/top-up drops the consumed asset-lock changeset instead of persisting it

consume_asset_lock now returns an AssetLockChangeSet whose documented purpose is to upsert the persisted row with status = Consumed while removing the in-memory entry. Both success paths ignore that returned changeset and only log errors, so Swift never receives the Consumed upsert. The result is that a successful registration or top-up leaves persistence at the prior status (Built/Broadcast/InstantSendLocked/ChainLocked), which breaks the historical lookup behavior described in tracking.rs and allows already-spent locks to come back on reload because only persisted Consumed rows are filtered 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/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] lines 567-579: Successful registration/top-up drops the consumed asset-lock changeset instead of persisting it
  `consume_asset_lock` now returns an `AssetLockChangeSet` whose documented purpose is to upsert the persisted row with `status = Consumed` while removing the in-memory entry. Both success paths ignore that returned changeset and only log errors, so Swift never receives the `Consumed` upsert. The result is that a successful registration or top-up leaves persistence at the prior status (`Built`/`Broadcast`/`InstantSendLocked`/`ChainLocked`), which breaks the historical lookup behavior described in `tracking.rs` and allows already-spent locks to come back on reload because only persisted `Consumed` rows are filtered out.

[shumkov]

Fixed in 8a8545c945. consume_asset_lock now queues the Consumed changeset internally (write lock released before queueing). Doc-comment explains why this method queues internally while sibling mutators defer to in-module callers (queue_asset_lock_changeset is pub(super)-scoped; registration lives outside the asset_lock module). Test gap surfaced in the commit message — full AssetLockManager scaffolding doesn't exist yet; end-to-end registration flow exercises this path.

[thepastaclaw]

🟡 Suggestion: Restore path hard-codes unresolved asset-lock transactions to BIP44 account 0

The restore payload now includes account_index, and the Rust loader uses that field to reinsert each unresolved funding transaction into standard_bip44_accounts[account_index]. Swift always serializes entry.account_index = 0, and the inline comment acknowledges that the wrong value causes the Rust side to drop the record. The SDK surface already allows asset locks to be funded from arbitrary BIP44 account indexes, so restarting a wallet with unresolved asset locks from a non-zero funding account will route those transactions to the wrong account or drop them outright, breaking the intended restart recovery path for those wallets.

_{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 2955-2965: Restore path hard-codes unresolved asset-lock transactions to BIP44 account 0
  The restore payload now includes `account_index`, and the Rust loader uses that field to reinsert each unresolved funding transaction into `standard_bip44_accounts[account_index]`. Swift always serializes `entry.account_index = 0`, and the inline comment acknowledges that the wrong value causes the Rust side to drop the record. The SDK surface already allows asset locks to be funded from arbitrary BIP44 account indexes, so restarting a wallet with unresolved asset locks from a non-zero funding account will route those transactions to the wrong account or drop them outright, breaking the intended restart recovery path for those wallets.

[thepastaclaw]

Code Review

The PR’s retry and recovery work is directionally correct, but six verified regressions remain in the new asset-lock and Swift FFI paths. Four of them break resumable asset-lock flows, one drops key metadata at the funded-registration boundary, and one introduces an unsafe Swift-to-Rust string lifetime bug.

Reviewed commit: 259d78a

🔴 6 blocking

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] lines 496-506: `UseAssetLock` retries always fail after Platform rejects an Instant proof
  `register_identity_with_funding` now retries every `InvalidInstantAssetLockProofSignatureError` by calling `self.asset_locks.upgrade_to_chain_lock_proof(&out_point, ...)`, and the top-up path does the same at lines 691-700. That helper only works for entries present in `tracked_asset_locks`; it returns `Asset lock ... is not tracked` otherwise. `IdentityFunding::UseAssetLock` explicitly sets `tracked_out_point: None` because the caller owns the lock lifecycle, so an externally supplied Instant proof can no longer survive a stale-IS rejection from Platform. Instead of surfacing the original SDK error, the wallet forces an internal retry path that cannot succeed for this funding variant.
- [BLOCKING] lines 567-578: Successful identity operations drop the `Consumed` asset-lock changeset
  `consume_asset_lock()` no longer persists by itself. It removes the in-memory entry and returns an `AssetLockChangeSet` whose `asset_locks` payload is the only thing that marks the persisted row as `Consumed`. Both success paths ignore that return value here and in the top-up branch at lines 736-747, so the asset lock disappears from memory but never transitions to `statusRaw = 4` in storage. After a restart, the same lock is reloaded as still actionable, which breaks the crash-recovery semantics this PR is adding.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] lines 360-365: Re-entering catch-up can destroy FFI handles that detached tasks are still using
  `catchUpStuckAssetLocks` starts detached tasks that call `asset_lock_manager_catch_up_blocking(..., 300)`, then a later invocation immediately runs `retainedAssetLockManagers.removeAll(...)`. Dropping those wrappers calls `asset_lock_manager_destroy` in `ManagedAssetLockManager.deinit`, even though the previous batch may still be blocked inside Rust. The method is public and documented for app-foreground and reconnect retries, so a second call within the five-minute window can invalidate live handles and abort the earlier catch-up batch with the `errorInvalidHandle` path that the code already logs as a programmer error.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] lines 2860-2865: Asset-lock restore rewrites every funding account index to zero
  The persisted restore buffer hardcodes `entry.account_index = 0` for tracked asset locks here and again for unresolved funding transactions at lines 2956-2965. That is not just a breadcrumb: Rust uses `account_index` to look up the source BIP44 account during recovery, proof waiting, and ChainLock promotion. The Swift SDK’s public `buildTransaction`, `createFundedProof`, and `sendToAddresses` APIs all accept nonzero account indices, so any client that uses them will have its resumable asset-lock state silently rehydrated under the wrong account after restart.

In `packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs`:
- [BLOCKING] lines 86-99: Funded identity-registration FFI drops `contract_bounds` from Swift pubkeys
  This decoder builds `IdentityPublicKeyV0` with `contract_bounds: None` for every row, even though the Swift caller marshals `contract_bounds_kind`, `contract_bounds_id`, and `contract_bounds_document_type` for each key. The older signer-based registration path already decodes those fields through `decode_contract_bounds`, but the new asset-lock-funded path does not. Encryption and decryption keys therefore lose their declared bounds at the FFI boundary and can be registered with different semantics from what Swift sent.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [BLOCKING] lines 108-131: `sendToAddresses` passes Rust C-string pointers backed by temporary Objective-C objects
  `sendToAddresses` builds `let cStrings = recipients.map { ($0.address as NSString).utf8String }` and then hands those pointers to Rust, where `core_wallet_send_to_addresses` immediately dereferences them with `CStr::from_ptr`. The array retains only raw pointers, not the bridged `NSString` instances that own the UTF-8 buffers, so the Rust side can read freed or corrupted memory if those temporaries are released before or during the call. This is an unsafe Swift-to-C lifetime bug on a transaction-building path.

[thepastaclaw]

🔴 Blocking: UseAssetLock retries always fail after Platform rejects an Instant proof

register_identity_with_funding now retries every InvalidInstantAssetLockProofSignatureError by calling self.asset_locks.upgrade_to_chain_lock_proof(&out_point, ...), and the top-up path does the same at lines 691-700. That helper only works for entries present in tracked_asset_locks; it returns Asset lock ... is not tracked otherwise. IdentityFunding::UseAssetLock explicitly sets tracked_out_point: None because the caller owns the lock lifecycle, so an externally supplied Instant proof can no longer survive a stale-IS rejection from Platform. Instead of surfacing the original SDK error, the wallet forces an internal retry path that cannot succeed for this funding variant.

_{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 496-506: `UseAssetLock` retries always fail after Platform rejects an Instant proof
  `register_identity_with_funding` now retries every `InvalidInstantAssetLockProofSignatureError` by calling `self.asset_locks.upgrade_to_chain_lock_proof(&out_point, ...)`, and the top-up path does the same at lines 691-700. That helper only works for entries present in `tracked_asset_locks`; it returns `Asset lock ... is not tracked` otherwise. `IdentityFunding::UseAssetLock` explicitly sets `tracked_out_point: None` because the caller owns the lock lifecycle, so an externally supplied Instant proof can no longer survive a stale-IS rejection from Platform. Instead of surfacing the original SDK error, the wallet forces an internal retry path that cannot succeed for this funding variant.

[thepastaclaw]

🔴 Blocking: Successful identity operations drop the Consumed asset-lock changeset

consume_asset_lock() no longer persists by itself. It removes the in-memory entry and returns an AssetLockChangeSet whose asset_locks payload is the only thing that marks the persisted row as Consumed. Both success paths ignore that return value here and in the top-up branch at lines 736-747, so the asset lock disappears from memory but never transitions to statusRaw = 4 in storage. After a restart, the same lock is reloaded as still actionable, which breaks the crash-recovery semantics this PR is adding.

_{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 567-578: Successful identity operations drop the `Consumed` asset-lock changeset
  `consume_asset_lock()` no longer persists by itself. It removes the in-memory entry and returns an `AssetLockChangeSet` whose `asset_locks` payload is the only thing that marks the persisted row as `Consumed`. Both success paths ignore that return value here and in the top-up branch at lines 736-747, so the asset lock disappears from memory but never transitions to `statusRaw = 4` in storage. After a restart, the same lock is reloaded as still actionable, which breaks the crash-recovery semantics this PR is adding.

[thepastaclaw]

🔴 Blocking: Re-entering catch-up can destroy FFI handles that detached tasks are still using

catchUpStuckAssetLocks starts detached tasks that call asset_lock_manager_catch_up_blocking(..., 300), then a later invocation immediately runs retainedAssetLockManagers.removeAll(...). Dropping those wrappers calls asset_lock_manager_destroy in ManagedAssetLockManager.deinit, even though the previous batch may still be blocked inside Rust. The method is public and documented for app-foreground and reconnect retries, so a second call within the five-minute window can invalidate live handles and abort the earlier catch-up batch with the errorInvalidHandle path that the code already logs as a programmer error.

_{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-365: Re-entering catch-up can destroy FFI handles that detached tasks are still using
  `catchUpStuckAssetLocks` starts detached tasks that call `asset_lock_manager_catch_up_blocking(..., 300)`, then a later invocation immediately runs `retainedAssetLockManagers.removeAll(...)`. Dropping those wrappers calls `asset_lock_manager_destroy` in `ManagedAssetLockManager.deinit`, even though the previous batch may still be blocked inside Rust. The method is public and documented for app-foreground and reconnect retries, so a second call within the five-minute window can invalidate live handles and abort the earlier catch-up batch with the `errorInvalidHandle` path that the code already logs as a programmer error.

[thepastaclaw]

🔴 Blocking: Asset-lock restore rewrites every funding account index to zero

The persisted restore buffer hardcodes entry.account_index = 0 for tracked asset locks here and again for unresolved funding transactions at lines 2956-2965. That is not just a breadcrumb: Rust uses account_index to look up the source BIP44 account during recovery, proof waiting, and ChainLock promotion. The Swift SDK’s public buildTransaction, createFundedProof, and sendToAddresses APIs all accept nonzero account indices, so any client that uses them will have its resumable asset-lock state silently rehydrated under the wrong account 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`:
- [BLOCKING] lines 2860-2865: Asset-lock restore rewrites every funding account index to zero
  The persisted restore buffer hardcodes `entry.account_index = 0` for tracked asset locks here and again for unresolved funding transactions at lines 2956-2965. That is not just a breadcrumb: Rust uses `account_index` to look up the source BIP44 account during recovery, proof waiting, and ChainLock promotion. The Swift SDK’s public `buildTransaction`, `createFundedProof`, and `sendToAddresses` APIs all accept nonzero account indices, so any client that uses them will have its resumable asset-lock state silently rehydrated under the wrong account after restart.

[thepastaclaw]

🔴 Blocking: Funded identity-registration FFI drops contract_bounds from Swift pubkeys

This decoder builds IdentityPublicKeyV0 with contract_bounds: None for every row, even though the Swift caller marshals contract_bounds_kind, contract_bounds_id, and contract_bounds_document_type for each key. The older signer-based registration path already decodes those fields through decode_contract_bounds, but the new asset-lock-funded path does not. Encryption and decryption keys therefore lose their declared bounds at the FFI boundary and can be registered with different semantics from what Swift sent.

💡 Suggested change

Suggested change

	let pubkey_bytes: Vec<u8> =
	slice::from_raw_parts(row.pubkey_bytes, row.pubkey_len).to_vec();
	let contract_bounds =
	crate::identity_registration_with_signer::decode_contract_bounds(
	row,
	purpose,
	i,
	"identity_pubkeys",
	)?;
	keys_map.insert(
	row.key_id,
	IdentityPublicKey::V0(IdentityPublicKeyV0 {
	id: row.key_id,
	purpose,
	security_level,
	contract_bounds,
	key_type,
	read_only: row.read_only,
	data: BinaryData::new(pubkey_bytes),
	disabled_at: None,
	}),
	);

_{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/identity_registration_funded_with_signer.rs`:
- [BLOCKING] lines 86-99: Funded identity-registration FFI drops `contract_bounds` from Swift pubkeys
  This decoder builds `IdentityPublicKeyV0` with `contract_bounds: None` for every row, even though the Swift caller marshals `contract_bounds_kind`, `contract_bounds_id`, and `contract_bounds_document_type` for each key. The older signer-based registration path already decodes those fields through `decode_contract_bounds`, but the new asset-lock-funded path does not. Encryption and decryption keys therefore lose their declared bounds at the FFI boundary and can be registered with different semantics from what Swift sent.

[thepastaclaw]

🔴 Blocking: sendToAddresses passes Rust C-string pointers backed by temporary Objective-C objects

sendToAddresses builds let cStrings = recipients.map { ($0.address as NSString).utf8String } and then hands those pointers to Rust, where core_wallet_send_to_addresses immediately dereferences them with CStr::from_ptr. The array retains only raw pointers, not the bridged NSString instances that own the UTF-8 buffers, so the Rust side can read freed or corrupted memory if those temporaries are released before or during the call. This is an unsafe Swift-to-C lifetime bug on a transaction-building path.

💡 Suggested change

Suggested change

	let cStrings = recipients.map { ($0.address as NSString).utf8String }
	let amounts = recipients.map { $0.amountDuffs }
	// Resolver-backed signer owns mnemonic access for the lifetime
	// of this call. Each Core ECDSA signature happens atomically
	// inside the resolver vtable (mnemonic fetched, key derived,
	// digest signed, buffers zeroed) — no priv key leaves Swift.
	let resolver = MnemonicResolver()
	try cStrings.withUnsafeBufferPointer { addrBuf in
	try amounts.withUnsafeBufferPointer { amountBuf in
	try core_wallet_send_to_addresses(
	handle,
	accountType.rawValue,
	accountIndex,
	addrBuf.baseAddress,
	amountBuf.baseAddress,
	UInt(recipients.count),
	&txBytesPtr,
	&txLen
	).check()
	try withExtendedLifetime(resolver) {
	try core_wallet_send_to_addresses(
	handle,
	accountType.rawValue,
	accountIndex,
	addrBuf.baseAddress,
	amountBuf.baseAddress,
	UInt(recipients.count),
	resolver.handle,
	&txBytesPtr,
	&txLen
	).check()
	let addressStorage: [UnsafeMutablePointer<CChar>?] = recipients.map { strdup($0.address) }
	defer { addressStorage.forEach { if let p = $0 { free(p) } } }
	let amounts = recipients.map { $0.amountDuffs }
	try addressStorage.withUnsafeBufferPointer { addrBuf in
	let addressesPtr: UnsafePointer<UnsafePointer<CChar>?>? = addrBuf.baseAddress.map {
	UnsafeRawPointer($0).assumingMemoryBound(to: UnsafePointer<CChar>?.self)
	}
	try amounts.withUnsafeBufferPointer { amountBuf in
	try withExtendedLifetime(resolver) {
	try core_wallet_send_to_addresses(
	handle,
	accountType.rawValue,
	accountIndex,
	addressesPtr,
	amountBuf.baseAddress,
	UInt(recipients.count),
	resolver.handle,
	&txBytesPtr,
	&txLen
	).check()
	}
	}
	}

_{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/CoreWallet/ManagedCoreWallet.swift`:
- [BLOCKING] lines 108-131: `sendToAddresses` passes Rust C-string pointers backed by temporary Objective-C objects
  `sendToAddresses` builds `let cStrings = recipients.map { ($0.address as NSString).utf8String }` and then hands those pointers to Rust, where `core_wallet_send_to_addresses` immediately dereferences them with `CStr::from_ptr`. The array retains only raw pointers, not the bridged `NSString` instances that own the UTF-8 buffers, so the Rust side can read freed or corrupted memory if those temporaries are released before or during the call. This is an unsafe Swift-to-C lifetime bug on a transaction-building path.

[thepastaclaw]

Code Review

Re-review at 9a82020. Multiple prior blockers (catch-up handle retention, SecretKey wipe, Consumed-status persistence, last_applied_chain_lock advance, missed-wakeup race, user_fee_increase threading) are resolved at HEAD. Two real blocking bugs remain: the funded-with-signer FFI silently drops contract_bounds from Swift pubkeys (sibling path decodes them), and the post-submission IS→CL upgrade is unreachable for IdentityFunding::UseAssetLock because upgrade_to_chain_lock_proof requires the outpoint to be in tracked_asset_locks. Two lower-severity FFI-boundary issues persist (sendToAddresses C-string lifetime, asset-lock restore hardcoding account_index=0).

Reviewed commit: 9a82020

🔴 1 blocking | 🟡 2 suggestion(s)

🤖 Prompt for all review comments with AI agents

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

In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [BLOCKING] lines 515-540: IS→CL upgrade after Platform rejection is unreachable for IdentityFunding::UseAssetLock
  On `InvalidInstantAssetLockProofSignatureError`, this branch unconditionally calls `self.asset_locks.upgrade_to_chain_lock_proof(&proof_out_point, CL_FALLBACK_TIMEOUT)`. That helper short-circuits at `packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:168-176` with `PlatformWalletError::AssetLockProofWait("Asset lock {} is not tracked")` whenever the outpoint is absent from `tracked_asset_locks`. The `IdentityFunding::UseAssetLock` arm at line 339 deliberately sets `tracked_out_point: None` (caller owns the lock lifecycle) and never inserts into `tracked_asset_locks`. Net effect: when a caller supplies an externally-managed IS proof that Platform rejects (e.g. quorum rotation), the wallet swallows the original SDK error and surfaces a misleading "is not tracked" failure — the very stale-IS case this fallback was added to handle. The same bug is mirrored in `top_up_identity_with_funding` at lines 710-731. Fix: gate the IS→CL upgrade on `tracked_out_point.is_some()` and re-raise the original `PlatformWalletError::Sdk(e)` for the `UseAssetLock` case, or extend `upgrade_to_chain_lock_proof` to accept an explicit account index for the caller-managed path.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] lines 2855-2965: Asset-lock restore buffers hardcode account_index = 0 even though the Rust loader uses it as a lookup key
  Both `buildAssetLockRestoreBuffer` (line 2865) and `buildUnresolvedAssetLockTxRecordBuffer` (line 2965) serialize `entry.account_index = 0`. The inline comment at line 2862-2864 ("The Rust load path doesn't read this field for anything load-bearing") is incorrect: in `packages/rs-platform-wallet-ffi/src/persistence.rs:2324-2336`, `spec.account_index` is stored into `TrackedAssetLock.account_index` AND used as the key in the per-account `BTreeMap`. Likewise `restore_unresolved_asset_lock_tx_records` at `persistence.rs:2739` reinserts funding transactions into `standard_bip44_accounts.get_mut(&rec.account_index)` and drops the row when no matching account exists. The public Swift asset-lock APIs (`buildTransaction`, `createFundedProof`, `sendToAddresses`) all accept arbitrary `accountIndex`, so any wallet funded from a nonzero BIP44 account silently loses crash-recovery for in-flight asset locks at restart. Today's iOS app happens to use account 0 only, which keeps this latent — but `PersistentAssetLock` needs a real `accountIndex` column (mirroring `identityIndexRaw`) before nonzero BIP44 accounts are supported.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [SUGGESTION] lines 108-134: sendToAddresses passes C-string pointers backed by bridged-NSString temporaries
  `let cStrings = recipients.map { ($0.address as NSString).utf8String }` produces an array of `UnsafePointer<CChar>?` whose backing UTF-8 buffers are owned by autoreleased `NSString` temporaries created inside the closure — the array retains the pointers, not the NSString owners. Rust immediately dereferences each with `CStr::from_ptr` in `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`. In practice within a single synchronous non-`@autoreleasepool` function the temporaries live until function exit (no pool drain between `map` and the FFI call), so no concrete misbehavior is reproducible at this SHA. But this is the documented Swift→C anti-pattern: Apple's NSString docs warn that `utf8String` returns memory "automatically freed just as a returned object would be released," any future refactor introducing an `await`, `withCheckedThrowingContinuation`, or nested `@autoreleasepool` between buffer construction and the FFI call invalidates the pointers, and every other FFI buffer in this file already uses owned `strdup`/`free` allocations. Worth tightening because this is a transaction-signing path.

[thepastaclaw]

🔴 Blocking: IS→CL upgrade after Platform rejection is unreachable for IdentityFunding::UseAssetLock

On InvalidInstantAssetLockProofSignatureError, this branch unconditionally calls self.asset_locks.upgrade_to_chain_lock_proof(&proof_out_point, CL_FALLBACK_TIMEOUT). That helper short-circuits at packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:168-176 with PlatformWalletError::AssetLockProofWait("Asset lock {} is not tracked") whenever the outpoint is absent from tracked_asset_locks. The IdentityFunding::UseAssetLock arm at line 339 deliberately sets tracked_out_point: None (caller owns the lock lifecycle) and never inserts into tracked_asset_locks. Net effect: when a caller supplies an externally-managed IS proof that Platform rejects (e.g. quorum rotation), the wallet swallows the original SDK error and surfaces a misleading "is not tracked" failure — the very stale-IS case this fallback was added to handle. The same bug is mirrored in top_up_identity_with_funding at lines 710-731. Fix: gate the IS→CL upgrade on tracked_out_point.is_some() and re-raise the original PlatformWalletError::Sdk(e) for the UseAssetLock case, or extend upgrade_to_chain_lock_proof to accept an explicit account index for the caller-managed path.

_{source: ['claude', '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 515-540: IS→CL upgrade after Platform rejection is unreachable for IdentityFunding::UseAssetLock
  On `InvalidInstantAssetLockProofSignatureError`, this branch unconditionally calls `self.asset_locks.upgrade_to_chain_lock_proof(&proof_out_point, CL_FALLBACK_TIMEOUT)`. That helper short-circuits at `packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs:168-176` with `PlatformWalletError::AssetLockProofWait("Asset lock {} is not tracked")` whenever the outpoint is absent from `tracked_asset_locks`. The `IdentityFunding::UseAssetLock` arm at line 339 deliberately sets `tracked_out_point: None` (caller owns the lock lifecycle) and never inserts into `tracked_asset_locks`. Net effect: when a caller supplies an externally-managed IS proof that Platform rejects (e.g. quorum rotation), the wallet swallows the original SDK error and surfaces a misleading "is not tracked" failure — the very stale-IS case this fallback was added to handle. The same bug is mirrored in `top_up_identity_with_funding` at lines 710-731. Fix: gate the IS→CL upgrade on `tracked_out_point.is_some()` and re-raise the original `PlatformWalletError::Sdk(e)` for the `UseAssetLock` case, or extend `upgrade_to_chain_lock_proof` to accept an explicit account index for the caller-managed path.

[thepastaclaw]

🟡 Suggestion: Asset-lock restore buffers hardcode account_index = 0 even though the Rust loader uses it as a lookup key

Both buildAssetLockRestoreBuffer (line 2865) and buildUnresolvedAssetLockTxRecordBuffer (line 2965) serialize entry.account_index = 0. The inline comment at line 2862-2864 ("The Rust load path doesn't read this field for anything load-bearing") is incorrect: in packages/rs-platform-wallet-ffi/src/persistence.rs:2324-2336, spec.account_index is stored into TrackedAssetLock.account_index AND used as the key in the per-account BTreeMap. Likewise restore_unresolved_asset_lock_tx_records at persistence.rs:2739 reinserts funding transactions into standard_bip44_accounts.get_mut(&rec.account_index) and drops the row when no matching account exists. The public Swift asset-lock APIs (buildTransaction, createFundedProof, sendToAddresses) all accept arbitrary accountIndex, so any wallet funded from a nonzero BIP44 account silently loses crash-recovery for in-flight asset locks at restart. Today's iOS app happens to use account 0 only, which keeps this latent — but PersistentAssetLock needs a real accountIndex column (mirroring identityIndexRaw) before nonzero BIP44 accounts are supported.

_{source: ['claude', '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 2855-2965: Asset-lock restore buffers hardcode account_index = 0 even though the Rust loader uses it as a lookup key
  Both `buildAssetLockRestoreBuffer` (line 2865) and `buildUnresolvedAssetLockTxRecordBuffer` (line 2965) serialize `entry.account_index = 0`. The inline comment at line 2862-2864 ("The Rust load path doesn't read this field for anything load-bearing") is incorrect: in `packages/rs-platform-wallet-ffi/src/persistence.rs:2324-2336`, `spec.account_index` is stored into `TrackedAssetLock.account_index` AND used as the key in the per-account `BTreeMap`. Likewise `restore_unresolved_asset_lock_tx_records` at `persistence.rs:2739` reinserts funding transactions into `standard_bip44_accounts.get_mut(&rec.account_index)` and drops the row when no matching account exists. The public Swift asset-lock APIs (`buildTransaction`, `createFundedProof`, `sendToAddresses`) all accept arbitrary `accountIndex`, so any wallet funded from a nonzero BIP44 account silently loses crash-recovery for in-flight asset locks at restart. Today's iOS app happens to use account 0 only, which keeps this latent — but `PersistentAssetLock` needs a real `accountIndex` column (mirroring `identityIndexRaw`) before nonzero BIP44 accounts are supported.

[thepastaclaw]

🟡 Suggestion: sendToAddresses passes C-string pointers backed by bridged-NSString temporaries

let cStrings = recipients.map { ($0.address as NSString).utf8String } produces an array of UnsafePointer<CChar>? whose backing UTF-8 buffers are owned by autoreleased NSString temporaries created inside the closure — the array retains the pointers, not the NSString owners. Rust immediately dereferences each with CStr::from_ptr in packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs. In practice within a single synchronous non-@autoreleasepool function the temporaries live until function exit (no pool drain between map and the FFI call), so no concrete misbehavior is reproducible at this SHA. But this is the documented Swift→C anti-pattern: Apple's NSString docs warn that utf8String returns memory "automatically freed just as a returned object would be released," any future refactor introducing an await, withCheckedThrowingContinuation, or nested @autoreleasepool between buffer construction and the FFI call invalidates the pointers, and every other FFI buffer in this file already uses owned strdup/free allocations. Worth tightening because this is a transaction-signing path.

💡 Suggested change

Suggested change

	let cStrings = recipients.map { ($0.address as NSString).utf8String }
	let amounts = recipients.map { $0.amountDuffs }
	// Resolver-backed signer owns mnemonic access for the lifetime
	// of this call. Each Core ECDSA signature happens atomically
	// inside the resolver vtable (mnemonic fetched, key derived,
	// digest signed, buffers zeroed) — no priv key leaves Swift.
	let resolver = MnemonicResolver()
	try cStrings.withUnsafeBufferPointer { addrBuf in
	try amounts.withUnsafeBufferPointer { amountBuf in
	try core_wallet_send_to_addresses(
	handle,
	accountType.rawValue,
	accountIndex,
	addrBuf.baseAddress,
	amountBuf.baseAddress,
	UInt(recipients.count),
	&txBytesPtr,
	&txLen
	).check()
	try withExtendedLifetime(resolver) {
	try core_wallet_send_to_addresses(
	handle,
	accountType.rawValue,
	accountIndex,
	addrBuf.baseAddress,
	amountBuf.baseAddress,
	UInt(recipients.count),
	resolver.handle,
	&txBytesPtr,
	&txLen
	).check()
	}
	}
	}
	let addressStorage: [UnsafeMutablePointer<CChar>?] = recipients.map { strdup($0.address) }
	defer { addressStorage.forEach { if let p = $0 { free(p) } } }
	let amounts = recipients.map { $0.amountDuffs }
	try addressStorage.withUnsafeBufferPointer { addrBuf in
	let addressesPtr: UnsafePointer<UnsafePointer<CChar>?>? = addrBuf.baseAddress.map {
	UnsafeRawPointer($0).assumingMemoryBound(to: UnsafePointer<CChar>?.self)
	}
	try amounts.withUnsafeBufferPointer { amountBuf in
	try withExtendedLifetime(resolver) {
	try core_wallet_send_to_addresses(
	handle,
	accountType.rawValue,
	accountIndex,
	addressesPtr,
	amountBuf.baseAddress,
	UInt(recipients.count),
	resolver.handle,
	&txBytesPtr,
	&txLen
	).check()
	}
	}
	}

_{source: ['claude', '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/CoreWallet/ManagedCoreWallet.swift`:
- [SUGGESTION] lines 108-134: sendToAddresses passes C-string pointers backed by bridged-NSString temporaries
  `let cStrings = recipients.map { ($0.address as NSString).utf8String }` produces an array of `UnsafePointer<CChar>?` whose backing UTF-8 buffers are owned by autoreleased `NSString` temporaries created inside the closure — the array retains the pointers, not the NSString owners. Rust immediately dereferences each with `CStr::from_ptr` in `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`. In practice within a single synchronous non-`@autoreleasepool` function the temporaries live until function exit (no pool drain between `map` and the FFI call), so no concrete misbehavior is reproducible at this SHA. But this is the documented Swift→C anti-pattern: Apple's NSString docs warn that `utf8String` returns memory "automatically freed just as a returned object would be released," any future refactor introducing an `await`, `withCheckedThrowingContinuation`, or nested `@autoreleasepool` between buffer construction and the FFI call invalidates the pointers, and every other FFI buffer in this file already uses owned `strdup`/`free` allocations. Worth tightening because this is a transaction-signing path.