feat: add macros and core-test crates #3174
Conversation
WalkthroughOhayo sensei! This update introduces a comprehensive test infrastructure and macro system for the Dojo framework. It adds new workspace dependencies, including Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant MacroSystem
participant DojoWorld
participant Contract
participant Event
participant Model
TestRunner->>MacroSystem: Expand #[dojo::model], #[dojo::event], etc.
MacroSystem->>Contract: Generate code (storage, events, traits)
MacroSystem->>Model: Generate introspection, serialization logic
MacroSystem->>Event: Generate event traits, serialization
TestRunner->>DojoWorld: Deploy world with resources (models, contracts, events)
TestRunner->>Contract: Deploy contract with init calldata
TestRunner->>DojoWorld: Register resource (model/event/contract)
TestRunner->>DojoWorld: Grant permissions (owner/writer)
TestRunner->>DojoWorld: Execute test (write/read/upgrade resource)
DojoWorld->>Contract: Call contract method (e.g., upgrade)
DojoWorld->>Event: Emit event
DojoWorld->>Model: Store/retrieve model data
TestRunner->>DojoWorld: Assert state, events, permissions
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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: 23
🧹 Nitpick comments (46)
crates/dojo/macros/Scarb.toml (1)
6-7: Consider documenting the empty cairo-plugin section, senseiThe
[cairo-plugin]section is defined but doesn't contain any configuration options. Consider adding a comment explaining why it's empty or what configuration might be added in the future.[cairo-plugin] +# No additional configuration needed for this plugincrates/dojo/macros/src/lib.rs (1)
1-5: Consider adding documentation for modules, senseiAdding documentation comments explaining the purpose of each module would enhance maintainability and help other developers understand the crate structure.
+//! Dojo macros crate providing procedural and attribute macros for the Dojo framework. +/// Attribute macros like `dojo::model`, `dojo::event`, `dojo::contract`, and `dojo::library` mod attributes; +/// Constants used throughout the macros crate mod constants; +/// Derive macros like `introspect` and `introspect_packed` mod derives; +/// Helper utilities for macro implementation mod helpers; +/// Inline procedural macros like `bytearray_hash` and `selector_from_tag` mod inlines;Cargo.toml (2)
157-157: Consider specifying a more precise version constraint, senseiThe version constraint for
cairo-lang-primitive-tokenis quite loose (just "1"), which could lead to compatibility issues if there are breaking changes within major version 1.-cairo-lang-primitive-token = "1" +cairo-lang-primitive-token = "1.0.0"
159-160: Add a follow-up task for the git dependency, senseiThe comment explains why a git dependency is used, but consider creating a follow-up task with a specific timeline to replace it with a published crate once available.
# refer to Scarb git until `cairo-lang-macro` 0.2.0 (new api) is deployed on crates.io cairo-lang-macro = { version = "0.2.0", git = "https://github.com/software-mansion/scarb.git" } +# TODO(DOJO-123): Replace git dependency with crates.io version when 0.2.0 is publishedcrates/dojo/core-test/src/tests/utils/key.cairo (1)
1-17: Consider adding more test cases, senseiWhile the current tests verify basic functionality, consider adding more test cases with different input values and edge cases (like empty spans or large values) to ensure robust behavior.
#[test] fn test_entity_id_from_keys() { let keys = [1, 2, 3].span(); assert( entity_id_from_serialized_keys(keys) == core::poseidon::poseidon_hash_span(keys), 'bad entity ID', ); + + // Test with empty span + let empty_keys = [].span(); + assert( + entity_id_from_serialized_keys(empty_keys) == core::poseidon::poseidon_hash_span(empty_keys), + 'empty keys entity ID error', + ); + + // Test with large values + let large_keys = [u256_from_felt252(1), u256_from_felt252(2)].span(); + assert( + entity_id_from_serialized_keys(large_keys) == core::poseidon::poseidon_hash_span(large_keys), + 'large values entity ID error', + ); }crates/dojo/macros/src/inlines/mod.rs (2)
6-9: Consider adding documentation for the bytearray_hash macro, sensei!While the implementation is clean, adding a brief doc comment explaining what this macro does and how it should be used would improve developer experience.
+/// Computes a Poseidon hash of a byte array at compile time. +/// +/// This macro takes a byte array literal and returns its hash as a felt252 constant. #[inline_macro] pub fn bytearray_hash(token_stream: TokenStream) -> ProcMacroResult { bytearray_hash::process(token_stream) }
11-14: Documentation would help users understand this macro's purpose too!Similar to the previous macro, adding a brief explanation of what this selector macro does would be helpful.
+/// Computes a selector from a tag string at compile time. +/// +/// This macro takes a tag string and returns its selector value as a felt252 constant. #[inline_macro] pub fn selector_from_tag(token_stream: TokenStream) -> ProcMacroResult { selector_from_tag::process(token_stream) }crates/dojo/core-test/src/tests/utils/naming.cairo (1)
11-16: Consider adding more edge cases for invalid names.While the current invalid test cases are good, adding tests for empty strings, very long names, names with only numbers, and names with unicode characters would improve coverage.
#[test] fn test_with_invalid_names() { assert!(!is_name_valid(@"n@me")); assert!(!is_name_valid(@"Name ")); assert!(!is_name_valid(@"-name")); + assert!(!is_name_valid(@"")); + assert!(!is_name_valid(@"123")); + // Test with a very long name (if there's a length restriction) + assert!(!is_name_valid(@"abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz")); + // Test with unicode characters (if ASCII-only is enforced) + assert!(!is_name_valid(@"名前")); }crates/dojo/macros/src/helpers/mod.rs (1)
1-7: Ohayo, sensei! The Member struct would benefit from more documentation.While the struct is well-defined, adding more detailed documentation about each field would improve clarity, especially for the
keyboolean field whose purpose isn't immediately clear.-/// Represents a member of a struct. +/// Represents a member of a struct in a model or event declaration. +/// +/// # Fields +/// +/// * `name` - The name of the struct member +/// * `ty` - The type of the struct member as a string +/// * `key` - Whether this member is marked as a key field, used for indexing in storage #[derive(Clone, Debug, PartialEq)] pub struct Member { pub name: String, pub ty: String, pub key: bool, }crates/dojo/core-test/src/tests/expanded/bytearray_hash.cairo (1)
43-51: Important differential test verifying hash uniqueness.This test confirms that different input strings produce different hash outputs, an essential property for any hashing function.
Sensei, consider extracting the common serialization and hashing logic into a helper function to reduce duplication across tests:
+fn compute_bytearray_hash(bytes: ByteArray) -> felt252 { + let mut array = array![]; + bytes.serialize(ref array); + poseidon_hash_span(array.span()) +} #[test] fn test_bytearray_hash() { let bytes: ByteArray = "foo"; let hash = bytearray_hash!("foo"); - let mut array = array![]; - bytes.serialize(ref array); - let computed = poseidon_hash_span(array.span()); + let computed = compute_bytearray_hash(bytes); assert_eq!(computed, hash); }crates/dojo/macros/src/helpers/debug.rs (1)
11-33: Effective macro debugging implementation, sensei!The implementation properly checks for both global and element-specific debug environment variables, formats the output with clear delimiters, and provides human-readable diagnostics when present.
Consider adding a verbosity parameter to control the detail level of the output:
-pub fn debug_macro(element: &str, res: &ProcMacroResult) { +pub fn debug_macro(element: &str, res: &ProcMacroResult, verbosity: Option<u8>) { let element = element.to_ascii_uppercase(); + let verbosity = verbosity.unwrap_or(1); if std::env::var("DOJO_DEBUG_MACRO").is_ok() || std::env::var(format!("DOJO_DEBUG_{element}_MACRO")).is_ok() { - let content = format!("content:\n{}", res.token_stream); + let content = if verbosity > 0 { + format!("content:\n{}", res.token_stream) + } else { + "".to_string() + }; let diagnostics = if res.diagnostics.is_empty() { "".to_string() } else { format!( "diagnostics:\n{}", res.diagnostics .iter() - .map(|d| d.to_pretty_string()) + .map(|d| if verbosity > 1 { d.to_pretty_string() } else { d.to_string() }) .collect::<Vec<_>>() .join("\n") ) };crates/dojo/macros/src/derives/introspect/generics.rs (1)
31-36: Potential assumption about naming convention.The code assumes that for each generic type
T, there exists a correspondingTIntrospectimplementor. This convention should be documented to ensure users of the macro understand this requirement.Consider adding a comment explaining the naming convention assumed by this code, such as:
let generic_impls = generic_types + // For each generic type T, we expect a corresponding TIntrospect implementation .iter() .map(|g| format!("{g}, impl {g}Introspect: dojo::meta::introspect::Introspect<{g}>")) .collect::<Vec<_>>() .join(", ");crates/dojo/core-test/src/utils.cairo (1)
1-35: Gas counter implementation looks great, sensei!The
GasCounterprovides a helpful utility for measuring gas consumption in tests. The implementation cleanly handles starting, ending, and formatting gas measurements.One small optimization opportunity exists in the
pad_startmethod:fn pad_start(str: ByteArray, len: u32) -> ByteArray { let mut missing: ByteArray = ""; let missing_len = if str.len() >= len { 0 } else { len - str.len() }; - while missing.len() < missing_len { - missing.append(@"."); - } + // Pre-allocate the dots in one operation if possible + if missing_len > 0 { + // Create a string of dots of the required length + // This would be more efficient than appending one by one + missing = ".".repeat(missing_len); // Note: implementation would depend on Cairo's string utilities + } missing + str }crates/dojo/core-test/src/tests/storage/storage.cairo (3)
16-23: Ohayo sensei – Assert the full batch result for stronger guarantees
get_manyreturns an array whose length is implicitly expected to matchlayout.len(). While you do check the first two items, an explicitassert(res.len() == layout.len(), …)would catch silent truncation bugs and makes the intent crystal‑clear.
27-35: Prefer.at()over direct indexing for safety
Using(*many[0])sidesteps bounds‑checking, whereasmany.at(0)would raise a descriptive error if the length assumption ever breaks. Swapping to.at()keeps the test honest without extra cost.
65-95: Loop can be written idiomatically and save a ton of gas
The manualloop { … if i == 1000 { break } … }pattern is easy to mis‑count and forces you to annotate a 2 000 000 000 gas ceiling. A simplefor i in 0..1000 { … }lowers byte‑code size, reduces gas, and improves readability.crates/dojo/macros/src/derives/introspect/mod.rs (1)
18-20: Surface parser diagnostics
_diagnosticsis parsed then immediately discarded. Forwarding them viaProcMacroResult::diagnostic?()(or equivalent) would give users actionable feedback instead of a generic “unsupported syntax” panic.crates/dojo/macros/src/derives/introspect/ty.rs (1)
24-35: Pass&strinstead of&Stringto avoid needless allocations
build_item_ty_from_type(item_type: &String)clones the string for every recursive call. Changing the signature to&strand forwarding slices (&array_item_type) removes allocation churn, especially noticeable in deeply nested tuples.crates/dojo/core-test/src/tests/world/namespace.cairo (1)
39-52: Ohayo sensei — good duplicate‑registration guard, but consider asserting the panic reason explicitly.You already rely on
#[should_panic(expected: …)], which is great. For extra robustness, you could also add a trailing assertion (unreached) to guarantee the call path actually panics when the attribute is removed by mistake.Not blocking, purely optional.
crates/dojo/macros/src/derives/introspect/size.rs (2)
92-99: Prefer&strover&Stringfor zero‑cost borrowing, sensei
compute_item_size_from_type(item_type: &String)needlessly requires an ownedString.
All call‑sites provide&Stringor&str; switching to&stravoids repeated allocations and better matches idiomatic Rust.-pub fn compute_item_size_from_type(item_type: &String) -> Vec<String> { +pub fn compute_item_size_from_type(item_type: &str) -> Vec<String> {You’ll need to adjust the internal calls accordingly.
19-21: Same usize coercion issue as in ACL comment, senseiSee the earlier note—emit
usize‑suffixed constants to stay future‑proof.crates/dojo/macros/src/attributes/event.rs (1)
246-247: Literal_hashis never used – consider_ = {unique_hash};to silence linter, senseiA growing number of Cairo linters flag unused bindings.
Nit‑level, but avoids warnings.crates/dojo/macros/src/attributes/model.rs (1)
150-183: Derive list may contain duplicates – de‑dup to keep generated code tidy
missing_derive_attr_namesis built withpush, which can add the same attribute more than once (e.g. whenEXPECTED_DERIVE_ATTR_NAMESalready containsIntrospect).
While the compiler tolerates repeated derives, it bloats the diff and confuses reviewers.- missing_derive_attr_names.push(attr.clone()); - model.model_value_derive_attr_names.push(attr); + if !missing_derive_attr_names.contains(&attr) { + missing_derive_attr_names.push(attr.clone()); + } + if !model.model_value_derive_attr_names.contains(&attr) { + model.model_value_derive_attr_names.push(attr); + }A small set‑based check keeps the output clean.
crates/dojo/core-test/src/tests/world/metadata.cairo (2)
17-27: Test assumes world id0– consider using constant for clarityHard‑coding
resource_id: 0ties the test to an implicit convention. Expose aconst WORLD_ID: u32 = 0;(or similar) in the world helper so future refactors don’t silently break the assertion line 26.const WORLD_ID: felt252 = 0; … let metadata = ResourceMetadata { resource_id: WORLD_ID, … }; assert(world.metadata(WORLD_ID) == metadata, 'invalid metadata');
45-67: Event spy already wraps common assertions – leverage itYou manually deserialize keys (l.52‑65). The forge
EventSpyAssertionsTraitexposesassert_emitted_exact()which can compare the full event tuple in one line and yields clearer failure messages.spy.assert_emitted_exact(@array![(world.contract_address, world::Event::MetadataUpdate(world::MetadataUpdate { … }))]);Less boilerplate, more readable tests!
crates/dojo/core-test/src/tests/world/model.cairo (2)
80-99:grant_owner+register_modelhappy‑path: add negative assertion for non‑ownerRight after you show the success path for
bob, consider asserting that an untouched random account cannot register the same model. This guards against regressions in ACL checks that could pass the positive test but fail the negative case.let eve: ContractAddress = 0xeve.try_into().unwrap(); snf_utils::set_account_address(eve); snf_utils::set_caller_address(eve); expect_panic(|| { world.register_model("dojo", class_hash); });Complements the writer / malicious‑contract tests already present.
165-194: Persisted data check is excellent – add assertion on pre‑upgrade storage sizeYou verify values after upgrade but not that no additional rows were introduced. A quick
assert(world_storage.count::<FooModelMemberAdded>() == 1)before and after the call would detect schema mistakes that silently duplicate storage slots.let before = world_storage.count::<FooModelMemberAdded>(); world.upgrade_model("dojo", class_hash); let after = world_storage.count::<FooModelMemberAdded>(); assert!(before == after, 'upgrade duplicated rows');crates/dojo/core-test/src/snf_utils.cairo (1)
30-31: Nit: typo in docstring“Declare and deploy and contract.” → “Declare and deploy a contract.”
Very small, but readers bump into it every time.crates/dojo/macros/src/derives/introspect/structs.rs (2)
51-62: Ohayo sensei – heavyformat!in hot macro pathUsing
format!inside the macro expansion loop allocates a newStringevery time the derive is used. For compile‑time performance you can build the token stream directly withquote!or string concatenation of&'static strslices.
167-178: Ohayo sensei –usizedetection could false‑positive
type_contains_usizedoes a naïvecontains("usize"), soMyUsizeWrapperorref_usize_addrwill incorrectly trigger the diagnostic. Consider parsing the type clause and matching on an actualast::PathSegmentequal tousize.crates/dojo/macros/src/attributes/library.rs (1)
146-155: Minor: avoid duplicateWorldProviderEventvariantsWhen an
Eventenum already exists and you still appendWorldProviderEventinsidemerge_event, duplication becomes possible. You can insert only if the variant is missing.crates/dojo/macros/src/derives/introspect/utils.rs (1)
42-58: Consider adding error handling for malformed input.The function works well for valid inputs but could benefit from handling malformed array type strings more gracefully.
pub fn get_array_item_type(ty: &str) -> String { if ty.starts_with("Array<") { - ty.trim() - .strip_prefix("Array<") - .unwrap() - .strip_suffix('>') - .unwrap() - .to_string() + ty.trim() + .strip_prefix("Array<") + .unwrap_or_else(|| panic!("Failed to strip 'Array<' prefix from '{}'", ty)) + .strip_suffix('>') + .unwrap_or_else(|| panic!("Failed to strip '>' suffix from '{}'", ty)) + .to_string() } else { - ty.trim() - .strip_prefix("Span<") - .unwrap() - .strip_suffix('>') - .unwrap() - .to_string() + ty.trim() + .strip_prefix("Span<") + .unwrap_or_else(|| panic!("Failed to strip 'Span<' prefix from '{}'", ty)) + .strip_suffix('>') + .unwrap_or_else(|| panic!("Failed to strip '>' suffix from '{}'", ty)) + .to_string() } }crates/dojo/macros/src/helpers/parser.rs (1)
123-151: Consider handling complex path segments in derive attributes.The extract_derive_attr_names method currently only handles simple path segments (e.g.,
Introspect), but not complex paths likefoo::bar::Baz.if let AttributeArgVariant::Unnamed(ast::Expr::Path(path)) = a.variant { - if let [ast::PathSegment::Simple(segment)] = &path.elements(db)[..] { - Some(segment.ident(db).text(db).to_string()) - } else { - None - } + // Handle both simple and complex paths + let segments = path.elements(db); + if !segments.is_empty() { + // For complex paths, take the last segment + if let ast::PathSegment::Simple(segment) = &segments[segments.len() - 1] { + Some(segment.ident(db).text(db).to_string()) + } else { + None + } + } else { + None + } } else { None }crates/dojo/core-test/src/tests/helpers/event.cairo (1)
47-75: Improve documentation for BadLayoutType test contract.The comment on line 71 explains the intent, but it would be helpful to elaborate on how this specifically tests bad layout types.
fn layout(self: @ContractState) -> dojo::meta::Layout { - // Should never happen as dojo::event always derive Introspect. + // Returns an empty layout intentionally to test the system's + // handling of invalid layout types. This tests the error case + // where a contract tries to register with an incompatible layout. dojo::meta::Layout::Fixed([].span()) }crates/dojo/core-test/src/tests/world/event.cairo (1)
85-103: Ohayo sensei – repetitive “manual‑decode” block is begging for a helper
The 15‑line pattern for manually unpackingEventRegistered/EventUpgraded(grab span, pop selector, Serde‑deserialize twoByteArrays, then assert) is duplicated in several tests. Extracting this logic into a small helper (e.g.assert_event_registered(spy, world_addr, expected_name, expected_ns, expected_hash)) will:
• keep each test to 2‑3 intent‑focused lines,
• reduce copy‑paste risk when the event schema changes,
• make panic messages clearer (the helper can print diff info).crates/dojo/core-test/src/tests/world/world.cairo (1)
155-172: Ohayo sensei – assert that anUpgradeevent was emitted
test_upgradeable_worldverifies thehello()call but never checks thatUpgrade(or equivalent) was actually fired. Without that, a silent no‑op implementation could pass.
Consider spying on events similar to other tests and assert(Event::WorldUpgraded { … })to guarantee the upgrade path executed.crates/dojo/core-test/src/tests/model/model.cairo (2)
249-254: Ohayo sensei – literal type clarity
world.write_member(foo.ptr(), selector!("v1"), 42);writes afelt252literal into au128field. While the implicit cast currently works, an explicit suffix (42_u128) avoids accidental widening & future compiler warnings.
Same pattern appears in multiple tests (42,43).
80-88: Ohayo sensei – worth assertingkeys,layout&schemaindividually
test_model_definitionbundles fourassert_eq!calls; if one fails we only know “something differed”. Splitting them or using a helper that prints a diff gives more actionable feedback.crates/dojo/core-test/src/tests/world/contract.cairo (1)
204-212: Comment contradicts the asserted failureThe doc‑comment states the account “should be able to deploy the contract” yet the test is annotated with
#[should_panic].
Please update either the access‑control expectation or drop the panic annotation so reader intent is unambiguous.Also applies to: 206-207
crates/dojo/macros/src/derives/introspect/enums.rs (1)
98-107:is_enum_packableignores dynamic field layoutsThe current check disallows any dynamic field (
!vs.2) – good – but also silently assumes identical ordering.
Edge‑case: different ordering of identical fixed‑size fields still yields identicalsizesvectors, passing this check even though packing offset semantics differ.
Consider hashing the concreteLayoutstrings instead of length/size alone for stronger equivalence.crates/dojo/core-test/src/tests/contract.cairo (1)
170-175: Name‑validation panic wording brittleHard‑coding the full validator regex in the expected string makes the test fragile when the regex is tweaked for new rules.
Recommendation: match on a stable substring (e.g.,"Namespace `` is invalid") to minimise future churn.crates/dojo/macros/src/attributes/contract.rs (1)
335-348: Constructor parameter check is brittle
is_valid_constructor_paramsrelies on a substring search over raw source text.
Whitespace, line breaks or attribute ordering can easily bypass the check (e.g.ref\nself: ContractState).
Parsing the parameter list into AST nodes and verifying their kinds would be more robust.Not blocking for now, but worth tightening to avoid false negatives.
crates/dojo/core-test/src/tests/helpers/model.cairo (1)
38-66: Minor: consider pre‑computing model selectors once
deploy_world_for_model_upgradesrecomputesselector_from_namesfor everyset_entitycall.
Caching the selector locally (or using aletbinding) makes the intent clearer and avoids redundant hashing work, even though the cost is negligible in tests.Nit‑level suggestion – feel free to skip.
crates/dojo/core-test/src/world.cairo (1)
170-172: Clarify salt derivation for contract registration
Usingbytearray_hash(name)assaltcouples deterministic deployment to a hashing strategy that could change. Consider exposing a dedicated helper (e.g.dojo::utils::namespace_salt) to make intent explicit and future‑proof.crates/dojo/core-test/src/tests/helpers/helpers.cairo (2)
162-170: Typo in field nameheigth
Ohayo sensei, tiny nit: theCharactermodel spells “height” asheigth. If this wasn’t intentional for fixture parity, consider correcting it to avoid future confusion.-pub heigth: felt252, +pub height: felt252,
269-277: Unnecessarymutvariable & potential ownership confusion
spawn_test_worldalready returns a freshWorldStorage. You declare it asmut, yet pass an immutable@WorldStoragepointer tosync_perms_and_inits. Dropping themutclarifies intent and prevents accidental state changes outside the helper.- let mut world = spawn_test_world([namespace_def].span()); - world.sync_perms_and_inits([bar_def].span()); + let world = spawn_test_world([namespace_def].span()); + world.sync_perms_and_inits([bar_def].span());
📜 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 (63)
Cargo.toml(1 hunks)crates/dojo/core-test/Scarb.toml(1 hunks)crates/dojo/core-test/src/lib.cairo(1 hunks)crates/dojo/core-test/src/snf_utils.cairo(1 hunks)crates/dojo/core-test/src/tests/contract.cairo(1 hunks)crates/dojo/core-test/src/tests/event/event.cairo(1 hunks)crates/dojo/core-test/src/tests/expanded/bytearray_hash.cairo(1 hunks)crates/dojo/core-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-test/src/tests/helpers/event.cairo(1 hunks)crates/dojo/core-test/src/tests/helpers/helpers.cairo(1 hunks)crates/dojo/core-test/src/tests/helpers/library.cairo(1 hunks)crates/dojo/core-test/src/tests/helpers/model.cairo(1 hunks)crates/dojo/core-test/src/tests/meta/introspect.cairo(1 hunks)crates/dojo/core-test/src/tests/model/model.cairo(1 hunks)crates/dojo/core-test/src/tests/storage/database.cairo(1 hunks)crates/dojo/core-test/src/tests/storage/packing.cairo(1 hunks)crates/dojo/core-test/src/tests/storage/storage.cairo(1 hunks)crates/dojo/core-test/src/tests/utils/hash.cairo(1 hunks)crates/dojo/core-test/src/tests/utils/key.cairo(1 hunks)crates/dojo/core-test/src/tests/utils/layout.cairo(1 hunks)crates/dojo/core-test/src/tests/utils/misc.cairo(1 hunks)crates/dojo/core-test/src/tests/utils/naming.cairo(1 hunks)crates/dojo/core-test/src/tests/world/acl.cairo(1 hunks)crates/dojo/core-test/src/tests/world/contract.cairo(1 hunks)crates/dojo/core-test/src/tests/world/event.cairo(1 hunks)crates/dojo/core-test/src/tests/world/metadata.cairo(1 hunks)crates/dojo/core-test/src/tests/world/model.cairo(1 hunks)crates/dojo/core-test/src/tests/world/namespace.cairo(1 hunks)crates/dojo/core-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core-test/src/tests/world/world.cairo(1 hunks)crates/dojo/core-test/src/utils.cairo(1 hunks)crates/dojo/core-test/src/world.cairo(1 hunks)crates/dojo/macros/Cargo.toml(1 hunks)crates/dojo/macros/Scarb.toml(1 hunks)crates/dojo/macros/src/attributes/contract.rs(1 hunks)crates/dojo/macros/src/attributes/event.rs(1 hunks)crates/dojo/macros/src/attributes/library.rs(1 hunks)crates/dojo/macros/src/attributes/mod.rs(1 hunks)crates/dojo/macros/src/attributes/model.rs(1 hunks)crates/dojo/macros/src/constants.rs(1 hunks)crates/dojo/macros/src/derives/introspect/enums.rs(1 hunks)crates/dojo/macros/src/derives/introspect/generics.rs(1 hunks)crates/dojo/macros/src/derives/introspect/layout.rs(1 hunks)crates/dojo/macros/src/derives/introspect/mod.rs(1 hunks)crates/dojo/macros/src/derives/introspect/size.rs(1 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(1 hunks)crates/dojo/macros/src/derives/introspect/ty.rs(1 hunks)crates/dojo/macros/src/derives/introspect/utils.rs(1 hunks)crates/dojo/macros/src/derives/mod.rs(1 hunks)crates/dojo/macros/src/helpers/checker.rs(1 hunks)crates/dojo/macros/src/helpers/debug.rs(1 hunks)crates/dojo/macros/src/helpers/diagnostic_ext.rs(1 hunks)crates/dojo/macros/src/helpers/formatter.rs(1 hunks)crates/dojo/macros/src/helpers/misc.rs(1 hunks)crates/dojo/macros/src/helpers/mod.rs(1 hunks)crates/dojo/macros/src/helpers/parser.rs(1 hunks)crates/dojo/macros/src/helpers/proc_macro_result_ext.rs(1 hunks)crates/dojo/macros/src/helpers/tokenizer.rs(1 hunks)crates/dojo/macros/src/inlines/bytearray_hash.rs(1 hunks)crates/dojo/macros/src/inlines/mod.rs(1 hunks)crates/dojo/macros/src/inlines/selector_from_tag.rs(1 hunks)crates/dojo/macros/src/lib.rs(1 hunks)crates/katana/explorer/ui(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/dojo/core-test/src/tests/storage/storage.cairo (1)
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within `crates/dojo/core/src/world/storage.cairo`, length checks between `entity_ids` and `values` are not required in test API functions like `write_values_from_ids_test`.
🧬 Code Graph Analysis (6)
crates/dojo/macros/src/helpers/proc_macro_result_ext.rs (1)
crates/dojo/macros/src/helpers/diagnostic_ext.rs (2)
with_error(4-4)with_error(9-11)
crates/dojo/macros/src/attributes/mod.rs (5)
crates/dojo/macros/src/helpers/debug.rs (1)
debug_macro(11-33)crates/dojo/macros/src/attributes/event.rs (1)
process(35-43)crates/dojo/macros/src/attributes/library.rs (1)
process(33-41)crates/dojo/macros/src/attributes/contract.rs (1)
process(33-41)crates/dojo/macros/src/attributes/model.rs (1)
process(41-49)
crates/dojo/macros/src/helpers/checker.rs (2)
crates/dojo/macros/src/helpers/parser.rs (2)
attrs(128-150)extract_derive_attr_names(123-151)crates/dojo/macros/src/helpers/proc_macro_result_ext.rs (2)
fail(6-6)fail(12-14)
crates/dojo/macros/src/helpers/parser.rs (3)
crates/dojo/macros/src/derives/introspect/structs.rs (2)
struct_ast(86-103)struct_ast(137-142)crates/dojo/macros/src/attributes/event.rs (1)
members(83-92)crates/dojo/macros/src/helpers/misc.rs (1)
members(20-34)
crates/dojo/macros/src/derives/introspect/layout.rs (5)
crates/dojo/macros/src/derives/introspect/utils.rs (7)
get_array_item_type(42-58)get_tuple_item_types(62-102)is_array(34-36)is_byte_array(30-32)is_tuple(38-40)is_unsupported_option_type(26-28)primitive_type_introspection(7-22)crates/dojo/macros/src/derives/introspect/size.rs (1)
get_tuple_item_types(112-115)crates/dojo/macros/src/derives/introspect/ty.rs (1)
get_tuple_item_types(45-48)crates/dojo/bindgen/src/plugins/mod.rs (1)
join(87-89)crates/dojo/macros/src/derives/introspect/structs.rs (1)
layouts(221-221)
crates/dojo/macros/src/derives/introspect/ty.rs (1)
crates/dojo/macros/src/derives/introspect/utils.rs (5)
get_array_item_type(42-58)get_tuple_item_types(62-102)is_array(34-36)is_byte_array(30-32)is_tuple(38-40)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (125)
crates/katana/explorer/ui (1)
1-1: Submodule pointer update approved
Ohayo sensei, bumping the UI subproject commit to11c969a5b10f98c47119e2fd2cbe60bad0c372c9looks correct. Please ensure this matches the intended upstream UI changes for the Cairo 2.11.2 upgrade.crates/dojo/macros/Scarb.toml (1)
1-7: Ohayo! Package configuration looks good, sensei!The package metadata is well-defined with appropriate name, version and edition.
crates/dojo/macros/src/lib.rs (1)
1-5: Ohayo! Module structure looks clean, sensei!The module structure for the macros crate provides a good organization for the different types of macros and their supporting code.
Cargo.toml (1)
156-160: Ohayo! New dependencies for macro support added, sensei!The additions of Cairo language dependencies are necessary for the new macros functionality.
crates/dojo/core-test/Scarb.toml (5)
1-4: Ohayo! Test package metadata looks good, sensei!Package name and description are appropriately defined for the test crate.
5-7: Cairo version specified correctly, sensei!The specified Cairo version (2.11.2) matches what was mentioned in the PR objectives.
9-16: Dependencies are properly configured, sensei!Dependencies on
starknet, localdojo, and the newly addeddojo_macrosare correctly specified, along with appropriate test dependencies.
18-19: External contract build correctly configured, sensei!The Starknet contract target with external build from the world contract path is properly set up.
21-25: Test script and tool configurations look good, sensei!The test script using
snforge testand the configuration to allow prebuilt plugins are both appropriate for this test package.crates/dojo/core-test/src/tests/utils/key.cairo (2)
1-10: Ohayo! Entity ID test looks good, sensei!The test correctly verifies that
entity_id_from_serialized_keysproduces the expected Poseidon hash of the input keys.
12-17: Key combination test is implemented correctly, sensei!The test properly verifies that
combine_keyproduces the expected combined key using Poseidon hashing.crates/dojo/macros/Cargo.toml (3)
1-7: Ohayo, sensei! The package configuration looks solid.The new
dojo_macroscrate is well-structured with workspace inheritance for standard metadata fields, which maintains consistency across the Dojo project.
8-10: Nice crate-type configuration!Setting up both
rlibandcdylibcrate types ensures the macros can be used as both a Rust library dependency and potentially loaded dynamically if needed.
11-26: Dependencies are well organized, sensei!All dependencies leverage workspace versioning with the
.workspace = truepattern, which ensures consistency across the project. The organization with a clear separation between core dependencies and specific ones at line 22 improves readability.crates/dojo/macros/src/inlines/mod.rs (1)
1-5: Ohayo! Module structure looks clean and organized.The imports and module declarations follow good Rust practices for organizing procedural macros.
crates/dojo/core-test/src/tests/utils/naming.cairo (2)
1-2: Ohayo! Clean import for the function under test.Single-purpose import aligns well with the test's focus.
3-9: Good test cases for valid names, sensei!The test covers a nice range of valid name patterns including lowercase, uppercase, alphanumeric, and names with underscore suffixes.
crates/dojo/macros/src/helpers/mod.rs (1)
9-31: Well-organized helper modules with clean re-exports!The modular approach to organizing helper utilities makes the codebase more maintainable and allows for more focused implementations. Re-exporting the modules' contents makes them easily accessible.
crates/dojo/core-test/src/tests/event/event.cairo (2)
1-9: Ohayo sensei! Well-structured event definition with clear field separation.The
FooEventstruct demonstrates good separation between key fields and value fields using the#[key]attribute. This structure will allow efficient event keying and filtering when emitted.
11-18: Clean test implementation validating event metadata consistency.The test elegantly verifies that the event definition's metadata (name, layout, schema) matches the static methods on the event type, ensuring that the
#[dojo::event]macro is generating consistent introspection capabilities.crates/dojo/core-test/src/tests/utils/hash.cairo (2)
1-11: Well-defined model structure with appropriate trait derivation.The
MyModelstruct correctly uses the#[dojo::model]attribute and#[key]attribute for thexfield. The derived traits (Drop,Copy,Serde) are appropriate for a model that will be stored and retrieved from storage.
13-19: Excellent selector computation verification test, sensei!This test properly validates that the selector computed from namespace and model name matches the one computed by the model's
selectormethod with the predefined namespace hash. This verification is critical for ensuring consistent model registration and access throughout the Dojo framework.crates/dojo/core-test/src/tests/expanded/bytearray_hash.cairo (4)
3-11: Ohayo! Good baseline test for bytearray_hash macro.This test correctly verifies that the hash computed by the
bytearray_hash!macro matches the runtime hash computed by serializing and hashing withposeidon_hash_span.
13-21: Good edge case test for empty string input.Testing the empty string case is important for ensuring the macro handles this edge case correctly.
23-31: Well-chosen boundary test for strings approaching felt252 limit.Testing with a 31-character string is strategic as it approaches the boundary of what can be stored in a single felt252, ensuring the macro handles this case properly.
33-41: Good test for strings exceeding felt252 capacity.Testing strings longer than 31 characters ensures the macro correctly handles multi-felt serialization cases.
crates/dojo/macros/src/helpers/debug.rs (1)
5-11: Ohayo! Well-documented debug helper with clear deprecation plan.Good documentation explaining the purpose and usage of the debug helper. The TODO note clearly indicates this is a temporary solution until
scarb expandintegration is complete.crates/dojo/macros/src/derives/mod.rs (2)
6-12: Ohayo sensei! The macro implementation looks clean and well organized.The
introspectmacro is implemented well, delegating to a shared process function with a flag to indicate unpacked processing. The debugging output is a nice touch for development and troubleshooting.
14-20: Good implementation of the packed variant.The implementation maintains consistency with the unpacked version by reusing the same process function. This is a good pattern to avoid code duplication while supporting different variants.
crates/dojo/macros/src/helpers/formatter.rs (3)
3-7: Ohayo sensei! Nice approach with the namespace struct.Using an empty struct as a namespace for related formatter functions is a clean pattern in Rust that aligns well with the language's organization principles.
8-15: Well-implemented serialization formatter.The
serialize_member_tyfunction handles the conditional prefix elegantly. The formatting ensures proper structure for the serialization code generation.
17-20: Clean and straightforward member declaration formatter.This simple utility function provides consistent formatting for member declarations, which will ensure generated code maintains a uniform style.
crates/dojo/core-test/src/tests/utils/misc.cairo (4)
3-9: Ohayo sensei! Good test case for detecting None values.The test verifies the
any_nonefunction correctly identifies when at least one element in the array isNone, which is an important edge case to cover.
11-17: Test covers important negative case.Ensuring
any_nonereturns false when noNonevalues are present is equally important to verify the function works correctly in all scenarios.
19-22: Good edge case testing for empty array.Testing the sum function with an empty array validates the function handles the boundary case correctly by returning the identity element (0).
24-30: Well-structured test for mixed Some/None values.This test effectively verifies that the sum function correctly handles arrays with a mix of
Somevalues andNonevalues by including only theSomevalues in the sum.crates/dojo/macros/src/derives/introspect/generics.rs (2)
7-12: Ohayo sensei! The function signature looks good.The function is well-named and has appropriate parameters for working with generic types in the context of introspection.
13-29: Clean extraction of generic type parameters.The code effectively filters out non-type parameters and collects only the type parameter names. The handling of the optional wrapped parameter list is done properly.
crates/dojo/macros/src/helpers/misc.rs (1)
9-37: Ohayo! The hash computation logic looks robust and well-implemented, sensei!The
compute_unique_hashfunction provides a sound approach to generating unique cryptographic hashes for element contracts based on their name, packing state, and member attributes. The use of Poseidon hash is appropriate for this cryptographic purpose.crates/dojo/macros/src/helpers/proc_macro_result_ext.rs (1)
5-21: Excellent trait extension for procedural macros, sensei!The
ProcMacroResultExttrait provides a clean, consistent interface for handling error cases and diagnostics in procedural macros. The implementation builds nicely on the existingDiagnosticsExttrait and follows good Rust conventions.crates/dojo/core-test/src/utils.cairo (1)
37-57: Array assertion function is clear and effective, sensei!The
assert_arrayfunction provides useful validation for array equality with descriptive error messages that include the index and values when mismatches occur. The implementation is straightforward and will be valuable for tests.crates/dojo/core-test/src/tests/storage/packing.cairo (8)
9-41: Bit operations tests are well-structured and thorough, sensei!The tests for
fpow,pow2_const, and bit shift operations provide good coverage of the fundamental bit operations used in the packing module.
43-84: Single value and mixed packing tests are comprehensive, sensei!These tests verify the correct packing and unpacking of individual values and combinations of different bit widths, which are essential for ensuring the reliability of the storage packing mechanism.
86-153: Multiple value packing tests provide excellent coverage, sensei!The tests thoroughly verify packing and unpacking of multiple values, including iterative scenarios with different bit widths and value transformations. This comprehensive testing ensures the robustness of batch operations.
155-238: Type-specific packing tests are excellent, sensei!These tests validate the packing and unpacking functionality across all supported data types (u8, u16, u32, u64, u128, felt252, bool, ContractAddress, ClassHash). This ensures type compatibility and correctness for all possible use cases.
240-293: U256 packing tests are thorough and well-structured, sensei!These tests verify the correct handling of u256 values, including both low-level and high-level packing/unpacking functions. Testing both the inner implementation and the public API ensures complete coverage.
295-338: Edge case tests are valuable, sensei!The tests for max felt252 values and single felt252 values ensure the packing mechanism works correctly with boundary values. This prevents potential overflow issues or incorrect behavior at the limits.
340-380: Offset and packed size calculation tests are comprehensive, sensei!These tests verify the correct handling of offsets in packing operations and accurate calculation of packed sizes for various layouts. This ensures proper memory usage and access patterns.
382-414: Error case tests are crucial, sensei!The tests for invalid inputs (oversized layout, exceeding length, layout too long) verify that the system properly rejects invalid inputs with appropriate error messages. These tests are essential for robust error handling.
crates/dojo/core-test/src/tests/utils/layout.cairo (5)
1-17: Ohayo! Nice test implementation for the layout finder, sensei!This test effectively verifies the happy path for
find_field_layout, ensuring it correctly locates and returns layouts by selector. The test structure is clean and assertions are well-placed.
19-30: Well-structured negative test case!Good work testing the negative case where a selector doesn't exist in the layouts. This ensures robust error handling in your lookup utilities.
32-47: Excellent model layout test implementation, sensei!This test properly validates the
find_model_field_layoutfunction's ability to retrieve a field layout from within a structured layout hierarchy.
49-62: Good negative test for model layout lookups!Testing the behavior when a field doesn't exist in a model layout ensures proper error handling throughout the system.
64-68: Smart error case testing, sensei!Using
#[should_panic]to verify error handling for invalid layout types is a great approach. This test ensures the function properly validates its inputs.crates/dojo/macros/src/helpers/diagnostic_ext.rs (2)
1-15: Ohayo! Excellent extension trait for diagnostics collection, sensei!The
DiagnosticsExttrait provides clean utility methods for creating and adding error diagnostics. This will make error handling much more consistent across your macro implementations.
17-30: Nice pretty-printing implementation for diagnostics!The
DiagnosticExttrait with itsto_pretty_stringmethod provides a consistent way to format diagnostic messages. This will improve the developer experience by making error messages more readable.crates/dojo/core-test/src/tests/helpers/library.cairo (2)
1-4: Ohayo! Clean interface declaration, sensei!This StarkNet interface trait is well-defined with a simple, focused method for testing purposes.
6-16: Great demonstration of the new library macro!This implementation shows how to use the new
#[dojo::library]macro while providing a simple implementation that returns a constant value. Perfect for testing the library registration and call mechanisms.crates/dojo/macros/src/constants.rs (1)
1-9: Ohayo! Excellent constants organization, sensei!Centralizing these constants is a great practice for maintainability. Having names for derive macros, expected attributes, delimiters, and special function names all in one place will make the macro implementations more consistent and easier to update.
crates/dojo/core-test/src/tests/world/storage.cairo (5)
1-2: Ohayo sensei! Looks good with all necessary imports.The imports bring in the
ModelStoragetrait and all the helper types needed for the tests. Clean and focused.
4-29: The write_simple test looks excellent!This test thoroughly validates the basic CRUD operations (create, read, update, delete) for a simple model. The flow of creating a model, writing it, reading it back, verifying values, erasing it, and then confirming erasure is clean and comprehensive.
31-76: Batch operations on copiable models tested well!The test properly creates multiple models with different values, writes them all at once, reads them back, verifies them individually, then erases them all and confirms the erasure. Good coverage of the batch operations API.
78-123: Strong test for non-copiable models!Similar to the previous test but for non-copiable models containing arrays and strings. The approach of using
pop_front()instead of indexed access is appropriate for non-copiable types. Well done!
125-136: Known issue documented with Option serializationThe test highlights a known serialization issue with
Option::Nonevalues. This is properly documented with comments suggesting potential mitigation strategies.Since this is a known issue that affects the correctness of your model data, could you create a follow-up task to address this serialization problem? Having
Option::Noneread asOption::Some(0)could lead to subtle bugs.crates/dojo/macros/src/inlines/bytearray_hash.rs (4)
1-8: Ohayo! All necessary imports present for the macro implementation.Good choice of imports from the Cairo language utilities and Dojo types. The custom helpers will make the macro implementation cleaner.
9-19: Macro processing function follows best practicesThe
processfunction correctly uses the parser database to parse inline arguments and provides a clear error message if parsing fails. The approach of delegating toprocess_astfor successful parsing is clean.
21-35: String handling in hash computation looks goodThe function properly extracts the string literal, removes quotes, computes a hash, and formats it as a 64-digit hex string. The token generation and error handling are robust.
37-82: Comprehensive test coverage for the macroThe tests cover both error paths (invalid input formats) and the happy path with a valid string input. The expected hash for "hello" is verified, ensuring the macro's correctness.
crates/dojo/macros/src/attributes/mod.rs (5)
1-8: Ohayo sensei! Clean module structure for attribute macros.The imports and module declarations establish a clear organization for the different attribute macros (model, event, contract, library) that will be provided.
9-15: Model attribute macro implementation looks goodThe macro properly delegates to the specialized implementation in
model::DojoModeland includes debug output. The use ofattribute_macrowith a parent specification is the correct approach.
17-23: Event attribute macro follows the same patternConsistent with the model macro implementation, this delegates to
event::DojoEventand includes debugging. The consistent pattern makes the code easy to understand.
25-31: Contract attribute macro maintains consistencyFollowing the established pattern, this macro delegates to
contract::DojoContractand includes the same debugging approach, maintaining consistency across the attribute implementations.
33-39: Library attribute completes the set with the same patternThe fourth attribute macro follows the same consistent pattern as the others, delegating to
library::DojoLibrary. The uniformity across all four macros is excellent for maintainability.crates/dojo/core-test/src/tests/storage/database.cairo (1)
1-17: Basic database operations tested well!The test correctly verifies that values can be stored and retrieved with the right content and length. Good assertion messages help with debugging.
crates/dojo/macros/src/helpers/tokenizer.rs (6)
1-4: Ohayo! Imports look clean and specific, sensei.The imports are well-focused on the necessary components from the Cairo language libraries needed for token manipulation and AST handling.
5-6: Great use of namespace pattern for utility functions, sensei!Using an empty struct as a namespace for related functionality follows good Rust patterns.
7-12: The tokenize method is clean and focused!This utility method provides a simple way to convert strings into token trees for use in macros.
14-24: Excellent documentation of the rebuild_original_struct method!The comments clearly explain why this method is necessary for handling attribute proc macros while avoiding duplicate processing of derive attributes.
25-36: Code properly preserves database context while handling struct elements.The consistent use of
SyntaxNodeWithDb::new(&el, db)ensures that database context is maintained when working with syntax nodes.
37-43: Elegant use of quote! macro for struct reconstruction.The token stream is rebuilt with clear structure, maintaining the original elements while excluding problematic derive attributes.
crates/dojo/macros/src/helpers/checker.rs (4)
1-8: Ohayo! Imports are well-organized and specific, sensei.The necessary components for diagnostics, parsing, and validation are properly imported. Good use of local and external dependencies.
9-13: Clean namespace pattern for checker utilities!Using an empty struct as a namespace for validation methods follows good Rust conventions.
14-30: Effective derive conflict detection!The method properly checks for conflicting derive attributes and provides a clear error message. Good validation for preventing incompatible attribute combinations.
32-42: Name validation logic is clean and informative.The method effectively validates element names and returns helpful error messages. The early return pattern with Option is elegant.
crates/dojo/macros/src/inlines/selector_from_tag.rs (5)
1-8: Ohayo! Well-structured imports for macro processing, sensei.The necessary components for token handling, parsing, and selector computation are properly imported.
9-19: Clean entry point for the macro processing.The process function effectively validates input and delegates to the AST processing function, with clear error handling.
21-43: Robust tag validation and selector computation.The function properly extracts the string, validates the tag format, computes the selector, and formats it as a hex string. Error messages are clear and specific.
45-75: Excellent test coverage for invalid inputs!The tests thoroughly check various error cases, including invalid parameter formats and types, with verification of the expected error messages.
77-105: Comprehensive testing of tag validation and computation.Tests cover both invalid tags and valid tags, validating the expected output format and values. The specific test with "ns-Position" provides good verification of the selector computation.
crates/dojo/core-test/src/lib.cairo (4)
1-13: Ohayo! Well-structured test utilities with clear exports, sensei.The conditional compilation for test targets is appropriate, and the public exports make key testing utilities easily accessible.
15-46: Excellent organization of test modules and helpers!The test structure is comprehensive, covering contracts, events, and expanded test cases. The helpers module with its various exports provides a solid foundation for testing.
48-68: Comprehensive test coverage across core components.The test modules cover introspection, models, storage, and utilities - ensuring all aspects of the Dojo framework are thoroughly tested.
70-80: Well-organized world-related test modules.The world test modules comprehensively cover ACL, contracts, events, metadata, models, namespaces, storage, and world functionality. This ensures the world contract ecosystem is thoroughly validated.
crates/dojo/macros/src/derives/introspect/mod.rs (1)
48-57: Verify generic handling to avoid malformed impl names
Whengeneric_typesis non‑empty the code injectsgeneric_implsbut notgeneric_typesinto the impl name, potentially producingFooIntrospect<>ifgeneric_implsis empty. Please double‑check the caller always supplies both strings coherently.crates/dojo/core-test/src/tests/world/namespace.cairo (2)
8-37: Ohayo sensei — solid positive‑flow tests!The happy‑path test exercises the full registration flow, covers event emission, and uses
bytearray_hashto cross‑check ownership. Everything looks tidy and deterministic to me.
72-79: Ohayo sensei — corner‑case coverage appreciated!Validating the empty‑string edge case keeps the naming rules tight. LGTM.
crates/dojo/core-test/src/tests/world/acl.cairo (2)
19-21: Type mismatch risk –u32literal insideOption<usize>, sensei
cumulated_sizesis au32, yet you emitOption::Some({})directly into Cairo code that expectsusize.
Although small literals silently coerce today, an upcoming Cairo strict‑numeric mode will reject this.- sizes.push(format!("Option::Some({})", cumulated_sizes)); + sizes.push(format!("Option::Some({}usize)", cumulated_sizes));Would you like a quick script to grep other occurrences of this pattern across the macro crate?
106-127: 🛠️ Refactor suggestionDerive list may contain duplicates – dedup before emit, sensei
missing_derive_attrsis appended toevent_value_derive_attr_nameswithout checking for pre‑existing entries.
If the struct already derivedCopy,Drop, etc., the resulting#[derive(..)]on{type_name}Valuecan contain duplicates, causing a compilation error in Cairo ≥2.11.Consider inserting a simple
HashSet(or manual linear check) to avoid duplicates before joining:- event_value_derive_attr_names = derive_attr_names + event_value_derive_attr_names = derive_attr_names .iter() .map(|d| d.to_string()) .filter(|d| d != DOJO_INTROSPECT_DERIVE && d != DOJO_PACKED_DERIVE) - .collect::<Vec<String>>(); + .collect::<std::collections::HashSet<_>>() + .into_iter() + .collect();Likely an incorrect or invalid review comment.
crates/dojo/macros/src/attributes/event.rs (1)
100-127: 🛠️ Refactor suggestionPotential derive duplication leads to compile errors, sensei
Here
event_value_derive_attr_namesstarts with all derives (minus Introspect/Packed) then you push missing ones, which can double‑insert when a trait is both present and considered “missing”. A quick dedup avoids “trait already derived” errors.- event.event_value_derive_attr_names = derive_attr_names - .iter() - .map(|d| d.to_string()) - .filter(|d| d != DOJO_INTROSPECT_DERIVE && d != DOJO_PACKED_DERIVE) - .collect::<Vec<String>>(); + use std::collections::HashSet; + let mut derives: HashSet<String> = derive_attr_names + .iter() + .map(|d| d.to_string()) + .filter(|d| d != DOJO_INTROSPECT_DERIVE && d != DOJO_PACKED_DERIVE) + .collect();(then convert back to
Vecwhen needed).Likely an incorrect or invalid review comment.
crates/dojo/macros/src/attributes/model.rs (1)
185-192:unique_hashserialized as decimal string – confirm target type
compute_unique_hash()returns a numeric felt? Converting it with.to_string()and splicing as{unique_hash}yields an unquoted decimal literal, which is fine only if the called site expects a felt literal. If the intention is aByteArray, you’ll need quotes:let _hash = bytearray!("{}");Please double‑check the expected type inside
ensure_uniqueto avoid a silent type mismatch.crates/dojo/core-test/src/snf_utils.cairo (1)
74-75: Potential ByteArray format mismatch
format!("e_{name}")produces aByteArrayonly if the implicitInto<ByteArray>is in scope forString. Confirm that conversion exists; otherwise call.into()explicitly.crates/dojo/macros/src/attributes/library.rs (1)
89-99: Constructor / init checks executed before merge flags
has_constructorandhas_initare set inside the same loop, but the validation occurs after the loop—good. However, because of the earlyreturnbug above they might never be reached. Once fixed, keep as is.crates/dojo/macros/src/derives/introspect/utils.rs (6)
3-4: Nicely structured TypeIntrospection struct!Ohayo sensei! The TypeIntrospection struct looks well-designed with appropriate derives. The tuple structure with size and bit width vector is a clean approach.
7-22: Complete primitive type mapping!The primitive_type_introspection function provides a comprehensive mapping of Cairo types to their introspection data. All essential primitive types are covered with their correct bit widths.
26-28: LGTM for Option check.Clean implementation for detecting unsupported Option types containing tuples.
30-40: Simple and effective type checks.These utility functions for detecting different types (ByteArray, Array/Span, tuple) are concise and clear.
62-102: Well implemented tuple parsing algorithm!Ohayo sensei! The get_tuple_item_types function handles nested tuples effectively by tracking parentheses depth. The algorithm correctly handles comma placement and nested structure extraction.
104-149: Excellent test coverage for tuple parsing!Comprehensive test cases covering simple tuples, nested tuples, and complex combinations with arrays. The test helper functions for array comparison and error messages are well designed.
crates/dojo/macros/src/helpers/parser.rs (5)
14-17: Clean implementation of DojoParser.Ohayo sensei! Using a struct without fields as a namespace for parsing functions is a good approach for organizing related functions.
19-33: Parser implementation for structs looks good.The method correctly parses a token stream and finds the first ItemStruct syntax node.
36-50: Parser implementation for modules looks good.Similar to the struct parser, this correctly finds the first ItemModule syntax node.
54-67: Parser implementation for inline args looks good.Appropriately uses parse_token_stream_expr for parsing expressions and finds the first ExprParenthesized node.
70-109: Well-implemented member parsing with ordering validation.The parsing logic correctly enforces that key members must precede non-key members while collecting all errors rather than stopping at the first one. Good handling of the parsing state with
parsing_keys.crates/dojo/core-test/src/tests/helpers/event.cairo (4)
7-22: Well-defined base event structs for testing.Ohayo sensei! These event structs provide a good foundation for testing various upgrade scenarios with both simple and complex field types.
24-39: Clear setup for event upgrade testing.The deploy_world_for_event_upgrades function creates a well-organized test environment with all the necessary event resources.
41-44: Good explanation for contract naming approach.Clear documentation explaining why the contracts are written without the dojo::event attribute and the prefix naming convention.
78-105: Well-structured test contracts for various upgrade scenarios.Ohayo sensei! The event contract implementations systematically cover different upgrade scenarios with consistent implementations of IDeployedResource and IStoredResource interfaces.
Also applies to: 108-135, 138-165, 168-195, 198-225, 228-255
crates/dojo/core-test/src/tests/expanded/selector_attack.cairo (3)
1-3: Good documentation of file purpose.Ohayo sensei! Clear explanation that this file contains manually expanded code for testing permissioned contract deployment.
4-19: Minimal attacker contract implementation.The contract provides just enough implementation to test deployment permissions with a fixed resource name.
22-64: Complete model implementation for testing resource registration.This contract thoroughly implements all required interfaces (IDeployedResource, IStoredResource, and IModel) with minimal but sufficient implementations to test resource registration security.
crates/dojo/core-test/src/tests/world/event.cairo (1)
77-82: Ohayo sensei – shared global test state can bleed across cases
set_account_addressandset_caller_addressmutate global test context but are never reset.
If another test happens to run after one that leftbobas caller, surprising authorisations could succeed or fail. Consider wrapping those mutations in a RAII‑style guard (CallerGuard) or at least call areset_env()in adefer/dropblock.crates/dojo/core-test/src/tests/world/world.cairo (1)
221-231:❌ Incorrect review comment
Ohayo sensei – hard‑coded caller id may rot, build it dynamically
expected_panicembeds2827, but the value must match whateverset_caller_addressreceives.
Iftest_address()changes or the helper picks a different account, the string mismatch will give a false‑negative.
Recommend formatting the message at runtime:- let expected_panic: ByteArray = - "Only the world can init contract `test_contract`, but caller is `2827`"; + let expected_panic: ByteArray = array!( + "Only the world can init contract `test_contract`, but caller is `" , + serialize(@starknet::to_felt(caller)), "`" + ).concat(); // pseudo‑code, adjust to ByteArray APIEnsures the assertion remains correct even if the test harness allocates a new caller.
🏁 Script executed:
#!/bin/bash # Search for set_caller_address occurrences in the test suite rg "set_caller_address" -n crates/dojo/core-test/src # Search for test_address helper in the repository rg "test_address" -n . # Search for the literal 2827 to see where it originates rg "2827" -n .Length of output: 73876
Ohayo sensei – this literal is intentional, not rot
In
crates/dojo/core-test/src/tests/world/world.cairothe test deliberately callssnf_utils::set_caller_address(2827.try_into().unwrap());and then asserts exactly “…caller is
2827”. Because both the panic message and the setter use the same hard‑coded value, there’s no drift risk from the test harness picking a different account. No dynamic formatting is needed here.Likely an incorrect or invalid review comment.
crates/dojo/core-test/src/tests/model/model.cairo (1)
64-77: Ohayo sensei – unused models Foo2 / Foo3 invite bit‑rot
The namespace registersFoo2andFoo3, yet no test exercises them. Either add dedicated tests (even a simple read/write round‑trip) or drop them fromnamespace_def()to keep the suite lean.crates/dojo/core-test/src/tests/world/contract.cairo (1)
237-255: Consider verifying caller/account impersonation logicIn this malicious‑contract scenario the account address is still set to
bobbutcaller_addressbecomes the malicious contract. Ifworldchecks onlyget_caller_address(and not the account) the exploit succeeds. Double‑check the upstreamonly_account_owner/msg::sender()semantics to ensure both identity facets are validated.crates/dojo/macros/src/derives/introspect/enums.rs (1)
125-134: Dynamic‑size flag not updated after inserting selector
is_dynamic_sizeis forwarded unchanged after you add the selector word. While the selector itself is static, the cumulative‑size calculation could now be wrong for previously‐empty variants. Double‑checkbuild_size_function_bodyto ensure it recomputesis_dynamic_sizeinternally; otherwise pass an updated flag here.
| #[test] | ||
| #[available_gas(l2_gas: 1500000)] | ||
| fn test_database_different_keys() { | ||
| let mut values = ArrayTrait::new(); | ||
| values.append(0x1); | ||
| values.append(0x2); | ||
|
|
||
| let mut other = ArrayTrait::new(); | ||
| other.append(0x3); | ||
| other.append(0x4); | ||
|
|
||
| set('table', 'key', values.span(), 0, [251, 251].span()); | ||
| set('table', 'other', other.span(), 0, [251, 251].span()); | ||
| let res = get('table', 'key', [251, 251].span()); | ||
| let other_res = get('table', 'other', [251, 251].span()); | ||
|
|
||
| assert(res.len() == values.len(), 'Lengths not equal'); | ||
| assert(res.at(0) == values.at(0), 'Values different at `key`!'); | ||
| assert(other_res.at(0) == other_res.at(0), 'Values different at `other`!'); | ||
| assert(other_res.at(0) != res.at(0), 'Values the same for different!'); | ||
| } |
There was a problem hiding this comment.
Same self-comparison issue in different keys test
Similar to the previous test, there's an issue at line 59 where you're comparing a value with itself, which will always pass regardless of the actual value.
- assert(other_res.at(0) == other_res.at(0), 'Values different at `other`!');
+ assert(other_res.at(0) == other.at(0), 'Values different at `other`!');📝 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.
| #[test] | |
| #[available_gas(l2_gas: 1500000)] | |
| fn test_database_different_keys() { | |
| let mut values = ArrayTrait::new(); | |
| values.append(0x1); | |
| values.append(0x2); | |
| let mut other = ArrayTrait::new(); | |
| other.append(0x3); | |
| other.append(0x4); | |
| set('table', 'key', values.span(), 0, [251, 251].span()); | |
| set('table', 'other', other.span(), 0, [251, 251].span()); | |
| let res = get('table', 'key', [251, 251].span()); | |
| let other_res = get('table', 'other', [251, 251].span()); | |
| assert(res.len() == values.len(), 'Lengths not equal'); | |
| assert(res.at(0) == values.at(0), 'Values different at `key`!'); | |
| assert(other_res.at(0) == other_res.at(0), 'Values different at `other`!'); | |
| assert(other_res.at(0) != res.at(0), 'Values the same for different!'); | |
| } | |
| #[test] | |
| #[available_gas(l2_gas: 1500000)] | |
| fn test_database_different_keys() { | |
| let mut values = ArrayTrait::new(); | |
| values.append(0x1); | |
| values.append(0x2); | |
| let mut other = ArrayTrait::new(); | |
| other.append(0x3); | |
| other.append(0x4); | |
| set('table', 'key', values.span(), 0, [251, 251].span()); | |
| set('table', 'other', other.span(), 0, [251, 251].span()); | |
| let res = get('table', 'key', [251, 251].span()); | |
| let other_res = get('table', 'other', [251, 251].span()); | |
| assert(res.len() == values.len(), 'Lengths not equal'); | |
| assert(res.at(0) == values.at(0), 'Values different at `key`!'); | |
| assert(other_res.at(0) == other.at(0), 'Values different at `other`!'); | |
| assert(other_res.at(0) != res.at(0), 'Values the same for different!'); | |
| } |
| #[test] | ||
| #[available_gas(l2_gas: 1500000)] | ||
| fn test_database_different_tables() { | ||
| let mut values = ArrayTrait::new(); | ||
| values.append(0x1); | ||
| values.append(0x2); | ||
|
|
||
| let mut other = ArrayTrait::new(); | ||
| other.append(0x3); | ||
| other.append(0x4); | ||
|
|
||
| set('first', 'key', values.span(), 0, [251, 251].span()); | ||
| set('second', 'key', other.span(), 0, [251, 251].span()); | ||
| let res = get('first', 'key', [251, 251].span()); | ||
| let other_res = get('second', 'key', [251, 251].span()); | ||
|
|
||
| assert(res.len() == values.len(), 'Lengths not equal'); | ||
| assert(res.at(0) == values.at(0), 'Values different at `first`!'); | ||
| assert(other_res.at(0) == other_res.at(0), 'Values different at `second`!'); | ||
| assert(other_res.at(0) != res.at(0), 'Values the same for different!'); | ||
| } |
There was a problem hiding this comment.
Fix self-comparison in assertion
Ohayo sensei! There's an issue in the assertion at line 37 - you're comparing the value with itself (other_res.at(0) == other_res.at(0)), which will always be true. This doesn't test what you likely intended.
- assert(other_res.at(0) == other_res.at(0), 'Values different at `second`!');
+ assert(other_res.at(0) == other.at(0), 'Values different at `second`!');📝 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.
| #[test] | |
| #[available_gas(l2_gas: 1500000)] | |
| fn test_database_different_tables() { | |
| let mut values = ArrayTrait::new(); | |
| values.append(0x1); | |
| values.append(0x2); | |
| let mut other = ArrayTrait::new(); | |
| other.append(0x3); | |
| other.append(0x4); | |
| set('first', 'key', values.span(), 0, [251, 251].span()); | |
| set('second', 'key', other.span(), 0, [251, 251].span()); | |
| let res = get('first', 'key', [251, 251].span()); | |
| let other_res = get('second', 'key', [251, 251].span()); | |
| assert(res.len() == values.len(), 'Lengths not equal'); | |
| assert(res.at(0) == values.at(0), 'Values different at `first`!'); | |
| assert(other_res.at(0) == other_res.at(0), 'Values different at `second`!'); | |
| assert(other_res.at(0) != res.at(0), 'Values the same for different!'); | |
| } | |
| #[test] | |
| #[available_gas(l2_gas: 1500000)] | |
| fn test_database_different_tables() { | |
| let mut values = ArrayTrait::new(); | |
| values.append(0x1); | |
| values.append(0x2); | |
| let mut other = ArrayTrait::new(); | |
| other.append(0x3); | |
| other.append(0x4); | |
| set('first', 'key', values.span(), 0, [251, 251].span()); | |
| set('second', 'key', other.span(), 0, [251, 251].span()); | |
| let res = get('first', 'key', [251, 251].span()); | |
| let other_res = get('second', 'key', [251, 251].span()); | |
| assert(res.len() == values.len(), 'Lengths not equal'); | |
| assert(res.at(0) == values.at(0), 'Values different at `first`!'); | |
| - assert(other_res.at(0) == other_res.at(0), 'Values different at `second`!'); | |
| + assert(other_res.at(0) == other.at(0), 'Values different at `second`!'); | |
| assert(other_res.at(0) != res.at(0), 'Values the same for different!'); | |
| } |
| for n in root_node.descendants(&db) { | ||
| match n.kind(&db) { | ||
| ItemStruct => { | ||
| let struct_ast = ast::ItemStruct::from_syntax_node(&db, n); | ||
| return structs::DojoStructIntrospect::process(&db, &struct_ast, is_packed); | ||
| } | ||
| ItemEnum => { | ||
| let enum_ast = ast::ItemEnum::from_syntax_node(&db, n); | ||
| return enums::DojoEnumIntrospect::process(&db, &enum_ast, is_packed); | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei – Only the first struct/enum is processed
process() returns as soon as it meets the first ItemStruct or ItemEnum, silently ignoring the rest of the file. Down the road this will hurt users who derive Introspect on multiple items in one module. Consider accumulating results and merging the token streams (e.g., quote! { #first #second }).
| match type_clause.ty(db) { | ||
| Expr::Path(path) => { | ||
| let path_type = path.as_syntax_node().get_text(db).trim().to_string(); | ||
| build_item_ty_from_type(&path_type) | ||
| } | ||
| Expr::Tuple(expr) => { | ||
| let tuple_type = expr.as_syntax_node().get_text(db).trim().to_string(); | ||
| build_tuple_ty_from_type(&tuple_type) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei – Non‑exhaustive match may reject valid Cairo 2.11 types
Expr::Array, Expr::Unary, or future type syntaxes fall through to the "ERROR" branch. That will cause false negatives once users feed in, e.g., Array<u128> literal syntax. A safer pattern is to call diagnostics.report() here or recursively delegate, so new variants degrade gracefully.
| fn test_writer_not_registered_resource() { | ||
| let world = deploy_world(); | ||
| let world = world.dispatcher; | ||
|
|
||
| // 42 is not a registered resource ID | ||
| !world.is_writer(42, 69.try_into().unwrap()); | ||
| } |
There was a problem hiding this comment.
Missing assertion – the test currently does nothing, sensei
The statement
!world.is_writer(42, 69.try_into().unwrap());merely computes a boolean and discards the result; the test will always pass, even if is_writer were to return true.
- // 42 is not a registered resource ID
- !world.is_writer(42, 69.try_into().unwrap());
+ // 42 is not a registered resource ID
+ assert(
+ !world.is_writer(42, 69.try_into().unwrap()),
+ 'non‑registered resource erroneously reported as writer',
+ );| format!( | ||
| "dojo::meta::introspect::Introspect::<{}>::layout()", | ||
| item_type | ||
| ) | ||
| } |
There was a problem hiding this comment.
Array element layout is built for the wrong type
Inside build_array_layout_from_type the fallback branch should reference the array item type, not the whole Array<T> wrapper.
Generating Introspect::<Array<u128>>::layout() makes the macro look for a (non‑existent) implementation for the wrapped type and breaks compilation.
- format!(
- "dojo::meta::introspect::Introspect::<{}>::layout()",
- item_type
- )
+ format!(
+ "dojo::meta::introspect::Introspect::<{}>::layout()",
+ array_item_type
+ )📝 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.
| format!( | |
| "dojo::meta::introspect::Introspect::<{}>::layout()", | |
| item_type | |
| ) | |
| } | |
| format!( | |
| "dojo::meta::introspect::Introspect::<{}>::layout()", | |
| array_item_type | |
| ) | |
| } |
| fn handle_init_fn( | ||
| &mut self, | ||
| db: &SimpleParserDatabase, | ||
| fn_ast: &ast::FunctionWithBody, | ||
| ) -> TokenStream { | ||
| self.has_init = true; | ||
|
|
||
| if let OptionReturnTypeClause::ReturnTypeClause(_) = | ||
| fn_ast.declaration(db).signature(db).ret_ty(db) | ||
| { | ||
| self.diagnostics.push_error(format!( | ||
| "The {DOJO_INIT_FN} function cannot have a return type." | ||
| )); | ||
| } | ||
|
|
||
| let fn_decl = fn_ast.declaration(db).as_syntax_node(); | ||
| let fn_decl = SyntaxNodeWithDb::new(&fn_decl, db); | ||
|
|
||
| quote! { | ||
| #[abi(per_item)] | ||
| #[generate_trait] | ||
| pub impl IDojoInitImpl of IDojoInit { | ||
| #[external(v0)] | ||
| #fn_decl { | ||
| if starknet::get_caller_address() != self.world_provider.world_dispatcher().contract_address { | ||
| core::panics::panic_with_byte_array( | ||
| @format!( | ||
| "Only the world can init contract `{}`, but caller is `{:?}`", | ||
| self.dojo_name(), | ||
| starknet::get_caller_address() | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Init handler drops the user‑supplied body
handle_init_fn lifts only the declaration (fn_decl) into the generated impl and forgets to re‑inject the original function body. As a result, any initialisation logic authored by the contract writer is silently discarded.
let fn_decl = fn_ast.declaration(db).as_syntax_node();
let fn_decl = SyntaxNodeWithDb::new(&fn_decl, db);
+ let fn_body = fn_ast.body(db).as_syntax_node();
+ let fn_body = SyntaxNodeWithDb::new(&fn_body, db);
@@
#[external(v0)]
- #fn_decl {
+ #fn_decl {
if starknet::get_caller_address() != self.world_provider.world_dispatcher().contract_address {
core::panics::panic_with_byte_array(
@format!(
"Only the world can init contract `{}`, but caller is `{:?}`",
self.dojo_name(),
starknet::get_caller_address()
)
);
}
+ #fn_body // preserve user code
}Without this fix, contracts that rely on custom initialisation will compile but behave unexpectedly at runtime.
| pub fn deploy_contract(class_hash: felt252, calldata: Span<felt252>) -> ContractAddress { | ||
| let (contract, _) = starknet::syscalls::deploy_syscall( | ||
| class_hash.try_into().unwrap(), 0, calldata, false, | ||
| ) | ||
| .unwrap(); | ||
| contract | ||
| } |
There was a problem hiding this comment.
Avoid unwrap() on low–level syscalls
Ohayo sensei! A failed deploy_syscall (e.g. out‑of‑gas, invalid class hash) will currently panic the whole test runner via the unwrap(). Propagate the Result upward or turn it into an explicit panic!(…) with a descriptive message so you can see what went wrong more quickly.
-let (contract, _) = starknet::syscalls::deploy_syscall(
- class_hash.try_into().unwrap(), 0, calldata, false,
-).unwrap();
+let (contract, _) = starknet::syscalls::deploy_syscall(
+ class_hash.try_into().expect("Invalid class‑hash felt252 → ClassHash"),
+ 0,
+ calldata,
+ false,
+).expect("Contract deployment failed");📝 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.
| pub fn deploy_contract(class_hash: felt252, calldata: Span<felt252>) -> ContractAddress { | |
| let (contract, _) = starknet::syscalls::deploy_syscall( | |
| class_hash.try_into().unwrap(), 0, calldata, false, | |
| ) | |
| .unwrap(); | |
| contract | |
| } | |
| pub fn deploy_contract(class_hash: felt252, calldata: Span<felt252>) -> ContractAddress { | |
| let (contract, _) = starknet::syscalls::deploy_syscall( | |
| class_hash.try_into().expect("Invalid class‑hash felt252 → ClassHash"), | |
| 0, | |
| calldata, | |
| false, | |
| ) | |
| .expect("Contract deployment failed"); | |
| contract | |
| } |
| let mut first_namespace = Option::None; | ||
|
|
||
| for ns in namespaces_defs { | ||
| let namespace = ns.namespace.clone(); | ||
| world.register_namespace(namespace.clone()); | ||
|
|
||
| if first_namespace.is_none() { | ||
| first_namespace = Option::Some(namespace.clone()); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Gracefully handle the “no‑namespace” edge case
first_namespace.unwrap() will panic if namespaces_defs is empty. Either assert a non‑empty slice at the API boundary or fall back to a default value.
-if first_namespace.is_none() {
- first_namespace = Option::Some(namespace.clone());
-}
+if first_namespace.is_none() {
+ first_namespace = Option::Some(namespace.clone());
+}
…
-WorldStorageTrait::new(world, @first_namespace.unwrap())
+let default_ns = first_namespace.unwrap_or_else(|| panic!("spawn_test_world: at least one namespace required"));
+WorldStorageTrait::new(world, @default_ns)🏁 Script executed:
#!/bin/bash
# Locate the first_namespace definition and usage
rg -n "first_namespace" crates/dojo/core-test/src/world.cairo
# Find where WorldStorageTrait::new is called
rg -n "WorldStorageTrait::new" crates/dojo/core-test/src/world.cairoLength of output: 418
Ohayo sensei,
Enforce non‑empty namespaces_defs to avoid unwrap() panic
The call to first_namespace.unwrap() on line 183 will panic if namespaces_defs is empty. Add an explicit assertion at the API boundary so this can’t happen unexpectedly:
--- a/crates/dojo/core-test/src/world.cairo
+++ b/crates/dojo/core-test/src/world.cairo
@@ func spawn_test_world{syscall_ptr, pedersen_ptr, range_check_ptr}(
- // …
+ // Ensure at least one namespace is provided.
+ assert(namespaces_defs.len() > 0, "spawn_test_world: at least one namespace required");
let mut first_namespace = Option::None;
for ns in namespaces_defs {
// …
}
WorldStorageTrait::new(world, @first_namespace.unwrap())Alternatively, replace the final unwrap() with an explicit match so you get a clear panic message:
let default_ns = match first_namespace {
Option::Some(ns) => ns,
Option::None => panic("spawn_test_world: at least one namespace required"),
};
WorldStorageTrait::new(world, @default_ns)• Location: crates/dojo/core-test/src/world.cairo
• Line 183: WorldStorageTrait::new(world, @first_namespace.unwrap())
| let primitives = [ | ||
| 'bool', 'u8', 'u16', 'u32', 'u64', 'u128', 'u256', 'i8', 'i16', 'i32', 'i64', 'i128', | ||
| 'felt252', 'ClassHash', 'ContractAddress', 'EthAddress', | ||
| ] | ||
| .span(); | ||
|
|
||
| let mut allowed_upgrades: Span<(felt252, Span<felt252>)> = [ | ||
| ('bool', ['felt252'].span()), | ||
| ('u8', ['u16', 'u32', 'usize', 'u64', 'u128', 'felt252'].span()), | ||
| ('u16', ['u32', 'usize', 'u64', 'u128', 'felt252'].span()), | ||
| ('u32', ['usize', 'u64', 'u128', 'felt252'].span()), ('u128', ['felt252'].span()), |
There was a problem hiding this comment.
usize appears in upgrade table but is missing from the primitive list
The primitives array does not include 'usize', yet several upgrade rules reference it. This silently skips the 'usize' cases in the nested loop, so those rules are never exercised.
-let primitives = [
- 'bool', 'u8', 'u16', 'u32', 'u64', 'u128', 'u256', 'i8', 'i16', 'i32', 'i64', 'i128',
- 'felt252', 'ClassHash', 'ContractAddress', 'EthAddress',
-].span();
+let primitives = [
+ 'bool', 'u8', 'u16', 'u32', 'u64', 'usize', 'u128', 'u256', 'i8', 'i16', 'i32', 'i64', 'i128',
+ 'felt252', 'ClassHash', 'ContractAddress', 'EthAddress',
+].span();This will ensure the 'usize' upgrade paths are actually validated.
📝 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.
| let primitives = [ | |
| 'bool', 'u8', 'u16', 'u32', 'u64', 'u128', 'u256', 'i8', 'i16', 'i32', 'i64', 'i128', | |
| 'felt252', 'ClassHash', 'ContractAddress', 'EthAddress', | |
| ] | |
| .span(); | |
| let mut allowed_upgrades: Span<(felt252, Span<felt252>)> = [ | |
| ('bool', ['felt252'].span()), | |
| ('u8', ['u16', 'u32', 'usize', 'u64', 'u128', 'felt252'].span()), | |
| ('u16', ['u32', 'usize', 'u64', 'u128', 'felt252'].span()), | |
| ('u32', ['usize', 'u64', 'u128', 'felt252'].span()), ('u128', ['felt252'].span()), | |
| let primitives = [ | |
| 'bool', 'u8', 'u16', 'u32', 'u64', 'usize', 'u128', 'u256', 'i8', 'i16', 'i32', 'i64', 'i128', | |
| 'felt252', 'ClassHash', 'ContractAddress', 'EthAddress', | |
| ].span(); |
Add
macrosandcore-testcrates to be integrated into Dojo after bumping to Cairo 2.11.2.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores