refactor: refactor according to clippy pedantic recommendation and ad…

…d 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

@@ -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"] }

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)

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 <https://github.com/paradigmxyz/reth/tree/main/testing/ef-tests>, it uses some code snippets from the Reth codebase and when possible, imports modules and helpers from it.

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"]

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<dyn std::error::Error>> {
     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<Artifact> = 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::<Vec<_>>();
-    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<dyn std::error::Error>> {
         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")?;

crates/ef-testing/src/bin/fetch_commit_kakarot.rs → crates/ef-testing/src/bin/fetch_kakarot_submodule_commit.rs

@@ -1,22 +1,21 @@
+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<dyn std::error::Error>> {
     #[derive(Deserialize)]
     struct Blob {
         path: String,
         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<Blob> = serde_json::from_value(response["tree"].clone())?;

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<String, serde_json::Value> =
-                    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<T: Case> Cases<T> {
     /// Run the contained test cases.
     pub async fn run(&self) -> Vec<CaseResult> {
         let mut results: Vec<CaseResult> = 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
     }

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 <https://github.com/paradigmxyz/reth/blob/main/testing/ef-tests/src/result.rs>
 #[derive(Clone, Debug, thiserror::Error)]
 pub enum RunnerError {
     /// Assertion error

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)`.

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 }
     }
 }

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)

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,

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 <https://github.com/paradigmxyz/reth/tree/main/testing/ef-tests>
 
 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;

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}"
         )));
     }
 

crates/ef-testing/src/utils/io.rs

@@ -12,10 +12,10 @@ pub(crate) fn load_file(path: &Path) -> Result<String, RunnerError> {
 }
 
 pub(crate) fn deserialize_into<T: for<'a> Deserialize<'a>>(
-    val: String,
+    val: &str,
     path: &Path,
 ) -> Result<T, RunnerError> {
-    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(),
     })