refactor(starknet_api,blockifier): to_discounted_gas_amount

[dorimedini-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-22T14:48:33Z
• Started at 2024-12-24T12:21:57Z
• Started at 2025-01-01T12:26:57Z
• Started at 2025-01-02T09:50:40Z
• Started at 2025-01-02T12:04:56Z
• Started at 2025-01-02T13:40:33Z
• Started at 2025-01-02T14:33:48Z

[dorimedini-starkware]

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @TzahiTaub)

---

crates/starknet_api/src/execution_resources.rs line 179 at r1 (raw file):

    l1_gas: GasAmount,
    l1_data_gas: GasAmount,
) -> GasAmount {

this change is to make explicit the fact that L2 gas is ignored here

Code quote:

pub fn to_discounted_l1_gas(
    l1_gas_price: NonzeroGasPrice,
    l1_data_gas_price: GasPrice,
    l1_gas: GasAmount,
    l1_data_gas: GasAmount,
) -> GasAmount {

[TzahiTaub]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/starknet_api/src/execution_resources.rs line 167 at r2 (raw file):

}

/// Compute l1_gas estimation from gas_vector using the following formula:

Why is this an "estimation"?

Suggestion:

Computes the total l1 gas amount from the different given l1 gas arguments using the following formula:

---

crates/starknet_api/src/execution_resources.rs line 185 at r2 (raw file):

    l1_data_gas: GasAmount,
) -> GasAmount {
    let l1_data_gas_fee = l1_data_gas.checked_mul(l1_data_gas_price).unwrap_or_else(|| {

Makes more sense to make the conversion here and have the type as it's in GasPriceVector IMO

Suggestion:

pub fn to_discounted_l1_gas(
    l1_gas_price: NonzeroGasPrice,
    l1_data_gas_price: NonzeroGasPrice,
    l1_gas: GasAmount,
    l1_data_gas: GasAmount,
) -> GasAmount {
    let l1_data_gas_fee = l1_data_gas.checked_mul(l1_data_gas_price.into()).unwrap_or_else(|| {

---

crates/blockifier/src/fee/fee_utils.rs line 47 at r2 (raw file):

        let discounted_l1_gas = to_discounted_l1_gas(
            gas_prices.l1_gas_price,
            gas_prices.l1_data_gas_price.into(),

Remove if you accept the suggestion below

Suggestion:

  gas_prices.l1_data_gas_price,

[dorimedini-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)

---

crates/blockifier/src/fee/fee_utils.rs line 47 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Remove if you accept the suggestion below

see below

---

crates/starknet_api/src/execution_resources.rs line 167 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Why is this an "estimation"?

maybe the wording is unclear: the actual value returned isn't really an amount of L1 gas, it's a "best effort" conversion, to be able to charge users signing deprecated resource bounds in proportion to actual computation resources / DA gas used.
I used the word "estimation", do you have a suggestion?
(your suggestion implies the returned value is actually L1 gas, when it really isn't..)

---

crates/starknet_api/src/execution_resources.rs line 185 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Makes more sense to make the conversion here and have the type as it's in GasPriceVector IMO

as an API decision: we require the L1 gas price to be nonzero, but the data gas price is allowed to be zero. Why enforce nonzero data gas price?

[dorimedini-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

a discussion (no related file):
py side

[TzahiTaub]

crates/starknet_api/src/execution_resources.rs line 185 at r2 (raw file):

Previously, dorimedini-starkware wrote…

as an API decision: we require the L1 gas price to be nonzero, but the data gas price is allowed to be zero. Why enforce nonzero data gas price?

I don't know but that's the type we have here

[dorimedini-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/starknet_api/src/execution_resources.rs line 185 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I don't know but that's the type we have here

yes, gas prices in general must be non-zero because sometimes we divide by them.
in this specific function, we divide by L1 gas price, but we only multiply by data gas price, so it can be zero

[TzahiTaub]

crates/starknet_api/src/execution_resources.rs line 185 at r2 (raw file):

Previously, dorimedini-starkware wrote…

yes, gas prices in general must be non-zero because sometimes we divide by them.
in this specific function, we divide by L1 gas price, but we only multiply by data gas price, so it can be zero

I don't understand, it's illegal for a use to give a 0 DA price, but you want to support it in this specific helper function?

[dorimedini-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/starknet_api/src/execution_resources.rs line 185 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I don't understand, it's illegal for a use to give a 0 DA price, but you want to support it in this specific helper function?

yes, because this function doesn't care.
GasPriceVector is used in many places, specifically in the block info, where we do not allow zero gas prices

[TzahiTaub]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/starknet_api/src/execution_resources.rs line 167 at r2 (raw file):

Previously, dorimedini-starkware wrote…

maybe the wording is unclear: the actual value returned isn't really an amount of L1 gas, it's a "best effort" conversion, to be able to charge users signing deprecated resource bounds in proportion to actual computation resources / DA gas used.
I used the word "estimation", do you have a suggestion?
(your suggestion implies the returned value is actually L1 gas, when it really isn't..)

Computes the total l1 gas amount estimation from the different given l1 gas arguments using the following formula:

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

py side

passes

[TzahiTaub]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)