feat!: remove the ability to send completely raw bytes via grpc#7117
feat!: remove the ability to send completely raw bytes via grpc#7117SWvheerden merged 3 commits intotari-project:developmentfrom
Conversation
WalkthroughThe changes enrich transaction-related gRPC responses by including detailed input and output commitment data. The wallet gRPC server methods now asynchronously fetch full transaction details and convert them into enhanced Changes
Possibly related PRs
Poem
✨ 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) 3 files 99 suites 48m 51s ⏱️ For more details on these failures, see this check. Results for commit fa2a809. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 1 errors 2 suites +2 27m 39s ⏱️ + 27m 39s For more details on these parsing errors and failures, see this check. Results for commit fa2a809. ± Comparison against base commit 1382008. ♻️ 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: 1
🧹 Nitpick comments (3)
applications/minotari_app_grpc/proto/wallet.proto (1)
1198-1199: Document newly added commitments fields in TransactionInfo.The new
input_commitments(tag 15) andoutput_commitments(tag 16) fields are essential for exposing cryptographic commitments. For better clarity in the generated API docs, consider adding inline comments describing:
- The format and expected encoding of each commitment (e.g., serialized Pedersen commitment bytes).
- Any size or count constraints clients should anticipate.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
598-630: Good enhancement with proper error handling.The method now returns enriched transaction information on success and properly handles errors by setting
transaction_infotoNone. The async operations are well-structured with appropriate error propagation.Note: Consider the performance implications of the additional async operations for fetching wallet address and transaction details.
819-862: Good enrichment of transfer results with consideration for performance.The method now provides enriched transaction information for each transfer result. The error handling is appropriate, and the structure maintains consistency with other methods.
Consider performance optimization for batch transfers, as fetching wallet address and transaction details for each transfer could be expensive. You might want to cache the wallet address since it's the same for all transfers in a batch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(14 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (11)
base_layer/core/src/transactions/transaction_protocol/sender.rs (2)
35-35: LGTM - Import addition supports new functionality.The
UncompressedCommitmentimport is correctly added to support the fallback mechanism in the newget_input_commitmentsmethod.
811-837: LGTM - Well-implemented output commitment retrieval.The method properly handles all transaction states and correctly collects commitments from outputs, change outputs, and recipient outputs. Error handling is appropriate with
InvalidStateErrorfor failed states.applications/minotari_app_grpc/proto/wallet.proto (1)
1154-1154: Append transaction_info to TransferResult for richer metadata.Adding the
transaction_infofield (tag 5) toTransferResultis correct and aligns with the server enhancements for returning full transaction details. Ensure that:
- The gRPC server handlers are updated to populate this new field in all code paths.
- Client stubs are regenerated and validated to handle the additional field without breaking existing integrations.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (8)
124-125: LGTM! Necessary imports for new commitment functionality.The added imports support the new async commitment fetching functionality and recipient state handling.
753-753: LGTM! Aligns with PR objectives for raw byte handling.This change correctly implements the PR objective by placing raw bytes into the open format with explicit transaction type, preventing confusion about payment ID format.
675-717: Consistent implementation with proper error handling.The changes follow the same pattern as the
claim_sha_atomic_swap_transactionmethod, maintaining consistency across the codebase while properly enriching transaction information.
934-956: Proper integration with async conversion function.The method correctly uses the new async conversion function with proper error handling for missing transactions.
1085-1104: Consistent commitment extraction with proper error handling.The inline extraction of output and input commitments is well-implemented with appropriate error handling for cases where input commitment extraction might fail.
1206-1248: Good consistency across transaction retrieval methods.The commitment extraction follows the same pattern as other methods, maintaining consistency in the codebase.
1287-1329: Proper implementation of commitment extraction.The method correctly extracts commitment data following the established pattern used in other transaction retrieval methods.
1675-1782: Excellent comprehensive refactor for commitment enrichment.This refactor successfully enables commitment data enrichment across all transaction states:
- PendingInbound: Correctly extracts commitments from finalized recipient state
- PendingOutbound: Properly uses async methods to fetch commitments from sender protocol with robust error handling
- Completed: Directly extracts commitments from transaction body
The error handling strategy is well-designed - logging warnings while gracefully degrading to empty vectors prevents transaction info retrieval failures while still providing commitment data when available.
The function signature change to async with
TransactionKeyManagerInterfaceparameter is necessary and well-integrated throughout the codebase.
| pub async fn get_input_commitments<KM: TransactionKeyManagerInterface>( | ||
| &self, | ||
| km: &KM, | ||
| ) -> Result<Vec<CompressedCommitment>, TPE> { | ||
| match &self.state { | ||
| SenderState::Initializing(info) | | ||
| SenderState::Finalizing(info) | | ||
| SenderState::SingleRoundMessageReady(info) | | ||
| SenderState::CollectingSingleSignature(info) => { | ||
| let mut commitments = Vec::new(); | ||
| for output in &info.inputs { | ||
| commitments.push(output.output.to_transaction_output(km).await?.commitment); | ||
| } | ||
| Ok(commitments) | ||
| }, | ||
| SenderState::FinalizedTransaction(txn) => Ok(txn | ||
| .body | ||
| .inputs() | ||
| .iter() | ||
| .map(|i| { | ||
| i.commitment() | ||
| .unwrap_or(&CompressedCommitment::from_commitment(UncompressedCommitment::default())) | ||
| .clone() | ||
| }) | ||
| .collect()), | ||
| SenderState::Failed(_) => Err(TPE::InvalidStateError), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the fallback mechanism for missing input commitments.
The method implementation is generally good, but the fallback to a default commitment for missing input commitments in finalized transactions (lines 859-861) is concerning:
i.commitment()
.unwrap_or(&CompressedCommitment::from_commitment(UncompressedCommitment::default()))
.clone()In a properly finalized transaction, all input commitments should be present. Using a default commitment could mask data integrity issues and lead to incorrect commitment data being returned.
Consider one of these approaches:
- Return an error if any input commitment is missing in a finalized transaction
- Log a warning and document why default commitments might be acceptable
- Add validation to ensure finalized transactions have complete commitment data
- .map(|i| {
- i.commitment()
- .unwrap_or(&CompressedCommitment::from_commitment(UncompressedCommitment::default()))
- .clone()
- })
+ .map(|i| {
+ i.commitment()
+ .ok_or(TPE::InvalidStateError)
+ .map(|c| c.clone())
+ })
+ .collect::<Result<Vec<_>, _>>()?,🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_protocol/sender.rs around lines
839 to 866, the current fallback to a default commitment when input commitments
are missing in finalized transactions can mask data integrity issues. Instead,
modify the code to check if each input commitment is present and return an error
if any are missing. This ensures that only valid, complete commitment data is
returned and prevents silent failures or incorrect data propagation.
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Just one comment on raw payment id.
Supplemented transaction onfo looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
1086-1128: 🛠️ Refactor suggestionGood commitment data extraction with proper error handling.
The commitment extraction enhances the transaction info with valuable cryptographic data. The error handling for input commitments is appropriate.
Consider extracting the commitment extraction logic into a helper function since this exact pattern is duplicated across
get_completed_transactions,get_all_completed_transactions, andget_block_height_transactionsmethods.fn extract_commitments(txn: &CompletedTransaction) -> (Vec<Vec<u8>>, Vec<Vec<u8>>) { let output_commitments = txn .transaction .body .outputs() .iter() .map(|o| o.commitment().as_bytes().to_vec()) .collect(); let input_commitments = txn .transaction .body .inputs() .iter() .map(|i| match i.commitment() { Ok(c) => c.as_bytes().to_vec(), Err(e) => { warn!(target: LOG_TARGET, "Failed to get input commitment: {}", e); vec![] }, }) .collect(); (output_commitments, input_commitments) }
♻️ Duplicate comments (3)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)
675-707: Same enhancement pattern as previous method.This follows the identical enrichment pattern as the
claim_sha_atomic_swap_transactionmethod.
1207-1249: Duplicate commitment extraction pattern.This follows the same commitment extraction pattern already reviewed in the
get_completed_transactionsmethod.
1288-1331: Duplicate commitment extraction pattern.This follows the same commitment extraction pattern already reviewed in the
get_completed_transactionsmethod.
🧹 Nitpick comments (2)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
598-630: Good enhancement with proper error handling.The enriched transaction response provides valuable commitment data. The async calls are properly handled with appropriate error conversion to gRPC Status types.
Consider extracting the common pattern of fetching wallet address and converting transaction to enriched info into a helper method since this pattern is repeated across multiple methods.
819-862: Enhancement is valuable but consider performance optimization.The enriched transaction responses provide excellent detail. However, the sequential async calls in the loop could impact performance for batch transfers.
Consider using
future::try_join_allto process the transaction enrichment calls in parallel for better performance with multiple transfers, similar to how the actual transfer operations are already handled.
📜 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(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (2)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
753-753: LGTM! Change aligns with PR objectives.This change correctly implements the PR objective by wrapping raw payment ID bytes in the open format instead of treating them as completely raw bytes. This addresses the user confusion mentioned in the PR description where raw data was assumed to be in the correct form.
934-956: LGTM! Proper async conversion with maintained error handling.The async conversion is necessary for the commitment retrieval functionality and maintains the same error handling patterns.
| #[allow(clippy::too_many_lines)] | ||
| async fn convert_wallet_transaction_into_transaction_info<KM: TransactionKeyManagerInterface>( | ||
| tx: models::WalletTransaction, | ||
| wallet_address: &TariAddress, | ||
| key_manager: &KM, | ||
| ) -> TransactionInfo { | ||
| use models::WalletTransaction::{Completed, PendingInbound, PendingOutbound}; | ||
| match tx { | ||
| PendingInbound(tx) => TransactionInfo { | ||
| tx_id: tx.tx_id.into(), | ||
| source_address: tx.source_address.to_vec(), | ||
| dest_address: wallet_address.to_vec(), | ||
| status: TransactionStatus::from(tx.status) as i32, | ||
| amount: tx.amount.into(), | ||
| is_cancelled: tx.cancelled, | ||
| direction: TransactionDirection::Inbound as i32, | ||
| fee: 0, | ||
| excess_sig: Default::default(), | ||
| timestamp: tx.timestamp.timestamp() as u64, | ||
| raw_payment_id: tx.payment_id.to_bytes(), | ||
| user_payment_id: tx.payment_id.user_data_as_bytes(), | ||
| mined_in_block_height: 0, | ||
| PendingInbound(tx) => { | ||
| let output_commitments = match tx.receiver_protocol.state { | ||
| RecipientState::Finalized(data) => vec![data.output.commitment.as_bytes().to_vec()], | ||
| _ => vec![], | ||
| }; | ||
| TransactionInfo { | ||
| tx_id: tx.tx_id.into(), | ||
| source_address: tx.source_address.to_vec(), | ||
| dest_address: wallet_address.to_vec(), | ||
| status: TransactionStatus::from(tx.status) as i32, | ||
| amount: tx.amount.into(), | ||
| is_cancelled: tx.cancelled, | ||
| direction: TransactionDirection::Inbound as i32, | ||
| fee: 0, | ||
| excess_sig: Default::default(), | ||
| timestamp: tx.timestamp.timestamp() as u64, | ||
| raw_payment_id: tx.payment_id.to_bytes(), | ||
| user_payment_id: tx.payment_id.user_data_as_bytes(), | ||
| mined_in_block_height: 0, | ||
| output_commitments, | ||
| input_commitments: vec![], | ||
| } | ||
| }, | ||
| PendingOutbound(tx) => TransactionInfo { | ||
| tx_id: tx.tx_id.into(), | ||
| source_address: wallet_address.to_vec(), | ||
| dest_address: tx.destination_address.to_vec(), | ||
| status: TransactionStatus::from(tx.status) as i32, | ||
| amount: tx.amount.into(), | ||
| is_cancelled: tx.cancelled, | ||
| direction: TransactionDirection::Outbound as i32, | ||
| fee: tx.fee.into(), | ||
| excess_sig: Default::default(), | ||
| timestamp: tx.timestamp.timestamp() as u64, | ||
| raw_payment_id: tx.payment_id.to_bytes(), | ||
| user_payment_id: tx.payment_id.user_data_as_bytes(), | ||
| mined_in_block_height: 0, | ||
| PendingOutbound(tx) => { | ||
| let output_commitments = match tx.sender_protocol.get_output_commitments(key_manager).await { | ||
| Ok(v) => v.into_iter().map(|c| c.as_bytes().to_vec()).collect(), | ||
| Err(e) => { | ||
| warn!(target: LOG_TARGET, "Failed to get output commitments: {}", e); | ||
| vec![] | ||
| }, | ||
| }; | ||
| let input_commitments = match tx.sender_protocol.get_input_commitments(key_manager).await { | ||
| Ok(v) => v.into_iter().map(|c| c.as_bytes().to_vec()).collect(), | ||
| Err(e) => { | ||
| warn!(target: LOG_TARGET, "Failed to get output commitments: {}", e); | ||
| vec![] | ||
| }, | ||
| }; | ||
| TransactionInfo { | ||
| tx_id: tx.tx_id.into(), | ||
| source_address: wallet_address.to_vec(), | ||
| dest_address: tx.destination_address.to_vec(), | ||
| status: TransactionStatus::from(tx.status) as i32, | ||
| amount: tx.amount.into(), | ||
| is_cancelled: tx.cancelled, | ||
| direction: TransactionDirection::Outbound as i32, | ||
| fee: tx.fee.into(), | ||
| excess_sig: Default::default(), | ||
| timestamp: tx.timestamp.timestamp() as u64, | ||
| raw_payment_id: tx.payment_id.to_bytes(), | ||
| user_payment_id: tx.payment_id.user_data_as_bytes(), | ||
| mined_in_block_height: 0, | ||
| output_commitments, | ||
| input_commitments, | ||
| } | ||
| }, | ||
| Completed(tx) => TransactionInfo { | ||
| tx_id: tx.tx_id.into(), | ||
| source_address: tx.source_address.to_vec(), | ||
| dest_address: tx.destination_address.to_vec(), | ||
| status: TransactionStatus::from(tx.status) as i32, | ||
| amount: tx.amount.into(), | ||
| is_cancelled: tx.cancelled.is_some(), | ||
| direction: TransactionDirection::from(tx.direction) as i32, | ||
| fee: tx.fee.into(), | ||
| timestamp: tx.timestamp.timestamp() as u64, | ||
| excess_sig: tx | ||
| Completed(tx) => { | ||
| let output_commitments = tx | ||
| .transaction | ||
| .first_kernel_excess_sig() | ||
| .map(|s| s.get_signature().to_vec()) | ||
| .unwrap_or_default(), | ||
| raw_payment_id: tx.payment_id.to_bytes(), | ||
| user_payment_id: tx.payment_id.user_data_as_bytes(), | ||
| mined_in_block_height: tx.mined_height.unwrap_or(0), | ||
| .body | ||
| .outputs() | ||
| .iter() | ||
| .map(|o| o.commitment().as_bytes().to_vec()) | ||
| .collect(); | ||
| let input_commitments = tx | ||
| .transaction | ||
| .body | ||
| .inputs() | ||
| .iter() | ||
| .map(|i| match i.commitment() { | ||
| Ok(c) => c.as_bytes().to_vec(), | ||
| Err(e) => { | ||
| warn!(target: LOG_TARGET, "Failed to get input commitment: {}", e); | ||
| vec![] | ||
| }, | ||
| }) | ||
| .collect(); | ||
| TransactionInfo { | ||
| tx_id: tx.tx_id.into(), | ||
| source_address: tx.source_address.to_vec(), | ||
| dest_address: tx.destination_address.to_vec(), | ||
| status: TransactionStatus::from(tx.status) as i32, | ||
| amount: tx.amount.into(), | ||
| is_cancelled: tx.cancelled.is_some(), | ||
| direction: TransactionDirection::from(tx.direction) as i32, | ||
| fee: tx.fee.into(), | ||
| timestamp: tx.timestamp.timestamp() as u64, | ||
| excess_sig: tx | ||
| .transaction | ||
| .first_kernel_excess_sig() | ||
| .map(|s| s.get_signature().to_vec()) | ||
| .unwrap_or_default(), | ||
| raw_payment_id: tx.payment_id.to_bytes(), | ||
| user_payment_id: tx.payment_id.user_data_as_bytes(), | ||
| mined_in_block_height: tx.mined_height.unwrap_or(0), | ||
| output_commitments, | ||
| input_commitments, | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Major refactor enables commitment functionality but verify breaking changes.
The async conversion and commitment retrieval significantly enhance the transaction info. However, there are a couple of issues to address:
Fix incorrect error message on line 1718:
- warn!(target: LOG_TARGET, "Failed to get output commitments: {}", e);
+ warn!(target: LOG_TARGET, "Failed to get input commitments: {}", e);The function signature change from synchronous to asynchronous is a breaking change. Verify that all callers of convert_wallet_transaction_into_transaction_info have been updated to handle the async nature.
🏁 Script executed:
#!/bin/bash
# Search for all calls to convert_wallet_transaction_into_transaction_info
# to verify they are properly awaited
rg -A 3 -B 3 "convert_wallet_transaction_into_transaction_info"Length of output: 4316
🏁 Script executed:
#!/bin/bash
# Show code around the loop that pushes converted transactions to verify `.await` usage
rg -n -C5 "for (tx_id, tx)" applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rsLength of output: 97
🏁 Script executed:
#!/bin/bash
# Display context around where transactions are collected to verify .await usage
rg -n -C10 "transactions.push" applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rsLength of output: 986
Async usage verified; fix incorrect input commitments log
All calls to convert_wallet_transaction_into_transaction_info are properly .awaited, so there are no remaining breaking‐change concerns. Only the copy/paste error in the warning message needs correction:
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (PendingOutbound branch):
- warn!(target: LOG_TARGET, "Failed to get output commitments: {}", e);
+ warn!(target: LOG_TARGET, "Failed to get input commitments: {}", e);🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs around
lines 1680 to 1710, the warning message for the error when getting input
commitments in the PendingOutbound match arm incorrectly repeats "Failed to get
output commitments". Update this log message to correctly say "Failed to get
input commitments" to accurately reflect the error context.
2 participants
Description
Changes the transfer command, that if you provide the raw bytes, it puts it into the open format.
This does change legacy behaviour, but most people who use this are not aware that the bytes if provided in raw from need to contain some other details as well.
By providing raw data to the wallet it thinks its i nthe correct form, but its not and this leads to weird looking payment id's
Summary by CodeRabbit