starknet_os_flow_tests: migrate test_v1_bound_accounts_cairo1

[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 ) 👈 (View in Graphite)
• 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
• 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.

[meship-starkware]

@meship-starkware reviewed 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 946 at r2 (raw file):

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

Please add the documentation as to why we create the signature after the transaction creation

Code quote:

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

---

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

            ),
        ]),
    )]);

Why is it ok to ignore the fee_token_address storage updates here?

Code quote:

    let expected_storage_updates = HashMap::from([(
        v1_bound_account_address,
        HashMap::from([
            (
                StarknetStorageKey(selector_from_name("Account_public_key").0.try_into().unwrap()),
                StarknetStorageValue(public_key),
            ),
            (
                StarknetStorageKey(
                    Pedersen::hash(&selector_from_name("SRC5_supported_interfaces").0, &isrc6_id)
                        .try_into()
                        .unwrap(),
                ),
                StarknetStorageValue(Felt::ONE),
            ),
        ]),
    )]);

[meship-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, and @Yoni-Starkware)

---

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

        compiled_class_hash,
        resource_bounds: *NON_TRIVIAL_RESOURCE_BOUNDS,
    };

Is the fundeed account an OZ account? Does it matter for the test?

Code quote:

    let declare_args = declare_tx_args! {
        sender_address: *FUNDED_ACCOUNT_ADDRESS,
        nonce: test_manager.next_nonce(*FUNDED_ACCOUNT_ADDRESS),
        class_hash,
        compiled_class_hash,
        resource_bounds: *NON_TRIVIAL_RESOURCE_BOUNDS,
    };

[dorimedini-starkware]

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 902 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Is the fundeed account an OZ account? Does it matter for the test?

it doesn't matter, the funded account is just the utility account used to setup the test

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Please add the documentation as to why we create the signature after the transaction creation

// Create an invoke tx, compute the hash, sign the hash and update the signature on the tx.

not enough?

---

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

Previously, meship-starkware (Meshi Peled) wrote…

Why is it ok to ignore the fee_token_address storage updates here?

hmmm... is it important to test that? I was under the impression that we care that the contract state was correctly updated, why would the fee collection mechanism behave any differently here?

[dorimedini-starkware]

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

[meship-starkware]

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

[meship-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

hmmm... is it important to test that? I was under the impression that we care that the contract state was correctly updated, why would the fee collection mechanism behave any differently here?

After reading the test again, I think you are right, the reason we add it is because we found the account so meaning this change will be in the state diff and the Python test check the whole state diff

[graphite-app]

Merge activity

• Oct 23, 12:28 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.