search

ethereum-optimism / design-docs

MIT License
26 stars 20 forks source link

Standardize on Custom errors #143

Closed ControlCplusControlV closed 3 weeks ago

ControlCplusControlV commented 4 weeks ago

Description

Set a clear standard on how to handle errors for OP Smart Contracts, using only custom errors and reverts.

Tests

No tests needed

Additional context

Carried over from discord, but the idea is to stop bikeshedding around using both require with error strings and custom errors and just standardized on custom errors, which I lay out in the doc. There was some concern around using require in scripts + tests, but after some discussion on discord we agreed assertEq should be able to cover that in most cases

Metadata optimism issue #12478

tynes commented 3 weeks ago

There will be many opinions on this, it is important to not bikeshed so not going to die on a hill over this but I do think there is value in common error types like Unauthorized, under this we will get SystemConfigUnauthorized and OptimismPortalUnauthorized which is a different 4 byte selector for the same thing, I suppose that is fine given that the ABIs are uploaded to 4byte directories.

I also prefer to have a _ in between the contract name and the error name, making it a bit easier to read. I do not think that we have any conditional logic based on returndata so there should be no internal backwards compatibility concerns. We have changed the returndata so many times at this point and nobody has complained, so i doubt that people are currently consuming the revert errors. The only time that I have every observed a revert personally was a deposit oog, with no good error message.

smartcontracts commented 3 weeks ago

I do think there is value in common error types like Unauthorized

One thing to note is that providing the contract name in the error means it's immediately obvious where an error was thrown.

I also prefer to have a _ in between the contract name and the error name, making it a bit easier to read.

Yep totally down with this, easier to read.

ControlCplusControlV commented 3 weeks ago

I will pushback against common errors solely because it is nice to not have to run everything with -vvv in order to determine where an occurred, since a contract name in 90% of cases will make it immediately obvious where failure occurred

tynes commented 3 weeks ago

Ok your arguments against common errors are good and I am on board with this standard

ControlCplusControlV commented 3 weeks ago

Discussed in meeting #150 , consensus is approved but PR's should be done file by file with prior discussion with the protocol team