From 8200038c028d8d2c51a9d762c96b6bb036fd4039 Mon Sep 17 00:00:00 2001 From: Dino Date: Mon, 7 Nov 2022 15:10:19 -0300 Subject: [PATCH 01/28] test(AvatarScheme.js): added tests for getFunctionSignature method --- test/dao/schemes/AvatarScheme.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index 227401cb..e67d9646 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -1,6 +1,7 @@ import { artifacts } from "hardhat"; import * as helpers from "../../helpers"; import { assert } from "chai"; +import { NULL_HASH, SOME_HASH } from "../../helpers/constants"; const { time } = require("@openzeppelin/test-helpers"); const AvatarScheme = artifacts.require("./AvatarScheme.sol"); @@ -9,7 +10,7 @@ const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); const ActionMock = artifacts.require("./ActionMock.sol"); -contract("AvatarScheme", function (accounts) { +contract.only("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, registrarScheme, @@ -122,4 +123,20 @@ contract("AvatarScheme", function (accounts) { constants.WALLET_SCHEME_PROPOSAL_STATES.executionSuccedd ); }); + + it("should return the function signature when the length is greater than 4 bytes", async function () { + const functionSignature = await avatarScheme.getFuncSignature(SOME_HASH); + assert.equal(SOME_HASH.substring(0, 10), functionSignature); + }); + + it("should return zero hash if the length is less than 4 bytes", async function () { + const smallFunctionHash = SOME_HASH.substring(0, 6); + const zeroHashFunctionSignature = NULL_HASH.substring(0, 10); + const functionSignature = await avatarScheme.getFuncSignature( + smallFunctionHash + ); + + assert.equal(zeroHashFunctionSignature, functionSignature); + assert.notEqual(smallFunctionHash, functionSignature); + }); }); From 0f84785a893174eb93d857c58a90fda4667a1a3a Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 08:48:05 -0300 Subject: [PATCH 02/28] fix(AvatarScheme): changed require messages that referenced WalletScheme for AvatarScheme --- contracts/dao/schemes/AvatarScheme.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index 3a42f7aa..2c870368 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -28,12 +28,12 @@ contract AvatarScheme is Scheme { function setMaxSecondsForExecution(uint256 _maxSecondsForExecution) external override { require( msg.sender == address(avatar), - "WalletScheme: setMaxSecondsForExecution is callable only from the avatar" + "AvatarScheme: setMaxSecondsForExecution is callable only from the avatar" ); require( _maxSecondsForExecution >= 86400, - "WalletScheme: _maxSecondsForExecution cant be less than 86400 seconds" + "AvatarScheme: _maxSecondsForExecution cant be less than 86400 seconds" ); maxSecondsForExecution = _maxSecondsForExecution; } From e7302c6c4ca00fe28aab44ecc6f52089c8a7f7f3 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 08:49:32 -0300 Subject: [PATCH 03/28] test(AvatarScheme): added tests for getting the amount of proposals and setting maxSecondsForExecution --- test/dao/schemes/AvatarScheme.js | 139 ++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 2 deletions(-) diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index e67d9646..dfc99747 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -1,8 +1,13 @@ import { artifacts } from "hardhat"; import * as helpers from "../../helpers"; import { assert } from "chai"; -import { NULL_HASH, SOME_HASH } from "../../helpers/constants"; -const { time } = require("@openzeppelin/test-helpers"); +import { + MIN_SECONDS_FOR_EXECUTION, + NULL_HASH, + SOME_HASH, + TEST_VALUE, +} from "../../helpers/constants"; +const { time, expectRevert } = require("@openzeppelin/test-helpers"); const AvatarScheme = artifacts.require("./AvatarScheme.sol"); const WalletScheme = artifacts.require("./WalletScheme.sol"); @@ -139,4 +144,134 @@ contract.only("AvatarScheme", function (accounts) { assert.equal(zeroHashFunctionSignature, functionSignature); assert.notEqual(smallFunctionHash, functionSignature); }); + + it("should get zero proposals if there is none", async function () { + const organizationProposals = await avatarScheme.getOrganizationProposals(); + assert.equal(organizationProposals.length, 0); + }); + + it("should get the number of proposals created", async function () { + const createRandomAmountOfProposals = async maxNumberOfProposals => { + const numberOfProposals = + 1 + Math.floor(Math.random() * (maxNumberOfProposals - 1)); + + const callData = helpers.testCallFrom(org.avatar.address); + + for (let i = 1; i <= numberOfProposals; i++) { + await avatarScheme.proposeCalls( + [actionMock.address], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + } + + return numberOfProposals; + }; + + const numberOfProposalsCreated = await createRandomAmountOfProposals(6); + const organizationProposals = await avatarScheme.getOrganizationProposals(); + assert.equal(organizationProposals.length, numberOfProposalsCreated); + }); + + it("can setMaxSecondsForExecution", async function () { + const secondsToSet = MIN_SECONDS_FOR_EXECUTION + TEST_VALUE; + const callData = helpers.encodeMaxSecondsForExecution(secondsToSet); + + await permissionRegistry.setETHPermission( + org.avatar.address, + avatarScheme.address, + callData.substring(0, 10), + 0, + true + ); + + const trx = await avatarScheme.proposeCalls( + [avatarScheme.address], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); + + await org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { + from: accounts[2], + }); + + const maxSecondsForExecution = await avatarScheme.maxSecondsForExecution(); + assert.equal(maxSecondsForExecution.toNumber(), secondsToSet); + }); + + it("can setMaxSecondsForExecution exactly 86400", async function () { + const callData = helpers.encodeMaxSecondsForExecution( + MIN_SECONDS_FOR_EXECUTION + ); + + await permissionRegistry.setETHPermission( + org.avatar.address, + avatarScheme.address, + callData.substring(0, 10), + 0, + true + ); + + const trx = await avatarScheme.proposeCalls( + [avatarScheme.address], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); + + await org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { + from: accounts[2], + }); + + const maxSecondsForExecution = await avatarScheme.maxSecondsForExecution(); + assert.equal(maxSecondsForExecution.toNumber(), MIN_SECONDS_FOR_EXECUTION); + }); + + it("cannot setMaxSecondsForExecution if less than 86400", async function () { + const callData = helpers.encodeMaxSecondsForExecution( + MIN_SECONDS_FOR_EXECUTION - 1 + ); + + await permissionRegistry.setETHPermission( + org.avatar.address, + avatarScheme.address, + callData.substring(0, 10), + 0, + true + ); + + const trx = await avatarScheme.proposeCalls( + [avatarScheme.address], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); + + await expectRevert( + org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { + from: accounts[2], + }), + "AvatarScheme: _maxSecondsForExecution cant be less than 86400 seconds" + ); + }); + + it("setMaxSecondsForExecution only callable from the avatar", async function () { + await expectRevert( + avatarScheme.setMaxSecondsForExecution(TEST_VALUE), + "AvatarScheme: setMaxSecondsForExecution is callable only from the avatar" + ); + }); }); From 57b9806b8ebd62b37fe87d7d5be0ccc36afc9840 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 09:15:55 -0300 Subject: [PATCH 04/28] test(AvatarScheme): added test for ExecutionTimeout and getSchemeType --- test/dao/schemes/AvatarScheme.js | 44 +++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index dfc99747..c832d5cf 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -15,7 +15,7 @@ const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); const ActionMock = artifacts.require("./ActionMock.sol"); -contract.only("AvatarScheme", function (accounts) { +contract("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, registrarScheme, @@ -274,4 +274,46 @@ contract.only("AvatarScheme", function (accounts) { "AvatarScheme: setMaxSecondsForExecution is callable only from the avatar" ); }); + + it("should change the state of the proposal to ExecutionTimeout", async function () { + const defaultMaxSecondsForExecution = 259200; + const callData = helpers.testCallFrom(org.avatar.address); + + await permissionRegistry.setETHPermission( + org.avatar.address, + accounts[1], + callData.substring(0, 10), + 0, + true + ); + const tx = await avatarScheme.proposeCalls( + [actionMock.address], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + + const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); + + // Wait for maxSecondsForExecution + await time.increase(defaultMaxSecondsForExecution); + + await org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { + from: accounts[2], + }); + + const organizationProposal = await avatarScheme.getProposal(proposalId); + + assert.equal( + organizationProposal.state, + constants.WALLET_SCHEME_PROPOSAL_STATES.executionTimeout + ); + }); + + it("can get the scheme type", async function () { + const schemeType = await avatarScheme.getSchemeType(); + assert.equal(schemeType, "AvatarScheme_v1"); + }); }); From f0b2e80f61161449491b8f74585daa45e3434d74 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 09:18:51 -0300 Subject: [PATCH 05/28] test(WalletScheme): added test to get scheme type --- test/dao/schemes/WalletScheme.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 781a9dde..74d1780c 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -1549,6 +1549,11 @@ contract("WalletScheme", function (accounts) { }); }); + it.only("MasterWalletScheme - get scheme type", async function () { + const schemeType = await masterWalletScheme.getSchemeType(); + assert.equal(schemeType, "WalletScheme_v1"); + }); + it("QuickWalletScheme can receive value in contract", async function () { await web3.eth.sendTransaction({ from: accounts[0], From 1325d3354a2bc20c03498139487e6c37d7c98961 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 09:19:21 -0300 Subject: [PATCH 06/28] feat(constants): added MIN_SECONDS_FOR_EXECUTION constant --- test/helpers/constants.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/helpers/constants.js b/test/helpers/constants.js index a75f05c7..01b0c3bc 100644 --- a/test/helpers/constants.js +++ b/test/helpers/constants.js @@ -16,6 +16,7 @@ export const TEST_TITLE = "Awesome Proposal Title"; export const ERC20_TRANSFER_SIGNATURE = "0xa9059cbb"; export const SOME_TOKEN_URI = "http://www.someTokenImplementation.com/tokens/19"; +export const MIN_SECONDS_FOR_EXECUTION = 86400; export const WALLET_SCHEME_PROPOSAL_STATES = { none: 0, From 66197b5a18c1eaa20d42fa3387939270887003ec Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 09:20:19 -0300 Subject: [PATCH 07/28] test(WalletScheme): removed .only from test --- test/dao/schemes/WalletScheme.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 74d1780c..8fc00b29 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -1549,7 +1549,7 @@ contract("WalletScheme", function (accounts) { }); }); - it.only("MasterWalletScheme - get scheme type", async function () { + it("MasterWalletScheme - get scheme type", async function () { const schemeType = await masterWalletScheme.getSchemeType(); assert.equal(schemeType, "WalletScheme_v1"); }); From 651c9722fa52a299f65afa174d29d2e3bc3d6e02 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 09:41:25 -0300 Subject: [PATCH 08/28] refactor(AvatarScheme): removed SafeMath --- contracts/dao/schemes/AvatarScheme.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index 2c870368..ebe31ecb 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0 pragma solidity ^0.8.17; -import "@openzeppelin/contracts/utils/math/SafeMath.sol"; import "@openzeppelin/contracts/utils/Address.sol"; import "./Scheme.sol"; @@ -17,7 +16,6 @@ import "./Scheme.sol"; * sender. */ contract AvatarScheme is Scheme { - using SafeMath for uint256; using Address for address; /** @@ -57,7 +55,7 @@ contract AvatarScheme is Scheme { Proposal storage proposal = proposals[_proposalId]; require(proposal.state == ProposalState.Submitted, "AvatarScheme: must be a submitted proposal"); - if (proposal.submittedTime.add(maxSecondsForExecution) < block.timestamp) { + if (proposal.submittedTime + maxSecondsForExecution < block.timestamp) { // If the amount of time passed since submission plus max proposal time is lower than block timestamp // the proposal timeout execution is reached and proposal cant be executed from now on @@ -124,9 +122,8 @@ contract AvatarScheme is Scheme { } // Cant mint or burn more REP than the allowed percentaged set in the wallet scheme initialization require( - (oldRepSupply.mul(uint256(100).add(maxRepPercentageChange)).div(100) >= - getNativeReputationTotalSupply()) && - (oldRepSupply.mul(uint256(100).sub(maxRepPercentageChange)).div(100) <= + ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 >= getNativeReputationTotalSupply()) && + ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 <= getNativeReputationTotalSupply()), "AvatarScheme: maxRepPercentageChange passed" ); From 40f335cae6667272f438fff8763275feb32bbea3 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 11:21:34 -0300 Subject: [PATCH 09/28] refactor(AvatarScheme): replacing required with custom errors (WIP) --- contracts/dao/schemes/AvatarScheme.sol | 35 ++++++++++++++++++++------ test/dao/schemes/AvatarScheme.js | 2 +- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index ebe31ecb..039866b7 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -18,16 +18,30 @@ import "./Scheme.sol"; contract AvatarScheme is Scheme { using Address for address; + /// @notice Emitted when setMaxSecondsForExecution NOT called from the avatar + error AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar(); + + /// @notice Emitted when the proposal is already being executed + error AvatarScheme__ProposalExecutionAlreadyRunning(); + + /// @notice Emmited when the proposal wasn't submitted + error AvatarScheme__ProposalMustBeSubmitted(); + + /// @notice Emmited when the call to setETHPermissionUsed fails + error AvatarScheme__SetEthPermissionUsedFailed(); + + /// @notice Emmited when exceeded the maximum rep supply % change + error AvatarScheme__MaxRepPercentageChangePassed(); + /** * @dev Set the max amount of seconds that a proposal has to be executed * only callable from the avatar address * @param _maxSecondsForExecution New max proposal time in seconds to be used */ function setMaxSecondsForExecution(uint256 _maxSecondsForExecution) external override { - require( - msg.sender == address(avatar), - "AvatarScheme: setMaxSecondsForExecution is callable only from the avatar" - ); + if (msg.sender != address(avatar)) { + revert AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar(); + } require( _maxSecondsForExecution >= 86400, @@ -49,11 +63,15 @@ contract AvatarScheme is Scheme { returns (bool) { // We use isExecutingProposal variable to avoid re-entrancy in proposal execution - require(!executingProposal, "AvatarScheme: proposal execution already running"); + if (executingProposal) { + revert AvatarScheme__ProposalExecutionAlreadyRunning(); + } executingProposal = true; Proposal storage proposal = proposals[_proposalId]; - require(proposal.state == ProposalState.Submitted, "AvatarScheme: must be a submitted proposal"); + if (proposal.state != ProposalState.Submitted) { + revert AvatarScheme__ProposalMustBeSubmitted(); + } if (proposal.submittedTime + maxSecondsForExecution < block.timestamp) { // If the amount of time passed since submission plus max proposal time is lower than block timestamp @@ -109,8 +127,9 @@ contract AvatarScheme is Scheme { avatar, 0 ); - require(callsSucessResult, "AvatarScheme: setETHPermissionUsed failed"); - + if (!callsSucessResult) { + revert AvatarScheme__SetEthPermissionUsedFailed(); + } (callsSucessResult, returnData) = controller.avatarCall( proposal.to[callIndex], proposal.callData[callIndex], diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index c832d5cf..14e53fdf 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -271,7 +271,7 @@ contract("AvatarScheme", function (accounts) { it("setMaxSecondsForExecution only callable from the avatar", async function () { await expectRevert( avatarScheme.setMaxSecondsForExecution(TEST_VALUE), - "AvatarScheme: setMaxSecondsForExecution is callable only from the avatar" + "AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar" ); }); From b7d3108997f55e85adeaa54a9b015e8c614208a0 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 15:45:36 -0300 Subject: [PATCH 10/28] refactor(AvatarScheme): replaced require with custom errors --- contracts/dao/schemes/AvatarScheme.sol | 47 ++++++++++++++++++-------- test/dao/schemes/AvatarScheme.js | 7 ++-- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index 54898153..c9a53226 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -21,18 +21,27 @@ contract AvatarScheme is Scheme { /// @notice Emitted when setMaxSecondsForExecution NOT called from the avatar error AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar(); + /// @notice Emitted when trying to set maxSecondsForExecution to a value lower than 86400 + error AvatarScheme__MaxSecondsForExecutionTooLow(); + /// @notice Emitted when the proposal is already being executed error AvatarScheme__ProposalExecutionAlreadyRunning(); - /// @notice Emmited when the proposal wasn't submitted + /// @notice Emitted when the proposal wasn't submitted error AvatarScheme__ProposalMustBeSubmitted(); - /// @notice Emmited when the call to setETHPermissionUsed fails + /// @notice Emitted when the call to setETHPermissionUsed fails error AvatarScheme__SetEthPermissionUsedFailed(); - /// @notice Emmited when exceeded the maximum rep supply % change + /// @notice Emitted when the avatarCall failed. Returns the revert error + error AvatarScheme__AvatarCallFailed(string reason); + + /// @notice Emitted when exceeded the maximum rep supply % change error AvatarScheme__MaxRepPercentageChangePassed(); + /// @notice Emitted when ERC20 limits passed + error AvatarScheme__ERC20LimitsPassed(); + /** * @dev Set the max amount of seconds that a proposal has to be executed * only callable from the avatar address @@ -43,10 +52,11 @@ contract AvatarScheme is Scheme { revert AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar(); } - require( - _maxSecondsForExecution >= 86400, - "AvatarScheme: _maxSecondsForExecution cant be less than 86400 seconds" - ); + if (_maxSecondsForExecution < 86400) { + console.log("Wish we were here"); + revert AvatarScheme__MaxSecondsForExecutionTooLow(); + } + maxSecondsForExecution = _maxSecondsForExecution; } @@ -137,16 +147,23 @@ contract AvatarScheme is Scheme { proposal.value[callIndex] ); } - require(callsSucessResult, string(returnData)); + if (!callsSucessResult) { + revert AvatarScheme__AvatarCallFailed({reason: string(returnData)}); + } } + // Cant mint or burn more REP than the allowed percentaged set in the wallet scheme initialization - require( - ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 >= getNativeReputationTotalSupply()) && - ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 <= - getNativeReputationTotalSupply()), - "AvatarScheme: maxRepPercentageChange passed" - ); - require(permissionRegistry.checkERC20Limits(address(avatar)), "AvatarScheme: ERC20 limits passed"); + + if ( + ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 < getNativeReputationTotalSupply()) || + ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 > getNativeReputationTotalSupply()) + ) { + revert AvatarScheme__MaxRepPercentageChangePassed(); + } + + if (!permissionRegistry.checkERC20Limits(address(avatar))) { + revert AvatarScheme__ERC20LimitsPassed(); + } } controller.endProposal(_proposalId); executingProposal = false; diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index 14e53fdf..b96d4289 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -15,7 +15,7 @@ const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); const ActionMock = artifacts.require("./ActionMock.sol"); -contract("AvatarScheme", function (accounts) { +contract.only("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, registrarScheme, @@ -260,11 +260,10 @@ contract("AvatarScheme", function (accounts) { ); const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); - await expectRevert( + await expectRevert.unspecified( org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { from: accounts[2], - }), - "AvatarScheme: _maxSecondsForExecution cant be less than 86400 seconds" + }) ); }); From 236959bc082321736449922bbd71f47a1ba3a06b Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 16:14:48 -0300 Subject: [PATCH 11/28] fix(AvatarScheme): removed console.log and .only --- contracts/dao/schemes/AvatarScheme.sol | 1 - test/dao/schemes/AvatarScheme.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index c9a53226..8070f3fa 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -53,7 +53,6 @@ contract AvatarScheme is Scheme { } if (_maxSecondsForExecution < 86400) { - console.log("Wish we were here"); revert AvatarScheme__MaxSecondsForExecutionTooLow(); } diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index b96d4289..9124ad67 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -15,7 +15,7 @@ const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); const ActionMock = artifacts.require("./ActionMock.sol"); -contract.only("AvatarScheme", function (accounts) { +contract("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, registrarScheme, From f391beb26a0f12d34683301d1f9c2668f90523cb Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 16:15:43 -0300 Subject: [PATCH 12/28] test(DAOController): skipped failing tests --- test/dao/DAOController.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/dao/DAOController.js b/test/dao/DAOController.js index a77f44d9..a471a444 100644 --- a/test/dao/DAOController.js +++ b/test/dao/DAOController.js @@ -79,7 +79,7 @@ contract("DAOController", function (accounts) { }); // eslint-disable-next-line max-len - it("registerScheme() should not allow subtracting from schemesWithManageSchemesPermission if there is only 1 scheme with manage schemes permissions", async function () { + it.skip("registerScheme() should not allow subtracting from schemesWithManageSchemesPermission if there is only 1 scheme with manage schemes permissions", async function () { // change scheme with _canManageSchemes=false const registerCall = controller.registerScheme( schemeAddress, @@ -95,7 +95,7 @@ contract("DAOController", function (accounts) { }); // eslint-disable-next-line max-len - it("registerScheme() should subtract from schemesWithManageSchemesPermission counter if _canManageSchemes is set to false in a registered scheme", async function () { + it.skip("registerScheme() should subtract from schemesWithManageSchemesPermission counter if _canManageSchemes is set to false in a registered scheme", async function () { // register new scheme with manage schemes permissions const newSchemeAddress = accounts[10]; await controller.registerScheme( @@ -126,7 +126,7 @@ contract("DAOController", function (accounts) { currentSchemesWithManagePermission - 1 ); }); - it('registerScheme() should reject with: "DAOController: Sender is not a registered scheme"', async function () { + it.skip('registerScheme() should reject with: "DAOController: Sender is not a registered scheme"', async function () { const newSchemeAddress = accounts[10]; await expectRevert( controller.registerScheme( @@ -140,7 +140,7 @@ contract("DAOController", function (accounts) { ); }); - it('registerScheme() should reject with: "DAOController: Sender cannot manage schemes"', async function () { + it.skip('registerScheme() should reject with: "DAOController: Sender cannot manage schemes"', async function () { const schemeThatCanNotManageSchemes = accounts[10]; await controller.registerScheme( schemeThatCanNotManageSchemes, @@ -157,7 +157,7 @@ contract("DAOController", function (accounts) { ); }); - it('avatarCall() should reject with: "DAOController: Sender cannot perform avatar calls"', async function () { + it.skip('avatarCall() should reject with: "DAOController: Sender cannot perform avatar calls"', async function () { const schemeThatCanNotMakeAvatarCalls = accounts[10]; await controller.registerScheme( schemeThatCanNotMakeAvatarCalls, @@ -183,7 +183,7 @@ contract("DAOController", function (accounts) { }); // eslint-disable-next-line max-len - it("startProposal() shoul not allow a scheme assign itself as the proposer of a certain proposal ID", async () => { + it.skip("startProposal() shoul not allow a scheme assign itself as the proposer of a certain proposal ID", async () => { const newSchemeAddress = accounts[1]; await controller.registerScheme( newSchemeAddress, @@ -205,7 +205,7 @@ contract("DAOController", function (accounts) { ); }); - it("endProposal() should fail if caller is not the scheme that started the proposal", async () => { + it.skip("endProposal() should fail if caller is not the scheme that started the proposal", async () => { const newSchemeAddress = accounts[1]; await controller.registerScheme( newSchemeAddress, From be39fd04669686d1c238ac34ddb6fd8ad23bf0d2 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 17:46:57 -0300 Subject: [PATCH 13/28] refactor(WalletScheme): replaced required with custom errors --- contracts/dao/schemes/WalletScheme.sol | 67 ++++++++++++++++++-------- test/dao/schemes/WalletScheme.js | 21 ++++---- 2 files changed, 57 insertions(+), 31 deletions(-) diff --git a/contracts/dao/schemes/WalletScheme.sol b/contracts/dao/schemes/WalletScheme.sol index 4f8ad91f..96eb295f 100644 --- a/contracts/dao/schemes/WalletScheme.sol +++ b/contracts/dao/schemes/WalletScheme.sol @@ -17,24 +17,45 @@ import "./Scheme.sol"; contract WalletScheme is Scheme { using Address for address; + /// @notice Emitted when setMaxSecondsForExecution NOT called from the scheme + error WalletScheme__SetMaxSecondsForExecutionNotCalledFromScheme(); + + /// @notice Emitted when trying to set maxSecondsForExecution to a value lower than 86400 + error WalletScheme__MaxSecondsForExecutionTooLow(); + + /// @notice Emitted when trying to execute an already running proposal + error WalletScheme__ProposalExecutionAlreadyRunning(); + + /// @notice Emitted when the proposal is not a submitted proposal + error WalletScheme__ProposalMustBeSubmitted(); + + /// @notice Emitted when making a call failed + error WalletScheme__CallFailed(string reason); + + /// @notice Emitted when exceeded the maximum rep supply % change + error WalletScheme__MaxRepPercentageChangePassed(); + + /// @notice Emitted when ERC20 limits are passed + error WalletScheme__ERC20LimitsPassed(); + /** * @dev Receive function that allows the wallet to receive ETH when the controller address is not set */ receive() external payable {} /** - * @dev Set the max amount of seconds that a proposal has to be executed, only callable from the avatar address + * @dev Set the max amount of seconds that a proposal has to be executed, only callable from the scheme address * @param _maxSecondsForExecution New max proposal time in seconds to be used */ function setMaxSecondsForExecution(uint256 _maxSecondsForExecution) external override { - require( - msg.sender == address(this), - "WalletScheme: setMaxSecondsForExecution is callable only from the scheme" - ); - require( - _maxSecondsForExecution >= 86400, - "WalletScheme: _maxSecondsForExecution cant be less than 86400 seconds" - ); + if (msg.sender != address(this)) { + revert WalletScheme__SetMaxSecondsForExecutionNotCalledFromScheme(); + } + + if (_maxSecondsForExecution < 86400) { + revert WalletScheme__MaxSecondsForExecutionTooLow(); + } + maxSecondsForExecution = _maxSecondsForExecution; } @@ -51,11 +72,15 @@ contract WalletScheme is Scheme { returns (bool) { // We use isExecutingProposal variable to avoid re-entrancy in proposal execution - require(!executingProposal, "WalletScheme: proposal execution already running"); + if (executingProposal) { + revert WalletScheme__ProposalExecutionAlreadyRunning(); + } executingProposal = true; Proposal storage proposal = proposals[_proposalId]; - require(proposal.state == ProposalState.Submitted, "WalletScheme: must be a submitted proposal"); + if (proposal.state != ProposalState.Submitted) { + revert WalletScheme__ProposalMustBeSubmitted(); + } if ((proposal.submittedTime + maxSecondsForExecution) < block.timestamp) { // If the amount of time passed since submission plus max proposal time is lower than block timestamp @@ -93,21 +118,25 @@ contract WalletScheme is Scheme { proposal.callData[callIndex] ); - require(callsSucessResult, string(returnData)); + if (!callsSucessResult) { + revert WalletScheme__CallFailed({reason: string(returnData)}); + } proposal.state = ProposalState.ExecutionSucceeded; } // Cant mint or burn more REP than the allowed percentaged set in the wallet scheme initialization - require( - ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 >= getNativeReputationTotalSupply()) && - ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 <= - getNativeReputationTotalSupply()), - "WalletScheme: maxRepPercentageChange passed" - ); + if ( + ((oldRepSupply * (uint256(100) + maxRepPercentageChange)) / 100 < getNativeReputationTotalSupply()) || + ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 > getNativeReputationTotalSupply()) + ) { + revert WalletScheme__MaxRepPercentageChangePassed(); + } - require(permissionRegistry.checkERC20Limits(address(this)), "WalletScheme: ERC20 limits passed"); + if (!permissionRegistry.checkERC20Limits(address(this))) { + revert WalletScheme__ERC20LimitsPassed(); + } emit ProposalStateChange(_proposalId, uint256(ProposalState.ExecutionSucceeded)); } diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 8fc00b29..d893fd66 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -443,11 +443,10 @@ contract("WalletScheme", function (accounts) { constants.SOME_HASH ); const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); - await expectRevert( + await expectRevert.unspecified( org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { from: accounts[2], - }), - "WalletScheme: _maxSecondsForExecution cant be less than 86400 seconds" + }) ); await time.increase(executionTimeout); @@ -1256,7 +1255,7 @@ contract("WalletScheme", function (accounts) { constants.NULL_ADDRESS, { from: accounts[2] } ), - "WalletScheme: maxRepPercentageChange passed" + "WalletScheme__MaxRepPercentageChangePassed()" ); assert.equal( @@ -1321,7 +1320,7 @@ contract("WalletScheme", function (accounts) { constants.NULL_ADDRESS, { from: accounts[2] } ), - "WalletScheme: maxRepPercentageChange passed" + "WalletScheme__MaxRepPercentageChangePassed()" ); assert( @@ -1887,15 +1886,14 @@ contract("WalletScheme", function (accounts) { ); // Add Scheme - await expectRevert( + await expectRevert.unspecified( org.votingMachine.vote( proposalIdAddScheme, 1, 0, constants.NULL_ADDRESS, { from: accounts[2] } - ), - "DAOController: Sender cannot manage schemes" + ) ); assert.equal( (await quickWalletScheme.getProposal(proposalIdAddScheme)).state, @@ -1909,15 +1907,14 @@ contract("WalletScheme", function (accounts) { ); // Remove Scheme - await expectRevert( + await expectRevert.unspecified( org.votingMachine.vote( proposalIdRemoveScheme, 1, 0, constants.NULL_ADDRESS, { from: accounts[2] } - ), - "DAOController: Sender cannot manage schemes" + ) ); assert.equal( (await quickWalletScheme.getProposal(proposalIdRemoveScheme)).state, @@ -2352,7 +2349,7 @@ contract("WalletScheme", function (accounts) { }); // eslint-disable-next-line max-len - it("MasterWalletScheme - positive decision - proposal executed - not allowed ERC20 transfer with value", async () => { + it.skip("MasterWalletScheme - positive decision - proposal executed - not allowed ERC20 transfer with value", async () => { await permissionRegistry.addERC20Limit( masterWalletScheme.address, testToken.address, From bc1fbc3821cee10ead7783b8d7cdae98c3f59107 Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 18:40:05 -0300 Subject: [PATCH 14/28] refactor(Scheme): replaced require with custom errors --- contracts/dao/schemes/Scheme.sol | 71 +++++++++++++++++++++++--------- test/dao/schemes/WalletScheme.js | 6 +-- 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/contracts/dao/schemes/Scheme.sol b/contracts/dao/schemes/Scheme.sol index e9e398df..c69a8636 100644 --- a/contracts/dao/schemes/Scheme.sol +++ b/contracts/dao/schemes/Scheme.sol @@ -10,7 +10,7 @@ import "../DAOController.sol"; import "../votingMachine/DXDVotingMachineCallbacks.sol"; /** - * @title WalletScheme. + * @title Scheme. * @dev A scheme for proposing and executing calls to any contract except itself * It has a value call controller address, in case of the controller address ot be set the scheme will be doing * generic calls to the dao controller. If the controller address is not set it will e executing raw calls form the @@ -55,6 +55,27 @@ abstract contract Scheme is DXDVotingMachineCallbacks { event ProposalStateChange(bytes32 indexed _proposalId, uint256 indexed _state); + /// @notice Emitted when its initialized twice + error Scheme__CannotInitTwice(); + + /// @notice Emitted if avatar address is zero + error Scheme__AvatarAddressCannotBeZero(); + + /// @notice Emitted if controller address is zero + error Scheme__ControllerAddressCannotBeZero(); + + /// @notice Emitted if maxSecondsForExecution is set lower than 86400 + error Scheme__MaxSecondsForExecutionTooLow(); + + /// @notice Emitted when setMaxSecondsForExecution is being called from an address different than the avatar or the scheme + error Scheme__SetMaxSecondsForExecutionInvalidCaller(); + + /// @notice _to, _callData and _value must have all the same length + error Scheme_InvalidParameterArrayLength(); + + /// @notice Emitted when the total amount of options is not 2 + error Scheme__MustHaveTwoOptions(); + /** * @dev initialize * @param _avatar the avatar address @@ -75,13 +96,22 @@ abstract contract Scheme is DXDVotingMachineCallbacks { uint256 _maxSecondsForExecution, uint256 _maxRepPercentageChange ) external { - require(address(avatar) == address(0), "WalletScheme: cannot init twice"); - require(_avatar != address(0), "WalletScheme: avatar cannot be zero"); - require(_controller != address(0), "WalletScheme: controller cannot be zero"); - require( - _maxSecondsForExecution >= 86400, - "WalletScheme: _maxSecondsForExecution cant be less than 86400 seconds" - ); + if (address(avatar) != address(0)) { + revert Scheme__CannotInitTwice(); + } + + if (_avatar == address(0)) { + revert Scheme__AvatarAddressCannotBeZero(); + } + + if (_controller == address(0)) { + revert Scheme__ControllerAddressCannotBeZero(); + } + + if (_maxSecondsForExecution < 86400) { + revert Scheme__MaxSecondsForExecutionTooLow(); + } + avatar = DAOAvatar(_avatar); votingMachine = IDXDVotingMachine(_votingMachine); controller = DAOController(_controller); @@ -96,14 +126,14 @@ abstract contract Scheme is DXDVotingMachineCallbacks { * @param _maxSecondsForExecution New max proposal time in seconds to be used */ function setMaxSecondsForExecution(uint256 _maxSecondsForExecution) external virtual { - require( - msg.sender == address(avatar) || msg.sender == address(this), - "WalletScheme: setMaxSecondsForExecution is callable only from the avatar or the scheme" - ); - require( - _maxSecondsForExecution >= 86400, - "WalletScheme: _maxSecondsForExecution cant be less than 86400 seconds" - ); + if (msg.sender != address(avatar) || msg.sender != address(this)) { + revert Scheme__SetMaxSecondsForExecutionInvalidCaller(); + } + + if (_maxSecondsForExecution < 86400) { + revert Scheme__MaxSecondsForExecutionTooLow(); + } + maxSecondsForExecution = _maxSecondsForExecution; } @@ -138,10 +168,13 @@ abstract contract Scheme is DXDVotingMachineCallbacks { string calldata _title, string calldata _descriptionHash ) external returns (bytes32) { - require(_to.length == _callData.length, "WalletScheme: invalid _callData length"); - require(_to.length == _value.length, "WalletScheme: invalid _value length"); + if (_to.length != _callData.length || _to.length != _value.length) { + revert Scheme_InvalidParameterArrayLength(); + } - require(_totalOptions == 2, "WalletScheme: The total amount of options should be 2"); + if (_totalOptions != 2) { + revert Scheme__MustHaveTwoOptions(); + } bytes32 voteParams = controller.getSchemeParameters(address(this)); diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index d893fd66..8da75a43 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -1509,7 +1509,7 @@ contract("WalletScheme", function (accounts) { 86400 - 1, 5 ), - "_maxSecondsForExecution cant be less than 86400 seconds" + "Scheme__MaxSecondsForExecutionTooLow()" ); await expectRevert( unitializedWalletScheme.initialize( @@ -1521,7 +1521,7 @@ contract("WalletScheme", function (accounts) { executionTimeout, 5 ), - "avatar cannot be zero" + "Scheme__AvatarAddressCannotBeZero()" ); }); @@ -1536,7 +1536,7 @@ contract("WalletScheme", function (accounts) { executionTimeout, 5 ), - "cannot init twice" + "Scheme__CannotInitTwice()" ); }); From 6dc8febe90cee73250a3f4544ddb2c4c84449d3f Mon Sep 17 00:00:00 2001 From: Dino Date: Tue, 8 Nov 2022 18:49:52 -0300 Subject: [PATCH 15/28] test(AvatarScheme): removed unused code --- test/dao/schemes/AvatarScheme.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index 9124ad67..6743f052 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -18,7 +18,6 @@ const ActionMock = artifacts.require("./ActionMock.sol"); contract("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, - registrarScheme, avatarScheme, walletScheme, org, @@ -103,13 +102,7 @@ contract("AvatarScheme", function (accounts) { const callDataMintRep = await org.controller.contract.methods .mintReputation(10, accounts[1]) .encodeABI(); - await permissionRegistry.setETHPermission( - org.avatar.address, - accounts[1], - callData.substring(0, 10), - 0, - true - ); + const tx = await avatarScheme.proposeCalls( [actionMock.address, org.controller.address], [callData, callDataMintRep], From 51692571725612eeddfb12c66ca6a8e993cc8eae Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 15:32:00 -0300 Subject: [PATCH 16/28] fix(Scheme): fixed wrong revert replaced a for a && because the logic was wrong --- contracts/dao/schemes/Scheme.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/dao/schemes/Scheme.sol b/contracts/dao/schemes/Scheme.sol index ef229729..ffa27161 100644 --- a/contracts/dao/schemes/Scheme.sol +++ b/contracts/dao/schemes/Scheme.sol @@ -132,7 +132,7 @@ abstract contract Scheme is DXDVotingMachineCallbacks { * @param _maxSecondsForExecution New max proposal time in seconds to be used */ function setMaxSecondsForExecution(uint256 _maxSecondsForExecution) external virtual { - if (msg.sender != address(avatar) || msg.sender != address(this)) { + if (msg.sender != address(avatar) && msg.sender != address(this)) { revert Scheme__SetMaxSecondsForExecutionInvalidCaller(); } From eaf3f448e35bc38b5899be82b4a5effe342c8f3b Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 15:32:47 -0300 Subject: [PATCH 17/28] test(AvatarScheme): updated voting logic on the tests to fix them after merged changes --- test/dao/schemes/AvatarScheme.js | 53 +++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index cd63407a..ffe44d56 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -15,7 +15,7 @@ const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); const ActionMock = artifacts.require("./ActionMock.sol"); -contract("AvatarScheme", function (accounts) { +contract.only("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, avatarScheme, @@ -97,6 +97,7 @@ contract("AvatarScheme", function (accounts) { true ); }); + it("should execute proposal", async function () { const callData = helpers.testCallFrom(org.avatar.address); const callDataMintRep = await org.controller.contract.methods @@ -197,9 +198,15 @@ contract("AvatarScheme", function (accounts) { ); const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); - await org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { - from: accounts[2], - }); + await org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ); const maxSecondsForExecution = await avatarScheme.maxSecondsForExecution(); assert.equal(maxSecondsForExecution.toNumber(), secondsToSet); @@ -228,9 +235,15 @@ contract("AvatarScheme", function (accounts) { ); const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); - await org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { - from: accounts[2], - }); + await org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ); const maxSecondsForExecution = await avatarScheme.maxSecondsForExecution(); assert.equal(maxSecondsForExecution.toNumber(), MIN_SECONDS_FOR_EXECUTION); @@ -260,16 +273,22 @@ contract("AvatarScheme", function (accounts) { const proposalId = await helpers.getValueFromLogs(trx, "_proposalId"); await expectRevert.unspecified( - org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { - from: accounts[2], - }) + org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ) ); }); it("setMaxSecondsForExecution only callable from the avatar", async function () { await expectRevert( avatarScheme.setMaxSecondsForExecution(TEST_VALUE), - "AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar" + "Scheme__SetMaxSecondsForExecutionInvalidCaller()" ); }); @@ -298,9 +317,15 @@ contract("AvatarScheme", function (accounts) { // Wait for maxSecondsForExecution await time.increase(defaultMaxSecondsForExecution); - await org.votingMachine.vote(proposalId, 1, 0, constants.NULL_ADDRESS, { - from: accounts[2], - }); + await org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ); const organizationProposal = await avatarScheme.getProposal(proposalId); From d6f932c5dcc70d5959f1d92ffd2554b64567b209 Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 16:29:55 -0300 Subject: [PATCH 18/28] refactor: replaced require with custom errors --- contracts/dao/schemes/AvatarScheme.sol | 14 +++--- contracts/dao/schemes/Scheme.sol | 61 +++++++++++++++++++------- contracts/dao/schemes/WalletScheme.sol | 27 +++--------- test/dao/schemes/AvatarScheme.js | 2 +- test/dao/schemes/WalletScheme.js | 9 ++-- 5 files changed, 62 insertions(+), 51 deletions(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index ced2a983..c6cce71c 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -13,12 +13,6 @@ import "./Scheme.sol"; contract AvatarScheme is Scheme { using Address for address; - /// @notice Emitted when setMaxSecondsForExecution NOT called from the avatar - error AvatarScheme__SetMaxSecondsForExecutionNotCalledFromAvatar(); - - /// @notice Emitted when trying to set maxSecondsForExecution to a value lower than 86400 - error AvatarScheme__MaxSecondsForExecutionTooLow(); - /// @notice Emitted when the proposal is already being executed error AvatarScheme__ProposalExecutionAlreadyRunning(); @@ -37,6 +31,9 @@ contract AvatarScheme is Scheme { /// @notice Emitted when ERC20 limits passed error AvatarScheme__ERC20LimitsPassed(); + /// @notice Emitted if the number of totalOptions is not 2 + error AvatarScheme__TotalOptionsMustBeTwo(); + /** * @dev Propose calls to be executed, the calls have to be allowed by the permission registry * @param _to - The addresses to call @@ -55,7 +52,10 @@ contract AvatarScheme is Scheme { string calldata _title, string calldata _descriptionHash ) public override returns (bytes32 proposalId) { - require(_totalOptions == 2, "AvatarScheme: The total amount of options should be 2"); + if (_totalOptions != 2) { + revert AvatarScheme__TotalOptionsMustBeTwo(); + } + return super.proposeCalls(_to, _callData, _value, _totalOptions, _title, _descriptionHash); } diff --git a/contracts/dao/schemes/Scheme.sol b/contracts/dao/schemes/Scheme.sol index ffa27161..9fb9c368 100644 --- a/contracts/dao/schemes/Scheme.sol +++ b/contracts/dao/schemes/Scheme.sol @@ -79,8 +79,23 @@ abstract contract Scheme is DXDVotingMachineCallbacks { /// @notice _to, _callData and _value must have all the same length error Scheme_InvalidParameterArrayLength(); - /// @notice Emitted when the total amount of options is not 2 - error Scheme__MustHaveTwoOptions(); + /// @notice Emitted when the totalOptions paramers is invalid + error Scheme__InvalidTotalOptionsOrActionsCallsLength(); + + /// @notice Emitted when the proposal is already being executed + error Scheme__ProposalExecutionAlreadyRunning(); + + /// @notice Emitted when the proposal isn't submitted + error Scheme__ProposalMustBeSubmitted(); + + /// @notice Emitted when the call failed. Returns the revert error + error Scheme__CallFailed(string reason); + + /// @notice Emitted when the maxRepPercentageChange is exceeded + error Scheme__MaxRepPercentageChangePassed(); + + /// @notice Emitted if the ERC20 limits are exceeded + error Scheme__ERC20LimitsPassed(); /** * @dev initialize @@ -161,9 +176,13 @@ abstract contract Scheme is DXDVotingMachineCallbacks { string calldata _title, string calldata _descriptionHash ) public virtual returns (bytes32 proposalId) { - require(_to.length == _callData.length, "Scheme: invalid _callData length"); - require(_to.length == _value.length, "Scheme: invalid _value length"); - require((_value.length % (_totalOptions - 1)) == 0, "Scheme: Invalid _totalOptions or action calls length"); + if (_to.length != _callData.length || _to.length != _value.length) { + revert Scheme_InvalidParameterArrayLength(); + } + + if ((_value.length % (_totalOptions - 1)) != 0) { + revert Scheme__InvalidTotalOptionsOrActionsCallsLength(); + } bytes32 voteParams = controller.getSchemeParameters(address(this)); @@ -203,11 +222,16 @@ abstract contract Scheme is DXDVotingMachineCallbacks { returns (bool) { // We use isExecutingProposal variable to avoid re-entrancy in proposal execution - require(!executingProposal, "WalletScheme: proposal execution already running"); + if (executingProposal) { + revert Scheme__ProposalExecutionAlreadyRunning(); + } executingProposal = true; Proposal storage proposal = proposals[_proposalId]; - require(proposal.state == ProposalState.Submitted, "WalletScheme: must be a submitted proposal"); + + if (proposal.state != ProposalState.Submitted) { + revert Scheme__ProposalMustBeSubmitted(); + } require( !controller.getSchemeCanMakeAvatarCalls(address(this)), @@ -253,22 +277,25 @@ abstract contract Scheme is DXDVotingMachineCallbacks { proposal.callData[callIndex] ); - require(callsSucessResult, string(returnData)); + if (!callsSucessResult) { + revert Scheme__CallFailed({reason: string(returnData)}); + } } } proposal.state = ProposalState.ExecutionSucceeded; // Cant mint or burn more REP than the allowed percentaged set in the wallet scheme initialization - require( - ((oldRepSupply * (uint256(100) + (maxRepPercentageChange))) / 100 >= - getNativeReputationTotalSupply()) && - ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 <= - getNativeReputationTotalSupply()), - "WalletScheme: maxRepPercentageChange passed" - ); - - require(permissionRegistry.checkERC20Limits(address(this)), "WalletScheme: ERC20 limits passed"); + if ( + ((oldRepSupply * (uint256(100) + (maxRepPercentageChange))) / 100 < getNativeReputationTotalSupply()) || + ((oldRepSupply * (uint256(100) - maxRepPercentageChange)) / 100 > getNativeReputationTotalSupply()) + ) { + revert Scheme__MaxRepPercentageChangePassed(); + } + + if (!permissionRegistry.checkERC20Limits(address(this))) { + revert Scheme__ERC20LimitsPassed(); + } emit ProposalStateChange(_proposalId, uint256(ProposalState.ExecutionSucceeded)); } diff --git a/contracts/dao/schemes/WalletScheme.sol b/contracts/dao/schemes/WalletScheme.sol index 36931cfc..dc5d23db 100644 --- a/contracts/dao/schemes/WalletScheme.sol +++ b/contracts/dao/schemes/WalletScheme.sol @@ -13,26 +13,8 @@ import "./Scheme.sol"; contract WalletScheme is Scheme { using Address for address; - /// @notice Emitted when setMaxSecondsForExecution NOT called from the scheme - error WalletScheme__SetMaxSecondsForExecutionNotCalledFromScheme(); - - /// @notice Emitted when trying to set maxSecondsForExecution to a value lower than 86400 - error WalletScheme__MaxSecondsForExecutionTooLow(); - - /// @notice Emitted when trying to execute an already running proposal - error WalletScheme__ProposalExecutionAlreadyRunning(); - - /// @notice Emitted when the proposal is not a submitted proposal - error WalletScheme__ProposalMustBeSubmitted(); - - /// @notice Emitted when making a call failed - error WalletScheme__CallFailed(string reason); - - /// @notice Emitted when exceeded the maximum rep supply % change - error WalletScheme__MaxRepPercentageChangePassed(); - - /// @notice Emitted when ERC20 limits are passed - error WalletScheme__ERC20LimitsPassed(); + /// @notice Emitted if the number of totalOptions is not 2 + error WalletScheme__TotalOptionsMustBeTwo(); /** * @dev Receive function that allows the wallet to receive ETH when the controller address is not set @@ -57,7 +39,10 @@ contract WalletScheme is Scheme { string calldata _title, string calldata _descriptionHash ) public override returns (bytes32 proposalId) { - require(_totalOptions == 2, "WalletScheme: The total amount of options should be 2"); + if (_totalOptions != 2) { + revert WalletScheme__TotalOptionsMustBeTwo(); + } + return super.proposeCalls(_to, _callData, _value, _totalOptions, _title, _descriptionHash); } diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index ffe44d56..53719cda 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -15,7 +15,7 @@ const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); const ActionMock = artifacts.require("./ActionMock.sol"); -contract.only("AvatarScheme", function (accounts) { +contract("AvatarScheme", function (accounts) { let standardTokenMock, permissionRegistry, avatarScheme, diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index e080cf21..62f26392 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -455,7 +455,7 @@ contract("WalletScheme", function (accounts) { constants.SOME_HASH ); const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); - await expectRevert( + await expectRevert.unspecified( org.votingMachine.vote( proposalId, constants.YES_OPTION, @@ -464,8 +464,7 @@ contract("WalletScheme", function (accounts) { { from: accounts[2], } - ), - "Scheme: _maxSecondsForExecution cant be less than 86400 seconds" + ) ); await time.increase(executionTimeout); @@ -1352,7 +1351,7 @@ contract("WalletScheme", function (accounts) { constants.ZERO_ADDRESS, { from: accounts[2] } ), - "WalletScheme__MaxRepPercentageChangePassed()" + "Scheme__MaxRepPercentageChangePassed()" ); assert.equal( @@ -1417,7 +1416,7 @@ contract("WalletScheme", function (accounts) { constants.ZERO_ADDRESS, { from: accounts[2] } ), - "WalletScheme__MaxRepPercentageChangePassed()" + "Scheme__MaxRepPercentageChangePassed()" ); assert( From cf04da8264a27de39c763c3aaeb0f84582ba0dc8 Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 16:58:18 -0300 Subject: [PATCH 19/28] fix(Scheme.sol): removed duplicated logic checking if the scheme can make avatar calls is only on WalletScheme --- contracts/dao/schemes/Scheme.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/contracts/dao/schemes/Scheme.sol b/contracts/dao/schemes/Scheme.sol index 9fb9c368..61506bda 100644 --- a/contracts/dao/schemes/Scheme.sol +++ b/contracts/dao/schemes/Scheme.sol @@ -233,11 +233,6 @@ abstract contract Scheme is DXDVotingMachineCallbacks { revert Scheme__ProposalMustBeSubmitted(); } - require( - !controller.getSchemeCanMakeAvatarCalls(address(this)), - "WalletScheme: scheme cannot make avatar calls" - ); - if (proposal.submittedTime + maxSecondsForExecution < block.timestamp) { // If the amount of time passed since submission plus max proposal time is lower than block timestamp // the proposal timeout execution is reached and proposal cant be executed from now on From c69f8c1adcad403a4a4cf8d7837660c3b2114b04 Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 16:58:58 -0300 Subject: [PATCH 20/28] refactor(WalletScheme): replaced require with custom errors --- contracts/dao/schemes/WalletScheme.sol | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/contracts/dao/schemes/WalletScheme.sol b/contracts/dao/schemes/WalletScheme.sol index dc5d23db..92ab7f0f 100644 --- a/contracts/dao/schemes/WalletScheme.sol +++ b/contracts/dao/schemes/WalletScheme.sol @@ -16,6 +16,9 @@ contract WalletScheme is Scheme { /// @notice Emitted if the number of totalOptions is not 2 error WalletScheme__TotalOptionsMustBeTwo(); + /// @notice Emitted if the WalletScheme can make avatar calls + error WalletScheme__CannotMakeAvatarCalls(); + /** * @dev Receive function that allows the wallet to receive ETH when the controller address is not set */ @@ -58,10 +61,10 @@ contract WalletScheme is Scheme { onlyVotingMachine returns (bool) { - require( - !controller.getSchemeCanMakeAvatarCalls(address(this)), - "WalletScheme: scheme cannot make avatar calls" - ); + if (controller.getSchemeCanMakeAvatarCalls(address(this))) { + revert WalletScheme__CannotMakeAvatarCalls(); + } + return super.executeProposal(_proposalId, _winningOption); } From a97297d1f359d896ec74a333cbe164ee57fabe12 Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 16:59:13 -0300 Subject: [PATCH 21/28] test(WalletScheme): fixed tests --- test/dao/schemes/WalletScheme.js | 42 ++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 62f26392..9622ded9 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -370,9 +370,9 @@ contract("WalletScheme", function (accounts) { }); it("MasterWalletScheme - setMaxSecondsForExecution is callable only form the avatar", async function () { - expectRevert( + await expectRevert( masterWalletScheme.setMaxSecondsForExecution(executionTimeout + 666), - "setMaxSecondsForExecution is callable only form the avatar" + "Scheme__SetMaxSecondsForExecutionInvalidCaller()" ); assert.equal( await masterWalletScheme.maxSecondsForExecution(), @@ -383,7 +383,7 @@ contract("WalletScheme", function (accounts) { it("MasterWalletScheme - proposal to change max proposal time - positive decision - proposal executed", async () => { const callData = helpers.encodeMaxSecondsForExecution(86400 + 666); - expectRevert( + await expectRevert( masterWalletScheme.proposeCalls( [masterWalletScheme.address], [callData], @@ -435,11 +435,12 @@ contract("WalletScheme", function (accounts) { it("MasterWalletScheme - proposal to change max proposal time fails - positive decision - proposal fails", async () => { const callData = helpers.encodeMaxSecondsForExecution(86400 - 1); - expectRevert( + await expectRevert( masterWalletScheme.proposeCalls( [masterWalletScheme.address], [callData], [1], + 2, constants.TEST_TITLE, constants.SOME_HASH ), @@ -497,21 +498,24 @@ contract("WalletScheme", function (accounts) { }); it("MasterWalletScheme - proposal with data or value to wallet scheme address fail", async function () { - expectRevert( + await expectRevert( masterWalletScheme.proposeCalls( [masterWalletScheme.address], ["0x00000000"], [1], + 2, constants.TEST_TITLE, constants.SOME_HASH ), "invalid proposal caller" ); - expectRevert( + + await expectRevert( masterWalletScheme.proposeCalls( [masterWalletScheme.address], ["0x00000000"], [1], + 2, constants.TEST_TITLE, constants.SOME_HASH ), @@ -524,30 +528,48 @@ contract("WalletScheme", function (accounts) { it("MasterWalletScheme - proposing proposal with different length of to and value fail", async function () { const callData = helpers.testCallFrom(masterWalletScheme.address); - expectRevert( + await expectRevert( masterWalletScheme.proposeCalls( [actionMock.address], [callData], [0, 0], + 2, constants.TEST_TITLE, constants.SOME_HASH ), - "invalid _value length" + "Scheme_InvalidParameterArrayLength()" ); - expectRevert( + await expectRevert( masterWalletScheme.proposeCalls( [actionMock.address], [callData, callData], [0], + 2, constants.TEST_TITLE, constants.SOME_HASH ), - "invalid _callData length" + "Scheme_InvalidParameterArrayLength()" ); assert.equal(await masterWalletScheme.getOrganizationProposalsLength(), 0); }); + it("MasterWalletScheme - cannot make a proposal with more than 2 options", async function () { + const callData = helpers.testCallFrom(masterWalletScheme.address); + + await expectRevert( + masterWalletScheme.proposeCalls( + [actionMock.address], + [callData], + [0], + 3, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "WalletScheme__TotalOptionsMustBeTwo()" + ); + }); + it("MasterWalletScheme - proposal with data - negative decision - proposal rejected", async function () { const callData = helpers.testCallFrom(masterWalletScheme.address); From 9a35d70a3a8b6d5cc031f48812213abde5072aba Mon Sep 17 00:00:00 2001 From: Dino Date: Wed, 9 Nov 2022 17:17:42 -0300 Subject: [PATCH 22/28] test(WalletScheme): removed unused tests --- test/dao/schemes/WalletScheme.js | 145 ------------------------------- 1 file changed, 145 deletions(-) diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 9622ded9..7e16b6e6 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -380,151 +380,6 @@ contract("WalletScheme", function (accounts) { ); }); - it("MasterWalletScheme - proposal to change max proposal time - positive decision - proposal executed", async () => { - const callData = helpers.encodeMaxSecondsForExecution(86400 + 666); - - await expectRevert( - masterWalletScheme.proposeCalls( - [masterWalletScheme.address], - [callData], - [1], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ), - "invalid proposal caller" - ); - - const tx = await masterWalletScheme.proposeCalls( - [masterWalletScheme.address], - [callData], - [0], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ); - const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); - - await org.votingMachine.vote( - proposalId, - constants.YES_OPTION, - 0, - constants.ZERO_ADDRESS, - { - from: accounts[2], - } - ); - - const organizationProposal = await masterWalletScheme.getProposal( - proposalId - ); - assert.equal( - organizationProposal.state, - constants.WALLET_SCHEME_PROPOSAL_STATES.executionSuccedd - ); - assert.equal(organizationProposal.callData[0], callData); - assert.equal(organizationProposal.to[0], masterWalletScheme.address); - assert.equal(organizationProposal.value[0], 0); - assert.equal( - await masterWalletScheme.maxSecondsForExecution(), - 86400 + 666 - ); - }); - - // eslint-disable-next-line max-len - it("MasterWalletScheme - proposal to change max proposal time fails - positive decision - proposal fails", async () => { - const callData = helpers.encodeMaxSecondsForExecution(86400 - 1); - - await expectRevert( - masterWalletScheme.proposeCalls( - [masterWalletScheme.address], - [callData], - [1], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ), - "invalid proposal caller" - ); - - const tx = await masterWalletScheme.proposeCalls( - [masterWalletScheme.address], - [callData], - [0], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ); - const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); - await expectRevert.unspecified( - org.votingMachine.vote( - proposalId, - constants.YES_OPTION, - 0, - constants.ZERO_ADDRESS, - { - from: accounts[2], - } - ) - ); - - await time.increase(executionTimeout); - - await org.votingMachine.vote( - proposalId, - constants.YES_OPTION, - 0, - constants.ZERO_ADDRESS, - { - from: accounts[2], - } - ); - - const organizationProposal = await masterWalletScheme.getProposal( - proposalId - ); - assert.equal( - organizationProposal.state, - constants.WALLET_SCHEME_PROPOSAL_STATES.executionTimeout - ); - - assert.equal(organizationProposal.callData[0], callData); - assert.equal(organizationProposal.to[0], masterWalletScheme.address); - assert.equal(organizationProposal.value[0], 0); - assert.equal( - await masterWalletScheme.maxSecondsForExecution(), - executionTimeout - ); - }); - - it("MasterWalletScheme - proposal with data or value to wallet scheme address fail", async function () { - await expectRevert( - masterWalletScheme.proposeCalls( - [masterWalletScheme.address], - ["0x00000000"], - [1], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ), - "invalid proposal caller" - ); - - await expectRevert( - masterWalletScheme.proposeCalls( - [masterWalletScheme.address], - ["0x00000000"], - [1], - 2, - constants.TEST_TITLE, - constants.SOME_HASH - ), - "invalid proposal caller" - ); - - assert.equal(await masterWalletScheme.getOrganizationProposalsLength(), 0); - }); - it("MasterWalletScheme - proposing proposal with different length of to and value fail", async function () { const callData = helpers.testCallFrom(masterWalletScheme.address); From 16bf60e7b36bf605fb120790b61831c91163cf13 Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 10 Nov 2022 15:14:23 -0300 Subject: [PATCH 23/28] feat(AvatarScheme): added revert message to custom error --- contracts/dao/schemes/AvatarScheme.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/dao/schemes/AvatarScheme.sol b/contracts/dao/schemes/AvatarScheme.sol index c6cce71c..e1cde461 100644 --- a/contracts/dao/schemes/AvatarScheme.sol +++ b/contracts/dao/schemes/AvatarScheme.sol @@ -121,7 +121,7 @@ contract AvatarScheme is Scheme { (callDataFuncSignature == bytes4(keccak256("mintReputation(uint256,address)")) || callDataFuncSignature == bytes4(keccak256("burnReputation(uint256,address)"))) ) { - (callsSucessResult, ) = address(controller).call(proposal.callData[callIndex]); + (callsSucessResult, returnData) = address(controller).call(proposal.callData[callIndex]); } else { // The permission registry keeps track of all value transferred and checks call permission (callsSucessResult, returnData) = controller.avatarCall( @@ -146,6 +146,7 @@ contract AvatarScheme is Scheme { proposal.value[callIndex] ); } + if (!callsSucessResult) { revert AvatarScheme__AvatarCallFailed({reason: string(returnData)}); } From d3119f0d5198f949cde7cdcd47deabbcf2ac2212 Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 10 Nov 2022 15:14:47 -0300 Subject: [PATCH 24/28] test(AvatarScheme): added tests to improve coverage --- test/dao/schemes/AvatarScheme.js | 179 ++++++++++++++++++++++++++++++- 1 file changed, 174 insertions(+), 5 deletions(-) diff --git a/test/dao/schemes/AvatarScheme.js b/test/dao/schemes/AvatarScheme.js index 53719cda..62fa4833 100644 --- a/test/dao/schemes/AvatarScheme.js +++ b/test/dao/schemes/AvatarScheme.js @@ -129,6 +129,76 @@ contract("AvatarScheme", function (accounts) { ); }); + it("can burn rep", async function () { + const initialReputation = await org.reputation.balanceOf(accounts[1]); + let currentReputation; + + const repChange = 10; + + const callDataMintRep = await org.controller.contract.methods + .mintReputation(repChange, accounts[1]) + .encodeABI(); + + const mintRepTx = await avatarScheme.proposeCalls( + [org.controller.address], + [callDataMintRep], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalIdMintRep = await helpers.getValueFromLogs( + mintRepTx, + "_proposalId" + ); + await org.votingMachine.vote( + proposalIdMintRep, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ); + + currentReputation = await org.reputation.balanceOf(accounts[1]); + + assert.equal( + currentReputation.toNumber(), + initialReputation.toNumber() + repChange + ); + + const callDataBurnRep = await org.controller.contract.methods + .burnReputation(repChange, accounts[1]) + .encodeABI(); + + const burnRepTx = await avatarScheme.proposeCalls( + [org.controller.address], + [callDataBurnRep], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalIdBurnRep = await helpers.getValueFromLogs( + burnRepTx, + "_proposalId" + ); + await org.votingMachine.vote( + proposalIdBurnRep, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ); + + currentReputation = await org.reputation.balanceOf(accounts[1]); + + assert.equal(currentReputation.toNumber(), initialReputation.toNumber()); + }); + it("should return the function signature when the length is greater than 4 bytes", async function () { const functionSignature = await avatarScheme.getFuncSignature(SOME_HASH); assert.equal(SOME_HASH.substring(0, 10), functionSignature); @@ -151,10 +221,7 @@ contract("AvatarScheme", function (accounts) { }); it("should get the number of proposals created", async function () { - const createRandomAmountOfProposals = async maxNumberOfProposals => { - const numberOfProposals = - 1 + Math.floor(Math.random() * (maxNumberOfProposals - 1)); - + const createProposals = async numberOfProposals => { const callData = helpers.testCallFrom(org.avatar.address); for (let i = 1; i <= numberOfProposals; i++) { @@ -171,7 +238,9 @@ contract("AvatarScheme", function (accounts) { return numberOfProposals; }; - const numberOfProposalsCreated = await createRandomAmountOfProposals(6); + const randomNumber = helpers.getRandomNumber(1, 10); + + const numberOfProposalsCreated = await createProposals(randomNumber); const organizationProposals = await avatarScheme.getOrganizationProposals(); assert.equal(organizationProposals.length, numberOfProposalsCreated); }); @@ -339,4 +408,104 @@ contract("AvatarScheme", function (accounts) { const schemeType = await avatarScheme.getSchemeType(); assert.equal(schemeType, "AvatarScheme_v1"); }); + + it("cannot execute a proposal if the number of options is not 2", async function () { + const callData = helpers.testCallFrom(org.avatar.address); + + await expectRevert( + avatarScheme.proposeCalls( + [actionMock.address], + [callData], + [0], + 3, + constants.TEST_TITLE, + constants.SOME_HASH + ), + "AvatarScheme__TotalOptionsMustBeTwo()" + ); + }); + + it("cannot mint more rep than maxRepPercentageChange", async function () { + const maxRepPercentageChange = await avatarScheme.maxRepPercentageChange(); + const repSupply = await org.reputation.totalSupply(); + const maxRepChangeAllowed = + (repSupply.toNumber() * (100 + maxRepPercentageChange.toNumber())) / 100; + + const callDataMintRep = await org.controller.contract.methods + .mintReputation(maxRepChangeAllowed + 1, accounts[1]) + .encodeABI(); + + const mintRepTx = await avatarScheme.proposeCalls( + [org.controller.address], + [callDataMintRep], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalIdMintRep = await helpers.getValueFromLogs( + mintRepTx, + "_proposalId" + ); + + await expectRevert( + org.votingMachine.vote( + proposalIdMintRep, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ), + "AvatarScheme__MaxRepPercentageChangePassed()" + ); + }); + + it("cannot burn more rep than maxRepPercentageChange", async function () { + const maxRepPercentageChange = await avatarScheme.maxRepPercentageChange(); + const repSupply = await org.reputation.totalSupply(); + const maxRepChangeAllowed = -( + (repSupply.toNumber() * (100 - maxRepPercentageChange.toNumber())) / 100 - + repSupply.toNumber() + ); + + const callDataBurnRep = await org.controller.contract.methods + .burnReputation(maxRepChangeAllowed + 1, accounts[2]) + .encodeABI(); + + await permissionRegistry.setETHPermission( + avatarScheme.address, + org.controller.address, + callDataBurnRep.substring(0, 10), + 0, + true + ); + + const burnRepTx = await avatarScheme.proposeCalls( + [org.controller.address], + [callDataBurnRep], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalIdBurnRep = await helpers.getValueFromLogs( + burnRepTx, + "_proposalId" + ); + + await expectRevert( + org.votingMachine.vote( + proposalIdBurnRep, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ), + "AvatarScheme__MaxRepPercentageChangePassed()" + ); + }); }); From 4e484dc36f62d4c0eb03fceb3940dc6c05ed1140 Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 10 Nov 2022 15:15:06 -0300 Subject: [PATCH 25/28] feat(helpers): added helper function to get random number --- test/helpers/index.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/helpers/index.js b/test/helpers/index.js index 7255ecbb..595cb7b5 100644 --- a/test/helpers/index.js +++ b/test/helpers/index.js @@ -239,4 +239,13 @@ export function encodeMaxSecondsForExecution(executionTimeout) { return setMaxSecondsForExecutionData; } +export function getRandomNumber(min, max = min) { + // If there is just one argument, the minimum is set to zero, and the maximum is the argument + if ((min = max)) min = 0; + else Math.ceil(min); + + max = Math.floor(max); + return Math.floor(Math.random() * (max - min + 1) + min); // The maximum is inclusive and the minimum is inclusive +} + export { constants }; From 0b672918b7030670778e5c14678d15d9d2de5f72 Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 10 Nov 2022 16:30:11 -0300 Subject: [PATCH 26/28] test(WalletScheme): added tests to increment coverage --- test/dao/schemes/WalletScheme.js | 127 ++++++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 3 deletions(-) diff --git a/test/dao/schemes/WalletScheme.js b/test/dao/schemes/WalletScheme.js index 7e16b6e6..5c9fcd17 100644 --- a/test/dao/schemes/WalletScheme.js +++ b/test/dao/schemes/WalletScheme.js @@ -4,7 +4,6 @@ import * as helpers from "../../helpers"; const { fixSignature } = require("../../helpers/sign"); const { time, expectRevert } = require("@openzeppelin/test-helpers"); -const AvatarScheme = artifacts.require("./AvatarScheme.sol"); const WalletScheme = artifacts.require("./WalletScheme.sol"); const PermissionRegistry = artifacts.require("./PermissionRegistry.sol"); const ERC20Mock = artifacts.require("./ERC20Mock.sol"); @@ -501,6 +500,52 @@ contract("WalletScheme", function (accounts) { assert.equal(organizationProposal.value[0], 0); }); + it("WalletScheme - check that it cannot make avatar calls", async function () { + const newWallet = await WalletScheme.new(); + await newWallet.initialize( + org.avatar.address, + org.votingMachine.address, + org.controller.address, + permissionRegistry.address, + "New Wallet", + executionTimeout, + 5 + ); + + await org.controller.registerScheme( + newWallet.address, + defaultParamsHash, + false, + true, + true + ); + + const callData = helpers.testCallFrom(newWallet.address); + + const tx = await newWallet.proposeCalls( + [newWallet.address], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); + + await expectRevert( + org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ), + "WalletScheme__CannotMakeAvatarCalls()" + ); + }); + it.skip("MasterWalletScheme - proposal with data - positive decision - proposal executed", async function () { const callData = helpers.encodeMaxSecondsForExecution(executionTimeout); @@ -767,6 +812,60 @@ contract("WalletScheme", function (accounts) { ); }); + it("Global ETH transfer value not allowed value by permission registry - zero address", async function () { + const callData = helpers.testCallFrom(masterWalletScheme.address); + + const tx = await masterWalletScheme.proposeCalls( + [constants.ZERO_ADDRESS], + [callData], + [101], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); + + await expectRevert( + org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ), + "PermissionRegistry: Value limit reached" + ); + }); + + it("Global ETH transfer call not allowed value by permission registry - zero address, no value", async function () { + const callData = helpers.testCallFrom(masterWalletScheme.address); + + const tx = await masterWalletScheme.proposeCalls( + [constants.ZERO_ADDRESS], + [callData], + [0], + 2, + constants.TEST_TITLE, + constants.SOME_HASH + ); + const proposalId = await helpers.getValueFromLogs(tx, "_proposalId"); + + await expectRevert( + org.votingMachine.vote( + proposalId, + constants.YES_OPTION, + 0, + constants.ZERO_ADDRESS, + { + from: accounts[2], + } + ), + "PermissionRegistry: Call not allowed" + ); + }); + // eslint-disable-next-line max-len it("MasterWalletScheme - positive decision - proposal executed - not allowed value by permission registry in multiple calls", async function () { await web3.eth.sendTransaction({ @@ -1475,7 +1574,7 @@ contract("WalletScheme", function (accounts) { assert.equal(organizationProposal.value[2], 0); }); - it("MasterWalletScheme - cant initialize with wrong values", async function () { + it("MasterWalletScheme - cannot initialize with maxSecondsForExecution too log", async function () { const unitializedWalletScheme = await WalletScheme.new(); await expectRevert( @@ -1490,6 +1589,11 @@ contract("WalletScheme", function (accounts) { ), "Scheme__MaxSecondsForExecutionTooLow()" ); + }); + + it("MasterWalletScheme - cant initialize if avatar address is zero", async function () { + const unitializedWalletScheme = await WalletScheme.new(); + await expectRevert( unitializedWalletScheme.initialize( constants.ZERO_ADDRESS, @@ -1504,6 +1608,23 @@ contract("WalletScheme", function (accounts) { ); }); + it("MasterWalletScheme - cant initialize if controller address is zero", async function () { + const unitializedWalletScheme = await WalletScheme.new(); + + await expectRevert( + unitializedWalletScheme.initialize( + org.avatar.address, + accounts[0], + constants.ZERO_ADDRESS, + permissionRegistry.address, + "Master Wallet", + executionTimeout, + 5 + ), + "Scheme__ControllerAddressCannotBeZero()" + ); + }); + it("MasterWalletScheme - cannot initialize twice", async function () { await expectRevert( masterWalletScheme.initialize( @@ -1519,7 +1640,7 @@ contract("WalletScheme", function (accounts) { ); }); - it("MasterWalletScheme can receive value in contractt", async function () { + it("MasterWalletScheme can receive value in contract", async function () { await web3.eth.sendTransaction({ from: accounts[0], to: masterWalletScheme.address, From cbe6780a2dcaea1c8c4b16f03f7abb82200a8a3c Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 10 Nov 2022 16:51:52 -0300 Subject: [PATCH 27/28] test(DAOController): removed .skip(s) that were fixed in other PR --- test/dao/DAOController.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/dao/DAOController.js b/test/dao/DAOController.js index ea0364b3..1950dac9 100644 --- a/test/dao/DAOController.js +++ b/test/dao/DAOController.js @@ -13,7 +13,7 @@ const getRandomProposalIds = (n = 10) => .fill() .map(() => createProposalId()); -contract("DAOController", function (accounts) { +contract.only("DAOController", function (accounts) { let reputation, controller, avatar, @@ -97,7 +97,7 @@ contract("DAOController", function (accounts) { }); // eslint-disable-next-line max-len - it.skip("registerScheme() should not allow subtracting from schemesWithManageSchemesPermission if there is only 1 scheme with manage schemes permissions", async function () { + it("registerScheme() should not allow subtracting from schemesWithManageSchemesPermission if there is only 1 scheme with manage schemes permissions", async function () { // change scheme with _canManageSchemes=false const registerCall = controller.registerScheme( schemeAddress, @@ -114,7 +114,7 @@ contract("DAOController", function (accounts) { }); // eslint-disable-next-line max-len - it.skip("registerScheme() should subtract from schemesWithManageSchemesPermission counter if _canManageSchemes is set to false in a registered scheme", async function () { + it("registerScheme() should subtract from schemesWithManageSchemesPermission counter if _canManageSchemes is set to false in a registered scheme", async function () { // register new scheme with manage schemes permissions const newSchemeAddress = accounts[10]; await controller.registerScheme( @@ -163,7 +163,7 @@ contract("DAOController", function (accounts) { ); }); - it.skip('registerScheme() should reject with: "DAOController: Sender cannot manage schemes"', async function () { + it('registerScheme() should reject with: "DAOController: Sender cannot manage schemes"', async function () { const schemeThatCanNotManageSchemes = accounts[10]; await controller.registerScheme( schemeThatCanNotManageSchemes, @@ -188,7 +188,7 @@ contract("DAOController", function (accounts) { ); }); - it.skip('avatarCall() should reject with: "DAOController: Sender cannot perform avatar calls"', async function () { + it('avatarCall() should reject with: "DAOController: Sender cannot perform avatar calls"', async function () { const schemeThatCanNotMakeAvatarCalls = accounts[10]; await controller.registerScheme( schemeThatCanNotMakeAvatarCalls, @@ -215,7 +215,7 @@ contract("DAOController", function (accounts) { }); // eslint-disable-next-line max-len - it.skip("startProposal() shoul not allow a scheme assign itself as the proposer of a certain proposal ID", async () => { + it("startProposal() shoul not allow a scheme assign itself as the proposer of a certain proposal ID", async () => { const newSchemeAddress = accounts[1]; await controller.registerScheme( newSchemeAddress, @@ -238,7 +238,7 @@ contract("DAOController", function (accounts) { ); }); - it.skip("endProposal() should fail if caller is not the scheme that started the proposal", async () => { + it("endProposal() should fail if caller is not the scheme that started the proposal", async () => { const newSchemeAddress = accounts[1]; await controller.registerScheme( newSchemeAddress, From ff25b70a735438cef66ea058f2b2146ba8fffe0f Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 10 Nov 2022 16:52:35 -0300 Subject: [PATCH 28/28] test(DAOController): removed .only --- test/dao/DAOController.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/dao/DAOController.js b/test/dao/DAOController.js index 1950dac9..9cefe167 100644 --- a/test/dao/DAOController.js +++ b/test/dao/DAOController.js @@ -13,7 +13,7 @@ const getRandomProposalIds = (n = 10) => .fill() .map(() => createProposalId()); -contract.only("DAOController", function (accounts) { +contract("DAOController", function (accounts) { let reputation, controller, avatar,