-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Maker Cli testing #311
Maker Cli testing #311
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -37,16 +37,18 @@ | |||||
FidelityBalance, | ||||||
/// Gets a new address | ||||||
NewAddress, | ||||||
// Send to an external wallet address. | ||||||
// Send to an external address and returns the transaction hex. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cli nit:
Suggested change
Otherwise this description will not appear while running the app. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done.. |
||||||
SendToAddress { | ||||||
address: String, | ||||||
amount: u64, | ||||||
fee: u64, | ||||||
}, | ||||||
/// Returns the tor address | ||||||
GetTorAddress, | ||||||
/// Returns the data dir | ||||||
/// Returns the data directory path | ||||||
GetDataDir, | ||||||
/// Stops the maker server | ||||||
Stop, | ||||||
} | ||||||
|
||||||
fn main() -> Result<(), MakerError> { | ||||||
|
@@ -95,12 +97,17 @@ | |||||
fee, | ||||||
})?; | ||||||
} | ||||||
// TODO: Test Coverage | ||||||
Commands::GetTorAddress => { | ||||||
send_rpc_req(&RpcMsgReq::GetTorAddress)?; | ||||||
} | ||||||
// TODO: Test Coverage | ||||||
Comment on lines
+100
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been covered in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done.. |
||||||
Commands::GetDataDir => { | ||||||
send_rpc_req(&RpcMsgReq::GetDataDir)?; | ||||||
} | ||||||
Commands::Stop => { | ||||||
send_rpc_req(&RpcMsgReq::Stop)?; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improvements in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed.. |
||||||
} | ||||||
} | ||||||
|
||||||
Ok(()) | ||||||
|
@@ -116,7 +123,7 @@ | |||||
let response_bytes = read_message(&mut stream)?; | ||||||
let response: RpcMsgResp = serde_cbor::from_slice(&response_bytes)?; | ||||||
|
||||||
println!("{:?}", response); | ||||||
println!("{}", response); | ||||||
|
||||||
Ok(()) | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,8 @@ | |
pub highest_fidelity_proof: RwLock<Option<FidelityProof>>, | ||
/// Is setup complete | ||
pub is_setup_complete: AtomicBool, | ||
/// Path for the data directory. | ||
pub data_dir: PathBuf, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should we do? Approach 1:make this Approach 2:make it private and have I feel, 2nd approach will be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot why I added the getter. If not required it can be removed. If we make this private all other fields should be private too. That is the right approach. But can be done later. Not a big need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, will take care of it in future. |
||
} | ||
|
||
#[allow(clippy::too_many_arguments)] | ||
|
@@ -141,15 +143,7 @@ | |
}; | ||
|
||
// Get provided data directory or the default data directory. | ||
let data_dir = if cfg!(feature = "integration-test") { | ||
// We only append port number in data-dir for integration test | ||
let port = port.expect("port value expected in Int tests"); | ||
data_dir.map_or(get_maker_dir().join(port.to_string()), |d| { | ||
d.join("maker").join(port.to_string()) | ||
}) | ||
} else { | ||
data_dir.unwrap_or(get_maker_dir()) | ||
}; | ||
let data_dir = data_dir.unwrap_or(get_maker_dir()); | ||
|
||
let wallet_dir = data_dir.join("wallets"); | ||
|
||
|
@@ -222,9 +216,14 @@ | |
connection_state: Mutex::new(HashMap::new()), | ||
highest_fidelity_proof: RwLock::new(None), | ||
is_setup_complete: AtomicBool::new(false), | ||
data_dir, | ||
}) | ||
} | ||
|
||
pub fn get_data_dir(&self) -> &PathBuf { | ||
&self.data_dir | ||
} | ||
|
||
/// Returns a reference to the Maker's wallet. | ||
pub fn get_wallet(&self) -> &RwLock<Wallet> { | ||
&self.wallet | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,7 +43,7 @@ impl Default for MakerConfig { | |
time_relative_fee_ppb: Amount::from_sat(100_000), | ||
min_size: 10_000, | ||
socks_port: 19050, | ||
directory_server_address: "directoryhiddenserviceaddress.onion:8080".to_string(), | ||
directory_server_address: "127.0.0.1:8080".to_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because clearnet connection won't work without setting the maker config DNS address. At least this will make default config work for local clearnet. For tor default value doesn't make sense anyway. |
||
fidelity_value: 5_000_000, // 5 million sats | ||
fidelity_timelock: 26_000, // Approx 6 months of blocks | ||
connection_type: ConnectionType::TOR, | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||||||
use std::{fmt::Display, path::PathBuf}; | ||||||||
|
||||||||
use bitcoind::bitcoincore_rpc::json::ListUnspentResultEntry; | ||||||||
use serde::{Deserialize, Serialize}; | ||||||||
|
||||||||
|
@@ -20,6 +22,7 @@ | |||||||
}, | ||||||||
GetTorAddress, | ||||||||
GetDataDir, | ||||||||
Stop, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a potential enhancement required here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bad idea to use ctrl+c anytime. If there's a waiting process it should just wait for it to finish then terminate. |
||||||||
} | ||||||||
|
||||||||
#[derive(Serialize, Deserialize, Debug)] | ||||||||
|
@@ -36,5 +39,27 @@ | |||||||
NewAddressResp(String), | ||||||||
SendToAddressResp(String), | ||||||||
GetTorAddressResp(String), | ||||||||
GetDataDirResp(String), | ||||||||
GetDataDirResp(PathBuf), | ||||||||
Comment on lines
-39
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason changing the type to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah doesn't matter. could be string as well. I would still like PathBuf as it tells its a path info, not any random string. |
||||||||
Shutdown, | ||||||||
} | ||||||||
|
||||||||
impl Display for RpcMsgResp { | ||||||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||||||||
match self { | ||||||||
Self::Pong => write!(f, "Pong"), | ||||||||
Self::NewAddressResp(addr) => write!(f, "{}", addr), | ||||||||
Self::SeedBalanceResp(bal) => write!(f, "{} sats", bal), | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: To remain consistency in cli->
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't conventions. Most wallets use "sats" as the unit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for this change is that
|
||||||||
Self::ContractBalanceResp(bal) => write!(f, "{} sats", bal), | ||||||||
Self::SwapBalanceResp(bal) => write!(f, "{} sats", bal), | ||||||||
Self::FidelityBalanceResp(bal) => write!(f, "{} sats", bal), | ||||||||
Self::SeedUtxoResp { utxos } => write!(f, "{:?}", utxos), | ||||||||
Self::SwapUtxoResp { utxos } => write!(f, "{:?}", utxos), | ||||||||
Self::FidelityUtxoResp { utxos } => write!(f, "{:?}", utxos), | ||||||||
Self::ContractUtxoResp { utxos } => write!(f, "{:?}", utxos), | ||||||||
Self::SendToAddressResp(tx_hex) => write!(f, "{:?}", tx_hex), | ||||||||
Self::GetTorAddressResp(addr) => write!(f, "{:?}", addr), | ||||||||
Self::GetDataDirResp(path) => write!(f, "{:?}", path), | ||||||||
Self::Shutdown => write!(f, "Shutdown Initiated"), | ||||||||
} | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
use std::{ | ||
io::ErrorKind, | ||
fs::File, | ||
io::{ErrorKind, Read}, | ||
net::{TcpListener, TcpStream}, | ||
path::PathBuf, | ||
sync::{atomic::Ordering::Relaxed, Arc}, | ||
thread::sleep, | ||
time::Duration, | ||
|
@@ -10,7 +12,7 @@ | |
|
||
use crate::{ | ||
maker::{error::MakerError, rpc::messages::RpcMsgResp, Maker}, | ||
utill::{get_maker_dir, get_tor_addrs, read_message, send_message}, | ||
utill::{read_message, send_message, ConnectionType}, | ||
wallet::{Destination, SendAmount}, | ||
}; | ||
use std::str::FromStr; | ||
|
@@ -144,16 +146,37 @@ | |
}; | ||
} | ||
RpcMsgReq::GetDataDir => { | ||
let path = get_maker_dir().display().to_string(); | ||
let resp = RpcMsgResp::GetDataDirResp(path); | ||
let path = maker.get_data_dir(); | ||
let resp = RpcMsgResp::GetDataDirResp(path.clone()); | ||
if let Err(e) = send_message(socket, &resp) { | ||
log::info!("Error sending RPC response {:?}", e); | ||
}; | ||
} | ||
RpcMsgReq::GetTorAddress => { | ||
let path = get_maker_dir().join("tor"); | ||
let resp = RpcMsgResp::GetTorAddressResp(get_tor_addrs(&path)?); | ||
if let Err(e) = send_message(socket, &resp) { | ||
if maker.config.connection_type == ConnectionType::CLEARNET { | ||
let resp = RpcMsgResp::GetTorAddressResp("Maker Tor is not running".to_string()); | ||
if let Err(e) = send_message(socket, &resp) { | ||
log::info!("Error sending RPC response {:?}", e); | ||
}; | ||
} else { | ||
let maker_hs_path_str = | ||
format!("/tmp/tor-rust-maker{}/hs-dir/hostname", maker.config.port); | ||
let maker_hs_path = PathBuf::from(maker_hs_path_str); | ||
let mut maker_file = File::open(maker_hs_path)?; | ||
Comment on lines
+164
to
+165
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of first creating a Pathbuf instance from string and then passing it to let mut maker_file = File::open(maker_hs_path_str)?; This will work becuase File::open considers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed. |
||
let mut maker_onion_addr: String = String::new(); | ||
maker_file.read_to_string(&mut maker_onion_addr)?; | ||
maker_onion_addr.pop(); | ||
let maker_address = format!("{}:{}", maker_onion_addr, maker.config.port); | ||
|
||
let resp = RpcMsgResp::GetTorAddressResp(maker_address); | ||
if let Err(e) = send_message(socket, &resp) { | ||
log::info!("Error sending RPC response {:?}", e); | ||
}; | ||
} | ||
} | ||
RpcMsgReq::Stop => { | ||
maker.shutdown.store(true, Relaxed); | ||
if let Err(e) = send_message(socket, &RpcMsgResp::Shutdown) { | ||
log::info!("Error sending RPC response {:?}", e); | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,4 @@ | ||||||||
#![cfg(feature = "integration-test")] | ||||||||
use std::{ | ||||||||
io::{BufRead, BufReader, Write}, | ||||||||
net::TcpStream, | ||||||||
|
@@ -13,35 +14,42 @@ use std::{ | |||||||
fn start_server() -> (Child, Receiver<String>) { | ||||||||
let (log_sender, log_receiver): (Sender<String>, Receiver<String>) = mpsc::channel(); | ||||||||
let mut directoryd_process = Command::new("./target/debug/directoryd") | ||||||||
.args(["-n", "clearnet"]) | ||||||||
.stdout(std::process::Stdio::piped()) | ||||||||
.stderr(std::process::Stdio::piped()) | ||||||||
.spawn() | ||||||||
.unwrap(); | ||||||||
|
||||||||
let stdout = directoryd_process.stdout.take().unwrap(); | ||||||||
let std_err = directoryd_process.stderr.take().unwrap(); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||||
thread::spawn(move || { | ||||||||
let reader = BufReader::new(stdout); | ||||||||
reader.lines().map_while(Result::ok).for_each(|line| { | ||||||||
println!("{}", line); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need of println statement. |
||||||||
log_sender.send(line).unwrap_or_else(|e| { | ||||||||
println!("Failed to send log: {}", e); | ||||||||
}); | ||||||||
}); | ||||||||
}); | ||||||||
|
||||||||
thread::spawn(move || { | ||||||||
let reader = BufReader::new(std_err); | ||||||||
reader.lines().map_while(Result::ok).for_each(|line| { | ||||||||
panic!("Error : {}", line); | ||||||||
}) | ||||||||
}); | ||||||||
|
||||||||
(directoryd_process, log_receiver) | ||||||||
} | ||||||||
|
||||||||
fn wait_for_server_start(log_receiver: &Receiver<String>) { | ||||||||
let mut server_started = false; | ||||||||
while let Ok(log_message) = log_receiver.recv_timeout(Duration::from_secs(5)) { | ||||||||
loop { | ||||||||
let log_message = log_receiver.recv().unwrap(); | ||||||||
if log_message.contains("RPC socket binding successful") { | ||||||||
server_started = true; | ||||||||
log::info!("DNS server started"); | ||||||||
break; | ||||||||
} | ||||||||
} | ||||||||
assert!( | ||||||||
server_started, | ||||||||
"Server did not start within the expected time" | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
fn send_addresses(addresses: &[&str]) { | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change default directory from
~/.coinswap/directory
to~/.coinswap/dns
.as we always create default dir as
~/.coinswap/dns
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack..