Skip to content

Commit

Permalink
feat(starknet_api): resource bounds struct now has typed fields (#1180)
Browse files Browse the repository at this point in the history
<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/starkware-libs/sequencer/1180)
<!-- Reviewable:end -->
  • Loading branch information
dorimedini-starkware authored Oct 9, 2024
1 parent 25949ac commit 0a090fa
Show file tree
Hide file tree
Showing 27 changed files with 192 additions and 138 deletions.
2 changes: 1 addition & 1 deletion crates/blockifier/src/execution/entry_point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ impl EntryPointExecutionContext {
TransactionInfo::Deprecated(context) => context.max_fee.saturating_div(
block_info.gas_prices.get_l1_gas_price_by_fee_type(&tx_info.fee_type()),
),
TransactionInfo::Current(context) => context.l1_resource_bounds().max_amount.into(),
TransactionInfo::Current(context) => context.l1_resource_bounds().max_amount,
};

// Use saturating upper bound to avoid overflow. This is safe because the upper bound is
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use cairo_vm::Felt252;
use num_traits::Pow;
use starknet_api::block::GasPrice;
use starknet_api::core::ChainId;
use starknet_api::data_availability::DataAvailabilityMode;
use starknet_api::execution_resources::GasAmount;
use starknet_api::transaction::{
AccountDeploymentData,
Calldata,
Expand Down Expand Up @@ -143,8 +145,8 @@ fn test_get_execution_info(
let nonce = nonce!(3_u16);
let sender_address = test_contract_address;

let max_amount = Fee(13);
let max_price_per_unit = Fee(61);
let max_amount = GasAmount(13);
let max_price_per_unit = GasPrice(61);

let expected_resource_bounds: Vec<Felt> = match (test_contract, version) {
(FeatureContract::LegacyTestContract, _) => vec![],
Expand All @@ -154,8 +156,8 @@ fn test_get_execution_info(
(_, _) => vec![
Felt::from(2u32), // Length of ResourceBounds array.
felt!(Resource::L1Gas.to_hex()), // Resource.
felt!(max_amount.0), // Max amount.
felt!(max_price_per_unit.0), // Max price per unit.
max_amount.into(), // Max amount.
max_price_per_unit.into(), // Max price per unit.
felt!(Resource::L2Gas.to_hex()), // Resource.
Felt::ZERO, // Max amount.
Felt::ZERO, // Max price per unit.
Expand Down Expand Up @@ -209,8 +211,8 @@ fn test_get_execution_info(
..Default::default()
},
resource_bounds: ValidResourceBounds::L1Gas(ResourceBounds {
max_amount: max_amount.0.try_into().expect("Failed to convert u128 to u64."),
max_price_per_unit: max_price_per_unit.0,
max_amount,
max_price_per_unit,
}),
tip: Tip::default(),
nonce_data_availability_mode: DataAvailabilityMode::L1,
Expand Down
12 changes: 6 additions & 6 deletions crates/blockifier/src/fee/fee_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl FeeCheckReport {
match &tx_context.tx_info {
TransactionInfo::Current(info) => get_fee_by_gas_vector(
&tx_context.block_context.block_info,
GasVector::from_l1_gas(info.l1_resource_bounds().max_amount.into()),
GasVector::from_l1_gas(info.l1_resource_bounds().max_amount),
&FeeType::Strk,
),
TransactionInfo::Deprecated(context) => context.max_fee,
Expand Down Expand Up @@ -152,9 +152,9 @@ impl FeeCheckReport {
gas_vector: &GasVector,
) -> FeeCheckResult<()> {
for (resource, max_amount, actual_amount) in [
(L1Gas, all_resource_bounds.l1_gas.max_amount.into(), gas_vector.l1_gas),
(L2Gas, all_resource_bounds.l2_gas.max_amount.into(), gas_vector.l2_gas),
(L1DataGas, all_resource_bounds.l1_data_gas.max_amount.into(), gas_vector.l1_data_gas),
(L1Gas, all_resource_bounds.l1_gas.max_amount, gas_vector.l1_gas),
(L2Gas, all_resource_bounds.l2_gas.max_amount, gas_vector.l2_gas),
(L1DataGas, all_resource_bounds.l1_data_gas.max_amount, gas_vector.l1_data_gas),
] {
if max_amount < actual_amount {
return Err(FeeCheckError::MaxGasAmountExceeded {
Expand All @@ -175,10 +175,10 @@ impl FeeCheckReport {
) -> FeeCheckResult<()> {
let total_discounted_gas_used =
gas_vector.to_discounted_l1_gas(tx_context.get_gas_prices());
if total_discounted_gas_used > l1_bounds.max_amount.into() {
if total_discounted_gas_used > l1_bounds.max_amount {
return Err(FeeCheckError::MaxGasAmountExceeded {
resource: L1Gas,
max_amount: l1_bounds.max_amount.into(),
max_amount: l1_bounds.max_amount,
actual_amount: total_discounted_gas_used,
});
}
Expand Down
8 changes: 4 additions & 4 deletions crates/blockifier/src/fee/fee_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,17 @@ pub fn verify_can_pay_committed_bounds(
// Sender will not be charged by `max_price_per_unit`, but this check should not
// depend on the current gas price.
{
Fee(u128::from(l1_gas.max_amount) * l1_gas.max_price_per_unit)
l1_gas.max_amount.saturating_mul(l1_gas.max_price_per_unit)
}
// TODO!(Aner): add tests
AllResources(AllResourceBounds { l1_gas, l2_gas, l1_data_gas }) => {
// Committed fee is sum of products (resource_max_amount * resource_max_price)
// of the different resources.
// The Sender will not be charged by`max_price_per_unit`, but this check should
// not depend on the current gas price
Fee(u128::from(l1_gas.max_amount) * l1_gas.max_price_per_unit
+ u128::from(l1_data_gas.max_amount) * l1_data_gas.max_price_per_unit
+ u128::from(l2_gas.max_amount) * l2_gas.max_price_per_unit)
l1_gas.max_amount.saturating_mul(l1_gas.max_price_per_unit)
+ l1_data_gas.max_amount.saturating_mul(l1_data_gas.max_price_per_unit)
+ l2_gas.max_amount.saturating_mul(l2_gas.max_price_per_unit)
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions crates/blockifier/src/transaction/account_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,18 +357,18 @@ impl AccountTransaction {
{
// TODO(Aner): refactor to indicate both amount and price are too low.
// TODO(Aner): refactor to return all amounts that are too low.
if minimal_gas_amount > resource_bounds.max_amount.into() {
if minimal_gas_amount > resource_bounds.max_amount {
return Err(TransactionFeeError::MaxGasAmountTooLow {
resource,
max_gas_amount: resource_bounds.max_amount.into(),
max_gas_amount: resource_bounds.max_amount,
minimal_gas_amount,
})?;
}
// TODO(Aner): refactor to return all prices that are too low.
if resource_bounds.max_price_per_unit < actual_gas_price.get().0 {
if resource_bounds.max_price_per_unit < actual_gas_price.get() {
return Err(TransactionFeeError::MaxGasPriceTooLow {
resource,
max_gas_price: resource_bounds.max_price_per_unit.into(),
max_gas_price: resource_bounds.max_price_per_unit,
actual_gas_price: actual_gas_price.into(),
})?;
}
Expand Down
29 changes: 14 additions & 15 deletions crates/blockifier/src/transaction/account_transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use cairo_vm::types::builtin_name::BuiltinName;
use cairo_vm::vm::runners::cairo_runner::ResourceTracker;
use pretty_assertions::assert_eq;
use rstest::rstest;
use starknet_api::block::GasPrice;
use starknet_api::core::{calculate_contract_address, ClassHash, ContractAddress, PatriciaKey};
use starknet_api::execution_resources::GasAmount;
use starknet_api::hash::StarkHash;
Expand Down Expand Up @@ -200,31 +201,29 @@ fn test_assert_actual_fee_in_bounds(
#[case] positive_flow: bool,
#[case] deprecated_tx: bool,
) {
let actual_fee_offset = if positive_flow { 0 } else { 1 };
let actual_fee_offset = Fee(if positive_flow { 0 } else { 1 });
if deprecated_tx {
let max_fee = 100;
let tx = account_invoke_tx(invoke_tx_args! {
max_fee: Fee(max_fee),
version: TransactionVersion::ONE,
});
let max_fee = Fee(100);
let tx = account_invoke_tx(invoke_tx_args! { max_fee, version: TransactionVersion::ONE });
let context = Arc::new(block_context.to_tx_context(&tx));
AccountTransaction::assert_actual_fee_in_bounds(&context, Fee(max_fee + actual_fee_offset));
AccountTransaction::assert_actual_fee_in_bounds(&context, max_fee + actual_fee_offset);
} else {
// All resources.
let l1_gas = ResourceBounds { max_amount: 2, max_price_per_unit: 3 };
let l2_gas = ResourceBounds { max_amount: 4, max_price_per_unit: 5 };
let l1_data_gas = ResourceBounds { max_amount: 6, max_price_per_unit: 7 };
let l1_gas = ResourceBounds { max_amount: GasAmount(2), max_price_per_unit: GasPrice(3) };
let l2_gas = ResourceBounds { max_amount: GasAmount(4), max_price_per_unit: GasPrice(5) };
let l1_data_gas =
ResourceBounds { max_amount: GasAmount(6), max_price_per_unit: GasPrice(7) };
let all_resource_bounds =
ValidResourceBounds::AllResources(AllResourceBounds { l1_gas, l2_gas, l1_data_gas });
let all_resource_fee = u128::from(l1_gas.max_amount) * l1_gas.max_price_per_unit
+ u128::from(l2_gas.max_amount) * l2_gas.max_price_per_unit
+ u128::from(l1_data_gas.max_amount) * l1_data_gas.max_price_per_unit
let all_resource_fee = l1_gas.max_amount.checked_mul(l1_gas.max_price_per_unit).unwrap()
+ l2_gas.max_amount.checked_mul(l2_gas.max_price_per_unit).unwrap()
+ l1_data_gas.max_amount.checked_mul(l1_data_gas.max_price_per_unit).unwrap()
+ actual_fee_offset;

// L1 resources.
let l1_resource_bounds = ValidResourceBounds::L1Gas(l1_gas);
let l1_resource_fee =
u128::from(l1_gas.max_amount) * l1_gas.max_price_per_unit + actual_fee_offset;
l1_gas.max_amount.checked_mul(l1_gas.max_price_per_unit).unwrap() + actual_fee_offset;

for (bounds, actual_fee) in
[(all_resource_bounds, all_resource_fee), (l1_resource_bounds, l1_resource_fee)]
Expand All @@ -234,7 +233,7 @@ fn test_assert_actual_fee_in_bounds(
version: TransactionVersion::THREE,
});
let context = Arc::new(block_context.to_tx_context(&tx));
AccountTransaction::assert_actual_fee_in_bounds(&context, Fee(actual_fee));
AccountTransaction::assert_actual_fee_in_bounds(&context, actual_fee);
}
}
}
Expand Down
28 changes: 14 additions & 14 deletions crates/blockifier/src/transaction/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,28 @@ pub enum TransactionFeeError {
#[error("Actual fee ({}) exceeded paid fee on L1 ({}).", actual_fee.0, paid_fee.0)]
InsufficientFee { paid_fee: Fee, actual_fee: Fee },
#[error(
"Resources bounds (l1 gas max amount: {l1_max_amount}, l1 gas max price: {l1_max_price}, \
l1 data max amount: {l1_data_max_amount}, l1 data max price: {l1_data_max_price}, l2 gas \
max amount: {l2_max_amount}, l2 gas max price: {l2_max_price}) exceed balance \
({balance})."
"Resources bounds (l1 gas max amount: {l1_max_amount:?}, l1 gas max price: \
{l1_max_price:?}, l1 data max amount: {l1_data_max_amount:?}, l1 data max price: \
{l1_data_max_price:?}, l2 gas max amount: {l2_max_amount:?}, l2 gas max price: \
{l2_max_price:?}) exceed balance ({balance})."
)]
ResourcesBoundsExceedBalance {
l1_max_amount: u64,
l1_max_price: u128,
l1_data_max_amount: u64,
l1_data_max_price: u128,
l2_max_amount: u64,
l2_max_price: u128,
l1_max_amount: GasAmount,
l1_max_price: GasPrice,
l1_data_max_amount: GasAmount,
l1_data_max_price: GasPrice,
l2_max_amount: GasAmount,
l2_max_price: GasPrice,
balance: BigUint,
},
#[error(
"Resource {resource} bounds (max amount: {max_amount}, max price): {max_price}) exceed \
balance ({balance})."
"Resource {resource} bounds (max amount: {max_amount:?}, max price): {max_price:?}) \
exceed balance ({balance})."
)]
GasBoundsExceedBalance {
resource: Resource,
max_amount: u64,
max_price: u128,
max_amount: GasAmount,
max_price: GasPrice,
balance: BigUint,
},
#[error("Max fee ({}) exceeds balance ({balance}).", max_fee.0, )]
Expand Down
18 changes: 9 additions & 9 deletions crates/blockifier/src/transaction/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,11 @@ pub fn run_invoke_tx(
/// Creates a `ResourceBoundsMapping` with the given `max_amount` and `max_price` for L1 gas limits.
/// No guarantees on the values of the other resources bounds.
// TODO: Check usages of this function and update to using all gas bounds.
pub fn l1_resource_bounds(max_amount: GasAmount, max_price: GasPrice) -> ValidResourceBounds {
ValidResourceBounds::L1Gas(ResourceBounds {
max_amount: max_amount.0,
max_price_per_unit: max_price.0,
})
pub fn l1_resource_bounds(
max_amount: GasAmount,
max_price_per_unit: GasPrice,
) -> ValidResourceBounds {
ValidResourceBounds::L1Gas(ResourceBounds { max_amount, max_price_per_unit })
}

#[fixture]
Expand Down Expand Up @@ -364,11 +364,11 @@ pub fn create_all_resource_bounds(
l1_data_max_price: GasPrice,
) -> ValidResourceBounds {
ValidResourceBounds::AllResources(AllResourceBounds {
l1_gas: ResourceBounds { max_amount: l1_max_amount.0, max_price_per_unit: l1_max_price.0 },
l2_gas: ResourceBounds { max_amount: l2_max_amount.0, max_price_per_unit: l2_max_price.0 },
l1_gas: ResourceBounds { max_amount: l1_max_amount, max_price_per_unit: l1_max_price },
l2_gas: ResourceBounds { max_amount: l2_max_amount, max_price_per_unit: l2_max_price },
l1_data_gas: ResourceBounds {
max_amount: l1_data_max_amount.0,
max_price_per_unit: l1_data_max_price.0,
max_amount: l1_data_max_amount,
max_price_per_unit: l1_data_max_price,
},
})
}
Expand Down
26 changes: 13 additions & 13 deletions crates/blockifier/src/transaction/transactions_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,16 +983,16 @@ fn test_insufficient_new_resource_bounds(

let default_resource_bounds = AllResourceBounds {
l1_gas: ResourceBounds {
max_amount: minimal_gas_vector.l1_gas.0,
max_price_per_unit: actual_strk_l1_gas_price.get().0,
max_amount: minimal_gas_vector.l1_gas,
max_price_per_unit: actual_strk_l1_gas_price.get(),
},
l2_gas: ResourceBounds {
max_amount: minimal_gas_vector.l2_gas.0,
max_price_per_unit: actual_strk_l2_gas_price.get().0,
max_amount: minimal_gas_vector.l2_gas,
max_price_per_unit: actual_strk_l2_gas_price.get(),
},
l1_data_gas: ResourceBounds {
max_amount: minimal_gas_vector.l1_data_gas.0,
max_price_per_unit: actual_strk_l1_data_gas_price.get().0,
max_amount: minimal_gas_vector.l1_data_gas,
max_price_per_unit: actual_strk_l1_data_gas_price.get(),
},
};

Expand Down Expand Up @@ -1029,14 +1029,14 @@ fn test_insufficient_new_resource_bounds(
(L2Gas, default_resource_bounds.l2_gas),
(L1DataGas, default_resource_bounds.l1_data_gas),
] {
if resource_bounds.max_amount == 0 {
if resource_bounds.max_amount == 0_u8.into() {
continue;
}
let mut invalid_resources = default_resource_bounds;
match insufficient_resource {
L1Gas => invalid_resources.l1_gas.max_amount -= 1,
L2Gas => invalid_resources.l2_gas.max_amount -= 1,
L1DataGas => invalid_resources.l1_data_gas.max_amount -= 1,
L1Gas => invalid_resources.l1_gas.max_amount.0 -= 1,
L2Gas => invalid_resources.l2_gas.max_amount.0 -= 1,
L1DataGas => invalid_resources.l1_data_gas.max_amount.0 -= 1,
}
let invalid_v3_tx = account_invoke_tx(InvokeTxArgs {
resource_bounds: ValidResourceBounds::AllResources(invalid_resources),
Expand All @@ -1059,9 +1059,9 @@ fn test_insufficient_new_resource_bounds(
for insufficient_resource in [L1Gas, L2Gas, L1DataGas] {
let mut invalid_resources = default_resource_bounds;
match insufficient_resource {
L1Gas => invalid_resources.l1_gas.max_price_per_unit -= 1,
L2Gas => invalid_resources.l2_gas.max_price_per_unit -= 1,
L1DataGas => invalid_resources.l1_data_gas.max_price_per_unit -= 1,
L1Gas => invalid_resources.l1_gas.max_price_per_unit.0 -= 1,
L2Gas => invalid_resources.l2_gas.max_price_per_unit.0 -= 1,
L1DataGas => invalid_resources.l1_data_gas.max_price_per_unit.0 -= 1,
}

let invalid_v3_tx = account_invoke_tx(InvokeTxArgs {
Expand Down
6 changes: 4 additions & 2 deletions crates/gateway/src/stateful_transaction_validator_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ use mockall::predicate::eq;
use num_bigint::BigUint;
use pretty_assertions::assert_eq;
use rstest::{fixture, rstest};
use starknet_api::block::GasPrice;
use starknet_api::core::Nonce;
use starknet_api::executable_transaction::Transaction;
use starknet_api::execution_resources::GasAmount;
use starknet_api::test_utils::deploy_account::executable_deploy_account_tx;
use starknet_api::test_utils::invoke::executable_invoke_tx;
use starknet_api::transaction::Resource;
Expand All @@ -36,8 +38,8 @@ pub const STATEFUL_VALIDATOR_FEE_ERROR: BlockifierStatefulValidatorError =
TransactionPreValidationError::TransactionFeeError(
TransactionFeeError::GasBoundsExceedBalance {
resource: Resource::L1DataGas,
max_amount: VALID_L1_GAS_MAX_AMOUNT,
max_price: VALID_L1_GAS_MAX_PRICE_PER_UNIT,
max_amount: GasAmount(VALID_L1_GAS_MAX_AMOUNT),
max_price: GasPrice(VALID_L1_GAS_MAX_PRICE_PER_UNIT),
balance: BigUint::ZERO,
},
),
Expand Down
6 changes: 5 additions & 1 deletion crates/gateway/src/stateless_transaction_validator.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use starknet_api::block::GasPrice;
use starknet_api::execution_resources::GasAmount;
use starknet_api::rpc_transaction::{
RpcDeclareTransaction,
RpcDeployAccountTransaction,
Expand Down Expand Up @@ -179,7 +181,9 @@ fn validate_resource_is_non_zero(
resource: Resource,
) -> StatelessTransactionValidatorResult<()> {
let resource_bounds = all_resource_bounds.get_bound(resource);
if resource_bounds.max_amount == 0 || resource_bounds.max_price_per_unit == 0 {
if resource_bounds.max_amount == GasAmount(0)
|| resource_bounds.max_price_per_unit == GasPrice(0)
{
return Err(StatelessTransactionValidatorError::ZeroResourceBounds {
resource,
resource_bounds,
Expand Down
5 changes: 3 additions & 2 deletions crates/mempool/src/mempool.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::HashMap;

use starknet_api::block::GasPrice;
use starknet_api::core::{ContractAddress, Nonce};
use starknet_api::executable_transaction::Transaction;
use starknet_api::transaction::{Tip, TransactionHash, ValidResourceBounds};
Expand Down Expand Up @@ -135,7 +136,7 @@ impl Mempool {
}

// TODO(Mohammad): Rename this method once consensus API is added.
fn _update_gas_price_threshold(&mut self, threshold: u128) {
fn _update_gas_price_threshold(&mut self, threshold: GasPrice) {
self.tx_queue._update_gas_price_threshold(threshold);
}

Expand Down Expand Up @@ -237,7 +238,7 @@ impl TransactionReference {
}
}

pub fn get_l2_gas_price(&self) -> u128 {
pub fn get_l2_gas_price(&self) -> GasPrice {
self.resource_bounds.get_l2_bounds().max_price_per_unit
}
}
Loading

0 comments on commit 0a090fa

Please sign in to comment.