Separate Node storage from Node Rules (#219)

* separate node storage from rules

* updated version number of rules

Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>

Author: macfarla

README.md

@@ -40,7 +40,7 @@ ONLY use these instructions if you are doing development work on the Dapp itself
 This is the easiest way to get started for development with the permissioning Dapp:
 
 #### Compile and migrate the contracts (Development mode) ####
-1. Delete your environment variables named `NODE_INGRESS_CONTRACT_ADDRESS`, `ACCOUNT_INGRESS_CONTRACT_ADDRESS`, `ACCOUNT_STORAGE_CONTRACT_ADDRESS` AND
+1. Delete your environment variables named `NODE_INGRESS_CONTRACT_ADDRESS`, `ACCOUNT_INGRESS_CONTRACT_ADDRESS`, `ACCOUNT_STORAGE_CONTRACT_ADDRESS`, `NODE_STORAGE_CONTRACT_ADDRESS` AND
 `CHAIN_ID` - you might need to restart your terminal session to have your changes applied. If you are using a `.env` file, you can comment out the variables.
 1. Start a terminal session and start a Truffle Ganache node running `truffle develop`. This will start a Ganache node and create a Truffle console session.
 1. In the truffle console, run all migrations from scratch with `migrate --reset`. Keep this terminal session open to maintain your Ganache node running.

contracts/ExposedNodeRulesList.sol

@@ -1,11 +1,16 @@
 pragma solidity 0.5.9;
 
 import "./NodeRulesList.sol";
+import "./NodeStorage.sol";
 
 
 contract ExposedNodeRulesList is NodeRulesList {
 
-    function _calculateKey(string calldata _enodeId, string calldata _host, uint16 _port) external pure returns(uint256) {
+    function _setStorage(NodeStorage _storage) public {
+        return setStorage(_storage);
+    }
+
+    function _calculateKey(string calldata _enodeId, string calldata _host, uint16 _port) external view returns(uint256) {
         return calculateKey(_enodeId, _host, _port);
     }
 

contracts/NodeRules.sol

@@ -26,7 +26,7 @@ contract NodeRules is NodeRulesProxy, NodeRulesList {
     // this will be used to protect data when upgrading contracts
     bool private readOnlyMode = false;
     // version of this contract: semver like 1.2.14 represented like 001002014
-    uint private version = 1000000;
+    uint private version = 3000000;
 
     NodeIngress private nodeIngressContract;
 
@@ -43,7 +43,8 @@ contract NodeRules is NodeRulesProxy, NodeRulesList {
         _;
     }
 
-    constructor (NodeIngress _nodeIngressAddress) public {
+    constructor (NodeIngress _nodeIngressAddress, NodeStorage _storage) public {
+        setStorage(_storage);
         nodeIngressContract = _nodeIngressAddress;
     }
 
@@ -134,13 +135,6 @@ contract NodeRules is NodeRulesProxy, NodeRulesList {
         return size();
     }
 
-    function getByIndex(uint index) external view returns (string memory enodeId, string memory host, uint16 port) {
-        if (index >= 0 && index < size()) {
-            enode memory item = allowlist[index];
-            return (item.enodeId, item.host, item.port);
-        }
-    }
-
     function triggerRulesChangeEvent(bool addsRestrictions) public {
         nodeIngressContract.emitRulesChangeEvent(addsRestrictions);
     }

contracts/NodeRulesList.sol

@@ -1,5 +1,6 @@
 pragma solidity 0.5.9;
 
+import "./NodeStorage.sol";
 
 
 contract NodeRulesList {
@@ -11,48 +12,37 @@ contract NodeRulesList {
         uint16 port;
     }
 
-    enode[] public allowlist;
-    mapping (uint256 => uint256) private indexOf; //1-based indexing. 0 means non-existent
+    NodeStorage private nodeStorage;
 
-    function calculateKey(string memory _enodeId, string memory _host, uint16 _port) internal pure returns(uint256) {
-        return uint256(keccak256(abi.encodePacked(_enodeId, _host, _port)));
+    function setStorage(NodeStorage _storage) internal {
+        nodeStorage = _storage;
+    }
+
+    function upgradeVersion(address _newVersion) internal {
+        nodeStorage.upgradeVersion(_newVersion);
     }
 
     function size() internal view returns (uint256) {
-        return allowlist.length;
+        return nodeStorage.size();
     }
 
     function exists(string memory _enodeId, string memory _host, uint16 _port) internal view returns (bool) {
-        return indexOf[calculateKey(_enodeId, _host, _port)] != 0;
+        return nodeStorage.exists(_enodeId, _host, _port);
     }
 
     function add(string memory _enodeId, string memory _host, uint16 _port) internal returns (bool) {
-        uint256 key = calculateKey(_enodeId, _host, _port);
-        if (indexOf[key] == 0) {
-            indexOf[key] = allowlist.push(enode(_enodeId, _host, _port));
-            return true;
-        }
-        return false;
+        return nodeStorage.add(_enodeId, _host, _port);
     }
 
     function remove(string memory _enodeId, string memory _host, uint16 _port) internal returns (bool) {
-        uint256 key = calculateKey(_enodeId, _host, _port);
-        uint256 index = indexOf[key];
-
-        if (index > 0 && index <= allowlist.length) { //1 based indexing
-            //move last item into index being vacated (unless we are dealing with last index)
-            if (index != allowlist.length) {
-                enode memory lastEnode = allowlist[allowlist.length - 1];
-                allowlist[index - 1] = lastEnode;
-                indexOf[calculateKey(lastEnode.enodeId, lastEnode.host, lastEnode.port)] = index;
-            }
-
-            //shrink array
-            allowlist.length -= 1; // mythx-disable-line SWC-101
-            indexOf[key] = 0;
-            return true;
-        }
-
-        return false;
+        return nodeStorage.remove(_enodeId, _host, _port);
+    }
+
+    function calculateKey(string memory _enodeId, string memory _host, uint16 _port) public view returns(uint256) {
+        return nodeStorage.calculateKey(_enodeId, _host, _port);
+    }
+
+    function getByIndex(uint index) external view returns (string memory enodeId, string memory host, uint16 port) {
+        return nodeStorage.getByIndex(index);
     }
 }

contracts/NodeStorage.sol

@@ -0,0 +1,104 @@
+pragma solidity 0.5.9;
+
+import "./Admin.sol";
+import "./NodeIngress.sol";
+
+
+contract NodeStorage {
+    event VersionChange(
+        address oldAddress,
+        address newAddress
+    );
+    // initialize this to the deployer of this contract
+    address private latestVersion = msg.sender;
+    address private owner = msg.sender;
+
+    NodeIngress private ingressContract;
+
+
+
+    // struct size = 82 bytes
+    struct enode {
+        string enodeId;
+        string ip;
+        uint16 port;
+    }
+
+    enode[] public allowlist;
+    mapping (uint256 => uint256) private indexOf; //1-based indexing. 0 means non-existent
+
+    constructor (NodeIngress _ingressContract) public {
+        ingressContract = _ingressContract;
+    }
+
+    modifier onlyLatestVersion() {
+        require(msg.sender == latestVersion, "only the latestVersion can modify the list");
+        _;
+    }
+
+    modifier onlyAdmin() {
+        if (address(0) == address(ingressContract)) {
+            require(msg.sender == owner, "only owner permitted since ingressContract is explicitly set to zero");
+        } else {
+            address adminContractAddress = ingressContract.getContractAddress(ingressContract.ADMIN_CONTRACT());
+
+            require(adminContractAddress != address(0), "Ingress contract must have Admin contract registered");
+            require(Admin(adminContractAddress).isAuthorized(msg.sender), "Sender not authorized");
+        }
+        _;
+    }
+
+    function upgradeVersion(address _newVersion) public onlyAdmin {
+        emit VersionChange(latestVersion, _newVersion);
+        latestVersion = _newVersion;
+    }
+
+    function size() public view returns (uint256) {
+        return allowlist.length;
+    }
+
+    function exists(string memory _enodeId, string memory _ip, uint16 _port) public view returns (bool) {
+        return indexOf[calculateKey(_enodeId, _ip, _port)] != 0;
+    }
+
+    function add(string memory _enodeId, string memory _ip, uint16 _port) public returns (bool) {
+        uint256 key = calculateKey(_enodeId, _ip, _port);
+        if (indexOf[key] == 0) {
+            indexOf[key] = allowlist.push(enode(_enodeId, _ip, _port));
+            return true;
+        }
+        return false;
+    }
+
+    function remove(string memory _enodeId, string memory _ip, uint16 _port) public returns (bool) {
+        uint256 key = calculateKey(_enodeId, _ip, _port);
+        uint256 index = indexOf[key];
+
+        if (index > 0 && index <= allowlist.length) { //1 based indexing
+            //move last item into index being vacated (unless we are dealing with last index)
+            if (index != allowlist.length) {
+                enode memory lastEnode = allowlist[allowlist.length - 1];
+                allowlist[index - 1] = lastEnode;
+                indexOf[calculateKey(lastEnode.enodeId, lastEnode.ip, lastEnode.port)] = index;
+            }
+
+            //shrink array
+            allowlist.length -= 1; // mythx-disable-line SWC-101
+            indexOf[key] = 0;
+            return true;
+        }
+
+        return false;
+    }
+
+    function getByIndex(uint index) external view returns (string memory enodeId, string memory ip, uint16 port) {
+        if (index >= 0 && index < size()) {
+            enode memory item = allowlist[index];
+            return (item.enodeId, item.ip, item.port);
+        }
+    }
+
+    function calculateKey(string memory _enodeId, string memory _ip, uint16 _port) public pure returns(uint256) {
+        return uint256(keccak256(abi.encodePacked(_enodeId, _ip, _port)));
+    }
+}

migrations/3_deploy_node_ingress_rules_contract.js

@@ -1,15 +1,18 @@
 const Web3Utils = require("web3-utils");
 const AllowlistUtils = require('../scripts/allowlist_utils');
 
-const NodeRules = artifacts.require("./NodeRules.sol");
+const Rules = artifacts.require("./NodeRules.sol");
 const NodeIngress = artifacts.require("./NodeIngress.sol");
+const NodeStorage = artifacts.require("./NodeStorage.sol");
 const Admin = artifacts.require("./Admin.sol");
 
 const adminContractName = Web3Utils.utf8ToHex("administration");
 const rulesContractName = Web3Utils.utf8ToHex("rules");
 
 /* The address of the node ingress contract if pre deployed */
 let nodeIngress = process.env.NODE_INGRESS_CONTRACT_ADDRESS;
+/* The address of the node storage contract if pre deployed */
+let nodeStorage = process.env.NODE_STORAGE_CONTRACT_ADDRESS;
 let retainCurrentRulesContract = AllowlistUtils.getRetainNodeRulesContract();
 
 async function logCurrentAllowlist(instance) {
@@ -51,14 +54,29 @@ module.exports = async(deployer, network) => {
     await nodeIngressInstance.setContractAddress(adminContractName, admin.address);
     console.log("   > Updated NodeIngress with Admin  address = " + admin.address);
 
-    await deployer.deploy(NodeRules, nodeIngress);
-    console.log("   > NodeRules deployed with NodeIngress.address = " + nodeIngress);
-    let nodeRulesContract = await NodeRules.deployed();
+    // STORAGE
+    var storageInstance;
+    if (! nodeStorage) {
+        // Only deploy if we haven't been provided a pre-deployed address
+        storageInstance = await deployer.deploy(NodeStorage, nodeIngress);
+        console.log("   > Deployed NodeStorage contract to address = " + NodeStorage.address);
+        nodeStorage = NodeStorage.address;
+    } else {
+        // is there a storage already deployed
+        storageInstance = await NodeStorage.at(nodeStorage);
+        console.log(">>> Using existing NodeStorage " + storageInstance.address);
+        // TODO check that this contract is a storage contract eg call a method
+    }
 
+    // rules -> storage
+    await deployer.deploy(Rules, nodeIngress, nodeStorage);
+    console.log("   > Rules deployed with NodeIngress.address = " + nodeIngress + "\n   > and storageAddress = " + nodeStorage);
+    console.log("   > Rules.address " + Rules.address);
+    let nodeRulesContract = await Rules.deployed();
 
 
-    await nodeIngressInstance.setContractAddress(rulesContractName, NodeRules.address);
-    console.log("   > Updated NodeIngress contract with NodeRules address = " + NodeRules.address);
+    await nodeIngressInstance.setContractAddress(rulesContractName, Rules.address);
+    console.log("   > Updated NodeIngress contract with NodeRules address = " + Rules.address);
 
     if(AllowlistUtils.isInitialAllowlistedNodesAvailable()) {
         console.log("   > Adding Initial Allowlisted eNodes ...");

package.json

@@ -113,7 +113,7 @@
     ]
   },
   "description": "Smart contracts and dapp implementing EEA spec onchain permissioning",
-  "version": "2.0.2",
+  "version": "2.1.0",
   "main": "README.md",
   "directories": {
     "test": "test"

test/test-node-ingress-proxy.js

@@ -1,6 +1,7 @@
 const NodeIngress = artifacts.require('NodeIngress.sol');
 const NodeRules = artifacts.require('NodeRules.sol');
 const Admin = artifacts.require('Admin.sol');
+const RulesStorage = artifacts.require('NodeStorage.sol');
 
 const RULES='0x72756c6573000000000000000000000000000000000000000000000000000000';
 const ADMIN='0x61646d696e697374726174696f6e000000000000000000000000000000000000';
@@ -15,13 +16,22 @@ contract ('NodeIngress (proxying permissioning check to rules contract)', () =>
   let nodeIngressContract;
   let nodeRulesContract;
   let adminContract;
+  let storageContract;
 
   beforeEach(async () => {
     nodeIngressContract = await NodeIngress.new();
     adminContract = await Admin.new();
+
+    // set the storage
+    storageContract = await RulesStorage.new(nodeIngressContract.address);
+    console.log("   >>> Storage contract deployed with address = " + storageContract.address);
 
     await nodeIngressContract.setContractAddress(ADMIN, adminContract.address);
-    nodeRulesContract = await NodeRules.new(nodeIngressContract.address);
+    nodeRulesContract = await NodeRules.new(nodeIngressContract.address, storageContract.address);
+
+    // set rules as the storage owner
+    await storageContract.upgradeVersion(nodeRulesContract.address);
+    console.log("   >>> Set storage owner to Rules.address");
 
     result = await nodeIngressContract.getContractAddress(ADMIN);
     assert.equal(result, adminContract.address, 'Admin contract should be reg');
@@ -56,29 +66,46 @@ contract ('NodeIngress (proxying permissioning check to rules contract)', () =>
     assert.equal(result, result2, "Call and proxy call did NOT return the same value");
   });
 
-  it('Should permit changing active NodeRules contract addresses', async () => {
+  it('Should permit changing active NodeRules contract addresses WHILE keeping existing storage', async () => {
     let result;
-    let result2;
-
-    // const icProxy = await NodeIngress.new();
-    const rcProxy1 = await NodeRules.new(nodeIngressContract.address);
-    const rcProxy2 = await NodeRules.new(nodeIngressContract.address);
 
     // Verify that the NodeRules contract has not been registered
     result = await nodeIngressContract.getContractAddress(RULES);
     assert.equal(result, "0x0000000000000000000000000000000000000000", 'NodeRules contract should NOT already be registered');
 
     // Register the initial NodeRules contract
+    await nodeIngressContract.setContractAddress(RULES, nodeRulesContract.address);
+    // Verify the initial rules contract has been registered
+    result = await nodeIngressContract.getContractAddress(RULES);
+    assert.equal(result, nodeRulesContract.address, 'Initial contract has NOT been registered correctly');
+
+    // Add the Enode to the NodeRules register
+    result = await nodeRulesContract.addEnode(enode1, node1Host, node1Port);
+
+    // Verify that the node is permitted
+    result = await nodeIngressContract.connectionAllowed(enode1, node1Host, node1Port);
+    assert.equal(result, true, "Connection SHOULD be allowed after Enodes have been registered");
+
+    // create a NEW Rules contract
+    const rcProxy1 = await NodeRules.new(nodeIngressContract.address, storageContract.address);
+    // existing rules calls upgrade to change storage owner to the new one
+    storageContract.upgradeVersion(rcProxy1.address);
+
+    // Register the NEW NodeRules contract
     await nodeIngressContract.setContractAddress(RULES, rcProxy1.address);
 
-    // Verify the initial rules contract has been registered
+    // Verify the NEW rules contract has been registered
     result = await nodeIngressContract.getContractAddress(RULES);
-    assert.equal(result, rcProxy1.address, 'Initial contract has NOT been registered correctly');
+    assert.equal(result, rcProxy1.address, 'NEW Rules contract has NOT been registered correctly');
 
-    // Verify that the newly registered contract is the initial version
+    // Verify that the newly registered contract is the correct version
     let contract = await NodeRules.at(result);
     result = await contract.getContractVersion();
-    assert.equal(web3.utils.toDecimal(result), 1000000, 'Initial contract is NOT the correct version');
+    assert.equal(web3.utils.toDecimal(result), 3000000, 'Rules contract is NOT the correct version');
+
+    // Verify that the node is permitted
+    result = await nodeIngressContract.connectionAllowed(enode1, node1Host, node1Port);
+    assert.equal(result, true, "Connection SHOULD be allowed after Enodes have been registered");
 
   });
 });