Merge pull request #58 from ProjectOpenSea/gas-updateStatus-scaledown

Improve `_updateStatus` scale-down
ProjectOpenSea · Mar 5, 2024 · fbb182b · fbb182b
2 parents 51dc0b5 + a564382
commit fbb182b
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 20 deletions.
diff --git a/reference/lib/ReferenceOrderValidator.sol b/reference/lib/ReferenceOrderValidator.sol
@@ -323,13 +323,9 @@ contract ReferenceOrderValidator is
                     || denominator > type(uint120).max
             ) {
                 // Derive greatest common divisor using euclidean algorithm.
-                uint256 scaleDown = _greatestCommonDivisor(
-                    numerator,
-                    _greatestCommonDivisor(filledNumerator, denominator)
-                );
+                uint256 scaleDown = _greatestCommonDivisor(filledNumerator, denominator);
 
-                // Scale all fractional values down by gcd.
-                numerator = numerator / scaleDown;
+                // Scale new filled fractional values down by gcd.
                 filledNumerator = filledNumerator / scaleDown;
                 denominator = denominator / scaleDown;
 

diff --git a/src/core/lib/OrderValidator.sol b/src/core/lib/OrderValidator.sol
@@ -517,15 +517,13 @@ contract OrderValidator is Executor, ZoneInteraction {
                         out := _a
                     }
 
-                    // Determine the amount to scale down the fill fractions.
-                    let scaleDown :=
-                        gcd(numerator, gcd(filledNumerator, denominator))
+                    // Determine the amount to scale down the new filled fraction.
+                    let scaleDown := gcd(filledNumerator, denominator)
 
                     // Ensure that the divisor is at least one.
                     let safeScaleDown := add(scaleDown, iszero(scaleDown))
 
-                    // Scale all fractional values down by gcd.
-                    numerator := div(numerator, safeScaleDown)
+                    // Scale new filled fractional values down by gcd.
                     filledNumerator := div(filledNumerator, safeScaleDown)
                     denominator := div(denominator, safeScaleDown)
 

diff --git a/test/foundry/FulfillAdvancedOrder.t.sol b/test/foundry/FulfillAdvancedOrder.t.sol
@@ -487,7 +487,7 @@ contract FulfillAdvancedOrder is BaseOrderTest {
             address(0)
         );
 
-        // Assert six-tenths of the order has been fulfilled.
+        // Assert three-fifths of the order has been fulfilled.
         {
             (
                 bool isValidated,
@@ -497,9 +497,9 @@ contract FulfillAdvancedOrder is BaseOrderTest {
             ) = context.consideration.getOrderStatus(orderHash);
             assertTrue(isValidated);
             assertFalse(isCancelled);
-            assertEq(totalFilled, 6);
+            assertEq(totalFilled, 3);
 
-            assertEq(totalSize, 10);
+            assertEq(totalSize, 5);
             assertEq(60, test1155_1.balanceOf(address(this), 1));
         }
     }

diff --git a/test/foundry/FulfillAvailableAdvancedOrder.t.sol b/test/foundry/FulfillAvailableAdvancedOrder.t.sol
@@ -996,7 +996,7 @@ contract FulfillAvailableAdvancedOrder is BaseOrderTest {
             100
         );
 
-        // Assert six-tenths of the offer has been fulfilled.
+        // Assert three-fifths of the offer has been fulfilled.
         {
             (
                 bool isValidated,
@@ -1006,9 +1006,9 @@ contract FulfillAvailableAdvancedOrder is BaseOrderTest {
             ) = context.consideration.getOrderStatus(orderHash);
             assertTrue(isValidated);
             assertFalse(isCancelled);
-            assertEq(totalFilled, 6);
+            assertEq(totalFilled, 3);
 
-            assertEq(totalSize, 10);
+            assertEq(totalSize, 5);
             assertEq(60, test1155_1.balanceOf(address(this), 1));
         }
     }
@@ -1093,7 +1093,7 @@ contract FulfillAvailableAdvancedOrder is BaseOrderTest {
             100
         );
 
-        // Assert six-tenths of the offer has been fulfilled.
+        // Assert three-fifths of the offer has been fulfilled.
         {
             (
                 bool isValidated,
@@ -1103,9 +1103,9 @@ contract FulfillAvailableAdvancedOrder is BaseOrderTest {
             ) = context.consideration.getOrderStatus(orderHash);
             assertTrue(isValidated);
             assertFalse(isCancelled);
-            assertEq(totalFilled, 6);
+            assertEq(totalFilled, 3);
 
-            assertEq(totalSize, 10);
+            assertEq(totalSize, 5);
             assertEq(60, test1155_1.balanceOf(address(this), 1));
         }
     }