refactor(tests-integration): state diff builder

[giladchase]

Extract deploy/declare/fund into dedicated builder.
Different contracts/accounts each need different handling in terms of
those three actions, and a logical split allows for easier debugging
during testing (from experience...).

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 73 lines in your changes missing coverage. Please review.

Project coverage is 0.61%. Comparing base (8f515f4) to head (2afbd7e).

Files with missing lines	Patch %	Lines
crates/tests-integration/src/state_reader.rs	0.00%	73 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           gilad/split-uses-state-reader   #1063      +/-   ##
================================================================
- Coverage                           0.62%   0.61%   -0.01%     
================================================================
  Files                                208     208              
  Lines                              21609   21619      +10     
  Branches                           21609   21619      +10     
================================================================
  Hits                                 134     134              
- Misses                             21464   21474      +10     
  Partials                              11      11              

Flag	Coverage Δ
	0.61% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[elintul]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @giladchase and @Yael-Starkware)

---

crates/tests-integration/src/state_reader.rs line 115 at r1 (raw file):

    accounts_defined_in_the_test: &[FeatureAccount],
    default_test_contracts: &[FeatureAccount],
    erc20_account: &FeatureAccount,

Types are confusing. :(

Suggestion:

    default_test_contracts: &[FeatureAccount],
    erc20_contract: &FeatureAccount,

---

crates/tests-integration/src/state_reader.rs line 123 at r1 (raw file):

    state_diff_builder.set_contracts(default_test_contracts).declare().deploy();

    // Declare and deploy and the erc20 contract, so that transfers from it can be made.

Suggestion:

ERC20

---

crates/tests-integration/src/state_reader.rs line 124 at r1 (raw file):

    // Declare and deploy and the erc20 contract, so that transfers from it can be made.
    state_diff_builder.set_contracts(std::slice::from_ref(erc20_account)).declare().deploy();

Consider chaining to test contracts instead. :)

Code quote:

std::slice::from_ref(erc20_account)

---

crates/tests-integration/src/state_reader.rs line 235 at r1 (raw file):

#[derive(Default)]
struct ThinStateDiffBuilder<'a> {
    contracts: &'a [FeatureAccount],

• in setter name and variables.
  Assuming they're all accounts. 😅

Suggestion:

accounts

---

crates/tests-integration/src/state_reader.rs line 236 at r1 (raw file):

struct ThinStateDiffBuilder<'a> {
    contracts: &'a [FeatureAccount],
    deprecated_declared_classes: Vec<ClassHash>,

Is a slice possible (and suitable) here?

Code quote:

Vec<ClassHash>

---

crates/tests-integration/src/state_reader.rs line 237 at r1 (raw file):

    contracts: &'a [FeatureAccount],
    deprecated_declared_classes: Vec<ClassHash>,
    declared_classes: IndexMap<ClassHash, starknet_api::core::CompiledClassHash>,

Why fully-qualified?

Code quote:

starknet_api::core::CompiledClassHash

---

crates/tests-integration/src/state_reader.rs line 241 at r1 (raw file):

    storage_diffs: IndexMap<ContractAddress, IndexMap<StorageKey, Felt>>,
    chain_info: ChainInfo,
    initial_balances: Felt,

Suggestion:

initial_account_balance: Felt

---

crates/tests-integration/src/state_reader.rs line 245 at r1 (raw file):

impl<'a> ThinStateDiffBuilder<'a> {
    const DEFAULT_INITIAL_BALANCE: u128 = BALANCE;

If only used inside new, perhaps define inside it?
Also, perhaps [TEST_]INITIAL_ACCOUNT_BALANCE, since "default" is usually related to a zeroed value.

Code quote:

const DEFAULT_INITIAL_BALANCE: u128 = BALANCE;

---

crates/tests-integration/src/state_reader.rs line 252 at r1 (raw file):

        let deployed_contracts = indexmap! {
            chain_info.fee_token_address(&FeeType::Eth) => erc20_class_hash,

Can you map over FeeType variants?

Code quote:

FeeType::Eth

---

crates/tests-integration/src/state_reader.rs line 257 at r1 (raw file):

        Self {
            chain_info: chain_info.clone(),

Is it possible to pass by-value?

Code quote:

chain_info: chain_info.clone()

---

crates/tests-integration/src/state_reader.rs line 285 at r1 (raw file):

    fn deploy(&mut self) -> &mut Self {
        for contract in self.contracts {
            self.deployed_contracts.insert(contract.sender_address, contract.class_hash());

Sanity Q: does this mean that I can invoke get_class_hash_at(address) on the created state reader and get the class hash inserted here?

Code quote:

self.deployed_contracts.insert

---

crates/tests-integration/src/state_reader.rs line 297 at r1 (raw file):

                    | FeatureContract::AccountWithoutValidations(_)
                    | FeatureContract::FaultyAccount(_),
                "Only Accounts can be funded"

Format actual type?

Code quote:

"Only Accounts can be funded"

---

crates/tests-integration/src/state_reader.rs line 300 at r1 (raw file):

            );

            let fee_token_address = get_fee_token_var_address(contract.sender_address);

Getter or not? Needed or not? 😬

Code quote:

contract.sender_address

---

crates/tests-integration/src/state_reader.rs line 305 at r1 (raw file):

                    .entry(self.chain_info.fee_token_address(&fee_type))
                    .or_default()
                    .insert(fee_token_address, self.initial_balances);

❤️

Code quote:

                    .entry(self.chain_info.fee_token_address(&fee_type))
                    .or_default()
                    .insert(fee_token_address, self.initial_balances);

---

crates/tests-integration/src/state_reader.rs line 313 at r1 (raw file):

    // TODO(deploy_account_support): delete method once we have batcher with execution.
    fn inject_accounts_into_state(&mut self, accounts_defined_in_the_test: &'a [FeatureAccount]) {

In all locations?

Suggestion:

test_defined_accounts

---

crates/tests-integration/src/state_reader.rs line 314 at r1 (raw file):

    // TODO(deploy_account_support): delete method once we have batcher with execution.
    fn inject_accounts_into_state(&mut self, accounts_defined_in_the_test: &'a [FeatureAccount]) {
        self.set_contracts(accounts_defined_in_the_test).declare().deploy().fund();

MAGNIV!
Is that a usecase of returning &mut self?

Code quote:

.declare().deploy().fund()