Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SignedMath with math utilities for signed integers #2686

Merged
merged 16 commits into from
Jan 12, 2022

Conversation

rotcivegaf
Copy link
Contributor

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Jul 6, 2021

Hello @rotcivegaf

I'm sorry we haven't reached out yet. This PR resurfaced as we are planning for the next release and we would love to merge it. Still some small issues must be addressed:

  • In average, the function can overflow. If bot values are type(int256).max, then the function will revert despite the average fitting in an int256. I encourage you to have a look at Math.average to see how this can be addressed.

  • In ceilDiv, you are using a int8 in a place where its not necessary. I would rather use int256 to avoid unnecessary implicit cast.

Can you have a go at this ?

@rotcivegaf
Copy link
Contributor Author

rotcivegaf commented Jul 9, 2021

Sorry for the delay, I was very busy

In the second point, when I remove the cast, I get this error from the compiler:
TypeError: Type uint8 is not implicitly convertible to expected type int256.

The other option I thought of is to replace that line with:

int256 r;
if(a % b != 0) r = 1;

I work in the first point, in this days

@Amxx
Copy link
Collaborator

Amxx commented Jul 9, 2021

Noted.

I think this point is less controversial, as it should not affect the produce bytecode
I think I would personally do

        int256 r = a % b == 0 ? int256(0) : int256(1);

@rotcivegaf
Copy link
Contributor Author

rotcivegaf commented Jul 9, 2021

In the ceilDiv signed average function, I think your idea its good, I commited

In the average signed average function, I try do with this way:

    function average(int256 a, int256 b) internal pure returns (int256) {
          return (a / 2) + (b / 2) + ((a % 2 + b % 2) / 2);
    }

But when calculate with an even signed number and an odd number, the return number is one(1) integer greater than the expect, this cause because this part of the equation (a % 2 + b % 2), I think the problem is because we have the number 0

I worked in this ugly solution, but I still thinking in another better

    function average(int256 a, int256 b) internal pure returns (int256) {
        if ((b < 0 && a > 0 && -b < a) || (a < 0 && b > 0 && -a < b))
            return (a / 2) + (b / 2) + (((a % 2 + b % 2) - 1) / 2);

        if ((b < 0 && a > 0 && -b > a) || (a < 0 && b > 0 && -a > b))
            return (a / 2) + (b / 2) + (((a % 2 + b % 2) + 1) / 2);

        return (a / 2) + (b / 2) + ((a % 2 + b % 2) / 2);
    }

@frangio
Copy link
Contributor

frangio commented Jul 9, 2021

This formula on StackOverflow looks really neat:

int avg = (a & b) + (a ^ b) / 2;

@Amxx
Copy link
Collaborator

Amxx commented Jul 9, 2021

This formula on StackOverflow looks really neat:

int avg = (a & b) + (a ^ b) / 2;

should we use that for Unsigned Math ? I guess it would be more gas efficient

@rotcivegaf
Copy link
Contributor Author

rotcivegaf commented Jul 9, 2021

Well with unsigned values its more complicated we have the number 0, and this a problem
This implementation int avg = (a & b) + (a ^ b) / 2; dont pass this tests:

  • one even signed number and one odd signed number
  • one even signed number and one odd number

But I use this idea in Math.sol to make another PR

EDIT: I update the code, the average function its ugly but works, I will research more to improve it. Any help are welcome

@Amxx Amxx mentioned this pull request Aug 24, 2021
1 task
@frangio
Copy link
Contributor

frangio commented Sep 2, 2021

We want a nicer definition of div in order to merge this. We think it should be possible to find one by tweaking the proof for unsigned div formula:
image

@frangio
Copy link
Contributor

frangio commented Oct 6, 2021

I keep trying to adapt the formula in an elegant way but I can't find one.

The issue is that the formula seems to round towards negative infinity and we want it to round to zero.

@Amxx Amxx mentioned this pull request Oct 18, 2021
@Amxx Amxx dismissed a stale review via 234cfb8 January 6, 2022 17:13
@Amxx
Copy link
Collaborator

Amxx commented Jan 6, 2022

I've refactored the average code.

We still need a changelog entry and some documentation. I'll work on that tomorrow

test/utils/math/SignedMath.test.js Outdated Show resolved Hide resolved
test/utils/math/SignedMath.test.js Outdated Show resolved Hide resolved
contracts/utils/math/SignedMath.sol Outdated Show resolved Hide resolved
Amxx and others added 2 commits January 7, 2022 15:26
@Amxx Amxx dismissed a stale review via 9241143 January 10, 2022 10:41
@frangio frangio changed the title Math for signed integers Add SignedMath with math utilities for signed integers Jan 12, 2022
@frangio
Copy link
Contributor

frangio commented Jan 12, 2022

@Amxx I've removed ceilDiv since it's not proper "ceiling". Let's add it back if someone requests something like it.

Please merge if it looks good.

@Amxx Amxx merged commit 3458c1e into OpenZeppelin:master Jan 12, 2022
@rotcivegaf
Copy link
Contributor Author

rotcivegaf commented Jan 12, 2022

I'm AFK, but I want to congratulate them
Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants