Allowed custom datadir for loggers #405

0xEgao · 2025-02-08T05:59:24Z

Please let me know if you'd like me to make any changes, this pr tries to solve the issue #402

mojoX911

Ack

mojoX911 · 2025-02-08T06:26:20Z

src/utill.rs

@@ -241,40 +243,12 @@ pub fn setup_directory_logger(filter: LevelFilter) {
 /// Setup function that will only run once, even if called multiple times.
 /// Takes log level to set the desired logging verbosity
 // TODO: Use the above setup logger functions.


All the todos comments now can be removed.

removed all the todos

mojoX911 · 2025-02-09T06:42:43Z

@0xEgao there are some test failure. Please look into it. Additionally you can add the git pre-commit hook in your local to always have basic checks covered before committing.

https://github.com/citadel-tech/coinswap?tab=readme-ov-file#setting-up-git-hooks

0xEgao · 2025-02-09T09:46:03Z

Hey @mojoX911 thanks for the feedback ,i have ran the neccessary tests as u suggested.You can review it now

codecov · 2025-02-09T13:24:28Z

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.43%. Comparing base (a00d05a) to head (4f3cae3).
Report is 37 commits behind head on master.

Files with missing lines	Patch %	Lines
src/utill.rs	84.61%	2 Missing ⚠️
src/bin/directoryd.rs	0.00%	1 Missing ⚠️
src/bin/makerd.rs	0.00%	1 Missing ⚠️
src/bin/taker.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   70.26%   70.43%   +0.17%     
==========================================
  Files          34       34              
  Lines        4261     4245      -16     
==========================================
- Hits         2994     2990       -4     
+ Misses       1267     1255      -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mojoX911 · 2025-02-09T13:52:41Z

src/bin/directory-cli.rs

@@ -10,6 +10,7 @@ use coinswap::{
    },
    utill::{read_message, send_message, setup_directory_logger},
 };
+//use dirs::data_dir;


Commented lines like these are left in some places. Please remove them.

I have made the changes, please review it.

mojoX911

Looks good, few more fixes needed.

mojoX911 · 2025-02-15T06:18:55Z

src/bin/directoryd.rs

@@ -35,9 +35,9 @@ struct Cli {
 }

 fn main() -> Result<(), DirectoryServerError> {
-    setup_directory_logger(log::LevelFilter::Info);
-
+    setup_directory_logger(log::LevelFilter::Info, None);


Pass the args.data_dir value here. This will make the log dir always fixed to default. Same for all other apps.

mojoX911 · 2025-02-15T06:19:43Z

src/bin/directory-cli.rs

@@ -43,7 +43,7 @@ fn send_rpc_req(mut stream: TcpStream, req: RpcMsgReq) -> Result<(), DirectorySe
 }

 fn main() -> Result<(), DirectoryServerError> {
-    setup_directory_logger(log::LevelFilter::Info);
+    setup_directory_logger(log::LevelFilter::Info, None);


I think we can remove the setup logger functions from the cli apps.

mojoX911 · 2025-02-15T06:20:16Z

src/bin/maker-cli.rs

@@ -76,7 +76,7 @@ enum Commands {
 }

 fn main() -> Result<(), MakerError> {
-    setup_maker_logger(log::LevelFilter::Info);
+    setup_maker_logger(log::LevelFilter::Info, None);


Same. Remove and check if anything fails in tests. It should not.

mojoX911 · 2025-02-15T06:20:31Z

src/bin/makerd.rs

@@ -49,8 +49,7 @@ struct Cli {
 }

 fn main() -> Result<(), MakerError> {
-    setup_maker_logger(log::LevelFilter::Info);
-
+    setup_maker_logger(log::LevelFilter::Info, None);


pass on args.data_dir.

mojoX911 · 2025-02-15T06:21:24Z

src/bin/taker.rs

@@ -114,6 +114,7 @@ fn main() -> Result<(), TakerError> {
            args.command,
            Commands::Recover | Commands::FetchOffers | Commands::Coinswap { .. }
        ),
+        None, //default path handled inside the function.


pass the args value.

mojoX911 · 2025-02-15T06:24:24Z

tests/test_framework/mod.rs

@@ -533,7 +532,7 @@ impl TestFramework {
        Arc<DirectoryServer>,
        JoinHandle<()>,
    ) {
-        setup_logger(log::LevelFilter::Info);
+        setup_logger(log::LevelFilter::Info, None);


This should be called after creating the temp folder in the line below. Then pass the temp folder.

mojoX911 · 2025-02-15T06:30:21Z

src/utill.rs

+        setup_taker_logger(filter, true, data_dir.clone());
+        setup_maker_logger(filter, data_dir.clone());
+        setup_directory_logger(filter, data_dir.clone());


This will merge all the logs into one file as data_dir value is same for all. You need to pass different subfolder for each logger. Just do data_dir.join)("maker") etc for each case.

This is still not perfect as it will merge all the maker logs together. but its test only, doesn't matter much.

Hey @mojoX911 is this supposed to be like this

pub fn setup_logger(filter: LevelFilter, data_dir: Option<PathBuf>) { Once::new().call_once(|| { env::set_var("RUST_LOG", "coinswap=info"); setup_taker_logger( filter, true, Some(data_dir.clone().unwrap_or_else(get_taker_dir).join("taker")), ); setup_maker_logger( filter, Some(data_dir.clone().unwrap_or_else(get_maker_dir).join("maker")), ); setup_directory_logger( filter, Some( data_dir .clone() .unwrap_or_else(get_dns_dir) .join("directory"), ), ); }); }

No. You need to do data_dir.and_then(|d| Some(d.join("taker")))

Hey @mojoX911 i have committed all the changes you requested,you can review them now

mojoX911

ACK. Just one last question.

mojoX911 · 2025-02-17T04:31:46Z

src/utill.rs

+        if let Err(e) = log4rs::init_config(config) {
+            eprintln!("Failed to initialize logger: {}", e);
+        }


any reason why we are suppressing the error?

I was getting my local branch specific error,so i tried to handle a particular error case,thanks for asking this,i have revert it back to unwrap(). You can review it.

mojoX911

Few nits and its good to go.

mojoX911 · 2025-02-17T14:53:04Z

src/utill.rs

+        if let Err(e) = log4rs::init_config(config) {
+            eprintln!("Failed to initialize logger: {}", e);
+        }


don't suppress the error. unwrap.

mojoX911 · 2025-02-17T14:53:14Z

src/utill.rs

+        if let Err(e) = log4rs::init_config(config) {
+            eprintln!("Failed to initialize logger: {}", e);
+        }


Have removed all suppressing error now.

mojoX911 · 2025-02-17T14:53:57Z

src/utill.rs

+        setup_taker_logger(filter, true, data_dir.clone().map(|d| d.join("taker")));
+        setup_maker_logger(filter, data_dir.clone().map(|d| d.join("maker")));
+        setup_directory_logger(filter, data_dir.clone().map(|d| d.join("directory")));


I think clone is not needed.

Thanks for the suggestion,instead of clone() i have used as_ref() for better memory efficiency.

mojoX911 · 2025-02-17T14:54:28Z

src/wallet/direct_send.rs

@@ -79,8 +79,7 @@ impl Wallet {
    /// ### Behavior
    /// - If [SendAmount::Max] is used, the function creates a transaction for the maximum possible
    ///   value to the specified destination.
-    /// - If [SendAmount::Amount] is used, a custom value is sent, and any remaining funds
-    ///    are held in a change address, if applicable.
+    /// - If [SendAmount::Amount] is used, a custom value is sent, and any remaining funds are held in a change address, if applicable.


No contextual change. Remove.

I was recieving an error while doing cargo clippy... in this line so i changed the comment to fit into one line.

mojoX911 · 2025-02-17T14:56:16Z

tests/taker_cli.rs

@@ -52,10 +52,10 @@ impl TakerCli {
            args.push(arg);
        }

-        let output = Command::new("./target/debug/taker")
+        let output = Command::new(env!("CARGO_BIN_EXE_taker"))


This is a good change. Can you add this to all places in the lib where we are using ./target/debug/... stuffs? This was another fix not listed in the issues, but we can get it done here.

Thanks for the complement,i have changed all the ./target/debug/...

mojoX911

Ack. Awaiting CI.

0xEgao · 2025-02-19T10:17:34Z

hey @mojoX911 the test is failing because the logging framework only allows one global logger to be set,and an attempt to create more logger result to this test failure,for that i did earlier was suppressing the error, i.e checking if logger was already initialized,do you suggest to go back to this,or any other way?

mojoX911 · 2025-02-19T10:39:15Z

Ah right. Probably thats the reason why we had the repeated function. Suppressing the error won't work, as they will silently fail the log setup.

lets revert back the original global setup logger, and just pass in custom datadirs into that.

0xEgao · 2025-02-19T10:44:14Z

Reverting back is an option ,but i think the issue is with Once::new().call_once(...) because it creates new instance every time we call the function, Can we use OnceLock so that it only runs once globally and don't create new instance everytime?

mojoX911 · 2025-02-19T10:51:36Z

Can we use OnceLock so that it only runs once globally and don't create new instance everytime?

We need to initiate 3 loggers for 3 systems, can we do it with OnceLock with the new design?

0xEgao · 2025-02-19T11:25:24Z

Hey @mojoX911 now i have implemented the OnceLock ,ran the tests locally everything worked fine for me,you can review that now, happy to contribute :D

mojoX911

Ack

Allowed custom datadir for loggers

106a25f

0xEgao mentioned this pull request Feb 8, 2025

Allow custom datadir for loggers. #402

Closed

mojoX911 requested changes Feb 8, 2025

View reviewed changes

0xEgao added 2 commits February 8, 2025 12:03

fmt && variable name changed

2cf2b6d

todos removed

04c43bb

0xEgao requested a review from mojoX911 February 8, 2025 06:35

0xEgao added 3 commits February 8, 2025 23:58

ownership error resolved

0b1b61f

bin error fixed

64d63c0

setup_loggers function handle the path automatically

1b8e2e0

ran test fixed errors

23dcab9

mojoX911 requested changes Feb 9, 2025

View reviewed changes

removed unused import comment

5b190cb

0xEgao requested a review from mojoX911 February 9, 2025 14:26

mojoX911 requested changes Feb 15, 2025

View reviewed changes

passed the args values and removed the loggers from cli

16ccc5a

0xEgao requested a review from mojoX911 February 15, 2025 08:28

merge logs in different files

908fdb4

mojoX911 reviewed Feb 17, 2025

View reviewed changes

revert back to unwrap

3454808

0xEgao requested a review from mojoX911 February 17, 2025 07:01

mojoX911 requested changes Feb 17, 2025

View reviewed changes

required changes done

a641f80

0xEgao requested a review from mojoX911 February 17, 2025 20:49

mojoX911 approved these changes Feb 19, 2025

View reviewed changes

using OnceLock

4f3cae3

0xEgao force-pushed the datadir branch from 026a1c4 to 4f3cae3 Compare February 19, 2025 11:22

0xEgao requested a review from mojoX911 February 19, 2025 11:27

mojoX911 approved these changes Feb 19, 2025

View reviewed changes

mojoX911 merged commit 3bf3a4d into citadel-tech:master Feb 19, 2025
8 checks passed

0xEgao deleted the datadir branch February 20, 2025 07:23

Allowed custom datadir for loggers #405

Allowed custom datadir for loggers #405

Conversation

0xEgao commented Feb 8, 2025 • edited Loading

mojoX911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojoX911 commented Feb 9, 2025

0xEgao commented Feb 9, 2025

codecov bot commented Feb 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojoX911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xEgao Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojoX911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xEgao Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

mojoX911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xEgao Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xEgao Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

mojoX911 left a comment

Choose a reason for hiding this comment

0xEgao commented Feb 19, 2025

mojoX911 commented Feb 19, 2025

0xEgao commented Feb 19, 2025

mojoX911 commented Feb 19, 2025

0xEgao commented Feb 19, 2025

mojoX911 left a comment

Choose a reason for hiding this comment

0xEgao commented Feb 8, 2025 •

edited

Loading

codecov bot commented Feb 9, 2025 •

edited

Loading

0xEgao Feb 15, 2025 •

edited

Loading

0xEgao Feb 17, 2025 •

edited

Loading

0xEgao Feb 17, 2025 •

edited

Loading

0xEgao Feb 17, 2025 •

edited

Loading