fix(platform-wallet): fail-closed on registration persist error (Found-017) [backport]#3659
fix(platform-wallet): fail-closed on registration persist error (Found-017) [backport]#3659lklimek wants to merge 3 commits into
Conversation
…ound-017) [backport] Semantic backport of the #3549-proven Found-017 fail-closed fix. Region diverged on v3.1-dev so the change was re-applied by hand. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR tightens error handling in wallet registration by ensuring persistence failures are truly atomic. When wallet storage fails, the in-memory insert is rolled back and an error is returned, rather than silently continuing. Documentation is updated to reflect this load-bearing semantics. ChangesWallet Registration Persistence Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
lklimek
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3659 +/- ##
============================================
- Coverage 87.16% 87.15% -0.01%
============================================
Files 2607 2606 -1
Lines 319420 319221 -199
============================================
- Hits 278413 278216 -197
+ Misses 41007 41005 -2
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "8148ea041180f5244c720499314a8d1ccebdf4714fe3ee287e3bd6f44163e7db"
)Xcode manual integration:
|
There was a problem hiding this comment.
Code Review
Surgical backport that aligns the persist-error path with the two existing rollback idioms in register_wallet (lines 327-334 and 343-348). The new branch correctly unwinds the just-inserted wallet_manager entry before returning WalletCreation, preserving the fail-closed invariant required by Found-017. No correctness or consensus issues; remaining notes are about regression coverage and a now-tripled duplication pattern.
Note: Inline review posting hit GitHub HTTP 422, so these verified findings were posted in the top-level review body.
Reviewed commit: e7fb1d5
🟡 1 suggestion(s) | 💬 2 nitpick(s)
3 additional finding(s)
suggestion: No in-crate regression test pins the registration persist-failure rollback
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 281)
The new branch is the substance of the Found-017 fix, but the v3.1-dev backport ships without a unit test exercising it. The PR description explicitly defers pin coverage to #3549 on a sibling branch, leaving this code path protected only by reviewer memory on v3.1-dev. A targeted regression test that injects a PlatformWalletPersistence stub whose store returns Err and asserts (a) register_wallet returns Err(PlatformWalletError::WalletCreation(_)) and (b) the wallet_id is absent from wallet_manager after the call (e.g. a retry with insert_wallet succeeds) would lock the invariant directly onto v3.1-dev. Without it, a future refactor that reverts the rollback to the prior log-and-continue shape, or that adds another insert_wallet-style side effect between lines 217-280 not covered by this unwind, would silently regress Found-017 on this branch without CI catching it.
nitpick: Three near-identical write-lock + remove_wallet rollback blocks in one function
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 281)
After this PR, register_wallet contains the same two-line rollback (acquire wallet_manager.write().await, call remove_wallet, discard the Result) three times — at 287-288, 328-329, and 343-344 — each preceding a WalletCreation error return. The duplication itself was pre-existing, but adding a third occurrence raises the cost of any future failure point added between insert and the final commit: missing one unwind step is exactly the Found-017 failure mode. A small private async helper (e.g. rollback_inserted_wallet(&self, wallet_id)) called as self.rollback_inserted_wallet(wallet_id).await; return Err(...); at each site would centralize the invariant. Pure refactor, no semantic change — probably out of scope for a backport, but worth doing on the forward branch.
nitpick: Rollback discards remove_wallet Result silently
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 288)
let _ = wm.remove_wallet(&wallet_id); discards a Result. In this exact call site wallet_id was just returned by the immediately preceding wm.insert_wallet, so a NotFound is essentially impossible today. The discard also matches the established convention at lines 329 and 344. Flagging only because if remove_wallet ever grows fallible side-effects (event-bus deregistration, secondary index cleanup) the rollback would become a silent partial unwind across all three sites in lockstep. A tracing::error! on Err here would convert that into a loud failure without altering the public contract. Not blocking; existing convention is defensible.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:281-293: No in-crate regression test pins the registration persist-failure rollback
The new branch is the substance of the Found-017 fix, but the v3.1-dev backport ships without a unit test exercising it. The PR description explicitly defers pin coverage to #3549 on a sibling branch, leaving this code path protected only by reviewer memory on v3.1-dev. A targeted regression test that injects a `PlatformWalletPersistence` stub whose `store` returns `Err` and asserts (a) `register_wallet` returns `Err(PlatformWalletError::WalletCreation(_))` and (b) the wallet_id is absent from `wallet_manager` after the call (e.g. a retry with `insert_wallet` succeeds) would lock the invariant directly onto v3.1-dev. Without it, a future refactor that reverts the rollback to the prior log-and-continue shape, or that adds another `insert_wallet`-style side effect between lines 217-280 not covered by this unwind, would silently regress Found-017 on this branch without CI catching it.
- [NITPICK] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:281-348: Three near-identical write-lock + remove_wallet rollback blocks in one function
After this PR, `register_wallet` contains the same two-line rollback (acquire `wallet_manager.write().await`, call `remove_wallet`, discard the Result) three times — at 287-288, 328-329, and 343-344 — each preceding a `WalletCreation` error return. The duplication itself was pre-existing, but adding a third occurrence raises the cost of any future failure point added between insert and the final commit: missing one unwind step is exactly the Found-017 failure mode. A small private async helper (e.g. `rollback_inserted_wallet(&self, wallet_id)`) called as `self.rollback_inserted_wallet(wallet_id).await; return Err(...);` at each site would centralize the invariant. Pure refactor, no semantic change — probably out of scope for a backport, but worth doing on the forward branch.
- [NITPICK] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:288-288: Rollback discards remove_wallet Result silently
`let _ = wm.remove_wallet(&wallet_id);` discards a `Result`. In this exact call site `wallet_id` was just returned by the immediately preceding `wm.insert_wallet`, so a NotFound is essentially impossible today. The discard also matches the established convention at lines 329 and 344. Flagging only because if `remove_wallet` ever grows fallible side-effects (event-bus deregistration, secondary index cleanup) the rollback would become a silent partial unwind across all three sites in lockstep. A `tracing::error!` on `Err` here would convert that into a loud failure without altering the public contract. Not blocking; existing convention is defensible.
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
No... I don't think this is correct... a usecase would be if someone has their wallet in their keychain, but the app is crashing on startup... we don't want to delete their wallet (risk of loosing funds).
|
@QuantumExplorer Thanks for flagging — traced the call graph to verify the concern, and I don't think the scenario is reachable here. Detail:
Loading goes through a separate path.
Worst real consequence: user re-attempts wallet creation (re-enters the seed they typed seconds ago). No funds-loss risk — the fresh wallet was never persisted and the user still holds the seed. The keychain-crash scenario you described would need a "load-from-keychain → re-register" path; that doesn't exist in this codebase (there's no keychain-load API that invokes Happy to add an inline code comment documenting this invariant ("register_wallet only runs for first-time creation; loaded wallets bypass this arm") so the concern doesn't resurface in future review — say the word. 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
@QuantumExplorer on top of what Claudius investigated, I would add that from user perspective, it is very important to fail on save, other approach can lead to data loss (user thinks data is persisted but it's not), and inconsistency. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The current head keeps the new fail-closed rollback on store() failure, but register_wallet is still not fully transactional: once persistence succeeds, later restore failures can still return Err while leaving a durably stored wallet behind. The branch also still lacks a local regression test for the new persist-failure rollback path. No new valid findings were introduced by the e7fb1d52d1932954b1fc5b1cbce585a8bf21b767..40177b4870ce2e73f3c6d4c143ec21c929251f71 delta; the new shielded-wallet unregister logic in remove_wallet looks correct.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3659 40177b4
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._
Reviewed commit: 40177b4
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: `register_wallet` can return `Err` after leaving the wallet durably persisted
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 281)
register_wallet persists the registration changeset before it runs the second-phase restore work (load_persisted() and initialize_from_persisted(...)). If either of those later steps fails, the code only removes the wallet from the in-memory manager and then returns WalletCreation, but there is no compensating delete in the persister API. That means callers can observe an apparent registration failure even though the wallet metadata and account snapshot were already committed and will be rehydrated on the next load_from_persistor(). This is a real partial-commit bug, not just a cleanup nit: the API reports all-or-nothing semantics, but the durable side effects are only partially rolled back.
suggestion: The new persist-failure rollback path still has no branch-local regression test
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 281)
This branch now removes the just-inserted wallet when self.persister.store(...) fails, but there is still no in-crate test that forces PlatformWalletPersistence::store to error and then proves a retry succeeds instead of hitting WalletAlreadyExists. Because this backport carries behavior that is different from the old branch and the only cited coverage lives elsewhere, the fix is still protected by reviewer memory rather than a local regression test.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:281-347: `register_wallet` can return `Err` after leaving the wallet durably persisted
`register_wallet` persists the registration changeset before it runs the second-phase restore work (`load_persisted()` and `initialize_from_persisted(...)`). If either of those later steps fails, the code only removes the wallet from the in-memory manager and then returns `WalletCreation`, but there is no compensating delete in the persister API. That means callers can observe an apparent registration failure even though the wallet metadata and account snapshot were already committed and will be rehydrated on the next `load_from_persistor()`. This is a real partial-commit bug, not just a cleanup nit: the API reports all-or-nothing semantics, but the durable side effects are only partially rolled back.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:281-293: The new persist-failure rollback path still has no branch-local regression test
This branch now removes the just-inserted wallet when `self.persister.store(...)` fails, but there is still no in-crate test that forces `PlatformWalletPersistence::store` to error and then proves a retry succeeds instead of hitting `WalletAlreadyExists`. Because this backport carries behavior that is different from the old branch and the only cited coverage lives elsewhere, the fix is still protected by reviewer memory rather than a local regression test.
|
@thepastaclaw Non-atomicity finding confirmed — 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Thanks for tracing this. That matches my read: #3659 covers the specific -failure path (no durable commit, so in-memory rollback is enough), while the restore-phase orphan case is a broader partial-commit bug after a successful durable write. #3736 is the right follow-up for that. I’m fine with keeping the restore-phase fix out of this surgical backport; from my side the remaining branch-local regression-test note is non-blocking. |
5 participants
Semantic backport to v3.1-dev of the #3549-proven Found-017 fail-closed fix.
Root cause:
register_walletlogged the registrationpersister.store(...)Errand fell through, then committed in-memory state and returnedOk. The wallet worked for the session but was silently lost on restart — write-acknowledged-but-not-durable.Fix: on the registration
store()Err, keep thetracing::error!, re-acquire the wallet-manager write guard,remove_wallet(&wallet_id)to roll back the in-memoryinsert_wallet, and returnErr(WalletCreation(...)). Fail-closed, mirroring the same function's existingload_persisted/initialize_from_persistedrollback idiom. Reuses the existingWalletCreationvariant — no new variant, no error-type churn.The v3.1-dev region diverged ~62 lines from origin/feat so a textual cherry-pick would not apply; re-applied by hand as a semantic transform. Single
remove_walletfully unwinds — onlyinsert_walletis committed at thestore()point (self.wallets.insertis downstream).Validation: correctness already proven on #3549 (deterministic network-free pin + positive guard, QA-PASS). v3.1-dev has no found_017 e2e pin by design; here:
cargo check --testsclean,clippy --lib -D warningsclean,cargo test --lib121 passed / 0 failed, rustfmt clean. Pin coverage stays #3549-only.Sibling PR to #3549.
🤖 Generated with Claude Code
Summary by CodeRabbit