test(blockifier): test contract for validate and execute merging #2456

yoavGrs · 2024-12-04T14:01:01Z

No description provided.

reviewable-StarkWare · 2024-12-04T14:01:13Z

This change is

yoavGrs · 2024-12-04T14:01:29Z

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

github-actions · 2024-12-04T14:02:34Z

Artifacts upload workflows:

yoavGrs

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

a discussion (no related file):
There are too many tests for the same function. I suggest deleting test_state_cache_merge and test_allocated_keys_two_transactions. WDYT?

codecov · 2024-12-04T14:15:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.99%. Comparing base (e3165c4) to head (96db9a1).
Report is 743 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2456       +/-   ##
===========================================
+ Coverage   40.10%   71.99%   +31.88%     
===========================================
  Files          26      101       +75     
  Lines        1895    13518    +11623     
  Branches     1895    13518    +11623     
===========================================
+ Hits          760     9732     +8972     
- Misses       1100     3371     +2271     
- Partials       35      415      +380

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

dorimedini-starkware

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @yoavGrs)

a discussion (no related file):

Previously, yoavGrs wrote…

There are too many tests for the same function. I suggest deleting test_state_cache_merge and test_allocated_keys_two_transactions. WDYT?

as long as the union of scenarios and union of assertions exist - sgtm

crates/blockifier/src/state/cached_state_test.rs line 490 at r1 (raw file):

#[case(true, felt!("0x7"), felt!("0x7"), true)]
#[case(false, felt!("0x0"), felt!("0x8"), false)]
#[case(true, felt!("0x7"), felt!("0x0"), false)]

give names to cases like this

Suggestion:

#[case::real_diff_nonzero_base(false, felt!("0x7"), felt!("0x8"), false)]
#[case::execute_allocation(true, felt!("0x0"), felt!("0x8"), true)]
#[case::validate_allocation(true, felt!("0x7"), felt!("0x7"), true)]
#[case::nonzero_to_zero_to_nonzero(false, felt!("0x0"), felt!("0x8"), false)]
#[case::zero_to_nonzero_to_zero(true, felt!("0x7"), felt!("0x0"), false)]

crates/blockifier/src/state/cached_state_test.rs line 516 at r1 (raw file):

    });
    let tx_execution_info =
        invoke_tx.execute(&mut transactional_state, &block_context, false, true).unwrap();

you shouldn't need to create a transactional state

Suggestion:

    let signature =
        TransactionSignature(vec![Felt::from(STORAGE_WRITE), validate_value, execute_value]);
    let invoke_tx = account_invoke_tx(invoke_tx_args! {
        signature,
        sender_address: contract_address,
        calldata: create_calldata(contract_address, "foo", &[]),
    });
    let tx_execution_info =
        invoke_tx.execute(&mut state, &block_context, false, true).unwrap();

crates/blockifier/src/state/cached_state_test.rs line 516 at r1 (raw file):

    });
    let tx_execution_info =
        invoke_tx.execute(&mut transactional_state, &block_context, false, true).unwrap();

why did you set charge_fee to false here?

Code quote:

false

crates/blockifier/feature_contracts/cairo1/account_faulty.cairo line 0 at r1 (raw file):
can you add the same functionality to the cairo0 contract?
that way we will be able to parametrize the test to both cairo versions

dorimedini-starkware

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @yoavGrs)

yoavGrs

Reviewable status: 1 of 8 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

as long as the union of scenarios and union of assertions exist - sgtm

So I'll delete only the second.

crates/blockifier/feature_contracts/cairo1/account_faulty.cairo line at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you add the same functionality to the cairo0 contract?
that way we will be able to parametrize the test to both cairo versions

Done.

crates/blockifier/src/state/cached_state_test.rs line 490 at r1 (raw file):

Previously, dorimedini-starkware wrote…

give names to cases like this

Done.

crates/blockifier/src/state/cached_state_test.rs line 516 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why did you set charge_fee to false here?

Changed.

crates/blockifier/src/state/cached_state_test.rs line 516 at r1 (raw file):

Previously, dorimedini-starkware wrote…

you shouldn't need to create a transactional state

Done.

dorimedini-starkware

Reviewed 6 of 7 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

dorimedini-starkware

Reviewed 1159 of 1159 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs)

a discussion (no related file):
remove the target_vscode from this PR...
where did it come from? did you set up your vscode in some non-standard way? or should this dir be added to everyone's gitignore?

dorimedini-starkware

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs)

yoavGrs

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

a discussion (no related file):

Previously, dorimedini-starkware wrote…

remove the target_vscode from this PR...
where did it come from? did you set up your vscode in some non-standard way? or should this dir be added to everyone's gitignore?

Done.
Maybe it should be added to '.gitignore'. Let's talk offline.

dorimedini-starkware

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs)

a discussion (no related file):

Previously, yoavGrs wrote…

Done.
Maybe it should be added to '.gitignore'. Let's talk offline.

I still see the files in this PR..

dorimedini-starkware

Reviewed 1158 of 1158 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs)

dorimedini-starkware

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

