fix: use sozo manifest when possible to avoid recomputing diff #2649
Conversation
WalkthroughOhayo, sensei! This pull request introduces several enhancements across multiple files, primarily adding a new boolean field 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: 3
🧹 Outside diff range and nitpick comments (15)
crates/dojo/world/src/contracts/contract_info.rs (3)
1-17: Ohayo! Consider enhancing documentation with usage examples.The documentation clearly explains the purpose and scenarios, but adding a simple usage example would make it even more helpful for developers.
Consider adding something like:
// Example: Creating ContractInfo from manifest let contract_info = ContractInfo::from(&manifest); // vs from world diff let contract_info = ContractInfo::from(&world_diff);
26-35: Consider deriving additional traits for better ergonomics.The struct could benefit from common traits like
Clone,PartialEq, andEqfor better usability in collections and comparisons.-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct ContractInfo {
37-64: Clean implementation with good tracing, sensei!The conversion from Manifest is well-structured and aligns with the PR's objective of prioritizing manifest usage.
Consider adding capacity hint to the HashMap for better performance:
- let mut contracts = HashMap::new(); + let mut contracts = HashMap::with_capacity(manifest.contracts.len() + 1);bin/sozo/src/commands/call.rs (1)
78-92: Consider extracting the world diff computation logic, sensei!While the implementation correctly prioritizes the manifest over computing diffs, we could improve code organization by extracting the world diff computation into a separate function.
Consider applying this refactor:
let contract_address = match &descriptor { ResourceDescriptor::Address(address) => Some(*address), ResourceDescriptor::Tag(tag) => { - let contracts: HashMap<String, ContractInfo> = - if self.diff || local_manifest.is_none() { - let (world_diff, _, _) = utils::get_world_diff_and_provider( - self.starknet.clone(), - self.world, - &ws, - ) - .await?; - - (&world_diff).into() - } else { - (&local_manifest.unwrap()).into() - }; - - contracts.get(tag).map(|c| c.address) + self.resolve_contract_address(tag, &local_manifest, &ws).await? } ResourceDescriptor::Name(_) => { unimplemented!("Expected to be a resolved tag with default namespace.") } } // Add this new method to the impl block: async fn resolve_contract_address( &self, tag: &str, local_manifest: &Option<Manifest>, ws: &Workspace, ) -> Result<Option<FieldElement>> { let contracts: HashMap<String, ContractInfo> = if self.diff || local_manifest.is_none() { let (world_diff, _, _) = utils::get_world_diff_and_provider( self.starknet.clone(), self.world, ws, ) .await?; (&world_diff).into() } else { local_manifest.as_ref().unwrap().into() }; Ok(contracts.get(tag).map(|c| c.address)) }bin/sozo/src/commands/execute.rs (1)
44-48: Ohayo! Consider enhancing the help text for thediffflag.The help text could be more explicit about the default behavior mentioned in the PR objectives.
#[arg(long)] #[arg(help = "If true, sozo will compute the diff of the world from the chain to translate \ - tags to addresses.")] + tags to addresses. By default, sozo will use the manifest when available and \ + only compute the diff when necessary.")] pub diff: bool,crates/sozo/scarbext/src/workspace.rs (3)
32-33: Enhance method documentation for better clarity.Ohayo sensei! While the documentation is present, it could be more descriptive. Consider adding:
- The purpose of reading the manifest
- What happens when the manifest doesn't exist
- Example usage
- /// Reads the manifest for the current profile. + /// Reads the manifest for the current profile from the workspace directory. + /// + /// Returns: + /// - Ok(Some(Manifest)) if the manifest exists and is valid + /// - Ok(None) if the manifest file doesn't exist + /// - Err if there are issues reading or parsing the manifest
142-156: Implementation looks solid with room for minor enhancements.Ohayo sensei! The implementation is well-structured and aligns perfectly with the PR's objective of optimizing contract address resolution. A few suggestions to make it even more robust:
Consider these enhancements:
fn read_manifest_profile(&self) -> Result<Option<Manifest>> { let profile_name = self.current_profile()?.to_string(); let manifest_name = format!("manifest_{}.json", &profile_name); let manifest_dir = self.manifest_path().parent().unwrap(); let manifest_dir = Filesystem::new(manifest_dir.into()); if !manifest_dir.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, "Dojo manifest file", self.config()) + .map_err(|e| anyhow::anyhow!("Failed to open manifest file: {}", e))?; - Ok(Some(serde_json::from_reader(file.deref_mut())?)) + serde_json::from_reader(file.deref_mut()) + .map(Some) + .map_err(|e| anyhow::anyhow!("Failed to parse manifest: {}", e))The changes add more specific error messages to help with debugging.
Line range hint
1-156: Excellent architectural alignment with PR objectives.The addition of
read_manifest_profileis a key enabler for the PR's goal of optimizing contract address resolution. The implementation provides a robust foundation for avoiding unnecessary diff recomputation by allowing manifest-based lookups when possible.Consider adding manifest validation or versioning to ensure compatibility as the manifest structure evolves. This could help prevent issues when working with manifests generated by different versions of the tool.
crates/dojo/world/src/diff/manifest.rs (1)
185-186: Clean refactor of tag handling, sensei!Moving the tag assignment before the match statements reduces code duplication and improves readability. This change maintains the same functionality while making the code more maintainable.
Consider extracting the common member mapping logic into a helper function to further reduce duplication:
fn map_members(members: &[MemberInfo]) -> Vec<Member> { members .iter() .map(|m| Member { name: m.name.clone(), ty: m.ty.clone(), key: m.key }) .collect() }Also applies to: 195-195, 206-206, 214-215, 224-224, 235-235
bin/sozo/src/commands/options/account/mod.rs (1)
97-99: Consider enhancing error context.While the implementation change is correct, we could improve the error handling by providing more context about the contracts parameter.
Consider this enhancement:
- let account = self.controller(url, provider, contracts).await?; + let account = self.controller(url, provider, contracts) + .await + .with_context(|| format!("Failed to create Controller account with {} contracts", contracts.len()))?;bin/sozo/src/commands/events.rs (2)
336-336: Ohayo! Consider enhancing the TODO comment with implementation details, sensei.The TODO comment about schema-based value printing could benefit from additional context. Consider adding:
- Which schema format will be used
- How the schema will be retrieved
- Expected improvements in value presentation
Would you like me to help create a GitHub issue to track this enhancement with detailed requirements?
Line range hint
341-350: Consider adding value validation and improving readability, sensei.While the simplified output format is more consistent, consider these improvements:
- Add validation for the values array length against expected schema
- Add temporary human-readable formatting for common value types (addresses, numbers, etc.)
- Consider adding debug-level logging for raw values
Here's a suggested enhancement:
format!( - "Selector: {:#066x}\nContract: {}\nKeys: {}\nValues: {}", + "Selector: {:#066x}\nContract: {}\nKeys: {}\nValues: {}\nDebug: {:?}", e.selector, contract_tag, e.keys .iter() .map(|k| format!("{:#066x}", k)) .collect::<Vec<String>>() .join(", "), e.values .iter() .map(|v| try_format_known_types(v).unwrap_or_else(|_| format!("{:#066x}", v))) .collect::<Vec<String>>() .join(", "), + e.values, // Raw debug output ),Would you like me to provide an implementation of the
try_format_known_typeshelper function?examples/spawn-and-move/manifest_dev.json (1)
Line range hint
444-604: Added batch operations support in the world interface.New functions added to support batch operations:
emit_eventsfor batch event emissionset_entitiesfor batch entity updatesdelete_entitiesfor batch entity deletionThis enhancement improves efficiency by reducing the number of contract calls needed for bulk operations.
Consider implementing rate limiting or size constraints for these batch operations to prevent potential DoS attacks or excessive gas consumption.
bin/sozo/src/commands/options/account/controller.rs (2)
168-168: Consider externalizingUDC_ADDRESSfor flexibility across networks.Ohayo, sensei! To make the
UDC_ADDRESSadaptable to different environments, consider moving it to a configuration file or environment variable. This change will make it easier to manage and update the address without altering the code.
190-190: Remove redundantunwrap()afterexpectin test code.Ohayo, sensei! The use of
unwrap()immediately afterexpect("Failed to read manifest")is unnecessary, asexpectalready unwraps theResult. Removing the redundantunwrap()improves code clarity.Apply this diff:
-let manifest = ws.read_manifest_profile().expect("Failed to read manifest").unwrap(); +let manifest = ws.read_manifest_profile().expect("Failed to read manifest");
📜 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 (12)
bin/sozo/src/commands/call.rs(5 hunks)bin/sozo/src/commands/events.rs(1 hunks)bin/sozo/src/commands/execute.rs(4 hunks)bin/sozo/src/commands/options/account/controller.rs(6 hunks)bin/sozo/src/commands/options/account/mod.rs(3 hunks)bin/sozo/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/contract_info.rs(1 hunks)crates/dojo/world/src/contracts/mod.rs(1 hunks)crates/dojo/world/src/diff/manifest.rs(6 hunks)crates/sozo/scarbext/src/workspace.rs(3 hunks)examples/simple/manifest_dev.json(1 hunks)examples/spawn-and-move/manifest_dev.json(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/simple/manifest_dev.json
🔇 Additional comments (21)
crates/dojo/world/src/contracts/mod.rs (2)
9-9: Ohayo! Clean module organization, sensei!
The new contract_info module is well-placed among related modules, following Rust's modular design principles.
15-15: LGTM! Excellent public interface design.
The public export of ContractInfo aligns perfectly with the PR's objective of optimizing contract address resolution by making the new functionality accessible where needed.
crates/dojo/world/src/contracts/contract_info.rs (1)
17-25: LGTM! Clean and well-organized imports, sensei.
bin/sozo/src/commands/call.rs (2)
Line range hint 1-48: Ohayo sensei! Clean implementation of the diff flag.
The new diff field and its documentation clearly explain its purpose in controlling whether to compute the world diff from the chain. The implementation aligns well with the PR's objective of optimizing contract address resolution.
106-107: Clean provider setup, sensei!
The provider setup maintains its functionality while aligning with the overall code structure.
bin/sozo/src/commands/execute.rs (3)
81-83: LGTM! Clean implementation of manifest-first approach.
The logic elegantly implements the PR's objective of prioritizing manifest usage while maintaining the diff as a fallback.
139-145: LGTM! Clean separation of provider and account creation.
The changes nicely integrate the new contract management approach while maintaining good separation of concerns.
81-100: Verify performance improvement from manifest-first approach.
The changes look good, but let's verify that using the manifest actually provides a performance benefit.
✅ Verification successful
Performance optimization verified through manifest-first approach
The manifest-first approach with diff fallback is consistently implemented across the codebase. The performance benefit is evident as:
- World diffs are only computed when necessary (manifest missing or --diff flag)
- All relevant commands (execute, call, migrate) follow this pattern
- Manifest reading is properly implemented in the workspace layer for reusability
The changes successfully achieve the performance objectives by avoiding unnecessary diff computations when manifest data is available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for other instances where we might be computing diffs unnecessarily
# This will help ensure we're consistently applying the optimization across the codebase
# Search for world_diff usage patterns
rg -A 5 'world_diff.*=.*get_world_diff'
# Search for manifest usage patterns to ensure we're consistently preferring it
rg -A 5 'read_manifest'
Length of output: 4039
crates/sozo/scarbext/src/workspace.rs (1)
6-6: LGTM! Import added for manifest functionality.
The import is correctly placed and necessary for the new manifest reading capabilities.
crates/dojo/world/src/diff/manifest.rs (1)
144-146: Ohayo! Clean and efficient implementation of contract address lookup.
The new get_contract_address method aligns perfectly with the PR's objective of optimizing address resolution by using the manifest when possible. The implementation is concise and uses find_map efficiently.
Let's verify the contract tag uniqueness to ensure reliable lookups:
✅ Verification successful
Ohayo sensei! The contract address lookup implementation is safe to use.
The get_contract_address method implementation is correct and efficient. While we couldn't find explicit tag uniqueness validation in the codebase, the method's behavior is safe because:
- It returns the first matching contract address using
find_map - Even in case of duplicate tags (which we didn't find in any manifest files), it would consistently return the first match
- The implementation aligns with the manifest's purpose of storing diff results
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate contract tags in manifest files
# Expected: No duplicate tags should be found
# Find all manifest files and check for duplicate tags
fd -e json | xargs -I {} sh -c 'echo "Checking {}:"; jq -r ".contracts[].tag" {} 2>/dev/null | sort | uniq -d'
Length of output: 3177
Script:
#!/bin/bash
# Let's check the actual implementation to verify tag uniqueness enforcement
# First, find the Manifest struct definition and any validation logic
ast-grep --pattern 'struct Manifest {
$$$
}'
# Also search for any validation or insertion logic related to contracts
rg -A 5 'fn add_contract|impl Manifest'
Length of output: 575
Script:
#!/bin/bash
# Let's examine the Contract struct and any related methods
ast-grep --pattern 'struct Contract {
$$$
}'
# And check for any contract addition or validation logic
rg -B 2 -A 10 'pub fn add_contract|contracts: Vec<Contract>' --type rust
Length of output: 123
bin/sozo/src/utils.rs (1)
158-160: Ohayo sensei! The optimization looks good!
The conversion from world_diff to contracts before account retrieval aligns perfectly with the PR's objective of optimizing contract address resolution. This change helps avoid unnecessary diff computations when the sozo manifest is available.
Let's verify the optimization by checking for any potential performance bottlenecks in the contract handling:
✅ Verification successful
Let me gather more information about the account handling to ensure our optimization is properly implemented.
Ohayo sensei! The optimization is working perfectly across the codebase!
The conversion from world_diff to contracts is consistently implemented in all relevant places:
bin/sozo/src/utils.rs: Main implementationbin/sozo/src/commands/execute.rs: Used with manifest fallbackbin/sozo/src/commands/call.rs: Also implements manifest fallback
The pattern shows proper optimization by avoiding redundant diff computations when manifest is available, and the account handling is consistent across all implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances where we might still be computing diffs unnecessarily
# Look for patterns that might indicate redundant diff computations
rg -A 3 "WorldDiff::new_from_chain" --type rust
# Check for consistent contract info handling patterns
ast-grep --pattern 'let contracts = $expr.into();'
Length of output: 1174
Script:
#!/bin/bash
# Check the account implementation to verify the contract handling
ast-grep --pattern 'fn account($$$) {
$$$
}'
# Look for any other places where we might be converting world_diff to contracts
rg -B 2 -A 2 "world_diff.*into\(\)" --type rust
# Check for any other potential contract resolution patterns
rg -B 2 -A 2 "account\(.*provider.*contracts" --type rust
Length of output: 1678
bin/sozo/src/commands/options/account/mod.rs (3)
1-1: Ohayo! Clean import additions for contract handling.
The new imports support the transition to manifest-based contract resolution, aligning perfectly with the PR objectives.
Also applies to: 8-8
74-89: Well-documented method signature update, sensei!
The documentation clearly explains the contracts parameter and its relationship to the controller feature. The comment about keeping the feature internalized to account option provides valuable context for future maintainers.
62-71: Controller method signature change looks good, sensei!
The removal of world_diff parameter in favor of contracts aligns with the PR's goal of using manifest data instead of recomputing diffs.
Let's verify the controller usage across the codebase:
✅ Verification successful
Controller method signature change is correctly implemented across the codebase, sensei!
The verification shows that:
- All controller method calls in the codebase have been updated to use the new signature with
contractsparameter - The implementation in
bin/sozo/src/commands/options/account/controller.rscorrectly uses the new parameter structure - No legacy calls using the old signature with
world_addressandworld_diffparameters were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all controller method calls have been updated
# to use the new signature with contracts parameter
# Search for controller method calls
rg -A 3 'controller\(' --type rust
Length of output: 698
Script:
#!/bin/bash
# Let's check the controller module implementation to verify the signature change
ast-grep --pattern 'fn create_controller($$$)'
# Also check for any other potential controller instantiation patterns
rg -B 3 -A 3 'Controller' --type rust
Length of output: 20711
examples/spawn-and-move/manifest_dev.json (5)
3-4: Ohayo sensei! World configuration has been updated with new hashes.
The world section has been updated with new class_hash and address values, which aligns with the PR's objective of optimizing contract address resolution.
7-29: Added comprehensive list of world entrypoints.
The addition of entrypoints array provides a clear interface for interacting with the world contract, including essential operations like entity management, metadata handling, and contract lifecycle management. This enhancement improves the manifest's completeness and usability.
Line range hint 109-233: Enhanced type system with new span and bool implementations.
Added new struct implementations:
core::array::Span::<core::array::Span::<core::felt252>>core::boolas an enum with True/False variants
These type system enhancements provide better support for handling arrays and boolean values in the contract interface.
Line range hint 1246-1954: Updated contract configurations with systems arrays.
Contract updates include:
- Actions contract (lines 1246-1584)
- Dungeon contract (lines 1587-1778)
- Mock token contract (lines 1781-1954)
Each contract now includes:
- Updated addresses and class hashes
- Empty IContract interface implementation
- Systems array listing available functions
- Selector for contract identification
This structured approach aligns with the PR's goal of improving contract address resolution by providing clear identifiers and available operations.
2143-2189: Added selectors for models and events.
Models and events have been updated with:
- Unique class hashes
- Namespace-prefixed tags
- Selectors for identification
This enhancement supports the PR's objective by providing unique identifiers for each model and event, facilitating efficient lookups.
Let's verify the uniqueness of selectors:
✅ Verification successful
This will help us verify both selector and class_hash uniqueness with better context, showing the associated tags for each identifier.
Selectors and class hashes are unique across all models and events, sensei! ✨
The verification shows that:
- All selectors have unique values (no duplicates found)
- All class hashes are unique (no duplicates found)
- Each model and event has its proper namespace-prefixed tag (ns-*)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all selectors are unique across models and events
# Extract all selectors and check for duplicates
echo "Checking for duplicate selectors..."
rg -o '"selector": "0x[a-f0-9]+"' | sort | uniq -d
Length of output: 133
Script:
#!/bin/bash
# Let's try a different approach to extract and verify selectors
# First, let's get all selectors with their tags for better context
echo "=== All Selectors with Tags ==="
rg '"selector":.+"tag":' -A 1 manifest_dev.json
# Also check class_hash uniqueness
echo -e "\n=== All Class Hashes ==="
rg '"class_hash":.+"tag":' -A 1 manifest_dev.json
Length of output: 355
Script:
#!/bin/bash
# Let's try finding the correct path to the manifest file first
echo "=== Looking for manifest files ==="
fd manifest
# Then we'll use the correct path to verify selectors and class hashes
echo -e "\n=== Searching for selector and tag patterns ==="
rg -l '"selector":.+"tag":'
Length of output: 425
Script:
#!/bin/bash
# Now that we found the correct path, let's examine the manifest content
echo "=== Checking selectors in spawn-and-move manifest ==="
rg '"selector"' examples/spawn-and-move/manifest_dev.json
echo -e "\n=== Checking class hashes in spawn-and-move manifest ==="
rg '"class_hash"' examples/spawn-and-move/manifest_dev.json
# Let's also check if there are any duplicate values
echo -e "\n=== Checking for duplicate selectors ==="
rg -o '"selector": "([^"]+)"' examples/spawn-and-move/manifest_dev.json | sort | uniq -d
echo -e "\n=== Checking for duplicate class hashes ==="
rg -o '"class_hash": "([^"]+)"' examples/spawn-and-move/manifest_dev.json | sort | uniq -d
Length of output: 3973
bin/sozo/src/commands/options/account/controller.rs (2)
1-1: Ohayo, sensei! Importing HashMap is appropriate for managing contracts.
The addition of use std::collections::HashMap; is necessary for handling the contracts parameter effectively.
5-5: Ohayo, sensei! Importing ContractInfo integrates contract data properly.
Including use dojo_world::contracts::contract_info::ContractInfo; ensures that ContractInfo is available for use with the contracts parameter.
| impl From<&WorldDiff> for HashMap<String, ContractInfo> { | ||
| fn from(world_diff: &WorldDiff) -> Self { | ||
| trace!("Converting world diff to contracts info."); | ||
| let mut contracts = HashMap::new(); | ||
|
|
||
| contracts.insert( | ||
| "world".to_string(), | ||
| ContractInfo { | ||
| tag: "world".to_string(), | ||
| address: world_diff.world_info.address, | ||
| entrypoints: world_diff.world_info.entrypoints.clone(), | ||
| }, | ||
| ); | ||
|
|
||
| for (selector, resource) in &world_diff.resources { | ||
| let tag = resource.tag(); | ||
|
|
||
| match resource { | ||
| ResourceDiff::Created(ResourceLocal::Contract(c)) => { | ||
| // The resource must exist, so the unwrap is safe here. | ||
| let address = world_diff.get_contract_address(*selector).unwrap(); | ||
| contracts.insert( | ||
| tag.clone(), | ||
| ContractInfo { tag: tag.clone(), address, entrypoints: c.systems.clone() }, | ||
| ); | ||
| } | ||
| ResourceDiff::Updated(ResourceLocal::Contract(l), ResourceRemote::Contract(r)) => { | ||
| contracts.insert( | ||
| tag.clone(), | ||
| ContractInfo { | ||
| tag: tag.clone(), | ||
| address: r.common.address, | ||
| entrypoints: l.systems.clone(), | ||
| }, | ||
| ); | ||
| } | ||
| ResourceDiff::Synced(ResourceLocal::Contract(l), ResourceRemote::Contract(r)) => { | ||
| contracts.insert( | ||
| tag.clone(), | ||
| ContractInfo { | ||
| tag: tag.clone(), | ||
| address: r.common.address, | ||
| entrypoints: l.systems.clone(), | ||
| }, | ||
| ); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| contracts | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace unwrap() with expect() for better error context.
While the comment indicates that the unwrap is safe, it's better to use expect() to provide context in case of a panic. This helps with debugging if the assumption ever becomes invalid.
- let address = world_diff.get_contract_address(*selector).unwrap();
+ let address = world_diff.get_contract_address(*selector)
+ .expect("Contract address must exist for Created resource");Also, consider adding a debug assertion to validate the assumption:
debug_assert!(
world_diff.get_contract_address(*selector).is_some(),
"Contract address must exist for Created resource"
);| let contract_address = contract_address.ok_or_else(|| { | ||
| let mut message = format!("Contract {descriptor} not found in the manifest."); | ||
| if self.diff { | ||
| message.push_str( | ||
| " Run the command again with `--diff` to force the fetch of data from the \ | ||
| chain.", | ||
| ); | ||
| } | ||
| anyhow!(message) | ||
| })?; |
There was a problem hiding this comment.
Fix inconsistent error message condition.
The error message suggests using --diff even when it's already enabled, which could be confusing to users.
let contract_address = contract_address.ok_or_else(|| {
let mut message = format!("Contract {descriptor} not found in the manifest.");
- if self.diff {
+ if !self.diff {
message.push_str(
" Run the command again with `--diff` to force the fetch of data from the \
chain.",
);
}
anyhow!(message)
})?;📝 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.
| let contract_address = contract_address.ok_or_else(|| { | |
| let mut message = format!("Contract {descriptor} not found in the manifest."); | |
| if self.diff { | |
| message.push_str( | |
| " Run the command again with `--diff` to force the fetch of data from the \ | |
| chain.", | |
| ); | |
| } | |
| anyhow!(message) | |
| })?; | |
| let contract_address = contract_address.ok_or_else(|| { | |
| let mut message = format!("Contract {descriptor} not found in the manifest."); | |
| if !self.diff { | |
| message.push_str( | |
| " Run the command again with `--diff` to force the fetch of data from the \ | |
| chain.", | |
| ); | |
| } | |
| anyhow!(message) | |
| })?; |
| #[tracing::instrument(name = "create_controller", skip(rpc_url, provider, contracts))] | ||
| pub async fn create_controller<P>( | ||
| // Ideally we can get the url from the provider so we dont have to pass an extra url param here | ||
| rpc_url: Url, | ||
| provider: P, | ||
| world_address: Felt, | ||
| world_diff: &WorldDiff, | ||
| contracts: &HashMap<String, ContractInfo>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty contracts to enhance robustness.
Sensei, please consider adding a check for an empty contracts HashMap in the create_controller function. This will prevent potential runtime errors and improve the function's resilience.
Suggested addition:
if contracts.is_empty() {
bail!("Contracts map is empty. Cannot collect policies without contracts.");
}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2649 +/- ##
==========================================
+ Coverage 57.34% 57.40% +0.06%
==========================================
Files 399 400 +1
Lines 49747 49903 +156
==========================================
+ Hits 28527 28647 +120
- Misses 21220 21256 +36 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/dojo/world/src/contracts/contract_info.rs (3)
26-35: Consider deriving additional traits for better ergonomics.The struct could benefit from deriving
CloneandPartialEqtraits.Clonewould be useful when you need to duplicate contract info, andPartialEqis already being used implicitly in the tests.-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq)] pub struct ContractInfo {
92-111: Refactor duplicated code in Updated and Synced branches.The
UpdatedandSyncedbranches contain identical code for creatingContractInfo. Consider extracting this into a helper function to reduce duplication.+ impl ContractInfo { + fn from_resource(tag: String, address: Felt, entrypoints: Vec<String>) -> Self { + Self { tag, address, entrypoints } + } + } match resource { ResourceDiff::Updated(ResourceLocal::Contract(l), ResourceRemote::Contract(r)) => { - contracts.insert( - tag.clone(), - ContractInfo { - tag: tag.clone(), - address: r.common.address, - entrypoints: l.systems.clone(), - }, - ); + contracts.insert( + tag.clone(), + ContractInfo::from_resource(tag.clone(), r.common.address, l.systems.clone()), + ); } ResourceDiff::Synced(ResourceLocal::Contract(l), ResourceRemote::Contract(r)) => { - contracts.insert( - tag.clone(), - ContractInfo { - tag: tag.clone(), - address: r.common.address, - entrypoints: l.systems.clone(), - }, - ); + contracts.insert( + tag.clone(), + ContractInfo::from_resource(tag.clone(), r.common.address, l.systems.clone()), + ); }
120-218: Consider adding edge case tests.The test coverage is good, but consider adding tests for edge cases:
- Empty contracts list in Manifest
- Empty entrypoints list
- Empty systems list in contracts
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/world/src/contracts/contract_info.rs(1 hunks)
🔇 Additional comments (2)
crates/dojo/world/src/contracts/contract_info.rs (2)
1-25: Ohayo sensei! Excellent documentation!
The documentation clearly explains the purpose of ContractInfo, the tradeoffs between using manifest vs world diff, and the rationale behind the implementation choices. This level of detail will greatly help future maintainers understand the code's intent.
37-64: LGTM! Clean and efficient implementation.
The From<&Manifest> implementation is well-structured, with good use of tracing and efficient handling of both world and contract entries.
On some operations like
executeorcall, we have to resolve tags to have contract addresses.Before, we recomputing the diff from the chain all the time. Now, by default we try to use the manifest.
If the manifest is not present, we fallback on the diff from the chain.
The user can still provide
--diffto force the diff to happen.Summary by CodeRabbit
Release Notes
New Features
diffparameter inCallArgsandExecuteArgsfor enhanced contract address resolution.ContractInfostruct to manage contract details effectively.get_contract_addressmethod for easier contract address retrieval by tag.read_manifest_profilemethod to facilitate reading manifest profiles.Improvements
EventEmitted.Bug Fixes