Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

Copy link
Collaborator Author

dorimedini-starkware commented Oct 5, 2025

@reviewable-StarkWare
Copy link

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Oct 5, 2025

Benchmark movements: No major performance changes detected.

@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from e7199e0 to e875064 Compare October 5, 2025 07:52
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from 210b6e4 to d3f102a Compare October 5, 2025 09:38
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from e875064 to 896b42a Compare October 5, 2025 09:38
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from d3f102a to 9594067 Compare October 6, 2025 16:51
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from 896b42a to 0a83a07 Compare October 6, 2025 16:51
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from f4e57f8 to 56bd729 Compare October 14, 2025 10:22
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from 440c636 to 306a91a Compare October 14, 2025 10:22
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from 56bd729 to 27de36e Compare October 15, 2025 11:17
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from 306a91a to 3694d5b Compare October 15, 2025 11:18
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from 27de36e to 4306e5d Compare October 19, 2025 18:42
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from 3694d5b to cdf4fba Compare October 19, 2025 18:43
Copy link
Collaborator

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

@meship-starkware reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 807 at r2 (raw file):

    let declare_args = declare_tx_args! {
        version: TransactionVersion::ZERO,
        max_fee: Fee(1_000_000_000_000_000),

Is it necessary? AFAIK Tx version 0 does not have a max fee

Suggestion:

 max_fee: Fee(1_000_000_000_000_000),   

crates/starknet_os_flow_tests/src/tests.rs line 856 at r2 (raw file):

        signature: TransactionSignature(Arc::new(vec![r, s])),
        ..validate_tx_args
    };

Please add the doc as to why we create the signature after the tx here

Code quote:

    let Signature { r, s } = ecdsa_sign(&private_key, &validate_tx.tx_hash()).unwrap().into();
    let validate_tx_args = invoke_tx_args! {
        signature: TransactionSignature(Arc::new(vec![r, s])),
        ..validate_tx_args
    };

@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from 4306e5d to 7d4aa74 Compare October 22, 2025 15:59
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from cdf4fba to 46a967c Compare October 22, 2025 15:59
Copy link
Collaborator

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

@meship-starkware reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, and @Yoni-Starkware)

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-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, 2 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 807 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Is it necessary? AFAIK Tx version 0 does not have a max fee

I see that it does have max_fee, because the type used is DeclareTransactionV0V1.
I don't think it matters, we are not trying to test fees here


crates/starknet_os_flow_tests/src/tests.rs line 856 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Please add the doc as to why we create the signature after the tx here

there is an explanation above:

    // Create a validate tx and add signature to the transaction. The dummy account used to call
    // `__validate__` does not check the signature, so we can use the signature field for
    // `__validate__`. This is done after creating the transaction so that we will have access
    // to the transaction hash.

is it unclear? we need to sign the tx hash, it's easier to first create the entire tx and then compute the signature

Copy link
Collaborator

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 856 at r2 (raw file):

Previously, dorimedini-starkware wrote…

there is an explanation above:

    // Create a validate tx and add signature to the transaction. The dummy account used to call
    // `__validate__` does not check the signature, so we can use the signature field for
    // `__validate__`. This is done after creating the transaction so that we will have access
    // to the transaction hash.

is it unclear? we need to sign the tx hash, it's easier to first create the entire tx and then compute the signature

No, this is fine. I simply missed it

@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function branch from 7d4aa74 to 56050a5 Compare October 23, 2025 10:12
@dorimedini-starkware dorimedini-starkware force-pushed the 10-02-starknet_os_flow_tests_migrate_test_v1_bound_accounts_cairo0 branch from 46a967c to 7cb9ec7 Compare October 23, 2025 10:12
@dorimedini-starkware dorimedini-starkware changed the base branch from 10-02-starknet_os_flow_tests_split_get_class_info_of_cairo0_contract_into_function to main October 23, 2025 10:26
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

@dorimedini-starkware reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @Yoni-Starkware)

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Oct 23, 2025
Merged via the queue into main with commit e79a92a Oct 23, 2025
52 of 56 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2025
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