Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import Foundation
import XCTest
import DashSDKFFI
@testable import SwiftDashSDK

/// Wallet imported MID-sync.
final class SpvManyTxMidSyncBackfillIntegrationTests: IntegrationTestCase {
/// Number of transactions / distinct funded addresses. Parameterized.
private let numTxs = 10

/// Funding amount per address, in DASH.
private let amountEachDash: Double = 0.001
private var amountEachDuffs: UInt64 { UInt64(amountEachDash * 1e8) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Floating-point conversion for Duffs is fragile.

UInt64(amountEachDash * 1e8) works for 0.001 (truncates 100000.000…002 to 100000), but breaks for other values — e.g. 0.0001 * 1e8 = 9999.999… truncates to 9999 instead of 10000. Define the amount in integer Duffs directly to avoid precision-dependent truncation.

🛡️ Proposed fix
-    private let amountEachDash: Double = 0.001
-    private var amountEachDuffs: UInt64 { UInt64(amountEachDash * 1e8) }
+    private let amountEachDuffs: UInt64 = 100_000 // 0.001 DASH
+    private var amountEachDash: Double { Double(amountEachDuffs) / 1e8 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private var amountEachDuffs: UInt64 { UInt64(amountEachDash * 1e8) }
private let amountEachDuffs: UInt64 = 100_000 // 0.001 DASH
private var amountEachDash: Double { Double(amountEachDuffs) / 1e8 }
🤖 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/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`
at 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.


func testManyTxImportedMidSyncBackfillsAllHistory() async throws {
let mnemonic = try Mnemonic.generate(wordCount: 24)

// 1. Derive N distinct external receive addresses from the KeyWallet
// layer (regtest-encoded — the manager's network drives encoding).
let km = try WalletManager(network: .regtest)
let walletId = try km.addWallet(mnemonic: mnemonic)
let addresses = try Self.deriveExternalAddresses(
manager: km, walletId: walletId, count: numTxs
)

// One funding tx per block.
for i in 0..<numTxs {
_ = try await env.coreRPC.sendToAddress(
amount: amountEachDash, address: addresses[i]
Comment on lines +22 to +29

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: No guard that deriveExternalAddresses returned exactly numTxs addresses before indexing

deriveExternalAddresses skips any null C-string entries when building result (line 132: if let cstr = arr[i]), so it could in principle return fewer than count addresses. The funding loop then indexes addresses[i] for i in 0..<numTxs unconditionally, which would crash with an out-of-range index rather than a clear assertion failure if that ever happened. Low likelihood given the FFI contract, but a cheap XCTAssertEqual(addresses.count, numTxs) before the loop would fail more legibly.

source: ['sonnet5-general']

)
_ = try await env.mine(1)
}
Comment on lines +26 to +32

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Funding loop bypasses env.fund()'s InstantSend/masternode-broadcast steps

I confirmed env.fund(address:dash:) (IntegrationTestEnv.swift:391-398) does sendToAddressbroadcastToMasternodes(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.

source: ['sonnet5-general']


let expectedTotal = UInt64(numTxs) * amountEachDuffs

// Start the SPV client EMPTY, then wait until it is ~20% synced so the
// wallet is imported genuinely mid-sync.
try await env.walletManager.startSpv(config: env.spvConfig)
let tipHeight = try await env.coreRPC.getBlockCount()
try await env.walletManager.waitUntilUpToDate(
height: tipHeight / 5,
timeout: 180
)
Comment on lines +36 to +43

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: "~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.

source: ['codex-general']


// Import the funded wallet into the still-syncing client, birthHeight
// 0 so it is contractually expected to scan history from genesis.
let imported = try await env.walletManager.createWallet(
mnemonic: mnemonic,
network: .regtest,
name: "many-tx",
createDefaultAccounts: true,
birthHeight: 0
)

// Wait until the wallet is fully synced, then check that the balance is

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Comment is truncated mid-sentence

"Wait until the wallet is fully synced, then check that the balance is" cuts off before completing the thought. Confirmed verbatim in the file.

Suggested change
// Wait until the wallet is fully synced, then check that the balance is
// Wait until the wallet is fully synced, then check that the balance
// reaches the expected total.

source: ['sonnet5-general']

try await env.walletManager.waitUntilUpToDate(
height: try await env.coreRPC.getBlockCount(),
timeout: 180
)
_ = try? await Wait.until("balance reaches expected", timeout: 120, pollInterval: 0.5) {
try imported.balance().total == expectedTotal
}
let atFullSync = try imported.balance().total

XCTAssertEqual(
atFullSync,
expectedTotal,
"atFullSync=\(atFullSync) expected=\(expectedTotal)"
)
Comment on lines +55 to +69

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: 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.

source: ['codex-general']

}

// MARK: - KeyWallet address enumeration

struct DerivationError: Error, CustomStringConvertible {
let description: String
}

/// Generate and return BIP-44 external (receive) addresses in
/// `[0, count)` from a KeyWallet `WalletManager` handle.
///
/// `managed_wallet_get_bip_44_external_address_range` is a single Rust-side
/// call that generates addresses lazily if they don't yet exist, so it can
/// hand back the full 0..<count range regardless of the default gap limit —
/// no Swift-side derivation or gap-limit walking required.
private static func deriveExternalAddresses(
manager: WalletManager, walletId: Data, count: Int
) throws -> [String] {
var error = FFIError()

guard let managedInfo = walletId.withUnsafeBytes({ raw in
wallet_manager_get_managed_wallet_info(
manager.ffiHandle,
raw.bindMemory(to: UInt8.self).baseAddress,
&error
)
}) else {
throw DerivationError(description: "wallet_manager_get_managed_wallet_info failed: \(Self.take(&error))")
}
defer { managed_wallet_info_free(managedInfo) }

// Non-owning: the manager retains ownership of the wallet handle.
guard let wallet = walletId.withUnsafeBytes({ raw in
wallet_manager_get_wallet(
manager.ffiHandle,
raw.bindMemory(to: UInt8.self).baseAddress,
&error
)
}) else {
throw DerivationError(description: "wallet_manager_get_wallet failed: \(Self.take(&error))")
}
Comment on lines +101 to +110

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: 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.

Suggested change
// Non-owning: the manager retains ownership of the wallet handle.
guard let wallet = walletId.withUnsafeBytes({ raw in
wallet_manager_get_wallet(
manager.ffiHandle,
raw.bindMemory(to: UInt8.self).baseAddress,
&error
)
}) else {
throw DerivationError(description: "wallet_manager_get_wallet failed: \(Self.take(&error))")
}
// Owning: the manager clones the wallet on each call; the caller
// must free it with wallet_free_const().
guard let wallet = walletId.withUnsafeBytes({ raw in
wallet_manager_get_wallet(
manager.ffiHandle,
raw.bindMemory(to: UInt8.self).baseAddress,
&error
)
}) else {
throw DerivationError(description: "wallet_manager_get_wallet failed: \(Self.take(&error))")
}
defer { wallet_free_const(wallet) }

source: ['codex-ffi-engineer', 'sonnet5-ffi-engineer']


var addressesOut: UnsafeMutablePointer<UnsafeMutablePointer<CChar>?>? = nil
var outCount = 0
let ok = managed_wallet_get_bip_44_external_address_range(
managedInfo,
wallet,
/* account_index */ 0,
/* start_index */ 0,
/* end_index */ UInt32(count),
&addressesOut,
&outCount,
&error
)
guard ok, let arr = addressesOut else {
throw DerivationError(description: "managed_wallet_get_bip_44_external_address_range failed: \(Self.take(&error))")
}
defer { address_array_free(arr, outCount) }

var result: [String] = []
result.reserveCapacity(outCount)
for i in 0..<outCount {
if let cstr = arr[i] {
result.append(String(cString: cstr))
}
}
return result
Comment on lines +129 to +136

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Validate derived address count matches requested count.

If the FFI returns fewer addresses than count (e.g. null entries skipped by the if let), result will have fewer elements and the subsequent addresses[i] access in the funding loop will crash with an unhelpful index-out-of-bounds error. Add a post-loop assertion to fail fast with a clear message.

🛡️ Proposed fix
     for i in 0..<outCount {
         if let cstr = arr[i] {
             result.append(String(cString: cstr))
         }
     }
+    guard result.count == count else {
+        throw DerivationError(
+            description: "deriveExternalAddresses: expected \(count) addresses, got \(result.count)"
+        )
+    }
     return result
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var result: [String] = []
result.reserveCapacity(outCount)
for i in 0..<outCount {
if let cstr = arr[i] {
result.append(String(cString: cstr))
}
}
return result
var result: [String] = []
result.reserveCapacity(outCount)
for i in 0..<outCount {
if let cstr = arr[i] {
result.append(String(cString: cstr))
}
}
guard result.count == count else {
throw DerivationError(
description: "deriveExternalAddresses: expected \(count) addresses, got \(result.count)"
)
}
return result
🤖 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/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvManyTxMidSyncBackfillIntegrationTests.swift`
around lines 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.

}

/// Read (and free) an `FFIError` message, returning a display string.
private static func take(_ error: inout FFIError) -> String {
guard let msg = error.message else { return "code \(error.code.rawValue)" }
defer {
error_message_free(msg)
error.message = nil
}
return String(cString: msg)
}
}
Loading