feat(ffi): add dash_spv_ffi_config_clear_peers#591
feat(ffi): add dash_spv_ffi_config_clear_peers#591QuantumExplorer wants to merge 3 commits intov0.42-devfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new exported FFI function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
|
|
|
When creating an SPV config for regtest, a default peer at 127.0.0.1:19899 is added. This doesn't match dashmate's Docker setup which maps Core P2P to different ports (e.g. 20001). FFI consumers (iOS/Swift) couldn't clear these defaults before adding their own peers, causing the SPV client to enter exclusive mode with a dead peer alongside the intended one. Adds dash_spv_ffi_config_clear_peers() so FFI consumers can remove default peers before adding custom ones via add_peer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/remove-hardcoded-regtest-peer
branch
from
4d7ea6e to
de085dc
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/remove-hardcoded-regtest-peer
branch
from
de085dc to
93eed45
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: 1
🧹 Nitpick comments (1)
dash-spv-ffi/src/config.rs (1)
169-177: Clarify the lifecycle constraint in docs.At Line 178, this mutates only the config object. Since
dash_spv_ffi_client_newclones config at creation time, calling this after client creation has no effect on the running client. Please make that explicit in this doc block to prevent integration misuse.Suggested doc update
/// Removes all configured peers from the configuration /// /// This is useful when the caller wants to start with a clean slate /// before adding custom peers via `dash_spv_ffi_config_add_peer`. +/// Must be called before `dash_spv_ffi_client_new`; the client clones +/// configuration at creation time, so later changes are not applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/config.rs` around lines 169 - 177, Update the doc comment for the function that removes all peers (the doc block above dash_spv_ffi_config_remove_all_peers) to explicitly state this mutates only the FFIClientConfig instance and does not affect any already-created client because dash_spv_ffi_client_new clones the config at creation time; mention that callers must remove peers before calling dash_spv_ffi_client_new if they expect the running client to reflect the change and reiterate the existing safety requirement that the config pointer must remain valid for the duration of the call.
🤖 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/config.rs`:
- Around line 178-184: Add a colocated unit test under #[cfg(test)] in the same
file that constructs a ClientConfig with at least one peer, wraps it in an
FFIClientConfig (matching how other tests do it), calls the exported unsafe
function dash_spv_ffi_config_clear_peers within an unsafe block, asserts the
function returns FFIErrorCode::Success as i32, and then asserts the underlying
ClientConfig.peers is empty; reference the types and symbols FFIClientConfig,
ClientConfig, and dash_spv_ffi_config_clear_peers when locating where to add the
test.
---
Nitpick comments:
In `@dash-spv-ffi/src/config.rs`:
- Around line 169-177: Update the doc comment for the function that removes all
peers (the doc block above dash_spv_ffi_config_remove_all_peers) to explicitly
state this mutates only the FFIClientConfig instance and does not affect any
already-created client because dash_spv_ffi_client_new clones the config at
creation time; mention that callers must remove peers before calling
dash_spv_ffi_client_new if they expect the running client to reflect the change
and reiterate the existing safety requirement that the config pointer must
remain valid for the duration of the call.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71f7a909-12c0-4523-9030-85bb2c0aadd4
📒 Files selected for processing (1)
dash-spv-ffi/src/config.rs
| pub unsafe extern "C" fn dash_spv_ffi_config_clear_peers(config: *mut FFIClientConfig) -> i32 { | ||
| null_check!(config); | ||
|
|
||
| let cfg = unsafe { &mut *((*config).inner as *mut ClientConfig) }; | ||
| cfg.peers.clear(); | ||
| FFIErrorCode::Success as i32 | ||
| } |
There was a problem hiding this comment.
Add direct unit coverage for the new exported function.
Lines 178-184 introduce new FFI behavior but there is no colocated test in this module diff. Please add a unit test that verifies peers are removed (e.g., create config with default peer, call clear, assert peer list is empty).
As per coding guidelines, "Write unit tests for new functionality in Rust code" and "Place unit tests alongside code with #[cfg(test)] attribute".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv-ffi/src/config.rs` around lines 178 - 184, Add a colocated unit test
under #[cfg(test)] in the same file that constructs a ClientConfig with at least
one peer, wraps it in an FFIClientConfig (matching how other tests do it), calls
the exported unsafe function dash_spv_ffi_config_clear_peers within an unsafe
block, asserts the function returns FFIErrorCode::Success as i32, and then
asserts the underlying ClientConfig.peers is empty; reference the types and
symbols FFIClientConfig, ClientConfig, and dash_spv_ffi_config_clear_peers when
locating where to add the test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #591 +/- ##
=============================================
- Coverage 67.40% 67.05% -0.36%
=============================================
Files 318 318
Lines 67020 67026 +6
=============================================
- Hits 45175 44943 -232
- Misses 21845 22083 +238
|
Tests cover: - clearing peers after adding multiple peers - clearing an already-empty peer list succeeds - null pointer returns NullPointer error code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Lets maybe just instead completely get rid of default_peers_for_network and add the peer where we need it? I don't think this "add the default peer for regtest" behaviour is good. I had similar problems before with this default peer behaviour on regtest.
2 participants
Summary
dash_spv_ffi_config_clear_peers()FFI function so iOS/Swift consumers can remove default peers before adding custom onesProblem
When creating an SPV config for regtest via
dash_spv_ffi_config_new(FFINetwork::Regtest), a default peer at127.0.0.1:19899is baked in. This doesn't match dashmate's Docker setup which maps Core P2P to port 20001. The iOS app callsdash_spv_ffi_config_add_peer("127.0.0.1:20001")but the dead 19899 peer remains, causing reconnect spam and intermittent QRInfo timeouts when requests get routed to it.Rust consumers can already do
config.peers.clear()since the field ispub, but FFI consumers had no way to clear defaults.Usage
Test plan
dash-spv-fficonfig tests pass (9/9)dash-spvunit tests pass (243/243)clear_peers+add_peerresults in only the intended peer🤖 Generated with Claude Code
Summary by CodeRabbit