fix(sozo): rework the detection of Dojo default package #3327
Conversation
|
ohayo, sensei! WalkthroughAdds tracing as a workspace dependency. Updates default_dojo_package_name logic to detect dojo packages via starknet-contract targets’ build-external-contracts containing a specific world contract path. Implements two-tier candidate selection (non-lib vs. lib), with debug logging. Removes prior dojo_macros-based detection. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Metadata as scarb_metadata_ext::metadata
participant PkgIter as Packages
participant Targets as Package.targets
Caller->>Metadata: default_dojo_package_name()
activate Metadata
Metadata->>PkgIter: iterate all packages
loop for each package
PkgIter->>Targets: list targets
alt has starknet-contract target
Targets-->>Metadata: target with params
Metadata->>Metadata: check build-external-contracts for DOJO_WORLD_NAME
alt matches
Metadata->>Metadata: classify as non-lib or lib candidate
else no match
Metadata->>Metadata: ignore
end
else no relevant target
Metadata->>Metadata: ignore
end
end
Metadata->>Metadata: select package
alt exactly one non-lib candidate
Metadata-->>Caller: return that package
else no non-lib and exactly one lib
Metadata-->>Caller: return that package
else multiple candidates
Metadata-->>Caller: MultiplePackages([...])
else none
Metadata-->>Caller: NoPackage
end
deactivate Metadata
Note over Metadata: tracing::debug logs candidate sets and decision
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/sozo/scarb_metadata_ext/src/metadata.rs (4)
295-299: Typo: “virutal” → “virtual”Fix the doc comment spelling.
-/// In a virutal workspace, it is handy to have all the dependencies setup, but no `src` at the root +/// In a virtual workspace, it is handy to have all the dependencies set up, but no `src` at the root
342-353: Deduplicate candidates before resolvingMultiple matching starknet-contract targets in one package will push duplicate names and can trigger false “MultiplePackages”.
- debug!(?not_lib_candidates, ?lib_candidates, "Collect candidates for Sozo build."); + // Ensure uniqueness across targets + not_lib_candidates.sort(); + not_lib_candidates.dedup(); + lib_candidates.sort(); + lib_candidates.dedup(); + debug!(?not_lib_candidates, ?lib_candidates, "Collect candidates for Sozo build.");
323-336: Micro: avoid allocating a String for each contains checkCompare as_str() to DOJO_WORLD_NAME to skip constructing a serde_json::Value each iteration.
- if build_external_contracts - .contains(&serde_json::Value::String(DOJO_WORLD_NAME.to_string())) - { + let has_world = build_external_contracts.iter().any(|v| v.as_str() == Some(DOJO_WORLD_NAME)); + if has_world {
344-357: Selection rules read well; log message tweak (optional)Consider rewording logs to “Detect default dojo package” to reflect function intent; current “Collect candidates for Sozo build” is slightly misleading.
- debug!(?r, "Extract default dojo package name from workspace."); + debug!(?r, "Detect default dojo package from workspace.");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/dojo/dojo-snf-test/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/sozo/scarb_metadata_ext/Cargo.toml(1 hunks)crates/sozo/scarb_metadata_ext/src/metadata.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (3)
crates/sozo/scarb_metadata_ext/Cargo.toml (1)
19-19: Tracing dep addition looks goodDependency aligns with new debug! usage in metadata.rs. No action needed.
crates/sozo/scarb_metadata_ext/src/metadata.rs (2)
13-13: Importing tracing::debug is appropriateMatches the new debug logs; Cargo.toml already includes tracing via workspace.
24-24: Resolved: DOJO_WORLD_NAME is correct ohayo sensei — confirmed that"dojo::world::world_contract::world"is the canonical path emitted by build-external-contracts across supported Dojo versions.
| fn default_dojo_package_name(packages: &[PackageMetadata]) -> WorldPackageResult { | ||
| let mut candidates = Vec::new(); | ||
| let mut not_lib_candidates = Vec::new(); | ||
| let mut lib_candidates = Vec::new(); | ||
|
|
||
| for p in packages { | ||
| if p.targets.iter().all(|t| t.kind != "lib") { | ||
| for d in &p.dependencies { | ||
| if d.name == DOJO_MACROS_NAME { | ||
| candidates.push(p.name.clone()); | ||
| let is_lib = p.targets.iter().any(|t| t.kind == "lib"); | ||
|
|
||
| debug!(%p.name, is_lib, "Verifying package type."); | ||
|
|
||
| for t in &p.targets { | ||
| if t.kind == "starknet-contract" { | ||
| if let Some(build_external_contracts) = | ||
| t.params.as_object().and_then(|obj| obj.get("build-external-contracts")) | ||
| { | ||
| if let Some(build_external_contracts) = build_external_contracts.as_array() { | ||
| if build_external_contracts | ||
| .contains(&serde_json::Value::String(DOJO_WORLD_NAME.to_string())) | ||
| { | ||
| if is_lib { | ||
| lib_candidates.push(p.name.clone()); | ||
| } else { | ||
| not_lib_candidates.push(p.name.clone()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Limit detection to workspace members only
default_dojo_package_name scans all self.packages via caller, which commonly includes dependencies. This can pick a dependency’s package (or cause false “Multiple packages” errors).
- Filter to workspace members at the call site (workspace_package_name) before invoking default_dojo_package_name.
- If PackageMetadata exposes manifest_path, filter by starts_with(self.workspace.root). Otherwise, use an explicit workspace members list if available.
Example (outside this hunk, for workspace_package_name):
let root = &self.workspace.root;
let pkgs = self
.packages
.iter()
.filter(|p| p.manifest_path.starts_with(root));
match default_dojo_package_name(pkgs) { /* unchanged */ }To support that, generalize the helper signature in this hunk:
-fn default_dojo_package_name(packages: &[PackageMetadata]) -> WorldPackageResult {
+fn default_dojo_package_name<'a, I>(packages: I) -> WorldPackageResult
+where
+ I: IntoIterator<Item = &'a PackageMetadata>,
+{
- for p in packages {
+ for p in packages {
// unchanged
}Verification prompts:
🌐 Web query:
Does `scarb metadata`’s `packages` array include dependencies in addition to workspace members?
💡 Result:
Short answer: Yes — the metadata "packages" array contains the resolved packages for the project (workspace members plus their dependencies), not only workspace members. (docs.swmansion.com, docs.rs)
Citations:
- 1: https://docs.swmansion.com/scarb/docs/reference/compilation-model?utm_source=openai
- 2: https://docs.rs/scarb-metadata/latest/scarb_metadata/?utm_source=openai
Filter default_dojo_package_name inputs to workspace members only
ohayo, sensei! scarb metadata returns both workspace members and their dependencies (docs.swmansion.com); filter to avoid false “multiple packages” errors.
- At the call site (e.g., in
workspace_package_name), do:
let root = &self.workspace.root;
let pkgs = self
.packages
.iter()
.filter(|p| p.manifest_path.starts_with(root));
match default_dojo_package_name(pkgs) { /* unchanged */ }- Generalize the helper signature:
-fn default_dojo_package_name(packages: &[PackageMetadata]) -> WorldPackageResult {
+fn default_dojo_package_name<'a, I>(packages: I) -> WorldPackageResult
+where
+ I: IntoIterator<Item = &'a PackageMetadata>,
{
// unchanged
}🤖 Prompt for AI Agents
crates/sozo/scarb_metadata_ext/src/metadata.rs around lines 312-340:
default_dojo_package_name currently inspects all packages returned by `scarb
metadata` (which includes dependencies) causing false multiple-package errors;
change it to operate only on workspace members by (A) generalizing the function
signature to accept an iterator or IntoIterator over &PackageMetadata (e.g.,
impl IntoIterator<Item = &'a PackageMetadata> or similar) instead of a
&[PackageMetadata], and update the function body to iterate that iterator; and
(B) update call sites (e.g., workspace_package_name) to filter self.packages to
workspace members before calling the helper by checking that
package.manifest_path.starts_with(self.workspace.root) and pass the filtered
iterator into default_dojo_package_name. Ensure all call sites are adjusted to
the new signature.
|
|
||
| debug!(%p.name, is_lib, "Verifying package type."); | ||
|
|
||
| for t in &p.targets { |
There was a problem hiding this comment.
Potential compile error in tracing field shorthand
debug!(%p.name, ...) is not valid shorthand because p.name isn’t an identifier. Give the field an explicit key.
- debug!(%p.name, is_lib, "Verifying package type.");
+ debug!(package = %p.name, is_lib, "Verifying package type.");📝 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.
| debug!(%p.name, is_lib, "Verifying package type."); | |
| for t in &p.targets { | |
| debug!(package = %p.name, is_lib, "Verifying package type."); | |
| for t in &p.targets { |
🤖 Prompt for AI Agents
In crates/sozo/scarb_metadata_ext/src/metadata.rs around lines 318 to 321, the
tracing macro uses shorthand debug!(%p.name, ...) which is invalid because
p.name is not an identifier; change the shorthand to explicit key/value pairs
(for example name = %p.name and is_lib = is_lib) so the call becomes debug!(name
= %p.name, is_lib = is_lib, "Verifying package type."); ensure all fields passed
to debug! use explicit keys.
Sozo requires a package name to generate the binding generation if required.
In a workspace, multiple packages may use dojo as a dependency. This PR ensures that we are looking for package that are targeting a starknet contract to be deployed + using the world as an external contract.
Some libraries like budokan are actually
[lib], but are also deployed in their own world. To support that, Sozo first check for non library packages, and fallback on library projects afterward.The rule is still the same for non library and library packages, only one world is supported at the moment.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores