Skip to content

fix(platform-wallet): make post-acceptance identity bookkeeping best-effort #4011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
QuantumExplorer merged 1 commit into from
Jul 5, 2026
Merged

fix(platform-wallet): make post-acceptance identity bookkeeping best-effort #4011

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
135 changes: 93 additions & 42 deletions packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ impl IdentityWallet {
/// (`InvalidInstantAssetLockProofSignatureError`).
/// 4. On success, add the confirmed identity to the local
/// `IdentityManager` and record each key's derivation breadcrumb.
/// Best-effort: Platform has already accepted, so a local
/// bookkeeping failure is logged, not propagated.
/// 5. Remove the tracked asset lock (if any) — the credit output
/// has been consumed, so the entry is no longer needed.
///
Expand Down Expand Up @@ -262,40 +264,71 @@ impl IdentityWallet {
Err(e) => return Err(PlatformWalletError::Sdk(e)),
};

// Step 4: bookkeeping — add to local IdentityManager + record
// key derivation breadcrumbs.
// Step 4 (best-effort): bookkeeping — add to local
// IdentityManager + record key derivation breadcrumbs.
//
// Platform has ALREADY accepted the registration, so a local
// bookkeeping failure must NOT propagate as `Err` — the caller
// would report failure for an identity that exists on chain,
// and the early return would skip Step 5's `consume_asset_lock`,
// leaving the spent lock in the Resumable Funding list where a
// Resume gets Platform's deterministic "lock already consumed"
// rejection. A missed local add self-heals on the next identity
// re-sync. This mirrors `register_from_addresses` Step 3.
{
use dpp::identity::accessors::IdentityGettersV0;

let mut wm = self.wallet_manager.write().await;
let info = wm.get_wallet_info_mut(&self.wallet_id).ok_or_else(|| {
PlatformWalletError::WalletNotFound(
"Wallet info not found in wallet manager".to_string(),
)
})?;
info.identity_manager.add_identity(
identity.clone(),
identity_index,
self.wallet_id,
&self.persister,
)?;
match wm.get_wallet_info_mut(&self.wallet_id) {
Some(info) => match info.identity_manager.add_identity(
identity.clone(),
identity_index,
self.wallet_id,
&self.persister,
) {
Ok(()) => {
let wallet_id = self.wallet_id;
let identity_id = identity.id();
let public_keys: Vec<(KeyID, IdentityPublicKey)> = identity
.public_keys()
.iter()
.map(|(k, v)| (*k, v.clone()))
.collect();

let wallet_id = self.wallet_id;
let identity_id = identity.id();
let public_keys: Vec<(KeyID, IdentityPublicKey)> = identity
.public_keys()
.iter()
.map(|(k, v)| (*k, v.clone()))
.collect();

if let Some(managed) = info.identity_manager.managed_identity_mut(&identity_id) {
managed.wallet_id = Some(wallet_id);
for (key_id, pub_key) in public_keys {
let key_index = key_id;
managed.add_key(
pub_key,
Some((wallet_id, identity_index, key_index)),
&self.persister,
if let Some(managed) =
info.identity_manager.managed_identity_mut(&identity_id)
{
managed.wallet_id = Some(wallet_id);
for (key_id, pub_key) in public_keys {
let key_index = key_id;
managed.add_key(
pub_key,
Some((wallet_id, identity_index, key_index)),
&self.persister,
);
}
}
}
Err(e) => {
// Breadcrumbs are skipped too: `IdentityAlreadyExists`
// can mean an out-of-wallet entry, and stamping
// `wallet_id` on one without moving buckets would
// contradict the manager's location index.
tracing::warn!(
error = %e,
identity_id = %identity.id(),
"register_identity_with_funding: identity registered on \
Platform but local add_identity failed; continuing so \
the spent asset lock is still consumed"
);
}
Comment on lines +312 to +324

@thepastaclaw thepastaclaw Jul 5, 2026

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.

🟡 Suggestion: Step 4 Err arm swallows every add_identity failure, not just IdentityAlreadyExists

The catch-all Err(e) arm at line 312 warns and continues for every error from add_identity, but the inline justification comment only reasons about IdentityAlreadyExists. Two consequences worth acknowledging in the comment or the match shape:

  1. add_identity returns IdentityAlreadyExists whether the pre-existing entry is in the wallet-owned bucket or the out-of-wallet bucket (verified: lifecycle.rs:45 guards on self.identity(...).is_some(), and accessors.rs:21-32 scans both buckets). For the out-of-wallet duplicate case, this PR now returns Ok(identity) while location_index still reports OutOfWallet, so a follow-up top_up_identity_with_funding on the same id will fail with IdentityIndexNotSet (identity_index returns None for the OOW discriminant at lifecycle.rs:144-149). The comment claims this self-heals on re-sync, but the current refresh/discovery paths mutate the existing managed identity rather than moving buckets, so it does not.
  2. For non-IdentityAlreadyExists variants (e.g. an internal persister propagation error surfaced by a future signature change), silently swallowing is not obviously the desired posture — the location-index reasoning does not apply.

Either discriminate on the error variant (promote/replace on IdentityAlreadyExists with an OutOfWallet location; propagate — or at least error-level log — the rest) or expand the comment so future maintainers know the catch-all is deliberate for reasons beyond the location-index invariant. Not blocking because the register-funded fresh-identity path where a same-id OOW entry already exists is a narrow scenario, and this PR is a strict improvement over the previous behavior that stranded the asset lock.

source: ['claude', 'codex']

},
None => {
tracing::warn!(
identity_id = %identity.id(),
"register_identity_with_funding: identity registered on \
Platform but wallet info was not found locally; skipping \
local persistence"
);
}
}
Expand Down Expand Up @@ -344,7 +377,8 @@ impl IdentityWallet {
/// Core-side timeout and Platform-side rejection (same as
/// register).
/// 4. Persist the new credit balance + remove the tracked asset
/// lock.
/// lock. Best-effort: Platform has already accepted, so a local
/// bookkeeping failure is logged, not propagated.
pub async fn top_up_identity_with_funding<AS>(
&self,
identity_id: &Identifier,
Expand Down Expand Up @@ -455,21 +489,38 @@ impl IdentityWallet {
Err(e) => return Err(PlatformWalletError::Sdk(e)),
};

// Step 4: persist the new balance + clean up the tracked lock.
// Step 4 (best-effort): persist the new balance + clean up the
// tracked lock.
//
// Platform has ALREADY accepted the top-up, so a missing local
// wallet must NOT propagate as `Err` — the caller would report
// failure for credits that exist on chain, and the early return
// would skip the `consume_asset_lock` below, leaving the spent
// lock in the Resumable Funding list where a Resume gets
// Platform's deterministic "lock already consumed" rejection.
// The stale local balance self-heals on the next identity
// re-sync. Same posture as register's Step 4.
{
let mut wm = self.wallet_manager.write().await;
let info = wm.get_wallet_info_mut(&self.wallet_id).ok_or_else(|| {
PlatformWalletError::WalletNotFound(
"Wallet info not found in wallet manager".to_string(),
)
})?;
if let Some(managed) = info.identity_manager.managed_identity_mut(identity_id) {
managed.identity.set_balance(new_balance);
if let Err(e) = self.persister.store(managed.snapshot_changeset().into()) {
tracing::error!(
match wm.get_wallet_info_mut(&self.wallet_id) {
Some(info) => {
if let Some(managed) = info.identity_manager.managed_identity_mut(identity_id) {
managed.identity.set_balance(new_balance);
if let Err(e) = self.persister.store(managed.snapshot_changeset().into()) {
tracing::error!(
identity = %identity_id,
error = %e,
"Failed to persist identity balance update after top_up"
);
}
}
}
None => {
tracing::warn!(
identity = %identity_id,
error = %e,
"Failed to persist identity balance update after top_up"
"top_up_identity_with_funding: top-up accepted on Platform \
but wallet info was not found locally; skipping balance \
persistence"
);
}
}
Expand Down
Loading