Refactor Initial Multiplier Logic

[3esmit]

Description

• Renames fields to more descritive variable names:
  • Account.multiplier -> Account.currentMP
  • multiplierSupply -> totalSupplyMP
  • stakeSupply -> totalSupplyBalance
• New field:
  • Account.initialMP: used for saving how much of currentMP was generated for bonusMP (can be renamed to bonusMP), retrieved when checking if MAX_BOOST is reached.
• processEpoch is now called at first of every external function that would need it, as a modifier.
• Accounts are properly initialized at first stake (Fixes Properly initialize genesis epoch for account #49)
• It's not possible anymore to unstake only a part of funds, or unstake all, or nothing.
• Fix in migration of processAccount, send only the difference of generated MP to new contract.
• Now with code reuse for math formulas
• Mint Intial (Bonus) MPs now use the same formula as the regular multiplier points, but deltaTime is lockedTime.
• Fixed the Lock and Mint function about locked funds. Now it mints correctly for increased time locked, and increased balance in remaining time locked. Fixes When stake unlocked, increasing stake and locking using stake() mints bonus MPs only for this increment #13
• Changed how lock function works: Now lock time can only increase (previously it reverted), now the parameter value represents time increased, and therefore this case does not exist anymore.

Checklist

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

• Added natspec comments?
• Ran forge snapshot?
• Ran pnpm lint?
• Ran forge test?
• Ran pnpm verify?

[0x-r4bbit]

@3esmit this looks like this needs to be rebased on top of #47

Also, for future PRs please select a reviewer so someone gets notified about it :)

[0x-r4bbit]

@3esmit I've merged #53 and #59 as discussed offline so you can rebase this PR on top of it (now latest develop)

Once done I will review your changes.
You might want to squash your commits into f476860 as we don't need the fix test, fix lint etc commits individually.

Let me know if you need any help with this!

[0x-r4bbit]

Very nice changes @3esmit !

As mentioned offline, i'd like to see some tests on this that show MPs are calculated properly.

@mart1n-xyz would you be able to give us some numbers for 2-3 scenarios (with lockup time, without lockup time, different amount of epochs etc) ?

We can then create tests in which we put in these numbers and see if the code behaves as expected.

[0x-r4bbit]

So we no longer allow for unstaking less than current stake amount?

@mart1n-xyz Double checking: is this desired?

[3esmit]

I removed because this function would give too much losses to user. It could be reimplemented if needed.

[mart1n-xyz]

I think we want to allow partial unstaking (with proportional slashing of MPs).

[3esmit]

Sure, we would have to define how thats calculated.

Currently it would work like that:

initial MP (bonus): reduce on 20% of balance, reduce 20% of initial MP.
current MP: reduce on 20% of balance, reduce 20% of current MP.

This might not be the most precise way to go for it, and we would have to consider if this causes other issues.

[3esmit]

Tests to be written:

• tests for process epoch:
  • should update pending rewards
  • should update total supply
  • should update total supply multiplier points
  • should increase epoch
• tests for process account:
  • should update user multiplier points
  • should update user epoch
  • should pay user proportional rewards
• tests for stake
  • should update user lockup time
  • should mint bonus multiplier points
  • should update bonus multiplier points (restake)
  • should update total supply multiplier points
  • should update total supply balance
  • should update total supply
• tests for lock
  • should update user lockup time
  • should update bonus multiplier points
  • should update total supply multiplier points
• tests for unstake
  • should burn multiplier points
  • should update total supply multiplier points
  • should update total supply balance
  • should update total supply

[mart1n-xyz]

Here's the cases we were simulating.
Case 1 may be interesting as it compares a user with a lock-up and without. I would suggest trying to replicate these parameters for a start.
Case 2 deals with restaking - not yet applicable
Case 3 might be again interesting as one of the 2 users joins a year later.
Case 4&5 are just scaling this to more users and doing stochastic behaviour.

[0x-r4bbit]

So one thing I've realised, is that I keep confusing lockUntil with locktime.
The former is a threshold, the latter is a period.

If we treat _time as something that can only be added to the existing lockUntil, then maybe we can consider renaming lock() to increaseLockTime() ?

Not something that has to happen in this commit/PR, but maybe something to consider in the future.

I always thought lock() would allow me to set my lockup time (say, from 2 years, to 4 years)

[3esmit]

Yes, that can be renamed like that to avoid confusions on development and audits.

Anyway, the UX would have to solve that for the user, no matter how its named or how the interface behaves.

[0x-r4bbit]

Thanks for the updates!

Let's add some tests related to the multiplier/reward calculation, then I think this is good to go.

We can then add all the other tests you've mentioned in your comment in a follow-up PR

[3esmit]

As I am writing tests I am finding other bugs, so I guess this will be a mega PR fixing many bugs.

[0x-r4bbit]

@3esmit I think it's fine if these are part of the refactor changes you've made.

If not, I think it'd be better if we isolate the changes made in this PR so it closes the 1-2 issues it was supposed to close.
Then, additional bugs you encounter, unrelated to the refactor changes can be tracked in additional issues and then tackled one by one.

Sometimes it can't be avoided, but we should strive for keep PRs as small as possible