feat: expose user payment id#7077
Conversation
|
Caution Review failedThe pull request is closed. ## Walkthrough
The changes introduce a structured approach for handling payment IDs by distinguishing between raw and user-defined formats in protobuf messages, gRPC server logic, and FFI interfaces. Logging verbosity is adjusted in several wallet service components, and transaction sending methods now include enhanced debug logs and earlier payment ID override logic. Additionally, contact address updates now incorporate user payment ID data when present.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------|---------------|
| applications/minotari_app_grpc/proto/wallet.proto | Replaced `bytes payment_id` with `bytes raw_payment_id` and added `UserPaymentId user_payment_id` in `PaymentRecipient`; renamed and added payment ID fields in `TransactionInfo` to distinguish raw and user formats. |
| applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs | Updated gRPC server logic to parse, validate, and handle `user_payment_id` fields, replacing direct use of raw payment ID bytes; responses now include both raw and user payment IDs. |
| base_layer/wallet/src/base_node_service/service.rs | Changed log level for handling base node service requests from `debug` to `trace`. |
| base_layer/wallet/src/connectivity_service/service.rs | Changed log level for RPC client acquisition logs from `debug` to `trace`. |
| base_layer/wallet/src/transaction_service/service.rs | Added debug logs for destination and amount in transaction sending; moved and logged payment ID override logic earlier in methods. |
| base_layer/wallet_ffi/src/lib.rs | Replaced single `payment_id` field with `raw_payment_id` and `user_payment_id` in `TariUtxo`; added FFI functions to retrieve both; renamed and updated payment ID getter function; added functions returning payment IDs as byte vectors for completed and pending transactions. |
| base_layer/wallet_ffi/wallet.h | Modified `TariUtxo` struct to have separate `raw_payment_id` and `user_payment_id` fields; renamed and added functions to get raw and user payment IDs with updated documentation. |
| base_layer/contacts/src/contacts_service/storage/sqlite_db.rs | Updated contact address upsert logic to include user payment ID data bytes in the combined address if present. |
| integration_tests/tests/steps/wallet_steps.rs | Updated test helper functions to initialize `PaymentRecipient` with `raw_payment_id` renamed and added `user_payment_id` set to `None`. |
| integration_tests/src/ffi/completed_transaction.rs | Changed FFI call to retrieve user payment ID instead of raw payment ID in completed transaction tests. |
| integration_tests/src/ffi/ffi_import.rs | Renamed FFI function for completed transaction payment ID retrieval to user payment ID variant. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant gRPC_Server
participant Wallet_Core
Client->>gRPC_Server: Send transaction with user_payment_id
gRPC_Server->>gRPC_Server: Validate user_payment_id (u256, utf8_string, user_bytes)
gRPC_Server->>Wallet_Core: Construct PaymentId (Open/Empty) and send transaction
Wallet_Core-->>gRPC_Server: TransactionInfo (raw_payment_id, user_payment_id)
gRPC_Server-->>Client: Response with raw_payment_id and user_payment_idPossibly related PRs
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
applications/minotari_app_grpc/proto/wallet.proto (1)
1139-1142: Document new payment ID fields and reserve removed tag in TransactionInfo.Add clear comments for the
raw_payment_idanduser_payment_idfields, and reserve the oldpayment_idtag (11) to maintain backward compatibility.Example diff:
diff --git a/applications/minotari_app_grpc/proto/wallet.proto b/applications/minotari_app_grpc/proto/wallet.proto --- a/applications/minotari_app_grpc/proto/wallet.proto +++ b/applications/minotari_app_grpc/proto/wallet.proto @@ message TransactionInfo { + // Reserve the old payment_id field tag (11) to prevent future reuse. + reserved 11; @@ - bytes raw_payment_id = 12; + // The raw blockchain payment ID bytes (previously payment_id). + bytes raw_payment_id = 12; @@ - bytes user_payment_id = 14; + // The payment ID provided by the user (UTF-8 string or raw bytes). + bytes user_payment_id = 14;applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
492-513: Consider refactoring duplicated payment ID validation logic.There's repeated code for extracting and validating the user payment ID in multiple places (in
send_sha_atomic_swap_transaction,transfer, and other methods). This validation logic could be refactored into a helper function to improve maintainability.+// Add this helper function to the implementation +fn extract_user_payment_id_bytes(user_pay_id: &tari_rpc::UserPaymentId) -> Result<Vec<u8>, Status> { + match ( + user_pay_id.u256.is_empty(), + user_pay_id.utf8_string.is_empty(), + user_pay_id.user_bytes.is_empty(), + ) { + (false, true, true) => Ok(user_pay_id.u256.clone()), + (true, false, true) => Ok(user_pay_id.utf8_string.as_bytes().to_vec()), + (true, true, false) => Ok(user_pay_id.user_bytes.clone()), + _ => Err(Status::invalid_argument( + "user_payment_id must be one of u256, utf8_string or user_bytes".to_string(), + )), + } +} // Then replace the repeated code blocks with calls to this functionAlso applies to: 696-717
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(6 hunks)base_layer/wallet/src/base_node_service/service.rs(1 hunks)base_layer/wallet/src/connectivity_service/service.rs(2 hunks)base_layer/wallet/src/transaction_service/service.rs(3 hunks)base_layer/wallet_ffi/src/lib.rs(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/wallet/src/transaction_service/service.rs (3)
base_layer/common_types/src/tari_address/dual_address.rs (1)
get_payment_id_user_data_bytes(129-131)base_layer/common_types/src/tari_address/mod.rs (1)
get_payment_id_user_data_bytes(242-247)base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
open(566-571)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (16)
base_layer/wallet/src/base_node_service/service.rs (1)
149-152: Reduced log verbosity by downgrading debug to trace.The log level for handling wallet base node service requests has been lowered to
traceto align with other services and reduce console noise. This change is consistent and non-functional.base_layer/wallet/src/connectivity_service/service.rs (2)
209-209: Align log level with verbosity strategy for wallet RPC acquisition.Downgraded the log level from
debugtotracefor obtaining the wallet RPC client, matching other service components.
249-249: Align log level with verbosity strategy for sync RPC acquisition.Downgraded the log level from
debugtotracefor obtaining the sync RPC client, ensuring consistent verbosity reduction.base_layer/wallet/src/transaction_service/service.rs (4)
1145-1145: Good addition of debug logging for transaction destination and amount.This log helps track transaction details at the beginning of the send process, improving visibility for debugging.
1163-1167: Improved payment ID override logic placement and debugging.Moving the payment ID override logic earlier in the method ensures the correct payment ID is used throughout the entire transaction creation process. The added debug logging helps trace payment ID changes when addresses contain the PAYMENT_ID feature.
This change aligns with the PR objective of improving clarity in how payment IDs are exposed and used, specifically allowing access to the raw user payment ID.
1778-1778: Good addition of debug logging for one-sided transaction details.Similar to the earlier logging enhancement, this helps track one-sided transaction parameters for debugging purposes.
1782-1783: Consistent debug logging for payment ID override in one-sided transactions.This debug logging matches the earlier implementation in the regular send_transaction method, providing consistent visibility when payment IDs are overridden by address features.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)
492-513: Good structured validation for user payment IDs.This new implementation correctly handles the user payment ID extraction with proper validation to ensure exactly one of the three possible fields is set. It creates a standardized approach for handling payment IDs across the codebase.
1016-1017: Improved payment ID clarity in transaction info.The code now clearly separates the raw payment ID from the user-supplied payment ID in the transaction information, providing better clarity for clients consuming this API. This change addresses the core issue mentioned in the PR objectives.
1367-1368: Good consistent implementation of split payment IDs across transaction types.The code consistently implements the separation of raw payment ID and user payment ID across all transaction types (pending inbound, pending outbound, and completed). This ensures a unified approach to payment ID handling throughout the codebase.
Also applies to: 1382-1383, 1401-1402
696-717:Details
❌ Incorrect review comment
Potential issue with hardcoded transaction type.
While the payment ID handling is implemented consistently with the
send_sha_atomic_swap_transactionmethod, I notice thatTxType::ClaimAtomicSwapis hardcoded in both places. This may not be appropriate for all transfer types.Consider making the transaction type appropriate to the context:
- tx_type: TxType::ClaimAtomicSwap, + tx_type: TxType::Transfer, // Or dynamically determine based on payment_typeVerify if
ClaimAtomicSwapis the correct type for standard transfers:
🏁 Script executed:
#!/bin/bash # Find usages of TxType in transaction code to understand appropriate values rg -A 3 "TxType::" | grep -v "use"Length of output: 81953
No change needed:
ClaimAtomicSwapis correct in this contextThis gRPC handler implements the claim atomic swap endpoint and rightly uses
TxType::ClaimAtomicSwap. There is no genericTransfervariant inTxType; standard transfers usePaymentToOtherorPaymentToSelf. Since this code mirrors the logic insend_claim_atomic_swap_transaction, no modifications are required here.Likely an incorrect or invalid review comment.
base_layer/wallet_ffi/src/lib.rs (5)
318-322: Improved distinction between payment ID typesThe separation of payment ID into
raw_payment_idanduser_payment_idfields provides clarity about which payment ID is being accessed, aligning well with the PR objectives.
352-360: Payment ID extraction and conversion implemented correctlyThe implementation properly extracts both the raw payment ID (full string representation) and the user payment ID (only user data portion) from the database wallet output.
875-910: New FFI function for raw payment ID follows existing patternsThe implementation of
tari_utxo_get_raw_payment_idfollows the same pattern as other FFI functions, with proper error handling, documentation, and safety comments.
926-926: Function rename for clarityRenaming
tari_utxo_get_payment_idtotari_utxo_get_user_payment_idmakes the API more explicit about which payment ID type is being returned.
938-938: Updated field reference in implementationThe implementation is correctly updated to access the new
user_payment_idfield, maintaining consistency with the struct changes.
Test Results (CI)1 080 tests 1 075 ✅ 15m 29s ⏱️ For more details on these failures, see this check. Results for commit 4074018. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)0 tests 0 ✅ 0s ⏱️ Results for commit 7fbfc85. ♻️ This comment has been updated with latest results. |
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Apart form the one comment below LGTM
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)base_layer/wallet_ffi/src/lib.rs(10 hunks)base_layer/wallet_ffi/wallet.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_app_grpc/proto/wallet.proto
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (13)
base_layer/wallet_ffi/wallet.h (3)
176-177: Improved API design with separate payment ID fields.The struct now clearly distinguishes between raw payment ID (with wallet-tracking data) and user-supplied payment ID, making the API more explicit and addressing the core issue outlined in the PR.
456-472: Function renamed for clarity and accuracy.The function has been properly renamed from
tari_utxo_get_payment_idtotari_utxo_get_raw_payment_idto explicitly indicate it returns the raw payment ID that includes wallet-specific tracking data. The documentation clearly explains the function's purpose and safety requirements.
474-490: Well-implemented new function for accessing user payment ID.This new function follows the existing code patterns and provides the critical functionality to access just the user-supplied payment ID without the additional tracking data added by the wallet. The documentation is comprehensive and consistent with other similar functions.
base_layer/wallet_ffi/src/lib.rs (10)
318-322: Improved payment ID handlingThe separation of payment IDs into raw and user versions is a good improvement that directly addresses the PR objective of distinguishing between wallet-appended data and user-supplied payment IDs.
352-360: LGTM: Correctly populating both payment ID fieldsThe implementation correctly handles both versions of the payment ID: raw format representing the full ID and user format showing only the user-supplied portion.
875-910: Good implementation of raw payment ID accessorThe new function follows the established FFI patterns with proper null checks, error handling, and memory management.
926-926: Clear function renamingRenaming to
tari_utxo_get_user_payment_idmakes the function's purpose explicit, which aligns well with the PR objectives.
938-938: Consistent implementationCorrectly accessing the user_payment_id field to match the renamed function.
4722-4722: Consistent naming across all transaction typesGood job maintaining consistent naming patterns across different transaction types.
4736-4736: Explicit function namingThe function name clearly indicates it's retrieving the user payment ID, improving API clarity.
4753-4754: Improved error handling with detailed messagesGood improvement to the error handling by providing more context in the error message.
5282-5283: Consistent error handling improvementGood consistency in error handling improvements across all transaction types.
5570-5571: Consistent error handling improvementGood consistency in error handling improvements for inbound transactions as well.
|
+1 This would solve my issue #7062 |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
base_layer/wallet_ffi/wallet.h (3)
168-179:⚠️ Potential issueAdding fields in the middle of
TariUtxosilently breaks the FFI ABI
raw_payment_idanduser_payment_idwere inserted beforemined_in_block.
For any consumer that was compiled against the previous layout, every field that follows these additions has now shifted, resulting in corrupted data being read/written at runtime.struct TariUtxo { const char *commitment; uint64_t value; uint64_t mined_height; uint64_t mined_timestamp; uint64_t lock_height; uint8_t status; const char *coinbase_extra; - const char *mined_in_block; + /* KEEP ALL NEW FIELDS AT THE END TO PRESERVE ABI */ + const char *mined_in_block; + const char *raw_payment_id; + const char *user_payment_id; };If re-ordering is impossible, bump the library’s major version or expose a new struct name so that downstream crates/applications can opt-in deliberately.
Failing to do so will cause subtle memory corruption and hard-to-diagnose crashes.
456-473: 💡 Verification agent🧩 Analysis chain
Documentation drift & rename fallout
- The comment block still says “payment id” instead of raw payment id, which is the whole point of the rename.
- All downstream code that called the previous
tari_utxo_get_payment_idnow fails to link. Provide an inlinestatic inlineshim or at least a#definefor one release cycle to ease migration.- Return type and safety notes are fine – thumbs-up for re-using the existing
string_destroycontract.
🏁 Script executed:
#!/bin/bash # Locate the new declaration in the header rg 'tari_utxo_get_raw_payment_id' -n base_layer/wallet_ffi/wallet.h # Search for any remaining references to the old name rg 'tari_utxo_get_payment_id' -n .Length of output: 430
Provide a temporary shim for the renamed function and correct the doc comment
Update the documentation block in
base_layer/wallet_ffi/wallet.hto consistently refer to raw payment id instead of “payment id.”Immediately after the new declaration, add a one‐cycle shim so existing callers still link:
// Deprecated alias to ease migration; will be removed in next release. static inline char *tari_utxo_get_payment_id(const struct TariUtxo *utxo, int *error_out) { return tari_utxo_get_raw_payment_id(utxo, error_out); }Or, if you prefer a macro:
#define tari_utxo_get_payment_id(...) \ tari_utxo_get_raw_payment_id(__VA_ARGS__)After shipping this, update or remove all old references in:
base_layer/wallet_ffi/src/lib.rs:12140integration_tests/src/ffi/ffi_import.rs:91Return type and safety notes are fine.
2369-2423: 💡 Verification agent🧩 Analysis chain
Inconsistent naming & incomplete migration for completed transactions
completed_transaction_user_get_payment_idexposes the user variant – good.completed_transaction_get_payment_id_as_bytesexposes the raw variant as bytes, but there is no string version (completed_transaction_get_raw_payment_id), and the oldcompleted_transaction_get_payment_iddeclaration has been removed.Recommendation:
+char *completed_transaction_get_raw_payment_id(TariCompletedTransaction *transaction, + int *error_out);Deprecate the former single-meaning name with a
#defineto maintain source compatibility for one release.
🏁 Script executed:
#!/bin/bash # List all payment ID related functions in the wallet FFI header rg -n 'completed_transaction_.*payment_id' base_layer/wallet_ffi/wallet.h || trueLength of output: 390
Add missing raw payment ID string variant
The FFI header currently provides:
completed_transaction_user_get_payment_id(string)completed_transaction_user_get_payment_id_as_bytes(bytes)completed_transaction_get_payment_id_as_bytes(bytes)but lacks a raw string variant. To complete the migration and maintain backwards-compatibility, add:
--- a/base_layer/wallet_ffi/wallet.h +++ b/base_layer/wallet_ffi/wallet.h @@ /* near existing payment ID methods */ +/** + * Gets the raw payment ID of a TariCompletedTransaction in string format + * + * ## Arguments + * `transaction` - Pointer to a TariCompletedTransaction + * `error_out` - Out parameter for error codes (must not be null) + * + * ## Returns + * `*const c_char` - Pointer to the char array; empty string if `transaction` is null + * + * # Safety + * Call `string_destroy` on the returned value to avoid memory leaks. + */ +char *completed_transaction_get_raw_payment_id(TariCompletedTransaction *transaction, + int *error_out);Then, for one release, alias the old name to this new function:
#define completed_transaction_get_payment_id completed_transaction_get_raw_payment_id—after which the old name can be formally deprecated and removed.
🧹 Nitpick comments (3)
base_layer/contacts/src/contacts_service/storage/sqlite_db.rs (2)
174-178: Consider adding debug logging for payment ID operationsWhile the implementation is correct, adding debug logging when applying payment ID user data or encountering errors would make troubleshooting easier in production environments.
if !k.get_payment_id_user_data_bytes().is_empty() { + log::debug!(target: LOG_TARGET, "Applying user payment ID data to contact address"); new_address = new_address .with_payment_id_user_data(k.get_payment_id_user_data_bytes()) - .map_err(|_| ContactsServiceStorageError::ConversionError)?; + .map_err(|e| { + log::debug!(target: LOG_TARGET, "Failed to apply user payment ID data: {}", e); + ContactsServiceStorageError::ConversionError + })?; }
234-303: Add test coverage for payment ID user data handlingThe current test suite lacks coverage for the new payment ID user data handling functionality. Consider adding a test case that verifies the correct behavior when an address with payment ID user data is used in a contact update.
This could be implemented by extending the existing
test_crudtest or adding a new test function specifically for this feature.base_layer/wallet_ffi/wallet.h (1)
2839-2875: Pending inbound: same asymmetry & possible copy-paste errorSame issue as outbound plus a small wording bug in the doc-block (“elements in the byte_vector don't match element_count” is irrelevant – no element count parameter exists).
Action items:
- Add missing string accessor
pending_inbound_transaction_user_get_payment_id.- Clean up the doc-block text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/contacts/src/contacts_service/storage/sqlite_db.rs(1 hunks)base_layer/wallet_ffi/wallet.h(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/contacts/src/contacts_service/storage/sqlite_db.rs (1)
base_layer/common_types/src/tari_address/mod.rs (1)
combine_addresses(191-226)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with stable
🔇 Additional comments (2)
base_layer/contacts/src/contacts_service/storage/sqlite_db.rs (1)
172-179: Good implementation of user payment ID handling in contact address updatesThis change correctly implements the user payment ID handling when updating contact addresses. After combining the stored address with a new address key, it explicitly adds the user payment ID data bytes to the combined address if they exist in the new address key. This aligns with the PR objective of properly exposing user payment IDs throughout the system.
base_layer/wallet_ffi/wallet.h (1)
456-491:Details
✅ Verification successful
Verify that all new symbols are exported from the Rust FFI crate
Header changes alone are not sufficient – the Rust
#[no_mangle] extern "C"functions must be added with identical names.
Run the following script to grep for missing implementations:Expect each name to be found at least once; missing names indicate a linker error waiting to happen.
Also applies to: 2369-2423, 2670-2706, 2839-2875
🏁 Script executed:
#!/bin/bash # Verify every new *_payment_id symbol in wallet.h exists in the Rust source rg -o $'(tari_utxo_get_raw_payment_id|tari_utxo_get_user_payment_id|completed_transaction_user_get_payment_id|completed_transaction_user_get_payment_id_as_bytes|completed_transaction_get_payment_id_as_bytes|pending_outbound_transaction_user_get_payment_id_as_bytes|pending_outbound_transaction_get_payment_id_as_bytes|pending_inbound_transaction_user_get_payment_id_as_bytes|pending_inbound_transaction_get_payment_id_as_bytes)' base_layer/wallet_ffi/src | sort -uLength of output: 1096
All new FFI symbols verified
Ran the provided grep againstbase_layer/wallet_ffi/src/lib.rsand confirmed that every*_payment_idfunction declared inwallet.his implemented with the correct#[no_mangle] extern "C"signature:
- tari_utxo_get_raw_payment_id
- tari_utxo_get_user_payment_id
- completed_transaction_user_get_payment_id
- completed_transaction_get_payment_id_as_bytes
- pending_outbound_transaction_user_get_payment_id_as_bytes
- pending_outbound_transaction_get_payment_id_as_bytes
- pending_inbound_transaction_user_get_payment_id_as_bytes
- pending_inbound_transaction_get_payment_id_as_bytes
No missing exports detected. Ready to merge.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
base_layer/wallet_ffi/wallet.h (2)
474-491:⚠️ Potential issueMissing byte-vector accessor for user payment ID
While you've added
tari_utxo_get_user_payment_idfor string access, there's no correspondingtari_utxo_get_user_payment_id_as_bytesfunction. This creates inconsistency with other payment ID accessors that provide both string and byte-vector variants.To maintain API consistency with other payment ID accessors, add a new function:
char *tari_utxo_get_user_payment_id_as_bytes(const struct TariUtxo *utxo, int *error_out);
2713-2749:⚠️ Potential issueMissing string accessor for user payment ID in pending outbound transactions
You've added byte vector accessors for both raw and user payment IDs, but there's no string accessor for the user payment ID. This creates inconsistency with the completed transaction API which offers both variants.
For API consistency, add a string accessor function:
char *pending_outbound_transaction_user_get_payment_id(TariPendingOutboundTransaction *transaction, int *error_out);base_layer/wallet_ffi/src/lib.rs (3)
4837-4846: Verify transaction pointer null check orderThe transaction pointer should be checked for null before being dereferenced, but the check occurs after dereferencing in the original code. Based on previous review comments, this pattern has caused issues elsewhere and should be fixed here too.
- let payment_id = (*transaction).payment_id.clone(); - if transaction.is_null() { + if transaction.is_null() { + *error_out = LibWalletError::from(InterfaceError::NullError("transaction".to_string())).code; + return ptr::null_mut(); + } + let payment_id = (*transaction).payment_id.clone();#!/bin/bash # Check for similar patterns in the file where dereferencing occurs before null check rg -A 5 "let payment_id = \(\*transaction\).payment_id.clone();" base_layer/wallet_ffi/src/lib.rs
5366-5375: Verify transaction pointer null check orderSimilar to the previous issue, the transaction pointer should be checked for null before being dereferenced.
5653-5663: Verify transaction pointer null check orderSimilar to previous instances, check for transaction null before dereferencing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(7 hunks)base_layer/wallet_ffi/src/lib.rs(16 hunks)base_layer/wallet_ffi/wallet.h(9 hunks)integration_tests/tests/steps/wallet_steps.rs(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- applications/minotari_app_grpc/proto/wallet.proto
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: cargo check with stable
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (26)
integration_tests/tests/steps/wallet_steps.rs (9)
715-730: Field structure updated to support explicit user payment ID.The code correctly updates the
PaymentRecipientstruct usage by:
- Renaming
payment_idtoraw_payment_id- Adding the new
user_payment_id: NonefieldThis change aligns with the PR objective to distinguish between raw and user-defined payment IDs.
785-795: Field structure updated consistently for one-sided transactions.The payment ID field structure has been properly updated in the one-sided transaction code path, maintaining consistency with the updated protobuf definitions.
888-899: Payment ID field structure updated for standard transfers.The code correctly implements the new payment ID structure in the standard transfer workflow, maintaining functional parity while adding the capability to track user payment IDs separately.
1335-1345: Payment ID structure updated for multi-transaction implementation.The changes maintain consistent payment ID handling across different transaction types by applying the same field structure updates to the multi-transaction implementation.
1681-1691: Payment ID fields updated for multi-recipient transactions (first recipient).The code properly updates the payment ID field structure for the first recipient in multi-recipient transactions, ensuring consistent handling of payment IDs across different transaction patterns.
1699-1710: Payment ID fields updated for multi-recipient transactions (second recipient).The code correctly applies the same payment ID field structure update to the second recipient, maintaining consistency in how payment IDs are handled throughout the codebase.
1820-1825: Payment ID structure updated for self-transfer transactions.The changes properly implement the new payment ID field structure for self-transfer transactions, using the
PaymentToSelftransaction type while maintaining consistent field naming.
1904-1915: Payment ID structure updated for HTLC/atomic swap transactions.The changes correctly update the payment ID field structure for atomic swap transactions, ensuring that the payment ID handling is consistent across all transaction types.
115-119:Details
❌ Incorrect review comment
Verify request field compatibility for
payment_id: Noneusage.I notice that several request structs still use
payment_id: Nonerather than the new field names. This appears intentional as these are likely different field definitions in different request types than thePaymentRecipientstruct.Also applies to: 501-504, 553-556, 642-642, 1037-1041, 1576-1578, 2146-2150
🏁 Script executed:
#!/bin/bash # Check if these request types still use payment_id field (not raw_payment_id) rg -A 2 -B 2 "struct (GetBalanceRequest|GetCompletedTransactionsRequest)" --glob "*.proto"Length of output: 92
🏁 Script executed:
#!/bin/bash # List all .proto files in the repository find . -type f -name '*.proto' # Inspect .proto message definitions for request types rg -A5 -B5 "message GetBalanceRequest" --glob '*.proto' rg -A5 -B5 "message GetCompletedTransactionsRequest" --glob '*.proto' # Fallback: inspect generated Rust struct definitions for these requests rg -A5 -B5 "struct GetBalanceRequest" --glob '*.rs' rg -A5 -B5 "struct GetCompletedTransactionsRequest" --glob '*.rs' # Search for field names in Rust code to check for payment_id vs raw_payment_id rg -n "payment_id" --glob '*.rs' rg -n "raw_payment_id" --glob '*.rs'Length of output: 66164
Incorrect assumption about
payment_idin GRPC requestsThe
GetBalanceRequestandGetCompletedTransactionsRequestmessages in
applications/minotari_app_grpc/proto/wallet.protostill defineUserPaymentId payment_id = 1;so using
payment_id: Nonein the Rust client tests is correct. The renames toraw_payment_idanduser_payment_idapply only to the FFI/CLI layers, not these protobuf definitions.Likely an incorrect or invalid review comment.
base_layer/wallet_ffi/wallet.h (4)
176-177: Approve changes to TariUtxo structThe split of the payment_id field into raw_payment_id and user_payment_id aligns with the PR objective of distinguishing between wallet-appended data and user-supplied payment IDs.
456-472: Renamed function correctly indicates it returns raw payment IDRenaming
tari_utxo_get_payment_idtotari_utxo_get_raw_payment_idclarifies the type of payment ID returned and maintains backward API compatibility by keeping the function's overall behavior.
793-834: Approve new address creation functions with payment ID supportThese new functions provide a clean way to create addresses with payment IDs attached, supporting both byte vector and UTF-8 string formats. The documentation clearly explains the parameters and return values.
2411-2466: Approve completed transaction payment ID accessorsThe implementation properly provides both string and byte vector accessors for both raw and user payment IDs, with clear documentation.
base_layer/wallet_ffi/src/lib.rs (13)
318-322: Good implementation of dual payment ID structureThe TariUtxo struct now clearly distinguishes between raw and user payment IDs with separate fields, which directly addresses the PR objective to expose the user payment ID.
352-360: Clean implementation of the payment ID initializationThe initialization handles both payment ID types appropriately, with the raw version using the full string representation and the user version extracting just the user data portion.
875-909: Well-implemented raw payment ID accessorThe tari_utxo_get_raw_payment_id function correctly handles error conditions and safely retrieves the raw payment ID.
912-941: Well-implemented user payment ID accessorThe tari_utxo_get_user_payment_id function properly handles null checks and error cases.
4848-4881: Well-implemented user payment ID as bytes accessorThe function has proper null checks and error handling for accessing the user payment ID as bytes.
4883-4916: Well-implemented raw payment ID as bytes accessorThe function properly handles null checks and error cases when retrieving the raw payment ID as bytes.
5377-5410: Well-implemented outbound transaction user payment ID as bytes accessorThe function properly validates input parameters and has appropriate error handling.
5412-5445: Well-implemented outbound transaction raw payment ID as bytes accessorThe function has proper null checks and error handling for accessing the raw payment ID.
5665-5698: Well-implemented inbound transaction user payment ID as bytes accessorThe function properly handles null checks and error cases.
5700-5733: Well-implemented inbound transaction raw payment ID as bytes accessorThe function properly validates input parameters and has appropriate error handling.
12227-12232: Test correctly updated to use new payment ID APIThe test has been properly adapted to use the new
tari_utxo_get_raw_payment_idfunction and check against theraw_payment_idfield.
12833-12835: Test correctly updated to use raw_payment_id fieldThe test has been updated to reference the new field name.
13112-13113: Test correctly updated to use raw_payment_id fieldSimilar to the previous test, this one has been properly updated to reference the new field name.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
integration_tests/src/ffi/completed_transaction.rs (1)
135-146: 💡 Verification agent🧩 Analysis chain
Method name should be updated to match the new FFI function
The method is still named
get_payment_id()but now callscompleted_transaction_get_user_payment_id(). This creates an inconsistency between the method name and its implementation that could confuse developers.- pub fn get_payment_id(&self) -> String { + pub fn get_user_payment_id(&self) -> String { let ptr; let mut error = 0; unsafe { ptr = ffi_import::completed_transaction_get_user_payment_id(self.ptr, &mut error); if error > 0 { - println!("completed_transaction_get_payment_id error {}", error); - panic!("completed_transaction_get_payment_id error"); + println!("completed_transaction_get_user_payment_id error {}", error); + panic!("completed_transaction_get_user_payment_id error"); } } FFIString::from_ptr(ptr).as_string() }Additionally, consider keeping both methods if backward compatibility is important:
- Keep
get_payment_id()but mark it as deprecated- Add the new
get_user_payment_id()methodLet's check for references to
get_payment_id()in the codebase:
🏁 Script executed:
#!/bin/bash # Look for references to the get_payment_id method to evaluate impact rg "get_payment_id\(\)" --type rustLength of output: 152
Rename get_payment_id to get_user_payment_id for consistency
Update the method signature, its error messages, and the single test reference to match the underlying FFI call:
- integration_tests/src/ffi/completed_transaction.rs
– Changepub fn get_payment_id→pub fn get_user_payment_id
– Update theprintln!andpanic!messages accordingly- integration_tests/tests/steps/wallet_ffi_steps.rs
– Change the call from.get_payment_id()→.get_user_payment_id()Diff:
--- a/integration_tests/src/ffi/completed_transaction.rs +++ b/integration_tests/src/ffi/completed_transaction.rs @@ -135,7 +135,7 @@ impl CompletedTransaction { - pub fn get_payment_id(&self) -> String { + pub fn get_user_payment_id(&self) -> String { let ptr; let mut error = 0; unsafe { ptr = ffi_import::completed_transaction_get_user_payment_id(self.ptr, &mut error); if error > 0 { - println!("completed_transaction_get_payment_id error {}", error); - panic!("completed_transaction_get_payment_id error"); + println!("completed_transaction_get_user_payment_id error {}", error); + panic!("completed_transaction_get_user_payment_id error"); } } FFIString::from_ptr(ptr).as_string() }--- a/integration_tests/tests/steps/wallet_ffi_steps.rs +++ b/integration_tests/tests/steps/wallet_ffi_steps.rs @@ -20,7 +20,7 @@ fn some_wallet_step() { - let payment_id = completed_transaction.get_payment_id(); + let payment_id = completed_transaction.get_user_payment_id();If you need to preserve backward compatibility, you can keep
get_payment_id()as a deprecated alias:#[deprecated(note = "Use `get_user_payment_id` instead")] pub fn get_payment_id(&self) -> String { self.get_user_payment_id() }
🧹 Nitpick comments (2)
integration_tests/src/ffi/completed_transaction.rs (2)
135-146: Error messages still reference the old function nameThe error messages still reference
completed_transaction_get_payment_idbut the code now callscompleted_transaction_get_user_payment_id. This inconsistency could make debugging more difficult.unsafe { ptr = ffi_import::completed_transaction_get_user_payment_id(self.ptr, &mut error); if error > 0 { - println!("completed_transaction_get_payment_id error {}", error); - panic!("completed_transaction_get_payment_id error"); + println!("completed_transaction_get_user_payment_id error {}", error); + panic!("completed_transaction_get_user_payment_id error"); } }
135-146: Consider adding documentation to explain payment ID typesThe PR introduces a distinction between raw payment IDs and user payment IDs. Adding documentation to this method would help developers understand this difference.
+ /// Returns the user-supplied payment ID without any wallet-specific additions. + /// This differs from the raw payment ID which may contain extra data appended by the wallet + /// to track its own transactions on the blockchain. pub fn get_payment_id(&self) -> String { let ptr; let mut error = 0; unsafe { ptr = ffi_import::completed_transaction_get_user_payment_id(self.ptr, &mut error); if error > 0 { println!("completed_transaction_get_payment_id error {}", error); panic!("completed_transaction_get_payment_id error"); } } FFIString::from_ptr(ptr).as_string() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration_tests/src/ffi/completed_transaction.rs(1 hunks)integration_tests/src/ffi/ffi_import.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration_tests/src/ffi/ffi_import.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
integration_tests/src/ffi/completed_transaction.rs (1)
integration_tests/src/ffi/ffi_import.rs (1)
completed_transaction_get_user_payment_id(307-310)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
base_layer/wallet_ffi/wallet.h (3)
489-490:⚠️ Potential issueMissing byte-vector accessor for user payment ID.
While you've added a string accessor for the user payment ID, there's no corresponding byte-vector accessor function (
tari_utxo_get_user_payment_id_as_bytes). This creates inconsistency with other payment ID accessors that have both string and byte-vector versions.For API consistency, add:
struct ByteVector *tari_utxo_get_user_payment_id_as_bytes(const struct TariUtxo *utxo, int *error_out);
2729-2730:⚠️ Potential issueMissing string accessor for user payment ID in pending outbound transactions.
While you've added a byte-vector accessor for the user payment ID, there's no corresponding string accessor function for pending outbound transactions. This creates inconsistency with the completed transaction API which has both.
For API consistency, add:
char *pending_outbound_transaction_user_get_payment_id(TariPendingOutboundTransaction *transaction, int *error_out);
2898-2899:⚠️ Potential issueMissing string accessor for user payment ID in pending inbound transactions.
Similar to the pending outbound transactions, you've added a byte-vector accessor but no string accessor for the user payment ID, creating inconsistency with the completed transaction API.
For API consistency, add:
char *pending_inbound_transaction_user_get_payment_id(TariPendingInboundTransaction *transaction, int *error_out);
🧹 Nitpick comments (3)
base_layer/wallet_ffi/src/lib.rs (3)
355-360: Potentially ambiguous serialisation ofPaymentId
raw_payment_idis produced withCString::new(format!("{}", x.payment_id))?
DisplayforPaymentIdmay yield hex, base-58, or debug text depending on upstream impls, whereas consumers will assume a specific canonical form.
Please confirm that:
format!("{}", x.payment_id)is guaranteed to be stable and machine-parsable across versions, or- Replace it with an explicit helper (
to_hex(),to_base58(), etc.) so that downstream C/FFI users know what to expect.
1500-1527: Minor API contract clarification
tari_address_create_with_payment_id_bytesconsumes the originaladdresspointer but does not free it, instead returning a cloned address containing the new payment ID.
Make sure the documentation clearly states that the caller must still destroy the originalTariWalletAddresswhen it is no longer needed; otherwise double frees or leaks may arise from incorrect assumptions.
4850-4894: Consider DRYing the repeated payment-ID marshaling logicAll three transaction types (
Completed,PendingOutbound,PendingInbound) now duplicate almost identical code paths for:
- user-payment-id →
CString- user-payment-id →
ByteVector- raw-payment-id →
ByteVectorA small helper (or even a macro) that converts a
&PaymentIdinto the desired representation would:
- Remove ~150 lines of boilerplate.
- Ensure future fixes (e.g. encoding changes) are applied in one place only.
Nice-to-have, but it will pay off quickly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/wallet_ffi/src/lib.rs(16 hunks)base_layer/wallet_ffi/wallet.h(9 hunks)
🔇 Additional comments (9)
base_layer/wallet_ffi/wallet.h (8)
176-177: Payment ID field separation is a valuable improvement.The separation of payment IDs into
raw_payment_idanduser_payment_idin theTariUtxostruct provides better clarity on the different payment ID types. This aligns with the PR objectives of ensuring all gRPC and FFI calls properly expose the user-supplied payment ID.
471-472: Function naming follows the new field structure.The function has been properly renamed from
tari_utxo_get_payment_idtotari_utxo_get_raw_payment_idto match the field name change.
810-813: New function to create address with payment ID bytes.This addition provides a clean way to create a TariWalletAddress with a payment ID as bytes, which is a helpful enhancement. The function follows the pattern of other factory methods in the codebase.
831-834: New function to create address with payment ID as UTF-8 string.This overload provides a convenient way to create a TariWalletAddress with a payment ID from a UTF-8 string, which increases API usability. The pattern is consistent with other factory methods.
2426-2427: Added user payment ID string accessor for completed transactions.This function provides a string representation of the user payment ID for completed transactions, which aligns with the goal of explicitly exposing user payment IDs.
2445-2446: Added byte-vector accessor for user payment ID in completed transactions.This function provides access to the user payment ID as bytes for completed transactions, maintaining API consistency by offering both string and byte-vector accessors.
2464-2465: Added byte-vector accessor for raw payment ID in completed transactions.This function provides access to the raw payment ID as bytes for completed transactions, completing the set of accessors needed for payment IDs.
2917-2918: Added byte-vector accessor for raw payment ID in pending inbound transactions.This function provides access to the raw payment ID as bytes for pending inbound transactions, which is consistent with the pattern established for other transaction types.
base_layer/wallet_ffi/src/lib.rs (1)
321-323: Guarantee proper ownership & deallocation for the two newCStringfields
raw_payment_idanduser_payment_idare stored as raw C-string pointers returned byCString::into_raw().
Please double-check that:
TariUtxo(or whichever wrapper frees it) now destructs all three string fields (coinbase_extra,raw_payment_id,user_payment_id) to avoid heap leaks.- Every FFI helper that clones a
TariUtxo(e.g. container helpers returning arrays) also replicates this freeing logic.If the drop/destructor logic was not updated, consider adding it or extending
tari_utxo_destroyaccordingly.
4 participants
Description
Currently, the wallet adds extra data to the payment id field to enable it to track its own transactions on the blockchain.
This however leads to weird results when trying to display the raw user payment id. This ensures that all grpc and ffi calls either straight up just provides the user provided payment id, or explicitly states which one it is, and provide both.
Summary by CodeRabbit