Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a .call wrapper with Solidity function call checks #2260

Closed
frangio opened this issue Jun 1, 2020 · 6 comments · Fixed by #2264
Closed

Add a .call wrapper with Solidity function call checks #2260

frangio opened this issue Jun 1, 2020 · 6 comments · Fixed by #2264
Labels
good first issue Low hanging fruit for new contributors to get involved!

Comments

@frangio
Copy link
Contributor

frangio commented Jun 1, 2020

Some contracts need to perform function calls that can't be expressed as Solidity functions and so must resort to low level .call. In most of these cases Solidity function call semantics are still desired but it's tricky to get all the details right. There's a couple of instances of this in this library: ERC721._checkOnERC721Received and SafeERC20.

We should provide a function that implements these details, possibly in the Address library. This function has to:

  1. Verify the target address is a contract. (Address.isContract)
  2. Use .call with a specified bytes data, and require the call to succeed, otherwise bubbling up the revert reason or using a default revert reason. (See the implementation of this in ERC721.)
  3. Return the returndata.

This function should then be used in the library in the places I linked above.

We have to come up with a name. Address.checkedCall comes to mind, but I'd like to see other options.

@nventuro nventuro added the good first issue Low hanging fruit for new contributors to get involved! label Jun 1, 2020
@julianmrodri
Copy link
Contributor

julianmrodri commented Jun 2, 2020

Other options: Address.safeCall, Address.secureCall, Address.validCall , Address.validatedCall. Or we can turn it around: Address.callWithChecks, Address.callAndCheck, Address.callWithValidation.

So the function should return the returndata from the low level call in case of success, and revert in case of failure?

@frangio
Copy link
Contributor Author

frangio commented Jun 2, 2020

Yes, exactly.

@nventuro
Copy link
Contributor

nventuro commented Jun 2, 2020

What about something like Address.functionCall()? After all, what we're doing is replicating the behavior of function calls, not Ethereum calls.

@julianmrodri
Copy link
Contributor

Address.functionCall() -> +1. I like it a lot, straight to the point.

@julianmrodri
Copy link
Contributor

julianmrodri commented Jun 4, 2020

@frangio @nventuro I'm working on this issue and I found out we may lose a bit of flexibility for setting the default revert message with this approach.

For example in the ERC721 case it currently sets revert default message ERC721: transfer to non ERC721Receiver implementer in some situations. After this change it would revert with a more generic message such as: Address: low-level call failed. This happens because handling the failed call now occurs in this generic function and not in the specific contract anymore.

Are you OK with this? Please let me know if I'm not clear enough. Also let me know if you think theres a solution to this that we can implement. Thanks!

@julianmrodri
Copy link
Contributor

@nventuro @frangio I submitted a Draft PR so you can take a look. It is only missing the Unit tests for the new function Address.functionCall which I will have shortly. Can you review if this is in the right direction? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Low hanging fruit for new contributors to get involved!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants