Merge pull request JoinColony#279 from KevinLiLu/feature/249-isGlobal…

…Skill-flag-in-ColonyNetwork-getSkill

Return isGlobalSkill boolean flag in ColonyNetwork.getSkill

contracts/ColonyNetwork.sol

@@ -82,12 +82,9 @@ contract ColonyNetwork is ColonyNetworkStorage {
     return colonyVersionResolver[_version];
   }
 
-  function getSkill(uint256 _skillId) public view returns (uint256, uint256) {
-    return (skills[_skillId].nParents, skills[_skillId].nChildren);
-  }
-
-  function isGlobalSkill(uint256 _skillId) public view returns (bool) {
-    return skills[_skillId].globalSkill;
+  function getSkill(uint256 _skillId) public view returns (uint256, uint256, bool) {
+    Skill storage skill = skills[_skillId];
+    return (skill.nParents, skill.nChildren, skill.globalSkill);
   }
 
   function getReputationRootHash() public view returns (bytes32) {

contracts/ColonyNetworkStorage.sol

@@ -53,7 +53,7 @@ contract ColonyNetworkStorage is DSAuth, DSMath {
     // `true` for a global skill reused across colonies or `false` for a skill mapped to a single colony's domain
     bool globalSkill;
   }
-  // Contains all global and local skills in the network, mapping skillId to Skill. Where skillId is 1-based unique identofier
+  // Contains all global and local skills in the network, mapping skillId to Skill. Where skillId is 1-based unique identifier
   mapping (uint256 => Skill) skills;
   // Number of skills in the network, including both global and local skills
   uint256 skillCount;

contracts/ColonyStorage.sol

@@ -160,7 +160,9 @@ contract ColonyStorage is DSAuth, DSMath {
 
   modifier globalSkill(uint256 _skillId) {
     IColonyNetwork colonyNetworkContract = IColonyNetwork(colonyNetworkAddress);
-    require(colonyNetworkContract.isGlobalSkill(_skillId));
+    bool isGlobalSkill;
+    (, , isGlobalSkill) = colonyNetworkContract.getSkill(_skillId);
+    require(isGlobalSkill);
     _;
   }
 

contracts/IColonyNetwork.sol

@@ -68,12 +68,8 @@ contract IColonyNetwork {
   /// @param _skillId Id of the skill
   /// @return nParents uint256 `skill.nParents` i.e. the number of parent skills of skill with id `_skillId`
   /// @return nChildren uint256 `skill.nChildren` i.e. the number of child skills of skill with id `_skillId`
-  function getSkill(uint256 _skillId) public view returns (uint256 nParents, uint256 nChildren);
-
-  /// @notice Checks if skill with id `_skillId` is a global skill
-  /// @param _skillId Id of the skill
-  /// @return isGlobalSkill true if skill with id `_skillId` is a global skill, false otherwise
-  function isGlobalSkill(uint256 _skillId) public view returns (bool isGlobalSkill);
+  /// @return isGlobalSkill true if specified skill is a global skill, otherwise false
+  function getSkill(uint256 _skillId) public view returns (uint256 nParents, uint256 nChildren, bool isGlobalSkill);
 
   /// @notice Adds a reputation update entry to log
   /// @dev Errors if it is called by anyone but a colony or if skill with id `_skillId` does not exist or

contracts/ReputationMiningCycle.sol

@@ -593,7 +593,7 @@ contract ReputationMiningCycle is PatriciaTreeProofs, DSMath {
     // We update skills in the order children, then parents, then the skill listed in the log itself.
     // If the amount in the log is positive, then no children are being updated.
     uint nParents;
-    (nParents, ) = IColonyNetwork(colonyNetworkAddress).getSkill(logEntry.skillId);
+    (nParents, , ) = IColonyNetwork(colonyNetworkAddress).getSkill(logEntry.skillId);
     uint nChildUpdates;
     if (logEntry.amount >= 0) { // solium-disable-line no-empty-blocks, whitespace
       // Then we have no child updates to consider

test/colony-network.js

@@ -112,14 +112,14 @@ contract("ColonyNetwork", accounts => {
       assert.equal(rootGlobalSkill[0].toNumber(), 0);
       assert.equal(rootGlobalSkill[1].toNumber(), 0);
 
-      const globalSkill1 = await colonyNetwork.isGlobalSkill.call(1);
-      assert.isTrue(globalSkill1);
+      const globalSkill1 = await colonyNetwork.getSkill.call(1);
+      assert.isTrue(globalSkill1[2]);
 
-      const globalSkill2 = await colonyNetwork.isGlobalSkill.call(2);
-      assert.isFalse(globalSkill2);
+      const globalSkill2 = await colonyNetwork.getSkill.call(2);
+      assert.isFalse(globalSkill2[2]);
 
-      const localSkill1 = await colonyNetwork.isGlobalSkill.call(3);
-      assert.isFalse(localSkill1);
+      const localSkill1 = await colonyNetwork.getSkill.call(3);
+      assert.isFalse(localSkill1[2]);
 
       const rootGlobalSkillId = await colonyNetwork.getRootGlobalSkillId.call();
       assert.equal(rootGlobalSkillId, 1);
@@ -142,8 +142,8 @@ contract("ColonyNetwork", accounts => {
       assert.equal(rootLocalSkill[0].toNumber(), 0);
       assert.equal(rootLocalSkill[1].toNumber(), 0);
 
-      const isGlobal = await colonyNetwork.isGlobalSkill.call(2);
-      assert.isFalse(isGlobal);
+      const skill = await colonyNetwork.getSkill.call(2);
+      assert.isFalse(skill[2]);
 
       const { colonyAddress } = logs[0].args;
       const colony = await Colony.at(colonyAddress);

test/meta-colony.js

@@ -463,4 +463,16 @@ contract("Meta Colony", accounts => {
       await checkErrorRevert(colony.setTaskSkill(1, 3));
     });
   });
+
+  describe("when getting a skill", () => {
+    it("should return a true flag if the skill is global", async () => {
+      const globalSkill = await colonyNetwork.getSkill.call(1);
+      assert.isTrue(globalSkill[2]);
+    });
+
+    it("should return a false flag if the skill is local", async () => {
+      const localSkill = await colonyNetwork.getSkill.call(2);
+      assert.isFalse(localSkill[2]);
+    });
+  });
 });