fix: u256 and ns for unity bindgen #2635
Conversation
WalkthroughOhayo, sensei! This pull request introduces several modifications to 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: 1
🧹 Outside diff range and nitpick comments (1)
crates/dojo/bindgen/src/plugins/unity/mod.rs (1)
206-215: Ohayo, sensei! Remove empty 'Start' and 'Update' methods if unnecessaryThe
Start()andUpdate()methods in the model class are empty. If the model class does not require Unity's lifecycle methods, consider removing them to simplify the code and improve readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/bindgen/src/plugins/unity/mod.rs(4 hunks)
🔇 Additional comments (2)
crates/dojo/bindgen/src/plugins/unity/mod.rs (2)
107-107: Ohayo, sensei! Addition of 'BigInteger' using statement is appropriate
Including using BigInteger = System.Numerics.BigInteger; ensures that BigInteger is correctly referenced for i128, u128, and u256 type mappings. This enhances type handling in both contract_imports and model_imports.
Also applies to: 120-120
347-358: Ohayo, sensei! Verify the handling of 'U256' composite type
The handle_arg_recursive function now handles CompositeType::Unknown for U256 by decomposing it into high and low fields and converting them to FieldElements. Please ensure that:
- The
U256type is defined with accessiblehighandlowfields. - This approach aligns with how
U256should be serialized in the system.
You can run the following script to verify the definition of U256:
✅ Verification successful
Ohayo, sensei! The U256 handling is correctly implemented
Based on the codebase analysis, the handling of U256 with high and low components is verified to be correct. The code uses multiple U256 implementations:
alloy_primitives::U256- Used extensively in Katana, with proper high/low bit manipulation:
let low_u128: u128 = (value & U256::from(u128::MAX)).to();
let high_u128: u128 = U256::from(value >> 128).to();starknet::core::types::U256- Used in Torii with proper high/low field access:
U256::from_words(value.low, value.high)The approach in the review code correctly decomposes U256 into high and low FieldElements, which aligns with both implementations' serialization patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'U256' is defined with 'high' and 'low' fields.
# Test: Search for the definition of 'U256' struct with 'high' and 'low' fields.
ast-grep --pattern $'struct U256 {{
high: $_,
low: $_,
$$$
}}'
Length of output: 69
Script:
#!/bin/bash
# Let's try a broader search for U256 definitions and usages
# Search for U256 struct definition in Rust files
rg "struct U256" -A 5
# Search for U256 type imports or uses
rg "use.*U256"
# Search for any mentions of U256 with high or low
rg "U256.*(?:high|low)|(?:high|low).*U256" -p
Length of output: 6296
Script:
#!/bin/bash
# Let's also check if U256 might be defined in dependencies
# by looking at Cargo.toml files and their dependencies
fd Cargo.toml --exec cat {}
Length of output: 54307
Script:
#!/bin/bash
# Check if U256 might be coming from common crypto/numeric libraries
rg "primitive-types|uint|num-bigint" Cargo.toml
Length of output: 70
Script:
#!/bin/bash
# Let's check for U256 in alloy-primitives since we see it's used in many imports
rg "use alloy_primitives::U256" -A 3
# Also check for any U256 implementations or methods with high/low
rg "impl.*U256|fn.*U256.*high|fn.*U256.*low" -A 5
Length of output: 4389
Script:
#!/bin/bash
# Let's also check the starknet crate's U256 usage since it's imported in many places
rg "use starknet::.*U256" -A 3
Length of output: 2582
| // Model definition for `{}` model | ||
| public class {}_{} : ModelInstance {{ | ||
| {} | ||
|
|
||
| // Start is called before the first frame update | ||
| void Start() {{ | ||
| }} | ||
|
|
||
| // Update is called once per frame | ||
| void Update() {{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider using C# namespaces instead of prefixing class names
Currently, the model class name includes the namespace appended with an underscore (e.g., namespace_ModelName). In C#, it's conventional to use the namespace keyword to define namespaces, which improves code organization and adheres to best practices.
public class {}_{} : ModelInstance {{
+// Suggested change:
+namespace {} {{
+ public class {} : ModelInstance {{
{}
}}
+}}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2635 +/- ##
==========================================
- Coverage 56.79% 56.76% -0.03%
==========================================
Files 397 397
Lines 49560 49573 +13
==========================================
- Hits 28148 28142 -6
- Misses 21412 21431 +19 ☔ View full report in Codecov by Sentry. |
| // Update is called once per frame | ||
| void Update() {{ | ||
| }} | ||
| // Model definition for `{}` model |
There was a problem hiding this comment.
| // Model definition for `{}` model | |
| // Model definition for `{}_{}` model |
Summary by CodeRabbit
New Features
BigIntegerand improved model class generation in the UnityPlugin.U256, generating separate instances for high and low values.Bug Fixes
Documentation