Skip to content

Commit

Permalink
chore(sequencer): simplify boolean expressions in `transaction contai…
Browse files Browse the repository at this point in the history
…ner` (#1595)

## Summary
Simplified logical statements in
`transaction_priority_comparisons_should_be_consistent_nonce_diff()`.

## Background
Previously there was an allow for `clippy::nonminimal_bool`. The
reasoning behind it was to match documented behavior. This change is
meant to explicitly state the expected behavior while still simplifying
the boolean expressions. This is in response to this comment:
#1561 (comment)

## Changes
- Simplified boolean expressions and moved the non-simplified versions
to the comments where applicable to provide context on the documented
behavior.

## Testing
Passing all tests

## Related Issues
closes #1583
  • Loading branch information
ethanoroshiba authored Oct 2, 2024
1 parent 29a5d19 commit 6fdf4ea
Showing 1 changed file with 18 additions and 27 deletions.
45 changes: 18 additions & 27 deletions crates/astria-sequencer/src/mempool/transactions_container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -877,11 +877,6 @@ mod tests {

// From https://doc.rust-lang.org/std/cmp/trait.PartialOrd.html
#[test]
// TODO (https://github.com/astriaorg/astria/issues/1583): rework assertions and remove attribute
#[expect(
clippy::nonminimal_bool,
reason = "we want explicit assertions here to match the documented expected behavior"
)]
fn transaction_priority_comparisons_should_be_consistent_nonce_diff() {
let instant = Instant::now();

Expand All @@ -900,41 +895,37 @@ mod tests {

// 1. a == b if and only if partial_cmp(a, b) == Some(Equal)
assert!(high == high); // Some(Equal)
assert!(!(high == low)); // Some(Greater)
assert!(!(low == high)); // Some(Less)
assert!(high != low); // Some(Greater)
assert!(low != high); // Some(Less)

// 2. a < b if and only if partial_cmp(a, b) == Some(Less)
assert!(low < high); // Some(Less)
assert!(!(high < high)); // Some(Equal)
assert!(!(high < low)); // Some(Greater)
assert!(high >= high); // Some(Equal)
assert!(high >= low); // Some(Greater)

// 3. a > b if and only if partial_cmp(a, b) == Some(Greater)
assert!(high > low); // Some(Greater)
assert!(!(high > high)); // Some(Equal)
assert!(!(low > high)); // Some(Less)
assert!(high <= high); // Some(Equal)
assert!(low <= high); // Some(Less)

// 4. a <= b if and only if a < b || a == b
assert!(low <= high); // a < b
assert!(high <= high); // a == b
assert!(!(high <= low)); // a > b
assert!(high > low); // !(b <= a)

// 5. a >= b if and only if a > b || a == b
assert!(high >= low); // a > b
assert!(high >= high); // a == b
assert!(!(low >= high)); // a < b
assert!(low < high); // !(b >= a)

// 6. a != b if and only if !(a == b)
assert!(high != low); // asserted !(high == low) above
assert!(low != high); // asserted !(low == high) above
assert!(!(high != high)); // asserted high == high above
assert!(high == high); // asserted high == high above
}

// From https://doc.rust-lang.org/std/cmp/trait.PartialOrd.html
#[test]
#[expect(
clippy::nonminimal_bool,
reason = "we want explicit assertions here to match the documented expected behavior"
)]
fn transaction_priority_comparisons_should_be_consistent_time_gap() {
let high = TransactionPriority {
nonce_diff: 0,
Expand All @@ -951,33 +942,33 @@ mod tests {

// 1. a == b if and only if partial_cmp(a, b) == Some(Equal)
assert!(high == high); // Some(Equal)
assert!(!(high == low)); // Some(Greater)
assert!(!(low == high)); // Some(Less)
assert!(high != low); // Some(Greater)
assert!(low != high); // Some(Less)

// 2. a < b if and only if partial_cmp(a, b) == Some(Less)
assert!(low < high); // Some(Less)
assert!(!(high < high)); // Some(Equal)
assert!(!(high < low)); // Some(Greater)
assert!(high >= high); // Some(Equal)
assert!(high >= low); // Some(Greater)

// 3. a > b if and only if partial_cmp(a, b) == Some(Greater)
assert!(high > low); // Some(Greater)
assert!(!(high > high)); // Some(Equal)
assert!(!(low > high)); // Some(Less)
assert!(high <= high); // Some(Equal)
assert!(low <= high); // Some(Less)

// 4. a <= b if and only if a < b || a == b
assert!(low <= high); // a < b
assert!(high <= high); // a == b
assert!(!(high <= low)); // a > b
assert!(high > low); // !(b <= a)

// 5. a >= b if and only if a > b || a == b
assert!(high >= low); // a > b
assert!(high >= high); // a == b
assert!(!(low >= high)); // a < b
assert!(low < high); // !(low >= high)

// 6. a != b if and only if !(a == b)
assert!(high != low); // asserted !(high == low) above
assert!(low != high); // asserted !(low == high) above
assert!(!(high != high)); // asserted high == high above
assert!(high == high); // asserted !(high != high) above
}

#[test]
Expand Down

0 comments on commit 6fdf4ea

Please sign in to comment.