bugfix: use the '<namespace>__<name>' format for model contract names #2406
Conversation
WalkthroughOhayo, sensei! The changes in this pull request involve renaming various models and adjusting their namespaces throughout the Dojo framework. This restructuring aims to enhance clarity and consistency in model references, ensuring that class hashes and identifiers align with the new naming conventions. Modifications include updates to import statements, model registrations, and JSON configuration files, while maintaining the core functionality of the code. Changes
Possibly related issues
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
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, codebase verification and nitpick comments (3)
examples/spawn-and-move/manifests/dev/base/abis/models/dojo_examples-Position-1e145e26.json (1)
417-417: Ohayo sensei! Update references to the old event name.The event name
dojo_examples::models::position::Eventstill appears in the following files, which need to be updated to the new format to ensure consistency and avoid breaking dependencies:
crates/dojo-lang/src/manifest_test_data/manifestbin/sozo/tests/test_data/manifest.jsonPlease update these references to align with the new event name format.
Analysis chain
Ohayo sensei! The event name change looks good and aligns with the PR objective.
The event name has been updated to follow the
<namespace>__<name>format, which enhances clarity and consistency in model referencing. This change is consistent with the overall goal of the pull request.To ensure the event name change does not break any dependencies, please run the following verification script:
Ohayo sensei! Let's re-run the verification with a corrected approach. I'll search for the old event name across all files to ensure no dependencies are broken. Please hold on.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the event name change does not break any dependencies. # Test: Search for references to the old event name. Expect: No results. rg --type cairo --type rust $'dojo_examples::models::position::Event'Length of output: 98
Script:
#!/bin/bash # Description: Verify the event name change does not break any dependencies. # Test: Search for references to the old event name across all files. rg 'dojo_examples::models::position::Event'Length of output: 251
crates/dojo-lang/src/model.rs (2)
Line range hint
99-141: Ohayo sensei! The code for parsing model parameters looks mostly good to me, but I have a couple of nitpicks.
The
processed_argshashmap can be replaced with a simpler boolean flag for each argument to track duplicates. This will make the code more readable and efficient.The
matchonarg_namecan be replaced withifconditions for better readability. Something like:if arg_name == MODEL_VERSION_NAME { parameters.version = get_model_version(db, arg_value, diagnostics); } else if arg_name == MODEL_NAMESPACE { parameters.namespace = get_model_namespace(db, arg_value, diagnostics); } else if arg_name == MODEL_NOMAPPING { parameters.nomapping = true; } else { diagnostics.push(PluginDiagnostic { ... }); }
Line range hint
154-704: Ohayo sensei! Thehandle_model_structfunction works but it's quite complex and can be improved.Here are some suggestions to refactor it:
Break it down into smaller functions for better readability and maintainability. For example, separate functions can be created for:
- Validating names
- Generating hashes and selectors
- Processing struct members to generate keys, values, and accessors
- Interpolating the final code template
Add more comments explaining what each section of the code is doing. The function is quite long, so comments will help future readers understand the code better.
Use more descriptive variable names, especially for the interpolation variables in the large code template at the end. Names like
type_name,model_version,serialized_keysetc. are a bit generic and not very descriptive in the context of this function.Consider moving the large code template string to a separate file or constant. It will declutter this function and the template can be reused elsewhere if needed.
Let me know if you would like me to submit a PR with these refactorings!
remybar
force-pushed
the
bugfix-avoid_same_model_contract_names
branch
from
f2a90cf to
ac2d311
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
examples/spawn-and-move/manifests/dev/base/models/dojo_examples-Position-1e145e26.toml (1)
2-3: Ohayo sensei! It looks like some references to the old qualified path still linger in the codebase.To keep everything consistent and avoid any potential hiccups, please update all occurrences of
dojo_examples::models::positiontodojo_examples::models::dojo_examples__position. Here are the files that need attention:
crates/dojo-lang/src/manifest_test_data/manifestbin/sozo/tests/test_data/manifest.jsonMake sure these changes are reflected throughout the codebase to ensure everything runs smoothly. Keep up the great work, sensei!
Analysis chain
Ohayo sensei! The changes to the
class_hash,original_class_hash, andqualified_pathare consistent with the PR objectives.The modifications indicate a change in the underlying class representation and a restructuring or renaming of the model's namespace. Please ensure that these changes are propagated throughout the codebase and that all references to this model are updated accordingly.
To verify the impact of these changes on other parts of the codebase, you can run the following script:
Also applies to: 6-6
Ohayo sensei! It seems like the initial verification didn't give us the clarity we need. Let's dig a bit deeper to ensure everything's in order. I'll run some additional checks to verify the propagation of the changes. Hang tight!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to the `Position` model have been updated. # Test 1: Search for the old qualified path. Expect: No occurrences. rg --type rust $'dojo_examples::models::position' # Test 2: Search for the new qualified path. Expect: Only occurrences of the new path. rg --type rust $'dojo_examples::models::dojo_examples__position'Length of output: 112
Script:
#!/bin/bash # Re-run the search with a broader scope and check for potential errors. # Test 1: Search for the old qualified path in all files. Expect: No occurrences. rg 'dojo_examples::models::position' --glob '*' # Test 2: Search for the new qualified path in all files. Expect: Only occurrences of the new path. rg 'dojo_examples::models::dojo_examples__position' --glob '*'Length of output: 1711
examples/spawn-and-move/manifests/dev/deployment/abis/models/dojo_examples-Moves-2e2accba.json (1)
433-433: Ohayo sensei! There's a lingering reference to the old event name.The event name
dojo_examples::models::moves::Eventis still present inbin/sozo/tests/test_data/manifest.json. Please update this reference to the new formatdojo_examples::models::dojo_examples__moves::Eventto ensure consistency across the codebase.
- File to update:
bin/sozo/tests/test_data/manifest.jsonAnalysis chain
Ohayo sensei! The event name change looks good, but please verify the usage.
The event name change is consistent with the PR objective of using the
<namespace>__<name>format for model contract names.However, this change may impact how the event is referenced in other parts of the codebase. Please ensure that all references to this event have been updated to use the new name.
Run the following script to verify the event usage:
Ohayo sensei! Let's make sure everything's in order. I'll run a couple of checks to verify the event name change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the `dojo_examples::models::moves::Event` event have been updated to use the new name. # Test: Search for the old event name. Expect: No occurrences of the old name. rg --type cairo --type rust $'dojo_examples::models::moves::Event'Length of output: 95
Script:
#!/bin/bash # Search for the old event name across all file types to ensure it's not being used anymore. rg 'dojo_examples::models::moves::Event' --glob '*.*' -A 2Length of output: 283
Script:
#!/bin/bash # Search for the new event name to confirm its usage in the codebase. rg 'dojo_examples::models::dojo_examples__moves::Event' --glob '*.*' -A 2Length of output: 1540
remybar
force-pushed
the
bugfix-avoid_same_model_contract_names
branch
2 times, most recently
from
924a9e1 to
d9cae6e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2406 +/- ##
==========================================
- Coverage 68.13% 67.97% -0.17%
==========================================
Files 364 364
Lines 47712 47818 +106
==========================================
- Hits 32509 32503 -6
- Misses 15203 15315 +112 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you @remybar for the work on that.
I'm trying to see any other option than having an arbitrary separator that may be messed up depending on how the user names the contract.
But it seems we don't have much alternative for now.
crates/dojo-lang/src/compiler.rs
Outdated
| let (source_path, source_name) = | ||
| contract_path.rsplit_once(CAIRO_PATH_SEPARATOR).unwrap_or(("", &contract_path)); | ||
| let (_, source_name) = | ||
| source_name.rsplit_once(naming::DOJO_CONTRACT_SEPARATOR).unwrap_or(("", source_name)); | ||
|
|
||
| let (path, name) = self.0.rsplit_once(CAIRO_PATH_SEPARATOR).unwrap_or(("", &self.0)); | ||
| let name = name.to_case(Case::Snake); | ||
|
|
||
| source_path == path && source_name == name | ||
| } |
There was a problem hiding this comment.
We're not including the namespace in the comparison here, may we have conflicts at some point?
There was a problem hiding this comment.
In fact, this match_contract_path function is used for external contracts only (the ones specified in Scarb.toml). I don't know if these external contracts can have a namespace in their names 🤔
But, for the spawn-and-move example, we have 2 external contracts without namespace (for example, the contract selector bestiary::RiverSkale must be matched with the contract path bestiary::dojo_examples_foes__river_skale).
So, I propose to compare namespaces if and only if the contract selector contains a namespace. But, in case of namespace remapping, is it possible to have an external contract with a namespace in its name remapped to another namespace ?
remybar
force-pushed
the
bugfix-avoid_same_model_contract_names
branch
from
35157ed to
bcc6f9b
Compare
Yes, I agree but I think it's not a really big constraint to avoid using |
| "name": "dojo_examples::models::dojo_examples__moves::Event", | ||
| "kind": "enum", | ||
| "variants": [] | ||
| } | ||
| ], | ||
| "tag": "dojo_examples-Moves", | ||
| "qualified_path": "dojo_examples::models::moves", | ||
| "qualified_path": "dojo_examples::models::dojo_examples__moves", |
There was a problem hiding this comment.
Tip
Codebase Verification
Ohayo sensei! It looks like we've got a bit of a mix-up with the moves::Event references. Some files are using the new path dojo_examples::models::dojo_examples__moves::Event, while others are still rocking the old path dojo_examples::models::moves::Event.
Here's what needs attention:
- Update all references to
dojo_examples::models::dojo_examples__moves::Eventto ensure consistency across the codebase.
Let's get those paths aligned to avoid any hiccups!
Analysis chain
Ohayo sensei! The renaming of moves event looks good to me.
The new name and qualified_path values enhance clarity by distinctly categorizing the event under the dojo_examples module.
However, please ensure that this change is thoroughly tested and validated to confirm that it behaves as expected and that all references to the moves event have been updated to match the new paths.
Run the following script to verify the event usage:
Ohayo sensei! Let's try a different approach to verify the usage of moves::Event in the codebase. I'll adjust the search command to look through all files and ensure everything aligns with the new paths.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all references to `moves` event match the new paths.
# Test: Search for the event usage. Expect: Only occurrences of the new paths.
rg --type cairo -A 5 $'moves::Event'
Length of output: 65
Script:
#!/bin/bash
# Description: Verify all references to `moves` event match the new paths.
# Test: Search for the event usage across all files.
rg -A 5 $'moves::Event'
Length of output: 3118
| "name": "dojo_examples::models::dojo_examples__mock_token::Event", | ||
| "kind": "enum", | ||
| "variants": [] | ||
| } | ||
| ], | ||
| "tag": "dojo_examples-MockToken", | ||
| "qualified_path": "dojo_examples::models::mock_token", | ||
| "qualified_path": "dojo_examples::models::dojo_examples__mock_token", |
There was a problem hiding this comment.
Tip
Codebase Verification
Ohayo sensei! There's an inconsistency in the mock_token::Event references.
The codebase contains both the new path dojo_examples::models::dojo_examples__mock_token::Event and an incorrect path dojo_examples::mock_token::mock_token::Event. Please review and update all references to ensure consistency with the new naming convention.
- Inconsistent References: Ensure all references to
mock_token::Eventare updated todojo_examples::models::dojo_examples__mock_token::Event.
This will help maintain clarity and prevent potential issues.
Analysis chain
Ohayo sensei! The renaming of mock_token event looks good to me.
The new name and qualified_path values enhance clarity by distinctly categorizing the event under the dojo_examples module.
However, please ensure that this change is thoroughly tested and validated to confirm that it behaves as expected and that all references to the mock_token event have been updated to match the new paths.
Run the following script to verify the event usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all references to `mock_token` event match the new paths.
# Test: Search for the event usage. Expect: Only occurrences of the new paths.
rg --type cairo -A 5 $'mock_token::Event'
Length of output: 70
Script:
#!/bin/bash
# Description: Verify all references to `mock_token` event match the new paths.
# Test: Search for the event usage without specifying a file type.
rg -A 5 'mock_token::Event'
Length of output: 5462
remybar
force-pushed
the
bugfix-avoid_same_model_contract_names
branch
from
bcc6f9b to
0aa518c
Compare
|
Replaced by #2457. |
Description
To avoid having conflicts between several models with a same name but in different namespaces, the model contract name now follow the following format:
<namespace>__<name>.Note that the double underscore
__is used as the separator betweennamespaceandname. That means it should not be used innamespaceandname. So, we probably should raise a warning/error if it's the case.Related issue
#2349
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bug Fixes
Chores