Solidity - multiplying tokens with price

Question

I'm writing a simple exchange script. It contains my token with 18 decimal places and ether (which is very similar). Users can add buy / sell offerts and the script displays them.

I can send the addBuyOffert transaction there which has two arguments: tokensAmount and priceForOneUnit (one unit means equivalent for 1 ether). On the ending of the BUY function it multiplies tokens by price and removes sender's wei equivalent. When user cancels the offert, he gets wei back.

When I was testing the script on microvalues without any decimal places it was working great. For example I called buy offert with 10 tokens, 10 wei and it removed from my account 100 wei (10 tokens * 10 wei) and the offert was added without any problems. The problem appeared after adding decimals and working with greater numbers. The final price was for not 1 token but for 1000000000000000000. For example when I put 0.02 coin and 0.01 eth to add buy offert, it sends 20000000000000000, 10000000000000000 and tries to multiply it which fails with SafeMath. Also tried with div with 1 ether and it also didn't give me precise results.

Knowing this I changed my code a bit. This is what I did so far, but I keep in mind that this isn't the best solution, because the first "if" gives me wrong result - tried with 1.1, 1.1, expected 1.21, but got 1.1 of eth to remove.

uint eth = 1 ether;
if(tokensAmount > eth)
{
    finalPrice = pBuyPrice.mul(tokensAmount.div(eth));
}
else if(tokensAmount < eth)
{
    finalPrice = pBuyPrice.div(eth.div(tokensAmount));
}
else
{
    finalPrice = pBuyPrice;
}

How can I make this calculation working properly?

Answer 1

1. For best accuracy, always multiply before you divide.

So change this:

finalPrice = pBuyPrice.mul(tokensAmount.div(eth));

To this:

finalPrice = pBuyPrice.mul(tokensAmount).div(eth);

2. For best accuracy, always avoid division where possible.

So change this:

finalPrice = pBuyPrice.div(eth.div(tokensAmount));

To this:

finalPrice = pBuyPrice.mul(tokensAmount).div(eth);

You may have noticed by now, that the first two cases in your logic are identical!

So you may as well change your entire method to this:

finalPrice = pBuyPrice.mul(tokensAmount).div(ether);

This calculates the price in Ether, but you might want to consider returning the price in Wei instead, and let the user handle it outside the function:

finalPrice = pBuyPrice.mul(tokensAmount);

This will ensure that there is no loss of information whatsoever.