-
Notifications
You must be signed in to change notification settings - Fork 10
refactor: drop useless DashSpvClient.sync_to_tip
#363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
It's basically doing nothing except logging some things.
084f124 to
b19bd7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/README.md (1)
52-69: Add theCancellationTokenimport so the example compiles.The README snippet uses
CancellationTokenwithout importing it. All examples and implementations in the codebase import it fromtokio_util::sync, which is a direct dependency.🩹 Proposed doc fix
use dash_spv::{ClientConfig, DashSpvClient}; +use tokio_util::sync::CancellationToken;dash-spv-ffi/src/client.rs (1)
569-575: Pre-existing bug: early return leaks the client from the guard.If the first
sync_progress()call fails (lines 571-574), the function returns without restoringspv_clientto the guard. This leavesinnerholdingNone, causing subsequent operations to fail with "Client not initialized". Compare with lines 586-588 which correctly restore the client before returning.Proposed fix
let start_height = match spv_client.sync_progress().await { Ok(progress) => progress.header_height, Err(e) => { tracing::error!("Failed to get initial height: {}", e); + let mut guard = client.inner.lock().unwrap(); + *guard = Some(spv_client); return Err(e); } };
🧹 Nitpick comments (1)
dash-spv-ffi/src/client.rs (1)
40-47: Consider simplifyingCallbackInfoenum.After removing the
Simplevariant,CallbackInfonow has only one variant (Detailed). While keeping it as an enum provides flexibility for future additions, you could simplify to a struct if no other variants are planned.
ZocoLini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FR i have never checked this method body before, definitely not the behavior i expected from a method with such name
DashSpvClient.sync_to_tipDashSpvClient.sync_to_tip
It's basically doing nothing except logging some things.
Based on:
Summary by CodeRabbit
Release Notes
Refactor
sync_to_tip()andstart_sync()public methods. Synchronization is now asynchronous via therun()method with cancellation token support.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.