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 2 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>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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.17% 87.17% -0.01%
============================================
Files 2601 2601
Lines 318220 318220
============================================
- Hits 277421 277420 -1
- Misses 40799 40800 +1
🚀 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: "b519084485c372e3e10ee0ea54671044fc2c959ab0fdf3a95fb08cd9621e8e05"
)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).
4 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