In the following, there will be guidelines for the process of writing readable, maintainable and secure contract code. We try to cover the entire process:
- before writing the code,
- while writing,
- when optimizing
- when reviewing.
In general the language of choice will be Solidity for our L1 contracts, so most of the specific style rules will be having that in mind, but the process for writing it can be adapted slightly for vyper, and will have an extension when Aztec.nr contracts are more mature.
When you plan to build a system, or write a contract to extend an existing one. This should be the go-to approach.
- Write a specification.
- Define how the system is planned to work, what trade-offs are made and why. Do this without writing an implementation! The spec should have a "TL;DR" or similar so people can quickly skim to see what you want to do and why.
- Write a reference implementation of the spec, focus on readability
- Solidity contracts especially can be a pain in the neck to read if an implementation is optimized heavily.
- Write extensive test for individual functions, integrations and scenarios
- Add fuzz-testing when the bases are covered
- Review reference implementation
- Fix any issues that have been revealed by the review
- Create an optimized implementation
Writing a spec can feel wasteful because it "slows" development down, but be really really practical when:
- Other people have to figure out what your code is supposed to do;
- Checking that the underpinning ideas of the implementation is sound.
For the spec, having a description of the data structures to very helpful to understand the system - don't think about storage layout and gas-costs for the initial design.
Secondly, we want a rather specific (but not to the level of implementation) description of the actions (functions) that can be executed on the "model". For any action, we ideally want to specify:
- Name: A
nameand inputs for the action, e.g.,transfer(from, to, amount) - Preconditions: A set of preconditions that must be satisfied for the action to execute successfully, e.g.,
balance[from] >= amount. - Effects A set of state updates, e.g.,
balance[from] -= amount && balance[to] += amount.
The spec should outline the dependencies of the contract (other protocols and tokens) and how they work. In the case of tokens for accounting purposes this includes listing the ways in which the balance can be altered (e.g., transfer, burn, mint, rebase, freeze, liquidate, etc). This is an important step, as it makes it a lot easier to:
- Assess the risk of the extension/protocol.
- Makes it more clear what types of actions the accounting needs to support, and if any are missing.
A reference implementation is a practical instantiation of the spec that should strive to maximize readability.
It should make it very clear what the purpose is, and how it acheives its goal, meaning that no heavy optimizations should be made. The reference is simply to be used as a good way to understand the code, and to build tests for. To make assessing the coverage of the code easier, ternary operators should be avoid, e.g., instead of a > b ? a : b please use explicit if-else branching.
Having a reference implementation, that can later be optimized, should also make it easier for auditors and external parties to give a first look at the protocol, e.g., if there is a flaw in the business logic, it should be possible to catch at this point.
Test to cover all the user intended flows should be written, as well as tests for failure cases and less intended flows.
To make the test useful for reviewers, a small comment describing it in easily understandable language should be added.
When the tests have been written, coverage should be ~100%. If not, put focus on the parts that are missing, is there some possible behavior that we have not thought about or what is the reasoning behind the missing coverage.
A happy path is a series of actions that we expect users to do, or be part of, e.g., users using the protocol as expected.
These tests are to handle the cases where someone deliberately are trying to mess with the accounting or extract funds.
This is tests that should have us end with a revert, any revert should have a tests where they will be hit.
For actions with user-controlled input amounts or where inner values depend on timing, property based testing (fuzzing) should be used to change time (between actions) and the input values. Remember to limit values to something that is possible, e.g., if the total supply of a token is fixed, values beyond the supply should not be accepted.
For every issue found, create a github issue. These should be grouped in one of the following groups informational, low, medium, high. Issues should be separate, but if the same problem have been spotted multiple places, e.g., off-by one for both deposit and withdraw, it can be grouped into one issue.
When reviewing, consult the references to have checklists to go through.
Also, is the contract planned to be used behind a proxy or directly? If behind a proxy, is there are deployment script that will initialize it, and does the upgrade mechanism work or is something missing.
For each of the github issue that have been created by the review, either a PR should be made that addresses the issue or it should be acknowledged.
Before writing the optimized implementation, the tests should be extended to the point where no test is lacking. Also, all tests should pass for both sets.
- Variable packing
- Cached variables (if read from storage multiple times, cache the value and use it)
- Unchecked math for increment in values where it is known that it will never overflow.
For each of the changes that are made, please add an estimate on the gas savings. If gas is saved and it don't significantly increase the code-size or heavily denegate the readability
There is a couple of different angles when reviewing smart contracts:
- Correctness: Checking correctness of the implementation, e.g., does the implementation matches the specification, and is the specification sound?
- Attack vectors, e.g., how can an attacker gain access or influence the system in an unintended way.
- Gas Optimizations
We generally strive to adhere to the soliditylang guidelines with a few extension outlined below:
- Any imports should be fully specified, e.g.,
import { IERC20 } from "./IERC20.sol"overimport "./IERC20.sol". - When using
uintalways be explicit with the size, e.g.,uint256oruint128or whatever - Beyond the ordering of functions, we use error codes, events, struct, storage, modifiers and then the functions. Functions again sorted as by the soliditylang guidelines.
- For
ifstatements, we should always add brackets{}even if it may be on a single line. - When overriding, always specify what is overridden, e.g.,
override(IRollupProcessor) _prefix on internal/private function names (unless if in library)- Don't prefix with
_on internal/private storage variables _prefix on function arguments- Push people to use Custom Errors instead of requires.
constantandimmutablevalues should be written inCAPITAL_CASE
Natspec should be written for all functions (internal mainly for clarity). Use the @notice tag for general explanation of the function that a user should be able to understand and @dev for more developer specific information.
/**
* @notice Consumes an entry from the Outbox
* @dev Only meaningfully callable by portals, otherwise should never hit an entry
* @dev Emits the `MessageConsumed` event when consuming messages
* @param _message - The L2 to L1 message
* @return entryKey - The key of the entry removed
*/
function consume(DataStructures.L2ToL1Msg memory _message)Many of these guidelines are enforced by solhint, with additions in https://github.com/LHerskind/solhint.
{
"extends": "solhint:recommended",
"rules": {
"compiler-version": ["error", ">=0.8.4"],
"no-inline-assembly": "off",
"func-visibility": ["error", { "ignoreConstructors": true }],
"no-empty-blocks": "off",
"no-unused-vars": ["error"],
"state-visibility": ["error"],
"not-rely-on-time": "off",
"const-name-snakecase": ["error", { "treatImmutableVarAsConstant": true }],
"var-name-mixedcase": ["error", { "treatImmutableVarAsConstant": true }],
"custom-error-name-camelcase": ["error", { "allowPrefix": true }],
"private-func-leading-underscore": ["error"],
"private-vars-no-leading-underscore": ["error"],
"func-param-name-leading-underscore": ["error"],
"func-param-name-mixedcase": ["error"],
"custom-error-over-require": ["error"],
"strict-override": ["error"],
"strict-import": ["error"],
"ordering": ["error"],
"comprehensive-interface": ["error"]
}
}