feat: add systems to manifest #2642
Conversation
A system is any function found in a dojo contract that is exposed in the ABI with the state mutability as external. Event if Dojo per se doesn't modify the storage of the contract, it's required that the user explicitely identify the systems with ref self.
WalkthroughOhayo, sensei! This pull request introduces several modifications across multiple files, primarily enhancing the Changes
Possibly related issues
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/dojo/world/src/local/mod.rs (1)
70-70: The default implementation is well-crafted, sensei!The empty vector initialization is appropriate. However, consider adding a capacity hint if you know the typical number of entrypoints to optimize allocations.
- entrypoints: vec![], + entrypoints: Vec::with_capacity(10), // Adjust capacity based on typical usagecrates/dojo/world/src/local/artifact_to_local.rs (2)
234-252: Consider adding documentation for system identification criteria, sensei!The implementation is elegant and handles nested ABI structures well. However, it would benefit from documentation explaining:
- What qualifies as a system
- Why external state mutability is the key criteria
- Examples of typical system functions
Add documentation like this:
+/// Extracts system functions from ABI entries. +/// +/// A system is any function that: +/// - Has external state mutability +/// - Is exposed in the contract's ABI +/// +/// # Arguments +/// * `abi` - Slice of ABI entries to process +/// +/// # Returns +/// Vector of system function names fn systems_from_abi(abi: &[AbiEntry]) -> Vec<String> {
306-351: Consider making the tests more maintainable, sensei!While the tests provide good coverage, the hardcoded list of expected systems in
test_load_world_from_directorymight be fragile to maintain. Consider:
- Moving the expected systems list to a constant or test fixture
- Adding assertions that verify system properties rather than exact names
- Adding test cases for edge cases (empty ABI, no external functions)
Example approach:
#[test] fn test_load_world_from_directory() { // ... existing setup ... // Verify essential properties instead of exact list assert!(!world.entrypoints.is_empty()); assert!(world.entrypoints.contains(&"upgrade".to_string())); assert!(world.entrypoints.iter().all(|s| !s.is_empty())); }crates/dojo/world/src/diff/mod.rs (1)
40-41: Ohayo sensei! The new field looks good, but could use better documentation.The field declaration is well-structured, but consider adding more descriptive documentation to explain what constitutes an entrypoint and its significance in the world status.
/// The status of the world. pub status: WorldStatus, - /// The entrypoints of the world. + /// The entrypoints of the world, representing external functions exposed in the ABI + /// that can be called to interact with the world contract. pub entrypoints: Vec<String>,examples/simple/manifest_dev.json (1)
Line range hint
109-233: Excellent addition of batch operation support!The ABI has been enhanced with new struct definitions and functions that enable efficient batch operations:
emit_eventsfor batch event emissionentitiesfor batch entity retrievalset_entitiesfor batch entity updatesdelete_entitiesfor batch entity deletionThese additions will significantly improve performance for bulk operations.
Consider documenting the performance characteristics and recommended batch sizes for these operations in the technical documentation.
Also applies to: 444-604
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
crates/dojo/world/src/diff/manifest.rs(11 hunks)crates/dojo/world/src/diff/mod.rs(3 hunks)crates/dojo/world/src/local/artifact_to_local.rs(7 hunks)crates/dojo/world/src/local/mod.rs(2 hunks)examples/simple/manifest_dev.json(16 hunks)
🔇 Additional comments (13)
crates/dojo/world/src/local/mod.rs (1)
39-41: Ohayo! The entrypoints field addition looks solid, sensei!
The field type and documentation clearly express the purpose of storing transaction-targetable world entrypoints.
crates/dojo/world/src/diff/manifest.rs (4)
36-37: Ohayo! The entrypoints field addition looks good, sensei!
The addition of entrypoints to WorldContract and its initialization in Manifest::new aligns well with the PR objectives of exposing systems in the manifest.
Also applies to: 116-116
58-60: Clean and consistent selector field additions across resource types!
The addition of the selector field with UfeHex serialization is well-implemented across all resource types (Contract, Model, Event). This enhancement properly supports the manifest's resource identification capabilities.
Also applies to: 76-78, 92-94
Line range hint 162-173: Clean implementation of selector initialization across conversion functions!
The selector initialization is consistently implemented across all resource conversion functions, maintaining proper error handling and matching the manifest structure requirements.
Also applies to: 190-201, 217-228
162-162: Verify the dojo_selector() implementation.
The conversion functions consistently use resource.dojo_selector() for selector initialization. Let's verify its implementation to ensure correct selector generation.
Also applies to: 173-173, 190-190, 201-201, 217-217, 228-228
✅ Verification successful
Implementation of dojo_selector() is consistent and well-structured
The verification shows that dojo_selector() is properly implemented across all resource types:
- Common resources use
naming::compute_selector_from_names(&self.namespace, &self.name) - Namespace resources use
naming::compute_bytearray_hash(&name)as a special case - Contract/Model/Event resources delegate to their common implementation
- ResourceDiff correctly forwards to the local resource implementation
The selector initialization in the conversion functions is correctly using this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dojo_selector implementation
rg -A 5 "fn dojo_selector"
Length of output: 3585
crates/dojo/world/src/local/artifact_to_local.rs (2)
10-10: Ohayo! Import looks good, sensei!
The addition of StateMutability is well-placed and necessary for identifying external systems from ABI entries.
34-34: Implementation aligns well with PR objectives, sensei!
The changes effectively collect and store systems from both world and contract ABIs. The separation between world entrypoints and contract systems is clean and well-structured.
Let's verify the contract systems are properly collected:
Also applies to: 60-60, 66-66, 83-83, 168-168
✅ Verification successful
Contract systems are properly collected and handled, sensei!
The verification shows that systems are correctly extracted from both world and contract ABIs through the systems_from_abi function. The implementation:
- Extracts systems from external functions in the ABI
- Properly handles both world entrypoints and contract systems
- Correctly processes contract resources with their respective namespaces
The code at line 34 initializing world_entrypoints is part of a well-structured flow where:
- World ABI systems are collected into
world_entrypoints - Contract systems are separately collected and handled with proper namespace association
- Both are integrated into the final WorldLocal structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that contract systems are properly collected from ABI
# Expected: Find instances where systems are extracted from contract ABIs
rg -A 3 "ResourceType::Contract.*systems.*clone" crates/dojo/world/src/
Length of output: 73
Script:
#!/bin/bash
# Let's try a broader search for contract systems handling
rg -A 5 "ResourceType::Contract" crates/dojo/world/src/
# Also search for systems collection in the artifact_to_local file
rg -A 5 "systems_from_abi" crates/dojo/world/src/local/artifact_to_local.rs
# Check the struct definition and implementation
rg -A 10 "impl WorldLocal" crates/dojo/world/src/local/
Length of output: 7399
crates/dojo/world/src/diff/mod.rs (2)
80-80: LGTM! Proper initialization from local world.
The entrypoints are correctly preserved from the local world during the NotDeployed status initialization.
117-117: Consider handling entrypoint differences between local and remote.
Currently, we always use local entrypoints even when the world is synced with remote. Consider if we should compare local and remote entrypoints to detect changes, similar to how we handle other resource differences.
Let's check if there are any remote entrypoint handling patterns in the codebase:
examples/simple/manifest_dev.json (4)
3-29: Ohayo sensei! The world section changes look good!
The world section has been properly updated with new class hash, address, and a comprehensive list of entrypoints that cover all essential world management functions.
Line range hint 1246-2111: Well-structured contract definitions with explicit system declarations!
The contracts are properly defined with:
- Unique selectors for each contract
- Explicit listing of available systems
- Consistent inclusion of the upgrade system
- Additional systems (system_1 through system_4) for ns-c1 and ns2-c1 contracts
This aligns perfectly with the PR objective of explicitly identifying systems in the manifest.
2117-2125: Please verify the empty model members.
Both models have empty members arrays. While this might be intentional, it's worth verifying if this is the expected state.
#!/bin/bash
# Description: Check if empty model members are expected
# Look for model definitions or references in the codebase
# Search for model definitions
echo "Searching for model definitions..."
rg -l "struct.*M\s*{" --type cairo
# Search for model member references
echo "Searching for model member references..."
rg -l "M\s*{\s*\w+\s*:" --type cairo2131-2139: Please verify the empty event members.
Both events have empty members arrays. While this might be intentional, it's worth verifying if this is the expected state.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2642 +/- ##
==========================================
+ Coverage 57.07% 57.17% +0.09%
==========================================
Files 399 399
Lines 49661 49731 +70
==========================================
+ Hits 28346 28434 +88
+ Misses 21315 21297 -18 ☔ View full report in Codecov by Sentry. |
A system is any function found in a dojo contract that is exposed in the ABI with the state mutability as external.
Event if Dojo per se doesn't modify the storage of the contract, it's required that the user explicitely identify the systems with ref self.
Also add selectors of resources in the manifest.
Summary by CodeRabbit
New Features
entrypointsfield in multiple data structures to enhance functionality related to world and contract management.manifest_dev.jsonto include new entrypoints and updated contract definitions.Bug Fixes
Documentation