-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_os_flow_tests: migrate test_block_info #9529
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
starknet_os_flow_tests: migrate test_block_info #9529
Conversation
b2c97a4 to
b81fdc8
Compare
1638319 to
8a1fdf8
Compare
b81fdc8 to
45cceaf
Compare
8a1fdf8 to
2787132
Compare
45cceaf to
b2fd2c7
Compare
2787132 to
492b9c7
Compare
b2fd2c7 to
a0abc86
Compare
492b9c7 to
6a5be89
Compare
a0abc86 to
cfd092e
Compare
6a5be89 to
3050ce8
Compare
cfd092e to
378371a
Compare
3050ce8 to
d26bb4a
Compare
519c39f to
9615107
Compare
fbd33e1 to
7e0d228
Compare
9615107 to
3486e88
Compare
7e0d228 to
a20b5be
Compare
3486e88 to
3b9680d
Compare
a20b5be to
2913251
Compare
3b9680d to
a786801
Compare
2913251 to
b36f6cd
Compare
a786801 to
227cff9
Compare
b36f6cd to
c4b1129
Compare
c4b1129 to
20e96e6
Compare
|
Benchmark movements: full_committer_flow performance improved 😺 full_committer_flow time: [16.136 ms 16.269 ms 16.408 ms] change: [-7.6444% -6.0254% -4.5131%] (p = 0.00 < 0.05) Performance has improved. Found 2 outliers among 100 measurements (2.00%) 2 (2.00%) high mild |
rotem-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rotem-starkware reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)
crates/starknet_os_flow_tests/src/tests.rs line 1907 at r3 (raw file):
DeclareTransaction::create(account_declare_tx, class_info, &CHAIN_ID_FOR_TESTS) .unwrap(); test_manager.add_cairo1_declare_tx(declare_tx.clone(), &test_contract_sierra);
Is the cloning necessary here?
crates/starknet_os_flow_tests/src/tests.rs line 1946 at r3 (raw file):
let empty_compiled_class_hash = empty_contract.get_compiled_class_hash(&HashVersion::V2); let declare_tx_args = declare_tx_args! { sender_address: block_info_account_address,
It's not like the python test, but I think it's correct.
IIUC we want to test __validate_declare__ of block_info_test_contract, so we need to send this tx from block_info_account_address.
@Yoni-Starkware can you PTAL?
Code quote:
sender_address: block_info_account_address,crates/starknet_os_flow_tests/src/tests.rs line 1970 at r3 (raw file):
// Test `constructor` in execute mode. let ctor_calldata = [Felt::ZERO]; // Not validate mode (execute mode). let salt = Felt::ZERO;
Suggestion:
let salt = Felt::ONE20e96e6 to
ac72ed9
Compare
dorimedini-starkware
left a comment
There was a problem hiding this 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 1 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, @rotem-starkware, and @Yoni-Starkware)
crates/starknet_os_flow_tests/src/tests.rs line 1907 at r3 (raw file):
Previously, rotem-starkware wrote…
Is the cloning necessary here?
Done.
crates/starknet_os_flow_tests/src/tests.rs line 1970 at r3 (raw file):
Previously, rotem-starkware wrote…
Done.
Yoni-Starkware
left a comment
There was a problem hiding this 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 1 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, and @rotem-starkware)
crates/starknet_os_flow_tests/src/tests.rs line 1946 at r3 (raw file):
Previously, rotem-starkware wrote…
It's not like the python test, but I think it's correct.
IIUC we want to test__validate_declare__ofblock_info_test_contract, so we need to send this tx fromblock_info_account_address.
@Yoni-Starkware can you PTAL?
Right, bug in py test
Yoni-Starkware
left a comment
There was a problem hiding this 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 1 files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, and @rotem-starkware)
a discussion (no related file):
Non-blocking, no need to be in this PR. Can you add the following asserts to the test contracts in execute mode?
assert (block_number != block_number_for_validate, 'INVALID_BLOCK_NUMBER');
assert (block_timestamp != block_timestamp_for_validate, 'INVALID_BLOCK_TIMESTAMP');Here
if is_validate {
assert (block_number == block_number_for_validate, 'INVALID_BLOCK_NUMBER');
assert (block_timestamp == block_timestamp_for_validate, 'INVALID_BLOCK_TIMESTAMP');
return starknet::VALIDATED;
}
assert (!is_validate, 'INVALID_IS_VALIDATE');
// Here
dorimedini-starkware
left a comment
There was a problem hiding this 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 1 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, and @rotem-starkware)
a discussion (no related file):
Previously, Yoni-Starkware (Yoni) wrote…
Non-blocking, no need to be in this PR. Can you add the following asserts to the test contracts in execute mode?
assert (block_number != block_number_for_validate, 'INVALID_BLOCK_NUMBER'); assert (block_timestamp != block_timestamp_for_validate, 'INVALID_BLOCK_TIMESTAMP');Here
if is_validate { assert (block_number == block_number_for_validate, 'INVALID_BLOCK_NUMBER'); assert (block_timestamp == block_timestamp_for_validate, 'INVALID_BLOCK_TIMESTAMP'); return starknet::VALIDATED; } assert (!is_validate, 'INVALID_IS_VALIDATE'); // Here
done (top of stack)
crates/starknet_os_flow_tests/src/tests.rs line 1946 at r3 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Right, bug in py test
Done.
rotem-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rotem-starkware reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)

No description provided.