diff --git a/contracts/core/extensions/CoreInternal.sol b/contracts/core/extensions/CoreInternal.sol index 6d147d149..fde37c02e 100644 --- a/contracts/core/extensions/CoreInternal.sol +++ b/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 + ); } /** diff --git a/contracts/core/interfaces/ICore.sol b/contracts/core/interfaces/ICore.sol index 066919d6a..caa40c0bf 100644 --- a/contracts/core/interfaces/ICore.sol +++ b/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; diff --git a/test/contracts/core/extensions/coreInternal.spec.ts b/test/contracts/core/extensions/coreInternal.spec.ts index 1b16ffa6b..9e9900806 100644 --- a/test/contracts/core/extensions/coreInternal.spec.ts +++ b/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,10 +223,11 @@ 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(); @@ -234,70 +235,191 @@ contract('CoreInternal', accounts => { 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 { - 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 { + 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 { + 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()); + }); }); }); }); diff --git a/utils/contract_logs/core.ts b/utils/contract_logs/core.ts index 2559f1d02..4b521484f 100644 --- a/utils/contract_logs/core.ts +++ b/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, diff --git a/utils/coreWrapper.ts b/utils/coreWrapper.ts index df5ca6371..c8d0bf3d0 100644 --- a/utils/coreWrapper.ts +++ b/utils/coreWrapper.ts @@ -165,6 +165,7 @@ export class CoreWrapper { const encodedName = SetUtils.stringToBytes(name); const encodedSymbol = SetUtils.stringToBytes(symbol); + // Creates but does not register the Set with Core as enabled const truffleSetToken = await SetToken.new( factory, componentAddresses, @@ -343,6 +344,7 @@ export class CoreWrapper { const encodedName = SetUtils.stringToBytes(name); const encodedSymbol = SetUtils.stringToBytes(symbol); + // Creates and registers the Set with Core as enabled const txHash = await core.create.sendTransactionAsync( factory, componentAddresses,