feat(core): split Introspect and DojoStore derive attributes #3294
Conversation
WalkthroughOhayo sensei! Added the Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source AST (struct/enum)
participant DojoMacro as DojoStore derive macro
participant Formatter as DojoFormatter
participant Output as Generated impl (TokenStream)
Source->>DojoMacro: derive expansion request
DojoMacro->>Formatter: query generics / helpers
DojoMacro->>DojoMacro: generate serialize/deserialize code
DojoMacro->>Output: return TokenStream (DojoStore impl)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 1
🧹 Nitpick comments (1)
crates/dojo/macros/src/derives/dojo_store.rs (1)
121-121: Document the variant indexing schemeThe implementation uses 1-based indexing for enum variants, with index 0 reserved for the default case. This non-standard indexing scheme should be documented to avoid confusion.
Consider adding a comment explaining the indexing:
let variant_name = variant.name(db).text(db).to_string(); let full_variant_name = format!("{enum_name}::{variant_name}"); + // Variants use 1-based indexing, with 0 reserved for Default let variant_index = index + 1;Also applies to: 183-183
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
examples/benchmark/Scarb.lockis excluded by!**/*.lockexamples/game-lib/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (17)
crates/dojo/core-tests/src/tests/benches/bench_data.cairo(2 hunks)crates/dojo/core-tests/src/tests/helpers/helpers.cairo(6 hunks)crates/dojo/core-tests/src/tests/model/model.cairo(3 hunks)crates/dojo/core-tests/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/macros/src/attributes/model.rs(2 hunks)crates/dojo/macros/src/constants.rs(1 hunks)crates/dojo/macros/src/derives/dojo_store.rs(1 hunks)crates/dojo/macros/src/derives/introspect/enums.rs(2 hunks)crates/dojo/macros/src/derives/introspect/generics.rs(0 hunks)crates/dojo/macros/src/derives/introspect/mod.rs(0 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(2 hunks)crates/dojo/macros/src/derives/mod.rs(2 hunks)crates/dojo/macros/src/helpers/formatter.rs(2 hunks)crates/dojo/world/src/contracts/abigen/model.rs(16 hunks)crates/dojo/world/src/contracts/abigen/world.rs(97 hunks)examples/spawn-and-move/src/actions.cairo(3 hunks)examples/spawn-and-move/src/models.cairo(3 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/macros/src/derives/introspect/mod.rs
- crates/dojo/macros/src/derives/introspect/generics.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code...
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Applied to files:
crates/dojo/macros/src/constants.rsexamples/spawn-and-move/src/models.cairocrates/dojo/macros/src/derives/introspect/enums.rscrates/dojo/world/src/contracts/abigen/model.rscrates/dojo/core-tests/src/tests/model/model.cairocrates/dojo/macros/src/derives/introspect/structs.rscrates/dojo/core-tests/src/tests/helpers/helpers.cairocrates/dojo/world/src/contracts/abigen/world.rscrates/dojo/macros/src/helpers/formatter.rscrates/dojo/core-tests/src/tests/storage/dojo_store.cairocrates/dojo/macros/src/derives/dojo_store.rscrates/dojo/core-tests/src/tests/benches/bench_data.cairo
📚 Learning: in the cairo codebase for the dojo project, within `crates/dojo/core/src/world/storage.cairo`, lengt...
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`.
Applied to files:
examples/spawn-and-move/src/actions.cairocrates/dojo/macros/src/attributes/model.rsexamples/spawn-and-move/src/models.cairocrates/dojo/macros/src/derives/introspect/enums.rscrates/dojo/world/src/contracts/abigen/model.rscrates/dojo/core-tests/src/tests/model/model.cairocrates/dojo/core-tests/src/tests/helpers/helpers.cairocrates/dojo/world/src/contracts/abigen/world.rscrates/dojo/core-tests/src/tests/storage/dojo_store.cairocrates/dojo/core-tests/src/tests/benches/bench_data.cairo
📚 Learning: in `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is...
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.530Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
Applied to files:
crates/dojo/macros/src/derives/introspect/enums.rs
🔇 Additional comments (21)
crates/dojo/macros/src/constants.rs (1)
4-4: LGTM! Constant addition aligns with the naming convention.Ohayo sensei! The new
DOJO_STORE_DERIVEconstant follows the existing pattern and correctly defines the identifier for the DojoStore derive macro.examples/spawn-and-move/src/actions.cairo (3)
46-46: Good formatting improvement.Ohayo sensei! Adding a blank line after imports improves code readability.
269-269: Consistent use of Dojo test utilities.The addition of
set_caller_addressimport aligns with using Dojo's testing framework consistently throughout the codebase.
349-349: Proper migration to Dojo test helper.The change from
starknet::testing::set_contract_address(caller)toset_caller_address(caller)maintains the same functionality while using the Dojo-specific test utility, which is more appropriate for this context.crates/dojo/macros/src/attributes/model.rs (2)
9-9: Proper import addition for the new constant.Ohayo sensei! The
DOJO_STORE_DERIVEimport is correctly added to support the new functionality.
190-194: Excellent implementation of auto-derive logic.This implementation correctly ensures that
DojoStoreis automatically included for models when not explicitly specified, which perfectly aligns with the PR objective of maintaining backward compatibility while decoupling the derives. The logic follows the same pattern as the existing Introspect handling.examples/spawn-and-move/src/models.cairo (3)
3-3: Correct addition of DojoStore derive to Direction enum.Ohayo sensei! The
DojoStorederive is properly added to theDirectionenum, enabling it to participate in Dojo's storage system independently of theIntrospectderive.
54-54: Proper DojoStore integration for Vec2 struct.The
DojoStorederive is correctly added to theVec2struct, which already usesIntrospectPacked. This demonstrates the successful decoupling where storage capabilities can be added independently.
74-74: Good DojoStore addition to PlayerItem struct.The
DojoStorederive is appropriately added to thePlayerItemstruct, complementing the existingIntrospectderive and enabling independent storage functionality.crates/dojo/macros/src/derives/mod.rs (2)
5-5: Proper module declaration for DojoStore derive.Ohayo sensei! The
dojo_storemodule is correctly declared alongside the existingintrospectmodule.
24-31: ohayo sensei!DojoStore derive macro implementation verified
- Confirmed
crates/dojo/macros/src/derives/dojo_store.rsexists and definespub(crate) fn process(token_stream) -> ProcMacroResultfollowing the same pattern as other derive macros.Looks great—approving these changes!
crates/dojo/world/src/contracts/abigen/model.rs (1)
1-905: Ohayo sensei! Skipping review of auto-generated file.Based on previous learnings, files in
crates/dojo/world/src/contracts/abigen/are auto-generated and should be excluded from code reviews.crates/dojo/macros/src/derives/introspect/structs.rs (1)
59-71: Ohayo sensei! Clean refactoring to use DojoFormatter utilities.The migration to
DojoFormatter::build_generic_typesandDojoFormatter::build_generic_implsproperly centralizes generic handling logic, aligning with the PR's goal of decoupling Introspect from DojoStore generation.crates/dojo/core-tests/src/tests/model/model.cairo (1)
40-40: Ohayo sensei! Test structures properly updated with explicit DojoStore derive.The addition of
DojoStorederive to test structures (AStruct,MyEnum,EnumWithUnitType,StructWithUnitType) correctly reflects the new requirement for explicit DojoStore derivation on data structures used in Dojo models.Also applies to: 81-81, 110-110, 118-118
crates/dojo/macros/src/derives/introspect/enums.rs (1)
65-71: Ohayo sensei! Consistent refactoring pattern for enum introspection.The enum introspect implementation correctly mirrors the struct changes, using
DojoFormatterutilities and removing embedded DojoStore generation. This maintains architectural consistency across both derive macros.crates/dojo/world/src/contracts/abigen/world.rs (1)
1-5548: Ohayo sensei! Skipping review of auto-generated code.This file is auto-generated by cainome (as indicated in the header comment) and based on our retrieved learnings, code in
crates/dojo/world/src/contracts/abigen/should be excluded from code reviews. The changes appear to be formatting improvements to serialization calls, but since this is generated code, manual review is not necessary.crates/dojo/core-tests/src/tests/helpers/helpers.cairo (1)
12-12: Ohayo! Consistent application of DojoStore derive looks good, sensei!The DojoStore derive has been properly added to all test helper structs and enums that have either Introspect or IntrospectPacked. This aligns perfectly with the PR objective of separating these concerns.
Also applies to: 44-44, 153-153, 180-180, 190-190, 204-204
crates/dojo/core-tests/src/tests/benches/bench_data.cairo (1)
1-1: Ohayo sensei! Benchmark data structures properly updated with DojoStore.All benchmark structs and enums (both regular and packed variants) now derive DojoStore alongside their existing Introspect/IntrospectPacked derives. This maintains consistency across the test suite.
Also applies to: 8-8, 16-16, 23-23, 66-66, 73-73, 81-81, 88-88
crates/dojo/core-tests/src/tests/storage/dojo_store.cairo (1)
3-3: Ohayo! Test types properly equipped with DojoStore derive, sensei!All test types in the DojoStore test file now derive DojoStore, enabling comprehensive testing of:
- Simple types (E, S)
- Complex nested types (SComplex, EComplex)
- Generic types (GenericStruct, GenericEnum)
- Types using generics (UseGenericStruct, UseGenericEnum)
This ensures thorough test coverage for the new DojoStore derive macro implementation.
Also applies to: 11-11, 18-18, 24-24, 32-32, 37-37, 43-43, 49-49
crates/dojo/macros/src/helpers/formatter.rs (1)
1-9: Ohayo sensei! Excellent addition of generic handling utilities!The new
build_generic_typesandbuild_generic_implsmethods provide clean abstractions for handling generics in derive macros. The implementation correctly:
- Extracts only type parameters (filtering out const/lifetime params)
- Formats generic implementations with proper bounds
- Handles the edge case when no generics are present
These utilities effectively support the new DojoStore derive macro's generic handling needs.
Also applies to: 86-134
crates/dojo/macros/src/derives/dojo_store.rs (1)
111-111: Ohayo sensei! Why does enum DojoStore require Serde trait?The enum implementation adds
+core::serde::Serdeas a required trait bound, but this isn't needed for the struct implementation. Is this intentional? If Serde is required for enum serialization internals, it might be worth documenting why.
| fn deserialize(ref values: Span<felt252>) -> Option<{enum_name}{generic_params}> {{ | ||
| let variant = *values.pop_front()?; | ||
| match variant {{ | ||
| 0 => Option::Some(Default::<{enum_name}{generic_params}>::default()), |
There was a problem hiding this comment.
Verify Default trait implementation before using it
The code assumes that Default trait is implemented for the enum when deserializing variant 0, but there's no compile-time guarantee of this.
The code at line 183 uses Default::default() without ensuring the enum implements Default. While the generic bounds include Default for enums with generics (line 112), non-generic enums don't have this requirement. This could lead to compilation errors.
Consider either:
- Always requiring Default trait in the bounds
- Handling the case when Default is not implemented
🤖 Prompt for AI Agents
In crates/dojo/macros/src/derives/dojo_store.rs at line 183, the code uses
Default::default() for the enum without ensuring the Default trait is
implemented for non-generic enums, which can cause compilation errors. To fix
this, modify the generic bounds to always require the Default trait for the enum
type regardless of generics, or add conditional logic to handle cases where
Default is not implemented, ensuring the code only calls Default::default() when
the trait is guaranteed.
There was a problem hiding this comment.
To keep this in the record, renaming DojoLegacyStorage to DojoLegacyStore could help the user understanding better the terminology (at least for the migration). WDYT?
Otherwise, it is great to have the separation between the Introspect system, and the actual serialization interface DojoStore.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/rebuild_test_artifacts.sh (1)
1-2: Enable bash strict mode for safer CI runsAdd strict mode to fail fast on errors and unset vars; this script touches many build steps.
Apply:
#!/bin/bash +set -euo pipefail
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
examples/spawn-and-move/dojo_dev.toml(1 hunks)scripts/rebuild_test_artifacts.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
| @@ -69,7 +69,7 @@ rpc_url = "http://localhost:5050/" | |||
| # Default account for katana with seed = 0 | |||
| account_address = "0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba" | |||
| private_key = "0x1800000000300000180000000000030000000000003006001800006600" | |||
| world_address = "0x24c2f8075163bbdcc44670869f15a282019d0e9b602bbb95b365d8350fc8a41" | |||
| world_address = "0x4051274bdbc46f78e4f8d139dd08061cdf34a479aff7555ee3b37cc052946be" | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid churn from hardcoded world_address; confirm this is the intended dev world
Ohayo sensei — bumping the world_address is fine for local dev, but it tends to cause config churn and accidental mismatches with scripts/tests. If the toolchain supports it, prefer parameterizing via env or an include; otherwise, confirm all references are updated.
Quick checks:
Optionally, if templating is supported in your loader, consider documenting an env-driven approach (e.g., WORLD_ADDRESS) to reduce future diffs.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all world_address entries and their files to spot mismatches
rg -n '^world_address\s*=\s*".*"' -A 0
# Search for the previous address anywhere (if present)
rg -n "0x24c2f8075163bbdcc44670869f15a282019d0e9b602bbb95b365d8350fc8a41" -A 0 || true
# Search for the new address anywhere else to ensure consistency
rg -n "0x4051274bdbc46f78e4f8d139dd08061cdf34a479aff7555ee3b37cc052946be" -A 0Length of output: 3723
Clean up remaining world_address references in dev configs
Ohayo sensei — I see you’ve bumped the dev world_address in examples/spawn-and-move/dojo_dev.toml, but there are still hardcoded references to the old address in other files. To avoid future churn (and mismatches with scripts/tests), please update these or move toward an env-driven parameter.
Files needing attention:
- examples/spawn-and-move/manifest_dev.json
- bin/sozo/tests/test_data/policies.json (all entries)
Suggested actions:
- Replace
"0x24c2f8075163bbdcc44670869f15a282019d0e9b602bbb95b365d8350fc8a41"with"0x4051274bdbc46f78e4f8d139dd08061cdf34a479aff7555ee3b37cc052946be"in the above files - Or parameterize via an env var (e.g.
WORLD_ADDRESS) in your loader or a shared include to keep these in sync
| @@ -32,7 +32,7 @@ if [ ! -f /tmp/katana ]; then | |||
| fi | |||
|
|
|||
| # Generates the database for testing by migrating the spawn and move example. | |||
| KATANA_RUNNER_BIN=/Users/glihm/cgg/katana/target/release/katana cargo generate-test-db | |||
| cargo generate-test-db | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Restore KATANA_RUNNER_BIN passthrough or remove now-dead /tmp/katana logic
Ohayo sensei — by dropping the inline KATANA_RUNNER_BIN assignment, the copy-to-/tmp/katana block (Lines 25-32) becomes unused here. If cargo generate-test-db expects KATANA_RUNNER_BIN, this is a behavior change; if it looks up katana via PATH, the /tmp copy is dead code.
Proposed minimal fix (keeps current behavior, uses /tmp/katana by default, allows override via env):
-cargo generate-test-db
+KATANA_RUNNER_BIN="${KATANA_RUNNER_BIN:-/tmp/katana}" cargo generate-test-dbTo verify whether KATANA_RUNNER_BIN is still consumed anywhere and how generate-test-db is invoked, run:
🏁 Script executed:
#!/bin/bash
# Find references to KATANA_RUNNER_BIN and generate-test-db usage across the repo
set -euo pipefail
rg -n "KATANA_RUNNER_BIN|generate-test-db" -A 2Length of output: 806
Remove dead /tmp/katana copy and unused KATANA_RUNNER_BIN logic
Ohayo sensei — it looks like xtask-generate-test-db doesn’t read the KATANA_RUNNER_BIN env var (it simply invokes katana via $PATH), so the block in scripts/rebuild_test_artifacts.sh that copies a runner into /tmp/katana and the inlined assignment you dropped are now no-ops. Let’s clean this up:
• In scripts/rebuild_test_artifacts.sh, delete lines 25–32 (the copy/chmod of /tmp/katana and any now-unused KATANA_RUNNER_BIN setup).
• Leave the invocation on line 35 as just:
cargo generate-test-dbThis will remove dead code without altering current behavior.
🤖 Prompt for AI Agents
In scripts/rebuild_test_artifacts.sh around lines 25–35, remove the dead
`/tmp/katana` copy/chmod block and any now-unused KATANA_RUNNER_BIN setup found
on lines 25–32 (the copy to /tmp/katana and related inlined assignment), and
keep the invocation on line 35 as a single call to `cargo generate-test-db`;
this removes the no-op runner-installation logic while preserving current
behavior.
Description
At the moment,
DojoStoretrait is implemented whenIntrospectorIntrospectPackedis provided. Unfortunately, that meansIntrospectfeature cannot be used withoutDojoStore, leading to generate uselessDojoStoreimpls for events for example.This PR aims to decouple
IntrospectandDojoStorebut, at the same time, ease the migration from an old Dojo version.That means:
dojo::modelattribute is set,DojoStore(andIntrospect) are automatically generated if not specified,DojoStorefor events or data structures used in events only,DojoStorederive attribute has to be specify on data structures used in Dojo models.Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Chores
Style
Bug Fixes