chore: sync v4.1-dev with v4.0-dev#3990
Open
QuantumExplorer wants to merge 8 commits into
Open
Conversation
…p-from-addresses (#3969) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Quantum Explorer <quantum@dash.org>
…ed anchor (#3977) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Quantum Explorer <quantum@dash.org>
… into one guarded seam (#3987) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ion on sync (#3982) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Quantum Explorer <quantum@dash.org>
…3988) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR adds a ChangesShielded Anchor Validation and Redrive
Address-Funded Identity Reconciliation
Platform Address Sync Reset
WASM Protocol Version Upgrade State
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operations
participant ShieldedStore
participant Platform
participant Coordinator
Operations->>Platform: fetch recorded anchor set
Operations->>ShieldedStore: witness_at_depth per checkpoint
Operations->>Operations: select shallowest recorded anchor
Operations->>ShieldedStore: arm_pending_release(anchor, activity_id)
Operations->>Platform: broadcast_shielded_spend_with_redrive
Platform-->>Operations: ShieldedSpendUnconfirmed (ambiguous)
Operations->>ShieldedStore: arm_redrive_record
Coordinator->>ShieldedStore: prefetch_stranded_release
Coordinator->>Platform: fetch recorded anchors
Coordinator->>ShieldedStore: redrive_pending_spends
Coordinator->>ShieldedStore: release_stranded_spends (pruned anchors)
sequenceDiagram
participant FFI
participant PlatformWallet
participant IdentityWallet
participant PlatformAddressWallet
participant Provider
FFI->>PlatformWallet: top_up_from_addresses/register/transfer
PlatformWallet->>IdentityWallet: perform identity operation
IdentityWallet-->>PlatformWallet: (AddressInfos, result)
PlatformWallet->>PlatformAddressWallet: reconcile_address_infos(AddressInfos)
PlatformAddressWallet->>Provider: commit_reconciliation
Provider-->>PlatformAddressWallet: ReconciliationOutcome
PlatformAddressWallet->>PlatformAddressWallet: persist PlatformAddressChangeSet
PlatformWallet-->>FFI: identity result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Collaborator
|
✅ Review complete (commit 42019c5) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3990 +/- ##
==========================================
Coverage 87.18% 87.19%
==========================================
Files 2632 2634 +2
Lines 327563 327718 +155
==========================================
+ Hits 285592 285748 +156
+ Misses 41971 41970 -1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs (1)
137-156: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t let local identity registration failure suppress address reconciliation.
After the SDK call succeeds, Platform has accepted the registration and returned
address_infos; any localadd_identity/manager error here prevents the composite wallet from reconciling spent funding addresses. Treat the local registration step as best-effort after acceptance, then still return(identity, address_infos).Suggested shape
- { - let mut wm = self.wallet_manager.write().await; - let info = wm.get_wallet_info_mut(&self.wallet_id).ok_or_else(|| { - crate::error::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, - )?; - } + { + let mut wm = self.wallet_manager.write().await; + match wm.get_wallet_info_mut(&self.wallet_id) { + Some(info) => { + if let Err(e) = info.identity_manager.add_identity( + identity.clone(), + identity_index, + self.wallet_id, + &self.persister, + ) { + tracing::warn!( + identity_index, + error = %e, + "Identity registration succeeded on Platform but local manager \ + registration failed; continuing so address balances can reconcile" + ); + } + } + None => { + tracing::warn!( + identity_index, + "Identity registration succeeded on Platform but wallet info was missing; \ + continuing so address balances can reconcile" + ); + } + } + }🤖 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/register_from_addresses.rs` around lines 137 - 156, The local identity persistence step in register_from_addresses is blocking reconciliation after the SDK has already accepted the registration and returned address_infos. Update the identity_manager.add_identity path in register_from_addresses to be best-effort only: capture or log any add_identity/manager error, but do not return early from the accepted registration flow, and still return (identity, address_infos) so PlatformWallet::register_from_addresses can reconcile spent addresses.packages/rs-platform-wallet/src/wallet/shielded/store.rs (1)
537-549: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winResolve pending/redrive state even for already-spent notes.
Line 539 skips cleanup when the note is already marked spent. A rehydrated redrive row plus a restored spent note can therefore leave stale reservations/redrive metadata alive and eligible for later redrive/release handling. Make
mark_spentidempotently clear pending/redrives whenever the nullifier is known, while preserving the existing return value for “newly marked spent”.Suggested direction
pub(super) fn mark_spent(&mut self, nullifier: &[u8; 32]) -> bool { if let Some(&idx) = self.nullifier_index.get(nullifier) { - if !self.notes[idx].is_spent { - self.notes[idx].is_spent = true; - // Promotion implies the spend confirmed; drop any - // matching pending reservation. Idempotent — the - // common path already cleared pending in the - // spend-flow finalizer. - self.pending_nullifiers.remove(nullifier); - // The spend landed — its redrive record (if armed) is - // resolved for every nullifier it carries. - self.drop_redrives_containing(nullifier); - return true; - } + let newly_spent = !self.notes[idx].is_spent; + self.notes[idx].is_spent = true; + self.pending_nullifiers.remove(nullifier); + self.drop_redrives_containing(nullifier); + return newly_spent; } false }Also mirror durable redrive-row deletion in the file-backed store for this idempotent path, not only when the boolean return is
true.🤖 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/shielded/store.rs` around lines 537 - 549, The mark_spent path in shielded store cleanup is not idempotent: when a known nullifier is already marked spent, it currently skips clearing pending reservations and redrive metadata. Update mark_spent to always remove the nullifier from pending_nullifiers and drop_redrives_containing once nullifier_index contains it, while keeping the return value true only for the first transition in note state. Also mirror this behavior in the file-backed store’s durable redrive-row deletion logic so the already-spent path clears persisted redrive rows too, not just the newly-spent case.
🤖 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.
Inline comments:
In
`@packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs`:
- Around line 286-296: The fund-from-asset-lock flow in `fund_from_asset_lock`
can still call `consume_asset_lock` even when `reconcile_address_infos` hit a
persistence failure and only logged it. Update this path to distinguish
successful durable balance persistence from a logged/hidden failure, either by
having `reconcile_address_infos` return persistence status or by introducing a
strict reconciliation variant for asset-lock funding. Then guard the
`consume_asset_lock` call so the lock is only marked `Consumed` when the balance
changeset was actually stored.
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs`:
- Around line 1002-1026: The conflict handling in commit_reconciliation is
incomplete: when the live-pool address_index already exists in state.addresses,
the code only logs and still pushes the entry after mutating state.found. Update
the commit path in provider::commit_reconciliation so conflicting entries are
skipped or removed before outcome.entries.push(entry), and ensure the
found/bijection updates only happen for non-conflicting entries. Use the
existing state.addresses.contains_left, state.addresses.get_by_right, and
outcome.entries flow to locate and gate the conflicting case.
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs`:
- Around line 440-452: The reset flow in reset_sync_state currently updates
wallet_manager and then later acquires the provider lock, leaving a window where
reconciliation/sync can repopulate managed balances. Move the wallet_info
balance clearing and provider.reset_sync_state() into the same provider.write()
critical section, or otherwise serialize both operations under the provider lock
in reset_sync_state so the reset is atomic and cannot race with
sync/reconciliation.
In `@packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- Around line 108-117: The durable shielded_pending_spends table is not being
cleared during wallet teardown, so stale redrive state can survive a Clear or
unregister and come back on reopen. Update the purge path in purge_wallet and
purge_all_subwallets in file_store.rs to delete rows from
shielded_pending_spends alongside the existing in-memory subwallet and
commitment tree cleanup, using the same wallet/account scoping already used by
the wallet purge logic.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 1736-1743: The anchor probe in shielded/operations.rs is
conflating a depth-specific witness failure with a real “note not present”
result, causing later checkpoint probing to stop early. Update the flow around
the anchor probe logic in the relevant witness lookup and the checkpoint loop so
a depth>0 store/witness error returns a distinct “skip this depth” outcome
instead of Ok(None), and make the caller only break on the true
post-dates/not-present condition. Use the existing witness_at_depth and
checkpoint selection logic to distinguish transient probe failures from actual
absence.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift`:
- Around line 225-291: The clear flow in
PlatformBalanceSyncService.clearLocalState can be overwritten by an already
in-flight refreshBalanceSnapshot() that completes after the wipe. Add a
service-local generation/token that is bumped when clearLocalState starts or
succeeds, then have refreshBalanceSnapshot() and its snapshot-apply path verify
the current token before publishing results so stale balance data is ignored.
Make the check part of the same PlatformBalanceSyncService state used by
clearDisplay()/persisted snapshot updates.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:
- Around line 374-381: The Clear action can overlap with Sync Now or another
Clear because it is fire-and-forget, so add an `isClearing` state to
`PlatformBalanceSyncService` and use it to gate both actions. Update the
`clearLocalState` flow to set `isClearing` true for the full Rust reset plus
SwiftData cleanup, then reset it in a finally path, and wire `CoreContentView`
button states to disable both Clear and Sync Now while `isClearing` is true.
---
Outside diff comments:
In
`@packages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rs`:
- Around line 137-156: The local identity persistence step in
register_from_addresses is blocking reconciliation after the SDK has already
accepted the registration and returned address_infos. Update the
identity_manager.add_identity path in register_from_addresses to be best-effort
only: capture or log any add_identity/manager error, but do not return early
from the accepted registration flow, and still return (identity, address_infos)
so PlatformWallet::register_from_addresses can reconcile spent addresses.
In `@packages/rs-platform-wallet/src/wallet/shielded/store.rs`:
- Around line 537-549: The mark_spent path in shielded store cleanup is not
idempotent: when a known nullifier is already marked spent, it currently skips
clearing pending reservations and redrive metadata. Update mark_spent to always
remove the nullifier from pending_nullifiers and drop_redrives_containing once
nullifier_index contains it, while keeping the return value true only for the
first transition in note state. Also mirror this behavior in the file-backed
store’s durable redrive-row deletion logic so the already-spent path clears
persisted redrive rows too, not just the newly-spent case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66867827-aefd-4996-82c9-2ba90bfb28c7
📒 Files selected for processing (32)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/identity_registration_with_signer.rspackages/rs-platform-wallet-ffi/src/identity_top_up.rspackages/rs-platform-wallet-ffi/src/identity_transfer.rspackages/rs-platform-wallet-ffi/src/platform_address_sync.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/register_from_addresses.rspackages/rs-platform-wallet/src/wallet/identity/network/top_up_from_addresses.rspackages/rs-platform-wallet/src/wallet/identity/network/transfer_to_addresses.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/sync.rspackages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/coordinator.rspackages/rs-platform-wallet/src/wallet/shielded/file_store.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/store.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerAddressSync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformAddressSyncGenerationTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PlatformBalanceSyncServiceClearTests.swiftpackages/wasm-sdk/src/queries/protocol.rspackages/wasm-sdk/tests/unit/conversion-simple-types.spec.ts
Comment on lines
+286
to
296
| // The seam persists before returning, and the persist MUST | ||
| // happen before `consume_asset_lock` so we never have a | ||
| // Consumed lock paired with a stale balance row on disk. | ||
| // Persistence errors are logged inside the seam rather than | ||
| // propagated: Platform already accepted the transition, and a | ||
| // persistence hiccup shouldn't mask that. | ||
| let cs = self | ||
| .reconcile_address_infos(&address_infos, "fund from asset lock") | ||
| .await; | ||
|
|
||
| if let Some(out_point) = tracked_out_point { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don’t consume the asset lock after an unobservable balance-persist failure.
Line 286 says persistence must happen before consume_asset_lock, but reconcile_address_infos logs store errors and still returns a changeset. This path can mark the lock Consumed while durable balance rows remain stale.
Consider exposing persistence status from the reconciliation seam or adding a strict variant for asset-lock funding that skips consume_asset_lock when the balance changeset could not be stored.
🤖 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/platform_addresses/fund_from_asset_lock.rs`
around lines 286 - 296, The fund-from-asset-lock flow in `fund_from_asset_lock`
can still call `consume_asset_lock` even when `reconcile_address_infos` hit a
persistence failure and only logged it. Update this path to distinguish
successful durable balance persistence from a logged/hidden failure, either by
having `reconcile_address_infos` return persistence status or by introducing a
strict reconciliation variant for asset-lock funding. Then guard the
`consume_asset_lock` call so the lock is only marked `Consumed` when the balance
changeset was actually stored.
Comment on lines
+1002
to
+1026
| if is_removal { | ||
| state.found.remove(&entry.address); | ||
| } else { | ||
| state.found.insert(entry.address, entry.funds); | ||
| } | ||
| // Merge pool-resolved addresses into the bijection so | ||
| // `current_balances` can pair the fresh funds with a | ||
| // derivation index. Never overwrite an existing pairing — | ||
| // `BiBTreeMap::insert` evicts conflicting pairs, which | ||
| // would orphan another address's `found` entry. | ||
| if state.addresses.get_by_right(&entry.address).is_none() { | ||
| if state.addresses.contains_left(&entry.address_index) { | ||
| tracing::error!( | ||
| account_index = entry.account_index, | ||
| address_index = entry.address_index, | ||
| address = %entry.address, | ||
| "commit_reconciliation: derivation index already \ | ||
| maps to a different address — state drift; \ | ||
| leaving the bijection untouched" | ||
| ); | ||
| } else { | ||
| state.addresses.insert(entry.address_index, entry.address); | ||
| } | ||
| } | ||
| outcome.entries.push(entry); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Drop entries when the live-pool index conflicts with the persisted bijection.
Line 1013 logs the conflict but Line 1026 still emits the entry after found was already mutated. That can persist an address under an index already owned by another address and make current_balances unable to round-trip the committed seed.
Proposed fix
+ if state.addresses.get_by_right(&entry.address).is_none()
+ && state.addresses.contains_left(&entry.address_index)
+ {
+ tracing::error!(
+ account_index = entry.account_index,
+ address_index = entry.address_index,
+ address = %entry.address,
+ "commit_reconciliation: derivation index already maps to a different address; dropping reconciliation entry"
+ );
+ continue;
+ }
+
if is_removal {
state.found.remove(&entry.address);
} else {
state.found.insert(entry.address, entry.funds);
}
@@
if state.addresses.get_by_right(&entry.address).is_none() {
- if state.addresses.contains_left(&entry.address_index) {
- tracing::error!(
- account_index = entry.account_index,
- address_index = entry.address_index,
- address = %entry.address,
- "commit_reconciliation: derivation index already \
- maps to a different address — state drift; \
- leaving the bijection untouched"
- );
- } else {
- state.addresses.insert(entry.address_index, entry.address);
- }
+ state.addresses.insert(entry.address_index, entry.address);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if is_removal { | |
| state.found.remove(&entry.address); | |
| } else { | |
| state.found.insert(entry.address, entry.funds); | |
| } | |
| // Merge pool-resolved addresses into the bijection so | |
| // `current_balances` can pair the fresh funds with a | |
| // derivation index. Never overwrite an existing pairing — | |
| // `BiBTreeMap::insert` evicts conflicting pairs, which | |
| // would orphan another address's `found` entry. | |
| if state.addresses.get_by_right(&entry.address).is_none() { | |
| if state.addresses.contains_left(&entry.address_index) { | |
| tracing::error!( | |
| account_index = entry.account_index, | |
| address_index = entry.address_index, | |
| address = %entry.address, | |
| "commit_reconciliation: derivation index already \ | |
| maps to a different address — state drift; \ | |
| leaving the bijection untouched" | |
| ); | |
| } else { | |
| state.addresses.insert(entry.address_index, entry.address); | |
| } | |
| } | |
| outcome.entries.push(entry); | |
| if state.addresses.get_by_right(&entry.address).is_none() | |
| && state.addresses.contains_left(&entry.address_index) | |
| { | |
| tracing::error!( | |
| account_index = entry.account_index, | |
| address_index = entry.address_index, | |
| address = %entry.address, | |
| "commit_reconciliation: derivation index already maps to a different address; dropping reconciliation entry" | |
| ); | |
| continue; | |
| } | |
| if is_removal { | |
| state.found.remove(&entry.address); | |
| } else { | |
| state.found.insert(entry.address, entry.funds); | |
| } | |
| // Merge pool-resolved addresses into the bijection so | |
| // `current_balances` can pair the fresh funds with a | |
| // derivation index. Never overwrite an existing pairing — | |
| // `BiBTreeMap::insert` evicts conflicting pairs, which | |
| // would orphan another address's `found` entry. | |
| if state.addresses.get_by_right(&entry.address).is_none() { | |
| state.addresses.insert(entry.address_index, entry.address); | |
| } | |
| outcome.entries.push(entry); |
🤖 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/platform_addresses/provider.rs` around
lines 1002 - 1026, The conflict handling in commit_reconciliation is incomplete:
when the live-pool address_index already exists in state.addresses, the code
only logs and still pushes the entry after mutating state.found. Update the
commit path in provider::commit_reconciliation so conflicting entries are
skipped or removed before outcome.entries.push(entry), and ensure the
found/bijection updates only happen for non-conflicting entries. Use the
existing state.addresses.contains_left, state.addresses.get_by_right, and
outcome.entries flow to locate and gate the conflicting case.
Comment on lines
+440
to
+452
| pub async fn reset_sync_state(&self) { | ||
| { | ||
| let mut wm = self.wallet_manager.write().await; | ||
| if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { | ||
| for account in info.core_wallet.all_platform_payment_managed_accounts_mut() { | ||
| account.clear_balances(); | ||
| } | ||
| } | ||
| } | ||
| let mut guard = self.provider.write().await; | ||
| if let Some(provider) = guard.as_mut() { | ||
| provider.reset_sync_state(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize reset with sync/reconciliation under the provider lock.
Line 448 releases wallet_manager before Line 449 acquires the provider lock, so a reconciliation/sync can repopulate managed balances and persist rows between those steps; reset then clears only the provider seed.
Proposed fix
pub async fn reset_sync_state(&self) {
- {
- let mut wm = self.wallet_manager.write().await;
- if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) {
- for account in info.core_wallet.all_platform_payment_managed_accounts_mut() {
- account.clear_balances();
- }
- }
- }
let mut guard = self.provider.write().await;
if let Some(provider) = guard.as_mut() {
provider.reset_sync_state();
}
+ let mut wm = self.wallet_manager.write().await;
+ if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) {
+ for account in info.core_wallet.all_platform_payment_managed_accounts_mut() {
+ account.clear_balances();
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub async fn reset_sync_state(&self) { | |
| { | |
| let mut wm = self.wallet_manager.write().await; | |
| if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { | |
| for account in info.core_wallet.all_platform_payment_managed_accounts_mut() { | |
| account.clear_balances(); | |
| } | |
| } | |
| } | |
| let mut guard = self.provider.write().await; | |
| if let Some(provider) = guard.as_mut() { | |
| provider.reset_sync_state(); | |
| } | |
| pub async fn reset_sync_state(&self) { | |
| let mut guard = self.provider.write().await; | |
| if let Some(provider) = guard.as_mut() { | |
| provider.reset_sync_state(); | |
| } | |
| let mut wm = self.wallet_manager.write().await; | |
| if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { | |
| for account in info.core_wallet.all_platform_payment_managed_accounts_mut() { | |
| account.clear_balances(); | |
| } | |
| } | |
| } |
🤖 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/platform_addresses/wallet.rs` around
lines 440 - 452, The reset flow in reset_sync_state currently updates
wallet_manager and then later acquires the provider lock, leaving a window where
reconciliation/sync can repopulate managed balances. Move the wallet_info
balance clearing and provider.reset_sync_state() into the same provider.write()
critical section, or otherwise serialize both operations under the provider lock
in reset_sync_state so the reset is atomic and cannot race with
sync/reconciliation.
Comment on lines
+108
to
+117
| "CREATE TABLE IF NOT EXISTS shielded_pending_spends ( | ||
| wallet_id BLOB NOT NULL, | ||
| account_index INTEGER NOT NULL, | ||
| activity_id BLOB NOT NULL, | ||
| anchor BLOB NOT NULL, | ||
| nullifiers BLOB NOT NULL, | ||
| st_bytes BLOB NOT NULL, | ||
| attempts INTEGER NOT NULL DEFAULT 0, | ||
| PRIMARY KEY (wallet_id, account_index, activity_id) | ||
| )", |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear the new pending-spend table during wallet purge/clear.
The new shielded_pending_spends table is durable, but the existing purge/clear lifecycle only clears in-memory subwallet state and the commitment tree. After a host Clear or wallet unregister, stale redrive rows can rehydrate on a later open and resurrect old reservations/rebroadcast state. Add deletion of this table in purge_wallet and purge_all_subwallets.
Suggested direction
fn purge_wallet(&mut self, wallet_id: WalletId) -> Result<(), Self::Error> {
self.subwallets.retain(|id, _| id.wallet_id != wallet_id);
+ let conn = self.pending_conn.lock().expect("pending_conn mutex");
+ conn.execute(
+ "DELETE FROM shielded_pending_spends WHERE wallet_id = ?1",
+ rusqlite::params![wallet_id.as_slice()],
+ )
+ .map_err(|e| FileShieldedStoreError(format!("purge pending spends for wallet: {e}")))?;
Ok(())
}
fn purge_all_subwallets(&mut self) -> Result<(), Self::Error> {
self.subwallets.clear();
+ let conn = self.pending_conn.lock().expect("pending_conn mutex");
+ conn.execute("DELETE FROM shielded_pending_spends", [])
+ .map_err(|e| FileShieldedStoreError(format!("purge all pending spends: {e}")))?;
Ok(())
}🤖 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/shielded/file_store.rs` around lines
108 - 117, The durable shielded_pending_spends table is not being cleared during
wallet teardown, so stale redrive state can survive a Clear or unregister and
come back on reopen. Update the purge path in purge_wallet and
purge_all_subwallets in file_store.rs to delete rows from
shielded_pending_spends alongside the existing in-memory subwallet and
commitment tree cleanup, using the same wallet/account scoping already used by
the wallet purge logic.
Comment on lines
+1736
to
+1743
| Err(e) => { | ||
| tracing::warn!( | ||
| position = *position, | ||
| depth, | ||
| error = %e, | ||
| "shielded anchor probe: witness_at_depth failed at depth > 0; skipping depth" | ||
| ); | ||
| return Ok(None); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t let a skipped depth terminate the anchor probe.
Line 1743 maps depth>0 store errors to Ok(None), but Line 1804 treats None as “the selected note post-dates this checkpoint; deeper checkpoints cannot cover it” and breaks. That means a transient/depth-specific witness error can prevent probing older recorded checkpoints. Return a distinct outcome for “skip this depth” and only break on a real “note not present at this checkpoint”.
Suggested direction
+ enum DepthBuild {
+ Built(Vec<SpendableNote>, Anchor),
+ NoteNotCovered,
+ SkipDepth,
+ }
+
let build_at_depth = |depth: usize,
strict: bool|
- -> Result<Option<(Vec<SpendableNote>, Anchor)>, PlatformWalletError> {
+ -> Result<DepthBuild, PlatformWalletError> {
...
- Ok(None) => return Ok(None),
+ Ok(None) => return Ok(DepthBuild::NoteNotCovered),
...
- return Ok(None);
+ return Ok(DepthBuild::SkipDepth);
...
- Ok(Some((spends, anchor)))
+ Ok(DepthBuild::Built(spends, anchor))
};
...
- Some((spends, anchor)) if recorded.contains(&anchor.to_bytes()) => {
+ DepthBuild::Built(spends, anchor) if recorded.contains(&anchor.to_bytes()) => {
return Ok((spends, anchor));
}
- Some(_) => continue,
- None => break,
+ DepthBuild::Built(_, _) | DepthBuild::SkipDepth => continue,
+ DepthBuild::NoteNotCovered => break,Also applies to: 1802-1804
🤖 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/shielded/operations.rs` around lines
1736 - 1743, The anchor probe in shielded/operations.rs is conflating a
depth-specific witness failure with a real “note not present” result, causing
later checkpoint probing to stop early. Update the flow around the anchor probe
logic in the relevant witness lookup and the checkpoint loop so a depth>0
store/witness error returns a distinct “skip this depth” outcome instead of
Ok(None), and make the caller only break on the true post-dates/not-present
condition. Use the existing witness_at_depth and checkpoint selection logic to
distinguish transient probe failures from actual absence.
Comment on lines
+225
to
+291
| func clearLocalState( | ||
| modelContext: ModelContext, | ||
| network: Network, | ||
| walletIdsOnNetwork: Set<Data> | ||
| ) async { | ||
| // 1) Reset the Rust-owned state BEFORE touching disk. Without | ||
| // this the in-memory watermark survives and the next "Sync | ||
| // Now" resumes incrementally (fast) instead of doing a full | ||
| // rescan; a still-registered background pass could also | ||
| // re-persist the rows we're about to delete. Fail closed — | ||
| // the reset is load-bearing for the wipe, so abort (surfacing | ||
| // the error) rather than leave a half-cleared state. | ||
| if let walletManager { | ||
| do { | ||
| try await walletManager.resetPlatformAddressSyncState() | ||
| } catch { | ||
| lastError = "Failed to reset platform-address sync state: \(error.localizedDescription)" | ||
| SDKLogger.error(lastError ?? "") | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // 2) Zero the volatile balance/sync fields of this network's | ||
| // address rows IN PLACE — do NOT delete them. A | ||
| // `PersistentPlatformAddress` row is durable derivation state | ||
| // (bech32m address, 20-byte hash, 33-byte public key, | ||
| // account/address index, derivation path), not just a balance | ||
| // cache. The BLAST balance callback `persistAddressBalances` | ||
| // only *updates* existing rows (it skips by `addressHash` when | ||
| // the row is missing), and no sync re-emits the rows in-session: | ||
| // Rust's `reset_sync_state` preserves the address bijection, so | ||
| // there is no pool extension to re-fire the address-emit path. | ||
| // Deleting would therefore leave the next "Sync Now" with | ||
| // nothing to upsert against — balances (and the spend/signing | ||
| // derivation lookups) would stay gone until an app restart | ||
| // re-seeds the rows. Zeroing clears what the user sees while | ||
| // keeping the metadata the rescan needs. | ||
| do { | ||
| let addresses = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddress>()) | ||
| for row in addresses where walletIdsOnNetwork.contains(row.walletId) { | ||
| row.balance = 0 | ||
| row.nonce = 0 | ||
| row.isUsed = false | ||
| row.firstSeenHeight = 0 | ||
| row.lastSeenHeight = 0 | ||
| row.lastUpdated = Date() | ||
| } | ||
|
|
||
| // The sync-state watermark is pure volatile sync state (no | ||
| // durable metadata) and is what actually forces a full rescan, | ||
| // so delete it outright — the next sync re-creates it. | ||
| let networkRaw = network.rawValue | ||
| let syncStates = try modelContext.fetch(FetchDescriptor<PersistentPlatformAddressesSyncState>()) | ||
| for row in syncStates where row.networkRaw == networkRaw { | ||
| modelContext.delete(row) | ||
| } | ||
|
|
||
| try modelContext.save() | ||
| } catch { | ||
| lastError = "Failed to clear persisted platform-address state: \(error.localizedDescription)" | ||
| SDKLogger.error(lastError ?? "") | ||
| return | ||
| } | ||
|
|
||
| // 3) Zero the published display mirror — only on full success. | ||
| clearDisplay() | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate in-flight balance refreshes before clearing.
A sync completion can already have queued refreshBalanceSnapshot() before Clear runs; if that async snapshot returns after Line 290, it can republish pre-clear balances over the cleared UI. Add a service-local generation/token and check it before applying snapshot results.
🐛 Proposed direction
class PlatformBalanceSyncService: ObservableObject {
+ private var balanceSnapshotGeneration: UInt64 = 0 func clearLocalState(
modelContext: ModelContext,
network: Network,
walletIdsOnNetwork: Set<Data>
) async {
+ balanceSnapshotGeneration &+= 1
// 1) Reset the Rust-owned state BEFORE touching disk.- Task {
- await refreshBalanceSnapshot()
+ let generation = balanceSnapshotGeneration
+ Task {
+ await refreshBalanceSnapshot(generation: generation)
}- private func refreshBalanceSnapshot() async {
+ private func refreshBalanceSnapshot(generation: UInt64) async {
guard let wallet = platformAddressWallet else { return }
...
+ guard generation == balanceSnapshotGeneration else { return }
addressBalances = newBalances🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformBalanceSyncService.swift`
around lines 225 - 291, The clear flow in
PlatformBalanceSyncService.clearLocalState can be overwritten by an already
in-flight refreshBalanceSnapshot() that completes after the wipe. Add a
service-local generation/token that is bumped when clearLocalState starts or
succeeds, then have refreshBalanceSnapshot() and its snapshot-apply path verify
the current token before publishing results so stale balance data is ignored.
Make the check part of the same PlatformBalanceSyncService state used by
clearDisplay()/persisted snapshot updates.
Comment on lines
374
to
+381
| Button { | ||
| platformBalanceSyncService.clearDisplay() | ||
| Task { | ||
| await platformBalanceSyncService.clearLocalState( | ||
| modelContext: modelContext, | ||
| network: platformState.currentNetwork, | ||
| walletIdsOnNetwork: walletIdsOnNetwork | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Gate sync actions while Clear is in progress.
The Clear task is fire-and-forget, so Sync Now or a second Clear can interleave with the Rust reset and SwiftData zero/delete sequence. Track isClearing in PlatformBalanceSyncService and disable both actions while it is true.
🐛 Proposed direction
- .disabled(platformBalanceSyncService.isSyncing)
+ .disabled(platformBalanceSyncService.isSyncing || platformBalanceSyncService.isClearing) Button {
Task {
await platformBalanceSyncService.clearLocalState(
modelContext: modelContext,
network: platformState.currentNetwork,
walletIdsOnNetwork: walletIdsOnNetwork
)
} .buttonStyle(.borderedProminent)
.tint(.red)
.controlSize(.mini)
+ .disabled(platformBalanceSyncService.isSyncing || platformBalanceSyncService.isClearing)🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`
around lines 374 - 381, The Clear action can overlap with Sync Now or another
Clear because it is fire-and-forget, so add an `isClearing` state to
`PlatformBalanceSyncService` and use it to gate both actions. Update the
`clearLocalState` flow to set `isClearing` true for the full Rust reset plus
SwiftData cleanup, then reset it in a finally path, and wire `CoreContentView`
button states to disable both Clear and Sync Now while `isClearing` is true.
thepastaclaw
approved these changes
Jul 3, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Clean, conflict-free sync merge of v4.0-dev into v4.1-dev (git diff --cc is empty). All seven forwarded commits were individually reviewed at their origin PRs. Every finding surfaced by the sub-reviewers traces back to those already-reviewed underlying PRs — no merge-time defects, no v4.1-specific logic clobbered, no scope-in issues.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
Collaborator
|
Review-bot triage for the CodeRabbit batch: these findings are against the v4.0-dev commits being forwarded into v4.1-dev, not against a conflict/resolution introduced by this sync merge itself. The current automated review at |
Collaborator
|
I prepared fixes for the CodeRabbit review feedback in #3991, targeting this PR head branch. I could not push directly to Validation run on the follow-up branch:
Additional note: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Routine sync merge (same pattern as #3978): brings the 4.0.x release line's fixes up to the v4.1 development branch.
What was done?
Merged
v4.0-devintov4.1-dev— a clean merge, no conflicts. Carries:reconcile_address_infos)How Has This Been Tested?
cargo test -p platform-wallet --lib --features shielded→ 326 passed on the merged tree; CI runs the full suite.Breaking Changes
None (merge only).
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes