test(mempool): refactor MempoolState to have Partial and Full states

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.21%. Comparing base (93de0bd) to head (232db3d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #507   +/-   ##
=======================================
  Coverage   81.21%   81.21%           
=======================================
  Files          42       42           
  Lines        1826     1826           
  Branches     1826     1826           
=======================================
  Hits         1483     1483           
  Misses        269      269           
  Partials       74       74           

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

[MohammadNassar1]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool_test.rs line 50 at r1 (raw file):

        Self { tx_pool: None, tx_queue: None, _phantom: PhantomData }
    }
}

Please move it to the end.
Usually, the struct implementations appear before the trait implementation.

Code quote:

impl Default for MempoolState<PartialState> {
    fn default() -> Self {
        Self { tx_pool: None, tx_queue: None, _phantom: PhantomData }
    }
}

---

crates/mempool/src/mempool_test.rs line 68 at r1 (raw file):

}

impl<T> MempoolState<T> {

What's T? I dont see it's usage.
Is it Partail Struct of Full Struct?

Code quote:

impl<T> MempoolState<T> {

[MohammadNassar1]

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

---

crates/mempool/src/mempool_test.rs line 27 at r1 (raw file):

    tx_queue: Option<TransactionQueue>,
    _phantom: PhantomData<T>,
}

Can you please add a comment that explains the magic here?

Code quote:

struct MempoolState<T> {
    tx_pool: Option<TransactionPool>,
    tx_queue: Option<TransactionQueue>,
    _phantom: PhantomData<T>,
}

[elintul]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool_test.rs line 26 at r1 (raw file):

    tx_pool: Option<TransactionPool>,
    tx_queue: Option<TransactionQueue>,
    _phantom: PhantomData<T>,

Fully-qualified, please; it's not too long.

Code quote:

PhantomData

---

crates/mempool/src/mempool_test.rs line 27 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Can you please add a comment that explains the magic here?

TAL at the conversation (inc. replies) here.

---

crates/mempool/src/mempool_test.rs line 30 at r1 (raw file):

struct FullState;
struct PartialState;

Suggestion:

#[derive(Debug)]
struct MempoolState<T> {
    tx_pool: Option<TransactionPool>,
    tx_queue: Option<TransactionQueue>,
    _phantom: PhantomData<T>,
}

#[derive(Debug)]
struct FullState;
#[derive(Debug)]
struct PartialState;

---

crates/mempool/src/mempool_test.rs line 33 at r1 (raw file):

impl MempoolState<FullState> {
    fn new<P, Q>(pool_txs: P, queue_txs: Q) -> Self

I also remember a suggestion to create TransactionReference objects in this function.
Please implement in a separate PR.

Code quote:

new

---

crates/mempool/src/mempool_test.rs line 68 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What's T? I dont see it's usage.
Is it Partail Struct of Full Struct?

Yup, that's what the state generic of.

---

crates/mempool/src/mempool_test.rs line 83 at r1 (raw file):

}

impl<T> From<MempoolState<T>> for Mempool {

Doesn't impl. change depending on the type of the state (partial/full)? unwrap won't always work.

Code quote:

impl<T> From<MempoolState<T>> for Mempool

[ayeletstarkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/mempool/src/mempool_test.rs line 30 at r1 (raw file):

struct FullState;
struct PartialState;

Why add #[derive(Debug)] if the structs aren't currently used for debugging? Isn't it better to add it only when we actually need it?

---

crates/mempool/src/mempool_test.rs line 33 at r1 (raw file):

Previously, elintul (Elin) wrote…

I also remember a suggestion to create TransactionReference objects in this function.
Please implement in a separate PR.

added a todo

---

crates/mempool/src/mempool_test.rs line 83 at r1 (raw file):

Previously, elintul (Elin) wrote…

Doesn't impl. change depending on the type of the state (partial/full)? unwrap won't always work.

Correct. How's this solution?

[elintul]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/mempool_test.rs line 30 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Why add #[derive(Debug)] if the structs aren't currently used for debugging? Isn't it better to add it only when we actually need it?

We know we'll need it, might as well add now.

---

crates/mempool/src/mempool_test.rs line 83 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Correct. How's this solution?

What would be the default? None or []?

---

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

use crate::transaction_queue::TransactionQueue;

/// MempoolState represents the internal state of the mempool.

State with present tense verb; do not include struct/variable names in doc., it might get outdated.

Suggestion:

/// R

---

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

/// Enables customized (and potentially inconsistent) creation for unit testing.
/// The `PhantomData` marks the state type (`FullState` or `PartialState`) without storing data of
/// that type, enabling type-safe operations based on the state.

Not sure I understand.

Code quote:

/// The `PhantomData` marks the state type (`FullState` or `PartialState`) without storing data of
/// that type, enabling type-safe operations based on the state.

---

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

impl Default for MempoolState<PartialState> {
    fn default() -> Self {
        Self { tx_pool: None, tx_queue: None, _phantom: std::marker::PhantomData }

Why is that the default implementation?

Code quote:

tx_pool: None, tx_queue: None

[ayeletstarkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/mempool/src/mempool_test.rs line 83 at r1 (raw file):

Previously, elintul (Elin) wrote…

What would be the default? None or []?

would be the default of TransactionPool and TransactionQueue

---

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

Previously, elintul (Elin) wrote…

Not sure I understand.

fixed

---

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

Previously, elintul (Elin) wrote…

Why is that the default implementation?

for the partial state, it's being used in _with_pool, _with_queue.

[ayeletstarkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @elintul and @giladchase)

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

for the partial state, it's being used in _with_pool, _with_queue.

not used anymore.

[elintul]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

fixed

"Artificially use generic type, for the compiler."

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

[elintul]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/mempool_test.rs line 26 at r4 (raw file):

    tx_pool: Option<TransactionPool>,
    tx_queue: Option<TransactionQueue>,
    // PhantomData<T> artificially use generic type, for the compiler.

Try not to quote parts from code (names, etc.) in doc., so it'll be less prone to becoming stale.

Suggestion:

// Artificially use generic type, for the compiler.

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

[elintul]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/mempool_test.rs line 320 at r6 (raw file):

    // Assert: the original transaction remains.
    let expected_mempool_state = MempoolState::with_pool([input.tx]);

But queue was compared before.

Code quote:

with_pool

---

crates/mempool/src/mempool_test.rs line 365 at r6 (raw file):

        TransactionReference::new(&input_big_tip_small_hash.tx),
        TransactionReference::new(&input_small_tip_big_hash.tx),
    ];

map, please. In all tests.

Code quote:

    let expected_queue_txs = [
        TransactionReference::new(&input_big_tip_small_hash.tx),
        TransactionReference::new(&input_small_tip_big_hash.tx),
    ];