chore: bump rust-dashcore to f92f114b83 (Rust-only)#3414
Merged
Conversation
Update all rust-dashcore workspace deps to rev f92f114b83. Minimum changes to compile — no Swift/iOS changes included. - Remove "std" feature from dashcore in rs-dpp (removed upstream) - Fix key-wallet-manager import path change - Add mark_instant_send_utxos and monitor_revision to WalletInfoInterface - Update WalletTransactionChecker signature (add update_balance param) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughUpdated workspace git revision pins, removed a Changes
Sequence Diagram(s)sequenceDiagram
participant WS as WalletService
participant SPVtmp as SPVClient (temp)
participant Manager as CoreWalletManager
participant SPVfinal as SPVClient (with SPVEventHandlers)
participant Handlers as SPVEventHandlers
WS->>SPVtmp: init(network, dataDir, startHeight)
SPVtmp->>Manager: sharedManager() / provide manager
Manager-->>WS: CoreWalletManager instance
WS->>SPVtmp: destroy()
WS->>Handlers: assemble handlers (including error handler)
WS->>SPVfinal: init(..., eventHandlers: Handlers)
SPVfinal-->>WS: SPV client ready with callbacks wired
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/examples/basic_usage.rs (1)
25-31: Compile themanagerexample explicitly.This import change only lives in a feature-gated example, and the stated
cargo check -p platform-walletdoes not build examples by default. Please runcargo check -p platform-wallet --example basic_usage --features managerlocally, and ideally add that target to CI so this re-export stays covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/examples/basic_usage.rs` around lines 25 - 31, The example uses a manager-gated import (WalletManager and PlatformWalletInfo) but examples aren't built by default; run cargo check -p platform-wallet --example basic_usage --features manager locally to compile this example and verify the re-export, and then add that example-target (cargo check with --example basic_usage --features manager) to CI so the feature-gated import remains covered and won't regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-platform-wallet/examples/basic_usage.rs`:
- Around line 25-31: The example uses a manager-gated import (WalletManager and
PlatformWalletInfo) but examples aren't built by default; run cargo check -p
platform-wallet --example basic_usage --features manager locally to compile this
example and verify the re-export, and then add that example-target (cargo check
with --example basic_usage --features manager) to CI so the feature-gated import
remains covered and won't regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f94e9197-c82e-4f7a-ba4d-6f9fbabaf2fe
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlpackages/rs-dpp/Cargo.tomlpackages/rs-platform-wallet/examples/basic_usage.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_info_interface.rspackages/rs-platform-wallet/src/platform_wallet_info/wallet_transaction_checker.rs
💤 Files with no reviewable changes (1)
- packages/rs-dpp/Cargo.toml
The new dashcore rev added an FFIEventCallbacks parameter to dash_spv_ffi_client_new. Pass Default for now. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk-ffi/src/unified.rs`:
- Around line 75-78: core_client is always created via
dash_spv_ffi::dash_spv_ffi_client_new but teardown is only compiled under
#[cfg(feature = "core")], causing leaks and mismatch with
dash_unified_sdk_has_core_support(); guard creation with the same feature flag:
only call dash_spv_ffi::dash_spv_ffi_client_new and assign core_client when
#[cfg(feature = "core")] is active, and in the not(feature = "core") case
provide a safe no-op value (e.g., None or core_client = std::ptr::null_mut() /
Option::None consistent with the rest of the file) so later code and
dash_unified_sdk_has_core_support() behave consistently with the destruction
paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dcd06fa-9453-47d3-a538-e1d32326ef37
📒 Files selected for processing (1)
packages/rs-sdk-ffi/src/unified.rs
Comment on lines
+75
to
+78
| let core_client = dash_spv_ffi::dash_spv_ffi_client_new( | ||
| config.core_config, | ||
| dash_spv_ffi::FFIEventCallbacks::default(), | ||
| ); |
Contributor
There was a problem hiding this comment.
Guard core client creation by feature flag to avoid platform-only leaks.
core_client is created unconditionally here, but destruction paths are #[cfg(feature = "core")]. In not(feature = "core") builds this can leak the client and makes behavior inconsistent with dash_unified_sdk_has_core_support().
Proposed fix
- // Create Core SDK client (always enabled in unified SDK)
- let core_client = dash_spv_ffi::dash_spv_ffi_client_new(
- config.core_config,
- dash_spv_ffi::FFIEventCallbacks::default(),
- );
+ // Create Core SDK client only when core feature is enabled
+ #[cfg(feature = "core")]
+ let core_client = dash_spv_ffi::dash_spv_ffi_client_new(
+ config.core_config,
+ dash_spv_ffi::FFIEventCallbacks::default(),
+ );
+ #[cfg(not(feature = "core"))]
+ let core_client = std::ptr::null_mut();📝 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.
Suggested change
| let core_client = dash_spv_ffi::dash_spv_ffi_client_new( | |
| config.core_config, | |
| dash_spv_ffi::FFIEventCallbacks::default(), | |
| ); | |
| // Create Core SDK client only when core feature is enabled | |
| #[cfg(feature = "core")] | |
| let core_client = dash_spv_ffi::dash_spv_ffi_client_new( | |
| config.core_config, | |
| dash_spv_ffi::FFIEventCallbacks::default(), | |
| ); | |
| #[cfg(not(feature = "core"))] | |
| let core_client = std::ptr::null_mut(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-sdk-ffi/src/unified.rs` around lines 75 - 78, core_client is
always created via dash_spv_ffi::dash_spv_ffi_client_new but teardown is only
compiled under #[cfg(feature = "core")], causing leaks and mismatch with
dash_unified_sdk_has_core_support(); guard creation with the same feature flag:
only call dash_spv_ffi::dash_spv_ffi_client_new and assign core_client when
#[cfg(feature = "core")] is active, and in the not(feature = "core") case
provide a safe no-op value (e.g., None or core_client = std::ptr::null_mut() /
Option::None consistent with the rest of the file) so later code and
dash_unified_sdk_has_core_support() behave consistently with the destruction
paths.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3414 +/- ##
============================================
- Coverage 79.93% 79.93% -0.01%
============================================
Files 2861 2861
Lines 280230 280236 +6
============================================
Hits 223993 223993
- Misses 56237 56243 +6
🚀 New features to boost your workflow:
|
- Pass FFIEventCallbacks at SPVClient construction instead of setting handlers individually (upstream API change) - Add SPVEventHandlers struct bundling all handler protocols - Add SPVClientErrorEventsHandler protocol and implementation - Update FFI callback signatures for new parameters - Remove key-wallet-manager from build_ios.sh (merged upstream) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift (1)
281-286:⚠️ Potential issue | 🟠 MajorPotential retain cycle between WalletService and event handlers.
The handler implementations hold strong references to
walletService, whileWalletService→SPVClient→SPVEventHandlers→ handlers creates a circular reference. This prevents deallocation when the service is released.Consider using
weakreferences:♻️ Suggested fix
internal final class SPVProgressUpdateEventHandlerImpl: SPVProgressUpdateEventHandler, Sendable { - private let walletService: WalletService + private weak var walletService: WalletService? init(walletService: WalletService) { self.walletService = walletService } func onProgressUpdate(_ progress: SPVSyncProgress) { Task { `@MainActor` in - walletService.syncProgress = progress + walletService?.syncProgress = progress } } }Apply the same pattern to all handler implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift` around lines 281 - 286, The handler class SPVProgressUpdateEventHandlerImpl currently holds a strong reference to WalletService causing a retain cycle via WalletService → SPVClient → SPVEventHandlers → handlers; change the walletService property to a weak optional (e.g. weak var walletService: WalletService?) and update initializer/use-sites to safely unwrap the optional before accessing, and apply the same weak-optional pattern to all other SPV*EventHandlerImpl classes to break the cycle.
🧹 Nitpick comments (3)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift (2)
490-528: Newstatusparameter andon_transaction_status_changedcallback are not utilized.The
FFITransactionContext statusparameter (line 503) is received but not forwarded toonTransactionReceived. Additionally,on_transaction_status_changedis set tonil(line 494).If transaction status tracking is needed for the wallet UI, this would require extending the
SPVWalletEventsHandlerprotocol. Consider documenting this as a future enhancement if intentionally deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift` around lines 490 - 528, The FFITransactionContext status passed into onSpvTransactionReceivedCallbackC is ignored and on_transaction_status_changed is left nil; update the SPVWalletEventsHandler protocol to accept a transaction status (e.g., add a parameter to onTransactionReceived or add a new onTransactionStatusChanged method), then modify intoFFIWalletEventCallbacks to wire on_transaction_status_changed to a new C callback (or keep forwarding status via onSpvTransactionReceivedCallbackC) and change onSpvTransactionReceivedCallbackC to forward the received status (FFITransactionContext) into the handler (SPVWalletEventsHandler.onTransactionReceived or the new method) so status is preserved across the FFI boundary.
241-252: New FFI parametersconfirmedTxidsandconfirmedTxidCountare unused.The callback now receives confirmed transaction IDs from the FFI layer but drops them. If this data is needed for wallet functionality (e.g., confirming pending transactions), the
SPVSyncEventsHandlerprotocol and implementations would need to be extended.If this is intentional for a minimal update, consider adding a
// TODO:comment for future implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift` around lines 241 - 252, The callback onSpvBlockProcessedCallbackC currently ignores the new FFI parameters confirmedTxids and confirmedTxidCount; update this function to convert the confirmedTxids UnsafePointer<Byte32>? into a Swift array (e.g., using bytePtrIntoData or similar for each 32-byte entry) and pass that array (or a count/optional) into the SPVSyncEventsHandler by extending the onBlocksProcessed method, or if you intentionally omit handling, add a clear TODO comment in onSpvBlockProcessedCallbackC referencing confirmedTxids and confirmedTxidCount and mark SPVSyncEventsHandler/onBlocksProcessed as a future extension point; use the symbols onSpvBlockProcessedCallbackC, confirmedTxids, confirmedTxidCount, rawPtrIntoSpvSyncEventsHandler, bytePtrIntoData, and SPVSyncEventsHandler to locate and apply the change.packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift (1)
120-153: Two-phase initialization creates an intermediate invalid state.After
dummyClient.destroy()on line 135,self.spvClientstill references the destroyed client until line 145. If any code between lines 135-153 throws (particularly thetry!on lines 145-150 or 153), the object would be left with an invalidspvClientreference.Additionally, the
dummyClientreference remains valid in scope and could accidentally be used after destruction.Consider restructuring to avoid the intermediate invalid state:
♻️ Suggested safer approach
- // Create SPV client first with dummy handlers to obtain the wallet manager, - // then destroy and recreate with real handlers that reference self. - let dummyClient = try! SPVClient( - network: network.sdkNetwork, - dataDir: dataDir, - startHeight: 0, - ) - - self.spvClient = dummyClient - - // Create the SDK wallet manager by reusing the SPV client's shared manager - // TODO: Investigate this error - self.walletManager = try! CoreWalletManager(spvClient: dummyClient, modelContainer: modelContainer) - - // Destroy the dummy client and recreate with real event handlers - dummyClient.destroy() + // Phase 1: Create temporary client to bootstrap wallet manager + let tempClient = try! SPVClient( + network: network.sdkNetwork, + dataDir: dataDir, + startHeight: 0, + ) + + // Create the SDK wallet manager by reusing the SPV client's shared manager + // TODO: Investigate this error + self.walletManager = try! CoreWalletManager(spvClient: tempClient, modelContainer: modelContainer) + + // Destroy temp client before creating the real one + tempClient.destroy() let handlers = SPVEventHandlers( progress: SPVProgressUpdateEventHandlerImpl(walletService: self), sync: SPVSyncEventsHandlerImpl(walletService: self), network: SPVNetworkEventsHandlerImpl(walletService: self), wallet: SPVWalletEventsHandlerImpl(walletService: self), error: SPVClientErrorEventsHandlerImpl(walletService: self) ) + // Phase 2: Create real client with handlers - assign directly self.spvClient = try! SPVClient( network: network.sdkNetwork, dataDir: dataDir, startHeight: 0, eventHandlers: handlers, ) // Recreate the wallet manager with the new client self.walletManager = try! CoreWalletManager(spvClient: self.spvClient, modelContainer: self.modelContainer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift` around lines 120 - 153, Don't assign the temporary SPV client to self.spvClient or self.walletManager before it's replaced; instead keep dummyClient as a local variable, create the initial CoreWalletManager with that local dummyClient, call dummyClient.destroy(), then construct the real SPVClient (with SPVEventHandlers) and only after that assign self.spvClient and recreate/assign self.walletManager from the real client. Also replace force-try usages around SPVClient(...) and CoreWalletManager(...) with proper error handling (do/catch) so an exception cannot leave self.spvClient pointing at a destroyed client; key symbols: dummyClient, SPVClient(...), dummyClient.destroy(), SPVEventHandlers(...), self.spvClient, CoreWalletManager(...), self.walletManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`:
- Around line 281-286: The handler class SPVProgressUpdateEventHandlerImpl
currently holds a strong reference to WalletService causing a retain cycle via
WalletService → SPVClient → SPVEventHandlers → handlers; change the
walletService property to a weak optional (e.g. weak var walletService:
WalletService?) and update initializer/use-sites to safely unwrap the optional
before accessing, and apply the same weak-optional pattern to all other
SPV*EventHandlerImpl classes to break the cycle.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift`:
- Around line 120-153: Don't assign the temporary SPV client to self.spvClient
or self.walletManager before it's replaced; instead keep dummyClient as a local
variable, create the initial CoreWalletManager with that local dummyClient, call
dummyClient.destroy(), then construct the real SPVClient (with SPVEventHandlers)
and only after that assign self.spvClient and recreate/assign self.walletManager
from the real client. Also replace force-try usages around SPVClient(...) and
CoreWalletManager(...) with proper error handling (do/catch) so an exception
cannot leave self.spvClient pointing at a destroyed client; key symbols:
dummyClient, SPVClient(...), dummyClient.destroy(), SPVEventHandlers(...),
self.spvClient, CoreWalletManager(...), self.walletManager.
In `@packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swift`:
- Around line 490-528: The FFITransactionContext status passed into
onSpvTransactionReceivedCallbackC is ignored and on_transaction_status_changed
is left nil; update the SPVWalletEventsHandler protocol to accept a transaction
status (e.g., add a parameter to onTransactionReceived or add a new
onTransactionStatusChanged method), then modify intoFFIWalletEventCallbacks to
wire on_transaction_status_changed to a new C callback (or keep forwarding
status via onSpvTransactionReceivedCallbackC) and change
onSpvTransactionReceivedCallbackC to forward the received status
(FFITransactionContext) into the handler
(SPVWalletEventsHandler.onTransactionReceived or the new method) so status is
preserved across the FFI boundary.
- Around line 241-252: The callback onSpvBlockProcessedCallbackC currently
ignores the new FFI parameters confirmedTxids and confirmedTxidCount; update
this function to convert the confirmedTxids UnsafePointer<Byte32>? into a Swift
array (e.g., using bytePtrIntoData or similar for each 32-byte entry) and pass
that array (or a count/optional) into the SPVSyncEventsHandler by extending the
onBlocksProcessed method, or if you intentionally omit handling, add a clear
TODO comment in onSpvBlockProcessedCallbackC referencing confirmedTxids and
confirmedTxidCount and mark SPVSyncEventsHandler/onBlocksProcessed as a future
extension point; use the symbols onSpvBlockProcessedCallbackC, confirmedTxids,
confirmedTxidCount, rawPtrIntoSpvSyncEventsHandler, bytePtrIntoData, and
SPVSyncEventsHandler to locate and apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52e35331-f90b-4e20-b333-d5677998d05b
📒 Files selected for processing (4)
packages/rs-sdk-ffi/build_ios.shpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVEventHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "521631a9c22e5d5ea502fe17854e12dbb7e4a7354cf57cf29ed9e400ddff4a9c"
)Xcode manual integration:
|
…get rev Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Self Reviewed |
6 tasks
5 tasks
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
1 participant
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.
Summary
8cbae416tof92f114b83"std"feature from dashcore in rs-dpp (removed upstream)key-wallet-managerimport path changemark_instant_send_utxosandmonitor_revisiontoWalletInfoInterfaceWalletTransactionChecker::check_core_transactionsignature (&mut Wallet,update_balanceparam)Test plan
cargo check -p dpp --all-featurespassescargo check -p platform-walletpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
New Features
Improvements