feat(dojo-lang): add bytearray_hash macro
#2946
Conversation
WalkthroughOhayo, sensei! This pull request introduces a comprehensive enhancement to the Dojo framework's testing and hashing capabilities. The changes span multiple crates, adding a new Changes
Possibly related PRs
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: 0
🧹 Nitpick comments (2)
crates/dojo/lang/src/inline_macros/poseidon_hash_string.rs (1)
18-62: Robust macro code generationOhayo, sensei! The
generate_codefunction effectively handles the macro logic:
- Validates that exactly one string argument is provided.
- Returns a diagnostic error if the argument count is incorrect.
- Computes the byte array hash using
naming::compute_bytearray_hash.- Builds the output code with proper hexadecimal formatting.
However, consider enhancing the string extraction for better robustness.
Suggestion: Improve string extraction to handle edge cases
In line 43, using
replace('\"', "")may not handle cases where the string contains escaped quotes or special characters. It's safer to parse the string literal properly.Apply this diff to enhance string extraction:
-let tag = &args[0].as_syntax_node().get_text(db).replace('\"', ""); +let ast::Expr::String(ref string_literal) = args[0] else { + return InlinePluginResult { + code: None, + diagnostics: vec![PluginDiagnostic { + stable_ptr: args[0].stable_ptr().untyped(), + message: "Expected a string literal.".to_string(), + severity: Severity::Error, + }], + }; +}; +let tag = string_literal.string_value(db);crates/dojo/core/src/world/storage.cairo (1)
49-65: Consider adding documentation for the new methods.The implementation looks good, but it would be helpful to add documentation explaining:
- The purpose of each method
- The relationship between namespace_hash, contract_name_hash, and the final selector
- The expected format and constraints of the input hashes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
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(2 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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (13)
crates/dojo/lang/src/inline_macros/poseidon_hash_string.rs (2)
12-12: Well-defined macro structOhayo, sensei! The
PoseidonHashStringMacrostruct is properly defined and sets the stage for the macro implementation.
14-16: Correct implementation of theNamedPlugintraitOhayo, sensei! Implementing the
NamedPlugintrait with the correctNAMEensures the macro is correctly registered as"poseidon_hash_string".crates/dojo/lang/src/attribute_macros/patches/contract.patch.cairo (1)
33-35: Addition ofworld_from_hashmethod looks goodOhayo, sensei! The new method
world_from_hashcorrectly allows retrieval ofWorldStorageusing a namespace hash. This enhances flexibility when working with hashed namespaces.crates/dojo/core-cairo-test/src/tests/expanded/poseidon_hash_string.cairo (5)
4-12: Comprehensive test forposeidon_hash_stringwith "foo"Ohayo, sensei! The test verifies that the computed hash matches the macro's output, ensuring correctness for a standard string input.
13-21: Effective test for empty string edge caseOhayo, sensei! Including a test for an empty string ensures the macro handles this edge case properly. Great attention to detail!
23-31: Boundary condition test with 31-character stringOhayo, sensei! Testing with a 31-character string effectively checks boundary conditions, confirming the macro's robustness.
33-41: Robustness test with a longer stringOhayo, sensei! Testing with a longer string like
"0123456789012345678901234567890foo"ensures the macro accurately handles strings beyond typical lengths.
43-51: Negative test case confirms hash inequalityOhayo, sensei! Verifying that different input strings produce different hashes strengthens the validity of the hashing function.
crates/dojo/core-cairo-test/src/lib.cairo (1)
41-41: LGTM!Ohayo, sensei! The new module follows the existing pattern and is correctly placed within the expanded tests section.
crates/dojo/lang/src/inline_macros/mod.rs (1)
15-15: LGTM!Ohayo, sensei! The new module and its export follow the established patterns and are correctly organized within the inline macros module.
Also applies to: 25-25
crates/dojo/lang/src/cairo_plugin.rs (1)
15-15: LGTM!Ohayo, sensei! The new macro is properly imported and registered in the plugin suite.
Also applies to: 29-29
crates/dojo/core/src/world/storage.cairo (2)
4-4: LGTM!Ohayo, sensei! The import and new constructor method are well-implemented.
Also applies to: 36-38
44-47: LGTM!Clean refactor of the dns method to use the new hash-based functionality.
glihm
changed the title
glihm
changed the title
bytearray_hash macro and related tests
There was a problem hiding this comment.
Thank you @bengineer42 for the contribution, renamed a bit for more consistency and fixed the scarb issue.
| .add_plugin::<BuiltinDojoPlugin>() | ||
| .add_inline_macro_plugin::<SelectorFromTagMacro>() | ||
| .add_inline_macro_plugin::<BytearrayHashMacro>(); |
There was a problem hiding this comment.
@bengineer42 fyi the issue was that you were trying to add 2 times the BuiltinDojoPlugin. This was causing the weird scarb issue on events, which is not related at all. But this was the reason.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/dojo/core-cairo-test/src/tests/expanded/bytearray_hash.cairo (1)
4-11: Consider a helper to reduce duplicated serialization in tests.Most of these tests repeat the
serialize→poseidon_hash_span→assert_eq!sequence. A helper function could reduce code repetition and improve readability.- let mut array = array![]; - bytes.serialize(ref array); - let computed = poseidon_hash_span(array.span()); - assert_eq!(computed, hash); + call_assert_hash(bytes, hash); // Suppose you define a helper functioncrates/dojo/core/src/world/storage.cairo (2)
47-53: Concise hash-based DNS lookup method.Logging or error tracking on failure might help debugging, but the current approach is acceptable.
55-58: Selector-based resolution is efficient.The internal match pattern is clean. If you anticipate new resource types, consider factoring out resource validation into a dedicated function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/expanded/bytearray_hash.cairo(1 hunks)crates/dojo/core/src/world/storage.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-cairo-test/src/lib.cairo
- examples/spawn-and-move/src/actions.cairo
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: build
- GitHub Check: docs
🔇 Additional comments (7)
crates/dojo/core-cairo-test/src/tests/expanded/bytearray_hash.cairo (5)
1-2: Ohayo sensei! All set up for hashing.These imports look correct. Poseidon-based hashing is consistent with the Dojo ecosystem.
14-21: Test coverage of empty case is commendable.Verifying boundary conditions like empty strings ensures robust hashing.
24-31: Good job including length-boundary tests.Ensuring the function handles exactly 31 characters addresses a common off-by-one pitfall.
34-41: Thorough test for longer strings.This scenario guards against performance or memory issues when hashing bigger data.
44-51: Nicely done verifying inequality.A negative test confirming that different inputs yield different hashes is crucial.
crates/dojo/core/src/world/storage.cairo (2)
35-37: Straightforward constructor.This is consistent with the existing
newconstructor. Consider adding inline documentation if additional validation or usage constraints arise later.
44-45: Improved clarity by delegating todns_from_hash.This encapsulation keeps
dnssuccinct, making it easier to maintain.
glihm
changed the title
bytearray_hash macro and related testsbytearray_hash macro
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2946 +/- ##
==========================================
- Coverage 55.91% 55.86% -0.06%
==========================================
Files 446 447 +1
Lines 57814 57859 +45
==========================================
- Hits 32325 32321 -4
- Misses 25489 25538 +49 ☔ View full report in Codecov by Sentry. |
Description
Replace #2686
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
Improvements
WorldStorageinstances using namespace hashesTesting
Dependency Updates