Convert katana/contracts into a Scarb workspace
#2523
Conversation
WalkthroughOhayo, sensei! The changes in this pull request involve updates across multiple files in the Katana project. Key modifications include updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Katana
participant StarkNet
User->>Katana: Deploy contract
Katana->>StarkNet: Interact with StarkNet
StarkNet-->>Katana: Response
Katana-->>User: Contract deployed successfully
🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
crates/katana/contracts/account/Scarb.toml (3)
8-10: Dependencies look sharp, sensei!The use of workspace versions for starknet and openzeppelin is a solid choice for maintaining consistency across the project. These dependencies are appropriate for a StarkNet contract project.
Consider specifying exact versions or version ranges for these dependencies in the workspace configuration. This practice can enhance reproducibility and make it easier to track which versions of these libraries are being used in your project.
12-13: Target configuration is on point, sensei!The StarkNet contract target is correctly defined, and setting 'sierra' to true is appropriate for the contract's compilation or deployment settings.
Consider exploring additional target configuration options that might be beneficial for your specific use case. For example, you might want to set explicit
casmorallowed-libfuncsoptions depending on your project requirements.
1-13: Ohayo once more, sensei! Overall, this Scarb.toml configuration is well-structured and appropriate.The file correctly defines the katana_account package, its dependencies, and the target configuration for a StarkNet contract. The use of workspace values promotes consistency across the project.
As you continue to develop the Katana project, consider maintaining a centralized document or README that explains the purpose and structure of this Scarb workspace. This will help new contributors understand how the various packages in the workspace interact and how to add new contracts or dependencies in the future.
crates/katana/contracts/Scarb.toml (1)
4-6: Package info is on point, sensei!The version and edition choices look solid. Starting with 0.1.0 is perfect for our new workspace. I'm curious about the specific features in the "2023_11" edition that benefit our project.
Consider adding a comment explaining why we chose this specific edition. It'll help future contributors understand our decision-making process.
crates/katana/contracts/account/src/lib.cairo (1)
29-37: Add documentation comments to theStoragestruct for clarity.Ohayo, sensei! Adding documentation to the
Storagestruct enhances readability and aids future development by explaining the purpose of each substorage component.Consider adding comments like this:
#[storage] +/// Storage struct combining account, SRC5, and upgradeable components. pub struct Storage { #[substorage(v0)] + /// Storage for account-related data. pub account: AccountComponent::Storage, #[substorage(v0)] + /// Storage for SRC5 interoperability. pub src5: SRC5Component::Storage, #[substorage(v0)] + /// Storage for upgradeability features. pub upgradeable: UpgradeableComponent::Storage }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
crates/katana/contracts/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
- crates/katana/contracts/.gitignore (1 hunks)
- crates/katana/contracts/.tool-versions (1 hunks)
- crates/katana/contracts/README.md (1 hunks)
- crates/katana/contracts/Scarb.toml (1 hunks)
- crates/katana/contracts/account/Scarb.toml (1 hunks)
- crates/katana/contracts/account/src/lib.cairo (1 hunks)
- crates/katana/contracts/legacy/README.md (1 hunks)
- crates/katana/contracts/messaging/cairo/.tool-versions (0 hunks)
- crates/katana/contracts/messaging/cairo/Scarb.toml (1 hunks)
- crates/katana/contracts/snfoundry.toml (1 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/contracts/messaging/cairo/.tool-versions
✅ Files skipped from review due to trivial changes (5)
- crates/katana/contracts/.gitignore
- crates/katana/contracts/.tool-versions
- crates/katana/contracts/README.md
- crates/katana/contracts/legacy/README.md
- crates/katana/contracts/snfoundry.toml
🧰 Additional context used
🔇 Additional comments (5)
crates/katana/contracts/messaging/cairo/Scarb.toml (3)
3-3: Ohayo, sensei! Excellent workspace version declaration!The change from a fixed version string to
version.workspace = trueis a great move. This allows for centralized version management across multiple packages in the Scarb workspace, making it easier to maintain consistent versions and update them all at once.
6-6: Dojo-level dependency management, sensei!Changing the starknet dependency to
starknet.workspace = trueis a wise decision. This ensures that all packages in the Scarb workspace use the same version of the starknet dependency, promoting consistency and simplifying updates across the project.
3-6: Ohayo once more, sensei! Your Scarb-fu is strong!The changes in this Scarb.toml file are perfectly aligned with the PR objectives of establishing a Scarb workspace for the
katana/contractsdirectory. By using workspace declarations for both version and dependencies, you're setting up a structure that will make it easier to manage and reproduce all feature contracts within Katana. This is a solid foundation for the new workspace structure. Keep up the excellent work!crates/katana/contracts/account/Scarb.toml (1)
1-4: Ohayo, sensei! Package configuration looks good!The package name "katana_account" accurately reflects its purpose within the Katana project. Using workspace values for version and edition ensures consistency across the project, which is a great practice.
crates/katana/contracts/Scarb.toml (1)
1-2: Ohayo, sensei! Workspace configuration looks good!The workspace setup aligns perfectly with our PR objectives. It's great to see the modular structure with separate members for "account" and "messaging/cairo". This will indeed facilitate the reproducibility of our contracts as intended.
| [workspace.dependencies] | ||
| starknet = "2.8.2" | ||
| openzeppelin = { git = "https://github.com/OpenZeppelin/cairo-contracts.git", tag = "v0.17.0" } | ||
| snforge_std = { git = "https://github.com/foundry-rs/starknet-foundry", tag = "v0.31.0" } | ||
| assert_macros = "2.8.2" |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo again, sensei!
I've spotted some version discrepancies in our dependencies:
- OpenZeppelin: Specified as
v0.17.0but the latest tag isv0.1.0. - Starknet Foundry: Specified as
v0.31.0but the latest tag isv0.20.1.
These mismatches suggest that the versions in Scarb.toml might be incorrect or ahead of the repository's actual releases. Please verify the intended versions to ensure compatibility and stability.
🔗 Analysis chain
Ohayo again, sensei! Dependencies are looking sharp!
The choice of dependencies is spot-on for our Starknet project. I appreciate the use of specific versions and tags - it's crucial for reproducibility.
Let's double-check if we're using the latest stable versions:
This script will help us ensure we're using the most up-to-date versions. If any discrepancies are found, we might want to consider updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the latest versions of our dependencies
# Check starknet version
echo "Latest starknet version:"
cargo search starknet | grep "starknet ="
# Check assert_macros version
echo "Latest assert_macros version:"
cargo search assert_macros | grep "assert_macros ="
# Check latest OpenZeppelin tag
echo "Latest OpenZeppelin tag:"
git ls-remote --tags https://github.com/OpenZeppelin/cairo-contracts.git | sort -V | tail -n 1
# Check latest Starknet Foundry tag
echo "Latest Starknet Foundry tag:"
git ls-remote --tags https://github.com/foundry-rs/starknet-foundry | sort -V | tail -n 1
Length of output: 804
| fn upgrade(ref self: ContractState, new_class_hash: ClassHash) { | ||
| self.account.assert_only_self(); | ||
| self.upgradeable.upgrade(new_class_hash); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider emitting an event upon upgrading the contract.
Ohayo, sensei! Emitting an event when the contract is upgraded improves transparency and allows off-chain services to track upgrades. Consider emitting the UpgradeableEvent after the upgrade operation.
Apply this diff to emit the event:
fn upgrade(ref self: ContractState, new_class_hash: ClassHash) {
self.account.assert_only_self();
self.upgradeable.upgrade(new_class_hash);
+ UpgradeableEvent::Upgraded { new_class_hash }.emit();
}Committable suggestion was skipped due to low confidence.
| pub fn constructor(ref self: ContractState, public_key: felt252) { | ||
| self.account.initializer(public_key); | ||
| } |
There was a problem hiding this comment.
Ensure all components are properly initialized in the constructor.
Ohayo, sensei! In the constructor, only the account component is initialized. To ensure all components function correctly, consider initializing the upgradeable and src5 components as well.
Apply this diff to initialize all components:
pub fn constructor(ref self: ContractState, public_key: felt252) {
self.account.initializer(public_key);
+ self.upgradeable.initializer();
+ self.src5.initializer();
}📝 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.
| pub fn constructor(ref self: ContractState, public_key: felt252) { | |
| self.account.initializer(public_key); | |
| } | |
| pub fn constructor(ref self: ContractState, public_key: felt252) { | |
| self.account.initializer(public_key); | |
| self.upgradeable.initializer(); | |
| self.src5.initializer(); | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2523 +/- ##
=======================================
Coverage 68.59% 68.59%
=======================================
Files 387 387
Lines 49988 49988
=======================================
+ Hits 34289 34290 +1
+ Misses 15699 15698 -1 ☔ View full report in Codecov by Sentry. |
from now on, all feature contracts on Katana should define its source code in this workspace. To ensure that all contracts can easily be reproduced.
Summary by CodeRabbit
New Features
Documentation
README.mdto include information on contract classes and legacy contracts.Chores