feat: add wrapper function for low level calls

[julianmrodri]

#2260

Fixes #2260

This PR creates a function Address.functionCall which can be used to wrap low level calls that need to be performed on other Smart contracts. .

It also modifies the ERC721.sol and SafeERC20.solcontracts to use this new function instead of performing directly the low level call. Tests for these contracts needed to be updated to match the new revert messages in certain situations.

[frangio]

This is looking great!

[julianmrodri]

Thanks! I think I might need some guidance on the Tests to include. Which kind of calls should I mock? Do you have any preference on what contract to use and what samples to use for the Unit tests?

[nventuro]

We might want to also add a value field for ETH transfers.

Thanks! I think I might need some guidance on the Tests to include.

Off the top of my head, interesting scenarios are the following (thought we might not need to test them all):

• call to a contract that implements the called function and returns some data
• call to a contract that implements the called function but reverts
• call to a contract that implements the called function but runs out of gas
• call to a contract that doesn't implement the function (and reverts)
• call to a non-contract

If we add a value field, we might also want to test functions that are not payable.

[julianmrodri]

@nventuro @frangio I've added the error message parameter so now we have the two options for functionCall (with or without the custom message). I also added some of the tests.

I would like you to review the TESTS to see if this is the correct direction. I had to make too many decisions and is better for you to validate if this is OK. Feel free to let me know if this is not the correct way of doing this!

So far I covered:

• Call to a contract that implements the called function successfully
• Call to a contract that implements the called function but reverts
• Call to a contract that doesn't implement the function (and reverts)
• Call to a non-contract

I was not able to test the case of a function that returns data with this approach. It is quite challenging as I get the Transaction info and not the returned value, to eventually decode. Do you know how this can be achieved? Thanks!

[frangio]

The code itself looks good.

[nventuro]

By the way I asked the Solidity dev team on gitter if they think our checks are correct, and they gave us green light:

nventuro
hello! I am trying to replicate Solidity function call semantics using call, and wanted to double-check >I'm not missing anything. This is what I'm currently doing:

check using extcodehash that the recipient account has code, revert otherwise
perform the call
if success, abi.decode the returndata
if failure, check the returndata for a revert reason (lenght > 0). if available, revert with that, >otherwise revert with a generic reason

Is that all?

chriseth
@nventuro looks good!

[nventuro]

@julianmrodri we're looking into publishing the first release candidate of v3.1 today, and want to include this helper in that release. If you don't mind, I'll go ahead and continue working on this branch so we can merge this PR. I'm letting you know so we don't both end up working on the same thing :)

[julianmrodri]

@nventuro Sure please go ahead! Sorry not to be able to complete it before, some work duties came up quite some busy days.

[nventuro]

@julianmrodri no need to apologize! Your help is greatly appreciated :)

[nventuro]

For some reason the links are not working (see https://deploy-preview-2264--openzeppelin-contracts-docs.netlify.app/contracts/3.x/api/utils#Address-functionCall-address-bytes-string-).

[julianmrodri]

Wow you took this to the next level. Awesome!

[frangio]

Ship it!

[frangio]

😍

[julianmrodri]

Congrats guys! Awesome job. Glad to be able to be part of this. 👍

[nventuro]

Thanks a lot to you both!