Skip to content

Commit

Permalink
refactor(blockifier): remove enforce fee from actual fee
Browse files Browse the repository at this point in the history
  • Loading branch information
avivg-starkware committed Sep 29, 2024
1 parent 5669fed commit 43641ba
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 25 deletions.
7 changes: 2 additions & 5 deletions crates/blockifier/src/concurrency/fee_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,8 @@ pub fn complete_fee_transfer_flow(
sequencer_balance,
);
} else {
assert_eq!(
tx_execution_info.receipt.fee,
Fee(0),
"Transaction with no fee transfer info must have zero fee."
)
let charge_fee = tx_context.tx_info.enforce_fee();
assert!(!charge_fee, "Transaction with no fee transfer info must have zero fee.")
}
}

Expand Down
13 changes: 8 additions & 5 deletions crates/blockifier/src/fee/receipt.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use cairo_vm::vm::runners::cairo_runner::ExecutionResources;
use starknet_api::core::ContractAddress;
use starknet_api::transaction::Fee;
use starknet_api::transaction::{Fee, TransactionVersion};

use crate::context::TransactionContext;
use crate::execution::call_info::ExecutionSummary;
Expand Down Expand Up @@ -92,12 +92,15 @@ impl TransactionReceipt {
&tx_context.get_gas_vector_computation_mode(),
);

// L1 handler transactions are not charged an L2 fee but it is compared to the L1 fee.
let fee = if tx_context.tx_info.enforce_fee() || tx_type == TransactionType::L1Handler {
tx_context.tx_info.get_fee_by_gas_vector(&tx_context.block_context.block_info, gas)
} else {
// To be backward compatible with the meaning of enforce fee in Declare V0.
let fee = if tx_type == TransactionType::Declare
&& tx_context.tx_info.signed_version() == TransactionVersion::ZERO
{
Fee(0)
} else {
tx_context.tx_info.get_fee_by_gas_vector(&tx_context.block_context.block_info, gas)
};

let da_gas = tx_resources
.starknet_resources
.get_state_changes_cost(tx_context.block_context.block_info.use_kzg_da);
Expand Down
2 changes: 1 addition & 1 deletion crates/blockifier/src/transaction/account_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ impl AccountTransaction {
charge_fee: bool,
concurrency_mode: bool,
) -> TransactionExecutionResult<Option<CallInfo>> {
if !charge_fee || actual_fee == Fee(0) {
if !charge_fee {
// Fee charging is not enforced in some transaction simulations and tests.
return Ok(None);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@ fn test_enforce_fee_false_works(block_context: BlockContext, #[case] version: Tr
)
.unwrap();
assert!(!tx_execution_info.is_reverted());
assert_eq!(tx_execution_info.receipt.fee, Fee(0));
}

// TODO(Dori, 15/9/2023): Convert version variance to attribute macro.
Expand Down Expand Up @@ -541,6 +540,7 @@ fn test_revert_invoke(
&block_context,
invoke_tx_args! {
max_fee,
resource_bounds: max_l1_resource_bounds(),
sender_address: account_address,
calldata: create_calldata(
test_contract_address,
Expand Down
3 changes: 2 additions & 1 deletion crates/blockifier/src/transaction/transaction_execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ impl<U: UpdatableState> ExecutableTransaction<U> for L1HandlerTransaction {
let tx_context = Arc::new(block_context.to_tx_context(self));
let limit_steps_by_resources = execution_flags.charge_fee;
let mut execution_resources = ExecutionResources::default();
let mut context = EntryPointExecutionContext::new_invoke(tx_context.clone(), limit_steps_by_resources);
let mut context =
EntryPointExecutionContext::new_invoke(tx_context.clone(), limit_steps_by_resources);
let mut remaining_gas = block_context.versioned_constants.tx_default_initial_gas();
let execute_call_info =
self.run_execute(state, &mut execution_resources, &mut context, &mut remaining_gas)?;
Expand Down
34 changes: 23 additions & 11 deletions crates/blockifier/src/transaction/transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1348,10 +1348,11 @@ fn test_declare_tx(
None,
ExecutionSummary::default(),
);

// For V0 transactions, the max fee is always 0.
let max_fee = if tx_version == TransactionVersion::ZERO { Fee(0) } else { Fee(MAX_FEE) };
let account_tx = declare_tx(
declare_tx_args! {
max_fee: Fee(MAX_FEE),
max_fee,
sender_address,
version: tx_version,
resource_bounds: max_l1_resource_bounds,
Expand All @@ -1370,7 +1371,8 @@ fn test_declare_tx(
);
let fee_type = &account_tx.fee_type();
let tx_context = &block_context.to_tx_context(&account_tx);
let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap();
let actual_execution_info =
account_tx.execute(state, block_context, tx_context.tx_info.enforce_fee(), true).unwrap(); //aviv: changed from true, true

// Build expected validate call info.
let expected_validate_call_info = declare_validate_callinfo(
Expand All @@ -1385,14 +1387,24 @@ fn test_declare_tx(
);

// Build expected fee transfer call info.
let expected_actual_fee =
actual_execution_info.receipt.resources.calculate_tx_fee(block_context, fee_type);
let expected_fee_transfer_call_info = expected_fee_transfer_call_info(
tx_context,
sender_address,
expected_actual_fee,
FeatureContract::ERC20(CairoVersion::Cairo0).get_class_hash(),
);
// aviv added 'if' condition. not pretty solution:
let expected_actual_fee = if tx_context.tx_info.signed_version() == TransactionVersion::ZERO {
Fee(0)
} else {
actual_execution_info.receipt.resources.calculate_tx_fee(block_context, fee_type)
};
// aviv added 'if' condition. not pretty solution:
let expected_fee_transfer_call_info =
if tx_context.tx_info.signed_version() == TransactionVersion::ZERO {
None
} else {
expected_fee_transfer_call_info(
tx_context,
sender_address,
expected_actual_fee,
FeatureContract::ERC20(CairoVersion::Cairo0).get_class_hash(),
)
};

let da_gas = starknet_resources.get_state_changes_cost(use_kzg_da);
let expected_cairo_resources = get_expected_cairo_resources(
Expand Down
9 changes: 8 additions & 1 deletion crates/starknet_api/src/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,14 @@ impl ValidResourceBounds {
// TODO(Nimrod): Default testing bounds should probably be AllResourceBounds variant.
#[cfg(any(feature = "testing", test))]
pub fn create_for_testing() -> Self {
Self::L1Gas(ResourceBounds { max_amount: 0, max_price_per_unit: 1 })
// Self::L1Gas(ResourceBounds { max_amount: 0, max_price_per_unit: 1 })
// Aviv: suggestion?
let resource_bounds = AllResourceBounds {
l1_gas: ResourceBounds { max_amount: 0, max_price_per_unit: 1 },
l2_gas: ResourceBounds { max_amount: 0, max_price_per_unit: 1 },
l1_data_gas: ResourceBounds { max_amount: 0, max_price_per_unit: 1 },
};
Self::AllResources(resource_bounds)
}
}

Expand Down

0 comments on commit 43641ba

Please sign in to comment.