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

chore(blockifier): added tests for StarknetResources::to_gas_vector #918

Open
wants to merge 1 commit into
base: amos/rename_max_resource_bounds
Choose a base branch
from

Conversation

amosStarkware
Copy link
Contributor

@amosStarkware amosStarkware commented Sep 22, 2024

This change is Reviewable

@amosStarkware amosStarkware force-pushed the amos/add_l2_gas_tests_1 branch 2 times, most recently from eb88b63 to f45fc68 Compare September 22, 2024 12:11
@amosStarkware amosStarkware changed the title WIP chore(blockifier): added tests for TransactionResources::to_gas_vector Sep 22, 2024
Copy link
Contributor Author

@amosStarkware amosStarkware 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


crates/blockifier/src/transaction/transactions_test.rs line 1946 at r1 (raw file):

            true => GasVector { l1_gas: 16023, l1_data_gas: 128, l2_gas: 25 },
            false => GasVector { l1_gas: 17675, l1_data_gas: 0, l2_gas: 25 },
        },

I copied these values from the expected values (i.e., I ran the test, it failed, and I copied the expected values)

Code quote:

        GasVectorComputationMode::All => match use_kzg_da {
            true => GasVector { l1_gas: 16023, l1_data_gas: 128, l2_gas: 25 },
            false => GasVector { l1_gas: 17675, l1_data_gas: 0, l2_gas: 25 },
        },

Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.30%. Comparing base (1bc08cb) to head (1a90c24).

Additional details and impacted files
@@                         Coverage Diff                         @@
##           amos/rename_max_resource_bounds     #918      +/-   ##
===================================================================
+ Coverage                            70.27%   70.30%   +0.02%     
===================================================================
  Files                                   87       87              
  Lines                                11109    11109              
  Branches                             11109    11109              
===================================================================
+ Hits                                  7807     7810       +3     
+ Misses                                2914     2911       -3     
  Partials                               388      388              
Flag Coverage Δ
70.30% <ø> (+0.02%) ⬆️

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.

Copy link
Contributor Author

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

+reviewer:@dorimedini-starkware

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

Copy link
Contributor Author

@amosStarkware amosStarkware 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, 2 unresolved discussions (waiting on @dorimedini-starkware)


-- commits line 1 at r1:
should I add L2 gas test cases to any more tests?

Code quote:

New commits in r1 on 22/09/2024 at 15:11:

Copy link
Contributor Author

@amosStarkware amosStarkware 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, 3 unresolved discussions (waiting on @dorimedini-starkware)


crates/blockifier/src/fee/receipt_test.rs line 441 at r1 (raw file):

        None,
        // The transfer entrypoint emits an event - pass the call info to count its resources.
        execution_call_info_iter,

This wasn't needed until now because in the NoL2Gas computation mode it was rounded to zero

Code quote:

        // The transfer entrypoint emits an event - pass the call info to count its resources.
        execution_call_info_iter,

Copy link
Contributor Author

@amosStarkware amosStarkware 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, 4 unresolved discussions (waiting on @dorimedini-starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1887 at r1 (raw file):

#[rstest]
fn test_l1_handler(

this is probably out of bounds for this PR, but I already did it so makes sense to add it

Code quote:

fn test_l1_handler(

@amosStarkware amosStarkware changed the title chore(blockifier): added tests for TransactionResources::to_gas_vector chore(blockifier): added tests for StarknetResources::to_gas_vector Sep 24, 2024
Copy link
Contributor Author

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

+reviewer:@aner-starkware

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

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.

1 participant