test(swift-sdk): cover imported-wallet history either side of registration

[ZocoLini]

Issue being fixed or feature implemented

SpvLateWalletBackfillIntegrationTests covers importing a single-transaction wallet into an SPV client that has already reached the chain tip. Nothing covered the harder shape: a wallet with many transactions across many addresses imported into a client that is still running and still taking blocks, where part of the history can only come back through the register-time rescan and part arrives live.

What was done?

Adds SpvManyTxWalletImportHistoryIntegrationTests. The funding straddles the registration so each recovery path is exercised by a half the other path provably cannot reach:

• Pre-registration half — 5 funding transactions (one per address, one per block) are mined, then the empty SPV client is started and synced to the tip. Script matching runs against the registered wallets' scripts, so nothing could have matched these at the time; they can only return through the genesis rescan that registering a birthHeight: 0 wallet starts.
• Post-registration half — 5 more are mined after createWallet returns, above the ceiling that rescan swept, so only live filter matching against the now-registered wallet can find them.

Neither half depends on timing: the test reaches the tip before it registers, and registers before it funds again.

Both the aggregate balance and the persisted per-transaction history are asserted, once after the rescan and again after the live half, so a failure names the path that broke. They are separate write paths — the balance atomic on one side, PersistentTransaction rows written by PlatformWalletPersistenceHandler on the other — so a regression that restores the right total while dropping individual transaction records is caught instead of passing silently.

Supporting details:

• Addresses come from a single managed_wallet_get_bip_44_external_address_range call, which generates them lazily Rust-side regardless of the gap limit — no Swift-side derivation or gap-limit walking.
• The FFIWallet that wallet_manager_get_wallet returns is a fresh box on every call, not a borrow of the manager's, so it is released with wallet_free_const.
• Funding goes through env.fund, which broadcasts to the masternodes and waits for the InstantSend lock before mining, matching the rest of the suite.
• The per-address amount is held in duffs and the DASH figure derived from it, so no UInt64(dash * 1e8) truncation.

How Has This Been Tested?

Test-only change; no production code is touched. Compiles clean (swift build --build-tests, no warnings) against v4.2-dev.

The integration suite itself needs a local dashmate devnet and is not part of CI — run it with:

packages/swift-sdk/run_integration_tests.sh --filter SpvManyTxWalletImportHistoryIntegrationTests

Breaking Changes

None.

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a Swift integration test that funds ten wallet addresses before and during SPV synchronization, imports the wallet with birthHeight: 0, and verifies the aggregate balance and persisted funding transaction IDs. It also adds FFI address enumeration and error cleanup helpers.

Changes

SPV mid-sync backfill

Layer / File(s)	Summary
Address derivation and FFI error handling packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift	Enumerates external BIP-44 addresses through FFI and releases managed resources and error-message pointers.
Funding transaction normalization packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift	Funds addresses, validates returned transaction IDs, and converts IDs to persisted wire order.
Mid-sync wallet history backfill test packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift	Funds five addresses before SPV synchronization and five during synchronization, imports the wallet from genesis, and asserts balance and transaction history.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: llbartekll, quantumexplorer, shumkov

Sequence Diagram(s)

sequenceDiagram
  participant IntegrationTest
  participant WalletManager
  participant SPVClient
  participant ImportedWallet

  IntegrationTest->>WalletManager: Derive addresses and fund five historical outputs
  IntegrationTest->>SPVClient: Start synchronization
  IntegrationTest->>WalletManager: Fund five live outputs
  IntegrationTest->>SPVClient: Import wallet with birthHeight 0
  SPVClient->>ImportedWallet: Synchronize wallet history
  ImportedWallet-->>IntegrationTest: Return balance and transaction history

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the Swift SDK integration test for imported-wallet history before and after registration.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/mid-flight-wallet

---

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit b0eafe7)

[coderabbitai]

Actionable comments posted: 2

🤖 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/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`:
- Line 13: Replace the floating-point amountEachDuffs calculation with an
integer Duffs constant or integer-based representation, and update
amountEachDash usage as needed so values such as 0.0001 map exactly to 10000
Duffs without floating-point multiplication or truncation.
- Around line 129-136: Add a post-loop assertion after the address collection
loop in the relevant address-derivation helper, verifying that result.count
equals outCount and reporting a clear failure message if not, before any
subsequent funding-loop indexing occurs.

🪄 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: c7cebbd2-68e1-46bf-817f-02d30eaf29db

📥 Commits

Reviewing files that changed from the base of the PR and between f7d7c8d and e40cd58.

📒 Files selected for processing (1)

• packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift

[thepastaclaw]

Code Review

Source: Sol orchestrator openai/gpt-5.6-sol (high, orchestration-only); reviewers: Sol gpt-5.6-sol and Sonnet claude-sonnet-5 for general, security-auditor, and ffi-engineer; verifier: Sonnet claude-sonnet-5; Opus: not sampled (bucket 3).

This PR adds a single new Swift integration test (SpvManyTxMidSyncBackfillIntegrationTests.swift, +148 lines, no other files touched — confirmed against parent commit b42d213 at head e40cd58). The test's core FFI usage for bulk address derivation is correct, but it leaks a Rust-owned FFIWallet handle behind an incorrect ownership comment, bypasses the suite's InstantSend-aware funding helper, doesn't actually guarantee the mid-sync precondition it's named for, and only checks aggregate balance rather than full transaction history despite the test name promising 'finds all tx'.

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

🤖 Prompt for all review comments with AI agents

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

In `packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift:101-110: Leaked FFIWallet from wallet_manager_get_wallet; ownership comment is factually wrong
  I checked the pinned key-wallet-ffi source directly (rev 647fa9820f3614090e4e5f5f2b709961d68e538b, cached at ~/.cargo/git/checkouts/rust-dashcore-*/647fa98/key-wallet-ffi/src/wallet_manager.rs:371-388). `wallet_manager_get_wallet` clones the wallet from the manager (`.cloned()`) and returns a brand-new heap allocation every call: `Box::into_raw(Box::new(FFIWallet::new(wallet)))`. Its doc comment is explicit: 'The returned wallet must be freed with wallet_free_const()', and `wallet_free_const` (wallet.rs:278-279) names this exact function by name as a caller of it. The manager does NOT retain ownership of this specific pointer.

  The comment on line 101, 'Non-owning: the manager retains ownership of the wallet handle,' is incorrect, and there is no `defer { wallet_free_const(wallet) }` anywhere in `deriveExternalAddresses`. Every test run leaks one boxed `FFIWallet`. The same incorrect assumption already exists in production `WalletManager.swift` (confirmed: `wallet_free_const` is never called there despite 3 call sites of `wallet_manager_get_wallet`), so this is a pre-existing pattern rather than something this PR invented — but this PR adds a new call site with an incorrect rationale restated verbatim, and it's easy to fix here.
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift:36-43: "~20% synced" precondition is not actually guaranteed — it depends on prior tests' chain growth, not this test's own state
  I traced the harness: `IntegrationTestEnv.bootstrap()` (IntegrationTestEnv.swift:113-165) runs exactly once per test process, syncs the SPV data dir to the chain tip at that moment via `createSpvCache()`, then snapshots it with `snapshotSpvCache()`. Every subsequent test's `tearDown` calls `resetState()` → `restoreSpvCacheFromSnapshot()` (lines 78-82, 223-241), which restores the working SPV dir to that *same* pre-synced snapshot before the next test runs. Meanwhile the underlying regtest node keeps mining blocks across the whole suite — it is never reset.

  So when this test calls `waitUntilUpToDate(height: tipHeight / 5)`, whether that wait does any real syncing work is entirely a function of how far the live chain has grown past the bootstrap-time snapshot height (i.e., how many blocks earlier tests in the same run happened to mine), not anything this test controls or asserts. `waitUntilUpToDate` (TestWallet.swift:69-89) returns as soon as cached headers/filters already meet the target — if the snapshot's cached height already exceeds `tipHeight/5` (plausible after any funding-heavy tests ran first), this reduces to importing into an already-synced client, silently testing the same late-import scenario `SpvLateWalletBackfillIntegrationTests` already covers instead of a genuine mid-flight race. Nothing in the test verifies headers/filters are actually below the current chain tip before `createWallet` runs.
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift:55-69: Test name promises 'finds all tx' but only aggregate balance is asserted, not transaction history
  The test method is `testManyTxImportedMidSyncBackfillsAllHistory` and the PR title says 'finds all tx', but the only assertion (lines 65-69) checks `imported.balance().total == expectedTotal`. Balance/UTXO tracking and transaction-history persistence are separate write paths — `PlatformWalletPersistenceHandler` independently writes rows into `PersistentTransaction`. A regression that restores the correct aggregate balance while dropping one or more of the 10 individual transaction records from history would pass this test undetected, since the 10 funding txids are never captured or checked against the imported wallet's history.
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift:26-32: Funding loop bypasses `env.fund()`'s InstantSend/masternode-broadcast steps
  I confirmed `env.fund(address:dash:)` (IntegrationTestEnv.swift:391-398) does `sendToAddress` → `broadcastToMasternodes(txid:)` → `waitForInstantSendLock(txid:)` → `mine(1)`, and every other test in this suite (`SpvLateWalletBackfillIntegrationTests`, `CoreSendIntegrationTests`, `SpvRestartIntegrationTests`, etc.) uses this helper. This new test instead calls `env.coreRPC.sendToAddress` directly and immediately `env.mine(1)`, skipping the masternode broadcast and InstantSend-lock wait. Since this test funds 10 separate addresses across 10 blocks in quick succession, it's the test most likely to expose the absence of that synchronization the rest of the suite already guards against, making it a plausible source of CI flakiness.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[thepastaclaw]

Final validation — Codex + Sonnet

This PR is a single new Swift integration test file (SpvManyTxMidSyncBackfillIntegrationTests.swift), rewritten wholesale in the latest delta (e40cd58..17f2352). I revalidated all 6 carried-forward prior findings against the exact head: 5 are FIXED (owned-FFIWallet leak now paired with defer { wallet_free_const(wallet) } and a corrected ownership comment; per-transaction history is now asserted via readTxids() alongside balance; funding goes through the new fund() helper which delegates to env.fund preserving InstantSend/masternode-broadcast; the truncated comment was replaced with a complete numbered walkthrough; and a guard result.count == count was added after address derivation). One carried-forward finding (the sync-precondition determinism concern) is STILL_VALID in a new form: I traced the pinned key-wallet-manager engine (rev dca5b05, rescan_batch/tick in dash-spv/src/sync/filters/manager.rs) and confirmed that script matching against a wallet's addresses cannot happen before that wallet is registered, and that createWallet(..., birthHeight: 0) triggers a genesis-to-tip rescan. Because the test mines and fully commits BOTH the 'historic' and 'live' halves before calling createWallet, both halves are recovered by the exact same post-registration rescan — the test does not actually exercise a concurrent/in-flight live-delivery code path distinct from the historic rescan, despite its comments claiming otherwise. This is a new path-scoped latest-delta observation (surfaced only once the timing-heuristic version of this finding was replaced) rather than a functional bug: the test remains a valid regression check for birthHeight-0 backfill, it just doesn't test the specific 'live in flight' scenario its own documentation describes. No other new latest-delta defects were found in the rewritten file.

Source: reviewers codex/general=gpt-5.6-sol(completed), codex/ffi-engineer=gpt-5.6-sol(completed), claude/general=claude-sonnet-5(completed), claude/ffi-engineer=claude-sonnet-5(completed); final verifier claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol (orchestration-only).

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — ffi-engineer (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (completed), claude-sonnet-5 — ffi-engineer (completed)

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

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

In `packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift:68-84: "Live half" blocks are fully mined and committed before the wallet is registered, so the test never exercises concurrent live-delivery
  The comment block at the top of the file (and step 4's comment) claims the live half is 'mined against the already-running client, so those blocks are in flight around the moment the wallet is registered' — i.e. that this half is recovered via real-time filter delivery to a registered wallet, distinct from the historic half's register-time rescan. In practice, all five `fund()` calls for the live half run to completion (each synchronously awaits InstantSend lock + one mined block) before `createWallet` is ever invoked. I traced the pinned key-wallet-manager engine (rev dca5b05): script matching against a wallet's addresses cannot occur before that wallet exists (`scripts_by_wallet` is only populated once a wallet is registered), and `createWallet(..., birthHeight: 0)` triggers a rescan from genesis to the current committed height (`rescan_batch`/tick rescan in `dash-spv/src/sync/filters/manager.rs`). Since both halves are already mined and committed by the time `createWallet` runs, both are recovered by the exact same register-time rescan — there is no code path in this test that is exercised only by the 'live' half. `filterHeightAtImport` is sampled but only used in failure diagnostics, never asserted against the new tip, so nothing here would catch a regression that broke live (post-registration) filter delivery while leaving the birthHeight-0 rescan intact. Consider registering the wallet before mining at least one live-half block (so at least one transaction can only be found via post-registration filter matching), or explicitly asserting `filterHeightAtImport` lags the tip at the point of registration.

[thepastaclaw]

Final validation — Codex + Sonnet

PR #4064's own commit (b0eafe7) adds exactly one file — the rewritten SpvManyTxWalletImportHistoryIntegrationTests.swift — which now genuinely separates register-time rescan recovery from live post-registration filter matching, and asserts both aggregate balance and persisted per-transaction history for each half. All 6 prior carried-forward findings are confirmed FIXED by direct code inspection at the exact head commit: the FFIWallet ownership comment and defer are now correct (lines 208-219), the tipHeight/5 timing heuristic is gone in favor of syncing to tip before registration, assertHistory checks readTxids() against expected txids, funding goes through env.fund (preserving InstantSend/masternode-broadcast), the truncated comment is gone, and a guard(result.count == count) throwing a named SetupError now protects the funding loop (lines 248-252) — this also resolves the sole open CodeRabbit finding (comment 3561308129). Codex's shielded-wallet 'drain floor' bug in fund_from_asset_lock.rs is real (confirmed by direct source read: the DrainAccountBalance match arm at lines 201-214 discards any caller-supplied minimum_lock_duffs and substitutes only the protocol-fee floor), but it is not part of this PR: git show --stat b0eafe7327 proves the PR's own commit touches only the Swift test file, and the bug's code was introduced by a separate commit (963f0d2, tagged '#4327' in its own commit message) that sits between the given merge-base and head purely due to branch/rebase history — not yet merged into origin/v4.2-dev (verified: git merge-base --is-ancestor 963f0d26fc origin/v4.2-dev fails, and origin/v4.2-dev's fund_from_asset_lock.rs lacks this code entirely). CodeRabbit's own walkthrough for this PR likewise scopes only the one Swift file. This is recorded as an out-of-scope follow-up rather than a blocker for #4064.

Source: reviewers codex/general=gpt-5.6-sol (completed), claude/general=claude-sonnet-5 (completed); final verifier claude/final-verifier=claude-sonnet-5 (completed, attempt 2 after a contract-validation retry). Orchestration-only (not reviewer evidence): openclaw-agent/cliproxy/gpt-5.6-sol.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (completed)