Update charge fee and add n_steps for reverted transactions (#787)

* Update charge fee * Fix fee test * Update n_reverted_steps usage * Add docs * apply doc suggestion to src/transaction/fee.rs Co-authored-by: Tomás <[email protected]> * Test test_reverted_transaction_wrong_entry_point * Update docs --------- Co-authored-by: Juan Bono <[email protected]> Co-authored-by: Tomás <[email protected]>
lambdaclass · Jul 17, 2023 · 5448721 · 5448721
1 parent 8809ba0
commit 5448721
Show file tree

Hide file tree

Showing 13 changed files with 268 additions and 223 deletions.
diff --git a/src/definitions/constants.rs b/src/definitions/constants.rs
@@ -22,6 +22,10 @@ lazy_static! {
 pub(crate) const LOG_MSG_TO_L1_ENCODED_DATA_SIZE: usize =
     (L2_TO_L1_MSG_HEADER_SIZE + 1) - LOG_MSG_TO_L1_N_TOPICS;
 
+/// Fee factor used for the final fee calculation:
+/// actual_fee = min(max_fee, consumed_resources) * FEE_FACTOR
+pub(crate) const FEE_FACTOR: u128 = 1;
+
 /// The (empirical) L1 gas cost of each Cairo step.
 pub(crate) const N_STEPS_FEE_WEIGHT: f64 = 0.01;
 

diff --git a/src/lib.rs b/src/lib.rs
@@ -287,7 +287,7 @@ mod test {
         let transaction = Transaction::InvokeFunction(invoke_function);
 
         let estimated_fee = estimate_fee(&[transaction], state, &block_context).unwrap();
-        assert_eq!(estimated_fee[0], (12, 0));
+        assert_eq!(estimated_fee[0], (30, 0));
     }
 
     #[test]
@@ -375,15 +375,10 @@ mod test {
         state.set_contract_classes(contract_classes).unwrap();
 
         let mut block_context = BlockContext::default();
-        block_context.cairo_resource_fee_weights = HashMap::from([
-            (String::from("l1_gas_usage"), 0.into()),
-            (String::from("pedersen_builtin"), 16.into()),
-            (String::from("range_check_builtin"), 70.into()),
-        ]);
         block_context.starknet_os_config.gas_price = 1;
 
         let estimated_fee = estimate_message_fee(&l1_handler, state, &block_context).unwrap();
-        assert_eq!(estimated_fee, (20081, 18471));
+        assert_eq!(estimated_fee, (18484, 18471));
     }
 
     #[test]
@@ -921,11 +916,6 @@ mod test {
             .unwrap();
 
         let mut block_context = BlockContext::default();
-        block_context.cairo_resource_fee_weights = HashMap::from([
-            (String::from("l1_gas_usage"), 0.into()),
-            (String::from("pedersen_builtin"), 16.into()),
-            (String::from("range_check_builtin"), 70.into()),
-        ]);
         block_context.starknet_os_config.gas_price = 1;
 
         simulate_transaction(

diff --git a/src/testing/state.rs b/src/testing/state.rs
@@ -358,6 +358,7 @@ mod tests {
 
         let mut actual_resources = HashMap::new();
         actual_resources.insert("l1_gas_usage".to_string(), 1224);
+        actual_resources.insert("n_steps".to_string(), 0);
 
         let transaction_exec_info = TransactionExecutionInfo {
             validate_info: None,
@@ -572,6 +573,7 @@ mod tests {
         )
         .unwrap();
         let actual_resources = HashMap::from([
+            ("n_steps".to_string(), 2933),
             ("l1_gas_usage".to_string(), 0),
             ("range_check_builtin".to_string(), 70),
             ("pedersen_builtin".to_string(), 16),

diff --git a/src/transaction/declare.rs b/src/transaction/declare.rs
@@ -18,19 +18,16 @@ use crate::{
     services::api::contract_classes::deprecated_contract_class::ContractClass,
     state::state_api::{State, StateReader},
     state::ExecutionResourcesManager,
-    transaction::{
-        error::TransactionError,
-        fee::{calculate_tx_fee, execute_fee_transfer, FeeInfo},
-    },
+    transaction::error::TransactionError,
     utils::{
         calculate_tx_resources, felt_to_hash, verify_no_calls_to_other_contracts, Address,
         ClassHash,
     },
 };
 use cairo_vm::felt::Felt252;
 use num_traits::Zero;
-use std::collections::HashMap;
 
+use super::fee::charge_fee;
 use super::{verify_version, Transaction};
 
 // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -176,6 +173,7 @@ impl Declare {
             TransactionType::Declare,
             changes,
             None,
+            0,
         )
         .map_err(|_| TransactionError::ResourcesCalculation)?;
 
@@ -243,38 +241,6 @@ impl Declare {
         Ok(Some(call_info))
     }
 
-    /// Calculates and charges the actual fee.
-    pub fn charge_fee<S: StateReader>(
-        &self,
-        state: &mut CachedState<S>,
-        resources: &HashMap<String, usize>,
-        block_context: &BlockContext,
-    ) -> Result<FeeInfo, TransactionError> {
-        if self.max_fee.is_zero() {
-            return Ok((None, 0));
-        }
-
-        let actual_fee = calculate_tx_fee(
-            resources,
-            block_context.starknet_os_config.gas_price,
-            block_context,
-        )?;
-
-        let mut tx_execution_context =
-            self.get_execution_context(block_context.invoke_tx_max_n_steps);
-        let fee_transfer_info = if self.skip_fee_transfer {
-            None
-        } else {
-            Some(execute_fee_transfer(
-                state,
-                block_context,
-                &mut tx_execution_context,
-                actual_fee,
-            )?)
-        };
-        Ok((fee_transfer_info, actual_fee))
-    }
-
     fn handle_nonce<S: State + StateReader>(&self, state: &mut S) -> Result<(), TransactionError> {
         if self.version.is_zero() || self.version == *QUERY_VERSION_BASE {
             return Ok(());
@@ -304,8 +270,16 @@ impl Declare {
         let mut tx_exec_info = self.apply(state, block_context)?;
         self.handle_nonce(state)?;
 
-        let (fee_transfer_info, actual_fee) =
-            self.charge_fee(state, &tx_exec_info.actual_resources, block_context)?;
+        let mut tx_execution_context =
+            self.get_execution_context(block_context.invoke_tx_max_n_steps);
+        let (fee_transfer_info, actual_fee) = charge_fee(
+            state,
+            &tx_exec_info.actual_resources,
+            block_context,
+            self.max_fee,
+            &mut tx_execution_context,
+            self.skip_fee_transfer,
+        )?;
 
         state.set_contract_class(&self.class_hash, &self.contract_class)?;
 
@@ -444,6 +418,7 @@ mod tests {
         });
 
         let actual_resources = HashMap::from([
+            ("n_steps".to_string(), 2348),
             ("l1_gas_usage".to_string(), 0),
             ("range_check_builtin".to_string(), 57),
             ("pedersen_builtin".to_string(), 15),

diff --git a/src/transaction/declare_v2.rs b/src/transaction/declare_v2.rs
@@ -1,3 +1,4 @@
+use super::fee::charge_fee;
 use super::{verify_version, Transaction};
 use crate::core::contract_address::{compute_casm_class_hash, compute_sierra_class_hash};
 use crate::definitions::constants::QUERY_VERSION_BASE;
@@ -13,23 +14,18 @@ use crate::{
         transaction_type::TransactionType,
     },
     execution::{
-        execution_entry_point::ExecutionEntryPoint, CallInfo, CallType,
-        TransactionExecutionContext, TransactionExecutionInfo,
+        execution_entry_point::ExecutionEntryPoint, CallType, TransactionExecutionContext,
+        TransactionExecutionInfo,
     },
     state::state_api::{State, StateReader},
     state::ExecutionResourcesManager,
-    transaction::{
-        error::TransactionError,
-        fee::{calculate_tx_fee, execute_fee_transfer, FeeInfo},
-        invoke_function::verify_no_calls_to_other_contracts,
-    },
+    transaction::{error::TransactionError, invoke_function::verify_no_calls_to_other_contracts},
     utils::{calculate_tx_resources, Address},
 };
 use cairo_lang_starknet::casm_contract_class::CasmContractClass;
 use cairo_lang_starknet::contract_class::ContractClass as SierraContractClass;
 use cairo_vm::felt::Felt252;
 use num_traits::Zero;
-use std::collections::HashMap;
 
 /// Represents a declare transaction in the starknet network.
 /// Declare creates a blueprint of a contract class that is used to deploy instances of the contract
@@ -274,43 +270,6 @@ impl DeclareV2 {
         Vec::from([bytes])
     }
 
-    /// Calculates and charges the actual fee.
-    /// ## Parameter:
-    /// - state: An state that implements the State and StateReader traits.
-    /// - resources: the resources that are in use by the contract
-    /// - block_context: The block that contains the execution context
-    pub fn charge_fee<S: StateReader>(
-        &self,
-        state: &mut CachedState<S>,
-        resources: &HashMap<String, usize>,
-        block_context: &BlockContext,
-    ) -> Result<FeeInfo, TransactionError> {
-        if self.max_fee.is_zero() {
-            return Ok((None, 0));
-        }
-
-        let actual_fee = calculate_tx_fee(
-            resources,
-            block_context.starknet_os_config.gas_price,
-            block_context,
-        )?;
-
-        let mut tx_execution_context =
-            self.get_execution_context(block_context.invoke_tx_max_n_steps);
-        let fee_transfer_info = if self.skip_fee_transfer {
-            None
-        } else {
-            Some(execute_fee_transfer(
-                state,
-                block_context,
-                &mut tx_execution_context,
-                actual_fee,
-            )?)
-        };
-
-        Ok((fee_transfer_info, actual_fee))
-    }
-
     // TODO: delete once used
     #[allow(dead_code)]
     fn handle_nonce<S: State + StateReader>(&self, state: &mut S) -> Result<(), TransactionError> {
@@ -347,33 +306,42 @@ impl DeclareV2 {
 
         let mut resources_manager = ExecutionResourcesManager::default();
 
-        let (validate_info, _remaining_gas) = if self.skip_validate {
-            (None, 0)
+        let (execution_result, _remaining_gas) = if self.skip_validate {
+            (ExecutionResult::default(), 0)
         } else {
             let (info, gas) = self.run_validate_entrypoint(
                 initial_gas,
                 state,
                 &mut resources_manager,
                 block_context,
             )?;
-            (Some(info), gas)
+            (info, gas)
         };
 
         let storage_changes = state.count_actual_storage_changes();
         let actual_resources = calculate_tx_resources(
             resources_manager,
-            &[validate_info.clone()],
+            &[execution_result.call_info.clone()],
             self.tx_type,
             storage_changes,
             None,
+            execution_result.n_reverted_steps,
         )?;
 
-        let (fee_transfer_info, actual_fee) =
-            self.charge_fee(state, &actual_resources, block_context)?;
+        let mut tx_execution_context =
+            self.get_execution_context(block_context.invoke_tx_max_n_steps);
+        let (fee_transfer_info, actual_fee) = charge_fee(
+            state,
+            &actual_resources,
+            block_context,
+            self.max_fee,
+            &mut tx_execution_context,
+            self.skip_fee_transfer,
+        )?;
         self.compile_and_store_casm_class(state)?;
 
         let mut tx_exec_info = TransactionExecutionInfo::new_without_fee_info(
-            validate_info,
+            execution_result.call_info,
             None,
             None,
             actual_resources,
@@ -415,7 +383,7 @@ impl DeclareV2 {
         state: &mut CachedState<S>,
         resources_manager: &mut ExecutionResourcesManager,
         block_context: &BlockContext,
-    ) -> Result<(CallInfo, u128), TransactionError> {
+    ) -> Result<(ExecutionResult, u128), TransactionError> {
         let calldata = [self.compiled_class_hash.clone()].to_vec();
 
         let entry_point = ExecutionEntryPoint {
@@ -433,7 +401,7 @@ impl DeclareV2 {
         let mut tx_execution_context =
             self.get_execution_context(block_context.validate_max_n_steps);
 
-        let ExecutionResult { call_info, .. } = if self.skip_execute {
+        let execution_result = if self.skip_execute {
             ExecutionResult::default()
         } else {
             entry_point.execute(
@@ -445,10 +413,13 @@ impl DeclareV2 {
                 block_context.validate_max_n_steps,
             )?
         };
-        let call_info = verify_no_calls_to_other_contracts(&call_info)?;
-        remaining_gas -= call_info.gas_consumed;
 
-        Ok((call_info, remaining_gas))
+        if execution_result.call_info.is_some() {
+            verify_no_calls_to_other_contracts(&execution_result.call_info)?;
+            remaining_gas -= execution_result.call_info.clone().unwrap().gas_consumed;
+        }
+
+        Ok((execution_result, remaining_gas))
     }
 
     // ---------------

diff --git a/src/transaction/deploy.rs b/src/transaction/deploy.rs
@@ -185,6 +185,7 @@ impl Deploy {
             self.tx_type,
             changes,
             None,
+            0,
         )?;
 
         Ok(TransactionExecutionInfo::new_without_fee_info(
@@ -230,7 +231,7 @@ impl Deploy {
         let ExecutionResult {
             call_info,
             revert_error,
-            ..
+            n_reverted_steps,
         } = call.execute(
             state,
             block_context,
@@ -247,6 +248,7 @@ impl Deploy {
             self.tx_type,
             changes,
             None,
+            n_reverted_steps,
         )?;
 
         Ok(TransactionExecutionInfo::new_without_fee_info(