Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

./utils/Actor.sol error handling #280

Closed
snissn opened this issue Feb 21, 2023 · 6 comments · Fixed by #307
Closed

./utils/Actor.sol error handling #280

snissn opened this issue Feb 21, 2023 · 6 comments · Fixed by #307
Assignees

Comments

@snissn
Copy link
Contributor

snissn commented Feb 21, 2023

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

🔗 zboto Link

@emmanuelm41
Copy link
Member

emmanuelm41 commented Feb 22, 2023

@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
Copy link
Contributor

raulk commented Feb 22, 2023

@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
Copy link
Contributor

raulk commented Feb 22, 2023

I personally feel uneasy about dumping unwrapped bytes on require.

@snissn
Copy link
Contributor Author

snissn commented Feb 22, 2023

@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
Copy link
Member

emmanuelm41 commented Feb 22, 2023

@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
Copy link
Contributor Author

snissn commented Feb 23, 2023

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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants