Refactor: config builder #370
Conversation
… is built using the config
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
📝 WalkthroughWalkthroughThis PR refactors the configuration API from direct object mutation to a builder-pattern approach. It replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Client Code
participant ConfigBuilder
participant Config
participant DashSpvClient
participant Storage
participant Network
rect rgb(200, 150, 255, 0.5)
Note over Client Code,Network: Old Flow (Direct Config)
Client Code->>Config: ClientConfig::new(network)
Config-->>Client Code: config
Client Code->>Config: config.set_data_dir(path)
Client Code->>Storage: DiskStorageManager::new(path)
Storage-->>Client Code: storage
Client Code->>DashSpvClient: client = DashSpvClient::new(config, storage, ...)
DashSpvClient-->>Client Code: client
end
rect rgb(150, 200, 255, 0.5)
Note over Client Code,Network: New Flow (Builder Pattern)
Client Code->>ConfigBuilder: ConfigBuilder::mainnet()
ConfigBuilder-->>Client Code: builder
Client Code->>ConfigBuilder: builder.storage_path(path)
ConfigBuilder-->>Client Code: builder
Client Code->>Config: config = builder.build()
Config-->>Client Code: config
Client Code->>Storage: DiskStorageManager::new(&config)
Storage-->>Client Code: storage
Client Code->>DashSpvClient: client = DashSpvClient::new(config, storage, ...)
DashSpvClient-->>Client Code: client
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
dash-spv-ffi/tests/c_tests/test_event_draining.c (1)
28-48: Use builder flow; current call passes a config pointer to a builder API.
dash_spv_ffi_config_builder_set_validation_modeexpectsFFIConfigBuilder*, butconfigis created viadash_spv_ffi_config_new. This ABI mismatch can cause undefined behavior. Please migrate this setup to the builder flow (storage path + validation mode) and build the final config before creating the client.🐛 Proposed fix (builder-based setup)
- // Create config - FFIClientConfig* config = dash_spv_ffi_config_new(REGTEST); - TEST_ASSERT(config != NULL); + // Create builder + FFIConfigBuilder* builder = dash_spv_ffi_config_builder_regtest(); + TEST_ASSERT(builder != NULL); // Set data directory to temporary location char temp_path[256]; snprintf(temp_path, sizeof(temp_path), "/tmp/dash_spv_test_%d", getpid()); - int result = dash_spv_ffi_config_set_data_dir(config, temp_path); + int result = dash_spv_ffi_config_builder_set_storage_path(builder, temp_path); TEST_ASSERT(result == FFIErrorCode_Success); // Set validation mode to none for faster testing - result = dash_spv_ffi_config_builder_set_validation_mode(config, FFIValidationMode_None); + result = dash_spv_ffi_config_builder_set_validation_mode(builder, FFIValidationMode_None); TEST_ASSERT(result == FFIErrorCode_Success); + // Build config + FFIConfig* config = dash_spv_ffi_config_builder_build(builder); + TEST_ASSERT(config != NULL); + // Create client FFIDashSpvClient* client = dash_spv_ffi_client_new(config); TEST_ASSERT(client != NULL);dash-spv-ffi/tests/integration/test_full_workflow.rs (1)
24-29: Builder API mismatch: test uses wrong config type for builder settersLines 28-29 call
dash_spv_ffi_config_builder_set_validation_mode()anddash_spv_ffi_config_builder_set_max_peers()withconfig, butdash_spv_ffi_config_new()returns*mut FFIClientConfig, not*mut FFIConfigBuilder. Builder setters expect the builder type.Use the correct builder API: call
dash_spv_ffi_config_builder_mainnet()(or appropriate network) to get a builder, apply builder setters to that, thendash_spv_ffi_config_builder_build()to get the final config. Follow the builder pattern lifecycle: create builder → set options → build → destroy builder (not mix old and new APIs).dash-spv-ffi/tests/c_tests/test_integration.c (1)
57-63: Replace non-existent config API with builder pattern.The test calls undefined functions
dash_spv_ffi_config_new()anddash_spv_ffi_config_set_data_dir(), which do not exist in the FFI API. Additionally, it attempts to call builder functions (dash_spv_ffi_config_builder_set_validation_mode,dash_spv_ffi_config_builder_set_max_peers) on a non-existent object.The correct pattern is:
- Create a builder:
dash_spv_ffi_config_builder_regtest()returns*mut FFIConfigBuilder- Configure the builder using
dash_spv_ffi_config_builder_set_*()functions- Build the config:
dash_spv_ffi_config_builder_build()returns*mut FFIConfig- Use config functions like
dash_spv_ffi_config_add_peer()on the resultingFFIConfigdash-spv-ffi/src/bin/ffi_cli.rs (1)
148-165: Add null check forcfgafterbuild()to prevent undefined behavior.Per the
dash_spv_ffi_config_builder_buildimplementation inconfig.rs(lines 360-377), the function returnsnull_mut()on build failure. Using a nullcfgpointer at line 153 (dash_spv_ffi_config_add_peer) or line 165 (dash_spv_ffi_client_new) would cause undefined behavior.🐛 Proposed fix
let cfg = dash_spv_ffi_config_builder_build(cfg_builder); + if cfg.is_null() { + eprintln!( + "Failed to build config: {}", + ffi_string_to_rust(dash_spv_ffi_get_last_error()) + ); + std::process::exit(1); + } + if let Some(peers) = matches.get_many::<String>("peer") {dash-spv-ffi/dash_spv_ffi.h (1)
591-605: Type mismatch in generated FFI header: "builder" functions incorrectly typed asFFIClientConfig*instead ofFFIConfigBuilder*.The C header declares that
dash_spv_ffi_config_builder_set_validation_mode()anddash_spv_ffi_config_builder_set_max_peers()acceptstruct FFIClientConfig *config, but the Rust implementations correctly take*mut FFIConfigBuilder. This type mismatch breaks the API contract—C callers will pass the wrong pointer type, as evidenced bytest_full_workflow.rs(line 28-29) passing aconfig(fromdash_spv_ffi_config_new()) to builder functions.The Rust source is correct. The issue originates from cbindgen's code generation—likely due to the forward-declaration of
FFIClientConfigincbindgen.tomlcausing type confusion. Additionally, functions declared in the header (dash_spv_ffi_config_new,dash_spv_ffi_config_mainnet, etc.) are missing from the Rust implementation.Fix cbindgen configuration or rename builder functions and their parameter types to properly reflect
FFIConfigBuilderin the generated header.dash-spv/src/storage/mod.rs (1)
142-156: TempDir is dropped whenwith_temp_dir()returns, before test operations complete.The
TempDircreated on line 148 goes out of scope and deletes the temporary directory when the function returns (line 156). Subsequent storage operations likestore_headers()would target a non-existent directory. Other tests in the same file (e.g., lines 433–435, 478–480) explicitly keepTempDiralive throughout their scope, indicating directory persistence is required.Return the
TempDiralongside the storage to ensure the directory persists for the test's duration:Suggested fix
#[cfg(test)] -pub async fn with_temp_dir() -> StorageResult<Self> { +pub async fn with_temp_dir() -> StorageResult<(Self, tempfile::TempDir)> { use tempfile::TempDir; use crate::ConfigBuilder; let temp_dir = TempDir::new()?; - Self::new( + let storage = Self::new( &ConfigBuilder::testnet() .storage_path(temp_dir.path()) .build() .expect("Valid configuration"), ) - .await + .await?; + Ok((storage, temp_dir)) }dash-spv-ffi/FFI_API.md (2)
1230-1243: Stale type definition:FFIClientConfigstill listed but was replaced.Line 1235 lists
FFIClientConfigin Core Types, but according to the summary and code changes, this type was replaced byFFIConfigandFFIConfigBuilder.Suggested fix
### Core Types - `FFIDashSpvClient` - SPV client handle -- `FFIClientConfig` - Client configuration +- `FFIConfig` - Client configuration (built from builder) +- `FFIConfigBuilder` - Configuration builder - `FFISyncProgress` - Synchronization progress
1262-1288: Usage example uses old API pattern.Lines 1267-1288 show
dash_spv_ffi_config_testnet()returningFFIClientConfig*, but the new API uses a builder pattern:dash_spv_ffi_config_builder_testnet()returnsFFIConfigBuilder*, thendash_spv_ffi_config_builder_build()returnsFFIConfig*.Suggested fix
### Basic SPV Client Usage ```c -// Create configuration -FFIClientConfig* config = dash_spv_ffi_config_testnet(); +// Create configuration using builder pattern +FFIConfigBuilder* builder = dash_spv_ffi_config_builder_testnet(); +FFIConfig* config = dash_spv_ffi_config_builder_build(builder); // Create client FFIDashSpvClient* client = dash_spv_ffi_client_new(config);
🤖 Fix all issues with AI agents
In `@dash-spv-ffi/tests/security/test_security.rs`:
- Around line 96-97: The test calls dash_spv_ffi_config_builder_set_max_peers
with a config pointer created by dash_spv_ffi_config_new (ABI mismatch); to fix,
allocate and use a builder via dash_spv_ffi_config_builder_new, call
dash_spv_ffi_config_builder_set_max_peers(builder, 10), then build the config
with dash_spv_ffi_config_builder_build (or equivalent) and assert results on the
build result or the produced config; update the assertions that currently use
dash_spv_ffi_config_new (also at the other occurrence) to follow this builder ->
set_max_peers -> build flow.
In `@dash-spv-ffi/tests/test_wallet_manager.rs`:
- Around line 62-66: The test currently creates a temporary CString inline, so
the pointer passed to dash_spv_ffi_config_builder_set_storage_path becomes
dangling when the CString is dropped; fix by creating a persistent CString
variable (e.g., storage_cstring) from temp_dir.path().to_str() and convert it to
a raw pointer with CString::into_raw(), pass that raw pointer to
dash_spv_ffi_config_builder_set_storage_path, and then later reclaim and free it
with CString::from_raw() after the config builder no longer needs the storage
path to avoid leaking memory.
- Around line 24-28: The pointer passed to
dash_spv_ffi_config_builder_set_storage_path is from a temporary CString that
gets dropped immediately (use-after-free); fix by creating a CString variable
that lives across the FFI call (e.g. let storage_c =
CString::new(temp_dir.path().to_str().unwrap()).unwrap(); then call
dash_spv_ffi_config_builder_set_storage_path(builder, storage_c.as_ptr());) so
the memory remains valid for the call (do not drop or let the CString go out of
scope until after the FFI call finishes; if you use into_raw(), ensure you later
reclaim with CString::from_raw()).
In `@dash-spv/tests/edge_case_filter_sync_test.rs`:
- Around line 157-165: ConfigBuilder::mainnet() currently calls
TempDir::new().unwrap().path() which drops the TempDir immediately and
invalidates the path; instead create a TempDir variable (e.g., temp_dir) and
pass temp_dir.path() into ConfigBuilder::storage_path so the directory's
lifetime is maintained for the duration of the test (ensuring the TempDir lives
at least until DiskStorageManager::new and any subsequent use of storage and
FilterSyncManager::new with received_heights).
- Around line 112-120: ConfigBuilder::mainnet currently calls
TempDir::new().unwrap().path() inline which drops the TempDir immediately and
can delete the temp directory before DiskStorageManager::new(&config) uses it;
fix by creating and binding the TempDir to a local variable (e.g., let tmp_dir =
TempDir::new().unwrap()), pass tmp_dir.path() into ConfigBuilder::storage_path
so the TempDir lives at least until after DiskStorageManager::new(&config)
completes, ensuring the temporary directory remains valid while
FilterSyncManager::new and DiskStorageManager::new are executed.
In `@dash-spv/tests/filter_header_verification_test.rs`:
- Around line 178-183: The TempDir returned by TempDir::new() is dropped
immediately because only its path() is passed into
ConfigBuilder::mainnet().storage_path(...), so the temporary directory is
deleted while DiskStorageManager::new(...) still uses it; fix by binding the
TempDir to a local variable (e.g., let tmp_dir = TempDir::new().unwrap();) and
pass tmp_dir.path() into ConfigBuilder::storage_path so tmp_dir lives for the
duration of the test (apply the same change where TempDir::new().unwrap().path()
is used around ConfigBuilder::mainnet()/storage_path and
DiskStorageManager::new()).
In `@dash-spv/tests/header_sync_test.rs`:
- Around line 26-30: The temporary directory created for storage_path is dropped
immediately because TempDir is constructed inline; bind the TempDir to a local
variable (e.g., let tmp_dir = TempDir::new().expect(...)) so it lives for the
duration of the test and then pass tmp_dir.path() into
ConfigBuilder::mainnet().validation_mode(ValidationMode::Basic).storage_path(...).build()
to ensure the directory remains available while the test runs.
In `@dash-spv/tests/peer_test.rs`:
- Around line 37-49: create_test_config now returns the TempDir alongside the
config and the test currently discards it causing the temp directory to be
dropped; update the test to destructure the tuple (e.g. let (config, _tmp_dir) =
create_test_config(Network::Testnet);) and keep the _tmp_dir variable in scope
for the lifetime of the test before calling PeerNetworkManager::new,
DiskStorageManager::new, WalletManager::new, and DashSpvClient::new so the
TempDir is not deleted prematurely.
- Around line 143-160: create_test_config currently drops its TempDir before the
network/storage/wallet/client are constructed, causing lifetime issues—keep the
TempDir alive by creating and binding the TempDir to a local variable that
remains in scope for the duration of the test (or change create_test_config to
accept/own an external TempDir), then construct config from that; also update
the add_peer call to match the new signature of add_peer (use the current
expected type—e.g., PeerAddr/SocketAddr/from_str or the Result-returning
variant—and call it accordingly) when preparing the config before calling
PeerNetworkManager::new, DiskStorageManager::new, WalletManager::new, and
DashSpvClient::new so all objects are built while the TempDir is still valid and
add_peer uses the correct type/signature.
- Around line 79-96: The TempDir used by create_test_config is being dropped
before the second scope, breaking peer persistence; ensure the temporary
directory lives for the whole test by creating and binding it to a
non-underscore variable before both client scopes (so it isn't dropped) and pass
the config that uses that TempDir into PeerNetworkManager::new,
DiskStorageManager::new and DashSpvClient::new; in short, move TempDir creation
out of the inner scope, store it in a stable variable (e.g., temp_dir) that
remains in scope for the test so peers persist across client restarts.
- Around line 20-30: The create_test_config helper currently drops the TempDir
immediately because it only passes TempDir::new().unwrap().path() into
ConfigBuilder; keep the temp directory alive by changing create_test_config to
return (Config, TempDir) instead of just Config so the TempDir is owned by the
caller; update the implementation that constructs Config via ConfigBuilder (use
a TempDir variable, pass temp_dir.path() to storage_path(), build the Config,
then return (config, temp_dir)), and update all call sites to destructure the
tuple (e.g., let (config, _temp_dir) = create_test_config(Network::Testnet)) so
the temporary directory is not deleted during test execution.
- Around line 179-195: The test suffers the TempDir lifetime bug: the temporary
directory created by create_test_config is dropped too early causing subsequent
uses (PeerNetworkManager::new, DiskStorageManager::new, WalletManager::new,
DashSpvClient::new) to see invalid paths. Fix by changing create_test_config to
return the TempDir alongside the Config (e.g. -> (Config, TempDir)) and update
the test to bind both (let (mut config, _tmpdir) =
create_test_config(Network::Testnet);), keeping _tmpdir in scope for the
duration of the test, then proceed to call config.add_peer,
PeerNetworkManager::new, DiskStorageManager::new, WalletManager::new, and
DashSpvClient::new as before.
In `@dash-spv/tests/wallet_integration_test.rs`:
- Around line 18-23: The test currently calls TempDir::new().unwrap().path()
inline when building the Config (via
ConfigBuilder::testnet().storage_path(...)), which drops the TempDir immediately
and deletes the directory before DiskStorageManager::new(&config) runs; fix by
creating and binding the TempDir to a local variable (e.g. let temp_dir =
TempDir::new().unwrap()), pass temp_dir.path() into
ConfigBuilder::storage_path(...) and keep temp_dir in scope for the lifetime of
the test so the storage path remains valid for DiskStorageManager::new(&config)
and subsequent test operations.
🟡 Minor comments (11)
dash-spv-ffi/tests/unit/test_memory_management.rs-73-78 (1)
73-78: Assert builder API results before using the config.
dash_spv_ffi_config_builder_set_storage_pathreturns a status, anddash_spv_ffi_config_builder_buildcan return null on failure. Please assert success to avoid silently running with an invalid or default config.💡 Suggested checks (apply in both places)
- let builder = dash_spv_ffi_config_builder_regtest(); + let builder = dash_spv_ffi_config_builder_regtest(); + assert!(!builder.is_null()); let path = CString::new(temp_dir.path().to_str().unwrap()).unwrap(); - dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()); - let config = dash_spv_ffi_config_builder_build(builder); + let rc = dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()); + assert_eq!(rc, FFIErrorCode::Success as i32); + let config = dash_spv_ffi_config_builder_build(builder); + assert!(!config.is_null());Also applies to: 270-275
dash-spv-ffi/README.md-83-84 (1)
83-84: Update documentation parameter names to reflect actual function signatures.The functions take a
builderhandle, not aconfighandle. The current documentation showsconfigas the parameter name, which contradicts the actual function signatures and is misleading.Documentation update
-- `dash_spv_ffi_config_builder_set_validation_mode(config, mode)` - Set validation mode -- `dash_spv_ffi_config_builder_set_max_peers(config, max)` - Set maximum peers +- `dash_spv_ffi_config_builder_set_validation_mode(builder, mode)` - Set validation mode +- `dash_spv_ffi_config_builder_set_max_peers(builder, max)` - Set maximum peersdash-spv/benches/storage.rs-37-41 (1)
37-41: KeepTempDiralive for the storage lifetime to prevent temp directory orphaning.
TempDir::new().unwrap().path()creates a temporary directory that's dropped immediately afterconfig.build(), causing the directory to be deleted.DiskStorageManager::new()then recreates it viafs::create_dir_all(), but without a cleanup handle, the directory persists and accumulates across iterations. StoreTempDiras a variable throughout the storage lifecycle.🔧 Suggested fix (keep TempDir alive during use)
- || async { - let config = ConfigBuilder::testnet() - .storage_path(TempDir::new().unwrap().path()) - .build() - .expect("Valid config"); - DiskStorageManager::new(&config).await.unwrap() - }, - |a| async { - let mut storage = a.await; + || async { + let temp_dir = TempDir::new().unwrap(); + let config = ConfigBuilder::testnet() + .storage_path(temp_dir.path()) + .build() + .expect("Valid config"); + let storage = DiskStorageManager::new(&config).await.unwrap(); + (storage, temp_dir) + }, + |a| async { + let (mut storage, _temp_dir) = a.await; for chunk in headers.chunks(CHUNK_SIZE as usize) { storage.store_headers(chunk).await.unwrap(); } }, BatchSize::SmallInput, ) }); - let config = ConfigBuilder::testnet() - .storage_path(TempDir::new().unwrap().path()) - .build() - .expect("Valid config"); + let temp_dir = TempDir::new().unwrap(); + let config = ConfigBuilder::testnet() + .storage_path(temp_dir.path()) + .build() + .expect("Valid config");dash-spv/tests/block_download_test.rs-118-124 (1)
118-124: KeepTempDiralive to prevent premature directory cleanup.
TempDir::new().unwrap().path()creates a temporary directory, extracts its path (which is copied into aPathBuf), and then theTempDiris immediately dropped. This deletes the actual directory before tests run, causing storage operations to fail silently or use wrong paths.Correct pattern (seen in
header_sync_test.rsline 92–94): Store theTempDirin a variable that remains in scope during the test. Updatecreate_test_config()to return(Config, TempDir)and modify all call sites to keep_temp_dirin scope.♻️ Suggested fix
-fn create_test_config() -> Config { - ConfigBuilder::testnet() - .enable_masternodes(false) - .validation_mode(dash_spv::types::ValidationMode::None) - .storage_path(TempDir::new().unwrap().path()) - .build() - .expect("Valid config") -} +fn create_test_config() -> (Config, TempDir) { + let temp_dir = TempDir::new().unwrap(); + let config = ConfigBuilder::testnet() + .enable_masternodes(false) + .validation_mode(dash_spv::types::ValidationMode::None) + .storage_path(temp_dir.path()) + .build() + .expect("Valid config"); + (config, temp_dir) +}Update call sites:
let (config, _temp_dir) = create_test_config();dash-spv-ffi/tests/test_client.rs-35-44 (1)
35-44: Assert builder API success in tests.Builder creation and setters are unchecked in both helpers. If any call fails, the test can proceed with a null config. Add asserts to fail fast and explicitly cover the new builder FFI APIs. Based on learnings, this strengthens unit coverage for new FFI functions.
🔧 Proposed fix
- let builder = dash_spv_ffi_config_builder_regtest(); + let builder = dash_spv_ffi_config_builder_regtest(); + assert!(!builder.is_null(), "Failed to create config builder"); let config = unsafe { let path = CString::new(temp_dir.path().to_str().unwrap()).unwrap(); - dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()); - dash_spv_ffi_config_builder_set_validation_mode(builder, FFIValidationMode::None); - dash_spv_ffi_config_builder_build(builder) + assert_eq!( + dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()), + FFIErrorCode::Success as i32 + ); + assert_eq!( + dash_spv_ffi_config_builder_set_validation_mode(builder, FFIValidationMode::None), + FFIErrorCode::Success as i32 + ); + let config = dash_spv_ffi_config_builder_build(builder); + assert!(!config.is_null(), "Failed to build config"); + config }; @@ - let builder = dash_spv_ffi_config_builder_testnet(); + let builder = dash_spv_ffi_config_builder_testnet(); + assert!(!builder.is_null(), "Failed to create config builder"); let temp_dir = TempDir::new().unwrap(); let path = CString::new(temp_dir.path().to_str().unwrap()).unwrap(); - dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()); - let config = dash_spv_ffi_config_builder_build(builder); + assert_eq!( + dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()), + FFIErrorCode::Success as i32 + ); + let config = dash_spv_ffi_config_builder_build(builder); + assert!(!config.is_null(), "Failed to build config");Also applies to: 158-163
dash-spv-ffi/tests/unit/test_async_operations.rs-61-72 (1)
61-72: Validate builder return codes and build result before use.Line 68/69 ignore the FFI error codes and Line 71 doesn’t check for a null config. If the builder rejects the path or build fails, the test can proceed with an invalid pointer and obscure the failure. Adding assertions also gives explicit coverage for the new builder FFI functions. Based on learnings, please add explicit unit-level validation for new FFI calls.
🔧 Proposed fix
- dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()); - dash_spv_ffi_config_builder_set_validation_mode(builder, FFIValidationMode::None); + assert_eq!( + dash_spv_ffi_config_builder_set_storage_path(builder, path.as_ptr()), + FFIErrorCode::Success as i32, + "Failed to set storage path" + ); + assert_eq!( + dash_spv_ffi_config_builder_set_validation_mode(builder, FFIValidationMode::None), + FFIErrorCode::Success as i32, + "Failed to set validation mode" + ); - let config = dash_spv_ffi_config_builder_build(builder); + let config = dash_spv_ffi_config_builder_build(builder); + assert!(!config.is_null(), "Failed to build config");dash-spv-ffi/examples/basic_usage.c-13-27 (1)
13-27: Fix builder pointer type and handle build failure.Line 13 declares the builder as
FFIConfig*, butdash_spv_ffi_config_builder_testnet()returnsFFIConfigBuilder*. These are distinct types, causing an incompatible pointer type error. Additionally,dash_spv_ffi_config_builder_build()can returnNULL, which is never checked before passingconfigtodash_spv_ffi_client_new()at line 29.🔧 Proposed fix
- FFIConfig* builder = dash_spv_ffi_config_builder_testnet(); + FFIConfigBuilder* builder = dash_spv_ffi_config_builder_testnet(); if (builder == NULL) { fprintf(stderr, "Failed to create config builder\n"); return 1; } @@ -25,6 +25,11 @@ int main() { } FFIConfig* config = dash_spv_ffi_config_builder_build(builder); + if (config == NULL) { + const char* error = dash_spv_ffi_get_last_error(); + fprintf(stderr, "Failed to build config: %s\n", error ? error : "unknown"); + return 1; + }dash-spv-ffi/include/dash_spv_ffi.h-826-834 (1)
826-834: Documentation error:dash_spv_ffi_config_destroyrefers to "builder" instead of "config".The safety documentation incorrectly states:
buildermust be a valid pointer to an FFIConfigBuilder, or nullThis should reference
configandFFIConfig:* # Safety -- `builder` must be a valid pointer to an FFIConfigBuilder, or null +- `config` must be a valid pointer to an FFIConfig, or null * - After calling this function, the config pointer becomes invalid and must not be used * - This function should only be called once per config instancedash-spv-ffi/tests/unit/test_configuration.rs-289-317 (1)
289-317: Test doesn't verify build failure for invalid max_peers(0).Line 316 calls
build()without checking the return value. Since max_peers(0) is rejected byConfigBuilder::validate()(line 148-150 in config.rs),build()will return null. The test should either:
- Verify that
build()returns null:assert!(config.is_null())- Or remove the invalid
max_peers(0)setting and only test valid edge cases (1 and u32::MAX)Currently, the test passes only because
destroy()safely handles null pointers—the actual validation failure is silently ignored.dash-spv-ffi/src/config.rs-543-559 (1)
543-559: Documentation mismatch: Safety comment references wrong parameter name.The doc comment on lines 546-547 mentions
builderbut the function takesconfig: *mut FFIConfig. This is confusing for FFI consumers.Suggested fix
/// Destroys an FFIConfig and frees its memory /// /// # Safety -/// - `builder` must be a valid pointer to an FFIConfigBuilder, or null +/// - `config` must be a valid pointer to an FFIConfig, or null /// - After calling this function, the config pointer becomes invalid and must not be used /// - This function should only be called once per config instancedash-spv-ffi/FFI_API.md-570-580 (1)
570-580: Documentation mismatch: Safety section references wrong parameter.Lines 577 and 580 mention
builderbut the function signature takesconfig: *mut FFIConfig. This propagates the same error from the source code docstring.Suggested fix
**Description:** -Destroys an FFIConfig and frees its memory # Safety - `builder` must be a valid pointer to an FFIConfigBuilder, or null - After calling this function, the config pointer becomes invalid and must not be used - This function should only be called once per config instance +Destroys an FFIConfig and frees its memory # Safety - `config` must be a valid pointer to an FFIConfig, or null - After calling this function, the config pointer becomes invalid and must not be used - This function should only be called once per config instance **Safety:** --`builder` must be a valid pointer to an FFIConfigBuilder, or null - After calling this function, the config pointer becomes invalid and must not be used - This function should only be called once per config instance +-`config` must be a valid pointer to an FFIConfig, or null - After calling this function, the config pointer becomes invalid and must not be used - This function should only be called once per config instance
🧹 Nitpick comments (18)
dash-spv/tests/chainlock_simple_test.rs (1)
35-41: Keep builder settings in sync with local flags.
enable_masternodesis defined but the builder hard-codestrue, which can drift if the flag changes later. Use the variable to keep the test consistent.♻️ Proposed fix
- let config = ConfigBuilder::mainnet() - .enable_filters(false) - .enable_masternodes(true) + let config = ConfigBuilder::mainnet() + .enable_filters(false) + .enable_masternodes(enable_masternodes)dash-spv-ffi/src/client.rs (1)
402-411: Outdated safety comment references old type name.The safety comment on line 406 still mentions
FFIClientConfig, but the function now usesFFIConfig. Update the documentation for consistency.📝 Suggested documentation fix
/// Update the running client's configuration. /// /// # Safety /// - `client` must be a valid pointer to an `FFIDashSpvClient`. -/// - `config` must be a valid pointer to an `FFIClientConfig`. +/// - `config` must be a valid pointer to an `FFIConfig`. /// - The network in `config` must match the client's network; changing networks at runtime is not supported. #[no_mangle] pub unsafe extern "C" fn dash_spv_ffi_client_update_config( client: *mut FFIDashSpvClient, config: *const FFIConfig, ) -> i32 {dash-spv-ffi/tests/c_tests/test_basic.c (1)
96-107: Naming inconsistency between setter and getter.The test sets the path via
dash_spv_ffi_config_set_data_dir(line 96) but retrieves it viadash_spv_ffi_config_get_storage_path(line 103). While this may work if both refer to the same underlying field, the inconsistent naming (data_dirvsstorage_path) is confusing.Consider renaming
dash_spv_ffi_config_set_data_dirtodash_spv_ffi_config_set_storage_pathfor consistency with the getter, or document that they are aliases.dash-spv-ffi/dash_spv_ffi.h (1)
693-701: Documentation inconsistency with function name.The doc comment says "Gets the data directory path" but the function is named
dash_spv_ffi_config_get_storage_path. Consider updating the comment to match the new naming:-/** - * Gets the data directory path from the configuration +/** + * Gets the storage path from the configurationdash-spv/src/main.rs (1)
252-263: Clarify variable naming:configis actually the builder untilbuild()is called.The variable
configholds a&mut ConfigBuilderreference from line 252 untilconfig.build()on line 281. While this works, the nameconfigis misleading since it's still a builder at this point. Consider renaming for clarity:- let config = cfg_builder.storage_path(data_dir.clone()).validation_mode(validation_mode); + let builder = cfg_builder.storage_path(data_dir.clone()).validation_mode(validation_mode);And adjust subsequent lines to use
builderuntilbuild()returns the actualConfig.dash-spv/src/client/config.rs (1)
147-175: Validation creates directories as a side effect - consider deferring.The validation function at line 161-162 calls
std::fs::create_dir_all(path)which has side effects during thebuild()call. This means even a failed build attempt may create directories on disk.Consider either:
- Moving directory creation to a separate initialization step after successful build
- Only validating the path is writable without creating directories
Also, the comment on line 152 is in Spanish ("Validación de mempool") - should be in English per codebase conventions.
Suggested changes
- // Validación de mempool + // Mempool validationdash-spv-ffi/include/dash_spv_ffi.h (1)
64-67: Consider documenting theworker_threadsfield duplication.Both
FFIConfigandFFIConfigBuilderhave aworker_threads: uint32_tfield alongside theinner: void*pointer. This suggestsworker_threadsis tracked at the FFI layer separately from the inner Rust config/builder.While the implementation in
dash-spv-ffi/src/config.rsshows this is intentional for managing the tokio runtime, adding a brief comment explaining why this field exists at the FFI struct level would improve maintainability.Also applies to: 173-176
dash-spv-ffi/src/config.rs (11)
88-106: Redundantunsafeblock inside already-unsafe function.Line 95 has an unnecessary nested
unsafeblock since the function is already markedunsafe extern "C".Suggested fix
- let builder = unsafe { &mut *((*builder).inner as *mut ConfigBuilder) }; + let builder = &mut *((*builder).inner as *mut ConfigBuilder);
114-123: Redundantunsafeblock.Same issue on line 120 - unnecessary nested
unsafe.Suggested fix
- let config = unsafe { &mut *((*builder).inner as *mut ConfigBuilder) }; + let config = &mut *((*builder).inner as *mut ConfigBuilder);
131-140: Redundantunsafeblock.Line 137 has the same pattern of unnecessary nested
unsafe.
149-169: Redundantunsafeblock.Line 160 has the same issue.
177-186: Redundantunsafeblock and unused variable.Line 183: redundant
unsafe. Also,_builderis retrieved but never used. Consider removing the dead code or adding a TODO explaining why it's kept.Suggested fix
pub unsafe extern "C" fn dash_spv_ffi_config_builder_set_relay_transactions( builder: *mut FFIConfigBuilder, _relay: bool, ) -> i32 { null_check!(builder); - - let _builder = unsafe { &mut *((*builder).inner as *mut ConfigBuilder) }; // relay_transactions not directly settable in current ClientConfig FFIErrorCode::Success as i32 }
194-203: Multiple functions have redundantunsafeblocks.Lines 200, 216, 233, 265, 282, 299, 316, 333 all have the same pattern of unnecessary nested
unsafeinside already-unsafe functions. Consider a batch cleanup.Also applies to: 210-219, 227-236, 243-251, 259-268, 276-285, 293-302, 310-319, 327-336
344-353: Redundantunsafeblock.Line 350 has the same pattern.
479-486: Redundantunsafeblock.Line 484 has unnecessary nested
unsafe.
495-507: Redundantunsafeblock.Line 505 has the same issue.
515-524: Redundantunsafeblock.Line 522 has the same pattern.
532-541: Redundantunsafeblock.Line 539 has the same issue.
| let result = dash_spv_ffi_config_builder_set_max_peers(config, 10); | ||
| assert_ne!(result, FFIErrorCode::Success as i32); |
There was a problem hiding this comment.
Builder setters are called with a config pointer (ABI mismatch).
dash_spv_ffi_config_builder_set_max_peers expects a FFIConfigBuilder*, but config is created via dash_spv_ffi_config_new. This is undefined behavior and may crash. Please use a builder to set max peers before building the config.
🐛 Proposed fix (builder-based max_peers)
- let result = dash_spv_ffi_config_builder_set_max_peers(config, 10);
- assert_ne!(result, FFIErrorCode::Success as i32);
+ // Builder-based config: set before build
+ let builder = dash_spv_ffi_config_builder_regtest();
+ let result = dash_spv_ffi_config_builder_set_max_peers(builder, 10);
+ assert_eq!(result, FFIErrorCode::Success as i32);
+ let config = dash_spv_ffi_config_builder_build(builder);
+ assert!(!config.is_null());
+ // ... use config, then destroy
+ dash_spv_ffi_config_destroy(config);- let result = dash_spv_ffi_config_builder_set_max_peers(config, u32::MAX);
- assert_eq!(result, FFIErrorCode::Success as i32);
+ let builder = dash_spv_ffi_config_builder_regtest();
+ let result = dash_spv_ffi_config_builder_set_max_peers(builder, u32::MAX);
+ assert_eq!(result, FFIErrorCode::Success as i32);
+ let config = dash_spv_ffi_config_builder_build(builder);
+ assert!(!config.is_null());Also applies to: 109-110
🤖 Prompt for AI Agents
In `@dash-spv-ffi/tests/security/test_security.rs` around lines 96 - 97, The test
calls dash_spv_ffi_config_builder_set_max_peers with a config pointer created by
dash_spv_ffi_config_new (ABI mismatch); to fix, allocate and use a builder via
dash_spv_ffi_config_builder_new, call
dash_spv_ffi_config_builder_set_max_peers(builder, 10), then build the config
with dash_spv_ffi_config_builder_build (or equivalent) and assert results on the
build result or the produced config; update the assertions that currently use
dash_spv_ffi_config_new (also at the other occurrence) to follow this builder ->
set_max_peers -> build flow.
| let temp_dir = TempDir::new().unwrap(); | ||
| dash_spv_ffi_config_builder_set_storage_path( | ||
| builder, | ||
| CString::new(temp_dir.path().to_str().unwrap()).unwrap().as_ptr(), | ||
| ); |
There was a problem hiding this comment.
Use-after-free: CString is dropped before pointer is used.
CString::new(...).unwrap().as_ptr() creates a temporary CString that is immediately dropped, leaving a dangling pointer passed to the FFI function.
Proposed fix
let temp_dir = TempDir::new().unwrap();
+ let storage_path = CString::new(temp_dir.path().to_str().unwrap()).unwrap();
dash_spv_ffi_config_builder_set_storage_path(
builder,
- CString::new(temp_dir.path().to_str().unwrap()).unwrap().as_ptr(),
+ storage_path.as_ptr(),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let temp_dir = TempDir::new().unwrap(); | |
| dash_spv_ffi_config_builder_set_storage_path( | |
| builder, | |
| CString::new(temp_dir.path().to_str().unwrap()).unwrap().as_ptr(), | |
| ); | |
| let temp_dir = TempDir::new().unwrap(); | |
| let storage_path = CString::new(temp_dir.path().to_str().unwrap()).unwrap(); | |
| dash_spv_ffi_config_builder_set_storage_path( | |
| builder, | |
| storage_path.as_ptr(), | |
| ); |
🤖 Prompt for AI Agents
In `@dash-spv-ffi/tests/test_wallet_manager.rs` around lines 24 - 28, The pointer
passed to dash_spv_ffi_config_builder_set_storage_path is from a temporary
CString that gets dropped immediately (use-after-free); fix by creating a
CString variable that lives across the FFI call (e.g. let storage_c =
CString::new(temp_dir.path().to_str().unwrap()).unwrap(); then call
dash_spv_ffi_config_builder_set_storage_path(builder, storage_c.as_ptr());) so
the memory remains valid for the call (do not drop or let the CString go out of
scope until after the FFI call finishes; if you use into_raw(), ensure you later
reclaim with CString::from_raw()).
| let temp_dir = TempDir::new().unwrap(); | ||
| dash_spv_ffi_config_builder_set_storage_path( | ||
| builder, | ||
| CString::new(temp_dir.path().to_str().unwrap()).unwrap().as_ptr(), | ||
| ); |
There was a problem hiding this comment.
Same CString lifetime issue as above.
The temporary CString is dropped immediately, resulting in a dangling pointer.
Proposed fix
let temp_dir = TempDir::new().unwrap();
+ let storage_path = CString::new(temp_dir.path().to_str().unwrap()).unwrap();
dash_spv_ffi_config_builder_set_storage_path(
builder,
- CString::new(temp_dir.path().to_str().unwrap()).unwrap().as_ptr(),
+ storage_path.as_ptr(),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let temp_dir = TempDir::new().unwrap(); | |
| dash_spv_ffi_config_builder_set_storage_path( | |
| builder, | |
| CString::new(temp_dir.path().to_str().unwrap()).unwrap().as_ptr(), | |
| ); | |
| let temp_dir = TempDir::new().unwrap(); | |
| let storage_path = CString::new(temp_dir.path().to_str().unwrap()).unwrap(); | |
| dash_spv_ffi_config_builder_set_storage_path( | |
| builder, | |
| storage_path.as_ptr(), | |
| ); |
🤖 Prompt for AI Agents
In `@dash-spv-ffi/tests/test_wallet_manager.rs` around lines 62 - 66, The test
currently creates a temporary CString inline, so the pointer passed to
dash_spv_ffi_config_builder_set_storage_path becomes dangling when the CString
is dropped; fix by creating a persistent CString variable (e.g.,
storage_cstring) from temp_dir.path().to_str() and convert it to a raw pointer
with CString::into_raw(), pass that raw pointer to
dash_spv_ffi_config_builder_set_storage_path, and then later reclaim and free it
with CString::from_raw() after the config builder no longer needs the storage
path to avoid leaking memory.
| let config = ConfigBuilder::mainnet() | ||
| .storage_path(TempDir::new().unwrap().path()) | ||
| .build() | ||
| .expect("Valid config"); | ||
|
|
||
| let received_heights = Arc::new(Mutex::new(HashSet::new())); | ||
| let mut filter_sync: FilterSyncManager<DiskStorageManager, MockNetworkManager> = | ||
| FilterSyncManager::new(&config, received_heights); | ||
| let mut filter_sync = FilterSyncManager::new(&config, received_heights); | ||
|
|
||
| let mut storage = DiskStorageManager::new(TempDir::new().unwrap().path().to_path_buf()) | ||
| .await | ||
| .expect("Failed to create tmp storage"); | ||
| let mut storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); |
There was a problem hiding this comment.
Potential use-after-free: TempDir drops before config is used.
The TempDir is created inline and immediately dropped after .path() is called. This means the temporary directory may be deleted before DiskStorageManager::new(&config) attempts to use it, causing test failures or undefined behavior.
Proposed fix
+ let temp_dir = TempDir::new().unwrap();
let config = ConfigBuilder::mainnet()
- .storage_path(TempDir::new().unwrap().path())
+ .storage_path(temp_dir.path())
.build()
.expect("Valid config");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let config = ConfigBuilder::mainnet() | |
| .storage_path(TempDir::new().unwrap().path()) | |
| .build() | |
| .expect("Valid config"); | |
| let received_heights = Arc::new(Mutex::new(HashSet::new())); | |
| let mut filter_sync: FilterSyncManager<DiskStorageManager, MockNetworkManager> = | |
| FilterSyncManager::new(&config, received_heights); | |
| let mut filter_sync = FilterSyncManager::new(&config, received_heights); | |
| let mut storage = DiskStorageManager::new(TempDir::new().unwrap().path().to_path_buf()) | |
| .await | |
| .expect("Failed to create tmp storage"); | |
| let mut storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); | |
| let temp_dir = TempDir::new().unwrap(); | |
| let config = ConfigBuilder::mainnet() | |
| .storage_path(temp_dir.path()) | |
| .build() | |
| .expect("Valid config"); | |
| let received_heights = Arc::new(Mutex::new(HashSet::new())); | |
| let mut filter_sync = FilterSyncManager::new(&config, received_heights); | |
| let mut storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); |
🤖 Prompt for AI Agents
In `@dash-spv/tests/edge_case_filter_sync_test.rs` around lines 112 - 120,
ConfigBuilder::mainnet currently calls TempDir::new().unwrap().path() inline
which drops the TempDir immediately and can delete the temp directory before
DiskStorageManager::new(&config) uses it; fix by creating and binding the
TempDir to a local variable (e.g., let tmp_dir = TempDir::new().unwrap()), pass
tmp_dir.path() into ConfigBuilder::storage_path so the TempDir lives at least
until after DiskStorageManager::new(&config) completes, ensuring the temporary
directory remains valid while FilterSyncManager::new and DiskStorageManager::new
are executed.
| let config = ConfigBuilder::mainnet() | ||
| .storage_path(TempDir::new().unwrap().path()) | ||
| .build() | ||
| .expect("Valid config"); | ||
|
|
||
| let received_heights = Arc::new(Mutex::new(HashSet::new())); | ||
| let mut filter_sync: FilterSyncManager<DiskStorageManager, MockNetworkManager> = | ||
| FilterSyncManager::new(&config, received_heights); | ||
| let mut filter_sync = FilterSyncManager::new(&config, received_heights); | ||
|
|
||
| let mut storage = DiskStorageManager::new(TempDir::new().unwrap().path().to_path_buf()) | ||
| .await | ||
| .expect("Failed to create tmp storage"); | ||
| let mut storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); |
There was a problem hiding this comment.
Same TempDir lifetime issue as above.
The temporary directory is dropped immediately after .path() is called, potentially invalidating the storage path before use.
Proposed fix
+ let temp_dir = TempDir::new().unwrap();
let config = ConfigBuilder::mainnet()
- .storage_path(TempDir::new().unwrap().path())
+ .storage_path(temp_dir.path())
.build()
.expect("Valid config");🤖 Prompt for AI Agents
In `@dash-spv/tests/edge_case_filter_sync_test.rs` around lines 157 - 165,
ConfigBuilder::mainnet() currently calls TempDir::new().unwrap().path() which
drops the TempDir immediately and invalidates the path; instead create a TempDir
variable (e.g., temp_dir) and pass temp_dir.path() into
ConfigBuilder::storage_path so the directory's lifetime is maintained for the
duration of the test (ensuring the TempDir lives at least until
DiskStorageManager::new and any subsequent use of storage and
FilterSyncManager::new with received_heights).
| let config = create_test_config(Network::Testnet); | ||
|
|
||
| // Create network manager | ||
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create storage manager | ||
| let storage_manager = DiskStorageManager::new(temp_path).await.unwrap(); | ||
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create wallet manager | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | ||
|
|
||
| let mut client = | ||
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); |
There was a problem hiding this comment.
Update to use fixed create_test_config that preserves TempDir.
After fixing the helper function to return the TempDir, this test should destructure the result to keep the directory alive.
🐛 Proposed fix for test_peer_connection
- let config = create_test_config(Network::Testnet);
+ let (config, _temp_dir) = create_test_config(Network::Testnet);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let config = create_test_config(Network::Testnet); | |
| // Create network manager | |
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | |
| // Create storage manager | |
| let storage_manager = DiskStorageManager::new(temp_path).await.unwrap(); | |
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | |
| // Create wallet manager | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | |
| let mut client = | |
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); | |
| let (config, _temp_dir) = create_test_config(Network::Testnet); | |
| // Create network manager | |
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | |
| // Create storage manager | |
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | |
| // Create wallet manager | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | |
| let mut client = | |
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); |
🤖 Prompt for AI Agents
In `@dash-spv/tests/peer_test.rs` around lines 37 - 49, create_test_config now
returns the TempDir alongside the config and the test currently discards it
causing the temp directory to be dropped; update the test to destructure the
tuple (e.g. let (config, _tmp_dir) = create_test_config(Network::Testnet);) and
keep the _tmp_dir variable in scope for the lifetime of the test before calling
PeerNetworkManager::new, DiskStorageManager::new, WalletManager::new, and
DashSpvClient::new so the TempDir is not deleted prematurely.
| let config = create_test_config(Network::Testnet); | ||
|
|
||
| // First run: connect and save peers | ||
| { | ||
| let config = create_test_config(Network::Testnet, Some(temp_dir)); | ||
|
|
||
| // Create network manager | ||
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create storage manager | ||
| let storage_manager = DiskStorageManager::new(temp_path.clone()).await.unwrap(); | ||
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create wallet manager | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | ||
| let wallet = | ||
| Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | ||
|
|
||
| let mut client = | ||
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); | ||
| DashSpvClient::new(config.clone(), network_manager, storage_manager, wallet) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Same TempDir lifetime issue as above.
The _temp_dir needs to be held for both scopes in this test since it tests peer persistence across client restarts.
🐛 Proposed fix for test_peer_persistence
- let config = create_test_config(Network::Testnet);
+ let (config, _temp_dir) = create_test_config(Network::Testnet);🤖 Prompt for AI Agents
In `@dash-spv/tests/peer_test.rs` around lines 79 - 96, The TempDir used by
create_test_config is being dropped before the second scope, breaking peer
persistence; ensure the temporary directory lives for the whole test by creating
and binding it to a non-underscore variable before both client scopes (so it
isn't dropped) and pass the config that uses that TempDir into
PeerNetworkManager::new, DiskStorageManager::new and DashSpvClient::new; in
short, move TempDir creation out of the inner scope, store it in a stable
variable (e.g., temp_dir) that remains in scope for the test so peers persist
across client restarts.
| let mut config = create_test_config(Network::Regtest); | ||
|
|
||
| // Add manual test peers (would need actual regtest nodes running) | ||
| config.peers = vec!["127.0.0.1:19899".parse().unwrap(), "127.0.0.1:19898".parse().unwrap()]; | ||
| config | ||
| .add_peer("127.0.0.1:19899".parse().unwrap()) | ||
| .add_peer("127.0.0.1:19898".parse().unwrap()); | ||
|
|
||
| // Create network manager | ||
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create storage manager | ||
| let storage_manager = DiskStorageManager::new(temp_path).await.unwrap(); | ||
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create wallet manager | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | ||
|
|
||
| let client = | ||
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); |
There was a problem hiding this comment.
Same TempDir lifetime issue and update needed for add_peer usage.
The fluent add_peer() API usage looks correct, but the TempDir issue needs to be addressed here as well.
🐛 Proposed fix for test_peer_disconnection
- let mut config = create_test_config(Network::Regtest);
+ let (mut config, _temp_dir) = create_test_config(Network::Regtest);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut config = create_test_config(Network::Regtest); | |
| // Add manual test peers (would need actual regtest nodes running) | |
| config.peers = vec!["127.0.0.1:19899".parse().unwrap(), "127.0.0.1:19898".parse().unwrap()]; | |
| config | |
| .add_peer("127.0.0.1:19899".parse().unwrap()) | |
| .add_peer("127.0.0.1:19898".parse().unwrap()); | |
| // Create network manager | |
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | |
| // Create storage manager | |
| let storage_manager = DiskStorageManager::new(temp_path).await.unwrap(); | |
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | |
| // Create wallet manager | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | |
| let client = | |
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); | |
| let (mut config, _temp_dir) = create_test_config(Network::Regtest); | |
| // Add manual test peers (would need actual regtest nodes running) | |
| config | |
| .add_peer("127.0.0.1:19899".parse().unwrap()) | |
| .add_peer("127.0.0.1:19898".parse().unwrap()); | |
| // Create network manager | |
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | |
| // Create storage manager | |
| let storage_manager = DiskStorageManager::new(&config).await.unwrap(); | |
| // Create wallet manager | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | |
| let client = | |
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); |
🤖 Prompt for AI Agents
In `@dash-spv/tests/peer_test.rs` around lines 143 - 160, create_test_config
currently drops its TempDir before the network/storage/wallet/client are
constructed, causing lifetime issues—keep the TempDir alive by creating and
binding the TempDir to a local variable that remains in scope for the duration
of the test (or change create_test_config to accept/own an external TempDir),
then construct config from that; also update the add_peer call to match the new
signature of add_peer (use the current expected type—e.g.,
PeerAddr/SocketAddr/from_str or the Result-returning variant—and call it
accordingly) when preparing the config before calling PeerNetworkManager::new,
DiskStorageManager::new, WalletManager::new, and DashSpvClient::new so all
objects are built while the TempDir is still valid and add_peer uses the correct
type/signature.
| let mut config = create_test_config(Network::Testnet); | ||
|
|
||
| // Add at least one peer to avoid "No peers specified" error | ||
| config.peers = vec!["127.0.0.1:19999".parse().unwrap()]; | ||
| config.add_peer("127.0.0.1:19999".parse().unwrap()); | ||
|
|
||
| // Create network manager | ||
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | ||
|
|
||
| // Create storage manager | ||
| let storage_manager = | ||
| DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path()) | ||
| .await | ||
| .expect("Failed to create tmp storage"); | ||
| DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); | ||
|
|
||
| // Create wallet manager | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | ||
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | ||
|
|
||
| let _client = | ||
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); |
There was a problem hiding this comment.
Same TempDir lifetime issue.
🐛 Proposed fix for test_max_peer_limit
- let mut config = create_test_config(Network::Testnet);
+ let (mut config, _temp_dir) = create_test_config(Network::Testnet);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut config = create_test_config(Network::Testnet); | |
| // Add at least one peer to avoid "No peers specified" error | |
| config.peers = vec!["127.0.0.1:19999".parse().unwrap()]; | |
| config.add_peer("127.0.0.1:19999".parse().unwrap()); | |
| // Create network manager | |
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | |
| // Create storage manager | |
| let storage_manager = | |
| DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path()) | |
| .await | |
| .expect("Failed to create tmp storage"); | |
| DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); | |
| // Create wallet manager | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network))); | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | |
| let _client = | |
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); | |
| let (mut config, _temp_dir) = create_test_config(Network::Testnet); | |
| // Add at least one peer to avoid "No peers specified" error | |
| config.add_peer("127.0.0.1:19999".parse().unwrap()); | |
| // Create network manager | |
| let network_manager = PeerNetworkManager::new(&config).await.unwrap(); | |
| // Create storage manager | |
| let storage_manager = | |
| DiskStorageManager::new(&config).await.expect("Failed to create tmp storage"); | |
| // Create wallet manager | |
| let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network()))); | |
| let _client = | |
| DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); |
🤖 Prompt for AI Agents
In `@dash-spv/tests/peer_test.rs` around lines 179 - 195, The test suffers the
TempDir lifetime bug: the temporary directory created by create_test_config is
dropped too early causing subsequent uses (PeerNetworkManager::new,
DiskStorageManager::new, WalletManager::new, DashSpvClient::new) to see invalid
paths. Fix by changing create_test_config to return the TempDir alongside the
Config (e.g. -> (Config, TempDir)) and update the test to bind both (let (mut
config, _tmpdir) = create_test_config(Network::Testnet);), keeping _tmpdir in
scope for the duration of the test, then proceed to call config.add_peer,
PeerNetworkManager::new, DiskStorageManager::new, WalletManager::new, and
DashSpvClient::new as before.
| let config = ConfigBuilder::testnet() | ||
| .enable_filters(false) | ||
| .storage_path(TempDir::new().unwrap().path()) | ||
| .enable_masternodes(true) | ||
| .build() | ||
| .expect("Valid config"); |
There was a problem hiding this comment.
Critical: TempDir is dropped immediately, invalidating the storage path.
The TempDir::new().unwrap() creates a temporary directory that is immediately dropped after .path() is called. When a TempDir is dropped, it deletes the underlying directory. This means by the time DiskStorageManager::new(&config) runs, the storage path may no longer exist.
🐛 Proposed fix: retain `TempDir` for the test duration
async fn create_test_client(
-) -> DashSpvClient<WalletManager<ManagedWalletInfo>, PeerNetworkManager, DiskStorageManager> {
+) -> (DashSpvClient<WalletManager<ManagedWalletInfo>, PeerNetworkManager, DiskStorageManager>, TempDir) {
+ let temp_dir = TempDir::new().unwrap();
let config = ConfigBuilder::testnet()
.enable_filters(false)
- .storage_path(TempDir::new().unwrap().path())
+ .storage_path(temp_dir.path())
.enable_masternodes(true)
.build()
.expect("Valid config");
// ... rest of function ...
- DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap()
+ (DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(), temp_dir)
}Then update the test functions to retain the TempDir:
async fn test_spv_client_creation() {
- let client = create_test_client().await;
+ let (client, _temp_dir) = create_test_client().await;🤖 Prompt for AI Agents
In `@dash-spv/tests/wallet_integration_test.rs` around lines 18 - 23, The test
currently calls TempDir::new().unwrap().path() inline when building the Config
(via ConfigBuilder::testnet().storage_path(...)), which drops the TempDir
immediately and deletes the directory before DiskStorageManager::new(&config)
runs; fix by creating and binding the TempDir to a local variable (e.g. let
temp_dir = TempDir::new().unwrap()), pass temp_dir.path() into
ConfigBuilder::storage_path(...) and keep temp_dir in scope for the lifetime of
the test so the storage path remains valid for DiskStorageManager::new(&config)
and subsequent test operations.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.