feat: add a simple proxy mempool

[uriel-starkware]

add some very basic proxy mempool that wrap a real mempool and handles the calls to its functions.
This is only a POC

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 76.74%. Comparing base (72a2b06) to head (a9640d3).
Report is 3 commits behind head on main.

Files	Patch %	Lines
crates/mempool_node/src/mempool_proxy.rs	88.57%	2 Missing and 2 partials ⚠️
crates/mempool_node/src/mempool.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #44       +/-   ##
===========================================
+ Coverage   64.64%   76.74%   +12.09%     
===========================================
  Files           5        7        +2     
  Lines          99      172       +73     
  Branches       99      172       +73     
===========================================
+ Hits           64      132       +68     
- Misses         35       37        +2     
- Partials        0        3        +3     

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

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @uriel-starkware)

---

-- commits line 4 at r1:
Please change the entire commit message to
Add a proxy mempool example test

---

crates/mempool_node/src/main.rs line 11 at r1 (raw file):

#[cfg(test)]
mod proxy_mempool_test;

Please order the lines according to the guideline, e.g.:

pub mod ...

use ...

#[cfg(test)]
mod config_test;

fn {... }

Code quote:

#[cfg(test)]
mod proxy_mempool_test;

---

crates/mempool_node/src/mempool.rs line 1 at r1 (raw file):

pub trait MemPool {

1. Refactor MemPool -> Mempool (throughout the entire code).
2. Please rename the trait to MempoolTrait

Code quote:

 MemPool

---

crates/mempool_node/src/mempool.rs line 2 at r1 (raw file):

pub trait MemPool {
    fn add_transaction(&mut self, trx: u32) -> impl std::future::Future<Output = bool> + Send;

This is an async function:

1. You will the #[async_trait] macro for the trait.
2. Please add the async keyword for the function.
3. The return value should be simply the return type, i.e., no Future.

---

crates/mempool_node/src/mempool.rs line 6 at r1 (raw file):

#[derive(Default)]
pub struct DummyActualMemPool {

This is simply Mempool

Code quote:

DummyActualMemPool

---

crates/mempool_node/src/mempool.rs line 11 at r1 (raw file):

impl DummyActualMemPool {
    pub fn new() -> Self {
        DummyActualMemPool {

IIUC you can replace this with Self.

---

crates/mempool_node/src/mempool.rs line 18 at r1 (raw file):

impl MemPool for DummyActualMemPool {
    async fn add_transaction(&mut self, trx: u32) -> bool {

Can you define these as a specific type instead of u32 and bool? I.e., create two dummy types a la

type AddTransactionCallType = u32;
type AddTransactionReturnType = bool;

---

crates/mempool_node/src/proxy_mempool.rs line 16 at r1 (raw file):

}

pub struct ProxyMemPool {

I'd prefer MempoolProxy, but keep as is if you prefer it this way.

---

crates/mempool_node/src/proxy_mempool_test.rs line 1 at r1 (raw file):

mod test {

test -> tests

---

crates/mempool_node/src/proxy_mempool_test.rs line 7 at r1 (raw file):

    use crate::{
        mempool::{self, MemPool},

Is this needed?

---

crates/mempool_node/src/proxy_mempool_test.rs line 16 at r1 (raw file):

        let mut proxy = proxy_mempool::ProxyMemPool::new(mempool);

        assert!(proxy.add_transaction(1).await);

Please check the return value is as expected, i.e., true.

[uriel-starkware]

Reviewable status: 0 of 6 files reviewed, 11 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

-- commits line 4 at r1:

Previously, Itay-Tsabary-Starkware wrote…

Please change the entire commit message to
Add a proxy mempool example test

Done.

---

crates/mempool_node/src/main.rs line 11 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please order the lines according to the guideline, e.g.:

pub mod ...

use ...

#[cfg(test)]
mod config_test;

fn {... }

Done.

---

crates/mempool_node/src/mempool.rs line 1 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

1. Refactor MemPool -> Mempool (throughout the entire code).
2. Please rename the trait to MempoolTrait

Done.

---

crates/mempool_node/src/mempool.rs line 2 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This is an async function:

1. You will the #[async_trait] macro for the trait.
2. Please add the async keyword for the function.
3. The return value should be simply the return type, i.e., no Future.

Done.

---

crates/mempool_node/src/mempool.rs line 6 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This is simply Mempool

Done.

---

crates/mempool_node/src/mempool.rs line 11 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

IIUC you can replace this with Self.

Done.

---

crates/mempool_node/src/mempool.rs line 18 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can you define these as a specific type instead of u32 and bool? I.e., create two dummy types a la

type AddTransactionCallType = u32;
type AddTransactionReturnType = bool;

Done.

---

crates/mempool_node/src/proxy_mempool.rs line 16 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I'd prefer MempoolProxy, but keep as is if you prefer it this way.

Done.

---

crates/mempool_node/src/proxy_mempool_test.rs line 1 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

test -> tests

Done.

---

crates/mempool_node/src/proxy_mempool_test.rs line 7 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is this needed?

Yes, if I comment out this line it yells it doesn't recognize the add_transaction function (ie, the trait) but you are correct that I don't need self, I can just import Mempool (same for mempool_proxy)

---

crates/mempool_node/src/proxy_mempool_test.rs line 16 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please check the return value is as expected, i.e., true.

What do you mean? If I change it to the following, it complains that I use a literal bool and suggests I use assert!(...)

Code snippet:

assert_eq!(proxy.add_transaction(1).await, true);

[Itay-Tsabary-Starkware]

Reviewed 1 of 6 files at r2.
Reviewable status: 1 of 6 files reviewed, 12 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_node/Cargo.toml line 13 at r2 (raw file):

[dependencies]
tokio.workspace = true
async-trait = "0.1.79"

Add a newline at the end

Code quote:

async-trait = "0.1.79"

---

crates/mempool_node/src/mempool.rs line 26 at r2 (raw file):

#[async_trait]
impl MempoolTrait for Mempool {
    async fn add_transaction(&mut self, trx: u32) -> bool {

Should be the new type.

---

crates/mempool_node/src/mempool.rs line 26 at r2 (raw file):

#[async_trait]
impl MempoolTrait for Mempool {
    async fn add_transaction(&mut self, trx: u32) -> bool {

Should be the new type.

---

crates/mempool_node/src/mempool.rs line 26 at r2 (raw file):

#[async_trait]
impl MempoolTrait for Mempool {
    async fn add_transaction(&mut self, trx: u32) -> bool {

Change trx to tx (throughout).

Code quote:

 trx

---

crates/mempool_node/src/mempool_proxy.rs line 10 at r2 (raw file):

enum ProxyFunc {
    AddTransaction(u32),

Should be the new type.

---

crates/mempool_node/src/mempool_proxy.rs line 14 at r2 (raw file):

enum ProxyRetValue {
    AddTransaction(bool),

Should be the new type.

---

crates/mempool_node/src/mempool_proxy.rs line 35 at r2 (raw file):

                            .send(ProxyRetValue::AddTransaction(ret_value))
                            .await
                            .unwrap();

Please use expect with an indicative message on why this should never fail.

---

crates/mempool_node/src/mempool_proxy.rs line 39 at r2 (raw file):

                }
            }
        });

Nicely done.

Code quote:

        task::spawn(async move {
            while let Some(call) = rx_call.recv().await {
                match call {
                    ProxyFunc::AddTransaction(trx) => {
                        let ret_value = mempool.lock().await.add_transaction(trx).await;
                        tx_ret_value
                            .send(ProxyRetValue::AddTransaction(ret_value))
                            .await
                            .unwrap();
                    }
                }
            }
        });

---

crates/mempool_node/src/mempool_proxy.rs line 56 at r2 (raw file):

            .unwrap();
        match self.rx_ret_value.recv().await {
            Some(ProxyRetValue::AddTransaction(b)) => b,

Please be consistent and indicative with variable names.

Code quote:

b

---

crates/mempool_node/src/proxy_mempool_test.rs line 16 at r1 (raw file):

Previously, uriel-starkware wrote…

What do you mean? If I change it to the following, it complains that I use a literal bool and suggests I use assert!(...)

I wouldn't want that the arbitrary return type chosen for this kick-off (i.e., bool) will dictate the assert syntax. If there's no other work around, e.g., using the newly-defined type alias, then please change its definition to something else.

[uriel-starkware]

Reviewable status: 1 of 6 files reviewed, 12 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_node/Cargo.toml line 13 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Add a newline at the end

Done.

---

crates/mempool_node/src/mempool.rs line 26 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Should be the new type.

Done.

---

crates/mempool_node/src/mempool.rs line 26 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Should be the new type.

Done.

---

crates/mempool_node/src/mempool.rs line 26 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Change trx to tx (throughout).

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 10 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Should be the new type.

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 14 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Should be the new type.

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 35 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please use expect with an indicative message on why this should never fail.

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 39 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Nicely done.

👍

---

crates/mempool_node/src/mempool_proxy.rs line 56 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please be consistent and indicative with variable names.

Done.

---

crates/mempool_node/src/proxy_mempool_test.rs line 16 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I wouldn't want that the arbitrary return type chosen for this kick-off (i.e., bool) will dictate the assert syntax. If there's no other work around, e.g., using the newly-defined type alias, then please change its definition to something else.

changed the add_transaction to return the size of inner vec of transactions and then asserted with the expected size after such calls

[Itay-Tsabary-Starkware]

Reviewed 1 of 6 files at r2.
Reviewable status: 2 of 6 files reviewed, 8 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_node/src/mempool_proxy.rs line 35 at r2 (raw file):

Previously, uriel-starkware wrote…

Done.

This call should never fail because the Receiver should not be dropped. Adjust the text accordingly please.

Please see the documentation here: https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.Sender.html#implementations

---

crates/mempool_node/src/mempool_proxy.rs line 56 at r4 (raw file):

            .send((ProxyFunc::AddTransaction(tx), tx_response))
            .await
            .expect("Receiver is always listening in a dedicated task");

-> "Receiver should be listening."

---

crates/mempool_node/src/mempool_proxy.rs line 59 at r4 (raw file):

        match rx_response.recv().await.expect(
            "Receiver of the function call always returns a return value after sending a func call",

remove this please.

Code quote:

return

---

crates/mempool_node/src/mempool_proxy.rs line 59 at r4 (raw file):

        match rx_response.recv().await.expect(
            "Receiver of the function call always returns a return value after sending a func call",

General phrasing of expect messages: X should Y

Code quote:

"Receiver of the function call always returns a return value after sending a func call",

---

crates/mempool_node/src/mempool_proxy_test.rs line 11 at r4 (raw file):

    async fn test_proxy_simple_add_transaction() {
        let mut proxy = MempoolProxy::default();
        assert_eq!(proxy.add_transaction(1).await, 1);

This isn't clear; suggestion: Create a new instance of the required type (i.e., calldata) with the value and pass it explicilty to the function. Same with the return value -- defined the expected value and assert it matches the required one.

Code quote:

1

---

crates/mempool_node/src/mempool_proxy_test.rs line 21 at r4 (raw file):

        task::spawn(async move {
            proxy2.add_transaction(2).await;
        });

Suggestion:

1. Wrap this in a loop of 5 or so iterations
2. Use async clone instead of move, removing the need to define proxy2 out side.
3. Collect the resultant values into a vector
4. Sort it, and compared to the sorted expected result

Code quote:

        task::spawn(async move {
            proxy2.add_transaction(2).await;
        });

---

crates/mempool_node/src/mempool_proxy_test.rs line 23 at r4 (raw file):

        });

        assert_eq!(proxy1.add_transaction(1).await, 1);

This is a race condition with the invocation of proxy2, please see my suggestion above.

---

crates/mempool_node/src/mempool_proxy_test.rs line 24 at r4 (raw file):

        assert_eq!(proxy1.add_transaction(1).await, 1);
        sleep(Duration::from_millis(1)).await;

This is too volatile, we can't rely that this sleep is sufficient.

[Itay-Tsabary-Starkware]

Reviewable status: 2 of 6 files reviewed, 8 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_node/src/mempool_proxy_test.rs line 24 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This is too volatile, we can't rely that this sleep is sufficient.

(please remove, this also becomes redundant following the changes above)

[uriel-starkware]

Reviewable status: 2 of 6 files reviewed, 8 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_node/src/mempool_proxy.rs line 35 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This call should never fail because the Receiver should not be dropped. Adjust the text accordingly please.

Please see the documentation here: https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.Sender.html#implementations

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 56 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

-> "Receiver should be listening."

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 59 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

remove this please.

Done.

---

crates/mempool_node/src/mempool_proxy.rs line 59 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

General phrasing of expect messages: X should Y

Done.

---

crates/mempool_node/src/mempool_proxy_test.rs line 11 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This isn't clear; suggestion: Create a new instance of the required type (i.e., calldata) with the value and pass it explicilty to the function. Same with the return value -- defined the expected value and assert it matches the required one.

Done.

---

crates/mempool_node/src/mempool_proxy_test.rs line 21 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Suggestion:

1. Wrap this in a loop of 5 or so iterations
2. Use async clone instead of move, removing the need to define proxy2 out side.
3. Collect the resultant values into a vector
4. Sort it, and compared to the sorted expected result

Done.

---

crates/mempool_node/src/mempool_proxy_test.rs line 23 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This is a race condition with the invocation of proxy2, please see my suggestion above.

Done.

---

crates/mempool_node/src/mempool_proxy_test.rs line 24 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

(please remove, this also becomes redundant following the changes above)

Done.

[Itay-Tsabary-Starkware]

Reviewed 1 of 4 files at r3, all commit messages.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_node/src/mempool_proxy.rs line 41 at r5 (raw file):

                            .expect("Receiver should be listening.");
                    }
                }

Should run in a new task

Code quote:

                match call {
                    (ProxyFunc::AddTransaction(tx), tx_response) => {
                        let ret_value = mempool.add_transaction(tx).await;
                        tx_response
                            .send(ProxyRetValue::AddTransaction(ret_value))
                            .await
                            .expect("Receiver should be listening.");
                    }
                }

---

crates/mempool_node/src/mempool_proxy_test.rs line 14 at r5 (raw file):

        let mut proxy = MempoolProxy::default();
        let tx: AddTransactionCallType = 1;
        let expect_result: AddTransactionReturnType = 1;

expected_result

Code quote:

expect_result

[uriel-starkware]

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_node/src/mempool_proxy_test.rs line 14 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

expected_result

Done.

[uriel-starkware]

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_node/src/mempool_proxy.rs line 41 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Should run in a new task

Done.

[Itay-Tsabary-Starkware]

Reviewed 2 of 3 files at r7, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @uriel-starkware)

---

crates/mempool_node/src/mempool_proxy_test.rs line 12 at r7 (raw file):

    };

    async fn test_mempool_simple_add_transaction<T>(mempool: T)

Please remove simple. Maybe replace with single_thread to indicate the difference with the concurrent option below.

Also please re-order the functions such that the util functions are at the top and the tests are at the bottom.

Code quote:

simple_

[uriel-starkware]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/mempool_node/src/mempool_proxy_test.rs line 12 at r7 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please remove simple. Maybe replace with single_thread to indicate the difference with the concurrent option below.

Also please re-order the functions such that the util functions are at the top and the tests are at the bottom.

Done.

[Itay-Tsabary-Starkware]

Reviewed all commit messages.
Reviewable status: 5 of 6 files reviewed, all discussions resolved

[Itay-Tsabary-Starkware]

Reviewed 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @uriel-starkware)