refactor(platform-wallet): expose the new core TransactionBuilder API#3970
Open
ZocoLini wants to merge 1 commit into
Open
refactor(platform-wallet): expose the new core TransactionBuilder API#3970ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a new Rust FFI transaction builder module for core wallets, exposing C-callable functions to configure outputs, funding, inputs, fees, and sign transactions. It refactors broadcast and address APIs around signed ChangesCore wallet transaction and address flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SendViewModel
participant CoreTransactionBuilder
participant ManagedPlatformWallet
participant RustFFI
participant CoreWallet
SendViewModel->>CoreTransactionBuilder: addOutput / setFunding / buildSigned
CoreTransactionBuilder->>RustFFI: core_wallet_tx_builder_* calls
RustFFI->>CoreWallet: set funding, add inputs, build and sign
CoreTransactionBuilder-->>SendViewModel: CoreTransaction
SendViewModel->>ManagedPlatformWallet: broadcastTransaction(CoreTransaction)
ManagedPlatformWallet->>RustFFI: core_wallet_broadcast_transaction(tx, account_type, account_index)
RustFFI->>CoreWallet: broadcast_transaction_releasing_reservation or broadcast_transaction
CoreWallet-->>ManagedPlatformWallet: txid
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
2 times, most recently
from
68065ab to
3159ebb
Compare
ZocoLini
changed the title
ZocoLini
requested review from
QuantumExplorer,
lklimek,
llbartekll and
shumkov
as code owners
Collaborator
|
✅ Review complete (commit 3161856) |
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
2 times, most recently
from
5265458 to
f265289
Compare
ZocoLini
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests.
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
🚀 New features to boost your workflow:
|
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
from
f265289 to
b6969a7
Compare
thepastaclaw
suggested changes
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
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.
llbartekll
previously approved these changes
Jul 2, 2026
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
from
b6969a7 to
5e3862d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlpackages/rs-dpp/src/state_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rspackages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
thepastaclaw
approved these changes
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
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_slicereturns(value, bytes_consumed), but the code destructures asOk((p, _))and discards the count. A caller that hands over a buffer containing a validTransactionPayloadfollowed 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.
d55453f to
909484a
Compare
thepastaclaw
reviewed
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
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.rsis 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 viaBox::from_raw(442-443) beforecheck_ptr!(core_signer_handle)andcheck_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.bis a localBox<FFITransactionBuilder>reclaimed from the raw pointer and dropped (freeing the outer allocation) at the end of the function scope. The writeb.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
approved these changes
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
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 asOk((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 (FFITransactionBuilderand the innerTransactionBuilder) are unconditionally reclaimed viaBox::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.
shumkov
reviewed
Jul 4, 2026
| self.wallet_id | ||
| } | ||
|
|
||
| pub fn wallet_manager(&self) -> &Arc<RwLock<WalletManager<PlatformWalletInfo>>> { |
Collaborator
There was a problem hiding this comment.
Why do we need this getter?
Collaborator
Author
There was a problem hiding this comment.
It is being used in:
pub unsafe extern "C" fn core_wallet_tx_builder_set_funding(...)
shumkov
previously approved these changes
Jul 4, 2026
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
from
909484a to
65075b2
Compare
Contributor
There was a problem hiding this comment.
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/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- Around line 19-21: The # Safety documentation for the broadcast FFI entrypoint
is garbled and incomplete; update the safety clause in the core_wallet broadcast
function to clearly state the full caller contract for handle, tx, and out_txid.
Use the broadcast symbol’s existing doc comment near the FFI wrapper to replace
the broken sentence with a complete, precise requirement that matches the actual
invariants callers must satisfy.
🪄 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: 7926ec09-3572-40d5-b079-28a2ece63772
📒 Files selected for processing (9)
packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
- packages/rs-platform-wallet/src/wallet/core/wallet.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift
- packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
4 times, most recently
from
5b0cff2 to
9458b7a
Compare
ZocoLini
changed the title
thepastaclaw
suggested changes
Jul 6, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers claude/opus + codex/gpt-5.5 for general, rust-quality, and ffi-engineer; verifier claude/opus; policy gate review_policy.enforce_backport_prereq_policy applied.
Cumulative review at 9458b7a7, including the required latest delta 65075b23..9458b7a7. The latest delta is broad because the branch absorbed current base changes, but the focused current PR surface remains the core TransactionBuilder FFI/Swift flow.
Result: 1 blocking, 1 suggestion, 2 nitpicks.
Prior finding reconciliation:
- prior-1: STILL VALID.
set_special_payloadstill decodes withOk((p, _))and discards the bincode consumed-byte count attransaction_builder.rs:293-303. - prior-2: STILL VALID.
build_signedis unchanged from65075b23attransaction_builder.rs:455-462and still reclaims the builder before validatingcore_signer_handle/out_tx; the comment documents intent but does not remove the public C-ABI ownership asymmetry.
Carried-forward prior findings:
- [SUGGESTION]
transaction_builder.rs:455-462:build_signedconsumes the builder before validating signer/output pointers. A null signer or out pointer returns an FFI error after the builder has been consumed, unlike a null builder error. The Swift wrapper handles this today, but the public C ABI remains surprising for future non-Swift callers. - [NITPICK]
transaction_builder.rs:293-303:set_special_payloadaccepts trailing bytes after a valid bincode payload. Checkingconsumed == bytes.len()would catch caller-side framing mistakes at the FFI boundary.
New latest-delta/current-head findings:
- [BLOCKING]
transaction_builder.rs:317-364: splitset_funding/build_signedbreaks key-wallet's documented atomic read-then-reserve invariant.set_fundingsnapshots spendable UTXOs and releases the wallet lock beforeassemble_unsignedlater reserves the selected inputs, so two FFI callers can observe the same UTXO as free and build conflicting transactions. The atomic send path keeps those steps under one wallet write lock; the split C ABI cannot. - [NITPICK]
transaction_builder.rs:266-277:core_wallet_tx_builder_set_current_heightis public, butset_fundingandbuild_signedboth overwrite the value with the wallet's processed height. Document that the wallet height wins or remove the redundant setter.
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] lines 317-364: Split set_funding/build_signed FFI breaks key-wallet's documented atomic read-then-reserve invariant.
- [SUGGESTION] lines 455-462: build_signed reclaims the builder before validating signer/output pointers. (carried forward)
- [NITPICK] lines 266-277: core_wallet_tx_builder_set_current_height is silently overridden by set_funding and build_signed.
- [NITPICK] lines 293-303: set_special_payload silently accepts trailing bytes after a valid bincode payload. (carried forward)
llbartekll
previously approved these changes
Jul 6, 2026
shumkov
previously approved these changes
Jul 6, 2026
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
from
9458b7a to
38cc69c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSafety docs omit the
account_typeinvariant.
CoreAccountTypeFFIis a#[repr(C)]enum crossing the FFI boundary; passing an out-of-range discriminant from the caller is undefined behavior beforeaccount_type.into()even runs. The# Safetysection only documentshandle. Consider documenting thataccount_typemust be a valid enum value (matching the sibling functions' convention).🤖 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/rs-platform-wallet-ffi/src/core_wallet/addresses.rs` around lines 63 - 64, The safety docs for the address FFI helper currently only mention `handle`, but the `account_type` argument also crosses the FFI boundary and must be a valid `CoreAccountTypeFFI` value. Update the `# Safety` section in the `addresses.rs` helper to explicitly state that `account_type` must be a valid enum discriminant, matching the convention used by the sibling address functions, so the contract is clear before `account_type.into()` is used.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs`:
- Around line 63-64: The safety docs for the address FFI helper currently only
mention `handle`, but the `account_type` argument also crosses the FFI boundary
and must be a valid `CoreAccountTypeFFI` value. Update the `# Safety` section in
the `addresses.rs` helper to explicitly state that `account_type` must be a
valid enum discriminant, matching the convention used by the sibling address
functions, so the contract is clear before `account_type.into()` is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b490f70-14f5-4ebc-a728-c3837ebc9fde
📒 Files selected for processing (12)
packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift
💤 Files with no reviewable changes (6)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
- packages/rs-platform-wallet/src/wallet/core/wallet.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
- packages/rs-platform-wallet/src/wallet/core/broadcast.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
from
38cc69c to
3ec2ace
Compare
thepastaclaw
reviewed
Jul 6, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus; claude general opus; codex ffi-engineer gpt-5.5; codex general gpt-5.5; verifier = claude opus; policy gate = review_policy.enforce_backport_prereq_policy.
Incremental + cumulative review for 3ec2ace1, including git diff 38cc69cb..3ec2ace1. The latest push is dominated by merged base-branch address-sync/height-pin work; the TransactionBuilder/broadcast surface from this PR is unchanged from 38cc69cb.
Prior finding reconciliation:
- prior-1: STILL VALID. CoinJoin funding still attaches account reservations, while CoinJoin broadcast still goes through the plain non-releasing path.
- prior-2: STILL VALID.
core_wallet_tx_builder_add_inputs_from_outpointsstill takes both a wallet and builder but lacks the builder-vs-wallet network guard used byset_fundingandbuild_signed.
Carried-forward prior findings:
- [SUGGESTION]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:72-85: CoinJoin-funded builds reserve UTXOs but broadcast never releases them on rejection. - [NITPICK]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:416-476:add_inputs_from_outpointsskips the wallet-vs-builder network check its sibling FFIs perform.
New findings in latest delta:
- None.
Result: 1 suggestion, 1 nitpick. No blockers.
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:72-85: CoinJoin-funded builds reserve UTXOs but broadcast never releases them on rejection
`CoreAccountTypeFFI::CoinJoin` funds via `core_wallet_tx_builder_set_funding`, which resolves the CoinJoin account through `managed_account_mut` (BIP44/BIP32/CoinJoin all share `ManagedCoreFundsAccount`) and calls `TransactionBuilder::set_funding(managed, account)` regardless of variant. That path attaches the account's `ReservationSet`, and `assemble_unsigned` inside `build_signed` reserves the selected inputs the same way it does for standard accounts. However, `as_standard_account_type` maps `CoinJoin` to `None`, and the `None` arm of `core_wallet_broadcast_transaction` invokes plain `broadcast_transaction`, never `broadcast_transaction_releasing_reservation`. On a definitive broadcast rejection for a CoinJoin-funded transaction, the reservation therefore stays held until the TTL backstop, blocking immediate retries. Either wire CoinJoin through reservation-releasing broadcast reconciliation or make `set_funding` skip attaching reservations for the CoinJoin variant.
- [NITPICK] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:416-476: `add_inputs_from_outpoints` skips the wallet-vs-builder network check its sibling FFIs perform
`core_wallet_tx_builder_set_funding` and `core_wallet_tx_builder_build_signed` both reject a builder whose `FFINetwork` does not match `wallet.network()`. `core_wallet_tx_builder_add_inputs_from_outpoints` also takes both the builder and a wallet handle but performs no such check before walking the account UTXO map and mutating the builder. `build_signed` re-catches the mismatch, so this cannot produce a wrong-network signed transaction, but one of the three wallet-aware FFI entry points silently mutates a foreign-network builder while the other two fail fast. Align the three entry points for consistency.
3ec2ace to
4c3dd04
Compare
thepastaclaw
reviewed
Jul 6, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus; claude general opus; claude rust-quality opus; codex ffi-engineer gpt-5.5; codex general gpt-5.5; codex rust-quality gpt-5.5; verifier = claude opus; specialists = rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
Prior reconciliation: prior-1 and prior-2 both remain STILL VALID at head 4c3dd04. Carried-forward findings: 2 (CoinJoin reservations not released on broadcast rejection; add_inputs_from_outpoints skips network guard). New findings in latest delta: none — the delta is a clean refactor migrating broadcast/address APIs from ManagedPlatformWallet to ManagedCoreWallet with matched Swift call-site updates and no new defects introduced.
Prior finding reconciliation:
- prior-1: STILL VALID. transaction_builder.rs is byte-identical between 3ec2ace and 4c3dd04. broadcast.rs line 48 still routes the None arm (CoinJoin via as_standard_account_type at line 82) through plain wallet.broadcast_transaction(&tx), while set_funding (line 393) still attaches the CoinJoin managed account's ReservationSet uniformly with BIP44/BIP32.
- prior-2: STILL VALID. core_wallet_tx_builder_set_funding (lines 358-365) and core_wallet_tx_builder_build_signed (lines 509-517) both enforce builder_network == wallet.network(); core_wallet_tx_builder_add_inputs_from_outpoints (lines 416-476) still resolves the wallet handle and mutates the builder without the equivalent guard.
Carried-forward prior findings:
- [SUGGESTION]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:72-85: CoinJoin-funded builds reserve UTXOs but broadcast never releases them on rejection - [NITPICK]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:416-476:add_inputs_from_outpointsskips the wallet-vs-builder network check its sibling FFIs perform
New findings in latest delta:
- None.
🟡 1 suggestion(s) | 💬 1 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`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:72-85: CoinJoin-funded builds reserve UTXOs but broadcast never releases them on rejection
The CoinJoin reserve/release contract is asymmetric across the FFI. `core_wallet_tx_builder_set_funding` routes all three account variants through `managed_account_mut`, which returns a `ManagedCoreFundsAccount` for CoinJoin identically to BIP44/BIP32, and `TransactionBuilder::set_funding(managed, account)` attaches that account's `ReservationSet` regardless of variant — so `build_signed` reserves the selected CoinJoin inputs. However, `as_standard_account_type` maps `CoinJoin` to `None` (line 82), and the `None` arm of `core_wallet_broadcast_transaction` (broadcast.rs:48) calls plain `wallet.broadcast_transaction(&tx)` instead of `broadcast_transaction_releasing_reservation`. Consequence: on a definitive broadcast rejection for a CoinJoin-funded transaction, Swift sees the failure and can retry, but Rust keeps the CoinJoin inputs reserved until the TTL backstop clears them, blocking that retry from reselecting those UTXOs. The doc comment at lines 73-77 (and broadcast.rs:16-17) frames CoinJoin as having no reservation to reconcile, which contradicts `set_funding`'s actual behavior. Fix by either wiring the CoinJoin arm through `broadcast_transaction_releasing_reservation` (extending reconciliation to CoinJoin) or by making `set_funding` skip attaching reservations for the CoinJoin variant so the reserve/release story stays symmetric.
4c3dd04 to
2bf1dff
Compare
ZocoLini
force-pushed
the
feat/core-tx-builder-ffi
branch
from
2bf1dff to
3161856
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/core/wallet.rs (1)
69-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated account-type dispatch logic.
The two
match account_type { BIP44 => ..., BIP32 => ..., CoinJoin => ... }blocks (xpub lookup and managed-account lookup) repeat the same dispatch pattern already implemented as standalone helpers (managed_account/managed_account_mut) inpackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs. Consider extracting a small private helper here (and/or moving the dispatch intokey_walletso both crates can share it) to avoid re-deriving the same match arms whenever a new account type is added.♻️ Example helper extraction
fn managed_account_mut<'a>( accounts: &'a mut ManagedAccountCollection, account_type: AccountTypePreference, account_index: u32, ) -> Option<&'a mut ManagedCoreFundsAccount> { match account_type { AccountTypePreference::BIP44 => accounts.standard_bip44_accounts.get_mut(&account_index), AccountTypePreference::BIP32 => accounts.standard_bip32_accounts.get_mut(&account_index), AccountTypePreference::CoinJoin => accounts.coinjoin_accounts.get_mut(&account_index), } }🤖 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/rs-platform-wallet/src/wallet/core/wallet.rs` around lines 69 - 119, The `set_gap_limit` method repeats the same `AccountTypePreference` dispatch twice for xpub and managed-account lookup, which should be centralized. Extract a small private helper in `wallet::core::wallet::set_gap_limit` (or share the existing `managed_account`/`managed_account_mut` pattern from `transaction_builder.rs`) so both lookups use one dispatch path, keeping the account-type matching in one place for easier maintenance when new account types are added.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/core/wallet.rs`:
- Around line 69-119: The `set_gap_limit` method repeats the same
`AccountTypePreference` dispatch twice for xpub and managed-account lookup,
which should be centralized. Extract a small private helper in
`wallet::core::wallet::set_gap_limit` (or share the existing
`managed_account`/`managed_account_mut` pattern from `transaction_builder.rs`)
so both lookups use one dispatch path, keeping the account-type matching in one
place for easier maintenance when new account types are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1d36b68-9b69-4cae-9ace-b16dbda3fb40
📒 Files selected for processing (10)
packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rspackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift
- packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
- packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift
- packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
- packages/rs-platform-wallet/src/wallet/core/broadcast.rs
shumkov
approved these changes
Jul 6, 2026
thepastaclaw
suggested changes
Jul 6, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers = claude general/rust-quality/ffi-engineer opus; codex general/rust-quality/ffi-engineer gpt-5.5; verifier = claude opus; specialists = rust-quality, ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
Incremental + cumulative review for 31618568, including the required latest delta git diff 2bf1dff7..31618568. That delta only touches packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs: it documents the intentional CoinJoin reservation behavior and adds the missing builder-vs-wallet network guard.
Prior finding reconciliation:
- prior-1: FIXED.
core_wallet_tx_builder_add_inputs_from_outpointsnow rejects builder/wallet network mismatches attransaction_builder.rs:430-443, matchingset_fundingandbuild_signed.
Carried-forward prior findings:
- None.
New findings in latest delta:
- None.
Cumulative current-head findings:
- [BLOCKING]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:421-493: Manual-input builds bypass UTXO reservations. - [BLOCKING]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:55-117: Validate FFI enum raw values before matching. - [BLOCKING]
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:96-189: Consumed builder can be reused through setters, passing a dangling Rust pointer. - [SUGGESTION]
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:170-598: Add FFI-level tests for the new transaction builder surface. - [NITPICK]
packages/rs-platform-wallet-ffi/src/dashpay.rs:527: Rustdoc link points to removed send API.
Result: 3 blocking, 1 suggestion, 1 nitpick.
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:421-493: Manual-input builds bypass UTXO reservations
`core_wallet_tx_builder_add_inputs_from_outpoints` validates that each outpoint belongs to the account, then calls key-wallet's `TransactionBuilder::add_inputs`, which only appends UTXOs. In key-wallet at rev 647fa98, `set_funding` is the only entry point that populates `self.reservations`; `assemble_unsigned` only reserves the selected inputs when `self.reservations` is `Some`. A builder funded solely through this new FFI/Swift `addInputs` path therefore returns a signed transaction without reserving those inputs, so a second build can immediately re-select the same UTXOs before broadcast, and the broadcast release-on-rejection path has nothing to release. This defeats the reservation guarantees this PR is explicitly adding for build-then-broadcast flows. Either populate the reservation set here (via the account's `ReservationSet`) so `assemble_unsigned` reserves the chosen inputs, or document that this entry point is only valid after `set_funding` and refuse to consume the builder otherwise.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:55-117: Validate FFI enum raw values before matching
`CoreAccountTypeFFI` and `CoreSelectionStrategyFFI` are new `#[repr(C)]` Rust enums accepted directly as parameters by exported C ABI functions (`core_wallet_tx_builder_set_selection_strategy`, `set_funding`, `add_inputs_from_outpoints`, `build_signed`, plus the neighboring broadcast/gap-limit FFIs). A foreign caller can pass any integer in an enum slot; an out-of-range discriminant is UB before `From`/`match` ever runs, not a recoverable `ErrorInvalidParameter`. This crate already documents and uses the safer pattern for exactly this reason — see `wallet_restore_types.rs:37-45` and `AccountTypeTagFFI::try_from_u8` — where raw `u8`/integer tags are carried across FFI and validated before matching. The new builder API should follow the same shape: take a raw integer parameter, validate with `try_from_*`, and return `ErrorInvalidParameter` on unknown values.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:170-598: Add FFI-level tests for the new transaction builder surface
The new `core_wallet_tx_builder_*` ABI surface uses manual `Box::from_raw` ownership transfer, the `take_builder`/`store_builder` in-place mutation pattern, pointer validation via `check_ptr!`, and the `FFICoreTransaction` free path. The new tests in `rs-platform-wallet/src/wallet/core/broadcast.rs` exercise the Rust wallet reservation behavior directly but bypass the FFI layer where most of the new Rust quality risk lives. A small FFI integration test driving create → configure → fund → build_signed → free, plus error paths (invalid/null pointers, network mismatch, double-build), would catch regressions in the ABI/lifetime code that ordinary wallet tests cannot.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:96-189: Consumed builder can be reused through setters, passing a dangling Rust pointer
`core_wallet_tx_builder_build_signed` at packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:517-518 consumes and frees both the `FFITransactionBuilder` and inner `TransactionBuilder` via `Box::from_raw`, and Swift sets `consumed = true` at line 216 before checking the result. However, only `buildSigned` guards on `consumed` (line 199). `setFunding`, `addInputs`, `addOutput`, `setChangeAddress`, `setFeeRate`, `setSelectionStrategy`, `setCurrentHeight`, and `setSpecialPayload` all continue to call their `core_wallet_tx_builder_*` counterparts with the same `handle` unconditionally. After any `buildSigned` — successful or throwing — a caller can hit any of these methods and pass a non-null but freed pointer into Rust. `check_ptr!` only rejects null, so Rust will dereference freed memory, causing UB or a crash. Guard every public mutating method on `consumed` before calling into Rust (throw the same 'already consumed' error).
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/dashpay.rs:527: Rustdoc link points to removed send API
This intra-doc link still points at `crate::core_wallet::core_wallet_send_to_addresses`, but this PR removes that FFI in favor of the transaction-builder flow (grep confirms the only remaining reference is this comment). `cargo doc` will now warn about a broken link, and readers are pointed to an API that no longer exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes