Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 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
124 changes: 94 additions & 30 deletions walletkit-core/src/storage/credential_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,7 @@ impl CredentialStore {
/// not exist.
pub fn delete_credential(&self, credential_id: u64) -> StorageResult<()> {
self.lock_inner()?.delete_credential(credential_id)?;
self.notify_vault_changed();
Ok(())
self.notify_vault_changed()
}

/// Stores a credential and optional associated data.
Expand All @@ -214,7 +213,7 @@ impl CredentialStore {
associated_data,
now,
)?;
self.notify_vault_changed();
self.notify_vault_changed()?;
Ok(id)
}

Expand Down Expand Up @@ -284,15 +283,15 @@ impl CredentialStore {
let count = inner.danger_delete_all_credentials()?;
drop(inner);
if count > 0 {
self.notify_vault_changed();
self.notify_vault_changed()?;
}
Ok(count)
}

/// Registers a backup manager that will be notified after vault mutations
/// ([`store_credential`](Self::store_credential),
/// [`delete_credential`](Self::delete_credential),
/// [`danger_delete_all_credentials`](Self::danger_delete_all_credentials)).
/// Backup failures are logged but do not affect the mutation result.
///
/// # Errors
///
Expand All @@ -306,6 +305,21 @@ impl CredentialStore {
})? = manager;
Ok(())
}

/// Triggers a one-off backup sync without mutating the vault.
///
/// Call this at initial backup creation to catch up any credentials
/// that were stored before the backup was set up. The registered
/// [`WalletKitBackupManager`] receives the same `on_vault_changed`
/// callback as after a normal vault mutation.
///
/// # Errors
///
/// Returns an error if no backup manager is configured, if the vault
/// export fails, or if the backup manager callback fails.
pub fn sync_backup(&self) -> StorageResult<()> {
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.

Hmm this is creating a cyclical pattern of Foreign -> Rust -> Foreign (and potentially more bindings after). Is this initial sync necessary? If there are no credentials there should be nothing to backup right?

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.

Just to be clear - it's the same case with regular credential store calls - they're also Foregin -> Rust -> Foregin

Copy link
Copy Markdown
Contributor Author

@thomas-waite thomas-waite Mar 16, 2026

Choose a reason for hiding this comment

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

Yep, the existing backup solution in Oxide is already doing this. I'm following the same pattern. The initial sync is necessary to backup whatever v4 credentials have already been put into the credential vault when the user enables backups (the callback based backup logic is only triggered when the vault is mutated)

self.notify_vault_changed()
}
}

/// Implementation not exposed to foreign bindings
Expand All @@ -318,40 +332,38 @@ impl CredentialStore {
.map_err(|_| StorageError::Lock("storage mutex poisoned".to_string()))
}

/// Best-effort export + notification to the backup manager, if one is set.
/// Exports a plaintext vault snapshot and notifies the registered backup
/// manager via [`on_vault_changed`](WalletKitBackupManager::on_vault_changed).
///
/// Called after vault mutations and by [`sync_backup`](Self::sync_backup).
/// Returns `Ok(())` if no backup manager is configured (noop).
///
/// Called after any vault mutation (store, delete) so the host app can
/// sync the updated vault to its backup. Failures are logged but never
/// propagated — the vault mutation has already succeeded and callers
/// should not see an error from a backup side-effect.
fn notify_vault_changed(&self) {
/// **Note:** errors from the backup callback are propagated to the caller.
/// Because this runs *after* the vault mutation has been committed, a
/// returned `Err` does not mean the mutation failed — only the backup
/// notification. Callers should inspect the error and handle it according
/// to its nature (e.g. log it, schedule a backup retry via
/// [`sync_backup`](Self::sync_backup), or surface it to the user) rather
/// than retrying the already-committed mutation.
fn notify_vault_changed(&self) -> StorageResult<()> {
// Hold the backup lock for the entire export+callback path. This
// serializes concurrent notifications so backups are delivered in
// mutation order. Recover the guard on poison — the manager is
// still valid after a prior panic.
let guard = self
.backup
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
// mutation order.
let guard = self.backup.lock().map_err(|_| {
StorageError::Lock("backup config mutex poisoned".to_string())
})?;

let dest_dir = guard.dest_dir();
if dest_dir.is_empty() {
return; // NoopBackupManager — nothing to do.
return Ok(()); // NoopBackupManager — nothing to do.
}

// Export a plaintext snapshot of the vault. The file is sensitive
// (unencrypted), so we wrap it in a guard that deletes it on drop —
// no matter how we exit (normal return, early return, or panic).
let vault_path = match self
let vault_path = self
.lock_inner()
.and_then(|inner| inner.export_vault_for_backup(&dest_dir))
{
Ok(path) => path,
Err(e) => {
tracing::error!("Failed to export vault for backup: {e}");
return;
}
};
.and_then(|inner| inner.export_vault_for_backup(&dest_dir))?;

let _cleanup = {
struct CleanupFile(String);
Expand All @@ -371,9 +383,7 @@ impl CredentialStore {
// Hand the path to the host app (e.g. iOS) so it can copy/upload
// the vault to Bedrock. The host must finish with the file during
// this synchronous call — the guard deletes it on return.
if let Err(e) = guard.on_vault_changed(vault_path) {
tracing::error!("Backup manager on_vault_changed failed: {e}");
}
guard.on_vault_changed(vault_path)
}

/// Retrieves a full credential including raw bytes by issuer schema ID.
Expand Down Expand Up @@ -1508,4 +1518,58 @@ mod tests {
cleanup_test_storage(&root);
cleanup_test_storage(&export_dir);
}

#[test]
fn test_sync_backup_triggers_notification_without_mutation() {
use world_id_core::Credential as CoreCredential;

let (store, manager, root, export_dir) = setup_store_with_backup();

// Store a credential so the vault is non-empty.
let cred: Credential = CoreCredential::new()
.issuer_schema_id(100)
.genesis_issued_at(1000)
.into();
store
.store_credential(&cred, &FieldElement::from(7u64), 9999, None, 1000)
.expect("store credential");
assert_eq!(manager.call_count(), 1);

// sync_backup triggers a notification without mutating the vault.
store.sync_backup().expect("sync_backup");
assert_eq!(manager.call_count(), 2);
assert!(
manager.last_file_existed(),
"exported vault file should exist during the callback"
);
let path = manager.last_path().unwrap();
assert!(
!std::path::Path::new(&path).exists(),
"exported vault file should be cleaned up after the callback"
);

// Vault contents are unchanged — still the same credential.
let credentials = store.list_credentials(None, 1000).expect("list");
assert_eq!(credentials.len(), 1);
assert_eq!(credentials[0].issuer_schema_id, 100);

cleanup_test_storage(&root);
cleanup_test_storage(&export_dir);
}

#[test]
fn test_sync_backup_on_empty_vault() {
let (store, manager, root, export_dir) = setup_store_with_backup();

// sync_backup on an empty vault should still trigger a notification.
store.sync_backup().expect("sync_backup");
assert_eq!(manager.call_count(), 1);
assert!(
manager.last_file_existed(),
"exported vault file should exist during the callback"
);

cleanup_test_storage(&root);
cleanup_test_storage(&export_dir);
}
}
14 changes: 10 additions & 4 deletions walletkit-core/src/storage/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,15 @@ pub trait WalletKitBackupManager: Send + Sync {
///
/// # Errors
///
/// Returning `Err` is treated as best-effort — the error is logged but
/// does not affect the vault mutation that triggered this call. Returning
/// `Result` (rather than `()`) ensures that host-side exceptions are
/// translated into a Rust `Err` by `UniFFI` instead of panicking.
/// Returning `Err` propagates the error to the caller of the vault
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.

It's unrelated to this PR but I'm realizing this could lead to very weird states where a credential has been saved to the vault, backup fails and an error is returned. The native app won't know the credential is saved and retry which will error. We should split the backup failures as error logs and/or return a separate status for this, but not fail the entire original call

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.

Yeah agree, should have a seperate status for this/error logs. Will do

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.

done

/// mutation (e.g. `store_credential`, `delete_credential`). Because the
/// vault mutation has already been committed by the time this callback
/// runs, callers should be aware that an `Err` result does **not** mean
/// the mutation failed — only that the backup notification failed.
/// Callers should inspect the returned error and handle it according to
/// its nature (e.g. log it, schedule a backup retry via
/// [`sync_backup`](crate::storage::CredentialStore::sync_backup), or
/// surface it to the user) rather than retrying the already-committed
/// mutation.
fn on_vault_changed(&self, vault_file_path: String) -> StorageResult<()>;
}
Loading