Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cairo_native): remove native FC folder #2464

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

meship-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r1.
Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @Yoni-Starkware)

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 12.82051% with 34 lines in your changes missing coverage. Please review.

Project coverage is 71.60%. Comparing base (e3165c4) to head (24398b4).
Report is 750 commits behind head on main.

Files with missing lines Patch % Lines
crates/blockifier/src/test_utils/contracts.rs 12.82% 34 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2464       +/-   ##
===========================================
+ Coverage   40.10%   71.60%   +31.49%     
===========================================
  Files          26       98       +72     
  Lines        1895    13611    +11716     
  Branches     1895    13611    +11716     
===========================================
+ Hits          760     9746     +8986     
- Misses       1100     3435     +2335     
- Partials       35      430      +395     

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

Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


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

            | Self::TestContract(version)
            | Self::ERC20(version)
            | Self::LegacyTestContract(version) => *version,

This has a problematic Meaning now. Up until now, if you had a version, you had all versions but the legacy contract only has Cairo1, and Native does not have Cairo0

Code quote:

Self::LegacyTestContract(version) =

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

            | Self::TestContract(v)
            | Self::ERC20(v)
            | Self::LegacyTestContract(v) => *v = version,

This is risky. We need to find a way to disallow the creation of Cairo0 LeagayContract. Which might be problematic for the all contacts method see below.

Code quote:

           | Self::LegacyTestContract(v) => *v = version,

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

                let mut other_contract = contract;
                other_contract.set_cairo_version(contract.cairo_version().other());
                vec![other_contract].into_iter()

I can go the direct way here, check for the legacy contract, and remove the has only casm and native, but I prefer the generalization here. This function assumes that there is no special treatment for Native CairoVersion.
Also, the fact that the default is Cairo0 makes us create a LegacyTestContract(Cairo0), which means nothing.

Code quote:

            } else if contract.has_only_casm_and_native() {
                let mut other_contract = contract;
                other_contract.set_cairo_version(contract.cairo_version().other());
                vec![other_contract].into_iter()

@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from 3e035be to faa8438 Compare December 5, 2024 07:25
Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 245 at r2 (raw file):

            }
            #[cfg(feature = "cairo_native")]
            CairoVersion::Native => panic!("This test does not support native CairoVersion."),

This won't work if we decide in the future to add native compiled files, but as we do not have a future plan to do so I think panic is a good solution.

Code quote:

CairoVersion::Native => panic!("This test does not support native CairoVersion."),

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

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


crates/blockifier/src/test_utils/contracts.rs line 450 at r2 (raw file):

            }
        })
    }

Let's make it simple. Remove all functions that are used only here, and implement all_contracts using a single match expression explicitly.
Remove the .other() function as well

Code quote:

    pub fn all_contracts() -> impl Iterator<Item = Self> {
        // EnumIter iterates over all variants with Default::default() as the cairo
        // version.
        Self::iter().flat_map(|contract| {
            if contract.has_all_versions() {
                let mut other_contract = contract;
                other_contract.set_cairo_version(contract.cairo_version().other());
                vec![contract, other_contract].into_iter()
            } else if contract.has_only_casm_and_native() {
                let mut other_contract = contract;
                other_contract.set_cairo_version(contract.cairo_version().other());
                vec![other_contract].into_iter()
            } else {
                vec![contract].into_iter()
            }
        })
    }

@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from faa8438 to 99ddcb0 Compare December 5, 2024 09:50
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)


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

Previously, meship-starkware (Meshi Peled) wrote…

This has a problematic Meaning now. Up until now, if you had a version, you had all versions but the legacy contract only has Cairo1, and Native does not have Cairo0

why does the legacy contract now have two cairo1 variants? why not only cairo1 non-native?
IIRC the legacy contract must be compiled with a legacy compiler --> too old for a native compilation; no?
IIUC we can't run native on old cairo1 contracts...
if I'm right, you can simply revert this added version data in the legacy variant and all is well again.
IF we will have a contract that doesn't have cairo0 but has two cairo1 versions, then you're right... we'll need the following refactor:

enum RunnableCairo1Version {
    Casm,
    Native,
}

enum RunnableVersion {
    Cairo0,
    Cairo1(RunnableCairo1Version),
}

enum FeatureContract {
    SecurityTests,
    TestContract(RunnableVersion),
    OnlyCairo1Contract(RunnableCairo1Version),
    ... etc
}

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

Previously, meship-starkware (Meshi Peled) wrote…

This is risky. We need to find a way to disallow the creation of Cairo0 LeagayContract. Which might be problematic for the all contacts method see below.

see above: I think we should disallow creation of both cairo0 and native, and then there's no problem


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

Previously, meship-starkware (Meshi Peled) wrote…

I can go the direct way here, check for the legacy contract, and remove the has only casm and native, but I prefer the generalization here. This function assumes that there is no special treatment for Native CairoVersion.
Also, the fact that the default is Cairo0 makes us create a LegacyTestContract(Cairo0), which means nothing.

see above (no native legacy): i think this can be simplified

contract.supported_versions().into_iter().map(|version| {
    let mut versioned_contract = contract.clone();
    versioned_contract.set_version(version);
    versioned_contract
})

crates/blockifier/src/test_utils/contracts.rs line 450 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Let's make it simple. Remove all functions that are used only here, and implement all_contracts using a single match expression explicitly.
Remove the .other() function as well

I suggest implementing a supported_versions() method and iterating over it.


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 155 at r3 (raw file):

        // TODO (Meshi 01/01/2025) Remove this once native folder is deleted.
        #[cfg(feature = "cairo_native")]
        CompilationArtifacts::Cairo1Native { .. } => {

do you even need this variant now that you aren't compiling sierra->native?

Code quote:

CompilationArtifacts::Cairo1Native

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 293 at r3 (raw file):

        #[cfg(feature = "cairo_native")]
        CairoVersion::Native => panic!("This test does not support native CairoVersion."),
    }

cleaner, IMO (no let mut x: T; lines)

Suggestion:

    let compiled_paths_on_filesystem = match cairo_version {
        CairoVersion::Cairo0 => {
            verify_and_get_files(cairo_version)
                .into_iter()
                .map(|metadata| metadata.compiled_path().to_string())
                .collect()
        }
        CairoVersion::Cairo1 => {
            let (fetched_compiled_paths_on_filesystem, sierra_paths_on_filesystem) =
                verify_and_get_files(cairo_version)
                    .into_iter()
                    .map(|metadata| (metadata.compiled_path(), metadata.sierra_path()))
                    .collect();

            let mut sierra_paths_from_enum: Vec<String> = FeatureContract::all_feature_contracts()
                .filter(|contract| contract.cairo_version() == cairo_version)
                .map(|contract| contract.get_sierra_path())
                .collect();

            sierra_paths_from_enum.sort();
            sierra_paths_on_filesystem.sort();
            assert_eq!(sierra_paths_from_enum, sierra_paths_on_filesystem);
            fetched_compiled_paths_on_filesystem
        }

        #[cfg(feature = "cairo_native")]
        CairoVersion::Native => panic!("This test does not support native CairoVersion."),
    }

crates/blockifier/src/test_utils/contracts.rs line 302 at r3 (raw file):

                    #[cfg(feature = "cairo_native")]
                    CairoVersion::Native => "sierra",
                },

Suggestion:

                "feature_contracts/cairo{}/{}{}.json",
                match cairo_version {
                    CairoVersion::Cairo0 => "0/compiled",
                    CairoVersion::Cairo1 => "1/compiled",
                    #[cfg(feature = "cairo_native")]
                    CairoVersion::Native => "1/sierra",
                },

@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from 99ddcb0 to 79cbf1d Compare December 5, 2024 11:33
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r3.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @meship-starkware)

Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


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

Previously, dorimedini-starkware wrote…

why does the legacy contract now have two cairo1 variants? why not only cairo1 non-native?
IIRC the legacy contract must be compiled with a legacy compiler --> too old for a native compilation; no?
IIUC we can't run native on old cairo1 contracts...
if I'm right, you can simply revert this added version data in the legacy variant and all is well again.
IF we will have a contract that doesn't have cairo0 but has two cairo1 versions, then you're right... we'll need the following refactor:

enum RunnableCairo1Version {
    Casm,
    Native,
}

enum RunnableVersion {
    Cairo0,
    Cairo1(RunnableCairo1Version),
}

enum FeatureContract {
    SecurityTests,
    TestContract(RunnableVersion),
    OnlyCairo1Contract(RunnableCairo1Version),
    ... etc
}

