feat: get all completed txs with pagination#7113
Conversation
|
""" WalkthroughA new gRPC method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletGrpcServer
participant WalletDB
Client->>WalletGrpcServer: GetAllCompletedTransactionsRequest(offset, limit)
WalletGrpcServer->>WalletDB: get_completed_transactions(None, None, None)
WalletDB-->>WalletGrpcServer: completed_transactions
WalletGrpcServer->>WalletDB: get_cancelled_completed_transactions()
WalletDB-->>WalletGrpcServer: cancelled_transactions
WalletGrpcServer->>WalletGrpcServer: Combine, sort, paginate transactions
WalletGrpcServer-->>Client: GetAllCompletedTransactionsResponse(transactions)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ 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: 0
🧹 Nitpick comments (2)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
1061-1130: Implementation is correct but consider performance optimizations for large datasets.The method implementation follows established patterns and correctly:
- Fetches both completed and cancelled transactions
- Sorts by timestamp in descending order
- Applies pagination with offset/limit
- Maps to TransactionInfo consistently
However, consider these improvements:
Performance consideration: The current approach loads all transactions into memory before applying pagination, which could be memory-intensive for wallets with large transaction histories. For production systems with thousands of transactions, consider implementing database-level pagination.
Minor improvements:
- completed_transactions.sort_by(|a, b| { - b.timestamp - .partial_cmp(&a.timestamp) - .expect("Should be able to compare timestamps") - }); + completed_transactions.sort_by(|a, b| { + b.timestamp.cmp(&a.timestamp) + });This change uses
cmpinstead ofpartial_cmpwithexpect, which is more appropriate for timestamps and avoids potential panics.
1099-1105: Pagination logic is correct and safe.The implementation properly handles pagination with reasonable defaults:
offsetdefaults to 0limitdefaults tousize::MAX(effectively unlimited)- Safe usage of
skip()andtake()Optional enhancement: Consider adding validation for pagination parameters to provide better error messages for unreasonable values:
+ let offset = req.offset.unwrap_or(0) as usize; + let limit = req.limit.map(|l| l as usize); + + // Optional: Add validation + if offset > completed_transactions.len() { + return Err(Status::invalid_argument("Offset exceeds total number of transactions")); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
🔇 Additional comments (3)
applications/minotari_app_grpc/proto/wallet.proto (2)
1051-1052: LGTM! Well-designed RPC method for paginated transaction retrieval.The new RPC method follows established patterns and includes proper pagination support through optional offset and limit parameters.
1423-1430: LGTM! Clean message definitions with proper pagination support.The request and response messages are well-designed:
- Optional pagination fields provide flexibility
- Reuses existing TransactionInfo type for consistency
- Follows protobuf naming conventions
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
52-53: LGTM! Properly imported required protobuf types.The new imports are correctly added and follow the existing import patterns in the file.
Test Results (Integration tests)0 tests 0 ✅ 0s ⏱️ Results for commit 25f7e4f. ♻️ This comment has been updated with latest results. |
Test Results (CI)1 331 tests 1 328 ✅ 19m 15s ⏱️ For more details on these failures, see this check. Results for commit 7d85d29. ♻️ This comment has been updated with latest results. |
2 participants
Description
Motivation and Context
We need a non-streamed method that returns non-cancelled and cancelled completed txs, sorted by timestamp(pending txs on top) that handles pagination.
How Has This Been Tested?
Locally, using grpc client(limit 2, offset: 1):
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit