feat: full PayRef implementation#7154
Conversation
WalkthroughThis update introduces comprehensive Payment Reference (PayRef) support across the Tari blockchain, wallet, node, FFI, and documentation. It adds PayRef generation, storage, querying, and verification mechanisms, including database schema migrations, gRPC and FFI APIs, CLI and UI enhancements, and extensive documentation for user and developer integration. Numerous code and schema adjustments facilitate PayRef-based payment identification and verification. Changes
Sequence Diagram(s)sequenceDiagram
participant WalletUser
participant ConsoleWallet
participant WalletService
participant NodeService
participant ChainStorage
WalletUser->>ConsoleWallet: Search/Show PayRef or ListTx
ConsoleWallet->>WalletService: get_payment_by_reference(payref) / get_transaction_pay_refs(tx_id)
WalletService->>NodeService: (if needed) fetch_output_by_payref(payref)
NodeService->>ChainStorage: fetch_output_by_payref(payref)
ChainStorage-->>NodeService: OutputMinedInfo (if found)
NodeService-->>WalletService: Output info or not found
WalletService-->>ConsoleWallet: PaymentDetails or PayRefs list
ConsoleWallet-->>WalletUser: Display PayRef info/status
sequenceDiagram
participant Exchange
participant WalletGrpc
participant WalletDB
participant BaseNodeGrpc
participant BaseNodeDB
Exchange->>WalletGrpc: GetPaymentByReference(payref)
WalletGrpc->>WalletDB: Query transaction by payref
WalletDB-->>WalletGrpc: TransactionInfo or not found
WalletGrpc-->>Exchange: TransactionInfo or error
Exchange->>BaseNodeGrpc: SearchPaymentReferences([payref])
BaseNodeGrpc->>BaseNodeDB: fetch_output_by_payref(payref)
BaseNodeDB-->>BaseNodeGrpc: OutputMinedInfo or not found
BaseNodeGrpc-->>Exchange: PaymentReferenceResponse stream
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 126 suites 41m 2s ⏱️ Results for commit 0ce902f. ♻️ 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: 13
🔭 Outside diff range comments (3)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
547-587:⚠️ Potential issueAdd PayRef index to database statistics collection.
The
payref_to_output_indexdatabase is missing from theall_dbs()method, which means it won't be included in database statistics and monitoring.fn all_dbs(&self) -> [(&'static str, &DatabaseRef); 31] { +fn all_dbs(&self) -> [(&'static str, &DatabaseRef); 32] { [ (LMDB_DB_METADATA, &self.metadata_db), // ... other databases ... (LMDB_DB_UNIQUE_ID_INDEX, &self.unique_id_index), + (LMDB_DB_PAYREF_TO_OUTPUT_INDEX, &self.payref_to_output_index), ( LMDB_DB_DELETED_TXO_HASH_TO_HEADER_INDEX, &self.deleted_txo_hash_to_header_index, ),applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
1173-1239:⚠️ Potential issueExtract duplicated output fetching logic.
This code duplicates the exact logic from
calculate_payment_references_for_transaction. Consider using the helper method instead of duplicating the code.-// Calculate PayRef for this transaction's first output (if mined) -let mut payment_refs = Vec::new(); - -// Get ALL wallet outputs (both spent and unspent) from output manager -let unspent_outputs = match output_manager.get_unspent_outputs().await { - Ok(outputs) => outputs, - Err(e) => { - warn!(target: LOG_TARGET, "Failed to get unspent outputs for PayRef calculation: {}", e); - Vec::new() - } -}; - -let spent_outputs = match output_manager.get_spent_outputs().await { - Ok(outputs) => outputs, - Err(e) => { - warn!(target: LOG_TARGET, "Failed to get spent outputs for PayRef calculation: {}", e); - Vec::new() - } -}; - -// Combine both spent and unspent outputs -let all_outputs: Vec<_> = unspent_outputs.into_iter().chain(spent_outputs.into_iter()).collect(); - -for commitment_bytes in &output_commitments { - // Find matching DbWalletOutput by commitment - if let Some(db_output) = all_outputs.iter() - .find(|o| o.commitment.as_bytes() == commitment_bytes) - { - // Check if output has mining data (mined_height and mined_in_block) - if db_output.mined_height.is_some() && db_output.mined_in_block.is_some() { - // Use existing PayRef generation method - if let Some(payref) = db_output.generate_payment_reference() { - payment_refs.push(payref.to_vec()); - } else { - payment_refs.push(vec![]); - } - } else { - payment_refs.push(vec![]); - } - } else { - payment_refs.push(vec![]); - } -} - -let payment_reference = payment_refs.get(0).cloned().unwrap_or_default().to_hex(); +// Use the helper method to calculate payment references +let server = self.clone(); // Or pass self if the spawned task allows it +let payment_refs = server.calculate_payment_references_for_transaction(&output_commitments, txn.mined_height.unwrap_or(0)).await; +let payment_reference = payment_refs.get(0).cloned().unwrap_or_default().to_hex();base_layer/wallet/src/output_manager_service/payment_reference.rs (1)
261-262:⚠️ Potential issueCritical: Add comprehensive tests for PayRef functionality.
This TODO indicates missing tests for a new critical feature. Payment Reference functionality should be thoroughly tested before deployment to ensure correctness and prevent potential financial issues.
Would you like me to generate comprehensive unit tests for the PayRef functionality or open a new issue to track this task?
♻️ Duplicate comments (2)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)
3117-3117: Consider using actual revealed amount instead of minimum value promise.Same consideration as in
search_payment_referencesregarding revealed amounts.
3102-3103:⚠️ Potential issueFix compilation error:
input.output_hash()method does not exist.Same issue as in
search_payment_references. The spending transaction hash should be obtained differently.
🧹 Nitpick comments (20)
applications/minotari_console_wallet/src/automation/commands.rs (1)
2714-2765: Acknowledge incomplete functionality but suggest enhancement for future iterations.The ShowPayRef command correctly retrieves and displays transaction details, but the payment reference correlation is incomplete. While the code appropriately acknowledges this limitation, consider implementing a more direct association between transactions and their payment references in future iterations.
For enhanced functionality, consider:
- // Try to find associated payment references - match output_service.get_all_payment_references().await { - Ok(payment_refs) => { - if payment_refs.is_empty() { - println!("\nNo payment references are available yet."); - println!("This may happen if:"); - println!("- No transactions have been mined and confirmed"); - println!("- Payment reference generation is not enabled"); - } else { - println!("\nNote: Payment reference correlation with specific transactions"); - println!("requires additional implementation. Use 'list-payrefs' to see all available"); - println!("payment references and correlate manually based on amount and timestamp."); - } - }, - Err(e) => eprintln!("Error getting payment references: {}", e), - } + // Try to find associated payment references for this specific transaction + match output_service.get_payment_references_for_transaction(args.transaction_id.into()).await { + Ok(payment_refs) => { + if payment_refs.is_empty() { + println!("\nNo payment references found for this transaction."); + } else { + println!("\nAssociated payment references:"); + for payref in payment_refs { + println!(" Payment Reference: {}", payref.payref_hex()); + } + } + }, + Err(e) => eprintln!("Error getting payment references: {}", e), + }applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
3017-3017: Consider using actual revealed amount instead of minimum value promise.The
minimum_value_promiseis a lower bound guarantee, not necessarily the actual revealed amount. Consider checking if the output has a revealed value proof to provide the actual amount.Consider adding logic to check for revealed value proofs:
-revealed_amount: Some(output_info.output.minimum_value_promise.as_u64()), +revealed_amount: if output_info.output.revealed_value_proof.is_some() { + Some(output_info.output.value.as_u64()) +} else { + Some(output_info.output.minimum_value_promise.as_u64()) +},base_layer/wallet/src/output_manager_service/storage/models.rs (1)
112-144: Add documentation for unsynced wallet handling behavior.The special handling for unsynced wallets (lines 117-123) is non-obvious and could benefit from documentation explaining why mined outputs are treated as confirmed when the wallet is not synced.
Add a doc comment explaining the rationale:
/// Get the PayRef status based on confirmation requirements +/// +/// For unsynced wallets (where current_tip_height is 0 or less than mined_height), +/// all mined outputs are treated as having sufficient confirmations. This ensures +/// that PayRefs are available immediately after wallet recovery or when the wallet +/// is temporarily out of sync, improving user experience. pub fn get_payment_reference_status(&self, current_tip_height: u64, required_confirmations: u64) -> PayRefStatus {applications/minotari_app_grpc/proto/base_node.proto (1)
569-574: Add input validation for payment reference format.The
payment_reference_hexfield expects a 64-character hex string (32 bytes). Consider adding a comment to document this requirement and ensure server-side validation.message SearchPaymentReferencesRequest { - // Payment reference as hex string (64 characters) + // Payment reference as hex string (exactly 64 characters representing 32 bytes) repeated string payment_reference_hex = 1; // Optional: include spent outputs in results bool include_spent = 2; }base_layer/wallet/src/output_manager_service/handle.rs (2)
296-301: Improve hex formatting consistency.The
FindPaymentByReferenceformatting creates a concatenated hex string without separators. Consider using the existingto_hex()method for consistency with other parts of the codebase.-FindPaymentByReference(payref) => write!(f, "FindPaymentByReference({})", payref.iter().map(|b| format!("{:02x}", b)).collect::<String>()), +FindPaymentByReference(payref) => write!(f, "FindPaymentByReference({})", payref.to_hex()),
1055-1070: Consider using consistent logging approach.The method uses
log::debug!directly instead of thedebug!macro used elsewhere in the codebase. Also, the "payref_debug:" prefix in log messages appears to be temporary debugging code.-pub async fn get_all_payment_references( - &mut self, -) -> Result<Vec<crate::output_manager_service::payment_reference::PaymentRecord>, OutputManagerError> { - log::debug!(target: "wallet::output_manager_service::handle", "payref_debug: get_all_payment_references() called via handle"); - match self - .handle - .call(OutputManagerRequest::GetAllPaymentReferences) - .await?? - { - OutputManagerResponse::PaymentReferences(references) => { - log::debug!(target: "wallet::output_manager_service::handle", "payref_debug: get_all_payment_references() returning {} references via handle", references.len()); - Ok(references) - }, - _ => Err(OutputManagerError::UnexpectedApiResponse), - } -} +pub async fn get_all_payment_references( + &mut self, +) -> Result<Vec<crate::output_manager_service::payment_reference::PaymentRecord>, OutputManagerError> { + match self + .handle + .call(OutputManagerRequest::GetAllPaymentReferences) + .await?? + { + OutputManagerResponse::PaymentReferences(references) => Ok(references), + _ => Err(OutputManagerError::UnexpectedApiResponse), + } +}base_layer/wallet/src/output_manager_service/payment_reference.rs (6)
75-77: Consider removing redundant PayRef storage in Available variant.The
Availablevariant stores the PayRef as[u8; 32], but this seems redundant since the PayRef would likely be known from the context where this status is used. Consider simplifying to justAvailable(u64)with only the confirmation count.pub enum PayRefStatus { /// PayRef is available with the specified number of confirmations - Available([u8; 32], u64), + Available(u64),
196-211: Extract duplicate payref_hex() methods into a trait.Both
PaymentDetailsandPaymentRecordimplement identicalpayref_hex()methods. This violates DRY principles and could lead to maintenance issues.Add a trait before the impl blocks:
/// Trait for types that contain a payment reference pub trait HasPaymentReference { fn payment_reference(&self) -> &[u8; 32]; /// Format the Payment Reference as a hex string fn payref_hex(&self) -> String { self.payment_reference().to_hex() } } impl HasPaymentReference for PaymentDetails { fn payment_reference(&self) -> &[u8; 32] { &self.payment_reference } } impl HasPaymentReference for PaymentRecord { fn payment_reference(&self) -> &[u8; 32] { &self.payment_reference } }Then remove the duplicate
payref_hex()implementations from lines 196-199 and 208-211.
226-228: Handle edge case when total length equals prefix + suffix.When
hex.len() == prefix + suffix, the current logic will display all characters without the "..." separator, which might be confusing to users expecting a shortened format.- if hex.len() >= (prefix + suffix) { + if hex.len() > (prefix + suffix + 3) { // +3 for "..." format!("{}...{}", &hex[0..prefix], &hex[hex.len()-suffix..]) } else { hex }
226-227: Add overflow protection for u8 to usize conversion.Converting
prefix_charsandsuffix_charsfromu8tousizecould theoretically cause issues on platforms whereusizeis smaller thanu8max value, though this is unlikely in practice.PayRefDisplayFormat::Custom { prefix_chars, suffix_chars } => { - let prefix = *prefix_chars as usize; - let suffix = *suffix_chars as usize; + let prefix = (*prefix_chars).min(32) as usize; + let suffix = (*suffix_chars).min(32) as usize;
244-252: Optimize parse_payref_hex to avoid intermediate allocation.The current implementation creates an intermediate
Vec<u8>before copying to the array. This allocation can be avoided.pub fn parse_payref_hex(hex_str: &str) -> Result<[u8; 32], String> { if hex_str.len() != 64 { return Err("Payment Reference must be exactly 64 hexadecimal characters".to_string()); } - let bytes = Vec::<u8>::from_hex(hex_str) - .map_err(|_| "Payment Reference must contain only valid hexadecimal characters".to_string())?; - - if bytes.len() != 32 { - return Err("Payment Reference must decode to exactly 32 bytes".to_string()); - } - let mut payref = [0u8; 32]; - payref.copy_from_slice(&bytes); + hex::decode_to_slice(hex_str, &mut payref) + .map_err(|_| "Payment Reference must contain only valid hexadecimal characters".to_string())?; Ok(payref) }
257-259: Consider reusing parse_payref_hex for validation.The
is_valid_payref_formatfunction duplicates validation logic. Consider reusingparse_payref_hexto ensure consistency./// Validate Payment Reference format pub fn is_valid_payref_format(hex_str: &str) -> bool { - hex_str.len() == 64 && hex_str.chars().all(|c| c.is_ascii_hexdigit()) + parse_payref_hex(hex_str).is_ok() }base_layer/wallet/src/output_manager_service/service.rs (1)
3714-3721: Complete the configuration storage implementation.This method returns a hardcoded default configuration. The TODO comment indicates this needs proper database storage.
Would you like me to generate the database schema and implementation for storing PayRef configuration?
applications/minotari_app_grpc/proto/wallet.proto (2)
1053-1094: Suggest unifying payment_reference type between request and response
TheGetPaymentByReferenceRPC accepts a 32-bytebytesfield, while the returnedPaymentDetails.payment_referenceis defined as astring(hex). Consider standardizing on one representation—either keep raw bytes throughout (with hex formatting in docs) or use hex-encoded strings for both—to avoid client confusion.
1653-1672: Suggest clarifying hex encoding in PaymentDetails.payment_reference
The comment forPaymentDetails.payment_referenceshould explicitly state that the string is the hex-encoded form of the 32-byte PayRef. This will guide client code in parsing and formatting the field correctly.base_layer/wallet_ffi/wallet.h (1)
250-293: Consider enhancing struct documentation to match file standards.While the struct definitions are technically correct, the documentation could be improved to match the detailed style used elsewhere in the file. Consider adding:
- Detailed description of each struct's purpose
- Documentation for each field explaining its meaning and constraints
- Information about the 32-byte payment reference format
Example enhancement for
TariPaymentDetails:/** * Payment Details FFI Type * * Represents detailed information about a payment transaction including its reference, * amount, and confirmation status. * * Fields: * - payment_reference: 32-byte unique payment reference identifier * - commitment: Hex string representation of the transaction commitment * - amount: Transaction amount in MicroMinotari * - block_height: The height at which this payment was mined * - confirmations: Number of blockchain confirmations * - mined_timestamp: Unix timestamp when the payment was mined */ struct TariPaymentDetails {base_layer/wallet_ffi/src/lib.rs (4)
10815-10859: Consider making Custom format parameters configurable.The Custom display format is hardcoded to 8 prefix and suffix chars. Consider extending the FFI struct to allow configurable prefix/suffix lengths.
10912-10941: Consider using the Hex trait for cleaner conversion.The manual hex conversion works but could be simplified.
- let hex_string = payref_slice.iter() - .map(|b| format!("{:02x}", b)) - .collect::<String>(); + let hex_string = payref_slice.to_hex();
11161-11211: Consolidate duplicate formatting cases.Format types 1 and 2 have identical implementations. Also consider using the Hex trait.
- let hex_string = payref_slice.iter() - .map(|b| format!("{:02x}", b)) - .collect::<String>(); + let hex_string = payref_slice.to_hex(); // Format according to type let formatted = match format_type { 0 => hex_string, // Full - 1 => { // Shortened (8...8) - if hex_string.len() >= 16 { - format!("{}...{}", &hex_string[0..8], &hex_string[hex_string.len()-8..]) - } else { - hex_string - } - }, - 2 => { // Custom (8...8) + 1 | 2 => { // Shortened or Custom (both use 8...8) if hex_string.len() >= 16 { format!("{}...{}", &hex_string[0..8], &hex_string[hex_string.len()-8..]) } else { hex_string } }, _ => hex_string, // Default to full };
11246-11303: Consider using a JSON library for safer construction.Manual JSON construction is error-prone and could lead to invalid JSON if values contain special characters.
Consider using
serde_jsonfor safer JSON construction:use serde_json::json; let verification_result = json!({ "status": status, "amount": payment_details.amount.as_u64(), "block_height": payment_details.block_height, "confirmations": payment_details.confirmations, "sufficient_confirmations": sufficient }).to_string();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
applications/minotari_app_grpc/Cargo.toml(1 hunks)applications/minotari_app_grpc/proto/base_node.proto(2 hunks)applications/minotari_app_grpc/proto/transaction.proto(1 hunks)applications/minotari_app_grpc/proto/wallet.proto(3 hunks)applications/minotari_app_grpc/src/conversions/transaction_output.rs(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/cli.rs(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(13 hunks)applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(3 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(3 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(1 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(3 hunks)base_layer/core/src/base_node/comms_interface/comms_request.rs(2 hunks)base_layer/core/src/base_node/comms_interface/comms_response.rs(3 hunks)base_layer/core/src/base_node/comms_interface/inbound_handlers.rs(1 hunks)base_layer/core/src/base_node/comms_interface/local_interface.rs(2 hunks)base_layer/core/src/chain_storage/async_db.rs(1 hunks)base_layer/core/src/chain_storage/blockchain_backend.rs(1 hunks)base_layer/core/src/chain_storage/blockchain_database.rs(1 hunks)base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(11 hunks)base_layer/core/src/test_helpers/blockchain.rs(1 hunks)base_layer/wallet/Cargo.toml(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(4 hunks)base_layer/wallet/src/output_manager_service/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/payment_reference.rs(1 hunks)base_layer/wallet/src/output_manager_service/service.rs(2 hunks)base_layer/wallet/src/output_manager_service/storage/models.rs(3 hunks)base_layer/wallet/src/output_manager_service/storage/output_status.rs(1 hunks)base_layer/wallet_ffi/src/lib.rs(6 hunks)base_layer/wallet_ffi/wallet.h(3 hunks)hashing/src/domains.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
base_layer/core/src/chain_storage/blockchain_backend.rs (4)
base_layer/core/src/base_node/comms_interface/local_interface.rs (2)
fetch_output_by_payref(361-373)check_output_spent_status(376-388)base_layer/core/src/test_helpers/blockchain.rs (2)
fetch_output_by_payref(317-319)check_output_spent_status(321-323)base_layer/core/src/chain_storage/blockchain_database.rs (2)
fetch_output_by_payref(452-455)check_output_spent_status(458-461)base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (4)
fetch_output_by_payref(1906-1909)fetch_output_by_payref(2339-2342)check_output_spent_status(1912-1915)check_output_spent_status(2344-2346)
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)
base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
output_hash(434-462)
base_layer/core/src/base_node/comms_interface/local_interface.rs (5)
base_layer/core/src/chain_storage/blockchain_backend.rs (2)
fetch_output_by_payref(114-114)check_output_spent_status(117-117)base_layer/core/src/test_helpers/blockchain.rs (2)
fetch_output_by_payref(317-319)check_output_spent_status(321-323)base_layer/core/src/chain_storage/blockchain_database.rs (2)
fetch_output_by_payref(452-455)check_output_spent_status(458-461)base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (4)
fetch_output_by_payref(1906-1909)fetch_output_by_payref(2339-2342)check_output_spent_status(1912-1915)check_output_spent_status(2344-2346)base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)
output_hash(434-462)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (6)
base_layer/wallet/src/output_manager_service/handle.rs (2)
new(412-420)get_all_payment_references(1055-1070)base_layer/wallet/src/output_manager_service/service.rs (3)
new(144-201)start(203-253)get_all_payment_references(3610-3712)base_layer/common_types/src/tari_address/mod.rs (3)
to_hex(394-397)default(88-90)default(429-431)base_layer/wallet/src/output_manager_service/storage/models.rs (2)
from(200-202)from(246-251)base_layer/wallet/src/output_manager_service/payment_reference.rs (1)
default(51-59)base_layer/wallet/src/wallet.rs (1)
start(178-388)
base_layer/wallet/src/output_manager_service/handle.rs (1)
base_layer/wallet/src/output_manager_service/service.rs (6)
iter(3837-3839)find_payment_by_reference(3548-3573)get_available_payment_references(3576-3606)get_all_payment_references(3610-3712)get_payment_reference_config(3715-3721)set_payment_reference_config(3724-3731)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
🔇 Additional comments (69)
base_layer/wallet/src/output_manager_service/mod.rs (1)
26-26: LGTM! Payment reference module correctly exposed.The module declaration follows the established pattern and is appropriately positioned among other public modules in the output manager service.
base_layer/wallet/Cargo.toml (1)
23-23: LGTM! Dependency addition is correctly formatted.The
tari_hashingworkspace dependency is properly added and necessary for the PayRef functionality that relies on domain-separated hashing.applications/minotari_console_wallet/src/wallet_modes.rs (1)
649-652: LGTM! Test coverage properly maintained for new CLI commands.The exhaustive match in the test correctly includes all four new PayRef-related CLI command variants, ensuring the test will continue to compile and pass as new commands are added.
applications/minotari_app_grpc/src/conversions/transaction_output.rs (1)
114-116: LGTM! Appropriate placeholder implementation with clear documentation.The empty payment reference with explanatory comment is a reasonable approach since the payment reference depends on the block hash, which isn't available at transaction creation time. The comment clearly explains the deferred population strategy.
hashing/src/domains.rs (1)
40-45: LGTM! Well-structured hash domain addition.The new
PaymentReferenceHashDomainfollows the established pattern perfectly with an appropriate domain string and version number. This will provide domain-separated hashing for PayRef generation.applications/minotari_app_grpc/Cargo.toml (1)
13-13: Necessary feature enablement for PayRef gRPC functionality.Adding the
"base_node"feature totari_coreis required to support the new payment reference-related RPC methods in the BaseNode gRPC services.applications/minotari_app_grpc/proto/transaction.proto (1)
118-121: Well-documented protobuf field addition.The new
payment_referencefield is properly documented and uses an appropriate field number and type. The addition is backward compatible and clearly explains the Blake2b hash computation.base_layer/wallet/src/output_manager_service/storage/output_status.rs (2)
24-24: Clean serde import addition.
30-30: Appropriate serialization traits for API support.Adding
SerializeandDeserializederives enablesOutputStatusto be included in serialized API responses for the new PayRef functionality. This is a clean, non-breaking change.base_layer/core/src/base_node/comms_interface/comms_response.rs (3)
35-35: Necessary imports for new response types.
64-65: Well-structured response variants addition.The new
OutputMinedInfoandInputMinedInforesponse variants follow the established pattern with appropriate Option wrappers for nullable responses.
103-104: Consistent Display implementation.The Display trait implementations for the new response variants follow the same concise format as existing variants.
applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (2)
335-381: LGTM! Well-implemented PayRef UI integration.The addition of the PayRef field to the transaction details view is correctly implemented:
- Constraint arrays properly expanded from 13 to 14 elements
- PayRef field consistently positioned at index 0 in both label and content layouts
- All existing fields correctly shifted down by one position
- Proper styling with magenta color matching existing field labels
520-526: Good error handling for payment reference display.The payment reference content handling properly provides a fallback to "N/A" when the payment reference is not available, maintaining consistent UI behavior.
base_layer/core/src/base_node/comms_interface/comms_request.rs (2)
86-87: Well-designed request variants for PayRef functionality.The new request variants are appropriately typed:
FetchOutputByPayRef([u8; 32])uses a fixed 32-byte array for payment referencesCheckOutputSpentStatus(HashOutput)uses the standard HashOutput type for output identificationThese align well with the PayRef system design and existing communication patterns.
142-147: Proper display formatting for debugging and logging.The Display implementation correctly formats the new request variants as hex strings, providing readable output for debugging and logging purposes. The use of
to_hex()is consistent with other similar formatting in this file.base_layer/core/src/chain_storage/async_db.rs (1)
157-159: Correctly implemented async wrappers for PayRef functionality.The new async methods properly use the
make_async_fn!macro pattern that is consistently used throughout this file. The method signatures correctly match the underlying synchronous database methods and include appropriate trace logging names.base_layer/core/src/chain_storage/blockchain_backend.rs (1)
112-117: Well-designed trait methods for PayRef backend support.The new trait methods are properly designed with:
- Appropriate signatures:
fetch_output_by_payreftakes a reference to avoid unnecessary copying of the 32-byte array, whilecheck_output_spent_statustakesHashOutputby value (consistent with other methods)- Consistent error handling: Both methods return
Result<Option<T>, ChainStorageError>following the established pattern- Clear documentation: Comments clearly describe the purpose and return behavior of each method
- Logical return types:
OutputMinedInfofor PayRef lookups andInputMinedInfofor spent status checksThese additions integrate seamlessly with the existing backend trait design.
base_layer/core/src/test_helpers/blockchain.rs (2)
317-319: LGTM! Clean delegation pattern for PayRef functionality.The
fetch_output_by_payrefmethod correctly follows the established delegation pattern in this test helper, allowing PayRef-based output queries to be tested through theTempDatabasewrapper.
321-323: LGTM! Consistent implementation for output spent status checking.The
check_output_spent_statusmethod properly delegates to the underlying LMDB database and maintains consistency with the existing method patterns in the test helper.base_layer/core/src/chain_storage/blockchain_database.rs (2)
451-455: LGTM! Payment reference lookup method implemented correctly.The
fetch_output_by_payrefmethod follows the established pattern of acquiring a read lock and delegating to the backend implementation. The method signature and documentation are appropriate for querying outputs by payment reference.
457-461: LGTM! Output spent status check method implemented correctly.The
check_output_spent_statusmethod properly follows the established database access pattern. The method provides a clean API for checking whether an output has been spent, which is essential for the PayRef functionality.base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)
457-464: LGTM! Clean implementation following established patterns.The new PayRef request handlers are well-implemented and consistent with existing code patterns in this file. Both handlers properly:
- Delegate to the blockchain database layer
- Use appropriate async error propagation with the
?operator- Return results wrapped in the correct response types
base_layer/core/src/base_node/comms_interface/local_interface.rs (3)
41-41: Good addition of required imports.The import statement correctly adds the new types needed for PayRef functionality.
360-373: Well-implemented PayRef lookup method.The
fetch_output_by_payrefmethod follows the established pattern for this interface:
- Clear documentation explaining the purpose
- Correct parameter handling (dereferencing the payref reference)
- Proper async error handling and response matching
- Consistent error handling with
UnexpectedApiResponsefor type mismatches
375-388: Clean implementation of output spent status check.The
check_output_spent_statusmethod is well-implemented:
- Clear method documentation
- Appropriate parameter types (
HashOutputpassed directly)- Consistent error handling and response pattern matching
- Follows the same structure as other interface methods
applications/minotari_console_wallet/src/automation/commands.rs (3)
2793-2826: Excellent implementation with good practices.The ListPayRefs command demonstrates several good practices:
- Comprehensive debug logging for troubleshooting
- Optional limit functionality with sensible default
- User-friendly output format with clear formatting
- Proper handling of empty results
- Consistent async/await usage
2827-2845: Simple and effective configuration display command.The PayRefConfig command provides a clear way to view payment reference configuration settings. The implementation correctly handles optional parameters and importantly notes that changes are not persisted, which sets proper user expectations.
2766-2792:Details
✅ Verification successful
Verify import statement and approve implementation.
The FindPayRef command implementation looks correct with proper error handling and comprehensive output. However, the import statement should be verified.
Please verify that the import path is correct:
🏁 Script executed:
#!/bin/bash # Description: Verify that the parse_payref_hex function exists in the specified module # Search for the parse_payref_hex function definition ast-grep --pattern 'fn parse_payref_hex($$$) { $$$ }' # Also search for any payment_reference module exports rg -A 5 "pub.*parse_payref_hex"Length of output: 736
Import verification successful – implementation approved
Confirmed that
parse_payref_hexis publicly defined inminotari_wallet::output_manager_service::payment_reference. No further changes needed.applications/minotari_console_wallet/src/ui/state/app_state.rs (4)
780-781: LGTM! Well-placed integration of PayRef calculation.The placement of the PayRef calculation call is optimal - it executes after transactions are loaded and sorted but before marking the state as updated, ensuring payment references are populated when the UI refreshes.
1250-1250: LGTM! Appropriate field addition for PayRef support.The
payment_reference_hexfield is correctly typed asOption<String>and follows the existing naming conventions in the struct.
1311-1311: LGTM! Correct field initialization.Initializing
payment_reference_hextoNoneis appropriate since the field will be populated during transaction loading by thecalculate_payment_references_for_transactionsmethod.
787-837:Details
✅ Verification successful
Verify performance impact and method dependency.
The PayRef calculation implementation looks solid with good error handling and extensive debug logging. However, there are a few considerations:
- Performance: The method processes all unspent and spent outputs, which could be expensive for wallets with many outputs.
- Method dependency: The code calls
output.generate_payment_reference()but this method's implementation isn't visible in the provided context.Let me verify the
generate_payment_reference()method exists and assess the performance characteristics:
🏁 Script executed:
#!/bin/bash # Description: Verify generate_payment_reference method exists and check for performance considerations # Search for generate_payment_reference method implementation echo "=== Searching for generate_payment_reference method ===" ast-grep --pattern 'fn generate_payment_reference($_) { $$$ }' echo "=== Searching for generate_payment_reference method calls ===" rg -A 3 "generate_payment_reference" echo "=== Checking output manager service get methods for potential performance impact ===" rg -A 5 "get_unspent_outputs\|get_spent_outputs"Length of output: 5897
Performance and Dependency Verification Completed
- The
generate_payment_reference()method is defined in
base_layer/wallet/src/output_manager_service/storage/models.rs
and returns anOption<[u8; 32]>by hashing the block hash (and commitment) with a Blake2b-based, domain-separated hasher.- Both
get_unspent_outputs()andget_spent_outputs()pull all outputs from theOutputManagerService. Iterating over them and hashing each entry is O(n) in the number of outputs.- For most wallets (hundreds of outputs), this will incur only a modest CPU cost, but if you expect very large output sets you may:
- Cache computed payment references, or
- Page or debounce UI updates to avoid blocking the UI thread.
No further changes are required here.
applications/minotari_console_wallet/src/cli.rs (2)
177-180: LGTM! Well-designed CLI command additions.The four new PayRef commands follow the existing naming conventions and provide comprehensive coverage for payment reference operations:
ShowPayRef: Display details for a specific transactionFindPayRef: Look up transaction by payment referenceListPayRefs: List payment references with filteringPayRefConfig: Configure PayRef behavior
480-512: LGTM! Comprehensive and well-structured argument definitions.The argument structures are well-designed with appropriate field types and helpful comments:
ShowPayRefArgs: Simple and focused with just transaction_idFindPayRefArgs: Clean interface for PayRef lookupListPayRefsArgs: Good flexibility with optional limit, status filtering, and privacy controlsPayRefConfigArgs: Comprehensive configuration options with all optional fieldsThe use of
Option<T>for configuration fields allows partial updates, which is a good UX pattern.applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
3002-3003:⚠️ Potential issueFix compilation error:
input.output_hash()method does not exist.The
inputfield frominput_infodoesn't have anoutput_hash()method. You should use the spending transaction hash or the transaction kernel hash instead.Apply this diff to fix the issue:
-let (is_spent, spent_height, spent_block_hash) = match node_service.check_output_spent_status(output_hash).await { - Ok(Some(input_info)) => (true, input_info.spent_height, input_info.header_hash.to_vec()), +let (is_spent, spent_height, spent_block_hash) = match node_service.check_output_spent_status(output_hash).await { + Ok(Some(input_info)) => (true, input_info.spent_height, input_info.header_hash.to_vec()),Likely an incorrect or invalid review comment.
base_layer/wallet/src/output_manager_service/storage/models.rs (3)
97-110: LGTM! Well-implemented PayRef generation.The method correctly uses domain-separated hashing and only generates payment references for mined outputs. The hash construction (block_hash || commitment) ensures uniqueness per output per block.
146-153: LGTM! Simple and correct PayRef matching.The method correctly handles the case where no PayRef can be generated and performs a simple equality check.
155-172: LGTM! Comprehensive payment details retrieval.The method correctly returns payment details only for confirmed PayRefs and includes all relevant information. Good use of the actual output value for the amount field.
applications/minotari_app_grpc/proto/base_node.proto (2)
103-106: LGTM! Well-designed PayRef RPC methods.The new RPC methods provide appropriate interfaces for payment reference lookups:
SearchPaymentReferencesuses server-streaming for efficient batch lookupsGetPublicPaymentInfoprovides a privacy-preserving single lookup suitable for block explorers
595-596: Good use of optional fields for confidential transactions.The
optional uint64 revealed_amountfields properly handle cases where transaction amounts may be hidden for confidentiality.Also applies to: 623-624
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)
645-654: Well-implemented payment reference generation.The PayRef generation correctly uses Blake2b with domain separation, following cryptographic best practices. The deterministic generation from block hash and commitment ensures consistency.
611-619: Correct PayRef index insertion.The PayRef is properly generated and indexed when outputs are inserted, maintaining the index integrity.
783-796: Proper PayRef cleanup on output spending.The implementation correctly removes PayRef entries when outputs are spent. The use of
.ok()to ignore errors is appropriate since the PayRef might not exist for older outputs.base_layer/wallet/src/output_manager_service/handle.rs (2)
160-167: LGTM! Well-structured enum variants for PayRef operations.The new request variants follow the existing pattern and use appropriate types. The fixed-size array
[u8; 32]for payment references ensures type safety.
356-360: LGTM! Response variants align with request types.The response variants properly match the corresponding request types with appropriate data structures.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
1362-1363: LGTM! Correctly uses the helper method.This implementation properly uses the
calculate_payment_references_for_transactionhelper method, avoiding code duplication.base_layer/wallet/src/output_manager_service/service.rs (1)
542-567: LGTM! Request handlers correctly integrated.The new PayRef request handlers follow the established pattern and correctly map requests to their corresponding methods and responses.
applications/minotari_app_grpc/proto/wallet.proto (9)
1354-1356: Verify addition of payment_reference field in TransactionInfo
The new fieldstring payment_reference = 17;must be reflected in all generated stubs, server handlers, and documentation. Please confirm there’s no tag collision with future fields and that default behavior remains backward-compatible.
1605-1608: Approve GetPaymentByReferenceRequest definition
GetPaymentByReferenceRequestcorrectly specifies a required 32-bytebytes payment_referencewith clear documentation.
1611-1614: Approve GetPaymentByReferenceResponse definition
GetPaymentByReferenceResponsecleanly wraps aPaymentDetailsmessage, aligning with other RPC patterns.
1617-1624: Approve GetUnspentPaymentReferencesRequest definition
Pagination fields (offset,limit) and optionalPaymentDirectionfilter follow established conventions.
1627-1632: Approve GetUnspentPaymentReferencesResponse definition
The response message includes a repeated list ofPaymentDetailsand atotal_count, which is ideal for paginated clients.
1635-1642: Approve GetAllPaymentReferencesRequest definition
This request mirrors the unspent variant and correctly supports direction filtering and pagination.
1645-1650: Approve GetAllPaymentReferencesResponse definition
Returning bothpayment_referencesandtotal_countis consistent with other list RPCs.
1675-1682: Approve PaymentDirection enum
The newly introducedPaymentDirectionenum accurately captures inbound vs. outbound filtering for PayRefs.
1685-1694: Approve PayRefStatus enum
ThePayRefStatusvalues (UNKNOWN,AVAILABLE,PENDING,NOT_MINED) comprehensively represent the possible states of a payment reference.base_layer/wallet_ffi/wallet.h (1)
121-122: Forward declaration looks good.The forward declaration follows the established pattern in the file.
base_layer/wallet_ffi/src/lib.rs (11)
85-91: LGTM!The imports for PayRef functionality are appropriate and necessary.
235-278: Well-structured FFI type definitions.The structs are properly designed for C compatibility with appropriate use of
#[repr(C)]and fixed-size arrays. TheCloneimplementation onTariPaymentRecordis correctly added for theget_atfunction.Note: The
commitmentfield inTariPaymentDetailswill require careful memory management in the destroy function.
6090-6096: Good refactoring for clarity.Using an intermediate variable improves readability.
10637-10688: Excellent implementation ofwallet_find_payment_by_reference.Proper null checks, error handling, and memory management for the commitment string.
10690-10774: Well-implemented payment reference listing functions.Both functions follow consistent patterns with proper error handling and type conversions. The PaymentDirection enum mapping correctly handles all cases.
10861-10910: Correct memory allocation pattern for FFI.Properly uses
libc::mallocfor C-compatible memory allocation with appropriate size validation.
10943-10977: Proper cleanup functions with correct memory management.The destroy functions correctly handle memory cleanup, especially
payment_details_destroywhich properly frees the nested commitment string.
10979-11030: Well-implemented array access pattern for FFI.Proper bounds checking and the use of clone() is appropriate for the get_at pattern.
11032-11038: Correct memory deallocation pairing with malloc.Properly uses
libc::freeto match thelibc::mallocallocation.
11086-11159: Good implementation with defensive error handling.The fallback to default config when fetching fails is a good practice.
11213-11244: Correct validation implementation.Properly validates hex format and length requirements.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
11041-11085:⚠️ Potential issueCritical: Insecure payment reference generation.
This function uses a simple XOR operation instead of proper Blake2b hashing as indicated in the comment. This is a security vulnerability if used in production.
🧹 Nitpick comments (13)
applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (3)
339-340: Consider using a constant for the number of detail fields.The hardcoded array size of 14 could lead to maintenance issues if fields are added or removed. Consider defining a constant or calculating the size dynamically.
- let constraints = [Constraint::Length(1); 14]; + const DETAIL_FIELDS_COUNT: usize = 14; + let constraints = [Constraint::Length(1); DETAIL_FIELDS_COUNT];Also applies to: 390-391
524-541: Good implementation of PayRef display with status-based coloring.The logic handles all combinations of PayRef hex and status properly. Consider defining constants or using pattern matching on enum variants instead of string prefix matching for better maintainability.
574-597: Well-implemented PayRef search with user-friendly features.The case-insensitive search with space removal is a good UX choice. The error message could be more specific about only searching completed transactions.
- self.error_message = Some(format!( - "No transaction found with PayRef containing '{}'\nPress Enter to continue.", - search_term - )); + self.error_message = Some(format!( + "No completed transaction found with PayRef containing '{}'\nPress Enter to continue.", + search_term + ));docs/src/payref_userguide.md (1)
17-19: Add language specifiers to fenced code blocks.The markdown linter correctly identifies that these code blocks should have language specifiers for better syntax highlighting.
For line 17:
-``` +```text PayRef = Blake2b_256(block_hash || commitment)For line 220:
-``` +```text Input: block_hash (32 bytes), commitment (32 bytes) Output: payment_reference (32 bytes) 1. Initialize Blake2b hasher with domain separation 2. hasher.update(block_hash) 3. hasher.update(commitment) 4. payment_reference = hasher.finalize()Also applies to: 220-228
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
17-17: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
base_layer/common_types/src/payment_reference.rs (1)
204-225: Well-designed formatting with multiple display options.The implementation handles different use cases well. Consider adding validation to ensure prefix + suffix doesn't exceed the hex string length in Custom format.
PayRefDisplayFormat::Custom { prefix_chars, suffix_chars } => { let prefix = *prefix_chars as usize; let suffix = *suffix_chars as usize; - if hex.len() >= (prefix + suffix) { + if hex.len() >= (prefix + suffix) && prefix + suffix < hex.len() { format!("{}...{}", &hex[0..prefix], &hex[hex.len()-suffix..]) } else { hex } }docs/src/09_adding_tari_to_your_exchange.md (6)
474-476: Specify fenced code block language.The Blake2b_256 formula block is missing a language identifier, which helps with syntax highlighting and linting (e.g., use
text orrust).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
474-474: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
479-482: Unify unordered list style.These list items use dashes (
-); to match the rest of the document and avoid lint errors, switch to asterisk-based bullets (*).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
479-479: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
480-480: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
481-481: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
482-482: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
488-488: Add comma before ‘but’.In the sentence "When a customer claims they sent a deposit but you don't see it in your systems:", add a comma before "but" for correct compound sentence punctuation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~488-~488: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...en a customer claims they sent a deposit but you don't see it in your systems: **Tr...(COMMA_COMPOUND_SENTENCE)
884-892: Consistent task list syntax.The integration checklist uses dashes for task items (
- [ ]). To satisfy markdownlint and maintain consistency, consider using asterisks (* [ ]) instead.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
885-885: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
886-886: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
887-887: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
888-888: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
889-889: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
890-890: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
891-891: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
892-892: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
899-912: Label code fence language for email template.The PayRef Request Template code block is not annotated with a language. Adding a language (e.g., ```text) will improve readability and satisfy lint rules.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
899-899: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
925-947: Label code fence language for email template.The PayRef Confirmed Template code block is also missing a language specifier. Please annotate it (e.g., ```text) for clarity and lint compliance.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
926-926: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
base_layer/wallet_ffi/src/lib.rs (2)
10929-10933: Consider using existing hex utilities.The manual hex formatting could be replaced with existing utilities for consistency:
- let hex_string = payref_slice.iter() - .map(|b| format!("{:02x}", b)) - .collect::<String>(); + let hex_string = payref_slice.to_hex();
11180-11183: Consider using existing hex utilities for consistency.Similar to the earlier comment, the manual hex formatting could use existing utilities:
- let hex_string = payref_slice.iter() - .map(|b| format!("{:02x}", b)) - .collect::<String>(); + let hex_string = payref_slice.to_hex();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(8 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(3 hunks)base_layer/common_types/Cargo.toml(1 hunks)base_layer/common_types/src/lib.rs(1 hunks)base_layer/common_types/src/payment_reference.rs(1 hunks)base_layer/core/src/chain_storage/async_db.rs(2 hunks)base_layer/core/src/chain_storage/blockchain_database.rs(1 hunks)base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(12 hunks)base_layer/wallet/src/output_manager_service/storage/models.rs(3 hunks)base_layer/wallet/tests/payment_reference_integration_tests.rs(1 hunks)base_layer/wallet_ffi/src/lib.rs(6 hunks)docs/src/09_adding_tari_to_your_exchange.md(1 hunks)docs/src/payref_userguide.md(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- base_layer/common_types/src/lib.rs
- base_layer/common_types/Cargo.toml
- base_layer/core/src/chain_storage/blockchain_database.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- base_layer/core/src/chain_storage/async_db.rs
- applications/minotari_console_wallet/src/ui/state/app_state.rs
- base_layer/wallet/src/output_manager_service/storage/models.rs
- base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/wallet_ffi/src/lib.rs (7)
base_layer/wallet_ffi/wallet.h (21)
wallet_find_payment_by_reference(4762-4764)wallet_get_available_payment_references(4769-4770)wallet_get_all_payment_references(4775-4776)wallet_get_payment_reference_config(4781-4782)wallet_set_payment_reference_config(4787-4789)parse_payment_reference_hex(4794-4794)payment_reference_to_hex(4799-4799)payment_details_destroy(4804-4804)string_destroy(367-367)payment_records_destroy(4809-4809)payref_config_destroy(4814-4814)payref_status_destroy(4819-4819)payment_records_get_length(4824-4824)payment_records_get_at(4829-4831)payment_record_destroy(4836-4836)payment_reference_destroy(4841-4841)wallet_generate_payment_reference_from_data(4847-4849)wallet_get_payment_reference_status(4854-4856)wallet_format_payment_reference(4861-4863)wallet_validate_payment_reference_format(4868-4868)wallet_verify_payment_reference(4873-4876)base_layer/wallet_ffi/src/error.rs (2)
from(73-113)from(120-355)applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (1)
new(46-58)base_layer/wallet/src/output_manager_service/service.rs (1)
new(144-201)base_layer/common_types/src/payment_reference.rs (2)
parse_payment_reference_hex(133-151)payment_reference_to_hex(192-194)integration_tests/src/ffi/ffi_import.rs (1)
string_destroy(83-83)integration_tests/src/ffi/wallet.rs (1)
drop(190-192)
🪛 LanguageTool
docs/src/09_adding_tari_to_your_exchange.md
[grammar] ~463-~463: Possible subject-verb agreement error.
Context: ... users claiming they sent payments that exchanges cannot verify. **Key Benefits for Exch...
(NNS_THAT_AGREEMENT)
[uncategorized] ~488-~488: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...en a customer claims they sent a deposit but you don't see it in your systems: **Tr...
(COMMA_COMPOUND_SENTENCE)
[duplication] ~499-~499: Possible typo: you repeated a word.
Context: ...upport: Enters PayRef into verification system 3. System: "Payment verified: 100 XTR received in...
(ENGLISH_WORD_REPEAT_RULE)
docs/src/payref_userguide.md
[uncategorized] ~33-~33: You might be missing the article “the” here.
Context: ... Select any confirmed transaction using Up/Down arrows 4. In the transaction detai...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~70-~70: ‘making a payment’ might be wordy. Consider a shorter alternative.
Context: ... ### For Users (Sending Payments) When making a payment to an exchange or business: 1. Send yo...
(EN_WORDINESS_PREMIUM_MAKING_A_PAYMENT)
[uncategorized] ~192-~192: You might be missing the article “the” here.
Context: ...ient confirmations have passed - Ensure wallet is synchronized with the blockchain ##...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~234-~234: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...s with blockchain reorganizations: - 1-4 confirmations: Status shows "Pending ...
(HYPHEN_TO_EN)
🪛 markdownlint-cli2 (0.17.2)
docs/src/09_adding_tari_to_your_exchange.md
474-474: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
479-479: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
480-480: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
481-481: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
482-482: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
885-885: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
886-886: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
887-887: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
888-888: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
889-889: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
890-890: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
891-891: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
892-892: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
899-899: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
926-926: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
docs/src/payref_userguide.md
17-17: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
220-220: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (22)
applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (5)
41-42: LGTM! Well-structured additions for PayRef search state.The new fields are properly defined and initialized, with clear naming that follows Rust conventions.
Also applies to: 55-56
636-637: Clear and consistent help text for PayRef search.The shortcut key and description follow the established UI pattern.
663-670: Well-designed search dialog with clear instructions.The dialog provides helpful guidance about partial match support and escape to cancel.
704-731: Robust input handling with appropriate validation.Good implementation restricting input to hexadecimal characters and spaces, with proper handling of control keys.
734-738: Clean implementation of search mode activation.docs/src/payref_userguide.md (2)
16-26: Excellent technical explanation of PayRef generation.The formula and properties are accurately described. The explanation clearly conveys the security and privacy benefits of the design.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
17-17: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
114-134: Helpful and accurate API integration examples.The gRPC examples are well-formatted and cover both wallet and base node verification scenarios.
base_layer/common_types/src/payment_reference.rs (4)
105-112: Secure and well-implemented PayRef generation function.The use of domain-separated hashing is a security best practice that prevents cross-protocol attacks. The implementation correctly concatenates inputs in a consistent order.
133-151: Robust hex parsing with comprehensive validation.Good error handling with descriptive messages. The length validation and hex decoding are properly implemented.
172-174: Efficient format validation function.Simple and correct implementation using appropriate character validation.
244-321: Comprehensive test coverage with good edge case handling.The tests thoroughly validate all functions including error cases and formatting options.
base_layer/wallet/tests/payment_reference_integration_tests.rs (4)
62-155: Excellent test coverage of the basic PayRef workflow.The test thoroughly validates generation, status checking, and verification with both sufficient and insufficient confirmations.
180-300: Comprehensive exchange verification workflow test.Excellent simulation of the real-world payment verification scenario between users and exchanges. The test properly validates both sufficient and insufficient confirmation scenarios.
332-403: Important test demonstrating PayRef stability requirements.This test effectively shows why confirmation requirements are critical for PayRef stability. The demonstration that PayRefs change during reorgs validates the design decision to require confirmations before showing PayRefs to users.
406-492: Well-designed performance test with reasonable bounds.The test validates that PayRef operations scale well with many outputs. The performance targets (5s generation, 100ms lookup) are appropriate for the use case.
base_layer/wallet_ffi/src/lib.rs (7)
85-91: LGTM!The added imports for PayRef-related types are appropriate and necessary for the FFI implementation.
236-278: Well-structured FFI type definitions!The PayRef FFI structs are properly defined with
#[repr(C)]and use appropriate C-compatible types. The design follows standard FFI patterns for representing complex data structures across the FFI boundary.
6094-6095: LGTM!Good use of the
to_hex()method for converting the cookie to hex format.
10641-10775: Well-implemented payment reference query functions!The
wallet_find_payment_by_reference,wallet_get_available_payment_references, andwallet_get_all_payment_referencesfunctions follow proper FFI patterns with:
- Comprehensive null pointer checks
- Proper error handling and error code setting
- Correct conversion between C arrays and Rust types
- Appropriate memory allocation for return values
10778-10860: Config management functions look good!The get/set PayRef configuration functions properly handle:
- Null pointer validation
- Enum conversions with sensible defaults
- Proper FFI struct conversion
11088-11160: Status checking implementation is solid!The
wallet_get_payment_reference_statusfunction properly:
- Queries the payment details
- Retrieves configuration for confirmation requirements
- Calculates status based on confirmations
- Handles all edge cases (not found, pending, available)
11248-11304: Excellent merchant verification function!The
wallet_verify_payment_referencefunction provides a comprehensive JSON response with all necessary verification details. The structured output format makes it easy for exchanges and merchants to integrate.
fluffypony
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
546-546:⚠️ Potential issueAdd the PayRef index database to all_dbs array.
The
all_dbs()method is missing the newpayref_to_output_indexdatabase. The array size should be 32, not 31, and the PayRef database should be included in the list.Apply this diff to fix the issue:
- fn all_dbs(&self) -> [(&'static str, &DatabaseRef); 31] { + fn all_dbs(&self) -> [(&'static str, &DatabaseRef); 32] { [ (LMDB_DB_METADATA, &self.metadata_db), (LMDB_DB_HEADERS, &self.headers_db), (LMDB_DB_HEADER_ACCUMULATED_DATA, &self.header_accumulated_data_db), (LMDB_DB_BLOCK_ACCUMULATED_DATA, &self.block_accumulated_data_db), (LMDB_DB_BLOCK_HASHES, &self.block_hashes_db), (LMDB_DB_UTXOS, &self.utxos_db), (LMDB_DB_INPUTS, &self.inputs_db), (LMDB_DB_TXOS_HASH_TO_INDEX, &self.txos_hash_to_index_db), (LMDB_DB_KERNELS, &self.kernels_db), (LMDB_DB_KERNEL_EXCESS_INDEX, &self.kernel_excess_index), (LMDB_DB_KERNEL_EXCESS_SIG_INDEX, &self.kernel_excess_sig_index), (LMDB_DB_KERNEL_MMR_SIZE_INDEX, &self.kernel_mmr_size_index), (LMDB_DB_UTXO_COMMITMENT_INDEX, &self.utxo_commitment_index), (LMDB_DB_CONTRACT_ID_INDEX, &self.contract_index), (LMDB_DB_UNIQUE_ID_INDEX, &self.unique_id_index), + (LMDB_DB_PAYREF_TO_OUTPUT_INDEX, &self.payref_to_output_index), ( LMDB_DB_DELETED_TXO_HASH_TO_HEADER_INDEX, &self.deleted_txo_hash_to_header_index, ),Also applies to: 561-585
♻️ Duplicate comments (2)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (2)
1901-1905:⚠️ Potential issueFix infinite recursion in check_output_spent_status.
The method is calling itself recursively, which will cause a stack overflow. It should be calling
fetch_input_in_txnto check if the output has been spent.Apply this diff to fix the infinite recursion:
fn check_output_spent_status(&self, output_hash: &HashOutput) -> Result<Option<InputMinedInfo>, ChainStorageError> { let txn = self.read_transaction()?; - self.check_output_spent_status(&output_hash) + self.fetch_input_in_txn(&txn, output_hash.as_slice()) }
2338-2340:⚠️ Potential issueFix recursive call in BlockchainBackend implementation.
This implementation has the same infinite recursion issue. It should directly call the implementation method.
Apply this diff to fix the issue:
fn check_output_spent_status(&self, output_hash: HashOutput) -> Result<Option<InputMinedInfo>, ChainStorageError> { - self.check_output_spent_status(&output_hash) + let txn = self.read_transaction()?; + self.fetch_input_in_txn(&txn, output_hash.as_slice()) }
🧹 Nitpick comments (2)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (2)
3087-3162: Well-implemented PayRef index migration.The migration correctly rebuilds the PayRef index for existing databases. The progress logging and transaction handling are appropriate.
Consider batching more operations per transaction to improve migration performance:
+ const BATCH_SIZE: usize = 100; + let mut batch_outputs = Vec::new(); + for height in 0..=chain_height { let read_txn = db.read_transaction()?; // Get block header to get block hash let header: Option<BlockHeader> = lmdb_get(&read_txn, &db.headers_db, &height)?; if let Some(header) = header { let block_hash = header.hash(); // Get all outputs for this block using the block hash let outputs: Vec<(Vec<u8>, TransactionOutputRowData)> = lmdb_fetch_matching_after( &read_txn, &db.utxos_db, block_hash.as_slice(), )?; drop(read_txn); if !outputs.is_empty() { - let write_txn = db.write_transaction()?; - - for (_, output_data) in outputs { - // Generate PayRef and add to index - let payref = LMDBDatabase::generate_payment_reference_for_output(&block_hash, &output_data.output.commitment); - let output_hash = &output_data.hash; - - // Insert into PayRef index - lmdb_replace( - &write_txn, - &db.payref_to_output_index, - &payref, - output_hash, - None, - )?; - - rebuild_count += 1; - } - - write_txn.commit()?; + for (_, output_data) in outputs { + batch_outputs.push((block_hash.clone(), output_data)); + + if batch_outputs.len() >= BATCH_SIZE { + let write_txn = db.write_transaction()?; + for (blk_hash, out_data) in batch_outputs.drain(..) { + let payref = LMDBDatabase::generate_payment_reference_for_output(&blk_hash, &out_data.output.commitment); + lmdb_replace( + &write_txn, + &db.payref_to_output_index, + &payref, + &out_data.hash, + None, + )?; + rebuild_count += 1; + } + write_txn.commit()?; + } + } } if height % 1000 == 0 { info!(target: LOG_TARGET, "PayRef rebuild progress: processed {} blocks, {} outputs", height, rebuild_count); } } } + + // Process remaining outputs + if !batch_outputs.is_empty() { + let write_txn = db.write_transaction()?; + for (blk_hash, out_data) in batch_outputs { + let payref = LMDBDatabase::generate_payment_reference_for_output(&blk_hash, &out_data.output.commitment); + lmdb_replace( + &write_txn, + &db.payref_to_output_index, + &payref, + &out_data.hash, + None, + )?; + rebuild_count += 1; + } + write_txn.commit()?; + }
1907-1910: Remove outdated TODO comment.This TODO comment is no longer relevant since the migration code (lines 3087-3162) already implements PayRef index rebuilding for databases created before PayRef support.
- // TODO: PayRef index rebuilding functionality - // This would allow rebuilding PayRef indexes for databases created before PayRef support - // Implementation would require proper integration with the blockchain backend interface -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(14 hunks)
🔇 Additional comments (3)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)
610-619: LGTM! PayRef generation and indexing on output insertion.The implementation correctly generates the PayRef using the block hash and output commitment, then indexes it for fast lookup. This follows the established pattern for other indexes in the codebase.
775-792: Good implementation of PayRef cleanup on spend.The code correctly removes PayRef index entries when outputs are spent, maintaining index consistency. Using
.ok()to ignore deletion errors is appropriate since PayRefs may not exist for outputs created before PayRef support was added.
1883-1897: Clean implementation of PayRef lookup.The function correctly implements PayRef to output lookup by first resolving the PayRef to an output hash, then reusing the existing
fetch_output_in_txnmethod. Good separation of concerns and code reuse.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
547-586: 🛠️ Refactor suggestionUpdate the all_dbs() method to include PayRef database.
The
all_dbs()method is missing the new PayRef database in its array, which could cause issues with database statistics and operations that iterate over all databases.Add the PayRef database to the array:
fn all_dbs(&self) -> [(&'static str, &DatabaseRef); 31] { [ (LMDB_DB_METADATA, &self.metadata_db), // ... other databases ... (LMDB_DB_UNIQUE_ID_INDEX, &self.unique_id_index), + (LMDB_DB_PAYREF_TO_OUTPUT_INDEX, &self.payref_to_output_index), ( LMDB_DB_DELETED_TXO_HASH_TO_HEADER_INDEX, &self.deleted_txo_hash_to_header_index, ), // ... rest of databases ... ] }You'll also need to update the array size from 31 to 32.
♻️ Duplicate comments (2)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
2334-2341: Verify the recursive call fix from past review.Based on past review comments, there was a recursive call issue in
check_output_spent_status. The current implementation appears to fix this by calling the internal method, but please confirm this is the intended fix.The method now correctly delegates to the internal implementation rather than recursively calling itself. This addresses the infinite recursion issue identified in previous reviews.
base_layer/wallet_ffi/src/lib.rs (1)
11041-11085:⚠️ Potential issueCritical: Insecure payment reference generation.
The function uses a simple XOR operation instead of proper Blake2b hashing as indicated in the comment. This is a security vulnerability if used in production.
The implementation should use the same Blake2b domain-separated hashing as the wallet service.
🧹 Nitpick comments (2)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (2)
776-794: Improve error handling for PayRef removal during input insertion.The PayRef removal logic is functionally correct but could benefit from better error handling. Currently, errors are silently ignored with
.ok(), which might mask legitimate database issues.Consider logging when PayRef removal fails:
// Remove PayRef index when output is spent // We need to find where this output was mined to calculate its PayRef if let Ok(Some(output_info)) = self.fetch_output_in_txn(txn, output_hash.as_slice()) { // Generate the PayRef using the same method as in generate_payment_reference let payref_bytes = Self::generate_payment_reference_for_output(&output_info.header_hash, &output_info.output.commitment); // Delete the PayRef index entry - lmdb_delete( + if let Err(e) = lmdb_delete( txn, &self.payref_to_output_index, &payref_bytes, "payref_to_output_index", - ).ok(); // Ignore errors in case the PayRef doesn't exist + ) { + // Only log if it's not a "key not found" error + if !matches!(e, ChainStorageError::ValueNotFound { .. }) { + warn!(target: LOG_TARGET, "Failed to remove PayRef for output {}: {}", output_hash.to_hex(), e); + } + } }
2959-2959: PayRef migration implementation is comprehensive but consider performance optimization.The migration correctly rebuilds the PayRef index for existing databases, but there are some performance considerations:
Strengths:
- Properly checks if migration is needed (empty index)
- Correctly iterates through all blocks and outputs
- Uses proper transaction handling
- Includes progress logging
Performance concerns:
- Creates a new write transaction for each block with outputs
- Could be memory-intensive for large databases
Consider batching the migration for better performance:
if !outputs.is_empty() { let write_txn = db.write_transaction()?; + let mut batch_count = 0; for (_, output_data) in outputs { // Generate PayRef and add to index let payref = LMDBDatabase::generate_payment_reference_for_output(&block_hash, &output_data.output.commitment); let output_hash = &output_data.hash; // Insert into PayRef index lmdb_replace( &write_txn, &db.payref_to_output_index, &payref, output_hash, None, )?; rebuild_count += 1; + batch_count += 1; + + // Commit batch every 1000 outputs to avoid large transactions + if batch_count >= 1000 { + write_txn.commit()?; + batch_count = 0; + let write_txn = db.write_transaction()?; + } } - write_txn.commit()?; + if batch_count > 0 { + write_txn.commit()?; + } }Also applies to: 3087-3162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base_layer/core/src/chain_storage/blockchain_database.rs(1 hunks)base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(14 hunks)base_layer/wallet_ffi/src/lib.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/core/src/chain_storage/blockchain_database.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/wallet_ffi/src/lib.rs (2)
base_layer/wallet_ffi/src/error.rs (2)
from(73-113)from(120-355)base_layer/common_types/src/payment_reference.rs (2)
parse_payment_reference_hex(133-151)payment_reference_to_hex(192-194)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (12)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (3)
162-162: LGTM: PayRef database setup is correctly implemented.The new PayRef-to-output index database is properly added to all necessary locations:
- Constant definition
- Database creation
- Field initialization
- Database reference
This follows the established pattern for other database indexes in the codebase.
Also applies to: 216-216, 274-274, 331-331
611-620: LGTM: PayRef generation and indexing is correctly implemented.The PayRef generation follows the specified formula using the shared utility function and properly indexes the result. The helper method correctly delegates to the shared
generate_payment_referencefunction with appropriate parameters.Key strengths:
- Uses domain-separated Blake2b hash as specified
- Properly maps PayRef to output hash for efficient lookups
- Follows established patterns for database indexing
Also applies to: 645-649
1884-1906: LGTM: PayRef lookup methods are well-implemented.The
fetch_output_by_payref_in_txnandcheck_output_spent_statusmethods are correctly implemented:
- Proper delegation to existing methods for consistency
- Good documentation explaining the functionality
- Correct error handling and return types
The methods follow established patterns and reuse existing functionality appropriately.
base_layer/wallet_ffi/src/lib.rs (9)
88-88: LGTM! Appropriate imports for PayRef functionality.The imports correctly bring in the necessary types for the Payment Reference implementation.
236-278: Well-structured FFI types for PayRef functionality.The FFI structs are properly designed with:
#[repr(C)]for C-compatible memory layout- Appropriate C types for cross-language compatibility
- Fixed-size arrays for payment references
- Clear documentation comments
Note that
TariPaymentDetails.commitmentuses a raw pointer which requires careful memory management - this is handled correctly in the corresponding destroy function.
6094-6094: Correct conversion of tor cookie to hex format.The change properly dereferences the tor_cookie pointer and converts it to hex format.
10641-10689: Robust implementation of payment reference lookup.The function demonstrates excellent FFI practices:
- Comprehensive null pointer checks
- Safe conversion of C arrays to Rust arrays
- Proper CString allocation for the commitment field
- Clear handling of the "not found" case by returning null
- Appropriate error propagation
10691-10775: Well-implemented payment reference listing functions.Both
wallet_get_available_payment_referencesandwallet_get_all_payment_referencesdemonstrate:
- Consistent implementation pattern
- Proper conversion of internal types to FFI types
- Safe handling of optional timestamps
- Correct mapping of PaymentDirection enum values
10777-10860: Correct implementation of PayRef configuration management.The get/set config functions properly handle:
- Bidirectional conversion between FFI and internal types
- Correct enum mapping for display formats
- Default values for custom format (8 chars prefix/suffix)
- Safe memory management with Box allocations
10862-10911: Proper hex parsing with C-compatible memory allocation.The function correctly:
- Validates hex string format
- Ensures exactly 32 bytes for payment reference
- Uses
libc::mallocfor C-compatible memory allocation- Includes proper error handling for allocation failures
11087-11304: Well-designed utility functions for PayRef operations.The utility functions provide comprehensive functionality:
wallet_get_payment_reference_status: Properly determines status based on confirmationswallet_format_payment_reference: Flexible formatting with Full/Shortened/Custom optionswallet_validate_payment_reference_format: Basic but effective hex validationwallet_verify_payment_reference: Returns structured JSON perfect for exchange integrationThe JSON format in the verification function is particularly well-suited for API consumers.
10944-11039: Excellent memory management implementation.The destroy functions and helper methods ensure proper cleanup:
- Each FFI type has a corresponding destroy function
payment_details_destroycorrectly frees the nested commitment stringpayment_reference_destroyuseslibc::freeto match thelibc::mallocallocation- Array access helpers (
get_length,get_at) include bounds checkingThis prevents memory leaks and ensures safe FFI usage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
docs/guide/grpc_overview.md (1)
553-562: Document all new Wallet PayRef RPCs
The "Payment Reference Methods" section only listsGetTransactionPayRefs. It should also includeGetTransactionsWithPayRefs,GetPaymentByReference,GetUnspentPaymentReferences, andGetAllPaymentReferenceswith their purpose, request/response schemas, and examples to match the existing style.base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)
667-755: 🛠️ Refactor suggestionReview complex migration batch processing for production readiness.
The migration batch processing is comprehensive with good practices like resumability, error handling, and disk space monitoring. However, the complexity warrants verification of edge cases:
Strengths:
- Resumable migration with checkpointing
- Batch processing for memory efficiency
- Retry logic for transient failures
- Disk space monitoring during migration
- Proper error categorization (retryable vs non-retryable)
Potential concerns:
- Nested transaction: Line 680 creates a read transaction while inside a write transaction context, which could cause issues
- Memory usage: Processing outputs in chunks is good, but verify the chunk size is appropriate for large blocks
Consider using the existing write transaction for reads to avoid nested transactions:
for height in batch_start..=batch_end { - let read_txn = db.read_transaction()?; - // Get block header to get block hash - let header: Option<BlockHeader> = lmdb_get(&read_txn, &db.headers_db, &height)?; + let header: Option<BlockHeader> = lmdb_get(&write_txn, &db.headers_db, &height)?; if let Some(header) = header { let block_hash = header.hash(); // Get all outputs for this block let outputs: Vec<(Vec<u8>, TransactionOutputRowData)> = - lmdb_fetch_matching_after(&read_txn, &db.utxos_db, block_hash.as_slice())?; + lmdb_fetch_matching_after(&write_txn, &db.utxos_db, block_hash.as_slice())?; - drop(read_txn);base_layer/wallet_ffi/src/lib.rs (1)
9522-9563:⚠️ Potential issueCritical: Still using total transaction amount for individual outputs.
This is a duplicate of a previously flagged issue. The function incorrectly assigns
tx.amount.as_u64()(total transaction amount) to each individual output's amount field. Each output should have its own specific amount value.This was previously identified in past reviews but remains unresolved. You need to fetch the actual amount for each output hash instead of using the total transaction amount. Consider:
- Adding an FFI function to get output details by hash
- Using existing UTXO queries to look up individual output amounts
- Modifying the transaction service to provide output amounts alongside hashes
- amount: tx.amount.as_u64(), + amount: get_output_amount_by_hash(&hash).unwrap_or_default(),
🧹 Nitpick comments (4)
docs/guide/applications_overview.md (1)
644-644: Consider hyphenating "Auto Update Configuration"
The heading "Auto Update Configuration" could be more readable as "Auto-Update Configuration" when used as a compound adjective.🧰 Tools
🪛 LanguageTool
[uncategorized] ~644-~644: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...- Blacklist period (default: 1440) **Auto Update Configuration**: -auto_update.update_...(AUTO_HYPHEN)
docs/guide/grpc_overview.md (1)
1020-1025: Add missing article for clarity
Change "Configure allowed methods in base node" to "Configure the allowed methods in the base node" for grammatical correctness.🧰 Tools
🪛 LanguageTool
[uncategorized] ~1023-~1023: You might be missing the article “the” here.
Context: ...lowlisting Configure allowed methods in base node: ```toml [base_node] grpc_server_a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
docs/src/09_adding_tari_to_your_exchange.md (2)
452-457: Clarify byte and bit reference
The note about checking the "second byte" and the "least significant bit" may confuse readers. Specify the exact byte index and bit position, for example:
- "Check the least significant bit of the second byte (0x01) to identify interactive addresses."
473-477: Specify language for code block
Annotate the code fence to enable syntax highlighting:- ``` + ```text PayRef = Blake2b_256(block_hash || output_hash)<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 474-474: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3b0fc3b3a9ddb736006676b97c6bc40cd46ff82c and 58d98cc994ea8332e90b540e261a0a7c4c8bcff8. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `applications/minotari_node/src/grpc/base_node_grpc_server.rs` (2 hunks) * `applications/minotari_node/src/grpc_method.rs` (4 hunks) * `base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs` (22 hunks) * `base_layer/wallet/src/transaction_service/handle.rs` (6 hunks) * `base_layer/wallet/src/transaction_service/protocols/transaction_receive_protocol.rs` (2 hunks) * `base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs` (6 hunks) * `base_layer/wallet/src/transaction_service/service.rs` (6 hunks) * `base_layer/wallet/tests/transaction_service_tests/storage.rs` (3 hunks) * `base_layer/wallet_ffi/src/lib.rs` (7 hunks) * `docs/guide/applications_overview.md` (1 hunks) * `docs/guide/grpc_overview.md` (1 hunks) * `docs/src/09_adding_tari_to_your_exchange.md` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * base_layer/wallet/tests/transaction_service_tests/storage.rs </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (6)</summary> * base_layer/wallet/src/transaction_service/protocols/transaction_receive_protocol.rs * base_layer/wallet/src/transaction_service/service.rs * applications/minotari_node/src/grpc_method.rs * base_layer/wallet/src/transaction_service/handle.rs * applications/minotari_node/src/grpc/base_node_grpc_server.rs * base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/src/09_adding_tari_to_your_exchange.md</summary> [grammar] ~463-~463: Possible subject-verb agreement error. Context: ... users claiming they sent payments that exchanges cannot verify. **Key Benefits for Exch... (NNS_THAT_AGREEMENT) --- [uncategorized] ~488-~488: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short). Context: ...en a customer claims they sent a deposit but you don't see it in your systems: **Tr... (COMMA_COMPOUND_SENTENCE) --- [duplication] ~499-~499: Possible typo: you repeated a word. Context: ...upport: Enters PayRef into verification system 3. System: "Payment verified: 100 XTR received in... (ENGLISH_WORD_REPEAT_RULE) </details> <details> <summary>docs/guide/applications_overview.md</summary> [uncategorized] ~343-~343: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ....low_power_polling_timeout=<seconds>` - Low power polling timeout (default: 300) - `walle... (EN_COMPOUND_ADJECTIVE_INTERNAL) --- [uncategorized] ~644-~644: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). Context: ...` - Blacklist period (default: 1440) **Auto Update Configuration**: - `auto_update.update_... (AUTO_HYPHEN) </details> <details> <summary>docs/guide/grpc_overview.md</summary> [uncategorized] ~1023-~1023: You might be missing the article “the” here. Context: ...lowlisting Configure allowed methods in base node: ```toml [base_node] grpc_server_a... (AI_EN_LECTOR_MISSING_DETERMINER_THE) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>docs/src/09_adding_tari_to_your_exchange.md</summary> 474-474: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 479-479: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 480-480: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 481-481: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 482-482: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 885-885: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 886-886: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 887-887: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 888-888: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 889-889: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 890-890: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 891-891: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 892-892: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 899-899: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 926-926: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (16)</summary><blockquote> <details> <summary>base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (9)</summary> `42-42`: **LGTM: PayRef infrastructure setup is well-structured.** The addition of PayRef imports, database constants, and migration optimization parameters is well-organized. The batch sizes and migration state structure provide good foundation for robust PayRef indexing. Also applies to: 159-159, 173-186 --- `627-635`: **LGTM: PayRef generation follows best practices.** The PayRef generation in `insert_output` correctly uses the shared utility function for consistency and domain separation. The immediate indexing ensures PayRefs are available as soon as outputs are created. --- `661-665`: **LGTM: Consistent PayRef generation utility.** Using the shared `generate_payment_reference` utility ensures consistent PayRef calculation across the codebase, following the formula `PayRef = Blake2b_256(block_hash || output_hash)` with domain separation. --- `884-912`: **LGTM: Robust PayRef cleanup during spending.** The PayRef cleanup logic when outputs are spent demonstrates excellent error handling: - Properly handles missing PayRefs (expected for older outputs) - Distinguishes between "not found" and actual database errors - Provides clear logging for debugging - Only fails the transaction on genuine database errors This addresses the previous review concerns about error handling. --- `1205-1230`: **LGTM: Intelligent PayRef cleanup during reorgs.** The PayRef cleanup during block reorganization correctly: - Only cleans PayRefs for outputs that should have them (not spent in same block, not burned) - Uses proper error handling that distinguishes expected vs unexpected errors - Provides appropriate logging for debugging The logic `!inputs.iter().any(|(_, r)| r.input.output_hash() == output_hash) && !utxo.output.is_burned()` correctly identifies outputs that should have PayRef entries. --- `1289-1299`: **LGTM: PayRef restoration during reorg rollback.** The code correctly restores PayRef entries when rolling back a reorg and moving inputs back to the UTXO set. Using the original output's mined header hash ensures PayRef consistency. --- `2020-2034`: **LGTM: Clean PayRef lookup implementation.** The `fetch_output_by_payref_in_txn` method provides a clean two-step lookup: 1. Resolve PayRef to output hash 2. Use existing `fetch_output_in_txn` for full output info This reuses existing functionality and maintains consistency. --- `2458-2461`: **LGTM: PayRef API exposure through BlockchainBackend trait.** The public `fetch_output_by_payref` method properly exposes PayRef functionality through the blockchain backend interface, enabling external access to PayRef lookups. --- `3079-3079`: **LGTM: Comprehensive PayRef migration with excellent error handling.** The PayRef migration logic demonstrates production-ready quality: **Excellent features:** - Resume capability with checkpointing to handle interruptions - Database integrity verification before migration - Comprehensive error handling with retry logic for transient failures - Disk space monitoring with appropriate thresholds - Proper cleanup of migration state on completion - Detailed logging for monitoring progress **Key improvements from previous reviews:** - Higher disk space thresholds (500MB/200MB vs previous 50MB) - Specific error type handling (only retry DbResizeRequired/AccessError) - Better resource monitoring during batch processing The migration version increment to 3 is appropriate for this significant schema change. Also applies to: 3207-3333 </details> <details> <summary>base_layer/wallet_ffi/src/lib.rs (7)</summary> `123-123`: **LGTM! Clean import additions for PayRef support.** The added imports for `generate_payment_reference` and `FixedHash` are necessary for the PayRef functionality and properly align with the implementation below. Also applies to: 130-130 --- `235-246`: **Well-designed FFI structs for C compatibility.** The `TariPaymentRecord` and `TariPaymentRecords` structs are properly designed for FFI: - Uses C-compatible types (`c_uchar`, `c_ulonglong`, `c_uint`) - Clear documentation of direction field values (0=Inbound, 1=Outbound, 2=Change) - Implements `Clone` for proper FFI handling - Uses `#[repr(C)]` for memory layout compatibility --- `4658-4659`: **LGTM! Simplified status extraction.** The direct assignment eliminates unnecessary intermediate variables while maintaining the same functionality. Also applies to: 5516-5517 --- `6063-6064`: **LGTM! Proper hex conversion for Tor authentication.** The use of `.to_hex()` provides clean string conversion for the Tor cookie authentication. --- `9564-9564`: **Good fix: Returns the constructed records.** The previous issue of missing return statement has been properly addressed - the function now correctly returns the built records vector. --- `9591-9631`: **Well-implemented PayRef lookup function.** The `wallet_get_transaction_by_payref` function properly: - Validates all input parameters with appropriate null checks - Converts ByteVector to FixedHash with error handling - Uses the correct transaction service method - Returns proper FFI-compatible pointers --- `10755-10814`: **Standard and correct FFI helper functions.** All the helper functions follow proper FFI patterns: - `payment_records_destroy`: Proper memory cleanup - `payment_records_get_length`: Safe length getter with null checks - `payment_records_get_at`: Bounds-checked index access with proper error handling - `payment_record_destroy`: Proper cleanup for individual records The implementations are safe and follow established FFI conventions in the codebase. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
base_layer/wallet_ffi/src/lib.rs (1)
9522-9535:⚠️ Potential issueCritical: Still using total transaction amount for individual outputs.
The code correctly generates PayRefs but incorrectly uses
tx.amount.as_u64()for all output types (sent, received, change). Each output should have its own specific amount, not the total transaction amount.This is the same issue flagged in previous reviews and still needs to be addressed.
You need to fetch the actual amount for each individual output hash instead of using the total transaction amount. Consider adding an FFI wrapper around a method to get output details by hash, then use it here:
- amount: tx.amount.as_u64(), + amount: get_output_amount_by_hash(wallet, &hash).unwrap_or(0),Also applies to: 9536-9549, 9550-9563
🧹 Nitpick comments (1)
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
2062-2065: Consider reducing logging verbosity.The
info!log statements for output counts inupdate_mined_heightmay be too verbose for production environments, potentially spamming logs during normal operation.Consider changing these to
debug!ortrace!level:- info!( + debug!( target: LOG_TARGET, "Transaction has {} sent outputs", sent.len() );Apply similar changes to the received and change output logging statements.
Also applies to: 2080-2082, 2097-2100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(11 hunks)applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(11 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(1 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(32 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(11 hunks)base_layer/wallet_ffi/src/lib.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/minotari_console_wallet/src/wallet_modes.rs
- base_layer/wallet/tests/transaction_service_tests/service.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- applications/minotari_console_wallet/src/ui/components/transactions_tab.rs
🔇 Additional comments (13)
base_layer/wallet_ffi/src/lib.rs (7)
123-130: LGTM! Clean import additions for PayRef functionality.The new imports for
generate_payment_referenceandFixedHashare properly added to support PayRef functionality in the FFI layer.
235-246: Well-structured FFI types for payment records.The
TariPaymentRecordandTariPaymentRecordsstructs are properly designed for FFI with appropriate C-compatible types and memory layout.
4658-4659: Minor refactoring to direct field access.These changes simplify the code by directly accessing the
statusfield instead of using method calls. The changes are consistent and correct.Also applies to: 5516-5517, 5804-5805
6063-6063: Fix for tor cookie hex conversion.Using
.to_hex()is the correct approach for converting bytes to hexadecimal representation. This appears to fix a previous implementation issue.
9564-9564: Return statement correctly added.The function now properly returns the constructed
TariPaymentRecords, fixing the previous issue where the records were built but not returned.
9591-9631: Robust PayRef-to-transaction lookup implementation.The function properly:
- Validates all input parameters
- Converts ByteVector to FixedHash with error handling
- Calls the underlying transaction service method
- Returns appropriate results with error propagation
The implementation is secure and handles edge cases correctly.
10759-10829: Standard FFI memory management functions.These helper functions provide proper lifecycle management for the new FFI types:
- Appropriate null checks
- Correct memory cleanup using
Box::from_raw- Bounds checking for array access
- Standard error handling patterns
The implementations follow established FFI patterns in the codebase.
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (6)
29-39: LGTM: Helper functions are well-implemented.The
fixedhash_vec_to_bytesandbytes_to_fixedhash_vecconversion functions are correctly implemented with proper error handling usingfilter_mapto handle invalid hash chunks.
985-989: LGTM: PayRef cleanup on unmined transactions.Properly deletes PayRefs when a transaction is set as unmined, with appropriate error handling that allows for the case where no PayRefs exist.
1153-1171: LGTM: PayRef lookup method is correctly implemented.The method properly handles the lookup chain from PayRef to transaction with appropriate error handling and null checks.
2429-2491: LGTM: Comprehensive PayrefSql implementation.The
PayrefSqlstruct and its methods provide complete CRUD operations with proper error handling, including upsert functionality and batch operations. The implementation follows consistent patterns with other SQL structs in the codebase.
2058-2110: LGTM: PayRef generation logic is correctly implemented.The PayRef generation in
update_mined_heightproperly handles all output types (sent, received, change) with appropriate null checking and uses the correctcommit_or_updatemethod for upsert functionality.
2159-2179: LGTM: Consistent PayRef cleanup implementation.The PayRef deletion logic in
set_as_unminedfollows the same pattern as the generation logic, properly handling all output types with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
base_layer/wallet_ffi/src/lib.rs (1)
9528-9569:⚠️ Potential issueCritical: Still using total transaction amount instead of individual output amounts.
This is a duplicate of a previously flagged issue. The function incorrectly assigns
tx.amount.as_u64()(total transaction amount) to every output record, when each output should have its own specific amount.This will cause incorrect amount reporting. Each output hash should be looked up to get its actual amount value, not the total transaction amount.
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
2467-2480: Remove excessive debug logging from production code.The
commit_or_updatemethod contains verboseinfo!logging statements that will clutter production logs and expose internal implementation details.- info!( - target: LOG_TARGET, - "payref: {:?}", self - ); let res = diesel::insert_into(payrefs::table) .values(self.clone()) .on_conflict(payrefs::output_hash) .do_update() .set(self) .execute(conn); - info!( - target: LOG_TARGET, - "result: {:?}", res - );
🧹 Nitpick comments (3)
applications/minotari_app_grpc/proto/wallet.proto (1)
1284-1286: Document new PayRef fields in TransactionInfo
The addedpayment_references_sent,payment_references_received, andpayment_references_changefields lack descriptive comments. Please annotate these fields in theTransactionInfomessage with details on their semantics and encoding.base_layer/wallet_ffi/src/lib.rs (2)
9528-9569: Refactor: Extract PayRef generation logic to reduce duplication.The PayRef generation logic is duplicated across sent, received, and change output loops. Consider extracting this into a helper function.
+fn generate_payref_record( + hash: &FixedHash, + block_hash: Option<FixedHash>, + tx: &CompletedTransaction, + direction: c_uint, + amount: c_ulonglong, +) -> TariPaymentRecord { + let mut payref = [0u8; 32]; + if let Some(block_hash) = block_hash { + let pay_ref = generate_payment_reference(&block_hash, hash); + payref.copy_from_slice(pay_ref.as_slice()); + } + TariPaymentRecord { + payment_reference: payref, + amount, + block_height: tx.mined_height.unwrap_or_default(), + mined_timestamp: tx.mined_timestamp.map(|ts| ts.timestamp() as c_ulonglong).unwrap_or(0), + direction, + } +} for hash in tx.sent_output_hashes { - let mut payref = [0u8; 32]; - if let Some(block_hash) = tx.mined_in_block { - let pay_ref = generate_payment_reference(&block_hash, &hash); - payref.copy_from_slice(pay_ref.as_slice()); - }; - records.push(TariPaymentRecord { - payment_reference: payref, - amount: tx.amount.as_u64(), - block_height: tx.mined_height.unwrap_or_default(), - mined_timestamp: tx.mined_timestamp.map(|ts| ts.timestamp() as c_ulonglong).unwrap_or(0), - direction: 1, - }); + records.push(generate_payref_record(&hash, tx.mined_in_block, &tx, 1, /* actual_amount */)); }
9539-9539: Use named constants for direction values.The hardcoded direction values (0, 1, 2) should be defined as named constants for better readability and maintainability.
+const PAYMENT_DIRECTION_INBOUND: c_uint = 0; +const PAYMENT_DIRECTION_OUTBOUND: c_uint = 1; +const PAYMENT_DIRECTION_CHANGE: c_uint = 2; - direction: 1, + direction: PAYMENT_DIRECTION_OUTBOUND, - direction: 0, + direction: PAYMENT_DIRECTION_INBOUND, - direction: 2, + direction: PAYMENT_DIRECTION_CHANGE,Also applies to: 9553-9553, 9567-9567
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
applications/minotari_app_grpc/proto/wallet.proto(5 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(11 hunks)applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(11 hunks)base_layer/wallet/src/output_manager_service/service.rs(2 hunks)base_layer/wallet/src/transaction_service/service.rs(6 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(32 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(11 hunks)base_layer/wallet_ffi/src/lib.rs(8 hunks)base_layer/wallet_ffi/wallet.h(4 hunks)comms/core/src/noise/socket.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- comms/core/src/noise/socket.rs
- base_layer/wallet/src/output_manager_service/service.rs
- base_layer/wallet/tests/transaction_service_tests/service.rs
- base_layer/wallet_ffi/wallet.h
- base_layer/wallet/src/transaction_service/service.rs
- applications/minotari_console_wallet/src/ui/components/transactions_tab.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (17)
applications/minotari_app_grpc/proto/wallet.proto (4)
1535-1547: New messages look correct
TheGetTransactionPayRefsRequestandGetTransactionPayRefsResponsedefinitions follow the expected pattern for RPC messages. No issues found.
1550-1558: GetTransactionsWithPayRefsResponse is well-formed
The response bundlingTransactionInfo, pay refs, andrecipient_countaligns with design objectives. No comments.
1560-1564: GetPaymentByReferenceRequest is valid
Thebytes payment_referencefield enforces the 32-byte hash. Definition is correct.
1566-1571: GetPaymentByReferenceResponse is valid
TheTransactionInfo transactionfield provides full details on lookup. Definition is correct.base_layer/wallet_ffi/src/lib.rs (6)
123-123: LGTM - Clean imports for PayRef functionality.The new imports
payment_reference::generate_payment_referenceandFixedHashare appropriate for the PayRef functionality being added.Also applies to: 130-130
236-246: Well-designed FFI structures for payment records.The
TariPaymentRecordstruct andTariPaymentRecordswrapper follow good FFI patterns with C-compatible types and proper memory layout.
4658-4659: Good optimization - removing unnecessary clones.Removing
.clone()calls when accessing transaction status is a good performance improvement since enum values can be copied directly.Also applies to: 5516-5517, 5804-5805
6063-6064: LGTM - Updated hex encoding method.The change from
hex::to_hexto.to_hex()method call appears to be a refactor to use a more direct API.
9597-9637: Well-implemented PayRef lookup function.The
wallet_get_transaction_by_payreffunction correctly handles null checks, error propagation, and type conversions. The use ofFixedHash::try_fromprovides proper validation.
10765-10835: Comprehensive FFI helper functions for payment records.The helper functions for managing
TariPaymentRecordslifecycle are well-implemented with proper null checks, bounds validation, and memory management. The collection pattern follows standard FFI practices.base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (7)
30-40: LGTM! Helper functions are well-implemented.The FixedHash conversion helper functions are correctly implemented with proper error handling using
filter_mapto skip invalid chunks and appropriate chunk size validation.
1268-1268: LGTM! Output hash fields properly added to SQL structs.The new optional
Vec<u8>fields for storing serialized output hashes are correctly added to all transaction SQL structs with appropriateOptiontypes for backward compatibility.Also applies to: 1544-1545, 1823-1825
986-990: LGTM! PayRef cleanup logic is properly implemented.The PayRef deletion in
set_transaction_as_unminedcorrectly handles cleanup with appropriate error handling that doesn't fail if no PayRefs exist.
1164-1182: LGTM! Transaction lookup by PayRef is correctly implemented.The
get_transaction_with_payrefmethod properly:
- Handles database connection and cipher acquisition
- Performs PayRef lookup with proper error handling
- Returns None when PayRef doesn't exist
- Converts transaction ID correctly and looks up the transaction
2037-2126: LGTM! PayRef generation logic is well-structured.The PayRef generation in
update_mined_heightcorrectly:
- Retrieves existing transaction data first
- Converts stored byte arrays to FixedHash vectors safely using helper functions
- Generates PayRefs for all output types (sent, received, change)
- Uses commit_or_update for proper upsert behavior
- Includes informative logging for debugging
The logic properly handles the case where output hashes may not exist and safely processes each category of outputs.
2131-2195: LGTM! PayRef cleanup in unmining is correctly implemented.The PayRef deletion logic in
set_as_unminedproperly:
- Retrieves existing transaction data
- Safely converts output hash bytes to FixedHash vectors
- Deletes PayRefs for all output categories
- Uses the correct hash references for deletion operations
2445-2507: LGTM! PayrefSql struct and operations are well-implemented.The PayrefSql struct provides comprehensive database operations including:
- Basic commit operation
- Upsert functionality with commit_or_update
- Deletion by output hash and transaction ID
- Lookup by output hash with proper Optional handling
- Correct use of Diesel ORM patterns
The database operations properly handle all required use cases for PayRef management.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs(20 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
🔇 Additional comments (9)
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (9)
42-42: LGTM!The import of
generate_payment_referenceis correctly added and will be used for PayRef generation.
159-159: LGTM on PayRef database setup!The PayRef database index is properly added throughout the database setup:
- Database constant defined
- Added to LMDBBuilder configuration
- Struct field added and initialized
- Included in
all_dbsmethod with correct count (32)The database setup follows the established patterns in the codebase.
Also applies to: 214-214, 272-272, 329-329, 562-562
610-619: LGTM on PayRef generation during output insertion!The PayRef generation and insertion logic is correct:
- Uses the block hash and output hash to generate PayRef
- Properly inserts into the PayRef index
- Follows the established pattern for database insertions
The placement after commitment insertion but before the main output insertion is appropriate.
644-648: LGTM on PayRef generation helper!The helper method properly delegates to the shared utility function and maintains consistency with the PayRef generation formula used throughout the system.
775-803: LGTM on improved PayRef cleanup logic!The PayRef cleanup during input insertion is well-implemented:
- Properly fetches output info to get the original block hash for PayRef calculation
- Uses appropriate error handling instead of silently ignoring errors
- Correctly handles the case where PayRef might not exist for older outputs
- Fails the transaction on actual database errors, maintaining data consistency
This addresses the concerns raised in past review comments about error handling.
1098-1123: LGTM on PayRef cleanup during reorg!The reorg PayRef cleanup logic is correctly implemented:
- Properly determines when PayRef cleanup should occur (not spent in same block, not burned)
- Uses the block hash to generate the correct PayRef for deletion
- Implements proper error handling that distinguishes between expected (not found) and actual errors
- Maintains transaction integrity by returning errors for genuine database issues
This addresses previous concerns about error handling consistency.
1182-1192: LGTM on PayRef restoration during reorg!The logic to restore PayRef entries when moving inputs back to the UTXO set during reorg is correct:
- Uses the original output's header hash to generate the correct PayRef
- Properly inserts the PayRef back into the index
- Maintains consistency with the PayRef generation logic
1913-1927: LGTM on PayRef lookup functionality!The PayRef lookup methods are well-implemented:
fetch_output_by_payref_in_txncorrectly looks up output hash by PayRef and delegates to existing fetch logicfetch_output_by_payrefproperly exposes the functionality through the public interface- Both methods follow established patterns in the codebase
Also applies to: 2351-2354
2972-2972: LGTM on migration version update!The migration version is correctly incremented to 3 to trigger the PayRef migration for existing databases.
Fixed payment id cutoff display on the console wallet detail transactions tab
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs (1)
117-131: Minor polish: avoid treatingNoneas0when comparing mined height
tx.mined_height.unwrap_or_default()equatesNonewith0, meaning a previously-unmined tx (height =None) and a tx mined at block 0 are indistinguishable and will not raisestate_changed.
If the blockchain ever allows block-height 0 (e.g. in a test-chain), this could cause silent state divergence.- if tx.mined_height.unwrap_or_default() != output_info_for_tx_id.mined_height.unwrap_or_default() { + if tx.mined_height != output_info_for_tx_id.mined_height {A direct
Optioncomparison is both simpler and semantically accurate.Also applies to: 208-222
base_layer/wallet/src/transaction_service/storage/models.rs (1)
298-329: Avoid repeated allocation incalculate_*_payment_referencesThese helpers rebuild
Vec<PaymentReference>on every call.
If they are hot-path, consider caching or returning an iterator:pub fn iter_received_payment_references( &self, ) -> impl Iterator<Item = PaymentReference> + '_ { self.mined_in_block .iter() .flat_map(|block_hash| self.received_output_hashes.iter().map(move |h| generate_payment_reference(block_hash, h))) }Not blocking, but worth profiling.
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (4)
30-40: Add explicit length validation when (de)serialisingFixedHashvectors
bytes_to_fixedhash_vecsilently drops trailing bytes that are not a multiple of 32 bytes, which can mask data-corruption bugs at runtime.
Consider asserting that the input length is a multiple of 32 (or returning an error) so malformed data is caught early.fn bytes_to_fixedhash_vec(bytes: &[u8]) -> Vec<FixedHash> { - bytes - .chunks_exact(32) - .filter_map(|chunk| FixedHash::try_from(chunk).ok()) - .collect() + assert!( + bytes.len() % 32 == 0, + "bytes_to_fixedhash_vec() expects a multiple of 32-byte slices, got {} bytes", + bytes.len() + ); + bytes + .chunks_exact(32) + .map(|chunk| FixedHash::try_from(chunk).expect("length asserted above")) + .collect() }
1171-1175: Avoid variable shadowing for clarityThe local
let payref = …shadows the function argumentpayref, which obscures the bug above and hurts readability. Rename the local binding (e.g.row) to make intent explicit.
2037-2113: Bulk-insert PayRefs to reduce DB round-trips
update_mined_heightexecutescommit_or_updateonce per output (three separate loops). On multi-output transactions this results in 𝑂(n) synchronous DB calls, which hurts latency on mobile/light wallets.Consider building a
Vec<PayrefSql>and inserting with a singleinsert_into(..).values(&vec)per category, or usingon_conflict_do_nothingfor the whole batch.
986-990: Redundant PayRef deletion
set_transaction_as_unmineddeletes PayRefs viaPayrefSql::delete_by_tx_idand then again one-by-one further down inCompletedTransactionSql::set_as_unmined, resulting in two deletions of the same rows. While harmless, it is extra work and can hide future issues.
Remove one of the two deletion paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
applications/minotari_console_wallet/src/ui/state/app_state.rs(6 hunks)base_layer/wallet/src/storage/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/protocols/transaction_validation_protocol.rs(7 hunks)base_layer/wallet/src/transaction_service/service.rs(31 hunks)base_layer/wallet/src/transaction_service/storage/models.rs(11 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(32 hunks)base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs(4 hunks)base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/wallet/src/storage/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_console_wallet/src/ui/state/app_state.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (1)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
resources(575-592)resources(598-615)resources(689-691)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (3)
base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (1)
846-853: Test updates correctly wire the newOutputManagerHandledependencyAll invocations of
TransactionValidationProtocol::new(..)have been updated to includeresources.output_manager_service.clone(), keeping the test-suite in sync with the constructor signature. No other behavioural changes are introduced and the cloning cost is negligible (OutputManagerHandleis a cheapArc).Also applies to: 888-889, 936-944, 1030-1037, 1097-1104, 1269-1276, 1379-1386
base_layer/wallet/src/transaction_service/service.rs (1)
1219-1243: Possible mis-classification of output hashes for self-paymentsFor the “pay-to-self” fast-path you categorise all outputs as
received_output_hashesand leavesent_output_hashes/change_output_hashes
empty:CompletedTransaction::new_with_output_hashes( /* … */, // tx meta vec![], // sent all_outputs, // received vec![], // change )If the transaction contains change, the PayRef derived from that output will be
labelled as received, which may break later direction checks and reconciliation
logic.Please confirm the expected semantics of the three hash buckets and adjust the
assignment (e.g. splitall_outputsinto sent vs. change) or add inline
documentation clarifying why this is correct for the self-payment case.base_layer/wallet/src/transaction_service/storage/models.rs (1)
332-348: Conversion keepsreceived_output_hashesbut dropssent_output_hashes
InboundTransaction::from(CompletedTransaction)copies onlyreceived_output_hashes.
If a completed inbound tx ever had change outputs recorded, that data is lost.
Verify that this is intentional; otherwise propagatechange_output_hashestoo.
base_layer/wallet/src/transaction_service/protocols/transaction_validation_protocol.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
base_layer/wallet/src/transaction_service/payment_reference.rs (4)
183-193: Utility duplication – extract common PayRef helpers
PaymentDetailsandPaymentRecordboth expose identical helpers (payref_hex, confirmation checks, formatting).
Duplicating logic invites drift; move these into a dedicated helper trait/impl (e.g.PayRefExt) and implement it for both structs.No functional change, but simpler maintenance & test coverage.
63-70: ProvideDisplayimpl and validation helper forPayRefDisplayFormatDown-stream UI / CLI code will repeatedly match on this enum.
Implementingfmt::Display(and a validation constructor forCustom) will:
- Remove boilerplate
matchblocks scattered across the codebase.- Prevent invalid
prefix_chars/suffix_charsat creation time instead of every formatting call.Example:
impl TryFrom<(u8,u8)> for PayRefDisplayFormat { type Error = &'static str; fn try_from((p,s): (u8,u8)) -> Result<Self, Self::Error> { if p == 0 && s == 0 { return Err("prefix and suffix cannot both be zero"); } Ok(Self::Custom{ prefix_chars: p, suffix_chars: s }) } }
190-192: Saturating comparison defends against u64 overflow
confirmationsisu64; although practically impossible to overflow, usingsaturating_subor>=is fine.
Consider future-proofing by making the API explicit:pub fn has_sufficient_confirmations(&self, required: u64) -> bool { self.confirmations.saturating_sub(required) >= 0 }
150-161: AddDisplayforVerificationStatusSerialisation is covered via
Serialize, but human-readable output (CLI logs, error messages) will benefit from a short, canonical string representation.impl fmt::Display for VerificationStatus { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let s = match self { VerificationStatus::Verified => "verified", VerificationStatus::InsufficientConfirmations => "insufficient_confirmations", VerificationStatus::NotFound => "not_found", VerificationStatus::Invalid => "invalid", }; f.write_str(s) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_app_grpc/proto/base_node.proto(2 hunks)applications/minotari_app_grpc/proto/wallet.proto(5 hunks)base_layer/wallet/src/transaction_service/payment_reference.rs(1 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(32 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- applications/minotari_app_grpc/proto/base_node.proto
- applications/minotari_app_grpc/proto/wallet.proto
- base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- 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
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)
2965-2976: 🛠️ Refactor suggestionHex-format validation duplicates earlier logic
These lines re-implement the 64-char ASCII-hex validation that was already flagged in previous reviews. Please extract the common logic into the proposed
parse_payment_reference_hexhelper to avoid copy-paste errors and keep validation consistent.-// Validate PayRef format (64 hex chars = 32 bytes) -if payref_hex.len() != 64 || !payref_hex.chars().all(|c| c.is_ascii_hexdigit()) { - ... -} +let payref_bytes = match parse_payment_reference_hex(&payref_hex) { + Ok(bytes) => bytes, + Err(status) => { + if tx.send(Err(status)).await.is_err() { break; } + continue; + } +};
2978-2991: 🛠️ Refactor suggestionUse the extracted helper for hex→
FixedHashconversion
FixedHash::from_hexplus manualmatchrepeats the conversion branch that the helper would already cover (and also performs length/character checks). Swapping to the helper removes duplication and short-circuits on error:-let payref_bytes = match FixedHash::from_hex(&payref_hex) { - Ok(v) => v, - Err(e) => { - ... - } -}; +let payref_bytes = match parse_payment_reference_hex(&payref_hex) { + Ok(v) => v, + Err(status) => { + if tx.send(Err(status)).await.is_err() { break; } + continue; + } +};docs/guide/grpc_overview.md (1)
553-564: Add missing Wallet PayRef RPCs documentation
The Wallet service’s "Payment Reference Methods" section currently only listsGetTransactionPayRefs. You need to document the other new Wallet RPCs introduced by this PR:
GetTransactionsWithPayRefsGetPaymentByReferenceGetUnspentPaymentReferencesGetAllPaymentReferences
Include purpose, request/response schemas, and usage examples to match the existing style.
🧹 Nitpick comments (1)
docs/guide/grpc_overview.md (1)
1023-1023: Grammar: add missing article
Revise for clarity:- Configure allowed methods in base node: + Configure allowed methods in the base node:🧰 Tools
🪛 LanguageTool
[uncategorized] ~1023-~1023: You might be missing the article “the” here.
Context: ...lowlisting Configure allowed methods in base node: ```toml [base_node] grpc_server_a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
applications/minotari_app_grpc/proto/base_node.proto(2 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(2 hunks)docs/guide/grpc_overview.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_app_grpc/proto/base_node.proto
🧰 Additional context used
🪛 LanguageTool
docs/guide/grpc_overview.md
[uncategorized] ~1023-~1023: You might be missing the article “the” here.
Context: ...lowlisting Configure allowed methods in base node: ```toml [base_node] grpc_server_a...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (1)
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
3006-3016: Potential privacy leak:min_value_promiseexposed unconditionallyThe response includes
min_value_promise(and full commitment). If the intention is privacy-preserving verification only, exposing value information may contradict the PR’s stated goals. Confirm this field is safe to reveal or gate it behind a configuration flag.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs (3)
224-228: DuplicatePayRefrefresh pattern – extract into helper to de–duplicateThe
should_we_trigger_tx_update_for_payref+trigger_full_tx_state_refreshpattern is repeated in three different branches (UTXO scanner completed, base-node peer change, base-node state change). Repeating the same four-line block makes future maintenance error-prone.Consider adding a small helper and re-using it:
+async fn maybe_trigger_full_tx_refresh_for_payref(&mut self) { + if self.should_we_trigger_tx_update_for_payref().await { + self.trigger_full_tx_state_refresh().await; + } +}and replace the current block with
-if self.should_we_trigger_tx_update_for_payref().await{ - self.trigger_full_tx_state_refresh().await; -} +self.maybe_trigger_full_tx_refresh_for_payref().await;This keeps the event loop succinct and localises PayRef-specific logic in one place.
241-244: Same duplicated PayRef check hereThis branch repeats the exact PayRef refresh logic. Once the helper suggested above is introduced, replace this block with a single call to avoid copy-paste logic drifting.
251-254: And here as wellThird occurrence of identical code. Refactor as per previous comment for consistency and easier future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
applications/minotari_console_wallet/src/ui/state/app_state.rs(6 hunks)applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs(3 hunks)base_layer/common_types/src/payment_reference.rs(1 hunks)base_layer/wallet/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- base_layer/wallet/Cargo.toml
- applications/minotari_console_wallet/src/ui/state/app_state.rs
- base_layer/common_types/src/payment_reference.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs (1)
applications/minotari_console_wallet/src/ui/state/app_state.rs (2)
trigger_wallet_scanned_height_update(1096-1100)should_we_trigger_tx_update_for_payref(880-887)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (1)
applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs (1)
332-335: Lightweight, non-blocking read – looks goodThe new async helper correctly takes only a read lock and does not hold it across an
.await, preventing potential dead-lock scenarios. Implementation is concise and delegates toAppState.
* development: (607 commits) Wallet GRPC port comment fix from 18142 to 18143 (tari-project#7221) feat: integrated address support for Ledger (tari-project#7198) chore: new release v4.1.1-pre.0 (tari-project#7211) fix: migration can now correctly resume after stopping (tari-project#7210) fix: only revalidated rejected transactions on startup (tari-project#7209) fix: add filtering flag back (tari-project#7208) feat: improve wallet balance checks from external clients (tari-project#7207) feat!: update grpc supply query (tari-project#7137) docs: Updated API GRPC and Exchange Guide (tari-project#7205) chore: new release v4.4.0-pre.0 (tari-project#7202) feat: update base node proto to search bytes (tari-project#7201) feat: full PayRef implementation (tari-project#7154) test: add ffi cucumber wallet balance test (tari-project#7189) chore: fix tests (tari-project#7196) fix(network-discovery): add back idle event handling (tari-project#7194) Update SECURITY.md (tari-project#7193) fix: transaction manager service unmined lookup (tari-project#7192) fix: wallet ffi database name mismatch for mobile wallet (tari-project#7191) fix: payment_id deserialize (tari-project#7187) fix: remove code for deleting stale peers (tari-project#7184) ...
3 participants
Description
Implements the Payment Reference (PayRef) system for Tari, providing globally unique identifiers for individual transaction outputs to enable payment verification in Mimblewimble-based blockchains. PayRefs solve the critical business problem of payment verification for exchanges and merchants without compromising privacy.
Key Features:
PayRef = Blake2b_256(block_hash || output_hash)for merkle proof compatibilityMotivation and Context
The primary use case is when users send payments to exchanges but the exchange claims they haven't received the payment. Currently, there's no reliable way to prove payments in Mimblewimble due to:
This creates significant business friction:
PayRefs provide a practical solution that ensures both sender and receiver can generate the same payment reference for verification, while maintaining privacy and enabling future merkle proof verification against block headers.
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Basic PayRef Generation:
Multi-Output Transactions:
PayRef Search:
tfor transactions, thensto searchReorg Safety Testing:
FFI Interface:
cargo buildwallet.hincludes new PayRef structures with array supportDocumentation:
docs/src/09_adding_tari_to_your_exchange.mddocs/src/payref_userguide.mdAPI Testing:
Breaking Changes
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Refactor
Chores