Feat/wallet multisig #7281
Feat/wallet multisig #7281
Conversation
WalkthroughThis change introduces comprehensive multisignature (multisig) UTXO support throughout the wallet and gRPC layers. It adds new CLI commands, gRPC methods, protobuf definitions, and wallet service logic for multisig creation, signing, encumbrance, and transaction handling. Supporting modules, error types, Docker configurations, and ignore files are also updated or introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Wallet
participant gRPC
participant TransactionService
participant OutputManager
CLI->>Wallet: Issue multisig command (e.g., CreateMultisigUtxo)
Wallet->>gRPC: Call corresponding multisig gRPC method
gRPC->>TransactionService: Forward multisig request (e.g., create_multisig_utxo)
TransactionService->>OutputManager: Create output(s), scan, encumber, etc.
OutputManager-->>TransactionService: Return outputs/results
TransactionService-->>gRPC: Return multisig operation result
gRPC-->>Wallet: Respond to gRPC call
Wallet-->>CLI: Output result/status
Estimated code review effort4 (60–120 minutes) 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 (
|
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (24)
.gitignore (1)
4-5: Pattern looks fine but consider grouping for discoverability
wallet*_datais a reasonable glob to exclude the per-wallet data dirs introduced by docker-compose.
For maintainability you may want to move it down to the “Ignore DataStore, Database and Log files” section (≈ line 36) where similar artifacts are already listed, so new contributors find all runtime-data ignores in one place.infrastructure/tari_script/src/script.rs (1)
68-68: Consider the encapsulation trade-off.Making the
scriptfield public breaks encapsulation by exposing internal implementation details. While this enables the required multisig script analysis functionality, consider if ascript()accessor method would better maintain the API boundary.However, given the performance-critical nature of script execution and the need for direct opcode manipulation in multisig workflows, this approach may be acceptable.
docker-compose.yml (1)
1-26: Well-structured Docker Compose setup for multisig testing.The configuration correctly isolates each wallet instance with separate data volumes and consistent network settings, which is ideal for testing multisig workflows.
Consider extracting common configuration to reduce duplication:
version: "3.8" +x-wallet-common: &wallet-common + build: . + entrypoint: ["minotari_console_wallet", "--base-path", "/wallet_data"] + command: ["--network", "esmeralda"] + services: wallet1: - build: . + <<: *wallet-common container_name: wallet1 volumes: - ./wallet1_data:/wallet_data - entrypoint: ["minotari_console_wallet", "--base-path", "/wallet_data"] - command: ["--network", "esmeralda"] wallet2: - build: . + <<: *wallet-common container_name: wallet2 volumes: - ./wallet2_data:/wallet_data - entrypoint: ["minotari_console_wallet", "--base-path", "/wallet_data"] - command: ["--network", "esmeralda"] wallet3: - build: . + <<: *wallet-common container_name: wallet3 volumes: - ./wallet3_data:/wallet_data - entrypoint: ["minotari_console_wallet", "--base-path", "/wallet_data"] - command: ["--network", "esmeralda"]base_layer/wallet/src/transaction_service/handle.rs (1)
1303-1318: Method implementation is correct but could improve error handling clarity.The method properly calls the service and handles errors, but the error mapping could be more specific.
Consider improving error handling specificity:
pub async fn insert_completed_transaction( &mut self, tx_id: TxId, transaction: CompletedTransaction, ) -> Result<TxId, TransactionStorageError> { self .handle .call(TransactionServiceRequest::InsertCompletedTransaction(tx_id, transaction)) .await - .map_err(|e| TransactionStorageError::UnexpectedResult(e.to_string()))? + .map_err(|e| TransactionStorageError::UnexpectedResult(format!("Service call failed: {}", e)))? .map(|_| ()) - .map_err(|e| TransactionStorageError::UnexpectedResult(e.to_string()))?; + .map_err(|e| TransactionStorageError::UnexpectedResult(format!("Transaction insertion failed: {}", e)))?; Ok(tx_id) }applications/minotari_console_wallet/src/automation/commands.rs (6)
88-88: Improve import formatting for readability.The imports should be formatted on separate lines for better readability and maintainability.
- blocks::pre_mine::get_pre_mine_items, consensus::ConsensusManager, covenants::Covenant, one_sided::shared_secret_to_output_encryption_key, transactions::{ + blocks::pre_mine::get_pre_mine_items, + consensus::ConsensusManager, + covenants::Covenant, + one_sided::shared_secret_to_output_encryption_key, + transactions::{
105-105: Fix closing brace placement.The closing brace should be on the same line as the last import for consistency.
- } + },
108-108: Split imports across multiple lines.Multiple imports on a single line reduce readability.
- commitment::HomomorphicCommitmentFactory, dhke::DiffieHellmanSharedSecret, ristretto::{RistrettoSecretKey} + commitment::HomomorphicCommitmentFactory, + dhke::DiffieHellmanSharedSecret, + ristretto::RistrettoSecretKey,
123-123: Break up long import line for better readability.This line is too long and contains deeply nested module imports that should be formatted across multiple lines.
- multisig::{encumber::collect_multisig_utxo_encumber, io::{save_multisig_member_signatures, save_multisig_output, save_multisig_party_output, save_multisig_utxo_encumber}, party::{create_multisig_party_member_output, send_multisig_utxo_by_leader, sign_multisig_utxo_by_member}, script::{get_utxo_by_commitment_hash, is_multisig_utxo}, session::{create_multisig_output, make_utxo_multisig}}, utils::{ + multisig::{ + encumber::collect_multisig_utxo_encumber, + io::{ + save_multisig_member_signatures, + save_multisig_output, + save_multisig_party_output, + save_multisig_utxo_encumber, + }, + party::{ + create_multisig_party_member_output, + send_multisig_utxo_by_leader, + sign_multisig_utxo_by_member, + }, + script::{get_utxo_by_commitment_hash, is_multisig_utxo}, + session::{create_multisig_output, make_utxo_multisig}, + }, + utils::{
2725-2735: Improve error message for tip height retrieval.The error message should be more descriptive about the failure to help with debugging.
-.ok_or(CommandError::General("Could not get tip height".to_string()))?; +.ok_or(CommandError::General("Failed to retrieve blockchain tip height from base node".to_string()))?;
2770-2772: Improve parameter validation error message.The error message should specify the actual values to help users correct their input.
-return Err(CommandError::General("n must be equal to the number of public keys".to_string())); +return Err(CommandError::General(format!( + "Parameter 'n' ({}) must be equal to the number of public keys provided ({})", + args.n, args.public_keys.len() +)));applications/minotari_console_wallet/src/automation/multisig/session.rs (1)
4-17: Consolidate and organize importsThe imports have redundancy and could be better organized.
TariScriptis imported twice and imports from the same crate should be grouped together.-use tari_common_types::{transaction::TxId}; -use tari_core::transactions::{tari_amount::MicroMinotari, transaction_components::{encrypted_data::PaymentId, EncryptedData, WalletOutputBuilder}, transaction_key_manager::{storage::sqlite_db::TransactionKeyManagerSqliteDatabase, TransactionKeyManagerInterface, TransactionKeyManagerWrapper}}; +use tari_common_types::transaction::TxId; +use tari_core::transactions::{ + tari_amount::MicroMinotari, + transaction_components::{ + encrypted_data::PaymentId, + EncryptedData, + OutputFeatures, + WalletOutputBuilder, + }, + transaction_key_manager::{ + storage::sqlite_db::TransactionKeyManagerSqliteDatabase, + TransactionKeyManagerInterface, + TransactionKeyManagerWrapper, + }, +}; use tari_crypto::{compressed_key::CompressedKey, ristretto::RistrettoPublicKey}; -use tari_script::{Opcode, TariScript}; +use tari_script::{ExecutionStack, Opcode, TariScript}; use tari_utilities::hex::Hex; -use tari_core::transactions::{ - transaction_components::{ - - OutputFeatures, - - }, -}; -use tari_script::{ExecutionStack};applications/minotari_console_wallet/src/automation/multisig/io.rs (2)
26-41: Remove redundant directory creation in load functionThe load function creates the directory if it doesn't exist, which is unnecessary for a read operation. If the directory doesn't exist, the file won't exist either.
pub async fn load_multisig_output(session_id: &str) -> Result<MultisigOutput, CommandError> { - - let out_dir = std::path::Path::new("/wallet_data"); - if !out_dir.exists() { - std::fs::create_dir_all(out_dir)?; - } - let file_path = std::path::Path::new("/wallet_data").join(format!("multisig_output-{}.json", session_id)); - + if !file_path.exists() { + return Err(CommandError::General(format!( + "Multisig output file not found: {}", + file_path.display() + ))); + } + let file = fs::File::open(file_path)?;
157-175: Consistent function parameter namingThe function uses
own_addressparameter but other similar functions use member/party terminology. Consider using consistent naming.Consider renaming
own_addresstomember_addressfor consistency with other multisig functions in the module.applications/minotari_console_wallet/src/automation/multisig/script.rs (1)
97-126: Add documentation for aggregate UTXO finalizationThis function handles critical multisig transaction finalization but lacks documentation. The comment on line 97 has a grammatical error.
-/// finalizes an already encumbered a n-of-m transaction +/// Finalizes an already encumbered n-of-m multisig transaction by aggregating +/// metadata and script signatures from multiple parties and applying the final +/// wallet script secret key. +/// +/// # Arguments +/// * `transaction_service` - Handle to the transaction service +/// * `tx_id` - Transaction ID to finalize +/// * `meta_signatures` - Vector of metadata signatures from all parties +/// * `script_signatures` - Vector of script signatures from all parties +/// * `wallet_script_secret_key` - The wallet's script secret key for finalization pub async fn finalize_aggregate_utxo(applications/minotari_console_wallet/src/cli.rs (2)
33-38: Clean up redundant importThere's a redundant import that should be consolidated.
The imports from
tari_common_typesandtari_corecan be better organized by grouping related items together.
510-548: Use proper documentation comments for struct fieldsThe struct fields use regular comments (
//) instead of documentation comments (///). This prevents the documentation from being included in generated docs.pub struct CreateMultisigUtxoTransferLeaderArgs { #[clap(long)] - // The commitment hash of the multisig UTXO + /// The commitment hash of the multisig UTXO pub utxo_commitment_hash: String, #[clap(long)] - // The recipient address of the multisig UTXO + /// The recipient address of the multisig UTXO pub recipient_address: TariAddress, - // How many parties are involved in the multisig + /// How many parties are involved in the multisig #[clap(long, default_value = "2")] pub n: u8, - // How many signatures are required to spend the multisig UTXO + /// How many signatures are required to spend the multisig UTXO #[clap(long, default_value = "2")] pub m: u8, #[clap(long, multiple = true)] - // list of public keys of the parties involved in the multisig + /// List of public keys of the parties involved in the multisig pub public_keys: Vec<UniPublicKey>, }Apply the same pattern to
CreateMultisigUtxoArgsstruct fields.applications/minotari_console_wallet/src/automation/multisig/party.rs (1)
251-259: Add validation for default signaturesChecking if signatures equal default values is a good safety check, but the error message could be more informative.
if script_signature.get_signature() == Signature::default().get_signature() || metadata_signature.get_signature() == Signature::default().get_signature() { - eprintln!( - "\nError: Script and/or metadata signatures not created (index {})!\n", - output_index, - ); - break; + return Err(CommandError::General(format!( + "Invalid signatures detected for output index {}: script_sig_valid={}, metadata_sig_valid={}", + output_index, + script_signature.get_signature() != Signature::default().get_signature(), + metadata_signature.get_signature() != Signature::default().get_signature() + ))); }applications/minotari_console_wallet/src/automation/multisig/types.rs (2)
3-3: Consider breaking up long import lines for better readability.This import line is quite long and could be split across multiple lines for improved readability.
Apply this formatting:
-use tari_common_types::{tari_address::TariAddress, transaction::TxId, types::{CompressedCommitment, CompressedPublicKey, PrivateKey, Signature}}; +use tari_common_types::{ + tari_address::TariAddress, + transaction::TxId, + types::{CompressedCommitment, CompressedPublicKey, PrivateKey, Signature} +};
16-16: Remove trailing whitespace.There's trailing whitespace after the field declaration.
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (5)
9-19: Consolidate duplicate import blocks.The imports from
tari_common_typesandtari_coreare split across multiple blocks and should be consolidated.Consolidate the imports:
-use tari_common_types::{ - types::{ - CompressedCommitment, - }, -}; - -use tari_core::transactions::{ - transaction_components::{ - OutputFeatures, - }, -};Move these types to the existing import blocks at lines 2-4 and 6-7 respectively.
69-69: Use consistent error output methods.This warning uses
println!while errors elsewhere useeprintln!. Consider usingeprintln!for all warnings and errors for consistency.- println!("Warning: Could not load leader party output for {}: {}. Skipping.", CompressedPublicKey::from(pubkey.clone()).to_public_key()?.to_hex(), e); + eprintln!("Warning: Could not load leader party output for {}: {}. Skipping.", CompressedPublicKey::from(pubkey.clone()).to_public_key()?.to_hex(), e);
110-110: Address the hardcoded minimum value promise.The comment indicates this is temporary. Consider creating a tracking issue for implementing proper minimum value promise logic.
Would you like me to create an issue to track implementing proper minimum value promise calculation?
254-254: Fix incorrect comment.The comment says "lets add our own signature to the list" but this is actually creating the signature, not adding an existing one.
- // lets add our own signature to the list + // create our own signature and add it to the list
597-597: Remove extra empty line at end of file.There's an unnecessary empty line at the end of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.dockerignore(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)applications/minotari_app_utilities/src/utilities.rs(2 hunks)applications/minotari_console_wallet/Cargo.toml(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(7 hunks)applications/minotari_console_wallet/src/automation/error.rs(1 hunks)applications/minotari_console_wallet/src/automation/mod.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/encumber.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/io.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/mod.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/party.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/script.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/session.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/types.rs(1 hunks)applications/minotari_console_wallet/src/cli.rs(3 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(3 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(2 hunks)base_layer/wallet/src/output_manager_service/handle.rs(4 hunks)base_layer/wallet/src/output_manager_service/service.rs(3 hunks)base_layer/wallet/src/transaction_service/handle.rs(4 hunks)base_layer/wallet/src/transaction_service/service.rs(1 hunks)docker-compose.yml(1 hunks)infrastructure/tari_script/src/script.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (20)
.dockerignore (1)
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
.gitignore (1)
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
applications/minotari_console_wallet/Cargo.toml (2)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: 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.
applications/minotari_app_utilities/src/utilities.rs (3)
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
infrastructure/tari_script/src/script.rs (1)
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.
base_layer/wallet/src/transaction_service/service.rs (1)
undefined
<retrieved_learning>
Learnt from: ksrichard
PR: #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.
</retrieved_learning>
base_layer/wallet/src/transaction_service/handle.rs (1)
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
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.
applications/minotari_console_wallet/src/wallet_modes.rs (2)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (1)
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.
base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
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.
applications/minotari_console_wallet/src/automation/multisig/io.rs (1)
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.
applications/minotari_console_wallet/src/automation/multisig/session.rs (1)
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.
applications/minotari_console_wallet/src/automation/multisig/script.rs (1)
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.
applications/minotari_console_wallet/src/automation/multisig/party.rs (1)
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.
applications/minotari_console_wallet/src/cli.rs (1)
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.
applications/minotari_console_wallet/src/automation/multisig/types.rs (2)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: 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.
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
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.
base_layer/wallet/src/output_manager_service/handle.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
applications/minotari_console_wallet/src/automation/commands.rs (5)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/dht/src/proto/mod.rs:141-142
Timestamp: 2025-05-02T07:12:23.985Z
Learning: The `PeerFeatures::from_bits_u32_truncate` method truncates a u32 to u8 bits but can still return `None` if the resulting bits don't match any valid flags, making the error handling with `.ok_or_else()` necessary even after truncation.
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.
🧬 Code Graph Analysis (5)
base_layer/wallet/src/transaction_service/handle.rs (1)
base_layer/wallet/src/transaction_service/storage/database.rs (2)
write(81-81)insert_completed_transaction(331-341)
base_layer/core/src/transactions/transaction_key_manager/interface.rs (2)
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (1)
stealth_address_script_spending_key_id(598-608)base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
stealth_address_script_spending_key_id(1705-1719)
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (2)
base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
stealth_address_script_spending_key_id(480-484)base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
stealth_address_script_spending_key_id(1705-1719)
base_layer/wallet/src/output_manager_service/service.rs (3)
base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs (18)
hashes(720-724)hashes(725-729)outputs(176-179)outputs(189-192)outputs(199-202)outputs(209-212)outputs(219-222)outputs(254-257)outputs(264-267)outputs(286-289)outputs(311-314)outputs(333-336)outputs(416-419)outputs(516-516)outputs(517-517)outputs(519-519)outputs(520-520)outputs(521-521)base_layer/wallet/src/output_manager_service/handle.rs (2)
confirm_encumberance(1020-1026)fetch_unspent_outputs_from_node(1010-1019)base_layer/wallet/src/transaction_service/service.rs (1)
fetch_unspent_outputs_from_node(1285-1308)
base_layer/core/src/transactions/transaction_key_manager/inner.rs (3)
base_layer/core/src/transactions/transaction_key_manager/interface.rs (1)
stealth_address_script_spending_key_id(480-484)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (1)
stealth_address_script_spending_key_id(598-608)applications/minotari_ledger_wallet/wallet/src/crypto/keys.rs (1)
from_uniform_bytes(211-220)
🔇 Additional comments (21)
.dockerignore (1)
3-3: 👍 Good call on ignoringwallet*_dataExcluding the mounted wallet volumes prevents accidental leakage of private keys or large LMDB files into images.
Nothing further to do here.applications/minotari_console_wallet/src/automation/mod.rs (1)
23-23: Module declaration LGTMAdding
pub mod multisig;cleanly exposes the new multisig automation code and keeps the public API tidy.
No issues spotted.applications/minotari_console_wallet/src/automation/error.rs (1)
101-103: Variant added – consider mapping it in higher-level error handling
PartyMemberNotFoundfits the new multisig workflow. Make sure any code that convertsCommandErrorto protocol-level responses or CLI exit codes treats this variant explicitly (e.g. distinctExitCode::mapping if needed) so users get a clear message.applications/minotari_app_utilities/src/utilities.rs (1)
27-27: LGTM: Clean addition of serialization support.The addition of
SerializeandDeserializetraits toUniPublicKeyfollows Rust best practices and enables the necessary JSON persistence for multisig functionality.Also applies to: 76-76
applications/minotari_console_wallet/Cargo.toml (1)
18-18: LGTM: Appropriate feature addition for multisig functionality.Adding the
base_nodefeature totari_coreenables the necessary blockchain interactions for multisig operations while maintaining selective feature usage.applications/minotari_console_wallet/src/automation/multisig/mod.rs (1)
1-6: LGTM: Well-organized module structure.The module declarations provide a clean separation of concerns for multisig functionality with intuitive naming:
iofor data persistencetypesfor data structuresscriptfor script utilitiessessionfor session managementpartyfor party operationsencumberfor transaction buildingThis follows Rust best practices for module organization.
base_layer/core/src/transactions/transaction_key_manager/interface.rs (2)
34-34: Import addition aligns with new method requirements.The RistrettoSecretKey import is necessary for the new stealth address method return type.
480-485: Well-designed stealth address key derivation method.The method signature is consistent with the trait's patterns and the implementation in
inner.rscorrectly derives ephemeral keys using domain-separated hashing. The error handling follows established conventions.applications/minotari_console_wallet/src/wallet_modes.rs (1)
651-656: Test updates correctly maintain coverage for new multisig commands.The empty match arms are appropriate for this parsing test, which only verifies that the CLI commands can be successfully parsed without executing their logic.
base_layer/wallet/src/transaction_service/handle.rs (3)
64-64: Import addition supports new method's error handling.The TransactionStorageError import is necessary for the new insert_completed_transaction method's return type.
215-215: Request variant follows established patterns.The new InsertCompletedTransaction variant is consistent with other request types in the enum.
411-413: Display implementation is properly formatted.The display format is consistent with other request variants and provides useful debugging information.
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (2)
33-33: Import addition looks good.The
RistrettoSecretKeyimport is necessary for the new method's return type.
598-608: Wrapper implementation follows established pattern.The method correctly delegates to the inner key manager implementation by acquiring a read lock and calling the corresponding method. This is consistent with all other methods in the wrapper.
base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
1705-1719: Implementation is cryptographically sound and well-structured.The method correctly:
- Retrieves private keys for both input key IDs
- Uses domain-separated hashing with the "script key" label (consistent with the existing
HASHER_LABEL_STEALTH_KEYconstant)- Derives a script private key from the hash using
from_uniform_bytes- Combines the spend and script private keys through addition
- Handles errors appropriately
This provides the private key counterpart to the existing
stealth_address_script_spending_keymethod, enabling multisig UTXO workflows as described in the PR context.applications/minotari_console_wallet/src/automation/commands.rs (1)
2118-2134: Good enhancement to show multisig information in UTXO exports.The addition of commitment hash and multisig flag to the UTXO export output is a valuable enhancement that helps users identify multisig outputs.
base_layer/wallet/src/output_manager_service/handle.rs (1)
1010-1019: Method implementation looks correctThe
fetch_unspent_outputs_from_nodemethod follows the established pattern for OutputManagerHandle methods with proper error handling and response matching.base_layer/wallet/src/output_manager_service/service.rs (3)
541-548: LGTM: Clean API extension for multisig support.The new request handlers follow the established pattern and provide proper error handling. The implementation correctly maps the request variants to their respective service methods and wraps responses appropriately.
2140-2140: LGTM: Appropriate visibility change for multisig integration.Making
confirm_encumberancepublic is necessary for the multisig automation workflow as shown in the relevant code snippets. The method remains simple and secure, only confirming encumbered outputs in the database.
3088-3119: LGTM: Robust implementation with proper error handling.The
fetch_unspent_outputs_from_nodemethod correctly:
- Makes RPC calls to fetch UTXOs by hash
- Handles conversion failures gracefully by logging warnings and continuing processing
- Returns valid outputs while skipping invalid ones
- Maintains good separation of concerns
The resilient error handling that continues processing other outputs instead of failing completely is a good design choice for production robustness.
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
107-107: Clarify the usage of payment_id.The comment
// ??suggests uncertainty about using the UTXO's payment_id. Please verify this is the correct payment_id to use or add clarification.
| # target | ||
| wallet*_data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-adding target into the context will massively bloat every build
With # target commented out, the whole Rust build tree (often > 1 GiB) is shipped to the Docker daemon on every build.
If you only need the single wallet binary, consider one of:
- Keep ignoring
targetand copy the binary from a dedicateddist/directory you export aftercargo build --release. - Use a multi-stage Dockerfile that builds the binary inside the image and then copies only the artifact into a tiny runtime stage.
-# target
+# keep build artifacts out of the context; the runtime stage can COPY from the builder stage
+target
+dist/This change will speed up local builds and CI dramatically while keeping images reproducible.
📝 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.
| # target | |
| wallet*_data | |
| # keep build artifacts out of the context; the runtime stage can COPY from the builder stage | |
| target | |
| dist/ | |
| wallet*_data |
🤖 Prompt for AI Agents
In the .dockerignore file around lines 2 to 3, the 'target' directory is
currently not ignored, causing the entire Rust build tree to be sent to the
Docker daemon and bloating builds. To fix this, uncomment or add 'target' to the
.dockerignore file to exclude it from the build context. Then, adjust your
Docker setup to either copy the binary from a separate 'dist/' directory after
building with 'cargo build --release' or implement a multi-stage Dockerfile that
builds the binary inside the image and copies only the final artifact to the
runtime stage.
Dockerfile
Outdated
| FROM ubuntu:24.04 | ||
| COPY ./target/debug/minotari_console_wallet /usr/local/bin/minotari_console_wallet | ||
|
|
||
| # Default command (can be overridden) | ||
| ENTRYPOINT ["/usr/local/bin/minotari_console_wallet"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ship a release-mode binary and trim the base image
The current image
- ships the debug binary (
target/debug/...) – ~3-4× larger and slower, - uses a full Ubuntu base – image size > 200 MB,
- relies on host compilation and requires
targetin build context.
A minimal, fully-reproducible alternative:
-FROM ubuntu:24.04
-COPY ./target/debug/minotari_console_wallet /usr/local/bin/minotari_console_wallet
+FROM rust:1.79-slim as builder
+WORKDIR /app
+# copy Cargo.toml & src to leverage docker cache more efficiently
+COPY Cargo.toml Cargo.lock .
+COPY applications/minotari_console_wallet ./applications/minotari_console_wallet
+RUN cargo build --release -p minotari_console_wallet
+
+FROM gcr.io/distroless/cc
+COPY --from=builder /app/target/release/minotari_console_wallet /usr/local/bin/minotari_console_walletBenefits: reproducible builds, smaller final image (~15 MB), no need to send target through the daemon.
At minimum, switch the COPY source to target/release so you don’t ship debug artefacts.
🤖 Prompt for AI Agents
In Dockerfile lines 1 to 5, the current image copies the debug binary from
target/debug, which is larger and slower. Change the COPY instruction to use the
release binary from target/release instead to ship a smaller, optimized binary.
Additionally, consider using a minimal base image like scratch or distroless to
reduce image size and avoid including unnecessary build context.
base_layer/core/src/transactions/transaction_key_manager/inner.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/commands.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/multisig/script.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/multisig/party.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/multisig/party.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/multisig/encumber.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/multisig/encumber.rs
Outdated
Show resolved
Hide resolved
Test Results (CI) 3 files 135 suites 51m 12s ⏱️ For more details on these failures, see this check. Results for commit 179fbb8. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 1 errors 1 suites +1 16m 2s ⏱️ + 16m 2s For more details on these parsing errors and failures, see this check. Results for commit 42dacc8. ± Comparison against base commit 4bbc859. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
158-596: Function complexity remains excessive despite past feedback.This function is still 439 lines long with complex nested logic, making it difficult to maintain and test. The previous review comment about refactoring this function into smaller, more manageable functions has not been addressed.
Consider extracting the following logical sections into separate helper functions:
- Output fetching and validation (lines 199-215)
- Script signature preparation (lines 217-280)
- Transaction building (lines 315-390)
- Receiver protocol handling (lines 391-510)
- Transaction finalization (lines 511-596)
This would improve readability, testability, and maintainability significantly.
🧹 Nitpick comments (1)
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
1-37: Clean up duplicate and redundant imports.The import section contains several duplicates and could be better organized:
-use tari_common_types::{ - key_branches::TransactionKeyManagerBranch, tari_address::TariAddress, transaction::{TransactionDirection, TransactionStatus, TxId}, types::{CompressedPublicKey, FixedHash, HashOutput, UncompressedCommitment, UncompressedPublicKey} -}; - -use tari_core::{borsh::SerializedSize, consensus::ConsensusConstants, covenants::Covenant, transactions::{fee::Fee, transaction_components::{payment_id::PaymentId, KernelFeatures, Transaction}, transaction_protocol::sender::TransactionSenderMessage, CryptoFactories, ReceiverTransactionProtocol, SenderTransactionProtocol}}; -use tari_core::transactions::transaction_components::RangeProofType; - -use tari_common_types::{ - types::{ - CompressedCommitment, - }, -}; - -use tari_core::transactions::{ - transaction_components::{ - OutputFeatures, - }, -}; +use tari_common_types::{ + key_branches::TransactionKeyManagerBranch, + tari_address::TariAddress, + transaction::{TransactionDirection, TransactionStatus, TxId}, + types::{CompressedCommitment, CompressedPublicKey, FixedHash, HashOutput, UncompressedCommitment, UncompressedPublicKey} +}; + +use tari_core::{ + borsh::SerializedSize, + consensus::ConsensusConstants, + covenants::Covenant, + transactions::{ + fee::Fee, + transaction_components::{OutputFeatures, payment_id::PaymentId, KernelFeatures, RangeProofType, Transaction}, + transaction_protocol::sender::TransactionSenderMessage, + CryptoFactories, ReceiverTransactionProtocol, SenderTransactionProtocol + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_console_wallet/src/automation/multisig/encumber.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/party.rs(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(4 hunks)base_layer/wallet/src/output_manager_service/service.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- base_layer/wallet/src/output_manager_service/service.rs
- base_layer/wallet/src/output_manager_service/handle.rs
- applications/minotari_console_wallet/src/automation/multisig/party.rs
🧰 Additional context used
🧠 Learnings (1)
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
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.
🔇 Additional comments (5)
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (5)
39-155: LGTM - Well-structured multisig collection function.The function properly handles loading multisig session configuration, iterates through participants while skipping the caller's own address, collects signatures and public keys, and processes each UTXO commitment. The error handling is appropriate with informative messages.
388-389: Method name consistency verifiedAll instances of the
confirm_encumbrancemethod are spelled correctly across the codebase. No occurrences of the incorrectconfirm_encumberancewere found.
563-584: Verified insert_completed_transaction implementationThe
insert_completed_transactionmethod exists in the transaction service and database layers:
- In
base_layer/wallet/src/transaction_service/handle.rsas
pub async fn insert_completed_transaction(&mut self, tx_id: TxId, transaction: CompletedTransaction) -> Result<TxId, TransactionStorageError>- In
base_layer/wallet/src/transaction_service/service.rs, which delegates to the database- In
base_layer/wallet/src/transaction_service/storage/database.rsas
pub fn insert_completed_transaction(&self, tx_id: TxId, transaction: CompletedTransaction) -> Result<Option<DbValue>, TransactionStorageError>Because these implementations accept a
CompletedTransactiongenerically, they already support multisig transactions without further changes. No action is required here.
404-423: Validate multisig Diffie-Hellman shared-secret aggregation in encumber.rsThe encumber flow in
applications/minotari_console_wallet/src/automation/multisig/encumber.rs
sums multiple public-key shares and then adds the self-share DH output before calling
CommsDHKE::from_canonical_bytes. This diverges from the single-party flow in
party.rs, which performs exactly one DH and immediately derives the encryption key.Please ensure this matches the multisig protocol spec:
• In encumber.rs, confirm that summing
dh_shared_secret_shares(lines 405–408) and then adding
get_diffie_hellman_shared_secret(…)(lines 410–423) produces the intended group shared secret.
• Compare against the single-signer pattern inparty.rs(lines 73–78) and any protocol documentation or test vectors.
• Verify the use ofUncompressedPublicKeyaddition andCommsDHKE::from_canonical_bytescorrectly implements threshold DH rather than an accidental point addition misuse.
224-280: Signature aggregation ordering verifiedI’ve confirmed that the
ephemeral_pubkeysvector in make_utxo_multisig (applications/minotari_console_wallet/src/automation/multisig/session.rs:36–53) is built in the same order as the CLI’spublic_keys, and thatget_multi_sig_script_components(script.rs) returns that exact ordering. In encumber.rs the loop overmulti_sig_public_keystherefore aligns signatures correctly, and the subsequentscript_signatures.len() == thresholdcheck matches the m-of-n requirement.No changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
applications/minotari_console_wallet/src/automation/multisig/script.rs (1)
65-97: Previous review comment still appliesThe function still has the issues mentioned in the previous review: it's marked as dead code, uses ascending sort instead of descending, and uses
eprintln!instead of proper logging.applications/minotari_console_wallet/src/automation/multisig/party.rs (1)
328-433: Previous review comment about logging and error handling still appliesThe function still uses
println!for warnings andeprintln!for errors instead of proper logging with thelogcrate. Critical failures should return errors rather than continuing.applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
198-627: Previous review comments still applyThe function still has the issues mentioned in previous reviews:
- At 439 lines, it's too complex and should be refactored into smaller functions
- The typo in
confirm_encumberance(line 423) should beconfirm_encumbrance
🧹 Nitpick comments (2)
applications/minotari_console_wallet/src/automation/multisig/script.rs (2)
111-111: Fix grammar in commentThe comment has a grammatical error.
-/// finalizes an already encumbered a n-of-m transaction +/// Finalizes an already encumbered n-of-m transaction
111-139: Remove unnecessary clone of transaction_serviceThe function receives ownership of
transaction_servicebut immediately clones it, which is unnecessary.pub async fn finalize_aggregate_utxo( - transaction_service: TransactionServiceHandle, + mut transaction_service: TransactionServiceHandle, tx_id: u64, meta_signatures: Vec<Signature>, script_signatures: Vec<Signature>, wallet_script_secret_key: PrivateKey, ) -> Result<TxId, CommandError> { - let mut transaction_service = transaction_service.clone();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
applications/minotari_console_wallet/src/automation/commands.rs(6 hunks)applications/minotari_console_wallet/src/automation/mod.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/encumber.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/io.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/mod.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/party.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/script.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/session.rs(1 hunks)applications/minotari_console_wallet/src/automation/multisig/types.rs(1 hunks)applications/minotari_console_wallet/src/cli.rs(3 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(2 hunks)base_layer/wallet/src/output_manager_service/handle.rs(4 hunks)base_layer/wallet/src/output_manager_service/service.rs(11 hunks)base_layer/wallet/src/transaction_service/handle.rs(4 hunks)base_layer/wallet/src/transaction_service/service.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/minotari_console_wallet/src/automation/multisig/mod.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- applications/minotari_console_wallet/src/automation/mod.rs
- base_layer/core/src/transactions/transaction_key_manager/wrapper.rs
- base_layer/core/src/transactions/transaction_key_manager/interface.rs
- base_layer/wallet/src/transaction_service/service.rs
- base_layer/wallet/src/transaction_service/handle.rs
- base_layer/core/src/transactions/transaction_key_manager/inner.rs
- applications/minotari_console_wallet/src/automation/multisig/io.rs
- base_layer/wallet/src/output_manager_service/handle.rs
- base_layer/wallet/src/output_manager_service/service.rs
- applications/minotari_console_wallet/src/automation/multisig/session.rs
- applications/minotari_console_wallet/src/cli.rs
- applications/minotari_console_wallet/src/automation/multisig/types.rs
- applications/minotari_console_wallet/src/automation/commands.rs
🧰 Additional context used
🧠 Learnings (3)
applications/minotari_console_wallet/src/automation/multisig/party.rs (1)
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.
applications/minotari_console_wallet/src/automation/multisig/script.rs (2)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
applications/minotari_console_wallet/src/automation/multisig/encumber.rs (1)
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.
🧬 Code Graph Analysis (1)
applications/minotari_console_wallet/src/automation/multisig/script.rs (5)
base_layer/core/src/common/one_sided.rs (1)
public_key_to_output_encryption_key(71-78)base_layer/wallet/src/output_manager_service/service.rs (2)
get_multi_sig_script_components(3557-3569)new(143-200)applications/minotari_ledger_wallet/wallet/src/crypto/keys.rs (1)
new_from_pk(306-309)base_layer/wallet/src/output_manager_service/handle.rs (1)
new(401-409)base_layer/wallet/src/transaction_service/handle.rs (2)
new(664-672)finalize_aggregate_utxo(984-1004)
🔇 Additional comments (3)
applications/minotari_console_wallet/src/automation/multisig/script.rs (2)
22-40: Well-implemented multisig script handling functionsBoth
is_multisig_utxoandget_multi_sig_script_componentsare correctly implemented. The component extraction function properly iterates through all opcodes rather than checking at a fixed index, making it more robust.
42-59: Correct implementation of public key aggregation functionsBoth
sum_public_keysandsum_public_keys_to_encryption_keycorrectly implement the cryptographic operations. The encryption key derivation properly uses theWalletOutputEncryptionKeysDomainHasherfor domain separation.applications/minotari_console_wallet/src/automation/multisig/party.rs (1)
61-167: Well-implemented party member output creationThe function correctly handles key validation, ephemeral key derivation, signature generation, and shared secret computation. The error handling is appropriate and the cryptographic operations follow the expected patterns.
applications/minotari_console_wallet/src/automation/multisig/party.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/automation/multisig/encumber.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
base_layer/wallet/src/output_manager_service/handle.rs (4)
167-168: Fix spelling error: "Encumberance" should be "Encumbrance".This is consistent with the past review comments. The correct spelling is "Encumbrance".
FetchUnspentOutputs(Vec<HashOutput>), - ConfirmEncumberance(TxId, Vec<WalletOutput>), + ConfirmEncumbrance(TxId, Vec<WalletOutput>),
301-303: Fix spelling error in Display implementation.The spelling error "Encumberance" needs to be corrected here as well.
- ConfirmEncumberance(tx_id, change_outputs) => { - write!(f, "ConfirmEncumberance: {}, {:?}", tx_id, change_outputs) + ConfirmEncumbrance(tx_id, change_outputs) => { + write!(f, "ConfirmEncumbrance: {}, {:?}", tx_id, change_outputs)
362-363: Fix spelling error in response enum.The spelling error "Encumberance" needs to be corrected in the response enum as well.
FetchUnspentOutputs(Vec<TransactionOutput>), - ConfirmEncumberance, + ConfirmEncumbrance,
1080-1090: Fix method to use corrected spelling and improve error handling.The method should use the corrected spelling and follow the consistent pattern of other handle methods with proper response matching.
- pub async fn confirm_encumbrance( - &mut self, - tx_id: TxId, - change_outputs: Vec<WalletOutput>, - ) -> Result<(), OutputManagerError> { - self.handle - .call(OutputManagerRequest::ConfirmEncumberance(tx_id, change_outputs)) - .await??; - - Ok(()) - } + pub async fn confirm_encumbrance( + &mut self, + tx_id: TxId, + change_outputs: Vec<WalletOutput>, + ) -> Result<(), OutputManagerError> { + match self + .handle + .call(OutputManagerRequest::ConfirmEncumbrance(tx_id, change_outputs)) + .await?? + { + OutputManagerResponse::ConfirmEncumbrance => Ok(()), + _ => Err(OutputManagerError::UnexpectedApiResponse), + } + }
🧹 Nitpick comments (4)
clients/rust/base_node_wallet_client/src/client/http.rs (1)
84-91: Consider removing console output in favor of logging.The improved error handling is beneficial, but the
println!on line 88 duplicates the logging on line 87. Consider using only the logging mechanism for consistency with the rest of the codebase.- println!("HTTP request to {} failed: {:?}", url, e); // Console outputapplications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
37-37: Split long import line for better readability.The import line is excessively long and difficult to read. Consider splitting it into multiple lines for better maintainability.
Apply this diff to improve readability:
-use minotari_app_grpc::tari_rpc::{ - self, payment_recipient::PaymentType, wallet_server, BroadcastSignedOneSidedTransactionRequest, BroadcastSignedOneSidedTransactionResponse, CheckConnectivityResponse, ClaimHtlcRefundRequest, ClaimHtlcRefundResponse, ClaimShaAtomicSwapRequest, ClaimShaAtomicSwapResponse, CoinSplitRequest, CoinSplitResponse, CommitmentSignature, CreateBurnTransactionRequest, CreateBurnTransactionResponse, CreateMultisigUtxoRequest, CreateMultisigUtxoResponse, CreateTemplateRegistrationRequest, CreateTemplateRegistrationResponse, FeePerGramStat, GetAddressResponse, GetAllCompletedTransactionsRequest, GetAllCompletedTransactionsResponse, GetBalanceRequest, GetBalanceResponse, GetBlockHeightTransactionsRequest, GetBlockHeightTransactionsResponse, GetCompleteAddressResponse, GetCompletedTransactionsRequest, GetCompletedTransactionsResponse, GetConnectivityRequest, GetFeeEstimateRequest, GetFeeEstimateResponse, GetFeePerGramStatsRequest, GetFeePerGramStatsResponse, GetIdentityRequest, GetIdentityResponse, GetPaymentByReferenceRequest, GetPaymentByReferenceResponse, GetPaymentIdAddressRequest, GetStateRequest, GetStateResponse, GetTransactionInfoRequest, GetTransactionInfoResponse, GetTransactionPayRefsRequest, GetTransactionPayRefsResponse, GetUnspentAmountsResponse, GetVersionRequest, GetVersionResponse, ImportTransactionsRequest, ImportTransactionsResponse, ImportUtxosRequest, ImportUtxosResponse, PrepareOneSidedTransactionForSigningRequest, PrepareOneSidedTransactionForSigningResponse, RegisterValidatorNodeRequest, RegisterValidatorNodeResponse, RevalidateRequest, RevalidateResponse, SendShaAtomicSwapRequest, SendShaAtomicSwapResponse, StartMultisigUtxoTransactionRequest, StartMultisigUtxoTransactionResponse, SubmitValidatorEvictionProofRequest, SubmitValidatorEvictionProofResponse, SubmitValidatorNodeExitRequest, SubmitValidatorNodeExitResponse, TransactionDirection, TransactionEvent, TransactionEventRequest, TransactionEventResponse, TransactionInfo, TransactionStatus, TransferRequest, TransferResponse, TransferResult, ValidateRequest, ValidateResponse -}; +use minotari_app_grpc::tari_rpc::{ + self, payment_recipient::PaymentType, wallet_server, + BroadcastSignedOneSidedTransactionRequest, BroadcastSignedOneSidedTransactionResponse, + CheckConnectivityResponse, ClaimHtlcRefundRequest, ClaimHtlcRefundResponse, + ClaimShaAtomicSwapRequest, ClaimShaAtomicSwapResponse, CoinSplitRequest, CoinSplitResponse, + CommitmentSignature, CreateBurnTransactionRequest, CreateBurnTransactionResponse, + CreateMultisigUtxoRequest, CreateMultisigUtxoResponse, CreateTemplateRegistrationRequest, + CreateTemplateRegistrationResponse, FeePerGramStat, GetAddressResponse, + GetAllCompletedTransactionsRequest, GetAllCompletedTransactionsResponse, GetBalanceRequest, + GetBalanceResponse, GetBlockHeightTransactionsRequest, GetBlockHeightTransactionsResponse, + GetCompleteAddressResponse, GetCompletedTransactionsRequest, GetCompletedTransactionsResponse, + GetConnectivityRequest, GetFeeEstimateRequest, GetFeeEstimateResponse, + GetFeePerGramStatsRequest, GetFeePerGramStatsResponse, GetIdentityRequest, + GetIdentityResponse, GetPaymentByReferenceRequest, GetPaymentByReferenceResponse, + GetPaymentIdAddressRequest, GetStateRequest, GetStateResponse, GetTransactionInfoRequest, + GetTransactionInfoResponse, GetTransactionPayRefsRequest, GetTransactionPayRefsResponse, + GetUnspentAmountsResponse, GetVersionRequest, GetVersionResponse, ImportTransactionsRequest, + ImportTransactionsResponse, ImportUtxosRequest, ImportUtxosResponse, + PrepareOneSidedTransactionForSigningRequest, PrepareOneSidedTransactionForSigningResponse, + RegisterValidatorNodeRequest, RegisterValidatorNodeResponse, RevalidateRequest, + RevalidateResponse, SendShaAtomicSwapRequest, SendShaAtomicSwapResponse, + StartMultisigUtxoTransactionRequest, StartMultisigUtxoTransactionResponse, + SubmitValidatorEvictionProofRequest, SubmitValidatorEvictionProofResponse, + SubmitValidatorNodeExitRequest, SubmitValidatorNodeExitResponse, TransactionDirection, + TransactionEvent, TransactionEventRequest, TransactionEventResponse, TransactionInfo, + TransactionStatus, TransferRequest, TransferResponse, TransferResult, ValidateRequest, + ValidateResponse, +};applications/minotari_app_grpc/proto/wallet.proto (1)
1110-1116: Clean up commented-out RPC methods.The commented-out RPC methods should either be implemented or removed to keep the proto file clean and avoid confusion.
rpc CreateMultisigUtxo (CreateMultisigUtxoRequest) returns (CreateMultisigUtxoResponse); rpc StartMultisigUtxoTransaction (StartMultisigUtxoTransactionRequest) returns (StartMultisigUtxoTransactionResponse); - // rpc CreateMultisigUtxoTransferMember (CreateMultisigUtxoTransferMemberRequest) returns (CreateMultisigUtxoTransferMemberResponse); - // rpc CollectMultisigUtxoEncumber (CollectMultisigUtxoEncumberRequest) returns (CollectMultisigUtxoEncumberResponse); - // rpc SignMultisigUtxoMember (SignMultisigUtxoMemberRequest) returns (SignMultisigUtxoMemberResponse); - // rpc SendMultisigUtxoLeader (SendMultisigUtxoLeaderRequest) returns (SendMultisigUtxoLeaderResponse);base_layer/wallet/src/output_manager_service/service.rs (1)
1184-1194: Remove debug print statement.The
println!statement should be replaced with proper logging for production code.- println!("Total value {}", total_value); + debug!(target: LOG_TARGET, "Total value: {}", total_value);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapplications/minotari_app_grpc/proto/descriptor.binis excluded by!**/*.bin
📒 Files selected for processing (19)
applications/minotari_app_grpc/build.rs(1 hunks)applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_app_grpc/src/lib.rs(1 hunks)applications/minotari_app_utilities/src/utilities.rs(2 hunks)applications/minotari_console_wallet/Cargo.toml(2 hunks)applications/minotari_console_wallet/src/automation/commands.rs(7 hunks)applications/minotari_console_wallet/src/automation/multisig/session.rs(1 hunks)applications/minotari_console_wallet/src/cli.rs(3 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(6 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(4 hunks)applications/minotari_node/src/grpc_method.rs(4 hunks)base_layer/common_types/src/transaction.rs(1 hunks)base_layer/wallet/src/output_manager_service/handle.rs(10 hunks)base_layer/wallet/src/output_manager_service/service.rs(22 hunks)base_layer/wallet/src/output_manager_service/storage/output_source.rs(2 hunks)base_layer/wallet/src/transaction_service/handle.rs(4 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(1 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(1 hunks)docker-compose.yml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- base_layer/common_types/src/transaction.rs
- applications/minotari_console_wallet/Cargo.toml
- applications/minotari_app_grpc/src/lib.rs
- applications/minotari_app_grpc/build.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- docker-compose.yml
- applications/minotari_app_utilities/src/utilities.rs
- applications/minotari_console_wallet/src/wallet_modes.rs
- base_layer/wallet/src/transaction_service/handle.rs
- applications/minotari_console_wallet/src/automation/multisig/session.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- applications/minotari_console_wallet/src/cli.rs
🧰 Additional context used
🧠 Learnings (7)
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
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.
clients/rust/base_node_wallet_client/src/client/http.rs (2)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/wallet/src/output_manager_service/handle.rs (7)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy.
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: 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.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/wallet/src/output_manager_service/storage/output_source.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
applications/minotari_app_grpc/proto/wallet.proto (1)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
base_layer/wallet/src/output_manager_service/service.rs (3)
undefined
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
🔇 Additional comments (27)
base_layer/wallet/src/output_manager_service/storage/output_source.rs (2)
45-45: LGTM! New Multisig variant properly added.The addition of the
Multisigvariant is correctly implemented and aligns with the multisig functionality being introduced.
66-69: LGTM! TryFrom implementation correctly updated.The integer mapping for the new
Multisigvariant is properly implemented with sequential numbering (13), and the error message range was correctly updated to reflect the new valid range.base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (1)
642-657: LGTM! Multisig scanning functionality properly implemented.The multisig scanning follows the same pattern as the existing recoverable outputs and one-sided payments scanning. The use of
ImportStatus::Importedfor multisig outputs is appropriate, and the error handling with output hash matching is correctly implemented.applications/minotari_node/src/grpc_method.rs (5)
71-72: LGTM! New multisig gRPC method variants properly added.The addition of
CreateMultisigUtxoandStartMultisigUtxoTransactionvariants is correctly implemented and aligns with the multisig functionality being introduced.
77-119: LGTM! ALL_VARIANTS array correctly updated.The array size was properly updated from 39 to 41, and the new variants were correctly added to the array. The implementation maintains consistency with the existing pattern.
123-123: LGTM! IntoIterator type correctly updated.The iterator type was properly updated to reflect the new array size of 41.
178-179: LGTM! FromStr implementation correctly extended.The string parsing for the new variants follows the established snake_case pattern and is correctly implemented.
279-280: LGTM! Exhaustive test correctly updated.The test was properly updated to include the new variants, ensuring that all enum variants are covered by the exhaustive test.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)
667-668: Good use of explicit trait method calls.The explicit calls to
TransactionResult::to_jsonandTransactionResult::from_jsonimprove code clarity.Also applies to: 698-699
2115-2116: Add validation for input parameters.The method should validate the input parameters before processing.
Apply this diff to add proper validation:
let message = request.into_inner(); + if message.m == 0 { + return Err(Status::invalid_argument( + "m must be greater than 0".to_string(), + )); + } + + if message.public_keys.is_empty() { + return Err(Status::invalid_argument( + "public keys list cannot be empty".to_string(), + )); + } + + if (message.m as usize) > message.public_keys.len() { + return Err(Status::invalid_argument( + "m must be less than or equal to the number of public keys".to_string(), + )); + } + let mut output_service = self.wallet.output_manager_service.clone();⛔ Skipped due to learnings
Learnt from: ksrichard PR: tari-project/tari#7129 File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29 Timestamp: 2025-05-29T20:59:32.020Z Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
2117-2117: Add explicit validation formandpublic_keys(keepmutonoutput_service)The
mutbinding onoutput_serviceis required becausecreate_multisig_outputtakes&mut OutputManagerHandle. However, we should validate the incoming parameters before calling into the session:• After parsing
public_keys:let public_keys = /* …collect as before… */; if public_keys.is_empty() { return Err(Status::invalid_argument( "At least one public key must be provided".into(), )); }• Before invoking
create_multisig_output, ensuremis within bounds:let m = message.m as usize; if m == 0 || m > public_keys.len() { return Err(Status::invalid_argument(format!( "Invalid threshold m: must be between 1 and {}", public_keys.len() ))); }These checks will yield clear
INVALID_ARGUMENTerrors instead of panics or opaque internal failures.⛔ Skipped due to learnings
Learnt from: martinserts PR: tari-project/tari#7122 File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0 Timestamp: 2025-06-16T14:49:31.831Z Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.Learnt from: ksrichard PR: tari-project/tari#7129 File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29 Timestamp: 2025-05-29T20:59:32.020Z Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.applications/minotari_app_grpc/proto/wallet.proto (1)
1123-1161: LGTM! Multisig message definitions are well-structured.The new multisig message types follow proper protobuf conventions with appropriate field types, numbering, and clear naming. The separation of concerns between creating multisig UTXOs and starting multisig transactions is logical.
base_layer/wallet/src/output_manager_service/handle.rs (7)
119-125: LGTM! New request variant follows existing patterns.The
CreatePayToAddressContainingOutputsrequest variant is well-structured and follows the established pattern of other output manager requests.
156-156: LGTM! Multisig scanning request added.The new
ScanOutputsForMultisigrequest variant appropriately extends the scanning capabilities for multisig outputs.
345-345: LGTM! Response variant for multisig scanning added.The new response variant appropriately matches the request for multisig output scanning.
354-357: LGTM! Response variant follows existing patterns.The
CreatePayToAddressContainingOutputsresponse variant properly matches its corresponding request and follows the established pattern.
851-859: LGTM! Multisig scanning method implemented correctly.The
scan_outputs_for_multisigmethod follows the established pattern of other scanning methods with proper error handling and response matching.
894-916: LGTM! New method follows established patterns.The
create_pay_to_address_containing_outputsmethod is well-implemented with proper error handling and response matching, consistent with other handle methods.
1066-1078: LGTM! Node output fetching method implemented correctly.The
fetch_unspent_outputs_from_nodemethod follows the established pattern with proper error handling and response matching.base_layer/wallet/src/output_manager_service/service.rs (8)
404-404: LGTM: Typo fix applied correctly.The method name has been corrected from
confirm_encumberancetoconfirm_encumbranceto fix the spelling error.
498-501: LGTM: New multisig scanning request handling added.The new
ScanOutputsForMultisigrequest variant is properly handled and maps to the corresponding response type.
528-542: LGTM: New pay-to-address transaction creation request added.The request handling follows the established pattern and properly calls the new
create_pay_to_address_containing_outputsmethod with appropriate error handling.
556-564: LGTM: New fetch unspent outputs request handling.The implementation correctly iterates through hash list and accumulates found outputs, with proper error propagation.
565-568: LGTM: New confirm encumbrance request handling added.The request properly delegates to the corrected
confirm_encumbrancemethod with appropriate response mapping.
1184-1356: LGTM: Well-structured transaction creation refactoring.The refactoring introduces a clean separation between self-pay and pay-to-address functionality by extracting common logic into
create_transaction_with_outputs_internal. The optional recipient address parameter enables code reuse while maintaining clear API boundaries.
2182-2182: Method name correctly updated.The method signature has been properly updated to use the corrected spelling
confirm_encumbrance.
3151-3162: LGTM: Method visibility and naming updated appropriately.The method has been correctly renamed from
fetch_utxo_from_nodetofetch_unspent_outputs_from_nodeand made public to support the new request handling pattern.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
| async fn scan_outputs_for_multisig( | ||
| &mut self, | ||
| outputs: Vec<(TransactionOutput, Option<TxId>)>, | ||
| ) -> Result<Vec<RecoveredOutput>, OutputManagerError> { | ||
| // 1. Get all your wallet's public keys (or just the spend key for now) | ||
| let my_pubkey = self.resources.key_manager.get_spend_key().await?.pub_key; | ||
| let mut scanned_outputs = vec![]; | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "scan_outputs_for_multisig test" | ||
| ); | ||
| for (output, tx_id) in outputs { | ||
| // 2. Check if the script is a multisig script | ||
| if let Some(Opcode::CheckMultiSigVerifyAggregatePubKey(_m, _n, pubkeys, _msg)) = | ||
| output.script.as_slice().iter().find(|op| matches!(op, Opcode::CheckMultiSigVerifyAggregatePubKey(..))) | ||
| { | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "Found multisig script in output with tx_id: {:?}, pubkeys: {:?}", | ||
| tx_id, | ||
| pubkeys | ||
| ); | ||
| // 3. Check if any of the multisig pubkeys match your wallet's pubkey | ||
| if pubkeys.iter().any(|pk| pk == &my_pubkey) { | ||
| // 4. Try to derive the decryption key (sum of pubkeys, or your multisig recovery logic) | ||
| let mut sum_pubkeys = UncompressedPublicKey::default(); | ||
| for pk in pubkeys { | ||
| sum_pubkeys = &sum_pubkeys + pk.to_public_key()?; | ||
| } | ||
|
|
||
| debug!( | ||
| target: LOG_TARGET, | ||
| "It is mine: {:?}, pubkeys: {:?}", | ||
| tx_id, | ||
| pubkeys | ||
| ); | ||
| let encryption_key = public_key_to_output_encryption_key(&CompressedPublicKey::new_from_pk(sum_pubkeys))?; | ||
|
|
||
| // 5. Try to decrypt the output | ||
| if let Ok((committed_value, spending_key, payment_id)) = | ||
| EncryptedData::decrypt_data(&encryption_key, &output.commitment, &output.encrypted_data) | ||
| { | ||
| if output.verify_mask( | ||
| &self.resources.factories.range_proof, | ||
| &spending_key, | ||
| committed_value.into(), | ||
| )? { | ||
| let spending_key_id = self.resources.key_manager.import_key(spending_key).await?; | ||
| let rewound_output = WalletOutput::new_with_rangeproof( | ||
| output.version, | ||
| committed_value, | ||
| spending_key_id, | ||
| output.features, | ||
| output.script, | ||
| ExecutionStack::new(vec![]), | ||
| TariKeyId::default(), // or derive as needed | ||
| output.sender_offset_public_key, | ||
| output.metadata_signature, | ||
| 0, | ||
| output.covenant, | ||
| output.encrypted_data, | ||
| output.minimum_value_promise, | ||
| output.proof, | ||
| payment_id, | ||
| ); | ||
| scanned_outputs.push((rewound_output, OutputSource::Multisig, tx_id)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| self.import_onesided_outputs(scanned_outputs).await | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify multisig scanning security and add error handling improvements.
The multisig scanning logic appears sound but has some areas for improvement:
-
Security consideration: The encryption key derivation from sum of public keys follows a deterministic pattern which is good for recovery but ensure this aligns with the multisig protocol specification.
-
Error handling: The method doesn't handle potential errors from
to_public_key()conversions gracefully. -
Script key derivation: Using
TariKeyId::default()for the script key might not be appropriate for all multisig scenarios.
🏁 Script executed:
#!/bin/bash
# Verify multisig scanning is used consistently across the codebase
rg -A 5 "scan_outputs_for_multisig" --type rustLength of output: 3543
Action Required: Improve error handling and script‐key derivation in multisig scanning
I’ve verified that scan_outputs_for_multisig is only defined in
base_layer/wallet/src/output_manager_service/service.rs and invoked via the HTTP handler and UTXO scanner, so usage is consistent. Before merging, please address the following:
• Handle to_public_key() failures explicitly rather than using ? inside the loop.
– In service.rs around the sum‐of‐pubkeys logic, map conversion errors into a clear OutputManagerError so you don’t abort with a generic crypto error.
• Replace TariKeyId::default() when constructing the rewound WalletOutput.
– Generate or import a proper key ID (e.g., via key_manager.issue_key_id() or a dedicated derivation for multisig scripts) so each output’s TariKeyId matches its recovery logic.
• Confirm that summing all pubkeys (instead of using an aggregate script key) matches our multisig protocol spec.
– If the spec uses a different aggregate or tweak, update the derivation here to align.
Files to update:
- base_layer/wallet/src/output_manager_service/service.rs (sum-pubkeys loop +
WalletOutput::new_with_rangeproof)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
base_layer/wallet/src/output_manager_service/service.rs (1)
3510-3582: Critical security and implementation issues in multisig scanning.The multisig scanning implementation has several significant issues that need to be addressed:
Improper script key derivation: Using
TariKeyId::default()(line 3565) is inappropriate for multisig outputs and could lead to key management issues.Inadequate error handling: The
to_public_key()conversions (lines 3537, 3546) use?which could cause the entire scan to fail on a single invalid key.Security concern: The deterministic encryption key derivation from sum of public keys (lines 3535-3538) needs verification against the multisig protocol specification.
Missing validation: No verification that the decrypted output actually belongs to this wallet beyond checking if the spend key is in the multisig set.
Please address these issues:
// 4. Try to derive the decryption key (sum of pubkeys, or your multisig recovery logic) let mut sum_pubkeys = UncompressedPublicKey::default(); for pk in pubkeys { - sum_pubkeys = &sum_pubkeys + pk.to_public_key()?; + sum_pubkeys = &sum_pubkeys + pk.to_public_key() + .map_err(|e| OutputManagerError::ServiceError( + format!("Invalid public key in multisig script: {}", e) + ))?; }let rewound_output = WalletOutput::new_with_rangeproof( output.version, committed_value, spending_key_id, output.features, output.script, ExecutionStack::new(vec![]), - TariKeyId::default(), // or derive as needed + self.resources.key_manager.get_spend_key().await?.key_id, output.sender_offset_public_key, output.metadata_signature, 0, output.covenant, output.encrypted_data, output.minimum_value_promise, output.proof, payment_id, );
🧹 Nitpick comments (2)
base_layer/wallet/src/output_manager_service/service.rs (2)
1184-1352: Excellent refactoring: Transaction creation logic generalized effectively.The refactoring successfully generalizes the transaction creation logic by:
- Extracting common functionality into
create_transaction_with_outputs_internal- Supporting both self-pay and address-based transactions via optional recipient address
- Maintaining backward compatibility with existing methods
- Following good separation of concerns
The commented-out code (lines 1230-1241) should be removed if it's no longer needed.
Consider removing the commented-out code block:
- // if let Some(addr) = &recipient_address { - // builder - // .with_recipient_data( - // push_pubkey_script(addr.public_spend_key()), - // Default::default(), - // Covenant::default(), - // MicroMinotari(0), - // MicroMinotari(0), - // addr.clone(), - // ) - // .await?; - // }
1194-1194: Remove debug print statement.The debug print statement should be removed from production code.
- println!("Total value {}", total_value);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(7 hunks)base_layer/wallet/src/output_manager_service/service.rs(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- applications/minotari_console_wallet/src/automation/commands.rs
🧰 Additional context used
🧠 Learnings (1)
base_layer/wallet/src/output_manager_service/service.rs (5)
undefined
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.657Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via get_last_scanned_height() rather than persisting it themselves. This follows separation of concerns architecture pattern.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using .unwrap() on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
🔇 Additional comments (6)
base_layer/wallet/src/output_manager_service/service.rs (6)
404-404: LGTM: Spelling correction applied consistently.The method name has been corrected from
confirm_encumberancetoconfirm_encumbranceto fix the spelling error.
498-501: LGTM: New multisig scanning functionality added.The new request handler properly delegates to the
scan_outputs_for_multisigmethod and follows the established pattern for output scanning operations.
528-542: LGTM: New address-based transaction creation functionality.The new request handler follows the established pattern and properly delegates to the
create_pay_to_address_containing_outputsmethod.
556-568: LGTM: New request handlers follow established patterns.Both new request handlers (
FetchUnspentOutputsandConfirmEncumberance) properly implement the expected functionality and follow the established error handling patterns.
2179-2179: LGTM: Method name spelling corrected.The method name has been corrected from
confirm_encumberancetoconfirm_encumbrancefor consistency.
3148-3159: LGTM: Method renamed for clarity and made public.The method has been appropriately renamed from
fetch_utxo_from_nodetofetch_unspent_outputs_from_nodefor better clarity and made public to support the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (3)
base_layer/wallet/src/transaction_service/service.rs (1)
1117-1120: This handler still lacks validation and event publishing.This is the same issue identified in the previous review. The
InsertCompletedTransactionhandler is missing transaction size validation and event publishing that are present in theImportTransactionhandler.applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
2060-2064: Add validation for m parameter.The method should validate that
mis greater than 0 and that the public keys list is not empty.
2058-2065: Remove commented-out code.Dead code should be removed to maintain code cleanliness.
🧹 Nitpick comments (13)
Dockerfile (1)
1-1: Consider using a minimal base image to reduce image size.While the use of
target/release(instead of debug) is good, the full Ubuntu base image results in a larger final image. Consider using a minimal base image likegcr.io/distroless/ccorscratchto significantly reduce the image size from ~200MB to ~15MB.Apply this diff for a minimal distroless approach:
-FROM ubuntu:24.04 +FROM gcr.io/distroless/cc COPY ./target/release/minotari_console_wallet /usr/local/bin/minotari_console_wallet - -RUN apt-get update && apt-get install -y ca-certificates - -# Default command (can be overridden) ENTRYPOINT ["/usr/local/bin/minotari_console_wallet"]Note: Distroless images include ca-certificates by default, eliminating the need for the apt-get command.
base_layer/wallet/src/transaction_service/multisig/script.rs (1)
59-61: Optimize commitment comparison by avoiding hex conversion.Converting to hex strings for comparison is inefficient. Consider comparing the commitment bytes directly or accepting a typed commitment parameter instead of a string.
Apply this diff to improve performance:
-pub fn get_utxo_by_commitment_hash(utxos: &[DbWalletOutput], commitment_hash: String) -> Option<&DbWalletOutput> { - utxos.iter().find(|utxo| utxo.commitment.to_hex() == commitment_hash) +pub fn get_utxo_by_commitment_hash(utxos: &[DbWalletOutput], commitment: &CompressedCommitment) -> Option<&DbWalletOutput> { + utxos.iter().find(|utxo| &utxo.commitment == commitment) }If you need to keep the string parameter for API compatibility, consider parsing it once:
pub fn get_utxo_by_commitment_hash(utxos: &[DbWalletOutput], commitment_hash: String) -> Option<&DbWalletOutput> { - utxos.iter().find(|utxo| utxo.commitment.to_hex() == commitment_hash) + let commitment_hash_lower = commitment_hash.to_lowercase(); + utxos.iter().find(|utxo| utxo.commitment.to_hex().to_lowercase() == commitment_hash_lower) }base_layer/wallet/src/transaction_service/handle.rs (3)
1507-1508: Remove unnecessaryreturnstatements.The explicit
returnstatements are not needed when they're the last expression in a block.Apply this diff to simplify the code:
- _ => { - return Err(TransactionServiceError::UnexpectedApiResponse); - } + _ => Err(TransactionServiceError::UnexpectedApiResponse),This change should be applied to all similar occurrences in the multisig handle methods.
Also applies to: 1532-1533, 1549-1550, 1566-1567, 1583-1584, 1599-1600
1539-1541: Remove unnecessary blank line.There's an extra blank line that doesn't match the formatting pattern of other methods.
Apply this diff:
pub async fn generate_multisig_member_secrets( &mut self, session_id: String ) -> Result<MultisigGenerateMemberSecretsOutput, TransactionServiceError> { -
1554-1557: Add missing comma after parameter.The
session_idparameter is missing a trailing comma, which is inconsistent with Rust formatting conventions.Apply this diff to all three methods:
- session_id: String + session_id: String,Also applies to: 1571-1574, 1588-1591
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
2054-2054: Add documentation for new multisig methods.Please add documentation comments explaining the purpose, parameters, and return values for each new multisig method to improve code maintainability.
Example for
create_multisig_utxo:+ /// Creates a new multisignature UTXO with the specified parameters. + /// + /// # Arguments + /// * `m` - The minimum number of signatures required to spend the UTXO + /// * `public_keys` - The public keys of all parties involved in the multisig + /// * `recipient_address` - The address that will receive the multisig UTXO + /// * `amount` - The amount in MicroMinotari to lock in the multisig UTXO async fn create_multisig_utxo(Also applies to: 2092-2092, 2131-2131, 2164-2164, 2203-2203, 2230-2230
base_layer/wallet/src/transaction_service/multisig/types.rs (3)
4-4: Clean up import formatting.The import has unnecessary nested braces.
-use tari_crypto::{ristretto::{RistrettoSecretKey}}; +use tari_crypto::ristretto::RistrettoSecretKey;
120-125: Add missing derives for consistency.
AggregateUtxoDatais missing the standard derives that other structs have. If this struct needs to be serialized/deserialized, add the appropriate derives.+#[derive(Debug, Clone, Serialize, Deserialize)] pub struct AggregateUtxoData { pub tx_id: TxId, pub metadata_signatures: Vec<Signature>, pub script_signatures: Vec<Signature>, pub offset: PrivateKey, }
126-126: Add newline at end of file.Files should end with a newline character for consistency with Unix conventions.
Add a newline after the closing brace of
AggregateUtxoData.base_layer/wallet/src/transaction_service/multisig/session.rs (4)
5-5: Break up long import line for better readability.This line is too long and should be split across multiple lines.
Apply this formatting improvement:
- key_branches::TransactionKeyManagerBranch, tari_address::TariAddress, transaction::{TransactionDirection, TransactionStatus, TxId}, types::{CompressedCommitment, CompressedPublicKey, FixedHash, HashOutput, PrivateKey, Signature, UncompressedCommitment, UncompressedPublicKey} + key_branches::TransactionKeyManagerBranch, + tari_address::TariAddress, + transaction::{TransactionDirection, TransactionStatus, TxId}, + types::{ + CompressedCommitment, CompressedPublicKey, FixedHash, HashOutput, + PrivateKey, Signature, UncompressedCommitment, UncompressedPublicKey + }
98-98: Remove commented code.This commented line should be removed if it's no longer needed.
- // let payment_id = PaymentId::default();
814-816: Remove commented code.These commented lines should be removed.
- // transaction_service - // .insert_completed_transaction(tx_id, completed_tx) - // .await - // .map_err(|e| TransactionServiceError::Other(format!("Failed to insert completed transaction: {}", e)))?;
977-977: Remove unnecessary explicit return.The explicit return statement is not needed here.
- return Ok(output_signatures); + Ok(output_signatures)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapplications/minotari_app_grpc/proto/descriptor.binis excluded by!**/*.bin
📒 Files selected for processing (16)
Dockerfile(1 hunks)applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(6 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(7 hunks)applications/minotari_node/src/grpc_method.rs(4 hunks)base_layer/wallet/Cargo.toml(1 hunks)base_layer/wallet/src/output_manager_service/service.rs(24 hunks)base_layer/wallet/src/transaction_service/handle.rs(7 hunks)base_layer/wallet/src/transaction_service/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/io.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/script.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/session.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/types.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(4 hunks)docker-compose.yml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- base_layer/wallet/src/transaction_service/mod.rs
- base_layer/wallet/Cargo.toml
- base_layer/wallet/src/transaction_service/multisig/mod.rs
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/minotari_console_wallet/src/automation/commands.rs
- applications/minotari_node/src/grpc_method.rs
- base_layer/wallet/src/output_manager_service/service.rs
- applications/minotari_app_grpc/proto/wallet.proto
🧰 Additional context used
🧠 Learnings (8)
Dockerfile (2)
Learnt from: leet4tari
PR: tari-project/tari#7206
File: .github/workflows/build_dockers_workflow.yml:306-316
Timestamp: 2025-06-18T07:33:35.297Z
Learning: In the Tari project's Docker CI workflow (.github/workflows/build_dockers_workflow.yml), the multi-arch-create-manifest job creates multiple Docker manifests, but only the first basic version tag manifest is critical for workflow success. The other manifest operations (latest alias, release tags, etc.) are non-critical and use `|| true` to prevent workflow failure, which is the intended design.
Learnt from: leet4tari
PR: tari-project/tari#7206
File: buildtools/docker_rig/tor.Dockerfile:0-0
Timestamp: 2025-06-18T08:09:27.943Z
Learning: In the Tari project's Docker builds (buildtools/docker_rig/tor.Dockerfile), APK package version constraints like `tor>${TOR_VERSION}` should remain unquoted despite general Alpine Linux documentation suggesting quoting. Adding quotes around the version constraint breaks the Docker build process in their specific build environment.
base_layer/wallet/src/transaction_service/multisig/io.rs (1)
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.
base_layer/wallet/src/transaction_service/handle.rs (3)
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.
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.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: 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.
base_layer/wallet/src/transaction_service/service.rs (5)
undefined
<retrieved_learning>
Learnt from: ksrichard
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7307
File: comms/core/src/connection_manager/peer_connection.rs:356-357
Timestamp: 2025-07-12T03:43:22.472Z
Learning: In base_layer/core/src/base_node/tari_pulse_service/mod.rs, the disconnect_if_unused call uses parameters (0, 2) because the tari_pulse_service creates exactly 2 substreams and 0 RPC sessions during health checks.
</retrieved_learning>
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (7)
Learnt from: SolfataraEmit
PR: tari-project/tari#6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field `timelocked_balance` in the terminal output for the `getBalance` method, which should be reflected in the documentation for accuracy. The `wallet.proto` file confirms that `timelocked_balance` is defined as the fourth field in the `GetBalanceResponse` message.
Learnt from: 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.
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.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: hansieodendaal
PR: tari-project/tari#7294
File: comms/dht/src/network_discovery/seed_strap.rs:721-735
Timestamp: 2025-07-09T08:13:37.206Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the 10-second STREAM_ITEM_TIMEOUT and retry logic are intentionally designed to handle service conflicts where other services kill seed peer connections during seedstrap operations. The underlying discovery_peer/dial_peer API timeouts are too lenient for seedstrap use cases, so the more aggressive timeout with retry logic is appropriate and necessary.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
base_layer/wallet/src/transaction_service/multisig/script.rs (1)
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.
base_layer/wallet/src/transaction_service/multisig/types.rs (1)
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.
base_layer/wallet/src/transaction_service/multisig/session.rs (1)
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.
🧬 Code Graph Analysis (2)
base_layer/wallet/src/transaction_service/handle.rs (3)
base_layer/wallet/src/transaction_service/storage/database.rs (2)
write(81-81)insert_completed_transaction(335-345)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (5)
create_multisig_utxo(2054-2090)generate_multisig_member_secrets(2131-2162)collect_multisig_utxo_encumber(2164-2201)sign_multisig_utxo_member(2203-2228)send_multisig_utxo(2230-2250)base_layer/wallet/src/transaction_service/multisig/session.rs (3)
create_multisig_utxo(42-117)create_multisig_transfer_session(119-168)collect_multisig_utxo_encumber(276-393)
base_layer/wallet/src/transaction_service/multisig/session.rs (10)
base_layer/wallet/src/transaction_service/service.rs (5)
transaction(1485-1490)transaction(1627-1632)transaction(1688-1693)new(239-332)tx(4362-4362)base_layer/core/src/common/one_sided.rs (2)
shared_secret_to_output_encryption_key(51-58)shared_secret_to_output_spending_key(81-88)infrastructure/tari_script/src/lib.rs (1)
push_pubkey_script(54-56)base_layer/wallet/src/transaction_service/multisig/script.rs (5)
derive_multisig_recovery_key_id(63-73)get_multi_sig_script_components(25-38)get_utxo_by_commitment_hash(59-61)sum_public_keys(40-46)sum_public_keys_to_encryption_key(48-57)base_layer/wallet/src/output_manager_service/service.rs (3)
get_multi_sig_script_components(3609-3621)new(142-198)encumber_aggregate_utxo(1349-1739)base_layer/wallet/src/transaction_service/handle.rs (5)
new(766-774)create_multisig_utxo(1487-1510)from(737-741)create_multisig_transfer_session(1512-1535)encumber_aggregate_utxo(1051-1109)base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
decrypt_data(122-155)base_layer/core/src/transactions/transaction_components/transaction_input.rs (2)
build_script_signature_challenge(167-187)metadata_signature(296-305)base_layer/core/src/transactions/transaction_components/wallet_output.rs (1)
new_with_rangeproof(127-161)base_layer/core/src/transactions/transaction_components/transaction_output.rs (1)
build_metadata_signature_challenge(374-404)
🔇 Additional comments (6)
base_layer/wallet/src/transaction_service/multisig/io.rs (2)
75-98: Good implementation of generic error handling.The
load_party_outputfunction demonstrates good practices with generic typing, proper error handling for missing files, and clear error messages.
87-93: Consistent error handling pattern for missing files.The error handling for missing files is well-implemented with descriptive error messages and proper use of
std::io::ErrorKind::NotFound.Also applies to: 139-144, 191-196
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)
24-24: Import changes look good.The added imports are necessary for the new multisig functionality and transaction result trait usage.
Also applies to: 34-34, 43-43
680-681: Good use of explicit trait method.Using the explicit trait method improves code clarity and type safety.
711-711: Consistent with serialization changes.Good to see the deserialization also using the explicit trait method for consistency.
2131-2139: Add session_id validation across all session-based methods.All methods that accept a
session_idshould validate it's not empty to prevent unnecessary service calls.Add validation at the start of each method:
async fn generate_multisig_member_secrets( &self, request: Request<GenerateMultisigMemberSecretsRequest>, ) -> Result<Response<GenerateMultisigMemberSecretsResponse>, Status> { let message = request.into_inner(); + if message.session_id.is_empty() { + return Err(Status::invalid_argument("session_id cannot be empty")); + } let result = self.wallet.transaction_service.clone().generate_multisig_member_secrets(Apply similar validation to
collect_multisig_utxo_encumber,sign_multisig_utxo_member, andsend_multisig_utxo.Also applies to: 2164-2171, 2203-2210, 2230-2237
⛔ Skipped due to learnings
Learnt from: ksrichard PR: tari-project/tari#7129 File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29 Timestamp: 2025-05-29T20:59:32.020Z Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.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.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)
2054-2090: Add missing parameter validation.The method is missing validation for several edge cases that could cause runtime errors or security issues.
Apply this diff to add proper validation:
+ if message.m == 0 { + return Err(Status::invalid_argument( + "m must be greater than 0".to_string(), + )); + } + + if message.public_keys.is_empty() { + return Err(Status::invalid_argument( + "public keys list cannot be empty".to_string(), + )); + } + if (message.m as usize) > message.public_keys.len() { return Err(Status::invalid_argument( "m must be less than or equal to the number of public keys".to_string(), )); }
2092-2114: Add comprehensive parameter validation.Similar to
create_multisig_utxo, this method should validate all parameters to prevent runtime errors and security issues.Add validation before line 2108:
+ if message.utxo_commitment_hash.is_empty() { + return Err(Status::invalid_argument( + "utxo_commitment_hash cannot be empty".to_string(), + )); + } + + if message.m == 0 { + return Err(Status::invalid_argument( + "m must be greater than 0".to_string(), + )); + } + + if public_keys.is_empty() { + return Err(Status::invalid_argument( + "public keys list cannot be empty".to_string(), + )); + } + + if (message.m as usize) > public_keys.len() { + return Err(Status::invalid_argument( + "m must be less than or equal to the number of public keys".to_string(), + )); + } + let result = self.wallet.transaction_service.clone().create_multisig_transfer_session(
2203-2228: Fix incorrect error message and add validation.The method has an incorrect error message and is missing input validation.
Apply this diff to fix the error message and add validation:
+ if message.session_id.is_empty() { + return Err(Status::invalid_argument( + "session_id cannot be empty".to_string(), + )); + } + let result = self.wallet.transaction_service.clone().sign_multisig_utxo_member(message.session_id).await; match result { Ok(signs) => { Ok(Response::new(SignMultisigUtxoMemberResponse { signatures: signs.into_iter() .map(|sign| MemberMultisigSignature { output_index: sign.output_index as u64, metadata_signature: sign.metadata_signature.to_binary().unwrap(), script_signature: sign.script_signature.to_binary().unwrap(), script_offset: sign.script_offset.to_binary().unwrap(), }) .collect(), })) }, Err(e) => { - Err(Status::internal(format!("Error creating multisig UTXO: {}", e))) + Err(Status::internal(format!("Error signing multisig UTXO member: {}", e))) }, }base_layer/wallet/src/output_manager_service/service.rs (1)
3464-3536: Critical issues remain unaddressed from previous review.The multisig scanning implementation still has the same issues identified in previous reviews:
- Error handling:
to_public_key()failures use?operator which could abort the entire scanning process- Script key derivation: Still uses
TariKeyId::default()instead of proper key derivation- Security verification needed: The sum-of-pubkeys encryption approach needs protocol alignment confirmation
Please address these issues before merging:
async fn scan_outputs_for_multisig( &mut self, outputs: Vec<(TransactionOutput, Option<TxId>)>, ) -> Result<Vec<RecoveredOutput>, OutputManagerError> { let my_pubkey = self.resources.key_manager.get_spend_key().await?.pub_key; let mut scanned_outputs = vec![]; for (output, tx_id) in outputs { if let Some(Opcode::CheckMultiSigVerifyAggregatePubKey(_m, _n, pubkeys, _msg)) = output.script.as_slice().iter().find(|op| matches!(op, Opcode::CheckMultiSigVerifyAggregatePubKey(..))) { if pubkeys.iter().any(|pk| pk == &my_pubkey) { - let mut sum_pubkeys = UncompressedPublicKey::default(); - for pk in pubkeys { - sum_pubkeys = &sum_pubkeys + pk.to_public_key()?; - } + let sum_pubkeys = pubkeys.iter() + .map(|pk| pk.to_public_key()) + .collect::<Result<Vec<_>, _>>() + .map_err(|e| OutputManagerError::ServiceError(format!("Failed to convert pubkey: {}", e)))? + .into_iter() + .fold(UncompressedPublicKey::default(), |acc, pk| &acc + &pk); let encryption_key = public_key_to_output_encryption_key(&CompressedPublicKey::new_from_pk(sum_pubkeys))?; if let Ok((committed_value, spending_key, payment_id)) = EncryptedData::decrypt_data(&encryption_key, &output.commitment, &output.encrypted_data) { if output.verify_mask(&self.resources.factories.range_proof, &spending_key, committed_value.into())? { let spending_key_id = self.resources.key_manager.import_key(spending_key).await?; + // Generate proper script key ID for multisig + let script_key_id = self.resources.key_manager.get_next_key( + TransactionKeyManagerBranch::Script.get_branch_key() + ).await?.key_id; let rewound_output = WalletOutput::new_with_rangeproof( output.version, committed_value, spending_key_id, output.features, output.script, ExecutionStack::new(vec![]), - TariKeyId::default(), // or derive as needed + script_key_id, output.sender_offset_public_key, output.metadata_signature, 0, output.covenant, output.encrypted_data, output.minimum_value_promise, output.proof, payment_id, ); scanned_outputs.push((rewound_output, OutputSource::Multisig, tx_id)); } } } } } self.import_onesided_outputs(scanned_outputs).await }
🧹 Nitpick comments (1)
base_layer/wallet/src/output_manager_service/service.rs (1)
556-567: Remove debug println statements before production.The debug
println!statements should be removed or converted to proper logging using thedebug!macro.Apply this diff to improve logging:
- OutputManagerRequest::FetchUnspentOutputs(hashes) => { - println!("fetch unspent outputs {:?}", hashes.iter().map(|h| h.to_hex()).collect::<Vec<_>>()); + OutputManagerRequest::FetchUnspentOutputs(hashes) => { + debug!(target: LOG_TARGET, "Fetching unspent outputs: {:?}", hashes.iter().map(|h| h.to_hex()).collect::<Vec<_>>()); let mut outputs = Vec::new(); for hash in hashes { if let Some(output) = self.fetch_unspent_outputs_from_node(hash).await? { outputs.push(output); } } - println!("fetched unspent outputs completed {:?}", outputs); + debug!(target: LOG_TARGET, "Fetched {} unspent outputs", outputs.len()); Ok(OutputManagerResponse::FetchUnspentOutputs(outputs)) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
applications/minotari_console_wallet/src/automation/commands.rs(6 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(7 hunks)base_layer/wallet/src/output_manager_service/service.rs(24 hunks)base_layer/wallet/src/transaction_service/service.rs(4 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(1 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
- clients/rust/base_node_wallet_client/src/client/http.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- base_layer/wallet/src/transaction_service/service.rs
🧰 Additional context used
🧠 Learnings (2)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (8)
<retrieved_learning>
Learnt from: SolfataraEmit
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: ksrichard
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7294
File: comms/dht/src/network_discovery/seed_strap.rs:721-735
Timestamp: 2025-07-09T08:13:37.206Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the 10-second STREAM_ITEM_TIMEOUT and retry logic are intentionally designed to handle service conflicts where other services kill seed peer connections during seedstrap operations. The underlying discovery_peer/dial_peer API timeouts are too lenient for seedstrap use cases, so the more aggressive timeout with retry logic is appropriate and necessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
base_layer/wallet/src/output_manager_service/service.rs (5)
undefined
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.657Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via get_last_scanned_height() rather than persisting it themselves. This follows separation of concerns architecture pattern.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
🧬 Code Graph Analysis (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (8)
applications/minotari_console_wallet/src/grpc/mod.rs (1)
convert_to_transaction_event(22-58)base_layer/wallet/src/transaction_service/offline_signing/models.rs (2)
to_json(69-71)from_json(47-67)base_layer/wallet/src/transaction_service/handle.rs (6)
create_multisig_utxo(1487-1510)new(766-774)generate_multisig_member_secrets(1537-1552)collect_multisig_utxo_encumber(1554-1569)sign_multisig_utxo_member(1571-1586)send_multisig_utxo(1588-1602)base_layer/wallet/src/transaction_service/multisig/session.rs (2)
create_multisig_utxo(42-117)collect_multisig_utxo_encumber(276-393)base_layer/common_types/src/tari_address/mod.rs (2)
from_hex(400-403)from_str(409-419)base_layer/wallet/src/transaction_service/service.rs (1)
new(239-332)applications/minotari_console_wallet/src/grpc/wallet_debouncer.rs (1)
new(60-82)applications/minotari_console_wallet/src/notifier/mod.rs (1)
new(78-90)
🔇 Additional comments (16)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (6)
24-24: LGTM: Import addition is appropriate.The addition of
str::FromStrimport is needed for parsing TariAddress from strings in the new multisig methods.
34-35: LGTM: Multisig-related imports are comprehensive.The new imports for multisig-related gRPC messages are appropriate and comprehensive for the functionality being added.
43-43: LGTM: Direct import improves clarity.The direct import of
SignedOneSidedTransactionResultimproves code clarity.
70-70: LGTM: ByteArray import is needed.The
ByteArrayimport is needed for the binary serialization operations in the new multisig methods.
680-681: LGTM: Explicit trait method usage improves clarity.The explicit use of the
TransactionResulttrait'sto_jsonmethod improves code clarity and ensures the correct method is called.
711-711: LGTM: Explicit trait method usage improves clarity.The explicit use of the
TransactionResulttrait'sfrom_jsonmethod improves code clarity and ensures the correct method is called.base_layer/wallet/src/output_manager_service/service.rs (10)
404-404: LGTM: Spelling correction applied correctly.The method name correction from
confirm_encumberancetoconfirm_encumbranceis properly implemented.
498-501: LGTM: New multisig scanning request handler added.The request handler for multisig output scanning is properly integrated into the request handling flow.
521-521: LGTM: Method call updated to use generalized internal method.The refactoring to use
create_transaction_with_outputs_internalwithNonerecipient address maintains backward compatibility.
528-542: LGTM: New request handler for pay-to-address transactions.The new
CreatePayToAddressContainingOutputsrequest handler correctly calls the internal method with the recipient address.
568-571: LGTM: New confirm encumbrance request handler.The handler correctly calls the renamed method and returns the appropriate response.
1183-1189: LGTM: Method signature generalized to support recipient address.The method signature change to accept an optional recipient address enables both pay-to-self and pay-to-address functionality while maintaining backward compatibility.
1377-1377: LGTM: Method calls updated to use new method name.The calls to
fetch_unspent_outputs_from_nodecorrectly use the renamed method.Also applies to: 1751-1751
1548-1548: LGTM: Method calls updated to use corrected spelling.All calls to
confirm_encumbranceuse the corrected method name.Also applies to: 1887-1887, 2124-2124, 2601-2601, 2818-2818, 3013-3013, 3214-3214, 3299-3299
2133-2154: LGTM: Method signature corrected.The method signature uses the correct spelling
confirm_encumbrance.
3102-3113: LGTM: Method renamed and made public.The method is properly renamed to
fetch_unspent_outputs_from_nodeand made public for external access.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (12)
base_layer/wallet/src/transaction_service/multisig/io.rs (4)
17-17: Remove unnecessary async from functions that don't perform async operations.These functions are marked
asyncbut don't contain any actual async operations (.awaitcalls). This adds unnecessary overhead and misleading API design.Also applies to: 34-34, 48-48, 121-121, 138-138, 160-160, 183-183
27-27: Replace print!/println! with proper logging.Using
print!andprintln!for logging is not appropriate for production code. These should use the proper logging framework.Also applies to: 65-65, 76-76
20-20: Make the wallet data directory configurable.The hardcoded
/wallet_datapath is repeated throughout all functions, making it difficult to configure for different environments or testing scenarios. Consider making this configurable via environment variable or configuration parameter.Also applies to: 35-35, 54-54, 89-89, 125-125, 139-139, 165-165, 187-187
20-23: Extract directory creation logic to avoid duplication.Directory creation logic is repeated in multiple functions. This should be extracted to a helper function.
Also applies to: 54-57, 125-128, 139-142, 165-168, 187-190
base_layer/wallet/src/transaction_service/multisig/session.rs (8)
141-141: Replace println! with proper error logging.Debug output should use a logging framework instead of stdout.
200-201: Use UUID for session ID to avoid collisions.Using timestamp as session ID could cause collisions if multiple sessions are created within the same second.
384-384: Replace println! with proper logging.Debug output should use a logging framework.
485-486: Propagate errors instead of just logging them.Errors should be collected and handled properly rather than just printed to stderr.
524-524: Remove or replace println! debug statements.Multiple debug print statements should be removed or replaced with proper logging.
Also applies to: 533-533, 553-553, 559-559, 558-558, 604-604, 621-621, 686-686, 724-724, 768-768, 809-809
494-934: Refactor this method - it's too long and has too many responsibilities.This 400+ line method violates the single responsibility principle and is difficult to maintain. Consider breaking it down into smaller, focused methods.
984-986: Improve error handling - propagate errors instead of printing and breaking.Errors should be properly propagated rather than printed to stderr and continuing with a break.
Also applies to: 998-1000, 1021-1023
1139-1143: Replace println! with proper logging for warnings.Warning messages should use a logging framework.
🧹 Nitpick comments (6)
base_layer/wallet/src/transaction_service/multisig/io.rs (2)
18-18: Remove unnecessary clone operation.The
multisig_outputis cloned unnecessarily before use. Since the function takes ownership, you can use it directly.- let output = multisig_output.clone(); - let out_dir = std::path::Path::new("/wallet_data"); if !out_dir.exists() { std::fs::create_dir_all(out_dir)?; } - let out_file = out_dir.join(format!("multisig_output-{}.json", output.session_id)); + let out_file = out_dir.join(format!("multisig_output-{}.json", multisig_output.session_id)); print!("Saving multisig output to: {}", out_file.display()); let file = fs::File::create(&out_file)?; - serde_json::to_writer_pretty(file, &output).map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?; + serde_json::to_writer_pretty(file, &multisig_output).map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?;
35-38: Remove directory creation from load functions.Creating directories in load functions is unnecessary. If the directory doesn't exist, there won't be any files to load. This should fail with an appropriate error instead.
pub async fn load_multisig_output(session_id: &str) -> Result<MultisigOutput, std::io::Error> { let out_dir = std::path::Path::new("/wallet_data"); - if !out_dir.exists() { - std::fs::create_dir_all(out_dir)?; - } - let file_path = std::path::Path::new("/wallet_data").join(format!("multisig_output-{}.json", session_id));Apply similar changes to
load_multisig_utxo_encumberandload_multisig_member_signatures.Also applies to: 139-142, 187-190
base_layer/wallet/src/transaction_service/multisig/session.rs (4)
51-52: Remove unnecessary clone operations.The
leaderandmemberfields are cloned unnecessarily since they're already owned by the struct.- let leader_output = multisig_output.leader.clone(); - let member_output = multisig_output.member.clone(); + let leader_output = multisig_output.leader; + let member_output = multisig_output.member;
530-530: Fix debug message numbering.The debug message shows "encumber_0.5" which seems like a typo or inconsistent numbering.
- println!("encumber_0.5"); + println!("encumber_1");
1096-1096: Remove unnecessary explicit return statement.The explicit
returnstatement is unnecessary in Rust. Use implicit return instead.- return Ok(output_signatures); + Ok(output_signatures)
1078-1086: Improve signature validation logic.Checking if signatures equal the default value is not a robust way to validate them. Consider using a more explicit validation method or returning an error type.
- if script_signature.get_signature() == Signature::default().get_signature() || - metadata_signature.get_signature() == Signature::default().get_signature() - { - eprintln!( - "\nError: Script and/or metadata signatures not created (index {})!\n", - output_index, - ); - break; - } + if script_signature.get_signature() == Signature::default().get_signature() || + metadata_signature.get_signature() == Signature::default().get_signature() + { + return Err(TransactionServiceError::Other(format!( + "Script and/or metadata signatures not created (index {})", + output_index + ))); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
applications/minotari_app_grpc/src/lib.rs(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(5 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(7 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(4 hunks)applications/minotari_node/src/grpc_method.rs(4 hunks)base_layer/wallet/src/output_manager_service/handle.rs(10 hunks)base_layer/wallet/src/output_manager_service/service.rs(22 hunks)base_layer/wallet/src/transaction_service/handle.rs(7 hunks)base_layer/wallet/src/transaction_service/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/io.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/script.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/session.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/types.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(5 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(1 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/wallet/src/transaction_service/multisig/mod.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- base_layer/wallet/src/transaction_service/mod.rs
- applications/minotari_app_grpc/src/lib.rs
- applications/minotari_console_wallet/src/wallet_modes.rs
- clients/rust/base_node_wallet_client/src/client/http.rs
- applications/minotari_node/src/grpc_method.rs
- base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
- base_layer/wallet/src/transaction_service/multisig/script.rs
- base_layer/wallet/src/output_manager_service/handle.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- base_layer/wallet/src/transaction_service/service.rs
- base_layer/wallet/src/transaction_service/handle.rs
- base_layer/wallet/src/output_manager_service/service.rs
- base_layer/wallet/src/transaction_service/multisig/types.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
base_layer/wallet/src/transaction_service/multisig/io.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: 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.
base_layer/wallet/src/transaction_service/multisig/session.rs (5)
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.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: hansieodendaal
PR: tari-project/tari#7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the context.connectivity.dial_peer method should fail fast and return an error if a peer cannot be dialed, rather than requiring retry logic for general connection failures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
base_layer/wallet/src/transaction_service/multisig/session.rs (4)
527-527: Remove debug println! statements throughout the method.Also applies to: 533-533, 553-553, 559-559, 621-621, 659-659, 709-709, 728-728, 793-793, 831-831, 875-875, 916-916
495-937: Refactor this excessively long method into smaller, focused functions.
1004-1006: Propagate errors properly instead of printing and breaking.Also applies to: 1027-1030, 1078-1081, 1087-1091
1144-1149: Replace println! with proper logging for warnings.Also applies to: 1155-1160
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base_layer/wallet/src/transaction_service/multisig/io.rs(1 hunks)base_layer/wallet/src/transaction_service/multisig/session.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- base_layer/wallet/src/transaction_service/multisig/io.rs
- base_layer/wallet/src/transaction_service/service.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
base_layer/wallet/src/transaction_service/multisig/session.rs (5)
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.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: hansieodendaal
PR: tari-project/tari#7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the context.connectivity.dial_peer method should fail fast and return an error if a peer cannot be dialed, rather than requiring retry logic for general connection failures.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
base_layer/wallet/src/transaction_service/multisig/io.rs (1)
17-23: Hard-coded storage path is still here (see earlier review).
The directory is fixed to/wallet_data; earlier feedback asked to make this configurable. Please refactor to read from config/env instead of a literal.base_layer/wallet/src/transaction_service/service.rs (3)
1142-1145: Add transaction validation and event publishing for consistency.This handler is missing validation and side effects that are present in the
ImportTransactionhandler (lines 1042-1067). For consistency and safety, this should include transaction size validation and event publishing.Apply this diff to add the missing validation and event publishing:
TransactionServiceRequest::InsertCompletedTransaction(tx_id, transaction) => { + check_transaction_size(&transaction.transaction, tx_id)?; self.db.insert_completed_transaction(tx_id, transaction)?; + let _size = self + .event_publisher + .send(Arc::new(TransactionEvent::TransactionImported(tx_id))); Ok(TransactionServiceResponse::TransactionImported(tx_id)) }
1183-1185: Fix duplicate error mapping.Line 1184-1185 has redundant error mapping applied twice. This is unnecessary and could mask the actual error type.
Apply this diff to fix the duplicate error mapping:
let config = load_multisig_output(&session_id) - .map_err(|e| TransactionServiceError::Other(e.to_string())) .map_err(|e| TransactionServiceError::Other(e.to_string()))?;
1263-1357: Refactor complex handler and fix logging.This handler is too complex (95+ lines) and contains multiple concerns. It should be broken down into smaller, focused functions. Additionally, the
println!statements on lines 1289, 1305, and 1352 should use proper logging.Consider breaking this handler into separate functions:
load_multisig_signatures- for loading member signaturesaggregate_multisig_signatures- for signature aggregation logicfinalize_multisig_transactions- for finalizing transactionsAlso replace the
println!statements with proper logging:println!( + warn!(target: LOG_TARGET, "Warning: Could not load multisig member signatures for {:?}: {}. Skipping.", pubkey, e ); println!( + warn!(target: LOG_TARGET, "Warning: Could not get ephemeral pubkey for {:?}: {}. Skipping.", pubkey, e ); println!("Error finalizing aggregate encumbered tx: {}", e); + error!(target: LOG_TARGET, "Error finalizing aggregate encumbered tx: {}", e);
🧹 Nitpick comments (3)
base_layer/wallet/src/transaction_service/multisig/io.rs (2)
25-27: Remove needless clone.
multisig_outputis taken by value, solet output = multisig_output.clone();just doubles memory & CPU. Use the moved value directly.- let output = multisig_output.clone(); + let output = multisig_output; // already owned
30-33: File writes silently overwrite & are not atomic.
UsingFile::createwill truncate any existing file, risking data loss and race conditions if multiple wallet processes run. Consider:•
OpenOptions::new().create_new(true)to fail when the file already exists, or
• write to a temp file andrenamefor atomic replacement.Also applies to: 121-125, 155-164
base_layer/wallet/src/transaction_service/service.rs (1)
1210-1212: Fix logging consistency.Line 1211 uses
log::debug!instead of the consistentdebug!macro withLOG_TARGETused throughout the rest of the file.Apply this diff to fix the logging consistency:
if own_address.public_spend_key() == &CompressedPublicKey::from(pubkey.clone()) { - log::debug!("Skipping own address: {}", own_address); + debug!(target: LOG_TARGET, "Skipping own address: {}", own_address); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/minotari_console_wallet/src/automation/commands.rs(5 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(7 hunks)base_layer/wallet/src/transaction_service/multisig/io.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
base_layer/wallet/src/transaction_service/multisig/io.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
applications/minotari_console_wallet/src/automation/commands.rs (1)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
base_layer/wallet/src/transaction_service/service.rs (7)
undefined
<retrieved_learning>
Learnt from: ksrichard
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
</retrieved_learning>
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of tokio::task::spawn(async move { with tokio::task::spawn_blocking().await in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7307
File: comms/core/src/connection_manager/peer_connection.rs:356-357
Timestamp: 2025-07-12T03:43:22.472Z
Learning: In base_layer/core/src/base_node/tari_pulse_service/mod.rs, the disconnect_if_unused call uses parameters (0, 2) because the tari_pulse_service creates exactly 2 substreams and 0 RPC sessions during health checks.
</retrieved_learning>
🔇 Additional comments (6)
applications/minotari_console_wallet/src/automation/commands.rs (2)
1887-1904: LGTM! Good enhancement to UTXO information display.The addition of commitment hash and multisig detection to the ExportUtxos command provides valuable information for users. The implementation correctly uses the
is_multisig_utxofunction and maintains consistency with the existing output format.
2536-2562: LGTM! Proper error handling and validation.This command correctly implements the established error handling pattern with:
- Proper parameter validation (m ≤ public_keys.len())
- Match statement for result handling
- Debug logging on success
- User feedback with transaction ID
- Consistent error message format
- Transaction ID monitoring
This is a good example of the expected pattern for other multisig commands.
base_layer/wallet/src/transaction_service/service.rs (4)
55-55: LGTM - Imports are appropriate for multisig functionality.The new imports for
UncompressedSignature,CompressedKey,RistrettoPublicKey,Hex, and the multisig module components are all necessary for the new multisig transaction handlers being added.Also applies to: 92-92, 94-94, 110-110, 136-146
1146-1163: LGTM - Well-structured multisig UTXO creation.The handler correctly creates a multisig session, generates the multisig UTXO, and submits it to self. The error handling and flow are appropriate.
1165-1178: LGTM - Clean multisig transfer session creation.The handler properly creates a multisig session and initializes a transfer session with the provided parameters. The implementation is clean and follows good practices.
1237-1261: LGTM - Well-implemented multisig signing handler.The handler properly loads the multisig output, member secrets, and encumber data, then performs the signing operation. The error handling and data flow are appropriate.
| SendMultisigUtxoLeader(args) => { | ||
| transaction_service.send_multisig_utxo(args.session_id).await?; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling pattern.
This command doesn't follow the established error handling pattern for this file. According to the codebase patterns, commands should use match statements with debug logging and user feedback on success, and eprintln! with format "{CommandName} error! {}" on errors.
SendMultisigUtxoLeader(args) => {
- transaction_service.send_multisig_utxo(args.session_id).await?;
+ match transaction_service.send_multisig_utxo(args.session_id).await {
+ Ok(tx_id) => {
+ debug!(target: LOG_TARGET, "send-multisig-utxo-leader concluded with tx_id {}", tx_id);
+ println!("Multisig UTXO sent successfully with tx_id: {}", tx_id);
+ tx_ids.push(tx_id);
+ },
+ Err(e) => eprintln!("SendMultisigUtxoLeader error! {}", e),
+ }
},🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
2489 to 2491, the SendMultisigUtxoLeader command does not follow the file's
consistent error handling pattern. Refactor the code to use a match statement on
the result of send_multisig_utxo, log debug information on success, provide user
feedback, and on error use eprintln! with the format "SendMultisigUtxoLeader
error! {}", passing the error message.
| CreateMultisigUtxoTransferLeader(args) => { | ||
| let mut transaction_service = wallet.transaction_service.clone(); | ||
| let public_keys = args | ||
| .public_keys | ||
| .iter() | ||
| .map(|pk| CompressedPublicKey::from(pk.clone())) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let output = transaction_service | ||
| .create_multisig_transfer_session( | ||
| args.utxo_commitment_hash, | ||
| args.m, | ||
| public_keys, | ||
| args.recipient_address, | ||
| ) | ||
| .await?; | ||
| save_multisig_output(output.clone()).map_err(|e| CommandError::General(e.to_string()))?; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling pattern.
This command should follow the established error handling pattern with proper success feedback and error handling.
CreateMultisigUtxoTransferLeader(args) => {
- let mut transaction_service = wallet.transaction_service.clone();
- let public_keys = args
- .public_keys
- .iter()
- .map(|pk| CompressedPublicKey::from(pk.clone()))
- .collect::<Vec<_>>();
-
- let output = transaction_service
- .create_multisig_transfer_session(
- args.utxo_commitment_hash,
- args.m,
- public_keys,
- args.recipient_address,
- )
- .await?;
- save_multisig_output(output.clone()).map_err(|e| CommandError::General(e.to_string()))?;
+ match async {
+ let mut transaction_service = wallet.transaction_service.clone();
+ let public_keys = args
+ .public_keys
+ .iter()
+ .map(|pk| CompressedPublicKey::from(pk.clone()))
+ .collect::<Vec<_>>();
+
+ let output = transaction_service
+ .create_multisig_transfer_session(
+ args.utxo_commitment_hash,
+ args.m,
+ public_keys,
+ args.recipient_address,
+ )
+ .await?;
+ save_multisig_output(output.clone()).map_err(|e| CommandError::General(e.to_string()))?;
+ Ok(())
+ }.await {
+ Ok(()) => {
+ debug!(target: LOG_TARGET, "create-multisig-utxo-transfer-leader concluded");
+ println!("Multisig UTXO transfer session created successfully");
+ },
+ Err(e) => eprintln!("CreateMultisigUtxoTransferLeader error! {}", e),
+ }
},🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
2517 to 2534, the CreateMultisigUtxoTransferLeader command lacks consistent
error handling and success feedback. Refactor the code to wrap the entire
operation in a proper error handling block that returns a CommandError on
failure, and add a success message or indication on successful completion to
align with the established pattern used in other commands.
| SignMultisigUtxoMember(args) => { | ||
| let own_address: TariAddress = wallet.get_wallet_interactive_address().await?; | ||
| let signatures = transaction_service | ||
| .sign_multisig_utxo_member(args.session_id.clone()) | ||
| .await?; | ||
| save_multisig_member_signatures(&args.session_id, signatures, own_address.clone()) | ||
| .map_err(|e| CommandError::General(e.to_string()))?; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling pattern.
This command should follow the established error handling pattern with proper success feedback and error handling.
SignMultisigUtxoMember(args) => {
- let own_address: TariAddress = wallet.get_wallet_interactive_address().await?;
- let signatures = transaction_service
- .sign_multisig_utxo_member(args.session_id.clone())
- .await?;
- save_multisig_member_signatures(&args.session_id, signatures, own_address.clone())
- .map_err(|e| CommandError::General(e.to_string()))?;
+ match async {
+ let own_address: TariAddress = wallet.get_wallet_interactive_address().await?;
+ let signatures = transaction_service
+ .sign_multisig_utxo_member(args.session_id.clone())
+ .await?;
+ save_multisig_member_signatures(&args.session_id, signatures, own_address.clone())
+ .map_err(|e| CommandError::General(e.to_string()))?;
+ Ok(())
+ }.await {
+ Ok(()) => {
+ debug!(target: LOG_TARGET, "sign-multisig-utxo-member concluded");
+ println!("Multisig UTXO signatures saved successfully");
+ },
+ Err(e) => eprintln!("SignMultisigUtxoMember error! {}", e),
+ }
},🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
2493 to 2500, the SignMultisigUtxoMember command lacks consistent error handling
and success feedback. Refactor the code to wrap the entire operation in a result
context, handle errors uniformly using the established pattern, and add a
success message or confirmation upon completion to provide clear feedback to the
user.
| CollectMultisigUtxoEncumber(args) => { | ||
| let outputs = transaction_service | ||
| .collect_multisig_utxo_encumber(args.session_id.clone()) | ||
| .await?; | ||
|
|
||
| save_multisig_utxo_encumber(&args.session_id.clone(), outputs) | ||
| .map_err(|e| CommandError::General(e.to_string()))?; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling pattern.
This command should follow the established error handling pattern with proper success feedback and error handling.
CollectMultisigUtxoEncumber(args) => {
- let outputs = transaction_service
- .collect_multisig_utxo_encumber(args.session_id.clone())
- .await?;
-
- save_multisig_utxo_encumber(&args.session_id.clone(), outputs)
- .map_err(|e| CommandError::General(e.to_string()))?;
+ match async {
+ let outputs = transaction_service
+ .collect_multisig_utxo_encumber(args.session_id.clone())
+ .await?;
+ save_multisig_utxo_encumber(&args.session_id.clone(), outputs)
+ .map_err(|e| CommandError::General(e.to_string()))?;
+ Ok(())
+ }.await {
+ Ok(()) => {
+ debug!(target: LOG_TARGET, "collect-multisig-utxo-encumber concluded");
+ println!("Multisig UTXO encumbered outputs collected successfully");
+ },
+ Err(e) => eprintln!("CollectMultisigUtxoEncumber error! {}", e),
+ }
},📝 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.
| CollectMultisigUtxoEncumber(args) => { | |
| let outputs = transaction_service | |
| .collect_multisig_utxo_encumber(args.session_id.clone()) | |
| .await?; | |
| save_multisig_utxo_encumber(&args.session_id.clone(), outputs) | |
| .map_err(|e| CommandError::General(e.to_string()))?; | |
| }, | |
| CollectMultisigUtxoEncumber(args) => { | |
| match async { | |
| let outputs = transaction_service | |
| .collect_multisig_utxo_encumber(args.session_id.clone()) | |
| .await?; | |
| save_multisig_utxo_encumber(&args.session_id.clone(), outputs) | |
| .map_err(|e| CommandError::General(e.to_string()))?; | |
| Ok(()) | |
| }.await { | |
| Ok(()) => { | |
| debug!(target: LOG_TARGET, "collect-multisig-utxo-encumber concluded"); | |
| println!("Multisig UTXO encumbered outputs collected successfully"); | |
| }, | |
| Err(e) => eprintln!("CollectMultisigUtxoEncumber error! {}", e), | |
| } | |
| }, |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
2502 to 2509, the CollectMultisigUtxoEncumber command lacks consistent error
handling and success feedback. Refactor the code to handle errors using the
established pattern, ensuring any errors from collect_multisig_utxo_encumber and
save_multisig_utxo_encumber are properly mapped and returned. Also, add a
success message or indication after the operation completes successfully to
maintain consistency with other commands.
| CreateMultisigUtxoTransferMember(args) => { | ||
| let output = transaction_service | ||
| .generate_multisig_member_secrets(args.session_id) | ||
| .await?; | ||
| save_multisig_party_output(output).map_err(|e| CommandError::General(e.to_string()))?; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling pattern.
This command should follow the established error handling pattern with proper success feedback and error handling.
CreateMultisigUtxoTransferMember(args) => {
- let output = transaction_service
- .generate_multisig_member_secrets(args.session_id)
- .await?;
- save_multisig_party_output(output).map_err(|e| CommandError::General(e.to_string()))?;
+ match async {
+ let output = transaction_service
+ .generate_multisig_member_secrets(args.session_id)
+ .await?;
+ save_multisig_party_output(output).map_err(|e| CommandError::General(e.to_string()))?;
+ Ok(())
+ }.await {
+ Ok(()) => {
+ debug!(target: LOG_TARGET, "create-multisig-utxo-transfer-member concluded");
+ println!("Multisig UTXO transfer member secrets generated successfully");
+ },
+ Err(e) => eprintln!("CreateMultisigUtxoTransferMember error! {}", e),
+ }
},📝 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.
| CreateMultisigUtxoTransferMember(args) => { | |
| let output = transaction_service | |
| .generate_multisig_member_secrets(args.session_id) | |
| .await?; | |
| save_multisig_party_output(output).map_err(|e| CommandError::General(e.to_string()))?; | |
| }, | |
| CreateMultisigUtxoTransferMember(args) => { | |
| match async { | |
| let output = transaction_service | |
| .generate_multisig_member_secrets(args.session_id) | |
| .await?; | |
| save_multisig_party_output(output) | |
| .map_err(|e| CommandError::General(e.to_string()))?; | |
| Ok(()) | |
| } | |
| .await { | |
| Ok(()) => { | |
| debug!(target: LOG_TARGET, "create-multisig-utxo-transfer-member concluded"); | |
| println!("Multisig UTXO transfer member secrets generated successfully"); | |
| }, | |
| Err(e) => eprintln!("CreateMultisigUtxoTransferMember error! {}", e), | |
| } | |
| }, |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
2511 to 2516, the CreateMultisigUtxoTransferMember command lacks consistent
error handling and success feedback. Modify the code to handle errors using the
established pattern by mapping errors to CommandError and add a success message
or confirmation after saving the multisig party output to provide clear feedback
on successful execution.
| let leader_file = out_dir.join(format!( | ||
| "multisig_party_output-leader-{}-{}.json", | ||
| leader_output.session_id, | ||
| leader_output.member_public_key.to_hex(), | ||
| )); | ||
|
|
||
| let file = fs::File::create(&leader_file)?; |
There was a problem hiding this comment.
Path-traversal vulnerability via session_id / key strings.
format!("…{}", user_input) is joined directly into the path; a crafted session_id like ../../evil will escape /wallet_data. Validate/whitelist characters (e.g., [a-zA-Z0-9_-]+) before building paths.
Also applies to: 65-72, 121-133, 156-176
🤖 Prompt for AI Agents
In base_layer/wallet/src/transaction_service/multisig/io.rs around lines 54 to
60, the session_id and member_public_key are directly used in path construction
without validation, leading to a path traversal vulnerability. To fix this,
validate or whitelist the session_id and key strings to allow only safe
characters such as alphanumeric, underscores, and hyphens before using them in
the file path. Apply similar validation to other occurrences in lines 65-72,
121-133, and 156-176.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
base_layer/wallet/src/transaction_service/handle.rs (1)
1704-1720: Align insert_completed_transaction with other handle methods.This method still deviates from the established pattern used by other handle methods.
Apply the suggested fix from the previous review:
pub async fn insert_completed_transaction( &mut self, tx_id: TxId, transaction: CompletedTransaction, - ) -> Result<TxId, TransactionStorageError> { - self.handle - .call(TransactionServiceRequest::InsertCompletedTransaction( - tx_id, - transaction, - )) - .await - .map_err(|e| TransactionStorageError::UnexpectedResult(e.to_string()))? - .map(|_| ()) - .map_err(|e| TransactionStorageError::UnexpectedResult(e.to_string()))?; - - Ok(tx_id) + ) -> Result<TxId, TransactionServiceError> { + match self + .handle + .call(TransactionServiceRequest::InsertCompletedTransaction(tx_id, transaction)) + .await?? + { + TransactionServiceResponse::TransactionImported(id) => Ok(id), + _ => Err(TransactionServiceError::UnexpectedApiResponse), + } }base_layer/wallet/src/transaction_service/multisig/session.rs (3)
527-527: Remove or replace println! debug statements.Multiple debug print statements should be removed or replaced with proper logging.
Also applies to: 533-533, 553-553, 559-559, 659-659, 709-709, 728-728, 745-745, 793-793, 831-831, 875-875, 916-916
495-937: Refactor this method - it's too long and has too many responsibilities.This 400+ line method violates the single responsibility principle and is difficult to maintain.
1144-1149: Replace println! with proper logging for warnings.Warning messages should use a logging framework.
Also applies to: 1155-1160
🧹 Nitpick comments (2)
base_layer/wallet/src/transaction_service/handle.rs (1)
1521-1630: Remove unnecessary return statements in multisig methods.The multisig methods follow the correct pattern but contain unnecessary explicit return statements that should be removed for consistency with Rust idioms.
Apply this pattern to all multisig methods:
- _ => { - return Err(TransactionServiceError::UnexpectedApiResponse); - }, + _ => Err(TransactionServiceError::UnexpectedApiResponse),base_layer/wallet/src/transaction_service/multisig/session.rs (1)
1102-1102: Remove unnecessary return statement.The explicit return statement is not needed in Rust.
- return Ok(output_signatures); + Ok(output_signatures)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapplications/minotari_app_grpc/proto/descriptor.binis excluded by!**/*.bin
📒 Files selected for processing (8)
applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_console_wallet/src/automation/commands.rs(5 hunks)applications/minotari_console_wallet/src/cli.rs(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(8 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(4 hunks)base_layer/wallet/src/transaction_service/handle.rs(7 hunks)base_layer/wallet/src/transaction_service/multisig/session.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- applications/minotari_app_grpc/proto/wallet.proto
🚧 Files skipped from review as they are similar to previous changes (5)
- applications/minotari_console_wallet/src/wallet_modes.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- base_layer/wallet/src/transaction_service/service.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- applications/minotari_console_wallet/src/cli.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
base_layer/wallet/src/transaction_service/handle.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: 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.
base_layer/wallet/src/transaction_service/multisig/session.rs (5)
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.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
Learnt from: hansieodendaal
PR: tari-project/tari#7294
File: comms/dht/src/network_discovery/seed_strap.rs:352-456
Timestamp: 2025-07-09T08:33:29.320Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the context.connectivity.dial_peer method should fail fast and return an error if a peer cannot be dialed, rather than requiring retry logic for general connection failures.
🔇 Additional comments (9)
base_layer/wallet/src/transaction_service/handle.rs (4)
66-74: LGTM!The imports are appropriate for the new multisig functionality being added.
272-290: Well-structured request variants for multisig operations.The new request variants follow consistent patterns and appropriately encapsulate the required data for each multisig operation.
529-566: Display implementations are consistent and informative.The Display implementations follow the established pattern and provide useful information for logging and debugging.
627-632: Response variants correctly map to their operations.Each response variant returns the appropriate data type for its corresponding multisig operation.
base_layer/wallet/src/transaction_service/multisig/session.rs (5)
1-111: Well-organized imports and struct definition.The imports are comprehensive and the generic struct definition properly encapsulates the required resources.
113-190: create_multisig_utxo implementation looks good.The method properly handles multisig UTXO creation with appropriate error handling and script construction.
192-243: Proper session ID generation and UTXO handling.The method correctly uses UUID for session ID generation and handles UTXO retrieval and decryption appropriately.
245-363: Well-implemented cryptographic parameter generation.The method properly generates member secrets with appropriate error handling and cryptographic operations.
365-493: Improved logging and error handling in encumber collection.The method now uses proper logging instead of println! and correctly propagates errors.
| Err(e) => { | ||
| eprintln!("\nError: Could not decrypt data! {}\n", e); | ||
| break; | ||
| }, |
There was a problem hiding this comment.
Complete the error handling improvements.
Some error paths still use eprintln! and break instead of proper error propagation.
Replace remaining error prints with proper error returns:
Err(e) => {
- eprintln!("\nError: Could not decrypt data! {}\n", e);
- break;
+ return Err(TransactionServiceError::Other(format!(
+ "Could not decrypt data for output {}: {}",
+ output_index, e
+ )));
},Also applies to: 1027-1030
🤖 Prompt for AI Agents
In base_layer/wallet/src/transaction_service/multisig/session.rs around lines
1003 to 1006 and 1027 to 1030, the error handling uses eprintln! and break which
should be replaced with proper error propagation. Change these error paths to
return the error using the appropriate Result error return syntax instead of
printing and breaking, ensuring consistent and idiomatic error handling
throughout the function.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (2)
2201-2244: Missing input validation as noted in previous reviews.The method is missing the validation that was recommended in previous review comments:
mshould be greater than 0mshould be less than or equal to the number of public keyspublic_keyslist should not be emptyutxo_commitment_hashshould not be empty
2445-2472: Multiple issues need to be addressed.This method has several problems that align with past review comments:
Missing session_id validation: Should validate that
message.session_idis not empty, similar to other methods.Unsafe .unwrap() usage: Lines 2464-2466 use
.unwrap()on serialization operations which can panic if serialization fails.Incorrect error message: Line 2470 says "Error creating multisig UTXO" but should be "Error signing multisig UTXO member".
Apply these fixes:
+ if message.session_id.is_empty() { + return Err(Status::invalid_argument("session_id cannot be empty".to_string())); + } let result = self .wallet .transaction_service .clone() .sign_multisig_utxo_member(message.session_id) .await; match result { Ok(signs) => Ok(Response::new(SignMultisigUtxoMemberResponse { signatures: signs .into_iter() - .map(|sign| MemberMultisigSignature { + .map(|sign| -> Result<MemberMultisigSignature, Status> { + Ok(MemberMultisigSignature { output_index: sign.output_index as u64, - metadata_signature: sign.metadata_signature.to_binary().unwrap(), - script_signature: sign.script_signature.to_binary().unwrap(), - script_offset: sign.script_offset.to_binary().unwrap(), + metadata_signature: sign.metadata_signature.to_binary() + .map_err(|e| Status::internal(format!("Failed to serialize metadata_signature: {}", e)))?, + script_signature: sign.script_signature.to_binary() + .map_err(|e| Status::internal(format!("Failed to serialize script_signature: {}", e)))?, + script_offset: sign.script_offset.to_binary() + .map_err(|e| Status::internal(format!("Failed to serialize script_offset: {}", e)))?, - }) - .collect(), + }) + }) + .collect::<Result<Vec<_>, _>>()?, })), - Err(e) => Err(Status::internal(format!("Error creating multisig UTXO: {}", e))), + Err(e) => Err(Status::internal(format!("Error signing multisig UTXO member: {}", e))),
🧹 Nitpick comments (4)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
2308-2443: Consider improving error handling approach.While the session_id validation is good, there are a few concerns with the error handling:
Use proper logging: Replace
eprintln!calls withwarn!orerror!using theLOG_TARGETconstant for consistency with the rest of the codebase.Silent failure handling: The
filter_mapapproach silently drops items with serialization failures. Consider whether it's better to return an error for the entire operation if any serialization fails, or at least log warnings about dropped items.- Err(e) => { - eprintln!("Error serializing input_stack: {}", e); - return None; - }, + Err(e) => { + warn!(target: LOG_TARGET, "Error serializing input_stack: {}", e); + return None; + },applications/minotari_app_grpc/proto/wallet.proto (3)
1155-1161: Missing documentation and request sequencing for the new multisig RPCsThe six newly-added RPCs have zero accompanying comments while the surrounding file is richly documented.
This breaks consistency and leaves future maintainers guessing at semantics, streaming vs unary behaviour, error models, required call order, etc.Add concise doc-blocks (similar to the preceding RPCs) that cover:
• Purpose and when each RPC must be invoked in the multisig flow
• Required/optional fields and validation rules
• Expected side-effects (e.g. transactions inserted, funds encumbered)
• Idempotency and error codesWithout this, client implementers will almost certainly mis-use the API.
1225-1226: Field name should be plural or singular consistentlyrepeated MultisigEncumberOutput encumber_output = 1;Because the field is
repeated, rename toencumber_outputsto match established protobuf naming conventions (connected_peers,transactions, etc.) and improve readability.
1233-1234: Consider returning a map keyed byoutput_indexfor signatures
repeated MemberMultisigSignature signatures = 1;Multisig signing is inherently index-addressable. Returning a
map<uint32, MemberMultisigSignature>(or embeddingoutput_indexas the map key) removes the need for clients to search the list for a given index and eliminates duplication risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
applications/minotari_app_grpc/proto/wallet.proto(1 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(8 hunks)base_layer/wallet/src/transaction_service/handle.rs(7 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(1 hunks)clients/rust/base_node_wallet_client/src/client/http.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
- clients/rust/base_node_wallet_client/src/client/http.rs
- base_layer/wallet/src/transaction_service/handle.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (8)
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
Learnt from: ksrichard
PR: #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.
Learnt from: hansieodendaal
PR: #7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: #7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of tokio::task::spawn(async move { with tokio::task::spawn_blocking().await in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: hansieodendaal
PR: #7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, HashOutput is a type alias for FixedHash, and FixedHash already implements Display trait with hex formatting by calling self.0.to_hex() internally. This means when formatting HashOutput values, there's no need to explicitly call .to_hex() as the Display implementation handles hex conversion automatically.
applications/minotari_app_grpc/proto/wallet.proto (3)
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
Learnt from: ksrichard
PR: #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.
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (4)
base_layer/wallet/src/transaction_service/offline_signing/models.rs (2)
to_json(69-71)from_json(47-67)base_layer/wallet/src/transaction_service/handle.rs (6)
create_multisig_utxo(1524-1547)new(803-811)generate_multisig_member_secrets(1574-1588)collect_multisig_utxo_encumber(1590-1604)sign_multisig_utxo_member(1606-1620)send_multisig_utxo(1622-1633)base_layer/wallet/src/transaction_service/multisig/session.rs (2)
create_multisig_utxo(113-190)collect_multisig_utxo_encumber(365-493)base_layer/common_types/src/tari_address/mod.rs (2)
from_hex(400-403)from_str(409-419)
🔇 Additional comments (7)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (6)
49-50: Clean imports for multisig functionality.The new imports for multisig gRPC types are well-organized and follow the existing pattern in the file.
Also applies to: 54-55, 59-60, 94-95, 104-105, 110-113
777-781: Good explicit trait method call.The explicit call to the
TransactionResulttrait'sto_jsonmethod improves code clarity and avoids potential ambiguity.
811-811: Consistent explicit trait method call.The explicit call to the
TransactionResulttrait'sfrom_jsonmethod is consistent with the previous method and improves code clarity.
2154-2199: Well-implemented multisig UTXO creation method.The method properly validates all input parameters, handles public key parsing with appropriate error messages, and follows the established pattern for gRPC methods in this service. The validation logic addresses the requirements from previous reviews.
2246-2306: Excellent error handling for serialization operations.This method demonstrates best practices with proper session_id validation and comprehensive error handling for all
to_binary()calls. The detailed error messages will help with debugging serialization issues.
2474-2496: Well-implemented multisig UTXO sending method.This method properly validates the session_id, handles errors appropriately, and has a correct error message that reflects the operation being performed.
applications/minotari_app_grpc/proto/wallet.proto (1)
1205-1210: Ambiguous types inStartMultisigUtxoTransactionResponse
repeated string utxosandrepeated string commitmentsdo not state what encoding they use (hex? base58?). Moreover,value/commitment_maskalready provide amount + mask, so returning raw commitments may be redundant.Consider either:
- Return well-typed
repeated bytes commitmentsonly, or- Wrap these in a message with explicit fields (e.g.
MultisigOutput).Clarifying this now avoids breaking-change churn later.
| message CreateMultisigUtxoRequest { | ||
| //The commitment hash of the multisig UTXO | ||
| uint64 amount = 1; | ||
|
|
||
| // Address of the recipient | ||
| string recipient_address = 2; | ||
|
|
||
| // How many signatures are required to spend the multisig UTXO | ||
| uint32 m = 3; | ||
|
|
||
| // List of public keys of the parties involved in the multisig | ||
| repeated string public_keys = 4; | ||
| } |
There was a problem hiding this comment.
Comment / field mismatch: amount is documented as a “commitment hash”
The comment says “The commitment hash of the multisig UTXO” yet the field is:
uint64 amount = 1;A uint64 cannot carry a hash. Either:
- The field should actually be the value (amount) – update the comment, or
- The field should be
bytes utxo_commitment_hash = 1;– and a separateuint64 amountadded.
Leaving this unchecked will confuse users and generate incorrect proto bindings.
Proposed diff if option 2 is intended:
-message CreateMultisigUtxoRequest {
- //The commitment hash of the multisig UTXO
- uint64 amount = 1;
+message CreateMultisigUtxoRequest {
+ // Commitment hash of the multisig UTXO (32-byte Blake2b, hex-encoded)
+ bytes utxo_commitment_hash = 1;
+ // Amount locked in the multisig output (µT)
+ uint64 amount = 2;
- string recipient_address = 2;
- uint32 m = 3;
- repeated string public_keys = 4;
+ string recipient_address = 3;
+ uint32 m = 4;
+ repeated bytes public_keys = 5;
}🤖 Prompt for AI Agents
In applications/minotari_app_grpc/proto/wallet.proto around lines 1173 to 1185,
the field `amount` is incorrectly documented as a "commitment hash" but is typed
as uint64, which cannot represent a hash. To fix this, either update the comment
to reflect that the field represents the amount value, or change the field to
`bytes utxo_commitment_hash = 1;` and add a separate `uint64 amount` field if
both are needed. Ensure the field name, type, and comment are consistent to
avoid confusion and incorrect proto bindings.
| message StartMultisigUtxoTransactionRequest { | ||
| //The commitment hash of the multisig UTXO | ||
| string utxo_commitment_hash = 1; | ||
|
|
||
| // Address of the recipient | ||
| string recipient_address = 2; | ||
|
|
||
| // How many signatures are required to spend the multisig UTXO | ||
| uint32 m = 3; | ||
|
|
||
| // List of public keys of the parties involved in the multisig | ||
| repeated string public_keys = 4; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use bytes not string for binary public keys & commitments
public_keys and utxo_commitment_hash are binary objects. Encoding them as string silently relies on a side-channel (base58 / hex) that is undocumented and hard to validate.
• Switch to bytes (as is done elsewhere for payment_reference, signature, etc.).
• Specify the expected encoding/length in the comments.
This keeps protobuf APIs language-agnostic and avoids accidental UTF-8 mangling.
🤖 Prompt for AI Agents
In applications/minotari_app_grpc/proto/wallet.proto between lines 1191 and
1203, the fields utxo_commitment_hash and public_keys are currently typed as
string but represent binary data. Change their types from string to bytes to
correctly represent binary data and avoid encoding issues. Additionally, update
the comments to specify the expected encoding or length for these bytes fields
to clarify their format and usage.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
applications/minotari_app_grpc/proto/wallet.proto (2)
1377-1389: Comment/documentation still mismatched with field typeThe comment says “The commitment hash of the multisig UTXO” yet
amountis auint64.
Either rename the field (amount) and update the comment to reflect that this is a value in µT or change the type tobytes utxo_commitment_hash.
Same issue forpublic_keys: these are binary public keys and should bebytes, notstring.
This was highlighted in a previous review and remains unresolved.
1395-1407: Binary fields encoded asstring– usebytes
utxo_commitment_hashand each element ofpublic_keysare binary (32-byte commitment / public key). Encoding them as UTF-8 strings is lossy and undefined.- string utxo_commitment_hash = 1; + bytes utxo_commitment_hash = 1; // 32 bytes, Blake2b-256 - repeated string public_keys = 4; + repeated bytes public_keys = 4; // 32-byte compressed pubkeys
🧹 Nitpick comments (1)
applications/minotari_app_grpc/proto/wallet.proto (1)
1239-1244: Add minimal doc-comments to new RPCs for parity with the rest of the serviceAll pre-existing RPCs have explanatory block comments. The six new multisig calls have none, which is an abrupt style break and will render autogenerated API docs incomplete. At minimum document purpose, request/response messages, and an example.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
applications/minotari_app_grpc/proto/wallet.proto(3 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
applications/minotari_app_grpc/proto/wallet.proto (4)
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy.
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
Learnt from: ksrichard
PR: #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.
| message CreateMultisigUtxoResponse { | ||
| string tx_id = 1; | ||
| } |
There was a problem hiding this comment.
tx_id type is inconsistent with the rest of the API
Elsewhere every transaction identifier is uint64. Returning it as string here breaks type-consistency, generates awkward language bindings, and invites parsing bugs.
-message CreateMultisigUtxoResponse {
- string tx_id = 1;
+message CreateMultisigUtxoResponse {
+ uint64 tx_id = 1;
}📝 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.
| message CreateMultisigUtxoResponse { | |
| string tx_id = 1; | |
| } | |
| message CreateMultisigUtxoResponse { | |
| - string tx_id = 1; | |
| + uint64 tx_id = 1; | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_app_grpc/proto/wallet.proto at lines 1391 to 1393, the
field tx_id in CreateMultisigUtxoResponse is defined as a string, which is
inconsistent with the rest of the API where transaction identifiers use uint64.
Change the type of tx_id from string to uint64 to maintain type consistency,
improve language bindings, and avoid parsing errors.
| repeated string utxos = 1; | ||
| repeated string commitments = 2; | ||
| uint64 value = 3; | ||
| bytes commitment_mask = 4; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated string outputs/commitments should be bytes and consider dedicated message
utxos and commitments are binary digests; return them as bytes, not string. Better yet, wrap the pair in a small message to keep indices aligned with meaning.
- repeated string utxos = 1;
- repeated string commitments = 2;
+ repeated bytes utxos = 1; // commitment hashes of inputs
+ repeated bytes commitments = 2; // commitment hashes of outputs📝 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.
| repeated string utxos = 1; | |
| repeated string commitments = 2; | |
| uint64 value = 3; | |
| bytes commitment_mask = 4; | |
| } | |
| repeated bytes utxos = 1; // commitment hashes of inputs | |
| repeated bytes commitments = 2; // commitment hashes of outputs | |
| uint64 value = 3; | |
| bytes commitment_mask = 4; | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_app_grpc/proto/wallet.proto around lines 1410 to 1414,
the fields `utxos` and `commitments` are currently defined as repeated strings
but should be repeated bytes since they represent binary digests. Change their
types from `string` to `bytes`. Additionally, create a dedicated message that
wraps each pair of utxo and commitment together to maintain alignment and
clarity, then use a repeated field of that message instead of separate repeated
fields.
| message SendMultisigUtxoResponse { | ||
| repeated string tx_ids = 1; | ||
| } |
There was a problem hiding this comment.
Return uint64 for tx_ids, not string
Transaction identifiers are numeric everywhere else. Keeping them as uint64 avoids client-side parsing and preserves sort order.
-message SendMultisigUtxoResponse {
- repeated string tx_ids = 1;
+message SendMultisigUtxoResponse {
+ repeated uint64 tx_ids = 1;
}📝 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.
| message SendMultisigUtxoResponse { | |
| repeated string tx_ids = 1; | |
| } | |
| message SendMultisigUtxoResponse { | |
| repeated uint64 tx_ids = 1; | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_app_grpc/proto/wallet.proto at lines 1444 to 1446, the
field tx_ids in the SendMultisigUtxoResponse message is currently defined as
repeated string, but it should be changed to repeated uint64 to match the
numeric type used elsewhere for transaction identifiers. Update the field type
from string to uint64 to avoid client-side parsing and maintain correct sort
order.
Description
Motivation and Context
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
.dockerignoreand.gitignoreto handle new wallet data directories.