Skip to content

refactor(platform-wallet): expose the new core TransactionBuilder API + rust-dashcore bump#3970

Open
ZocoLini wants to merge 2 commits into
v4.1-devfrom
feat/core-tx-builder-ffi
Open

refactor(platform-wallet): expose the new core TransactionBuilder API + rust-dashcore bump#3970
ZocoLini wants to merge 2 commits into
v4.1-devfrom
feat/core-tx-builder-ffi

Conversation

@ZocoLini

@ZocoLini ZocoLini commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator
  • Bump rust-dashcore to pull in key-wallet's new TransactionBuilder API
  • Core transaction builder exposed over FFI
  • set_gap_limit for any account type (focus: CoinJoin)
  • Swift wrappers: CoreTransactionBuilder (fluent API), CoreTransaction, ManagedCoreWallet.broadcastTransaction/setGapLimit, and migration of SendViewModel (core→core) to the new build→broadcast flow

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

  • New Features

    • Added support for building, signing, and broadcasting core wallet transactions in a more structured way.
    • Introduced a new option to adjust account gap limits for core wallet address discovery.
  • Bug Fixes

    • Improved transaction signing compatibility for core wallet-related flows.
    • Updated payment flow handling to work with the new transaction object-based workflow.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@ZocoLini, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4d073928-3465-400a-9df9-6d3be58eb424

📥 Commits

Reviewing files that changed from the base of the PR and between 5e3862d and 909484a.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • Cargo.toml
  • packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
  • packages/rs-platform-wallet/src/wallet/core/wallet.rs
  • packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
📝 Walkthrough

Walkthrough

This PR bumps Dash workspace dependency revisions, adds extended_public_key to the Signer trait implementations (production and test signers), refactors core wallet transaction handling into a new FFI TransactionBuilder module, adds a gap-limit setter, and updates Swift bindings and the example app's send flow accordingly.

Changes

Signer trait extended_public_key completeness

Layer / File(s) Summary
MnemonicResolverCoreSigner derivation refactor
packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs
Splits derivation into derive_extended_priv helper and derive_priv, adds extended_public_key to the Signer implementation with explicit key wiping.
Test signer stubs
packages/rs-dpp/src/state_transition/mod.rs, .../address_funding_from_asset_lock_transition/signing_tests.rs, .../shield_from_asset_lock_transition/signing_tests.rs
Adds extended_public_key implementations to FixedKeySigner test signers, returning unsupported errors.

Core wallet transaction builder and broadcast refactor

Layer / File(s) Summary
CoreWallet gap-limit and send_to_addresses removal
packages/rs-platform-wallet/src/wallet/core/wallet.rs, .../broadcast.rs
Adds wallet_manager() and set_gap_limit(...) to CoreWallet, removes send_to_addresses from broadcast.rs.
Rust FFI transaction builder module
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs, mod.rs
Adds FFITransactionBuilder/FFICoreTransaction types and C API to create, fund, add inputs, configure, build/sign, and free transactions.
FFI broadcast and gap-limit bindings
packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs, addresses.rs
Changes core_wallet_broadcast_transaction to accept FFICoreTransaction, removes core_wallet_send_to_addresses/core_wallet_free_tx_bytes, adds core_wallet_set_gap_limit.
Swift CoreTransactionBuilder and ManagedCoreWallet bindings
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift, ManagedCoreWallet.swift
Adds CoreTransaction/CoreTransactionBuilder Swift classes, adds setGapLimit, changes broadcastTransaction to accept CoreTransaction, removes old sendToAddresses.
Example app send flow update
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
Replaces sendToAddresses call with explicit build/sign/broadcast via CoreTransactionBuilder in the coreToCore send path.
Dependency revision bump
Cargo.toml
Updates git rev for Dash core-related crates to a new commit hash.

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

Sequence Diagram(s)

sequenceDiagram
    participant SendViewModel
    participant CoreTransactionBuilder
    participant FFI as Rust FFI (transaction_builder.rs)
    participant CoreWallet
    participant ManagedCoreWallet

    SendViewModel->>CoreTransactionBuilder: init(network)
    SendViewModel->>CoreTransactionBuilder: addOutput(address, amount)
    SendViewModel->>CoreTransactionBuilder: setFunding(wallet, accountType, accountIndex)
    CoreTransactionBuilder->>FFI: core_wallet_tx_builder_set_funding
    FFI->>CoreWallet: resolve managed account, last processed height
    SendViewModel->>CoreTransactionBuilder: buildSigned(wallet, accountType, accountIndex)
    CoreTransactionBuilder->>FFI: core_wallet_tx_builder_build_signed(mnemonic resolver)
    FFI->>CoreWallet: build + sign transaction
    FFI-->>CoreTransactionBuilder: FFICoreTransaction (bytes, fee)
    CoreTransactionBuilder-->>SendViewModel: CoreTransaction
    SendViewModel->>ManagedCoreWallet: broadcastTransaction(tx)
    ManagedCoreWallet->>FFI: core_wallet_broadcast_transaction(tx)
    FFI-->>ManagedCoreWallet: txid
    ManagedCoreWallet-->>SendViewModel: txid
Loading

Possibly related PRs

  • dashpay/platform#3634: The retrieved PR expands the Signer trait surface by adding an extended_public_key implementation on MnemonicResolverCoreSigner, matched here by FixedKeySigner test signer updates.
  • dashpay/platform#3671: DPP test signers updated to implement Signer::extended_public_key, supporting the retrieved PR's signer-based asset-lock signing flow.
  • dashpay/platform#3904: Both PRs update the Swift example app's Core send flow (SendViewModel/executeSend), one for multi-output builder logic and the other for multi-recipient UI.

Suggested labels: ready for final review

Suggested reviewers: shumkov, llbartekll, thepastaclaw

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: exposing the new core TransactionBuilder API and bumping rust-dashcore.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/core-tx-builder-ffi

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.

@ZocoLini ZocoLini force-pushed the feat/core-tx-builder-ffi branch 2 times, most recently from 68065ab to 3159ebb Compare July 1, 2026 08:04
@shumkov shumkov added this to the v4.1.0 milestone Jul 2, 2026
@shumkov shumkov changed the base branch from v4.0-dev to v4.1-dev July 2, 2026 07:51
@ZocoLini ZocoLini changed the title refactor(platform-wallet-ffi): expose the new core TransactionBuilder API refactor(platform-wallet): expose the new core TransactionBuilder API Jul 2, 2026
@ZocoLini ZocoLini marked this pull request as ready for review July 2, 2026 07:58
@thepastaclaw

thepastaclaw commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

✅ Review complete (commit 909484a)

@ZocoLini ZocoLini force-pushed the feat/core-tx-builder-ffi branch from 3159ebb to 5265458 Compare July 2, 2026 08:52
@ZocoLini ZocoLini force-pushed the feat/core-tx-builder-ffi branch from 5265458 to f265289 Compare July 2, 2026 09:30
@ZocoLini ZocoLini changed the title refactor(platform-wallet): expose the new core TransactionBuilder API refactor(platform-wallet): expose the new core TransactionBuilder API + rust-dashcore bump Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.54%. Comparing base (9f9092c) to head (909484a).
⚠️ Report is 1 commits behind head on v4.1-dev.

❗ There is a different number of reports uploaded between BASE (9f9092c) and HEAD (909484a). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (9f9092c) HEAD (909484a)
rust 3 1
Additional details and impacted files
@@              Coverage Diff              @@
##           v4.1-dev    #3970       +/-   ##
=============================================
- Coverage     87.18%   52.54%   -34.64%     
=============================================
  Files          2632       11     -2621     
  Lines        327563     1707   -325856     
=============================================
- Hits         285592      897   -284695     
+ Misses        41971      810    -41161     
Components Coverage Δ
dpp ∅ <ø> (∅)
drive ∅ <ø> (∅)
drive-abci ∅ <ø> (∅)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value ∅ <ø> (∅)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier ∅ <ø> (∅)
🚀 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.

@ZocoLini ZocoLini force-pushed the feat/core-tx-builder-ffi branch from f265289 to b6969a7 Compare July 2, 2026 09:50

@thepastaclaw thepastaclaw left a comment

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.

Code Review

The PR cleanly exposes key-wallet's TransactionBuilder through the FFI/Swift wrappers, but the take/store pattern is unsafe on the two async, wallet-lookup-backed steps (set_funding, add_inputs_from_outpoints): they take the builder out via mem::take before the fallible lookups, and on error the drained default is silently left in the handle — discarding all previously-configured outputs/change/fee/payload. This is a real hazard for the PR's own Send flow. A build_signed docstring inaccuracy and unchecked cross-network address parsing are additional concerns.

🔴 2 blocking | 🟡 4 suggestion(s) | 💬 3 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/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:282-318: set_funding silently empties the builder on any lookup failure
  `take_builder()` is called at line 282, before the fallible wallet/account lookups inside the async block (`get_wallet_and_info_mut`, the account-type match, `managed_account_mut` — lines 286-300). Because `take_builder` is `std::mem::take`, it swaps `TransactionBuilder::default()` into the FFI handle's inner slot and hands the live builder to the async closure. If any lookup fails, the closure returns `Err`, the outer `match funded { Err(e) => ... }` never calls `store_builder`, and `taken` is dropped. The caller's `FFITransactionBuilder` is left holding an empty default — every prior `add_output`, `set_change_address`, `set_fee_rate`, `set_special_payload` call is silently discarded. The PR's own SendViewModel pattern is `addOutput × N → setFunding → buildSigned` on a single builder, so a transient setFunding error (e.g. wrong account index, wallet momentarily unavailable) that the Swift caller catches and retries will silently produce a transaction with zero outputs. Restore the taken builder on the error branch (or defer `take_builder` until after every fallible lookup has succeeded).
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:353-386: add_inputs_from_outpoints has the same state-loss defect as set_funding
  Same shape as the set_funding finding: `let taken = (*builder).take_builder();` at line 353 runs before the async wallet lookup and the per-outpoint `managed.utxos.get(op)` checks (lines 356-372). If the wallet isn't found, the managed account isn't found, or any single requested outpoint isn't in the account's UTXO set (an explicitly-documented error path per the doc comment at lines 321-324), the closure returns `Err` and the outer error arm returns without calling `store_builder`, leaving the FFI handle pointing at an empty default builder. A single bad outpoint wipes every previously-configured output/change/fee/funding rather than leaving the builder untouched or reporting only the failed input-selection step. Defer `take_builder` until after the fallible lookups, or restore the taken builder on the error branch.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:389-397: build_signed docstring claims it frees the builder — it does not
  The doc says "This function also frees the builder". The implementation only calls `take_builder()` (line 417), which leaves a defaulted `TransactionBuilder` in place via `mem::take`; neither the outer `Box<FFITransactionBuilder>` (line 139) nor the inner `Box<TransactionBuilder>` (line 138) is freed. `core_wallet_tx_builder_destroy` is still required. The Swift wrapper is safe because `CoreTransactionBuilder.deinit` unconditionally calls destroy (CoreTransactionBuilder.swift:72), but any other C-ABI consumer that trusts the comment literally will leak the outer + inner boxes on every send (or attempt a double-free if they then also call destroy on what they think is a freed handle). Correct the doc to say the internal state is consumed but the handle must still be released via `core_wallet_tx_builder_destroy`.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:144-181: add_output / set_change_address accept cross-network addresses via assume_checked()
  Both entry points parse the caller-supplied address as `DashAddress::from_str(addr_str)?.assume_checked()`, which bypasses the network-tag check on the `NetworkUnchecked` result. Because `add_output` and `set_change_address` run before a wallet handle is attached, there is no network context to validate against at those call sites — but `set_funding` and `build_signed` both hold a wallet handle whose `network()` is known. A testnet address handed to a mainnet builder (or vice versa) currently passes through and gets embedded verbatim into the output script, with no user-visible error. Defer network validation to `set_funding`/`build_signed` where `wallet.network()` is available and reject addresses whose network does not match (`require_network`), or at minimum document that the caller owns network validation.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-339: check_ptr!(outpoints) rejects a legitimate zero-length call
  `check_ptr!(outpoints)` unconditionally errors if the pointer is null, even when `outpoints_len == 0`. Swift's `Array.withUnsafeBufferPointer` on an empty array commonly yields a nil `baseAddress`, so a legitimate "add zero extra inputs" call would deterministically fail with `ErrorNullPointer` even though `slice::from_raw_parts` would never dereference the pointer for a zero-length slice. The removed `core_wallet_send_to_addresses` handled this correctly with a `count > 0` guard on the pointer checks. Guard the null check on `outpoints_len > 0`.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:419-435: build_signed acquires a write lock but only reads state
  The async block in `core_wallet_tx_builder_build_signed` opens `wallet_manager().write().await` and uses `get_wallet_info_mut`, but only performs immutable reads (`last_processed_height`, `managed_account(&info.core_wallet.accounts, ...)`) — nothing under the write lock is actually mutated. Holding the write lock through the synchronous mnemonic-resolver round trip serializes unrelated read paths behind every sign step. Prefer `read().await` + `get_wallet_info` here.

Comment thread packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs Outdated
Comment thread packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs Outdated
Comment thread packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs Outdated
llbartekll
llbartekll previously approved these changes Jul 2, 2026

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 4

🤖 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/core_wallet/transaction_builder.rs`:
- Around line 147-151: The FFI transaction builder currently trusts the
caller-provided network and can validate or sign against a wallet from a
different network. Update `core_wallet_tx_builder_set_funding` and
`core_wallet_tx_builder_build_signed` in `transaction_builder.rs` to compare
`(*builder).network` with `wallet.network()` and reject mismatches before
proceeding. Use the existing `FFITransactionBuilder`, `set_funding`, and
`build_signed` paths as the enforcement points, and keep the `build_signed`
check as a backstop even if funding was never set.

In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 7-10: The `broadcast_transaction` doc comment is outdated because
it still describes building and signing via `TransactionBuilder`, while the
method now only broadcasts an already signed `Transaction`. Update the docs in
`broadcast_transaction` to state that it broadcasts the caller-provided signed
transaction and remove the misleading `TransactionBuilder` reference, keeping
the summary focused on broadcast-only behavior.

In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift`:
- Around line 82-175: The public mutators on CoreTransactionBuilder can still be
called after buildSigned has consumed and freed the underlying Rust builder,
which leaves later FFI calls using an invalid handle. Add the same lifecycle
check used by buildSigned, such as ensureUsable, at the start of every remaining
mutator including setFunding, addInputs, addOutput, setChangeAddress,
setFeeRate, setSelectionStrategy, setCurrentHeight, and setSpecialPayload so
each method fails fast once consumed is true.

In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 462-479: The send flow in SendViewModel is doing synchronous
CoreTransactionBuilder build/sign/broadcast work on the MainActor, which can
block the UI. Move the transaction creation and core.broadcastTransaction path
out of the `@MainActor` context by performing the CoreTransactionBuilder
operations and broadcast on a background task/queue, then only return to the
MainActor for UI state updates. Keep the logic around builder.addOutput,
setFunding, buildSigned, and core.broadcastTransaction intact, but ensure that
the expensive FFI calls are not executed on the main thread.
🪄 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: 648827b8-f4f3-43a4-aa78-02dee85d632a

📥 Commits

Reviewing files that changed from the base of the PR and between 4341d07 and 5e3862d.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • Cargo.toml
  • packages/rs-dpp/src/state_transition/mod.rs
  • packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rs
  • packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
  • packages/rs-platform-wallet/src/wallet/core/broadcast.rs
  • packages/rs-platform-wallet/src/wallet/core/wallet.rs
  • packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
  • packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift

Comment thread packages/rs-platform-wallet/src/wallet/core/broadcast.rs

@thepastaclaw thepastaclaw left a comment

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.

Code Review

Incremental review of 5e3862d vs b6969a7: the delta cleanly resolves 8 of 9 prior findings on transaction_builder.rs (state-loss in set_funding/add_inputs_from_outpoints, cross-network address acceptance, build_signed docstring/free semantics, zero-length outpoints, write-lock-for-read path, transaction_free double-free footgun, and the missing thread-safety doc). Only prior-9 (set_special_payload silently discards bincode trailing bytes) is unchanged and carried forward. Two new suggestion/nitpick observations in the delta: build_signed reclaims the builder before validating core_signer_handle/out_tx, and core_wallet_tx_builder_destroy nulls b.inner on a Box that immediately drops. No blocking issues.

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

Carried-forward findings already raised (1)

These findings were not re-posted as new inline comments because an existing review thread already covers them.

  • [NITPICK] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:278-287: set_special_payload silently accepts trailing bytes after a valid bincode payload — Carried forward from prior-9, unchanged in this delta. bincode::decode_from_slice returns (value, bytes_consumed), but the code destructures as Ok((p, _)) and discards the count. A caller that hands over a buffer containing a valid TransactionPayload followed by any trailing bytes (framin...
🤖 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-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:439-446: build_signed reclaims the builder before validating core_signer_handle / out_tx
  The current order is: `check_ptr!(builder)` → `Box::from_raw(builder)` and `Box::from_raw(ffi.inner)` → `check_ptr!(core_signer_handle)` → `check_ptr!(out_tx)`. If a caller passes a valid `builder` but a null `core_signer_handle` or null `out_tx`, the builder is silently freed before the FFI returns `ErrorNullPointer`. Memory is correct (Rust drops the reclaimed boxes on early return), but the caller loses their in-progress builder with no way to retry and the behavior is asymmetric versus the usual FFI convention ("error return → pointer is untouched"). The Swift wrapper compensates because `CoreTransactionBuilder.buildSigned` sets `consumed = true` unconditionally, but any non-Swift consumer that trusts the convention would double-free. Moving the two additional pointer checks above the `Box::from_raw` calls preserves the "always consumed on success/build failure" contract while surfacing programmer errors non-destructively.

Comment thread packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs Outdated
@ZocoLini ZocoLini force-pushed the feat/core-tx-builder-ffi branch from 5e3862d to d55453f Compare July 2, 2026 14:29
@ZocoLini ZocoLini force-pushed the feat/core-tx-builder-ffi branch from d55453f to 909484a Compare July 2, 2026 14:39

@thepastaclaw thepastaclaw left a comment

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.

Code Review

Incremental review of PR #3970 at d55453f. The latest delta (5e3862d..d55453f) is a mechanical follow-up to an upstream key-wallet trait split: extended_public_key is dropped from four Signer impls (three rs-dpp test doubles and rs-sdk-ffi's MnemonicResolverCoreSigner), plus rust-dashcore/key-wallet Cargo pins bumped. No new findings in this delta. packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs is byte-identical to the prior head, so all three prior nitpick/suggestion findings on that file are carried forward at the same line ranges. All eight previously-fixed findings remain fixed. No blockers.

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

Carried-forward findings already raised (3)

These findings were not re-posted as new inline comments because an existing review thread already covers them.

  • [NITPICK] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:278-287: set_special_payload silently accepts trailing bytes after a valid bincode payload — Carried forward from the prior review — transaction_builder.rs is unchanged in this delta. bincode::decode_from_slice(bytes, config::standard()) returns (payload, bytes_consumed), but line 280 binds the second element to _ and discards it. If a Swift caller passes a buffer containing a we...
  • [SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:439-446: build_signed reclaims the builder before validating core_signer_handle / out_tx — Carried forward from the prior review — file unchanged. check_ptr!(builder) passes, then both heap boxes are unconditionally reclaimed via Box::from_raw (442-443) before check_ptr!(core_signer_handle) and check_ptr!(out_tx) (445-446). This is intentional per the inline comment and matches...
  • [NITPICK] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:503-506: core_wallet_tx_builder_destroy nulls b.inner right before b is dropped — Carried forward from the prior review — file unchanged. b is a local Box<FFITransactionBuilder> reclaimed from the raw pointer and dropped (freeing the outer allocation) at the end of the function scope. The write b.inner = std::ptr::null_mut(); on line 506 mutates a field nothing subsequen...
🤖 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-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:439-446: build_signed reclaims the builder before validating core_signer_handle / out_tx
  Carried forward from the prior review — file unchanged. `check_ptr!(builder)` passes, then both heap boxes are unconditionally reclaimed via `Box::from_raw` (442-443) before `check_ptr!(core_signer_handle)` and `check_ptr!(out_tx)` (445-446). This is intentional per the inline comment and matches the Swift wrapper's unconditional `consumed = true` after the FFI call, so the shipped consumer is memory-safe. However, the asymmetry (null `builder` returns error and leaves it untouched; null `core_signer_handle`/`out_tx` returns error but has already silently freed the builder) violates the usual FFI convention of "error return implies inputs untouched" and is not documented in the `# Safety` block. Any other binding built against that convention would double-free by calling `core_wallet_tx_builder_destroy` afterward. Moving the two null-checks above the reclaim would restore convention without breaking the Swift wrapper (which sets `consumed = true` regardless). If keeping the current ordering, extend the `# Safety` comment to state the builder is always consumed once `builder` itself is non-null.

@thepastaclaw thepastaclaw left a comment

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.

Code Review

Incremental review reconciled against prior findings. The latest delta (d55453f..909484a) is a single 3-line cleanup in core_wallet_tx_builder_destroy removing the redundant b.inner = null_mut() write, which exactly closes prior-3. Two prior findings remain unaddressed and are carried forward: prior-1 (set_special_payload silently accepts trailing bytes after bincode decode) and prior-2 (build_signed reclaims/frees the builder before validating core_signer_handle/out_tx). No new defects were introduced by this delta.

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

Carried-forward findings already raised (2)

These findings were not re-posted as new inline comments because an existing review thread already covers them.

  • [NITPICK] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:277-287: set_special_payload silently accepts trailing bytes after a valid bincode payload — Carried forward from the prior review (prior-1); the latest delta did not touch this hunk. bincode::decode_from_slice(bytes, config::standard()) returns (payload, bytes_consumed), but line 280 destructures as Ok((p, _)) and discards the consumed count. If a caller passes a buffer whose firs...
  • [SUGGESTION] (deduped existing open thread) packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:439-446: build_signed reclaims the builder before validating core_signer_handle / out_tx — Carried forward from the prior review (prior-2); the latest delta did not touch this block. check_ptr!(builder) passes, then both heap boxes (FFITransactionBuilder and the inner TransactionBuilder) are unconditionally reclaimed via Box::from_raw (442-443) before `check_ptr!(core_signer_ha...
🤖 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-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:439-446: build_signed reclaims the builder before validating core_signer_handle / out_tx
  Carried forward from the prior review (prior-2); the latest delta did not touch this block. `check_ptr!(builder)` passes, then both heap boxes (`FFITransactionBuilder` and the inner `TransactionBuilder`) are unconditionally reclaimed via `Box::from_raw` (442-443) before `check_ptr!(core_signer_handle)` and `check_ptr!(out_tx)` (445-446). This is memory-safe as shipped (the reclaimed values simply drop on the early-return path) and matches the Swift wrapper's unconditional `consumed = true` after the call, but the asymmetry — null `builder` returns error and leaves it untouched, while null `core_signer_handle`/`out_tx` returns error yet silently frees the builder — breaks the usual FFI convention of "error return implies inputs untouched" and is not documented in the `# Safety` block. A future non-Swift binding built to that convention would call `core_wallet_tx_builder_destroy` after the error and double-free. Reordering the null checks before the two `Box::from_raw` calls keeps the `build_signed`-consumes-builder invariant on the success path while giving callers a recoverable error on trivial argument-validation failures.

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.

4 participants