refactor: move the Cairo 1 hint processor into its own crate - #2390
Conversation
|
|
Benchmark Results for unmodified programs 🚀
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2390 +/- ##
=======================================
Coverage 96.21% 96.21%
=======================================
Files 107 108 +1
Lines 37925 37927 +2
=======================================
+ Hits 36490 36492 +2
Misses 1435 1435 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware partially reviewed 23 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware, orizi, and yuvalsw).
cairo1-hint-processor/src/program.rs line 18 at r1 (raw file):
/// /// This is a free function rather than a `TryFrom` impl because both types are foreign to this /// crate.
remove
Code quote:
/// This is a free function rather than a `TryFrom` impl because both types are foreign to this
/// crate.The cairo-vm library depended on cairo-lang-casm and cairo-lang-starknet-classes behind the cairo-1-hints feature, which pulled the whole Cairo 1 compiler into the dependency tree of anyone who only wanted to run Cairo 0 programs. Executing Cairo 1 hints means speaking the compiler's Hint AST, so rather than copying those definitions into cairo-vm, the hint processor moves to a new cairo1-hint-processor crate that owns the compiler dependency. cairo-vm is now free of cairo-lang-* in every dependency kind, including dev, and the cairo-1-hints feature is gone rather than merely emptied. The move needed no new public surface on cairo-vm: the tests that came along switched from the private CairoRunner::program field to the existing get_program()/data_len() accessors. TryFrom<CasmContractClass> for Program could not come along as an impl, since both types are foreign to the new crate; it is now the free function program_from_casm_contract_class. Nothing outside cairo-vm's own tests called it, so nothing is stranded. cairo1-run needed only an import swap - it already produced cairo_lang_casm::hints::Hint and the hint processor still consumes exactly that type, so no conversion layer exists anywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9ea5588 to
2668732
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 22 of 23 files reviewed, 1 unresolved discussion (waiting on eytan-starkware, TomerStarkware, and yuvalsw).
cairo1-hint-processor/src/program.rs line 18 at r1 (raw file):
Previously, TomerStarkware wrote…
remove
Done.
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed 1 file.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on eytan-starkware and yuvalsw).
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on eytan-starkware and yuvalsw).
What
cairo-vmdepended oncairo-lang-casmandcairo-lang-starknet-classesbehind thecairo-1-hintsfeature, which pulled the whole Cairo 1 compiler into the dependency tree of anyone who only wanted to run Cairo 0 programs.Executing Cairo 1 hints means speaking the compiler's
HintAST, so rather than copying those definitions intocairo-vm, the hint processor moves into a newcairo1-hint-processorcrate that owns the compiler dependency. Git tracked the four files as renames (93–99% similarity), so the diff reads as a move rather than a rewrite.cairo-vmis now free ofcairo-lang-*in every dependency kind, including dev, and thecairo-1-hintsfeature is gone rather than merely emptied:The
,devmatters: it's what catches a dev-dependency cycle, which is what you'd get by leaving the Cairo 1 tests invmand pointing them at the new crate. Those 55 tests moved into the new crate instead.Breaking changes
cairo-1-hintsfeature ofcairo-vmis removed. Depend oncairo1-hint-processorinstead. Itsextensive_hintsfeature forwards tocairo-vm/extensive_hints.cairo_vm::hint_processor::cairo_1_hint_processor::*→cairo1_hint_processor::*.impl TryFrom<CasmContractClass> for Programis replaced by the free functioncairo1_hint_processor::program_from_casm_contract_class. It could not remain aTryFromimpl because both types are foreign to the new crate. Nothing outsidecairo-vm's own tests called it, so the impl and its only callers moved together and nothing is stranded.Per the CHANGELOG's version guide, 3.x (main) is the branch for "new features with breaking API changes", so hard removal is in-policy.
Notes for review
cairo-vm. I expected to have to widen somepub(crate)items for the move. The only gap was the moved tests reading the privateCairoRunner::programfield;get_program()andProgram::data_len()already existed, so those are used instead.cairo1-runneeded only an import swap (two lines plus one manifest line). It already producedcairo_lang_casm::hints::Hintand the hint processor still consumes exactly that type, so there is no conversion layer anywhere.extensive_hintsforwarding is load-bearing. There is a#[cfg(feature = "extensive_hints")]insidedict_manager.rs; after the move it resolves against the new crate's features, so without the forwarding feature it would have silently evaluated tofalsewith no error or warning.cairo1-hint-processoris a new publishable crate and will need publishing alongsidecairo-vm.Verification
cargo test -p cairo1-hint-processor -p cairo1-run— 55 + 35 + 134 passed, 0 failed.cargo clippyclean on all three touched crates;cargo macheteclean;cargo build --workspaceclean.cargo test -p cairo-vmwas not run: its test modulesinclude_bytes!generated Cairo 0 JSON fixtures that need the Pythoncairo-langtoolchain, which isn't available in my environment. This is pre-existing and unrelated to the diff, but flagging it rather than implying full coverage — CI covers it.Also updated: the
cargo all-featuresCI matrix (adds the new crate), the nextest feature list and threeMakefiletargets (all of which passedcairo-1-hintsand would now hard-fail),CHANGELOG.md, anddocs/onboarding.md. Dropped the deadcairo-lang-runnerworkspace dependency while inCargo.toml.🤖 Generated with Claude Code
This change is