Merge pull request #62 from etherisc/bugfix/registry_deregister

Refactor _contractNames to use EnumberableSet

.vscode/settings.json

@@ -8,7 +8,7 @@
     "solidity.remappingsUnix": [
         "@openzeppelin/=/home/vscode/.brownie/packages/OpenZeppelin/openzeppelin-contracts@4.7.0",
         "@chainlink/=/home/vscode/.brownie/packages/smartcontractkit/chainlink@1.6.0",
-        "@etherisc/gif-interface/=/home/vscode/.brownie/packages/etherisc/gif-interface@597a29c",
+        "@etherisc/gif-interface/=/home/vscode/.brownie/packages/etherisc/gif-interface@2d633da",
     ],
     "peacock.remoteColor": "1D3C43",
     "workbench.colorCustomizations": {

brownie-config.yaml

@@ -24,7 +24,7 @@ compiler:
         remappings:
             - "@openzeppelin=OpenZeppelin/openzeppelin-contracts@4.7.0"
             - "@chainlink=smartcontractkit/chainlink@1.6.0"
-            - "@etherisc/gif-interface=etherisc/gif-interface@597a29c"
+            - "@etherisc/gif-interface=etherisc/gif-interface@2d633da"
 
 # packages below will be added to brownie
 # you may use 'brownie pm list' after 'brownie compile' 
@@ -34,7 +34,7 @@ dependencies:
     # github dependency format: <owner>/<repository>@<release>
     - OpenZeppelin/openzeppelin-contracts@4.7.0
     - smartcontractkit/chainlink@1.6.0
-    - etherisc/gif-interface@597a29c
+    - etherisc/gif-interface@2d633da
 
 # exclude open zeppeling contracts  when calculating test coverage
 # https://eth-brownie.readthedocs.io/en/v1.10.3/config.html#exclude_paths

contracts/modules/RegistryController.sol

@@ -7,11 +7,15 @@ import "@etherisc/gif-interface/contracts/modules/IRegistry.sol";
 
 import "@openzeppelin/contracts/proxy/utils/Initializable.sol";
 import "@openzeppelin/contracts/utils/Strings.sol";
+import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
+
 
 contract RegistryController is
     IRegistry,
     CoreController
 {
+    using EnumerableSet for EnumerableSet.Bytes32Set;
+
     /**
      * @dev  Save number of items to iterate through
      * Currently we have < 20 contracts.
@@ -27,9 +31,8 @@ contract RegistryController is
     uint256 public startBlock;
 
     mapping(bytes32 /* release */ => mapping(bytes32 /* contract name */ => address /* contract address */)) public _contracts;
-    mapping(bytes32 /* release */ => bytes32[] /* contract names */) public _contractNames;
     mapping(bytes32 /* release */ => uint256 /* number of contracts in release */) public _contractsInRelease;
-
+    mapping(bytes32 /* release */ => EnumerableSet.Bytes32Set /* contract names */) private _contractNames;
 
     function initializeRegistry(bytes32 _initialRelease) public initializer {
         // _setupRegistry(address(this));
@@ -41,7 +44,7 @@ contract RegistryController is
         // registry proxy.
         release = _initialRelease;
         _contracts[release]["InstanceOperatorService"] = _msgSender();
-        _contractNames[release].push("InstanceOperatorService");
+        EnumerableSet.add(_contractNames[release], "InstanceOperatorService");
         _contractsInRelease[release] = 1;
 
 
@@ -140,11 +143,11 @@ contract RegistryController is
 
         // TODO think about how to avoid this loop
         for (uint256 i = 0; i < countContracts; i += 1) {
-            bytes32 contractName = _contractNames[release][i];
+            bytes32 name = EnumerableSet.at(_contractNames[release], i);
             _registerInRelease(
                 _newRelease,
-                contractName,
-                _contracts[release][contractName]
+                name,
+                _contracts[release][name]
             );
         }
 
@@ -154,11 +157,11 @@ contract RegistryController is
     }
 
     function contracts() external override view returns (uint256 _numberOfContracts) {
-        _numberOfContracts = _contractNames[release].length;
+        _numberOfContracts = EnumerableSet.length(_contractNames[release]);
     }
 
-    function contractNames() external override view returns (bytes32[] memory _contractNamesOut) {
-        _contractNamesOut = _contractNames[release];
+    function contractName(uint256 idx) external override view returns (bytes32 _contractName) {
+        _contractName = EnumerableSet.at(_contractNames[release], idx);
     }
 
     /**
@@ -184,19 +187,19 @@ contract RegistryController is
         bool isNew = false;
 
         require(
-            _contractNames[_release].length < MAX_CONTRACTS,
+            EnumerableSet.length(_contractNames[_release]) < MAX_CONTRACTS,
             "ERROR:REC-005:MAX_CONTRACTS_LIMIT"
         );
 
         if (_contracts[_release][_contractName] == address(0)) {
-            _contractNames[_release].push(_contractName);
+            EnumerableSet.add(_contractNames[_release], _contractName);
             _contractsInRelease[_release]++;
             isNew = true;
         }
 
         _contracts[_release][_contractName] = _contractAddress;
         require(
-            _contractsInRelease[_release] == _contractNames[_release].length,
+            _contractsInRelease[_release] == EnumerableSet.length(_contractNames[_release]),
             "ERROR:REC-006:CONTRACT_NUMBER_MISMATCH"
         );
 
@@ -216,30 +219,16 @@ contract RegistryController is
         internal
         onlyInstanceOperator
     {
-        uint256 indexToDelete;
-        uint256 countContracts = _contractNames[_release].length;
-
-        // TODO think about how to avoid this loop
-        for (uint256 i = 0; i < countContracts; i += 1) {
-            if (_contractNames[_release][i] == _contractName) {
-                indexToDelete = i;
-                break;
-            }
-        }
+        require(EnumerableSet.contains(_contractNames[_release], _contractName), "ERROR:REC-009:CONTRACT_UNKNOWN");
 
-        if (indexToDelete < countContracts - 1) {
-            _contractNames[_release][indexToDelete] = _contractNames[_release][
-                countContracts - 1
-            ];
-        }
+        EnumerableSet.remove(_contractNames[_release], _contractName);
 
-        _contractNames[_release].pop();
         _contractsInRelease[_release] -= 1;
+        delete _contracts[_release][_contractName];
+
         require(
-            _contractsInRelease[_release] == _contractNames[_release].length,
-            "ERROR:REC-010:CONTRACT_NUMBER_MISMATCH"
-        );
-
-        emit LogContractDeregistered(_release, _contractName);
+            _contractsInRelease[_release] == EnumerableSet.length(_contractNames[_release]),
+            "ERROR:REC-010:CONTRACT_NUMBER_MISMATCH");
+        emit LogContractDeregistered(_release, _contractName);            
     }
 }

contracts/services/InstanceService.sol

@@ -123,8 +123,8 @@ contract InstanceService is
         numberOfContracts = _registry.contracts();
     }
 
-    function contractNames() external view override returns (bytes32[] memory names) {
-        names = _registry.contractNames();
+    function contractName(uint256 idx) external view override returns (bytes32 name) {
+        name = _registry.contractName(idx);
     }
 
     /* access */

tests/test_registry.py

@@ -14,6 +14,7 @@
     INSTANCE_OPERATOR_SERVICE_NAME,
     REGISTRY_CONTROLLER_NAME,
     REGISTRY_NAME,
+    ZERO_ADDRESS,
 )
 
 from scripts.util import (
@@ -59,7 +60,7 @@ def test_registry_max_coponents(registry, owner):
 
     assert 100 == registry.contracts()
 
-    namesFromRegistry = registry.contractNames()
+    namesFromRegistry = get_contract_names(registry)
 
     # ignore first three elements 
     for i in range(len(namesFromRegistry[3:])):
@@ -70,3 +71,38 @@ def test_registry_max_coponents(registry, owner):
         tx = TestCoin.deploy({'from': owner})
         registry.register(s2b32("OneTooMany"), tx, {'from': owner})
 
+
+def test_registry_deregister(registry, owner):
+    assert GIF_RELEASE == b322s(registry.getRelease({'from': owner}))
+
+    name1 = s2b32("TestCoin1")
+    name2 = s2b32("TestCoin2")
+    name3 = s2b32("TestCoin3")
+
+    tx1 = TestCoin.deploy({'from': owner})
+    tx = registry.register(name1, tx1, {'from': owner})
+
+    tx2 = TestCoin.deploy({'from': owner})
+    tx = registry.register(name2, tx2, {'from': owner})
+
+    assert tx1 == registry.getContract(name1)
+    assert tx2 == registry.getContract(name2)
+
+    with brownie.reverts("ERROR:REC-009:CONTRACT_UNKNOWN"):
+        tx = registry.deregister(name3, {'from': owner})
+
+    assert tx1 == registry.getContract(name1)
+    assert tx2 == registry.getContract(name2)
+
+    tx = registry.deregister(name1, {'from': owner})
+    print(tx.info())
+
+    assert ZERO_ADDRESS == registry.getContract(name1)
+    assert tx2 == registry.getContract(name2)
+
+
+def get_contract_names(registry):
+    contract_names = []
+    for i in range(registry.contracts()):
+        contract_names.append(registry.contractName(i))
+    return contract_names