refactor(skills): collapse redundant Cairo modules and sync upstream optimization#8
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR consolidates the Key findings:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| cairo-optimization/references/legacy-full.md | Adds Rule 12 (felt252→BoundedInt via u128 decomposition) and attribution header; introduces a correctness regression in the Rule 11 GOOD example by dropping the required tuple destructuring of hades_permutation's return value. |
| cairo-optimization/scripts/bounded_int_calc.py | New CLI tool to compute Cairo BoundedInt trait impl bounds; functional for well-formed inputs but lacks validation that lo ≤ hi for each operand range, allowing silently inverted bounds to be generated. |
| cairo-optimization/scripts/profile.py | New deterministic Cairo profiling pipeline (snforge + scarb modes); package resolution fallback does not verify the package name against the top-level Scarb.toml, which can silently profile the wrong package. |
| cairo-contract-authoring/references/language.md | New Cairo language fundamentals reference; code examples are accurate for Cairo 2.x (linear types, snapshots, refs, collections, error handling, modules). |
| cairo-contract-authoring/references/legacy-full.md | Adds component wiring checklist, cross-contract dispatcher guidance, and OZ hardening checklist; relative path fixes are correct; content is accurate. |
| cairo-optimization/references/profiling.md | New profiling reference covering CLI usage, exit codes, pitfalls, and tool installation; installation instructions use curl-pipe-to-sh patterns which is standard for these tools but worth acknowledging. |
| THIRD_PARTY.md | New third-party attribution registry; correctly documents feltroidprime as upstream author of cairo-optimization with commit hash and permission reference. |
| openzeppelin-cairo/SKILL.md | Deleted — OZ Cairo module collapsed into cairo-contract-authoring; all routing references (SKILL.md, README.md, llms.txt) have been updated consistently. |
Sequence Diagram
sequenceDiagram
participant User
participant profile.py
participant snforge/scarb
participant cairo-profiler
participant pprof
User->>profile.py: python3 profile.py profile --mode snforge/scarb ...
profile.py->>profile.py: _find_repo_root() + _find_package_dir()
profile.py->>snforge/scarb: snforge test --save-trace-data OR scarb execute --save-profiler-trace-data
snforge/scarb-->>profile.py: exit code (fail → exit 2)
profile.py->>profile.py: _find_trace_snforge() / _find_trace_scarb()
Note over profile.py: exit 3 if trace not found
profile.py->>cairo-profiler: cairo-profiler build-profile <trace.json> --show-libfuncs
cairo-profiler-->>profile.py: <name>.pb.gz (fail → exit 4)
profile.py->>cairo-profiler: cairo-profiler view <name>.pb.gz --sample <metric> --limit 20
cairo-profiler-->>User: top-20 functions printed
profile.py->>pprof: pprof -png -sample_index=<metric> -output <name>.png <name>.pb.gz
pprof-->>profile.py: <name>.png (fail → exit 5)
profile.py-->>User: Profile path + PNG path summary (exit 0)
Comments Outside Diff (3)
-
cairo-optimization/references/legacy-full.md, line 688-689 (link)Rule 11 GOOD example drops required tuple destructuring
hades_permutationreturns(felt252, felt252, felt252), not a barefelt252. The previous code correctly destructured the result:let (hash, _, _) = hades_permutation(x, y, 2);
The new code
let h = hades_permutation(x, y, 2)binds the full 3-tuple toh. Any downstream code that passeshwhere afelt252is expected will fail to compile. This is a regression in a high-visibility example that users are likely to copy directly. -
cairo-optimization/scripts/bounded_int_calc.py, line 1068-1079 (link)No validation that input ranges are well-ordered (lo ≤ hi)
None of the
calc_*functions check thata_lo ≤ a_hiorb_lo ≤ b_hi. Inverted input ranges silently propagate to generated Cairo code with invertedBoundedIntbounds (e.g.,BoundedInt<50, 0>), which the Sierra specialiser will reject with a cryptic error rather than a clear user message from this tool.The most dangerous case is
calc_sub: ifa_lo > a_hi, the formularesult_lo = a_lo - b_hiandresult_hi = a_hi - b_locan produce aresult_lo > result_hibound without any warning.Consider adding an early guard in each function, for example:
def calc_add(a_lo: int, a_hi: int, b_lo: int, b_hi: int) -> tuple[int, int]: if a_lo > a_hi: print(f"ERROR: First operand range is inverted: [{a_lo}, {a_hi}]", file=sys.stderr) sys.exit(1) if b_lo > b_hi: print(f"ERROR: Second operand range is inverted: [{b_lo}, {b_hi}]", file=sys.stderr) sys.exit(1) ...
The same guard should be applied to
calc_sub,calc_mul, andcalc_div. -
cairo-optimization/scripts/profile.py, line 1372-1381 (link)Fallback to repo root does not verify the package name
When
packages/<package>/Scarb.tomlis not found, the function falls back torepo_rootif any top-levelScarb.tomlexists — without checking that the package declared in that file actually matchesargs.package. This means a user running--package falconagainst a repo whose root is a package namedmy_projectwill silently profile the wrong package instead of getting a clear error.Consider parsing the
Scarb.tomlto confirm the[package] namefield, or at minimum pass--package <name>to the snforge/scarb invocation so the tool itself enforces the match.
Last reviewed commit: ae1b91d
|
Addressed all current bot findings in follow-up commit
Validation rerun:
|
cbf34b7 to
1dcd255
Compare
Summary
This PR collapses redundant Cairo skill modules and syncs optimization content with the latest upstream source.
Changes
openzeppelin-cairo/skill module.cairo-contract-authoring:references/language.md(language fundamentals from PR #295 context).references/legacy-full.md.cairo-optimizationwith latest feltroidprime content:references/legacy-full.mdwith rules 1-12 + BoundedInt updates.references/profiling.mdfrom benchmarking-cairo.scripts/profile.py,scripts/bounded_int_calc.py.upstream,upstream_commit,sync_date,upstream_paths,permission_ref.THIRD_PARTY.md.README.mdandCONTRIBUTING.mdwith third-party attribution/sync rules.SKILL.md,README.md,llms.txt.Validation
.venv/bin/python scripts/quality/validate_skills.pyOK: validated 8 SKILL.md filesNote
This is the default-branch-targeted version of the earlier branch-targeted PR so review tooling can run against
main.