feat: add custom accounts to test rpc state reader

[giladchase]

Make spawn_test_rpc_state_reader get an iterator of accounts and have count the number of occurrences by itself, instead of getting specific numbers per account.

To keep the diff small, the map of contract->n_instances_of_contract is cast back to the old type immediately.
Subsequent PRs will remove this cast and make the whole flow use the map.

---

This change is 

[giladchase]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)

---

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

    // map feature contracts to n_instances inside the account array
    let mut account_to_n_instances: IndexMap<FeatureContract, usize> =
        IndexMap::from_iter(accounts.into_iter().counts());

This line will compile once this Blockifier PR is merged.

Code quote:

        IndexMap::from_iter(accounts.into_iter().counts());

[Yael-Starkware]

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @dafnamatsry and @giladchase)

---

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

        let default_account_contract =
            FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1);
        let accounts = std::iter::repeat(default_account_contract).take(n_accounts);

I think creating the IndexMap here, to begin with, will be simpler, and then you can remove the conversion to indexMap that follows,

Code quote:

let accounts = std::iter::repeat(default_account_contract).take(n_accounts);

---

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

/// A variable number of identical accounts and test contracts are initialized and funded.
pub async fn spawn_test_rpc_state_reader(
    accounts: impl IntoIterator<Item = FeatureContract>,

as commented above,
why not get here the account_to_n_instances IndexMap in the first place?
And then remove the conversion to IndexMap.

Code quote:

accounts:

---

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

    for contract in [
        FeatureContract::TestContract(CairoVersion::Cairo0),
        FeatureContract::TestContract(CairoVersion::Cairo0),

Suggestion:

CairoVersion::Cairo1

[giladchase]

Reviewable status: 3 of 13 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

I think creating the IndexMap here, to begin with, will be simpler, and then you can remove the conversion to indexMap that follows,

I agree it's inefficient, but we should hide non-intuitive implementation details like instance_ids from the API, which makes the API easier to understand for the user (the testwriter).

I suggest merging this version first, if OK by you then immediately think about refactoring. This approach keeps the current stack simpler and allows us to improve working code, rather than divergent components.

---

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

    for contract in [
        FeatureContract::TestContract(CairoVersion::Cairo0),
        FeatureContract::TestContract(CairoVersion::Cairo0),

Done.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.43%. Comparing base (c66762d) to head (894d522).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
+ Coverage   83.38%   83.43%   +0.04%     
==========================================
  Files          37       37              
  Lines        1770     1775       +5     
  Branches     1770     1775       +5     
==========================================
+ Hits         1476     1481       +5     
  Misses        215      215              
  Partials       79       79              

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

[giladchase]

Reviewable status: 2 of 13 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

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

Previously, Yael-Starkware (YaelD) wrote…

as commented above,
why not get here the account_to_n_instances IndexMap in the first place?
And then remove the conversion to IndexMap.

No one will understand why such a structure is needed I think, except us two and Dori, it is too arcane, not natural ("why does a state-reader need a hashmap of feature to n-instances? can't it fund them without this?")

Perhaps an abstraction is needed for this, but this is out of scope here I think, since this is still a stabilization PR.

I suggest discussing a refactor after we stabilize the test.

[Yael-Starkware]

Reviewable status: 2 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @giladchase)

---

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

Previously, giladchase wrote…

I agree it's inefficient, but we should hide non-intuitive implementation details like instance_ids from the API, which makes the API easier to understand for the user (the testwriter).

I suggest merging this version first, if OK by you then immediately think about refactoring. This approach keeps the current stack simpler and allows us to improve working code, rather than divergent components.

but the API is new(n_accounts), and that pretty straightforward. if someone needs to change the internal functions , they will have to understand the logic anyway.

Sure we can discuss later

---

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

Previously, giladchase wrote…

No one will understand why such a structure is needed I think, except us two and Dori, it is too arcane, not natural ("why does a state-reader need a hashmap of feature to n-instances? can't it fund them without this?")

Perhaps an abstraction is needed for this, but this is out of scope here I think, since this is still a stabilization PR.

I suggest discussing a refactor after we stabilize the test.

sure

[giladchase]

Reviewable status: 2 of 13 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

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

but the API is new(n_accounts), and that pretty straightforward

new(n_accounts) is only part of the API, and in fact it is the one not currently used (but will be used soon).

Right now we're using new_for_accounts_contracts(accounts).

Also, I don't see how this follows your previous message 🤔 you suggested passing a hashmap as an arg, and if a user is using new(n_accounts), they'll still need to pass a hashmap, only now it'll have one key.

if someone needs to change the internal functions , they will have to understand the logic anyway.

Not all of the logic.
The whole stack of SetupIntegrationTest and the state reader initialization is quite big, and if someone needs to change something small there, it should be easy for them to jump in and do it, without having to any implementation details they don't need to.
In particular, the whole instance_id thing is quite unintuitive, and it'd be best to hide that one as much as possible, otherwise the tax on developer time will be big in the long run.

[Yael-Starkware]

Reviewable status: 2 of 13 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @giladchase)

---

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

Previously, giladchase wrote…

but the API is new(n_accounts), and that pretty straightforward

new(n_accounts) is only part of the API, and in fact it is the one not currently used (but will be used soon).

Right now we're using new_for_accounts_contracts(accounts).

Also, I don't see how this follows your previous message 🤔 you suggested passing a hashmap as an arg, and if a user is using new(n_accounts), they'll still need to pass a hashmap, only now it'll have one key.

if someone needs to change the internal functions , they will have to understand the logic anyway.

Not all of the logic.
The whole stack of SetupIntegrationTest and the state reader initialization is quite big, and if someone needs to change something small there, it should be easy for them to jump in and do it, without having to any implementation details they don't need to.
In particular, the whole instance_id thing is quite unintuitive, and it'd be best to hide that one as much as possible, otherwise the tax on developer time will be big in the long run.

ack

[Yael-Starkware]

Reviewable status: 2 of 13 files reviewed, all discussions resolved (waiting on @dafnamatsry)

[giladchase]

Reviewed 3 of 5 files at r1, 1 of 9 files at r2, 6 of 8 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)