fix(bench): fix sozo bench #2601
Conversation
WalkthroughOhayo, sensei! This pull request introduces several changes across configuration files and a new benchmark test. 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: 2
🧹 Outside diff range and nitpick comments (6)
crates/benches/contracts/dojo_dev.toml (4)
1-4: LGTM! Consider documenting the seed choice.Ohayo! The world configuration looks good, sensei. The predictable seed value "bench" is appropriate for benchmarking purposes.
Consider adding a comment explaining why this specific seed value was chosen to help future maintainers understand the reasoning.
17-19: Document the significance of initialization values.Ohayo sensei! The hex value
0xfffeis used for both components. Could you please document why this specific value was chosen?Add a comment explaining the significance of
0xfffefor future maintainers.
21-26: Document the permission structure rationale.The writers and owners configuration is quite complex, sensei. While it works, it might benefit from documentation explaining why certain namespaces need these specific permissions for the benchmark.
Consider adding comments to explain:
- Why
ns-Mneeds write access to bothnsandns2components- The relationship between writers and owners in this benchmark context
28-30: Document migration configuration decisions.Ohayo sensei! The migration section would benefit from some context:
- Why is this specific initialization order required?
- Why is
ns-c3being skipped?Add comments explaining these configuration choices to help other developers understand the benchmark setup requirements.
crates/dojo/test-utils/src/compiler.rs (2)
Line range hint
196-270: Ohayo! Let's make the file handling more robust, sensei!The
copy_project_tempfunction could benefit from some improvements:
- Several
unwrapcalls could panic - consider proper error handling- Hard-coded strings like "Scarb.toml" should be constants
- Path manipulations could be more robust using proper path joining methods
Here's a suggested improvement for part of the function:
+const SCARB_TOML: &str = "Scarb.toml"; +const WORKSPACE_KEY: &str = "workspace"; +const DEPENDENCIES_KEY: &str = "dependencies"; if file_name == "Scarb.toml" { - let mut contents = String::new(); - File::open(&dest_path) - .and_then(|mut file| file.read_to_string(&mut contents)) - .unwrap_or_else(|_| panic!("Failed to read {file_name}")); + let contents = fs::read_to_string(&dest_path) + .map_err(|e| io::Error::new( + e.kind(), + format!("Failed to read {}: {}", file_name, e) + ))?;
Line range hint
1-329: Ohayo! Some architectural suggestions for the test utilities, sensei!While the simplification of Cairo plugin handling is good, consider these architectural improvements:
- Create a dedicated error type for test utilities instead of using raw
unwrap()- Consider introducing a builder pattern for test configuration
- Add logging for better test debugging
Example error type:
#[derive(Debug, thiserror::Error)] pub enum TestUtilError { #[error("Failed to copy project: {0}")] ProjectCopy(#[from] std::io::Error), #[error("Config error: {0}")] Config(#[from] anyhow::Error), }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
crates/benches/contracts/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/benches/contracts/Scarb.toml(1 hunks)crates/benches/contracts/dojo_dev.toml(1 hunks)crates/dojo/test-utils/src/compiler.rs(1 hunks)output_sozo.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- output_sozo.txt
🧰 Additional context used
🪛 Gitleaks
crates/benches/contracts/dojo_dev.toml
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
crates/benches/contracts/Scarb.toml (3)
6-7: External contract build configuration looks good, sensei!
The starknet-contract target is correctly configured to build the world contract from dojo.
1-11: Verify removed configurations impact, sensei!
According to the AI summary, several configuration sections were removed ([cairo], [tool.dojo], [tool.dojo.env]). These settings might have moved to the new dojo_dev.toml file.
Let's verify the configuration migration:
#!/bin/bash
# Description: Verify if removed configurations exist in dojo_dev.toml
# Expect: Configurations should be present in dojo_dev.toml
# Check for the presence of dojo_dev.toml
echo "Checking for dojo_dev.toml:"
fd "dojo_dev.toml"
# If found, check its contents for migrated configurations
echo -e "\nChecking for migrated configurations:"
rg "\[(tool\.dojo|cairo|env)\]" -g "dojo_dev.toml"2-2: Ohayo sensei! Version bump looks good but needs verification.
The cairo-version and package version updates align with the PR objectives for fixing the bench system.
Let's verify the version compatibility:
#!/bin/bash
# Description: Verify cairo version compatibility across the project
# Expect: All Scarb.toml files should use compatible cairo versions
# Search for cairo-version declarations
echo "Checking cairo version declarations:"
rg "cairo-version" -g "Scarb.toml"
# Search for package versions
echo -e "\nChecking package versions:"
rg "^version = " -g "Scarb.toml"Also applies to: 4-4
crates/benches/contracts/dojo_dev.toml (1)
13-15: LGTM! Clean namespace configuration.
The namespace configuration is well-structured, sensei. The mappings are clear and logical.
crates/dojo/test-utils/src/compiler.rs (2)
9-9: Ohayo! Import cleanup looks good, sensei!
The removal of Cairo plugin-related imports aligns well with the simplification goals.
Line range hint 314-329: Consider maintaining error propagation, sensei!
While simplifying the function is good, removing error handling (anyhow::Result) might hide configuration issues. Even in test utilities, proper error propagation helps with debugging when tests fail.
Let's check how this function is used in the codebase:
Consider keeping the Result type and explicit error handling:
-pub fn build_test_config(path: &str, profile: Profile) -> Config {
+pub fn build_test_config(path: &str, profile: Profile) -> anyhow::Result<Config> {
let cache_dir = TempDir::new().unwrap();
let path = Utf8PathBuf::from_path_buf(path.into()).unwrap();
println!("path: {:?}", path);
- Config::builder(path.canonicalize_utf8().unwrap())
+ Ok(Config::builder(path.canonicalize_utf8()?)
.global_cache_dir_override(Some(Utf8Path::from_path(cache_dir.path()).unwrap()))
.ui_verbosity(Verbosity::Verbose)
.log_filter_directive(env::var_os("SCARB_LOG"))
.profile(profile)
- .build()
+ .build()?)| dojo = { git = "https://github.com/dojoengine/dojo", tag = "v1.0.0-rc.0" } | ||
| starknet = "=2.8.4" |
There was a problem hiding this comment.
💡 Codebase verification
Dependencies are mostly aligned, with one minor inconsistency sensei!
Most of the project uses starknet 2.8.4, but there's one outlier in crates/katana/contracts/Scarb.toml using version 2.8.2. The dojo dependencies are properly configured, with the benches using the git tag that matches the package version (v1.0.0-rc.0).
crates/katana/contracts/Scarb.toml: Consider updating starknet from "2.8.2" to "=2.8.4" to maintain consistency
🔗 Analysis chain
Dependencies alignment check needed.
The dojo dependency tag matches the package version, which is good practice. The addition of starknet dependency with a specific version is also appropriate.
Let's verify dependency consistency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent dependency versions across the project
# Expect: All dojo and starknet dependencies should use compatible versions
# Search for dojo dependencies
echo "Checking dojo dependency declarations:"
rg "dojo = \{" -g "Scarb.toml" -A 2
# Search for starknet dependencies
echo -e "\nChecking starknet dependency declarations:"
rg "^starknet = " -g "Scarb.toml"
Length of output: 1486
Script:
#!/bin/bash
# Let's try a more focused search for dependencies in the specific file and related ones
# Check the specific file's dependencies
echo "Checking dependencies in benches/contracts/Scarb.toml:"
cat crates/benches/contracts/Scarb.toml
# Check dojo dependencies across all Scarb.toml files
echo -e "\nChecking all dojo dependencies:"
rg "dojo.*=.*" -g "Scarb.toml"
# Check starknet dependencies across all Scarb.toml files
echo -e "\nChecking all starknet dependencies:"
rg "starknet.*=.*" -g "Scarb.toml"
Length of output: 2046
| [env] | ||
| rpc_url = "http://localhost:5050/" | ||
| # Default account for katana with seed = 0 | ||
| account_address = "0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec" | ||
| private_key = "0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912" | ||
| #world_address = "0x077c0dc7c1aba7f8842aff393ce6aa71fa675b4ced1bc927f7fc971b6acd92fc" |
There was a problem hiding this comment.
Security: Move sensitive data to environment variables.
Ohayo sensei! While this is a development configuration, it's still risky to hardcode private keys in configuration files as they might accidentally be committed to version control.
Consider this safer approach:
[env]
rpc_url = "http://localhost:5050/"
# Default account for katana with seed = 0
-account_address = "0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec"
-private_key = "0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912"
+account_address = "${BENCH_ACCOUNT_ADDRESS}"
+private_key = "${BENCH_PRIVATE_KEY}"Then provide these values through environment variables during benchmarking.
📝 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.
| [env] | |
| rpc_url = "http://localhost:5050/" | |
| # Default account for katana with seed = 0 | |
| account_address = "0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec" | |
| private_key = "0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912" | |
| #world_address = "0x077c0dc7c1aba7f8842aff393ce6aa71fa675b4ced1bc927f7fc971b6acd92fc" | |
| [env] | |
| rpc_url = "http://localhost:5050/" | |
| # Default account for katana with seed = 0 | |
| account_address = "${BENCH_ACCOUNT_ADDRESS}" | |
| private_key = "${BENCH_PRIVATE_KEY}" | |
| #world_address = "0x077c0dc7c1aba7f8842aff393ce6aa71fa675b4ced1bc927f7fc971b6acd92fc" |
🧰 Tools
🪛 Gitleaks
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Summary by CodeRabbit
New Features
Sozo.Coldcomponent to evaluate performance.Bug Fixes
Documentation