Skip to content

Commit

Permalink
fix: improve precision loss when unstaking and add testso
Browse files Browse the repository at this point in the history 
This commit changes the calculation for when MPs are reduced globally
and for the user that is unstaking.

Instead of getting an `amountRatio` first and using that the
multiplication, we're now applying the `SCALE_FACTOR` to both, the
numerator and denominator to maintain the ratio while increasing
precision.
  • Loading branch information
@0x-r4bbit
0x-r4bbit committed Oct 9, 2024
1 parent d54b1ab commit 8b718ee
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 29 deletions.
28 changes: 14 additions & 14 deletions .gas-report
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,32 +15,32 @@
| src/RewardsStreamerMP.sol:RewardsStreamerMP contract | | | | | |
|------------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost | Deployment Size | | | | |
| 1100217 | 4957 | | | | |
| 1105804 | 4983 | | | | |
| Function Name | min | avg | median | max | # calls |
| MAX_LOCKING_PERIOD | 228 | 228 | 228 | 228 | 18 |
| MAX_MULTIPLIER | 229 | 229 | 229 | 229 | 24 |
| MIN_LOCKING_PERIOD | 229 | 229 | 229 | 229 | 8 |
| SCALE_FACTOR | 251 | 251 | 251 | 251 | 28 |
| accountedRewards | 373 | 1075 | 373 | 2373 | 37 |
| getUserInfo | 1553 | 1553 | 1553 | 1553 | 32 |
| potentialMP | 330 | 330 | 330 | 330 | 37 |
| rewardIndex | 373 | 427 | 373 | 2373 | 37 |
| stake | 167821 | 217684 | 228608 | 249401 | 31 |
| totalMP | 352 | 352 | 352 | 352 | 37 |
| totalStaked | 330 | 330 | 330 | 330 | 37 |
| unstake | 75267 | 113408 | 133268 | 133945 | 5 |
| updateGlobalState | 30008 | 67918 | 80335 | 80335 | 9 |
| accountedRewards | 373 | 963 | 373 | 2373 | 44 |
| getUserInfo | 1553 | 1553 | 1553 | 1553 | 41 |
| potentialMP | 330 | 330 | 330 | 330 | 44 |
| rewardIndex | 373 | 418 | 373 | 2373 | 44 |
| stake | 167821 | 215459 | 228608 | 249401 | 35 |
| totalMP | 352 | 352 | 352 | 352 | 44 |
| totalStaked | 330 | 330 | 330 | 330 | 44 |
| unstake | 75511 | 107650 | 110519 | 134250 | 10 |
| updateGlobalState | 30008 | 69159 | 80335 | 80335 | 10 |


| test/mocks/MockToken.sol:MockToken contract | | | | | |
|---------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost | Deployment Size | | | | |
| 639406 | 3369 | | | | |
| Function Name | min | avg | median | max | # calls |
| approve | 46346 | 46346 | 46346 | 46346 | 110 |
| balanceOf | 561 | 1334 | 561 | 2561 | 168 |
| mint | 51284 | 58124 | 51284 | 68384 | 110 |
| transfer | 34390 | 47690 | 51490 | 51490 | 9 |
| approve | 46346 | 46346 | 46346 | 46346 | 120 |
| balanceOf | 561 | 1327 | 561 | 2561 | 201 |
| mint | 51284 | 58124 | 51284 | 68384 | 120 |
| transfer | 34390 | 48070 | 51490 | 51490 | 10 |



Expand Down
22 changes: 12 additions & 10 deletions .gas-snapshot
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
IntegrationTest:testStake() (gas: 1377603)
IntegrationTest:testStake() (gas: 1378213)
RewardsStreamerTest:testStake() (gas: 869874)
StakeTest:test_StakeMultipleAccounts() (gas: 438756)
StakeTest:test_StakeMultipleAccountsAndRewards() (gas: 586002)
Expand All @@ -9,14 +9,16 @@ StakeTest:test_StakeOneAccountAndRewards() (gas: 415039)
StakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 284120)
StakeTest:test_StakeOneAccountWithMinLockUp() (gas: 284152)
StakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 284196)
UnstakeTest:test_StakeMultipleAccounts() (gas: 438756)
UnstakeTest:test_StakeMultipleAccountsAndRewards() (gas: 586069)
UnstakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 449192)
UnstakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 470881)
UnstakeTest:test_StakeMultipleAccounts() (gas: 438778)
UnstakeTest:test_StakeMultipleAccountsAndRewards() (gas: 586002)
UnstakeTest:test_StakeMultipleAccountsWithMinLockUp() (gas: 449214)
UnstakeTest:test_StakeMultipleAccountsWithRandomLockUp() (gas: 470903)
UnstakeTest:test_StakeOneAccount() (gas: 267795)
UnstakeTest:test_StakeOneAccountAndRewards() (gas: 415039)
UnstakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 284186)
UnstakeTest:test_StakeOneAccountAndRewards() (gas: 415061)
UnstakeTest:test_StakeOneAccountWithMaxLockUp() (gas: 284120)
UnstakeTest:test_StakeOneAccountWithMinLockUp() (gas: 284152)
UnstakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 284263)
UnstakeTest:test_UnstakeOneAccount() (gas: 445796)
UnstakeTest:test_UnstakeOneAccountAndRewards() (gas: 556852)
UnstakeTest:test_StakeOneAccountWithRandomLockUp() (gas: 284196)
UnstakeTest:test_UnstakeMultipleAccounts() (gas: 616327)
UnstakeTest:test_UnstakeMultipleAccountsAndRewards() (gas: 937593)
UnstakeTest:test_UnstakeOneAccount() (gas: 446306)
UnstakeTest:test_UnstakeOneAccountAndRewards() (gas: 557157)
10 changes: 5 additions & 5 deletions src/RewardsStreamerMP.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,17 +118,17 @@ contract RewardsStreamerMP is ReentrancyGuard {
}

uint256 previousStakedBalance = user.stakedBalance;
user.stakedBalance -= amount;
totalStaked -= amount;

uint256 amountRatio = (amount * SCALE_FACTOR) / previousStakedBalance;
uint256 mpToReduce = (user.userMP * amountRatio) / SCALE_FACTOR;
uint256 potentialMPToReduce = (user.userPotentialMP * amountRatio) / SCALE_FACTOR;
uint256 mpToReduce = (user.userMP * amount * SCALE_FACTOR) / (previousStakedBalance * SCALE_FACTOR);
uint256 potentialMPToReduce =
(user.userPotentialMP * amount * SCALE_FACTOR) / (previousStakedBalance * SCALE_FACTOR);

user.stakedBalance -= amount;
user.userMP -= mpToReduce;
user.userPotentialMP -= potentialMPToReduce;
totalMP -= mpToReduce;
potentialMP -= potentialMPToReduce;
totalStaked -= amount;

bool success = STAKING_TOKEN.transfer(msg.sender, amount);
if (!success) {
Expand Down
121 changes: 121 additions & 0 deletions test/RewardsStreamerMP.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,4 +814,125 @@ contract UnstakeTest is StakeTest {
})
);
}

function test_UnstakeMultipleAccounts() public {
test_StakeMultipleAccounts();

_unstake(alice, 10e18);
_unstake(bob, 10e18);

checkStreamer(
CheckStreamerParams({
totalStaked: 20e18,
totalMP: 20e18,
potentialMP: 80e18,
stakingBalance: 20e18,
rewardBalance: 0,
rewardIndex: 0,
accountedRewards: 0
})
);

checkUser(
CheckUserParams({
user: alice,
rewardBalance: 0,
stakedBalance: 0,
rewardIndex: 0,
userMP: 0,
userPotentialMP: 0
})
);

checkUser(
CheckUserParams({
user: bob,
rewardBalance: 0,
stakedBalance: 20e18,
rewardIndex: 0,
userMP: 20e18,
userPotentialMP: 80e18
})
);
}

function test_UnstakeMultipleAccountsAndRewards() public {
test_StakeMultipleAccountsAndRewards();

_unstake(alice, 10e18);

checkStreamer(
CheckStreamerParams({
totalStaked: 30e18,
totalMP: 30e18,
potentialMP: 120e18,
stakingBalance: 30e18,
// alice owned a 25% of the pool, so 25% of the rewards are paid out to alice (250)
rewardBalance: 750e18,
rewardIndex: 125e17, // reward index remains unchanged
accountedRewards: 750e18
})
);

checkUser(
CheckUserParams({
user: alice,
rewardBalance: 250e18,
stakedBalance: 0,
rewardIndex: 125e17,
userMP: 0,
userPotentialMP: 0
})
);

_unstake(bob, 10e18);

checkStreamer(
CheckStreamerParams({
totalStaked: 20e18,
totalMP: 20e18,
potentialMP: 80e18,
stakingBalance: 20e18,
rewardBalance: 0, // bob should've now gotten the rest of the rewards
rewardIndex: 125e17,
accountedRewards: 0
})
);

checkUser(
CheckUserParams({
user: bob,
rewardBalance: 750e18,
stakedBalance: 20e18,
rewardIndex: 125e17,
userMP: 20e18,
userPotentialMP: 80e18
})
);

_unstake(bob, 20e18);

checkStreamer(
CheckStreamerParams({
totalStaked: 0,
totalMP: 0,
potentialMP: 0,
stakingBalance: 0,
rewardBalance: 0,
rewardIndex: 125e17,
accountedRewards: 0
})
);

checkUser(
CheckUserParams({
user: bob,
rewardBalance: 750e18,
stakedBalance: 0,
rewardIndex: 125e17,
userMP: 0,
userPotentialMP: 0
})
);
}
}

0 comments on commit 8b718ee

Please sign in to comment.