Skip to content

backport: refactor(wallet): simplify platform sync by removing PlatformSyncMode#635

Merged
lklimek merged 12 commits into
v1.0-devfrom
zk-extract/platform-sync-simplification
Feb 25, 2026
Merged

backport: refactor(wallet): simplify platform sync by removing PlatformSyncMode#635
lklimek merged 12 commits into
v1.0-devfrom
zk-extract/platform-sync-simplification

Conversation

@lklimek
Copy link
Copy Markdown
Contributor

@lklimek lklimek commented Feb 24, 2026

Summary

  • Removes the PlatformSyncMode enum (Auto/ForceFull/TerminalOnly) and the 250-line apply_recent_balance_changes() method
  • Replaces complex full/terminal sync logic with SDK-managed incremental sync via sync_address_balances(provider, config, last_ts)
  • Removes last_full_sync_balance from PlatformAddressInfo and last_terminal_block from DB
  • Simplifies RefreshMode to just All/CoreOnly/PlatformOnly
  • Updates Platform SDK to rev 0fa82e6652097d17a700d8bcc006d6b2aa922c6e which includes the incremental sync API

Test plan

Manual test scenarios: docs/ai-design/2026-02-24-platform-sync-simplification/manual-test-scenarios.md

  • Fresh wallet sync (no prior sync state)
  • Incremental sync (wallet already has balances)
  • Refresh mode dropdown (All / Core Only / Platform Only)
  • Balance after funding via asset lock / UTXOs
  • Balance after withdrawal
  • Locked wallet error path
  • Network switching (Mainnet/Testnet/Regtest)
  • Regtest empty balance tree
  • No DAPI endpoints
  • Database migration from old schema
  • Pending refresh after transfer

🤖 Co-authored by Claudius the Magnificent AI Agent

Remove the PlatformSyncMode enum (Auto/ForceFull/TerminalOnly) and
terminal sync logic (apply_recent_balance_changes, last_terminal_block,
last_full_sync_balance). The SDK now handles incremental sync internally
via AddressProvider::current_balances() and last_sync_height().

Key changes:
- Remove PlatformSyncMode enum from backend_task::wallet
- Simplify fetch_platform_address_balances to use new SDK API with
  stored state (with_stored_state, current_balances, last_sync_height)
- Change CoreTask::RefreshWalletInfo to use bool instead of
  Option<PlatformSyncMode>
- Remove last_full_sync_balance from PlatformAddressInfo
- Simplify database sync info to 2-tuple (timestamp, height)
- Remove set_last_terminal_block from database
- Simplify RefreshMode enum (remove PlatformFull, PlatformTerminal,
  CoreAndPlatformFull, CoreAndPlatformTerminal variants)

Note: requires updated dash-sdk with new sync_address_balances API.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek self-assigned this Feb 24, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 24, 2026

Warning

Rate limit exceeded

@lklimek has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 28 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 260d18d and b6bc156.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (17)
  • Cargo.toml
  • docs/ai-design/2026-02-24-platform-sync-simplification/manual-test-scenarios.md
  • src/backend_task/core/mod.rs
  • src/backend_task/identity/register_identity.rs
  • src/backend_task/identity/top_up_identity.rs
  • src/backend_task/mod.rs
  • src/backend_task/wallet/fetch_platform_address_balances.rs
  • src/backend_task/wallet/fund_platform_address_from_asset_lock.rs
  • src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs
  • src/backend_task/wallet/mod.rs
  • src/backend_task/wallet/withdraw_from_platform_address.rs
  • src/context/transaction_processing.rs
  • src/context/wallet_lifecycle.rs
  • src/database/wallet.rs
  • src/model/wallet/asset_lock_transaction.rs
  • src/model/wallet/mod.rs
  • src/ui/wallets/wallets_screen/mod.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch zk-extract/platform-sync-simplification

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

