feat: separate the resource registration and permission phase for tes… #2627
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2627 +/- ##
==========================================
- Coverage 56.91% 56.90% -0.02%
==========================================
Files 397 397
Lines 49466 49467 +1
==========================================
- Hits 28154 28147 -7
- Misses 21312 21320 +8 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! This pull request introduces several updates across multiple files in the Dojo project. Key changes include the addition of a new public trait Changes
Possibly related PRs
Suggested reviewers
🪧 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: 1
🧹 Outside diff range and nitpick comments (3)
examples/simple/src/lib.cairo (2)
127-128: Clean resource registration implementation, sensei!The use of
TestResourcefor both model and contract registration provides a clear separation of concerns, aligning perfectly with the PR's goal of separating resource registration from permission handling.This approach makes testing more maintainable as resources can be registered independently of their permissions.
134-138: Excellent separation of contract setup and permission sync!The new approach using
ContractDefTraitfollowed bysync_perms_and_initsclearly separates the resource registration from permission handling, matching the PR objectives and Sozo framework patterns.Consider replacing the magic number
0xffin the init calldata with a named constant to improve code readability:- .with_init_calldata([0xff].span()); + .with_init_calldata([INITIAL_VALUE].span());examples/spawn-and-move/src/actions.cairo (1)
234-239: Consider adding documentation for writer permissions.The new
contract_defs()function nicely implements the separation of contract definitions. However, it would be helpful to add a comment explaining the purpose of the writer permissions being set on line 237.fn contract_defs() -> Span<ContractDef> { [ ContractDefTrait::new(@"ns", @"actions") + // Set writer permissions for the namespace to allow the contract to modify state .with_writer_of([dojo::utils::bytearray_hash(@"ns")].span()) ].span() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers.cairo(2 hunks)crates/dojo/core-cairo-test/src/world.cairo(3 hunks)examples/simple/src/lib.cairo(1 hunks)examples/spawn-and-move/src/actions.cairo(2 hunks)
🔇 Additional comments (15)
crates/dojo/core-cairo-test/src/lib.cairo (2)
13-13: Ohayo! The trait export addition looks good, sensei!
The addition of WorldStorageTestTrait to the exports aligns well with the PR's objective of separating resource registration and permission phases for testing purposes.
Line range hint 25-33: Verify the status of commented test modules.
There are several commented-out test modules. Let's verify if they should remain commented out or if they need to be re-enabled:
// mod model {
// mod model;
// }
...
//mod entities;
//mod resources;
//mod world;
examples/simple/src/lib.cairo (1)
118-120: Ohayo! Import changes align well with the new architecture.
The addition of WorldStorageTestTrait supports the separation of resource registration and permission phases, as outlined in the PR objectives.
crates/dojo/core-cairo-test/src/tests/helpers.cairo (2)
8-8: Ohayo! Import changes align with the new architecture.
The updated imports reflect the shift towards using NamespaceDef for resource registration, which aligns well with the PR's objective of separating resource registration from permissions.
241-241: Ohayo sensei! Resource registration change looks good.
The switch to TestResource::Model aligns perfectly with the PR's goal of separating resource registration from permissions. This change simplifies the registration phase while maintaining the same functionality.
Let's verify that this change is consistent across other test files:
examples/spawn-and-move/src/actions.cairo (3)
213-216: Ohayo sensei! Clean import organization.
The new imports from dojo_cairo_test align perfectly with the PR's objective to separate resource registration and permission phases.
224-227: Clean resource registration, sensei!
The explicit registration of models, events, and contracts in namespace_def() follows the new pattern of separating resource registration from permissions.
248-249: Verify consistent usage of sync_perms_and_inits across tests.
The sync_perms_and_inits() calls are correctly placed in the test functions. Let's ensure this pattern is consistently followed across all test functions in the codebase.
Also applies to: 284-284
✅ Verification successful
Let me gather more specific information about the test functions to ensure we have a complete picture.
This will help us see the complete test function implementations and verify if the sync calls are properly placed after world spawning in each test.
Ohayo sensei! Based on the comprehensive test results, I can now provide a definitive response about the sync_perms_and_inits usage.
The sync_perms_and_inits call is correctly used in test functions that require it
The sync_perms_and_inits is used in the spawn-and-move example's test functions where permission initialization is needed:
test_world_test_set()- correctly calls sync_perms_and_inits after world spawntest_move()- correctly calls sync_perms_and_inits after world spawn
Other test functions in the codebase use different initialization patterns based on their specific needs:
- Core test functions use
deploy_world()which handles initialization internally - Resource/ACL tests use explicit permission grants via
grant_owner/grant_writer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all test functions properly initialize permissions
# Look for test functions that spawn a test world but might miss the sync call
# Search for test function definitions
echo "Test functions that spawn a world:"
rg "fn test_.*\{" -A 5 | rg "spawn_test_world"
# Search for sync_perms_and_inits usage
echo -e "\nSync calls in test functions:"
rg "sync_perms_and_inits"
Length of output: 1391
Script:
#!/bin/bash
# Get full context of test functions in spawn-and-move/actions.cairo
echo "=== Test functions in spawn-and-move/actions.cairo ==="
rg "fn test_" examples/spawn-and-move/src/actions.cairo -B 1 -A 10
# Get full context of test functions in core-cairo-test
echo -e "\n=== Test functions in core-cairo-test ==="
rg "fn test_" crates/dojo/core-cairo-test/src/tests/world/ -B 1 -A 10
Length of output: 99284
crates/dojo/core-cairo-test/src/world.cairo (7)
9-9: Definition of TestClassHash Approved
Ohayo sensei! The introduction of TestClassHash as an alias for felt252 enhances code clarity and consistency.
20-22: Update to TestResource Enum Variants Looks Good
Ohayo sensei! Changing the TestResource enum variants to use TestClassHash aligns with the new type alias and maintains consistency across the codebase.
25-30: Introduction of ContractDescriptor Enum
Ohayo sensei! The new ContractDescriptor enum effectively represents contracts either by address or by name and namespace, providing flexibility in contract handling.
58-63: Updated ContractDefImpl::new Function
Ohayo sensei! The updated new function in ContractDefImpl now constructs a ContractDef with a ContractDescriptor::Named, which improves the representation of contracts by name and namespace.
171-209: Validation of sync_perms_and_inits Implementation
Ohayo sensei! The implementation of sync_perms_and_inits in WorldStorageInternalTestImpl correctly handles both Address and Named variants of ContractDescriptor. The method properly retrieves the contract address and synchronizes permissions and initializations.
175-185: Handling of ContractDescriptor::Named Variant
Ohayo sensei! When handling the ContractDescriptor::Named variant, the code retrieves the contract address by querying the dispatcher. Ensure that the resource exists and is of type Resource::Contract to avoid runtime panics.
If you would like, I can help implement additional error handling to gracefully handle cases where the contract is not found. Do you want me to provide a code suggestion?
187-194: Permission Granting Logic Approved
Ohayo sensei! The loops granting writer and owner permissions to contracts are correctly implemented. The code iterates over writer_of and owner_of, ensuring that the appropriate permissions are assigned.
| TestResource::Contract(ch) => { | ||
| world.register_contract(*ch, namespace.clone(), (*ch).try_into().unwrap()); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Parameter Mismatch in register_contract Call
Ohayo sensei! After reviewing the IWorld trait definition, I can confirm there is indeed an issue. The register_contract function expects three parameters:
salt: felt252namespace: ByteArrayclass_hash: ClassHash
However, in the code:
world.register_contract(*ch, namespace.clone(), (*ch).try_into().unwrap());The same class hash value *ch is being used both as the salt and the class hash, which is incorrect. The salt should be a unique value to determine the contract's address.
Looking at other usages in the codebase, they typically use a literal salt value like 'salt1':
world.register_contract('salt1', namespace, class_hash);🔗 Analysis chain
Potential Issue with world.register_contract Parameters
Ohayo sensei! There might be a concern with the parameters passed to world.register_contract in the spawn_test_world function. Specifically, the class hash *ch is being used both as the contract address and the class hash:
world.register_contract(*ch, namespace.clone(), (*ch).try_into().unwrap());Since *ch is a TestClassHash (a felt252 representing the class hash), using it as the contract address may not be correct. Please verify that the correct contract address is being provided.
To ensure that register_contract receives the correct parameters, you can run the following script to check where register_contract is defined and how it's used elsewhere:
Let me check the interface definition to understand the expected parameter types.
Let me try a different approach to find the interface definition.
Let me search specifically for the register_contract function in the IWorld trait.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `world.register_contract` to verify correct parameter usage.
# Search for all occurrences of `register_contract` in the codebase
rg 'register_contract\(' -A 2
Length of output: 7841
Script:
#!/bin/bash
# Find the interface definition for register_contract
ast-grep --pattern 'fn register_contract($$$)'
Length of output: 48
Script:
#!/bin/bash
# Search for the interface definition in the core module
rg -A 5 'trait IWorld' crates/dojo/core/src/
Length of output: 1428
Script:
#!/bin/bash
# Search for register_contract function in IWorld trait
rg -B 2 -A 4 'fn register_contract' crates/dojo/core/src/world/iworld.cairo
Length of output: 406
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
scripts/cairo_fmt.sh (1)
3-12: Consider enhancing error handling and user experience, sensei!The script could benefit from some defensive programming improvements:
Here's a suggested enhancement:
#!/bin/bash +# Exit on error +set -e + +# Check if scarb is installed +if ! command -v scarb &> /dev/null; then + echo "Error: scarb is not installed" + exit 1 +fi + +# Usage message +usage() { + echo "Usage: $0 [--fix]" + echo " --fix Apply formatting fixes instead of checking" + exit 1 +} + +# Parse arguments option="--check" - -if [ "$1" == "--fix" ]; then +if [ "$#" -gt 1 ]; then + usage +elif [ "$1" = "--fix" ]; then option="" +elif [ "$#" -eq 1 ]; then + usage fi -scarb --manifest-path examples/spawn-and-move/Scarb.toml fmt $option -scarb --manifest-path examples/simple/Scarb.toml fmt $option -scarb --manifest-path crates/dojo/core/Scarb.toml fmt $option -scarb --manifest-path crates/dojo/core-cairo-test/Scarb.toml fmt $option +# Function to run scarb fmt with error handling +run_fmt() { + local manifest_path=$1 + if [ ! -f "$manifest_path" ]; then + echo "Error: Manifest not found at $manifest_path" + exit 1 + fi + scarb --manifest-path "$manifest_path" fmt $option +} + +# Run formatting for all manifests +run_fmt "examples/spawn-and-move/Scarb.toml" +run_fmt "examples/simple/Scarb.toml" +run_fmt "crates/dojo/core/Scarb.toml" +run_fmt "crates/dojo/core-cairo-test/Scarb.toml"These improvements add:
- Early exit on errors
- Scarb installation check
- Usage message
- Better argument validation
- Manifest existence validation
- Function to handle repeated logic
crates/dojo/core-cairo-test/src/tests/contract.cairo (1)
103-103: Clean change, sensei! Consider documenting the security aspect.The dispatcher assignment is consistent. Consider adding a comment explaining why direct upgrades must be restricted to the world contract for better documentation of security constraints.
fn test_upgrade_direct() { let world = deploy_world(); + // Direct upgrades must be restricted to the world contract to maintain + // centralized control over contract upgrades let world = world.dispatcher;crates/dojo/core-cairo-test/src/tests/helpers.cairo (2)
191-195: Consider extracting the model ID calculation into a helper function.The model ID calculation using
poseidon_hash_spancould be reused across different operations. Consider extracting it into a helper function for better maintainability and reusability.+ fn get_foo_model_id(caller: ContractAddress) -> felt252 { + core::poseidon::poseidon_hash_span([caller.into()].span()) + } fn delete_foo(self: @ContractState) { let mut world = self.world(@"dojo"); - let ptr = ModelPtr::<Foo>::Id( - core::poseidon::poseidon_hash_span([get_caller_address().into()].span()) - ); + let ptr = ModelPtr::<Foo>::Id(get_foo_model_id(get_caller_address())); world.erase_model_ptr(ptr); }
201-204: Consider documenting the empty resources span.While the empty resources span is intentional for deploying a bare world, it would be helpful to document why no resources are registered by default.
/// Deploys an empty world with the `dojo` namespace. + /// No resources are registered by default to allow for minimal test environments + /// where specific resources can be registered later as needed. pub fn deploy_world() -> WorldStorage { let namespace_def = NamespaceDef { namespace: "dojo", resources: [].span(), }; spawn_test_world([namespace_def].span()) }crates/dojo/core-cairo-test/src/world.cairo (2)
58-83: Consider enhancing error handling in with_init_calldata.Ohayo sensei! While the implementation looks good overall, the panic message in
with_init_calldatacould be more informative by including the actual address that caused the error.- panic!("Cannot set init_calldata for address descriptor") + panic!("Cannot set init_calldata for contract at address {}", address)
184-223: Consider enhancing error handling in sync_perms_and_inits.Ohayo sensei! While the implementation follows Sozo's approach nicely, there are a few areas where error handling could be improved:
- The "Contract not found" panic could include the namespace and name for better debugging
- Consider adding validation for empty writer_of and owner_of spans
- panic!("Contract not found"), + panic!("Contract not found for namespace: {} and name: {}", *namespace, *name),crates/dojo/core-cairo-test/src/tests/world/world.cairo (3)
74-107: Event handling looks solid, but could use additional test cases.The new approach using
NamespaceDeffor event registration andsync_perms_and_initsfor permissions is well structured. Consider adding these test cases:
- Attempt to emit event without proper permissions
- Verify event emission with multiple namespaces
Would you like me to help implement these additional test cases, sensei?
Line range hint
262-314: Consider using constants for error messages, sensei!The contract initialization tests are thorough, but the error messages are hardcoded strings. Consider extracting them to constants:
+ const ERROR_ONLY_WORLD_INIT: felt252 = 'Only the world can init contract'; + const ERROR_CONTRACT_NOT_DEPLOYED: felt252 = 'CONTRACT_NOT_DEPLOYED'; #[test] fn test_can_call_init_only_world_args() { // ... - let expected_panic: ByteArray = "Only the world can init contract `test_contract_with_dojo_init_args`, but caller is `0`"; + let expected_panic: ByteArray = format!("{} `test_contract_with_dojo_init_args`, but caller is `0`", ERROR_ONLY_WORLD_INIT); // ... }This would:
- Make error messages consistent across tests
- Easier to update messages in the future
- Prevent typos in error strings
Line range hint
1-314: Ohayo! Great architectural improvements overall!The separation of resource registration and permissions phases through
NamespaceDefandsync_perms_and_initsprovides better testing capabilities and aligns well with the Sozo framework. The consistent use of namespaces and proper permission handling sets a good foundation for future extensions.Consider these future improvements:
- Add integration tests that verify the complete lifecycle of resource registration → permission setup → initialization
- Consider implementing a test helper trait for common namespace and permission setup patterns
- Document the expected order of operations (registration → permissions → initialization) in the test file header
crates/dojo/core-cairo-test/src/tests/world/acl.cairo (1)
Line range hint
1-322: Consider some test refactoring opportunities, sensei!To improve the test maintainability, consider:
- Extracting common test setup patterns into helper functions
- Defining constants for frequently used test addresses (e.g.,
0xa11ce,0xb0b,0xdead)Example helper function:
fn setup_test_with_owner(owner: ContractAddress) -> (IWorldDispatcher, felt252) { let (world, foo_selector) = deploy_world_and_foo(); let world = world.dispatcher; starknet::testing::set_account_contract_address(owner); starknet::testing::set_contract_address(owner); (world, foo_selector) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/contract.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/helpers.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/acl.cairo(15 hunks)crates/dojo/core-cairo-test/src/tests/world/world.cairo(14 hunks)crates/dojo/core-cairo-test/src/world.cairo(3 hunks)crates/dojo/core/src/world/storage.cairo(1 hunks)scripts/cairo_fmt.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core-cairo-test/src/lib.cairo
🔇 Additional comments (16)
scripts/cairo_fmt.sh (1)
3-7: Ohayo! Clean implementation of format option handling!
The conditional logic for toggling between format checking and fixing is well-structured and follows shell scripting best practices.
crates/dojo/core-cairo-test/src/tests/contract.cairo (3)
72-72: Ohayo! Clean dispatcher usage, sensei!
The explicit assignment of world.dispatcher improves clarity and maintains consistency with the new resource management approach.
180-180: Excellent validation test, sensei!
The dispatcher assignment is consistent. The test properly validates namespace naming rules.
#!/bin/bash
# Search for the namespace validation implementation to verify the regex pattern
rg -A 5 "register_namespace" --type cairo89-89: Looking good, sensei! Let's verify the panic message.
The dispatcher assignment is consistent. Since this is a negative test case, we should verify that the panic message accurately reflects the current error handling.
✅ Verification successful
The previous command failed because the file type wasn't recognized. Let's try searching directly in the test file to understand the error handling and validation being performed.
Ohayo! The panic message is perfectly aligned, sensei!
The test case correctly expects the panic message ('ENTRYPOINT_NOT_FOUND', 'ENTRYPOINT_FAILED') which matches the test's purpose of validating failure when attempting to upgrade from a world that's not a world provider. The error handling is consistent with other test cases in the file that validate similar failure scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related panic messages in the codebase
rg -A 2 "ENTRYPOINT_NOT_FOUND" --type cairo
Length of output: 71
Script:
#!/bin/bash
# Search for panic messages in the test file and surrounding context
rg -A 5 "panic" crates/dojo/core-cairo-test/src/tests/contract.cairo
# Search for any error-related strings in the test file
rg -A 5 "error|fail|invalid" crates/dojo/core-cairo-test/src/tests/contract.cairo
Length of output: 2311
crates/dojo/core-cairo-test/src/tests/helpers.cairo (2)
3-3: Ohayo sensei! Import changes look good!
The updated imports align well with the PR's objective of separating resource registration and permissions. The addition of WorldStorage and related traits provides better structure for managing world state in tests.
Also applies to: 6-8
227-231: Excellent separation of resource registration and permissions!
The implementation perfectly aligns with the PR objectives:
- Resources are registered via NamespaceDef
- Permissions are defined via ContractDef
- Phases are properly separated using sync_perms_and_inits
crates/dojo/core-cairo-test/src/world.cairo (5)
9-17: LGTM! Well-documented type alias.
Ohayo sensei! The introduction of TestClassHash type alias with comprehensive documentation clearly explains the rationale for using felt252 instead of ClassHash. This improves code ergonomics by eliminating the need for repeated .try_into().unwrap() calls in test files.
19-22: LGTM! Consistent use of TestClassHash.
The enum variants now use TestClassHash consistently, aligning with the new type alias and making the code more ergonomic for testing purposes.
25-30: LGTM! Clear contract descriptor abstraction.
The ContractDescriptor enum provides a clean abstraction for handling both address-based and named contracts, with clear documentation for each variant.
33-48: LGTM! Enhanced contract definition structure.
The updated ContractDef struct with the new ContractDescriptor field provides better flexibility for handling both Dojo and external contracts. The documentation clearly explains the distinction and usage.
174-176: **** The parameter mismatch issue in register_contract identified in the past review is still present.
crates/dojo/core-cairo-test/src/tests/world/world.cairo (2)
1-15: Ohayo sensei! Clean import organization aligns with new architecture.
The updated imports properly support the separation of resource registration and permission phases, particularly with the addition of NamespaceDef and WorldStorageTestTrait.
Line range hint 18-51: Consistent use of namespaced model registration, sensei!
The transition to namespace-based model registration with "dojo" namespace is well implemented. The pattern of obtaining the dispatcher via world.dispatcher is consistently applied.
crates/dojo/core-cairo-test/src/tests/world/acl.cairo (2)
13-14: Ohayo! LGTM on the consistent world dispatcher usage!
The consistent pattern of accessing the world dispatcher directly aligns well with the PR's objective of separating resource registration and permission phases. This change makes the dispatcher usage more explicit throughout the test suite.
Also applies to: 39-40, 49-50, 72-73, 87-88, 111-112, 128-129, 142-143, 152-153, 175-176, 190-191, 214-215, 237-238, 276-277, 308-309
Line range hint 1-322: Test coverage looks solid, sensei!
The modifications maintain comprehensive test coverage across all ACL scenarios:
- Owner permission management
- Writer permission controls
- Namespace access restrictions
- Security checks against malicious contracts
- Unauthorized access attempts
All test assertions and panic messages remain properly aligned with the expected behavior.
crates/dojo/core/src/world/storage.cairo (1)
43-44: Ohayo sensei! The renaming from contract_selector to resource_selector aligns well with the PR objectives.
The change reflects a more generic and flexible approach to resource handling, which is consistent with the separation of resource registration and permission phases. This modification enhances the API's clarity by accurately representing its broader scope beyond just contracts.
Let's verify the impact of this renaming:
✅ Verification successful
Ohayo sensei! The renaming from contract_selector to resource_selector has been properly propagated across the codebase.
The search results show that:
- All old usages of
contract_selectorare in different contexts (like function parameters) and not related to the renamed method - The new
resource_selectormethod is being used consistently in the codebase - The change aligns with the broader resource management approach, as evidenced by its usage in auth operations and world contracts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old method name
rg "contract_selector" --type cairo
# Search for usages of the new method name to ensure consistent adoption
rg "resource_selector" --type cairo
Length of output: 131
Script:
#!/bin/bash
# Search for any remaining references to the old method name without file type filter
rg "contract_selector"
# Search for usages of the new method name
rg "resource_selector"
# Also search in git history to see the change
git log -p | rg "contract_selector|resource_selector"
Length of output: 20027
Before, the
ContractDefwas containing the class for registration and the contract definition for permissions and initialization.However, to test more efficiently, we prefer first registering the resources, and then apply permissions and init exactly like sozo is doing.
This PR aims at providing this behavior, where:
NamespaceDefto register all resources.sync_perms_and_initswhere permissions are first applied, and then the contracts are initialized.Summary by CodeRabbit
Release Notes
New Features
WorldStorageTestTraitfor enhanced testing capabilities.ContractDescriptorfor simplified resource handling.MyInterfaceImplandActionsImplimplementations.sozobinary in CI workflow, expanding testing coverage.Bug Fixes
reset_player_configmethod.Documentation
Refactor
contract_selectortoresource_selectorfor broader functionality inWorldStorageInternalImpl.