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,starknet_api): move l1handler to executable tx #1723

Conversation

yair-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

yair-starkware commented Oct 31, 2024

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

Join @yair-starkware and the rest of your teammates on Graphite Graphite

@yair-starkware yair-starkware marked this pull request as ready for review October 31, 2024 12:10
@yair-starkware yair-starkware force-pushed the 10-31-chore_blockifier_starknet_api_move_l1handler_to_executable_tx branch from 92fad74 to dafdb36 Compare October 31, 2024 12:16
Copy link

Artifacts upload triggered. View details here

@yair-starkware yair-starkware self-assigned this Oct 31, 2024
@yair-starkware yair-starkware force-pushed the 10-31-chore_blockifier_starknet_api_move_l1handler_to_executable_tx branch from dafdb36 to 2cae216 Compare October 31, 2024 12:28
Copy link

Artifacts upload triggered. View details here

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.74%. Comparing base (e3165c4) to head (a27848a).
Report is 159 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1723       +/-   ##
===========================================
+ Coverage   40.10%   65.74%   +25.64%     
===========================================
  Files          26      145      +119     
  Lines        1895    17839    +15944     
  Branches     1895    17839    +15944     
===========================================
+ Hits          760    11729    +10969     
- Misses       1100     5345     +4245     
- Partials       35      765      +730     

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

Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @giladchase, @noaov1, and @yair-starkware)

a discussion (no related file):
After looking into it again, I think we might have to add L1HandlerTransaction to the executable_transaction enum, because the transactin providers get_txs return type has to be consistent. I'm fine with adding it, even if the mempool can't return it. WDYT? @yair-starkware @noaov1 @giladchase



crates/blockifier/src/test_utils/struct_impls.rs line 227 at r2 (raw file):

}

pub trait CreateForTesting {

How about moving this impl also? Creating a trait just for a single impl block seems weird

Copy link
Collaborator

@noaov1 noaov1 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 7 files at r1.
Reviewable status: 1 of 7 files reviewed, 2 unresolved discussions (waiting on @giladchase and @yair-starkware)

a discussion (no related file):

get_txs return type has to be consistent

I see that it is currently divided into two providers (one for Transaction and one for L1HandlerTransaction. Anyway, it can be consistent by defining a Transaction enum that contains an account transaction and an L1 handler. I think the separation is required (the execution flows are different).
WDYT?


Copy link
Contributor Author

@yair-starkware yair-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: 1 of 7 files reviewed, 2 unresolved discussions (waiting on @alonh5, @giladchase, and @noaov1)

a discussion (no related file):

Previously, noaov1 (Noa Oved) wrote…

get_txs return type has to be consistent

I see that it is currently divided into two providers (one for Transaction and one for L1HandlerTransaction. Anyway, it can be consistent by defining a Transaction enum that contains an account transaction and an L1 handler. I think the separation is required (the execution flows are different).
WDYT?

We talked about it in the meeting.
Each provider can return its own type and the blockifier adds another type which is an enum of both of them.
We convert executable_transaction into the blockifier type anyways.



crates/blockifier/src/test_utils/struct_impls.rs line 227 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

How about moving this impl also? Creating a trait just for a single impl block seems weird

We don't have testing instances for the other types in SN_API so I don't want to introduce a new module just for that

Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase, @noaov1, and @yair-starkware)


crates/blockifier/src/test_utils/struct_impls.rs line 227 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

We don't have testing instances for the other types in SN_API so I don't want to introduce a new module just for that

You can add a cfg(test) over it

@yair-starkware
Copy link
Contributor Author

crates/blockifier/src/test_utils/struct_impls.rs line 227 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You can add a cfg(test) over it

It uses selector_from_namewhich is defined in blockifier

@yair-starkware yair-starkware force-pushed the 10-31-chore_blockifier_starknet_api_move_l1handler_to_executable_tx branch from 2cae216 to 46fb6bd Compare November 3, 2024 09:23
Copy link

github-actions bot commented Nov 3, 2024

Artifacts upload triggered. View details here

Copy link
Collaborator

@alonh5 alonh5 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 r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase, @noaov1, and @yair-starkware)


crates/blockifier/src/test_utils/struct_impls.rs line 227 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

It uses selector_from_namewhich is defined in blockifier

Okay so leave it here. Can you turn it into a regular function please?
l1_handler_tx() in a separate file like the other 3 txs

@yair-starkware yair-starkware force-pushed the 10-31-chore_blockifier_starknet_api_move_l1handler_to_executable_tx branch from 46fb6bd to a24007c Compare November 3, 2024 13:18
@yair-starkware yair-starkware force-pushed the 10-31-chore_blockifier_starknet_api_move_l1handler_to_executable_tx branch from a24007c to a27848a Compare November 3, 2024 13:24
Copy link

github-actions bot commented Nov 3, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware requested a review from alonh5 November 3, 2024 13:24
Copy link
Contributor Author

@yair-starkware yair-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 9 files reviewed, 1 unresolved discussion (waiting on @alonh5, @giladchase, and @noaov1)


crates/blockifier/src/test_utils/struct_impls.rs line 227 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Okay so leave it here. Can you turn it into a regular function please?
l1_handler_tx() in a separate file like the other 3 txs

Done.

Copy link
Collaborator

@alonh5 alonh5 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 2 of 5 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase and @noaov1)

@yair-starkware yair-starkware merged commit 6220d82 into main Nov 3, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 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.

4 participants