diff --git a/contracts/common/IForwarder.sol b/contracts/common/IForwarder.sol deleted file mode 100644 index a54b8cfdc..000000000 --- a/contracts/common/IForwarder.sol +++ /dev/null @@ -1,18 +0,0 @@ -/* - * SPDX-License-Identifier: MIT - */ - -pragma solidity ^0.4.24; - - -interface IForwarder { - function isForwarder() external pure returns (bool); - - // TODO: this should be external - // See https://github.com/ethereum/solidity/issues/4832 - function canForward(address sender, bytes evmCallScript) public view returns (bool); - - // TODO: this should be external - // See https://github.com/ethereum/solidity/issues/4832 - function forward(bytes evmCallScript) public; -} diff --git a/contracts/common/IForwarderFee.sol b/contracts/common/IForwarderFee.sol deleted file mode 100644 index 0fecb3418..000000000 --- a/contracts/common/IForwarderFee.sol +++ /dev/null @@ -1,10 +0,0 @@ -/* - * SPDX-License-Identifier: MIT - */ - -pragma solidity ^0.4.24; - - -interface IForwarderFee { - function forwardFee() external view returns (address, uint256); -} diff --git a/contracts/forwarding/IAbstractForwarder.sol b/contracts/forwarding/IAbstractForwarder.sol new file mode 100644 index 000000000..4a1a8c17b --- /dev/null +++ b/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; + } +} diff --git a/contracts/forwarding/IForwarder.sol b/contracts/forwarding/IForwarder.sol new file mode 100644 index 000000000..a20579b01 --- /dev/null +++ b/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; + } +} diff --git a/contracts/forwarding/IForwarderFee.sol b/contracts/forwarding/IForwarderFee.sol new file mode 100644 index 000000000..621816420 --- /dev/null +++ b/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); +} diff --git a/contracts/forwarding/IForwarderPayable.sol b/contracts/forwarding/IForwarderPayable.sol new file mode 100644 index 000000000..e94fbfd8a --- /dev/null +++ b/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; + } +} diff --git a/contracts/forwarding/IForwarderWithContext.sol b/contracts/forwarding/IForwarderWithContext.sol new file mode 100644 index 000000000..33e8c8074 --- /dev/null +++ b/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; + } +} diff --git a/contracts/forwarding/IForwarderWithContextPayable.sol b/contracts/forwarding/IForwarderWithContextPayable.sol new file mode 100644 index 000000000..629c56f7a --- /dev/null +++ b/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; + } +} diff --git a/contracts/test/mocks/apps/disputable/DisputableAppMock.sol b/contracts/test/mocks/apps/disputable/DisputableAppMock.sol index 4e8d10ba1..9a96655d9 100644 --- a/contracts/test/mocks/apps/disputable/DisputableAppMock.sol +++ b/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"; diff --git a/contracts/test/mocks/forwarding/ForwardingMock.sol b/contracts/test/mocks/forwarding/ForwardingMock.sol new file mode 100644 index 000000000..6bd25f6fd --- /dev/null +++ b/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 { } +} diff --git a/contracts/test/tests/TestConversionHelpers.sol b/contracts/test/tests/TestConversionHelpers.sol index ea7da4c6f..ed3612502 100644 --- a/contracts/test/tests/TestConversionHelpers.sol +++ b/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); } } diff --git a/test/contracts/forwarding/forwarding.js b/test/contracts/forwarding/forwarding.js new file mode 100644 index 000000000..2f89722ba --- /dev/null +++ b/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 })) + }) + }) +})