-
Notifications
You must be signed in to change notification settings - Fork 8
Contributing doc change suggestions #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f6ad557
f689611
f9708f7
42b1295
d637785
d2663ff
2198230
3cb1447
b0369b4
a203be3
e8b2abd
1c182ce
95e1fb8
67fbf0c
50966bb
38ed598
1a11c6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,3 +5,4 @@ cache/ | |
| docs/autogenerated | ||
| *.sol | ||
| deployments/ | ||
| snapshots/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,28 @@ | ||
| # Contributing | ||
|
|
||
| For the latest version of this document, see [here](https://github.com/Uniswap/foundry-template/blob/main/CONTRIBUTING.md). | ||
|
|
||
| - [Install](#install) | ||
| - [Pre-commit Hooks](#pre-commit-hooks) | ||
| - [Requirements for merge](#requirements-for-merge) | ||
| - [Branching](#branching) | ||
| - [Main](#main) | ||
| - [Staging](#staging) | ||
| - [Dev](#dev) | ||
| - [Feature](#feature) | ||
| - [Fix](#fix) | ||
| - [Audit](#audit) | ||
| - [Code Practices](#code-practices) | ||
| - [Code Style](#code-style) | ||
| - [Solidity Versioning](#solidity-versioning) | ||
| - [Interfaces](#interfaces) | ||
| - [NatSpec \& Comments](#natspec--comments) | ||
| - [Testing](#testing) | ||
| - [Best Practices](#best-practices) | ||
| - [IR Compilation](#ir-compilation) | ||
| - [Gas Metering](#gas-metering) | ||
| - [Deployment](#deployment) | ||
| - [Deployment](#deployment-1) | ||
| - [Deployment Info Generation](#deployment-info-generation) | ||
| - [Bytecode Hash](#bytecode-hash) | ||
| - [Monorepo](#monorepo) | ||
| - [Dependency Management](#dependency-management) | ||
| - [Releases](#releases) | ||
|
|
||
| ## Install | ||
|
|
@@ -56,23 +61,19 @@ This section outlines the branching strategy of this repo. | |
|
|
||
| ### Main | ||
|
|
||
| The main branch is supposed to reflect the deployed state on all networks. Any pull requests into this branch MUST come from the staging branch. | ||
|
|
||
| ### Staging | ||
|
|
||
| The staging branch reflects new code complete deployments or upgrades containing fixes and/or features. Any pull requests into this branch MUST come from the dev branch. The staging branch is used for security audits and deployments. Once the deployment is complete and verified as well as deployment log files are generated, the branch can be merged into main. For more information on the deployment and log file generation check [here](#deployment). | ||
| The main branch is supposed to reflect the deployed state on all networks. Only audited code should be merged into main. Squashed commits from dev or feature branches should be merged into the main branch using a regular merge strategy. | ||
|
|
||
| ### Dev | ||
|
|
||
| This is the active development branch. All pull requests into this branch MUST come from fix or feature branches. Upon code completion this branch is merged into staging for auditing and deployment. PRs into this branch should squash all commits into a single commit. | ||
| This is the active development branch. Upon code completion this branch is frozen on an audit branch. PRs into this branch should squash all commits into a single commit. In case of multiple parallel development efforts, each project should have its own dev branch with the naming convention `dev/<project_name>` to ensure work in progress is isolated and does not end up on the main branch. | ||
|
|
||
| ### Feature | ||
|
|
||
| Any new feature should be developed on a separate branch. The naming convention for these branches is `feat/*`. Once the feature is complete, a pull request into the dev branch can be created. | ||
| Feature branches should be owned by one responsible developer. The dev branch should be the target of the feature branch. In pre-audit and pre-deployment repositories feature branches may be merged into the main branch directly. Generally, feature branches should be squashed into a single commit before merging. | ||
|
|
||
| ### Fix | ||
| ### Audit | ||
|
|
||
| Any bug fixes should be developed on a separate branch. The naming convention for these branches is `fix/*`. Once the fix is complete, a pull request into the dev branch can be created. | ||
| Before an audit, the code should be frozen on a branch dedicated to the audit with the naming convention `audit/<provider>`. Each fix in response to an audit finding should be developed on its own branch. The naming convention for these branches is `audit/<provider>/<issue_number>`. The PR title should include the provider and the issue number at minimum. Sometimes it is desirable to have all of the fixes in a single PR for review. In this case, each fix PR should be included via the `merge` strategy, and the combined PR can be merged into the dev branch via `merge` to preserve the order of the fix commits. | ||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete branching section There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think specifying WHEN we use dev branches would be nice, as we've started to introduce that into repos that have continual developments. Also including a note that all deployed versions should be tagged
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a release section, lmk if you want any additional information here. |
||
| ## Code Practices | ||
|
|
||
|
|
@@ -91,6 +92,18 @@ The repo follows the official [Solidity Style Guide](https://docs.soliditylang.o | |
| } | ||
| ``` | ||
|
|
||
| - Naming collisions should be avoided using a single trailing underscore. | ||
|
gretzke marked this conversation as resolved.
|
||
|
|
||
| ```solidity | ||
| contract TestContract { | ||
| uint256 public foo; | ||
|
|
||
| constructor(uint256 foo_) { | ||
| foo = foo_; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| - Events should generally be emitted immediately after the state change that they | ||
| represent, and should be named in the past tense. Some exceptions may be made for gas | ||
| efficiency if the result doesn't affect observable ordering of events. | ||
|
|
@@ -117,13 +130,29 @@ The repo follows the official [Solidity Style Guide](https://docs.soliditylang.o | |
|
|
||
| - Unchecked arithmetic blocks should contain comments explaining why overflow is guaranteed not to happen or permissible. If the reason is immediately apparent from the line above the unchecked block, the comment may be omitted. | ||
|
|
||
| ### Solidity Versioning | ||
|
gretzke marked this conversation as resolved.
|
||
|
|
||
| Contracts that are meant to be deployed MUST have an explicit version set in the `pragma` statement. | ||
|
|
||
| ```solidity | ||
| pragma solidity 0.8.X; | ||
| ``` | ||
|
|
||
| Abstract contracts, libraries and interfaces MUST use the caret (`^`) range operator to specify the version range to ensure better compatibility. | ||
|
|
||
| ```solidity | ||
| pragma solidity ^0.X.0; | ||
| ``` | ||
|
|
||
| Libraries and abstract contracts using functionality introduced in newer versions of Solidity can use caret range operators with higher path versions (e.g., `^0.8.24` when using transient storage opcodes). For interfaces, it should be considered to use the greater than or equal to (`>=`) range operator to ensure better compatibility with future versions of Solidity. | ||
|
|
||
| ### Interfaces | ||
|
|
||
| Every contract MUST implement their corresponding interface that includes all externally callable functions, errors and events. | ||
|
|
||
| ### NatSpec & Comments | ||
|
|
||
| Interfaces should be the entrypoint for all contracts. When exploring the a contract within the repository, the interface MUST contain all relevant information to understand the functionality of the contract in the form of NatSpec comments. This includes all externally callable functions, errors and events. The NatSpec documentation MUST be added to the functions, errors and events within the interface. This allows a reader to understand the functionality of a function before moving on to the implementation. The implementing functions MUST point to the NatSpec documentation in the interface using `@inheritdoc`. Internal and private functions shouldn't have NatSpec documentation except for `@dev` comments, whenever more context is needed. Additional comments within a function should only be used to give more context to more complex operations, otherwise the code should be kept readable and self-explanatory. | ||
| Interfaces should be the entrypoint for all contracts. When exploring the a contract within the repository, the interface MUST contain all relevant information to understand the functionality of the contract in the form of NatSpec comments. This includes all externally callable functions, errors and events. The NatSpec documentation MUST be added to the functions, errors and events within the interface. This allows a reader to understand the functionality of a function before moving on to the implementation. The implementing functions MUST point to the NatSpec documentation in the interface using `@inheritdoc`. Internal and private functions shouldn't have NatSpec documentation except for `@dev` comments, whenever more context is needed. Additional comments within a function should only be used to give more context to more complex operations, otherwise the code should be kept readable and self-explanatory. Single line NatSpec comments should use a triple slash (`///`) to ensure compact documentation. | ||
|
|
||
| ## Testing | ||
|
|
||
|
|
@@ -133,26 +162,59 @@ Differential testing should be used to compare assembly implementations with imp | |
|
|
||
| New features must be merged with associated tests. Bug fixes should have a corresponding test that fails without the bug fix. | ||
|
|
||
| ### Gas Metering | ||
| ### Best Practices | ||
|
|
||
| The [Forge Gas Snapshot](https://github.com/marktoda/forge-gas-snapshot) library is used to measure the gas cost of individual actions. To ensure that the measured gas is accurate, tests have to be run using the isolate argument to generate the correct snapshot and ensure that CI passes: | ||
| Best practices and naming conventions should be followed as outlined in the [Foundry Book](https://getfoundry.sh/forge/tests/overview). | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Foundrybook reference for testing best practices and naming conventions |
||
|
|
||
| ```sh | ||
| $ forge test --isolate | ||
| ``` | ||
| ### IR Compilation | ||
|
|
||
| When adding new functionality, a new gas snapshot should be added, preferably using `snapLastCall`. | ||
| When the contracts are compiled via IR, tests should be compiled without IR and the contracts deployed from their bytecode to ensure quick compilation times and bytecode consistency. Check [here](https://github.com/Uniswap/foundry-template/blob/613f81c107cd2885a869dbe4afc1da4f96ed9218/foundry.toml#L22-L28) and [here](https://github.com/Uniswap/foundry-template/blob/main/test/deployers/CounterDeployer.sol) for an example. | ||
|
gretzke marked this conversation as resolved.
|
||
|
|
||
| ### Gas Metering | ||
|
|
||
| Gas for function calls should be metered using the built in `vm.snapshotGasLastCall` function in forge. To meter across multiple calls `vm.startSnapshotGas` and `vm.stopSnapshotGas` can be used. Tests that measure gas should be annotated with `/// forge-config: default.isolate = true` and not be fuzzed to ensure that the gas snapshot is accurate and consistent for CI verification. All external functions should have a gas snapshot test, diverging paths within a function should have appropriate gas snapshot tests. | ||
| For more information on gas metering see the [Forge cheatcodes reference](https://getfoundry.sh/reference/cheatcodes/gas-snapshots/#snapshotgas-cheatcodes). | ||
|
|
||
| ## Deployment | ||
|
|
||
| After deployments are executed a script is provided that extracts deployment information from the `run-latest.json` file within the `broadcast` directory generated while the forge script runs. From this information a JSON and markdown file is generated using the [Forge Chronicles](https://github.com/0xPolygon/forge-chronicles) library containing various information about the deployment itself as well as past deployments. | ||
| After deployments are executed a script is provided that extracts deployment information from the `run-latest.json` file within the `broadcast` directory generated while the forge script runs. From this information a JSON and markdown file is generated using the [Forge Chronicles](https://github.com/uniswap/forge-chronicles) library containing various information about the deployment itself as well as past deployments. | ||
|
|
||
| ### Bytecode Hash | ||
|
|
||
| ### Deployment | ||
| Bytecode hash MUST be set to `none` in the `foundry.toml` file to ensure that the bytecode is consistent. | ||
|
|
||
| To deploy the contracts, provide the `--broadcast` flag to the forge script command. Should the etherscan verification time out, it can be picked up again by replacing the `--broadcast` flag with `--resume`. | ||
| Deploy the contracts to one of the predefined networks by providing the according key with the `--rpc-url` flag. Most of the predefined networks require the `INFURA_KEY` environment variable to be set in the `.env` file. | ||
| Including the `--verify` flag will verify deployed contracts on Etherscan. Define the appropriate environment variable for the Etherscan api key in the `.env` file. | ||
| ### Monorepo | ||
|
|
||
| ```shell | ||
| forge script script/Deploy.s.sol --broadcast --rpc-url <rpc_url> --verify | ||
| Contracts should be integrated into the [Smart Contract Monorepo](https://github.com/uniswap/contracts) to be deployed via the deployer cli tool. | ||
|
|
||
| ## Dependency Management | ||
|
|
||
| The preferred way to manage dependencies is using [`forge install`](https://book.getfoundry.sh/forge/dependencies). This ensures that your project uses the correct versions and structure for all external libraries. | ||
|
|
||
| However, in cases where there is a Solidity version mismatch between your project and a dependency, it may be required to include the compiled bytecode directly in a utility contract. This approach allows to deploy the dependency using the correct bytecode, regardless of source compatibility. This may be especially helpful for integration testing. | ||
|
|
||
| First, it should be checked if the deployer is already part of [Briefcase](https://github.com/Uniswap/briefcase/tree/main/src/deployers). If so, import the deployer from here. | ||
|
|
||
| Otherwise, a custom deploy contract should be created. Below is an example of how to deploy a contract using hardcoded bytecode and the `CREATE2` opcode. | ||
|
|
||
| ```solidity | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.24; | ||
| contract BytecodeDeployer { | ||
| /// @dev Deploys a contract using CREATE2 with the provided bytecode and salt. | ||
| /// @param bytecode The contract bytecode to deploy. | ||
| /// @param salt The salt to use for CREATE2. | ||
| /// @return addr The address of the deployed contract. | ||
| function deploy(bytes memory bytecode, bytes32 salt) public returns (address addr) { | ||
| require(bytecode.length != 0, "Bytecode is empty"); | ||
| assembly { | ||
| addr := create2(bytecode, salt) | ||
| if iszero(extcodesize(addr)) { revert(0, 0) } | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Releases | ||
|
|
||
| Every deployment and changes made to contracts after deployment should be accompanied by a tag and release on GitHub. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "Increment counter number": "26263", | ||
| "Set counter number": "26337" | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.