feat(execution): add l2_gas_price to FeeEstimation

[TzahiTaub]

This change is 

[TzahiTaub]

• feat(mempool): add l2_gas_price support in BlockHeader #652
• feat(execution): add l2_gas_price to FeeEstimation #644 👈
• chore(execution): add l2_gas_price to PendingData #643
• feat(starknet_api, protobuf): add l2_gas_price to block header #641
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @TzahiTaub and the rest of your teammates on Graphite

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.90%. Comparing base (b0cfe82) to head (2e11701).
Report is 116 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (2e11701). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (2e11701)
	3	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #644       +/-   ##
===========================================
- Coverage   74.18%   50.90%   -23.29%     
===========================================
  Files         359      131      -228     
  Lines       36240    18906    -17334     
  Branches    36240    18906    -17334     
===========================================
- Hits        26886     9624    -17262     
- Misses       7220     8219      +999     
+ Partials     2134     1063     -1071     

Flag	Coverage Δ
	50.90% <100.00%> (-23.29%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dorimedini-starkware]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ShahakShama and @TzahiTaub)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

    pub struct FeeEstimation {
        pub gas_consumed: Felt,
        pub gas_price: GasPrice,

can this be renamed to l1_gas_price...? confusing as it is now
or is this some object that needs to be BC?

Code quote:

pub gas_price: GasPrice,

---

crates/papyrus_rpc/src/v0_7/execution_test.rs line 163 at r1 (raw file):

        price_in_wei: GasPrice(1),
        price_in_fri: GasPrice(0),
    };

the first two prices are "sane" (pretty close to actual prices), should L2_GAS_PRICE be the same? @ShahakShama

Code quote:

    pub static ref GAS_PRICE: GasPricePerToken = GasPricePerToken{
        price_in_wei: GasPrice(100 * u128::pow(10, 9)),
        price_in_fri: GasPrice(0),
    };
    pub static ref DATA_GAS_PRICE: GasPricePerToken = GasPricePerToken{
        price_in_wei: GasPrice(1),
        price_in_fri: GasPrice(0),
    };
    pub static ref L2_GAS_PRICE: GasPricePerToken = GasPricePerToken{
        price_in_wei: GasPrice(1),
        price_in_fri: GasPrice(0),
    };

[ShahakShama]

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

---

crates/papyrus_execution/src/objects.rs line 100 at r1 (raw file):

    /// The gas price for DA blob.
    pub data_gas_price: GasPrice,
    // TODO(Tzahi): Add l2_gas_consumed + verify estimation is close enough to l1 gas fee if user

This TODO is very unclear. Try to rephrase it. The + is the confusing part IMO

---

crates/papyrus_execution/src/objects.rs line 160 at r1 (raw file):

) -> ExecutionResult<FeeEstimation> {
    let gas_prices = &block_context.block_info().gas_prices;
    let (gas_price, data_gas_price, l2_gas_price) = (

As Dori said, change gas_price to l1_gas_price (and if it's nahoog to write l1_data_gas instead of data_gas write l1_data_gas_price as well)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can this be renamed to l1_gas_price...? confusing as it is now
or is this some object that needs to be BC?

Dan recently said that in the RPC we support only the latest version, so no need for BC
We should verify with @ArielElp what are the names of fields of FeeEstimation in rpc v0.8

---

crates/papyrus_rpc/src/v0_7/execution_test.rs line 163 at r1 (raw file):

Previously, dorimedini-starkware wrote…

the first two prices are "sane" (pretty close to actual prices), should L2_GAS_PRICE be the same? @ShahakShama

the DATA_GAS_PRICE isn't tested yet, that's why we put 1. For now, you can keep the l2 gas price as you did. Just add a TODO to add tests for data gas and l2 gas

[TzahiTaub]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, and @ShahakShama)

---

crates/papyrus_execution/src/objects.rs line 100 at r1 (raw file):

Previously, ShahakShama wrote…

This TODO is very unclear. Try to rephrase it. The + is the confusing part IMO

I think I meant (:) ) - we have an automatic translation of l1_gas_price to l2_gas_price in the version constants (l1\_to\_l2\_gas\_price\_conversion). I think we would like to verify that running txs with l1_gas_price only, and running them using l1_gas_price and the l2_gas_price as computed automatically, will give close overall_fee results (otherwise, we have some inconsistency). "close" and not exact equality as we use rounding.
Makes sense?

[TzahiTaub]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, and @ShahakShama)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, ShahakShama wrote…

Dan recently said that in the RPC we support only the latest version, so no need for BC
We should verify with @ArielElp what are the names of fields of FeeEstimation in rpc v0.8

Is rpc considered the only user interface with theoretically BC issues, and in any other crate we can do whatever we want?

[TzahiTaub]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, and @ShahakShama)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Is rpc considered the only user interface with theoretically BC issues, and in any other crate we can do whatever we want?

And I see this truct (FeeEstimation) is returned in rpc calls - we would like to keep the same names for v7 and v6 if possible AFAIU, I think we don't want to runover those names for existing API. (I mean, making v7 return l1_gas_price is not exactly BC issue, its making the V7 route in our rpc server diverge from the V7 spec).

[ShahakShama]

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

---

crates/papyrus_execution/src/objects.rs line 100 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I think I meant (:) ) - we have an automatic translation of l1_gas_price to l2_gas_price in the version constants (l1\_to\_l2\_gas\_price\_conversion). I think we would like to verify that running txs with l1_gas_price only, and running them using l1_gas_price and the l2_gas_price as computed automatically, will give close overall_fee results (otherwise, we have some inconsistency). "close" and not exact equality as we use rounding.
Makes sense?

Yes. Now write it inside the comment

---

crates/papyrus_execution/src/objects.rs line 160 at r1 (raw file):

Previously, ShahakShama wrote…

As Dori said, change gas_price to l1_gas_price (and if it's nahoog to write l1_data_gas instead of data_gas write l1_data_gas_price as well)

Checking with @ArielElp on slack if it's l1_data_gas or data_gas

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

And I see this truct (FeeEstimation) is returned in rpc calls - we would like to keep the same names for v7 and v6 if possible AFAIU, I think we don't want to runover those names for existing API. (I mean, making v7 return l1_gas_price is not exactly BC issue, its making the V7 route in our rpc server diverge from the V7 spec).

We only need to support the latest version. Change the jsonschema file to have l1_gas_price instead of gas_price and I'm checking with Ariel whether data_gas needs renaming or not

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp and @TzahiTaub)

[dorimedini-starkware]

Reviewed 2 of 18 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: 4 of 19 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @ShahakShama, and @TzahiTaub)

---

bin/protoc line 0 at r5 (raw file):
should this file be tracked by git...?
where does it come from?
@ShahakShama maybe related to AlonL's changes?

[dorimedini-starkware]

Reviewed 1 of 18 files at r3, 14 of 14 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp, @TzahiTaub, and @yoavGrs)

[yoavGrs]

Reviewable status: 18 of 19 files reviewed, 4 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, @ShahakShama, and @TzahiTaub)

---

bin/protoc line at r5 (raw file):

Previously, dorimedini-starkware wrote…

should this file be tracked by git...?
where does it come from?
@ShahakShama maybe related to AlonL's changes?

Reverted

[dorimedini-starkware]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @TzahiTaub)

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, ShahakShama wrote…

We only need to support the latest version. Change the jsonschema file to have l1_gas_price instead of gas_price and I'm checking with Ariel whether data_gas needs renaming or not

Change gas_price to l1_gas_price and data_gas_price to l1_data_gas_price

[ShahakShama]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

[yoavGrs]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, ShahakShama wrote…

Change gas_price to l1_gas_price and data_gas_price to l1_data_gas_price

I'm opening a new PR for this

[ShahakShama]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

[dorimedini-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, yoavGrs wrote…

I'm opening a new PR for this

@yoavGrs can you link to it when it's open?

[yoavGrs]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

---

crates/papyrus_execution/src/testing_instances.rs line 69 at r1 (raw file):

Previously, dorimedini-starkware wrote…

@yoavGrs can you link to it when it's open?

https://reviewable.io/reviews/starkware-libs/sequencer/1007

[dorimedini-starkware]

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