NM created a new contract that is the same as the legacy contract and called it test_entry_point_v1. We decided it was unnecessary, and now every Cairo1 contract also has a Native version. You are right about the compiler, though. So we can either have two legacy Sierras, or we can get back the native entry point V1 contract. I think adding the NM contract is the better answer. WDYT @Yoni-Starkware?


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 155 at r3 (raw file):

Previously, dorimedini-starkware wrote…

do you even need this variant now that you aren't compiling sierra->native?

The short answer is no, but this would mean that natives would not be treated differently in compile. This is fine for now, but if we would like to use the contracts compile method outside of the feature contracts test, it might be problematic.


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 293 at r3 (raw file):

Previously, dorimedini-starkware wrote…

cleaner, IMO (no let mut x: T; lines)

Done.


crates/blockifier/src/test_utils/contracts.rs line 302 at r3 (raw file):

                    #[cfg(feature = "cairo_native")]
                    CairoVersion::Native => "sierra",
                },

Done.

@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch 2 times, most recently from 62d9f14 to a57e810 Compare December 5, 2024 12:32
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)


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

I think adding the NM contract is the better answer

you mean bringing back the test_entry_point_v1 contract? sounds simplest to me too


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 155 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

The short answer is no, but this would mean that natives would not be treated differently in compile. This is fine for now, but if we would like to use the contracts compile method outside of the feature contracts test, it might be problematic.

As long as native compilation is done during runtime - I don't see a reason to implement any useless code in the feature contracts testing framework.
if we want to start precompiling, we'll refactor... but that can wait


crates/blockifier/src/test_utils/contracts.rs line 22 at r3 (raw file):

#[cfg(feature = "cairo_native")]
use crate::execution::native::contract_class::NativeCompiledClassV1;
#[cfg(feature = "cairo_native")]

do you still need this gate...? you deleted the relevant import

Code quote:

#[cfg(feature = "cairo_native")]

Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Yoni-Starkware)


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

Previously, dorimedini-starkware wrote…

I think adding the NM contract is the better answer

you mean bringing back the test_entry_point_v1 contract? sounds simplest to me too

In this case, either the test_entry_point_v1 contract would have both Ciro1 and Native versions, and then it would be just like the legacy contract, or it would only have native, and then the feature contract test would also have to check native. I prefer the first case. I think it easier to assume that for all Native contracts, we have a casm one, especially when Native also needs the casm file

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)


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

or it would only have native, and then the feature contract test would also have to check native

wdym "the feature contract test would also have to check native"?
you mean the recompile test?
can't the recompile test treat cairo1 casm and native the same way? it is not responsible for generating native artifacts; it just needs to create sierra + casm for all cairo1 sources...
I was thinking the recompilation logic would just treat both the legacy contract and the NM contract as "cairo1" for purpose of recompilation

@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from a57e810 to a3dbc9f Compare December 5, 2024 18:28
@meship-starkware meship-starkware changed the base branch from main to meship/contracts/add_runnable_cairo_version_struct December 5, 2024 18:29
Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 38 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils/contracts.rs line 22 at r3 (raw file):

Previously, dorimedini-starkware wrote…

do you still need this gate...? you deleted the relevant import

Done.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r5.
Reviewable status: 0 of 38 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

@meship-starkware meship-starkware force-pushed the meship/contracts/add_runnable_cairo_version_struct branch from 7b28f4a to 1cd3784 Compare December 8, 2024 09:32
@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from a3dbc9f to 69a8e31 Compare December 8, 2024 09:32
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 38 of 38 files at r7, 16 of 16 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils/contracts.rs line 450 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I suggest implementing a supported_versions() method and iterating over it.

concrete suggestion in another comment


crates/blockifier/src/test_utils/contracts.rs line 138 at r8 (raw file):

            Self::SierraExecutionInfoV1Contract(rv) => {
                assert!(matches!(version, CairoVersion::Cairo1(_)));
                *rv = version.get_runnable_version()

... and then you won't need the get_runnable_version method on the CairoVersion enum

Suggestion:

                match version {
                    CairoVersion::Cairo0 => panic!("{self:?} has not Cairo0 variant."),
                    CairoVersion::Cairo1(runnable) => *rv = runnable,
                }

crates/blockifier/src/test_utils/contracts.rs line 424 at r7 (raw file):

    }

    pub fn all_contracts() -> impl Iterator<Item = Self> {

EDIT: I didn't see the support_cairo_version before this comment, but my suggestion

  1. is still simpler IMO
  2. supports native variants
  3. doesn't rely on the developer to remember to update the function... as it is currently written, if i add another contract (with cairo0, for example), support_cairo_version won't return it in the vector, and the compiler won't complain. only the feature contracts test will fail, right?

how about:

pub fn all_contract_versions(&self) -> impl Iterator<Item = Self>

which returns 1, 2 or 3 contracts, depending on which versions are supported?
this will be a much simpler match:

match self {
    TestContract(_) | ..all contracts with all versions supported.. => {
        // create 2 or 3 variants, depending on native feature active or not
    }
    Legacy | ... all contracts with only one version.. => vec![self].into(),
    SpecialNativeContract => RunnableCairo1::iter().map(|v| ..create cairo1 variant..)
}

that way, in the all_contracts iterator can simply do

Self::iter().flat_map(|contract| contract.all_contract_versions())

Code quote:

pub fn all_contracts() -> impl Iterator<Item = Self>

crates/blockifier/src/test_utils.rs line 100 at r8 (raw file):

    }

    pub fn get_runnable_version(&self) -> RunnableCairoVersion {

ah...
let's differentiate between the "cairo1 runtime" and the "general cairo runtime".
why do you need to convert a CairoVersion to a RunnableCairoVersion at all?
only the Cairo1 variant should have this conversion...
I would prefer a "runs_casm" boolean method, instead of abusing the runnable enum (which should be for cairo1 only)

Code quote:

pub fn get_runnable_version(&self) -> RunnableCairoVersion

Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 38 files at r7, 2 of 16 files at r8.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/feature_contracts/cairo1/test_contract_execution_info_v1.cairo line 1 at r8 (raw file):

#[starknet::contract]

This contract is unnecessary if we use the legacy contract and ignore the Sierra version constraint on the native compilation for the V1 execution info test. Still, I think I'd prefer to keep it for now and add a PR that will remove this contract later so we can Start adding more tests for native.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 16 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

@meship-starkware meship-starkware force-pushed the meship/contracts/add_runnable_cairo_version_struct branch from 1cd3784 to 50f6e95 Compare December 8, 2024 12:23
@meship-starkware meship-starkware force-pushed the meship/contracts/add_runnable_cairo_version_struct branch from 50f6e95 to f8fbee1 Compare December 8, 2024 13:45
@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from 69a8e31 to 60b4d1d Compare December 8, 2024 13:46
Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 25 of 40 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils.rs line 100 at r8 (raw file):

Previously, dorimedini-starkware wrote…

ah...
let's differentiate between the "cairo1 runtime" and the "general cairo runtime".
why do you need to convert a CairoVersion to a RunnableCairoVersion at all?
only the Cairo1 variant should have this conversion...
I would prefer a "runs_casm" boolean method, instead of abusing the runnable enum (which should be for cairo1 only)

Done.


crates/blockifier/src/test_utils/contracts.rs line 450 at r2 (raw file):

Previously, dorimedini-starkware wrote…

concrete suggestion in another comment

Done.


crates/blockifier/src/test_utils/contracts.rs line 424 at r7 (raw file):

Previously, dorimedini-starkware wrote…

EDIT: I didn't see the support_cairo_version before this comment, but my suggestion

  1. is still simpler IMO
  2. supports native variants
  3. doesn't rely on the developer to remember to update the function... as it is currently written, if i add another contract (with cairo0, for example), support_cairo_version won't return it in the vector, and the compiler won't complain. only the feature contracts test will fail, right?

how about:

pub fn all_contract_versions(&self) -> impl Iterator<Item = Self>

which returns 1, 2 or 3 contracts, depending on which versions are supported?
this will be a much simpler match:

match self {
    TestContract(_) | ..all contracts with all versions supported.. => {
        // create 2 or 3 variants, depending on native feature active or not
    }
    Legacy | ... all contracts with only one version.. => vec![self].into(),
    SpecialNativeContract => RunnableCairo1::iter().map(|v| ..create cairo1 variant..)
}

that way, in the all_contracts iterator can simply do

Self::iter().flat_map(|contract| contract.all_contract_versions())

I don't understand why we need the native contract as well, but I agree that a compilation error when adding a contract is better than failing a test. LMK, if you feel it's too complicated @Yoni-Starkware @dorimedini-starkware
Done.


crates/blockifier/src/test_utils/contracts.rs line 138 at r8 (raw file):

Previously, dorimedini-starkware wrote…

... and then you won't need the get_runnable_version method on the CairoVersion enum

Done.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 15 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils/contracts.rs line 424 at r7 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I don't understand why we need the native contract as well, but I agree that a compilation error when adding a contract is better than failing a test. LMK, if you feel it's too complicated @Yoni-Starkware @dorimedini-starkware
Done.

I think now is better :)


crates/blockifier/src/test_utils/contracts.rs line 438 at r9 (raw file):

                    ret_val.push(native_contract);
                }
                ret_val

how about this? you won't need the assert, and this function can now be called even when self isn't Cairo0

Suggestion:

                #[cfg(not(feature = "cairo_native"))]
                let versions = vec![
                    CairoVersion::Cairo0,
                    CairoVersion::Cairo1(RunnableCairo1::Casm)
                ];
                #[cfg(feature = "cairo_native")]
                let versions = vec![
                    CairoVersion::Cairo0,
                    CairoVersion::Cairo1(RunnableCairo1::Casm),
                    CairoVersion::Cairo1(RunnableCairo1::Native),
                ];
                versions.iter().map(|v| {
                    let versioned_contract = *self;
                    versioned_contract.set_cairo_version(v);
                    versioned_contract
                })

crates/blockifier/src/test_utils/contracts.rs line 452 at r9 (raw file):

                    ret_val.push(native_contract);
                }
                ret_val

see above: conditionally define a versions vector by feature, and then map.
that way you won't need the assert.
consider, also, doing this (only compute the versions vector in the match, and share the map logic):

let versions = match self {
    ...
};
versions.iter().map(|v| {
    let versioned_contract = *self;
    versioned_contract.set_cairo_version(v);
    versioned_contract
})

Code quote:

                // This would only work if the default runnable cairo1 is Casm.
                assert!(self.cairo_version() == CairoVersion::Cairo1(RunnableCairo1::Casm));
                let mut ret_val = vec![*self];
                #[cfg(feature = "cairo_native")]
                {
                    let mut native_contract = *self;
                    native_contract
                        .set_cairo_version(CairoVersion::Cairo1(RunnableCairo1::Native));
                    ret_val.push(native_contract);
                }
                ret_val

@meship-starkware meship-starkware force-pushed the meship/contracts/add_runnable_cairo_version_struct branch from f8fbee1 to cd34971 Compare December 8, 2024 14:28
@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from 60b4d1d to 01e3d80 Compare December 8, 2024 14:28
@meship-starkware meship-starkware force-pushed the meship/contracts/add_runnable_cairo_version_struct branch from cd34971 to 53605f5 Compare December 8, 2024 14:32
@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from 01e3d80 to 9c9c78c Compare December 8, 2024 15:02
Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 39 of 40 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/test_utils/contracts.rs line 438 at r9 (raw file):

Previously, dorimedini-starkware wrote…

how about this? you won't need the assert, and this function can now be called even when self isn't Cairo0

Done.


crates/blockifier/src/test_utils/contracts.rs line 452 at r9 (raw file):

Previously, dorimedini-starkware wrote…

see above: conditionally define a versions vector by feature, and then map.
that way you won't need the assert.
consider, also, doing this (only compute the versions vector in the match, and share the map logic):

let versions = match self {
    ...
};
versions.iter().map(|v| {
    let versioned_contract = *self;
    versioned_contract.set_cairo_version(v);
    versioned_contract
})

I think that the match should return the final vector. I made a helper function that shares the map logic. I can have the match return the version list, but then I would have to separate the only cairo1 from the only cairo0. If you think that is better, then I'll do it.

Base automatically changed from meship/contracts/add_runnable_cairo_version_struct to main December 8, 2024 15:11
@meship-starkware meship-starkware force-pushed the meship/remove_FC_native_folder branch from 9c9c78c to 24398b4 Compare December 8, 2024 15:12
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 38 files at r7, 1 of 16 files at r8, 5 of 15 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

@Yoni-Starkware Yoni-Starkware merged commit ef39c7f into main Dec 9, 2024
14 checks passed
@meship-starkware meship-starkware deleted the meship/remove_FC_native_folder branch December 9, 2024 06:48
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants