feat(mempool): implement remove method for transaction queue

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.40%. Comparing base (2c64fcd) to head (e56b0d1).
Report is 1 commits behind head on main.

Files	Patch %	Lines
crates/mempool/src/transaction_queue.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   83.69%   83.40%   -0.30%     
==========================================
  Files          37       37              
  Lines        1717     1723       +6     
  Branches     1717     1723       +6     
==========================================
  Hits         1437     1437              
- Misses        203      209       +6     
  Partials       77       77              

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

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 52 at r2 (raw file):

    }

    /// At most one transaction is associated with a single address in the queue.

Suggestion:

/// Removes the transaction of the given account address from the queue. 
/// This is well-defined, since there is at most one transaction per address in the queue.

---

crates/mempool/src/transaction_queue.rs line 53 at r2 (raw file):

    /// At most one transaction is associated with a single address in the queue.
    pub fn _remove(mut self, address: &ContractAddress) -> Option<TransactionReference> {

Temporary name until used?

Code quote:

_remove

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 53 at r2 (raw file):

    /// At most one transaction is associated with a single address in the queue.
    pub fn _remove(mut self, address: &ContractAddress) -> Option<TransactionReference> {

Pass self by reference, while address is Copy so no need.

Suggestion:

&mut self, address: ContractAddress

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 53 at r2 (raw file):

Previously, elintul (Elin) wrote…

Temporary name until used?

Exactly

---

crates/mempool/src/transaction_queue.rs line 53 at r2 (raw file):

Previously, elintul (Elin) wrote…

Pass self by reference, while address is Copy so no need.

Done.

[giladchase]

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

---

crates/mempool/src/transaction_queue.rs line 58 at r3 (raw file):

            self.queue.remove(&tx.into());
            tx
        })

I like the chaining attempt, but it's best to keep side-effects away from map; it's intended goal is for transformation, rather than side-effects.

Suggestion:

        let tx = self.address_to_tx.remove(&address)?
        
        self.queue.remove(&tx.into());
        tx
        
        /// Technically this will also work, but this feels more complicated.
        // let tx = self.address_to_tx.remove(&address)?;
        // self.queue.take(&tx.into()).map(|tx| tx.0)
        

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 58 at r3 (raw file):

Previously, giladchase wrote…

I like the chaining attempt, but it's best to keep side-effects away from map; it's intended goal is for transformation, rather than side-effects.

Done.

[giladchase]

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

[elintul]

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

[giladchase]

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

[elintul]

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

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 54 at r5 (raw file):

    /// Removes the transaction of the given account address from the queue.
    /// This is well-defined, since there is at most one transaction per address in the queue.
    pub fn _remove(&mut self, address: ContractAddress) -> Option<TransactionReference> {

I don't think we should return the reference, maybe a bool is enough?

Code quote:

Option<TransactionReference>

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/transaction_queue.rs line 54 at r5 (raw file):

Previously, elintul (Elin) wrote…

I don't think we should return the reference, maybe a bool is enough?

Done

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/transaction_queue.rs line 54 at r5 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done

WDYT about:

self.address_to_tx.remove(&address).map_or(false, |tx| self.queue.remove(&tx.into()))

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/transaction_queue.rs line 54 at r5 (raw file):

Previously, elintul (Elin) wrote…

WDYT about:

self.address_to_tx.remove(&address).map_or(false, |tx| self.queue.remove(&tx.into()))

But we said that we dont use map in this case, right?
and it's not a simple logic, it has a side effect.

[elintul]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

---

crates/mempool/src/transaction_queue.rs line 54 at r5 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

But we said that we dont use map in this case, right?
and it's not a simple logic, it has a side effect.

It's the or branch, the map side is pure. @giladchase - WDYT?