feat: limit txs searches#7236
Conversation
adds the options to limit tx searches
WalkthroughThis change introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WalletAPI
participant TransactionService
participant Database
Caller->>WalletAPI: get_completed_transactions(max_limit)
WalletAPI->>TransactionService: get_completed_transactions(..., max_limit)
TransactionService->>Database: fetch_completed_transactions(..., max_limit)
Database-->>TransactionService: Vec<CompletedTransaction> (up to max_limit)
TransactionService-->>WalletAPI: Vec<CompletedTransaction>
WalletAPI-->>Caller: Vec<CompletedTransaction>
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)base_layer/wallet_ffi/src/lib.rs✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🔭 Outside diff range comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
9069-9093: Inefficient implementation for single transaction lookupThis function fetches a single transaction by ID but still uses
get_completed_transactionswith a limit parameter and then searches through the results. This is inefficient and themax_search_limitparameter doesn't make sense for a by-ID lookup.Consider using
get_any_transactiondirectly, similar to the refactoredwallet_get_cancelled_transaction_by_id:-let completed_transactions = - (*wallet) - .runtime - .block_on((*wallet).wallet.transaction_service.get_completed_transactions( - None, - None, - None, - max_search_limit, - )); - -match completed_transactions { - Ok(completed_transactions) => { +let transaction = (*wallet).runtime.block_on( + (*wallet) + .wallet + .transaction_service + .get_any_transaction(transaction_id.into()), +); + +match transaction { + Ok(Some(WalletTransaction::Completed(tx))) => { + Box::into_raw(Box::new(tx)) + }, + Ok(Some(_)) | Ok(None) => { + *error_out = LibWalletError::from(WalletError::TransactionServiceError( + TransactionServiceError::TransactionDoesNotExist, + )).code; + ptr::null_mut() + }, + Err(e) => { + *error_out = LibWalletError::from(WalletError::TransactionServiceError(e)).code; + ptr::null_mut() + }, +}
♻️ Duplicate comments (2)
base_layer/wallet_ffi/src/lib.rs (2)
9133-9162: Same inefficiency for pending inbound transaction lookupThis function has the same issue - it fetches multiple transactions and filters by ID instead of using a direct lookup.
9214-9243: Same inefficiency for pending outbound transaction lookupThis function follows the same inefficient pattern.
🧹 Nitpick comments (5)
applications/minotari_console_wallet/src/ui/state/app_state.rs (2)
757-757: Consider making the transaction limit configurable.The hardcoded limit of 500 transactions aligns with the PR objective, but consider making this configurable through the wallet configuration to allow users to adjust based on their needs.
Additionally, ensure users are aware that only the latest 500 transactions are displayed, as this could impact user experience if they expect to see their complete transaction history.
Consider adding a configuration parameter:
- .get_completed_transactions(None, None, None, 500) + .get_completed_transactions(None, None, None, self.wallet_config.max_displayed_transactions.unwrap_or(500))
764-764: The cancelled transaction limit appears reasonable but consider consistency.The limit of 100 for cancelled transactions is reasonable given their typically lower frequency compared to completed transactions. However, like the completed transactions limit, this hardcoded value could benefit from being configurable.
Consider maintaining consistency with the completed transactions approach if you decide to make limits configurable.
base_layer/wallet/src/transaction_service/storage/database.rs (3)
185-185: Consider validating max_limit parameter values.The enum variants now include
max_limitparameters, which is correct for the feature. However, consider adding validation to ensure reasonable limit values.The
u64type allows very large values that could potentially cause performance issues or memory exhaustion. Consider:// Add validation helper function fn validate_max_limit(limit: u64) -> Result<u64, TransactionStorageError> { const MAX_REASONABLE_LIMIT: u64 = 100_000; // Adjust as needed if limit == 0 { Err(TransactionStorageError::InvalidArgument("max_limit cannot be zero".to_string())) } else if limit > MAX_REASONABLE_LIMIT { Err(TransactionStorageError::InvalidArgument(format!("max_limit {} exceeds maximum allowed {}", limit, MAX_REASONABLE_LIMIT))) } else { Ok(limit) } }Also applies to: 188-188
225-225: Improve debugging by including max_limit values in Debug output.The Debug implementation ignores the
max_limitparameter values, which reduces debugging utility when investigating query behavior.- CompletedTransactions(_) => { - write!(f, "CompletedTransactions ") - }, + CompletedTransactions(limit) => { + write!(f, "CompletedTransactions(limit: {})", limit) + },- CancelledCompletedTransactions(_) => { - write!(f, "CancelledCompletedTransactions") - }, + CancelledCompletedTransactions(limit) => { + write!(f, "CancelledCompletedTransactions(limit: {})", limit) + },Also applies to: 234-234
858-858: Consider including max_limit values in Display output for better observability.Similar to the Debug implementation, the Display implementation ignores the
max_limitparameter values, which could be helpful for logging and monitoring.- DbKey::CompletedTransactions(_) => f.write_str("All Complete Transactions"), + DbKey::CompletedTransactions(limit) => write!(f, "All Complete Transactions (limit: {})", limit),- DbKey::CancelledCompletedTransactions(_) => f.write_str("All Cancelled Complete Transactions"), + DbKey::CancelledCompletedTransactions(limit) => write!(f, "All Cancelled Complete Transactions (limit: {})", limit),Also applies to: 861-861
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(4 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(1 hunks)base_layer/wallet/src/transaction_service/handle.rs(4 hunks)base_layer/wallet/src/transaction_service/service.rs(3 hunks)base_layer/wallet/src/transaction_service/storage/database.rs(10 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(8 hunks)base_layer/wallet_ffi/src/lib.rs(23 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: cargo check with stable
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
🔇 Additional comments (26)
applications/minotari_console_wallet/src/automation/commands.rs (1)
796-798: ```shell
#!/bin/bashInspect the implementation of find_completed_transactions_filter_payment_id_block_hash
rg -n "fn find_completed_transactions_filter_payment_id_block_hash" -C30 base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
Search for the use of .limit in the completed transactions queries
rg -n ".limit" -C5 base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
</details> <details> <summary>base_layer/wallet/src/transaction_service/service.rs (3)</summary> `897-905`: **LGTM: Clean implementation of transaction limit feature** The addition of the `max_limit` parameter to the `GetCompletedTransactions` request and its proper passing to the database layer is well implemented and aligns with the PR objective to limit transaction searches for improved performance. --- `923-927`: **LGTM: Consistent implementation of transaction limit** The `GetCancelledCompletedTransactions` case correctly implements the same `max_limit` pattern as the completed transactions, maintaining consistency across the transaction retrieval APIs. --- `1034-1034`: **LGTM: Improved error specificity** The refinement from a parameterless `DbKey::CompletedTransactions` variant to `DbKey::CompletedTransactions(1)` provides better error context and appears to be a logical improvement in error handling specificity. </details> <details> <summary>applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (5)</summary> `1103-1103`: **LGTM: Properly maintains unlimited search for specific transaction queries.** The addition of the fourth parameter with value `0` (indicating no limit) is correct for this specific transaction search endpoint, maintaining existing behavior while supporting the new limiting feature. --- `1209-1209`: **Good: Request extraction moved earlier to access the limit field.** Moving the request extraction before the service calls allows proper access to the `req.limit` field used in subsequent method calls. --- `1217-1217`: **LGTM: Correctly passes request limit for general transaction retrieval.** The use of `req.limit` as the fourth parameter appropriately applies the requested limit for the `get_all_completed_transactions` endpoint, which aligns with the PR's goal of limiting transaction searches. --- `1227-1227`: **LGTM: Consistent limit application for cancelled transactions.** The limit parameter is properly passed to `get_cancelled_completed_transactions`, ensuring both completed and cancelled transactions respect the same limit when fetching all transactions. --- `1334-1334`: **LGTM: Maintains unlimited search for block height specific queries.** Similar to the general `get_completed_transactions`, using `0` for the limit parameter preserves the existing behavior for block height specific searches while supporting the new limiting infrastructure. </details> <details> <summary>base_layer/wallet_ffi/src/lib.rs (1)</summary> `9311-9332`: **Excellent refactoring using `get_any_transaction`** This refactoring simplifies the code and improves efficiency by using a single lookup instead of searching through multiple transaction lists. The conversion logic properly handles all transaction types. This pattern should be applied to the other by-ID lookup functions as well. </details> <details> <summary>base_layer/wallet/src/transaction_service/handle.rs (5)</summary> `82-87`: **LGTM! Consistent parameter addition.** The `max_limit` parameter is correctly added to the `GetCompletedTransactions` variant, maintaining consistency with the existing optional filter parameters. --- `94-94`: **LGTM! Appropriate variant transformation.** The change from a unit variant to a parameterized variant `GetCancelledCompletedTransactions(u64)` is consistent with the approach of adding limits across transaction retrieval methods. --- `230-230`: **LGTM! Display implementation correctly updated.** The `fmt::Display` implementation properly handles the new parameterized variant format for `GetCancelledCompletedTransactions`. --- `1006-1026`: **LGTM! Method signature and call properly updated.** The `get_completed_transactions` method correctly: - Adds the `max_limit: u64` parameter to the signature - Passes the parameter through to the service request - Maintains consistency with the existing method structure --- `1046-1058`: **LGTM! Method signature and call properly updated.** The `get_cancelled_completed_transactions` method correctly: - Adds the `max_limit: u64` parameter to the signature - Passes the parameter through to the service request using the new parameterized variant - Maintains consistency with the other transaction retrieval methods </details> <details> <summary>base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (5)</summary> `180-183`: **LGTM! Correct handling of parameterized enum variants.** The addition of the underscore pattern matching for the new parameterized `DbKey` variants is correct, and returning `OperationNotSupported` is appropriate for these collection-level operations. --- `297-304`: **LGTM! Consistent implementation of limit parameter.** The fetch method correctly extracts the `max_limit` parameter from the `DbKey` variants and passes it to the SQL query methods. The implementation is consistent between both completed and cancelled transaction cases. Also applies to: 321-328 --- `377-380`: **LGTM! Appropriate contains() behavior for limited collections.** Returning `false` for the `contains()` method on limited collection keys is semantically correct, as checking containment doesn't make sense for a collection that may only return a subset of items. --- `1147-1172`: **LGTM! Consistent limit implementation across query methods.** The `find_completed_transactions_filter_payment_id_block_hash` method correctly implements the same limit pattern as other query methods. The logic for handling `max_limit = 0` as "no limit" and applying the limit to the query is consistent and correct. --- `872-886`: **Check for potential overflow in limit conversion.** The implementation correctly applies the limit parameter and handles the special case where `max_limit = 0` means no limit. However, consider validating that large `u64` values don't overflow when cast to `i64`. ```shell #!/bin/bash # Search for similar u64 to i64 conversions in the codebase to check for consistent overflow handling rg "as i64" --type rust -A 2 -B 2base_layer/wallet/src/transaction_service/storage/database.rs (6)
165-165: LGTM - Parameter addition to trait method.The
max_limitparameter is correctly added to the trait method signature, maintaining consistency with the PR objective to limit transaction searches.
503-510: LGTM - Method signature and implementation are consistent.The
max_limitparameter is properly added to the method signature and correctly passed through to the backend implementation.
625-625: LGTM - Parameter threading is correct.The
max_limitparameter is properly added to the public method and correctly passed to the internal implementation.Also applies to: 627-627
639-644: LGTM - Method signature update is consistent.The method now accepts
max_limitand correctly passes it through to the internal implementation, maintaining consistency with other similar methods.
671-671: LGTM - Internal method properly updated.The internal
get_completed_transactions_by_cancelledmethod correctly incorporates themax_limitparameter in both the signature and enum variant construction.Also applies to: 674-674, 676-676
693-698: LGTM - Backend call properly updated.The call to
find_completed_transactions_filter_payment_id_block_hashcorrectly includes the newmax_limitparameter, maintaining consistency across the trait implementation.
Test Results (Integration tests) 2 files 1 errors 1 suites 15m 45s ⏱️ For more details on these parsing errors and failures, see this check. Results for commit e03a7a8. ♻️ This comment has been updated with latest results. |
Test Results (CI) 3 files 126 suites 44m 24s ⏱️ Results for commit 2ef6433. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/wallet/tests/transaction_service_tests/storage.rs (1)
366-438: Consider adding test coverage for transaction limiting functionality.While the current changes correctly update the API calls for existing functionality, consider adding specific test cases to verify the transaction limiting behavior introduced by this PR. Tests should verify:
- When
max_limitis set to a specific number (e.g., 2), only that many transactions are returned- When
max_limitis 0, all transactions are returned (confirming unlimited behavior)- When
max_limitexceeds available transactions, all available transactions are returnedExample test addition:
+ // Test transaction limiting functionality + let limited_completed_txs = db.get_completed_transactions(None, None, None, 2).unwrap(); + assert!(limited_completed_txs.len() <= 2, "Should respect max_limit parameter"); + + let limited_cancelled_txs = db.get_cancelled_completed_transactions(1).unwrap(); + assert!(limited_cancelled_txs.len() <= 1, "Should respect max_limit parameter");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(11 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(5 hunks)base_layer/wallet/tests/transaction_service_tests/storage.rs(2 hunks)base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs(8 hunks)base_layer/wallet_ffi/src/lib.rs(21 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- base_layer/wallet_ffi/src/lib.rs
- base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (6)
base_layer/wallet/tests/transaction_service_tests/storage.rs (2)
366-366: LGTM: API signature updates are consistent.The updates to
get_completed_transactionsmethod calls correctly include the newmax_limitparameter with value0, which appears to represent unlimited retrieval appropriate for comprehensive testing.Also applies to: 424-424, 432-432
426-426: LGTM: Cancelled transaction method calls updated correctly.The
get_cancelled_completed_transactionsmethod calls have been properly updated to include themax_limitparameter with value0for unlimited retrieval in tests.Also applies to: 438-438
base_layer/wallet/tests/transaction_service_tests/service.rs (4)
2286-2286: LGTM! Correctly updated method signature to include max_limit parameter.The addition of the
0parameter aligns with the newget_completed_transactionsmethod signature that requires amax_limit: u64argument. Using0as the limit is appropriate for tests where you want to retrieve all transactions without pagination.
2391-2391: LGTM! Consistent parameter updates across all transaction service calls.All three calls to
get_completed_transactionshave been consistently updated with the newmax_limitparameter set to0. This ensures the test can retrieve all transactions to properly validate the expected counts (4 for Alice, 3 for Bob, 1 for Carol).Also applies to: 2395-2395, 2400-2400
5581-5581: LGTM! Transaction broadcast test properly updated for new API.Both calls in the
transaction_service_tx_broadcasttest function correctly include the newmax_limitparameter. Using0allows the test to retrieve all completed transactions to verify transaction status and behavior during the broadcast process.Also applies to: 5689-5689
6363-6363: LGTM! Ordering test correctly adapted for pagination parameter.The
test_completed_transactions_orderingtest appropriately includes the newmax_limitparameter. Using0ensures all 4 test transactions are retrieved to properly validate the timestamp-based ordering logic.
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Hi, looking good, some comments though.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
base_layer/wallet_ffi/src/lib.rs (3)
8752-8752: Parameter ordering remains inconsistent across FFI functions.The
max_search_limitparameter positioning is still inconsistent across functions:
- Bulk retrieval functions: 2nd parameter (before
error_out)- By-ID functions: 3rd parameter (after
error_out)This inconsistency makes the API harder to use and more error-prone.
Also applies to: 8818-8818, 8898-8898, 8970-8970, 9127-9127, 9208-9208
9127-9127: Remove unnecessarymax_search_limitparameter from by-ID functions.The
max_search_limitparameter doesn't make sense for functions that retrieve a single transaction by ID, as these functions return at most one transaction.Also applies to: 9208-9208
8750-8754: Breaking API change: parameter order modification not addressed.Adding
max_search_limitas the second parameter still breaks backward compatibility. Existing code callingwallet_get_completed_transactions(wallet, error_out)will fail.Consider adding the new parameter at the end of the parameter list to maintain backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/wallet_ffi/src/lib.rs(20 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
9084-9105: Verify the inverted logic in transaction status check.The refactored function returns transactions that are NOT
CompletedorBroadcaststatus, which seems counterintuitive for a function namedwallet_get_completed_transaction_by_id. This may not be equivalent to the previous implementation.- if completed_transaction.status != TransactionStatus::Completed && - completed_transaction.status != TransactionStatus::Broadcast + if completed_transaction.status == TransactionStatus::Completed || + completed_transaction.status == TransactionStatus::BroadcastPlease verify this logic aligns with the expected behavior and past implementation.
2 participants
Description
adds the option to limit transaction searches and not read the entire completed transactions database.
Limits the console wallet to only view the latest 500 txs.
Summary by CodeRabbit
New Features
Refactor