Feature/bonding curve

[balasan]

Fixes #819

🚀 Description

Adding a simple bonding curve contract (aka automatic market maker) based on Bancor formula. The codes is based on and includes code by Bancor: https://github.com/bancorprotocol/contracts.

I have extracted the power function from the original BancorFormula.sol contract into a separate Power.sol contract in the math folder. This function depends on some pre-generated constants and is beyond my understanding. Although there is a test for it, including it in the library does depend on trusting the Bancor code to some degree and might be hard to audit. I can see this being a reason to not merge this PR, but differing the decision to the Zeppelin team.

I have also pushed the code to a separate repo: https://github.com/relevant-community/bonding-curve so in the case it is not a good fit for Zeppelin we can maintain it there.

Some background reading about bonding curves:

https://hackernoon.com/how-to-make-bonding-curves-for-continuous-token-models-3784653f8b17
https://medium.com/@simondlr/tokens-2-0-curved-token-bonding-in-curation-markets-1764a2e0bee5
and Bancor formula:
https://drive.google.com/file/d/0B3HPNP-GDn7aRkVaV3dkVl9NS2M/view

• 📘 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).

[shrugs]

@balasan

there are a few linting errors on Power.sol, primarily along the usage of the [. run npm run lint:sol to see all of them or check the output at https://travis-ci.org/OpenZeppelin/zeppelin-solidity/jobs/356012100

The error was caused by a ganache error, restarting the job now

[shrugs]

Ignore my comments on BancorFormula.sol and Power.sol if they're supposed to be changed as little as possible from the original bancor contracts

[shrugs]

@dev prefixes

[shrugs]

{ } to match codebase style

[shrugs]

likewise { } here

[shrugs]

{ } here

[shrugs]

here

[shrugs]

here

[shrugs]

is there a better name for temp1 and temp2?

[shrugs]

documentation for this variable?

[shrugs]

Can you use the

/**
 * asfsd
 * asdf
 */  

style for these block comments as we do elsewhere?

[shrugs]

they should probably also be prefixed with @dev

[balasan]

seems like npm run lint:sol:fix edited a few other files hope thats fine...

[shrugs]

LGTM, but I'd like an expert in BondingCurve theory to take a peek as well. Can Billy come review this as well? cc @spalladino as well, since I know you did some research into TCRs for zeppelin_os

[okwme]

i'd sudgest to:

• Make BondingCurveMock.sol constructor payable so that the initial _poolBalance can be filled without triggering the fallback buy() function
• Make BondingCurveMock.sol constructor assign the totalSupply_ to an address so that they actually exist
• truffle-config.js needs to use a gasPrice that matches the gasPrice in BondingCurve.test.js (might only be relevant to relevant-community/bonding-curve)

[balasan]

Thanks @okwme commited all of those updates
One new issue I found is that the contract becomes stuck if everyone sells all tokens and totalSupply & poolBalance are 0. This is because BancorFormula does not work when either of those values is 0.
Issue + proposed solutions opened here: relevant-community/bonding-curve#2

[oed]

@balasan Cool stuff! Need to dive deeper into the code :D
Q: do you have any plans for supporting other tokens than just ether as the bonding token?

[balasan]

yeah, should be fairly easy to do this

[barakman]

@balasan:
BancorFormula writer here (the contract, not the formula).

With regards to "This function depends on some pre-generated constants and is beyond my understanding" - I can explain into details if you're interested.
In any case, each set of constants can be generated by a script in Python (as documented in the contract).
All of these constants are generally dependent on two constants only - MAX_PRECISION (set to 127) and the number of coefficients in the exponentiation function (set to 34).
Anyone using this code may choose to play around with those two constants, which would ultimately impact accuracy and performance (which are "tightly coupled" in a typical "trade-off relationship").
Initially, the code was implemented with a fixed PRECISION of 32. With this level of precision, it was imperative to use 34 coefficients in order to ensure reasonable accuracy. However, after introducing dynamic precision (which can go as high as 127), the number of coefficients becomes much less critical for the sake of accuracy. For a given input, using a smaller number of coefficients would allow a higher value of precision passed to the exponentiation function (thus keeping the level of accuracy while improving the performance and reducing the overall gas cost).

BTW, this contract has since been optimized (for performance mostly), and you can find it at https://github.com/bancorprotocol/contracts/tree/0.3.18 (though the implementation is slightly more cumbersome, so I doubt you would want to incorporate it into Zeppelin).

With regards to testing - there are web3 tests in both Python in Javascript.
But while the JS test is primarily for sanity assertion, the Python testing infrastructure provides a much wider coverage (regression if you will). I can go into details here too if needed.

With regards to "the contract becomes stuck if everyone sells all tokens and totalSupply & poolBalance are 0" - the same thing would happen if everyone buys until supply/balance are MAX_NUM (2 ^ 129) or higher. I'm not so sure that this is a plausible scenario however (nor is the scenario that you've described), but that's probably a product-related decision.

Thanks

[balasan]

hey @barakman! thanks for all the additional info
the power function is very cool
need to spend some more time looking at the code to understand how all the actual computations are made, but high-level makes sense

[nventuro]

Hello @balasan, and thank you for your interest and contribution! I'm sorry so much time has passed with no response from us.

I really want for Bonding Curves to be added to OZ and want to get this merged, but worry that the PR as it is has become too large (there's the Bonding Curve itself, the Bancor formula, the Power library, tests, etc.).

Would you be willing to split this into smaller, self-contained PRs so that we can more rapidly review changes and merge? Thanks!

[balasan]

@nventuro I think it would make sense to break out the Power function into a separate PR. Bonding Curve and Bancor formula should probably go together — does that work?

[nventuro]

@balasan since the Bancor formula is its own contract, I think it makes sense for there for be a PR introducing it, with tests, etc., and then build on top of it (same with the power functions).

[nventuro]

Closing due to staleness.