From f38447ad5f15f761d34e99d34123819276923f7f Mon Sep 17 00:00:00 2001 From: Arni Hod Date: Thu, 4 Apr 2024 10:48:15 +0300 Subject: [PATCH] feat: stateless validator add non zero fee check --- Cargo.toml | 2 + crates/gateway/Cargo.toml | 1 + crates/gateway/src/errors.rs | 12 +++- crates/gateway/src/starknet_api_test_utils.rs | 52 +++++++++++++-- crates/gateway/src/transaction_validator.rs | 62 ++++++++++++++++-- .../gateway/src/transaction_validator_test.rs | 64 +++++++++++++++++-- 6 files changed, 175 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7a1a2dcd..ce5d2603 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,8 @@ pretty_assertions = "1.4.0" rstest = "0.17.0" serde = { version = "1.0.193", features = ["derive"] } serde_json = "1.0" +# TODO(Arni, 1/5/2024): Use a fixed version once the StarkNet API is stable. starknet_api = { git = "https://github.com/starkware-libs/starknet-api.git", rev = "51bc38a" } +strum = "0.24.1" thiserror = "1.0" tokio = { version = "1", features = ["full"] } diff --git a/crates/gateway/Cargo.toml b/crates/gateway/Cargo.toml index 99a3d5e2..7f976689 100644 --- a/crates/gateway/Cargo.toml +++ b/crates/gateway/Cargo.toml @@ -14,6 +14,7 @@ hyper.workspace = true serde.workspace = true serde_json.workspace = true starknet_api.workspace = true +strum.workspace = true thiserror.workspace = true [dev-dependencies] diff --git a/crates/gateway/src/errors.rs b/crates/gateway/src/errors.rs index 1c301dde..237e9ea2 100644 --- a/crates/gateway/src/errors.rs +++ b/crates/gateway/src/errors.rs @@ -1,3 +1,5 @@ +use starknet_api::transaction::{Resource, ResourceBounds}; + use thiserror::Error; #[derive(Debug, Error)] @@ -22,6 +24,14 @@ pub enum GatewayConfigError { #[derive(Debug, Error)] #[cfg_attr(test, derive(PartialEq))] -pub enum TransactionValidatorError {} +pub enum TransactionValidatorError { + #[error("Expected a positive amount of {resource:?}. Got {resource_bounds:?}.")] + ZeroFee { + resource: Resource, + resource_bounds: ResourceBounds, + }, + #[error("The resource bounds mapping is missing a resource {resource:?}.")] + MissingResource { resource: Resource }, +} pub type TransactionValidatorResult = Result; diff --git a/crates/gateway/src/starknet_api_test_utils.rs b/crates/gateway/src/starknet_api_test_utils.rs index 7834a078..6d1975ad 100644 --- a/crates/gateway/src/starknet_api_test_utils.rs +++ b/crates/gateway/src/starknet_api_test_utils.rs @@ -7,10 +7,12 @@ use starknet_api::external_transaction::{ use starknet_api::transaction::{ResourceBounds, ResourceBoundsMapping}; // Utils. -pub fn create_external_declare_tx_for_testing() -> ExternalTransaction { +pub fn create_external_declare_tx_for_testing( + resource_bounds: ResourceBoundsMapping, +) -> ExternalTransaction { ExternalTransaction::Declare(ExternalDeclareTransaction::V3( ExternalDeclareTransactionV3 { - resource_bounds: zero_resource_bounds_mapping(), + resource_bounds, contract_class: Default::default(), tip: Default::default(), signature: Default::default(), @@ -25,10 +27,12 @@ pub fn create_external_declare_tx_for_testing() -> ExternalTransaction { )) } -pub fn create_external_deploy_account_tx_for_testing() -> ExternalTransaction { +pub fn create_external_deploy_account_tx_for_testing( + resource_bounds: ResourceBoundsMapping, +) -> ExternalTransaction { ExternalTransaction::DeployAccount(ExternalDeployAccountTransaction::V3( ExternalDeployAccountTransactionV3 { - resource_bounds: zero_resource_bounds_mapping(), + resource_bounds, tip: Default::default(), contract_address_salt: Default::default(), class_hash: Default::default(), @@ -42,9 +46,11 @@ pub fn create_external_deploy_account_tx_for_testing() -> ExternalTransaction { )) } -pub fn create_external_invoke_tx_for_testing() -> ExternalTransaction { +pub fn create_external_invoke_tx_for_testing( + resource_bounds: ResourceBoundsMapping, +) -> ExternalTransaction { ExternalTransaction::Invoke(ExternalInvokeTransaction::V3(ExternalInvokeTransactionV3 { - resource_bounds: zero_resource_bounds_mapping(), + resource_bounds, tip: Default::default(), signature: Default::default(), nonce: Default::default(), @@ -70,3 +76,37 @@ pub fn zero_resource_bounds_mapping() -> ResourceBoundsMapping { ]) .expect("Resource bounds mapping has unexpected structure.") } + +pub fn non_zero_l1_resource_bounds_mapping() -> ResourceBoundsMapping { + ResourceBoundsMapping::try_from(vec![ + ( + starknet_api::transaction::Resource::L1Gas, + ResourceBounds { + max_amount: 1, + max_price_per_unit: 1, + }, + ), + ( + starknet_api::transaction::Resource::L2Gas, + ResourceBounds::default(), + ), + ]) + .expect("Resource bounds mapping has unexpected structure.") +} + +pub fn non_zero_l2_resource_bounds_mapping() -> ResourceBoundsMapping { + ResourceBoundsMapping::try_from(vec![ + ( + starknet_api::transaction::Resource::L1Gas, + ResourceBounds::default(), + ), + ( + starknet_api::transaction::Resource::L2Gas, + ResourceBounds { + max_amount: 1, + max_price_per_unit: 1, + }, + ), + ]) + .expect("Resource bounds mapping has unexpected structure.") +} diff --git a/crates/gateway/src/transaction_validator.rs b/crates/gateway/src/transaction_validator.rs index bcbf8f35..6afa7f0a 100644 --- a/crates/gateway/src/transaction_validator.rs +++ b/crates/gateway/src/transaction_validator.rs @@ -1,24 +1,76 @@ -use starknet_api::external_transaction::ExternalTransaction; +use strum::IntoEnumIterator; -use crate::errors::TransactionValidatorResult; +use starknet_api::external_transaction::{ + ExternalDeclareTransaction, ExternalDeployAccountTransaction, ExternalInvokeTransaction, + ExternalTransaction, +}; +use starknet_api::transaction::Resource; + +use crate::errors::{TransactionValidatorError, TransactionValidatorResult}; #[cfg(test)] #[path = "transaction_validator_test.rs"] mod transaction_validator_test; -pub struct TransactionValidatorConfig {} +#[derive(Default)] +pub struct TransactionValidatorConfig { + // TODO(Arni, 1/5/2024): Consider squashing this config into a single field. Is it possible to + // use more than one gas for fee in a single flow? + // If true, validates that the reousrce bounds are not zero. + pub validate_non_zero_l1_gas_fee: bool, + pub validate_non_zero_l2_gas_fee: bool, +} pub struct TransactionValidator { pub config: TransactionValidatorConfig, } impl TransactionValidator { - pub fn validate(&self, _tx: ExternalTransaction) -> TransactionValidatorResult<()> { + pub fn validate(&self, tx: ExternalTransaction) -> TransactionValidatorResult<()> { // TODO(Arni, 1/5/2024): Add a mechanism that validate the sender address is not blocked. // TODO(Arni, 1/5/2024): Validate transaction version. - // TODO(Arni, 4/4/2024): Validate fee non zero. // TODO(Arni, 4/4/2024): Validate tx signature and calldata are not too long. + self.validate_fee(&tx)?; + + Ok(()) + } + + fn validate_fee(&self, tx: &ExternalTransaction) -> TransactionValidatorResult<()> { + let resource_bounds_mapping = match tx { + ExternalTransaction::Declare(tx) => match tx { + ExternalDeclareTransaction::V3(tx) => &tx.resource_bounds, + }, + ExternalTransaction::DeployAccount(tx) => match tx { + ExternalDeployAccountTransaction::V3(tx) => &tx.resource_bounds, + }, + ExternalTransaction::Invoke(tx) => match tx { + ExternalInvokeTransaction::V3(tx) => &tx.resource_bounds, + }, + }; + + for resource in Resource::iter() { + if (resource == Resource::L1Gas && !self.config.validate_non_zero_l1_gas_fee) + || (resource == Resource::L2Gas && !self.config.validate_non_zero_l2_gas_fee) + { + continue; + } + let resource_bounds = resource_bounds_mapping.0.get(&resource); + match resource_bounds { + None => { + return Err(TransactionValidatorError::MissingResource { resource }); + } + Some(resource_bounds) => { + if resource_bounds.max_amount == 0 || resource_bounds.max_price_per_unit == 0 { + return Err(TransactionValidatorError::ZeroFee { + resource, + resource_bounds: *resource_bounds, + }); + } + } + } + } + Ok(()) } } diff --git a/crates/gateway/src/transaction_validator_test.rs b/crates/gateway/src/transaction_validator_test.rs index b891fb1f..7758ad45 100644 --- a/crates/gateway/src/transaction_validator_test.rs +++ b/crates/gateway/src/transaction_validator_test.rs @@ -1,31 +1,83 @@ use rstest::rstest; use starknet_api::external_transaction::ExternalTransaction; +use starknet_api::transaction::{Resource, ResourceBounds, ResourceBoundsMapping}; use crate::starknet_api_test_utils::{ create_external_declare_tx_for_testing, create_external_deploy_account_tx_for_testing, - create_external_invoke_tx_for_testing, + create_external_invoke_tx_for_testing, non_zero_l1_resource_bounds_mapping, + non_zero_l2_resource_bounds_mapping, zero_resource_bounds_mapping, }; use crate::transaction_validator::{ - TransactionValidator, TransactionValidatorConfig, TransactionValidatorResult, + TransactionValidator, TransactionValidatorConfig, TransactionValidatorError, + TransactionValidatorResult, }; -const VALIDATOR_CONFIG_FOR_TESTING: TransactionValidatorConfig = TransactionValidatorConfig {}; +const VALIDATOR_CONFIG_FOR_TESTING: TransactionValidatorConfig = TransactionValidatorConfig { + validate_non_zero_l1_gas_fee: true, + validate_non_zero_l2_gas_fee: false, +}; #[rstest] +#[case::ignore_resource_bounds( + TransactionValidatorConfig{ + validate_non_zero_l1_gas_fee: false, + validate_non_zero_l2_gas_fee: false, + }, + create_external_invoke_tx_for_testing(zero_resource_bounds_mapping()), + Ok(()) +)] +#[case::malformed_resource_bounds( + VALIDATOR_CONFIG_FOR_TESTING, + create_external_invoke_tx_for_testing(ResourceBoundsMapping::default()), + Err(TransactionValidatorError::MissingResource { resource: Resource::L1Gas }) +)] +#[case::malformed_l2_gas_resource_bounds( + TransactionValidatorConfig{ + validate_non_zero_l1_gas_fee: false, + validate_non_zero_l2_gas_fee: true, + }, + create_external_invoke_tx_for_testing(ResourceBoundsMapping::default()), + Err(TransactionValidatorError::MissingResource { resource: Resource::L2Gas }) +)] +#[case::invalid_resource_bounds( + VALIDATOR_CONFIG_FOR_TESTING, + create_external_invoke_tx_for_testing(zero_resource_bounds_mapping()), + Err(TransactionValidatorError::ZeroFee{ + resource: Resource::L1Gas, resource_bounds: ResourceBounds::default() + }) +)] +#[case::invalid_l2_gas_resource_bounds( + TransactionValidatorConfig{ + validate_non_zero_l1_gas_fee: false, + validate_non_zero_l2_gas_fee: true, + }, + create_external_invoke_tx_for_testing(non_zero_l1_resource_bounds_mapping()), + Err(TransactionValidatorError::ZeroFee{ + resource: Resource::L2Gas, resource_bounds: ResourceBounds::default() + }) +)] #[case::valid_declare_tx( VALIDATOR_CONFIG_FOR_TESTING, - create_external_declare_tx_for_testing(), + create_external_declare_tx_for_testing(non_zero_l1_resource_bounds_mapping()), Ok(()) )] #[case::valid_deploy_account_tx( VALIDATOR_CONFIG_FOR_TESTING, - create_external_deploy_account_tx_for_testing(), + create_external_deploy_account_tx_for_testing(non_zero_l1_resource_bounds_mapping(),), Ok(()) )] #[case::valid_invoke_tx( VALIDATOR_CONFIG_FOR_TESTING, - create_external_invoke_tx_for_testing(), + create_external_invoke_tx_for_testing(non_zero_l1_resource_bounds_mapping()), + Ok(()) +)] +#[case::valid_l2_gas_invoke_tx( + TransactionValidatorConfig{ + validate_non_zero_l1_gas_fee: false, + validate_non_zero_l2_gas_fee: true, + }, + create_external_invoke_tx_for_testing(non_zero_l2_resource_bounds_mapping()), Ok(()) )] fn test_transaction_validator(