fix: address PR review issues for rpc endpoint profiles

okwn · okwn · commit bc0b0bda2907 · 2026-04-02T23:02:26.000+03:00
diff --git a/ows/crates/ows-cli/src/commands/rpc.rs b/ows/crates/ows-cli/src/commands/rpc.rs
@@ -15,22 +15,27 @@ fn nearest_profile<'a>(input: &str, profiles: &'a [&str]) -> Option<&'a str> {
 /// Compute Levenshtein distance between two strings (no external deps).
 #[allow(clippy::needless_range_loop)]
 fn edit_distance(a: &str, b: &str) -> usize {
-    let mut matrix: Vec<Vec<usize>> = vec![vec![0; b.len() + 1]; a.len() + 1];
-    for i in 0..=a.len() {
+    let a_chars: Vec<char> = a.chars().collect();
+    let b_chars: Vec<char> = b.chars().collect();
+    let a_len = a_chars.len();
+    let b_len = b_chars.len();
+
+    let mut matrix: Vec<Vec<usize>> = vec![vec![0; b_len + 1]; a_len + 1];
+    for i in 0..=a_len {
         matrix[i][0] = i;
     }
-    for j in 0..=b.len() {
+    for j in 0..=b_len {
         matrix[0][j] = j;
     }
-    for (i, ca) in a.char_indices() {
-        for (j, cb) in b.char_indices() {
+    for (i, ca) in a_chars.iter().enumerate() {
+        for (j, cb) in b_chars.iter().enumerate() {
             let cost = if ca == cb { 0 } else { 1 };
             matrix[i + 1][j + 1] = (matrix[i][j + 1] + 1)
                 .min(matrix[i + 1][j] + 1)
                 .min(matrix[i][j] + cost);
         }
     }
-    matrix[a.len()][b.len()]
+    matrix[a_len][b_len]
 }
 
 /// Show the active RPC profile and its endpoints, or a specific profile.
@@ -136,17 +141,21 @@ pub fn add(name: &str, chains: &[String], urls: &[String]) -> Result<(), CliErro
         ));
     }
 
-    // Validate all chains first before making any changes
-    for chain in chains {
-        let _ = ows_core::parse_chain(chain)
-            .map_err(|e| CliError::InvalidArgs(format!("invalid chain '{}': {}", chain, e)))?;
-    }
+    // Validate and normalize all chains first before making any changes
+    let normalized_chains: Vec<String> = chains
+        .iter()
+        .map(|chain| {
+            ows_core::parse_chain(chain)
+                .map(|c| c.chain_id.to_string())
+                .map_err(|e| CliError::InvalidArgs(format!("invalid chain '{}': {}", chain, e)))
+        })
+        .collect::<Result<Vec<_>, _>>()?;
 
     let mut config = Config::load_or_default();
 
     let is_new = config.profile(name).is_none();
 
-    for (chain, url) in chains.iter().zip(urls.iter()) {
+    for (chain, url) in normalized_chains.iter().zip(urls.iter()) {
         config.upsert_profile_endpoint(name, chain, url.clone());
     }
 
@@ -155,7 +164,7 @@ pub fn add(name: &str, chains: &[String], urls: &[String]) -> Result<(), CliErro
     if is_new {
         println!("Created new profile: {}", name);
     }
-    for (chain, url) in chains.iter().zip(urls.iter()) {
+    for (chain, url) in normalized_chains.iter().zip(urls.iter()) {
         println!("Added {} -> {} in profile '{}'", chain, url, name);
     }
 
@@ -274,6 +283,20 @@ mod tests {
         assert_eq!(edit_distance("team-dev", "team-prod"), 4);
     }
 
+    #[test]
+    fn test_edit_distance_multibyte_utf8() {
+        // Regression: edit_distance must count characters, not bytes.
+        // 'é' is 2 bytes in UTF-8; a byte-oriented implementation would produce
+        // wrong matrix dimensions and incorrect indices for multi-byte chars.
+        assert_eq!(edit_distance("café", "cafe"), 1); // 'é'(2 bytes) vs 'e'(1 byte): cost 1
+        assert_eq!(edit_distance("cafe", "café"), 1);
+        assert_eq!(edit_distance("résumé", "resume"), 2); // 'é','é'(2 bytes each) -> 'e','e': cost 2
+        assert_eq!(edit_distance("日本", "日本"), 0); // identical 6-byte strings
+        assert_eq!(edit_distance("日本", "日韩"), 1); // different 3-byte chars: substitution cost 1
+        assert_eq!(edit_distance("hello世界", "hello世界"), 0); // identical mixed ASCII/multibyte
+        assert_eq!(edit_distance("hello世界", "hello世界!"), 1); // insertion at end
+    }
+
     #[test]
     fn test_nearest_profile() {
         let profiles = vec!["team-dev", "mainnet-evm", "staging"];
diff --git a/ows/crates/ows-core/src/config.rs b/ows/crates/ows-core/src/config.rs
@@ -65,7 +65,9 @@ pub struct Config {
     #[serde(default = "default_vault_path")]
     pub vault_path: PathBuf,
     /// Legacy flat RPC endpoints (chain_id -> url). Used for backward compatibility.
-    #[serde(default)]
+    /// Empty by default when loading from disk — built-in defaults come from
+    /// `Config::default_rpc()` at runtime and are not persisted.
+    #[serde(default, skip_serializing_if = "HashMap::is_empty")]
     pub rpc: HashMap<String, String>,
     /// New structured RPC config with profile support.
     /// When present, takes precedence over legacy `rpc` field for profile lookups.
@@ -144,9 +146,13 @@ impl Default for Config {
 }
 
 impl Config {
-    /// Look up an RPC URL by chain identifier from legacy flat rpc map.
-    pub fn rpc_url(&self, chain: &str) -> Option<&str> {
-        self.rpc.get(chain).map(|s| s.as_str())
+    /// Look up an RPC URL by chain identifier.
+    /// Checks: user-defined global rpc > built-in defaults.
+    pub fn rpc_url(&self, chain: &str) -> Option<String> {
+        self.rpc
+            .get(chain)
+            .cloned()
+            .or_else(|| Config::default_rpc().get(chain).cloned())
     }
 
     /// Returns the name of the active profile, if any.
@@ -209,6 +215,7 @@ impl Config {
     }
 
     /// Remove a chain endpoint from a profile. Deletes profile if empty.
+    /// Clears active_profile if the deleted profile was the active one.
     pub fn remove_profile_endpoint(&mut self, profile_name: &str, chain: &str) -> bool {
         let config = match self.rpc_config.as_mut() {
             Some(c) => c,
@@ -221,6 +228,9 @@ impl Config {
         let removed = profile.chains.remove(chain).is_some();
         if removed && profile.chains.is_empty() {
             config.profiles.remove(profile_name);
+            if config.active_profile.as_deref() == Some(profile_name) {
+                config.active_profile = None;
+            }
         }
         removed
     }
@@ -265,7 +275,12 @@ impl Config {
 
     /// Load config from a specific path, merging user overrides on top of defaults.
     pub fn load_or_default_from(path: &std::path::Path) -> Self {
-        let mut config = Config::default();
+        let mut config = Config {
+            // Start with empty rpc — built-in defaults come from `default_rpc()` at
+            // runtime; this avoids baking them into the saved config file.
+            rpc: HashMap::new(),
+            ..Config::default()
+        };
         if path.exists() {
             if let Ok(contents) = std::fs::read_to_string(path) {
                 if let Ok(user_config) = serde_json::from_str::<Config>(&contents) {
@@ -326,20 +341,26 @@ mod tests {
     #[test]
     fn test_rpc_lookup_hit() {
         let config = Config::default();
-        assert_eq!(config.rpc_url("eip155:1"), Some("https://eth.llamarpc.com"));
+        assert_eq!(
+            config.rpc_url("eip155:1"),
+            Some("https://eth.llamarpc.com".to_string())
+        );
     }
 
     #[test]
     fn test_default_rpc_endpoints() {
         let config = Config::default();
-        assert_eq!(config.rpc_url("eip155:1"), Some("https://eth.llamarpc.com"));
+        assert_eq!(
+            config.rpc_url("eip155:1"),
+            Some("https://eth.llamarpc.com".to_string())
+        );
         assert_eq!(
             config.rpc_url("eip155:137"),
-            Some("https://polygon-rpc.com")
+            Some("https://polygon-rpc.com".to_string())
         );
         assert_eq!(
             config.rpc_url("solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp"),
-            Some("https://api.mainnet-beta.solana.com")
+            Some("https://api.mainnet-beta.solana.com".to_string())
         );
     }
 
@@ -383,9 +404,13 @@ mod tests {
     #[test]
     fn test_load_or_default_nonexistent() {
         let config = Config::load_or_default_from(std::path::Path::new("/nonexistent/config.json"));
-        // Should have all default RPCs
-        assert_eq!(config.rpc.len(), 18);
-        assert_eq!(config.rpc_url("eip155:1"), Some("https://eth.llamarpc.com"));
+        // rpc is empty (built-in defaults come from default_rpc() at runtime)
+        assert!(config.rpc.is_empty());
+        // rpc_url still resolves defaults via fallback
+        assert_eq!(
+            config.rpc_url("eip155:1"),
+            Some("https://eth.llamarpc.com".to_string())
+        );
     }
 
     #[test]
@@ -401,10 +426,13 @@ mod tests {
         std::fs::write(&config_path, serde_json::to_string(&user_config).unwrap()).unwrap();
 
         let config = Config::load_or_default_from(&config_path);
-        assert_eq!(config.rpc_url("eip155:1"), Some("https://custom-eth.rpc"));
+        assert_eq!(
+            config.rpc_url("eip155:1"),
+            Some("https://custom-eth.rpc".to_string())
+        );
         assert_eq!(
             config.rpc_url("eip155:137"),
-            Some("https://polygon-rpc.com")
+            Some("https://polygon-rpc.com".to_string())
         );
         assert_eq!(config.vault_path, PathBuf::from("/tmp/custom-vault"));
     }
@@ -536,7 +564,7 @@ mod tests {
         // Defaults still present via legacy rpc
         assert_eq!(
             config.rpc_url("eip155:137"),
-            Some("https://polygon-rpc.com")
+            Some("https://polygon-rpc.com".to_string())
         );
     }
 
@@ -555,7 +583,10 @@ mod tests {
 
         let config = Config::load_or_default_from(&config_path);
         // Old flat rpc is preserved
-        assert_eq!(config.rpc_url("eip155:1"), Some("https://old-custom.eth"));
+        assert_eq!(
+            config.rpc_url("eip155:1"),
+            Some("https://old-custom.eth".to_string())
+        );
         // rpc_config is None since old format doesn't have it
         assert!(config.rpc_config.is_none());
     }
@@ -589,7 +620,10 @@ mod tests {
             Some("https://new-profile.eth".to_string())
         );
         // But legacy rpc is still accessible directly
-        assert_eq!(config.rpc_url("eip155:1"), Some("https://legacy.eth"));
+        assert_eq!(
+            config.rpc_url("eip155:1"),
+            Some("https://legacy.eth".to_string())
+        );
     }
 
     #[test]
@@ -632,6 +666,20 @@ mod tests {
         assert_eq!(config.active_profile(), None);
     }
 
+    #[test]
+    fn test_remove_last_chain_clears_active() {
+        // Regression: removing the last chain from the active profile via
+        // remove_profile_endpoint should clear active_profile.
+        let mut config = Config::default();
+        config.upsert_profile_endpoint("dev", "eip155:1", "https://dev-eth.example.com".into());
+        config.set_active_profile(Some("dev".into()));
+
+        assert_eq!(config.active_profile(), Some("dev"));
+        assert!(config.remove_profile_endpoint("dev", "eip155:1"));
+        assert!(config.profile("dev").is_none());
+        assert_eq!(config.active_profile(), None);
+    }
+
     #[test]
     fn test_delete_profile_nonexistent() {
         let mut config = Config::default();
@@ -700,4 +748,48 @@ mod tests {
         let config = Config::default();
         assert!(config.profile_names().collect::<Vec<_>>().is_empty());
     }
+
+    #[test]
+    fn test_save_config_does_not_bake_in_defaults() {
+        // Regression: saving a config with only profile-based RPC should NOT
+        // write the built-in defaults into the rpc field.
+        let dir = tempfile::tempdir().unwrap();
+        let config_path = dir.path().join("config.json");
+
+        // Build a config that only has a profile (no global rpc overrides)
+        let mut config = Config::load_or_default_from(&config_path);
+        config.upsert_profile_endpoint("dev", "eip155:1", "https://dev-eth.example.com".into());
+        config.set_active_profile(Some("dev".into()));
+
+        // Serialize and write to disk (as CLI save does)
+        let json = serde_json::to_string(&config).unwrap();
+        std::fs::write(&config_path, &json).unwrap();
+
+        // Read back as Value and verify rpc field is absent/empty
+        let parsed: serde_json::Value = serde_json::from_str(&json).unwrap();
+
+        // rpc field must be absent or empty — no baked-in defaults
+        let rpc = parsed.get("rpc");
+        assert!(
+            rpc.is_none() || rpc == Some(&serde_json::Value::Object(Default::default())),
+            "rpc field should be absent or empty, got: {:?}",
+            rpc
+        );
+
+        // profiles should be present
+        assert!(
+            parsed
+                .get("rpc_config")
+                .and_then(|c| c.get("profiles"))
+                .is_some(),
+            "profiles should be saved"
+        );
+
+        // Reload from disk and verify profile still works
+        let reloaded = Config::load_or_default_from(&config_path);
+        assert_eq!(
+            reloaded.profile_rpc_url("eip155:1"),
+            Some("https://dev-eth.example.com")
+        );
+    }
 }
