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

wording clarifications #20

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
38 changes: 20 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,42 +16,44 @@ We should be as specific and thorough as possible when defining our style, testi

#### 1. Names of internal functions in a library should not have an underscore prefix.

The style guide states

> Underscore Prefix for Non-external Functions and Variables

One of the motivations for this rule is that it is a helpful visual clue.
YES:

> Leading underscores allow you to immediately recognize the intent of such functions...
```solidity
Library.function()
```

We agree that a leading underscore is a useful visual clue, and this is why we oppose using them for internal library functions that can be called from other contracts. Visually, it looks wrong.
NO:

```solidity
Library._function()
```

or
If a function should never be called from another contract, it should be marked private and its name should have a leading underscore.

#### Justification
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have Justification elsewhere where we explain reasoning, so I am hesitant to mix up the format. (I used to have something like this but removed for consistency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think exceptions should require justification. This is the only exception we've listed so I would expect future ones to have justification as well, but might not be present right now


The style guide states `Underscore Prefix for Non-external Functions and Variables`. One motivation for this rule is providing a helpful visual clue:

> Leading underscores allow you to immediately recognize the intent of such functions...

We agree that a leading underscore is a useful visual clue, and this is why we oppose using them for internal library functions that can be called from other contracts. Visually, it looks wrong.

```solidity
using Library for bytes
bytes._function()
```

Note, we cannot remedy this by insisting on the use public functions. Whether a library functions are internal or external has important implications. From the [Solidity documentation](https://docs.soliditylang.org/en/latest/contracts.html#libraries)

> ... the code of internal library functions that are called from a contract and all functions called from therein will at compile time be included in the calling contract, and a regular JUMP call will be used instead of a DELEGATECALL.
Insisting on the use public functions could avoid this issue. However, we should not rely on this as a remedy as developers may prefer internal functions because they are more gas efficient to call, due to how libraries are compiled in Solidity:

Developers may prefer internal functions because they are more gas efficient to call.

If a function should never be called from another contract, it should be marked private and its name should have a leading underscore.
> ... the code of internal library functions that are called from a contract and all functions called from therein will at compile time be included in the calling contract, and a regular JUMP call will be used instead of a DELEGATECALL. ([source](https://docs.soliditylang.org/en/latest/contracts.html#libraries))

### C. Additions

#### 1. Errors

##### A. Prefer custom errors.

Custom errors are in some cases more gas efficient and allow passing useful information.
Custom errors should be used wherever possible (an exception could be introducing inconsistencies to existing code bases) - they allow for passing of useful information and are in some cases more gas efficient.

##### B. Custom error names should be CapWords style.

Expand Down Expand Up @@ -190,7 +192,7 @@ Assembly code is hard to read and audit. We should avoid it unless the gas savin

##### A. Avoid unnecessary version Pragma constraints.

While the main contracts we deploy should specify a single Solidity version, all supporting contracts and libraries should have as open a Pragma as possible. A good rule of thumb is to the next major version. For example
While the main contracts we deploy should specify a single Solidity version, all supporting contracts and libraries should have as open a Pragma as possible. A good rule of thumb is to use the next major version. For example

```solidity
pragma solidity ^0.8.0;
Expand Down Expand Up @@ -313,7 +315,7 @@ contract TransferFromTest {
}
```

#### 4. Prefer tests that test one thing.
#### 4. Write tests that test one thing.

This is generally good practice, but especially so because Forge does not give line numbers on assertion failures. This makes it hard to track down what, exactly, failed if a test has many assertions.

Expand Down Expand Up @@ -421,7 +423,7 @@ We should avoid adding to these or defining any remappings explicitly, as it mak

### D. Upgradability

#### 1. Prefer [ERC-7201](https://eips.ethereum.org/EIPS/eip-7201) "Namespaced Storage Layout" convention to avoid storage collisions.
#### 1. Use [ERC-7201](https://eips.ethereum.org/EIPS/eip-7201) "Namespaced Storage Layout" convention to avoid storage collisions.

### E. Structs

Expand Down
Loading