feat: import transactions via grpc#7078
Conversation
|
""" WalkthroughA new gRPC method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletGrpcServer
participant TransactionService
participant EventPublisher
participant UI
Client->>WalletGrpcServer: ImportTransactions(JSON string)
WalletGrpcServer->>WalletGrpcServer: Deserialize JSON to WalletTransaction list
loop For each WalletTransaction
WalletGrpcServer->>TransactionService: Import transaction
TransactionService->>EventPublisher: Publish TransactionImported event
EventPublisher->>UI: Notify TransactionImported(tx_id)
TransactionService-->>WalletGrpcServer: Result (success/failure)
end
WalletGrpcServer->>Client: ImportTransactionsResponse(tx_ids)
Possibly related PRs
Poem
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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 (
|
Test Results (CI)1 090 tests 1 086 ✅ 16m 18s ⏱️ For more details on these failures, see this check. Results for commit e952975. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 11 suites +11 1h 14m 5s ⏱️ + 1h 14m 5s For more details on these failures, see this check. Results for commit e952975. ± Comparison against base commit 7ab4d16. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
applications/minotari_app_grpc/proto/wallet.proto (4)
1000-1002: Missing Documentation for New RPC
The newImportTransactionsRPC lacks a descriptive comment block. Please add a doc comment above it, outlining:
- Purpose of the RPC
- Details of the
file_pathparameter- Expected behavior and error codes
- Example usage
1360-1363: Add Documentation for ImportTransactionsRequest
TheImportTransactionsRequestmessage is undocumented. Add a proto comment describing thefile_pathfield, its expected format (e.g., newline-delimited JSON), any path restrictions, and validation rules.
1365-1367: Add Documentation for ImportTransactionsResponse
TheImportTransactionsResponsemessage is missing comments. Describe thattx_idsis the list of successfully imported transaction IDs and under what conditions it may be empty or partially populated.
1365-1367: Optional: Renametx_idsfor Consistency
Other RPCs usetransaction_ids(e.g.,GetTransactionInfoRequest). Consider renamingtx_idstotransaction_idsfor clarity and consistency across the API.applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
1222-1223: Consider using the logging framework instead of println for consistency.Using
printlnfor logging successful transaction imports is inconsistent with the project's logging practices. The rest of the codebase uses the logging framework with theLOG_TARGETconstant.- println!("Imported tx: {}", id); + info!(target: LOG_TARGET, "Imported tx: {}", id);
1225-1226: Replace eprintln with proper error logging.Using
eprintlnfor error logging is inconsistent with the project's logging practices and doesn't provide the proper context information.- Err(e) => eprintln!("Could not import tx {}", e), + Err(e) => error!(target: LOG_TARGET, "Could not import tx: {}", e),
📜 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 selected for processing (3)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check-title
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (2)
applications/minotari_console_wallet/src/automation/commands.rs (1)
2064-2067: Provide immediate console feedback for stealth sends
Addingprintln!("Transaction ID: {}", tx_id);gives end users clear confirmation of the generated transaction ID when using theSendOneSidedToStealthAddresscommand.applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
36-36: Appropriately updated imports to include the new transaction request/response types.The import statement now includes the new
ImportTransactionsRequestandImportTransactionsResponsetypes necessary for the new gRPC endpoint.
| let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?; | ||
| let mut txs: Vec<WalletTransaction> = Vec::new(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add security validation for file path access.
The method reads a file path from user input without validation, which could potentially lead to directory traversal attacks. Consider implementing path validation or restricting file access to a specific directory.
You could add validation like this:
- let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?;
+ // Validate file path to prevent directory traversal attacks
+ let path = std::path::Path::new(&request.file_path);
+ if !path.is_file() {
+ return Err(Status::invalid_argument(format!("'{}' is not a valid file", request.file_path)));
+ }
+
+ // Optionally, restrict to a specific directory
+ let allowed_dir = std::path::Path::new("/path/to/allowed/directory");
+ if !path.starts_with(allowed_dir) {
+ return Err(Status::permission_denied("Access to this file path is not allowed"));
+ }
+
+ let file_contents = std::fs::read_to_string(request.file_path)
+ .map_err(|e| Status::invalid_argument(format!("Failed to read file: {}", e)))?;Run a security scan to check for similar file I/O operations across the codebase:
🏁 Script executed:
#!/bin/bash
# Search for potential file I/O operations without path validation
echo "Searching for file I/O operations that might need path validation"
rg "fs::(read|open|create)" --type=rustLength of output: 5855
Enforce safe file path handling in gRPC file read
The RPC handler in applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs reads request.file_path without any validation, allowing directory traversal or unauthorized access. Please add canonicalization and a directory whitelist:
- File: applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (lines 1209–1210)
- Ensure the path is canonicalized and resides under a known base directory
- Verify the canonical path points to an existing regular file
Suggested diff:
// Validate and restrict user-supplied file paths to prevent directory traversal
- let file_contents = std::fs::read_to_string(request.file_path)
- .map_err(|_| Status::invalid_argument("File path is malformed!"))?;
+ let requested = std::path::Path::new(&request.file_path);
+ let canonical = requested
+ .canonicalize()
+ .map_err(|_| Status::invalid_argument("Invalid file path"))?;
+ // Define and canonicalize your allowed directory
+ let base = std::path::Path::new("/var/lib/minotari/allowed");
+ let base_canonical = base
+ .canonicalize()
+ .map_err(|_| Status::internal("Server misconfiguration"))?;
+ if !canonical.starts_with(&base_canonical) {
+ return Err(Status::permission_denied("Access to this file is not allowed"));
+ }
+ if !canonical.is_file() {
+ return Err(Status::invalid_argument("Path is not a file"));
+ }
+ let file_contents = std::fs::read_to_string(&canonical)
+ .map_err(|e| Status::invalid_argument(format!("Failed to read file: {}", e)))?;📝 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.
| let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?; | |
| let mut txs: Vec<WalletTransaction> = Vec::new(); | |
| // Validate and restrict user-supplied file paths to prevent directory traversal | |
| let requested = std::path::Path::new(&request.file_path); | |
| let canonical = requested | |
| .canonicalize() | |
| .map_err(|_| Status::invalid_argument("Invalid file path"))?; | |
| // Define and canonicalize your allowed directory | |
| let base = std::path::Path::new("/var/lib/minotari/allowed"); | |
| let base_canonical = base | |
| .canonicalize() | |
| .map_err(|_| Status::internal("Server misconfiguration"))?; | |
| if !canonical.starts_with(&base_canonical) { | |
| return Err(Status::permission_denied("Access to this file is not allowed")); | |
| } | |
| if !canonical.is_file() { | |
| return Err(Status::invalid_argument("Path is not a file")); | |
| } | |
| let file_contents = std::fs::read_to_string(&canonical) | |
| .map_err(|e| Status::invalid_argument(format!("Failed to read file: {}", e)))?; | |
| let mut txs: Vec<WalletTransaction> = Vec::new(); |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs around
lines 1209 to 1210, the code reads a file path from user input without
validation, risking directory traversal attacks. Fix this by canonicalizing the
requested file path and verifying it is within a predefined base directory.
Also, check that the canonical path points to an existing regular file before
reading it. If validation fails, return an appropriate error status to prevent
unauthorized file access.
| async fn import_transactions( | ||
| &self, | ||
| request: Request<ImportTransactionsRequest>, | ||
| ) -> Result<Response<ImportTransactionsResponse>, Status> { | ||
| let request = request.into_inner(); | ||
| // Extract file contents | ||
| let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?; | ||
| let mut txs: Vec<WalletTransaction> = Vec::new(); | ||
| for line in file_contents.lines() { | ||
| serde_json::from_str(line) | ||
| .map(|tx| txs.push(tx)) | ||
| .map_err(|_| Status::invalid_argument("File path is malformed!"))?; | ||
| } | ||
| let mut transaction_service = self.get_transaction_service(); | ||
|
|
||
| let mut tx_ids = Vec::new(); | ||
| for tx in txs { | ||
| match transaction_service.import_transaction(tx).await { | ||
| Ok(id) => { | ||
| println!("Imported tx: {}", id); | ||
| tx_ids.push(id.into()); | ||
| }, | ||
| Err(e) => eprintln!("Could not import tx {}", e), | ||
| }; | ||
| } | ||
| Ok(Response::new(ImportTransactionsResponse { tx_ids })) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Improve error handling and file processing for the new import_transactions method.
The implementation of the new gRPC endpoint is functional but has several areas that could be improved:
- The file I/O error handling uses a generic "File path is malformed!" message for all errors, which is not very informative.
- Reading the entire file into memory at once could cause problems with large transaction files.
- Transaction import failures are only printed to stderr without returning this information to the client.
Here's a suggested implementation with improved error handling and streaming file processing:
async fn import_transactions(
&self,
request: Request<ImportTransactionsRequest>,
) -> Result<Response<ImportTransactionsResponse>, Status> {
let request = request.into_inner();
- // Extract file contents
- let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?;
- let mut txs: Vec<WalletTransaction> = Vec::new();
- for line in file_contents.lines() {
- serde_json::from_str(line)
- .map(|tx| txs.push(tx))
- .map_err(|_| Status::invalid_argument("File path is malformed!"))?;
- }
+ // Open the file with proper error handling
+ let file = std::fs::File::open(&request.file_path).map_err(|e|
+ Status::invalid_argument(format!("Failed to open file '{}': {}", request.file_path, e)))?;
+ let reader = std::io::BufReader::new(file);
+
+ // Process the file line by line instead of loading it all into memory
+ let mut txs: Vec<WalletTransaction> = Vec::new();
+ for (line_num, line_result) in std::io::BufRead::lines(reader).enumerate() {
+ let line = line_result.map_err(|e|
+ Status::invalid_argument(format!("Failed to read line {}: {}", line_num + 1, e)))?;
+
+ serde_json::from_str(&line)
+ .map(|tx| txs.push(tx))
+ .map_err(|e| Status::invalid_argument(
+ format!("Invalid JSON at line {}: {}", line_num + 1, e)
+ ))?;
+ }
+
let mut transaction_service = self.get_transaction_service();
let mut tx_ids = Vec::new();
+ let mut failed_imports = Vec::new();
for tx in txs {
match transaction_service.import_transaction(tx).await {
Ok(id) => {
println!("Imported tx: {}", id);
tx_ids.push(id.into());
},
- Err(e) => eprintln!("Could not import tx {}", e),
+ Err(e) => {
+ error!(target: LOG_TARGET, "Failed to import transaction: {}", e);
+ failed_imports.push(e.to_string());
+ }
};
}
+
+ if !failed_imports.is_empty() && tx_ids.is_empty() {
+ // If all imports failed, return an error
+ return Err(Status::internal(format!("All transaction imports failed: {}", failed_imports.join(", "))));
+ }
+
Ok(Response::new(ImportTransactionsResponse { tx_ids }))
}Additionally, consider adding:
- A check to verify the file exists before opening it
- A size limit for the file to prevent denial of service attacks
- A method to return both successful and failed transaction imports in the response
Improve error handling and streaming in import_transactions
This endpoint currently reads the entire file into memory, hides I/O errors behind a generic message, and never surfaces import failures to the client. Let’s address all three:
• File: applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Method: import_transactions (around lines 1203–1229)
• Replace read_to_string with File::open + BufReader for streaming and detailed errors
• Add per‐line JSON parse errors with line numbers
• Collect failed imports and return an error or include failures in the response
Suggested diff:
async fn import_transactions(
&self,
request: Request<ImportTransactionsRequest>,
) -> Result<Response<ImportTransactionsResponse>, Status> {
let request = request.into_inner();
- // Extract file contents
- let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?;
- let mut txs: Vec<WalletTransaction> = Vec::new();
- for line in file_contents.lines() {
- serde_json::from_str(line)
- .map(|tx| txs.push(tx))
- .map_err(|_| Status::invalid_argument("File path is malformed!"))?;
- }
+ // Open file with detailed error
+ let path = &request.file_path;
+ if !std::path::Path::new(path).exists() {
+ return Err(Status::invalid_argument(format!("File not found: {}", path)));
+ }
+ let file = std::fs::File::open(path).map_err(|e|
+ Status::invalid_argument(format!("Failed to open '{}': {}", path, e))
+ )?;
+ let reader = std::io::BufReader::new(file);
+
+ // Stream JSON lines with per‐line error reporting
+ let mut txs = Vec::new();
+ for (i, line_res) in reader.lines().enumerate() {
+ let line = line_res.map_err(|e|
+ Status::invalid_argument(format!("Read error at line {}: {}", i + 1, e))
+ )?;
+ let tx: WalletTransaction = serde_json::from_str(&line).map_err(|e|
+ Status::invalid_argument(format!("Invalid JSON at line {}: {}", i + 1, e))
+ )?;
+ txs.push(tx);
+ }
let mut transaction_service = self.get_transaction_service();
let mut tx_ids = Vec::new();
+ let mut failures = Vec::new();
for tx in txs {
match transaction_service.import_transaction(tx).await {
Ok(id) => {
println!("Imported tx: {}", id);
tx_ids.push(id.into());
}
- Err(e) => eprintln!("Could not import tx {}", e),
+ Err(e) => {
+ error!(target: LOG_TARGET, "Import failed: {}", e);
+ failures.push(e.to_string());
+ }
};
}
+ // If every import failed, return an error
+ if !failures.is_empty() && tx_ids.is_empty() {
+ return Err(Status::internal(format!(
+ "All imports failed: {}",
+ failures.join("; ")
+ )));
+ }
Ok(Response::new(ImportTransactionsResponse { tx_ids }))
}Additional considerations:
- Enforce a maximum file size before streaming to mitigate DoS.
- Extend the response (e.g. add a
failed_tx_idsor error details field) so clients can reconcile partial failures. - Log at appropriate levels and include
LOG_TARGETconstant if not already defined.
📝 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 import_transactions( | |
| &self, | |
| request: Request<ImportTransactionsRequest>, | |
| ) -> Result<Response<ImportTransactionsResponse>, Status> { | |
| let request = request.into_inner(); | |
| // Extract file contents | |
| let file_contents = std::fs::read_to_string(request.file_path).map_err(|_| Status::invalid_argument("File path is malformed!"))?; | |
| let mut txs: Vec<WalletTransaction> = Vec::new(); | |
| for line in file_contents.lines() { | |
| serde_json::from_str(line) | |
| .map(|tx| txs.push(tx)) | |
| .map_err(|_| Status::invalid_argument("File path is malformed!"))?; | |
| } | |
| let mut transaction_service = self.get_transaction_service(); | |
| let mut tx_ids = Vec::new(); | |
| for tx in txs { | |
| match transaction_service.import_transaction(tx).await { | |
| Ok(id) => { | |
| println!("Imported tx: {}", id); | |
| tx_ids.push(id.into()); | |
| }, | |
| Err(e) => eprintln!("Could not import tx {}", e), | |
| }; | |
| } | |
| Ok(Response::new(ImportTransactionsResponse { tx_ids })) | |
| } | |
| async fn import_transactions( | |
| &self, | |
| request: Request<ImportTransactionsRequest>, | |
| ) -> Result<Response<ImportTransactionsResponse>, Status> { | |
| let request = request.into_inner(); | |
| // Open file with detailed error | |
| let path = &request.file_path; | |
| if !std::path::Path::new(path).exists() { | |
| return Err(Status::invalid_argument(format!("File not found: {}", path))); | |
| } | |
| let file = std::fs::File::open(path).map_err(|e| { | |
| Status::invalid_argument(format!("Failed to open '{}': {}", path, e)) | |
| })?; | |
| let reader = std::io::BufReader::new(file); | |
| // Stream JSON lines with per-line error reporting | |
| let mut txs = Vec::new(); | |
| for (i, line_res) in reader.lines().enumerate() { | |
| let line = line_res.map_err(|e| { | |
| Status::invalid_argument(format!("Read error at line {}: {}", i + 1, e)) | |
| })?; | |
| let tx: WalletTransaction = serde_json::from_str(&line).map_err(|e| { | |
| Status::invalid_argument(format!("Invalid JSON at line {}: {}", i + 1, e)) | |
| })?; | |
| txs.push(tx); | |
| } | |
| let mut transaction_service = self.get_transaction_service(); | |
| let mut tx_ids = Vec::new(); | |
| let mut failures = Vec::new(); | |
| for tx in txs { | |
| match transaction_service.import_transaction(tx).await { | |
| Ok(id) => { | |
| println!("Imported tx: {}", id); | |
| tx_ids.push(id.into()); | |
| } | |
| Err(e) => { | |
| error!(target: LOG_TARGET, "Import failed: {}", e); | |
| failures.push(e.to_string()); | |
| } | |
| }; | |
| } | |
| // If every import failed, return an error | |
| if !failures.is_empty() && tx_ids.is_empty() { | |
| return Err(Status::internal(format!( | |
| "All imports failed: {}", | |
| failures.join("; ") | |
| ))); | |
| } | |
| Ok(Response::new(ImportTransactionsResponse { tx_ids })) | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs around
lines 1203 to 1229, improve import_transactions by replacing read_to_string with
streaming file reading using File::open and BufReader to handle large files
efficiently. Add a file existence check and enforce a maximum file size limit
before processing to prevent DoS attacks. Enhance error handling by providing
detailed I/O error messages instead of a generic "File path is malformed!"
message, and include per-line JSON parsing errors with line numbers. Collect
both successful and failed transaction imports, then extend the
ImportTransactionsResponse to return this information to the client, allowing
them to reconcile partial failures. Finally, ensure proper logging with
appropriate log levels and include a LOG_TARGET constant if missing.
|
|
SWvheerden
changed the title
2 participants
Description
In Tari Universe we want to import tx when view wallet instance is running. It's only possible via grpc due to exclusive file lock.
Motivation and Context
How Has This Been Tested?
https://drive.google.com/file/d/1iV-cLxqvnTBX0QSs6lAzqg55Zn5aGOu2/view
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
Summary by CodeRabbit