From 8278e86332a0f99d3d8decf905712a5a8d62d280 Mon Sep 17 00:00:00 2001 From: Rens Rooimans Date: Wed, 18 Oct 2023 16:27:45 +0200 Subject: [PATCH 1/3] solidity style guide 2.0 --- contracts/STYLE.md | 234 -------------------------- contracts/STYLE_GUIDE.md | 347 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 347 insertions(+), 234 deletions(-) delete mode 100644 contracts/STYLE.md create mode 100644 contracts/STYLE_GUIDE.md diff --git a/contracts/STYLE.md b/contracts/STYLE.md deleted file mode 100644 index d9692a65210..00000000000 --- a/contracts/STYLE.md +++ /dev/null @@ -1,234 +0,0 @@ -# Solidity Style Guide - -## Background - -Our starting point is the [official Solidity Style Guide](https://solidity.readthedocs.io/en/v0.8.0/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/.prettierrc.js). We are trying to automate as much of this styleguide with Solhint as possible. - -### Code Organization - -- Group functionality together. E.g. Declare structs, events, and helper functions near the functions that use them. This is helpful when reading code because the related pieces are localized. It is also consistent with inheritance and libraries, which are separate pieces of code designed for a specific goal. - - Why not follow the Solidity recommendation of grouping by visibility? Visibility is clearly defined next to the method signature, making it trivial to check. However, searching can be deceiving because of inherited methods. Given this inconsistency in grouping, we find it easier to read and more consistent to organize code around functionality. Additionally, we recommend testing the public interface for any Solidity contract to ensure it only exposes expected methods. - -### Delineate Unaudited Code - -- In a large repo it is worthwhile to keep code that has not yet been audited separate from the code that has been audited. This allows you to easily keep track of which files need to be reviewed. - - E.g. we keep unaudited code in a directory named `dev`. Only once it has been audited we move the audited files out of `dev` and only then is it considered safe to deploy. - -## Variables - -### Visibility - -- All contract variables should be private. Getters should be explicitly written and documented when you want to expose a variable publicly. Whether a getter function reads from storage, a constant, or calculates a value from somewhere else, that’s all implementation details that should not be exposed to the consumer by casing or other conventions. - -Examples: - -Good: - -```javascript -uint256 private s_myVar; - -function getMyVar() external view returns(uint256){ - return s_myVar; -} -``` - -Bad: - -```javascript -uint256 public s_myVar; -``` - -### Naming and Casing - -- Function arguments are named like this: `argumentName`. No leading or trailing underscores necessary. -- Storage variables prefixed with an `s_` to make it clear that they live in storage and are expensive to read and write: `s_variableName`. They should always be private, and you should write explicit getters if you want to expose a storage variable. -- Immutable variables should be prefixed with an `i_` to make it clear that they are immutable. E.g. `i_decimalPlaces`. They should always be private, and you should write explicit getters if you want to expose an immutable variable. -- Internal/private constants should be all caps with underscores: `FOO_BAR`. Like other contract variables, constants should not be public. Create getter methods if you want to publicly expose constants. -- Explicitly declare variable size: `uint256` not just `uint`. In addition to being explicit, it matches the naming used to calculate function selectors. - -Examples: - -Good: - -```javascript -uint256 private s_myVar; -uint256 private immutable i_myImmutVar; -uint256 private constant MY_CONST_VAR; - -function multiplyMyVar(uint256 multiplier) external view returns(uint256){ - return multiplier * s_myVar; -} -``` - -Bad: - -```javascript -uint private s_myVar; -uint256 private immutable myImmutVar; -uint256 private constant s_myConstVar; - -function multiplyMyVar_(uint _multiplier) external view returns(uint256){ - return _mutliplier * s_myVar; -} -``` - -### Types - -- If you are storing an address and know/expect it to be of a type(or interface), make the variable that type. This more clearly documents the behavior of this variable than the `address` type and often leads to less casting code whenever the address is used. - -Examples: - -Good: - -```javascript -import "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol"; -// . -// . -// . -AggregatorV3Interface private s_priceFeed; - -constructor(address priceFeed) { - s_priceFeed = AggregatorV3Interface(priceFeed); -} -``` - -Bad: - -```javascript -import "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol"; -// . -// . -// . -address private s_priceFeed; - -constructor(address priceFeed) { - s_priceFeed = priceFeed; -} -``` - -## Functions - -### Visibility - -- Method visibility should always be explicitly declared. Contract’s [public interfaces should be tested](https://github.com/smartcontractkit/chainlink/blob/master/contracts/test/test-helpers/helpers.ts#L221) to enforce this and make sure that internal logic is not accidentally exposed. - -### Naming - -- Function names should start with imperative verbs, not nouns or other tenses. - - `requestData` not `dataRequest` - - `approve` not `approved` -- Prefix private and internal methods with an underscore. There should never be a publicly callable method starting with an underscore. - - E.g. `_setOwner(address)` -- Prefix your public getters with `get` and your public setters with `set`. - - `getConfig` and `setConfig`. - -## Modifiers - -- Only extract a modifier once a check is duplicated in multiple places. Modifiers arguably hurt readability, so we have found that they are not worth extracting until there is duplication. -- Modifiers should be treated as if they are view functions. They should not change state, only read it. While it is possible to change state in a modifier, it is unconventional and surprising. - -### Naming - -There are two common classes of modifiers, and their name should be prefixed accordingly to quickly represent their behavior: - -- Control flow modifiers: Prefix the modifier name with `if` in the case that a modifier only enables or disables the subsequent code in the modified method, but does not revert. -- Reverting modifiers: Prefix the modifier name with `validate` in the case that a modifier reverts if a condition is not met. - -### Return Values - -- If an address is cast as a contract type, return the type, do not cast back to the address type. This prevents the consumer of the method signature from having to cast again, but presents an equivalent API for off-chain APIs. Additionally it is a more declarative API, providing more context if we return a type. - -## Events - -- 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`.) - -### Naming - -- When possible event names should correspond to the method they are in or the action that is being taken. Events preferably follow the format , 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`. - -## Errors - -### Use Custom Errors - -Whenever possible (Solidity v0.8+) use [custom errors](https://blog.soliditylang.org/2021/04/21/custom-errors/) instead of emitting strings. This saves contract code size and simultaneously provides more informative error messages. - -### Expose Errors - -It is common to call a contract and then check the call succeeded: - -```javascript -(bool success, ) = to.call(data); -require(success, "Contract call failed"); -``` - -While this may look descriptive it swallows the error. Instead bubble up the error: - -```javascript -error YourError(bytes response); - -(bool success, bytes memory response) = to.call(data); -if (!success) { revert YourError(response); } -``` - -This will cost slightly more gas to copy the response into memory, but will ultimately make contract usage more understandable and easier to debug. Whether it is worth the extra gas is a judgement call you’ll have to make based on your needs. - -The original error will not be human readable in an off-chain explorer because it is RLP hex encoded but is easily decoded with standard Solidity ABI decoding tools, or a hex to UTF-8 converter and some basic ABI knowledge. - -## Control Flow - -### `if` Statements - -Always wrap the result statement of your `if` conditions in a closure, even if it is only one line. - -Bad: - -```javascript - if (condition) statement; -``` - -Good: - -```javascript - if (condition) { statement; } -``` - -## Interfaces - -### Scope - -- Interfaces should be as concise as reasonably possible. Break it up into smaller composable interfaces when that is sensible. - -### Naming - -- Up through Solidity version 0.8: Interfaces should be named `FooInterface`, this follows our historical naming pattern. -- Starting in Solidity v0.9: Interfaces should be named `IFoo` instead of `FooInterface`. This follows the patterns of popular [libraries like OpenZeppelin’s](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L9). - -## Vendor Dependencies - -- That’s it, vendor your Solidity dependencies. Supply chain attacks are all the rage these days. There is not yet a silver bullet for best way to vendor, it depends on the size of your project and your needs. You should be as explicit as possible about where the code comes from and make sure that this is enforced in some way; e.g. reference a hash. Some options: - - NPM packages work for repos already in the JavaScript ecosystem. If you go this route you should lock to a hash of the repo or use a proxy registry like GitHub Packages. - - Git submodules are great if you don’t mind git submodules. - - Copy and paste the code into a `vendor` directory. Record attribution of the code and license in the repo along with the commit or version that you pulled the code from. - -## Common Behaviors - -### Transferring Ownership - -- When transferring control, whether it is of a token or a role in a contract, prefer "safe ownership" transfer patterns where the recipient must accept ownership. This avoids accidentally burning the control. This is also inline with the secure pattern of [prefer pull over push](https://consensys.github.io/smart-contract-best-practices/recommendations/#favor-pull-over-push-for-external-calls). - -### Use Factories - -- If you expect multiple instances of a contract to be deployed, it is probably best to [build a factory](https://www.quicknode.com/guides/solidity/how-to-create-a-smart-contract-factory-in-solidity-using-hardhat) as this allows for simpler deployments later. Additionally it reduces the burden of verifying the correctness of the contract deployment. If many people have to deploy an instance of a contract then doing so with a contract makes it much easier for verification because instead of checking the code hash and/or the compiler and maybe the source code, you only have to check that the contract was deployed through a factory. -- Factories can add some pain when deploying with immutable variables. In general it is difficult to parse those out immutable variables from internal transactions. There is nothing inherently wrong with contracts deployed in this manner but at the time of writing they may not easily verify on Etherscan. - -### Call with Exact Gas - -- `call` accepts a gas parameter, but that parameter is a ceiling on gas usage. If a transaction does not have enough gas, `call` will simply provide as much gas as it safely can. This is unintuitive and can lead to transactions failing for unexpected reasons. We have [an implementation of `callWithExactGas`](https://github.com/smartcontractkit/chainlink/blob/075f3e2caf61b8685d2dc78714f1ee39764fda17/contracts/src/v0.8/KeeperRegistry.sol#L792) to ensure the precise gas amount requested is provided. - -## Picking a Pragma - -- If a contract or library is expected to be imported by outside parties then the pragma should be kept as loose as possible without sacrificing safety. We publish versions for every minor semver version of Solidity, and maintain a corresponding set of tests for each published version. - - Examples: libraries, interfaces, abstract contracts, and contracts expected to be inherited from -- Otherwise, Solidity contracts should have a pragma which is locked to a specific version. - - Example: Most concrete contracts. -- Avoid changing pragmas after audit. Unless there is a bug that has affects your contract, then you should try to stick to a known good pragma. In practice this means we typically only support one (occasionally two) pragma for any “major”(minor by semver naming) Solidity version. diff --git a/contracts/STYLE_GUIDE.md b/contracts/STYLE_GUIDE.md new file mode 100644 index 00000000000..2578e07b427 --- /dev/null +++ b/contracts/STYLE_GUIDE.md @@ -0,0 +1,347 @@ +# 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 styleguide 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 do so in accordance to this guide if it would conflict with other practices in that existing file. Consistency is preferred. + +We will be looking into `forge fmt`, but for now we still use `prettier`. + +# Code Organization + +- Group functionality together. E.g. Declare structs, events, and helper functions near the functions that use them. This is helpful when reading code because the related pieces are localized. It is also consistent with inheritance and libraries, which are separate pieces of code designed for a specific goal. +- 🤔Why not follow the Solidity recommendation of grouping by visibility? Visibility is clearly defined next to the method signature, making it trivial to check. However, searching can be deceiving because of inherited methods. Given this inconsistency in grouping, we find it easier to read and more consistent to organize code around functionality. Additionally, we recommend testing the public interface for any Solidity contract to ensure it only exposes expected methods. +- Follow the [Solidity folder structure CLIP](https://github.com/smartcontractkit/CLIPs/tree/main/clips/2023-04-13-solidity-folder-structure) + +### Comments + +- Comments should be in the `//` (default) or `///` (natspec) format, not the `/* */` format. +- Comments should follow [NatSpec](https://docs.soliditylang.org/en/latest/natspec-format.html) +- Besides comment above functions/structs, comments should live everywhere a reader might be confused. Don’t overestimate the reader of your contract, expect confusion in many places and document accordingly. This will help massively during audits and onboarding new team members. +- Headers should be used to group functionality, the following header style and length is recommended. + - Don’t use headers for a single function, or to say “getters”. Group by functionality e.g. the `Tokens and pools` , or `fees` logic within the CCIP OnRamp. + +```solidity + // ================================================================ + // │ Tokens and pools │ + // ================================================================ + +.... + + // ================================================================ + // │ Fees │ + // ================================================================ +``` + +### Imports + +- Imports should always be explicit +- Imports should follow the following format: + +```solidity +import {IInterface} from "../interfaces/IInterface.sol"; + +import {AnythingElse} from "../code/AnythingElse.sol"; + +import {ThirdPartyCode} from "../../vendor/ThirdPartyCode.sol"; +``` + +- An example would be + +```solidity +import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; +import {IPool} from "../interfaces/pools/IPool.sol"; + +import {AggregateRateLimiter} from "../AggregateRateLimiter.sol"; +import {Client} from "../libraries/Client.sol"; + +import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/utils/SafeERC20.sol"; +import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/IERC20.sol"; +``` + +- Sorting imports alphabetically within the sections is nice, but since we don’t have any tooling to automatically do this it is not required. + +## Delineate Unaudited Code + +- In a large repo it is worthwhile to keep code that has not yet been audited separate from the code that has been audited. This allows you to easily keep track of which files need to be reviewed. + - E.g. we keep unaudited code in a directory named `dev` that exists within each projects folder. Only once it has been audited we move the audited files out of `dev` and only then is it considered safe to deploy. + - This `dev` folder also has implications for when code is valid for bug bounties, so be extra careful to move functionality out of a `dev` folder. + +# Variables + +### Visibility + +- All contract variables should be private by default. Getters should be explicitly written and documented when you want to expose a variable publicly. Whether a getter function reads from storage, a constant, or calculates a value from somewhere else, that’s all implementation details that should not be exposed to the consumer by casing or other conventions. + +### Naming and Casing + +- Function arguments are named like this: `argumentName`. No leading or trailing underscores necessary. + - Some exceptions can be made when a function argument shadows a function with the same name +- Names should be explicit on the unit it contains, e.g. a network fee that is charged in USD cents + +```solidity +uint256 fee; // bad +uint256 networkFee; // bad +uint256 networkFeeUSD; // bad +uint256 networkFeeUSDCents; // good +``` + +- Storage variables prefixed with an `s_` to make it clear that they live in storage and are expensive to read and write: `s_variableName`. They should always be private, and you should write explicit getters if you want to expose a storage variable. +- Immutable variables should be prefixed with an `i_` to make it clear that they are immutable. E.g. `i_decimalPlaces`. They should always be private, and you should write explicit getters if you want to expose an immutable variable. +- Internal/private constants should be all caps with underscores: `FOO_BAR`. Like other contract variables, constants should not be public. Create getter methods if you want to publicly expose constants. +- Explicitly declare variable size: `uint256` not just `uint`. In addition to being explicit, it matches the naming used to calculate function selectors. +- Mapping should always be named if Solidity allows it (≥0.8.18) + +### Types + +- If you are storing an address and know/expect it to be of a type(or interface), make the variable that type. This more clearly documents the behavior of this variable than the `address` type and often leads to less casting code whenever the address is used. + +### Structs + +- All structs should be packed to have the lowest memory footprint to reduce gas usage. Even structs that will never be written to storage should be packed. + - A contract can be considered a struct; it should also be packed to reduce gas cost. +- Structs should contain struct packing comments to clearly indicate the storage slot layout + - Using the exact characters from the example below will ensure visually appealing struct packing comments. + - Notice there is no line on the unpacked last `fee` item. +- Struct should contain comments, clearly indicating the denomination of values e.g. 0.01 USD if the variable name doesn’t already do that (which it should). + - Simple tool that could help packing structs and adding comments: https://github.com/RensR/Spack + +```solidity +/// @dev Struct to hold the fee configuration for a fee token, same as the FeeTokenConfig but with +/// token included so that an array of these can be passed in to setFeeTokenConfig to set the mapping +struct FeeTokenConfigArgs { + address token; // ────────────╮ Token address + uint32 networkFeeUSD; // │ Flat network fee to charge for messages, multiples of 0.01 USD + // │ multiline comments should work like this. More fee info + uint64 gasMultiplier; // ─────╯ Price multiplier for gas costs, 1e18 based so 11e17 = 10% extra cost + uint64 premiumMultiplier; // ─╮ Multiplier for fee-token-specific premiums + bool enabled; // ─────────────╯ Whether this fee token is enabled + uint256 fee; // The flat fee the user pays in juels +} +``` + +# Functions + +### Visibility + +- Method visibility should always be explicitly declared. Contract’s [public interfaces should be tested](https://github.com/smartcontractkit/chainlink/blob/master/contracts/test/test-helpers/helpers.ts#L221) to enforce this and make sure that internal logic is not accidentally exposed. + +### Naming + +- Function names should start with imperative verbs, not nouns or other tenses. + - `requestData` not `dataRequest` + - `approve` not `approved` + - `getFeeParameters` not `feeParameters` +- Prefix private and internal methods with an underscore. There should never be a publicly callable method starting with an underscore. + - E.g. `_setOwner(address)` +- Prefix your public getters with `get` and your public setters with `set`. + - `getConfig` and `setConfig`. + +### Return Values + +- If an address is cast as a contract type, return the type, do not cast back to the address type. This prevents the consumer of the method signature from having to cast again, but presents an equivalent API for off-chain APIs. Additionally it is a more declarative API, providing more context if we return a type. +- Returned values should always be explicit. Using named return values and then returning with an empty return should be avoided + +```solidity +// Bad +function getNum() external view returns (uint64 num) { + num = 4; + return; +} + +// Good +function getNum() external view returns (uint64 num) { + num = 4; + return num; +} + +// Good +function getNum() external view returns (uint64) { + return 4; +} + +// Good +function getNum() external view returns (uint64 num) { + return 4; +} +``` + +# Modifiers + +- Only extract a modifier once a check is duplicated in multiple places. Modifiers arguably hurt readability, so we have found that they are not worth extracting until there is duplication. +- Modifiers should be treated as if they are view functions. They should not change state, only read it. While it is possible to change state in a modifier, it is unconventional and surprising. +- Modifiers tend to bloat contract size because the code is duplicated wherever the modifier is used. + +# Events + +- 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 + +### Naming + +- When possible event names should correspond to the method they are in or the action that is being taken. Events preferably follow the format , 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`. + +# Errors + +### Use Custom Errors + +Use [custom errors](https://blog.soliditylang.org/2021/04/21/custom-errors/) instead of emitting strings. This saves contract code size and simultaneously provides more informative error messages. + +### Expose Errors + +It is common to call a contract and then check the call succeeded: + +```solidity +(bool success, ) = to.call(data); +require(success, "Contract call failed"); +``` + +While this may look descriptive it swallows the error. Instead, bubble up the error: + +```solidity +bool success; +retData = new bytes(maxReturnBytes); +assembly { + // call and return whether we succeeded. ignore return data + // call(gas,addr,value,argsOffset,argsLength,retOffset,retLength) + success := call(gasLimit, target, 0, add(payload, 0x20), mload(payload), 0, 0) + + // limit our copy to maxReturnBytes bytes + let toCopy := returndatasize() + if gt(toCopy, maxReturnBytes) { + toCopy := maxReturnBytes + } + // Store the length of the copied bytes + mstore(retData, toCopy) + // copy the bytes from retData[0:_toCopy] + returndatacopy(add(retData, 0x20), 0, toCopy) +} +return (success, retData); +``` + +This will cost slightly more gas to copy the response into memory, but will ultimately make contract usage more understandable and easier to debug. Whether it is worth the extra gas is a judgement call you’ll have to make based on your needs. + +The original error will not be human readable in an off-chain explorer because it is RLP hex encoded but is easily decoded with standard Solidity ABI decoding tools, or a hex to UTF-8 converter and some basic ABI knowledge. + +# Interfaces + +### Scope + +- Interfaces should be as concise as reasonably possible. Break it up into smaller composable interfaces when that is sensible. + +### Naming + +- Interfaces should be named `IFoo` instead of `FooInterface`. This follows the patterns of popular [libraries like OpenZeppelin’s](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L9). + +# 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 recommend 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. + +### Vendor dependencies + +- That’s it, vendor your Solidity dependencies. Supply chain attacks are all the rage these days. There is not yet a silver bullet for best way to vendor, it depends on the size of your project and your needs. You should be as explicit as possible about where the code comes from and make sure that this is enforced in some way; e.g. reference a hash. Some options: + - NPM packages work for repos already in the JavaScript ecosystem. If you go this route you should lock to a hash of the repo or use a proxy registry like GitHub Packages. + - Copy and paste the code into a `vendor` directory. Record attribution of the code and license in the repo along with the commit or version that you pulled the code from. + - Foundry uses git submodules for its dependencies. We only use the `forge-std` lib through submodules, we don’t import any non-Foundry-testing code through this method. + + +# Common Behaviors + +### Transferring Ownership + +- When transferring control, whether it is of a token or a role in a contract, prefer "safe ownership" transfer patterns where the recipient must accept ownership. This avoids accidentally burning the control. This is also inline with the secure pattern of [prefer pull over push](https://consensys.github.io/smart-contract-best-practices/recommendations/#favor-pull-over-push-for-external-calls). + +### Call with Exact Gas + +- `call` accepts a gas parameter, but that parameter is a ceiling on gas usage. If a transaction does not have enough gas, `call` will simply provide as much gas as it safely can. This is unintuitive and can lead to transactions failing for unexpected reasons. We have [an implementation of `callWithExactGas`](https://github.com/smartcontractkit/chainlink/blob/075f3e2caf61b8685d2dc78714f1ee39764fda17/contracts/src/v0.8/KeeperRegistry.sol#L792) to ensure the precise gas amount requested is provided. + +### Sending tokens + +- Prefer [ERC20.safeTransfer](https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#SafeERC20) over ERC20.transfer + +### Gas golfing + +- Golf your code. Make it cheap, within reason. + - Focus on the hot path + - Check the [Solidity guild page](https://www.notion.so/Solidity-Guild-642cfdcfdb0a403c9a8fdb5015ff8244?pvs=21) for recordings on gas golfing +- Most of the cost of executing Solidity is related to reading/writing 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++` +- If you’re unsure about golfing, ask in the #tech-solidity channel + +# Testing + +- Test using Foundry, do not use Hardhat +- Aim for 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 + +# Picking a Pragma + +- If a contract or library is expected to be imported by outside parties then the pragma should be kept as loose as possible without sacrificing safety. We publish versions for every minor semver version of Solidity, and maintain a corresponding set of tests for each published version. + - Examples: libraries, interfaces, abstract contracts, and contracts expected to be inherited from +- Otherwise, Solidity contracts should have a pragma which is locked to a specific version. + - Example: Most concrete contracts. +- Avoid changing pragmas after audit. Unless there is a bug that has affects your contract, then you should try to stick to a known good pragma. In practice this means we typically only support one (occasionally two) pragma for any “major”(minor by semver naming) Solidity version. +- The current advised pragma is `0.8.19` or higher, lower versions should be avoided when starting a new project. Newer versions can be considered. +- All contracts should have a SPDX license identifier. If unsure about which one to pick, please consult with legal. Most older contracts have been MIT, but some of the newer products have been using BUSL-1.1 + +# Versioning + +Contracts should implement the following interface + +```solidity +interface ITypeAndVersion { + function typeAndVersion() external pure returns (string memory); +} +``` + +Here are some examples of what this should look like: + +```solidity +contract AccessControlledFoo is Foo { + // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables + string public constant override typeAndVersion = "AccessControlledFoo 1.0.0"; +} + +contract OffchainAggregator is ITypeAndVersion { + function version() public returns(uint256) { + return 4; + } + + // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables + string public constant override typeAndVersion = "OffchainAggregator 1.0.0"; +} + +// Next version of Aggregator contract +contract SuperDuperAggregator is ITypeAndVersion { + function version() public returns(uint256) { + return 5; + } + /// This is a new contract that has not been released yet, so we + /// add a `-dev` suffix to the typeAndVersion. + + + // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables + string public constant override typeAndVersion = "SuperDuperAggregator 1.1.0-dev"; +} +``` + +All contracts will expose a `typeAndVersion` constant. +The string has the following format: `-` with the `-dev` part only being applicable to contracts that have not been fully released. +Try to fit it into 32 bytes to keep impact on contract sizes minimal. +Solhint will complain about a public constant variable that isn’t FULL_CAPS without the solhint-disable comment. \ No newline at end of file From 494fab22a215df6c0312b1832b0a0ef9b6cf01b8 Mon Sep 17 00:00:00 2001 From: Rens Rooimans Date: Fri, 27 Oct 2023 13:06:09 +0200 Subject: [PATCH 2/3] split into rules and guidelines --- contracts/STYLE_GUIDE.md | 297 +++++++++++++++++++++++---------------- 1 file changed, 174 insertions(+), 123 deletions(-) diff --git a/contracts/STYLE_GUIDE.md b/contracts/STYLE_GUIDE.md index 2578e07b427..418e3d30e16 100644 --- a/contracts/STYLE_GUIDE.md +++ b/contracts/STYLE_GUIDE.md @@ -1,4 +1,10 @@ -# Background +# 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. +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 styleguide with Solhint as possible. @@ -6,17 +12,25 @@ This guide is not meant to be applied retroactively. There is no need to rewrite We will be looking into `forge fmt`, but for now we still use `prettier`. -# Code Organization +# Guidelines + +## Code Organization - Group functionality together. E.g. Declare structs, events, and helper functions near the functions that use them. This is helpful when reading code because the related pieces are localized. It is also consistent with inheritance and libraries, which are separate pieces of code designed for a specific goal. - 🤔Why not follow the Solidity recommendation of grouping by visibility? Visibility is clearly defined next to the method signature, making it trivial to check. However, searching can be deceiving because of inherited methods. Given this inconsistency in grouping, we find it easier to read and more consistent to organize code around functionality. Additionally, we recommend testing the public interface for any Solidity contract to ensure it only exposes expected methods. - Follow the [Solidity folder structure CLIP](https://github.com/smartcontractkit/CLIPs/tree/main/clips/2023-04-13-solidity-folder-structure) -### Comments +### Delineate Unaudited Code -- Comments should be in the `//` (default) or `///` (natspec) format, not the `/* */` format. -- Comments should follow [NatSpec](https://docs.soliditylang.org/en/latest/natspec-format.html) -- Besides comment above functions/structs, comments should live everywhere a reader might be confused. Don’t overestimate the reader of your contract, expect confusion in many places and document accordingly. This will help massively during audits and onboarding new team members. +- In a large repo it is worthwhile to keep code that has not yet been audited separate from the code that has been audited. This allows you to easily keep track of which files need to be reviewed. + - E.g. we keep unaudited code in a directory named `dev` that exists within each projects folder. Only once it has been audited we move the audited files out of `dev` and only then is it considered safe to deploy. + - This `dev` folder also has implications for when code is valid for bug bounties, so be extra careful to move functionality out of a `dev` folder. + + +### comments +- Besides comment above functions/structs, comments should live everywhere a reader might be confused. + Don’t overestimate the reader of your contract, expect confusion in many places and document accordingly. + This will help massively during audits and onboarding new team members. - Headers should be used to group functionality, the following header style and length is recommended. - Don’t use headers for a single function, or to say “getters”. Group by functionality e.g. the `Tokens and pools` , or `fees` logic within the CCIP OnRamp. @@ -32,50 +46,9 @@ We will be looking into `forge fmt`, but for now we still use `prettier`. // ================================================================ ``` -### Imports - -- Imports should always be explicit -- Imports should follow the following format: - -```solidity -import {IInterface} from "../interfaces/IInterface.sol"; - -import {AnythingElse} from "../code/AnythingElse.sol"; - -import {ThirdPartyCode} from "../../vendor/ThirdPartyCode.sol"; -``` - -- An example would be - -```solidity -import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; -import {IPool} from "../interfaces/pools/IPool.sol"; - -import {AggregateRateLimiter} from "../AggregateRateLimiter.sol"; -import {Client} from "../libraries/Client.sol"; - -import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/utils/SafeERC20.sol"; -import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/IERC20.sol"; -``` - -- Sorting imports alphabetically within the sections is nice, but since we don’t have any tooling to automatically do this it is not required. - -## Delineate Unaudited Code - -- In a large repo it is worthwhile to keep code that has not yet been audited separate from the code that has been audited. This allows you to easily keep track of which files need to be reviewed. - - E.g. we keep unaudited code in a directory named `dev` that exists within each projects folder. Only once it has been audited we move the audited files out of `dev` and only then is it considered safe to deploy. - - This `dev` folder also has implications for when code is valid for bug bounties, so be extra careful to move functionality out of a `dev` folder. - -# Variables - -### Visibility - -- All contract variables should be private by default. Getters should be explicitly written and documented when you want to expose a variable publicly. Whether a getter function reads from storage, a constant, or calculates a value from somewhere else, that’s all implementation details that should not be exposed to the consumer by casing or other conventions. - -### Naming and Casing +## Variables - Function arguments are named like this: `argumentName`. No leading or trailing underscores necessary. - - Some exceptions can be made when a function argument shadows a function with the same name - Names should be explicit on the unit it contains, e.g. a network fee that is charged in USD cents ```solidity @@ -85,12 +58,6 @@ uint256 networkFeeUSD; // bad uint256 networkFeeUSDCents; // good ``` -- Storage variables prefixed with an `s_` to make it clear that they live in storage and are expensive to read and write: `s_variableName`. They should always be private, and you should write explicit getters if you want to expose a storage variable. -- Immutable variables should be prefixed with an `i_` to make it clear that they are immutable. E.g. `i_decimalPlaces`. They should always be private, and you should write explicit getters if you want to expose an immutable variable. -- Internal/private constants should be all caps with underscores: `FOO_BAR`. Like other contract variables, constants should not be public. Create getter methods if you want to publicly expose constants. -- Explicitly declare variable size: `uint256` not just `uint`. In addition to being explicit, it matches the naming used to calculate function selectors. -- Mapping should always be named if Solidity allows it (≥0.8.18) - ### Types - If you are storing an address and know/expect it to be of a type(or interface), make the variable that type. This more clearly documents the behavior of this variable than the `address` type and often leads to less casting code whenever the address is used. @@ -119,11 +86,6 @@ struct FeeTokenConfigArgs { } ``` -# Functions - -### Visibility - -- Method visibility should always be explicitly declared. Contract’s [public interfaces should be tested](https://github.com/smartcontractkit/chainlink/blob/master/contracts/test/test-helpers/helpers.ts#L221) to enforce this and make sure that internal logic is not accidentally exposed. ### Naming @@ -131,47 +93,23 @@ struct FeeTokenConfigArgs { - `requestData` not `dataRequest` - `approve` not `approved` - `getFeeParameters` not `feeParameters` -- Prefix private and internal methods with an underscore. There should never be a publicly callable method starting with an underscore. - - E.g. `_setOwner(address)` + - Prefix your public getters with `get` and your public setters with `set`. - `getConfig` and `setConfig`. ### Return Values -- If an address is cast as a contract type, return the type, do not cast back to the address type. This prevents the consumer of the method signature from having to cast again, but presents an equivalent API for off-chain APIs. Additionally it is a more declarative API, providing more context if we return a type. -- Returned values should always be explicit. Using named return values and then returning with an empty return should be avoided +- If an address is cast as a contract type, return the type, do not cast back to the address type. + This prevents the consumer of the method signature from having to cast again, but presents an equivalent API for off-chain APIs. + Additionally, it is a more declarative API, providing more context if we return a type. -```solidity -// Bad -function getNum() external view returns (uint64 num) { - num = 4; - return; -} - -// Good -function getNum() external view returns (uint64 num) { - num = 4; - return num; -} - -// Good -function getNum() external view returns (uint64) { - return 4; -} - -// Good -function getNum() external view returns (uint64 num) { - return 4; -} -``` - -# Modifiers +## Modifiers - Only extract a modifier once a check is duplicated in multiple places. Modifiers arguably hurt readability, so we have found that they are not worth extracting until there is duplication. - Modifiers should be treated as if they are view functions. They should not change state, only read it. While it is possible to change state in a modifier, it is unconventional and surprising. - Modifiers tend to bloat contract size because the code is duplicated wherever the modifier is used. -# Events +## Events - 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 @@ -180,11 +118,6 @@ function getNum() external view returns (uint64 num) { - When possible event names should correspond to the method they are in or the action that is being taken. Events preferably follow the format , 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`. -# Errors - -### Use Custom Errors - -Use [custom errors](https://blog.soliditylang.org/2021/04/21/custom-errors/) instead of emitting strings. This saves contract code size and simultaneously provides more informative error messages. ### Expose Errors @@ -222,17 +155,12 @@ This will cost slightly more gas to copy the response into memory, but will ulti The original error will not be human readable in an off-chain explorer because it is RLP hex encoded but is easily decoded with standard Solidity ABI decoding tools, or a hex to UTF-8 converter and some basic ABI knowledge. -# Interfaces -### Scope +## Interfaces - Interfaces should be as concise as reasonably possible. Break it up into smaller composable interfaces when that is sensible. -### Naming - -- Interfaces should be named `IFoo` instead of `FooInterface`. This follows the patterns of popular [libraries like OpenZeppelin’s](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L9). - -# Dependencies +## 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 recommend to check if some functionality might already be in there before either writing it yourself or adding a third party dependency. @@ -247,7 +175,7 @@ The original error will not be human readable in an off-chain explorer because i - Foundry uses git submodules for its dependencies. We only use the `forge-std` lib through submodules, we don’t import any non-Foundry-testing code through this method. -# Common Behaviors +## Common Behaviors ### Transferring Ownership @@ -265,7 +193,6 @@ 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 - - Check the [Solidity guild page](https://www.notion.so/Solidity-Guild-642cfdcfdb0a403c9a8fdb5015ff8244?pvs=21) for recordings on gas golfing - Most of the cost of executing Solidity is related to reading/writing storage - Calling other contracts will also be costly - Common types to safely use are @@ -274,9 +201,9 @@ The original error will not be human readable in an off-chain explorer because i - prefer `++i` over `i++` - If you’re unsure about golfing, ask in the #tech-solidity channel -# Testing +## Testing -- Test using Foundry, do not use Hardhat +- Test using Foundry. - Aim for 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 @@ -290,7 +217,7 @@ The original error will not be human readable in an off-chain explorer because i - 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 -# Picking a Pragma +## Picking a Pragma - If a contract or library is expected to be imported by outside parties then the pragma should be kept as loose as possible without sacrificing safety. We publish versions for every minor semver version of Solidity, and maintain a corresponding set of tests for each published version. - Examples: libraries, interfaces, abstract contracts, and contracts expected to be inherited from @@ -300,7 +227,8 @@ The original error will not be human readable in an off-chain explorer because i - The current advised pragma is `0.8.19` or higher, lower versions should be avoided when starting a new project. Newer versions can be considered. - All contracts should have a SPDX license identifier. If unsure about which one to pick, please consult with legal. Most older contracts have been MIT, but some of the newer products have been using BUSL-1.1 -# Versioning + +## Versioning Contracts should implement the following interface @@ -315,33 +243,156 @@ Here are some examples of what this should look like: ```solidity contract AccessControlledFoo is Foo { // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables - string public constant override typeAndVersion = "AccessControlledFoo 1.0.0"; + string public constant override typeAndVersion = "AccessControlledFoo 1.0.0"; } contract OffchainAggregator is ITypeAndVersion { - function version() public returns(uint256) { + // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables + string public constant override typeAndVersion = "OffchainAggregator 1.0.0"; + + function getData() public returns(uint256) { return 4; - } - - // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables - string public constant override typeAndVersion = "OffchainAggregator 1.0.0"; + } } // Next version of Aggregator contract contract SuperDuperAggregator is ITypeAndVersion { - function version() public returns(uint256) { - return 5; - } - /// This is a new contract that has not been released yet, so we - /// add a `-dev` suffix to the typeAndVersion. - + /// This is a new contract that has not been released yet, so we + /// add a `-dev` suffix to the typeAndVersion. // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables string public constant override typeAndVersion = "SuperDuperAggregator 1.1.0-dev"; + + function getData() public returns(uint256) { + return 5; + } +} +``` + +All contracts will expose a `typeAndVersion` constant. +The string has the following format: `-` with the `-dev` part only being applicable to contracts that have not been fully released. +Try to fit it into 32 bytes to keep impact on contract sizes minimal. +Solhint will complain about a public constant variable that isn’t FULL_CAPS without the solhint-disable comment. + + + + + + + + + + +# Rules + +All rules have a `rule` tag which indicated how the rule is enforced. + + +## Comments + +- Comments should be in the `//` (default) or `///` (natspec) format, not the `/* */` format. + - rule: `tbd` +- Comments should follow [NatSpec](https://docs.soliditylang.org/en/latest/natspec-format.html) + - rule: `tbd` + +### Imports + +- Imports should always be explicit + - rule: `no-global-import` +- Imports should follow the following format: + - rule: `tbd` + +```solidity +import {IInterface} from "../interfaces/IInterface.sol"; + +import {AnythingElse} from "../code/AnythingElse.sol"; + +import {ThirdPartyCode} from "../../vendor/ThirdPartyCode.sol"; +``` + +- An example would be + +```solidity +import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; +import {IPool} from "../interfaces/pools/IPool.sol"; + +import {AggregateRateLimiter} from "../AggregateRateLimiter.sol"; +import {Client} from "../libraries/Client.sol"; + +import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/utils/SafeERC20.sol"; +import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/IERC20.sol"; +``` + +- Sorting imports alphabetically within the sections is nice, but since we don’t have any tooling to automatically do this it is not required. + +## Variables + +### Visibility + +All contract variables should be private by default. Getters should be explicitly written and documented when you want to expose a variable publicly. + Whether a getter function reads from storage, a constant, or calculates a value from somewhere else, that’s all implementation details that should not be exposed to the consumer by casing or other conventions. + +rule: tbd + +### Naming and Casing + +- Storage variables prefixed with an `s_` to make it clear that they live in storage and are expensive to read and write: `s_variableName`. They should always be private, and you should write explicit getters if you want to expose a storage variable. + - rule: `chainlink-solidity/prefix-storage-variables-with-s-underscore` +- Immutable variables should be prefixed with an `i_` to make it clear that they are immutable. E.g. `i_decimalPlaces`. They should always be private, and you should write explicit getters if you want to expose an immutable variable. + - rule: `chainlink-solidity/prefix-immutable-variables-with-i` +- Internal/private constants should be all caps with underscores: `FOO_BAR`. Like other contract variables, constants should not be public. Create getter methods if you want to publicly expose constants. + - rule: `chainlink-solidity/all-caps-constant-storage-variables` +- Explicitly declare variable size: `uint256` not just `uint`. In addition to being explicit, it matches the naming used to calculate function selectors. + - rule: `explicit-types` +- Mapping should always be named if Solidity allows it (≥0.8.18) + - rule: `tbd` + + +## Functions + +### Visibility + +- Method visibility should always be explicitly declared. + - rule: `state-visibility` + +- Prefix private and internal methods with an underscore. There should never be a publicly callable method starting with an underscore. + - E.g. `_setOwner(address)` + - rule: `chainlink-solidity/prefix-internal-functions-with-underscore` + +### Return values + +- Returned values should always be explicit. Using named return values and then returning with an empty return should be avoided + - rule: `chainlink-solidity/explicit-returns` + +```solidity +// Bad +function getNum() external view returns (uint64 num) { + num = 4; + return; +} + +// Good +function getNum() external view returns (uint64 num) { + num = 4; + return num; +} + +// Good +function getNum() external view returns (uint64 num) { + return 4; } ``` -All contracts will expose a `typeAndVersion` constant. -The string has the following format: `-` with the `-dev` part only being applicable to contracts that have not been fully released. -Try to fit it into 32 bytes to keep impact on contract sizes minimal. -Solhint will complain about a public constant variable that isn’t FULL_CAPS without the solhint-disable comment. \ No newline at end of file +## Errors + +### Use Custom Errors + +Use [custom errors](https://blog.soliditylang.org/2021/04/21/custom-errors/) instead of emitting strings. This saves contract code size and simultaneously provides more informative error messages. + +rule: `custom-errors` + +## Interfaces + +Interfaces should be named `IFoo` instead of `FooInterface`. This follows the patterns of popular [libraries like OpenZeppelin’s](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L9). + +rule: `tbd` \ No newline at end of file From 8540ab1aac5cfa81f9ecd14249706e8787ccd087 Mon Sep 17 00:00:00 2001 From: Rens Rooimans Date: Fri, 27 Oct 2023 14:04:43 +0200 Subject: [PATCH 3/3] improve layout --- contracts/STYLE_GUIDE.md | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/contracts/STYLE_GUIDE.md b/contracts/STYLE_GUIDE.md index 418e3d30e16..3868117d4b9 100644 --- a/contracts/STYLE_GUIDE.md +++ b/contracts/STYLE_GUIDE.md @@ -27,7 +27,7 @@ We will be looking into `forge fmt`, but for now we still use `prettier`. - This `dev` folder also has implications for when code is valid for bug bounties, so be extra careful to move functionality out of a `dev` folder. -### comments +## comments - Besides comment above functions/structs, comments should live everywhere a reader might be confused. Don’t overestimate the reader of your contract, expect confusion in many places and document accordingly. This will help massively during audits and onboarding new team members. @@ -85,7 +85,7 @@ struct FeeTokenConfigArgs { uint256 fee; // The flat fee the user pays in juels } ``` - +## Functions ### Naming @@ -113,9 +113,6 @@ 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 - -### Naming - - When possible event names should correspond to the method they are in or the action that is being taken. Events preferably follow the format , 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`. @@ -153,7 +150,7 @@ return (success, retData); This will cost slightly more gas to copy the response into memory, but will ultimately make contract usage more understandable and easier to debug. Whether it is worth the extra gas is a judgement call you’ll have to make based on your needs. -The original error will not be human readable in an off-chain explorer because it is RLP hex encoded but is easily decoded with standard Solidity ABI decoding tools, or a hex to UTF-8 converter and some basic ABI knowledge. +The original error will not be human-readable in an off-chain explorer because it is RLP hex encoded but is easily decoded with standard Solidity ABI decoding tools, or a hex to UTF-8 converter and some basic ABI knowledge. ## Interfaces @@ -204,7 +201,7 @@ The original error will not be human readable in an off-chain explorer because i ## Testing - Test using Foundry. -- Aim for 90%+ *useful* coverage as a baseline, but (near) 100% is achievable in Solidity. Always 100% test the critical path. +- 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. @@ -295,11 +292,11 @@ All rules have a `rule` tag which indicated how the rule is enforced. - Comments should follow [NatSpec](https://docs.soliditylang.org/en/latest/natspec-format.html) - rule: `tbd` -### Imports +## Imports - Imports should always be explicit - rule: `no-global-import` -- Imports should follow the following format: +- Imports have follow the following format: - rule: `tbd` ```solidity @@ -323,14 +320,12 @@ import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/tok import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/IERC20.sol"; ``` -- Sorting imports alphabetically within the sections is nice, but since we don’t have any tooling to automatically do this it is not required. - ## Variables ### Visibility All contract variables should be private by default. Getters should be explicitly written and documented when you want to expose a variable publicly. - Whether a getter function reads from storage, a constant, or calculates a value from somewhere else, that’s all implementation details that should not be exposed to the consumer by casing or other conventions. +Whether a getter function reads from storage, a constant, or calculates a value from somewhere else, that’s all implementation details that should not be exposed to the consumer by casing or other conventions. rule: tbd @@ -385,8 +380,6 @@ function getNum() external view returns (uint64 num) { ## Errors -### Use Custom Errors - Use [custom errors](https://blog.soliditylang.org/2021/04/21/custom-errors/) instead of emitting strings. This saves contract code size and simultaneously provides more informative error messages. rule: `custom-errors`