feat(mempool): add pending transaction type

[MohammadNassar1]

This change is 

[elintul]

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

---

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

        }
    }

Aren't there these getters in SN API?

---

crates/mempool/src/transaction_queue.rs line 71 at r1 (raw file):

}

/// Encapsulates a transaction reference to assess its order (i.e., priority).

Update comment.

Code quote:

priority

---

crates/mempool/src/transaction_queue.rs line 72 at r1 (raw file):

/// Encapsulates a transaction reference to assess its order (i.e., priority).
/// A transaction with gas price below the threshold. Sorted according to the gas price.

This belongs next to the data structure that will be holding them; it's our of scope for the transaction struct itself.

Code quote:

/// A transaction with gas price below the threshold. Sorted according to the gas price.

---

crates/mempool/src/transaction_queue.rs line 77 at r1 (raw file):

/// Compare transactions based only on their gas price, a uint, using the Eq trait. It ensures that
/// two gas price are either exactly equal or not.

But it's the PartialEq trait, I'm confused.
Also, this comment isn't clear to me in general. Did you understand it?

Code quote:

/// Compare transactions based only on their gas price, a uint, using the Eq trait. It ensures that
/// two gas price are either exactly equal or not.

---

crates/mempool/src/transaction_queue.rs line 101 at r1 (raw file):

        Some(self.cmp(other))
    }
}

I wouldn't repeat all comments for the two structs. Better to refer the reader to PendingTransaction's documentation.

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Aren't there these getters in SN API?

No.

---

crates/mempool/src/transaction_queue.rs line 101 at r1 (raw file):

Previously, elintul (Elin) wrote…

I wouldn't repeat all comments for the two structs. Better to refer the reader to PendingTransaction's documentation.

Done

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

No.

Don't think they should be public though. Note visibility in general; and specifically when the compiler doesn't complain about unused code - means the code is public, we should be convinced it needs to be.

---

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

    }

    pub fn get_l2_gas_price(&self) -> Option<u128> {

Do we need this?

Code quote:

get_l2_gas_price

---

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

}

/// This struct behaves similarly to [`PendingTransaction`], It encapsulates a transaction reference

Suggestion:

, encapsulating

---

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

/// # See Also
///
/// - [`PendingTransaction`]: for details.

How do I see the resulting doc. file from this?

Code quote:

/// # See Also
///
/// - [`PendingTransaction`]: for details.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 77 at r1 (raw file):

Previously, elintul (Elin) wrote…

But it's the PartialEq trait, I'm confused.
Also, this comment isn't clear to me in general. Did you understand it?

I think Gilad added this comment.
What's unclear exactly?
PartialEq implements eq method.

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Don't think they should be public though. Note visibility in general; and specifically when the compiler doesn't complain about unused code - means the code is public, we should be convinced it needs to be.

It needs to be public so that it can be used in tx queue file.

---

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

Previously, elintul (Elin) wrote…

Do we need this?

I assume you mean l1.
Removed; it will be added once mempool considers the l1 price.

---

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

}

/// This struct behaves similarly to [`PendingTransaction`], It encapsulates a transaction reference

Done.

---

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

Previously, elintul (Elin) wrote…

How do I see the resulting doc. file from this?

Not sure I get it.
What do you mean?

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It needs to be public so that it can be used in tx queue file.

Okay, cool. I thought it was the other way around.

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Not sure I get it.
What do you mean?

The HTML doc. that is created from this doc.; I think you should run cargo build docs or something similar. Was wondering about this syntax, since I don't recognize it.

---

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

struct PendingTransaction(pub TransactionReference);

/// Compare transactions based only on their gas price, a uint, using the Eq trait. It ensures that

[elintul]

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

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

The HTML doc. that is created from this doc.; I think you should run cargo build docs or something similar. Was wondering about this syntax, since I don't recognize it.

It seems that it only generates public code, and transactino_queue is a private module.
See output:
file:///home/lt-mohammadn/workspace/sequencer1/target/doc/starknet_mempool/mempool/struct.Mempool.html

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 105 at r4 (raw file):

/// to assess its order (i.e., tip).
///
/// # See also `PendingTransaction` docstring for details.

Suggestion:

/// to assess its order (i.e., tip); see its documentation for more details.

[elintul]

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

---

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

    pub resource_bounds: ResourceBoundsMapping,
}

Keep error messages (tip and L2 gas) in same format.
Also, is this the earliest point of time this can happen in?

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Keep error messages (tip and L2 gas) in same format.
Also, is this the earliest point of time this can happen in?

Done.
As the current Transaction type may be a deprecated version, this is the earliest point.

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.
As the current Transaction type may be a deprecated version, this is the earliest point.

@ArniStarkware, should be changed.

---

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

            .get(&Resource::L2Gas)
            .map(|bounds| bounds.max_price_per_unit)
            .expect("Expected L2Gas resource found")

Why is it still optional here? This should change in 0.13.3, right?
Please align the error message here too.

Code quote:

            .map(|bounds| bounds.max_price_per_unit)
            .expect("Expected L2Gas resource found")

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Why is it still optional here? This should change in 0.13.3, right?
Please align the error message here too.

because resource bounds is a map of resource to resource bound.
So here it is optional since the map keys "may" not include L2Resource.

What do you mean by "align the error message"? Here, I cannot say "expected a valid resource but received none." The resource is not none, the resource is not existed, so I wrote "L2Gas should be found". wdyt?

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

because resource bounds is a map of resource to resource bound.
So here it is optional since the map keys "may" not include L2Resource.

What do you mean by "align the error message"? Here, I cannot say "expected a valid resource but received none." The resource is not none, the resource is not existed, so I wrote "L2Gas should be found". wdyt?

You can say expected a valid X value, without say what you received.

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

You can say expected a valid X value, without say what you received.

Done.

[elintul]

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

---

crates/mempool/src/mempool.rs line 212 at r8 (raw file):

            nonce: tx.nonce(),
            tx_hash: tx.tx_hash(),
            tip: tx.tip().expect("Expected a valid tip value, but received None."),

Suggestion:

"Expected a valid tip value."

---

crates/mempool/src/mempool.rs line 215 at r8 (raw file):

            resource_bounds: tx
                .resource_bounds()
                .expect("Expected a valid resource bounds value, but received None.")

Suggestion:

"Expected a valid resource bounds value."

---

crates/mempool/src/mempool.rs line 225 at r8 (raw file):

            .get(&Resource::L2Gas)
            .map(|bounds| bounds.max_price_per_unit)
            .expect("Expected a valid L2Gas resource.")

Suggestion:

"Expected a valid L2 gas resource bounds."

[elintul]

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

[ArniStarkware]

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

Previously, elintul (Elin) wrote…

@ArniStarkware, should be changed.

I don't follow.

[ArniStarkware]

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

Previously, ArniStarkware (Arnon Hod) wrote…

I don't follow.

I do follow :)

[elintul]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware and @giladchase)