fix(lang): change the upgrade contract salt to allow multicall #2617
Conversation
WalkthroughOhayo, sensei! This pull request introduces several modifications across multiple files, primarily focusing on enhancing error messaging, user interface interactions, and version compatibility checks in the migration process. Key updates include improved error handling in the 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: 0
🧹 Outside diff range and nitpick comments (2)
bin/sozo/src/utils.rs (1)
Line range hint
156-182: Ohayo! Great addition of account verification and UI handling, sensei!The changes improve user experience by:
- Properly handling UI spinner during password prompts
- Adding account existence verification
Consider enhancing the error message to guide users on how to resolve the account not found situation.
- return Err(anyhow!("Account with address {:#x} doesn't exist.", account.address())); + return Err(anyhow!("Account with address {:#x} doesn't exist. Please ensure the account is deployed and the correct address is provided.", account.address()));crates/sozo/ops/src/migrate/mod.rs (1)
322-326: Ohayo! Consider extracting the repeated counting logic.The pattern of checking for non-empty calls and incrementing the counter is repeated three times. We could make this more maintainable by extracting it into a helper function.
Consider this refactor:
fn increment_if_has_calls(n_resources: &mut usize, calls: &[Call]) { if !calls.is_empty() { *n_resources += 1; } }Then use it in each case:
- if !contract_calls.is_empty() { - n_resources += 1; - } + increment_if_has_calls(&mut n_resources, &contract_calls);Also applies to: 332-336, 342-346
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
bin/sozo/src/utils.rs(1 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/sozo/ops/src/migrate/mod.rs(4 hunks)
🔇 Additional comments (5)
bin/sozo/src/utils.rs (3)
43-44: Ohayo sensei! Nice improvement to the error message!
The additional clarification about world upgrades in the warning message helps users understand that address mismatches are expected during upgrade operations.
Line range hint 184-251: Excellent implementation of version compatibility checks, sensei!
The version compatibility logic is well-implemented with:
- Clear documentation
- Specific backward compatibility rules
- Comprehensive error handling
- Thorough test coverage
Line range hint 1-251: Verify alignment with PR objectives
While the changes in this file improve error handling and user experience, I don't see direct changes related to the PR's main objective of fixing the upgrade contract salt for multicall support. Could you verify if changes in other files address this core requirement?
crates/sozo/ops/src/migrate/mod.rs (1)
628-631: Ohayo sensei! The UI feedback implementation looks great!
The addition of UI feedback during world deployment and upgrade operations improves user experience by providing clear status updates. This aligns well with the PR's objective of enhancing the migration process.
Also applies to: 635-635, 659-659
crates/dojo/core/src/world/world_contract.cairo (1)
697-708: Ohayo, sensei! The updated salt computation ensures unique upgrades during multicalls
By hashing the transaction hash with the namespace hash to generate the salt, this implementation effectively prevents conflicts when upgrading multiple contracts in the same multicall transaction.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
scripts/extract_test_db.sh (1)
1-2: Enhance documentation for better maintainability.The current comment could be more descriptive about the script's purpose and requirements.
Consider expanding the documentation:
-# Ensures that the user has locally the db dir in /tmp. +# This script prepares test databases by extracting them to /tmp directory. +# It's used as part of the test infrastructure and is called by rebuild_test_artifacts.sh. +# +# Requirements: +# - Write access to /tmp directory +# - spawn-and-move-db.tar.gz and types-test-db.tar.gz files in current directory🧰 Tools
🪛 Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
examples/spawn-and-move/dojo_dev.toml (1)
Line range hint
1-24: Consider adding a comment explaining the world address derivation.Since this is an example file that developers might use as reference, it would be helpful to add a comment explaining how this world address was derived, especially in the context of the new salt implementation for multicalls.
Add a comment above the world_address line:
[env] rpc_url = "http://localhost:5050/" # Default account for katana with seed = 0 account_address = "0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba" private_key = "0x1800000000300000180000000000030000000000003006001800006600" +# World address derived using the updated salt implementation for multicall support world_address = "0xecd610a15ce418719008d506c662ec3e420cf17b4b6d086e53281f30425115"scripts/rebuild_test_artifacts.sh (2)
10-10: Consider adding more descriptive formatting comment.The comment "Some formatting" could be more informative about what's being formatted.
-# Some formatting. +# Format Rust code using nightly formatter.
Line range hint
1-42: Consider adding error handling for critical commands.The script executes several critical commands but doesn't check their exit status. This could lead to silent failures.
Consider adding error handling like this:
#!/bin/bash + +# Exit on error +set -e + +# Function to handle errors +handle_error() { + echo "Error: Command failed on line $1" + exit 1 +} + +# Set error handler +trap 'handle_error $LINENO' ERRbin/sozo/tests/test_data/policies.json (1)
1-117: Consider documenting the target address purposes.Ohayo sensei! While the organization is good, it would be helpful to add comments explaining the purpose of each target address group (e.g., world operations, player operations, etc.).
Here's a suggested format:
[ + // World contract - Handles core world operations { "target": "0xecd610a15ce418719008d506c662ec3e420cf17b4b6d086e53281f30425115", "method": "uuid" }, + // Player contract - Manages player-related operations { "target": "0x48c075712ddb98febd836b09049e3d36c7b061fc7027ba5021f8cf00778b7bf", "method": "spawn" },CONTRIBUTING.md (1)
54-83: Ohayo sensei! Consider enhancing the testing documentation.The new testing section is comprehensive and well-structured. Consider adding these helpful details:
- Required disk space for the compressed and extracted databases
- Explanation of what the
POLICIES_FIX=1environment variable does and when it should be used- Expected execution time for the test suite
Add these details by applying this diff:
## Testing the changes To speed the test suite and avoid migrating dojo projects again, `katana` databases are compressed and stored in the repo. + +> Note: The compressed databases require approximately X MB of disk space, and Y MB when extracted. If you don't have any change in the `dojo/core` crate or any cairo example, you only have to extract the databases: ```bash bash scripts/extract_test_db.shTo test your changes, if you have modified the
dojo/corecrate or any cairo example, you will need to regenerate the databases:# Prints the policies to then be copied into the `sozo/tests/test_data/policies.json` test file to ensure entrypoints and addresses are up to date. +# The POLICIES_FIX environment variable enables automatic policy updates during testing. POLICIES_FIX=1 cargo nextest run --all-features --build-jobs 20 --workspace --nocapture policies # Ensures the test databases are up to date. bash scripts/rebuild_test_artifacts.sh
+> Note: The complete test suite typically takes X minutes to run.
</blockquote></details> <details> <summary>xtask/generate-test-db/src/main.rs (2)</summary><blockquote> `39-61`: **Ohayo! Consider enhancing error handling and messaging, sensei!** While the validation logic is good, we could make it more robust and user-friendly: 1. The unwrap() calls could be replaced with proper error handling 2. The error message could include the actual mismatched values Here's a suggested improvement: ```diff let deterministic_world_address = world_local.deterministic_world_address() - .unwrap(); + .map_err(|e| anyhow::anyhow!("Failed to get deterministic world address: {}", e))?; let config_world_address = if let Some(env) = &profile_config.env { env.world_address() .map_or_else( || world_local.deterministic_world_address(), - |wa| Ok(Felt::from_str(wa).unwrap()), + |wa| Felt::from_str(wa).map_err(|e| anyhow::anyhow!("Invalid world address in config: {}", e)) ) - .unwrap() + .map_err(|e| anyhow::anyhow!("Failed to get configured world address: {}", e))? } else { deterministic_world_address }; if deterministic_world_address != config_world_address { panic!( - "The deterministic world address is different from the config world address. Please \ - review the `dojo_dev.toml` file of spawn-and-move example." + "World address mismatch in spawn-and-move example:\n\ + Deterministic: {}\n\ + Configured : {}\n\ + Please review the `dojo_dev.toml` file.", + deterministic_world_address, + config_world_address ); }
99-99: Enhance documentation clarity, sensei!The comment about deterministic address usage could be more descriptive.
Consider expanding the comment:
- // No world address in config, so it should always pick the deterministic one. + // When no world_address is specified in the configuration (dojo_dev.toml), + // we default to using the deterministic address calculated from the contract's + // class hash and constructor calldata.
📜 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 (6)
CONTRIBUTING.md(2 hunks)bin/sozo/tests/test_data/policies.json(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)scripts/extract_test_db.sh(1 hunks)scripts/rebuild_test_artifacts.sh(2 hunks)xtask/generate-test-db/src/main.rs(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...e the state of your World. - dojo/core: The core contract of Dojo written in Ca...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ... of Dojo written in Cairo. - dojo/lang: The Dojo plugin for the Cairo compiler....
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Shellcheck
scripts/extract_test_db.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (7)
examples/spawn-and-move/dojo_dev.toml (1)
14-14: Ohayo sensei! Verify the world address consistency.
The updated world address should be verified to ensure it matches the deterministic address generation with the new salt implementation for multicalls.
✅ Verification successful
Ohayo sensei! The world address is verified and consistent.
The world address 0xecd610a15ce418719008d506c662ec3e420cf17b4b6d086e53281f30425115 is correctly used in both the example configuration and the policies.json file, where it's associated with core world contract methods (uuid, register_model, register_contract, etc.). This consistency indicates the address is properly aligned with the system's requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for world address references and related tests
# to ensure consistency across the codebase
# Look for world address references
rg -l "0xecd610a15ce418719008d506c662ec3e420cf17b4b6d086e53281f30425115"
# Search for related test cases that might need updating
rg -l "world_address.*example.*spawn.*move" --type rust
Length of output: 207
Script:
#!/bin/bash
# Let's check the content of the policies.json to understand the context of this address
cat bin/sozo/tests/test_data/policies.json
# Let's also check if there are any other world addresses in the codebase for comparison
rg -g '!target' -g '!*.lock' "world_address.*=.*0x[a-fA-F0-9]" --no-filename
# Check for any documentation or comments about world address generation
rg -g '!target' "world.*address.*generation" -A 3 -B 3
Length of output: 4349
scripts/rebuild_test_artifacts.sh (2)
8-8: Ohayo sensei! The sozo binary build looks good!
Adding the sozo binary build before formatting is a logical step since it's needed for later commands.
18-18: The cleanup comment update improves clarity.
The change from "Cleanup" to "Manual forced cleanup" better describes the intention of this section.
bin/sozo/tests/test_data/policies.json (3)
11-35: Player-related operations consolidated under a single target.
Nice consolidation sensei! All player-related methods (spawn, move, set_player_config, etc.) are now using the same target address 0x48c07.... This is a good practice for better organization and maintenance.
47-111: World operations consolidated under a single target.
Excellent organization! All world-related operations (uuid, metadata, registration, etc.) are consolidated under 0xecd61.... This makes the system more maintainable and easier to understand.
Let's verify this is the correct world address:
#!/bin/bash
# Search for world address references in configuration files
echo "Verifying world address in config files..."
rg "0xecd610a15ce418719008d506c662ec3e420cf17b4b6d086e53281f30425115" --type toml3-5:
Ohayo sensei! Multiple upgrade targets might conflict with multicall objectives.
I notice there are 5 different target addresses for the "upgrade" method. This could potentially lead to conflicts during multicall operations, which is the exact issue this PR aims to solve.
Let's verify the upgrade targets usage:
Also applies to: 7-9, 39-41, 43-45, 115-117
xtask/generate-test-db/src/main.rs (1)
39-61: Verify test coverage for multicall upgrades
Since this PR addresses multicall upgrade issues, we should ensure our test databases support this scenario.
Let's check if the test databases include multicall upgrade scenarios:
✅ Verification successful
World address verification is sufficient for upgrade testing
The spawn-and-move example already includes comprehensive upgrade capabilities:
- It has the
IUpgradeableinterface implementation - Contains proper upgrade events and structures
- Includes world address verification in both dev and release configurations
The current world address verification in generate-test-db is adequate because:
- It ensures consistent world addresses between local and config environments
- The upgrade functionality is part of the core contract interface, not specific to multicalls
- Test database generation correctly maintains world contract addressing, which is crucial for all upgrade scenarios including multicalls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for multicall upgrade tests in the workspace
rg -l 'multicall.*upgrade|upgrade.*multicall' examples/spawn-and-move/
rg -l 'multicall.*upgrade|upgrade.*multicall' crates/torii/types-test/
Length of output: 145
Script:
#!/bin/bash
# Let's check for any upgrade-related tests or configurations in spawn-and-move example
rg -A 5 'world_address|upgrade' examples/spawn-and-move/
# Also check for any upgrade-related code in the test database generation
rg -A 5 'world_address|upgrade' xtask/generate-test-db/
Length of output: 18098
| @@ -0,0 +1,6 @@ | |||
| # Ensures that the user has locally the db dir in /tmp. | |||
There was a problem hiding this comment.
Ohayo sensei! Add a shebang line for shell script portability.
The script is missing a shebang line which is essential for proper shell script execution across different environments.
Apply this diff at the beginning of the file:
+#!/bin/bash
# Ensures that the user has locally the db dir in /tmp.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Ensures that the user has locally the db dir in /tmp. | |
| #!/bin/bash | |
| # Ensures that the user has locally the db dir in /tmp. |
🧰 Tools
🪛 Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
| rm -rf /tmp/spawn-and-move-db | ||
| rm -rf /tmp/types-test-db | ||
| tar xzf spawn-and-move-db.tar.gz -C /tmp/ | ||
| tar xzf types-test-db.tar.gz -C /tmp/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation checks, sensei!
The script should handle potential failures and validate operations for robustness.
Consider this improved version with error handling:
+set -euo pipefail
+
+# Check if source files exist
+for file in spawn-and-move-db.tar.gz types-test-db.tar.gz; do
+ if [ ! -f "$file" ]; then
+ echo "Error: Required file $file not found"
+ exit 1
+ fi
+done
+
+# Check if we have write permission to /tmp
+if [ ! -w "/tmp" ]; then
+ echo "Error: No write permission in /tmp directory"
+ exit 1
+fi
+
rm -rf /tmp/spawn-and-move-db
rm -rf /tmp/types-test-db
-tar xzf spawn-and-move-db.tar.gz -C /tmp/
-tar xzf types-test-db.tar.gz -C /tmp/
+
+# Extract with error checking
+for file in spawn-and-move-db.tar.gz types-test-db.tar.gz; do
+ echo "Extracting $file..."
+ if ! tar xzf "$file" -C /tmp/; then
+ echo "Error: Failed to extract $file"
+ exit 1
+ fi
+done
+
+echo "Database extraction completed successfully"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf /tmp/spawn-and-move-db | |
| rm -rf /tmp/types-test-db | |
| tar xzf spawn-and-move-db.tar.gz -C /tmp/ | |
| tar xzf types-test-db.tar.gz -C /tmp/ | |
| set -euo pipefail | |
| # Check if source files exist | |
| for file in spawn-and-move-db.tar.gz types-test-db.tar.gz; do | |
| if [ ! -f "$file" ]; then | |
| echo "Error: Required file $file not found" | |
| exit 1 | |
| fi | |
| done | |
| # Check if we have write permission to /tmp | |
| if [ ! -w "/tmp" ]; then | |
| echo "Error: No write permission in /tmp directory" | |
| exit 1 | |
| fi | |
| rm -rf /tmp/spawn-and-move-db | |
| rm -rf /tmp/types-test-db | |
| # Extract with error checking | |
| for file in spawn-and-move-db.tar.gz types-test-db.tar.gz; do | |
| echo "Extracting $file..." | |
| if ! tar xzf "$file" -C /tmp/; then | |
| echo "Error: Failed to extract $file" | |
| exit 1 | |
| fi | |
| done | |
| echo "Database extraction completed successfully" |
|
|
||
| # Extracts the database for testing. | ||
| bash ./scripts/extract_test_db.sh |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! The script needs execute permissions
The script exists but lacks execute permissions (currently: -rw-r--r--). Please add execute permissions using:
chmod +x ./scripts/extract_test_db.sh🔗 Analysis chain
Verify database extraction script existence and permissions.
The script now relies on extract_test_db.sh for database setup. Let's ensure it exists and has proper permissions.
Let me check the content of the script to better understand its state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the extract_test_db.sh script exists and is executable
# Test: Check if the script exists and has execute permissions
if [ -x "./scripts/extract_test_db.sh" ]; then
echo "Script exists and is executable"
else
echo "Warning: ./scripts/extract_test_db.sh is missing or not executable"
ls -l ./scripts/extract_test_db.sh 2>/dev/null || echo "File not found"
fi
Length of output: 298
Script:
#!/bin/bash
# Check the content of extract_test_db.sh
cat ./scripts/extract_test_db.sh
Length of output: 227
| - `dojo/core`: The core contract of Dojo written in Cairo. | ||
| - `dojo/lang`: The Dojo plugin for the Cairo compiler. |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Found inconsistent path formats in documentation files.
The path format change needs to be applied consistently. Found remaining hyphenated formats in:
DEVELOPMENT.md: Usesdojo-coreanddojo-langcrates/dojo/world/src/contracts/abigen/README.md: Referencesdojo-core
These instances should be updated to use the new slash format (dojo/core, dojo/lang) to maintain consistency with the changes in CONTRIBUTING.md.
🔗 Analysis chain
Ohayo sensei! Verify path format consistency across the codebase.
The path format change from hyphenated (dojo-core, dojo-lang) to slash format (dojo/core, dojo/lang) looks good, but let's ensure this change is consistent throughout the documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining hyphenated formats in documentation
rg -g '*.md' 'dojo-(core|lang)'
Length of output: 348
🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...e the state of your World. - dojo/core: The core contract of Dojo written in Ca...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ... of Dojo written in Cairo. - dojo/lang: The Dojo plugin for the Cairo compiler....
(UNLIKELY_OPENING_PUNCTUATION)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2617 +/- ##
==========================================
- Coverage 57.93% 57.93% -0.01%
==========================================
Files 393 393
Lines 48567 48587 +20
==========================================
+ Hits 28139 28149 +10
- Misses 20428 20438 +10 ☔ View full report in Codecov by Sentry. |
Description
Since only contracts are using external salt, for the upgrade we needed something else than the transaction hash, which is the same for all contract upgrades in the same transaction using multi calls. This was mostly happening is the same contract is registered in multiple namespaces.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor