feat: get_all_completed_transactions bitflag status filtering#7161
Merged
SWvheerden merged 4 commits intotari-project:developmentfrom Jun 3, 2025
Merged
Conversation
Contributor
WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant gRPC_Server
participant TransactionStore
Client->>gRPC_Server: GetAllCompletedTransactionsRequest(status_bitflag)
gRPC_Server->>TransactionStore: Fetch all completed transactions
TransactionStore-->>gRPC_Server: Return transaction list
gRPC_Server->>gRPC_Server: Filter transactions by status_bitflag
gRPC_Server-->>Client: Return filtered transactions
Possibly related PRs
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 (7)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
applications/minotari_app_grpc/proto/wallet.proto (1)
1427-1431: Add documentation for thestatus_bitflagfieldInclude a comment above the new field to explain its purpose and default behavior:
message GetAllCompletedTransactionsRequest { uint64 offset = 1; uint64 limit = 2; - uint32 status_bitflag = 3; + // Optional bitmask to filter returned transactions by their status flags. + // If zero (default or omitted), all completed transactions are returned; + // otherwise, only transactions for which (status & status_bitflag) != 0 are included. + uint32 status_bitflag = 3; }applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
1205-1205: Optimize the bitflag filtering implementation and add validation.The bitflag filtering logic is correct, but there are several improvements that can be made:
- Remove the unnecessary
.clone()call ontx.statussince enum-to-u32 casting doesn't require cloning- Add validation for the
status_bitflagvalue to prevent potential overflow- Add documentation to explain the bitflag filtering logic
Apply this diff to optimize and improve the implementation:
- .filter(|tx| req.status_bitflag == 0 || (req.status_bitflag & (1 << (tx.status.clone() as u32))) != 0) + .filter(|tx| { + // If status_bitflag is 0, return all transactions (no filtering) + // Otherwise, check if the transaction's status bit is set in the bitflag + req.status_bitflag == 0 || (req.status_bitflag & (1 << (tx.status as u32))) != 0 + })Additionally, consider adding validation near the beginning of the method to ensure the bitflag value is reasonable:
// Validate status_bitflag to prevent potential overflow (assuming status enum has < 32 variants) if req.status_bitflag != 0 && req.status_bitflag >= (1 << 32) { return Err(Status::invalid_argument("status_bitflag value is too large")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
Test Results (CI)371 tests 370 ✅ 7m 39s ⏱️ For more details on these failures, see this check. Results for commit 0a0af9f. |
use uint64
SWvheerden
approved these changes
Jun 3, 2025
SWvheerden
pushed a commit
that referenced
this pull request
Jun 12, 2025
Description --- For some reason it was removed while merging. Origin: #7161 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added the ability to filter completed transactions by status when viewing transaction history. - **Enhancements** - Improved transaction history pagination to work with filtered results. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 13, 2025
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
How Has This Been Tested?
locally, using nodejs client:
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit