Skip to content

Commit

Permalink
Merge pull request JoinColony#268 from JoinColony/fix/266-add-domain
Browse files Browse the repository at this point in the history 
Improve adding domains
  • Loading branch information
@elenadimitrova
elenadimitrova authored Jul 10, 2018
2 parents 2a54255 + 965ce4d commit 2e5a28d
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 60 deletions.
15 changes: 7 additions & 8 deletions contracts/Colony.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,19 +117,18 @@ contract Colony is ColonyStorage, PatriciaTreeProofs {
return colonyNetwork.addSkill(_parentSkillId, true);
}

function addDomain(uint256 _parentSkillId) public
function addDomain(uint256 _parentDomainId) public
domainExists(_parentDomainId)
auth
localSkill(_parentSkillId)
{
// Note: remove that when we start allowing more domain hierarchy levels
// Instead check that the parent skill id belongs to this colony own domain
// Get the local skill id of the root domain
uint256 rootDomainSkillId = domains[1].skillId;
require(_parentSkillId == rootDomainSkillId, "colony-parent-skill-not-root");
// Note: Remove when we want to allow more domain hierarchy levels
require(_parentDomainId == 1, "colony-parent-skill-not-root");

uint256 parentSkillId = domains[_parentDomainId].skillId;

// Setup new local skill
IColonyNetwork colonyNetwork = IColonyNetwork(colonyNetworkAddress);
uint256 newLocalSkill = colonyNetwork.addSkill(_parentSkillId, false);
uint256 newLocalSkill = colonyNetwork.addSkill(parentSkillId, false);

// Add domain to local mapping
initialiseDomain(newLocalSkill);
Expand Down
6 changes: 0 additions & 6 deletions contracts/ColonyStorage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,6 @@ contract ColonyStorage is DSAuth, DSMath {
_;
}

modifier localSkill(uint256 _skillId) {
IColonyNetwork colonyNetworkContract = IColonyNetwork(colonyNetworkAddress);
require(!colonyNetworkContract.isGlobalSkill(_skillId));
_;
}

modifier skillExists(uint256 _skillId) {
IColonyNetwork colonyNetworkContract = IColonyNetwork(colonyNetworkAddress);
require(_skillId <= colonyNetworkContract.getSkillCount());
Expand Down
5 changes: 3 additions & 2 deletions contracts/IColony.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,9 @@ contract IColony {

/// @notice Add a colony domain, and its respective local skill under skill with id `_parentSkillId`
/// New funding pot is created and associated with the domain here
/// @param _parentSkillId Id of the local skill under which the new skill will be added
function addDomain(uint256 _parentSkillId) public;
/// @param _parentDomainId Id of the domain under which the new one will be added
/// @dev Adding new domains is currently retricted to one level only, i.e. `_parentDomainId` has to be the root domain id: 1
function addDomain(uint256 _parentDomainId) public;

/// @notice Get the domain's local skill and funding pot id
/// @param _id Id of the domain which details to get
Expand Down
12 changes: 3 additions & 9 deletions test/colony.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,7 @@ contract("Colony", addresses => {

describe("when adding domains", () => {
it("should log DomainAdded and PotAdded events", async () => {
const skillCount = await colonyNetwork.getSkillCount.call();
await expectAllEvents(colony.addDomain(skillCount.toNumber()), ["DomainAdded", "PotAdded"]);
await expectAllEvents(colony.addDomain(1), ["DomainAdded", "PotAdded"]);
});
});

Expand Down Expand Up @@ -304,8 +303,7 @@ contract("Colony", addresses => {
});

it("should set the task domain correctly", async () => {
const skillCount = await colonyNetwork.getSkillCount.call();
await colony.addDomain(skillCount.toNumber());
await colony.addDomain(1);
await makeTask({ colony, domainId: 2 });
const task = await colony.getTask.call(1);
assert.equal(task[8].toNumber(), 2);
Expand Down Expand Up @@ -1247,11 +1245,7 @@ contract("Colony", addresses => {

it("should log a TaskDomainChanged event, if the task domain gets changed", async () => {
const taskId = await makeTask({ colony });

// Create a domain, change task's domain
const skillCount = await colonyNetwork.getSkillCount.call();
await colony.addDomain(skillCount.toNumber());

await colony.addDomain(1);
await expectEvent(colony.setTaskDomain(taskId, 2), "TaskDomainChanged");
});

Expand Down
52 changes: 17 additions & 35 deletions test/meta-colony.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,8 @@ contract("Meta Colony", accounts => {

describe("when adding domains in the meta colony", () => {
it("should be able to add new domains as children to the root domain", async () => {
await metaColony.addDomain(2);
await metaColony.addDomain(1);

const skillCount = await colonyNetwork.getSkillCount.call();
assert.equal(skillCount.toNumber(), 4);
const domainCount = await metaColony.getDomainCount.call();
Expand All @@ -282,9 +283,9 @@ contract("Meta Colony", accounts => {
assert.equal(rootSkillChild.toNumber(), 4);
});

it("should NOT be able to add a child local skill more than one level from the root local skill", async () => {
await metaColony.addDomain(2);
await checkErrorRevert(metaColony.addDomain(4));
it("should NOT be able to add a child domain more than one level away from the root domain", async () => {
await metaColony.addDomain(1);
await checkErrorRevert(metaColony.addDomain(2));

const skillCount = await colonyNetwork.getSkillCount.call();
assert.equal(skillCount.toNumber(), 4);
Expand All @@ -305,13 +306,13 @@ contract("Meta Colony", accounts => {
});

it("someone who does not have owner role should not be able to add domains", async () => {
await checkErrorRevert(colony.addDomain(3, { from: OTHER_ACCOUNT }));
await checkErrorRevert(colony.addDomain(1, { from: OTHER_ACCOUNT }));
});

it("should be able to add new domains as children to the root domain", async () => {
await colony.addDomain(4);
await colony.addDomain(4);
await colony.addDomain(4);
await colony.addDomain(1);
await colony.addDomain(1);
await colony.addDomain(1);

const skillCount = await colonyNetwork.getSkillCount.call();
assert.equal(skillCount.toNumber(), 7);
Expand Down Expand Up @@ -342,25 +343,6 @@ contract("Meta Colony", accounts => {
assert.equal(rootSkillChild3.toNumber(), 7);
});

it("should NOT be able to add a child local skill more than one level from the root local skill", async () => {
await colony.addDomain(4);
await checkErrorRevert(colony.addDomain(5));

const skillCount = await colonyNetwork.getSkillCount.call();
assert.equal(skillCount.toNumber(), 5);
const domainCount = await colony.getDomainCount.call();
assert.equal(domainCount.toNumber(), 2);
});

it("should NOT be able to add a child local skill to a global skill parent", async () => {
await checkErrorRevert(colony.addDomain(1));

const skillCount = await colonyNetwork.getSkillCount.call();
assert.equal(skillCount.toNumber(), 4);
const domainCount = await colony.getDomainCount.call();
assert.equal(domainCount.toNumber(), 1);
});

it("should NOT be able to add a new local skill by anyone but a Colony", async () => {
await checkErrorRevert(colonyNetwork.addSkill(2, false));

Expand All @@ -369,10 +351,13 @@ contract("Meta Colony", accounts => {
});

it("should NOT be able to add a new root local skill", async () => {
await checkErrorRevert(colonyNetwork.addSkill(0, false));
const skillCountBefore = await colonyNetwork.getSkillCount.call();
const rootDomain = await colony.getDomain(1);
const rootLocalSkillId = rootDomain[0].toNumber();
await checkErrorRevert(colonyNetwork.addSkill(rootLocalSkillId, false));
const skillCountAfter = await colonyNetwork.getSkillCount.call();

const skillCount = await colonyNetwork.getSkillCount.call();
assert.equal(skillCount.toNumber(), 4);
assert.equal(skillCountBefore.toNumber(), skillCountAfter.toNumber());
});
});

Expand All @@ -387,8 +372,7 @@ contract("Meta Colony", accounts => {
});

it("should be able to set domain on task", async () => {
const colonyRootDomain = await colony.getDomain(1);
await colony.addDomain(colonyRootDomain[0].toString());
await colony.addDomain(1);
const taskId = await makeTask({ colony });

await colony.setTaskDomain(taskId, 2);
Expand All @@ -398,9 +382,7 @@ contract("Meta Colony", accounts => {
});

it("should NOT allow a non-manager to set domain on task", async () => {
const colonyRootDomain = await colony.getDomain(1);
await colony.addDomain(colonyRootDomain[0].toString());

await colony.addDomain(1);
await makeTask({ colony });
await checkErrorRevert(colony.setTaskDomain(1, 2, { from: OTHER_ACCOUNT }));
const task = await colony.getTask.call(1);
Expand Down

0 comments on commit 2e5a28d

Please sign in to comment.