fix(StakeManager): initial MP should not increase when locking #94
Conversation
Some of the functions on our contracts were confusing. This commit changes them so they describe what they actually do.
|
Interesting, this change broke the |
|
The issue here is that, when we |
0x-r4bbit
force-pushed
the
fix/lock-init-mp
branch
from
c163c88 to
cef950a
Compare
| @@ -222,9 +222,18 @@ contract StakeManager is Ownable { | |||
| if (deltaTime < MIN_LOCKUP_PERIOD || deltaTime > MAX_LOCKUP_PERIOD) { | |||
| revert StakeManager__InvalidLockTime(); | |||
| } | |||
| _mintInitialMP(account, _timeToIncrease, 0); | |||
| //update account storage | |||
There was a problem hiding this comment.
_mintInitalMP() increases account.initialMP which I don't think we want when we lock()
| account.balance, | ||
| account.initialMP, | ||
| account.currentMP | ||
| ); |
There was a problem hiding this comment.
Instead we want to mint whatever MPs the account is entitled for.
@3esmit this ultimately relies on getMPToMin(account.balance, _timeToIncrease) just like before.
However, I'm wondering if we shouldn't instead do
_getMPToMint(account.balance, _timeToIncrease - account.lastMint)
What do you think?
| account.lockUntil = lockUntil; | ||
| account.lastMint = block.timestamp; |
There was a problem hiding this comment.
The are storage updates that were otherwise happening in _mintInitialMP().
Question @3esmit: I think we need to update currentEpoch.totalSupply as well no?
If yes, then we could potentially just leverage _mintMP() instead.
When an account calls `lock()` on the `StakeManager` it ultimately called `_mintInitialMP()` which besides increasing the accounts `currentMP`, it also increases the accounts `initialMP`. This seems incorrect, as `initialMP` should only increase at first stake. Also, notice how inside `lock()` it always passes an `amount` of `0` to to `_mintInitialMP()`, so it only ever uses one code path of that function. This commit adjust `lock()` such that it no longer uses `_mintInitialMP()`. Instead it makes use of `_getMaxMPToMint` based on the lock up time delta. It also removes a test that claims that `initialMP` should increase on `lock()` and instead adds a test an a CVL rule ensure `initialMP` doesn't increase when locking.
0x-r4bbit
force-pushed
the
fix/lock-init-mp
branch
from
cef950a to
0bf544d
Compare
When an account calls
lock()on theStakeManagerit ultimatelycalled
_mintInitialMP()which besides increasing the accountscurrentMP, it also increases the accountsinitialMP.This seems incorrect, as
initialMPshould only increase at firststake.
Also, notice how inside
lock()it always passes anamountof0toto
_mintInitialMP(), so it only ever uses one code path of thatfunction.
This commit adjust
lock()such that it no longer uses_mintInitialMP(). Instead it makes use of_getMaxMPToMintbased onthe lock up time delta.
It also removes a test that claims that
initialMPshould increase onlock()and instead adds a test an a CVL rule ensureinitialMPdoesn't increase when locking.
Checklist
Ensure you completed all of the steps below before submitting your pull request:
forge snapshot?pnpm gas-report?pnpm lint?forge test?pnpm verify?