From 8d98dc146c8a44eb04714898ce2800eebd2901d0 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 | 9 ++-- .../src/test_utils/deploy_account.rs | 9 +++- crates/blockifier/src/test_utils/invoke.rs | 7 ++-- .../src/test_utils/transfers_generator.rs | 5 +-- .../src/transaction/account_transaction.rs | 12 +++--- .../transaction/account_transactions_test.rs | 3 +- .../blockifier/src/transaction/test_utils.rs | 17 ++++---- .../src/transaction/transactions_test.rs | 42 ++++++++----------- 8 files changed, 48 insertions(+), 56 deletions(-) diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index 261e175f59..62d343605f 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; @@ -229,7 +228,7 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { let mut state_2 = TransactionalState::create_transactional(&mut versioned_state_proxy_2); // Prepare transactions - let deploy_account_tx_1 = deploy_account_tx( + let account_tx_1 = deploy_account_tx( deploy_account_tx_args! { class_hash: account_without_validation.get_class_hash(), resource_bounds: l1_resource_bounds( @@ -239,7 +238,6 @@ 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 class_hash = grindy_account.get_class_hash(); @@ -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(); diff --git a/crates/blockifier/src/test_utils/deploy_account.rs b/crates/blockifier/src/test_utils/deploy_account.rs index 092e659c86..5bce5e5504 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 65f032d8a2..73ffe5ffa5 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. @@ -23,7 +24,7 @@ pub fn invoke_tx(invoke_args: InvokeTxArgs) -> InvokeTransaction { }; match only_query { - true => InvokeTransaction::new_for_query(invoke_tx, tx_hash), - false => InvokeTransaction::new(invoke_tx, tx_hash), + true => AccountTransaction::Invoke(InvokeTransaction::new_for_query(invoke_tx, tx_hash)), + false => AccountTransaction::Invoke(InvokeTransaction::new(invoke_tx, tx_hash)), } } diff --git a/crates/blockifier/src/test_utils/transfers_generator.rs b/crates/blockifier/src/test_utils/transfers_generator.rs index 77f26a6c28..aa14add889 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 76bad0b168..03b21a2009 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( @@ -228,13 +228,15 @@ impl AccountTransaction { } pub fn calldata_length(&self) -> usize { - let calldata = match self { + self.calldata().0.len() + } + + pub fn calldata(&self) -> Calldata { + match self { Self::Declare(_tx) => calldata![], Self::DeployAccount(tx) => tx.constructor_calldata(), Self::Invoke(tx) => tx.calldata(), - }; - - calldata.0.len() + } } pub fn signature_length(&self) -> usize { diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 3d39a48b54..e5f1082ace 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 e8a8936760..17a158253b 100644 --- a/crates/blockifier/src/transaction/test_utils.rs +++ b/crates/blockifier/src/transaction/test_utils.rs @@ -138,9 +138,8 @@ pub fn deploy_and_fund_account( deploy_tx_args: DeployAccountTxArgs, ) -> (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_tx = deploy_account_tx(deploy_tx_args, nonce_manager); + let account_address = 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. @@ -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 ed9ac027de..76ce6f9b36 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -438,7 +438,7 @@ 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 account_tx = invoke_tx(invoke_tx_args! { sender_address: account_contract_address, calldata: create_trivial_calldata(test_contract_address), resource_bounds, @@ -446,9 +446,9 @@ fn test_invoke_tx( // 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 = Calldata(Arc::clone(&account_tx.calldata().0)); //Aviv how to access? + 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);