Implement cooldown periods

[0x-r4bbit]

The cooldown periods in Status Staking Summary are not implemented.

UPDATE: Explanding this.

There's two "periods" that need to be implemented: deposit_cooldown_period and withdrawal_cooldown_period

These are meant to work as follows:

1. depost_cooldown_period starts when accounts stake funds without_ a lock up time. As long as accounts are within the deposit_cooldown_period, they can immediately unstake their funds (mutliplier points will be lost then)
2. withdrawal_cooldown_period starts after accounts signalled they want to unstake and their deposit_cooldown_period has passed. This period should also apply to the process of claiming rewards (rewards will stay in rewardCollector vault until then but can be immediately "re-staked")

This issue is considered done when the following has been implemented:

• deposit_cooldown_period functionality when unstaking funds
  • Unit tests
  • documentation
  • Application properties
  • Bonus: formal verification
• withdrawal_cooldown_period functionality when unstaking funds
  • Unit tests
  • docs
  • Application properties
  • Bonus: formal verification

Notice that implementing withdrawal_cooldown_period for reward collector vault depends on #34 .
So it's possible to first only implement it for unstake()

[mart1n-xyz]

Cooldown periods will be implemented in the following way:

• Stake deposit and withdrawal cooldowns in Stake Manager
• Reward withdrawal in RewardCollector contract

[0x-r4bbit]

Updated the issue description accordingly.
@mart1n-xyz please let me know if there's anything wrong or a detail missing you think should be added here.

[mart1n-xyz]

Looks good to me

[0x-r4bbit]

After I started working on the deposit_cooldown_period I ran into a couple of questions which brought up a new discussion on how this feature is supposed to work in the more general sense.

I'd like to hereby lay out the options we have and their tradeoffs we have to make for either of these options to work.

Prerequisite

First and foremost I think it's important to understand what impact the deposit_cooldown_period has in terms of semantics and how it would be implemented.

• The deposit_cooldown_period should activate if and only if an account stakes without a lock up period. Accounts that stake without a lock up period can unstake at any time, however, they will not accrue any rewards or multiplier points during the deposit_cooldown_period.
• This essentially really just means that any minting of rewards and multiplier points is delayed until the deposit_cooldown_period is over.
• Because minting of rewards and multiplier points is delayed, accounts need a way to retroactively receive their rewards and multiplier points once the deposit cooldown period has passed.

In addition to the above we need to consider that we haven't yet decided if users will be able to stake into a single vault more than once or not. This will have effects on the usability of this protocol. As part of this discussion, I'll consider both paths to be a possibility: users can either stake into new vault, or add stake to existing vaults.

deposit_cooldown_period in a nutshell

The deposit_cooldown_period is just number that tells the protocol how long this cooldown period lasts (say, 2 weeks).
When an account calls stake() without a lock up period, this will initialise a property in the protocol, that keeps track of when the deposit_cooldown_period is over. In essence this means

uint256 depositCooldownUntil = block.timestamp + deposit_cooldown_period

This is important. This field will be used to determine whether or not rewards and multiplier points will be minted. It also tells use when the stake has happened, as we can calculate it as:

uint256 stakeTimestamp = depositCooldownUntil - deposit_cooldown_period

Which we can later use to then retroactively mint the rewards and multiplier points for the deposit cooldown period.

Further more, it makes sense to keep the depositCooldownUntil inside of the StakeVault instead of the StakeManager. The StakeManager doesn't have to know about every account's deposit cooldown period. The StakeVault would just ensure that the stake() functionality on the StakeManager will only be executed after the deposit cooldown period is over.

This means, users will have to send an additional transaction to signal the StakeManager that their account can be processed.

User scenarios

Next, let's discuss the different scenarios we can run into considering single and multi vault cases.

• multi-vault - Multiple vaults are required for multiple stake transactions. Funds can only be staked into a vault if the vault is empty
• single-vault - Users can add stake to an existing vault, even if it's not empty. Spinning up multiple vaults is optional.

Scenario 1 (multi-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. Once deposit cooldown period is over, actual staking is initialized (via user tx)
4. Rewards and MP are minted retroactively
5. Adding new stake (with or without lockup period requires deployment of new vault)

Scenario 2 (multi-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. User unstakes while cooldown period is running
4. No rewards or MP have been minted so far
5. User can re-enter same vault for staking as it's empty

Scenario 3 (multi-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. User decides to increase lockup time for stake
4. ???

This scenario needs discussion. Right now, (increasing) lock of staked funds only applies to staked funds that the StakeManager is aware of. In the case of the deposit cooldown still being active, the StakeManager doesn't know about these funds yet.

Two options here:

1. We simply don't allow locking of funds that are still in deposit cooldown and wait for cooldown to finish
2. We signal staking to the StakeManager with the existing funds, retroactively mind the rewards + multiplier points for the already passed time, then set the lock for the funds moving forward.

In 2) the MIN_LOCKUP_PERIOD should apply.

@3esmit @mart1n-xyz I think this covers all scenarios as of now for the case we're only supporting one vault per staking action. Please let me know if there's another scenario here.

Scenario 4 (single-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. Once deposit cooldown period is over, actual staking is initialized (via user tx)
4. Rewards and MP are minted retroactively
5. Adding new stake (without lockup time) basically repeats the process starting at 1)

Scenario 5 (single-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. User adds additional funds (without lockup time)
4. ???

Here we have to take into account that the deposit cooldown has partially elapsed. Now the user adds more stake with needs its own deposit cooldown. Again, few ways to go about this:

1. We simply reset the depositCooldownUntil, effectively delaying the accumulation of rewards and MP further and keep doing that as long as funds are being added to the vault before the cooldown period has elapsed. This is a downside for the user, because there's a point where the first funds could already start accruing rewards while the second round of funds should be in deposit cooldown.
2. We can also treat a vault that has a depositCooldownUntil > 0 as "locked", meaning, users have to wait until deposit cooldown is over, until they can add more stake to the vault (or they can spin up a new vault if they don't want to wait)
3. We actually keep track of every stake operation without lock up period individually, so something like:

mapping(uint256 depositLockUntil => uint256 amount) deposits;

Remember though:

This means, users will have to send an additional transaction to signal the StakeManager that their account can be processed.

^ For this we then need to decide how handle this given that we'd now track multiple stake funds and their deposit cooldowns. One way to go about this is the following:

Let's say the function to signal the StakeManager that the depositCooldown is over is called doStake(amount).
Based on amount we can now find all entries of deposits of which the sum of them are <= amount. This is non-trivial because there can be many different combinations of deposits that would allow for that. Also there might be a remainder, which mean we either would actually stake the exact amount that was specified, or we need to update some entry to have the remainder.

Another option would be to only allow doStake(amount) with an amount == some(deposits[i]). That would require a front-end to list all the available amounts that can be staked (deposit cooldown elapsed). Keep in mind that this allows for scenarios where there's multiple amounts (of same value even) that would need to be staked individually, which might be an unexpected user experience.

Scenario 6 (single-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. User adds additional funds (with lockup time)
4. ???

In this case, I think it's reasonable to say: with a lockup time, there's no deposit cooldown, so the amount of additional funds will be registered directly with the StakeManager.

However, as of now, if I'm understanding correctly, this will cause the account to progress in epochs. The question then is: what happens when the deposit cooldown period for the first round of funds is over? Can we still retroactively mint rewards and MP for those, even though the account is already much further down in terms of epochs?

Another option is to go with solution 2) of Scenario 5:

2. We can also treat a vault that has a depositCooldownUntil > 0 as "locked", meaning, users have to wait until deposit cooldown is over, until they can add more stake to the vault (or they can spin up a new vault if they don't want to wait)

Scenario 7 (single-vault)

1. User stakes funds (without lockup time)
2. deposit cooldown is initialized
3. User inceases lockup time
4. ???

This would be the same as Scenario 3, regardless of single- or multi vault.

Two options here:

1. We simply don't allow locking of funds that are still in deposit cooldown and wait for cooldown to finish
2. We signal staking to the StakeManager with the existing funds, retroactively mind the rewards + multiplier points for > the already passed time, then set the lock for the funds moving forward.

@3esmit @mart1n-xyz I'd appreciate any feedback you have on all of this.

I think generally, it's fair to say that the deposit_cooldown_period introduced a lot of complexity that we could avoid if we didn't it in the first place. Otherwise, I think one reasonable way to deal with all of this is to treat any vaults, that are within a deposit cooldown, as "locked" and unable to receive new funds until the period has elapsed.

[mart1n-xyz]

This means, users will have to send an additional transaction to signal the StakeManager that their account can be processed.

I think this can be executed in the first processAccount() call. Then, I guess, this would need to be managed by StakeManager. However, I think it would simplify the rest of the discussion, no? Or is this not feasible?

1. We simply don't allow locking of funds that are still in deposit cooldown and wait for cooldown to finish
2. We signal staking to the StakeManager with the existing funds, retroactively mind the rewards + multiplier points for the already passed time, then set the lock for the funds moving forward.

From a dev perspective, I'd prefer 1. for simplicity. On the contrary, as a user, I know the frustration that you deposit the stake and it is not yet earning anything - it is a strong incentive to lock.

Scenario 5 - I think the following would be a good compromise here:

We can also treat a vault that has a depositCooldownUntil > 0 as "locked", meaning, users have to wait until deposit cooldown is over, until they can add more stake to the vault (or they can spin up a new vault if they don't want to wait)

However, as of now, if I'm understanding correctly, this will cause the account to progress in epochs. The question then is: what happens when the deposit cooldown period for the first round of funds is over? Can we still retroactively mint rewards and MP for those, even though the account is already much further down in terms of epochs?

I think this should be possible if we indeed track the cooldown in StakeManager. We can predict on aggregate how many MPs and stake will exit cooldown and become active in the next epoch (recorded at stake transaction). And the rest of the user triggered calculations can happen with the first processAccount().

You make a good point that all deposits within an epoch are treated the same - as a single deposit. With 2 week cooldown and 1 week epoch, we can have max 1 epoch of additional deposit. Not sure it helps but it gives some specifics to scenarios.

[0x-r4bbit]

I think this can be executed in the first processAccount() call. Then, I guess, this would need to be managed by StakeManager. However, I think it would simplify the rest of the discussion, no? Or is this not feasible?

@mart1n-xyz so the idea with this approach would be: basically don't even let the StakeManager know that a vault has some funds and only let the StakeManager know ones the deposit cooldown in the vault has elapsed.

In other words, I'd probably introduce two new functions (and change one)

• StakeVault.deposit(uint256 amount) which just transfer funds to vault, without letting manager know (locktime is 0), deposit cooldown is initialized
• StakeVault.stake(amount) can be called when deposit cooldown is over (registers funds with manager, starts processAccount), but no longer transfer tokens (as this has already happened.
• StakeVault.depositAndStake(uint256 amount, uint256 increaseLocktime) - The same as the above two combined, basically when staking with a lock up time

Something along those lines.

From a dev perspective, I'd prefer 1. for simplicity. On the contrary, as a user, I know the frustration that you deposit the stake and it is not yet earning anything - it is a strong incentive to lock.

So here, I think it's important to keep in mind that: due to the deposit cooldown you'll not earn anything anyways, if you decide to unstake before deposit cooldown is elapsed and,
Once deposit cooldown is over and you actually register with the manager, you will mint your rewards retroactively.

So one could say that your funds are earning rewards silently.

[0x-r4bbit]

I'm starting to implement some of the ideas discussed in the issue.
While I'm crafting a test for a new deposit(uint256 amount) function, which is bascially transfering funds to the vault and initializing the depositCooldownUntil marker, I realized there's another thing that needs clarification, even if we block additional deposits from happening while there's a cooldown.

Imagine the following scenario (I realized this is very similar to Scenario 5 discussed above, only difference is that the deposit cooldown period has passed, so all solutions from Scenario 5 apply here):

1. User deposits funds into vault (deposit cooldown is initialized)
2. During cooldown, no more funds can be deposited
3. During cooldown, funds that are cooling down can be withdrawn
4. User waits for cooldown to finish
5. User can call stake() to actually stake (and retroactively mint rewards)
6. However, user decides to deposit() again (which is now possible due to cooldown period being over)
7. depositCooldownUntil gets extended by cooldown period

^ Here, if we keep a trivial check in stake() that block.timestamp has to be > depositCooldownUntil, the user is not able to stake() the funds from the first deposit(). The new cooldown threshold prevents the user from staking funds that went through their own cooldown already.

How do we want to handle this? I see a few options:

1. In addition to a "withdrawable" balance, we'll keep track of stakeableBalance, which is the amount of any deposit that went through the cooldown period. Then, when a user calls stake(amount) we'll operate on stakeableBalance and can still have deposits in cooldown phase. However, this adds another complexity: Given that we want to retroactively mint rewards we need to keep track of every deposit's timestamp. Which will bring us back to solution 3 of scenario 5:

We actually keep track of every stake operation without lock up period individually, so something like:
mapping(uint256 depositLockUntil => uint256 amount) deposits;

But then, this will also introduce the additional complexity that, when a user calls stake(amount) we need to decide which of the deposits we actually want to register with the StakeManager.

2. We can avoid the complexity of 1) and instead force the user to first stake() any unstaked deposits. In other words, step 5 in the scenario laid out above is not an option. deposit() will revert as long as there's unstaked funds in the vault.

How does everyone feel about this?

[0x-r4bbit]

I just realized, we'll have a similar complexity with the withdraw_cooldown_period.
The way I would see this work is the following:

1. User has deposited and then staked some funds
2. User decides to unstake(amount) - this will deregister amount from StakeManager an initialized withdrawCooldownUntil
3. Once withdrawCooldownUntil is over, users can call withdraw() on any "withdrawable" funds (there might be already "withdrawable" funds, namely, any deposits that weren't staked and got past their deposit cooldown period)

This works fine if the user decides to unstake() all staked funds.
But lets imagine this scenario:

1. User staked 1000 tokens
2. User unstakes 500 tokens (withdraw cooldown initialized)
3. Withdraw period has elapsed, but user hasn't withdrawn yet
4. User unstakes remaining 500 (or less) tokens
5. Withdraw cooldown is initialized again

So here, we similar need to keep track of funds that were withdrawn before.

In an additional scenario where the user unstake()s while there's already unstaked funds and the withdraw cooldown hasn't elapsed yet, we're again in a state where we'd need to keep track of all withdrawals and their timestamps.

I guess, similarly, we could avoid this complexity by saying, users can only unstake() when there's no withdraw_cooldown_period active.

[0x-r4bbit]

Here's another one that maybe @mart1n-xyz can answer as this might have an impact on how well the reward system is balanced. Here's a scenario:

1. User deposit() into vault (no lock up time, deposit cooldown period starts)
2. Deposit cooldown has elapsed, so user will now call stake() to get the funds registered with the manager

In step 2) it's theoretically possible to give stake() a lock up time.

Questions:

1. Do we want o allow that, or should the stake step after a deposit cooldown period always have 0 lock up time?
2. If we do allow for it, and we retroactively mint the rewards for the deposit cooldown period, will also retroactively apply the lockup time that is passed to stake() ?

In other words, do we want to allow users to also retroactively apply a lockup time for their deposit that was done without a lockup time? Alternatively, we could/would retroactively mint rewards for the deposit cooldown period up until now and then apply the lockup going forward.

[mart1n-xyz]

How does everyone feel about this?

I still feel the solution to majority of these problems indeed lies in tracking the cooldowns in StakeManager and not async in StakeVault. Even if not, in this case, we can make sure processAccount is called if any additional stake is being added (called from within deposit()), activating the previous deposit and emptying the cooldown queue for the new deposit.

I guess, similarly, we could avoid this complexity by saying, users can only unstake() when there's no withdraw_cooldown_period active.

I think this is preferable, I have actually experienced this behaviour in other staking contracts. We want to limit response (selling) to price action and having a gradual unstaking is a strong mechanism here. Otherwise, we'd just delay the response by 2 weeks, this way we may somewhat smooth it out.

1. Do we want to allow that, or should the stake step after a deposit cooldown period always have 0 lock up time?

If we have this deposit()/stake() split, we need to have the user specify the lock up time in deposit(). This is because for locked vaults, the cooldown does not apply (obviously, we need MIN_LOCKUP_TIME>2 weeks). This is another reason why I think we need to get the call to StakeManager.

Let me know if I am missing something.

[0x-r4bbit]

I still feel the solution to majority of these problems indeed lies in tracking the cooldowns in StakeManager and not async in StakeVault

I actually started with having the cooldown periods tracked in the manager, but after discussing this with @3esmit we decided that this would make processAccount() more complex and it is already very complex and we want to limit complexity. In addition, it seems to simplify stuff if the manager doesn't know anything about cooldown periods and only needs to care about funds that actually make it into the manager.

If we have this deposit()/stake() split, we need to have the user specify the lock up time in deposit(). This is because for locked vaults, the cooldown does not apply (obviously, we need MIN_LOCKUP_TIME>2 weeks). This is another reason why I think we need to get the call to StakeManager.

@mart1n-xyz deposit() by definition means lockup time == 0. This is because, as mentioned above, the manager doesn't need to know anything about these funds until they actually become relevant (after deposit cooldown has expired). So users first deposit, then stake (or withdraw before)

If you want to deposit and stake right away in one go (which requires a lockup time) you'd need to call depositAndStake(amount, time) - a new function I'm introducing but it essentially combins deposit() + stake() with a bunch of sanity checks.

However, still, in case of using deposit(), there'll be a point where the user actually registers its funds with the stakemanager by calling StakeVault.stake(). That function requires the deposit cooldown to be expired.

At this point we really just need to decide a) do we allow for apply a lock time now? Basically as if we did stake + lock() and if yes then b) would the lock time retroactively apply to rewards minting as well.

I personally think it makes more sense to treat it as

deposit() -> cooldown -> stake() -> lock(lockTime) // applies lock time after retroactively minting rewards 

instead of

deposit() -> cooldown -> stake(lockTime) // applies lock time as part of retroactively minting rewards

[0x-r4bbit]

Here's a draft branch that shows what this looks like in action (note: it's a WIP)

• StakeVault.deposit(uint256 amount)
  8c5dd44#diff-b710313a5571054e746fc0e0d1332e5894fc76a55ffb035711d912c00bf8f826R63-R66
• StakeVault.stake(uint256 amount)
  8c5dd44#diff-b710313a5571054e746fc0e0d1332e5894fc76a55ffb035711d912c00bf8f826R77-R79
• StakeVault.depositAndStake(uint256 amount, uint256 time)
  8c5dd44#diff-b710313a5571054e746fc0e0d1332e5894fc76a55ffb035711d912c00bf8f826R81-R90

Again, it's a WIP. Some things that are missing is the unstake/withdrawal handling and also the ability to call _stake() with an additional timestamp that tells StakeManager from which point onwards it needs to retroactively mint rewards.

[mart1n-xyz]

Thank you for explaining, this makes sense. I may have been missing your point earlier.

a) do we allow for apply a lock time now?

Yes, it makes sense to allow it. As you mention, user can call lock() anyway. I agree with the deposit() -> cooldown -> stake() -> lock(lockTime).

I do not see why we should retroactively grant lock time from cooldown - it is user's fault in the first place that he didn't lock at the onset. Allowing for this introduces inconsistency. The dominant strategy would be to always go this path since you have the same outcome as with depositAndStake() but can withdraw anytime within those 2 weeks.

[0x-r4bbit]

Another scenario, this time considering the withdraw_cooldown_period.

The withdraw_cooldown_period is similar to the deposit cooldown period, just that it will be initialized when a user unstakes funds. So here's the flow:

1. User has some funds staked
2. User calls unstake(amount)
3. This will initialize withdrawCooldownUntil
4. User will have to call withdraw() once block.timestamp > withdrawCooldownUntil

Very similar to deposit cooldown, similar things apply. If the user unstake()s again, while in withdraw cooldown, the cooldown period gets reset (this reasonable?)

But, what if the following happens:

1. User has some funds staked
2. User deposits into vault more funds (no staking) - initializes deposit cooldown
3. User unstakes some of the staked funds - initializes withdraw cooldown
4. User wants to withdraw funds from deposit done in 2) but can't because there's a withdraw cooldown

Does this mean that withdraw cooldown has always highes priorty? Or maybe we also don't allow for unstaking while in deposit cool down?

[mart1n-xyz]

Does this mean that withdraw cooldown has always highes priorty? Or maybe we also don't allow for unstaking while in deposit cool down?

I'd argue for disabling unstaking while there's an active withdrawal cooldown as per one of my previous comments. Reseting the cooldown is another option but I think suboptimal. Here, I'd again consider that we are dealing with epoch time units - hence withdrawals within one epoch can be treated as a single one.

[0x-r4bbit]

This is blocked by #91 and inherently blocked by #48

[0x-r4bbit]

Unassigning and rolling back to "TODO" state.
At this point we don't even know if we want to keep cooldown periods.