-
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #311 +/- ##
==========================================
- Coverage 78.26% 73.73% -4.53%
==========================================
Files 32 33 +1
Lines 4840 4063 -777
==========================================
- Hits 3788 2996 -792
- Misses 1052 1067 +15 ☔ View full report in Codecov by Sentry. |
70bcf0e
to
4e0365f
Compare
refactor test_framework mod.rs to expose some common functions. some other changes in maker to make things consistent.
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
Here are some enhancements, nits, probable bugs , I have mentioned.
I have some ideas regarding better handling our threads in dns
& maker-cli
-> but I need some more time on that , so will mentioned about them in next round of review.
@@ -74,6 +76,9 @@ fn main() -> Result<(), MakerError> { | |||
Commands::NewAddress => { | |||
send_rpc_req(&RpcMsgReq::NewAddress)?; | |||
} | |||
Commands::Stop => { | |||
send_rpc_req(&RpcMsgReq::Stop)?; |
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.
Improvements in send_rpc_req
api:
- It could take the ownership of the
req
instead of taking its reference -> as the passedRpcMsgReq
variant is not used after calling this api. - We must not hardcode the
rpc_port
of makerd i.e127.0.0.1:6103
otherwise the maker-cli cannot connect to makerd in case the maker changes its rpc_port.
So IMO, create a new argument asmaker_rpc_port
with default set as127.0.0.1:6103
.
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.
Agreed..
@@ -175,7 +182,7 @@ impl TestFramework { | |||
thread::sleep(Duration::from_secs(5)); // Sleep for some time avoid resource unavailable error. | |||
Arc::new( | |||
Maker::init( | |||
Some(temp_dir.clone()), | |||
Some(temp_dir.join(port.0.to_string()).clone()), |
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.
- Cloning is not required.
Some(temp_dir.join(port.0.to_string()).clone()), | |
Some(temp_dir.join(port.0.to_string())), |
- Also Currently
generate_blocks
andsend_to_address
are methods oftest_framework
but now we have certain IT likecli-app tests
which do not useTestFramework
struct and thus they are not able to use these api's and have to write seperate api which are exactly same to these one.
So IMO, make these two api general purpose by modifying their function signature:
pub fn generate_blocks(&bitcoind, n: u64)
Similarly for send_to_address
too.
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...
@@ -39,7 +39,7 @@ use coinswap::{ | |||
wallet::RPCConfig, | |||
}; | |||
|
|||
fn get_random_tmp_dir() -> PathBuf { | |||
pub fn get_random_tmp_dir() -> PathBuf { |
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.
We don't require this api becuase:
- Can directly use
temp_dir().join(".coinswap")
to get the tempcoinswap
directory. - Here , a random alphanumeric chars are appended to get a unique temp
coinswap
directory which is not required as the contents of temp dir is removed if it already contains some data , thus ensuring that test often start with a empty temp dir.
@@ -37,16 +37,18 @@ enum Commands { | |||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
cli nit:
// Send to an external address and returns the transaction hex. | |
/// Send to an external address and returns the transaction hex. |
Otherwise this description will not appear while running the app.
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..
@@ -108,6 +108,8 @@ pub struct Maker { | |||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do?
Approach 1:
make this data_dir
field as public and then we don't require to have get_data_dir
api.
Approach 2:
make it private and have get_data_dir
api.
I feel, 2nd approach will be better.
what's your opinion?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will take care of it in future.
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 comment
The 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 comment
The 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.
GetDataDirResp(String), | ||
GetDataDirResp(PathBuf), |
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.
Any reason changing the type to PathBuf
?
I don't see any extra benefits on using PathBuf
.
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.
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.
@@ -11,7 +11,7 @@ use std::{path::PathBuf, str::FromStr, sync::Arc}; | |||
author = option_env ! ("CARGO_PKG_AUTHORS").unwrap_or(""))] | |||
struct Cli { | |||
/// Optional network type. | |||
#[clap(long, short = 'n', default_value = "clearnet", possible_values = &["tor", "clearnet"])] | |||
#[clap(long, short = 'n', default_value = "tor", possible_values = &["tor", "clearnet"])] | |||
network: String, | |||
/// Optional DNS data directory. Default value : "~/.coinswap/directory" |
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..
assert_eq!(seed_balance, "1000000 sats"); | ||
assert_eq!(swap_balance, "0 sats"); | ||
assert_eq!(fidelity_balance, "5000000 sats"); |
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.
Inside the start_makerd
api -> we fund the maker wallet with 0.1 BTC
-> then we spent 0.05 BTC
for Fidelity Bonds
+ 1000 sats
for miner fees -> then how we can get the seed-balance equals to the amount we fund the wallet.
I guess, there is some syncing problem.
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.
solved.
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.
yes.. that should not happen.. worth looking into it..
// TODO: Test Coverage | ||
Commands::GetTorAddress => { | ||
send_rpc_req(&RpcMsgReq::GetTorAddress)?; | ||
} | ||
// TODO: Test Coverage |
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.
It's been covered in maker-cli
test -> can remove them.
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..
Closing this pr as we now have #326 to extend it's work. |
Fixes #205
Adds other auxiliary changes as needed.