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

refactor(blockifier): account_tx from enum to struct w api::executable_tx #1688

Merged

Conversation

avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 43f0616 to 057567e Compare October 30, 2024 14:50
Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [32.534 ms 32.665 ms 32.800 ms]
change: [-4.5625% -2.7976% -1.2002%] (p = 0.00 < 0.05)
Performance has improved.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 057567e to f3b0e65 Compare October 30, 2024 16:09
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from f3b0e65 to 16be6ad Compare October 30, 2024 16:49
@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/change_invoke_deploy_return_accounttx to avivg/blockifier/impl_classinfo_starknet_api October 30, 2024 16:50
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/impl_classinfo_starknet_api branch from 166b9ed to c01142e Compare November 4, 2024 12:46
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 16be6ad to e7fd5f5 Compare November 4, 2024 14:08
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from e7fd5f5 to 39c6b2b Compare November 4, 2024 14:11
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 62.90323% with 46 lines in your changes missing coverage. Please review.

Project coverage is 46.84%. Comparing base (e3165c4) to head (038d7b3).
Report is 316 commits behind head on main.

Files with missing lines Patch % Lines
...lockifier/src/transaction/transaction_execution.rs 9.09% 20 Missing ⚠️
.../blockifier/src/transaction/account_transaction.rs 78.87% 15 Missing ⚠️
crates/native_blockifier/src/py_transaction.rs 0.00% 8 Missing ⚠️
crates/native_blockifier/src/py_declare.rs 0.00% 1 Missing ⚠️
crates/native_blockifier/src/py_deploy_account.rs 0.00% 1 Missing ⚠️
crates/native_blockifier/src/py_invoke_function.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   40.10%   46.84%   +6.73%     
==========================================
  Files          26      222     +196     
  Lines        1895    25593   +23698     
  Branches     1895    25593   +23698     
==========================================
+ Hits          760    11988   +11228     
- Misses       1100    12810   +11710     
- Partials       35      795     +760     

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

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 39c6b2b to bd75e0d Compare November 4, 2024 18:11
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/impl_classinfo_starknet_api branch from c01142e to b9d906d Compare November 4, 2024 18:12
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from bd75e0d to c71386b Compare November 4, 2024 18:37
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/impl_classinfo_starknet_api branch 3 times, most recently from 011775d to 340ab90 Compare November 5, 2024 09:27
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from c71386b to 48d8645 Compare November 5, 2024 10:05
Copy link

github-actions bot commented Nov 5, 2024

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 48d8645 to 136ae0c Compare November 5, 2024 11:20
Copy link

github-actions bot commented Nov 5, 2024

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/impl_classinfo_starknet_api branch from 340ab90 to 977f134 Compare November 5, 2024 11:29
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 136ae0c to b4217e2 Compare November 5, 2024 12:02
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/switch_blockifier_classinfo_by_api_classinfo branch 2 times, most recently from e44509b to 0425ea9 Compare November 11, 2024 14:51
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 896b70a to a472a82 Compare November 11, 2024 14:52
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

@meship-starkware meship-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 12 of 14 files at r12.
Reviewable status: 27 of 29 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)

Copy link
Contributor

@meship-starkware meship-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 14 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)

Copy link
Contributor Author

@avivg-starkware avivg-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, 3 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/transaction/account_transaction.rs line 87 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can this be public? If so, consider removing the new and new_for_query methods.

Yes, will take care in a separate PR


crates/blockifier/src/transaction/account_transaction.rs line 100 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider moving some of the logic (e.g. the match) to the Transaction enum in starknet api.

Sounds good, adding a todo, to consider this change separately.


crates/blockifier/src/transaction/account_transaction.rs line 131 at r9 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider moving this logic to starknet api
Applies to all the getters methods in this file.
(can be done in a separate PR)

Will address in a separate PR

some elaboration: I was considering doing so ( for each fn it's slightly different and there wasn't any simple conversion that worked for all).
I chose to keep it simple for now to make all changes here easier to follow.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/switch_blockifier_classinfo_by_api_classinfo branch from 0425ea9 to 13baecd Compare November 11, 2024 17:35
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from a472a82 to 9733a98 Compare November 11, 2024 17:35
Copy link

Artifacts upload triggered. View details here

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 1 of 2 files at r13, all commit messages.
Reviewable status: 28 of 29 files reviewed, all discussions resolved (waiting on @meship-starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/switch_blockifier_classinfo_by_api_classinfo branch from 13baecd to 39f1077 Compare November 12, 2024 07:33
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 9733a98 to 3f08cb4 Compare November 12, 2024 07:33
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/switch_blockifier_classinfo_by_api_classinfo to graphite-base/1688 November 12, 2024 08:43
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 3f08cb4 to 2330759 Compare November 12, 2024 08:44
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware changed the base branch from graphite-base/1688 to main November 12, 2024 08:44
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/create_struct_account_transaction branch from 2330759 to 038d7b3 Compare November 12, 2024 08:44
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@avivg-starkware avivg-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 5 of 24 files at r3, 2 of 7 files at r4, 1 of 12 files at r5, 1 of 6 files at r6, 3 of 6 files at r7, 3 of 14 files at r10, 12 of 14 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware dismissed noaov1’s stale review November 12, 2024 09:25

all addressed. approved verbally. Noa on days off

@avivg-starkware avivg-starkware merged commit 5911b9c into main Nov 12, 2024
12 checks passed
@avivg-starkware avivg-starkware deleted the avivg/blockifier/create_struct_account_transaction branch November 12, 2024 14:23
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 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.

5 participants