fix: better URL handling in client

Author: iamquang95

crates/charon/src/obolapi/client.rs

@@ -14,14 +14,11 @@ use crate::obolapi::error::{Error, Result};
 /// Default HTTP request timeout if not specified.
 const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10);
 
-/// Launchpad URL path format string for a given cluster lock hash.
-const LAUNCHPAD_RETURN_PATH_FMT: &str = "/lock/0x{}/launchpad";
-
 /// REST client for Obol API requests.
 #[derive(Debug, Clone)]
 pub struct Client {
     /// Base Obol API URL.
-    base_url: String,
+    base_url: Url,
 
     /// HTTP request timeout.
     _req_timeout: Duration,
@@ -53,29 +50,30 @@ impl ClientOptions {
 impl Client {
     /// Creates a new Obol API client.
     pub fn new(url_str: &str, options: ClientOptions) -> Result<Self> {
-        Url::parse(url_str)?;
-
         let req_timeout = options.timeout.unwrap_or(DEFAULT_TIMEOUT);
 
         let http_client = reqwest::Client::builder().timeout(req_timeout).build()?;
 
+        // Ensure base_url ends with a trailing slash for proper URL joining
+        let normalized_url = if url_str.ends_with('/') {
+            url_str.to_string()
+        } else {
+            format!("{}/", url_str)
+        };
+        let base_url = Url::parse(&normalized_url)?;
+
         Ok(Self {
-            base_url: url_str.to_string(),
+            base_url,
             _req_timeout: req_timeout,
             http_client,
         })
     }
 
-    /// Returns the base URL from the baseURL stored in client.
-    pub(crate) fn url(&self) -> Url {
-        Url::parse(&self.base_url).expect("parse Obol API URL, this should never happen")
-    }
-
     /// Returns the Launchpad cluster dashboard page for a
     /// given lock, on the given Obol API client.
-    pub fn launchpad_url_for_lock(&self, lock: &Lock) -> String {
-        let url = self.build_url(&launchpad_url_path(lock));
-        url.to_string()
+    pub fn launchpad_url_for_lock(&self, lock: &Lock) -> Result<String> {
+        let url = self.build_url(&launchpad_url_path(lock))?;
+        Ok(url.to_string())
     }
 
     /// Returns a reference to the HTTP client for making requests.
@@ -84,18 +82,10 @@ impl Client {
     }
 
     /// Builds a URL by safely appending a path to the base URL.
-    pub(crate) fn build_url(&self, path: &str) -> Url {
-        let mut base = self.url();
-        let current = base.path().trim_end_matches('/');
-        let new_path = path.trim_start_matches('/');
-
-        if current.is_empty() {
-            base.set_path(&format!("/{}", new_path));
-        } else {
-            base.set_path(&format!("{}/{}", current, new_path));
-        }
-
-        base
+    /// Strip leading '/' from path for proper URL joining
+    pub(crate) fn build_url(&self, path: &str) -> Result<Url> {
+        let path = path.trim_start_matches('/');
+        Ok(self.base_url.join(path)?)
     }
 
     /// Makes an HTTP POST request.
@@ -209,7 +199,7 @@ impl Client {
 
 fn launchpad_url_path(lock: &Lock) -> String {
     let hash_hex = hex::encode(&lock.lock_hash).to_uppercase();
-    LAUNCHPAD_RETURN_PATH_FMT.replace("{}", &hash_hex)
+    format!("/lock/0x{}/launchpad", &hash_hex)
 }
 
 #[cfg(test)]
@@ -247,73 +237,89 @@ mod tests {
 
     #[test]
     fn test_new_client_valid_url() {
-        let client = Client::new("https://api.obol.tech", ClientOptions::default());
-        assert!(client.is_ok());
+        assert!(Client::new("https://api.obol.tech", ClientOptions::default()).is_ok());
     }
 
     #[test]
     fn test_new_client_invalid_url() {
-        let client = Client::new("not-a-url", ClientOptions::default());
-        assert!(client.is_err());
+        assert!(Client::new("not-a-url", ClientOptions::default()).is_err());
     }
 
     #[test]
-    fn test_launchpad_url_path() {
-        let lock = test_lock_with_hash(vec![0x12, 0x34, 0xab, 0xcd]);
-        let path = launchpad_url_path(&lock);
-        assert_eq!(path, "/lock/0x1234ABCD/launchpad");
-    }
+    fn test_base_url_normalization() {
+        let c1 = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
+        assert_eq!(c1.base_url.as_str(), "https://api.obol.tech/");
 
-    #[test]
-    fn test_launchpad_url_for_lock() {
-        let client = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
-        let lock = test_lock_with_hash(vec![0x12, 0x34, 0xab, 0xcd]);
-        let url = client.launchpad_url_for_lock(&lock);
-        assert_eq!(url, "https://api.obol.tech/lock/0x1234ABCD/launchpad");
-    }
+        let c2 = Client::new("https://api.obol.tech/", ClientOptions::default()).unwrap();
+        assert_eq!(c2.base_url.as_str(), "https://api.obol.tech/");
 
-    #[test]
-    fn test_build_url_with_root_base() {
-        // Base path is "/" (root)
-        let client = Client::new("https://api.obol.tech/", ClientOptions::default()).unwrap();
+        let c3 = Client::new("https://api.obol.tech/v1", ClientOptions::default()).unwrap();
+        assert_eq!(c3.base_url.as_str(), "https://api.obol.tech/v1/");
 
-        let url = client.build_url("/definition");
-        assert_eq!(url.path(), "/definition");
+        let c4 = Client::new("https://api.obol.tech/v1/", ClientOptions::default()).unwrap();
+        assert_eq!(c4.base_url.as_str(), "https://api.obol.tech/v1/");
+    }
 
-        let url = client.build_url("definition");
-        assert_eq!(url.path(), "/definition");
+    #[test]
+    fn test_build_url_root_base() {
+        let client = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
+        assert_eq!(
+            client.build_url("definition").unwrap().as_str(),
+            "https://api.obol.tech/definition"
+        );
+        assert_eq!(
+            client.build_url("/definition").unwrap().as_str(),
+            "https://api.obol.tech/definition"
+        );
+        assert_eq!(
+            client
+                .build_url("exp/partial_exits/0xabc")
+                .unwrap()
+                .as_str(),
+            "https://api.obol.tech/exp/partial_exits/0xabc"
+        );
     }
 
     #[test]
-    fn test_build_url_with_non_root_base() {
-        // Base path is "/v1"
+    fn test_build_url_versioned_base() {
         let client = Client::new("https://api.obol.tech/v1", ClientOptions::default()).unwrap();
-
-        let url = client.build_url("/definition");
-        assert_eq!(url.path(), "/v1/definition");
-
-        let url = client.build_url("definition");
-        assert_eq!(url.path(), "/v1/definition");
+        assert_eq!(
+            client.build_url("definition").unwrap().as_str(),
+            "https://api.obol.tech/v1/definition"
+        );
+        assert_eq!(
+            client.build_url("/lock").unwrap().as_str(),
+            "https://api.obol.tech/v1/lock"
+        );
+        assert_eq!(
+            client
+                .build_url("exp/exit/0xlock/5/0xkey")
+                .unwrap()
+                .as_str(),
+            "https://api.obol.tech/v1/exp/exit/0xlock/5/0xkey"
+        );
     }
 
     #[test]
-    fn test_build_url_with_trailing_slash() {
-        // Base path has trailing slash
-        let client = Client::new("https://api.obol.tech/api/", ClientOptions::default()).unwrap();
-
-        let url = client.build_url("/definition");
-        assert_eq!(url.path(), "/api/definition");
-
-        let url = client.build_url("definition");
-        assert_eq!(url.path(), "/api/definition");
+    fn test_launchpad_url_path() {
+        let lock = test_lock_with_hash(vec![0x12, 0x34, 0xab, 0xcd]);
+        assert_eq!(launchpad_url_path(&lock), "/lock/0x1234ABCD/launchpad");
     }
 
     #[test]
-    fn test_build_url_empty_base() {
-        // Edge case: empty path (should be treated as root)
-        let client = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
+    fn test_launchpad_url_for_lock() {
+        let lock = test_lock_with_hash(vec![0x12, 0x34, 0xab, 0xcd]);
 
-        let url = client.build_url("/definition");
-        assert_eq!(url.path(), "/definition");
+        let c1 = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
+        assert_eq!(
+            c1.launchpad_url_for_lock(&lock).unwrap(),
+            "https://api.obol.tech/lock/0x1234ABCD/launchpad"
+        );
+
+        let c2 = Client::new("https://api.obol.tech/v1", ClientOptions::default()).unwrap();
+        assert_eq!(
+            c2.launchpad_url_for_lock(&lock).unwrap(),
+            "https://api.obol.tech/v1/lock/0x1234ABCD/launchpad"
+        );
     }
 }

crates/charon/src/obolapi/exit.rs

@@ -243,15 +243,6 @@ const SSZ_MAX_EXITS: usize = 65536;
 const SSZ_LEN_PUB_KEY: usize = 48;
 const SSZ_LEN_BLS_SIG: usize = 96;
 
-const LOCK_HASH_PATH: &str = "{lock_hash}";
-const VAL_PUBKEY_PATH: &str = "{validator_pubkey}";
-const SHARE_INDEX_PATH: &str = "{share_index}";
-
-const SUBMIT_PARTIAL_EXIT_TMPL: &str = "/exp/partial_exits/{lock_hash}";
-const DELETE_PARTIAL_EXIT_TMPL: &str =
-    "/exp/partial_exits/{lock_hash}/{share_index}/{validator_pubkey}";
-const FETCH_FULL_EXIT_TMPL: &str = "/exp/exit/{lock_hash}/{share_index}/{validator_pubkey}";
-
 impl Client {
     /// Posts the set of msg's to the Obol API, for a given lock hash.
     /// It respects the timeout specified in the Client instance.
@@ -265,7 +256,7 @@ impl Client {
         let lock_hash_str = to_0x(lock_hash);
         let path = submit_partial_exit_url(&lock_hash_str);
 
-        let url = self.build_url(&path);
+        let url = self.build_url(&path)?;
 
         // Sort by validator index ascending
         exit_blobs.sort_by_key(|blob| {
@@ -311,7 +302,7 @@ impl Client {
 
         let path = fetch_full_exit_url(val_pubkey, &to_0x(lock_hash), share_index);
 
-        let url = self.build_url(&path);
+        let url = self.build_url(&path)?;
 
         // Create authentication blob
         let exit_auth_data = FullExitAuthBlob {
@@ -391,7 +382,7 @@ impl Client {
 
         let path = delete_partial_exit_url(val_pubkey, &to_0x(lock_hash), share_index);
 
-        let url = self.build_url(&path);
+        let url = self.build_url(&path)?;
 
         let exit_auth_data = FullExitAuthBlob {
             lock_hash: lock_hash.to_vec(),
@@ -416,23 +407,20 @@ impl Client {
 
 /// Returns the partial exit Obol API URL for a given lock hash.
 fn submit_partial_exit_url(lock_hash: &str) -> String {
-    SUBMIT_PARTIAL_EXIT_TMPL.replace(LOCK_HASH_PATH, lock_hash)
+    format!("/exp/partial_exits/{}", lock_hash)
 }
 
 /// Returns the delete partial exit Obol API URL.
 fn delete_partial_exit_url(val_pubkey: &str, lock_hash: &str, share_index: u64) -> String {
-    DELETE_PARTIAL_EXIT_TMPL
-        .replace(VAL_PUBKEY_PATH, val_pubkey)
-        .replace(LOCK_HASH_PATH, lock_hash)
-        .replace(SHARE_INDEX_PATH, &share_index.to_string())
+    format!(
+        "/exp/partial_exits/{}/{}/{}",
+        lock_hash, share_index, val_pubkey
+    )
 }
 
 /// Returns the full exit Obol API URL.
 fn fetch_full_exit_url(val_pubkey: &str, lock_hash: &str, share_index: u64) -> String {
-    FETCH_FULL_EXIT_TMPL
-        .replace(VAL_PUBKEY_PATH, val_pubkey)
-        .replace(LOCK_HASH_PATH, lock_hash)
-        .replace(SHARE_INDEX_PATH, &share_index.to_string())
+    format!("/exp/exit/{}/{}/{}", lock_hash, share_index, val_pubkey)
 }
 
 fn put_bytes_n(hh: &mut Hasher, bytes: &[u8], expected_len: usize) -> Result<()> {
@@ -451,7 +439,6 @@ fn put_bytes_n(hh: &mut Hasher, bytes: &[u8], expected_len: usize) -> Result<()>
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::obolapi::ClientOptions;
 
     #[test]
     fn test_submit_partial_exit_url() {
@@ -471,72 +458,6 @@ mod tests {
         assert_eq!(url, "/exp/exit/0xlockhash/5/0xpubkey");
     }
 
-    #[test]
-    fn test_build_submit_partial_exit_url_root_base() {
-        let client = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
-        let path = submit_partial_exit_url("0xabcd1234");
-        let url = client.build_url(&path);
-        assert_eq!(
-            url.as_str(),
-            "https://api.obol.tech/exp/partial_exits/0xabcd1234"
-        );
-    }
-
-    #[test]
-    fn test_build_submit_partial_exit_url_v1_base() {
-        let client = Client::new("https://api.obol.tech/v1", ClientOptions::default()).unwrap();
-        let path = submit_partial_exit_url("0xabcd1234");
-        let url = client.build_url(&path);
-        assert_eq!(
-            url.as_str(),
-            "https://api.obol.tech/v1/exp/partial_exits/0xabcd1234"
-        );
-    }
-
-    #[test]
-    fn test_build_delete_partial_exit_url_root_base() {
-        let client = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
-        let path = delete_partial_exit_url("0xpubkey", "0xlockhash", 5);
-        let url = client.build_url(&path);
-        assert_eq!(
-            url.as_str(),
-            "https://api.obol.tech/exp/partial_exits/0xlockhash/5/0xpubkey"
-        );
-    }
-
-    #[test]
-    fn test_build_delete_partial_exit_url_v1_base() {
-        let client = Client::new("https://api.obol.tech/v1", ClientOptions::default()).unwrap();
-        let path = delete_partial_exit_url("0xpubkey", "0xlockhash", 5);
-        let url = client.build_url(&path);
-        assert_eq!(
-            url.as_str(),
-            "https://api.obol.tech/v1/exp/partial_exits/0xlockhash/5/0xpubkey"
-        );
-    }
-
-    #[test]
-    fn test_build_fetch_full_exit_url_root_base() {
-        let client = Client::new("https://api.obol.tech", ClientOptions::default()).unwrap();
-        let path = fetch_full_exit_url("0xpubkey", "0xlockhash", 5);
-        let url = client.build_url(&path);
-        assert_eq!(
-            url.as_str(),
-            "https://api.obol.tech/exp/exit/0xlockhash/5/0xpubkey"
-        );
-    }
-
-    #[test]
-    fn test_build_fetch_full_exit_url_v1_base() {
-        let client = Client::new("https://api.obol.tech/v1", ClientOptions::default()).unwrap();
-        let path = fetch_full_exit_url("0xpubkey", "0xlockhash", 5);
-        let url = client.build_url(&path);
-        assert_eq!(
-            url.as_str(),
-            "https://api.obol.tech/v1/exp/exit/0xlockhash/5/0xpubkey"
-        );
-    }
-
     /// These test vectors were generated from Go `charon/app/obolapi` using
     /// fastssz
     #[test]