Skip to content
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

WIP: Fix transfer dividendability #218

Closed
wants to merge 6 commits into from
Closed

Conversation

uivlis
Copy link
Contributor

@uivlis uivlis commented Feb 21, 2020

Fixes #215.

Alright, I've been banging my head with this and still can't figure it out.
The first test fails by an order of just 10.
The second reverts.
Also, had to add an .abs() somewhere in the differential because there was a subtraction overflow, not sure if the formula thus preserves its validity.
Begging for help...

But still, I am not convinced we need this. Dividends are disbursed to holders based on the balances at the time of dividend incoming, not at the time of claiming, aren't they (in which case the transfer bug is a bug no more)? If I have 50% of a company, and I disburse dividends annually, say I make in February 100$, and in December I quit from the board. At the end of the year, shouldn't I get those 50$ that is rightfully mine? If we solve this bug, the answer would be a staggering no.

@alcueca
Copy link
Contributor

alcueca commented Feb 21, 2020

The abs() certainly is going to make the formula invalid. Probably that issue is one of decimals. Maybe a number of being converted to fixed point twice.

A test failing by a factor of 10 will be either an issue with my calculations, or one of the test constants off by a factor of 10.

I've got a few things today, but I'll have a look later on. Thanks a lot for your effort with this.

@alcueca
Copy link
Contributor

alcueca commented Feb 21, 2020

Ok, I had a look. It all probably down to not knowing exactly what is a fixed point number and what isn't. The verbosity of Fixidity doesn't help.

When you get a function with wei quantities like releaseDividends(uint256 amount), that amount parameter is a fixed point number, but in a different format than fixidity.

A token that implements ERC20Detailed has it's own fixed point math system fueled by decimals(). We need to be aware of that when converting them to Fixidity.

When you call .newFixed() Fixidity will just add 24 zeroes at the end of the number. With a wei amount that is unnecessary and will most likely lead to overflows. If a token already has 18 decimals, it is in fixed point. We just need to add 6 zeroes to convert the number to the 24 decimal system of Fixidity.

Let's leave this branch here. We need to fix a few things in the previous PR. It worked quite by accident 😮

@alcueca
Copy link
Contributor

alcueca commented Feb 25, 2020

See if after merging #224 we can work on this one again.

When using DecimalMath.sol remember that wei amounts already are decimal numbers and don't need any conversion.

@uivlis
Copy link
Contributor Author

uivlis commented Feb 26, 2020

Thanks, @albertocuestacanada, DecimalMath does prove a real simplifier. Still, I have my two unanswered comments. First,

Also, I had to add a .abs() somewhere in the differential because there was a subtraction overflow, not sure if the formula thus preserves its validity.

And also,

But still, I am not convinced we need this. Dividends are disbursed to holders based on the balances at the time of dividend incoming, not at the time of claiming, aren't they (in which case the transfer bug is a bug no more)? If I have 50% of a company, and I disburse dividends annually, say I make in February 100$, and in December I quit from the board. At the end of the year, shouldn't I get those 50$ that is rightfully mine? If we solve this bug, the answer would be a staggering no.

@alcueca
Copy link
Contributor

alcueca commented Feb 26, 2020

I had a look at the code, and this will be cleaner to fix once we migrate to solidity 0.6 and the ERC20 hooks that openzeppelin has implemented become available.

The .abs() can't be there, I'll explain the formula later on this week. I'll also reply to the comment on whether we need this at all then.

@alcueca alcueca closed this Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ERC20DividendableEth for transfers.
2 participants