fix: add filtering flag back#7208
Conversation
Test Results (CI) 3 files 126 suites 44m 12s ⏱️ For more details on these failures, see this check. Results for commit a98c761. ♻️ This comment has been updated with latest results. |
WalkthroughThe change updates the transaction retrieval logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletGrpcServer
participant TransactionStore
Client->>WalletGrpcServer: get_all_completed_transactions(req)
WalletGrpcServer->>TransactionStore: fetch all completed transactions
WalletGrpcServer->>WalletGrpcServer: filter transactions by status bitflag
WalletGrpcServer->>WalletGrpcServer: apply offset and limit
WalletGrpcServer->>Client: return filtered and paginated 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)
✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
[error] 1182-1296: Clippy error: this function has too many lines (105/100). Function 'get_all_completed_transactions' exceeds the maximum allowed lines. See https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines
[warning] 1230-1230: Clippy warning: using clone on type TransactionStatus which implements the Copy trait. Suggestion: try removing the clone call: tx.status. See https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
| for txn in completed_transactions | ||
| .into_iter() | ||
| .filter(|tx| req.status_bitflag == 0 || (req.status_bitflag & (1 << (tx.status.clone() as u32))) != 0) | ||
| .skip(offset) | ||
| .take(limit) | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bit-flag filter causes type‐mismatch & unnecessary clone() – will not compile with u64 flags
req.status_bitflag comes from the protobuf message as an unsigned integer (u64 in the generated code).
The right-hand operand created by
(1 << (tx.status.clone() as u32))is an i32, so the & operation (u64 & i32) fails to type-check.
In addition, TransactionStatus is Copy; the clone() is redundant and flagged by Clippy.
Proposed fix:
- .filter(|tx| req.status_bitflag == 0 || (req.status_bitflag & (1 << (tx.status.clone() as u32))) != 0)
+ .filter(|tx| {
+ // When no filter is supplied, include every tx
+ if req.status_bitflag == 0 {
+ return true;
+ }
+ // Convert both sides to the same integer type before the bitwise op
+ let status_bit = 1u64 << (tx.status as u32);
+ (req.status_bitflag & status_bit) != 0
+ })Benefits
• Compiles for any 64-bit flag mask.
• Removes the clone() call, satisfying Clippy.
• Reads clearer with an early return for the zero-mask case.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 1230-1230: Clippy warning: using clone on type TransactionStatus which implements the Copy trait. Suggestion: try removing the clone call: tx.status. See https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs around
lines 1228 to 1233, the bit-flag filter uses a type-mismatched expression where
req.status_bitflag is u64 but the shifted value is i32, causing a compilation
error. Remove the unnecessary clone() on tx.status since it is Copy, and cast
tx.status to u64 before shifting 1 by that amount to ensure both operands in the
bitwise AND are u64. Also, consider adding an early return if req.status_bitflag
is zero to simplify the filter logic.
Test Results (Integration tests) 2 files 10 suites 2h 14m 21s ⏱️ For more details on these failures, see this check. Results for commit c7fda4d. |
* development: (607 commits) Wallet GRPC port comment fix from 18142 to 18143 (tari-project#7221) feat: integrated address support for Ledger (tari-project#7198) chore: new release v4.1.1-pre.0 (tari-project#7211) fix: migration can now correctly resume after stopping (tari-project#7210) fix: only revalidated rejected transactions on startup (tari-project#7209) fix: add filtering flag back (tari-project#7208) feat: improve wallet balance checks from external clients (tari-project#7207) feat!: update grpc supply query (tari-project#7137) docs: Updated API GRPC and Exchange Guide (tari-project#7205) chore: new release v4.4.0-pre.0 (tari-project#7202) feat: update base node proto to search bytes (tari-project#7201) feat: full PayRef implementation (tari-project#7154) test: add ffi cucumber wallet balance test (tari-project#7189) chore: fix tests (tari-project#7196) fix(network-discovery): add back idle event handling (tari-project#7194) Update SECURITY.md (tari-project#7193) fix: transaction manager service unmined lookup (tari-project#7192) fix: wallet ffi database name mismatch for mobile wallet (tari-project#7191) fix: payment_id deserialize (tari-project#7187) fix: remove code for deleting stale peers (tari-project#7184) ...
2 participants
Description
For some reason it was removed while merging.
Origin: #7161
Summary by CodeRabbit
New Features
Enhancements