Forwarder: Extend interfaces (#583)

* forwarder: move to separate dir and make fns external

* forwarder: provide payable interface

* forwarder: provide forwarder interface with extra data

* forwarder: remove unused import

* forwarder: improve forwarders hierarchy

* Forwarding: update forwarding interface names, add tests (#590)

* tests: remove wrong (and unused) test import

Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>

contracts/forwarding/IAbstractForwarder.sol

@@ -0,0 +1,43 @@
+/*
+ * SPDX-License-Identifier:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+
+/**
+* @title Abstract forwarder interface
+* @dev This is the base interface for all forwarders.
+*      Forwarding allows separately installed applications (smart contracts implementing the forwarding interface) to execute multi-step actions via EVM scripts.
+*      You should only support the forwarding interface if your "action step" is asynchronous (e.g. requiring a delay period or a voting period).
+*      Note: you should **NOT** directly inherit from this interface; see one of the other, non-abstract interfaces available.
+*/
+contract IAbstractForwarder {
+    enum ForwarderType {
+        NOT_IMPLEMENTED,
+        NO_CONTEXT,
+        WITH_CONTEXT
+    }
+
+    /**
+    * @dev Tell whether the proposed forwarding path (an EVM script) from the given sender is allowed.
+    *      The implemented `forward()` method **MUST NOT** revert if `canForward()` returns true for the same parameters.
+    * @return True if the sender's proposed path is allowed
+    */
+    function canForward(address sender, bytes evmScript) external view returns (bool);
+
+    /**
+    * @dev Tell the forwarder type
+    * @return Forwarder type
+    */
+    function forwarderType() external pure returns (ForwarderType);
+
+    /**
+    * @dev Report whether the implementing app is a forwarder
+    *      Required for backwards compatibility with aragonOS 4
+    * @return Always true
+    */
+    function isForwarder() external pure returns (bool) {
+        return true;
+    }
+}

contracts/forwarding/IForwarder.sol

@@ -0,0 +1,28 @@
+/*
+ * SPDX-License-Identifier:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+import "./IAbstractForwarder.sol";
+
+
+/**
+* @title Forwarder interface
+* @dev This is the basic forwarder interface, that only supports forwarding an EVM script.
+*      It does not support forwarding additional context or receiving ETH; other interfaces are available to support those.
+*/
+contract IForwarder is IAbstractForwarder {
+    /**
+    * @dev Forward an EVM script
+    */
+    function forward(bytes evmScript) external;
+
+    /**
+    * @dev Tell the forwarder type
+    * @return Always 1 (ForwarderType.NO_CONTEXT)
+    */
+    function forwarderType() external pure returns (ForwarderType) {
+        return ForwarderType.NO_CONTEXT;
+    }
+}

contracts/forwarding/IForwarderFee.sol

@@ -0,0 +1,18 @@
+/*
+ * SPDX-License-Identifier:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+
+/**
+* @title Forwarder fee interface
+* @dev Interface for declaring any fee requirements for the `forward()` function.
+*/
+interface IForwarderFee {
+    /**
+    * @dev Provide details about the required fee token and amount
+    * @return Fee token and fee amount. If ETH, expects EtherTokenConstant.
+    */
+    function forwardFee() external view returns (address feeToken, uint256 feeAmount);
+}

contracts/forwarding/IForwarderPayable.sol

@@ -0,0 +1,30 @@
+/*
+ * SPDX-License-Identifier:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+import "./IForwarder.sol";
+import "./IForwarderFee.sol";
+
+
+/**
+* @title Payable forwarder interface
+* @dev This is the basic forwarder interface, that only supports forwarding an EVM script.
+*      Unlike `IForwarder`, this interface allows `forward()` to receive ETH and thereby includes the IForwarderFee interface.
+*      It is **RECOMMENDED** that only apps requiring a payable `forward()` use this interface.
+*/
+contract IForwarderPayable is IAbstractForwarder, IForwarderFee {
+    /**
+    * @dev Forward an EVM script
+    */
+    function forward(bytes evmScript) external payable;
+
+    /**
+    * @dev Tell the forwarder type
+    * @return Always 1 (ForwarderType.NO_CONTEXT)
+    */
+    function forwarderType() external pure returns (ForwarderType) {
+        return ForwarderType.NO_CONTEXT;
+    }
+}

contracts/forwarding/IForwarderWithContext.sol

@@ -0,0 +1,27 @@
+/*
+ * SPDX-License-Identifier:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+import "./IAbstractForwarder.sol";
+
+
+/**
+* @title Forwarder interface requiring context information
+* @dev This forwarder interface allows for additional context to be attached to the action by the sender.
+*/
+contract IForwarderWithContext is IAbstractForwarder {
+    /**
+    * @dev Forward an EVM script with an attached context
+    */
+    function forward(bytes evmScript, bytes context) external;
+
+    /**
+    * @dev Tell the forwarder type
+    * @return Always 2 (ForwarderType.WITH_CONTEXT)
+    */
+    function forwarderType() external pure returns (ForwarderType) {
+        return ForwarderType.WITH_CONTEXT;
+    }
+}

contracts/forwarding/IForwarderWithContextPayable.sol

@@ -0,0 +1,30 @@
+/*
+ * SPDX-License-Identifier:    MIT
+ */
+
+pragma solidity ^0.4.24;
+
+import "./IAbstractForwarder.sol";
+import "./IForwarderFee.sol";
+
+
+/**
+* @title Payable forwarder interface requiring context information
+* @dev This forwarder interface allows for additional context to be attached to the action by the sender.
+*      Unlike `IForwarderWithContext`, this interface allows `forward()` to receive ETH and thereby includes the IForwarderFee interface.
+*      It is **RECOMMENDED** that only apps requiring a payable `forward()` use this interface.
+*/
+contract IForwarderWithContextPayable is IAbstractForwarder, IForwarderFee {
+    /**
+    * @dev Forward an EVM script with an attached context
+    */
+    function forward(bytes evmScript, bytes context) external payable;
+
+    /**
+    * @dev Tell the forwarder type
+    * @return Always 2 (ForwarderType.WITH_CONTEXT)
+    */
+    function forwarderType() external pure returns (ForwarderType) {
+        return ForwarderType.WITH_CONTEXT;
+    }
+}

contracts/test/mocks/apps/disputable/DisputableAppMock.sol

@@ -1,6 +1,5 @@
 pragma solidity 0.4.24;
 
-import "../../../../common/IForwarder.sol";
 import "../../../../apps/disputable/DisputableAragonApp.sol";
 
 

contracts/test/mocks/forwarding/ForwardingMock.sol

@@ -0,0 +1,42 @@
+pragma solidity 0.4.24;
+
+import "../../../forwarding/IAbstractForwarder.sol";
+import "../../../forwarding/IForwarderFee.sol";
+import "../../../forwarding/IForwarder.sol";
+import "../../../forwarding/IForwarderPayable.sol";
+import "../../../forwarding/IForwarderWithContext.sol";
+import "../../../forwarding/IForwarderWithContextPayable.sol";
+
+
+contract BaseForwarderMock is IAbstractForwarder {
+    function canForward(address, bytes) external view returns (bool) {
+        return true;
+    }
+}
+
+
+contract BaseForwarderPayableMock is BaseForwarderMock, IForwarderFee {
+    function forwardFee() external view returns (address, uint256) {
+        return (address(0), 0);
+    }
+}
+
+
+contract ForwarderMock is BaseForwarderMock, IForwarder {
+    function forward(bytes) external { }
+}
+
+
+contract ForwarderPayableMock is BaseForwarderPayableMock, IForwarderPayable {
+    function forward(bytes) external payable { }
+}
+
+
+contract ForwarderWithContextMock is BaseForwarderMock, IForwarderWithContext {
+    function forward(bytes, bytes) external { }
+}
+
+
+contract ForwarderWithContextPayableMock is BaseForwarderPayableMock, IForwarderWithContextPayable {
+    function forward(bytes, bytes) external payable { }
+}

contracts/test/tests/TestConversionHelpers.sol

@@ -7,11 +7,11 @@ import "../../common/ConversionHelpers.sol";
 
 
 contract InvalidBytesLengthConversionThrows {
-    function tryConvertLength(uint256 _badLength) public {
+    function tryConvertLength(uint256 _badLength) public pure {
         bytes memory arr = new bytes(_badLength);
 
         // Do failing conversion
-        uint256[] memory arrUint = ConversionHelpers.dangerouslyCastBytesToUintArray(arr);
+        ConversionHelpers.dangerouslyCastBytesToUintArray(arr);
     }
 }
 

test/contracts/forwarding/forwarding.js

@@ -0,0 +1,116 @@
+const { assertRevert } = require('../../helpers/assertThrow')
+
+// Mocks
+const ForwarderMock = artifacts.require('ForwarderMock')
+const ForwarderPayableMock = artifacts.require('ForwarderPayableMock')
+const ForwarderWithContextMock = artifacts.require('ForwarderWithContextMock')
+const ForwarderWithContextPayableMock = artifacts.require('ForwarderWithContextPayableMock')
+
+const EMPTY_BYTES = '0x'
+const ForwarderType = {
+  NOT_IMPLEMENTED: 0,
+  NO_CONTEXT: 1,
+  WITH_CONTEXT: 2,
+}
+
+contract('Forwarders', () => {
+  context('IForwarder', () => {
+    let forwarder
+
+    beforeEach(async () => {
+      forwarder = await ForwarderMock.new()
+    })
+
+    it('is a forwarder', async () => {
+      assert.isTrue(await forwarder.isForwarder(), 'should be a forwarder')
+    })
+
+    it('reports correct forwarder type', async () => {
+      assert.equal(await forwarder.forwarderType(), ForwarderType.NO_CONTEXT, 'should report correct forwarding type')
+    })
+
+    it('can forward', async () => {
+      assert.doesNotThrow(async () => await forwarder.forward(EMPTY_BYTES))
+    })
+
+    it('cannot forward with ETH payment', async () => {
+      // Override the contract ABI to let us attempt sending value into this non-payable forwarder
+      const payableForwarder = ForwarderPayableMock.at(forwarder.address)
+      await assertRevert(payableForwarder.forward(EMPTY_BYTES, { value: 1 }))
+    })
+  })
+
+  context('IForwarderPayable', () => {
+    let forwarder
+
+    beforeEach(async () => {
+      forwarder = await ForwarderPayableMock.new()
+    })
+
+    it('is a forwarder', async () => {
+      assert.isTrue(await forwarder.isForwarder(), 'should be a forwarder')
+    })
+
+    it('reports correct forwarder type', async () => {
+      assert.equal(await forwarder.forwarderType(), ForwarderType.NO_CONTEXT, 'should report correct forwarding type')
+    })
+
+    it('can forward', async () => {
+      assert.doesNotThrow(async () => await forwarder.forward(EMPTY_BYTES))
+    })
+
+    it('can forward with ETH payment', async () => {
+      assert.doesNotThrow(async () => await forwarder.forward(EMPTY_BYTES, { value: 1 }))
+    })
+  })
+
+  context('IForwarderWithContext', () => {
+    let forwarder
+
+    beforeEach(async () => {
+      forwarder = await ForwarderWithContextMock.new()
+    })
+
+    it('is a forwarder', async () => {
+      assert.isTrue(await forwarder.isForwarder(), 'should be a forwarder')
+    })
+
+    it('reports correct forwarder type', async () => {
+      assert.equal(await forwarder.forwarderType(), ForwarderType.WITH_CONTEXT, 'should report correct forwarding type')
+    })
+
+    it('can forward', async () => {
+      assert.doesNotThrow(async () => await forwarder.forward(EMPTY_BYTES, EMPTY_BYTES))
+    })
+
+    it('cannot forward with ETH payment', async () => {
+      // Override the contract ABI to let us attempt sending value into this non-payable forwarder
+      const payableForwarder = ForwarderWithContextPayableMock.at(forwarder.address)
+      await assertRevert(payableForwarder.forward(EMPTY_BYTES, EMPTY_BYTES, { value: 1 }))
+    })
+  })
+
+  context('IForwarderWithContextPayable', () => {
+    let forwarder
+
+    beforeEach(async () => {
+      forwarder = await ForwarderWithContextPayableMock.new()
+    })
+
+    it('is a forwarder', async () => {
+      assert.isTrue(await forwarder.isForwarder(), 'should be a forwarder')
+    })
+
+    it('reports correct forwarder type', async () => {
+      assert.equal(await forwarder.forwarderType(), ForwarderType.WITH_CONTEXT, 'should report correct forwarding type')
+    })
+
+    it('can forward', async () => {
+      assert.doesNotThrow(async () => await forwarder.forward(EMPTY_BYTES, EMPTY_BYTES))
+    })
+
+    it('can forward with ETH payment', async () => {
+      assert.doesNotThrow(async () => await forwarder.forward(EMPTY_BYTES, EMPTY_BYTES, { value: 1 }))
+    })
+  })
+})