[Do not merge]review: add comments and simple refactor

crates/citadel-lens/src/api.rs

@@ -29,23 +29,25 @@ pub fn build_router(state: AppState) -> Router {
         .route("/health", get(health))
         .route("/api/v1/health", get(health))
         .route("/ready", get(ready))
-        // Releases
-        .route("/api/v1/releases", get(list_releases))
-        .route("/api/v1/releases", post(create_release))
-        .route("/api/v1/releases/:id", get(get_release))
-        .route("/api/v1/releases/:id", delete(delete_release))
-        // Categories
-        .route("/api/v1/content-categories", get(list_categories))
-        // Featured releases (for flagship home page)
-        .route("/api/v1/featured-releases", get(list_featured_releases))
-        // Account (identity management)
-        .route("/api/v1/account/:public_key", get(get_account))
-        // Network mesh topology map
-        .route("/api/v1/map", get(get_network_map))
-        // Mesh state (slots, peers, TGP sessions)
-        .route("/api/v1/mesh/state", get(get_mesh_state))
-        // WebSocket for real-time mesh updates
-        .route("/api/v1/ws/mesh", get(ws_mesh_handler))
+        .nest("/api/v1",
+            // Releases
+            .route("/releases", get(list_releases))
+            .route("/releases", post(create_release))
+            .route("/releases/:id", get(get_release))
+            .route("/releases/:id", delete(delete_release))
+            // Categories
+            .route("/content-categories", get(list_categories))
+            // Featured releases (for flagship home page)
+            .route("/featured-releases", get(list_featured_releases))
+            // Account (identity management)
+            .route("/account/:public_key", get(get_account))
+            // Network mesh topology map
+            .route("/map", get(get_network_map))
+            // Mesh state (slots, peers, TGP sessions)
+            .route("/mesh/state", get(get_mesh_state))
+            // WebSocket for real-time mesh updates
+            .route("/ws/mesh", get(ws_mesh_handler))
+        )
         .layer(cors)
         .with_state(state)
 }
@@ -73,6 +75,8 @@ async fn list_releases(
     Ok(Json(releases))
 }
 
+// Ben Review: Premature optimisation possible: instead of String, some sort of optimised string
+// such as a Cow.
 #[derive(Debug, Deserialize)]
 struct CreateReleaseRequest {
     title: String,
@@ -128,6 +132,7 @@ async fn delete_release(
     let state = state.read().await;
     match state.storage.delete_release(&id) {
         Ok(()) => StatusCode::NO_CONTENT,
+        // BenPH review: log the actual error
         Err(_) => StatusCode::INTERNAL_SERVER_ERROR,
     }
 }
@@ -155,6 +160,7 @@ async fn list_featured_releases(
     let releases = state
         .storage
         .list_releases()
+        // BenPH review: log the actual error
         .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
     Ok(Json(releases))
 }
@@ -253,6 +259,7 @@ struct PeerEdge {
     from: String,
     to: String,
     connection_type: String,  // "neighbor" or "relay"
+    // BenPH review: consider using us with u64
     latency_ms: Option<u32>,
     bidirectional: bool,
 }
@@ -263,9 +270,11 @@ struct NetworkStats {
     server_nodes: u32,
     browser_nodes: u32,
     total_edges: u32,
+    // BenPH review: consider using us with u64
     avg_latency_ms: Option<f64>,
 }
 
+// BenPH review: This is a particularly long function. consider refactoring
 async fn get_network_map(
     State(state): State<AppState>,
 ) -> Json<NetworkMap> {
@@ -289,6 +298,8 @@ async fn get_network_map(
             z: claim.coord.z,
         }
     } else {
+        // BenPH review: consider use of Derive(default)
+        //  - the `self_slot_opt` can be initialised with `unwrap_or_default()`
         // No slot claimed yet - position at origin until claimed
         HexSlot { index: None, q: 0, r: 0, z: 0 }
     };
@@ -314,6 +325,9 @@ async fn get_network_map(
     if let Some(ref mesh_state) = state.mesh_state {
         let mesh = mesh_state.read().await;
 
+        // BenPH review: use an iterator, map and collect, or declare nodes and edges with an
+        // expected capacity. Should already be handled via optimiser, but clearly stating intent
+        // can help
         // Add all connected peers as nodes
         // Use peer_id (hashmap key) as authoritative ID - gets updated from hello
         for (peer_id, peer) in &mesh.peers {
@@ -408,6 +422,7 @@ async fn get_network_map(
     })
 }
 
+// BenPH review: this can likely return an &str based on the input id
 /// Extract short ID from peer ID (strips "b3b3/" prefix, shows first 12 chars)
 fn short_peer_id(id: &str) -> String {
     let hash = id.strip_prefix("b3b3/").unwrap_or(id);
@@ -455,6 +470,7 @@ struct PeerSummary {
     last_seen_ms: u64,
 }
 
+// BenPH review: state could probably be bassed in as a `RwLockReadGuard<AppState>`
 async fn get_mesh_state(
     State(state): State<AppState>,
 ) -> Json<MeshStateResponse> {

crates/citadel-lens/src/mesh.rs

@@ -60,6 +60,10 @@
 //! - 2 vertical (directly above/below)
 //! - 12 extended (6 above + 6 below diagonals)
 
+// BenPH review: lots of use of VerifyingKey in a String representation where the length of the
+// BenPH review: Lot's of different things going on. suggest mesh as a top level module that
+// re-exports, the individual components.
+// string is always known. Suggest a structure that doesn't require dynamic memory
 use crate::error::Result;
 use crate::storage::Storage;
 use crate::vdf_race::{VdfRace, VdfLink, AnchoredSlotClaim, claim_has_priority};
@@ -100,7 +104,7 @@ pub fn verify_peer_id(claimed_id: &str, pubkey: &VerifyingKey) -> bool {
 }
 
 /// Mesh node identity and state
-#[derive(Debug, Clone)]
+#[deriie(Debug, Clone)]
 pub struct MeshPeer {
     pub id: String,
     pub addr: SocketAddr,
@@ -118,6 +122,8 @@ pub struct SlotClaim {
     pub index: u64,
     /// 3D hex coordinate
     pub coord: HexCoord,
+    // BenPH review: suggest a wrapper type: `struct PeerId(id: <some string type>)`. Similarly for
+    // other commonly used things represented as a primitive, such as public key.
     /// PeerID that claimed this slot
     pub peer_id: String,
     /// Public key of the claiming peer (for TGP)
@@ -225,6 +231,7 @@ pub struct TgpSession {
     pub peer_tgp_addr: SocketAddr,
 }
 
+// BenPH review: consider using BTreeMap|Set
 /// Mesh service state
 pub struct MeshState {
     /// Our node ID (PeerID)
@@ -402,6 +409,7 @@ impl MeshService {
             listen_addr,
             entry_peers,
             storage,
+            // BenPH review: use `default..`
             state: Arc::new(RwLock::new(MeshState {
                 self_id,
                 signing_key,
@@ -469,6 +477,7 @@ impl MeshService {
         // This function currently floods a claim without TGP validation
         // See docs/MESH_PROTOCOL.md for the correct protocol
 
+        // BenPH review: use a helper function that takes a RwWriteGuard<AppState>
         let mut state = self.state.write().await;
 
         // Check if slot is already claimed
@@ -518,6 +527,7 @@ impl MeshService {
     /// Genesis seed for VDF chain (shared across all nodes in the mesh)
     /// In production, this would be derived from network genesis block or similar
     const VDF_GENESIS_SEED: [u8; 32] = [
+        // BenPH review: use something akin to as_bytes::<...>("....") instead if possible
         0x43, 0x49, 0x54, 0x41, 0x44, 0x45, 0x4c, 0x2d,  // "CITADEL-"
         0x56, 0x44, 0x46, 0x2d, 0x47, 0x45, 0x4e, 0x45,  // "VDF-GENE"
         0x53, 0x49, 0x53, 0x2d, 0x53, 0x45, 0x45, 0x44,  // "SIS-SEED"
@@ -538,6 +548,8 @@ impl MeshService {
     /// Initialize VDF race when joining existing mesh
     /// Takes chain links from bootstrap peer
     pub async fn init_vdf_join(&self, chain_links: Vec<VdfLink>) -> bool {
+        // Ben review use a RwWriteGuard<AppState> helper if possible.
+        // review followup: scan for other places where acquiring the guard is a first line thing
         let mut state = self.state.write().await;
         let signing_key = state.signing_key.clone();
 
@@ -558,6 +570,7 @@ impl MeshService {
     /// Claim a slot with VDF anchoring for deterministic priority
     /// Returns the anchored claim for flooding to the network
     pub async fn claim_slot_with_vdf(&self, index: u64) -> Option<AnchoredSlotClaim> {
+        // Ben review use a RwWriteGuard<AppState> helper if possible.
         let mut state = self.state.write().await;
 
         // Ensure VDF race is initialized
@@ -836,6 +849,7 @@ impl MeshService {
         Some((rounds, slots))
     }
 
+    // BenPH review: much of the following functions can be a single dot-chain
     /// Check if we should adopt another chain (heavier)
     pub async fn cvdf_should_adopt(&self, other_rounds: &[CvdfRound]) -> bool {
         let state = self.state.read().await;
@@ -876,6 +890,7 @@ impl MeshService {
         state.cvdf.is_some()
     }
 
+    // BenPH review: consider renaming to `main_cvdf_loop`
     /// Run CVDF coordination loop
     /// This handles periodic attestation and round production
     pub async fn run_cvdf_loop(&self) {
@@ -937,6 +952,7 @@ impl MeshService {
     ///
     /// Returns `true` if slot was successfully occupied.
     pub async fn attempt_slot_via_tgp(&self, target_slot: u64) -> bool {
+        // BenPH review: long function, consider breaking up
         let state = self.state.read().await;
 
         // Get mesh size for threshold calculation
@@ -1017,7 +1033,7 @@ impl MeshService {
                 warn!("Invalid public key for {}", peer_id);
                 continue;
             };
-
+.
             // Get cached TGP keypair (zerocopy - just clone the Arc's content)
             let my_keypair = {
                 let state = self.state.read().await;
@@ -1136,6 +1152,7 @@ impl MeshService {
         priority_a < priority_b  // Lower hash wins
     }
 
+    // BenPH review: long function, consider breaking up
     /// Process a slot claim from another node
     /// Returns true if WE lost our slot to this claim (caller should reclaim)
     pub async fn process_slot_claim(&self, index: u64, peer_id: String, coord: (i64, i64, i64), public_key: Option<Vec<u8>>) -> bool {
@@ -1247,6 +1264,7 @@ impl MeshService {
         self.flood_tx.clone()
     }
 
+    // BenPH review: consider renaming to `main_...`
     /// Run TGP UDP listener - receives incoming TGP messages from any peer
     /// This is connectionless - we can receive from anyone who knows our address
     /// Event-driven: immediately responds after receiving each message
@@ -1280,6 +1298,7 @@ impl MeshService {
         }
     }
 
+    // BenPH review: long function, consider breaking up
     /// Handle incoming TGP message from UDP.
     /// Returns the peer_id if message was processed (for sending response).
     /// Uses symmetric TGP - party roles determined by public key comparison.

crates/citadel-lens/src/node.rs

@@ -27,6 +27,7 @@ pub struct LensConfig {
     /// P2P listen address (for mesh)
     pub p2p_addr: SocketAddr,
 
+    // BenPH review: consider something like `type BsPeers(string)`
     /// Bootstrap peers
     pub bootstrap_peers: Vec<String>,
 
@@ -100,11 +101,15 @@ impl LensConfig {
 pub struct LensState {
     pub storage: Arc<Storage>,
     pub config: LensConfig,
+    // BenPH review: go to all places where read and write guards are obtained imediately on
+    // function entry, and consider passing in the RwLockRead|WriteGuard<MeshState> in some way
     pub mesh_state: Option<Arc<RwLock<MeshState>>>,
 }
 
 /// A Lens node instance.
 pub struct LensNode {
+    // BenPH review: go to all places where read and write guards are obtained imediately on
+    // function entry, and consider passing in the RwLockRead|WriteGuard<LensState> in some way
     state: Arc<RwLock<LensState>>,
     config: LensConfig,
 }
@@ -144,7 +149,7 @@ impl LensNode {
     /// Get shared storage for admin socket.
     pub async fn storage(&self) -> Arc<Storage> {
         Arc::clone(&self.state.read().await.storage)
-    }
+    }.
 
     /// Run the node (starts HTTP server, admin socket, and P2P mesh).
     pub async fn run(self) -> Result<()> {