feat: add GetBlockHeightTransactions grpc method#7081
feat: add GetBlockHeightTransactions grpc method#7081SWvheerden merged 9 commits intotari-project:developmentfrom
Conversation
WalkthroughA new gRPC method, Changes
Sequence Diagram(s)sequenceDiagram
participant NodeJS_Client as Node.js Client
participant gRPC_Server as WalletGrpcServer (Rust)
participant Wallet_Service as Wallet Service
NodeJS_Client->>gRPC_Server: GetBlockHeightTransactions(block_height)
gRPC_Server->>Wallet_Service: get_completed_transactions(block_height)
Wallet_Service-->>gRPC_Server: List<Transaction>
gRPC_Server-->>NodeJS_Client: GetBlockHeightTransactionsResponse(transactions)
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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
🧹 Nitpick comments (2)
clients/nodejs/wallet_grpc_client/index.js (2)
7-8: Remove debug console.log statement.This debug statement prints the path to the proto file on every import, which is unnecessary in production code.
-const path = require("path"); -console.log(path.join(__dirname, "../../../applications/minotari_app_grpc/proto/wallet.proto")); +const path = require("path");
94-100: Convert example code to proper test or documentation.This self-executing function appears to be test/example code. Such code would be better placed in a separate test file or converted to a commented example.
- -(async () => { - const a = Client.connect('localhost:18183'); - const {version} = await a.getVersion(); - console.log(version); - const resp = await a.GetBlockHeightTransactions({block_height: 1550}); - console.log(resp); -})() + +// Example usage: +// (async () => { +// const a = Client.connect('localhost:18183'); +// const {version} = await a.getVersion(); +// console.log(version); +// const resp = await a.GetBlockHeightTransactions({block_height: 1550}); +// console.log(resp); +// })()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
clients/nodejs/wallet_grpc_client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
applications/minotari_app_grpc/proto/wallet.proto(5 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(2 hunks)clients/nodejs/wallet_grpc_client/index.js(3 hunks)
🔇 Additional comments (5)
applications/minotari_app_grpc/proto/wallet.proto (2)
387-435: Well-structured RPC documentation.The documentation for the new
GetBlockHeightTransactionsRPC method is comprehensive and follows the established pattern in the file. It clearly explains the purpose, request parameters, restrictions, and includes a practical JavaScript example with sample response.
1407-1416: Clean message definitions.The message definitions for
GetBlockHeightTransactionsRequestandGetBlockHeightTransactionsResponseare appropriately structured and maintain consistency with the existing protocol definitions.clients/nodejs/wallet_grpc_client/index.js (1)
53-53: LGTM - Appropriate function registration.The new
GetBlockHeightTransactionsmethod is correctly added to the functions array.applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
89-90: LGTM - Properly imported new message types.The new request and response message types are correctly imported.
1032-1054: Clean and efficient transaction mapping.The transaction mapping logic is well-implemented, correctly extracting and converting all the necessary fields from the transaction objects to their protocol buffer representation.
| async fn get_block_height_transactions( | ||
| &self, | ||
| request: Request<GetBlockHeightTransactionsRequest>, | ||
| ) -> Result<Response<GetBlockHeightTransactionsResponse>, Status> { | ||
| let start = std::time::Instant::now(); | ||
| trace!( | ||
| target: LOG_TARGET, | ||
| "GetBlockHeightTransactions: Incoming GRPC request" | ||
| ); | ||
| let message = request.into_inner(); | ||
| let block_height = message.block_height; | ||
|
|
||
| let mut transaction_service = self.get_transaction_service(); | ||
| let transactions = transaction_service | ||
| .get_completed_transactions(None, None, Some(block_height)) | ||
| .await | ||
| .map_err(|err| Status::not_found(format!("No transactions found at block height {}: {:?}", block_height, err)))?; | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "GetBlockHeightTransactions: Found {} transactions at block height {}", | ||
| transactions.len(), | ||
| block_height | ||
| ); | ||
|
|
||
| let transactions = transactions | ||
| .iter() | ||
| .map(|txn| TransactionInfo { | ||
| tx_id: txn.tx_id.into(), | ||
| source_address: txn.source_address.to_vec(), | ||
| dest_address: txn.destination_address.to_vec(), | ||
| status: TransactionStatus::from(txn.status.clone()) as i32, | ||
| amount: txn.amount.into(), | ||
| is_cancelled: txn.cancelled.is_some(), | ||
| direction: TransactionDirection::from(txn.direction.clone()) as i32, | ||
| fee: txn.fee.into(), | ||
| timestamp: txn.timestamp.timestamp() as u64, | ||
| excess_sig: txn | ||
| .transaction | ||
| .first_kernel_excess_sig() | ||
| .unwrap_or(&Signature::default()) | ||
| .get_signature() | ||
| .to_vec(), | ||
| payment_id: txn.payment_id.to_bytes(), | ||
| mined_in_block_height: txn.mined_height.unwrap_or(0), | ||
| }) | ||
| .collect(); | ||
|
|
||
| trace!(target: LOG_TARGET, "'get_block_height_transactions' completed in {:.2?}", start.elapsed()); | ||
|
|
||
| Ok(Response::new(GetBlockHeightTransactionsResponse { transactions })) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning an empty list instead of an error when no transactions are found.
The implementation correctly queries transactions at a specific block height, but returns a not_found error status if no transactions exist at that height. This differs from similar methods like get_completed_transactions, where an empty list would be a more appropriate response for a valid block height with no transactions.
Consider modifying line 1024 to handle the empty transaction case:
- let transactions = transaction_service
- .get_completed_transactions(None, None, Some(block_height))
- .await
- .map_err(|err| Status::not_found(format!("No transactions found at block height {}: {:?}", block_height, err)))?;
+ let transactions = match transaction_service
+ .get_completed_transactions(None, None, Some(block_height))
+ .await {
+ Ok(txns) => txns,
+ Err(err) => {
+ debug!(
+ target: LOG_TARGET,
+ "No transactions found at block height {}: {:?}", block_height, err
+ );
+ vec![]
+ }
+ };This approach would return an empty list of transactions rather than an error when no transactions exist at the given block height, which is more consistent with typical API behavior for list endpoints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn get_block_height_transactions( | |
| &self, | |
| request: Request<GetBlockHeightTransactionsRequest>, | |
| ) -> Result<Response<GetBlockHeightTransactionsResponse>, Status> { | |
| let start = std::time::Instant::now(); | |
| trace!( | |
| target: LOG_TARGET, | |
| "GetBlockHeightTransactions: Incoming GRPC request" | |
| ); | |
| let message = request.into_inner(); | |
| let block_height = message.block_height; | |
| let mut transaction_service = self.get_transaction_service(); | |
| let transactions = transaction_service | |
| .get_completed_transactions(None, None, Some(block_height)) | |
| .await | |
| .map_err(|err| Status::not_found(format!("No transactions found at block height {}: {:?}", block_height, err)))?; | |
| debug!( | |
| target: LOG_TARGET, | |
| "GetBlockHeightTransactions: Found {} transactions at block height {}", | |
| transactions.len(), | |
| block_height | |
| ); | |
| let transactions = transactions | |
| .iter() | |
| .map(|txn| TransactionInfo { | |
| tx_id: txn.tx_id.into(), | |
| source_address: txn.source_address.to_vec(), | |
| dest_address: txn.destination_address.to_vec(), | |
| status: TransactionStatus::from(txn.status.clone()) as i32, | |
| amount: txn.amount.into(), | |
| is_cancelled: txn.cancelled.is_some(), | |
| direction: TransactionDirection::from(txn.direction.clone()) as i32, | |
| fee: txn.fee.into(), | |
| timestamp: txn.timestamp.timestamp() as u64, | |
| excess_sig: txn | |
| .transaction | |
| .first_kernel_excess_sig() | |
| .unwrap_or(&Signature::default()) | |
| .get_signature() | |
| .to_vec(), | |
| payment_id: txn.payment_id.to_bytes(), | |
| mined_in_block_height: txn.mined_height.unwrap_or(0), | |
| }) | |
| .collect(); | |
| trace!(target: LOG_TARGET, "'get_block_height_transactions' completed in {:.2?}", start.elapsed()); | |
| Ok(Response::new(GetBlockHeightTransactionsResponse { transactions })) | |
| } | |
| async fn get_block_height_transactions( | |
| &self, | |
| request: Request<GetBlockHeightTransactionsRequest>, | |
| ) -> Result<Response<GetBlockHeightTransactionsResponse>, Status> { | |
| let start = std::time::Instant::now(); | |
| trace!( | |
| target: LOG_TARGET, | |
| "GetBlockHeightTransactions: Incoming GRPC request" | |
| ); | |
| let message = request.into_inner(); | |
| let block_height = message.block_height; | |
| let mut transaction_service = self.get_transaction_service(); | |
| - let transactions = transaction_service | |
| - .get_completed_transactions(None, None, Some(block_height)) | |
| - .await | |
| - .map_err(|err| Status::not_found(format!("No transactions found at block height {}: {:?}", block_height, err)))?; | |
| + let transactions = match transaction_service | |
| + .get_completed_transactions(None, None, Some(block_height)) | |
| + .await | |
| + { | |
| + Ok(txns) => txns, | |
| + Err(err) => { | |
| + debug!( | |
| + target: LOG_TARGET, | |
| + "No transactions found at block height {}: {:?}", block_height, err | |
| + ); | |
| + vec![] | |
| + } | |
| + }; | |
| debug!( | |
| target: LOG_TARGET, | |
| "GetBlockHeightTransactions: Found {} transactions at block height {}", | |
| transactions.len(), | |
| block_height | |
| ); | |
| let transactions = transactions | |
| .iter() | |
| .map(|txn| TransactionInfo { | |
| tx_id: txn.tx_id.into(), | |
| source_address: txn.source_address.to_vec(), | |
| dest_address: txn.destination_address.to_vec(), | |
| status: TransactionStatus::from(txn.status.clone()) as i32, | |
| amount: txn.amount.into(), | |
| is_cancelled: txn.cancelled.is_some(), | |
| direction: TransactionDirection::from(txn.direction.clone()) as i32, | |
| fee: txn.fee.into(), | |
| timestamp: txn.timestamp.timestamp() as u64, | |
| excess_sig: txn | |
| .transaction | |
| .first_kernel_excess_sig() | |
| .unwrap_or(&Signature::default()) | |
| .get_signature() | |
| .to_vec(), | |
| payment_id: txn.payment_id.to_bytes(), | |
| mined_in_block_height: txn.mined_height.unwrap_or(0), | |
| }) | |
| .collect(); | |
| trace!(target: LOG_TARGET, "'get_block_height_transactions' completed in {:.2?}", start.elapsed()); | |
| Ok(Response::new(GetBlockHeightTransactionsResponse { transactions })) | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs between
lines 1008 and 1058, the current code returns a not_found error if no
transactions are found at the specified block height. Instead, modify the code
around line 1024 to check if the returned transactions list is empty and, if so,
proceed without error by returning an empty list. This means removing the
map_err that converts empty results into an error and allowing the function to
return an empty transactions vector in the response, aligning with typical API
behavior for list endpoints.
Test Results (CI) 2 files 60 suites 35m 36s ⏱️ For more details on these failures, see this check. Results for commit ba078e4. |
Test Results (Integration tests) 2 files + 2 11 suites +11 1h 47m 56s ⏱️ + 1h 47m 56s For more details on these failures, see this check. Results for commit ba078e4. ± Comparison against base commit 7ab4d16. |
…to krakaw-add-grpc-tx-method
2 participants
Description
Added a non-streaming block transactions grpc method
Motivation and Context
We needed a non-streaming block transactions grpc method
How Has This Been Tested?
Tested locally
What process can a PR reviewer use to test or verify this change?
Request a block your wallet has gotten transactions on, they should appear in the result.
Breaking Changes
Summary by CodeRabbit
New Features
Documentation
Style