refactor(libp2p): typed data crate + client server feature set #2877
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
🪧 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: 1
🧹 Nitpick comments (2)
crates/torii/libp2p/src/typed_data.rs (2)
447-455: Ohayo sensei! Consider enhancing the macro's input validation.The macro handles basic parsing well, but could be more robust with additional validation:
macro_rules! from_str { ($string:expr, $type:ty) => { + if $string.is_empty() { + return Err(Error::InvalidMessageError("Empty string provided".to_string())); + } if $string.starts_with("0x") || $string.starts_with("0X") { + let hex_str = &$string[2..]; + if hex_str.len() % 2 != 0 { + return Err(Error::InvalidMessageError("Invalid hex string length".to_string())); + } + if !hex_str.chars().all(|c| c.is_ascii_hexdigit()) { + return Err(Error::InvalidMessageError("Invalid hex characters".to_string())); + } <$type>::from_str_radix(&$string[2..], 16) } else { + if !$string.chars().all(|c| c.is_ascii_digit() || c == '-') { + return Err(Error::InvalidMessageError("Invalid decimal characters".to_string())); + } <$type>::from_str($string) - }.map_err(|e| Error::InvalidMessageError(format!("Failed to parse number: {}", e))) + }.map_err(|e| Error::InvalidMessageError(format!("Failed to parse {} value '{}': {}", + stringify!($type), $string, e))) }; }
594-624: Reduce repetition with a helper macro, sensei!The current implementation repeats the same pattern for each numeric type. Consider introducing a helper macro to reduce this repetition:
macro_rules! impl_numeric_parsing { ($($ty:ident),*) => { $( Primitive::$ty(v) => { *v = Some(from_str!(string, $ty)?); } )* } } // Usage in match statement: match primitive { impl_numeric_parsing!(i8, i16, i32, i64, i128, u8, u16, u32, u64, u128), // ... rest of the match arms }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/libp2p/src/typed_data.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
| macro_rules! from_str { | ||
| ($string:expr, $type:ty) => { | ||
| if $string.starts_with("0x") || $string.starts_with("0X") { | ||
| <$type>::from_str_radix(&$string[2..], 16) | ||
| } else { | ||
| <$type>::from_str($string) | ||
| }.map_err(|e| Error::InvalidMessageError(format!("Failed to parse number: {}", e))) | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Additional test coverage for hex string parsing is indeed needed.
The codebase has tests using hex values, but they're mainly for address handling. Your numeric type parsing deserves its own test cases, especially for the hex string functionality in the from_str! macro. The original suggestion for adding a dedicated test case remains valid.
- The suggested test in
typed_data.rswould improve coverage of an important parsing feature - Consider adding similar tests for other numeric types (i64, u32, etc.) to ensure consistent behavior
🔗 Analysis chain
Ensure consistent hex string handling across all numeric types.
The current implementation handles hex strings uniformly through the from_str! macro, which is good! However, consider adding a test case to verify this behavior:
#[test]
fn test_hex_string_parsing() {
let mut ty = Ty::Primitive(Primitive::U64(None));
parse_value_to_ty(&PrimitiveType::String("0xFF".to_string()), &mut ty).unwrap();
assert_eq!(ty.as_primitive().unwrap().as_u64().unwrap(), 255);
}Let's verify the test coverage:
Also applies to: 594-624
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if we have tests for hex string parsing
rg -l 'test.*hex.*parse|parse.*hex.*test' .
Length of output: 45
Script:
#!/bin/bash
# Search for test functions in typed_data.rs
rg -A 2 "#\[test\]" crates/torii/libp2p/src/typed_data.rs
# Search for any hex string literals in test code
rg -l "0x[0-9a-fA-F]+" crates/torii/libp2p/src/typed_data.rs
# Search for parse_value_to_ty usage in tests
ast-grep --pattern 'fn $name() { $$$ parse_value_to_ty $$$ }'
Length of output: 742
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2877 +/- ##
=======================================
Coverage 55.77% 55.78%
=======================================
Files 446 448 +2
Lines 57794 57735 -59
=======================================
- Hits 32234 32205 -29
+ Misses 25560 25530 -30 ☔ View full report in Codecov by Sentry. |
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/torii/typed-data/src/error.rs (1)
1-37: Ohayo! Well-structured error handling, sensei!The error enum provides comprehensive coverage of failure modes with descriptive messages. However, consider adding documentation comments to explain when each variant is used.
Add rustdoc comments to document each variant:
#[derive(Error, Debug)] pub enum Error { + /// Returned when a type doesn't match the expected format or constraints #[error("Invalid type: {0}")] InvalidType(String), + /// Returned when attempting to use a type that doesn't exist in the system #[error("Type not found: {0}")] TypeNotFound(String), // ... add docs for other variantscrates/torii/libp2p/src/test.rs (1)
503-732: Well-structured client messaging tests!The tests cover both non-WASM and WASM environments with proper timeout handling and error cases.
However, consider adding more assertions to verify the message content in addition to just checking if a message was received.
crates/torii/libp2p/Cargo.toml (1)
8-11: Ohayo! Consider separating default features for better granularity.The current default feature includes both client and server components, which might lead to unnecessary dependencies being pulled in when only one component is needed.
Consider this alternative feature configuration:
[features] client = [] -default = ["client", "server"] +default = ["client"] server = ["dep:sqlx", "dep:torii-core"]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.toml(1 hunks)crates/torii/client/Cargo.toml(1 hunks)crates/torii/client/src/client/error.rs(1 hunks)crates/torii/libp2p/Cargo.toml(2 hunks)crates/torii/libp2p/src/client/mod.rs(1 hunks)crates/torii/libp2p/src/error.rs(2 hunks)crates/torii/libp2p/src/lib.rs(1 hunks)crates/torii/libp2p/src/server/mod.rs(2 hunks)crates/torii/libp2p/src/test.rs(1 hunks)crates/torii/libp2p/src/tests.rs(0 hunks)crates/torii/libp2p/src/types.rs(1 hunks)crates/torii/typed-data/Cargo.toml(1 hunks)crates/torii/typed-data/src/error.rs(1 hunks)crates/torii/typed-data/src/lib.rs(1 hunks)crates/torii/typed-data/src/typed_data.rs(18 hunks)
💤 Files with no reviewable changes (1)
- crates/torii/libp2p/src/tests.rs
✅ Files skipped from review due to trivial changes (4)
- crates/torii/libp2p/src/types.rs
- crates/torii/typed-data/Cargo.toml
- crates/torii/typed-data/src/lib.rs
- crates/torii/libp2p/src/client/mod.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/libp2p/src/test.rs
[error] 1-1: Formatting error detected. Line containing 'use std::error::Error;' needs to be removed according to rustfmt
🔇 Additional comments (11)
crates/torii/libp2p/src/lib.rs (1)
4-13: Ohayo! Module organization looks clean, sensei!The changes follow Rust's conventional module naming patterns and properly separate concerns using feature gates.
crates/torii/client/src/client/error.rs (1)
19-19: Ohayo! Path update looks good, sensei!The update to
torii_relay::error::Errormaintains consistency with the module renaming pattern across the codebase.crates/torii/libp2p/src/error.rs (1)
9-9: Ohayo! Error handling integration looks solid, sensei!The new error variants properly integrate the typed-data functionality while maintaining clean error propagation through
#[from]derivation.However, consider adding a test to verify the error conversion paths:
Also applies to: 51-55
crates/torii/libp2p/src/server/mod.rs (2)
35-35: Ohayo, sensei! Import path update looks good.The change from
errorstoerroraligns with the standard Rust module naming convention for error handling.
132-132: Enhanced peer identification format.The format string now includes both package name and version (
/{}/{}) instead of just/torii-relay/{}, providing more detailed peer identification.crates/torii/libp2p/src/test.rs (1)
21-501: Excellent test coverage for typed data functionality!The test suite thoroughly covers:
- Primitive type parsing and mapping
- Complex type handling (structs, enums)
- Model to typed data conversion
- Error cases
crates/torii/typed-data/src/typed_data.rs (2)
436-445: Ohayo! Nice macro for consistent string parsing, sensei!The
from_str!macro elegantly handles both hexadecimal and decimal string parsing with consistent error handling.
873-874: Enhanced error handling in encode method.The error handling is now more specific with proper context in error messages.
Also applies to: 885-885
crates/torii/client/Cargo.toml (1)
24-24: Ohayo! Proper feature flag usage, sensei!The addition of the "client" feature to torii-relay dependency aligns with the feature set restructuring.
crates/torii/libp2p/Cargo.toml (1)
30-31: LGTM! Well-structured optional dependencies, sensei!The optional dependencies are properly configured and tied to the server feature, following Rust's best practices for conditional compilation.
Cargo.toml (1)
127-127: LGTM! Clean workspace dependency addition.The torii-typed-data dependency is properly integrated into the workspace following the established patterns.
crates/torii/libp2p/src/test.rs
Outdated
|
|
||
| use std::error::Error; |
There was a problem hiding this comment.
Fix formatting issue, sensei!
According to rustfmt, this blank line needs to be removed.
Apply this diff to fix the formatting:
-
use std::error::Error;📝 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.
| use std::error::Error; | |
| use std::error::Error; |
🧰 Tools
🪛 GitHub Actions: ci
[error] 1-1: Formatting error detected. Line containing 'use std::error::Error;' needs to be removed according to rustfmt
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/torii/libp2p/src/test.rs (5)
20-149: Enhance test coverage with negative test cases, sensei!While the primitive type tests are comprehensive for valid inputs, consider adding negative test cases to verify error handling:
- Invalid number formats
- Out of range values
- Malformed U256 objects
151-424: Reduce test data duplication, sensei!Consider extracting the common test data structure into a helper function or constant to avoid duplication between
test_parse_complex_to_tyandtest_map_ty_to_complex.Example:
fn create_test_player_config() -> Ty { Ty::Struct(Struct { name: "PlayerConfig".to_string(), children: vec![ // ... existing structure ... ], }) }
493-493: Consider using a more flexible path resolution, sensei!The hardcoded path
"mocks/model_PlayerConfig.json"might cause issues when running tests from different directories. Consider:
- Using environment variables or test constants for paths
- Making the path relative to the test file using
std::env::current_dir()
534-601: Extract database setup into a helper function, sensei!The database initialization code is quite lengthy. Consider extracting it into a helper function to improve test readability and reusability.
async fn setup_test_database() -> (sqlx::Pool<sqlx::Sqlite>, tempfile::NamedTempFile) { let tempfile = NamedTempFile::new().unwrap(); // ... rest of the setup code ... (pool, tempfile) }
684-685: Make timeout duration configurable, sensei!The 5-second timeout is hardcoded in both native and WASM tests. Consider extracting it into a constant or configuration value for easier adjustment.
const MESSAGE_TIMEOUT_SECS: u64 = 5;Also applies to: 727-728
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/libp2p/src/test.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
crates/torii/libp2p/src/test.rs (1)
1-2: Fix formatting issue, sensei!According to rustfmt, this blank line needs to be removed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
crates/torii/typed-data/src/test.rs (2)
8-79: Ohayo! The primitive type parsing tests look solid, sensei!The test coverage is comprehensive, covering all primitive types with appropriate assertions. Consider using parameterized tests to reduce code duplication.
use test_case::test_case; #[test_case(Primitive::U8(None), PrimitiveType::Number(Number::from(1u64)), Primitive::U8(Some(1)))] #[test_case(Primitive::U16(None), PrimitiveType::Number(Number::from(1u64)), Primitive::U16(Some(1)))] // ... more test cases fn test_parse_primitive_to_ty_parameterized(initial: Primitive, input: PrimitiveType, expected: Primitive) { let mut ty = Ty::Primitive(initial); parse_value_to_ty(&input, &mut ty).unwrap(); assert_eq!(ty, Ty::Primitive(expected)); }
139-309: Complex type parsing tests are thorough but could be more maintainable, sensei!While the test coverage is excellent, consider breaking down the complex test structure into smaller, focused helper functions.
fn create_player_item_struct(item_id: u32, quantity: u32) -> Struct { Struct { name: "PlayerItem".to_string(), children: vec![ Member { name: "item_id".to_string(), ty: Ty::Primitive(Primitive::U32(Some(item_id))), key: false, }, Member { name: "quantity".to_string(), ty: Ty::Primitive(Primitive::U32(Some(quantity))), key: false, }, ], } }crates/torii/libp2p/src/test.rs (3)
187-187: Consider removing unnecessary sleep, sensei!The 2-second sleep before message verification seems arbitrary and could make tests flaky. Consider implementing a proper message acknowledgment mechanism instead.
214-218: Extract certificate hash to a constant, sensei!The hardcoded certificate hash should be moved to a constant for better maintainability and reusability.
+ const RELAY_CERT_HASH: &str = "uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g"; + let mut client = RelayClient::new( - "/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g".to_string(), + format!("/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/{}", RELAY_CERT_HASH), )?;
224-226: Extract topic name to a constant and improve error handling.The hardcoded topic name "mawmaw" should be a constant, and the message publishing could benefit from better error handling.
+ const TEST_TOPIC: &str = "mawmaw"; + const TEST_MESSAGE: &str = "mimi"; - client.command_sender.subscribe("mawmaw".to_string()).await?; + client.command_sender.subscribe(TEST_TOPIC.to_string()).await?; client.command_sender.wait_for_relay().await?; - client.command_sender.publish("mawmaw".to_string(), "mimi".as_bytes().to_vec()).await?; + client.command_sender.publish(TEST_TOPIC.to_string(), TEST_MESSAGE.as_bytes().to_vec()).await?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/torii/libp2p/Cargo.toml(2 hunks)crates/torii/libp2p/src/test.rs(1 hunks)crates/torii/typed-data/src/lib.rs(1 hunks)crates/torii/typed-data/src/test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/typed-data/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/torii/typed-data/src/test.rs (2)
81-137: The type-to-primitive mapping tests are well-structured!Good symmetrical testing with the parse tests, ensuring bidirectional conversion works correctly.
311-412: Complex type mapping tests look good!The test ensures correct bidirectional conversion for complex types. The same helper functions suggested for
test_parse_complex_to_tywould be beneficial here as well.crates/torii/libp2p/Cargo.toml (1)
8-11: Feature flags and dependencies are well-organized, sensei!The separation of client and server features with optional dependencies is a good practice for reducing binary size and compilation time.
Also applies to: 28-28, 30-31
crates/torii/libp2p/src/test.rs (1)
1-11: Ohayo sensei! Let's clean up these imports.There are unnecessary blank lines between imports that should be removed for better readability.
crates/torii/typed-data/src/test.rs
Outdated
| #[test] | ||
| fn test_model_to_typed_data() { | ||
| let ty = Ty::Struct(Struct { | ||
| name: "PlayerConfig".to_string(), | ||
| children: vec![ | ||
| Member { | ||
| name: "player".to_string(), | ||
| ty: Ty::Primitive(Primitive::ContractAddress(Some(Felt::ONE))), | ||
| key: true, | ||
| }, | ||
| Member { name: "name".to_string(), ty: Ty::ByteArray("mimi".to_string()), key: false }, | ||
| Member { | ||
| name: "items".to_string(), | ||
| // array of PlayerItem struct | ||
| ty: Ty::Array(vec![Ty::Struct(Struct { | ||
| name: "PlayerItem".to_string(), | ||
| children: vec![ | ||
| Member { | ||
| name: "item_id".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(1))), | ||
| key: false, | ||
| }, | ||
| Member { | ||
| name: "quantity".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(1))), | ||
| key: false, | ||
| }, | ||
| ], | ||
| })]), | ||
| key: false, | ||
| }, | ||
| // a favorite_item field with enum type Option<PlayerItem> | ||
| Member { | ||
| name: "favorite_item".to_string(), | ||
| ty: Ty::Enum(Enum { | ||
| name: "Option".to_string(), | ||
| option: Some(1), | ||
| options: vec![ | ||
| EnumOption { name: "None".to_string(), ty: Ty::Tuple(vec![]) }, | ||
| EnumOption { | ||
| name: "Some".to_string(), | ||
| ty: Ty::Struct(Struct { | ||
| name: "PlayerItem".to_string(), | ||
| children: vec![ | ||
| Member { | ||
| name: "item_id".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(69))), | ||
| key: false, | ||
| }, | ||
| Member { | ||
| name: "quantity".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(42))), | ||
| key: false, | ||
| }, | ||
| ], | ||
| }), | ||
| }, | ||
| ], | ||
| }), | ||
| key: false, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| let typed_data = | ||
| TypedData::from_model(ty, Domain::new("Test", "1", "Test", Some("1"))).unwrap(); | ||
|
|
||
| let path = "mocks/model_PlayerConfig.json"; | ||
| let file = std::fs::File::open(path).unwrap(); | ||
| let reader = std::io::BufReader::new(file); | ||
|
|
||
| let file_typed_data: TypedData = serde_json::from_reader(reader).unwrap(); | ||
|
|
||
| assert_eq!(typed_data.encode(Felt::ZERO).unwrap(), file_typed_data.encode(Felt::ZERO).unwrap()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the model test more robust, sensei!
The test relies on an external JSON file with a hardcoded path, which could cause issues in different environments.
Consider either:
- Embedding the expected JSON as a const string in the test file
- Using a relative path with the
env!("CARGO_MANIFEST_DIR")macro:
let path = format!("{}/mocks/model_PlayerConfig.json", env!("CARGO_MANIFEST_DIR"));| let timeout = wasm_timer::Delay::new(std::time::Duration::from_secs(2)); | ||
| let mut message_future = client.message_receiver.lock().await; | ||
| let message_future = message_future.next(); | ||
|
|
||
| match select(message_future, timeout).await { | ||
| Either::Left((Some(_message), _)) => { | ||
| println!("Test OK: Received message within 5 seconds."); | ||
| Ok(()) | ||
| } | ||
| _ => { | ||
| println!("Test Failed: Did not receive message within 5 seconds."); | ||
| Err("Timeout reached without receiving a message".into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align timeout duration with the error message.
The timeout duration (2 seconds) doesn't match the error message which mentions 5 seconds. Consider using a constant and aligning these values.
+ const MESSAGE_TIMEOUT: Duration = Duration::from_secs(5);
- let timeout = wasm_timer::Delay::new(std::time::Duration::from_secs(2));
+ let timeout = wasm_timer::Delay::new(MESSAGE_TIMEOUT);
match select(message_future, timeout).await {
Either::Left((Some(_message), _)) => {
- println!("Test OK: Received message within 5 seconds.");
+ println!("Test OK: Received message within {} seconds.", MESSAGE_TIMEOUT.as_secs());
Ok(())
}
_ => {
- println!("Test Failed: Did not receive message within 5 seconds.");
+ println!("Test Failed: Did not receive message within {} seconds.", MESSAGE_TIMEOUT.as_secs());
Err("Timeout reached without receiving a message".into())
}
}Committable suggestion skipped: line range outside the PR's diff.
| loop { | ||
| select! { | ||
| entity = sqlx::query("SELECT * FROM entities").fetch_one(&pool) => if entity.is_ok() { | ||
| println!("Test OK: Received message within 5 seconds."); | ||
| return Ok(()); | ||
| }, | ||
| _ = sleep(Duration::from_secs(5)) => { | ||
| println!("Test Failed: Did not receive message within 5 seconds."); | ||
| return Err("Timeout reached without receiving a message".into()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test reliability with proper cleanup and constants.
The message verification loop has several areas for improvement:
- The 5-second timeout should be defined as a constant
- The SQL query should be parameterized
- Missing cleanup of database and network resources
Here's a suggested improvement:
+ const MESSAGE_VERIFICATION_TIMEOUT: Duration = Duration::from_secs(5);
+ const MESSAGE_QUERY: &str = "SELECT * FROM entities";
loop {
select! {
- entity = sqlx::query("SELECT * FROM entities").fetch_one(&pool) => if entity.is_ok() {
+ entity = sqlx::query(MESSAGE_QUERY).fetch_one(&pool) => if entity.is_ok() {
println!("Test OK: Received message within 5 seconds.");
+ cleanup_resources(&pool).await?;
return Ok(());
},
- _ = sleep(Duration::from_secs(5)) => {
+ _ = sleep(MESSAGE_VERIFICATION_TIMEOUT) => {
println!("Test Failed: Did not receive message within 5 seconds.");
+ cleanup_resources(&pool).await?;
return Err("Timeout reached without receiving a message".into());
}
}
}
+ async fn cleanup_resources(pool: &SqlitePool) -> Result<(), Box<dyn Error>> {
+ // Clean up database
+ sqlx::query("DELETE FROM entities").execute(pool).await?;
+ Ok(())
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/torii/libp2p/src/test.rs (3)
186-187: 🛠️ Refactor suggestionAlign sleep duration with timeout.
The sleep duration (2 seconds) is inconsistent with the timeout check (5 seconds) in the following loop.
188-200: 🛠️ Refactor suggestionEnhance test reliability with proper cleanup.
The test should clean up resources after completion and use parameterized queries.
227-240: 🛠️ Refactor suggestionAlign timeout duration with error messages.
The timeout duration (2 seconds) doesn't match the error message which mentions 5 seconds.
🧹 Nitpick comments (4)
crates/torii/libp2p/src/test.rs (2)
113-113: Ohayo sensei! Consider using dynamic port allocation.The hardcoded ports (9900, 9901, 9902) could cause conflicts in CI environments or when running multiple tests in parallel.
Consider using port 0 to let the OS assign available ports:
- let mut relay_server = Relay::new(db, provider, 9900, 9901, 9902, None, None)?; + let mut relay_server = Relay::new(db, provider, 0, 0, 0, None, None)?;
213-217: Make certificate hash configurable.The hardcoded certificate hash could make tests brittle and harder to maintain.
Consider making it configurable through environment variables or test parameters:
+ const DEFAULT_CERT_HASH: &str = "uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g"; + let mut client = RelayClient::new( - "/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/\ - uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g" + format!("/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/{}", + std::env::var("TEST_CERT_HASH").unwrap_or(DEFAULT_CERT_HASH.to_string())) .to_string(), )?;crates/torii/typed-data/src/test.rs (2)
9-80: Consider adding negative test cases, sensei!While the primitive type parsing tests are thorough for valid inputs, they could be more robust with additional test cases for:
- Invalid input values (e.g., negative numbers for unsigned types)
- Type mismatches (e.g., string for number types)
- Overflow scenarios
Example addition:
#[test] fn test_parse_primitive_to_ty_invalid_inputs() { let mut ty = Ty::Primitive(Primitive::U8(None)); let value = PrimitiveType::String("not_a_number".to_string()); assert!(parse_value_to_ty(&value, &mut ty).is_err()); }
140-310: Consider using named constants for test values, sensei!The test uses magic numbers and hardcoded strings that could be more meaningful with named constants:
const TEST_PLAYER_NAME: &str = "mimi"; const TEST_ITEM_ID: u32 = 1; const TEST_QUANTITY: u32 = 1;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/libp2p/src/test.rs(1 hunks)crates/torii/typed-data/src/lib.rs(1 hunks)crates/torii/typed-data/src/test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/typed-data/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/torii/libp2p/src/test.rs (1)
1-11: Ohayo sensei! Clean and well-organized imports!The imports are nicely structured and the WASM configuration is properly isolated with conditional compilation.
crates/torii/typed-data/src/test.rs (3)
1-7: LGTM! Well-organized imports, sensei!The imports are logically organized and include all necessary dependencies.
82-138: Verify the number representation consistency, sensei!There's an inconsistency in how numbers are represented:
- U8 through USize use
PrimitiveType::Number- U64 and larger types use
PrimitiveType::StringWhile this might be intentional due to number size limitations, it should be documented to explain the reasoning.
482-484: Consider making the model test more robust, sensei!The test relies on an external JSON file with a hardcoded path, which could cause issues in different environments.
| let ty = Ty::Struct(Struct { | ||
| name: "PlayerConfig".to_string(), | ||
| children: vec![ | ||
| Member { | ||
| name: "player".to_string(), | ||
| ty: Ty::Primitive(Primitive::ContractAddress(Some(Felt::ONE))), | ||
| key: true, | ||
| }, | ||
| Member { name: "name".to_string(), ty: Ty::ByteArray("mimi".to_string()), key: false }, | ||
| Member { | ||
| name: "items".to_string(), | ||
| ty: Ty::Array(vec![Ty::Struct(Struct { | ||
| name: "PlayerItem".to_string(), | ||
| children: vec![ | ||
| Member { | ||
| name: "item_id".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(1))), | ||
| key: false, | ||
| }, | ||
| Member { | ||
| name: "quantity".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(1))), | ||
| key: false, | ||
| }, | ||
| ], | ||
| })]), | ||
| key: false, | ||
| }, | ||
| Member { | ||
| name: "favorite_item".to_string(), | ||
| ty: Ty::Enum(Enum { | ||
| name: "Option".to_string(), | ||
| option: Some(1_u8), | ||
| options: vec![ | ||
| EnumOption { name: "None".to_string(), ty: Ty::Tuple(vec![]) }, | ||
| EnumOption { | ||
| name: "Some".to_string(), | ||
| ty: Ty::Struct(Struct { | ||
| name: "PlayerItem".to_string(), | ||
| children: vec![ | ||
| Member { | ||
| name: "item_id".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(1))), | ||
| key: false, | ||
| }, | ||
| Member { | ||
| name: "quantity".to_string(), | ||
| ty: Ty::Primitive(Primitive::U32(Some(1))), | ||
| key: false, | ||
| }, | ||
| ], | ||
| }), | ||
| }, | ||
| ], | ||
| }), | ||
| key: false, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| let value = PrimitiveType::Object( | ||
| vec![ | ||
| ("player".to_string(), PrimitiveType::String("1".to_string())), | ||
| ("name".to_string(), PrimitiveType::String("mimi".to_string())), | ||
| ( | ||
| "items".to_string(), | ||
| PrimitiveType::Array(vec![PrimitiveType::Object( | ||
| vec![ | ||
| ("item_id".to_string(), PrimitiveType::Number(Number::from(1u64))), | ||
| ("quantity".to_string(), PrimitiveType::Number(Number::from(1u64))), | ||
| ] | ||
| .into_iter() | ||
| .collect(), | ||
| )]), | ||
| ), | ||
| ( | ||
| "favorite_item".to_string(), | ||
| PrimitiveType::Object( | ||
| vec![( | ||
| "Some".to_string(), | ||
| PrimitiveType::Object( | ||
| vec![ | ||
| ("item_id".to_string(), PrimitiveType::Number(Number::from(1u64))), | ||
| ("quantity".to_string(), PrimitiveType::Number(Number::from(1u64))), | ||
| ] | ||
| .into_iter() | ||
| .collect(), | ||
| ), | ||
| )] | ||
| .into_iter() | ||
| .collect(), | ||
| ), | ||
| ), | ||
| ] | ||
| .into_iter() | ||
| .collect(), | ||
| ); | ||
|
|
||
| assert_eq!(value, map_ty_to_primitive(&ty).unwrap()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce test code duplication, sensei!
The test structure is largely duplicated from test_parse_complex_to_ty. Consider extracting common test data setup into helper functions:
fn create_test_player_config() -> Ty {
Ty::Struct(Struct {
name: "PlayerConfig".to_string(),
children: vec![
// ... common structure
],
})
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/libp2p/Cargo.toml (1)
22-31: Clean dependency organization with optional flags!Good job making server-specific dependencies optional! This helps reduce the dependency tree for client-only usage.
However, consider adding a comment explaining the dependency split between client and server features for better maintainability.
[dependencies] futures.workspace = true serde.workspace = true # preserve order anyhow.workspace = true serde_json.workspace = true starknet.workspace = true thiserror.workspace = true torii-typed-data.workspace = true tracing.workspace = true +# Server-specific dependencies sqlx = { workspace = true, optional = true } torii-core = { workspace = true, optional = true }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
bin/torii/Cargo.toml(1 hunks)crates/torii/libp2p/Cargo.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
bin/torii/Cargo.toml (1)
42-42: Ohayo sensei! The dependency configuration looks good!The explicit feature selection for
torii-relaywith bothclientandserverfeatures makes sense for the binary crate, as it needs full functionality.crates/torii/libp2p/Cargo.toml (2)
40-44: Well-structured WASM testing setup!Good addition of WASM-specific testing dependencies. This ensures proper testing in browser environments.
46-53: Verify libp2p feature compatibilityThe feature sets between native and WASM targets are different. Please ensure that the reduced feature set for WASM doesn't break any required functionality.
Let's check for any feature-dependent code that might break in WASM:
✅ Verification successful
Ohayo sensei!
Feature differences are properly handled ✨
The WASM build correctly uses WebRTC (via websys) as its transport mechanism, which is the appropriate choice for browser environments, while native builds use TCP/WebSocket/DNS/relay. The different feature sets are intentional and well-implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of features that are only enabled in native target rg --type rust '\b(dns|quic|relay|websocket)\b' crates/torii/libp2p/src/Length of output: 1689
Script:
#!/bin/bash # Search for WASM-specific code paths and configurations rg --type rust 'cfg\(target_arch\s*=\s*"wasm32"\)' crates/torii/libp2p/src/ # Also check for WebRTC and WebSocket usage in WASM rg --type rust '\b(webrtc|websys)\b' crates/torii/libp2p/src/Length of output: 802
Script:
#!/bin/bash # Get content of files with WASM implementations, showing context rg -B 5 -A 10 --type rust 'cfg\(target_arch\s*=\s*"wasm32"\)' crates/torii/libp2p/src/ # Also check for transport setup patterns ast-grep --pattern 'Transport::new($$$)'Length of output: 4807
crates/torii/libp2p/Cargo.toml
Outdated
| [features] | ||
| client = [ ] | ||
| server = [ "dep:sqlx", "dep:torii-core", "dep:dojo-types", "dep:dojo-world", "dep:starknet-crypto", "dep:chrono", "dep:libp2p-webrtc", "dep:rand" ] |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Your client feature needs its dependencies, sensei!
The client module uses several libp2p-related dependencies that should be gated behind the client feature:
- Add
"dep:libp2p","dep:futures"to the client feature - For WASM support:
"dep:libp2p-webrtc-websys","dep:libp2p-websocket-websys" - Other essential deps:
"dep:tracing","dep:serde_json"
🔗 Analysis chain
Ohayo! The empty client feature needs attention, sensei!
The client feature is currently empty. If there are client-specific dependencies or functionality, they should be included in this feature flag.
Let's check if there are any client-specific dependencies that should be gated behind this feature:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for #[cfg(feature = "client")] usage
rg --type rust 'cfg\(feature = "client"\)' crates/torii/libp2p/src/
Length of output: 127
Script:
#!/bin/bash
# Check the context around client feature flag
rg --type rust -B 3 -A 10 'cfg\(feature = "client"\)' crates/torii/libp2p/src/lib.rs
# List all dependencies in use
ast-grep --pattern 'use $_::$$$'
Length of output: 66114
Script:
#!/bin/bash
# Check the contents of the client module
cat crates/torii/libp2p/src/client/mod.rs
# Look for any use statements in client-related files
find crates/torii/libp2p/src/client -type f -name "*.rs" -exec rg '^use' {} \;
Length of output: 10184
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/libp2p/Cargo.toml (1)
9-9:⚠️ Potential issueOhayo sensei! The client feature needs its dependencies!
The
clientfeature is empty but the client module uses several libp2p-related dependencies. These should be gated behind theclientfeature:
"dep:libp2p""dep:futures"- For WASM support:
"dep:libp2p-webrtc-websys","dep:libp2p-websocket-websys"- Other essential deps:
"dep:tracing","dep:serde_json"
🧹 Nitpick comments (1)
crates/torii/libp2p/Cargo.toml (1)
40-44: Ohayo sensei! Consider using workspace versions for WASM dependencies!The WASM dev-dependencies use fixed versions which could become outdated. Consider moving these to workspace-level version management for better maintainability:
tracing-wasm = { workspace = true } wasm-bindgen-futures = { workspace = true } wasm-bindgen-test = { workspace = true } wasm-timer = { workspace = true }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)bin/torii/Cargo.toml(1 hunks)crates/torii/libp2p/Cargo.toml(1 hunks)crates/torii/libp2p/src/server/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- bin/torii/Cargo.toml
- crates/torii/libp2p/src/server/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (2)
crates/torii/libp2p/Cargo.toml (2)
22-31: Ohayo! Nice work on the dependency organization, sensei!The changes look good:
- Clean separation of server-specific dependencies using
optional = true- Addition of
torii-typed-dataaligns with the PR objectives
46-53: Ohayo! Excellent target-specific configuration, sensei!The libp2p feature sets are well-configured:
- Non-WASM target includes full feature set
- WASM target includes browser-compatible features
- Consistent git revision (
cdc9638) across all libp2p dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/torii/libp2p/src/test.rs (2)
188-200: 🛠️ Refactor suggestionEnhance test reliability with proper cleanup, sensei!
The message verification loop needs improvement with proper cleanup and constants.
227-240: 🛠️ Refactor suggestionAlign timeout duration with error message, sensei!
The timeout duration (2 seconds) doesn't match the error message which mentions 5 seconds.
🧹 Nitpick comments (2)
crates/torii/libp2p/src/test.rs (2)
186-186: Ohayo sensei! Consider removing arbitrary sleep.The 2-second sleep seems arbitrary and could make the test flaky. Consider using a proper wait mechanism or explaining the necessity of this delay in a comment.
- sleep(std::time::Duration::from_secs(2)).await; + // Wait for message propagation through the network + const PROPAGATION_DELAY: Duration = Duration::from_millis(500); + sleep(PROPAGATION_DELAY).await;
213-217: Extract certificate hash as a configuration constant, sensei!The hardcoded certificate hash should be configurable to make the test more maintainable.
+ const CERT_HASH: &str = "uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g"; + const RELAY_ADDRESS: &str = "/ip4/127.0.0.1/udp/9091/webrtc-direct"; + let mut client = RelayClient::new( - "/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/\ - uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g" - .to_string(), + format!("{}/certhash/{}", RELAY_ADDRESS, CERT_HASH), )?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/torii/indexer/src/processors/erc20_legacy_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc20_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc721_legacy_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc721_transfer.rs(1 hunks)crates/torii/indexer/src/processors/event_message.rs(1 hunks)crates/torii/indexer/src/processors/metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/register_event.rs(1 hunks)crates/torii/indexer/src/processors/register_model.rs(1 hunks)crates/torii/indexer/src/processors/store_del_record.rs(1 hunks)crates/torii/indexer/src/processors/store_set_record.rs(1 hunks)crates/torii/indexer/src/processors/store_update_member.rs(1 hunks)crates/torii/indexer/src/processors/store_update_record.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_event.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(1 hunks)crates/torii/libp2p/src/test.rs(1 hunks)crates/torii/sqlite/src/executor/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (15)
- crates/torii/indexer/src/processors/erc721_transfer.rs
- crates/torii/indexer/src/processors/store_del_record.rs
- crates/torii/indexer/src/processors/upgrade_model.rs
- crates/torii/indexer/src/processors/store_update_record.rs
- crates/torii/indexer/src/processors/upgrade_event.rs
- crates/torii/indexer/src/processors/register_event.rs
- crates/torii/indexer/src/processors/register_model.rs
- crates/torii/indexer/src/processors/erc20_transfer.rs
- crates/torii/indexer/src/processors/store_set_record.rs
- crates/torii/indexer/src/processors/erc20_legacy_transfer.rs
- crates/torii/indexer/src/processors/metadata_update.rs
- crates/torii/indexer/src/processors/store_update_member.rs
- crates/torii/indexer/src/processors/erc721_legacy_transfer.rs
- crates/torii/sqlite/src/executor/mod.rs
- crates/torii/indexer/src/processors/event_message.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/libp2p/src/test.rs
[error] 28-28: Incorrect ordering of imports. The file needs to be formatted according to Rust formatting standards.
| use std::error::Error; | ||
|
|
||
| use crate::client::RelayClient; | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); | ||
| use dojo_types::primitive::Primitive; | ||
| use katana_runner::KatanaRunner; | ||
| #[cfg(target_arch = "wasm32")] | ||
| use wasm_bindgen_test::*; | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Let's fix the import ordering.
The imports need to be reorganized according to Rust's standard grouping (std → external → crate).
Apply this diff to fix the import ordering:
use std::error::Error;
-
-use crate::client::RelayClient;
-
#[cfg(target_arch = "wasm32")]
wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser);
+
use dojo_types::primitive::Primitive;
use katana_runner::KatanaRunner;
#[cfg(target_arch = "wasm32")]
use wasm_bindgen_test::*;
+
+use crate::client::RelayClient;📝 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.
| use std::error::Error; | |
| use crate::client::RelayClient; | |
| #[cfg(target_arch = "wasm32")] | |
| wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); | |
| use dojo_types::primitive::Primitive; | |
| use katana_runner::KatanaRunner; | |
| #[cfg(target_arch = "wasm32")] | |
| use wasm_bindgen_test::*; | |
| use std::error::Error; | |
| #[cfg(target_arch = "wasm32")] | |
| wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); | |
| use dojo_types::primitive::Primitive; | |
| use katana_runner::KatanaRunner; | |
| #[cfg(target_arch = "wasm32")] | |
| use wasm_bindgen_test::*; | |
| use crate::client::RelayClient; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/torii/libp2p/src/test.rs (1)
213-217: Make certificate hash configurable.The hardcoded certificate hash makes the test brittle and environment-dependent.
+ const DEFAULT_CERT_HASH: &str = "uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g"; + let mut client = RelayClient::new( - "/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/\ - uEiCAoeHQh49fCHDolECesXO0CPR7fpz0sv0PWVaIahzT4g" + format!( + "/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/{}", + std::env::var("TEST_CERT_HASH").unwrap_or(DEFAULT_CERT_HASH.to_string()) + ) .to_string(), )?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/libp2p/src/test.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (3)
crates/torii/libp2p/src/test.rs (3)
1-11: Ohayo sensei! Let's fix the import ordering.The imports need to be reorganized according to Rust's standard grouping (std → external → crate).
188-200: Enhance test reliability with proper cleanup and constants.The message verification loop has several areas for improvement:
- The timeout duration should be defined as a constant
- The SQL query should be parameterized
- Missing cleanup of database and network resources
227-240: Align timeout duration with the error message.The timeout duration (2 seconds) doesn't match the error message which mentions 5 seconds.
| let mut relay_server = Relay::new(db, provider, 9900, 9901, 9902, None, None)?; | ||
| tokio::spawn(async move { | ||
| relay_server.run().await; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for relay server initialization.
The relay server is spawned without proper error handling. If the server fails to start, the test might hang or fail without a clear reason.
- tokio::spawn(async move {
- relay_server.run().await;
+ tokio::spawn(async move {
+ if let Err(e) = relay_server.run().await {
+ tracing::error!("Relay server failed: {}", 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.
| let mut relay_server = Relay::new(db, provider, 9900, 9901, 9902, None, None)?; | |
| tokio::spawn(async move { | |
| relay_server.run().await; | |
| }); | |
| let mut relay_server = Relay::new(db, provider, 9900, 9901, 9902, None, None)?; | |
| tokio::spawn(async move { | |
| if let Err(e) = relay_server.run().await { | |
| tracing::error!("Relay server failed: {}", e); | |
| } | |
| }); |
| sleep(std::time::Duration::from_secs(2)).await; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace arbitrary sleep with proper synchronization.
Using a fixed sleep duration is not reliable for test synchronization.
Consider using a channel or event to signal when the client is ready:
- sleep(std::time::Duration::from_secs(2)).await;
+ let (ready_tx, ready_rx) = tokio::sync::oneshot::channel();
+ client.command_sender.on_ready(move || {
+ let _ = ready_tx.send(());
+ });
+ let _ = tokio::time::timeout(
+ Duration::from_secs(5),
+ ready_rx
+ ).await.map_err(|_| "Client failed to initialize within timeout")?;Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
torii-typed-datapackage to the workspace.Bug Fixes
Refactor
Tests
libp2pcrate.