Payroll: clarify how rates are handled and add unit tests

[facuspagnuolo]

Adding unit tests to ensure how rates handling in payroll (cc #821). I also rolled back the last change we did to the PPF request to confirm it was working properly.

[coveralls]

Coverage remained the same at 97.015% when pulling 5a6570a on payroll_rates_handling_tests into 5f190c5 on master.

[sohkai]

🙇

[facuspagnuolo]

@izqui @sohkai as you can see in a9f3a07, I changed transfer amount formula we were using in the contract. I think the previous one was wrong, and that's why using an inverse rate was working fine. The amount to be transferred must be in the allowed token units [AT], the owed amount is in denomination token units [DT], and the rate is fetched using the denomination token as the quote which is in [DT/AT], then we should divide the owed amount by the rate instead of multiplying it.

Besides that, I did some more tweaks to change all the test suite to use a real denomination token (USD) with real salaries amount.

[sohkai]

I think the previous one was wrong, and that's why using an inverse rate was working fine. The amount to be transferred must be in the allowed token units [AT], the owed amount is in denomination token units [DT], and the rate is fetched using the denomination token as the quote which is in [DT/AT], then we should divide the owed amount by the rate instead of multiplying it.

Hmm, I think this might've been due to the original change being reverted in 4a0f4b3#diff-9d7adaffbcce19867f027a983a30bcc7R668. This change to _getExchangeRate() basically does the division for us, in the price feed (if the price feed's base token isn't the denomination token).

I think I still prefer _totalAmount.mul(exchangeRate).mul(tokenAllocation) (and its associated change in _getExchangeRate()) for clarity. Depending on the price feed, it can also be a little bit more accurate, since the truncations all happen at the end (if the base token in the price feed is the denomination token).

---

To illustrate, previously (with the change to _getExchangeRate()) we had:

owedAmount [DT] * rate [AT/DT] / (ONE) * (allocation / 100) == payment [AT]

Now we have:

owedAmount [DT] / rate [DT/AT] * (ONE) * (allocation / 100) == payment [AT]

It's subtle, but the first case with multiplication is a lot more clear to me.

[facuspagnuolo]

It's subtle, but the first case with multiplication is a lot more clear to me.

It is and it was to me, but I thought we were also really sure that the PPF was always being used using the denomination token as quote.