feat: validate payment id size#7335
Conversation
…ing stuck in the mempool
…ons for replace_by_fee
…and replace_by_fee
Test Results (Integration tests)1 tests 1 ✅ 0s ⏱️ For more details on these parsing errors, see this check. Results for commit fc6f4c5. |
|
Marking it as ready for review just to get some code rabbit comments, do not merge yet |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (10)
base_layer/wallet/src/output_manager_service/service.rs (7)
2052-2055: Consider error handling for PaymentId construction.The function call
PaymentId::open_from_string()returns aPaymentIddirectly, but based on the pattern of other similar calls in the file, this should also use a constructor method and handle potential errors.- .with_payment_id(PaymentId::open_from_string( - "Pay to self transaction", - TxType::PaymentToSelf, - )); + .with_payment_id(PaymentId::open_from_string( + "Pay to self transaction", + TxType::PaymentToSelf, + ) + .map_err(|e| OutputManagerError::ServiceError(format!("Invalid payment ID: {}", e)))?);
2502-2508: Consider error handling for PaymentId construction.The
.unwrap()call could panic if the PaymentId construction fails. Consider handling this error more gracefully.- .with_payment_id(PaymentId::open_from_string( - &format!( - "Coin split transaction, {} into {} outputs", - accumulated_amount, number_of_splits - ), - TxType::CoinSplit, - )) + .with_payment_id(PaymentId::open_from_string( + &format!( + "Coin split transaction, {} into {} outputs", + accumulated_amount, number_of_splits + ), + TxType::CoinSplit, + ) + .map_err(|e| OutputManagerError::BuildError(format!("Invalid payment ID: {}", e)))?)
2536-2536: Consider error handling for PaymentId construction.The function call could fail during PaymentId construction. Consider handling this error more gracefully.
- PaymentId::open_from_string(&format!("{} even coin splits", number_of_splits), TxType::CoinSplit), + PaymentId::open_from_string(&format!("{} even coin splits", number_of_splits), TxType::CoinSplit) + .map_err(|e| OutputManagerError::ServiceError(format!("Invalid payment ID: {}", e)))?,
2673-2676: Consider error handling for PaymentId construction.The function call could fail during PaymentId construction. Consider handling this error more gracefully.
- let payment_id = PaymentId::open_from_string( - &format!("Coin split, {} into {} outputs", accumulated_amount, number_of_splits), - TxType::CoinSplit, - ); + let payment_id = PaymentId::open_from_string( + &format!("Coin split, {} into {} outputs", accumulated_amount, number_of_splits), + TxType::CoinSplit, + ) + .map_err(|e| OutputManagerError::ServiceError(format!("Invalid payment ID: {}", e)))?;
3034-3034: Consider error handling for PaymentId construction.The function call could fail during PaymentId construction. Consider handling this error more gracefully.
- .with_payment_id(PaymentId::open_from_string("scraping wallet", TxType::PaymentToOther)) + .with_payment_id(PaymentId::open_from_string("scraping wallet", TxType::PaymentToOther) + .map_err(|e| OutputManagerError::ServiceError(format!("Invalid payment ID: {}", e)))?)
3131-3134: Consider error handling for PaymentId construction.The function call could fail during PaymentId construction. Consider handling this error more gracefully.
- .with_payment_id(PaymentId::open_from_string( - "SHA-XTR atomic swap", - TxType::ClaimAtomicSwap, - )) + .with_payment_id(PaymentId::open_from_string( + "SHA-XTR atomic swap", + TxType::ClaimAtomicSwap, + ) + .map_err(|e| OutputManagerError::ServiceError(format!("Invalid payment ID: {}", e)))?)
3214-3217: Consider error handling for PaymentId construction.The function call could fail during PaymentId construction. Consider handling this error more gracefully.
- .with_payment_id(PaymentId::open_from_string( - "SHA-XTR atomic refund", - TxType::HtlcAtomicSwapRefund, - )) + .with_payment_id(PaymentId::open_from_string( + "SHA-XTR atomic refund", + TxType::HtlcAtomicSwapRefund, + ) + .map_err(|e| OutputManagerError::ServiceError(format!("Invalid payment ID: {}", e)))?)base_layer/core/src/transactions/transaction_components/payment_id.rs (3)
497-525: Replace panic! with proper error handlingThe
add_sender_addressmethod usespanic!whennew_address_and_datafails, which could cause runtime crashes if the total size exceeds 256 bytes. This should return aResultinstead.pub fn add_sender_address( self, sender_address: TariAddress, sender_one_sided: bool, fee: MicroMinotari, tx_type: Option<TxType>, -) -> PaymentId { +) -> Result<PaymentId, String> { match self.inner { InnerPaymentId::Open { user_data, tx_type } => { - match PaymentId::new_address_and_data(sender_address, fee, sender_one_sided, tx_type, user_data) { - Ok(payment_id) => payment_id, - Err(e) => panic!("Cannot create AddressAndData PaymentId: {}", e), - } + PaymentId::new_address_and_data(sender_address, fee, sender_one_sided, tx_type, user_data) }, InnerPaymentId::Empty => { - match PaymentId::new_address_and_data( + PaymentId::new_address_and_data( sender_address, fee, sender_one_sided, tx_type.unwrap_or_default(), vec![], - ) { - Ok(payment_id) => payment_id, - Err(e) => panic!("Cannot create AddressAndData PaymentId: {}", e), - } + ) }, - _ => self, + _ => Ok(self), } }
609-636: Validate lengths before converting to u8The
expect()calls could panic if address length, user data length, or hash count exceed 255. Add validation to return an error or handle gracefully.Consider validating these lengths before the conversion:
if address_bytes.len() > 255 || user_data.len() > 255 { // Handle error - perhaps store in Raw variant or return error }For sent_output_hashes on line 635:
if sent_output_hashes.len() > 255 { // Handle error appropriately }
657-669: Document the premine edge caseThis special handling for values < 1000 appears to be a compatibility hack. Please add a comment explaining why this edge case exists and what premine scenario it addresses.
// edge case for premine: +// Historical compatibility: Early premine transactions used 8-byte values < 1000 +// that need to be parsed as Open variant with PaymentToOther type if bytes.len() == SIZE_VALUE {
♻️ Duplicate comments (2)
base_layer/core/src/transactions/transaction_protocol/sender.rs (1)
1384-1385: Same change as above.Also applies to: 1510-1511, 1623-1624, 1816-1817
applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
1446-1448: Same unchecked constructor used here — see previous comment for rationale and suggested fix.
🧹 Nitpick comments (5)
base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs (2)
380-382: Minor repetition in tests
PaymentId::new_empty()is invoked several times in this file’s tests.
If aDefaultimpl exists (or could be added) returning the empty variant, test code could simply callPaymentId::default()to reduce repetition. Pure nit – feel free to ignore.Also applies to: 422-424
123-137: Optional micro-optimization: avoid one clone
self.payment_id = payment_id.clone();is followed by passingpayment_idby value toencrypt_data_for_recovery, forcing an extra allocation.
A tiny saving can be made by:self.payment_id = payment_id.clone(); // keep self.encrypted_data = key_manager .encrypt_data_for_recovery( &self.commitment_mask_key_id, custom_recovery_key_id, self.value.as_u64(), - payment_id, + self.payment_id.clone(), // reuse existing clone ) .await?;Not critical, but reads slightly cleaner.
base_layer/wallet/tests/transaction_service_tests/service.rs (1)
622-623:PaymentId::new_empty()looks good – considerDefaultfor tersenessThe direct replacement of the removed
PaymentId::Emptyenum with the validated constructorPaymentId::new_empty()aligns with the new API and keeps the tests compiling.
If thePaymentIdtype now implementsDefault, you could shave a few characters off and improve readability by passingPaymentId::default()instead, which conveys the same “empty” intent without introducing another semantic alias.This is purely a nicety; the current code is perfectly correct.
Also applies to: 1485-1486, 1789-1790, 1911-1912, 2018-2019, 2061-2062, 2559-2560, 2689-2691
applications/minotari_console_wallet/src/ui/state/app_state.rs (1)
1051-1056: Condense burn-flag calculation for readabilityThe two-level
if/else if ... elsecan be collapsed into a single boolean expression, which reads more clearly and eliminates three explicittrue/falseliterals.-let burn = if tx.transaction.body.contains_burn() { - true -} else if let Some(tx_type) = tx.payment_id.get_tx_type() { - tx_type == TxType::Burn -} else { - false -}; +let burn = tx.transaction.body.contains_burn() + || matches!(tx.payment_id.get_tx_type(), Some(TxType::Burn));No behavioral change – just simpler, idiomatic Rust without branching.
base_layer/wallet/src/transaction_service/service.rs (1)
3895-3899: Logic condition can be simplified for readabilityThe current check is correct but hard to parse:
if !(completed_tx.status == TransactionStatus::Completed || completed_tx.status == TransactionStatus::Broadcast || completed_tx.status == TransactionStatus::MinedUnconfirmed) || completed_tx.transaction.body.kernels().is_empty()A clearer version:
+let is_allowed_status = matches!( + completed_tx.status, + TransactionStatus::Completed + | TransactionStatus::Broadcast + | TransactionStatus::MinedUnconfirmed +); + +if !is_allowed_status || completed_tx.transaction.body.kernels().is_empty() {Same semantics, greatly improved readability.
applications/minotari_console_wallet/src/ui/components/send_tab.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs
Outdated
Show resolved
Hide resolved
| /// Unchecked constructor for Empty payment ID | ||
| /// Use this for migration from old direct enum construction | ||
| pub fn empty() -> Self { | ||
| PaymentId { | ||
| inner: InnerPaymentId::Empty, | ||
| } | ||
| } | ||
|
|
||
| /// Unchecked constructor for Open payment ID | ||
| /// Use this for migration from old direct enum construction | ||
| /// WARNING: This bypasses validation - use new_open() for new code | ||
| pub fn open_unchecked(user_data: Vec<u8>, tx_type: TxType) -> Self { | ||
| PaymentId { | ||
| inner: InnerPaymentId::Open { user_data, tx_type }, | ||
| } | ||
| } | ||
|
|
||
| /// Unchecked constructor for Raw payment ID | ||
| /// Use this for migration from old direct enum construction | ||
| /// WARNING: This bypasses validation - use new_raw() for new code | ||
| pub fn raw_unchecked(data: Vec<u8>) -> Self { | ||
| PaymentId { | ||
| inner: InnerPaymentId::Raw(data), | ||
| } | ||
| } | ||
|
|
||
| /// Unchecked constructor for AddressAndData payment ID | ||
| /// Use this for migration from old direct enum construction | ||
| /// WARNING: This bypasses validation - use new_address_and_data() for new code | ||
| pub fn address_and_data_unchecked( | ||
| sender_address: TariAddress, | ||
| sender_one_sided: bool, | ||
| fee: MicroMinotari, | ||
| tx_type: TxType, | ||
| user_data: Vec<u8>, | ||
| ) -> Self { | ||
| PaymentId { | ||
| inner: InnerPaymentId::AddressAndData { | ||
| sender_address, | ||
| sender_one_sided, | ||
| fee, | ||
| tx_type, | ||
| user_data, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| /// Unchecked constructor for TransactionInfo payment ID | ||
| /// Use this for migration from old direct enum construction | ||
| /// WARNING: This bypasses validation - use new_transaction_info() for new code | ||
| pub fn transaction_info_unchecked( | ||
| recipient_address: TariAddress, | ||
| sender_one_sided: bool, | ||
| amount: MicroMinotari, | ||
| fee: MicroMinotari, | ||
| tx_type: TxType, | ||
| sent_output_hashes: Vec<FixedHash>, | ||
| user_data: Vec<u8>, | ||
| ) -> Self { | ||
| PaymentId { | ||
| inner: InnerPaymentId::TransactionInfo { | ||
| recipient_address, | ||
| sender_one_sided, | ||
| amount, | ||
| fee, | ||
| tx_type, | ||
| sent_output_hashes, | ||
| user_data, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add deprecation attributes to unchecked constructors
These unchecked constructors bypass validation and should be marked with #[deprecated] to discourage usage and ensure they're removed in future versions.
/// Unchecked constructor for Empty payment ID
/// Use this for migration from old direct enum construction
+#[deprecated(since = "0.1.0", note = "Use new_empty() instead")]
pub fn empty() -> Self {
/// Unchecked constructor for Open payment ID
/// Use this for migration from old direct enum construction
/// WARNING: This bypasses validation - use new_open() for new code
+#[deprecated(since = "0.1.0", note = "Use new_open() instead - this bypasses validation")]
pub fn open_unchecked(user_data: Vec<u8>, tx_type: TxType) -> Self {Apply similar deprecation attributes to the other unchecked constructors.
🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_components/payment_id.rs between
lines 1048 and 1118, the unchecked constructors bypass validation and should be
marked with #[deprecated] attributes to discourage their use and signal planned
removal. Add #[deprecated] annotations with appropriate messages to all these
unchecked constructor functions, including empty(), open_unchecked(),
raw_unchecked(), address_and_data_unchecked(), and transaction_info_unchecked(),
to clearly indicate they bypass validation and recommend using the corresponding
new_ constructors instead.
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
This looking good and in the right direction.
I do think while we are changing everything here, lets add this: #7052
Make sure we handle the errors properly and don't panic
| }; | ||
|
|
||
| // Maximum size for a PaymentID in bytes (256 bytes) | ||
| const MAX_PAYMENT_ID_SIZE: usize = 256; |
There was a problem hiding this comment.
this is a consensus constant
| InnerPaymentId::Open { user_data, tx_type } => { | ||
| match PaymentId::new_address_and_data(sender_address, fee, sender_one_sided, tx_type, user_data) { | ||
| Ok(payment_id) => payment_id, | ||
| Err(e) => panic!("Cannot create AddressAndData PaymentId: {}", e), |
There was a problem hiding this comment.
never panic, should create an error here, and propagate that back out.
|
|
||
| #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Default)] | ||
| pub enum PaymentId { | ||
| pub struct PaymentId { |
There was a problem hiding this comment.
| pub struct PaymentId { | |
| pub struct MemoField { |
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Default)] | ||
| pub enum InnerPaymentId { |
There was a problem hiding this comment.
| pub enum InnerPaymentId { | |
| enum InnerMemoField { |
Dont expose this at all
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
base_layer/core/src/transactions/transaction_components/payment_id.rs (1)
1063-1125: Add deprecation attributes to unchecked constructorsThese unchecked constructors bypass validation and should be marked with
#[deprecated]attributes as previously suggested.
🧹 Nitpick comments (2)
base_layer/core/src/transactions/transaction_components/payment_id.rs (2)
38-40: Fix typo in commentThe comment has a grammatical error.
-// We pad the bytes to min this size, so that we can use the same size for AddressAndData and TransactionInfo +// We pad the bytes to minimum this size, so that we can use the same size for AddressAndData and TransactionInfo
1127-1157: Consider documenting the default address behaviorThe
get_transaction_infomethod returnsTariAddress::default()for missing sender/recipient addresses with a comment "Will be set by caller based on context". This implicit behavior could be confusing.Consider either:
- Returning
Option<TariAddress>for addresses that need to be set by the caller- Adding clearer documentation about this behavior in the method's doc comment
- Using a more explicit placeholder or error state instead of the default address
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs(4 hunks)base_layer/core/src/transactions/transaction_components/payment_id.rs(29 hunks)base_layer/wallet/src/output_manager_service/error.rs(2 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(2 hunks)base_layer/wallet/src/output_manager_service/service.rs(22 hunks)base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs(4 hunks)base_layer/wallet/src/transaction_service/service.rs(21 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(2 hunks)base_layer/wallet_ffi/src/lib.rs(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs
- base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs
- base_layer/core/src/transactions/transaction_components/encrypted_data.rs
- base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
- base_layer/wallet/src/transaction_service/service.rs
- base_layer/wallet_ffi/src/lib.rs
- base_layer/wallet/src/output_manager_service/service.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
base_layer/wallet/src/output_manager_service/error.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/core/src/transactions/transaction_components/payment_id.rs (7)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#7189
File: integration_tests/tests/steps/wallet_cli_steps.rs:464-467
Timestamp: 2025-06-07T06:49:32.174Z
Learning: In integration test code, using panic! for setup validation and prerequisite checking is acceptable and often preferred, as it provides immediate feedback when test scenarios are incorrectly configured rather than allowing tests to continue in invalid states.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
🔇 Additional comments (4)
base_layer/core/src/transactions/transaction_components/payment_id.rs (3)
186-344: Well-designed validation implementationThe constructor methods correctly implement size validation with detailed error messages and proper Result types, avoiding panics as requested in previous reviews. The recursive size calculation for nested PaymentIds in addresses is particularly well thought out.
483-503: Proper error propagation in add_sender_addressGood change to make this method return a Result, properly propagating validation errors from the new constructors.
1996-2331: Excellent test coverage for validation logicThe test suite comprehensively covers the new validation logic including:
- Size limits for all PaymentId variants
- Recursive PaymentId validation in addresses
- Padding behavior
- Edge cases and maximum valid sizes
- Serialization round-trip testing
Well done on the thorough testing approach.
base_layer/wallet/src/output_manager_service/error.rs (1)
157-158: Appropriate error variant for PaymentId validationThe addition of
InvalidPaymentIdFormat(String)error variant properly supports the new PaymentId validation logic, allowing detailed error messages to be propagated from the validation methods.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
385-385: LGTM: Return type correctly updated to MemoField.The return type change is consistent with the refactor. Consider updating the method name in a future refactor to use "memo_field" terminology for consistency, though keeping "payment_id" may be intentional for API compatibility.
base_layer/core/src/transactions/transaction_components/memo_field.rs (4)
44-45: Document the purpose of padding constantsThe constants
PADDING_SIZEandPADDING_SIZE_NO_TAGwould benefit from documentation explaining their purpose and why these specific values (130 and 129) were chosen.-// We pad the bytes to min this size, so that we can use the same size for AddressAndData and TransactionInfo +// We pad the bytes to min this size, so that we can use the same size for AddressAndData and TransactionInfo +// This ensures consistent serialization size and helps prevent size-based information leakage const PADDING_SIZE: usize = 130; +// Size without the tag byte (used for validation during deserialization) const PADDING_SIZE_NO_TAG: usize = 129;
219-292: Consider using a proper error type instead of StringThe constructor methods return
Result<Self, String>for validation errors. Consider defining a proper error enum for better error handling and type safety.+#[derive(Debug, Clone, PartialEq)] +pub enum MemoFieldError { + SizeExceeded { + total_size: usize, + limit: usize, + details: String + }, +} + +impl std::fmt::Display for MemoFieldError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MemoFieldError::SizeExceeded { total_size, limit, details } => { + write!(f, "PaymentId exceeds {}-byte limit: {} bytes ({})", limit, total_size, details) + } + } + } +} + +impl std::error::Error for MemoFieldError {} pub fn new_address_and_data( sender_address: TariAddress, fee: MicroMinotari, sender_one_sided: bool, tx_type: TxType, payment_id: Vec<u8>, -) -> Result<Self, String> { +) -> Result<Self, MemoFieldError> { // Calculate the actual size this PaymentId would occupy (including any nested PaymentIds in the address) let total_size = Self::calculate_address_and_data_size(&sender_address, payment_id.len()); if total_size > MAX_ENCRYPTED_DATA_SIZE { - return Err(format!( - "PaymentId exceeds {}-byte limit: {} bytes (address: {} bytes, payment_id: {} bytes, overhead: {} \ - bytes)", - MAX_ENCRYPTED_DATA_SIZE, - total_size, - sender_address.get_size(), - payment_id.len(), - total_size - sender_address.get_size() - payment_id.len() - )); + return Err(MemoFieldError::SizeExceeded { + total_size, + limit: MAX_ENCRYPTED_DATA_SIZE, + details: format!( + "address: {} bytes, payment_id: {} bytes, overhead: {} bytes", + sender_address.get_size(), + payment_id.len(), + total_size - sender_address.get_size() - payment_id.len() + ), + }); }
639-651: Document the premine edge case handlingThe special handling for values < 1000 in 8-byte payloads appears to be a workaround for historical premine transactions. This should be documented more clearly.
let raw_bytes = bytes.to_vec(); -// edge case for premine: +// Special case for historical premine transactions: +// Early premine outputs used 8-byte values that should be interpreted as Open payment IDs +// rather than raw values when the value is less than 1000 if bytes.len() == SIZE_VALUE { let bytes_array: [u8; SIZE_VALUE] = bytes.try_into().expect("We already test the length"); let v = u64::from_le_bytes(bytes_array); if v < 1000 { return MemoField { inner: InnerMemoField::Open { tx_type: TxType::PaymentToOther, payment_id: bytes.to_vec(), }, }; } }
1249-1404: Consider extracting test data generation to fixturesThe
create_test_data_arrayfunction is quite long. Consider extracting common test addresses and payment IDs to test fixtures or constants for better reusability across tests.+mod test_fixtures { + use super::*; + + pub const TEST_SINGLE_ADDRESS: &str = "f3S7XTiyKQauZpDUjdR8NbcQ33MYJigiWiS44ccZCxwAAjk"; + pub const TEST_DUAL_ADDRESS: &str = "f425UWsDp714RiN53c1G6ek57rfFnotB5NCMyrn4iDgbR8i2sXVHa4xSsedd66o9KmkRgErQnyDdCaAdNLzcKrj7eUb"; + + pub fn single_address() -> TariAddress { + TariAddress::from_base58(TEST_SINGLE_ADDRESS).unwrap() + } + + pub fn dual_address() -> TariAddress { + TariAddress::from_base58(TEST_DUAL_ADDRESS).unwrap() + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
applications/minotari_console_wallet/src/automation/commands.rs(27 hunks)applications/minotari_console_wallet/src/grpc/mod.rs(3 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(23 hunks)applications/minotari_console_wallet/src/notifier/mod.rs(2 hunks)applications/minotari_console_wallet/src/ui/components/burn_tab.rs(2 hunks)applications/minotari_console_wallet/src/ui/components/send_tab.rs(2 hunks)applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(7 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(5 hunks)applications/minotari_console_wallet/src/ui/state/tasks.rs(3 hunks)applications/minotari_merge_mining_proxy/src/block_template_manager.rs(2 hunks)applications/minotari_miner/src/run_miner.rs(2 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(3 hunks)base_layer/common_types/src/tari_address/dual_address.rs(10 hunks)base_layer/common_types/src/tari_address/mod.rs(2 hunks)base_layer/contacts/src/contacts_service/storage/sqlite_db.rs(1 hunks)base_layer/core/src/blocks/pre_mine/mod.rs(2 hunks)base_layer/core/src/test_helpers/mod.rs(2 hunks)base_layer/core/src/transactions/coinbase_builder.rs(23 hunks)base_layer/core/src/transactions/test_helpers.rs(5 hunks)base_layer/core/src/transactions/transaction_components/encrypted_data.rs(5 hunks)base_layer/core/src/transactions/transaction_components/memo_field.rs(1 hunks)base_layer/core/src/transactions/transaction_components/mod.rs(1 hunks)base_layer/core/src/transactions/transaction_components/test.rs(2 hunks)base_layer/core/src/transactions/transaction_components/unblinded_output.rs(2 hunks)base_layer/core/src/transactions/transaction_components/wallet_output.rs(5 hunks)base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs(6 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(4 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(4 hunks)base_layer/core/src/transactions/transaction_protocol/proto/transaction_sender.rs(2 hunks)base_layer/core/src/transactions/transaction_protocol/recipient.rs(2 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(9 hunks)base_layer/core/src/transactions/transaction_protocol/single_receiver.rs(5 hunks)base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs(4 hunks)base_layer/core/src/validation/block_body/test.rs(2 hunks)base_layer/core/tests/tests/node_comms_interface.rs(3 hunks)base_layer/tari_mining_helper_ffi/src/lib.rs(2 hunks)base_layer/wallet/src/output_manager_service/error.rs(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(10 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(3 hunks)base_layer/wallet/src/output_manager_service/service.rs(21 hunks)base_layer/wallet/src/output_manager_service/storage/models.rs(2 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs(2 hunks)base_layer/wallet/src/transaction_service/handle.rs(23 hunks)base_layer/wallet/src/transaction_service/offline_signing/models.rs(2 hunks)base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs(3 hunks)base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs(3 hunks)base_layer/wallet/src/transaction_service/service.rs(22 hunks)base_layer/wallet/src/transaction_service/storage/database.rs(2 hunks)base_layer/wallet/src/transaction_service/storage/models.rs(11 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(24 hunks)base_layer/wallet/src/wallet.rs(7 hunks)base_layer/wallet/tests/output_manager_service_tests/service.rs(16 hunks)base_layer/wallet/tests/support/utils.rs(3 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(44 hunks)base_layer/wallet/tests/transaction_service_tests/storage.rs(12 hunks)base_layer/wallet_ffi/src/callback_handler_tests.rs(6 hunks)base_layer/wallet_ffi/src/lib.rs(20 hunks)integration_tests/src/ffi/wallet.rs(3 hunks)integration_tests/src/miner.rs(2 hunks)integration_tests/src/wallet_ffi.rs(2 hunks)integration_tests/tests/steps/wallet_ffi_steps.rs(3 hunks)integration_tests/tests/steps/wallet_steps.rs(17 hunks)
✅ Files skipped from review due to trivial changes (11)
- base_layer/wallet/src/output_manager_service/error.rs
- base_layer/wallet/src/transaction_service/storage/database.rs
- base_layer/core/src/transactions/transaction_components/mod.rs
- base_layer/wallet/src/transaction_service/offline_signing/models.rs
- base_layer/core/src/validation/block_body/test.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- base_layer/wallet/src/output_manager_service/handle.rs
- base_layer/wallet/src/transaction_service/storage/models.rs
- base_layer/core/src/transactions/transaction_components/wallet_output.rs
- base_layer/common_types/src/tari_address/dual_address.rs
- base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
🚧 Files skipped from review as they are similar to previous changes (31)
- base_layer/core/src/transactions/transaction_protocol/recipient.rs
- base_layer/wallet/tests/support/utils.rs
- applications/minotari_merge_mining_proxy/src/block_template_manager.rs
- base_layer/core/src/blocks/pre_mine/mod.rs
- base_layer/core/src/transactions/transaction_protocol/single_receiver.rs
- base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs
- base_layer/wallet/tests/transaction_service_tests/storage.rs
- base_layer/core/src/transactions/transaction_components/test.rs
- base_layer/core/tests/tests/node_comms_interface.rs
- applications/minotari_console_wallet/src/ui/components/send_tab.rs
- applications/minotari_miner/src/run_miner.rs
- base_layer/core/src/test_helpers/mod.rs
- base_layer/wallet_ffi/src/lib.rs
- base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs
- applications/minotari_console_wallet/src/ui/components/burn_tab.rs
- base_layer/tari_mining_helper_ffi/src/lib.rs
- applications/minotari_node/src/grpc/base_node_grpc_server.rs
- applications/minotari_console_wallet/src/ui/components/transactions_tab.rs
- integration_tests/src/miner.rs
- base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs
- base_layer/wallet/src/wallet.rs
- base_layer/core/src/transactions/transaction_components/encrypted_data.rs
- base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs
- base_layer/wallet/tests/output_manager_service_tests/service.rs
- base_layer/wallet/tests/transaction_service_tests/service.rs
- base_layer/core/src/transactions/transaction_protocol/sender.rs
- base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs
- base_layer/core/src/transactions/test_helpers.rs
- applications/minotari_console_wallet/src/ui/state/app_state.rs
- base_layer/core/src/transactions/coinbase_builder.rs
- base_layer/wallet/src/transaction_service/service.rs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
applications/minotari_console_wallet/src/notifier/mod.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/wallet/src/output_manager_service/storage/models.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
integration_tests/tests/steps/wallet_ffi_steps.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
applications/minotari_console_wallet/src/grpc/mod.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/contacts/src/contacts_service/storage/sqlite_db.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
base_layer/core/src/transactions/transaction_components/unblinded_output.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
integration_tests/src/ffi/wallet.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
applications/minotari_console_wallet/src/ui/state/tasks.rs (5)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
integration_tests/src/wallet_ffi.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
integration_tests/tests/steps/wallet_steps.rs (4)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
base_layer/common_types/src/tari_address/mod.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/dht/src/proto/mod.rs:141-142
Timestamp: 2025-05-02T07:12:23.985Z
Learning: The `PeerFeatures::from_bits_u32_truncate` method truncates a u32 to u8 bits but can still return `None` if the resulting bits don't match any valid flags, making the error handling with `.ok_or_else()` necessary even after truncation.
base_layer/wallet/src/output_manager_service/service.rs (5)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: comms/core/src/peer_manager/storage/database.rs:88-123
Timestamp: 2025-05-03T04:49:27.646Z
Learning: SQLite does not support the DEFAULT keyword in INSERT statements in the same way PostgreSQL does, which makes Diesel's batch insert API problematic when used with SQLite databases.
</retrieved_learning>
base_layer/core/src/transactions/transaction_components/memo_field.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7189
File: integration_tests/tests/steps/wallet_cli_steps.rs:464-467
Timestamp: 2025-06-07T06:49:32.174Z
Learning: In integration test code, using panic! for setup validation and prerequisite checking is acceptable and often preferred, as it provides immediate feedback when test scenarios are incorrectly configured rather than allowing tests to continue in invalid states.
base_layer/wallet/src/transaction_service/handle.rs (4)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: SWvheerden
PR: tari-project/tari#7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.666Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via `get_last_scanned_height()` rather than persisting it themselves. This follows separation of concerns architecture pattern.
applications/minotari_console_wallet/src/automation/commands.rs (3)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
🧬 Code Graph Analysis (3)
base_layer/wallet_ffi/src/callback_handler_tests.rs (1)
base_layer/core/src/transactions/transaction_components/memo_field.rs (1)
open_from_string(901-908)
integration_tests/tests/steps/wallet_steps.rs (1)
base_layer/core/src/transactions/transaction_components/memo_field.rs (1)
open_from_string(901-908)
base_layer/common_types/src/tari_address/mod.rs (1)
base_layer/common_types/src/tari_address/dual_address.rs (1)
get_memo_field_payment_id_bytes(129-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ledger build tests
- GitHub Check: cargo check with stable
- GitHub Check: ci
🔇 Additional comments (80)
base_layer/wallet/src/output_manager_service/storage/models.rs (1)
32-32: LGTM: Clean migration from PaymentId to MemoFieldThe import and field type changes are consistent with the broader codebase migration from
PaymentIdtoMemoField. The usage pattern remains the same (cloning fromoutput.payment_id) which indicates good backward compatibility.Also applies to: 59-59
base_layer/core/src/transactions/transaction_protocol/proto/transaction_sender.rs (1)
36-36: LGTM: Consistent PaymentId to MemoField migrationThe import and method call changes are straightforward and maintain the same conversion logic. The
from_bytesmethod signature appears to be preserved in the newMemoFieldimplementation.Also applies to: 110-110
applications/minotari_console_wallet/src/notifier/mod.rs (1)
336-336: LGTM: Updated method calls for MemoField APIThe method name change from
user_data_as_string()topayment_id_as_string()is more descriptive and aligns with the newMemoFieldAPI. The functionality remains the same - extracting the payment ID as a string for notification scripts.Also applies to: 352-352
base_layer/wallet_ffi/src/callback_handler_tests.rs (1)
52-53: LGTM: Test code updated for MemoField APIThe test code has been properly updated to use the new
MemoField::open_from_stringmethod with the requiredTxType::PaymentToOtherparameter. The choice ofPaymentToOtheris appropriate for these test transactions, and all instances are updated consistently.Also applies to: 325-325, 361-361, 381-381, 435-435, 471-471
applications/minotari_console_wallet/src/grpc/mod.rs (1)
33-33: LGTM: Updated method calls for MemoField APIThe method name change from
user_data_as_bytes()topayment_id_as_bytes()is consistent with the newMemoFieldAPI and provides clearer semantics. All three transaction types (Completed, Outbound, Inbound) are updated consistently in the gRPC event conversion.Also applies to: 44-44, 55-55
base_layer/wallet/src/output_manager_service/storage/sqlite_db/new_output_sql.rs (1)
82-82: LGTM: Method name update aligns with MemoField migration.The change from
user_data_as_bytes()topayment_id_as_bytes()is consistent with the broader migration fromPaymentIdtoMemoFieldand provides clearer API naming.integration_tests/tests/steps/wallet_ffi_steps.rs (3)
28-28: LGTM: Import updated for MemoField migration.The import change from
PaymentIdtoMemoFieldaligns with the codebase-wide migration to the new payment identifier type.
222-225: LGTM: Constructor usage updated consistently.The change from
PaymentId::open_from_stringtoMemoField::open_from_stringmaintains the same API pattern while using the new type, ensuring test compatibility with the migration.
235-238: LGTM: Consistent MemoField usage in one-sided transactions.The constructor usage follows the same pattern as the previous change, maintaining consistency across both transaction types.
base_layer/core/src/transactions/transaction_components/unblinded_output.rs (2)
41-41: LGTM: Import updated for MemoField migration.The import change reflects the migration from the
payment_idmodule to the newmemo_fieldmodule.
148-152: LGTM: Method signature updated for MemoField migration.The parameter type change from
PaymentIdtoMemoFieldis consistent with the codebase-wide migration. This represents a breaking change to the public API, which is expected for this migration.base_layer/contacts/src/contacts_service/storage/sqlite_db.rs (1)
174-178: LGTM: Method calls updated for memo field API.The changes from
get_payment_id_user_data_bytes()toget_memo_field_payment_id_bytes()andwith_payment_id_user_data()towith_memo_field_payment_id()are consistent with the new memo field API naming convention while preserving the same functionality.integration_tests/src/wallet_ffi.rs (2)
33-33: LGTM: Import updated for MemoField migration.The import change from
PaymentIdtoMemoFieldaligns with the codebase-wide type migration.
161-173: LGTM: FFI method signature updated consistently.The parameter type change from
PaymentIdtoMemoFieldin thesend_transactionmethod maintains API consistency with the underlying wallet implementation while adapting the FFI layer to the new type.base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (3)
41-41: LGTM! Import updated correctly for MemoField refactor.The import change from
PaymentIdtoMemoFieldis correct and aligns with the broader refactoring effort to enforce size validation on payment identifiers.
95-95: LGTM! Struct field type updated correctly.The field type change from
PaymentIdtoMemoFieldmaintains the same semantic meaning while adding enhanced validation capabilities. The field name remains consistent, ensuring interface compatibility.
121-121: LGTM! Constructor parameter type updated consistently.The parameter type change from
PaymentIdtoMemoFieldmaintains consistency with the struct field definition and import changes. The direct assignment on line 138 will work correctly with the new type.integration_tests/tests/steps/wallet_steps.rs (8)
59-59: LGTM: Import updated to support MemoField migration.The import statement correctly updates to use
MemoFieldandTxTypefrom the newmemo_fieldmodule, supporting the broader refactor fromPaymentIdtoMemoField.
818-827: LGTM: Consistent MemoField usage in one-sided transaction.The replacement of
PaymentId::open_from_stringwithMemoField::open_from_stringmaintains the same API signature and behavior. The transaction typeTxType::PaymentToOtherremains unchanged, ensuring semantic consistency.
884-893: LGTM: Proper MemoField integration in one-sided transaction.The migration correctly preserves the payment ID string formatting and transaction type semantics while adopting the new
MemoFieldabstraction.
997-1007: LGTM: Interactive transaction properly migrated to MemoField.The change maintains the existing message format and uses the appropriate
TxType::PaymentToOtherfor interactive transactions between wallets.
1114-1124: LGTM: Consistent MemoField usage in multi-transaction scenario.The migration properly handles the bulk transaction case with the same pattern used throughout the file.
2197-2201: LGTM: Self-transfer correctly uses TxType::PaymentToSelf.The migration appropriately uses
TxType::PaymentToSelffor transactions where the sender and receiver are the same wallet, demonstrating proper transaction type classification.
2776-2780: LGTM: Import operations properly migrated with appropriate transaction type.The UTXO import operation correctly uses
TxType::ImportedUtxoNoneRewindable, maintaining the semantic meaning while adopting the newMemoFieldstructure.
3291-3291: LGTM: Burn transaction correctly migrated to MemoField.The burn transaction appropriately uses
TxType::Burnwith a simple message, demonstrating the migration covers all transaction types in the integration tests.applications/minotari_console_wallet/src/ui/state/tasks.rs (3)
38-38: LGTM: Import updated for MemoField migration.The import correctly switches from
PaymentIdtoMemoFieldas part of the coordinated migration across the codebase.
57-57: LGTM: Function parameter type updated correctly.The function signature properly adopts
MemoFieldwhile maintaining the same functionality. The parameter is passed through to the transaction service which has been correspondingly updated.
111-111: LGTM: Function parameter type updated correctly.The burn transaction function signature properly adopts
MemoFieldwhile maintaining the same functionality. The parameter is passed through to the transaction service which has been correspondingly updated.integration_tests/src/ffi/wallet.rs (3)
34-34: LGTM: Import updated for MemoField migration.The import correctly switches from
PaymentIdtoMemoFieldas part of the coordinated migration across the codebase.
356-356: LGTM: Function parameter type updated correctly.The function signature properly adopts
MemoFieldfor the payment ID parameter, maintaining the same FFI interface functionality.
369-369: LGTM: Method call updated to match MemoField API.The method call correctly switches from
user_data_as_string()topayment_id_as_string()to match the newMemoFieldAPI, maintaining the same string conversion functionality for FFI purposes.base_layer/common_types/src/tari_address/mod.rs (3)
50-50: LGTM: Constant made public for MemoField validation.Making
MAX_ENCRYPTED_DATA_SIZEpublic is appropriate as it's likely needed by the newMemoFieldimplementation for size validation across modules.
245-247: LGTM: Method renamed for consistency with MemoField terminology.The method name correctly changes from
get_payment_id_user_data_bytestoget_memo_field_payment_id_bytesand the internal call toDualAddressis updated accordingly, maintaining the same functionality while adopting consistent terminology.
252-256: LGTM: Method renamed for consistency with MemoField terminology.The method name correctly changes from
with_payment_id_user_datatowith_memo_field_payment_idand the internal call toadd_memo_field_payment_idis updated accordingly, maintaining the same functionality while adopting consistent terminology.base_layer/core/src/transactions/transaction_key_manager/inner.rs (4)
106-106: LGTM: Import update aligns with PaymentId to MemoField migration.The import change from
PaymentIdtoMemoFieldis consistent with the comprehensive refactor described in the PR objectives.
1625-1625: LGTM: Parameter type updated for enhanced validation.The change from
PaymentIdtoMemoFieldparameter maintains interface compatibility while adding the 256-byte size validation mentioned in the PR objectives.
1650-1650: LGTM: Return type updated for consistency with migration.The return type change from
PaymentIdtoMemoFieldis consistent with the systematic migration and maintains the same semantic meaning.
1665-1665: LGTM: Tuple return type updated for migration consistency.The return type change from
PaymentIdtoMemoFieldin the tuple maintains the same interface contract while providing enhanced validation capabilities.base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (4)
41-41: LGTM: Import update consistent with migration.The import change from
PaymentIdtoMemoFieldaligns with the comprehensive refactor and is consistent with the changes in the inner implementation.
388-388: LGTM: Parameter type updated to match interface.The parameter type change from
PaymentIdtoMemoFieldmaintains consistency with the inner implementation and provides enhanced validation.
402-402: LGTM: Return type updated for interface consistency.The return type change from
PaymentIdtoMemoFieldmaintains consistency with the inner implementation and the overall migration strategy.
415-415: LGTM: Tuple return type updated for migration completion.The return type change from
PaymentIdtoMemoFieldin the tuple completes the consistent migration pattern across both the wrapper and inner implementations.base_layer/core/src/transactions/transaction_key_manager/interface.rs (3)
47-47: LGTM: Import change aligns with the MemoField refactor.The import update from PaymentId to MemoField is consistent with the broader refactor to introduce size validation for payment identifiers.
377-377: LGTM: Method signature correctly updated to use MemoField.The parameter type change from PaymentId to MemoField maintains the same semantics while adding size validation as intended by the refactor.
392-392: LGTM: Tuple return type correctly updated with MemoField.The return type change maintains the logical structure while incorporating the new MemoField type for the payment identifier component.
base_layer/wallet/src/output_manager_service/service.rs (20)
22-22: LGTM - Clean import removal.The removal of
TryIntofrom imports is appropriate as it's no longer needed with theMemoFieldrefactor.
58-58: LGTM - Required imports for MemoField refactor.The addition of
MemoFieldandTxTypeimports is necessary for the PaymentId to MemoField migration.
313-314: Excellent error handling improvement.Good to see the
.unwrap()has been replaced with proper error handling that maps toOutputManagerError::ServiceErrorwith a descriptive message. This follows the established error handling patterns.
851-858: Good error handling with relevant context.The error mapping includes size information (
"Invalid payment ID, size: {}") which is particularly relevant for this PR focused on payment ID size validation. The proper error handling replaces the previous.unwrap()usage.
1030-1030: LGTM - Type migration to MemoField.The parameter type change from
PaymentIdtoMemoFieldis consistent with the broader refactor.
1152-1152: LGTM - Consistent type migration.Another appropriate parameter type change from
PaymentIdtoMemoField.
1278-1283: Excellent API migration with proper error handling.The replacement of pattern matching on
PaymentIdvariants withMemoField::get_u64_data()accessor method is clean. The error handling includes transaction ID context which aids in debugging.
1312-1312: LGTM - Parameter type updated.The
tx_payment_idparameter type change toMemoFieldis consistent with the refactor.
1894-1901: Good use of MemoField API with proper error handling.The
add_sender_addressmethod call demonstrates the new MemoField builder-style API. The error mapping toOutputManagerError::InvalidPaymentIdFormatis appropriate for the context.
1966-1966: LGTM - Function parameter type updated.The
payment_idparameter type change toMemoFieldmaintains consistency with the refactor.
2043-2046: Appropriate MemoField constructor usage.The
MemoField::open_from_stringconstructor with transaction typePaymentToSelfis well-suited for pay-to-self transactions.
2798-2798: LGTM - Parameter type migration.The
payment_idparameter type change toMemoFieldis consistent with the overall refactor.
2807-2814: Consistent MemoField API usage.The
add_sender_addressmethod call follows the same pattern as other usages with appropriate error handling.
2882-2882: LGTM - Function parameter type updated.The
payment_idparameter type change toMemoFieldmaintains consistency.
2493-2499: Good descriptive MemoField usage.The
MemoField::open_from_stringconstructor with a descriptive message about coin splitting and appropriateTxType::CoinSplitprovides clear context for the operation.
2527-2527: Consistent coin split MemoField usage.The
MemoField::open_from_stringconstructor maintains consistency with other coin split operations.
2664-2667: Appropriate MemoField constructor for coin split.The
MemoField::open_from_stringusage for coin split operations is consistent with established patterns.
3027-3027: Appropriate MemoField for wallet scraping.The
MemoField::open_from_stringconstructor with "scraping wallet" description andTxType::PaymentToOtheris appropriate for this operation.
3124-3127: Appropriate MemoField for atomic swap claim.The
MemoField::open_from_stringconstructor with "SHA-XTR atomic swap" description andTxType::ClaimAtomicSwapis well-suited for this specialized operation.
3207-3210: Appropriate MemoField for atomic swap refund.The
MemoField::open_from_stringconstructor with "SHA-XTR atomic refund" description andTxType::HtlcAtomicSwapRefundcorrectly categorizes this refund operation.base_layer/wallet/src/transaction_service/handle.rs (4)
45-45: Import statement correctly updated for the MemoField refactor.The import of
MemoFieldis properly aligned with the type substitution throughout the file.
107-236: Enum variant type substitutions are consistent and correct.All
PaymentIdtypes have been properly replaced withMemoFieldacross theTransactionServiceRequestenum variants. The changes maintain type consistency and align with the refactor objectives.
765-1471: Method signatures consistently updated with MemoField type.All public API methods in
TransactionServiceHandlehave been properly updated to useMemoFieldinstead ofPaymentId. The changes maintain API consistency while introducing the new size validation constraints.
1-1589: Comprehensive and well-executed type refactor.The migration from
PaymentIdtoMemoFieldis thorough and consistent throughout the file. This change successfully introduces size validation constraints (256 bytes) while maintaining API compatibility and follows the architectural goals outlined in the PR objectives.base_layer/core/src/transactions/transaction_components/memo_field.rs (1)
1406-1459: Excellent security test for payment ID confusionThe
test_payment_id_parsing_confusiontest is well-designed to catch potential security issues where a craftedOpenpayment ID could be misinterpreted as a different variant during deserialization. This type of proactive security testing is commendable.applications/minotari_console_wallet/src/automation/commands.rs (10)
90-90: LGTM! Import update aligns with the refactor.The import change from
PaymentIdtoMemoFieldandTxTypeis correct and necessary for the type migration. The addition ofTxTypeindicates proper support for transaction-specific validation in the newMemoFieldAPI.
167-167: Function signature correctly updated for new type.The parameter type change from
PaymentIdtoMemoFieldmaintains the same semantic meaning while enabling the new validation capabilities as per the PR objectives.
186-186: Consistent parameter type update.The
burn_tarifunction parameter type change follows the same pattern as other functions in this refactor, maintaining consistency across the codebase.
217-217: Comprehensive parameter type updates across transaction functions.All transaction-related functions consistently update their
payment_idparameters fromPaymentIdtoMemoField. This systematic approach ensures type safety and validation consistency across the entire transaction API surface.Also applies to: 254-254, 306-306, 322-322, 339-339, 360-360, 384-384, 403-403
471-471: Method call updated to use new API.The method call change from
user_data_as_string()topayment_id_as_string()uses the newMemoFieldAPI and provides clearer semantic meaning for the operation.
497-497: Consistent method call update.The method call follows the same pattern as other updates in this file, maintaining consistency in the new API usage.
616-616: Method call consistently updated.The method call change maintains consistency with the new
MemoFieldAPI throughout the file.
763-763: Constructor calls properly updated with transaction type context.The
MemoField::open_from_stringconstructor calls correctly replacePaymentId::open_from_stringand include appropriate transaction type information. The use ofTxType::Burnfor burn transactions anddetect_tx_metadata()for address-based type detection shows proper validation context is being provided.Also applies to: 942-945, 1304-1307
1813-1813: Additional constructor calls properly updated.These
MemoField::open_from_stringcalls maintain the same pattern, usingdetect_tx_metadata()for automatic type detection and explicitTxType::CoinSplitwhere appropriate. The transaction type context enables proper validation.Also applies to: 1831-1831, 1858-1858, 1870-1870
2030-2030: Specialized transaction type categorization in constructor calls.The
MemoField::open_from_stringcalls use appropriate specialized transaction types (TxType::ClaimAtomicSwap,TxType::HtlcAtomicSwapRefund,TxType::ValidatorNodeRegistration) for different operations. This granular categorization enables proper validation specific to each transaction type.Also applies to: 2053-2053, 2073-2073, 2106-2106, 2512-2512
| // we dont know where the tari address ends and the user data starts, so we need to find it using the checksum | ||
| fn find_tari_address(bytes: &[u8]) -> Result<(TariAddress, usize), String> { | ||
| if bytes.len() < TARI_ADDRESS_INTERNAL_SINGLE_SIZE { | ||
| return Err("Not enough bytes for single TariAddress".to_string()); | ||
| } | ||
| // Now we have to try and brute force a match here | ||
| let mut offset = 0; | ||
| while (TARI_ADDRESS_INTERNAL_DUAL_SIZE + offset) <= bytes.len() { | ||
| if let Ok(address) = TariAddress::from_bytes(&bytes[..(TARI_ADDRESS_INTERNAL_DUAL_SIZE + offset)]) { | ||
| return Ok((address, TARI_ADDRESS_INTERNAL_DUAL_SIZE + offset)); | ||
| } | ||
| offset += 1; | ||
| } | ||
| if let Ok(address) = TariAddress::from_bytes(&bytes[..TARI_ADDRESS_INTERNAL_SINGLE_SIZE]) { | ||
| return Ok((address, TARI_ADDRESS_INTERNAL_SINGLE_SIZE)); | ||
| } | ||
| Err("No valid TariAddress found".to_string()) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consider optimizing the TariAddress finding algorithm
The find_tari_address method uses a brute force approach with potentially O(n) iterations. While this might be necessary for backward compatibility, consider documenting the performance implications and any alternatives that were considered.
🏁 Script executed:
#!/bin/bash
# Check if there are any performance benchmarks or tests for find_tari_address
rg -A 5 "benchmark.*find_tari_address|bench.*find_tari_address|perf.*find_tari_address" --type rustLength of output: 101
Add performance benchmarks and document implications for find_tari_address
I didn’t find any existing benchmarks or performance tests for find_tari_address. Since it uses a brute-force, O(n) approach, please:
- Document the expected performance characteristics (e.g., average/maximum byte lengths, typical iteration counts).
- Outline any alternative strategies you considered (such as indexing or early exits) and why the current approach was chosen (e.g., backward compatibility).
- (Optional but recommended) Add a simple benchmark or unit test measuring its runtime for representative inputs, so future changes can be evaluated against a baseline.
This will help readers understand the trade-offs and make future optimizations easier.
🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_components/memo_field.rs around
lines 861 to 878, the find_tari_address function lacks documentation on its
performance and no benchmarks exist. Add comments describing its O(n)
brute-force approach, typical input sizes, and iteration counts. Explain
alternative methods considered and why this approach was chosen, such as
backward compatibility. Additionally, create a benchmark or unit test to measure
runtime on representative inputs to establish a performance baseline for future
changes.
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
Looking very good, just two unchecked functions we need to ensure are fine
| } | ||
|
|
||
| /// Helper function to set the 'recipient_address' of a 'PaymentId::TransactionInfo' | ||
| pub fn transaction_info_set_address(&mut self, address: TariAddress) { |
There was a problem hiding this comment.
this can make the tx larger, this needs to check the size
| } | ||
| } | ||
|
|
||
| pub fn transaction_info_set_sent_output_hashes(&mut self, sent_output_hashes: Vec<FixedHash>) { |
There was a problem hiding this comment.
same here, this adds 32 bytes for each hash
2 participants
Description
Closes: #7096 and #7052
Renames
PaymentIdtoMemoFieldand adds constructor methods with input validation, as the total size of aMemoFieldis limited to 256 bytes.Breaking Changes
The
PaymentIdtype has been renamed toMemoField. All external applications using this type will need to update their code to reflect the new naming.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests