Skip to content

Conversation

julio4
Copy link
Collaborator

@julio4 julio4 commented Sep 24, 2025

PartialEq impl for Eligibility do not coerce to bool, only allows to compare against bool values.
It might be better to explicitly convert to bool when necessary.

@julio4 julio4 requested a review from karim-agha as a code owner September 24, 2025 01:46
@karim-agha
Copy link
Collaborator

karim-agha commented Sep 24, 2025

The current PartialEq<bool> allows us to use the following syntax:

if bundle.is_eligible() {
  // code
}

// or

let bundles = stream.filter(|b| bundle.is_eligible());

What downsides do you see to having this syntactic sugar? The alternative with this pr would be

if bundle.is_eligible().as_bool() {
  // code
}

// or

let bundles = stream.filter(|b| !(!bundle.is_eligible()));

@julio4
Copy link
Collaborator Author

julio4 commented Sep 25, 2025

The current PartialEq<bool> allows us to use the following syntax:

if bundle.is_eligible() {
  // code
}

I agree that this is the best syntax we could have, however condition operands only accept boolean expressions and will not implicitly call cmp impls such as PartialEq. So this will fails:

         if bundle.is_eligible(block) {
            ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `bool`, found `Eligibility`

The PartialEq allows to do that:

if bundle.is_eligible() == true {
  // code
}

But I think as_bool is a more idiomatic and explicit way of doing it.

@karim-agha
Copy link
Collaborator

Maybe we can implement as_bool in terms of From/Into?

@julio4
Copy link
Collaborator Author

julio4 commented Sep 26, 2025

Maybe we can implement as_bool in terms of From/Into?

We can as well, but there's still the need to call .into() in condition.
as_bool is a bit more explicit, and Into a bit more straigthforward, imo both are fine.

@julio4 julio4 force-pushed the fix/bundle-eligibility branch from 6efa709 to d554c41 Compare September 29, 2025 02:50
@julio4
Copy link
Collaborator Author

julio4 commented Sep 29, 2025

Changed to Into

@julio4 julio4 force-pushed the fix/bundle-eligibility branch from d554c41 to f4e7285 Compare September 29, 2025 03:37
@dmarzzz dmarzzz merged commit 4a64ae3 into flashbots:main Oct 6, 2025
3 checks passed
@julio4 julio4 deleted the fix/bundle-eligibility branch October 6, 2025 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants