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

Payroll: revert to old way of obtaining exchange rate and add more clarity #821

Merged
merged 7 commits into from
Apr 26, 2019

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Apr 25, 2019

Reverts to the old way of calculating the payment amount and tries to make it more obvious.

@sohkai sohkai changed the base branch from master to payroll_rates_handling_tests April 26, 2019 12:15
@sohkai sohkai marked this pull request as ready for review April 26, 2019 13:55
// and applied allocation percentage
uint256 tokenAmount = _totalAmount.mul(exchangeRate).mul(tokenAllocation);
// Divide by 100 for the allocation and by ONE for the exchange rate precision
tokenAmount = tokenAmount / (100 * ONE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding all these comments :)

@@ -12,6 +12,11 @@ module.exports = (artifacts, web3) => {
const DAI_RATE = formatRate(1) // 1 DAI = 1 USD
const ANT_RATE = formatRate(0.5) // 1 ANT = 0.5 USD

function exchangedAmount(amount, rate, tokenAllocation) {
// Mimic EVM calculation and truncation for token conversion
return amount.mul(rate).mul(inverseRate).div(ONE.div(100)).trunc()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think inverseRate is not declared, shouldn't it be tokenAllocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ forgot to push. It's here now: 2795a19

@sohkai sohkai force-pushed the payroll-fix-exchange-rate branch from 7ede1b4 to 2795a19 Compare April 26, 2019 14:47
Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

// and applied allocation percentage
uint256 tokenAmount = _totalAmount.mul(exchangeRate).mul(tokenAllocation);
// Divide by 100 for the allocation and by ONE for the exchange rate precision
tokenAmount = tokenAmount / (100 * ONE);

finance.newImmediatePayment(token, employeeAddress, tokenAmount, paymentReference);
emit SendPayment(employeeAddress, token, tokenAmount, paymentReference);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are at it let's log the exchange rate used as it would be really helpful if we ever need to debug something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, I haven't forgot 😉. Will do in another PR.

@sohkai sohkai changed the title Payroll: fix exchange rate and add explicit tests Payroll: revert to old way of obtaining exchange rate and add more clarity Apr 26, 2019
@sohkai sohkai merged commit 5a6570a into payroll_rates_handling_tests Apr 26, 2019
@sohkai sohkai deleted the payroll-fix-exchange-rate branch April 26, 2019 21:20
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.015% when pulling 2795a19 on payroll-fix-exchange-rate into 174bea7 on payroll_rates_handling_tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants