Test: withdraw unit tests

[paterson1]

Context

Closes #491

Changes proposed in this pull request

Added Withdraw event to StakingPool contract and withdrawNXMStakeAndRewards method to TokenControllerMock. Also in setup update master address for tokenController.

Test plan

Added unit tests to StakingPool/withdraw.js.

Checklist

• Rebased the base branch
• Attached corresponding Github issue
• Prefixed the name with the type of change (i.e. feat, chore, test)
• Performed a self-review of my own code
• Followed the style guidelines of this project
• Made corresponding changes to the documentation
• Didn't generate new warnings
• Didn't generate failures on existing tests
• Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

• 🍰 = Nice to have but not essential.
• 💡 = Suggestion or a comment based on personal opinion
• 🔨 = I believe this should be changed.
• 🤔 = I don’t understand something, do you mind giving me more context?
• 🚀 = Feedback

[shark0der]

Pushed a failed test. We need to test all values properly and not only replicate the formulas in the contract.

[patitonar]

Pushed a failed test.

While reviewing this test I noticed that the total amount of minted rewards was not being distributed to the users and manager, there was a small amount that remained locked in the contract. After debugging I found that the cause of this small amount was a precision loss in the calculation of accNxmPerRewardsShare.

As it is calculated with the following formula elapsed * _rewardPerSecond / _rewardsSharesSupply there were a lot of decimals truncated causing the accumulated value to be less than the actual amount to distribute. The solution I thought for this was to calculate and store the accumulator with an increased precision of ONE_NXM and then scale back the value when calculating the amount of the rewards to transfer.

I applied that change in the commit Increase accNxmPerRewardsShare precision

Let me know your thoughts on this @shark0der

[shark0der]

The accNxmPerRewardsShare fix looks good to me. In terms of semantics I think the previous value was:

the accumulated nxm per 1 reward share

where "1" was actually "1 wei"

The new value represents

the accmuluated nxm per 1e18 reward shares

which makes perfect sense and solves the precision loss issue.

[shark0der]

Right now it seems to me that all tests are using a single staker (unless I missed something). Could you please add a new test where 2+ stakers would deposit and withdraw their stake and rewards? I'm not expecting to discover any issues, but just in case, you know!