From d911a8bd9d79f514f6fd5b6131a5495efcb6d0ee Mon Sep 17 00:00:00 2001 From: Mathew Cormier Date: Tue, 10 Mar 2020 10:39:25 -0400 Subject: [PATCH 1/8] chore: update Aragon chat links (#576) --- CONTRIBUTING.md | 4 ++-- readme.md | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 51b282139..6aa96299e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -42,7 +42,7 @@ If the answer to either of those two questions are "yes", then you're probably d ## 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. + - You may want to ask on the issue or in the [aragonOS Spectrum channel](https://spectrum.chat/aragon/aragonos) 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! @@ -69,4 +69,4 @@ aragonOS is generally meant to be used as a library by developers but includes c ## 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! +If you need help, please reach out to Aragon core contributors and community members in the [aragonOS Spectrum channel](https://spectrum.chat/aragon/aragonos). We'd love to hear from you and know what you're working on! diff --git a/readme.md b/readme.md index 6ea126912..958e7c487 100644 --- a/readme.md +++ b/readme.md @@ -43,3 +43,7 @@ Check the [Aragon Developer Portal](https://hack.aragon.org) for detailed docume ## Contributing For more details about contributing to aragonOS, please check the [contributing guide](./CONTRIBUTING.md). + +## Help + +For help and support, feel free to contact us at any time on the [aragonOS Spectrum channel](https://spectrum.chat/aragon/aragonos). \ No newline at end of file From 8d40feaf6495223d31c0178b48b4894f47b67b5b Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Fri, 29 May 2020 10:41:40 -0300 Subject: [PATCH 2/8] ACL: Fix inline doc and functions indentation (#579) --- contracts/acl/ACL.sol | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 6ac8a57ec..841ee1c0f 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -32,7 +32,6 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { 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)); @@ -233,10 +232,10 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { } /** - * @dev Function called by apps to check ACL on kernel or to check permission statu + * @dev Function called by apps to check ACL on kernel or to check permission status * @param _who Sender of the original call * @param _where Address of the app - * @param _where Identifier for a group of actions in app + * @param _what Identifier for a group of actions in app (role) * @param _how Permission parameters * @return boolean indicating whether the ACL allows the role or not */ @@ -269,7 +268,10 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { address _where, bytes32 _what, uint256[] _how - ) public view returns (bool) + ) + public + view + returns (bool) { if (_paramsHash == EMPTY_PARAM_HASH) { return true; @@ -322,7 +324,10 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { address _where, bytes32 _what, uint256[] _how - ) internal view returns (bool) + ) + internal + view + returns (bool) { if (_paramId >= permissionParams[_paramsHash].length) { return false; // out of bounds From 7d53d168a9c1eada2c32f835690e3a929540c3e1 Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Fri, 26 Jun 2020 11:56:23 -0300 Subject: [PATCH 3/8] Disputable: Implement disputable base app (#581) * disputable: implement disputable base app * disputable: fix linter * disputable: remove circular dependency * disputable: rename base app * disputable: organize functions and improve documentation * disputable: improve interface and documentation * disputable: improve interfaces * disputable: add setting ID to IAgreement * disputables: improve functions layout * IAgreement: re-order events and functions * DisputableAragonApp: update comments * disputables: improve interfaces * disputables: increase coverage * disputables: remove web3-provider-engine dep Co-authored-by: Brett Sun --- .../apps/disputable/DisputableAragonApp.sol | 161 +++++++++ contracts/apps/disputable/IAgreement.sol | 103 ++++++ contracts/apps/disputable/IDisputable.sol | 42 +++ contracts/lib/arbitration/IArbitrable.sol | 62 ++++ contracts/lib/arbitration/IArbitrator.sol | 52 +++ contracts/lib/standards/ERC165.sol | 15 + .../mocks/apps/disputable/AgreementMock.sol | 13 + .../apps/disputable/DisputableAppMock.sol | 82 +++++ .../mocks/lib/arbitration/ArbitrableMock.sol | 30 ++ .../apps/disputable/disputable_app.js | 309 ++++++++++++++++++ test/contracts/lib/arbitration/arbitrable.js | 29 ++ 11 files changed, 898 insertions(+) create mode 100644 contracts/apps/disputable/DisputableAragonApp.sol create mode 100644 contracts/apps/disputable/IAgreement.sol create mode 100644 contracts/apps/disputable/IDisputable.sol create mode 100644 contracts/lib/arbitration/IArbitrable.sol create mode 100644 contracts/lib/arbitration/IArbitrator.sol create mode 100644 contracts/lib/standards/ERC165.sol create mode 100644 contracts/test/mocks/apps/disputable/AgreementMock.sol create mode 100644 contracts/test/mocks/apps/disputable/DisputableAppMock.sol create mode 100644 contracts/test/mocks/lib/arbitration/ArbitrableMock.sol create mode 100644 test/contracts/apps/disputable/disputable_app.js create mode 100644 test/contracts/lib/arbitration/arbitrable.js diff --git a/contracts/apps/disputable/DisputableAragonApp.sol b/contracts/apps/disputable/DisputableAragonApp.sol new file mode 100644 index 000000000..cf13062dd --- /dev/null +++ b/contracts/apps/disputable/DisputableAragonApp.sol @@ -0,0 +1,161 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + +import "./IAgreement.sol"; +import "./IDisputable.sol"; +import "../AragonApp.sol"; +import "../../lib/token/ERC20.sol"; +import "../../lib/math/SafeMath64.sol"; + + +contract DisputableAragonApp is IDisputable, AragonApp { + /* Validation errors */ + string internal constant ERROR_SENDER_NOT_AGREEMENT = "DISPUTABLE_SENDER_NOT_AGREEMENT"; + string internal constant ERROR_AGREEMENT_STATE_INVALID = "DISPUTABLE_AGREEMENT_STATE_INVAL"; + + // This role is used to protect who can challenge actions in derived Disputable apps. However, it is not required + // to be validated in the app itself as the connected Agreement is responsible for performing the check on a challenge. + // bytes32 public constant CHALLENGE_ROLE = keccak256("CHALLENGE_ROLE"); + bytes32 public constant CHALLENGE_ROLE = 0xef025787d7cd1a96d9014b8dc7b44899b8c1350859fb9e1e05f5a546dd65158d; + + // bytes32 public constant SET_AGREEMENT_ROLE = keccak256("SET_AGREEMENT_ROLE"); + bytes32 public constant SET_AGREEMENT_ROLE = 0x8dad640ab1b088990c972676ada708447affc660890ec9fc9a5483241c49f036; + + // bytes32 internal constant AGREEMENT_POSITION = keccak256("aragonOS.appStorage.agreement"); + bytes32 internal constant AGREEMENT_POSITION = 0x6dbe80ccdeafbf5f3fff5738b224414f85e9370da36f61bf21c65159df7409e9; + + modifier onlyAgreement() { + require(address(_getAgreement()) == msg.sender, ERROR_SENDER_NOT_AGREEMENT); + _; + } + + /** + * @notice Challenge disputable action #`_disputableActionId` + * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier + * is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook. + * @param _disputableActionId Identifier of the action to be challenged + * @param _challengeId Identifier of the challenge in the context of the Agreement + * @param _challenger Address that submitted the challenge + */ + function onDisputableActionChallenged(uint256 _disputableActionId, uint256 _challengeId, address _challenger) external onlyAgreement { + _onDisputableActionChallenged(_disputableActionId, _challengeId, _challenger); + } + + /** + * @notice Allow disputable action #`_disputableActionId` + * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier + * is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook. + * @param _disputableActionId Identifier of the action to be allowed + */ + function onDisputableActionAllowed(uint256 _disputableActionId) external onlyAgreement { + _onDisputableActionAllowed(_disputableActionId); + } + + /** + * @notice Reject disputable action #`_disputableActionId` + * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier + * is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook. + * @param _disputableActionId Identifier of the action to be rejected + */ + function onDisputableActionRejected(uint256 _disputableActionId) external onlyAgreement { + _onDisputableActionRejected(_disputableActionId); + } + + /** + * @notice Void disputable action #`_disputableActionId` + * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier + * is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook. + * @param _disputableActionId Identifier of the action to be voided + */ + function onDisputableActionVoided(uint256 _disputableActionId) external onlyAgreement { + _onDisputableActionVoided(_disputableActionId); + } + + /** + * @notice Set Agreement to `_agreement` + * @param _agreement Agreement instance to be set + */ + function setAgreement(IAgreement _agreement) external auth(SET_AGREEMENT_ROLE) { + IAgreement agreement = _getAgreement(); + require(agreement == IAgreement(0) && _agreement != IAgreement(0), ERROR_AGREEMENT_STATE_INVALID); + + AGREEMENT_POSITION.setStorageAddress(address(_agreement)); + emit AgreementSet(_agreement); + } + + /** + * @dev Tell the linked Agreement + * @return Agreement + */ + function getAgreement() external view returns (IAgreement) { + return _getAgreement(); + } + + /** + * @dev Internal implementation of the `onDisputableActionChallenged` hook + * @param _disputableActionId Identifier of the action to be challenged + * @param _challengeId Identifier of the challenge in the context of the Agreement + * @param _challenger Address that submitted the challenge + */ + function _onDisputableActionChallenged(uint256 _disputableActionId, uint256 _challengeId, address _challenger) internal; + + /** + * @dev Internal implementation of the `onDisputableActionRejected` hook + * @param _disputableActionId Identifier of the action to be rejected + */ + function _onDisputableActionRejected(uint256 _disputableActionId) internal; + + /** + * @dev Internal implementation of the `onDisputableActionAllowed` hook + * @param _disputableActionId Identifier of the action to be allowed + */ + function _onDisputableActionAllowed(uint256 _disputableActionId) internal; + + /** + * @dev Internal implementation of the `onDisputableActionVoided` hook + * @param _disputableActionId Identifier of the action to be voided + */ + function _onDisputableActionVoided(uint256 _disputableActionId) internal; + + /** + * @dev Create a new action in the Agreement + * @param _disputableActionId Identifier of the action in the context of the Disputable + * @param _context Link to human-readable context for the given action + * @param _submitter Address that submitted the action + * @return Unique identifier for the created action in the context of the Agreement + */ + function _newAgreementAction(uint256 _disputableActionId, bytes _context, address _submitter) internal returns (uint256) { + IAgreement agreement = _ensureAgreement(); + return agreement.newAction(_disputableActionId, _context, _submitter); + } + + /** + * @dev Close action in the Agreement + * @param _actionId Identifier of the action in the context of the Agreement + */ + function _closeAgreementAction(uint256 _actionId) internal { + IAgreement agreement = _ensureAgreement(); + agreement.closeAction(_actionId); + } + + /** + * @dev Tell the linked Agreement + * @return Agreement + */ + function _getAgreement() internal view returns (IAgreement) { + return IAgreement(AGREEMENT_POSITION.getStorageAddress()); + } + + /** + * @dev Tell the linked Agreement or revert if it has not been set + * @return Agreement + */ + function _ensureAgreement() internal view returns (IAgreement) { + IAgreement agreement = _getAgreement(); + require(agreement != IAgreement(0), ERROR_AGREEMENT_STATE_INVALID); + return agreement; + } +} diff --git a/contracts/apps/disputable/IAgreement.sol b/contracts/apps/disputable/IAgreement.sol new file mode 100644 index 000000000..333cb7a58 --- /dev/null +++ b/contracts/apps/disputable/IAgreement.sol @@ -0,0 +1,103 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + +import "../../acl/IACLOracle.sol"; +import "../../lib/token/ERC20.sol"; +import "../../lib/arbitration/IArbitrable.sol"; + + +contract IAgreement is IArbitrable, IACLOracle { + + event Signed(address indexed signer, uint256 settingId); + event SettingChanged(uint256 settingId); + event DisputableAppActivated(address indexed disputable); + event DisputableAppDeactivated(address indexed disputable); + event CollateralRequirementChanged(address indexed disputable, uint256 collateralRequirementId); + event ActionSubmitted(uint256 indexed actionId, address indexed disputable); + event ActionClosed(uint256 indexed actionId); + event ActionChallenged(uint256 indexed actionId, uint256 indexed challengeId); + event ActionSettled(uint256 indexed actionId, uint256 indexed challengeId); + event ActionDisputed(uint256 indexed actionId, uint256 indexed challengeId); + event ActionAccepted(uint256 indexed actionId, uint256 indexed challengeId); + event ActionVoided(uint256 indexed actionId, uint256 indexed challengeId); + event ActionRejected(uint256 indexed actionId, uint256 indexed challengeId); + + enum ChallengeState { + Waiting, + Settled, + Disputed, + Rejected, + Accepted, + Voided + } + + function sign() external; + + function activate( + address _disputable, + ERC20 _collateralToken, + uint256 _actionAmount, + uint256 _challengeAmount, + uint64 _challengeDuration + ) + external; + + function deactivate(address _disputable) external; + + function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external returns (uint256); + + function closeAction(uint256 _actionId) external; + + function challengeAction(uint256 _actionId, uint256 _settlementOffer, bool _finishedSubmittingEvidence, bytes _context) external; + + function settleAction(uint256 _actionId) external; + + function disputeAction(uint256 _actionId, bool _finishedSubmittingEvidence) external; + + function getSigner(address _signer) external view returns (uint256 lastSettingIdSigned, bool mustSign); + + function getCurrentSettingId() external view returns (uint256); + + function getSetting(uint256 _settingId) external view returns (IArbitrator arbitrator, string title, bytes content); + + function getDisputableInfo(address _disputable) external view returns (bool registered, uint256 currentCollateralRequirementId); + + function getCollateralRequirement(address _disputable, uint256 _collateralId) external view + returns ( + ERC20 collateralToken, + uint256 actionAmount, + uint256 challengeAmount, + uint64 challengeDuration + ); + + function getAction(uint256 _actionId) external view + returns ( + address disputable, + uint256 disputableActionId, + uint256 collateralRequirementId, + uint256 settingId, + address submitter, + bool closed, + bytes context, + uint256 currentChallengeId + ); + + function getChallenge(uint256 _challengeId) external view + returns ( + uint256 actionId, + address challenger, + uint64 endDate, + bytes context, + uint256 settlementOffer, + uint256 arbitratorFeeAmount, + ERC20 arbitratorFeeToken, + ChallengeState state, + bool submitterFinishedEvidence, + bool challengerFinishedEvidence, + uint256 disputeId, + uint256 ruling + ); +} diff --git a/contracts/apps/disputable/IDisputable.sol b/contracts/apps/disputable/IDisputable.sol new file mode 100644 index 000000000..30e8ce783 --- /dev/null +++ b/contracts/apps/disputable/IDisputable.sol @@ -0,0 +1,42 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + +import "./IAgreement.sol"; +import "../../lib/token/ERC20.sol"; +import "../../lib/standards/ERC165.sol"; + + +contract IDisputable is ERC165 { + bytes4 internal constant ERC165_INTERFACE_ID = bytes4(0x01ffc9a7); + bytes4 internal constant DISPUTABLE_INTERFACE_ID = bytes4(0xef113021); + + event AgreementSet(IAgreement indexed agreement); + + function setAgreement(IAgreement _agreement) external; + + function onDisputableActionChallenged(uint256 _disputableActionId, uint256 _challengeId, address _challenger) external; + + function onDisputableActionAllowed(uint256 _disputableActionId) external; + + function onDisputableActionRejected(uint256 _disputableActionId) external; + + function onDisputableActionVoided(uint256 _disputableActionId) external; + + function getAgreement() external view returns (IAgreement); + + function canChallenge(uint256 _disputableActionId) external view returns (bool); + + function canClose(uint256 _disputableActionId) external view returns (bool); + + /** + * @dev Query if a contract implements a certain interface + * @param _interfaceId The interface identifier being queried, as specified in ERC-165 + * @return True if the contract implements the requested interface and if its not 0xffffffff, false otherwise + */ + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == DISPUTABLE_INTERFACE_ID || _interfaceId == ERC165_INTERFACE_ID; + } +} diff --git a/contracts/lib/arbitration/IArbitrable.sol b/contracts/lib/arbitration/IArbitrable.sol new file mode 100644 index 000000000..979f7d9a9 --- /dev/null +++ b/contracts/lib/arbitration/IArbitrable.sol @@ -0,0 +1,62 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + +import "./IArbitrator.sol"; +import "../../lib/standards/ERC165.sol"; + + +/** +* @title Arbitrable interface +* @dev This interface is the one extended by `IAgreement` so it can be used by `IArbitrator`, its dispute resolution protocol. +* This interface was manually-copied from https://github.com/aragon/aragon-court/blob/v1.1.3/contracts/arbitration/IArbitrable.sol +* since we are using different solidity versions. +*/ +contract IArbitrable is ERC165 { + bytes4 internal constant ERC165_INTERFACE_ID = bytes4(0x01ffc9a7); + bytes4 internal constant ARBITRABLE_INTERFACE_ID = bytes4(0x88f3ee69); + + /** + * @dev Emitted when an IArbitrable instance's dispute is ruled by an IArbitrator + * @param arbitrator IArbitrator instance ruling the dispute + * @param disputeId Identification number of the dispute being ruled by the arbitrator + * @param ruling Ruling given by the arbitrator + */ + event Ruled(IArbitrator indexed arbitrator, uint256 indexed disputeId, uint256 ruling); + + /** + * @dev Emitted when new evidence is submitted for the IArbitrable instance's dispute + * @param arbitrator IArbitrator submitting the evidence for + * @param disputeId Identification number of the dispute receiving new evidence + * @param submitter Address of the account submitting the evidence + * @param evidence Data submitted for the evidence of the dispute + * @param finished Whether or not the submitter has finished submitting evidence + */ + event EvidenceSubmitted(IArbitrator indexed arbitrator, uint256 indexed disputeId, address indexed submitter, bytes evidence, bool finished); + + /** + * @dev Submit evidence for a dispute + * @param _disputeId Id of the dispute in the Court + * @param _evidence Data submitted for the evidence related to the dispute + * @param _finished Whether or not the submitter has finished submitting evidence + */ + function submitEvidence(uint256 _disputeId, bytes _evidence, bool _finished) external; + + /** + * @dev Give a ruling for a certain dispute, the account calling it must have rights to rule on the contract + * @param _disputeId Identification number of the dispute to be ruled + * @param _ruling Ruling given by the arbitrator, where 0 is reserved for "refused to make a decision" + */ + function rule(uint256 _disputeId, uint256 _ruling) external; + + /** + * @dev ERC165 - Query if a contract implements a certain interface + * @param _interfaceId The interface identifier being queried, as specified in ERC-165 + * @return True if this contract supports the given interface, false otherwise + */ + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == ARBITRABLE_INTERFACE_ID || _interfaceId == ERC165_INTERFACE_ID; + } +} diff --git a/contracts/lib/arbitration/IArbitrator.sol b/contracts/lib/arbitration/IArbitrator.sol new file mode 100644 index 000000000..5cfa63a4b --- /dev/null +++ b/contracts/lib/arbitration/IArbitrator.sol @@ -0,0 +1,52 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + +import "../../lib/token/ERC20.sol"; + +/** +* @title Arbitrator interface +* @dev This interface is the one used by `IAgreement` as the dispute resolution protocol. +* This interface was manually-copied from https://github.com/aragon/aragon-court/blob/v1.1.3/contracts/arbitration/IArbitrator.sol +* since we are using different solidity versions. +*/ +interface IArbitrator { + /** + * @dev Create a dispute over the Arbitrable sender with a number of possible rulings + * @param _possibleRulings Number of possible rulings allowed for the dispute + * @param _metadata Optional metadata that can be used to provide additional information on the dispute to be created + * @return Dispute identification number + */ + function createDispute(uint256 _possibleRulings, bytes _metadata) external returns (uint256); + + /** + * @dev Close the evidence period of a dispute + * @param _disputeId Identification number of the dispute to close its evidence submitting period + */ + function closeEvidencePeriod(uint256 _disputeId) external; + + /** + * @dev Execute the Arbitrable associated to a dispute based on its final ruling + * @param _disputeId Identification number of the dispute to be executed + */ + function executeRuling(uint256 _disputeId) external; + + /** + * @dev Tell the dispute fees information to create a dispute + * @return recipient Address where the corresponding dispute fees must be transferred to + * @return feeToken ERC20 token used for the fees + * @return feeAmount Total amount of fees that must be allowed to the recipient + */ + function getDisputeFees() external view returns (address recipient, ERC20 feeToken, uint256 feeAmount); + + /** + * @dev Tell the subscription fees information for a subscriber to be up-to-date + * @param _subscriber Address of the account paying the subscription fees for + * @return recipient Address where the corresponding subscriptions fees must be transferred to + * @return feeToken ERC20 token used for the subscription fees + * @return feeAmount Total amount of fees that must be allowed to the recipient + */ + function getSubscriptionFees(address _subscriber) external view returns (address recipient, ERC20 feeToken, uint256 feeAmount); +} diff --git a/contracts/lib/standards/ERC165.sol b/contracts/lib/standards/ERC165.sol new file mode 100644 index 000000000..8fd948fc1 --- /dev/null +++ b/contracts/lib/standards/ERC165.sol @@ -0,0 +1,15 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + + +interface ERC165 { + /** + * @dev Query if a contract implements a certain interface + * @param _interfaceId The interface identifier being queried, as specified in ERC-165 + * @return True if the contract implements the requested interface and if its not 0xffffffff, false otherwise + */ + function supportsInterface(bytes4 _interfaceId) external pure returns (bool); +} diff --git a/contracts/test/mocks/apps/disputable/AgreementMock.sol b/contracts/test/mocks/apps/disputable/AgreementMock.sol new file mode 100644 index 000000000..8254e328f --- /dev/null +++ b/contracts/test/mocks/apps/disputable/AgreementMock.sol @@ -0,0 +1,13 @@ +pragma solidity 0.4.24; + + +contract AgreementMock { + function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external returns (uint256) { + // do nothing + return 0; + } + + function closeAction(uint256 _actionId) external { + // do nothing + } +} diff --git a/contracts/test/mocks/apps/disputable/DisputableAppMock.sol b/contracts/test/mocks/apps/disputable/DisputableAppMock.sol new file mode 100644 index 000000000..42b893a64 --- /dev/null +++ b/contracts/test/mocks/apps/disputable/DisputableAppMock.sol @@ -0,0 +1,82 @@ +pragma solidity 0.4.24; + +import "../../../../common/IForwarder.sol"; +import "../../../../apps/disputable/DisputableAragonApp.sol"; + + +contract DisputableAppMock is DisputableAragonApp { + bytes4 public constant ERC165_INTERFACE = ERC165_INTERFACE_ID; + bytes4 public constant DISPUTABLE_INTERFACE = DISPUTABLE_INTERFACE_ID; + + event DisputableChallenged(uint256 indexed id); + event DisputableAllowed(uint256 indexed id); + event DisputableRejected(uint256 indexed id); + event DisputableVoided(uint256 indexed id); + + function initialize() external { + initialized(); + } + + function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external { + _newAgreementAction(_disputableActionId, _context, _submitter); + } + + function closeAction(uint256 _actionId) external { + _closeAgreementAction(_actionId); + } + + function canChallenge(uint256 /*_disputableActionId*/) external view returns (bool) { + return true; + } + + function canClose(uint256 /*_disputableActionId*/) external view returns (bool) { + return true; + } + + function interfaceID() external pure returns (bytes4) { + IDisputable iDisputable; + return iDisputable.setAgreement.selector ^ + iDisputable.onDisputableActionChallenged.selector ^ + iDisputable.onDisputableActionAllowed.selector ^ + iDisputable.onDisputableActionRejected.selector ^ + iDisputable.onDisputableActionVoided.selector ^ + iDisputable.getAgreement.selector; + } + + function erc165interfaceID() external pure returns (bytes4) { + ERC165 erc165; + return erc165.supportsInterface.selector; + } + + /** + * @dev Challenge an entry + * @param _id Identification number of the entry to be challenged + */ + function _onDisputableActionChallenged(uint256 _id, uint256 /* _challengeId */, address /* _challenger */) internal { + emit DisputableChallenged(_id); + } + + /** + * @dev Allow an entry + * @param _id Identification number of the entry to be allowed + */ + function _onDisputableActionAllowed(uint256 _id) internal { + emit DisputableAllowed(_id); + } + + /** + * @dev Reject an entry + * @param _id Identification number of the entry to be rejected + */ + function _onDisputableActionRejected(uint256 _id) internal { + emit DisputableRejected(_id); + } + + /** + * @dev Void an entry + * @param _id Identification number of the entry to be voided + */ + function _onDisputableActionVoided(uint256 _id) internal { + emit DisputableVoided(_id); + } +} diff --git a/contracts/test/mocks/lib/arbitration/ArbitrableMock.sol b/contracts/test/mocks/lib/arbitration/ArbitrableMock.sol new file mode 100644 index 000000000..7217335ab --- /dev/null +++ b/contracts/test/mocks/lib/arbitration/ArbitrableMock.sol @@ -0,0 +1,30 @@ +pragma solidity ^0.4.24; + +import "../../../../lib/arbitration/IArbitrable.sol"; +import "../../../../lib/arbitration/IArbitrator.sol"; + + +contract ArbitrableMock is IArbitrable { + bytes4 public constant ERC165_INTERFACE = ERC165_INTERFACE_ID; + bytes4 public constant ARBITRABLE_INTERFACE = ARBITRABLE_INTERFACE_ID; + + IArbitrator internal arbitrator; + + function submitEvidence(uint256 /* _disputeId */, bytes /* _evidence */, bool /* _finished */) external { + // do nothing + } + + function rule(uint256 /* _disputeId */, uint256 /* _ruling */) external { + // do nothing + } + + function interfaceID() external pure returns (bytes4) { + IArbitrable iArbitrable; + return iArbitrable.submitEvidence.selector ^ iArbitrable.rule.selector; + } + + function erc165interfaceID() external pure returns (bytes4) { + ERC165 erc165; + return erc165.supportsInterface.selector; + } +} diff --git a/test/contracts/apps/disputable/disputable_app.js b/test/contracts/apps/disputable/disputable_app.js new file mode 100644 index 000000000..84f3f903e --- /dev/null +++ b/test/contracts/apps/disputable/disputable_app.js @@ -0,0 +1,309 @@ +const { assertRevert } = require('../../../helpers/assertThrow') +const { getEventArgument } = require('../../../helpers/events') +const { getNewProxyAddress } = require('../../../helpers/events') +const { assertEvent, assertAmountOfEvents } = require('../../../helpers/assertEvent')(web3) + +const ACL = artifacts.require('ACL') +const Kernel = artifacts.require('Kernel') +const DAOFactory = artifacts.require('DAOFactory') +const AgreementMock = artifacts.require('AgreementMock') +const DisputableApp = artifacts.require('DisputableAppMock') +const EVMScriptRegistryFactory = artifacts.require('EVMScriptRegistryFactory') + +contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) => { + let disputable, disputableBase, dao, acl + + const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000' + + before('deploy DAO', async () => { + const kernelBase = await Kernel.new(true) + const aclBase = await ACL.new() + const registryFactory = await EVMScriptRegistryFactory.new() + const daoFact = await DAOFactory.new(kernelBase.address, aclBase.address, registryFactory.address) + + const receipt = await daoFact.newDAO(owner) + dao = await Kernel.at(getEventArgument(receipt, 'DeployDAO', 'dao')) + acl = await ACL.at(await dao.acl()) + disputableBase = await DisputableApp.new() + + const APP_MANAGER_ROLE = await kernelBase.APP_MANAGER_ROLE() + await acl.createPermission(owner, dao.address, APP_MANAGER_ROLE, owner, { from: owner }) + }) + + beforeEach('install disputable app', async () => { + const initializeData = disputableBase.contract.initialize.getData() + const receipt = await dao.newAppInstance('0x1234', disputableBase.address, initializeData, false, { from: owner }) + disputable = await DisputableApp.at(getNewProxyAddress(receipt)) + + const SET_AGREEMENT_ROLE = await disputable.SET_AGREEMENT_ROLE() + await acl.createPermission(owner, disputable.address, SET_AGREEMENT_ROLE, owner, { from: owner }) + }) + + describe('supportsInterface', () => { + it('supports ERC165', async () => { + assert.isTrue(await disputable.supportsInterface('0x01ffc9a7'), 'does not support ERC165') + + assert.equal(await disputable.ERC165_INTERFACE(), '0x01ffc9a7', 'ERC165 interface ID does not match') + assert.equal(await disputable.erc165interfaceID(), await disputable.ERC165_INTERFACE(), 'ERC165 interface ID does not match') + }) + + it('supports IDisputable', async () => { + assert.isTrue(await disputable.supportsInterface('0xef113021'), 'does not support IDisputable') + + assert.equal(await disputable.interfaceID(), '0xef113021') + assert.equal(await disputable.DISPUTABLE_INTERFACE(), await disputable.interfaceID(), 'IDisputable interface ID does not match') + }) + + it('does not support 0xffffffff', async () => { + assert.isFalse(await disputable.supportsInterface('0xffffffff'), 'should not support 0xffffffff') + }) + }) + + describe('setAgreement', () => { + context('when the sender has permissions', () => { + const from = owner + + const itSetsTheAgreementAddress = agreement => { + it('sets the agreement', async () => { + await disputable.setAgreement(agreement, { from }) + + const currentAgreement = await disputable.getAgreement() + assert.equal(currentAgreement, agreement, 'disputable agreement does not match') + }) + + it('emits an event', async () => { + const receipt = await disputable.setAgreement(agreement, { from }) + + assertAmountOfEvents(receipt, 'AgreementSet') + assertEvent(receipt, 'AgreementSet', { agreement }) + }) + } + + context('when the agreement was not set', () => { + context('when trying to set a new the agreement', () => { + itSetsTheAgreementAddress(agreement) + }) + + context('when trying to unset the agreement', () => { + it('reverts', async () => { + await assertRevert(disputable.setAgreement(ZERO_ADDRESS, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL') + }) + }) + }) + + context('when the agreement was already set', () => { + beforeEach('set agreement', async () => { + await disputable.setAgreement(agreement, { from }) + }) + + context('when trying to re-set the agreement', () => { + it('reverts', async () => { + await assertRevert(disputable.setAgreement(agreement, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL') + }) + }) + + context('when trying to set a new agreement', () => { + it('reverts', async () => { + await assertRevert(disputable.setAgreement(anotherAgreement, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL') + }) + }) + + context('when trying to unset the agreement', () => { + it('reverts', async () => { + await assertRevert(disputable.setAgreement(ZERO_ADDRESS, { from }), 'DISPUTABLE_AGREEMENT_STATE_INVAL') + }) + }) + }) + }) + + context('when the sender does not have permissions', () => { + const from = someone + + it('reverts', async () => { + await assertRevert(disputable.setAgreement(agreement, { from }), 'APP_AUTH_FAILED') + }) + }) + }) + + describe('newAction', () => { + context('when the agreement is not set', () => { + it('reverts', async () => { + await assertRevert(disputable.newAction(0, '0x00', owner), 'DISPUTABLE_AGREEMENT_STATE_INVAL') + }) + }) + + context('when the agreement is set', () => { + beforeEach('set agreement', async () => { + const agreement = await AgreementMock.new() + await disputable.setAgreement(agreement.address, { from: owner }) + }) + + it('does not revert', async () => { + await disputable.newAction(0, '0x00', owner) + }) + }) + }) + + describe('closeAction', () => { + context('when the agreement is not set', () => { + it('reverts', async () => { + await assertRevert(disputable.closeAction(0), 'DISPUTABLE_AGREEMENT_STATE_INVAL') + }) + }) + + context('when the agreement is set', () => { + beforeEach('set agreement', async () => { + const agreement = await AgreementMock.new() + await disputable.setAgreement(agreement.address, { from: owner }) + }) + + it('does not revert', async () => { + await disputable.closeAction(0) + }) + }) + }) + + describe('onDisputableActionChallenged', () => { + const disputableId = 0, challengeId = 0, challenger = owner + + context('when the agreement was already set', () => { + const agreement = someone + + beforeEach('set agreement', async () => { + await disputable.setAgreement(agreement, { from: owner }) + }) + + context('when the sender is the agreement', () => { + const from = agreement + + it('does not fails', async () => { + const receipt = await disputable.onDisputableActionChallenged(disputableId, challengeId, challenger, { from }) + + assertAmountOfEvents(receipt, 'DisputableChallenged') + }) + }) + + context('when the sender is not the agreement', () => { + const from = owner + + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionChallenged(disputableId, challengeId, challenger, { from }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + context('when the agreement was not set', () => { + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionChallenged(disputableId, challengeId, challenger, { from: someone }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + describe('onDisputableActionAllowed', () => { + const disputableId = 0 + + context('when the agreement was already set', () => { + const agreement = someone + + beforeEach('set agreement', async () => { + await disputable.setAgreement(agreement, { from: owner }) + }) + + context('when the sender is the agreement', () => { + const from = agreement + + it('does not fails', async () => { + const receipt = await disputable.onDisputableActionAllowed(disputableId, { from }) + + assertAmountOfEvents(receipt, 'DisputableAllowed') + }) + }) + + context('when the sender is not the agreement', () => { + const from = owner + + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionAllowed(disputableId, { from }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + context('when the agreement was not set', () => { + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionAllowed(disputableId, { from: someone }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + describe('onDisputableActionRejected', () => { + const disputableId = 0 + + context('when the agreement was already set', () => { + const agreement = someone + + beforeEach('set agreement', async () => { + await disputable.setAgreement(agreement, { from: owner }) + }) + + context('when the sender is the agreement', () => { + const from = agreement + + it('does not fails', async () => { + const receipt = await disputable.onDisputableActionRejected(disputableId, { from }) + + assertAmountOfEvents(receipt, 'DisputableRejected') + }) + }) + + context('when the sender is not the agreement', () => { + const from = owner + + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionRejected(disputableId, { from }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + context('when the agreement was not set', () => { + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionRejected(disputableId, { from: someone }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + describe('onDisputableActionVoided', () => { + const disputableId = 0 + + context('when the agreement was already set', () => { + const agreement = someone + + beforeEach('set agreement', async () => { + await disputable.setAgreement(agreement, { from: owner }) + }) + + context('when the sender is the agreement', () => { + const from = agreement + + it('does not fails', async () => { + const receipt = await disputable.onDisputableActionVoided(disputableId, { from }) + + assertAmountOfEvents(receipt, 'DisputableVoided') + }) + }) + + context('when the sender is not the agreement', () => { + const from = owner + + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionVoided(disputableId, { from }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) + + context('when the agreement was not set', () => { + it('reverts', async () => { + await assertRevert(disputable.onDisputableActionVoided(disputableId, { from: someone }), 'DISPUTABLE_SENDER_NOT_AGREEMENT') + }) + }) + }) +}) diff --git a/test/contracts/lib/arbitration/arbitrable.js b/test/contracts/lib/arbitration/arbitrable.js new file mode 100644 index 000000000..04e035afa --- /dev/null +++ b/test/contracts/lib/arbitration/arbitrable.js @@ -0,0 +1,29 @@ +const Arbitrable = artifacts.require('ArbitrableMock') + +contract('Arbitrable', () => { + let arbitrable + + beforeEach('create arbitrable instance', async () => { + arbitrable = await Arbitrable.new() + }) + + describe('supportsInterface', () => { + it('supports ERC165', async () => { + assert.isTrue(await arbitrable.supportsInterface('0x01ffc9a7'), 'does not support ERC165') + + assert.equal(await arbitrable.ERC165_INTERFACE(), '0x01ffc9a7', 'ERC165 interface ID does not match') + assert.equal(await arbitrable.erc165interfaceID(), await arbitrable.ERC165_INTERFACE(), 'ERC165 interface ID does not match') + }) + + it('supports IArbitrable', async () => { + assert.isTrue(await arbitrable.supportsInterface('0x88f3ee69'), 'does not support IArbitrable') + + assert.equal(await arbitrable.ARBITRABLE_INTERFACE(), '0x88f3ee69', 'IArbitrable interface ID does not match') + assert.equal(await arbitrable.interfaceID(), await arbitrable.ARBITRABLE_INTERFACE(), 'IArbitrable interface ID does not match') + }) + + it('does not support 0xffffffff', async () => { + assert.isFalse(await arbitrable.supportsInterface('0xffffffff'), 'should not support 0xffffffff') + }) + }) +}) From 4b283dc9e75618930e45375d0cdefadc51a6c736 Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Fri, 26 Jun 2020 11:59:04 -0300 Subject: [PATCH 4/8] Chore: Bump v5.0.0-beta.0 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 5d778bc74..53449adaf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@aragon/os", - "version": "4.4.0", + "version": "5.0.0-beta.0", "description": "Core contracts for Aragon", "scripts": { "compile": "truffle compile", From e948841887571ea6468e1ab96678f4ae65ce5051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Tue, 30 Jun 2020 15:19:09 +0200 Subject: [PATCH 5/8] Disputable apps: add missing pieces for transaction fees module (#586) * Disputable apps: add missing pieces for transaction fees module * arbitration: Add IAragonCourtArbitrator * arbitration: Remove IAragonCourtArbitrator * 5.0.0-beta.1 * disputable apps: fixes after rebase on next * disputable apps: Add ITransactionFeesOracle to Agreement settings --- contracts/apps/disputable/IAgreement.sol | 9 ++++++++- contracts/apps/disputable/IDisputable.sol | 4 +++- .../lib/arbitration/ITransactionFeesOracle.sol | 12 ++++++++++++ .../mocks/apps/disputable/DisputableAppMock.sol | 13 ++++++++----- package.json | 2 +- test/contracts/apps/disputable/disputable_app.js | 10 ++++++---- 6 files changed, 38 insertions(+), 12 deletions(-) create mode 100644 contracts/lib/arbitration/ITransactionFeesOracle.sol diff --git a/contracts/apps/disputable/IAgreement.sol b/contracts/apps/disputable/IAgreement.sol index 333cb7a58..c91b56f63 100644 --- a/contracts/apps/disputable/IAgreement.sol +++ b/contracts/apps/disputable/IAgreement.sol @@ -7,6 +7,7 @@ pragma solidity ^0.4.24; import "../../acl/IACLOracle.sol"; import "../../lib/token/ERC20.sol"; import "../../lib/arbitration/IArbitrable.sol"; +import "../../lib/arbitration/ITransactionFeesOracle.sol"; contract IAgreement is IArbitrable, IACLOracle { @@ -61,7 +62,13 @@ contract IAgreement is IArbitrable, IACLOracle { function getCurrentSettingId() external view returns (uint256); - function getSetting(uint256 _settingId) external view returns (IArbitrator arbitrator, string title, bytes content); + function getSetting(uint256 _settingId) external view + returns ( + IArbitrator arbitrator, + ITransactionFeesOracle transactionFeesOracle, + string title, + bytes content + ); function getDisputableInfo(address _disputable) external view returns (bool registered, uint256 currentCollateralRequirementId); diff --git a/contracts/apps/disputable/IDisputable.sol b/contracts/apps/disputable/IDisputable.sol index 30e8ce783..f015feb67 100644 --- a/contracts/apps/disputable/IDisputable.sol +++ b/contracts/apps/disputable/IDisputable.sol @@ -11,7 +11,7 @@ import "../../lib/standards/ERC165.sol"; contract IDisputable is ERC165 { bytes4 internal constant ERC165_INTERFACE_ID = bytes4(0x01ffc9a7); - bytes4 internal constant DISPUTABLE_INTERFACE_ID = bytes4(0xef113021); + bytes4 internal constant DISPUTABLE_INTERFACE_ID = bytes4(0x737c65f9); event AgreementSet(IAgreement indexed agreement); @@ -39,4 +39,6 @@ contract IDisputable is ERC165 { function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { return _interfaceId == DISPUTABLE_INTERFACE_ID || _interfaceId == ERC165_INTERFACE_ID; } + + function appId() public view returns (bytes32); } diff --git a/contracts/lib/arbitration/ITransactionFeesOracle.sol b/contracts/lib/arbitration/ITransactionFeesOracle.sol new file mode 100644 index 000000000..df0cd3bbf --- /dev/null +++ b/contracts/lib/arbitration/ITransactionFeesOracle.sol @@ -0,0 +1,12 @@ +pragma solidity ^0.4.24; + +import "../token/ERC20.sol"; + + +interface ITransactionFeesOracle { + function setFee(bytes32 appId, ERC20 token, uint256 amount) external; + function setFees(bytes32[] _appIds, ERC20[] _tokens, uint256[] _amounts) external; + function unsetFee(bytes32 _appId) external; + function unsetFees(bytes32[] _appIds) external; + function getFee(bytes32 appId) external view returns (ERC20, uint256, address); +} diff --git a/contracts/test/mocks/apps/disputable/DisputableAppMock.sol b/contracts/test/mocks/apps/disputable/DisputableAppMock.sol index 42b893a64..4e8d10ba1 100644 --- a/contracts/test/mocks/apps/disputable/DisputableAppMock.sol +++ b/contracts/test/mocks/apps/disputable/DisputableAppMock.sol @@ -36,11 +36,14 @@ contract DisputableAppMock is DisputableAragonApp { function interfaceID() external pure returns (bytes4) { IDisputable iDisputable; return iDisputable.setAgreement.selector ^ - iDisputable.onDisputableActionChallenged.selector ^ - iDisputable.onDisputableActionAllowed.selector ^ - iDisputable.onDisputableActionRejected.selector ^ - iDisputable.onDisputableActionVoided.selector ^ - iDisputable.getAgreement.selector; + iDisputable.onDisputableActionChallenged.selector ^ + iDisputable.onDisputableActionAllowed.selector ^ + iDisputable.onDisputableActionRejected.selector ^ + iDisputable.onDisputableActionVoided.selector ^ + iDisputable.getAgreement.selector ^ + iDisputable.canChallenge.selector ^ + iDisputable.canClose.selector ^ + iDisputable.appId.selector; } function erc165interfaceID() external pure returns (bytes4) { diff --git a/package.json b/package.json index 53449adaf..4d6c9c091 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@aragon/os", - "version": "5.0.0-beta.0", + "version": "5.0.0-beta.1", "description": "Core contracts for Aragon", "scripts": { "compile": "truffle compile", diff --git a/test/contracts/apps/disputable/disputable_app.js b/test/contracts/apps/disputable/disputable_app.js index 84f3f903e..4f08896b9 100644 --- a/test/contracts/apps/disputable/disputable_app.js +++ b/test/contracts/apps/disputable/disputable_app.js @@ -14,6 +14,8 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) => let disputable, disputableBase, dao, acl const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000' + const DISPUTABLE_INTERFACE = '0x737c65f9' + const ERC165_INTERFACE = '0x01ffc9a7' before('deploy DAO', async () => { const kernelBase = await Kernel.new(true) @@ -41,16 +43,16 @@ contract('DisputableApp', ([_, owner, agreement, anotherAgreement, someone]) => describe('supportsInterface', () => { it('supports ERC165', async () => { - assert.isTrue(await disputable.supportsInterface('0x01ffc9a7'), 'does not support ERC165') + assert.isTrue(await disputable.supportsInterface(ERC165_INTERFACE), 'does not support ERC165') - assert.equal(await disputable.ERC165_INTERFACE(), '0x01ffc9a7', 'ERC165 interface ID does not match') + assert.equal(await disputable.ERC165_INTERFACE(), ERC165_INTERFACE, 'ERC165 interface ID does not match') assert.equal(await disputable.erc165interfaceID(), await disputable.ERC165_INTERFACE(), 'ERC165 interface ID does not match') }) it('supports IDisputable', async () => { - assert.isTrue(await disputable.supportsInterface('0xef113021'), 'does not support IDisputable') + assert.isTrue(await disputable.supportsInterface(DISPUTABLE_INTERFACE), 'does not support IDisputable') - assert.equal(await disputable.interfaceID(), '0xef113021') + assert.equal(await disputable.interfaceID(), DISPUTABLE_INTERFACE) assert.equal(await disputable.DISPUTABLE_INTERFACE(), await disputable.interfaceID(), 'IDisputable interface ID does not match') }) From d068879e59738bfbddadd099fb0d7fe72915367a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Fingen?= Date: Wed, 1 Jul 2020 23:03:43 +0200 Subject: [PATCH 6/8] disputable apps: ITransactionFeesOracle enhancements (#588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * disputable apps: ITransactionFeesOracle enhancements - Add payment endpoint - Rename methods to be “transaction specific” - Add events * disputable apps: Remove benficiary from transaction fees oracle * disputable: Rename ITransactionFeesOracle to AragonAppFeesCashier * disputable apps: change payAppFees param --- contracts/apps/disputable/IAgreement.sol | 4 ++-- .../lib/arbitration/IAragonAppFeesCashier.sol | 17 +++++++++++++++++ .../lib/arbitration/ITransactionFeesOracle.sol | 12 ------------ 3 files changed, 19 insertions(+), 14 deletions(-) create mode 100644 contracts/lib/arbitration/IAragonAppFeesCashier.sol delete mode 100644 contracts/lib/arbitration/ITransactionFeesOracle.sol diff --git a/contracts/apps/disputable/IAgreement.sol b/contracts/apps/disputable/IAgreement.sol index c91b56f63..0fc264d6d 100644 --- a/contracts/apps/disputable/IAgreement.sol +++ b/contracts/apps/disputable/IAgreement.sol @@ -7,7 +7,7 @@ pragma solidity ^0.4.24; import "../../acl/IACLOracle.sol"; import "../../lib/token/ERC20.sol"; import "../../lib/arbitration/IArbitrable.sol"; -import "../../lib/arbitration/ITransactionFeesOracle.sol"; +import "../../lib/arbitration/IAragonAppFeesCashier.sol"; contract IAgreement is IArbitrable, IACLOracle { @@ -65,7 +65,7 @@ contract IAgreement is IArbitrable, IACLOracle { function getSetting(uint256 _settingId) external view returns ( IArbitrator arbitrator, - ITransactionFeesOracle transactionFeesOracle, + IAragonAppFeesCashier aragonAppFeesCashier, string title, bytes content ); diff --git a/contracts/lib/arbitration/IAragonAppFeesCashier.sol b/contracts/lib/arbitration/IAragonAppFeesCashier.sol new file mode 100644 index 000000000..adbe696c5 --- /dev/null +++ b/contracts/lib/arbitration/IAragonAppFeesCashier.sol @@ -0,0 +1,17 @@ +pragma solidity ^0.4.24; + +import "../token/ERC20.sol"; + + +interface IAragonAppFeesCashier { + event AppFeeSet(bytes32 indexed appId, ERC20 token, uint256 amount); + event AppFeeUnset(bytes32 indexed appId); + event AppFeePaid(address indexed by, bytes32 appId, bytes data); + + function setAppFee(bytes32 _appId, ERC20 _token, uint256 _amount) external; + function setAppFees(bytes32[] _appIds, ERC20[] _tokens, uint256[] _amounts) external; + function unsetAppFee(bytes32 _appId) external; + function unsetAppFees(bytes32[] _appIds) external; + function payAppFees(bytes32 _appId, bytes _data) external; + function getAppFee(bytes32 _appId) external view returns (ERC20, uint256); +} diff --git a/contracts/lib/arbitration/ITransactionFeesOracle.sol b/contracts/lib/arbitration/ITransactionFeesOracle.sol deleted file mode 100644 index df0cd3bbf..000000000 --- a/contracts/lib/arbitration/ITransactionFeesOracle.sol +++ /dev/null @@ -1,12 +0,0 @@ -pragma solidity ^0.4.24; - -import "../token/ERC20.sol"; - - -interface ITransactionFeesOracle { - function setFee(bytes32 appId, ERC20 token, uint256 amount) external; - function setFees(bytes32[] _appIds, ERC20[] _tokens, uint256[] _amounts) external; - function unsetFee(bytes32 _appId) external; - function unsetFees(bytes32[] _appIds) external; - function getFee(bytes32 appId) external view returns (ERC20, uint256, address); -} From a70d806c74387e4c479cb4effd40d0d0e85685d0 Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Wed, 1 Jul 2020 20:15:13 -0300 Subject: [PATCH 7/8] ACL: Multiple enhancements (#584) * acl: optimize internal calls * acl: allow knowing original user when set to any * acl: move grant permission modifier to internal method * ACL: Apply suggestions from @sohkai Co-authored-by: Brett Sun * acl: move grant permission modifier to external methods * acl: clarify ACL oracle interface * acl: improve internal logic eval functions naming * acl: inline internal oracle helper function * acl: add more test cases for oracles * acl: add previous acl oracle interface * ACL: update complex ACLOracle tests to be separate tests Co-authored-by: Brett Sun --- contracts/acl/ACL.sol | 260 +++++++++++++------- contracts/acl/IACL.sol | 4 +- contracts/acl/IACLOracle.sol | 16 +- contracts/acl/IACLOracleV1.sol | 15 ++ contracts/test/helpers/ACLHelper.sol | 28 ++- contracts/test/tests/TestACLInterpreter.sol | 72 +++++- test/contracts/acl/acl_params.js | 65 +++-- 7 files changed, 317 insertions(+), 143 deletions(-) create mode 100644 contracts/acl/IACLOracleV1.sol diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index 841ee1c0f..b1ade009a 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -57,13 +57,13 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { event ChangePermissionManager(address indexed app, bytes32 indexed role, address indexed manager); modifier onlyPermissionManager(address _app, bytes32 _role) { - require(msg.sender == getPermissionManager(_app, _role), ERROR_AUTH_NO_MANAGER); + require(msg.sender == _getPermissionManager(_app, _role), ERROR_AUTH_NO_MANAGER); _; } modifier noPermissionManager(address _app, bytes32 _role) { // only allow permission creation (or re-creation) when there is no manager - require(getPermissionManager(_app, _role) == address(0), ERROR_EXISTENT_MANAGER); + require(_getPermissionManager(_app, _role) == address(0), ERROR_EXISTENT_MANAGER); _; } @@ -107,10 +107,8 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { * @param _app Address of the app in which the role will be allowed (requires app to depend on kernel for ACL) * @param _role Identifier for the group of actions in app given access to perform */ - function grantPermission(address _entity, address _app, bytes32 _role) - external - { - grantPermissionP(_entity, _app, _role, new uint256[](0)); + function grantPermission(address _entity, address _app, bytes32 _role) external onlyPermissionManager(_app, _role) { + _grantPermissionP(_entity, _app, _role, new uint256[](0)); } /** @@ -121,12 +119,8 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { * @param _role Identifier for the group of actions in app given access to perform * @param _params Permission parameters */ - function grantPermissionP(address _entity, address _app, bytes32 _role, uint256[] _params) - public - onlyPermissionManager(_app, _role) - { - bytes32 paramsHash = _params.length > 0 ? _saveParams(_params) : EMPTY_PARAM_HASH; - _setPermission(_entity, _app, _role, paramsHash); + function grantPermissionP(address _entity, address _app, bytes32 _role, uint256[] _params) external onlyPermissionManager(_app, _role) { + _grantPermissionP(_entity, _app, _role, _params); } /** @@ -201,7 +195,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { * @return Length of the array */ function getPermissionParamsLength(address _entity, address _app, bytes32 _role) external view returns (uint) { - return permissionParams[permissions[permissionHash(_entity, _app, _role)]].length; + return permissionParams[permissions[_permissionHash(_entity, _app, _role)]].length; } /** @@ -217,7 +211,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { view returns (uint8, uint8, uint240) { - Param storage param = permissionParams[permissions[permissionHash(_entity, _app, _role)]][_index]; + Param storage param = permissionParams[permissions[_permissionHash(_entity, _app, _role)]][_index]; return (param.id, param.op, param.value); } @@ -227,8 +221,8 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { * @param _role Identifier for a group of actions in app * @return address of the manager for the permission */ - function getPermissionManager(address _app, bytes32 _role) public view returns (address) { - return permissionManager[roleHash(_app, _role)]; + function getPermissionManager(address _app, bytes32 _role) external view returns (address) { + return _getPermissionManager(_app, _role); } /** @@ -239,45 +233,50 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { * @param _how Permission parameters * @return boolean indicating whether the ACL allows the role or not */ - function hasPermission(address _who, address _where, bytes32 _what, bytes memory _how) public view returns (bool) { - return hasPermission(_who, _where, _what, ConversionHelpers.dangerouslyCastBytesToUintArray(_how)); + function hasPermission(address _who, address _where, bytes32 _what, bytes _how) external view returns (bool) { + return _hasPermission(_who, _where, _what, ConversionHelpers.dangerouslyCastBytesToUintArray(_how)); } - function hasPermission(address _who, address _where, bytes32 _what, uint256[] memory _how) public view returns (bool) { - bytes32 whoParams = permissions[permissionHash(_who, _where, _what)]; - if (whoParams != NO_PERMISSION && evalParams(whoParams, _who, _where, _what, _how)) { - return true; - } - - bytes32 anyParams = permissions[permissionHash(ANY_ENTITY, _where, _what)]; - if (anyParams != NO_PERMISSION && evalParams(anyParams, ANY_ENTITY, _where, _what, _how)) { - return true; - } - - return false; + /** + * @dev Function called by apps to check ACL on kernel or to check permission status + * @param _who Sender of the original call + * @param _where Address of the app + * @param _what Identifier for a group of actions in app (role) + * @param _how Permission parameters + * @return boolean indicating whether the ACL allows the role or not + */ + function hasPermission(address _who, address _where, bytes32 _what, uint256[] _how) external view returns (bool) { + return _hasPermission(_who, _where, _what, _how); } - function hasPermission(address _who, address _where, bytes32 _what) public view returns (bool) { - uint256[] memory empty = new uint256[](0); - return hasPermission(_who, _where, _what, empty); + /** + * @dev Function called by apps to check ACL on kernel or to check permission status + * @param _who Sender of the original call + * @param _where Address of the app + * @param _what Identifier for a group of actions in app (role) + * @return boolean indicating whether the ACL allows the role or not + */ + function hasPermission(address _who, address _where, bytes32 _what) external view returns (bool) { + uint256[] memory emptyParams = new uint256[](0); + return _hasPermission(_who, _where, _what, emptyParams); } - function evalParams( - bytes32 _paramsHash, - address _who, - address _where, - bytes32 _what, - uint256[] _how - ) - public + /** + * @dev Function called by apps to evaluate ACL params + * @param _paramsHash Params hash identifier + * @param _user Sender of the original call + * @param _who Grantee of the role + * @param _where Address of the app + * @param _what Identifier for a group of actions in app (role) + * @param _how Permission parameters + * @return boolean indicating whether the ACL allows the role or not + */ + function evalParams(bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + external view returns (bool) { - if (_paramsHash == EMPTY_PARAM_HASH) { - return true; - } - - return _evalParam(_paramsHash, 0, _who, _where, _what, _how); + return _evalParams(_paramsHash, _user, _who, _where, _what, _how); } /** @@ -288,11 +287,19 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { _setPermissionManager(_manager, _app, _role); } + /** + * @dev Internal function to grant a permission with parameters + */ + function _grantPermissionP(address _entity, address _app, bytes32 _role, uint256[] _params) internal { + bytes32 paramsHash = _params.length > 0 ? _saveParams(_params) : EMPTY_PARAM_HASH; + _setPermission(_entity, _app, _role, paramsHash); + } + /** * @dev Internal function called to actually save the permission */ function _setPermission(address _entity, address _app, bytes32 _role, bytes32 _paramsHash) internal { - permissions[permissionHash(_entity, _app, _role)] = _paramsHash; + permissions[_permissionHash(_entity, _app, _role)] = _paramsHash; bool entityHasPermission = _paramsHash != NO_PERMISSION; bool permissionHasParams = entityHasPermission && _paramsHash != EMPTY_PARAM_HASH; @@ -302,6 +309,17 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { } } + /** + * @dev Internal function that sets management of a role + */ + function _setPermissionManager(address _newManager, address _app, bytes32 _role) internal { + permissionManager[_roleHash(_app, _role)] = _newManager; + emit ChangePermissionManager(_app, _role, _newManager); + } + + /** + * @dev Internal function to save encoded params + */ function _saveParams(uint256[] _encodedParams) internal returns (bytes32) { bytes32 paramHash = keccak256(abi.encodePacked(_encodedParams)); Param[] storage params = permissionParams[paramHash]; @@ -317,14 +335,50 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { return paramHash; } - function _evalParam( - bytes32 _paramsHash, - uint32 _paramId, - address _who, - address _where, - bytes32 _what, - uint256[] _how - ) + /** + * @dev Internal function to get manager for permission + */ + function _getPermissionManager(address _app, bytes32 _role) internal view returns (address) { + return permissionManager[_roleHash(_app, _role)]; + } + + /** + * @dev Internal function to perform ACL checks + */ + function _hasPermission(address _who, address _where, bytes32 _what, uint256[] memory _how) internal view returns (bool) { + bytes32 whoParams = permissions[_permissionHash(_who, _where, _what)]; + if (whoParams != NO_PERMISSION && _evalParams(whoParams, _who, _who, _where, _what, _how)) { + return true; + } + + bytes32 anyParams = permissions[_permissionHash(ANY_ENTITY, _where, _what)]; + if (anyParams != NO_PERMISSION && _evalParams(anyParams, _who, ANY_ENTITY, _where, _what, _how)) { + return true; + } + + return false; + } + + /** + * @dev Internal function to perform an ACL check + */ + function _evalParams(bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + internal + view + returns (bool) + { + if (_paramsHash == EMPTY_PARAM_HASH) { + return true; + } + + // `_evalParam()` will internally traverse all the parameters, starting from the first parameter (0) + return _evalParam(_paramsHash, 0, _user, _who, _where, _what, _how); + } + + /** + * @dev Internal function to perform an ACL check on a single permission parameter + */ + function _evalParam(bytes32 _paramsHash, uint32 _paramId, address _user, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) @@ -336,7 +390,9 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { Param memory param = permissionParams[_paramsHash][_paramId]; if (param.id == LOGIC_OP_PARAM_ID) { - return _evalLogic(param, _paramsHash, _who, _where, _what, _how); + return (Op(param.op) == Op.IF_ELSE) + ? _evalIfElseLogicOp(param, _paramsHash, _user, _who, _where, _what, _how) + : _evalNonIfElseLogicOp(param, _paramsHash, _user, _who, _where, _what, _how); } uint256 value; @@ -344,7 +400,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { // get value if (param.id == ORACLE_PARAM_ID) { - value = checkOracle(IACLOracle(param.value), _who, _where, _what, _how) ? 1 : 0; + value = _checkOracle(IACLOracle(param.value), _user, _who, _where, _what, _how) ? 1 : 0; comparedTo = 1; } else if (param.id == BLOCK_NUMBER_PARAM_ID) { value = getBlockNumber(); @@ -363,30 +419,37 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { return uint256(value) > 0; } - return compare(value, Op(param.op), comparedTo); + return _compare(value, Op(param.op), comparedTo); } - function _evalLogic(Param _param, bytes32 _paramsHash, address _who, address _where, bytes32 _what, uint256[] _how) + /** + * @dev Internal function to eval an IF-ELSE logic operator + */ + function _evalIfElseLogicOp(Param _param, bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) { - if (Op(_param.op) == Op.IF_ELSE) { - uint32 conditionParam; - uint32 successParam; - uint32 failureParam; + uint32 conditionParam; + uint32 successParam; + uint32 failureParam; - (conditionParam, successParam, failureParam) = decodeParamsList(uint256(_param.value)); - bool result = _evalParam(_paramsHash, conditionParam, _who, _where, _what, _how); + (conditionParam, successParam, failureParam) = decodeParamsList(uint256(_param.value)); + bool result = _evalParam(_paramsHash, conditionParam, _user, _who, _where, _what, _how); - return _evalParam(_paramsHash, result ? successParam : failureParam, _who, _where, _what, _how); - } - - uint32 param1; - uint32 param2; + return _evalParam(_paramsHash, result ? successParam : failureParam, _user, _who, _where, _what, _how); + } - (param1, param2,) = decodeParamsList(uint256(_param.value)); - bool r1 = _evalParam(_paramsHash, param1, _who, _where, _what, _how); + /** + * @dev Internal function to eval a non-IF-ELSE logic operator + */ + function _evalNonIfElseLogicOp(Param _param, bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + internal + view + returns (bool) + { + (uint32 param1, uint32 param2,) = decodeParamsList(uint256(_param.value)); + bool r1 = _evalParam(_paramsHash, param1, _user, _who, _where, _what, _how); if (Op(_param.op) == Op.NOT) { return !r1; @@ -400,7 +463,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { return false; } - bool r2 = _evalParam(_paramsHash, param2, _who, _where, _what, _how); + bool r2 = _evalParam(_paramsHash, param2, _user, _who, _where, _what, _how); if (Op(_param.op) == Op.XOR) { return r1 != r2; @@ -409,21 +472,17 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { return r2; // both or and and depend on result of r2 after checks } - function compare(uint256 _a, Op _op, uint256 _b) internal pure returns (bool) { - if (_op == Op.EQ) return _a == _b; // solium-disable-line lbrace - if (_op == Op.NEQ) return _a != _b; // solium-disable-line lbrace - if (_op == Op.GT) return _a > _b; // solium-disable-line lbrace - if (_op == Op.LT) return _a < _b; // solium-disable-line lbrace - if (_op == Op.GTE) return _a >= _b; // solium-disable-line lbrace - if (_op == Op.LTE) return _a <= _b; // solium-disable-line lbrace - return false; - } - - function checkOracle(IACLOracle _oracleAddr, address _who, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) { - bytes4 sig = _oracleAddr.canPerform.selector; - + /** + * @dev Internal function to perform an ACL oracle check + */ + function _checkOracle(IACLOracle _oracleAddr, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + internal + view + returns (bool) + { // a raw call is required so we can return false if the call reverts, rather than reverting - bytes memory checkCalldata = abi.encodeWithSelector(sig, _who, _where, _what, _how); + bytes4 sig = _oracleAddr.canPerform.selector; + bytes memory checkCalldata = abi.encodeWithSelector(sig, _user, _who, _where, _what, _how); bool ok; assembly { @@ -455,18 +514,29 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { } /** - * @dev Internal function that sets management + * @dev Internal function to hash a role */ - function _setPermissionManager(address _newManager, address _app, bytes32 _role) internal { - permissionManager[roleHash(_app, _role)] = _newManager; - emit ChangePermissionManager(_app, _role, _newManager); - } - - function roleHash(address _where, bytes32 _what) internal pure returns (bytes32) { + function _roleHash(address _where, bytes32 _what) internal pure returns (bytes32) { return keccak256(abi.encodePacked("ROLE", _where, _what)); } - function permissionHash(address _who, address _where, bytes32 _what) internal pure returns (bytes32) { + /** + * @dev Internal function to hash a permission + */ + function _permissionHash(address _who, address _where, bytes32 _what) internal pure returns (bytes32) { return keccak256(abi.encodePacked("PERMISSION", _who, _where, _what)); } + + /** + * @dev Internal function to eval an ACL operator + */ + function _compare(uint256 _a, Op _op, uint256 _b) internal pure returns (bool) { + if (_op == Op.EQ) return _a == _b; // solium-disable-line lbrace + if (_op == Op.NEQ) return _a != _b; // solium-disable-line lbrace + if (_op == Op.GT) return _a > _b; // solium-disable-line lbrace + if (_op == Op.LT) return _a < _b; // solium-disable-line lbrace + if (_op == Op.GTE) return _a >= _b; // solium-disable-line lbrace + if (_op == Op.LTE) return _a <= _b; // solium-disable-line lbrace + return false; + } } diff --git a/contracts/acl/IACL.sol b/contracts/acl/IACL.sol index 3a304bfe3..57a8668ad 100644 --- a/contracts/acl/IACL.sol +++ b/contracts/acl/IACL.sol @@ -8,7 +8,5 @@ pragma solidity ^0.4.24; interface IACL { function initialize(address permissionsCreator) external; - // TODO: this should be external - // See https://github.com/ethereum/solidity/issues/4832 - function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool); + function hasPermission(address who, address where, bytes32 what, bytes how) external view returns (bool); } diff --git a/contracts/acl/IACLOracle.sol b/contracts/acl/IACLOracle.sol index e837d3a36..98065e8cc 100644 --- a/contracts/acl/IACLOracle.sol +++ b/contracts/acl/IACLOracle.sol @@ -5,6 +5,20 @@ pragma solidity ^0.4.24; +/** +* @title ACL Oracle interface +* @dev This interface simply defines a check method that must be implemented by smart contracts to be plugged in as ACL oracles. +* ACL oracles are the most suitable way to have external contracts validating ACL permissions with custom logic. +*/ interface IACLOracle { - function canPerform(address who, address where, bytes32 what, uint256[] how) external view returns (bool); + /** + * @dev Tells whether `user` can execute `what`(`how`) in `where` if it's currently set up for `who` + * @param user Entity trying to execute `what` in `where` + * @param who Entity to which `what` is granted based on the current ACL permissions configuration + * @param where Entity where `what` is trying to be executed + * @param what Identifier of the action willing to be executed in `where` + * @param how Can be used to define a set of arguments to give more context about `what` is trying to be executed in `where` + * @return True if the user is allowed to execute the requested action for the given context, false otherwise + */ + function canPerform(address user, address who, address where, bytes32 what, uint256[] how) external view returns (bool); } diff --git a/contracts/acl/IACLOracleV1.sol b/contracts/acl/IACLOracleV1.sol new file mode 100644 index 000000000..c54c4eb78 --- /dev/null +++ b/contracts/acl/IACLOracleV1.sol @@ -0,0 +1,15 @@ +/* + * SPDX-License-Identifier: MIT + */ + +pragma solidity ^0.4.24; + + +/** +* @title Previous version of the ACL Oracle interface +* @dev This interface simply defines a check method that must be implemented by smart contracts to be plugged in as ACL oracles. +* ACL oracles are the most suitable way to have external contracts validating ACL permissions with custom logic. +*/ +interface IACLOracleV1 { + function canPerform(address who, address where, bytes32 what, uint256[] how) external view returns (bool); +} diff --git a/contracts/test/helpers/ACLHelper.sol b/contracts/test/helpers/ACLHelper.sol index 265d6a84d..87ba4b57f 100644 --- a/contracts/test/helpers/ACLHelper.sol +++ b/contracts/test/helpers/ACLHelper.sol @@ -15,27 +15,27 @@ contract ACLHelper { contract AcceptOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) { return true; } } contract RejectOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) { return false; } } contract RevertOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) { revert(); } } contract AssertOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) { assert(false); } } @@ -44,14 +44,14 @@ contract AssertOracle is IACLOracle { contract StateModifyingOracle /* is IACLOracle */ { bool modifyState; - function canPerform(address, address, bytes32, uint256[]) external returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external returns (bool) { modifyState = true; return true; } } contract EmptyDataReturnOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) { assembly { return(0, 0) } @@ -59,13 +59,13 @@ contract EmptyDataReturnOracle is IACLOracle { } contract ConditionalOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[] how) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[] how) external view returns (bool) { return how[0] > 0; } } contract OverGasLimitOracle is IACLOracle { - function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { + function canPerform(address, address, address, bytes32, uint256[]) external view returns (bool) { while (true) { // Do an SLOAD to increase the per-loop gas costs uint256 loadFromStorage; @@ -74,3 +74,15 @@ contract OverGasLimitOracle is IACLOracle { return true; } } + +contract OnlyOwnerOracle is IACLOracle { + address owner; + + constructor (address _owner) public { + owner = _owner; + } + + function canPerform(address user, address, address, bytes32, uint256[]) external view returns (bool) { + return user == owner; + } +} diff --git a/contracts/test/tests/TestACLInterpreter.sol b/contracts/test/tests/TestACLInterpreter.sol index 05bb9275c..b09365291 100644 --- a/contracts/test/tests/TestACLInterpreter.sol +++ b/contracts/test/tests/TestACLInterpreter.sol @@ -39,7 +39,7 @@ contract TestACLInterpreter is ACL, ACLHelper { assertEval(arr(msg.sender), 0, Op.NEQ, uint256(this), true); } - function testGreatherThan() public { + function testGreaterThan() public { assertEval(arr(uint256(10), 11), 0, Op.GT, 9, true); assertEval(arr(uint256(10), 11), 0, Op.GT, 10, false); assertEval(arr(uint256(10), 11), 1, Op.GT, 10, true); @@ -51,7 +51,7 @@ contract TestACLInterpreter is ACL, ACLHelper { assertEval(arr(uint256(10), 11), 1, Op.LT, 10, false); } - function testGreatherThanOrEqual() public { + function testGreaterThanOrEqual() public { assertEval(arr(uint256(10), 11), 0, Op.GTE, 9, true); assertEval(arr(uint256(10), 11), 0, Op.GTE, 10, true); assertEval(arr(uint256(10), 11), 1, Op.GTE, 12, false); @@ -87,9 +87,13 @@ contract TestACLInterpreter is ACL, ACLHelper { // conditional oracle returns true if first param > 0 ConditionalOracle conditionalOracle = new ConditionalOracle(); - assertEval(arr(uint256(1)), ORACLE_PARAM_ID, Op.EQ, uint256(conditionalOracle), true); assertEval(arr(uint256(0), uint256(1)), ORACLE_PARAM_ID, Op.EQ, uint256(conditionalOracle), false); + + // only owner oracle returns true if sender is the defined owner + OnlyOwnerOracle onlyOwnerOracle = new OnlyOwnerOracle(msg.sender); + assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(onlyOwnerOracle), msg.sender, ANY_ENTITY, true); + assertEval(arr(), ORACLE_PARAM_ID, Op.EQ, uint256(onlyOwnerOracle), address(this), ANY_ENTITY, false); } function testReturn() public { @@ -127,10 +131,51 @@ contract TestACLInterpreter is ACL, ACLHelper { params[5] = Param(0, uint8(Op.LT), uint240(10)); params[6] = Param(PARAM_VALUE_PARAM_ID, uint8(Op.RET), 0); + // must return true for arg = 10 assertEval(params, arr(uint256(10)), true); + } + function testAnotherComplexCombination() public { + // if (oracle and block number > block number - 1) then arg 0 < 10 and oracle else false + Param[] memory params = new Param[](7); + params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 4, 6)); + params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); + params[2] = Param(ORACLE_PARAM_ID, uint8(Op.EQ), uint240(new AcceptOracle())); + params[3] = Param(BLOCK_NUMBER_PARAM_ID, uint8(Op.GT), uint240(block.number - 1)); params[4] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(5, 2)); + params[5] = Param(0, uint8(Op.LT), uint240(10)); + params[6] = Param(PARAM_VALUE_PARAM_ID, uint8(Op.RET), 0); + + // Requires 0 < 10 AND oracle, so 10 should fail and 9 should pass assertEval(params, arr(uint256(10)), false); + assertEval(params, arr(uint256(9)), true); + } + + function testComplexCombinationWithOnlyOwnerOracle () public { + // Oracle only returns true for msg.sender + OnlyOwnerOracle onlyOwnerOracle = new OnlyOwnerOracle(msg.sender); + + // if (oracle and block number > block number - 1) then arg 0 < 10 and oracle else false + Param[] memory params = new Param[](7); + params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 4, 6)); + params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); + params[2] = Param(ORACLE_PARAM_ID, uint8(Op.EQ), uint240(onlyOwnerOracle)); + params[3] = Param(BLOCK_NUMBER_PARAM_ID, uint8(Op.GT), uint240(block.number - 1)); + params[4] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(5, 2)); + params[5] = Param(0, uint8(Op.LT), uint240(10)); + params[6] = Param(PARAM_VALUE_PARAM_ID, uint8(Op.RET), 0); + + // Requires 0 < 10 AND oracle, so 10 should always fail + assertEval(params, arr(uint256(10)), address(this), ANY_ENTITY, false); + assertEval(params, arr(uint256(10)), msg.sender, ANY_ENTITY, false); + assertEval(params, arr(uint256(10)), address(this), address(0), false); + assertEval(params, arr(uint256(10)), msg.sender, address(0), false); + + // 9 only passes if the caller is msg.sender + assertEval(params, arr(uint256(9)), address(this), ANY_ENTITY, false); + assertEval(params, arr(uint256(9)), msg.sender, ANY_ENTITY, true); + assertEval(params, arr(uint256(9)), address(this), address(0), false); + assertEval(params, arr(uint256(9)), msg.sender, address(0), true); } function testParamOutOfBoundsFail() public { @@ -233,26 +278,33 @@ contract TestACLInterpreter is ACL, ACLHelper { assertEval(params, false); } - - function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, bool expected) internal { + function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, address user, address who, bool expected) internal { Param[] memory params = new Param[](1); params[0] = Param(argId, uint8(op), uint240(value)); - assertEval(params, args, expected); + assertEval(params, args, user, who, expected); + } + + function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, bool expected) internal { + assertEval(args, argId, op, value, address(0), address(0), expected); } function assertEval(Param[] memory params, bool expected) internal { - assertEval(params, new uint256[](0), expected); + assertEval(params, new uint256[](0), address(0), address(0), expected); } function assertEval(Param[] memory params, uint256[] memory args, bool expected) internal { - bytes32 paramHash = encodeAndSaveParams(params); - bool allow = _evalParam(paramHash, 0, address(0), address(0), bytes32(0), args); + assertEval(params, args, address(0), address(0), expected); + } + + function assertEval(Param[] memory params, uint256[] memory args, address user, address who, bool expected) internal { + bytes32 paramHash = _encodeAndSaveParams(params); + bool allow = _evalParam(paramHash, 0, user, who, address(0), bytes32(0), args); Assert.equal(allow, expected, "eval got unexpected result"); } event LogParam(bytes32 param); - function encodeAndSaveParams(Param[] memory params) internal returns (bytes32) { + function _encodeAndSaveParams(Param[] memory params) internal returns (bytes32) { uint256[] memory encodedParams = new uint256[](params.length); for (uint256 i = 0; i < params.length; i++) { diff --git a/test/contracts/acl/acl_params.js b/test/contracts/acl/acl_params.js index f9d1cc96b..d880cdea5 100644 --- a/test/contracts/acl/acl_params.js +++ b/test/contracts/acl/acl_params.js @@ -10,6 +10,7 @@ const AcceptOracle = artifacts.require('AcceptOracle') const RejectOracle = artifacts.require('RejectOracle') const RevertOracle = artifacts.require('RevertOracle') const AssertOracle = artifacts.require('AssertOracle') +const OnlyOwnerOracle = artifacts.require('OnlyOwnerOracle') const OverGasLimitOracle = artifacts.require('OverGasLimitOracle') const StateModifyingOracle = artifacts.require('StateModifyingOracle') @@ -20,7 +21,7 @@ const getExpectedGas = gas => gas * 63 / 64 contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppAddress]) => { let aclBase, kernelBase, acl, kernel - const MOCK_APP_ROLE = "0xAB" + const MOCK_APP_ROLE = '0xAB' before(async () => { kernelBase = await Kernel.new(true) // petrify immediately @@ -38,23 +39,26 @@ contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppA context('> ACL Oracle', () => { let aclParams - const testOraclePermissions = ({ shouldHavePermission }) => { - const allowText = shouldHavePermission ? 'allows' : 'disallows' - + const testOraclePermissions = ({ allowsAnyAddress, allowsSpecificAddress }) => { describe('when permission is set for ANY_ADDR', () => { beforeEach(async () => { await acl.grantPermissionP(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE, [aclParams]) }) - it(`ACL ${allowText} actions for ANY_ADDR`, async () => { - assertion = shouldHavePermission ? assert.isTrue : assert.isFalse + it(`ACL ${allowsAnyAddress ? 'allows' : 'disallows'} actions for ANY_ADDR`, async () => { + const assertion = allowsAnyAddress ? assert.isTrue : assert.isFalse assertion(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE)) }) - it(`ACL ${allowText} actions for specific address`, async () => { - assertion = shouldHavePermission ? assert.isTrue : assert.isFalse + it(`ACL ${allowsSpecificAddress ? 'allows' : 'disallows'} actions for specific address`, async () => { + const assertion = allowsSpecificAddress ? assert.isTrue : assert.isFalse assertion(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE)) }) + + it(`ACL ${allowsAnyAddress ? 'allows' : 'disallows'} actions other address`, async () => { + const assertion = allowsAnyAddress ? assert.isTrue : assert.isFalse + assertion(await acl.hasPermission(noPermission, mockAppAddress, MOCK_APP_ROLE)) + }) }) describe('when permission is set for specific address', async () => { @@ -62,30 +66,40 @@ contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppA await acl.grantPermissionP(specificEntity, mockAppAddress, MOCK_APP_ROLE, [aclParams]) }) - it(`ACL ${allowText} actions for specific address`, async () => { - assertion = shouldHavePermission ? assert.isTrue : assert.isFalse + it(`ACL ${allowsSpecificAddress ? 'allows' : 'disallows'} actions for specific address`, async () => { + const assertion = allowsSpecificAddress ? assert.isTrue : assert.isFalse assertion(await acl.hasPermission(specificEntity, mockAppAddress, MOCK_APP_ROLE)) }) - it('ACL disallows actions for other addresses', async () => { - assert.isFalse(await acl.hasPermission(noPermission, mockAppAddress, MOCK_APP_ROLE)) + it('ACL disallows actions for ANY_ADDR', async () => { assert.isFalse(await acl.hasPermission(ANY_ADDR, mockAppAddress, MOCK_APP_ROLE)) }) + + it('ACL disallows other address', async () => { + assert.isFalse(await acl.hasPermission(noPermission, mockAppAddress, MOCK_APP_ROLE)) + }) }) } - describe('when the oracle accepts', () => { - let acceptOracle - + describe('when the oracle accepts always', () => { before(async () => { - acceptOracle = await AcceptOracle.new() + const acceptOracle = await AcceptOracle.new() aclParams = permissionParamEqOracle(acceptOracle.address) }) - testOraclePermissions({ shouldHavePermission: true }) + testOraclePermissions({ allowsAnyAddress: true, allowsSpecificAddress: true }) + }) + + describe('when the oracle accepts specific addresses', () => { + before(async () => { + const onlyOwnerOracle = await OnlyOwnerOracle.new(specificEntity) + aclParams = permissionParamEqOracle(onlyOwnerOracle.address) + }) + + testOraclePermissions({ allowsAnyAddress: false, allowsSpecificAddress: true }) }) - for (let [description, FailingOracle] of [ + for (const [description, FailingOracle] of [ ['rejects', RejectOracle], ['reverts', RevertOracle], ]) { @@ -97,7 +111,7 @@ contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppA aclParams = permissionParamEqOracle(failingOracle.address) }) - testOraclePermissions({ shouldHavePermission: false }) + testOraclePermissions({ allowsAnyAddress: false, allowsSpecificAddress: false }) }) } @@ -109,12 +123,11 @@ contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppA aclParams = permissionParamEqOracle(stateModifyingOracle.address) }) - testOraclePermissions({ shouldHavePermission: false }) + testOraclePermissions({ allowsAnyAddress: false, allowsSpecificAddress: false }) }) - // Both the assert and oog gas cases should be similar, since assert should eat all the - // available gas - for (let [description, FailingOutOfGasOracle] of [ + // Both the assert and oog gas cases should be similar, since assert should eat all the available gas + for (const [description, FailingOutOfGasOracle] of [ ['asserts', AssertOracle], ['uses all available gas', OverGasLimitOracle], ]) { @@ -126,13 +139,13 @@ contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppA aclParams = permissionParamEqOracle(overGasLimitOracle.address) }) - testOraclePermissions({ shouldHavePermission: false }) + testOraclePermissions({ allowsAnyAddress: false, allowsSpecificAddress: false }) describe('gas', () => { describe('when permission is set for ANY_ADDR', () => { // Note `evalParams()` is called twice when calling `hasPermission` for `ANY_ADDR`, so // gas costs are much, much higher to compensate for the 63/64th rule on the second call - const MEDIUM_GAS = 3000000 + const MEDIUM_GAS = 3500000 const LOW_GAS = 2900000 beforeEach(async () => { @@ -164,7 +177,7 @@ contract('ACL params', ([permissionsRoot, specificEntity, noPermission, mockAppA }) describe('when permission is set for specific address', async () => { - const MEDIUM_GAS = 190000 + const MEDIUM_GAS = 200000 // Note that these gas values are still quite high for causing reverts in "low gas" // situations, as we incur some overhead with delegating into proxies and other checks. // Assuming we incur 40-60k gas overhead for this, we only have ~140,000 gas left. From e6ea55f8f0fe35f664ef276461e9fb900dd6e086 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Thu, 2 Jul 2020 16:45:17 +0200 Subject: [PATCH 8/8] ACL: update user/who parameters to who/grantee (#589) * ACL: update user/who parameters to who/grantee * ACL: update user/who variable names in tests * ACL: fix lint --- contracts/acl/ACL.sol | 44 ++++++++++++--------- contracts/acl/IACLOracle.sol | 20 +++++----- contracts/acl/IACLOracleV1.sol | 6 +-- contracts/test/tests/TestACLInterpreter.sol | 8 ++-- 4 files changed, 43 insertions(+), 35 deletions(-) diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index b1ade009a..f92fee36a 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -264,19 +264,19 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { /** * @dev Function called by apps to evaluate ACL params * @param _paramsHash Params hash identifier - * @param _user Sender of the original call - * @param _who Grantee of the role + * @param _who Sender of the original call + * @param _grantee Grantee of the role * @param _where Address of the app * @param _what Identifier for a group of actions in app (role) * @param _how Permission parameters * @return boolean indicating whether the ACL allows the role or not */ - function evalParams(bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + function evalParams(bytes32 _paramsHash, address _who, address _grantee, address _where, bytes32 _what, uint256[] _how) external view returns (bool) { - return _evalParams(_paramsHash, _user, _who, _where, _what, _how); + return _evalParams(_paramsHash, _who, _grantee, _where, _what, _how); } /** @@ -362,7 +362,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { /** * @dev Internal function to perform an ACL check */ - function _evalParams(bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + function _evalParams(bytes32 _paramsHash, address _who, address _grantee, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) @@ -372,13 +372,13 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { } // `_evalParam()` will internally traverse all the parameters, starting from the first parameter (0) - return _evalParam(_paramsHash, 0, _user, _who, _where, _what, _how); + return _evalParam(_paramsHash, 0, _who, _grantee, _where, _what, _how); } /** * @dev Internal function to perform an ACL check on a single permission parameter */ - function _evalParam(bytes32 _paramsHash, uint32 _paramId, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + function _evalParam(bytes32 _paramsHash, uint32 _paramId, address _who, address _grantee, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) @@ -391,8 +391,8 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { if (param.id == LOGIC_OP_PARAM_ID) { return (Op(param.op) == Op.IF_ELSE) - ? _evalIfElseLogicOp(param, _paramsHash, _user, _who, _where, _what, _how) - : _evalNonIfElseLogicOp(param, _paramsHash, _user, _who, _where, _what, _how); + ? _evalIfElseLogicOp(param, _paramsHash, _who, _grantee, _where, _what, _how) + : _evalNonIfElseLogicOp(param, _paramsHash, _who, _grantee, _where, _what, _how); } uint256 value; @@ -400,7 +400,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { // get value if (param.id == ORACLE_PARAM_ID) { - value = _checkOracle(IACLOracle(param.value), _user, _who, _where, _what, _how) ? 1 : 0; + value = _checkOracle(IACLOracle(param.value), _who, _grantee, _where, _what, _how) ? 1 : 0; comparedTo = 1; } else if (param.id == BLOCK_NUMBER_PARAM_ID) { value = getBlockNumber(); @@ -425,7 +425,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { /** * @dev Internal function to eval an IF-ELSE logic operator */ - function _evalIfElseLogicOp(Param _param, bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + function _evalIfElseLogicOp(Param _param, bytes32 _paramsHash, address _who, address _grantee, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) @@ -435,21 +435,29 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { uint32 failureParam; (conditionParam, successParam, failureParam) = decodeParamsList(uint256(_param.value)); - bool result = _evalParam(_paramsHash, conditionParam, _user, _who, _where, _what, _how); + bool result = _evalParam(_paramsHash, conditionParam, _who, _grantee, _where, _what, _how); - return _evalParam(_paramsHash, result ? successParam : failureParam, _user, _who, _where, _what, _how); + return _evalParam(_paramsHash, result ? successParam : failureParam, _who, _grantee, _where, _what, _how); } /** * @dev Internal function to eval a non-IF-ELSE logic operator */ - function _evalNonIfElseLogicOp(Param _param, bytes32 _paramsHash, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + function _evalNonIfElseLogicOp( + Param _param, + bytes32 _paramsHash, + address _who, + address _grantee, + address _where, + bytes32 _what, + uint256[] _how + ) internal view returns (bool) { (uint32 param1, uint32 param2,) = decodeParamsList(uint256(_param.value)); - bool r1 = _evalParam(_paramsHash, param1, _user, _who, _where, _what, _how); + bool r1 = _evalParam(_paramsHash, param1, _who, _grantee, _where, _what, _how); if (Op(_param.op) == Op.NOT) { return !r1; @@ -463,7 +471,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { return false; } - bool r2 = _evalParam(_paramsHash, param2, _user, _who, _where, _what, _how); + bool r2 = _evalParam(_paramsHash, param2, _who, _grantee, _where, _what, _how); if (Op(_param.op) == Op.XOR) { return r1 != r2; @@ -475,14 +483,14 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { /** * @dev Internal function to perform an ACL oracle check */ - function _checkOracle(IACLOracle _oracleAddr, address _user, address _who, address _where, bytes32 _what, uint256[] _how) + function _checkOracle(IACLOracle _oracleAddr, address _who, address _grantee, address _where, bytes32 _what, uint256[] _how) internal view returns (bool) { // a raw call is required so we can return false if the call reverts, rather than reverting bytes4 sig = _oracleAddr.canPerform.selector; - bytes memory checkCalldata = abi.encodeWithSelector(sig, _user, _who, _where, _what, _how); + bytes memory checkCalldata = abi.encodeWithSelector(sig, _who, _grantee, _where, _what, _how); bool ok; assembly { diff --git a/contracts/acl/IACLOracle.sol b/contracts/acl/IACLOracle.sol index 98065e8cc..f050c6ac6 100644 --- a/contracts/acl/IACLOracle.sol +++ b/contracts/acl/IACLOracle.sol @@ -7,18 +7,18 @@ pragma solidity ^0.4.24; /** * @title ACL Oracle interface -* @dev This interface simply defines a check method that must be implemented by smart contracts to be plugged in as ACL oracles. -* ACL oracles are the most suitable way to have external contracts validating ACL permissions with custom logic. +* @dev This interface simply defines a predicate method that must be implemented by smart contracts intended to be used as ACL Oracles. +* ACL oracles should be used if you would like to protect a permission with custom logic from an external contract. */ interface IACLOracle { /** - * @dev Tells whether `user` can execute `what`(`how`) in `where` if it's currently set up for `who` - * @param user Entity trying to execute `what` in `where` - * @param who Entity to which `what` is granted based on the current ACL permissions configuration - * @param where Entity where `what` is trying to be executed - * @param what Identifier of the action willing to be executed in `where` - * @param how Can be used to define a set of arguments to give more context about `what` is trying to be executed in `where` - * @return True if the user is allowed to execute the requested action for the given context, false otherwise + * @dev Tells whether `sender` can execute `what` (and `how`) in `where` for the grantee `who` + * @param who Sender of the original call + * @param grantee Grantee of the permission being evaluated + * @param where Address of the app + * @param what Identifier for a group of actions in app (role) + * @param how Permission parameters + * @return True if the action should be accepted */ - function canPerform(address user, address who, address where, bytes32 what, uint256[] how) external view returns (bool); + function canPerform(address who, address grantee, address where, bytes32 what, uint256[] how) external view returns (bool); } diff --git a/contracts/acl/IACLOracleV1.sol b/contracts/acl/IACLOracleV1.sol index c54c4eb78..4db9beed4 100644 --- a/contracts/acl/IACLOracleV1.sol +++ b/contracts/acl/IACLOracleV1.sol @@ -6,9 +6,9 @@ pragma solidity ^0.4.24; /** -* @title Previous version of the ACL Oracle interface -* @dev This interface simply defines a check method that must be implemented by smart contracts to be plugged in as ACL oracles. -* ACL oracles are the most suitable way to have external contracts validating ACL permissions with custom logic. +* @title Previous version of the ACL Oracle interface (aragonOS@4) +* @dev This interface simply defines a predicate method that must be implemented by smart contracts intended to be used as ACL Oracles. +* ACL oracles should be used if you would like to protect a permission with custom logic from an external contract. */ interface IACLOracleV1 { function canPerform(address who, address where, bytes32 what, uint256[] how) external view returns (bool); diff --git a/contracts/test/tests/TestACLInterpreter.sol b/contracts/test/tests/TestACLInterpreter.sol index b09365291..33c9cd89e 100644 --- a/contracts/test/tests/TestACLInterpreter.sol +++ b/contracts/test/tests/TestACLInterpreter.sol @@ -278,10 +278,10 @@ contract TestACLInterpreter is ACL, ACLHelper { assertEval(params, false); } - function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, address user, address who, bool expected) internal { + function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, address who, address grantee, bool expected) internal { Param[] memory params = new Param[](1); params[0] = Param(argId, uint8(op), uint240(value)); - assertEval(params, args, user, who, expected); + assertEval(params, args, who, grantee, expected); } function assertEval(uint256[] memory args, uint8 argId, Op op, uint256 value, bool expected) internal { @@ -296,9 +296,9 @@ contract TestACLInterpreter is ACL, ACLHelper { assertEval(params, args, address(0), address(0), expected); } - function assertEval(Param[] memory params, uint256[] memory args, address user, address who, bool expected) internal { + function assertEval(Param[] memory params, uint256[] memory args, address who, address grantee, bool expected) internal { bytes32 paramHash = _encodeAndSaveParams(params); - bool allow = _evalParam(paramHash, 0, user, who, address(0), bytes32(0), args); + bool allow = _evalParam(paramHash, 0, who, grantee, address(0), bytes32(0), args); Assert.equal(allow, expected, "eval got unexpected result"); }