acir_field's FieldElement::is_negative invalid in some cases

[michaeljklein]

Aim

FieldElement::is_negative should be correct in all cases

Expected Behavior

I expect FieldElement::is_negative to be correct in all cases

Bug

acvm-repo/acir_field/src/field_element.rs

    fn is_negative(&self) -> bool {
        self.neg().num_bits() < self.num_bits()

As far as I can tell, this will be incorrect when the field element is in the middle of its range.

Instead, I believe we should compare without num_bits, i.e. self.neg() < self

To Reproduce

No response

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

[TomAFrench]

I've had a little look into this and the logic of this function is correct for how we're using it.

The only place we're using it is when converting a field element into an i128 and so we're going to have values in the ranges 0..2^127-1 (representing positive integers) and p-2^127..p (representing negative integers). If we were to use field elements outside of this range we would get invalid results anyway.

That said, this method is devoid of any context as it is which opens it up to being misused in future. We should just inline it into to_i128 as a result.