feat(kotlin-sdk): bind every wallet's shielded sub-wallet for multi-wallet flows#4046
Conversation
…allet flows Android port of #4038 (SwiftExampleApp): engine-bind EVERY loaded wallet's shielded (Orchard) sub-wallet with the Rust sync coordinator, not just the lexicographically-smallest wallet. Unblocks SH-14/15/16 (cross-wallet shielded transfer / unshield / withdraw). Kotlin-only — the Rust coordinator was already multi-wallet, and the per-wallet READ surfaces (Receive sheet, balances, self-shield guard, identity funding) already read per-wallet. Same "single UI mirror + multi-engine-bind" design: ShieldedService keeps mirroring one wallet for the Sync tab via bind(); the new bindEngine() does engine-only registration, and AppContainer.rebindWalletScopedServices() runs the fleet pass through the pure engineBindOtherWallets seam, placed BEFORE the fallible sync-start calls. bindEngine has NO already-bound fast path on purpose: shieldedDefaultAddress reflects the wallet-level sub-wallet binding, which survives clearShieldedStorage, so skipping on it would leave post-Clear wallets permanently unregistered (iOS lesson, verified on-simulator there). Clear (SH-12) is fleet-aware too, fixing a latent multi-wallet bug the port would otherwise expose: clearShieldedStorage empties the SHARED commitment tree, but the Room wipe was scoped to the bound wallet — a non-mirror wallet's surviving watermark would restore a position ahead of the emptied tree on re-bind and gate-skip every note (the known Room↔SQLite watermark-divergence freeze). Clear now wipes every wallet's rows and re-binds the whole fleet: mirror via bind(), others via bindEngine(), regardless of the mirror bind's outcome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
✅ Review complete (commit 949ea30) |
There was a problem hiding this comment.
Code Review
Source: reviewers: opus/claude-general failed (extra usage quota; resets Jul 10, 8am America/Chicago), gpt-5.5/codex-general completed; verifier: gpt-5.5/codex; specialists: none.
Both verified findings are valid and in scope for PR #4046. The PR claims fleet-wide shielded binding and clear behavior, but the current implementation still has one over-broad Room wipe across networks and one ordering path where platform-address sync failure prevents the new engine-bind pass from running.
🔴 2 blocking
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/ShieldedService.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/ShieldedService.kt:463-468: Shielded clear wipes other networks' Room rows
`clearShieldedStorage()` runs on the active `PlatformWalletManager`, whose shielded coordinator is network-scoped and opened from `shielded_tree_${manager.network.networkName}.sqlite`. These `deleteAll...` calls wipe every shielded row in the shared Room database, including wallets on other networks whose Rust shielded stores were not reset. Clearing shielded state on one network can therefore delete another network's notes, activity, and watermarks while leaving that network's Rust tree intact, causing data loss and Room/Rust divergence after switching networks.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/di/AppContainer.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/di/AppContainer.kt:211-214: Platform sync failure can skip shielded engine binds
The PR description and inline comment require the multi-wallet engine-bind pass to run before fallible sync-start calls so a start failure cannot leave non-mirror wallets unregistered. This block checks and starts platform-address sync before `shieldedService.bind(...)` and `engineBindOtherWallets(...)`, inside the same `try`. If the platform-address running check or start call throws, control jumps to the catch and no shielded coordinator registration happens for the non-mirror wallets until a later rebind or relaunch. Move the platform-address start block after the shielded bind and engine-bind pass.
| db.withTransaction { | ||
| db.shieldedDao().deleteActivityByWallet(walletId) | ||
| db.shieldedDao().deleteOutgoingNotesByWallet(walletId) | ||
| db.shieldedDao().deleteNotesByWallet(walletId) | ||
| db.shieldedDao().deleteSyncStatesByWallet(walletId) | ||
| db.shieldedDao().deleteAllActivity() | ||
| db.shieldedDao().deleteAllOutgoingNotes() | ||
| db.shieldedDao().deleteAllNotes() | ||
| db.shieldedDao().deleteAllSyncStates() | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Shielded clear wipes other networks' Room rows
clearShieldedStorage() runs on the active PlatformWalletManager, whose shielded coordinator is network-scoped and opened from shielded_tree_${manager.network.networkName}.sqlite. These deleteAll... calls wipe every shielded row in the shared Room database, including wallets on other networks whose Rust shielded stores were not reset. Clearing shielded state on one network can therefore delete another network's notes, activity, and watermarks while leaving that network's Rust tree intact, causing data loss and Room/Rust divergence after switching networks.
| db.withTransaction { | |
| db.shieldedDao().deleteActivityByWallet(walletId) | |
| db.shieldedDao().deleteOutgoingNotesByWallet(walletId) | |
| db.shieldedDao().deleteNotesByWallet(walletId) | |
| db.shieldedDao().deleteSyncStatesByWallet(walletId) | |
| db.shieldedDao().deleteAllActivity() | |
| db.shieldedDao().deleteAllOutgoingNotes() | |
| db.shieldedDao().deleteAllNotes() | |
| db.shieldedDao().deleteAllSyncStates() | |
| } | |
| val activeWalletIds = db.walletDao() | |
| .getByNetwork(mgr.network.ffiValue) | |
| .map { it.walletId } | |
| db.withTransaction { | |
| for (id in activeWalletIds) { | |
| db.shieldedDao().deleteActivityByWallet(id) | |
| db.shieldedDao().deleteOutgoingNotesByWallet(id) | |
| db.shieldedDao().deleteNotesByWallet(id) | |
| db.shieldedDao().deleteSyncStatesByWallet(id) | |
| } | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in e75ea53 — Shielded clear wipes other networks' Room rows no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…e fallible sync starts Addresses the two blocking review findings on #4046. - ShieldedService.clearLocalState: the Rust reset is network-scoped (`shielded_tree_<network>.sqlite`), so a global `deleteAll*` Room wipe dropped OTHER networks' notes/activity/watermarks while their Rust trees stayed populated → data loss + Room/Rust divergence after a network switch. Wipe only the active network's fleet (`mgr.wallets`, which is network-scoped) via the per-wallet `delete*ByWallet` DAO methods. - AppContainer.rebindWalletScopedServices: moved the fallible platform-address sync start to AFTER the shielded bind + engine-bind pass. Previously a throw from the platform-address running-check/start jumped to the catch and left the non-mirror wallets' shielded coordinators unregistered until a later rebind/relaunch — matching what the existing comment already promised. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both blocking findings addressed in e75ea53:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers=general:claude/opus(failed:extra-usage), general:codex/gpt-5.5(failed:auth), ffi-engineer:claude/opus(failed:extra-usage), ffi-engineer:codex/gpt-5.5(failed:auth), fallback openclaw general+ffi (ok); verifier=openclaw/manual-code-check(ok).
Reviewed commit: e75ea53e. Prior reviewed commit: 5baf9ac1.
Carried-forward prior findings: none. Both prior blockers are FIXED at current head.
New findings in latest delta 5baf9ac1..e75ea53e: one residual suggestion (Clear only iterates currently loaded same-network wallets) and one KDoc nit.
The latest push scopes the Room wipe to mgr.wallets with per-wallet deletes and moves platform-address sync start after mirror bind + engine-bind, matching the review fixes claimed in the commit message.
🟡 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/ShieldedService.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/ShieldedService.kt:469-476: Clear still misses unloaded same-network shielded rows
The cross-network wipe is fixed: `clearLocalState` now deletes only wallets from the network-scoped `mgr.wallets` map via per-wallet DAO methods. However, `loadPersistedWallets()` only publishes wallets that successfully `getWallet`, so a same-network wallet that still has Room shielded rows (failed restore / not currently loaded) is skipped by this loop. After `clearShieldedStorage()` empties the shared per-network tree, leftover watermarks/notes for those unloaded wallets can later re-enter through `restore_for_wallet` and recreate the Room↔tree divergence freeze Clear is meant to prevent. Prefer wiping every Room wallet on `mgr.network` (for example `walletDao().getByNetwork(mgr.network.ffiValue)`), not only the currently loaded fleet.
| db.withTransaction { | ||
| db.shieldedDao().deleteActivityByWallet(walletId) | ||
| db.shieldedDao().deleteOutgoingNotesByWallet(walletId) | ||
| db.shieldedDao().deleteNotesByWallet(walletId) | ||
| db.shieldedDao().deleteSyncStatesByWallet(walletId) | ||
| for (other in mgr.wallets.value.values) { | ||
| val wid = other.walletId | ||
| db.shieldedDao().deleteActivityByWallet(wid) | ||
| db.shieldedDao().deleteOutgoingNotesByWallet(wid) | ||
| db.shieldedDao().deleteNotesByWallet(wid) | ||
| db.shieldedDao().deleteSyncStatesByWallet(wid) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Clear still misses unloaded same-network shielded rows
The cross-network wipe is fixed: clearLocalState now deletes only wallets from the network-scoped mgr.wallets map via per-wallet DAO methods. However, loadPersistedWallets() only publishes wallets that successfully getWallet, so a same-network wallet that still has Room shielded rows (failed restore / not currently loaded) is skipped by this loop. After clearShieldedStorage() empties the shared per-network tree, leftover watermarks/notes for those unloaded wallets can later re-enter through restore_for_wallet and recreate the Room↔tree divergence freeze Clear is meant to prevent. Prefer wiping every Room wallet on mgr.network (for example walletDao().getByNetwork(mgr.network.ffiValue)), not only the currently loaded fleet.
| db.withTransaction { | |
| db.shieldedDao().deleteActivityByWallet(walletId) | |
| db.shieldedDao().deleteOutgoingNotesByWallet(walletId) | |
| db.shieldedDao().deleteNotesByWallet(walletId) | |
| db.shieldedDao().deleteSyncStatesByWallet(walletId) | |
| for (other in mgr.wallets.value.values) { | |
| val wid = other.walletId | |
| db.shieldedDao().deleteActivityByWallet(wid) | |
| db.shieldedDao().deleteOutgoingNotesByWallet(wid) | |
| db.shieldedDao().deleteNotesByWallet(wid) | |
| db.shieldedDao().deleteSyncStatesByWallet(wid) | |
| } | |
| val activeWalletIds = db.walletDao() | |
| .getByNetwork(mgr.network.ffiValue) | |
| .map { it.walletId } | |
| db.withTransaction { | |
| for (wid in activeWalletIds) { | |
| db.shieldedDao().deleteActivityByWallet(wid) | |
| db.shieldedDao().deleteOutgoingNotesByWallet(wid) | |
| db.shieldedDao().deleteNotesByWallet(wid) | |
| db.shieldedDao().deleteSyncStatesByWallet(wid) | |
| } | |
| } |
source: ['openclaw-ffi-engineer']
There was a problem hiding this comment.
Resolved in 949ea30 — Clear still misses unloaded same-network shielded rows no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| * the bound wallet: with multi-engine-bind, every loaded wallet has | ||
| * rows, and [clearShieldedStorage] empties the SHARED tree — a | ||
| * non-mirror wallet whose Room watermark survived would restore a | ||
| * position ahead of the now-empty tree on re-bind and gate-skip | ||
| * every note (the exact Room↔SQLite watermark-divergence freeze | ||
| * this Clear exists to fix). | ||
| * 3. **Re-bind + restart sync** — [clearShieldedStorage] dropped every |
There was a problem hiding this comment.
💬 Nitpick: clearLocalState KDoc still says wipe is GLOBAL
Step 2 of the clearLocalState KDoc still describes the Room wipe as deliberately GLOBAL / every wallet's rows, but the implementation is now network-fleet scoped and explicitly avoids other networks. Update the doc to match the per-network contract so future edits don't reintroduce deleteAll*.
source: ['openclaw-general', 'openclaw-ffi-engineer']
There was a problem hiding this comment.
Resolved in 949ea30 — clearLocalState KDoc still says wipe is GLOBAL no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…lets Follow-up to the review on the per-network Clear fix: - Wipe by `walletDao().getByNetwork(mgr.network.ffiValue)` instead of `mgr.wallets`, which only holds the successfully-loaded fleet. A same-network wallet that failed restore still has Room shielded rows; the loaded-only loop skipped it, so after clearShieldedStorage empties the shared per-network tree its leftover watermark could re-enter via restore_for_wallet and recreate the Room/tree freeze Clear is meant to prevent. - Update the clearLocalState KDoc (step 2) to describe the per-network contract instead of the old "deliberately GLOBAL" wording, so a future edit doesn't reintroduce deleteAll*. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Follow-up review addressed in 949ea30:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of 949ea30 vs prior head e75ea53. Both prior findings (Room wipe missing unloaded same-network wallets; KDoc mislabeling the wipe as GLOBAL) are FIXED and moved to resolved_findings. No new findings in the latest delta. Codex agents failed to run due to a refresh-token auth error; reconciliation is based on the two Claude agents plus direct verification of ShieldedService.kt and WalletDao.kt at head. No carried-forward findings.
QuantumExplorer
merged commit 8adf257
into
feat/kotlin-sdk-and-example-app
2 participants
Issue being fixed or feature implemented
Android port of #4038 (merged for SwiftExampleApp). The Kotlin app bound only the lexicographically-smallest wallet's shielded (Orchard) sub-wallet to the Rust sync coordinator (
AppContainer.rebindWalletScopedServices→ singleshieldedService.bind). With multiple wallets loaded, every non-first wallet's shielded pool was invisible — no receive address, no note sync — blocking the multi-wallet shielded QA rows SH-14/15/16 and the substance of CORE-21.Kotlin-only change: the Rust coordinator was already multi-wallet, and the Android per-wallet read surfaces (Receive sheet, wallet-detail balance, self-shield guard, identity-funding gate) already resolve per-wallet — they just returned nothing for wallets that were never registered.
What was done?
Same "single UI mirror + multi-engine-bind" design as iOS:
ShieldedServicekeeps mirroring ONE wallet (the first) for the global Sync tab viabind(...).ShieldedService.bindEngine(manager, walletId, dbPath, accounts)does engine-only registration (configure + bind, best-effort, no mirror repoint), returning success for fleet passes.AppContainer.rebindWalletScopedServices()engine-binds every other loaded wallet through the pure, unit-testedengineBindOtherWalletsseam — placed before the fallible sync-start calls so a start failure cannot leave non-mirror wallets unregistered (iOS review lesson).bindEngine, on purpose: the only cheap probe (shieldedDefaultAddress) reflects the wallet-level sub-wallet binding, which survivesclearShieldedStorage— skipping on it would leave post-Clear wallets permanently unregistered. This exact trap was hit and verified on-simulator during the iOS round.clearShieldedStorage()empties the SHARED commitment tree, but the Room wipe was scoped to the bound wallet — a non-mirror wallet's survivingshielded_sync_stateswatermark would restore a position ahead of the emptied tree on re-bind and gate-skip every note (the known Room↔SQLite watermark-divergence freeze). Clear now wipes every wallet's shielded rows and re-binds the whole fleet (mirror viabind, others viabindEngine), regardless of the mirror bind's outcome.Docs:
TEST_PLAN.mdrows SH-12/13/14/15/16 and CORE-21 updated (both wallets bound automatically; no wallet-swap needed).How Has This Been Tested?
./gradlew :sdk:testDebugUnitTest :app:assembleDebug— BUILD SUCCESSFUL; all 4 newShieldedEngineBindPlanTestcases pass (binds every non-mirror wallet, one failure doesn't stop the rest, single-wallet no-op, mirror skipped by value not position).tdash1…addresses,Shielded engine-boundlogcat line for wallet B, one Roomshielded_sync_statesrow per wallet after a pass) is in progress on a clean AVD; will report results in a comment. The identical design was fully verified on-simulator for iOS in feat(swift-example-app): bind every wallet's shielded sub-wallet for multi-wallet flows #4038.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code