feat: platform-address funding from asset-lock proofs

[shumkov]

Issue being fixed or feature implemented

Follow-up to #3634 (Core-funded identity registration via asset-lock proofs). That PR shipped the identity-create side of the asset-lock-funded pipeline end-to-end through Swift. This PR ships the platform-address funding side: build asset-lock tx from a Core (SPV) balance → wait for IS-lock (or fall back to ChainLock) → submit an AddressFundingFromAssetLockTransition → consume the lock on success. No asset-lock private key crosses the FFI boundary; both Core-side derivation and the outer ST signature route through an external key_wallet::signer::Signer.

AddressFundingFromAssetLockTransition already existed at the DPP/SDK/wallet layers but only on a raw-private-key path. Nothing was wired end-to-end through the wallet orchestrator, FFI, Swift SDK, or app UI.

What was done?

End-to-end Core→Platform address top-up, plus a UX layer matching identity-registration's parity, plus reviewer-driven hardening.

Pipeline. New PlatformAddressWallet::top_up<S, AS> covers build → broadcast → wait-IS-or-CL → submit-with-CL-retry → IS→CL-fallback → consume. The CL-height retry helper and IS-timeout-aware funding resolver were lifted out of wallet/identity/network/registration.rs into a shared wallet/asset_lock/orchestration.rs so identity-register, identity-top-up, and address-top-up all share one source of truth for retry budgets and timeouts. IdentityFunding enum became AssetLockFunding in the same move.

Signer pair. New try_from_asset_lock_with_signers<S, AS> on AddressFundingFromAssetLockTransition mirrors the identity-create signers pattern from #3634. try_from_asset_lock_with_signer was renamed _with_signer_and_private_key for parity with that rename (4 in-tree callers updated). New TopUpAddress::top_up_with_signers on the SDK trait threads settings.user_fee_increase end-to-end — load-bearing for the CL-height retry path (silent drop would let retries hash identically and hit Tenderdash's 24h invalid-tx cache).

FFI + Swift. platform_address_wallet_top_up_signer + _resume_top_up_with_existing_asset_lock_signer mirror the identity-side FFI shape. Swift wrappers (topUpFromCore, resumeTopUpFromAssetLock) follow the swift-sdk CLAUDE.md "thin marshaller" rule — internal MnemonicResolver() pinned via withExtendedLifetime((signer, coreSigner)).

Example app UX. Top-up entry via a + button on the Platform Balance row. The flow drives a 5-step live progress view (Building → Broadcasting → Waiting for IS → Waiting for CL → Funding platform address) backed by an AddressTopUpController + AddressTopUpCoordinator mirroring the identity-side single-flight + retention-sweep pattern. A "Pending Platform Top Ups" card on the wallet-detail screen surfaces both in-flight controllers and orphaned PersistentAssetLock rows for crash-recovery resume. Storage Explorer shows the recipient platform address on consumed top-up locks (back-filled by Swift on .completed since Rust doesn't know the recipient — it's chosen at ST-submit time).

Reviewer findings. Six review agents ran in parallel after the initial pipeline landed. Notable items addressed: silent-zero / index-0 fallbacks in write_address_balances_changeset now tracing::error! and skip; recipient-hash back-fill uses a pre-submit outpoint snapshot (deterministic, not racy newest-updatedAt); recipient type plumbed through the controller (was hardcoded P2PKH); bech32m HRP picker now reads the wallet's network (was hardcoded tdash); standalone progress view gained a Dismiss button on .failed; FFI decode_funding_addresses rejects duplicates; Core funding-account picker filters to confirmed > 0 and canSubmit gates on balance >= amount. Latency budget + cancellation contract + retry scope documented at the orchestration layer.

How Has This Been Tested?

• cargo check --workspace clean; cargo clippy --all-targets -- -D warnings clean.
• cargo test -p platform-wallet --lib — 124/124 green (includes the lifted submit_with_cl_height_retry regression pin).
• cargo test -p dpp --lib state_transition::state_transitions::address_funds — 90/90 green.
• iOS simulator build green end-to-end.
• End-to-end testnet validation pending. The build is green but no fund-from-Core platform-address top-up has been exercised live yet. Will follow up once the testnet run lands.

Breaking Changes

• DPP — AddressFundingFromAssetLockTransitionMethodsV0::try_from_asset_lock_with_signer → try_from_asset_lock_with_signer_and_private_key. New try_from_asset_lock_with_signers<S, AS> sibling for the external-signer shape.
• IdentityFunding enum moved to platform_wallet::AssetLockFunding. The old alias was removed — external consumers should follow the rename.
• PlatformAddressWallet::new gains an asset_locks: Arc<AssetLockManager<SpvBroadcaster>> parameter. Only in-tree caller is PlatformWallet::new.
• PersistentAssetLock gains two optional SwiftData fields (recipientPlatformAddressHash, recipientPlatformAddressType) — lightweight migration; existing rows read nil.
• Legacy platform_address_wallet_fund_from_asset_lock FFI and its wallet counterpart removed. The orchestrated top_up_signer entry point covers the same use case via AssetLockFunding::FromExistingAssetLock.

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

[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

Walkthrough

This PR enables funding Platform (P2PKH) addresses directly from Core Layer 1 asset locks using external signers, eliminating private-key exposure over FFI. It extends DPP state transitions with explicit signer constructors, introduces shared orchestration for IS→CL fallback and CL-height retries, refactors identity funding to reuse the orchestration layer, and adds comprehensive Swift SDK and example-app UI for address-funding flows with progress tracking and resumption support.

Changes

Rust signer-based asset-lock funding

Layer / File(s)	Summary
DPP state transition signer API packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/methods/mod.rs, packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/methods/v0/mod.rs	New trait methods try_from_asset_lock_with_signer_and_private_key (private-key explicit) and try_from_asset_lock_with_signers (external signer + derivation path) with version dispatch to v0 implementations.
DPP v0 implementation packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rs	Implements both signer paths: private-key-in-process signs asset-lock proof and per-input witnesses; external-signer path pre-signs witnesses then routes outer signing through core signer's trust boundary.
Test updates packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs, packages/rs-drive-abci/tests/strategy_tests/strategy.rs, packages/strategy-tests/src/lib.rs	Updates test helpers and strategy tests to use the explicit try_from_asset_lock_with_signer_and_private_key constructor.
Asset-lock orchestration packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs, packages/rs-platform-wallet/src/wallet/asset_lock/mod.rs, packages/rs-platform-wallet/src/wallet/asset_lock/manager.rs	New module defining AssetLockFunding enum (FromWalletBalance, FromExistingAssetLock), IS→CL timeout fallback via resolve_funding_with_is_timeout_fallback, and submit_with_cl_height_retry that bumps user_fee_increase and retries on consensus error 10506 within a time budget. Expands queue_asset_lock_changeset visibility to pub(crate).
Identity wallet refactoring packages/rs-platform-wallet/src/wallet/identity/network/registration.rs, packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs, packages/rs-platform-wallet/src/wallet/identity/network/top_up.rs, packages/rs-platform-wallet/src/wallet/identity/types/funding.rs, packages/rs-platform-wallet/src/wallet/identity/types/mod.rs, packages/rs-platform-wallet/src/wallet/identity/mod.rs	Removes local outpoint extraction and CL-height retry helpers; registration and top-up flows now use shared resolve_funding_with_is_timeout_fallback and submit_with_cl_height_retry. Consolidates IdentityFunding into a re-export of AssetLockFunding for backward compatibility.
Platform address wallet top-up packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs, packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs, packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs	New top_up method on PlatformAddressWallet that validates recipient addresses, resolves funding via orchestration (with IS→CL fallback), submits via submit_with_cl_height_retry, and writes balance changes. Adds asset_locks manager field and wallet_id() accessor. Removes fund_from_asset_lock module; adds top_up module.
FFI top-up entry points packages/rs-platform-wallet-ffi/src/platform_addresses/top_up.rs, packages/rs-platform-wallet-ffi/src/platform_addresses/mod.rs	Two #[no_mangle] FFI functions: platform_address_wallet_top_up_signer (fresh Core funding) and platform_address_wallet_resume_top_up_with_existing_asset_lock_signer (resume with existing lock). Includes decode_funding_addresses helper for safe FFI array decoding.
FFI legacy and identity updates packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs, packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rs, packages/rs-platform-wallet-ffi/src/platform_address_types.rs	Fixes private-key network derivation from wallet in fund_from_asset_lock; updates identity registration FFI to dispatch on AssetLockFunding variants; renames comment to reflect top_up context.
Module re-exports and dependencies packages/rs-platform-wallet/src/lib.rs, packages/rs-platform-wallet/Cargo.toml	Adds AssetLockFunding re-export for external consumers; removes duplicate IdentityFunding from identity re-exports. Adds drive-proof-verifier dependency.
SDK trait extension packages/rs-sdk/src/platform/transition/top_up_address.rs	Extends TopUpAddress trait with top_up_with_signers method for external signer support. Adds shared broadcast_and_collect_address_infos helper and updates legacy path to use explicit try_from_asset_lock_with_signer_and_private_key.

Swift SDK and example app signer-based funding UI

Layer / File(s)	Summary
Swift wallet funding methods packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift	Adds TopUpRecipient model and public topUpFromCore / resumeTopUpFromAssetLock methods that marshal FFI arguments, apply remainder-output fee strategy, and invoke Rust top-up functions while keeping MnemonicResolver alive.
Persistent model updates packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift	Adds recipientPlatformAddressHash and recipientPlatformAddressType optional fields for address-funded asset locks, with documentation for nil semantics and backward compatibility.
Address top-up controllers packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpController.swift, packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpCoordinator.swift, packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/PlatformWalletManager+AddressTopUpCoordinator.swift	AddressTopUpController manages phase state machine (idle, inFlight, completed, failed) and slot identity. AddressTopUpCoordinator enforces single-flight semantics per slot, schedules ~30s retention sweep after completion, and provides coordinator instance via per-manager lazy initialization.
Progress and list views packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressTopUpProgressView.swift, packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift	AddressTopUpProgressSection renders 1–5 step timeline with state classification based on lock status and time windows. PendingPlatformTopUpsList merges in-flight controllers and resumable asset locks with phase-specific icons and resume buttons.
Main top-up flow packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpPlatformAddressView.swift	Guides user through fresh top-ups or resume mode with recipient selection, amount entry, pre-submit outpoint capture, and post-completion recipient backfill to asset-lock rows. Submits via coordinator's single-flight controller.
Wallet detail integration packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift	Adds fund-platform affordance to Platform Balance card, displays pending/resumable asset-lock list, and wires TopUpPlatformAddressView sheets for both fresh and resume flows.
Storage detail display packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift	Updates AssetLockStorageDetailView to show recipient platform address, address type label, and computed DIP-0018 bech32m address for address-funded locks using wallet network HRP.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

• QuantumExplorer

Poem

🐰 A bunny hops through chains with signer grace,
Platform funds now flow through outer-space,
No keys cross borders—trust boundaries kept,
While Swift screens guide the coins that leapt,
bounce bounce ✨

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

[coderabbitai]

Actionable comments posted: 5

Caution

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

⚠️ Outside diff range comments (1)

packages/rs-drive-abci/tests/strategy_tests/strategy.rs (1)

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

Prevent stale balance state when funding transition creation fails.

Line 2602 mutates current_addresses_with_balance before the transition is built, and Line 2623 (.ok()?) silently drops builder/signing errors. That can leave a credited address without a corresponding transition and mask test regressions.

Proposed fix

-        current_addresses_with_balance.register_new_address_keep_only_highest(
-            address,
-            funded_amount,
-            self.max_addresses_to_choose_from_in_cache,
-        );
         let mut outputs = BTreeMap::new();
         outputs.insert(address.clone(), None);

         tracing::debug!(?outputs, "Preparing funding transition");
         let funding_transition =
             AddressFundingFromAssetLockTransitionV0::try_from_asset_lock_with_signer_and_private_key(
                 asset_lock_proof,
                 asset_lock_private_key.as_slice(),
                 BTreeMap::new(),
                 outputs,
                 vec![AddressFundsFeeStrategyStep::ReduceOutput(0)],
                 signer,
                 0,
                 platform_version,
             )
             .await
-            .ok()?;
+            .expect("expected to create address funding from asset lock transition");
+
+        current_addresses_with_balance.register_new_address_keep_only_highest(
+            address,
+            funded_amount,
+            self.max_addresses_to_choose_from_in_cache,
+        );

         Some(funding_transition)

🤖 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-drive-abci/tests/strategy_tests/strategy.rs` around lines 2602 -
2623, The test mutates current_addresses_with_balance via
current_addresses_with_balance.register_new_address_keep_only_highest before
calling
AddressFundingFromAssetLockTransitionV0::try_from_asset_lock_with_signer_and_private_key,
and then silently drops errors with .ok()?, leaving stale credited addresses if
transition creation fails; change the flow so you only call
register_new_address_keep_only_highest after the
try_from_asset_lock_with_signer_and_private_key future returns Ok (or,
alternatively, attempt the transition first and on Err do not mutate the cache
or explicitly rollback the registration), i.e., move the call to
current_addresses_with_balance.register_new_address_keep_only_highest (or
perform a compensating removal) to occur after successful creation of
funding_transition (the result of
try_from_asset_lock_with_signer_and_private_key) so failures don’t leave
inconsistent state.

🤖 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/platform_addresses/fund_with_funding.rs`:
- Around line 221-240: In decode_funding_addresses, currently using
address_map.insert silently overwrites duplicates; change the logic to detect
duplicate recipients (e.g., check address_map.contains_key(&addr) or use the
BTreeMap::entry API) when iterating over FundingAddressEntryFFI entries and
return a PlatformWalletFFIResult::err (use
PlatformWalletFFIResultCode::ErrorInvalidParameter) with a message mentioning
the duplicate platform address (derived from entry.address) instead of
inserting, so duplicate funding recipients are rejected.

In
`@packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs`:
- Around line 250-254: The code currently swallows a successful on-chain/top-up
by returning PlatformAddressChangeSet::default() when
wm.get_wallet_info_mut(...) or
core_wallet.platform_payment_managed_account_at_index_mut(...) is missing, which
yields a silent empty local changeset; instead, when submit succeeded but the
wallet or account cannot be found for write-back, return an explicit error or
construct a changeset that reflects the confirmed top-up (do not use
PlatformAddressChangeSet::default()). Update the branches around
wm.get_wallet_info_mut(&self.wallet_id) and
platform_payment_managed_account_at_index_mut(platform_account_index) to either
(a) return Err(...) with context about the successful submit and missing
wallet/account so the caller can reconcile, or (b) build and return a
PlatformAddressChangeSet that includes the applied top-up amount and metadata
from the submit result; apply the same change to the similar check at the other
location (lines ~290-291).
- Around line 271-281: The code silently defaults address_index to 0 when no
matching address is found, which hides invariant breaks and can corrupt
PlatformAddressChangeSet; replace the unwrap_or(0) on the result of the find_map
over account.addresses.addresses (the block that calls
PlatformP2PKHAddress::from_address and compares to p2pkh) with a proper error
propagation: return an Err (or use expect with a clear message) when no matching
index is found, and update the surrounding function to propagate that error (use
? or map_err as appropriate) so missing address_index surfaces instead of
defaulting to the first address.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- Around line 460-465: The code hardcodes the fee-strategy discriminant by
constructing FeeStrategyStepFFI(step_type: 1, index: remainderIndex) which
couples Swift to Rust's enum layout; change this to use a symbol exported from
FFI (or a generated Swift constant) for the ReduceOutput variant instead of the
literal 1 (e.g., replace step_type: 1 with step_type:
FFI_FEE_STRATEGY_REDUCE_OUTPUT or the generated Swift enum/case provided by the
bindings), update all occurrences such as the FeeStrategyStepFFI creation near
ffiAddresses and the similar block at lines ~558-563 to reference that exported
constant/enum case, and if needed add the new constant in the Rust FFI layer so
the Swift wrapper consumes the canonical discriminant rather than mirroring it.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift`:
- Around line 275-290: coreAccountOptions currently includes accounts with zero
or insufficient confirmed balance and canSubmit does not validate the selected
account against parsedDuffs, allowing impossible submissions; update
coreAccountOptions (and the similar getter around lines 324-330) to filter
accounts by balanceConfirmed >= parsedDuffs or, alternatively, keep
coreAccountOptions as-is but modify canSubmit to check
selectedAccount.balanceConfirmed >= parsedDuffs before allowing submission.
Reference the CoreAccountOption mapping (accountIndex, balanceDuffs), the
coreAccountOptions computed property and the canSubmit logic, and ensure the UI
picker only shows spendable accounts or that canSubmit gates submission based on
balanceDuffs >= parsedDuffs.

---

Outside diff comments:
In `@packages/rs-drive-abci/tests/strategy_tests/strategy.rs`:
- Around line 2602-2623: The test mutates current_addresses_with_balance via
current_addresses_with_balance.register_new_address_keep_only_highest before
calling
AddressFundingFromAssetLockTransitionV0::try_from_asset_lock_with_signer_and_private_key,
and then silently drops errors with .ok()?, leaving stale credited addresses if
transition creation fails; change the flow so you only call
register_new_address_keep_only_highest after the
try_from_asset_lock_with_signer_and_private_key future returns Ok (or,
alternatively, attempt the transition first and on Err do not mutate the cache
or explicitly rollback the registration), i.e., move the call to
current_addresses_with_balance.register_new_address_keep_only_highest (or
perform a compensating removal) to occur after successful creation of
funding_transition (the result of
try_from_asset_lock_with_signer_and_private_key) so failures don’t leave
inconsistent state.

🪄 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: 0af12630-b1e9-426d-ac09-66abb82fbb24

📥 Commits

Reviewing files that changed from the base of the PR and between 91859f9 and c547c4d.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (24)

• packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/methods/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/methods/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs
• packages/rs-drive-abci/tests/strategy_tests/strategy.rs
• packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs
• packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs
• packages/rs-platform-wallet-ffi/src/platform_addresses/mod.rs
• packages/rs-platform-wallet/Cargo.toml
• packages/rs-platform-wallet/src/wallet/asset_lock/mod.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs
• packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
• packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
• packages/rs-platform-wallet/src/wallet/identity/types/funding.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/mod.rs
• packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs
• packages/rs-platform-wallet/src/wallet/platform_wallet.rs
• packages/rs-sdk/src/platform/transition/top_up_address.rs
• packages/strategy-tests/src/lib.rs
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift

[codecov]

Codecov Report

❌ Patch coverage is 89.87342% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.17%. Comparing base (e230c9e) to head (3619d2d).

Files with missing lines	Patch %	Lines
..._funding_from_asset_lock_transition/methods/mod.rs	80.95%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           v3.1-dev    #3671      +/-   ##
============================================
+ Coverage     87.16%   87.17%   +0.01%     
============================================
  Files          2607     2607              
  Lines        319420   319489      +69     
============================================
+ Hits         278413   278506      +93     
+ Misses        41007    40983      -24     

Components	Coverage Δ
dpp	87.71% <89.87%> (+0.04%)	⬆️
drive	85.95% <ø> (ø)
drive-abci	89.60% <ø> (ø)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.17% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	49.16% <ø> (ø)

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

[thepastaclaw]

Code Review

PR cleanly mirrors the identity-side asset-lock funding pipeline (PR #3634) for platform addresses, with a thoughtful refactor lifting shared retry/resolver logic into asset_lock/orchestration. No blocking correctness or security issues identified after verification — the duplicate-recipient concern raised by codex as blocking matches existing patterns in parse_outputs and the legacy fund_from_asset_lock FFI, so it is downgraded to a suggestion. Main concerns are test coverage on the new orchestrator wrapper, a missing persister call (inconsistent with transfer.rs but consistent with the legacy raw-key funding path), and several minor contract/doc/UX mismatches.

🟡 4 suggestion(s) | 💬 6 nitpick(s)

10 additional finding(s)

suggestion: Duplicate funding recipients silently collapse in BTreeMap decode

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)

decode_funding_addresses inserts each FFI row into a BTreeMap without checking key collisions, so if Swift submits two rows for the same PlatformAddress, the second silently overwrites the first. Two consequences: (a) the funded output set differs from what the caller's [FundingRecipient] ordering and remainderIndex (ManagedPlatformAddressWallet.swift:460-465, 558-563) describe, and (b) a duplicate that flips has_balance can re-designate which entry is the None remainder, changing fee-strategy targeting after deduplication.

Note: this same silent-insert pattern exists in the sibling parse_outputs (platform_address_types.rs:189-204) used by the transfer path and in the legacy fund_from_asset_lock.rs:35-44 decode loop — codex's claim that 'the transfer API already rejects duplicate recipients' is inaccurate. So this is a pre-existing FFI pattern rather than a new regression, but the new funding API is a good place to start tightening it: rejecting duplicates here protects callers from a class of confusing protocol-side failures.

pub(super) unsafe fn decode_funding_addresses(
    addresses: *const FundingAddressEntryFFI,
    addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
    let mut address_map = BTreeMap::new();
    for entry in std::slice::from_raw_parts(addresses, addresses_count) {
        let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
            PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!("invalid platform address: {e}"),
            )
        })?;
        let balance = if entry.has_balance { Some(entry.balance) } else { None };
        if address_map.insert(addr, balance).is_some() {
            return Err(PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                "duplicate platform address in funding recipients",
            ));
        }
    }
    Ok(address_map)
}

suggestion: Funding success path doesn't push changeset through the persister

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 201)

After a successful submit, write_address_balances_changeset updates ManagedPlatformAccount in memory and returns a PlatformAddressChangeSet, but the orchestrator does not call self.persister.store(cs.clone().into()). The transfer path (transfer.rs:155-159) explicitly persists, with a long comment explaining that without persistence initialize_from_persisted on the next process start would seed stale balances and break auto_select_inputs.

The legacy fund_from_asset_lock (fund_from_asset_lock.rs:100-102) has the same gap, so this PR isn't introducing the divergence — it's preserving it on the new orchestrated path. Worth closing on both paths so the new user-facing fund-from-Core flow is crash-resumable without relying on the next BLAST sync to reconcile.

        if !cs.addresses.is_empty() {
            if let Err(e) = self.persister.store(cs.clone().into()) {
                tracing::error!("Failed to persist platform-address funding changeset: {}", e);
            }
        }

        Ok(cs)

suggestion: Swift FundingRecipient advertises addressType=1 (P2SH) but Rust FFI rejects it

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)

FundingRecipient docs at :382 say 0 = P2PKH, 1 = P2SH, and both new entry points forward addressType unchanged into PlatformAddressFFI (:449, :551). On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress at packages/rs-platform-wallet-ffi/src/platform_address_types.rs:40-42 only accepts 0 and returns Unsupported address type for anything else. The Rust orchestrator also explicitly enforces P2PKH-only (fund_with_funding.rs:336-339).

Swift callers will only learn this after crossing the FFI boundary and getting a generic invalid-parameter error. The preflight is the right place to surface it.

        for r in recipients {
            guard r.addressType == 0 else {
                throw PlatformWalletError.invalidParameter(
                    "FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
                )
            }
            guard r.hash.count == 20 else {
                throw PlatformWalletError.invalidParameter(
                    "FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
                )
            }
        }

suggestion: No direct tests for fund_addresses_with_funding orchestration branches

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)

submit_with_cl_height_retry has an explicit regression-pinning test (orchestration.rs:476), and the underlying DPP and SDK primitives are exercised in their crates. But the new orchestrator itself — which composes pre-flight + funding resolution + retry submission + IS→CL rejection fallback + balance writeback + asset-lock consume — has no direct test.

The most fragile branch is the IS-rejection fallback at lines 173-197: it restarts submit_with_cl_height_retry from the original settings (settings is Copy, so the bumped value from the first call is discarded). That is intentional (the proof type changes, so the ST hash differs and Tenderdash's invalid-tx cache no longer applies), but a follow-up refactor switching to &mut settings would silently break it. The PR description also marks end-to-end testnet validation as still pending. A targeted #[tokio::test] (e.g. with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success, would lock in the orchestration shape.

nitpick: Silent unwrap_or(0) for address_index in funding changeset writeback

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 271)

write_address_balances_changeset finds address_index by walking account.addresses.addresses and matching by PlatformP2PKHAddress, falling back to 0 on miss. Pre-flight uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool hasn't been hydrated for the given gap-limit slot, and the from_address(...).ok() silently swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry attributing the funded balance to address_index = 0 while every other field is correct — exactly the kind of silent off-by-one a SwiftData persister or UI will surface as 'all my funds landed on address #0'.

The same pattern exists in transfer.rs:119-129 (preserved, not introduced), but the orchestrated funding path has more chances for divergence because the address may have just been auto-picked from the unused pool. Prefer tracing::warn! on the fallback or skip the changeset entry when the index can't be resolved.

nitpick: Inline Duration::from_secs(300) should join the other named timeout constants

packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 404)

The module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch. Promote to e.g. IS_LOCK_RESUME_TIMEOUT so a future tuning pass touches one site. Also note: fund_with_funding.rs:143 resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait, while the resolver here uses 300s — worth a comment if intentional or aligning if not.

nitpick: Doc claim of 'shared with the legacy raw-private-key FFI' is inaccurate

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)

The comment on decode_funding_addresses claims it's 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' In fact, platform_addresses/fund_from_asset_lock.rs:35-44 still inlines its own decode loop with for entry in std::slice::from_raw_parts(addresses, addresses_count) { let addr = unwrap_result_or_return!(...); }. The two paths are not actually sharing decoding logic, so a future tightening (e.g. duplicate rejection, P2SH boundary check, error-message wording) on one will silently diverge from the other. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.

nitpick: Single DAPI 10506 response can force fee escalation across new funding path

packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 217)

submit_with_cl_height_retry treats a single InvalidAssetLockProofCoreChainHeightError from the connected DAPI node as authoritative and bumps PutSettings::user_fee_increase before resubmitting. With fund_addresses_with_funding now wrapping Core-funded address top-ups in this helper, a malicious or desynchronized node can drive up to ~CL_HEIGHT_RETRY_BUDGET / CL_HEIGHT_RETRY_DELAY retries with escalating fees per address-funding attempt without holding the asset-lock key.

Not a new vector in this PR — the identity-funding path uses the same helper — but worth tracking as a bounded griefing surface across a new trust boundary. A future hardening pass should consider second-source confirmation or node rotation before accepting a fee bump.

nitpick: Recipient picker over-restricts to !isUsed && balance == 0

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 309)

recipientCandidates requires both !$0.isUsed and $0.balance == 0. The Rust-side pre-flight in validate_recipient_addresses only requires that the address belong to the supplied platform-payment account — it does not require the address to be unused or empty. Users who have previously funded or received to a platform address cannot pick it again from this screen even though the protocol accepts it.

If the intent is to bias the default toward fresh addresses without forbidding used ones, prefer sorting (used last) over filtering them out, or split the filter from the auto-pick policy in autoSelectRecipient. If the intent is genuinely to forbid them, the UI should explain why.

nitpick: decode_funding_addresses relies on caller's check_ptr! for null-safety

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)

The safety doc says addresses must be non-null. Inside, it unconditionally calls std::slice::from_raw_parts(addresses, addresses_count) — which is UB on a null data pointer even when count == 0 (the function requires a non-null aligned dangling-or-valid pointer). Today's two callers both do check_ptr!(addresses) first, so it's sound. But because the helper is pub(super), a future caller in the same module that forgets the prelude is one keystroke from UB.

Sibling helpers in the same crate (parse_outputs, parse_explicit_inputs) defensively check if ptr.is_null() && count > 0 { ... } internally, but even that pattern isn't quite right for the count == 0 case. Adding if addresses.is_null() { return Err(...); } unconditionally aligns this helper with the rest of the crate without changing today's behavior.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

Incremental review at ff31d50. The new delta is a single SwiftUI commit (Fund L2 entry point relocated to an inline '+' affordance on the Platform Balance row) — verified clean, no new findings. All 10 prior findings from c547c4d were re-verified against current source and remain STILL VALID; none of the underlying Rust / FFI / Swift orchestration / picker code was touched. Note: the prior #8 (DAPI 10506 fee-escalation griefing) has gained explicit trust-model documentation at orchestration.rs:217-234 since the prior review — carried forward as a documented bounded risk rather than blocking. Carrying forward 10 findings within the 10-finding budget.

🟡 4 suggestion(s) | 💬 6 nitpick(s)

10 additional finding(s) omitted (not in diff).

Findings not attached inline (unchanged lines / prior carried-forward findings)

• **[suggestion] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:594 — Swift FundingRecipient advertises addressType=1 (P2SH) but Rust FFI rejects it

  Verified at ff31d50: the Swift FundingRecipient doc states 0 = P2PKH, 1 = P2SH, and both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress only accepts 0 and returns 'Unsupported address type' for anything else; the orchestrator additionally enforces P2PKH-only. fundFromCoreAssetLockPreflight (594-613) checks hash.count == 20 but does NOT check addressType == 0, so callers learn the constraint only after crossing the FFI boundary and getting a generic invalid-parameter error. The preflight is the right place to surface it — alternatively, narrow the Swift API surface (e.g. drop the byte and assume P2PKH) until P2SH is actually implemented on the Rust side.

• **[suggestion] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs:221 — Duplicate funding recipients silently collapse in BTreeMap decode

  Verified at ff31d50 (lines 221-241 unchanged from c547c4d): decode_funding_addresses inserts each FFI row into a BTreeMap without checking key collisions. If Swift submits two rows for the same PlatformAddress, the second silently overwrites the first. Two consequences: (a) the funded output set diverges from the caller's [FundingRecipient] ordering and remainderIndex computation (Swift indexes the pre-collapse array at ManagedPlatformAddressWallet.swift:460-465, 558-563), and (b) a duplicate that flips has_balance can re-designate which entry is the None remainder, changing fee-strategy targeting. BTreeMap::insert's return value already detects the collision at zero extra cost.

• **[suggestion] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:205 — Funding success path doesn't push changeset through the persister

  Verified at ff31d50 (lines 205-223 unchanged from c547c4d): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount in memory and returns a PlatformAddressChangeSet, but the orchestrator never calls self.persister.store(cs.clone().into()). The transfer path explicitly persists (with a comment about how initialize_from_persisted would otherwise seed stale balances and break auto_select_inputs on next process start). The legacy fund_from_asset_lock has the same gap, so this PR preserves rather than introduces the divergence — but closing it on the new path makes the user-facing fund-from-Core flow crash-resumable without relying on the next BLAST sync to reconcile. Alternatively, change the return type so the caller is forced to acknowledge the not-yet-persisted state.

• **[suggestion] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:88 — No direct tests for fund_addresses_with_funding orchestration branches

  Verified at ff31d50: submit_with_cl_height_retry has an explicit regression-pinning test and the underlying DPP/SDK primitives are exercised elsewhere, but the new orchestrator — composing pre-flight, funding resolution, retry submission, IS→CL rejection fallback, balance writeback, and asset-lock consume — has no direct test. The most fragile branch is the IS-rejection fallback at lines 173-197: it restarts submit_with_cl_height_retry from the original settings (settings is Copy, so the bumped value from the first call is discarded). That is intentional (the proof type changes, so the ST hash differs and Tenderdash's invalid-tx cache no longer applies), but a follow-up refactor switching to &mut settings would silently break it. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success, would lock in the orchestration shape. PR description marks end-to-end testnet validation as still pending.

• **[nitpick] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs:210 — Doc claim of 'shared with the legacy raw-private-key FFI' is inaccurate

  Verified at ff31d50: the comment on decode_funding_addresses claims it's 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' In fact, platform_addresses/fund_from_asset_lock.rs:35-44 still inlines its own decode loop. The two paths are not actually sharing decoding logic, so a future tightening (duplicate rejection, P2SH boundary check, error-message wording) on one will silently diverge from the other. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.

• **[nitpick] packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs:402 — Inline Duration::from_secs(300) should join the other named timeout constants

  Verified at ff31d50: the module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as named policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch at line 404. Promote to e.g. IS_LOCK_RESUME_TIMEOUT. Also: fund_with_funding.rs:143 resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait, while the resolver here uses 300s — worth a comment if intentional or aligning if not.

• **[nitpick] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:271 — Silent unwrap_or(0) for address_index in funding changeset writeback

  Verified at ff31d50: write_address_balances_changeset resolves address_index by walking account.addresses.addresses and matching by PlatformP2PKHAddress, falling back to 0 with unwrap_or(0) on miss. Pre-flight uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool hasn't been hydrated for the given gap-limit slot, and from_address(...).ok() silently swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry attributing the funded balance to address_index = 0 while every other field is correct — exactly the kind of silent off-by-one a SwiftData persister will surface as 'all my funds landed on address #0'. The same pattern exists in transfer.rs:119-129 (preserved, not introduced). Prefer tracing::warn! on the fallback, or skip the changeset entry when the index can't be resolved.

• **[nitpick] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift:309 — Recipient picker over-restricts to !isUsed && balance == 0

  Verified at ff31d50 (line 312 unchanged — the latest delta refactored only WalletDetailView, not FundPlatformAddressView): recipientCandidates requires both !$0.isUsed and $0.balance == 0. The Rust-side pre-flight in validate_recipient_addresses only requires that the address belong to the supplied platform-payment account — it does not require the address to be unused or empty. Users who have previously funded or received to a platform address cannot pick it again from this screen even though the protocol accepts it. If the intent is to bias the default toward fresh addresses, prefer sorting (used last) over filtering, or split the filter from the auto-pick policy in autoSelectRecipient. If the intent is genuinely to forbid them, the UI should explain why.

• **[nitpick] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs:218 — decode_funding_addresses relies on caller's check_ptr! for null-safety

  Verified at ff31d50: the // Safety doc requires non-null addresses. Inside, it unconditionally calls std::slice::from_raw_parts(addresses, addresses_count) — which is UB on a null data pointer even when count == 0 per the slice contract. Today's two callers both do check_ptr!(addresses) first, so it's sound. But the helper is pub(super), and a future intra-module caller that forgets the prelude — or fast-paths addresses_count == 0 on the hunch that 'an empty array can't matter' — would silently introduce UB at an FFI trust boundary consuming attacker-controlled pointers. Sibling helpers (parse_outputs, parse_explicit_inputs) defensively null-check internally; aligning here costs nothing and removes a sharp edge from the boundary.

• **[nitpick] packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs:217 — Single DAPI 10506 response can force fee escalation across new funding path

  Verified at ff31d50: submit_with_cl_height_retry treats a single InvalidAssetLockProofCoreChainHeightError from the connected DAPI node as authoritative and bumps PutSettings::user_fee_increase before resubmitting. With fund_addresses_with_funding now wrapping Core-funded address top-ups in this helper, a malicious or desynchronized node can drive up to ~CL_HEIGHT_RETRY_BUDGET / CL_HEIGHT_RETRY_DELAY retries with escalating fees per address-funding attempt without holding the asset-lock key. The trust model is now explicitly documented at orchestration.rs:217-234 (bounded budget, attack-unprofitability reasoning), so this is documented technical debt rather than an undisclosed defect — carrying forward for visibility. A future hardening pass should consider second-source confirmation or node rotation before accepting a fee bump.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

Incremental re-review at 9889023. The single new commit since ff31d50 is SwiftUI-only (FundPlatformAddressView: BIP44 Core-account picker filter + live FFI balance) and is verified clean. All substantive findings from the prior cumulative review at ff31d50 were re-checked against current source at 9889023 and remain valid; the new commit does not touch the Rust orchestrator, FFI decoder, Swift preflight/marshaling, recipient-picker filter, or test surface. Two reviewers independently converged on the same 8 underlying issues; carrying forward 10 findings within budget.

🟡 4 suggestion(s) | 💬 6 nitpick(s)

10 additional finding(s)

suggestion: Swift preflight accepts `addressType` values that Rust then rejects across the FFI boundary

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 606)

Verified at 9889023: fundFromCoreAssetLockPreflight (594-613) validates hash.count == 20 but does not validate addressType. The public Swift FundingRecipient documents 0 = P2PKH, 1 = P2SH, and both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress only accepts 0 (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-43) and the orchestrator additionally enforces P2PKH-only. A caller building a P2SH FundingRecipient therefore passes Swift preflight, crosses the FFI boundary, and surfaces only a generic invalid-parameter error from Rust. Surface this constraint in the preflight (or narrow the Swift API surface to drop the byte until P2SH is implemented on the Rust side).

        for r in recipients {
            guard r.addressType == 0 else {
                throw PlatformWalletError.invalidParameter(
                    "FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
                )
            }
            guard r.hash.count == 20 else {
                throw PlatformWalletError.invalidParameter(
                    "FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
                )
            }
        }

suggestion: Duplicate funding recipients silently collapse in BTreeMap decode

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)

Verified at 9889023 (line 238): decode_funding_addresses calls address_map.insert(addr, balance) without inspecting the return value, so two FFI rows for the same PlatformAddress silently overwrite each other. Two concrete consequences across the boundary: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array the Swift caller passed in — which is significant because Swift derives the remainder index from that pre-collapse array (ManagedPlatformAddressWallet.swift:460-465, 558-563); and (b) if the duplicate row flips has_balance, the lone None remainder target can shift, changing fee-strategy attribution silently after dedup. BTreeMap::insert's return already exposes the collision at zero cost — reject explicitly at the FFI boundary.

pub(super) unsafe fn decode_funding_addresses(
    addresses: *const FundingAddressEntryFFI,
    addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
    if addresses.is_null() {
        return Err(PlatformWalletFFIResult::err(
            PlatformWalletFFIResultCode::ErrorInvalidParameter,
            "addresses pointer is null",
        ));
    }
    let mut address_map = BTreeMap::new();
    for entry in std::slice::from_raw_parts(addresses, addresses_count) {
        let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
            PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!("invalid platform address: {e}"),
            )
        })?;
        let balance = if entry.has_balance { Some(entry.balance) } else { None };
        if address_map.insert(addr, balance).is_some() {
            return Err(PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                "duplicate platform address in funding recipients",
            ));
        }
    }
    Ok(address_map)
}

suggestion: Funding success path mutates in-memory balances but never pushes the changeset through the persister

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 205)

Verified at 9889023 (lines 205-223): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount and returns a PlatformAddressChangeSet, but the orchestrator returns immediately after consume_asset_lock without invoking self.persister.store(cs.clone().into()). The transfer path explicitly persists, with an inline comment that stale persisted balances would otherwise break initialize_from_persisted and auto_select_inputs on next process start. The Rust crate owns the persister, so the Swift caller cannot compensate from the returned changeset. The legacy fund_from_asset_lock has the same gap, so this is preserved rather than introduced — but the new Core-funded user-facing path stays crash-fragile until a sync reconciles. Note the relevance to the new delta in this PR: the Swift app now reads Core balances live from accountBalances(for:) instead of stale SwiftData precisely to dodge the same lag — the Platform side has an exact mirror here.

        let cs = self
            .write_address_balances_changeset(platform_account_index, &address_infos)
            .await;

        if !cs.addresses.is_empty() {
            if let Err(e) = self.persister.store(cs.clone().into()) {
                tracing::error!("Failed to persist platform-address funding changeset: {}", e);
            }
        }

        if let Some(out_point) = tracked_out_point {
            // Cleanup failure can only mean WalletNotFound (the wallet
            // handle that just funded the addresses vanished). Surface
            // as a warn — Platform DID accept the top-up, so
            // propagating the error to the caller would be misleading.
            if let Err(e) = self.asset_locks.consume_asset_lock(&out_point).await {
                tracing::warn!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock failed after successful Platform submit"
                );
            }
        }

        Ok(cs)

suggestion: No direct tests for fund_addresses_with_funding orchestration branches

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)

Verified at 9889023: submit_with_cl_height_retry has an explicit regression-pinning test and DPP/SDK primitives are exercised elsewhere, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. The most fragile branch is the IS-rejection fallback: it intentionally restarts submit_with_cl_height_retry from the original settings value (settings is Copy, so the bumped value from the first call is discarded) because the proof type changes and the ST hash differs, so Tenderdash's invalid-tx cache no longer applies. A future refactor switching to &mut settings would silently break that. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success would pin the orchestration shape. PR description marks end-to-end testnet validation as still pending.

nitpick: Inline `Duration::from_secs(300)` should join the other named timeout constants

packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 402)

Verified at 9889023 (line 404 confirmed): the module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as named policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch. Additionally, fund_with_funding.rs resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait while the resolver here uses 300s; if intentional, add a comment, otherwise align. The mismatch matters because both timeouts gate when the FFI top_up call returns to Swift. Promote to e.g. IS_LOCK_RESUME_TIMEOUT.

nitpick: Doc claim of "shared with the legacy raw-private-key FFI" is inaccurate

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)

Verified at 9889023 (lines 214-216): the doc on decode_funding_addresses states it is 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' That is not true — platform_addresses/fund_from_asset_lock.rs keeps its own inline for entry in from_raw_parts(...) decode loop. The two FFI decode paths therefore do not share logic, so any future tightening of one (duplicate rejection, null handling, P2SH boundary check, error wording) will silently diverge. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.

nitpick: Silent `unwrap_or(0)` fabricates `address_index = 0` when recipient lookup fails

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 271)

Verified at 9889023 (lines 271-281): write_address_balances_changeset resolves address_index by walking account.addresses.addresses, parsing each stored address, matching by PlatformP2PKHAddress, and falling back to 0 via unwrap_or(0) on miss. Preflight membership uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool is not hydrated for the relevant gap-limit slot, and the from_address(...).ok() swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry with correct hash/balance but wrong address_index, which a SwiftData persister will surface as 'all my funds landed on address #0'. Same pattern exists in transfer.rs:119-129 (preserved, not introduced). Either tracing::warn! and skip the entry, or surface the invariant violation explicitly.

nitpick: Recipient picker over-restricts to `!isUsed && balance == 0`

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 317)

Verified at 9889023 (line 320): recipientCandidates requires !$0.isUsed && $0.balance == 0. The Rust-side preflight in validate_recipient_addresses only requires the address to belong to the supplied platform-payment account — it does not require unused or zero-balance. As a result, users cannot pick previously-funded or previously-received-on platform addresses from this screen even though the protocol and FFI accept them. If the intent is to bias the default toward fresh addresses, sort (used last) rather than filter, or split the filter from the auto-pick policy in autoSelectRecipient. If the intent is genuinely to forbid them, the UI should explain why.

nitpick: `decode_funding_addresses` relies on caller's `check_ptr!` for null-safety at an FFI trust boundary

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)

Verified at 9889023: the // Safety doc requires non-null addresses, but the body unconditionally calls std::slice::from_raw_parts(addresses, addresses_count). Per the slice contract this is UB on a null data pointer even when count == 0. Today's two FFI entry points gate with check_ptr!(addresses), so current call sites are sound. But the helper is pub(super) and sits on a C ABI trust boundary consuming caller-supplied raw pointers; a future intra-module caller that omits check_ptr! (or fast-paths addresses_count == 0 on the hunch that 'empty can't matter') would silently introduce UB the optimizer is free to compile into a memory-safety bug. Sibling helpers in this crate (parse_outputs, parse_explicit_inputs) defensively null-check internally; aligning here costs nothing.

pub(super) unsafe fn decode_funding_addresses(
    addresses: *const FundingAddressEntryFFI,
    addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
    if addresses.is_null() {
        return Err(PlatformWalletFFIResult::err(
            PlatformWalletFFIResultCode::ErrorInvalidParameter,
            "addresses pointer is null",
        ));
    }
    let mut address_map = BTreeMap::new();
    for entry in std::slice::from_raw_parts(addresses, addresses_count) {
        let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
            PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!("invalid platform address: {e}"),
            )
        })?;
        let balance = if entry.has_balance { Some(entry.balance) } else { None };
        address_map.insert(addr, balance);
    }
    Ok(address_map)
}

nitpick: Single DAPI 10506 response authoritatively bumps fees across new platform-address funding path

packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 217)

Verified at 9889023: submit_with_cl_height_retry treats the first InvalidAssetLockProofCoreChainHeightError from the connected DAPI node as authoritative and bumps PutSettings::user_fee_increase before resubmitting, with up to roughly CL_HEIGHT_RETRY_BUDGET / CL_HEIGHT_RETRY_DELAY (~14) escalating retries. This PR extends the helper to wrap fund_addresses_with_funding, so a malicious or desynchronized DAPI node can now drive the same bounded fee-escalation grief on Core-funded platform-address top-ups in addition to identity registration. Attacker needs neither the asset-lock key nor Platform consensus control — just to be the queried endpoint. The trust model is explicitly documented in source (orchestration.rs:217-234) as accepted technical debt with a bounded-griefing/attack-unprofitability rationale, so this is carried forward for visibility rather than as an undisclosed defect. A future hardening pass should consider second-source confirmation or node rotation before accepting a fee bump.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[coderabbitai]

Actionable comments posted: 1

🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift`:
- Around line 255-258: The footerText currently instructs "Tap Dismiss..." but
AddressFundingProgressView does not provide a dismiss action; update the view
(the block that renders the footer around where footerText is used, currently
the code section rendering the footer UI) to add a Button labeled "Dismiss" that
calls the same coordinator dismissal used in the inline funding view (call
coordinator.dismiss() or the same coordinator method used there). Keep
footerText as-is for the message, but render the Button when isFailed is true
and wire it to coordinator.dismiss() so tapping Dismiss clears the entry exactly
like the inline funding view.

🪄 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: 72b743d3-fb27-4c90-85e4-4e95626f17ca

📥 Commits

Reviewing files that changed from the base of the PR and between c547c4d and 9efcf6e.

📒 Files selected for processing (6)

• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressFundingController.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressFundingCoordinator.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/PlatformWalletManager+AddressFundingCoordinator.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift

[thepastaclaw]

Code Review

All major findings re-verified at 9efcf6e. No blocking issues. The SwiftExampleApp-only delta since 9889023 introduces one new concrete logic gap (AddressFundingProgressSection's @query has no per-recipient scope, so concurrent address-fundings on one wallet can alias onto the same progress row), plus a coordinator/sheet-lifetime gap that undercuts the stated goal of surviving sheet dismissal. The 10 prior Rust/FFI/Swift-SDK findings all reproduce at the same line ranges. Two additional prior nitpicks (recipient picker UX divergence; DAPI 10506 fee-escalation surface) are retained below as carried-forward prior findings outside the 10 expanded finding budget.

🟡 6 suggestion(s) | 💬 4 nitpick(s)

10 expanded finding(s)

suggestion: Swift preflight accepts `FundingRecipient.addressType` values that Rust then rejects

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)

Verified at 9efcf6e (lines 594-613): fundFromCoreAssetLockPreflight checks recipients.isEmpty, the single-nil-remainder invariant, and hash.count == 20, but never inspects addressType. Both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-43) only accepts 0, and the orchestrator additionally enforces P2PKH-only. A caller building a P2SH FundingRecipient therefore passes the synchronous preflight, spins up the Task, marshals the handle, and surfaces only a generic invalid-parameter error from Rust — defeating the entire point of the preflight, which the doc says exists to deliver a synchronous error before paying for the Task detach + handle marshaling. Either validate addressType == 0 in the preflight, or drop the byte from the public Swift surface until P2SH is actually wired up Rust-side.

