chore: review wallet based TODOs (#3825)

Description
---
This PR is part of a review of the TODO statements in the code related to the wallet. In this PR I have gone through the TODOs related to the wallet and I have dealt with them in one of three ways

1. Deleted them if I don’t think they are relevant anymore. In this case I will call them out in the PR to be reviewed specifically with a PR comment
2. Fixed them if it felt like a quick fix, I will also call out this fix in a PR comment
3. Logged them in an issue for future attention. In this case I tagged them with the #LOGGED tag

applications/tari_app_grpc/src/conversions/output_features.rs

@@ -26,7 +26,7 @@ use tari_common_types::{
     array::copy_into_fixed_array,
     types::{Commitment, PublicKey},
 };
-use tari_core::transactions::transaction::{
+use tari_core::transactions::transaction_components::{
     AssetOutputFeatures,
     MintNonFungibleFeatures,
     OutputFeatures,

applications/tari_app_grpc/src/conversions/transaction.rs

@@ -26,7 +26,7 @@ use std::{
 };
 
 use tari_common_types::transaction::{TransactionDirection, TransactionStatus, TxId};
-use tari_core::transactions::transaction::Transaction;
+use tari_core::transactions::transaction_components::Transaction;
 use tari_crypto::ristretto::RistrettoSecretKey;
 use tari_utilities::ByteArray;
 

applications/tari_app_grpc/src/conversions/transaction_input.rs

@@ -25,7 +25,7 @@ use std::convert::{TryFrom, TryInto};
 use tari_common_types::types::{Commitment, PublicKey};
 use tari_core::{
     covenants::Covenant,
-    transactions::transaction::{TransactionInput, TransactionInputVersion},
+    transactions::transaction_components::{TransactionInput, TransactionInputVersion},
 };
 use tari_crypto::{
     script::{ExecutionStack, TariScript},

applications/tari_app_grpc/src/conversions/transaction_kernel.rs

@@ -25,7 +25,7 @@ use std::convert::{TryFrom, TryInto};
 use tari_common_types::types::Commitment;
 use tari_core::transactions::{
     tari_amount::MicroTari,
-    transaction::{KernelFeatures, TransactionKernel, TransactionKernelVersion},
+    transaction_components::{KernelFeatures, TransactionKernel, TransactionKernelVersion},
 };
 use tari_crypto::tari_utilities::{ByteArray, Hashable};
 

applications/tari_app_grpc/src/conversions/transaction_output.rs

@@ -25,7 +25,7 @@ use std::convert::{TryFrom, TryInto};
 use tari_common_types::types::{BulletRangeProof, Commitment, PublicKey};
 use tari_core::{
     covenants::Covenant,
-    transactions::transaction::{TransactionOutput, TransactionOutputVersion},
+    transactions::transaction_components::{TransactionOutput, TransactionOutputVersion},
 };
 use tari_crypto::script::TariScript;
 use tari_utilities::{ByteArray, Hashable};

applications/tari_app_grpc/src/conversions/unblinded_output.rs

@@ -27,7 +27,7 @@ use tari_core::{
     covenants::Covenant,
     transactions::{
         tari_amount::MicroTari,
-        transaction::{TransactionOutputVersion, UnblindedOutput},
+        transaction_components::{TransactionOutputVersion, UnblindedOutput},
     },
 };
 use tari_crypto::script::{ExecutionStack, TariScript};

applications/tari_base_node/src/grpc/base_node_grpc_server.rs

@@ -48,7 +48,7 @@ use tari_core::{
     iterators::NonOverlappingIntegerPairIter,
     mempool::{service::LocalMempoolService, TxStorageResponse},
     proof_of_work::PowAlgorithm,
-    transactions::transaction::Transaction,
+    transactions::transaction_components::Transaction,
 };
 use tari_p2p::{auto_update::SoftwareUpdaterHandle, services::liveness::LivenessHandle};
 use tari_utilities::{hex::Hex, message_format::MessageFormat, ByteArray, Hashable};

applications/tari_console_wallet/src/automation/command_parser.rs

@@ -128,8 +128,8 @@ pub fn parse_command(command: &str) -> Result<ParsedCommand, ParseError> {
         CoinSplit => parse_coin_split(args)?,
         DiscoverPeer => parse_public_key(args)?,
         Whois => parse_whois(args)?,
-        ExportUtxos => parse_export_utxos(args)?, // todo: only show X number of utxos
-        ExportSpentUtxos => parse_export_spent_utxos(args)?, // todo: only show X number of utxos
+        ExportUtxos => parse_export_utxos(args)?,
+        ExportSpentUtxos => parse_export_spent_utxos(args)?,
         CountUtxos => Vec::new(),
         SetBaseNode => parse_public_key_and_address(args)?,
         SetCustomBaseNode => parse_public_key_and_address(args)?,

applications/tari_console_wallet/src/automation/commands.rs

@@ -44,7 +44,7 @@ use tari_comms::{
 use tari_comms_dht::{envelope::NodeDestination, DhtDiscoveryRequester};
 use tari_core::transactions::{
     tari_amount::{uT, MicroTari, Tari},
-    transaction::{TransactionOutput, UnblindedOutput},
+    transaction_components::{TransactionOutput, UnblindedOutput},
 };
 use tari_crypto::{
     keys::PublicKey as PublicKeyTrait,
@@ -113,7 +113,7 @@ pub struct SentTransaction {}
 fn get_transaction_parameters(
     args: Vec<ParsedArgument>,
 ) -> Result<(MicroTari, MicroTari, PublicKey, String), CommandError> {
-    // TODO: Consolidate "fee per gram" in codebase
+    // TODO: Consolidate "fee per gram" in codebase #LOGGED
     let fee_per_gram = 25 * uT;
 
     use ParsedArgument::*;
@@ -138,7 +138,7 @@ fn get_transaction_parameters(
 fn get_init_sha_atomic_swap_parameters(
     args: Vec<ParsedArgument>,
 ) -> Result<(MicroTari, MicroTari, PublicKey, String), CommandError> {
-    // TODO: Consolidate "fee per gram" in codebase
+    // TODO: Consolidate "fee per gram" in codebase #LOGGED
     let fee_per_gram = 25 * uT;
 
     use ParsedArgument::*;
@@ -801,7 +801,7 @@ pub async fn command_runner(
                 let name = parsed.args[0].to_string();
                 let message = format!("Register asset: {}", name);
                 let mut manager = wallet.asset_manager.clone();
-                // todo: key manager
+                // todo: key manager #LOGGED
                 let mut rng = rand::thread_rng();
                 let (_, public_key) = PublicKey::random_keypair(&mut rng);
                 let (tx_id, transaction) = manager

applications/tari_console_wallet/src/automation/error.rs

@@ -26,7 +26,7 @@ use log::*;
 use tari_common::exit_codes::{ExitCode, ExitError};
 use tari_core::transactions::{
     tari_amount::{MicroTariError, TariConversionError},
-    transaction::TransactionError,
+    transaction_components::TransactionError,
 };
 use tari_utilities::hex::HexError;
 use tari_wallet::{

applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs

@@ -79,7 +79,7 @@ use tari_common_types::{
 use tari_comms::{types::CommsPublicKey, CommsNode};
 use tari_core::transactions::{
     tari_amount::MicroTari,
-    transaction::{OutputFeatures, UnblindedOutput},
+    transaction_components::{OutputFeatures, UnblindedOutput},
 };
 use tari_crypto::{ristretto::RistrettoPublicKey, tari_utilities::Hashable};
 use tari_utilities::{hex::Hex, ByteArray};
@@ -739,8 +739,8 @@ impl wallet_server::Wallet for WalletGrpcServer {
         let mut transaction_service = self.wallet.transaction_service.clone();
         let message = request.into_inner();
 
-        // TODO: Clean up unwrap
-        let asset_public_key = PublicKey::from_bytes(message.asset_public_key.as_slice()).unwrap();
+        let asset_public_key =
+            PublicKey::from_bytes(message.asset_public_key.as_slice()).map_err(|e| Status::internal(e.to_string()))?;
         let asset = asset_manager
             .get_owned_asset_by_pub_key(&asset_public_key)
             .await

applications/tari_console_wallet/src/init/mod.rs

@@ -534,7 +534,7 @@ pub async fn start_wallet(
     base_node: &Peer,
     wallet_mode: &WalletMode,
 ) -> Result<(), ExitError> {
-    // TODO gRPC interfaces for setting base node
+    // TODO gRPC interfaces for setting base node #LOGGED
     debug!(target: LOG_TARGET, "Setting base node peer");
 
     let net_address = base_node

applications/tari_console_wallet/src/main.rs

@@ -242,10 +242,14 @@ fn enable_tracing() {
     global::set_text_map_propagator(opentelemetry_jaeger::Propagator::new());
     let tracer = opentelemetry_jaeger::new_pipeline()
         .with_service_name("tari::console_wallet")
-        .with_tags(vec![KeyValue::new("pid", process::id().to_string()), KeyValue::new("current_exe", env::current_exe().unwrap().to_str().unwrap_or_default().to_owned())])
-        // TODO: uncomment when using tokio 1
-        // .install_batch(opentelemetry::runtime::Tokio)
-        .install_simple()
+        .with_tags(vec![
+            KeyValue::new("pid", process::id().to_string()),
+            KeyValue::new(
+                "current_exe",
+                env::current_exe().unwrap().to_str().unwrap_or_default().to_owned(),
+            ),
+        ])
+        .install_batch(opentelemetry::runtime::Tokio)
         .unwrap();
     let telemetry = tracing_opentelemetry::layer().with_tracer(tracer);
     let subscriber = Registry::default().with(telemetry);

applications/tari_console_wallet/src/ui/components/notification_tab.rs

@@ -4,9 +4,9 @@
 // are cleared.
 // Currently notifications are only added from the wallet_event_monitor which has
 // add_notification method.
-// TODO: auto delete old notifications.
+// TODO: auto delete old notifications. #LOGGED
 // TODO: add interaction with the notifications, e.g. if I have a pending transaction
-//       notification, the UI should go there if I click on it.
+//       notification, the UI should go there if I click on it. #LOGGED
 
 use tari_comms::runtime::Handle;
 use tui::{

applications/tari_console_wallet/src/ui/components/send_tab.rs

@@ -401,7 +401,7 @@ impl SendTab {
                                     self.to_field.clone(),
                                     amount.into(),
                                     self.selected_unique_id.clone(),
-                                    None, // TODO: Select the actual value
+                                    None,
                                     fee_per_gram,
                                     self.message_field.clone(),
                                     tx,
@@ -419,7 +419,7 @@ impl SendTab {
                                     self.to_field.clone(),
                                     amount.into(),
                                     self.selected_unique_id.clone(),
-                                    None, // TODO: select actual value
+                                    None,
                                     fee_per_gram,
                                     self.message_field.clone(),
                                     tx,

applications/tari_console_wallet/src/ui/components/tabs_container.rs

@@ -91,7 +91,6 @@ impl<B: Backend> TabsContainer<B> {
 impl<B: Backend> Component<B> for TabsContainer<B> {
     fn draw(&mut self, _: &mut Frame<B>, _: Rect, _: &AppState) {
         // Use draw_titles and draw_content instead,
-        // TODO: Create a layout and draw both
         unimplemented!()
     }
 

applications/tari_console_wallet/src/ui/components/transactions_tab.rs

@@ -1,6 +1,7 @@
 use std::collections::HashMap;
 
 use chrono::{DateTime, Local};
+use log::*;
 use tari_common_types::transaction::{TransactionDirection, TransactionStatus};
 use tokio::runtime::Handle;
 use tui::{
@@ -19,6 +20,8 @@ use crate::ui::{
     MAX_WIDTH,
 };
 
+const LOG_TARGET: &str = "wallet::console_wallet::transaction_tab";
+
 pub struct TransactionsTab {
     balance: Balance,
     selected_tx_list: SelectedTransactionList,
@@ -571,8 +574,9 @@ impl<B: Backend> Component<B> for TransactionsTab {
             },
             // Rebroadcast
             'r' => {
-                // TODO: use this result
-                let _res = Handle::current().block_on(app_state.rebroadcast_all());
+                if let Err(e) = Handle::current().block_on(app_state.rebroadcast_all()) {
+                    error!(target: LOG_TARGET, "Error rebroadcasting transactions: {}", e);
+                }
             },
             'a' => app_state.toggle_abandoned_coinbase_filter(),
             '\n' => match self.selected_tx_list {

applications/tari_console_wallet/src/ui/state/app_state.rs

@@ -248,10 +248,7 @@ impl AppState {
     // Return alias or pub key if the contact is not in the list.
     pub fn get_alias(&self, pub_key: &RistrettoPublicKey) -> String {
         let pub_key_hex = format!("{}", pub_key);
-        // TODO: We can uncomment this to indicated unknown origin, otherwise there is our pub key.
-        // if self.get_identity().public_key == pub_key_hex {
-        //     return "Unknown".to_string();
-        // }
+
         match self
             .cached_data
             .contacts
@@ -533,7 +530,7 @@ impl AppState {
 
     pub fn get_default_fee_per_gram(&self) -> MicroTari {
         use Network::*;
-        // TODO: TBD
+        // TODO: TBD #LOGGED
         match self.node_config.network {
             MainNet | LocalNet | Igor | Dibbler => MicroTari(5),
             Ridcully | Stibbons | Weatherwax => MicroTari(25),

applications/tari_merge_mining_proxy/src/common/merge_mining.rs

@@ -25,7 +25,7 @@ use std::convert::{TryFrom, TryInto};
 use tari_app_grpc::tari_rpc as grpc;
 use tari_core::{
     blocks::NewBlockTemplate,
-    transactions::transaction::{TransactionKernel, TransactionOutput},
+    transactions::transaction_components::{TransactionKernel, TransactionOutput},
 };
 
 use crate::error::MmProxyError;

applications/tari_merge_mining_proxy/src/common/proxy.rs

@@ -76,7 +76,6 @@ pub fn into_body_from_response(resp: Response<json::Value>) -> Response<Body> {
 
 /// Reads the `Body` until there is no more to read
 pub async fn read_body_until_end(body: &mut Body) -> Result<BytesMut, MmProxyError> {
-    // TODO: Perhaps there is a more efficient way to do this
     let mut bytes = BytesMut::new();
     while let Some(data) = body.next().await {
         let data = data?;

applications/tari_stratum_transcoder/src/common/mining.rs

@@ -25,7 +25,7 @@ use std::convert::TryFrom;
 use tari_app_grpc::tari_rpc as grpc;
 use tari_core::{
     blocks::NewBlockTemplate,
-    transactions::transaction::{TransactionKernel, TransactionOutput},
+    transactions::transaction_components::{TransactionKernel, TransactionOutput},
 };
 
 use crate::error::StratumTranscoderProxyError;

applications/tari_stratum_transcoder/src/common/proxy.rs

@@ -56,7 +56,6 @@ pub fn into_body_from_response(resp: Response<json::Value>) -> Response<Body> {
 
 /// Reads the `Body` until there is no more to read
 pub async fn read_body_until_end(body: &mut Body) -> Result<BytesMut, StratumTranscoderProxyError> {
-    // TODO: Perhaps there is a more efficient way to do this
     let mut bytes = BytesMut::new();
     while let Some(data) = body.next().await {
         let data = data?;

applications/test_faucet/src/main.rs

@@ -19,7 +19,7 @@ use tari_core::{
         tari_amount::{MicroTari, T},
         test_helpers,
         test_helpers::generate_keys,
-        transaction::{KernelFeatures, OutputFeatures, TransactionKernel, TransactionOutput},
+        transaction_components::{KernelFeatures, OutputFeatures, TransactionKernel, TransactionOutput},
         CryptoFactories,
     },
 };

base_layer/core/src/base_node/comms_interface/comms_response.rs

@@ -34,7 +34,7 @@ use crate::{
     blocks::{Block, BlockHeader, ChainHeader, HistoricalBlock, NewBlockTemplate},
     chain_storage::UtxoMinedInfo,
     proof_of_work::Difficulty,
-    transactions::transaction::{Transaction, TransactionKernel, TransactionOutput},
+    transactions::transaction_components::{Transaction, TransactionKernel, TransactionOutput},
 };
 
 /// API Response enum

base_layer/core/src/base_node/comms_interface/local_interface.rs

@@ -40,7 +40,7 @@ use crate::{
     blocks::{Block, ChainHeader, HistoricalBlock, NewBlockTemplate},
     chain_storage::UtxoMinedInfo,
     proof_of_work::PowAlgorithm,
-    transactions::transaction::{TransactionKernel, TransactionOutput},
+    transactions::transaction_components::{TransactionKernel, TransactionOutput},
 };
 
 pub type BlockEventSender = broadcast::Sender<Arc<BlockEvent>>;

base_layer/core/src/base_node/rpc/service.rs

@@ -60,7 +60,7 @@ use crate::{
         },
         types::{Signature as SignatureProto, Transaction as TransactionProto},
     },
-    transactions::transaction::Transaction,
+    transactions::transaction_components::Transaction,
 };
 
 const LOG_TARGET: &str = "c::base_node::rpc";
@@ -432,7 +432,6 @@ impl<B: BlockchainBackend + 'static> BaseNodeWalletService for BaseNodeWalletRpc
 
         for position in message.mmr_positions {
             if position > u32::MAX as u64 {
-                // TODO: in future, bitmap may support higher than u32
                 return Err(RpcStatus::bad_request("position must fit into a u32"));
             }
             let position = position as u32;

base_layer/core/src/base_node/state_machine_service/states/horizon_state_sync/error.rs

@@ -33,7 +33,7 @@ use tokio::task;
 use crate::{
     base_node::{comms_interface::CommsInterfaceError, state_machine_service::states::helpers::BaseNodeRequestError},
     chain_storage::{ChainStorageError, MmrTree},
-    transactions::transaction::TransactionError,
+    transactions::transaction_components::TransactionError,
     validation::ValidationError,
 };
 

base_layer/core/src/base_node/state_machine_service/states/horizon_state_sync/horizon_state_synchronization.rs

@@ -65,7 +65,7 @@ use crate::{
         SyncUtxosRequest,
         SyncUtxosResponse,
     },
-    transactions::transaction::{TransactionKernel, TransactionOutput},
+    transactions::transaction_components::{TransactionKernel, TransactionOutput},
     validation::helpers,
 };
 

base_layer/core/src/blocks/block.rs

@@ -42,7 +42,7 @@ use crate::{
     transactions::{
         aggregated_body::AggregateBody,
         tari_amount::MicroTari,
-        transaction::{
+        transaction_components::{
             KernelFeatures,
             OutputFlags,
             Transaction,

base_layer/core/src/blocks/genesis_block.rs

@@ -37,7 +37,7 @@ use crate::{
     transactions::{
         aggregated_body::AggregateBody,
         tari_amount::MicroTari,
-        transaction::{KernelFeatures, OutputFeatures, OutputFlags, TransactionKernel, TransactionOutput},
+        transaction_components::{KernelFeatures, OutputFeatures, OutputFlags, TransactionKernel, TransactionOutput},
     },
 };
 

base_layer/core/src/chain_storage/async_db.rs

@@ -66,7 +66,7 @@ use crate::{
     },
     common::rolling_vec::RollingVec,
     proof_of_work::{PowAlgorithm, TargetDifficultyWindow},
-    transactions::transaction::{TransactionKernel, TransactionOutput},
+    transactions::transaction_components::{TransactionKernel, TransactionOutput},
 };
 
 const LOG_TARGET: &str = "c::bn::async_db";

base_layer/core/src/chain_storage/blockchain_backend.rs

@@ -30,7 +30,7 @@ use crate::{
         Reorg,
         UtxoMinedInfo,
     },
-    transactions::transaction::{TransactionInput, TransactionKernel},
+    transactions::transaction_components::{TransactionInput, TransactionKernel},
 };
 
 /// Identify behaviour for Blockchain database backends. Implementations must support `Send` and `Sync` so that

base_layer/core/src/chain_storage/blockchain_database.rs

@@ -79,7 +79,7 @@ use crate::{
     common::rolling_vec::RollingVec,
     consensus::{chain_strength_comparer::ChainStrengthComparer, ConsensusConstants, ConsensusManager},
     proof_of_work::{monero_rx::MoneroPowData, PowAlgorithm, TargetDifficultyWindow},
-    transactions::transaction::{TransactionInput, TransactionKernel},
+    transactions::transaction_components::{TransactionInput, TransactionKernel},
     validation::{
         helpers::calc_median_timestamp,
         DifficultyCalculator,