From e72660056e00c93d6b054dfb08ff34a1c67cb799 Mon Sep 17 00:00:00 2001
From: Sean Bowe <ewillbefull@gmail.com>
Date: Sat, 15 Jul 2017 15:32:37 -0600
Subject: [PATCH] Ordering cleanup for Fq/Fq2, with documentation. Closes #9.

---
 src/bls12_381/ec.rs  |  4 ++--
 src/bls12_381/fq.rs  | 37 +++++++++++++++++++++++++++++++------
 src/bls12_381/fq2.rs |  7 ++++---
 3 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/src/bls12_381/ec.rs b/src/bls12_381/ec.rs
index b2b18a9..4350b12 100644
--- a/src/bls12_381/ec.rs
+++ b/src/bls12_381/ec.rs
@@ -695,7 +695,7 @@ pub mod g1 {
                     negy.negate();
 
                     // Get the parity of the sqrt we found.
-                    let parity = y.into_repr() > negy.into_repr();
+                    let parity = y > negy;
 
                     Ok(G1Affine {
                         x: x,
@@ -735,7 +735,7 @@ pub mod g1 {
 
                 // If the correct y coordinate is the largest (lexicographically),
                 // the bit should be set.
-                if affine.y.into_repr() > negy.into_repr() {
+                if affine.y > negy {
                     res.0[0] |= 1 << 6; // Set second highest bit.
                 }
             }
diff --git a/src/bls12_381/fq.rs b/src/bls12_381/fq.rs
index 0505f68..b7f8198 100644
--- a/src/bls12_381/fq.rs
+++ b/src/bls12_381/fq.rs
@@ -1,4 +1,5 @@
 use ::{Field, PrimeField, SqrtField, PrimeFieldRepr};
+use std::cmp::Ordering;
 use super::fq2::Fq2;
 
 // q = 4002409555221667393417789825735904156556882819939007885332058136124031650490837864442687629129015664037894272559787
@@ -233,22 +234,22 @@ impl From<u64> for FqRepr {
 
 impl Ord for FqRepr {
     #[inline(always)]
-    fn cmp(&self, other: &FqRepr) -> ::std::cmp::Ordering {
+    fn cmp(&self, other: &FqRepr) -> Ordering {
         for (a, b) in self.0.iter().rev().zip(other.0.iter().rev()) {
             if a < b {
-                return ::std::cmp::Ordering::Less
+                return Ordering::Less
             } else if a > b {
-                return ::std::cmp::Ordering::Greater
+                return Ordering::Greater
             }
         }
 
-        ::std::cmp::Ordering::Equal
+        Ordering::Equal
     }
 }
 
 impl PartialOrd for FqRepr {
     #[inline(always)]
-    fn partial_cmp(&self, other: &FqRepr) -> Option<::std::cmp::Ordering> {
+    fn partial_cmp(&self, other: &FqRepr) -> Option<Ordering> {
         Some(self.cmp(other))
     }
 }
@@ -357,6 +358,21 @@ impl PrimeFieldRepr for FqRepr {
 #[derive(Copy, Clone, PartialEq, Eq)]
 pub struct Fq(FqRepr);
 
+/// `Fq` elements are ordered lexicographically.
+impl Ord for Fq {
+    #[inline(always)]
+    fn cmp(&self, other: &Fq) -> Ordering {
+        self.into_repr().cmp(&other.into_repr())
+    }
+}
+
+impl PartialOrd for Fq {
+    #[inline(always)]
+    fn partial_cmp(&self, other: &Fq) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
 impl ::std::fmt::Debug for Fq
 {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
@@ -847,7 +863,7 @@ use rand::{SeedableRng, XorShiftRng, Rand};
 fn test_fq_repr_ordering() {
     fn assert_equality(a: FqRepr, b: FqRepr) {
         assert_eq!(a, b);
-        assert!(a.cmp(&b) == ::std::cmp::Ordering::Equal);
+        assert!(a.cmp(&b) == Ordering::Equal);
     }
 
     fn assert_lt(a: FqRepr, b: FqRepr) {
@@ -1718,3 +1734,12 @@ fn fq_field_tests() {
     ::tests::field::random_sqrt_tests::<Fq>();
     ::tests::field::random_frobenius_tests::<Fq, _>(Fq::char(), 13);
 }
+
+#[test]
+fn test_fq_ordering() {
+    // FqRepr's ordering is well-tested, but we still need to make sure the Fq
+    // elements aren't being compared in Montgomery form.
+    for i in 0..100 {
+        assert!(Fq::from_repr(FqRepr::from(i+1)) > Fq::from_repr(FqRepr::from(i)));
+    }
+}
diff --git a/src/bls12_381/fq2.rs b/src/bls12_381/fq2.rs
index cfec0a8..ec22782 100644
--- a/src/bls12_381/fq2.rs
+++ b/src/bls12_381/fq2.rs
@@ -1,5 +1,5 @@
 use rand::{Rng, Rand};
-use ::{Field, SqrtField, PrimeField};
+use ::{Field, SqrtField};
 use super::fq::{Fq, FROBENIUS_COEFF_FQ2_C1, NEGATIVE_ONE};
 
 use std::cmp::Ordering;
@@ -11,13 +11,14 @@ pub struct Fq2 {
     pub c1: Fq
 }
 
+/// `Fq2` elements are ordered lexicographically.
 impl Ord for Fq2 {
     #[inline(always)]
     fn cmp(&self, other: &Fq2) -> Ordering {
-        match self.c1.into_repr().cmp(&other.c1.into_repr()) {
+        match self.c1.cmp(&other.c1) {
             Ordering::Greater => Ordering::Greater,
             Ordering::Less => Ordering::Less,
-            Ordering::Equal => self.c0.into_repr().cmp(&other.c0.into_repr())
+            Ordering::Equal => self.c0.cmp(&other.c0)
         }
     }
 }