chore(protocol-contracts): remove unstake recipient from ProtocolStaking (#1370)

* chore(protocol-contracts): remove unstake recipient from `ProtocolStaking`

* Update protocol-contracts/staking/contracts/ProtocolStaking.sol

Co-authored-by: James Toussaint <[email protected]>

* docs

* terminate early for 0 shares redemption

* Remove recipient from `TokensUnstaked` event

---------

Co-authored-by: James Toussaint <[email protected]>

Author: arr00

protocol-contracts/staking/contracts/OperatorStaking.sol

@@ -110,6 +110,7 @@ contract OperatorStaking is ERC20, Ownable, ReentrancyGuardTransient {
      * @param owner The owner of the shares.
      */
     function requestRedeem(uint208 shares, address controller, address owner) public virtual {
+        if (shares == 0) return;
         require(controller != address(0), InvalidController());
         if (msg.sender != owner) {
             _spendAllowance(owner, msg.sender, shares);
@@ -126,10 +127,7 @@ contract OperatorStaking is ERC20, Ownable, ReentrancyGuardTransient {
             );
 
         (, uint48 lastReleaseTime, uint208 controllerSharesRedeemed) = _unstakeRequests[controller].latestCheckpoint();
-        uint48 releaseTime = protocolStaking_.unstake(
-            address(this),
-            SafeCast.toUint256(SignedMath.max(assetsToWithdraw, 0))
-        );
+        uint48 releaseTime = protocolStaking_.unstake(SafeCast.toUint256(SignedMath.max(assetsToWithdraw, 0)));
         assert(releaseTime >= lastReleaseTime); // should never happen
         _unstakeRequests[controller].push(releaseTime, controllerSharesRedeemed + shares);
 

protocol-contracts/staking/contracts/ProtocolStaking.sol

@@ -35,8 +35,8 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
         uint256 _totalEligibleStakedWeight;
         // Stake - release
         uint48 _unstakeCooldownPeriod;
-        mapping(address recipient => Checkpoints.Trace208) _unstakeRequests;
-        mapping(address recipient => uint256) _released;
+        mapping(address account => Checkpoints.Trace208) _unstakeRequests;
+        mapping(address account => uint256) _released;
         // Reward - issuance curve
         uint256 _lastUpdateTimestamp;
         uint256 _lastUpdateReward;
@@ -58,7 +58,7 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
     /// @dev Emitted when tokens are staked by an account.
     event TokensStaked(address indexed account, uint256 amount);
     /// @dev Emitted when tokens are unstaked by an account.
-    event TokensUnstaked(address indexed account, address indexed recipient, uint256 amount, uint48 releaseTime);
+    event TokensUnstaked(address indexed account, uint256 amount, uint48 releaseTime);
     /// @dev Emitted when tokens are released to a recipient after the unstaking cooldown period.
     event TokensReleased(address indexed recipient, uint256 amount);
     /// @dev Emitted when rewards of an account are claimed.
@@ -70,8 +70,6 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
     /// @dev Emitted when the reward recipient of an account is updated.
     event RewardsRecipientSet(address indexed account, address indexed recipient);
 
-    /// @dev Emitted when an account unstakes to the zero address.
-    error InvalidUnstakeRecipient();
     /// @dev The account cannot be made eligible.
     error InvalidEligibleAccount(address account);
     /// @dev The tokens cannot be transferred.
@@ -117,34 +115,33 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
     }
 
     /**
-     * @dev Unstake `amount` tokens from `msg.sender`'s staked balance to `recipient`.
-     * @param recipient The recipient where unstaked tokens should be sent.
-     * @param amount The amount of tokens to unstake.
-     * @return releaseTime The timestamp when the unstaked tokens can be released.
+     * @dev Unstake `amount` tokens from `msg.sender`'s staked balance to `msg.sender`.
      *
      * NOTE: Unstaked tokens are released by calling {release} after {unstakeCooldownPeriod}.
+     * WARNING: Unstake release times are strictly increasing per account even if the cooldown period
+     * is reduced. For a given account to fully realize the reduction in cooldown period, they may need
+     * to wait up to `OLD_COOLDOWN_PERIOD - NEW_COOLDOWN_PERIOD` seconds after the cooldown period is updated.
+     *
+     * @param amount The amount of tokens to unstake.
+     * @return releaseTime The timestamp when the unstaked tokens can be released.
      */
-    function unstake(address recipient, uint256 amount) public returns (uint48) {
-        require(recipient != address(0), InvalidUnstakeRecipient());
+    function unstake(uint256 amount) public returns (uint48) {
         _burn(msg.sender, amount);
 
         ProtocolStakingStorage storage $ = _getProtocolStakingStorage();
         (, uint256 lastReleaseTime, uint256 totalRequestedToWithdraw) = $
-            ._unstakeRequests[recipient]
+            ._unstakeRequests[msg.sender]
             .latestCheckpoint();
         uint48 releaseTime = SafeCast.toUint48(Math.max(Time.timestamp() + $._unstakeCooldownPeriod, lastReleaseTime));
-        $._unstakeRequests[recipient].push(releaseTime, uint208(totalRequestedToWithdraw + amount));
+        $._unstakeRequests[msg.sender].push(releaseTime, uint208(totalRequestedToWithdraw + amount));
 
-        emit TokensUnstaked(msg.sender, recipient, amount, releaseTime);
+        emit TokensUnstaked(msg.sender, amount, releaseTime);
         return releaseTime;
     }
 
     /**
      * @dev Releases tokens requested for unstaking after the cooldown period to `account`.
      * @param account The account to release tokens to.
-     *
-     * WARNING: If this contract is upgraded to add slashing, the ability to withdraw to a
-     * different address should be reconsidered.
      */
     function release(address account) public virtual {
         ProtocolStakingStorage storage $ = _getProtocolStakingStorage();
@@ -206,7 +203,7 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
 
     /**
      * @dev Sets the {unstake} cooldown period in seconds to `unstakeCooldownPeriod`. Only callable
-     * by `MANAGER_ROLE` role.
+     * by `MANAGER_ROLE` role. See {unstake} for important notes regarding the cooldown period.
      * @param unstakeCooldownPeriod_ The new unstake cooldown period.
      */
     function setUnstakeCooldownPeriod(uint48 unstakeCooldownPeriod_) public onlyRole(MANAGER_ROLE) {

protocol-contracts/staking/test/OperatorStaking.test.ts

@@ -78,6 +78,13 @@ describe('OperatorStaking', function () {
       await expect(this.token.balanceOf(this.mock)).to.eventually.be.eq(0);
     });
 
+    it('zero redemption should terminate early', async function () {
+      await expect(this.mock.connect(this.staker1).requestRedeem(0, this.staker1, this.staker1)).to.not.emit(
+        this.mock,
+        'RedeemRequest',
+      );
+    });
+
     it('should not redeem twice', async function () {
       await this.mock.connect(this.staker2).deposit(ethers.parseEther('5'), this.staker2);
       await this.mock.connect(this.staker1).deposit(ethers.parseEther('10'), this.staker1);
@@ -306,7 +313,7 @@ describe('OperatorStaking', function () {
 
       await expect(this.mock.connect(this.staker2).requestRedeem(ethers.parseEther('2'), this.staker2, this.staker2))
         .to.emit(this.protocolStaking, 'TokensUnstaked')
-        .withArgs(this.mock, this.mock, ethers.parseEther('0.5'), anyValue);
+        .withArgs(this.mock, ethers.parseEther('0.5'), anyValue);
     });
 
     it('take excess into account on requestRedeem after slashing fully covered', async function () {
@@ -320,7 +327,7 @@ describe('OperatorStaking', function () {
 
       await expect(this.mock.connect(this.staker2).requestRedeem(ethers.parseEther('1'), this.staker2, this.staker2))
         .to.emit(this.protocolStaking, 'TokensUnstaked')
-        .withArgs(this.mock, this.mock, 0, anyValue);
+        .withArgs(this.mock, 0, anyValue);
 
       await time.increase(30);
       await expect(this.mock.maxRedeem(this.staker2)).to.eventually.eq(0);

protocol-contracts/staking/test/ProtocolStaking.test.ts

@@ -208,34 +208,18 @@ describe('Protocol Staking', function () {
 
     it('should not transfer instantly', async function () {
       await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60); // 1 minute
-      await expect(this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('50')))
+      await expect(this.mock.connect(this.staker1).unstake(ethers.parseEther('50')))
         .to.emit(this.mock, 'Transfer')
         .withArgs(this.staker1, ethers.ZeroAddress, ethers.parseEther('50'))
-        .to.not.emit(this.token, 'Transfer');
-    });
-
-    it('should be able to unstake to someone else', async function () {
-      const currentTime = BigInt(await time.latest());
-      const cooldownPeriod = BigInt(await this.mock.unstakeCooldownPeriod());
-      await expect(this.mock.connect(this.staker1).unstake(this.staker2, ethers.parseEther('50')))
         .to.emit(this.mock, 'TokensUnstaked')
-        .withArgs(this.staker1, this.staker2, ethers.parseEther('50'), currentTime + cooldownPeriod + 1n);
-      await mine();
-      await expect(this.mock.release(this.staker2))
-        .to.emit(this.token, 'Transfer')
-        .withArgs(this.mock, this.staker2, ethers.parseEther('50'));
-    });
-
-    it('should not unstake to zero address', async function () {
-      await expect(
-        this.mock.connect(this.staker1).unstake(ethers.ZeroAddress, ethers.parseEther('50')),
-      ).to.be.revertedWithCustomError(this.mock, 'InvalidUnstakeRecipient');
+        .withArgs(this.staker1, ethers.parseEther('50'), anyValue)
+        .to.not.emit(this.token, 'Transfer');
     });
 
     describe('Release', function () {
       it('should transfer after cooldown complete', async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60); // 1 minute
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('50'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('50'));
         await expect(this.mock.awaitingRelease(this.staker1)).to.eventually.eq(ethers.parseEther('50'));
 
         await timeIncreaseNoMine(60);
@@ -248,7 +232,7 @@ describe('Protocol Staking', function () {
 
       it('should only release once', async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60); // 1 minute
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('50'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('50'));
 
         await timeIncreaseNoMine(60);
 
@@ -262,18 +246,18 @@ describe('Protocol Staking', function () {
 
       it("should not release if cooldown isn't complete", async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60);
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('50'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('50'));
 
         await timeIncreaseNoMine(30);
         await expect(this.mock.release(this.staker1)).to.not.emit(this.token, 'Transfer');
       });
 
       it('should combine multiple complete withdrawals', async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60); // 1 minute
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('50'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('50'));
 
         await timeIncreaseNoMine(30);
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('50'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('50'));
         await expect(this.mock.awaitingRelease(this.staker1)).to.eventually.eq(ethers.parseEther('100'));
 
         await timeIncreaseNoMine(60);
@@ -285,13 +269,13 @@ describe('Protocol Staking', function () {
 
       it('should only release completed cooldowns in batch', async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60); // 1 minute
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('25'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('25'));
 
         await timeIncreaseNoMine(20);
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('25'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('25'));
 
         await timeIncreaseNoMine(20);
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('25'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('25'));
 
         await timeIncreaseNoMine(40);
         await expect(this.mock.release(this.staker1))
@@ -301,11 +285,11 @@ describe('Protocol Staking', function () {
 
       it('should handle decrease in cooldown period gracefully', async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(120);
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('25'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('25'));
 
         await timeIncreaseNoMine(30);
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(30);
-        await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('25'));
+        await this.mock.connect(this.staker1).unstake(ethers.parseEther('25'));
 
         // advance 30 seconds. Still need to wait another 60 seconds for the original unstake request to complete.
         await timeIncreaseNoMine(30);
@@ -323,7 +307,7 @@ describe('Protocol Staking', function () {
 
       const beforetotalStakedWeight = await this.mock.totalStakedWeight();
       const beforeStaker1Log = await this.mock.weight(await this.mock.balanceOf(this.staker1));
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('75'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('75'));
       const afterStaker1Log = await this.mock.weight(await this.mock.balanceOf(this.staker1));
       const aftertotalStakedWeight = await this.mock.totalStakedWeight();
       expect(beforetotalStakedWeight - aftertotalStakedWeight).to.equal(beforeStaker1Log - afterStaker1Log);
@@ -375,7 +359,7 @@ describe('Protocol Staking', function () {
 
       await timeIncreaseNoMine(10);
 
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
 
       // time passes (10 seconds) while no one is staked
       await timeIncreaseNoMine(10);
@@ -400,9 +384,9 @@ describe('Protocol Staking', function () {
       await timeIncreaseNoMine(10);
 
       // 3 in rewards for 1 (since 1 block at the beginning alone)
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
       // 3 in rewards for 2 (since 1 block at the end alone)
-      await this.mock.connect(this.staker2).unstake(this.staker2, ethers.parseEther('100'));
+      await this.mock.connect(this.staker2).unstake(ethers.parseEther('100'));
 
       await timeIncreaseNoMine(10);
 
@@ -431,7 +415,7 @@ describe('Protocol Staking', function () {
 
       await timeIncreaseNoMine(10);
 
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
 
       // time passes (10 seconds) while no one is staked
       await timeIncreaseNoMine(10);
@@ -456,9 +440,9 @@ describe('Protocol Staking', function () {
       await timeIncreaseNoMine(10);
 
       // 3 in rewards for 1 (since 1 block at the beginning alone)
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
       // 3 in rewards for 2 (since 1 block at the end alone)
-      await this.mock.connect(this.staker2).unstake(this.staker2, ethers.parseEther('100'));
+      await this.mock.connect(this.staker2).unstake(ethers.parseEther('100'));
 
       await timeIncreaseNoMine(10);
 
@@ -487,7 +471,7 @@ describe('Protocol Staking', function () {
 
       await timeIncreaseNoMine(10);
 
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
 
       // time passes (10 seconds) while no one is staked
       await timeIncreaseNoMine(10);
@@ -512,9 +496,9 @@ describe('Protocol Staking', function () {
       await timeIncreaseNoMine(10);
 
       // 3 in rewards for 1 (since 1 block at the beginning alone)
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
       // 3 in rewards for 2 (since 1 block at the end alone)
-      await this.mock.connect(this.staker2).unstake(this.staker2, ethers.parseEther('100'));
+      await this.mock.connect(this.staker2).unstake(ethers.parseEther('100'));
 
       await timeIncreaseNoMine(10);
 
@@ -543,7 +527,7 @@ describe('Protocol Staking', function () {
 
       await timeIncreaseNoMine(10);
 
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
 
       // time passes (10 seconds) while no one is staked
       await timeIncreaseNoMine(10);
@@ -568,9 +552,9 @@ describe('Protocol Staking', function () {
       await timeIncreaseNoMine(10);
 
       // 3 in rewards for 1 (since 1 block at the beginning alone)
-      await this.mock.connect(this.staker1).unstake(this.staker1, ethers.parseEther('100'));
+      await this.mock.connect(this.staker1).unstake(ethers.parseEther('100'));
       // 3 in rewards for 2 (since 1 block at the end alone)
-      await this.mock.connect(this.staker2).unstake(this.staker2, ethers.parseEther('100'));
+      await this.mock.connect(this.staker2).unstake(ethers.parseEther('100'));
 
       await timeIncreaseNoMine(10);
 
@@ -595,6 +579,15 @@ describe('Protocol Staking', function () {
           .withArgs(eligibleAccountRole, this.staker1, this.manager);
       });
 
+      it('should not update total staked amount if no balance', async function () {
+        await this.mock.connect(this.manager).addEligibleAccount(this.staker1);
+        await this.mock.connect(this.staker1).stake(ethers.parseEther('1'));
+
+        const weightBefore = await this.mock.totalStakedWeight();
+        await this.mock.connect(this.manager).addEligibleAccount(this.staker2);
+        await expect(this.mock.totalStakedWeight()).to.eventually.eq(weightBefore);
+      });
+
       it('should reflect in eligible account storage', async function () {
         await this.mock.connect(this.manager).addEligibleAccount(this.staker1);
         await this.mock.connect(this.manager).addEligibleAccount(this.staker2);