Use an enum to represent layout name (#1715)

* Use enum for LayoutName * Move LayoutName to its own module + fix tests * Fix clap enum parsing * Fix tests * Fix test & doc * Manually implement ValueEnum * fmt * Simplify code * Move arbitrary impl to module * Fix benchmark code * Add changelog entry * Remove unnecessary impl * Use value instead of reference Co-authored-by: Mario Rugiero <[email protected]> * Fix test added by merge --------- Co-authored-by: Mario Rugiero <[email protected]>
lambdaclass · Apr 18, 2024 · 932986c · 932986c
1 parent 4b17118
commit 932986c
Show file tree

Hide file tree

Showing 30 changed files with 240 additions and 216 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 #### Upcoming Changes
 
+* refactor(BREAKING): Use an enum to represent layout name[#1715](https://github.com/lambdaclass/cairo-vm/pull/1715)
+  * Add enum `LayoutName` to represent cairo layout names.
+  * `CairoRunConfig`, `Cairo1RunConfig` & `CairoRunner` field `layout` type changed from `String` to `LayoutName`.
+  * `CairoLayout` field `name` type changed from `String` to `LayoutName`.
+
 * fix(BREAKING): Remove unsafe impl of `Add<usize> for &'a Relocatable`[#1718](https://github.com/lambdaclass/cairo-vm/pull/1718)
 
 * fix(BREAKING): Handle triple dereference references[#1708](https://github.com/lambdaclass/cairo-vm/pull/1708)

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/README.md b/README.md
@@ -209,7 +209,7 @@ When running a Cairo program directly using the Cairo-vm repository you would fi
   ```rust
   let mut vm = VirtualMachine::new(false);
 
-  let mut cairo_runner = CairoRunner::new(&program, "all_cairo", false);
+  let mut cairo_runner = CairoRunner::new(&program, LayoutName::all_cairo, false);
 
   let mut hint_processor = BuiltinHintProcessor::new_empty();
 

diff --git a/bench/criterion_benchmark.rs b/bench/criterion_benchmark.rs
@@ -1,5 +1,5 @@
 use cairo_vm::{
-    types::program::Program,
+    types::{layout_name::LayoutName, program::Program},
     vm::{runners::cairo_runner::CairoRunner, vm_core::VirtualMachine},
 };
 use criterion::{black_box, criterion_group, criterion_main, BatchSize, Criterion};
@@ -29,7 +29,7 @@ fn build_many_runners(c: &mut Criterion) {
             _ = black_box(
                 CairoRunner::new(
                     black_box(&program),
-                    black_box("starknet_with_keccak"),
+                    black_box(LayoutName::starknet_with_keccak),
                     black_box(false),
                 )
                 .unwrap(),
@@ -46,7 +46,7 @@ fn load_program_data(c: &mut Criterion) {
         b.iter_batched(
             || {
                 (
-                    CairoRunner::new(&program, "starknet_with_keccak", false).unwrap(),
+                    CairoRunner::new(&program, LayoutName::starknet_with_keccak, false).unwrap(),
                     VirtualMachine::new(false),
                 )
             },

diff --git a/bench/iai_benchmark.rs b/bench/iai_benchmark.rs
@@ -2,7 +2,7 @@ use core::hint::black_box;
 use iai_callgrind::main;
 
 use cairo_vm::{
-    types::program::Program,
+    types::{layout_name::LayoutName, program::Program},
     vm::{runners::cairo_runner::CairoRunner, vm_core::VirtualMachine},
 };
 
@@ -31,7 +31,8 @@ fn parse_program_helper() -> Program {
 #[inline(never)]
 fn build_runner() {
     let program = parse_program_helper();
-    let runner = CairoRunner::new(black_box(&program), "starknet_with_keccak", false).unwrap();
+    let runner =
+        CairoRunner::new(black_box(&program), LayoutName::starknet_with_keccak, false).unwrap();
     core::mem::drop(black_box(runner));
 }
 
@@ -41,7 +42,7 @@ fn build_runner_helper() -> (CairoRunner, VirtualMachine) {
     //Picked the biggest one at the time of writing
     let program = include_bytes!("../cairo_programs/benchmarks/keccak_integration_benchmark.json");
     let program = Program::from_bytes(program.as_slice(), Some("main")).unwrap();
-    let runner = CairoRunner::new(&program, "starknet_with_keccak", false).unwrap();
+    let runner = CairoRunner::new(&program, LayoutName::starknet_with_keccak, false).unwrap();
     let vm = VirtualMachine::new(false);
     (runner, vm)
 }

diff --git a/cairo-vm-cli/Cargo.toml b/cairo-vm-cli/Cargo.toml
@@ -8,7 +8,7 @@ readme.workspace = true
 keywords.workspace = true
 
 [dependencies]
-cairo-vm = { workspace = true, features = ["std"] }
+cairo-vm = { workspace = true, features = ["std", "clap"] }
 cairo-vm-tracer = { workspace = true, optional = true }
 clap = { version = "4.3.10", features = ["derive"] }
 mimalloc = { version = "0.1.37", default-features = false, optional = true }

diff --git a/cairo-vm-cli/src/main.rs b/cairo-vm-cli/src/main.rs
@@ -6,6 +6,7 @@ use cairo_vm::cairo_run::{self, EncodeTraceError};
 use cairo_vm::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor;
 #[cfg(feature = "with_tracer")]
 use cairo_vm::serde::deserialize_program::DebugInfo;
+use cairo_vm::types::layout_name::LayoutName;
 use cairo_vm::vm::errors::cairo_run_errors::CairoRunError;
 use cairo_vm::vm::errors::trace_errors::TraceError;
 use cairo_vm::vm::errors::vm_errors::VirtualMachineError;
@@ -42,8 +43,8 @@ struct Args {
     entrypoint: String,
     #[structopt(long = "memory_file")]
     memory_file: Option<PathBuf>,
-    #[clap(long = "layout", default_value = "plain", value_parser=validate_layout)]
-    layout: String,
+    #[clap(long = "layout", default_value = "plain", value_enum)]
+    layout: LayoutName,
     #[structopt(long = "proof_mode")]
     proof_mode: bool,
     #[structopt(long = "secure_run")]
@@ -69,22 +70,6 @@ struct Args {
     tracer: bool,
 }
 
-fn validate_layout(value: &str) -> Result<String, String> {
-    match value {
-        "plain"
-        | "small"
-        | "dex"
-        | "recursive"
-        | "starknet"
-        | "starknet_with_keccak"
-        | "recursive_large_output"
-        | "all_cairo"
-        | "all_solidity"
-        | "dynamic" => Ok(value.to_string()),
-        _ => Err(format!("{value} is not a valid layout")),
-    }
-}
-
 #[derive(Debug, Error)]
 enum Error {
     #[error("Invalid arguments")]
@@ -173,7 +158,7 @@ fn run(args: impl Iterator<Item = String>) -> Result<(), Error> {
         entrypoint: &args.entrypoint,
         trace_enabled,
         relocate_mem: args.memory_file.is_some() || args.air_public_input.is_some(),
-        layout: &args.layout,
+        layout: args.layout,
         proof_mode: args.proof_mode,
         secure_run: args.secure_run,
         allow_missing_builtins: args.allow_missing_builtins,
@@ -410,28 +395,4 @@ mod tests {
     fn test_main() {
         main().unwrap();
     }
-
-    #[test]
-    fn test_valid_layouts() {
-        let valid_layouts = vec![
-            "plain",
-            "small",
-            "dex",
-            "starknet",
-            "starknet_with_keccak",
-            "recursive_large_output",
-            "all_cairo",
-            "all_solidity",
-        ];
-
-        for layout in valid_layouts {
-            assert_eq!(validate_layout(layout), Ok(layout.to_string()));
-        }
-    }
-
-    #[test]
-    fn test_invalid_layout() {
-        let invalid_layout = "invalid layout name";
-        assert!(validate_layout(invalid_layout).is_err());
-    }
 }
diff --git a/cairo1-run/Cargo.toml b/cairo1-run/Cargo.toml
@@ -9,7 +9,7 @@ keywords.workspace = true
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-cairo-vm = {workspace = true, features = ["std", "cairo-1-hints"]}
+cairo-vm = {workspace = true, features = ["std", "cairo-1-hints", "clap"]}
 
 cairo-lang-sierra-type-size = { version = "2.5.4", default-features = false }
 cairo-lang-sierra-ap-change = { version = "2.5.4", default-features = false }

diff --git a/cairo1-run/src/cairo_run.rs b/cairo1-run/src/cairo_run.rs
@@ -32,7 +32,7 @@ use cairo_vm::{
     serde::deserialize_program::{
         ApTracking, BuiltinName, FlowTrackingData, HintParams, ReferenceManager,
     },
-    types::{program::Program, relocatable::MaybeRelocatable},
+    types::{layout_name::LayoutName, program::Program, relocatable::MaybeRelocatable},
     vm::{
         errors::{runner_errors::RunnerError, vm_errors::VirtualMachineError},
         runners::{
@@ -59,7 +59,7 @@ pub struct Cairo1RunConfig<'a> {
     pub serialize_output: bool,
     pub trace_enabled: bool,
     pub relocate_mem: bool,
-    pub layout: &'a str,
+    pub layout: LayoutName,
     pub proof_mode: bool,
     // Should be true if either air_public_input or cairo_pie_output are needed
     // Sets builtins stop_ptr by calling `final_stack` on each builtin
@@ -75,7 +75,7 @@ impl Default for Cairo1RunConfig<'_> {
             serialize_output: false,
             trace_enabled: false,
             relocate_mem: false,
-            layout: "plain",
+            layout: LayoutName::plain,
             proof_mode: false,
             finalize_builtins: false,
             append_return_values: false,
@@ -1170,7 +1170,7 @@ mod tests {
         // Set proof_mode
         let cairo_run_config = Cairo1RunConfig {
             proof_mode,
-            layout: "all_cairo",
+            layout: LayoutName::all_cairo,
             append_return_values: !proof_mode, // This is so we can test appending return values when not running in proof_mode
             finalize_builtins: true,
             ..Default::default()

diff --git a/cairo1-run/src/main.rs b/cairo1-run/src/main.rs
@@ -6,7 +6,7 @@ use cairo_run::Cairo1RunConfig;
 use cairo_vm::{
     air_public_input::PublicInputError,
     cairo_run::EncodeTraceError,
-    types::errors::program_errors::ProgramError,
+    types::{errors::program_errors::ProgramError, layout_name::LayoutName},
     vm::errors::{
         memory_errors::MemoryError, runner_errors::RunnerError, trace_errors::TraceError,
         vm_errors::VirtualMachineError,
@@ -32,8 +32,8 @@ struct Args {
     trace_file: Option<PathBuf>,
     #[structopt(long = "memory_file")]
     memory_file: Option<PathBuf>,
-    #[clap(long = "layout", default_value = "plain", value_parser=validate_layout)]
-    layout: String,
+    #[clap(long = "layout", default_value = "plain", value_enum)]
+    layout: LayoutName,
     #[clap(long = "proof_mode", value_parser)]
     proof_mode: bool,
     #[clap(long = "air_public_input", requires = "proof_mode")]
@@ -109,22 +109,6 @@ fn process_args(value: &str) -> Result<FuncArgs, String> {
     Ok(FuncArgs(args))
 }
 
-fn validate_layout(value: &str) -> Result<String, String> {
-    match value {
-        "plain"
-        | "small"
-        | "dex"
-        | "recursive"
-        | "starknet"
-        | "starknet_with_keccak"
-        | "recursive_large_output"
-        | "all_cairo"
-        | "all_solidity"
-        | "dynamic" => Ok(value.to_string()),
-        _ => Err(format!("{value} is not a valid layout")),
-    }
-}
-
 #[derive(Debug, Error)]
 pub enum Error {
     #[error("Invalid arguments")]
@@ -214,7 +198,7 @@ fn run(args: impl Iterator<Item = String>) -> Result<Option<String>, Error> {
         proof_mode: args.proof_mode,
         serialize_output: args.print_output,
         relocate_mem: args.memory_file.is_some() || args.air_public_input.is_some(),
-        layout: &args.layout,
+        layout: args.layout,
         trace_enabled: args.trace_file.is_some() || args.air_public_input.is_some(),
         args: &args.args.0,
         finalize_builtins: args.air_private_input.is_some() || args.cairo_pie_output.is_some(),

diff --git a/examples/custom_hint/src/main.rs b/examples/custom_hint/src/main.rs
@@ -6,6 +6,7 @@ use cairo_vm::hint_processor::builtin_hint_processor::hint_utils::get_integer_fr
 use cairo_vm::hint_processor::hint_processor_definition::HintReference;
 use cairo_vm::serde::deserialize_program::ApTracking;
 use cairo_vm::types::exec_scope::ExecutionScopes;
+use cairo_vm::types::layout_name::LayoutName;
 use cairo_vm::vm::{errors::hint_errors::HintError, vm_core::VirtualMachine};
 use cairo_vm::Felt252;
 use std::collections::HashMap;
@@ -46,7 +47,7 @@ fn main() {
     cairo_run(
         &buffer,
         &CairoRunConfig {
-            layout: "all_cairo",
+            layout: LayoutName::all_cairo,
             ..Default::default()
         },
         &mut hint_processor,

diff --git a/examples/hyper_threading/src/main.rs b/examples/hyper_threading/src/main.rs
@@ -1,7 +1,7 @@
 use cairo_vm::{
     cairo_run::{cairo_run_program, CairoRunConfig},
     hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor,
-    types::program::Program,
+    types::{layout_name::LayoutName, program::Program},
 };
 use rayon::iter::{IntoParallelIterator, ParallelIterator};
 use std::path::Path;
@@ -51,7 +51,7 @@ fn main() {
             entrypoint: "main",
             trace_enabled: false,
             relocate_mem: false,
-            layout: "all_cairo",
+            layout: LayoutName::all_cairo,
             proof_mode: true,
             secure_run: Some(false),
             ..Default::default()

diff --git a/examples/wasm-demo/src/lib.rs b/examples/wasm-demo/src/lib.rs
@@ -3,6 +3,7 @@ mod utils;
 use cairo_vm::{
     cairo_run::{cairo_run, CairoRunConfig},
     hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor,
+    types::layout_name::LayoutName,
 };
 use wasm_bindgen::prelude::*;
 
@@ -32,7 +33,7 @@ pub fn run_cairo_program() -> Result<String, JsError> {
     let mut hint_executor = BuiltinHintProcessor::new_empty();
 
     let cairo_run_config = CairoRunConfig {
-        layout: "all_cairo",
+        layout: LayoutName::all_cairo,
         relocate_mem: true,
         trace_enabled: true,
         ..Default::default()

diff --git a/vm/Cargo.toml b/vm/Cargo.toml
@@ -80,6 +80,9 @@ ark-std = { workspace = true, optional = true }
 # Enable arbitrary when fuzzing
 arbitrary = { workspace = true, features = ["derive"], optional = true }
 
+# Used to derive clap traits for CLIs
+clap = { version = "4.3.10", features = ["derive"], optional = true}
+
 [dev-dependencies]
 assert_matches = "1.5.0"
 rstest = { version = "0.17.0", default-features = false }

diff --git a/vm/src/air_private_input.rs b/vm/src/air_private_input.rs
@@ -208,6 +208,7 @@ impl AirPrivateInputSerializable {
 
 #[cfg(test)]
 mod tests {
+    use crate::types::layout_name::LayoutName;
     #[cfg(feature = "std")]
     use {
         super::*,
@@ -299,7 +300,7 @@ mod tests {
             proof_mode: true,
             relocate_mem: true,
             trace_enabled: true,
-            layout: "small",
+            layout: LayoutName::small,
             ..Default::default()
         };
         let (runner, vm) = crate::cairo_run::cairo_run(include_bytes!("../../cairo_programs/proof_programs/fibonacci.json"), &config, &mut crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor::new_empty()).unwrap();

diff --git a/vm/src/air_public_input.rs b/vm/src/air_public_input.rs
@@ -192,11 +192,13 @@ mod tests {
     #[case(include_bytes!("../../cairo_programs/proof_programs/pedersen_test.json"))]
     #[case(include_bytes!("../../cairo_programs/proof_programs/ec_op.json"))]
     fn serialize_and_deserialize_air_public_input(#[case] program_content: &[u8]) {
+        use crate::types::layout_name::LayoutName;
+
         let config = crate::cairo_run::CairoRunConfig {
             proof_mode: true,
             relocate_mem: true,
             trace_enabled: true,
-            layout: "all_cairo",
+            layout: LayoutName::all_cairo,
             ..Default::default()
         };
         let (runner, vm) = crate::cairo_run::cairo_run(program_content, &config, &mut crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor::new_empty()).unwrap();