Refactor #disableSet to #registerSet

contracts/core/extensions/CoreInternal.sol

@@ -48,6 +48,12 @@ contract CoreInternal is
         bool _status
     );
 
+    // Logs a change in the registration of a Set
+    event SetRegistrationChanged(
+        address _set,
+        bool _status
+    );
+
     // Logs when the protocol fee status has been updated
     event FeeStatusChange(
         address _sender,
@@ -102,25 +108,39 @@ contract CoreInternal is
     }
 
     /**
-     * Disable a set token in the mapping of tracked set tokens. Can only
-     * be disables by owner of Core.
+     * Add or remove a Set to the mapping and array of tracked Sets. Can
+     * only be called by owner of Core.
      *
-     * @param  _set   The address of the SetToken to disable
+     * @param  _set       The address of the Set
+     * @param  _enabled   Enable or disable the Set
      */
-    function disableSet(
-        address _set
+    function registerSet(
+        address _set,
+        bool _enabled
     )
         external
         onlyOwner
     {
-        // Verify Set was created by Core and is enabled
-        require(state.validSets[_set], "UNKNOWN_SET");
-
-        // Mark as false in validSet mapping
-        state.validSets[_set] = false;
+        // Only execute if target enabled state is opposite of current state
+        // This is to prevent arbitrary addresses from being added to validSets
+        // if they were never enabled before
+        if (_enabled != state.validSets[_set]) {
+            if (_enabled) {
+                // Add the Set to setTokens array (we know it doesn't already exist in the array)
+                state.setTokens.push(_set);
+            } else {
+                // Remove the Set from setTokens array
+                state.setTokens = state.setTokens.remove(_set);
+            }
+
+            // Mark the Set respectively in validSets mapping
+            state.validSets[_set] = _enabled;
+        }
 
-        // Find and remove from setTokens array
-        state.setTokens = state.setTokens.remove(_set);
+        emit SetRegistrationChanged(
+            _set,
+            _enabled
+        );
     }
 
     /**

contracts/core/interfaces/ICore.sol

@@ -122,12 +122,15 @@ interface ICore {
         external;
 
     /**
-     * Disable a set token in the mapping of tracked set tokens.
+     * Add or remove a Set to the mapping and array of tracked Sets. Can
+     * only be called by owner of Core.
      *
-     * @param  _set   The address of the SetToken to remove
+     * @param  _set       The address of the Set
+     * @param  _enabled   Enable or disable the Set
      */
-    function disableSet(
-        address _set
+    function registerSet(
+        address _set,
+        bool _enabled
     )
         external;
 

test/contracts/core/extensions/coreInternal.spec.ts

@@ -15,7 +15,7 @@ import {
   SetTokenContract,
   SetTokenFactoryContract,
   TransferProxyContract,
-  VaultContract
+  VaultContract,
 } from '@utils/contracts';
 import { expectRevertError } from '@utils/tokenAssertions';
 import { Blockchain } from '@utils/blockchain';
@@ -24,6 +24,7 @@ import {
   getExpectedFeeStatusChangeLog,
   ExchangeRegistered,
   FactoryRegistrationChanged,
+  SetRegistrationChanged,
 } from '@utils/contract_logs/core';
 import { CoreWrapper } from '@utils/coreWrapper';
 import { ERC20Wrapper } from '@utils/erc20Wrapper';
@@ -44,7 +45,6 @@ contract('CoreInternal', accounts => {
   const [
     ownerAccount,
     otherAccount,
-    nonSetAccount,
     protocolAccount,
     zeroExWrapperAddress,
   ] = accounts;
@@ -223,81 +223,203 @@ contract('CoreInternal', accounts => {
     });
   });
 
-  describe('#disableSet', async () => {
+  describe('#registerSet', async () => {
     let setToken: SetTokenContract;
     let subjectCaller: Address;
     let subjectSet: Address;
+    let subjectShouldEnable: boolean;
 
     beforeEach(async () => {
       vault = await coreWrapper.deployVaultAsync();
       transferProxy = await coreWrapper.deployTransferProxyAsync();
       setTokenFactory = await coreWrapper.deploySetTokenFactoryAsync(core.address);
       await coreWrapper.setDefaultStateAndAuthorizationsAsync(core, vault, transferProxy, setTokenFactory);
 
-      const components = await erc20Wrapper.deployTokensAsync(2, ownerAccount);
-      const componentAddresses = _.map(components, token => token.address);
-      const componentUnits = _.map(components, () => STANDARD_NATURAL_UNIT); // Multiple of naturalUnit
-
-      // Deploy another Set for branch coverage
-      await coreWrapper.createSetTokenAsync(
-        core,
-        setTokenFactory.address,
-        componentAddresses,
-        componentUnits,
-        STANDARD_NATURAL_UNIT,
-      );
-
-      setToken = await coreWrapper.createSetTokenAsync(
-        core,
-        setTokenFactory.address,
-        componentAddresses,
-        componentUnits,
-        STANDARD_NATURAL_UNIT,
-      );
-
       subjectCaller = ownerAccount;
-      subjectSet = setToken.address;
     });
 
-    async function subject(): Promise<string> {
-      return core.disableSet.sendTransactionAsync(
-        subjectSet,
-        { from: subjectCaller },
-      );
-    }
+    describe('when enabling a set', async () => {
+      beforeEach(async () => {
+        const components = await erc20Wrapper.deployTokensAsync(2, ownerAccount);
+        const componentAddresses = _.map(components, token => token.address);
+        const componentUnits = _.map(components, () => STANDARD_NATURAL_UNIT); // Multiple of naturalUnit
+
+        // Create and enable an arbitrary set for branch coverage
+        await coreWrapper.createSetTokenAsync(
+          core,
+          setTokenFactory.address,
+          componentAddresses,
+          componentUnits,
+          STANDARD_NATURAL_UNIT,
+        );
 
-    it('disables set token address correctly', async () => {
-      await subject();
+        // Create another set (unenabled) to use in our tests
+        setToken = await coreWrapper.deploySetTokenAsync(
+          setTokenFactory.address,
+          componentAddresses,
+          componentUnits,
+          STANDARD_NATURAL_UNIT,
+        );
 
-      const isSetValid = await core.validSets.callAsync(setToken.address);
-      expect(isSetValid).to.be.false;
-    });
+        subjectSet = setToken.address;
+        subjectShouldEnable = true;
+      });
 
-    it('removes setToken address from setTokens array', async () => {
-      await subject();
+      async function subject(): Promise<string> {
+        return core.registerSet.sendTransactionAsync(
+          subjectSet,
+          subjectShouldEnable,
+          { from: subjectCaller },
+        );
+      }
 
-      const approvedSetTokens = await core.setTokens.callAsync();
-      expect(approvedSetTokens).to.not.include(setToken.address);
-      expect(approvedSetTokens.length).to.equal(1);
-    });
+      it('starts with 1 enabled set', async () => {
+        const approvedSetTokens = await core.setTokens.callAsync();
+        expect(approvedSetTokens.length).to.equal(1);
+      });
 
-    describe('when the caller is not the owner of the contract', async () => {
-      beforeEach(async () => {
-        subjectCaller = otherAccount;
+      it('enables set address correctly', async () => {
+        await subject();
+
+        const isSetValid = await core.validSets.callAsync(setToken.address);
+        expect(isSetValid).to.be.true;
       });
 
-      it('should revert', async () => {
-        await expectRevertError(subject());
+      it('adds set address to setTokens array', async () => {
+        await subject();
+
+        const approvedSetTokens = await core.setTokens.callAsync();
+        expect(approvedSetTokens).to.include(setToken.address);
+        expect(approvedSetTokens.length).to.equal(2);
+      });
+
+      it('adds set address to setTokens array only once over multiple calls', async () => {
+        await subject();
+        await subject();
+
+        const isSetValid = await core.validSets.callAsync(setToken.address);
+        expect(isSetValid).to.be.true;
+
+        const approvedSetTokens = await core.setTokens.callAsync();
+        expect(approvedSetTokens).to.include(setToken.address);
+        expect(approvedSetTokens.length).to.equal(2);
+      });
+
+      it('emits a SetRegistrationChanged event', async () => {
+        const txHash = await subject();
+        const formattedLogs = await setTestUtils.getLogsFromTxHash(txHash);
+         const expectedLogs: Log[] = [
+          SetRegistrationChanged(
+            core.address,
+            subjectSet,
+            subjectShouldEnable,
+          ),
+        ];
+
+        await SetTestUtils.assertLogEquivalence(formattedLogs, expectedLogs);
+      });
+
+      describe('when the caller is not the owner of the contract', async () => {
+        beforeEach(async () => {
+          subjectCaller = otherAccount;
+        });
+
+        it('should revert', async () => {
+          await expectRevertError(subject());
+        });
       });
     });
 
-    describe('when the Set is not enabled or valid', async () => {
+    describe('when disabling a set', async () => {
       beforeEach(async () => {
-        subjectSet = nonSetAccount;
+        const components = await erc20Wrapper.deployTokensAsync(2, ownerAccount);
+        const componentAddresses = _.map(components, token => token.address);
+        const componentUnits = _.map(components, () => STANDARD_NATURAL_UNIT); // Multiple of naturalUnit
+
+        // Create and enable an arbitrary set for branch coverage
+        await coreWrapper.createSetTokenAsync(
+          core,
+          setTokenFactory.address,
+          componentAddresses,
+          componentUnits,
+          STANDARD_NATURAL_UNIT,
+        );
+
+        // Create and enable another set to use in our tests
+        setToken = await coreWrapper.createSetTokenAsync(
+          core,
+          setTokenFactory.address,
+          componentAddresses,
+          componentUnits,
+          STANDARD_NATURAL_UNIT,
+        );
+
+        subjectSet = setToken.address;
+        subjectShouldEnable = false;
       });
 
-      it('should revert', async () => {
-        await expectRevertError(subject());
+      async function subject(): Promise<string> {
+        return core.registerSet.sendTransactionAsync(
+          subjectSet,
+          subjectShouldEnable,
+          { from: subjectCaller },
+        );
+      }
+
+      it('starts with 2 enabled sets', async () => {
+        const approvedSetTokens = await core.setTokens.callAsync();
+        expect(approvedSetTokens.length).to.equal(2);
+      });
+
+      it('disables set address correctly', async () => {
+        await subject();
+
+        const isSetValid = await core.validSets.callAsync(setToken.address);
+        expect(isSetValid).to.be.false;
+      });
+
+      it('removes set address from setTokens array', async () => {
+        await subject();
+
+        const approvedSetTokens = await core.setTokens.callAsync();
+        expect(approvedSetTokens).to.not.include(setToken.address);
+        expect(approvedSetTokens.length).to.equal(1);
+      });
+
+      it('disables set address successfully over multiple calls', async () => {
+        await subject();
+        await subject();
+
+        const isSetValid = await core.validSets.callAsync(setToken.address);
+        expect(isSetValid).to.be.false;
+
+        const approvedSetTokens = await core.setTokens.callAsync();
+        expect(approvedSetTokens).to.not.include(setToken.address);
+        expect(approvedSetTokens.length).to.equal(1);
+      });
+
+      it('emits a SetRegistrationChanged event', async () => {
+        const txHash = await subject();
+        const formattedLogs = await setTestUtils.getLogsFromTxHash(txHash);
+         const expectedLogs: Log[] = [
+          SetRegistrationChanged(
+            core.address,
+            subjectSet,
+            subjectShouldEnable,
+          ),
+        ];
+
+        await SetTestUtils.assertLogEquivalence(formattedLogs, expectedLogs);
+      });
+
+      describe('when the caller is not the owner of the contract', async () => {
+        beforeEach(async () => {
+          subjectCaller = otherAccount;
+        });
+
+        it('should revert', async () => {
+          await expectRevertError(subject());
+        });
       });
     });
   });

utils/contract_logs/core.ts

@@ -84,6 +84,21 @@ export function ExchangeRegistered(
   };
 }
 
+export function SetRegistrationChanged(
+  _coreAddress: Address,
+  _set: Address,
+  _status: boolean,
+): Log {
+  return {
+    event: 'SetRegistrationChanged',
+    address: _coreAddress,
+    args: {
+      _set,
+      _status,
+    },
+  };
+}
+
 export function getExpectedFeeStatusChangeLog(
   _coreAddress: Address,
   _sender: Address,