feat(sozo): External contracts management #2995
Conversation
remybar
force-pushed
the
feat-core-starknet_contract_support
branch
2 times, most recently
from
8123c32 to
caed4b7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2995 +/- ##
==========================================
+ Coverage 57.14% 57.37% +0.22%
==========================================
Files 429 430 +1
Lines 56834 57211 +377
==========================================
+ Hits 32477 32824 +347
- Misses 24357 24387 +30 ☔ View full report in Codecov by Sentry. |
|
Ohayo, sensei! WalkthroughThis pull request updates JSON configuration files with new target addresses and method names, and introduces new asynchronous utility functions for on-chain class declaration and contract deployment checks. It enhances external contract management by adding new structures, enums, and fields in diff, local, and remote modules. The migration process now includes methods for syncing external contracts, and several new contract modules (Bank, ERC20Token, Saloon, Dungeon) are added along with dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Declare Function Caller
participant Decl as is_declared
participant Prov as Provider
Caller->>Decl: Call is_declared(class_name, class_hash)
Decl->>Prov: get_class(class_hash)
Prov-->>Decl: Return class info / not found
Decl-->>Caller: Return true/false
sequenceDiagram
participant Client as Caller
participant Deployer as Deployer
participant UDC as deploy_via_udc_getcall
Client->>Deployer: deploy_via_udc(params)
Deployer->>UDC: deploy_via_udc_getcall(params)
UDC-->>Deployer: Optional(contract address, call)
alt Contract already deployed
Deployer-->>Client: TransactionResult::Noop
else Needs deployment
Deployer->>Client: Execute deployment & return receipt
end
Suggested labels
Tip 🌐 Web search-backed reviews and chat
🪧 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: 1
🧹 Nitpick comments (8)
crates/dojo/world/src/config/profile_config.rs (1)
95-102: Ohayo sensei! Thorough doc comments on thevalidatemethod.
These guidelines clearly describe how external contracts should be validated, helping future devs understand the constraints.crates/dojo/world/src/local/artifact_to_local.rs (2)
35-35: Ohayo sensei!
Introducingexternal_contract_classesis a nice step toward organizing external contract data.Consider a short doc comment explaining what goes into
external_contract_classesfor clarity.
76-117: Ohayo sensei!
Markingdojo_resource_foundto true at these points effectively skips the fallback path. This conditional logic is straightforward, though repeated flags might be collapsed in future if you see duplication.crates/sozo/ops/src/migrate/mod.rs (2)
542-604: Ohayo sensei! Thesync_external_contractsmethod is a solid addition.
Declaring first, then deploying keeps the code cohesive. The separate usage ofDeployerandInvokeris logically clean.It might be nice to add a test that ensures coverage for the newly created external contracts. This would address the codecov warnings.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 596-597: crates/sozo/ops/src/migrate/mod.rs#L596-L597
Added lines #L596 - L597 were not covered by tests
834-853: Ohayo sensei!external_contract_classesimproves consistency.
This helper neatly extracts classes that need declaration. Consider returning an error if an unexpected variant arises, rather thanNone.examples/spawn-and-move/src/bank.cairo (1)
1-15: Consider enhancing the Bank contract with standard patterns, sensei!While the basic structure is good, consider adding:
- Owner-only modifier for future privileged operations
- Events for ownership changes
- Owner transfer functionality
Here's a suggested implementation:
#[starknet::contract] mod Bank { use starknet::storage::StoragePointerWriteAccess; use starknet::ContractAddress; + use starknet::get_caller_address; #[storage] struct Storage { owner: ContractAddress, } + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + OwnershipTransferred: OwnershipTransferred, + } + + #[derive(Drop, starknet::Event)] + struct OwnershipTransferred { + previous_owner: ContractAddress, + new_owner: ContractAddress, + } + #[constructor] fn constructor(ref self: ContractState, owner: ContractAddress) { self.owner.write(owner); + self.emit(Event::OwnershipTransferred(OwnershipTransferred { + previous_owner: ContractAddress::from(0), + new_owner: owner + })); } + + fn assert_only_owner(self: @ContractState) { + let caller = get_caller_address(); + assert(caller == self.owner.read(), 'Caller is not the owner'); + } + + #[external(v0)] + fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { + self.assert_only_owner(); + let previous_owner = self.owner.read(); + self.owner.write(new_owner); + self.emit(Event::OwnershipTransferred(OwnershipTransferred { + previous_owner, + new_owner + })); + } }examples/spawn-and-move/src/erc20_token.cairo (1)
25-35: Add input validation and documentation to constructor, sensei!While the implementation is solid, consider these improvements:
- Add input validation for initial_supply
- Add documentation for the constructor parameters
+ /// @notice Initializes the ERC20 token + /// @param name The name of the token + /// @param symbol The symbol of the token + /// @param initial_supply The initial amount to mint + /// @param recipient The address to receive the initial supply #[constructor] fn constructor( ref self: ContractState, name: ByteArray, symbol: ByteArray, initial_supply: u256, recipient: ContractAddress, ) { + assert(initial_supply > 0, 'Initial supply must be positive'); + assert(recipient.is_non_zero(), 'Invalid recipient'); self.erc20.initializer(name, symbol); self.erc20.mint(recipient, initial_supply); }crates/dojo/world/src/diff/external_contract.rs (1)
19-24: Consider optimizing clone operations, sensei!Both implementations use
clone()which could be expensive for large contract data. Consider using references where possible.- pub fn class_data(&self) -> ExternalContractClassLocal { + pub fn class_data(&self) -> &ExternalContractClassLocal { match self { - ExternalContractClassDiff::Created(c) => c.clone(), - ExternalContractClassDiff::Synced(c) => c.clone(), + ExternalContractClassDiff::Created(c) => c, + ExternalContractClassDiff::Synced(c) => c, } }Also applies to: 28-33
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (23)
bin/sozo/tests/test_data/policies.json(1 hunks)crates/dojo/utils/src/tx/declarer.rs(2 hunks)crates/dojo/utils/src/tx/deployer.rs(2 hunks)crates/dojo/world/Cargo.toml(1 hunks)crates/dojo/world/src/config/profile_config.rs(5 hunks)crates/dojo/world/src/contracts/contract_info.rs(1 hunks)crates/dojo/world/src/diff/external_contract.rs(1 hunks)crates/dojo/world/src/diff/manifest.rs(4 hunks)crates/dojo/world/src/diff/mod.rs(7 hunks)crates/dojo/world/src/local/artifact_to_local.rs(8 hunks)crates/dojo/world/src/local/external_contract.rs(1 hunks)crates/dojo/world/src/local/mod.rs(3 hunks)crates/dojo/world/src/remote/events_to_remote.rs(2 hunks)crates/dojo/world/src/remote/mod.rs(1 hunks)crates/sozo/ops/src/migrate/mod.rs(6 hunks)crates/sozo/scarbext/src/workspace.rs(1 hunks)examples/spawn-and-move/Scarb.toml(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/manifest_dev.json(6 hunks)examples/spawn-and-move/src/bank.cairo(1 hunks)examples/spawn-and-move/src/erc20_token.cairo(1 hunks)examples/spawn-and-move/src/lib.cairo(1 hunks)examples/spawn-and-move/src/saloon.cairo(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/src/saloon.cairo
🧰 Additional context used
🪛 GitHub Check: codecov/patch
crates/dojo/world/src/diff/manifest.rs
[warning] 99-99: crates/dojo/world/src/diff/manifest.rs#L99
Added line #L99 was not covered by tests
crates/dojo/utils/src/tx/declarer.rs
[warning] 94-94: crates/dojo/utils/src/tx/declarer.rs#L94
Added line #L94 was not covered by tests
[warning] 156-157: crates/dojo/utils/src/tx/declarer.rs#L156-L157
Added lines #L156 - L157 were not covered by tests
[warning] 161-161: crates/dojo/utils/src/tx/declarer.rs#L161
Added line #L161 was not covered by tests
crates/sozo/ops/src/migrate/mod.rs
[warning] 415-422: crates/sozo/ops/src/migrate/mod.rs#L415-L422
Added lines #L415 - L422 were not covered by tests
[warning] 447-447: crates/sozo/ops/src/migrate/mod.rs#L447
Added line #L447 was not covered by tests
[warning] 450-453: crates/sozo/ops/src/migrate/mod.rs#L450-L453
Added lines #L450 - L453 were not covered by tests
[warning] 533-534: crates/sozo/ops/src/migrate/mod.rs#L533-L534
Added lines #L533 - L534 were not covered by tests
[warning] 536-536: crates/sozo/ops/src/migrate/mod.rs#L536
Added line #L536 was not covered by tests
[warning] 596-597: crates/sozo/ops/src/migrate/mod.rs#L596-L597
Added lines #L596 - L597 were not covered by tests
crates/dojo/utils/src/tx/deployer.rs
[warning] 58-58: crates/dojo/utils/src/tx/deployer.rs#L58
Added line #L58 was not covered by tests
[warning] 80-80: crates/dojo/utils/src/tx/deployer.rs#L80
Added line #L80 was not covered by tests
crates/dojo/world/src/diff/mod.rs
[warning] 176-178: crates/dojo/world/src/diff/mod.rs#L176-L178
Added lines #L176 - L178 were not covered by tests
[warning] 185-186: crates/dojo/world/src/diff/mod.rs#L185-L186
Added lines #L185 - L186 were not covered by tests
crates/dojo/world/src/remote/events_to_remote.rs
[warning] 146-146: crates/dojo/world/src/remote/events_to_remote.rs#L146
Added line #L146 was not covered by tests
[warning] 153-153: crates/dojo/world/src/remote/events_to_remote.rs#L153
Added line #L153 was not covered by tests
crates/dojo/world/src/local/artifact_to_local.rs
[warning] 183-184: crates/dojo/world/src/local/artifact_to_local.rs#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 190-193: crates/dojo/world/src/local/artifact_to_local.rs#L190-L193
Added lines #L190 - L193 were not covered by tests
[warning] 207-207: crates/dojo/world/src/local/artifact_to_local.rs#L207
Added line #L207 was not covered by tests
[warning] 218-218: crates/dojo/world/src/local/artifact_to_local.rs#L218
Added line #L218 was not covered by tests
[warning] 244-246: crates/dojo/world/src/local/artifact_to_local.rs#L244-L246
Added lines #L244 - L246 were not covered by tests
[warning] 251-255: crates/dojo/world/src/local/artifact_to_local.rs#L251-L255
Added lines #L251 - L255 were not covered by tests
[warning] 382-382: crates/dojo/world/src/local/artifact_to_local.rs#L382
Added line #L382 was not covered by tests
[warning] 385-385: crates/dojo/world/src/local/artifact_to_local.rs#L385
Added line #L385 was not covered by tests
🔇 Additional comments (57)
crates/dojo/world/src/config/profile_config.rs (6)
5-5: Ohayo sensei! Clean import addition here.
No issues detected, and the usage ofbailandResultfromanyhowis consistent with the new error handling further down.
15-22: Ohayo sensei! Great addition ofExternalContractConfig.
This data structure looks well-formed and easy to extend. The optionalconstructor_datafield is especially helpful for flexible contract initialization.
33-33: Ohayo sensei!
The newexternal_contractsfield neatly accommodates external contract configurations. This matches the struct you introduced and keeps the config modular.
103-129: Ohayo sensei! Validation logic is robust.
This method correctly bails out on duplicate instance names or conflicting usage ofNone. The approach with a HashMap is straightforward and performs well for typical usage.
130-132: Ohayo sensei! ReturningOk(())is succinct.
All good here; no further changes needed.
313-389: Ohayo sensei! Comprehensive test coverage.
The test scenarios cover duplicates, missing instances, and valid combinations, ensuring thevalidatemethod’s correctness. Nicely done.crates/dojo/world/src/local/artifact_to_local.rs (4)
5-19: Ohayo sensei! Imports look well-curated.
Pulling inFromStr,bail, and others are consistent with the new external contract use cases and advanced error handling.
180-206: Ohayo sensei! Handling 'Classic Starknet contract' detection is cool.
You correctly bail when the name cannot be determined. The logging provides good debugging info.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 183-184: crates/dojo/world/src/local/artifact_to_local.rs#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 190-193: crates/dojo/world/src/local/artifact_to_local.rs#L190-L193
Added lines #L190 - L193 were not covered by tests
210-258: Ohayo sensei! The external contracts creation block looks solid.
You decode constructor data, compute addresses, and trace the instance. This is straightforward and aligns with the newExternalContractConfig.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 218-218: crates/dojo/world/src/local/artifact_to_local.rs#L218
Added line #L218 was not covered by tests
[warning] 244-246: crates/dojo/world/src/local/artifact_to_local.rs#L244-L246
Added lines #L244 - L246 were not covered by tests
[warning] 251-255: crates/dojo/world/src/local/artifact_to_local.rs#L251-L255
Added lines #L251 - L255 were not covered by tests
370-386: Ohayo sensei! Clever approach for extracting the contract name from the ABI.
Scanning for the lastevent enumis an interesting approach, but ensure no unexpected ABI structure breaks it.Would you like to add negative or edge-case tests for unexpected event definitions?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 382-382: crates/dojo/world/src/local/artifact_to_local.rs#L382
Added line #L382 was not covered by tests
[warning] 385-385: crates/dojo/world/src/local/artifact_to_local.rs#L385
Added line #L385 was not covered by testscrates/sozo/ops/src/migrate/mod.rs (5)
31-31: Ohayo sensei!
Including these new diffs (ExternalContractClassDiff,ExternalContractDiff, etc.) is essential for external contract migration. Thumbs up.
102-112: Ohayo sensei!
The addition ofexternal_contracts_have_changedseamlessly integrates external contract syncing into the overall migration flow. Great approach to unify the diff checks.
402-459: Ohayo sensei! Thedeclare_classesfunction is well-structured.
You handle multiple accounts in parallel to declare classes quickly. This is a helpful performance optimization for large migrations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 415-422: crates/sozo/ops/src/migrate/mod.rs#L415-L422
Added lines #L415 - L422 were not covered by tests
[warning] 447-447: crates/sozo/ops/src/migrate/mod.rs#L447
Added line #L447 was not covered by tests
[warning] 450-453: crates/sozo/ops/src/migrate/mod.rs#L450-L453
Added lines #L450 - L453 were not covered by tests
531-531: Ohayo sensei!
Invokingdeclare_classesnicely centralizes class declaration. This DRY approach clarifies the migration pipeline.
541-541: Ohayo sensei!
Returning early withOk(has_changed)is consistent. Clear control flow.examples/spawn-and-move/src/lib.cairo (1)
2-4: Ohayo! The new module declarations look good, sensei!The addition of erc20_token, bank, and saloon modules aligns well with the external contracts management objective.
crates/dojo/world/src/local/external_contract.rs (2)
4-13: Excellent structure for ExternalContractLocal, sensei!The structure includes all necessary fields for managing external contracts as specified in the PR objectives:
- contract_name and instance_name for identification
- salt for address computation
- constructor_data in both string and Felt formats
15-20: ExternalContractClassLocal looks good too!The structure properly captures class-specific information including the CASM class hash.
crates/dojo/world/src/diff/external_contract.rs (2)
6-9: Ohayo! Clean enum design for contract class differences!The enum effectively captures the two states a contract class can be in. Good use of documentation too!
13-16: LGTM! Consistent pattern with contract differences.The enum mirrors the class diff structure, maintaining consistency in the codebase.
crates/dojo/world/src/remote/mod.rs (1)
32-35: LGTM! Clear tracking of external contract states.The new fields provide a clean way to track deployed contracts and declared classes.
crates/dojo/utils/src/tx/deployer.rs (2)
41-65: Nice separation of concerns, sensei!The new
deploy_via_udc_getcallmethod cleanly separates the deployment check from the actual deployment operation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-58: crates/dojo/utils/src/tx/deployer.rs#L58
Added line #L58 was not covered by tests
58-58: Add test coverage for deployment scenarios.Important code paths are not covered by tests:
- Line 58: Contract already deployed case
- Line 80: Noop result case
Would you like me to help generate comprehensive test cases for these scenarios?
Also applies to: 80-80
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-58: crates/dojo/utils/src/tx/deployer.rs#L58
Added line #L58 was not covered by testscrates/dojo/utils/src/tx/declarer.rs (2)
143-163: Clean implementation of declaration check!The
is_declaredfunction has good error handling and logging. However, some error paths need test coverage.Would you like me to help generate test cases for:
- ClassHashNotFound error case
- Provider error case
- Successful declaration case
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-157: crates/dojo/utils/src/tx/declarer.rs#L156-L157
Added lines #L156 - L157 were not covered by tests
[warning] 161-161: crates/dojo/utils/src/tx/declarer.rs#L161
Added line #L161 was not covered by tests
91-95: LGTM! Efficient declaration check.Good use of early return pattern when class is already declared.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 94-94: crates/dojo/utils/src/tx/declarer.rs#L94
Added line #L94 was not covered by testscrates/sozo/scarbext/src/workspace.rs (1)
119-119: Ohayo! Nice addition of validation check, sensei!Adding the validation step right after loading the configuration is a good practice to catch any issues with external contracts configuration early in the process.
crates/dojo/world/src/local/mod.rs (3)
19-19: Ohayo! Good modularization of external contract functionality!Clean separation of external contract logic into its own module with proper re-export.
Also applies to: 22-22
41-44: Well-structured addition of external contract fields, sensei!The new fields in WorldLocal provide clear separation between contract classes and instances, which is essential for managing external contracts effectively.
78-79: Clean default implementation!Proper initialization of the new fields in the Default trait implementation.
crates/dojo/world/src/contracts/contract_info.rs (1)
157-157: Ohayo! Good test coverage update, sensei!Added initialization of external_contracts in test manifest to maintain test coverage.
crates/dojo/world/src/diff/manifest.rs (4)
21-21: Clean addition to Manifest struct!The external_contracts field is properly integrated into the Manifest structure.
98-116: Well-designed ExternalContract struct, sensei!The struct has all necessary fields for managing external contracts:
- class_hash for contract identification
- contract_name and instance_name for clear differentiation
- address for deployed contract location
- abi and constructor_calldata for interaction
However, there's a gap in test coverage for this new struct.
Would you like me to help generate test cases for the ExternalContract struct to improve coverage?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 99-99: crates/dojo/world/src/diff/manifest.rs#L99
Added line #L99 was not covered by tests
162-176: Nice implementation of external contract handling!The external contract processing in Manifest::new() is well-structured:
- Properly extracts contract data
- Handles ABI retrieval from contract classes
- Maintains clean error handling
182-182: Good attention to deterministic output!Sorting external contracts by instance_name ensures consistent manifest generation.
crates/dojo/world/src/diff/mod.rs (2)
75-78: Ohayo! New fields added for external contract management.The new fields enhance the
WorldDiffstruct to track external contract classes and contracts.
176-178: Test coverage needed for external contract management, sensei.These code paths handling external contract class and contract creation are not covered by tests.
Please add test cases to cover:
- Creation of external contract classes
- Creation of external contracts
Also applies to: 185-186
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 176-178: crates/dojo/world/src/diff/mod.rs#L176-L178
Added lines #L176 - L178 were not covered by testscrates/dojo/world/src/remote/events_to_remote.rs (1)
136-157: Ohayo! New async method for loading external contract states.The implementation is clean and follows good async practices. However, some code paths need test coverage.
Please add test cases to cover:
- Loading declared external contract classes
- Loading deployed external contracts
- Error cases when contracts are not declared/deployed
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-146: crates/dojo/world/src/remote/events_to_remote.rs#L146
Added line #L146 was not covered by tests
[warning] 153-153: crates/dojo/world/src/remote/events_to_remote.rs#L153
Added line #L153 was not covered by testsexamples/spawn-and-move/Scarb.toml (1)
16-16: Ohayo! OpenZeppelin dependency added.Using a specific version (0.20.0) is good for reproducibility.
✅ Verification successful
🌐 Web query:
What is the latest version of OpenZeppelin for Cairo/Starknet?Length of output: 3274
Ohayo, sensei! The version specified in
examples/spawn-and-move/Scarb.tomlis set to v0.20.0, which matches the latest release for OpenZeppelin Contracts for Cairo/Starknet (released on December 6, 2024). Everything looks spot-on for reproducibility. Great work!crates/dojo/world/Cargo.toml (1)
13-13: Ohayo! Dojo-utils dependency enabled.The workspace dependency is now active, which is needed for the new external contract management features.
examples/spawn-and-move/dojo_dev.toml (4)
26-31: Ohayo, sensei! The configuration for the GoldToken entry is well-structured. It properly declares the external ERC20Token with an explicit instance name, salt, and a detailed constructor_data array. Just make sure that the values (e.g., the token name, symbol, and initial supply) precisely match the token’s constructor signature.
32-37: Ohayo, sensei! The WoodToken entry mirrors the GoldToken entry nicely. The instance name, salt, and constructor data are consistent with what’s expected. Please verify that using the same salt and initial supply is intentional for both tokens.
38-42: Ohayo, sensei! The Bank contract is defined correctly. Notice that no explicit instance_name is provided—this should default to the contract_name ("Bank"). Confirm that this behavior is exactly what you intended per the documentation.
43-47: Ohayo, sensei! The Saloon contract is added with an empty constructor_data array, which is acceptable when no constructor arguments are needed. Just double-check that this omission is intentional for the Saloon’s design.bin/sozo/tests/test_data/policies.json (3)
3-49: Ohayo, sensei! The first group of policy entries now uses the updated method names—such as "upgrade", "spawn", "move", "set_player_config", "update_player_config_name", "reset_player_config", "set_player_server_profile", "set_models", "enter_dungeon", and "upgrade"—which align perfectly with the new external contracts management enhancements. This consistency in the first batch is spot on!
90-113: Ohayo, sensei! The policy entries for actions like "emit_event", "emit_events", "set_entity", "set_entities", "delete_entity", and "delete_entities" appear consistent and remain unchanged. These look good as long as they match the corresponding contract interfaces.
114-129: Ohayo, sensei! The entries handling ownership and writer configurations (e.g., "grant_owner", "revoke_owner", "grant_writer", "revoke_writer") are intact and consistent. Nice work ensuring these critical permissions remain unaltered.examples/spawn-and-move/manifest_dev.json (11)
2-29: Ohayo, sensei! The "world" section has been updated with a new class_hash and address. The entrypoints array remains consistent with previous definitions. Please double-check that the new world address correctly reflects the deployed on-chain contract and that the updated class_hash is as expected.
1253-1607: Ohayo, sensei! The "ns-actions" contract is now updated with a new address, class_hash, and ABI. Notably, the systems array now lists actions like "spawn", "move", "set_player_config", "update_player_config_name", "reset_player_config", "set_player_server_profile", "set_models", "enter_dungeon", and "upgrade". This aligns with the new external contracts management changes. Just verify that the ABI modifications fully support the updated interfaces.
1608-1801: Ohayo, sensei! The "ns-dungeon" contract has been updated accordingly. The changes beginning at line 1609—including the new address and any ABI modifications—ensure that the dungeon functionalities integrate properly into the system. Make sure that these interface updates support the new asynchronous deployment checks.
1802-1977: Ohayo, sensei! The "ns-mock_token" contract now reflects a new address and class_hash with a robust ABI for ERC20-like behavior. Ensure that the selector and systems for this contract fit within the overall network’s expectations and that the ABI changes are backward compatible with existing tests.
1978-2160: Ohayo, sensei! The "ns-others" contract details are updated with a new address, class_hash, and systems list, including an init_calldata value of "0xff". Please confirm that this initialization value is intentional and that the updated configuration correctly supports the intended operations for this module.
2163-2211: Ohayo, sensei! Several new model entries—such as "ns-Flatbow", "ns-Message", "ns-MockToken", "ns-Moves", "ns-PlayerConfig", "ns-Position", and "ns-RiverSkale"—have been added with updated class_hash values and selectors. Please check that each model is uniquely defined and correctly referenced by the corresponding contracts and systems.
2212-2225: Ohayo, sensei! The events section now includes updated entries like "ns-Moved" and "ns-MyInit". Their new class_hash values and selectors look correct. Just ensure these events are properly emitted by their respective contracts during runtime.
2226-2253: Ohayo, sensei! The external contract entry for "Bank" is registered correctly with its class_hash, contract_name, instance_name, address, and ABI—including the constructor and event definitions. This entry properly follows the updated guidelines for external contract management.
2254-2614: Ohayo, sensei! The external contract for the ERC20Token instance "GoldToken" is well-defined. The ABI, constructor_calldata (with the token name, symbol, initial supply, and owner address), and other details align with the updated ERC20 integration. Double-check the numeric values and hexadecimal representations to avoid any overflow or formatting issues.
2615-2634: Ohayo, sensei! The Saloon contract is registered succinctly with an empty constructor_calldata array, indicating it requires no constructor parameters. The ABI and class_hash should be cross-checked with the actual deployed contract to ensure consistency.
2635-2996: Ohayo, sensei! The external contract for the ERC20Token instance "WoodToken" is properly configured, mirroring the setup for GoldToken. Its ABI and constructor_calldata are clearly specified. Please confirm that the address and constructor parameters are accurate and consistent with the project’s overall ERC20Token specifications.
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "set_entity" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "uuid" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "set_entities" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "set_metadata" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "delete_entity" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "register_namespace" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "delete_entities" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "register_event" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "grant_owner" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "register_model" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "revoke_owner" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "register_contract" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "grant_writer" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "init_contract" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "revoke_writer" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "upgrade_event" | ||
| }, | ||
| { | ||
| "target": "0x736c456dec44741fad1940876e62342a626ca73fd4ac574a94eb3ded2e25d8d", | ||
| "method": "upgrade" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "upgrade_model" | ||
| }, | ||
| { | ||
| "target": "0xe1fb33e10629d61ac3eb7c9bdca6a0ce947bcf7ecf598fbc0c42f9c76b3808", | ||
| "method": "upgrade" | ||
| "target": "0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2", | ||
| "method": "upgrade_contract" | ||
| }, |
There was a problem hiding this comment.
Ohayo, sensei! I’ve noticed that the policies for the target address
"0x5baea2d83fc19bae80dc5d4626a27b2b2d5012822cd862c56ed7007eb92eaa2"
still show legacy method names such as "uuid", "set_metadata", "register_namespace", "register_event", "register_model", "register_contract", "init_contract", "upgrade_event", "upgrade_model", and "upgrade_contract". This does not match the updated naming conventions outlined in the PR summary. Please review these entries and update them to the new names (e.g., "uuid" → "upgrade", "set_metadata" → "spawn", etc.) to maintain consistency across the system.
crates/sozo/ops/src/migrate/mod.rs
Outdated
| let external_contracts_have_changed = self.sync_external_contracts(ui).await?; | ||
|
|
There was a problem hiding this comment.
For now that this task is totally independent, let move it before the world or after the initialization of the contracts.
This may change later if we implement the registration of the tokens, since registering a token requires the permissions to be set. (So it may be placed between permissions and initialization of contracts (which may use a token during this phase, but this is for later)
| contract.class_hash, | ||
| contract.salt, | ||
| &contract.raw_constructor_data, |
There was a problem hiding this comment.
If two contracts have different instance name, but same contract name, same salt and same constructor arguments, it will fail because the address doesn't take in account the instance name.
You could hash the instance name and the salt to have the final salt. This ensures unicity based on the instance name, which is what instance name is for from what I understand. 👍
There was a problem hiding this comment.
Yes, good idea! Thanks 🙏
remybar
force-pushed
the
feat-core-starknet_contract_support
branch
from
88586b0 to
baa87b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
examples/spawn-and-move/src/lib.cairo (1)
2-4: Ohayo! The new modules look well-organized, sensei!The addition of
erc20_token,bank, andsaloonmodules aligns well with the external contracts management enhancement.Consider grouping related modules together:
pub mod actions; +pub mod models; +pub mod utils; pub mod erc20_token; pub mod bank; pub mod saloon; -pub mod models; -pub mod utils; pub mod others; pub mod mock_token;examples/spawn-and-move/manifest_dev.json (2)
2615-2634: The Saloon contract needs more implementation details, sensei!The Saloon contract is currently empty with no storage or functions defined. This might need to be implemented before it can be useful in the system.
Would you like me to help generate a basic implementation for the Saloon contract with common gaming functionalities?
2635-2995: The Wood token configuration mirrors the Gold token nicely!The Wood token shares the same structure and initial supply as the Gold token, which provides consistency.
Consider implementing a token factory pattern if more resource tokens will be added in the future, as this would streamline the deployment process.
crates/dojo/world/src/diff/manifest.rs (1)
98-116: Ohayo! Consider adding more documentation for ExternalContract, sensei!While the struct is well-designed, it would benefit from more detailed documentation explaining each field's purpose and constraints.
Add documentation like this:
#[serde_as] #[derive(Clone, Default, Debug, Serialize, Deserialize)] +/// Represents an external contract in the manifest. +/// +/// # Fields +/// * `class_hash` - The class hash of the deployed contract +/// * `contract_name` - The name of the contract class +/// * `instance_name` - A unique identifier for this contract instance +/// * `address` - The deployed contract's address +/// * `abi` - The contract's ABI +/// * `constructor_calldata` - Optional constructor arguments pub struct ExternalContract {crates/dojo/world/src/config/profile_config.rs (1)
96-132: Consider optimizing the validation logic!While the validation logic is thorough, it could be more efficient by avoiding multiple iterations.
Here's an optimized version:
pub fn validate(&self) -> Result<()> { if let Some(contracts) = &self.external_contracts { - let mut map = HashMap::<String, Vec<Option<String>>>::new(); + let mut map = HashMap::<String, HashSet<Option<String>>>::new(); for contract in contracts { - map.entry(contract.contract_name.clone()) - .or_default() - .push(contract.instance_name.clone()); + let instances = map.entry(contract.contract_name.clone()).or_default(); + if !instances.insert(contract.instance_name.clone()) { + bail!( + "There are duplicated instance names for the external contract name '{}'", + contract.contract_name + ); + } } for (contract_name, instance_names) in map { if instance_names.len() > 1 && instance_names.contains(&None) { bail!( "There must be only one [[external_contracts]] block in the profile \ config mentioning the contract name '{}' without instance name.", contract_name ); } - - let set: HashSet<_> = instance_names.iter().cloned().collect(); - if set.len() != instance_names.len() { - bail!( - "There are duplicated instance names for the external contract name '{}'", - contract_name - ); - } } } Ok(()) }crates/dojo/world/src/local/artifact_to_local.rs (1)
191-195: Enhance the error message for better debugging, sensei.The error message could be more descriptive by including additional context about why the contract name couldn't be found.
- bail!( - "Unable to find the name of the contract in the file {}", - path.file_name().unwrap().to_str().unwrap() - ); + bail!( + "Unable to find the contract name in file {}. Ensure the contract has a valid \ + event enum declaration in its ABI.", + path.file_name() + .and_then(|f| f.to_str()) + .unwrap_or("<unknown>") + );crates/dojo/world/src/diff/mod.rs (1)
75-78: Add documentation for the new fields, sensei.Consider adding detailed documentation for the new fields to explain their purpose and usage:
- /// The external contract classes. + /// A mapping of contract names to their diff status, tracking changes in external contract + /// classes between local and remote states. pub external_contract_classes: HashMap<String, ExternalContractClassDiff>, - /// The external contracts. + /// A list of external contract instances and their diff status, tracking deployment + /// and initialization states between local and remote environments. pub external_contracts: Vec<ExternalContractDiff>,crates/dojo/world/src/remote/events_to_remote.rs (1)
136-157: Enhance error handling for external contract states, sensei.While the implementation is clean, consider improving error handling to provide more context when operations fail.
pub async fn load_external_contract_states<P: Provider>( &mut self, provider: &P, external_contract_classes: Vec<(String, Felt)>, external_contracts: HashMap<String, Felt>, ) -> Result<()> { for (name, hash) in external_contract_classes { - if dojo_utils::is_declared(&name, hash, provider).await? { + match dojo_utils::is_declared(&name, hash, provider).await { + Ok(true) => { trace!(name, "External contract class already declared."); self.declared_external_contract_classes.push(name); + } + Ok(false) => { + trace!(name, "External contract class not declared."); + } + Err(e) => { + return Err(anyhow!("Failed to check if contract class '{}' is declared: {}", name, e)); + } } } for (name, address) in external_contracts { - if dojo_utils::is_deployed(address, provider).await? { + match dojo_utils::is_deployed(address, provider).await { + Ok(true) => { trace!(name, "External contract already deployed."); self.deployed_external_contracts.push(name); + } + Ok(false) => { + trace!(name, "External contract not deployed."); + } + Err(e) => { + return Err(anyhow!("Failed to check if contract '{}' is deployed: {}", name, e)); + } } } Ok(()) }crates/sozo/ops/src/migrate/mod.rs (1)
403-459: Nice parallel declaration implementation, sensei! Consider some improvements.The parallel declaration logic is well-implemented, but could benefit from:
- More detailed error messages
- Better tracing for debugging
async fn declare_classes( &self, ui: &mut MigrationUi, classes: HashMap<Felt, LabeledClass>, ) -> Result<(), MigrationError<A::SignError>> { let accounts = self.get_accounts().await; let n_classes = classes.len(); if accounts.is_empty() { - trace!("Declaring classes with migrator account."); + trace!( + account = ?self.world.account.address(), + "Declaring {} classes with migrator account", + n_classes + ); let mut declarer = Declarer::new(&self.world.account, self.txn_config); declarer.extend_classes(classes.into_values().collect()); let ui_text = format!("Declaring {} classes...", n_classes); ui.update_text_boxed(ui_text); declarer.declare_all().await?; } else { - trace!("Declaring classes with {} accounts.", accounts.len()); + trace!( + n_accounts = accounts.len(), + n_classes, + "Declaring classes using multiple accounts" + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (23)
bin/sozo/tests/test_data/policies.json(2 hunks)crates/dojo/utils/src/tx/declarer.rs(2 hunks)crates/dojo/utils/src/tx/deployer.rs(2 hunks)crates/dojo/world/Cargo.toml(1 hunks)crates/dojo/world/src/config/profile_config.rs(5 hunks)crates/dojo/world/src/contracts/contract_info.rs(1 hunks)crates/dojo/world/src/diff/external_contract.rs(1 hunks)crates/dojo/world/src/diff/manifest.rs(4 hunks)crates/dojo/world/src/diff/mod.rs(7 hunks)crates/dojo/world/src/local/artifact_to_local.rs(8 hunks)crates/dojo/world/src/local/external_contract.rs(1 hunks)crates/dojo/world/src/local/mod.rs(3 hunks)crates/dojo/world/src/remote/events_to_remote.rs(2 hunks)crates/dojo/world/src/remote/mod.rs(1 hunks)crates/sozo/ops/src/migrate/mod.rs(5 hunks)crates/sozo/scarbext/src/workspace.rs(1 hunks)examples/spawn-and-move/Scarb.toml(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/manifest_dev.json(1 hunks)examples/spawn-and-move/src/bank.cairo(1 hunks)examples/spawn-and-move/src/erc20_token.cairo(1 hunks)examples/spawn-and-move/src/lib.cairo(1 hunks)examples/spawn-and-move/src/saloon.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/dojo/world/Cargo.toml
- examples/spawn-and-move/Scarb.toml
- crates/dojo/world/src/contracts/contract_info.rs
- examples/spawn-and-move/src/bank.cairo
- examples/spawn-and-move/src/saloon.cairo
- crates/dojo/world/src/remote/mod.rs
- crates/sozo/scarbext/src/workspace.rs
- crates/dojo/world/src/local/external_contract.rs
- examples/spawn-and-move/src/erc20_token.cairo
- crates/dojo/utils/src/tx/declarer.rs
- crates/dojo/world/src/diff/external_contract.rs
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (17)
examples/spawn-and-move/manifest_dev.json (2)
2226-2253: Ohayo! Let's review the Bank contract configuration, sensei!The Bank contract is properly configured with an owner address. However, using the same owner address for multiple contracts might need consideration from a security perspective.
Consider documenting the owner address management strategy and verifying if this is intended for development only:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Find all occurrences of the owner address to verify its usage rg -A 2 "0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba"Length of output: 8455
Ohayo, sensei! Based on the search results, the owner address
0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba
is used consistently across multiple files in the repository (in tests, development manifests, and configuration files). This widespread usage strongly suggests it is a deliberate choice for development and testing purposes.It would be beneficial to add documentation detailing the owner address strategy—especially clarifying that this address is intended for development and should be replaced or managed differently for production.
2254-2614: The ERC20Token implementation looks solid!The Gold token is well-configured with:
- Name: "Gold"
- Symbol: "GOLD"
- Initial supply: 0x10000000000000
However, let's verify if the initial supply is appropriate for the use case:
crates/dojo/utils/src/tx/deployer.rs (2)
40-65: Ohayo! Nice separation of concerns in the deployment process, sensei!The new
deploy_via_udc_getcallmethod effectively separates the deployment check from the actual deployment process, making the code more modular and reusable.
67-81: Clean refactoring of deploy_via_udc, sensei!The updated method now handles the deployment check result elegantly, returning
TransactionResult::Noopwhen no deployment is needed.crates/dojo/world/src/local/mod.rs (3)
19-19: Ohayo! Clean module organization, sensei!Good addition of the external contract module and its public re-export.
Also applies to: 22-22
41-44: Well-documented fields for external contract management!The new fields are clearly documented and follow the existing pattern.
78-79: Clean Default implementation update!The Default trait implementation is properly updated to initialize the new fields.
crates/dojo/world/src/diff/manifest.rs (2)
162-176: Clean external contract processing logic!The implementation correctly handles the contract data extraction and ABI mapping.
182-182: Good sorting for deterministic output!Maintaining deterministic output by sorting external contracts is a good practice.
crates/dojo/world/src/config/profile_config.rs (2)
15-22: Ohayo! Well-structured config struct, sensei!The ExternalContractConfig struct is well-designed with clear field names and appropriate optionality.
314-389: Excellent test coverage!The test cases thoroughly cover various validation scenarios including edge cases.
crates/dojo/world/src/local/artifact_to_local.rs (1)
225-228: Nice implementation of the salt computation, sensei!The salt computation correctly incorporates both the instance name and the provided salt, ensuring uniqueness even when contracts share the same name and constructor arguments.
crates/dojo/world/src/diff/mod.rs (1)
171-187: Clean implementation of external contract diffing, sensei!The comparison logic for external contracts follows the same pattern as other resources, making the code consistent and maintainable.
crates/sozo/ops/src/migrate/mod.rs (1)
546-604: Well-structured external contract synchronization, sensei!The implementation correctly handles both declaration and deployment phases, and follows the suggested pattern of moving external contract operations to a separate phase.
bin/sozo/tests/test_data/policies.json (3)
3-3: Target Address Update for "upgrade"
Ohayo, sensei! Theupgrademethod's target address has been updated to0xca72f1cd782b614fa12c8b54afa895a169a4de1792738d4e3f09d0929f7834. Please double-check that this new address correctly reflects the external contract endpoint as outlined in the PR objectives.
43-44: Upgrade Method Target Modification
Ohayo, sensei! Theupgrademethod now uses the target address0x608ffd2e6b74a7bede256770ebe3d07bc65c79622e6a9396ea764011152102. Ensure that this address is aligned with the updated external contracts management strategy and that any corresponding deployment or upgrade logic is adjusted accordingly.
47-48: Validation of Target Address Reversion
Ohayo, sensei! The target address for theupgrademethod is updated here to0x780e3207b4f11b56f32cc0f19975af5b3a4df3cad6a4b0ab59a1702ba0304d8. This change appears to revert to a previously used address. Please confirm that this is intentional within the new upgrade cycle and that it won’t lead to any conflicts during deployment.
| fn contract_name_from_abi(abi: &[AbiEntry]) -> Option<String> { | ||
| for entry in abi.iter().rev() { | ||
| if let AbiEntry::Event(AbiEvent::Typed(TypedAbiEvent::Enum(e))) = entry { | ||
| let mut it = e.name.rsplit("::"); | ||
| let _ = it.next(); | ||
| return Some(it.next().unwrap().to_string()); | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Consider making the contract name extraction more robust, sensei.
The current implementation has a few potential issues:
- Assumes the last event enum always contains the contract name
- Uses unwrap which could panic if the split iterator is empty
Consider this safer implementation:
fn contract_name_from_abi(abi: &[AbiEntry]) -> Option<String> {
for entry in abi.iter().rev() {
if let AbiEntry::Event(AbiEvent::Typed(TypedAbiEvent::Enum(e))) = entry {
let mut it = e.name.rsplit("::");
- let _ = it.next();
- return Some(it.next().unwrap().to_string());
+ if let (Some(_), Some(name)) = (it.next(), it.next()) {
+ return Some(name.to_string());
+ }
}
}
None
}📝 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.
| fn contract_name_from_abi(abi: &[AbiEntry]) -> Option<String> { | |
| for entry in abi.iter().rev() { | |
| if let AbiEntry::Event(AbiEvent::Typed(TypedAbiEvent::Enum(e))) = entry { | |
| let mut it = e.name.rsplit("::"); | |
| let _ = it.next(); | |
| return Some(it.next().unwrap().to_string()); | |
| } | |
| } | |
| None | |
| } | |
| fn contract_name_from_abi(abi: &[AbiEntry]) -> Option<String> { | |
| for entry in abi.iter().rev() { | |
| if let AbiEntry::Event(AbiEvent::Typed(TypedAbiEvent::Enum(e))) = entry { | |
| let mut it = e.name.rsplit("::"); | |
| if let (Some(_), Some(name)) = (it.next(), it.next()) { | |
| return Some(name.to_string()); | |
| } | |
| } | |
| } | |
| None | |
| } |
| fn external_contract_classes( | ||
| &self, | ||
| contract_class: &ExternalContractClassDiff, | ||
| ) -> Option<(Felt, LabeledClass)> { | ||
| match contract_class { | ||
| ExternalContractClassDiff::Created(c) => Some(( | ||
| c.casm_class_hash, | ||
| LabeledClass { | ||
| label: c.contract_name.clone(), | ||
| casm_class_hash: c.casm_class_hash, | ||
| class: c.class.clone().flatten().unwrap(), | ||
| }, | ||
| )), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors in class flattening, sensei.
The unwrap on class flattening could be replaced with proper error handling.
fn external_contract_classes(
&self,
contract_class: &ExternalContractClassDiff,
- ) -> Option<(Felt, LabeledClass)> {
+ ) -> Option<Result<(Felt, LabeledClass), anyhow::Error>> {
match contract_class {
ExternalContractClassDiff::Created(c) => Some((
c.casm_class_hash,
LabeledClass {
label: c.contract_name.clone(),
casm_class_hash: c.casm_class_hash,
- class: c.class.clone().flatten().unwrap(),
+ class: c.class.clone().flatten()?,
},
)),
_ => None,
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/world/src/local/artifact_to_local.rs (2)
36-36: Ohayo! Consider adding error context for better debugging, sensei.While the external contract handling is well-structured, the error messages could be more informative.
Consider enhancing the error context:
- bail!( - "Unable to find the name of the contract in the file {}", - path.file_name().unwrap().to_str().unwrap() - ); + bail!( + "Unable to find the name of the contract in file '{}'. Ensure the contract \ + has a properly defined event enum.", + path.file_name().unwrap().to_str().unwrap() + );Also applies to: 77-78, 181-206
211-263: Ohayo! Consider validating constructor data earlier, sensei.The constructor data validation happens deep in the processing loop. Moving it earlier could prevent unnecessary processing.
Consider validating constructor data before processing:
if let Some(contracts) = &profile_config.external_contracts { for contract in contracts { + // Validate constructor data early + let raw_constructor_data = if let Some(data) = &contract.constructor_data { + match decode_calldata(data) { + Ok(data) => data, + Err(e) => bail!( + "Invalid constructor data for contract '{}': {}", + contract.contract_name, e + ), + } + } else { + vec![] + }; + if let Some(local_class) = external_contract_classes.get(&contract.contract_name) { - let raw_constructor_data = if let Some(data) = &contract.constructor_data { - decode_calldata(data)? - } else { - vec![] - };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/world/src/local/artifact_to_local.rs(8 hunks)
🔇 Additional comments (2)
crates/dojo/world/src/local/artifact_to_local.rs (2)
6-17: LGTM! Clean import organization, sensei.The new imports are well-organized and properly scoped for the enhanced functionality.
376-392: The contract name extraction implementation looks good!The function follows the suggested robust implementation from the previous review.
Description
To be managed by sozo, external Cairo contracts (i.e non Dojo contracts) must be declared in your
dojo_<profile>.tomlfile. For each contract instance a[[external_contracts]]must be created with:contract_name: the name of the Cairo contract to deploy,instance_name: if you want to deploy several instances of a same Cairo contract (for example, ERC20), you have to give a specific name to each instance. If you have only one instance, don't provide any value forinstance_name, it will automatically be set to thecontract_namevalue,salt: salt value to use to compute the contract address,constructor_data: a list of calldata to give to the Cairo contract constructor. Must use the Dojo calldata format. If the constructor does not have any parameter, just omit this parameter.Then, sozo will declare and deploy these external contracts and update the manifest output file.
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
New Features
Enhancements