crates/solver-demo/src/operations/init/mod.rs (2)

313-388: Extract shared oracle lookup to reduce duplication.

get_input_oracle_for_chain and get_output_oracle_for_chain are identical except for the "input" / "output" key. Consider a single helper:

Suggested refactor

+fn get_oracle_for_chain(
+	settlement_config: &SettlementConfig,
+	chain_id: u64,
+	direction: &str, // "input" or "output"
+) -> Option<String> {
+	settlement_config.implementations.values().find_map(|impl_config| {
+		let network_ids = impl_config.get("network_ids")?.as_array()?;
+		let has_chain = network_ids.iter().any(|id| id.as_u64() == Some(chain_id));
+		if !has_chain {
+			return None;
+		}
+		let oracles = impl_config.get("oracles")?.as_object()?;
+		let direction_table = oracles.get(direction)?.as_object()?;
+		let arr = direction_table.get(&chain_id.to_string())?.as_array()?;
+		arr.first()?.as_str().map(String::from)
+	})
+}
+
 fn get_input_oracle_for_chain(
 	settlement_config: &SettlementConfig,
 	chain_id: u64,
 ) -> Option<String> {
-	// ... 35 lines of nested ifs ...
+	get_oracle_for_chain(settlement_config, chain_id, "input")
 }
 
 fn get_output_oracle_for_chain(
 	settlement_config: &SettlementConfig,
 	chain_id: u64,
 ) -> Option<String> {
-	// ... 35 lines of nested ifs ...
+	get_oracle_for_chain(settlement_config, chain_id, "output")
 }

This also flattens the deeply nested if let chains (6+ levels) into idiomatic ?-chaining.

---

391-394: Remove unused _config_name parameter.

The parameter is unused (underscore-prefixed). Removing it from both the signature and the call site (line 190) simplifies the API.

crates/solver-service/src/apis/quote/mod.rs (1)

363-371: Consider extracting a shared test helper for solver engine construction.

The pattern of creating a SolverEngine with empty JSON configs for strategy/pricing is duplicated across at least three test files (order.rs, validators/order.rs, quote/mod.rs). A shared test utility would reduce boilerplate and keep the migration consistent.

crates/solver-service/src/apis/quote/validation.rs (1)

723-765: Significant test config boilerplate duplication across files.

This exact JSON config block is repeated in server.rs::build_test_solver_engine, tokens.rs::create_mock_solver_engine, tokens.rs::create_test_config, and here. Consider extracting a shared test_utils::default_test_config() helper to reduce maintenance burden and drift risk.

crates/solver-service/src/config_merge.rs (1)

959-973: Consider consolidating identical JSON clone helpers.

json_identity and json_clone both just clone the value; keeping a single helper (or inlining .clone()) would reduce surface area.

Also applies to: 1070-1073, 1569-1572

crates/solver-settlement/src/utils.rs (1)

90-98: Negative JSON integers silently become large u64 chain IDs.

v.as_i64().map(|i| i as u64) will wrap negative values (e.g., -1 → u64::MAX - 1). While chain IDs should always be positive, a validation guard would catch misconfigured JSON early.

Proposed fix

-					v.as_i64().map(|i| i as u64).ok_or_else(|| {
+					v.as_u64().ok_or_else(|| {
 						SettlementError::ValidationFailed(format!(
 							"Destination chain ID must be integer for route from chain {chain_id}"
 						))
 					})

serde_json::Value::as_u64() exists and directly rejects negative numbers, making this both simpler and safer.

crates/solver-pricing/src/implementations/defillama.rs (1)

67-83: cache_duration_seconds could silently accept negative values.

v.as_i64().unwrap_or(60) as u64 at line 82–83 will wrap negative JSON integers into large u64 values. Consider using as_u64() or adding a non-negative check, since the validator at line 458 only checks that the value is parseable as i64, not that it's non-negative.

crates/solver-account/src/implementations/local.rs (1)

29-66: Good use of deny_unknown_fields and dedicated config struct.

The LocalWalletConfig with from_json + validate follows the same pattern as KmsWalletConfig, providing consistent config parsing across account implementations. The hex validation logic is correct.

One consideration: private_key is held as a plain String in LocalWalletConfig. While the struct is private and short-lived (only used during parsing), be mindful that serde error messages or debug formatting could inadvertently leak the key value. The #[derive(Debug)] on line 30 would include the private key in debug output.

Consider removing Debug derive or redacting the field

-#[derive(Debug, Deserialize)]
+#[derive(Deserialize)]
 #[serde(deny_unknown_fields)]
 struct LocalWalletConfig {
 	private_key: String,
 }

Alternatively, implement Debug manually to redact private_key.

crates/solver-storage/src/implementations/file.rs (1)

140-146: TTL parsing uses as_i64() then casts to u64.

The Schema validation (line 882) enforces min: Some(0) and create_storage calls validate_config before from_config, so negative values are rejected in the normal flow. However, if from_config is called independently without prior validation, negative values would silently wrap. Using as_u64() would be a defensive improvement.

crates/solver-settlement/src/implementations/direct.rs (1)

476-502: Test config construction is clean — consider a helper to reduce boilerplate.

The repeated pattern of building serde_json::Value::Object with nested maps across 7+ tests is verbose. A small helper function (similar to create_test_oracle_config) that builds a valid base JSON config would reduce duplication and improve maintainability.

crates/solver-settlement/src/implementations/hyperlane.rs (1)

1322-1350: parse_address_table silently returns empty map for non-object input.

If table is not a JSON object (e.g., it's a string or array), the function silently returns an empty HashMap instead of erroring. Since the caller already validated via schema that mailboxes and igp_addresses are tables, this is safe in practice, but a non-object value slipping through would be a silent misconfiguration.

crates/solver-demo/src/core/config.rs (2)

62-62: Blocking I/O in async context.

std::fs::read_to_string blocks the async runtime's thread. The non-JSON path uses SolverFullConfig::from_file which internally uses tokio::fs::read_to_string. Use tokio::fs::read_to_string here as well for consistency.

♻️ Proposed fix

-			let content = std::fs::read_to_string(path).map_err(|e| {
+			let content = tokio::fs::read_to_string(path).await.map_err(|e| {

---

76-76: unwrap() on path.to_str() can panic on non-UTF-8 paths.

While unlikely in practice, this is inconsistent with the careful error handling elsewhere in this function.

♻️ Proposed fix

-			SolverFullConfig::from_file(path.to_str().unwrap()).await?
+			SolverFullConfig::from_file(
+				path.to_str()
+					.ok_or_else(|| Error::InvalidConfig("Config path is not valid UTF-8".into()))?
+			).await?

crates/solver-demo/src/operations/init/mod.rs (3)

419-436: Sanitization looks correct but consider a length cap.

The function handles special characters and the empty-result edge case well. For very long solver IDs, the generated filename could exceed filesystem limits (255 chars on most filesystems). Consider truncating to a safe length.

---

457-533: Deep nesting in oracle extraction functions — consider refactoring.

Both get_input_oracle_for_chain and get_output_oracle_for_chain have 8+ levels of nesting with near-identical structure. This is a good candidate for extraction into a shared helper, e.g.:

fn get_oracle_for_chain(
    settlement_config: &SettlementConfig,
    chain_id: u64,
    oracle_type: &str, // "input" or "output"
) -> Option<String> { ... }

This would eliminate the duplication and reduce cognitive complexity.

---

785-826: EnvVarGuard and CwdGuard will require unsafe blocks in Rust 2024 edition.

Since Rust 2024, std::env::set_var and std::env::remove_var are unsafe fn due to unsoundness in multi-threaded programs on non-Windows platforms. These calls currently compile but will fail in 2024 edition without wrapping in unsafe {} blocks. While the test isolation context mitigates the practical risk here, plan to update these to unsafe { std::env::set_var(...) } during 2024 edition migration, or consider using std::process::Command::env() if applicable to your use case.

crates/solver-demo/src/operations/init/mod.rs (1)

467-467: ⚠️ Potential issue | 🟡 Minor

Use as_u64() for chain ID checks (same issue as previously flagged).

chain_id is u64; as_i64() will miss valid large IDs and introduces lossy cast semantics.

Suggested patch

-					.any(|id| id.as_i64().is_some_and(|i| i as u64 == chain_id));
+					.any(|id| id.as_u64().is_some_and(|i| i == chain_id));
...
-					.any(|id| id.as_i64().is_some_and(|i| i as u64 == chain_id));
+					.any(|id| id.as_u64().is_some_and(|i| i == chain_id));

#!/bin/bash
# Expect no matches after applying the fix.
rg -nP '\.as_i64\(\)\.is_some_and\(\|i\| i as u64 == chain_id\)' crates/solver-demo/src/operations/init/mod.rs

Also applies to: 506-506

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/solver-demo/src/operations/init/mod.rs` at line 467, The code checks
JSON numbers with .as_i64().is_some_and(|i| i as u64 == chain_id) which loses
large u64 values; change the check to use .as_u64() instead (e.g. .any(|id|
id.as_u64().is_some_and(|i| i == chain_id)) or .any(|id|
id.as_u64().map_or(false, |i| i == chain_id))) so the closure that uses chain_id
(the .any(|id| ... ) lambda) compares u64-to-u64 without casting.

crates/solver-service/src/apis/tokens.rs (1)

342-400: Deduplicate repeated JSON config fixtures to reduce drift risk.

Lines 342-400, 732-810, and 814-875 duplicate a large shared config shape. This is brittle when schema fields evolve and increases maintenance cost.

♻️ Proposed refactor sketch

+fn base_test_config_json() -> serde_json::Value {
+    serde_json::json!({
+        "solver": {
+            "id": "test-solver",
+            "monitoring_timeout_seconds": 30,
+            "min_profitability_pct": 1.0
+        },
+        "storage": {
+            "primary": "memory",
+            "cleanup_interval_seconds": 3600,
+            "implementations": { "memory": {} }
+        },
+        "delivery": { "min_confirmations": 1, "implementations": {} },
+        "account": {
+            "primary": "local",
+            "implementations": {
+                "local": {
+                    "private_key": "0x1234567890123456789012345678901234567890123456789012345678901234"
+                }
+            }
+        },
+        "discovery": { "implementations": {} },
+        "order": {
+            "implementations": {},
+            "strategy": { "primary": "simple", "implementations": { "simple": {} } }
+        },
+        "settlement": { "implementations": {} }
+    })
+}
+
+fn config_from_networks(networks: serde_json::Value, err: &str) -> Config {
+    let mut v = base_test_config_json();
+    v["networks"] = networks;
+    serde_json::from_value(v).expect(err)
+}
+
 fn create_test_config() -> Config {
-    serde_json::from_value(serde_json::json!({ ...full duplicated object... }))
-        .expect("Failed to parse test config")
+    config_from_networks(
+        serde_json::json!({
+            "1": { ... },
+            "137": { ... }
+        }),
+        "Failed to parse test config",
+    )
 }
 
 fn create_test_config_with_empty_tokens() -> Config {
-    serde_json::from_value(serde_json::json!({ ...full duplicated object... }))
-        .expect("Failed to parse test config with empty tokens")
+    config_from_networks(
+        serde_json::json!({
+            "1": { ... "tokens": [] },
+            "137": { ... "tokens": [] }
+        }),
+        "Failed to parse test config with empty tokens",
+    )
 }

Also applies to: 732-810, 814-875

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/solver-service/src/apis/tokens.rs` around lines 342 - 400, The large
JSON test config used to build a solver_config::Config (the local variable
config in crates/solver-service/src/apis/tokens.rs) is duplicated across three
regions; extract it into a single reusable helper (e.g., fn make_test_config()
-> solver_config::Config or fn test_config_value() -> serde_json::Value) and
have each test call that helper and then .expect(...) as before. Replace the
inline serde_json::json!({ ... }) blocks with a call to the new helper, update
any callers to mutate only the few fields they need for specific cases, and keep
the helper in the same module or a shared test utils module so all occurrences
(the blocks around lines shown) reference one source of truth.

crates/solver-service/src/apis/quote/validation.rs (1)

723-765: Deduplicate the JSON test config fixture to avoid schema drift.

These two helpers duplicate a large config payload. A shared base fixture builder will reduce breakage when solver_config::Config evolves.

♻️ Proposed refactor

+fn base_test_config_json() -> serde_json::Value {
+    serde_json::json!({
+        "solver": {
+            "id": "test-solver",
+            "monitoring_timeout_seconds": 30,
+            "min_profitability_pct": 1.0
+        },
+        "storage": {
+            "primary": "memory",
+            "cleanup_interval_seconds": 3600,
+            "implementations": { "memory": {} }
+        },
+        "delivery": { "min_confirmations": 1, "implementations": {} },
+        "account": {
+            "primary": "local",
+            "implementations": {
+                "local": {
+                    "private_key": "0x1234567890123456789012345678901234567890123456789012345678901234"
+                }
+            }
+        },
+        "discovery": { "implementations": {} },
+        "order": {
+            "implementations": {},
+            "strategy": {
+                "primary": "simple",
+                "implementations": { "simple": {} }
+            }
+        },
+        "settlement": { "implementations": {} },
+        "networks": {}
+    })
+}
+
 async fn create_mock_solver_with_networks(networks: NetworksConfig) -> SolverEngine {
-    let config: Config = serde_json::from_value(serde_json::json!({ ... }))
+    let config: Config = serde_json::from_value(base_test_config_json())
         .expect("Failed to parse test config");
     ...
 }
 
 fn create_test_config_with_callbacks(
     simulate_callbacks: bool,
     whitelist: Vec<String>,
 ) -> solver_config::Config {
-    serde_json::from_value(serde_json::json!({ ... }))
+    let mut cfg = base_test_config_json();
+    cfg["order"]["simulate_callbacks"] = serde_json::json!(simulate_callbacks);
+    cfg["order"]["callback_whitelist"] = serde_json::json!(whitelist);
+    serde_json::from_value(cfg)
         .expect("Failed to parse test config")
 }

Also applies to: 772-774, 2097-2141

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/solver-service/src/apis/quote/validation.rs` around lines 723 - 765,
Multiple tests duplicate a large JSON fixture when constructing a Config (seen
as the local variable config: Config built via serde_json::from_value); extract
that JSON into a single helper (e.g., fn make_test_config() -> Config or fn
test_config_value(overrides: Option<Value>) -> Value) and have tests call it
instead of inlining the JSON blob. Implement the helper in this module (or a
shared test util) to return a parsed Config (using serde_json::from_value
internally) and add an optional parameter/map to apply small overrides for the
few tests that need differences; replace the duplicated inline serde_json::json!
blocks (including the other occurrences noted) with calls to the new helper so
schema changes to solver_config::Config are centralized.

crates/solver-demo/src/operations/env/mod.rs (1)

718-749: Consider using proper JSON parsing for more robust address injection.

The text-based string replacement approach works but has edge cases:

1. No validation that resulting JSON remains valid after replacement
2. replace() replaces all occurrences—unintended matches in comments or nested strings could be affected
3. The unquoted pattern branch (line 724) adding quotes could break JSON structure in edge cases

For a local development CLI this is likely acceptable, but proper serde_json parsing would be more robust.

♻️ Suggested approach using serde_json

// Pseudocode approach:
let mut json: serde_json::Value = serde_json::from_str(&content)?;

// Recursively walk the JSON tree and replace matching string values
fn replace_in_value(value: &mut serde_json::Value, old: &str, new: &str) {
    match value {
        serde_json::Value::String(s) if s == old => *s = new.to_string(),
        serde_json::Value::Object(map) => {
            for v in map.values_mut() {
                replace_in_value(v, old, new);
            }
        }
        serde_json::Value::Array(arr) => {
            for v in arr {
                replace_in_value(v, old, new);
            }
        }
        _ => {}
    }
}

replace_in_value(&mut json, &placeholder_address, actual_address);
let updated_content = serde_json::to_string_pretty(&json)?;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/solver-demo/src/operations/env/mod.rs` around lines 718 - 749, Replace
the fragile text-replace logic that builds patterns_to_replace and uses
updated_content.replace(...) with a proper serde_json-based approach: parse
content using serde_json::from_str into a serde_json::Value, implement a small
recursive function (e.g., replace_in_value(&mut serde_json::Value, old: &str,
new: &str)) that only replaces serde_json::Value::String values equal to
placeholder_address with actual_address, call that on the parsed JSON, then
serialize back with serde_json::to_string_pretty and write it to target_file;
keep the existing replacement_made/warning behavior by tracking whether
replace_in_value made any change and log the same message via logging::warning
if no replacement occurred, and return the same Error::Io/serde_json errors
mapped as needed.