Zondax / filecoin-solidity

Filecoin Solidity API Library
Apache License 2.0
94 stars 43 forks source link

./utils/Actor.sol error handling #280

Closed snissn closed 1 year ago

snissn commented 1 year ago

Error handling is done via

require(success == true, CALL_ERROR_MESSAGE); where CALL_ERROR_MESSAGE is a constant. This swallows any potential error messages that were propogated from the delegated contract call. I suggest changing the require check to something like

require(success == true, data); to ensure the error message gets bubbled up properly.

Note may be preferable to change success == true to just checking the bool success

:link: zboto Link

emmanuelm41 commented 1 year ago

@snissn when success is false, do we have an error message on data? If that is the case, we will apply the change as it will be more insightful for devs!

raulk commented 1 year ago

@snissn what do you think about using custom error types here? https://blog.soliditylang.org/2021/04/21/custom-errors/ They are not yet supported in require, but it sounds like the benefits may be worth it anyway:

Currently, there is no convenient way to catch errors in Solidity, but this is planned, and progress can be tracked in issue #11278. Also, require(condition, "error message") should be translated to if (!condition) revert CustomError().

raulk commented 1 year ago

I personally feel uneasy about dumping unwrapped bytes on require.

snissn commented 1 year ago

@emmanuelm41 Here's an example of propagating errors in solidity https://gist.github.com/snissn/9553d86da644d63749a062d7dfc11aa8 (shout out to chat gpt for helping me write this contract yesterday 😄 ) remix.ethereum.org shows me the error here when i run it:

image

@raulk defintely defer to you and others on the content of the error messages and how to handle them. Big picture my thinking is if there's an error on the fvm side, it would be nice as a user of the solidity filecoin library to see that error if they're relevant and actionable AND there seemed to be a place in the filecoin library that was swallowing the error that could potentially throw/escalate it in when it reverts

emmanuelm41 commented 1 year ago

@snissn the question here is what actors return when the call is not successful. I see we can propagate them, but I am not sure what actors actually return today

snissn commented 1 year ago

From a conversation with @stebalien there is not really an error code that the FVM propogats to the EVM that is worth exposing to the user.

That said we can add custom errors to our contract to improve usability of the api

This is a new-ish solidity feature! (April 2021)