-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Adding RoundedDivMath (with bankers' rounding) to contracts/math #1712
Conversation
Hello @AnthonyAkentiev, thank you for your contribution! I had not heard of bankers' rounding, this is quite interesting. What if instead of creating a new library we added these functions to |
Maybe |
@nventuro that's a good idea! ok, let me refactor everything and i will commit to this PR. I'll add functions to the Thx |
Thanks for contributing @AnthonyAkentiev! I'm having some trouble trying to understand where it is appropriate to use banker's rounding. I considered potential use in In real life with real money this is probably acceptable because it's very little money and you can afford it, but the blockchain and smart contracts are stricter than that, and a contract like Would you mind sharing some thoughts on this issue? Would it be possible to write guidelines for developers on when it does or doesn't make sense to use banker's rounding? |
@frangio I have described why banker's rounding should be used here - https://hackernoon.com/bankers-rounding-for-smart-contracts-2bccd0b664 "...but I think it can run into a situation where the total owed is more than the actual balance that is available to pay out (say, if the amount is 7 wei, split 50-50)" - yes, that's why you should use EXPLICIT and well-understood rounding. So your client will know that 3.5 is ALWAYS rounded to 3. And so you owe him 3, but not 3.5 or 4. In case of "rounding were not discussed at all" 3.5 is either 3 or 4 :-) So your client might "sue" you because he thinks that you owe him 4, but you think you owe him 3. Banker's rounding reduce accumulation of error on one side. So the client will be more happy than when you say "we always round down (owe you a bit less)". In our case we need bankers rounding also because our math on the frontend uses it. So we don't want our numbers to be different after 10000 payouts. |
I can see this being a problem with traditional finance, but in the blockchain there is necessarily explicit rounding defined in the smart contract, which the client would have agreed to. Regarding the problem of bias, if there are many operations to accumulate error I think a better solution is to accumulate the remainder until it's divisible, like what we do in Again, the main problem I see with this approach is the ease with which it could be exploited, due to blockchain data being publicly available. As a general building block to include in OpenZeppelin, I'm not sold on banker's rounding. I think |
@frangio i didn't get how "traditional finance" is different from what we have in the blockchain :-) for me it's the same math. And i thought that if "blockchain world" lacks that -> that would be great to have it. It's an opt-in, not a "requirement to use". In my opinion this line is a good example why EXPLICIT rounding should be used (not necessary bankers' rounding) But still, i can understand you, because you don't want OpenZeppelin to grow into big chunk of code. |
Adding rounded div to contracts/math. The idea is described by me here - https://hackernoon.com/bankers-rounding-for-smart-contracts-2bccd0b664
Please review the PR.
I can fix or improve the code if needed. There are many different ways how this can be implemented and used. I am really glad that i can help the whole community!
OpenZeppelin rules!