From 3c6d2461081519dc94a19901eea8c54d354b0307 Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Mon, 6 May 2024 13:49:55 +0300 Subject: [PATCH 1/3] test: test feature contract files structure and consistency with rust Signed-off-by: Dori Medini --- crates/blockifier/src/test_utils.rs | 7 ++ crates/blockifier/src/test_utils/contracts.rs | 33 +++++- .../feature_contracts_compatibility_test.rs | 101 ++++++++++++------ 3 files changed, 108 insertions(+), 33 deletions(-) diff --git a/crates/blockifier/src/test_utils.rs b/crates/blockifier/src/test_utils.rs index 35b7de8876..58be205ff0 100644 --- a/crates/blockifier/src/test_utils.rs +++ b/crates/blockifier/src/test_utils.rs @@ -71,6 +71,13 @@ impl CairoVersion { _ => panic!("Transaction version {:?} is not supported.", tx_version), } } + + pub fn other(&self) -> Self { + match self { + Self::Cairo0 => Self::Cairo1, + Self::Cairo1 => Self::Cairo0, + } + } } // Storage keys. diff --git a/crates/blockifier/src/test_utils/contracts.rs b/crates/blockifier/src/test_utils/contracts.rs index 5b584b7175..0c8fac8072 100644 --- a/crates/blockifier/src/test_utils/contracts.rs +++ b/crates/blockifier/src/test_utils/contracts.rs @@ -2,6 +2,8 @@ use starknet_api::core::{ClassHash, ContractAddress, PatriciaKey}; use starknet_api::deprecated_contract_class::ContractClass as DeprecatedContractClass; use starknet_api::hash::StarkHash; use starknet_api::{class_hash, contract_address, patricia_key}; +use strum::IntoEnumIterator; +use strum_macros::EnumIter; use crate::execution::contract_class::{ContractClass, ContractClassV0, ContractClassV1}; use crate::test_utils::{get_raw_contract_class, CairoVersion}; @@ -54,7 +56,7 @@ const ERC20_CONTRACT_PATH: &str = /// Enum representing all feature contracts. /// The contracts that are implemented in both Cairo versions include a version field. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, EnumIter)] pub enum FeatureContract { AccountWithLongValidate(CairoVersion), AccountWithoutValidations(CairoVersion), @@ -79,6 +81,17 @@ impl FeatureContract { } } + fn has_two_versions(&self) -> bool { + match self { + Self::AccountWithLongValidate(_) + | Self::AccountWithoutValidations(_) + | Self::Empty(_) + | Self::FaultyAccount(_) + | Self::TestContract(_) => true, + Self::SecurityTests | Self::ERC20 | Self::LegacyTestContract => false, + } + } + fn get_cairo_version_bit(&self) -> u32 { match self.cairo_version() { CairoVersion::Cairo0 => 0, @@ -101,7 +114,7 @@ impl FeatureContract { } } - fn get_compiled_path(&self) -> String { + pub fn get_compiled_path(&self) -> String { let cairo_version = self.cairo_version(); let contract_name = match self { Self::AccountWithLongValidate(_) => ACCOUNT_LONG_VALIDATE_NAME, @@ -115,7 +128,7 @@ impl FeatureContract { Self::ERC20 => return ERC20_CONTRACT_PATH.into(), }; format!( - "./feature_contracts/cairo{}/compiled/{}{}.json", + "feature_contracts/cairo{}/compiled/{}{}.json", match cairo_version { CairoVersion::Cairo0 => "0", CairoVersion::Cairo1 => "1", @@ -176,4 +189,18 @@ impl FeatureContract { pub fn get_raw_class(&self) -> String { get_raw_contract_class(&self.get_compiled_path()) } + + pub fn all_contracts() -> impl Iterator { + // EnumIter iterates over all variants with Default::default() as the cairo + // version. + Self::iter().flat_map(|contract| { + if contract.has_two_versions() { + let mut other_contract = contract; + other_contract.set_cairo_version(contract.cairo_version().other()); + vec![contract, other_contract].into_iter() + } else { + vec![contract].into_iter() + } + }) + } } diff --git a/crates/blockifier/tests/feature_contracts_compatibility_test.rs b/crates/blockifier/tests/feature_contracts_compatibility_test.rs index 34d77a229b..c9caf8d0cf 100644 --- a/crates/blockifier/tests/feature_contracts_compatibility_test.rs +++ b/crates/blockifier/tests/feature_contracts_compatibility_test.rs @@ -1,7 +1,12 @@ use std::fs; use std::process::Command; -const FEATURE_CONTRACTS_DIR: &str = "feature_contracts/cairo0"; +use blockifier::test_utils::contracts::FeatureContract; +use blockifier::test_utils::CairoVersion; +use pretty_assertions::assert_eq; + +const CAIRO0_FEATURE_CONTRACTS_DIR: &str = "feature_contracts/cairo0"; +const CAIRO1_FEATURE_CONTRACTS_DIR: &str = "feature_contracts/cairo1"; const COMPILED_CONTRACTS_SUBDIR: &str = "compiled"; const FIX_COMMAND: &str = "FIX_FEATURE_TEST=1 cargo test -- --ignored"; @@ -35,35 +40,9 @@ const FIX_COMMAND: &str = "FIX_FEATURE_TEST=1 cargo test -- --ignored"; // `COMPILED_CONTRACTS_SUBDIR`. // 2. for each `X.cairo` file in `TEST_CONTRACTS` there exists an `X_compiled.json` file in // `COMPILED_CONTRACTS_SUBDIR` which equals `starknet-compile-deprecated X.cairo --no_debug_info`. -fn verify_feature_contracts_compatibility(fix: bool) { - for file in fs::read_dir(FEATURE_CONTRACTS_DIR).unwrap() { - let path = file.unwrap().path(); - - // Test `TEST_CONTRACTS` file and directory structure. - if !path.is_file() { - if let Some(dir_name) = path.file_name() { - assert_eq!( - dir_name, - COMPILED_CONTRACTS_SUBDIR, - "Found directory '{}' in `{FEATURE_CONTRACTS_DIR}`, which should contain only \ - the `{COMPILED_CONTRACTS_SUBDIR}` directory.", - dir_name.to_string_lossy() - ); - continue; - } - } - let path_str = path.to_string_lossy(); - assert_eq!( - path.extension().unwrap(), - "cairo", - "Found a non-Cairo file '{path_str}' in `{FEATURE_CONTRACTS_DIR}`" - ); - +fn verify_feature_contracts_compatibility(fix: bool, cairo_version: CairoVersion) { + for (path_str, file_name, existing_compiled_path) in verify_and_get_files(cairo_version) { // Compare output of cairo-file on file with existing compiled file. - let file_name = path.file_stem().unwrap().to_string_lossy(); - let existing_compiled_path = format!( - "{FEATURE_CONTRACTS_DIR}/{COMPILED_CONTRACTS_SUBDIR}/{file_name}_compiled.json" - ); let mut command = Command::new("starknet-compile-deprecated"); command.args([&path_str, "--no_debug_info"]); if file_name.starts_with("account") { @@ -93,9 +72,71 @@ fn verify_feature_contracts_compatibility(fix: bool) { } } +/// Verifies that the feature contracts directory contains the expected contents, and returns a list +/// of pairs (source_path, base_filename, compiled_path) for each contract. +fn verify_and_get_files(cairo_version: CairoVersion) -> Vec<(String, String, String)> { + let mut paths = vec![]; + let directory = match cairo_version { + CairoVersion::Cairo0 => CAIRO0_FEATURE_CONTRACTS_DIR, + CairoVersion::Cairo1 => CAIRO1_FEATURE_CONTRACTS_DIR, + }; + let compiled_extension = match cairo_version { + CairoVersion::Cairo0 => "_compiled.json", + CairoVersion::Cairo1 => ".casm.json", + }; + for file in fs::read_dir(directory).unwrap() { + let path = file.unwrap().path(); + + // Verify `TEST_CONTRACTS` file and directory structure. + if !path.is_file() { + if let Some(dir_name) = path.file_name() { + assert_eq!( + dir_name, + COMPILED_CONTRACTS_SUBDIR, + "Found directory '{}' in `{directory}`, which should contain only the \ + `{COMPILED_CONTRACTS_SUBDIR}` directory.", + dir_name.to_string_lossy() + ); + continue; + } + } + let path_str = path.to_string_lossy(); + assert_eq!( + path.extension().unwrap(), + "cairo", + "Found a non-Cairo file '{path_str}' in `{directory}`" + ); + + let file_name = path.file_stem().unwrap().to_string_lossy(); + let existing_compiled_path = + format!("{directory}/{COMPILED_CONTRACTS_SUBDIR}/{file_name}{compiled_extension}"); + + paths.push((path_str.to_string(), file_name.to_string(), existing_compiled_path)); + } + + paths +} + +#[test] +fn verify_feature_contracts_match_enum() { + let mut compiled_paths_from_enum: Vec = FeatureContract::all_contracts() + // ERC20 is a special case - not in the feature_contracts directory. + .filter(|contract| !matches!(contract, FeatureContract::ERC20)) + .map(|contract| contract.get_compiled_path()) + .collect(); + let mut compiled_paths_on_filesystem: Vec = verify_and_get_files(CairoVersion::Cairo0) + .into_iter() + .chain(verify_and_get_files(CairoVersion::Cairo1)) + .map(|(_, _, compiled_path)| compiled_path) + .collect(); + compiled_paths_from_enum.sort(); + compiled_paths_on_filesystem.sort(); + assert_eq!(compiled_paths_from_enum, compiled_paths_on_filesystem); +} + #[test] #[ignore] fn verify_feature_contracts() { let fix_features = std::env::var("FIX_FEATURE_TEST").is_ok(); - verify_feature_contracts_compatibility(fix_features) + verify_feature_contracts_compatibility(fix_features, CairoVersion::Cairo0) } From 7fba16f3987f0acdb29389567b0724652ee698e5 Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Mon, 6 May 2024 15:30:42 +0300 Subject: [PATCH 2/3] chore: move compilation logic to FeatureContracts enum Signed-off-by: Dori Medini --- crates/blockifier/src/test_utils.rs | 3 +- .../src/test_utils/cairo_compile.rs | 21 +++++++ crates/blockifier/src/test_utils/contracts.rs | 63 +++++++++++++++++-- .../feature_contracts_compatibility_test.rs | 29 +++------ 4 files changed, 91 insertions(+), 25 deletions(-) create mode 100644 crates/blockifier/src/test_utils/cairo_compile.rs diff --git a/crates/blockifier/src/test_utils.rs b/crates/blockifier/src/test_utils.rs index 58be205ff0..30589146f9 100644 --- a/crates/blockifier/src/test_utils.rs +++ b/crates/blockifier/src/test_utils.rs @@ -1,3 +1,4 @@ +pub mod cairo_compile; pub mod contracts; pub mod declare; pub mod deploy_account; @@ -49,7 +50,7 @@ pub const TEST_ERC20_CONTRACT_CLASS_HASH: &str = "0x1010"; pub const ERC20_CONTRACT_PATH: &str = "./ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls_compiled.json"; -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum CairoVersion { Cairo0, Cairo1, diff --git a/crates/blockifier/src/test_utils/cairo_compile.rs b/crates/blockifier/src/test_utils/cairo_compile.rs new file mode 100644 index 0000000000..ad9568da37 --- /dev/null +++ b/crates/blockifier/src/test_utils/cairo_compile.rs @@ -0,0 +1,21 @@ +use std::process::Command; + +/// Compiles a Cairo0 program using the deprecated compiler. +pub fn cairo0_compile(path: String, extra_arg: Option, debug_info: bool) -> Vec { + let mut command = Command::new("starknet-compile-deprecated"); + if let Some(extra_arg) = extra_arg { + command.arg(extra_arg); + } + if !debug_info { + command.args([&path, "--no_debug_info"]); + } + let compile_output = command.output().unwrap(); + let stderr_output = String::from_utf8(compile_output.stderr).unwrap(); + assert!(compile_output.status.success(), "{stderr_output}"); + compile_output.stdout +} + +/// Compiles a Cairo1 program using the compiler version set in the Cargo.toml. +pub fn cairo1_compile(_path: String) -> Vec { + todo!(); +} diff --git a/crates/blockifier/src/test_utils/contracts.rs b/crates/blockifier/src/test_utils/contracts.rs index 0c8fac8072..12f6c05ead 100644 --- a/crates/blockifier/src/test_utils/contracts.rs +++ b/crates/blockifier/src/test_utils/contracts.rs @@ -6,6 +6,7 @@ use strum::IntoEnumIterator; use strum_macros::EnumIter; use crate::execution::contract_class::{ContractClass, ContractClassV0, ContractClassV1}; +use crate::test_utils::cairo_compile::{cairo0_compile, cairo1_compile}; use crate::test_utils::{get_raw_contract_class, CairoVersion}; // This file contains featured contracts, used for tests. Use the function 'test_state' in @@ -51,8 +52,10 @@ const SECURITY_TEST_CONTRACT_NAME: &str = "security_tests_contract"; const TEST_CONTRACT_NAME: &str = "test_contract"; // ERC20 contract is in a unique location. +const ERC20_BASE_NAME: &str = "ERC20"; const ERC20_CONTRACT_PATH: &str = "./ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls_compiled.json"; +const ERC20_CONTRACT_SOURCE_PATH: &str = "./ERC20_without_some_syscalls/ERC20/ERC20.cairo"; /// Enum representing all feature contracts. /// The contracts that are implemented in both Cairo versions include a version field. @@ -69,7 +72,7 @@ pub enum FeatureContract { } impl FeatureContract { - fn cairo_version(&self) -> CairoVersion { + pub fn cairo_version(&self) -> CairoVersion { match self { Self::AccountWithLongValidate(version) | Self::AccountWithoutValidations(version) @@ -114,9 +117,8 @@ impl FeatureContract { } } - pub fn get_compiled_path(&self) -> String { - let cairo_version = self.cairo_version(); - let contract_name = match self { + fn contract_base_name(&self) -> &str { + match self { Self::AccountWithLongValidate(_) => ACCOUNT_LONG_VALIDATE_NAME, Self::AccountWithoutValidations(_) => ACCOUNT_WITHOUT_VALIDATIONS_NAME, Self::Empty(_) => EMPTY_CONTRACT_NAME, @@ -124,8 +126,31 @@ impl FeatureContract { Self::LegacyTestContract => LEGACY_CONTRACT_NAME, Self::SecurityTests => SECURITY_TEST_CONTRACT_NAME, Self::TestContract(_) => TEST_CONTRACT_NAME, + Self::ERC20 => ERC20_BASE_NAME, + } + } + + pub fn get_source_path(&self) -> String { + match self { + // Special case: ERC20 contract in a different location. + Self::ERC20 => ERC20_CONTRACT_SOURCE_PATH.into(), + _not_erc20 => format!( + "feature_contracts/cairo{}/{}.cairo", + match self.cairo_version() { + CairoVersion::Cairo0 => "0", + CairoVersion::Cairo1 => "1", + }, + self.contract_base_name() + ), + } + } + + pub fn get_compiled_path(&self) -> String { + let cairo_version = self.cairo_version(); + let contract_name = match self { // ERC20 is a special case - not in the feature_contracts directory. Self::ERC20 => return ERC20_CONTRACT_PATH.into(), + _not_erc20 => self.contract_base_name(), }; format!( "feature_contracts/cairo{}/compiled/{}{}.json", @@ -141,6 +166,31 @@ impl FeatureContract { ) } + /// Compiles the feature contract and returns the compiled contract as a byte vector. + /// Panics if the contract is ERC20, as ERC20 contract recompilation is not supported. + pub fn compile(&self) -> Vec { + if matches!(self, Self::ERC20) { + panic!("ERC20 contract recompilation not supported."); + } + match self.cairo_version() { + CairoVersion::Cairo0 => { + let extra_arg: Option = match self { + // Account contracts require the account_contract flag. + FeatureContract::AccountWithLongValidate(_) + | FeatureContract::AccountWithoutValidations(_) + | FeatureContract::FaultyAccount(_) => Some("--account_contract".into()), + FeatureContract::SecurityTests => Some("--disable_hint_validation".into()), + FeatureContract::Empty(_) + | FeatureContract::TestContract(_) + | FeatureContract::LegacyTestContract => None, + FeatureContract::ERC20 => unreachable!(), + }; + cairo0_compile(self.get_source_path(), extra_arg, false) + } + CairoVersion::Cairo1 => cairo1_compile(self.get_source_path()), + } + } + pub fn set_cairo_version(&mut self, version: CairoVersion) { match self { Self::AccountWithLongValidate(v) @@ -203,4 +253,9 @@ impl FeatureContract { } }) } + + pub fn all_feature_contracts() -> impl Iterator { + // ERC20 is a special case - not in the feature_contracts directory. + Self::all_contracts().filter(|contract| !matches!(contract, Self::ERC20)) + } } diff --git a/crates/blockifier/tests/feature_contracts_compatibility_test.rs b/crates/blockifier/tests/feature_contracts_compatibility_test.rs index c9caf8d0cf..f72c8c0bc6 100644 --- a/crates/blockifier/tests/feature_contracts_compatibility_test.rs +++ b/crates/blockifier/tests/feature_contracts_compatibility_test.rs @@ -1,5 +1,4 @@ use std::fs; -use std::process::Command; use blockifier::test_utils::contracts::FeatureContract; use blockifier::test_utils::CairoVersion; @@ -41,20 +40,12 @@ const FIX_COMMAND: &str = "FIX_FEATURE_TEST=1 cargo test -- --ignored"; // 2. for each `X.cairo` file in `TEST_CONTRACTS` there exists an `X_compiled.json` file in // `COMPILED_CONTRACTS_SUBDIR` which equals `starknet-compile-deprecated X.cairo --no_debug_info`. fn verify_feature_contracts_compatibility(fix: bool, cairo_version: CairoVersion) { - for (path_str, file_name, existing_compiled_path) in verify_and_get_files(cairo_version) { + for contract in FeatureContract::all_feature_contracts() + .filter(|contract| contract.cairo_version() == cairo_version) + { // Compare output of cairo-file on file with existing compiled file. - let mut command = Command::new("starknet-compile-deprecated"); - command.args([&path_str, "--no_debug_info"]); - if file_name.starts_with("account") { - command.arg("--account_contract"); - } - if file_name.starts_with("security") { - command.arg("--disable_hint_validation"); - } - let compile_output = command.output().unwrap(); - let stderr_output = String::from_utf8(compile_output.stderr).unwrap(); - assert!(compile_output.status.success(), "{stderr_output}"); - let expected_compiled_output = compile_output.stdout; + let expected_compiled_output = contract.compile(); + let existing_compiled_path = contract.get_compiled_path(); if fix { fs::write(&existing_compiled_path, &expected_compiled_output).unwrap(); @@ -64,9 +55,9 @@ fn verify_feature_contracts_compatibility(fix: bool, cairo_version: CairoVersion if String::from_utf8(expected_compiled_output).unwrap() != existing_compiled_contents { panic!( - "{path_str} does not compile to {existing_compiled_path}.\nRun `{FIX_COMMAND}` to \ - fix the expected test according to locally installed \ - `starknet-compile-deprecated`.\n" + "{} does not compile to {existing_compiled_path}.\nRun `{FIX_COMMAND}` to fix the \ + expected test according to locally installed `starknet-compile-deprecated`.\n", + contract.get_source_path() ); } } @@ -119,9 +110,7 @@ fn verify_and_get_files(cairo_version: CairoVersion) -> Vec<(String, String, Str #[test] fn verify_feature_contracts_match_enum() { - let mut compiled_paths_from_enum: Vec = FeatureContract::all_contracts() - // ERC20 is a special case - not in the feature_contracts directory. - .filter(|contract| !matches!(contract, FeatureContract::ERC20)) + let mut compiled_paths_from_enum: Vec = FeatureContract::all_feature_contracts() .map(|contract| contract.get_compiled_path()) .collect(); let mut compiled_paths_on_filesystem: Vec = verify_and_get_files(CairoVersion::Cairo0) From aed63a4268aab482e083da330ae745b6d65f649c Mon Sep 17 00:00:00 2001 From: Dori Medini Date: Mon, 6 May 2024 17:17:26 +0300 Subject: [PATCH 3/3] chore: fetch Cairo1 compiler version from Cargo.toml Signed-off-by: Dori Medini --- Cargo.lock | 1 + Cargo.toml | 1 + crates/blockifier/Cargo.toml | 1 + .../src/test_utils/cairo_compile.rs | 49 +++++++++++++++++++ 4 files changed, 52 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 79aae00c76..6f4edbd788 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -436,6 +436,7 @@ dependencies = [ "strum_macros 0.24.3", "test-case", "thiserror", + "toml", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 217ddbdcae..0fece228b8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -55,6 +55,7 @@ strum = "0.24.1" strum_macros = "0.24.3" tempfile = "3.7.0" test-case = "2.2.2" +toml = "0.8" thiserror = "1.0.37" [workspace.lints.rust] diff --git a/crates/blockifier/Cargo.toml b/crates/blockifier/Cargo.toml index e63f33baef..8140763d96 100644 --- a/crates/blockifier/Cargo.toml +++ b/crates/blockifier/Cargo.toml @@ -48,6 +48,7 @@ starknet_api = { workspace = true, features = ["testing"] } strum.workspace = true strum_macros.workspace = true thiserror.workspace = true +toml.workspace = true [dev-dependencies] assert_matches.workspace = true diff --git a/crates/blockifier/src/test_utils/cairo_compile.rs b/crates/blockifier/src/test_utils/cairo_compile.rs index ad9568da37..74a62b4a64 100644 --- a/crates/blockifier/src/test_utils/cairo_compile.rs +++ b/crates/blockifier/src/test_utils/cairo_compile.rs @@ -1,5 +1,54 @@ use std::process::Command; +use cached::proc_macro::cached; +use serde::{Deserialize, Serialize}; + +/// Objects for simple deserialization of Cargo.toml to fetch the Cairo1 compiler version. +/// The compiler itself isn't actually a dependency, so we compile by using the version of the +/// cairo-lang-casm crate. +/// The choice of cairo-lang-casm is arbitrary, as all compiler crate dependencies should have the +/// same version. +/// Deserializes: +/// """ +/// ... +/// [workspace.dependencies] +/// ... +/// cairo-lang-casm = VERSION +/// ... +/// """ +/// where `VERSION` can be a simple "x.y.z" version string or an object with a "version" field. +#[derive(Debug, Serialize, Deserialize)] +#[serde(untagged)] +enum DependencyValue { + String(String), + Object { version: String }, +} + +#[derive(Debug, Serialize, Deserialize)] +struct CairoLangCasmDependency { + #[serde(rename = "cairo-lang-casm")] + cairo_lang_casm: DependencyValue, +} + +#[derive(Debug, Serialize, Deserialize)] +struct WorkspaceFields { + dependencies: CairoLangCasmDependency, +} + +#[derive(Debug, Serialize, Deserialize)] +struct CargoToml { + workspace: WorkspaceFields, +} + +#[cached] +/// Returns the version of the Cairo1 compiler* defined in the root Cargo.toml. +pub fn cairo1_compiler_version() -> String { + let cargo_toml: CargoToml = toml::from_str(include_str!("../../../../Cargo.toml")).unwrap(); + match cargo_toml.workspace.dependencies.cairo_lang_casm { + DependencyValue::String(version) | DependencyValue::Object { version } => version.clone(), + } +} + /// Compiles a Cairo0 program using the deprecated compiler. pub fn cairo0_compile(path: String, extra_arg: Option, debug_info: bool) -> Vec { let mut command = Command::new("starknet-compile-deprecated");