Skip to content

Commit

Permalink
Disputable: Move set agreement to Kernel
Browse files Browse the repository at this point in the history
  • Loading branch information
@facuspagnuolo
facuspagnuolo committed Jul 22, 2020
1 parent ae3b1bd commit e64c1f9
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 36 deletions.
12 changes: 6 additions & 6 deletions contracts/apps/disputable/DisputableAragonApp.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import "../../lib/math/SafeMath64.sol";

contract DisputableAragonApp is IDisputable, AragonApp {
/* Validation errors */
string internal constant ERROR_SENDER_NOT_KERNEL = "DISPUTABLE_SENDER_NOT_KERNEL";
string internal constant ERROR_SENDER_NOT_AGREEMENT = "DISPUTABLE_SENDER_NOT_AGREEMENT";
string internal constant ERROR_AGREEMENT_STATE_INVALID = "DISPUTABLE_AGREEMENT_STATE_INVAL";

Expand All @@ -21,9 +22,6 @@ contract DisputableAragonApp is IDisputable, AragonApp {
// bytes32 public constant CHALLENGE_ROLE = keccak256("CHALLENGE_ROLE");
bytes32 public constant CHALLENGE_ROLE = 0xef025787d7cd1a96d9014b8dc7b44899b8c1350859fb9e1e05f5a546dd65158d;

// bytes32 public constant SET_AGREEMENT_ROLE = keccak256("SET_AGREEMENT_ROLE");
bytes32 public constant SET_AGREEMENT_ROLE = 0x8dad640ab1b088990c972676ada708447affc660890ec9fc9a5483241c49f036;

// bytes32 internal constant AGREEMENT_POSITION = keccak256("aragonOS.appStorage.agreement");
bytes32 internal constant AGREEMENT_POSITION = 0x6dbe80ccdeafbf5f3fff5738b224414f85e9370da36f61bf21c65159df7409e9;

Expand Down Expand Up @@ -78,9 +76,11 @@ contract DisputableAragonApp is IDisputable, AragonApp {
* @notice Set Agreement to `_agreement`
* @param _agreement Agreement instance to be set
*/
function setAgreement(IAgreement _agreement) external auth(SET_AGREEMENT_ROLE) {
IAgreement agreement = _getAgreement();
require(agreement == IAgreement(0) && _agreement != IAgreement(0), ERROR_AGREEMENT_STATE_INVALID);
function setAgreement(IAgreement _agreement) external {
require(IKernel(msg.sender) == kernel(), ERROR_SENDER_NOT_KERNEL);

IAgreement currentAgreement = _getAgreement();
require(currentAgreement == IAgreement(0) && _agreement != IAgreement(0), ERROR_AGREEMENT_STATE_INVALID);

AGREEMENT_POSITION.setStorageAddress(address(_agreement));
emit AgreementSet(_agreement);
Expand Down
38 changes: 27 additions & 11 deletions contracts/kernel/Kernel.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,30 @@ import "../common/Petrifiable.sol";
import "../common/VaultRecoverable.sol";
import "../factory/AppProxyFactory.sol";
import "../lib/misc/ERCProxy.sol";
import "../apps/disputable/IAgreement.sol";
import "../apps/disputable/IDisputable.sol";


// solium-disable-next-line max-len
contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstants, Petrifiable, IsContract, VaultRecoverable, AppProxyFactory, ACLSyntaxSugar {
/* Hardcoded constants to save gas
bytes32 public constant APP_MANAGER_ROLE = keccak256("APP_MANAGER_ROLE");
*/
// bytes32 public constant APP_MANAGER_ROLE = keccak256("APP_MANAGER_ROLE");
bytes32 public constant APP_MANAGER_ROLE = 0xb6d92708f3d4817afc106147d969e229ced5c46e65e0a5002a0d391287762bd0;

// bytes32 public constant SET_AGREEMENT_ROLE = keccak256("SET_AGREEMENT_ROLE");
bytes32 public constant SET_AGREEMENT_ROLE = 0x8dad640ab1b088990c972676ada708447affc660890ec9fc9a5483241c49f036;

string private constant ERROR_APP_NOT_CONTRACT = "KERNEL_APP_NOT_CONTRACT";
string private constant ERROR_INVALID_APP_CHANGE = "KERNEL_INVALID_APP_CHANGE";
string private constant ERROR_AUTH_FAILED = "KERNEL_AUTH_FAILED";

modifier auth(bytes32 _role, uint256[] memory _params) {
require(
hasPermission(msg.sender, address(this), _role, ConversionHelpers.dangerouslyCastUintArrayToBytes(_params)),
ERROR_AUTH_FAILED
);
_;
}

/**
* @dev Constructor that allows the deployer to choose if the base instance should be petrified immediately.
* @param _shouldPetrify Immediately petrify this instance so that it can never be initialized
Expand All @@ -34,6 +45,19 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
}
}

/**
* @dev Set an Agreement for a disputable app
* @notice Set `_agreement` as the Agreement for the disputable app `_disputableApp`
* @param _disputableApp Address of the disputable app to set the agreement of
* @param _agreement Agreement instance to be set
*/
function setAgreement(IDisputable _disputableApp, IAgreement _agreement)
external
auth(SET_AGREEMENT_ROLE, arr(_disputableApp, _agreement))
{
_disputableApp.setAgreement(_agreement);
}

/**
* @dev Initialize can only be called once. It saves the block number in which it was initialized.
* @notice Initialize this kernel instance along with its ACL and set `_permissionsCreator` as the entity that can create other permissions
Expand Down Expand Up @@ -227,12 +251,4 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
_setApp(_namespace, _appId, _app);
}
}

modifier auth(bytes32 _role, uint256[] memory _params) {
require(
hasPermission(msg.sender, address(this), _role, ConversionHelpers.dangerouslyCastUintArrayToBytes(_params)),
ERROR_AUTH_FAILED
);
_;
}
}
46 changes: 27 additions & 19 deletions test/contracts/apps/disputable/disputable_app.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const { toChecksumAddress } = require('web3-utils')
const { assertRevert } = require('../../../helpers/assertThrow')
const { getEventArgument } = require('../../../helpers/events')
const { getNewProxyAddress } = require('../../../helpers/events')
const { decodeEventsOfType } = require('../../../helpers/decodeEvent')
const { assertEvent, assertAmountOfEvents } = require('../../../helpers/assertEvent')(web3)

const ACL = artifacts.require('ACL')
Expand Down Expand Up @@ -30,15 +32,15 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>

const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE()
await acl.createPermission(owner, dao.address, APP_MANAGER_ROLE, owner, { from: owner })

const SET_AGREEMENT_ROLE = await kernelBase.SET_AGREEMENT_ROLE()
await acl.createPermission(owner, dao.address, SET_AGREEMENT_ROLE, owner, { from: owner })
})

beforeEach('install disputable app', async () => {
const initializeData = disputableBase.contract.initialize.getData()
const receipt = await dao.newAppInstance('0x1234', disputableBase.address, initializeData, false, { from: owner })
disputable = await DisputableApp.at(getNewProxyAddress(receipt))

const SET_AGREEMENT_ROLE = await disputable.SET_AGREEMENT_ROLE()
await acl.createPermission(owner, disputable.address, SET_AGREEMENT_ROLE, owner, { from: owner })
})

describe('supportsInterface', () => {
Expand Down Expand Up @@ -67,17 +69,19 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>

const itSetsTheAgreementAddress = agreement => {
it('sets the agreement', async () => {
await disputable.setAgreement(agreement, { from })
await dao.setAgreement(disputable.address, agreement, { from })

const currentAgreement = await disputable.getAgreement()
assert.equal(currentAgreement, agreement, 'disputable agreement does not match')
})

it('emits an event', async () => {
const receipt = await disputable.setAgreement(agreement, { from })
const { tx } = await dao.setAgreement(disputable.address, agreement, { from })
const receipt = await web3.eth.getTransactionReceipt(tx)
const logs = decodeEventsOfType(receipt, DisputableApp.abi, 'AgreementSet')

assertAmountOfEvents(receipt, 'AgreementSet')
assertEvent(receipt, 'AgreementSet', { agreement })
assertAmountOfEvents({ logs }, 'AgreementSet')
assertEvent({ logs }, 'AgreementSet', { agreement: toChecksumAddress(agreement) })
})
}

Expand All @@ -88,31 +92,31 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>

context('when trying to unset the agreement', () => {
it('reverts', async () => {
await assertRevert(disputable.setAgreement(ZERO_ADDRESS, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
await assertRevert(dao.setAgreement(disputable.address, ZERO_ADDRESS, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
})
})
})

context('when the agreement was already set', () => {
beforeEach('set agreement', async () => {
await disputable.setAgreement(agreement, { from })
await dao.setAgreement(disputable.address, agreement, { from })
})

context('when trying to re-set the agreement', () => {
it('reverts', async () => {
await assertRevert(disputable.setAgreement(agreement, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
await assertRevert(dao.setAgreement(disputable.address, agreement, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
})
})

context('when trying to set a new agreement', () => {
it('reverts', async () => {
await assertRevert(disputable.setAgreement(anotherAgreement, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
await assertRevert(dao.setAgreement(disputable.address, anotherAgreement, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
})
})

context('when trying to unset the agreement', () => {
it('reverts', async () => {
await assertRevert(disputable.setAgreement(ZERO_ADDRESS, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
await assertRevert(dao.setAgreement(disputable.address, ZERO_ADDRESS, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL')
})
})
})
Expand All @@ -122,7 +126,11 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
const from = someone

it('reverts', async () => {
await assertRevert(disputable.setAgreement(agreement, { from }), 'APP_AUTH_FAILED')
await assertRevert(dao.setAgreement(disputable.address, agreement, { from }), 'KERNEL_AUTH_FAILED')
})

it('reverts', async () => {
await assertRevert(disputable.setAgreement(agreement, { from }), 'DISPUTABLE_SENDER_NOT_KERNEL')
})
})
})
Expand All @@ -139,7 +147,7 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>

beforeEach('set agreement', async () => {
agreement = await AgreementMock.new()
await disputable.setAgreement(agreement.address, { from: owner })
await dao.setAgreement(disputable.address, agreement.address, { from: owner })
})

it('does not revert', async () => {
Expand Down Expand Up @@ -167,7 +175,7 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
context('when the agreement is set', () => {
beforeEach('set agreement', async () => {
const agreement = await AgreementMock.new()
await disputable.setAgreement(agreement.address, { from: owner })
await dao.setAgreement(disputable.address, agreement.address, { from: owner })
})

it('does not revert', async () => {
Expand All @@ -183,7 +191,7 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
const agreement = someone

beforeEach('set agreement', async () => {
await disputable.setAgreement(agreement, { from: owner })
await dao.setAgreement(disputable.address, agreement, { from: owner })
})

context('when the sender is the agreement', () => {
Expand Down Expand Up @@ -219,7 +227,7 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
const agreement = someone

beforeEach('set agreement', async () => {
await disputable.setAgreement(agreement, { from: owner })
await dao.setAgreement(disputable.address, agreement, { from: owner })
})

context('when the sender is the agreement', () => {
Expand Down Expand Up @@ -255,7 +263,7 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
const agreement = someone

beforeEach('set agreement', async () => {
await disputable.setAgreement(agreement, { from: owner })
await dao.setAgreement(disputable.address, agreement, { from: owner })
})

context('when the sender is the agreement', () => {
Expand Down Expand Up @@ -291,7 +299,7 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
const agreement = someone

beforeEach('set agreement', async () => {
await disputable.setAgreement(agreement, { from: owner })
await dao.setAgreement(disputable.address, agreement, { from: owner })
})

context('when the sender is the agreement', () => {
Expand Down

0 comments on commit e64c1f9

Please sign in to comment.