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

test: streamline the v0 version tx for invoke tx towards code dedup in test utils #747

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Sep 9, 2024

This change is Reviewable

Copy link
Contributor Author

ArniStarkware commented Sep 9, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

Join @ArniStarkware and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Sep 9, 2024

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.458 ms 65.718 ms 66.130 ms]
change: [-8.8578% -5.4025% -2.3102%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe

@ArniStarkware ArniStarkware changed the base branch from arni/test_utils/tx_creator/executable_tx to graphite-base/747 September 9, 2024 11:49
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 73ecaad to baf5737 Compare September 9, 2024 11:52
@ArniStarkware ArniStarkware changed the base branch from graphite-base/747 to main September 9, 2024 11:52
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.61%. Comparing base (2cf3d70) to head (fd8a886).

Additional details and impacted files
@@                              Coverage Diff                               @@
##           arni/test_utils/deploy_account/blockifier     #747       +/-   ##
==============================================================================
+ Coverage                                      49.00%   76.61%   +27.61%     
==============================================================================
  Files                                            195      102       -93     
  Lines                                          23300    13677     -9623     
  Branches                                       23300    13677     -9623     
==============================================================================
- Hits                                           11418    10479      -939     
+ Misses                                         11367     2742     -8625     
+ Partials                                         515      456       -59     

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

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from baf5737 to 51c315a Compare September 9, 2024 14:06
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 51c315a to e36c0d0 Compare September 15, 2024 10:46
@ArniStarkware ArniStarkware changed the base branch from main to arni/test_utils/executable_tx_creator/align_declare_with_account_tx September 15, 2024 10:46
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.337 ms 66.401 ms 66.473 ms]
change: [-8.3913% -5.1461% -2.2775%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/executable_tx_creator/align_declare_with_account_tx branch from c569fec to bf37c40 Compare September 15, 2024 13:07
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from e36c0d0 to ce0392e Compare September 15, 2024 13:07
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.467 ms 66.569 ms 66.674 ms]
change: [-8.9706% -5.7897% -3.1217%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe

Copy link
Contributor

@MohammadNassar1 MohammadNassar1 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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

a discussion (no related file):
Can you please explain the change here?
Why does V0 have a special treatment, and why is it no longer necessary?


@ArniStarkware ArniStarkware changed the base branch from arni/test_utils/executable_tx_creator/align_declare_with_account_tx to graphite-base/747 September 15, 2024 14:46
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from ce0392e to 1608275 Compare September 15, 2024 14:47
@ArniStarkware ArniStarkware changed the base branch from graphite-base/747 to main September 15, 2024 14:47
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 1608275 to 7a04c94 Compare September 15, 2024 14:47
Copy link
Contributor Author

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

a discussion (no related file):
For version 0 transactions, the field entry_point_selector must always be a specific thing:

/// V0 transactions should always select the __execute__ entry point.
/// The original code from the blockifier crate was:

To compute the entry point selector value, you must use functions specifically from blockifier::abi::abi_utils, namely: selector_from_name. In this PR, we compute the value manually, offline, and hard code it in SNAPI.

The question becomes do we want to avoid this hard coding of a value.
The other thing we can do is move the abi utils into snapi.
Stuff like moving the function starknet_keccak into snapi - which might be fair game....


@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch 2 times, most recently from a2e08cb to 0c39a59 Compare September 29, 2024 08:26
@ArniStarkware ArniStarkware changed the base branch from main to arni/test_utils/tx_creator/align_tx_hash_on_account_tx_creator September 29, 2024 08:26
@ArniStarkware
Copy link
Contributor Author

crates/starknet_api/src/test_utils/invoke.rs line 79 at r3 (raw file):

}

static EXECUTE_ENTRY_POINT_SELECTOR: OnceLock<EntryPointSelector> = OnceLock::new();

Use lazylock

Code quote:

static EXECUTE_ENTRY_POINT_SELECTOR: OnceLock<EntryPointSelector> = OnceLock::new();

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/align_tx_hash_on_account_tx_creator branch from bd7724b to f74d1b4 Compare October 26, 2024 16:52
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 0c39a59 to f25ee2e Compare October 26, 2024 16:52
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware changed the base branch from arni/test_utils/tx_creator/align_tx_hash_on_account_tx_creator to graphite-base/747 October 27, 2024 07:23
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from f25ee2e to 8c57193 Compare October 27, 2024 07:23
@ArniStarkware ArniStarkware changed the base branch from graphite-base/747 to main October 27, 2024 07:24
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 8c57193 to 9557b39 Compare October 27, 2024 07:24
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

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.717 ms 35.025 ms 35.393 ms]
change: [+1.2448% +2.1121% +3.0844%] (p = 0.00 < 0.05)
Performance has regressed.
Found 20 outliers among 100 measurements (20.00%)
4 (4.00%) high mild
16 (16.00%) high severe

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 9557b39 to 0e25403 Compare November 4, 2024 11:20
Copy link

github-actions bot commented Nov 4, 2024

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/tx_creator/streamline_invoke_v0 branch from 0e25403 to fd8a886 Compare November 10, 2024 14:01
@ArniStarkware ArniStarkware changed the base branch from main to arni/test_utils/deploy_account/blockifier November 10, 2024 14:01
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware deleted the branch arni/test_utils/deploy_account/blockifier November 11, 2024 08:30
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 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.

2 participants