refactor(blockifier): split feature contracts to groups based on their…

[meship-starkware]

… tags

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.

Project coverage is 71.34%. Comparing base (e3165c4) to head (c82646b).
Report is 690 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/test_utils/cairo_compile.rs	0.00%	10 Missing ⚠️
crates/blockifier/src/test_utils/contracts.rs	0.00%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2236       +/-   ##
===========================================
+ Coverage   40.10%   71.34%   +31.24%     
===========================================
  Files          26      102       +76     
  Lines        1895    13636    +11741     
  Branches     1895    13636    +11741     
===========================================
+ Hits          760     9729     +8969     
- Misses       1100     3493     +2393     
- Partials       35      414      +379     

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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-26T08:57:43Z
• Started at 2024-11-28T17:02:43Z
• Started at 2024-12-01T06:12:47Z
• Started at 2024-12-01T12:24:02Z
• Started at 2024-12-02T07:28:56Z
• Started at 2024-12-02T14:04:31Z
• Started at 2024-12-02T14:06:39Z
• Started at 2024-12-02T14:08:07Z
• Started at 2024-12-04T07:49:21Z

[dorimedini-starkware]

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

---

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 46 at r1 (raw file):

            prepare_group_tag_compiler_deps(tag_override.clone());
        }
        for contract in feature_contracts

this loop will be parallelized?

Code quote:

for contract in feature_contracts

---

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

        &self,
        tag_override: Option<String>,
        cargo_nightly_arg: Option<String>,

this pair appears often in this PR; please create a TagAndToolchain struct containing these, and use it

1. in function signatures,
2. as function return values (where relevant),
3. as the key type on your TagToContractsMapping type

Code quote:

        tag_override: Option<String>,
        cargo_nightly_arg: Option<String>,

---

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

        tag_override: Option<String>,
        cargo_nightly_arg: Option<String>,
    ) -> Vec<u8> {

why do you inject this input now?
tag + nightly arg is still a property of the enum variant; you can still use the fixed_tag_and_rust_toolchain to get it.
you can also use fixed_tag_and_rust_toolchain in the implementation of feature_contracts_by_tag

Code quote:

    pub fn compile(
        &self,
        tag_override: Option<String>,
        cargo_nightly_arg: Option<String>,
    ) -> Vec<u8> {

---

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

            }
            let tag_and_version = contract.fixed_tag_and_rust_toolchain();
            let contracts_to_insert = if contract.has_two_versions() {

what does this function mean, now that there are 3 possible versions (with native)?
this function should indicate cairo1 vs cairo0, and your parallelization feature is for cairo1 only

Code quote:

 if contract.has_two_versions()

---

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

            let contracts_to_insert = if contract.has_two_versions() {
                let mut other_contract = contract;
                other_contract.set_cairo_version(contract.cairo_version().other());

how is this defined...?

Code quote:

cairo_version().other()

---

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

                .entry(tag_and_version)
                .or_insert_with(Vec::new)
                .extend(contracts_to_insert);

why add cairo0 contracts to this list...?

Code quote:

.extend(contracts_to_insert);

---

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

}

fn prepare_cairo1_compiler_deps(git_tag_override: Option<String>) {

this function makes no changes to the state of the machine it's running on, so I would rename

Suggestion:

fn verify_cairo1_compiler_deps

[meship-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

what does this function mean, now that there are 3 possible versions (with native)?
this function should indicate cairo1 vs cairo0, and your parallelization feature is for cairo1 only

I thought about having two Types: one Cairo version and the second completion type (the names are not set). I also thought about addressing it in a different PR, where I will remove the Cairo Native folder.
https://reviewable.io/reviews/starkware-libs/sequencer/1301#-O93wpyO6HNmUICVLokU

---

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 46 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this loop will be parallelized?

That is the intention. For now, our priority is to save the Sierra files and remove the native folder, so I don't know when it will happen.

---

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

    }

    pub fn feature_contracts_by_tag() -> TagToContractsMapping {

I decided to create a new function because all_contracts returns an iterator of all the contracts, and then all feature contracts filter the erc_20 contract, so I didn't know if I could remove the ERC20 in all contracts. Also, there is another use that needs the iterator form

Code quote:

 fn feature_contracts_by_tag() -> TagToContractsMapping {

[meship-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

this function makes no changes to the state of the machine it's running on, so I would rename

Done.

---

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

Previously, dorimedini-starkware wrote…

this pair appears often in this PR; please create a TagAndToolchain struct containing these, and use it

1. in function signatures,
2. as function return values (where relevant),
3. as the key type on your TagToContractsMapping type

Done.

---

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

Previously, dorimedini-starkware wrote…

why do you inject this input now?
tag + nightly arg is still a property of the enum variant; you can still use the fixed_tag_and_rust_toolchain to get it.
you can also use fixed_tag_and_rust_toolchain in the implementation of feature_contracts_by_tag

Done.

---

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

Previously, dorimedini-starkware wrote…

why add cairo0 contracts to this list...?

Done.

---

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

            }
        }
        _ => {

I don't like the implicit check here, but doing it explicitly will force me to duplicate the cairo1 behavior because we have the feature flag. It won't be necessary with Doris' changes, so maybe I can keep it and add a TODO. WDYT?

[dorimedini-starkware]

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

---

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

Previously, meship-starkware (Meshi Peled) wrote…

I don't like the implicit check here, but doing it explicitly will force me to duplicate the cairo1 behavior because we have the feature flag. It won't be necessary with Doris' changes, so maybe I can keep it and add a TODO. WDYT?

keep it and add a TODO sgtm :)

---

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

    pub fn cairo1_feature_contracts_by_tag() -> TagToContractsMapping {
        // EnumIter iterates over all variants with Default::default() as the cairo
        // version.

delete, not relevant here (you are not using Self::iter())

Code quote:

        // EnumIter iterates over all variants with Default::default() as the cairo
        // version.

[meship-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

delete, not relevant here (you are not using Self::iter())

Done.

[dorimedini-starkware]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)