fix(dojo-lang): verify constructor args to follow dojo rules #2944
Conversation
WalkthroughOhayo, sensei! The pull request introduces updates to the Scarb dependencies in the Changes
Sequence DiagramsequenceDiagram
participant ContractImpl as Contract Implementation
participant ValidationFunc as is_valid_constructor_params()
participant Diagnostics as Diagnostics Vector
ContractImpl->>ValidationFunc: Check constructor parameters
ValidationFunc-->>ContractImpl: Validation result
alt Invalid Parameters
ValidationFunc->>Diagnostics: Push error diagnostic
else Valid Parameters
ContractImpl->>ContractImpl: Continue processing
end
The sequence diagram illustrates the new validation flow for constructor parameters, showing how the 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 0
🧹 Nitpick comments (1)
crates/dojo/lang/src/attribute_macros/contract.rs (1)
387-398: Consider making the validation more robust.While the validation logic works, it could be more resilient to whitespace variations.
Consider this improvement:
fn is_valid_constructor_params(params: &str) -> bool { - let frags = params.split(",").collect::<Vec<_>>(); + let frags: Vec<_> = params.split(',').map(str::trim).collect(); if frags.len() != 1 { return false; } - frags.first().unwrap().starts_with("ref self: ContractState") + frags.first().unwrap().trim().starts_with("ref self: ContractState") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/contract.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/dojo/lang/src/attribute_macros/contract.rs (1)
175-185: Ohayo, sensei! The constructor validation looks solid!The error message clearly explains why custom constructor parameters are discouraged and guides users toward using
dojo_initinstead.Cargo.toml (1)
215-217: Verify the Scarb dependency update.All Scarb dependencies are consistently updated to the same commit hash, which is good. However, let's verify this version.
✅ Verification successful
Ohayo sensei! The Scarb dependency update looks good 🍜
The update is a minor chore change fixing a constructor in the dojo plugin, with no breaking changes or reported issues. All dependencies are correctly pinned to the same commit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the Scarb commit for any breaking changes or issues # Fetch commit info gh api \ -H "Accept: application/vnd.github+json" \ repos/dojoengine/scarb/commits/b5ab351aa9b52dcf27a397021d84a423afd016dc \ --jq '.commit.message' # Check for any issues referencing this commit gh api \ -H "Accept: application/vnd.github+json" \ search/issues?q=b5ab351aa9b52dcf27a397021d84a423afd016dc+repo:dojoengine/scarb \ --jq '.items[].title'Length of output: 337
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2944 +/- ##
=======================================
Coverage 55.75% 55.75%
=======================================
Files 445 445
Lines 57627 57657 +30
=======================================
+ Hits 32129 32149 +20
- Misses 25498 25508 +10 ☔ View full report in Codecov by Sentry. |
Dojo contracts are always deployed with an empty calldata for constructor. This is done as such for two reasons:
This PR aims at ensuring the user is warned in case that a custom constructor is used. This is something possible, and some logic may be done in the constructor in some specific cases.
However, the recommended way to initialize a contract is adding the
dojo_initfunction, which ensures that all models and permissions are already done.Summary by CodeRabbit
Dependency Updates
scarb,scarb-metadata, andscarb-uidependencies to a new commit hash.Code Improvements