Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator Author

dorimedini-starkware commented Oct 8, 2025

@dorimedini-starkware dorimedini-starkware self-assigned this Oct 8, 2025
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review October 8, 2025 13:33
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 37bc0f5 to ecd96d8 Compare October 8, 2025 13:44
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 3439f71 to f5f799d Compare October 8, 2025 13:44
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from ecd96d8 to 7c79c67 Compare October 8, 2025 13:49
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from f5f799d to 0885c59 Compare October 8, 2025 13:49
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 7c79c67 to e550ab8 Compare October 8, 2025 13:56
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 0885c59 to cebdf7a Compare October 8, 2025 13:56
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from e550ab8 to e8347e7 Compare October 8, 2025 14:57
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from cebdf7a to 500ef05 Compare October 8, 2025 14:57
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from e8347e7 to 18ced25 Compare October 9, 2025 08:29
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 500ef05 to 16c98a3 Compare October 9, 2025 08:29
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 18ced25 to 41b8174 Compare October 9, 2025 09:35
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 16c98a3 to fa08c78 Compare October 9, 2025 09:38
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 41b8174 to 7931da2 Compare October 9, 2025 09:48
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 4bcc172 to 23150bc Compare October 15, 2025 11:22
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 7f9f6b6 to 3802aa6 Compare October 19, 2025 18:44
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 23150bc to 192867e Compare October 19, 2025 18:44
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 3802aa6 to c657f60 Compare October 23, 2025 08:52
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 192867e to 517c832 Compare October 23, 2025 08:52
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from c657f60 to eab25e1 Compare October 28, 2025 14:12
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from eab25e1 to 12da053 Compare November 13, 2025 11:46
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 517c832 to a889042 Compare November 13, 2025 11:46
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract branch from 12da053 to 07c78ec Compare November 13, 2025 12:31
@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from a889042 to 8ab3dc7 Compare November 13, 2025 12:31
Copy link
Contributor

@rotem-starkware rotem-starkware left a 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 r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 1624 at r1 (raw file):

    let tx_info_writer = FeatureContract::TxInfoWriter;
    let class_hash = get_class_hash_of_feature_contract(tx_info_writer);
    // Initialize the test manager with the tx info writer already declared.

When it's declared?


crates/starknet_os_flow_tests/src/tests.rs line 1625 at r1 (raw file):

    let class_hash = get_class_hash_of_feature_contract(tx_info_writer);
    // Initialize the test manager with the tx info writer already declared.
    // We can ignore the address of the dpeloyed instance.

// We can ignore the address of the deployed instance.

Code quote:

// We can ignore the address of the dpeloyed instance.

crates/starknet_os_flow_tests/src/tests.rs line 1727 at r1 (raw file):

        Transaction::Account(AccountTransaction::Declare(declare_tx)),
        Transaction::L1Handler(l1_handler_tx),
    ] {

IIUC in the python test we check also the first declare tx (of the contract) and the fund tx.
Is it not required?


crates/starknet_os_flow_tests/src/tests.rs line 1749 at r1 (raw file):

            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), Felt::ZERO);
        }

I think the inverse condition is more readable:

if matches!(tx.tx_type(), TransactionType::L1Handler) {
            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), Felt::ZERO);
        } else {
            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), tx.version().0);
        }

Code quote:

        if !matches!(tx.tx_type(), TransactionType::L1Handler) {
            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), tx.version().0);
        } else {
            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), Felt::ZERO);
        }

crates/starknet_os_flow_tests/src/tests.rs line 1767 at r1 (raw file):

    );
    test_output.assert_account_balance_change(tx_info_account_address);
    test_output.assert_account_balance_change(contract_address!(TEST_SEQUENCER_ADDRESS));

Do we need to check that the account balance of the funding account changed?

@dorimedini-starkware dorimedini-starkware changed the base branch from 10-08-blockifier_test_utils_add_tx_info_writer_cairo0_contract to main-v0.14.1-committer November 19, 2025 13:42
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: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware, @meship-starkware, @rotem-starkware, and @Yoni-Starkware)


crates/starknet_os_flow_tests/src/tests.rs line 1624 at r1 (raw file):

Previously, rotem-starkware wrote…

When it's declared?

when calling new_with_default_initial_state, you can pass contracts you want to (declare and) deploy as part of the initial state. that's what I mean: I declare and deploy the tx info writer, but I ignore the deployed instance and deploy again as part ofthe OS flow test (the initial state does not get proven by the OS).


crates/starknet_os_flow_tests/src/tests.rs line 1625 at r1 (raw file):

Previously, rotem-starkware wrote…

// We can ignore the address of the deployed instance.

Done.


crates/starknet_os_flow_tests/src/tests.rs line 1727 at r1 (raw file):

Previously, rotem-starkware wrote…

IIUC in the python test we check also the first declare tx (of the contract) and the fund tx.
Is it not required?

the contract is not declared in this block.
if this contract was special (in the sense that it would be interesting to see that the OS can process it's declare tx), then I would say it's important to run it in the block, but it doesn't have any special syscalls or anything


crates/starknet_os_flow_tests/src/tests.rs line 1749 at r1 (raw file):

Previously, rotem-starkware wrote…

I think the inverse condition is more readable:

if matches!(tx.tx_type(), TransactionType::L1Handler) {
            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), Felt::ZERO);
        } else {
            contract_storage_updates
                .insert(get_storage_var_address("version", tx_type), tx.version().0);
        }

done (and removed some duplication while i'm there)


crates/starknet_os_flow_tests/src/tests.rs line 1767 at r1 (raw file):

Previously, rotem-starkware wrote…

Do we need to check that the account balance of the funding account changed?

Done.

@dorimedini-starkware dorimedini-starkware force-pushed the 10-08-starknet_os_flow_tests_migrate_test_deprecated_tx_info branch from 8ab3dc7 to b561061 Compare November 19, 2025 13:42
Copy link
Contributor

@rotem-starkware rotem-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Nov 19, 2025
Merged via the queue into main-v0.14.1-committer with commit b8947e1 Nov 19, 2025
16 of 20 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 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