Straighten out bridged skill trees so they match

Author: area

contracts/colonyNetwork/ColonyNetwork.sol

@@ -156,7 +156,6 @@ contract ColonyNetwork is BasicMetaTransaction, ColonyNetworkStorage, Multicall
 
     bridgeSkillIfNotMiningChain(skillCount);
 
-    emit SkillAdded(skillCount, _parentSkillId);
     return skillCount;
   }
 
@@ -175,11 +174,17 @@ contract ColonyNetwork is BasicMetaTransaction, ColonyNetworkStorage, Multicall
       bridgeData[miningBridgeAddress].skillCreationAfter
     );
 
+    // This succeeds if not set, but we don't want to block e.g. domain creation if that's the situation we're in,
+    // and we can re-call this function to bridge later if necessary.
     (bool success, bytes memory returnData) = miningBridgeAddress.call(payload);
     require(success, "colony-network-unable-to-bridge-skill-creation");
   }
 
   function addSkillToChainTree(uint256 _parentSkillId, uint256 _skillId) private {
+    // This indicates a new root local skill bridged from another chain. We don't do anything to the tree
+    // in this scenario, other than incrementing
+    // (this mirrors the behaviour of not calling addSkill() in initialiseRootLocalSkill)
+    if (_parentSkillId != 0 && _parentSkillId << 128 == 0) { return; }
 
     Skill storage parentSkill = skills[_parentSkillId];
     // Global and local skill trees are kept separate
@@ -222,11 +227,13 @@ contract ColonyNetwork is BasicMetaTransaction, ColonyNetworkStorage, Multicall
         treeWalkingCounter += 1;
       }
     } else {
-      // Add a global skill
+      // Add a global skill. Should not be possible on a non-mining chain
       require(isMiningChain(), "colony-network-not-mining-chain");
       s.globalSkill = true;
       skills[_skillId] = s;
     }
+
+    emit SkillAdded(_skillId, _parentSkillId);
   }
 
   function addSkillFromBridge(uint256 _parentSkillId, uint256 _skillId) public always onlyMiningChain() {
@@ -238,7 +245,6 @@ contract ColonyNetwork is BasicMetaTransaction, ColonyNetworkStorage, Multicall
     if (networkSkillCounts[bridge.chainId] + 1 == _skillId){
       addSkillToChainTree(_parentSkillId, _skillId);
       networkSkillCounts[bridge.chainId] += 1;
-      emit SkillAdded(_skillId, _parentSkillId);
     } else if (networkSkillCounts[bridge.chainId] < _skillId){
       pendingSkillAdditions[bridge.chainId][_skillId] = _parentSkillId;
       // TODO: Event?
@@ -263,14 +269,11 @@ contract ColonyNetwork is BasicMetaTransaction, ColonyNetworkStorage, Multicall
 
     uint256 parentSkillId = pendingSkillAdditions[bridge.chainId][_skillId];
     require(parentSkillId != 0, "colony-network-no-such-bridged-skill");
-    if (parentSkillId > bridge.chainId << 128){
-      addSkillToChainTree(parentSkillId, _skillId);
-    }
+    addSkillToChainTree(parentSkillId, _skillId);
     networkSkillCounts[bridge.chainId] += 1;
 
     // Delete the pending addition
     delete pendingSkillAdditions[bridge.chainId][_skillId];
-    emit SkillAdded(_skillId, parentSkillId);
   }
 
   function getParentSkillId(uint _skillId, uint _parentSkillIndex) public view returns (uint256) {

test/cross-chain/cross-chain.js

@@ -541,6 +541,39 @@ contract("Cross-chain", (accounts) => {
       tx = await foreignColonyNetwork.bridgeSkillIfNotMiningChain(skillCount, { gasLimit: 1000000 });
       await checkErrorRevertEthers(tx.wait(), "colony-network-unable-to-bridge-skill-creation");
     });
+
+    it("colony root local skill structures end up the same on both chains", async () => {
+      const homeColonyRootLocalSkillId = await homeColony.getRootLocalSkill();
+      let homeColonyRootLocalSkill = await homeColonyNetwork.getSkill(homeColonyRootLocalSkillId);
+
+      const foreignColonyRootLocalSkillId = await foreignColony.getRootLocalSkill();
+      let foreignColonyRootLocalSkill = await foreignColonyNetwork.getSkill(foreignColonyRootLocalSkillId);
+
+      expect(homeColonyRootLocalSkill.nParents.toString()).to.equal(foreignColonyRootLocalSkill.nParents.toString());
+      expect(homeColonyRootLocalSkill.nChildren.toString()).to.equal(foreignColonyRootLocalSkill.nChildren.toString());
+
+      let tx = await homeColony.addLocalSkill();
+      await tx.wait();
+
+      const p = getPromiseForNextBridgedTransaction();
+      tx = await foreignColony.addLocalSkill();
+      await tx.wait();
+      await p;
+      homeColonyRootLocalSkill = await homeColonyNetwork.getSkill(homeColonyRootLocalSkillId);
+      foreignColonyRootLocalSkill = await foreignColonyNetwork.getSkill(foreignColonyRootLocalSkillId);
+
+      expect(homeColonyRootLocalSkill.nParents.toString()).to.equal(foreignColonyRootLocalSkill.nParents.toString());
+      expect(homeColonyRootLocalSkill.nChildren.toString()).to.equal(foreignColonyRootLocalSkill.nChildren.toString());
+
+      let zeroSkill = await foreignColonyNetwork.getSkill(ethers.BigNumber.from(foreignChainId).mul(ethers.BigNumber.from(2).pow(128)));
+      expect(zeroSkill.nChildren.toNumber()).to.equal(0);
+
+      zeroSkill = await homeColonyNetwork.getSkill(ethers.BigNumber.from(foreignChainId).mul(ethers.BigNumber.from(2).pow(128)));
+      expect(zeroSkill.nChildren.toNumber()).to.equal(0);
+
+      zeroSkill = await homeColonyNetwork.getSkill(0);
+      expect(zeroSkill.nChildren.toNumber()).to.equal(0);
+    });
   });
 
   describe("while earning reputation on another chain", async () => {
@@ -566,14 +599,12 @@ contract("Cross-chain", (accounts) => {
       // console.log(txDataToBeSentToAMB);
 
       // process.exit(1)
-      console.log("****asdf");
       let p = getPromiseForNextBridgedTransaction();
       // Emit reputation
       await foreignColony.emitDomainReputationReward(1, accounts[0], "0x1337");
       // See that it's bridged to the inactive log
-      console.log("asdf");
       await p;
-      console.log("asdf");
+
       const logAddress = await homeColonyNetwork.getReputationMiningCycle(false);
       const reputationMiningCycleInactive = await new ethers.Contract(logAddress, IReputationMiningCycle.abi, ethersHomeSigner);