Ensure serviceType cannot be re-added + Validate stake bounds on serv…

…iceType creation. Add tests to confirm SP cannot register against invalid type & delegation is still possible after serviceType removal.

eth-contracts/contracts/ServiceTypeManager.sol

@@ -62,7 +62,6 @@ contract ServiceTypeManager is InitializableV2 {
 
     // ========================================= Service Type Logic =========================================
 
-    /// @notice Add a new service type
     /**
      * @notice Add a new service type
      * @param _serviceType - type of service to add
@@ -79,6 +78,17 @@ contract ServiceTypeManager is InitializableV2 {
 
         require(msg.sender == governanceAddress, "Only callable by Governance contract");
         require(!this.serviceTypeIsValid(_serviceType), "Already known service type");
+        require(
+            _serviceTypeMax > _serviceTypeMin,
+            "Max stake must be non-zero and greater than min stake"
+        );
+
+        // Ensure serviceType cannot be re-added if it previously existed and was removed
+        // stored maxStake > 0 means it was previously added and removed
+        require(
+            serviceTypeInfo[_serviceType].maxStake == 0,
+            "Cannot re-add serviceType after it was removed."
+        );
 
         validServiceTypes.push(_serviceType);
         serviceTypeInfo[_serviceType] = ServiceTypeInfo({
@@ -114,6 +124,7 @@ contract ServiceTypeManager is InitializableV2 {
 
         // Mark as invalid
         serviceTypeInfo[_serviceType].isValid = false;
+        // Note - stake bounds are not reset so they can be checked to prevent serviceType from being re-added
     }
 
     /**

eth-contracts/test/delegateManager.test.js

@@ -1549,12 +1549,13 @@ contract('DelegateManager', async (accounts) => {
       })
     })
 
-    it('Undelegate after serviceType removal', async () => {
+    it('Delegate ops after serviceType removal', async () => {
       /**
        * Confirm initial state of serviceType and serviceProvider
        * Delegate stake to Staker
        * Remove serviceType
        * Confirm new state of serviceType and serviceProvider
+       * Confirm delegation to SP still works
        * Deregister SP of serviceType
        * Undelegate stake from Staker
        * Confirm new delegation state
@@ -1613,6 +1614,19 @@ contract('DelegateManager', async (accounts) => {
       assert.isTrue(spDetails.minAccountStake.eq(minStakeBN), 'Expected minAccountStake == dpTypeMin')
       assert.isTrue(spDetails.maxAccountStake.eq(maxStakeBN), 'Expected maxAccountStake == dpTypeMax')
 
+      // Confirm delegation to SP still works after serviceType removal
+      await token.approve(
+        stakingAddress,
+        delegationAmount,
+        { from: delegatorAccount1 }
+      )
+      await delegateManager.delegateStake(
+        stakerAccount,
+        delegationAmount,
+        { from: delegatorAccount1 }
+      )
+      const totalDelegationAmount = delegationAmount.add(delegationAmount)
+
       // Deregister SP + unstake
       await _lib.deregisterServiceProvider(
         serviceProviderFactory,
@@ -1624,10 +1638,10 @@ contract('DelegateManager', async (accounts) => {
       await time.advanceBlockTo(deregisterRequestInfo.lockupExpiryBlock)
       await serviceProviderFactory.decreaseStake({ from: stakerAccount })
 
-      // Undelegate all
+      // Undelegate total amount
       await delegateManager.requestUndelegateStake(
         stakerAccount,
-        delegationAmount,
+        totalDelegationAmount,
         { from: delegatorAccount1 }
       )
       const undelegateRequestInfo = await delegateManager.getPendingUndelegateRequest(delegatorAccount1)

eth-contracts/test/serviceProvider.test.js

@@ -21,6 +21,7 @@ const testCreatorNodeType = web3.utils.utf8ToHex('creator-node')
 const testInvalidType = web3.utils.utf8ToHex('invalid-type')
 const testEndpoint = 'https://localhost:5000'
 const testEndpoint1 = 'https://localhost:5001'
+const testEndpoint2 = 'https://localhost:5002'
 
 const MIN_STAKE_AMOUNT = 10
 const VOTING_PERIOD = 10
@@ -40,6 +41,7 @@ contract('ServiceProvider test', async (accounts) => {
   const guardianAddress = proxyDeployerAddress
   const stakerAccount = accounts[11]
   const stakerAccount2 = accounts[12]
+  const stakerAccount3 = accounts[13]
   const callValue = _lib.toBN(0)
   const cnTypeMin = _lib.audToWei(10)
   const cnTypeMax = _lib.audToWei(10000000)
@@ -351,6 +353,28 @@ contract('ServiceProvider test', async (accounts) => {
       )
     })
 
+    it('Fail to add serviceType with invalid bounds', async () => {
+      const testOtherType = web3.utils.utf8ToHex('other')
+
+      // Register with zero maxbounds fails
+      await _lib.assertRevert(
+        _lib.addServiceType(testOtherType, _lib.audToWei(4), _lib.audToWei(0), governance, guardianAddress, serviceTypeManagerProxyKey)
+        /* Cannot check revert msg bc call is made via governance */
+      )
+
+      // Register with max stake <= min stake fails
+      await _lib.assertRevert(
+        _lib.addServiceType(testOtherType, _lib.audToWei(4), _lib.audToWei(2), governance, guardianAddress, serviceTypeManagerProxyKey)
+        /* Cannot check revert msg bc call is made via governance */
+      )
+
+      // Register with zero min and maxbounds fails
+      await _lib.assertRevert(
+        _lib.addServiceType(testOtherType, _lib.audToWei(0), _lib.audToWei(0), governance, guardianAddress, serviceTypeManagerProxyKey)
+        /* Cannot check revert msg bc call is made via governance */
+      )
+    })
+
     it('Deregister endpoint and confirm transfer of staking balance to owner', async () => {
       // Confirm staking contract has correct amt
       assert.isTrue((await getStakeAmountForAccount(stakerAccount)).eq(DEFAULT_AMOUNT))
@@ -1158,13 +1182,15 @@ contract('ServiceProvider test', async (accounts) => {
       )
     })
 
-    it('Deregister SP after serviceType removal', async () => {
+    it('Operations after serviceType removal', async () => {
       /** 
        * Confirm initial state of serviceType and serviceProvider
        * Remove serviceType
        * Confirm new state of serviceType and serviceProvider
        * Deregister SP of serviceType
-       * Confirm final state of serviceType and serviceProvider
+       * Confirm new state of serviceType and serviceProvider
+       * Attempt to register new SP after serviceType removal
+       * Confirm serviceType cannot be re-added
        */
 
       const minStakeBN = _lib.toBN(dpTypeMin)
@@ -1225,6 +1251,26 @@ contract('ServiceProvider test', async (accounts) => {
       assert.isTrue(spDetails2.numberOfEndpoints.isZero(), 'Expected numberOfEndpoints == 0')
       assert.isTrue(spDetails2.minAccountStake.isZero(), 'Expected minAccountStake == 0')
       assert.isTrue(spDetails2.maxAccountStake.isZero(), 'Expected maxAccountStake == 0')
+
+      // Confirm new SP cannot be registered after serviceType removal
+      await _lib.assertRevert(
+        _lib.registerServiceProvider(
+          token,
+          staking,
+          serviceProviderFactory,
+          testDiscProvType,
+          testEndpoint2,
+          DEFAULT_AMOUNT,
+          stakerAccount3
+        ),
+        "Valid service type required"
+      )
+
+      // Confirm serviceType cannot be re-added
+      await _lib.assertRevert(
+        _lib.addServiceType(testDiscProvType, dpTypeMin, dpTypeMax, governance, guardianAddress, serviceTypeManagerProxyKey)
+        /* Cannot check revert msg bc call is made via governance */
+      )
     })
   })
 })