chore(blockifier): add runnable cairo version to the cairo version enum #2503

meship-starkware · 2024-12-05T17:20:20Z

No description provided.

reviewable-StarkWare · 2024-12-05T17:20:29Z

This change is

github-actions · 2024-12-05T17:22:13Z

Artifacts upload workflows:

codecov · 2024-12-05T18:02:18Z

Codecov Report

Attention: Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.

Project coverage is 49.83%. Comparing base (e3165c4) to head (53605f5).
Report is 749 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/test_utils/contracts.rs	70.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2503      +/-   ##
==========================================
+ Coverage   40.10%   49.83%   +9.73%     
==========================================
  Files          26      291     +265     
  Lines        1895    33458   +31563     
  Branches     1895    33458   +31563     
==========================================
+ Hits          760    16675   +15915     
- Misses       1100    15652   +14552     
- Partials       35     1131    +1096

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

dorimedini-starkware

Reviewed 33 of 33 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware)

crates/blockifier/src/test_utils/contracts.rs line 131 at r1 (raw file):

            Self::SierraExecutionInfoV1Contract => {
                CairoVersion::Cairo1(RunnableCairoVersion::Native)
            }

this will change in the next PR to return the inner runnable cairo1 version?
or will this contract stay "only native"?

Code quote:

            Self::SierraExecutionInfoV1Contract => {
                CairoVersion::Cairo1(RunnableCairoVersion::Native)
            }

crates/blockifier/src/test_utils/contracts.rs line 310 at r1 (raw file):

    pub fn get_sierra_path(&self) -> String {
        assert_ne!(self.cairo_version(), CairoVersion::Cairo0);
        assert_ne!(self, &Self::ERC20(CairoVersion::Cairo1(RunnableCairoVersion::Casm)));

what if the ERC20 is native? this can be constructed, compiler won't protect us

Code quote:

assert_ne!(self, &Self::ERC20(CairoVersion::Cairo1(RunnableCairoVersion::Casm)));

crates/blockifier/src/test_utils.rs line 63 at r1 (raw file):

#[derive(Clone, Hash, PartialEq, Eq, Copy, Debug)]
pub enum RunnableCairoVersion {

please add the 1 for clarity.
I would also drop the Version - it's not a difference in versions, actually, just the execution framework

Suggestion:

RunnableCairo1

crates/blockifier/src/test_utils.rs line 106 at r1 (raw file):

            }
        }
    }

is this deleted in PR 2464? because it should be...
if native feature is active, calling this function should always panic imo, Cairo0.other_version() is not well defined if native exists

Code quote:

    pub fn other(&self) -> Self {
        match self {
            Self::Cairo0 => CairoVersion::Cairo1(RunnableCairoVersion::Casm),
            CairoVersion::Cairo1(RunnableCairoVersion::Casm) => Self::Cairo0,
            #[cfg(feature = "cairo_native")]
            CairoVersion::Cairo1(RunnableCairoVersion::Native) => {
                panic!("There is no other version for native")
            }
        }
    }

crates/blockifier/src/test_utils.rs line 136 at r1 (raw file):

            Self::CairoVersion(CairoVersion::Cairo1(RunnableCairoVersion::Native)) => {
                TrackedResource::SierraGas
            }

less boilerplate :)

Suggestion:

            Self::CairoVersion(CairoVersion::Cairo1(_)) => TrackedResource::SierraGas,

crates/blockifier/src/test_utils.rs line 370 at r1 (raw file):

                    )
                }
            }

this macro should behave the same for all cairo1 contracts regardless of running framework, right?

Suggestion:

            CairoVersion::Cairo1(_) => {
                if let $crate::transaction::errors::TransactionExecutionError::ValidateTransactionError {
                    error, ..
                } = $error {
                    assert_eq!(
                        error.to_string(),
                        "Execution failed. Failure reason: 0x496e76616c6964207363656e6172696f \
                         ('Invalid scenario')."
                    )
                }
            }

dorimedini-starkware

Reviewed 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware)

meship-starkware

Reviewable status: 0 of 33 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware)

crates/blockifier/src/test_utils.rs line 63 at r1 (raw file):