fix: dust left in unallocatedOffset despite allocating all LQTY
#112
Conversation
| @@ -606,6 +606,7 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own | |||
| _requireNoNegatives(_absoluteLQTYVetos); | |||
| // If the goal is to remove all votes from an initiative, including in _initiativesToReset is enough | |||
| _requireNoNOP(_absoluteLQTYVotes, _absoluteLQTYVetos); | |||
| _requireNoSimultaneousVoteAndVeto(_absoluteLQTYVotes, _absoluteLQTYVetos); | |||
| pure | ||
| { | ||
| for (uint256 i; i < _absoluteLQTYVotes.length; i++) { | ||
| require(_absoluteLQTYVotes[i] == 0 || _absoluteLQTYVetos[i] == 0, "Governance: vote-and-veto"); |
There was a problem hiding this comment.
Much nicer than the original double negation! ;-)
| uint256 offset = userState.unallocatedOffset * lqtyAmount / userState.unallocatedLQTY; | ||
|
|
||
| userState.unallocatedLQTY -= lqtyAmount; | ||
| userState.unallocatedOffset -= offset; |
There was a problem hiding this comment.
I guess it’s fine (specially because the difference should be almost negligible), but I wonder if we should put a comment mentioning that this is “path dependent” (related to the order of the array).
Let’s put an example with numbers. Let’s say unallocated LQTY is 10, unallocated offset is 5, and we allocate 3 and 7.
Before, we would have:
- offset[0] = floor(3*5/10) = floor(1.5) = 1
- offset[1] = floor(7*5/10) = floor(3.5) = 3
So there would be a remainder of 1 for the offset, when it should be zero.
Now, we’ll have:
- offset[0] = floor(3*5/10) = floor(1.5) = 1
- offset[1] = floor(7*4/7) = floor(4) = 4
(as we first reduce unallocated amounts, from 10/5 to 7/4)
So now the final offset is zero as we want, but all the error is “accumulated” in the last initiative.
There was a problem hiding this comment.
Yes, it is path-dependent. I agree it makes sense to document this.
I wouldn't say that all the error is accumulated in the last initiative, more like spread out over all initiatives. Consider an example with more than 2 initiatives. Let's say we have 4 LQTY and an offset of 3, which we want to split up equally between 4 initiatives.
Ideal:
offset[0] = 3*1/4 = 0.75
offset[1] = 3*1/4 = 0.75
offset[2] = 3*1/4 = 0.75
offset[3] = 3*1/4 = 0.75Before:
offset[0] = floor(3*1/4) = floor(0.75) = 0 // error[0] = 0 - 0.75 = -0.75
offset[1] = floor(3*1/4) = floor(0.75) = 0 // error[1] = 0 - 0.75 = -0.75
offset[2] = floor(3*1/4) = floor(0.75) = 0 // error[2] = 0 - 0.75 = -0.75
offset[3] = floor(3*1/4) = floor(0.75) = 0 // error[3] = 0 - 0.75 = -0.75Now:
offset[0] = floor(3*1/4) = floor(0.75) = 0 // error[0] = 0 - 0.75 = -0.75
offset[1] = floor(3*1/3) = floor(1.00) = 1 // error[1] = 1 - 0.75 = 0.25
offset[2] = floor(2*1/2) = floor(1.00) = 1 // error[2] = 1 - 0.75 = 0.25
offset[3] = floor(1*1/1) = floor(1.00) = 1 // error[3] = 1 - 0.75 = 0.25There was a problem hiding this comment.
Yes, right! Which means the issue is even less bad.
There was a problem hiding this comment.
I am now wondering if this is an improvement at all. Sure, the "average" offset error when allocating all of one's LQTY is zero, but it doesn't change the fact that there are (small) errors in the offsets allocated to the individual initiatives. Previously, the per-initiative error was in the (-1, 0] range, whereas now it's somewhere in (-1, 1).
Is this worth it? It's not like you can continue accumulating "dust" in unallocatedOffset, as we force you to fully deallocate before making any new allocations, which "resets" the accumulated error.
There was a problem hiding this comment.
Yes, good point, but I still think that having unallocatedLQTY zero, but non-zero unallocatedOffset feels a little bit ugly. I wonder if having a positive error in the offset of the initiatives can introduce any new problem.
There was a problem hiding this comment.
Added assertions and tests to ensure this.
There was a problem hiding this comment.
Added comment about path-dependency.
There was a problem hiding this comment.
If someone deposits LQTY then immediately allocates it to several initatives, their voting power assigned to each initatitive should be 0, but due to the rounding error, some will get positive and some will get negative voting power, right? (assuming no clamping)
It seems like either users or initatives need to bear the error.
Anyway still clamping lqtyToVotes, right? So in practice we don't get negative voting power. Given that, I'm happy with the solution for clearing the user's dust.
There was a problem hiding this comment.
When someone has zero unallocated voting power, it implies that their unallocated offset is a multiple of their unallocated LQTY, thus there can be no rounding error when allocating it to any number of initiatives.
| uint256 waitTime; | ||
| } | ||
|
|
||
| function test_NoDustInUnallocatedOffsetAfterAllocatingAllLQTY(uint256[3] memory _votes, StakingOp[4] memory _stakes) |
test/Governance.t.sol
Outdated
| // By waiting `initialVotingPower` seconds while having 1 wei LQTY staked, | ||
| // we accrue exactly `initialVotingPower` | ||
| vm.warp(block.timestamp + initialVotingPower); | ||
| governance.depositLQTY(583399417581888701); |
There was a problem hiding this comment.
Where dies this number come from?
There was a problem hiding this comment.
Just some random prime. Actually, there's nothing special about it, any number big enough to result in rounding error would work, even round numbers.
test/Governance.t.sol
Outdated
| // we accrue exactly `initialVotingPower` | ||
| vm.warp(block.timestamp + initialVotingPower); | ||
|
|
||
| governance.depositLQTY(583399417581888701); |
This addresses CS-V2Gov-051: Rounding Error on Offset Calculation.
It should be noted that we're not actually eliminating the rounding error — that's not feasible unless we stop mixing together LQTY staked at different timestamps in
userState— we're merely feeding it back intounallocatedOffsetso that there's no "dust" remaining after a total allocation of one's LQTY.Closes #111.