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 custom errors #45

Closed
wants to merge 1 commit into from
Closed

Add custom errors #45

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2021

Custom errors decrease both deploy and runtime gas costs.
Additionally all expected errors are listed in the abi file which can be used to improve the UX of contract interactions.

Introduction blog post

  • Add custom errors
  • Update tests

@ghost ghost marked this pull request as draft October 26, 2021 12:13
@CodeSandwich
Copy link
Collaborator

Thank you for the contribution! It's a great idea to switch to the modern, cheap errors.

I haven't used them before, what kind of gas saving are we talking about and in which cases? How well supported is it in the ecosystem? I'd expect Etherscan to already display proper messages when the source code is submitted. What does it look like in wallets, e.g. in MetaMask?

@ghost
Copy link
Author

ghost commented Oct 26, 2021

@CodeSandwich I did not test them with Metamask but I think they already support custom errors.

Here is an example of how a custom error could be used on the front-end:

console.log(
    "Insufficient balance for transfer. " +
    `Needed ${decoded.required.toString()} but only ` +
    `${decoded.available.toString()} available.`
);

Regarding the costs, I don't have any numbers for this contract but have a look at the following Yul code:

revert Unauthorized();

let free_mem_ptr := mload(64)
mstore(free_mem_ptr, 0x82b4290000000000000000000000000000000000000000000000000000000000)
revert(free_mem_ptr, 4)

revert("Unauthorized");

let free_mem_ptr := mload(64)
mstore(free_mem_ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(add(free_mem_ptr, 4), 32)
mstore(add(free_mem_ptr, 36), 12)
mstore(add(free_mem_ptr, 68), "Unauthorized")
revert(free_mem_ptr, 100)

As you can see, the produced Yul code for the custom error revert Unauthorized(); is much less compared to revert("Unauthorized");.

@gh0stwheel
Copy link

@byterose -- thank you for your contribution!

(CC @manuelpolzhofer)

@CodeSandwich -- a few days ago, I think you asked me to take a look at this and share my input. I wasn't familiar with these new errors either, so I did some quick reading about them. From my quick research, I don't believe that this is something that's worth switching over to at this stage of the project, given that our main goal right now is to get the MVP deployed as quickly as possible, and given that we're not sure how these errors would interact with wallets like Metamask. It's also not clear to me that there would be major gas savings, and that gas savings would only exist in corner cases in any event.

I would propose that we add this to a backlog of tasks to work on after the MVP has been deployed. At that time, if we wanted to evaluate this, I would propose that we write a few test cases to evaluate gas savings and also test how it works with Metamask.

Thoughts?

@ghost
Copy link
Author

ghost commented Oct 30, 2021

@gh0stwheel thank you for your input.

One thing to keep in mind is that a require statement uses a revert underneath. If you use a require statement with a string argument, it will revert with that string as error data. To see how this would look like at a Yul level, have a look at my previous comment. In conclusion not much would change, since both use revert underneath.

I also recommend to watch this video, custom errors are introduced at 16:50.

P.S. I did not find a way yet to use DSTest to test reverts, that's why the CI is failing as well.

@CodeSandwich
Copy link
Collaborator

Let's shelve it for now. I've opened an issue #61 to keep track of that idea.

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

Successfully merging this pull request may close these issues.

2 participants