fix(platform-wallet): make post-acceptance identity bookkeeping best-effort#4011
Conversation
…effort `register_identity_with_funding` and `top_up_identity_with_funding` ran their local IdentityManager bookkeeping (`get_wallet_info_mut(...)?` and `add_identity(...)?`) with `?` AFTER Platform had already accepted the registration/top-up. A failure there propagated as `Err`, so the app reported failure for an identity/credits that exist on chain, and the early return skipped the subsequent `consume_asset_lock` — stranding the spent lock in the Resumable Funding list, where a Resume gets Platform's deterministic "lock already consumed" rejection. Convert both post-acceptance steps to warn-and-continue so the flow reaches `consume_asset_lock`, matching the posture PR #4008 gave the sibling `register_from_addresses` Step 3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
📝 WalkthroughWalkthroughRegistration and top-up entry points in registration.rs now treat local IdentityManager updates and wallet-state persistence as best-effort after Platform acceptance. Failures to update or persist local bookkeeping log warnings instead of returning errors; documentation comments were updated accordingly. ChangesBest-effort bookkeeping changes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Review complete (commit b3e4395) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs (1)
503-527: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSilent skip when the managed identity is missing.
If
get_wallet_info_mutreturnsSome(info)butmanaged_identity_mut(identity_id)returnsNone, the balance is neither updated nor persisted and nothing is logged. This can happen if the identity is removed between the Step 1 read-lock lookup and this Step 4 write-lock. TheNonewallet-info branch logs a warning, but this inner miss does not, leaving a stale local balance with no trace — inconsistent with the stated best-effort "log, don't propagate" posture.🔎 Add an observability warning for the missing-managed-identity case
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" ); } + } else { + tracing::warn!( + identity = %identity_id, + "top_up_identity_with_funding: top-up accepted on Platform \ + but managed identity was not found locally; skipping balance \ + persistence" + ); } }🤖 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/src/wallet/identity/network/registration.rs` around lines 503 - 527, The top-up flow in registration::top_up_identity_with_funding silently ignores the case where get_wallet_info_mut returns Some(info) but info.identity_manager.managed_identity_mut(identity_id) returns None. Add a tracing::warn! in that inner None branch with the identity_id and enough context to show the balance update was skipped after a successful Platform top-up, matching the existing best-effort logging style used in the wallet-info-missing branch. Keep the persistence path unchanged for the Some(managed) case, but make sure the missing-managed-identity path is observable instead of silently leaving the local balance stale.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/registration.rs`:
- Around line 503-527: The top-up flow in
registration::top_up_identity_with_funding silently ignores the case where
get_wallet_info_mut returns Some(info) but
info.identity_manager.managed_identity_mut(identity_id) returns None. Add a
tracing::warn! in that inner None branch with the identity_id and enough context
to show the balance update was skipped after a successful Platform top-up,
matching the existing best-effort logging style used in the wallet-info-missing
branch. Keep the persistence path unchanged for the Some(managed) case, but make
sure the missing-managed-identity path is observable instead of silently leaving
the local balance stale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d15df93-4d87-44e3-aaa8-4945518d9b4e
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
There was a problem hiding this comment.
Code Review
Well-scoped follow-up to #4008 that converts two post-acceptance ? sites in the asset-lock-funded register/top-up flows into warn-and-continue, so consume_asset_lock runs and the spent lock is not stranded. No consensus, serialization, proof, or FFI surface is touched. Verified against b3e4395: IdentityManager::add_identity does raise IdentityAlreadyExists for both buckets, but the OOW-collision path is speculative for freshly-registered identities and the PR is a strict improvement over the prior stranded-lock behavior. Two observability suggestions worth surfacing; no in-scope blockers.
Source: reviewers claude/opus (general), codex/gpt-5.5 (general), claude/opus (rust-quality), codex/gpt-5.5 (rust-quality); verifier claude/opus; specialists: rust-quality.
🟡 1 suggestion(s) | 💬 1 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/src/wallet/identity/network/registration.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:312-324: 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.
- [NITPICK] packages/rs-platform-wallet/src/wallet/identity/network/registration.rs:505-527: Top-up Step 4 silently no-ops when managed identity is missing under the write lock
When `wm.get_wallet_info_mut(&self.wallet_id)` returns `Some(info)` but `info.identity_manager.managed_identity_mut(identity_id)` returns `None` (line 507), the `if let Some(managed)` block exits without logging. That's the case where the identity was present at Step 1 under the read lock but disappeared before Step 4 acquired the write lock — Platform accepted the top-up and the new balance is dropped with no operator signal. The outer `None` arm at line 518 warns; the inner `None` case should too, matching the register-flow style ("top-up accepted on Platform but managed identity vanished locally; skipping balance persistence"). Small, but the PR's stated posture is that every post-acceptance skip is logged.
| 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" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 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:
add_identityreturnsIdentityAlreadyExistswhether the pre-existing entry is in the wallet-owned bucket or the out-of-wallet bucket (verified:lifecycle.rs:45guards onself.identity(...).is_some(), andaccessors.rs:21-32scans both buckets). For the out-of-wallet duplicate case, this PR now returnsOk(identity)whilelocation_indexstill reportsOutOfWallet, so a follow-uptop_up_identity_with_fundingon the same id will fail withIdentityIndexNotSet(identity_indexreturnsNonefor the OOW discriminant atlifecycle.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.- For non-
IdentityAlreadyExistsvariants (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']
2 participants
Issue being fixed or feature implemented
PR #4008 made post-acceptance
add_identitybest-effort (warn, not?) in the address-fundedregister_from_addressesflow. The sibling asset-lock flows inidentity/network/registration.rsstill propagated post-acceptance failures with?:register_identity_with_funding— Step 4'sget_wallet_info_mut(...).ok_or_else(...)?andinfo.identity_manager.add_identity(...)?both ran after Platform accepted theIdentityCreate.top_up_identity_with_funding— Step 4'sget_wallet_info_mut(...).ok_or_else(...)?ran after Platform accepted the top-up.In both, an early
Errthere had two bad consequences:consume_asset_lock, leaving the spent lock in the Resumable Funding list. A user Resume then hits Platform's deterministic "lock already consumed" rejection — a dead-end for a lock that did its job.What was done?
Converted both post-acceptance steps to the same warn-and-continue posture as
register_from_addressesStep 3 (#4008):get_wallet_info_mutreturningNone→tracing::warn!and skip local persistence instead of returningWalletNotFound.add_identityreturningErr(register flow) →tracing::warn!and skip the key-breadcrumb stamping (anIdentityAlreadyExistsmay point at an out-of-wallet entry; stampingwallet_idwithout moving buckets would contradict the location index).consume_asset_lock, so the spent lock is cleaned up. Stale local state self-heals on the next identity re-sync.Method doc-comment step lists updated to note the best-effort posture. Comment style mirrors
register_from_addresses.rsStep 3.How Has This Been Tested?
cargo fmt --allcargo test -p platform-wallet— green (8 passed, 0 failed).The unit-test module in
registration.rscovers only the IS→CL timeout discriminator; exercising the post-acceptance path would require a live SDK submission mock the module doesn't set up, so — as with #4008's treatment ofregister_from_addresses— no unit test was added for this branch.Breaking Changes
None. Local-only behavior change (error → warn); no consensus, serialization, or API surface change.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit