From 1b6ef7981f33691af31e0b499b8cde72c8e0b9da Mon Sep 17 00:00:00 2001 From: Aviv Greenburg Date: Mon, 28 Oct 2024 17:43:27 +0200 Subject: [PATCH] refactor(blockifier): change return type of invoke_tx deploy_account_tx to account_transaction --- .../src/concurrency/versioned_state_test.rs | 11 ++--- .../src/test_utils/deploy_account.rs | 9 +++- crates/blockifier/src/test_utils/invoke.rs | 8 ++-- .../src/test_utils/transfers_generator.rs | 5 +-- .../src/transaction/account_transaction.rs | 2 +- .../transaction/account_transactions_test.rs | 3 +- .../blockifier/src/transaction/test_utils.rs | 17 +++---- .../src/transaction/transactions_test.rs | 44 ++++++++----------- 8 files changed, 45 insertions(+), 54 deletions(-) diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index 261e175f591..6cc48c8d1a4 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -44,7 +44,6 @@ use crate::test_utils::deploy_account::deploy_account_tx; use crate::test_utils::dict_state_reader::DictStateReader; use crate::test_utils::initial_test_state::test_state; use crate::test_utils::{CairoVersion, BALANCE, DEFAULT_STRK_L1_GAS_PRICE}; -use crate::transaction::account_transaction::AccountTransaction; use crate::transaction::objects::HasRelatedFeeType; use crate::transaction::test_utils::{default_all_resource_bounds, l1_resource_bounds}; use crate::transaction::transactions::ExecutableTransaction; @@ -239,8 +238,7 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { }, &mut NonceManager::default(), ); - let account_tx_1 = AccountTransaction::DeployAccount(deploy_account_tx_1); - let enforce_fee = account_tx_1.enforce_fee(); + let enforce_fee = deploy_account_tx_1.enforce_fee(); let class_hash = grindy_account.get_class_hash(); let ctor_storage_arg = felt!(1_u8); @@ -252,9 +250,8 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { constructor_calldata: constructor_calldata.clone(), }; let nonce_manager = &mut NonceManager::default(); - let deploy_account_tx_2 = deploy_account_tx(deploy_tx_args, nonce_manager); - let account_address = deploy_account_tx_2.contract_address(); - let account_tx_2 = AccountTransaction::DeployAccount(deploy_account_tx_2); + let account_tx_2 = deploy_account_tx(deploy_tx_args, nonce_manager); + let account_address = account_tx_2.sender_address(); let tx_context = block_context.to_tx_context(&account_tx_2); let fee_type = tx_context.tx_info.fee_type(); @@ -270,7 +267,7 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { // Execute transactions thread::scope(|s| { s.spawn(move || { - let result = account_tx_1.execute(&mut state_1, &block_context_1, enforce_fee, true); + let result = deploy_account_tx_1.execute(&mut state_1, &block_context_1, enforce_fee, true); assert_eq!(result.is_err(), enforce_fee); // Transaction fails iff we enforced the fee charge (as the acount is not funded). }); s.spawn(move || { diff --git a/crates/blockifier/src/test_utils/deploy_account.rs b/crates/blockifier/src/test_utils/deploy_account.rs index 092e659c867..5bce5e5504e 100644 --- a/crates/blockifier/src/test_utils/deploy_account.rs +++ b/crates/blockifier/src/test_utils/deploy_account.rs @@ -2,12 +2,13 @@ use starknet_api::core::calculate_contract_address; use starknet_api::test_utils::deploy_account::DeployAccountTxArgs; use starknet_api::test_utils::NonceManager; +use crate::transaction::account_transaction::AccountTransaction; use crate::transaction::transactions::DeployAccountTransaction; pub fn deploy_account_tx( deploy_tx_args: DeployAccountTxArgs, nonce_manager: &mut NonceManager, -) -> DeployAccountTransaction { +) -> AccountTransaction { let tx_hash = deploy_tx_args.tx_hash; let contract_address = calculate_contract_address( deploy_tx_args.contract_address_salt, @@ -21,5 +22,9 @@ pub fn deploy_account_tx( deploy_tx_args, nonce_manager.next(contract_address), ); - DeployAccountTransaction::new(deploy_account_tx, tx_hash, contract_address) + AccountTransaction::DeployAccount(DeployAccountTransaction::new( + deploy_account_tx, + tx_hash, + contract_address, + )) } diff --git a/crates/blockifier/src/test_utils/invoke.rs b/crates/blockifier/src/test_utils/invoke.rs index 65f032d8a20..d4fff4923bb 100644 --- a/crates/blockifier/src/test_utils/invoke.rs +++ b/crates/blockifier/src/test_utils/invoke.rs @@ -2,10 +2,11 @@ use starknet_api::test_utils::invoke::InvokeTxArgs; use starknet_api::transaction::{InvokeTransactionV0, TransactionVersion}; use crate::abi::abi_utils::selector_from_name; +use crate::transaction::account_transaction::AccountTransaction; use crate::transaction::constants::EXECUTE_ENTRY_POINT_NAME; use crate::transaction::transactions::InvokeTransaction; -pub fn invoke_tx(invoke_args: InvokeTxArgs) -> InvokeTransaction { +pub fn invoke_tx(invoke_args: InvokeTxArgs) -> AccountTransaction { let tx_hash = invoke_args.tx_hash; let only_query = invoke_args.only_query; // TODO: Make TransactionVersion an enum and use match here. @@ -22,8 +23,9 @@ pub fn invoke_tx(invoke_args: InvokeTxArgs) -> InvokeTransaction { starknet_api::test_utils::invoke::invoke_tx(invoke_args) }; - match only_query { + let invoke_tx = match only_query { true => InvokeTransaction::new_for_query(invoke_tx, tx_hash), false => InvokeTransaction::new(invoke_tx, tx_hash), - } + }; + AccountTransaction::Invoke(invoke_tx) } diff --git a/crates/blockifier/src/test_utils/transfers_generator.rs b/crates/blockifier/src/test_utils/transfers_generator.rs index 77f26a6c28d..aa14add8892 100644 --- a/crates/blockifier/src/test_utils/transfers_generator.rs +++ b/crates/blockifier/src/test_utils/transfers_generator.rs @@ -179,14 +179,13 @@ impl TransfersGenerator { felt!(0_u8) // Calldata: msb amount. ]; - let tx = invoke_tx(invoke_tx_args! { + invoke_tx(invoke_tx_args! { max_fee: self.config.max_fee, sender_address, calldata: execute_calldata, version: self.config.tx_version, nonce, - }); - AccountTransaction::Invoke(tx) + }) } } diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 76bad0b168a..219a65333c8 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -211,7 +211,7 @@ impl AccountTransaction { // Calldata for validation contains transaction fields that cannot be obtained by calling // `et_tx_info()`. - fn validate_entrypoint_calldata(&self) -> Calldata { + pub fn validate_entrypoint_calldata(&self) -> Calldata { match self { Self::Declare(tx) => calldata![tx.class_hash().0], Self::DeployAccount(tx) => Calldata( diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 3d39a48b547..e5f1082acee 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -182,7 +182,7 @@ fn test_fee_enforcement( ) { let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo0); let state = &mut test_state(&block_context.chain_info, BALANCE, &[(account, 1)]); - let deploy_account_tx = deploy_account_tx( + let account_tx = deploy_account_tx( deploy_account_tx_args! { class_hash: account.get_class_hash(), max_fee: Fee(if zero_bounds { 0 } else { MAX_FEE.0 }), @@ -205,7 +205,6 @@ fn test_fee_enforcement( &mut NonceManager::default(), ); - let account_tx = AccountTransaction::DeployAccount(deploy_account_tx); let enforce_fee = account_tx.create_tx_info().enforce_fee(); assert_ne!(zero_bounds, enforce_fee); let result = account_tx.execute(state, &block_context, enforce_fee, true); diff --git a/crates/blockifier/src/transaction/test_utils.rs b/crates/blockifier/src/transaction/test_utils.rs index e8a8936760f..af43d788e21 100644 --- a/crates/blockifier/src/transaction/test_utils.rs +++ b/crates/blockifier/src/transaction/test_utils.rs @@ -139,8 +139,7 @@ pub fn deploy_and_fund_account( ) -> (AccountTransaction, ContractAddress) { // Deploy an account contract. let deploy_account_tx = deploy_account_tx(deploy_tx_args, nonce_manager); - let account_address = deploy_account_tx.contract_address(); - let account_tx = AccountTransaction::DeployAccount(deploy_account_tx); + let account_address = deploy_account_tx.sender_address(); // Update the balance of the about-to-be deployed account contract in the erc20 contract, so it // can pay for the transaction execution. @@ -153,7 +152,7 @@ pub fn deploy_and_fund_account( .unwrap(); } - (account_tx, account_address) + (deploy_account_tx, account_address) } /// Initializes a state and returns a `TestInitData` instance. @@ -282,7 +281,7 @@ pub fn create_account_tx_for_validate_test( true => constants::FELT_TRUE, false => constants::FELT_FALSE, })]; - let deploy_account_tx = deploy_account_tx( + deploy_account_tx( deploy_account_tx_args! { max_fee, resource_bounds, @@ -293,12 +292,11 @@ pub fn create_account_tx_for_validate_test( constructor_calldata, }, nonce_manager, - ); - AccountTransaction::DeployAccount(deploy_account_tx) + ) } TransactionType::InvokeFunction => { let execute_calldata = create_calldata(sender_address, "foo", &[]); - let invoke_tx = invoke_tx(invoke_tx_args! { + invoke_tx(invoke_tx_args! { max_fee, resource_bounds, signature, @@ -306,15 +304,14 @@ pub fn create_account_tx_for_validate_test( calldata: execute_calldata, version: tx_version, nonce: nonce_manager.next(sender_address), - }); - AccountTransaction::Invoke(invoke_tx) + }) } _ => panic!("{tx_type:?} is not an account transaction."), } } pub fn account_invoke_tx(invoke_args: InvokeTxArgs) -> AccountTransaction { - AccountTransaction::Invoke(invoke_tx(invoke_args)) + invoke_tx(invoke_args) } pub fn run_invoke_tx( diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index ed9ac027def..d67775130a4 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -438,17 +438,17 @@ fn test_invoke_tx( let state = &mut test_state(chain_info, BALANCE, &[(account_contract, 1), (test_contract, 1)]); let test_contract_address = test_contract.get_instance_address(0); let account_contract_address = account_contract.get_instance_address(0); - let invoke_tx = invoke_tx(invoke_tx_args! { + let calldata = create_trivial_calldata(test_contract_address); + let account_tx = invoke_tx(invoke_tx_args! { sender_address: account_contract_address, - calldata: create_trivial_calldata(test_contract_address), + calldata: Calldata(Arc::clone(&calldata.0)), resource_bounds, }); // Extract invoke transaction fields for testing, as it is consumed when creating an account // transaction. - let calldata = Calldata(Arc::clone(&invoke_tx.calldata().0)); - let calldata_length = invoke_tx.calldata().0.len(); - let signature_length = invoke_tx.signature().0.len(); + let calldata_length = account_tx.calldata_length(); + let signature_length = account_tx.signature_length(); let state_changes_for_fee = StateChangesCount { n_storage_updates: 1, n_modified_contracts: 1, @@ -462,9 +462,8 @@ fn test_invoke_tx( None, ExecutionSummary::default(), ); - let sender_address = invoke_tx.sender_address(); + let sender_address = account_tx.sender_address(); - let account_tx = AccountTransaction::Invoke(invoke_tx); let tx_context = block_context.to_tx_context(&account_tx); let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap(); @@ -948,13 +947,13 @@ fn test_max_fee_exceeds_balance( )}; // Deploy. - let invalid_tx = AccountTransaction::DeployAccount(deploy_account_tx( + let invalid_tx = deploy_account_tx( deploy_account_tx_args! { resource_bounds, class_hash: test_contract.get_class_hash() }, &mut NonceManager::default(), - )); + ); assert_resource_bounds_exceed_balance_failure(state, block_context, invalid_tx); // V1 Invoke. @@ -1673,7 +1672,7 @@ fn test_deploy_account_tx( let account = FeatureContract::AccountWithoutValidations(cairo_version); let account_class_hash = account.get_class_hash(); let state = &mut test_state(chain_info, BALANCE, &[(account, 1)]); - let deploy_account = deploy_account_tx( + let account_tx = deploy_account_tx( deploy_account_tx_args! { resource_bounds: default_all_resource_bounds, class_hash: account_class_hash @@ -1683,10 +1682,8 @@ fn test_deploy_account_tx( // Extract deploy account transaction fields for testing, as it is consumed when creating an // account transaction. - let class_hash = deploy_account.class_hash(); - let deployed_account_address = deploy_account.contract_address(); - let constructor_calldata = deploy_account.constructor_calldata(); - let salt = deploy_account.contract_address_salt(); + let class_hash = account_tx.class_hash().unwrap(); + let deployed_account_address = account_tx.sender_address(); let user_initial_gas = user_initial_gas_from_bounds(default_all_resource_bounds); // Update the balance of the about to be deployed account contract in the erc20 contract, so it @@ -1702,20 +1699,18 @@ fn test_deploy_account_tx( .unwrap(); } - let account_tx = AccountTransaction::DeployAccount(deploy_account); 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(); // Build expected validate call info. - let validate_calldata = - [vec![class_hash.0, salt.0], (*constructor_calldata.0).clone()].concat(); + let validate_calldata = account_tx.validate_entrypoint_calldata(); let expected_gas_consumed = 0; let expected_validate_call_info = expected_validate_call_info( account_class_hash, constants::VALIDATE_DEPLOY_ENTRY_POINT_NAME, expected_gas_consumed, - Calldata(validate_calldata.into()), + validate_calldata, deployed_account_address, cairo_version, account @@ -1820,14 +1815,13 @@ fn test_deploy_account_tx( // Negative flow. // Deploy to an existing address. - let deploy_account = deploy_account_tx( + let account_tx = deploy_account_tx( deploy_account_tx_args! { resource_bounds: default_all_resource_bounds, class_hash: account_class_hash }, &mut nonce_manager, ); - let account_tx = AccountTransaction::DeployAccount(deploy_account); let error = account_tx.execute(state, block_context, true, true).unwrap_err(); assert_matches!( error, @@ -1852,25 +1846,24 @@ fn test_fail_deploy_account_undeclared_class_hash( let state = &mut test_state(chain_info, BALANCE, &[]); let mut nonce_manager = NonceManager::default(); let undeclared_hash = class_hash!("0xdeadbeef"); - let deploy_account = deploy_account_tx( + let account_tx = deploy_account_tx( deploy_account_tx_args! { resource_bounds: default_all_resource_bounds, class_hash: undeclared_hash }, &mut nonce_manager, ); - let tx_context = block_context.to_tx_context(&deploy_account); + let tx_context = block_context.to_tx_context(&account_tx); let fee_type = tx_context.tx_info.fee_type(); // Fund account, so as not to fail pre-validation. state .set_storage_at( chain_info.fee_token_address(&fee_type), - get_fee_token_var_address(deploy_account.contract_address()), + get_fee_token_var_address(account_tx.sender_address()), felt!(BALANCE.0), ) .unwrap(); - let account_tx = AccountTransaction::DeployAccount(deploy_account); let error = account_tx.execute(state, block_context, true, true).unwrap_err(); assert_matches!( error, @@ -2198,13 +2191,12 @@ fn test_only_query_flag( .concat() .into(), ); - let invoke_tx = crate::test_utils::invoke::invoke_tx(invoke_tx_args! { + let account_tx = crate::test_utils::invoke::invoke_tx(invoke_tx_args! { calldata: execute_calldata, resource_bounds: default_all_resource_bounds, sender_address, only_query, }); - let account_tx = AccountTransaction::Invoke(invoke_tx); let tx_execution_info = account_tx.execute(state, block_context, true, true).unwrap(); assert_eq!(tx_execution_info.revert_error, None);