chore(blockifier): move contract compilation logic to separate module + contracts.rs

[dorimedini-starkware]

This change is 

[dorimedini-starkware]

• feat(blockifier): recompile Cairo1 in the CI #197
• chore(blockifier): add is_legacy method #196
• chore(blockifier): add text file with current compiler version for CI use #195
• chore(blockifier): verify local cairo repo state before compiling #194
• feat(blockifier): support Cairo1 local recompilation #184
• chore(blockifier): verify cairo compiler repo (with correct tag) #183
• feat(blockifier): verify cairo-lang version before Cairo0 compilation #182
• chore(blockifier): fetch compiler repo version #181
• chore(blockifier): move contract compilation logic to separate module + contracts.rs #180 👈
• chore(blockifier): refactor feature contract path getters for different paths #179
• main

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

Join @dorimedini-starkware and the rest of your teammates on Graphite

[nimrod-starkware]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @TzahiTaub)

---

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

/// Compiles a Cairo0 program using the deprecated compiler.
pub fn cairo0_compile(path: String, extra_arg: Option<String>, debug_info: bool) -> Vec<u8> {
    let mut command = Command::new("starknet-compile-deprecated");

Consider defining it as a constant

Code quote:

"starknet-compile-deprecated"

---

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

                    FeatureContract::ERC20(_) => unreachable!(),
                };
                cairo0_compile(self.get_source_path(), extra_arg, false)

Shouldn't that be configurable by whoever calls compile?

Code quote:

 false

[dorimedini-starkware]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, @nimrod-starkware, and @TzahiTaub)

---

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

Previously, nimrod-starkware wrote…

Consider defining it as a constant

why? this is the only usage of this string

---

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

Previously, nimrod-starkware wrote…

Shouldn't that be configurable by whoever calls compile?

yes, but this is only a preparation; we currently have no use case for the true case (this is not a callable script, it's just part of tests ATM)

[nimrod-starkware]

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @TzahiTaub)

[TzahiTaub]

Reviewed 3 of 4 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, and @meship-starkware)

---

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

        let mut command = Command::new("starknet-compile-deprecated");
        command.args([&path_str, "--no_debug_info"]);
        if file_name.starts_with("account") {

FaultyAccount didn't fulfill this condition in the past, and now it does—is this the desired behavior? (I assume so; I'm just verifying.)

Code quote:

file_name.starts_with("account")

---

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

            panic!(
                "{} does not compile to {existing_compiled_path}.\nRun `{FIX_COMMAND}` to fix the \
                 expected test according to locally installed `starknet-compile-deprecated`.\n",

?

Suggestion:

               existing file according

---

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

        .map(|contract| contract.get_compiled_path())
        .collect();
    let mut compiled_paths_on_filesystem: Vec<String> = verify_and_get_files(CairoVersion::Cairo0)

Do we really need to enforce the dir hierarchy?
Can't we be satisfied with verifying the contracts we get by all_feature_contracts and the source/compiled_path functions exist in the dir, and that's it? (I think it'll be nice if we can remove that function).

Code quote:

verify_and_get_files

[dorimedini-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, and @TzahiTaub)

---

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

Previously, TzahiTaub (Tzahi) wrote…

FaultyAccount didn't fulfill this condition in the past, and now it does—is this the desired behavior? (I assume so; I'm just verifying.)

it should be compiled as an account, yes

---

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

Previously, TzahiTaub (Tzahi) wrote…

Do we really need to enforce the dir hierarchy?
Can't we be satisfied with verifying the contracts we get by all_feature_contracts and the source/compiled_path functions exist in the dir, and that's it? (I think it'll be nice if we can remove that function).

I want to prevent the case where an untracked contract is manually added. it's possible to miss adding it to the enum; why not enforce it?

[TzahiTaub]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)

---

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

Previously, dorimedini-starkware wrote…

I want to prevent the case where an untracked contract is manually added. it's possible to miss adding it to the enum; why not enforce it?

OK, 👍

[dorimedini-starkware]

Merge activity

• Aug 11, 11:28 AM EDT: @dorimedini-starkware started a stack merge that includes this pull request via Graphite.
• Aug 11, 11:31 AM EDT: Graphite rebased this pull request as part of a merge.
• Aug 11, 11:39 AM EDT: @dorimedini-starkware merged this pull request with Graphite.