blockifier: test CAIRO1_SUPPORTED_BUILTINS stays in sync with the compiler builtin order#14875
Conversation
db113ca to
05a4ead
Compare
1bba8d4 to
aac9d86
Compare
05a4ead to
22e6d51
Compare
aac9d86 to
dc50c5d
Compare
PR SummaryLow Risk Overview Test imports are updated to pull Reviewed by Cursor Bugbot for commit b824480. Bugbot is set up for automated code reviews on this repo. Configure here. |
dc50c5d to
03f7c0a
Compare
03f7c0a to
f0bc787
Compare
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on Yoni-Starkware).
Cargo.toml line 271 at r1 (raw file):
colored = "3" const_format = "0.2.30" convert_case = "0.11.0"
Consider avoiding this new external dep
Code quote:
convert_case = "0.11.0"crates/blockifier/src/execution/entry_point_execution_test.rs line 13 at r1 (raw file):
use starknet_api::transaction::fields::Calldata; use super::{validate_entry_point_builtins, CAIRO1_SUPPORTED_BUILTINS};
Avoid using super::
Code quote:
use super::{validate_entry_point_builtins, CAIRO1_SUPPORTED_BUILTINS};crates/blockifier/src/execution/entry_point_execution_test.rs line 144 at r1 (raw file):
.iter() .map(|builtin| builtin.to_str().to_case(Case::UpperCamel)) .collect();
Converting to UpperCamel might create false negatives: blake2s -> Blake2S
Consider converting the compiler builtins to snake case instead.
Alternatively, we can avoid using the convert_case external dep for this comparison and just remove all underscores from the blockifier builtins and lowercase the compiler builtins to compare them:
let supported_builtins: Vec<String> = CAIRO1_SUPPORTED_BUILTINS
.iter()
.map(|builtin| builtin.to_str().replace('_', ""))
.collect();
let compiler_builtins: Vec<String> = ENTRY_POINT_BUILTIN_ORDER
.iter()
.map(|generic_id| generic_id.0.to_ascii_lowercase())
.collect();
avi-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on Yoni-Starkware).
…piler builtin order Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f0bc787 to
b824480
Compare
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware reviewed 4 files and all commit messages, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Yoni-Starkware).
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Yoni-Starkware).
What
Adds a unit test,
cairo1_supported_builtins_in_sync_with_compiler_order, that asserts the blockifier'sCAIRO1_SUPPORTED_BUILTINS(from #14841) is identical — same set and canonical order — to the Cairo compiler'sENTRY_POINT_BUILTIN_ORDER(cairo_lang_starknet_classes::casm_contract_class). Each Sierra generic type id is mapped to itsBuiltinName, mirroring the naming the compiler itself emits inCasmContractClass::from_contract_class. A newly-added compiler builtin with no mapping arm trips apanic!, forcing both the map andCAIRO1_SUPPORTED_BUILTINSto be updated together.Why
Follow-up to a review comment on #14841:
CAIRO1_SUPPORTED_BUILTINSis hand-maintained (mirroring the OSSelectableBuiltins). This test keeps it from silently drifting from the compiler's source-of-truth ordering as the Cairo compiler evolves.ENTRY_POINT_BUILTIN_ORDERonly becamepubin cairo 2.19.4, which is why the reviewer asked for this "after bumping to 2.19.4".Base
Targets
main-v0.14.3, which now contains both #14841 (CAIRO1_SUPPORTED_BUILTINS) and the merged #14873 (cairo compiler v2.19.4, providing the now-pubENTRY_POINT_BUILTIN_ORDER). This PR adds only the sync test — one file.Testing
cargo test -p blockifier cairo1_supported_builtins_in_sync_with_compiler_orderpasses.cargo clippy -p blockifier --all-targetsandrust_fmtclean.🤖 Generated with Claude Code