Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions packages/rs-platform-wallet-ffi/src/asset_lock/built_callback.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
//! C callback that reports the outpoint of a newly built asset lock.

use std::os::raw::c_void;

use dashcore::OutPoint;
use platform_wallet::wallet::asset_lock::AssetLockBuiltObserver;

use crate::asset_lock_persistence::outpoint_to_bytes;

/// Receives the outpoint of an asset lock that a funding call has just
/// built, before the lock's row is persisted or the lock is broadcast.
///
/// `out_point_36` points to 36 bytes (32-byte raw txid followed by the
/// 4-byte little-endian vout, the layout of `AssetLockEntryFFI.out_point`)
/// and is valid only for the duration of the call. The callback runs
/// synchronously on the funding call's worker thread, at most once per
/// call, and must return promptly without calling back into the wallet.
/// Record the outpoint before returning: the lock's row is persisted right
/// after. The funding call's result then tells what happened to the lock:
/// `ErrorTransactionBroadcastRejected` (26) means it was removed again; any
/// other failure leaves it tracked and resumable by this outpoint.
/// `NULL` means no callback.
pub type AssetLockBuiltFn =
Option<unsafe extern "C" fn(context: *mut c_void, out_point_36: *const u8)>;

/// A caller-supplied [`AssetLockBuiltFn`] with its opaque context.
///
/// The context is held as `usize` so the funding future that captures it
/// stays `Send + 'static`; the caller's contract keeps it valid for the
/// whole (blocking) FFI call.
#[derive(Clone, Copy)]
pub(crate) struct AssetLockBuiltCallback {
func: unsafe extern "C" fn(context: *mut c_void, out_point_36: *const u8),
context: usize,
}

impl AssetLockBuiltCallback {
/// `None` when the caller passed no callback.
pub(crate) fn new(func: AssetLockBuiltFn, context: *mut c_void) -> Option<Self> {
func.map(|func| Self {
func,
context: context as usize,
})
}

/// The callback as the observer the funding paths take.
pub(crate) fn into_observer(self) -> impl Fn(OutPoint) + Send + Sync {
move |out_point| self.notify(out_point)
}

fn notify(&self, out_point: OutPoint) {
let bytes = outpoint_to_bytes(&out_point);
// SAFETY: the caller guarantees `func` and `context` stay valid for
// the duration of the FFI call this callback belongs to, and `bytes`
// outlives the call below.
unsafe { (self.func)(self.context as *mut c_void, bytes.as_ptr()) }
}
}

/// `on_built` as the optional observer reference the funding paths take.
pub(crate) fn observer_ref<F>(on_built: &Option<F>) -> Option<&AssetLockBuiltObserver<'_>>
where
F: Fn(OutPoint) + Send + Sync,
{
on_built
.as_ref()
.map(|notify| notify as &AssetLockBuiltObserver<'_>)
}

#[cfg(test)]
mod tests {
use super::*;
use dashcore::hashes::Hash;
use dashcore::Txid;
use std::sync::Mutex;

/// Appends the reported outpoint to the `Mutex<Vec<[u8; 36]>>` the
/// context points to.
unsafe extern "C" fn record(context: *mut c_void, out_point_36: *const u8) {
let received = &*(context as *const Mutex<Vec<[u8; 36]>>);
let mut bytes = [0u8; 36];
std::ptr::copy_nonoverlapping(out_point_36, bytes.as_mut_ptr(), 36);
received.lock().unwrap().push(bytes);
}

#[test]
fn should_yield_no_callback_for_an_absent_function() {
assert!(AssetLockBuiltCallback::new(None, std::ptr::null_mut()).is_none());
}

#[test]
fn should_pass_the_context_and_the_entry_layout_outpoint() {
let received = Mutex::new(Vec::new());
let out_point = OutPoint::new(Txid::from_byte_array([7; 32]), 3);
let callback = AssetLockBuiltCallback::new(
Some(record),
&received as *const Mutex<Vec<[u8; 36]>> as *mut c_void,
)
.expect("callback present");

let on_built = Some(callback.into_observer());
observer_ref(&on_built).expect("observer present")(out_point);

assert_eq!(
received.into_inner().unwrap(),
vec![outpoint_to_bytes(&out_point)]
);
}
}
3 changes: 3 additions & 0 deletions packages/rs-platform-wallet-ffi/src/asset_lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
//! Mirrors the structure of `platform_wallet::wallet::asset_lock`.

mod build;
mod built_callback;
mod manager;
mod sync;

pub use build::*;
pub use built_callback::AssetLockBuiltFn;
pub(crate) use built_callback::{observer_ref, AssetLockBuiltCallback};
pub use manager::*;
pub use sync::*;
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
//! resuming a tracked asset lock by outpoint (crash-recovery shape).

use std::collections::BTreeMap;
use std::os::raw::c_void;

use dashcore::hashes::Hash;
use dpp::address_funds::PlatformAddress;
use platform_wallet::wallet::asset_lock::AssetLockFunding;
use rs_sdk_ffi::{MnemonicResolverCoreSigner, MnemonicResolverHandle, SignerHandle, VTableSigner};

use crate::asset_lock::{observer_ref, AssetLockBuiltCallback, AssetLockBuiltFn};
use crate::check_ptr;
use crate::core_wallet_types::OutPointFFI;
use crate::error::*;
Expand Down Expand Up @@ -65,6 +67,10 @@ use crate::{unwrap_option_or_return, unwrap_result_or_return};
/// `*mut MnemonicResolverHandle` produced by
/// [`crate::dash_sdk_mnemonic_resolver_create`]. The caller retains
/// ownership.
/// - `on_asset_lock_built`, when non-null, must be a valid C function
/// pointer for the duration of the call; `on_asset_lock_built_context`
/// is passed to it opaquely and must remain valid for the duration of the
/// call (or be null). See [`AssetLockBuiltFn`] for its contract.
#[no_mangle]
#[allow(clippy::too_many_arguments)]
pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_signer(
Expand All @@ -79,6 +85,8 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_signer(
signer_address_handle: *mut SignerHandle,
core_signer_handle: *mut MnemonicResolverHandle,
out_changeset: *mut PlatformAddressChangeSetFFI,
on_asset_lock_built: AssetLockBuiltFn,
on_asset_lock_built_context: *mut c_void,
) -> PlatformWalletFFIResult {
Comment on lines 76 to 90

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.

🔴 Blocking: Do not change existing C export signatures in place

The existing exported symbol platform_address_wallet_fund_from_asset_lock_signer now requires AssetLockBuiltFn and its context, and the same in-place ABI change is made to platform_address_wallet_fund_from_asset_lock_external_signer and the two shielded funding exports. A host compiled against the previous C prototype will still call the unchanged symbol with the old argument count. The new callee then reads the callback and context from registers or stack locations that the old caller did not provide; if the callback value is non-null, AssetLockBuiltCallback::notify can call an arbitrary address, causing undefined behavior or a process crash. Marking the PR as breaking and telling source callers to pass NULL, NULL does not make an already-built binary compatible. Preserve the old symbols with their old signatures and add versioned/new symbols for the callback-enabled ABI, or introduce an explicit ABI version boundary and update rebuilt Swift/JNI callers to use it.

source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changing this one: these exports have no separately built consumer, so the old-binary-on-new-library case does not occur.

  • Swift links librs_unified_sdk_ffi.a from DashSDKFFI.xcframework, which build_ios.sh builds from one tree together with the cbindgen header that Swift compiles against. The released xcframework zip ships both, and dashwallet-ios builds it at its .platform-ref. A mismatched prototype is a compile error there.
  • Kotlin reaches these functions through rs-unified-sdk-jni, which calls them as Rust functions over the rlib (already updated in this PR). The Java_* entry points Kotlin calls keep their signatures, and build_android.sh drops the standalone libplatform_wallet_ffi.so.
  • Nothing in the repository loads the cdylib dynamically.

The repository does not keep these C prototypes stable across builds. The coding conventions treat even removing an extern "C" function as ordinary work described in the PR body, and there is precedent on these very exports: #3793 added surplus_output_ptr/surplus_output_len to platform_wallet_manager_shielded_fund_from_asset_lock in place, and #5649 replaced dash_sdk_derive_connect_key_with_resolver without keeping the old symbol. Versioned duplicates here would add four exports that no matching build calls. Binary stability for the FFI would be a crate-wide policy, which is worth an issue of its own if maintainers want it. This change is listed under "C ABI" in the description.

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.

The bundled Swift static library and the JNI path are rebuilt together, so those in-tree consumers are not the counterexample. However, platform-wallet-ffi still declares staticlib, cdylib, and rlib, and its README explicitly documents consuming the generated .so/.dylib from C or C++; those consumers can load a newer library with an older caller and are not protected by the synchronized build. The prior in-place changes do not establish ABI compatibility, and marking this release as breaking does not prevent the old symbol from being called with the old argument list, so the existing exports still need to retain their signatures or be supplemented with new symbols.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The README's C/C++ section builds the library from source, and the same build runs cbindgen (build.rs) to regenerate platform_wallet_ffi.h, so a consumer that follows it gets the new prototype along with the new library. No release publishes libplatform_wallet_ffi.so/.dylib: release-swift-sdk.yml ships the xcframework (static library plus header) and release-kotlin-sdk.yml ships the Android AAR, whose libdash_sdk_jni.so is called only by the Kotlin classes packaged with it. Neither artifact lets an older caller resolve a newer library.

The repository also does not treat these exports as a stable ABI: the coding conventions (book/src/contributing/coding-conventions.md, "Commits, pull requests") class removing an extern "C" function as ordinary feat:/refactor: work described in the PR body. A changed signature is no stronger a break than a removed symbol. The error-code registry is a different case: those integers are mirrored by hand in Swift and Kotlin, so a renumbering compiles cleanly and is misread. A changed prototype fails to compile against the regenerated header.

A versioned C ABI for platform-wallet-ffi (old symbols kept as wrappers, suffixed callback-capable ones added) would be a policy for the whole crate and should be settled with the maintainers in its own issue, not introduced for four functions here. I'm leaving this thread open for them.

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.

The synchronized Swift/JNI builds protect only callers rebuilt from this checkout; they do not protect a previously compiled C/C++ object or an application that links an older object against a newly built static or dynamic library. This crate explicitly produces staticlib and cdylib, and its README documents C/C++ consumption, so regenerating the header makes a source rebuild fail or succeed correctly but does not make the existing symbol ABI-safe for already-compiled callers. Prior in-place breaks are precedent for the same compatibility problem, not evidence that this change is safe; these exports still need old-signature wrappers or distinct callback-enabled symbols.

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.

Weighing in as an rs-platform-wallet-ffi owner: I agree with Roman and don't consider this blocking.

  • The repository does not treat the C prototypes as a stable ABI. book/src/contributing/coding-conventions.md classes removing an extern "C" function as ordinary feat:/refactor: work, and feat(drive): shielded fees for Shield/ShieldFromAssetLock + shield credit conservation #3793 already appended surplus_output_ptr/surplus_output_len to platform_wallet_manager_shielded_fund_from_asset_lock in place.
  • Nothing ships the cdylib on its own. The Swift release carries the xcframework and the cbindgen header built from one tree, and Kotlin reaches these functions through the JNI crate over the rlib. A caller built against the old header fails to compile against the new one; it cannot resolve the new symbol with the old arity.
  • ERROR_CODE_REGISTRY.md's "shipped host binary" rule is about the integer codes, which Swift and Kotlin mirror by hand and which renumber silently. That is a different failure mode from a changed prototype.

If maintainers want versioned C exports for this crate, that's a crate-wide policy and belongs in its own issue, not in this PR.

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.

I still consider this blocking. The repository itself documents the opposite compatibility assumption in core_wallet_signed_payment_finalize: it explicitly preserves an old C signature because a host's compiled code and DashSDKFFI.xcframework can meet at different versions, and warns that adding an argument in place can make the callee treat an absent argument as an address. That applies to these exports as well; the README documents both staticlib and independently consumable cdylib outputs, so rebuilding the generated header only protects source rebuilds, not an already-compiled C/C++ caller or separately packaged binary. The prior in-place change is precedent for the same ABI hazard, not evidence that this four-export change is safe; please retain wrappers with the old signatures or add distinct callback-enabled symbols.

check_ptr!(out_changeset);
// Sentinel first: address decoding, the wallet lookup, and the async
Expand All @@ -98,6 +106,8 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_signer(
// capture is `Send + 'static` (raw pointers are `!Send`).
let signer_addr = signer_address_handle as usize;
let core_signer_addr = core_signer_handle as usize;
let built_callback =
AssetLockBuiltCallback::new(on_asset_lock_built, on_asset_lock_built_context);

let option = PLATFORM_ADDRESS_WALLET_STORAGE.with_item(handle, |wallet| {
let wallet_clone = wallet.clone();
Expand All @@ -117,6 +127,7 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_signer(
network,
)
};
let on_built = built_callback.map(AssetLockBuiltCallback::into_observer);
wallet_clone
.fund_from_asset_lock(
AssetLockFunding::FromWalletBalance {
Expand All @@ -132,6 +143,7 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_signer(
address_signer,
&asset_lock_signer,
None,
observer_ref(&on_built),
)
.await
})
Expand Down Expand Up @@ -242,6 +254,8 @@ pub unsafe extern "C" fn platform_address_wallet_resume_fund_from_asset_lock_sig
address_signer,
&asset_lock_signer,
None,
// A resume builds no new lock.
None,
)
.await
})
Expand Down Expand Up @@ -365,6 +379,10 @@ pub(super) unsafe fn decode_funding_addresses(
/// - `handle` must be a live `PlatformAddressWallet` handle; a stale or
/// already-destroyed one is rejected by the handle table rather than
/// dereferenced.
/// - `on_asset_lock_built`, when non-null, must be a valid C function
/// pointer for the duration of the call; `on_asset_lock_built_context`
/// is passed to it opaquely and must remain valid for the duration of the
/// call (or be null). See [`AssetLockBuiltFn`] for its contract.
#[no_mangle]
#[allow(clippy::too_many_arguments)]
pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_external_signer(
Expand All @@ -379,6 +397,8 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_external_s
signer_address_handle: *mut SignerHandle,
core_signer_handle: *mut MnemonicResolverHandle,
out_changeset: *mut PlatformAddressChangeSetFFI,
on_asset_lock_built: AssetLockBuiltFn,
on_asset_lock_built_context: *mut c_void,
) -> PlatformWalletFFIResult {
check_ptr!(out_changeset);
// Sentinel first — see `platform_address_wallet_fund_from_asset_lock_signer`.
Expand All @@ -394,6 +414,8 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_external_s

let signer_addr = signer_address_handle as usize;
let core_signer_addr = core_signer_handle as usize;
let built_callback =
AssetLockBuiltCallback::new(on_asset_lock_built, on_asset_lock_built_context);

let option = PLATFORM_ADDRESS_WALLET_STORAGE.with_item(handle, |wallet| {
let wallet_clone = wallet.clone();
Expand All @@ -410,6 +432,7 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_external_s
network,
)
};
let on_built = built_callback.map(AssetLockBuiltCallback::into_observer);
wallet_clone
.fund_from_asset_lock_external(
AssetLockFunding::FromWalletBalance {
Expand All @@ -421,6 +444,7 @@ pub unsafe extern "C" fn platform_address_wallet_fund_from_asset_lock_external_s
address_signer,
&asset_lock_signer,
None,
observer_ref(&on_built),
)
.await
})
Expand Down Expand Up @@ -573,6 +597,8 @@ pub unsafe extern "C" fn platform_address_wallet_resume_fund_from_asset_lock_ext
address_signer,
&asset_lock_signer,
None,
// A resume builds no new lock.
None,
)
.await
})
Expand Down
Loading
Loading