[DO NOT MERGE] Disputable: Move set agreement to Kernel #599
Conversation
facuspagnuolo
requested review from
bingen and
sohkai
and removed request for
bingen
facuspagnuolo
changed the title
facuspagnuolo
force-pushed
the
move_set_agreement_to_kernel
branch
from
fa0d40b to
595ed33
Compare
There was a problem hiding this comment.
A few minor comments, but LGTM!
For other's sake, we decided to batch this breaking change with a few more breaking changes down the road as we otherwise cannot use the same DAOFactorys and DAO templates as before to create test organizations with Agreements.
| @@ -15,6 +17,8 @@ interface IKernelEvents { | |||
|
|
|||
| // This should be an interface, but interfaces can't inherit yet :( | |||
| contract IKernel is IKernelEvents, IVaultRecoverable { | |||
| function setAgreement(IDisputable _disputableApp, IAgreement _agreement) external; | |||
|
|
|||
| function acl() public view returns (IACL); | |||
| function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool); | |||
|
|
|||
There was a problem hiding this comment.
Two comments:
- Are we able to move all the interfaces to be external?
- Should we use
AragonAppas the type forsetApp()'sappparameter? I'm debating if this interface dependency tree is worth it, but if not, then we may also want to typesetAgreement()with just addresses.
There was a problem hiding this comment.
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
|
|
||
| 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) |
There was a problem hiding this comment.
Oh interesting, was the receipt we got as the return of dao.setAgreement() not sufficient?
There was a problem hiding this comment.
For some reason, it wasn't working. I think this will be fixed when we upgrade to Truffle 5 + helpers
|
As an alternative implementation, we could introduce the concept of It would work similarly to how This change in the ACL could be useful for other cases as well, such as setting permissions just once for the admin functions of an app that the DAO has multiple instances of (e.g. multiple Voting instances). |
This PR moves the
setAgreementsecurity check to the Kernel, allowing us to define Agreement<>Kernel permissions instead of Agreement<>Disputable ones.