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

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

[shumkov]

Fixed in 81acae6726. Added accountIndexRaw: Int32 = 0 column to PersistentAssetLock and plumbed account_index through both the inbound FFI callback (onPersistAssetLocks) and the load-time snapshot. Both restore-marshalling sites (buildAssetLockRestoreBuffer for asset-lock entries, buildUnresolvedAssetLockTxRecords for tx records) now use UInt32(bitPattern: record.accountIndexRaw) instead of hardcoded 0. SwiftData lightweight migration keeps pre-feature rows safe (default 0 = the bytes the hardcoded restore was using = correct for the BIP44-account-0 common case); any subsequent upsert on a pre-feature row populates the column with the real value from AssetLockEntryFFI.account_index.

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

[shumkov]

Fixed in 81acae6726. Added accountIndexRaw: Int32 = 0 column to PersistentAssetLock and plumbed account_index through both the inbound FFI callback (onPersistAssetLocks) and the load-time snapshot. Both restore-marshalling sites (buildAssetLockRestoreBuffer for asset-lock entries, buildUnresolvedAssetLockTxRecords for tx records) now use UInt32(bitPattern: record.accountIndexRaw) instead of hardcoded 0. SwiftData lightweight migration keeps pre-feature rows safe (default 0 = the bytes the hardcoded restore was using = correct for the BIP44-account-0 common case); any subsequent upsert on a pre-feature row populates the column with the real value from AssetLockEntryFFI.account_index.

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

[shumkov]

Removed IdentityFunding::UseAssetLock entirely in 0fb17cce3b. Audit confirmed zero production callers in the workspace — the variant's own docstring acknowledged it was for "callers that manage asset locks via a sibling component (evo-tool's tasks, integration tests, etc.)" and none of those consumers live here.

Both reviewer findings (N1 = the IS-timeout fallback, N2 = the Platform IS-rejection retry) collapse with the variant's removal. The IS→CL machinery requires a tracked lock to drive upgrade_to_chain_lock_proof; UseAssetLock's tracked_out_point: None deliberately bypassed tracking, so the fallback was unreachable by design — exactly what the reviewer described.

Future consumers needing an external proof should register it through AssetLockManager first (creating a tracked entry) and then submit via FromExistingAssetLock. That path has working lifecycle + fallback.

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

[shumkov]

Fixed in da7938a013. The funded-registration decoder now calls the existing decode_contract_bounds helper from identity_registration_with_signer, so the kind / id / document_type triple round-trips correctly and Encryption/Decryption purposes get rejected at the FFI boundary when kind == 0 (instead of silently registering an unusable key).

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

[shumkov]

Fixed in da7938a013. sendToAddresses now owns the C-string storage via strdup + defer free instead of pointing at bridged NSString temporaries. Eliminates the use-after-free Apple's utf8String docs explicitly warn about.

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

[shumkov]

Removed IdentityFunding::UseAssetLock entirely in 0fb17cce3b. Audit confirmed zero production callers in the workspace — the variant's own docstring acknowledged it was for "callers that manage asset locks via a sibling component (evo-tool's tasks, integration tests, etc.)" and none of those consumers live here.

Both reviewer findings (N1 = the IS-timeout fallback, N2 = the Platform IS-rejection retry) collapse with the variant's removal. The IS→CL machinery requires a tracked lock to drive upgrade_to_chain_lock_proof; UseAssetLock's tracked_out_point: None deliberately bypassed tracking, so the fallback was unreachable by design — exactly what the reviewer described.

Future consumers needing an external proof should register it through AssetLockManager first (creating a tracked entry) and then submit via FromExistingAssetLock. That path has working lifecycle + fallback.

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

[shumkov]

Fixed in da7938a013. sendToAddresses now owns the C-string storage via strdup + defer free instead of pointing at bridged NSString temporaries. Eliminates the use-after-free Apple's utf8String docs explicitly warn about.

[codecov]

Codecov Report

❌ Patch coverage is 52.57353% with 129 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.84%. Comparing base (cd93b2f) to head (1e3264d).
⚠️ Report is 8 commits behind head on v3.1-dev.

Files with missing lines	Patch %	Lines
...entity/identity_create_transition/v0/v0_methods.rs	46.15%	28 Missing ⚠️
.../identity/identity_topup_transition/methods/mod.rs	24.32%	28 Missing ⚠️
...identity/identity_create_transition/methods/mod.rs	31.57%	26 Missing ⚠️
...dentity/identity_topup_transition/v0/v0_methods.rs	4.00%	24 Missing ⚠️
packages/rs-dpp/src/state_transition/mod.rs	74.44%	23 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           v3.1-dev    #3634      +/-   ##
============================================
- Coverage     88.06%   87.84%   -0.23%     
============================================
  Files          2521     2537      +16     
  Lines        308995   311316    +2321     
============================================
+ Hits         272122   273472    +1350     
- Misses        36873    37844     +971     

Components	Coverage Δ
dpp	87.83% <46.69%> (-0.18%)	⬇️
drive	87.03% <ø> (-0.03%)	⬇️
drive-abci	90.05% <100.00%> (ø)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.17% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	53.13% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/26052304059
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "6c73d31a78e30c5532b2b6f9adc4af611a03bfa9597576eb0758597b161c7001"
)

Xcode manual integration:

• Download 'DashSDKFFI.xcframework' artifact from the run link above.
• Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
• If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

[thepastaclaw]

Code Review

Verified at 1e3264d. All prior blocking findings remain resolved — contract_bounds decoding on the funded-signer FFI path, removal of the unreachable UseAssetLock variant, Consumed-status persistence via self-queued changeset, accountIndexRaw threading through restore buffers, strdup-owned C-strings in sendToAddresses, per-task ARC retention of the catch-up FFI handle, user_fee_increase threading, and the pinned Notified::enable() pattern for the missed-wakeup race. One real logic gap remains in the Swift Resumable Registrations filter (terminal status=4 Consumed locks leak into the UI after local identity delete, showing a perpetual spinner). Three suggestion-severity items: a duplicate FFI pubkey decoder (already drifted once), a defense-in-depth zeroization gap in derive_priv, and missing test coverage for the new CL-height retry fee-bump loop.

Reviewed commit: 1e3264d

🟡 4 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/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift`:
- [SUGGESTION] line 37: isVisibleAsResumable accepts Consumed (4), leaking terminal locks into Resumable Registrations after a local identity delete
  `isVisibleAsResumable: statusRaw >= 1` has no upper bound, and `IdentitiesContentView.crossWalletResumableLocks` (IdentitiesContentView.swift:304) uses the same `statusRaw >= 1` floor. The new `AssetLockStatus::Consumed = 4` discriminant introduced by this PR is terminal — the inline doc at lines 26–28 even says "Consumed (4) was already used to fund an identity and cannot be reused; the persisted row survives only for historical lookup" — so it should not surface in the resume UI.

In the happy path this is masked by the `(walletId, identityIndex)` anti-join because `PersistentIdentity` claims the slot. But IdentitiesContentView exposes a 'Delete a single identity locally' action (IdentitiesContentView.swift:331+) whose doc-comment explicitly says it does NOT touch the funding `PersistentAssetLock`. After such a delete the slot frees up, the Consumed lock re-enters the cross-wallet list, and the row falls into the `else` branch of `trailingAffordance` (IdentitiesContentView.swift:535–551) because `canFundIdentity` (correctly) rejects status 4 — producing a perpetual `ProgressView` labelled "Waiting for InstantSendLock or ChainLock…" for a lock that can never be resumed. Tapping through would also error out: `resume_asset_lock` rejects Consumed entries with `AssetLockProofWait("Asset lock {} is already Consumed — nothing to resume")`.

The new test `testForwardCompatibleStatusFloor` (CreateIdentityResumableTests.swift:93–100) actively pins the wrong behavior — it asserts `statusRaw=4` surfaces, with a comment treating 4 as a hypothetical future value. Now that 4 IS Consumed, both the filter and the test need updating. Add the same bound to `crossWalletResumableLocks` for consistency.

In `packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs`:
- [SUGGESTION] lines 54-110: Duplicate unsafe IdentityPubkeyFFI decoder — sibling registration path already diverged once on contract_bounds
  `decode_identity_pubkeys()` here reconstructs `IdentityPublicKeyV0` with the same field-by-field logic that already exists in `packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs:349-379`. The earlier `contract_bounds` regression occurred precisely because one decoder evolved while the other did not — both now call `decode_contract_bounds(...)` but the surrounding row→key construction (KeyType/Purpose/SecurityLevel validation, null-buffer check, `IdentityPublicKeyV0` field mapping) is still duplicated across two unsafe FFI entrypoints. Because semantic drift here silently changes the on-chain meaning of Swift-supplied keys, the safer shape is a single shared decoder helper for `IdentityPubkeyFFI` rows that both entrypoints call, so future field additions cannot land in only one path. The error-surfacing styles differ (`Result<_, PlatformWalletFFIResult>` here vs. `unwrap_result_or_return!` there), but that can be normalized to the `Result` form and `unwrap_result_or_return!`-ed at the single remaining call site.

In `packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- [SUGGESTION] lines 277-288: derive_priv leaves BIP-32 ExtendedPrivKey SecretKey scalars unwiped in memory
  `derive_priv` builds `master: ExtendedPrivKey` and `derived: ExtendedPrivKey`, each owning a `secp256k1::SecretKey` containing a 32-byte private scalar. `SecretKey` does NOT zeroize on drop in the `secp256k1` crate — that is exactly why `sign_ecdsa`/`public_key` later call `non_secure_erase()` on their reconstructed `secret` before drop. The module doc-comment (lines 39–44) explicitly promises that "Every intermediate that carries key material … is wrapped in Zeroizing and dropped before the method returns," but neither ExtendedPrivKey is wiped — both go out of scope still holding live scalars. The canonical copy is already `Zeroizing<[u8;32]>` and returned to the caller, so this is defense-in-depth divergence rather than a fresh exposure, but it weakens the documented invariant. Calling `non_secure_erase()` on `derived.private_key` and `master.private_key` before they drop (or shadowing them through a local SecretKey you erase explicitly) brings the implementation back in line with the doc.

In `packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- [SUGGESTION] lines 156-215: submit_with_cl_height_retry has no dedicated test pinning the fee-bump behavior
  `submit_with_cl_height_retry()` is load-bearing for crash-free identity registration/top-up under consensus code 10506, and this PR series already regressed it once (`user_fee_increase` was threaded incorrectly before being fixed). The existing tests only pin the timeout discriminator; there is no deterministic unit test that feeds a stub submit closure returning repeated `InvalidAssetLockProofCoreChainHeightError`s and asserts (a) successive attempts receive incremented `PutSettings::user_fee_increase` (hash-cache avoidance depends on this), and (b) the original error surfaces once `CL_HEIGHT_RETRY_BUDGET` is exhausted. The retry logic is entirely local and time-driven, so a paused-Tokio test would be cheap and would directly protect Tenderdash hash-cache avoidance behavior from a future silent regression.

[thepastaclaw]

🟡 Suggestion: isVisibleAsResumable accepts Consumed (4), leaking terminal locks into Resumable Registrations after a local identity delete

isVisibleAsResumable: statusRaw >= 1 has no upper bound, and IdentitiesContentView.crossWalletResumableLocks (IdentitiesContentView.swift:304) uses the same statusRaw >= 1 floor. The new AssetLockStatus::Consumed = 4 discriminant introduced by this PR is terminal — the inline doc at lines 26–28 even says "Consumed (4) was already used to fund an identity and cannot be reused; the persisted row survives only for historical lookup" — so it should not surface in the resume UI.

In the happy path this is masked by the (walletId, identityIndex) anti-join because PersistentIdentity claims the slot. But IdentitiesContentView exposes a 'Delete a single identity locally' action (IdentitiesContentView.swift:331+) whose doc-comment explicitly says it does NOT touch the funding PersistentAssetLock. After such a delete the slot frees up, the Consumed lock re-enters the cross-wallet list, and the row falls into the else branch of trailingAffordance (IdentitiesContentView.swift:535–551) because canFundIdentity (correctly) rejects status 4 — producing a perpetual ProgressView labelled "Waiting for InstantSendLock or ChainLock…" for a lock that can never be resumed. Tapping through would also error out: resume_asset_lock rejects Consumed entries with AssetLockProofWait("Asset lock {} is already Consumed — nothing to resume").

The new test testForwardCompatibleStatusFloor (CreateIdentityResumableTests.swift:93–100) actively pins the wrong behavior — it asserts statusRaw=4 surfaces, with a comment treating 4 as a hypothetical future value. Now that 4 IS Consumed, both the filter and the test need updating. Add the same bound to crossWalletResumableLocks for consistency.

💡 Suggested change

Suggested change

	var isVisibleAsResumable: Bool { statusRaw >= 1 }
	var isVisibleAsResumable: Bool { statusRaw >= 1 && statusRaw <= 3 }

_{source: ['claude']}

🤖 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/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift`:
- [SUGGESTION] line 37: isVisibleAsResumable accepts Consumed (4), leaking terminal locks into Resumable Registrations after a local identity delete
  `isVisibleAsResumable: statusRaw >= 1` has no upper bound, and `IdentitiesContentView.crossWalletResumableLocks` (IdentitiesContentView.swift:304) uses the same `statusRaw >= 1` floor. The new `AssetLockStatus::Consumed = 4` discriminant introduced by this PR is terminal — the inline doc at lines 26–28 even says "Consumed (4) was already used to fund an identity and cannot be reused; the persisted row survives only for historical lookup" — so it should not surface in the resume UI.

In the happy path this is masked by the `(walletId, identityIndex)` anti-join because `PersistentIdentity` claims the slot. But IdentitiesContentView exposes a 'Delete a single identity locally' action (IdentitiesContentView.swift:331+) whose doc-comment explicitly says it does NOT touch the funding `PersistentAssetLock`. After such a delete the slot frees up, the Consumed lock re-enters the cross-wallet list, and the row falls into the `else` branch of `trailingAffordance` (IdentitiesContentView.swift:535–551) because `canFundIdentity` (correctly) rejects status 4 — producing a perpetual `ProgressView` labelled "Waiting for InstantSendLock or ChainLock…" for a lock that can never be resumed. Tapping through would also error out: `resume_asset_lock` rejects Consumed entries with `AssetLockProofWait("Asset lock {} is already Consumed — nothing to resume")`.

The new test `testForwardCompatibleStatusFloor` (CreateIdentityResumableTests.swift:93–100) actively pins the wrong behavior — it asserts `statusRaw=4` surfaces, with a comment treating 4 as a hypothetical future value. Now that 4 IS Consumed, both the filter and the test need updating. Add the same bound to `crossWalletResumableLocks` for consistency.

[thepastaclaw]

🟡 Suggestion: Duplicate unsafe IdentityPubkeyFFI decoder — sibling registration path already diverged once on contract_bounds

decode_identity_pubkeys() here reconstructs IdentityPublicKeyV0 with the same field-by-field logic that already exists in packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs:349-379. The earlier contract_bounds regression occurred precisely because one decoder evolved while the other did not — both now call decode_contract_bounds(...) but the surrounding row→key construction (KeyType/Purpose/SecurityLevel validation, null-buffer check, IdentityPublicKeyV0 field mapping) is still duplicated across two unsafe FFI entrypoints. Because semantic drift here silently changes the on-chain meaning of Swift-supplied keys, the safer shape is a single shared decoder helper for IdentityPubkeyFFI rows that both entrypoints call, so future field additions cannot land in only one path. The error-surfacing styles differ (Result<_, PlatformWalletFFIResult> here vs. unwrap_result_or_return! there), but that can be normalized to the Result form and unwrap_result_or_return!-ed at the single remaining call site.

_{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`:
- [SUGGESTION] lines 54-110: Duplicate unsafe IdentityPubkeyFFI decoder — sibling registration path already diverged once on contract_bounds
  `decode_identity_pubkeys()` here reconstructs `IdentityPublicKeyV0` with the same field-by-field logic that already exists in `packages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rs:349-379`. The earlier `contract_bounds` regression occurred precisely because one decoder evolved while the other did not — both now call `decode_contract_bounds(...)` but the surrounding row→key construction (KeyType/Purpose/SecurityLevel validation, null-buffer check, `IdentityPublicKeyV0` field mapping) is still duplicated across two unsafe FFI entrypoints. Because semantic drift here silently changes the on-chain meaning of Swift-supplied keys, the safer shape is a single shared decoder helper for `IdentityPubkeyFFI` rows that both entrypoints call, so future field additions cannot land in only one path. The error-surfacing styles differ (`Result<_, PlatformWalletFFIResult>` here vs. `unwrap_result_or_return!` there), but that can be normalized to the `Result` form and `unwrap_result_or_return!`-ed at the single remaining call site.

[thepastaclaw]

🟡 Suggestion: derive_priv leaves BIP-32 ExtendedPrivKey SecretKey scalars unwiped in memory

derive_priv builds master: ExtendedPrivKey and derived: ExtendedPrivKey, each owning a secp256k1::SecretKey containing a 32-byte private scalar. SecretKey does NOT zeroize on drop in the secp256k1 crate — that is exactly why sign_ecdsa/public_key later call non_secure_erase() on their reconstructed secret before drop. The module doc-comment (lines 39–44) explicitly promises that "Every intermediate that carries key material … is wrapped in Zeroizing and dropped before the method returns," but neither ExtendedPrivKey is wiped — both go out of scope still holding live scalars. The canonical copy is already Zeroizing<[u8;32]> and returned to the caller, so this is defense-in-depth divergence rather than a fresh exposure, but it weakens the documented invariant. Calling non_secure_erase() on derived.private_key and master.private_key before they drop (or shadowing them through a local SecretKey you erase explicitly) brings the implementation back in line with the doc.

_{source: ['claude']}

🤖 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-sdk-ffi/src/mnemonic_resolver_core_signer.rs`:
- [SUGGESTION] lines 277-288: derive_priv leaves BIP-32 ExtendedPrivKey SecretKey scalars unwiped in memory
  `derive_priv` builds `master: ExtendedPrivKey` and `derived: ExtendedPrivKey`, each owning a `secp256k1::SecretKey` containing a 32-byte private scalar. `SecretKey` does NOT zeroize on drop in the `secp256k1` crate — that is exactly why `sign_ecdsa`/`public_key` later call `non_secure_erase()` on their reconstructed `secret` before drop. The module doc-comment (lines 39–44) explicitly promises that "Every intermediate that carries key material … is wrapped in Zeroizing and dropped before the method returns," but neither ExtendedPrivKey is wiped — both go out of scope still holding live scalars. The canonical copy is already `Zeroizing<[u8;32]>` and returned to the caller, so this is defense-in-depth divergence rather than a fresh exposure, but it weakens the documented invariant. Calling `non_secure_erase()` on `derived.private_key` and `master.private_key` before they drop (or shadowing them through a local SecretKey you erase explicitly) brings the implementation back in line with the doc.

[thepastaclaw]

🟡 Suggestion: submit_with_cl_height_retry has no dedicated test pinning the fee-bump behavior

submit_with_cl_height_retry() is load-bearing for crash-free identity registration/top-up under consensus code 10506, and this PR series already regressed it once (user_fee_increase was threaded incorrectly before being fixed). The existing tests only pin the timeout discriminator; there is no deterministic unit test that feeds a stub submit closure returning repeated InvalidAssetLockProofCoreChainHeightErrors and asserts (a) successive attempts receive incremented PutSettings::user_fee_increase (hash-cache avoidance depends on this), and (b) the original error surfaces once CL_HEIGHT_RETRY_BUDGET is exhausted. The retry logic is entirely local and time-driven, so a paused-Tokio test would be cheap and would directly protect Tenderdash hash-cache avoidance behavior from a future silent regression.

_{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`:
- [SUGGESTION] lines 156-215: submit_with_cl_height_retry has no dedicated test pinning the fee-bump behavior
  `submit_with_cl_height_retry()` is load-bearing for crash-free identity registration/top-up under consensus code 10506, and this PR series already regressed it once (`user_fee_increase` was threaded incorrectly before being fixed). The existing tests only pin the timeout discriminator; there is no deterministic unit test that feeds a stub submit closure returning repeated `InvalidAssetLockProofCoreChainHeightError`s and asserts (a) successive attempts receive incremented `PutSettings::user_fee_increase` (hash-cache avoidance depends on this), and (b) the original error surfaces once `CL_HEIGHT_RETRY_BUDGET` is exhausted. The retry logic is entirely local and time-driven, so a paused-Tokio test would be cheap and would directly protect Tenderdash hash-cache avoidance behavior from a future silent regression.