refactor: update & fix unity bindgen #2631
Conversation
WalkthroughOhayo, sensei! The changes in this pull request enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UnityPlugin
participant ModelStruct
participant CSharpCode
User->>UnityPlugin: Request code generation
UnityPlugin->>ModelStruct: Check for model struct
alt Model struct found
UnityPlugin->>UnityPlugin: Filter tokens
UnityPlugin->>CSharpCode: Format relevant tokens
else Model struct not found
UnityPlugin-->>User: Error: Model struct not found
end
UnityPlugin-->>User: Return generated C# code
🪧 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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/bindgen/src/plugins/unity/mod.rs(2 hunks)
🔇 Additional comments (3)
crates/dojo/bindgen/src/plugins/unity/mod.rs (3)
280-282: LGTM! Proper enum filtering implementation.
The check ensures that only enums referenced by the model structs are formatted, maintaining consistency with the struct handling approach.
287-288: LGTM! Clean model formatting with proper namespace.
The implementation correctly uses the namespace from tag and formats the model using the filtered struct.
490-490: LGTM! Proper contract address wrapping.
The contract address is correctly wrapped in FieldElement, ensuring proper type conversion for Starknet interaction.
Let's verify that all contract address usages are consistently wrapped:
✅ Verification successful
Let me modify the search to look for specific patterns in both C# and Rust code to verify the contract address handling consistency. The previous regex was too complex and unsupported. This will help us see the surrounding context of how contract addresses are used.
Contract address wrapping is correctly implemented, sensei!
The verification shows that contract addresses are properly handled across the codebase:
- In Unity (C#), the contract address is wrapped with FieldElement as seen in the reviewed line
- In TypeScript bindings, contract addresses are handled as strings and properly converted during execution
- In GraphQL/Rust code, addresses are consistently handled as strings for external interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct contract address usage without FieldElement wrapping
rg -g '*.cs' 'contractAddress(?!.*FieldElement)'
Length of output: 302
Script:
#!/bin/bash
# Search for contract address usage patterns with proper context
rg --pcre2 "contractAddress" -g "*.cs" -B2 -A2
# Also search in Rust code to understand the context better
rg --pcre2 "contractAddress" -g "*.rs" -B2 -A2
Length of output: 9190
| let model_struct = model_struct.expect("model struct not found"); | ||
| handled_tokens | ||
| .iter() | ||
| .filter(|(_, s)| { | ||
| model_struct.inners.iter().any(|inner| { | ||
| inner.token.type_name() == s.type_name() | ||
| && inner.token.type_name() != "ByteArray" | ||
| }) | ||
| }) | ||
| .for_each(|(_, s)| { | ||
| out += UnityPlugin::format_struct(s).as_str(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider improving error handling for model struct lookup.
While the filtering logic for handled tokens is well-implemented, the expect() call on line 261 could cause a panic if the model struct is not found. Consider using a more graceful error handling approach.
- let model_struct = model_struct.expect("model struct not found");
+ let model_struct = model_struct.ok_or_else(|| {
+ anyhow::anyhow!("Model struct not found for {}", model.tag)
+ })?;📝 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 model_struct = model_struct.expect("model struct not found"); | |
| handled_tokens | |
| .iter() | |
| .filter(|(_, s)| { | |
| model_struct.inners.iter().any(|inner| { | |
| inner.token.type_name() == s.type_name() | |
| && inner.token.type_name() != "ByteArray" | |
| }) | |
| }) | |
| .for_each(|(_, s)| { | |
| out += UnityPlugin::format_struct(s).as_str(); | |
| }); | |
| let model_struct = model_struct.ok_or_else(|| { | |
| anyhow::anyhow!("Model struct not found for {}", model.tag) | |
| })?; | |
| handled_tokens | |
| .iter() | |
| .filter(|(_, s)| { | |
| model_struct.inners.iter().any(|inner| { | |
| inner.token.type_name() == s.type_name() | |
| && inner.token.type_name() != "ByteArray" | |
| }) | |
| }) | |
| .for_each(|(_, s)| { | |
| out += UnityPlugin::format_struct(s).as_str(); | |
| }); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2631 +/- ##
==========================================
- Coverage 56.91% 56.79% -0.13%
==========================================
Files 397 397
Lines 49467 49560 +93
==========================================
- Hits 28153 28146 -7
- Misses 21314 21414 +100 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/dojo/bindgen/src/plugins/unity/mod.rs (2)
264-274: Consider consolidating duplicate filtering logic.The filtering logic for structs and enums is very similar. Consider extracting this into a shared helper function to improve maintainability.
+ fn filter_dependencies( + handled_tokens: &HashMap<String, Composite>, + model_struct: &Composite, + composite_type: CompositeType, + ) -> Vec<String> { + handled_tokens + .iter() + .filter(|(_, s)| { + model_struct.inners.iter().any(|inner| { + s.r#type == composite_type + && check_token_in_recursively(&inner.token, &s.type_name()) + && (composite_type != CompositeType::Struct + || inner.token.type_name() != "ByteArray") + }) + }) + .map(|(k, _)| k.clone()) + .collect() + } - let struct_keys: Vec<String> = handled_tokens - .iter() - .filter(|(_, s)| { - model_struct.inners.iter().any(|inner| { - s.r#type == CompositeType::Struct - && check_token_in_recursively(&inner.token, &s.type_name()) - && inner.token.type_name() != "ByteArray" - }) - }) - .map(|(k, _)| k.clone()) - .collect(); + let struct_keys = filter_dependencies(handled_tokens, model_struct, CompositeType::Struct); - let enum_keys: Vec<String> = handled_tokens - .iter() - .filter(|(_, s)| { - model_struct.inners.iter().any(|inner| { - s.r#type == CompositeType::Enum - && check_token_in_recursively(&inner.token, &s.type_name()) - }) - }) - .map(|(k, _)| k.clone()) - .collect(); + let enum_keys = filter_dependencies(handled_tokens, model_struct, CompositeType::Enum);Also applies to: 292-301
568-582: Ohayo sensei! Consider adding documentation for the recursive checker.The
check_token_in_recursivelyfunction is well-implemented but could benefit from documentation explaining its purpose and behavior.+/// Recursively checks if a token or any of its nested tokens match the given type name. +/// +/// # Arguments +/// * `token` - The token to check +/// * `type_name` - The type name to match against +/// +/// # Returns +/// `true` if the token or any of its nested tokens match the type name fn check_token_in_recursively(token: &Token, type_name: &str) -> bool {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/bindgen/src/plugins/unity/mod.rs(7 hunks)
🔇 Additional comments (1)
crates/dojo/bindgen/src/plugins/unity/mod.rs (1)
261-261: Ohayo sensei! Consider improving error handling for model struct lookup.
The expect() call could cause a panic if the model struct is not found. Consider using a more graceful error handling approach.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation