feat(starknet_l1_provider): add interface to query ethereum gas price

[guy-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• feat(starknet_l1_provider): integrate gas price components in the node #4081
• feat(starknet_l1_provider): add L1 gas price scraper #4000
• feat(starknet_l1_provider): add the L1GasPriceProvider implementation #3794
• feat(starknet_l1_provider): add the trait interface for L1 gas price provider #3792
• feat(starknet_l1_provider): add interface to query ethereum gas price #3679 👈 (View in Graphite)
• main

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

[asmaastarkware]

I’m also curious about ur questions

Reviewed 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @giladchase, @guy-starkware, and @matan-starkware)

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 196 at r3 (raw file):

            .await?;
        if let Some(block) = block {
            match (block.header.timestamp, block.header.base_fee_per_gas) {

why do we just force gas price to have some value?

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 206 at r3 (raw file):

        } else {
            Ok(None)
        }

Suggestion:

        let Some(block) = block else {
            Ok(None)
        };
        match (block.header.timestamp, block.header.base_fee_per_gas) {
            (timestamp, Some(base_fee_per_gas)) => Ok(Some(PriceSample {
                timestamp,
                base_fee_per_gas,
                blob_fee: block.header.blob_fee().unwrap_or(0),
            })),
            _ => Ok(None),
        } 

---

crates/papyrus_base_layer/src/lib.rs line 64 at r3 (raw file):

    ) -> Result<Vec<L1Event>, Self::Error>;

    async fn get_block_timestamp(&self, block_number: u64) -> Result<Option<u64>, Self::Error>;

why not pass L1BlockNumber instead of u64? (and bellow)

---

crates/papyrus_base_layer/src/lib.rs line 74 at r3 (raw file):

}

/// A struct the holds together the data on the baselayer's gas prices, for a given timestamp.

Suggestion:

/// A struct that holds together the data on the baselayer's gas prices, for a given timestamp.

---

crates/papyrus_base_layer/src/base_layer_test.rs line 86 at r3 (raw file):

    // if !in_ci() {
    //     return;
    // }

[guy-starkware]

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @asmaastarkware, @giladchase, and @matan-starkware)

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 196 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

why do we just force gas price to have some value?

I think that in the general case data gas can be None, but for the specific goal of PriceSample it needs to have a value, even if it is zero. The way I think about it is the PriceSample is used to estimate the cost of the L2 block, using mean over many samples. If we start having None values in there it will make our life a lot more complicated. On the other hand, I think that None really means there is no cost for blobs, in which case zero is a good value for it.

---

crates/papyrus_base_layer/src/lib.rs line 64 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

why not pass L1BlockNumber instead of u64? (and bellow)

Yes, good idea.

---

crates/papyrus_base_layer/src/base_layer_test.rs line 86 at r3 (raw file):

    // if !in_ci() {
    //     return;
    // }

Actually I'm going to uncomment this. It is the same for the other tests (don't know why, maybe Gilad will know).

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 206 at r3 (raw file):

        } else {
            Ok(None)
        }

Done.

---

crates/papyrus_base_layer/src/lib.rs line 74 at r3 (raw file):

}

/// A struct the holds together the data on the baselayer's gas prices, for a given timestamp.

Done.

[asmaastarkware]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase and @matan-starkware)

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 196 at r3 (raw file):

Previously, guy-starkware wrote…

I think that in the general case data gas can be None, but for the specific goal of PriceSample it needs to have a value, even if it is zero. The way I think about it is the PriceSample is used to estimate the cost of the L2 block, using mean over many samples. If we start having None values in there it will make our life a lot more complicated. On the other hand, I think that None really means there is no cost for blobs, in which case zero is a good value for it.

do u know in which cases gas price (base_fee_per_gas) is None?

[matan-starkware]

Reviewed 1 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase and @guy-starkware)

---

crates/papyrus_base_layer/src/lib.rs line 79 at r4 (raw file):

        &self,
        block_number: L1BlockNumber,
    ) -> Result<Option<PriceSample>, Self::Error>;

Unless there is a use case that specifically needs these, I'd rather not add them, just to keep the interface smaller.

(If this was some public library repo then adding as much as we can would be useful, but it's just used by people who can add these fns if they need them)

Suggestion:

    async fn get_price_sample(
        &self,
        block_number: L1BlockNumber,
    ) -> Result<Option<PriceSample>, Self::Error>;

---

crates/papyrus_base_layer/src/lib.rs line 89 at r4 (raw file):

}

impl Display for PriceSample {

Why do we need Display? Is Debug not sufficient (your implementation looks identical to what Debug would show)

Code quote:

impl Display for PriceSample {

[guy-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase and @matan-starkware)

---

crates/papyrus_base_layer/src/ethereum_base_layer_contract.rs line 196 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

do u know in which cases gas price (base_fee_per_gas) is None?

I'm not sure. I am guessing that it happens when the blob demand is low (i.e., there's more room in blob than people want).

---

crates/papyrus_base_layer/src/lib.rs line 79 at r4 (raw file):

Previously, matan-starkware wrote…

Unless there is a use case that specifically needs these, I'd rather not add them, just to keep the interface smaller.

(If this was some public library repo then adding as much as we can would be useful, but it's just used by people who can add these fns if they need them)

Done.

---

crates/papyrus_base_layer/src/lib.rs line 89 at r4 (raw file):

Previously, matan-starkware wrote…

Why do we need Display? Is Debug not sufficient (your implementation looks identical to what Debug would show)

Yes, I guess you are right.

[matan-starkware]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase and @guy-starkware)

---

crates/papyrus_base_layer/src/base_layer_test.rs line 100 at r5 (raw file):

        "Gas price: {}, data gas price: {:?}, timestamp: {}",
        price_sample.blob_fee, price_sample.base_fee_per_gas, price_sample.timestamp
    );

Good thing to discuss with Gilad

Remove the print statement since you have the asserts

Suggestion:

    // TODO(guyn): Figure out how these numbers are calculated, instead of just printing and testing
    // against what we got. Use this println! to get the numbers to put into the asserts.

[guy-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase and @matan-starkware)

---

crates/papyrus_base_layer/src/base_layer_test.rs line 100 at r5 (raw file):

Previously, matan-starkware wrote…

Good thing to discuss with Gilad

Remove the print statement since you have the asserts

Done.

[matan-starkware]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)