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 sqrt for math #3242

Merged
merged 21 commits into from Jun 7, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -17,6 +17,7 @@
* `ERC721`, `ERC1155`: simplified revert reasons. ([#3254](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3254), ([#3438](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3438)))
* `ERC721`: removed redundant require statement. ([#3434](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3434))
* `PaymentSplitter`: add `releasable` getters. ([#3350](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3350))
* `Math`: Add a `sqrt` function. ([#3242](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3242))
Amxx marked this conversation as resolved.
Show resolved Hide resolved

## 4.6.0 (2022-04-26)

Expand Down
4 changes: 4 additions & 0 deletions contracts/mocks/MathMock.sol
Expand Up @@ -29,4 +29,8 @@ contract MathMock {
) public pure returns (uint256) {
return Math.mulDiv(a, b, denominator, direction);
}

function sqrt(uint256 a) public pure returns (uint256) {
return Math.sqrt(a);
}
}
58 changes: 58 additions & 0 deletions contracts/utils/math/Math.sol
Expand Up @@ -149,4 +149,62 @@ library Math {
}
return result;
}

/**
* @dev Returns the square root of a number.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only the case for perfect squares, we should document the behavior for the other numbers. I suppose it rounds down.

Does it make sense (is it possible?) to provide a version with a Rounding argument?

Do you think we can document some aspect of the efficiency of this function? It's O(1) but can we say anything stronger in terms of the constants involved?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a version with Rounding

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could provide an upper bound on the gas cost, I'm not sure what else we can say

*
* Inspired by Henry S. Warren, Jr.'s "Hacker's Delight" (Chapter 11).
*/
function sqrt(uint256 a) internal pure returns (uint256) {
if (a == 0) {
return 0;
}

// For our first guess, we use the log2 of the square root. We do that by shifting `a` and only counting half
// of the bits. the partial result produced is a power of two that verifies `result <= sqrt(a) < 2 * result`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is clear or accurate.

For our first guess, we use the log2 of the square root.

What does it mean that we "use the log2"? I read this like saying that our guess is log2(sqrt(a)), but I don't think this is true.

I also find the part about shifting a confusing.

uint256 result = 1;
uint256 x = a;
if (x > 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write this like this, otherwise we basically need to count Fs to know if the code is right.

        if (x > (1 << 128) - 1) {

The compiler seems to optimize this expression to a constant, even when optimizations are disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then what about doing x >= 1 << 128. Would that also get optimized? to the same bytecode. Because there is no gte opcode I'm worried it would be compiled to not(lt(..., ...))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found an even better rewording.

x >>= 128;
result <<= 64;
}
if (x > 0xFFFFFFFFFFFFFFFF) {
x >>= 64;
result <<= 32;
}
if (x > 0xFFFFFFFF) {
x >>= 32;
result <<= 16;
}
if (x > 0xFFFF) {
x >>= 16;
result <<= 8;
}
if (x > 0xFF) {
x >>= 8;
result <<= 4;
}
if (x > 0xF) {
x >>= 4;
result <<= 2;
}
if (x > 0x3) {
result <<= 1;
}

// At this point `result` is an estimation with one bit of precision. We know the true value is a uint128,
// since it is the square root of a uint256. Newton's method converges quadratically (precision doubles at
// every iteration). We thus need at most 7 iteration to turn our partial result with one bit of precision
// into the expected uint128 result.
unchecked {
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
return min(result, a / result);
}
}
}
16 changes: 16 additions & 0 deletions test/utils/math/Math.test.js
Expand Up @@ -182,4 +182,20 @@ contract('Math', function (accounts) {
});
});
});

describe('sqrt', function () {
it('rounds down', async function () {
expect(await this.math.sqrt(new BN('0'))).to.be.bignumber.equal('0');
expect(await this.math.sqrt(new BN('1'))).to.be.bignumber.equal('1');
expect(await this.math.sqrt(new BN('2'))).to.be.bignumber.equal('1');
expect(await this.math.sqrt(new BN('3'))).to.be.bignumber.equal('1');
expect(await this.math.sqrt(new BN('4'))).to.be.bignumber.equal('2');
expect(await this.math.sqrt(new BN('144'))).to.be.bignumber.equal('12');
expect(await this.math.sqrt(new BN('1000000'))).to.be.bignumber.equal('1000');
expect(await this.math.sqrt(new BN('1000001'))).to.be.bignumber.equal('1000');
expect(await this.math.sqrt(new BN('1002000'))).to.be.bignumber.equal('1000');
expect(await this.math.sqrt(new BN('1002001'))).to.be.bignumber.equal('1001');
expect(await this.math.sqrt(MAX_UINT256)).to.be.bignumber.equal('340282366920938463463374607431768211455');
});
});
});