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.

Sign up for GitHub

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

Proposal to move types and errors files to dedicated folders #145

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Proposal to move types and errors files to dedicated folders #145

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbb4c42
Proposal to move types and errors files to dedicated folders
AmadiMichael Oct 29, 2024
ba32233
add possible alternative
AmadiMichael Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions protocol/types-and-error-folders.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Proposal to move types and errors files to dedicated folders

## Context

The OP Stack contracts in some instances define errors and custom types in a separate file from the contract(s) that use them. This is common among contracts that are part of a common folder e.g L1/, L2/ etc which might have a common error or/and types file where they import the needed errors and types from. This has the advantage of re-usage of errors and types across contracts without duplicating code, ease of modifiability and extensibility and makes it easy to compare errors and types used by different contracts.

## Problem Statement

These error and types files are most times located in a libraries or lib folder e.g `libraries/` `L1/libraries/`, `L2/libraries/` but these are not actual libraries and so do not belong in the libraries folder. This can also make it hard to find at first, especially to a developer newly exploring the codebase.

## Proposed Solution

The proposed solution is to move these error and types files to dedicated folders e.g `L1/errors/`, `L1/types/`, `L2/errors/`, `L2/types/` etc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider how this relates to #143

I think there may be a future where errors are always defined in the contracts themselves, especially since #143 suggests having the error format be ContractName_SomeError.

Copy link
Contributor

@maurelian maurelian Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the principle here. There's a reason we wanted error strings to be prefixed with the contract name, and I find generic Unauthorized errors less helpful unless you're printing the trace (which requires more careful re

That said, I do like the idea of consolidating errors into a few locations, so what we could do is:

error Unauthorized(string location);

// and called as: 
revert Unauthorized(type(SomeContract).name);

// this would be nice too, but for some reason is failing for me in `chisel`: 
revert Unauthorized(type(this).name);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, can you bring this up on #143?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For how many errors do you think we could actually consolidate like that? I do like this syntax but if its just like 3 errors or so I think it might be fine to duplicate, but will continue discussion on #143

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree here. If it's just a few errors then I don't think it's worth it. Having a single unified standard is valuable.


## Alternatives Considered

No alternatives considered.
smartcontracts marked this conversation as resolved.
Show resolved Hide resolved

## Risks, Uncertainties and Considerations

### Consideration

- Should the dedicated folders be located at the `src/` level or within the same folder as the contracts that use them e.g `L1/`?