Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(lang): add the ability to get a WorldStorage from a namespace hash #2686
base: feat/poseidon-macro
Are you sure you want to change the base?
feat(lang): add the ability to get a WorldStorage from a namespace hash #2686
Changes from 6 commits
b1f3532
5a9fa53
adc5566
a4c9109
a897f4c
21a19b7
3b772ce
ef21c50
8a1f9d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Missing #[test] attribute and consider additional negative cases!
Apply this diff to fix the attribute:
+#[test] fn test_poseidon_hash_string_ne() {
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
new_from_hash
).Having something like:
Will be the most efficient way to query a resource, with no Poseidon hash at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if
poseidon_hash_bytearray
would not be more consistent with the dojo utils functions already usingbytearray
instead of string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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 incrates/dojo/lang/src/semantics/test_data/
. All other test files listed in the macro have their corresponding files present in the test directory.crates/dojo/lang/src/semantics/test_data/poseidon_hash_string
get
,set
,selector_from_tag
etc. 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:
Length of output: 192
Script:
Length of output: 586
Script:
Length of output: 2474
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be removed as not used.