fix: memo naming from payment_id#7385
Conversation
- Fixed memo naming where it still referenced payment_id - Added a unit test to verify a wallet payment_id address can provide its payment_id.
WalkthroughThis update introduces trace-level logging to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletGrpcServer
participant Logger
Client->>WalletGrpcServer: get_payment_id_address(request)
WalletGrpcServer->>Logger: Trace log raw payment ID bytes and string
WalletGrpcServer->>Logger: Trace log interactive address before/after memo field
WalletGrpcServer->>Logger: Trace log one-sided address before/after memo field
WalletGrpcServer-->>Client: return address with memo field
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs (2)
108-112: Cachememo_sizeto avoid repeated calculations.The literal expression
memo.get_size()is evaluated in multiple places. Capturing it once (as shown above) improves readability and guards against accidental mismatch if the layout logic changes.-let mut data = vec![0; STATIC_ENCRYPTED_DATA_SIZE_TOTAL + memo.get_size()]; +let mut data = vec![0; STATIC_ENCRYPTED_DATA_SIZE_TOTAL + memo_size]; ... -data[SIZE_TAG + SIZE_NONCE..SIZE_TAG + SIZE_NONCE + SIZE_VALUE + SIZE_MASK + memo.get_size()] +data[SIZE_TAG + SIZE_NONCE..SIZE_TAG + SIZE_NONCE + SIZE_VALUE + SIZE_MASK + memo_size]
207-211: Consider renamingget_payment_id_size→get_memo_sizeand updating the doc‐comment.Now that “payment id” has been generalised to “memo”, keeping the old terminology here may be confusing to new consumers of the API. Unless there is a strong backward-compatibility reason, updating the method name and comment will make the public surface consistent with the rest of this PR.
No code diff supplied because this change touches the public API – please update consciously if acceptable.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
324-329: Duplicateto_base58()conversions & outdated terminology
interactive_address.to_base58()is called twice; the second call repeats the same conversion afterwith_memo_field_payment_id.
Micro-optimisation: cache the first string or useDisplayimpl.Log text still uses “id”, but the surrounding refactor moves to “memo”. Consider renaming for consistency and to avoid future confusion.
334-339: One-sided address logging suffers the same issuesSame concerns as above: sensitive memo leaks and repeated string conversions. Apply the same fixes here for consistency.
base_layer/core/src/transactions/transaction_components/memo_field.rs (3)
300-308: Error message still refers to “Memo” length limit but constant may driftThe panic-string literally prints “256-byte limit”. If
MAX_ENCRYPTED_DATA_SIZEis ever changed, the hard-coded value will become misleading.
Recommend formatting the message with the constant’s value instead of an inline literal.- "Memo exceeds {}-byte limit: {} bytes (data: {} bytes, tag: 1 byte)", + "Memo exceeds {}-byte limit: {} bytes (data: {} bytes, tag: 1 byte)",(and drop the literal 256 from all similar strings).
325-333: Public API still usespayment_idterminology
new_openreceivespayment_id: Vec<u8>while comments/doc-strings were renamed to “Memo”.
For long-term clarity consider a follow-up deprecation cycle that:
- Adds
memo: Vec<u8>synonym parameters / getters.- Marks the
payment_ididentifiers as deprecated.- Updates downstream call-sites.
Not blocking this PR but worth tracking.
788-792: Log level might be too noisy for non-fatal parsing failures
debug!("Failed to parse Memo …")will emit for every decode miss and could flood logs when peers send unknown payloads.
Consider downgrading totrace!()or gating behindlog::log_enabled!(Level::Debug)with rate-limiting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)base_layer/common_types/src/tari_address/mod.rs(1 hunks)base_layer/core/src/transactions/transaction_components/encrypted_data.rs(2 hunks)base_layer/core/src/transactions/transaction_components/memo_field.rs(25 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 Learning: the minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for t...
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.
Applied to files:
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
📚 Learning: in applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pa...
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.
Applied to files:
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rsbase_layer/core/src/transactions/transaction_components/memo_field.rs
📚 Learning: the minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for t...
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.
Applied to files:
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
📚 Learning: the minedinfo struct in the tari blockchain database contains two optional fields: `input: option
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.
Applied to files:
base_layer/core/src/transactions/transaction_components/memo_field.rs
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/core/src/transactions/transaction_components/memo_field.rs📚 Learning: in tari's one-sided transaction offline signing implementation, the script_keys vector in the script...
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.
Applied to files:
base_layer/core/src/transactions/transaction_components/memo_field.rs
📚 Learning: in the tari codebase, request validation for rpc services is handled at the service layer using the ...
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.
Applied to files:
base_layer/core/src/transactions/transaction_components/memo_field.rs
⏰ 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: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: ledger build tests
- GitHub Check: ci
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (1)
base_layer/common_types/src/tari_address/mod.rs (1)
1010-1051: Rename test variables for consistency and verify memo data– Align variable names with the new “memo_field” terminology.
– Confirm that each hardcoded base58 address decodes to aDualvariant and yields the expected memo bytes ("vgfve"and"Hello").
– Add a negative test for aSingle‐variant address to assert that:
•get_memo_field_payment_id_bytes()returns an emptyVec<u8>, and
•with_memo_field_payment_id()returns the correctTariAddressError.Apply this diff to update naming:
- let pmnt_id_address = TariAddress::from_base58( + let memo_address = TariAddress::from_base58( "f75xWw72BhjRuSatHg4MtqgqzejhZJEmHH7DyYceQDVfKdepfY22CfPJUFQyhkao28gp7cbVqVdR9zczg9eKpoYjGUBH6G32SB", ) .unwrap(); - assert_eq!(pmnt_id_address.get_memo_field_payment_id_bytes(), [ + assert_eq!(memo_address.get_memo_field_payment_id_bytes(), [ 118, 103, 102, 118, 101 ]); assert_eq!( - String::from_utf8_lossy(&pmnt_id_address.get_memo_field_payment_id_bytes()).to_string(), + String::from_utf8_lossy(&memo_address.get_memo_field_payment_id_bytes()).to_string(), "vgfve" );Please verify that the two existing addresses both decode to
Dualand carry the expected memo payloads before merging.
| debug!( | ||
| target: LOG_TARGET, | ||
| "get_payment_id_address: payment_id: '{:?}' / '{}'", | ||
| message.payment_id, String::from_utf8_lossy(&message.payment_id), | ||
| ); |
There was a problem hiding this comment.
Debug log leaks raw user data
payment_id (now a generic memo) is logged verbatim in both byte form and lossy UTF-8.
This can inadvertently expose sensitive metadata in log files and in aggregated log pipelines.
- debug!(target: LOG_TARGET, "get_payment_id_address: payment_id: '{:?}' / '{}'", …);
+ trace!(target: LOG_TARGET, "get_payment_id_address: memo length: {}", message.payment_id.len());Either drop the value entirely or hash/truncate it before printing.
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs around
lines 313 to 317, the debug log prints the raw payment_id data in both byte and
UTF-8 forms, which risks exposing sensitive user information. To fix this,
remove the direct logging of the raw payment_id or replace it with a hashed or
truncated version to protect sensitive metadata from being exposed in logs.
| memo: MemoField, | ||
| ) -> Result<EncryptedData, EncryptedDataError> { | ||
| // Encode the value and mask | ||
| let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + payment_id.get_size()]); | ||
| let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + memo.get_size()]); | ||
| bytes[..SIZE_VALUE].clone_from_slice(value.as_u64().to_le_bytes().as_ref()); | ||
| bytes[SIZE_VALUE..SIZE_VALUE + SIZE_MASK].clone_from_slice(mask.as_bytes()); | ||
| bytes[SIZE_VALUE + SIZE_MASK..].clone_from_slice(&payment_id.to_bytes()); | ||
| bytes[SIZE_VALUE + SIZE_MASK..].clone_from_slice(&memo.to_bytes()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit memo-size guard and prefer copy_from_slice over clone_from_slice.
A couple of minor but worthwhile tweaks:
-
memo.get_size()can overflowMAX_ENCRYPTED_DATA_SIZEsilently and only be caught later by theMaxSizeBytes::try_fromcall. A direct check here fails fast with a descriptive error. -
clone_from_sliceis unnecessary foru8slices –copy_from_sliceis slightly leaner and avoids theClonebound.
-let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + memo.get_size()]);
+let memo_size = memo.get_size();
+
+if memo_size > (MAX_ENCRYPTED_DATA_SIZE - STATIC_ENCRYPTED_DATA_SIZE_TOTAL) {
+ return Err(EncryptedDataError::IncorrectLength(format!(
+ "Memo length {} exceeds maximum of {} bytes",
+ memo_size,
+ MAX_ENCRYPTED_DATA_SIZE - STATIC_ENCRYPTED_DATA_SIZE_TOTAL
+ )));
+}
+
+let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + memo_size]);
-bytes[..SIZE_VALUE].clone_from_slice(value.as_u64().to_le_bytes().as_ref());
-bytes[SIZE_VALUE..SIZE_VALUE + SIZE_MASK].clone_from_slice(mask.as_bytes());
-bytes[SIZE_VALUE + SIZE_MASK..].clone_from_slice(&memo.to_bytes());
+bytes[..SIZE_VALUE].copy_from_slice(value.as_u64().to_le_bytes().as_ref());
+bytes[SIZE_VALUE..SIZE_VALUE + SIZE_MASK].copy_from_slice(mask.as_bytes());
+bytes[SIZE_VALUE + SIZE_MASK..].copy_from_slice(&memo.to_bytes());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| memo: MemoField, | |
| ) -> Result<EncryptedData, EncryptedDataError> { | |
| // Encode the value and mask | |
| let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + payment_id.get_size()]); | |
| let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + memo.get_size()]); | |
| bytes[..SIZE_VALUE].clone_from_slice(value.as_u64().to_le_bytes().as_ref()); | |
| bytes[SIZE_VALUE..SIZE_VALUE + SIZE_MASK].clone_from_slice(mask.as_bytes()); | |
| bytes[SIZE_VALUE + SIZE_MASK..].clone_from_slice(&payment_id.to_bytes()); | |
| bytes[SIZE_VALUE + SIZE_MASK..].clone_from_slice(&memo.to_bytes()); | |
| // Encode the value and mask | |
| let memo_size = memo.get_size(); | |
| if memo_size > (MAX_ENCRYPTED_DATA_SIZE - STATIC_ENCRYPTED_DATA_SIZE_TOTAL) { | |
| return Err(EncryptedDataError::IncorrectLength(format!( | |
| "Memo length {} exceeds maximum of {} bytes", | |
| memo_size, | |
| MAX_ENCRYPTED_DATA_SIZE - STATIC_ENCRYPTED_DATA_SIZE_TOTAL | |
| ))); | |
| } | |
| let mut bytes = Zeroizing::new(vec![0; SIZE_VALUE + SIZE_MASK + memo_size]); | |
| bytes[..SIZE_VALUE].copy_from_slice(value.as_u64().to_le_bytes().as_ref()); | |
| bytes[SIZE_VALUE..SIZE_VALUE + SIZE_MASK].copy_from_slice(mask.as_bytes()); | |
| bytes[SIZE_VALUE + SIZE_MASK..].copy_from_slice(&memo.to_bytes()); |
🤖 Prompt for AI Agents
In base_layer/core/src/transactions/transaction_components/encrypted_data.rs
around lines 89 to 96, add an explicit check to ensure that memo.get_size() does
not exceed MAX_ENCRYPTED_DATA_SIZE before proceeding, returning a descriptive
error if it does. Also, replace all instances of clone_from_slice used on u8
slices with copy_from_slice for better performance and to avoid unnecessary
Clone trait bounds.
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
Looks mostly correct,
but the logs must be trace, and the docs strings are wrong in a few places.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/transactions/transaction_components/memo_field.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/transactions/transaction_components/memo_field.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/transactions/transaction_components/memo_field.rs
Outdated
Show resolved
Hide resolved
hansieodendaal
force-pushed
the
ho_payment_id_not_shown
branch
from
71f616e to
22d576e
Compare
3 participants
Description
get_payment_id_address.Motivation and Context
These changes were a result of investigating issue #7363.
How Has This Been Tested?
System-level testing
What process can a PR reviewer use to test or verify this change?
Code review.
Breaking Changes
Summary by CodeRabbit
Bug Fixes
Tests
Style