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.

Sign up for GitHub

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

Jump to bottom

refactor: wrap executable tx in account/l1handler enum #1788

Merged
merged 1 commit into from
Nov 7, 2024
Merged

refactor: wrap executable tx in account/l1handler enum #1788

merged 1 commit into from
Nov 7, 2024

Conversation

yair-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@yair-starkware Graphite App
Copy link
Contributor Author

yair-starkware commented Nov 4, 2024 •
edited
Loading

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 mentioned this pull request Nov 4, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware requested a review from alonh5 November 4, 2024 12:24
@yair-starkware yair-starkware marked this pull request as ready for review November 4, 2024 12:24
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.440 ms 34.474 ms 34.509 ms]
change: [-4.8260% -3.2346% -1.8217%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild

@codecov Codecov
Copy link

codecov bot commented Nov 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 48.14815% with 28 lines in your changes missing coverage. Please review.

Project coverage is 45.36%. Comparing base (e3165c4) to head (75fcd63).
Report is 233 commits behind head on main.

Files with missing lines Patch % Lines
.../blockifier/src/transaction/account_transaction.rs 56.25% 7 Missing ⚠️
crates/batcher/src/transaction_provider.rs 0.00% 6 Missing ⚠️
crates/mempool/src/communication.rs 0.00% 6 Missing ⚠️
...lockifier/src/transaction/transaction_execution.rs 66.66% 2 Missing and 1 partial ⚠️
crates/starknet_api/src/executable_transaction.rs 57.14% 3 Missing ⚠️
crates/mempool_types/src/communication.rs 0.00% 2 Missing ⚠️
crates/batcher/src/block_builder.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1788      +/-   ##
==========================================
+ Coverage   40.10%   45.36%   +5.25%     
==========================================
  Files          26      254     +228     
  Lines        1895    30967   +29072     
  Branches     1895    30967   +29072     
==========================================
+ Hits          760    14047   +13287     
- Misses       1100    16099   +14999     
- Partials       35      821     +786

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

@yair-starkware yair-starkware force-pushed the yair/rename_executable_tx branch from 4d00569 to dd144cb Compare November 4, 2024 13:23
@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from 44d4e91 to efcdbfc Compare November 4, 2024 13:23
@yair-starkware yair-starkware self-assigned this Nov 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware force-pushed the yair/rename_executable_tx branch from dd144cb to dc514f9 Compare November 4, 2024 13:28
@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from efcdbfc to 579bc18 Compare November 4, 2024 13:28
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware force-pushed the yair/rename_executable_tx branch from dc514f9 to 710db0e Compare November 4, 2024 13:44
@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from 579bc18 to cff8057 Compare November 4, 2024 13:44
@yair-starkware yair-starkware mentioned this pull request Nov 4, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from cff8057 to b0da47a Compare November 4, 2024 13:57
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

alonh5
alonh5 approved these changes Nov 4, 2024
View reviewed changes
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 9 of 20 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

ArniStarkware
ArniStarkware reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@ArniStarkware ArniStarkware 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 11 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

@ArniStarkware
Copy link
Contributor

crates/blockifier/src/transaction/transaction_execution.rs line 50 at r2 (raw file):

            }
            starknet_api::executable_transaction::Transaction::L1Handler(tx) => {
                Ok(Transaction::L1Handler(tx.clone()))

Shouldn't the TryFrom take ownership of the struct?

Suggestion:

Ok(Transaction::L1Handler(tx))

@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from b0da47a to 6201fee Compare November 7, 2024 08:59
yair-starkware
yair-starkware commented Nov 7, 2024
View reviewed changes
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: 18 of 20 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ArniStarkware)

crates/blockifier/src/transaction/transaction_execution.rs line 50 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Shouldn't the TryFrom take ownership of the struct?

Done.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

ArniStarkware
ArniStarkware reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@ArniStarkware ArniStarkware 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 r3.
Reviewable status: 19 of 20 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @yair-starkware)

@yair-starkware yair-starkware force-pushed the yair/rename_executable_tx branch from 710db0e to 7ad1b50 Compare November 7, 2024 09:26
@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from 6201fee to 1a245f8 Compare November 7, 2024 09:26
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from 1a245f8 to c944056 Compare November 7, 2024 09:36
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.869 ms 33.960 ms 34.044 ms]
change: [-4.7867% -3.0046% -1.4312%] (p = 0.00 < 0.05)
Performance has improved.
Found 18 outliers among 100 measurements (18.00%)
4 (4.00%) low severe
13 (13.00%) low mild
1 (1.00%) high mild

@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from c944056 to 1641e09 Compare November 7, 2024 09:49
yair-starkware
yair-starkware commented Nov 7, 2024
View reviewed changes
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: 11 of 20 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ArniStarkware)

crates/blockifier/src/transaction/transaction_execution.rs line 50 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Remove the clone :)

Done.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

ArniStarkware
ArniStarkware reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@ArniStarkware ArniStarkware 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 r5.
Reviewable status: 12 of 20 files reviewed, 1 unresolved discussion (waiting on @alonh5)

@yair-starkware yair-starkware changed the base branch from yair/rename_executable_tx to graphite-base/1788 November 7, 2024 09:57
@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from 1641e09 to 7bba7db Compare November 7, 2024 09:58
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

@yair-starkware yair-starkware changed the base branch from graphite-base/1788 to main November 7, 2024 09:58
@yair-starkware yair-starkware force-pushed the yair/executable_tx_enum branch from 7bba7db to 75fcd63 Compare November 7, 2024 09:58
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2024

Artifacts upload triggered. View details here

ArniStarkware
ArniStarkware reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@ArniStarkware ArniStarkware 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 11 files at r2, 1 of 8 files at r4.
Reviewable status: 13 of 20 files reviewed, all discussions resolved (waiting on @alonh5)

ArniStarkware
ArniStarkware approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@ArniStarkware ArniStarkware 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 20 files at r1, 4 of 11 files at r2, 6 of 8 files at r4, 1 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alonh5)

@yair-starkware yair-starkware merged commit 2084527 into main Nov 7, 2024
21 checks passed
@yair-starkware Graphite App
Copy link
Contributor Author

Merge activity

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

This was referenced Nov 7, 2024
Merged
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ArniStarkware ArniStarkware ArniStarkware approved these changes

@alonh5 alonh5 Awaiting requested review from alonh5

Assignees

@yair-starkware yair-starkware

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yair-starkware @reviewable-StarkWare @ArniStarkware @alonh5