test(starknet_mempool): add case for tx in state

[ArniStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ArniStarkware]

• test(starknet_mempool): create has_tx_from_address unit test #3217
• test(starknet_mempool): mempool content builder with state #3152
• test(starknet_mempool): add case for tx in state #3153 👈 (View in Graphite)
• feat(starknet_mempool): implement has_tx_from_address #3126
• chore(starknet_mempool): rename to tx_from_address_exists to better reflect implementaion #3173
• main

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

[Yael-Starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @ayeletstarkware)

---

crates/starknet_mempool/src/mempool_test.rs line 944 at r1 (raw file):

    (mempool_content_builder, deployer_address)
}

I think we should test 3 cases:

1. we call add_tx and see that we get true for deploy_account_exists
2. after get_txs we call get_txs, and see that the result is still true
3. after that we call commit and see that the result is still true

Code quote:

/// Adds the address of an account to the mempool state as part of the tentative state. Returns the
/// mempool content builder and the deployer address.
fn mempool_content_builder_with_account_in_state() -> (MempoolContentBuilder, ContractAddress) {
    let deployer_address = contract_address!(100_u32);
    let mut state = MempoolState::default();
    let _nonce = state.get_or_insert(deployer_address, Nonce(Felt::ONE));
    assert_eq!(_nonce, Nonce(Felt::ONE));

    let mempool_content_builder = MempoolContentBuilder::new().with_state(state);

    (mempool_content_builder, deployer_address)
}

[ArniStarkware]

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

---

crates/starknet_mempool/src/mempool_test.rs line 944 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think we should test 3 cases:

1. we call add_tx and see that we get true for deploy_account_exists
2. after get_txs we call get_txs, and see that the result is still true
3. after that we call commit and see that the result is still true

Done, a simplified version of that.

The issue with my test is that it is not functional.
Tomorrow, I will add a creation for testing for the state.

[ArniStarkware]

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

---

crates/starknet_mempool/src/mempool_test.rs line 944 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done, a simplified version of that.

The issue with my test is that it is not functional.
Tomorrow, I will add a creation for testing for the state.

Done.
Now unit test.

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @ayeletstarkware)

---

crates/starknet_mempool/src/mempool_test.rs line 944 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.
Now unit test.

I wouldn't want to assume anything about the internal structure of the mempool, I think we should test only external apis here. Meaning, actually call add_tx, get_txs and commit. The test shouldn't depend on the internal mempool implementation.

We can use the MempoolContentBuilder to create an initial non-empty state for the test.

[ArniStarkware]

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @alonh5, @ayeletstarkware, and @Yael-Starkware)

---

crates/starknet_mempool/src/mempool_test.rs line 896 at r6 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.

Now moved to flow tests.

[Yael-Starkware]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @ayeletstarkware)

---

crates/starknet_mempool/src/mempool_test.rs line 893 at r7 (raw file):

    assert!(!mempool.has_tx_from_address(contract_address!(100_u32)));
}

just for the sake of making the code clear and readable, maybe the two tests should be together.
or otherwise, maybe add a comment that the positive flow is being tested as a flow test?

or maybe the best solution is to start the flow test by checking that the tx doesn't exist?

wdyt?

Code quote:

#[rstest]
fn has_tx_from_address_negative_flow() {
    let mempool = MempoolContentBuilder::new().build_into_mempool();

    assert!(!mempool.has_tx_from_address(contract_address!(100_u32)));
}

[Yael-Starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @ayeletstarkware)

---

crates/starknet_mempool/src/mempool_test.rs line 893 at r7 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

just for the sake of making the code clear and readable, maybe the two tests should be together.
or otherwise, maybe add a comment that the positive flow is being tested as a flow test?

or maybe the best solution is to start the flow test by checking that the tx doesn't exist?

wdyt?

ok, now that I see the next PR , I retract this comment.

[Yael-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ayeletstarkware)

[ayeletstarkware]

Reviewed 1 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/starknet_mempool/tests/flow_test.rs line 311 at r8 (raw file):

#[rstest]
fn has_tx_from_address(mut mempool: Mempool) {

Maybe something with "test_mempool_state_retains_address_across_api_calls`?

Code quote:

fn has_tx_from_address(mut mempool: Mempool)

---

crates/starknet_mempool/tests/flow_test.rs line 328 at r8 (raw file):

    let nonces = [(address, 1)];
    commit_block(&mut mempool, nonces, []);
    assert!(mempool.has_tx_from_address(contract_address!(address)));

I'm updating the test's documentation based on the mempool tests.

Suggestion:

    // Setup.
    let address = "0x64";
    let input_address_64 = add_tx_input!(address: address);

    // Test.
    add_tx(&mut mempool, &add_tx_args);
    // Assert: Mempool state includes the address of the added transaction.
    assert!(mempool.has_tx_from_address(contract_address!(address)));

    // Test.
    mempool.get_txs(1).unwrap();
    // Assert: The Mempool state still contains the address, even after it was sent to the batcher.
    assert!(mempool.has_tx_from_address(contract_address!(address)));

    // Test.
    let nonces = [(address, 1)];
    commit_block(&mut mempool, nonces, []);
    // Assert: Mempool state still contains the address, even though the transaction was committed.
    // Note that in the future, the Mempool's state may be periodically cleared from records of old
    // committed transactions. Mirroring this behavior may require a modification of this test.
    assert!(mempool.has_tx_from_address(contract_address!(address)));

[ArniStarkware]

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

---

crates/starknet_mempool/tests/flow_test.rs line 311 at r8 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Maybe something with "test_mempool_state_retains_address_across_api_calls`?

Done. Also added a comment.

---

crates/starknet_mempool/tests/flow_test.rs line 328 at r8 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I'm updating the test's documentation based on the mempool tests.

Done.

[ayeletstarkware]

Reviewed 1 of 3 files at r3, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @Yael-Starkware)

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion

---

crates/starknet_mempool/src/test_utils.rs line 216 at r9 (raw file):

        )
    };
    (address: $address:expr) => {

Can't we use an arm that already exists? I don't really understand why we have so many arms in the first place.

[ArniStarkware]

crates/starknet_mempool/src/test_utils.rs line 216 at r9 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can't we use an arm that already exists? I don't really understand why we have so many arms in the first place.

Dori tried simplifying this test util, to no avail. (Exactly to have as few branches as possible.

[alonh5]

Reviewable status: all files reviewed, 1 unresolved discussion

---

crates/starknet_mempool/src/test_utils.rs line 216 at r9 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Dori tried simplifying this test util, to no avail. (Exactly to have as few branches as possible.

Still, can you use an existing arm? Some arms receive an address.
Even if we can't simplify it, at least let's not make it more complicated

[alonh5]

Reviewable status: complete! all files reviewed, all discussions resolved

---

crates/starknet_mempool/src/test_utils.rs line 216 at r9 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Still, can you use an existing arm? Some arms receive an address.
Even if we can't simplify it, at least let's not make it more complicated

Ayelet explained offline. This is also fine.