Reset no longer needed on staking & fix underflow #110
Conversation
There was a problem hiding this comment.
Looks good!
I think there are some other places where we may update tests to the fact that we don’t need resets on deposit/withdrawal anymore, like:
https://github.com/liquity/V2-gov/blob/main/test/recon/targets/GovernanceTargets.sol#L110
https://github.com/liquity/V2-gov/blob/main/test/recon/targets/GovernanceTargets.sol#L283
But happy to merge this as is, we can fix that later if it’s more convenient.
src/Governance.sol
Outdated
| @@ -203,7 +196,6 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own | |||
| function withdrawLQTY(uint256 _lqtyAmount, bool _doSendRewards, address _recipient) public nonReentrant { | |||
| // check that user has reset before changing lqty balance | |||
There was a problem hiding this comment.
I guess we should remove this comment now.
This sequence of events was triggering a bug in older version of `Governance`, which is why the requirement to reset all allocations before staking/unstaking was introduced. As we are removing that requirement, it makes sense to add a test case for this.
|
I'd fix the invariant tests in a separate PR later, as there are already some broken ones after the voting power refactor. |
|
On the other hand, I dug up the test case I wrote to demonstrate the bug that was the whole reason for the reset requirement, updated it, and added it to the test suite. |
|
Awesome, thanks!! |
Address remaining TODO in
_increaseUserVoteTrackers()by eliminating reset requirement.Fixes #108.
Fix possible underflow in
withdrawLQTY().Fixes #109.