Merge pull request #944 from Onomebello/chore/address-review-findings-request-id-docs-magic-numbers

Add request_id middleware test, doc comments, and validation constants

Author: Maki-Zeninn

contracts/router-core/src/lib.rs

@@ -1033,6 +1033,23 @@ impl RouterCore {
     /// The dependency is stored as a direct prerequisite for `route`. The
     /// dependency must already exist, and adding the edge must not introduce a
     /// cycle. Caller must be the admin.
+    ///
+    /// # Arguments
+    /// * `env` - The Soroban environment.
+    /// * `caller` - The address initiating the call; must be the admin.
+    /// * `route` - The route that will depend on `depends_on`.
+    /// * `depends_on` - The route that must exist and resolve before `route`.
+    ///
+    /// # Returns
+    /// `Ok(())` on success.
+    ///
+    /// # Errors
+    /// * [`RouterError::Unauthorized`] — if `caller` is not the admin.
+    /// * [`RouterError::RouteNotFound`] — if `route` or `depends_on` does not exist.
+    /// * [`RouterError::CircularDependency`] — if `route` and `depends_on` are the
+    ///   same, or if adding the edge would create a dependency cycle.
+    /// * [`RouterError::RecursionLimitExceeded`] — if the existing dependency
+    ///   graph is too deep to safely validate for cycles.
     pub fn set_route_dependency(
         env: Env,
         caller: Address,
@@ -1081,6 +1098,17 @@ impl RouterCore {
     }
 
     /// Return the direct dependencies for a route.
+    ///
+    /// # Arguments
+    /// * `env` - The Soroban environment.
+    /// * `route` - The name of the route to look up.
+    ///
+    /// # Returns
+    /// A [`Vec<String>`] of the route names that `route` directly depends on,
+    /// in the order they were added. Empty if `route` has no dependencies.
+    ///
+    /// # Errors
+    /// * [`RouterError::RouteNotFound`] — if `route` does not exist.
     pub fn get_route_dependencies(env: Env, route: String) -> Result<Vec<String>, RouterError> {
         router_common::extend_instance_ttl(&env, INSTANCE_TTL_THRESHOLD, INSTANCE_TTL_EXTEND_TO);
         if !env.storage().instance().has(&DataKey::Route(route.clone())) {
@@ -1091,6 +1119,26 @@ impl RouterCore {
     }
 
     /// Resolve a route together with all of its dependencies in dependency-first order.
+    ///
+    /// Walks the dependency graph rooted at `name` depth-first and returns each
+    /// resolved route paired with its address, ordered so that every
+    /// dependency appears before the routes that depend on it.
+    ///
+    /// # Arguments
+    /// * `env` - The Soroban environment.
+    /// * `name` - The name of the route to resolve, along with its dependencies.
+    ///
+    /// # Returns
+    /// A [`Vec<(String, Address)>`] of `(route_name, address)` pairs in
+    /// dependency-first order, including `name` itself last.
+    ///
+    /// # Errors
+    /// * [`RouterError::RouterPaused`] — if the router is globally paused.
+    /// * [`RouterError::RouteNotFound`] — if `name` or any of its dependencies
+    ///   does not exist.
+    /// * [`RouterError::RoutePaused`] — if `name` or any of its dependencies is paused.
+    /// * [`RouterError::CircularDependency`] — if the dependency graph contains a cycle.
+    /// * [`RouterError::RecursionLimitExceeded`] — if the dependency graph is too deep.
     pub fn resolve_with_dependencies(
         env: Env,
         name: String,

metrics/src/server.rs

@@ -305,6 +305,37 @@ mod tests {
         assert_eq!(response.status(), StatusCode::SERVICE_UNAVAILABLE);
     }
 
+    #[tokio::test]
+    async fn test_request_id_middleware_sets_response_header() {
+        let registry = Registry::new();
+        let state = AppState { registry };
+        let app = Router::new()
+            .route("/health", get(health_handler))
+            .layer(middleware::from_fn(request_id_middleware))
+            .with_state(state);
+
+        let response = app
+            .oneshot(
+                Request::builder()
+                    .uri("/health")
+                    .body(Body::empty())
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+
+        assert_eq!(response.status(), StatusCode::OK);
+        let request_id = response
+            .headers()
+            .get("x-request-id")
+            .and_then(|v| v.to_str().ok())
+            .unwrap_or("");
+        assert!(
+            !request_id.is_empty(),
+            "expected non-empty x-request-id header"
+        );
+    }
+
     #[tokio::test]
     async fn test_ready_returns_200_when_router_up_is_one() {
         let registry = Registry::new();

metrics/src/validation.rs

@@ -35,15 +35,21 @@ impl ValidationError {
 
 // ── Validation rules ──────────────────────────────────────────────────────────
 
-/// Validate a contract ID: must be a 56-character alphanumeric Stellar address.
+/// Required length of a Stellar contract ID (a base32-style strkey/address string).
+const STELLAR_CONTRACT_ID_LEN: usize = 56;
+
+/// Maximum allowed length of a route name.
+const MAX_ROUTE_NAME_LEN: usize = 64;
+
+/// Validate a contract ID: must be a `STELLAR_CONTRACT_ID_LEN`-character alphanumeric Stellar address.
 pub fn validate_contract_id(id: &str) -> Result<(), ValidationError> {
     if id.is_empty() {
         return Err(ValidationError::new("contract_id must not be empty"));
     }
-    if id.len() != 56 {
-        return Err(ValidationError::new(
-            "contract_id must be exactly 56 characters",
-        ));
+    if id.len() != STELLAR_CONTRACT_ID_LEN {
+        return Err(ValidationError::new(format!(
+            "contract_id must be exactly {STELLAR_CONTRACT_ID_LEN} characters"
+        )));
     }
     if !id.chars().all(|c| c.is_ascii_alphanumeric()) {
         return Err(ValidationError::new(
@@ -53,7 +59,7 @@ pub fn validate_contract_id(id: &str) -> Result<(), ValidationError> {
     Ok(())
 }
 
-/// Validate a route name: non-empty, max 64 chars, alphanumeric + underscore/hyphen.
+/// Validate a route name: non-empty, max `MAX_ROUTE_NAME_LEN` chars, alphanumeric + underscore/hyphen.
 ///
 /// Not yet called from any handler — kept for validating route-name query
 /// params once an endpoint accepts one.
@@ -62,10 +68,10 @@ pub fn validate_route_name(name: &str) -> Result<(), ValidationError> {
     if name.is_empty() {
         return Err(ValidationError::new("route name must not be empty"));
     }
-    if name.len() > 64 {
-        return Err(ValidationError::new(
-            "route name must be 64 characters or fewer",
-        ));
+    if name.len() > MAX_ROUTE_NAME_LEN {
+        return Err(ValidationError::new(format!(
+            "route name must be {MAX_ROUTE_NAME_LEN} characters or fewer"
+        )));
     }
     if !name
         .chars()
@@ -108,7 +114,7 @@ mod tests {
 
     #[test]
     fn valid_contract_id() {
-        let id = "A".repeat(56);
+        let id = "A".repeat(STELLAR_CONTRACT_ID_LEN);
         assert!(validate_contract_id(&id).is_ok());
     }
 
@@ -124,7 +130,7 @@ mod tests {
 
     #[test]
     fn contract_id_with_special_chars_rejected() {
-        let id = format!("{}!", "A".repeat(55));
+        let id = format!("{}!", "A".repeat(STELLAR_CONTRACT_ID_LEN - 1));
         assert!(validate_contract_id(&id).is_err());
     }
 
@@ -140,7 +146,7 @@ mod tests {
 
     #[test]
     fn long_route_name_rejected() {
-        assert!(validate_route_name(&"a".repeat(65)).is_err());
+        assert!(validate_route_name(&"a".repeat(MAX_ROUTE_NAME_LEN + 1)).is_err());
     }
 
     #[test]