fix: returndata (#419)

* refactor: return data behavior

* chore: fmt

* refactor: test utils return from context

* address pr review

* fix: stopped-> set_stopped

crates/evm/src/context.cairo

@@ -140,9 +140,9 @@ struct ExecutionContext {
     destroyed_contracts: Array<EthAddress>,
     events: Array<Event>,
     create_addresses: Array<EthAddress>,
-    return_data: Array<u8>,
+    // Return data of a child context.
+    return_data: Span<u8>,
     parent_ctx: Nullable<ExecutionContext>,
-    child_return_data: Option<Span<u8>>,
 }
 
 impl DefaultBoxExecutionContext of Default<Box<ExecutionContext>> {
@@ -165,8 +165,7 @@ impl ExecutionContextImpl of ExecutionContextTrait {
         starknet_address: ContractAddress,
         call_ctx: CallContext,
         parent_ctx: Nullable<ExecutionContext>,
-        child_return_data: Option<Span<u8>>,
-        return_data: Array<u8>,
+        return_data: Span<u8>,
     ) -> ExecutionContext {
         ExecutionContext {
             id,
@@ -180,7 +179,6 @@ impl ExecutionContextImpl of ExecutionContextTrait {
             create_addresses: Default::default(),
             return_data,
             parent_ctx,
-            child_return_data,
         }
     }
 
@@ -232,12 +230,12 @@ impl ExecutionContextImpl of ExecutionContextTrait {
 
     #[inline(always)]
     fn return_data(self: @ExecutionContext) -> Span<u8> {
-        self.return_data.span()
+        *self.return_data
     }
 
     /// Stops the current execution context.
     #[inline(always)]
-    fn stop(ref self: ExecutionContext) {
+    fn set_stopped(ref self: ExecutionContext) {
         self.status = Status::Stopped;
     }
 
@@ -247,9 +245,8 @@ impl ExecutionContextImpl of ExecutionContextTrait {
     /// (it is stopped) and contract creation and contract storage writes are
     /// reverted on its finalization.
     #[inline(always)]
-    fn revert(ref self: ExecutionContext, revert_reason: Span<u8>) {
+    fn set_reverted(ref self: ExecutionContext) {
         self.status = Status::Reverted;
-        ArrayExtensionTrait::concat(ref self.return_data, revert_reason);
     }
 
     // *************************************************************************
@@ -301,11 +298,6 @@ impl ExecutionContextImpl of ExecutionContextTrait {
         'print debug'.print();
     }
 
-    #[inline(always)]
-    fn set_return_data(ref self: ExecutionContext, value: Array<u8>) {
-        self.return_data = value;
-    }
-
     #[inline(always)]
     fn set_pc(ref self: ExecutionContext, value: u32) {
         self.program_counter = value;
@@ -316,10 +308,6 @@ impl ExecutionContextImpl of ExecutionContextTrait {
         *self.program_counter
     }
 
-    #[inline(always)]
-    fn child_return_data(self: @ExecutionContext) -> Option<Span<u8>> {
-        *self.child_return_data
-    }
 
     #[inline(always)]
     fn append_event(ref self: ExecutionContext, event: Event) {

crates/evm/src/instructions/environmental_information.cairo

@@ -165,7 +165,7 @@ impl EnvironmentInformationImpl of EnvironmentInformationTrait {
     /// Get the size of return data.
     /// # Specification: https://www.evm.codes/#3d?fork=shanghai
     fn exec_returndatasize(ref self: Machine) -> Result<(), EVMError> {
-        let size: u32 = self.return_data().len();
+        let size = self.return_data().len();
         self.stack.push(size.into())
     }
 

crates/evm/src/instructions/stop_and_arithmetic_operations.cairo

@@ -17,7 +17,7 @@ impl StopAndArithmeticOperations of StopAndArithmeticOperationsTrait {
     /// Halts the execution of the current program.
     /// # Specification: https://www.evm.codes/#00?fork=shanghai
     fn exec_stop(ref self: Machine) -> Result<(), EVMError> {
-        self.stop();
+        self.set_stopped();
         Result::Ok(())
     }
 

crates/evm/src/instructions/system_operations.cairo

@@ -32,10 +32,10 @@ impl SystemOperations of SystemOperationsTrait {
     fn exec_return(ref self: Machine) -> Result<(), EVMError> {
         let offset = self.stack.pop_usize()?;
         let size = self.stack.pop_usize()?;
-        let mut return_data = array![];
+        let mut return_data = Default::default();
         self.memory.load_n(size, ref return_data, offset);
-        self.set_return_data(return_data);
-        self.stop();
+        self.set_return_data(return_data.span());
+        self.set_stopped();
         Result::Ok(())
     }
 

crates/evm/src/interpreter.cairo

@@ -2,6 +2,7 @@
 
 /// Internal imports.
 use evm::context::{CallContextTrait, Status};
+use evm::context::{ExecutionContextTrait, ExecutionContext};
 use evm::errors::{EVMError, PC_OUT_OF_BOUNDS};
 use evm::instructions::{
     duplication_operations, environmental_information, ExchangeOperationsTrait, logging_operations,
@@ -24,7 +25,7 @@ trait EVMInterpreterTrait {
     fn run(ref self: EVMInterpreter, ref machine: Machine);
     /// Decodes the opcode at `pc` and executes the associated function.
     fn decode_and_execute(ref self: EVMInterpreter, ref machine: Machine) -> Result<(), EVMError>;
-    fn handle_revert(ref self: EVMInterpreter, ref machine: Machine);
+    fn finalize_revert(ref self: EVMInterpreter, ref machine: Machine);
 }
 
 
@@ -49,16 +50,16 @@ impl EVMInterpreterImpl of EVMInterpreterTrait {
                         machine.storage_journal.finalize_local();
                         if machine.is_root() {
                             machine.storage_journal.finalize_global();
-                        }
+                        };
                     },
-                    Status::Reverted => { self.handle_revert(ref machine); }
+                    Status::Reverted => { self.finalize_revert(ref machine); }
                 }
             },
             Result::Err(error) => {
                 // If an error occurred, revert execution machine.
                 // Currently, revert reason is a Span<u8>.
-                machine.revert(u256_to_bytes_array(error.into()).span());
-                self.handle_revert(ref machine);
+                machine.set_reverted();
+                self.finalize_revert(ref machine);
             }
         }
     }
@@ -656,9 +657,9 @@ impl EVMInterpreterImpl of EVMInterpreterTrait {
         return Result::Err(EVMError::UnknownOpcode(opcode));
     }
 
-    /// Handles the revert of an execution context.
+    /// Finalizes the revert of an execution context.
     /// Clears all pending journal entries, not finalizing the pending state changes applied inside this context.
-    fn handle_revert(ref self: EVMInterpreter, ref machine: Machine) {
+    fn finalize_revert(ref self: EVMInterpreter, ref machine: Machine) {
         machine.storage_journal.clear_local();
     //TODO add the rest of the revert handling.
     }

crates/evm/src/machine.cairo

@@ -6,6 +6,7 @@ use evm::{
     },
     stack::{Stack, StackTrait}, memory::{Memory, MemoryTrait}
 };
+use nullable::{match_nullable, FromNullableResult};
 
 use starknet::{EthAddress, ContractAddress};
 
@@ -77,9 +78,9 @@ impl MachineCurrentContextImpl of MachineCurrentContextTrait {
     }
 
     #[inline(always)]
-    fn revert(ref self: Machine, revert_reason: Span<u8>) {
+    fn set_reverted(ref self: Machine) {
         let mut current_execution_ctx = self.current_ctx.unbox();
-        current_execution_ctx.revert(revert_reason);
+        current_execution_ctx.set_reverted();
         self.current_ctx = BoxTrait::new(current_execution_ctx);
     }
 
@@ -144,14 +145,14 @@ impl MachineCurrentContextImpl of MachineCurrentContextTrait {
     #[inline(always)]
     fn return_data(ref self: Machine) -> Span<u8> {
         let current_execution_ctx = self.current_ctx.unbox();
-        let return_data = current_execution_ctx.return_data.span();
+        let return_data = current_execution_ctx.return_data;
         self.current_ctx = BoxTrait::new(current_execution_ctx);
         return_data
     }
 
     /// Stops the current execution context.
     #[inline(always)]
-    fn stop(ref self: Machine) {
+    fn set_stopped(ref self: Machine) {
         let mut current_execution_ctx = self.current_ctx.unbox();
         current_execution_ctx.status = Status::Stopped;
         self.current_ctx = BoxTrait::new(current_execution_ctx);
@@ -258,20 +259,27 @@ impl MachineCurrentContextImpl of MachineCurrentContextTrait {
         is_root
     }
 
+    /// Sets the `return_data` field of the appropriate execution context,
+    /// taking into acount EVM specs: If the current context is the root
+    /// context, sets the return_data field of the root context.  If the current
+    /// context is a subcontext, sets the return_data field of the parent.
+    /// Should be called when returning from a context.
     #[inline(always)]
-    fn set_return_data(ref self: Machine, value: Array<u8>) {
-        let mut current_execution_ctx = self.current_ctx.unbox();
-        current_execution_ctx.return_data = value;
-        self.current_ctx = BoxTrait::new(current_execution_ctx);
-    }
-
-    /// Getter for the return data of a child context, accessed from its parent context
-    /// Enabler for RETURNDATASIZE and RETURNDATACOPY opcodes
-    #[inline(always)]
-    fn child_return_data(ref self: Machine) -> Option<Span<u8>> {
-        let mut current_execution_ctx = self.current_ctx.unbox();
-        let child_return_data = current_execution_ctx.child_return_data();
-        self.current_ctx = BoxTrait::new(current_execution_ctx);
-        child_return_data
+    fn set_return_data(ref self: Machine, value: Span<u8>) {
+        let mut current_ctx = self.current_ctx.unbox();
+        let maybe_parent_ctx = current_ctx.parent_ctx;
+        match match_nullable(maybe_parent_ctx) {
+            // Due to ownership mechanism, both branches need to explicitly re-bind the parent_ctx.
+            FromNullableResult::Null => {
+                current_ctx.return_data = value;
+                current_ctx.parent_ctx = Default::default();
+            },
+            FromNullableResult::NotNull(parent_ctx) => {
+                let mut parent_ctx = parent_ctx.unbox();
+                parent_ctx.return_data = value;
+                current_ctx.parent_ctx = NullableTrait::new(parent_ctx);
+            }
+        }
+        self.current_ctx = BoxTrait::new(current_ctx);
     }
 }

crates/evm/src/tests/test_execution_context.cairo

@@ -72,13 +72,7 @@ fn test_execution_context_new() {
 
     // When
     let mut execution_context = ExecutionContextTrait::new(
-        context_id,
-        evm_address,
-        starknet_address,
-        call_ctx,
-        parent_ctx,
-        Default::default(),
-        return_data
+        context_id, evm_address, starknet_address, call_ctx, parent_ctx, return_data.span()
     );
 
     // Then
@@ -107,7 +101,7 @@ fn test_execution_context_stop_and_revert() {
     let mut execution_context = setup_execution_context();
 
     // When
-    execution_context.stop();
+    execution_context.set_stopped();
 
     // Then
     assert(execution_context.stopped() == true, 'should be stopped');
@@ -121,11 +115,10 @@ fn test_execution_context_revert() {
 
     // When
     let revert_reason = array![0, 1, 2, 3].span();
-    execution_context.revert(revert_reason);
+    execution_context.set_reverted();
 
     // Then
     assert(execution_context.reverted() == true, 'should be reverted');
-    assert(execution_context.return_data() == revert_reason, 'wrong revert reason');
 }
 
 #[test]
@@ -157,20 +150,6 @@ fn test_is_root() {
 }
 
 
-#[test]
-#[available_gas(300000)]
-fn test_child_return_data() {
-    // Given
-    let mut execution_context = setup_execution_context();
-
-    // When
-    let child_return_data = execution_context.child_return_data().unwrap();
-
-    // Then
-    assert(child_return_data == array![1, 2, 3].span(), 'wrong child_return_data');
-}
-
-
 #[test]
 #[available_gas(300000)]
 fn test_origin() {
@@ -181,5 +160,5 @@ fn test_origin() {
     let origin = execution_context.origin();
 
     // Then
-    assert(origin == evm_address(), 'wrong child_return_data');
+    assert(origin == evm_address(), 'wrong origin');
 }