feat(katana-rpc): rpc server builder #2788
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes across various files, primarily focusing on the handling of CORS (Cross-Origin Resource Sharing) configurations. Key modifications include updating the 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: 3
🧹 Outside diff range and nitpick comments (5)
crates/katana/rpc/rpc/src/lib.rs (1)
117-132: Ohayo sensei! Consider refactoring to reduce code duplication in thestart()method.The blocks of code for starting the server with and without metrics are similar. Extracting the common logic into a helper function can improve maintainability and readability.
Here's a suggested refactor:
pub async fn start(&self, addr: SocketAddr) -> Result<RpcServerHandle, Error> { let mut modules = self.module.clone(); let health_check_proxy = if self.health_check { modules.merge(HealthCheck)?; Some(HealthCheck::proxy()) } else { None }; let middleware = ServiceBuilder::new() .option_layer(self.cors.clone()) .option_layer(health_check_proxy) .timeout(Duration::from_secs(20)); let builder = ServerBuilder::new() .set_middleware(middleware) .set_host_filtering(AllowHosts::Any) .max_connections(self.max_connections); - let handle = if self.metrics { - let logger = RpcServerMetrics::new(&modules); - let server = builder.set_logger(logger).build(addr).await?; - - let addr = server.local_addr()?; - let handle = server.start(modules)?; - - RpcServerHandle { addr, handle } - } else { - let server = builder.build(addr).await?; - - let addr = server.local_addr()?; - let handle = server.start(modules)?; - - RpcServerHandle { addr, handle } - }; + let server = if self.metrics { + let logger = RpcServerMetrics::new(&modules); + builder.set_logger(logger).build(addr).await? + } else { + builder.build(addr).await? + }; + + let addr = server.local_addr()?; + let handle = server.start(modules)?; + + let handle = RpcServerHandle { addr, handle }; info!(target: "rpc", %addr, "RPC server started."); Ok(handle) }crates/katana/node/src/config/rpc.rs (1)
33-33: Consider documenting CORS configurationSensei, please consider adding documentation for the
cors_originsfield to explain:
- Expected format of CORS origins
- How to configure common scenarios
- Impact of empty vector vs. previous None behavior
crates/katana/rpc/rpc/Cargo.toml (1)
13-13: Ohayo! Clean dependency additions for RPC enhancementThe new dependencies are well-chosen for building a robust RPC server:
http: Core HTTP typestower: Middleware frameworktower-http: HTTP-specific middleware including CORSserde_json: JSON serializationConsider documenting the middleware stack architecture in the README or module documentation, showing how these components work together in the RPC server builder.
Also applies to: 25-25, 29-30
crates/katana/cli/src/utils.rs (1)
195-220: Ohayo, sensei! The CORS serialization implementation looks solid!The implementation is well-structured with proper error handling. A small suggestion to make the code more robust:
Consider adding debug context to the error messages to make troubleshooting easier:
pub fn serialize_cors_origins<S>(values: &[HeaderValue], serializer: S) -> Result<S::Ok, S::Error> where S: Serializer, { let string = values .iter() .map(|v| v.to_str()) .collect::<Result<Vec<_>, _>>() - .map_err(serde::ser::Error::custom)? + .map_err(|e| serde::ser::Error::custom(format!("Invalid header value: {}", e)))? .join(","); serializer.serialize_str(&string) }crates/katana/cli/src/args.rs (1)
620-637: Ohayo! The CORS origins test looks solid, sensei!The test effectively validates:
- Multiple origin parsing
- Support for wildcard (
*)- Both HTTP and HTTPS origins
- Proper
HeaderValueconversionHowever, consider adding a test case for invalid CORS origins to ensure proper error handling.
Add this test case:
#[test] #[cfg(feature = "server")] fn parse_cors_origins() { let config = NodeArgs::parse_from([ "katana", "--http.cors_origins", "*,http://localhost:3000,https://example.com", ]) .config() .unwrap(); let cors_origins = config.rpc.cors_origins; assert_eq!(cors_origins.len(), 3); assert!(cors_origins.contains(&HeaderValue::from_static("*"))); assert!(cors_origins.contains(&HeaderValue::from_static("http://localhost:3000"))); assert!(cors_origins.contains(&HeaderValue::from_static("https://example.com"))); + + // Test invalid CORS origin + let result = NodeArgs::parse_from([ + "katana", + "--http.cors_origins", + "not-a-valid-origin", + ]) + .config(); + + assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(2 hunks)crates/katana/cli/src/options.rs(3 hunks)crates/katana/cli/src/utils.rs(2 hunks)crates/katana/node/src/config/rpc.rs(3 hunks)crates/katana/node/src/lib.rs(5 hunks)crates/katana/rpc/rpc/Cargo.toml(2 hunks)crates/katana/rpc/rpc/src/cors.rs(1 hunks)crates/katana/rpc/rpc/src/health.rs(1 hunks)crates/katana/rpc/rpc/src/lib.rs(1 hunks)
🔇 Additional comments (7)
crates/katana/rpc/rpc/src/cors.rs (1)
1-138: Ohayo sensei! Excellent implementation of the CORS middleware.
The code is well-structured and provides flexible configuration options for CORS policies. This will greatly enhance the security and usability of the RPC server.
crates/katana/cli/Cargo.toml (1)
13-13: Ohayo! LGTM - Clean workspace dependency addition
The addition of katana-rpc as a workspace dependency aligns perfectly with the RPC server builder feature, sensei!
crates/katana/node/src/config/rpc.rs (2)
4-4: Ohayo! New import looks good
Clean import of HeaderValue for enhanced CORS handling.
33-33: Verify breaking changes in CORS configuration
The change from Option<Vec<String>> to Vec<HeaderValue> for cors_origins is a breaking change that might affect existing configurations and code.
Also applies to: 46-46
crates/dojo/test-utils/src/sequencer.rs (1)
121-121: LGTM! The CORS origins initialization is correct.
The change aligns well with the new CORS handling approach across the codebase.
crates/katana/cli/src/options.rs (1)
92-97: The CORS configuration changes look great!
The switch to Vec<HeaderValue> with proper serialization/deserialization handling improves type safety and robustness. The default implementation with an empty vector is also appropriate.
Also applies to: 119-119
crates/katana/cli/src/args.rs (1)
400-400: Ohayo! LGTM on the HeaderValue import, sensei!
The addition of HeaderValue from katana_rpc::cors enhances type safety for CORS origin handling.
|
|
||
| let builder = ServerBuilder::new() | ||
| .set_middleware(middleware) | ||
| .set_host_filtering(AllowHosts::Any) |
There was a problem hiding this comment.
Ohayo sensei! Please verify the use of AllowHosts::Any in host filtering.
Using AllowHosts::Any allows requests from any host, which may pose security risks in production environments. Consider restricting allowed hosts or making this configurable to enhance security.
| .allow_methods([Method::POST, Method::GET]) | ||
| .allow_headers([hyper::header::CONTENT_TYPE, "argent-client".parse().unwrap(), "argent-version".parse().unwrap()]); |
There was a problem hiding this comment.
Ohayo sensei! Avoid using .unwrap() when parsing header values.
Using .unwrap() may cause the application to panic if the parsing fails. Consider handling the potential error or using .expect() with a meaningful message.
Apply this diff to handle the parsing result:
let cors = Cors::new()
.allow_origins(config.cors_origins.clone())
// Allow `POST` and `GET` methods when accessing the resource
.allow_methods([Method::POST, Method::GET])
.allow_headers([
hyper::header::CONTENT_TYPE,
- "argent-client".parse().unwrap(),
- "argent-version".parse().unwrap(),
+ HeaderValue::from_static("argent-client"),
+ HeaderValue::from_static("argent-version"),
]);📝 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.
| .allow_methods([Method::POST, Method::GET]) | |
| .allow_headers([hyper::header::CONTENT_TYPE, "argent-client".parse().unwrap(), "argent-version".parse().unwrap()]); | |
| .allow_methods([Method::POST, Method::GET]) | |
| .allow_headers([hyper::header::CONTENT_TYPE, HeaderValue::from_static("argent-client"), HeaderValue::from_static("argent-version")]); |
| impl From<HealthCheck> for Methods { | ||
| fn from(_: HealthCheck) -> Self { | ||
| let mut module = RpcModule::new(()); | ||
| module.register_method(HealthCheck::METHOD, |_, _| Ok(json!({ "health": true }))).unwrap(); |
There was a problem hiding this comment.
Ohayo sensei! Handle potential errors when registering methods.
Using .unwrap() may cause the server to panic if the method registration fails. Consider handling the error gracefully or propagating it appropriately.
Apply this diff to handle the error:
module.register_method(HealthCheck::METHOD, |_, _| Ok(json!({ "health": true })))?;📝 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.
| module.register_method(HealthCheck::METHOD, |_, _| Ok(json!({ "health": true }))).unwrap(); | |
| module.register_method(HealthCheck::METHOD, |_, _| Ok(json!({ "health": true })))?; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/katana/rpc/rpc/src/cors.rs (1)
13-59: Consider adding validation and more configuration options, sensei!While the implementation is solid, consider these enhancements:
- Add validation for malformed origins in the builder methods
- Consider adding methods for other CORS headers like
max-ageandexpose-headers- Add a method to configure
allow_credentialsExample enhancement:
pub fn allow_credentials(self, allow: bool) -> Self { Self(self.0.allow_credentials(allow)) } pub fn max_age(self, max_age: Duration) -> Self { Self(self.0.max_age(max_age)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/rpc/rpc/src/cors.rs(1 hunks)
🔇 Additional comments (2)
crates/katana/rpc/rpc/src/cors.rs (2)
1-11: Ohayo sensei! Clean and well-structured imports and struct definition.
The imports are appropriate, and the documentation follows Rust conventions. The wrapper pattern for Cors struct is a good choice for encapsulating the CorsLayer functionality.
61-138: Consider additional origin validation and security measures.
The wildcard handling is clever, but consider these security enhancements:
- Add validation for malformed origin values
- Consider logging when converting to wildcard mode
- Add documentation about security implications of using wildcards
Also, verify that the origin parsing aligns with the RFC 6454 specification.
✅ Verification successful
Let me analyze the actual implementation and validation patterns before making security recommendations.
Let me check how CORS is actually implemented and configured before making any security recommendations, sensei!
The CORS implementation is secure and well-designed
Ohayo sensei! After reviewing the implementation, I can confirm that the CORS handling is actually quite robust and secure:
- The code properly delegates to the battle-tested
tower-httpCORS implementation - The wildcard handling in
AllowOrigins::listis intentional and follows best practices by converting any wildcard presence intoAllowOrigin::any() - The implementation provides multiple secure configuration options:
exact()for strict single-origin policiesmirror_request()for dynamic origin validationlist()with proper wildcard detection- Clear documentation about security implications of permissive modes
The original review concerns about additional validation were unnecessary as the implementation correctly relies on the underlying tower-http crate's validation mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for origin validation patterns in the codebase
rg -A 3 'origin.*validate|validate.*origin'
# Look for any security-related comments about CORS
rg -A 3 'SECURITY:.*CORS|TODO:.*CORS|FIXME:.*CORS'
Length of output: 98
Script:
#!/bin/bash
# Let's check how HeaderValue is used and validated in the codebase
ast-grep --pattern 'HeaderValue::from_str($$$)'
# Check for any CORS-related tests or validation
rg -l 'cors|CORS' --type rust
# Look for any origin-related code in test files
rg 'origin' --type rust -g '*test*.rs' -A 3
# Check if there's any validation in the tower-http cors module being used
ast-grep --pattern 'impl $_ for AllowOrigin'
Length of output: 729
Script:
#!/bin/bash
# Let's examine the tower-http cors module usage
rg -A 5 "use tower_http::cors" --type rust
# Check how CORS is configured in the actual server setup
rg -A 10 "cors::" --type rust
# Look for any CORS-related tests
rg -A 5 "test.*cors|cors.*test" --type rust -i
Length of output: 15047
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn wildcard_in_list() { | ||
| let origins = vec![ | ||
| HeaderValue::from_static("http://example.com"), | ||
| HeaderValue::from_static("*"), | ||
| HeaderValue::from_static("http://other.com"), | ||
| ]; | ||
|
|
||
| let _ = AllowOrigins::list(origins); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Let's enhance the test coverage, sensei!
The current test suite only covers the basic wildcard scenario. Consider adding tests for:
- Invalid origin formats
- Multiple wildcards in the list
- Empty origin list
- Various From implementations
- Builder pattern methods
Example test additions:
#[test]
fn test_invalid_origin_format() {
let origins = vec![
HeaderValue::from_static("invalid-origin"),
HeaderValue::from_static("http://valid.com"),
];
let cors = AllowOrigins::list(origins);
// Assert expected behavior
}
#[test]
fn test_builder_pattern() {
let cors = Cors::new()
.allow_origins(vec![HeaderValue::from_static("http://example.com")])
.allow_methods(vec![http::Method::GET]);
// Assert configuration
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/katana/rpc/rpc/src/cors.rs (1)
61-102: Consider adding origin validation, sensei!While the wildcard handling is solid, consider adding validation for origin format in the
listmethod. Invalid origin formats could cause issues at runtime.pub fn list<I>(origins: I) -> Self where I: IntoIterator<Item = HeaderValue>, { let origins = origins.into_iter().collect::<Vec<_>>(); + // Validate origin format + for origin in &origins { + if let Ok(s) = origin.to_str() { + if origin != &WILDCARD && !s.starts_with("http://") && !s.starts_with("https://") { + // Use proper error handling in production + debug_assert!(false, "Invalid origin format: {}", s); + } + } + } if origins.iter().any(|o| o == WILDCARD) { Self(cors::AllowOrigin::any()) } else { Self(cors::AllowOrigin::list(origins)) } }crates/katana/cli/src/utils.rs (1)
195-220: Consider validating origins during deserialization, sensei!While the implementation handles empty strings well, consider validating the origin format during deserialization to catch invalid origins early.
pub fn deserialize_cors_origins<'de, D>(deserializer: D) -> Result<Vec<HeaderValue>, D::Error> where D: Deserializer<'de>, { - String::deserialize(deserializer)? + let origins = String::deserialize(deserializer)? .split(',') .map(|s| s.trim()) - .filter(|s| !s.is_empty()) + .filter(|s| { + if s.is_empty() { + return false; + } + // Allow wildcard or validate URL format + s == "*" || s.starts_with("http://") || s.starts_with("https://") + }) .map(HeaderValue::from_str) .collect::<Result<Vec<HeaderValue>, _>>() - .map_err(serde::de::Error::custom) + .map_err(serde::de::Error::custom)?; + + if origins.is_empty() { + Ok(vec![]) + } else { + Ok(origins) + } }crates/katana/rpc/rpc/src/lib.rs (2)
27-34: Ohayo sensei! Consider expanding error handling for specific scenarios.The error enum could benefit from additional variants to handle specific failure cases such as:
- Connection/binding errors
- Configuration validation errors
- Middleware initialization errors
pub enum Error { #[error(transparent)] Jsonrpsee(#[from] jsonrpsee::core::Error), #[error("RPC server has already been stopped")] AlreadyStopped, + + #[error("Failed to bind to address: {0}")] + BindError(std::io::Error), + + #[error("Invalid configuration: {0}")] + ConfigError(String), + + #[error("Middleware initialization failed: {0}")] + MiddlewareError(String), }
36-51: Ohayo sensei! Consider adding utility methods to RpcServerHandle.The handle implementation could be enhanced with methods for:
- Checking if the server is running
- Getting current connection count
- Graceful shutdown with timeout
impl RpcServerHandle { + /// Returns true if the server is currently running + pub fn is_running(&self) -> bool { + !self.handle.is_stopped() + } + + /// Attempts to stop the server with a timeout + pub async fn stop_with_timeout(&self, timeout: Duration) -> Result<(), Error> { + tokio::time::timeout(timeout, self.stopped()) + .await + .map_err(|_| Error::AlreadyStopped)?; + Ok(()) + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(2 hunks)crates/katana/cli/src/options.rs(3 hunks)crates/katana/cli/src/utils.rs(2 hunks)crates/katana/node/src/config/rpc.rs(3 hunks)crates/katana/node/src/lib.rs(5 hunks)crates/katana/rpc/rpc/Cargo.toml(2 hunks)crates/katana/rpc/rpc/src/cors.rs(1 hunks)crates/katana/rpc/rpc/src/health.rs(1 hunks)crates/katana/rpc/rpc/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/katana/cli/Cargo.toml
- crates/dojo/test-utils/src/sequencer.rs
- crates/katana/cli/src/args.rs
- crates/katana/cli/src/options.rs
- crates/katana/node/src/config/rpc.rs
- crates/katana/rpc/rpc/Cargo.toml
- crates/katana/rpc/rpc/src/health.rs
🔇 Additional comments (5)
crates/katana/rpc/rpc/src/cors.rs (3)
1-61: Ohayo! The CORS implementation looks solid, sensei!
The implementation follows the builder pattern nicely, with clear documentation and type-safe methods. The Layer trait implementation enables seamless integration as middleware.
104-138: Clean and comprehensive type conversions, sensei!
The From implementations provide a complete set of type conversions, making the API flexible and ergonomic to use.
140-154: Enhance test coverage for robust validation, sensei!
The current test suite only covers the basic wildcard scenario. Consider adding more test cases.
crates/katana/rpc/rpc/src/lib.rs (1)
112-115:
Ohayo sensei! Security concern with host filtering configuration.
Using AllowHosts::Any allows requests from any host, which could expose the RPC server to unauthorized access.
Consider:
- Using
AllowHosts::Localhostfor development - Making host filtering configurable
- Implementing a whitelist of allowed hosts
- .set_host_filtering(AllowHosts::Any)
+ .set_host_filtering(config.allowed_hosts.unwrap_or(AllowHosts::Localhost))Likely invalid or redundant comment.
crates/katana/node/src/lib.rs (1)
265-269:
Ohayo sensei! Unsafe CORS header parsing.
The current implementation uses unwrap() on header parsing which could cause panics.
Apply this diff to handle the parsing safely:
let cors = Cors::new()
.allow_origins(config.cors_origins.clone())
.allow_methods([Method::POST, Method::GET])
.allow_headers([
hyper::header::CONTENT_TYPE,
- "argent-client".parse().unwrap(),
- "argent-version".parse().unwrap(),
+ HeaderValue::from_static("argent-client"),
+ HeaderValue::from_static("argent-version"),
]);| modules.merge(StarknetApiServer::into_rpc(api.clone()))?; | ||
| modules.merge(StarknetWriteApiServer::into_rpc(api.clone()))?; | ||
| modules.merge(StarknetTraceApiServer::into_rpc(api))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Enhance error handling for module merging.
The current implementation might not provide clear error context when module merging fails.
Consider wrapping the merge operations with additional context:
- modules.merge(StarknetApiServer::into_rpc(api.clone()))?;
- modules.merge(StarknetWriteApiServer::into_rpc(api.clone()))?;
- modules.merge(StarknetTraceApiServer::into_rpc(api))?;
+ modules
+ .merge(StarknetApiServer::into_rpc(api.clone()))
+ .map_err(|e| anyhow::anyhow!("Failed to merge Starknet API: {}", e))?;
+ modules
+ .merge(StarknetWriteApiServer::into_rpc(api.clone()))
+ .map_err(|e| anyhow::anyhow!("Failed to merge Starknet Write API: {}", e))?;
+ modules
+ .merge(StarknetTraceApiServer::into_rpc(api))
+ .map_err(|e| anyhow::anyhow!("Failed to merge Starknet Trace API: {}", e))?;📝 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.
| modules.merge(StarknetApiServer::into_rpc(api.clone()))?; | |
| modules.merge(StarknetWriteApiServer::into_rpc(api.clone()))?; | |
| modules.merge(StarknetTraceApiServer::into_rpc(api))?; | |
| modules | |
| .merge(StarknetApiServer::into_rpc(api.clone())) | |
| .map_err(|e| anyhow::anyhow!("Failed to merge Starknet API: {}", e))?; | |
| modules | |
| .merge(StarknetWriteApiServer::into_rpc(api.clone())) | |
| .map_err(|e| anyhow::anyhow!("Failed to merge Starknet Write API: {}", e))?; | |
| modules | |
| .merge(StarknetTraceApiServer::into_rpc(api)) | |
| .map_err(|e| anyhow::anyhow!("Failed to merge Starknet Trace API: {}", e))?; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2788 +/- ##
==========================================
+ Coverage 55.58% 55.61% +0.02%
==========================================
Files 436 439 +3
Lines 55521 55678 +157
==========================================
+ Hits 30862 30965 +103
- Misses 24659 24713 +54 ☔ View full report in Codecov by Sentry. |
basically encapsulating most of the rpc building logic into a struct, and implement a
Corsstruct to manage how we handle wildcard (ie*) in the allowed origins list. see the comment in:dojo/crates/katana/rpc/rpc/src/cors.rs
Lines 62 to 70 in c000ff4
this pr also move where we handle the parsing of the origins to the cli phase.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores