A simplified implementation of function `_cbrt`

[barakman]

For the calculation of a cubic root, which is implemented in the math library of twocryptoswap and probably in a few other places, consider a simplified version which by and large performs the same number of operations (i.e., consumes the same amount of gas):

def _cbrt(n: uint256) -> uint256:
    if (n > 84):
        x: uint256 = n
        y: uint256 = 1 << unsafe_add(unsafe_div(_snekmate_log_2(n, False), 3), 1)
        while (x > y):
            x = y
            y = unsafe_div(unsafe_add(x << 1, unsafe_div(n, unsafe_mul(x, x))), 3)
        return x
    return 0x49249249249249246db6db6db6db6db6db6db6db6db692492492492492249248 >> unsafe_mul(n, 3) & 0x7

[bout3fiddy]

what is this black magic

edit: Vyper does not have while loops.

If you want to re-submite with tests, then please do go ahead!

i've made a small demo of this (but then converted to something that works). feel free to continue prototyping in the notebook:

https://colab.research.google.com/drive/1LCaPnKDkm6AELL9UPAM-CwgzUDXPgqTm?usp=sharing

[bout3fiddy]

ok I looked into it further and I think this one's not so useful for cryptoswap. we dont per se need cube root values for integers below 84. cryptoswap has an 18 precision so...

[barakman]

ok I looked into it further and I think this one's not so useful for cryptoswap. we dont per se need cube root values for integers below 84. cryptoswap has an 18 precision so...

The function implements a general-case handling for all values larger than 84.
It then adds a special-case handling for all values equal to or smaller than 84.

Your comment implies that you have somehow mistaken this function for handling only the latter.

It is overall a much more simplified solution compared with the one currently implemented in your codebase.
And that - a simplified version which by and large performs the same number of operations - is my primary point here.

[barakman]

what is this black magic

edit: Vyper does not have while loops.

If you want to re-submite with tests, then please do go ahead!

i've made a small demo of this (but then converted to something that works). feel free to continue prototyping in the notebook:

https://colab.research.google.com/drive/1LCaPnKDkm6AELL9UPAM-CwgzUDXPgqTm?usp=sharing

There's no black magic here (or you could say the same about the current implementation).
It simply encodes the first 85 output values (cubic roots of input values 0 thru 84) in a single integer.

I'm a little worried about your other comment though, where you seem to have interpreted the check of n > 84 as an indication that the function only handles input values up to 84. Were you perhaps a little hasty while reading it?
Please see my previous comment (above), which refers to that incorrect interpretation.

Regarding your comment about 'while' loops, you'd note that the same number of operations is conducted in the current implementation, just without an actual loop.

Hence I wrote in my original description:

A simplified version which by and large performs the same number of operations

[barakman]

cryptoswap has an 18 precision s

On top of everything that I've already mentioned above, the suggestion being made here is about function cbrt, not about cryptoswap in general.

And there is nothing in the code which indicates that this function is used only for amounts of the specific magnitude that you've mentioned. In fact, this function is being called in several different places, and it is quite likely that it takes input of various different magnitudes under different scenarios.

Your point would possibly make sense if the context was, for example, ERC20-related functions.
Here, the context is a low-level / general-purpose function.