Standardize on Custom errors

[ControlCplusControlV]

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]

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]

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]

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]

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

[mds1]

Have we verified there are no forge issues with assertEq when used in setUp() or constructors?

[ControlCplusControlV]

I looked through open foundry issues and didn't see anything? Are there anything specific concerns you had in mind?

[mds1]

This was the original issue where assertions didn't work in setUp and I think in constructors too: foundry-rs/foundry#1291

That was a long time ago, but I'd suggest setting up a quick test to verify that the assertEq methods still cause failures when hit in both setUp and constructors, just to be safe

[ControlCplusControlV]

Re-ran your POC test

Ran 1 test suite in 124.97ms (414.42µs CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/Counter.t.sol:SetUpFailureTest
[FAIL. Reason: setup failed: assertion failed] setUp() (gas: 0)

Test now fails in the setUp process which is as expect and doesn't run the other tests, LGTM?

[ControlCplusControlV]

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