Merge GSNContext into GSNRecipient (#1906)

* Merge GSNContext into GSNRecipient

* Update GSNRecipient.test.js

* Update GSNRecipient.sol

* Make GSNRecipient inherit from Context

contracts/GSN/GSNContext.sol

@@ -1,88 +0,0 @@
-pragma solidity ^0.5.0;
-
-import "./Context.sol";
-
-/**
- * @dev Enables GSN support on `Context` contracts by recognizing calls from
- * RelayHub and extracting the actual sender and call data from the received
- * calldata.
- *
- * > This contract does not perform all required tasks to implement a GSN
- * recipient contract: end users should use `GSNRecipient` instead.
- */
-contract GSNContext is Context {
-    address internal _relayHub = 0xD216153c06E857cD7f72665E0aF1d7D82172F494;
-
-    event RelayHubChanged(address indexed oldRelayHub, address indexed newRelayHub);
-
-    constructor() internal {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function _upgradeRelayHub(address newRelayHub) internal {
-        address currentRelayHub = _relayHub;
-        require(newRelayHub != address(0), "GSNContext: new RelayHub is the zero address");
-        require(newRelayHub != currentRelayHub, "GSNContext: new RelayHub is the current one");
-
-        emit RelayHubChanged(currentRelayHub, newRelayHub);
-
-        _relayHub = newRelayHub;
-    }
-
-    // Overrides for Context's functions: when called from RelayHub, sender and
-    // data require some pre-processing: the actual sender is stored at the end
-    // of the call data, which in turns means it needs to be removed from it
-    // when handling said data.
-
-    function _msgSender() internal view returns (address) {
-        if (msg.sender != _relayHub) {
-            return msg.sender;
-        } else {
-            return _getRelayedCallSender();
-        }
-    }
-
-    function _msgData() internal view returns (bytes memory) {
-        if (msg.sender != _relayHub) {
-            return msg.data;
-        } else {
-            return _getRelayedCallData();
-        }
-    }
-
-    function _getRelayedCallSender() private pure returns (address result) {
-        // We need to read 20 bytes (an address) located at array index msg.data.length - 20. In memory, the array
-        // is prefixed with a 32-byte length value, so we first add 32 to get the memory read index. However, doing
-        // so would leave the address in the upper 20 bytes of the 32-byte word, which is inconvenient and would
-        // require bit shifting. We therefore subtract 12 from the read index so the address lands on the lower 20
-        // bytes. This can always be done due to the 32-byte prefix.
-
-        // The final memory read index is msg.data.length - 20 + 32 - 12 = msg.data.length. Using inline assembly is the
-        // easiest/most-efficient way to perform this operation.
-
-        // These fields are not accessible from assembly
-        bytes memory array = msg.data;
-        uint256 index = msg.data.length;
-
-        // solhint-disable-next-line no-inline-assembly
-        assembly {
-            // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those.
-            result := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff)
-        }
-        return result;
-    }
-
-    function _getRelayedCallData() private pure returns (bytes memory) {
-        // RelayHub appends the sender address at the end of the calldata, so in order to retrieve the actual msg.data,
-        // we must strip the last 20 bytes (length of an address type) from it.
-
-        uint256 actualDataLength = msg.data.length - 20;
-        bytes memory actualData = new bytes(actualDataLength);
-
-        for (uint256 i = 0; i < actualDataLength; ++i) {
-            actualData[i] = msg.data[i];
-        }
-
-        return actualData;
-    }
-}

contracts/GSN/GSNRecipient.sol

@@ -1,9 +1,9 @@
 pragma solidity ^0.5.0;
 
 import "./IRelayRecipient.sol";
-import "./GSNContext.sol";
-import "./bouncers/GSNBouncerBase.sol";
 import "./IRelayHub.sol";
+import "./Context.sol";
+import "./bouncers/GSNBouncerBase.sol";
 
 /**
  * @dev Base GSN recipient contract: includes the {IRelayRecipient} interface and enables GSN support on all contracts
@@ -11,16 +11,42 @@ import "./IRelayHub.sol";
  *
  * Not all interface methods are implemented (e.g. {acceptRelayedCall}, derived contracts must provide one themselves.
  */
-contract GSNRecipient is IRelayRecipient, GSNContext, GSNBouncerBase {
+contract GSNRecipient is IRelayRecipient, Context, GSNBouncerBase {
+    // Default RelayHub address, deployed on mainnet and all testnets at the same address
+    address private _relayHub = 0xD216153c06E857cD7f72665E0aF1d7D82172F494;
+
     /**
-     * @dev Returns the `RelayHub` address for this recipient contract.
+     * @dev Emitted when a contract changes its {IRelayHub} contract to a new one.
+     */
+    event RelayHubChanged(address indexed oldRelayHub, address indexed newRelayHub);
+
+    /**
+     * @dev Returns the address of the {IRelayHub} contract for this recipient.
      */
     function getHubAddr() public view returns (address) {
         return _relayHub;
     }
 
     /**
-     * @dev Returns the version string of the `RelayHub` for which this recipient implementation was built.
+     * @dev Switches to a new {IRelayHub} instance. This method is added for future-proofing: there's no reason to not
+     * use the default instance.
+     *
+     * IMPORTANT: After upgrading, the {GSNRecipient} will no longer be able to receive relayed calls from the old
+     * {IRelayHub} instance. Additionally, all funds should be previously withdrawn via {_withdrawDeposits}.
+     */
+    function _upgradeRelayHub(address newRelayHub) internal {
+        address currentRelayHub = _relayHub;
+        require(newRelayHub != address(0), "GSNRecipient: new RelayHub is the zero address");
+        require(newRelayHub != currentRelayHub, "GSNRecipient: new RelayHub is the current one");
+
+        emit RelayHubChanged(currentRelayHub, newRelayHub);
+
+        _relayHub = newRelayHub;
+    }
+
+    /**
+     * @dev Returns the version string of the {IRelayHub} for which this recipient implementation was built. If
+     * {_upgradeRelayHub} is used, the new {IRelayHub} instance should be compatible with this version.
      */
     // This function is view for future-proofing, it may require reading from
     // storage in the future.
@@ -37,4 +63,73 @@ contract GSNRecipient is IRelayRecipient, GSNContext, GSNBouncerBase {
     function _withdrawDeposits(uint256 amount, address payable payee) internal {
         IRelayHub(_relayHub).withdraw(amount, payee);
     }
+
+    // Overrides for Context's functions: when called from RelayHub, sender and
+    // data require some pre-processing: the actual sender is stored at the end
+    // of the call data, which in turns means it needs to be removed from it
+    // when handling said data.
+
+    /**
+     * @dev Replacement for msg.sender. Returns the actual sender of a transaction: msg.sender for regular transactions,
+     * and the end-user for GSN relayed calls (where msg.sender is actually `RelayHub`).
+     *
+     * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.sender`, and use {_msgSender} instead.
+     */
+    function _msgSender() internal view returns (address) {
+        if (msg.sender != _relayHub) {
+            return msg.sender;
+        } else {
+            return _getRelayedCallSender();
+        }
+    }
+
+    /**
+     * @dev Replacement for msg.data. Returns the actual calldata of a transaction: msg.data for regular transactions,
+     * and a reduced version for GSN relayed calls (where msg.data contains additional information).
+     *
+     * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.data`, and use {_msgData} instead.
+     */
+    function _msgData() internal view returns (bytes memory) {
+        if (msg.sender != _relayHub) {
+            return msg.data;
+        } else {
+            return _getRelayedCallData();
+        }
+    }
+
+    function _getRelayedCallSender() private pure returns (address result) {
+        // We need to read 20 bytes (an address) located at array index msg.data.length - 20. In memory, the array
+        // is prefixed with a 32-byte length value, so we first add 32 to get the memory read index. However, doing
+        // so would leave the address in the upper 20 bytes of the 32-byte word, which is inconvenient and would
+        // require bit shifting. We therefore subtract 12 from the read index so the address lands on the lower 20
+        // bytes. This can always be done due to the 32-byte prefix.
+
+        // The final memory read index is msg.data.length - 20 + 32 - 12 = msg.data.length. Using inline assembly is the
+        // easiest/most-efficient way to perform this operation.
+
+        // These fields are not accessible from assembly
+        bytes memory array = msg.data;
+        uint256 index = msg.data.length;
+
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those.
+            result := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff)
+        }
+        return result;
+    }
+
+    function _getRelayedCallData() private pure returns (bytes memory) {
+        // RelayHub appends the sender address at the end of the calldata, so in order to retrieve the actual msg.data,
+        // we must strip the last 20 bytes (length of an address type) from it.
+
+        uint256 actualDataLength = msg.data.length - 20;
+        bytes memory actualData = new bytes(actualDataLength);
+
+        for (uint256 i = 0; i < actualDataLength; ++i) {
+            actualData[i] = msg.data[i];
+        }
+
+        return actualData;
+    }
 }

contracts/mocks/GSNContextMock.sol

@@ -1,46 +0,0 @@
-pragma solidity ^0.5.0;
-
-import "./ContextMock.sol";
-import "../GSN/GSNContext.sol";
-import "../GSN/IRelayRecipient.sol";
-
-// By inheriting from GSNContext, Context's internal functions are overridden automatically
-contract GSNContextMock is ContextMock, GSNContext, IRelayRecipient {
-    function getHubAddr() public view returns (address) {
-        return _relayHub;
-    }
-
-    function acceptRelayedCall(
-        address,
-        address,
-        bytes calldata,
-        uint256,
-        uint256,
-        uint256,
-        uint256,
-        bytes calldata,
-        uint256
-    )
-        external
-        view
-        returns (uint256, bytes memory)
-    {
-        return (0, "");
-    }
-
-    function preRelayedCall(bytes calldata) external returns (bytes32) {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function postRelayedCall(bytes calldata, bool, uint256, bytes32) external {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function getRelayHub() public view returns (address) {
-        return _relayHub;
-    }
-
-    function upgradeRelayHub(address newRelayHub) public {
-        return _upgradeRelayHub(newRelayHub);
-    }
-}

contracts/mocks/GSNRecipientMock.sol

@@ -1,8 +1,10 @@
 pragma solidity ^0.5.0;
 
+import "./ContextMock.sol";
 import "../GSN/GSNRecipient.sol";
 
-contract GSNRecipientMock is GSNRecipient {
+// By inheriting from GSNRecipient, Context's internal functions are overridden automatically
+contract GSNRecipientMock is ContextMock, GSNRecipient {
     function withdrawDeposits(uint256 amount, address payable payee) public {
         _withdrawDeposits(amount, payee);
     }
@@ -22,4 +24,8 @@ contract GSNRecipientMock is GSNRecipient {
     function postRelayedCall(bytes calldata, bool, uint256, bytes32) external {
         // solhint-disable-previous-line no-empty-blocks
     }
+
+    function upgradeRelayHub(address newRelayHub) public {
+        return _upgradeRelayHub(newRelayHub);
+    }
 }

test/GSN/GSNContext.test.js

@@ -1,78 +0,0 @@
-const { BN, constants, expectEvent, expectRevert } = require('openzeppelin-test-helpers');
-const { ZERO_ADDRESS } = constants;
-const gsn = require('@openzeppelin/gsn-helpers');
-
-const GSNContextMock = artifacts.require('GSNContextMock');
-const ContextMockCaller = artifacts.require('ContextMockCaller');
-
-const { shouldBehaveLikeRegularContext } = require('./Context.behavior');
-
-contract('GSNContext', function ([_, deployer, sender, newRelayHub]) {
-  beforeEach(async function () {
-    this.context = await GSNContextMock.new();
-    this.caller = await ContextMockCaller.new();
-  });
-
-  describe('get/set RelayHub', function () {
-    const singletonRelayHub = '0xD216153c06E857cD7f72665E0aF1d7D82172F494';
-
-    it('initially returns the singleton instance address', async function () {
-      expect(await this.context.getRelayHub()).to.equal(singletonRelayHub);
-    });
-
-    it('can be upgraded to a new RelayHub', async function () {
-      const { logs } = await this.context.upgradeRelayHub(newRelayHub);
-      expectEvent.inLogs(logs, 'RelayHubChanged', { oldRelayHub: singletonRelayHub, newRelayHub });
-    });
-
-    it('cannot upgrade to the same RelayHub', async function () {
-      await expectRevert(
-        this.context.upgradeRelayHub(singletonRelayHub),
-        'GSNContext: new RelayHub is the current one'
-      );
-    });
-
-    it('cannot upgrade to the zero address', async function () {
-      await expectRevert(this.context.upgradeRelayHub(ZERO_ADDRESS), 'GSNContext: new RelayHub is the zero address');
-    });
-
-    context('with new RelayHub', function () {
-      beforeEach(async function () {
-        await this.context.upgradeRelayHub(newRelayHub);
-      });
-
-      it('returns the new instance address', async function () {
-        expect(await this.context.getRelayHub()).to.equal(newRelayHub);
-      });
-    });
-  });
-
-  context('when called directly', function () {
-    shouldBehaveLikeRegularContext(sender);
-  });
-
-  context('when receiving a relayed call', function () {
-    beforeEach(async function () {
-      await gsn.fundRecipient(web3, { recipient: this.context.address });
-    });
-
-    describe('msgSender', function () {
-      it('returns the relayed transaction original sender', async function () {
-        const { tx } = await this.context.msgSender({ from: sender, useGSN: true });
-        await expectEvent.inTransaction(tx, GSNContextMock, 'Sender', { sender });
-      });
-    });
-
-    describe('msgData', function () {
-      it('returns the relayed transaction original data', async function () {
-        const integerValue = new BN('42');
-        const stringValue = 'OpenZeppelin';
-        const callData = this.context.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI();
-
-        // The provider doesn't properly estimate gas for a relayed call, so we need to manually set a higher value
-        const { tx } = await this.context.msgData(integerValue, stringValue, { gas: 1000000, useGSN: true });
-        await expectEvent.inTransaction(tx, GSNContextMock, 'Data', { data: callData, integerValue, stringValue });
-      });
-    });
-  });
-});

test/GSN/GSNRecipient.test.js

@@ -1,23 +1,94 @@
-const { balance, ether, expectRevert } = require('openzeppelin-test-helpers');
+const { balance, BN, constants, ether, expectEvent, expectRevert } = require('openzeppelin-test-helpers');
+const { ZERO_ADDRESS } = constants;
+
 const gsn = require('@openzeppelin/gsn-helpers');
 
 const { expect } = require('chai');
 
 const GSNRecipientMock = artifacts.require('GSNRecipientMock');
+const ContextMockCaller = artifacts.require('ContextMockCaller');
+
+const { shouldBehaveLikeRegularContext } = require('./Context.behavior');
 
-contract('GSNRecipient', function ([_, payee]) {
+contract('GSNRecipient', function ([_, payee, sender, newRelayHub]) {
   beforeEach(async function () {
     this.recipient = await GSNRecipientMock.new();
   });
 
-  it('returns the RelayHub address address', async function () {
-    expect(await this.recipient.getHubAddr()).to.equal('0xD216153c06E857cD7f72665E0aF1d7D82172F494');
-  });
-
   it('returns the compatible RelayHub version', async function () {
     expect(await this.recipient.relayHubVersion()).to.equal('1.0.0');
   });
 
+  describe('get/set RelayHub', function () {
+    const singletonRelayHub = '0xD216153c06E857cD7f72665E0aF1d7D82172F494';
+
+    it('initially returns the singleton instance address', async function () {
+      expect(await this.recipient.getHubAddr()).to.equal(singletonRelayHub);
+    });
+
+    it('can be upgraded to a new RelayHub', async function () {
+      const { logs } = await this.recipient.upgradeRelayHub(newRelayHub);
+      expectEvent.inLogs(logs, 'RelayHubChanged', { oldRelayHub: singletonRelayHub, newRelayHub });
+    });
+
+    it('cannot upgrade to the same RelayHub', async function () {
+      await expectRevert(
+        this.recipient.upgradeRelayHub(singletonRelayHub),
+        'GSNRecipient: new RelayHub is the current one'
+      );
+    });
+
+    it('cannot upgrade to the zero address', async function () {
+      await expectRevert(
+        this.recipient.upgradeRelayHub(ZERO_ADDRESS), 'GSNRecipient: new RelayHub is the zero address'
+      );
+    });
+
+    context('with new RelayHub', function () {
+      beforeEach(async function () {
+        await this.recipient.upgradeRelayHub(newRelayHub);
+      });
+
+      it('returns the new instance address', async function () {
+        expect(await this.recipient.getHubAddr()).to.equal(newRelayHub);
+      });
+    });
+  });
+
+  context('when called directly', function () {
+    beforeEach(async function () {
+      this.context = this.recipient; // The Context behavior expects the contract in this.context
+      this.caller = await ContextMockCaller.new();
+    });
+
+    shouldBehaveLikeRegularContext(sender);
+  });
+
+  context('when receiving a relayed call', function () {
+    beforeEach(async function () {
+      await gsn.fundRecipient(web3, { recipient: this.recipient.address });
+    });
+
+    describe('msgSender', function () {
+      it('returns the relayed transaction original sender', async function () {
+        const { tx } = await this.recipient.msgSender({ from: sender, useGSN: true });
+        await expectEvent.inTransaction(tx, GSNRecipientMock, 'Sender', { sender });
+      });
+    });
+
+    describe('msgData', function () {
+      it('returns the relayed transaction original data', async function () {
+        const integerValue = new BN('42');
+        const stringValue = 'OpenZeppelin';
+        const callData = this.recipient.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI();
+
+        // The provider doesn't properly estimate gas for a relayed call, so we need to manually set a higher value
+        const { tx } = await this.recipient.msgData(integerValue, stringValue, { gas: 1000000, useGSN: true });
+        await expectEvent.inTransaction(tx, GSNRecipientMock, 'Data', { data: callData, integerValue, stringValue });
+      });
+    });
+  });
+
   context('with deposited funds', async function () {
     const amount = ether('1');