feat: add support for #[dojo::library] - #2938
Conversation
|
Caution Review failedThe pull request is closed. Library Resource Management in DojoOhayo, sensei! 🌟 Let me break down this epic pull request that introduces library resource management to the Dojo framework. WalkthroughThis pull request comprehensively adds library resource support across the Dojo ecosystem. It introduces a new resource type called Changes
Possibly Related PRs
Suggested Reviewers
Sensei, this PR is a game-changer for library management in Dojo! 🚀🔧 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (12)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
🧹 Nitpick comments (9)
crates/dojo/lang/src/attribute_macros/library.rs (2)
41-56: Simplify loop over module namesOhayo sensei,
The loop at lines 41-56 iterates over a single element
[("name", &name.to_string())]. It's more straightforward to remove the loop and directly check the module name. This enhances readability and reduces unnecessary complexity.Apply this diff to simplify the code:
-for (id, value) in [("name", &name.to_string())] { +let id = "name"; +let value = &name.to_string(); if !naming::is_name_valid(value) { return PluginResult { code: None, diagnostics: vec![PluginDiagnostic { stable_ptr: module_ast.stable_ptr().0, message: format!( "The contract {id} '{value}' can only contain characters (a-z/A-Z), \ digits (0-9) and underscore (_)." ), severity: Severity::Error, }], remove_original_item: false, }; } -}
157-158: Remove debug code before deploymentOhayo sensei,
The call to
crate::debug_expandappears to be for debugging purposes. To prevent unnecessary output in production, consider removing or commenting out this line.Apply this diff to remove the debug statement:
- crate::debug_expand(&format!("LIBRARY PATCH: {name}"), &code);crates/dojo/world/src/local/resource.rs (1)
42-49: Consider adding documentation sensei!While the structure is good, it would be helpful to add documentation explaining:
- Purpose of the LibraryLocal struct
- How systems are used in the library context
- Relationship with ContractLocal
crates/dojo/world/src/diff/manifest.rs (1)
66-81: Ohayo sensei! Let's add some documentation to the DojoLibrary struct.The struct looks well-designed, mirroring DojoContract where appropriate, but it would benefit from documentation explaining its purpose and fields.
Consider adding documentation:
#[serde_as] #[derive(Clone, Default, Debug, Serialize, Deserialize)] +/// Represents a library resource in the Dojo framework. pub struct DojoLibrary { + /// The class hash of the library contract. #[serde_as(as = "UfeHex")] pub class_hash: Felt, + /// The ABI of the library contract. pub abi: Vec<AbiEntry>, + /// The unique tag identifying the library. pub tag: String, + /// The unique selector for the library. #[serde_as(as = "UfeHex")] pub selector: Felt, + /// The systems implemented by the library. pub systems: Vec<String>, }crates/dojo/core/src/world/iworld.cairo (1)
134-134: Ohayo! Small documentation fix needed sensei.The documentation refers to "contract" instead of "library" in the upgrade_library method's parameters.
- /// * `namespace` - The namespace of the contract to be upgraded. + /// * `namespace` - The namespace of the library to be upgraded.crates/dojo/world/src/remote/events_to_remote.rs (1)
245-269: Solid implementation of the LibraryRegistered handler!The implementation follows the same pattern as other resource registrations, with proper namespace whitelisting and error handling.
However, consider adding a debug log when successfully registering a library, similar to other handlers:
trace!(?r, "Library registered."); +debug!( + namespace, + library = e.name.to_string()?, + "Library registered successfully." +);crates/sozo/ops/src/migrate/mod.rs (1)
631-694: Consider adding documentation for the libraries_calls_classes function, sensei!While the implementation is solid, adding documentation would help explain:
- The purpose of the function
- The expected format of the ResourceDiff parameter
- The return value structure
+/// Gathers the calls required to sync the libraries and their classes to be declared. +/// +/// # Arguments +/// * `resource` - The resource diff containing library information +/// +/// # Returns +/// A tuple containing: +/// * Vector of calls for registering/upgrading libraries +/// * HashMap of class hashes mapped to their labeled classes async fn libraries_calls_classes( &self, resource: &ResourceDiff, ) -> Result<(Vec<Call>, HashMap<Felt, LabeledClass>), MigrationError<A::SignError>> {crates/dojo/core/src/world/storage.cairo (2)
51-53: Ohayo! Consider documenting the library case behavior, sensei!The implementation looks correct, but it would be helpful to document why libraries use a zero contract address. This special case might not be immediately obvious to other developers.
Resource::Library(( class_hash, _, - )) => { Option::Some((0.try_into().unwrap(), class_hash)) }, + )) => { + // Libraries are identified by class_hash only, using zero address as they're not deployed + Option::Some((0.try_into().unwrap(), class_hash)) + },
51-53: Ohayo! We should add tests for library resource handling, sensei!The new library case in the
dnsfunction needs test coverage to ensure correct behavior, especially around the zero address case.Would you like me to help create test cases for:
- Library resource resolution
- Zero address handling
- Invalid library cases
🛑 Comments failed to post (8)
crates/dojo/lang/src/attribute_macros/library.rs (1)
64-91: 🛠️ Refactor suggestion
Refactor closure to a for loop for clarity
Ohayo sensei,
Modifying external variables (
has_event,has_storage, etc.) within a closure can lead to unintended side effects and confusion. Refactoring the closure into aforloop improves readability and ensures proper variable management.Apply this diff to refactor the code:
-if let MaybeModuleBody::Some(body) = module_ast.body(db) { - let mut body_nodes: Vec<_> = body - .iter_items_in_cfg(db, metadata.cfg_set) - .flat_map(|el| { - if let ast::ModuleItem::Enum(ref enum_ast) = el { - if enum_ast.name(db).text(db).to_string() == "Event" { - has_event = true; - } - } else if let ast::ModuleItem::Struct(ref struct_ast) = el { - if struct_ast.name(db).text(db).to_string() == "Storage" { - has_storage = true; - } - } else if let ast::ModuleItem::FreeFunction(ref fn_ast) = el { - let fn_decl = fn_ast.declaration(db); - let fn_name = fn_decl.name(db).text(db); - - if fn_name == CONSTRUCTOR_FN { - has_constructor = true; - } - - if fn_name == DOJO_INIT_FN { - has_init = true; - } - } - - vec![RewriteNode::Copied(el.as_syntax_node())] - }) - .collect(); +if let MaybeModuleBody::Some(body) = module_ast.body(db) { + let mut body_nodes: Vec<_> = Vec::new(); + for el in body.iter_items_in_cfg(db, metadata.cfg_set) { + if let ast::ModuleItem::Enum(ref enum_ast) = el { + if enum_ast.name(db).text(db).to_string() == "Event" { + has_event = true; + } + } else if let ast::ModuleItem::Struct(ref struct_ast) = el { + if struct_ast.name(db).text(db).to_string() == "Storage" { + has_storage = true; + } + } else if let ast::ModuleItem::FreeFunction(ref fn_ast) = el { + let fn_decl = fn_ast.declaration(db); + let fn_name = fn_decl.name(db).text(db); + + if fn_name == CONSTRUCTOR_FN { + has_constructor = true; + } + + if fn_name == DOJO_INIT_FN { + has_init = true; + } + } + + body_nodes.push(RewriteNode::Copied(el.as_syntax_node())); + }📝 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 mut body_nodes: Vec<_> = Vec::new(); for el in body.iter_items_in_cfg(db, metadata.cfg_set) { if let ast::ModuleItem::Enum(ref enum_ast) = el { if enum_ast.name(db).text(db).to_string() == "Event" { has_event = true; } } else if let ast::ModuleItem::Struct(ref struct_ast) = el { if struct_ast.name(db).text(db).to_string() == "Storage" { has_storage = true; } } else if let ast::ModuleItem::FreeFunction(ref fn_ast) = el { let fn_decl = fn_ast.declaration(db); let fn_name = fn_decl.name(db).text(db); if fn_name == CONSTRUCTOR_FN { has_constructor = true; } if fn_name == DOJO_INIT_FN { has_init = true; } } body_nodes.push(RewriteNode::Copied(el.as_syntax_node())); }crates/dojo/core/src/world/world_contract.cairo (1)
810-846: 💡 Codebase verification
Resource::Library handling needs completion in permission checks
Ohayo sensei! Found inconsistencies in Resource::Library handling:
assert_caller_permissions: Missing explicit Library case in namespace_hash match at line ~730panic_with_details: Missing Library case in resource_name match at line ~780These should explicitly handle Library resources instead of falling through to catch-all cases.
🔗 Analysis chain
Verify consistent resource handling for libraries
Ohayo sensei,
The
register_libraryfunction introduces library registration. Please ensure that all resource handling logic throughout the contract, especially in permission checks and resource management functions, consistently includes theResource::Libraryvariant.Run the following script to search for all match statements that should handle
Resource::Library:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for match statements over Resource to verify inclusion of Resource::Library # Find all matches over Resource enum rg 'match\s+\w+\.resources\.read\([^)]*\)\s*\{' -A 10 | grep -B 10 -A 10 'Resource::' # Check for missing cases of Resource::Library rg 'match\s+\w+\.resources\.read\([^)]*\)\s*\{' -A 10 | grep -A 10 -E 'Resource::(Contract|Model|Event|Namespace|World|Unregistered)' | grep -B 10 -A 10 -v 'Resource::Library'Length of output: 17348
crates/dojo/world/src/diff/compare.rs (1)
29-39: 💡 Codebase verification
Ohayo! Found issues with panic handling, sensei!
The
as_library_or_panic()implementation has several problems:
- Error message incorrectly says "expected to be a contract" instead of "library"
- No type validation before calling the panic-prone function
- Unsafe assumption in
compare()that remote is always a libraryConsider:
- Adding type validation before calling
as_library_or_panic()- Fixing the error message
- Using
matches!()or pattern matching for safer type checking🔗 Analysis chain
Verify panic conditions sensei!
While the implementation follows the pattern of ContractLocal, we should verify the panic conditions in
as_library_or_panic(). Consider adding tests to ensure proper error handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering as_library_or_panic rg -l "as_library_or_panic.*test"Length of output: 35
Script:
#!/bin/bash # Find implementation of as_library_or_panic ast-grep --pattern 'fn as_library_or_panic' # Also search with ripgrep for any related code rg 'as_library_or_panic' -A 3Length of output: 824
Script:
#!/bin/bash # Search for LibraryLocal usage and type checks rg "LibraryLocal" -B 2 -A 2 # Search for ResourceRemote construction/matching ast-grep --pattern 'match $_ { ResourceRemote::$_ => $$$, $$$}'Length of output: 1926
crates/dojo/world/src/contracts/contract_info.rs (1)
127-127:
⚠️ Potential issueOhayo! Libraries are not included in the contract info mapping.
While the test data includes a library entry, the
From<&Manifest>implementation forHashMap<String, ContractInfo>doesn't handle libraries. This means libraries won't be accessible through the contract info mapping.Consider adding library handling in the implementation:
impl From<&Manifest> for HashMap<String, ContractInfo> { fn from(manifest: &Manifest) -> Self { trace!("Converting manifest to contracts info."); let mut contracts = HashMap::new(); contracts.insert( "world".to_string(), ContractInfo { tag: "world".to_string(), address: manifest.world.address, entrypoints: manifest.world.entrypoints.clone(), }, ); for c in &manifest.contracts { contracts.insert( c.tag.clone(), ContractInfo { tag: c.tag.clone(), address: c.address, entrypoints: c.systems.clone(), }, ); } + + for l in &manifest.libraries { + contracts.insert( + l.tag.clone(), + ContractInfo { + tag: l.tag.clone(), + address: Felt::ZERO, // Libraries don't have addresses + entrypoints: l.systems.clone(), + }, + ); + } contracts } }Also applies to: 150-156
crates/dojo/world/src/remote/resource.rs (2)
298-304:
⚠️ Potential issueIncorrect error message in as_library_or_panic sensei!
The error message incorrectly states that the resource is expected to be a contract.
ResourceRemote::Library(library) => library, - _ => panic!("Resource is expected to be a contract: {:?}.", self), + _ => panic!("Resource is expected to be a library: {:?}.", self),📝 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./// Get the library remote if the resource is a library, otherwise panic. pub fn as_library_or_panic(&self) -> &LibraryRemote { match self { ResourceRemote::Library(library) => library, _ => panic!("Resource is expected to be a library: {:?}.", self), } }
177-177:
⚠️ Potential issueCritical: Incorrect namespace implementation for libraries sensei!
The namespace method is returning the name instead of the namespace for libraries.
- ResourceRemote::Library(l) => l.common.name.clone(), + ResourceRemote::Library(l) => l.common.namespace.clone(),📝 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.ResourceRemote::Library(l) => l.common.namespace.clone(),crates/dojo/world/src/remote/permissions.rs (1)
100-102:
⚠️ Potential issueCritical: Library permissions need proper implementation, sensei!
The current implementation uses
unreachable!()for library permissions, which is not production-ready. Libraries should have proper permission handling like other resources.Consider implementing proper permission handling:
- ResourceRemote::Library(_) => { - // ? - unreachable!() - } + ResourceRemote::Library(library) => { + library.common.update_writer(contract_address, is_writer) + }Also applies to: 115-118
Cargo.toml (1)
215-220:
⚠️ Potential issueFix scarb dependency configuration to resolve build failures, sensei!
The current configuration using local paths is causing build failures. Either:
- Restore the git dependencies, or
- Ensure the local scarb repository is properly set up at the specified paths
-scarb = { path = "../scarb/scarb" } -scarb-metadata = { path = "../scarb/scarb-metadata" } -scarb-ui = { path = "../scarb/utils/scarb-ui" } +scarb = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } +scarb-metadata = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } +scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" }📝 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.scarb = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } scarb-metadata = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } # scarb = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } # scarb-metadata = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" } # scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "f67331d99bca24c13ad56af6752b93f23e5ac373" }🧰 Tools
🪛 GitHub Actions: ci
[error] Failed to load manifest for workspace member. Missing dependency manifest for 'dojo-utils', which ultimately fails due to missing scarb Cargo.toml at '/__w/dojo/scarb/scarb/Cargo.toml'
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/dojo/world/src/contracts/contract_info.rs (1)
Line range hint
127-156:Manifestconversion does not includelibraries, and tests do not verifylibrariesfield.Ohayo, sensei! In the
test_manifest_to_contracts_infotest, thelibrariesfield in theManifestis populated, but the conversion fromManifesttoHashMap<String, ContractInfo>does not include libraries. As a result, the test does not verify that libraries are correctly included incontracts_info. Please update the conversion logic and the test to include libraries.Apply this diff to include libraries in the conversion and update the test:
@@ -33,6 +33,15 @@ impl From<&Manifest> for HashMap<String, ContractInfo> { }, ); } + for lib in &manifest.libraries { + contracts.insert( + lib.tag.clone(), + ContractInfo { + tag: lib.tag.clone(), + address: Felt::ZERO, // Adjust if libraries have addresses + entrypoints: lib.systems.clone(), + }, + ); + } + contracts }Update the test to verify that the library is included:
@@ -155,6 +155,9 @@ fn test_manifest_to_contracts_info() { assert_eq!(contracts_info["ns-test_contract"].entrypoints, vec!["system_1".to_string()]); assert_eq!(contracts_info["ns-test_contract"].tag, "ns-test_contract".to_string()); + assert_eq!(contracts_info["ns-test_library"].address, Felt::ZERO); // Or the correct address + assert_eq!(contracts_info["ns-test_library"].entrypoints, vec!["system_1".to_string()]); + assert_eq!(contracts_info["ns-test_library"].tag, "ns-test_library".to_string()); }
🧹 Nitpick comments (3)
crates/sozo/ops/src/migrate/mod.rs (1)
631-694: Ohayo! Consider reducing code duplication in library migration, sensei!While the implementation is functionally correct, there's significant code duplication between
libraries_calls_classesandcontracts_calls_classes. Consider extracting common logic into a shared helper function.Here's a suggested approach:
- Create a helper function for common logic:
async fn resource_calls_classes<T>( &self, resource: &ResourceDiff, register_fn: impl Fn(&ByteArray, &ClassHash) -> Call, upgrade_fn: impl Fn(&ByteArray, &ClassHash) -> Call, ) -> Result<(Vec<Call>, HashMap<Felt, LabeledClass>), MigrationError<A::SignError>>
- Use the helper in both methods:
async fn libraries_calls_classes( &self, resource: &ResourceDiff, ) -> Result<(Vec<Call>, HashMap<Felt, LabeledClass>), MigrationError<A::SignError>> { self.resource_calls_classes( resource, |ns, ch| self.world.register_library_getcall(ns, ch), |ns, ch| self.world.upgrade_library_getcall(ns, ch), ).await }crates/dojo/core/src/world/world_contract.cairo (2)
810-846: Ohayo! The library registration implementation is well-structured, sensei! 📚The function follows best practices with proper permission checks and validation. However, consider adding documentation comments to explain the function's purpose and parameters.
Add documentation similar to other registration functions:
+/// Registers a library in the world. +/// +/// # Arguments +/// +/// * `namespace` - The namespace to register the library under. +/// * `class_hash` - The class hash of the library contract. +/// +/// # Returns +/// +/// The class hash of the registered library. fn register_library( ref self: ContractState, namespace: ByteArray, class_hash: ClassHash, ) -> ClassHash {
849-886: Consider adding compatibility checks during library upgrades, sensei! 🔍While the upgrade implementation is solid, it might be beneficial to add compatibility checks between the old and new library versions, similar to how
assert_resource_upgradabilityworks for other resources.This would help prevent breaking changes during upgrades. Consider:
- Checking interface compatibility
- Validating function signatures
- Ensuring backward compatibility
Would you like me to propose a detailed implementation for these checks?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
bin/sozo/src/commands/inspect.rs(6 hunks)crates/dojo/core/src/contract/interface.cairo(1 hunks)crates/dojo/core/src/lib.cairo(2 hunks)crates/dojo/core/src/world/iworld.cairo(2 hunks)crates/dojo/core/src/world/resource.cairo(3 hunks)crates/dojo/core/src/world/world_contract.cairo(6 hunks)crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo(1 hunks)crates/dojo/lang/src/cairo_plugin.rs(3 hunks)crates/dojo/world/src/contracts/abigen/world.rs(13 hunks)crates/dojo/world/src/contracts/contract_info.rs(2 hunks)crates/dojo/world/src/diff/compare.rs(3 hunks)crates/dojo/world/src/diff/manifest.rs(6 hunks)crates/dojo/world/src/lib.rs(1 hunks)crates/dojo/world/src/local/artifact_to_local.rs(4 hunks)crates/dojo/world/src/local/resource.rs(8 hunks)crates/dojo/world/src/remote/events_to_remote.rs(4 hunks)crates/dojo/world/src/remote/permissions.rs(2 hunks)crates/dojo/world/src/remote/resource.rs(15 hunks)crates/sozo/ops/src/migrate/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/dojo/world/src/lib.rs
- crates/dojo/core/src/contract/interface.cairo
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/core/src/world/resource.cairo
- crates/dojo/world/src/diff/compare.rs
- crates/dojo/core/src/lib.cairo
- crates/dojo/world/src/remote/events_to_remote.rs
- crates/dojo/world/src/remote/permissions.rs
- crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo
- crates/dojo/world/src/diff/manifest.rs
🔇 Additional comments (22)
crates/dojo/world/src/contracts/abigen/world.rs (7)
393-478: Addition ofLibraryRegisteredandLibraryUpgradedstructs is correct.Ohayo, sensei! The implementation of
LibraryRegisteredandLibraryUpgradedstructs, along with theirCairoSerdetrait implementations, looks good.
Line range hint
1078-1315: Inclusion of newEventvariants for libraries is properly handled.Ohayo, sensei! The addition of
LibraryRegisteredandLibraryUpgradedvariants to theEventenum, and updating the serialization and deserialization logic accordingly, is correctly implemented.
1820-1896: Deserialization logic for library events is implemented correctly.Ohayo, sensei! The
TryFrom<&starknet::core::types::EmittedEvent>implementation now correctly handlesLibraryRegisteredandLibraryUpgradedevents. The deserialization seems accurate.
2777-2853: Event handling for library events inTryFrom<&Event>is appropriate.Ohayo, sensei! The implementation for handling
LibraryRegisteredandLibraryUpgradedin theTryFrom<&Event>trait looks correct.
Line range hint
3446-3535:Resourceenum updated withLibraryvariant correctly.Ohayo, sensei! The addition of the
Libraryvariant to theResourceenum, along with the necessary serialization and deserialization logic, is properly implemented.
Line range hint
3985-4023: Addition ofregister_librarymethods is appropriate.Ohayo, sensei! The
register_library_getcallandregister_librarymethods are correctly added, following the existing patterns for similar methods.
4374-4407:upgrade_librarymethods are implemented correctly.Ohayo, sensei! The
upgrade_library_getcallandupgrade_librarymethods have been added appropriately, and they mirror the structure of other upgrade methods.crates/dojo/world/src/local/resource.rs (8)
14-14:ResourceLocalenum now includesLibraryvariant.Ohayo, sensei! The addition of the
Library(LibraryLocal)variant to theResourceLocalenum looks good.
42-49: Definition ofLibraryLocalstruct is consistent.Ohayo, sensei! The
LibraryLocalstruct is defined appropriately, matching the structure of other resource structs.
91-91:namemethod handlesLibraryvariant correctly.Ohayo, sensei! The
namemethod now correctly retrieves the name forLibraryresources.
102-102:namespacemethod updated forLibraryresources.Ohayo, sensei! The
namespacemethod now includes support forLibraryresources.
112-112:class_hashmethod includesLibraryvariant.Ohayo, sensei! The
class_hashmethod now correctly returns the class hash forLibraryresources.
123-123:abimethod updated to supportLibraryresources.Ohayo, sensei! The
abimethod now retrieves the ABI forLibraryresources appropriately.
162-162:resource_typemethod recognizesLibraryvariant.Ohayo, sensei! The
resource_typemethod now correctly identifiesLibraryresources asResourceType::Library.
173-173:commonmethod handlesLibraryresources properly.Ohayo, sensei! The
commonmethod now returns the common information forLibraryresources as expected.crates/dojo/core/src/world/iworld.cairo (2)
86-93: Ohayo! Theregister_librarymethod looks well-structured!The method signature and documentation align perfectly with other registration methods in the trait.
129-136: Theupgrade_librarymethod follows the established pattern, sensei!The implementation maintains consistency with other upgrade methods in the trait.
crates/dojo/world/src/remote/resource.rs (1)
16-16: Ohayo! TheLibraryRemotestruct looks clean and follows the pattern!The struct definition aligns with other remote resource types, maintaining consistency.
Also applies to: 50-54
crates/dojo/world/src/local/artifact_to_local.rs (1)
21-21: Ohayo! The library artifact handling looks solid, sensei!The implementation follows the established pattern for resource handling, with appropriate logging and error handling.
Also applies to: 111-138
bin/sozo/src/commands/inspect.rs (1)
119-129: Ohayo! TheLibraryInspectstruct looks well-designed!The struct follows the established pattern for resource inspection, with appropriate fields and display attributes.
crates/dojo/core/src/world/world_contract.cairo (2)
71-72: Ohayo! The event definitions look solid, sensei! 🎉The
LibraryRegisteredandLibraryUpgradedevents follow the established pattern and contain all necessary fields for tracking library changes.Also applies to: 112-126
1239-1244: Ohayo! The Library variant handling is well-integrated, sensei! ✨The error handling for libraries follows the established pattern and provides clear error messages.
| ResourceRemote::Model(m) => m.common.namespace.clone(), | ||
| ResourceRemote::Event(e) => e.common.namespace.clone(), | ||
| ResourceRemote::Namespace(ns) => ns.name.clone(), | ||
| ResourceRemote::Library(l) => l.common.name.clone(), |
There was a problem hiding this comment.
Fix namespace implementation for libraries, sensei!
The namespace method incorrectly returns common.name instead of common.namespace for libraries.
Apply this fix:
- ResourceRemote::Library(l) => l.common.name.clone(),
+ ResourceRemote::Library(l) => l.common.namespace.clone(),📝 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.
| ResourceRemote::Library(l) => l.common.name.clone(), | |
| ResourceRemote::Library(l) => l.common.namespace.clone(), |
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
Thank you @notV4l for taking the time on that. 🙏
Basically using a dojo_name on library will be complicated. We could remove this for now, and only relying on the registration process by the user and expect the user to provide both the namespace and the hash.
| World, | ||
| #[default] | ||
| Unregistered, | ||
| Library: (ClassHash, felt252), |
There was a problem hiding this comment.
Could you please add in the comment section above the meaning of the felt?
| }, | ||
| Resource::Library(( | ||
| class_hash, _, | ||
| )) => { Option::Some((0.try_into().unwrap(), class_hash)) }, |
There was a problem hiding this comment.
Let's use a constant for address 0.
|
|
||
| let namespace_hash = bytearray_hash(@namespace); | ||
|
|
||
| let contract = IDeployedResourceLibraryDispatcher { class_hash }; |
There was a problem hiding this comment.
The problem here is that we're doing a library call, which can replace the class hash of the world.
I think this is what you meant with the issue about permissions.
Library calls are forbidden in the world as they are not safe.
For a library, we should just accept the class hash, since it can only be registered by someone that has access to the namespace, we have a trust assumption here.
In this context, the contract name may be arbitrary and not directly bound to the library (since we can't do a library call), hence passed as argument. Then the selector is computed from the two inputs (namespace + name).
| let contract = IDeployedResourceLibraryDispatcher { class_hash }; | ||
| let contract_name = contract.dojo_name(); |
There was a problem hiding this comment.
Same here, doing this is unsafe. Refer to the comment above for register.
| let d = IDeployedResourceLibraryDispatcher { class_hash }; | ||
| format!("library (or its namespace) `{}`", d.dojo_name()) |
There was a problem hiding this comment.
Here it's ok since we panic only.
| // TODO: rename impl ?? | ||
| #[abi(embed_v0)] | ||
| pub impl $name$__DeployedContractImpl of IDeployedResource<ContractState> { | ||
| fn dojo_name(self: @ContractState) -> ByteArray { | ||
| "$name$" | ||
| } | ||
| } |
There was a problem hiding this comment.
We may not be able to use this at all due to the library calls being unsafe. Can be removed, or kept for debugging purposes.
| #[serde_as(as = "UfeHex")] | ||
| pub class_hash: Felt, | ||
| /// ABI of the contract. | ||
| pub abi: Vec<AbiEntry>, |
There was a problem hiding this comment.
For better readability, I was wondering if it would be better to have the ABIs in a separate object in the manifest. Like so, we could humanly ready the whole manifest ignoring the abis key, wdyt? This will be done in other PR, but wondering what's your take on that.
| namespace.update_writer(contract_address, is_writer) | ||
| } | ||
| ResourceRemote::Library(_) => { | ||
| // ? |
There was a problem hiding this comment.
The behavior is the same as contract. The contract address here is the address of the writer, not the address of the library. This will then register true/false for the library into the world's storage.
| namespace.update_owner(contract_address, is_owner) | ||
| } | ||
| ResourceRemote::Library(_) => { | ||
| // ? |
There was a problem hiding this comment.
Same behavior, like mentioned above. 👍
| pub fn as_library_or_panic(&self) -> &LibraryRemote { | ||
| match self { | ||
| ResourceRemote::Library(library) => library, | ||
| _ => panic!("Resource is expected to be a contract: {:?}.", self), |
There was a problem hiding this comment.
| _ => panic!("Resource is expected to be a contract: {:?}.", self), | |
| _ => panic!("Resource is expected to be a library: {:?}.", self), |
glihm
changed the title
#[dojo::library]#[dojo::library]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/dojo/lang/src/cairo_plugin.rs (1)
102-102: Consider enhancing module documentation, sensei! 📚While the library attribute is correctly declared, the module-level documentation (around line 36) should be updated to include libraries in the list of supported Dojo features.
/// Currently Dojo mainly supports: /// - Contracts: which are built from attribute macros on a module. /// - Interfaces: which are built from attribute macros on a trait. /// - Models: which are built from attribute macros on a struct. /// - Events: which are built from attribute macros on a struct. + /// - Libraries: which are built from attribute macros on a module. /// - Enums: mostly used for deriving introspect to be used into a model or event.crates/sozo/ops/src/migrate/mod.rs (2)
433-443: Inconsistent variable naming in library resource handlingOhayo sensei, I noticed that within the library resource handling code, variables are named
contract_callsandcontract_classes, which may cause confusion. Consider renaming them tolibrary_callsandlibrary_classesfor clarity and consistency.Apply this diff to rename the variables:
- let (contract_calls, contract_classes) = + let (library_calls, library_classes) = self.libraries_calls_classes(resource).await?; - if !contract_calls.is_empty() { + if !library_calls.is_empty() { - invoker.extend_calls(contract_calls); - classes.extend(contract_classes); + invoker.extend_calls(library_calls); + classes.extend(library_classes);
631-694: Consider refactoring to reduce code duplicationOhayo sensei, the
libraries_calls_classesfunction shares similar logic withcontracts_calls_classes,models_calls_classes, andevents_calls_classes. To adhere to the DRY principle, consider abstracting the common logic into a generic function.crates/dojo/world/src/diff/manifest.rs (1)
66-81: Ohayo sensei! TheDojoLibrarystruct looks good but could be documented better.The struct follows the same pattern as
DojoContract, which is great for consistency. However, it would benefit from documentation comments for each field.Add documentation like this:
#[serde_as] #[derive(Clone, Default, Debug, Serialize, Deserialize)] pub struct DojoLibrary { + /// Class hash of the library contract. #[serde_as(as = "UfeHex")] pub class_hash: Felt, + /// ABI of the library contract. pub abi: Vec<AbiEntry>, + /// Tag used to identify the library. pub tag: String, + /// Selector computed from the library's namespace and name. #[serde_as(as = "UfeHex")] pub selector: Felt, + /// List of system functions exposed by the library. pub systems: Vec<String>, }bin/sozo/src/commands/inspect.rs (1)
396-411: Optimize the library status handling, sensei!The tuple destructuring of
_current_class_hashis unnecessary since it's not used in this block.Simplify the code:
- let (_current_class_hash, status) = match resource { + let status = match resource { - ResourceDiff::Created(_) => { - (resource.current_class_hash(), ResourceStatus::Created) - } + ResourceDiff::Created(_) => ResourceStatus::Created, - ResourceDiff::Updated(_, _remote) => { - (resource.current_class_hash(), ResourceStatus::Updated) - } + ResourceDiff::Updated(_, _remote) => ResourceStatus::Updated, ResourceDiff::Synced(_, remote) => ( - remote.current_class_hash(), - if has_dirty_perms { + if has_dirty_perms { ResourceStatus::DirtyLocalPerms } else { ResourceStatus::Synced }, ), };crates/dojo/core/src/world/world_contract.cairo (1)
849-886: Clean upgrade mechanism, sensei!The
upgrade_libraryimplementation:
- Properly validates ownership
- Maintains resource type safety
- Directly updates the class hash without unsafe library calls
However, consider adding a comment explaining why library upgrades don't need the same deployment validation as contracts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
bin/sozo/src/commands/inspect.rs(6 hunks)crates/dojo/core-cairo-test/src/world.cairo(2 hunks)crates/dojo/core/src/contract/interface.cairo(1 hunks)crates/dojo/core/src/lib.cairo(2 hunks)crates/dojo/core/src/world/iworld.cairo(2 hunks)crates/dojo/core/src/world/resource.cairo(3 hunks)crates/dojo/core/src/world/world_contract.cairo(6 hunks)crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo(1 hunks)crates/dojo/lang/src/cairo_plugin.rs(3 hunks)crates/dojo/world/src/contracts/abigen/world.rs(13 hunks)crates/dojo/world/src/contracts/contract_info.rs(2 hunks)crates/dojo/world/src/diff/compare.rs(3 hunks)crates/dojo/world/src/diff/manifest.rs(6 hunks)crates/dojo/world/src/lib.rs(1 hunks)crates/dojo/world/src/local/artifact_to_local.rs(4 hunks)crates/dojo/world/src/local/resource.rs(8 hunks)crates/dojo/world/src/remote/events_to_remote.rs(4 hunks)crates/dojo/world/src/remote/permissions.rs(2 hunks)crates/dojo/world/src/remote/resource.rs(15 hunks)crates/sozo/ops/src/migrate/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/dojo/world/src/lib.rs
- crates/dojo/core/src/contract/interface.cairo
- crates/dojo/world/src/remote/permissions.rs
- crates/dojo/world/src/local/resource.rs
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/resource.cairo
- crates/dojo/world/src/diff/compare.rs
- crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo
- crates/dojo/world/src/contracts/abigen/world.rs
🔇 Additional comments (18)
crates/dojo/lang/src/cairo_plugin.rs (2)
12-13: Ohayo! Import changes look good, sensei! 🎋The new imports for
DojoLibraryandDOJO_LIBRARY_ATTRare well-organized alongside other Dojo attributes.Also applies to: 17-17
62-63: Implementation looks solid, sensei! 🌟The library attribute handling follows the same robust pattern as contract handling. However, let's verify the error handling in
DojoLibrary::from_module.✅ Verification successful
Ohayo! Error handling looks perfect, sensei! 🎯
The
DojoLibrary::from_moduleimplementation follows the same robust error handling pattern asDojoContract, using the diagnostics vector and proper name validation. The code maintains consistency with other Dojo attribute handlers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in DojoLibrary implementation ast-grep --pattern 'impl DojoLibrary { $$$ fn from_module($_) { $$$ PluginDiagnostic { $$$ } $$$ } }'Length of output: 131
Script:
#!/bin/bash # First, find the file containing DojoLibrary rg "struct DojoLibrary" -A 5 # Then search for from_module implementation rg "fn from_module" -A 10 -B 2 # Backup search for any DojoLibrary mentions rg "DojoLibrary" -A 3Length of output: 7069
crates/dojo/core-cairo-test/src/world.cairo (2)
23-23: Addition ofLibraryvariant toTestResourceenumOhayo sensei, the addition of the
Libraryvariant to theTestResourceenum enhances resource representation and is implemented correctly.
176-178: Proper handling ofLibraryvariant inspawn_test_worldfunctionOhayo sensei, the
Libraryvariant is properly handled in thespawn_test_worldfunction, ensuring libraries are registered correctly.crates/dojo/world/src/diff/manifest.rs (1)
210-233: The library conversion function looks solid, sensei!The implementation follows the same pattern as
resource_diff_to_dojo_contract, which maintains consistency in the codebase.crates/dojo/world/src/remote/resource.rs (1)
298-304:⚠️ Potential issueFix the error message in
as_library_or_panic, sensei!The error message incorrectly states "contract" instead of "library".
Apply this fix:
- _ => panic!("Resource is expected to be a contract: {:?}.", self), + _ => panic!("Resource is expected to be a library: {:?}.", self),Likely invalid or redundant comment.
crates/dojo/core/src/world/iworld.cairo (1)
86-94: Excellent documentation for the new library methods, sensei!The
register_libraryandupgrade_librarymethods are well-documented and follow the same pattern as their contract counterparts.Also applies to: 129-137
crates/dojo/world/src/remote/events_to_remote.rs (4)
21-23: LGTM!Ohayo! The import of
LibraryRemotealigns with the new library functionality being added.
69-70: LGTM!The addition of library event selectors is consistent with the new library management capabilities.
247-271: LGTM!The library registration event handling follows the established pattern for other resources, sensei. Using
Felt::ZEROas the address is appropriate since libraries are class-hash based and don't have contract addresses.
317-331: LGTM!The library upgrade event handling is well-implemented and consistent with other resource upgrade patterns.
crates/dojo/world/src/local/artifact_to_local.rs (3)
21-21: LGTM! The library interface constant and enum variant are well-structured.The additions follow the established patterns in the codebase for interface constants and resource type variants.
Also applies to: 259-259
111-138: Ohayo sensei! The library resource handling looks solid.The implementation mirrors the contract resource handling pattern, including proper namespace validation, resource registration, and trace logging.
271-272: Clean implementation of library type identification, sensei!The code follows the established pattern for resource type identification and name extraction.
crates/dojo/core/src/world/world_contract.cairo (4)
71-72: Ohayo! Event definitions look good, sensei!The new events
LibraryRegisteredandLibraryUpgradedfollow the established pattern of other resource events in the contract, maintaining consistency in event handling.Also applies to: 112-126
45-45: Clean import addition, sensei!The
IDeployedResourceLibraryDispatcherimport aligns with the existing pattern of resource dispatchers.
810-846: Excellent safety implementation, sensei!The
register_libraryfunction avoids the unsafe library call pattern mentioned in the previous review by directly using the class hash. This is a secure approach as:
- Only namespace owners can register libraries
- No contract deployment is needed, reducing attack surface
- Proper permission checks are in place
1239-1244: Nice error handling addition, sensei!The error handling for library resources follows the established pattern, providing clear error messages that include the library name.
|
|
||
| use super::*; | ||
| use crate::diff::{DojoContract, DojoModel, WorldContract}; | ||
| use crate::diff::{DojoContract, DojoLibrary, DojoModel, WorldContract}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test does not verify inclusion of DojoLibrary in contracts_info
Ohayo sensei, the test test_manifest_to_contracts_info adds a DojoLibrary to the manifest, but does not verify its inclusion in the contracts_info HashMap. Since contracts_info currently does not include libraries, consider updating the implementation to include libraries or adjust the test to reflect the actual behavior.
If the intent is to include libraries in contracts_info, you might update the From<&Manifest> implementation to include libraries:
impl From<&Manifest> for HashMap<String, ContractInfo> {
fn from(manifest: &Manifest) -> Self {
trace!("Converting manifest to contracts info.");
let mut contracts = HashMap::new();
contracts.insert(
"world".to_string(),
ContractInfo {
tag: "world".to_string(),
address: manifest.world.address,
entrypoints: manifest.world.entrypoints.clone(),
},
);
for c in &manifest.contracts {
contracts.insert(
c.tag.clone(),
ContractInfo {
tag: c.tag.clone(),
address: c.address,
entrypoints: c.systems.clone(),
},
);
}
+ for l in &manifest.libraries {
+ contracts.insert(
+ l.tag.clone(),
+ ContractInfo {
+ tag: l.tag.clone(),
+ address: Felt::ZERO, // Update with actual address if available
+ entrypoints: l.systems.clone(),
+ },
+ );
+ }
contracts
}
}Also applies to: 150-156
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
crates/dojo/world/src/contracts/abigen/world.rs (1)
3872-3912: Ohayo sensei!
The newly introducedregister_librarymethods expand contract functionality. You could add optional validation for version strings to handle upgrades gracefully.crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo (1)
23-28: Add documentation for the internal trait sensei!The
$name$InternalTraitand its implementation lack documentation explaining the purpose and usage of theworldmethod.crates/dojo/world/src/local/resource.rs (1)
42-50: Add version field documentation sensei!Please add documentation for the
versionfield specifying the expected format (e.g., semantic versioning).crates/dojo/lang/src/attribute_macros/library.rs (1)
20-29: Add documentation for public APIOhayo sensei! Consider adding documentation for the public structs and their fields to improve API clarity.
#[derive(Debug, Clone, Default)] +/// Parameters for configuring a contract pub struct ContractParameters { + /// The namespace for the contract pub namespace: Option<String>, } #[derive(Debug)] +/// Represents a Dojo library with its diagnostics and systems pub struct DojoLibrary { + /// Collection of diagnostic messages diagnostics: Vec<PluginDiagnostic>, + /// List of system functions systems: Vec<String>, }crates/dojo/world/src/diff/manifest.rs (2)
66-83: Add documentation for DojoLibrary structOhayo sensei! Consider adding documentation for the struct and its fields:
#[serde_as] #[derive(Clone, Default, Debug, Serialize, Deserialize)] +/// Represents a Dojo library with its metadata and implementation details pub struct DojoLibrary { /// Class hash of the contract. #[serde_as(as = "UfeHex")] pub class_hash: Felt, /// ABI of the contract. pub abi: Vec<AbiEntry>, /// Tag of the contract. pub tag: String, /// Selector of the contract. #[serde_as(as = "UfeHex")] pub selector: Felt, /// Systems of the library. pub systems: Vec<String>, /// Version of the library pub version: String, }
232-232: Remove redundant cloneOhayo sensei! The
versionis already owned, soclone()is unnecessary:- version: version.clone(), + version,crates/dojo/world/src/local/artifact_to_local.rs (1)
267-267: Add test for library resource type identificationOhayo sensei! Consider adding a test case for the library resource type:
#[test] fn test_identify_resource_type() { // ... existing tests ... + assert_eq!( + identify_resource_type(&AbiImpl { + interface_name: LIBRARY_INTF.to_string(), + name: "Library__DojoLibraryImpl".to_string() + }), + ResourceType::Library("Library".to_string()) + ); }Also applies to: 279-280
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
bin/sozo/src/commands/hash.rs(2 hunks)bin/sozo/src/commands/inspect.rs(6 hunks)crates/dojo/core-cairo-test/src/world.cairo(2 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/iworld.cairo(1 hunks)crates/dojo/core/src/world/resource.cairo(3 hunks)crates/dojo/core/src/world/storage.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(6 hunks)crates/dojo/lang/src/attribute_macros/library.rs(1 hunks)crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo(1 hunks)crates/dojo/world/src/config/profile_config.rs(4 hunks)crates/dojo/world/src/contracts/abigen/world.rs(12 hunks)crates/dojo/world/src/contracts/contract_info.rs(2 hunks)crates/dojo/world/src/diff/compare.rs(3 hunks)crates/dojo/world/src/diff/manifest.rs(6 hunks)crates/dojo/world/src/local/artifact_to_local.rs(4 hunks)crates/dojo/world/src/local/resource.rs(8 hunks)crates/dojo/world/src/remote/events_to_remote.rs(3 hunks)crates/dojo/world/src/remote/resource.rs(15 hunks)crates/sozo/ops/src/migrate/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/dojo/core-cairo-test/src/world.cairo
- crates/dojo/core/src/world/resource.cairo
- crates/dojo/world/src/remote/events_to_remote.rs
- crates/dojo/world/src/diff/compare.rs
- crates/dojo/core/src/world/storage.cairo
- crates/dojo/world/src/remote/resource.rs
🔇 Additional comments (33)
crates/dojo/world/src/contracts/abigen/world.rs (10)
393-429: Ohayo sensei! Great addition.
This struct aligns with the existing pattern of resource registration. It's neat that you're capturing name, namespace, and class hash. If versioning is needed in the future, you could mirror how other resources handle optional fields.
430-437: Ohayo sensei!
Including dedicated methods such asevent_selector()andevent_name()forLibraryRegisteredmirrors your existing event approach. Everything looks consistent.
1037-1037: Ohayo sensei!
AddingLibraryRegisteredto theEventenum is a logical extension of your event structure.
1063-1063: Ohayo sensei!
Your match arm forEvent::LibraryRegisterednow properly accounts for serialized size.
1137-1142: Ohayo sensei!
SerializingLibraryRegisteredwithin the event ensures it behaves consistently with the rest of your event types.
1767-1809: Ohayo sensei!
This parsing logic forLibraryRegisteredin thetry_from(EmittedEvent)flow lines up with your approach for other resources. Confirm you have test coverage for malformed/edge inputs.
2691-2733: Ohayo sensei!
Double-check that the library data is parsed consistently in thistry_from(Event)block. Everything else looks proper for library events.
Line range hint
3299-3331: Ohayo sensei!
AddingLibraryto theResourceenum is a powerful step for resource extensibility. Keep an eye on potential version conflicts.
3372-3380: Ohayo sensei!
Nice addition of library support incairo_serialize. This ensures consistent encoding of the new resource type.
3411-3416: Ohayo sensei!
Your deserialization logic forResource::Librarymatches the existing pattern. Smooth integration overall.crates/dojo/core/src/world/errors.cairo (4)
30-30: Ohayo sensei!
Explicitly labeling it asResource (Event)enhances clarity for debugging. Nicely done.
34-34: Ohayo sensei!
Explicitly calling itResource (Model)maintains consistency among resource error messages.
38-39: Ohayo sensei!
Marking it asResource (Contract)is a good approach to keep messages specific and clear.
41-46: Ohayo sensei!
A dedicated error for the newLibraryresource is excellent. Prompting users about version updates is a practical nudge.crates/dojo/world/src/config/profile_config.rs (4)
23-23: Ohayo sensei!
Introducinglibrariesenhances the configurability of the system for library resources.
34-35: Ohayo sensei!
Good addition oflib_versions. Having a direct mapping of library name to version is straightforward and reduces confusion.
188-189: Ohayo sensei!
Defining[lib_versions]in the test TOML ensures your newly added field is properly tested. Great job covering that path.
270-273: Ohayo sensei!
Asserting thelib_versionscontent in the config is a solid way to confirm the parsing logic.bin/sozo/src/commands/hash.rs (2)
135-137: Ohayo! LGTM sensei!The library namespace handling follows the same pattern as other resources, maintaining consistency in the codebase.
170-172: Looks good sensei!The library resource name handling is implemented consistently with other resource types.
crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo (1)
1-31: Review safety implications of library calls sensei!Previous review comments indicated potential safety concerns with library calls.
Let's check for any safety measures in place:
#!/bin/bash # Search for safety-related code around library calls rg -A 5 "unsafe|safety|secure" --type cairocrates/dojo/world/src/local/resource.rs (1)
14-14: LGTM sensei!The Library variant is consistently handled across all match arms in the ResourceLocal implementation.
Also applies to: 93-93, 104-104, 114-114, 125-125, 164-164, 175-175
crates/dojo/world/src/contracts/contract_info.rs (2)
127-127: Test does not verify inclusion ofDojoLibraryincontracts_infoOhayo sensei! The test
test_manifest_to_contracts_infoadds aDojoLibraryto the manifest, but does not verify its inclusion in thecontracts_infoHashMap.Also applies to: 150-157
Line range hint
31-63: Consider handling libraries in theFrom<&Manifest>implementationOhayo sensei! The
From<&Manifest>implementation currently doesn't handle libraries. Consider whether libraries should be included in thecontracts_infoHashMap.Run this script to check if libraries need contract info:
✅ Verification successful
Libraries should not be included in the ContractInfo HashMap
Ohayo sensei! While libraries are indeed used in the codebase, they are fundamentally different from contracts - they don't have addresses or entrypoints, and instead use class hashes. The current ContractInfo structure is specifically designed for contracts. If library metadata tracking is needed, it should be implemented in a separate structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if libraries are used like contracts in the codebase # Search for library registration and usage patterns rg -A 5 "register_library|ILibrary"Length of output: 6263
crates/dojo/lang/src/attribute_macros/library.rs (2)
48-49:⚠️ Potential issueFix error message to reference library instead of contract
Ohayo sensei! The error message incorrectly references "contract" instead of "library":
- "The contract {id} '{value}' can only contain characters (a-z/A-Z), \ + "The library {id} '{value}' can only contain characters (a-z/A-Z), \Likely invalid or redundant comment.
123-128:⚠️ Potential issueAdd error handling for version retrieval
Ohayo sensei! The version retrieval uses
expectwhich will panic if versions are missing. Consider proper error handling:- let version = profile_config - .lib_versions - .as_ref() - .expect("expected lib version a") - .get(&format!("{}-{}", ns, name)) - .expect("expected lib version b"); + let version = profile_config + .lib_versions + .as_ref() + .ok_or_else(|| anyhow::anyhow!("Library versions not configured"))? + .get(&format!("{}-{}", ns, name)) + .ok_or_else(|| anyhow::anyhow!("Version not found for library {}-{}", ns, name))?;Likely invalid or redundant comment.
crates/sozo/ops/src/migrate/mod.rs (2)
149-153: LGTM! Library metadata upload follows the established pattern.The implementation correctly handles library metadata upload, following the same pattern used for other resources.
637-686: LGTM! Library registration implementation looks solid.The implementation:
- Correctly handles class declaration and registration
- Properly validates namespace ownership
- Includes appropriate tracing
- Follows the same pattern as other resources
crates/dojo/core/src/world/world_contract.cairo (3)
111-119: Ohayo! The LibraryRegistered event looks good sensei!The event structure follows the established pattern and includes all necessary fields with appropriate key annotations.
802-841: The register_library implementation is robust but needs test coverage.The implementation correctly:
- Validates namespace and name
- Checks permissions
- Prevents duplicate registrations
- Emits appropriate event
However, there's no test coverage visible for this new functionality.
Would you like me to help generate comprehensive test cases for the library registration functionality?
1194-1199: LGTM! Library support in panic_with_details is consistent.The implementation follows the same pattern as other resources and correctly uses IDeployedResourceLibraryDispatcher.
bin/sozo/src/commands/inspect.rs (2)
80-80: Ohayo! Clean enum variant addition, sensei! 🎉The
Libraryvariant is well-placed within theResourceInspectenum, maintaining consistency with other resource types.
119-131: TheLibraryInspectstruct looks solid, sensei!The struct follows the same pattern as other resource inspection structs, with appropriate field annotations for table display.
| /// Registers and declare a library associated with the world and returns the class_hash of | ||
| /// newly declared library. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `namespace` - The namespace of the contract to be registered. | ||
| /// * `class_hash` - The class hash of the library. | ||
| /// * `name` - The name of the library. | ||
| /// * `version` - The version of the library. | ||
| fn register_library( | ||
| ref self: T, | ||
| namespace: ByteArray, | ||
| class_hash: ClassHash, | ||
| name: ByteArray, | ||
| version: ByteArray, | ||
| ) -> ClassHash; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Version validation is indeed missing sensei!
The codebase search confirms that there's no validation for the version format in the register_library method or elsewhere. The original suggestion to add semantic versioning validation remains valid.
🔗 Analysis chain
Ohayo! Consider adding version format validation sensei!
While the method signature and documentation look good, consider adding validation for the version format to ensure it follows semantic versioning (e.g., "1.0.0").
Let's check if version validation is implemented elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version validation in the codebase
rg -A 5 "version.*validate|validate.*version"
Length of output: 65584
| let version = diff | ||
| .profile_config | ||
| .lib_versions | ||
| .as_ref() | ||
| .expect("expected lib_versions") | ||
| .get(&tag) | ||
| .expect("library mush have a version"); |
There was a problem hiding this comment.
Add error handling for version retrieval
Ohayo sensei! The version retrieval uses expect which will panic if versions are missing. Consider proper error handling:
- let version = diff
- .profile_config
- .lib_versions
- .as_ref()
- .expect("expected lib_versions")
- .get(&tag)
- .expect("lib_version not found");
+ let version = diff
+ .profile_config
+ .lib_versions
+ .as_ref()
+ .ok_or_else(|| anyhow::anyhow!("Library versions not configured"))?
+ .get(&tag)
+ .ok_or_else(|| anyhow::anyhow!("Version not found for library {}", tag))?;📝 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 version = diff | |
| .profile_config | |
| .lib_versions | |
| .as_ref() | |
| .expect("expected lib_versions") | |
| .get(&tag) | |
| .expect("library mush have a version"); | |
| let version = diff | |
| .profile_config | |
| .lib_versions | |
| .as_ref() | |
| .ok_or_else(|| anyhow::anyhow!("Library versions not configured"))? | |
| .get(&tag) | |
| .ok_or_else(|| anyhow::anyhow!("Version not found for library {}", tag))?; |
| let version = profile_config | ||
| .lib_versions | ||
| .as_ref() | ||
| .expect("expected lib version a") | ||
| .get(&format!("{}-{}", ns, name)) | ||
| .expect("expected lib version b"); |
There was a problem hiding this comment.
Add error handling for version retrieval
Ohayo sensei! The version retrieval uses expect which will panic if versions are missing. Consider proper error handling:
- let version = profile_config
- .lib_versions
- .as_ref()
- .expect("expected lib version a")
- .get(&format!("{}-{}", ns, name))
- .expect("expected lib version b");
+ let version = profile_config
+ .lib_versions
+ .as_ref()
+ .ok_or_else(|| anyhow::anyhow!("Library versions not configured"))?
+ .get(&format!("{}-{}", ns, name))
+ .ok_or_else(|| anyhow::anyhow!("Version not found for library {}-{}", ns, name))?;📝 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 version = profile_config | |
| .lib_versions | |
| .as_ref() | |
| .expect("expected lib version a") | |
| .get(&format!("{}-{}", ns, name)) | |
| .expect("expected lib version b"); | |
| let version = profile_config | |
| .lib_versions | |
| .as_ref() | |
| .ok_or_else(|| anyhow::anyhow!("Library versions not configured"))? | |
| .get(&format!("{}-{}", ns, name)) | |
| .ok_or_else(|| anyhow::anyhow!("Version not found for library {}-{}", ns, name))?; |
| ResourceRemote::Library(_library_remote), | ||
| ) = resource | ||
| { | ||
| panic!("libraries cannot be updated!") |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Library immutability constraint needs documentation
The codebase enforces that libraries cannot be updated through a panic, but this important constraint is not documented anywhere. Users should be informed about this limitation before attempting library updates.
- Consider adding documentation in README or relevant docs explaining:
- That libraries are immutable
- The rationale behind this design decision
- Any workarounds or alternatives users should consider
🔗 Analysis chain
Verify that library immutability is documented.
The code enforces library immutability through a panic, which is good. However, this important constraint should be clearly documented for users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if library immutability is documented in README or docs
rg -i "librar.*immutable|librar.*cannot be (updated|upgraded)"
Length of output: 150
Script:
#!/bin/bash
# Broader search for library-related documentation
echo "=== Searching for library-related docs ==="
fd -t f -e md -e txt | xargs rg -i "librar(y|ies)" -A 2 -B 2
echo -e "\n=== Checking specific terms in docs ==="
rg -i "librar(y|ies).*(static|fixed|permanent|chang|modif)"
echo -e "\n=== Looking for README files ==="
fd README -t f
Length of output: 67705
| ResourceType::Library => { | ||
| let (_current_class_hash, status) = match resource { | ||
| ResourceDiff::Created(_) => { | ||
| (resource.current_class_hash(), ResourceStatus::Created) | ||
| } | ||
| ResourceDiff::Updated(_, _remote) => { | ||
| (resource.current_class_hash(), ResourceStatus::Updated) | ||
| } | ||
| ResourceDiff::Synced(_, remote) => ( | ||
| remote.current_class_hash(), | ||
| if has_dirty_perms { | ||
| ResourceStatus::DirtyLocalPerms | ||
| } else { | ||
| ResourceStatus::Synced | ||
| }, | ||
| ), | ||
| }; | ||
|
|
||
| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | ||
| ResourceStatus::MigrationSkipped | ||
| } else { | ||
| status | ||
| }; | ||
|
|
||
| let version = world_diff | ||
| .profile_config | ||
| .lib_versions | ||
| .as_ref() | ||
| .expect("expected lib_versions") | ||
| .get(&resource.tag()) | ||
| .expect("lib_version not found"); | ||
|
|
||
| ResourceInspect::Library(LibraryInspect { | ||
| tag: resource.tag(), | ||
| status, | ||
| current_class_hash: format!("{:#066x}", resource.current_class_hash()), | ||
| selector: format!("{:#066x}", resource.dojo_selector()), | ||
| version: version.to_string(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential panic in library version lookup, sensei! 🚨
The code uses expect() which could panic if the library version is not found. Consider using more graceful error handling.
Here's a suggested improvement:
- let version = world_diff
- .profile_config
- .lib_versions
- .as_ref()
- .expect("expected lib_versions")
- .get(&resource.tag())
- .expect("lib_version not found");
+ let version = world_diff
+ .profile_config
+ .lib_versions
+ .as_ref()
+ .and_then(|versions| versions.get(&resource.tag()))
+ .map(ToString::to_string)
+ .unwrap_or_else(|| "unknown".to_string());📝 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.
| ResourceType::Library => { | |
| let (_current_class_hash, status) = match resource { | |
| ResourceDiff::Created(_) => { | |
| (resource.current_class_hash(), ResourceStatus::Created) | |
| } | |
| ResourceDiff::Updated(_, _remote) => { | |
| (resource.current_class_hash(), ResourceStatus::Updated) | |
| } | |
| ResourceDiff::Synced(_, remote) => ( | |
| remote.current_class_hash(), | |
| if has_dirty_perms { | |
| ResourceStatus::DirtyLocalPerms | |
| } else { | |
| ResourceStatus::Synced | |
| }, | |
| ), | |
| }; | |
| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | |
| ResourceStatus::MigrationSkipped | |
| } else { | |
| status | |
| }; | |
| let version = world_diff | |
| .profile_config | |
| .lib_versions | |
| .as_ref() | |
| .expect("expected lib_versions") | |
| .get(&resource.tag()) | |
| .expect("lib_version not found"); | |
| ResourceInspect::Library(LibraryInspect { | |
| tag: resource.tag(), | |
| status, | |
| current_class_hash: format!("{:#066x}", resource.current_class_hash()), | |
| selector: format!("{:#066x}", resource.dojo_selector()), | |
| version: version.to_string(), | |
| }) | |
| } | |
| ResourceType::Library => { | |
| let (_current_class_hash, status) = match resource { | |
| ResourceDiff::Created(_) => { | |
| (resource.current_class_hash(), ResourceStatus::Created) | |
| } | |
| ResourceDiff::Updated(_, _remote) => { | |
| (resource.current_class_hash(), ResourceStatus::Updated) | |
| } | |
| ResourceDiff::Synced(_, remote) => ( | |
| remote.current_class_hash(), | |
| if has_dirty_perms { | |
| ResourceStatus::DirtyLocalPerms | |
| } else { | |
| ResourceStatus::Synced | |
| }, | |
| ), | |
| }; | |
| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | |
| ResourceStatus::MigrationSkipped | |
| } else { | |
| status | |
| }; | |
| let version = world_diff | |
| .profile_config | |
| .lib_versions | |
| .as_ref() | |
| .and_then(|versions| versions.get(&resource.tag())) | |
| .map(ToString::to_string) | |
| .unwrap_or_else(|| "unknown".to_string()); | |
| ResourceInspect::Library(LibraryInspect { | |
| tag: resource.tag(), | |
| status, | |
| current_class_hash: format!("{:#066x}", resource.current_class_hash()), | |
| selector: format!("{:#066x}", resource.dojo_selector()), | |
| version: version.to_string(), | |
| }) | |
| } |
| let mut contracts_disp = vec![]; | ||
| let mut models_disp = vec![]; | ||
| let mut events_disp = vec![]; | ||
| let mut libraries_disp = vec![]; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Tests needed for library display order, sensei! 🧪
The implementation shows a specific display order (namespaces → contracts → libraries → models → events), but there are no tests verifying this behavior. Consider adding tests to ensure:
- Library resources are correctly collected
- Libraries are properly sorted by tag
- Display order remains consistent
🔗 Analysis chain
Clean integration of library display, sensei! 👍
The changes to inspect_world properly integrate library resource display alongside other resource types. However, consider adding a test to verify the display order.
Let's verify the test coverage:
Also applies to: 305-308, 317-317, 321-321
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to library display
rg -l "test.*library.*display" --type rust
Length of output: 44
Script:
#!/bin/bash
# Search for inspect-related tests
echo "=== Inspect Tests ==="
rg "mod tests" -A 5 bin/sozo/src/commands/inspect.rs
# Search for library-related tests
echo -e "\n=== Library Tests ==="
rg "#\[test\]" -A 5 -B 1 bin/sozo/src/commands/inspect.rs | rg -A 5 -B 1 "library"
# Look for the implementation
echo -e "\n=== Library Display Implementation ==="
rg "libraries_disp" -A 5 bin/sozo/src/commands/inspect.rs
Length of output: 1329
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
crates/dojo/world/src/contracts/contract_info.rs (1)
37-75:⚠️ Potential issueImplementation missing for library resources
Ohayo sensei, both
From<&Manifest>andFrom<&WorldDiff>implementations need to be updated to handle library resources consistently. Currently, libraries are stored in the manifest but not included in the contract info mapping.Consider updating both implementations to handle libraries:
For
From<&Manifest>:impl From<&Manifest> for HashMap<String, ContractInfo> { fn from(manifest: &Manifest) -> Self { // ... existing code ... + for l in &manifest.libraries { + contracts.insert( + l.tag.clone(), + ContractInfo { + tag_or_name: l.tag.clone(), + address: Felt::ZERO, // Update with actual address if available + entrypoints: l.systems.clone(), + }, + ); + } contracts } }For
From<&WorldDiff>:impl From<&WorldDiff> for HashMap<String, ContractInfo> { fn from(world_diff: &WorldDiff) -> Self { // ... existing code ... for (selector, resource) in &world_diff.resources { let tag = resource.tag(); match resource { + ResourceDiff::Created(ResourceLocal::Library(l)) => { + let address = world_diff.get_contract_address(*selector).unwrap(); + contracts.insert( + tag.clone(), + ContractInfo { + tag_or_name: tag.clone(), + address, + entrypoints: l.systems.clone(), + }, + ); + } + ResourceDiff::Updated(ResourceLocal::Library(l), ResourceRemote::Library(r)) => { + contracts.insert( + tag.clone(), + ContractInfo { + tag_or_name: tag.clone(), + address: r.common.address, + entrypoints: l.systems.clone(), + }, + ); + } + ResourceDiff::Synced(ResourceLocal::Library(l), ResourceRemote::Library(r)) => { + contracts.insert( + tag.clone(), + ContractInfo { + tag_or_name: tag.clone(), + address: r.common.address, + entrypoints: l.systems.clone(), + }, + ); + } // ... existing code ... } } contracts } }Also applies to: 77-146
crates/dojo/world/src/local/artifact_to_local.rs (1)
454-487: 🛠️ Refactor suggestionAdd test cases for library resource type
Ohayo sensei! The test coverage should be expanded to include:
#[test] fn test_identify_resource_type_library() { assert_eq!( identify_resource_type(&AbiImpl { interface_name: LIBRARY_INTF.to_string(), name: "Library__DojoLibraryImpl".to_string() }), ResourceType::Library("Library".to_string()) ); }
♻️ Duplicate comments (2)
bin/sozo/src/commands/inspect.rs (1)
459-466:⚠️ Potential issuePotential panic in library version lookup, sensei! 🚨
The code uses
expect()which could panic if the library version is not found.- let version = world_diff - .profile_config - .lib_versions - .as_ref() - .expect("expected lib_versions") - .get(&resource.tag()) - .expect("lib_version not found"); + let version = world_diff + .profile_config + .lib_versions + .as_ref() + .and_then(|versions| versions.get(&resource.tag())) + .map(ToString::to_string) + .unwrap_or_else(|| "unknown".to_string());crates/dojo/world/src/local/artifact_to_local.rs (1)
122-165: 🛠️ Refactor suggestionAdd error handling for version retrieval
Ohayo sensei! The version retrieval could be improved with proper error handling:
- if let Some(version) = profile_config.lib_versions.as_ref() { - if let Some(v) = version.get(&format!("{}-{}", ns, name)) { + if let Some(version) = profile_config.lib_versions.as_ref().ok_or_else(|| anyhow::anyhow!("Library versions not configured"))? { + if let Some(v) = version.get(&format!("{}-{}", ns, name)).ok_or_else(|| anyhow::anyhow!("Version not found for library {}-{}", ns, name))? {Add test coverage for library resource handling
Consider adding test cases to verify:
- Library resource creation with valid version
- Warning when version is missing
- Error handling for invalid versions
🧹 Nitpick comments (6)
crates/dojo/world/src/config/profile_config.rs (2)
246-247: Consider expanding test coverage for library configurations.Ohayo sensei! While the basic test case for library versions is good, we could enhance the test coverage by:
- Testing empty/null cases for both new fields
- Adding test cases for library resource configurations similar to models/contracts
- Validating version format constraints if any exist
Here's a suggested test addition:
#[test] fn test_profile_config_libraries() { let content = r#" [world] name = "test" seed = "abcd" [[libraries]] tag = "ns1-lib1" description = "This is the lib1 library" icon_uri = "ipfs://dojo/lib1.png" [lib_versions] "ns1-lib1" = "1.0.0" "ns1-lib2" = "0.1.0" "#; let config = toml::from_str::<ProfileConfig>(content).unwrap(); assert!(config.libraries.is_some()); let libraries = config.libraries.unwrap(); assert_eq!(libraries.len(), 1); assert_eq!(libraries[0].tag, "ns1-lib1"); assert!(config.lib_versions.is_some()); let versions = config.lib_versions.unwrap(); assert_eq!(versions.len(), 2); assert_eq!(versions.get("ns1-lib1"), Some(&"1.0.0".to_string())); }Also applies to: 328-331
100-145: Consider adding validation for library configurations.Ohayo sensei! The
validate()method could be enhanced to ensure:
- Version strings follow semantic versioning format
- Library versions correspond to registered libraries
Here's a suggested enhancement to the validation method:
pub fn validate(&self) -> Result<()> { + // Validate library versions + if let Some(versions) = &self.lib_versions { + for (lib, version) in versions { + // Verify version format (semver) + if !is_valid_semver(version) { + bail!( + "Library version '{}' for '{}' is not a valid semantic version", + version, lib + ); + } + + // Verify library exists if libraries are specified + if let Some(libraries) = &self.libraries { + if !libraries.iter().any(|l| l.tag == *lib) { + bail!( + "Version specified for unknown library '{}'", + lib + ); + } + } + } + } + if let Some(contracts) = &self.external_contracts { // ... existing validation ... } Ok(()) } +fn is_valid_semver(version: &str) -> bool { + version.split('.').filter_map(|s| s.parse::<u32>().ok()).count() == 3 +}crates/dojo/core/src/world/world_contract.cairo (1)
803-842: Ohayo sensei! Please review the library registration security.The implementation follows good practices for resource registration, but based on past review comments, we should ensure that library calls are handled safely to prevent unauthorized class hash replacements.
Consider adding a comment explaining the security assumptions around namespace ownership and library registration.
fn register_library( ref self: ContractState, namespace: ByteArray, class_hash: ClassHash, name: ByteArray, version: ByteArray, ) -> ClassHash { + // Security note: Library registration is restricted to namespace owners. + // We accept the class hash directly since library calls are not safe in the world contract. + // The contract name is constructed from name and version, and may not be directly bound to the library. let caller = get_caller_address();crates/sozo/ops/src/migrate/mod.rs (2)
503-513: Consider enhancing error handling in the library sync branch.While the implementation is correct, consider adding error logging or tracing for failed library syncs to help with debugging.
ResourceType::Library => { + trace!("Syncing library resource: {}", resource.tag()); let (library_calls, library_classes) = self.libraries_calls_classes(resource).await?; if !library_calls.is_empty() { + trace!("Library sync generated {} calls", library_calls.len()); n_resources += 1; } invoker.extend_calls(library_calls); classes.extend(library_classes); }
762-762: Enhance the panic message for library updates.The current panic message could be more informative to help users understand why libraries cannot be updated.
- panic!("libraries cannot be updated!") + panic!("Library updates are not supported - libraries are designed to be immutable once registered!")crates/dojo/core-cairo-test/src/tests/helpers/library.cairo (1)
6-16: Consider adding documentation for the library module, sensei! 📚While the implementation is correct, it would be helpful to add documentation explaining:
- The purpose of this library
- The significance of the hardcoded value 42
- Example usage
#[dojo::library] +/// A simple library implementation for testing purposes. +/// Returns a hardcoded value of 42 for demonstration. +/// +/// # Example +/// ```cairo +/// let byte = library_a::get_byte(); +/// assert(byte == 42, 'should return 42'); +/// ``` pub mod library_a { use super::LibraryA;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/benchmark/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(3 hunks)bin/sozo/src/commands/inspect.rs(6 hunks)crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/library.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/world.cairo(2 hunks)crates/dojo/core/src/lib.cairo(2 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(6 hunks)crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo(1 hunks)crates/dojo/lang/src/cairo_plugin.rs(3 hunks)crates/dojo/world/src/config/profile_config.rs(4 hunks)crates/dojo/world/src/contracts/contract_info.rs(2 hunks)crates/dojo/world/src/diff/manifest.rs(6 hunks)crates/dojo/world/src/local/artifact_to_local.rs(4 hunks)crates/dojo/world/src/remote/events_to_remote.rs(3 hunks)crates/sozo/ops/src/migrate/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/errors.cairo
- crates/dojo/world/src/diff/manifest.rs
- Cargo.toml
- crates/dojo/lang/src/attribute_macros/patches/library.patch.cairo
🔇 Additional comments (15)
crates/dojo/world/src/config/profile_config.rs (1)
33-33: LGTM! Clean and well-documented additions.Ohayo sensei! The new fields for library support are well-structured and properly documented. The optional nature of both fields aligns with the existing pattern in the struct.
Also applies to: 45-46
crates/dojo/world/src/contracts/contract_info.rs (1)
178-185: Test does not verify inclusion ofDojoLibraryincontracts_infoOhayo sensei, the test
test_manifest_to_contracts_infoadds aDojoLibraryto the manifest, but does not verify its inclusion in thecontracts_infoHashMap. Sincecontracts_infocurrently does not include libraries, consider updating the implementation to include libraries or adjust the test to reflect the actual behavior.Also applies to: 196-203
crates/dojo/core/src/world/world_contract.cairo (2)
45-46: Ohayo! Event system changes look good!The event system changes for library support follow the established patterns in the codebase, with proper key annotations and consistent structure.
Also applies to: 72-73, 112-119
1199-1204: Library resource panic handling looks good!As noted in a previous review, this implementation is safe since it only handles panic details.
crates/sozo/ops/src/migrate/mod.rs (2)
154-158: LGTM! Clean implementation of library metadata upload.The implementation follows the same pattern as other resources, maintaining code consistency.
717-720: Add documentation explaining library immutability.Ohayo sensei! The code enforces library immutability but lacks documentation explaining this important constraint.
Add documentation to explain:
- That libraries are immutable by design
- The rationale behind this design decision
- Any workarounds users should consider
/// Gathers the calls required to sync the libraries' classes to be declared. /// +/// # Library Immutability +/// +/// Libraries in Dojo are designed to be immutable once registered. This means: +/// - Libraries cannot be upgraded after registration +/// - To modify a library's functionality, you must register a new library with a different version +/// - This design ensures consistent behavior and prevents dependency conflicts +/// /// Returns a tuple of calls and (casm_class_hash, class) to be declared.crates/dojo/core-cairo-test/src/tests/helpers/library.cairo (1)
1-4: Ohayo! The trait definition looks good, sensei! 👋The
LibraryAtrait is well-defined with a clear interface and proper use of generics.crates/dojo/core-cairo-test/src/lib.cairo (1)
62-63: LGTM! Clean module organization, sensei! 🎯The library module is properly integrated into the helpers section, following the established pattern.
bin/sozo/src/commands/inspect.rs (1)
119-131: Clean library inspection implementation, sensei! ✨The
LibraryInspectstruct is well-designed and follows the established pattern of other resource types.crates/dojo/world/src/local/artifact_to_local.rs (2)
25-25: LGTM! Library interface and resource type additionsThe new library interface constant and enum variant are well-integrated with the existing resource types.
Also applies to: 373-373
385-386: LGTM! Library resource type identificationThe library resource type identification follows the established pattern consistently.
crates/dojo/world/src/remote/events_to_remote.rs (1)
69-69: LGTM! Library event selector additionThe
LibraryRegisteredevent selector is properly integrated with other event selectors.crates/dojo/core/src/world/storage.cairo (3)
80-82: Ohayo sensei! Clean implementation of library resource handling!The use of
starknet::contract_address_const::<0>()is a good practice for zero address representation.
87-92: Clean and focused implementation, sensei!The method provides a convenient way to extract just the contract address, following good API design principles.
94-99: Well-structured companion method, sensei!The method nicely complements
dns_addressby providing a focused way to extract just the class hash.
| #[test] | ||
| fn test_register_library() { | ||
| let world = deploy_world(); | ||
| let world = world.dispatcher; | ||
|
|
||
| world.register_library("dojo", library_a::TEST_CLASS_HASH.try_into().unwrap(), "liba", "0_1_0"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for library registration, sensei! 🧪
The test should verify that the library was registered successfully. Consider adding assertions to check:
- The library's existence in the world
- The correct version and class hash
- The library's functionality after registration
#[test]
fn test_register_library() {
let world = deploy_world();
let world = world.dispatcher;
world.register_library("dojo", library_a::TEST_CLASS_HASH.try_into().unwrap(), "liba", "0_1_0");
+
+ // Verify library registration
+ let library = LibraryALibraryDispatcher { contract_address: world.contract_address };
+ assert(library.get_byte() == 42, 'library not registered correctly');
}📝 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.
| #[test] | |
| fn test_register_library() { | |
| let world = deploy_world(); | |
| let world = world.dispatcher; | |
| world.register_library("dojo", library_a::TEST_CLASS_HASH.try_into().unwrap(), "liba", "0_1_0"); | |
| } | |
| #[test] | |
| fn test_register_library() { | |
| let world = deploy_world(); | |
| let world = world.dispatcher; | |
| world.register_library("dojo", library_a::TEST_CLASS_HASH.try_into().unwrap(), "liba", "0_1_0"); | |
| // Verify library registration | |
| let library = LibraryALibraryDispatcher { contract_address: world.contract_address }; | |
| assert(library.get_byte() == 42, 'library not registered correctly'); | |
| } |
|
|
||
| print_table(&namespaces_disp, Some(Color::FG_BRIGHT_BLACK), None); | ||
| print_table(&contracts_disp, Some(Color::FG_BRIGHT_BLACK), None); | ||
| print_table(&libraries_disp, Some(Color::FG_BRIGHT_BLACK), None); |
There was a problem hiding this comment.
Remove duplicate library table printing, sensei! 🔄
The library table is printed twice at lines 357 and 360. Remove one of these calls.
print_table(&namespaces_disp, Some(Color::FG_BRIGHT_BLACK), None);
print_table(&contracts_disp, Some(Color::FG_BRIGHT_BLACK), None);
print_table(&libraries_disp, Some(Color::FG_BRIGHT_BLACK), None);
print_table(&models_disp, Some(Color::FG_BRIGHT_BLACK), None);
print_table(&events_disp, Some(Color::FG_BRIGHT_BLACK), None);
- print_table(&libraries_disp, Some(Color::FG_BRIGHT_BLACK), None);
print_table(&external_contracts_disp, Some(Color::FG_BRIGHT_BLACK), None);Also applies to: 360-360
| WorldEvent::LibraryRegistered(e) => { | ||
| let namespace = e.namespace.to_string()?; | ||
|
|
||
| if !is_whitelisted(whitelisted_namespaces, &namespace) { | ||
| debug!( | ||
| namespace, | ||
| contract = e.name.to_string()?, | ||
| "Library's namespace not whitelisted." | ||
| ); | ||
|
|
||
| return Ok(()); | ||
| } | ||
|
|
||
| let full_name = e.name.to_string().unwrap(); | ||
| let version = full_name.split(&"_v").last().expect("expected version"); | ||
| let name = full_name.replace(&format!("_v{}", version), ""); | ||
| let r = ResourceRemote::Library(LibraryRemote { | ||
| common: CommonRemoteInfo::new( | ||
| e.class_hash.into(), | ||
| &namespace, | ||
| &name.to_string(), | ||
| Felt::ZERO, | ||
| ), | ||
| version: version.to_string(), | ||
| }); | ||
| trace!(?r, "Library registered."); | ||
|
|
||
| self.add_resource(r); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve version extraction safety
Ohayo sensei! The version extraction could be made safer:
- let version = full_name.split(&"_v").last().expect("expected version");
+ let version = full_name.split(&"_v").last()
+ .ok_or_else(|| anyhow::anyhow!("Invalid library name format: missing version"))?;Add test coverage for library event handling
Consider adding test cases to verify:
- Library registration event handling
- Version extraction from library name
- Handling of invalid library names
Here's a suggested test case:
#[tokio::test]
async fn test_library_registered_event() {
let mut world_remote = WorldRemote::default();
let event = WorldEvent::LibraryRegistered(world::LibraryRegistered {
class_hash: Felt::ONE.into(),
name: ByteArray::from_string("lib_v1.0.0").unwrap(),
namespace: ByteArray::from_string("ns").unwrap(),
});
world_remote.match_event(event, &NO_WHITELIST).unwrap();
let selector = naming::compute_selector_from_names("ns", "lib");
assert!(world_remote.resources.contains_key(&selector));
let resource = world_remote.resources.get(&selector).unwrap();
assert!(matches!(resource, ResourceRemote::Library(_)));
}
2 participants
Description
Support
#[dojo::library]Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Release Notes
New Features
Libraryvariant to resource types for better categorization.Improvements
Technical Updates