refactor: remove SyncState::Initializing variant#465
Conversation
📝 WalkthroughWalkthroughRemoved the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
| WaitingForConnections = 1, | ||
| WaitForEvents = 2, | ||
| WaitForEvents = 1, | ||
| WaitingForConnections = 2, |
There was a problem hiding this comment.
Two things:
- Why does the enum start in 1 right now??
- WaitForEvents doesn't seem right, what about Idle??
There was a problem hiding this comment.
- Why does the enum start in 1 right now??
Missed that, fixed!
- WaitForEvents doesn't seem right, what about Idle??
Thats out of scope here. Im not against it but i think WaitForEvents is fine too as thats what the managers actually do in that state.
1d16871 to
b6d56fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/src/types.rs`:
- Around line 53-58: FFISyncState's numeric discriminants changed and break the
FFI ABI; restore the original explicit discriminant values used by the C header
so consumers don't need to recompile (or intentionally bump the ABI). Update the
enum FFISyncState declaration to assign the original numeric values for each
variant (e.g., ensure Syncing and Synced use the previous discriminants) so the
in-memory layout matches the generated cbindgen header.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
dash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/types.rsdash-spv-ffi/tests/test_types.rsdash-spv/src/sync/block_headers/manager.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/chainlock/manager.rsdash-spv/src/sync/filter_headers/manager.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/instantsend/manager.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/progress.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/sync/sync_manager.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- dash-spv/src/sync/instantsend/manager.rs
- dash-spv-ffi/tests/test_types.rs
- dash-spv/src/sync/sync_manager.rs
- dash-spv/src/sync/filter_headers/manager.rs
- dash-spv/src/sync/sync_coordinator.rs
The `Initializing` state was only used as a default before `initialize()` was called. Replace it with `WaitForEvents` as the default state to simplify the state machine.
b6d56fd to
b05fa62
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/include/dash_spv_ffi.h`:
- Around line 23-27: The enum discriminants for WaitForEvents,
WaitingForConnections, Syncing, Synced, and Error were renumbered, which breaks
FFI/ABI consumers; revert to the original numeric values (or explicitly assign
the legacy integers) for the symbols in the header (the enum defined in
dash_spv_ffi.h) so precompiled clients interpret runtime states correctly, and
if you need different semantics change only the Rust-side state mapping rather
than the exported discriminant values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
dash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/types.rsdash-spv-ffi/tests/test_types.rsdash-spv/src/sync/block_headers/manager.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/chainlock/manager.rsdash-spv/src/sync/filter_headers/manager.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/instantsend/manager.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/progress.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/sync/sync_manager.rs
✅ Files skipped from review due to trivial changes (1)
- dash-spv/src/sync/masternodes/manager.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- dash-spv/src/sync/sync_manager.rs
- dash-spv/src/sync/filters/manager.rs
- dash-spv/src/sync/sync_coordinator.rs
- dash-spv/src/sync/filter_headers/manager.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- dash-spv/src/sync/block_headers/manager.rs
- dash-spv-ffi/src/types.rs
The
Initializingstate was only used as a default beforeinitialize()was called. Replace it withWaitForEventsas the default state to simplify the state machine.Summary by CodeRabbit
Refactor
Tests