Skip to content

fix: pagination for all completed transactions gRPC method#7366

Merged
SWvheerden merged 6 commits intotari-project:developmentfrom
MCozhusheck:fix/paginated-get-all-completed-transactions
Jul 23, 2025
Merged

fix: pagination for all completed transactions gRPC method#7366
SWvheerden merged 6 commits intotari-project:developmentfrom
MCozhusheck:fix/paginated-get-all-completed-transactions

Conversation

@MCozhusheck
Copy link
Copy Markdown
Contributor

@MCozhusheck MCozhusheck commented Jul 23, 2025

Description

Closes: #7274

Adds new methods GetAllCompletedTransactionsStream which fixes pagination and deprecates GetAllCompletedTransactions.

This PR changes query for all completed transaction to utilize SQL LIMIT and OFFSET instead of filtering it in memory and also returns data as stream instead as a single response.

How Has This Been Tested?

Run cargo check and tests to verify every check is passed.

Use 3rd party tool like grpcurl to check method response:

# Test existing API
grpcurl -d '{"offset":0,"limit":100,"status_bitflag":0}' \
  localhost:18143 tari.wallet.Wallet/GetAllCompletedTransactions

# Test new streaming API
grpcurl -d '{"offset":0,"limit":100,"status_bitflag":0}' \
  localhost:18143 tari.wallet.Wallet/GetAllCompletedTransactionsStream

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

Summary by CodeRabbit

  • New Features

    • Introduced a new streaming API for retrieving completed wallet transactions, enabling efficient, paginated, and progressive loading.
    • Added support for filtering completed transactions by status and retrieving them in configurable chunks.
    • Enforced a maximum limit of 50 transactions per request in the API.
  • Improvements

    • Enhanced performance and memory efficiency when loading large numbers of completed transactions.
    • Updated documentation to indicate that the previous non-streaming API is deprecated and recommend using the new streaming method.
    • Added deprecation warnings and comments to guide migration from the old API.
  • Bug Fixes

    • Improved logging messages for transaction retrieval actions.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jul 23, 2025

Walkthrough

This change introduces a new streaming gRPC method for retrieving completed wallet transactions with database-backed pagination and optional status filtering. The server, service, and storage layers are extended to support paginated queries, and the previous non-streaming method is marked as deprecated. Proto definitions, server logic, and database access are updated accordingly.

Changes

File(s) Change Summary
applications/minotari_app_grpc/proto/wallet.proto Added streaming RPC GetAllCompletedTransactionsStream; deprecated non-streaming method; updated comments and field docs.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs Implemented new streaming RPC; refactored old method for paginated querying; added chunked streaming logic and deprecation.
base_layer/wallet/src/transaction_service/handle.rs Added paginated request variant and async handler; extended enum and display logic.
base_layer/wallet/src/transaction_service/service.rs Added handling for paginated transaction requests in main service handler.
base_layer/wallet/src/transaction_service/storage/database.rs Added trait and struct methods for paginated transaction retrieval.
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs Implemented paginated, optionally bitflag-filtered transaction queries for SQLite backend.
base_layer/wallet/src/transaction_service/error.rs Added new error variant InvalidArgument for invalid request parameters.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant WalletGrpcServer
    participant TransactionServiceHandle
    participant TransactionService
    participant Database

    Client->>WalletGrpcServer: GetAllCompletedTransactionsStream(request)
    loop while more transactions
        WalletGrpcServer->>TransactionServiceHandle: get_completed_transactions_paginated(offset, limit, status_filter)
        TransactionServiceHandle->>TransactionService: GetCompletedTransactionsPaginated(offset, limit, status_filter)
        TransactionService->>Database: get_completed_transactions_paginated(offset, limit, status_filter)
        Database-->>TransactionService: Vec<CompletedTransaction>
        TransactionService-->>TransactionServiceHandle: Vec<CompletedTransaction>
        TransactionServiceHandle-->>WalletGrpcServer: Vec<CompletedTransaction>
        WalletGrpcServer-->>Client: stream GetCompletedTransactionsResponse (chunk)
    end
Loading

Estimated code review effort

4 (~90 minutes)

Possibly related PRs

Suggested reviewers

  • SWvheerden

Poem

A rabbit streams through fields of code,
Paging through transactions, lightening the load.
Old ways now gently set aside,
As chunks of data swiftly glide.
With every hop, performance grows—
In streaming fields, efficiency flows!
🐇💨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a5093b6 and c6ad2fe.

📒 Files selected for processing (3)
  • applications/minotari_app_grpc/proto/wallet.proto (2 hunks)
  • base_layer/wallet/src/transaction_service/error.rs (1 hunks)
  • base_layer/wallet/src/transaction_service/service.rs (1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/wallet/src/transaction_service/error.rs (1)

Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.

🚧 Files skipped from review as they are similar to previous changes (2)
  • base_layer/wallet/src/transaction_service/service.rs
  • applications/minotari_app_grpc/proto/wallet.proto
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/wallet/src/transaction_service/error.rs (1)

Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: file licenses
  • GitHub Check: cargo check with stable
  • GitHub Check: Cucumber tests / Base Layer
  • GitHub Check: Cucumber tests / FFI
  • GitHub Check: ci
  • GitHub Check: ledger build tests
🔇 Additional comments (1)
base_layer/wallet/src/transaction_service/error.rs (1)

225-226: LGTM! Error variant correctly added for input validation.

The new InvalidArgument(String) variant follows the established patterns in the error enum and provides appropriate error handling for the new paginated transaction retrieval functionality. The error message format is consistent with other variants and the placement at the end of the enum maintains backward compatibility.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@MCozhusheck MCozhusheck marked this pull request as ready for review July 23, 2025 10:00
@MCozhusheck MCozhusheck requested a review from a team as a code owner July 23, 2025 10:00
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
base_layer/wallet/src/transaction_service/service.rs (1)

1008-1015: LGTM! Implementation follows established patterns.

The new GetCompletedTransactionsPaginated request handler is correctly implemented and follows the same pattern as other request handlers in this method. The parameters are properly passed through to the database layer and the response is correctly wrapped.

However, consider adding input validation for the pagination parameters:

 TransactionServiceRequest::GetCompletedTransactionsPaginated {
     offset,
     limit,
     status_filter,
-} => Ok(TransactionServiceResponse::CompletedTransactions(
-    self.db
-        .get_completed_transactions_paginated(offset, limit, status_filter)?,
-)),
+} => {
+    // Validate pagination parameters
+    if limit == 0 {
+        return Err(TransactionServiceError::InvalidArgument("limit must be greater than 0".to_string()));
+    }
+    Ok(TransactionServiceResponse::CompletedTransactions(
+        self.db
+            .get_completed_transactions_paginated(offset, limit, status_filter)?,
+    ))
+},
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)

1277-1281: Consider adding performance monitoring.

Following the pattern established in other methods in this file, consider adding performance monitoring to track query execution time, especially since this method will be used for streaming operations that may be called frequently.

+        let start = Instant::now();
         let mut conn = self.database_connection.get_pooled_connection()?;
+        let acquire_lock = start.elapsed();
         let cipher = acquire_read_lock!(self.cipher);

         use diesel::prelude::*;
         
         // ... existing query logic ...
         
-        query
+        let result = query
             .order(completed_transactions::timestamp.desc())
             .offset(offset as i64)
             .limit(limit as i64)
             .load::<CompletedTransactionSql>(&mut conn)?
             .into_iter()
             .map(|ct: CompletedTransactionSql| {
                 CompletedTransaction::try_from(ct, &cipher).map_err(TransactionStorageError::from)
             })
-            .collect::<Result<Vec<CompletedTransaction>, TransactionStorageError>>()
+            .collect::<Result<Vec<CompletedTransaction>, TransactionStorageError>>()?;
+
+        if start.elapsed().as_millis() > 0 {
+            trace!(
+                target: LOG_TARGET,
+                "sqlite profile - find_completed_transactions_paginated: lock {} + db_op {} = {} ms",
+                acquire_lock.as_millis(),
+                (start.elapsed() - acquire_lock).as_millis(),
+                start.elapsed().as_millis()
+            );
+        }
+
+        Ok(result)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)

1280-1407: Good refactoring with chunked pagination.

The implementation correctly handles pagination and adds a deprecation warning. The chunked approach with a maximum of 100 items per chunk is a good balance for memory efficiency.

Consider documenting the chunk size choice (100) with a comment explaining the rationale.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad5aac9 and a5093b6.

📒 Files selected for processing (6)
  • applications/minotari_app_grpc/proto/wallet.proto (2 hunks)
  • applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (5 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 (2 hunks)
  • base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1 hunks)
🧠 Learnings (7)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/wallet/src/transaction_service/handle.rs (2)

Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)

Learnt from: hansieodendaal
PR: #7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the hard_delete_all_stale_peers method in comms/core/src/peer_manager/storage/database.rs, the SQL query intentionally uses exact equality (peers.features = ?) rather than bitwise operations (peers.features & ? != 0) when matching COMMUNICATION_NODE features. This is the intended behavior to match only peers with exactly the COMMUNICATION_NODE feature, excluding those with additional feature flags.

applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.

Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.

Learnt from: hansieodendaal
PR: #7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the fetch_peers_from_connection and collect_peer_stream functions rely on RPC streaming, and when the main connection is closed by another process, collect_peer_stream times out after STREAM_ITEM_TIMEOUT because it cannot detect that the peer can no longer respond, returning an empty vector of peers. This is why the connection state check is important for the retry logic.

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.

base_layer/wallet/src/transaction_service/storage/database.rs (1)

Learnt from: SWvheerden
PR: #7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.666Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via get_last_scanned_height() rather than persisting it themselves. This follows separation of concerns architecture pattern.

applications/minotari_app_grpc/proto/wallet.proto (2)

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.

base_layer/wallet/src/transaction_service/service.rs (1)

Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
base_layer/wallet/src/transaction_service/handle.rs (2)

Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)

Learnt from: hansieodendaal
PR: #7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the hard_delete_all_stale_peers method in comms/core/src/peer_manager/storage/database.rs, the SQL query intentionally uses exact equality (peers.features = ?) rather than bitwise operations (peers.features & ? != 0) when matching COMMUNICATION_NODE features. This is the intended behavior to match only peers with exactly the COMMUNICATION_NODE feature, excluding those with additional feature flags.

applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.

Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.

Learnt from: hansieodendaal
PR: #7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the fetch_peers_from_connection and collect_peer_stream functions rely on RPC streaming, and when the main connection is closed by another process, collect_peer_stream times out after STREAM_ITEM_TIMEOUT because it cannot detect that the peer can no longer respond, returning an empty vector of peers. This is why the connection state check is important for the retry logic.

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.

base_layer/wallet/src/transaction_service/storage/database.rs (1)

Learnt from: SWvheerden
PR: #7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.666Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via get_last_scanned_height() rather than persisting it themselves. This follows separation of concerns architecture pattern.

applications/minotari_app_grpc/proto/wallet.proto (2)

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.

Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.

base_layer/wallet/src/transaction_service/service.rs (1)

Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: ledger build tests
  • GitHub Check: test (mainnet, stagenet)
  • GitHub Check: test (testnet, esmeralda)
  • GitHub Check: cargo check with stable
  • GitHub Check: test (nextnet, nextnet)
  • GitHub Check: ci
🔇 Additional comments (13)
base_layer/wallet/src/transaction_service/storage/database.rs (2)

176-181: Well-designed trait method for paginated queries.

The method signature is consistent with existing trait methods and properly designed for database-backed pagination with optional status filtering.


878-886: LGTM! Consistent wrapper implementation.

The method follows the established pattern of other database wrapper methods and properly delegates to the backend implementation.

base_layer/wallet/src/transaction_service/handle.rs (3)

95-99: Well-structured enum variant for paginated requests.

The variant follows established naming conventions and parameter patterns, with types that properly match the corresponding database method.


279-279: Consistent display implementation.

The display implementation follows the established pattern of other enum variants in the trait.


1269-1287: Properly implemented handle method.

The method follows the established pattern of other handle methods with correct async implementation, error handling, and response type matching.

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (2)

1250-1252: Add input validation for pagination parameters.

The method should validate that offset and limit parameters are within reasonable bounds to prevent potential performance issues or resource exhaustion.

+        // Validate pagination parameters
+        const MAX_LIMIT: u64 = 10_000; // Define reasonable maximum
+        if limit == 0 || limit > MAX_LIMIT {
+            return Err(TransactionStorageError::InvalidArgument(
+                format!("Limit must be between 1 and {}", MAX_LIMIT)
+            ));
+        }
+
         let mut conn = self.database_connection.get_pooled_connection()?;
⛔ Skipped due to learnings
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

1245-1287: Bitflag-to-status mapping is correct; no changes needed

Verified that the TransactionStatus enum discriminants are defined as sequential integers (Completed = 0, Broadcast = 1, … OneSidedUnconfirmed = 8), so mapping bit position i → status value i is accurate. The current loop over 0..32 correctly builds the list of status codes.

Using trailing_zeros() to iterate only set bits is a valid micro-optimization but not required here. No fixes are necessary.

Likely an incorrect or invalid review comment.

applications/minotari_app_grpc/proto/wallet.proto (2)

1804-1809: Clarify & enforce limit semantics.

  1. The comment’s grammar is unclear (“limited to 50 if passed higher number than it will be reduced to 50”).
  2. Nothing in the proto enforces the cap; the server must clamp it.

Consider tightening both documentation and validation:

-  // limited to 50 if passed higher number than it will be reduced to 50
+  // Maximum page size. Values > 50 will be clamped to 50 by the server.

Optionally, expose the max as a constant in the service docs or an enum so clients don’t hard-code 50.

Ensure the backend handler actually clamps the value; otherwise the proto comment becomes misleading.


1811-1814: Also deprecate the response message.

GetAllCompletedTransactionsResponse is now only used by the deprecated RPC.
Marking the message itself as deprecated prevents accidental reuse and triggers compiler warnings.

-message GetAllCompletedTransactionsResponse {
-  repeated TransactionInfo transactions = 1;
-}
+message GetAllCompletedTransactionsResponse {
+  option deprecated = true;
+  repeated TransactionInfo transactions = 1;
+}

[ suggest_optional_refactor ]

applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)

242-242: LGTM!

The new associated type follows the established naming convention and pattern for streaming responses.


1146-1147: Good catch on the log message correction.

The updated log message now correctly reflects the method name.


1185-1186: Consistent log message updates.

All log messages have been correctly updated to match the method name.

Also applies to: 1256-1260


1409-1610: Excellent streaming implementation.

The new streaming method is well-designed with:

  • Appropriate chunk size (50) for responsive streaming
  • Comprehensive error handling that sends errors to the client
  • Graceful handling of client disconnection
  • Detailed logging at appropriate levels
  • Defensive programming with saturating_sub

The implementation provides better memory efficiency and responsiveness compared to the batch method.

Comment on lines 1102 to +1109
// Get all completed transactions including cancelled ones, sorted by timestamp and paginated
// DEPRECATED: Use GetAllCompletedTransactionsStream for better performance and memory efficiency
rpc GetAllCompletedTransactions(GetAllCompletedTransactionsRequest) returns (GetAllCompletedTransactionsResponse);

// Get all completed transactions including cancelled ones, sorted by timestamp and paginated (streaming version)
// Recommended: Use this streaming version for better performance and progressive loading
rpc GetAllCompletedTransactionsStream(GetAllCompletedTransactionsRequest) returns (stream GetCompletedTransactionsResponse);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Mark the unary RPC as formally deprecated and add explicit options on both RPCs.

You rely on a comment to signal deprecation, but the proto spec already provides option deprecated = true; on an RPC MethodOptions.
This makes the deprecation machine-readable for code-gen (Rust, Go, Java, TS-grpc, etc.) and raises compile-time warnings rather than relying on humans to read comments.

-  // DEPRECATED: Use GetAllCompletedTransactionsStream for better performance and memory efficiency
-  rpc GetAllCompletedTransactions(GetAllCompletedTransactionsRequest)
-      returns (GetAllCompletedTransactionsResponse);
+  rpc GetAllCompletedTransactions(GetAllCompletedTransactionsRequest)
+      returns (GetAllCompletedTransactionsResponse) {
+    option deprecated = true;           // machine-readable
+  }

-  // Get all completed transactions including cancelled ones, sorted by timestamp and paginated (streaming version)
-  // Recommended: Use this streaming version for better performance and progressive loading
-  rpc GetAllCompletedTransactionsStream(GetAllCompletedTransactionsRequest) returns (stream GetCompletedTransactionsResponse);
+  // Streaming version – preferred for pagination + memory efficiency.
+  rpc GetAllCompletedTransactionsStream(GetAllCompletedTransactionsRequest)
+      returns (stream GetCompletedTransactionsResponse);

This tiny change improves DX across every generated client.
(No functional impact on the wire.)

🤖 Prompt for AI Agents
In applications/minotari_app_grpc/proto/wallet.proto around lines 1102 to 1109,
the unary RPC GetAllCompletedTransactions is marked as deprecated only via
comments. To fix this, add the option deprecated = true; to its RPC method
options to formally mark it as deprecated in the proto spec. Also, add explicit
options (such as idempotency level or other relevant options) to both
GetAllCompletedTransactions and GetAllCompletedTransactionsStream RPCs to
improve clarity and support for code generation tools. This change will enable
machine-readable deprecation and better developer experience without affecting
runtime behavior.

@SWvheerden SWvheerden merged commit 8b3bb29 into tari-project:development Jul 23, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add paging to load transaction via grpc

2 participants