suggestion: Duplicate funding recipients silently collapse in the FFI BTreeMap decode

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)

Verified at 9efcf6e (line 238): decode_funding_addresses calls address_map.insert(addr, balance) and ignores the return value, so two FFI rows for the same PlatformAddress silently overwrite each other at the C ABI boundary. Two concrete consequences: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array the Swift caller built — Swift derives the remainder index from that pre-collapse array, so after dedup the Rust side may see a different recipient set and a different None remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder target can shift, redirecting fees and remainder credits silently. BTreeMap::insert's return value already exposes the collision at zero cost — reject explicitly at the boundary so the input shape the caller passed is the input shape the orchestrator executes.

suggestion: Progress section mis-attributes asset-lock state when two address-fundings run concurrently on one wallet

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 51)

New at 9efcf6e. AddressFundingProgressSection.init constructs _activeLocks with a #Predicate scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc. The body then unconditionally drives currentStep, broadcastSubStep, and isInChainLockFallback from activeLocks.first (lines 109-127, 145-149, 154-163). AddressFundingController is keyed (walletId, platformAccountIndex, recipientHash) precisely to allow many concurrent fundings on one wallet (per its own doc), but PersistentAssetLock carries no recipient-hash linkage — only outPointHex, walletId, fundingTypeRaw, statusRaw, updatedAt. So when two AssetLockAddressTopUp flows are in flight, both progress views observe the same activeLocks query and both pick whichever lock most recently transitioned updatedAt. The identity-side analog disambiguates via identityIndexRaw; this view has no equivalent. Concrete consequence: Funding A's UI can advance to Step 3 because Funding B's lock just transitioned to Broadcast, even though A is still building its asset-lock tx. The controller's per-attempt .phase keeps .completed/.failed correct, but step count and CL-fallback footer can mis-attribute. Single-funding usage is unaffected; close this before the planned 'Pending Platform Funding' surface ships. Options: (a) return the asset-lock outpoint from the FFI on creation so the controller can scope its own @Query, or (b) add a recipient-hash field to PersistentAssetLock populated when the orchestrator picks the credit-output destination.

suggestion: In-flight funding is not restorable after sheet dismissal despite the new coordinator

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 59)

Verified at 9efcf6e: activeController is held only as @State local to this view (line 64) and never rehydrated from walletManager.addressFundingCoordinator on appear. The sheet is still presented via a plain .sheet from WalletDetailView, and .onAppear(perform: autoSelectDefaults) only resets defaults. So the user can dismiss the sheet interactively (Cancel is disabled while .inFlight, but swipe-down/interactive dismissal on iOS is not), and on re-presentation the view starts from activeController == nil and renders the blank form — the only way back to the original controller is to manually recreate the same slot and re-submit. That directly undercuts the in-source comment (lines 59-64) that lifetime ownership lives on the coordinator so 'view dismissal mid-flight doesn't lose the work'. Either rehydrate from the coordinator in .onAppear keyed by (walletId, platformAccountIndex, recipientHash), or block interactive dismissal while in-flight.

suggestion: Funding success path mutates in-memory balances but never pushes the changeset through the persister

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 205)

Verified at 9efcf6e (lines 205-223): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount and returns a PlatformAddressChangeSet, but the orchestrator returns immediately after consume_asset_lock without invoking self.persister.store(cs.clone().into()). The transfer path explicitly persists with an inline comment that stale persisted balances would otherwise break initialize_from_persisted and auto_select_inputs on next process start. The Rust crate owns the persister, so the Swift caller cannot compensate from the returned changeset. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local wallet doesn't know — affecting auto-input selection and any UI reading balances from the persisted SwiftData store. The legacy fund_from_asset_lock has the same gap (preserved, not introduced). Notable: the new AddressFundingCoordinator keeps the controller alive across view dismissal precisely so the success transition is observable — that's negated for a process restart while the persister gap remains.

suggestion: No direct tests for fund_addresses_with_funding orchestration branches

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)

Verified at 9efcf6e: submit_with_cl_height_retry has an explicit regression-pinning test and DPP/SDK primitives are exercised elsewhere, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. The most fragile branch is the IS-rejection fallback: it intentionally restarts submit_with_cl_height_retry from the original settings value — because PutSettings is Copy, the bumped value from the first call is discarded, which is required because the proof type changes, the ST hash differs, and Tenderdash's invalid-tx cache no longer applies. A future refactor switching to &mut settings would silently break that retry-cache-bypass invariant. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, and (c) consume-on-success would pin the orchestration shape. PR description marks end-to-end testnet validation as still pending.

nitpick: Doc claim of "shared with the legacy raw-private-key FFI" is inaccurate

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)

Verified at 9efcf6e (lines 214-216): the doc on decode_funding_addresses states it is 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' Confirmed false — platform_addresses/fund_from_asset_lock.rs:35-44 still has its own inline for entry in from_raw_parts(...) decode loop. The two FFI decode paths therefore do not share logic, so any future tightening of one (duplicate rejection, null handling, P2SH boundary check, error wording) will silently diverge across the boundary. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.

nitpick: Inline `Duration::from_secs(300)` should join the named timeout constants

packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 402)

Verified at 9efcf6e (line 404): the module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as named policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch. Separately, fund_with_funding.rs resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait while the resolver here uses 300s; if intentional, document why, otherwise align. Both timeouts gate when the FFI top_up call returns to Swift — and the new AddressFundingProgressSection.instantLockTimeout = 300.0 is yet another mirror that will drift independently from the Rust side. Promote to e.g. IS_LOCK_RESUME_TIMEOUT and reconcile the three sites.

nitpick: `decode_funding_addresses` relies on caller's `check_ptr!` for null-safety at an FFI trust boundary

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)

Verified at 9efcf6e: the // Safety doc requires non-null addresses, but the body unconditionally calls std::slice::from_raw_parts(addresses, addresses_count). Per the slice contract this is UB on a null data pointer even when count == 0. Today's two FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. But the helper is pub(super) and sits on a C ABI trust boundary consuming caller-supplied raw pointers; a future intra-module caller that omits check_ptr! — or fast-paths addresses_count == 0 on the hunch that 'empty can't matter' — would silently introduce UB the optimizer is free to compile into a memory-safety bug. Sibling helpers in this crate defensively null-check internally; aligning here costs nothing and removes a sharp edge from a security boundary.

nitpick: Silent `unwrap_or(0)` fabricates `address_index = 0` when recipient lookup fails

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 271)

Verified at 9efcf6e (lines 271-281): write_address_balances_changeset resolves address_index by walking account.addresses.addresses, parsing each stored address via PlatformP2PKHAddress::from_address(...).ok(), matching by PlatformP2PKHAddress, and falling back to 0 via unwrap_or(0) on miss. Preflight membership uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool is not hydrated for the relevant gap-limit slot, and the .ok() swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry with correct hash/balance but wrong address_index, which a SwiftData persister will surface as 'all my funds landed on address #0'. If a downstream operation ever derives signing keys or HD paths from address_index without re-matching the hash, the silent fabrication becomes a key-derivation mismatch. Same pattern exists in transfer.rs:119-129 (preserved, not introduced). Either tracing::warn! and skip the entry, or surface the invariant violation explicitly.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

Carried-forward prior findings retained from 9889023

These two 9889023 findings were also re-validated as still applicable at 9efcf6e2; I am keeping them in the review body explicitly so the cumulative review does not erase prior unresolved findings.

nitpick: Recipient picker over-restricts to `!isUsed && balance == 0`

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 320)

Still valid at 9efcf6e2: recipientCandidates continues to filter candidate platform addresses with !$0.isUsed && $0.balance == 0, while the Rust-side validate_recipient_addresses path only requires that the address belong to the supplied platform-payment account. That keeps the example UI narrower than the wallet orchestration actually supports, so users cannot pick some valid addresses through the demo flow.

nitpick: Single DAPI 10506 response authoritatively bumps fees across the new funding path

packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (lines 217-297)

Still valid at 9efcf6e2: submit_with_cl_height_retry still treats the first InvalidAssetLockProofCoreChainHeightError response from the connected DAPI node as authoritative enough to increase PutSettings::user_fee_increase before resubmitting. This remains an accepted technical-debt/security-surface nitpick rather than a blocker, but it is still part of the cumulative finding set for this head.

[thepastaclaw]

Code Review

Cumulative verifier output at 06c1930. Of the 10 prior findings, 1 (in-flight controller restorability after sheet dismissal) is FIXED by the new PendingPlatformFundingsList surface; the other 9 reproduce at unchanged line ranges. New delta is SwiftExampleApp-only — three new findings in the recipient-stamping + bech32m + pending-list code: the bech32m renderer hardcodes the testnet HRP regardless of wallet network, backfillRecipientOnConsumedLock matches consumed rows by status alone (race across concurrent fundings) and hardcodes the address-type byte, and the new PendingPlatformFundingsList anti-join is a no-op (controllers don't carry the outpoint they're driving) so the same lock can appear twice. Carrying forward 7 still-valid prior findings + 3 new ones; budget overflow dropped: inline 300s timeout, unwrap_or(0) address_index, parseOutPoint inverse coupling. No blocking issues.

🟡 8 suggestion(s) | 💬 2 nitpick(s)

10 additional finding(s)

suggestion: Swift preflight accepts FundingRecipient.addressType values that Rust then rejects

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)

Verified at 06c1930 (lines 594-613, unchanged from 9efcf6e): fundFromCoreAssetLockPreflight validates recipients.isEmpty, the single-nil-remainder invariant, and hash.count == 20, but never inspects addressType. Both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress only accepts 0 and the orchestrator additionally enforces P2PKH-only at validate_recipient_addresses. A caller building a P2SH FundingRecipient therefore passes the synchronous preflight, spins up the Task, marshals the handle, and only then surfaces a generic invalid-parameter error from Rust — defeating the documented purpose of the preflight (deliver a synchronous error before paying for the Task detach). This is also load-bearing for this PR's new backfillRecipientOnConsumedLock, which also hardcodes type=0 — both surfaces would need to be extended in lockstep if P2SH is ever wired up. Either reject addressType != 0 in the preflight, or drop the byte from the public Swift surface until Rust supports it.

        for r in recipients {
            guard r.addressType == 0 else {
                throw PlatformWalletError.invalidParameter(
                    "FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
                )
            }
            guard r.hash.count == 20 else {
                throw PlatformWalletError.invalidParameter(
                    "FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
                )
            }
        }

suggestion: Duplicate funding recipients silently collapse in FFI BTreeMap decode

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)

Verified at 06c1930: line 238 calls address_map.insert(addr, balance) and discards the return value, so two FFI rows for the same PlatformAddress silently overwrite each other at the C ABI boundary. Two concrete consequences: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array the Swift caller built — Swift derives the remainder index from that pre-collapse array (ManagedPlatformAddressWallet.swift:460-465, 558-563), so after dedup Rust may see a different recipient set and a different None remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder target can shift, redirecting fees and remainder credits silently. BTreeMap::insert's return value already exposes the collision at zero cost — reject explicitly at the boundary so the input shape the caller passed is the input shape the orchestrator executes.

suggestion: Progress section mis-attributes asset-lock state when two address-fundings run concurrently on one wallet

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 51)

Verified at 06c1930 (lines 60-66 unchanged): AddressFundingProgressSection.init builds _activeLocks with a #Predicate scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc, and the body drives currentStep, broadcastSubStep, and isInChainLockFallback from activeLocks.first (109-127, 145-149, 154-163). AddressFundingController is keyed (walletId, platformAccountIndex, recipientHash) precisely to allow many concurrent fundings on one wallet, but PersistentAssetLock carries no recipient-hash linkage during the in-flight phase — only outPointHex, walletId, fundingTypeRaw, statusRaw, updatedAt. The new recipientPlatformAddressHash column added in this PR is populated only after Consumed (statusRaw == 4), so it doesn't disambiguate during statusRaw ∈ [0,3]. Concrete consequence: Funding A's UI can advance to Step 3 because Funding B's lock just transitioned to Broadcast. The new PendingPlatformFundingsList explicitly invites concurrent fundings to coexist on one wallet, so this matters more than before. Fix: (a) propagate the asset-lock outpoint into AddressFundingController so the predicate can scope by outpoint, or (b) populate recipientPlatformAddressHash on the row at broadcast time, not just at consume.

suggestion: Bech32m platform-address renderer hardcodes 'tdash' HRP, mis-renders on mainnet

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift (line 2012)

New at 06c1930. Verified at lines 2015-2021: networkHRP() unconditionally returns "tdash" with an inline comment that 'Wallet row lookup is best-effort; we keep the explorer self-contained here.' The Rust canonical encoder (packages/rs-dpp/src/address_funds/platform_address.rs) picks the HRP from hrp_for_network(network) — dash on mainnet, tdash on testnet/devnet/regtest. Consequence: on mainnet the storage explorer renders funded platform-address rows as tdash1q…. A user copying that string back through any Rust-owned tool (CLI, SDK) gets rejected by PlatformAddress::from_bech32m_string because the HRP doesn't match the network — the round-trip Swift↔Rust silently breaks at the explorer boundary, which is exactly the surface users open to audit a funding when something looks wrong. record.walletId is already on the row; a FetchDescriptor<PersistentWallet>(predicate: walletId == record.walletId) lookup at render time would surface the network. Better still per the Swift SDK CLAUDE.md rule ('No re-implementing protocol constants … as Swift mirrors'), expose a single FFI helper that calls to_bech32m_string and returns the string for display, since the entire bech32mEncode/convertBits/type-byte mapping is a hand-rolled Swift mirror of canonical Rust logic with no round-trip test pinning them.

suggestion: backfillRecipientOnConsumedLock matches consumed rows by status alone and hardcodes P2PKH, races on concurrent fundings

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 629)

New at 06c1930. Verified at lines 629-665. Two distinct gaps in the same routine: (1) Line 640 hardcodes let recipientType: UInt8 = 0 // P2PKH discriminant., but the originally-FFI-submitted addressType byte (the thing this back-fill exists to document) is never captured on the controller. Today benign because the FFI rejects type != 0, but the moment either side relaxes (Swift preflight or Rust FFI) the storage explorer will display a wrong type discriminant for a successful funding. (2) Lines 641-649 match solely on (walletId, fundingTypeRaw==4, statusRaw==4, recipientPlatformAddressHash==nil) newest-first. With two back-to-back fundings on the same wallet, persister-callback ordering (Rust-side background actor) and SwiftUI .onChange ordering (MainActor continuation) are independent — Funding A's lock can reach Consumed first while Funding B's controller publishes .completed to SwiftUI first, stamping A's lock with B's recipient hash, and vice versa on A's later .completed. The in-source comment ('FIFO by completion time') asserts an invariant the runtime doesn't actually guarantee, and a single misordered match permanently mis-attributes both rows (the back-fill never reconsiders an already-stamped row). The downstream impact is the storage explorer's audit trail showing fundings going to addresses they didn't go to. Fix at the source: have Rust return the consumed outpoint(s) on the FFI changeset (the orchestrator already has tracked_out_point at fund_with_funding.rs:209), then back-stamp by outpoint instead of by status. Stash recipient.addressType on AddressFundingController while you're there.

suggestion: PendingPlatformFundingsList anti-join is a no-op — same lock can render twice and be resumed while in flight

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)

New at 06c1930. Verified at lines 45-58: the construction of the exclusion set for resumableLocks is gated by inFlight.compactMap { _ in nil } — every entry returns nil, so the set is always empty. The inline comment acknowledges this ('The de-dupe set therefore never has entries today'). The SwiftData status filter (statusRaw ∈ [1,3]) excludes Consumed locks, but for an in-flight controller whose asset lock has just been broadcast, the same outpoint sits in status 2/3 long enough to render in the orphan list while its driving controller is concurrently in the in-flight list. Tapping Resume opens FundPlatformAddressView in resume mode against the same outpoint and the user can pick a different recipient than the original in-flight flow chose; the coordinator's single-flight gate (keyed on (walletId, platformAccountIndex, recipientHash)) doesn't catch it because the recipient hash differs. Two concurrent FFI calls then race against the same asset-lock outpoint. Bounded — Platform's asset-lock-consumption check rejects the second top_up ST so no double-credit — but the user pays full FFI roundtrip cost on a guaranteed-to-fail submission, and the failure surfaces as a generic Platform reject which masks the duplicate-attempt cause. Fix per the comment's own roadmap: have the controller expose its asset-lock outpoint once broadcast and feed it into the exclusion set.

suggestion: Funding success path mutates in-memory balances but never pushes the changeset through the persister

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 205)

Verified at 06c1930 (lines 205-223 unchanged): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount and returns a PlatformAddressChangeSet, but the orchestrator returns immediately after consume_asset_lock without invoking self.persister.store(cs.clone().into()). The transfer path explicitly persists with an inline comment that stale persisted balances would otherwise break initialize_from_persisted and auto_select_inputs on next process start. The Rust crate owns the persister, so the Swift caller cannot compensate from the returned changeset. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local wallet doesn't know — affecting auto-input selection and any UI reading balances from the persisted SwiftData store. The new resumable-orphan surface in this PR doesn't help here: by the time we reach this code path the lock is already consumed, so there's nothing to resume — only the new credit balance is lost from persistence.

suggestion: No direct tests for fund_addresses_with_funding orchestration branches

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)

Verified at 06c1930: submit_with_cl_height_retry has an explicit regression-pinning test and DPP/SDK primitives are exercised elsewhere, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. The most fragile branch is the IS-rejection fallback: it intentionally restarts submit_with_cl_height_retry from the original settings value — because PutSettings is Copy, the bumped value from the first call is discarded, which is required because the proof type changes, the ST hash differs, and Tenderdash's invalid-tx cache no longer applies. A future refactor switching to &mut settings would silently break that retry-cache-bypass invariant. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success, and (d) resume-from-existing-asset-lock would pin the orchestration shape. PR description marks end-to-end testnet validation as still pending.

nitpick: Doc claim of 'shared with the legacy raw-private-key FFI' is inaccurate

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)

Verified at 06c1930 (lines 214-216 unchanged): the doc on decode_funding_addresses claims it is 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' Confirmed false — platform_addresses/fund_from_asset_lock.rs:35-44 still has its own inline for entry in from_raw_parts(...) decode loop and never calls decode_funding_addresses. The two FFI decode paths therefore do not share logic, so any future tightening of one (duplicate rejection, null handling, P2SH boundary check, error wording) will silently diverge across the boundary. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.

nitpick: decode_funding_addresses relies on caller's check_ptr! for null-safety at an FFI trust boundary

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)

Verified at 06c1930 (lines 218-226): the // Safety doc requires non-null addresses, but the body unconditionally calls std::slice::from_raw_parts(addresses, addresses_count). Per the slice contract this is UB on a null data pointer even when count == 0. Today's two FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. But the helper is pub(super) and sits on a C ABI trust boundary consuming caller-supplied raw pointers; a future intra-module caller that omits check_ptr! — or fast-paths addresses_count == 0 on the hunch that 'empty can't matter' — would silently introduce UB the optimizer is free to compile into a memory-safety bug. Sibling helpers in this crate (parse_outputs, parse_explicit_inputs) defensively null-check internally; aligning here costs nothing and removes a sharp edge from a security boundary.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[coderabbitai]

🧹 Nitpick comments (1)

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

2044-2127: 💤 Low value

Consider extracting bech32m helpers to a shared utility.

These ~80 lines of bech32m encoding logic are correct but could be reused elsewhere (e.g., if other views need to display platform addresses). For now this is acceptable as display-only code in an example app, but if platform address rendering spreads to more surfaces, extracting to a shared Bech32m utility would reduce duplication.

🤖 Prompt for AI Agents

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

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`
around lines 2044 - 2127, This code block duplicates bech32m logic and should be
extracted to a reusable utility: create a new Bech32m (or Bech32mUtils)
module/class and move the functions convertBits, bech32mEncode,
bech32mCreateChecksum, bech32mHRPExpand and bech32mPolymod into it as
public/static helpers; update StorageRecordDetailViews to import/use that
utility (call Bech32m.bech32mEncode(...) etc.), keep signatures and behavior
identical, and add a small unit test or usage example to ensure parity after
refactor.

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

Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`:
- Around line 2044-2127: This code block duplicates bech32m logic and should be
extracted to a reusable utility: create a new Bech32m (or Bech32mUtils)
module/class and move the functions convertBits, bech32mEncode,
bech32mCreateChecksum, bech32mHRPExpand and bech32mPolymod into it as
public/static helpers; update StorageRecordDetailViews to import/use that
utility (call Bech32m.bech32mEncode(...) etc.), keep signatures and behavior
identical, and add a small unit test or usage example to ensure parity after
refactor.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c339ff33-ba31-4939-9f70-429cf78450e0

📥 Commits

Reviewing files that changed from the base of the PR and between 9efcf6e and 0a0ba19.

📒 Files selected for processing (7)

• packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs
• packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift

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

• packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift
• packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs

[thepastaclaw]

Code Review

I revalidated the cumulative review at 0753138fffcaffaf02ada11275ff1d9bba5d8b59 against the checked-out source. None of the 10 prior findings were fixed; all remain reproducible at this head, and the AddressFundingProgressView.swift rewrite introduces one additional UI-history regression. To stay within the 10-comment budget, I merged the stale decode_funding_addresses doc-comment nit into the concrete FFI decoder drift finding below.

🟡 9 suggestion(s) | 💬 1 nitpick(s)

10 additional finding(s)

suggestion: Swift preflight still accepts recipient address types that Rust rejects

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)

fundFromCoreAssetLockPreflight checks only non-empty input, the single-nil remainder invariant, and hash.count == 20. Both funding entry points then pass FundingRecipient.addressType through unchanged, but PlatformAddress::try_from in Rust accepts only address_type == 0, and validate_recipient_addresses rejects non-P2PKH recipients as well. That means unsupported recipient types pass the synchronous Swift preflight and fail only after the async FFI setup and Rust call begin, which defeats the point of the preflight guard.

for r in recipients {
    guard r.addressType == 0 else {
        throw PlatformWalletError.invalidParameter(
            "FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
        )
    }
    guard r.hash.count == 20 else {
        throw PlatformWalletError.invalidParameter(
            "FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
        )
    }
}

suggestion: FFI funding-address decode still mutates the caller's recipient set and has already drifted from the legacy entry point

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)

decode_funding_addresses inserts each entry into a BTreeMap and ignores the return value from insert, so duplicate platform addresses are silently overwritten at the ABI boundary. That changes the recipient set after Swift has already chosen the fee strategy and the single None remainder target. The helper's own comment claims this logic is shared with the legacy raw-key FFI path, but platform_addresses/fund_from_asset_lock.rs still has a separate inline decode loop, so the two entry points can keep drifting on duplicate rejection and other validation rules.

suggestion: Progress view still binds to the wrong asset-lock when two address fundings run concurrently on one wallet

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 61)

The @Query is still scoped only by walletId and fundingTypeRaw == 4, and every step computation reads activeLocks.first. AddressFundingController is keyed by (walletId, platformAccountIndex, recipientHash), but PersistentAssetLock still does not carry that discriminator while the funding is in flight. If two address top-ups run concurrently on the same wallet, both progress views observe the same lock set and whichever row updated most recently drives both steppers.

suggestion: Platform-address renderer still hardcodes the testnet HRP

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift (line 2012)

networkHRP() always returns "tdash". The storage view therefore renders mainnet recipient platform addresses with a testnet bech32m prefix, which is not just cosmetic: copying that string back into Rust-side parsing fails on network mismatch. The surrounding comment says the code should derive the HRP from the owning wallet, but this implementation never does.

suggestion: Consumed-lock backfill still races concurrent completions and overwrites the recipient type

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 621)

backfillRecipientOnConsumedLock still selects the newest row matching (walletId, fundingTypeRaw == 4, statusRaw == 4, recipientPlatformAddressHash == nil) and stamps that row with the current controller's recipient hash. If two fundings complete close together on the same wallet, callback ordering can stamp the wrong consumed lock. The method also hardcodes recipientPlatformAddressType = 0, so even if another address type is submitted later the stored audit trail is forced to P2PKH.

suggestion: Pending fundings anti-join is still disabled, so the same lock can be shown twice

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)

The exclusion set is built with inFlight.compactMap { _ in nil }, so it is always empty. That means a lock already owned by an in-flight controller can also appear in the resumable-orphan list while its persisted row is still in statuses 1...3. The user can then trigger a second resume submission against the same outpoint and hit a later duplicate rejection instead of being prevented at the UI layer.

suggestion: Successful address funding still updates only in-memory balances and never persists the changeset

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 201)

After a successful top-up, fund_addresses_with_funding writes the new balances into the managed account and returns the PlatformAddressChangeSet, but it never calls self.persister.store(cs.clone().into()). Other mutating address flows such as transfer persist immediately for exactly this reason. If the process dies before a later sync runs, the platform accepted the top-up but persisted local state still reflects the old balances, which can break restart-time balance display and input selection.

suggestion: `fund_addresses_with_funding` still has no direct orchestration tests

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)

There are still no tests that exercise fund_addresses_with_funding itself. The method owns the recipient preflight, funding resolution, IS-timeout fallback, IS-rejection fallback, CL-height retry wiring, balance writeback, and consume-on-success cleanup. The repository only has a dedicated test for the lower-level submit_with_cl_height_retry helper, so the most failure-prone branches in this new orchestrator remain unpinned.

nitpick: Unsafe FFI decoder still relies on callers to null-check before `from_raw_parts`

packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)

decode_funding_addresses documents that addresses must be non-null, but it immediately calls std::slice::from_raw_parts(addresses, addresses_count) without enforcing that precondition itself. The current extern entry points do call check_ptr!(addresses) first, so this is not an immediate exploit in the shipped API, but the helper is reusable within the module and a future caller can reintroduce UB by forgetting that outer guard. This null check belongs in the unsafe helper itself, not in every caller's memory.

pub(super) unsafe fn decode_funding_addresses(
    addresses: *const FundingAddressEntryFFI,
    addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
    if addresses.is_null() {
        return Err(PlatformWalletFFIResult::err(
            PlatformWalletFFIResultCode::ErrorInvalidParameter,
            "addresses pointer is null",
        ));
    }

    let mut address_map = BTreeMap::new();
    for entry in std::slice::from_raw_parts(addresses, addresses_count) {
        let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
            PlatformWalletFFIResult::err(
                PlatformWalletFFIResultCode::ErrorInvalidParameter,
                format!("invalid platform address: {e}"),
            )
        })?;
        let balance = if entry.has_balance {
            Some(entry.balance)
        } else {
            None
        };
        address_map.insert(addr, balance);
    }
    Ok(address_map)
}

suggestion: The rewritten 5-step progress view now rewrites timed-out InstantSend waits as if they were skipped

packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 176)

step3WasSkipped now returns lock.statusRaw != 2, and stepState uses that once the flow advances past step 3. On the timeout-fallback path, step 3 is actually active for up to instantLockTimeout while the row remains statusRaw == 1; after the fallback succeeds and the row becomes statusRaw == 3, the UI retroactively fades that completed wait as a skipped step. The new implementation therefore loses history for the common IS-timeout path and misrepresents what the user just waited through.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

I revalidated the checked-out worktree at 91b9604d8df3eea213b4159d44f3c6b1f58f0918 against the cited Rust, FFI, and Swift sources. All three prior findings from review 36f3ccb1 are still valid and still present after the rename to fund_from_asset_lock; I did not confirm any additional distinct issue introduced by the latest delta. CodeRabbit provided no inline findings to classify for this run.

🟡 3 suggestion(s)

3 additional finding(s)

suggestion: Asset-lock funding still reports success and consumes the tracked lock after a partial proof result

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs (line 273)

This path now rejects only the wholly empty address_infos case, but it still never verifies that every requested recipient resolved to Some(AddressInfo) before returning success and consuming the tracked asset lock. That matters because the proof helper preserves None entries for requested addresses instead of rejecting them: collect_address_infos_from_proof() only checks key-set equality and then converts each value into Option<AddressInfo> unchanged (packages/rs-sdk/src/platform/transition/address_inputs.rs:51-94). write_address_balances_changeset() then logs and skips both None proof entries and address-pool misses (fund_from_asset_lock.rs:351-396), so this function can return Ok(cs) with missing recipient credits recorded locally while consume_asset_lock() removes the only resumable lock record. That is the same carried-forward correctness hole as the prior review, and the current head still contains it.

let missing_recipients: Vec<String> = addresses
    .keys()
    .filter_map(|address| match address_infos.get(address) {
        Some(Some(_)) => None,
        _ => Some(format!("{address:?}")),
    })
    .collect();

if !missing_recipients.is_empty() {
    return Err(PlatformWalletError::AddressSync(format!(
        "Address-funding ST succeeded but the proof omitted usable AddressInfo for recipient(s): {}",
        missing_recipients.join(", ")
    )));
}

let cs = self
    .write_address_balances_changeset(platform_account_index, &address_infos)
    .await;

suggestion: Successful asset-lock funding still updates balances in memory without persisting the changeset

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs (line 283)

After a successful submit, this flow mutates the managed platform account via write_address_balances_changeset() and returns Ok(cs), but unlike the sibling mutating flows in platform_addresses/transfer.rs and platform_addresses/sync.rs, it never sends the non-empty changeset through self.persister.store(...). There is no compensating persistence in the Swift caller; the Swift side only decodes the returned FFI changeset into transient UpdatedBalance values and frees it. A restart after a successful funding can therefore reload stale persisted platform-address balances even though Platform already accepted the transition and the asset lock may already be marked consumed. This is the same carried-forward restart-consistency gap as the prior review, and it remains present on the current head.

let cs = self
    .write_address_balances_changeset(platform_account_index, &address_infos)
    .await;

if !cs.is_empty() {
    if let Err(e) = self.persister.store(cs.clone().into()) {
        tracing::error!("Failed to persist fund-from-asset-lock changeset: {}", e);
    }
}

suggestion: Swift fund-from-asset-lock preflight still accepts recipient address types that the Rust FFI rejects

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)

fundFromAssetLockPreflight() validates only emptiness, the single-remainder invariant, and hash.count == 20; it never validates FundFromAssetLockRecipient.addressType. At the same time, the public Swift API still documents 0 = P2PKH, 1 = P2SH (ManagedPlatformAddressWallet.swift:381-383), while the Rust FFI decoder accepts only address_type == 0 and returns Unsupported address type for everything else (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-42). The result is that unsupported recipient types still clear the synchronous Swift preflight and fail only later across FFI, which defeats the point of the preflight guard. This is the same carried-forward API-contract mismatch as the prior review.

for r in recipients {
    guard r.addressType == 0 else {
        throw PlatformWalletError.invalidParameter(
            "FundFromAssetLockRecipient.addressType must be 0 (P2PKH) for platform-address funding"
        )
    }
    guard r.hash.count == 20 else {
        throw PlatformWalletError.invalidParameter(
            "FundFromAssetLockRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
        )
    }
}

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

Verified against the checked-out 9bbdc6676083664e98e3c74bb5c09aa786cb3166 worktree. The latest incremental delta from 91b9604d8df3eea213b4159d44f3c6b1f58f0918 is minimal and does not add a new defect, but all three prior findings are still valid and should be carried forward: one partial-proof handling bug in the Rust asset-lock funding path, one missing persistence step in that same success path, and one Swift preflight/FFI contract mismatch.

🟡 3 suggestion(s)

3 additional finding(s)

suggestion: Asset-lock funding still treats partial proved recipient data as success and can consume the tracked lock anyway

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs (line 273)

fund_from_asset_lock() only rejects the fully empty address_infos result before updating balances and proceeding to consume_asset_lock(). That is not sufficient, because collect_address_infos_from_proof() preserves None values for requested addresses as long as the key set matches, and write_address_balances_changeset() then logs and skips those None entries instead of failing. The result is a success path where one or more requested recipients can be omitted from the proved response, local bookkeeping stays incomplete, the FFI still reports success to Swift, and the tracked asset lock can be marked consumed. That destroys the wallet's resumable record even though not every requested credit was confirmed back through the proved result.

let expected_recipient_count = addresses.len();
if address_infos.is_empty() {
    return Err(PlatformWalletError::AddressSync(format!(
        "Address-funding ST succeeded but the proof returned no address infos \
         (expected {} recipient(s)); refusing to consume the asset lock with \
         no recorded credits",
        expected_recipient_count
    )));
}

let missing_recipients: Vec<String> = addresses
    .keys()
    .filter_map(|address| match address_infos.get(address) {
        Some(Some(_)) => None,
        _ => Some(format!("{address:?}")),
    })
    .collect();

if !missing_recipients.is_empty() {
    return Err(PlatformWalletError::AddressSync(format!(
        "Address-funding ST succeeded but the proof omitted usable AddressInfo for recipient(s): {}",
        missing_recipients.join(", ")
    )));
}

let cs = self
    .write_address_balances_changeset(platform_account_index, &address_infos)
    .await;

if let Some(out_point) = tracked_out_point {
    if let Err(e) = self.asset_locks.consume_asset_lock(&out_point).await {
        match &e {
            PlatformWalletError::WalletNotFound(_) => {
                tracing::warn!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock: wallet handle vanished after successful Platform submit"
                );
            }
            _ => {
                tracing::error!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock failed unexpectedly after successful Platform submit; \
                     the lock row stays non-Consumed and will surface as Resumable. \
                     A user Resume on it will be rejected by Platform with 'lock already consumed'."
                );
            }
        }
    }
}

Ok(cs)

suggestion: Successful asset-lock funding still mutates wallet balances without persisting the changeset

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs (line 283)

After write_address_balances_changeset() runs, this path returns the in-memory PlatformAddressChangeSet and may consume the asset lock, but it never calls self.persister.store(...). Sibling mutating flows such as platform_addresses/transfer.rs and platform_addresses/sync.rs do persist their changesets. The Swift side only decodes the returned FFI payload and does not compensate for persistence. A restart after a successful top-up can therefore reload stale platform-address balances from storage even though Platform accepted the transition and the local asset-lock record may already be consumed.

let cs = self
    .write_address_balances_changeset(platform_account_index, &address_infos)
    .await;

if !cs.is_empty() {
    if let Err(e) = self.persister.store(cs.clone().into()) {
        tracing::error!("Failed to persist fund-from-asset-lock changeset: {}", e);
    }
}

if let Some(out_point) = tracked_out_point {
    if let Err(e) = self.asset_locks.consume_asset_lock(&out_point).await {
        match &e {
            PlatformWalletError::WalletNotFound(_) => {
                tracing::warn!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock: wallet handle vanished after successful Platform submit"
                );
            }
            _ => {
                tracing::error!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock failed unexpectedly after successful Platform submit; \
                     the lock row stays non-Consumed and will surface as Resumable. \
                     A user Resume on it will be rejected by Platform with 'lock already consumed'."
                );
            }
        }
    }
}

Ok(cs)

suggestion: Swift preflight still accepts recipient address types that the Rust FFI rejects

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)

fundFromAssetLockPreflight() validates only emptiness, the single-remainder invariant, and 20-byte hashes. Meanwhile the public Swift type still documents addressType as supporting both 0 = P2PKH and 1 = P2SH, but the Rust FFI decoder accepts only address_type == 0 and returns Unsupported address type for everything else. That means unsupported recipients pass the synchronous Swift preflight and fail only after crossing the FFI boundary, which breaks the documented contract of the preflight guard.

for r in recipients {
    guard r.addressType == 0 else {
        throw PlatformWalletError.invalidParameter(
            "FundFromAssetLockRecipient.addressType must be 0 (P2PKH) for platform-address funding"
        )
    }
    guard r.hash.count == 20 else {
        throw PlatformWalletError.invalidParameter(
            "FundFromAssetLockRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
        )
    }
}

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

I verified the review set against the checked-out e94ff19748c2007e16020c8f9e05f0ca64bee17f worktree and the pinned dash-spv dependency revision from Cargo.lock. All three prior wallet/Swift findings from the earlier review are still valid in the current head, and there is one additional new issue in the latest SPV runtime delta. No CodeRabbit findings were provided, so there was nothing to reconcile there.

🟡 4 suggestion(s)

4 additional finding(s)

suggestion: Asset-lock funding still reports success when the proof omits a requested recipient

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs (line 265)

This path only rejects the fully empty address_infos case before it writes balances and consumes the tracked asset lock. That is not sufficient because collect_address_infos_from_proof() accepts the expected key set even when some values are None (packages/rs-sdk/src/platform/transition/address_inputs.rs:51-96), and write_address_balances_changeset() then logs and skips those None entries at lines 356-367 instead of failing. The result is a silent partial-success path: the wallet returns Ok(cs) and can mark the asset lock consumed even though at least one requested recipient was never confirmed or written, which destroys the only resumable record for that missing credit.

let expected_recipient_count = addresses.len();
if address_infos.is_empty() {
    return Err(PlatformWalletError::AddressSync(format!(
        "Address-funding ST succeeded but the proof returned no address infos \
         (expected {} recipient(s)); refusing to consume the asset lock with \
         no recorded credits",
        expected_recipient_count
    )));
}

let missing_recipients: Vec<String> = addresses
    .keys()
    .filter_map(|address| match address_infos.get(address) {
        Some(Some(_)) => None,
        _ => Some(format!("{address:?}")),
    })
    .collect();

if !missing_recipients.is_empty() {
    return Err(PlatformWalletError::AddressSync(format!(
        "Address-funding ST succeeded but the proof omitted usable AddressInfo for recipient(s): {}",
        missing_recipients.join(", ")
    )));
}

let cs = self
    .write_address_balances_changeset(platform_account_index, &address_infos)
    .await;

suggestion: Successful asset-lock funding updates balances in memory but never persists the changeset

packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs (line 283)

After write_address_balances_changeset() mutates the managed platform account, this flow returns the PlatformAddressChangeSet and may consume the tracked asset lock without ever calling self.persister.store(...). Sibling mutating flows in the same module do persist their balance changes (transfer.rs:146-159, sync.rs:76-82). That leaves durable wallet state behind the in-memory state: after a restart, storage can reload stale platform-address balances even though Platform already accepted the top-up and the wallet may already have consumed the local asset-lock record.

let cs = self
    .write_address_balances_changeset(platform_account_index, &address_infos)
    .await;

if !cs.is_empty() {
    if let Err(e) = self.persister.store(cs.clone().into()) {
        tracing::error!("Failed to persist fund-from-asset-lock changeset: {}", e);
    }
}

if let Some(out_point) = tracked_out_point {
    if let Err(e) = self.asset_locks.consume_asset_lock(&out_point).await {
        match &e {
            PlatformWalletError::WalletNotFound(_) => {
                tracing::warn!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock: wallet handle vanished after successful Platform submit"
                );
            }
            _ => {
                tracing::error!(
                    outpoint = %out_point,
                    error = %e,
                    "consume_asset_lock failed unexpectedly after successful Platform submit; \
                     the lock row stays non-Consumed and will surface as Resumable. \
                     A user Resume on it will be rejected by Platform with 'lock already consumed'."
                );
            }
        }
    }
}

Ok(cs)

suggestion: Swift preflight advertises P2SH recipients even though the Rust FFI rejects them

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 381)

FundFromAssetLockRecipient documents addressType as 0 = P2PKH, 1 = P2SH, and fundFromAssetLockPreflight() only checks emptiness, the single remainder recipient, and 20-byte hashes. The Rust FFI decoder still accepts only address_type == 0 and returns Unsupported address type for anything else (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-43). That leaves the public Swift contract broader than the implementation behind it: unsupported recipients pass synchronous Swift validation and fail only after crossing the FFI boundary.

for r in recipients {
    guard r.addressType == 0 else {
        throw PlatformWalletError.invalidParameter(
            "FundFromAssetLockRecipient.addressType must be 0 (P2PKH) for platform-address funding"
        )
    }
    guard r.hash.count == 20 else {
        throw PlatformWalletError.invalidParameter(
            "FundFromAssetLockRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
        )
    }
}

suggestion: SpvRuntime holds the client read lock across `client.run()`, so `stop()` cannot shut it down

packages/rs-platform-wallet/src/spv/runtime.rs (line 138)

run() acquires self.client.read().await and keeps that guard alive across client.run().await. stop() needs self.client.write().await before it can call c.stop(). That lock ordering blocks shutdown of a running client. The problem is real in the pinned dependency: DashSpvClient::run() is explicitly documented and implemented as a loop that runs until stop() flips its running state, then it calls self.stop() before returning (dash-spv/src/client/sync_coordinator.rs:27-150), while DashSpvClient::stop() is the method that performs that state flip (dash-spv/src/client/lifecycle.rs:190-220). Holding the outer RwLockReadGuard for the full await therefore prevents SpvRuntime::stop() from ever reaching the inner stop call.

let client = {
    let client_guard = self.client.read().await;
    client_guard
        .as_ref()
        .cloned()
        .ok_or(PlatformWalletError::SpvNotRunning)?
};

let result = client
    .run()
    .await
    .map_err(|e| PlatformWalletError::SpvError(e.to_string()));

let mut client = self.client.write().await;
let _ = client.take();

result

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

Cumulative incremental verification at 2dbcc2a (delta from e94ff19 is a cargo-fmt module reordering plus a v3.1-dev merge bringing additive FFI error variants — no behavioral changes in the review-critical paths). All four prior findings are STILL VALID and carried forward: (1) fund_from_asset_lock only rejects the wholly-empty address_infos case and silently skips per-recipient None proof entries while still consuming the tracked asset lock; (2) the same success path mutates in-memory account state without ever calling persister.store(...); (3) the Swift fundFromAssetLockPreflight still omits the addressType==0 check that the Rust FFI decoder requires; (4) SpvRuntime::run holds the client read lock across client.run().await, deadlocking against stop()'s write-lock acquisition. No new defects in this delta. All findings are suggestions, so review_action is COMMENT.

