refactor: move wallet services into crate - #1590
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR reorganizes wallet service modules into the new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Walletd
participant SDK as WalletSdk
participant Svc as sdk_services
participant AM as AccountMonitor
participant TX as TransactionService
participant SC as StealthUtxoScannerWorker
Note over Walletd,Svc: New startup wiring
Walletd->>Svc: spawn_services(shutdown_signal, Notify, WalletSdk)
activate Svc
Svc->>AM: start AccountMonitor
Svc->>TX: start TransactionService
Svc->>SC: start StealthUtxoScannerWorker (UtxoScanner::new)
Svc-->>Walletd: return Services { services_fut, account_monitor_handle, transaction_service_handle }
deactivate Svc
rect rgba(235,245,255,0.5)
Note right of Walletd: Handles and Notify now come from sdk_services
end
sequenceDiagram
autonumber
actor Caller
participant AR as AccountRecoveryService
participant NI as IndexerJsonRpc (feature)
participant Store as WalletStore
participant AM as AccountMonitor
Caller->>AR: try_recover_account(key)
AR->>NI: query account state
NI-->>AR: response / error
alt valid response
AR->>Store: persist recovered data
AR->>AM: update/notify
AR-->>Caller: Ok(true/false)
else network error
AR-->>Caller: Err(AccountRecoveryError::NetworkInterfaceError)
else invalid response
AR-->>Caller: Err(AccountRecoveryError::InvalidResponse)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
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 |
2c63bdc to
474935b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/tari_walletd/src/services/mod.rs (1)
32-74: Trigger shutdown on service crashIn
run_tari_ootle_walletd’stokio::select!(lib.rs 159–168), before returningErr(err)in theservices.services_futbranch, callshutdown_signal.trigger()so all other spawned tasks receive the shutdown signal and can exit cleanly.
🧹 Nitpick comments (2)
applications/tari_walletd/src/handlers/nfts.rs (1)
126-126: Inconsistent usage of wait_for_result helper.Line 126 uses the imported
wait_for_resultdirectly, while line 318 uses the fully-qualified pathcrate::handlers::helpers::wait_for_result. Consider using the imported version consistently for cleaner code.Apply this diff to use the imported helper consistently:
- let finalized = crate::handlers::helpers::wait_for_result(&mut events, tx_id).await?; + let finalized = wait_for_result(&mut events, tx_id).await?;Also applies to: 318-318
crates/wallet/sdk_services/src/account_recovery/service.rs (1)
170-225: Deduplicate the recovered-account labelMinor nit: we call
format!("recovered-account-{}", key.key_index)twice in the two match arms. Hoisting it to alet label = format!(...)once before the match (and reusingSome(label.as_str())) would save the extra allocation and keep the two branches consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
applications/tari_walletd/Cargo.toml(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(1 hunks)applications/tari_walletd/src/handlers/context.rs(2 hunks)applications/tari_walletd/src/handlers/helpers.rs(1 hunks)applications/tari_walletd/src/handlers/nfts.rs(3 hunks)applications/tari_walletd/src/handlers/rpc.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(2 hunks)applications/tari_walletd/src/lib.rs(2 hunks)applications/tari_walletd/src/services/mod.rs(1 hunks)applications/tari_walletd/src/services/template_monitor.rs(1 hunks)crates/wallet/sdk_services/Cargo.toml(1 hunks)crates/wallet/sdk_services/src/account_monitor.rs(1 hunks)crates/wallet/sdk_services/src/account_recovery/error.rs(1 hunks)crates/wallet/sdk_services/src/account_recovery/mod.rs(1 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(4 hunks)crates/wallet/sdk_services/src/lib.rs(1 hunks)crates/wallet/sdk_services/src/transaction_service/handle.rs(1 hunks)crates/wallet/sdk_services/src/transaction_service/mod.rs(1 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/mod.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(5 hunks)utilities/tariswap_test_bench/Cargo.toml(1 hunks)utilities/tariswap_test_bench/src/runner.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
applications/tari_walletd/src/services/mod.rs (2)
applications/tari_walletd/src/handlers/context.rs (2)
account_monitor(86-88)transaction_service(90-92)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
applications/tari_walletd/src/services/template_monitor.rs (1)
crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
applications/tari_walletd/src/handlers/transaction.rs (2)
applications/tari_walletd/src/handlers/context.rs (1)
transaction_service(90-92)applications/tari_walletd/src/handlers/helpers.rs (5)
get_account(112-123)get_account_or_default(140-157)invalid_params(159-170)not_found(180-187)transaction_rejected(196-201)
crates/wallet/sdk_services/src/lib.rs (2)
applications/tari_walletd/src/handlers/context.rs (2)
account_monitor(86-88)transaction_service(90-92)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
applications/tari_walletd/src/handlers/nfts.rs (1)
applications/tari_walletd/src/handlers/helpers.rs (1)
wait_for_result(31-53)
applications/tari_walletd/src/lib.rs (4)
crates/wallet/sdk_services/src/notify.rs (2)
notify(21-23)new(12-15)applications/tari_walletd/src/handlers/context.rs (2)
config(94-96)new(40-58)applications/tari_walletd/src/services/mod.rs (1)
spawn_services(32-74)crates/wallet/sdk_services/src/account_recovery/service.rs (1)
new(41-51)
crates/wallet/sdk_services/src/account_recovery/service.rs (2)
applications/tari_walletd/src/handlers/context.rs (1)
account_monitor(86-88)crates/engine_types/src/substate.rs (1)
component(590-595)
applications/tari_walletd/src/handlers/context.rs (1)
crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
crates/wallet/sdk_services/src/account_monitor.rs (1)
crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
crates/wallet/sdk_services/src/account_recovery/error.rs (1)
applications/tari_walletd/src/handlers/context.rs (1)
account_monitor(86-88)
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (38)
applications/tari_walletd/src/services/mod.rs (1)
20-30: LGTM! Import refactor aligns with PR objectives.The imports have been correctly updated to use the new
tari_ootle_wallet_sdk_servicescrate, successfully decoupling wallet services from walletd. The import structure is clear and properly organized.crates/wallet/sdk_services/src/transaction_service/handle.rs (1)
10-10: LGTM! Import path updated to match module reorganization.The import path for
Replyhas been correctly updated fromcrate::services::Replytocrate::Reply, aligning with the broader refactoring to consolidate types at the crate root.applications/tari_walletd/Cargo.toml (1)
20-20: LGTM! Feature flag enabled for indexer JSON-RPC support.The
indexer_jrpcfeature has been appropriately enabled fortari_ootle_wallet_sdk_services, consistent with the refactoring to modularize wallet services.applications/tari_walletd/src/handlers/accounts.rs (1)
33-33: LGTM! Import path updated for event type.The import path for
TransactionSubmittedEventhas been correctly updated totari_ootle_wallet_sdk_services::events, reflecting the reorganization of wallet services into a dedicated crate.utilities/tariswap_test_bench/Cargo.toml (1)
12-12: LGTM! Feature flag enabled for test utilities.The
indexer_jrpcfeature has been correctly enabled for the test bench, ensuring compatibility with the refactored wallet services.crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
22-22: LGTM! Log target updated to reflect module structure.The
LOG_TARGEThas been appropriately renamed to"tari::ootle::wallet_services::utxo_recovery", aligning with the broader refactoring to organize wallet services into a dedicated crate.applications/tari_walletd/src/services/template_monitor.rs (1)
17-17: LGTM! Import path correctly updated.The import path has been successfully migrated from the local crate to
tari_ootle_wallet_sdk_services, aligning with the PR objective to decouple wallet services.applications/tari_walletd/src/handlers/rpc.rs (1)
5-5: LGTM! Import path correctly updated.
AuthLoginRequestEventhas been successfully migrated to the externaltari_ootle_wallet_sdk_services::eventsmodule, consistent with the refactoring objectives.crates/wallet/sdk_services/src/account_monitor.rs (2)
47-52: LGTM! Imports correctly reorganized for the new crate structure.The imports have been appropriately updated to use crate-local paths (
crate::events,crate::notify,crate::utxo_scanner), reflecting that this module now resides within thetari_ootle_wallet_sdk_servicescrate.
54-54: LGTM! LOG_TARGET correctly updated.The log target has been appropriately changed from
wallet_daemontowallet_services, accurately reflecting the module's new location in the refactored crate structure.applications/tari_walletd/src/handlers/transaction.rs (2)
17-17: LGTM! Import paths correctly updated.
WalletEventandTransactionServiceErrorhave been successfully migrated totari_ootle_wallet_sdk_services, with appropriate module separation (eventsandtransaction_servicerespectively).
45-49: LGTM! Import block correctly reorganized.The import statement has been properly restructured to remove the migrated types while maintaining the necessary local imports.
crates/wallet/sdk_services/src/utxo_scanner/mod.rs (2)
8-8: LGTM! Worker module correctly declared.The new
workermodule has been properly added to the utxo_scanner module structure.
14-14: LGTM! Worker items correctly re-exported.The
pub use worker::*;statement appropriately exposes the worker module's public items at the utxo_scanner level, consistent with the existing pattern used for other submodules (error,scanner,utxo_recovery).utilities/tariswap_test_bench/src/runner.rs (1)
11-11: LGTM: Import path updated consistently.The import path change from
indexer_jrpc_impltoindexer_jrpcaligns with the PR's refactoring objective to reorganize wallet services into a new crate structure.crates/wallet/sdk_services/src/transaction_service/mod.rs (1)
10-10: LGTM: Visibility elevation aligns with refactoring goal.Changing from
pub(super)topubmakes the transaction service exports publicly accessible, consistent with the PR's objective to expose wallet services as a reusable crate.crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (2)
28-28: LGTM: Log target updated to reflect new namespace.The log target change from
wallettowallet_servicesaligns with the module reorganization into thetari_ootle_wallet_sdk_servicescrate.
32-32: LGTM: Struct visibility elevated for external access.Changing
UtxoScannerRoundfrompub(crate)topubexposes it for external use, consistent with the PR's goal to make wallet services reusable outside of walletd.crates/wallet/sdk_services/src/transaction_service/service.rs (2)
28-28: LGTM: Event imports relocated to dedicated module.The import path change from
crate::servicestocrate::eventsreflects the reorganization of event types into a dedicated module, improving code organization.
32-32: LGTM: Log target updated to reflect crate rename.The log target change from
wallet_daemontowallet_servicesaccurately reflects the service's relocation to thetari_ootle_wallet_sdk_servicescrate.applications/tari_walletd/src/handlers/helpers.rs (2)
18-21: LGTM: Imports consolidated and updated to new crate.The grouped import from
tari_ootle_wallet_sdk_servicesconsolidates event and network interface types, aligning with the PR's refactoring to expose wallet services as a separate crate.
29-29: LGTM: Local import clarified.Moving
ApplicationErrorCodeto a separate import fromcrate::jrpc_serverimproves clarity by distinguishing local from external dependencies.crates/wallet/sdk_services/src/account_recovery/mod.rs (1)
1-8: LGTM! Clean module structure.The module organization follows Rust conventions by declaring submodules and re-exporting their public items at the top level. This provides a clean public API for the account_recovery functionality.
applications/tari_walletd/src/lib.rs (2)
45-49: LGTM! Import path reorganization aligns with the refactoring objective.The consolidation of service imports from the new
tari_ootle_wallet_sdk_servicescrate successfully decouples these services from walletd, achieving the PR's stated goal.
96-100: Clean usage of externalized AccountRecoveryService.The instantiation now uses the imported type directly without module prefixes, which improves readability.
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (5)
6-6: Good use of std::future::poll_fn.Using
std::future::poll_fninstead offutures::future::poll_fnreduces dependencies and is appropriate sincepoll_fnis available in the standard library.
25-27: LGTM! Import and logging updates align with the crate refactoring.The direct import of
UtxoScannerfrom the crate module and the updated LOG_TARGET path correctly reflect the new module structure.
83-83: Consistent use of poll_fn without futures prefix.The updated call site correctly uses the imported
poll_fnwithout thefutures::prefix.
187-187: Cleaner UtxoScanner instantiation.Using the directly imported
UtxoScanner::newinstead of a fully-qualified path improves readability.
46-48: No changes needed—public API in use externally
The method is invoked inapplications/tari_walletd/src/services/mod.rs:54, so itspubvisibility is intentional and required.applications/tari_walletd/src/handlers/context.rs (1)
6-12: LGTM! Clean import consolidation.The reorganization of imports under
tari_ootle_wallet_sdk_servicessuccessfully consolidates service-related types into the externalized crate without changing any public APIs or behavior.applications/tari_walletd/src/handlers/nfts.rs (4)
22-22: LGTM! Removed unused import.The
TransactionIdimport is no longer needed sincewait_for_resultis now imported from helpers.
39-39: Good refactoring to use centralized wait_for_result.Moving to the shared helper function eliminates code duplication.
288-288: Same into_unversioned() transformation applied here.Consistent with the change on line 118. Same verification concern applies.
118-118: Approve existing into_unversioned usage: NFT handler’s use ofinto_unversioned()matches validator and accounts patterns, aligning with transaction builder design.crates/wallet/sdk_services/src/lib.rs (1)
4-14: Modular re-exports look solidNice cleanup—splitting the services into explicit modules while gating
indexer_jrpckeeps the crate lean for consumers that don’t need the indexer client. Likewise, constrainingReplytopub(crate)tightens the API surface without hurting internals.crates/wallet/sdk_services/Cargo.toml (1)
17-35: Feature wiring matches the crate re-orgTurning
tari_indexer_client/reqwestinto optional deps under the newindexer_jrpcfeature, plus widening the tokio feature set, lines up perfectly with the module gating. Keeps downstream builds minimal unless they opt in.crates/wallet/sdk_services/src/account_recovery/error.rs (1)
4-25: Error consolidation reads well
AccountRecoveryErrornicely captures each subsystem we touch and keeps conversions ergonomic with#[from]. The stringlyNetworkInterfaceError/InvalidResponsevariants cover the remaining cases cleanly.
474935b to
3fbf709
Compare
2 participants
Description
refactor: move wallet services into crate
Motivation and Context
Decouple these services from walletd to allow a custom wallet to be built
How Has This Been Tested?
Code compiles
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores