feat(katana): extract RpcOptions from ServerOptions #3084
Conversation
notV4l
changed the title
notV4l
changed the title
|
Ohayo sensei! Below is the detailed breakdown of the pull request: WalkthroughThis PR involves the removal of the conditional compilation attribute Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/cli/src/args.rs (1)
253-253: Consider moving CORS origins to RpcOptions as well.While you've migrated most RPC-related configurations, CORS origins are still accessed from
self.server. For complete separation of concerns, consider moving this field to the RpcOptions struct in a future update.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/katana/cli/src/args.rs(5 hunks)
🔇 Additional comments (6)
crates/katana/cli/src/args.rs (6)
106-108: Ohayo! The separation of RPC options looks good.Adding a dedicated RpcOptions struct helps maintain a clear separation of concerns between server configuration and RPC-specific functionality, making the codebase more modular and maintainable.
230-230: Nice migration from server to rpc options, sensei!The code now correctly uses
self.rpc.http_modulesinstead ofself.server.http_modules, maintaining functionality while improving the architecture.
278-284: Great migration of RPC parameters, sensei!You've correctly updated all the relevant parameters to reference the new
rpcfield instead ofserver, which aligns with the separation of concerns.
433-433: Properly handling config file merging - ohayo!Added the necessary merge call for the new RPC options when loading from a config file.
716-723: Test documentation updated correctly!The test comments have been updated to reflect the new command structure with
--rpc.apiinstead of the previous naming.
731-731: Accurate test case update!The error test case has been properly migrated to use the new
--rpc.apiparameter naming.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3084 +/- ##
=======================================
Coverage 57.22% 57.22%
=======================================
Files 446 446
Lines 60547 60564 +17
=======================================
+ Hits 34648 34658 +10
- Misses 25899 25906 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// API's offered over the HTTP-RPC interface. | ||
| #[arg(long = "http.api", value_name = "MODULES")] | ||
| #[arg(long = "rpc.api", value_name = "MODULES", alias = "http.api")] |
There was a problem hiding this comment.
I added an alias here to make sure it's backward compatible. We may remove it in the future.
|
Fixing the Torii test issue. |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (17)
crates/torii/mcp/src/resources/mod.rs (1)
6-8: Consider adding documentation for future resources, sensei!The
get_resources()function currently returns an empty vector with a comment indicating resources can be added later. Consider adding a doc comment to explain how resources should be structured when added in the future.+/// Returns a list of available resources. +/// +/// This function will be populated with resources as they are implemented. pub fn get_resources() -> Vec<Resource> { vec![] // Add resources as needed }crates/torii/mcp/src/tools/mod.rs (1)
13-15: Simple tool aggregation - nice approach!The current implementation works well for the current set of tools. For future extensibility, you might consider implementing a registry pattern if the number of tools grows significantly.
-pub fn get_tools() -> Vec<Tool> { - vec![query::get_tool(), schema::get_tool()] -} +pub fn get_tools() -> Vec<Tool> { + // This approach scales better when adding many tools + let mut tools = Vec::new(); + tools.push(query::get_tool()); + tools.push(schema::get_tool()); + // Easy to add more tools here + tools +}crates/torii/mcp/src/tools/schema.rs (2)
33-51: Refactor duplicate SQL queriesOhayo! I notice the schema queries in the match arms are almost identical, with only the WHERE clause for table filtering being different. Consider refactoring to reduce duplication:
- let schema_query = match table_filter { - Some(_table) => "SELECT - m.name as table_name, - p.* - FROM sqlite_master m - JOIN pragma_table_info(m.name) p - WHERE m.type = 'table' - AND m.name = ? - ORDER BY m.name, p.cid" - .to_string(), - _ => "SELECT - m.name as table_name, - p.* - FROM sqlite_master m - JOIN pragma_table_info(m.name) p - WHERE m.type = 'table' - ORDER BY m.name, p.cid" - .to_string(), -}; + let mut schema_query = "SELECT + m.name as table_name, + p.* + FROM sqlite_master m + JOIN pragma_table_info(m.name) p + WHERE m.type = 'table'".to_string(); + + if table_filter.is_some() { + schema_query.push_str("\n AND m.name = ?"); + } + + schema_query.push_str("\n ORDER BY m.name, p.cid");This makes the code more maintainable and reduces duplication.
53-56: Consider table name validationSensei, while using parameterized queries is good for preventing SQL injection, it would be even better to validate the table name parameter. SQLite table names follow specific rules, and validating the input would provide an additional layer of security.
Consider adding a validation check for the table name before executing the query.
crates/torii/sqlite/src/utils.rs (1)
189-228: Well-implemented row to JSON conversion with improvement opportunitiesNice implementation of the SQLite row to JSON conversion! The function handles different data types appropriately and includes fallback mechanisms.
Consider these improvements:
- Error handling: Replace
unwrap()with proper error handling to prevent potential panics:- let table_name: String = row.try_get("table_name").unwrap(); + let table_name: String = row.try_get("table_name") + .map_err(|e| format!("Failed to get table_name: {}", e))?;
- Type fallback optimization: The fallback type handling could be simplified using a helper function:
fn get_value_as_json<T: serde::Serialize>(row: &sqlx::sqlite::SqliteRow, i: usize) -> serde_json::Value where T: for<'a> sqlx::decode::Decode<'a, sqlx::sqlite::Sqlite> + sqlx::types::Type<sqlx::sqlite::Sqlite> { match row.try_get::<T, _>(i) { Ok(val) => serde_json::to_value(val).unwrap_or(serde_json::Value::Null), Err(_) => serde_json::Value::Null, } }This would make the fallback section cleaner and more maintainable.
crates/torii/sqlite/src/lib.rs (5)
51-51: Ohayo sensei! Include a note on mutability constraints.Storing
configwithinSqlis convenient but be mindful when performing updates at runtime, as concurrency handling might be required if the configuration changes.
759-769: Ohayo sensei! Single source of truth foris_key.When invoking
add_columns_recursivewithfalse, ensure that keys are consistently handled later, to avoid hidden indexing issues.
805-815: Ohayo sensei! This function is large and intricate.A single method with multiple nested match arms can grow complex. Consider splitting out smaller helper functions for readability.
901-934: Ohayo sensei! Decomposition for tuple elements.Index-based naming can sometimes confuse debugging. Document the generated columns or add logs for clarity.
935-1009: Ohayo sensei! Pay attention toTy::Enumconstraints.Your custom check constraints are helpful, but dynamic expansions of permissible enum variants might break older data rows if not carefully tested.
crates/torii/libp2p/src/server/mod.rs (2)
86-96: Ohayo sensei! Thepeersparameter helps with auto-dialing.Dialing known peers on initialization fosters immediate connectivity. Just consider a fallback if a peer is offline.
365-372: Ohayo sensei! Logging invalid signatures as a warning.A crisp separation of message authenticity from normal flow. Just ensure repeated invalid submissions aren’t spammy.
crates/torii/server/src/handlers/mcp.rs (3)
59-70: Ohayo sensei! Enhancedhandle_initializeincludes resource subscription capabilities.Advertising features in capabilities ensures better client discovery. Keep the version info updated.
136-155: Ohayo sensei! Solid SSE session creation.Generating a unique session ID with
Uuidis a good pattern. Just be mindful of session cleanup if a client disconnects.
308-341: Ohayo sensei! Unified SSE and regular requests inhandlemethod.Graceful fallback to SSE or normal route. This is neat. Confirm timeouts or keep-alive pings if sessions remain idle.
crates/torii/mcp/src/types/mod.rs (2)
25-30: Ohayo sensei, consider consistent naming for notification fields.The fields
_jsonrpc,_method, and_paramswith leading underscores may cause confusion about intended usage. If they’re truly needed, consider removing the underscore or adding a brief comment clarifying their purpose.
73-78: Ohayo sensei, consider clarifying the session identifier usage.
_session_idindicates it might be unused or only for logging/debugging. If necessary, rename it or add a brief doc comment clarifying its purpose.
🛑 Comments failed to post (3)
crates/torii/runner/src/lib.rs (1)
153-158: 🛠️ Refactor suggestion
Ohayo sensei! Consider enforcing exclusive usage.
Whenall_model_indicesistrue, specifying specificmodel_indicescan cause confusion. Either override them automatically or prompt the user.if self.args.sql.all_model_indices && self.args.sql.model_indices.is_some() { warn!( target: LOG_TARGET, "all_model_indices is true, which will override any specific indices in model_indices" ); + // Possibly clear self.args.sql.model_indices to avoid confusion + // self.args.sql.model_indices = None; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if self.args.sql.all_model_indices && self.args.sql.model_indices.is_some() { warn!( target: LOG_TARGET, "all_model_indices is true, which will override any specific indices in model_indices" ); // Possibly clear self.args.sql.model_indices to avoid confusion // self.args.sql.model_indices = None; }crates/torii/libp2p/src/server/mod.rs (1)
194-198: 🛠️ Refactor suggestion
Ohayo sensei!
swarm.dial(peer.parse()?);usage.Safe unwrapping can crash if parse fails. Consider more explicit error checks to avoid runtime panic.
- swarm.dial(peer.parse::<Multiaddr>().unwrap())?; + if let Ok(addr) = peer.parse::<Multiaddr>() { + swarm.dial(addr)?; + } else { + warn!("Failed to parse peer multiaddr: {}", peer); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// We dial all of our peers. Our server will then broadcast // all incoming offchain messages to all of our peers. for peer in peers { if let Ok(addr) = peer.parse::<Multiaddr>() { swarm.dial(addr)?; } else { warn!("Failed to parse peer multiaddr: {}", peer); } }crates/torii/server/src/handlers/mcp.rs (1)
287-299: 🛠️ Refactor suggestion
Ohayo sensei! Reading resources stub.
Currently defaults to
method_not_found. Ensure future expansions handle reading logic or partial errors gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/cli/src/args.rs (1)
230-230: Ohayo sensei! Potential duplication in module assignment.Line 230 redefines a local
modulesvariable inrpc_config. Ensure there's no unintentional shadowing or confusion with the existing code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (11)
crates/katana/cli/src/args.rs(4 hunks)crates/katana/cli/src/file.rs(2 hunks)crates/katana/cli/src/options.rs(3 hunks)crates/katana/cli/src/args.rs(1 hunks)crates/katana/cli/src/args.rs(0 hunks)crates/katana/cli/src/options.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs(6 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(1 hunks)crates/katana/cli/src/args.rs(3 hunks)crates/katana/cli/src/file.rs(2 hunks)crates/katana/cli/src/options.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/katana/cli/src/file.rs
- crates/katana/cli/src/options.rs
- crates/katana/cli/src/args.rs
- crates/katana/cli/src/args.rs
- crates/katana/cli/src/args.rs
- crates/katana/cli/src/file.rs
- crates/katana/cli/src/options.rs
🔇 Additional comments (16)
crates/katana/cli/src/args.rs (2)
106-109: Ohayo sensei! Newrpcfield introduced under feature flag.This addition looks correct for separating RPC-specific configurations. Verify that usage outside of
serverfeature is handled or guarded properly.Do you need any additional checks for scenarios when
#[cfg(feature = "server")]is not enabled?
278-285: Ohayo sensei! Confirm defaults are handled properly.You're assigning these RPC-related configs as
Some(...). If users don't provide values, these fields might remain at their defaults. Double-check that the rest of the code properly handlesNonevs.Some(0 or default).crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
51-60: Ohayo sensei! Mind potential deep nesting.
token_has_inner_compositechecks for one level of array/tuple. For arrays of arrays, ensureto_composite()recursion covers deeper nesting if needed.
68-73: Ohayo sensei! Straightforward enum detection.
token_is_custom_enumlogic is concise and correct. Nicely done.crates/katana/cli/src/options.rs (5)
102-114: Ohayo sensei! Removal ofServerOptionsdefault implementation.Confirm if removing or altering the
Defaulttrait won’t break external code that may rely on it.
115-130: Ohayo sensei! Verify removal ofmergeinServerOptions.Double-check existing callers of
mergeto ensure no breakage occurs.
132-141: Ohayo sensei! NewRpcOptionsstructure.This new struct effectively separates RPC concerns. The approach looks good.
174-176: Ohayo sensei! Double-check uninitialized fields inDefault.
max_request_body_sizeandmax_response_body_sizemight need defaults for consistency.
189-209: Ohayo sensei! Partialmergecould lead to leftover defaults.If the caller sets certain fields but not others, confirm that the final result is intentional. Great approach for a flexible configuration, though.
crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs (7)
2-2: Ohayo sensei, new dependency usage recognized.
The newly introducedconvert_casecrate enables standardized naming conventions across your generated code. Good stuff!
5-5: Ohayo sensei, imports look on point.
Bringing intoken_is_custom_enumandtoken_is_enumaligns nicely with the upcoming logic updates.
26-45: Ohayo sensei, yourgenerate_simple_enummethod is tidy.
Its straightforward design for enumerating variants into a TypeScript array and type meets the goal neatly.
47-78: Ohayo sensei, thegenerate_custom_enummethod is equally robust.
Handling each variant’s typed data paves the way for more flexible enum structures in TypeScript.
79-90: Ohayo sensei, the logic ingenerateis smooth sailing.
Neatly checks for enum-type composites and routes to the correct generation function, preventing duplication.
176-190: Ohayo sensei,test_custom_enumis comprehensive.
The test effectively ensures correctness of the generated definitions, confirming your new function’s behavior.
220-254: Ohayo sensei,create_custom_enum_tokenis a solid test fixture.
This function nicely demonstrates nested enum tokens and ensures your generator can handle deeper structures.
Summary by CodeRabbit
http.apias an alternative torpc.apifor specifying modules.