chore: remove legacy sync code #414
Conversation
📝 WalkthroughWalkthroughThis PR removes the legacy sequential synchronization pipeline, chainlock manager, terminal UI, persistent chain state, many sync-related types/FFI mappings, message handling, and associated tests/docs, significantly shrinking the public API surface and sync control flow. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
xdustinface
force-pushed
the
chore/drop-legacy-sync
branch
3 times, most recently
from
f5a88f0 to
9d1b5c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv/ARCHITECTURE.md (1)
235-242:⚠️ Potential issue | 🟠 MajorUpdate architecture doc to reflect removal of legacy sync.
This document still describes the parallel sync manager/event-bus architecture and legacy sync components, which the PR explicitly removes. Please align the architecture narrative with the new post-legacy sync design (or clearly mark this section as historical if that’s intended).
Based on learnings: “Applies to dash-spv/src/sync/**/*.rs : Use sequential phase-based synchronization via
SyncManagerfor organized sync coordination”.dash-spv/src/client/lifecycle.rs (1)
158-168:⚠️ Potential issue | 🟠 MajorLock order contradicts the documented ordering.
This path locks storage and then acquires
mempool_state; the module’s lock-ordering guidance calls formempool_statebefore storage, which can deadlock if another path follows the guideline. Load the state, drop the storage lock, then updatemempool_state.Proposed fix to honor lock ordering
- if let Some(state) = self - .storage - .lock() - .await - .load_mempool_state() - .await - .map_err(SpvError::Storage)? - { - *self.mempool_state.write().await = state; - } + let state = { + let mut storage = self.storage.lock().await; + storage + .load_mempool_state() + .await + .map_err(SpvError::Storage)? + }; + if let Some(state) = state { + *self.mempool_state.write().await = state; + }
🧹 Nitpick comments (2)
dash-spv/src/client/core.rs (1)
1-9: Stale documentation: Terminal UI accessors no longer exist.Line 9 mentions "Terminal UI accessors" but this PR removes all terminal UI functionality. Update the module-level doc comment to remove this reference.
📝 Suggested doc update
//! Core DashSpvClient struct definition and simple accessor methods. //! //! This module contains: //! - The main `DashSpvClient` struct definition //! - Simple getters for wallet, network, storage, etc. //! - Storage operations (clear_storage, clear_sync_state, clear_filters) //! - State queries (is_running, tip_hash, tip_height, chain_state, stats) //! - Configuration updates -//! - Terminal UI accessorsdash-spv/src/main.rs (1)
274-282: Consider simplifying the discarded result pattern.The
let _ = ...?pattern works but may confuse readers since_typically signals an intentionally unused value, yet the?propagates errors. A more idiomatic approach would clarify intent.💡 Alternative patterns
- let _ = wallet_manager.create_wallet_from_mnemonic( + wallet_manager.create_wallet_from_mnemonic( mnemonic_phrase.as_str(), "", 0, key_wallet::wallet::initialization::WalletAccountCreationOptions::default(), )?;Or if you want to be explicit about ignoring the return value:
- let _ = wallet_manager.create_wallet_from_mnemonic( + let _wallet_id = wallet_manager.create_wallet_from_mnemonic( mnemonic_phrase.as_str(), "", 0, key_wallet::wallet::initialization::WalletAccountCreationOptions::default(), )?;
xdustinface
force-pushed
the
chore/drop-legacy-sync
branch
from
9d1b5c7 to
08605e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/main.rs (1)
276-281:⚠️ Potential issue | 🟡 MinorUse
WalletAccountCreationOptions::Defaultinstead ofdefault()to match codebase conventions.The enum variant
WalletAccountCreationOptions::Defaultshould be used explicitly rather than the trait methoddefault(). All tests, examples, and implementations throughout the codebase consistently use the explicit enum variant form. While both are technically valid due to the#[derive(Default)]macro on the enum, using the explicit form is the established idiom in this codebase.wallet_manager.create_wallet_from_mnemonic( mnemonic_phrase.as_str(), "", 0, key_wallet::wallet::initialization::WalletAccountCreationOptions::Default, )?;
🧹 Nitpick comments (1)
dash-spv/ARCHITECTURE.md (1)
237-241: Keep the refactoring list consistent with current types layout.The refactoring list now only mentions
types/events.rswithMempoolRemovalReason. If other type splits were removed or completed as part of dropping legacy sync, consider updating or pruning this list so it reflects the current, intended layout and doesn’t imply pending work that no longer exists.
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
we added 2k aprox lines with the new sync right??, feels like a lot of lines tbh
Yes, around 2k lines. I don't think it makes sense to judge whether its a lot or not just by the total number of lines. |
2 participants
Drop all the code relevant to the legacy sync module.
Summary by CodeRabbit
Breaking Changes
Documentation
Tests