Skip to content

Latest commit

 

History

History
314 lines (208 loc) · 12.3 KB

README.md

File metadata and controls

314 lines (208 loc) · 12.3 KB
Raw

Frax Style Guide (Work In Progress)

The intention of this style guide is first and foremost to increase readability of code and reduce the potential for bugs. Readability reduces overhead for new readers which includes colleagues and most importantly auditors. Additionally, consistency reduces cognitive overhead. When people read our code they should walk away feeling “Holy shit these guys are obsessed”. This comes with trade-offs, most obvious the speed at which code is initially written.

This style guide is meant to be collaborative and in its current state is just a first pass (with some highly opinionated items), so if you disagree please reach out or make a PR instead of just ignoring it. Good programmers are flexible and the guide is pointless if we don’t follow it.

Principles

Optimize for the reader (auditor) and fellow developers, not the writer

We explicitly optimize for the experience of the reader, not the writer. Implicitly this means that the speed of writing new code will decrease. The ultimate goal of these rules is to provide a framework that allows new readers the ability to quickly understand the codebase and to reduce the surface area for auditor focus.

Be consistent

Good engineers are flexible not dogmatic, don’t mix and match styles. Consistency reduces cognitive overhead for readers.

Repository

Use kebab-case for repository names and folder names

Example: @FraxFinance/fraxlend-dev

Use -dev suffix to indicate internal repos, public repos should have the same name without -dev

Example: Private Repo: fraxlend-dev Public Repo: fraxlend

Utilize forge + prettier as the default formatter

This repo contains a .prettierrc.json file for use with prettier. Prettier is significantly smarter and has better line-wrapping heuristics than forge

Utilize solhint as the default linter

This repo contains a .solhint.json file for use with solhint

Utilize husky and lint-staged to lint all files pre-commit

This repo has an example .lintstagedrc file and .husky folder for reference

File Names

One Contract/Interface/Library per File

Utilize an import file if you would like to group files together by namespace for easy importing

Example:

// This file aggregates the different contracts and should be named appropriately
import { SomeContract } from ".SomeContract.sol";
import { SomeContract1 } from ".SomeContract1.sol";
import { SomeContract2 } from ".SomeContract2.sol";
import { SomeContract3 } from ".SomeContract3.sol";

Use PascalCase.sol for file names containing Contracts

Use IPascalCase.sol for file names containing interfaces

File Name should exactly match the name of the contract/interface/library

Files containing tests should start with word Test and end with .t.sol

Example: TestFraxlendPairCore.t.sol

Use camelCase for files containing a group of free functions

Example: deployFunctions.sol (contains no contracts just a collection of free functions)

Root directory should contains src, test, script directories

src contains contracts and interfaces which are deployed

test contains test contracts

script contains files for scripting including repo management and deploy scripts

Misc

Use pnpm to install github dependencies

Why: submodules dont provide lockfiles or commit binding. We need to be sure that we are running/deploying the exact same code across machines.
Why: Breaking changes are common and explicitly upgrading can prevent issues across machines

Prefer node_module packages over github packages for secure code

Why: Latest code can be unstable and pnpm packages are typically release candidates

Use Proper licenses

Proprietary: // SPDX-License-Identifier: BUSL-1.1

Open and composable: TODO

Language Features

Do not rely on via-ir to solve stack-too-deep errors

Why: via-ir is slow and reduces testing speed. If reaching stack too deep your code complexity is probably too high. For src code, reduce complexity, for tests use param/return structs.

Use named input params when invoking functions

Required for functions with 2+ parameters

Example:

function fooBar(address _foo, address _bar) internal;

//Good
fooBar({ _foo: address(123), _bar: address(345)});

//Bad
fooBar(address(345), address(123));

Why:

  • Reduces potential bugs introduced during refactors which change order of functions
  • Reduces overhead for readers as code intention is more clear, eliminates need to look at function definition

Source File Structure

File Structure should be in the following order:

  1. license
  2. pragma
  3. Frax Ascii art
  4. imports
  5. ConstructorParams Struct
  6. Contract

Imports should occur in the following order:

  1. github packages
  2. node_module packages
  3. contracts
  4. interfaces
  • within each category, items should be alphabetical

Use named imports { SomeContract } from "./SomeContract.sol";

Why:

  • Forge best practices
  • Reduces the need for the reader to have additional context when coming across an identifier. Because identifiers are otherwise inherited, the location of an identifier is not known to the reader without a global search or IDE tooling
  • Solves issues with naming collisions
  • Makes compilation (and therefore testing) faster and makes verified code smaller

Call inherited contract constructors in the order they are inherited

Why: Helps solve stack-too-deep errors, consistency, reinforces order of constructor execution

Naming

Immutable and constant variables should be CONSTANT_CASE

Internal and private functions should be prepended with an underscore _someInternalFunction _somePrivateFunction

Avoid abbreviations and acronyms in variables names, prefer descriptive variable names

Code should be self-documenting. Give as descriptive a name as possible. It is far more important to make your code immediately understandable by a new reader than to optimize line wrapping. Do not use abbreviations that are ambiguous or unfamiliar to readers outside your project, and do not abbreviate by deleting letters within a word.

Allowed
errorCount No abbreviation.
dnsConnectionIndex Most people know what "DNS" stands for.
referrerUrl Ditto for "URL".
customerId "Id" is both ubiquitous and unlikely to be misunderstood.
Disallowed
n Meaningless.
nErr Ambiguous abbreviation.
nCompConns Ambiguous abbreviation.
wgcConnections Only your group knows what this stands for.
pcReader Lots of things can be abbreviated "pc".
cstmrId Deletes internal letters.
kSecondsPerDay Do not use Hungarian notation.

Method names

Method names are written in camelCase and are typically verbs or verb phrases

Example: sendMessage or stopProcess.

Enum names are written in PascalCase and should generally be singular nouns. Individual items within the enum are named in CONSTANT_CASE.

Parameter names are written in camelCase and prepended with an _ if memory or without to designate storage pointers

Example:

function sendMessage(string memory _messageBody, string storage messageHeader);

Storage variable names are written in camelCase

Local variable names are written in _camelCase and prepended with an underscore to differentiate from storage variables

Use google’s camelCase algorithm to handle acronyms/abbreviations and edge cases

Additional Info: Google camelCase Algorithm
Additional clarity: for words like iOS veFXS or gOhm, when prefix letter is single treat as one word, when prefix letters are >1 (frxETH or veFXS) treat prefix as separate word:

Prose form Correct Incorrect
veFXS veFxs veFXS
some gOhm item someGohmItem someGOHMItem
XML HTTP request xmlHttpRequest XMLHTTPRequest
iOS ios IOS
new customer ID newCustomerId newCustomerID
inner stopwatch innerStopwatch innerStopWatch
supports IPv6 on iOS? supportsIpv6OnIos supportsIPv6OnIOS
YouTube importer youTubeImporter YoutubeImporter*
gOHM ERC-20 gohmErc20 gOHMERC20
some veFXS item someVeFxsItem someVeFXSItem

Code Structure / Architecture

Be thoughtful about breaking up code and inheriting.

One pattern used in fraxlend is a separation of view helper functions and the core logic. This is a separation by threat model.

Group related code together

Be thoughtful about the order in which you would like to present information to a new reader. For internal functions used in a single place, group them next to the external function they are used in. If they are used in multiple places follow guidelines below.

Within a contract code should be in the following order:

[Open for discussion on this item]

  1. storage variables
  2. constructor
  3. internals/helpers used in multiple places
  4. external functions
  5. custom errors
  6. Events should be defined in the interface.

Explicitly assign default values to named return params for maximum readability

Empty catch blocks should have comments explaining why they are acceptable

[Important] Separate calculation and storage mutation functions when possible.

This aids in testing and separation of concerns, helps adhere to checks-effects-interactions patterns. For calculations, use storage pointers as arguments to reduce SLOADs if necessary. Allows for the creation of preview functions to create a preview of some action, this is required for compose-ability (see ERC4626 for an example of preview functions)
See: TODO: governance example in delegation

Avoid mutations when possible

Gas savings is not a good reason to mutate. Write readable code then optimize gas when necessary. Makes debugging easier and code understanding more clear.

Avoid modifiers unless you need to take action both before and after function invocation

Create internal functions in the form of _requireCondition() which will revert on failure. Why: This allows optimizer to reduce bytecode more efficiently, works better with IDE and analysis tools, and increases code intention without needing to jump to modifier definition. Also order of execution is explicit. [Exception: when you need to run code both BEFORE and AFTER the wrapped function]

    // BAD
    modifier TimelockOnly() {
        if (msg.sender != TIMELOCK_ADDRESS) revert()
    }

    function WithdrawFees() TimelockOnly {
        // some code
    }

    // GOOD
    function _requireSenderIsTimelock() internal {
        if (msg.sender != TIMELOCK_ADDRESS) revert Unauthorized();
    }

    function WithdrawFees() {
        _requireSenderIsTimelock();
        // some code
    }

Use custom errors over require statements, saves both byte code and gas

    // Bad
    function _requireSenderIsTimelock() internal {
        require(msg.sender = TIMELOCK_ADDRESS, "unauthorized");
    }

    // Good
    function _requireSenderIsTimelock() internal {
        if (msg.sender != TIMELOCK_ADDRESS) revert Unauthorized();
    }

Dont return expressions, instead assign named return param variable

Tests

Separate scenario setup and test functions.

Why: Tests for impure functions should be tested under a variety of state conditions. Separating test assertions and scenario setups allows for re-using code across multiple scenarios. See

Write tests in a way that you can run the test suite against deployed contracts afterwards

See: Frax Governance Test Example
See: Fraxlend Oracles Test Example

Utilize fuzz tests

Separate helpers to separate file

Create invariant assertions that can be used in multiple places

Dont sleep on echidna

Pre-Audit Checklist

  • 100% test coverage
  • Adheres to style guide
  • Example deployment instructions
  • Slither and Mythril instructions and remaining issues addressed
  • Video walkthrough of each external function flow
  • Access control documentation
  • Threat modeling documentation
  • Heavy commenting aimed at new reader
  • No TODO, console.log, or commented out code