feat: clap parsing durations

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves changes across multiple configuration structures and introduces a new dependency and function for parsing durations. Understanding the impact of these changes on the system's behavior and ensuring compatibility requires a moderate level of effort.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The parse_duration function does not handle cases where the input string is empty or contains only whitespace, which could lead to runtime errors if not validated elsewhere.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/config.rs
suggestion	Consider handling empty or whitespace-only strings in the parse_duration function to prevent runtime errors. This can be achieved by checking if the trimmed input is empty before attempting to parse it. [important]
relevant line	Err(anyhow!("invalid duration format: {}", s))

---

relevant file	src/config.rs
suggestion	Add unit tests for the parse_duration function to ensure it correctly parses various time formats and handles errors as expected. This is crucial for validating the robustness of the parsing logic. [important]
relevant line	fn parse_duration(s: &str) -> anyhow::Result {

---

relevant file	src/config.rs
suggestion	Ensure consistency in environment variable naming. The environment variable for sync_interval is SYNC_INTERVAL, but for external_rpc_storage_timeout and perm_storage_timeout, it's not clear if they follow the same naming convention. Consider standardizing these for clarity and maintainability. [medium]
relevant line	#[arg(long = "perm-storage-timeout", value_parser=parse_duration, env = "PERM_STORAGE_TIMEOUT")]

---

relevant file	src/config.rs
suggestion	Validate the parse_duration function against a wider range of inputs, including negative numbers, extremely large values, and non-numeric inputs, to ensure it handles all edge cases appropriately. [important]
relevant line	// try humantime

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Improve error handling by checking for empty or whitespace-only duration strings Consider handling the case where the duration string might be empty or contain only whitespace, which could lead to a misleading error message. You can trim the string and check if it's empty before attempting to parse it. src/config.rs [770-784] fn parse_duration(s: &str) -> anyhow::Result<Duration> { + let trimmed = s.trim(); + if trimmed.is_empty() { + return Err(anyhow!("duration string is empty")); + } // try millis - let millis: Result<u64, _> = s.parse(); + let millis: Result<u64, _> = trimmed.parse(); if let Ok(millis) = millis { return Ok(Duration::from_millis(millis)); } ... } Suggestion importance[1-10]: 8 Why: This suggestion improves error handling by addressing potential issues with empty or whitespace-only strings, which is crucial for robustness in parsing functions.	8
Usability	Clarify the error message for unrecognized duration formats to guide users on acceptable inputs Consider adding a specific error message for the case when the duration format is not recognized as either milliseconds or a human-readable format. This can help users understand exactly what went wrong. src/config.rs [770-784] fn parse_duration(s: &str) -> anyhow::Result<Duration> { ... // error - Err(anyhow!("invalid duration format: {}", s)) + Err(anyhow!("invalid duration format: {}. Ensure the format is either in milliseconds or a recognized human-readable format.", s)) } Suggestion importance[1-10]: 7 Why: Providing clearer error messages enhances usability by helping users understand what input formats are acceptable, which can prevent user errors and reduce support queries.	7
Maintainability	Add original input to error message for better debugging of duration parsing failures To enhance the robustness of the duration parsing, consider logging the original input string in case of a parsing error. This can help in debugging issues related to incorrect format inputs. src/config.rs [770-784] fn parse_duration(s: &str) -> anyhow::Result<Duration> { ... // error - Err(anyhow!("invalid duration format: {}", s)) + Err(anyhow!("invalid duration format for input '{}': {}", s, s)) } Suggestion importance[1-10]: 6 Why: Including the original input in error messages can significantly aid in debugging, although it's a relatively minor enhancement in the context of the overall application.	6
Best practice	Ensure thread safety in the parse_duration function to avoid issues in multi-threaded environments To prevent potential conflicts or unexpected behavior, ensure that the parse_duration function is thread-safe if used in a multi-threaded context, possibly by avoiding any mutable global state. src/config.rs [770-784] +fn parse_duration(s: &str) -> anyhow::Result<Duration> { + ... +} - Suggestion importance[1-10]: 5 Why: While ensuring thread safety is important, the suggestion lacks specific actionable changes and does not point out any existing thread safety issues in the provided function.	5