|
| 1 | +# Contributing |
| 2 | + |
| 3 | +1. [How to Contribute](#1-how-to-contribute) |
| 4 | +2. [Setting up your development environment](#2-setting-up-your-development-environment) |
| 5 | + 1. [Installing Dependencies](#21-installing-dependencies) |
| 6 | + 2. [Signing Commits](#22-signing-commits) |
| 7 | + 3. [Style Guide](#23-style-guide) |
| 8 | + 4. [Deploying Contracts](#24-deploying-contracts) |
| 9 | + 5. [Troubleshooting Issues](#25-troubleshooting-issues) |
| 10 | +3. [Proposing Changes](#3-proposing-changes) |
| 11 | + 1. [Writing Tests](#31-writing-tests) |
| 12 | + 2. [Writing Docs](#32-writing-docs) |
| 13 | + 3. [Auditing Changes](#33-auditing-changes) |
| 14 | + 4. [Creating the PR](#34-creating-the-pr) |
| 15 | + |
| 16 | +## 1. How to Contribute |
| 17 | + |
| 18 | +Thanks for your interest in improving the Farcaster Contracts! |
| 19 | + |
| 20 | +No contribution is too small and we welcome to your help. There's always something to work on, no matter how |
| 21 | +experienced you are. If you're looking for ideas, start with the |
| 22 | +[good first issue](https://github.com/farcasterxyz/contracts/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22) |
| 23 | +or [help wanted](https://github.com/farcasterxyz/contracts/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22) |
| 24 | +sections in the issues. You can help make Farcaster better by: |
| 25 | + |
| 26 | +- Opening issues or adding details to existing issues |
| 27 | +- Fixing bugs in the code |
| 28 | +- Making tests or the ci builds faster |
| 29 | +- Improving the documentation |
| 30 | +- Keeping dependencies up-to-date |
| 31 | +- Proposing and implementing new features |
| 32 | + |
| 33 | +Before you get down to coding, take a minute to consider this: |
| 34 | + |
| 35 | +- If your proposal modifies the [farcaster protocol](https://github.com/farcasterxyz/protocol/), open an issue there |
| 36 | + first. |
| 37 | +- If your proposal is a non-trivial change, consider opening an issue first to get buy-in. |
| 38 | +- If your issue is a small bugfix or improvement, you can simply make the changes and open the PR. |
| 39 | + |
| 40 | +## 2. Setting up your development environment |
| 41 | + |
| 42 | +### 2.1. Installing Dependencies |
| 43 | + |
| 44 | +Install the following packages globally before you get started: |
| 45 | + |
| 46 | +- [Foundry](https://github.com/foundry-rs/foundry) - smart contract toolchain |
| 47 | +- [Slither](https://github.com/crytic/slither#how-to-install) - smart contract static analysis tool |
| 48 | +- [Yarn](https://classic.yarnpkg.com/lang/en/docs/install) - supporting javascript package manager |
| 49 | + |
| 50 | +Once they are installed globally, run `yarn install`, `forge build`, `forge test` and `slither .` to verify that they |
| 51 | +are working. Expect the slither command to print several warnings which are false positives or non-issues. |
| 52 | + |
| 53 | +### 2.2. Signing Commits |
| 54 | + |
| 55 | +All commits must be signed with a GPG key, which is a second factor that proves that your commits came from a device |
| 56 | +in your control. It protects against the case where your GitHub account gets compromised. To get started: |
| 57 | + |
| 58 | +1. [Generate new GPG Keys](https://help.github.com/en/github/authenticating-to-github/generating-a-new-gpg-key). We |
| 59 | + recommend using [GPG Suite](https://gpgtools.org/) on OSX. |
| 60 | +2. Use `gpg-agent` to remember your password locally |
| 61 | + |
| 62 | +```bash |
| 63 | +vi ~/.gnupg/gpg-agent.conf |
| 64 | + |
| 65 | +default-cache-ttl 100000000 |
| 66 | +max-cache-ttl 100000000 |
| 67 | +``` |
| 68 | + |
| 69 | +3. Upload your GPG Keys to your Github Account |
| 70 | +4. Configure Git to [use your keys when signing](https://help.github.com/en/github/authenticating-to-github/telling-git-about-your-signing-key). |
| 71 | +5. Configure Git to always sign commits by running `git config --global commit.gpgsign true` |
| 72 | +6. Commit all changes with your usual git commands and you should see a `Verified` badge near your commits |
| 73 | + |
| 74 | +### 2.3 Style Guide |
| 75 | + |
| 76 | +- `yarn lint` uses [prettier solidity](https://github.com/prettier-solidity/prettier-plugin-solidity) to find and |
| 77 | + auto-correct common problems. |
| 78 | +- `yarn lint:check` performs the same checks, but alerts on errors and does not fix them. |
| 79 | + |
| 80 | +Code follows the [Solidity style guide](https://docs.soliditylang.org/en/v0.8.16/style-guide.html) and documentation |
| 81 | +follows Ethereum [Natspec](https://docs.soliditylang.org/en/develop/natspec-format.html) unless otherwise specified. If |
| 82 | +you use VS Code, you can lint-on-save by installing the [Solidity](https://marketplace.visualstudio.com/items?itemName=JuanBlanco.solidity) |
| 83 | +and [ESLint](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint) extensions. |
| 84 | + |
| 85 | +### 2.4 Deploying Contracts |
| 86 | + |
| 87 | +#### Locally (Using Anvil) |
| 88 | + |
| 89 | +Anvil can be used to run a local instance of a blockchain which is useful for rapid testing and prototyping. |
| 90 | + |
| 91 | +1. Run `anvil` in a shell to start the local blockchain. |
| 92 | +2. Copy one of the private keys that anvil outputs to pass in as an arg in the next step. |
| 93 | +3. Run `forge script script/IdRegistry.s.sol:IdRegistryScript --private-key <private_key> --broadcast --verify --fork-url http://127.0.0.1:8545` |
| 94 | + to deploy the contract |
| 95 | + |
| 96 | +#### To Goerli (Using A Node Provider) |
| 97 | + |
| 98 | +A node provider like Alchemy, Infura or Coinbase can be used to deploy the contract to the Goerli network. First, |
| 99 | +create a `.env` file with the following secrets: |
| 100 | + |
| 101 | +- `GOERLI_RPC_URL`- get this from your node provider (alchemy/infura) |
| 102 | +- `GOERLI_PRIVATE_KEY` - [export](https://metamask.zendesk.com/hc/en-us/articles/360015289632-How-to-export-an-account-s-private-key) |
| 103 | + this from a non-critical metamask wallet that has some goerli eth |
| 104 | +- `ETHERSCAN_KEY` - get this from the [etherscan api](https://etherscan.io/myapikey.) |
| 105 | + |
| 106 | +1. Load the environment variables into your shell with `source .env` |
| 107 | + |
| 108 | +2. Run `forge script script/IdRegistry.s.sol:IdRegistryScript --rpc-url $GOERLI_RPC_URL --private-key $GOERLI_PRIVATE_KEY --broadcast --verify --etherscan-api-key $ETHERSCAN_KEY -vvvv` |
| 109 | + |
| 110 | +This can take several seconds and generates .json outputs with deployment details. Do not commit these changes unless |
| 111 | +you are making a new public deployment. Passing in the Etherscan params to verify the contract makes your code publicly |
| 112 | +available on the contract and allows calling the contract through the UI. |
| 113 | + |
| 114 | +### 2.5 Troubleshooting Issues |
| 115 | + |
| 116 | +#### Solc dyld error on Apple M1 |
| 117 | + |
| 118 | +If you see a solc dyld error like the one below and you are on an M1, follow the steps here: |
| 119 | +https://github.com/foundry-rs/foundry/issues/2712 |
| 120 | + |
| 121 | +```bash |
| 122 | +Solc Error: dyld[35225]: Library not loaded: '/opt/homebrew/opt/z3/lib/libz3.dylib' |
| 123 | + Referenced from: '/Users/<yourusername>/.svm/0.8.16/solc-0.8.16' |
| 124 | + Reason: tried: '/opt/homebrew/opt/z3/lib/libz3.dylib' (no such file), '/usr/local/lib/libz3.dylib' (no such file), '/usr/lib/libz3.dylib' (no such file) |
| 125 | +``` |
| 126 | + |
| 127 | +#### Etherscan verification fails |
| 128 | + |
| 129 | +There's an intermittent issue with Etherscan where verification of the contract fails during deployment. Redeploying |
| 130 | +the contract later seems to resolve this. |
| 131 | + |
| 132 | +#### Estimating Gas Changes |
| 133 | + |
| 134 | +Forge's gas reporting tool can be inaccurate when run across our entire test suite. Gas estimates can be lower than |
| 135 | +expected because tests cover failure cases that terminate early, or higher than expected because the first test run |
| 136 | +always causes storage initialization. A more robust way is to write a special suite of tests that mimic real-world |
| 137 | +usage of contracts and estimate gas on those calls only. Our gas estimation suite can be invoked with |
| 138 | +`forge test --match-contract GasUsage --gas-report`. |
| 139 | + |
| 140 | +## 3. Proposing Changes |
| 141 | + |
| 142 | +When proposing a change, make sure that you've followed all of these steps before you ask for a review. |
| 143 | + |
| 144 | +### 3.1. Writing Tests |
| 145 | + |
| 146 | +All changes that involve features or bugfixes should have supporting Foundry tests. The tests should: |
| 147 | + |
| 148 | +- Live in the `test/` folder |
| 149 | +- Fuzz all possible states including user inputs and time warping |
| 150 | +- Cover all code paths and states that were added |
| 151 | +- Be written as unit tests, and with supporting feature tests if necessary |
| 152 | + |
| 153 | +### 3.2 Writing Docs |
| 154 | + |
| 155 | +All changes should have supporting documentation that makes reviewing and understand the code easy. You should: |
| 156 | + |
| 157 | +- Update high-level changes in the [contract docs](docs/docs.md). |
| 158 | +- Write or update Natspec comments for any relevant functions, variables, constants, events and params. |
| 159 | +- Add comments explaining the 'why' when code is not obvious. |
| 160 | +- Add a `Safety: ..` comment explaining every use of `unsafe`. |
| 161 | +- Add a comment explaining every usage of assembly or fancy math. |
| 162 | +- Add a comment explaining every gas optimization along with how much gas it saves. |
| 163 | + |
| 164 | +### 3.3 Auditing Changes |
| 165 | + |
| 166 | +You should walk through the following steps locally before pushing a PR for review: |
| 167 | + |
| 168 | +- Check gas usage by running `forge test -vvv --match-contract Gas` |
| 169 | +- Check the coverage with `forge coverage` |
| 170 | +- Look for issues with [slither](https://github.com/crytic/slither) by running `slither .` |
| 171 | +- Walk through the [solcurity checklist](https://github.com/transmissions11/solcurity) |
| 172 | + |
| 173 | +If your changes increase gas usage, reduce coverage, introduce new slither issues or violate any solcurity rules |
| 174 | +you must document the rationale clearly in the PR and in the code if appropriate. |
| 175 | + |
| 176 | +### 3.4. Creating the PR |
| 177 | + |
| 178 | +All submissions must be opened as a Pull Request with a full CI run completed. Assign PR's to [@v / varunsrin](https://github.com/varunsrin) |
| 179 | +for review. When creating your PR: |
| 180 | + |
| 181 | +- The PR titles _must_ follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) spec |
| 182 | +- Commit titles _should_ follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) spec |
| 183 | + |
| 184 | +As an example, a good PR title would look like this: |
| 185 | + |
| 186 | +``` |
| 187 | +fix(IdRegistry): idCounter should be incremented by 1 before transferring the id |
| 188 | +``` |
| 189 | + |
| 190 | +While a good commit message might look like this: |
| 191 | + |
| 192 | +``` |
| 193 | +fix(IdRegistry): idCounter should be incremented by 1 before transferring the id |
| 194 | +
|
| 195 | +idCounter was being incremented after the transfer id call which increases gas utilization |
| 196 | +in some conditions |
| 197 | +``` |
0 commit comments