fix(swift-sdk): sort transfer outputs lexicographically before ReduceOutput

[llbartekll]

Issue being fixed or feature implemented

Fixes #3738 — ManagedPlatformAddressWallet.transfer() was underpaying recipients whenever the resolved change address sorted lexicographically before any recipient address. HIGH severity, silent fund misdirection.

What was done?

The Swift wrapper was targeting the fee-reduction step by insertion index:

ffiOutputs = [...recipients, change]
let changeIndex = UInt16(ffiOutputs.count - 1)   // ⚠ insertion order
feeStrategy = [FeeStrategyStepFFI(step_type: 1, index: changeIndex)]

But the Rust side collects outputs into BTreeMap<PlatformAddress, Credits>, which canonicalizes them lexicographically by address — variant byte first (P2pkh = 0 < P2sh = 1), then the 20-byte hash. DPP's fee-deduction layer (rs-dpp/src/address_funds/fee_strategy/v0.rs) resolves ReduceOutput(N) against that canonicalized list, not Swift's insertion order. When the change address sorted before any recipient, the wrong row got decremented: the recipient was underpaid by the fee amount, and the change came back full. The bug hit randomly based on which address got generated as change, so it bypassed integration tests most runs.

Fix: mirror the Rust ordering in Swift via a new buildSortedFFIOutputs helper.

• Build (addressType, hash, balance) rows for all outputs + change.
• Sort by (addressType, hash) — addressType ascending first, then hash via Data.lexicographicallyPrecedes (unsigned byte-by-byte).
• Look up the change row's actual position in the sorted list.
• Only then marshal into AddressBalanceEntryFFI.

The helper is internal static so the unit tests can hit it directly without spinning up FFI handles.

Per discussion in #3738, this is the structurally complete fix for the Swift side. No additional FFI consumers are planned, so a Rust-side transfer_with_change_address bridge is out of scope.

How Has This Been Tested?

Two regression tests on the pure helper (in SwiftExampleApp/SwiftExampleAppTests/ManagedPlatformAddressWalletTests.swift, auto-discovered via the existing PBXFileSystemSynchronizedRootGroup):

1. test_buildSortedFFIOutputs_changeSortsBeforeRecipient_indexIsZero — exact Swift: ReduceOutput(insertion-index) in ManagedPlatformAddressWallet.transfer underpays recipients when change sorts before recipient #3738 scenario. Recipient hash 0xFF…, change hash 0x00…. Asserts changeIndex == 0 and both rows land in the correct sorted positions with balances preserved. Pre-fix this returned changeIndex == 1 and Rust would have decremented the recipient.

2. test_buildSortedFFIOutputs_multipleRecipients_changeInMiddle — three outputs, change sorts into the middle position. Recipients at 0x10… / 0xF0…, change at 0x80…. Asserts changeIndex == 1 and all three rows in the correct sorted positions. The 0x80 midpoint deliberately crosses the signed-byte boundary — if the comparator ever degrades to signed-byte semantics, this test fails immediately. Also defends against any off-by-one or last-position assumption regression.

End-to-end build verification was blocked locally by a stale DashSDKFFI.xcframework (recent FFI constants from PR #3651 aren't in the framework I had built on May 25) — unrelated to this change. The two pre-existing errors are in PlatformWalletResult.swift, not in the file modified here. swift-frontend reported zero errors in ManagedPlatformAddressWallet.swift during the failed build, confirming my changes parse and type-check cleanly. Will need ./build_ios.sh to refresh the framework before CI/test run.

Breaking Changes

None. The FFI ABI is unchanged; only the values Swift puts into the existing slots change.

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

Summary by CodeRabbit

• Bug Fixes
  • Fixed a critical issue in wallet transfer operations where transaction outputs were not being sorted in the correct canonical order. The change amount is now properly computed and aligned with expected output ordering before processing. This ensures accurate transaction construction, reliable fee calculations, and proper handling of change outputs in wallet transfers.

[coderabbitai]

📝 Walkthrough

Walkthrough

ManagedPlatformAddressWallet.transfer() now sorts recipients and change outputs into canonical lexicographic order before constructing FFI rows, computing the correct changeIndex aligned with Rust's BTreeMap canonicalization instead of insertion order, and targeting fee reduction to the correct output.

Changes

Output Canonicalization Fix

Layer / File(s)	Summary
Output sorting and fee-index alignment packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift	buildSortedFFIOutputs() merges and sorts recipients plus change by address-type discriminant then 20-byte hash to match Rust's lexicographic order, returns the changeIndex within the sorted output list, and transfer() integrates this helper to align the fee-strategy ReduceOutput index with canonical ordering.
Regression tests for sorted output ordering packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/ManagedPlatformAddressWalletTests.swift	Two test cases validate buildSortedFFIOutputs(): one confirms change sorts before recipient when insertion order differs (expects changeIndex == 0), and another verifies change sorts into the middle of multiple recipients across address boundaries (expects changeIndex == 1), each asserting correct row ordering and balances.

Sequence Diagram

sequenceDiagram
    participant Transfer as transfer()
    participant BuildSorted as buildSortedFFIOutputs()
    participant Sort as Lexicographic Sort
    participant FFI as AddressBalanceEntryFFI
    participant FeeStrat as Fee Strategy
    
    Transfer->>BuildSorted: recipients, change tuple
    BuildSorted->>BuildSorted: merge into combined list
    BuildSorted->>Sort: sort by address-type, then hash
    Sort->>BuildSorted: sorted output items
    BuildSorted->>FFI: convert each to FFI row
    BuildSorted->>BuildSorted: track change position (changeIndex)
    BuildSorted-->>Transfer: (ffiOutputs, changeIndex)
    Transfer->>FeeStrat: ReduceOutput(changeIndex)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• shumkov
• QuantumExplorer

Poem

🐰 Outputs once scattered in insertion's chaotic way,
Now sorted by address in lexicographic array.
The change no longer lost in the reordering fray,
Fee reduction hits its mark—hip-hip-hooray! 🌟

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: sorting transfer outputs lexicographically before applying fee reduction via ReduceOutput.
Linked Issues check	✅ Passed	The PR implements the preferred Fix (a) from #3738: Swift now sorts outputs lexicographically before building the FFI array and passes the correct ReduceOutput index, with comprehensive regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the underpayment issue in #3738: one core helper function and two focused regression tests with no unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 fix/swift-platform-transfer-reduce-output-order

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[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/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- Around line 353-355: The current force-unwrapping when computing changeIdx can
trap; in ManagedPlatformAddressWallet (the changeIdx computation using
rows.firstIndex { $0.addressType == change.addressType && $0.hash == change.hash
}), replace the forced unwrap with a guarded lookup: use guard let idx =
rows.firstIndex(...) and then safely convert with UInt16(exactly: idx) (or check
idx <= UInt16.max); if either step fails, return or throw an appropriate wallet
error instead of crashing. Ensure the surrounding method returns the same error
type/path used elsewhere in this file for lookup/conversion failures.

🪄 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: bd6fc088-6611-4edc-b094-f9460115c048

📥 Commits

Reviewing files that changed from the base of the PR and between ef398cb and 0fce8fa.

📒 Files selected for processing (2)

• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift
• packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/ManagedPlatformAddressWalletTests.swift

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

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

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

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

Xcode manual integration:

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

[thepastaclaw]

Code Review

PR correctly aligns Swift's FeeStrategyStep::ReduceOutput index with Rust's BTreeMap<PlatformAddress, _> canonicalization by sorting outputs on (addressType ascending, hash lexicographic) — faithfully mirroring derive(Ord) on the Rust enum. Pre-existing dedup guards at ManagedPlatformAddressWallet.swift:160-165 and :198-202 ensure no duplicate hashes ever reach buildSortedFFIOutputs, so the only inline blocking finding from codex-general does not apply. No in-scope issues; only out-of-scope follow-ups worth tracking.

Out-of-scope follow-up suggestions (4)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Canonical-ordering decision lives in Swift, against swift-sdk/CLAUDE.md — buildSortedFFIOutputs makes a structural decision (canonical row order + which index encodes ReduceOutput) in Swift that mirrors a Rust-private invariant: derive(Ord) on enum PlatformAddress { P2pkh([u8;20]), P2sh([u8;20]) }. packages/swift-sdk/CLAUDE.md is explicit that decisions belong in Rust and Swift should only marshal. If anyone reorders the Rust variants, adds a new one, or changes PlatformAddressFFI's wire encoding so that address_type bytes no longer match the Ord ordinals, this helper will silently desynchronize and re-introduce the fund-misdirection bug. The PR description acknowledges this and chooses Swift-side for urgency — defensible, but worth tracking.
  • Follow-up: Open a separate issue to expose a Rust FFI like platform_address_wallet_transfer_with_change that takes recipients + change and computes the canonical change index internally, removing the cross-language ordering duplication.
• Sort test coverage doesn't exercise the addressType axis — Both new tests in ManagedPlatformAddressWalletTests.swift use addressType: 0 (P2PKH) for every row, so the addressType != addressType branch of the comparator (P2pkh vs P2sh, which is the half tied to a Rust enum declaration order) isn't exercised. Not blocking — existing tests catch the original bug and the 0x7F/0x80 boundary risk — and the Rust FFI currently rejects non-zero address types anyway.
  • Follow-up: Add a unit test with a mixed-variant (P2PKH + P2SH) layout once P2SH support is wired through TryFrom<PlatformAddressFFI> in packages/rs-platform-wallet-ffi/src/platform_address_types.rs.
• Recipient/change collision guards key on 20-byte hash only, not full (addressType, hash) — recipientHashes, selected-input filtering, and the explicit-change-address validation in ManagedPlatformAddressWallet.swift compare only hash, so a P2PKH(hash) and P2SH(hash) pair would be flagged as colliding even though Rust's BTreeMap<PlatformAddress, _> would treat them as distinct keys. Only P2PKH is currently accepted on the Rust side, so this has no current behavioral impact, but it will need to change in lockstep if P2SH is enabled.
  • Follow-up: Track this together with enabling P2SH support in TryFrom<PlatformAddressFFI>; audit all Swift hash-only equality checks to compare (addressType, hash).
• Swift docs claim P2SH support that the Rust FFI parser rejects — ManagedPlatformAddressWallet.TransferOutput and ChangeAddress document addressType as 0 = P2PKH, 1 = P2SH, but TryFrom<PlatformAddressFFI> for PlatformAddress in packages/rs-platform-wallet-ffi/src/platform_address_types.rs returns 'Unsupported address type' for anything but 0. Pre-existing interop gap, unaffected by this PR.
  • Follow-up: Either add P2SH support to the FFI's TryFrom<PlatformAddressFFI> or narrow the Swift API and docs to the address types the Rust FFI actually accepts.