refactor(challenge-sdk): remove hardcoded routes, add custom route handler support - #48
Conversation
…lenge handlers
Remove challenge-specific hardcoded routes from the platform SDK and extend
the architecture so that each challenge defines its own routes and handlers.
The platform SDK should provide generic infrastructure, not dictate
challenge-specific API routes like /leaderboard, /submit, /status, etc.
Challenges now declare their own routes via ServerChallenge::routes() and
handle them via ServerChallenge::handle_route().
Key changes:
- Remove RoutesManifest::with_standard_routes() which hardcoded /submit,
/status/:hash, /leaderboard, /config, /stats, /health challenge routes
and its associated test (crates/challenge-sdk/src/routes.rs)
- Add ChallengeContext struct providing route handlers with access to the
local sled database, challenge ID, epoch, and block height
(crates/challenge-sdk/src/server.rs)
- Extend ServerChallenge trait with routes() and handle_route() default
methods so challenges declare and handle their own custom routes without
breaking existing implementations (crates/challenge-sdk/src/server.rs)
- Add fallback handler in ChallengeServer::run() that matches incoming
requests against challenge-declared routes and delegates to
handle_route() (crates/challenge-sdk/src/server.rs)
- Implement challenge_call RPC method in the JSON-RPC handler, enabling
route invocation via JSON-RPC with params {challengeId, method, path,
body, query}. Add handle_async() to support async route handler
callbacks (crates/rpc-server/src/jsonrpc.rs)
- Convert handle_single_request to async and wire it through
handle_async() so challenge_call works end-to-end. Update test call
sites accordingly (crates/rpc-server/src/server.rs)
- Export ChallengeContext, ChallengeRoute, RouteRequest, RouteResponse
from lib.rs and prelude (crates/challenge-sdk/src/lib.rs)
- Update doc comments in routes.rs and server.rs to show the
challenge-defines-its-own-routes pattern as the primary usage example
📝 WalkthroughWalkthroughThese changes introduce platform-level route extensibility for challenges by adding a Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Server as Challenge Server
participant Handler as custom_route_handler
participant Challenge as ServerChallenge
participant DB as ChallengeDatabase
Client->>Server: HTTP Request (GET /challenges/custom-route)
Server->>Handler: Route to fallback handler
Handler->>Challenge: Resolve matching route via routes()
alt Route Found
Handler->>Handler: Build ChallengeContext<br/>(challenge_id, epoch, block_height)
Handler->>Handler: Build RouteRequest<br/>(method, path, query, headers, body)
Handler->>Challenge: await handle_route(ctx, request)
Challenge->>DB: Query challenge data (via context.db)
DB-->>Challenge: Challenge data
Challenge-->>Handler: RouteResponse
Handler->>Client: HTTP Response<br/>(StatusCode, JSON Body)
else Route Not Found
Handler->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/challenge-sdk/src/routes.rs (1)
1-54:⚠️ Potential issue | 🟡 MinorModule doc comments are garbled — duplicated fragments and orphaned code outside fenced blocks.
Lines 5–9 appear to be a partial snippet that was left over from editing (the
usestatement andimplopening without a surrounding```textfence, and line 9 is a dangling sentence fragment). Lines 46–54 duplicate content from earlier: they contain a closing comment about "generic building blocks" and raw code (_ => RouteResponse::not_found(), closing braces) outside any code fence.Suggested cleanup
-//! Provides the generic route infrastructure for challenges to define custom -//! HTTP routes that get mounted on the RPC server. Each challenge declares its -//! own routes and handlers via the `ServerChallenge` trait — the platform SDK -//! does NOT hardcode any challenge-specific routes. -//! use platform_challenge_sdk::server::{ServerChallenge, ChallengeContext}; -//! impl ServerChallenge for MyChallenge { -//! // ... challenge_id, name, version, evaluate ... -//! -//! on the RPC server. Each challenge can expose its own API endpoints. +//! Provides the generic route infrastructure for challenges to define custom +//! HTTP routes that get mounted on the RPC server. Each challenge declares its +//! own routes and handlers via the `ServerChallenge` trait — the platform SDK +//! does NOT hardcode any challenge-specific routes. //! //! # Example //! //! ```text //! use platform_challenge_sdk::routes::*; +//! use platform_challenge_sdk::server::{ServerChallenge, ChallengeContext}; //! //! impl Challenge for MyChallenge { //! fn routes(&self) -> Vec<ChallengeRoute> { @@ ..keep the example as-is through line 43.. //! } //! } //! ``` -//! -//! // The platform SDK provides the generic building blocks (ChallengeRoute, -//! // RouteRequest, RouteResponse, RouteRegistry, RouteBuilder, RoutesManifest, -//! // HttpMethod) — challenges use these to declare their own routes. -//! _ => RouteResponse::not_found() -//! } -//! } -//! } -//! ``` +//! +//! The platform SDK provides the generic building blocks (`ChallengeRoute`, +//! `RouteRequest`, `RouteResponse`, `RouteRegistry`, `RouteBuilder`, +//! `RoutesManifest`, `HttpMethod`) — challenges use these to declare their own +//! routes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/challenge-sdk/src/routes.rs` around lines 1 - 54, Module doc comments are garbled with orphaned code fragments and duplicated content; clean up the docblock in routes.rs by removing the stray `use platform_challenge_sdk::routes::*;` / `impl` fragments outside the fenced example, ensure the example code (the impl Challenge for MyChallenge example that mentions ChallengeRoute, RouteRequest, RouteResponse) is fully enclosed in a single triple-backtick fenced block and ends with a single closing fence, and replace the duplicated raw code after the example with a short prose paragraph describing the SDK building blocks (mentioning ChallengeRoute, RouteRequest, RouteResponse, RouteRegistry, RouteBuilder, RoutesManifest, HttpMethod) so there is no orphaned `_ => RouteResponse::not_found()` or extra braces outside the fenced example.
🧹 Nitpick comments (3)
crates/rpc-server/src/jsonrpc.rs (1)
340-345: Sync-path error forchallenge_callleaks internal detail.The message
"challenge_call must be invoked via handle_async()"exposes an internal routing concern. Since all requests now go throughhandle_async()inserver.rs(line 403), this branch should be unreachable in normal operation, but if hit, a user-facing message like"Internal routing error"would be more appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc-server/src/jsonrpc.rs` around lines 340 - 345, The error branch for the "challenge","call" path currently returns JsonRpcResponse::error(req.id, INTERNAL_ERROR, "challenge_call must be invoked via handle_async()") which leaks internal routing details; change the user-facing message to a generic one (e.g., "Internal routing error") while keeping the same error code and response shape so callers get a non-sensitive, consistent INTERNAL_ERROR. Update the message in the branch that constructs JsonRpcResponse::error for the ["challenge","call"] match (referencing the challenge_call handling and existing JsonRpcResponse::error/INTERNAL_ERROR usage) to a generic internal error string.crates/rpc-server/src/server.rs (1)
353-356: Batch JSON-RPC support is incomplete (pre-existing).The batch path only processes the first element and discards the rest. The JSON-RPC 2.0 spec requires returning an array of responses for batch requests. Consider tracking this as technical debt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc-server/src/server.rs` around lines 353 - 356, The batch handling in server.rs currently only processes the first element of a JSON-RPC array and returns a single response; update the code that checks body.as_array() to iterate over all elements, call handle_single_request for each (e.g., map or spawn futures calling handle_single_request(element.clone(), &handler)), await and collect all individual responses into a JSON array, and return that array as the batch response per JSON-RPC 2.0; ensure you preserve async behavior and error/notification handling for each item when building the aggregated response.crates/challenge-sdk/src/server.rs (1)
581-615:routes()is re-evaluated on every fallback request — consider caching.
state.challenge.routes()allocates a newVec<ChallengeRoute>on every request that hits the fallback (including random crawlers, favicon, etc.). Since routes are declared at trait-implementation time and don't change at runtime, caching them once at startup (e.g., inServerState) would avoid repeated allocation and pattern matching for every unmatched path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/challenge-sdk/src/server.rs` around lines 581 - 615, The handler currently calls state.challenge.routes() on every request which reallocates the Vec; instead add a cached field (e.g., cached_routes: Vec<ChallengeRoute>) to ServerState and populate it once when constructing ServerState (using C::routes() / ServerChallenge::routes()), then change custom_route_handler to use State(state).cached_routes (or similar) rather than calling state.challenge.routes() repeatedly; update ServerState construction site to build and store the routes so custom_route_handler, route matching loop, and any references to dynamic routes use the cached Vec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/challenge-sdk/src/server.rs`:
- Around line 635-648: The current code creates a new ephemeral sled DB per
request by calling ChallengeDatabase::open(...) with ChallengeId::new() inside
the handler when constructing ChallengeContext; move DB creation out of the
handler and into the server initialization (e.g., ChallengeServer::build() or
run()) so a single ChallengeDatabase instance is opened once and stored on
ServerState (alongside challenge and pending_count), then modify
custom_route_handler to construct ChallengeContext using the shared
ServerState.db, preserving challenge_id/epoch/block_height as before; also
remove the redundant unwrap_or_else retry and ensure the shared DB is opened
with a deterministic ChallengeId or configuration to avoid temp-file leaks.
In `@crates/rpc-server/src/jsonrpc.rs`:
- Around line 1157-1165: challenge_call builds a RouteRequest with an empty
params map so path parameters are never extracted and handlers using
req.param("...") receive None; update challenge_call to perform the same route
matching used by the HTTP handler: locate the route registry used by the server,
match the incoming path against registered route patterns, extract any named
path parameters and populate RouteRequest.params with those key/value pairs
before invoking the handler (i.e., ensure RouteRequest { params, method, path,
... } contains the extracted params), or if you cannot perform matching here add
clear documentation on challenge_call stating that path params are not populated
so handlers must not rely on req.param.
- Around line 1140-1155: The code path in challenge_call that checks
challenge_routes vs chain_state only verifies existence by name but keeps using
the original challenge_id (which may be a name) so downstream handlers get the
wrong identifier; change the binding to let mut challenge_id (instead of an
immutable var) and, when you find a match in self.chain_state.read().challenges
(the block using found), set challenge_id = actual_id (the canonical key from
chain.challenges) before continuing so JsonRpcResponse::error and the route
handler receive the canonical challenge ID; ensure you update uses in the
surrounding scope (the routes lookup and later handler invocation) to use the
resolved actual_id.
---
Outside diff comments:
In `@crates/challenge-sdk/src/routes.rs`:
- Around line 1-54: Module doc comments are garbled with orphaned code fragments
and duplicated content; clean up the docblock in routes.rs by removing the stray
`use platform_challenge_sdk::routes::*;` / `impl` fragments outside the fenced
example, ensure the example code (the impl Challenge for MyChallenge example
that mentions ChallengeRoute, RouteRequest, RouteResponse) is fully enclosed in
a single triple-backtick fenced block and ends with a single closing fence, and
replace the duplicated raw code after the example with a short prose paragraph
describing the SDK building blocks (mentioning ChallengeRoute, RouteRequest,
RouteResponse, RouteRegistry, RouteBuilder, RoutesManifest, HttpMethod) so there
is no orphaned `_ => RouteResponse::not_found()` or extra braces outside the
fenced example.
---
Nitpick comments:
In `@crates/challenge-sdk/src/server.rs`:
- Around line 581-615: The handler currently calls state.challenge.routes() on
every request which reallocates the Vec; instead add a cached field (e.g.,
cached_routes: Vec<ChallengeRoute>) to ServerState and populate it once when
constructing ServerState (using C::routes() / ServerChallenge::routes()), then
change custom_route_handler to use State(state).cached_routes (or similar)
rather than calling state.challenge.routes() repeatedly; update ServerState
construction site to build and store the routes so custom_route_handler, route
matching loop, and any references to dynamic routes use the cached Vec.
In `@crates/rpc-server/src/jsonrpc.rs`:
- Around line 340-345: The error branch for the "challenge","call" path
currently returns JsonRpcResponse::error(req.id, INTERNAL_ERROR, "challenge_call
must be invoked via handle_async()") which leaks internal routing details;
change the user-facing message to a generic one (e.g., "Internal routing error")
while keeping the same error code and response shape so callers get a
non-sensitive, consistent INTERNAL_ERROR. Update the message in the branch that
constructs JsonRpcResponse::error for the ["challenge","call"] match
(referencing the challenge_call handling and existing
JsonRpcResponse::error/INTERNAL_ERROR usage) to a generic internal error string.
In `@crates/rpc-server/src/server.rs`:
- Around line 353-356: The batch handling in server.rs currently only processes
the first element of a JSON-RPC array and returns a single response; update the
code that checks body.as_array() to iterate over all elements, call
handle_single_request for each (e.g., map or spawn futures calling
handle_single_request(element.clone(), &handler)), await and collect all
individual responses into a JSON array, and return that array as the batch
response per JSON-RPC 2.0; ensure you preserve async behavior and
error/notification handling for each item when building the aggregated response.
| // ============================================================================ | ||
| // CHALLENGE CONTEXT | ||
| // ============================================================================ | ||
|
|
||
| /// Context provided to route handlers, giving access to shared resources | ||
| /// | ||
| /// Route handlers receive this to access the local sled database and chain | ||
| /// state when handling custom routes. | ||
| pub struct ChallengeContext { | ||
| /// Local challenge database (sled) | ||
| pub db: Arc<ChallengeDatabase>, | ||
| /// Challenge ID | ||
| pub challenge_id: String, | ||
| /// Current epoch | ||
| pub epoch: u64, | ||
| /// Current block height | ||
| pub block_height: u64, | ||
| } |
There was a problem hiding this comment.
ChallengeContext fields epoch and block_height are always zero in the fallback handler.
While the struct is well-designed, the fallback handler at line 647 hardcodes epoch: 0 and block_height: 0. Route handlers that rely on these fields will get stale/incorrect values. Consider plumbing actual chain state into the context, or documenting that these fields are placeholder in standalone server mode.
| // Build a minimal ChallengeContext (no database in fallback handler) | ||
| // In production, the ChallengeContext would be populated by the validator node | ||
| let ctx = ChallengeContext { | ||
| db: Arc::new( | ||
| ChallengeDatabase::open(std::env::temp_dir(), crate::types::ChallengeId::new()) | ||
| .unwrap_or_else(|_| { | ||
| ChallengeDatabase::open(std::env::temp_dir(), crate::types::ChallengeId::new()) | ||
| .expect("Failed to open temporary challenge database") | ||
| }), | ||
| ), | ||
| challenge_id: state.challenge.challenge_id().to_string(), | ||
| epoch: 0, | ||
| block_height: 0, | ||
| }; |
There was a problem hiding this comment.
Per-request ephemeral database — data doesn't persist and sled files leak.
Every matched custom-route request opens a new sled database with a fresh ChallengeId::new() in temp_dir. This means:
- Data written by the handler is lost immediately after the response — successive requests never see each other's state.
- Disk resources leak — sled creates directories/files that are never cleaned up.
- The fallback in
unwrap_or_elseretries the identical operation with yet another random ID, which won't resolve the underlying failure.
The ChallengeContext.db should be created once (e.g., during ChallengeServer::build() or run()) and shared across requests via the ServerState, similar to how challenge and pending_count are already shared.
Sketch: lift DB into ServerState
pub struct ServerState<C: ServerChallenge> {
pub challenge: Arc<C>,
pub config: ServerConfig,
pub started_at: Instant,
pub pending_count: Arc<RwLock<u32>>,
+ pub db: Arc<ChallengeDatabase>,
}Then in custom_route_handler, build the context from the shared state:
- let ctx = ChallengeContext {
- db: Arc::new(
- ChallengeDatabase::open(std::env::temp_dir(), crate::types::ChallengeId::new())
- .unwrap_or_else(|_| {
- ChallengeDatabase::open(std::env::temp_dir(), crate::types::ChallengeId::new())
- .expect("Failed to open temporary challenge database")
- }),
- ),
- challenge_id: state.challenge.challenge_id().to_string(),
- epoch: 0,
- block_height: 0,
- };
+ let ctx = ChallengeContext {
+ db: Arc::clone(&state.db),
+ challenge_id: state.challenge.challenge_id().to_string(),
+ epoch: 0,
+ block_height: 0,
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/challenge-sdk/src/server.rs` around lines 635 - 648, The current code
creates a new ephemeral sled DB per request by calling
ChallengeDatabase::open(...) with ChallengeId::new() inside the handler when
constructing ChallengeContext; move DB creation out of the handler and into the
server initialization (e.g., ChallengeServer::build() or run()) so a single
ChallengeDatabase instance is opened once and stored on ServerState (alongside
challenge and pending_count), then modify custom_route_handler to construct
ChallengeContext using the shared ServerState.db, preserving
challenge_id/epoch/block_height as before; also remove the redundant
unwrap_or_else retry and ensure the shared DB is opened with a deterministic
ChallengeId or configuration to avoid temp-file leaks.
| // Verify the challenge has registered routes | ||
| { | ||
| let routes = self.challenge_routes.read(); | ||
| if !routes.contains_key(&challenge_id) { | ||
| // Try to find by name | ||
| let chain = self.chain_state.read(); | ||
| let found = chain.challenges.values().any(|c| c.name == challenge_id); | ||
| if !found { | ||
| return JsonRpcResponse::error( | ||
| id, | ||
| CHALLENGE_NOT_FOUND, | ||
| format!("Challenge '{}' not found or has no routes", challenge_id), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
challenge_call doesn't resolve challenge name to its actual ID.
When the challenge isn't found by ID in challenge_routes but is found by name in chain_state.challenges, the code only confirms existence (found = true) but continues using the original challenge_id string (which is the name). The route handler on line 1170 then receives the name, not the canonical ID.
Contrast with the HTTP handler in crates/rpc-server/src/server.rs (lines 240–258) which resolves actual_id when a name match is found. This inconsistency means the same challenge may be reachable via HTTP but fail via challenge_call (or vice versa), since the handler callback receives different identifiers.
Suggested fix
{
let routes = self.challenge_routes.read();
if !routes.contains_key(&challenge_id) {
// Try to find by name
let chain = self.chain_state.read();
- let found = chain.challenges.values().any(|c| c.name == challenge_id);
- if !found {
- return JsonRpcResponse::error(
- id,
- CHALLENGE_NOT_FOUND,
- format!("Challenge '{}' not found or has no routes", challenge_id),
- );
+ let actual_id = chain
+ .challenges
+ .values()
+ .find(|c| c.name == challenge_id)
+ .map(|c| c.id.to_string());
+ drop(chain);
+
+ match actual_id {
+ Some(resolved) if routes.contains_key(&resolved) => {
+ challenge_id = resolved;
+ }
+ _ => {
+ return JsonRpcResponse::error(
+ id,
+ CHALLENGE_NOT_FOUND,
+ format!("Challenge '{}' not found or has no routes", challenge_id),
+ );
+ }
}
}
}(This requires changing challenge_id from immutable to let mut challenge_id = ... at line 1109.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/rpc-server/src/jsonrpc.rs` around lines 1140 - 1155, The code path in
challenge_call that checks challenge_routes vs chain_state only verifies
existence by name but keeps using the original challenge_id (which may be a
name) so downstream handlers get the wrong identifier; change the binding to let
mut challenge_id (instead of an immutable var) and, when you find a match in
self.chain_state.read().challenges (the block using found), set challenge_id =
actual_id (the canonical key from chain.challenges) before continuing so
JsonRpcResponse::error and the route handler receive the canonical challenge ID;
ensure you update uses in the surrounding scope (the routes lookup and later
handler invocation) to use the resolved actual_id.
| let request = RouteRequest { | ||
| method, | ||
| path, | ||
| params: std::collections::HashMap::new(), | ||
| query, | ||
| headers: std::collections::HashMap::new(), | ||
| body, | ||
| auth_hotkey: None, | ||
| }; |
There was a problem hiding this comment.
challenge_call skips route matching — path params are never extracted.
The RouteRequest is built with an empty params map. Unlike the HTTP handler in server.rs (lines 273–283), which iterates over registered routes and extracts path parameters (e.g., /:hash → {"hash": "abc123"}), this code path passes the raw path through. Challenge handlers that rely on req.param("hash") will get None when invoked via challenge_call.
Consider performing route matching here as well, or at minimum document that challenge_call does not populate path params.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/rpc-server/src/jsonrpc.rs` around lines 1157 - 1165, challenge_call
builds a RouteRequest with an empty params map so path parameters are never
extracted and handlers using req.param("...") receive None; update
challenge_call to perform the same route matching used by the HTTP handler:
locate the route registry used by the server, match the incoming path against
registered route patterns, extract any named path parameters and populate
RouteRequest.params with those key/value pairs before invoking the handler
(i.e., ensure RouteRequest { params, method, path, ... } contains the extracted
params), or if you cannot perform matching here add clear documentation on
challenge_call stating that path params are not populated so handlers must not
rely on req.param.
…nge routes Security fixes for PR #48 custom route support: - Add challengeId validation (length limit, allowed characters) in challenge_call RPC method and challenge_route_handler HTTP handler - Enhance validate_path to reject null bytes and percent-encoded path traversal patterns (%2e%2e, %2e., .%2e) - Enforce requires_auth flag on matched routes in both RPC and HTTP handlers, returning 401/UNAUTHORIZED when auth is required - Make validate_path and validate_challenge_id pub(crate) for reuse across jsonrpc and server modules
Summary
Removes hardcoded challenge-specific routes from the platform SDK and introduces a generic mechanism for challenges to declare and handle their own routes via the
ServerChallengetrait.The platform SDK should provide infrastructure, not dictate challenge-specific API endpoints like
/leaderboardor/submit. Challenges now define their own routes and handlers, accessing shared resources through aChallengeContext.Changes
crates/challenge-sdk/src/routes.rsRoutesManifest::with_standard_routes()which hardcoded/submit,/status/:hash,/leaderboard,/config,/stats,/healthtest_routes_manifest_with_standard_routesChallengeRoute,RouteRequest,RouteResponse,RouteRegistry,RouteBuilder,RoutesManifest,HttpMethod)crates/challenge-sdk/src/server.rsChallengeContextstruct providing route handlers with access to local sled database, challenge ID, epoch, and block heightroutes()default method toServerChallengetrait for declaring custom routeshandle_route(&self, ctx: &ChallengeContext, request: RouteRequest) -> RouteResponsedefault method for handling route requestscustom_route_handlerfallback inChallengeServer::run()that delegates unmatched paths to the challenge'shandle_route()crates/challenge-sdk/src/lib.rsChallengeContextfromservermoduleChallengeRoute,RouteRequest,RouteResponseto preludecrates/rpc-server/src/jsonrpc.rschallenge_callRPC method that routes JSON-RPC requests to challenge route handlershandle_async()method to support async route handler invocationchallenge_callto therpc_methodslistchallengeId,method,path,body,querycrates/rpc-server/src/server.rshandle_single_requestto async to supporthandle_async()flowBreaking Changes
RoutesManifest::with_standard_routes()is removed. Challenges should declare their own routes viaServerChallenge::routes()instead.ServerChallengeimpls continue to compile without modification.Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
with_standard_routes()convenience method; manual route configuration is now required.