From ddb602fba07de264491dd55b275badb2041df70f Mon Sep 17 00:00:00 2001
From: Julian R <56316686+julianmrodri@users.noreply.github.com>
Date: Thu, 4 Jun 2020 02:58:51 -0300
Subject: [PATCH 01/15] feat: add wrapper function for low level calls

---
 contracts/mocks/AddressImpl.sol     |  4 ++++
 contracts/token/ERC20/SafeERC20.sol | 15 +++------------
 contracts/token/ERC721/ERC721.sol   | 19 +++----------------
 contracts/utils/Address.sol         | 27 +++++++++++++++++++++++++++
 test/token/ERC20/SafeERC20.test.js  |  2 +-
 test/token/ERC721/ERC721.test.js    |  4 ++--
 test/utils/Address.test.js          |  4 ++++
 7 files changed, 44 insertions(+), 31 deletions(-)

diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol
index 0bcfcbb85fc..7bfcb7d8258 100644
--- a/contracts/mocks/AddressImpl.sol
+++ b/contracts/mocks/AddressImpl.sol
@@ -13,6 +13,10 @@ contract AddressImpl {
         Address.sendValue(receiver, amount);
     }
 
+    function functionCall(address target, bytes calldata data) external {
+        Address.functionCall(target, data);
+    }
+
     // sendValue's tests require the contract to hold Ether
     receive () external payable { }
 }
diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol
index a815e3a3c4b..92142ae7ea7 100644
--- a/contracts/token/ERC20/SafeERC20.sol
+++ b/contracts/token/ERC20/SafeERC20.sol
@@ -56,19 +56,10 @@ library SafeERC20 {
      */
     function _callOptionalReturn(IERC20 token, bytes memory data) private {
         // We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since
-        // we're implementing it ourselves.
-
-        // A Solidity high level call has three parts:
-        //  1. The target address is checked to verify it contains contract code
-        //  2. The call itself is made, and success asserted
-        //  3. The return value is decoded, which in turn checks the size of the returned data.
-        // solhint-disable-next-line max-line-length
-        require(address(token).isContract(), "SafeERC20: call to non-contract");
-
-        // solhint-disable-next-line avoid-low-level-calls
-        (bool success, bytes memory returndata) = address(token).call(data);
-        require(success, "SafeERC20: low-level call failed");
+        // we're implementing it ourselves. We use {Address.functionCall} to perform this call, which verifies that
+        // the target address contains contract code and also asserts for success in the low-level call.
 
+        bytes memory returndata = address(token).functionCall(data);
         if (returndata.length > 0) { // Return data is optional
             // solhint-disable-next-line max-line-length
             require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");
diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol
index 0e9653cef1e..1499aab4317 100644
--- a/contracts/token/ERC721/ERC721.sol
+++ b/contracts/token/ERC721/ERC721.sol
@@ -497,28 +497,15 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
         if (!to.isContract()) {
             return true;
         }
-        // solhint-disable-next-line avoid-low-level-calls
-        (bool success, bytes memory returndata) = to.call(abi.encodeWithSelector(
+        bytes memory returndata = to.functionCall(abi.encodeWithSelector(
             IERC721Receiver(to).onERC721Received.selector,
             _msgSender(),
             from,
             tokenId,
             _data
         ));
-        if (!success) {
-            if (returndata.length > 0) {
-                // solhint-disable-next-line no-inline-assembly
-                assembly {
-                    let returndata_size := mload(returndata)
-                    revert(add(32, returndata), returndata_size)
-                }
-            } else {
-                revert("ERC721: transfer to non ERC721Receiver implementer");
-            }
-        } else {
-            bytes4 retval = abi.decode(returndata, (bytes4));
-            return (retval == _ERC721_RECEIVED);
-        }
+        bytes4 retval = abi.decode(returndata, (bytes4));
+        return (retval == _ERC721_RECEIVED);
     }
 
     function _approve(address to, uint256 tokenId) private {
diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index 2c54fe44968..1f326505cbc 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -57,4 +57,31 @@ library Address {
         (bool success, ) = recipient.call{ value: amount }("");
         require(success, "Address: unable to send value, recipient may have reverted");
     }
+
+    /**
+     * @dev Replacement for Solidity's low-level `call`: performs a low-level
+     * call with `data` to the target address `target`. Returns the `returndata`
+     * provided by the low-level call.
+     *
+     * The call is not executed if the target address is not a contract.
+     */
+    function functionCall(address target, bytes memory data) internal returns (bytes memory) {
+        require(isContract(target), "Address: call to non-contract");
+
+        // solhint-disable-next-line avoid-low-level-calls
+        (bool success, bytes memory returndata) = target.call(data);
+        if (!success) {
+            if (returndata.length > 0) {
+                // solhint-disable-next-line no-inline-assembly
+                assembly {
+                    let returndata_size := mload(returndata)
+                    revert(add(32, returndata), returndata_size)
+                }
+            } else {
+                revert("Address: low-level call failed");
+            }
+        } else {
+            return returndata;
+        }      
+    }
 }
diff --git a/test/token/ERC20/SafeERC20.test.js b/test/token/ERC20/SafeERC20.test.js
index c50496ab7a0..11eac3100ef 100644
--- a/test/token/ERC20/SafeERC20.test.js
+++ b/test/token/ERC20/SafeERC20.test.js
@@ -15,7 +15,7 @@ describe('SafeERC20', function () {
       this.wrapper = await SafeERC20Wrapper.new(hasNoCode);
     });
 
-    shouldRevertOnAllCalls('SafeERC20: call to non-contract');
+    shouldRevertOnAllCalls('Address: call to non-contract');
   });
 
   describe('with token that returns false on all calls', function () {
diff --git a/test/token/ERC721/ERC721.test.js b/test/token/ERC721/ERC721.test.js
index 10285322694..38a2ea90ca8 100644
--- a/test/token/ERC721/ERC721.test.js
+++ b/test/token/ERC721/ERC721.test.js
@@ -404,7 +404,7 @@ describe('ERC721', function () {
             const nonReceiver = this.token;
             await expectRevert(
               this.token.safeTransferFrom(owner, nonReceiver.address, tokenId, { from: owner }),
-              'ERC721: transfer to non ERC721Receiver implementer'
+              'Address: low-level call failed'
             );
           });
         });
@@ -463,7 +463,7 @@ describe('ERC721', function () {
             const nonReceiver = this.token;
             await expectRevert(
               this.token.safeMint(nonReceiver.address, tokenId),
-              'ERC721: transfer to non ERC721Receiver implementer'
+              'Address: low-level call failed'
             );
           });
         });
diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index 1229302caa2..31e762b27ec 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -90,4 +90,8 @@ describe('Address', function () {
       });
     });
   });
+
+  describe('functionCall', function () {
+    // TODO
+  });
 });

From dde2161956cd076bd4a25c825a59a796a7b34860 Mon Sep 17 00:00:00 2001
From: Julian R <56316686+julianmrodri@users.noreply.github.com>
Date: Sun, 7 Jun 2020 22:43:31 -0300
Subject: [PATCH 02/15] add error message parameter

---
 contracts/token/ERC721/ERC721.sol |  2 +-
 contracts/utils/Address.sol       | 13 ++++++++++++-
 test/token/ERC721/ERC721.test.js  |  4 ++--
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol
index 1499aab4317..5c7150dca3d 100644
--- a/contracts/token/ERC721/ERC721.sol
+++ b/contracts/token/ERC721/ERC721.sol
@@ -503,7 +503,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Enumerable
             from,
             tokenId,
             _data
-        ));
+        ), "ERC721: transfer to non ERC721Receiver implementer");
         bytes4 retval = abi.decode(returndata, (bytes4));
         return (retval == _ERC721_RECEIVED);
     }
diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index 1f326505cbc..1b66b212fff 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -66,6 +66,17 @@ library Address {
      * The call is not executed if the target address is not a contract.
      */
     function functionCall(address target, bytes memory data) internal returns (bytes memory) {
+      return functionCall(target, data, "Address: low-level call failed");
+    }
+
+    /**
+     * @dev Replacement for Solidity's low-level `call`: performs a low-level
+     * call with `data` to the target address `target`. Returns the `returndata`
+     * provided by the low-level call. Uses `errorMessage` as default revert message.
+     
+     * The call is not executed if the target address is not a contract.
+     */
+    function functionCall(address target, bytes memory data, string memory errorMessage) internal returns (bytes memory) {
         require(isContract(target), "Address: call to non-contract");
 
         // solhint-disable-next-line avoid-low-level-calls
@@ -78,7 +89,7 @@ library Address {
                     revert(add(32, returndata), returndata_size)
                 }
             } else {
-                revert("Address: low-level call failed");
+                revert(errorMessage);
             }
         } else {
             return returndata;
diff --git a/test/token/ERC721/ERC721.test.js b/test/token/ERC721/ERC721.test.js
index 38a2ea90ca8..10285322694 100644
--- a/test/token/ERC721/ERC721.test.js
+++ b/test/token/ERC721/ERC721.test.js
@@ -404,7 +404,7 @@ describe('ERC721', function () {
             const nonReceiver = this.token;
             await expectRevert(
               this.token.safeTransferFrom(owner, nonReceiver.address, tokenId, { from: owner }),
-              'Address: low-level call failed'
+              'ERC721: transfer to non ERC721Receiver implementer'
             );
           });
         });
@@ -463,7 +463,7 @@ describe('ERC721', function () {
             const nonReceiver = this.token;
             await expectRevert(
               this.token.safeMint(nonReceiver.address, tokenId),
-              'Address: low-level call failed'
+              'ERC721: transfer to non ERC721Receiver implementer'
             );
           });
         });

From 5fa623824e6c679a83b53011c9332c267cca70a3 Mon Sep 17 00:00:00 2001
From: Julian R <56316686+julianmrodri@users.noreply.github.com>
Date: Mon, 8 Jun 2020 02:25:00 -0300
Subject: [PATCH 03/15] adding unit tests and required mocks

---
 contracts/mocks/AddressImpl.sol      |  4 +-
 contracts/mocks/CallReceiverMock.sol | 16 ++++++++
 test/utils/Address.test.js           | 57 ++++++++++++++++++++++++++--
 3 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100644 contracts/mocks/CallReceiverMock.sol

diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol
index 7bfcb7d8258..b298c52d778 100644
--- a/contracts/mocks/AddressImpl.sol
+++ b/contracts/mocks/AddressImpl.sol
@@ -13,8 +13,8 @@ contract AddressImpl {
         Address.sendValue(receiver, amount);
     }
 
-    function functionCall(address target, bytes calldata data) external {
-        Address.functionCall(target, data);
+    function functionCall(address target, bytes calldata data) external returns (bytes memory) {
+        return Address.functionCall(target, data);
     }
 
     // sendValue's tests require the contract to hold Ether
diff --git a/contracts/mocks/CallReceiverMock.sol b/contracts/mocks/CallReceiverMock.sol
new file mode 100644
index 00000000000..88028cd7426
--- /dev/null
+++ b/contracts/mocks/CallReceiverMock.sol
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.6.0;
+
+contract CallReceiverMock {
+  
+    event MockFunctionCalled();
+    
+    function mockFunction() public {
+        emit MockFunctionCalled();
+    }
+
+    function mockFunctionReverts() public pure {
+        revert();
+    }
+}
diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index 31e762b27ec..8d9861485da 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -1,10 +1,11 @@
-const { accounts, contract } = require('@openzeppelin/test-environment');
+const { accounts, contract, web3 } = require('@openzeppelin/test-environment');
 
-const { balance, ether, expectRevert, send } = require('@openzeppelin/test-helpers');
+const { balance, ether, expectRevert, send, expectEvent } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
 
 const AddressImpl = contract.fromArtifact('AddressImpl');
 const EtherReceiver = contract.fromArtifact('EtherReceiverMock');
+const CallReceiver = contract.fromArtifact('CallReceiverMock');
 
 describe('Address', function () {
   const [ recipient, other ] = accounts;
@@ -92,6 +93,56 @@ describe('Address', function () {
   });
 
   describe('functionCall', function () {
-    // TODO
+    beforeEach(async function () {
+      this.contractRecipient = await CallReceiver.new();
+    });
+
+    context('with valid contract receiver', function () {
+      it('calls the requested function', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunction',
+          type: 'function',
+          inputs: [],
+        }, []);
+        const { tx } = await this.mock.functionCall(this.contractRecipient.address, abiEncodedCall);
+        await expectEvent.inTransaction(tx, CallReceiver, 'MockFunctionCalled');
+      });
+
+      it('reverts when the called function reverts', async function () {
+        const data = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunctionReverts',
+          type: 'function',
+          inputs: [],
+        }, []);
+        await expectRevert(
+          this.mock.functionCall(this.contractRecipient.address, data),
+          'Address: low-level call failed'
+        );
+      });
+
+      it('reverts when function does not exist', async function () {
+        const data = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunctionDoesNotExist',
+          type: 'function',
+          inputs: [],
+        }, []);
+        await expectRevert(
+          this.mock.functionCall(this.contractRecipient.address, data),
+          'Address: low-level call failed'
+        );
+      });
+    });
+
+    context('with non-contract receiver', function () {
+      it('reverts when address is not a contract', async function () {
+        const [ recipient ] = accounts;
+        const data = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunction',
+          type: 'function',
+          inputs: [],
+        }, []);
+        await expectRevert(this.mock.functionCall(recipient, data), 'Address: call to non-contract');
+      });
+    });
   });
 });

From 54784ffc5df0785bfbb3d6e8f2566956ad87b6cf Mon Sep 17 00:00:00 2001
From: Julian R <56316686+julianmrodri@users.noreply.github.com>
Date: Mon, 8 Jun 2020 02:40:31 -0300
Subject: [PATCH 04/15] implement error message on SafeERC20

---
 contracts/token/ERC20/SafeERC20.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/token/ERC20/SafeERC20.sol b/contracts/token/ERC20/SafeERC20.sol
index 92142ae7ea7..fcc301b79d4 100644
--- a/contracts/token/ERC20/SafeERC20.sol
+++ b/contracts/token/ERC20/SafeERC20.sol
@@ -59,7 +59,7 @@ library SafeERC20 {
         // we're implementing it ourselves. We use {Address.functionCall} to perform this call, which verifies that
         // the target address contains contract code and also asserts for success in the low-level call.
 
-        bytes memory returndata = address(token).functionCall(data);
+        bytes memory returndata = address(token).functionCall(data, "SafeERC20: low-level call failed");
         if (returndata.length > 0) { // Return data is optional
             // solhint-disable-next-line max-line-length
             require(abi.decode(returndata, (bool)), "SafeERC20: ERC20 operation did not succeed");

From 0a0ad5e1265d36d406745f213c44741dfb40d742 Mon Sep 17 00:00:00 2001
From: Julian R <56316686+julianmrodri@users.noreply.github.com>
Date: Mon, 8 Jun 2020 02:54:05 -0300
Subject: [PATCH 05/15] fixed variable name in tests

---
 test/utils/Address.test.js | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index 8d9861485da..54de1fef4cf 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -109,25 +109,25 @@ describe('Address', function () {
       });
 
       it('reverts when the called function reverts', async function () {
-        const data = web3.eth.abi.encodeFunctionCall({
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
           name: 'mockFunctionReverts',
           type: 'function',
           inputs: [],
         }, []);
         await expectRevert(
-          this.mock.functionCall(this.contractRecipient.address, data),
+          this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
           'Address: low-level call failed'
         );
       });
 
       it('reverts when function does not exist', async function () {
-        const data = web3.eth.abi.encodeFunctionCall({
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
           name: 'mockFunctionDoesNotExist',
           type: 'function',
           inputs: [],
         }, []);
         await expectRevert(
-          this.mock.functionCall(this.contractRecipient.address, data),
+          this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
           'Address: low-level call failed'
         );
       });
@@ -136,12 +136,12 @@ describe('Address', function () {
     context('with non-contract receiver', function () {
       it('reverts when address is not a contract', async function () {
         const [ recipient ] = accounts;
-        const data = web3.eth.abi.encodeFunctionCall({
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
           name: 'mockFunction',
           type: 'function',
           inputs: [],
         }, []);
-        await expectRevert(this.mock.functionCall(recipient, data), 'Address: call to non-contract');
+        await expectRevert(this.mock.functionCall(recipient, abiEncodedCall), 'Address: call to non-contract');
       });
     });
   });

From 6f494961156ce9a775751335ddc67ee059363b22 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 10 Jun 2020 16:57:32 -0300
Subject: [PATCH 06/15] Add missing tests

---
 contracts/mocks/AddressImpl.sol      |  8 +++-
 contracts/mocks/CallReceiverMock.sol | 26 +++++++++++--
 test/utils/Address.test.js           | 56 +++++++++++++++++++++++++---
 3 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol
index b298c52d778..2dd2c7ccb89 100644
--- a/contracts/mocks/AddressImpl.sol
+++ b/contracts/mocks/AddressImpl.sol
@@ -5,6 +5,8 @@ pragma solidity ^0.6.0;
 import "../utils/Address.sol";
 
 contract AddressImpl {
+    event CallReturnValue(string data);
+
     function isContract(address account) external view returns (bool) {
         return Address.isContract(account);
     }
@@ -13,8 +15,10 @@ contract AddressImpl {
         Address.sendValue(receiver, amount);
     }
 
-    function functionCall(address target, bytes calldata data) external returns (bytes memory) {
-        return Address.functionCall(target, data);
+    function functionCall(address target, bytes calldata data) external {
+        bytes memory returnData = Address.functionCall(target, data);
+
+        emit CallReturnValue(abi.decode(returnData, (string)));
     }
 
     // sendValue's tests require the contract to hold Ether
diff --git a/contracts/mocks/CallReceiverMock.sol b/contracts/mocks/CallReceiverMock.sol
index 88028cd7426..4ece16cfe90 100644
--- a/contracts/mocks/CallReceiverMock.sol
+++ b/contracts/mocks/CallReceiverMock.sol
@@ -3,14 +3,32 @@
 pragma solidity ^0.6.0;
 
 contract CallReceiverMock {
-  
+
     event MockFunctionCalled();
-    
-    function mockFunction() public {
+
+    uint256[] private _array;
+
+    function mockFunction() public returns (string memory) {
         emit MockFunctionCalled();
+
+        return "0x1234";
     }
 
-    function mockFunctionReverts() public pure {
+    function mockFunctionRevertsNoReason() public pure {
         revert();
     }
+
+    function mockFunctionRevertsReason() public pure {
+        revert("CallReceiverMock: reverting");
+    }
+
+    function mockFunctionThrows() public pure {
+        assert(false);
+    }
+
+    function mockFunctionOutOfGas() public {
+        for (uint256 i = 0; ; ++i) {
+            _array.push(i);
+        }
+    }
 }
diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index 54de1fef4cf..c2bd6edbc39 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -5,7 +5,7 @@ const { expect } = require('chai');
 
 const AddressImpl = contract.fromArtifact('AddressImpl');
 const EtherReceiver = contract.fromArtifact('EtherReceiverMock');
-const CallReceiver = contract.fromArtifact('CallReceiverMock');
+const CallReceiverMock = contract.fromArtifact('CallReceiverMock');
 
 describe('Address', function () {
   const [ recipient, other ] = accounts;
@@ -94,7 +94,7 @@ describe('Address', function () {
 
   describe('functionCall', function () {
     beforeEach(async function () {
-      this.contractRecipient = await CallReceiver.new();
+      this.contractRecipient = await CallReceiverMock.new();
     });
 
     context('with valid contract receiver', function () {
@@ -104,16 +104,59 @@ describe('Address', function () {
           type: 'function',
           inputs: [],
         }, []);
-        const { tx } = await this.mock.functionCall(this.contractRecipient.address, abiEncodedCall);
-        await expectEvent.inTransaction(tx, CallReceiver, 'MockFunctionCalled');
+
+        const receipt = await this.mock.functionCall(this.contractRecipient.address, abiEncodedCall);
+
+        expectEvent(receipt, 'CallReturnValue', { data: '0x1234' });
+        await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
+      });
+
+      it('reverts when the called function reverts with no reason', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunctionRevertsNoReason',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        await expectRevert(
+          this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
+          'Address: low-level call failed'
+        );
+      });
+
+      it('reverts when the called function reverts, bubbling up the revert reason', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunctionRevertsReason',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        await expectRevert(
+          this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
+          'CallReceiverMock: reverting'
+        );
+      });
+
+      it('reverts when the called function runs out of gas', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunctionOutOfGas',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        await expectRevert(
+          this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
+          'Address: low-level call failed'
+        );
       });
 
-      it('reverts when the called function reverts', async function () {
+      it('reverts when the called function throws', async function () {
         const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
-          name: 'mockFunctionReverts',
+          name: 'mockFunctionThrows',
           type: 'function',
           inputs: [],
         }, []);
+
         await expectRevert(
           this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
           'Address: low-level call failed'
@@ -126,6 +169,7 @@ describe('Address', function () {
           type: 'function',
           inputs: [],
         }, []);
+
         await expectRevert(
           this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
           'Address: low-level call failed'

From ae8484c80d3ec81935c963fcad6b58e5aebf8842 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 10 Jun 2020 16:57:41 -0300
Subject: [PATCH 07/15] Improve docs.

---
 contracts/utils/Address.sol | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index 1b66b212fff..f94fa1b8297 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -59,30 +59,38 @@ library Address {
     }
 
     /**
-     * @dev Replacement for Solidity's low-level `call`: performs a low-level
-     * call with `data` to the target address `target`. Returns the `returndata`
-     * provided by the low-level call.
+     * @dev Performs a Solidity function call using a low level `call`. A
+     * plain`call` is an unsafe replacement for a function call: use this
+     * function instead.
      *
-     * The call is not executed if the target address is not a contract.
+     * If `target` reverts with a revert reason, it is bubbled up by this
+     * function (like regular Solidity function calls).
+     *
+     * Requirements:
+     *
+     * - `target` must be a contract.
+     * - calling `target` with `data` must not revert.
      */
     function functionCall(address target, bytes memory data) internal returns (bytes memory) {
       return functionCall(target, data, "Address: low-level call failed");
     }
 
     /**
-     * @dev Replacement for Solidity's low-level `call`: performs a low-level
-     * call with `data` to the target address `target`. Returns the `returndata`
-     * provided by the low-level call. Uses `errorMessage` as default revert message.
-     
-     * The call is not executed if the target address is not a contract.
+     * @dev Same as {Address-functionCall-address-bytes-}, but with
+     * `errorMessage` as a custom revert reason when `target` reverts.
      */
     function functionCall(address target, bytes memory data, string memory errorMessage) internal returns (bytes memory) {
         require(isContract(target), "Address: call to non-contract");
 
         // solhint-disable-next-line avoid-low-level-calls
         (bool success, bytes memory returndata) = target.call(data);
-        if (!success) {
+        if (success) {
+            return returndata;
+        } else {
+            // Look for revert reason and bubble it up if present
             if (returndata.length > 0) {
+                // The easiest way to bubble the revert reason is using memory via assembly
+
                 // solhint-disable-next-line no-inline-assembly
                 assembly {
                     let returndata_size := mload(returndata)
@@ -91,8 +99,6 @@ library Address {
             } else {
                 revert(errorMessage);
             }
-        } else {
-            return returndata;
-        }      
+        }
     }
 }

From 9154a5ff63e24dff974874181dd4c9a29051c76c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 10 Jun 2020 19:32:13 -0300
Subject: [PATCH 08/15] Add functionCallWithValue

---
 contracts/mocks/AddressImpl.sol      |  6 +++
 contracts/mocks/CallReceiverMock.sol | 16 ++++---
 contracts/utils/Address.sol          | 22 +++++++++-
 test/utils/Address.test.js           | 66 ++++++++++++++++++++++++++++
 4 files changed, 103 insertions(+), 7 deletions(-)

diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol
index 2dd2c7ccb89..20b48d3b1d3 100644
--- a/contracts/mocks/AddressImpl.sol
+++ b/contracts/mocks/AddressImpl.sol
@@ -21,6 +21,12 @@ contract AddressImpl {
         emit CallReturnValue(abi.decode(returnData, (string)));
     }
 
+    function functionCallWithValue(address target, bytes calldata data, uint256 value) external {
+        bytes memory returnData = Address.functionCallWithValue(target, data, value);
+
+        emit CallReturnValue(abi.decode(returnData, (string)));
+    }
+
     // sendValue's tests require the contract to hold Ether
     receive () external payable { }
 }
diff --git a/contracts/mocks/CallReceiverMock.sol b/contracts/mocks/CallReceiverMock.sol
index 4ece16cfe90..2e3297617b9 100644
--- a/contracts/mocks/CallReceiverMock.sol
+++ b/contracts/mocks/CallReceiverMock.sol
@@ -8,25 +8,31 @@ contract CallReceiverMock {
 
     uint256[] private _array;
 
-    function mockFunction() public returns (string memory) {
+    function mockFunction() public payable returns (string memory) {
         emit MockFunctionCalled();
 
         return "0x1234";
     }
 
-    function mockFunctionRevertsNoReason() public pure {
+    function mockFunctionNonPayable() public returns (string memory) {
+        emit MockFunctionCalled();
+
+        return "0x1234";
+    }
+
+    function mockFunctionRevertsNoReason() public payable {
         revert();
     }
 
-    function mockFunctionRevertsReason() public pure {
+    function mockFunctionRevertsReason() public payable {
         revert("CallReceiverMock: reverting");
     }
 
-    function mockFunctionThrows() public pure {
+    function mockFunctionThrows() public payable {
         assert(false);
     }
 
-    function mockFunctionOutOfGas() public {
+    function mockFunctionOutOfGas() public payable {
         for (uint256 i = 0; ; ++i) {
             _array.push(i);
         }
diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index f94fa1b8297..bf7d1d90eca 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -72,7 +72,7 @@ library Address {
      * - calling `target` with `data` must not revert.
      */
     function functionCall(address target, bytes memory data) internal returns (bytes memory) {
-      return functionCall(target, data, "Address: low-level call failed");
+      return _functionCallWithValue(target, data, 0, "Address: low-level call failed");
     }
 
     /**
@@ -80,10 +80,28 @@ library Address {
      * `errorMessage` as a custom revert reason when `target` reverts.
      */
     function functionCall(address target, bytes memory data, string memory errorMessage) internal returns (bytes memory) {
+        return _functionCallWithValue(target, data, 0, errorMessage);
+    }
+
+    function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
+        return _functionCallWithValue(target, data, value, "Address: low-level call with value failed");
+    }
+
+    /**
+     * @dev Same as {Address-functionCallWithValue-address-bytes-uint256-}, but
+     * with `errorMessage` as a custom revert reason when `target` reverts.
+     */
+    function functionCallWithValue(address target, bytes memory data, uint256 value, string memory errorMessage) internal returns (bytes memory) {
+        return _functionCallWithValue(target, data, value, errorMessage);
+    }
+
+    function _functionCallWithValue(address target, bytes memory data, uint256 weiValue, string memory errorMessage) private returns (bytes memory) {
         require(isContract(target), "Address: call to non-contract");
 
+        require(address(this).balance >= weiValue, "Address: insufficient balance for call");
+
         // solhint-disable-next-line avoid-low-level-calls
-        (bool success, bytes memory returndata) = target.call(data);
+        (bool success, bytes memory returndata) = target.call{ value: weiValue }(data);
         if (success) {
             return returndata;
         } else {
diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index c2bd6edbc39..c8450e4c8d2 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -189,4 +189,70 @@ describe('Address', function () {
       });
     });
   });
+
+  describe('functionCallWithValue', function () {
+    beforeEach(async function () {
+      this.contractRecipient = await CallReceiverMock.new();
+    });
+
+    context('with zero value', function () {
+      it('calls the requested function', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunction',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        const receipt = await this.mock.functionCallWithValue(this.contractRecipient.address, abiEncodedCall, 0);
+
+        expectEvent(receipt, 'CallReturnValue', { data: '0x1234' });
+        await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
+      });
+    });
+
+    context('with non-zero value', function () {
+      const amount = ether('1.2');
+
+      it('reverts if insufficient sender balance', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunction',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        await expectRevert(
+          this.mock.functionCallWithValue(this.contractRecipient.address, abiEncodedCall, amount),
+          'Address: insufficient balance for call'
+        );
+      });
+
+      it('calls the requested function', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunction',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        await send.ether(other, this.mock.address, amount);
+        const receipt = await this.mock.functionCallWithValue(this.contractRecipient.address, abiEncodedCall, amount);
+
+        expectEvent(receipt, 'CallReturnValue', { data: '0x1234' });
+        await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
+      });
+
+      it('reverts when calling non-payable functions', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunctionNonPayable',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        await send.ether(other, this.mock.address, amount);
+        await expectRevert(
+          this.mock.functionCallWithValue(this.contractRecipient.address, abiEncodedCall, amount),
+          'Address: low-level call with value failed'
+        );
+      });
+    });
+  });
 });

From 65701ae81f7552b5b3f89680f59088e0830a1684 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Wed, 10 Jun 2020 20:56:35 -0300
Subject: [PATCH 09/15] Add functionCallWithValue

---
 contracts/mocks/AddressImpl.sol |  2 +-
 contracts/utils/Address.sol     | 14 ++++++++++++++
 test/utils/Address.test.js      | 18 +++++++++++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/contracts/mocks/AddressImpl.sol b/contracts/mocks/AddressImpl.sol
index 20b48d3b1d3..19dcb15b440 100644
--- a/contracts/mocks/AddressImpl.sol
+++ b/contracts/mocks/AddressImpl.sol
@@ -21,7 +21,7 @@ contract AddressImpl {
         emit CallReturnValue(abi.decode(returnData, (string)));
     }
 
-    function functionCallWithValue(address target, bytes calldata data, uint256 value) external {
+    function functionCallWithValue(address target, bytes calldata data, uint256 value) external payable {
         bytes memory returnData = Address.functionCallWithValue(target, data, value);
 
         emit CallReturnValue(abi.decode(returnData, (string)));
diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index bf7d1d90eca..faaeae8ea0f 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -83,6 +83,20 @@ library Address {
         return _functionCallWithValue(target, data, 0, errorMessage);
     }
 
+    /**
+     * @dev Performs a Solidity function call using a low level `call`,
+     * transferring `value` wei. A plain`call` is an unsafe replacement for a
+     * function call: use this function instead.
+     *
+     * If `target` reverts with a revert reason, it is bubbled up by this
+     * function (like regular Solidity function calls).
+     *
+     * Requirements:
+     *
+     * - `target` must be a contract.
+     * - the calling contract must have an ETH balance of at least `value`.
+     * - calling `target` with `data` must not revert.
+     */
     function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
         return _functionCallWithValue(target, data, value, "Address: low-level call with value failed");
     }
diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index c8450e4c8d2..d772eaa86e9 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -226,7 +226,7 @@ describe('Address', function () {
         );
       });
 
-      it('calls the requested function', async function () {
+      it('calls the requested function with existing value', async function () {
         const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
           name: 'mockFunction',
           type: 'function',
@@ -240,6 +240,22 @@ describe('Address', function () {
         await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
       });
 
+      it('calls the requested function with transaction funds', async function () {
+        const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
+          name: 'mockFunction',
+          type: 'function',
+          inputs: [],
+        }, []);
+
+        expect(await balance.current(this.mock.address)).to.be.bignumber.equal('0');
+        const receipt = await this.mock.functionCallWithValue(
+          this.contractRecipient.address, abiEncodedCall, amount, { from: other, value: amount }
+        );
+
+        expectEvent(receipt, 'CallReturnValue', { data: '0x1234' });
+        await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
+      });
+
       it('reverts when calling non-payable functions', async function () {
         const abiEncodedCall = web3.eth.abi.encodeFunctionCall({
           name: 'mockFunctionNonPayable',

From cb390e836cf0a56a22dec484aec0742c1d39a50f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 11 Jun 2020 11:11:11 -0300
Subject: [PATCH 10/15] Skip balance check on non-value functionCall variants

---
 contracts/utils/Address.sol | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index faaeae8ea0f..f1abea09621 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -106,14 +106,13 @@ library Address {
      * with `errorMessage` as a custom revert reason when `target` reverts.
      */
     function functionCallWithValue(address target, bytes memory data, uint256 value, string memory errorMessage) internal returns (bytes memory) {
+        require(address(this).balance >= balance, "Address: insufficient balance for call");
         return _functionCallWithValue(target, data, value, errorMessage);
     }
 
     function _functionCallWithValue(address target, bytes memory data, uint256 weiValue, string memory errorMessage) private returns (bytes memory) {
         require(isContract(target), "Address: call to non-contract");
 
-        require(address(this).balance >= weiValue, "Address: insufficient balance for call");
-
         // solhint-disable-next-line avoid-low-level-calls
         (bool success, bytes memory returndata) = target.call{ value: weiValue }(data);
         if (success) {

From 008a9629e449e92edad4153cf8b01f01a7a4be43 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 11 Jun 2020 11:14:02 -0300
Subject: [PATCH 11/15] Increase out of gas test timeout

---
 test/utils/Address.test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index d772eaa86e9..8a165a2a0ac 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -148,7 +148,7 @@ describe('Address', function () {
           this.mock.functionCall(this.contractRecipient.address, abiEncodedCall),
           'Address: low-level call failed'
         );
-      });
+      }).timeout(5000);
 
       it('reverts when the called function throws', async function () {
         const abiEncodedCall = web3.eth.abi.encodeFunctionCall({

From d2c376fe35627c53999265ded08f725bcfc9d6fc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 11 Jun 2020 11:29:22 -0300
Subject: [PATCH 12/15] Fix compile errors

---
 contracts/utils/Address.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index f1abea09621..aac7a1e20a4 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -72,7 +72,7 @@ library Address {
      * - calling `target` with `data` must not revert.
      */
     function functionCall(address target, bytes memory data) internal returns (bytes memory) {
-      return _functionCallWithValue(target, data, 0, "Address: low-level call failed");
+      return functionCall(target, data, "Address: low-level call failed");
     }
 
     /**
@@ -98,7 +98,7 @@ library Address {
      * - calling `target` with `data` must not revert.
      */
     function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
-        return _functionCallWithValue(target, data, value, "Address: low-level call with value failed");
+        return functionCallWithValue(target, data, value, "Address: low-level call with value failed");
     }
 
     /**
@@ -106,7 +106,7 @@ library Address {
      * with `errorMessage` as a custom revert reason when `target` reverts.
      */
     function functionCallWithValue(address target, bytes memory data, uint256 value, string memory errorMessage) internal returns (bytes memory) {
-        require(address(this).balance >= balance, "Address: insufficient balance for call");
+        require(address(this).balance >= value, "Address: insufficient balance for call");
         return _functionCallWithValue(target, data, value, errorMessage);
     }
 

From e303cce52c7fbcbe738c621e3639130e87b3f327 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 11 Jun 2020 14:30:11 -0300
Subject: [PATCH 13/15] Apply suggestions from code review

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
---
 contracts/utils/Address.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol
index aac7a1e20a4..1a96adae229 100644
--- a/contracts/utils/Address.sol
+++ b/contracts/utils/Address.sol
@@ -77,7 +77,7 @@ library Address {
 
     /**
      * @dev Same as {Address-functionCall-address-bytes-}, but with
-     * `errorMessage` as a custom revert reason when `target` reverts.
+     * `errorMessage` as a fallback revert reason when `target` reverts.
      */
     function functionCall(address target, bytes memory data, string memory errorMessage) internal returns (bytes memory) {
         return _functionCallWithValue(target, data, 0, errorMessage);
@@ -103,7 +103,7 @@ library Address {
 
     /**
      * @dev Same as {Address-functionCallWithValue-address-bytes-uint256-}, but
-     * with `errorMessage` as a custom revert reason when `target` reverts.
+     * with `errorMessage` as a fallback revert reason when `target` reverts.
      */
     function functionCallWithValue(address target, bytes memory data, uint256 value, string memory errorMessage) internal returns (bytes memory) {
         require(address(this).balance >= value, "Address: insufficient balance for call");

From f10c9af2954b20aaba87fe56bf86df71629e0aa8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 11 Jun 2020 15:51:22 -0300
Subject: [PATCH 14/15] Add missing tests

---
 test/utils/Address.test.js | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js
index 8a165a2a0ac..8c08442874f 100644
--- a/test/utils/Address.test.js
+++ b/test/utils/Address.test.js
@@ -233,9 +233,13 @@ describe('Address', function () {
           inputs: [],
         }, []);
 
+        const tracker = await balance.tracker(this.contractRecipient.address);
+
         await send.ether(other, this.mock.address, amount);
         const receipt = await this.mock.functionCallWithValue(this.contractRecipient.address, abiEncodedCall, amount);
 
+        expect(await tracker.delta()).to.be.bignumber.equal(amount);
+
         expectEvent(receipt, 'CallReturnValue', { data: '0x1234' });
         await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
       });
@@ -247,11 +251,15 @@ describe('Address', function () {
           inputs: [],
         }, []);
 
+        const tracker = await balance.tracker(this.contractRecipient.address);
+
         expect(await balance.current(this.mock.address)).to.be.bignumber.equal('0');
         const receipt = await this.mock.functionCallWithValue(
           this.contractRecipient.address, abiEncodedCall, amount, { from: other, value: amount }
         );
 
+        expect(await tracker.delta()).to.be.bignumber.equal(amount);
+
         expectEvent(receipt, 'CallReturnValue', { data: '0x1234' });
         await expectEvent.inTransaction(receipt.tx, CallReceiverMock, 'MockFunctionCalled');
       });

From 4cc61abd7d4d5df6f0d896df979df8d024b55235 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 11 Jun 2020 16:22:29 -0300
Subject: [PATCH 15/15] Add changelog entry

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 690074f961f..e6076d09b37 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### New features
  * `SafeCast`: added functions to downcast signed integers (e.g. `toInt32`), improving usability of `SignedSafeMath`. ([#2243](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2243))
+ * `functionCall`: new helpers that replicate Solidity's function call semantics, reducing the need to rely on `call`. ([#2264](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2264))
 
 ### Improvements
  * `ReentrancyGuard`: reduced overhead of using the `nonReentrant` modifier. ([#2171](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2171))