🟡 4 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 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/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:273-326: Asset-lock funding still reports success and consumes the tracked lock on partial proof results
  The post-submit guard at lines 273-281 only rejects the wholly-empty `address_infos.is_empty()` case before falling through to `write_address_balances_changeset()` and `consume_asset_lock()` at line 304. `write_address_balances_changeset()` at lines 351-397 logs and `continue`s past both per-recipient `None` proof entries and address-pool misses — it never propagates the failure. Combined with `collect_address_infos_from_proof()` (in `packages/rs-sdk/src/platform/transition/address_inputs.rs`), which preserves `None` values for requested recipients as long as the key set matches, this means `fund_from_asset_lock` can return `Ok(cs)` with one or more requested recipients silently dropped while `consume_asset_lock()` terminally destroys the only resumable record for that L1 funding outpoint. The function's own comment at 265-272 already characterizes the case as a 'DAPI / proof-verifier contract violation, NOT a successful zero-credit funding'; the empty-map guard should extend to any `None` or pool-miss entry. Today's only in-tree caller is single-recipient so the practical exposure is bounded, but the public signature accepts arbitrary multi-recipient maps and asset locks represent non-refundable L1 funds.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:283-326: Successful asset-lock funding mutates wallet balances without persisting the changeset
  After `write_address_balances_changeset()` mutates the in-memory `ManagedPlatformAccount`, this flow returns `Ok(cs)` and may proceed to `consume_asset_lock()` without ever calling `self.persister.store(cs.clone().into())`. Sibling mutating flows in the same module do persist their non-empty balance changesets (`platform_addresses/transfer.rs` and `platform_addresses/sync.rs`). The Swift caller does not compensate — `ManagedPlatformAddressWallet.swift` only decodes the FFI changeset into transient `UpdatedBalance` values and frees it (lines 488/585), and per the swift-sdk CLAUDE.md the Swift layer must not re-persist what Rust has already mutated; persistence is owned by the Rust persister callback. Consequently, after a successful top-up the durable wallet state lags the in-memory state until the next BLAST sync writes a row over the top. On app restart before that sync, `initialize_from_persisted` will seed `account.address_credit_balance` from the stale pre-top-up persisted row, while the asset-lock record may already be marked `Consumed`. Subsequent `auto_select_inputs` will under-budget and produce protocol-level rejections until a sync repairs the rows.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:594-613: Swift fund-from-asset-lock preflight still accepts recipient address types the Rust FFI rejects
  `fundFromAssetLockPreflight()` validates only `recipients.isEmpty`, the single-remainder invariant, and `r.hash.count == 20`. It never inspects `FundFromAssetLockRecipient.addressType`, even though the type at lines 381-394 documents the field as `0 = P2PKH, 1 = P2SH`, and both `fundFromAssetLock` and `resumeFundFromAssetLock` marshal `r.addressType` straight into `PlatformAddressFFI`. On the Rust side, `impl TryFrom<PlatformAddressFFI> for PlatformAddress` (`packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-45`) accepts only `address_type == 0` and returns `Err("Unsupported address type")` for anything else, including the `1 = P2SH` discriminant the Swift type still advertises. A caller following the documented Swift API and passing `addressType: 1` clears the synchronous preflight, pays for the `Task.detached` setup and signer pinning, and only then receives a generic FFI error — defeating the preflight's stated purpose of producing a synchronous, type-specific Swift error before the FFI roundtrip. The same gap applies to `resumeFundFromAssetLock` because it shares the recipient marshaling.

In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:132-153: SpvRuntime holds the client read lock across `client.run()`, blocking `stop()` from shutting it down
  `SpvRuntime::run()` acquires `self.client.read().await` at line 138 and binds `client = client_guard.as_ref().ok_or(...)?` at 139-141, keeping that `RwLockReadGuard` alive across the long-running `client.run().await` at lines 143-146 (the explicit `drop(client_guard)` at line 148 only fires after `run()` returns). `SpvRuntime::stop()` at lines 156-164 must acquire `self.client.write().await` before it can call `c.stop()`. With tokio's `RwLock`, write acquisitions block on outstanding readers, so `stop()` cannot make progress while `run()` is mid-loop — and the pinned `DashSpvClient::run()` is implemented as a loop that exits only when `stop()` flips its running flag, so the loop never terminates on its own. The result is a graceful-shutdown deadlock reachable from any caller (including FFI/Swift teardown paths). Because the inner Option<SpvClient> is not currently stored as Arc<SpvClient>, the cleanest fix is either to change the storage to Arc/clone the handle out before awaiting, or to drive shutdown via a side-channel signal (e.g. a `Notify` or `CancellationToken`) so `stop()` does not need the write lock to take effect.

[thepastaclaw]

Code Review

Cumulative incremental verification at 8d40d8c. The delta from 2dbcc2a is purely additive test code (a new signing_tests module for AddressFundingFromAssetLockTransition plus its mod declaration). No production code changed. All four prior findings reproduce verbatim at this head and are carried forward; no new defects introduced by the delta. All carried-forward findings are suggestion-level, so the review action is COMMENT.

🟡 3 suggestion(s) | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 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/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:273-326: Asset-lock funding reports success and consumes the tracked lock on partial proof results
  Carried forward from 2dbcc2a5 — unchanged at HEAD 8d40d8ce. Verified: the post-submit guard at lines 273-281 only rejects the wholly-empty `address_infos.is_empty()` case before falling through to `write_address_balances_changeset()` and `consume_asset_lock()` at line 304. `write_address_balances_changeset()` at lines 351-397 logs and `continue`s past both per-recipient `None` proof entries (362-368) and address-pool misses (380-397) — it never propagates failure. Combined with `collect_address_infos_from_proof()` (`packages/rs-sdk/src/platform/transition/address_inputs.rs`), which preserves `None` values for requested recipients as long as the key set matches, `fund_from_asset_lock` can return `Ok(cs)` with one or more requested recipients silently dropped while `consume_asset_lock()` terminally destroys the only resumable record for that L1 funding outpoint. The function's own comment at 265-272 already characterizes this as a 'DAPI / proof-verifier contract violation, NOT a successful zero-credit funding'; the empty-map guard should extend to any `None` or pool-miss entry. The current in-tree caller is single-recipient so practical exposure is bounded, but the public signature accepts arbitrary multi-recipient maps and asset locks represent non-refundable L1 funds. Convergent finding from both Claude and Codex passes (4 agents each).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:283-326: Successful asset-lock funding mutates wallet balances without persisting the changeset
  Carried forward from 2dbcc2a5 — unchanged at HEAD 8d40d8ce. Verified: after `write_address_balances_changeset()` mutates the in-memory `ManagedPlatformAccount` (`account.set_address_credit_balance(...)` at line 373), this flow returns `Ok(cs)` at line 326 and proceeds through `consume_asset_lock()` (304) without ever calling `self.persister.store(cs.clone().into())`. Sibling mutating flows do persist their non-empty balance changesets (`platform_addresses/transfer.rs` and `platform_addresses/sync.rs`). Per `packages/swift-sdk/CLAUDE.md`, the Swift layer must not re-persist what Rust has already mutated — persistence is owned by the Rust persister callback — so there's no compensating Swift write. Consequently, after a successful top-up the durable wallet state lags the in-memory state until the next BLAST sync writes a row over the top. On app restart before that sync, `initialize_from_persisted` will seed `account.address_credit_balance` from the stale pre-top-up persisted row while the asset-lock record may already be marked `Consumed`. Subsequent `auto_select_inputs` will under-budget and produce protocol-level rejections until a sync repairs the rows. The persist must happen before `consume_asset_lock` so we never have a Consumed lock paired with a stale balance row. Convergent finding from both Claude and Codex passes.

In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:132-164: SpvRuntime holds the client read lock across `client.run()`, deadlocking `stop()`
  Carried forward from 2dbcc2a5 — unchanged at HEAD 8d40d8ce. Verified: `SpvRuntime::run()` acquires `self.client.read().await` at line 138 and binds `client = client_guard.as_ref().ok_or(...)?` at 139-141, keeping that `RwLockReadGuard` alive across the long-running `client.run().await` at lines 143-146 (the explicit `drop(client_guard)` at line 148 only fires after `run()` returns). `SpvRuntime::stop()` at lines 156-164 must acquire `self.client.write().await` before it can call `c.stop()`. With tokio's `RwLock`, write acquisitions block on outstanding readers, so `stop()` cannot make progress while `run()` is mid-loop — and `DashSpvClient::run()` is a loop that exits only when `stop()` flips its running flag, so the loop never terminates on its own. This is a graceful-shutdown deadlock reachable from any caller, including the FFI/Swift teardown path that relies on `spawn_in_background` at 169-176. Security/availability implications: shutdown DoS, leaked file descriptors / network sockets / locked RocksDB instances. The cleanest fixes are to store the inner client as `Arc<SpvClient>` and clone the handle before awaiting, or to drive shutdown via a side-channel signal (`Notify` / `CancellationToken`) so `stop()` doesn't need the write lock to take effect. Convergent finding from both Claude and Codex passes.

[thepastaclaw]

Code Review

Cumulative incremental review at d8c3ce7. Three of four prior findings are resolved by the new commits: partial-proof rejection via validate_address_infos_complete, persister.store called before consume_asset_lock, and Swift addressType==0 preflight guard. The SpvRuntime read-lock-across-run() deadlock is unchanged and carried forward (both reviewers, 0.97). Codex flags one new state-divergence in the very persistence path this PR introduces: write_address_balances_changeset mutates the in-memory account balance before the address-index lookup, so when the lookup misses (pool changed between preflight and post-submit) the in-memory write happens but no changeset entry is pushed, leaving persister.store() unable to record that recipient. Two suggestion-severity findings; no blockers — review_action is COMMENT.

🟡 2 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 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/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:138-164: SpvRuntime holds the client read lock across `client.run()`, deadlocking `stop()`
  Verified at HEAD d8c3ce74 (file untouched in this delta). `SpvRuntime::run()` acquires `self.client.read().await` at line 138, binds `client = client_guard.as_ref().ok_or(...)?` at 139-141, and keeps that `RwLockReadGuard` alive across the long-running `client.run().await` at 143-146. The explicit `drop(client_guard)` at line 148 only fires after `run()` returns. `SpvRuntime::stop()` at 156-164 must acquire `self.client.write().await` before it can call `c.stop()`. With tokio's `RwLock`, write acquisitions block on outstanding readers, so `stop()` cannot make progress while `run()` is mid-loop — and `DashSpvClient::run()` is itself a loop that exits only after its internal running flag is flipped, which `stop()` cannot reach. Graceful-shutdown deadlock reachable from any caller, including the FFI/Swift teardown path that relies on `spawn_in_background` at 169-176. Implications: shutdown DoS, leaked file descriptors / sockets / RocksDB locks on the SPV side; on iOS/macOS a SIGKILL workaround can leave inconsistent on-disk state across launches. Convergent finding from Claude and Codex; persisting across multiple reviews unaddressed. Cleanest fixes: store the inner client as `Arc<DashSpvClient>` and clone the handle out of the read guard before awaiting, or drive shutdown via a side-channel signal (`tokio::sync::Notify` / `CancellationToken`) so `stop()` doesn't need the write lock during the run loop.

In `packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:392-420: Address-pool miss branch mutates the in-memory balance but omits the changeset entry, defeating the new persist-before-consume invariant
  In `write_address_balances_changeset` the recipient loop calls `account.set_address_credit_balance(p2pkh, funds.balance, key_source.as_ref())` at line 396 BEFORE attempting to recover `address_index` at lines 403-413. When the address pool no longer contains the recipient (the documented "pool mutated between preflight and post-submit" race), the `else { continue; }` branch logs and skips, but the in-memory `ManagedPlatformAccount` has already been mutated while no `PlatformAddressBalanceEntry` is pushed onto `cs.addresses`. The caller at lines 299-308 then runs `self.persister.store(cs.clone().into())`, which never persists this recipient. The net result is exactly the divergence this PR's persist-before-consume change is trying to eliminate: in-memory balance updated, durable row stale, asset lock subsequently consumed. Reorder so the address-index lookup precedes the balance mutation — if the lookup misses, the `continue` runs before any in-memory write, and the recipient is dropped consistently from both stores until the next BLAST sync repairs them.

[thepastaclaw]

Code Review

Incremental review at head 3619d2d. Prior finding #2 (write_address_balances_changeset persist-before-consume) is FIXED by the latest commit, which moves account.set_address_credit_balance after the address_index lookup and skips on pool miss without mutating in-memory state. Prior finding #1 (SpvRuntime read lock held across client.run().await, deadlocking stop()) remains STILL VALID — runtime.rs is unchanged in the latest delta. No new defects identified in the latest delta itself.

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 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/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:138-164: [Carried forward from d8c3ce74 review — STILL VALID] SpvRuntime holds the client read lock across `client.run().await`, deadlocking `stop()`
  Re-verified at HEAD 3619d2da; this file is unchanged in the latest delta and the issue persists. In `SpvRuntime::run()` (lines 132-153), a `RwLockReadGuard` is acquired at line 138 (`let client_guard = self.client.read().await`), `client` is borrowed from it at 139-141, and the guard is held alive across the long-running `client.run().await` at 143-146 — the explicit `drop(client_guard)` at line 148 only executes after `run()` returns. `SpvRuntime::stop()` at 156-164 must acquire `self.client.write().await` before it can call `c.stop().await`. With Tokio's `RwLock`, a write acquisition blocks on outstanding readers (and queued writers also block new readers under write-preferring fairness), so `stop()` cannot make progress while the sync loop is running. Because `DashSpvClient::run()` exits only after its own internal stop flag flips, and only `stop()` can flip it, this is a true graceful-shutdown deadlock reachable from any teardown path (process exit, FFI/Swift `SpvRuntime` destruction, test cleanup). Practical impact: shutdown DoS and leaked sockets, file descriptors, and RocksDB locks on the SPV side. Fix: clone an `Arc<DashSpvClient>` out from under the read guard before awaiting `run()`, or drive shutdown through a side channel (`tokio::sync::Notify` / `CancellationToken`) so `stop()` does not contend for the write lock during the run loop.