Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(starknet_l1_provider): add the L1GasPriceProvider implementation #3794

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

guy-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 7c61569 to f9e47dd Compare January 29, 2025 11:13
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 1b01757 to 76b5421 Compare January 29, 2025 11:13
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from f9e47dd to 740fc7b Compare January 30, 2025 13:56
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 76b5421 to d32d4f3 Compare January 30, 2025 13:56
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 740fc7b to 4dcedb4 Compare February 3, 2025 11:31
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from d32d4f3 to d3793af Compare February 3, 2025 11:31
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 4dcedb4 to 9803183 Compare February 3, 2025 14:06
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from d3793af to 3bfb770 Compare February 3, 2025 14:06
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 9803183 to 6bf02d6 Compare February 4, 2025 09:15
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 3bfb770 to 4d73838 Compare February 4, 2025 09:15
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 6bf02d6 to 24cada3 Compare February 4, 2025 12:49
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 4d73838 to d4c3ad4 Compare February 4, 2025 12:49
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 24cada3 to ece1129 Compare February 5, 2025 13:26
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch 2 times, most recently from 0203056 to 02ab392 Compare February 5, 2025 13:33
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch 2 times, most recently from 852008c to 450282a Compare February 5, 2025 15:13
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 02ab392 to e89fe3c Compare February 5, 2025 15:15
Copy link

github-actions bot commented Feb 5, 2025

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [35.306 ms 35.393 ms 35.531 ms]
change: [-4.0230% -2.5322% -1.2204%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

full_committer_flow performance improved 😺
full_committer_flow time: [31.118 ms 31.163 ms 31.210 ms]
change: [-2.5041% -1.9060% -1.4225%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild

@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_interface branch from 450282a to 73e55cf Compare February 5, 2025 15:40
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from e89fe3c to 6c8d247 Compare February 6, 2025 08:41
@guy-starkware guy-starkware changed the base branch from guyn/l1price/provider_interface to main February 6, 2025 08:41
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 6c8d247 to cf06a20 Compare February 6, 2025 12:09
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 1337a1b to b0aa91a Compare February 9, 2025 14:07
@matan-starkware matan-starkware self-requested a review February 9, 2025 14:10
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from b0aa91a to 28a2f0c Compare February 9, 2025 14:15
Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 9 files at r3, all commit messages.
Reviewable status: 4 of 10 files reviewed, 13 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 63 at r2 (raw file):

Previously, guy-starkware wrote…

OK, but I don't really understand why not make it serializable yet.

The why is that I want this PR to focus on creating and testing the provider. Making the config serializable and connecting to the component infrastructure is a distinct PR.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 76 at r5 (raw file):

    ) -> Result<(), L1GasPriceProviderError> {
        let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0);
        if height.0 != last_plus_one {

Suggestion:

        if height.0 != self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0) {

crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 77 at r5 (raw file):

        let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0);
        if height.0 != last_plus_one {
            return Err(L1GasPriceProviderError::AddPriceInfoError(format!(

Suggestion:

InvalidHeight(

crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 103 at r5 (raw file):

        // Could not find a block with the requested timestamp and lag.
        let Some(last_index_rev) = index_last_timestamp_rev else {
            return Err(L1GasPriceProviderError::GetPriceInfoError(format!(

Suggestion:

MissingData

crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 109 at r5 (raw file):

        };
        // We need to convert the index to the forward direction.
        let last_index = self.data.len() - last_index_rev - 1;

Why bother with this? It seems like you keep doing +1 later, so just keep the old value to use

Code quote:

        let last_index = self.data.len() - last_index_rev - 1;

crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 111 at r5 (raw file):

        let last_index = self.data.len() - last_index_rev - 1;

        let num_blocks = usize::try_from(self.config.number_of_blocks_for_mean).unwrap();

expect

Code quote:

unwrap();

crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 113 at r5 (raw file):

        let num_blocks = usize::try_from(self.config.number_of_blocks_for_mean).unwrap();
        if last_index + 1 < num_blocks {
            return Err(L1GasPriceProviderError::GetPriceInfoError(format!(

Suggestion:

MissingData

@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 28a2f0c to 991f91a Compare February 9, 2025 15:46
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 63 at r2 (raw file):

Previously, matan-starkware wrote…

The why is that I want this PR to focus on creating and testing the provider. Making the config serializable and connecting to the component infrastructure is a distinct PR.

OK. I just thought that was standard boilerplate but I don't mind doing that later.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 109 at r5 (raw file):

Previously, matan-starkware wrote…

Why bother with this? It seems like you keep doing +1 later, so just keep the old value to use

Yes, good idea.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 111 at r5 (raw file):

Previously, matan-starkware wrote…

expect

I highly doubt this will ever trigger, but ok.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 76 at r5 (raw file):

    ) -> Result<(), L1GasPriceProviderError> {
        let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0);
        if height.0 != last_plus_one {

I'm reusing this variable in the error message, and don't want to repeat the whole thing.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 77 at r5 (raw file):

        let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0);
        if height.0 != last_plus_one {
            return Err(L1GasPriceProviderError::AddPriceInfoError(format!(

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 103 at r5 (raw file):

        // Could not find a block with the requested timestamp and lag.
        let Some(last_index_rev) = index_last_timestamp_rev else {
            return Err(L1GasPriceProviderError::GetPriceInfoError(format!(

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 113 at r5 (raw file):

        let num_blocks = usize::try_from(self.config.number_of_blocks_for_mean).unwrap();
        if last_index + 1 < num_blocks {
            return Err(L1GasPriceProviderError::GetPriceInfoError(format!(

Done.

@matan-starkware matan-starkware self-requested a review February 10, 2025 09:45
@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch 2 times, most recently from ea204ab to 5d496df Compare February 10, 2025 10:23
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [35.118 ms 35.183 ms 35.260 ms]
change: [-4.0196% -2.5819% -1.3465%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 9 files at r3, 7 of 7 files at r7, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @guy-starkware)


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 63 at r2 (raw file):

Previously, guy-starkware wrote…

OK. I just thought that was standard boilerplate but I don't mind doing that later.

It's standard, but it isn't needed to define this struct. It's needed when we connect this to the node's infrastructure.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 111 at r5 (raw file):

Previously, guy-starkware wrote…

I highly doubt this will ever trigger, but ok.

Agreed. I thought we had a linter for this, CI should have failed.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 35 at r6 (raw file):

    InvalidHeight(String),
    #[error("Failed to add price info: {0}")]
    MissingData(String),

Suggestion:

    #[error("Failed to calculate price info: {0}")]
    MissingData(String),

crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 37 at r6 (raw file):

    MissingData(String),
    #[error("Failed to get price info: {0}")]
    GetPriceInfoError(String),

Unused, remove?

Code quote:

    #[error("Failed to get price info: {0}")]
    GetPriceInfoError(String),

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 12 at r6 (raw file):

// of 6 + 60 (which is the lag margin).
// Returns the provider, the final timestamp, and the lag margin.
fn make_provider_with_few_samples() -> (L1GasPriceProvider, u64, u64) {

Just set up the provider, don't get to prescriptive about how it will be used. We expect anyone reading/writing a test will look at this setup function.

Suggestion:

fn make_provider() -> (L1GasPriceProvider, Vec<PriceSample>) {

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 24 at r6 (raw file):

        let sample = PriceSample { timestamp: time, base_fee_per_gas: price, blob_fee: price + 1 };
        provider.add_price_info(BlockNumber(block_num), sample).unwrap();
    }

The issue with using let price = i.try_into().unwrap(); is that it masks what range you are averaging over. avg(1, 2, 3, 4, 5) == avg(2, 3, 4).

I recommend building the vec manually (you explicitly write vec~ with 5 entries):

  • prices should be multiples of number_of_blocks_for_mean so tests don't need to worry about integer division issues
  • Make sure the prices are "random" (e.g. avoid the issue above).
    Explain these goes in a comment above the vec.

Code quote:

    for i in 0..5 {
        let block_num = i.try_into().unwrap();
        let price = i.try_into().unwrap();
        let time = (i * 2).try_into().unwrap();
        let sample = PriceSample { timestamp: time, base_fee_per_gas: price, blob_fee: price + 1 };
        provider.add_price_info(BlockNumber(block_num), sample).unwrap();
    }

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 36 at r6 (raw file):

    // This calculation will grab 3 samples from the middle of the range.
    let (gas_price, data_gas_price) =
        provider.get_price_info(BlockTimestamp(final_timestamp + lag)).unwrap();

If you really want to be pedantic we can return the config from make_provider_with_few_samples but this saves a bit of code. (do this throughout the tests)

Suggestion:

provider.config.lag

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 38 at r6 (raw file):

        provider.get_price_info(BlockTimestamp(final_timestamp + lag)).unwrap();
    // The gas prices (set arbitrarily to equal the block number) should go from
    let gas_price_calculation = (1 + 2 + 3) / 3;

Makes the test a little more flexible; you are committing this test to elements (1, 2, 3) instead of assuming the prices directly.

Suggestion:

    let gas_price_calculation = (samples[1].gas_fee + samples[2].gas_fee + samples[3].gas_fee) / 3;

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 40 at r6 (raw file):

    let gas_price_calculation = (1 + 2 + 3) / 3;
    // The data gas is one more than the gas price.
    let data_gas_calculation = gas_price_calculation + 1;

change this to be like the gas price

Code quote:

    // The data gas is one more than the gas price.
    let data_gas_calculation = gas_price_calculation + 1;

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 61 at r6 (raw file):

    // This should not change the results if we ask for the same timestamp.
    assert_eq!(gas_price, gas_price_new);
    assert_eq!(data_gas_price, data_gas_price_new);

Add another and check we get a missing data error

Code quote:

    // This should not change the results if we ask for the same timestamp.
    assert_eq!(gas_price, gas_price_new);
    assert_eq!(data_gas_price, data_gas_price_new);

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 75 at r6 (raw file):

        provider.get_price_info(BlockTimestamp(final_timestamp + lag + 2)).unwrap();
    assert!(gas_price_new > gas_price);
    assert!(data_gas_price_new > data_gas_price);

switching to multiplication shows the intent is to increase by another quanta of time, as opposed to adding 2 is a magic number based on what we set the lag to (and would need to be updated if we change the lag).

Suggestion:

    // If we take a higher timestamp the gas prices should change.
    let (gas_price_new, data_gas_price_new) =
        provider.get_price_info(BlockTimestamp(final_timestamp + lag * 2)).unwrap();
    assert_ne!(gas_price_new, gas_price);
    assert_ne!(data_gas_price_new, data_gas_price);

@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch 2 times, most recently from 09bc942 to c42acce Compare February 10, 2025 11:39
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 10 files reviewed, 11 unresolved discussions (waiting on @matan-starkware)


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 111 at r5 (raw file):

Previously, matan-starkware wrote…

Agreed. I thought we had a linter for this, CI should have failed.

I have used unwraps in several place before, never saw it show up on CI. I think in cases of trivial conversion (like integer types) this is not really an issue, but of course I'll go with common convensions.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 37 at r6 (raw file):

Previously, matan-starkware wrote…

Unused, remove?

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 24 at r6 (raw file):

Previously, matan-starkware wrote…

The issue with using let price = i.try_into().unwrap(); is that it masks what range you are averaging over. avg(1, 2, 3, 4, 5) == avg(2, 3, 4).

I recommend building the vec manually (you explicitly write vec~ with 5 entries):

  • prices should be multiples of number_of_blocks_for_mean so tests don't need to worry about integer division issues
  • Make sure the prices are "random" (e.g. avoid the issue above).
    Explain these goes in a comment above the vec.

I think I can accomplish most of this using (block number)^2. I'm not sure I can avoid integer division in all cases, and I don't think I should... we are explicitly calculating a mean of integers and will round down the results.

We should ask Product if they want us to use median instead, which is much better in this case, but that's a different matter.

WDYT?


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 36 at r6 (raw file):

Previously, matan-starkware wrote…

If you really want to be pedantic we can return the config from make_provider_with_few_samples but this saves a bit of code. (do this throughout the tests)

I don't understand what is the problem with the way it is now...


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 38 at r6 (raw file):

Previously, matan-starkware wrote…

Makes the test a little more flexible; you are committing this test to elements (1, 2, 3) instead of assuming the prices directly.

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 40 at r6 (raw file):

Previously, matan-starkware wrote…

change this to be like the gas price

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 61 at r6 (raw file):

Previously, matan-starkware wrote…

Add another and check we get a missing data error

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 75 at r6 (raw file):

Previously, matan-starkware wrote…

switching to multiplication shows the intent is to increase by another quanta of time, as opposed to adding 2 is a magic number based on what we set the lag to (and would need to be updated if we change the lag).

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs line 35 at r6 (raw file):

    InvalidHeight(String),
    #[error("Failed to add price info: {0}")]
    MissingData(String),

Done.

@matan-starkware matan-starkware self-requested a review February 10, 2025 13:02
Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware)


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 24 at r6 (raw file):

Previously, guy-starkware wrote…

I think I can accomplish most of this using (block number)^2. I'm not sure I can avoid integer division in all cases, and I don't think I should... we are explicitly calculating a mean of integers and will round down the results.

We should ask Product if they want us to use median instead, which is much better in this case, but that's a different matter.

WDYT?

Product has specified mean. The units in practice are so large this won't matter.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 36 at r6 (raw file):

Previously, guy-starkware wrote…

I don't understand what is the problem with the way it is now...

I'm trying to simplify the setup interface to return fewer things. The lag parameter is part of the config, so I'd rather access it as part of the config


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 16 at r8 (raw file):

// of 6 + 60 (which is the lag margin).
// Returns the provider, the final timestamp, and the lag margin.
fn make_provider_with_few_samples() -> (L1GasPriceProvider, u64, Vec<PriceSample>) {

I don't think we should have all these comments, the tests themselves show how to use this.

I also don't think we should return the final timestamp, we can use

    let (gas_price, data_gas_price) =
        provider.get_price_info(BlockTimestamp(samples.last().unwrap().timestamp)).unwrap();

(of you can use samples[len-1] + lag if you really want to).

Suggestion:

// Make a provider with each consecutive sample spaced `config.lag_margin_seconds` apart.
fn setup() -> (L1GasPriceProvider, Vec<PriceSample>) {

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 41 at r8 (raw file):

    // This calculation will grab 3 samples from the middle of the range.
    let (gas_price, data_gas_price) =
        provider.get_price_info(BlockTimestamp(final_timestamp + lag)).unwrap();

Let's just take the last timestamp instead of returning and adding

Suggestion:

    let (gas_price, data_gas_price) =
        provider.get_price_info(BlockTimestamp(samples.last().unwrap().timestamp)).unwrap();

crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 46 at r8 (raw file):

    let gas_price_calculation =
        (samples[1].base_fee_per_gas + samples[2].base_fee_per_gas + samples[3].base_fee_per_gas)
            / 3;

and data gas

Suggestion:

 config.num_samples;

@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from c42acce to 4de3321 Compare February 10, 2025 14:13
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @matan-starkware)


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 12 at r6 (raw file):

Previously, matan-starkware wrote…

Just set up the provider, don't get to prescriptive about how it will be used. We expect anyone reading/writing a test will look at this setup function.

see the comment below.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 24 at r6 (raw file):

Previously, matan-starkware wrote…

Product has specified mean. The units in practice are so large this won't matter.

Oh, I'm not worried about the integer division, I'm saying median will be better because it will not be sensitive to short spikes in the gas price, while still giving a good estimate of the prices. Of course this isn't something we can decide on by ourselves... but maybe worth telling them about it.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 36 at r6 (raw file):

Previously, matan-starkware wrote…

I'm trying to simplify the setup interface to return fewer things. The lag parameter is part of the config, so I'd rather access it as part of the config

OK, got it. I just noticed that I'm making the same let lag = ... statement every time I call the setup function, so I wanted to include it to save some boilerplate. But I don't really care either way.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 16 at r8 (raw file):

Previously, matan-starkware wrote…

I don't think we should have all these comments, the tests themselves show how to use this.

I also don't think we should return the final timestamp, we can use

    let (gas_price, data_gas_price) =
        provider.get_price_info(BlockTimestamp(samples.last().unwrap().timestamp)).unwrap();

(of you can use samples[len-1] + lag if you really want to).

I think there's a bit of confusion here between the lag and the block interval. As this is evidently not clear, I'm leaving the big comment.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 41 at r8 (raw file):

Previously, matan-starkware wrote…

Let's just take the last timestamp instead of returning and adding

The lag is not the same as the interval between samples. I still have to add lag because that tells the provider how far back to go to get the last sample to look at.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 46 at r8 (raw file):

Previously, matan-starkware wrote…

and data gas

Done.

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [35.572 ms 35.637 ms 35.717 ms]
change: [-3.9288% -2.4643% -1.1992%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severe

@matan-starkware matan-starkware self-requested a review February 11, 2025 07:43
Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 16 at r8 (raw file):

Previously, guy-starkware wrote…

I think there's a bit of confusion here between the lag and the block interval. As this is evidently not clear, I'm leaving the big comment.

Ah I thought you had changed the lag definition. All right I want us to change the comment though to be a little more general and also shorter. Here's a suggestion:

// Make a provider with five samples. Timestamps are 2 seconds apart, starting from 0.
// To get the prices for a specific set of blocks (according to the final block) use samples[final_index].timestamp + config.lag.
// Returns the provider and a vector of samples to compare with.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 37 at r9 (raw file):

    let (provider, samples) = make_provider();
    let lag = provider.config.lag_margin_seconds;
    let number: u128 = provider.config.number_of_blocks_for_mean.into();

Suggestion:

    let num_samples: u128 = provider.config.number_of_blocks_for_mean.into();

@guy-starkware guy-starkware force-pushed the guyn/l1price/provider_impl branch from 4de3321 to 96b7a48 Compare February 11, 2025 10:24
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 16 at r8 (raw file):

Previously, matan-starkware wrote…

Ah I thought you had changed the lag definition. All right I want us to change the comment though to be a little more general and also shorter. Here's a suggestion:

// Make a provider with five samples. Timestamps are 2 seconds apart, starting from 0.
// To get the prices for a specific set of blocks (according to the final block) use samples[final_index].timestamp + config.lag.
// Returns the provider and a vector of samples to compare with.

Done.


crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs line 37 at r9 (raw file):

    let (provider, samples) = make_provider();
    let lag = provider.config.lag_margin_seconds;
    let number: u128 = provider.config.number_of_blocks_for_mean.into();

Done.

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@guy-starkware guy-starkware added this pull request to the merge queue Feb 11, 2025
Merged via the queue into main with commit f126efc Feb 11, 2025
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants