Skip to content
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

Jump to bottom

tests: Coverage improvements #474

Merged
merged 3 commits into from
Feb 11, 2019
Merged

tests: Coverage improvements #474

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
960ff83
APMRegistry: fix tests checking APMRegistry doesn't init without requ…
sohkai Feb 8, 2019
76bac53
EVMScriptRegistry: add more tests for coverage
sohkai Feb 8, 2019
8bb05ad
ENSSubdomainRegistrary: fix initialization test
sohkai Feb 8, 2019
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
74 changes: 41 additions & 33 deletions contracts/test/mocks/APMRegistryFactoryMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,49 @@
pragma solidity 0.4.24;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mock has been refactored to be simpler, and more targeted about what's missing.


import "../../apm/APMRegistry.sol";
import "../../apm/Repo.sol";
import "../../ens/ENSSubdomainRegistrar.sol";

import "../../factory/DAOFactory.sol";
import "../../factory/ENSFactory.sol";

// Mock that doesn't grant enough permissions
// external ENS
// Only usable with new ENS instance

import "../../factory/APMRegistryFactory.sol";
contract APMRegistryFactoryMock is APMInternalAppNames {
DAOFactory public daoFactory;
APMRegistry public registryBase;
Repo public repoBase;
ENSSubdomainRegistrar public ensSubdomainRegistrarBase;
ENS public ens;

contract APMRegistryFactoryMock is APMRegistryFactory {
constructor(
DAOFactory _daoFactory,
APMRegistry _registryBase,
Repo _repoBase,
ENSSubdomainRegistrar _ensSubBase,
ENS _ens,
ENSFactory _ensFactory
)
APMRegistryFactory(_daoFactory, _registryBase, _repoBase, _ensSubBase, _ens, _ensFactory) public {}

function newAPM(bytes32, bytes32, address) public returns (APMRegistry) {}

function newBadAPM(bytes32 tld, bytes32 label, address _root, bool withoutACL) public returns (APMRegistry) {
bytes32 node = keccak256(abi.encodePacked(tld, label));
) public
{
daoFactory = _daoFactory;
registryBase = _registryBase;
repoBase = _repoBase;
ensSubdomainRegistrarBase = _ensSubBase;
ens = _ensFactory.newENS(this);
}

// Assume it is the test ENS
if (ens.owner(node) != address(this)) {
// If we weren't in test ens and factory doesn't have ownership, will fail
ens.setSubnodeOwner(tld, label, this);
}
function newFailingAPM(
bytes32 _tld,
bytes32 _label,
address _root,
bool _withoutNameRole
)
public
returns (APMRegistry)
{
// Set up ENS control
bytes32 node = keccak256(abi.encodePacked(_tld, _label));
ens.setSubnodeOwner(_tld, _label, this);

Kernel dao = daoFactory.newDAO(this);
ACL acl = ACL(dao.acl());
Expand Down Expand Up @@ -55,34 +73,24 @@ contract APMRegistryFactoryMock is APMRegistryFactory {
bytes32 repoAppId = keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(REPO_APP_NAME))));
dao.setApp(dao.APP_BASES_NAMESPACE(), repoAppId, repoBase);

emit DeployAPM(node, apm);

// Grant permissions needed for APM on ENSSubdomainRegistrar
acl.createPermission(apm, ensSub, ensSub.POINT_ROOTNODE_ROLE(), _root);

if (withoutACL) {
// Don't grant all permissions needed for APM to initialize
if (_withoutNameRole) {
acl.createPermission(apm, ensSub, ensSub.CREATE_NAME_ROLE(), _root);
}

acl.createPermission(apm, ensSub, ensSub.POINT_ROOTNODE_ROLE(), _root);

configureAPMPermissions(acl, apm, _root);

// allow apm to create permissions for Repos in Kernel
bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE();

if (!withoutACL) {
if (!_withoutNameRole) {
bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE();
acl.grantPermission(apm, acl, permRole);
}

// Permission transition to _root
acl.setPermissionManager(_root, dao, dao.APP_MANAGER_ROLE());
acl.revokePermission(this, acl, permRole);
acl.grantPermission(_root, acl, permRole);
acl.setPermissionManager(_root, acl, permRole);

// Initialize
ens.setOwner(node, ensSub);
ensSub.initialize(ens, node);

// This should fail since we haven't given all required permissions
apm.initialize(ensSub);

return apm;
Expand Down
29 changes: 20 additions & 9 deletions test/apm_registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,17 @@ contract('APMRegistry', accounts => {
assert.equal(await repo.getVersionsCount(), 2, 'should have created version')
})

it('fails to init with existing ENS deployment if not owner of tld', async () => {
const ensReceipt = await ensFactory.newENS(accounts[0])
const ens2 = ENS.at(ensReceipt.logs.filter(l => l.event == 'DeployENS')[0].args.ens)
const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, '0x00')

// Factory doesn't have ownership over 'eth' tld
await assertRevert(async () => {
await newFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner)
})
})

it('fails to create empty repo name', async () => {
return assertRevert(async () => {
await registry.newRepo('', repoDev, { from: apmOwner })
Expand All @@ -103,7 +114,7 @@ contract('APMRegistry', accounts => {
})
})

context('creating test.aragonpm.eth repo', () => {
context('> Creating test.aragonpm.eth repo', () => {
let repo = {}

beforeEach(async () => {
Expand Down Expand Up @@ -164,22 +175,22 @@ contract('APMRegistry', accounts => {
})
})

context('APMRegistry created with lacking permissions', () => {
context('> Created with missing permissions', () => {
let apmFactoryMock

before(async () => {
apmFactoryMock = await APMRegistryFactoryMock.new(daoFactory.address, ...baseAddrs, '0x0', ensFactory.address)
apmFactoryMock = await APMRegistryFactoryMock.new(daoFactory.address, ...baseAddrs, ensFactory.address)
})

it('fails if factory doesnt give permission to create permissions', async () => {
return assertRevert(async () => {
await apmFactoryMock.newBadAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, true)
it('fails if factory doesnt give permission to create names', async () => {
await assertRevert(async () => {
await apmFactoryMock.newFailingAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, true)
})
})

it('fails if factory doesnt give permission to create names', async () => {
return assertRevert(async () => {
await apmFactoryMock.newBadAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, false)
it('fails if factory doesnt give permission to create permissions', async () => {
await assertRevert(async () => {
await apmFactoryMock.newFailingAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, false)
})
})
})
Expand Down
18 changes: 13 additions & 5 deletions test/ens_subdomains.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,19 @@ const Kernel = artifacts.require('Kernel')
const ACL = artifacts.require('ACL')

const APMRegistry = artifacts.require('APMRegistry')
const AppProxyUpgradeable = artifacts.require('AppProxyUpgradeable')
const ENSSubdomainRegistrar = artifacts.require('ENSSubdomainRegistrar')
const Repo = artifacts.require('Repo')
const APMRegistryFactory = artifacts.require('APMRegistryFactory')
const DAOFactory = artifacts.require('DAOFactory')

const EMPTY_BYTES = '0x'

// Using APMFactory in order to reuse it
contract('ENSSubdomainRegistrar', accounts => {
let baseDeployed, apmFactory, ensFactory, daoFactory, ens, registrar
let baseDeployed, apmFactory, ensFactory, dao, daoFactory, ens, registrar
let APP_BASES_NAMESPACE

const ensOwner = accounts[0]
const apmOwner = accounts[1]
const repoDev = accounts[2]
Expand All @@ -38,6 +43,8 @@ contract('ENSSubdomainRegistrar', accounts => {
const kernelBase = await Kernel.new(true) // petrify immediately
const aclBase = await ACL.new()
daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, '0x00')

APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE()
})

beforeEach(async () => {
Expand All @@ -49,7 +56,7 @@ contract('ENSSubdomainRegistrar', accounts => {
const apmAddr = receipt.logs.filter(l => l.event == 'DeployAPM')[0].args.apm
const registry = APMRegistry.at(apmAddr)

const dao = Kernel.at(await registry.kernel())
dao = Kernel.at(await registry.kernel())
const acl = ACL.at(await dao.acl())

registrar = ENSSubdomainRegistrar.at(await registry.registrar())
Expand Down Expand Up @@ -99,11 +106,12 @@ contract('ENSSubdomainRegistrar', accounts => {
})

it('fails if initializing without rootnode ownership', async () => {
const reg = await ENSSubdomainRegistrar.new()
const ens = await ENS.new()
const newRegProxy = await AppProxyUpgradeable.new(dao.address, namehash('apm-enssub.aragonpm.eth'), EMPTY_BYTES)
const newReg = ENSSubdomainRegistrar.at(newRegProxy.address)

return assertRevert(async () => {
await reg.initialize(ens.address, holanode)
await assertRevert(async () => {
await newReg.initialize(ens.address, holanode)
})
})
})
34 changes: 15 additions & 19 deletions test/evm_script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ contract('EVM Script', accounts => {

assertEvent(receipt, 'DisableExecutor')
assert.isFalse(executorEntry[1], "executor should now be disabled")
assert.equal(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return zero addr')
})

it('can re-enable an executor', async () => {
Expand All @@ -117,12 +118,14 @@ contract('EVM Script', accounts => {
await reg.disableScriptExecutor(newExecutorId, { from: boss })
let executorEntry = await reg.executors(newExecutorId)
assert.isFalse(executorEntry[1], "executor should now be disabled")
assert.equal(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return zero addr')

const receipt = await reg.enableScriptExecutor(newExecutorId, { from: boss })
executorEntry = await reg.executors(newExecutorId)

assertEvent(receipt, 'EnableExecutor')
assert.isTrue(executorEntry[1], "executor should now be re-enabled")
assert.notEqual(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return non-zero addr')
})

it('fails to add a new executor without the correct permissions', async () => {
Expand Down Expand Up @@ -153,6 +156,13 @@ contract('EVM Script', accounts => {
})
})

it('fails to enable a non-existent executor', async () => {
await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss })
await assertRevert(async () => {
await reg.enableScriptExecutor(newExecutorId + 1, { from: boss })
})
})

it('fails to disable an already disabled executor', async () => {
await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss })
await reg.disableScriptExecutor(newExecutorId, { from: boss })
Expand Down Expand Up @@ -326,37 +336,23 @@ contract('EVM Script', accounts => {
})
})

context('registry', () => {
it('can be disabled', async () => {
context('> Registry actions', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that these tests have been simplified as most of the functionality was already tested earlier.

it("can't be executed once disabled", async () => {
await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss })
const receipt = await reg.disableScriptExecutor(1, { from: boss })
const isEnabled = (await reg.executors(1))[1] // enabled flag is second in struct
await reg.disableScriptExecutor(1, { from: boss })

assertEvent(receipt, 'DisableExecutor')
assert.equal(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR, 'getting disabled executor should return zero addr')
assert.isFalse(isEnabled, 'executor should be disabled')
return assertRevert(async () => {
await executorApp.execute(encodeCallScript([]))
})
})

it('can be re-enabled', async () => {
let isEnabled
await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss })

// First, disable the executor
// Disable then re-enable the executor
await reg.disableScriptExecutor(1, { from: boss })
isEnabled = (await reg.executors(1))[1] // enabled flag is second in struct
assert.equal(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR, 'getting disabled executor should return zero addr')
assert.isFalse(isEnabled, 'executor should be disabled')

// Then re-enable it
const receipt = await reg.enableScriptExecutor(1, { from: boss })
isEnabled = (await reg.executors(1))[1] // enabled flag is second in struct
await reg.enableScriptExecutor(1, { from: boss })

assertEvent(receipt, 'EnableExecutor')
assert.notEqual(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR, 'getting enabled executor should be non-zero addr')
assert.isTrue(isEnabled, 'executor should be enabled')
await executorApp.execute(encodeCallScript([]))
})
})
Expand Down