This is a guide for Coinbase engineers developing EVM-based smart contracts. We use Solidity when developing such contracts, so we call it a "Solidity Style Guide." This guide also covers development and testing practices. We are sharing this publicly in case it is useful to others.

We should be as specific and thorough as possible when defining our style, testing, and development practices. Any time we save not having to debate these things on pull requests is productive time that can go into other discussion and review. Following the style guide is evidence of care.

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.

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.

Library._function()

or

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

... 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.

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.

Custom errors are in some cases more gas efficient and allow passing useful information.

For example, InsufficientBalance.

Events should track things that happened and so should be past tense. Using past tense also helps avoid naming collisions with structs or functions.

We are aware this does not follow precedent from early ERCs, like ERC-20. However it does align with some more recent high profile Solidity, e.g. 1, 2, 3.

YES:

event OwnerUpdated(address newOwner);

NO:

event OwnerUpdate(address newOwner);

YES:

event OwnerUpdated(address newOwner);

NO:

event UpdatedOwner(address newOwner);

In short functions, named return arguments are unnecessary.

NO:

function add(uint a, uint b) public returns (uint result) {
  result = a + b;
}

Named return arguments can be helpful in functions with multiple returned values.

function validate(UserOperation calldata userOp) external returns (bytes memory context, uint256 validationData)

However, it is important to be explicit when returning early.

YES:

function validate(UserOperation calldata userOp) external returns (bytes memory context, uint256 validationData) {
  context = "";
  validationData = 1;

  if (condition) {
    return (context, validationData);
  }
}

NO:

function validate(UserOperation calldata userOp) external returns (bytes memory context, uint256 validationData) {
  context = "";
  validationData = 1;
  if (condition) {
    return;
  }
}

Passing arguments to functions, events, and errors with explicit naming is helpful for clarity, especially when the name of the variable passed does not match the parameter name.

YES:

pow({base: x, exponent: y, scalar: v})

NO:

pow(x, y, v)

Explicit naming parameters in mapping types is helpful for clarity, especially when nesting is used.

YES:

mapping(address account => mapping(address asset => uint256 amount)) public balances;

NO:

mapping(uint256 => mapping(address => uint256)) public balances;

If a function or set of functions could reasonably be defined as its own contract or as a part of a larger contract, prefer defining it as part of a larger contract. This makes the code easier to understand and audit.

Note this does not mean that we should avoid inheritance, in general. Inheritance is useful at times, most especially when building on existing, trusted contracts. For example, do not reimplement Ownable functionality to avoid inheritance. Inherit Ownable from a trusted vendor, such as OpenZeppelin or Solady.

Interfaces separate NatSpec from contract logic, requiring readers to do more work to understand the code. For this reason, they should be avoided.

Assembly code is hard to read and audit. We should avoid it unless the gas savings are very consequential, e.g. > 25%.

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

pragma solidity ^0.8.0;

Named imports help readers understand what exactly is being used and where it is originally declared.

YES:

import {Contract} from "./contract.sol"

NO:

import "./contract.sol"

For convenience, named imports do not have to be used in test files.

YES:

import {A} from './A.sol'
import {B} from './B.sol'

NO:

import {B} from './B.sol'
import {A} from './A.sol'

For example

import {Math} from '/solady/Math.sol'

import {MyHelper} from './MyHelper.sol'

In test files, imports from /test should be their own group, as well.

import {Math} from '/solady/Math.sol'

import {MyHelper} from '../src/MyHelper.sol'

import {Mock} from './mocks/Mock.sol'

Sometimes authors and readers find it helpful to comment dividers between groups of functions. This permitted, however ensure the style guide ordering of functions is still followed.

For example

/// External Functions ///

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/*                   VALIDATION OPERATIONS                    */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

ASCII art is permitted in the space between the end of the Pragmas and the beginning of the imports.

For example, ERC20.t.sol

For example,

• ERC20Test
• TransferFromTest

For example

• test_transferFrom_debitsFromAccountBalance
• test_transferFrom_debitsFromAccountBalance_whenCalledViaPermit
• test_transferFrom_reverts_whenAmountExceedsBalance

If the contract is named after a function, then function name can be omitted.

contract TransferFromTest {
  function test_debitsFromAccountBalance() ...
}

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.

YES:

function test_transferFrom_debitsFrom() {
  ...
}

function test_transferFrom_creditsTo() {  
  ...
}

function test_transferFrom_emitsCorrectly() {
  ...
}

function test_transferFrom_reverts_whenAmountExceedsBalance() {
  ...
}

NO:

function test_transferFrom_works() {
  // debits correctly
  // credits correctly
  // emits correctly
  // reverts correctly
}

Note, this does not mean a test should only ever have one assertion. Sometimes having multiple assertions is helpful for certainty on what is being tested.

function test_transferFrom_creditsTo() {
  assertEq(balanceOf(to), 0);
  ...
  assertEq(balanceOf(to), amount);
}

YES:

function test_transferFrom_creditsTo() {
  assertEq(balanceOf(to), 0);
  uint amount = 10;
  transferFrom(from, to, amount);
  assertEq(balanceOf(to), amount);
}

NO:

function test_transferFrom_creditsTo() {
  assertEq(balanceOf(to), 0);
  transferFrom(from, to, 10);
  assertEq(balanceOf(to), 10);
}

All else being equal, prefer fuzz tests.

YES:

function test_transferFrom_creditsTo(uint amount) {
  assertEq(balanceOf(to), 0);
  transferFrom(from, to, amount);
  assertEq(balanceOf(to), amount);
}

NO:

function test_transferFrom_creditsTo() {
  assertEq(balanceOf(to), 0);
  uint amount = 10;
  transferFrom(from, to, amount);
  assertEq(balanceOf(to), amount);
}

Remappings help Forge find dependencies based on import statements. Forge will automatically deduce some remappings, for example

forge-std/=lib/forge-std/src/
solmate/=lib/solmate/src/

We should avoid adding to these or defining any remappings explicitly, as it makes our project harder for others to use as a dependency. For example, if our project depends on Solmate and so does theirs, we want to avoid our project having some irregular import naming, resolved with a custom remapping, which will conflict with their import naming.

uint32 will give the contract ~82 years of validity (2^32 / (60*60*24*365)) - (2024 - 1970). If space allows, uint40 is the preferred size.

Minimally including @notice. @param and @return should be present if there are parameters or return values.

Structs can be documented with a @notice above and, if desired, @dev for each field.

/// @notice A struct describing an accounts position
struct Position {
  /// @dev The unix timestamp (seconds) of the block when the position was created.
  uint created;
  /// @dev The amount of ETH in the position 
  uint amount;
}

For easier reading, add a new line between tag types, when multiple are present and there are three or more lines.

YES:

/// @notice ...
///
/// @dev ...
/// @dev ...
/// 
/// @param ...
/// @param ...
/// 
/// @return

NO:

/// @notice ...
/// @dev ...
/// @dev ...
/// @param ...
/// @param ...
/// @return

If you using the @author tag, it should be

/// @author Coinbase

Optionally followed by a link to the public Github repository.