OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.76k stars 11.76k forks source link

Add `signum` Function to `SignedMath` #4779

Open pcaversaccio opened 9 months ago

pcaversaccio commented 9 months ago

I was wondering, whether OZ should offer a proper signum (sign might be too confusing due to cryptographic functions) function that returns the indication (-1, 0, or 1) of the sign of a 32-byte signed integer. Thoughts?

Example Implementation

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

library SignedMath {
    /**
     * @dev Returns the indication of the sign of a 32-byte signed integer.
     * @notice The function returns `-1` if `x < 0`, `0` if `x == 0`, and `1` if `x > 0`.
     * For more details on finding the sign of a signed integer, please refer to:
     * https://graphics.stanford.edu/~seander/bithacks.html#CopyIntegerSign.
     */
    function signum(int256 a) internal pure returns (int256 b) {
        /// @solidity memory-safe-assembly
        assembly {
            b := sub(sgt(a, 0), slt(a, 0))
        }
    }
}
Tigersame commented 9 months ago

nice

Amxx commented 9 months ago

@pcaversaccio What are the use cases for that function? Why return a int256 and not an Enum ?

pcaversaccio commented 9 months ago

@pcaversaccio What are the use cases for that function? Why return a int256 and not an Enum ?

I remember some Curve code that ensures that a partial derivative must be negative and thereafter multiplies it by the sign (-1) to make it again a positive number since they needed the abs value. About the int256 vs. Enum: good question, first, I thought to follow the conventions from other programming languages / frameworks that use the -1, 0, 1 (not all define 0 as a separate return) (see e.g. numpy.sign). Second, an Enum is an uint8 under the hood and if you're interested in the sign, you will work on the int domain usually. So it naturally translates into the same type domain.

Amxx commented 9 months ago

I remember some Curve code that ensures that a partial derivative must be negative and thereafter multiplies it by the sign (-1) to make it again a positive number since they needed the abs value.

I'm not sure signum really helps with that. You can check that the value is negative by directly comparing it 0 ... and if that passes you negate it.

I don't have any stong opinion against that proposal, but I also want to make sure we don't add code would end up unused, and that would affect the discoverability of the other code.

Amxx commented 9 months ago

I can't find this helper in PRBMath. I would expect people to need that for the actual math that PRBMath addresses before they need that for the "basic" math we provide.

pcaversaccio commented 9 months ago

I can't find this helper in PRBMath. I would expect people to need that for the actual math that PRBMath addresses before they need that for the "basic" math we provide.

That makes perfect sense to me, that's why I opened an issue for discussion first (instead of directly doing a PR). Let's see if it's added to PRBMath first.

SteMak commented 8 months ago

I remember some Curve code that ensures that a partial derivative must be negative and thereafter multiplies it by the sign (-1) to make it again a positive number since they needed the abs value.

Generally, the signum function should not be used to get an absolute positive or negative value, the abs function exists for the purpose.

The signum may be needed in the case your equation depends only on a sign of the variable, not the variable itself. The only signum usecase I remember is sin(θ) = sign(cos(θ − π/2)) * sqrt(1 - cos(θ)^2), here it is used to not deal with an unknown sign during equation transformations.