src/config/settings.rs (1)

52-67: Consider safer accessor methods to avoid potential panics

The getter methods use unwrap() which could panic if the global config hasn't been initialized. Consider using Result or Option return types or documenting that these should only be called after initialization.

impl Settings {
    // ...
-    pub fn get_ln() -> Lightning {
-        MOSTRO_CONFIG.get().unwrap().lightning.clone()
+    pub fn get_ln() -> Option<Lightning> {
+        MOSTRO_CONFIG.get().map(|config| config.lightning.clone())
    }

-    pub fn get_mostro() -> Mostro {
-        MOSTRO_CONFIG.get().unwrap().mostro.clone()
+    pub fn get_mostro() -> Option<Mostro> {
+        MOSTRO_CONFIG.get().map(|config| config.mostro.clone())
    }

-    pub fn get_db() -> Database {
-        MOSTRO_CONFIG.get().unwrap().database.clone()
+    pub fn get_db() -> Option<Database> {
+        MOSTRO_CONFIG.get().map(|config| config.database.clone())
    }

-    pub fn get_nostr() -> Nostr {
-        MOSTRO_CONFIG.get().unwrap().nostr.clone()
+    pub fn get_nostr() -> Option<Nostr> {
+        MOSTRO_CONFIG.get().map(|config| config.nostr.clone())
    }
}

Alternatively, you could keep the current implementation but add a documentation comment explaining that these methods should only be called after init_global_settings has been called.

src/config/types.rs (4)

1-3: Fix redundant comments.

The file starts with two nearly identical comments. Consider consolidating them into a single comprehensive documentation comment to better describe the purpose and content of this file.

-// File with the types for the configuration settings
-// Initialize the types for the configuration settings
+/// This file defines the configuration data structures used throughout the application
+/// and implements conversions between them and the main Settings type.

---

6-7: Fix incorrect doc comment syntax.

The current format // / is not valid Rust documentation syntax. Use /// for documentation comments.

-// / Implement the TryFrom trait for each of the structs in Settings
-// / This allows you to convert from Settings to each of the structs directly.
+/// Implement the TryFrom trait for each of the structs in Settings
+/// This allows you to convert from Settings to each of the structs directly.

---

4-5: Consider adding more specific imports.

The code only imports Deserialize from serde, but in the future you might need other traits like Serialize. Consider using more explicit imports or a wildcard import if more serde traits might be needed.

-use serde::Deserialize;
+use serde::{Deserialize, Serialize};

Or alternatively:

-use serde::Deserialize;
+use serde::*;

---

45-58: Consider grouping related configuration fields.

The Mostro struct contains many fields with different purposes. Consider grouping related fields into nested structs for better organization and readability.

Example:

#[derive(Debug, Deserialize, Default, Clone)]
pub struct Fees {
    pub fee: f64,
    pub max_routing_fee: f64,
}

#[derive(Debug, Deserialize, Default, Clone)]
pub struct Limits {
    pub max_order_amount: u32,
    pub min_payment_amount: u32,
}

#[derive(Debug, Deserialize, Default, Clone)]
pub struct Mostro {
    pub fees: Fees,
    pub limits: Limits,
    pub expiration_hours: u32,
    // other fields...
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/config/util.rs (3)

1-5: Prefer Rust-doc comments over free-form // / comments

The file-header comments start with a peculiar // / prefix, which is not picked up by rustdoc.
Switching to idiomatic //! (module-level) or /// (item-level) comments will allow the documentation to be rendered by cargo doc and IDEs.

-// / Utility functions for the config module
-// / This module provides utility functions for the config module.
-// / It includes functions to initialize the default settings directory and create a settings file from the template if it doesn't exist.
-// / It also includes functions to add a trailing slash to a path if it doesn't already have one.
+//! Utility helpers for the `config` module.
+//!
+//!  * `init_default_dir` – creates `~/.mostro` (or a user-supplied path) on first run.
+//!  * `has_trailing_slash` / `add_trailing_slash` – path helpers that behave
+//!    consistently on Unix and Windows.

---

18-26: Avoid hard-coding separators; lean on MAIN_SEPARATOR

b'\\' and b'/' work, but the intent is clearer (and one branch can be removed) if we compare against the platform’s canonical separator supplied by std lib.

-#[cfg(windows)]
-pub fn has_trailing_slash(p: &Path) -> bool {
-    let last = p.as_os_str().encode_wide().last();
-    last == Some(b'\\' as u16) || last == Some(b'/' as u16)
-}
+#[cfg(windows)]
+pub fn has_trailing_slash(p: &Path) -> bool {
+    use std::path::MAIN_SEPARATOR;
+    let last = p.as_os_str().encode_wide().last();
+    last == Some(MAIN_SEPARATOR as u16)
+}

If you still want to treat / as valid on Windows, add a comment explaining why.
This keeps the function short, removes the magic numbers and improves readability.

---

68-72: Use structured logging instead of println!

Elsewhere in the codebase you already employ tracing::info!.
Uniform logging makes it easier to redirect output and to keep stdout clean for TUI/CLI interactions.

-        println!(
+        tracing::info!(
             "Created settings file from template at {} for mostro - Edit it to configure your Mostro instance",
             config_path.display()
-        );
+        );

src/config/settings.rs (1)

64-82: Getter methods clone the entire sub-config each time

Cloning small structs is cheap, but if these grow (e.g., include TLS certs or big strings) it becomes wasteful.
Two options:

1. Return &'static references (&'static Lightning) using MOSTRO_CONFIG.get().unwrap().
2. Expose the entire Settings once and let callers borrow what they need.

Not urgent, but worth considering for larger deployments.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#481
File: src/config/util.rs:48-76
Timestamp: 2025-05-02T14:21:39.391Z
Learning: The `init_default_dir` function in Mostro intentionally exits the process (std::process::exit(0)) after creating a new configuration file so users can edit the configuration before restarting the application.

src/config/util.rs (3)

19-27: Use consistent logging approach

The create_template_file function uses println! for logging, while init_default_dir uses a mix of println! and structured logging (as suggested by the previous review comments). Consider using the same logging approach throughout the codebase for consistency.

-        println!("Settings file not found - creating default settings file");
+        tracing::info!("Settings file not found - creating default settings file");
         std::fs::write(file_name, include_bytes!("../../settings.tpl.toml"))
             .expect("Failed to write template file");
         // Print a message to the user
-        println!(
+        tracing::info!(
             "Created settings file from template at {} for Mostro",
             config_path.display()
         );
-        println!("Please edit the settings file with  your settings and run Mostro again");
+        tracing::info!("Please edit the settings file with your settings and run Mostro again");

---

27-27: Fix typo in message

There's an extra space in "with your settings".

-        println!("Please edit the settings file with  your settings and run Mostro again");
+        println!("Please edit the settings file with your settings and run Mostro again");

---

68-71: Inconsistent use of "mostro" vs "Mostro" in log messages

The message uses both lowercase "mostro" and uppercase "Mostro". Consider standardizing on one capitalization style for consistency.

-            "Created settings file from template at {} for mostro - Edit it to configure your Mostro instance",
+            "Created settings file from template at {} for Mostro - Edit it to configure your Mostro instance",

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/config/util.rs (3)

34-38: Consider using tracing instead of println for consistency

You're using tracing::info! for the success message at the end of the function, but println! for the configuration template creation message. For consistency, consider using the tracing framework throughout.

-        println!(
-            "Created settings file from template at {} for Mostro - Edit it to configure your Mostro instance",
-            config_file_path.display()
-        );
+        tracing::info!(
+            "Created settings file from template at {} for Mostro - Edit it to configure your Mostro instance",
+            config_file_path.display()
+        );

---

45-46: Consider using a more specific error type for TOML parsing

The current implementation wraps the TOML parsing error as an IOError, which isn't technically accurate. Consider using a more specific error type for parsing errors to provide better diagnostics.

-    let mut settings: Settings = toml::from_str(&contents)
-        .map_err(|e| MostroInternalErr(ServiceError::IOError(e.to_string())))?;
+    let mut settings: Settings = toml::from_str(&contents)
+        .map_err(|e| MostroInternalErr(ServiceError::ParseError(e.to_string())))?;

This assumes you have a ParseError variant in your ServiceError enum. If not, you might want to consider adding one.

---

49-49: Consider making the database filename configurable

The database filename "mostro.db" is hardcoded. Consider either:

1. Making it configurable through the settings file
2. Using a constant at the module level for better maintainability

+const DB_FILENAME: &str = "mostro.db";
+
// Later in the code:
-    settings.database.url = format!("sqlite://{}", settings_dir.join("mostro.db").display());
+    settings.database.url = format!("sqlite://{}", settings_dir.join(DB_FILENAME).display());

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#481
File: src/config/util.rs:48-76
Timestamp: 2025-05-02T14:21:39.391Z
Learning: The `init_default_dir` function in Mostro intentionally exits the process (std::process::exit(0)) after creating a new configuration file so users can edit the configuration before restarting the application.