fix: ensure the manifest existence is correctly tested #2713
Conversation
WalkthroughOhayo, sensei! This pull request introduces several changes across different files. Key modifications include a minor visual adjustment to the spinner in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (4)bin/sozo/tests/test_data/policies.json (4)
The consolidation of all core world management methods (uuid, set_metadata, register_*, etc.) under a single target address (0x70058e...056e) appears intentional and well-organized. This grouping suggests proper separation of concerns.
The game-related methods (spawn, move, player configurations, etc.) are consistently mapped to a single target address (0x1cc58...dce3), which maintains good separation from core world operations.
Let's ensure these new target addresses are consistently used across related configuration files and contracts. ✅ Verification successfulTarget addresses are consistently used across configuration files The verification shows that both target addresses are properly referenced:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for the new target addresses in other configuration files and contracts
echo "Checking core world target usage..."
rg "0x70058e3886cb7411e8a77db90ee3dd453ac16b763b30bd99b3c8440fe42056e"
echo "Checking game operations target usage..."
rg "0x1cc5853cbf65c96d7c3c8f951e6315f3c1f19a34ace7b579da9ba87b974dce3"
# Check for any remaining references to old addresses that might need updating
echo "Checking for potential missed updates in manifest files..."
fd -e json -x grep -l "manifest" {} \; | xargs rg "0x54d0f13bf3fb5f15a8674c5204aad35e3022af96bcc23bdbd16b7e297ffd399"
Length of output: 4473
I notice there are four different target addresses for the
This will help ensure the upgrade process is well-documented and intentional. ✅ Verification successfulOhayo sensei! The multiple upgrade targets are intentionally mapped to different contracts Based on the manifest_dev.json findings, each upgrade target corresponds to a specific contract implementation:
This is a valid pattern where each contract type has its own dedicated upgrade policy entry. The multiple entries allow for granular control over upgrading different parts of the system independently. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any documentation or related code that explains these upgrade targets
rg -l "0x7e5b903baa82407f74bd0a573352e0ca33f2b9fac3bcc2e35230119841cd08|0x32bc6ef441c52acfc62ee1186885760803d88e165119fdad6e16f9424547981|0x72b95d835f6158dd1454c1b4463a91222c6088f1bf5dd2cdd09268f92dad16f" --type md
Length of output: 216 Script: #!/bin/bash
# Let's search for these target addresses in all files to understand their context
rg -A 5 "0x7e5b903baa82407f74bd0a573352e0ca33f2b9fac3bcc2e35230119841cd08|0x32bc6ef441c52acfc62ee1186885760803d88e165119fdad6e16f9424547981|0x72b95d835f6158dd1454c1b4463a91222c6088f1bf5dd2cdd09268f92dad16f"
# Also search for any test files or configuration that might explain the upgrade process
fd -e rs -e toml -e json "upgrade|test" --exec grep -l "upgrade"
Length of output: 2947 🪧 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 (2)
crates/sozo/scarbext/src/workspace.rs (1)
149-152: Ohayo sensei! The manifest existence check looks good!The change correctly fixes the issue by checking for the specific manifest file's existence instead of just the directory. This aligns perfectly with the PR objectives and will prevent unnecessary re-syncing of the world from the chain.
However, we could make the error handling even more robust.
Consider adding explicit error context when the file exists but can't be opened:
if !manifest_dir.child(manifest_name.clone()).exists() { return Ok(None); } - let mut file = manifest_dir.open_ro(manifest_name, "Dojo manifest file", self.config())?; + let mut file = manifest_dir + .open_ro(manifest_name.clone(), "Dojo manifest file", self.config()) + .map_err(|e| anyhow::anyhow!("Failed to open manifest file {}: {}", manifest_name, e))?;examples/spawn-and-move/manifest_dev.json (1)
Line range hint
1-2189: Consider adding schema validation for the manifest file.To prevent configuration errors and ensure the manifest file's integrity, consider implementing JSON schema validation.
This would help:
- Validate the structure of the manifest file
- Ensure all required fields are present
- Verify the format of addresses and class hashes
- Maintain consistency across different environments
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
crates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/sozo/ops/src/migration_ui.rs(1 hunks)crates/sozo/scarbext/src/workspace.rs(1 hunks)examples/simple/Scarb.toml(1 hunks)examples/spawn-and-move/manifest_dev.json(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/sozo/ops/src/migration_ui.rs
🔇 Additional comments (6)
examples/simple/Scarb.toml (2)
Line range hint 1-24: Ensure version consistency across dependencies
Ohayo! The cairo-version and starknet dependency are both pinned to 2.8.4, which is good for reproducibility. However, let's verify this version is consistent across other configuration files in the project.
✅ Verification successful
Version consistency verified across most Scarb.toml files
Ohayo! I've found that cairo-version and starknet dependency are consistently set to 2.8.4 across most Scarb.toml files in the project, with one intentional exception in a test data directory (bin/sozo/tests/test_data/invalid_cairo_version/Scarb.toml) which is specifically meant to test invalid version handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version consistency across config files
# Look for other Scarb.toml files and their versions
fd Scarb.toml --exec grep -H "cairo-version\|starknet.*2\.8\.4"
Length of output: 784
9-9: Ohayo sensei! Verify CASM generation impact on the manifest file handling
The addition of casm = true enables Cairo Assembly output generation. While this aligns with the PR's objective of proper manifest file handling, we should verify that this doesn't affect the build artifacts' location that the manifest existence check relies upon.
✅ Verification successful
Let me check how the build configuration and manifest paths are handled to ensure CASM doesn't affect the manifest location.
Ohayo sensei! CASM enablement is safe for manifest handling
Based on the codebase analysis, the manifest file handling is independent of CASM configuration:
- Manifest files are consistently placed under
target/<profile>/manifest_*.json - The manifest path handling in
sozois profile-aware but CASM-agnostic - Build scripts show consistent manifest path usage regardless of build configuration
The casm = true setting only affects the compilation artifacts and doesn't interfere with manifest file location or existence checking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if CASM output affects manifest file location
# Look for manifest file path references in the codebase
rg -g '!target' -g '!*.json' "manifest_.+\.json" -A 3
Length of output: 2347
Script:
#!/bin/bash
# Check if CASM output affects build target paths
rg -g '!target' "target.*manifest" -A 2
# Check build configuration handling in the codebase
ast-grep --pattern 'fn $_(casm: $_) {
$$$
}'
Length of output: 1722
crates/sozo/scarbext/src/workspace.rs (1)
149-152: Verify manifest file handling across the codebase
Let's ensure all callers properly handle the None case from this method.
✅ Verification successful
I need to see how this method is actually called and handled in the codebase to verify the impact of the None case.
I need to see the actual implementation and error handling to verify the impact of returning None.
Let me try with more precise patterns to find the implementation and usage, sensei!
Ohayo sensei! Based on the code analysis, I can now provide the final response:
✅ Manifest None case is properly handled across the codebase
All callers of read_manifest_profile() handle the None case correctly:
- In
utils.rs, it explicitly checkslocal_manifest.is_none() - In
call.rs, it uses the?operator and handles the None case in the control flow - In
account/controller.rs, it's the only place that unwraps, but it's in test code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to read_manifest_profile to verify proper handling
ast-grep --pattern 'read_manifest_profile()' -A 5
# Search for any direct manifest file access that might bypass this method
rg "manifest_.*\.json" --type rust
Length of output: 1107
Script:
#!/bin/bash
# Let's search for the actual usage of read_manifest_profile method
ast-grep --pattern 'read_manifest_profile(' -A 10
# Also search for any error handling around manifest operations
rg "manifest.*\?" --type rust -A 3
Length of output: 41230
Script:
#!/bin/bash
# Let's check the implementation of read_manifest_profile and its callers
ast-grep --pattern 'fn read_manifest_profile' -A 10
# Also check for any error handling in the callers
ast-grep --pattern 'read_manifest_profile()?' -A 5
Length of output: 106
Script:
#!/bin/bash
# Search for the function definition and its usage with proper Rust syntax
ast-grep --pattern 'fn read_manifest_profile(&self)' -A 10
# Look for method calls with proper Rust syntax
rg 'ws\.read_manifest_profile\(\)' -A 5 --type rust
Length of output: 2408
examples/spawn-and-move/manifest_dev.json (3)
3-4: Ohayo sensei! World contract configuration has been updated.
The world contract's class_hash and address have been modified, which is consistent with the contract redeployment pattern.
Let's verify the world contract deployment:
✅ Verification successful
Ohayo sensei! World contract configuration is properly synchronized! ✨
The world contract's configuration is consistent across all relevant files:
- The address
0x70058e3886cb7411e8a77db90ee3dd453ac16b763b30bd99b3c8440fe42056eis correctly set in bothmanifest_dev.jsonanddojo_dev.toml - The class_hash
0x79d9ce84b97bcc2a631996c3100d57966fc2f5b061fb1ec4dfd0040976bcac6is properly referenced in the manifest files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify world contract deployment consistency
# Check if the new world address exists in other manifest files
rg -l "0x70058e3886cb7411e8a77db90ee3dd453ac16b763b30bd99b3c8440fe42056e"
Length of output: 153
Script:
#!/bin/bash
# Let's verify if the class_hash is consistent across files
rg -l "0x79d9ce84b97bcc2a631996c3100d57966fc2f5b061fb1ec4dfd0040976bcac6"
# Also check the content of dojo_dev.toml to ensure address matches
cat examples/spawn-and-move/dojo_dev.toml
Length of output: 737
2181-2181: Event class hashes have been updated.
The class hashes for the Moved and MyInit events have been updated, maintaining consistency with the contract changes.
Let's verify event configuration consistency:
Also applies to: 2187-2187
✅ Verification successful
Event class hash update is correctly configured
The new class hash 0x2999886b1a0ee530eb279f9808632b70750ac93418da8493fb554eebfb7b99a is properly set for the "ns-Moved" event in the manifest_dev.json file. The MyInit event also has its unique class hash 0x4771fc691467af7b9b606ae34d5fa9abe762adb856d65d2c508ce66b3941671. No references to old class hashes were found in the codebase, confirming a clean update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify event configuration consistency
# Check if old event class hashes exist in other files
rg "0x3dc79bcf7e88304d1331cdf9e232ff9f78ac19a719e998bd68016eb5ebb3e38|0x7ddb49b2db32089d1bb14a6f66d91582fffdd09d18325187840de65ae443598"
Length of output: 138
Script:
#!/bin/bash
# Let's search for the new class hash in the manifest files
rg "0x2999886b1a0ee530eb279f9808632b70750ac93418da8493fb554eebfb7b99a" examples/
# Also search for any other class hashes in manifest files to compare
rg -A 1 -B 1 "\"class_hash\":" examples/
Length of output: 8895
1246-1247: Contract addresses and class hashes have been updated consistently.
The changes maintain the contract deployment pattern across all contracts:
- Actions contract (ns-actions)
- Dungeon contract (ns-dungeon)
- Mock token contract (ns-mock_token)
- Others contract (ns-others)
Let's verify contract deployment consistency:
Also applies to: 1587-1587, 1781-1781, 1957-1957
✅ Verification successful
Ohayo! Contract addresses and class hashes have been updated consistently across all files, sensei!
The old contract addresses only appear in test data files (bin/sozo/tests/test_data/policies.json), which is expected and doesn't affect the actual contract deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment consistency
# Check if any of the old contract addresses still exist in other files
rg "0x7d32611d1b96df37b041f0a285bf340227cebc144d46c2f34cedd81e2fd5239|0x74967fafd9ea26b0c14287fdafa5867cf6e2d16d9e6fda3dde4361a7cf75c9d|0x7077c3246c125a91b17e8c0a90f45af790ad6feabe65a3df225277d9eb02310|0x7e2c18814dd45847ae85d3c8eb40196cc2aa869614efd1ff67edf380c45cb8e"
Length of output: 1612
Script:
#!/bin/bash
# Let's check if the new contract addresses are consistently used across the codebase
rg "0x1cc5853cbf65c96d7c3c8f951e6315f3c1f19a34ace7b579da9ba87b974dce3|0x636c2cf31b094097625cb5ada96f54ee9a3f7bc6d8cde00cc85e5ef0c622c8b"
# Also check manifest files for any potential inconsistencies
fd manifest.*\.json --exec cat {}
Length of output: 119701
The existence of the
manifest_<PROFILE>.jsonfile is important to speed, to avoid re-syncing the world from the chain.However, the check was made on the directory of the file, instead of the file itself. So the
--diffoption wasn't applied or if the file is removed.Summary by CodeRabbit
New Features
Bug Fixes
Chores