yoavGrs mentioned this pull request Dec 4, 2024

feat(blockifier): add comprehensive state diff versioned constant #2407

Merged

yoavGrs requested a review from dorimedini-starkware December 4, 2024 14:01

yoavGrs marked this pull request as ready for review December 4, 2024 14:01

yoavGrs commented Dec 4, 2024

View reviewed changes

dorimedini-starkware requested changes Dec 4, 2024

View reviewed changes

yoavGrs force-pushed the yoav/compression/merge_state_diff branch from 9a66da9 to bd621f5 Compare December 4, 2024 14:21

yoavGrs force-pushed the yoav/compression/test_contract branch from 3ef88f8 to 4bec2fe Compare December 4, 2024 14:21

dorimedini-starkware reviewed Dec 4, 2024

View reviewed changes

yoavGrs force-pushed the yoav/compression/test_contract branch from 4bec2fe to 42b1ca0 Compare December 4, 2024 16:10

yoavGrs commented Dec 4, 2024

View reviewed changes

yoavGrs force-pushed the yoav/compression/test_contract branch from 42b1ca0 to ce766a8 Compare December 4, 2024 16:20

dorimedini-starkware approved these changes Dec 4, 2024

View reviewed changes

yoavGrs force-pushed the yoav/compression/merge_state_diff branch 2 times, most recently from 923bc81 to c9454fc Compare December 4, 2024 16:44

yoavGrs force-pushed the yoav/compression/test_contract branch from ce766a8 to 24761cc Compare December 4, 2024 16:44

yoavGrs force-pushed the yoav/compression/merge_state_diff branch 2 times, most recently from 3a7681a to fb29524 Compare December 5, 2024 09:13

yoavGrs force-pushed the yoav/compression/test_contract branch from 24761cc to 27c808f Compare December 5, 2024 09:13

yoavGrs force-pushed the yoav/compression/merge_state_diff branch from fb29524 to dc573fe Compare December 5, 2024 10:00

yoavGrs force-pushed the yoav/compression/test_contract branch from 27c808f to fb3d179 Compare December 5, 2024 10:01

yoavGrs mentioned this pull request Dec 5, 2024

feat(blockifier): get revert receipt only in case of revert #2471

Merged

dorimedini-starkware requested changes Dec 5, 2024

View reviewed changes

yoavGrs force-pushed the yoav/compression/merge_state_diff branch from dc573fe to 5861d2a Compare December 5, 2024 10:53

yoavGrs force-pushed the yoav/compression/test_contract branch from fb3d179 to 7c7f02d Compare December 5, 2024 10:57

yoavGrs changed the base branch from yoav/compression/merge_state_diff to graphite-base/2456 December 5, 2024 12:08

yoavGrs force-pushed the graphite-base/2456 branch from 5861d2a to 61340f7 Compare December 5, 2024 12:26

yoavGrs force-pushed the yoav/compression/test_contract branch from 7c7f02d to 4fb6d8f Compare December 5, 2024 12:26

yoavGrs changed the base branch from graphite-base/2456 to main December 5, 2024 12:26

yoavGrs force-pushed the yoav/compression/test_contract branch from 4fb6d8f to 4312ac1 Compare December 5, 2024 12:30

dorimedini-starkware reviewed Dec 5, 2024

View reviewed changes

yoavGrs commented Dec 5, 2024

View reviewed changes

dorimedini-starkware requested changes Dec 5, 2024

View reviewed changes

test(blockifier): test contract for validate and execute merging

96db9a1

yoavGrs force-pushed the yoav/compression/test_contract branch from 4312ac1 to 96db9a1 Compare December 5, 2024 15:40

dorimedini-starkware reviewed Dec 5, 2024

View reviewed changes

dorimedini-starkware approved these changes Dec 5, 2024

View reviewed changes

yoavGrs merged commit c97fe52 into main Dec 8, 2024
14 checks passed

github-actions bot locked and limited conversation to collaborators Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(blockifier): test contract for validate and execute merging #2456

test(blockifier): test contract for validate and execute merging #2456

yoavGrs commented Dec 4, 2024

reviewable-StarkWare commented Dec 4, 2024

yoavGrs commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

yoavGrs left a comment

codecov bot commented Dec 4, 2024 •

edited

Loading

dorimedini-starkware left a comment

dorimedini-starkware left a comment

yoavGrs left a comment

dorimedini-starkware left a comment

dorimedini-starkware left a comment

dorimedini-starkware left a comment

yoavGrs left a comment

dorimedini-starkware left a comment

dorimedini-starkware left a comment

dorimedini-starkware left a comment

test(blockifier): test contract for validate and execute merging #2456

test(blockifier): test contract for validate and execute merging #2456

Conversation

yoavGrs commented Dec 4, 2024

reviewable-StarkWare commented Dec 4, 2024

yoavGrs commented Dec 4, 2024 • edited Loading

github-actions bot commented Dec 4, 2024 • edited Loading

yoavGrs left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

yoavGrs left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

yoavGrs left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

dorimedini-starkware left a comment

Choose a reason for hiding this comment

yoavGrs commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading