fix(dojo-lang): remove restriction for u256 as keys in models#2890
fix(dojo-lang): remove restriction for u256 as keys in models#2890glihm merged 3 commits intodojoengine:mainfrom
Conversation
WalkthroughOhayo, sensei! The pull request introduces a new struct Changes
Sequence DiagramsequenceDiagram
participant Parser
participant Members
Parser->>Members: map() instead of filter_map()
Note over Parser,Members: Unconditional member creation
Members-->>Parser: Return all members without validation
Ohayo and happy coding, sensei! 🥷🏼🔧 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
glihm
changed the title
|
Needs to be checked at Torii level, since multiple felt key are not supported at the moment. |
I think it should be all good as we directly use the raw felts to serialize them into our keys string. We should still do some testing to be sure there are no other unexpected issues though |
|
and at that point, we could also use any type as the key. as they are all serializable to felt. if at the dojo level it is supported, we could support enums as keys |
![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
🔭 Outside diff range comments (1)
crates/dojo/lang/src/attribute_macros/element.rs (1)
Line range hint
42-42: Unused parameterdiagnosticsOhayo, sensei! The
diagnosticsparameter in theparse_membersfunction is now unused due to the removal of the validation logic. To keep the code clean, consider removing this parameter.Apply this diff to remove the unused parameter:
pub fn parse_members( db: &dyn SyntaxGroup, members: &[MemberAst], - diagnostics: &mut Vec<PluginDiagnostic>, ) -> Vec<Member> {
🧹 Nitpick comments (3)
crates/dojo/lang/src/attribute_macros/element.rs (1)
Line range hint
3-3: Remove unused importsOhayo, sensei! The imports
cairo_lang_diagnostics::SeverityandTypedStablePtrare no longer used and can be removed to tidy up the code.Apply this diff to remove the unused imports:
use cairo_lang_defs::patcher::RewriteNode; -use cairo_lang_diagnostics::Severity; use cairo_lang_defs::plugin::PluginDiagnostic; use cairo_lang_syntax::node::ast::Member as MemberAst; use cairo_lang_syntax::node::db::SyntaxGroup; -use cairo_lang_syntax::node::TypedStablePtr; use cairo_lang_syntax::node::helpers::QueryAttrs;Also applies to: 7-7
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
28-37: IncludeFoo3innamespace_def()for proper testingOhayo, sensei! The new model
Foo3with au256key has been defined but is not included in thenamespace_def()function. To ensureFoo3is properly registered and tested, please add it to thenamespace_def()resources.Apply this diff to include
Foo3:fn namespace_def() -> NamespaceDef { NamespaceDef { namespace: "dojo_cairo_test", resources: [ TestResource::Model(m_Foo::TEST_CLASS_HASH.try_into().unwrap()), TestResource::Model(m_Foo2::TEST_CLASS_HASH.try_into().unwrap()), + TestResource::Model(m_Foo3::TEST_CLASS_HASH.try_into().unwrap()), ].span() } }
28-37: Add tests forFoo3to validateu256key functionalityOhayo, sensei! To fully ensure that the
u256key functions correctly, consider adding tests forFoo3similar to the existing tests forFooandFoo2. This will help confirm that the changes meet the desired objectives.Do you want me to help generate the test cases for
Foo3?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/dojo/lang/src/attribute_macros/element.rs
[error] 3-3: Unused import: 'cairo_lang_diagnostics::Severity'
[error] 7-7: Unused import: 'TypedStablePtr'
[error] 42-42: Unused variable: 'diagnostics'. Consider prefixing with underscore if intentional
[error] 42-42: Inefficient argument type: '&mut Vec' should be '&mut [PluginDiagnostic]'
🔇 Additional comments (1)
crates/dojo/lang/src/attribute_macros/element.rs (1)
46-49: Simplification aligns with PR objectivesOhayo, sensei! The changes to the
parse_membersfunction remove the previous validation logic, allowingu256to be used as keys. This aligns perfectly with the PR objective to remove restrictions onu256keys. The simplification enhances code readability.
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
Thanks for the work here @bengineer42.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockcrates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
crates/dojo/core-cairo-test/Scarb.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(2 hunks)crates/dojo/lang/src/attribute_macros/event.rs(1 hunks)crates/dojo/lang/src/attribute_macros/model.rs(1 hunks)scripts/rebuild_test_artifacts.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ensure-windows
- GitHub Check: test
- GitHub Check: test-hurl
- GitHub Check: dojo-spawn-and-move-example-test
- GitHub Check: dojo-core-test
🔇 Additional comments (5)
crates/dojo/core-cairo-test/Scarb.toml (1)
6-6: Ohayo sensei! Verify version bump compatibility.The version change from rc.0 to 1.0.11 is significant. Please ensure this aligns with semantic versioning principles, especially since we're adding support for u256 keys which could be considered a feature addition.
✅ Verification successful
Ohayo sensei! Version bump is safe to proceed! ✨
The version change to 1.0.11 aligns perfectly with other core packages in the ecosystem. Since all internal dependencies use path-based references rather than version constraints, this change won't break any existing integrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this version bump affects other dependent packages rg "dojo_cairo_test.*version" --type tomlLength of output: 43
Script:
#!/bin/bash # First, find all Scarb.toml files fd Scarb.toml # Then, let's see their content to check dependencies fd Scarb.toml --exec cat {} # Also check if there's a changelog fd -i changelogLength of output: 4290
scripts/rebuild_test_artifacts.sh (1)
37-37: Ohayo sensei! The test command addition looks good.The placement of the sozo test command is appropriate in the build sequence, ensuring that the core-cairo-test package (including the new u256 key support) is properly tested before database operations.
Let's verify that the test command works as expected:
✅ Verification successful
Ohayo sensei! Your test command is unique and valid!
The manifest exists and this is the first
sozo testcommand in our shell scripts, making it a pioneer in our testing approach. Keep up the innovative spirit! 🚀🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if sozo test command is used elsewhere for consistency rg "sozo test" --type sh # Verify the manifest path exists test -f crates/dojo/core-cairo-test/Scarb.toml && echo "Manifest exists" || echo "Manifest missing"Length of output: 232
crates/dojo/lang/src/attribute_macros/element.rs (1)
37-44: The simplified parse_members implementation looks good, sensei!The removal of type validation aligns with the PR objective to allow u256 as keys in models. The code is now more permissive and follows the principle of minimal restrictions.
Let's verify that this change doesn't break existing model declarations:
✅ Verification successful
Ohayo! The parse_members changes are safe and well-contained, sensei!
The removal of type validation doesn't affect the core requirement of having at least one #[key] attribute in models and events. The change only removes unnecessary type restrictions, making the system more flexible while maintaining structural integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing model declarations to ensure compatibility ast-grep --pattern 'struct $name { #[key] $field: u256, $$$ }'Length of output: 70
Script:
#!/bin/bash # Search for struct declarations with key attributes using a more permissive pattern echo "=== Searching for structs with key attributes ===" ast-grep --pattern 'struct $name { #[$attr] $field: $_, $$$ }' # Search for any type validation related code echo -e "\n=== Searching for type validation code ===" rg -n "type.*valid" --type rustLength of output: 463
Script:
#!/bin/bash # Search for Cairo model declarations echo "=== Searching for model declarations ===" rg -l "#\[derive\(Model\)]" --type rust # Search for element macro usage echo -e "\n=== Searching for element macro usage ===" rg -n "element!" --type rust -A 2 # Search for key attribute usage in Cairo files echo -e "\n=== Searching for key attributes in Cairo files ===" rg -n "#\[key\]" --type rust -A 1Length of output: 1237
crates/dojo/lang/src/attribute_macros/event.rs (1)
61-61: Clean adaptation of parse_members call, sensei!The change maintains essential validations while removing unnecessary type restrictions.
crates/dojo/lang/src/attribute_macros/model.rs (1)
75-75: Clean adaptation of parse_members call with good validation retention, sensei!The change maintains essential model validations while enabling u256 keys. Consider adding test cases specifically for models with u256 keys to ensure proper serialization and storage.
Let's verify the test coverage:
| use cairo_lang_syntax::node::db::SyntaxGroup; | ||
| use cairo_lang_syntax::node::helpers::QueryAttrs; | ||
| use cairo_lang_syntax::node::{Terminal, TypedStablePtr, TypedSyntaxNode}; | ||
| use cairo_lang_syntax::node::{Terminal, TypedSyntaxNode}; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! The Terminal import is indeed unused, sensei!
The Terminal trait is imported but never used throughout the file. You can safely remove this import.
🔗 Analysis chain
Ohayo! The Terminal import appears to be unused, sensei.
The Terminal trait is imported but not used in the modified code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if Terminal is used in the file
rg "Terminal" crates/dojo/lang/src/attribute_macros/element.rs
Length of output: 120
Script:
#!/bin/bash
# Show the entire file content
echo "=== Full file content ==="
cat crates/dojo/lang/src/attribute_macros/element.rs
# Look for potential indirect usage patterns
echo -e "\n=== Checking for potential Terminal-related patterns ==="
ast-grep --pattern 'impl $_ for $_' crates/dojo/lang/src/attribute_macros/element.rs
Length of output: 3467
3 participants
Description
Allows use of u256 as a key in a dojo model
Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Foo3with modified key field type.sozobinary.Code Changes