Optimize gas for _getFraction in AmountDeriver

[Chomtana]

Motivation

_getFraction in AmountDeriver can reduce gas on storing exact flag and duplicated if (!exact) check.

I have submitted this gas optimization as a part of code4rena. Sadly, purposed solution written at code4rena report is wrong due to lack of time.

Now I have corrected the solution and open this pull request.

Solution

[](https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/AmountDeriver.sol#L101-L111

      // Ensure fraction can be applied to the value with no remainder. Note
      // that the denominator cannot be zero.
      bool exact;
      assembly {
          // Ensure new value contains no remainder via mulmod operator.
          // Credit to @hrkrshnn + @axic for proposing this optimal solution.
          exact := iszero(mulmod(value, numerator, denominator))
      }

      // Ensure that division gave a final result with no remainder.
      if (!exact) {
          revert InexactFraction();
      }

Can be simplified into

      assembly {
            // Ensure new value contains no remainder via mulmod operator.
            // Credit to @hrkrshnn + @axic for proposing this optimal solution.
            // Further optimization by @Chomtana and @0age
            if mulmod(value, numerator, denominator) {
                mstore(0, InexactFraction_error_signature)
                revert(0, InexactFraction_error_len)
            }
      }

Where InexactFraction_error_signature and InexactFraction_error_len is precomputed and stored as constant based on revert InexactFraction();

This reduce gas on storing exact flag and duplicated if (!exact) check)

I have already submit my name to contributor in #348

[0age]

What if mul(value, numerator) overflows?

[Chomtana]

Ah, my bad. So, revert to just optimize the mulmod check part.

[0age]

This can be optimized a little further as the gt(..., 0) is unnecessary; just use the mulmod as the condtional!

[axic]

Are you sure this does anything? The double iszero(iszero(...)) should be optimised out.

[Chomtana]

Just mulmod(value, numerator, denominator) should be worked as @0age said since we need to test whether mulmod(value, numerator, denominator) is not zero and it is unsigned which means mulmod(value, numerator, denominator) is truthy

[axic]

I mean if the change does anything gas wise. The double iszero should be removed by the optimiser.

[Chomtana]

It does by removing unnecessary exact variable.

exact := iszero(mulmod(value, numerator, denominator))
if iszero(exact) {
...
}

Don't sure whether optimiser can remove double iszero that is stored into intermediate variable first.

[axic]

There are many optimiser stages, including one which operates on opcode level, where known stack movement are supported.

[Chomtana]

I have just tested gas on remix using 2 separate contracts with same function name getFraction for both case and adding
a global variable assignment to the end

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.13;

error InexactFraction();

// error InexactFraction() @ AmountDerivationErrors.sol
uint256 constant InexactFraction_error_signature = (
    0xc63cf08900000000000000000000000000000000000000000000000000000000
);
uint256 constant InexactFraction_error_len = 0x20;

contract TestMulMod {
    uint256 public x;

    /**
     * @dev Internal pure function to return a fraction of a given value and to
     *      ensure the resultant value does not have any fractional component.
     *      Note that this function assumes that zero will never be supplied as
     *      the denominator parameter; invalid / undefined behavior will result
     *      should a denominator of zero be provided.
     *
     * @param numerator   A value indicating the portion of the order that
     *                    should be filled.
     * @param denominator A value indicating the total size of the order. Note
     *                    that this value cannot be equal to zero.
     * @param value       The value for which to compute the fraction.
     *
     * @return newValue The value after applying the fraction.
     */
    function getFraction(
        uint256 numerator,
        uint256 denominator,
        uint256 value
    ) public returns (uint256 newValue) {
        // Return value early in cases where the fraction resolves to 1.
        if (numerator == denominator) {
            return value;
        }

        // Ensure fraction can be applied to the value with no remainder. Note
        // that the denominator cannot be zero.
        assembly {
            // Ensure new value contains no remainder via mulmod operator.
            // Credit to @hrkrshnn + @axic for proposing this optimal solution.
            // Further optimization by @Chomtana and @0age
            if mulmod(value, numerator, denominator) {
                mstore(0, InexactFraction_error_signature)
                revert(0, InexactFraction_error_len)
            }
        }

        // Multiply the numerator by the value and ensure no overflow occurs.
        uint256 valueTimesNumerator = value * numerator;

        // Divide and check for remainder. Note that denominator cannot be zero.
        assembly {
            // Perform division without zero check.
            newValue := div(valueTimesNumerator, denominator)
        }

        x = newValue;
    }

    /**
     * @dev Internal pure function to return a fraction of a given value and to
     *      ensure the resultant value does not have any fractional component.
     *      Note that this function assumes that zero will never be supplied as
     *      the denominator parameter; invalid / undefined behavior will result
     *      should a denominator of zero be provided.
     *
     * @param numerator   A value indicating the portion of the order that
     *                    should be filled.
     * @param denominator A value indicating the total size of the order. Note
     *                    that this value cannot be equal to zero.
     * @param value       The value for which to compute the fraction.
     *
     * @return newValue The value after applying the fraction.
     */
    // function getFraction(
    //     uint256 numerator,
    //     uint256 denominator,
    //     uint256 value
    // ) public returns (uint256 newValue) {
    //     // Return value early in cases where the fraction resolves to 1.
    //     if (numerator == denominator) {
    //         return value;
    //     }

    //     // Ensure fraction can be applied to the value with no remainder. Note
    //     // that the denominator cannot be zero.
    //     bool exact;
    //     assembly {
    //         // Ensure new value contains no remainder via mulmod operator.
    //         // Credit to @hrkrshnn + @axic for proposing this optimal solution.
    //         exact := iszero(mulmod(value, numerator, denominator))
    //     }

    //     // Ensure that division gave a final result with no remainder.
    //     if (!exact) {
    //         revert InexactFraction();
    //     }

    //     // Multiply the numerator by the value and ensure no overflow occurs.
    //     uint256 valueTimesNumerator = value * numerator;

    //     // Divide and check for remainder. Note that denominator cannot be zero.
    //     assembly {
    //         // Perform division without zero check.
    //         newValue := div(valueTimesNumerator, denominator)
    //     }

    //     x = newValue;
    // }
}

Normal case (2, 5, 10)
Before: 24189 gas
After: 24176 gas

Revert case (2, 5, 8)
Before: 21845 gas
After: 21804 gas

[axic]

I think these should have the error signature as a comment, i.e. error InexactFraction().

Also a reference to the definition AmountDerivationErrors.sol may be useful. Note: do not remove the Solidity definition otherwise it will not be included in the output JSONs.

[Chomtana]

More comments has been added

[0age]

Let's just put the constants in the ConsiderationConstants.sol file; otherwise LGTM

[hrkrshnn]

Why use 0.8.7 here?

[Chomtana]

It is copied from ConsiderationConstants.sol let ask @0age should we change to >=0.8.13 or are there any specific cases? In my opinion, it should be >=0.8.13.

[Chomtana]

There are too many >=0.8.7 so, separate PR should be better.

[Chomtana]

#416

[Chomtana]

Let's just put the constants in the ConsiderationConstants.sol file; otherwise LGTM

I have already moved constants to ConsiderationConstants

[0age]

you can remove the credit here :)

[0age]

Appears as though the gas savings in the success case are likely dependent on optimization settings (though given Seaport is using viaIR and 0.8.14 I imagine they're being applied) but the revert case is much faster and it does cut down on code. Merging, but might consider adding some additional documentation on what states zero and non-zero mulmod results indicate.