chore(mempool): move account nonces update check to add tx test

[ayeletstarkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 15.26%. Comparing base (b0cfe82) to head (73a8306).
Report is 49 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (73a8306). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (73a8306)
	3	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #878       +/-   ##
===========================================
- Coverage   74.18%   15.26%   -58.93%     
===========================================
  Files         359       31      -328     
  Lines       36240     2738    -33502     
  Branches    36240     2738    -33502     
===========================================
- Hits        26886      418    -26468     
+ Misses       7220     2302     -4918     
+ Partials     2134       18     -2116     

Flag	Coverage Δ
	15.26% <ø> (-58.93%)	⬇️

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.

[ayeletstarkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion

---

crates/mempool/src/mempool_test.rs line 490 at r2 (raw file):

        .with_queue(expected_queue_txs)
        .build();
    expected_mempool_content.assert_eq_pool_and_queue_content(&mempool);

I think it would be better to delete this function since we now have more fields to assert, and we can check each one separately as needed.

Code quote:

assert_eq_pool_and_queue_content

[ArniStarkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware)

---

Cargo.lock line 9793 at r2 (raw file):

dependencies = [
 "async-trait",
 "derive_more 0.99.17",

The lock was updated, but no toml files were updated. This seems odd...

Code quote:

 "derive_more 0.99.17"

---

crates/mempool/src/mempool_test.rs line 490 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I think it would be better to delete this function since we now have more fields to assert, and we can check each one separately as needed.

Delete it from this test - and not in general, I see.
Is this the intended behavior?

---

crates/mempool/src/mempool_test.rs line 497 at r2 (raw file):

        .with_queue(expected_queue_txs)
        .with_account_nonces(expected_account_nonces)
        .build();

For the record, this suggested code does not require .collect. I am not sure it is a value we should aspire to, though. This suggestion is less readable.

Suggestion:

    // Assert: transactions are ordered by priority.
    let expected_mempool_content = MempoolContentBuilder::new()
        .with_account_nonces(add_tx_inputs.iter().map(|input| {
            let Account { sender_address, state: AccountState { nonce } } = input.account;
            (sender_address, nonce)
        }))
        .with_queue(add_tx_inputs.iter().map(|input| TransactionReference::new(&input.tx)))
        .with_pool(add_tx_inputs.into_iter().map(|input| input.tx))
        .build();

---

crates/mempool/src/mempool_test.rs line 497 at r2 (raw file):

        .with_queue(expected_queue_txs)
        .with_account_nonces(expected_account_nonces)
        .build();

Please align the order of the itereables and the order of the with_ methods (If it is possible). (Not necessarily in the suggested order - but in a readable way).

Suggestion:

    // Assert: transactions are ordered by priority.
    let expected_account_nonces: Vec<(ContractAddress, Nonce)> = add_tx_inputs
        .iter()
        .map(|input| {
            let Account { sender_address, state: AccountState { nonce } } = input.account;
            (sender_address, nonce)
        })
        .collect();
    let expected_queue_txs: Vec<TransactionReference> =
        add_tx_inputs.iter().map(|input| TransactionReference::new(&input.tx)).collect();
    let expected_pool_txs = add_tx_inputs.into_iter().map(|input| input.tx);
    let expected_mempool_content = MempoolContentBuilder::new()
        .with_account_nonces(expected_account_nonces)
        .with_queue(expected_queue_txs)
        .with_pool(expected_pool_txs)
        .build();

[ArniStarkware]

crates/mempool/src/mempool_test.rs line 497 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

For the record, this suggested code does not require .collect. I am not sure it is a value we should aspire to, though. This suggestion is less readable.

Collect is happening inside anyway.

[ArniStarkware]

crates/mempool/src/mempool_test.rs line 497 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Collect is happening inside anyway.

So, I regret that comment.

[ArniStarkware]

crates/mempool/src/mempool_test.rs line 497 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please align the order of the itereables and the order of the with_ methods (If it is possible). (Not necessarily in the suggested order - but in a readable way).

Please also extend this order to the assert lines.

[ayeletstarkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)

---

Cargo.lock line 9793 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The lock was updated, but no toml files were updated. This seems odd...

removed

---

crates/mempool/src/mempool_test.rs line 490 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Delete it from this test - and not in general, I see.
Is this the intended behavior?

I considered deleting this function, but it was decided to keep it, so I’m returning to using it.

---

crates/mempool/src/mempool_test.rs line 497 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please also extend this order to the assert lines.

Due to borrowing constraints, the iterators are in this order. I’ve arranged the following flow as optimally as possible.

[ArniStarkware]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)