From 8118faa3cda1dd6a94dbab8d62d2520069783213 Mon Sep 17 00:00:00 2001 From: Elias Tazartes <66871571+Eikix@users.noreply.github.com> Date: Tue, 19 Sep 2023 16:57:04 +0700 Subject: [PATCH] refactor: refactor according to clippy pedantic recommendation and add more documentation (#82) * refactor: slight refactor * docs: update readme * chore: run and apply when relevant * fix: fix clippy * feat: adding context for remote vs local sha comparison * fix: fix makefile * docs: refactor slightly and write docs for fetch_dump_katana * docs: acknowledge reth in readme * fix: fix clippy * fix: fix comments * fix: fix write test state argument type --- Cargo.toml | 9 +++-- Makefile | 2 +- README.md | 11 ++++++ crates/ef-testing/Cargo.toml | 12 +++--- .../ef-testing/src/bin/fetch_dump_katana.rs | 39 ++++++++----------- ...t.rs => fetch_kakarot_submodule_commit.rs} | 17 ++++---- crates/ef-testing/src/models/case.rs | 24 +++++++----- crates/ef-testing/src/models/error.rs | 2 +- crates/ef-testing/src/models/result.rs | 6 +-- crates/ef-testing/src/models/suite.rs | 3 +- crates/ef-testing/src/storage/mod.rs | 4 +- crates/ef-testing/src/storage/models.rs | 3 +- crates/ef-testing/src/traits/mod.rs | 6 +-- crates/ef-testing/src/utils/assert.rs | 6 +-- crates/ef-testing/src/utils/io.rs | 4 +- crates/ef-testing/tests/tests.rs | 5 ++- 16 files changed, 83 insertions(+), 70 deletions(-) rename crates/ef-testing/src/bin/{fetch_commit_kakarot.rs => fetch_kakarot_submodule_commit.rs} (55%) diff --git a/Cargo.toml b/Cargo.toml index 5a4dbb49..1a6b1725 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,7 @@ authors = [ "Johann Bestowrous <@jobez>", "Harsh Bajpai <@bajpai244>", "Danilo Kim <@danilowhk>", + "Fred Tupas <@ftupas>", ] description = "EF standard testing for Kakarot" homepage = "https://github.com/kkrt-labs" @@ -21,7 +22,9 @@ license = "MIT" [workspace.dependencies] # Eth deps -ef-tests = { git = "https://github.com/paradigmxyz/reth.git", tag = "v0.1.0-alpha.7", features = ["ef-tests"] } +ef-tests = { git = "https://github.com/paradigmxyz/reth.git", tag = "v0.1.0-alpha.7", features = [ + "ef-tests", +] } reth-primitives = { git = "https://github.com/paradigmxyz/reth.git", tag = "v0.1.0-alpha.7" } revm-primitives = "1.1" reth-rlp = { git = "https://github.com/paradigmxyz/reth.git", tag = "v0.1.0-alpha.7" } @@ -40,12 +43,12 @@ starknet_api = { git = "https://github.com/starkware-libs/starknet-api", rev = " # Other async-trait = "0.1.58" bytes = "1" -chrono = { version = "0.4.26", features = ["serde"]} +chrono = { version = "0.4.26", features = ["serde"] } ctor = "0.2.4" dotenv = "0.15.0" eyre = "0.6.8" regex = "1.9.3" -reqwest = { version = "0.11.20", features = ["gzip"]} +reqwest = { version = "0.11.20", features = ["gzip"] } rstest = "0.18.1" thiserror = "1.0.47" tokio = { version = "1.21.2", features = ["macros"] } diff --git a/Makefile b/Makefile index db5d29bc..43d7f2f9 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ fetch-dump: cargo run --features dump --bin fetch-dump-katana $(KAKAROT_COMMIT): - cargo run --features fetch-commit --bin fetch-commit-kakarot + cargo run --features fetch-commit --bin fetch-kakarot-submodule-commit # Runs the Ethereum Foundation tests ef-tests: $(KAKAROT_COMMIT) diff --git a/README.md b/README.md index b436892f..7419e090 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,13 @@ This repository contains the execution of the EF standard execution layer tests. +## Requirements + +- nextest: to install [nextest](https://nexte.st/index.html), run `cargo install cargo-nextest --locked` +- A GitHub token in your `.env` file: + - Copy the `.env.example` file to a `.env` file + - Create a [GitHub token](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens) and add it inside the `.env` file. + ## Setup In order to set up the repo and start the testing, please follow the below @@ -15,3 +22,7 @@ instructions: To run the whole test suite, execute `make ef-tests` To run a specific test or list of tests, execute `make target=regular_expression ef-test` where regular_expression allows you to filter on the specific tests you want to run. + +## Acknowledgement + +This repository is heavily inspired by , it uses some code snippets from the Reth codebase and when possible, imports modules and helpers from it. diff --git a/crates/ef-testing/Cargo.toml b/crates/ef-testing/Cargo.toml index 4ebf0b0d..21c02c1c 100644 --- a/crates/ef-testing/Cargo.toml +++ b/crates/ef-testing/Cargo.toml @@ -36,13 +36,13 @@ eyre = { workspace = true } thiserror = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } -regex = { workspace= true } -reqwest = { workspace= true, optional = true } -rstest = { workspace= true } +regex = { workspace = true } +reqwest = { workspace = true, optional = true } +rstest = { workspace = true } tokio = { workspace = true } tracing = "0.1.37" walkdir = { workspace = true } -zip = { workspace= true, optional = true } +zip = { workspace = true, optional = true } [dev-dependencies] tracing-subscriber = "0.3.17" @@ -58,6 +58,6 @@ path = "src/bin/fetch_dump_katana.rs" required-features = ["dump"] [[bin]] -name = "fetch-commit-kakarot" -path = "src/bin/fetch_commit_kakarot.rs" +name = "fetch-kakarot-submodule-commit" +path = "src/bin/fetch_kakarot_submodule_commit.rs" required-features = ["fetch-commit"] diff --git a/crates/ef-testing/src/bin/fetch_dump_katana.rs b/crates/ef-testing/src/bin/fetch_dump_katana.rs index cc34ef6e..985b1252 100644 --- a/crates/ef-testing/src/bin/fetch_dump_katana.rs +++ b/crates/ef-testing/src/bin/fetch_dump_katana.rs @@ -19,39 +19,33 @@ struct WorkflowRun { head_branch: String, } +/// Fetches the Katana dump from Kakarot-RPC artifacts +/// On every PR and merge, thanks to a CI job in Kakarot-RPC, the state of a Starknet devnet +/// with all of Kakarot Cairo smart contracts deployed is dumped in an artifact called 'dump-katana' +/// Starting a Starknet local chain from checkpoint speeds up all our processes. fn main() -> Result<(), Box> { dotenv().ok(); let token = std::env::var("GITHUB_TOKEN").map_err(|_| eyre::eyre!("Missing GITHUB_TOKEN in .env"))?; - let url = "https://api.github.com/repos/sayajin-labs/kakarot-rpc/actions/artifacts"; + let url = "https://api.github.com/repos/kkrt-labs/kakarot-rpc/actions/artifacts"; - let client = reqwest::blocking::Client::builder(); - let response: serde_json::Value = client + let client = reqwest::blocking::Client::builder() .user_agent("reqwest-rust") - .build()? - .get(url) - .send()? - .json()?; + .build()?; + let response: serde_json::Value = client.get(url).send()?.json()?; - // Filter the artifacts to only include the ones we care about - // and sort by updated_at + // Filter the artifacts to only include dump-katana artifacts + // and find the latest one by key `updated_at` let artifacts: Vec = serde_json::from_value(response["artifacts"].clone())?; - let mut artifacts = artifacts + let latest_artifact = artifacts .into_iter() .filter(|artifact| { artifact.name == "dump-katana" && artifact.workflow_run.head_branch == "main" }) - .collect::>(); - artifacts.sort_by_key(|artifact| artifact.updated_at); - artifacts.reverse(); - - // Find the latest artifact - let latest_artifact = artifacts - .first() + .max_by_key(|artifact| artifact.updated_at) .ok_or_else(|| eyre::eyre!("Missing artifact value for Katana dump"))?; // Download the artifact - let client = reqwest::blocking::Client::builder(); let mut headers = header::HeaderMap::new(); let mut auth = header::HeaderValue::from_str(&format!("Bearer {}", token))?; auth.set_sensitive(true); @@ -61,12 +55,13 @@ fn main() -> Result<(), Box> { header::HeaderValue::from_static("application/vnd.github+json"), ); - let mut response = client - .user_agent("reqwest-rust") + let client_gzip = reqwest::blocking::Client::builder() + .user_agent("reqwest-rust-with-gzip") .gzip(true) .default_headers(headers) - .build()? - .get(&latest_artifact.archive_download_url) + .build()?; + let mut response = client_gzip + .get(latest_artifact.archive_download_url) .send()?; let mut out = fs::File::create("temp.zip")?; diff --git a/crates/ef-testing/src/bin/fetch_commit_kakarot.rs b/crates/ef-testing/src/bin/fetch_kakarot_submodule_commit.rs similarity index 55% rename from crates/ef-testing/src/bin/fetch_commit_kakarot.rs rename to crates/ef-testing/src/bin/fetch_kakarot_submodule_commit.rs index 9839a251..9a6171dc 100644 --- a/crates/ef-testing/src/bin/fetch_commit_kakarot.rs +++ b/crates/ef-testing/src/bin/fetch_kakarot_submodule_commit.rs @@ -1,5 +1,9 @@ +use reqwest::blocking::Client; use serde::Deserialize; +/// Fetches the commit hash of the Kakarot submodule used inside the Kakarot-RPC repository +/// Note that the latest commit of the Kakarot repository https://github.com/kkrt-labs/kakarot +/// May not be aligned with the latest commit of https://github.com/kkrt-labs/kakarot-rpc/tree/main/lib/kakarot fn main() -> Result<(), Box> { #[derive(Deserialize)] struct Blob { @@ -7,16 +11,11 @@ fn main() -> Result<(), Box> { sha: String, } - // Fetch Kakarot RPC tree - let url = "https://api.github.com/repos/sayajin-labs/kakarot-rpc/git/trees/main?recursive=1"; + let kakarot_rpc_tree_url = + "https://api.github.com/repos/kkrt-labs/kakarot-rpc/git/trees/main?recursive=1"; - let client = reqwest::blocking::Client::builder(); - let response: serde_json::Value = client - .user_agent("reqwest-rust") - .build()? - .get(url) - .send()? - .json()?; + let client = Client::builder().user_agent("reqwest-rust").build()?; + let response: serde_json::Value = client.get(kakarot_rpc_tree_url).send()?.json()?; // Filter the blobs to only include kakarot submodule let blobs: Vec = serde_json::from_value(response["tree"].clone())?; diff --git a/crates/ef-testing/src/models/case.rs b/crates/ef-testing/src/models/case.rs index 1693f064..4770b70d 100644 --- a/crates/ef-testing/src/models/case.rs +++ b/crates/ef-testing/src/models/case.rs @@ -62,6 +62,10 @@ async fn handle_pre_state( //// from more general logic that can be used across tests impl BlockchainTestCase { /// Returns whether a given test should be skipped + /// # Panics + /// + /// Will panic if the file name cannot be stringified. + #[must_use] pub fn should_skip(path: &Path) -> bool { let name = path.file_name().unwrap().to_str().unwrap(); @@ -128,7 +132,7 @@ impl BlockchainTestCase { let block = test .blocks .first() - .ok_or(RunnerError::Other("test has no blocks".to_string()))? + .ok_or_else(|| RunnerError::Other("test has no blocks".to_string()))? .clone(); // we adjust the rlp to correspond with our currently hardcoded CHAIN_ID let tx_encoded = get_signed_rlp_encoded_transaction( @@ -188,7 +192,7 @@ impl BlockchainTestCase { continue; } Some(state) => { - assert_contract_post_state(test_case_name, evm_address, expected_state, state)? + assert_contract_post_state(test_case_name, evm_address, expected_state, state)?; } }; } @@ -226,13 +230,13 @@ impl Case for BlockchainTestCase { let general_state_tests_path = general_state_tests_path.as_path(); Ok(Self { tests: { - let s = load_file(path)?; - deserialize_into(s, path)? + let file = load_file(path)?; + deserialize_into(&file, path)? }, transaction: { - let s = load_file(general_state_tests_path)?; + let file = load_file(general_state_tests_path)?; let test: BTreeMap = - deserialize_into(s, general_state_tests_path)?; + deserialize_into(&file, general_state_tests_path)?; let case = test .into_values() @@ -243,7 +247,7 @@ impl Case for BlockchainTestCase { })? .clone(); - deserialize_into(case.to_string(), general_state_tests_path)? + deserialize_into(&case.to_string(), general_state_tests_path)? }, name: test_name.to_string(), skip: Self::should_skip(path), @@ -261,7 +265,7 @@ impl Case for BlockchainTestCase { None => None, }; - for (test_name, case) in self.tests.iter() { + for (test_name, case) in &self.tests { if matches!(case.network, ForkSpec::Shanghai) { if let Some(ref test_regexp) = test_regexp { if !test_regexp.is_match(test_name) { @@ -302,8 +306,8 @@ impl Cases { /// Run the contained test cases. pub async fn run(&self) -> Vec { let mut results: Vec = Vec::new(); - for (path, case) in self.test_cases.iter() { - results.push(CaseResult::new(path, case, case.run().await)) + for (path, case) in &self.test_cases { + results.push(CaseResult::new(path, case, case.run().await)); } results } diff --git a/crates/ef-testing/src/models/error.rs b/crates/ef-testing/src/models/error.rs index 73b0b7b8..a018a9ca 100644 --- a/crates/ef-testing/src/models/error.rs +++ b/crates/ef-testing/src/models/error.rs @@ -7,7 +7,7 @@ use starknet::{ }; use starknet_api::StarknetApiError; -/// Error type based off https://github.com/paradigmxyz/reth/blob/main/testing/ef-tests/src/result.rs +/// Error type based off #[derive(Clone, Debug, thiserror::Error)] pub enum RunnerError { /// Assertion error diff --git a/crates/ef-testing/src/models/result.rs b/crates/ef-testing/src/models/result.rs index 78cbbc6d..a3c9ac4a 100644 --- a/crates/ef-testing/src/models/result.rs +++ b/crates/ef-testing/src/models/result.rs @@ -20,7 +20,7 @@ pub struct CaseResult { impl CaseResult { /// Create a new test result. pub fn new(path: &Path, case: &impl Case, result: Result<(), RunnerError>) -> Self { - CaseResult { + Self { desc: case.description(), path: path.into(), result, @@ -34,9 +34,7 @@ pub(crate) fn assert_tests_pass(suite_name: &str, path: &Path, results: &[CaseRe print_results(suite_name, path, &passed, &failed, &skipped); - if !failed.is_empty() { - panic!("Some tests failed (see above)"); - } + assert!(failed.is_empty(), "Some tests failed (see above)"); } /// Categorize test results into `(passed, failed, skipped)`. diff --git a/crates/ef-testing/src/models/suite.rs b/crates/ef-testing/src/models/suite.rs index e3a57097..aea73bb7 100644 --- a/crates/ef-testing/src/models/suite.rs +++ b/crates/ef-testing/src/models/suite.rs @@ -10,7 +10,8 @@ pub struct BlockchainTestSuite { } impl BlockchainTestSuite { - pub fn new(name: String) -> Self { + #[must_use] + pub const fn new(name: String) -> Self { Self { name } } } diff --git a/crates/ef-testing/src/storage/mod.rs b/crates/ef-testing/src/storage/mod.rs index 2753ec21..3474da95 100644 --- a/crates/ef-testing/src/storage/mod.rs +++ b/crates/ef-testing/src/storage/mod.rs @@ -42,7 +42,7 @@ pub fn write_test_state( let mut kakarot_storage = Vec::new(); for (address, account_info) in state.iter() { let mut starknet_contract_storage = Vec::new(); - let address = Felt252Wrapper::from(address.to_owned()).into(); + let address = Felt252Wrapper::from(*address).into(); let starknet_address = compute_starknet_address(kakarot_address, class_hashes.proxy_class_hash, address); @@ -140,7 +140,7 @@ pub(crate) fn starknet_storage_key_value( Ok((storage_key, storage_value)) } -/// Returns the is_initialized storage tuple. +/// Returns the `is_initialized` storage tuple. pub(crate) fn generate_is_initialized_storage( ) -> Result<(StarknetStorageKey, StarkFelt), RunnerError> { starknet_storage_key_value("is_initialized_", &[], FieldElement::ONE) diff --git a/crates/ef-testing/src/storage/models.rs b/crates/ef-testing/src/storage/models.rs index 48bd74ff..ef9d2091 100644 --- a/crates/ef-testing/src/storage/models.rs +++ b/crates/ef-testing/src/storage/models.rs @@ -7,7 +7,8 @@ pub struct ClassHashes { } impl ClassHashes { - pub fn new( + #[must_use] + pub const fn new( proxy_class_hash: FieldElement, eoa_class_hash: FieldElement, contract_account_class_hash: FieldElement, diff --git a/crates/ef-testing/src/traits/mod.rs b/crates/ef-testing/src/traits/mod.rs index 67d93e23..24166e1e 100644 --- a/crates/ef-testing/src/traits/mod.rs +++ b/crates/ef-testing/src/traits/mod.rs @@ -1,5 +1,5 @@ //! Traits definition -//! Inspired by https://github.com/paradigmxyz/reth/tree/main/testing/ef-tests +//! Inspired by use async_trait::async_trait; use std::{ @@ -52,9 +52,9 @@ pub trait Suite { let mut test_cases = Vec::new(); - for test_case_path in test_cases_paths.into_iter() { + for test_case_path in test_cases_paths { let case = Self::Case::load(&test_case_path).expect("test case should load"); - test_cases.push((test_case_path, case)) + test_cases.push((test_case_path, case)); } let results = Cases { test_cases }.run().await; diff --git a/crates/ef-testing/src/utils/assert.rs b/crates/ef-testing/src/utils/assert.rs index 258a9349..e894e753 100644 --- a/crates/ef-testing/src/utils/assert.rs +++ b/crates/ef-testing/src/utils/assert.rs @@ -87,8 +87,7 @@ pub fn assert_empty_post_state( if !is_code_empty || !is_storage_empty || !is_nonce_zero { return Err(RunnerError::Assertion(format!( - "{} expected empty post state, got {:#?}", - test_name, state + "{test_name} expected empty post state, got {state:#?}" ))); } @@ -97,8 +96,7 @@ pub fn assert_empty_post_state( if expected_balance != actual_balance { return Err(RunnerError::Assertion(format!( - "{} expected balance {:#32x}, got {:#32x}", - test_name, expected_balance, actual_balance + "{test_name} expected balance {expected_balance:#32x}, got {actual_balance:#32x}" ))); } diff --git a/crates/ef-testing/src/utils/io.rs b/crates/ef-testing/src/utils/io.rs index 4e6e7857..bc0de916 100644 --- a/crates/ef-testing/src/utils/io.rs +++ b/crates/ef-testing/src/utils/io.rs @@ -12,10 +12,10 @@ pub(crate) fn load_file(path: &Path) -> Result { } pub(crate) fn deserialize_into Deserialize<'a>>( - val: String, + val: &str, path: &Path, ) -> Result { - serde_json::from_str(&val).map_err(|error| RunnerError::Io { + serde_json::from_str(val).map_err(|error| RunnerError::Io { path: path.into(), error: error.to_string(), }) diff --git a/crates/ef-testing/tests/tests.rs b/crates/ef-testing/tests/tests.rs index 89f88732..be1fb28a 100644 --- a/crates/ef-testing/tests/tests.rs +++ b/crates/ef-testing/tests/tests.rs @@ -31,12 +31,15 @@ fn setup() { } pub fn verify_kakarot_sha() -> Result { + // This is the SHA hash of the latest Kakarot submodule commit, inside Kakarot-RPC let remote_sha = fs::read_to_string("../../.katana/remote_kakarot_sha")?; + // This is your local SHA hash of the Kakarot commit currently used in the dump. let local_sha = fs::read_to_string("../../.katana/kakarot_sha")?; + // Helper check to remind you to locally run `make fetch-dump` often if remote_sha != local_sha { return Err(eyre::eyre!(format!( - "Kakarot commit hash mismatch: local: {}, remote: {}", + "Kakarot commit hash mismatch: local: {}, remote (kakarot submodule in kakarot-rpc repository): {}", local_sha, remote_sha ))); }