Contribute Bancors Power Function

[tarrencev]

Contributes toward #819, addressed feedback in #827

🚀 Description

Adding Bancors Power function created by @barakman, which is necessary for the implementation of bonding curves, particularly logarithmic functions.

This PR extracts from logic from #827, contributed by @balasan, as per the feedback in that PR.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

Tagging reviewers of previous PR: @balasan @shrugs @oed @okwme @barakman @nventuro

[barakman]

@tarrencev:
You might want to incorporate BancorFormula's most recent version, which you can find here.

It provides an optimized version of log and an optimized version of exp, each of which reducing the gas consumption on a specific range of input.

One may configure these specific ranges in this file, and then regenerate the contract by running the scripts in this folder.

It is worth noting that the power function is neither designated nor used with base values smaller than 1.

Not designated, because (as documented in the code) instead of calculating base ^ exp, we calculate e ^ (log(base) * exp). And base < 1 implies log(base) < 0, which is of course very difficult (or even impossible) to handle in our "purely unsigned" implementation.

Not used, because each one of its callers (functions calculatePurchaseReturn, calculateSaleReturn and calculateCrossConnectorReturn, which implement the formulas from Bancor's white-paper), uses it with base values larger than 1.

Thanks

[tarrencev]

@barakman thank you for taking a look and providing feedback. I will migrate this over to the new version.

In that case, perhaps we should assert that _baseN >= _baseD in the power function?

[barakman]

@tarrencev:
Yes, of course.

The reason why I refrained from adding this assertion, is the fact that for the specific purposes of the BancorFormula contract, the power function is never used with _baseN < _baseD.

If you detach it from this contract into a standalone contract (or library), then such assert is definitely in place.

Side note:
It is more appropriate to use a require here, since it applies to user input, which may well be smaller than 1 (assert is for something that is not supposed to happen unless there's a bug in your code).

Thanks

[tarrencev]

Thanks again @barakman. I've merged the latest Bancor implementation of the power function, including the additional require as discussed.

Please let me know if you have any further feedback.

[barakman]

@tarrencev:

Well, for once, it might be worth adding all the relevant Python scripts used for auto-generating different parts of the code.

Each such file is mentioned in a comment above the relevant code snippet in the contract, and they are all located here (probably best to add the entire folder).

Additionally, you may as well replace assert(_baseN < MAX_NUM) with require, since (again) this applies for user input (I should probably fix it in Bancor's contract as well).

Thanks

[tarrencev]

@barakman: I've addressed the feedback re: assert.

As for adding the Python scripts, I am happy to do that if the maintainers are OK with introducing Python. I agree it makes sense to do so.

[barakman]

@tarrencev:

Another thing...

If memory serves correctly, the initial motivation for this thread derives from a different thread, aiming at adding Bonding Curve support in OpenZeppelin infrastructure, based on the two formulas in Bancor's white-paper.

I do not know whether or not these two threads are still related in any manner, but please note that those formulas are implemented in functions calculatePurchaseReturn and calculateSaleReturn, which you have excluded from the BancorFormula code pushed here.

Thanks

[barakman]

@tarrencev:

I've realized that changing the assert to require will break the BancorFormula.js test.

You can fix that in either one of the following ways:

1. There is one remaining assert which also needs to be replaced with require for the same reason as the previous one. Fix it, and then in BancorFormula.js, change ERROR_MESSAGE = 'invalid opcode' to ERROR_MESSAGE = 'revert'.
2. Revert your previous fix (assert to require), and wait for Bancor's next release, where I've applied all those fixes, but in a slightly cleaner manner (a better infrastructure for catching asserts and requires). This fix is currently available at branch 0.4.4 (which also contains some additional documentation of the exponentiation infrastructure).

If you're not relying on BancorFormula.js for testing, then you can just go ahead and replace the remaining assert with require.

Thanks

[barakman]

@tarrencev:

I've recently extended the documentation of the exponentiation infrastructure (more specifically, functions optimalLog and optimalExp, due to a question raised on GitHub).

Again, it is currently available at branch 0.4.4, which AFAIK is supposed to be released relatively soon.

As I mentioned before, this branch also includes the replacement of asserts into requires (including all necessary changes in JS test files).

Thanks

[nventuro]

Closing due to staleness.