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

test(starknet_batcher): setup gas prices for testing #2445

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@ArniStarkware ArniStarkware requested a review from alonh5 December 4, 2024 10:01
@ArniStarkware ArniStarkware marked this pull request as ready for review December 4, 2024 10:01
@ArniStarkware ArniStarkware changed the title test(batcher): setup gas prices for testing test(starknet_batcher): setup gas prices for testing Dec 4, 2024
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from 862c770 to 11f3e05 Compare December 4, 2024 10:03
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/starknet_api/src/block.rs line 456 at r1 (raw file):

// TODO(Arni): Remove derive of Default. Gas prices should always be set.
#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)]

I used this to compare the results. There is no need for this struct to implement these properties.

Suggestion:

#[derive(Clone, Debug, Default, Deserialize, Serialize)]

@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from 11f3e05 to 57d8ea0 Compare December 4, 2024 10:08
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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: 0 of 4 files reviewed, all discussions resolved (waiting on @alonh5)


crates/starknet_api/src/block.rs line 456 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I used this to compare the results. There is no need for this struct to implement these properties.

Done.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (arni/gas_prices/fix_default_strk_l1_gas_price@71ef06d). Learn more about missing BASE report.

Additional details and impacted files
@@                               Coverage Diff                                @@
##             arni/gas_prices/fix_default_strk_l1_gas_price    #2445   +/-   ##
================================================================================
  Coverage                                                 ?   80.02%           
================================================================================
  Files                                                    ?       98           
  Lines                                                    ?    13596           
  Branches                                                 ?    13596           
================================================================================
  Hits                                                     ?    10880           
  Misses                                                   ?     2232           
  Partials                                                 ?      484           

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

@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/fix_default_strk_l1_gas_price branch from 0dcf39a to 71ef06d Compare December 9, 2024 09:42
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from 57d8ea0 to f32d098 Compare December 9, 2024 09:42
Copy link
Collaborator

@alonh5 alonh5 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 4 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)


crates/starknet_api/src/test_utils.rs line 138 at r3 (raw file):

    NonzeroGasPrice::new_unchecked(GasPrice(25 * u128::pow(10, 5)));
pub const DEFAULT_STRK_L2_GAS_PRICE: NonzeroGasPrice =
    NonzeroGasPrice::new_unchecked(GasPrice(25 * u128::pow(10, 5)));

This is a good place to use static lazy if you want them to match the default l1 price.

Code quote:

pub const DEFAULT_ETH_L2_GAS_PRICE: NonzeroGasPrice =
    NonzeroGasPrice::new_unchecked(GasPrice(25 * u128::pow(10, 5)));
pub const DEFAULT_STRK_L2_GAS_PRICE: NonzeroGasPrice =
    NonzeroGasPrice::new_unchecked(GasPrice(25 * u128::pow(10, 5)));

@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from f32d098 to af99903 Compare December 16, 2024 15:49
@ArniStarkware ArniStarkware changed the base branch from arni/gas_prices/fix_default_strk_l1_gas_price to arni/test_utils/gas_prices/move_consts_to_snapi December 16, 2024 15:49
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/gas_prices/move_consts_to_snapi branch from 105bf9a to 3e497cf Compare December 16, 2024 18:31
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from af99903 to ff3cbab Compare December 16, 2024 18:31
Copy link

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [29.746 ms 29.794 ms 29.849 ms]
change: [-2.2122% -1.9862% -1.7662%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/gas_prices/move_consts_to_snapi branch from 3e497cf to dfca6ec Compare December 16, 2024 18:52
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from ff3cbab to 7df0d42 Compare December 16, 2024 18:52
Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.132 ms 30.193 ms 30.255 ms]
change: [+1.1434% +1.3841% +1.6358%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild

@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from 7df0d42 to fabc719 Compare December 17, 2024 07:56
@ArniStarkware ArniStarkware requested a review from alonh5 December 17, 2024 07:56
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/starknet_api/src/test_utils.rs line 138 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This is a good place to use static lazy if you want them to match the default l1 price.

  1. There is no need to match them to the l1 gas price.
  2. The mechanism that does the conversion is dependent on versioned constants, which are in the blockifier repo.
  3. If we put DEFAULT_ETH_L2_GAS_PRICE and DEFAULT_STRK_L2_GAS_PRICE inside a lazy evaluation block of any type, we would have to do the same for DEFAULT_GAS_PRICES. As I said in item 1 of this list - there is no benefit to that.

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/gas_prices/move_consts_to_snapi branch from dfca6ec to 5506889 Compare December 17, 2024 11:57
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from fabc719 to ee7467d Compare December 17, 2024 11:57
Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

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

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/gas_prices/move_consts_to_snapi branch from 5506889 to 3c92529 Compare December 18, 2024 11:24
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from ee7467d to 2db8500 Compare December 18, 2024 11:24
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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 r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

@ArniStarkware ArniStarkware changed the base branch from arni/test_utils/gas_prices/move_consts_to_snapi to graphite-base/2445 December 18, 2024 11:42
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from 2db8500 to 423ec5d Compare December 18, 2024 11:42
@ArniStarkware ArniStarkware changed the base branch from graphite-base/2445 to main December 18, 2024 11:42
@ArniStarkware ArniStarkware force-pushed the arni/gas_prices/default_for_testing/unchecked branch from 423ec5d to 00179e3 Compare December 18, 2024 11:43
Copy link
Contributor Author

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

Copy link
Contributor Author

ArniStarkware commented Dec 18, 2024

Merge activity

  • Dec 18, 7:09 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 18, 7:09 AM EST: A user merged this pull request with Graphite.

@ArniStarkware ArniStarkware merged commit 3fc110b into main Dec 18, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants