[H02] Delegators can prevent service providers from deregistering endpoints

eth-contracts/contracts/DelegateManager.sol

@@ -22,6 +22,12 @@ contract DelegateManager is InitializableV2 {
     string private constant ERROR_MINIMUM_DELEGATION = (
         "DelegateManager: Minimum delegation amount required"
     );
+    string private constant ERROR_ONLY_SP_GOVERNANCE = (
+        "DelegateManager: Only callable by target SP or governance"
+    );
+    string private constant ERROR_DELEGATOR_STAKE = (
+        "DelegateManager: Delegator must be staked for SP"
+    );
 
     address private governanceAddress;
     address private stakingAddress;
@@ -40,6 +46,18 @@ contract DelegateManager is InitializableV2 {
     // Minimum amount of delegation allowed
     uint256 private minDelegationAmount;
 
+    /**
+     * Lockup duration for a remove delegator request
+     * @notice must be >= Governance.votingPeriod
+     */
+    uint256 private removeDelegatorLockupDuration;
+
+    /**
+     * Evaluation period for a remove delegator request
+     * @notice added to expiry block calculated for removeDelegatorLockupDuration
+     */
+    uint256 private removeDelegatorEvalDuration;
+
     // Staking contract ref
     ERC20Mintable private audiusToken;
 
@@ -67,6 +85,10 @@ contract DelegateManager is InitializableV2 {
     // Requester to pending undelegate request
     mapping (address => UndelegateStakeRequest) private undelegateRequests;
 
+    // Pending remove delegator requests
+    // service provider -> (delegator -> lockupExpiryBlock)
+    mapping (address => mapping (address => uint256)) private removeDelegatorRequests;
+
     event IncreaseDelegatedStake(
       address indexed _delegator,
       address indexed _serviceProvider,
@@ -120,6 +142,10 @@ contract DelegateManager is InitializableV2 {
         // Default minimum delegation amount set to 100AUD
         minDelegationAmount = 100 * 10**uint256(18);
         InitializableV2.initialize();
+
+        // TODO: Hard code legit values or take this in constructor
+        removeDelegatorLockupDuration = 100;
+        removeDelegatorEvalDuration = 10;
     }
 
     /**
@@ -218,7 +244,7 @@ contract DelegateManager is InitializableV2 {
         address delegator = msg.sender;
         require(
             _delegatorExistsForSP(delegator, _target),
-            "DelegateManager: Delegator must be staked for SP"
+            ERROR_DELEGATOR_STAKE
         );
 
         // Confirm no pending delegation request
@@ -514,7 +540,56 @@ contract DelegateManager is InitializableV2 {
     }
 
     /**
-     * @notice Allow a service provider to forcibly remove a delegator
+     * @notice Initiate forcible removal of a delegator
+     * @param _serviceProvider - address of service provider
+     * @param _delegator - address of delegator
+     */
+    function requestRemoveDelegator(address _serviceProvider, address _delegator) external {
+        _requireIsInitialized();
+
+        // TODO: Consolidate error message into const
+        require(
+            msg.sender == _serviceProvider || msg.sender == governanceAddress,
+            ERROR_ONLY_SP_GOVERNANCE
+        );
+
+        require(
+            removeDelegatorRequests[_serviceProvider][_delegator] == 0,
+            "DelegateManager: Pending remove delegator request"
+        );
+
+        require(
+            _delegatorExistsForSP(_delegator, _serviceProvider),
+            ERROR_DELEGATOR_STAKE
+        );
+
+        // Update lockup
+        removeDelegatorRequests[_serviceProvider][_delegator] = (
+            block.number + removeDelegatorLockupDuration
+        );
+    }
+
+    /**
+     * @notice Cancel pending removeDelegator request
+     * @param _serviceProvider - address of service provider
+     * @param _delegator - address of delegator
+     */
+    function cancelRemoveDelegator(address _serviceProvider, address _delegator) external {
+        require(
+            msg.sender == _serviceProvider || msg.sender == governanceAddress,
+            ERROR_ONLY_SP_GOVERNANCE
+        );
+        require(
+            removeDelegatorRequests[_serviceProvider][_delegator] != 0,
+            "DelegateManager: No pending request"
+        );
+
+        // Reset lockup expiry
+        removeDelegatorRequests[_serviceProvider][_delegator] = 0;
+    }
+
+    /**
+     * @notice Evaluate removeDelegator request
      * @param _serviceProvider - address of service provider
      * @param _delegator - address of delegator
      * @return Updated total amount delegated to the service provider by delegator
@@ -525,8 +600,27 @@ contract DelegateManager is InitializableV2 {
 
         require(
             msg.sender == _serviceProvider || msg.sender == governanceAddress,
-            "DelegateManager: Only callable by target SP or governance"
+            ERROR_ONLY_SP_GOVERNANCE
+        );
+
+        // TODO: Evaluate
+        require(
+            removeDelegatorRequests[_serviceProvider][_delegator] != 0,
+            "DelegateManager: No pending request"
         );
+
+        // Enforce lockup expiry block
+        require(
+            block.number >= removeDelegatorRequests[_serviceProvider][_delegator],
+            "DelegateManager: Lockup must be expired"
+        );
+
+        // Enforce evaluation window for request
+        require(
+            block.number < removeDelegatorRequests[_serviceProvider][_delegator] + removeDelegatorEvalDuration,
+            "DelegateManager: RemoveDelegator evaluation window expired"
+        );
+
         uint256 unstakeAmount = delegateInfo[_delegator][_serviceProvider];
         // Unstake on behalf of target service provider
         Staking(stakingAddress).undelegateStakeFor(
@@ -558,6 +652,9 @@ contract DelegateManager is InitializableV2 {
 
         // Remove from list of delegators
         _removeFromDelegatorsList(_serviceProvider, _delegator);
+
+        // Reset lockup expiry
+        removeDelegatorRequests[_serviceProvider][_delegator] = 0;
     }
 
     /**
@@ -599,6 +696,28 @@ contract DelegateManager is InitializableV2 {
         emit MinDelegationUpdated(_minDelegationAmount);
     }
 
+    /**
+     * @notice Update remove delegator lockup duration
+     * @param _duration - new lockup duration
+     */
+    function updateRemoveDelegatorLockupDuration(uint256 _duration) external {
+        _requireIsInitialized();
+
+        require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE);
+        removeDelegatorLockupDuration = _duration;
+    }
+
+    /**
+     * @notice Update remove delegator evaluation window duration
+     * @param _duration - new window duration
+     */
+    function updateRemoveDelegatorEvalDuration(uint256 _duration) external {
+        _requireIsInitialized();
+
+        require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE);
+        removeDelegatorEvalDuration = _duration;
+    }
+
     /**
      * @notice Set the Governance address
      * @dev Only callable by Governance address
@@ -706,6 +825,22 @@ contract DelegateManager is InitializableV2 {
         return (req.serviceProvider, req.amount, req.lockupExpiryBlock);
     }
 
+    /**
+     * @notice Get status of pending remove delegator request for a given address
+     * @param _serviceProvider - address of the service provider
+     * @param _delegator - address of the delegator
+     * @return - current lockup expiry block for remove delegator request
+     */
+    function getPendingRemoveDelegatorRequest(
+        address _serviceProvider,
+        address _delegator
+    ) external view returns (uint256)
+    {
+        _requireIsInitialized();
+
+        return removeDelegatorRequests[_serviceProvider][_delegator];
+    }
+
     /// @notice Get current undelegate lockup duration
     function getUndelegateLockupDuration()
     external view returns (uint256)
@@ -733,6 +868,24 @@ contract DelegateManager is InitializableV2 {
         return minDelegationAmount;
     }
 
+    /// @notice Get the duration for remove delegator request lockup
+    function getRemoveDelegatorLockupDuration()
+    external view returns (uint256)
+    {
+        _requireIsInitialized();
+
+        return removeDelegatorLockupDuration;
+    }
+
+    /// @notice Get the duration for evaluation of remove delegator operations
+    function getRemoveDelegatorEvalDuration()
+    external view returns (uint256)
+    {
+        _requireIsInitialized();
+
+        return removeDelegatorEvalDuration;
+    }
+
     /// @notice Get the Governance address
     function getGovernanceAddress() external view returns (address) {
         _requireIsInitialized();

eth-contracts/test/delegateManager.test.js

@@ -1712,14 +1712,27 @@ contract('DelegateManager', async (accounts) => {
         'Expect pending request'
       )
 
-      // fail to removeDelegator from not a SP or governance
+      // Fail to call removeDelegator from not a SP or governance
       await _lib.assertRevert(
         delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: delegatorAccount1 }),
         "Only callable by target SP or governance"
       )
 
-      // Forcibly remove the delegator from service provider account
+      // Confirm failure without a pending request
+      await _lib.assertRevert(
+        delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount }),
+        "No pending request"
+      )
+
+      // Remove delegator
+      await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount })
+
+      let requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1)
+
+      // Move to valid block and actually perform remove
+      await time.advanceBlockTo(requestTargetBlock)
       await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount })
+
       let stakeAfterRemoval = await delegateManager.getDelegatorStakeForServiceProvider(delegatorAccount1, stakerAccount)
       let delegatorsList = await delegateManager.getDelegatorsList(stakerAccount)
       pendingUndelegateRequest = await delegateManager.getPendingUndelegateRequest(delegatorAccount1)
@@ -1768,14 +1781,19 @@ contract('DelegateManager', async (accounts) => {
       )
     })
 
-    it('Deregister delegator', async () => {
+    // Validate behavior around removeDelegator
+    //       - expiry block calculated correctly (done)
+    //       - cancelRemoveDelegator behavior resets request (done)
+    //       - evaluation window enforced (done) 
+    //       - invalid delegator for this sp during call to removeDelegator (done)
+    //       - Pending request before call to requestRemoveDelegator
+    it('removeDelegator validation', async () => {
       const delegationAmount = _lib.toBN(100)
+      const delegatorAccount2 = accounts[5]
       // Transfer tokens to delegator
       await token.transfer(delegatorAccount1, delegationAmount, { from: proxyDeployerAddress })
-      await token.approve(
-        stakingAddress,
-        delegationAmount,
-        { from: delegatorAccount1 })
+      await token.transfer(delegatorAccount2, delegationAmount, { from: proxyDeployerAddress })
+      await token.approve(stakingAddress, delegationAmount, { from: delegatorAccount1 })
       await delegateManager.delegateStake(
         stakerAccount,
         delegationAmount,
@@ -1788,9 +1806,26 @@ contract('DelegateManager', async (accounts) => {
         delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: delegatorAccount1 }),
         "Only callable by target SP or governance"
       )
-
-      // Forcibly remove the delegator from service provider account
+
+      let removeReqDuration = await delegateManager.getRemoveDelegatorLockupDuration()
+      let removeReqEvalDuration = await delegateManager.getRemoveDelegatorEvalDuration()
+
+      // Remove delegator
+      let tx = await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount })
+      let blocknumber = _lib.toBN(tx.receipt.blockNumber)
+      let expectedTarget = blocknumber.add(removeReqDuration)
+
+      let requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1)
+      assert.isTrue(requestTargetBlock.eq(expectedTarget), 'Target unexpected')
+
+      // Move to valid block and actually perform remove
+      await time.advanceBlockTo(requestTargetBlock)
       await delegateManager.removeDelegator(stakerAccount, delegatorAccount1, { from: stakerAccount })
+
+      requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount1)
+      assert.isTrue(requestTargetBlock.eq(_lib.toBN(0)), 'Reset expected')
+
+      // Forcibly remove the delegator from service provider account
       let stakeAfterRemoval = await delegateManager.getDelegatorStakeForServiceProvider(delegatorAccount1, stakerAccount)
       let delegatorsList = await delegateManager.getDelegatorsList(stakerAccount)
       assert.isTrue(stakeAfterRemoval.eq(_lib.toBN(0)), 'Expect 0 delegated stake')
@@ -1799,6 +1834,61 @@ contract('DelegateManager', async (accounts) => {
       let delegatorTokenBalance2 = await token.balanceOf(delegatorAccount1)
       let diff = delegatorTokenBalance2.sub(delegatorTokenBalance)
       assert.isTrue(diff.eq(delegationAmount), 'Expect full delegation amount to be refunded')
+
+      // Try to remove a delegator that does not yet exist, confirm failure
+      await _lib.assertRevert(
+        delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }),
+        'Delegator must be staked for SP'
+      )
+
+      // Delegate from a new account
+      await token.approve(
+        stakingAddress,
+        delegationAmount,
+        { from: delegatorAccount2 })
+      await delegateManager.delegateStake(
+        stakerAccount,
+        delegationAmount,
+        { from: delegatorAccount2 })
+
+      // Request removal
+      tx = await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount })
+      blocknumber = _lib.toBN(tx.receipt.blockNumber)
+      expectedTarget = blocknumber.add(removeReqDuration)
+
+      requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2)
+      assert.isTrue(requestTargetBlock.eq(expectedTarget), 'Target unexpected')
+
+      // Call from wrong account
+      await _lib.assertRevert(
+        delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2),
+        'Only callable by target SP'
+      )
+
+      // Cancel and validate request
+      await delegateManager.cancelRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount })
+      let requestTargetBlockAfterCancel = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2)
+      assert.isTrue(requestTargetBlockAfterCancel.eq(_lib.toBN(0)), 'Expect reset')
+
+      // Reissue request
+      await delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount })
+      requestTargetBlock = await delegateManager.getPendingRemoveDelegatorRequest(stakerAccount, delegatorAccount2)
+      let evalBlock = requestTargetBlock.add(removeReqEvalDuration)
+
+      // Progress to the evaluation block
+      await time.advanceBlockTo(evalBlock)
+
+      // Confirm rejection after window
+      await _lib.assertRevert(
+        delegateManager.removeDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }),
+        'RemoveDelegator evaluation window expired'
+       )
+
+      // Retry should fail here as the request has not been cancelled yet, but the window has expired
+      await _lib.assertRevert(
+        delegateManager.requestRemoveDelegator(stakerAccount, delegatorAccount2, { from: stakerAccount }),
+        'Pending remove delegator request'
+      )
     })
 
     describe('Service provider decrease stake behavior', async () => {