refactor(challenge-sdk): remove hardcoded routes, add custom route handler support#47
Open
refactor(challenge-sdk): remove hardcoded routes, add custom route handler support#47
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
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes introduce custom route handling for challenges and asynchronous RPC request processing. A new ChallengeContext is added to encapsulate shared state for route handlers, the ServerChallenge trait gains routes() and handle_route() methods for custom routing, and RPC request handling is converted to async with a new challenge_call endpoint. The RoutesManifest::with_standard_routes() builder is removed. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTPServer as HTTP Server<br/>(Challenge)
participant Handler as custom_route_handler
participant RouteMatch as Route Matcher
participant Challenge as ServerChallenge<br/>Implementation
participant DB as ChallengeDatabase
Client->>HTTPServer: HTTP Request<br/>(path, method, body)
HTTPServer->>Handler: Fallback handler invoked
Handler->>RouteMatch: Match request against<br/>declared routes
RouteMatch-->>Handler: Route found
Handler->>DB: Create temporary<br/>ChallengeDatabase
Handler->>Handler: Build ChallengeContext<br/>(db, challenge_id, epoch, height)
Handler->>Handler: Build RouteRequest<br/>(method, path, body, query)
Handler->>Challenge: handle_route(&context,<br/>request)
Challenge-->>Handler: RouteResponse
Handler->>HTTPServer: Convert to HTTP<br/>response
HTTPServer-->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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 comment is malformed — duplicate fragments and code outside fenced blocks.
The doc comment has several issues:
- Lines 5–7 contain raw Rust code (
use ...,impl ServerChallenge ...) that sit outside the```textfence starting at line 13.- Line 9 is a dangling sentence fragment ("on the RPC server. Each challenge can expose its own API endpoints.") left over from the previous version.
- Lines 46–54 duplicate the tail of the example (the
_ => RouteResponse::not_found()arm and closing braces) plus an extra closing code fence, resulting in doubled output in rustdoc.This will render as garbled documentation. Please consolidate into a single, well-formed doc block.
Suggested structure
-//! 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. -//! -//! # Example -//! -//! ```text -//! use platform_challenge_sdk::routes::*; -//! -//! impl Challenge for MyChallenge { +//! 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::server::{ServerChallenge, ChallengeContext}; +//! use platform_challenge_sdk::routes::*; +//! +//! impl ServerChallenge for MyChallenge { +//! // ... challenge_id, name, version, evaluate ... +//! //! fn routes(&self) -> Vec<ChallengeRoute> {And remove the duplicated closing block at lines 46–54.
🤖 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, The module-level doc comment is malformed: move the stray raw Rust lines (the use/import and impl ServerChallenge/ChallengeContext examples) inside the existing fenced code block, remove the dangling sentence fragment ("on the RPC server...") and delete the duplicated tail lines that repeat the `_ => RouteResponse::not_found()` arm and closing braces so the example is a single well-formed fenced block; ensure the example references the actual symbols used (ServerChallenge, ChallengeContext, ChallengeRoute, RouteResponse) and that only one closing ``` is present.
🧹 Nitpick comments (3)
crates/challenge-sdk/src/server.rs (2)
272-289:ChallengeContextshould deriveClone(and optionallyDebug) for ergonomic use in handlers.All fields are cheaply cloneable (
Arc,String,u64). DerivingClonewould allow handlers to move the context into spawned tasks, andDebugaids troubleshooting.Suggested change
-pub struct ChallengeContext { +#[derive(Clone, Debug)] +pub struct ChallengeContext {🤖 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 272 - 289, The ChallengeContext struct lacks derived traits; add at least #[derive(Clone)] (and optionally #[derive(Debug, Clone)]) to the ChallengeContext definition so instances can be cheaply cloned and moved into spawned tasks—this affects the struct named ChallengeContext and its fields db (Arc<ChallengeDatabase>), challenge_id (String), epoch (u64), and block_height (u64).
594-605:state.challenge.routes()is called on every fallback request — consider caching.Since
routes()is called for every unmatched request (including 404s for genuinely unknown paths), and the route list is unlikely to change at runtime, you could cache the result once at startup (it's already computed at line 452) and store it inServerState.🤖 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 594 - 605, Replace the per-request call to state.challenge.routes() with a cached routes collection on ServerState: add a field (e.g., routes_cache: Arc<Vec<Route>> or similar) to ServerState, initialize it at startup where the routes are originally computed, and then use that cached field (e.g., state.routes_cache) instead of calling state.challenge.routes() in the fallback handler; ensure the cached container is thread-safe (Arc/clone) and update the fallback loop to iterate over the cached routes to populate matched_params.crates/rpc-server/src/server.rs (1)
353-366: Batch JSON-RPC only processes the first request — consider adding a TODO or tracking issue.The comment at line 354 ("For batch, we'd return an array - for now just handle first") reflects a pre-existing limitation. Per the JSON-RPC 2.0 spec, batch requests should process all elements and return an array of responses. Now that processing is async, it would be natural to handle the full batch with concurrent execution.
Would you like me to open an issue to track full batch JSON-RPC support?
🤖 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 - 366, The batch branch currently only processes the first element (body.as_array() -> arr.first()) and returns a single response; change it to iterate over all items, spawn or collect async tasks (e.g., via FuturesUnordered or join_all) calling handle_single_request for each Value, await all concurrently, collect individual JsonRpcResponse results into a Vec<Value>, preserve request ordering (or document ordering behavior), filter out notifications (null responses) per JSON‑RPC spec, and return a Json array of responses instead of a single element (use JsonRpcResponse conversions and StatusCode handling as needed); if you can't fully implement now add a TODO referencing batch support and open a tracking issue.
🤖 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 fallback handler currently constructs a new temporary
sled DB on every request by calling ChallengeDatabase::open(...) inside the
creation of ChallengeContext, causing lack of persistence and resource leaks;
instead, create/open the ChallengeDatabase once at server startup (e.g., in
ChallengeServer::run or ChallengeServerBuilder::build), store it in shared state
(add a db: Arc<ChallengeDatabase> to ServerState or ChallengeServer), and change
custom_route_handler to clone/share that Arc when building the ChallengeContext
rather than calling ChallengeDatabase::open per request; remove the per-request
retry logic that re-creates a fresh DB and use the single shared DB instance for
all fallback requests.
In `@crates/rpc-server/src/jsonrpc.rs`:
- Around line 1140-1165: The JSON-RPC challenge_call must resolve a challenge
name to its UUID and run route matching to populate path params like the HTTP
path does; update the code that currently checks self.challenge_routes and
chain_state so that if challenge_id is a name you look up the actual UUID
(mirror the logic in server.rs::challenge_route_handler to produce resolved_id)
and use resolved_id when invoking the handler, and then perform the same route
lookup/matching against the ChallengeRoute(s) for that resolved_id to fill
RouteRequest.params (and any derived headers/query handling) instead of leaving
params empty so JSON-RPC handlers receive the same params as HTTP calls;
reference challenge_call, self.challenge_routes, self.chain_state, RouteRequest
and the matching logic from challenge_route_handler to implement this.
---
Outside diff comments:
In `@crates/challenge-sdk/src/routes.rs`:
- Around line 1-54: The module-level doc comment is malformed: move the stray
raw Rust lines (the use/import and impl ServerChallenge/ChallengeContext
examples) inside the existing fenced code block, remove the dangling sentence
fragment ("on the RPC server...") and delete the duplicated tail lines that
repeat the `_ => RouteResponse::not_found()` arm and closing braces so the
example is a single well-formed fenced block; ensure the example references the
actual symbols used (ServerChallenge, ChallengeContext, ChallengeRoute,
RouteResponse) and that only one closing ``` is present.
---
Nitpick comments:
In `@crates/challenge-sdk/src/server.rs`:
- Around line 272-289: The ChallengeContext struct lacks derived traits; add at
least #[derive(Clone)] (and optionally #[derive(Debug, Clone)]) to the
ChallengeContext definition so instances can be cheaply cloned and moved into
spawned tasks—this affects the struct named ChallengeContext and its fields db
(Arc<ChallengeDatabase>), challenge_id (String), epoch (u64), and block_height
(u64).
- Around line 594-605: Replace the per-request call to state.challenge.routes()
with a cached routes collection on ServerState: add a field (e.g., routes_cache:
Arc<Vec<Route>> or similar) to ServerState, initialize it at startup where the
routes are originally computed, and then use that cached field (e.g.,
state.routes_cache) instead of calling state.challenge.routes() in the fallback
handler; ensure the cached container is thread-safe (Arc/clone) and update the
fallback loop to iterate over the cached routes to populate matched_params.
In `@crates/rpc-server/src/server.rs`:
- Around line 353-366: The batch branch currently only processes the first
element (body.as_array() -> arr.first()) and returns a single response; change
it to iterate over all items, spawn or collect async tasks (e.g., via
FuturesUnordered or join_all) calling handle_single_request for each Value,
await all concurrently, collect individual JsonRpcResponse results into a
Vec<Value>, preserve request ordering (or document ordering behavior), filter
out notifications (null responses) per JSON‑RPC spec, and return a Json array of
responses instead of a single element (use JsonRpcResponse conversions and
StatusCode handling as needed); if you can't fully implement now add a TODO
referencing batch support and open a tracking issue.
Comment on lines
+1140
to
+1165
| // 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), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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 doesn't resolve challenge name → ID, nor populate path params — inconsistent with the HTTP handler.
Two issues in the challenge lookup and request construction:
-
Name-to-ID resolution missing: If the caller passes a challenge name instead of its UUID, lines 1145-1153 verify it exists but never resolve the actual ID. The
handleris then invoked with the raw name. Compare withchallenge_route_handlerinserver.rs(lines 240–258) which properly resolvesactual_idfrom the name. -
Route matching not performed:
paramsis always an emptyHashMap(line 1160). The HTTP path (challenge_route_handlerinserver.rs) performs route matching against declaredChallengeRoutes and populates path params (e.g.,:hash→"abc123"). Challenge handlers that rely onrequest.paramswill receive different inputs depending on whether the call came via HTTP or JSON-RPC.
Suggested fix sketch
- // 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),
- );
- }
- }
- }
-
- let request = RouteRequest {
- method,
- path,
- params: std::collections::HashMap::new(),
- query,
- headers: std::collections::HashMap::new(),
- body,
- auth_hotkey: None,
- };
+ // Resolve challenge name → ID and look up registered routes
+ let (resolved_id, challenge_routes) = {
+ let routes = self.challenge_routes.read();
+ if let Some(r) = routes.get(&challenge_id) {
+ (challenge_id.clone(), r.clone())
+ } else {
+ // Try to find by name
+ let chain = self.chain_state.read();
+ let actual_id = chain
+ .challenges
+ .values()
+ .find(|c| c.name == challenge_id)
+ .map(|c| c.id.to_string());
+ match actual_id.and_then(|id| routes.get(&id).cloned().map(|r| (id, r))) {
+ Some((id, r)) => (id, r),
+ None => {
+ return JsonRpcResponse::error(
+ id,
+ CHALLENGE_NOT_FOUND,
+ format!("Challenge '{}' not found or has no routes", challenge_id),
+ );
+ }
+ }
+ }
+ };
+
+ // Perform route matching to populate path params
+ let mut matched_params = std::collections::HashMap::new();
+ for route in &challenge_routes {
+ if let Some(params) = route.matches(&method, &path) {
+ matched_params = params;
+ break;
+ }
+ }
+
+ let request = RouteRequest {
+ method,
+ path,
+ params: matched_params,
+ query,
+ headers: std::collections::HashMap::new(),
+ body,
+ auth_hotkey: None,
+ };Then use resolved_id when invoking the handler.
🤖 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 - 1165, The JSON-RPC
challenge_call must resolve a challenge name to its UUID and run route matching
to populate path params like the HTTP path does; update the code that currently
checks self.challenge_routes and chain_state so that if challenge_id is a name
you look up the actual UUID (mirror the logic in
server.rs::challenge_route_handler to produce resolved_id) and use resolved_id
when invoking the handler, and then perform the same route lookup/matching
against the ChallengeRoute(s) for that resolved_id to fill RouteRequest.params
(and any derived headers/query handling) instead of leaving params empty so
JSON-RPC handlers receive the same params as HTTP calls; reference
challenge_call, self.challenge_routes, self.chain_state, RouteRequest and the
matching logic from challenge_route_handler to implement this.
…lidation - Remove #![allow(dead_code, unused_variables, unused_imports)] from challenge-sdk lib.rs - Replace .expect() and .unwrap_or_else() in custom_route_handler with proper error handling - Fix per-request temp database creation (DoS vector) using OnceLock for shared fallback DB - Replace .unwrap_or() on StatusCode::from_u16 with explicit match in both server.rs files - Add HTTP method validation in challenge_call (only GET/POST/PUT/DELETE/PATCH allowed) - Add path traversal protection in challenge_call (reject '..' segments, require '/' prefix) - Add input length limits: path <= 2048 chars, body <= 1MB, query params <= 100 - Fix unused import warnings exposed by removing blanket allow attribute
…server.rs - Add #[cfg(feature = "http-server")] gated imports for tracing macros (debug, error, info) used in run() and custom_route_handler - Add #[cfg(feature = "http-server")] gated import for std::sync::OnceLock used in custom_route_handler's static FALLBACK_DB - Remove unused local imports (Json, State, StatusCode) from run() method that shadow top-level imports and are not used locally These fixes resolve compilation errors when building with the http-server feature flag and eliminate unused import warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 opinionated challenge routes like
/leaderboardor/submit. Each challenge (including WASM modules) should define its own API surface.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 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 matches and dispatches to challenge-defined routescrates/challenge-sdk/src/lib.rsChallengeContextfromservermoduleChallengeRoute,RouteRequest,RouteResponseto the preludecrates/rpc-server/src/jsonrpc.rschallenge_callRPC method (was documented but missing from match arms)handle_async()method to support async route handler invocationchallenge_callto therpc_methodslistchallengeId,method,path,body,querycrates/rpc-server/src/server.rshandle_single_requestfrom sync to async to supporthandle_async()Breaking Changes
RoutesManifest::with_standard_routes()is removed. Challenges that relied on it should define their routes viaServerChallenge::routes()instead.Summary by CodeRabbit
Release Notes
New Features
routes()andhandle_route()methods.challenge_callfor invoking custom routes asynchronously.Breaking Changes
with_standard_routes()convenience builder from routes API.