From c8887b85fddbd0987802c22ce363803a11667789 Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Thu, 3 Aug 2023 12:18:28 +0200 Subject: [PATCH] refactor: use Nullable in Stack (#47) --- src/errors.cairo | 2 + src/lib.cairo | 3 ++ src/stack.cairo | 108 ++++++++++++++----------------------- src/tests/test_stack.cairo | 72 ++++++------------------- 4 files changed, 61 insertions(+), 124 deletions(-) create mode 100644 src/errors.cairo diff --git a/src/errors.cairo b/src/errors.cairo new file mode 100644 index 000000000..b11d52f0e --- /dev/null +++ b/src/errors.cairo @@ -0,0 +1,2 @@ +const STACK_OVERFLOW: felt252 = 'Kakarot: StackOverflow'; +const STACK_UNDERFLOW: felt252 = 'Kakarot: StackUnderflow'; diff --git a/src/lib.cairo b/src/lib.cairo index 4289530b5..c24666c2a 100644 --- a/src/lib.cairo +++ b/src/lib.cairo @@ -16,5 +16,8 @@ mod context; // Utils module mod utils; +// Errors module +mod errors; + // tests mod tests; diff --git a/src/stack.cairo b/src/stack.cairo index cb6519e4d..ac3e693c1 100644 --- a/src/stack.cairo +++ b/src/stack.cairo @@ -17,24 +17,34 @@ // Core lib imports use dict::Felt252DictTrait; use option::OptionTrait; +use array::ArrayTrait; use traits::Into; use result::ResultTrait; -use array::ArrayTrait; use kakarot::utils::constants; use debug::PrintTrait; +use box::BoxTrait; +use nullable::{nullable_from_box, NullableTrait}; +use kakarot::errors; -struct Stack { - items: Felt252Dict, - len: usize, +// This should be in the corelib +trait NullableTraitExt { + fn new(value: T) -> Nullable; } -impl DestructStack of Destruct { - fn destruct(self: Stack) nopanic { - self.items.squash(); +impl NullableTraitExtImpl of NullableTraitExt { + fn new(value: u256) -> Nullable { + let nullable = nullable_from_box(BoxTrait::new(value)); + nullable } } +#[derive(Destruct)] +struct Stack { + items: Felt252Dict>, + len: usize, +} + trait StackTrait { fn new() -> Stack; fn push(ref self: Stack, item: u256) -> (); @@ -53,24 +63,26 @@ impl StackImpl of StackTrait { /// Returns /// * Stack The new stack instance. fn new() -> Stack { - let items: Felt252Dict = Default::default(); + let items: Felt252Dict> = Default::default(); Stack { items, len: 0 } } - /// Pushes a new item onto the stack. + /// Pushes a new item onto the stack. If this operation would overflow the stack, + /// panics with a StackOverflow error. /// Parameters /// * self The stack instance. /// * item The item to push onto the stack. fn push(ref self: Stack, item: u256) -> () { // we can store at most 1024 256-bits words if self.len() == constants::STACK_MAX_DEPTH { - panic_with_felt252('Kakarot: StackOverflow') + panic_with_felt252(errors::STACK_OVERFLOW) } - self.insert_u256(item, self.len()); + self.items.insert(self.len.into(), NullableTraitExt::new(item)); self.len += 1; } - /// Pops the top item off the stack. + /// Pops the top item off the stack. If the stack is empty, + /// leaves the stack unchanged. /// Returns /// * Option The popped item, or None if the stack is empty. fn pop(ref self: Stack) -> Option { @@ -79,7 +91,8 @@ impl StackImpl of StackTrait { } let last_index = self.len() - 1; self.len -= 1; - Option::Some(self.get_u256(last_index)) + let item = self.items.get(last_index.into()); + Option::Some(item.deref()) } /// Pops N elements from the stack. @@ -91,7 +104,7 @@ impl StackImpl of StackTrait { /// * Array An array containing the popped items fn pop_n(ref self: Stack, mut n: usize) -> Array { if n > self.len() { - panic_with_felt252('Kakarot: StackUnderflow'); + panic_with_felt252(errors::STACK_UNDERFLOW); } let mut popped_items = ArrayTrait::::new(); loop { @@ -112,25 +125,29 @@ impl StackImpl of StackTrait { Option::None(()) } else { let last_index = self.len() - 1; - Option::Some(self.get_u256(last_index)) + let item = self.items.get(last_index.into()); + Option::Some(item.deref()) } } /// Peeks at the item at the given index on the stack. /// index is 0-based, 0 being the top of the stack. + /// If the index is too large, panics with a StackUnderflow error. /// # Arguments /// * `self` - the Stack instance /// * `index` - the index of the item to peek at - + /// /// Returns /// * u256 The item at the given index, or None if the stack is empty. fn peek_at(ref self: Stack, index: usize) -> u256 { if index >= self.len() { - panic_with_felt252('Kakarot: StackUnderflow'); + panic_with_felt252(errors::STACK_UNDERFLOW); } - let item = self.get_u256(self.len() - 1 - index); - item + let position = self.len() - 1 - index; + let item = self.items.get(position.into()); + + item.deref() } /// Swaps the item at the given index with the on on the top of the stack. @@ -145,10 +162,10 @@ impl StackImpl of StackTrait { } let position_0 = self.len() - 1; let position_item = position_0 - index; - let top_item = self.get_u256(position_0); - let swapped_item = self.get_u256(position_item); - self.insert_u256(top_item, position_item); - self.insert_u256(swapped_item, position_0); + let top_item = self.items.get(position_0.into()); + let swapped_item = self.items.get(position_item.into()); + self.items.insert(position_0.into(), swapped_item.into()); + self.items.insert(position_item.into(), top_item.into()); } /// Returns the length of the stack. @@ -171,48 +188,3 @@ impl StackImpl of StackTrait { *self.len == 0 } } - -/// Trait for helping with stack operations on 256-bit unsigned integers -trait StackU256HelperTrait { - fn dict_len(ref self: Stack) -> usize; - fn insert_u256(ref self: Stack, item: u256, index: usize); - fn get_u256(ref self: Stack, index: usize) -> u256; -} - -/// Implementation of `StackU256HelperTrait` -impl StackU256HelperImpl of StackU256HelperTrait { - /// Returns the length of the dictionary - /// - /// # Returns - /// `felt252` - the length of the dictionary - fn dict_len(ref self: Stack) -> usize { - (self.len() * 2) - } - - /// Inserts a 256-bit unsigned integer `item` into the stack at the given `index` - /// - /// # Arguments - /// * `item` - the 256-bit unsigned integer to insert into the stack - /// * `index` - the index at which to insert the item in the stack - fn insert_u256(ref self: Stack, item: u256, index: usize) { - let real_index: felt252 = index.into() * 2; - self.items.insert(real_index, item.high); - self.items.insert(real_index + 1, item.low); - } - - /// Gets a 256-bit unsigned integer from the stack at the given `index` - /// - /// # Arguments - /// * `index` - the index of the item to retrieve from the stack - /// - /// # Returns - /// `u256` - the 256-bit unsigned integer retrieved from the stack - fn get_u256(ref self: Stack, index: usize) -> u256 { - let real_index: felt252 = index.into() * 2; - let high = self.items.get(real_index.into()); - let low = self.items.get(real_index.into() + 1); - let item = u256 { low: low, high: high }; - item - } -} - diff --git a/src/tests/test_stack.cairo b/src/tests/test_stack.cairo index e04b12a98..7e831b634 100644 --- a/src/tests/test_stack.cairo +++ b/src/tests/test_stack.cairo @@ -10,7 +10,7 @@ use kakarot::stack::StackTrait; use kakarot::utils::constants; #[test] -#[available_gas(2000000)] +#[available_gas(11500)] fn test_stack_new_should_return_empty_stack() { // When let mut stack = StackTrait::new(); @@ -20,7 +20,7 @@ fn test_stack_new_should_return_empty_stack() { } #[test] -#[available_gas(2000000)] +#[available_gas(40000)] fn test__empty__should_return_if_stack_is_empty() { // Given let mut stack = StackTrait::new(); @@ -35,7 +35,7 @@ fn test__empty__should_return_if_stack_is_empty() { } #[test] -#[available_gas(2000000)] +#[available_gas(35000)] fn test__len__should_return_the_length_of_the_stack() { // Given let mut stack = StackTrait::new(); @@ -55,7 +55,7 @@ mod push { use option::OptionTrait; use super::constants; #[test] - #[available_gas(2000000)] + #[available_gas(60000)] fn test_should_add_an_element_to_the_stack() { // Given let mut stack = StackTrait::new(); @@ -72,7 +72,7 @@ mod push { } #[test] - #[available_gas(2000000000000000)] + #[available_gas(27000000)] #[should_panic(expected: ('Kakarot: StackOverflow', ))] fn test_should_fail_when_overflow() { // Given @@ -101,7 +101,7 @@ mod pop { use option::OptionTrait; #[test] - #[available_gas(2000000)] + #[available_gas(95000)] fn test_should_pop_an_element_from_the_stack() { // Given let mut stack = StackTrait::new(); @@ -118,7 +118,7 @@ mod pop { } #[test] - #[available_gas(2000000)] + #[available_gas(220000)] fn test_should_pop_N_elements_from_the_stack() { // Given let mut stack = StackTrait::new(); @@ -138,7 +138,7 @@ mod pop { } #[test] - #[available_gas(2000000)] + #[available_gas(35000)] fn test_pop_return_none_when_stack_underflow() { // Given let mut stack = StackTrait::new(); @@ -149,7 +149,7 @@ mod pop { } #[test] - #[available_gas(2000000)] + #[available_gas(50000)] #[should_panic(expected: ('Kakarot: StackUnderflow', ))] fn test_pop_n_should_fail_when_stack_underflow() { // Given @@ -167,7 +167,7 @@ mod peek { use option::OptionTrait; #[test] - #[available_gas(2000000)] + #[available_gas(80000)] fn test_should_return_last_item() { // Given let mut stack = StackTrait::new(); @@ -182,7 +182,7 @@ mod peek { } #[test] - #[available_gas(2000000)] + #[available_gas(95000)] fn test_should_return_stack_at_given_index_when_value_is_0() { // Given let mut stack = StackTrait::new(); @@ -198,7 +198,7 @@ mod peek { } #[test] - #[available_gas(2000000)] + #[available_gas(95000)] fn test_should_return_stack_at_given_index_when_value_is_1() { // Given let mut stack = StackTrait::new(); @@ -214,7 +214,7 @@ mod peek { } #[test] - #[available_gas(2000000)] + #[available_gas(35000)] #[should_panic(expected: ('Kakarot: StackUnderflow', ))] fn test_should_fail_when_underflow() { // Given @@ -229,7 +229,7 @@ mod peek { mod swap { use super::StackTrait; #[test] - #[available_gas(2000000)] + #[available_gas(400000)] fn test_should_swap_2_stack_items() { // Given let mut stack = StackTrait::new(); @@ -261,7 +261,7 @@ mod swap { } #[test] - #[available_gas(2000000)] + #[available_gas(50000)] #[should_panic(expected: ('Kakarot: StackUnderflow', ))] fn test_should_fail_when_index_1_is_underflow() { // Given @@ -272,7 +272,7 @@ mod swap { } #[test] - #[available_gas(2000000)] + #[available_gas(60000)] #[should_panic(expected: ('Kakarot: StackUnderflow', ))] fn test_should_fail_when_index_2_is_underflow() { // Given @@ -283,43 +283,3 @@ mod swap { let result = stack.swap_i(2); } } - - -#[cfg(test)] -mod stack_helper_tests { - use super::StackTrait; - use kakarot::stack::StackU256HelperTrait; - use dict::Felt252DictTrait; - use traits::Into; - use debug::PrintTrait; - - #[test] - fn test_dict_len() { - let mut stack = StackTrait::new(); - stack.len = 1; - let dict_len = stack.dict_len(); - assert(dict_len == 2, 'dict length should be 2'); - } - - #[test] - fn test_insert_u256() { - let mut stack = StackTrait::new(); - let expected: u256 = u256 { low: 100, high: 100 }; - stack.insert_u256(expected, 0); - let high = stack.items.get(0); - let low = stack.items.get(1); - let actual = u256 { low: low, high: high }; - assert(expected == actual, 'u256 not matching expected'); - } - - #[test] - fn test_get_u256() { - let mut stack = StackTrait::new(); - let v1: u256 = 100; - let v2: u256 = 101; - stack.insert_u256(v1, 0); - stack.insert_u256(v2, 1); - let item = stack.get_u256(1); - assert(v2 == item, 'u256 item should be 101'); - } -}