From 40f30ec9519b1ca1dd96d9340ba446cbea8fa687 Mon Sep 17 00:00:00 2001
From: Bjerg <onbjerg@users.noreply.github.com>
Date: Mon, 9 Jan 2023 17:31:53 +0100
Subject: [PATCH] refactor: clean up `SocketAddr` value parser (#777)

- Rename the function
- Add more docs explaining the supported formats
- Remove support for empty string (just use an `Option`),
  and remove support for `:` (should be considered a typo)
- Reduce allocations of strings
---
 bin/reth/src/node/mod.rs |  4 +--
 bin/reth/src/util/mod.rs | 59 +++++++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/bin/reth/src/node/mod.rs b/bin/reth/src/node/mod.rs
index ff4f297ce..f07b891fc 100644
--- a/bin/reth/src/node/mod.rs
+++ b/bin/reth/src/node/mod.rs
@@ -8,7 +8,7 @@ use crate::{
     util::{
         chainspec::{chain_spec_value_parser, ChainSpecification},
         init::{init_db, init_genesis},
-        socketaddr_value_parser,
+        parse_socket_address,
     },
     NetworkOpts,
 };
@@ -66,7 +66,7 @@ pub struct Command {
     /// Enable Prometheus metrics.
     ///
     /// The metrics will be served at the given interface and port.
-    #[arg(long, value_name = "SOCKET", value_parser = socketaddr_value_parser)]
+    #[arg(long, value_name = "SOCKET", value_parser = parse_socket_address)]
     metrics: Option<SocketAddr>,
 
     /// Set the chain tip manually for testing purposes.
diff --git a/bin/reth/src/util/mod.rs b/bin/reth/src/util/mod.rs
index cade43f92..00b945653 100644
--- a/bin/reth/src/util/mod.rs
+++ b/bin/reth/src/util/mod.rs
@@ -38,27 +38,30 @@ pub(crate) fn hash_or_num_value_parser(value: &str) -> Result<BlockHashOrNumber,
     }
 }
 
-/// Parse [SocketAddr]
-pub(crate) fn socketaddr_value_parser(value: &str) -> Result<SocketAddr, eyre::Error> {
-    const DEFAULT_DOMAIN: &str = "localhost";
-    const DEFAULT_PORT: u16 = 9000;
-    let value = if value.is_empty() || value == ":" {
-        format!("{DEFAULT_DOMAIN}:{DEFAULT_PORT}")
-    } else if value.starts_with(':') {
-        format!("{DEFAULT_DOMAIN}{value}")
-    } else if value.ends_with(':') {
-        format!("{value}{DEFAULT_PORT}")
-    } else if value.parse::<u16>().is_ok() {
-        format!("{DEFAULT_DOMAIN}:{value}")
-    } else if value.contains(':') {
-        value.to_string()
-    } else {
-        format!("{value}:{DEFAULT_PORT}")
-    };
-    match value.to_socket_addrs() {
-        Ok(mut iter) => iter.next().ok_or(eyre::Error::msg(format!("\"{value}\""))),
-        Err(e) => Err(eyre::Error::from(e).wrap_err(format!("\"{value}\""))),
+/// Parse a [SocketAddr] from a `str`.
+///
+/// The following formats are checked:
+///
+/// - If the value can be parsed as a `u16` or starts with `:` it is considered a port, and the
+/// hostname is set to `localhost`.
+/// - If the value contains `:` it is assumed to be the format `<host>:<port>`
+/// - Otherwise it is assumed to be a hostname
+///
+/// An error is returned if the value is empty.
+pub(crate) fn parse_socket_address(value: &str) -> Result<SocketAddr, eyre::Error> {
+    if value.is_empty() {
+        eyre::bail!("Cannot parse socket address from an empty string");
     }
+
+    if value.starts_with(':') || value.parse::<u16>().is_ok() {
+        ("localhost", 9000).to_socket_addrs()
+    } else if value.contains(':') {
+        value.to_socket_addrs()
+    } else {
+        (value, 9000).to_socket_addrs()
+    }?
+    .next()
+    .ok_or_else(|| eyre::eyre!("Could not parse socket address from {}", value))
 }
 
 /// Tracing utility
@@ -129,16 +132,16 @@ pub mod reth_tracing {
 
 #[cfg(test)]
 mod tests {
-    use std::net::ToSocketAddrs;
-
-    use super::socketaddr_value_parser;
+    use super::*;
 
     #[test]
-    fn parse_socketaddr_with_default() {
-        let expected = "localhost:9000".to_socket_addrs().unwrap().next().unwrap();
-        let test_values = ["localhost:9000", ":9000", "9000", "localhost:", "localhost", ":", ""];
-        for value in test_values {
-            assert_eq!(socketaddr_value_parser(value).expect("value_parser failed"), expected);
+    fn parse_socket_addresses() {
+        for value in ["localhost:9000", ":9000", "9000", "localhost"] {
+            let socket_addr = parse_socket_address(value)
+                .expect(&format!("could not parse socket address: {}", value));
+
+            assert!(socket_addr.ip().is_loopback());
+            assert_eq!(socket_addr.port(), 9000);
         }
     }
 }