Skip to content

Improve adding domains #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 10, 2018
Merged

Improve adding domains #268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e08a44
When adding a new domain, pass in parent domain id
elenadimitrova Jul 5, 2018
706996f
Changes following Alex's review
elenadimitrova Jul 6, 2018
965ce4d
Fix test using invalid skill id
elenadimitrova Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions contracts/Colony.sol
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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