chore(blockifier): separate create and validate logic on gas prices

[ArniStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ArniStarkware]

• chore(blockifier): move block info to starknet api #2246 : 2 dependent PRs (#2230 , #2237 )
• chore(blockifier): move GasPrices to snapi #2245
• feat(blockifier): derive serialize for block info #2244
• chore(blockifier): rename the getters for the gas prices struct #2269
• chore(blockifier): separate create and validate logic on gas prices #2243 👈 (View in Graphite)
• main

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

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-24T12:09:38Z
• Started at 2024-11-24T12:21:19Z
• Started at 2024-11-24T12:27:02Z
• Started at 2024-11-24T14:49:22Z
• Started at 2024-11-25T11:42:52Z
• Started at 2024-11-25T11:49:44Z
• Started at 2024-11-25T14:44:21Z
• Started at 2024-11-25T14:56:46Z

[codecov]

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 52.71%. Comparing base (e3165c4) to head (7d6ad29).
Report is 576 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/blockifier/block.rs	90.47%	2 Missing ⚠️
crates/starknet_batcher/src/block_builder.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2243       +/-   ##
===========================================
+ Coverage   40.10%   52.71%   +12.60%     
===========================================
  Files          26      228      +202     
  Lines        1895    25689    +23794     
  Branches     1895    25689    +23794     
===========================================
+ Hits          760    13542    +12782     
- Misses       1100    11170    +10070     
- Partials       35      977      +942     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[alonh5]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @ArniStarkware, and @dorimedini-starkware)

---

a discussion (no related file):
Why do we need this change? Specifically if new() is now a private method, what's the difference between safe_new() and the previous new()?

[ArniStarkware]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @alonh5, @aner-starkware, and @dorimedini-starkware)

---

a discussion (no related file):

Previously, alonh5 (Alon Haramati) wrote…

Why do we need this change? Specifically if new() is now a private method, what's the difference between safe_new() and the previous new()?

The goal is to move new() into starknet API.
You do have a point - it will be public at that point. Should I change the visibility of new in this PR?

[alonh5]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware, @ArniStarkware, and @dorimedini-starkware)

---

a discussion (no related file):

Previously, ArniStarkware (Arnon Hod) wrote…

The goal is to move new() into starknet API.
You do have a point - it will be public at that point. Should I change the visibility of new in this PR?

I think the reason there is a new() function is for that validation. So in that case I would remove the new() function and the getters completely and make the fields public. WDYT?

---

crates/blockifier/src/blockifier/block.rs line 88 at r2 (raw file):

    }

    pub fn safe_new(

Suggestion:

validated_new

[ArniStarkware]

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @alonh5, @aner-starkware, and @dorimedini-starkware)

---

a discussion (no related file):

Previously, alonh5 (Alon Haramati) wrote…

I think the reason there is a new() function is for that validation. So in that case I would remove the new() function and the getters completely and make the fields public. WDYT?

Done.

---

crates/blockifier/src/blockifier/block.rs line 88 at r2 (raw file):

    }

    pub fn safe_new(

Done.

[alonh5]

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

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @ArniStarkware, and @dorimedini-starkware)

---

crates/blockifier/src/blockifier/block.rs line 108 at r3 (raw file):

            FeeType::Eth => &self.eth_gas_prices,
        }
    }

You can also remove these now.

Code quote:

    pub fn get_l1_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonzeroGasPrice {
        self.get_gas_prices_by_fee_type(fee_type).l1_gas_price
    }

    pub fn get_l1_data_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonzeroGasPrice {
        self.get_gas_prices_by_fee_type(fee_type).l1_data_gas_price
    }

    pub fn get_l2_gas_price_by_fee_type(&self, fee_type: &FeeType) -> NonzeroGasPrice {
        self.get_gas_prices_by_fee_type(fee_type).l2_gas_price
    }

    pub fn get_gas_prices_by_fee_type(&self, fee_type: &FeeType) -> &GasPriceVector {
        match fee_type {
            FeeType::Strk => &self.strk_gas_prices,
            FeeType::Eth => &self.eth_gas_prices,
        }
    }

[ArniStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @aner-starkware, and @dorimedini-starkware)

---

crates/blockifier/src/blockifier/block.rs line 108 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You can also remove these now.

No - I can not. I looked into it.
It is used in many locations, non-trivially.

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @ArniStarkware, and @dorimedini-starkware)

---

crates/blockifier/src/blockifier/block.rs line 108 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

No - I can not. I looked into it.
It is used in many locations, non-trivially.

Oh I see. Let's leave them then.
Can you remove the get_ prefix and the _by_fee_type suffix?
The Rust style guide says you shouldn't use get_ and the _by_fee_type is also redundant because the fee type argument signifies that.

[ArniStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @aner-starkware, and @dorimedini-starkware)

---

crates/blockifier/src/blockifier/block.rs line 108 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Oh I see. Let's leave them then.
Can you remove the get_ prefix and the _by_fee_type suffix?
The Rust style guide says you shouldn't use get_ and the _by_fee_type is also redundant because the fee type argument signifies that.

Done. Now on the next PR on this stack: #2269.

[alonh5]

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