lklimek and others added 2 commits February 24, 2026 11:07
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek
Copy link
Copy Markdown
Contributor Author

lklimek commented Feb 24, 2026

🔍 Audit Summary — PR #635

Reviewed by: Claude Code with a 3-agent team:

  • rust-quality-reviewer (rust-developer, Opus) — code quality, correctness, idiomatic Rust
  • security-reviewer (security-engineer, Opus) — fund safety, transaction integrity, race conditions
  • consistency-reviewer (project-reviewer, Sonnet) — DB migrations, dead code, convention adherence

Overall Assessment

Risk: LOW-MEDIUM. This PR is a net improvement — the UTXO two-phase select/remove pattern is a significant safety upgrade, the platform sync simplification removes ~250 lines of error-prone manual balance delta arithmetic, and the fee calculation is properly tested. The findings below are mostly pre-existing gaps surfaced by the refactoring, not new regressions.

Findings

# Severity Location Description Agents
1 🟡 Medium asset_lock_transaction.rs:430+ UTXOs removed before broadcast — if broadcast fails after signing, UTXOs are removed from wallet tracking but never spent on-chain. Wallet shows incorrect balance until next UTXO reload. Security, Rust
2 🟡 Medium register_identity.rs:129+, top_up_identity.rs:145+ Store-after-broadcast race — asset lock TX is broadcast before being stored in DB, leaving an SPV InstantSend race window. fund_platform_address_from_wallet_utxos.rs was correctly fixed to store-before-broadcast, but these two paths were not. Security
3 🟡 Medium initialization.rs:319,358 Fresh-install schema retains removed columnslast_terminal_block and last_full_sync_balance are still in create_tables() DDL, but no code reads/writes them. No DB version bump to clean this up. Consistency
4 🟠 Low database/wallet.rs:1142 Column semantically repurposedlast_platform_sync_checkpoint now stores SDK new_sync_height, different semantics from old "checkpoint". Rust variable renamed but SQL column not. Consistency
5 🟠 Low register_identity.rs:201,238, top_up_identity.rs:179,223 .unwrap() on RwLock in FundWithUtxo path — adjacent FundWithWallet paths were correctly updated to .map_err(), but FundWithUtxo still uses .unwrap(). Rust, Security
6 🟠 Low database/wallet.rs:976 _is_sync_operation parameter does nothing — underscore prefix suppresses warning but parameter is dead. All callers pass values that are ignored. All 3
7 🟠 Low model/wallet/mod.rs:2002 set_platform_address_info_from_sync is trivial alias — now identical to set_platform_address_info. Dead abstraction. Consistency
8 🟠 Low create_asset_lock.rs:20,63 Dead _used_utxos bindings — UTXO cleanup moved into wallet method, but return values still captured as dead bindings. Rust
9 ⚪ Nitpick fetch_platform_address_balances.rs:23 wallets.read().unwrap() — inconsistent with .map_err() improvements elsewhere in this PR. Rust, Security
10 ⚪ Nitpick fetch_platform_address_balances.rs:45-46 Unnecessary let mut provider = provider; rebinding. Rust

Pre-existing / Outside-diff Issues

Findings #1, #2, #3, #5, #8 affect files not in the GitHub diff (they're from backported commits already merged to v1.0-dev). These are noted for awareness but inline comments cannot be placed on them.

Finding #1 detail: The remove_selected_utxos call happens inside asset_lock_transaction_from_private_key (after signing, before broadcast). If broadcast subsequently fails, UTXOs are lost from tracking. Recommendation: either move remove_selected_utxos to after successful broadcast, add UTXO restoration on broadcast failure, or trigger a UTXO reload on error paths.

Finding #2 detail: register_identity.rs broadcasts at line 129, stores at line 135. top_up_identity.rs broadcasts at line 145, stores at line 148. The fund_platform_address_from_wallet_utxos.rs shows the correct pattern (store at line 87, broadcast at line 109, cleanup on failure at line 115). Apply the same pattern to the identity paths.

