Skip to content

fix(swift-sdk): wrapper for FFITxOutput that correctly handles alloc memory#3472

Merged
QuantumExplorer merged 1 commit intov3.1-devfrom
fix/solve-txoutput-leak
Apr 10, 2026
Merged

fix(swift-sdk): wrapper for FFITxOutput that correctly handles alloc memory#3472
QuantumExplorer merged 1 commit intov3.1-devfrom
fix/solve-txoutput-leak

Conversation

@ZocoLini
Copy link
Copy Markdown
Collaborator

@ZocoLini ZocoLini commented Apr 9, 2026

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

  • Refactor
    • Updated the transaction output model used by wallet signing. The public signing API signature changed, so callers must construct and pass outputs using the new output type.
    • Example app updated to use the new output construction when creating and signing transactions.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 9, 2026

📝 Walkthrough

Walkthrough

Refactors transaction output representation: removes Transaction.Output, adds standalone TxOutput class that manages C-string addresses with strdup/free, and updates buildSignedTransaction signatures and call sites to accept [TxOutput].

Changes

Cohort / File(s) Summary
Type Definition
packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TxOutput.swift, packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
Added TxOutput class storing address: UnsafeMutablePointer<CChar> and amount: UInt64, with toFFI() and deinit freeing the C string; removed nested Transaction.Output struct and its toFFI() conversion.
Wallet APIs
packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift, packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
Changed buildSignedTransaction(..., outputs: ...) parameter type from [Transaction.Output] to [TxOutput]; WalletManager removed per-output cleanup loop in defer.
Call Sites / UI
packages/swift-sdk/SwiftExampleApp/.../SendViewModel.swift
Updated send flow to construct and pass TxOutput(address:amount:) instances instead of Transaction.Output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibbled code and made it neat,
Outputs hopped out on nimble feet.
strdup for addresses, free at end,
A little refactor from your rabbit friend. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(swift-sdk): wrapper for FFITxOutput that correctly handles alloc memory' directly describes the main change: introducing a wrapper (TxOutput class) for FFITxOutput that properly manages allocated memory via strdup and free.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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/solve-txoutput-leak

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

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

@github-actions github-actions bot added this to the v3.1.0 milestone Apr 9, 2026
@ZocoLini ZocoLini changed the title wrapper for FFITxOutput taht correctly handles alloc memory fix(swift-sdk): wrapper for FFITxOutput that correctly handles alloc memory Apr 9, 2026
@ZocoLini ZocoLini marked this pull request as ready for review April 9, 2026 21:53
@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Apr 9, 2026

⏳ Review in progress (commit eb2825b)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

✅ DashSDKFFI.xcframework built for this PR.

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

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

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 273-275: The recipient address is used untrimmed when constructing
the TxOutput in SendViewModel, which can let a trailing space slip past
detection but break signing/broadcast; before creating TxOutput(address:
recipientAddress, ...), trim the input (e.g., use
recipientAddress.trimmingCharacters(in: .whitespacesAndNewlines) or the existing
trimmed variable) and pass that trimmed string into TxOutput so the address used
for signing/broadcast matches the validated value.
🪄 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: d0396a08-ab76-44b2-ae4e-553e1e54f7c4

📥 Commits

Reviewing files that changed from the base of the PR and between 6f72a64 and 2710c85.

📒 Files selected for processing (5)
  • packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/CoreWalletManager.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/TxOutput.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
💤 Files with no reviewable changes (1)
  • packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Transaction.swift

@ZocoLini ZocoLini force-pushed the fix/solve-txoutput-leak branch from 2710c85 to eb2825b Compare April 9, 2026 22:08
@QuantumExplorer QuantumExplorer merged commit 9d799d3 into v3.1-dev Apr 10, 2026
44 checks passed
@QuantumExplorer QuantumExplorer deleted the fix/solve-txoutput-leak branch April 10, 2026 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants