feat: add option to do search via payment id #6993
Conversation
WalkthroughThis update introduces support for user-defined payment identifiers across the wallet system. The gRPC API for balance and completed transactions queries now allows specifying a payment ID in multiple formats. Backend services, database schema, and migration scripts have been updated to store and filter balances and transactions by this identifier. The wallet's output manager and transaction storage layers have been extended to handle the new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletGrpcServer
participant OutputManagerHandle
participant OutputManagerService
participant Database
Client->>WalletGrpcServer: get_balance(request: GetBalanceRequest)
alt payment_id provided
WalletGrpcServer->>OutputManagerHandle: get_balance_for_payment_id(payment_id)
OutputManagerHandle->>OutputManagerService: GetBalancePaymentId(payment_id)
OutputManagerService->>Database: get_balance_payment_id(tip, payment_id)
Database-->>OutputManagerService: Balance
OutputManagerService-->>OutputManagerHandle: Balance
OutputManagerHandle-->>WalletGrpcServer: Balance
WalletGrpcServer-->>Client: GetBalanceResponse (filtered)
else no payment_id
WalletGrpcServer->>OutputManagerHandle: get_balance()
OutputManagerHandle->>OutputManagerService: GetBalance
OutputManagerService->>Database: get_balance(tip)
Database-->>OutputManagerService: Balance
OutputManagerService-->>OutputManagerHandle: Balance
OutputManagerHandle-->>WalletGrpcServer: Balance
WalletGrpcServer-->>Client: GetBalanceResponse (overall)
end
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
base_layer/wallet/src/output_manager_service/storage/database/mod.rs (1)
187-192: The new method looks good, but needs a small formatting fix.The implementation correctly delegates to the backend implementation, allowing balance queries filtered by payment ID.
- pub fn get_balance_payment_id( - &self, - current_tip_for_time_lock_calculation: Option<u64>, payment_id : Vec<u8> - ) -> Result<Balance, OutputManagerStorageError> { + pub fn get_balance_payment_id( + &self, + current_tip_for_time_lock_calculation: Option<u64>, + payment_id: Vec<u8> + ) -> Result<Balance, OutputManagerStorageError> {base_layer/wallet/migrations/2025-04-25-161400_payment_id/up.sql (1)
1-5: Remove unrelated comment from the migration file.The comment on lines 1-2 refers to dropping and recreating a table due to removal of columns and default values, but this migration is only adding columns.
--- Any old 'outputs' will not be valid due to the removal of 'coinbase_block_height' and removal of default value for --- 'spending_priority', so we drop and recreate the table. ALTER TABLE outputs ADD user_payment_id BLOB NULL;base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs (1)
968-988: Fix the log message to match the method name.The implementation correctly queries balance by payment ID, but the log message still uses "get_balance" instead of "get_balance_payment_id".
let result = OutputSql::get_balance_payment_id(current_tip_for_time_lock_calculation, payment_id, &mut conn); if start.elapsed().as_millis() > 0 { trace!( target: LOG_TARGET, - "sqlite profile - get_balance: lock {} + db_op {} = {} ms", + "sqlite profile - get_balance_payment_id: lock {} + db_op {} = {} ms", acquire_lock.as_millis(), (start.elapsed() - acquire_lock).as_millis(), start.elapsed().as_millis() ); }applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
313-327: Consider refactoring the payment ID extraction logic.The nested match expression works but could be more readable with a more direct approach to extracting the payment ID.
- let bytes = match (user_payment_id.u256.is_empty(), user_payment_id.utf8_string.is_empty(), user_payment_id.user_bytes.is_empty()){ - (false, true, true) => { - user_payment_id.u256 - }, - (true, false, true) => { - user_payment_id.utf8_string.as_bytes().to_vec() - }, - (true, true, false) => { - user_payment_id.user_bytes - }, - _ => { - return Err(Status::invalid_argument("user_payment_id must be one of u256, utf8_string or user_bytes".to_string())); - } - }; + // Count how many payment ID formats are provided + let mut provided_formats = 0; + let mut bytes = Vec::new(); + + if !user_payment_id.u256.is_empty() { + provided_formats += 1; + bytes = user_payment_id.u256; + } + + if !user_payment_id.utf8_string.is_empty() { + provided_formats += 1; + bytes = user_payment_id.utf8_string.as_bytes().to_vec(); + } + + if !user_payment_id.user_bytes.is_empty() { + provided_formats += 1; + bytes = user_payment_id.user_bytes; + } + + if provided_formats != 1 { + return Err(Status::invalid_argument("user_payment_id must be provided in exactly one format: u256, utf8_string, or user_bytes".to_string())); + }base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (2)
530-534: Fix spacing in method signature.There's a missing space after the colon in the parameter declaration.
- pub fn get_balance_payment_id( - current_tip_for_time_lock_calculation: Option<u64>, - payment_id:Vec<u8>, - conn: &mut SqliteConnection, - ) -> Result<Balance, OutputManagerStorageError> { + pub fn get_balance_payment_id( + current_tip_for_time_lock_calculation: Option<u64>, + payment_id: Vec<u8>, + conn: &mut SqliteConnection, + ) -> Result<Balance, OutputManagerStorageError> {
528-645: Consider refactoring to reduce code duplication with get_balance method.The
get_balance_payment_idmethod is nearly identical to the existingget_balancemethod, with the main difference being the additional filtering byuser_payment_id. This duplication could lead to maintenance issues if one method is updated but the other is not.Consider refactoring both methods to use a common helper function that accepts an optional payment ID parameter, reducing duplication and ensuring consistent handling.
// Add a new private helper method + fn get_balance_internal( + current_tip_for_time_lock_calculation: Option<u64>, + payment_id: Option<Vec<u8>>, + conn: &mut SqliteConnection, + ) -> Result<Balance, OutputManagerStorageError> { + // Implementation that handles both cases + } + + pub fn get_balance( + current_tip_for_time_lock_calculation: Option<u64>, + conn: &mut SqliteConnection, + ) -> Result<Balance, OutputManagerStorageError> { + Self::get_balance_internal(current_tip_for_time_lock_calculation, None, conn) + } + + pub fn get_balance_payment_id( + current_tip_for_time_lock_calculation: Option<u64>, + payment_id: Vec<u8>, + conn: &mut SqliteConnection, + ) -> Result<Balance, OutputManagerStorageError> { + Self::get_balance_internal(current_tip_for_time_lock_calculation, Some(payment_id), conn) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_console_wallet/Cargo.toml(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)base_layer/wallet/migrations/2025-04-25-161400_payment_id/down.sql(1 hunks)base_layer/wallet/migrations/2025-04-25-161400_payment_id/up.sql(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(3 hunks)base_layer/wallet/src/output_manager_service/service.rs(2 hunks)base_layer/wallet/src/output_manager_service/storage/database/backend.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/database/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs(3 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs(2 hunks)base_layer/wallet/src/schema.rs(4 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
base_layer/wallet/src/output_manager_service/storage/database/mod.rs (4)
base_layer/wallet/src/output_manager_service/storage/database/backend.rs (1)
get_balance_payment_id(91-91)base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs (1)
get_balance_payment_id(968-988)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (1)
get_balance_payment_id(530-645)base_layer/wallet/src/output_manager_service/service.rs (1)
get_balance_payment_id(816-820)
base_layer/wallet/src/output_manager_service/service.rs (4)
base_layer/wallet/src/output_manager_service/storage/database/backend.rs (1)
get_balance_payment_id(91-91)base_layer/wallet/src/output_manager_service/storage/database/mod.rs (1)
get_balance_payment_id(187-192)base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs (1)
get_balance_payment_id(968-988)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (1)
get_balance_payment_id(530-645)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (22)
applications/minotari_console_wallet/Cargo.toml (1)
83-83: LGTM: gRPC feature enabled by default.Adding "grpc" to the default feature set is appropriate for supporting the new payment ID search functionality.
base_layer/wallet/src/output_manager_service/storage/database/backend.rs (1)
90-91: LGTM: Well-designed method for payment ID filtering.The new method extends the balance retrieval functionality in a clean way by maintaining a consistent interface with the existing
get_balancemethod while adding payment ID filtering capability.applications/minotari_app_grpc/proto/wallet.proto (1)
258-260: LGTM: GetBalanceRequest updated to support payment ID filtering.The GetBalanceRequest message now includes a payment_id field, enabling clients to request balances filtered by a specific identifier.
base_layer/wallet/src/output_manager_service/service.rs (2)
337-344: Handler for new GetBalancePaymentId request looks good.The implementation correctly follows the same pattern as the existing GetBalance handler, retrieving chain metadata from the base node service and passing the payment_id parameter to the new get_balance_payment_id method.
816-820: Implementation of get_balance_payment_id method is well structured.The method correctly retrieves the balance from the database using the payment_id filter, logs it at the trace level, and returns the result. The implementation mirrors the existing get_balance method, maintaining consistency in the codebase.
base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs (3)
69-69: New user_payment_id field added to NewOutputSql struct.The field is correctly defined as Option<Vec> to match the schema definition, allowing for nullable user payment IDs.
82-88: Proper extraction of user_payment_id from payment_id.The implementation correctly extracts the user data from the payment_id and handles empty data appropriately by setting to None when empty, which is storage-efficient.
124-124: Struct initialization includes the new user_payment_id field.The field is consistently included in the struct initialization, completing the implementation of the new field.
base_layer/wallet/src/schema.rs (1)
40-40: Schema updated consistently across all relevant tables.The nullable binary column
user_payment_idhas been added to all necessary tables (completed_transactions, inbound_transactions, outbound_transactions, and outputs) with consistent type definitions, enabling proper filtering of balances by payment ID.Also applies to: 56-56, 83-83, 124-124
base_layer/wallet/src/output_manager_service/handle.rs (3)
64-64: Feature implementation looks good.This new variant in the
OutputManagerRequestenum adds the capability to filter balance queries by payment ID, which is exactly what the PR aims to achieve.
169-169: Implementation of Display trait is appropriate.The formatting for the new enum variant provides sufficient information without exposing potentially sensitive payment ID data.
502-507: New method is well implemented.The
get_balance_for_payment_idmethod follows the same pattern as other methods in this struct, with proper error handling and async/await flow. Good implementation that integrates seamlessly with the existing codebase.base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (8)
1155-1155: New field added correctly.Adding
user_payment_idas an optional field to theInboundTransactionSqlstruct properly supports the new feature.
1331-1336: Good handling of empty payment IDs.The code correctly extracts user data from payment IDs and handles empty values by converting them to
None, which will be stored as NULL in the database. This improves storage efficiency.
1348-1348: Field initialized properly.The extracted
user_payment_idvalue is correctly assigned to the struct field.
1588-1593: Consistent implementation for OutboundTransactionSql.The same user payment ID extraction logic is applied consistently here, matching the implementation in
InboundTransactionSql.
1606-1606: Field assigned correctly.The
user_payment_idvalue is properly included in the struct initialization.
1701-1701: New field added to CompletedTransactionSql.The
user_payment_idfield is consistently added to this struct, maintaining parity with the other transaction SQL structs.
1968-1973: Consistent extraction logic applied.The same user payment ID extraction logic is applied here, ensuring consistent behavior across all transaction types.
1994-1994: Field initialized properly in constructor.The
user_payment_idis correctly assigned in the struct initialization.applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
310-338: Implement payment ID filtering in balance retrieval.The implementation correctly adds support for retrieving balances filtered by payment ID, handling multiple input formats appropriately. The error handling for invalid arguments is clear and informative.
A few observations about the implementation:
- It correctly validates that only one of the three payment ID formats is provided
- It properly converts between different representations (bytes, strings)
- It returns appropriate error status codes for different failure scenarios
- The method gracefully falls back to the original behavior when no payment ID is provided
base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (1)
113-113: Add user_payment_id field to OutputSql struct.The addition of an optional field to store user-defined payment IDs is appropriate. Using
Option<Vec<u8>>allows for flexibility in the payment ID format and ensures compatibility with various consumer implementations.
| @@ -0,0 +1 @@ | |||
| -- This file should undo anything in `up.sql` | |||
There was a problem hiding this comment.
Incomplete migration rollback script.
The down.sql file contains only a placeholder comment without the necessary SQL statements to undo the schema changes made in the corresponding up.sql migration.
Based on the AI summary, you need to add statements to drop the user_payment_id columns:
-- This file should undo anything in `up.sql`
+ALTER TABLE outputs DROP COLUMN user_payment_id;
+ALTER TABLE completed_transactions DROP COLUMN user_payment_id;
+ALTER TABLE inbound_transactions DROP COLUMN user_payment_id;
+ALTER TABLE outbound_transactions DROP COLUMN user_payment_id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- This file should undo anything in `up.sql` | |
| -- This file should undo anything in `up.sql` | |
| ALTER TABLE outputs DROP COLUMN user_payment_id; | |
| ALTER TABLE completed_transactions DROP COLUMN user_payment_id; | |
| ALTER TABLE inbound_transactions DROP COLUMN user_payment_id; | |
| ALTER TABLE outbound_transactions DROP COLUMN user_payment_id; |
| message user_payment_id { | ||
| bytes u256 = 1; | ||
| string utf8_string = 2; | ||
| bytes user_bytes = 3; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the user_payment_id message using oneof and follow naming conventions.
The current implementation allows clients to potentially provide multiple payment ID formats simultaneously, which could lead to ambiguity. Also, the message name doesn't follow protobuf naming conventions.
Apply these improvements:
-message user_payment_id {
- bytes u256 = 1;
- string utf8_string = 2;
- bytes user_bytes = 3;
+message UserPaymentId {
+ oneof id_format {
+ bytes u256 = 1;
+ string utf8_string = 2;
+ bytes user_bytes = 3;
+ }
}Also update the reference:
- user_payment_id payment_id = 1;
+ UserPaymentId payment_id = 1;Using oneof ensures exactly one format is provided, eliminating potential ambiguity in client requests.
| "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ | ||
| FROM outputs WHERE status = ? AND user_payment_id = ?\ | ||
| UNION ALL \ | ||
| SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ | ||
| FROM outputs WHERE source != ? AND status = ? OR status = ? OR status = ? AND user_payment_id = ? \ | ||
| UNION ALL \ | ||
| SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ | ||
| FROM outputs WHERE status = ? OR status = ? OR status = ? AND user_payment_id = ?", | ||
| ) |
There was a problem hiding this comment.
Fix SQL query WHERE clause grouping for non-time-locked balances.
Similar to the previous issue, the SQL query without time lock calculations also has incorrect placement of the AND user_payment_id = ? condition.
SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \
-FROM outputs WHERE source != ? AND status = ? OR status = ? OR status = ? AND user_payment_id = ? \
+FROM outputs WHERE source != ? AND (status = ? OR status = ? OR status = ?) AND user_payment_id = ? \
UNION ALL \
SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \
-FROM outputs WHERE status = ? OR status = ? OR status = ? AND user_payment_id = ?",
+FROM outputs WHERE (status = ? OR status = ? OR status = ?) AND user_payment_id = ?",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ | |
| FROM outputs WHERE status = ? AND user_payment_id = ?\ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ | |
| FROM outputs WHERE source != ? AND status = ? OR status = ? OR status = ? AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ | |
| FROM outputs WHERE status = ? OR status = ? OR status = ? AND user_payment_id = ?", | |
| ) | |
| "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ | |
| FROM outputs WHERE status = ? AND user_payment_id = ?\ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ | |
| FROM outputs WHERE source != ? AND (status = ? OR status = ? OR status = ?) AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ | |
| FROM outputs WHERE (status = ? OR status = ? OR status = ?) AND user_payment_id = ?", |
| "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ | ||
| FROM outputs WHERE status = ? AND maturity <= ? AND script_lock_height <= ? AND user_payment_id = ? \ | ||
| UNION ALL \ | ||
| SELECT coalesce(sum(value), 0) as amount, 'time_locked_balance' as category \ | ||
| FROM outputs WHERE status = ? AND maturity > ? OR script_lock_height > ? AND user_payment_id = ? \ | ||
| UNION ALL \ | ||
| SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ | ||
| FROM outputs WHERE source != ? AND status = ? OR status = ? OR status = ? AND user_payment_id = ? \ | ||
| UNION ALL \ | ||
| SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ | ||
| FROM outputs WHERE status = ? OR status = ? OR status = ? AND user_payment_id = ?", |
There was a problem hiding this comment.
Fix SQL query WHERE clause grouping for time-locked balances.
The SQL query has incorrect placement of the AND user_payment_id = ? condition in relation to the OR conditions, which could lead to unexpected results when filtering outputs.
The condition AND user_payment_id = ? at the end of each query section will only apply to the last condition in the OR chain, not to the entire group of conditions. This could result in retrieving balances that don't match the payment ID.
SELECT coalesce(sum(value), 0) as amount, 'time_locked_balance' as category \
-FROM outputs WHERE status = ? AND maturity > ? OR script_lock_height > ? AND user_payment_id = ? \
+FROM outputs WHERE status = ? AND ((maturity > ? OR script_lock_height > ?) AND user_payment_id = ?) \
UNION ALL \
SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \
-FROM outputs WHERE source != ? AND status = ? OR status = ? OR status = ? AND user_payment_id = ? \
+FROM outputs WHERE source != ? AND (status = ? OR status = ? OR status = ?) AND user_payment_id = ? \
UNION ALL \
SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \
-FROM outputs WHERE status = ? OR status = ? OR status = ? AND user_payment_id = ?",
+FROM outputs WHERE (status = ? OR status = ? OR status = ?) AND user_payment_id = ?",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ | |
| FROM outputs WHERE status = ? AND maturity <= ? AND script_lock_height <= ? AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'time_locked_balance' as category \ | |
| FROM outputs WHERE status = ? AND maturity > ? OR script_lock_height > ? AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ | |
| FROM outputs WHERE source != ? AND status = ? OR status = ? OR status = ? AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ | |
| FROM outputs WHERE status = ? OR status = ? OR status = ? AND user_payment_id = ?", | |
| "SELECT coalesce(sum(value), 0) as amount, 'available_balance' as category \ | |
| FROM outputs WHERE status = ? AND maturity <= ? AND script_lock_height <= ? AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'time_locked_balance' as category \ | |
| FROM outputs WHERE status = ? AND ((maturity > ? OR script_lock_height > ?) AND user_payment_id = ?) \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_incoming_balance' as category \ | |
| FROM outputs WHERE source != ? AND (status = ? OR status = ? OR status = ?) AND user_payment_id = ? \ | |
| UNION ALL \ | |
| SELECT coalesce(sum(value), 0) as amount, 'pending_outgoing_balance' as category \ | |
| FROM outputs WHERE (status = ? OR status = ? OR status = ?) AND user_payment_id = ?", |
Test Results (CI)0 tests 0 ✅ 0s ⏱️ Results for commit c8829ba. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)0 tests 0 ✅ 0s ⏱️ Results for commit c8829ba. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
base_layer/wallet_ffi/src/lib.rs (1)
8211-8638: Consider extending FFI layer to expose payment_id filtering functionality.All calls to
get_completed_transactionshave been updated with theNoneparameter to indicate no payment ID filtering. However, the FFI layer doesn't seem to expose the new payment ID filtering functionality to callers.If this is intentional (perhaps planned for a future update), that's fine. Otherwise, consider updating the FFI function signatures to accept an optional payment ID parameter and pass it through to the transaction service.
base_layer/wallet/src/transaction_service/handle.rs (1)
82-970: Consider documenting the payment ID parameterWhile the implementation is correct, it would be helpful to add documentation comments explaining the payment ID parameter's purpose and expected format for better developer experience.
- GetCompletedTransactions(Option<Vec<u8>>), + /// GetCompletedTransactions retrieves all completed transactions + /// optionally filtered by a user payment ID + GetCompletedTransactions(Option<Vec<u8>>),And for the method:
- pub async fn get_completed_transactions( - &mut self, - payment_id: Option<Vec<u8>>, - ) -> Result<Vec<CompletedTransaction>, TransactionServiceError> { + /// Retrieves completed transactions from the transaction service + /// + /// # Parameters + /// * `payment_id` - Optional byte vector representing a payment ID to filter transactions + /// + /// # Returns + /// A vector of completed transactions, optionally filtered by payment ID + pub async fn get_completed_transactions( + &mut self, + payment_id: Option<Vec<u8>>, + ) -> Result<Vec<CompletedTransaction>, TransactionServiceError> {base_layer/wallet/src/transaction_service/storage/database.rs (2)
629-656: Consider parameter order consistencyThe method signature changed from:
fn get_completed_transactions_by_cancelled(&self, cancelled: bool)to:
fn get_completed_transactions_by_cancelled(&self, payment_id: Option<Vec<u8>>, cancelled: bool)It's generally better to append new optional parameters rather than inserting them before existing ones to maintain a more intuitive API evolution.
Consider refactoring to:
-fn get_completed_transactions_by_cancelled( - &self, - payment_id: Option<Vec<u8>>, - cancelled: bool, -) -> Result<Vec<CompletedTransaction>, TransactionStorageError> { +fn get_completed_transactions_by_cancelled( + &self, + cancelled: bool, + payment_id: Option<Vec<u8>>, +) -> Result<Vec<CompletedTransaction>, TransactionStorageError> {And update all callers accordingly.
159-162: Add logging for payment ID filteringFor better observability and debugging, consider adding logging when filtering by payment ID.
pub fn get_completed_transactions_by_payment_id( &self, payment_id: Vec<u8>, ) -> Result<Vec<CompletedTransaction>, TransactionStorageError> { + debug!( + target: LOG_TARGET, + "Retrieving completed transactions filtered by payment ID" + ); let t = self.db.find_completed_transactions_for_payment_id(payment_id)?; Ok(t) }Also applies to: 485-491
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(2 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(3 hunks)base_layer/wallet/src/output_manager_service/service.rs(2 hunks)base_layer/wallet/src/output_manager_service/storage/database/backend.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/database/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs(3 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs(2 hunks)base_layer/wallet/src/transaction_service/handle.rs(3 hunks)base_layer/wallet/src/transaction_service/service.rs(1 hunks)base_layer/wallet/src/transaction_service/storage/database.rs(4 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(10 hunks)base_layer/wallet_ffi/src/lib.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/minotari_console_wallet/src/ui/state/app_state.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- base_layer/wallet/src/output_manager_service/storage/database/mod.rs
- base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs
- base_layer/wallet/src/output_manager_service/handle.rs
- base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs
- applications/minotari_app_grpc/proto/wallet.proto
- base_layer/wallet/src/output_manager_service/service.rs
- base_layer/wallet/src/output_manager_service/storage/database/backend.rs
- base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
- base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (14)
base_layer/wallet/src/transaction_service/service.rs (1)
893-895: Implementation of payment ID filtering for completed transactions looks good.The code correctly modifies the
GetCompletedTransactionsrequest handler to accept and pass apayment_idparameter to the database query, enabling filtering of completed transactions by payment identifier as intended.base_layer/wallet_ffi/src/lib.rs (6)
8211-8211: Updated API call to include payment_id parameter.This change adapts the call to the updated
get_completed_transactionsmethod signature which now accepts an optional payment ID filter parameter. TheNonevalue indicates that no payment ID filtering is applied in this context.
8279-8279: Updated API call to include payment_id parameter.This change adapts the call to the updated
get_completed_transactionsmethod signature which now accepts an optional payment ID filter parameter. TheNonevalue indicates that no payment ID filtering is applied in this context.
8349-8349: Updated API call to include payment_id parameter.This change adapts the call to the updated
get_completed_transactionsmethod signature which now accepts an optional payment ID filter parameter. TheNonevalue indicates that no payment ID filtering is applied in this context.
8504-8504: Updated API call to include payment_id parameter.This change adapts the call to the updated
get_completed_transactionsmethod signature which now accepts an optional payment ID filter parameter. TheNonevalue indicates that no payment ID filtering is applied in this context.
8565-8565: Updated API call to include payment_id parameter.This change adapts the call to the updated
get_completed_transactionsmethod signature which now accepts an optional payment ID filter parameter. TheNonevalue indicates that no payment ID filtering is applied in this context.
8638-8638: Updated API call to include payment_id parameter.This change adapts the call to the updated
get_completed_transactionsmethod signature which now accepts an optional payment ID filter parameter. TheNonevalue indicates that no payment ID filtering is applied in this context.base_layer/wallet/src/transaction_service/handle.rs (3)
82-82: Added payment ID filter to GetCompletedTransactionsThe enum variant has been updated to include an optional payment ID parameter (
Option<Vec<u8>>), enabling transaction filtering by user-defined payment identifiers.
211-211: LGTM for Display implementation updateThe Display trait implementation was appropriately updated to handle the new parameter, maintaining a clean string representation without exposing the filter details.
963-970: LGTM for method signature and implementation updateThe
get_completed_transactionsmethod signature and implementation were updated to accept an optional payment ID parameter and pass it through to the service request correctly.base_layer/wallet/src/transaction_service/storage/database.rs (4)
159-162: LGTM for new TransactionBackend trait methodThe new method
find_completed_transactions_for_payment_idextends the backend trait with payment ID filtering capability.
485-491: LGTM for additional helper methodThe new
get_completed_transactions_by_payment_idmethod provides a clean API to retrieve transactions filtered by payment ID.
599-604: LGTM for updated method signaturesThe
get_completed_transactionsandget_cancelled_completed_transactionsmethods were correctly updated to support the payment ID filter parameter.Also applies to: 607-607
638-655:✅ Verification successful
Verify transaction backend implementations
The changes assume that each backend implementation of
TransactionBackendwill properly handle the newfind_completed_transactions_for_payment_idmethod. Ensure all implementations are updated, including test doubles if any.
🏁 Script executed:
#!/bin/bash # Check all backend implementations for the new method echo "Searching for all TransactionBackend implementors..." rg -A 5 "impl\s+.*TransactionBackend\s+for\s+.*" --type rust # Check if they implement the new method echo "Checking for find_completed_transactions_for_payment_id implementations..." rg "fn\s+find_completed_transactions_for_payment_id" --type rustLength of output: 1349
All TransactionBackend implementations updated
I’ve verified that there are only two
impl TransactionBackend for …in the repo (the SQLite backend and the in-code default), and both now definefind_completed_transactions_for_payment_id. No additional test-double implementations ofTransactionBackendwere found. There are no outstanding updates required here.
| fn get_completed_transactions_by_cancelled( | ||
| &self, | ||
| payment_id: Option<Vec<u8>>, | ||
| cancelled: bool, | ||
| ) -> Result<Vec<CompletedTransaction>, TransactionStorageError> { | ||
| let key = if cancelled { | ||
| DbKey::CancelledCompletedTransactions | ||
| } else { | ||
| DbKey::CompletedTransactions | ||
| }; | ||
|
|
||
| let t = match self.db.fetch(&key) { | ||
| Ok(None) => log_error( | ||
| key, | ||
| TransactionStorageError::UnexpectedResult("Could not retrieve completed transactions".to_string()), | ||
| ), | ||
| Ok(Some(DbValue::CompletedTransactions(pt))) => Ok(pt), | ||
| Ok(Some(other)) => unexpected_result(key, other), | ||
| Err(e) => log_error(key, e), | ||
| }?; | ||
| Ok(t) | ||
| match payment_id { | ||
| Some(payment_id) => self.db.find_completed_transactions_for_payment_id(payment_id), | ||
| None => { | ||
| let t = match self.db.fetch(&key) { | ||
| Ok(None) => log_error( | ||
| key, | ||
| TransactionStorageError::UnexpectedResult( | ||
| "Could not retrieve completed transactions".to_string(), | ||
| ), | ||
| ), | ||
| Ok(Some(DbValue::CompletedTransactions(pt))) => Ok(pt), | ||
| Ok(Some(other)) => unexpected_result(key, other), | ||
| Err(e) => log_error(key, e), | ||
| }?; | ||
| Ok(t) | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential issue with cancelled status filtering
When a payment ID is provided, the method directly calls find_completed_transactions_for_payment_id without considering the cancelled parameter. This could lead to unexpected results where cancelled transactions are included when they shouldn't be, depending on the backend implementation.
Ensure the cancelled filter is applied when using a payment ID by modifying the implementation:
fn get_completed_transactions_by_cancelled(
&self,
payment_id: Option<Vec<u8>>,
cancelled: bool,
) -> Result<Vec<CompletedTransaction>, TransactionStorageError> {
let key = if cancelled {
DbKey::CancelledCompletedTransactions
} else {
DbKey::CompletedTransactions
};
match payment_id {
- Some(payment_id) => self.db.find_completed_transactions_for_payment_id(payment_id),
+ Some(payment_id) => {
+ let transactions = self.db.find_completed_transactions_for_payment_id(payment_id)?;
+ // Filter by cancelled status to maintain consistent behavior
+ let filtered = transactions
+ .into_iter()
+ .filter(|tx| (tx.cancelled.is_some()) == cancelled)
+ .collect();
+ Ok(filtered)
+ },
None => {
let t = match self.db.fetch(&key) {
Ok(None) => log_error(
key,
TransactionStorageError::UnexpectedResult(
"Could not retrieve completed transactions".to_string(),
),
),
Ok(Some(DbValue::CompletedTransactions(pt))) => Ok(pt),
Ok(Some(other)) => unexpected_result(key, other),
Err(e) => log_error(key, e),
}?;
Ok(t)
},
}
}Alternatively, you could extend the trait method to accept the cancelled status:
-fn find_completed_transactions_for_payment_id(
- &self,
- payment_id: Vec<u8>,
-) -> Result<Vec<CompletedTransaction>, TransactionStorageError>;
+fn find_completed_transactions_for_payment_id(
+ &self,
+ payment_id: Vec<u8>,
+ cancelled: bool,
+) -> Result<Vec<CompletedTransaction>, TransactionStorageError>;And update all implementations accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn get_completed_transactions_by_cancelled( | |
| &self, | |
| payment_id: Option<Vec<u8>>, | |
| cancelled: bool, | |
| ) -> Result<Vec<CompletedTransaction>, TransactionStorageError> { | |
| let key = if cancelled { | |
| DbKey::CancelledCompletedTransactions | |
| } else { | |
| DbKey::CompletedTransactions | |
| }; | |
| let t = match self.db.fetch(&key) { | |
| Ok(None) => log_error( | |
| key, | |
| TransactionStorageError::UnexpectedResult("Could not retrieve completed transactions".to_string()), | |
| ), | |
| Ok(Some(DbValue::CompletedTransactions(pt))) => Ok(pt), | |
| Ok(Some(other)) => unexpected_result(key, other), | |
| Err(e) => log_error(key, e), | |
| }?; | |
| Ok(t) | |
| match payment_id { | |
| Some(payment_id) => self.db.find_completed_transactions_for_payment_id(payment_id), | |
| None => { | |
| let t = match self.db.fetch(&key) { | |
| Ok(None) => log_error( | |
| key, | |
| TransactionStorageError::UnexpectedResult( | |
| "Could not retrieve completed transactions".to_string(), | |
| ), | |
| ), | |
| Ok(Some(DbValue::CompletedTransactions(pt))) => Ok(pt), | |
| Ok(Some(other)) => unexpected_result(key, other), | |
| Err(e) => log_error(key, e), | |
| }?; | |
| Ok(t) | |
| }, | |
| } | |
| } | |
| fn get_completed_transactions_by_cancelled( | |
| &self, | |
| payment_id: Option<Vec<u8>>, | |
| cancelled: bool, | |
| ) -> Result<Vec<CompletedTransaction>, TransactionStorageError> { | |
| let key = if cancelled { | |
| DbKey::CancelledCompletedTransactions | |
| } else { | |
| DbKey::CompletedTransactions | |
| }; | |
| match payment_id { | |
| Some(payment_id) => { | |
| let transactions = self | |
| .db | |
| .find_completed_transactions_for_payment_id(payment_id)?; | |
| // Filter by cancelled status to maintain consistent behavior | |
| let filtered = transactions | |
| .into_iter() | |
| .filter(|tx| (tx.cancelled.is_some()) == cancelled) | |
| .collect(); | |
| Ok(filtered) | |
| }, | |
| None => { | |
| let t = match self.db.fetch(&key) { | |
| Ok(None) => log_error( | |
| key, | |
| TransactionStorageError::UnexpectedResult( | |
| "Could not retrieve completed transactions".to_string(), | |
| ), | |
| ), | |
| Ok(Some(DbValue::CompletedTransactions(pt))) => Ok(pt), | |
| Ok(Some(other)) => unexpected_result(key, other), | |
| Err(e) => log_error(key, e), | |
| }?; | |
| Ok(t) | |
| }, | |
| } | |
| } |
Description
Motivation and Context
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Database
Configuration