From 49b37f8488b1ac1f86e88d21ff6a677cb822293c Mon Sep 17 00:00:00 2001 From: Nimrod Weiss Date: Sun, 25 Aug 2024 12:08:36 +0300 Subject: [PATCH] refactor(fee): remove resource mapping usage in rpc and adjust to use all resources --- .../src/stateless_transaction_validator.rs | 11 ++---- .../stateless_transaction_validator_test.rs | 38 +++++++++++++++---- .../src/starknet_api_test_utils.rs | 32 +++++++++++----- crates/starknet_api/src/rpc_transaction.rs | 31 ++++++++------- .../starknet_api/src/rpc_transaction_test.rs | 7 ++-- crates/starknet_api/src/transaction.rs | 4 +- 6 files changed, 76 insertions(+), 47 deletions(-) diff --git a/crates/gateway/src/stateless_transaction_validator.rs b/crates/gateway/src/stateless_transaction_validator.rs index b663bf5852..0a60bf2e06 100644 --- a/crates/gateway/src/stateless_transaction_validator.rs +++ b/crates/gateway/src/stateless_transaction_validator.rs @@ -1,12 +1,11 @@ use starknet_api::rpc_transaction::{ - ResourceBoundsMapping, RpcDeclareTransaction, RpcDeployAccountTransaction, RpcInvokeTransaction, RpcTransaction, }; use starknet_api::state::EntryPoint; -use starknet_api::transaction::Resource; +use starknet_api::transaction::{AllResourceBounds, Resource}; use starknet_types_core::felt::Felt; use tracing::{instrument, Level}; @@ -176,14 +175,10 @@ impl StatelessTransactionValidator { } fn validate_resource_is_non_zero( - resource_bounds_mapping: &ResourceBoundsMapping, + all_resource_bounds: &AllResourceBounds, resource: Resource, ) -> StatelessTransactionValidatorResult<()> { - let resource_bounds = match resource { - Resource::L1Gas => resource_bounds_mapping.l1_gas, - Resource::L2Gas => resource_bounds_mapping.l2_gas, - Resource::L1DataGas => todo!(), - }; + let resource_bounds = all_resource_bounds.get_bound(resource); if resource_bounds.max_amount == 0 || resource_bounds.max_price_per_unit == 0 { return Err(StatelessTransactionValidatorError::ZeroResourceBounds { resource, diff --git a/crates/gateway/src/stateless_transaction_validator_test.rs b/crates/gateway/src/stateless_transaction_validator_test.rs index 880706615e..0a7440b964 100644 --- a/crates/gateway/src/stateless_transaction_validator_test.rs +++ b/crates/gateway/src/stateless_transaction_validator_test.rs @@ -13,9 +13,15 @@ use mempool_test_utils::starknet_api_test_utils::{ }; use rstest::rstest; use starknet_api::core::EntryPointSelector; -use starknet_api::rpc_transaction::{ContractClass, EntryPointByType, ResourceBoundsMapping}; +use starknet_api::rpc_transaction::{ContractClass, EntryPointByType}; use starknet_api::state::EntryPoint; -use starknet_api::transaction::{Calldata, Resource, ResourceBounds, TransactionSignature}; +use starknet_api::transaction::{ + AllResourceBounds, + Calldata, + Resource, + ResourceBounds, + TransactionSignature, +}; use starknet_api::{calldata, felt}; use starknet_types_core::felt::Felt; @@ -67,7 +73,11 @@ fn default_validator_config_for_testing() -> &'static StatelessTransactionValida validate_non_zero_l2_gas_fee: false, ..default_validator_config_for_testing().clone() }, - create_resource_bounds_mapping(NON_EMPTY_RESOURCE_BOUNDS, ResourceBounds::default()), + create_resource_bounds_mapping( + NON_EMPTY_RESOURCE_BOUNDS, + ResourceBounds::default(), + ResourceBounds::default(), + ), calldata![], TransactionSignature::default() )] @@ -77,7 +87,11 @@ fn default_validator_config_for_testing() -> &'static StatelessTransactionValida validate_non_zero_l2_gas_fee: true, ..default_validator_config_for_testing().clone() }, - create_resource_bounds_mapping(ResourceBounds::default(), NON_EMPTY_RESOURCE_BOUNDS), + create_resource_bounds_mapping( + ResourceBounds::default(), + NON_EMPTY_RESOURCE_BOUNDS, + ResourceBounds::default(), + ), calldata![], TransactionSignature::default() )] @@ -87,7 +101,11 @@ fn default_validator_config_for_testing() -> &'static StatelessTransactionValida validate_non_zero_l2_gas_fee: true, ..default_validator_config_for_testing().clone() }, - create_resource_bounds_mapping(NON_EMPTY_RESOURCE_BOUNDS, NON_EMPTY_RESOURCE_BOUNDS), + create_resource_bounds_mapping( + NON_EMPTY_RESOURCE_BOUNDS, + NON_EMPTY_RESOURCE_BOUNDS, + ResourceBounds::default(), + ), calldata![], TransactionSignature::default() )] @@ -111,7 +129,7 @@ fn default_validator_config_for_testing() -> &'static StatelessTransactionValida )] fn test_positive_flow( #[case] config: StatelessTransactionValidatorConfig, - #[case] resource_bounds: ResourceBoundsMapping, + #[case] resource_bounds: AllResourceBounds, #[case] tx_calldata: Calldata, #[case] signature: TransactionSignature, #[values(TransactionType::Declare, TransactionType::DeployAccount, TransactionType::Invoke)] @@ -141,14 +159,18 @@ fn test_positive_flow( validate_non_zero_l2_gas_fee: true, ..default_validator_config_for_testing().clone() }, - create_resource_bounds_mapping(NON_EMPTY_RESOURCE_BOUNDS, ResourceBounds::default()), + create_resource_bounds_mapping( + NON_EMPTY_RESOURCE_BOUNDS, + ResourceBounds::default(), + ResourceBounds::default(), + ), StatelessTransactionValidatorError::ZeroResourceBounds{ resource: Resource::L2Gas, resource_bounds: ResourceBounds::default() } )] fn test_invalid_resource_bounds( #[case] config: StatelessTransactionValidatorConfig, - #[case] resource_bounds: ResourceBoundsMapping, + #[case] resource_bounds: AllResourceBounds, #[case] expected_error: StatelessTransactionValidatorError, #[values(TransactionType::Declare, TransactionType::DeployAccount, TransactionType::Invoke)] tx_type: TransactionType, diff --git a/crates/mempool_test_utils/src/starknet_api_test_utils.rs b/crates/mempool_test_utils/src/starknet_api_test_utils.rs index 1688bc4111..8ab10d890d 100644 --- a/crates/mempool_test_utils/src/starknet_api_test_utils.rs +++ b/crates/mempool_test_utils/src/starknet_api_test_utils.rs @@ -14,7 +14,6 @@ use starknet_api::data_availability::DataAvailabilityMode; use starknet_api::executable_transaction::{InvokeTransaction, Transaction}; use starknet_api::rpc_transaction::{ ContractClass, - ResourceBoundsMapping, RpcDeclareTransactionV3, RpcDeployAccountTransactionV3, RpcInvokeTransactionV3, @@ -22,6 +21,7 @@ use starknet_api::rpc_transaction::{ }; use starknet_api::transaction::{ AccountDeploymentData, + AllResourceBounds, Calldata, ContractAddressSalt, DeprecatedResourceBoundsMapping as ExecutableResourceBoundsMapping, @@ -49,6 +49,8 @@ pub const VALID_L1_GAS_MAX_AMOUNT: u64 = 203484; pub const VALID_L1_GAS_MAX_PRICE_PER_UNIT: u128 = 100000000000; pub const VALID_L2_GAS_MAX_AMOUNT: u64 = 203484; pub const VALID_L2_GAS_MAX_PRICE_PER_UNIT: u128 = 100000000000; +pub const VALID_L1_DATA_GAS_MAX_AMOUNT: u64 = 203484; +pub const VALID_L1_DATA_GAS_MAX_PRICE_PER_UNIT: u128 = 100000000000; pub const TEST_SENDER_ADDRESS: u128 = 0x1000; // Utils. @@ -60,7 +62,7 @@ pub enum TransactionType { pub fn rpc_tx_for_testing( tx_type: TransactionType, - resource_bounds: ResourceBoundsMapping, + resource_bounds: AllResourceBounds, calldata: Calldata, signature: TransactionSignature, ) -> RpcTransaction { @@ -96,18 +98,24 @@ pub fn rpc_tx_for_testing( pub const NON_EMPTY_RESOURCE_BOUNDS: ResourceBounds = ResourceBounds { max_amount: 1, max_price_per_unit: 1 }; +// TODO(Nimrod): Delete this function. pub fn create_resource_bounds_mapping( l1_resource_bounds: ResourceBounds, l2_resource_bounds: ResourceBounds, -) -> ResourceBoundsMapping { - ResourceBoundsMapping { l1_gas: l1_resource_bounds, l2_gas: l2_resource_bounds } + l1_data_resource_bounds: ResourceBounds, +) -> AllResourceBounds { + AllResourceBounds { + l1_gas: l1_resource_bounds, + l2_gas: l2_resource_bounds, + l1_data_gas: l1_data_resource_bounds, + } } -pub fn zero_resource_bounds_mapping() -> ResourceBoundsMapping { - create_resource_bounds_mapping(ResourceBounds::default(), ResourceBounds::default()) +pub fn zero_resource_bounds_mapping() -> AllResourceBounds { + AllResourceBounds::default() } -pub fn test_resource_bounds_mapping() -> ResourceBoundsMapping { +pub fn test_resource_bounds_mapping() -> AllResourceBounds { create_resource_bounds_mapping( ResourceBounds { max_amount: VALID_L1_GAS_MAX_AMOUNT, @@ -117,6 +125,10 @@ pub fn test_resource_bounds_mapping() -> ResourceBoundsMapping { max_amount: VALID_L2_GAS_MAX_AMOUNT, max_price_per_unit: VALID_L2_GAS_MAX_PRICE_PER_UNIT, }, + ResourceBounds { + max_amount: VALID_L1_DATA_GAS_MAX_AMOUNT, + max_price_per_unit: VALID_L1_DATA_GAS_MAX_PRICE_PER_UNIT, + }, ) } @@ -369,7 +381,7 @@ pub struct InvokeTxArgs { pub sender_address: ContractAddress, pub calldata: Calldata, pub version: TransactionVersion, - pub resource_bounds: ResourceBoundsMapping, + pub resource_bounds: AllResourceBounds, pub tip: Tip, pub nonce_data_availability_mode: DataAvailabilityMode, pub fee_data_availability_mode: DataAvailabilityMode, @@ -400,7 +412,7 @@ impl Default for InvokeTxArgs { pub struct DeployAccountTxArgs { pub signature: TransactionSignature, pub version: TransactionVersion, - pub resource_bounds: ResourceBoundsMapping, + pub resource_bounds: AllResourceBounds, pub tip: Tip, pub nonce_data_availability_mode: DataAvailabilityMode, pub fee_data_availability_mode: DataAvailabilityMode, @@ -434,7 +446,7 @@ pub struct DeclareTxArgs { pub signature: TransactionSignature, pub sender_address: ContractAddress, pub version: TransactionVersion, - pub resource_bounds: ResourceBoundsMapping, + pub resource_bounds: AllResourceBounds, pub tip: Tip, pub nonce_data_availability_mode: DataAvailabilityMode, pub fee_data_availability_mode: DataAvailabilityMode, diff --git a/crates/starknet_api/src/rpc_transaction.rs b/crates/starknet_api/src/rpc_transaction.rs index 2c402c2d9b..046fc7cb1f 100644 --- a/crates/starknet_api/src/rpc_transaction.rs +++ b/crates/starknet_api/src/rpc_transaction.rs @@ -18,11 +18,11 @@ use crate::data_availability::DataAvailabilityMode; use crate::state::EntryPoint; use crate::transaction::{ AccountDeploymentData, + AllResourceBounds, Calldata, ContractAddressSalt, PaymasterData, Resource, - ResourceBounds, Tip, TransactionSignature, }; @@ -63,7 +63,7 @@ macro_rules! implement_ref_getters { impl RpcTransaction { implement_ref_getters!( (nonce, Nonce), - (resource_bounds, ResourceBoundsMapping), + (resource_bounds, AllResourceBounds), (signature, TransactionSignature), (tip, Tip) ); @@ -135,7 +135,7 @@ pub struct RpcDeclareTransactionV3 { pub signature: TransactionSignature, pub nonce: Nonce, pub contract_class: ContractClass, - pub resource_bounds: ResourceBoundsMapping, + pub resource_bounds: AllResourceBounds, pub tip: Tip, pub paymaster_data: PaymasterData, pub account_deployment_data: AccountDeploymentData, @@ -151,7 +151,7 @@ pub struct RpcDeployAccountTransactionV3 { pub class_hash: ClassHash, pub contract_address_salt: ContractAddressSalt, pub constructor_calldata: Calldata, - pub resource_bounds: ResourceBoundsMapping, + pub resource_bounds: AllResourceBounds, pub tip: Tip, pub paymaster_data: PaymasterData, pub nonce_data_availability_mode: DataAvailabilityMode, @@ -165,7 +165,7 @@ pub struct RpcInvokeTransactionV3 { pub calldata: Calldata, pub signature: TransactionSignature, pub nonce: Nonce, - pub resource_bounds: ResourceBoundsMapping, + pub resource_bounds: AllResourceBounds, pub tip: Tip, pub paymaster_data: PaymasterData, pub account_deployment_data: AccountDeploymentData, @@ -192,17 +192,16 @@ pub struct EntryPointByType { pub l1handler: Vec, } -// The serialization of the struct in transaction is in capital letters, not following the spec. -#[derive(Clone, Debug, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] -pub struct ResourceBoundsMapping { - pub l1_gas: ResourceBounds, - pub l2_gas: ResourceBounds, -} - -impl From for crate::transaction::DeprecatedResourceBoundsMapping { - fn from(mapping: ResourceBoundsMapping) -> crate::transaction::DeprecatedResourceBoundsMapping { - let map = - BTreeMap::from([(Resource::L1Gas, mapping.l1_gas), (Resource::L2Gas, mapping.l2_gas)]); +// TODO(Nimrod): Remove this conversion. +impl From for crate::transaction::DeprecatedResourceBoundsMapping { + fn from( + all_resource_bounds: AllResourceBounds, + ) -> crate::transaction::DeprecatedResourceBoundsMapping { + let map = BTreeMap::from([ + (Resource::L1Gas, all_resource_bounds.l1_gas), + (Resource::L2Gas, all_resource_bounds.l2_gas), + (Resource::L1DataGas, all_resource_bounds.l1_data_gas), + ]); crate::transaction::DeprecatedResourceBoundsMapping(map) } } diff --git a/crates/starknet_api/src/rpc_transaction_test.rs b/crates/starknet_api/src/rpc_transaction_test.rs index e2fc6de4d9..92777b53ec 100644 --- a/crates/starknet_api/src/rpc_transaction_test.rs +++ b/crates/starknet_api/src/rpc_transaction_test.rs @@ -7,7 +7,6 @@ use crate::core::{ClassHash, CompiledClassHash, ContractAddress, Nonce, Patricia use crate::rpc_transaction::{ ContractClass, DataAvailabilityMode, - ResourceBoundsMapping, RpcDeclareTransaction, RpcDeclareTransactionV3, RpcDeployAccountTransaction, @@ -18,6 +17,7 @@ use crate::rpc_transaction::{ }; use crate::transaction::{ AccountDeploymentData, + AllResourceBounds, Calldata, ContractAddressSalt, PaymasterData, @@ -27,10 +27,11 @@ use crate::transaction::{ }; use crate::{contract_address, felt, patricia_key}; -fn create_resource_bounds_for_testing() -> ResourceBoundsMapping { - ResourceBoundsMapping { +fn create_resource_bounds_for_testing() -> AllResourceBounds { + AllResourceBounds { l1_gas: ResourceBounds { max_amount: 100, max_price_per_unit: 12 }, l2_gas: ResourceBounds { max_amount: 58, max_price_per_unit: 31 }, + l1_data_gas: ResourceBounds { max_amount: 66, max_price_per_unit: 25 }, } } diff --git a/crates/starknet_api/src/transaction.rs b/crates/starknet_api/src/transaction.rs index 5aa8cdcb08..807849436a 100644 --- a/crates/starknet_api/src/transaction.rs +++ b/crates/starknet_api/src/transaction.rs @@ -963,6 +963,7 @@ pub enum ValidResourceBounds { AllResources(AllResourceBounds), } +#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize)] pub struct AllResourceBounds { pub l1_gas: ResourceBounds, pub l2_gas: ResourceBounds, @@ -970,8 +971,7 @@ pub struct AllResourceBounds { } impl AllResourceBounds { - #[allow(dead_code)] - fn get_bound(&self, resource: Resource) -> ResourceBounds { + pub fn get_bound(&self, resource: Resource) -> ResourceBounds { match resource { Resource::L1Gas => self.l1_gas, Resource::L2Gas => self.l2_gas,