refactor(skills): collapse redundant Cairo modules and sync upstream optimization#7
refactor(skills): collapse redundant Cairo modules and sync upstream optimization#7omarespejel wants to merge 2 commits into
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 collapses the standalone
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| cairo-optimization/scripts/profile.py | New Cairo profiling CLI — solid overall structure, but has a timestamp bug where pb.gz and png filenames can diverge if a minute boundary is crossed between the two _profile_filename calls, contradicting the documented guarantee. |
| cairo-optimization/scripts/bounded_int_calc.py | BoundedInt bound calculator — logic is correct for add/sub/mul/div, but format_bound() is a dead-code no-op with identical branches for positive and negative values. |
| cairo-optimization/references/legacy-full.md | Upstream-synced optimization reference — Rule 12 (felt252→BoundedInt via u128) added correctly, but Rule 11 (hades_permutation) row remains in the Quick Reference table while its detailed explanation section was deleted, leaving a dangling reference. |
| cairo-optimization/SKILL.md | Updated cairo-optimization skill metadata and quick-start — version bumped, upstream provenance fields added correctly, but the script path in step 2 (scripts/profile.py) is missing the cairo-optimization/ prefix needed for repo-root invocation. |
| cairo-optimization/references/profiling.md | New profiling reference guide — comprehensive and well-structured; tool installation section includes curl |
| cairo-contract-authoring/references/language.md | New Cairo language fundamentals reference — well-structured with accurate type system, ownership, traits, generics, and module layout coverage; no issues found. |
| cairo-contract-authoring/references/legacy-full.md | Enriched authoring reference — adds component wiring checklist, cross-contract dispatcher guidance, and OZ hardening checklist; all additions are accurate and the path fixes (../../ prefixes) are correct. |
| THIRD_PARTY.md | New third-party attribution registry — correctly documents feltroidprime upstream with commit pinning and permission reference; workflow instructions align with CONTRIBUTING.md additions. |
| scripts/site/build_site.py | Site builder updated to drop openzeppelin-cairo from MODULES list (now 7 modules); HTML generation uses html.escape correctly and no security regressions observed. |
| CONTRIBUTING.md | Added third-party sync rules section — clear provenance metadata requirements and permission traceability gate; no issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Developer runs profile.py] --> B{mode?}
B -->|snforge| C[snforge test --save-trace-data]
B -->|scarb| D[scarb execute --save-profiler-trace-data]
C --> E{returncode == 0?}
D --> E
E -->|no| F[exit 2: execution failed]
E -->|yes| G{snforge or scarb?}
G -->|snforge| H[glob snfoundry_trace/*.json]
G -->|scarb| I[target/execute/pkg/execution1/cairo_profiler_trace.json]
H --> J{trace found?}
I --> J
J -->|no| K[exit 3: trace not found]
J -->|yes| L[cairo-profiler build-profile --show-libfuncs]
L --> M{pb.gz written?}
M -->|no| N[exit 4: profiler failed]
M -->|yes| O[cairo-profiler view top-20 functions]
O --> P[pprof -png output png_path]
P --> Q{PNG written?}
Q -->|no| R[exit 5: pprof failed]
Q -->|yes| S[print profile + PNG paths]
style F fill:#f87171
style K fill:#f87171
style N fill:#f87171
style R fill:#f87171
style S fill:#86efac
Comments Outside Diff (1)
-
cairo-optimization/references/legacy-full.md, line 30-32 (link)Rule 11 row retained in Quick Reference table but the detailed section was deleted
The Quick Reference table still lists:
| 11 | Fast 2-input Poseidon | `poseidon_hash_span([x,y])` | `hades_permutation(x, y, 2)` |However, the upstream-sync diff deleted the entire
### 11. Always use \hades_permutation` for 2-input Poseidon hashes` section that contained the BAD/GOOD code examples and the explanation. The table now points to guidance that no longer exists in this file, leaving readers without the supporting rationale and code examples for this optimization rule.Either restore the deleted section or remove row 11 from the table to keep the reference consistent.
Prompt To Fix With AI
This is a comment left during a code review. Path: cairo-optimization/references/legacy-full.md Line: 30-32 Comment: **Rule 11 row retained in Quick Reference table but the detailed section was deleted** The Quick Reference table still lists: ``` | 11 | Fast 2-input Poseidon | `poseidon_hash_span([x,y])` | `hades_permutation(x, y, 2)` | ``` However, the upstream-sync diff deleted the entire `### 11. Always use \`hades_permutation\` for 2-input Poseidon hashes` section that contained the BAD/GOOD code examples and the explanation. The table now points to guidance that no longer exists in this file, leaving readers without the supporting rationale and code examples for this optimization rule. Either restore the deleted section or remove row 11 from the table to keep the reference consistent. How can I resolve this? If you propose a fix, please make it concise.
Last reviewed commit: f376747
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _profile_filename(output_dir: str, package: str, name: str, metric: str, commit: str, ext: str) -> str: | ||
| """Generate deterministic profile filename.""" | ||
| ts = datetime.now().strftime("%y-%m-%d-%H:%M") | ||
| return os.path.join(output_dir, f"{ts}_{package}_{name}_{metric}_{commit}.{ext}") |
There was a problem hiding this comment.
Timestamp computed separately for pb.gz and png, breaking the "computed once" guarantee
_profile_filename calls datetime.now() on every invocation. In main() (lines 323–324), it is called twice in succession:
pb_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, "pb.gz")
png_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, "png")If execution crosses a minute boundary between these two calls, the .pb.gz and .png files will receive different timestamps — exactly the mismatch the documentation in references/profiling.md claims the CLI prevents:
"When running steps manually, the pb.gz and png may get different timestamps if they cross a minute boundary. The CLI computes the timestamp once so both files always match."
The fix is to compute the timestamp once and pass it explicitly:
def _profile_filename(output_dir: str, package: str, name: str, metric: str, commit: str, ts: str, ext: str) -> str:
return os.path.join(output_dir, f"{ts}_{package}_{name}_{metric}_{commit}.{ext}")Then in main():
ts = datetime.now().strftime("%y-%m-%d-%H:%M")
pb_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, ts, "pb.gz")
png_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, ts, "png")Prompt To Fix With AI
This is a comment left during a code review.
Path: cairo-optimization/scripts/profile.py
Line: 79-82
Comment:
**Timestamp computed separately for pb.gz and png, breaking the "computed once" guarantee**
`_profile_filename` calls `datetime.now()` on every invocation. In `main()` (lines 323–324), it is called twice in succession:
```python
pb_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, "pb.gz")
png_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, "png")
```
If execution crosses a minute boundary between these two calls, the `.pb.gz` and `.png` files will receive **different** timestamps — exactly the mismatch the documentation in `references/profiling.md` claims the CLI prevents:
> "When running steps manually, the pb.gz and png may get different timestamps if they cross a minute boundary. The CLI computes the timestamp once so both files always match."
The fix is to compute the timestamp once and pass it explicitly:
```python
def _profile_filename(output_dir: str, package: str, name: str, metric: str, commit: str, ts: str, ext: str) -> str:
return os.path.join(output_dir, f"{ts}_{package}_{name}_{metric}_{commit}.{ext}")
```
Then in `main()`:
```python
ts = datetime.now().strftime("%y-%m-%d-%H:%M")
pb_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, ts, "pb.gz")
png_path = _profile_filename(output_dir, args.package, args.name, args.metric, commit, ts, "png")
```
How can I resolve this? If you propose a fix, please make it concise.| def format_bound(value: int) -> str: | ||
| """Format a bound value, handling negatives.""" | ||
| if value < 0: | ||
| return str(value) | ||
| return str(value) |
There was a problem hiding this comment.
format_bound is a no-op — both branches return the same value
Both branches of the conditional return an identical expression (str(value)), making the function equivalent to simply calling str(). The docstring says "handling negatives" but no actual special handling is implemented:
def format_bound(value: int) -> str:
"""Format a bound value, handling negatives."""
if value < 0:
return str(value) # same as the else branch
return str(value) # identicalThis looks like an incomplete implementation. BoundedInt bounds cannot actually be negative in Cairo (the Sierra specializer requires MIN >= 0), so this function likely just needs to remain str(value). If the intent was to validate or format negatives differently, the logic is missing.
| def format_bound(value: int) -> str: | |
| """Format a bound value, handling negatives.""" | |
| if value < 0: | |
| return str(value) | |
| return str(value) | |
| def format_bound(value: int) -> str: | |
| """Format a bound value.""" | |
| return str(value) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cairo-optimization/scripts/bounded_int_calc.py
Line: 33-37
Comment:
**`format_bound` is a no-op — both branches return the same value**
Both branches of the conditional return an identical expression (`str(value)`), making the function equivalent to simply calling `str()`. The docstring says "handling negatives" but no actual special handling is implemented:
```python
def format_bound(value: int) -> str:
"""Format a bound value, handling negatives."""
if value < 0:
return str(value) # same as the else branch
return str(value) # identical
```
This looks like an incomplete implementation. BoundedInt bounds cannot actually be negative in Cairo (the Sierra specializer requires `MIN >= 0`), so this function likely just needs to remain `str(value)`. If the intent was to validate or format negatives differently, the logic is missing.
```suggestion
def format_bound(value: int) -> str:
"""Format a bound value."""
return str(value)
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| 1. Confirm baseline behavior with tests. | ||
| 2. Identify hotspots via resource reports. | ||
| 2. Profile target paths with `python3 scripts/profile.py profile`. |
There was a problem hiding this comment.
Incorrect script path — scripts/profile.py is not resolvable from the repo root
The Quick Start instruction says:
2. Profile target paths with `python3 scripts/profile.py profile`.
But the script lives at cairo-optimization/scripts/profile.py. A user (or an AI agent) running this from the repository root would need:
python3 cairo-optimization/scripts/profile.py profile
The same mismatch appears in cairo-optimization/references/profiling.md (line 12 and throughout), which says "use python3 scripts/profile.py profile (from this repository)". The instruction in profiling.md is contextually anchored to the cairo-optimization/ subdirectory — but SKILL.md is loaded in repo-root context. The path is ambiguous and will silently fail for agents running commands from the repo root.
Consider using the fully qualified path cairo-optimization/scripts/profile.py or a repo-relative instruction like "from the cairo-optimization/ directory, run python3 scripts/profile.py profile".
Prompt To Fix With AI
This is a comment left during a code review.
Path: cairo-optimization/SKILL.md
Line: 29
Comment:
**Incorrect script path — `scripts/profile.py` is not resolvable from the repo root**
The Quick Start instruction says:
```
2. Profile target paths with `python3 scripts/profile.py profile`.
```
But the script lives at `cairo-optimization/scripts/profile.py`. A user (or an AI agent) running this from the repository root would need:
```
python3 cairo-optimization/scripts/profile.py profile
```
The same mismatch appears in `cairo-optimization/references/profiling.md` (line 12 and throughout), which says "use `python3 scripts/profile.py profile` (from this repository)". The instruction in `profiling.md` is contextually anchored to the `cairo-optimization/` subdirectory — but `SKILL.md` is loaded in repo-root context. The path is ambiguous and will silently fail for agents running commands from the repo root.
Consider using the fully qualified path `cairo-optimization/scripts/profile.py` or a repo-relative instruction like "from the `cairo-optimization/` directory, run `python3 scripts/profile.py profile`".
How can I resolve this? If you propose a fix, please make it concise.| # Via snfoundryup installer | ||
| curl -L https://raw.githubusercontent.com/foundry-rs/starknet-foundry/master/scripts/install.sh | sh | ||
| snfoundryup |
There was a problem hiding this comment.
curl | sh install patterns in a security-focused reference
Two of the tool installation commands pipe remote content directly into a shell without any integrity check:
curl -L https://raw.githubusercontent.com/foundry-rs/starknet-foundry/master/scripts/install.sh | shcurl -L https://raw.githubusercontent.com/software-mansion/cairo-profiler/main/scripts/install.sh | shFor a repository whose explicit purpose is teaching secure Cairo/Starknet practices, recommending curl | sh without noting the security trade-off sends a mixed message. Consider adding a brief note recommending the asdf path (which is listed first and avoids piping remote scripts) or suggesting users inspect the script before running it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cairo-optimization/references/profiling.md
Line: 161-163
Comment:
**`curl | sh` install patterns in a security-focused reference**
Two of the tool installation commands pipe remote content directly into a shell without any integrity check:
```bash
curl -L https://raw.githubusercontent.com/foundry-rs/starknet-foundry/master/scripts/install.sh | sh
```
```bash
curl -L https://raw.githubusercontent.com/software-mansion/cairo-profiler/main/scripts/install.sh | sh
```
For a repository whose explicit purpose is teaching secure Cairo/Starknet practices, recommending `curl | sh` without noting the security trade-off sends a mixed message. Consider adding a brief note recommending the `asdf` path (which is listed first and avoids piping remote scripts) or suggesting users inspect the script before running it.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
This PR applies the Cairo skill collapse/update plan on top of
codex/audit-corpus-wave1.What changed
openzeppelin-cairo/skill module.cairo-contract-authoring:references/language.md(PR #295 cairo language content).references/legacy-full.mdwith component wiring checklist, cross-contract dispatcher guidance, and OZ hardening checklist.cairo-optimizationto feltroidprime latest:references/legacy-full.mdwith upstream-synced content (rules 1-12 + BoundedInt updates).references/profiling.mdfrombenchmarking-cairo.scripts/profile.py,scripts/bounded_int_calc.py.upstream,upstream_commit,sync_date,upstream_paths, andpermission_ref.SKILL.md,README.md,llms.txt,scripts/site/build_site.pywebsite/data/site-data.jsonandwebsite/index.html.THIRD_PARTY.md.README.mdandCONTRIBUTING.mdwith third-party sync/attribution instructions.Validation
python3 scripts/site/build_site.py --domain starkskills.orgpython scripts/quality/validate_skills.py(OK: validated 8 SKILL.md files)Attribution
feltroidprime/cairo-skillsis retained with author metadata and explicit upstream commit pinning.THIRD_PARTY.md.