feat(lang): add the ability to get a WorldStorage from a namespace hash #2686
Conversation
WalkthroughOhayo, sensei! This pull request introduces several enhancements to the Dojo codebase, primarily focusing on the Poseidon hash functionality. A new module for testing the Poseidon hash string is added, along with multiple unit tests to validate its behavior. Additionally, new methods are introduced in the Changes
Possibly related PRs
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
crates/dojo/core-cairo-test/src/lib.cairo (1)
41-41: Ohayo sensei! The module addition looks good, but could use some documentation.The placement and naming of the new
poseidon_hash_stringmodule is appropriate. Consider adding a brief doc comment explaining its purpose and relationship to the WorldStorage functionality.Add a doc comment before the module declaration:
+ /// Tests for Poseidon hash string functionality used in WorldStorage namespace operations mod poseidon_hash_string;crates/dojo/lang/src/semantics/tests.rs (1)
25-26: Consider grouping related hash function tests together.Since this test is related to Poseidon hash functionality, consider organizing similar hash-related tests under a common prefix or in a dedicated test module for better maintainability.
Consider restructuring the test cases like this:
test_file_test!( dojo_semantics, "src/plugin/semantics/test_data", { get: "get", set: "set", selector_from_tag: "selector_from_tag", get_models_test_class_hashes: "get_models_test_class_hashes", spawn_test_world: "spawn_test_world", - poseidon_hash_string: "poseidon_hash_string", + // Hash-related tests + hash_poseidon_string: "poseidon_hash_string", + // Add other hash-related tests here }, test_semantics );crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo (1)
23-41: Good boundary testing, but consider additional cases!The tests for 31 characters and longer strings are valuable. Consider adding tests for:
- Maximum allowed string length
- UTF-8 characters
- Special characters
crates/dojo/lang/src/cairo_plugin.rs (1)
28-29: Consider consolidating the plugin registrations, sensei!The current implementation has two separate chain calls for adding inline macro plugins. We can make this more elegant by chaining them together.
Here's a suggested improvement:
- suite.add_plugin::<BuiltinDojoPlugin>().add_inline_macro_plugin::<SelectorFromTagMacro>(); - suite.add_plugin::<BuiltinDojoPlugin>().add_inline_macro_plugin::<PoseidonHashStringMacro>(); + suite + .add_plugin::<BuiltinDojoPlugin>() + .add_inline_macro_plugin::<SelectorFromTagMacro>() + .add_inline_macro_plugin::<PoseidonHashStringMacro>();examples/spawn-and-move/src/actions.cairo (1)
314-324: The test coverage looks good, sensei! Consider expanding it.While the current test effectively verifies the basic functionality, consider adding test cases for:
- Invalid namespace hashes
- Empty namespace
- Special characters in namespace
Here's a suggested expansion of the test cases:
#[test] #[available_gas(30000000)] #[should_panic(expected: ('invalid namespace hash',))] fn test_world_from_hash_invalid() { let ndef = namespace_def(); let mut world = spawn_test_world([ndef].span()); world.sync_perms_and_inits(contract_defs()); let invalid_hash: felt252 = 0; world.dispatcher.new_from_hash(invalid_hash); }crates/dojo/core/src/world/storage.cairo (1)
35-37: Ohayo! The implementation looks clean, but needs documentation, sensei!The new method provides a direct way to create WorldStorage from a hash, which is good! However, we should add documentation to explain its purpose and usage.
Consider adding documentation like this:
+ /// Creates a new WorldStorage instance from an existing namespace hash. + /// This is useful when you already have the hash and want to avoid recomputing it. + /// @param self - The IWorldDispatcher reference + /// @param namespace_hash - The precomputed namespace hash + /// @return WorldStorage instance initialized with the provided dispatcher and hash fn new_from_hash(self: @IWorldDispatcher, namespace_hash: felt252) -> WorldStorage { WorldStorage { dispatcher: *self, namespace_hash } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/contract.patch.cairo(1 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/inline_macros/mod.rs(2 hunks)crates/dojo/lang/src/inline_macros/poseidon_hash_string.rs(1 hunks)crates/dojo/lang/src/semantics/tests.rs(1 hunks)examples/spawn-and-move/src/actions.cairo(2 hunks)
🔇 Additional comments (14)
crates/dojo/lang/src/attribute_macros/patches/contract.patch.cairo (1)
33-35: Ohayo! Implementation looks clean, but needs documentation.
The new method follows good patterns and integrates well with the existing world provider functionality.
Consider adding documentation to explain:
- When to use
world_from_hashvsworld - Expected format/constraints of the namespace_hash parameter
+ /// Gets a WorldStorage instance from a namespace hash
+ /// @param namespace_hash The felt252 hash of the namespace
+ /// @return WorldStorage instance for the given namespace
fn world_from_hash(self: @ContractState, namespace_hash: felt252) -> dojo::world::storage::WorldStorage {Let's verify the WorldStorageTrait::new_from_hash implementation exists:
✅ Verification successful
The previous ast-grep pattern might have been too specific. Let's try these broader searches to find the implementation and trait definition.
Ohayo! The implementation is verified and correct, sensei!
The search results confirm that new_from_hash is properly implemented in WorldStorageTrait at crates/dojo/core/src/world/storage.cairo. The implementation matches exactly what's being used in the contract:
fn new_from_hash(self: @IWorldDispatcher, namespace_hash: felt252) -> WorldStorage {
WorldStorage { dispatcher: *self, namespace_hash }
}The method is a simple constructor that takes a namespace hash directly instead of computing it from a ByteArray, which aligns perfectly with the contract's usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for new_from_hash implementation in WorldStorageTrait
ast-grep --pattern 'impl $_ of WorldStorageTrait {
$$$
fn new_from_hash($_) {
$$$
}
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Let's try a broader search for WorldStorageTrait and new_from_hash
rg "WorldStorageTrait" -A 5 -B 5
# Also search specifically for new_from_hash function
rg "fn new_from_hash" -A 5 -B 5
# And let's check trait definition
rg "trait WorldStorageTrait" -A 10
Length of output: 7877
crates/dojo/core-cairo-test/src/lib.cairo (1)
41-41: Verify the implementation of the new test module.
Let's ensure the test module is properly implemented with comprehensive test cases.
✅ Verification successful
Let me gather more information about the test module implementation.
Test module implementation is well-covered, sensei! ✨
The poseidon_hash_string test module has comprehensive test coverage with 6 test cases:
- Basic string hashing test
- Empty string test
- 31-character string test
- Long string test (>31 chars)
- Variable input test
- Inequality test
Each test properly verifies the hash computation by comparing against the expected result using the poseidon_hash_span function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the poseidon_hash_string test module
# Check the test module implementation
rg -A 10 "mod poseidon_hash_string"
# Look for test functions in the module
ast-grep --pattern 'fn test_$_($$$) {
$$$
}'
Length of output: 1864
Script:
#!/bin/bash
# Check for the actual implementation of poseidon_hash_string test module
fd poseidon_hash_string.cairo --exec cat {}
# Look for any test functions in files containing poseidon_hash_string
rg -l "poseidon_hash_string" | xargs rg "fn test_" -A 5
Length of output: 8070
crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo (3)
1-3: Ohayo sensei! The imports look good!
The necessary poseidon hash functionality is properly imported.
4-12: Clean and focused test implementation!
The test effectively validates the basic hash functionality for a simple string.
13-21: Excellent edge case coverage, sensei!
Testing empty string input is crucial for robust hash functionality.
crates/dojo/lang/src/inline_macros/mod.rs (2)
15-15: Ohayo sensei! The module declaration looks good!
The new poseidon_hash_string module declaration follows the established pattern and maintains proper alphabetical ordering.
25-25: The export declaration is well-structured, sensei!
The PoseidonHashStringMacro export follows the established pattern and maintains proper alphabetical ordering.
Let's verify the macro implementation:
✅ Verification successful
The macro implementation is complete and well-structured, sensei!
The PoseidonHashStringMacro correctly:
- Implements the required
InlineMacroExprPlugintrait - Has a proper
generate_codemethod with expected signature - Includes input validation and error handling
- Generates appropriate code using
PatchBuilder
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the PoseidonHashStringMacro implementation
# Check if the macro implements required traits
ast-grep --pattern 'impl InlineMacroExprPlugin for PoseidonHashStringMacro'
# Check if the macro has the generate_code method
ast-grep --pattern 'fn generate_code(&self, $_, $_, $_) -> InlinePluginResult'
Length of output: 4479
crates/dojo/lang/src/cairo_plugin.rs (1)
15-15: Ohayo! Clean import addition, sensei!
The import follows the existing pattern and maintains consistency with the codebase.
examples/spawn-and-move/src/actions.cairo (2)
216-216: Import looks good, sensei!
The WorldStorageTrait import is appropriately placed and necessary for the new hash-based world storage functionality.
207-209: Ohayo sensei! The implementation looks solid!
The new world_default_from_hash method provides a clean way to access world storage using a hashed namespace, maintaining consistency with the existing pattern while adding new functionality.
Let's verify that the hash is used consistently across the codebase:
crates/dojo/core/src/world/storage.cairo (2)
35-37: Consider validating the namespace_hash parameter
The method accepts any felt252 value as namespace_hash without validation.
Let's check if there are any validation patterns in existing code:
✅ Verification successful
Let me check if there's any validation being done for namespace_hash in the world contract:
Let me check one more thing about namespace registration:
Ohayo sensei! After reviewing the codebase, I can now provide a clear response about the namespace_hash validation:
The namespace_hash validation is already handled at registration time
The WorldStorage's new_from_hash method is safe to use as-is because:
- Namespaces must be registered first via
register_namespace - Registration validates the namespace name format using
is_name_valid - Registration prevents duplicate registrations
- The World contract maintains the mapping of valid namespace_hashes
The hash parameter in new_from_hash is meant to be used with pre-validated namespace hashes that were obtained through the proper registration flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for namespace_hash validation patterns
# Expected: Find examples of hash validation
rg -B 3 -A 3 "namespace_hash.*felt252" "crates/dojo"
Length of output: 31859
Script:
#!/bin/bash
# Look for namespace validation in world contract implementation
ast-grep --pattern 'fn is_namespace_registered(self: @ContractState, namespace_hash: felt252) -> bool {
$$$
}'
# Also check for any other validation patterns
rg -B 3 -A 3 "namespace.*valid" "crates/dojo"
Length of output: 2479
Script:
#!/bin/bash
# Look for namespace registration implementation
rg -B 5 -A 5 "register_namespace" "crates/dojo"
Length of output: 16364
35-37: Verify test coverage for the new method
While the implementation is straightforward, we should ensure it's properly tested.
Let's check for test coverage:
✅ Verification successful
The previous search failed because we were looking in a specific directory that didn't exist. Let's broaden our search to find where tests might be located and any usage of the method.
Let's check the test directory that seems most relevant to core functionality and also look for inline tests in the implementation file itself, sensei!
Ohayo sensei! Let's take a look at the actual file content and check for any test files in the world module directory that might have been missed by our previous searches!
Test coverage exists in test configuration
Ohayo sensei! Looking at the code, I can see that the new_from_hash method is actually covered by the test configuration. The file includes a #[cfg(target: "test")] section with comprehensive test implementations for EventStorageTest, ModelStorageTest, and ModelValueStorageTest traits that all rely on the WorldStorage struct and its initialization methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for test cases covering new_from_hash
# Expected: Find test files containing new_from_hash usage
rg -l "new_from_hash" "crates/dojo/core/src/world/tests"
Length of output: 127
Script:
#!/bin/bash
# Let's first find where the tests might be located
fd "test" --type d
# Then search for any test files that might reference new_from_hash
rg -l "new_from_hash" --type rust
Length of output: 921
Script:
#!/bin/bash
# Let's look in the core-cairo-test directory since it seems most relevant
rg "new_from_hash" crates/dojo/core-cairo-test/ -A 5 -B 5
# Also check the actual implementation file for any existing test modules
rg "mod test" crates/dojo/core/src/world/storage.cairo
Length of output: 114
Script:
#!/bin/bash
# Let's check the content of the storage.cairo file to see its structure
cat crates/dojo/core/src/world/storage.cairo
# Also look for any test files in the world module directory
fd "test" crates/dojo/core/src/world/
Length of output: 18121
crates/dojo/lang/src/inline_macros/poseidon_hash_string.rs (2)
36-36:
Ohayo, sensei! Escape double quotes in the error message.
In line 36, the double quotes inside the string need to be escaped to prevent a compile-time error.
Apply this diff to fix the issue:
- message: "Invalid arguments. Expected \"poseidon_hash_string!(\"tag\")\"".to_string(),
+ message: "Invalid arguments. Expected \"poseidon_hash_string!(\\\"tag\\\")\"".to_string(),Likely invalid or redundant comment.
48-48: Ohayo, sensei! Verify the format specifier in {:#64x}.
The format specifier {:#64x} might not produce a fixed-width hexadecimal representation as intended.
Run the following script to test the formatting:
Ensure the output aligns with the expected format of a 64-character hexadecimal string.
✅ Verification successful
Ohayo, sensei! The format specifier {:#64x} is correct and consistent with codebase usage.
Looking at the codebase, there are numerous examples of using similar format specifiers for hexadecimal formatting of selectors and other values. The # flag ensures the 0x prefix, and 64 width ensures consistent padding. This is particularly evident in:
crates/dojo/lang/src/inline_macros/poseidon_hash_string.rs: Uses{:#64x}for selector formattingcrates/dojo/lang/src/inline_macros/selector_from_tag.rs: Uses the same format specifier- Other parts of the codebase use similar patterns like
{:#066x}for consistent hex formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the output of the format specifier.
# Test: Check how the selector is formatted.
echo "Testing format specifier output:"
rustc -e 'let selector: u128 = 0x123456789ABCDEF; println!("{:#64x}", selector);'
Length of output: 231
Script:
#!/bin/bash
# Let's examine the actual code usage and context
rg -A 5 -B 5 "format!\(.*#.*x.*selector" --type rust
# Also check other similar format usages in the codebase
rg -A 2 -B 2 "format!\(.*#.*x" --type rust
# Look for tests or examples that might show the expected format
rg -A 5 -B 5 "selector.*format.*x" --type rust
Length of output: 118736
|
|
||
| poseidon_hash_string: "poseidon_hash_string", |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Missing test file for poseidon_hash_string
The test case poseidon_hash_string is added to the test macro but the corresponding test file is missing in crates/dojo/lang/src/semantics/test_data/. All other test files listed in the macro have their corresponding files present in the test directory.
- Missing test file:
crates/dojo/lang/src/semantics/test_data/poseidon_hash_string - Other test files like
get,set,selector_from_tagetc. are properly present
🔗 Analysis chain
Ohayo sensei! Verify the test file existence and structure.
The new test case poseidon_hash_string has been added to the test suite, but we should verify that the corresponding test file exists at the expected location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of the poseidon_hash_string test file
# Check if the test file exists
fd "poseidon_hash_string" "src/plugin/semantics/test_data"
# If found, examine its content for proper test structure
if [ $? -eq 0 ]; then
rg -A 10 "test|assert" "src/plugin/semantics/test_data/poseidon_hash_string.cairo"
fi
Length of output: 192
Script:
#!/bin/bash
# Let's first find the correct test data directory structure
fd -t d "test_data"
# Then search for the poseidon test file in the correct location
fd "poseidon_hash_string" .
# Let's also check the test file structure in semantics/tests.rs to understand the pattern
rg -A 5 "test_file_test!" "crates/dojo/lang/src/semantics/tests.rs"
Length of output: 586
Script:
#!/bin/bash
# Let's check if the test file exists in the correct location
fd "poseidon_hash_string" "crates/dojo/lang/src/semantics/test_data"
# Let's also check the content of the test macro to understand the full context
cat "crates/dojo/lang/src/semantics/tests.rs"
# And examine the actual test files that exist
ls -la "crates/dojo/lang/src/semantics/test_data" 2>/dev/null || echo "Directory not found"
Length of output: 2474
| fn test_poseidon_hash_string_ne() { | ||
| let bytes: ByteArray = "foo"; | ||
| let hash = poseidon_hash_string!("bar"); | ||
| let mut array = array![]; | ||
| bytes.serialize(ref array); | ||
| let computed = poseidon_hash_span(array.span()); | ||
| assert_ne!(computed, hash); | ||
| } |
There was a problem hiding this comment.
Critical: Missing #[test] attribute and consider additional negative cases!
- The test won't be executed without the test attribute.
- Consider adding more negative tests:
- Similar strings (e.g., "foo" vs "fooo")
- Case variations (e.g., "foo" vs "FOO")
- Different string lengths with same content
Apply this diff to fix the attribute:
+#[test]
fn test_poseidon_hash_string_ne() {📝 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.
| fn test_poseidon_hash_string_ne() { | |
| let bytes: ByteArray = "foo"; | |
| let hash = poseidon_hash_string!("bar"); | |
| let mut array = array![]; | |
| bytes.serialize(ref array); | |
| let computed = poseidon_hash_span(array.span()); | |
| assert_ne!(computed, hash); | |
| } | |
| #[test] | |
| fn test_poseidon_hash_string_ne() { | |
| let bytes: ByteArray = "foo"; | |
| let hash = poseidon_hash_string!("bar"); | |
| let mut array = array![]; | |
| bytes.serialize(ref array); | |
| let computed = poseidon_hash_span(array.span()); | |
| assert_ne!(computed, hash); | |
| } |
| fn test_poseidon_hash_string_var() { | ||
| let bytes: ByteArray = "foo"; | ||
| let bytes2: ByteArray = "foo"; | ||
| let hash = poseidon_hash_string!(bytes); | ||
| let mut array = array![]; | ||
| bytes2.serialize(ref array); | ||
| let computed = poseidon_hash_span(array.span()); | ||
| assert_eq!(computed, hash); | ||
| } |
There was a problem hiding this comment.
Critical: Missing #[test] attribute!
The test won't be executed without the test attribute.
Apply this diff to fix:
+#[test]
fn test_poseidon_hash_string_var() {📝 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.
| fn test_poseidon_hash_string_var() { | |
| let bytes: ByteArray = "foo"; | |
| let bytes2: ByteArray = "foo"; | |
| let hash = poseidon_hash_string!(bytes); | |
| let mut array = array![]; | |
| bytes2.serialize(ref array); | |
| let computed = poseidon_hash_span(array.span()); | |
| assert_eq!(computed, hash); | |
| } | |
| #[test] | |
| fn test_poseidon_hash_string_var() { | |
| let bytes: ByteArray = "foo"; | |
| let bytes2: ByteArray = "foo"; | |
| let hash = poseidon_hash_string!(bytes); | |
| let mut array = array![]; | |
| bytes2.serialize(ref array); | |
| let computed = poseidon_hash_span(array.span()); | |
| assert_eq!(computed, hash); | |
| } |
| }; | ||
| } | ||
|
|
||
| let tag = &args[0].as_syntax_node().get_text(db).replace('\"', ""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Properly handle string literals for the tag.
Using replace('\"', "") may not correctly handle all cases, especially if the tag contains escaped quotes or special characters.
Consider parsing the string literal to accurately extract the tag value, handling any escaped characters appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo (2)
4-12: Add documentation and use const for test dataThe test logic is solid, but consider these improvements for better maintainability:
+/// Tests that poseidon_hash_string! macro produces the same result as manual serialization +/// and hashing for a basic string. +const TEST_STRING: &str = "foo"; #[test] fn test_poseidon_hash_string() { - let bytes: ByteArray = "foo"; - let hash = poseidon_hash_string!("foo"); + let bytes: ByteArray = TEST_STRING; + let hash = poseidon_hash_string!(TEST_STRING); let mut array = array![]; bytes.serialize(ref array); let computed = poseidon_hash_span(array.span()); assert_eq!(computed, hash); }
13-21: Add documentation for edge case testGood edge case testing! Consider adding explicit documentation:
+/// Tests the hash computation for an empty string, which is an important edge case +/// that could potentially cause issues in string handling. #[test] fn test_poseidon_hash_string_empty() {examples/spawn-and-move/src/actions.cairo (2)
207-209: Ohayo sensei! Consider documenting the relationship withworld_defaultThe implementation looks correct, but it would be helpful to document how this method relates to
world_defaultand when to use each approach.Add a doc comment explaining the purpose:
+ /// Alternative to `world_default` that uses the hashed namespace directly. + /// This is equivalent to `world_default` but skips the string-to-hash conversion. fn world_default_from_hash(self: @ContractState) -> dojo::world::WorldStorage {
314-324: Ohayo sensei! The test coverage looks solidThe test effectively verifies that the world storage created from a hash matches the expected properties. However, we could enhance it by testing the equivalence with
world_default.Consider adding this assertion to verify equivalence:
assert_eq!(storage.namespace_hash, world.namespace_hash); assert_eq!(storage.dispatcher.contract_address, world.dispatcher.contract_address); + // Verify equivalence with world_default + let (actions_system_addr, _) = world.dns(@"actions").unwrap(); + let actions_system = IActionsDispatcher { contract_address: actions_system_addr }; + let default_storage = actions_system.world_default(); + assert_eq!(storage.namespace_hash, default_storage.namespace_hash);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/contract.patch.cairo(1 hunks)examples/spawn-and-move/src/actions.cairo(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo/core/src/world/storage.cairo
- crates/dojo/lang/src/attribute_macros/patches/contract.patch.cairo
🔇 Additional comments (2)
crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo (2)
44-51:
Critical: Fix incorrect test data in inequality check
Ohayo sensei! There's a critical issue in this test. The ByteArray is created with "foo" but compared against the hash of "bar". This makes the test meaningless as it's comparing different things.
Fix the test data consistency:
#[test]
fn test_poseidon_hash_string_ne() {
- let bytes: ByteArray = "foo";
+ let bytes: ByteArray = "bar";
let hash = poseidon_hash_string!("bar");
let mut array = array![];
bytes.serialize(ref array);
let computed = poseidon_hash_span(array.span());
assert_ne!(computed, hash);
}Additionally, consider adding more inequality test cases:
- Similar strings (e.g., "foo" vs "fooo")
- Case variations (e.g., "foo" vs "FOO")
Likely invalid or redundant comment.
23-41: Clarify the significance of 31 characters and add boundary tests
The test cases for 31 characters and longer strings are good, but:
- Why is 31 characters significant? Consider adding a comment explaining this.
- Consider adding more boundary tests:
- 30 characters (just below 31)
- 32 characters (just above 31)
- Maximum allowed length
Let's check if there are any length constraints in the implementation:
crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo
Show resolved
Hide resolved
| fn new_from_hash(world: IWorldDispatcher, namespace_hash: felt252) -> WorldStorage { | ||
| WorldStorage { dispatcher: world, namespace_hash } | ||
| } | ||
|
|
There was a problem hiding this comment.
If we add this to save the Poseidon hashes on the serialized byte array, we should also add one for the dns. Cairo doesn't support function overloading.
Since computing the dojo selector requires three steps:
- Hashing the namespace (saved by the
new_from_hash). - Hashing the name.
- Hashing them together.
Having something like:
let (ca, ch) = world.dns_from_hash/selector(selector_from_tag!("eternum-map_system")).unwrap();Will be the most efficient way to query a resource, with no Poseidon hash at all.
| pub struct PoseidonHashStringMacro; | ||
|
|
||
| impl NamedPlugin for PoseidonHashStringMacro { | ||
| const NAME: &'static str = "poseidon_hash_string"; |
There was a problem hiding this comment.
| const NAME: &'static str = "poseidon_hash_string"; | |
| const NAME: &'static str = "poseidon_hash_string"; |
Wondering if poseidon_hash_bytearray would not be more consistent with the dojo utils functions already using bytearray instead of string.
| fn test_world_from_hash() { | ||
| let ndef = namespace_def(); | ||
| let mut world = spawn_test_world([ndef].span()); | ||
| world.sync_perms_and_inits(contract_defs()); |
There was a problem hiding this comment.
May be removed as not used.
| world.sync_perms_and_inits(contract_defs()); |
glihm
changed the title
|
@bengineer42 let me know if you don't have bandwidth I'll wrap it up. 🫡 |
|
Sorry didn't see you had reviewed will look into it today |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/core/src/world/storage.cairo (2)
36-39: Consider adding hash validation.The new constructor looks clean, sensei! However, consider adding validation for the namespace_hash parameter to ensure it's a valid Poseidon hash value.
fn new_from_hash(world: IWorldDispatcher, namespace_hash: felt252) -> WorldStorage { + // Validate that namespace_hash is a valid Poseidon hash + assert(namespace_hash != 0, 'Invalid namespace hash'); WorldStorage { dispatcher: world, namespace_hash } }
49-55: Consider caching the selector computation.The selector computation in
dns_from_hashcould be cached to optimize repeated lookups for the same namespace and contract name combination.fn dns_from_hash( self: @WorldStorage, contract_name_hash: felt252 ) -> Option<(ContractAddress, ClassHash)> { + // Consider adding a cached_selector field to WorldStorage + // and checking/updating it here Self::dns_from_selector( self, poseidon_hash_span([*self.namespace_hash, contract_name_hash].span()) ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/core/src/world/storage.cairo(2 hunks)
🔇 Additional comments (3)
crates/dojo/core/src/world/storage.cairo (3)
4-4: LGTM! Import addition is well-placed.
Ohayo! The addition of poseidon_hash_span import aligns perfectly with the new hash-based functionality.
44-47: LGTM! Clean refactor of DNS lookup.
The modification to use dns_from_hash improves code reusability while maintaining functionality.
57-65: LGTM! Error handling is consistent.
The implementation of dns_from_selector follows the codebase's error handling patterns with proper use of Option type.
Description
Added the ability to get a WorldStorage from a namespace hash
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
Release Notes
New Features
poseidon_hash_stringmodule for enhanced testing capabilities related to the Poseidon hash function.WorldStorageinstances from a hash.PoseidonHashStringMacrointo the Dojo plugin suite for improved code generation.Bug Fixes
poseidon_hash_stringfunction to ensure correctness.Documentation
poseidon_hash_stringfunctionality.world_default_from_hashmethod to validate its functionality.