Stake top-ups #1893
Stake top-ups #1893
Conversation
The idea behind a top-up is to let the staker add KEEP tokens to existing delegations. This is a two-step process: first, staker initiates a top-up, locking new KEEPs in the staking contract. Once the initialization period is over, staker (or anyone else) can commit the top-up, increasing operator's stake. From the smart contract's perspective, top-ups are performed using the same interface as delegations: `receiveApproval` on `TokenStaking` contract. If the given operator is active, tokens are treated as a top-up. If the given operator is not known, tokens are used to create a new delegation. Operators who have their stake undelegated cannot have their stake topped-up. It means that we still enforce operator's uniqueness - operator address cannot be reused after delegating and recovering the stake. This is important because token staking escrow assumes operator's uniqueness when accepting token deposit.
All redelegations from token staking escrow should be considered as top-ups if they go to an existing, active operator.
There was a problem hiding this comment.
Left a few small notes here and there. Overall looks good.
For bytecode after this PR, I think the question for me becomes “can we push more things into libraries if we give those libraries references to other contracts/libraries”. For example, pushing most of initiateTopUp into the TopUp library by giving it access to escrow, token grant, and grant staking or something of the sort.
solidity/contracts/TokenStaking.sol
Outdated
| /// @notice Receives approval of token transfer and stakes the approved amount. | ||
| /// @dev Makes sure provided token contract is the same one linked to this contract. | ||
| /// @notice Receives approval of token transfer and stakes the approved | ||
| /// amount or top-ups an existing delegation with the approved amount. |
There was a problem hiding this comment.
I think we can say ‘or adds the approved amount to an existing delegation (a “top-up”)’.
solidity/contracts/TokenStaking.sol
Outdated
| /// @dev Makes sure provided token contract is the same one linked to this contract. | ||
| /// @notice Receives approval of token transfer and stakes the approved | ||
| /// amount or top-ups an existing delegation with the approved amount. | ||
| /// In case of a top-up, it is expected that the operator stake passed |
There was a problem hiding this comment.
And talk about the “case of an existing delegation”.
I also think you're noting “it is expected”, but actually it is required that the existing delegation has passed the initialization period, etc etc.
solidity/contracts/TokenStaking.sol
Outdated
| /// using liquid tokens, only liquid tokens from the same owner can be used | ||
| /// to top-up the stake. | ||
| /// @dev Makes sure provided token contract is the same one linked to this | ||
| /// contract. |
There was a problem hiding this comment.
I would express this more as “Requires that the provided token contract be the same one linked to this contract.”
| uint256 _value, | ||
| address _token, | ||
| bytes memory _extraData | ||
| ) public { | ||
| require(ERC20Burnable(_token) == token, "Unrecognized token contract"); | ||
| require(_value >= minimumStake(), "Value must be greater than the minimum stake"); | ||
| require(_extraData.length >= 60, "Corrupted delegation data"); |
There was a problem hiding this comment.
Would be nice to make this == 60 || == 92, though I realize we've got code size limits to think about.
There was a problem hiding this comment.
Having the experience from #1902: adding a separate requires costs more but expanding an existing one shouldn't be a problem. Just to be safe, please let me deal with it in the PR with the existing stake bridging, along with the comment from @nkuba.
| /// @notice Tries to capture delegation data if the pending delegation has | ||
| /// been created from a grant. There are only two possibilities and they | ||
| /// need to be handled differently: delegation comes from the TokenGrant | ||
| /// contract or delegation comes from TokenStakincEscrow. In those two cases |
Made it clearer what a "top-up" is and what are the requirements of TokenStaking.approveAndCall. Fixed one typo.
That was my first choice. I can't recall exactly why I abandoned this path but I think it was a mix of the number of parameters I had to pass and the fact I couldn't easily make it work without |
|
@Shadowfiend @nkuba Thanks for the review, this is ready for another chance. |
Reduce bytecode size of TokenStaking contract Extracting stake locks to a separate library (#1888) provided some space in `TokenStaking` bytecode but we quickly used all of that space for the implementation of top-ups (#1893). In this PR, we are doing small changes, one by one, to reduce the bytecode size to allow for stake bridge implementation (#1883). This PR reduces the bytecode from `24596` to `24270` (`-326` bytes) and will let us add just a few lines of code, an absolute minimum needed for the stake bridge implementation.
Allow bridging already-staked tokens on new staking contract # Background We'll be leveraging the tBTC deposit pause to deploy a new staking contract with a few features that weren't possible in the v1 release of the Keep core contracts. In particular, this staking contract will support “top-ups” (see #1893), meaning the ability to delegate additional tokens to an existing delegation relationship (i.e., an existing operator/beneficiary/authorizer triplet). This introduces a key complication, however: tokens already delegated through the current staking contract must remain locked in that contract for 60 days, the length of the undelegation period. This means that existing token owners who have used the old staking contract must wait 60 days to port those balances over to the new contract. # Proposal The core goal is for the Keep team to make an amount of KEEP tokens available to temporarily stake on behalf of a token owner, provided that they have those same tokens currently locked up in the old staking contract. Undelegation of all tokens in the new staking contract would be blocked until the temporarily-staked tokens provided by the Keep team are repaid by the token owner. The expected mode of usage for users who have already staked on the old contract is to: 1. Copy stake. 2. Undelegate from the old staking contract. 3. Operate as normal on the new staking contract. 4. Once the old staking contract's undelegated balance is available for recovery, recover it and use the recovered tokens to repay the `StakingPortBacker` contract. # Implemenation A number of KEEP tokens equivalent to the currently-staked token amount will be locked in a special `StakingPortBacker` contract. The implementation is very similar to what's been proposed in #1883 except the four major differences: - `copyStake` and repay mechanism (by `receiveApproval`) are defined on `StakingPortBacker` - Until the delegation is paid back, the owner of the new delegation is the `StakingPortBacker`. Once the delegation is paid back, the ownership of the delegation is transferred to the owner of the original staking relationship. If the delegation has not been paid back on time, the owner of `StakingPortBacker` may decide to force-undelegate from the new staking contract. - The owner of `StakingPortBacker` needs to explicitly mark which delegations are allowed to be copied. The reason for explicitly listing which relationships are allowed to use the supply is to avoid a situation when someone stakes on the old contract later, after deploying the new staking contract version, just to drain the supply from `StakingPortBacker` contract. Explicitly specifying which relationships are allowed (by calling `addAllowedOperator`) lets the contract owner to snapshot the staking state before transferring the tokens to provide additional liquidity for this snapshot. - Top-ups are not allowed until the delegation is paid back. I assume it to be an additional incentivize to pay back sooner than later. I have also gone through the grants and it does not seem to be a very severe limitation given what's currently staked and how stakeaheads were used.
Since #1893 recover/cancel stake should go directly to `TokenStaking` contract.
The idea behind a top-up is to let the staker add KEEP tokens to existing delegations. This is a two-step process: first, staker
initiates a top-up, locking new KEEPs in the staking contract. Once the initialization period is over, staker (or anyone else)
can commit the top-up, increasing operator's stake.
From the smart contract's perspective, top-ups are performed using the same interface as delegations:
receiveApprovalonTokenStakingcontract. If the given operator is active, tokens are treated as a top-up. If the given operator is not known, tokens are used to create a new delegation.Operators who have their stake undelegated cannot have their stake topped-up. It means that we still enforce operator's uniqueness - operator address cannot be reused after delegating and recovering the stake. This is important because token staking escrow assumes operator's uniqueness when accepting a token deposit. Without operator's uniqueness, bookkeeping in the escrow would become a nightmare.
For the same reason, staking contract enforces the same source of token for a top-up as used for the initial delegation. If the initial delegation was done from a grant, the same grant has to be used for a top-up to the operator. If the initial delegation was done using owner's liquid tokens, liquid tokens from the same owner are required for a top-up.
Top-up is a two-step process for gas efficiency reasons. For the random beacon group selection I see no risk - staker is always free to send any number of tickets they want, so postponing commitment of a top-up gives no advantage. I see a little risk of manipulation for a sortition pool, when waiting with committing a top-up may allow for some degree of manipulation when the seed is known. However, given that anyone can commit the top-up, I don't think we add any new issues to the existing threat model given that operator undelegation could pose a similar thread in the pool.
The following use-cases are supported: