From 9d5874dbe3de75e0a9ab94abb5b96d7f8a1c2e08 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Tue, 17 Sep 2024 16:59:50 +0200 Subject: [PATCH] review comments --- base_layer/wallet_ffi/src/error.rs | 6 ++ base_layer/wallet_ffi/src/lib.rs | 32 +++++++++- base_layer/wallet_ffi/wallet.h | 2 + clients/ffi_client/index.js | 1 + clients/ffi_client/recovery.js | 1 + comms/core/src/net_address/mod.rs | 2 +- comms/core/src/net_address/multiaddr_range.rs | 64 +++++-------------- integration_tests/src/ffi/comms_config.rs | 1 + integration_tests/src/ffi/ffi_import.rs | 1 + 9 files changed, 58 insertions(+), 52 deletions(-) diff --git a/base_layer/wallet_ffi/src/error.rs b/base_layer/wallet_ffi/src/error.rs index 187ff1e5d1..e90770be83 100644 --- a/base_layer/wallet_ffi/src/error.rs +++ b/base_layer/wallet_ffi/src/error.rs @@ -57,6 +57,8 @@ pub enum InterfaceError { InvalidEmojiId, #[error("An error has occurred due to an invalid argument: `{0}`")] InvalidArgument(String), + #[error("An internal error has occurred: `{0}`")] + InternalError(String), #[error("Balance Unavailable")] BalanceError, } @@ -106,6 +108,10 @@ impl From for LibWalletError { code: 9, message: format!("Pointer error on {}:{:?}", p, v), }, + InterfaceError::InternalError(_) => Self { + code: 10, + message: format!("{:?}", v), + }, } } } diff --git a/base_layer/wallet_ffi/src/lib.rs b/base_layer/wallet_ffi/src/lib.rs index 358e358d23..bd7e6f618c 100644 --- a/base_layer/wallet_ffi/src/lib.rs +++ b/base_layer/wallet_ffi/src/lib.rs @@ -126,6 +126,7 @@ use tari_common_types::{ }; use tari_comms::{ multiaddr::Multiaddr, + net_address::{MultiaddrRange, MultiaddrRangeList, IP4_TCP_TEST_ADDR_RANGE}, peer_manager::{NodeIdentity, PeerQuery}, transports::MemoryTransport, types::CommsPublicKey, @@ -5200,6 +5201,7 @@ pub unsafe extern "C" fn transport_config_destroy(transport: *mut TariTransportC /// `database_path` - The database path char array pointer which. This is the folder path where the /// database files will be created and the application has write access to /// `discovery_timeout_in_secs`: specify how long the Discovery Timeout for the wallet is. +/// `exclude_dial_test_addresses`: exclude dialing of test addresses; this should be 'true' for production wallets /// `error_out` - Pointer to an int which will be modified to an error code should one occur, may not be null. Functions /// as an out parameter. /// @@ -5218,6 +5220,7 @@ pub unsafe extern "C" fn comms_config_create( datastore_path: *const c_char, discovery_timeout_in_secs: c_ulonglong, saf_message_duration_in_secs: c_ulonglong, + exclude_dial_test_addresses: bool, error_out: *mut c_int, ) -> *mut TariCommsConfig { let mut error = 0; @@ -5295,6 +5298,20 @@ pub unsafe extern "C" fn comms_config_create( MultiaddrList::from(vec![public_address]) }; + let excluded_dial_addresses = if exclude_dial_test_addresses { + let multi_addr_range = match MultiaddrRange::from_str(IP4_TCP_TEST_ADDR_RANGE) { + Ok(val) => val, + Err(e) => { + error = LibWalletError::from(InterfaceError::InternalError(e)).code; + ptr::swap(error_out, &mut error as *mut c_int); + return ptr::null_mut(); + }, + }; + MultiaddrRangeList::from(vec![multi_addr_range]) + } else { + MultiaddrRangeList::from(vec![]) + }; + let config = TariCommsConfig { override_from: None, public_addresses: addresses, @@ -5327,8 +5344,7 @@ pub unsafe extern "C" fn comms_config_create( minimum_desired_tcpv4_node_ratio: 0.0, ..Default::default() }, - // FIXME: This should be set to 'IP4_TCP_TEST_ADDR_RANGE', but cucumber tests need to use that range - excluded_dial_addresses: vec![].into(), + excluded_dial_addresses, ..Default::default() }, allow_test_addresses: true, @@ -10239,6 +10255,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -10403,6 +10420,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -10630,6 +10648,7 @@ mod test { db_path_str, 20, 10800, + false, error_ptr, ); @@ -10693,6 +10712,7 @@ mod test { db_path_str, 20, 10800, + false, error_ptr, ); @@ -10776,6 +10796,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -10953,6 +10974,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -11091,6 +11113,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -11310,6 +11333,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -11536,6 +11560,7 @@ mod test { db_path_alice_str, 20, 10800, + false, error_ptr, ); @@ -11797,6 +11822,7 @@ mod test { db_path_str, 20, 10800, + false, error_ptr, ); let passphrase: *const c_char = CString::into_raw(CString::new("niao").unwrap()) as *const c_char; @@ -12177,6 +12203,7 @@ mod test { alice_db_path_str, 20, 10800, + false, error_ptr, ); let passphrase: *const c_char = CString::into_raw(CString::new("niao").unwrap()) as *const c_char; @@ -12241,6 +12268,7 @@ mod test { bob_db_path_str, 20, 10800, + false, error_ptr, ); let passphrase: *const c_char = CString::into_raw(CString::new("niao").unwrap()) as *const c_char; diff --git a/base_layer/wallet_ffi/wallet.h b/base_layer/wallet_ffi/wallet.h index 21a13dc6e4..c8419af904 100644 --- a/base_layer/wallet_ffi/wallet.h +++ b/base_layer/wallet_ffi/wallet.h @@ -2758,6 +2758,7 @@ void transport_config_destroy(TariTransportConfig *transport); * `database_path` - The database path char array pointer which. This is the folder path where the * database files will be created and the application has write access to * `discovery_timeout_in_secs`: specify how long the Discovery Timeout for the wallet is. + * `exclude_dial_test_addresses`: exclude dialing of test addresses; this should be 'true' for production wallets * `error_out` - Pointer to an int which will be modified to an error code should one occur, may not be null. Functions * as an out parameter. * @@ -2774,6 +2775,7 @@ TariCommsConfig *comms_config_create(const char *public_address, const char *datastore_path, unsigned long long discovery_timeout_in_secs, unsigned long long saf_message_duration_in_secs, + bool exclude_dial_test_addresses, int *error_out); /** diff --git a/clients/ffi_client/index.js b/clients/ffi_client/index.js index bfb30fdea1..6faea7baee 100644 --- a/clients/ffi_client/index.js +++ b/clients/ffi_client/index.js @@ -39,6 +39,7 @@ try { "./wallet", 30, 600, + false, err ); diff --git a/clients/ffi_client/recovery.js b/clients/ffi_client/recovery.js index 02875dea0a..286e4a5943 100644 --- a/clients/ffi_client/recovery.js +++ b/clients/ffi_client/recovery.js @@ -53,6 +53,7 @@ try { "./recovery", 30, 600, + false, err ); diff --git a/comms/core/src/net_address/mod.rs b/comms/core/src/net_address/mod.rs index 34e2645984..1219f0d95a 100644 --- a/comms/core/src/net_address/mod.rs +++ b/comms/core/src/net_address/mod.rs @@ -29,4 +29,4 @@ mod mutliaddresses_with_stats; pub use mutliaddresses_with_stats::MultiaddressesWithStats; mod multiaddr_range; -pub use multiaddr_range::{serde_multiaddr_range, MultiaddrRange, MultiaddrRangeList, IP4_TCP_TEST_ADDR_RANGE}; +pub use multiaddr_range::{MultiaddrRange, MultiaddrRangeList, IP4_TCP_TEST_ADDR_RANGE}; diff --git a/comms/core/src/net_address/multiaddr_range.rs b/comms/core/src/net_address/multiaddr_range.rs index f30542b5be..1bbf2b913a 100644 --- a/comms/core/src/net_address/multiaddr_range.rs +++ b/comms/core/src/net_address/multiaddr_range.rs @@ -19,9 +19,9 @@ pub const IP4_TCP_TEST_ADDR_RANGE: &str = "/ip4/127.*.*.*/tcp/*"; /// A struct containing either an Ipv4AddrRange or a Multiaddr. If a range of IP addresses and/or ports needs to be /// specified, the MultiaddrRange can be used, but it only supports IPv4 addresses with the TCP protocol. #[derive(Debug, Clone, Serialize, PartialEq, Eq)] -pub struct MultiaddrRange { - ipv4_addr_range: Option, - multiaddr: Option, +pub enum MultiaddrRange { + Ipv4AddrRange(Ipv4AddrRange), + Multiaddr(Multiaddr), } impl FromStr for MultiaddrRange { @@ -29,15 +29,9 @@ impl FromStr for MultiaddrRange { fn from_str(s: &str) -> Result { if let Ok(multiaddr) = Multiaddr::from_str(s) { - Ok(MultiaddrRange { - ipv4_addr_range: None, - multiaddr: Some(multiaddr), - }) + Ok(MultiaddrRange::Multiaddr(multiaddr)) } else if let Ok(ipv4_addr_range) = Ipv4AddrRange::from_str(s) { - Ok(MultiaddrRange { - ipv4_addr_range: Some(ipv4_addr_range), - multiaddr: None, - }) + Ok(MultiaddrRange::Ipv4AddrRange(ipv4_addr_range)) } else { Err("Invalid format for both Multiaddr and Ipv4AddrRange".to_string()) } @@ -46,12 +40,9 @@ impl FromStr for MultiaddrRange { impl fmt::Display for MultiaddrRange { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if let Some(ipv4_addr_range) = &self.ipv4_addr_range { - write!(f, "{}", ipv4_addr_range) - } else if let Some(multiaddr) = &self.multiaddr { - write!(f, "{}", multiaddr) - } else { - write!(f, "None") + match self { + MultiaddrRange::Ipv4AddrRange(ipv4_addr_range) => write!(f, "{}", ipv4_addr_range), + MultiaddrRange::Multiaddr(multiaddr) => write!(f, "{}", multiaddr), } } } @@ -59,20 +50,17 @@ impl fmt::Display for MultiaddrRange { impl MultiaddrRange { /// Check if the given Multiaddr is contained within the MultiaddrRange range pub fn contains(&self, addr: &Multiaddr) -> bool { - if let Some(ipv4_addr_range) = &self.ipv4_addr_range { - return ipv4_addr_range.contains(addr); - } - if let Some(multiaddr) = &self.multiaddr { - return multiaddr == addr; + match self { + MultiaddrRange::Ipv4AddrRange(ipv4_addr_range) => ipv4_addr_range.contains(addr), + MultiaddrRange::Multiaddr(multiaddr) => multiaddr == addr, } - false } } -// ----------------- Ipv4AddrRange ----------------- -// A struct containing an Ipv4Range and a PortRange +/// ----------------- Ipv4AddrRange ----------------- +/// A struct containing an Ipv4Range and a PortRange #[derive(Debug, Clone, Serialize, PartialEq, Eq)] -struct Ipv4AddrRange { +pub struct Ipv4AddrRange { ip_range: Ipv4Range, port_range: PortRange, } @@ -280,6 +268,7 @@ impl fmt::Display for PortRange { } } +/// ----------------- MultiaddrRangeList ----------------- /// Supports deserialization from a sequence of strings or comma-delimited strings #[derive(Debug, Default, Clone, Serialize, PartialEq, Eq)] pub struct MultiaddrRangeList(Vec); @@ -397,29 +386,6 @@ impl<'de> Deserialize<'de> for MultiaddrRange { } } -pub mod serde_multiaddr_range { - use std::str::FromStr; - - use serde::{de::Error, Deserialize, Deserializer, Serializer}; - - use crate::net_address::MultiaddrRange; - - pub fn serialize(value: &[MultiaddrRange], serializer: S) -> Result - where S: Serializer { - let strings: Vec = value.iter().map(|v| v.to_string()).collect(); - serializer.serialize_str(&strings.join(",")) - } - - pub fn deserialize<'de, D>(deserializer: D) -> Result, D::Error> - where D: Deserializer<'de> { - let strings: Vec = Vec::deserialize(deserializer)?; - strings - .into_iter() - .map(|item| MultiaddrRange::from_str(&item).map_err(D::Error::custom)) - .collect() - } -} - #[cfg(test)] mod test { use std::{ diff --git a/integration_tests/src/ffi/comms_config.rs b/integration_tests/src/ffi/comms_config.rs index 2d1eb6e8da..b8de76866b 100644 --- a/integration_tests/src/ffi/comms_config.rs +++ b/integration_tests/src/ffi/comms_config.rs @@ -49,6 +49,7 @@ impl CommsConfig { CString::new(base_dir).unwrap().into_raw(), 30, 600, + false, // This needs to be 'false' for the tests to pass &mut error, ); if error > 0 { diff --git a/integration_tests/src/ffi/ffi_import.rs b/integration_tests/src/ffi/ffi_import.rs index 70830118ed..539c73ff7b 100644 --- a/integration_tests/src/ffi/ffi_import.rs +++ b/integration_tests/src/ffi/ffi_import.rs @@ -367,6 +367,7 @@ extern "C" { datastore_path: *const c_char, discovery_timeout_in_secs: c_ulonglong, saf_message_duration_in_secs: c_ulonglong, + exclude_dial_test_addresses: bool, error_out: *mut c_int, ) -> *mut TariCommsConfig; pub fn comms_config_destroy(wc: *mut TariCommsConfig);