Skip to content

refactor: make WalletManager a pure state machine by removing tokio broadcast channel #585

Description

@QuantumExplorer

Problem

WalletManager in key-wallet embeds a tokio::sync::broadcast::Sender<WalletEvent> and the WalletInterface trait exposes subscribe_events() -> broadcast::Receiver<WalletEvent>. This forces a tokio runtime dependency on key-wallet, which should be a pure wallet primitives library.

The tokio dependency exists solely for this broadcast channel — there is no other runtime usage. It was inherited from the key-wallet-manager crate when it was merged into key-wallet in PR #503. PR #584 gates it behind a feature flag as a stopgap, but the root cause remains: event emission is baked into the state machine.

Why this matters

  • key-wallet cannot be used in non-tokio environments (WASM, embedded, synchronous apps) with the manager enabled
  • The WalletInterface trait leaks a tokio type in its public API
  • Events are emitted during processing, which means consumers can observe partial state (e.g., a TransactionReceived event fires while the block is still being processed)
  • The broadcast channel silently drops events when there are no subscribers (let _ = self.event_sender.send(event))

Proposed Architecture: Outbox Pattern

Mutation methods return events instead of emitting them internally. The WalletManager becomes a pure state machine that accumulates events as side effects of state transitions and returns them to the caller.

Before (current)

WalletManager::process_block()
  → internally sends WalletEvent on broadcast channel
  → caller spawns a monitor task to receive events
  → monitor task calls EventHandler::on_wallet_event()

After (proposed)

WalletManager::process_block()
  → returns (BlockProcessingResult, Vec<WalletEvent>)
  → caller dispatches events directly via EventHandler::on_wallet_event()

Specific changes

WalletInterface trait — return events from mutation methods:

Method Current return New return
process_block BlockProcessingResult (BlockProcessingResult, Vec<WalletEvent>)
process_mempool_transaction MempoolTransactionResult (MempoolTransactionResult, Vec<WalletEvent>)
process_instant_send_lock () Vec<WalletEvent>
update_synced_height () Vec<WalletEvent>
subscribe_events broadcast::Receiver removed

WalletManager struct:

  • Remove event_sender: broadcast::Sender<WalletEvent> field
  • Rename emit_balance_changes()collect_balance_changes() returning Vec<WalletEvent>
  • check_transaction_in_all_wallets() returns (CheckTransactionsResult, Vec<WalletEvent>)

dash-spv — dispatch at call sites:

  • BlocksManager::process_buffered_blocks() collects wallet events from returned tuples and passes them up to the SyncCoordinator
  • MempoolManager::handle_tx() and mark_instant_send() do the same
  • SyncCoordinator dispatches wallet events through EventHandler::on_wallet_event() alongside sync events
  • Remove the wallet event broadcast monitor task from DashSpvClient::run()

dash-spv-ffi — no changes needed. FFIEventCallbacks already implements EventHandler::on_wallet_event() which dispatches to C callbacks. It's called the same way regardless of where the call originates.

key-wallet Cargo.toml:

  • Remove tokio from [dependencies] entirely
  • Add tokio to [dev-dependencies] only (for #[tokio::test])
  • The manager feature becomes dependency-free

Benefits

  1. key-wallet with manager has zero runtime dependencies (no tokio, no rayon with the existing feature gate)
  2. Events are delivered after the method returns — no partial-state observations
  3. No silent event drops — events are always returned, caller decides what to do
  4. Simpler testing — assert on the returned Vec<WalletEvent> directly instead of subscribing to a channel
  5. The WalletInterface trait has no tokio types in its API

Migration Risks

  • Multi-subscriber loss: The broadcast channel supported multiple subscribers. After this change, the EventHandler is the single dispatch point. Tests that need direct event access should use a TestEventHandler that republishes events.
  • Event delivery timing: Events are now batched per method call rather than emitted inline. For process_block, all transaction events are returned together after the full block is processed. This is actually safer but is a behavior change.
  • Breaking trait change: WalletInterface signature changes affect MockWallet, NonMatchingMockWallet, and any external implementors.

Suggested PR Sequence

PR 1 (key-wallet): Change return types on WalletInterface methods and WalletManager internals to return Vec<WalletEvent>. Keep subscribe_events() temporarily with #[deprecated]. Update key-wallet tests.

PR 2 (dash-spv): Update BlocksManager, MempoolManager, and SyncCoordinator to consume returned events and dispatch via EventHandler. Remove the broadcast monitor task. Update integration tests.

PR 3 (key-wallet cleanup): Remove subscribe_events(), remove event_sender field, remove tokio from dependencies. Final verification that cargo check -p key-wallet --features manager works without tokio.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions