test(cairo_native): test papyrus state reader get compiled class

[meship-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-25T12:05:50Z
• Started at 2024-12-25T12:22:21Z
• Started at 2024-12-25T12:25:26Z
• Started at 2024-12-29T07:35:05Z
• Started at 2024-12-29T08:16:22Z
• Started at 2024-12-29T08:16:55Z
• Started at 2024-12-30T19:11:41Z
• Started at 2024-12-30T19:38:57Z
• Started at 2024-12-30T20:00:28Z
• Started at 2024-12-30T20:06:39Z
• Started at 2024-12-31T06:17:08Z
• Started at 2025-01-02T08:14:08Z
• Started at 2025-01-02T08:44:21Z
• Started at 2025-01-02T13:43:04Z
• Started at 2025-01-08T15:22:23Z
• Started at 2025-01-08T15:24:09Z
• Started at 2025-01-09T12:00:47Z

[meship-starkware]

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions

---

Cargo.lock line 7708 at r4 (raw file):

 "blockifier",
 "indexmap 2.6.0",
 "infra_utils",

No need will revert

---

crates/papyrus_state_reader/Cargo.toml line 30 at r4 (raw file):

[build-dependencies]
infra_utils.workspace = true

No need will revert

[meship-starkware]

Reviewed 2 of 8 files at r3, 3 of 5 files at r4.
Reviewable status: 5 of 9 files reviewed, 2 unresolved discussions

[meship-starkware]

Reviewable status: 5 of 9 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 150 at r6 (raw file):

        assert!(run_cairo_native);
    }
    // We only cache the sierra is the contract is cairo1 and the run cairo native flag is on.

Suggestion:

// We only cache the sierra if the contract is cairo1 and the run cairo native flag is on.

[noaov1]

Reviewed 2 of 8 files at r3, 5 of 5 files at r4, 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

---

crates/papyrus_state_reader/Cargo.toml line 9 at r6 (raw file):

[features]
cairo_native = ["blockifier/cairo_native"]

Suggestion:

[features]
cairo_native = ["blockifier/cairo_native"]
testing = ["rstest"]

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 93 at r6 (raw file):

    contract: FeatureContract,
    contract_manager_config: ContractClassManagerConfig,
) -> papyrus_storage::StorageResult<PapyrusReader> {

Suggestion:

fn build_papyrus_state_reader_and_declare_contract(
    class_hash: ClassHash,
    contract: FeatureContract,
    contract_manager_config: ContractClassManagerConfig,
) {

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 103 at r6 (raw file):

            let thin_state_diff = ThinStateDiff {
                declared_classes: IndexMap::from([(class_hash, test_compiled_class_hash)]),
                nonces: IndexMap::from([(contract.get_instance_address(1), Nonce(1.into()))]),

Why is it needed?

Code quote:

                nonces: IndexMap::from([(contract.get_instance_address(1), Nonce(1.into()))]),

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 138 at r6 (raw file):

#[case(false, false)]
#[cfg_attr(feature = "cairo_native", case(true, false))]
#[cfg_attr(feature = "cairo_native", case(true, true))]

Name the cases?

Code quote:

#[case(false, false)]
#[cfg_attr(feature = "cairo_native", case(true, false))]
#[cfg_attr(feature = "cairo_native", case(true, true))]

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 145 at r6 (raw file):

    #[case] run_cairo_native: bool,
    #[case] wait_on_native_compilation: bool,
) -> papyrus_storage::StorageResult<()> {

Suggestion:

fn test_get_compiled_class(
    #[values(CairoVersion::Cairo0, CairoVersion::Cairo1(RunnableCairo1::Casm))]
    cairo_version: CairoVersion,
    #[values(true, false)] is_cached: bool,
    #[case] run_cairo_native: bool,
    #[case] wait_on_native_compilation: bool,
){

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 146 at r6 (raw file):

    #[case] wait_on_native_compilation: bool,
) -> papyrus_storage::StorageResult<()> {
    // sanity check

Suggestion:

// Sanity check.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 149 at r6 (raw file):

    if wait_on_native_compilation {
        assert!(run_cairo_native);
    }

Suggestion:

    if !run_cairo_native {
        assert!(!wait_on_native_compilation);
    }

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 150 at r6 (raw file):

        assert!(run_cairo_native);
    }
    // We only cache the sierra is the contract is cairo1 and the run cairo native flag is on.

Suggestion:

    // We store the sierra with the casm only when the casm is cairo1 and the native flag is enabled.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 166 at r6 (raw file):

        contract_manager_config,
    )?;

What about the case where the native is cached?

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 168 at r6 (raw file):

    if is_cached {
        // Here we are testing the flow that the classes are already in the cache.

Why is it interesting? You can assume that get_cached_casm returns a cached casm (it does not matter if it was cached before)

Suggestion:

        // Simulate the scenario where the classes are already in the cache.
        // Create a cached casm and store it in the cache.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 188 at r6 (raw file):

            "When compiling cairo1 contract with the wait_on_native_compilation flag we expect to \
             get the native class."
        );

Suggestion:

    if cached_with_sierra {
         // TODO: Test that a compilation request was sent.
        if wait_on_native_compilation {
            #[cfg(feature = "cairo_native")]
            assert_matches!(
                compiled_class,
                RunnableCompiledClass::V1Native(_),
                "We should have waited to the native class."
            );
        }
        else {
             assert_matches!(
                compiled_class,
                RunnableCompiledClass::V1(_)
            );
            
        }

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 189 at r6 (raw file):

             get the native class."
        );
    } else {

We should consider adding an assert that the native flag is off when the native feature is off.

Suggestion:

} else {
    // Cairo native flag is off. 

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 194 at r6 (raw file):

            test_contract.get_runnable_class(),
            "get compiled class should return the correct class"
        );

Suggestion:

        assert_eq!(
            compiled_class,
            test_contract.get_runnable_class(),
            "`get_compiled_class` should return the casm"
        );

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 204 at r6 (raw file):

        assert_matches!(cached_casm, Some(CachedCasm::WithoutSierra(_)));
    }
    Ok(())

Not needed. It should be tested when testing get_cached_casm.

Code quote:

    // Check that the casm cached type is as expected.
    let cached_casm = papyrus_reader.contract_class_manager.get_casm(&test_class_hash);
    if cached_with_sierra {
        assert_matches!(cached_casm, Some(CachedCasm::WithSierra(_, _)));
    } else {
        assert_matches!(cached_casm, Some(CachedCasm::WithoutSierra(_)));
    }
    Ok(())

[avi-starkware]

crates/papyrus_state_reader/src/papyrus_state_test.rs line 163 at r7 (raw file):

    let cached_with_sierra = run_cairo_native && matches!(cairo_version, CairoVersion::Cairo1(_));
    // We don't need a native contract because we only use the contract to get the casm amd sierra
    // classes.

Suggestion:

    // We don't need a native contract because we only use the contract to get the casm and sierra
    // classes.

[meship-starkware]

Reviewable status: 6 of 9 files reviewed, 10 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 103 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is it needed?

Done.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 166 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What about the case where the native is cached?

I will add a different test for it is too different from the other cases

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 168 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is it interesting? You can assume that get_cached_casm returns a cached casm (it does not matter if it was cached before)

The flow would be slightly different if they had been cached before, but you are right. It is not different enough to justify a test case

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 189 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

We should consider adding an assert that the native flag is off when the native feature is off.

Done.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 204 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Not needed. It should be tested when testing get_cached_casm.

I disagree we cache the casm in this function. I want to see that we did not cache the wrong thing in some cases. that is the reason I only check the type and, not the contracts

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 93 at r6 (raw file):

    contract: FeatureContract,
    contract_manager_config: ContractClassManagerConfig,
) -> papyrus_storage::StorageResult<PapyrusReader> {

Done.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 145 at r6 (raw file):

    #[case] run_cairo_native: bool,
    #[case] wait_on_native_compilation: bool,
) -> papyrus_storage::StorageResult<()> {

Done.

---

crates/papyrus_state_reader/Cargo.toml line 9 at r6 (raw file):

[features]
cairo_native = ["blockifier/cairo_native"]

Done.

[meship-starkware]

Reviewable status: 6 of 9 files reviewed, 11 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 163 at r7 (raw file):

    let cached_with_sierra = run_cairo_native && matches!(cairo_version, CairoVersion::Cairo1(_));
    // We don't need a native contract because we only use the contract to get the casm amd sierra
    // classes.

Done.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 232 at r7 (raw file):

#[cfg(feature = "cairo_native")]
#[test]
fn test_get_compiled_class_when_native_is_cached() {

I can add it as a case in the test_compiled class, but it will result in more cases (I can no longer use values on the Cairo version), and the test would be a bit harder to read. On a positive note, if I merge this test, the compiled test will have a similar build to the function. I think that splitting it into two tests is more elegant, but I am not fixed on this

Code quote:

fn test_get_compiled_class_when_native_is_cached() {

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 188 at r6 (raw file):

            "When compiling cairo1 contract with the wait_on_native_compilation flag we expect to \
             get the native class."
        );

Done.

[meship-starkware]

Reviewable status: 6 of 9 files reviewed, 11 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 168 at r6 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

The flow would be slightly different if they had been cached before, but you are right. It is not different enough to justify a test case

Done.

[meship-starkware]

Reviewable status: 6 of 9 files reviewed, 11 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 166 at r6 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I will add a different test for it is too different from the other cases

Done.

[noaov1]

Reviewed 2 of 3 files at r7, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 204 at r6 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I disagree we cache the casm in this function. I want to see that we did not cache the wrong thing in some cases. that is the reason I only check the type and, not the contracts

We don't. We cache the casm in get_cached_casm and decide on the cached type in `get_compiled_class_inner.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 232 at r7 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I can add it as a case in the test_compiled class, but it will result in more cases (I can no longer use values on the Cairo version), and the test would be a bit harder to read. On a positive note, if I merge this test, the compiled test will have a similar build to the function. I think that splitting it into two tests is more elegant, but I am not fixed on this

I think that you can add it with a native_exist flag (and in this case, set the native in the cache and expect a ntive class as the returned value). But it can also be as a separate test.
If you choose to separate, consider asserting no native in cache in the previous test.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 88 at r9 (raw file):

fn build_papyrus_state_reader_and_declare_contract(
    class_hash: ClassHash,

Why is it needed?
Can you use get_class_hash of FeatureContract?

Code quote:

    class_hash: ClassHash,

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 177 at r9 (raw file):

    if cached_with_sierra {
        // TODO: Test that a compilation request was sent.

Next PR?

Code quote:

        // TODO: Test that a compilation request was sent.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 189 at r9 (raw file):

                compiled_class,
                RunnableCompiledClass::V1(_),
                "`get_compiled_class` should return Cario1 casm"

Suggestion:

                "We do not wait for native, return the cairo1 casm."

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 197 at r9 (raw file):

            RunnableCompiledClass::V0(_),
            "`get_compiled_class` should return Cario0 casm"
        );

Why is it needed?

Code quote:

    } else if run_cairo_native {
        assert_matches!(
            compiled_class,
            RunnableCompiledClass::V0(_),
            "`get_compiled_class` should return Cario0 casm"
        );

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 218 at r9 (raw file):

fn test_get_compiled_class_when_native_is_cached() {
    let ((storage_reader, _), _) = papyrus_storage::test_utils::get_test_storage();
    let test_contract = FeatureContract::TestContract(CairoVersion::Cairo1(RunnableCairo1::Native));

Papyrus does not hold native classes.

Suggestion:

    let test_contract = FeatureContract::TestContract(CairoVersion::Cairo1(RunnableCairo1::Cairo1));

---

crates/blockifier/src/state/contract_class_manager.rs line 154 at r9 (raw file):

    }

    #[cfg(all(feature = "cairo_native", feature = "testing"))]

Suggestion:

#[cfg(all(feature = "cairo_native", feature = "testing", test))]

[meship-starkware]

Reviewable status: 4 of 9 files reviewed, 7 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 204 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

We don't. We cache the casm in get_cached_casm and decide on the cached type in `get_compiled_class_inner.

Done.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 177 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Next PR?

Yes let's do it in another PR.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 197 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is it needed?

I wanted to ensure that if we run with Cairo native, the only case we do not save the Sierra is Cairo 0.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 218 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Papyrus does not hold native classes.

We don't save the casm in this case

---

crates/blockifier/src/state/contract_class_manager.rs line 154 at r9 (raw file):

    }

    #[cfg(all(feature = "cairo_native", feature = "testing"))]

No need becuse we only call it from another crate.

---

crates/papyrus_state_reader/src/papyrus_state_test.rs line 189 at r9 (raw file):

                compiled_class,
                RunnableCompiledClass::V1(_),
                "`get_compiled_class` should return Cario1 casm"

Done.