refactor: dashcore exposes a FFI compatible Network enum so dependents don't have to make their own wrapper#642
Merged
xdustinface merged 1 commit intov0.42-devfrom Apr 16, 2026
Conversation
Contributor
|
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 52 minutes and 12 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 (55)
📝 WalkthroughWalkthroughThis change centralizes FFI network type support by moving Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #642 +/- ##
=============================================
- Coverage 67.99% 67.62% -0.37%
=============================================
Files 318 319 +1
Lines 67877 67894 +17
=============================================
- Hits 46153 45914 -239
- Misses 21724 21980 +256
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
dash-spv-ffi/FFI_API.md (1)
239-246:⚠️ Potential issue | 🟡 MinorUpdate safety documentation to match new type.
The function signature was correctly updated to return
DashcoreNetwork, but the safety documentation on line 246 still referencesFFINetwork::Mainnet. This should be updated toDashcoreNetwork::Mainnetfor consistency.📝 Proposed fix
**Safety:** -- `config` must be a valid pointer to an FFIClientConfig or null - If null, returns FFINetwork::Mainnet as default +- `config` must be a valid pointer to an FFIClientConfig or null - If null, returns DashcoreNetwork::Mainnet as default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/FFI_API.md` around lines 239 - 246, Update the safety documentation for dash_spv_ffi_config_get_network so the default network type matches the updated return type: replace the incorrect reference to FFINetwork::Mainnet with DashcoreNetwork::Mainnet in the Safety section and any other occurrences; verify the doc mentions the function name dash_spv_ffi_config_get_network and the pointer type FFIClientConfig remain correct.dash-spv-ffi/src/config.rs (1)
249-254:⚠️ Potential issue | 🟡 MinorUpdate stale doc type name in getter docs.
The safety/docs comment still references
FFINetwork::Mainnet; it should now referenceDashcoreNetwork::Mainnetto match the signature and behavior.🤖 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 249 - 254, Update the stale docs in the network getter comment: replace the incorrect type reference `FFINetwork::Mainnet` with `DashcoreNetwork::Mainnet` in the safety/docs block that describes the network getter (the comment starting "Gets the network type from the configuration" which mentions `FFIClientConfig`); ensure the comment matches the actual return/signature of the function so it references `DashcoreNetwork::Mainnet`.key-wallet-ffi/src/account.rs (1)
229-235:⚠️ Potential issue | 🟡 MinorRefresh outdated network type names in docs.
These comments still reference
FFINetwork::Mainnet; update them toDashcoreNetwork::Mainnetto keep the FFI docs accurate.Also applies to: 340-346, 457-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account.rs` around lines 229 - 235, Update the FFI doc comments that still mention the old enum name FFINetwork::Mainnet to use DashcoreNetwork::Mainnet instead; locate the doc blocks above the exported functions (the comment starting "Get the network of an account" and the other similar safety docs around the same file) and replace the outdated FFINetwork references with DashcoreNetwork (e.g., DashcoreNetwork::Mainnet) so all three occurrences (around the shown block and at the other noted comment ranges) reflect the correct type name.key-wallet-ffi/FFI_API.md (2)
4394-4397:⚠️ Potential issue | 🟠 MajorUsage example is no longer valid after signature change.
wallet_manager_createnow requires aDashcoreNetworkargument, but the example still calls it with onlyerror.Suggested doc fix
// Create wallet manager FFIError error = {0}; -FFIWalletManager* manager = wallet_manager_create(&error); +FFIWalletManager* manager = wallet_manager_create(DashcoreNetwork_Mainnet, &error); if (error.code != 0) { // Handle error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/FFI_API.md` around lines 4394 - 4397, Update the usage example to match the new wallet_manager_create signature by passing a DashcoreNetwork argument along with the FFIError pointer; locate the example where FFIError error = {0}; FFIWalletManager* manager = wallet_manager_create(&error); and change the call to include an appropriate DashcoreNetwork value (e.g., MAINNET/TESTNET enum) so the call becomes wallet_manager_create(<DashcoreNetwork>, &error) and subsequent error handling using error.code remains the same.
4378-4378:⚠️ Potential issue | 🟠 MajorStale type name in Type Definitions section.
FFINetworkis still listed, but the documented API now usesDashcoreNetwork. This will confuse consumers using the type reference section.Suggested doc fix
-- `FFINetwork` - Network enumeration +- `DashcoreNetwork` - Network enumeration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/FFI_API.md` at line 4378, Update the Type Definitions section to replace the stale type name FFINetwork with the current DashcoreNetwork everywhere it appears; search for the symbol FFINetwork in the docs (specifically the Type Definitions list and any type references) and change the label and any descriptive text to DashcoreNetwork so the documented API matches the implementation and avoids confusing consumers.
🧹 Nitpick comments (4)
key-wallet-ffi/src/wallet_manager_tests.rs (1)
867-868: Consider removing redundant import.The
use dashcore::ffi::DashcoreNetwork;on line 867 is redundant since line 8 already importsDashcoreNetworkat the module level. The same applies to lines 1067-1068.♻️ Proposed cleanup
#[cfg(feature = "bincode")] #[test] fn test_create_wallet_from_mnemonic_return_serialized_bytes() { - use dashcore::ffi::DashcoreNetwork; - let mut error = FFIError::success();And similarly for lines 1067-1068.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 867 - 868, Remove the redundant imports of DashcoreNetwork: locate the duplicate `use dashcore::ffi::DashcoreNetwork;` statements in wallet_manager_tests.rs that repeat a module-level import and delete the extra occurrences (including the later duplicates around the tests). Keep the single module-level `use dashcore::ffi::DashcoreNetwork;` and remove any test-local re-imports to avoid redundancy.dash/src/lib.rs (1)
93-127: Consider isolating exported C ABI in a dedicated crate/module boundary.Relying on many
cbindgen:ignoreannotations is workable but brittle over time. A dedicated FFI crate/surface would reduce accidental header exposure risk and maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/lib.rs` around lines 93 - 127, The public module list currently depends on many cbindgen:ignore annotations (see modules like network, blockdata, bloom, policy, pow, sml, taproot) which is brittle; create a dedicated FFI crate or a single top-level ffi boundary module and move all C ABI exports into it (keep the existing pub mod ffi as the only C-facing surface), remove scattered cbindgen:ignore attributes, update module visibility so internal modules (network, blockdata, bloom, policy, pow, sml, taproot, etc.) remain private or crate-only, and re-export only the safe, stable types/functions from the new ffi surface; also add a Cargo feature (e.g., "ffi") gating the ffi crate/module and adjust any callers to import from the new ffi boundary instead of exposing internal modules directly.key-wallet-ffi/src/managed_account.rs (2)
488-488: Documentation references outdated type name.The doc comment on line 488 mentions
FFINetwork::Mainnetbut the function now returnsDashcoreNetwork. Update the comment to reflect the new type.-/// - Returns `FFINetwork::Mainnet` if the account is null +/// - Returns `DashcoreNetwork::Mainnet` if the account is null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` at line 488, The doc comment currently references the old type name `FFINetwork::Mainnet`; update the comment so it mentions the new return type `DashcoreNetwork` (e.g., `DashcoreNetwork::Mainnet`) to match the function's actual return type and keep documentation consistent with the `managed_account` API.
1253-1253: Documentation references outdated type name.Same issue as above - the doc comment references
FFINetwork::Mainnetbut should referenceDashcoreNetwork::Mainnet.-/// - Returns `FFINetwork::Mainnet` if the account is null +/// - Returns `DashcoreNetwork::Mainnet` if the account is null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` at line 1253, Update the doc comment that incorrectly references FFINetwork::Mainnet to use the correct type name DashcoreNetwork::Mainnet in the managed_account.rs documentation; search for occurrences of FFINetwork (e.g., the comment near the function that returns a network when the account is null) and replace them with DashcoreNetwork to ensure the docs reference the correct enum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/cbindgen.toml`:
- Line 17: The cbindgen configuration's includes entry incorrectly references
"../dashcore/dashcore.h"; update the includes key to point to the correct header
location used by the dash dependency (e.g., change the value to
"../dash/dashcore.h") or remove the includes entry entirely if dashcore does not
produce a C header, ensuring the includes array matches the actual header
produced by the dash crate.
In `@key-wallet-ffi/src/lib.rs`:
- Line 34: The tests in account_derivation_tests.rs still reference
crate::FFINetwork::Testnet but lib.rs no longer re-exports FFINetwork; update
account_derivation_tests.rs to import FFINetwork from the types module (e.g.,
add a module-level use crate::types::FFINetwork;) or change each qualified use
to crate::types::FFINetwork::Testnet so all occurrences (lines using
crate::FFINetwork::Testnet at locations noted) resolve to the types::FFINetwork
definition; ensure all Testnet references consistently point to
crate::types::FFINetwork.
In `@key-wallet-ffi/src/types.rs`:
- Line 8: Replace all uses and imports of FFINetwork with
dashcore::ffi::DashcoreNetwork: update any import like crate::types::FFINetwork
to use dashcore::ffi::DashcoreNetwork, and change literal usages (e.g.,
FFINetwork::Testnet) to DashcoreNetwork::Testnet; also update documentation
comments that mention FFINetwork in managed_account and account to reference
dashcore::ffi::DashcoreNetwork. Ensure the new type is imported where needed or
fully qualified (dashcore::ffi::DashcoreNetwork) to avoid shadowing, and run
tests to catch any remaining references.
---
Outside diff comments:
In `@dash-spv-ffi/FFI_API.md`:
- Around line 239-246: Update the safety documentation for
dash_spv_ffi_config_get_network so the default network type matches the updated
return type: replace the incorrect reference to FFINetwork::Mainnet with
DashcoreNetwork::Mainnet in the Safety section and any other occurrences; verify
the doc mentions the function name dash_spv_ffi_config_get_network and the
pointer type FFIClientConfig remain correct.
In `@dash-spv-ffi/src/config.rs`:
- Around line 249-254: Update the stale docs in the network getter comment:
replace the incorrect type reference `FFINetwork::Mainnet` with
`DashcoreNetwork::Mainnet` in the safety/docs block that describes the network
getter (the comment starting "Gets the network type from the configuration"
which mentions `FFIClientConfig`); ensure the comment matches the actual
return/signature of the function so it references `DashcoreNetwork::Mainnet`.
In `@key-wallet-ffi/FFI_API.md`:
- Around line 4394-4397: Update the usage example to match the new
wallet_manager_create signature by passing a DashcoreNetwork argument along with
the FFIError pointer; locate the example where FFIError error = {0};
FFIWalletManager* manager = wallet_manager_create(&error); and change the call
to include an appropriate DashcoreNetwork value (e.g., MAINNET/TESTNET enum) so
the call becomes wallet_manager_create(<DashcoreNetwork>, &error) and subsequent
error handling using error.code remains the same.
- Line 4378: Update the Type Definitions section to replace the stale type name
FFINetwork with the current DashcoreNetwork everywhere it appears; search for
the symbol FFINetwork in the docs (specifically the Type Definitions list and
any type references) and change the label and any descriptive text to
DashcoreNetwork so the documented API matches the implementation and avoids
confusing consumers.
In `@key-wallet-ffi/src/account.rs`:
- Around line 229-235: Update the FFI doc comments that still mention the old
enum name FFINetwork::Mainnet to use DashcoreNetwork::Mainnet instead; locate
the doc blocks above the exported functions (the comment starting "Get the
network of an account" and the other similar safety docs around the same file)
and replace the outdated FFINetwork references with DashcoreNetwork (e.g.,
DashcoreNetwork::Mainnet) so all three occurrences (around the shown block and
at the other noted comment ranges) reflect the correct type name.
---
Nitpick comments:
In `@dash/src/lib.rs`:
- Around line 93-127: The public module list currently depends on many
cbindgen:ignore annotations (see modules like network, blockdata, bloom, policy,
pow, sml, taproot) which is brittle; create a dedicated FFI crate or a single
top-level ffi boundary module and move all C ABI exports into it (keep the
existing pub mod ffi as the only C-facing surface), remove scattered
cbindgen:ignore attributes, update module visibility so internal modules
(network, blockdata, bloom, policy, pow, sml, taproot, etc.) remain private or
crate-only, and re-export only the safe, stable types/functions from the new ffi
surface; also add a Cargo feature (e.g., "ffi") gating the ffi crate/module and
adjust any callers to import from the new ffi boundary instead of exposing
internal modules directly.
In `@key-wallet-ffi/src/managed_account.rs`:
- Line 488: The doc comment currently references the old type name
`FFINetwork::Mainnet`; update the comment so it mentions the new return type
`DashcoreNetwork` (e.g., `DashcoreNetwork::Mainnet`) to match the function's
actual return type and keep documentation consistent with the `managed_account`
API.
- Line 1253: Update the doc comment that incorrectly references
FFINetwork::Mainnet to use the correct type name DashcoreNetwork::Mainnet in the
managed_account.rs documentation; search for occurrences of FFINetwork (e.g.,
the comment near the function that returns a network when the account is null)
and replace them with DashcoreNetwork to ensure the docs reference the correct
enum.
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 867-868: Remove the redundant imports of DashcoreNetwork: locate
the duplicate `use dashcore::ffi::DashcoreNetwork;` statements in
wallet_manager_tests.rs that repeat a module-level import and delete the extra
occurrences (including the later duplicates around the tests). Keep the single
module-level `use dashcore::ffi::DashcoreNetwork;` and remove any test-local
re-imports to avoid redundancy.
🪄 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: adac0bb1-dbfc-4de1-8d78-604bca06b4aa
📒 Files selected for processing (55)
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.mddash-spv-ffi/cbindgen.tomldash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/config.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_client.rsdash-spv-ffi/tests/test_config.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/test_utils.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/unit/test_client_lifecycle.rsdash-spv-ffi/tests/unit/test_configuration.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_memory_management.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash/Cargo.tomldash/build.rsdash/cbindgen.tomldash/src/ffi/mod.rsdash/src/ffi/network.rsdash/src/lib.rsffi-c-tests/header-tests/all.cffi-c-tests/header-tests/dashcore.ckey-wallet-ffi/Cargo.tomlkey-wallet-ffi/FFI_API.mdkey-wallet-ffi/cbindgen.tomlkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/derivation_tests.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rs
💤 Files with no reviewable changes (1)
- dash-spv-ffi/tests/test_utils.rs
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
refactor/ffi-dashcore-network
branch
from
2e6b9fa to
cb025e0
Compare
github-actions
bot
removed
the
merge-conflict
ZocoLini
commented
Apr 15, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-ffi/FFI_API.md (1)
2361-2368:⚠️ Potential issue | 🟡 MinorUpdate stale
FFINetworkwording in the fallback docs.After the signature migration to
DashcoreNetwork, Line 2365, Line 3190, and Line 3382 still document null fallback asFFINetwork::Mainnet. Please regenerate/update doc comments so type names are consistent.Also applies to: 3186-3194, 3378-3386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/FFI_API.md` around lines 2361 - 2368, Update the stale doc comments that still refer to FFINetwork::Mainnet so they match the new return type DashcoreNetwork: replace occurrences of "FFINetwork::Mainnet" with "DashcoreNetwork::Mainnet" in the doc comments for account_get_network (signature: account_get_network(account: *const FFIAccount) -> DashcoreNetwork) and the other affected API entries noted in the review (the similar null-fallback descriptions at the other occurrences). Ensure the safety/fallback lines consistently state the DashcoreNetwork type and value.
🧹 Nitpick comments (1)
dash/build.rs (1)
4-4: Minor inconsistency: use importedenvmodule.Line 4 uses
std::env::varwhile the rest of the function (lines 40-42) uses the importedenv::var. For consistency, use the imported module.♻️ Suggested fix
- if std::env::var("CARGO_FEATURE_FFI").is_ok() { + if env::var("CARGO_FEATURE_FFI").is_ok() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/build.rs` at line 4, The if condition in build.rs uses std::env::var while the rest of the file uses the imported env module; update the conditional to use the imported env::var to keep imports consistent (replace the std::env::var call in the if statement with env::var), ensuring no other behavior changes in the build script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 25-29: The test dereferences the returned pointer from
wallet_manager::wallet_manager_create via unsafe { (*manager).network() } before
checking if manager is null; reorder the checks so you first assert that manager
is not null (manager.is_null() false) and that error shows success, then perform
unsafe dereference to call (*manager).network(); specifically, move the
assert!(!manager.is_null()) (and the error code assertion on *error) ahead of
the unsafe deref to avoid undefined behavior when wallet_manager_create returns
null.
---
Outside diff comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 2361-2368: Update the stale doc comments that still refer to
FFINetwork::Mainnet so they match the new return type DashcoreNetwork: replace
occurrences of "FFINetwork::Mainnet" with "DashcoreNetwork::Mainnet" in the doc
comments for account_get_network (signature: account_get_network(account: *const
FFIAccount) -> DashcoreNetwork) and the other affected API entries noted in the
review (the similar null-fallback descriptions at the other occurrences). Ensure
the safety/fallback lines consistently state the DashcoreNetwork type and value.
---
Nitpick comments:
In `@dash/build.rs`:
- Line 4: The if condition in build.rs uses std::env::var while the rest of the
file uses the imported env module; update the conditional to use the imported
env::var to keep imports consistent (replace the std::env::var call in the if
statement with env::var), ensuring no other behavior changes in the build
script.
🪄 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: 374fe3f9-884e-4706-91b1-511e99ee33a4
📒 Files selected for processing (57)
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.mddash-spv-ffi/cbindgen.tomldash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/config.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_client.rsdash-spv-ffi/tests/test_config.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/test_utils.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/unit/test_client_lifecycle.rsdash-spv-ffi/tests/unit/test_configuration.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_memory_management.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash/Cargo.tomldash/build.rsdash/cbindgen.tomldash/src/ffi/mod.rsdash/src/ffi/network.rsdash/src/lib.rsffi-c-tests/header-tests/all.cffi-c-tests/header-tests/dashcore.ckey-wallet-ffi/Cargo.tomlkey-wallet-ffi/FFI_API.mdkey-wallet-ffi/cbindgen.tomlkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/derivation_tests.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/managed_wallet_tests.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rs
💤 Files with no reviewable changes (1)
- dash-spv-ffi/tests/test_utils.rs
✅ Files skipped from review due to trivial changes (22)
- ffi-c-tests/header-tests/all.c
- key-wallet-ffi/src/address_pool.rs
- key-wallet-ffi/tests/test_addr_simple.rs
- dash/src/ffi/mod.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- dash-spv-ffi/tests/unit/test_error_handling.rs
- key-wallet-ffi/Cargo.toml
- key-wallet-ffi/src/managed_wallet.rs
- dash-spv-ffi/Cargo.toml
- dash-spv-ffi/tests/unit/test_async_operations.rs
- key-wallet-ffi/tests/debug_wallet_add.rs
- key-wallet-ffi/tests/test_passphrase_wallets.rs
- key-wallet-ffi/cbindgen.toml
- dash-spv-ffi/tests/unit/test_type_conversions.rs
- dash-spv-ffi/tests/test_config.rs
- dash/cbindgen.toml
- dash-spv-ffi/cbindgen.toml
- key-wallet-ffi/src/account_collection.rs
- key-wallet-ffi/tests/test_managed_account_collection.rs
- dash-spv-ffi/tests/unit/test_client_lifecycle.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- dash/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (19)
- dash-spv-ffi/tests/unit/test_memory_management.rs
- key-wallet-ffi/tests/test_import_wallet.rs
- key-wallet-ffi/src/lib.rs
- key-wallet-ffi/src/types.rs
- key-wallet-ffi/src/derivation_tests.rs
- dash-spv-ffi/tests/test_client.rs
- key-wallet-ffi/src/wallet_manager_serialization_tests.rs
- dash-spv-ffi/FFI_API.md
- key-wallet-ffi/src/transaction.rs
- key-wallet-ffi/tests/test_account_collection.rs
- key-wallet-ffi/src/address.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet-ffi/src/account.rs
- dash-spv-ffi/src/config.rs
- dash/src/lib.rs
- key-wallet-ffi/src/wallet_manager.rs
- key-wallet-ffi/src/wallet.rs
- key-wallet-ffi/src/keys.rs
- dash/src/ffi/network.rs
xdustinface
reviewed
Apr 15, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Why did you rename it to DashcoreNetwork instead of FFINetwork? I think its a bit confusing because we have Network inside dashcore. I think FFINetwork fits still better since its the FFI layer of the dashcore crate and then follows the naming of all other FFI types?
Collaborator
Author
|
usually c symbols are prefixed with the library's name they belong to to avoid conflict (since there are no namespaces or anything similar) and FFI felt ambiguous and easy to conflict for users or for us in a future, but yeah, I can go with FFINetwork, it takes me 1 sec to make the change |
ZocoLini
force-pushed
the
refactor/ffi-dashcore-network
branch
from
cb025e0 to
05e4291
Compare
xdustinface
reviewed
Apr 16, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/ffi/network.rs`:
- Around line 53-75: The test currently only checks dashcore_network_get_name;
add unit tests that exercise the conversion impls (From<Network> for FFINetwork
and From<FFINetwork> for Network) by asserting round-trip conversions for every
variant of FFINetwork/Network (Mainnet, Testnet, Regtest, Devnet): e.g., for
each Network::X assert FFINetwork::from(Network::X) yields the expected
FFINetwork variant and that Network::from(FFINetwork::from(Network::X)) ==
Network::X, and likewise start from each FFINetwork::X to ensure Network::from
and back produces the original; place these assertions alongside or in a new
test (e.g., test_network_conversions) referencing the FFINetwork and Network
types and the From impls.
- Around line 38-44: dashcore_network_get_name currently takes an FFINetwork
enum directly which allows invalid discriminants from C and UB; change its
signature to accept a primitive (c_int), validate by converting with
FFINetwork::try_from (or TryFrom<c_int>) inside the function, and if conversion
fails return std::ptr::null() as *const ffi::c_char; on success map the
validated FFINetwork to the appropriate C string (ffi::c_char) as before. Ensure
you reference FFINetwork, dashcore_network_get_name, TryFrom/c_int conversion
and return null pointer on invalid input.
In `@ffi-c-tests/header-tests/dashcore.c`:
- Around line 1-3: The PR uses a too-narrow prefix—this change adds a new shared
FFI-facing API surface (the generated dashcore header included via
"dashcore/dashcore.h" and the public enum consumed by downstream crates), so
update the PR title prefix from "refactor:" to "feat:" (or your repo’s allowed
feature prefix) and adjust the PR description to reflect that a new API/feature
is introduced; also ensure the pr-title.yml workflow allows the chosen prefix
for this change.
🪄 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: d502a008-b8b7-4dd8-88b4-736a42cf144f
📒 Files selected for processing (55)
dash-spv-ffi/Cargo.tomldash-spv-ffi/cbindgen.tomldash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/config.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/test_client.rsdash-spv-ffi/tests/test_config.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/test_utils.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/unit/test_client_lifecycle.rsdash-spv-ffi/tests/unit/test_configuration.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_memory_management.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash/Cargo.tomldash/build.rsdash/cbindgen.tomldash/src/ffi/mod.rsdash/src/ffi/network.rsdash/src/lib.rsffi-c-tests/header-tests/all.cffi-c-tests/header-tests/dashcore.ckey-wallet-ffi/Cargo.tomlkey-wallet-ffi/FFI_API.mdkey-wallet-ffi/cbindgen.tomlkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/address_tests.rskey-wallet-ffi/src/derivation.rskey-wallet-ffi/src/keys.rskey-wallet-ffi/src/keys_tests.rskey-wallet-ffi/src/lib.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/managed_wallet_tests.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_serialization_tests.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/debug_wallet_add.rskey-wallet-ffi/tests/integration_test.rskey-wallet-ffi/tests/test_account_collection.rskey-wallet-ffi/tests/test_addr_simple.rskey-wallet-ffi/tests/test_import_wallet.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-ffi/tests/test_passphrase_wallets.rs
💤 Files with no reviewable changes (1)
- dash-spv-ffi/tests/test_utils.rs
✅ Files skipped from review due to trivial changes (27)
- key-wallet-ffi/tests/test_import_wallet.rs
- dash/Cargo.toml
- key-wallet-ffi/tests/test_managed_account_collection.rs
- ffi-c-tests/header-tests/all.c
- key-wallet-ffi/tests/test_passphrase_wallets.rs
- key-wallet-ffi/tests/debug_wallet_add.rs
- dash-spv-ffi/Cargo.toml
- dash-spv-ffi/tests/test_config.rs
- key-wallet-ffi/src/account_tests.rs
- key-wallet-ffi/Cargo.toml
- key-wallet-ffi/src/account.rs
- dash-spv-ffi/tests/dashd_sync/context.rs
- dash-spv-ffi/src/bin/ffi_cli.rs
- key-wallet-ffi/src/managed_account.rs
- dash/cbindgen.toml
- key-wallet-ffi/src/managed_wallet_tests.rs
- key-wallet-ffi/src/wallet_manager.rs
- dash-spv-ffi/tests/test_types.rs
- key-wallet-ffi/tests/test_account_collection.rs
- key-wallet-ffi/cbindgen.toml
- dash-spv-ffi/tests/unit/test_memory_management.rs
- dash-spv-ffi/cbindgen.toml
- key-wallet-ffi/src/account_collection.rs
- dash-spv-ffi/src/config.rs
- key-wallet-ffi/FFI_API.md
- key-wallet-ffi/src/lib.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- dash/src/ffi/mod.rs
- dash-spv-ffi/tests/unit/test_client_lifecycle.rs
- dash-spv-ffi/tests/test_client.rs
- dash-spv-ffi/tests/unit/test_error_handling.rs
- key-wallet-ffi/src/account_derivation_tests.rs
- key-wallet-ffi/src/managed_wallet.rs
- key-wallet-ffi/tests/test_addr_simple.rs
- key-wallet-ffi/src/keys_tests.rs
- key-wallet-ffi/tests/integration_test.rs
- key-wallet-ffi/src/wallet_manager_serialization_tests.rs
- key-wallet-ffi/src/wallet_manager_tests.rs
- key-wallet-ffi/src/derivation.rs
- dash-spv-ffi/tests/unit/test_type_conversions.rs
- dash/src/lib.rs
- key-wallet-ffi/src/transaction.rs
- key-wallet-ffi/src/address.rs
- dash/build.rs
- key-wallet-ffi/src/address_pool.rs
…e to make their own wrapper
ZocoLini
force-pushed
the
refactor/ffi-dashcore-network
branch
from
05e4291 to
66416d9
Compare
xdustinface
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
So, I noticed we have, at least, two FFI wrappers for the Network enum, one in key-wallet-ffi and the other one in platform, with this PR my intention is to make dashcode able to generate a header with all the symbols it would like to export for FFI usage (right now only the Network enum as FFINetwork), this way key-wallet-ffi, dash-spv-ffi and rs-platform-wallet-ffi use the same enum avoiding present and future issues
Something I would like to discuss, I made a module and feature in dashcore called 'ffi' because I didn't want to create a new crate, I don't feel like a crate is the right approach for this but I ended up having to add '/// cbindgen:ignore' in a bunch of modules to avoid cbindgen from generating unwanted symbols in the header file, what looks kinda ugly. Should I keep it this way or create a new crate??
Thing that I have done in this PR:
Summary by CodeRabbit
Release Notes
New Features
Refactor
FFINetworktype to dashcore's FFI module, centralizing network definitions for all FFI consumers.