feat: add isqrt built-in #3069
feat: add isqrt built-in #3069
Conversation
vyper/builtin_functions/functions.py
Outdated
| "sqrt": Sqrt(), | ||
| "sqrt_solmate": SqrtSolmate(), |
There was a problem hiding this comment.
why not update the original Sqrt instead?
There was a problem hiding this comment.
That was the original idea indeed, but did not want to introduce that breaking change without prompt from a core contributor.
I'll make the changes to the original sqrt then.
There was a problem hiding this comment.
i don't think we should get rid of the original sqrt. sqrt on decimals is pretty useful, especially once we open up the decimals type to be generic.
There was a problem hiding this comment.
Yeah, but sounds like this should basically expand the types that sqrt works with, so if it's decimals it uses the previous and if it's integers it uses this one
There was a problem hiding this comment.
i think if it's possible, use the same algorithm for both types. there will be fewer gas surprises that way
There was a problem hiding this comment.
@charles-cooper It doesn't look possible since exponentiation is not allowed for decimal type. I do like the idea @fubuloubu mentioned, where sqrt(x: decimal) would just yield what it does right now and sqrt(x: uint256) would use the optimised version.
Regarding gas surprises: is that really an issue? I don't expect users to use the decimal type at all.
There was a problem hiding this comment.
discussed offline -- the gas difference is ok. @bout3fiddy added a note that we will try to reify the code paths later
Codecov Report
@@ Coverage Diff @@
## master #3069 +/- ##
==========================================
+ Coverage 88.32% 88.34% +0.02%
==========================================
Files 97 97
Lines 10980 10999 +19
Branches 2597 2599 +2
==========================================
+ Hits 9698 9717 +19
Misses 832 832
Partials 450 450
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bout3fiddy
changed the title
|
looks like Lines 40 to 44 in e7733e1 |
charles-cooper
changed the title
…e, fix: remove a few unnecessary tests; test_sqrt_valid_range borks for a few select test cases, investigating ...
docs/built-in-functions.rst
Outdated
| @@ -664,6 +664,25 @@ Math | |||
| >>> ExampleContract.foo(9.0) | |||
| 3.0 | |||
|
|
|||
| .. py:function:: isqrt(x: uint256) -> uint256 | |||
|
|
|||
| Return the square root of the provided integer number, using Babylonian square root algorithm. | |||
There was a problem hiding this comment.
| Return the square root of the provided integer number, using Babylonian square root algorithm. | |
| Return the (integer) square root of the provided integer number, using the Babylonian square root algorithm. The rounding mode is to round down to the nearest integer. For instance, ``isqrt(101) == 10``. |
…t: add clarification to rounding step in builtin code
no more need to backport, 3.7 tests were dropped in #3071 |
What I did
Inspired from [t11s's solmate fixed point sqrt utility algo].(https://github.com/transmissions11/solmate/blob/9cf1428245074e39090dceacb0c28b1f684f584c/src/utils/FixedPointMathLib.sol#L166)
Michael from Curve made a version of this in vyper. I added it as a builtin and changed a few tests.
How I did it
Take code from Michael, amend a few things, write tests
How to verify it
pytest tests/parser/types/numbers/test_sqrt_solmate.pyCommit message
Adds
isqrtbuiltin. For a better explanation of the algorithm, refer to: https://github.com/Gaussian-Process/solmate/blob/837de01395312eb89e607fdc64fb7bb9c03207c3/src/utils/FixedPointMathLib.solDescription for the changelog
Cute Animal Picture
rottweiler puppy