Skip to content
Draft
Show file tree
Hide file tree
Changes from all 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
31 changes: 31 additions & 0 deletions packages/rs-platform-wallet-ffi/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,34 @@ pub unsafe extern "C" fn platform_wallet_manager_destroy(
}
PlatformWalletFFIResult::ok()
}

/// Remove one wallet from the manager. Idempotent on missing wallets.
#[no_mangle]
pub unsafe extern "C" fn platform_wallet_manager_remove_wallet(
manager_handle: Handle,
wallet_id: *const [u8; 32],
) -> PlatformWalletFFIResult {
check_ptr!(wallet_id);
let wallet_id_value = *wallet_id;

let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(manager_handle, |manager| {
runtime().block_on(manager.remove_wallet(&wallet_id_value))
});
let result = unwrap_option_or_return!(option);
match result {
Ok(_) => PlatformWalletFFIResult::ok(),
// Idempotency: a wallet that's already gone is the success
// state callers want. Everything else is a real failure.
Err(platform_wallet::PlatformWalletError::WalletNotFound(_)) => {
PlatformWalletFFIResult::ok()
}
Err(e) => PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorWalletOperation,
format!(
"Failed to remove wallet {}: {}",
hex::encode(wallet_id_value),
e
),
),
}
}
159 changes: 158 additions & 1 deletion packages/rs-platform-wallet-ffi/src/persistence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use platform_wallet::changeset::{
};
use platform_wallet::wallet::platform_wallet::WalletId;
use platform_wallet::wallet::{PerAccountPlatformAddressState, PerWalletPlatformAddressState};
use std::collections::BTreeMap;
use std::collections::{BTreeMap, BTreeSet};
use std::ffi::CString;
use std::os::raw::c_void;
use std::slice;
Expand Down Expand Up @@ -362,17 +362,47 @@ pub struct PersistenceCallbacks {
unsafe impl Send for PersistenceCallbacks {}
unsafe impl Sync for PersistenceCallbacks {}

impl Default for PersistenceCallbacks {
fn default() -> Self {
Self {
context: std::ptr::null_mut(),
on_changeset_begin_fn: None,
on_changeset_end_fn: None,
on_store_fn: None,
on_flush_fn: None,
on_persist_address_balances_fn: None,
on_persist_wallet_changeset_fn: None,
on_persist_sync_state_fn: None,
on_persist_account_registrations_fn: None,
on_load_wallet_list_fn: None,
on_load_wallet_list_free_fn: None,
on_persist_wallet_metadata_fn: None,
on_persist_account_address_pools_fn: None,
on_persist_identities_fn: None,
on_persist_identity_keys_fn: None,
on_persist_token_balances_fn: None,
on_persist_contacts_fn: None,
on_get_core_tx_record_fn: None,
on_get_core_tx_record_free_fn: None,
}
}
}

/// In-memory persister that accumulates changesets and notifies via callbacks.
pub struct FFIPersister {
callbacks: PersistenceCallbacks,
pending: RwLock<BTreeMap<WalletId, PlatformWalletChangeSet>>,
/// Wallet ids removed from the manager. Stale handles may still call
/// `store`; registration metadata clears retirement for same-id reimport.
retired: RwLock<BTreeSet<WalletId>>,
}

impl FFIPersister {
pub fn new(callbacks: PersistenceCallbacks) -> Self {
Self {
callbacks,
pending: RwLock::new(BTreeMap::new()),
retired: RwLock::new(BTreeSet::new()),
}
}
}
Expand All @@ -383,6 +413,14 @@ impl PlatformWalletPersistence for FFIPersister {
wallet_id: WalletId,
changeset: PlatformWalletChangeSet,
) -> Result<(), PersistenceError> {
if wallet_id != WalletId::default() {
if changeset.wallet_metadata.is_some() {
self.retired.write().remove(&wallet_id);
} else if self.retired.read().contains(&wallet_id) {
return Ok(());
}
}
Comment on lines +416 to +422
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.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Retirement gate has a race window against concurrent retire_wallet.

store() only checks retired once up front, then runs callback persistence and pending merge later. If retire_wallet() runs between those points, stale writes can still be persisted for a just-retired wallet. This undermines the “drop post-delete writes” guarantee.

Consider a per-wallet lifecycle lock/state machine so retirement check + persistence path are coordinated atomically for non-registration writes.

🤖 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/persistence.rs` around lines 416 - 422,
store() currently only checks the retired set once and can persist changes if
retire_wallet() runs concurrently, allowing stale writes for just-retired
wallets; modify the persistence path to coordinate retirement and writes
atomically by introducing a per-wallet lifecycle lock or state machine (e.g., a
per-wallet mutex/atomic state) and use it in both store() and retire_wallet() so
that store() acquires the wallet lock/state before running the callback and
verifies retirement state again (or aborts) before committing pending merges;
ensure you reference and protect the same retired set and the code paths that
examine changeset.wallet_metadata and perform pending merges so no post-retire
writes can be committed.


// Bracket the whole per-kind callback sequence with a
// begin/end pair so clients (Swift, etc.) can treat the
// round as a single atomic transaction: begin opens a
Expand Down Expand Up @@ -909,6 +947,10 @@ impl PlatformWalletPersistence for FFIPersister {
}

fn flush(&self, wallet_id: WalletId) -> Result<(), PersistenceError> {
if wallet_id != WalletId::default() && self.retired.read().contains(&wallet_id) {
return Ok(());
}

// Notify caller.
if let Some(cb) = self.callbacks.on_flush_fn {
let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr()) };
Expand All @@ -926,6 +968,15 @@ impl PlatformWalletPersistence for FFIPersister {
Ok(())
}

fn retire_wallet(&self, wallet_id: WalletId) {
if wallet_id == WalletId::default() {
return;
}

self.pending.write().remove(&wallet_id);
self.retired.write().insert(wallet_id);
}

fn load(&self) -> Result<ClientStartState, PersistenceError> {
// If Swift hasn't wired up `on_load_wallet_list_fn` there's
// nothing to restore — treat as a fresh client.
Expand Down Expand Up @@ -2287,3 +2338,109 @@ unsafe fn slice_from_raw<'a>(ptr: *const u8, len: usize) -> &'a [u8] {
slice::from_raw_parts(ptr, len)
}
}

#[cfg(test)]
mod tests {
use super::*;

use platform_wallet::changeset::WalletMetadataEntry;
use std::sync::atomic::{AtomicUsize, Ordering};

struct CallbackCounts {
metadata: AtomicUsize,
stores: AtomicUsize,
flushes: AtomicUsize,
}

impl CallbackCounts {
fn new() -> Self {
Self {
metadata: AtomicUsize::new(0),
stores: AtomicUsize::new(0),
flushes: AtomicUsize::new(0),
}
}
}

unsafe extern "C" fn metadata_callback(
context: *mut c_void,
_wallet_id: *const u8,
_network: FFINetwork,
_birth_height: u32,
) -> i32 {
let counts = &*(context as *const CallbackCounts);
counts.metadata.fetch_add(1, Ordering::SeqCst);
0
}

unsafe extern "C" fn store_callback(context: *mut c_void, _wallet_id: *const u8) -> i32 {
let counts = &*(context as *const CallbackCounts);
counts.stores.fetch_add(1, Ordering::SeqCst);
0
}

unsafe extern "C" fn flush_callback(context: *mut c_void, _wallet_id: *const u8) -> i32 {
let counts = &*(context as *const CallbackCounts);
counts.flushes.fetch_add(1, Ordering::SeqCst);
0
}

fn callbacks(counts: &CallbackCounts) -> PersistenceCallbacks {
PersistenceCallbacks {
context: counts as *const CallbackCounts as *mut c_void,
on_persist_wallet_metadata_fn: Some(metadata_callback),
on_store_fn: Some(store_callback),
on_flush_fn: Some(flush_callback),
..Default::default()
}
}

#[test]
fn retired_wallet_drops_non_registration_store_and_flush() {
let counts = CallbackCounts::new();
let persister = FFIPersister::new(callbacks(&counts));
let wallet_id = [7u8; 32];

persister
.store(wallet_id, PlatformWalletChangeSet::default())
.unwrap();
assert_eq!(counts.stores.load(Ordering::SeqCst), 1);
assert!(persister.pending.read().contains_key(&wallet_id));

persister.retire_wallet(wallet_id);
assert!(!persister.pending.read().contains_key(&wallet_id));

persister
.store(wallet_id, PlatformWalletChangeSet::default())
.unwrap();
persister.flush(wallet_id).unwrap();

assert_eq!(counts.metadata.load(Ordering::SeqCst), 0);
assert_eq!(counts.stores.load(Ordering::SeqCst), 1);
assert_eq!(counts.flushes.load(Ordering::SeqCst), 0);
}

#[test]
fn registration_store_clears_retirement() {
let counts = CallbackCounts::new();
let persister = FFIPersister::new(callbacks(&counts));
let wallet_id = [8u8; 32];

persister.retire_wallet(wallet_id);

let registration = PlatformWalletChangeSet {
wallet_metadata: Some(WalletMetadataEntry {
network: key_wallet::Network::Testnet,
birth_height: 42,
}),
..Default::default()
};
persister.store(wallet_id, registration).unwrap();
persister.flush(wallet_id).unwrap();

assert_eq!(counts.metadata.load(Ordering::SeqCst), 1);
assert_eq!(counts.stores.load(Ordering::SeqCst), 1);
assert_eq!(counts.flushes.load(Ordering::SeqCst), 1);
assert!(!persister.retired.read().contains(&wallet_id));
}
}
7 changes: 7 additions & 0 deletions packages/rs-platform-wallet/src/changeset/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ pub trait PlatformWalletPersistence: Send + Sync {
/// clear that wallet's buffer.
fn flush(&self, wallet_id: WalletId) -> Result<(), PersistenceError>;

/// Stop accepting future writes for a wallet removed from the manager.
///
/// Persisters that can receive callbacks from stale wallet handles should
/// drop any buffered state and ignore later non-registration writes for this
/// id. Backends without an in-memory callback gate can use the default no-op.
fn retire_wallet(&self, _wallet_id: WalletId) {}

/// Load the full client state from storage.
///
/// Returns a [`ClientStartState`] — a ready-to-boot snapshot covering
Expand Down
Loading
Loading