deprecate + unit test for verifyCallResultFromTarget

Author: Amxx

contracts/utils/Address.sol

@@ -130,21 +130,24 @@ library Address {
      * @dev Tool to verify that a low level call to smart-contract was successful, and reverts if the target
      * was not a contract or bubbling up the revert reason (falling back to {Errors.FailedCall}) in case
      * of an unsuccessful call.
+     *
+     * NOTE: This function is DEPRECATED and may be remove in the next major release.
      */
     function verifyCallResultFromTarget(
         address target,
         bool success,
         bytes memory returndata
     ) internal view returns (bytes memory) {
-        if (!success) {
-            _revert(returndata);
-        } else {
-            // only check if target is a contract if the call was successful and the return data is empty
-            // otherwise we already know that it was a contract
-            if (returndata.length == 0 && target.code.length == 0) {
-                revert AddressEmptyCode(target);
-            }
+        // only check if target is a contract if the call was successful and the return data is empty
+        // otherwise we already know that it was a contract
+        if (success && (returndata.length > 0 || target.code.length > 0)) {
             return returndata;
+        } else if (success) {
+            revert AddressEmptyCode(target);
+        } else if (returndata.length > 0) {
+            LowLevelCall.bubbleRevert(returndata);
+        } else {
+            revert Errors.FailedCall();
         }
     }
 
@@ -153,19 +156,9 @@ library Address {
      * revert reason or with a default {Errors.FailedCall} error.
      */
     function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) {
-        if (!success) {
-            _revert(returndata);
-        } else {
+        if (success) {
             return returndata;
-        }
-    }
-
-    /**
-     * @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedCall}.
-     */
-    function _revert(bytes memory returndata) private pure {
-        // Look for revert reason and bubble it up if present
-        if (returndata.length > 0) {
+        } else if (returndata.length > 0) {
             LowLevelCall.bubbleRevert(returndata);
         } else {
             revert Errors.FailedCall();

test/utils/Address.test.js

@@ -5,6 +5,9 @@ const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic');
 
 const coder = ethers.AbiCoder.defaultAbiCoder();
 
+const fakeContract = { interface: ethers.Interface.from(['error SomeCustomErrorWithoutArgs()']) };
+const returndata = fakeContract.interface.encodeErrorResult('SomeCustomErrorWithoutArgs');
+
 async function fixture() {
   const [recipient, other] = await ethers.getSigners();
 
@@ -274,8 +277,56 @@ describe('Address', function () {
 
   describe('verifyCallResult', function () {
     it('returns returndata on success', async function () {
-      const returndata = '0x123abc';
-      expect(await this.mock.$verifyCallResult(true, returndata)).to.equal(returndata);
+      await expect(this.mock.$verifyCallResult(true, returndata)).to.eventually.equal(returndata);
+    });
+
+    it('bubble returndata on failure', async function () {
+      await expect(this.mock.$verifyCallResult(false, returndata)).to.be.revertedWithCustomError(
+        fakeContract,
+        'SomeCustomErrorWithoutArgs',
+      );
+    });
+
+    it('standard error on failure without returndata', async function () {
+      await expect(this.mock.$verifyCallResult(false, '0x')).to.be.revertedWithCustomError(this.mock, 'FailedCall');
+    });
+  });
+
+  describe('verifyCallResultFromTarget', function () {
+    it('success with non-empty returndata', async function () {
+      await expect(this.mock.$verifyCallResultFromTarget(this.mock, true, returndata)).to.eventually.equal(returndata);
+      await expect(this.mock.$verifyCallResultFromTarget(this.recipient, true, returndata)).to.eventually.equal(
+        returndata,
+      );
+    });
+
+    it('success with empty returndata', async function () {
+      await expect(this.mock.$verifyCallResultFromTarget(this.mock, true, '0x')).to.eventually.equal('0x');
+      await expect(this.mock.$verifyCallResultFromTarget(this.recipient, true, '0x'))
+        .to.be.revertedWithCustomError(this.mock, 'AddressEmptyCode')
+        .withArgs(this.recipient);
+    });
+
+    it('failure with non-empty returndata', async function () {
+      await expect(this.mock.$verifyCallResultFromTarget(this.mock, false, returndata)).to.revertedWithCustomError(
+        fakeContract,
+        'SomeCustomErrorWithoutArgs',
+      );
+      await expect(this.mock.$verifyCallResultFromTarget(this.recipient, false, returndata)).to.revertedWithCustomError(
+        fakeContract,
+        'SomeCustomErrorWithoutArgs',
+      );
+    });
+
+    it('failure with empty returndata', async function () {
+      await expect(this.mock.$verifyCallResultFromTarget(this.mock, false, '0x')).to.be.revertedWithCustomError(
+        this.mock,
+        'FailedCall',
+      );
+      await expect(this.mock.$verifyCallResultFromTarget(this.recipient, false, '0x')).to.be.revertedWithCustomError(
+        this.mock,
+        'FailedCall',
+      );
     });
   });
 });