feat!: full http wallet refactor#7215
Conversation
…ode-wallet-service-as-http
…ode-wallet-service-as-http
…-as-http' into feature/base-node-wallet-service-as-http
…-as-http' into feature/base-node-wallet-service-as-http
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
base_layer/wallet/src/wallet.rs (1)
252-266: Refactor duplicated URL parsing logic and improve error handling.The URL parsing logic is duplicated and the error messages could expose internal details. Consider extracting this into a helper method and improving error handling.
base_layer/wallet_ffi/src/lib.rs (2)
6880-6888: Fix UTF-8 error handling to prevent panics.The
expect()call will panic if the C string contains invalid UTF-8. This is a known issue that should be addressed with proper error handling.
14126-14127: Fix the URL typo in the test case.The IP address is malformed with a missing octet. This is a known issue from previous reviews.
applications/minotari_console_wallet/src/ui/state/app_state.rs (1)
681-681: Ensure confirmations aren't under‐reported when using the last scanned height.The change from querying the base node for current tip height to using the wallet's last scanned height could result in incorrect confirmation calculations if the wallet is not fully synced.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
179-192: Fix potential infinite loop in block height search.The loop repeatedly queries the same height while decrementing an unused height variable, which could cause an infinite loop if the base node doesn't know about the next block.
502-510: Address TODO: Remove duplicate block saving logic.The TODO comment correctly identifies that the last scanned block is saved twice, which is redundant since it's already saved within the loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
applications/minotari_console_wallet/src/automation/commands.rs(10 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(7 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(5 hunks)applications/minotari_node/src/config.rs(2 hunks)base_layer/wallet/src/config.rs(4 hunks)base_layer/wallet/src/utxo_scanner_service/initializer.rs(4 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(3 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(13 hunks)base_layer/wallet/src/wallet.rs(7 hunks)base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs(8 hunks)base_layer/wallet_ffi/src/lib.rs(42 hunks)common/src/configuration/bootstrap.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/minotari_node/src/config.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- base_layer/wallet/src/utxo_scanner_service/initializer.rs
- base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs
- base_layer/wallet/src/utxo_scanner_service/service.rs
- applications/minotari_console_wallet/src/automation/commands.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/wallet/src/config.rs (3)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
base_layer/wallet_ffi/src/lib.rs (4)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
applications/minotari_console_wallet/src/wallet_modes.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/wallet/src/wallet.rs (3)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
applications/minotari_console_wallet/src/ui/state/app_state.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (5)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (2)
common/src/configuration/bootstrap.rs (1)
applications/minotari_node/src/config.rs (1)
network(83-85)
base_layer/wallet/src/wallet.rs (5)
base_layer/wallet/src/config.rs (1)
default(134-178)base_layer/wallet/src/utxo_scanner_service/initializer.rs (1)
new(61-76)base_layer/wallet/src/transaction_service/service.rs (1)
new(243-336)base_layer/wallet/src/connectivity_service/handle.rs (1)
new(42-48)base_layer/wallet/src/connectivity_service/initializer.rs (1)
new(43-48)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (24)
common/src/configuration/bootstrap.rs (1)
124-133: LGTM! Well-structured centralized port configuration.The new function follows the established pattern and provides a clean, centralized way to manage default HTTP service ports across different networks. The port assignments are logical and consistent.
base_layer/wallet/src/config.rs (4)
32-32: LGTM! Proper import of centralized port configuration.The import correctly brings in the centralized
wallet_http_service_default_portfunction for consistent port usage.
39-39: LGTM! Adding URL parsing support for HTTP client configuration.The
url::Urlimport is necessary for the new HTTP client URL parsing in the default configuration.
125-131: LGTM! New configuration fields align with HTTP client refactor.The addition of
http_client_urlandscanning_intervalfields properly supports the transition from peer-based connectivity to HTTP client factory approach. The field types and documentation are appropriate.
140-176: LGTM! Well-implemented default configuration for HTTP client.The default configuration correctly:
- Uses the centralized port function for network-specific defaults
- Constructs a valid localhost URL with proper error handling
- Sets a reasonable 60-second scanning interval
- Maintains consistency with existing configuration patterns
applications/minotari_console_wallet/src/wallet_modes.rs (5)
84-110: LGTM! Function signature simplified by removing peer configuration.The removal of the peer configuration parameter from
command_modealigns with the HTTP client factory refactor and simplifies the function interface.
153-202: LGTM! Function signature simplified by removing peer configuration.The removal of the peer configuration parameter from
script_modeis consistent with the broader refactoring and improves code simplicity.
205-249: LGTM! Function signatures cleaned up for HTTP client approach.The
wallet_or_exitandtui_modefunctions are properly updated to remove peer configuration parameters, making the interface cleaner and aligned with the new HTTP client factory approach.
251-251: LGTM! Function signature simplified for HTTP client approach.The
tui_modefunction signature is correctly updated to remove the peer configuration parameter.
334-358: LGTM! Recovery mode properly updated for HTTP client URL.The recovery mode correctly transitions from peer configuration to using the HTTP client URL from wallet configuration. The URL parsing includes proper error handling and the change maintains functionality while simplifying the interface.
base_layer/wallet/src/wallet.rs (5)
142-144: LGTM! Proper generic parameterization for HTTP client factory.The addition of the
THttpClientFactorygeneric parameter with appropriate trait bounds correctly supports the new HTTP client factory architecture while maintaining type safety.Also applies to: 166-174
100-100: LGTM! Proper imports for HTTP client factory support.The addition of
url::UrlandDefaultHttpClientFactoryimports correctly supports the new HTTP client configuration functionality.Also applies to: 104-106
151-151: LGTM! Field type updated for generic HTTP client factory.The
wallet_connectivityfield type is correctly updated to use the genericTHttpClientFactoryparameter.
335-335: LGTM! Handle retrieval updated for generic type parameter.The
expect_handlecall is correctly updated to use the genericWalletConnectivityHandle<THttpClientFactory>type.
401-401: LGTM! Unnecessary string conversion removed.The removal of the redundant
.to_string()call onenv!("CARGO_PKG_VERSION")is a good cleanup since the macro already returns a string literal.base_layer/wallet_ffi/src/lib.rs (5)
84-84: Import changes look appropriate for the HTTP refactor.The new imports for
DefaultHttpClientFactory,HttpClientFactory, and other HTTP-related types are consistent with the refactor objectives.
6995-6995: HTTP client URL configuration is correctly implemented.The
http_client_urlfield is properly set from the parsed HTTP base node parameter.
10380-10380: Verify the function name and return type consistency.The function name
wallet_get_fee_per_gram_stats(plural) suggests it returns multiple statistics, but the return type is nowTariFeePerGramStat(singular). Ensure this change aligns with the intended API.
10746-10746: Cloning by reference is more efficient.The change to clone by reference
(&(*records).0)[index as usize].clone()is a good optimization compared to the previous approach.
11747-11748: Test cases consistently updated with HTTP base node parameter.All test cases have been properly updated to include the new
http_base_nodeparameter with the correct localhost URL.Also applies to: 11801-11802, 11926-11927, 12163-12164, 12236-12237, 12322-12323, 12543-12544, 12689-12690, 12842-12843, 13115-13116, 13397-13398, 13667-13668, 14055-14056
applications/minotari_console_wallet/src/ui/state/app_state.rs (2)
97-99: LGTM! Constructor simplified correctly for HTTP client refactor.The removal of connectivity handle parameters aligns well with the broader refactor from peer-based connectivity to HTTP client factory approach.
412-414: LGTM! Clean HTTP client URL accessor method.This method properly exposes the HTTP client URL from the wallet configuration, supporting the new HTTP-based connectivity architecture.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
68-88: LGTM! Excellent structural improvements for the HTTP client refactor.The new
SyncResultstruct provides a clean return type for scanning operations, and the generic parameters properly abstract the key manager and HTTP client factory dependencies.
515-566: Excellent implementation of key manager-based output recovery.The new
search_for_owned_outputsmethod properly integrates with the key manager to securely decrypt and identify owned outputs. The approach correctly handles both change outputs (using view key) and received outputs (using Diffie-Hellman shared secret), improving both security and performance.
| let http_url = if http_base_node.is_null() { | ||
| *error_out = LibWalletError::from(InterfaceError::NullError("http_base_node".to_string())).code; | ||
| return false; | ||
| } else { | ||
| match CStr::from_ptr(http_base_node).to_str() { | ||
| Ok(v) => match Url::parse(v) { | ||
| Ok(url) => url, | ||
| Err(e) => { | ||
| *error_out = | ||
| LibWalletError::from(InterfaceError::InvalidArgument(format!("Url is not valid: {}", e))).code; | ||
| return false; | ||
| }, | ||
| }, | ||
| _ => { | ||
| *error_out = LibWalletError::from(InterfaceError::PointerError("http_base_node".to_string())).code; | ||
| return false; | ||
| }, | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling quality compared to wallet_create.
The URL parsing in wallet_start_recovery has robust error handling for both UTF-8 conversion and URL validation, while wallet_create uses unsafe expect() calls. Consider standardizing the error handling approach.
Apply similar robust error handling to wallet_create:
let http_base_node = if http_base_node.is_null() {
*error_out = LibWalletError::from(InterfaceError::InvalidArgument("base node url".to_string())).code;
return ptr::null_mut();
} else {
- CStr::from_ptr(http_base_node)
- .to_str()
- .expect("A non-null base node should be able to be converted to string")
- .to_owned()
+ match CStr::from_ptr(http_base_node).to_str() {
+ Ok(v) => match Url::parse(v) {
+ Ok(url) => url.to_string(),
+ Err(e) => {
+ *error_out = LibWalletError::from(InterfaceError::InvalidArgument(
+ format!("Invalid URL: {}", e)
+ )).code;
+ return ptr::null_mut();
+ }
+ },
+ Err(e) => {
+ *error_out = LibWalletError::from(InterfaceError::InvalidArgument(
+ format!("Invalid UTF-8 in http_base_node: {}", e)
+ )).code;
+ return ptr::null_mut();
+ }
+ }
};🤖 Prompt for AI Agents
In base_layer/wallet_ffi/src/lib.rs around lines 10084 to 10102, the URL parsing
in wallet_start_recovery uses safe error handling with proper checks for null
pointers, UTF-8 conversion errors, and URL parsing errors, returning detailed
error codes. To fix the inconsistency, refactor wallet_create to replace unsafe
expect() calls with similar error handling: check for null pointers, handle
UTF-8 conversion errors using to_str() with match, and validate the URL with
Url::parse(), returning appropriate error codes on failure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
174-191: Fix potential infinite loop in block height search.The loop repeatedly queries the same height (
last_scanned_block.height + 1) while decrementing an unusedheightvariable. This could cause an infinite loop if the base node doesn't know about the next block.The issue remains the same as identified in previous review - the loop needs to properly update the query height to ensure progress and eventual termination.
387-512: Address duplicate block saving and improve streaming logic.The scanning logic has been well-refactored to use streaming, but the duplicate block saving issue identified in previous reviews remains.
The TODO comment at lines 503-509 correctly identifies that the last scanned block is saved twice - once within the loop and again outside it.
🧹 Nitpick comments (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
514-565: Fix incomplete timing measurement in output scanning.The key recovery logic is well-implemented, but there's an issue with the timing measurements.
} let scanned_time = start.elapsed(); - let start = Instant::now(); - - let one_sided_time = start.elapsed(); + trace!( target: LOG_TARGET, - "Scanned for outputs: outputs took {} ms , one-sided took {} ms", + "Scanned for outputs: took {} ms", scanned_time.as_millis(), - one_sided_time.as_millis(), );The
one_sided_timemeasurement is meaningless since no work is performed between the secondstartandelapsed()calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(14 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (6)
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (7)
base_layer/wallet/src/transaction_service/service.rs (5)
transaction(1235-1240)transaction(1377-1382)transaction(1438-1443)new(243-336)start(339-612)base_layer/core/src/common/one_sided.rs (1)
shared_secret_to_output_encryption_key(51-58)base_layer/wallet/src/utxo_scanner_service/service.rs (2)
run(115-162)new(69-88)base_layer/wallet/src/output_manager_service/service.rs (2)
new(142-198)start(200-250)base_layer/core/src/transactions/transaction_components/encrypted_data.rs (2)
from_bytes(158-170)decrypt_data(122-155)base_layer/wallet/src/utxo_scanner_service/initializer.rs (1)
new(61-76)base_layer/core/src/transactions/transaction_components/wallet_output.rs (1)
commitment(388-395)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (9)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (9)
68-73: LGTM: Clean result structure for scanning operations.The
SyncResultstruct provides a clear encapsulation of scanning results with all relevant metrics.
75-88: LGTM: Well-structured generic design for HTTP client factory.The updated struct definition cleanly replaces peer-based connectivity with a generic HTTP client factory pattern, improving modularity and testability.
95-132: LGTM: Improved error handling with consistent retry logic.The refactored run method properly uses
anyhow::Errorthroughout and implements a cleaner retry mechanism with exponential backoff via sleep.
221-288: LGTM: Well-structured scanning coordination logic.The
attempt_syncmethod properly orchestrates the scanning process with good separation of concerns and clear progress tracking.
290-304: LGTM: Clean chain tip retrieval with proper error handling.The method safely extracts chain tip information with appropriate defaults for missing metadata.
567-627: LGTM: Comprehensive output recovery with proper error handling.The method correctly handles both regular outputs and one-sided payments with appropriate status classification and robust error handling.
629-682: LGTM: Proper UTXO import with appropriate error handling.The import logic correctly handles different output types and provides proper error handling for duplicate outputs.
684-694: LGTM: Clean recovery mode management.The recovery mode methods properly use the database client key-value store with consistent error handling.
736-787: LGTM: Robust birthday-based scanning start calculation.The method properly calculates scanning start height with good error handling and informative logging.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
applications/minotari_console_wallet/src/ui/state/app_state.rs (1)
651-651: Confirmations may be under-reported when wallet is not fully synced.Using
wallet.db.get_last_scanned_height()instead of the actual chain tip height can result in showing fewer confirmations than actually exist if the wallet is behind in scanning.Consider fetching the actual tip height via HTTP when calculating confirmations:
#!/bin/bash # Description: Check how tip height is used elsewhere in the codebase # Look for other uses of get_last_scanned_height echo "=== Other uses of get_last_scanned_height ===" rg "get_last_scanned_height" -A 3 -B 3 # Check if there's an HTTP method to get tip height echo "=== Check for HTTP tip height methods ===" rg "tip.*height|height.*tip" --type rust -g "*http*" -A 2base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
501-507: Remove duplicate block saving logic.This is the same issue identified in previous reviews. The last scanned block is already saved within the loop, making this redundant.
Apply this fix to remove the duplicate saving:
} // We need to update the last one - if let Some(scanned_block) = prev_scanned_block { - self.resources.db.clear_scanned_blocks_before_height( - scanned_block.height.saturating_sub(SCANNED_BLOCK_CACHE_SIZE), - true, - )?; - self.resources.db.save_scanned_block(scanned_block)?; - } + // Note: Block saving is already handled within the loop above
183-189: Fix infinite loop in block height search.This is the same issue identified in previous reviews. The loop queries a fixed height while decrementing an unused variable, potentially causing an infinite loop.
Apply this fix to use the height variable correctly:
- let mut height = last_scanned_block.height; let mut next_header; // Keep going backwards until we find a header that is known to the base node + let mut query_height = last_scanned_block.height + 1; loop { next_header = wallet_service_client - .get_header_by_height(height + 1) + .get_header_by_height(query_height) .await?; if next_header.is_some() { break; } - height = height.saturating_sub(1); + query_height = query_height.saturating_sub(1); + if query_height == 0 { + return Err(anyhow!("Could not find any known block header")); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
applications/minotari_console_wallet/src/automation/commands.rs(10 hunks)applications/minotari_console_wallet/src/recovery.rs(5 hunks)applications/minotari_console_wallet/src/ui/components/receive_tab.rs(3 hunks)applications/minotari_console_wallet/src/ui/components/register_template_tab.rs(0 hunks)applications/minotari_console_wallet/src/ui/components/send_tab.rs(5 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(7 hunks)applications/minotari_console_wallet/src/ui/state/tasks.rs(1 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(5 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(14 hunks)
💤 Files with no reviewable changes (1)
- applications/minotari_console_wallet/src/ui/components/register_template_tab.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- applications/minotari_console_wallet/src/wallet_modes.rs
- applications/minotari_console_wallet/src/automation/commands.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
applications/minotari_console_wallet/src/ui/state/tasks.rs (4)
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
applications/minotari_console_wallet/src/ui/components/send_tab.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
applications/minotari_console_wallet/src/recovery.rs (4)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (6)
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
applications/minotari_console_wallet/src/ui/state/app_state.rs (4)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (15)
applications/minotari_console_wallet/src/ui/components/receive_tab.rs (1)
32-32: LGTM! UI layout properly adjusted for removed fields.The constraint changes and constant updates correctly reflect the removal of the interactive Tari address display, aligning with the HTTP wallet refactor.
Also applies to: 60-63, 67-67, 74-74, 78-78, 90-90, 94-94, 101-101, 106-106, 113-113
applications/minotari_console_wallet/src/ui/state/tasks.rs (1)
1-410: Transaction sending functionality preserved.Verified that all transaction sending is routed through
send_one_sided_to_stealth_address_transactionand there are no remaining references to the removedsend_transaction_task.• Multiple call sites confirmed (console UI, automation commands, gRPC server, tests, FFI)
• Nosend_transaction_taskusages foundapplications/minotari_console_wallet/src/recovery.rs (2)
25-25: Good simplification of wallet recovery interface.The removal of
base_node_configparameter and more specific imports align well with the HTTP refactor. Thedead_codeallowance is appropriate for CLI-specific functionality.Also applies to: 29-30, 35-36, 85-85
91-97: Well-implemented retry logic for wallet recovery.The addition of a failure counter and retry limit check improves robustness. The error handling provides clear feedback when recovery fails after the specified number of attempts.
Also applies to: 134-134, 148-148, 162-162
applications/minotari_console_wallet/src/ui/components/send_tab.rs (1)
119-121: UI instructions simplified for unified transaction flow.The change to show "S/O" for sending reflects the unification to one-sided transactions only.
applications/minotari_console_wallet/src/ui/state/app_state.rs (2)
53-53: Clean removal of connectivity dependencies.The structural changes properly remove peer management dependencies and add the new HTTP client URL accessor.
Also applies to: 66-66, 97-99, 382-384
528-528: Consistent removal of wallet_identity parameter.The simplification aligns with accessing identity information directly through the wallet instance.
Also applies to: 1087-1087
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (8)
23-65: LGTM: Import changes align with HTTP client refactor.The imports have been appropriately updated to support the new HTTP client-based architecture, removing peer-based dependencies and adding necessary types for the key manager-based scanning approach.
68-74: LGTM: SyncResult struct provides clear result aggregation.The struct appropriately encapsulates synchronization results with clear field names and types.
75-94: LGTM: Struct refactor supports HTTP client factory pattern.The generic type parameters and new key_manager field properly support the HTTP client-based architecture while maintaining type safety.
95-132: LGTM: Simplified run loop with improved error handling.The refactored run method eliminates peer cycling complexity and uses consistent error handling with
anyhow::Error.
512-563: LGTM: Efficient key manager-based output identification.The new scanning logic properly uses the key manager to identify owned outputs before downloading full block data, improving efficiency. The dual approach for change outputs (view key only) and received outputs (DH with sender offset) is correct.
406-412: LGTM: Streaming UTXO sync implementation.The HTTP client streaming approach is well-implemented with proper shutdown signal handling and error management.
288-302: LGTM: Clean chain tip header retrieval.The method properly extracts tip information from the HTTP client response with appropriate defaults.
734-786: LGTM: Robust scanning start header determination.The method correctly handles wallet birthday calculation and provides appropriate error handling for missing headers.
| match self.confirmation_dialog { | ||
| None => (), | ||
| Some(ConfirmationDialogType::Normal) | Some(ConfirmationDialogType::StealthAddress) => { | ||
| Some(ConfirmationDialogType::Normal) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Transaction flow unified to one-sided transactions only.
The removal of normal transaction support is a significant change. All transactions now go through send_one_sided_to_stealth_address_transaction.
Ensure this architectural change is documented in user-facing documentation:
Also applies to: 276-292, 454-454, 533-533
🌐 Web query:
Are one-sided transactions the only transaction type supported in the latest Tari wallet after the HTTP refactor?
💡 Result:
As of the latest updates, the Tari wallet supports multiple transaction types beyond one-sided transactions. The wallet's capabilities are determined by its feature set, which is represented by an encoded value. Each bit in this value denotes a specific feature, allowing for various combinations. For example, a feature set of 0x03 (binary 00000011) indicates support for both one-sided and interactive transactions. (rfc.tari.com)
Therefore, the Tari wallet is not limited to one-sided transactions; it also supports interactive transactions and combinations thereof, depending on the specific feature set enabled.
Citations:
Clarify supported transaction types in Minotari console wallet UI
The console wallet UI now exclusively calls send_one_sided_to_stealth_address_transaction, removing any interactive (“normal”) send flows. Note that the Tari wallet backend still supports interactive transactions via feature-set flags (RFC [1]), but this isn’t exposed here. Please update user-facing documentation to:
- State that the Minotari console wallet only supports one-sided stealth transactions
- Clarify that interactive transactions remain available in the backend (e.g. feature set
0x03) - Outline any roadmap for adding interactive sends to this UI
Affected locations:
- applications/minotari_console_wallet/src/ui/components/send_tab.rs: lines 251, 276–292, 454, 533
[Citation]
- https://rfc.tari.com/print?utm_source=openai [rfc.tari.com]
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/ui/components/send_tab.rs around
lines 251, 276–292, 454, and 533, update all user-facing documentation and UI
text to clearly state that the Minotari console wallet only supports one-sided
stealth transactions via send_one_sided_to_stealth_address_transaction. Also,
add clarifications that interactive ("normal") transactions are still supported
by the Tari wallet backend through feature-set flags (e.g., 0x03) but are not
exposed in this UI. Finally, include any relevant roadmap notes about potential
future support for interactive sends in this UI.
|
Following a system-level testing approach, the only real issue I could find related specifically to the http wallet was that wallet recovery could not be completed due to a unique header hash constraint violation - this was fixed in a previous commit. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (1)
256-1406: All transaction protocol tests are currently disabledAll
tx_broadcast_protocol_*andtx_validation_protocol_*test bodies have been fully commented out, leaving no active coverage for the wallet’s core transaction broadcast and validation logic.• Affected file:
‑base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs(lines 256–1406) – every#[tokio::test]block is commented out.
• No other active tests invokingTransactionBroadcastProtocolorTransactionValidationProtocolwere found under*/tests/*.To restore confidence and coverage:
- Re-implement these protocol tests using the new HTTP‐based connectivity layer and mock server endpoints.
- Replace the existing RPC mock patterns with HTTP request/response mocks.
- Update assertions to reflect any changes in event publishing or error handling in the revised connectivity model.
- Ensure all branches (submission success, rejection, restart, reorg, etc.) are covered.
♻️ Duplicate comments (5)
base_layer/wallet/tests/output_manager_service_tests/service.rs (2)
106-106: Remove unused parameter_with_connection.The
_with_connectionparameter is no longer used after the migration to HTTP client mocks.- _with_connection: bool,
167-167: Remove unused parameter_height.The
_heightparameter is no longer used after removing the RPC mock setup.- _height: Option<u64>,base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (1)
202-238: Remove blanket dead code suppression and clean up unused functions.As noted in previous reviews, the blanket
#[allow(dead_code)]should be removed. Theadd_transaction_to_databasefunction appears genuinely unused since all test bodies are commented out and should be deleted.Apply this diff to remove unused code:
-// Allow dead code while the tests are commented out -#[allow(dead_code)] -pub async fn add_transaction_to_database( - tx_id: TxId, - amount: MicroMinotari, - status: Option<TransactionStatus>, - db: TransactionDatabase<TransactionServiceSqliteDatabase>, -) { - let key_manager_handle = create_memory_db_key_manager().unwrap(); - let uo0 = make_input(&mut OsRng, 10 * amount, &OutputFeatures::default(), &key_manager_handle).await; - let (txs1, _uou1) = schema_to_transaction( - &[txn_schema!(from: vec![uo0.clone()], to: vec![amount])], - &key_manager_handle, - ) - .await; - let tx1 = (*txs1[0]).clone(); - let completed_tx1 = CompletedTransaction::new( - tx_id, - TariAddress::default(), - TariAddress::default(), - amount, - 200 * uT, - tx1, - status.unwrap_or(TransactionStatus::Completed), - Utc::now(), - TransactionDirection::Outbound, - None, - None, - PaymentId::open_from_string("Test", TxType::PaymentToOther), - ) - .unwrap(); - db.insert_completed_transaction(tx_id, completed_tx1).unwrap(); -} - -/// Simple task that responds with a OutputManagerResponse::TransactionCancelled response to any request made on this -/// channel -#[allow(dead_code)]Keep
oms_reply_channel_taskas it's actively used in the setup function.base_layer/wallet_ffi/src/lib.rs (2)
6880-6888: Fix unsafe error handling in wallet_create function.The current implementation uses
expect()which will panic on invalid UTF-8, making the FFI unsafe. This is inconsistent with the robust error handling used inwallet_start_recovery.Apply this diff to use safe error handling:
let http_base_node = if http_base_node.is_null() { - *error_out = LibWalletError::from(InterfaceError::InvalidArgument("base node url".to_string())).code; - return ptr::null_mut(); + *error_out = LibWalletError::from(InterfaceError::NullError("http_base_node".to_string())).code; + return ptr::null_mut(); } else { - CStr::from_ptr(http_base_node) - .to_str() - .expect("A non-null base node should be able to be converted to string") - .to_owned() + match CStr::from_ptr(http_base_node).to_str() { + Ok(v) => match Url::parse(v) { + Ok(url) => url.to_string(), + Err(e) => { + *error_out = LibWalletError::from(InterfaceError::InvalidArgument( + format!("Invalid URL: {}", e) + )).code; + return ptr::null_mut(); + } + }, + Err(e) => { + *error_out = LibWalletError::from(InterfaceError::InvalidArgument( + format!("Invalid UTF-8 in http_base_node: {}", e) + )).code; + return ptr::null_mut(); + } + } };
14120-14121: Fix typo in HTTP URL.The URL is missing an octet in the IP address.
Apply this diff to fix the typo:
-let http_base_node_address: *const c_char = - CString::new("http://127.0.1:2222").unwrap().into_raw() as *const c_char; +let http_base_node_address: *const c_char = + CString::new("http://127.0.0.1:2222").unwrap().into_raw() as *const c_char;
🧹 Nitpick comments (2)
base_layer/wallet/tests/output_manager_service_tests/service.rs (2)
1307-1884: Address commented-out test coverage.The
test_txo_validationtest represents significant test coverage that has been disabled. This test should be reimplemented using HTTP mocks to maintain testing of transaction output validation functionality.This large integration test covers critical wallet functionality including UTXO validation, balance management, and blockchain state handling. Would you like me to help design the HTTP mock-based replacement for this test to restore the lost coverage?
1889-2084: Address commented-out revalidation test.The
test_txo_revalidationtest is also disabled, representing another gap in test coverage for output revalidation scenarios including blockchain reorganizations.This test covers important revalidation logic for handling blockchain reorgs and output status changes. Should this test also be reimplemented with HTTP mocks to restore comprehensive test coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
applications/minotari_console_wallet/src/wallet_modes.rs(5 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(2 hunks)base_layer/wallet/tests/output_manager_service_tests/service.rs(8 hunks)base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs(8 hunks)base_layer/wallet_ffi/src/ffi_basenode_state.rs(0 hunks)base_layer/wallet_ffi/src/lib.rs(42 hunks)
💤 Files with no reviewable changes (1)
- base_layer/wallet_ffi/src/ffi_basenode_state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
applications/minotari_console_wallet/src/wallet_modes.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/wallet_ffi/src/lib.rs (5)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of tokio::task::spawn(async move { with tokio::task::spawn_blocking().await in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
base_layer/wallet/tests/output_manager_service_tests/service.rs (5)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (6)
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.039Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
🔇 Additional comments (20)
applications/minotari_console_wallet/src/wallet_modes.rs (6)
109-109: LGTM! Function call updated correctly.The
wallet_or_exitfunction call has been properly updated to remove the peer configuration parameter, aligning with the refactoring to HTTP client-based connectivity.
201-201: LGTM! Consistent with peer config removal.The function call matches the updated signature without peer configuration.
215-215: LGTM! TUI mode calls updated correctly.Both
tui_modefunction calls have been properly updated to remove peer configuration parameters, maintaining consistency across the refactoring.Also applies to: 244-244
251-251: LGTM! Function signature correctly updated.The
tui_modefunction signature has been properly updated to remove peer configuration, consistent with the HTTP client-based refactoring.
358-358: LGTM! Recovery TUI mode call updated correctly.The function call has been properly updated to match the new signature without peer configuration.
334-336: http_client_url Is Properly Defaulted and Loaded
The console wallet’sWalletConfig::load_from(cfg)(applications/minotari_console_wallet/src/config.rs:44) already populateshttp_client_urlwith a sensible default (http://127.0.0.1:<base_node_port>, see base_layer/wallet/src/config.rs). No additional action is needed unless you intend to override the base node URL manually.base_layer/wallet/tests/output_manager_service_tests/service.rs (2)
23-23: LGTM! Import updates align with HTTP client refactoring.The imports have been properly updated to use the new
WalletConnectivityHandleandMockHttpClientFactoryinstead of peer-based connectivity.Also applies to: 27-27, 31-31, 83-83
95-95: LGTM! Test structure updated for HTTP client mocks.The test setup has been properly migrated from peer-based RPC mocks to HTTP client factory mocks, maintaining the same testing patterns with the new connectivity approach.
Also applies to: 124-124, 157-157, 192-192
base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (1)
89-101: Approve the connectivity refactor direction.The setup function correctly transitions from RPC-based mocking to HTTP client factory approach, which aligns with the broader wallet connectivity refactor objectives.
base_layer/wallet_ffi/src/lib.rs (11)
84-85: LGTM! HTTP client factory integration looks correct.The new imports for
DefaultHttpClientFactoryandHttpClientFactoryare properly added to support the HTTP-based wallet refactor.
113-114: LGTM! WalletKeyManager import added correctly.The
WalletKeyManagerimport is properly added to support the refactored wallet functionality.
117-117: LGTM! Additional imports for HTTP functionality.The
rand::rngs::OsRngandurl::Urlimports are correctly added to support the HTTP client functionality and URL parsing.Also applies to: 195-195
6731-6731: LGTM! Parameter signature updated for HTTP base node.The function signature correctly adds the
http_base_nodeparameter to replace peer-based configuration.
6995-6995: LGTM! HTTP client URL configuration updated.The wallet configuration correctly uses the
http_base_nodeparameter for the HTTP client URL.
10047-10047: LGTM! Recovery function parameter updated.The
wallet_start_recoveryfunction correctly adds thehttp_base_nodeparameter.
10070-10071: LGTM! HTTP client factory integration in recovery.The recovery task builder correctly uses the
DefaultHttpClientFactorywith theWalletKeyManagertype.
10084-10102: LGTM! Excellent error handling in recovery function.The HTTP URL parsing and validation in
wallet_start_recoverydemonstrates robust error handling with proper UTF-8 validation and URL parsing checks. This is the correct pattern that should be applied towallet_createas well.
10105-10105: LGTM! HTTP client factory properly initialized.The recovery task builder correctly initializes the
DefaultHttpClientFactorywith the parsed HTTP URL.
10380-10380: LGTM! Return type simplified for fee stats.The function return type has been correctly changed from multiple fee stats to a single
TariFeePerGramStatpointer, aligning with the simplified fee stats handling.Also applies to: 10395-10395
11747-11748: LGTM! Test cases updated with correct HTTP URLs.All test cases have been properly updated to use HTTP base node addresses instead of peer configurations. The localhost URLs are correctly formatted.
Also applies to: 11801-11802, 11926-11927, 12163-12164, 12236-12237, 12322-12323, 12543-12544, 12689-12690, 12842-12843, 13115-13116, 13397-13398, 13667-13668, 14049-14050
4 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Removals
Documentation
Chores