diff --git a/crates/citadel-lens/src/api.rs b/crates/citadel-lens/src/api.rs index 3c775bc..5773d07 100644 --- a/crates/citadel-lens/src/api.rs +++ b/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, 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, } +// BenPH review: This is a particularly long function. consider refactoring async fn get_network_map( State(state): State, ) -> Json { @@ -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` async fn get_mesh_state( State(state): State, ) -> Json { diff --git a/crates/citadel-lens/src/mesh.rs b/crates/citadel-lens/src/mesh.rs index 4779b6b..69d4e12 100644 --- a/crates/citadel-lens/src/mesh.rs +++ b/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: )`. 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 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) -> bool { + // Ben review use a RwWriteGuard 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 { + // Ben review use a RwWriteGuard 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>) -> 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. diff --git a/crates/citadel-lens/src/node.rs b/crates/citadel-lens/src/node.rs index 5307103..42462cf 100644 --- a/crates/citadel-lens/src/node.rs +++ b/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, @@ -100,11 +101,15 @@ impl LensConfig { pub struct LensState { pub storage: Arc, 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 in some way pub mesh_state: Option>>, } /// 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 in some way state: Arc>, config: LensConfig, } @@ -144,7 +149,7 @@ impl LensNode { /// Get shared storage for admin socket. pub async fn storage(&self) -> Arc { Arc::clone(&self.state.read().await.storage) - } + }. /// Run the node (starts HTTP server, admin socket, and P2P mesh). pub async fn run(self) -> Result<()> {