chore: bump to cairo 2.12.2 - #3335
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughOhayo, sensei! This PR updates Scarb/Cairo/StarkNet versions, replaces git pins with versioned cairo-lang crates, removes several lifetimes in macros helpers while adjusting call-site borrowing, syncs Scarb versioning to include [workspace.package], updates example/test metadata and policy JSON entries. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/spawn-and-move/external/Scarb.toml (1)
3-7: Ohayo sensei — example pins StarkNet 2.11.4; likely incompatible with 2.12.2 workspaceBuild can fail or resolve oddly when mixing 2.11.x with 2.12.2.
Propose:
[package] name = "hello" version = "1.7.0-alpha.3" edition = "2024_07" [dependencies] -starknet = "2.11.4" +starknet = "2.12.2"crates/dojo/dojo-snf-test/Scarb.toml (1)
5-12: Ohayo sensei — align Cairo/StarkNet/snforge_std/assert_macros to 2.12.2/0.48.1 for toolchain consistencycrates/dojo/dojo-snf-test/Scarb.toml currently pins cairo-version = "2.12.0", starknet = "2.12.0", snforge_std = "0.48.0", assert_macros = "2.12.0" — bump to:
- cairo-version = "2.12.2"
- starknet = "2.12.2"
- snforge_std = "0.48.1"
- assert_macros = "2.12.2"
Repo already targets 2.12.2 / 0.48.1 (.tool-versions, Cargo.toml, examples, core-tests); mixed patch levels can break workspace builds/tests.
🧹 Nitpick comments (8)
crates/dojo/core-tests/Scarb.toml (1)
5-18: Pin to exact 2.12.2 for deterministic CI buildsCurrent "~2.12" style can drift under new patch releases.
Suggested:
version = "1.7.0-alpha.3" edition = "2024_07" -cairo-version = "2.12" +cairo-version = "2.12.2" [dependencies] -starknet = "2.12" +starknet = "2.12.2" [dev-dependencies] snforge_std = "0.48.1" -assert_macros = "2.12" +assert_macros = "2.12.2"crates/dojo/dojo-cairo-test/Scarb.toml (1)
5-12: Ohayo sensei — bump manifest pins to exact 2.12.2Match workspace’s 2.12.2 and avoid resolver drift.
version = "1.7.0-alpha.3" edition = "2024_07" -cairo-version = "2.12" +cairo-version = "2.12.2" [dependencies] -starknet = "2.12" -cairo_test = "2.12" +starknet = "2.12.2" +cairo_test = "2.12.2"examples/game-lib/bestiary/Scarb.toml (1)
3-15: Ohayo sensei — minor: dev test crate should follow 2.12.2Small polish for consistency with toolchain pin.
[dev-dependencies] -cairo_test = "2.12" +cairo_test = "2.12.2" dojo_cairo_test = { path = "../../../crates/dojo/dojo-cairo-test" }crates/dojo/macros/src/derives/introspect/ty.rs (1)
21-24: Optional: avoid "ERROR" sentinel; return Result insteadIf this bubbles into codegen, a typed error is safer than
"ERROR".to_string().-pub fn build_ty_from_type_clause(db: &dyn SyntaxGroup, type_clause: &TypeClause) -> String { +pub fn build_ty_from_type_clause(db: &dyn SyntaxGroup, type_clause: &TypeClause) -> Result<String, TyError> { match type_clause.ty(db) { ... - _ => { - // diagnostic message already handled in layout building - "ERROR".to_string() - } + _ => Err(TyError::Unsupported(type_clause.clone())), } }crates/dojo/macros/src/derives/introspect/layout.rs (1)
80-81: Ohayo, sensei — Borrowing avoids extra allocs; add consistent trimming.Path arm trims, tuple/array arms don’t. To be safe, trim these too.
Apply this diff:
- let tuple_type = expr.as_syntax_node().get_text_without_trivia(db); - get_packed_tuple_layout_from_type(diagnostics, &tuple_type) + let tuple_type = expr.as_syntax_node().get_text_without_trivia(db); + get_packed_tuple_layout_from_type(diagnostics, tuple_type.trim()) @@ - let arr_type = expr.as_syntax_node().get_text_without_trivia(db); - get_packed_item_layout_from_type(diagnostics, &arr_type) + let arr_type = expr.as_syntax_node().get_text_without_trivia(db); + get_packed_item_layout_from_type(diagnostics, arr_type.trim())Also applies to: 84-85
crates/dojo/macros/src/derives/introspect/structs.rs (1)
164-165: Ohayo, sensei — Preferexpect(...)over bareunwrap().Keeps better diagnostics if selector computation ever fails.
- let field_selector = get_selector_from_name(&field_name).unwrap(); + let field_selector = + get_selector_from_name(&field_name).expect("valid field name for selector");crates/dojo/macros/src/helpers/misc.rs (1)
28-39: Ohayo, sensei — Avoid intermediate Vec inextend.Stream the iterator directly to cut one allocation.
- hashes.extend( - members - .map(|m| { - poseidon_hash_many(&[ - naming::compute_bytearray_hash(&m.name(db).text(db)), - naming::compute_bytearray_hash( - &m.type_clause(db).ty(db).as_syntax_node().get_text_without_trivia(db), - ), - ]) - }) - .collect::<Vec<_>>(), - ); + hashes.extend(members.map(|m| { + poseidon_hash_many(&[ + naming::compute_bytearray_hash(&m.name(db).text(db)), + naming::compute_bytearray_hash( + &m.type_clause(db).ty(db).as_syntax_node().get_text_without_trivia(db), + ), + ]) + }));scripts/scarb_version_sync.sh (1)
83-97: Preserve indentation when updating version lines.Current substitution drops leading spaces. Keep original indent for cleaner diffs.
- awk -v ver="$VERSION" ' + awk -v ver="$VERSION" ' BEGIN { in_pkg=0 } /^[[:space:]]*\[package\][[:space:]]*$/ { in_pkg=1; print; next } /^[[:space:]]*\[workspace\.package\][[:space:]]*$/ { in_pkg=1; print; next } /^[[:space:]]*\[[^]]+\][[:space:]]*$/ && $0 !~ /^\[(package|workspace\.package)\]/ { in_pkg=0; print; next } { if (in_pkg && $0 ~ /^[[:space:]]*version[[:space:]]*=/) { - sub(/^[[:space:]]*version[[:space:]]*=[[:space:]]*".*"/, "version = \"" ver "\"") - print + match($0, /^[[:space:]]*/); indent = substr($0, RSTART, RLENGTH) + $0 = indent "version = \"" ver "\"" + print } else { print } } ' "$file" > "$tmp"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/dojo/core/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.tool-versions(1 hunks)Cargo.toml(1 hunks)crates/dojo/core-tests/Scarb.toml(1 hunks)crates/dojo/core/Scarb.toml(1 hunks)crates/dojo/dojo-cairo-test/Scarb.toml(1 hunks)crates/dojo/dojo-snf-test/Scarb.toml(1 hunks)crates/dojo/macros/src/derives/dojo_store.rs(1 hunks)crates/dojo/macros/src/derives/introspect/layout.rs(1 hunks)crates/dojo/macros/src/derives/introspect/size.rs(1 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(2 hunks)crates/dojo/macros/src/derives/introspect/ty.rs(1 hunks)crates/dojo/macros/src/helpers/checker.rs(1 hunks)crates/dojo/macros/src/helpers/formatter.rs(2 hunks)crates/dojo/macros/src/helpers/misc.rs(2 hunks)crates/dojo/macros/src/helpers/parser.rs(5 hunks)examples/game-lib/Scarb.toml(1 hunks)examples/game-lib/armory/Scarb.toml(1 hunks)examples/game-lib/bestiary/Scarb.toml(1 hunks)examples/simple/Scarb.toml(2 hunks)examples/spawn-and-move/Scarb.toml(2 hunks)examples/spawn-and-move/external/Scarb.toml(1 hunks)scripts/cairo_fmt.sh(0 hunks)scripts/scarb_version_sync.sh(4 hunks)
💤 Files with no reviewable changes (1)
- scripts/cairo_fmt.sh
🧰 Additional context used
🧬 Code graph analysis (4)
crates/dojo/macros/src/helpers/checker.rs (1)
crates/dojo/macros/src/helpers/parser.rs (1)
attrs(124-147)
crates/dojo/macros/src/helpers/misc.rs (3)
crates/dojo/macros/src/helpers/parser.rs (1)
members(75-104)crates/dojo/macros/src/attributes/event.rs (1)
members(83-92)crates/dojo/types/src/naming.rs (1)
compute_bytearray_hash(84-87)
crates/dojo/macros/src/helpers/formatter.rs (2)
crates/dojo/macros/src/helpers/misc.rs (1)
members(29-38)crates/dojo/macros/src/helpers/parser.rs (1)
members(75-104)
crates/dojo/macros/src/helpers/parser.rs (2)
crates/dojo/macros/src/helpers/misc.rs (1)
members(29-38)crates/dojo/macros/src/attributes/event.rs (1)
members(83-92)
⏰ 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: clippy
🔇 Additional comments (23)
.tool-versions (1)
1-1: Ohayo sensei — LGTM on pinning Scarb 2.12.2examples/simple/Scarb.toml (1)
2-20: Ohayo sensei — nice, fully aligned to 2.12.2All pins look consistent; no action needed.
crates/dojo/macros/src/derives/introspect/ty.rs (1)
7-20: Ohayo sensei — borrowing change is correct and avoids needless allocationsPassing
&path_type/&tuple_type/&arr_typeinto&strAPI is spot on.examples/spawn-and-move/Scarb.toml (2)
4-6: Ohayo, sensei — Version/toolchain bumps look consistent.1.7.0-alpha.3 + cairo =2.12.2 align with the PR intent. LGTM.
Please run the repo’s version sync script and a quick
snforge teston this example to confirm no lockfile drift after the bump.
14-24: Ohayo, sensei — Dep pins match Cairo 2.12.2.
starknet = 2.12.2andassert_macros = 2.12.2are aligned. No issues spotted.crates/dojo/core/Scarb.toml (1)
6-6: Ohayo, sensei — Package version bump is clean.
dojo→ 1.7.0‑alpha.3 matches workspace updates. LGTM.examples/game-lib/Scarb.toml (1)
8-8: Ohayo, sensei — Workspace package version synced.Patch to 1.7.0‑alpha.3 looks good.
crates/dojo/macros/src/derives/introspect/structs.rs (1)
155-162: Ohayo, sensei — Borrow-by-ref change is good.Passing
&member_typereduces copies and matches the helper signature.crates/dojo/macros/src/helpers/checker.rs (1)
18-22: Ohayo, sensei — Lifetime removal on attrs is tidy.Signature simplification aligns with parser helpers. LGTM.
Confirm all call sites now pass
impl Iterator<Item = Attribute>without extra clones.crates/dojo/macros/src/helpers/misc.rs (1)
18-23: Ohayo, sensei — API simplification looks good.Dropping the lifetime and owning
Memberiterator matches upstream changes.crates/dojo/macros/src/derives/introspect/size.rs (1)
24-37: Ohayo, sensei — borrowed &str passing looks correct and keeps allocations minimal.No functional change; Deref coercions from String to &str are sound here.
scripts/scarb_version_sync.sh (3)
3-12: Ohayo, sensei — usage/docs now reflect [package] and [workspace.package].Messaging updates are clear and accurate.
50-61: Dual-scope check logic is solid.Section enter/exit handling for [package] and [workspace.package] is correct and portable awk.
142-143: Success message now mentions both sections — nice.Good UX touch.
crates/dojo/macros/src/helpers/formatter.rs (2)
57-58: Ohayo, sensei — passing &member_ty aligns with the signature; good borrow hygiene.
72-75: Public API tweak: Iterator<Item = MemberAst> (no lifetime).Looks compatible with recent cairo AST types; confirm all internal callers updated.
Optional consistency follow-up: consider changing
serialize_primitive_member_ty(member_name: &String, ...)to accept&strto match the rest of the API.crates/dojo/macros/src/derives/dojo_store.rs (1)
55-57: Ohayo, sensei — borrow&member_tyinto deserializer is correct.Keeps
member_tyavailable for later use and matches the callee’s&strAPI.crates/dojo/macros/src/helpers/parser.rs (5)
16-20: Ohayo, sensei — returning owningast::ItemStructsimplifies lifetimes.
33-37: Owningast::ItemModule>return mirrors the struct change — consistent.
51-55: Inline args now return owningExprParenthesized— LGTM.
67-75: Iterator<Item = MemberAst> input is a nice ergonomics win.Flow and diagnostics behavior remain unchanged.
120-123: Attr iterator without lifetimes looks good.Minor note: cloning Attribute is fine here; no action needed.
Cargo.toml (1)
105-127: Ohayo, sensei — Cairo toolchain bump looks good (target 2.12.2); verify no stray cairo-lang deps remain.*Automated sandbox checks failed (rg/PCRE or /dev/fd errors). Run locally and report any non-2.12.2 entries:
# list all occurrences rg -n --hidden 'cairo-lang-' -g '!**/Cargo.lock' -g '!target/**' || true# detect cairo-lang deps whose version != 2.12.2 python3 - <<'PY' import re,glob pat=re.compile(r'^\s*(cairo-lang-[\w-]+)\s*=\s*(?:"([^"]+)"|\{[^}]*version\s*=\s*"([^"]+)"[^}]*\})') for fn in glob.glob('**/Cargo.toml', recursive=True): if '/target/' in fn or fn.endswith('Cargo.lock'): continue with open(fn, encoding='utf-8') as fh: for i,l in enumerate(fh,1): m=pat.match(l) if m: v=m.group(2) or m.group(3) if v and v!='2.12.2': print(f"{fn}:{i}: {m.group(1)} = \"{v}\"") PY
| scarb 2.12.2 | ||
| starknet-foundry 0.48.1 | ||
| cairo-profiler 0.9.0 | ||
| katana 1.7.0-alpha.3 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick repo-wide drift check script
Run to spot non-2.12.2 leftovers (cairo-version, starknet, cairo_test, snforge_std).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "cairo-version not at 2.12.2:"
rg -nH --glob '**/Scarb.toml' '^\s*cairo-version\s*=\s*".*"' | grep -v '2\.12\.2' || echo "OK"
echo -e "\nstarknet dep not at 2.12.2:"
rg -nH --glob '**/Scarb.toml' '^\s*starknet\s*=\s*".*"' | grep -v '2\.12\.2' || echo "OK"
echo -e "\nsnforge_std versions found:"
rg -nH --glob '**/Scarb.toml' '^\s*snforge_std\s*=\s*".*"'
echo -e "\ncairo_test not at 2.12.2:"
rg -nH --glob '**/Scarb.toml' '^\s*cairo_test\s*=\s*".*"' | grep -v '2\.12\.2' || echo "OK"Length of output: 1114
ohayo sensei — version drift detected: align Scarb cairo-version / starknet / cairo_test to 2.12.2 and snforge_std to 0.48.1
Script run — mismatches found:
- cairo-version not at 2.12.2:
- examples/game-lib/Scarb.toml:11:cairo-version = "2.12"
- crates/dojo/core/Scarb.toml:2:cairo-version = "2.12"
- crates/dojo/dojo-cairo-test/Scarb.toml:7:cairo-version = "2.12"
- crates/dojo/dojo-snf-test/Scarb.toml:7:cairo-version = "2.12.0"
- crates/dojo/core-tests/Scarb.toml:7:cairo-version = "2.12"
- starknet dep not at 2.12.2:
- examples/spawn-and-move/external/Scarb.toml:7:starknet = "2.11.4"
- examples/game-lib/Scarb.toml:15:starknet = "2.12"
- crates/dojo/dojo-snf-test/Scarb.toml:10:starknet = "2.12.0"
- crates/dojo/dojo-cairo-test/Scarb.toml:10:starknet = "2.12"
- crates/dojo/core-tests/Scarb.toml:10:starknet = "2.12"
- crates/dojo/core/Scarb.toml:10:starknet = "2.12"
- snforge_std versions found:
- examples/spawn-and-move/Scarb.toml:22:snforge_std = "0.48.0"
- crates/dojo/dojo-snf-test/Scarb.toml:11:snforge_std = "0.48.0"
- crates/dojo/core-tests/Scarb.toml:17:snforge_std = "0.48.1"
- cairo_test not at 2.12.2:
- examples/game-lib/bestiary/Scarb.toml:14:cairo_test = "2.12"
- examples/game-lib/armory/Scarb.toml:14:cairo_test = "2.12"
- crates/dojo/dojo-cairo-test/Scarb.toml:11:cairo_test = "2.12"
Update these entries to 2.12.2 (and unify snforge_std to 0.48.1 to match .tool-versions) or document justified exceptions.
🤖 Prompt for AI Agents
In .tool-versions lines 1-4 and across the repo files listed, update mismatched
Scarb/Cairo/Starknet/snforge_std/cairo_test versions to match .tool-versions:
set cairo-version and cairo_test to "2.12.2" in examples/game-lib/Scarb.toml
(line 11), crates/dojo/core/Scarb.toml (line 2),
crates/dojo/dojo-cairo-test/Scarb.toml (line 7),
crates/dojo/dojo-snf-test/Scarb.toml (line 7), and
crates/dojo/core-tests/Scarb.toml (line 7); set starknet to "2.12.2" in
examples/spawn-and-move/external/Scarb.toml (line 7),
examples/game-lib/Scarb.toml (line 15), crates/dojo/dojo-snf-test/Scarb.toml
(line 10), crates/dojo/dojo-cairo-test/Scarb.toml (line 10),
crates/dojo/core-tests/Scarb.toml (line 10), and crates/dojo/core/Scarb.toml
(line 10); set snforge_std to "0.48.1" in examples/spawn-and-move/Scarb.toml
(line 22), crates/dojo/dojo-snf-test/Scarb.toml (line 11), and ensure any
differing entry like crates/dojo/core-tests/Scarb.toml (line 17) is aligned or
documented as an exception; run the repo-wide search to verify no other
Scarb.toml entries remain at older versions.
| [package] | ||
| name = "armory" | ||
| version = "1.7.0-alpha.2" | ||
| version = "1.7.0-alpha.3" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version bump LGTM; please confirm workspace/Cairo pin alignment.
Ohayo, sensei! The package version change to 1.7.0‑alpha.3 looks consistent. Since the PR targets Cairo/Scarb 2.12.2, please verify:
- Workspace
cairo-versionis set to "2.12.2" and this crate inherits it viacairo-version.workspace = true. .tool-versionspinsscarb 2.12.2.- Optional: consider pinning
cairo_testto "2.12.2" for fully reproducible dev builds.
Run this to double-check:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workspace cairo-version (expect 2.12.2):"
rg -nP '^\s*cairo-version\s*=\s*"\d+\.\d+\.\d+"' -g 'Scarb.toml' -C2
echo "All member crates inherit cairo-version from workspace:"
rg -nP '^\s*cairo-version\.workspace\s*=\s*true' -g '**/Scarb.toml' | wc -l
rg -nP '^\s*cairo-version\s*=\s*"\d' -g '**/Scarb.toml' -v || true
echo ".tool-versions scarb pin (expect 2.12.2):"
test -f .tool-versions && rg -nP '^scarb\s+2\.12\.2$' .tool-versions || echo "No .tool-versions or mismatch"
echo "cairo_test versions across manifests (optionally pin to 2.12.2):"
rg -nP '^\s*cairo_test\s*=\s*"\d+\.\d+(\.\d+)?"' -g '**/Scarb.toml' -C1Length of output: 13364
Version bump LGTM — align Cairo/Starknet pins
Ohayo, sensei!
- .tool-versions: scarb 2.12.2 — OK.
- Action: add cairo-version = "2.12.2" to examples/game-lib/Scarb.toml (workspace). armory and bestiary set cairo-version.workspace = true but the workspace has no cairo-version.
- Action: update crates/dojo/dojo-snf-test/Scarb.toml (cairo-version = "2.12.0") → "2.12.2" or switch it to workspace inheritance.
- Action: pin cairo_test dev-deps to "2.12.2" in examples/game-lib/armory, examples/game-lib/bestiary, and crates/dojo/dojo-cairo-test (they currently use "2.12").
- Action: review external example examples/spawn-and-move/external/Scarb.toml (starknet = "2.11.4") for compatibility with 2.12.2.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (18)
crates/dojo/macros/src/helpers/checker.rs (1)
25-31: Avoid allocs in contains-check.Compare as
&strto avoidto_string()churn, or precompute aHashSet<&str>for O(1) membership. Example:- if attr_names.contains(&DOJO_INTROSPECT_DERIVE.to_string()) - && attr_names.contains(&DOJO_PACKED_DERIVE.to_string()) + let has = |s: &str| attr_names.iter().any(|n| n == s); + if has(DOJO_INTROSPECT_DERIVE) && has(DOJO_PACKED_DERIVE)crates/dojo/macros/src/helpers/misc.rs (1)
29-39: Drop the intermediate Vec in extend.You can extend from the iterator directly to save one allocation.
- hashes.extend( - members - .map(|m| { - poseidon_hash_many(&[ - naming::compute_bytearray_hash(&m.name(db).text(db)), - naming::compute_bytearray_hash( - &m.type_clause(db).ty(db).as_syntax_node().get_text_without_trivia(db), - ), - ]) - }) - .collect::<Vec<_>>(), - ); + hashes.extend(members.map(|m| { + poseidon_hash_many(&[ + naming::compute_bytearray_hash(&m.name(db).text(db)), + naming::compute_bytearray_hash( + &m.type_clause(db).ty(db).as_syntax_node().get_text_without_trivia(db), + ), + ]) + }));crates/dojo/macros/src/helpers/parser.rs (1)
67-105: Key-order checking is correct; minor nit on construction.Logic reads clean. If you want, preallocate when possible by threading a length hint (not required).
crates/dojo/macros/src/derives/introspect/size.rs (1)
21-37: Ohayo, sensei — pass‑by‑ref changes are solid.Tiny nit: if the API exposes an
is_empty, prefer it overlen() == 0for tuples; otherwise keep as-is.crates/dojo/macros/src/helpers/formatter.rs (3)
72-88: Generalize the iterator for better ergonomics.Accept
IntoIteratorto support slices, Vecs, and iterators without extra.into_iter()at call sites.- pub fn serialize_keys_and_values( - db: &dyn SyntaxGroup, - members: impl Iterator<Item = MemberAst>, + pub fn serialize_keys_and_values( + db: &dyn SyntaxGroup, + members: impl IntoIterator<Item = MemberAst>, serialized_keys: &mut Vec<String>, serialized_values: &mut Vec<String>, use_serde: bool, ) { - members.for_each(|member| { + for member in members.into_iter() { let serialized = Self::serialize_member_ty(db, &member, true, use_serde); if member.has_attr(db, "key") { serialized_keys.push(serialized); } else { serialized_values.push(serialized); } - }); + } }
28-36: Prefer &str over &String and avoid an allocation.Make
serialize_primitive_member_tytake&str, then pass a borrowed name directly (no.to_string()).- let member_name = member_ast.name(db).text(db).to_string(); - Self::serialize_primitive_member_ty(&member_name, with_self, use_serde) + let member_name = member_ast.name(db).text(db); + Self::serialize_primitive_member_ty(&member_name, with_self, use_serde)- pub(crate) fn serialize_primitive_member_ty( - member_name: &String, + pub(crate) fn serialize_primitive_member_ty( + member_name: &str, with_self: bool, use_serde: bool, ) -> String {Run a quick grep to confirm other call sites (if any) still compile after the signature tweak.
132-134: Tiny clone nit: avoid intermediate Vec.Use iterator cloning instead of
to_vec()to skip one allocation.- if !gen_types.is_empty() { - gen_impls.extend(additional_impls.to_vec()); - } + if !gen_types.is_empty() { + gen_impls.extend(additional_impls.iter().cloned()); + }examples/game-lib/armory/Scarb.toml (1)
3-3: Ohayo, sensei — pin dev cairo_test to 2.12.2 for determinism.Most manifests in this PR pin to 2.12.2. Here it’s "2.12" (floating patch). Recommend pinning to avoid CI drift.
[dev-dependencies] -cairo_test = "2.12" +cairo_test = "2.12.2" dojo_cairo_test = { path = "../../../crates/dojo/dojo-cairo-test" }crates/dojo/dojo-cairo-test/Scarb.toml (1)
5-5: Ohayo, sensei — align cairo-version and deps to 2.12.2.This crate still uses "2.12" (un-pinned) for cairo-version/starknet/cairo_test. For a 2.12.2 bump, pin to 2.12.2 to match the workspace and tools.
version = "1.7.0-alpha.3" edition = "2024_07" -cairo-version = "2.12" +cairo-version = "2.12.2" [dependencies] -starknet = "2.12" -cairo_test = "2.12" +starknet = "2.12.2" +ca iro_test = "2.12.2" dojo = { path = "../core" }crates/sozo/mcp/tests/stdio_tests.rs (1)
72-72: Consider increasing 5s STDIO timeouts to reduce flakiness on cold builds.Initial spawn/initialize and first reads frequently exceed 5s in CI. Suggest 15–30s for init/list/read, similar to the 30s used for build.
- let response = timeout(Duration::from_secs(5), self.send_request(init_request)).await??; + let response = timeout(Duration::from_secs(20), self.send_request(init_request)).await??;Apply similarly to other 5s timeouts in this file.
Also applies to: 129-129, 156-156, 189-189, 224-224
examples/game-lib/bestiary/Scarb.toml (1)
3-3: Ohayo, sensei — pin dev cairo_test to 2.12.2.Same rationale as armory: avoid floating "2.12".
[dev-dependencies] -cairo_test = "2.12" +cairo_test = "2.12.2" dojo_cairo_test = { path = "../../../crates/dojo/dojo-cairo-test" }examples/spawn-and-move/Scarb.toml (1)
4-4: Optional: bump snforge_std to match .tool-versions (0.48.1).Currently dev-deps use snforge_std "0.48.0". Aligning to 0.48.1 avoids plugin/tooling mismatch.
[dev-dependencies] -snforge_std = "0.48.0" +snforge_std = "0.48.1" assert_macros = "2.12.2"crates/dojo/core-tests/Scarb.toml (1)
17-19: Optional: pin assert_macros to 2.12.2.Minor consistency tweak with other manifests.
snforge_std = "0.48.1" -assert_macros = "2.12" +assert_macros = "2.12.2"scripts/scarb_version_sync.sh (4)
15-15: Consider failing fast: add -e to the strict modeYou already use -u and -o pipefail; adding -e improves failure visibility without breaking the conditional checks used below.
-set -uo pipefail +set -euo pipefail
60-60: Make section exit check whitespace‑robustMinor: the negative match doesn’t allow leading spaces, unlike the positive matches above. Safe today due to ordering, but tightening avoids future footguns.
-/^[[:space:]]*\[[^]]+\][[:space:]]*$/ && $0 !~ /^\[(package|workspace\.package)\]/ { in_pkg=0 } +/^[[:space:]]*\[[^]]+\][[:space:]]*$/ && $0 !~ /^[[:space:]]*\[(package|workspace\.package)\]/ { in_pkg=0 }
83-97: Preserve trailing comments when updating version linesCurrent replace strips end‑of‑line comments. Preserve them to avoid losing useful notes.
- awk -v ver="$VERSION" ' + awk -v ver="$VERSION" ' BEGIN { in_pkg=0 } /^[[:space:]]*\[package\][[:space:]]*$/ { in_pkg=1; print; next } /^[[:space:]]*\[workspace\.package\][[:space:]]*$/ { in_pkg=1; print; next } - /^[[:space:]]*\[[^]]+\][[:space:]]*$/ && $0 !~ /^\[(package|workspace\.package)\]/ { in_pkg=0; print; next } + /^[[:space:]]*\[[^]]+\][[:space:]]*$/ && $0 !~ /^[[:space:]]*\[(package|workspace\.package)\]/ { in_pkg=0; print; next } { if (in_pkg && $0 ~ /^[[:space:]]*version[[:space:]]*=/) { - sub(/^[[:space:]]*version[[:space:]]*=[[:space:]]*".*"/, "version = \"" ver "\"") - print + # Preserve trailing content after the closing quote + line = $0 + q1 = index(line, "\"") + if (q1 > 0) { + rest = substr(line, q1+1) + q2 = index(rest, "\"") + if (q2 > 0) { + trailing = substr(rest, q2+1) # everything after the closing quote + print substr(line, 1, q1-1) "version = \"" ver "\"" trailing + } else { + print "version = \"" ver "\"" + } + } else { + print "version = \"" ver "\"" + } } else { print } } ' "$file" > "$tmp"
142-143: Echo final status per mode and rootTiny UX polish: include $ROOT and mode in the success line to help CI logs.
-echo "All Scarb.toml [package].version and [workspace.package].version match $VERSION" +echo "All Scarb.toml under $ROOT match $VERSION ([package] and [workspace.package])"Cargo.toml (1)
88-90: Keep the katana runner pin documentedGit pin kept (rpc 0.9 branch). Consider opening a tracking issue to move to a released version when available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/dojo/core/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.github/workflows/release-dispatch.yml(1 hunks).tool-versions(1 hunks)Cargo.toml(1 hunks)crates/dojo/core-tests/Scarb.toml(1 hunks)crates/dojo/core/Scarb.toml(1 hunks)crates/dojo/dojo-cairo-test/Scarb.toml(1 hunks)crates/dojo/dojo-snf-test/Scarb.toml(1 hunks)crates/dojo/macros/src/derives/dojo_store.rs(1 hunks)crates/dojo/macros/src/derives/introspect/layout.rs(1 hunks)crates/dojo/macros/src/derives/introspect/size.rs(1 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(2 hunks)crates/dojo/macros/src/derives/introspect/ty.rs(1 hunks)crates/dojo/macros/src/helpers/checker.rs(1 hunks)crates/dojo/macros/src/helpers/formatter.rs(2 hunks)crates/dojo/macros/src/helpers/misc.rs(2 hunks)crates/dojo/macros/src/helpers/parser.rs(5 hunks)crates/sozo/mcp/tests/stdio_tests.rs(1 hunks)examples/game-lib/Scarb.toml(1 hunks)examples/game-lib/armory/Scarb.toml(1 hunks)examples/game-lib/bestiary/Scarb.toml(1 hunks)examples/simple/Scarb.toml(2 hunks)examples/spawn-and-move/Scarb.toml(2 hunks)examples/spawn-and-move/external/Scarb.toml(1 hunks)scripts/cairo_fmt.sh(0 hunks)scripts/scarb_version_sync.sh(4 hunks)
💤 Files with no reviewable changes (1)
- scripts/cairo_fmt.sh
🧰 Additional context used
🧬 Code graph analysis (4)
crates/dojo/macros/src/helpers/checker.rs (1)
crates/dojo/macros/src/helpers/parser.rs (1)
attrs(124-147)
crates/dojo/macros/src/helpers/misc.rs (3)
crates/dojo/macros/src/helpers/parser.rs (1)
members(75-104)crates/dojo/macros/src/attributes/event.rs (1)
members(83-92)crates/dojo/types/src/naming.rs (1)
compute_bytearray_hash(84-87)
crates/dojo/macros/src/helpers/parser.rs (2)
crates/dojo/macros/src/helpers/misc.rs (1)
members(29-38)crates/dojo/macros/src/attributes/event.rs (1)
members(83-92)
crates/dojo/macros/src/helpers/formatter.rs (2)
crates/dojo/macros/src/helpers/misc.rs (1)
members(29-38)crates/dojo/macros/src/helpers/parser.rs (1)
members(75-104)
⏰ 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: clippy
🔇 Additional comments (23)
crates/dojo/macros/src/helpers/misc.rs (1)
18-23: Ohayo, sensei — API tweak is consistent with the lifetime-free ASTs.
members: impl Iterator<Item = Member>fits the new pattern.crates/dojo/macros/src/helpers/parser.rs (2)
16-20: Ohayo, sensei — owned AST return types LGTM.Returning owned
ast::Item*nodes simplifies usage and removes lifetime noise.Also applies to: 33-37, 51-55
119-148: Prefer borrowingAttributeand usingflat_mapohayo sensei — I couldn't verify whether
structurizeaccepts&Attribute; if it does, change the iterator toItem = &Attributeand remove theclone(). Replacefilter_map(...).flatten()withflat_mapfor clarity.Location: crates/dojo/macros/src/helpers/parser.rs:119-148
crates/dojo/macros/src/derives/dojo_store.rs (1)
54-57: Ohayo, sensei — borrowingmember_tyis the right move.Matches the updated formatter API; no functional change.
crates/dojo/macros/src/derives/introspect/ty.rs (1)
9-20: Ohayo, sensei — pass‑by‑ref cleanup LGTM.Borrowing the local
Strings intobuild_item_ty_from_type(&str)avoids moves without changing behavior.crates/dojo/macros/src/derives/introspect/structs.rs (1)
151-161: usize check on key types: good guard.Borrowing the member type into
type_contains_usizeis tidy and avoids needless clones.crates/dojo/macros/src/derives/introspect/layout.rs (1)
79-85: Normalize whitespace like the path branch.You
trim()forExpr::Pathbut not for tuple/array; normalize for consistency and to avoid stray spaces.- let tuple_type = expr.as_syntax_node().get_text_without_trivia(db); - get_packed_tuple_layout_from_type(diagnostics, &tuple_type) + let tuple_type = expr.as_syntax_node().get_text_without_trivia(db); + get_packed_tuple_layout_from_type(diagnostics, tuple_type.trim())- let arr_type = expr.as_syntax_node().get_text_without_trivia(db); - get_packed_item_layout_from_type(diagnostics, &arr_type) + let arr_type = expr.as_syntax_node().get_text_without_trivia(db); + get_packed_item_layout_from_type(diagnostics, arr_type.trim())[suggest_minor_issue]
crates/dojo/macros/src/helpers/checker.rs (1)
18-22: Ohayo, sensei — signature simplification looks good; confirm structurize's ownership before switching to &Attribute.Found
attr.clone().structurize(db).argsin crates/dojo/macros/src/helpers/parser.rs (~line 126), which indicatesstructurizeis currently used by-value;AttributeStructurize's definition is external to this repo — verify the trait's signature in the cairo_lang_syntax dependency or update callers before changing toIterator<Item = &Attribute>.crates/dojo/macros/src/helpers/formatter.rs (2)
57-58: Ohayo, sensei — borrow change looks right; double‑check deref to &str.
member_tynow passes by reference. Ensure the concrete type fromget_text_without_trivia(db)deref‑coerces to&str. If it doesn’t, switch the call tomember_ty.as_str()(or fall back tomember_ty.to_string()and borrow).
72-75: LGTM on the API simplification.Dropping the lifetime and using
Iterator<Item = MemberAst>aligns with the lifetime‑free AST across the PR.crates/sozo/mcp/tests/stdio_tests.rs (1)
236-236: Ohayo, sensei — expected manifest version updated correctly.Assertion matches the repo-wide bump to 1.7.0-alpha.3.
examples/spawn-and-move/Scarb.toml (1)
4-4: Ohayo, sensei — nice, this manifest fully aligns to 2.12.2.Version, cairo-version, starknet, and assert_macros are consistent with the bump.
Also applies to: 6-6, 14-14, 23-23
crates/dojo/core-tests/Scarb.toml (1)
5-5: Ohayo, sensei — version bump looks good.Matches workspace alpha.3; snforge_std already at 0.48.1.
.tool-versions (1)
1-1: Ohayo, sensei — Scarb pinned to 2.12.2.Matches the target Cairo toolchain; good.
crates/dojo/dojo-snf-test/Scarb.toml (1)
5-5: Ohayo, sensei — align Cairo/Starknet to 2.12.2 and snforge_std to 0.48.1.Goal of this PR is cairo 2.12.2, but crates/dojo/dojo-snf-test/Scarb.toml still pins cairo-version/starknet/assert_macros to 2.12.0 and snforge_std to 0.48.0 — bump to avoid plugin/runtime mismatches with .tool-versions (scarb 2.12.2, foundry 0.48.1) and CI flakiness. Bump for consistency.
File: crates/dojo/dojo-snf-test/Scarb.toml
Apply:[package] name = "dojo_snf_test" description = "Testing library for Dojo using snfoundry test runner." -version = "1.7.0-alpha.3" +version = "1.7.0-alpha.3" edition = "2024_07" -cairo-version = "2.12.0" +cairo-version = "2.12.2" [dependencies] -starknet = "2.12.0" -snforge_std = "0.48.0" -assert_macros = "2.12.0" +starknet = "2.12.2" +snforge_std = "0.48.1" +assert_macros = "2.12.2" dojo = { path = "../core" }Run to confirm no stragglers:
rg -n --hidden -g '!**/target/**' -e '^\s*(cairo-version|starknet|assert_macros|snforge_std)\s*=\s*".*"' -S # verify outputs show cairo-version/starknet/assert_macros == "2.12.2" and snforge_std == "0.48.1"examples/game-lib/Scarb.toml (1)
8-8: Ohayo sensei — workspace version bump LGTMAligned with the alpha.3 wave; no concerns.
crates/dojo/core/Scarb.toml (1)
6-6: Ohayo sensei — core crate version bump LGTMMatches the workspace versioning strategy.
examples/simple/Scarb.toml (1)
2-2: Ohayo sensei — Cairo/Starknet/test pins updated cleanlycairo-version/starknet/cairo_test and package version all aligned to 2.12.2/alpha.3; looks good.
Also applies to: 4-4, 16-16, 19-19
scripts/scarb_version_sync.sh (1)
41-48: Ohayo sensei — finder is solidNull‑delimited find with prunes is portable and safe for odd paths. Nice.
examples/spawn-and-move/external/Scarb.toml (1)
3-3: ohayo sensei — Align external example to Starknet 2.12.xFound examples/spawn-and-move/external/Scarb.toml:7 still pins starknet = "2.11.4"; bump to 2.12.2 to match the rest of the repo.
Apply this diff:
[package] name = "hello" version = "1.7.0-alpha.3" edition = "2024_07" [dependencies] -starknet = "2.11.4" +starknet = "2.12.2"Cargo.toml (2)
105-127: Ohayo sensei — cairo‑lang crates mostly pinned to 2.12.2; one outlier foundrg check failed due to unsupported lookahead; Cargo.toml (lines 105–127) contains cairo-lang-primitive-token = "1". Confirm whether that should be 2.12.2 or is intentionally v1; if unintended, update.
33-33: ohayo sensei — Workspace version matches 1.7.0-alpha.3; add CI sync check
- Verification: running the provided check output: "All Scarb.toml [package].version and [workspace.package].version match 1.7.0-alpha.3".
- Caveat: the run logged "/dev/fd/63: No such file or directory" and "No Scarb.toml files found under .". Confirm Scarb.toml files exist in the CI workspace and adapt the script for CI shells that lack /dev/fd (otherwise the check may be unreliable).
- Action (optional): add this CI check to block mismatched versions:
#!/usr/bin/env bash # Use the workspace version as the source of truth VER="$(rg -n '^\s*version\s*=\s*"' -n -m1 -N --no-filename -g Cargo.toml -r '$0' | sed -n 's/.*"\(.*\)".*/\1/p')" bash scripts/scarb_version_sync.sh --version "$VER" --check --root ..github/workflows/release-dispatch.yml (1)
34-39: ohayo sensei — Add an explicit Scarb.lock refresh step to the release workflowReplace the reminder with a concrete step that refreshes lockfiles across all Scarb workspaces and commits any changes before creating the PR.
- run: | VERSION=${{ inputs.version }} VERSION=${VERSION#v} bash scripts/scarb_version_sync.sh --version $VERSION - cargo release version $VERSION --execute --no-confirm && cargo release replace --execute --no-confirm + cargo release version $VERSION --execute --no-confirm && cargo release replace --execute --no-confirm + - name: Refresh Scarb.lock files + run: | + set -euo pipefail + # Update lockfiles in all Scarb projects/examples + find . -type d \( -name .git -o -name target \) -prune -o -type f -name Scarb.toml -print0 \ + | xargs -0 -n1 -I{} bash -lc 'cd "$(dirname "{}")" && (scarb fetch || scarb build)'Verification notes: repo has 11 Scarb.toml and 10 tracked Scarb.lock files (examples/game-lib/bestiary has Scarb.toml but no Scarb.lock); current git status shows no modified Scarb.lock files.
After adding the step, confirm it actually updates lockfiles by checking for changes before create-pull-request (e.g. run in-workflow: git status --porcelain | rg -n 'Scarb.lock').
| } else { | ||
| let field_name = member.name(db).text(db); | ||
| let field_selector = get_selector_from_name(field_name).unwrap(); | ||
| let field_selector = get_selector_from_name(&field_name).unwrap(); | ||
| let field_layout = super::layout::get_layout_from_type_clause( | ||
| db, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t unwrap() selector derivation; surface a diagnostic instead.
A malformed identifier would panic macro expansion. Prefer pushing an error and continuing.
- let field_selector = get_selector_from_name(&field_name).unwrap();
+ let field_selector = match get_selector_from_name(&field_name) {
+ Ok(s) => s,
+ Err(e) => {
+ self.diagnostics.push_error(format!(
+ "Invalid field name '{field_name}' for selector: {e}"
+ ));
+ // Fallback to zero to let other diagnostics surface.
+ 0
+ }
+ };🏁 Script executed:
#!/bin/bash
# Ensure all call sites to `get_selector_from_name` in macros avoid `.unwrap()`
rg -n "get_selector_from_name\([^)]+\)\.unwrap\(" -g 'crates/dojo/macros/**'Length of output: 139
Don’t unwrap selector derivation; surface a diagnostic instead.
ohayo sensei — Found 1 occurrence at crates/dojo/macros/src/derives/introspect/structs.rs:164. Replace the .unwrap() with a match that pushes an error into self.diagnostics and returns a safe fallback (e.g., 0) so macro expansion won’t panic.
🤖 Prompt for AI Agents
In crates/dojo/macros/src/derives/introspect/structs.rs around lines 162 to 166,
the call to get_selector_from_name(&field_name).unwrap() can panic; replace the
unwrap with a match that handles None by pushing a diagnostic onto
self.diagnostics (include context like the struct name/field_name and that
selector derivation failed) and use a safe fallback selector value (e.g., 0) so
macro expansion continues; when Some(selector) is returned, proceed to use it as
before.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/test.yml (2)
65-65: Ohayo, sensei — Scarb 2.12.2 bump looks good; dedupe the version to avoid future drift.Centralize the version in env and reference it in all four setup-scarb steps.
env: CARGO_TERM_COLOR: always RUST_VERSION: 1.88.0 + SCARB_VERSION: "2.12.2" @@ - scarb-version: "2.12.2" + scarb-version: "${{ env.SCARB_VERSION }}" @@ - scarb-version: "2.12.2" + scarb-version: "${{ env.SCARB_VERSION }}" @@ - scarb-version: "2.12.2" + scarb-version: "${{ env.SCARB_VERSION }}" @@ - scarb-version: "2.12.2" + scarb-version: "${{ env.SCARB_VERSION }}"Also applies to: 123-123, 133-133, 150-150
92-96: Fix env var case for Windows toolchain selection.
${{ env.rust_version }}is undefined; the env key isRUST_VERSION. This can cause the Windows job to pick an unintended toolchain.- toolchain: ${{ env.rust_version }} + toolchain: ${{ env.RUST_VERSION }}bin/sozo/tests/test_data/policies.json (3)
111-153: Duplicate method mappings to 0x61d2… — OK if multi‑target allowed; suggest grouping for stability.If multiple targets per method are expected, great; otherwise this could create ambiguous expectations in consumers.
Consider co-locating all entries for a given method or sorting by method then target to reduce churn in diffs.
155-197: Mirror cluster to 0x5db7… — double‑check runtime handles multi‑target “OR” correctly.Ensure the evaluator treats these as independent allow rules, not last‑write‑wins.
Optionally add a JSON‑Schema + CI check to lock structure and method enums; I can draft one if useful.
199-205: Method name casing: ‘deployContract’ differs from snake_case elsewhere.If method matching is case‑sensitive, confirm this exact casing is required by the caller.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/dojo/core-tests/Scarb.lockis excluded by!**/*.lockexamples/game-lib/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/external/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
.github/workflows/test.yml(4 hunks)bin/sozo/tests/test_data/policies.json(3 hunks)crates/dojo/core-tests/.snfoundry_cache/.prev_tests_failed(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- crates/dojo/core-tests/.snfoundry_cache/.prev_tests_failed
- examples/spawn-and-move/dojo_dev.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-28T23:35:04.367Z
Learnt from: glihm
PR: dojoengine/dojo#2691
File: bin/sozo/tests/test_data/policies.json:7-32
Timestamp: 2024-11-28T23:35:04.367Z
Learning: The file `bin/sozo/tests/test_data/policies.json` is a test file containing policies used by sessions to determine which methods can be called for which contract addresses. There is not much to verify or check in this file.
Applied to files:
bin/sozo/tests/test_data/policies.json
⏰ 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: clippy
🔇 Additional comments (1)
bin/sozo/tests/test_data/policies.json (1)
103-109: Swapped upgrade targets — verify they match the intended 2.12.2 fixtures.Looks like a deliberate remap. Please confirm these are the right deployed addresses for this bump.
| { | ||
| "target": "0x4ca02e0da71179ec7d0c250c717eb90d70f599282600e545dc1a07de9e4f6f9", | ||
| "method": "upgrade" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ohayo sensei — new top‑level upgrade entry looks fine; confirm merge semantics.
If the policy loader de‑dupes/OR‑merges entries, this is OK. Please confirm order doesn’t shadow earlier/later “upgrade” entries.
Run this to confirm behavior/duplicates:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=bin/sozo/tests/test_data/policies.json
# JSON valid and objects have required keys
jq -e 'type=="array" and all(.[]; has("target") and has("method"))' "$f" >/dev/null || { echo "Invalid schema"; exit 1; }
# Hex format check (0x + 64 hex)
jq -e 'all(.[]; .target|test("^0x[0-9a-f]{64}$"))' "$f" >/dev/null || { echo "Bad target format"; exit 1; }
echo "Upgrade entries:"
jq -r '.[] | select(.method=="upgrade")' "$f"
echo "Methods with >1 target (ensure this is intentional):"
jq -r '
group_by(.method)
| map({method: .[0].method, targets: (map(.target)|unique), count: (map(.target)|unique|length)})
| map(select(.count>1))
' "$f"Length of output: 17
Ohayo sensei — malformed target(s) in bin/sozo/tests/test_data/policies.json; fix before verifying merge semantics.
Verification script returned "Bad target format". Ensure every .target matches 0x[0-9a-f]{64} (0x + 64 lowercase hex chars) or update the loader to accept the existing format, then re-run the provided checks.
🤖 Prompt for AI Agents
In bin/sozo/tests/test_data/policies.json around lines 2 to 5, the "target"
value is malformed for the verifier; ensure each .target either matches the
exact regex 0x[0-9a-f]{64} by replacing the current value with a lowercase
0x-prefixed 64-hex-char string, or if the intended format differs, update the
loader/validator to accept the current format and adjust its validation regex
accordingly, then re-run the verification checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/core-tests/src/tests/world/acl.cairo (1)
53-55: Ohayo sensei — make the panic expectation less brittle by matching a substringThe #[should_panic(expected = "...")] matcher uses substring matching, so trimming the dynamic address is safe.
File: crates/dojo/core-tests/src/tests/world/acl.cairo (lines 53-55)
-#[should_panic( - expected: "Contract `0x384b52e0b89bb6221ef3158cab06c8da5e0be5a37ca0450794550c28e2896b2` does NOT have OWNER role on model (or its namespace) `Foo`", -)] +#[should_panic( + expected: "does NOT have OWNER role on model (or its namespace) `Foo`", +)]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/dojo/dojo-cairo-test/Scarb.lockis excluded by!**/*.lockcrates/dojo/dojo-snf-test/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/dojo/core-tests/src/tests/world/acl.cairo(4 hunks)crates/dojo/core-tests/src/tests/world/contract.cairo(2 hunks)crates/dojo/core-tests/src/tests/world/event.cairo(1 hunks)crates/dojo/core-tests/src/tests/world/metadata.cairo(1 hunks)crates/dojo/core-tests/src/tests/world/model.cairo(1 hunks)crates/dojo/core-tests/src/tests/world/world.cairo(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-05T04:29:12.288Z
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within `crates/dojo/core/src/world/storage.cairo`, length checks between `entity_ids` and `values` are not required in test API functions like `write_values_from_ids_test`.
Applied to files:
crates/dojo/core-tests/src/tests/world/model.cairocrates/dojo/core-tests/src/tests/world/world.cairo
⏰ 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: clippy
🔇 Additional comments (11)
crates/dojo/core-tests/src/tests/world/event.cairo (1)
300-301: Update should_panic address – LGTMOhayo, sensei — updated malicious_contract address matches the Cairo 2.12.2 artifacts.
crates/dojo/core-tests/src/tests/world/metadata.cairo (1)
112-113: Update should_panic address – LGTMOhayo, sensei — expected panic string refreshed to new contract address is correct.
crates/dojo/core-tests/src/tests/world/model.cairo (1)
357-359: Update should_panic address – LGTMOhayo, sensei — new malicious_contract address aligns with the bumped toolchain.
crates/dojo/core-tests/src/tests/world/world.cairo (1)
250-251: Update should_panic address – LGTMOhayo, sensei — owner/caller contract address in the panic string updated appropriately.
crates/dojo/core-tests/src/tests/world/acl.cairo (4)
53-55: Update should_panic address – LGTMOhayo, sensei — address replacement looks good.
90-92: Update should_panic address – LGTMConsistent with Cairo 2.12.2 build outputs.
153-155: Update should_panic address – LGTMMatches the new malicious_contract address used elsewhere.
190-192: Update should_panic address – LGTMCorrect address update for the revoke writer path.
crates/dojo/core-tests/src/tests/world/contract.cairo (3)
244-246: Update should_panic address – LGTMOhayo, sensei — new address for malicious deploy path is correct.
352-354: Update should_panic address – LGTMAddress update for upgrade-through-malicious flow looks consistent.
244-246: ohayo sensei — Quick sweep: verify no stale hardcoded addresses remainrg returned no output; re-run the search across the repo and confirm none of these addresses are present: 0x12628844c00f37b6e7ac0d9dba2941553f09f9b7b1de979103f64f072db26f6, 0x252567a4ea339b58d479b6fd744d818af064abe29cc04fc746178e0d269c0b6, 0x6e1592b8353d0fc098469cce233353af2b92a5b88ef81e8355e93c009a9617a
1 participant
Summary by CodeRabbit
Chores
Refactor
Tests