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(blockifier): update test_revert_on_resource_overuse to include new resource bounds #1314

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Oct 10, 2024

This change is Reviewable

@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review October 10, 2024 14:48
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from a400602 to e569b59 Compare October 10, 2024 15:11
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch 4 times, most recently from b5ed508 to c513c51 Compare October 10, 2024 16:04
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.15%. Comparing base (e3165c4) to head (c48181f).
Report is 84 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1314       +/-   ##
===========================================
+ Coverage   40.10%   69.15%   +29.04%     
===========================================
  Files          26      100       +74     
  Lines        1895    13447    +11552     
  Branches     1895    13447    +11552     
===========================================
+ Hits          760     9299     +8539     
- Misses       1100     3747     +2647     
- Partials       35      401      +366     

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

@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from e569b59 to 26404f4 Compare October 13, 2024 08:13
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from c513c51 to 4c7d6a1 Compare October 13, 2024 08:13
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from 26404f4 to 1e42bb3 Compare October 13, 2024 08:16
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 4c7d6a1 to 88806ee Compare October 13, 2024 08:16
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from 1e42bb3 to b0bcf2b Compare October 13, 2024 08:36
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 88806ee to 657a9af Compare October 13, 2024 08:36
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from b0bcf2b to 241f6da Compare October 13, 2024 12:39
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 657a9af to ca28099 Compare October 13, 2024 12:39
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 427f99a to 3d0bf82 Compare October 28, 2024 14:48
Copy link

Artifacts upload triggered. View details here

@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch 2 times, most recently from 52ba961 to 0b8953a Compare October 29, 2024 10:54
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 3d0bf82 to 83bd562 Compare October 29, 2024 10:54
Copy link

Artifacts upload triggered. View details here

@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from 0b8953a to 6de4946 Compare October 29, 2024 15:30
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 83bd562 to 3096d35 Compare October 29, 2024 15:30
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

@yoavGrs yoavGrs left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)


crates/blockifier/feature_contracts/cairo0/test_contract.cairo line 100 at r2 (raw file):

        return ();
    }
    send_message(7);

Why do you need it?

Code quote:

send_message(7);

crates/blockifier/src/transaction/post_execution_test.rs line 339 at r2 (raw file):

    // reverted.
    let low_max_fee = Fee(execution_info_measure.receipt.fee.0 - 1);
    let low_bounds = if version != TransactionVersion::THREE {

For future versions.

Suggestion:

if version < TransactionVersion::THREE

@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds branch from 6de4946 to f0f7db2 Compare October 30, 2024 11:37
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 3096d35 to 161c029 Compare October 30, 2024 11:37
Copy link

Artifacts upload triggered. View details here

@dorimedini-starkware dorimedini-starkware changed the base branch from 10-10-test_blockifier_update_test_insufficient_max_fee_reverts_to_include_new_resource_bounds to graphite-base/1314 October 30, 2024 12:17
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 161c029 to 4f52227 Compare October 30, 2024 12:17
@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/1314 to main October 30, 2024 12:18
@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from 4f52227 to e72e877 Compare October 30, 2024 12:18
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@dorimedini-starkware dorimedini-starkware force-pushed the 10-10-test_blockifier_update_test_revert_on_resource_overuse_to_include_new_resource_bounds branch from e72e877 to c48181f Compare October 30, 2024 12:23
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-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 @yoavGrs)


crates/blockifier/feature_contracts/cairo0/test_contract.cairo line 100 at r2 (raw file):

Previously, yoavGrs wrote…

Why do you need it?

for the all_bounds_insufficient_l1_gas case.
we need something that costs L1 gas even when user is paying for L2 stuff with L2 gas

Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

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

@dorimedini-starkware dorimedini-starkware merged commit 2e2fe25 into main Oct 30, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 3, 2024
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.

2 participants