fix(sozo): remove skipped contracts from manifest #2787
Conversation
WalkthroughOhayo, sensei! This pull request introduces changes primarily to the Changes
Possibly related PRs
🪧 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 (1)
crates/dojo/world/src/diff/manifest.rs (1)
125-128: Consider extracting skip logic to a traitOhayo sensei! While the current implementation works well, we could make the skip functionality more reusable by extracting it to a trait. This would allow other parts of the system to utilize the same skip logic without duplicating code.
Example approach:
trait Skippable { fn should_skip(&self, config: &ProfileConfig) -> bool; } impl Skippable for Resource { fn should_skip(&self, config: &ProfileConfig) -> bool { config.is_skipped(&self.tag()) } }Would you like me to elaborate on this suggestion or provide a more detailed implementation?
📜 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 (3)
crates/dojo/world/src/diff/manifest.rs(1 hunks)crates/katana/feeder-gateway/Cargo.toml(0 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/feeder-gateway/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (1)
crates/dojo/world/src/diff/manifest.rs (1)
125-128: Ohayo! Clean implementation of the skip functionality!
The addition of the skip check effectively addresses the issue of removing skipped contracts from the manifest, exactly as intended in the PR objectives.
Let's verify the is_skipped functionality with this script:
✅ Verification successful
Let's gather more information about the is_skipped implementation to ensure it's properly tested.
Let's check the actual implementation of is_skipped and any related tests.
Ohayo! The skip functionality looks solid, sensei!
The implementation is properly tested and well-integrated:
- The
is_skippedfunction is correctly implemented inProfileConfig, checking against theskip_contractslist - The functionality is tested in the config tests with example contract "module::my-contract"
- Multiple examples in the codebase (dojo_saya.toml, dojo_sepolia.toml, dojo_dev.toml) demonstrate proper usage of
skip_contracts
The skip check in the manifest is correctly leveraging this tested functionality, making it a reliable implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify is_skipped implementation and test coverage
# Check for tests of is_skipped
rg -A 5 "is_skipped.*test"
# Look for toml config examples using skip_contracts
rg -A 3 "skip_contracts.*=.*\[.*\]"
# Check for any existing skip-related test files
fd -t f "skip.*test"
Length of output: 828
Script:
#!/bin/bash
# Check the implementation of is_skipped
ast-grep --pattern 'fn is_skipped'
# Look for any unit tests in the profile_config.rs file
rg -A 10 "#\[test\]" crates/dojo/world/src/config/profile_config.rs
# Check for any integration tests that might test this functionality
fd -t f "test.*rs" crates/dojo/world/src | xargs rg -l "is_skipped"
Length of output: 1245
Script:
#!/bin/bash
# Find the implementation of is_skipped using a more specific pattern
ast-grep --pattern 'impl $_$ {
$$$
fn is_skipped($_$) $_$ {
$$$
}
$$$
}'
# Look for any tests that might use skip_contracts
rg -A 5 "skip_contracts" crates/dojo/world/src/config/profile_config.rs
# Check for any manifest-related tests that might verify skip functionality
rg -l "manifest.*test" crates/dojo/world/src/diff/
Length of output: 901
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2787 +/- ##
=======================================
Coverage 56.00% 56.00%
=======================================
Files 434 434
Lines 55090 55097 +7
=======================================
+ Hits 30854 30858 +4
- Misses 24236 24239 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/sozo/ops/src/migrate/mod.rs (2)
416-416: Consider standardizing the trace message format.Ohayo sensei! While the trace logging is good, the message format differs slightly from the other skip traces:
- Line 253: "Contract init skipping resource."
- Line 348: "Sync permissions skipping resource."
- Line 416: "Sync skipping resource."
Consider standardizing these messages for consistency in logs.
- trace!(tag = resource.tag(), "Sync skipping resource."); + trace!(tag = resource.tag(), "Resource sync skipping resource.");
Line range hint
253-416: Consider enhancing skip tracing with additional context.The trace logs could be more informative by including additional context about why the resource was skipped (e.g., configuration setting that triggered the skip).
- trace!(tag = resource.tag(), "Contract init skipping resource."); + trace!( + tag = resource.tag(), + reason = "skip_contracts config", + "Contract init skipping resource." + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/sozo/ops/src/migrate/mod.rs(3 hunks)
🔇 Additional comments (2)
crates/sozo/ops/src/migrate/mod.rs (2)
253-253: LGTM! Trace logging added for skipped contract initialization.
The trace log provides clear visibility into which contracts are being skipped during initialization.
348-348: LGTM! Trace logging added for skipped permission syncing.
The trace log effectively tracks resources being skipped during permission synchronization.
Description
During the migration, some contracts can be ignored using the following setting in
dojo_<PROFILE>.toml:However, even if sozo correctly skips the contract during the resource synchronization, the manifest was including them.
This PR ensures they are removed from the generated manifest file.
Related issue
Fixes #2786.
Summary by CodeRabbit
New Features
Chores
Cargo.tomlby removing workspace configuration for the license file.dojo_dev.tomlby removing a trailing newline.