From e64c1f9283589fc5b7c6ab9e1a824d6bbf3f0291 Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Wed, 22 Jul 2020 19:03:28 -0300 Subject: [PATCH] Disputable: Move set agreement to Kernel --- .../apps/disputable/DisputableAragonApp.sol | 12 ++--- contracts/kernel/Kernel.sol | 38 ++++++++++----- .../apps/disputable/disputable_app.js | 46 +++++++++++-------- 3 files changed, 60 insertions(+), 36 deletions(-) diff --git a/contracts/apps/disputable/DisputableAragonApp.sol b/contracts/apps/disputable/DisputableAragonApp.sol index 8e2907e18..ef36a4d41 100644 --- a/contracts/apps/disputable/DisputableAragonApp.sol +++ b/contracts/apps/disputable/DisputableAragonApp.sol @@ -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"; @@ -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; @@ -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); diff --git a/contracts/kernel/Kernel.sol b/contracts/kernel/Kernel.sol index 1fc919055..f7f7c35e2 100644 --- a/contracts/kernel/Kernel.sol +++ b/contracts/kernel/Kernel.sol @@ -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 @@ -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 @@ -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 - ); - _; - } } diff --git a/test/contracts/apps/disputable/disputable_app.js b/test/contracts/apps/disputable/disputable_app.js index 6096657a2..880e904f2 100644 --- a/test/contracts/apps/disputable/disputable_app.js +++ b/test/contracts/apps/disputable/disputable_app.js @@ -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') @@ -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', () => { @@ -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) }) }) } @@ -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') }) }) }) @@ -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') }) }) }) @@ -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 () => { @@ -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 () => { @@ -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', () => { @@ -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', () => { @@ -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', () => { @@ -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', () => {