test(RewardsStreamerMP): ensure bonusMP and maxMP are decreased

[0x-r4bbit]

correctly at unstake

This commit adds some tests to check that, if a user (partically) unstakes their funds, their initial and bonus MP get decreased proportionally, as well as their max mp.

Closes #46

Checklist

Ensure you completed all of the steps below before submitting your pull request:

• [ ] Added natspec comments?
• [x] Ran pnpm adorno?
• [ ] Ran pnpm verify?

[0x-r4bbit]

This should also address #43

[3esmit]

The test seem biased. It dosent seem to check for proportional reduce in MPs. But I might not be totally familiar with your test script. I'll submit a commit with a test I think is correct for this behavior. I'll also make the same on Stake Manager, as there it could also not be correct as we changed the logic a lot and dont have all proper tests for unstaking (we focused a lot on process account, epoch and estimation of MPs)

[0x-r4bbit]

@3esmit thank you for taking a look!

The test seem biased. It dosent seem to check for proportional reduce in MPs

Can you elaborate what you mean by that? Currently, at unstake, MPs are proportionally reduced based on the amount you're unstaking. This includes all of your MP (bonusMP + accrued MP).

The test provided shows this with concrete numbers, you can plug in any you like (happy to add another test for completeness-sake)

Or are you referring to anything else?

[3esmit]

It looks biased because the tests are against hardcoded values instead of using calculations based on the inputs, and the values used for testing are too nice/round - e.g. calculating against 1 year that gives exactly double the value of balance.

[3esmit]

I reviewed the math from scratch, and determined that it indeed would return the correct values for when $A_{balance}$ increases, accrues, and later decreases by unstake.

RewardsStreamerMP uses the following formula:

$$ MP_{total2} = MP{total1} - \frac{MP{total1} \times A{reduced}}{A_{balance_1}} $$

I found a simplification for this formula

$$ MP_{total2} = A{balance1} \times \frac{MP{total1}}{A{balance_1}} $$

However, for increasing $A{balance}$ (restake) this will not work, because $MP{accrued}$ (and consequently $MP_{total}$) is not retroactively increased, unless the calculation is done incrementally (i.e., based on the balance during each respective period).

To solve that we would need to store $MP{bonus}$ (or extract it from $$MP{max} - MP{MAX}(A{balance})$$), so the $MP{accrued}$ can be extracted from $MP{total}$ and the $MP_{bonus2}$ can be recalculated from $t{locked}$ (pro-rated estimated from $A{balance}$ and $MP{bonus}$), while $MP_{accrued}$ is proportionally reduced. Another option would be to save timestamp of last balance change and use it to determine all other values.

---

Also, I didnt understand yet why the $MP_{bonus}$ is being calculated as it is. Currently RewardsStreamerMP uses:

$$ MP{bonus} = A{balance} * \frac{\left(\frac{t{locked} \times M{MAX} \times {SCALE{FACTOR}}} {t{MAX}} \right)}{SCALE_{FACTOR}} $$

Why not use the normal formula?

$$ MP{\text{bonus}} = A{balance} + \left( \frac{A{balance} \times t{locked}}{t_{year}} \right) $$

Maybe that should be another issue opened?

---

About the commit I submitted, I recognize it is currently wrong, I am still having to familiarize with the formulas on this contract and the difference in use storing $MP{max}$ instead of $MP{bonus}$, which I believe is a good improvement as it reduces calculations (I'll do that in StakeManager as well).

I'll fix them and create a test for the scenario with stake + time passes + unstake + time passes + stake + time passes + unstake + (...) to test if this is actually a bug, and if it is, how impactful, and what can be done to fix or mitigate it.

[0x-r4bbit]

About the commit I submitted, I recognize it is currently wrong [...] I'll fix them and create a test for the scenario with stake + time passes + unstake + time passes + stake + time passes + unstake + (...) to test if this is actually a bug, and if it is, how impactful, and what can be done to fix or mitigate it.

Awesome. Given that this PR otherwise provides the correct tests and the math seems right, would you mind adding your test refinemint in a separate PR? That would unblock this one so we can get it merged.

[0x-r4bbit]

Maybe that should be another issue opened?

Will create an issue to discuss this.

[0x-r4bbit]

@3esmit created https://github.com/vacp2p/staking-reward-streamer/issues/54 @gravityblast if you can, please also take a look.

[0x-r4bbit]

RewardsStreamerMP uses the following formula:

$$ MP_{total2} = MP{total1} - \frac{MP{total1} \times A{reduced}}{A_{balance_1}} $$

I found a simplification for this formula

$$ MP_{total2} = A{balance1} \times \frac{MP{total1}}{A{balance_1}} $$

However, for increasing $A{balance}$ (restake) this will not work, because $MP{accrued}$ (and consequently > $MP_{total}$) is not retroactively increased, unless the calculation is done incrementally (i.e., based on the balance > during each respective period).

To solve that we would need to store $MP{bonus}$ (or extract it from $$MP{max} - MP{MAX}(A{balance})$$), so the >$MP{accrued}$ can be extracted from $MP{total}$ and the $MP_{bonus2}$ can be recalculated from $t{locked}$ (pro-rated estimated from $A{balance}$ and $MP{bonus}$), while $MP_{accrued}$ is proportionally reduced. Another option would be to save timestamp of last balance change and use it to determine all other values.

@3esmit so you're saying, we either use the formula you've suggest, which however would require more changes to properly update the contract's state, or we just stick with the formula that's currently implemented?

[3esmit]

The simplification changes nothing besides less mathematical operations. To fix the issue we would have to implement a way of recalculating bonusmp so it can be increased separately

[0x-r4bbit]

@3esmit Thanks again for providing your additional test.

I've rebased all changes on top the latest changes to the user.maxMP and totalMaxMP. I've adjusted one line in the test you've provided, according to the changes.

The test now passes.

You can see the commit I've added to check.

Once you and @gravityblast give green light on this, I'm going to squash the last three commits into your commit that adds the additional test before this gets merged.

[0x-r4bbit]

I've squashed 1f42c71 in to bd9c683 and pushed again. Will merge once green