Write Foundry Guide and update Solidity Style Guide (#15199)

* write guide * fix wrong opts and prettier comment * add section on using correct foundry version
smartcontractkit · Nov 18, 2024 · d7d1750 · d7d1750
1 parent 6f95b09
commit d7d1750
Show file tree

Hide file tree

Showing 2 changed files with 266 additions and 22 deletions.
diff --git a/contracts/FOUNDRY_GUIDE.md b/contracts/FOUNDRY_GUIDE.md
@@ -0,0 +1,246 @@
+# Foundry Guide
+
+We lock Foundry to a specific version in the `GNUmakefile`.
+To ensure you have the correct local version run `make foundry`.
+When you see formatting or gas differences between local and CI, it often means there is a version mismatch.
+We use a locked version to avoid formatting or gas changes that suddenly pop up in CI when breaking changes are pushed to the nightly Foundry feed.
+
+
+## How to start a new Foundry project
+
+There are several files to modify when starting a new Solidity project.
+Everything starts with a foundry profile in `contracts/foundry.toml`,
+this profile name will be used in most of the other files.
+We will assume the profile is called `newproject`.
+
+The foundry profile should look similar to this.
+
+```toml
+[profile.newproject]
+solc_version = '0.8.24'
+src = 'src/v0.8/newproject'
+test = 'src/v0.8/newproject/test'
+optimizer_runs = 1_000_000
+evm_version = 'paris'
+```
+
+After that, we have to enable CI by editing the following files.
+
+- `.github/CODEOWNERS`
+  - Add `newproject` in three places.
+    - `/contracts/**/*newproject* @smartcontractkit/newproject`
+    - `/contracts/src/v0.8/*newproject* @smartcontractkit/newproject`
+    - `/core/gethwrappers/*newproject* @smartcontractkit/newproject`
+  - Look at the file layout for the correct positions for each of these lines. Please keep the ordering alphabetical. 
+- `.github/workflows/solidity-foundry.yml`
+  - Add `newproject` to the `Define test matrix` section.
+    - Set the min coverage >=98%.
+    - Enable run-gas-snapshot.
+    - Enable run-forge-fmt.
+  - Add `newproject` to the `Checkout the repo` step.
+- `.github/workflows/solidity-hardhat.yml`
+  - Add `newproject` to the ignored list to avoid hardhat CI running for `newproject` changes.
+- `contracts/GNUmakefile`
+  - Add `newproject` to the ALL_FOUNDRY_PRODUCTS list in alphabetical order.
+- `contracts/.prettierignore`
+  - Add `src/v0.8/newproject/**` .
+
+To enable geth wrapper generation, you will also have to create the following files.
+
+- `contracts/scripts`
+  - Create a file called `native_solc_compile_all_newproject`.
+    - See example below.
+- `core/gethwrappers`
+  - Create a folder `newproject`.
+  - It's easiest to copy another projects folder and replace the contracts in `go_generate.go` with your own contracts.
+    - `ccip` is a good version to copy.
+    - Remove the contents of the `generated` folder.
+    - Remove the contents of the `generated-wrapper-dependency-versions-do-not-edit.txt` file.
+    - Remove the contents of the `mocks` folder.
+- If you need mocks, define them in `.mockery.yaml`.
+
+```bash
+#!/usr/bin/env bash
+
+set -e
+
+echo " ┌──────────────────────────────────────────────┐"
+echo " │       Compiling Newproject contracts...      │"
+echo " └──────────────────────────────────────────────┘"
+
+SOLC_VERSION="0.8.24"
+OPTIMIZE_RUNS=1000000
+# Optional optimizer run overrides. Please remove if not used
+OPTIMIZE_RUNS_SINGLE_CONTRACT=5000
+
+
+SCRIPTPATH="$( cd "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
+python3 -m pip install --require-hashes -r "$SCRIPTPATH"/requirements.txt
+solc-select install $SOLC_VERSION
+solc-select use $SOLC_VERSION
+export SOLC_VERSION=$SOLC_VERSION
+
+ROOT="$( cd "$(dirname "$0")" >/dev/null 2>&1 ; cd ../../ && pwd -P )"
+
+compileContract () {
+  local contract
+  contract=$(basename "$1" ".sol")
+
+  local optimize_runs=$OPTIMIZE_RUNS
+
+  case $1 in
+    "newproject/SingleContract.sol")
+      optimize_runs=$OPTIMIZE_RUNS_SINGLE_CONTRACT
+      ;;
+  esac
+
+  solc --overwrite --optimize --optimize-runs $optimize_runs --metadata-hash none \
+      -o "$ROOT"/contracts/solc/v$SOLC_VERSION/"$contract" \
+      --abi --bin --allow-paths "$ROOT"/contracts/src/v0.8 \
+      --bin-runtime --hashes --metadata --metadata-literal --combined-json abi,hashes,metadata,srcmap,srcmap-runtime \
+      --evm-version paris \
+      "$ROOT"/contracts/src/v0.8/"$1"
+}
+
+compileContract newproject/SingleContract.sol
+compileContract newproject/OtherContract.sol
+
+```
+
+You should now have a fully set-up project with CI enabled.
+Create a PR that introduces this setup without adding all the project's Solidity complexity, ideally before you start.
+This is important
+because the people approving the PR for this CI are different people from the ones approving the Solidity code.
+
+## Testing with Foundry
+
+We aim for (near) 100% line coverage.
+Line coverage can sometimes be misleading though, so you should also look at the branch coverage. 
+The CI will only take line coverage into account, which means branch coverage spot checks are highly recommended.
+Setting the line coverage requirement to ~100% means you will almost guarantee all branches are also taken.
+
+We have a strict layout and naming convention for Foundry tests. 
+This is to ensure consistency within the Chainlink codebases
+and make it easier for developers to work on different projects.
+If your Foundry project does not yet follow the structures described below, please consider refactoring it.
+The test naming structure is especially important as CI depends on it for its snapshot generation.
+
+
+### Test file layout
+
+Each foundry project has its own folder in the appropriate Solidity version folder. Within this folder there is a `test`
+folder that contains all tests. This test folder mirrors the non-test folder structure with the exception that for each 
+contract to be tested, there is a folder with that contract's name. Within that folder, there is a test file for each
+function that is tested and optionally a setup file which is shared between the function tests. Each file has a single
+contract with the name `<Contract>_<function>` e.g. `contract OnRamp_getFee is OnRampSetup`. 
+
+Consider the following folder structure.
+```
+├── Router.sol
+├── FeeQuoter.sol
+├── onRamp
+│   ├── OnRamp.sol
+│   └── AnotherOnRamp.sol
+```
+
+The folder including tests would look like this.
+
+```
+├── Router.sol
+├── FeeQuoter.sol
+├── onRamp
+│   ├── OnRamp.sol
+│   └── AnotherOnRamp.sol
+├── test
+│   ├── Router
+│   │   ├── Router.ccipSend.t.sol
+│   │   ├── Router.recoverTokens.t.sol
+│   │   ├── RouterSetup.t.sol
+│   │   └── ....
+│   ├── FeeQuoter
+│   │   ├── FeeQuoter.onReport.t.sol
+│   │   ├── FeeQuoter.updatePrices.t.sol
+│   │   └── ....
+│   ├── onRamp
+│   │   ├── OnRamp
+│   │   │   ├── OnRamp.constructor.t.sol
+│   │   │   ├── OnRamp.getFee.t.sol
+│   │   │   └── ....
+│   │   ├── AnotherOnRamp
+│   │   │   ├── AnotherOnRamp.constructor.t.sol
+│   │   │   ├── AnotherOnRamp.getFee.t.sol
+│   │   │   └── ....
+```
+
+### Test naming
+
+Tests are named according to the following format:
+
+```
+test_FunctionName_Description for standard tests.
+test_FunctionName_RevertWhen_Condition for tests expecting a revert.
+testFuzz_FunctionName_Description for fuzz tests.
+testFork_FunctionName_Description for tests that fork from a network.
+```
+
+Including the function name first will group tests for the same function together in the gas snapshot. Using this format
+will automatically exclude fuzz, fork and reverting tests from the gas snapshot. This leads to a less flaky snapshot
+with fewer merge conflicts.
+
+Examples of correct test names for a function `getFee` are:
+
+```
+test_getFee - the base success case
+test_getFee_MultipleFeeTokens - another success case with a specific scenario
+test_getFee_RevertWhen_CursedByRMN - getFee reverts when it's cursed by the RMN. The error name should be used as condition when there is a single tests that checks for it
+testFuzz_getFee_OnlyFeeTokens - a fuzz test that asserts that only fee tokens are used
+testFork_getFee_UniswapV3MainnetFee - a fork test that uses Uniswap V3 on mainnet to get the fee
+```
+
+
+### What to test
+
+Foundry unit tests should cover at least the following
+
+- The happy path
+- All emitted events.
+  - Use `vm.expectEmit()`.
+  - Since all state updates should emit an event, this implicitly means we test all state updates.
+- All revert reasons.
+  - Use `vm.expectRevert(...)`.
+
+Consider if a fuzz test makes sense.
+It often doesn't, but when it does, it can be very powerful. 
+Fork tests can be considered when the code relies on existing contracts or their state. 
+Focus on unit tests before exploring more advanced testing.
+
+## Best practices
+
+Check out the official [Foundry best practices section](https://book.getfoundry.sh/tutorials/best-practices).
+
+- There should be no code between `vm.expectEmit`/`vm.expectRevert` and the function call.
+  - Test setup should be done before the `vm.expect` call.
+- Set the block number and timestamp in `foundry.toml`.
+  - It is preferred to set these values to some reasonable value close to reality.
+  - There are already globally applied values in the `foundry.toml` file in this repo.
+- Reference errors and events from the original contracts, do not duplicate them.
+- Prefer `makeAddr("string describing the contract");` over `address(12345);`.
+- Pin the fork test block number to cache the results of the RPC.
+- If you see something being done in existing code, that doesn't mean it is automatically correct. 
+  - This document will evolve over time, and often it won't make sense to go back and refactor an entire codebase when our preferences change.
+
+
+## Tips and tricks
+
+- Use `make snapshot` to generate the correct snapshot for the selected Foundry profile.
+  - Use `make snapshot-diff` to see the diff between the local snapshot and your latest changes.
+- use `make wrappers` to generate the gethwrappers for the selected Foundry profile.
+- Use `vm.recordLogs();` to record all logs emitted
+  - Use `vm.getRecordedLogs()` to get the logs emitted. 
+  - This way you can assert that a log was *not* emitted.
+- Run `forge coverage --report lcov` to output code coverage
+  - This can be rendered as inline coverage using e.g. Coverage Gutters for VSCode
+- You can provide inline config for fuzz/invariant tests
+- You can find the function selectors for a given function or error using `cast sig <FUNC_SIG>`
+  - Run `forge selectors list` to see the entire list of selectors split by the contract name.
+
diff --git a/contracts/STYLE_GUIDE.md b/contracts/STYLE_GUIDE.md
@@ -1,16 +1,23 @@
 # Structure
 
 This guide is split into two sections: [Guidelines](#guidelines) and [Rules](#rules).
-Guidelines are recommendations that should be followed but are hard to enforce in an automated way.
+Guidelines are recommendations that should be followed, but are hard to enforce in an automated way.
 Rules are all enforced through CI, this can be through Solhint rules or other tools.
 
 ## Background
 
-Our starting point is the [official Solidity Style Guide](https://docs.soliditylang.org/en/v0.8.21/style-guide.html) and [ConsenSys's Secure Development practices](https://consensys.github.io/smart-contract-best-practices/), but we deviate in some ways. We lean heavily on [Prettier](https://github.com/smartcontractkit/chainlink/blob/develop/contracts/.prettierrc) for formatting, and if you have to set up a new Solidity project we recommend starting with [our prettier config](https://github.com/smartcontractkit/chainlink/blob/develop/contracts/.prettierrc). We are trying to automate as much of this style guide with Solhint as possible.
+Our starting point is the [official Solidity Style Guide](https://docs.soliditylang.org/en/v0.8.21/style-guide.html) and
+[ConsenSys's Secure Development practices](https://consensys.github.io/smart-contract-best-practices/), 
+but we deviate in some ways.
+We are trying to automate as much of this style guide with Solhint as possible.
 
-This guide is not meant to be applied retroactively. There is no need to rewrite existing code to adhere to this guide, and when making (small) changes in existing files, it is not required to adhere to this guide if it conflicts with other practices in that existing file. Consistency is preferred.
+This guide is not meant to be applied retroactively.
+There is no need to rewrite existing code to adhere to this guide.
+When making (small) changes in existing files,
+it is not required to adhere to this guide if it conflicts with other practices in that existing file.
+Consistency is preferred.
 
-We will be looking into `forge fmt`, but for now, we still use `prettier`.
+We use `forge fmt` for all new projects, but some older ones still rely on `prettier`.
 
 
 # <a name="guidelines"></a>Guidelines
@@ -113,7 +120,7 @@ struct FeeTokenConfigArgs {
 
 - Events should only be triggered on state changes. If the value is set but not changed, we prefer avoiding a log emission indicating a change. (e.g. Either do not emit a log, or name the event `ConfigSet` instead of `ConfigUpdated`.)
 - Events should be emitted for all state changes, not emitting should be an exception
-- When possible event names should correspond to the method they are in or the action that is being taken. Events preferably follow the format <subject><actionPerformed>, where the action performed is the past tense of the imperative verb in the method name.  e.g. calling `setConfig` should emit an event called `ConfigSet`, not `ConfigUpdated` in a method named `setConfig`.
+- When possible, event names should correspond to the method they are in or the action that is being taken. Events preferably follow the format <subject><actionPerformed>, where the action performed is the past tense of the imperative verb in the method name.  e.g. calling `setConfig` should emit an event called `ConfigSet`, not `ConfigUpdated` in a method named `setConfig`.
 
 
 ### Expose Errors
@@ -125,7 +132,7 @@ It is common to call a contract and then check the call succeeded:
 require(success, "Contract call failed");
 ```
 
-While this may look descriptive it swallows the error. Instead, bubble up the error:
+While this may look descriptive, it swallows the error. Instead, bubble up the error:
 
 ```solidity
 bool success;
@@ -160,7 +167,7 @@ The original error will not be human-readable in an off-chain explorer because i
 ## Dependencies
 
 - Prefer not reinventing the wheel, especially if there is an Openzeppelin wheel.
-- The `shared` folder can be treated as a first-party dependency and it is recommended to check if some functionality might already be in there before either writing it yourself or adding a third party dependency.
+- The `shared` folder can be treated as a first-party dependency, and it is recommended to check if some functionality might already be in there before either writing it yourself or adding a third party dependency.
 - When we have reinvented the wheel already (like with ownership), it is OK to keep using these contracts. If there are clear benefits of using another standard like OZ, we can deprecate the custom implementation and start using the new standard in all new projects. Migration will not be required unless there are serious issues with the old implementation.
 - When the decision is made to use a new standard, it is no longer allowed to use the old standard for new projects.
 
@@ -191,28 +198,19 @@ The original error will not be human-readable in an off-chain explorer because i
 - Golf your code. Make it cheap, within reason.
   - Focus on the hot path
 - Most of the cost of executing Solidity is related to reading/writing storage
+  - Pack your structs and top level contract storage
 - Calling other contracts will also be costly
 - Common types to safely use are
   - uint40 for timestamps (or uint32 if you really need the space)
   - uint96 for LINK, as there are only 1b LINK tokens
 - prefer `++i` over `i++`
+- Avoid `unchecked` 
+- Only use `assembly` when there is no option to do something in Solidity or when it saves a significant amount of gas over the alternative implementation.
 - If you’re unsure about golfing, ask in the #tech-solidity channel
 
 ## Testing
 
-- Test using Foundry.
-- Aim for at least 90% *useful* coverage as a baseline, but (near) 100% is achievable in Solidity. Always 100% test the critical path.
-  - Make sure to test for each event emitted
-  - Test each reverting path
-- Consider fuzzing, start with stateless (very easy in Foundry) and if that works, try stateful fuzzing.
-- Consider fork testing if applicable
-
-### Foundry
-
-- Create a Foundry profile for each project folder in `foundry.toml`
-- Foundry tests live in the project folder in `src`, not in the `contracts/test/` folder
-- Set the block number and timestamp. It is preferred to set these values to some reasonable value close to reality.
-- There should be no code between `vm.expectEmit`/`vm.expectRevert` and the function call
+Please read the [Foundry Guide](FOUNDRY_GUIDE.md). No new tests should be written in Hardhat.
 
 ## Picking a Pragma
 
@@ -284,7 +282,7 @@ All rules have a `rule` tag which indicates how the rule is enforced.
 
 ## Comments
 
-- Comments should be in the `//` (default) or `///` (Natspec) format, not the `/*  */` format.
+- Comments should be in the `//` (default) or `///` (for Natspec) format, not the `/*  */` or `/**  **/` format.
   - rule: `tbd`
 - Comments should follow [NatSpec](https://docs.soliditylang.org/en/latest/natspec-format.html)
   - rule: `tbd`
@@ -304,7 +302,7 @@ import {AnythingElse} from "../code/AnythingElse.sol";
 import {ThirdPartyCode} from "../../vendor/ThirdPartyCode.sol";
 ```
 
-- An example would be
+- An example would be the following. Note that third party interfaces go with the third party imports.
 
 ```solidity
 import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";