Finding #3 detail: create_tables() DDL in initialization.rs still includes last_terminal_block INTEGER DEFAULT 0 (line 319) and last_full_sync_balance INTEGER DEFAULT NULL (line 358). Bump DEFAULT_DB_VERSION to 28, remove these from fresh-install DDL. Migration stubs for existing users are fine as-is.

Positive Observations

  • Two-phase UTXO pattern (select_unspent_utxos_for + remove_selected_utxos) eliminates the old fund-loss bug where signing failure permanently removed UTXOs
  • Overflow-safe arithmeticchecked_add and i64::try_from replace raw as casts in financial calculations
  • Excellent test coveragecalculate_asset_lock_fee has 7 targeted tests with "BUG TEST" comments explaining what each prevents
  • Clean PlatformSyncMode removal — no orphan references anywhere, RefreshMode correctly collapsed to 3 variants
  • Store-before-broadcast pattern correctly applied in fund_platform_address_from_wallet_utxos.rs
  • Comprehensive manual test scenarios — 11 scenarios with edge-case checklist
  • SPV-aware reload logicreload_utxos correctly returns no-op in SPV mode

Redundancy

3 agents reported 10 unique findings. Cross-agent overlap:

🤖 Co-authored by Claudius the Magnificent AI Agent

lklimek and others added 4 commits February 24, 2026 22:50
- DB schema v28: drop obsolete columns (last_terminal_block,
  last_full_sync_balance), rename last_platform_sync_checkpoint →
  last_platform_sync_height, with SQLite ≥3.35 runtime check
- Store asset lock TX before broadcast to prevent SPV InstantSend race
- Defer UTXO removal until after successful broadcast
- Replace .unwrap() on RwLock with .map_err() to avoid panics
- Remove unused _is_sync_operation param and set_platform_address_info_from_sync wrapper
- Fix redundant let-mut rebinding in fetch_platform_address_balances
- Extract broadcast_and_commit_asset_lock() on AppContext to consolidate
  the store→broadcast→cleanup→UTXO-removal pattern from 5 code paths

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… interface (#653)

- Bump DEFAULT_DB_VERSION 27 → 28
- Drop last_terminal_block from wallet table (unused after sync simplification)
- Drop last_full_sync_balance from platform_address_balances table
- Rename last_platform_sync_checkpoint → last_platform_sync_height
- Add runtime SQLite ≥3.35 check (required for DROP COLUMN)
- Idempotent migration: checks column existence before each ALTER
- Remove unused _is_sync_operation param from set_platform_address_info()
- Remove set_platform_address_info_from_sync() wrapper
- Fix redundant let-mut rebinding in fetch_platform_address_balances

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread src/database/wallet.rs Outdated
Comment thread src/database/wallet.rs
Comment thread src/model/wallet/mod.rs Outdated
Comment thread src/backend_task/wallet/fetch_platform_address_balances.rs
@lklimek lklimek marked this pull request as ready for review February 25, 2026 11:23
@lklimek lklimek requested a review from Copilot February 25, 2026 11:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR backports a Platform sync simplification by removing the legacy PlatformSyncMode (auto/full/terminal) and replacing the previous full+terminal sync logic with SDK-managed incremental syncing via sync_address_balances(provider, config, last_ts). It also updates persistence to drop obsolete sync columns and introduces a shared “store → broadcast → commit UTXO removal” helper for asset lock flows.

Changes:

  • Simplifies refresh controls (UI + task APIs) to a boolean “sync Platform” flag and RefreshMode::{All, CoreOnly, PlatformOnly}.
  • Refactors Platform address balance sync to use SDK incremental sync and new persisted (last_sync_timestamp, last_sync_height) state.
  • Adjusts asset-lock flows to persist-before-broadcast and defer UTXO removal until after successful broadcast.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/ui/wallets/wallets_screen/mod.rs Simplifies refresh dropdown options and updates refresh actions to new task APIs.
src/model/wallet/mod.rs Removes last_full_sync_balance; adds incremental-sync baseline fields/methods to WalletAddressProvider.
src/model/wallet/asset_lock_transaction.rs Stops removing selected UTXOs inside asset-lock builder; returns them to caller.
src/database/wallet.rs Drops last_full_sync_balance read/write; updates Platform sync info getters/setters.
src/database/initialization.rs Bumps DB version to 28; adds migration to drop obsolete columns and rename checkpoint column.
src/context/wallet_lifecycle.rs Updates DB writes to new set_platform_address_info signature.
src/context/transaction_processing.rs Adds broadcast_and_commit_asset_lock helper and related imports.
src/backend_task/wallet/withdraw_from_platform_address.rs Updates Platform refresh call to new fetch_platform_address_balances(seed_hash) signature.
src/backend_task/wallet/mod.rs Removes PlatformSyncMode and its use from wallet task definitions.
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs Switches to new broadcast/store/commit helper and simplified balance refresh.
src/backend_task/wallet/fund_platform_address_from_asset_lock.rs Updates Platform refresh call to new fetch_platform_address_balances(seed_hash) signature.
src/backend_task/wallet/fetch_platform_address_balances.rs Replaces legacy full/terminal logic with SDK incremental sync and persisted sync state.
src/backend_task/mod.rs Updates wallet-task dispatch for changed FetchPlatformAddressBalances shape.
src/backend_task/identity/top_up_identity.rs Uses new broadcast/store/commit helper; improves poisoned-lock error handling.
src/backend_task/identity/register_identity.rs Uses new broadcast/store/commit helper; improves poisoned-lock error handling.
src/backend_task/core/mod.rs Replaces Option<PlatformSyncMode> with bool for “sync Platform” during Core refresh.
docs/ai-design/2026-02-24-platform-sync-simplification/manual-test-scenarios.md Adds manual test scenarios for simplified sync + migration paths.
Cargo.toml Bumps dash-sdk git rev to include incremental sync API.
Cargo.lock Updates lockfile to match new dash-sdk rev and transitive dependency changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/database/initialization.rs Outdated
Comment thread src/model/wallet/asset_lock_transaction.rs
Comment thread src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs
Comment thread src/context/transaction_processing.rs
Comment thread src/context/transaction_processing.rs
Comment thread src/model/wallet/mod.rs
lklimek and others added 4 commits February 25, 2026 12:55
The v28 schema changes (drop obsolete sync columns, rename
last_platform_sync_checkpoint → last_platform_sync_height) will be
applied separately and should not ship on this branch.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve merge conflict in fund_platform_address_from_wallet_utxos.rs
(keep simplified fetch_platform_address_balances call without PlatformSyncMode).
Fix wallets_screen/mod.rs: get_platform_sync_info returns (u64, u64) on
this branch, not a 3-tuple.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove dead `_is_sync_operation` parameter from `Database::set_platform_address_info`
  and all call sites (Finding #6)
- Add doc comment on `set_platform_sync_info` explaining column name drift:
  `last_platform_sync_checkpoint` now stores SDK sync height (Finding #4)
- Remove trivial `set_platform_address_info_from_sync` delegate and update
  callers to use `set_platform_address_info` directly (Finding #7)
- Combine unnecessary `let provider` + `let mut provider = provider`
  rebinding into single `let mut` block (Finding #10)
- Document UTXO selection race window on `broadcast_and_commit_asset_lock`:
  std::sync::RwLock guard is !Send so it cannot span async broadcast

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek enabled auto-merge (squash) February 25, 2026 12:39
@lklimek lklimek merged commit 3c7fb0b into v1.0-dev Feb 25, 2026
4 checks passed
@lklimek lklimek deleted the zk-extract/platform-sync-simplification branch February 25, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants