Skip to content

[DO NOT MERGE] Disputable: Move set agreement to Kernel #599

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

Draft
facuspagnuolo wants to merge 3 commits into
base: next
Choose a base branch
from
+70 −37
Draft

[DO NOT MERGE] Disputable: Move set agreement to Kernel #599

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
e64c1f9
Disputable: Move set agreement to Kernel
facuspagnuolo Jul 22, 2020
595ed33
Disputable: Add `setAgreement` to IKernel interface
facuspagnuolo Jul 23, 2020
c83847a
Disputable: Small tweaks
facuspagnuolo Jul 24, 2020
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
12 changes: 6 additions & 6 deletions contracts/apps/disputable/DisputableAragonApp.sol
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions contracts/kernel/IKernel.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ pragma solidity ^0.4.24;

import "../acl/IACL.sol";
import "../common/IVaultRecoverable.sol";
import "../apps/disputable/IAgreement.sol";
import "../apps/disputable/IDisputable.sol";


interface IKernelEvents {
Expand All @@ -20,4 +22,7 @@ contract IKernel is IKernelEvents, IVaultRecoverable {

Copy link
Contributor

@sohkai sohkai Jul 24, 2020

Choose a reason for hiding this comment

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

Two comments:

  1. Are we able to move all the interfaces to be external?
  2. Should we use AragonApp as the type for setApp()'s app parameter? I'm debating if this interface dependency tree is worth it, but if not, then we may also want to type setAgreement() with just addresses.

Copy link
Contributor Author

@facuspagnuolo facuspagnuolo Jul 24, 2020

Choose a reason for hiding this comment

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

I was planning to discuss this as one of the things to change in the new breaking version, but yea, I'd love to polish all the interfaces we provide

function setApp(bytes32 namespace, bytes32 appId, address app) public;
function getApp(bytes32 namespace, bytes32 appId) public view returns (address);

// solium-disable function-order
function setAgreement(IDisputable _disputableApp, IAgreement _agreement) external;
}
38 changes: 27 additions & 11 deletions contracts/kernel/Kernel.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import "./KernelConstants.sol";
import "./KernelStorage.sol";
import "../acl/IACL.sol";
import "../acl/ACLSyntaxSugar.sol";
import "../apps/disputable/IAgreement.sol";
import "../apps/disputable/IDisputable.sol";
import "../common/ConversionHelpers.sol";
import "../common/IsContract.sol";
import "../common/Petrifiable.sol";
Expand All @@ -15,15 +17,24 @@ import "../lib/misc/ERCProxy.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
);
_;
}
}
52 changes: 32 additions & 20 deletions test/contracts/apps/disputable/disputable_app.js
View file
Open in desktop
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)
Copy link
Contributor

@sohkai sohkai Jul 24, 2020

Choose a reason for hiding this comment

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

Oh interesting, was the receipt we got as the return of dao.setAgreement() not sufficient?

Copy link
Contributor Author

@facuspagnuolo facuspagnuolo Jul 24, 2020

Choose a reason for hiding this comment

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

For some reason, it wasn't working. I think this will be fixed when we upgrade to Truffle 5 + helpers

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 @@ -121,8 +125,16 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) =>
context('when the sender does not have permissions', () => {
const from = someone

it('reverts', async () => {
await assertRevert(disputable.setAgreement(agreement, { from }), 'APP_AUTH_FAILED')
context('when going through the kernel', () => {
it('reverts', async () => {
await assertRevert(dao.setAgreement(disputable.address, agreement, { from }), 'KERNEL_AUTH_FAILED')
})
})

context('when going through the disputable', () => {
it('reverts', async () => {
await assertRevert(disputable.setAgreement(agreement, { from }), 'DISPUTABLE_SENDER_NOT_KERNEL')
})
})
})
})
Expand All @@ -139,7 +151,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 +179,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 +195,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 +231,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 +267,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 +303,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