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_reexecution): add test cases #1796

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.81%. Comparing base (e3165c4) to head (368ebd1).
Report is 219 commits behind head on main.

Files with missing lines Patch % Lines
...s/blockifier_reexecution/src/state_reader/utils.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1796      +/-   ##
==========================================
+ Coverage   40.10%   41.81%   +1.70%     
==========================================
  Files          26      197     +171     
  Lines        1895    23310   +21415     
  Branches     1895    23310   +21415     
==========================================
+ Hits          760     9747    +8987     
- Misses       1100    13106   +12006     
- Partials       35      457     +422     

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

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from ff501b0 to 55cae6b Compare November 5, 2024 08:14
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from 7ea3ad5 to 5ec6337 Compare November 5, 2024 08:15
@AvivYossef-starkware AvivYossef-starkware marked this pull request as ready for review November 5, 2024 08:17
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 55cae6b to 5c024b1 Compare November 5, 2024 09:46
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from 5ec6337 to f46fb4f Compare November 5, 2024 09:46
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 5c024b1 to 55bf24c Compare November 5, 2024 10:40
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from f46fb4f to 7c9403b Compare November 5, 2024 10:40
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 55bf24c to 6caff94 Compare November 5, 2024 11:02
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from 7c9403b to a75a137 Compare November 5, 2024 11:02
Copy link
Collaborator

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

a discussion (no related file):
where is the usage of these changes? cannot review unused code, need context


@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 6caff94 to 5e03cc0 Compare November 5, 2024 14:19
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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 @aner-starkware and @dorimedini-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

where is the usage of these changes? cannot review unused code, need context

This is an example of all the cases that interest us. I ran the RPC test command with them,
and we might use them as examples in the CI.


Copy link
Collaborator

@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 @aner-starkware and @AvivYossef-starkware)

a discussion (no related file):

Previously, AvivYossef-starkware wrote…

This is an example of all the cases that interest us. I ran the RPC test command with them,
and we might use them as examples in the CI.

maybe add a block_numbers_for_reexecution function that

  1. reads the JSON, one per version of starknet after and including 0.13.0
  2. appends the specific block numbers with extra examples
    ?
    that way you'll only need #[allow(unused)] once, right?

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch 2 times, most recently from 4b59672 to 87a168f Compare November 6, 2024 07:47
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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 @aner-starkware and @dorimedini-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

maybe add a block_numbers_for_reexecution function that

  1. reads the JSON, one per version of starknet after and including 0.13.0
  2. appends the specific block numbers with extra examples
    ?
    that way you'll only need #[allow(unused)] once, right?

Do we want to add all possible examples for blocks with different tx types? ( total 12 blocks including the blocks per version )


@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 87a168f to ca502ec Compare November 6, 2024 08:16
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from a75a137 to 887cfad Compare November 6, 2024 08:16
Copy link
Collaborator

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

Reviewed 1 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

a discussion (no related file):

Previously, AvivYossef-starkware wrote…

Do we want to add all possible examples for blocks with different tx types? ( total 12 blocks including the blocks per version )

yes


@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch 2 times, most recently from 396ba95 to a095dc1 Compare November 6, 2024 12:00
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from 887cfad to 143cedd Compare November 6, 2024 12:00
Copy link
Collaborator

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch 2 times, most recently from 2efb4d5 to 5e0f94a Compare November 6, 2024 12:17
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from 143cedd to a452353 Compare November 6, 2024 12:17
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/blockifier_reexecution_add_l1_handler_tx to graphite-base/1796 November 6, 2024 12:31
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from a452353 to 56aa722 Compare November 6, 2024 12:31
@AvivYossef-starkware AvivYossef-starkware changed the base branch from graphite-base/1796 to main November 6, 2024 12:32
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch 2 times, most recently from e8aa6cd to c59fcc9 Compare November 6, 2024 13:54
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @dorimedini-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

yes

Done.


@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_cases branch from c59fcc9 to 368ebd1 Compare November 6, 2024 14:36
Copy link
Collaborator

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

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

a discussion (no related file):

Previously, AvivYossef-starkware wrote…

Done.

can you delete the constants with the special block numbers? if not, why are they still needed?


Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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 @aner-starkware and @dorimedini-starkware)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

can you delete the constants with the special block numbers? if not, why are they still needed?

We use them in the test file, I can fetch the JSON and extract them instead of using constants


Copy link
Collaborator

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

a discussion (no related file):

Previously, AvivYossef-starkware wrote…

We use them in the test file, I can fetch the JSON and extract them instead of using constants

can be in a separate PR... maybe we want something smarter, that can also be used to parametrize Aner's offline test


@AvivYossef-starkware AvivYossef-starkware merged commit ca0bf91 into main Nov 6, 2024
10 checks passed
Copy link
Contributor Author

Merge activity

  • Nov 6, 10:02 AM EST: A user merged this pull request with Graphite.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 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.

3 participants