test(blockifier): parametrize feature contracts and tests by runnable type

[dorimedini-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[dorimedini-starkware]

• refactor(blockifier): cairo version enum has only two variants #2352
• test(blockifier): parametrize feature contracts and tests by runnable type #2344 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-28T13:40:35Z
• Started at 2024-11-28T13:52:39Z
• Started at 2024-11-28T13:57:41Z
• Started at 2024-11-28T14:52:19Z
• Started at 2024-12-02T08:22:23Z
• Started at 2024-12-02T08:50:40Z
• Started at 2024-12-02T09:05:08Z
• Started at 2024-12-03T13:02:41Z

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 49.59%. Comparing base (e3165c4) to head (29115d1).
Report is 642 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/test_utils/contracts.rs	81.96%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2344      +/-   ##
==========================================
+ Coverage   40.10%   49.59%   +9.49%     
==========================================
  Files          26      291     +265     
  Lines        1895    32936   +31041     
  Branches     1895    32936   +31041     
==========================================
+ Hits          760    16336   +15576     
- Misses       1100    15505   +14405     
- Partials       35     1095    +1060     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dorimedini-starkware]

Reviewable status: 0 of 45 files reviewed, all discussions resolved

---

a discussion (no related file):
This PR will not get in until native compiled contracts exist in our test framework (and even then may indicate more required work).

What was done here:

1. Created a RunnableContractVersion enum for the FeatureContracts variants.
2. Stopped using CairoVersion for FeatureContracts variants (and removed native variant in the next PR)
3. Updated test utils that construct feature contracts to accept the new enum instead of CairoVersion
4. Updated and parametrized tests (or marked with TODOs to parametrize).
5. Note that parametrization creates a lot of boilerplate, e.g:

#[rstest]
fn test(#[values(Cairo0, Cairo1)] cairo_version)

now becomes

#[rstest]
#[case(Cairo0)]
#[case(Cairo1Casm)]
#[cfg_attr(feature = "cairo_native", case(Cairo1Native))]
fn test(#[case] cairo_version, #[case] some_bool)

[meship-starkware]

Reviewed 9 of 44 files at r1, 35 of 36 files at r2, all commit messages.
Reviewable status: 44 of 45 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/test_utils/transfers_generator.rs line 12 at r2 (raw file):

use starknet_types_core::felt::Felt;

use super::contracts::RunnableContractVersion;

Please Avoid using super

Code quote:

use super::contracts::RunnableContractVersion;

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 535 at r2 (raw file):

#[cfg_attr(
    feature = "cairo_native",
    case::declare_cairo1(RunnableContractVersion::Cairo1Casm, TransactionVersion::THREE)

Shouldn't it be native?

Code quote:

case::declare_cairo1(RunnableContractVersion::Cairo1Casm

---

crates/blockifier/src/execution/stack_trace_test.rs line 289 at r2 (raw file):

    "fail",
    "0x6661696c ('fail')", (0_u16, 0_u16))
)]

Note to self: see if it makes the NM stack_trace test PR irrelevant.

Code quote:

    "invoke_call_chain",
    "0x4469766973696f6e2062792030 ('Division by 0')", (0_u16, 0_u16))
)]
#[cfg_attr(feature = "cairo_native", case(
    RunnableContractVersion::Cairo1Native,
    "fail",
    "0x6661696c ('fail')", (0_u16, 0_u16))
)]

[dorimedini-starkware]

Reviewable status: 44 of 45 files reviewed, 3 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 535 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Shouldn't it be native?

whoops! done, thanks

---

crates/blockifier/src/test_utils/transfers_generator.rs line 12 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Please Avoid using super

Done.

[meship-starkware]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

[meship-starkware]

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/blockifier/src/execution/stack_trace_test.rs line 289 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Note to self: see if it makes the NM stack_trace test PR irrelevant.

The answer is no, but there will be merge conflicts.
https://reviewable.io/reviews/starkware-libs/sequencer/2164#-

[meship-starkware]

Reviewable status: all files reviewed (commit messages unreviewed), 5 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/test_utils/contracts.rs line 104 at r4 (raw file):

        Self::Cairo0
    }
}

Why is the default cairo0? I Would have chosen Cairo1Casm as the default.

Code quote:

impl Default for RunnableContractVersion {
    fn default() -> Self {
        Self::Cairo0
    }
}

---

crates/blockifier/src/test_utils/contracts.rs line 118 at r4 (raw file):

            panic!("Transaction version {:?} is not supported.", tx_version)
        }
    }

Will this support Native as well? If so, would we choose according to the Sierra version (after AvivY will add the Sierra to the declared TX)?

Code quote:

    pub fn from_declare_tx_version(tx_version: TransactionVersion) -> Self {
        if tx_version == TransactionVersion::ZERO || tx_version == TransactionVersion::ONE {
            Self::Cairo0
        } else if tx_version == TransactionVersion::TWO || tx_version == TransactionVersion::THREE {
            Self::Cairo1Casm
        } else {
            panic!("Transaction version {:?} is not supported.", tx_version)
        }
    }

---

crates/blockifier/src/test_utils/contracts.rs line 127 at r4 (raw file):

            RunnableContractVersion::Cairo1Casm => Self::Cairo1,
            #[cfg(feature = "cairo_native")]
            RunnableContractVersion::Cairo1Native => Self::Native,

The intention is for Cairo1Native to also return to Cairo1, right? Do you wait until we can remove the cairo_native folder to do so?

Code quote:

            RunnableContractVersion::Cairo0 => Self::Cairo0,
            RunnableContractVersion::Cairo1Casm => Self::Cairo1,
            #[cfg(feature = "cairo_native")]
            RunnableContractVersion::Cairo1Native => Self::Native,

---

crates/blockifier/src/test_utils/contracts.rs line 475 at r4 (raw file):

                    RunnableContractVersion::Cairo0,
                    "Default version in iteration must be Cairo0 for this logic to be correct."
                );

Why? Because we only handle the cairo1 cases? Can't it be casm, and we will change the handling of casm to cairo0?

Code quote:

                assert_eq!(
                    contract.runnable_contract_version(),
                    RunnableContractVersion::Cairo0,
                    "Default version in iteration must be Cairo0 for this logic to be correct."
                );

[dorimedini-starkware]

Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @meship-starkware)

---

crates/blockifier/src/test_utils/contracts.rs line 104 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Why is the default cairo0? I Would have chosen Cairo1Casm as the default.

because the default CairoVersion is Cairo0, I am being consistent

---

crates/blockifier/src/test_utils/contracts.rs line 118 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Will this support Native as well? If so, would we choose according to the Sierra version (after AvivY will add the Sierra to the declared TX)?

I am assuming the difference between casm and native is irrelevant for declares... a declare declares sierra anyway

---

crates/blockifier/src/test_utils/contracts.rs line 127 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

The intention is for Cairo1Native to also return to Cairo1, right? Do you wait until we can remove the cairo_native folder to do so?

yes, done here (next PR)

---

crates/blockifier/src/test_utils/contracts.rs line 475 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Why? Because we only handle the cairo1 cases? Can't it be casm, and we will change the handling of casm to cairo0?

the logic of the has_all_runnable_versions case is: add cairo1 and native instances.
if the original instance is not cairo0, this logic is incorrect.
want me to explicitly add the three contract versions here? that sounds better even, I can use EnumIter on RunnableContractVersion and I won't need the #[cfg] here at all...
done