feat!: ensure payref persists during recovery#7225
feat!: ensure payref persists during recovery#7225SWvheerden merged 5 commits intotari-project:developmentfrom
Conversation
add payref support in payment id
WalkthroughThis change moves the Changes
Sequence Diagram(s)sequenceDiagram
participant Wallet as Wallet Service
participant OutputMgr as Output Manager Service
participant DB as Output Manager DB
Wallet->>OutputMgr: confirm_pending_transaction(tx_id, Some(change_outputs))
OutputMgr->>OutputMgr: finalize transaction, get finalized change outputs
OutputMgr->>DB: confirm_encumbered_outputs(tx_id, change_outputs)
DB->>DB: update outputs, set encrypted data and metadata signature
DB-->>OutputMgr: confirmation result
OutputMgr-->>Wallet: confirmation result
sequenceDiagram
participant Protocol as SenderTransactionProtocol
participant KeyMgr as KeyManager
Protocol->>KeyMgr: build_transaction(info)
KeyMgr-->>Protocol: (transaction, Option<change_output>)
Protocol->>Protocol: store FinalizedTransaction { final_transaction, change_output }
Protocol->>Protocol: get_finalized_change_output()
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
85-97: Cap the size ofpayment_idbefore encryption
encrypt_dataallocatesSIZE_VALUE + SIZE_MASK + payment_id.get_size()without validating thatget_size()is within a reasonable bound.
BecausePaymentIdis externally supplied, a malicious caller could craft a huge payment-id, resulting in an oversized ciphertext and unnecessary memory/database growth.Verify the maximum length expected for on-chain metadata (e.g. legacy
TariAddresssize ≤ 65 bytes) and reject anything above that before allocation.[security]
base_layer/wallet/src/transaction_service/storage/models.rs (1)
326-368: Preserve the real source address when derivingCompletedTransaction
from_outboundsetssource_address: Default::default(),which later bubbles up via
WalletTransaction::source_address().
Down-stream UI/reporting will mis-attribute the transaction (empty address /T::default()).If the wallet cannot determine the sender’s address yet, consider:
- source_address: Default::default(), + source_address: tx.sender_protocol.get_source_address_or_default(),or pass an
Option<TariAddress>to keep the information explicit.
♻️ Duplicate comments (2)
base_layer/wallet/tests/output_manager_service_tests/storage.rs (2)
467-468: Same minor allocation point as aboveThe comment on lines 182-184 applies here as well.
617-618: Same minor allocation point as aboveThe comment on lines 182-184 applies here as well.
🧹 Nitpick comments (21)
base_layer/wallet/migrations/2025-06-17-120000_remove_change_hashes/up.sql (1)
5-5: Ensure reversible migration
You may want to add a correspondingdown.sqlscript to restore thechange_output_hashescolumn for rollback support.base_layer/core/tests/tests/node_comms_interface.rs (1)
46-46:PaymentIdis imported but never used
PaymentIdis not referenced anywhere in this test file, which will raise anunused_importswarning (or an error if the CI uses#![deny(unused_imports)]).
Consider dropping the import:- payment_id::PaymentId, + /* PaymentId no longer required */base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs (1)
433-434:sent_output_hashesis initialised but never populatedA new
sent_output_hashes: Vec::new()field is added to thePaymentId::TransactionInfoinstance, yet the vector is not subsequently updated in this builder.
If the intention is to persist the hashes of the outputs once they are known, ensure they are pushed later in the transaction-building flow; otherwise the field will always remain empty and provide no value.If the information will be added elsewhere, please confirm; if not, consider removing the field or wiring it up here.
base_layer/core/src/transactions/transaction_components/mod.rs (1)
61-61: Publicly exposing the newpayment_idmoduleDeclaring
pub mod payment_id;is necessary, but you might also want to re-export the key types to preserve the previous ergonomic path:pub mod payment_id; +pub use payment_id::{PaymentId, TxType};That way downstream code can continue to do
use transaction_components::PaymentIdif desired.base_layer/wallet/tests/utxo_scanner/mod.rs (1)
861-863: Add rationale for ignoring the one-sided payments testPlease include a comment explaining why
test_utxo_scanner_one_sided_paymentsis marked#[ignore], and add aTODOor issue reference to re-enable it once one-sided payment reference persistence is fully supported.Would you like help drafting the ignore rationale or linking this to an issue?
base_layer/wallet/src/transaction_service/storage/database.rs (2)
748-756: Minor: make the intent explicit when extracting fee and hashesUsing
unwrap_or_default()is fine, but it silently treats a missing field as zero fee / empty sent-hash list.
Explicit fall-back makes the behaviour clearer and prevents accidental masking of malformedPaymentIds:-let fee = payment_id.get_fee().unwrap_or_default(); -let sent_hashes = payment_id.get_sent_hashes().unwrap_or_default(); +let fee = payment_id + .get_fee() + .unwrap_or_else(|| { + debug!(target: LOG_TARGET, "No fee embedded in PaymentId – defaulting to 0"); + MicroMinotari::from(0) + }); + +let sent_hashes = payment_id + .get_sent_hashes() + .unwrap_or_else(|| { + debug!(target: LOG_TARGET, "No sent hashes in PaymentId – using empty vec"); + Vec::new() + });Not critical, but it improves debuggability.
769-770: Avoid double allocation ofsent_hashesYou already have the vector in
sent_hashes; re-using it avoids a second allocation:- sent_hashes, - vec![hash], + sent_hashes, // provided by PaymentId + vec![hash], // received hash (imported output)base_layer/wallet/tests/output_manager_service_tests/storage.rs (1)
182-184: Avoid per-iteration allocation of an empty vector
Vec::new()creates a new allocation every time the loop runs.
Although this is only a test and the cost is negligible, passing a static empty slice (if the API permits) or reusing a singleconst EMPTY: Vec<DbWalletOutput> = Vec::new();would be marginally more efficient and makes the intention explicit.base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
2535-2536: Minor: keep test-only imports shallowThe deep path
transaction_components::payment_id::{PaymentId, TxType}works, but pulling from the crate’s public re-export (if one exists) keeps the test decoupled from internal layout shifts.No action required if a public re-export is not (yet) available.
base_layer/wallet/src/output_manager_service/storage/database/backend.rs (1)
67-74: Doc-comment out of date with new parameterThe sentence preceding this method still reads “This method confirms that a transaction negotiation is complete and outputs can be fully encumbered.”
Now that we also update change-output metadata, it would be clearer to mention that the supplied slice contains the finalized change outputs that must have their encrypted data / metadata updated./// This method confirms that a transaction negotiation is complete, fully encumbers the outputs /// against the pending transaction, and updates the encrypted data/metadata of the provided /// finalized change outputs.Minor but helps future maintainers.
base_layer/wallet_ffi/src/lib.rs (1)
12818-12822: Test data aligned with new struct, but consider covering non-empty casesThe addition of
sent_output_hashes: vec![]satisfies the new field, yet the test only exercises the empty path. Adding at least one hash would give stronger coverage of the serialization path introduced in this PR.base_layer/wallet/src/transaction_service/storage/models.rs (1)
449-457:source_address()may returnNonefor outbound tx – clarify intentReturning
NoneforPendingOutboundwhile alwaysSomefor the other variants makes the API asymmetric.
Consider documenting the behaviour (“Nonewhen the wallet itself is the sender”) or return the wallet’s own address instead to avoidOptionjuggling by callers.base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs (2)
855-873: Comment/code mismatch – ensure we update only necessary fieldsThe comment states “Only the
ephemeral_pubkeyandu_yportion needs to be updated”, yet theUpdateOutputincludes five metadata fields plusencrypted_dataandhash.Either:
- Expand the comment to reflect reality, or
- Trim the update to the minimal required subset to prevent unintended overwrites.
Consistency here avoids future maintenance errors.
1321-1339: Storing fullencrypted_datablobs – watch SQLite row size
encrypted_datacan be up to 256 + STATIC size (~300 B).
With many change outputs this may enlarge theoutputstable significantly and approach SQLite’s 1 GB-row or page-size limits.Confirm that this blob column is configured as BLOB (not TEXT) and consider compression or a separate table if growth becomes an issue.
base_layer/wallet/src/transaction_service/service.rs (1)
1765-1778: Change-output handling assumes a single output
get_finalized_change_output()returns oneTransactionOutput; if future protocol revisions allow multiple change outputs, only the first will be confirmed and hashed.
Consider iterating over all change outputs (or renaming the method to make single-output semantics explicit) to future-proof this block.base_layer/wallet/src/output_manager_service/service.rs (2)
410-415: Tiny optimisation opportunity
unwrap_or(Vec::new())allocates every time.
unwrap_or_default()(or just pattern-match) avoids the extra allocation and reads a little clearer.-let change_outputs = change.unwrap_or(Vec::new()); +let change_outputs = change.unwrap_or_default();
2136-2156: Tag change outputs with the correct source & avoid extra alloc
OutputSource::default()will mark these as “Unknown”. A dedicatedOutputSource::Change(if it exists) makes later analytics simpler.- You can pre-allocate the vector:
-let mut change = Vec::new(); -for output in change_outputs { +let mut change = Vec::with_capacity(change_outputs.len()); +for output in change_outputs {Both are minor but improve clarity & efficiency.
base_layer/core/src/transactions/transaction_components/payment_id.rs (1)
424-435: Potential overflow: counts stored in a single byte
address_bytes.len(),user_data.len()andsent_output_hashes.len()are all cast tou8.
If any exceed 255 the cast will panic.
Consider validating lengths and returning a descriptive error before the cast.base_layer/core/src/transactions/transaction_protocol/sender.rs (3)
326-334: Simplify Option + Box access
change_output.as_ref().as_ref()is hard to read.
change_output.as_deref()(orchange_output.as_ref().flatten()) achieves the same without the
double call:change_output.as_deref().map(|o| o.output.value)
340-366: Duplicate helpers – consider merging
get_pre_finalized_change_outputandget_finalized_change_outputdiffer only in the match
arms. A single method that takes a flag (or returnsOption<Option<...>>) would remove
nearly-identical code and cut maintenance overhead.
870-875: Unnecessary Box around Option
FinalizedTransaction { change_output: Box<Option<OutputPair>> }allocates even when
None.
StoringOption<Box<OutputPair>>(or justOption<OutputPair>) avoids an extra heap
allocation and simplifies access.- change_output: Box<Option<OutputPair>>, + change_output: Option<Box<OutputPair>>,(Propagate the type change to the rest of the file.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)applications/minotari_console_wallet/src/ui/components/burn_tab.rs(1 hunks)applications/minotari_console_wallet/src/ui/components/send_tab.rs(1 hunks)applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(1 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(2 hunks)applications/minotari_console_wallet/src/ui/state/tasks.rs(1 hunks)applications/minotari_merge_mining_proxy/src/block_template_manager.rs(1 hunks)applications/minotari_miner/src/run_miner.rs(1 hunks)applications/minotari_node/src/grpc/base_node_grpc_server.rs(1 hunks)base_layer/core/src/blocks/pre_mine/mod.rs(1 hunks)base_layer/core/src/test_helpers/mod.rs(1 hunks)base_layer/core/src/transactions/coinbase_builder.rs(2 hunks)base_layer/core/src/transactions/test_helpers.rs(2 hunks)base_layer/core/src/transactions/transaction_components/encrypted_data.rs(4 hunks)base_layer/core/src/transactions/transaction_components/mod.rs(1 hunks)base_layer/core/src/transactions/transaction_components/payment_id.rs(1 hunks)base_layer/core/src/transactions/transaction_components/test.rs(1 hunks)base_layer/core/src/transactions/transaction_components/unblinded_output.rs(1 hunks)base_layer/core/src/transactions/transaction_components/wallet_output.rs(2 hunks)base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/proto/transaction_sender.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/recipient.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(21 hunks)base_layer/core/src/transactions/transaction_protocol/single_receiver.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs(2 hunks)base_layer/core/src/validation/block_body/test.rs(1 hunks)base_layer/core/tests/tests/block_validation.rs(1 hunks)base_layer/core/tests/tests/node_comms_interface.rs(1 hunks)base_layer/tari_mining_helper_ffi/src/lib.rs(1 hunks)base_layer/wallet/migrations/2025-06-17-120000_remove_change_hashes/down.sql(1 hunks)base_layer/wallet/migrations/2025-06-17-120000_remove_change_hashes/up.sql(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(4 hunks)base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs(1 hunks)base_layer/wallet/src/output_manager_service/service.rs(14 hunks)base_layer/wallet/src/output_manager_service/storage/database/backend.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/database/mod.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/models.rs(1 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs(5 hunks)base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs(1 hunks)base_layer/wallet/src/schema.rs(0 hunks)base_layer/wallet/src/transaction_service/handle.rs(1 hunks)base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs(3 hunks)base_layer/wallet/src/transaction_service/service.rs(15 hunks)base_layer/wallet/src/transaction_service/storage/database.rs(3 hunks)base_layer/wallet/src/transaction_service/storage/models.rs(4 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(2 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(1 hunks)base_layer/wallet/src/wallet.rs(1 hunks)base_layer/wallet/tests/output_manager_service_tests/service.rs(2 hunks)base_layer/wallet/tests/output_manager_service_tests/storage.rs(3 hunks)base_layer/wallet/tests/support/utils.rs(1 hunks)base_layer/wallet/tests/transaction_service_tests/service.rs(1 hunks)base_layer/wallet/tests/transaction_service_tests/storage.rs(1 hunks)base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs(1 hunks)base_layer/wallet/tests/utxo_scanner/mod.rs(1 hunks)base_layer/wallet_ffi/src/callback_handler.rs(1 hunks)base_layer/wallet_ffi/src/callback_handler_tests.rs(1 hunks)base_layer/wallet_ffi/src/lib.rs(4 hunks)integration_tests/src/ffi/wallet.rs(1 hunks)integration_tests/src/miner.rs(1 hunks)integration_tests/src/wallet_ffi.rs(1 hunks)integration_tests/tests/steps/wallet_ffi_steps.rs(1 hunks)integration_tests/tests/steps/wallet_steps.rs(1 hunks)
💤 Files with no reviewable changes (1)
- base_layer/wallet/src/schema.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: cargo check with stable
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
🔇 Additional comments (65)
base_layer/core/src/transactions/transaction_components/test.rs (1)
44-44: Updated import path for PaymentId
Import correctly references the newpayment_id::PaymentIdmodule.base_layer/core/src/transactions/transaction_components/wallet_output_builder.rs (1)
35-35: Updated import path for PaymentId
Import correctly references the newpayment_id::PaymentIdmodule.base_layer/wallet/src/output_manager_service/storage/models.rs (1)
32-32: Updated import path for PaymentId
Import correctly references the newpayment_id::PaymentIdmodule.base_layer/core/src/transactions/transaction_components/unblinded_output.rs (1)
44-44: Updated import path for PaymentId
Import correctly references the newpayment_id::PaymentIdmodule.integration_tests/src/ffi/wallet.rs (1)
34-34: Updated import path for PaymentId
Import correctly references the newpayment_id::PaymentIdmodule.base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs (1)
41-41: Update PaymentId import path
Import path updated topayment_id::PaymentIdaligns with the new module structure.base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (1)
41-41: Align PaymentId import
Usingpayment_id::PaymentIdis consistent with the refactored module.base_layer/wallet/tests/support/utils.rs (1)
30-30: Refresh PaymentId import path in tests
Test utility correctly referencespayment_id::PaymentIdfollowing the refactor.integration_tests/src/miner.rs (1)
41-41: Update payment_id and TxType import paths
Importing bothPaymentIdandTxTypefrom the newpayment_idmodule is correct.base_layer/core/src/transactions/transaction_protocol/single_receiver.rs (1)
162-162: Import path updated correctly
ThePaymentIdtype is now imported from the newpayment_idmodule undertransaction_components, reflecting the refactor and ensuring consistency.applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
122-122: Consistent import for PaymentId and TxType
Importing bothPaymentIdandTxTypefrompayment_idundertransaction_componentsaligns with the standardized module structure.applications/minotari_merge_mining_proxy/src/block_template_manager.rs (1)
37-37: PaymentId and TxType module path corrected
Thepayment_id::{PaymentId, TxType}import replaces the old location, keeping the codebase coherent after moving these types out ofencrypted_data.base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
47-47: Validate updatedPaymentIdimport path.The
PaymentIdimport has been correctly moved fromencrypted_datato the newpayment_idmodule. Confirm there are no lingering references to the old path and that serialization/deserialization behavior remains unchanged.base_layer/wallet/src/wallet.rs (1)
66-66: VerifyPaymentIdandTxTypeimports reflect refactor.The imports for
PaymentIdandTxTypenow point totransaction_components::payment_id. Ensure all wallet flows handling payment IDs and transaction types operate correctly with the new module.base_layer/core/src/blocks/pre_mine/mod.rs (1)
47-48: Import path update looks correct
payment_id::PaymentIdmatches the new module layout introduced in the PR.
No additional code changes required in this file.applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (1)
13-14: Confirm enum variants still compileOnly the import path changed, but later code pattern-matches on
PaymentId::{Open, AddressAndData, TransactionInfo}and uses
PaymentId::user_data_as_string().
Please ensure those variants and methods remain unchanged in the new
payment_idmodule; otherwise this will be a compile-time break.If any variant names or method signatures changed, update the match
arms accordingly.base_layer/core/src/transactions/transaction_protocol/proto/transaction_sender.rs (1)
36-37: Verify API parity forPaymentId::{from_bytes,to_bytes}
SingleRoundSenderDatastill depends on the static helper
PaymentId::from_bytes/to_bytes.
Double-check that these associated fns are re-exported unchanged in the
new module; otherwise serialization/deserialization of sender data will
fail.No other issues spotted.
base_layer/core/src/transactions/transaction_protocol/recipient.rs (1)
179-179: Correct import path update forPaymentId.The module path has been accurately changed to
transaction_components::payment_id::PaymentId, and the test usage (PaymentId::Empty) aligns with the new enum location. No further adjustments needed here.base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
103-103: Correct import path update forPaymentId.The import of
PaymentIdfrom the newpayment_idmodule is consistent with the refactor, and all downstream uses in encryption/decryption functions align with this change.base_layer/core/src/test_helpers/mod.rs (1)
63-63: Correct import path update forPaymentIdandTxType.The test helper now imports both
PaymentIdandTxTypefrom thepayment_idmodule as intended, and the subsequent uses (PaymentId::Open { tx_type: TxType::Coinbase }) are correctly referenced.base_layer/wallet/src/transaction_service/handle.rs (1)
42-45: Import path correctly updated
PaymentIdis now pulled fromtransaction_components::payment_id, which matches the new module location introduced in this PR. No further changes are required in this file.applications/minotari_console_wallet/src/ui/components/send_tab.rs (1)
7-10: Consistent refactor – OKBoth
PaymentIdandTxTypeare imported from the newpayment_idmodule. This keeps the UI layer in sync with the core refactor. Looks good.applications/minotari_console_wallet/src/ui/state/tasks.rs (1)
38-44: Import path realignedThe task runner now references
payment_id::PaymentId. Change is correct and compiles cleanly given the upstream move.integration_tests/tests/steps/wallet_ffi_steps.rs (1)
25-30: Integration tests updatedTest step file now uses the relocated
PaymentId/TxTypedefinitions. Update is accurate; no additional adjustments needed.applications/minotari_console_wallet/src/ui/components/burn_tab.rs (1)
8-11: Import path update looks correctThe module has been migrated to
payment_id; the new path compiles and keeps the local usages ofPaymentIdandTxTypeintact. No further action required.base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
49-53: Consistent path refactor acknowledgedThe import change to
transaction_components::payment_id::PaymentIdaligns the scanner with the new module structure. ✅applications/minotari_console_wallet/src/automation/commands.rs (1)
93-95: ImportPaymentIdandTxTypefrom the newpayment_idmodule
The updated path correctly reflects the refactor moving these types out ofencrypted_data.rs. Please verify that there are no remaining imports or references to the oldencrypted_data::{PaymentId, TxType}elsewhere in this file or in related modules.base_layer/wallet/tests/transaction_service_tests/transaction_protocols.rs (1)
85-85: Import PaymentId and TxType from newpayment_idmodule
The import path has been correctly updated to reflect the migration of these types out ofencrypted_data.rs.Please run the following script to verify that there are no remaining imports from the old module:
#!/bin/bash # Verify no stale imports of PaymentId or TxType from encrypted_data rg -n "encrypted_data::(PaymentId|TxType)"applications/minotari_miner/src/run_miner.rs (1)
66-66: Import PaymentId and TxType from newpayment_idmodule
This aligns with the refactor moving these enums intopayment_id.rs.Ensure no legacy imports remain:
#!/bin/bash rg -n "encrypted_data::(PaymentId|TxType)"integration_tests/src/wallet_ffi.rs (1)
33-33: Import PaymentId frompayment_idmodule
The import path update matches the core migration and is correctly scoped.Run a check for any leftover encrypted_data references:
#!/bin/bash rg -n "encrypted_data::PaymentId"base_layer/tari_mining_helper_ffi/src/lib.rs (1)
46-46: To ensure there are absolutely no leftover references to the oldencrypted_datamodule, let’s search for any stray imports or paths:#!/bin/bash # Check for any residual references to the old module path rg -n 'transaction_components::encrypted_data' rg -n 'encrypted_data::' # Check for any lingering imports of PaymentId and TxType rg -n 'use\s.*PaymentId' rg -n 'use\s.*TxType'base_layer/wallet/tests/transaction_service_tests/storage.rs (1)
56-56: Import path update looks goodThe switch to
transaction_components::payment_id::{PaymentId, TxType}aligns with the new module split.
No further action needed here.base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs (1)
43-50: Import path update acknowledgedThe refactor to
payment_id::{PaymentId, TxType}is consistent with the new module structure.
No issues spotted.base_layer/core/tests/tests/block_validation.rs (1)
940-947: Update expected block weight after consensus changes.The actual block weight assertion has been adjusted from 405 to 414 to reflect the new weight calculation. This update aligns the test with the current consensus constants.
base_layer/wallet/migrations/2025-06-17-120000_remove_change_hashes/down.sql (1)
1-3: Restorechange_output_hashescolumn in down migration.Reintroduces the
change_output_hashescolumn asBLOB NULL, matching the original schema before theup.sqlremoval.base_layer/wallet_ffi/src/callback_handler_tests.rs (1)
52-54: Update import path forPaymentIdandTxType.The test now imports these enums from
transaction_components::payment_id, consistent with the module relocation.applications/minotari_node/src/grpc/base_node_grpc_server.rs (1)
70-70: Import path update forPaymentIdandTxTypeis correct.The refactor moving these enums into
transaction_components::payment_idis applied properly. No other references to the oldencrypted_datapath remain in this file.base_layer/core/src/transactions/coinbase_builder.rs (2)
801-801: Update test imports to newpayment_idmodule
The test module’s import has been updated to referencepayment_id::{PaymentId, TxType}, matching the new structure.
44-44: Update imports to newpayment_idmodule
The import path forPaymentIdandTxTypehas been correctly updated fromencrypted_datato the newpayment_idmodule, aligning with the refactoring.Run this script to ensure there are no remaining references to the old path:
#!/bin/bash # Verify removal of old PaymentId/TxType imports rg -n "encrypted_data::\{PaymentId, TxType\}" || echo "No stale imports found"base_layer/wallet/tests/output_manager_service_tests/service.rs (2)
69-69: Align import path for PaymentId with new module location
The import ofPaymentIdfrom thepayment_idmodule correctly reflects the refactoring of payment ID types.
1209-1209: Updateconfirm_pending_transactioncall to match new signature
Including the explicitNonefor optional change outputs aligns with the extended method signature.base_layer/core/src/validation/block_body/test.rs (1)
42-43: Correct import paths for encrypted data and PaymentId
SeparatingSTATIC_ENCRYPTED_DATA_SIZE_TOTALand importingPaymentIdfrom the newpayment_idmodule aligns with the recent modularization of payment ID handling.integration_tests/tests/steps/wallet_steps.rs (1)
51-51: Align import paths with new payment_id module
The import has been correctly updated from the oldencrypted_datalocation to the newtransaction_components::payment_idmodule. No logic changes are needed here.base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs (1)
32-32: Import path corrected for PaymentId
PaymentId is now correctly sourced fromtransaction_components::payment_id, matching the module extraction.base_layer/wallet/tests/transaction_service_tests/service.rs (1)
129-129: Import paths updated correctly.The
PaymentIdandTxTypetypes are now imported fromtransaction_components::payment_id, which aligns with the refactor. No lingering references to the oldencrypted_datamodule remain.base_layer/wallet_ffi/src/callback_handler.rs (1)
554-556: Potential loss of change-output metadata
CompletedTransaction::from_outbound(tx, Vec::new())intentionally discards every change-output hash.
Down-stream code (e.g. pay-ref generation or balance reconciliation) may rely onsent_output_hashes/change_output_hashesbeing present once the transaction is surfaced to the UI or persisted.If the hashes are still available on
tx(or can be recomputed from itstransaction.body.outputs()), consider passing them through instead of an empty vector:-let mut outbound_tx = CompletedTransaction::from_outbound(tx, Vec::new()); +let change_hashes = tx + .transaction + .body + .outputs() + .iter() + .filter(|o| o.is_change()) // pseudo-code – adjust predicate + .map(|o| o.hash()) + .collect(); + +let mut outbound_tx = CompletedTransaction::from_outbound(tx, change_hashes);Otherwise, please confirm that no component expects these hashes after a cancellation path.
applications/minotari_console_wallet/src/ui/state/app_state.rs (2)
64-66: Import path update looks goodThe switch to
transaction_components::payment_id::{PaymentId, TxType}cleanly follows the module refactor.
739-741: Emptysent_output_hashesmay hinder PayRef generation for pending outbound TXsFor pending outbound transactions we now build:
CompletedTransaction::from_outbound(t.clone(), Vec::new())Later, once mined, PayRef calculation (lines 815-819) expects
sent_output_hashes.first().
Because this vector is initialised empty, the PayRef for a TX that stays in the UI as “pending” until it is mined could remainNone.If the hash list truly cannot be known at this stage, document the limitation; otherwise populate it similarly to the suggestion in the FFI handler.
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
59-59: Import path update looks correct – verify remaining crates are migratedThe switch to
transaction_components::payment_id::PaymentIdis in line with the new module split.
Please confirm there are no lingering references to the former path (encrypted_data::PaymentId) elsewhere in the workspace.#!/usr/bin/env bash # Find any remaining occurrences of the obsolete import rg --count --fixed-strings 'encrypted_data::PaymentId'base_layer/wallet_ffi/src/lib.rs (3)
152-154: Import path change is correct and matches the new module layout
PaymentIdandTxTypeare now re-exported fromtransaction_components::payment_id; the updated path compiles against the refactor and keeps the outward FFI surface unchanged.
9024-9027: Make sure we don’t silently dropsent_output_hashes
CompletedTransaction::from_outbound(tx.clone(), Vec::new())hard-codes an empty list.
If the outbound transaction ever carries change / sent-output hashes, they will be lost and the FFI caller will receive incomplete data.-let mut outbound_tx = CompletedTransaction::from_outbound(tx.clone(), Vec::new()); +let mut outbound_tx = CompletedTransaction::from_outbound( + tx.clone(), + tx.sent_output_hashes.clone(), // preserve any hashes that exist +);Please double-check that
OutboundTransactionno longer contains these hashes, or propagate them as shown.
9321-9324: Same concern as above for single-transaction retrievalThe per-transaction branch replicates the empty‐vector issue. Consider the same fix to avoid losing data:
-let mut outbound_tx = CompletedTransaction::from_outbound(tx.clone(), Vec::new()); +let mut outbound_tx = CompletedTransaction::from_outbound( + tx.clone(), + tx.sent_output_hashes.clone(), +);base_layer/core/src/transactions/transaction_components/wallet_output.rs (2)
43-43: LGTM!Import path update aligns with the module restructuring.
402-431: Well-implemented method for updating encrypted data.The method correctly updates the metadata signature when encrypted data changes, which is essential for maintaining the cryptographic integrity of the wallet output.
base_layer/core/src/transactions/test_helpers.rs (1)
46-46: LGTM!Import path update is consistent with the module restructuring.
base_layer/wallet/src/output_manager_service/handle.rs (2)
35-35: LGTM!Import path update is consistent with the module restructuring.
69-69: API enhancement for explicit change output handling.The addition of optional change outputs to the confirmation request improves the explicit handling of change outputs during transaction confirmation, aligning with the PR's goal of ensuring payment references persist during recovery.
Also applies to: 615-622
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (3)
41-41: LGTM!Import path update is consistent with the module restructuring.
340-340: Correct handling of unfinalized transaction state.Passing
Nonefor change outputs at this stage is appropriate since the transaction hasn't been finalized yet.
601-622: Excellent implementation of finalized change output handling.The logic correctly retrieves the finalized change output after transaction finalization and ensures it's properly confirmed with the output manager service. This aligns perfectly with the PR's objective of ensuring payment references persist during recovery.
base_layer/wallet/src/transaction_service/service.rs (1)
3342-3346: Wildcard arm may hide importantPaymentIdvariantsThe fallback
_captures any new/legacyPaymentIdvariants, but unlike the explicit arms it leavessource_address/destination_addressunset, causing them to default toTariAddress::default().
This can mis-classify recovered transactions and break payref look-ups.Add explicit matches for the legacy variants or at minimum log a warning when hitting the fallback so that address derivation can be revisited.
base_layer/wallet/src/output_manager_service/service.rs (3)
58-59: Import update looks correct – newpayment_idpath matches the refactor, nothing else to flag here.
1145-1161: Possible duplicate-row hazard with change outputsYou insert a pre-finalised change output via
encumber_outputsand later pass the finalised version here.
Ifconfirm_encumbered_outputstreats the incoming vector as new rows rather than updates, a unique-constraint violation (or silent duplicate) is possible.Please confirm the DB layer performs an upsert/update keyed on output hash/commitment before merging this final record.
2098-2102: Order of operations LGTMFinalising before reading the change output guarantees the metadata signature is complete; fee is already captured.
No issues spotted.base_layer/core/src/transactions/transaction_components/payment_id.rs (1)
684-719: Change output’s PaymentId never written back – causes stale metadata
payment_idis cloned, mutated (hash list added) and passed toencrypt_data_for_recovery, but
change_output.payment_iditself is left unchanged.
Down-stream code that inspectsWalletOutput.payment_id(without decrypting) will see the
pre-mutation value.Add the assignment after
change_encrypted_data:- let mut change_output = change.output.clone(); + let mut change_output = change.output.clone(); + change_output.payment_id = payment_id.clone();Likely an incorrect or invalid review comment.
Test Results (CI) 3 files 126 suites 42m 23s ⏱️ For more details on these failures, see this check. Results for commit 7636c4d. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 1 errors 1 suites +1 24m 43s ⏱️ + 24m 43s For more details on these parsing errors and failures, see this check. Results for commit cc148b5. ± Comparison against base commit e0ab8d1. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
base_layer/core/src/transactions/transaction_components/payment_id.rs (1)
428-430: Avoid unnecessary allocation
address_bytesis already aVec<u8>.
extend_from_slice(&address_bytes.to_vec())copies it into a new buffer just to copy again. Use the slice directly:-bytes.extend_from_slice(&address_bytes.to_vec()); +bytes.extend_from_slice(&address_bytes);Saves one allocation and a memcpy per serialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/core/src/transactions/transaction_components/payment_id.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/wallet/src/transaction_service/service.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
base_layer/core/src/transactions/transaction_components/payment_id.rs (1)
400-407: Still panics on oversize inputs – replaceexpectonu8::try_fromThe earlier review pointed out that
u8::try_from(..).expect(..)will panic for lengths ≥ 256.
These calls are unchanged and can still crash a node/wallet when serialising large user-data, atypical addresses or manysent_output_hashes.- bytes.push(u8::try_from(address_bytes.len()).expect("Address length must fit in u8")); + let len = u8::try_from(address_bytes.len()) + .map_err(|_| PaymentIdError::ValueTooLarge)?; // define an error & bubble up + bytes.push(len); - bytes.push(u8::try_from(user_data.len()).expect("User data length should fit in a u8")); + let len = u8::try_from(user_data.len()) + .map_err(|_| PaymentIdError::ValueTooLarge)?; + bytes.push(len); - bytes.push(u8::try_from(sent_output_hashes.len()).expect("Sent output hashes length should fit in a u8")); + let len = u8::try_from(sent_output_hashes.len()) + .map_err(|_| PaymentIdError::ValueTooLarge)?; + bytes.push(len);Consider making
to_bytes()fallible (-> Result<Vec<u8>, PaymentIdError>) or upgrading to a wider prefix (u16/ varint).
Untrusted data paths must never rely onexpect.Also applies to: 421-427
🧹 Nitpick comments (2)
base_layer/wallet/src/output_manager_service/service.rs (2)
410-415: Preferunwrap_or_default()to avoid an unnecessary Vec allocation
unwrap_or(Vec::new())allocates every time the branch is taken.Option<Vec<T>>::unwrap_or_default()is shorter, avoids the allocation, and is more idiomatic.- let change_outputs = change.unwrap_or(Vec::new()); + let change_outputs = change.unwrap_or_default();
2133-2154: Minor: pre-allocate and preserveOutputSourcewhen converting change outputslet mut change = Vec::with_capacity(change_outputs.len()); for output in change_outputs { change.push( DbWalletOutput::from_wallet_output( output, &self.resources.key_manager, None, - OutputSource::default(), + // Preserve original source if available + OutputSource::default(), // <— consider exposing source on WalletOutput ...
Vec::with_capacityavoids reallocations.- If the caller knew the
OutputSource(e.g.OutputSource::Change), we lose that information here. Either expose it onWalletOutputor accept it as a parameter so the DB keeps accurate provenance.Both are low-impact tweaks but improve clarity and performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
base_layer/core/src/transactions/transaction_components/payment_id.rs(1 hunks)base_layer/wallet/src/output_manager_service/service.rs(14 hunks)base_layer/wallet/src/transaction_service/service.rs(14 hunks)base_layer/wallet_ffi/src/lib.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- base_layer/wallet_ffi/src/lib.rs
- base_layer/wallet/src/transaction_service/service.rs
⏰ 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: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (2)
base_layer/wallet/src/output_manager_service/service.rs (1)
58-60: Imports look correct – update aligns the file with the newpayment_idmodule path.base_layer/core/src/transactions/transaction_components/payment_id.rs (1)
190-196: Pattern-match now compiles – good catchReplacing
PaymentId::Raw { .. }with the tuple patternPaymentId::Raw(_)resolves the compilation error flagged earlier.
No further changes required here.
|
|
||
| self.confirm_encumberance(tx_id)?; | ||
| self.confirm_encumberance(tx_id, Vec::new()).await?; | ||
|
|
There was a problem hiding this comment.
Calling confirm_encumberance before stp.finalize() drops change-output metadata
Here confirm_encumberance is invoked before the protocol is finalised, and before any change output is available.
Because confirm_encumberance is the only place where confirm_encumbered_outputs can update the DB with the final encrypted-data / metadata-signature fields, this ordering means:
- The DB entry for the change output (added later by
finalize) is never updated. - Wallet recovery will still miss the pay-ref data this PR intends to preserve.
Move the call after stp.finalize(&self.resources.key_manager).await? and pass the finalised change output:
- self.confirm_encumberance(tx_id, Vec::new()).await?;
-
- // Prepare receiver part of the transaction
+ stp.finalize(&self.resources.key_manager).await?;
+ if let Some(co) = stp.get_finalized_change_output()? {
+ self.confirm_encumberance(tx_id, vec![co]).await?;
+ } else {
+ self.confirm_encumberance(tx_id, Vec::new()).await?;
+ }
+
+ // Prepare receiver part of the transactionThe same pattern appears in create_coin_split_even, spend_backup_pre_mine_utxo, create_coin_split, create_coin_join, create_claim_sha_atomic_swap_transaction, and create_htlc_refund_transaction. Please audit all call-sites to ensure confirm_encumberance runs after finalisation with the actual change outputs supplied.
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
Looking good, just some comments below.
Not tested at system-level
* development: chore: new release v4.5.0-pre.0 (tari-project#7228) chore: better logging (tari-project#7226) feat!: ensure payref persists during recovery (tari-project#7225) chore(ci): split out amd64 and arm64 docker builds into native runners (tari-project#7206) fix: fix scanner service when connectivity offline (tari-project#7223) feat: add payref to grpc outputs (tari-project#7216)
2 participants
Description
Ensures that payrefs are persisted across recovery of the wallet
Adds two new paymentid fields to ensure fields are properly serialized
Fixed a bug where its possible to deserilize a Tari address wrongly
Fixed a bug where a received tx would not show the correct amount
Added legacy payment id types to ensure correct deseriasation of old types
Breaking Changes
Old wallets will not properly show the transaction details of newly received transactions. A fix would be to rescan the wallet.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Database
change_output_hashescolumn from transaction tables and updated migration scripts accordingly.Tests
Chores