refactor(platform-wallet): expose the new core TransactionBuilder API + rust-dashcore bump#3970
refactor(platform-wallet): expose the new core TransactionBuilder API + rust-dashcore bump#3970ZocoLini wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR bumps Dash workspace dependency revisions, adds ChangesSigner trait extended_public_key completeness
Core wallet transaction builder and broadcast refactor
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
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
68065ab to
3159ebb
Compare
|
✅ Review complete (commit 909484a) |
3159ebb to
5265458
Compare
5265458 to
f265289
Compare
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:
|
f265289 to
b6969a7
Compare
thepastaclaw
left a comment
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.
b6969a7 to
5e3862d
Compare
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
left a comment
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.
5e3862d to
d55453f
Compare
d55453f to
909484a
Compare
thepastaclaw
left a comment
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
left a comment
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.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes