From 37b26124a248957fc9599af6787048b6ff01edca Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 15:38:49 -0500 Subject: [PATCH 01/49] Add encodeParam to ACLHelpers --- contracts/acl/ACLSyntaxSugar.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 38ca29fdc..ab7f7ed7a 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -86,6 +86,10 @@ contract ACLSyntaxSugar { contract ACLHelpers { + function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { + return uint256(id) << 248 | uint256(op) << 240 | value; + } + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } From 85dda17e87f7a4e93e2407e1ceeaf559d1922b4a Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 20:20:40 -0500 Subject: [PATCH 02/49] Move Op and Param structs from ACL to ACLHelpers --- contracts/acl/ACL.sol | 10 ---------- contracts/acl/ACLSyntaxSugar.sol | 13 ++++++++++++- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index c9c62d770..cc699c837 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -15,16 +15,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { */ bytes32 public constant CREATE_PERMISSIONS_ROLE = 0x0b719b33c83b8e5d300c521cb8b54ae9bd933996a14bef8c2f4e0285d2d2400a; - enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types - - struct Param { - uint8 id; - uint8 op; - uint240 value; // even though value is an uint240 it can store addresses - // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal - // op and id take less than 1 byte each so it can be kept in 1 sstore - } - uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200; uint8 internal constant TIMESTAMP_PARAM_ID = 201; // 202 is unused diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index ab7f7ed7a..ca2968f55 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -86,10 +86,21 @@ contract ACLSyntaxSugar { contract ACLHelpers { + + enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types + + struct Param { + uint8 id; + uint8 op; + uint240 value; // even though value is an uint240 it can store addresses + // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal + // op and id take less than 1 byte each so it can be kept in 1 sstore + } + function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { return uint256(id) << 248 | uint256(op) << 240 | value; } - + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } From e3b49eaaff51b474329a8b283534cfcec65df68b Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 20:36:02 -0500 Subject: [PATCH 03/49] Add encoreParams and encodeParam for Param structs --- contracts/acl/ACLSyntaxSugar.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index ca2968f55..8c063887d 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -97,6 +97,19 @@ contract ACLHelpers { // op and id take less than 1 byte each so it can be kept in 1 sstore } + function encodeParams(Param[] params) internal pure returns (uint256[]) { + uint256[] memory encodedParams = new uint256[](params.length); + + for (uint i = 0; i < params.length; i++) + encodedParams[i] = encodeParam(params[i]); + + return encodedParams; + } + + function encodeParam(Param param) internal pure returns (uint256) { + return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; + } + function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { return uint256(id) << 248 | uint256(op) << 240 | value; } From bc0e525a689f0d5c32c5db2866045f61e075961a Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 22:16:37 -0500 Subject: [PATCH 04/49] Add TestACLHelpers --- contracts/test/TestACLHelpers.sol | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 contracts/test/TestACLHelpers.sol diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol new file mode 100644 index 000000000..39bd0d218 --- /dev/null +++ b/contracts/test/TestACLHelpers.sol @@ -0,0 +1,26 @@ +pragma solidity 0.4.24; + +import "./helpers/Assert.sol"; +import "../acl/ACL.sol"; +import "../acl/ACLSyntaxSugar.sol"; + + +contract TestACLHelpers is ACLHelpers, ACL { + + function testEncodeParam() public { + Param memory param = Param({ + id: 2, + op: uint8(Op.EQ), + value: 5294967297 + }); + + uint256 encodedParam = encodeParam(param.id, param.op, param.value); + + (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam); + + Assert.equal(uint256(param.id), uint256(id), "Encoded id is not equal"); + Assert.equal(uint256(param.op), uint256(op), "Encoded op is not equal"); + Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal"); + } + +} From a97c86dc9e1ea3b085240febff53f2aa791cf7ff Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 10:10:17 -0500 Subject: [PATCH 05/49] Remove encodeParam with uint params --- contracts/acl/ACLSyntaxSugar.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 8c063887d..52df934f7 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -110,10 +110,6 @@ contract ACLHelpers { return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; } - function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { - return uint256(id) << 248 | uint256(op) << 240 | value; - } - function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } From fee7fc145dc9cc930b28b9d41f23f272d104c573 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 10:30:13 -0500 Subject: [PATCH 06/49] Add AclHelpers js launch test --- contracts/test/TestACLHelpers.sol | 2 +- test/acl_helpers.js | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 test/acl_helpers.js diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 39bd0d218..7576a47a7 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -14,7 +14,7 @@ contract TestACLHelpers is ACLHelpers, ACL { value: 5294967297 }); - uint256 encodedParam = encodeParam(param.id, param.op, param.value); + uint256 encodedParam = encodeParam(param); (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam); diff --git a/test/acl_helpers.js b/test/acl_helpers.js new file mode 100644 index 000000000..244f52787 --- /dev/null +++ b/test/acl_helpers.js @@ -0,0 +1,3 @@ +const runSolidityTest = require('./helpers/runSolidityTest') + +runSolidityTest('TestACLHelpers') \ No newline at end of file From ecf2f5c64fcfe29de62ea46acacd5b183626d78a Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 10:44:01 -0500 Subject: [PATCH 07/49] Add encodeParams test --- contracts/test/TestACLHelpers.sol | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 7576a47a7..4d0cd5ac6 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -23,4 +23,35 @@ contract TestACLHelpers is ACLHelpers, ACL { Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal"); } + function testEncodeParams() public { + Param[] memory params = new Param[](2); + + params[0] = Param({ + id: 1, + op: uint8(Op.EQ), + value: 5294967297 + }); + + params[1] = Param({ + id: 2, + op: uint8(Op.EQ), + value: 5294967297 + }); + + + uint256[] memory encodedParam = encodeParams(params); + + (uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]); + + Assert.equal(uint256(params[0].id), uint256(id0), "Encoded id is not equal"); + Assert.equal(uint256(params[0].op), uint256(op0), "Encoded op is not equal"); + Assert.equal(uint256(params[0].value), uint256(value0), "Encoded value is not equal"); + + (uint32 id1, uint32 op1, uint32 value1) = decodeParamsList(encodedParam[1]); + + Assert.equal(uint256(params[1].id), uint256(id1), "Encoded id is not equal"); + Assert.equal(uint256(params[1].op), uint256(op1), "Encoded op is not equal"); + Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); + } + } From 85422355c99b6a994daf5ef00d83b9212ef5f752 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 11:46:38 -0500 Subject: [PATCH 08/49] Add brackets around for loop --- contracts/acl/ACLSyntaxSugar.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 52df934f7..386b8d2d0 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -100,8 +100,9 @@ contract ACLHelpers { function encodeParams(Param[] params) internal pure returns (uint256[]) { uint256[] memory encodedParams = new uint256[](params.length); - for (uint i = 0; i < params.length; i++) + for (uint i = 0; i < params.length; i++) { encodedParams[i] = encodeParam(params[i]); + } return encodedParams; } From a85c5aa452eec83f7049041249e159294bc7c6dc Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 13:45:30 -0500 Subject: [PATCH 09/49] Remove ACL import and inheritance --- contracts/test/TestACLHelpers.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 4d0cd5ac6..abaec988e 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -1,11 +1,10 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; -import "../acl/ACL.sol"; import "../acl/ACLSyntaxSugar.sol"; -contract TestACLHelpers is ACLHelpers, ACL { +contract TestACLHelpers is ACLHelpers { function testEncodeParam() public { Param memory param = Param({ From b3ba05d9ade7f5428475f21b11094c1e9a95e668 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 19:00:22 -0500 Subject: [PATCH 10/49] Move Op and Param to ACLParams contract --- contracts/acl/ACLParams.sol | 15 +++++++++++++++ contracts/acl/ACLSyntaxSugar.sol | 13 +++---------- 2 files changed, 18 insertions(+), 10 deletions(-) create mode 100644 contracts/acl/ACLParams.sol diff --git a/contracts/acl/ACLParams.sol b/contracts/acl/ACLParams.sol new file mode 100644 index 000000000..a449e8d11 --- /dev/null +++ b/contracts/acl/ACLParams.sol @@ -0,0 +1,15 @@ +pragma solidity ^0.4.24; + + +contract ACLParams { + + enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types + + struct Param { + uint8 id; + uint8 op; + uint240 value; // even though value is an uint240 it can store addresses + // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal + // op and id take less than 1 byte each so it can be kept in 1 sstore + } +} \ No newline at end of file diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 386b8d2d0..069fa6cce 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -4,6 +4,8 @@ pragma solidity ^0.4.24; +import "./ACLParams.sol"; + contract ACLSyntaxSugar { function arr() internal pure returns (uint256[]) {} @@ -85,17 +87,8 @@ contract ACLSyntaxSugar { } -contract ACLHelpers { - - enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types +contract ACLHelpers is ACLParams { - struct Param { - uint8 id; - uint8 op; - uint240 value; // even though value is an uint240 it can store addresses - // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal - // op and id take less than 1 byte each so it can be kept in 1 sstore - } function encodeParams(Param[] params) internal pure returns (uint256[]) { uint256[] memory encodedParams = new uint256[](params.length); From 79100ce4f5ad0205b5f95039814d90139306a366 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 20:48:01 -0500 Subject: [PATCH 11/49] Add encodeOperator and encodeIfElse tests --- contracts/test/TestACLHelpers.sol | 45 +++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index abaec988e..32d00bec4 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -1,10 +1,12 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; +import "./helpers/ACLHelper.sol"; import "../acl/ACLSyntaxSugar.sol"; +import "../acl/ACL.sol"; -contract TestACLHelpers is ACLHelpers { +contract TestACLHelpers is ACL, ACLHelper { function testEncodeParam() public { Param memory param = Param({ @@ -23,19 +25,33 @@ contract TestACLHelpers is ACLHelpers { } function testEncodeParams() public { - Param[] memory params = new Param[](2); + Param[] memory params = new Param[](6); params[0] = Param({ - id: 1, - op: uint8(Op.EQ), - value: 5294967297 + id: LOGIC_OP_PARAM_ID, + op: uint8(Op.IF_ELSE), + value: encodeIfElse(1, 2, 3) }); params[1] = Param({ + id: LOGIC_OP_PARAM_ID, + op: uint8(Op.AND), + value: encodeOperator(2, 3) + }); + + params[2] = Param({ id: 2, op: uint8(Op.EQ), - value: 5294967297 - }); + value: 1 + }); + + params[3] = Param({ + id: 3, + op: uint8(Op.NEQ), + value: 2 + }); + + uint256[] memory encodedParam = encodeParams(params); @@ -50,7 +66,20 @@ contract TestACLHelpers is ACLHelpers { Assert.equal(uint256(params[1].id), uint256(id1), "Encoded id is not equal"); Assert.equal(uint256(params[1].op), uint256(op1), "Encoded op is not equal"); - Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); + Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); + + (uint32 id2, uint32 op2, uint32 value2) = decodeParamsList(encodedParam[2]); + + Assert.equal(uint256(params[2].id), uint256(id2), "Encoded id is not equal"); + Assert.equal(uint256(params[2].op), uint256(op2), "Encoded op is not equal"); + Assert.equal(uint256(params[2].value), uint256(value2), "Encoded value is not equal"); + + (uint32 id3, uint32 op3, uint32 value3) = decodeParamsList(encodedParam[3]); + + Assert.equal(uint256(params[3].id), uint256(id3), "Encoded id is not equal"); + Assert.equal(uint256(params[3].op), uint256(op3), "Encoded op is not equal"); + Assert.equal(uint256(params[3].value), uint256(value3), "Encoded value is not equal"); + } } From 9b4f8f313809e9d0a2ebcb714e1f6a23fe1138d0 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 21:02:22 -0500 Subject: [PATCH 12/49] Compact params --- contracts/test/TestACLHelpers.sol | 35 +++++-------------------------- 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 32d00bec4..9b39cd72a 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -9,11 +9,7 @@ import "../acl/ACL.sol"; contract TestACLHelpers is ACL, ACLHelper { function testEncodeParam() public { - Param memory param = Param({ - id: 2, - op: uint8(Op.EQ), - value: 5294967297 - }); + Param memory param = Param(2, uint8(Op.EQ), 5294967297); uint256 encodedParam = encodeParam(param); @@ -27,31 +23,10 @@ contract TestACLHelpers is ACL, ACLHelper { function testEncodeParams() public { Param[] memory params = new Param[](6); - params[0] = Param({ - id: LOGIC_OP_PARAM_ID, - op: uint8(Op.IF_ELSE), - value: encodeIfElse(1, 2, 3) - }); - - params[1] = Param({ - id: LOGIC_OP_PARAM_ID, - op: uint8(Op.AND), - value: encodeOperator(2, 3) - }); - - params[2] = Param({ - id: 2, - op: uint8(Op.EQ), - value: 1 - }); - - params[3] = Param({ - id: 3, - op: uint8(Op.NEQ), - value: 2 - }); - - + params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); + params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); + params[2] = Param(2, uint8(Op.EQ), 1); + params[3] = Param(3, uint8(Op.NEQ), 2); uint256[] memory encodedParam = encodeParams(params); From b05682887c9c10990253700975f6cb5c7ebd8e34 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 23:10:23 -0500 Subject: [PATCH 13/49] Expose ACLHelpers functions --- contracts/acl/ACLSyntaxSugar.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 069fa6cce..1c04e6d0d 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -104,15 +104,15 @@ contract ACLHelpers is ACLParams { return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; } - function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { + function decodeParamOp(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } - function decodeParamId(uint256 _x) internal pure returns (uint8 b) { + function decodeParamId(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 31)); } - function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { + function decodeParamsList(uint256 _x) public pure returns (uint32 a, uint32 b, uint32 c) { a = uint32(_x); b = uint32(_x >> (8 * 4)); c = uint32(_x >> (8 * 8)); From 0aa8d3ca9e1a4e85651056db54f80fd66d7c31f8 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Tue, 20 Nov 2018 09:00:37 -0500 Subject: [PATCH 14/49] Fix linter issues --- contracts/acl/ACLSyntaxSugar.sol | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 1c04e6d0d..ef9836fd4 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -90,20 +90,6 @@ contract ACLSyntaxSugar { contract ACLHelpers is ACLParams { - function encodeParams(Param[] params) internal pure returns (uint256[]) { - uint256[] memory encodedParams = new uint256[](params.length); - - for (uint i = 0; i < params.length; i++) { - encodedParams[i] = encodeParam(params[i]); - } - - return encodedParams; - } - - function encodeParam(Param param) internal pure returns (uint256) { - return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; - } - function decodeParamOp(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } @@ -117,4 +103,18 @@ contract ACLHelpers is ACLParams { b = uint32(_x >> (8 * 4)); c = uint32(_x >> (8 * 8)); } + + function encodeParams(Param[] params) internal pure returns (uint256[]) { + uint256[] memory encodedParams = new uint256[](params.length); + + for (uint i = 0; i < params.length; i++) { + encodedParams[i] = encodeParam(params[i]); + } + + return encodedParams; + } + + function encodeParam(Param param) internal pure returns (uint256) { + return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; + } } From e96f7c02c9a8c576ec8dc498209d181ecde23040 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Wed, 12 Dec 2018 17:41:16 +0100 Subject: [PATCH 15/49] chore: update license text (#467) --- LICENSE | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/LICENSE b/LICENSE index 3f5487ced..6ed1d577c 100644 --- a/LICENSE +++ b/LICENSE @@ -1,9 +1,7 @@ - Copyright (C) 2017-2018 - GNU GENERAL PUBLIC LICENSE Version 3, 29 June 2007 - Copyright (C) Aragon Association + Copyright (C) 2007 Free Software Foundation, Inc. Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. @@ -703,7 +701,24 @@ SOFTWARE. Note: -Unless otherwise specified, individual files are licensed as GPL-3.0-or-later. +Unless otherwise specified, individual files are licensed as GPL-3.0-or-later +and are assumed to include the following text: + + Copyright (C) 2017-2018 Aragon Association + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . + Individual files contain the following tag instead of the full license text if they are licensed differently: From da862e2c8d2666f8a15be74f4d401659e3e1b497 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 14 Dec 2018 18:18:12 +0100 Subject: [PATCH 16/49] KernelProxy: emit SetApp event on construction (#466) Allows a frontend to follow all `SetApp()` events emitted from the start of a KernelProxy's lifespan. --- contracts/kernel/IKernel.sol | 7 +++++-- contracts/kernel/KernelProxy.sol | 8 +++++++- package.json | 1 + test/helpers/decodeEvent.js | 14 ++++++++++++++ test/kernel_upgrade.js | 14 ++++++++++++++ 5 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 test/helpers/decodeEvent.js diff --git a/contracts/kernel/IKernel.sol b/contracts/kernel/IKernel.sol index 0775cec14..e1a2b40e5 100644 --- a/contracts/kernel/IKernel.sol +++ b/contracts/kernel/IKernel.sol @@ -8,10 +8,13 @@ import "../acl/IACL.sol"; import "../common/IVaultRecoverable.sol"; -// This should be an interface, but interfaces can't inherit yet :( -contract IKernel is IVaultRecoverable { +interface IKernelEvents { event SetApp(bytes32 indexed namespace, bytes32 indexed appId, address app); +} + +// This should be an interface, but interfaces can't inherit yet :( +contract IKernel is IKernelEvents, IVaultRecoverable { function acl() public view returns (IACL); function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool); diff --git a/contracts/kernel/KernelProxy.sol b/contracts/kernel/KernelProxy.sol index 8e30c1f2a..d298d8e28 100644 --- a/contracts/kernel/KernelProxy.sol +++ b/contracts/kernel/KernelProxy.sol @@ -7,7 +7,7 @@ import "../common/DepositableDelegateProxy.sol"; import "../common/IsContract.sol"; -contract KernelProxy is KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy { +contract KernelProxy is IKernelEvents, KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy { /** * @dev KernelProxy is a proxy contract to a kernel implementation. The implementation * can update the reference, which effectively upgrades the contract @@ -16,6 +16,12 @@ contract KernelProxy is KernelStorage, KernelAppIds, KernelNamespaceConstants, I constructor(IKernel _kernelImpl) public { require(isContract(address(_kernelImpl))); apps[KERNEL_CORE_NAMESPACE][KERNEL_CORE_APP_ID] = _kernelImpl; + + // Note that emitting this event is important for verifying that a KernelProxy instance + // was never upgraded to a malicious Kernel logic contract over its lifespan. + // This starts the "chain of trust", that can be followed through later SetApp() events + // emitted during kernel upgrades. + emit SetApp(KERNEL_CORE_NAMESPACE, KERNEL_CORE_APP_ID, _kernelImpl); } /** diff --git a/package.json b/package.json index 9e02b7bb7..9fade8b5b 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,7 @@ "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1", "truffle-extract": "^1.2.1", + "web3-eth-abi": "1.0.0-beta.33", "web3-utils": "1.0.0-beta.33" }, "dependencies": { diff --git a/test/helpers/decodeEvent.js b/test/helpers/decodeEvent.js new file mode 100644 index 000000000..0a8a2d7e8 --- /dev/null +++ b/test/helpers/decodeEvent.js @@ -0,0 +1,14 @@ +const abi = require('web3-eth-abi') + +module.exports = { + decodeEventsOfType: (receipt, contractAbi, eventName) => { + const eventAbi = contractAbi.filter(abi => abi.name === eventName && abi.type === 'event')[0] + const eventSignature = abi.encodeEventSignature(eventAbi) + const eventLogs = receipt.logs.filter(l => l.topics[0] === eventSignature) + return eventLogs.map(log => { + log.event = abi.name + log.args = abi.decodeLog(eventAbi.inputs, log.data, log.topics.slice(1)) + return log + }) + } +} diff --git a/test/kernel_upgrade.js b/test/kernel_upgrade.js index 22f4494d1..db0d908b7 100644 --- a/test/kernel_upgrade.js +++ b/test/kernel_upgrade.js @@ -1,4 +1,5 @@ const { assertRevert } = require('./helpers/assertThrow') +const { decodeEventsOfType } = require('./helpers/decodeEvent') const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') @@ -47,6 +48,19 @@ contract('Kernel upgrade', accounts => { assert.equal(proxyType, UPGRADEABLE, "Proxy type should be 2 (upgradeable)") }) + it('emits SetApp event', async () => { + const kernelProxy = await KernelProxy.new(kernelBase.address) + const receipt = web3.eth.getTransactionReceipt(kernelProxy.transactionHash) + + const setAppLogs = decodeEventsOfType(receipt, kernelProxy.abi, 'SetApp') + assert.equal(setAppLogs.length, 1) + + const setAppArgs = setAppLogs[0].args + assert.equal(setAppArgs.namespace, CORE_NAMESPACE, 'Kernel namespace should match') + assert.equal(setAppArgs.appId, KERNEL_APP_ID, 'Kernel app id should match') + assert.equal(setAppArgs.app.toLowerCase(), kernelBase.address.toLowerCase(), 'Kernel base address should match') + }) + it('fails to create a KernelProxy if the base is 0', async () => { return assertRevert(async () => { await KernelProxy.new(ZERO_ADDR) From 480b2f64305760395e6c7d29db98bc2fc3b282f5 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 15:38:49 -0500 Subject: [PATCH 17/49] Add encodeParam to ACLHelpers --- contracts/acl/ACLSyntaxSugar.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 38ca29fdc..ab7f7ed7a 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -86,6 +86,10 @@ contract ACLSyntaxSugar { contract ACLHelpers { + function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { + return uint256(id) << 248 | uint256(op) << 240 | value; + } + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } From bdeee789772d7b49de26b2a58aa2b95aece8ec09 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 20:20:40 -0500 Subject: [PATCH 18/49] Move Op and Param structs from ACL to ACLHelpers --- contracts/acl/ACL.sol | 10 ---------- contracts/acl/ACLSyntaxSugar.sol | 13 ++++++++++++- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index c9c62d770..cc699c837 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -15,16 +15,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { */ bytes32 public constant CREATE_PERMISSIONS_ROLE = 0x0b719b33c83b8e5d300c521cb8b54ae9bd933996a14bef8c2f4e0285d2d2400a; - enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types - - struct Param { - uint8 id; - uint8 op; - uint240 value; // even though value is an uint240 it can store addresses - // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal - // op and id take less than 1 byte each so it can be kept in 1 sstore - } - uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200; uint8 internal constant TIMESTAMP_PARAM_ID = 201; // 202 is unused diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index ab7f7ed7a..ca2968f55 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -86,10 +86,21 @@ contract ACLSyntaxSugar { contract ACLHelpers { + + enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types + + struct Param { + uint8 id; + uint8 op; + uint240 value; // even though value is an uint240 it can store addresses + // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal + // op and id take less than 1 byte each so it can be kept in 1 sstore + } + function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { return uint256(id) << 248 | uint256(op) << 240 | value; } - + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } From 04c2048a62b8a916cb2bbaf6d1f2c7b8f2a643ab Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 20:36:02 -0500 Subject: [PATCH 19/49] Add encoreParams and encodeParam for Param structs --- contracts/acl/ACLSyntaxSugar.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index ca2968f55..8c063887d 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -97,6 +97,19 @@ contract ACLHelpers { // op and id take less than 1 byte each so it can be kept in 1 sstore } + function encodeParams(Param[] params) internal pure returns (uint256[]) { + uint256[] memory encodedParams = new uint256[](params.length); + + for (uint i = 0; i < params.length; i++) + encodedParams[i] = encodeParam(params[i]); + + return encodedParams; + } + + function encodeParam(Param param) internal pure returns (uint256) { + return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; + } + function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { return uint256(id) << 248 | uint256(op) << 240 | value; } From 2530b15d6d741a4b5636fa12339c71c092302125 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 5 Nov 2018 22:16:37 -0500 Subject: [PATCH 20/49] Add TestACLHelpers --- contracts/test/TestACLHelpers.sol | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 contracts/test/TestACLHelpers.sol diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol new file mode 100644 index 000000000..39bd0d218 --- /dev/null +++ b/contracts/test/TestACLHelpers.sol @@ -0,0 +1,26 @@ +pragma solidity 0.4.24; + +import "./helpers/Assert.sol"; +import "../acl/ACL.sol"; +import "../acl/ACLSyntaxSugar.sol"; + + +contract TestACLHelpers is ACLHelpers, ACL { + + function testEncodeParam() public { + Param memory param = Param({ + id: 2, + op: uint8(Op.EQ), + value: 5294967297 + }); + + uint256 encodedParam = encodeParam(param.id, param.op, param.value); + + (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam); + + Assert.equal(uint256(param.id), uint256(id), "Encoded id is not equal"); + Assert.equal(uint256(param.op), uint256(op), "Encoded op is not equal"); + Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal"); + } + +} From 45727e3581d55ea3086757c76e70b940b416ee02 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 10:10:17 -0500 Subject: [PATCH 21/49] Remove encodeParam with uint params --- contracts/acl/ACLSyntaxSugar.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 8c063887d..52df934f7 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -110,10 +110,6 @@ contract ACLHelpers { return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; } - function encodeParam(uint8 id, uint8 op, uint240 value) internal pure returns (uint256) { - return uint256(id) << 248 | uint256(op) << 240 | value; - } - function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } From 97327f3473fac29990149e012c3d748a13d72722 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 10:30:13 -0500 Subject: [PATCH 22/49] Add AclHelpers js launch test --- contracts/test/TestACLHelpers.sol | 2 +- test/acl_helpers.js | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 test/acl_helpers.js diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 39bd0d218..7576a47a7 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -14,7 +14,7 @@ contract TestACLHelpers is ACLHelpers, ACL { value: 5294967297 }); - uint256 encodedParam = encodeParam(param.id, param.op, param.value); + uint256 encodedParam = encodeParam(param); (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam); diff --git a/test/acl_helpers.js b/test/acl_helpers.js new file mode 100644 index 000000000..244f52787 --- /dev/null +++ b/test/acl_helpers.js @@ -0,0 +1,3 @@ +const runSolidityTest = require('./helpers/runSolidityTest') + +runSolidityTest('TestACLHelpers') \ No newline at end of file From c44a02546ef0e3602237116f37faa3742df43327 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 10:44:01 -0500 Subject: [PATCH 23/49] Add encodeParams test --- contracts/test/TestACLHelpers.sol | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 7576a47a7..4d0cd5ac6 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -23,4 +23,35 @@ contract TestACLHelpers is ACLHelpers, ACL { Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal"); } + function testEncodeParams() public { + Param[] memory params = new Param[](2); + + params[0] = Param({ + id: 1, + op: uint8(Op.EQ), + value: 5294967297 + }); + + params[1] = Param({ + id: 2, + op: uint8(Op.EQ), + value: 5294967297 + }); + + + uint256[] memory encodedParam = encodeParams(params); + + (uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]); + + Assert.equal(uint256(params[0].id), uint256(id0), "Encoded id is not equal"); + Assert.equal(uint256(params[0].op), uint256(op0), "Encoded op is not equal"); + Assert.equal(uint256(params[0].value), uint256(value0), "Encoded value is not equal"); + + (uint32 id1, uint32 op1, uint32 value1) = decodeParamsList(encodedParam[1]); + + Assert.equal(uint256(params[1].id), uint256(id1), "Encoded id is not equal"); + Assert.equal(uint256(params[1].op), uint256(op1), "Encoded op is not equal"); + Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); + } + } From b647866f24e292d76769a4773c2b7550d716025f Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 11:46:38 -0500 Subject: [PATCH 24/49] Add brackets around for loop --- contracts/acl/ACLSyntaxSugar.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 52df934f7..386b8d2d0 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -100,8 +100,9 @@ contract ACLHelpers { function encodeParams(Param[] params) internal pure returns (uint256[]) { uint256[] memory encodedParams = new uint256[](params.length); - for (uint i = 0; i < params.length; i++) + for (uint i = 0; i < params.length; i++) { encodedParams[i] = encodeParam(params[i]); + } return encodedParams; } From aa08e27f504a3019f2fb7e9b6bbcc4b664ad244c Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Thu, 8 Nov 2018 13:45:30 -0500 Subject: [PATCH 25/49] Remove ACL import and inheritance --- contracts/test/TestACLHelpers.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 4d0cd5ac6..abaec988e 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -1,11 +1,10 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; -import "../acl/ACL.sol"; import "../acl/ACLSyntaxSugar.sol"; -contract TestACLHelpers is ACLHelpers, ACL { +contract TestACLHelpers is ACLHelpers { function testEncodeParam() public { Param memory param = Param({ From b05c010820fa8066922882993157fb647929772c Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 19:00:22 -0500 Subject: [PATCH 26/49] Move Op and Param to ACLParams contract --- contracts/acl/ACLParams.sol | 15 +++++++++++++++ contracts/acl/ACLSyntaxSugar.sol | 13 +++---------- 2 files changed, 18 insertions(+), 10 deletions(-) create mode 100644 contracts/acl/ACLParams.sol diff --git a/contracts/acl/ACLParams.sol b/contracts/acl/ACLParams.sol new file mode 100644 index 000000000..a449e8d11 --- /dev/null +++ b/contracts/acl/ACLParams.sol @@ -0,0 +1,15 @@ +pragma solidity ^0.4.24; + + +contract ACLParams { + + enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types + + struct Param { + uint8 id; + uint8 op; + uint240 value; // even though value is an uint240 it can store addresses + // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal + // op and id take less than 1 byte each so it can be kept in 1 sstore + } +} \ No newline at end of file diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 386b8d2d0..069fa6cce 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -4,6 +4,8 @@ pragma solidity ^0.4.24; +import "./ACLParams.sol"; + contract ACLSyntaxSugar { function arr() internal pure returns (uint256[]) {} @@ -85,17 +87,8 @@ contract ACLSyntaxSugar { } -contract ACLHelpers { - - enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types +contract ACLHelpers is ACLParams { - struct Param { - uint8 id; - uint8 op; - uint240 value; // even though value is an uint240 it can store addresses - // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal - // op and id take less than 1 byte each so it can be kept in 1 sstore - } function encodeParams(Param[] params) internal pure returns (uint256[]) { uint256[] memory encodedParams = new uint256[](params.length); From 9c84dc3330e207cd3db05d544f1c47d2c5e50be8 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 20:48:01 -0500 Subject: [PATCH 27/49] Add encodeOperator and encodeIfElse tests --- contracts/test/TestACLHelpers.sol | 45 +++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index abaec988e..32d00bec4 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -1,10 +1,12 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; +import "./helpers/ACLHelper.sol"; import "../acl/ACLSyntaxSugar.sol"; +import "../acl/ACL.sol"; -contract TestACLHelpers is ACLHelpers { +contract TestACLHelpers is ACL, ACLHelper { function testEncodeParam() public { Param memory param = Param({ @@ -23,19 +25,33 @@ contract TestACLHelpers is ACLHelpers { } function testEncodeParams() public { - Param[] memory params = new Param[](2); + Param[] memory params = new Param[](6); params[0] = Param({ - id: 1, - op: uint8(Op.EQ), - value: 5294967297 + id: LOGIC_OP_PARAM_ID, + op: uint8(Op.IF_ELSE), + value: encodeIfElse(1, 2, 3) }); params[1] = Param({ + id: LOGIC_OP_PARAM_ID, + op: uint8(Op.AND), + value: encodeOperator(2, 3) + }); + + params[2] = Param({ id: 2, op: uint8(Op.EQ), - value: 5294967297 - }); + value: 1 + }); + + params[3] = Param({ + id: 3, + op: uint8(Op.NEQ), + value: 2 + }); + + uint256[] memory encodedParam = encodeParams(params); @@ -50,7 +66,20 @@ contract TestACLHelpers is ACLHelpers { Assert.equal(uint256(params[1].id), uint256(id1), "Encoded id is not equal"); Assert.equal(uint256(params[1].op), uint256(op1), "Encoded op is not equal"); - Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); + Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); + + (uint32 id2, uint32 op2, uint32 value2) = decodeParamsList(encodedParam[2]); + + Assert.equal(uint256(params[2].id), uint256(id2), "Encoded id is not equal"); + Assert.equal(uint256(params[2].op), uint256(op2), "Encoded op is not equal"); + Assert.equal(uint256(params[2].value), uint256(value2), "Encoded value is not equal"); + + (uint32 id3, uint32 op3, uint32 value3) = decodeParamsList(encodedParam[3]); + + Assert.equal(uint256(params[3].id), uint256(id3), "Encoded id is not equal"); + Assert.equal(uint256(params[3].op), uint256(op3), "Encoded op is not equal"); + Assert.equal(uint256(params[3].value), uint256(value3), "Encoded value is not equal"); + } } From dbce97e0796e2a23421eb2c1cf4d87b7a497cdc5 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 21:02:22 -0500 Subject: [PATCH 28/49] Compact params --- contracts/test/TestACLHelpers.sol | 35 +++++-------------------------- 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 32d00bec4..9b39cd72a 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -9,11 +9,7 @@ import "../acl/ACL.sol"; contract TestACLHelpers is ACL, ACLHelper { function testEncodeParam() public { - Param memory param = Param({ - id: 2, - op: uint8(Op.EQ), - value: 5294967297 - }); + Param memory param = Param(2, uint8(Op.EQ), 5294967297); uint256 encodedParam = encodeParam(param); @@ -27,31 +23,10 @@ contract TestACLHelpers is ACL, ACLHelper { function testEncodeParams() public { Param[] memory params = new Param[](6); - params[0] = Param({ - id: LOGIC_OP_PARAM_ID, - op: uint8(Op.IF_ELSE), - value: encodeIfElse(1, 2, 3) - }); - - params[1] = Param({ - id: LOGIC_OP_PARAM_ID, - op: uint8(Op.AND), - value: encodeOperator(2, 3) - }); - - params[2] = Param({ - id: 2, - op: uint8(Op.EQ), - value: 1 - }); - - params[3] = Param({ - id: 3, - op: uint8(Op.NEQ), - value: 2 - }); - - + params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); + params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); + params[2] = Param(2, uint8(Op.EQ), 1); + params[3] = Param(3, uint8(Op.NEQ), 2); uint256[] memory encodedParam = encodeParams(params); From dd2f8f2cb8e2a3a8a64392eddf57665598f73876 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 19 Nov 2018 23:10:23 -0500 Subject: [PATCH 29/49] Expose ACLHelpers functions --- contracts/acl/ACLSyntaxSugar.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 069fa6cce..1c04e6d0d 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -104,15 +104,15 @@ contract ACLHelpers is ACLParams { return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; } - function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { + function decodeParamOp(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } - function decodeParamId(uint256 _x) internal pure returns (uint8 b) { + function decodeParamId(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 31)); } - function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { + function decodeParamsList(uint256 _x) public pure returns (uint32 a, uint32 b, uint32 c) { a = uint32(_x); b = uint32(_x >> (8 * 4)); c = uint32(_x >> (8 * 8)); From 22ec76466d2ba72ffd649f19f04bfc195cf9b870 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Tue, 20 Nov 2018 09:00:37 -0500 Subject: [PATCH 30/49] Fix linter issues --- contracts/acl/ACLSyntaxSugar.sol | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 1c04e6d0d..ef9836fd4 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -90,20 +90,6 @@ contract ACLSyntaxSugar { contract ACLHelpers is ACLParams { - function encodeParams(Param[] params) internal pure returns (uint256[]) { - uint256[] memory encodedParams = new uint256[](params.length); - - for (uint i = 0; i < params.length; i++) { - encodedParams[i] = encodeParam(params[i]); - } - - return encodedParams; - } - - function encodeParam(Param param) internal pure returns (uint256) { - return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; - } - function decodeParamOp(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } @@ -117,4 +103,18 @@ contract ACLHelpers is ACLParams { b = uint32(_x >> (8 * 4)); c = uint32(_x >> (8 * 8)); } + + function encodeParams(Param[] params) internal pure returns (uint256[]) { + uint256[] memory encodedParams = new uint256[](params.length); + + for (uint i = 0; i < params.length; i++) { + encodedParams[i] = encodeParam(params[i]); + } + + return encodedParams; + } + + function encodeParam(Param param) internal pure returns (uint256) { + return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; + } } From 93e5ba13be9dad73d4ecc047e01c4f3b4f8de5ca Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Sun, 3 Feb 2019 16:43:04 -0500 Subject: [PATCH 31/49] Fix spaces and array size --- contracts/acl/ACLSyntaxSugar.sol | 2 -- contracts/test/TestACLHelpers.sol | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index ef9836fd4..b606297b6 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -88,8 +88,6 @@ contract ACLSyntaxSugar { contract ACLHelpers is ACLParams { - - function decodeParamOp(uint256 _x) public pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 9b39cd72a..71ec45d64 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -21,7 +21,7 @@ contract TestACLHelpers is ACL, ACLHelper { } function testEncodeParams() public { - Param[] memory params = new Param[](6); + Param[] memory params = new Param[](4); params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); @@ -31,6 +31,7 @@ contract TestACLHelpers is ACL, ACLHelper { uint256[] memory encodedParam = encodeParams(params); + (uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]); Assert.equal(uint256(params[0].id), uint256(id0), "Encoded id is not equal"); From ad1b7f77da92c4a7189d122924a8766ed2388c80 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Sun, 3 Feb 2019 17:20:28 -0500 Subject: [PATCH 32/49] Use loop for testEncodeParams --- contracts/test/TestACLHelpers.sol | 32 +++++++------------------------ 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 71ec45d64..de7db6478 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -26,35 +26,17 @@ contract TestACLHelpers is ACL, ACLHelper { params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); params[2] = Param(2, uint8(Op.EQ), 1); - params[3] = Param(3, uint8(Op.NEQ), 2); - + params[3] = Param(3, uint8(Op.NEQ), 2); uint256[] memory encodedParam = encodeParams(params); - - (uint32 id0, uint32 op0, uint32 value0) = decodeParamsList(encodedParam[0]); - - Assert.equal(uint256(params[0].id), uint256(id0), "Encoded id is not equal"); - Assert.equal(uint256(params[0].op), uint256(op0), "Encoded op is not equal"); - Assert.equal(uint256(params[0].value), uint256(value0), "Encoded value is not equal"); - - (uint32 id1, uint32 op1, uint32 value1) = decodeParamsList(encodedParam[1]); - - Assert.equal(uint256(params[1].id), uint256(id1), "Encoded id is not equal"); - Assert.equal(uint256(params[1].op), uint256(op1), "Encoded op is not equal"); - Assert.equal(uint256(params[1].value), uint256(value1), "Encoded value is not equal"); - - (uint32 id2, uint32 op2, uint32 value2) = decodeParamsList(encodedParam[2]); - - Assert.equal(uint256(params[2].id), uint256(id2), "Encoded id is not equal"); - Assert.equal(uint256(params[2].op), uint256(op2), "Encoded op is not equal"); - Assert.equal(uint256(params[2].value), uint256(value2), "Encoded value is not equal"); - - (uint32 id3, uint32 op3, uint32 value3) = decodeParamsList(encodedParam[3]); + for (uint256 i = 0; i < 4; i++) { + (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam[i]); - Assert.equal(uint256(params[3].id), uint256(id3), "Encoded id is not equal"); - Assert.equal(uint256(params[3].op), uint256(op3), "Encoded op is not equal"); - Assert.equal(uint256(params[3].value), uint256(value3), "Encoded value is not equal"); + Assert.equal(uint256(params[i].id), uint256(id), "Encoded id is not equal"); + Assert.equal(uint256(params[i].op), uint256(op), "Encoded op is not equal"); + Assert.equal(uint256(params[i].value), uint256(value), "Encoded value is not equal"); + } } From c4e0fb172c925c64f07d5216dec0c36bc0846a6d Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 7 Feb 2019 17:03:26 +0100 Subject: [PATCH 33/49] chore: pin ganache-cli to 6.2.3 (#472) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 9fade8b5b..72e041694 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "eth-ens-namehash": "^2.0.8", "eth-gas-reporter": "^0.1.1", "ethereumjs-abi": "^0.6.5", - "ganache-cli": "^6.2.0", + "ganache-cli": "6.2.3", "mocha-lcov-reporter": "^1.3.0", "solidity-coverage": "0.5.8", "solium": "^1.1.8", From a026e222e3a504cc4b47a1df0a3b69e6574b5bed Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 12 Feb 2019 00:08:00 +0100 Subject: [PATCH 34/49] feat: Add SafeERC20 (#469) --- .solcover.js | 1 + contracts/common/SafeERC20.sol | 156 +++++++++++++++++ contracts/common/VaultRecoverable.sol | 9 +- contracts/test/mocks/SafeERC20Mock.sol | 36 ++++ contracts/test/mocks/TokenMock.sol | 62 ++++++- contracts/test/mocks/TokenReturnFalseMock.sol | 110 ++++++++++++ .../test/mocks/TokenReturnMissingMock.sol | 105 +++++++++++ ..._recover_funds.js => recovery_to_vault.js} | 165 ++++++++++++++---- test/safe_erc20.js | 130 ++++++++++++++ test/time_helpers.js | 2 +- 10 files changed, 740 insertions(+), 36 deletions(-) create mode 100644 contracts/common/SafeERC20.sol create mode 100644 contracts/test/mocks/SafeERC20Mock.sol create mode 100644 contracts/test/mocks/TokenReturnFalseMock.sol create mode 100644 contracts/test/mocks/TokenReturnMissingMock.sol rename test/{proxy_recover_funds.js => recovery_to_vault.js} (65%) create mode 100644 test/safe_erc20.js diff --git a/.solcover.js b/.solcover.js index 706360613..38a69357e 100644 --- a/.solcover.js +++ b/.solcover.js @@ -3,6 +3,7 @@ const skipFiles = [ 'test', 'acl/ACLSyntaxSugar.sol', 'common/DepositableStorage.sol', // Used in tests that send ETH + 'common/SafeERC20.sol', // solidity-coverage fails on assembly if (https://github.com/sc-forks/solidity-coverage/issues/287) 'common/UnstructuredStorage.sol' // Used in tests that send ETH ] diff --git a/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol new file mode 100644 index 000000000..fa11c0921 --- /dev/null +++ b/contracts/common/SafeERC20.sol @@ -0,0 +1,156 @@ +// Inspired by AdEx (https://github.com/AdExNetwork/adex-protocol-eth/blob/b9df617829661a7518ee10f4cb6c4108659dd6d5/contracts/libs/SafeERC20.sol) +// and 0x (https://github.com/0xProject/0x-monorepo/blob/737d1dc54d72872e24abce5a1dbe1b66d35fa21a/contracts/protocol/contracts/protocol/AssetProxy/ERC20Proxy.sol#L143) + +pragma solidity ^0.4.24; + +import "../lib/token/ERC20.sol"; + + +library SafeERC20 { + // Before 0.5, solidity has a mismatch between `address.transfer()` and `token.transfer()`: + // https://github.com/ethereum/solidity/issues/3544 + bytes4 private constant TRANSFER_SELECTOR = 0xa9059cbb; + + string private constant ERROR_TOKEN_BALANCE_REVERTED = "SAFE_ERC_20_BALANCE_REVERTED"; + string private constant ERROR_TOKEN_ALLOWANCE_REVERTED = "SAFE_ERC_20_ALLOWANCE_REVERTED"; + + function invokeAndCheckSuccess(address _addr, bytes memory _calldata) + private + returns (bool) + { + bool ret; + assembly { + let ptr := mload(0x40) // free memory pointer + + let success := call( + gas, // forward all gas + _addr, // address + 0, // no value + add(_calldata, 0x20), // calldata start + mload(_calldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + // Check number of bytes returned from last function call + switch returndatasize + + // No bytes returned: assume success + case 0 { + ret := 1 + } + + // 32 bytes returned: check if non-zero + case 0x20 { + // Only return success if returned data was true + // Already have output in ptr + ret := eq(mload(ptr), 1) + } + + // Not sure what was returned: don't mark as success + default { } + } + } + return ret; + } + + function staticInvoke(address _addr, bytes memory _calldata) + private + view + returns (bool, uint256) + { + bool success; + uint256 ret; + assembly { + let ptr := mload(0x40) // free memory pointer + + success := staticcall( + gas, // forward all gas + _addr, // address + add(_calldata, 0x20), // calldata start + mload(_calldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + ret := mload(ptr) + } + } + return (success, ret); + } + + /** + * @dev Same as a standards-compliant ERC20.transfer() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeTransfer(ERC20 _token, address _to, uint256 _amount) internal returns (bool) { + bytes memory transferCallData = abi.encodeWithSelector( + TRANSFER_SELECTOR, + _to, + _amount + ); + return invokeAndCheckSuccess(_token, transferCallData); + } + + /** + * @dev Same as a standards-compliant ERC20.transferFrom() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeTransferFrom(ERC20 _token, address _from, address _to, uint256 _amount) internal returns (bool) { + bytes memory transferFromCallData = abi.encodeWithSelector( + _token.transferFrom.selector, + _from, + _to, + _amount + ); + return invokeAndCheckSuccess(_token, transferFromCallData); + } + + /** + * @dev Same as a standards-compliant ERC20.approve() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeApprove(ERC20 _token, address _spender, uint256 _amount) internal returns (bool) { + bytes memory approveCallData = abi.encodeWithSelector( + _token.approve.selector, + _spender, + _amount + ); + return invokeAndCheckSuccess(_token, approveCallData); + } + + /** + * @dev Static call into ERC20.balanceOf(). + * Reverts if the call fails for some reason (should never fail). + */ + function staticBalanceOf(ERC20 _token, address _owner) internal view returns (uint256) { + bytes memory balanceOfCallData = abi.encodeWithSelector( + _token.balanceOf.selector, + _owner + ); + + (bool success, uint256 tokenBalance) = staticInvoke(_token, balanceOfCallData); + require(success, ERROR_TOKEN_BALANCE_REVERTED); + + return tokenBalance; + } + + /** + * @dev Static call into ERC20.allowance(). + * Reverts if the call fails for some reason (should never fail). + */ + function staticAllowance(ERC20 _token, address _owner, address _spender) internal view returns (uint256) { + bytes memory allowanceCallData = abi.encodeWithSelector( + _token.allowance.selector, + _owner, + _spender + ); + + (bool success, uint256 allowance) = staticInvoke(_token, allowanceCallData); + require(success, ERROR_TOKEN_ALLOWANCE_REVERTED); + + return allowance; + } +} diff --git a/contracts/common/VaultRecoverable.sol b/contracts/common/VaultRecoverable.sol index c298d66b4..810ac0cb3 100644 --- a/contracts/common/VaultRecoverable.sol +++ b/contracts/common/VaultRecoverable.sol @@ -8,11 +8,15 @@ import "../lib/token/ERC20.sol"; import "./EtherTokenConstant.sol"; import "./IsContract.sol"; import "./IVaultRecoverable.sol"; +import "./SafeERC20.sol"; contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { + using SafeERC20 for ERC20; + string private constant ERROR_DISALLOWED = "RECOVER_DISALLOWED"; string private constant ERROR_VAULT_NOT_CONTRACT = "RECOVER_VAULT_NOT_CONTRACT"; + string private constant ERROR_TOKEN_TRANSFER_FAILED = "RECOVER_TOKEN_TRANSFER_FAILED"; /** * @notice Send funds to recovery Vault. This contract should never receive funds, @@ -27,8 +31,9 @@ contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { if (_token == ETH) { vault.transfer(address(this).balance); } else { - uint256 amount = ERC20(_token).balanceOf(this); - ERC20(_token).transfer(vault, amount); + ERC20 token = ERC20(_token); + uint256 amount = token.staticBalanceOf(this); + require(token.safeTransfer(vault, amount), ERROR_TOKEN_TRANSFER_FAILED); } } diff --git a/contracts/test/mocks/SafeERC20Mock.sol b/contracts/test/mocks/SafeERC20Mock.sol new file mode 100644 index 000000000..92b660ced --- /dev/null +++ b/contracts/test/mocks/SafeERC20Mock.sol @@ -0,0 +1,36 @@ +pragma solidity 0.4.24; + +import "../../common/SafeERC20.sol"; +import "../../lib/token/ERC20.sol"; + + +contract SafeERC20Mock { + using SafeERC20 for ERC20; + event Result(bool result); + + function transfer(ERC20 token, address to, uint256 amount) external returns (bool) { + bool result = token.safeTransfer(to, amount); + emit Result(result); + return result; + } + + function transferFrom(ERC20 token, address from, address to, uint256 amount) external returns (bool) { + bool result = token.safeTransferFrom(from, to, amount); + emit Result(result); + return result; + } + + function approve(ERC20 token, address spender, uint256 amount) external returns (bool) { + bool result = token.safeApprove(spender, amount); + emit Result(result); + return result; + } + + function allowance(ERC20 token, address owner, address spender) external view returns (uint256) { + return token.staticAllowance(owner, spender); + } + + function balanceOf(ERC20 token, address owner) external view returns (uint256) { + return token.staticBalanceOf(owner); + } +} diff --git a/contracts/test/mocks/TokenMock.sol b/contracts/test/mocks/TokenMock.sol index 7442d80be..a93f777aa 100644 --- a/contracts/test/mocks/TokenMock.sol +++ b/contracts/test/mocks/TokenMock.sol @@ -1,4 +1,4 @@ -// Stripped from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol pragma solidity 0.4.24; @@ -8,14 +8,18 @@ import "../../lib/math/SafeMath.sol"; contract TokenMock { using SafeMath for uint256; mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; uint256 private totalSupply_; + bool private allowTransfer_; + event Approval(address indexed owner, address indexed spender, uint256 value); event Transfer(address indexed from, address indexed to, uint256 value); // Allow us to set the inital balance for an account on construction constructor(address initialAccount, uint256 initialBalance) public { balances[initialAccount] = initialBalance; totalSupply_ = initialBalance; + allowTransfer_ = true; } function totalSupply() public view returns (uint256) { return totalSupply_; } @@ -29,12 +33,31 @@ contract TokenMock { return balances[_owner]; } + /** + * @dev Function to check the amount of tokens that an owner allowed to a spender. + * @param _owner address The address which owns the funds. + * @param _spender address The address which will spend the funds. + * @return A uint256 specifying the amount of tokens still available for the spender. + */ + function allowance(address _owner, address _spender) public view returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + /** * @dev Transfer token for a specified address * @param _to The address to transfer to. * @param _value The amount to be transferred. */ function transfer(address _to, uint256 _value) public returns (bool) { + require(allowTransfer_); require(_value <= balances[msg.sender]); require(_to != address(0)); @@ -43,4 +66,41 @@ contract TokenMock { emit Transfer(msg.sender, _to, _value); return true; } + + /** + * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + * Beware that changing an allowance with this method brings the risk that someone may use both the old + * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this + * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * @param _spender The address which will spend the funds. + * @param _value The amount of tokens to be spent. + */ + function approve(address _spender, uint256 _value) public returns (bool) { + // Assume we want to protect for the race condition + require(allowed[msg.sender][_spender] == 0); + + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + return true; + } + + /** + * @dev Transfer tokens from one address to another + * @param _from address The address which you want to send tokens from + * @param _to address The address which you want to transfer to + * @param _value uint256 the amount of tokens to be transferred + */ + function transferFrom(address _from, address _to, uint256 _value) public returns (bool) { + require(allowTransfer_); + require(_value <= balances[_from]); + require(_value <= allowed[_from][msg.sender]); + require(_to != address(0)); + + balances[_from] = balances[_from].sub(_value); + balances[_to] = balances[_to].add(_value); + allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value); + emit Transfer(_from, _to, _value); + return true; + } } diff --git a/contracts/test/mocks/TokenReturnFalseMock.sol b/contracts/test/mocks/TokenReturnFalseMock.sol new file mode 100644 index 000000000..6011fc033 --- /dev/null +++ b/contracts/test/mocks/TokenReturnFalseMock.sol @@ -0,0 +1,110 @@ +// Standards compliant token that is returns false instead of reverting for +// `transfer()`, `transferFrom()`, and `approve(). +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol + +pragma solidity 0.4.24; + + +contract TokenReturnFalseMock { + mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; + uint256 private totalSupply_; + bool private allowTransfer_; + + event Approval(address indexed owner, address indexed spender, uint256 value); + event Transfer(address indexed from, address indexed to, uint256 value); + + // Allow us to set the inital balance for an account on construction + constructor(address initialAccount, uint256 initialBalance) public { + balances[initialAccount] = initialBalance; + totalSupply_ = initialBalance; + allowTransfer_ = true; + } + + function totalSupply() public view returns (uint256) { return totalSupply_; } + + /** + * @dev Gets the balance of the specified address. + * @param _owner The address to query the the balance of. + * @return An uint256 representing the amount owned by the passed address. + */ + function balanceOf(address _owner) public view returns (uint256) { + return balances[_owner]; + } + + /** + * @dev Function to check the amount of tokens that an owner allowed to a spender. + * @param _owner address The address which owns the funds. + * @param _spender address The address which will spend the funds. + * @return A uint256 specifying the amount of tokens still available for the spender. + */ + function allowance(address _owner, address _spender) public view returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + + /** + * @dev Transfer token for a specified address + * @param _to The address to transfer to. + * @param _value The amount to be transferred. + */ + function transfer(address _to, uint256 _value) public returns (bool) { + if (!allowTransfer_ || _to == address(0) || _value > balances[msg.sender]) { + return false; + } + + balances[msg.sender] = balances[msg.sender] - _value; + balances[_to] = balances[_to] + _value; + emit Transfer(msg.sender, _to, _value); + return true; + } + + /** + * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + * Beware that changing an allowance with this method brings the risk that someone may use both the old + * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this + * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * @param _spender The address which will spend the funds. + * @param _value The amount of tokens to be spent. + */ + function approve(address _spender, uint256 _value) public returns (bool) { + // Assume we want to protect for the race condition + if (allowed[msg.sender][_spender] != 0) { + return false; + } + + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + return true; + } + + /** + * @dev Transfer tokens from one address to another + * @param _from address The address which you want to send tokens from + * @param _to address The address which you want to transfer to + * @param _value uint256 the amount of tokens to be transferred + */ + function transferFrom(address _from, address _to, uint256 _value) public returns (bool) { + if (!allowTransfer_ || + _to == address(0) || + _value > balances[_from] || + _value > allowed[_from][msg.sender] + ) { + return false; + } + + balances[_from] = balances[_from] - _value; + balances[_to] = balances[_to] + _value; + allowed[_from][msg.sender] = allowed[_from][msg.sender] - _value; + emit Transfer(_from, _to, _value); + return true; + } +} diff --git a/contracts/test/mocks/TokenReturnMissingMock.sol b/contracts/test/mocks/TokenReturnMissingMock.sol new file mode 100644 index 000000000..8b339b1c8 --- /dev/null +++ b/contracts/test/mocks/TokenReturnMissingMock.sol @@ -0,0 +1,105 @@ +// Non-standards compliant token that is missing return values for +// `transfer()`, `transferFrom()`, and `approve(). +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol + +pragma solidity 0.4.24; + +import "../../lib/math/SafeMath.sol"; + + +contract TokenReturnMissingMock { + using SafeMath for uint256; + mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; + uint256 private totalSupply_; + bool private allowTransfer_; + + event Approval(address indexed owner, address indexed spender, uint256 value); + event Transfer(address indexed from, address indexed to, uint256 value); + + // Allow us to set the inital balance for an account on construction + constructor(address initialAccount, uint256 initialBalance) public { + balances[initialAccount] = initialBalance; + totalSupply_ = initialBalance; + allowTransfer_ = true; + } + + function totalSupply() public view returns (uint256) { return totalSupply_; } + + /** + * @dev Gets the balance of the specified address. + * @param _owner The address to query the the balance of. + * @return An uint256 representing the amount owned by the passed address. + */ + function balanceOf(address _owner) public view returns (uint256) { + return balances[_owner]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + + /** + * @dev Function to check the amount of tokens that an owner allowed to a spender. + * @param _owner address The address which owns the funds. + * @param _spender address The address which will spend the funds. + * @return A uint256 specifying the amount of tokens still available for the spender. + */ + function allowance(address _owner, address _spender) public view returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @dev Transfer token for a specified address + * @param _to The address to transfer to. + * @param _value The amount to be transferred. + */ + function transfer(address _to, uint256 _value) public { + require(allowTransfer_); + require(_value <= balances[msg.sender]); + require(_to != address(0)); + + balances[msg.sender] = balances[msg.sender].sub(_value); + balances[_to] = balances[_to].add(_value); + emit Transfer(msg.sender, _to, _value); + } + + /** + * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + * Beware that changing an allowance with this method brings the risk that someone may use both the old + * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this + * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * @param _spender The address which will spend the funds. + * @param _value The amount of tokens to be spent. + */ + function approve(address _spender, uint256 _value) public { + // Assume we want to protect for the race condition + require(allowed[msg.sender][_spender] == 0); + + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + } + + /** + * @dev Transfer tokens from one address to another + * @param _from address The address which you want to send tokens from + * @param _to address The address which you want to transfer to + * @param _value uint256 the amount of tokens to be transferred + */ + function transferFrom(address _from, address _to, uint256 _value) public { + require(allowTransfer_); + require(_value <= balances[_from]); + require(_value <= allowed[_from][msg.sender]); + require(_to != address(0)); + + balances[_from] = balances[_from].sub(_value); + balances[_to] = balances[_to].add(_value); + allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value); + emit Transfer(_from, _to, _value); + } +} diff --git a/test/proxy_recover_funds.js b/test/recovery_to_vault.js similarity index 65% rename from test/proxy_recover_funds.js rename to test/recovery_to_vault.js index 94bdddd9a..678c90fd0 100644 --- a/test/proxy_recover_funds.js +++ b/test/recovery_to_vault.js @@ -14,6 +14,8 @@ const AppStubDepositable = artifacts.require('AppStubDepositable') const AppStubConditionalRecovery = artifacts.require('AppStubConditionalRecovery') const EtherTokenConstantMock = artifacts.require('EtherTokenConstantMock') const TokenMock = artifacts.require('TokenMock') +const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock') +const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock') const VaultMock = artifacts.require('VaultMock') const KernelDepositableMock = artifacts.require('KernelDepositableMock') @@ -22,34 +24,70 @@ const getEvent = (receipt, event, arg) => { return receipt.logs.filter(l => l.ev const APP_ID = hash('stub.aragonpm.test') const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies -contract('Proxy funds', accounts => { +contract('Recovery to vault', accounts => { let aclBase, appBase, appConditionalRecoveryBase let APP_ADDR_NAMESPACE, ETH const permissionsRoot = accounts[0] // Helpers - const recoverEth = async (target, vault) => { + const recoverEth = async ({ shouldFail, target, vault }) => { const amount = 1 const initialBalance = await getBalance(target.address) const initialVaultBalance = await getBalance(vault.address) const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) - await target.transferToVault(ETH) - assert.equal((await getBalance(target.address)).valueOf(), 0) - assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount).valueOf()) + + const recoverAction = () => target.transferToVault(ETH) + + if (shouldFail) { + await assertRevert(recoverAction) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance) + } else { + await recoverAction() + assert.equal((await getBalance(target.address)).valueOf(), 0) + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount)) + } + } + + const recoverTokens = async ({ shouldFail, tokenContract, target, vault }) => { + const amount = 1 + const token = await tokenContract.new(accounts[0], 1000) + const initialBalance = await token.balanceOf(target.address) + const initialVaultBalance = await token.balanceOf(vault.address) + await token.transfer(target.address, amount) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + + const recoverAction = () => target.transferToVault(token.address) + + if (shouldFail) { + await assertRevert(recoverAction) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance) + } else { + await recoverAction() + assert.equal((await token.balanceOf(target.address)).valueOf(), 0) + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount)) + } } - const recoverTokens = async (target, vault) => { + const failingRecoverTokens = async ({ tokenContract, target, vault }) => { const amount = 1 - const token = await TokenMock.new(accounts[0], 1000) + const token = await tokenContract.new(accounts[0], 1000) const initialBalance = await token.balanceOf(target.address) const initialVaultBalance = await token.balanceOf(vault.address) await token.transfer(target.address, amount) assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) - await target.transferToVault(token.address) - assert.equal((await token.balanceOf(target.address)).valueOf(), 0) - assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount).valueOf()) + + // Stop token from being transferable + await token.setAllowTransfer(false) + + // Try to transfer + await assertRevert(() => target.transferToVault(token.address)) + + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance) } const failWithoutVault = async (target, kernel) => { @@ -64,6 +102,22 @@ contract('Proxy funds', accounts => { }) } + // Token test groups + const tokenTestGroups = [ + { + title: 'standards compliant, reverting token', + tokenContract: TokenMock, + }, + { + title: 'standards compliant, non-reverting token', + tokenContract: TokenReturnFalseMock, + }, + { + title: 'non-standards compliant, missing return token', + tokenContract: TokenReturnMissingMock, + }, + ] + // Initial setup before(async () => { aclBase = await ACL.new() @@ -141,9 +195,25 @@ contract('Proxy funds', accounts => { ) ) - it('kernel recovers tokens', async () => { - await recoverTokens(kernel, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`kernel recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + vault, + target: kernel + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`kernel reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + vault, + target: kernel, + }) + }) + } context('> App without kernel', () => { beforeEach(async () => { @@ -152,15 +222,16 @@ contract('Proxy funds', accounts => { }) it('does not recover ETH', skipCoverageIfVaultProxy(async () => - await assertRevert( - () => recoverEth(target, vault) - ) + await recoverEth({ target, vault, shouldFail: true }) )) it('does not recover tokens', async () => - await assertRevert( - () => recoverTokens(target, vault) - ) + await recoverTokens({ + target, + vault, + shouldFail: true, + tokenContract: TokenMock, + }) ) }) @@ -187,13 +258,29 @@ contract('Proxy funds', accounts => { }) }) - it('recovers ETH', skipCoverageIfVaultProxy( - async () => await recoverEth(target, vault) + it('recovers ETH', skipCoverageIfVaultProxy(async () => + await recoverEth({ target, vault }) )) - it('recovers tokens', async () => { - await recoverTokens(target, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + target, + vault, + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + target, + vault, + }) + }) + } it('fails if vault is not contract', async () => { await failWithoutVault(target, kernel) @@ -211,16 +298,30 @@ contract('Proxy funds', accounts => { target = app }) - it('does not allow recovering ETH', skipCoverageIfVaultProxy( + it('does not allow recovering ETH', skipCoverageIfVaultProxy(async () => // Conditional stub doesnt allow eth recoveries - () => assertRevert( - () => recoverEth(target, vault) - ) + await recoverEth({ target, vault, shouldFail: true }) )) - it('allows recovering tokens', async () => { - await recoverTokens(target, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`allows recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + target, + vault, + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + target, + vault, + }) + }) + } }) }) } @@ -255,7 +356,7 @@ contract('Proxy funds', accounts => { }) it('recovers ETH from the kernel', skipCoverage(async () => { - await recoverEth(kernel, vault) + await recoverEth({ target: kernel, vault }) })) }) }) diff --git a/test/safe_erc20.js b/test/safe_erc20.js new file mode 100644 index 000000000..7996f091e --- /dev/null +++ b/test/safe_erc20.js @@ -0,0 +1,130 @@ +const { assertRevert } = require('./helpers/assertThrow') + +// Mocks +const SafeERC20Mock = artifacts.require('SafeERC20Mock') +const TokenMock = artifacts.require('TokenMock') +const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock') +const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock') + +const assertMockResult = (receipt, result) => { + const events = receipt.logs.filter(x => x.event == 'Result') + const eventArg = events[0].args.result + assert.equal(eventArg, result, `Result not expected (got ${eventArg} instead of ${result})`) +} + +contract('SafeERC20', accounts => { + const [owner, receiver] = accounts + const initialBalance = 10000 + let safeERC20Mock + + before(async () => { + safeERC20Mock = await SafeERC20Mock.new() + }) + + const testGroups = [ + { + title: 'Standards compliant, reverting token', + tokenContract: TokenMock, + }, + { + title: 'Standards compliant, non-reverting token', + tokenContract: TokenReturnFalseMock, + }, + { + title: 'Non-standards compliant, missing return token', + tokenContract: TokenReturnMissingMock, + }, + ] + + for ({ title, tokenContract} of testGroups) { + context(`> ${title}`, () => { + let tokenMock + + beforeEach(async () => { + tokenMock = await tokenContract.new(owner, initialBalance) + }) + + it('can correctly transfer', async () => { + // Add balance to the mock + const transferAmount = 5000 + await tokenMock.transfer(safeERC20Mock.address, transferAmount) + + // Transfer it all back from the mock + const receipt = await safeERC20Mock.transfer(tokenMock.address, owner, transferAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should be correct') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should be correct') + }) + + it('detects failed transfer', async () => { + // Attempt transfer when mock has no balance + const receipt = await safeERC20Mock.transfer(tokenMock.address, owner, 1000) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should stay the same') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('can correctly approve', async () => { + const approvedAmount = 5000 + + // Create approval from the mock + const receipt = await safeERC20Mock.approve(tokenMock.address, receiver, approvedAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.allowance(safeERC20Mock.address, receiver)).valueOf(), approvedAmount, 'Allowance of receiver should be correct') + }) + + it('detects failed approve', async () => { + const preApprovedAmount = 5000 + + // Create pre-exisiting approval + await safeERC20Mock.approve(tokenMock.address, receiver, preApprovedAmount) + + // Attempt to create another approval without reseting it back to 0 + const receipt = await safeERC20Mock.approve(tokenMock.address, receiver, preApprovedAmount - 500) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.allowance(safeERC20Mock.address, receiver)).valueOf(), preApprovedAmount, 'Allowance of receiver should be the pre-existing value') + }) + + it('can correctly transferFrom', async () => { + // Create approval + const approvedAmount = 5000 + await tokenMock.approve(safeERC20Mock.address, approvedAmount) + + // Transfer to receiver through the mock + const receipt = await safeERC20Mock.transferFrom(tokenMock.address, owner, receiver, approvedAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance - approvedAmount, 'Balance of owner should be correct') + assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), approvedAmount, 'Balance of receiver should be correct') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('detects failed transferFrom', async () => { + // Attempt transfer to receiver through the mock when mock wasn't approved + const receipt = await safeERC20Mock.transferFrom(tokenMock.address, owner, receiver, 5000) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should stay the same') + assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), 0, 'Balance of receiver should stay the same') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('gives correct value with static allowance', async () => { + // Create approval + const approvedAmount = 5000 + await tokenMock.approve(safeERC20Mock.address, approvedAmount) + + const approval = (await safeERC20Mock.allowance(tokenMock.address, owner, safeERC20Mock.address)).valueOf() + assert.equal(approval, approvedAmount, 'Mock should return correct allowance') + assert.equal((await tokenMock.allowance(owner, safeERC20Mock.address)).valueOf(), approval, "Mock should match token contract's allowance") + }) + + it('gives correct value with static balanceOf', async () => { + const balance = (await safeERC20Mock.balanceOf(tokenMock.address, owner)).valueOf() + assert.equal(balance, initialBalance, 'Mock should return correct balance') + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), balance, "Mock should match token contract's balance") + }) + }) + } +}) diff --git a/test/time_helpers.js b/test/time_helpers.js index 828640260..599afe88b 100644 --- a/test/time_helpers.js +++ b/test/time_helpers.js @@ -1,4 +1,4 @@ -contract('TimeHelpers test', accounts => { +contract('TimeHelpers', accounts => { let timeHelpersMock before(async () => { From 5c61d10ad6b20e64f662154cc456f451656a2618 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 12 Feb 2019 00:08:52 +0100 Subject: [PATCH 35/49] tests: Coverage improvements (#474) * APMRegistry: fix tests checking APMRegistry doesn't init without required permissions * EVMScriptRegistry: add more tests for coverage * ENSSubdomainRegistrary: fix initialization test --- .../test/mocks/APMRegistryFactoryMock.sol | 74 ++++++++++--------- test/apm_registry.js | 29 +++++--- test/ens_subdomains.js | 18 +++-- test/evm_script.js | 34 ++++----- 4 files changed, 89 insertions(+), 66 deletions(-) diff --git a/contracts/test/mocks/APMRegistryFactoryMock.sol b/contracts/test/mocks/APMRegistryFactoryMock.sol index cadd24c0a..455948b59 100644 --- a/contracts/test/mocks/APMRegistryFactoryMock.sol +++ b/contracts/test/mocks/APMRegistryFactoryMock.sol @@ -1,31 +1,49 @@ pragma solidity 0.4.24; +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()); @@ -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; diff --git a/test/apm_registry.js b/test/apm_registry.js index 810428fc8..fa83e14a3 100644 --- a/test/apm_registry.js +++ b/test/apm_registry.js @@ -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 }) @@ -103,7 +114,7 @@ contract('APMRegistry', accounts => { }) }) - context('creating test.aragonpm.eth repo', () => { + context('> Creating test.aragonpm.eth repo', () => { let repo = {} beforeEach(async () => { @@ -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) }) }) }) diff --git a/test/ens_subdomains.js b/test/ens_subdomains.js index 708bc605b..62156bab2 100644 --- a/test/ens_subdomains.js +++ b/test/ens_subdomains.js @@ -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] @@ -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 () => { @@ -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()) @@ -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) }) }) }) diff --git a/test/evm_script.js b/test/evm_script.js index f3c5bd4e2..9615e02c2 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -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 () => { @@ -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 () => { @@ -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 }) @@ -326,37 +336,23 @@ contract('EVM Script', accounts => { }) }) - context('registry', () => { - it('can be disabled', async () => { + context('> Registry actions', () => { + 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([])) }) }) From c37d4fd11fa096f85bd0f80dedf0e86bb5967adc Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 12 Feb 2019 00:10:10 +0100 Subject: [PATCH 36/49] 4.1.0-rc.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 72e041694..8847f133f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@aragon/os", - "version": "4.0.1", + "version": "4.1.0-rc.1", "description": "Core contracts for Aragon", "scripts": { "compile": "truffle compile", From 3d8c54f1b4526acce54e4d05f5a1e242a5853d95 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Tue, 12 Feb 2019 17:31:49 +0100 Subject: [PATCH 37/49] chore: upgrade solium (#476) --- contracts/acl/ACLSyntaxSugar.sol | 4 +++- contracts/apps/AppProxyUpgradeable.sol | 2 +- package.json | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 38ca29fdc..5e5fe9028 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -6,7 +6,9 @@ pragma solidity ^0.4.24; contract ACLSyntaxSugar { - function arr() internal pure returns (uint256[]) {} + function arr() internal pure returns (uint256[]) { + // solium-disable-previous-line no-empty-blocks + } function arr(bytes32 _a) internal pure returns (uint256[] r) { return arr(uint256(_a)); diff --git a/contracts/apps/AppProxyUpgradeable.sol b/contracts/apps/AppProxyUpgradeable.sol index 60d4cfa0c..84ca19578 100644 --- a/contracts/apps/AppProxyUpgradeable.sol +++ b/contracts/apps/AppProxyUpgradeable.sol @@ -14,7 +14,7 @@ contract AppProxyUpgradeable is AppProxyBase { AppProxyBase(_kernel, _appId, _initializePayload) public // solium-disable-line visibility-first { - + // solium-disable-previous-line no-empty-blocks } /** diff --git a/package.json b/package.json index 8847f133f..97e2b9e08 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,7 @@ "ganache-cli": "6.2.3", "mocha-lcov-reporter": "^1.3.0", "solidity-coverage": "0.5.8", - "solium": "^1.1.8", + "solium": "^1.2.3", "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1", "truffle-extract": "^1.2.1", From 8cf60391c55e16be2c7b3a4723e5cfddfee9ffb2 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Wed, 13 Feb 2019 16:50:07 -0500 Subject: [PATCH 38/49] Convert back ACLHelpers functions to internal --- contracts/acl/ACLSyntaxSugar.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 128d701d1..f84be0489 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -90,15 +90,15 @@ contract ACLSyntaxSugar { contract ACLHelpers is ACLParams { - function decodeParamOp(uint256 _x) public pure returns (uint8 b) { + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 30)); } - function decodeParamId(uint256 _x) public pure returns (uint8 b) { + function decodeParamId(uint256 _x) internal pure returns (uint8 b) { return uint8(_x >> (8 * 31)); } - function decodeParamsList(uint256 _x) public pure returns (uint32 a, uint32 b, uint32 c) { + function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { a = uint32(_x); b = uint32(_x >> (8 * 4)); c = uint32(_x >> (8 * 8)); From 3c909ef7ebc6399565fb3f30587c804c8e6ee795 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Wed, 13 Feb 2019 23:10:14 -0500 Subject: [PATCH 39/49] Expose encodeOperator and encodeIfElse in ACLHelpers --- contracts/acl/ACLSyntaxSugar.sol | 10 +++++++++- contracts/test/TestACLHelpers.sol | 2 +- contracts/test/TestACLInterpreter.sol | 2 +- contracts/test/helpers/ACLHelper.sol | 11 ----------- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index f84be0489..24fba6641 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -116,5 +116,13 @@ contract ACLHelpers is ACLParams { function encodeParam(Param param) internal pure returns (uint256) { return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; - } + } + + function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) { + return uint240(param1 + (param2 << 32) + (0 << 64)); + } + + function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) { + return uint240(condition + (successParam << 32) + (failureParam << 64)); + } } diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index de7db6478..8c7985641 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -6,7 +6,7 @@ import "../acl/ACLSyntaxSugar.sol"; import "../acl/ACL.sol"; -contract TestACLHelpers is ACL, ACLHelper { +contract TestACLHelpers is ACL { function testEncodeParam() public { Param memory param = Param(2, uint8(Op.EQ), 5294967297); diff --git a/contracts/test/TestACLInterpreter.sol b/contracts/test/TestACLInterpreter.sol index ca909e91e..4e9d90cae 100644 --- a/contracts/test/TestACLInterpreter.sol +++ b/contracts/test/TestACLInterpreter.sol @@ -5,7 +5,7 @@ import "./helpers/Assert.sol"; import "./helpers/ACLHelper.sol"; -contract TestACLInterpreter is ACL, ACLHelper { +contract TestACLInterpreter is ACL { function testEqualityUint() public { // Assert param 0 is equal to 10, given that params are [10, 11] assertEval(arr(uint256(10), 11), 0, Op.EQ, 10, true); diff --git a/contracts/test/helpers/ACLHelper.sol b/contracts/test/helpers/ACLHelper.sol index 404978572..74241755c 100644 --- a/contracts/test/helpers/ACLHelper.sol +++ b/contracts/test/helpers/ACLHelper.sol @@ -3,17 +3,6 @@ pragma solidity 0.4.24; import "../../acl/IACLOracle.sol"; -contract ACLHelper { - function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) { - return encodeIfElse(param1, param2, 0); - } - - function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) { - return uint240(condition + (successParam << 32) + (failureParam << 64)); - } -} - - contract AcceptOracle is IACLOracle { function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { return true; From 584f4bc87619dbf3912a4dd1c476a67a1892d4a3 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 18 Feb 2019 02:58:19 -0500 Subject: [PATCH 40/49] Add MIT license to ACLParams --- contracts/acl/ACLParams.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/acl/ACLParams.sol b/contracts/acl/ACLParams.sol index a449e8d11..e26c96e71 100644 --- a/contracts/acl/ACLParams.sol +++ b/contracts/acl/ACLParams.sol @@ -1,3 +1,7 @@ +/* + * SPDX-License-Identitifer: MIT + */ + pragma solidity ^0.4.24; From 05d3d269b7db2fba13a74c1a632f8985eafd1e60 Mon Sep 17 00:00:00 2001 From: Maxime Cote Date: Mon, 18 Feb 2019 03:14:19 -0500 Subject: [PATCH 41/49] Rename ACLHelper to ACLOracleHelper --- contracts/test/TestACLHelpers.sol | 1 - contracts/test/TestACLInterpreter.sol | 2 +- contracts/test/helpers/{ACLHelper.sol => ACLOracleHelper.sol} | 0 3 files changed, 1 insertion(+), 2 deletions(-) rename contracts/test/helpers/{ACLHelper.sol => ACLOracleHelper.sol} (100%) diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 8c7985641..8ac466580 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -1,7 +1,6 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; -import "./helpers/ACLHelper.sol"; import "../acl/ACLSyntaxSugar.sol"; import "../acl/ACL.sol"; diff --git a/contracts/test/TestACLInterpreter.sol b/contracts/test/TestACLInterpreter.sol index 4e9d90cae..c9056b24c 100644 --- a/contracts/test/TestACLInterpreter.sol +++ b/contracts/test/TestACLInterpreter.sol @@ -2,7 +2,7 @@ pragma solidity 0.4.24; import "../acl/ACL.sol"; import "./helpers/Assert.sol"; -import "./helpers/ACLHelper.sol"; +import "./helpers/ACLOracleHelper.sol"; contract TestACLInterpreter is ACL { diff --git a/contracts/test/helpers/ACLHelper.sol b/contracts/test/helpers/ACLOracleHelper.sol similarity index 100% rename from contracts/test/helpers/ACLHelper.sol rename to contracts/test/helpers/ACLOracleHelper.sol From d2268c8f7cd8d2ed2d34c4171a0d687f7113d004 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 22 Feb 2019 12:34:57 +0100 Subject: [PATCH 42/49] VaultRecoverable: emit event on successful recovery (#480) --- contracts/common/IVaultRecoverable.sol | 2 ++ contracts/common/VaultRecoverable.sol | 10 ++++-- test/recovery_to_vault.js | 45 ++++++++++++++++---------- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/contracts/common/IVaultRecoverable.sol b/contracts/common/IVaultRecoverable.sol index f747b9a0f..3c8c3676c 100644 --- a/contracts/common/IVaultRecoverable.sol +++ b/contracts/common/IVaultRecoverable.sol @@ -6,6 +6,8 @@ pragma solidity ^0.4.24; interface IVaultRecoverable { + event RecoverToVault(address indexed vault, address indexed token, uint256 amount); + function transferToVault(address token) external; function allowRecoverability(address token) external view returns (bool); diff --git a/contracts/common/VaultRecoverable.sol b/contracts/common/VaultRecoverable.sol index 810ac0cb3..74aacd41d 100644 --- a/contracts/common/VaultRecoverable.sol +++ b/contracts/common/VaultRecoverable.sol @@ -28,13 +28,17 @@ contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { address vault = getRecoveryVault(); require(isContract(vault), ERROR_VAULT_NOT_CONTRACT); + uint256 balance; if (_token == ETH) { - vault.transfer(address(this).balance); + balance = address(this).balance; + vault.transfer(balance); } else { ERC20 token = ERC20(_token); - uint256 amount = token.staticBalanceOf(this); - require(token.safeTransfer(vault, amount), ERROR_TOKEN_TRANSFER_FAILED); + balance = token.staticBalanceOf(this); + require(token.safeTransfer(vault, balance), ERROR_TOKEN_TRANSFER_FAILED); } + + emit RecoverToVault(vault, _token, balance); } /** diff --git a/test/recovery_to_vault.js b/test/recovery_to_vault.js index 678c90fd0..045e66d75 100644 --- a/test/recovery_to_vault.js +++ b/test/recovery_to_vault.js @@ -1,3 +1,4 @@ +const assertEvent = require('./helpers/assertEvent') const { assertRevert } = require('./helpers/assertThrow') const { skipCoverage } = require('./helpers/coverage') const { getBalance } = require('./helpers/web3') @@ -36,18 +37,23 @@ contract('Recovery to vault', accounts => { const initialBalance = await getBalance(target.address) const initialVaultBalance = await getBalance(vault.address) const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) - assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') const recoverAction = () => target.transferToVault(ETH) if (shouldFail) { await assertRevert(recoverAction) - assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) - assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target balance should be same as before') + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance, 'Vault balance should should be same as before') } else { - await recoverAction() - assert.equal((await getBalance(target.address)).valueOf(), 0) - assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount)) + const recoverReceipt = await recoverAction() + assert.equal((await getBalance(target.address)).valueOf(), 0, 'Target balance should be 0') + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount), 'Vault balance should include recovered amount') + + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'vault'), vault.address, 'RecoverToVault event should have correct vault') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'token'), ETH, 'RecoverToVault event should have correct token') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'amount'), amount, 'RecoverToVault event should have correct amount') + assertEvent(recoverReceipt, 'RecoverToVault', 1) } } @@ -57,18 +63,23 @@ contract('Recovery to vault', accounts => { const initialBalance = await token.balanceOf(target.address) const initialVaultBalance = await token.balanceOf(vault.address) await token.transfer(target.address, amount) - assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') const recoverAction = () => target.transferToVault(token.address) if (shouldFail) { await assertRevert(recoverAction) - assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) - assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target balance should be same as before') + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance, 'Vault balance should should be same as before') } else { - await recoverAction() - assert.equal((await token.balanceOf(target.address)).valueOf(), 0) - assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount)) + const recoverReceipt = await recoverAction() + assert.equal((await token.balanceOf(target.address)).valueOf(), 0, 'Target balance should be 0') + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount), 'Vault balance should include recovered amount') + + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'vault'), vault.address, 'RecoverToVault event should have correct vault') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'token'), token.address, 'RecoverToVault event should have correct token') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'amount'), amount, 'RecoverToVault event should have correct amount') + assertEvent(recoverReceipt, 'RecoverToVault', 1) } } @@ -78,7 +89,7 @@ contract('Recovery to vault', accounts => { const initialBalance = await token.balanceOf(target.address) const initialVaultBalance = await token.balanceOf(vault.address) await token.transfer(target.address, amount) - assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') // Stop token from being transferable await token.setAllowTransfer(false) @@ -86,8 +97,8 @@ contract('Recovery to vault', accounts => { // Try to transfer await assertRevert(() => target.transferToVault(token.address)) - assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) - assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target balance should be same as before') + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance, 'Vault balance should should be same as before') } const failWithoutVault = async (target, kernel) => { @@ -96,7 +107,7 @@ contract('Recovery to vault', accounts => { const initialBalance = await getBalance(target.address) await kernel.setRecoveryVaultAppId(vaultId) const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) - assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') return assertRevert(async () => { await target.transferToVault(ETH) }) @@ -134,7 +145,7 @@ contract('Recovery to vault', accounts => { // Test both the Kernel itself and the KernelProxy to make sure their behaviours are the same for (const kernelType of ['Kernel', 'KernelProxy']) { - context(`> ${kernelType}`, () => { + context.only(`> ${kernelType}`, () => { let kernelBase, kernel before(async () => { From 722e25e5d301c25a877dc9275b65b9f2594cbcfb Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 22 Feb 2019 17:30:48 +0100 Subject: [PATCH 43/49] test: remove accidentally placed .only() in tests (#483) --- test/recovery_to_vault.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/recovery_to_vault.js b/test/recovery_to_vault.js index 045e66d75..a92714540 100644 --- a/test/recovery_to_vault.js +++ b/test/recovery_to_vault.js @@ -145,7 +145,7 @@ contract('Recovery to vault', accounts => { // Test both the Kernel itself and the KernelProxy to make sure their behaviours are the same for (const kernelType of ['Kernel', 'KernelProxy']) { - context.only(`> ${kernelType}`, () => { + context(`> ${kernelType}`, () => { let kernelBase, kernel before(async () => { From dbb0e06b7b370cddd080fb181db6d6a879e39e1f Mon Sep 17 00:00:00 2001 From: usetech-llc Date: Thu, 7 Mar 2019 19:57:42 +0300 Subject: [PATCH 44/49] feat: Add radspec strings to ENSSubdomainRegistrar --- contracts/ens/ENSSubdomainRegistrar.sol | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/contracts/ens/ENSSubdomainRegistrar.sol b/contracts/ens/ENSSubdomainRegistrar.sol index 671985e19..9fa88113b 100644 --- a/contracts/ens/ENSSubdomainRegistrar.sol +++ b/contracts/ens/ENSSubdomainRegistrar.sol @@ -29,6 +29,12 @@ contract ENSSubdomainRegistrar is AragonApp, ENSConstants { event NewName(bytes32 indexed node, bytes32 indexed label); event DeleteName(bytes32 indexed node, bytes32 indexed label); + /** + * @dev Initialize can only be called once. It saves the block number in which it was initialized. This contract must be the owner of the `_rootNode` node so that it can create subdomains. + * @notice Initialize this ENSSubdomainRegistrar instance with `_ens` as the root ENS registry and `_rootNode` as the node to allocate subdomains under + * @param _ens Address of ENS registry + * @param _rootNode Node to allocate subdomains under + */ function initialize(AbstractENS _ens, bytes32 _rootNode) public onlyInit { initialized(); @@ -39,15 +45,31 @@ contract ENSSubdomainRegistrar is AragonApp, ENSConstants { rootNode = _rootNode; } + /** + * @notice Create a new ENS subdomain record for `_label` and assign ownership to `_owner` + * @param _label Label of new subdomain + * @param _owner Owner of new subdomain + * @return node Hash of created node + */ function createName(bytes32 _label, address _owner) external auth(CREATE_NAME_ROLE) returns (bytes32 node) { return _createName(_label, _owner); } + /** + * @notice Create a new ENS subdomain record for `_label` that resolves to `_target` and is owned by this ENSSubdomainRegistrar + * @param _label Label of new subdomain + * @param _target Ethereum address this new subdomain label will point to + * @return node Hash of created node + */ function createNameAndPoint(bytes32 _label, address _target) external auth(CREATE_NAME_ROLE) returns (bytes32 node) { node = _createName(_label, this); _pointToResolverAndResolve(node, _target); } + /** + * @notice Deregister ENS subdomain record for `_label` + * @param _label Label of subdomain to deregister + */ function deleteName(bytes32 _label) external auth(DELETE_NAME_ROLE) { bytes32 node = getNodeForLabel(_label); @@ -65,6 +87,10 @@ contract ENSSubdomainRegistrar is AragonApp, ENSConstants { emit DeleteName(node, _label); } + /** + * @notice Resolve this ENSSubdomainRegistrar's root node to `_target` + * @param _target Ethereum address root node will point to + */ function pointRootNode(address _target) external auth(POINT_ROOTNODE_ROLE) { _pointToResolverAndResolve(rootNode, _target); } From db9342fb2e81e86f637cade72ef96cb83a639faa Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 7 Mar 2019 17:59:33 +0100 Subject: [PATCH 45/49] fix: update radspec strings (#489) --- contracts/apm/APMRegistry.sol | 2 ++ contracts/apm/Repo.sol | 2 +- contracts/kernel/Kernel.sol | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/apm/APMRegistry.sol b/contracts/apm/APMRegistry.sol index 031014145..b457f4b70 100644 --- a/contracts/apm/APMRegistry.sol +++ b/contracts/apm/APMRegistry.sol @@ -31,6 +31,8 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMInternalAppNames { /** * NEEDS CREATE_NAME_ROLE and POINT_ROOTNODE_ROLE permissions on registrar + * @dev Initialize can only be called once. It saves the block number in which it was initialized + * @notice Initialize this APMRegistry instance and set `_registrar` as the ENS subdomain registrar * @param _registrar ENSSubdomainRegistrar instance that holds registry root node ownership */ function initialize(ENSSubdomainRegistrar _registrar) public onlyInit { diff --git a/contracts/apm/Repo.sol b/contracts/apm/Repo.sol index 095aab21b..30b54d430 100644 --- a/contracts/apm/Repo.sol +++ b/contracts/apm/Repo.sol @@ -30,7 +30,7 @@ contract Repo is AragonApp { /** * @dev Initialize can only be called once. It saves the block number in which it was initialized. - * @notice Initializes a Repo to be usable + * @notice Initialize this Repo */ function initialize() public onlyInit { initialized(); diff --git a/contracts/kernel/Kernel.sol b/contracts/kernel/Kernel.sol index 3698d520b..71c312d21 100644 --- a/contracts/kernel/Kernel.sol +++ b/contracts/kernel/Kernel.sol @@ -35,7 +35,7 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant /** * @dev Initialize can only be called once. It saves the block number in which it was initialized. - * @notice Initializes a kernel instance along with its ACL and sets `_permissionsCreator` as the entity that can create other permissions + * @notice Initialize this kernel instance along with its ACL and set `_permissionsCreator` as the entity that can create other permissions * @param _baseAcl Address of base ACL app * @param _permissionsCreator Entity that will be given permission over createPermission */ From b18c1a5db7c233df676852dca3be3726ab3dccf4 Mon Sep 17 00:00:00 2001 From: Ian Britto Date: Thu, 7 Mar 2019 14:30:36 -0300 Subject: [PATCH 46/49] Create CONTRIBUTING.md --- CONTRIBUTING.md | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..51b282139 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,72 @@ +# Contributing to aragonOS + +:tada: Thank you for being interested in contributing to aragonOS! :tada: + +Feel welcome and read the following sections in order to know how to ask questions and how to work on something. + +There are many ways to contribute, from writing tutorials or blog posts, improving the documentation, submitting bug reports and feature requests or writing code which can be incorporated into the project. + +All members of our community are expected to follow our [Code of Conduct](https://wiki.aragon.org/documentation/Code_of_Conduct/). Please make sure you are welcoming and friendly in all of our spaces. + +## Your first contribution + +Unsure where to begin contributing to aragonOS? + +You can start with a [Good First Issue](https://github.com/aragon/aragonOS/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22) + +> Good first issues are usually for small features, additional tests, spelling / grammar fixes, formatting changes, or other clean up. + +Start small, pick a subject you care about, are familiar with, or want to learn. + +If you're not already familiar with git or Github, here are a couple of friendly tutorials: [First Contributions](https://github.com/firstcontributions/first-contributions), [Open Source Guide](https://opensource.guide/), and [How to Contribute to an Open Source Project on GitHub](https://egghead.io/series/how-to-contribute-to-an-open-source-project-on-github). + +## How to file an issue or report a bug + +If you see a problem, you can report it in our [issue tracker](https://github.com/aragon/aragonOS/issues). + +Please take a quick look to see if the issue doesn't already exist before filing yours. + +Do your best to include as many details as needed in order for someone else to fix the problem and resolve the issue. + +#### If you find a security vulnerability, do NOT open an issue. Email security@aragon.org instead. + +In order to determine whether you are dealing with a security issue, ask yourself these two questions: + +- Can I access or steal something that's not mine, or access something I shouldn't have access to? +- Can I disable something for other people? + +If the answer to either of those two questions are "yes", then you're probably dealing with a security issue. Note that even if you answer "no" to both questions, you may still be dealing with a security issue, so if you're unsure, please send a email. + +#### A [bug bounty program](https://wiki.aragon.org/dev/bug_bounty/) is available for rewarding contributors who find security vulnerabilities with payouts up to $50,000. + +## Fixing issues + +1. [Find an issue](https://github.com/aragon/aragonOS/issues) that you are interested in. + - You may want to ask on the issue or on Aragon Chat's [#dev channel](https://aragon.chat/channel/dev) if anyone has already started working on the issue. +1. Fork and clone a local copy of the repository. +1. Make the appropriate changes for the issue you are trying to address or the feature that you want to add. + - Make sure to add tests! +1. Push the changes to the remote repository. +1. Submit a pull request in Github, explaining any changes and further questions you may have. +1. Wait for the pull request to be reviewed. +1. Make changes to the pull request if the maintainer recommends them. +1. Celebrate your success after your pull request is merged! + +It's OK if your pull request is not perfect (no pull request is). +The reviewer will be able to help you fix any problems and improve it! + +You can also edit a page directly through your browser by clicking the "EDIT" link in the top-right corner of any page and then clicking the pencil icon in the github copy of the page. + +## Styleguide and development processes + +We generally follow [Solidity's style guide](https://solidity.readthedocs.io/en/v0.4.24/style-guide.html) and have set up [Ethlint](https://github.com/duaraghav8/Ethlint) to automatically lint the project. + +Due to the sensitive nature of Solidity, usually at least two reviewers are required before merging any pull request with code changes. + +### Licensing + +aragonOS is generally meant to be used as a library by developers but includes core components that are not generally useful to extend. Any interfaces or contracts meant to be used by other developers are licensed as MIT and have their Solidity pragmas left unpinned. All other contracts are licensed as GPL-3 and are pinned to a specific Solidity version. + +## Community + +If you need help, please reach out to Aragon core contributors and community members in the Aragon Chat [#dev](https://aragon.chat/channel/dev) [#dev-help](https://aragon.chat/channel/dev-help) channels. We'd love to hear from you and know what you're working on! From 37f8eff828bc64e92597caa614edb086e4aedd3f Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 7 Mar 2019 18:35:09 +0100 Subject: [PATCH 47/49] chore: update readme.md --- readme.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/readme.md b/readme.md index 797883ad1..6ea126912 100644 --- a/readme.md +++ b/readme.md @@ -1,6 +1,6 @@ # aragonOS [![Travis branch](https://img.shields.io/travis/aragon/aragonOS/master.svg?style=for-the-badge)](https://travis-ci.org/aragon/aragonOS) [![Coveralls branch](https://img.shields.io/coveralls/aragon/aragonOS/master.svg?style=for-the-badge)](https://coveralls.io/github/aragon/aragonOS?branch=master) [![npm](https://img.shields.io/npm/v/@aragon/os.svg?style=for-the-badge)](https://www.npmjs.com/package/@aragon/os) -This repo contains Aragon's reference implementation for [aragonOS](https://hack.aragon.org/docs/aragonos-intro.html). +This repo contains Aragon's reference implementation of [aragonOS](https://hack.aragon.org/docs/aragonos-intro.html). #### 🚨 Security review status: bug bounty aragonOS 4 has undergone two independent professional security reviews, and the issues raised have been resolved. However there is a [bug bounty program](https://wiki.aragon.org/dev/bug_bounty/) for rewarding hackers who find security vulnerabilities. There is a bounty pool of $250,000 USD, you can find more information [here](https://wiki.aragon.org/dev/bug_bounty/). @@ -10,9 +10,9 @@ Don't be shy to contribute even the smallest tweak. Everyone will be especially ## Documentation -Visit the [Aragon Developer Portal](https://hack.aragon.org/docs/aragonos-intro.html) for in depth documentation on the [architecture](https://hack.aragon.org/docs/aragonos-ref.html) and different parts of the system. +Visit the [Aragon Developer Portal](https://hack.aragon.org/docs/aragonos-intro.html) for in-depth documentation on the [architecture](https://hack.aragon.org/docs/aragonos-ref.html) and different parts of the system. -## Installing aragonOS +## Developing aragonOS locally ```sh npm install @@ -32,14 +32,14 @@ OWNER=[APM owner address] ENS=[ENS registry address] npx truffle exec --network - `ENS`: If no ENS registry address is provided, it will deploy a dummy ENS instance to the network. If the ENS registry is provided, the name `aragonpm.eth` must be owned by the deployer account. - `OWNER`: The account that will be the initial owner of the APM registry -## Using aragonOS for making Aragon apps +## Adding aragonOS as a dependency to your Aragon app ``` npm i --save-dev @aragon/os ``` -Check the [Aragon Developer Portal](https://hack.aragon.org) for detailed documentation and tutorials on how to use aragonOS. +Check the [Aragon Developer Portal](https://hack.aragon.org) for detailed documentation and tutorials on how to use aragonOS to build an Aragon app. ## Contributing -For details about how to contribute you can check the [contributing guide](https://wiki.aragon.one/dev/aragonOS_how_to_contribute/) on the wiki. +For more details about contributing to aragonOS, please check the [contributing guide](./CONTRIBUTING.md). From 032f5778f6f5e11abfcfd27dfe3873944851c2f1 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 7 Mar 2019 19:27:39 +0100 Subject: [PATCH 48/49] fix: move ACLHelpers to its own file --- contracts/acl/ACL.sol | 5 ++-- contracts/acl/ACLHelpers.sol | 46 +++++++++++++++++++++++++++++++ contracts/acl/ACLSyntaxSugar.sol | 41 --------------------------- contracts/test/TestACLHelpers.sol | 18 +++++------- 4 files changed, 56 insertions(+), 54 deletions(-) create mode 100644 contracts/acl/ACLHelpers.sol diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index cc699c837..2ff2a991b 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -2,14 +2,15 @@ pragma solidity 0.4.24; import "../apps/AragonApp.sol"; import "../common/TimeHelpers.sol"; -import "./ACLSyntaxSugar.sol"; +import "./ACLHelpers.sol"; +import "./ACLParams.sol"; import "./IACL.sol"; import "./IACLOracle.sol"; /* solium-disable function-order */ // Allow public initialize() to be first -contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { +contract ACL is IACL, TimeHelpers, AragonApp, ACLParams, ACLHelpers { /* Hardcoded constants to save gas bytes32 public constant CREATE_PERMISSIONS_ROLE = keccak256("CREATE_PERMISSIONS_ROLE"); */ diff --git a/contracts/acl/ACLHelpers.sol b/contracts/acl/ACLHelpers.sol new file mode 100644 index 000000000..829297e79 --- /dev/null +++ b/contracts/acl/ACLHelpers.sol @@ -0,0 +1,46 @@ +/* + * SPDX-License-Identitifer: MIT + */ + +pragma solidity ^0.4.24; + +import "./ACLParams.sol"; + + +contract ACLHelpers is ACLParams { + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { + return uint8(_x >> (8 * 30)); + } + + function decodeParamId(uint256 _x) internal pure returns (uint8 b) { + return uint8(_x >> (8 * 31)); + } + + function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { + a = uint32(_x); + b = uint32(_x >> (8 * 4)); + c = uint32(_x >> (8 * 8)); + } + + function encodeParams(Param[] params) internal pure returns (uint256[]) { + uint256[] memory encodedParams = new uint256[](params.length); + + for (uint i = 0; i < params.length; i++) { + encodedParams[i] = encodeParam(params[i]); + } + + return encodedParams; + } + + function encodeParam(Param param) internal pure returns (uint256) { + return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; + } + + function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) { + return uint240(param1 + (param2 << 32) + (0 << 64)); + } + + function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) { + return uint240(condition + (successParam << 32) + (failureParam << 64)); + } +} diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 24fba6641..428fd4af4 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -4,8 +4,6 @@ pragma solidity ^0.4.24; -import "./ACLParams.sol"; - contract ACLSyntaxSugar { function arr() internal pure returns (uint256[]) { @@ -87,42 +85,3 @@ contract ACLSyntaxSugar { r[4] = _e; } } - - -contract ACLHelpers is ACLParams { - function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { - return uint8(_x >> (8 * 30)); - } - - function decodeParamId(uint256 _x) internal pure returns (uint8 b) { - return uint8(_x >> (8 * 31)); - } - - function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { - a = uint32(_x); - b = uint32(_x >> (8 * 4)); - c = uint32(_x >> (8 * 8)); - } - - function encodeParams(Param[] params) internal pure returns (uint256[]) { - uint256[] memory encodedParams = new uint256[](params.length); - - for (uint i = 0; i < params.length; i++) { - encodedParams[i] = encodeParam(params[i]); - } - - return encodedParams; - } - - function encodeParam(Param param) internal pure returns (uint256) { - return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; - } - - function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) { - return uint240(param1 + (param2 << 32) + (0 << 64)); - } - - function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) { - return uint240(condition + (successParam << 32) + (failureParam << 64)); - } -} diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol index 8ac466580..49264d120 100644 --- a/contracts/test/TestACLHelpers.sol +++ b/contracts/test/TestACLHelpers.sol @@ -1,12 +1,10 @@ pragma solidity 0.4.24; import "./helpers/Assert.sol"; -import "../acl/ACLSyntaxSugar.sol"; -import "../acl/ACL.sol"; +import "../acl/ACLHelpers.sol"; -contract TestACLHelpers is ACL { - +contract TestACLHelpers is ACLHelpers { function testEncodeParam() public { Param memory param = Param(2, uint8(Op.EQ), 5294967297); @@ -23,9 +21,9 @@ contract TestACLHelpers is ACL { Param[] memory params = new Param[](4); params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); - params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); - params[2] = Param(2, uint8(Op.EQ), 1); - params[3] = Param(3, uint8(Op.NEQ), 2); + params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); + params[2] = Param(2, uint8(Op.EQ), 1); + params[3] = Param(3, uint8(Op.NEQ), 2); uint256[] memory encodedParam = encodeParams(params); @@ -35,8 +33,6 @@ contract TestACLHelpers is ACL { Assert.equal(uint256(params[i].id), uint256(id), "Encoded id is not equal"); Assert.equal(uint256(params[i].op), uint256(op), "Encoded op is not equal"); Assert.equal(uint256(params[i].value), uint256(value), "Encoded value is not equal"); - } - - } - + } + } } From 7366642805d08f8431ddfecfb465dc1867644669 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 7 Mar 2019 23:18:02 +0100 Subject: [PATCH 49/49] feat: move ACL param IDs to ACLParams --- contracts/acl/ACL.sol | 8 -------- contracts/acl/ACLParams.sol | 13 +++++++++++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 2ff2a991b..109cbe748 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -16,14 +16,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLParams, ACLHelpers { */ bytes32 public constant CREATE_PERMISSIONS_ROLE = 0x0b719b33c83b8e5d300c521cb8b54ae9bd933996a14bef8c2f4e0285d2d2400a; - uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200; - uint8 internal constant TIMESTAMP_PARAM_ID = 201; - // 202 is unused - uint8 internal constant ORACLE_PARAM_ID = 203; - uint8 internal constant LOGIC_OP_PARAM_ID = 204; - uint8 internal constant PARAM_VALUE_PARAM_ID = 205; - // TODO: Add execution times param type? - /* Hardcoded constant to save gas bytes32 public constant EMPTY_PARAM_HASH = keccak256(uint256(0)); */ diff --git a/contracts/acl/ACLParams.sol b/contracts/acl/ACLParams.sol index e26c96e71..479baeaa6 100644 --- a/contracts/acl/ACLParams.sol +++ b/contracts/acl/ACLParams.sol @@ -6,8 +6,17 @@ pragma solidity ^0.4.24; contract ACLParams { + // Op types + enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } - enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types + // Op IDs + uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200; + uint8 internal constant TIMESTAMP_PARAM_ID = 201; + // 202 is unused + uint8 internal constant ORACLE_PARAM_ID = 203; + uint8 internal constant LOGIC_OP_PARAM_ID = 204; + uint8 internal constant PARAM_VALUE_PARAM_ID = 205; + // TODO: Add execution times param type? struct Param { uint8 id; @@ -16,4 +25,4 @@ contract ACLParams { // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal // op and id take less than 1 byte each so it can be kept in 1 sstore } -} \ No newline at end of file +}