starknet_os_flow_tests: add control for expected reverting txs

[dorimedini-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[dorimedini-starkware]

• starknet_os_flow_tests: migrate test_v1_bound_accounts_cairo1 #9496 : 2 dependent PRs (#9501 , #9514 )
• starknet_os_flow_tests: add V1-bound cairo1 account for flow tests #9495
• starknet_api: fix compiler->SNAPI contract class ABI string conversion for tests #9494
• starknet_os_flow_tests: migrate test_v1_bound_accounts_cairo0 #9493
• starknet_os_flow_tests: split get_class_info_of_cairo0_contract into function #9492
• starknet_os_flow_tests: more informative execution failure #9491
• starknet_os_flow_tests: add V1-bound cairo0 account for flow tests #9490
• starknet_os_flow_tests: localize expected storage updates in test_os_logic #9485
• starknet_os_flow_tests: migrate test_os_logic and some small refactors #9484
• blockifier_test_utils: add functionality to test contract for test_os_logic #9483
• blockifier_test_utils: add test_contract2 cairo0 contract #9482
• blockifier_test_utils: add delegate_proxy cairo0 contract #9481
• starknet_os_flow_tests: add control for expected reverting txs #9480 👈 (View in Graphite)
• starknet_os_flow_tests: migrate test_reverted_l1_handler_tx #9474
• starknet_os_flow_tests: reduce boilerplate with add_funded_account_invoke #9563
• starknet_os_flow_tests: move nonce manager into test manager #9562
• starknet_os_flow_tests: migrate test_reverted_invoke_tx #9473
• starknet_os_flow_tests: reduce boilerplate with perform_default_validations #9472
• main

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

[dorimedini-starkware]

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @amosStarkware and @TzahiTaub)

---

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

        sender_address: *FUNDED_ACCOUNT_ADDRESS,
        nonce: nonce_manager.next(*FUNDED_ACCOUNT_ADDRESS),
        calldata: create_calldata(test_contract_address, "write_and_revert", &[Felt::ONE, Felt::TWO]),

was previously reverted, but for the wrong reason (missing params)

Code quote:

 &[Felt::ONE, Felt::TWO]

[TzahiTaub]

@TzahiTaub reviewed 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware)

---

crates/starknet_os_flow_tests/src/test_manager.rs line 94 at r3 (raw file):

    tx: BlockifierTransaction,
    revert_reason: Option<String>,
}

Just to ask, do you think the optional field is better than having an enum for the entire tx? With something like this pseudo?

Suggestion:

pub(crate) enum FlowTestTx {
    succesful/accepted({tx: BlockifierTransaction})
    reverted({
    tx: BlockifierTransaction,
    revert_reason: String}
}

---

crates/starknet_os_flow_tests/src/test_manager.rs line 445 at r3 (raw file):

    fn verify_execution_outputs(
        revert_reasons: &[Option<String>],
        execution_outputs: &[(TransactionExecutionInfo, StateMaps)],

Please verify that both slices are of the same length

Code quote:

        revert_reasons: &[Option<String>],
        execution_outputs: &[(TransactionExecutionInfo, StateMaps)],

---

crates/starknet_os_flow_tests/src/test_manager.rs line 529 at r3 (raw file):

                .into_iter()
                .map(|flow_test_tx| (flow_test_tx.tx, flow_test_tx.revert_reason))
                .unzip();

Suggestion:

            let (block_txs, revert_reasons): (Vec<_>, Vec<_>) = block_txs_with_reason
                .into_iter()
                .map(|flow_test_tx| (flow_test_tx.tx, flow_test_tx.revert_reason))
                .unzip();
            // Clone the block info for later use.

[AvivYossef-starkware]

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @amosStarkware and @TzahiTaub)

---

crates/starknet_os_flow_tests/src/test_manager.rs line 333 at r3 (raw file):

Previously, dorimedini-starkware wrote…

StarknetOsInput uses this type as the key for the deprecated_compiled_classes map, is this a bug?
since "class hash" refers to sierra hash and "compiled class hash" refers to casm hash, maybe "compiled class hash" is a better term for a cairo0 compiled contract hash. no?

What is this represent? The hash of what?
I see that in thin state_diff it saved by class_hash.
There is class_hash for deprecated contract class here

#[derive(Debug, Default, Clone, Eq, PartialEq, Deserialize, Serialize)]
pub struct StateDiff {
    pub deployed_contracts: IndexMap<ContractAddress, ClassHash>,
    pub storage_diffs: IndexMap<ContractAddress, IndexMap<StorageKey, Felt>>,
    pub declared_classes: IndexMap<ClassHash, (CompiledClassHash, SierraContractClass)>,
    pub migrated_compiled_classes: IndexMap<ClassHash, CompiledClassHash>,
    pub deprecated_declared_classes: IndexMap<ClassHash, DeprecatedContractClass>,
    pub nonces: IndexMap<ContractAddress, Nonce>,
}

[dorimedini-starkware]

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @amosStarkware and @TzahiTaub)

---

crates/starknet_os_flow_tests/src/test_manager.rs line 333 at r3 (raw file):

Previously, AvivYossef-starkware wrote…

What is this represent? The hash of what?
I see that in thin state_diff it saved by class_hash.
There is class_hash for deprecated contract class here

#[derive(Debug, Default, Clone, Eq, PartialEq, Deserialize, Serialize)]
pub struct StateDiff {
    pub deployed_contracts: IndexMap<ContractAddress, ClassHash>,
    pub storage_diffs: IndexMap<ContractAddress, IndexMap<StorageKey, Felt>>,
    pub declared_classes: IndexMap<ClassHash, (CompiledClassHash, SierraContractClass)>,
    pub migrated_compiled_classes: IndexMap<ClassHash, CompiledClassHash>,
    pub deprecated_declared_classes: IndexMap<ClassHash, DeprecatedContractClass>,
    pub nonces: IndexMap<ContractAddress, Nonce>,
}

this represents the hash of a cairo0 contract.
there is only one way to compute this hash (right?)
(unlike cairo1, where we have sierra/casm hashes)

in any case, since the issue is the type in the OS input struct, I would say this is out of scope... and not a blocker, because the underlying type is a Felt anyway

[AvivYossef-starkware]

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @amosStarkware and @TzahiTaub)

---

crates/starknet_os_flow_tests/src/test_manager.rs line 333 at r3 (raw file):

Previously, dorimedini-starkware wrote…

this represents the hash of a cairo0 contract.
there is only one way to compute this hash (right?)
(unlike cairo1, where we have sierra/casm hashes)

in any case, since the issue is the type in the OS input struct, I would say this is out of scope... and not a blocker, because the underlying type is a Felt anyway

Yes, it's definitely out of scope,
But we should change it in a different PR to be consistent

[dorimedini-starkware]

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @amosStarkware and @TzahiTaub)

---

crates/starknet_os_flow_tests/src/test_manager.rs line 333 at r3 (raw file):

Previously, AvivYossef-starkware wrote…

Yes, it's definitely out of scope,
But we should change it in a different PR to be consistent

PR

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 of 1 files at r2, 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)