adex tool was introduced

[rozhkovdmitrii]

DO NOT FORGET TO SQUASH COMMITTS ON MERGE

adex command line utility was introduced that supplies

Commands: init, start, stop, status

[rozhkovdmitrii]

@shamardy @ozkanonur @borngraced
Guys, I'm happy to introduce the version 0.1.0

[onur-ozkan]

Great implementation :)

I have a few suggestions/notes from my first review iteration:

[shamardy]

Great work!
I have a few comments/questions for my first review :)

[borngraced]

Great work! I have couple questions and some questions

[onur-ozkan]

The main idea is avoid touching mm2 dep tree, breaking changes, etc while creating this new tool

Yes, but it touches it:

• https://github.com/KomodoPlatform/atomicDEX-API/pull/1729/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR5281
• https://github.com/KomodoPlatform/atomicDEX-API/pull/1729/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR5462
• https://github.com/KomodoPlatform/atomicDEX-API/pull/1729/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR7861

Three package versions have been changed without any objective reason. That is what I said: "It would be better to revert these changes before merging."

Oh I totally misunderstood, sorry

[rozhkovdmitrii]

@DeckerSU, let me comment on it, please.

adex_cli is excluded from the manifest by using exclude and has its own dependency tree. It's okay, but why does it touch package versions in the main Cargo.toml? What is the purpose of changing the quote, proc-macro2, and termcolor versions? From my point of view, it would be better to revert these changes before merging.

As we know, if any module is in the exclude section, it means that it will not be built with all the goals listed in the workspace. Therefore, adex-cli can be built separately using the --manifest-path option. I guess you want to avoid using new versions as much as possible, even if they were in an excluded part of the project or in an entirely different repository to not wasting time for nothing.
By the way, I fixed the version of gstuff to "=0.7.4" because if it was used in a standalone build, it would be included with version 0.7.15 and would result in using deprecated parts from the new version.
Another thing I would like to share with you is that I plan to move it into another repository if our team lets me do it.
Please give me some time to analyze and fix all the versions that were changed in the resulting build tree.
Thank you for your review

Hm, I see the difference in original Cargo.lock, of coarse it should be solved

cc: @ozkanonur

[DeckerSU]

I guess else branch of let-else statement here will never work ... bcz mm2_binary.file_name() will always return Some("mm2") or Some("mm2.exe") ... depends on platform, whether the file actually exists or not.

[rozhkovdmitrii]

I know it, that is just paranoic mode. If we are prone to not use any forms of unwrap or extend we will never meet any panic

[onur-ozkan]

Please consider using expect or panic under unwrap_or_else

[rozhkovdmitrii]

done

[DeckerSU]

Hm, I see the difference in original Cargo.lock, of coarse it should be solved

Yes, I mentioned exactly these differences, nothing else. Sorry, maybe I wasn't so clear.

[rozhkovdmitrii]

@DeckerSU
rolled back

[DeckerSU]

Seems you are missed / did not pay attention to few recommendations in #1729 (review) , related to the "Matching with Fork::Parent should not be considered as a successful launch".

Let's imagine the following case:

1. mm2 binary exists, but we accidentally did chmod -x mm2 on it.
2. After ./adex-cli start user will see the following message:

Successfully started: "mm2", forked pid: 579956
thread 'main' panicked at 'Failed to execute process: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/scenarios/mm2_proc_mng.rs:117:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Errors and reports about a successful start at the same time mislead.

In all other aspects it looks good to me.

[onur-ozkan]

You just have avoid writing Successfully started: "mm2", forked pid: $id when mm2 is not an executable. @rozhkovdmitrii

[rozhkovdmitrii]

You just have avoid writing Successfully started: "mm2", forked pid: $id when mm2 is not an executable. @rozhkovdmitrii

Sure, I know, thank you )
The matter is right after fork the program releases terminal even if that is the main process and I want to save the output straight

[rozhkovdmitrii]

Seems you are missed / did not pay attention to few recommendations in #1729 (review) , related to the "Matching with Fork::Parent should not be considered as a successful launch".

Let's imagine the following case:

1. `mm2` binary exists, but we accidentally did `chmod -x mm2` on it.

2. After `./adex-cli start` user will see the following message:

...

@DeckerSU

done

cc: @ozkanonur

P.S: if I used something like this the output would make the output messy

    if let Ok(Fork::Child) = daemon(true, true) {
        command.output().expect("Failed to start: {program:?}");
        exit(1);
    };
    info!("Successfully started: {program:?}");

[rozhkovdmitrii]

Found more deliberate decision using fork and setsid together instead of daemon. Works well, no ugly things in source code
to: @DeckerSU
cc: @ozkanonur

Diff from last review

[DeckerSU]

The look of it has improved, but there is still a potential for a report of a “successful start” even if the outcome was a failure:

1. Let's imagine user already have started instance of mm2 from some other folder.
2. In folder with adex-cli he have mm2 file without rights on execution (chmod -x mm2).
3. After ./adex-cli start user will see the message Successfully started: "mm2", but in real - nothing is started.

Also, can you explain why you chose to use fork() to launch the mm2, rather than simple spawn new process and "detach"? What was your thought process behind this decision? Just curious to know.

use std::process::Command;

let child_process = Command::new("my_child_process_binary")
    .args(&["arg1", "arg2"])
    .spawn()
    .expect("Failed to start child process");

// detach the child process
let pid = child_process.id();
std::mem::forget(child_process);

println!("Started child process with PID {}", pid);

[rozhkovdmitrii]

...
std::mem::forget(child_process);

println!("Started child process with PID {}", pid);

@DeckerSU, to tell you the truth, using std::mem::forget was not obvious. Thank you for great decision ), this one is the best of coarse!

[shamardy]

Thanks for the fixes @rozhkovdmitrii! Changes since my last review looks good.

[rozhkovdmitrii]

If we want CLI to work with remote nodes, then yes it should work over HTTP

Should be HTTPS, else the rpc password is sent in cleartext over internet. There is an issue about this #1324 and i think it's an easy task.

#1873