From a9a7dcde69027583947592bd555ba1e8fbb0b681 Mon Sep 17 00:00:00 2001 From: Stephane Gosselin Date: Mon, 3 Sep 2018 11:51:51 -0400 Subject: [PATCH 1/5] add removeModule() function in ModuleRegistry.sol --- contracts/ModuleRegistry.sol | 49 ++++++++- contracts/interfaces/IModuleRegistry.sol | 9 +- test/k_module_registry.js | 123 ++++++++++++++++++++--- 3 files changed, 163 insertions(+), 18 deletions(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index 453123490..10a1c2ac6 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -19,6 +19,8 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo mapping (address => address[]) public reputation; // Mapping contain the list of addresses of Module factory for a particular type mapping (uint8 => address[]) public moduleList; + // Mapping to store the index of the moduleFacorty in the moduleList + mapping(address => uint8) public moduleListIndex; // contains the list of verified modules mapping (address => bool) public verified; // Contains the list of the available tags corresponds to the module type @@ -30,10 +32,16 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo event LogModuleRegistered(address indexed _moduleFactory, address indexed _owner); // Emit when the module get verified by the Polymath team event LogModuleVerified(address indexed _moduleFactory, bool _verified); + // Emit when a moduleFactory is removed by Polymath or moduleFactory owner + event LogModuleRemoved(address indexed _moduleFactory, address indexed _decisionMaker); constructor (address _polymathRegistry) public RegistryUpdater(_polymathRegistry) { + moduleList[1].push(address(0x0000000000000000000000000000000000000000)); + moduleList[2].push(address(0x0000000000000000000000000000000000000000)); + moduleList[3].push(address(0x0000000000000000000000000000000000000000)); + moduleList[4].push(address(0x0000000000000000000000000000000000000000)); } /** @@ -60,14 +68,47 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo function registerModule(address _moduleFactory) external whenNotPaused returns(bool) { require(registry[_moduleFactory] == 0, "Module factory should not be pre-registered"); IModuleFactory moduleFactory = IModuleFactory(_moduleFactory); - require(moduleFactory.getType() != 0, "Factory type should not equal to 0"); - registry[_moduleFactory] = moduleFactory.getType(); - moduleList[moduleFactory.getType()].push(_moduleFactory); + uint8 kind = moduleFactory.getType(); + require(kind != 0, "Factory kind should not equal to 0"); + registry[_moduleFactory] = kind; + moduleListIndex[_moduleFactory] = uint8(moduleList[kind].length); + moduleList[kind].push(_moduleFactory); reputation[_moduleFactory] = new address[](0); emit LogModuleRegistered (_moduleFactory, Ownable(_moduleFactory).owner()); return true; } + /** + * @notice Called by moduleFactory owner or registry curator to delete a moduleFactory + * @param _moduleFactory is the address of the module factory to be deleted + * @return bool + */ + function removeModule(address _moduleFactory) external whenNotPaused returns(bool) { + require(registry[_moduleFactory] != 0, "Module factory should be registered"); + require(msg.sender == Ownable(_moduleFactory).owner() || msg.sender == owner, + "msg.sender must be moduleFactory owner or registry curator"); + + uint8 index = moduleListIndex[_moduleFactory]; + require(index != 0, "ModuleFactory index is not valid"); + uint8 kind = registry[_moduleFactory]; + uint8 last = uint8(moduleList[kind].length - 1); + address temp = moduleList[kind][last]; + + // pop from array and re-order + moduleList[kind][index] = temp; + moduleListIndex[temp] = index; + delete moduleList[kind][last]; + moduleList[kind].length--; + + delete registry[_moduleFactory]; + delete reputation[_moduleFactory]; + delete verified[_moduleFactory]; + delete moduleListIndex[_moduleFactory]; + + emit LogModuleRemoved (_moduleFactory, msg.sender); + return true; + } + /** * @notice Called by Polymath to verify modules for SecurityToken to use. * @notice A module can not be used by an ST unless first approved/verified by Polymath @@ -121,7 +162,7 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo /** * @notice Use to get the reputation of the Module factory * @param _factoryAddress Ethereum contract address of the module factory - * @return address array which have the list of securityToken's uses that module factory + * @return address array which have the list of securityToken's uses that module factory */ function getReputationOfFactory(address _factoryAddress) public view returns(address[]) { return reputation[_factoryAddress]; diff --git a/contracts/interfaces/IModuleRegistry.sol b/contracts/interfaces/IModuleRegistry.sol index 820e8f502..d6378b209 100644 --- a/contracts/interfaces/IModuleRegistry.sol +++ b/contracts/interfaces/IModuleRegistry.sol @@ -17,6 +17,13 @@ interface IModuleRegistry { */ function registerModule(address _moduleFactory) external returns(bool); + /** + * @notice Called by moduleFactory owner or registry curator to delete a moduleFactory + * @param _moduleFactory is the address of the module factory to be deleted + * @return bool + */ + function removeModule(address _moduleFactory) external returns(bool); + /** * @notice Use to get all the tags releated to the functionality of the Module Factory. * @param _moduleType Type of module @@ -49,7 +56,7 @@ interface IModuleRegistry { /** * @notice Use to get the reputation of the Module factory * @param _factoryAddress Ethereum contract address of the module factory - * @return address array which have the list of securityToken's uses that module factory + * @return address array which have the list of securityToken's uses that module factory */ function getReputationOfFactory(address _factoryAddress) public view returns(address[]); diff --git a/test/k_module_registry.js b/test/k_module_registry.js index 436b9a1f0..ea95cfe01 100644 --- a/test/k_module_registry.js +++ b/test/k_module_registry.js @@ -52,7 +52,8 @@ contract('ModuleRegistry', accounts => { let I_TickerRegistry; let I_FeatureRegistry; let I_SecurityTokenRegistry; - let I_CappedSTOFactory; + let I_CappedSTOFactory1; + let I_CappedSTOFactory2; let I_STFactory; let I_SecurityToken; let I_CappedSTO; @@ -244,10 +245,10 @@ contract('ModuleRegistry', accounts => { ); - I_CappedSTOFactory = await CappedSTOFactory.new(I_PolyToken.address, 0, 0, 0, { from: account_polymath }); + I_CappedSTOFactory1 = await CappedSTOFactory.new(I_PolyToken.address, 0, 0, 0, { from: account_polymath }); assert.notEqual( - I_CappedSTOFactory.address.valueOf(), + I_CappedSTOFactory1.address.valueOf(), "0x0000000000000000000000000000000000000000", "CappedSTOFactory contract was not deployed" ); @@ -307,11 +308,11 @@ contract('ModuleRegistry', accounts => { assert.equal(tx.logs[0].args._owner, account_polymath); - tx = await I_ModuleRegistry.registerModule(I_CappedSTOFactory.address, { from: account_polymath }); + tx = await I_ModuleRegistry.registerModule(I_CappedSTOFactory1.address, { from: account_polymath }); assert.equal( tx.logs[0].args._moduleFactory, - I_CappedSTOFactory.address, + I_CappedSTOFactory1.address, "CappedSTOFactory is not registerd successfully" ); @@ -373,10 +374,10 @@ contract('ModuleRegistry', accounts => { }); it("Should successfully verify the module -- false", async() => { - let tx = await I_ModuleRegistry.verifyModule(I_CappedSTOFactory.address, false, { from: account_polymath }); + let tx = await I_ModuleRegistry.verifyModule(I_CappedSTOFactory1.address, false, { from: account_polymath }); assert.equal( tx.logs[0].args._moduleFactory, - I_CappedSTOFactory.address, + I_CappedSTOFactory1.address, "Failed in verifying the module" ); assert.equal( @@ -539,7 +540,7 @@ contract('ModuleRegistry', accounts => { let bytesSTO = web3.eth.abi.encodeFunctionCall(functionSignature, [startTime, endTime, cap, rate, fundRaiseType, account_fundsReceiver]); let errorThrown = false; try { - const tx = await I_SecurityToken.addModule(I_CappedSTOFactory.address, bytesSTO, 0, 0, { from: token_owner, gas: 60000000 }); + const tx = await I_SecurityToken.addModule(I_CappedSTOFactory1.address, bytesSTO, 0, 0, { from: token_owner, gas: 60000000 }); } catch(error) { errorThrown = true; console.log(` tx revert -> Module is un-verified`.grey); @@ -549,19 +550,19 @@ contract('ModuleRegistry', accounts => { }); it("Should successfully add the CappedSTO module. Because module is deployed by the owner of ST", async() => { - I_CappedSTOFactory = await CappedSTOFactory.new(I_PolyToken.address, 0, 0, 0, { from: token_owner }); + I_CappedSTOFactory2 = await CappedSTOFactory.new(I_PolyToken.address, 0, 0, 0, { from: token_owner }); assert.notEqual( - I_CappedSTOFactory.address.valueOf(), + I_CappedSTOFactory2.address.valueOf(), "0x0000000000000000000000000000000000000000", "CappedSTOFactory contract was not deployed" ); - let tx = await I_ModuleRegistry.registerModule(I_CappedSTOFactory.address, { from: token_owner }); + let tx = await I_ModuleRegistry.registerModule(I_CappedSTOFactory2.address, { from: token_owner }); assert.equal( tx.logs[0].args._moduleFactory, - I_CappedSTOFactory.address, + I_CappedSTOFactory2.address, "CappedSTOFactory is not registerd successfully" ); @@ -571,7 +572,7 @@ contract('ModuleRegistry', accounts => { endTime = startTime + duration.days(30); let bytesSTO = web3.eth.abi.encodeFunctionCall(functionSignature, [startTime, endTime, cap, rate, fundRaiseType, account_fundsReceiver]); - tx = await I_SecurityToken.addModule(I_CappedSTOFactory.address, bytesSTO, 0, 0, { from: token_owner, gas: 60000000 }); + tx = await I_SecurityToken.addModule(I_CappedSTOFactory2.address, bytesSTO, 0, 0, { from: token_owner, gas: 60000000 }); assert.equal(tx.logs[2].args._type, stoKey, "CappedSTO doesn't get deployed"); assert.equal( @@ -584,6 +585,102 @@ contract('ModuleRegistry', accounts => { }); + describe("Test cases for removeModule()", async() => { + + it("Should fail if msg.sender not curator or owner", async() => { + let errorThrown = false; + try { + await I_ModuleRegistry.removeModule(I_CappedSTOFactory2.address, { from: account_temp }); + } catch(error) { + errorThrown = true; + console.log(` tx revert -> Module is un-verified`.grey); + ensureException(error); + } + assert.ok(errorThrown, message); + }); + + it("Should successfully remove module and delete data if msg.sender is curator", async() => { + let snap = await takeSnapshot(); + + let sto1id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory1.address); + let sto2id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory2.address); + let sto1 = await I_ModuleRegistry.moduleList.call(3,sto1id); + let sto2 = await I_ModuleRegistry.moduleList.call(3,sto2id); + + assert.equal(sto1,I_CappedSTOFactory1.address); + assert.equal(sto2,I_CappedSTOFactory2.address); + assert.equal(3,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); + + let tx = await I_ModuleRegistry.removeModule(sto1, { from: account_polymath }); + + assert.equal(tx.logs[0].args._moduleFactory, sto1, "Event is not properly emitted for _moduleFactory"); + assert.equal(tx.logs[0].args._decisionMaker, account_polymath, "Event is not properly emitted for _decisionMaker"); + + let sto1id_end = await I_ModuleRegistry.moduleListIndex.call(sto1); + let sto2id_end = await I_ModuleRegistry.moduleListIndex.call(sto2); + let sto1_end = await I_ModuleRegistry.moduleList.call(3,sto1id_end); + let sto2_end = await I_ModuleRegistry.moduleList.call(3,sto2id_end); + + // re-ordering + assert.equal(sto2_end,sto2); + // set to zero address + assert.equal(sto1_end,'0x0000000000000000000000000000000000000000'); + // delete related data + assert.equal(await I_ModuleRegistry.registry.call(sto1), 0); + assert.equal(0,(await I_ModuleRegistry.getReputationOfFactory.call(sto1))); + assert.equal(2,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); + assert.equal(await I_ModuleRegistry.moduleListIndex.call(sto1), 0); + assert.equal(await I_ModuleRegistry.verified.call(sto1), false); + + await revertToSnapshot(snap); + }); + + it("Should successfully remove module and delete data if msg.sender is owner", async() => { + let sto1id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory1.address); + let sto2id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory2.address); + let sto1 = await I_ModuleRegistry.moduleList.call(3,sto1id); + let sto2 = await I_ModuleRegistry.moduleList.call(3,sto2id); + + assert.equal(sto1,I_CappedSTOFactory1.address); + assert.equal(sto2,I_CappedSTOFactory2.address); + assert.equal(3,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); + + let tx = await I_ModuleRegistry.removeModule(sto2, { from: token_owner }); + + assert.equal(tx.logs[0].args._moduleFactory, sto2, "Event is not properly emitted for _moduleFactory"); + assert.equal(tx.logs[0].args._decisionMaker, token_owner, "Event is not properly emitted for _decisionMaker"); + + let sto1id_end = await I_ModuleRegistry.moduleListIndex.call(sto1); + let sto2id_end = await I_ModuleRegistry.moduleListIndex.call(sto2); + let sto1_end = await I_ModuleRegistry.moduleList.call(3,sto1id_end); + let sto2_end = await I_ModuleRegistry.moduleList.call(3,sto2id_end); + + // re-ordering + assert.equal(sto2_end,'0x0000000000000000000000000000000000000000'); + // set to zero address + assert.equal(sto1_end,sto1); + // delete related data + assert.equal(await I_ModuleRegistry.registry.call(sto2), 0); + assert.equal(0,(await I_ModuleRegistry.getReputationOfFactory.call(sto2))); + assert.equal(2,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); + assert.equal(await I_ModuleRegistry.moduleListIndex.call(sto2), 0); + assert.equal(await I_ModuleRegistry.verified.call(sto2), false); + }); + + it("Should fail if module already removed", async() => { + let errorThrown = false; + try { + await I_ModuleRegistry.removeModule(I_CappedSTOFactory2.address, { from: account_polymath }); + } catch(error) { + errorThrown = true; + console.log(` tx revert -> Module is un-verified`.grey); + ensureException(error); + } + assert.ok(errorThrown, message); + }); + + }); + describe("Test cases for IRegistry functionality", async() => { describe("Test cases for reclaiming funds", async() => { From 7a341fe688dea180f03f41a49a284c64ded385c5 Mon Sep 17 00:00:00 2001 From: Stephane Gosselin Date: Mon, 3 Sep 2018 14:43:27 -0400 Subject: [PATCH 2/5] typo in comment --- contracts/ModuleRegistry.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index bea3a6f5a..73ea5cae2 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -20,7 +20,7 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo mapping (address => address[]) public reputation; // Mapping contain the list of addresses of Module factory for a particular type mapping (uint8 => address[]) public moduleList; - // Mapping to store the index of the moduleFacorty in the moduleList + // Mapping to store the index of the moduleFactory in the moduleList mapping(address => uint8) public moduleListIndex; // contains the list of verified modules mapping (address => bool) public verified; From f6942388ed75dbaa5653eba0094bf6f42e226e8c Mon Sep 17 00:00:00 2001 From: Stephane Gosselin Date: Mon, 3 Sep 2018 16:44:36 -0400 Subject: [PATCH 3/5] fix naming --- contracts/ModuleRegistry.sol | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index bea3a6f5a..78250c0c9 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -75,11 +75,11 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo function registerModule(address _moduleFactory) external whenNotPaused returns(bool) { require(registry[_moduleFactory] == 0, "Module factory should not be pre-registered"); IModuleFactory moduleFactory = IModuleFactory(_moduleFactory); - uint8 kind = moduleFactory.getType(); - require(kind != 0, "Factory kind should not equal to 0"); - registry[_moduleFactory] = kind; - moduleListIndex[_moduleFactory] = uint8(moduleList[kind].length); - moduleList[kind].push(_moduleFactory); + uint8 moduleType = moduleFactory.getType(); + require(moduleType != 0, "Factory moduleType should not equal to 0"); + registry[_moduleFactory] = moduleType; + moduleListIndex[_moduleFactory] = uint8(moduleList[moduleType].length); + moduleList[moduleType].push(_moduleFactory); reputation[_moduleFactory] = new address[](0); emit LogModuleRegistered (_moduleFactory, Ownable(_moduleFactory).owner()); return true; @@ -97,15 +97,15 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo uint8 index = moduleListIndex[_moduleFactory]; require(index != 0, "ModuleFactory index is not valid"); - uint8 kind = registry[_moduleFactory]; - uint8 last = uint8(moduleList[kind].length - 1); - address temp = moduleList[kind][last]; + uint8 moduleType = registry[_moduleFactory]; + uint8 last = uint8(moduleList[moduleType].length - 1); + address temp = moduleList[moduleType][last]; // pop from array and re-order - moduleList[kind][index] = temp; + moduleList[moduleType][index] = temp; moduleListIndex[temp] = index; - delete moduleList[kind][last]; - moduleList[kind].length--; + delete moduleList[moduleType][last]; + moduleList[moduleType].length--; delete registry[_moduleFactory]; delete reputation[_moduleFactory]; From 3570f49e3ab48581d7bf5d58eca4c3ec392d6a2c Mon Sep 17 00:00:00 2001 From: Stephane Gosselin Date: Mon, 3 Sep 2018 17:09:58 -0400 Subject: [PATCH 4/5] fix index 0 workaround --- contracts/ModuleRegistry.sol | 9 ++------ test/k_module_registry.js | 40 +++++++++++------------------------- 2 files changed, 14 insertions(+), 35 deletions(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index 40097f219..1c1fa0d3b 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -21,7 +21,7 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo // Mapping contain the list of addresses of Module factory for a particular type mapping (uint8 => address[]) public moduleList; // Mapping to store the index of the moduleFactory in the moduleList - mapping(address => uint8) public moduleListIndex; + mapping(address => uint8) private moduleListIndex; // contains the list of verified modules mapping (address => bool) public verified; // Contains the list of the available tags corresponds to the module type @@ -39,10 +39,6 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo constructor (address _polymathRegistry) public RegistryUpdater(_polymathRegistry) { - moduleList[1].push(address(0x0000000000000000000000000000000000000000)); - moduleList[2].push(address(0x0000000000000000000000000000000000000000)); - moduleList[3].push(address(0x0000000000000000000000000000000000000000)); - moduleList[4].push(address(0x0000000000000000000000000000000000000000)); } /** @@ -91,13 +87,12 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo * @return bool */ function removeModule(address _moduleFactory) external whenNotPaused returns(bool) { - require(registry[_moduleFactory] != 0, "Module factory should be registered"); require(msg.sender == Ownable(_moduleFactory).owner() || msg.sender == owner, "msg.sender must be moduleFactory owner or registry curator"); uint8 index = moduleListIndex[_moduleFactory]; - require(index != 0, "ModuleFactory index is not valid"); uint8 moduleType = registry[_moduleFactory]; + require(registry[_moduleFactory] == moduleType && moduleType != 0, "Module factory should be registered"); uint8 last = uint8(moduleList[moduleType].length - 1); address temp = moduleList[moduleType][last]; diff --git a/test/k_module_registry.js b/test/k_module_registry.js index 80515d607..ddeb209f1 100644 --- a/test/k_module_registry.js +++ b/test/k_module_registry.js @@ -629,68 +629,52 @@ contract('ModuleRegistry', accounts => { it("Should successfully remove module and delete data if msg.sender is curator", async() => { let snap = await takeSnapshot(); - let sto1id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory1.address); - let sto2id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory2.address); - let sto1 = await I_ModuleRegistry.moduleList.call(3,sto1id); - let sto2 = await I_ModuleRegistry.moduleList.call(3,sto2id); + let sto1 = await I_ModuleRegistry.moduleList.call(3,0); + let sto2 = await I_ModuleRegistry.moduleList.call(3,1); assert.equal(sto1,I_CappedSTOFactory1.address); assert.equal(sto2,I_CappedSTOFactory2.address); - assert.equal(3,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); + assert.equal((await I_ModuleRegistry.getModuleListOfType.call(3)).length, 2); let tx = await I_ModuleRegistry.removeModule(sto1, { from: account_polymath }); assert.equal(tx.logs[0].args._moduleFactory, sto1, "Event is not properly emitted for _moduleFactory"); assert.equal(tx.logs[0].args._decisionMaker, account_polymath, "Event is not properly emitted for _decisionMaker"); - let sto1id_end = await I_ModuleRegistry.moduleListIndex.call(sto1); - let sto2id_end = await I_ModuleRegistry.moduleListIndex.call(sto2); - let sto1_end = await I_ModuleRegistry.moduleList.call(3,sto1id_end); - let sto2_end = await I_ModuleRegistry.moduleList.call(3,sto2id_end); + let sto2_end = await I_ModuleRegistry.moduleList.call(3,0); // re-ordering assert.equal(sto2_end,sto2); - // set to zero address - assert.equal(sto1_end,'0x0000000000000000000000000000000000000000'); // delete related data assert.equal(await I_ModuleRegistry.registry.call(sto1), 0); - assert.equal(0,(await I_ModuleRegistry.getReputationOfFactory.call(sto1))); - assert.equal(2,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); - assert.equal(await I_ModuleRegistry.moduleListIndex.call(sto1), 0); + assert.equal(await I_ModuleRegistry.getReputationOfFactory.call(sto1), 0); + assert.equal((await I_ModuleRegistry.getModuleListOfType.call(3)).length, 1); assert.equal(await I_ModuleRegistry.verified.call(sto1), false); await revertToSnapshot(snap); }); it("Should successfully remove module and delete data if msg.sender is owner", async() => { - let sto1id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory1.address); - let sto2id = await I_ModuleRegistry.moduleListIndex.call(I_CappedSTOFactory2.address); - let sto1 = await I_ModuleRegistry.moduleList.call(3,sto1id); - let sto2 = await I_ModuleRegistry.moduleList.call(3,sto2id); + let sto1 = await I_ModuleRegistry.moduleList.call(3,0); + let sto2 = await I_ModuleRegistry.moduleList.call(3,1); assert.equal(sto1,I_CappedSTOFactory1.address); assert.equal(sto2,I_CappedSTOFactory2.address); - assert.equal(3,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); + assert.equal((await I_ModuleRegistry.getModuleListOfType.call(3)).length, 2); let tx = await I_ModuleRegistry.removeModule(sto2, { from: token_owner }); assert.equal(tx.logs[0].args._moduleFactory, sto2, "Event is not properly emitted for _moduleFactory"); assert.equal(tx.logs[0].args._decisionMaker, token_owner, "Event is not properly emitted for _decisionMaker"); - let sto1id_end = await I_ModuleRegistry.moduleListIndex.call(sto1); - let sto2id_end = await I_ModuleRegistry.moduleListIndex.call(sto2); - let sto1_end = await I_ModuleRegistry.moduleList.call(3,sto1id_end); - let sto2_end = await I_ModuleRegistry.moduleList.call(3,sto2id_end); + let sto1_end = await I_ModuleRegistry.moduleList.call(3,0); // re-ordering - assert.equal(sto2_end,'0x0000000000000000000000000000000000000000'); - // set to zero address assert.equal(sto1_end,sto1); // delete related data assert.equal(await I_ModuleRegistry.registry.call(sto2), 0); - assert.equal(0,(await I_ModuleRegistry.getReputationOfFactory.call(sto2))); - assert.equal(2,(await I_ModuleRegistry.getModuleListOfType.call(3)).length); - assert.equal(await I_ModuleRegistry.moduleListIndex.call(sto2), 0); + assert.equal(await I_ModuleRegistry.getReputationOfFactory.call(sto2), 0); + assert.equal((await I_ModuleRegistry.getModuleListOfType.call(3)).length, 1); assert.equal(await I_ModuleRegistry.verified.call(sto2), false); }); From 07385c7b4dc40e6469acf298abed10fc6217986b Mon Sep 17 00:00:00 2001 From: Stephane Gosselin Date: Mon, 3 Sep 2018 17:13:43 -0400 Subject: [PATCH 5/5] gas optimizations --- contracts/ModuleRegistry.sol | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/contracts/ModuleRegistry.sol b/contracts/ModuleRegistry.sol index 1c1fa0d3b..41c49c8a8 100644 --- a/contracts/ModuleRegistry.sol +++ b/contracts/ModuleRegistry.sol @@ -87,18 +87,21 @@ contract ModuleRegistry is IModuleRegistry, Pausable, RegistryUpdater, ReclaimTo * @return bool */ function removeModule(address _moduleFactory) external whenNotPaused returns(bool) { + uint8 moduleType = registry[_moduleFactory]; + + require(moduleType != 0, "Module factory should be registered"); require(msg.sender == Ownable(_moduleFactory).owner() || msg.sender == owner, "msg.sender must be moduleFactory owner or registry curator"); uint8 index = moduleListIndex[_moduleFactory]; - uint8 moduleType = registry[_moduleFactory]; - require(registry[_moduleFactory] == moduleType && moduleType != 0, "Module factory should be registered"); uint8 last = uint8(moduleList[moduleType].length - 1); address temp = moduleList[moduleType][last]; // pop from array and re-order - moduleList[moduleType][index] = temp; - moduleListIndex[temp] = index; + if (index != last) { + moduleList[moduleType][index] = temp; + moduleListIndex[temp] = index; + } delete moduleList[moduleType][last]; moduleList[moduleType].length--;