Skip to content

apollo_compile_to_casm: test that the bundled libfunc list rejects what it excludes - #15141

Open
Yoni-Starkware wants to merge 1 commit into
main-v0.14.4from
yonatan/libfunc-test-followups
Open

Yoni-Starkware wants to merge 1 commit into
main-v0.14.4from
yonatan/libfunc-test-followups

Conversation

@Yoni-Starkware

Copy link
Copy Markdown
Collaborator

Follow-ups deferred from the #15133 review.

The test

Nothing so far distinguished Bundled from the built-in lists: the list selection gates acceptance but does not change the emitted CASM, so comparing outputs can't tell them apart. A class using an excluded libfunc can.

resources/coupon_contract.sierra.json is such a class, with its Cairo source committed beside it. Run against the three selections directly:

List Result
all compiles
audited compiles
bundled rejectedLibfunc coupon_buy is not allowed

Both built-in lists permit coupons, so rejecting this class is the only behaviour that separates our list from either of them. bundled_libfuncs_list_rejects_an_excluded_libfunc asserts all three.

This only became possible once #15136 landed — on #15133 alone the bundled list held exactly the same entries as audited.

Other review follow-ups

  • PENDING_LIBFUNCS had no staleness guard. It is only used to permit extras, so an entry that has since been audited is neither missing nor extra and sits there indefinitely. Added the mirror of the EXCLUDED_LIBFUNCS check; it flagged all three sha512_* entries, which are now in the audited list at the pinned 2.19.4, so the const is emptied.
  • The libfunc argument assertions moved to compiler_test.rs. They sat inside a test that constructs SierraToCasmCompiler, so they only ran where the pinned starknet-sierra-compile is installed, though the function needs nothing but a resolvable path. They now also pin the bundled path, not just the flag — pointing Bundled at the wrong file previously still passed.
  • test_max_memory_usage positive flow overrode max_memory_usage with the value SIERRA_COMPILATION_CONFIG already sets, so it rebuilt the same compiler and compared a value with itself. Now uses a limit that differs from the default.

Testing

cargo test -p apollo_compile_to_casm — 13 passed. apollo_sierra_compilation_config, apollo_node_config, apollo_deployments — 30 passed. fmt + clippy --all-targets -- -D warnings clean.

🤖 Generated with Claude Code

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@cursor

cursor Bot commented Sep 16, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Changes are confined to test fixtures, test assertions, and wiring libfunc list names to constants; no production compilation behavior beyond using the canonical builtin list name strings.

Overview
Adds a coupon contract fixture (coupon_contract.sierra.json plus Cairo source with experimental coupons enabled) that uses coupon_buy, which the shipped bundled allowlist deliberately excludes.

A new integration test bundled_libfuncs_list_rejects_an_excluded_libfunc asserts All and Audited compile that class while Bundled fails with coupon_buy is not allowed—the only behavioral difference from comparing CASM output alone.

Test hygiene: PENDING_LIBFUNCS is cleared (sha512 entries are now audited) and gains a staleness check mirroring EXCLUDED_LIBFUNCS. Libfunc CLI argument tests move to lightweight compiler_test.rs (no Sierra binary), pinning built-in list names via BUILTIN_* constants and verifying the bundled list resolves to the correct file path. test_max_memory_usage’s positive case uses a non-default memory cap so it actually exercises the limit.

libfunc_list_arg now passes Cairo’s BUILTIN_AUDITED_LIBFUNCS_LIST / BUILTIN_ALL_LIBFUNCS_LIST instead of string literals "audited" / "all".

Reviewed by Cursor Bugbot for commit d46a670. Bugbot is set up for automated code reviews on this repo. Configure here.

@avi-starkware avi-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avi-starkware+AGNT made 6 comments.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on avi-starkware and Yoni-Starkware).


crates/apollo_compile_to_casm/src/compiler_test.rs line 6 at r1 (raw file):

use crate::constants::BUNDLED_ALLOWED_LIBFUNCS_PATH;

#[test]

built_in_libfunc_lists_are_selected_by_name asserts the same string literals that libfunc_list_arg is written with, so it pins the code against itself rather than against cairo-lang.

The match arms in compiler.rs spell "audited" and "all" on lines 75-76, and the test spells them again. If cairo-lang renamed a built-in list, the compiler invocation would break and this test would stay green.

cairo_lang_starknet_classes::allowed_libfuncs::{BUILTIN_AUDITED_LIBFUNCS_LIST, BUILTIN_ALL_LIBFUNCS_LIST} are both pub const in the pinned 2.19.4 and would pin the actual contract. Neither is imported anywhere in the repo today, so this needs a new use here.

Dropping the test is a fair answer too, given what it covers as written.

Nonblocking.


crates/apollo_compile_to_casm/src/compile_test.rs line 38 at r1 (raw file):

};

// Libfuncs in allowed_libfuncs.json but not yet in Cairo's audited list.

PENDING_LIBFUNCS is empty now, so the pending_set filter and the new staleness assert are both no-ops. Keep the empty hook, or delete the three pieces and re-add them the next time allowed_libfuncs.json runs ahead of the audited list?

Asking which you intended, not reporting a bug. The guard works. Populating it fires the assert as designed:

PENDING_LIBFUNCS = &["array_append"]
-> panics at compile_test.rs:188

And there is a real argument for keeping it: it is the only assert that catches a pending entry that has since been audited. Such an entry sits in both maps, so the earlier missing/extra check stops seeing it, and this one takes over. If that is the intent, saying so in the comment above the constant would make the empty slice read as deliberate rather than leftover.

Nonblocking.


crates/apollo_compile_to_casm/src/compile_test.rs line 49 at r1 (raw file):

const EXCLUDED_LIBFUNCS: &[&str] = &["coupon_buy", "coupon_call", "coupon_refund"];

// A class using an excluded libfunc. Regenerate from the sibling .cairo with:

The regeneration recipe on EXCLUDED_LIBFUNC_CLASS_PATH does not regenerate coupon_contract.sierra.json.

coupon_buy sits behind an experimental Cairo feature that has to be enabled in a crate config, so the fixture cannot be produced from a single .cairo file at all. I ran the documented command four ways against the cairo-package starknet-compile:

# as written, pointed at the sibling .cairo
Error: The given path is a file, but --single-file was not supplied.

# adding -s
error[E2167]: Coupons are disabled in the current crate. You can enable them by
              enabling the coupons experimental feature in the crate config.
error[E2030]: Wrong number of arguments. Expected 1, found: 2

# pointed at resources/ as a crate
Failed to load project config: No such file or directory

# a real crate dir whose cairo_project.toml carries
#   [config.global] edition = "2024_07"
#   [config.global.experimental_features] coupons = true
exit 0

Only the last one produces the fixture. Worth noting -s alone looks like the fix and is not.

scripts/install_compiler_binaries.sh also never installs starknet-compile. It installs starknet-sierra-compile and starknet-native-compile, so whoever picks this up next has to find the binary under target/bin/cairo_package__<CAIRO1_COMPILER_VERSION>/cairo/bin/. Worth naming in the comment.

Committing the crate layout the fixture was built from is the durable fix. Spelling the full recipe out in the comment works too.

I'd fix it rather than leave it, because nothing in CI compiles the .cairo, so drift between it and the committed .sierra.json stays invisible.

On the good news side, the committed fixture is genuine: regenerating it that last way reproduced the file byte for byte except sierra_program felts 2 and 5, which carry the sierra and compiler versions.

Caveat on all of the above: measured with starknet-compile 2.19.0-rc.3. No 2.19.4 build exists on my machine, though the repo pins 2.19.4.

Nonblocking.


crates/apollo_compile_to_casm/src/compile_test.rs line 213 at r1 (raw file):

}

/// The default selects the bundled list, which has to be resolved from disk; a node that ships

The doc comment on bundled_libfuncs_list_rejects_an_excluded_libfunc opens with two lines written for a different test.

Lines 213-214 documented compile_with_the_default_config before this change. The new test landed between them and that function, and Rust attaches a contiguous /// block downward, so all four lines now document the new test:

/// The default selects the bundled list, which has to be resolved from disk; a node that ships
/// without it would panic on startup rather than fail a compilation.
/// The built-in lists both permit [`EXCLUDED_LIBFUNCS`], so rejecting this class is the only
/// behaviour that tells the bundled list apart from `Audited` and `All`.
#[test]
fn bundled_libfuncs_list_rejects_an_excluded_libfunc() {

The new test never builds SierraCompilationConfig::default(), so the first sentence misdescribes it. And compile_with_the_default_config on line 243 is left with no doc at all.

Moving lines 213-214 back down onto compile_with_the_default_config fixes it.

One thing that does not fix it, in case it looks like it would: putting a blank line between the two halves. rustc reads a blank line inside a doc block as a paragraph break and still attaches both halves to the item below. I checked that before writing this.

Nonblocking.


crates/apollo_compile_to_casm/src/compile_test.rs line 226 at r1 (raw file):

        ..SIERRA_COMPILATION_CONFIG
    });
    let bundled_list_compiler = SierraToCasmCompiler::new(SierraCompilationConfig {

The bundled-compiler construction is written three times in this file now. Lines 226-229 are byte-identical to lines 204-207 in compile_against_the_bundled_libfuncs_list, and 222-225 differ only in the variant:

md5(204-207) = 78ed5c9284336cc83a30167fc24499c3
md5(226-229) = 78ed5c9284336cc83a30167fc24499c3

A helper next to compiler() collapses each site to one line:

fn compiler_with_libfuncs_list(allowed_libfuncs_list: AllowedLibfuncsList) -> SierraToCasmCompiler {
    SierraToCasmCompiler::new(SierraCompilationConfig {
        allowed_libfuncs_list,
        ..SIERRA_COMPILATION_CONFIG
    })
}

compiler() can then delegate to it with AllowedLibfuncsList::All, since SIERRA_COMPILATION_CONFIG already sets that on line 35.

Nonblocking, and easy to drop if you'd rather keep each construction spelled out.


crates/apollo_compile_to_casm/src/compile_test.rs line 231 at r1 (raw file):

    });

    compiler().compile(excluded_libfunc_class.clone()).expect("`All` must accept the class.");

The All compile on this line cannot fail while the Audited compile just below it passes.

audited is a subset of all in cairo-lang-starknet-classes 2.19.4, which is what Cargo.lock resolves at this commit, and upstream has a test enforcing the relation, so it holds by invariant rather than by luck:

audited_not_in_all == []

So the line spawns a compiler subprocess that carries no signal the Audited line does not already carry.

Before you act on that, there is a reason to keep it. The doc comment above the test claims "The built-in lists both permit [EXCLUDED_LIBFUNCS]", and this line is what demonstrates the "both" half. If you want that claim in the doc, keeping the line that backs it is a fair trade. The leaner version drops the line and states the subset relation in the comment instead.

Your call, and I'm happy either way. Nonblocking.

@Yoni-Starkware
Yoni-Starkware force-pushed the yonatan/libfunc-test-followups branch from e74a7c1 to c109f2d Compare September 17, 2026 17:24
…at it excludes

Add a class using a coupon libfunc as a fixture, with the crate it is built from, since coupons
are behind an experimental feature that a single source file cannot enable. It compiles under both
built-in lists and is rejected by the bundled one, which is the only behaviour that tells the three
apart.

Follow-ups from the #15133 review:
- Guard `PENDING_LIBFUNCS` against entries that have since been audited, mirroring the
  `EXCLUDED_LIBFUNCS` check, and drop the three entries it flags.
- Move the libfunc argument assertions into `compiler_test.rs`, where they run without the
  compiler binary, and pin the bundled path rather than only the flag.
- Give the positive flow in `test_max_memory_usage` a limit that differs from the default, so it
  no longer compares a value with itself.

Take the built-in list names from `cairo_lang_starknet_classes` rather than spelling them, so the
compiler invocation cannot drift from the names cairo-lang accepts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Yoni-Starkware
Yoni-Starkware force-pushed the yonatan/libfunc-test-followups branch from c109f2d to d46a670 Compare September 17, 2026 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants