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 56.96% 56.60% -0.36%
==========================================
Files 430 432 +2
Lines 57052 57850 +798
==========================================
+ Hits 32500 32748 +248
- Misses 24552 25102 +550 ☔ View full report in Codecov by Sentry. |
|
Ohayo, sensei! Here’s the detailed breakdown of the changes: WalkthroughMultiple modifications were introduced across the repository. In configuration files, target addresses for upgrade methods were updated. In the TX modules, a new asynchronous declaration check and a refined deployment flow using UDC were added. The world modules were updated to support external contract types through new structs, enums, and fields in both local and remote contexts. Migration logic was extended to handle external contracts, and command-line inspection and error handling were improved. Finally, several new external contract definitions and dependencies (ERC20, ERC721, ERC1155, Bank, Saloon) were added to the spawn-and-move examples. Changes
Suggested labels
🪧 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.
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.
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.
There was a problem hiding this comment.
Could you add a mint function to the ERC20 (the OZ standard doesn't expose it by default), which is useful to test dojo, something like this:
#[abi(per_item)]
#[generate_trait]
impl ERC20DemoTrait of ERC20Demo {
#[external(v0)]
fn mint(ref self: ContractState, amount: u256) {
self.erc20.mint(starknet::get_caller_address(), amount);
}
}The sozo inspect command is not showing those contracts, would you mind adding them? Checking in the manifest works well for addresses, but having them in the sozo inspect could give quicker view of the state of each contract.
Finally, currently sozo execute doesn't support world, we may add it + the support for the external contract since those are without namespaces for now. We can use the instance_name to match, wdyt?
This will ease interactions with ERCs. :)
remybar
force-pushed
the
feat-core-starknet_contract_support
branch
from
3c09be3 to
6f45808
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/sozo/ops/src/migrate/mod.rs (1)
542-604: 🛠️ Refactor suggestionAdd error handling for deployment failures.
The deployment of external contracts via UDC could fail for various reasons (e.g., insufficient funds, invalid constructor arguments). Consider adding specific error handling for deployment failures.
if let Some((_, call)) = deployer .deploy_via_udc_getcall( contract.class_hash, contract.salt, &contract.raw_constructor_data, Felt::ZERO, ) .await? { + trace!( + contract_name = contract.contract_name, + instance_name = contract.instance_name, + "Deploying external contract" + ); invoker.add_call(call); +} else { + return Err(MigrationError::DeploymentError { + contract_name: contract.contract_name.clone(), + instance_name: contract.instance_name.clone(), + }); }
🧹 Nitpick comments (10)
crates/dojo/utils/src/tx/deployer.rs (2)
40-65: Ohayo! Nice separation of concerns sensei!The extraction of deployment check logic into a separate function improves modularity and reusability. The Option return type elegantly handles the case when deployment is not needed.
This pattern could be reused for other contract operations that need similar pre-execution checks.
113-129: Consider enhancing the trace logging sensei!The error case could benefit from trace logging to help with debugging deployment issues.
match provider.get_class_hash_at(BlockId::Tag(BlockTag::Pending), contract_address).await { - Err(ProviderError::StarknetError(StarknetError::ContractNotFound)) => Ok(false), + Err(ProviderError::StarknetError(StarknetError::ContractNotFound)) => { + trace!( + contract_address = format!("{:#066x}", contract_address), + "Contract not deployed." + ); + Ok(false) + },crates/dojo/utils/src/tx/declarer.rs (1)
142-163: Consider optimizing the block tag usage sensei!The
BlockId::Tag(BlockTag::Pending)could be defined as a constant to avoid repeated construction.+const PENDING_BLOCK: BlockId = BlockId::Tag(BlockTag::Pending); + pub async fn is_declared<P>( class_name: &String, class_hash: Felt, provider: &P, ) -> Result<bool, ProviderError> where P: Provider, { - match provider.get_class(BlockId::Tag(BlockTag::Pending), class_hash).await { + match provider.get_class(PENDING_BLOCK, class_hash).await {examples/spawn-and-move/src/erc20_token.cairo (2)
25-35: Consider adding input validation, sensei!While the constructor implementation is correct, consider adding validation for:
- Non-zero initial supply
- Non-zero recipient address
fn constructor( ref self: ContractState, name: ByteArray, symbol: ByteArray, initial_supply: u256, recipient: ContractAddress, ) { + // Validate inputs + assert(initial_supply > 0, 'Initial supply must be > 0'); + assert(recipient != 0, 'Invalid recipient'); self.erc20.initializer(name, symbol); self.erc20.mint(recipient, initial_supply); }
1-36: Consider expanding token support, sensei!As suggested by glihm-sensei, consider adding support for ERC1155 and ERC721 tokens to enhance the framework's capabilities. This would enable:
- NFT support through ERC721
- Multi-token support through ERC1155
- Better testing with Torii indexing
examples/spawn-and-move/manifest_dev.json (1)
2226-2634: Configuration looks good, sensei! Consider adding documentation.The external contracts configuration is well-structured with all required fields. Consider adding comments to document:
- Purpose of each contract
- Relationship between contracts
- Expected token flows
bin/sozo/src/commands/inspect.rs (2)
42-43: Ohayo sensei!
Usinginspect_elementwhenelementis provided is a clear approach. Consider adding a quick log or debug statement if the element isn't found, to assist in troubleshooting.
454-466: Ohayo sensei!
Returning aStringfromstatus()is perfectly valid. If you ever need more structured handling of statuses later, consider using an enum or returning&'static str.crates/dojo/world/src/local/artifact_to_local.rs (1)
181-206: Ohayo! Consider making the trace logging more robust, sensei.The trace logging at line 184 uses unwrap which could panic if the path doesn't have a valid file name.
- trace!( - filename = path.file_name().unwrap().to_str().unwrap(), - "Classic Starknet contract." - ); + trace!( + filename = path.file_name() + .and_then(|f| f.to_str()) + .unwrap_or("unknown"), + "Classic Starknet contract." + );crates/sozo/ops/src/migrate/mod.rs (1)
403-459: Ohayo! Consider improving error handling for class declarations, sensei.The error handling for class declarations could be more informative. Currently, it only forwards the error message as a string.
Consider creating specific error variants in
MigrationErrorfor different class declaration failure cases:pub enum MigrationError<E> { DeclareClassError(String), + ClassAlreadyDeclared(String), + ClassDeclarationFailed { class_hash: String, reason: String }, }Then update the error handling:
- return Err(MigrationError::DeclareClassError(e.to_string())); + if e.to_string().contains("Class already declared") { + return Err(MigrationError::ClassAlreadyDeclared(e.to_string())); + } + return Err(MigrationError::ClassDeclarationFailed { + class_hash: format!("{:#x}", labeled_class.casm_class_hash), + reason: e.to_string(), + });
📜 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 (24)
bin/sozo/src/commands/inspect.rs(8 hunks)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 (15)
- crates/dojo/world/Cargo.toml
- examples/spawn-and-move/Scarb.toml
- examples/spawn-and-move/src/saloon.cairo
- crates/dojo/world/src/remote/mod.rs
- crates/sozo/scarbext/src/workspace.rs
- crates/dojo/world/src/contracts/contract_info.rs
- crates/dojo/world/src/local/mod.rs
- crates/dojo/world/src/diff/manifest.rs
- examples/spawn-and-move/src/bank.cairo
- examples/spawn-and-move/src/lib.cairo
- crates/dojo/world/src/local/external_contract.rs
- crates/dojo/world/src/remote/events_to_remote.rs
- crates/dojo/world/src/diff/mod.rs
- examples/spawn-and-move/dojo_dev.toml
- bin/sozo/tests/test_data/policies.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (22)
crates/dojo/utils/src/tx/deployer.rs (1)
67-81: Clean refactoring sensei!The integration with
deploy_via_udc_getcallis well done, maintaining the existing functionality while improving code organization.crates/dojo/utils/src/tx/declarer.rs (1)
91-95: Nice consistency with deployer pattern sensei!The integration with
is_declaredfollows the same pattern as the deployer module, making the codebase more consistent and maintainable.examples/spawn-and-move/src/erc20_token.cairo (1)
1-24: Ohayo! Clean and secure implementation, sensei!The ERC20 token implementation follows best practices by:
- Using OpenZeppelin's battle-tested ERC20 component
- Properly structuring storage and events
- Following standard ERC20 patterns
examples/spawn-and-move/manifest_dev.json (1)
2254-2994: Verify token configuration intent, sensei!Both GoldToken and WoodToken share:
- Same class_hash:
0x1c2a30a51500465288c00a56a4d698bdae3c7f7e105f25cb36106db8de128b8- Same initial supply:
0x10000000000000- Same recipient:
0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bbaPlease verify if this is intentional or if they should have different parameters.
bin/sozo/src/commands/inspect.rs (8)
5-5: Ohayo sensei!
These imports look fine for integrating external contract diffs into the inspection logic. Great job adding theExternalContractDiffreference here.
20-22: Ohayo sensei!
Renaming this argument toelementclearly communicates that it could be a resource or an external contract instance name. Nicely done.
36-36: Ohayo sensei!
Destructuring the newelementfield is straightforward and helps keep therunmethod clean.
138-147: Ohayo sensei!
TheExternalContractInspectstruct is well-structured. All relevant fields (contract name, salt, etc.) are neatly included for display. No issues spotted.
179-194: Ohayo sensei!
Splitting the logic between resource inspection and external contract inspection is an elegant solution. One subtle behavior is that if there's both a matching selector and a matching external contract name, the resource logic will take precedence. Make sure this condition aligns with your intended usage.
263-269: Ohayo sensei!
Printing external contract details in TOML format is consistent with how resources are handled. This is nicely consistent.
493-496: Ohayo sensei!
This minimal helper for section headers is a neat way to give each section of output a distinct visual highlight. Looks good!
502-502: Ohayo sensei!
Callingprint_section_header(line)when encountering a TOML heading is a neat approach that maintains readability in the console output.crates/dojo/world/src/diff/external_contract.rs (1)
1-42: Ohayo sensei!
Introducing distinct enums for external contract classes (ExternalContractClassDiff) and contracts (ExternalContractDiff) is well-thought-out, enabling clear differentiation between newly added and synced states. Methods likeclass_data()andcontract_data()neatly return the local definitions for downstream use. Returning a plainStringfromstatus()is fine for now; consider using an enum if you need more structured states in the future.crates/dojo/world/src/config/profile_config.rs (5)
5-6: Ohayo sensei!
Bringing inbailfrom theanyhowcrate and referencingnamingfromdojo_typessets the stage for robust validation in this file. Good move.
16-23: Ohayo sensei!
TheExternalContractConfigstruct neatly captures external contract configuration (contract name, instance name, salt, constructor parameters). This reduces confusion and centralizes external contract settings.
34-34: Ohayo sensei!
Addingexternal_contractstoProfileConfigprovides a comprehensive place to track these definitions in the config. Looks good.
96-142: Ohayo sensei!
Your validation logic smartly covers the main pitfalls: multiple blocks for the same contract without a defined instance name, duplicated instance names, and invalid instance names. This ensures a clean configuration. Nicely done!
323-445: Ohayo sensei!
Thetest_profile_config_validationtest suite thoroughly checks duplicates, missing names, and invalid instance names. This is an excellent example of well-rounded test coverage.crates/dojo/world/src/local/artifact_to_local.rs (3)
225-228: Consider improving salt computation for better uniqueness.Based on past review comments, the salt computation should include the instance name to ensure uniqueness when two contracts have different instance names but the same contract name and constructor arguments.
279-290: LGTM! Clean initialization of new fields.The WorldLocal struct initialization properly includes the new external contract fields with good error handling for missing world artifact.
376-392: Verify the assumption about event enum naming convention.The implementation assumes that the last event enum in the ABI always contains the contract name. While this might be true for the current implementation, it could be fragile if the ABI structure changes.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify if all contract ABIs follow the same event enum naming pattern # Search for event enums in contract ABIs and check their naming pattern # Find all contract ABI files fd --extension json --exec sh -c ' echo "Checking file: {}" jq -r ".abi[] | select(.type == \"event\" and .kind == \"enum\") | .name" {} 2>/dev/null 'Length of output: 5474
Ohayo, sensei! The event enum naming convention holds up.
The scan of contract ABI files confirms that the naming pattern consistently follows the expected format (e.g., "
test_contract::test_contract::TestContract::Event" returns "TestContract"). The function correctly assumes that the contract name is the second-to-last component when splitting by "::".While this assumption is valid for the current ABI structure, please remain mindful that any future change in the ABI format could require revisiting this logic.
crates/sozo/ops/src/migrate/mod.rs (1)
834-853: Handle potential errors in class flattening.The unwrap on class flattening could panic if the flattening fails.
There was a problem hiding this comment.
Looking nice!
When inside inspect world, we should also output the external contracts.
Adding something like:
let mut external_contracts_disp = vec![];
for contract in world_diff.external_contracts.values() {
external_contracts_disp.push(external_contract_diff_display(contract));
}And then doing sorting + print_table should do the trick. 👍
If you prefer, other ERCs can be made in an other PR, or we can wait them to be added here before merging, up to you. Same for the world and external contracts supports in sozo execute.
bin/sozo/src/commands/inspect.rs
Outdated
| #[derive(Debug, Serialize)] | ||
| struct ExternalContractInspect { | ||
| contract_name: String, | ||
| instance_name: String, | ||
| class_hash: String, | ||
| status: String, | ||
| salt: String, | ||
| constructor_calldata: Vec<String>, | ||
| address: String, | ||
| } |
There was a problem hiding this comment.
| #[derive(Debug, Serialize)] | |
| struct ExternalContractInspect { | |
| contract_name: String, | |
| instance_name: String, | |
| class_hash: String, | |
| status: String, | |
| salt: String, | |
| constructor_calldata: Vec<String>, | |
| address: String, | |
| } | |
| #[derive(Debug, Tabled, Serialize)] | |
| struct ExternalContractInspect { | |
| #[tabled(rename = "External Contract")] | |
| contract_name: String, | |
| #[tabled(rename = "Instance Name")] | |
| instance_name: String, | |
| #[tabled(skip)] | |
| class_hash: String, | |
| #[tabled(rename = "Status")] | |
| status: ResourceStatus, | |
| #[tabled(skip)] | |
| salt: String, | |
| #[tabled(skip)] | |
| constructor_calldata: Vec<String>, | |
| #[tabled(rename = "Contrat Address")] | |
| address: String, | |
| } |
wdyt about something like this as default printing?
There was a problem hiding this comment.
Wow, world inspection is amazing ! I fully agree with that 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
bin/sozo/src/commands/execute.rs (1)
105-140: Ohayo! Let's make this code even more elegant, sensei!The contract resolution logic is well-structured, but we can improve its readability and maintainability.
Consider these improvements:
let contract_address = if tag_or_address == "world" { - match contracts.get(&tag_or_address) { - Some(c) => c.address, + match contracts.get(&tag_or_address) { + Some(world_contract) => world_contract.address, None => bail!("Unable to find the world address."), } } else { - // first, try to find the contract to call among Dojo contracts - let descriptor = ResourceDescriptor::from_string(&tag_or_address)? - .ensure_namespace(&profile_config.namespace.default); - - let mut contract_address = match &descriptor { - ResourceDescriptor::Address(address) => Some(*address), - ResourceDescriptor::Tag(tag) => contracts.get(tag).map(|c| c.address), - ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") - } - }; - - // if not found, try to find a Starknet contract matching with the provided - // contract name. - if contract_address.is_none() { - contract_address = contracts.get(&tag_or_address).map(|c| c.address); - } + resolve_contract_address( + &tag_or_address, + &contracts, + &profile_config.namespace.default, + self.diff, + )? +}; +fn resolve_contract_address( + tag_or_address: &str, + contracts: &HashMap<String, Contract>, + default_namespace: &str, + diff_enabled: bool, +) -> Result<FieldElement> { + // First, try to find the contract among Dojo contracts + let descriptor = ResourceDescriptor::from_string(tag_or_address)? + .ensure_namespace(default_namespace); + + let mut contract_address = match &descriptor { + ResourceDescriptor::Address(address) => Some(*address), + ResourceDescriptor::Tag(tag) => contracts.get(tag).map(|contract| contract.address), + ResourceDescriptor::Name(_) => { + unimplemented!("Expected to be a resolved tag with default namespace.") + } + }; + + // If not found, try to find a Starknet contract + if contract_address.is_none() { + contract_address = contracts.get(tag_or_address).map(|contract| contract.address); + } + contract_address.ok_or_else(|| { + let mut message = format!("Contract {descriptor} not found in the manifest."); + if diff_enabled { + message.push_str( + " Run the command again with `--diff` to force the fetch of data from the chain.", + ); + } + anyhow!(message) + }) }bin/sozo/src/commands/call.rs (3)
86-106: Consider refactoring the contract resolution logic, sensei!The current implementation has two separate lookups for contract resolution. This could be simplified for better maintainability.
Here's a suggested refactoring that combines the lookups:
- let mut contract_address = match &descriptor { - ResourceDescriptor::Address(address) => Some(*address), - ResourceDescriptor::Tag(tag) => { - // Try to find the contract to call among Dojo contracts - contracts.get(tag).map(|c| c.address) - } - ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") - } - }; - - if contract_address.is_none() { - contract_address = match &tag_or_address { - ResourceDescriptor::Name(name) => contracts.get(name).map(|c| c.address), - ResourceDescriptor::Address(_) | ResourceDescriptor::Tag(_) => { - // A contract should have already been found while searching for a Dojo - // contract. - None - } - } - } + let contract_address = match (&descriptor, &tag_or_address) { + (ResourceDescriptor::Address(address), _) => Some(*address), + (ResourceDescriptor::Tag(tag), _) => contracts.get(tag).map(|c| c.address), + (_, ResourceDescriptor::Name(name)) => contracts.get(name).map(|c| c.address), + _ => None + };
79-82: Enhance error messages for better debugging, sensei!The current error message doesn't specify whether the lookup failed for a Dojo contract or an external contract.
Here's a suggested improvement:
- bail!( - "Unable to get the list of contracts, either from the world or from the \ - local manifest." - ), + bail!( + "Unable to get the list of contracts. No local manifest found and world \ + diff is disabled." + ), - let contract_address = contract_address - .ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))?; + let contract_address = contract_address.ok_or_else(|| { + let lookup_type = if matches!(tag_or_address, ResourceDescriptor::Name(_)) { + "external" + } else { + "Dojo" + }; + anyhow!("{} contract {} not found", lookup_type, descriptor) + })?;Also applies to: 108-109
132-135: Consider structured output format for better parsing, sensei!The current output format is a simple space-separated list. Consider adding a JSON output option for better machine readability.
Here's a suggested enhancement:
println!( + "{}", if self.json { + serde_json::to_string(&output)? + } else { "[ {} ]", output.iter().map(|o| format!("0x{:x}", o)).collect::<Vec<_>>().join(" ") + } );This would require adding a new
jsonflag toCallArgs:#[arg(long)] #[arg(help = "Output results in JSON format")] pub json: bool,crates/dojo/world/src/diff/external_contract.rs (3)
5-18: Ohayo! Consider enhancing the documentation for better clarity, sensei.The documentation could be more descriptive by explaining:
- The purpose and use cases of each variant
- When each variant is used in the lifecycle of an external contract
/// The difference between a local and a remote external contract class. +/// +/// # Variants +/// - `Created`: Represents a newly created contract class that exists only locally +/// - `Synced`: Represents a contract class that is synchronized with the remote state #[derive(Debug)] pub enum ExternalContractClassDiff { Created(ExternalContractClassLocal), Synced(ExternalContractClassLocal), } /// The difference between a local and a remote external contract. +/// +/// # Variants +/// - `Created`: Represents a newly created contract instance that exists only locally +/// - `Synced`: Represents a contract instance that is synchronized with the remote state #[derive(Debug)]
20-27: Consider returning a reference instead of cloning, sensei.The
class_datamethod currently clones the data for both variants. For better performance, especially with large contract classes, consider returning a reference.- 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, } }
29-42: Similar optimization opportunity for contract_data method, sensei.The
contract_datamethod also unnecessarily clones the data. Consider returning a reference here as well.- pub fn contract_data(&self) -> ExternalContractLocal { + pub fn contract_data(&self) -> &ExternalContractLocal { match self { - ExternalContractDiff::Created(c) => c.clone(), - ExternalContractDiff::Synced(c) => c.clone(), + ExternalContractDiff::Created(c) => c, + ExternalContractDiff::Synced(c) => c, } }The
addressmethod looks good as it returns a copy of the primitiveFelttype.bin/sozo/src/commands/inspect.rs (2)
186-201: Enhance error message clarity, sensei.The error message could be more helpful by specifying what was searched for.
- Err(anyhow::anyhow!("Resource or external contract not found locally.")) + Err(anyhow::anyhow!( + "Resource or external contract '{}' not found locally.", + element_name + ))
468-484: Optimize to avoid unnecessary cloning, sensei.The function clones the contract data unnecessarily. Consider using references since we suggested changing
contract_data()to return a reference.fn external_contract_diff_display(contract: &ExternalContractDiff) -> ExternalContractInspect { - let contract_data = contract.contract_data(); + let contract_data = contract.contract_data(); // This will return &ExternalContractLocal after our previous change ExternalContractInspect { - contract_name: contract_data.contract_name, - instance_name: contract_data.instance_name, + contract_name: contract_data.contract_name.clone(), + instance_name: contract_data.instance_name.clone(), address: contract_data.address.to_fixed_hex_string(), class_hash: contract_data.class_hash.to_fixed_hex_string(), status: match contract { ExternalContractDiff::Created(_) => ResourceStatus::Created, ExternalContractDiff::Synced(_) => ResourceStatus::Synced, }, salt: contract_data.salt.to_fixed_hex_string(), - constructor_calldata: contract_data.constructor_data, + constructor_calldata: contract_data.constructor_data.clone(), } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bin/sozo/src/commands/auth.rs(1 hunks)bin/sozo/src/commands/call.rs(3 hunks)bin/sozo/src/commands/execute.rs(4 hunks)bin/sozo/src/commands/inspect.rs(10 hunks)bin/sozo/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/contract_info.rs(12 hunks)crates/dojo/world/src/diff/external_contract.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- bin/sozo/src/utils.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (9)
crates/dojo/world/src/contracts/contract_info.rs (4)
26-35: Ohayo! The changes to ContractInfo look good, sensei!The field rename from
tagtotag_or_namebetter reflects its purpose, and addingPartialEqenables proper equality comparisons.
62-71: Ohayo! Nice addition of external contracts support, sensei!The implementation correctly handles external contracts by mapping them to ContractInfo instances with empty entrypoints.
131-142: Ohayo! The WorldDiff conversion looks good, sensei!The implementation properly handles external contracts, and the comment about entrypoints not being available for Starknet contracts is helpful.
232-284: Ohayo! Excellent test coverage, sensei!The test cases thoroughly verify the external contracts functionality with multiple instances and proper assertions.
bin/sozo/src/commands/auth.rs (1)
751-751: Ohayo! The test update looks good, sensei!The field rename from
tagtotag_or_namemaintains consistency with the ContractInfo struct changes.bin/sozo/src/commands/execute.rs (2)
29-32: Ohayo! Help text looks great, sensei!The updated help text clearly explains the three possible types of contract references: Dojo contracts, Starknet contracts, and the world contract.
108-108: Ohayo! Error handling looks solid, sensei!The error messages are clear and actionable:
- World contract errors are concise and to the point
- Contract resolution errors include helpful suggestions about using
--diffAlso applies to: 129-139
bin/sozo/src/commands/call.rs (1)
3-3: Ohayo! Nice improvements to the command interface, sensei!The updated help text now clearly explains all possible input types for
tag_or_address, making it more user-friendly for both Dojo and external Starknet contracts.Also applies to: 22-24
bin/sozo/src/commands/inspect.rs (1)
270-276: LGTM! Clean and consistent implementation.The function follows the established pattern for inspection functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
examples/spawn-and-move/src/externals/erc20.cairo (1)
23-33: Storage struct structure is well-defined, buddy.
Organizing substorages for ERC20 and ownership is clean. Consider adding doc comments explaining each substorage for new collaborators.examples/spawn-and-move/src/externals/erc721.cairo (2)
6-13: Nicely done bringing in the needed components, sensei!
You’ve structured your import ofOwnableComponentandSRC5Componentwell. Double-check that no relevant disclaimers or disclaimers on object references are missing for future code expansions.
35-44: Event enum is well-defined, ohayo sensei!
Flattening multiple event streams is helpful. Document each event shape for downstream tooling clarity.examples/spawn-and-move/src/externals/erc1155.cairo (2)
25-33: Storage struct is well set, sensei!
Including multiple substorages helps modularity. Keep an eye on expansions if more features get added.
67-77: Batch mint is a tidy addition, sensei!
Allows multiple tokens minted in one shot, which is efficient. Watch out for large loops iftoken_idsis big. Consider gas or performance constraints.examples/spawn-and-move/src/externals/bank.cairo (1)
1-14: Ohayo! Let's make this Bank more secure, sensei! 🏦While the basic owner pattern is implemented correctly, consider these enhancements:
- Add events for ownership changes
- Implement access control for future functions
- Add documentation for the contract's purpose
Here's a suggested implementation:
#[starknet::contract] mod Bank { use starknet::storage::StoragePointerWriteAccess; use starknet::ContractAddress; + use starknet::event::EventEmitter; + + #[event] + #[derive(Drop, starknet::Event)] + enum Event { + OwnershipTransferred: OwnershipTransferred, + } + + #[derive(Drop, starknet::Event)] + struct OwnershipTransferred { + previous_owner: ContractAddress, + new_owner: ContractAddress, + } #[storage] struct Storage { owner: ContractAddress, } #[constructor] fn constructor(ref self: ContractState, owner: ContractAddress) { + // Emit event for initial ownership + self.emit(Event::OwnershipTransferred( + OwnershipTransferred { previous_owner: 0.try_into().unwrap(), new_owner: owner } + )); self.owner.write(owner); } }examples/spawn-and-move/dojo_dev.toml (2)
26-30: Ohayo! Consider using unique salt values for each contract instance, sensei!The current configuration uses the same salt value "1" across different contract instances. While this works, using unique salts would prevent potential address collisions if the same contract type is deployed multiple times.
Consider using descriptive salts that reflect the token's purpose:
-salt = "1" +salt = "gold_token_v1"
38-42: Ohayo sensei! Consider using IPFS for NFT metadata!The current configuration uses a direct HTTP URL for NFT metadata. This approach has several limitations:
- Centralization risk if the domain expires or changes
- No content integrity guarantees
- Limited flexibility for metadata updates
Consider using IPFS for metadata storage since an IPFS gateway is already configured in the environment section:
-constructor_data = ["0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba", "str:Badge", "str:BDG", "str:https://badge.com/" ] +constructor_data = ["0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba", "str:Badge", "str:BDG", "str:ipfs://" ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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/externals.cairo(1 hunks)examples/spawn-and-move/src/externals/bank.cairo(1 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)examples/spawn-and-move/src/externals/erc20.cairo(1 hunks)examples/spawn-and-move/src/externals/erc721.cairo(1 hunks)examples/spawn-and-move/src/externals/saloon.cairo(1 hunks)examples/spawn-and-move/src/lib.cairo(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/src/lib.cairo
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: ensure-wasm
🔇 Additional comments (20)
examples/spawn-and-move/src/externals/erc20.cairo (4)
1-3: Ohayo sensei, license and version references look correct!
All good here with the MIT license sequence and the library version.
6-11: Solid usage of OpenZeppelin components, sensei!
Linking bothERC20ComponentandOwnableComponentis a neat modular approach. Make sure you confirm the imported version is stable and aligned with your project's needs.
40-52: Constructor usage is nicely laid out, sensei!
It’s good to see the owner and token parameters being set from the start. If there’s any risk of biginitial_supply, consider verifying it doesn’t exceed your numeric limits or cause unexpected overflows.
58-61: Mint function looks functional, ohayo sensei!
Ownership checks are on point. Verify that the underlying ERC20 library emits a properTransferevent on mint calls to keep logs consistent and up-to-date.examples/spawn-and-move/src/externals/erc721.cairo (3)
1-3: All good on license and version references, buddy!
Your contract header is consistent with best practices for open source usage.
46-56: Constructor’s initialization seems clean, buddy!
You properly set owner, name, symbol, and base URI. That said, consider verifying that base URI is a valid format before callinginitializerto avoid invalid references.
62-70: Mint function implemented well, sensei!
Ownership is enforced. If you also plan to handle a burn or token transfer override, confirm it addresses re-entrancy or unexpected behavior scenarios.examples/spawn-and-move/src/externals/erc1155.cairo (4)
1-3: Ohayo sensei, license details here are fine!
MIT license coverage looks good with correct version references for library usage.
6-13: Comprehensive usage of ERC1155, SRC5, and Ownable components, buddy!
Emitting combined events is a plus. Just ensure no collisions in event naming or indexing.
46-50: Constructor sets base URI effectively, sensei!
Make sure the provided URI is validated for correctness or safe usage in external metadata tools.
56-65: Single mint function with acceptance checks, buddy!
Ensuring only the owner can mint avoids unwanted token creation. Confirm thatmint_with_acceptance_checktriggers the correct event logs for external tooling.examples/spawn-and-move/src/externals.cairo (1)
1-5: Ohayo! Nice module organization, sensei! 🎋The module structure effectively separates different contract types and includes all major token standards (ERC20, ERC721, ERC1155) as requested. This organization will make it easier for users to work with different token types in spawn and move functionalities.
examples/spawn-and-move/src/externals/saloon.cairo (1)
1-8: Ohayo! The Saloon seems a bit empty, sensei! 🍵The contract structure is in place, but it needs:
- Documentation explaining its purpose and intended usage
- Implementation of core functionality
- Storage variables if needed
Could you share more details about the intended functionality of this contract?
examples/spawn-and-move/Scarb.toml (2)
17-17: Let's verify the OpenZeppelin version, sensei! 📦Let's ensure we're using a stable and compatible version of OpenZeppelin.
✅ Verification successful
🌐 Web query:
What is the latest stable version of OpenZeppelin for Cairo 2.9.2?💡 Result:
The latest stable version of OpenZeppelin Contracts for Cairo compatible with Cairo 2.x is v0.20.0 (released December 6, 2024)[^2]1. This version:
- Supports Scarb v2.x (specifically tested with Scarb v2.9.x)1
- Includes compatibility with Cairo compiler versions up to at least v2.x (as indicated by its dependency on Sierra v1.x)23
- Introduces new features like SRC9 integration and improved governance components[^2]
For Cairo 2.x development, you should use:
[dependencies] openzeppelin = { version = "0.20" }Key compatibility notes:
1️⃣ The documentation explicitly shows compatibility with Scarb v2.x toolchain1
2️⃣ New storage features likeMap/Vectypes (introduced in Cairo v2.x) are supported2
3️⃣ Contains breaking changes from previous versions regarding mutability modifiers[^2]Citations:
- 1: https://blog.openzeppelin.com/announcing-openzeppelin-contracts-for-cairo-v0-2-0
- 2: https://github.com/OpenZeppelin/cairo-contracts/releases
- 3: https://docs.openzeppelin.com/contracts/2.x/releases-stability
- 4: https://community.starknet.io/t/cairo-v2-7-0-is-coming/114362
- 5: https://docs.openzeppelin.com/contracts/5.x/backwards-compatibility
- 6: https://www.npmjs.com/package/@openzeppelin/contracts?activeTab=versions
- 7: https://community.starknet.io/t/cairo-v2-6-0-is-out/113687
- 8: https://wizard.openzeppelin.com/cairo
- 9: https://www.openzeppelin.com/cairo-contracts
- 10: https://docs.openzeppelin.com/contracts-cairo/0.20.0/
Ohayo, sensei! OpenZeppelin version verified.
The TOML file using
openzeppelin = "0.20.0"is correct as it matches the latest stable version for Cairo 2.9.2. No changes required.
14-14: Heads up about experimental features, sensei!⚠️ Using experimental libfuncs could introduce potential risks. Please ensure this is necessary for your use case.
✅ Verification successful
🌐 Web query:
What are the risks of using experimental libfuncs in Cairo?💡 Result:
Using experimental
libfuncsin Cairo introduces several risks primarily related to stability, security, and compatibility. These risks stem from their unproven nature in production environments and potential deviations from Cairo 1.0’s safety guarantees:1. Runtime Instability
Experimental
libfuncsmay lack rigorous testing for edge cases or complex interactions with other components of Cairo’s type system (e.g., ownership rules forBox<T>). This could lead to runtime failures during transaction execution[1].2. Security Vulnerabilities
Cairo’s safety guarantees rely on
libfuncsgenerating correct-by-construction CASM code[1]. Experimentallibfuncsmight bypass these safeguards:
- Memory errors: Untested pointer operations could violate Cairo’s single-write memory model[1].
- Gas calculation flaws: Poorly implemented gas metering (e.g.,
withdraw_gas) might fail to prevent infinite loops[1].3. Backend Compatibility Issues
While not directly part of Sierra’s
libfuncdesign:
- Experimental OpenGL integrations (via projects like glitz) have historically caused rendering artifacts due to immature driver support[4][6].
- Hardware acceleration paths for experimental backends may behave unpredictably compared to stable X11/PDF targets[7][9].
4. Compilation Failures
Experimental
libfuncsmight:
- Generate invalid CASM code that fails sequencer validation[1]
- Conflict with linear type system checks (e.g., improper
drop<T>usage)[1]- Produce uncompressed dictionaries rejected by
dict_squash[1]5. Deprecation Risks
Experimental features face higher likelihood of API changes or removal in future compiler versions without backward compatibility[6][9].
In summary, while experimental
libfuncsenable innovation (e.g., new cryptographic primitives), they bypass Cairo’s core safety mechanisms until formally verified and integrated into Sierra’s trusted function set[1]. Developers should prioritize stablelibfuncsfor production contracts unless willing to accept transaction failures or protocol-level rejection risks[1][7].Citations:
- 1: https://starknetastro-7783.xlog.page/Cairo-10-de-nei-bu-ji-zhi--tan-suo-Sierra?locale=en
- 2: https://hackage.haskell.org/package/cairo-0.13.8.2/docs/Graphics-Rendering-Cairo.html
- 3: https://onemileatatime.com/news/egypt-threatens-me-criticizing-cairo-airport/
- 4: https://github.com/msteinert/cairo
- 5: https://www.cairographics.org/OpenGL/
- 6: https://www.cairographics.org/todo/
- 7: https://www.cairographics.org
- 8: https://www.itl.nist.gov/div898/software/dataplot/refman1/auxillar/cairo.htm
- 9: https://gitlab.freedesktop.org/cairo/cairo
- 10: https://www.datasciencecentral.com/neat-r-plots-with-the-cairo-graphics-library/
Ohayo, sensei!
The web query confirms that using experimental libfuncs in Cairo carries notable risks—ranging from runtime instability and potential security vulnerabilities (like memory and gas metering issues) to backend compatibility challenges and compilation failures. This reaffirms the need for caution when opting into experimental features. Please ensure that their use is essential for your project's requirements.
examples/spawn-and-move/dojo_dev.toml (4)
32-36: Same salt value issue as GoldToken
44-48: Same URL configuration concern as Badge NFT
50-53: LGTM! Clean and minimal configuration, sensei!
55-58: LGTM! Simple and clean configuration!examples/spawn-and-move/manifest_dev.json (1)
2226-4687: LGTM! Auto-generated manifest looks good, sensei!The manifest correctly reflects all the external contract configurations from dojo_dev.toml.
Footnotes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
bin/sozo/src/commands/inspect.rs (4)
20-22: Ohayo! The help text could be more descriptive, sensei.The help text could be enhanced to better explain the element parameter and its format.
- #[arg(help = "The tag of the resource or the external contract instance name to inspect. If \ - not provided, a world summary will be displayed.")] + #[arg(help = "The tag of the resource (e.g., 'models/position') or the external contract instance \ + name (e.g., 'erc20_token') to inspect. If not provided, a world summary will be \ + displayed.")]
186-201: Consider adding error context, sensei.The error message could be more helpful by including the element name in the error message.
- Err(anyhow::anyhow!("Resource or external contract not found locally.")) + Err(anyhow::anyhow!("Resource or external contract '{}' not found locally.", element_name))
324-326: Consider adding a length check before sorting, sensei.Add a length check to avoid unnecessary sorting when the list is empty.
for contract in world_diff.external_contracts.values() { external_contracts_disp.push(external_contract_diff_display(contract)); } + + if !external_contracts_disp.is_empty() { external_contracts_disp.sort_by_key(|c| format!("{}-{}", c.contract_name, c.instance_name)); + }Also applies to: 332-332
468-484: Consider adding documentation for the return value, sensei.The function's documentation could be enhanced to describe the returned struct's fields.
-/// Displays the external contract diff. +/// Displays the external contract diff. +/// +/// Returns an `ExternalContractInspect` struct containing: +/// - contract_name: The name of the Cairo contract +/// - instance_name: The unique instance name +/// - address: The deployed contract address +/// - class_hash: The contract's class hash +/// - status: The current status (Created/Synced) +/// - salt: The salt used for address computation +/// - constructor_calldata: The constructor parameters
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/inspect.rs(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (2)
bin/sozo/src/commands/inspect.rs (2)
138-154: LGTM! Clean struct definition, sensei.The
ExternalContractInspectstruct is well-organized with appropriate field annotations and types.
270-276: The function looks good, sensei!The
inspect_external_contractfunction is concise and follows the same pattern asinspect_resource.
Thanks for these great comments !
A last pending point: |
|
Thank you @remybar!
About this, it's all fine and the external contract should take precedence. We could also disable the default namespace at some point in the CLI, since adding an imho, disabling the support for default namespace could make things less ambiguous, wdyt? |
There was a problem hiding this comment.
Thank you for the work here @remybar!
I simplified the mint functions to have something easier to call and without the safety checks.
Let's iterate with the version. About the the default namespace, let's see if we just ignore it in the future for execute and call. 👍
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
examples/spawn-and-move/src/externals/erc20.cairo (1)
58-61: Ohayo sensei, mint function lacks an ownership check.While you've rightfully noted this is only for testing, be cautious about the production use case. An unguarded
mintallows any caller to inflate the token supply. If needed, apply anonlyOwneror similar control:fn mint(ref self: ContractState, amount: u256) { + // Consider restricting to owner in production: + // self.ownable.ensure_owner(starknet::get_caller_address()); self.erc20.mint(starknet::get_caller_address(), amount); }examples/spawn-and-move/src/externals/erc721.cairo (1)
46-56: Constructor implementation looks good, but could use input validation!While the initialization is correct, consider adding validation for the owner address.
Consider adding this validation:
fn constructor( ref self: ContractState, owner: ContractAddress, name: ByteArray, symbol: ByteArray, base_uri: ByteArray, ) { + assert(!owner.is_zero(), 'Owner cannot be zero'); self.ownable.initializer(owner); self.erc721.initializer(name, symbol, base_uri); }examples/spawn-and-move/manifest_dev.json (2)
2911-2937: Bank contract could benefit from more functionality, sensei!The Bank contract appears to be a minimal implementation with just owner initialization. Consider enhancing it with common banking features like deposits, withdrawals, and balance tracking.
Would you like me to propose an enhanced Bank contract implementation with these features?
4162-4654: ERC20 WoodToken shares same implementation as GoldToken, sensei!Consider creating a base token contract to avoid code duplication between GoldToken and WoodToken.
+ Create a base token contract: + contract BaseToken is ERC20, Ownable { + constructor( + address owner, + string memory name, + string memory symbol, + uint256 initialSupply, + address recipient + ) ERC20(name, symbol) { + _transferOwnership(owner); + _mint(recipient, initialSupply); + } + + function mint(uint256 amount) external onlyOwner { + _mint(msg.sender, amount); + } + } + + // Then inherit from it: + contract GoldToken is BaseToken { + constructor( + address owner, + uint256 initialSupply, + address recipient + ) BaseToken(owner, "Gold", "GOLD", initialSupply, recipient) {} + } + + contract WoodToken is BaseToken { + constructor( + address owner, + uint256 initialSupply, + address recipient + ) BaseToken(owner, "Wood", "WOOD", initialSupply, recipient) {} + }examples/spawn-and-move/src/externals/erc1155.cairo (2)
11-14: Consider adding documentation for components, sensei!While the component declarations are correct, adding documentation explaining each component's purpose would improve code maintainability.
+ /// ERC1155 component for token functionality component!(path: ERC1155Component, storage: erc1155, event: ERC1155Event); + /// SRC5 component for introspection support component!(path: SRC5Component, storage: src5, event: SRC5Event); + /// Ownable component for access control component!(path: OwnableComponent, storage: ownable, event: OwnableEvent);
15-24: Ohayo! Consider implementing ERC1155 hooks for extended functionality.While the basic implementations are in place, implementing custom hooks could provide additional features like transfer validation or notifications.
+ impl ERC1155HooksImpl = ERC1155HooksEmptyImpl<ContractState>;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/spawn-and-move/manifest_dev.json(1 hunks)examples/spawn-and-move/src/externals/erc1155.cairo(1 hunks)examples/spawn-and-move/src/externals/erc20.cairo(1 hunks)examples/spawn-and-move/src/externals/erc721.cairo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (15)
examples/spawn-and-move/src/externals/erc20.cairo (4)
1-2: Ohayo sensei, verifying the MIT license usage.It's great to see the MIT license properly declared. Just be sure it's consistent with any library you're pulling in and that your entire project maintains a compatible licensing strategy.
10-11: Ohayo sensei, component macros look well-structured.Utilizing
component!for bothERC20ComponentandOwnableComponentis a concise approach to integrate token and ownership functionalities. This separation enhances modularity and readability. Good job!
31-38: Ohayo sensei, combined event enum is nicely flattened.Flattening the event variants from both
ERC20ComponentandOwnableComponentsimplifies event handling. This approach helps keep your emitted logs consistent.
40-52: Ohayo sensei, verify the initial supply minting logic.Your constructor mints the
initial_supplyto therecipient. This is fine for many scenarios, but confirm that this aligns with your intended distribution. Otherwise, consider minting to the contract owner to maintain control.examples/spawn-and-move/src/externals/erc721.cairo (5)
1-10: Ohayo! The contract structure and imports look solid, sensei!The contract follows OpenZeppelin's best practices and includes all necessary components for a complete ERC721 implementation.
11-14: Component declarations are well-organized!The component macro usage correctly sets up the storage and event paths for each component.
15-24: Implementation organization is clean and clear!Good separation between external and internal implementations. The
embed_v0attribute ensures proper ABI generation.
25-33: Storage structure follows best practices!The substorage versioning (v0) is properly set for all components, allowing for future upgrades.
35-44: Event enum is properly structured!The
flatattribute is correctly used to flatten component events into the main event enum.examples/spawn-and-move/manifest_dev.json (4)
2226-2909: Ohayo! The ERC721 Badge contract looks well-configured, sensei!The contract implementation includes:
- Proper ownership controls through OpenZeppelin's Ownable
- Standard ERC721 interface implementation
- Secure mint function with access controls
- Well-structured constructor with name, symbol, and base URI
2938-3431: ERC20 GoldToken implementation looks solid, sensei!The contract includes:
- Standard ERC20 functionality
- Ownable for access control
- Mint function for token creation
- Initial supply setup in constructor
3432-4141: ERC1155 Rewards contract is well-structured, sensei!The implementation provides:
- Both single and batch minting capabilities
- Standard ERC1155 interface compliance
- URI support for token metadata
- Proper access controls
2226-4654: Verify the owner address initialization across contracts, sensei!All contracts use the same owner address
0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba. Ensure this is intentional and the address is properly secured.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the owner address is used consistently and securely # Look for owner address usage echo "Checking owner address usage across contracts..." rg -A 2 "0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba" # Check for any hardcoded addresses echo "Checking for other hardcoded addresses..." rg -A 2 "0x[a-fA-F0-9]{64}"Length of output: 93247
Ohayo, sensei! Owner Address Verification Across Contracts
Based on the output from our search, the owner address
0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba
is used consistently across various configuration files, manifests, tests, and deployment scripts. This uniform usage appears intentional for the examples and test environments, which is common in such contexts.Please double-check that using a hardcoded and shared owner address is safe for your production setup. For demos and testing, it looks fine, but make sure this design meets your security requirements in live deployments.
examples/spawn-and-move/src/externals/erc1155.cairo (2)
1-10: Ohayo! LGTM on the contract setup, sensei!The contract properly imports the required OpenZeppelin components and follows the standard structure.
25-44: Storage and event definitions look great, sensei!The storage versioning and event flattening are properly implemented.
| /// This implementation is not secure, only for testing purposes and quick minting. | ||
| #[generate_trait] | ||
| #[abi(per_item)] | ||
| impl ERC721Demo of ERC721DemoTrait { | ||
| #[external(v0)] | ||
| fn mint(ref self: ContractState, token_id: u256) { | ||
| self.erc721.mint(starknet::get_caller_address(), token_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security warning in mint function needs attention, sensei!
The mint function is marked as insecure in the comments. Consider implementing proper access control.
Add Ownable access control:
#[external(v0)]
fn mint(ref self: ContractState, token_id: u256) {
+ // Only owner can mint tokens
+ self.ownable.assert_only_owner();
self.erc721.mint(starknet::get_caller_address(), token_id);
}Also, consider adding these validations:
- Check if token_id already exists
- Implement a max supply limit
Let me help you verify if there are any existing security measures in the OpenZeppelin implementation:
🏁 Script executed:
#!/bin/bash
# Search for security checks in the OpenZeppelin ERC721 implementation
rg -A 5 "mint.*token_id" Length of output: 2236
Ohayo sensei! Based on the search results, we've confirmed that the ERC721 mint function in examples/spawn-and-move/src/externals/erc721.cairo does not include any access control or validation measures. There are no calls to an Ownable check like self.ownable.assert_only_owner() nor validations to verify if a token already exists or enforce a max supply.
- File & Location:
examples/spawn-and-move/src/externals/erc721.cairo(around lines 58-66). - Issue: The current implementation lacks proper access control, making it insecure as noted in the review.
- Recommendation: Introduce Ownable-based access control (e.g., call
self.ownable.assert_only_owner()) and add token existence and max supply validations.
📝 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.
| /// This implementation is not secure, only for testing purposes and quick minting. | |
| #[generate_trait] | |
| #[abi(per_item)] | |
| impl ERC721Demo of ERC721DemoTrait { | |
| #[external(v0)] | |
| fn mint(ref self: ContractState, token_id: u256) { | |
| self.erc721.mint(starknet::get_caller_address(), token_id); | |
| } | |
| } | |
| /// This implementation is not secure, only for testing purposes and quick minting. | |
| #[generate_trait] | |
| #[abi(per_item)] | |
| impl ERC721Demo of ERC721DemoTrait { | |
| #[external(v0)] | |
| fn mint(ref self: ContractState, token_id: u256) { | |
| // Only owner can mint tokens | |
| self.ownable.assert_only_owner(); | |
| self.erc721.mint(starknet::get_caller_address(), token_id); | |
| } | |
| } |
| { | ||
| "class_hash": "0x385397f49e3e40c38890a195e1866f1fb25326cfde124a27b7eaf3f238bf081", | ||
| "contract_name": "Saloon", | ||
| "instance_name": "Saloon", | ||
| "address": "0x4cdfebcb371188d5f2fb54b51d5de43a62c1b5998a397021f48807483db72a7", | ||
| "abi": [ | ||
| { | ||
| "type": "constructor", | ||
| "name": "constructor", | ||
| "inputs": [] | ||
| }, | ||
| { | ||
| "type": "event", | ||
| "name": "dojo_examples::externals::saloon::Saloon::Event", | ||
| "kind": "enum", | ||
| "variants": [] | ||
| } | ||
| ], | ||
| "constructor_calldata": [] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Saloon contract needs more functionality, sensei!
The Saloon contract is currently a minimal implementation with no functionality. Consider adding features like:
- Player interaction mechanics
- Resource management
- Event emission for activities
Would you like me to propose an enhanced Saloon contract implementation with these features?
| #[constructor] | ||
| fn constructor(ref self: ContractState, owner: ContractAddress, base_uri: ByteArray) { | ||
| self.ownable.initializer(owner); | ||
| self.erc1155.initializer(base_uri); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation in constructor, sensei!
The constructor should validate that the owner address is not zero.
#[constructor]
fn constructor(ref self: ContractState, owner: ContractAddress, base_uri: ByteArray) {
+ assert(!owner.is_zero(), 'Owner cannot be zero');
self.ownable.initializer(owner);
self.erc1155.initializer(base_uri);
}📝 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.
| #[constructor] | |
| fn constructor(ref self: ContractState, owner: ContractAddress, base_uri: ByteArray) { | |
| self.ownable.initializer(owner); | |
| self.erc1155.initializer(base_uri); | |
| } | |
| #[constructor] | |
| fn constructor(ref self: ContractState, owner: ContractAddress, base_uri: ByteArray) { | |
| assert(!owner.is_zero(), 'Owner cannot be zero'); | |
| self.ownable.initializer(owner); | |
| self.erc1155.initializer(base_uri); | |
| } |
| /// This implementation is not secure, only for testing purposes and quick minting. | ||
| #[generate_trait] | ||
| #[abi(per_item)] | ||
| impl ExternalImpl of ExternalTrait { | ||
| #[external(v0)] | ||
| fn mint(ref self: ContractState, token_id: u256, value: u256) { | ||
| self | ||
| .erc1155 | ||
| .update( | ||
| starknet::contract_address_const::<0x0>(), | ||
| starknet::get_caller_address(), | ||
| array![token_id].span(), | ||
| array![value].span(), | ||
| ); | ||
| // Seems to not be supported by default dojo account. | ||
| // self.erc1155.mint_with_acceptance_check(account, token_id, value, data); | ||
| } | ||
|
|
||
| #[external(v0)] | ||
| fn batch_mint(ref self: ContractState, token_ids: Span<u256>, values: Span<u256>) { | ||
| self | ||
| .erc1155 | ||
| .update( | ||
| starknet::contract_address_const::<0x0>(), | ||
| starknet::get_caller_address(), | ||
| token_ids, | ||
| values, | ||
| ); | ||
| // Seems to not be supported by default dojo account. | ||
| // self.erc1155.batch_mint_with_acceptance_check(account, token_ids, values, data); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security concerns in minting functions!
- Using zero address as
fromis risky - No access control on mint functions
- Missing acceptance checks
Consider implementing these security improvements:
#[generate_trait]
#[abi(per_item)]
impl ExternalImpl of ExternalTrait {
#[external(v0)]
+ #[ownable::check_owner]
fn mint(ref self: ContractState, token_id: u256, value: u256) {
+ let caller = starknet::get_caller_address();
+ assert(!caller.is_zero(), 'Invalid caller');
self
.erc1155
.update(
- starknet::contract_address_const::<0x0>(),
+ caller,
starknet::get_caller_address(),
array![token_id].span(),
array![value].span(),
);
}
#[external(v0)]
+ #[ownable::check_owner]
fn batch_mint(ref self: ContractState, token_ids: Span<u256>, values: Span<u256>) {
+ let caller = starknet::get_caller_address();
+ assert(!caller.is_zero(), 'Invalid caller');
self
.erc1155
.update(
- starknet::contract_address_const::<0x0>(),
+ caller,
starknet::get_caller_address(),
token_ids,
values,
);
}
}📝 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.
| /// This implementation is not secure, only for testing purposes and quick minting. | |
| #[generate_trait] | |
| #[abi(per_item)] | |
| impl ExternalImpl of ExternalTrait { | |
| #[external(v0)] | |
| fn mint(ref self: ContractState, token_id: u256, value: u256) { | |
| self | |
| .erc1155 | |
| .update( | |
| starknet::contract_address_const::<0x0>(), | |
| starknet::get_caller_address(), | |
| array![token_id].span(), | |
| array![value].span(), | |
| ); | |
| // Seems to not be supported by default dojo account. | |
| // self.erc1155.mint_with_acceptance_check(account, token_id, value, data); | |
| } | |
| #[external(v0)] | |
| fn batch_mint(ref self: ContractState, token_ids: Span<u256>, values: Span<u256>) { | |
| self | |
| .erc1155 | |
| .update( | |
| starknet::contract_address_const::<0x0>(), | |
| starknet::get_caller_address(), | |
| token_ids, | |
| values, | |
| ); | |
| // Seems to not be supported by default dojo account. | |
| // self.erc1155.batch_mint_with_acceptance_check(account, token_ids, values, data); | |
| } | |
| } | |
| /// This implementation is not secure, only for testing purposes and quick minting. | |
| #[generate_trait] | |
| #[abi(per_item)] | |
| impl ExternalImpl of ExternalTrait { | |
| #[external(v0)] | |
| #[ownable::check_owner] | |
| fn mint(ref self: ContractState, token_id: u256, value: u256) { | |
| let caller = starknet::get_caller_address(); | |
| assert(!caller.is_zero(), 'Invalid caller'); | |
| self | |
| .erc1155 | |
| .update( | |
| caller, | |
| starknet::get_caller_address(), | |
| array![token_id].span(), | |
| array![value].span(), | |
| ); | |
| // Seems to not be supported by default dojo account. | |
| // self.erc1155.mint_with_acceptance_check(account, token_id, value, data); | |
| } | |
| #[external(v0)] | |
| #[ownable::check_owner] | |
| fn batch_mint(ref self: ContractState, token_ids: Span<u256>, values: Span<u256>) { | |
| let caller = starknet::get_caller_address(); | |
| assert(!caller.is_zero(), 'Invalid caller'); | |
| self | |
| .erc1155 | |
| .update( | |
| caller, | |
| starknet::get_caller_address(), | |
| token_ids, | |
| values, | |
| ); | |
| // Seems to not be supported by default dojo account. | |
| // self.erc1155.batch_mint_with_acceptance_check(account, token_ids, values, data); | |
| } | |
| } |
|
(I will check the wasm issue) |
@remybar the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/world/src/remote/events_to_remote.rs (2)
134-175: Ohayo! Consider enhancing error handling and performance, sensei!While the implementation is solid, here are two suggestions for improvement:
- The error handling could be more descriptive for debugging purposes.
- Consider batching the provider calls for better performance when dealing with multiple contracts.
Here's a suggested improvement for error handling:
match provider.get_class(BlockId::Tag(BlockTag::Pending), hash).await { Err(ProviderError::StarknetError(StarknetError::ClassHashNotFound)) => {} Ok(_) => { trace!( name, class_hash = format!("{:#066x}", hash), "External contract class already declared." ); self.declared_external_contract_classes.push(name); } - Err(e) => return Err(e.into()), + Err(e) => return Err(anyhow::anyhow!("Failed to get class for {}: {}", name, e)), };
415-721: Ohayo! Let's add tests for the new functionality, sensei!The new
load_external_contract_statesmethod needs test coverage to maintain the project's high testing standards.Would you like me to help generate test cases for the new method? I can provide comprehensive tests covering:
- Successfully loading declared and deployed contracts
- Handling undeclared/undeployed contracts
- Error cases
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
Cargo.toml(1 hunks)crates/dojo/world/Cargo.toml(0 hunks)crates/dojo/world/src/remote/events_to_remote.rs(2 hunks)crates/torii/grpc/src/client.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/dojo/world/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
crates/torii/grpc/src/client.rs (1)
3-3: Ohayo sensei, great usage of conditional compilation!
This line ensures thatDurationis only included when building for non-WASM targets. It cleanly separates code paths for different architectures, which helps maintain clarity and reduce build issues.crates/dojo/world/src/remote/events_to_remote.rs (1)
9-9: Ohayo! Clean import organization, sensei!The addition of
HashMapalongsideHashSetis well-organized and follows Rust's standard import conventions.Cargo.toml (1)
204-204: Ohayo! Nice move to async-first approach, sensei!The removal of the "blocking" feature from reqwest aligns with Rust's async-first philosophy and could improve performance. However, let's verify that all existing code has been updated to handle async requests.
Let's check for any remaining blocking reqwest calls:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for potential blocking reqwest calls that need to be updated rg -l 'reqwest::blocking' || echo "No blocking reqwest calls found" # Search for any .block() calls on reqwest that might need async/await rg '\.block\(\)' || echo "No .block() calls found"Length of output: 1413
Ohayo, sensei!
The search confirms that no explicit blocking reqwest calls remain in the code. The.block()usages that appear are part of other domain-specific method calls (e.g., onchain_specorclient) and are not related to reqwest's blocking client.
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