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

build(blockifier_reexecution): add support for l1 handler tx #1765

Merged

Conversation

AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

codecov bot commented Nov 3, 2024

Codecov Report

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

Project coverage is 41.76%. Comparing base (e3165c4) to head (5e0f94a).
Report is 217 commits behind head on main.

Files with missing lines Patch % Lines
...ution/src/state_reader/reexecution_state_reader.rs 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1765      +/-   ##
==========================================
+ Coverage   40.10%   41.76%   +1.66%     
==========================================
  Files          26      197     +171     
  Lines        1895    23309   +21414     
  Branches     1895    23309   +21414     
==========================================
+ Hits          760     9736    +8976     
- Misses       1100    13116   +12016     
- Partials       35      457     +422     

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

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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)


crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs line 45 at r1 (raw file):

                Transaction::Invoke(_) | Transaction::DeployAccount(_) => {
                    BlockifierTransaction::from_api(tx, tx_hash, None, None, None, false)
                        .map_err(ReexecutionError::from)

do you really need these?
why not replace with ??

times 3

Code quote:

.map_err(ReexecutionError::from)

crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs line 68 at r1 (raw file):

                        tx_hash,
                        None,
                        Some(Fee(u128::MAX)),

there is a constant MAX_FEE in the blockifier, isn't it public?

Suggestion:

Some(Fee(MAX_FEE))

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch from 96eeca3 to f31706d Compare November 4, 2024 10:56
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from c85ea0e to cad4312 Compare November 4, 2024 10:56
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 1 files at r2, 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_fix_bug_in_get_contract_class branch from f31706d to d339e52 Compare November 4, 2024 15:25
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from cad4312 to ff501b0 Compare November 4, 2024 15:25
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch 2 times, most recently from 12fe759 to 208e455 Compare November 5, 2024 08:09
@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
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: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @dorimedini-starkware)


crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs line 45 at r1 (raw file):

Previously, dorimedini-starkware wrote…

do you really need these?
why not replace with ??

times 3

it works


crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs line 68 at r1 (raw file):

Previously, dorimedini-starkware wrote…

there is a constant MAX_FEE in the blockifier, isn't it public?

Are you sure? I don't find it

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch from 208e455 to 07d6e9e Compare November 5, 2024 09:45
@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_fix_bug_in_get_contract_class branch from 07d6e9e to 6458aac Compare November 5, 2024 10:40
@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_fix_bug_in_get_contract_class branch from 6458aac to a2eb905 Compare November 5, 2024 10:58
@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
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 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)


crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs line 68 at r1 (raw file):

Previously, AvivYossef-starkware wrote…

Are you sure? I don't find it

here

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch from a2eb905 to 625dd68 Compare November 5, 2024 14:18
@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: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @dorimedini-starkware)


crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs line 68 at r1 (raw file):

Previously, dorimedini-starkware wrote…

here

Thanks

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:

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

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch from 625dd68 to b93bc31 Compare November 5, 2024 15:40
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 5e03cc0 to 4b59672 Compare November 5, 2024 15:41
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch from b93bc31 to c728fca Compare November 6, 2024 07:47
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch 2 times, most recently from 87a168f to ca502ec 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 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch from c728fca to 5993328 Compare November 6, 2024 11:36
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from ca502ec to 396ba95 Compare November 6, 2024 11: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 4 of 4 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_fix_bug_in_get_contract_class branch 2 times, most recently from 97eee29 to 68a18f9 Compare November 6, 2024 12:00
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 396ba95 to a095dc1 Compare November 6, 2024 12:00
@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/blockifier_reexecution_fix_bug_in_get_contract_class to graphite-base/1765 November 6, 2024 12:16
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from a095dc1 to 2efb4d5 Compare November 6, 2024 12:16
@AvivYossef-starkware AvivYossef-starkware changed the base branch from graphite-base/1765 to main November 6, 2024 12:17
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/blockifier_reexecution_add_l1_handler_tx branch from 2efb4d5 to 5e0f94a Compare November 6, 2024 12:17
@AvivYossef-starkware AvivYossef-starkware merged commit b18eb32 into main Nov 6, 2024
10 checks passed
Copy link
Contributor Author

Merge activity

  • Nov 6, 7:31 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