chore(blockifier): refactor feature contract path getters for different paths

[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

[aner-starkware]

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

// ERC20 contract is in a unique location.
const ERC20_BASE_NAME: &str = "ERC20";

Why are you adding this, if it's never used?

Code quote:

const ERC20_BASE_NAME: &str = "ERC20";

[aner-starkware]

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

            Self::SecurityTests => SECURITY_TEST_CONTRACT_NAME,
            Self::TestContract(_) => TEST_CONTRACT_NAME,
            Self::ERC20(_) => ERC20_BASE_NAME,

The above question

Code quote:

Self::ERC20(_) => ERC20_BASE_NAME,

[aner-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @noaov1)

---

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

        let contract_name = match self {
            // ERC20 is a special case - not in the feature_contracts directory.
            Self::ERC20(cairo_version) => {

This is a bit confusing - you're over

Code quote:

        let cairo_version = self.cairo_version();
        let contract_name = match self {
            // ERC20 is a special case - not in the feature_contracts directory.
            Self::ERC20(cairo_version) => {

[dorimedini-starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

---

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

Previously, aner-starkware wrote…

Why are you adding this, if it's never used?

Done.

---

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

Previously, aner-starkware wrote…

The above question

Done.

---

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

Previously, aner-starkware wrote…

This is a bit confusing - you're over

what am I over?

[aner-starkware]

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

Previously, dorimedini-starkware wrote…

what am I over?

Sorry, I forgot to complete the sentence 😅
You're overriding cairo_version before ever using it. Maybe consider postponing the first let statement, or using a different variable name (e.g., if you want to assert_eq)?

[dorimedini-starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

---

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

Previously, aner-starkware wrote…

Sorry, I forgot to complete the sentence 😅
You're overriding cairo_version before ever using it. Maybe consider postponing the first let statement, or using a different variable name (e.g., if you want to assert_eq)?

Right! Done.

[aner-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @noaov1)

---

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

            let base_name = Path::new(&source_path)
                .file_name()
                .unwrap()

Are we cool with all these unwraps?

Code quote:

.unwrap()

[dorimedini-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

---

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

Previously, aner-starkware wrote…

Are we cool with all these unwraps?

in a test util? I'd say yes, would you prefer some be expects?

[aner-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

in a test util? I'd say yes, would you prefer some be expects?

OK, just checking.

[aner-starkware]

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

[TzahiTaub]

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

---

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

                .unwrap()
                .strip_suffix(".cairo")
                .unwrap();

Your choice, but it seems easier to define a private func for the match mapping and just call it here and in the above, than to do this processing.

Code quote:

            let base_name = Path::new(&source_path)
                .file_name()
                .unwrap()
                .to_str()
                .unwrap()
                .strip_suffix(".cairo")
                .unwrap();

[dorimedini-starkware]

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

---

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

Previously, TzahiTaub (Tzahi) wrote…

Your choice, but it seems easier to define a private func for the match mapping and just call it here and in the above, than to do this processing.

wdym?
not that ERC20_CAIRO0_CONTRACT_SOURCE_PATH and ERC20_CAIRO0_CONTRACT_PATH are not the same

[dorimedini-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

wdym?
not that ERC20_CAIRO0_CONTRACT_SOURCE_PATH and ERC20_CAIRO0_CONTRACT_PATH are not the same

*note that

[TzahiTaub]

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

---

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

Previously, dorimedini-starkware wrote…

*note that

In "in the above" I meant here (in the else clause) and in the else of get_source_path. I.e., something like

fn get_non_erc20_base_filename(&self)) -> String:
      match self {
                    Self::AccountWithLongValidate(_) => ACCOUNT_LONG_VALIDATE_NAME,
                    Self::AccountWithoutValidations(_) => ACCOUNT_WITHOUT_VALIDATIONS_NAME,
                    Self::Empty(_) => EMPTY_CONTRACT_NAME,
                    Self::FaultyAccount(_) => FAULTY_ACCOUNT_NAME,
                    Self::LegacyTestContract => LEGACY_CONTRACT_NAME,
                    Self::SecurityTests => SECURITY_TEST_CONTRACT_NAME,
                    Self::TestContract(_) => TEST_CONTRACT_NAME,
                    Self::ERC20(_) => unreachable!(),
                }
 pub fn get_source_path(&self) -> String {
...
    else {
        let base_name = self.get_non_erc20_base_filename()
...
 pub fn get_compiled_path(&self) -> String {
...
    else {
        let base_name = self.get_non_erc20_base_filename()

[TzahiTaub]

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

[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:29 AM EDT: @dorimedini-starkware merged this pull request with Graphite.