solidity style guide 2.0 #10995

RensR · 2023-10-18T14:33:43Z

Our preferences have evolved, and it's time to have the guide reflect that. This iterations aims to be more opinionated, leading to more consistent code.

After this guide has been approved, the solhint rule suite will be updated to enforce the new styling rules.

github-actions · 2023-10-18T14:34:07Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

DeividasK · 2023-10-23T08:18:02Z

contracts/STYLE_GUIDE.md

+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.


There's definitely an eslint rule for this. I'd say that this shouldn't be mentioned here, as otherwise someone who prefers this might make style comments, and these can be annoying.

Maybe it would be worthwhile to add a general rule of thumb that I use: "Anything to do with style should be a formatting plugin. If you care enough about a particular style, add or write that plugin, so that others wouldn't need to manually do this."

I've split the doc into a Guidelines and a Rules section. All entries in the Rules section have a solhint rule

DeividasK · 2023-10-23T08:19:37Z

contracts/STYLE_GUIDE.md

+
+### Comments
+
+- Comments should be in the `//` (default) or `///` (natspec) format, not the `/*  */` format.


... not the /* */ format.

This should be a rule that automatically converts these comments to // ones or warns/errors linting. I'd like to avoid having any style comments on PRs.

Sounds like a prettier thing, we can look into it but doesn't feel very high prio to me

DeividasK · 2023-10-23T08:20:56Z

contracts/STYLE_GUIDE.md

+import {IInterface} from "../interfaces/IInterface.sol";
+
+import {AnythingElse} from "../code/AnythingElse.sol";
+
+import {ThirdPartyCode} from "../../vendor/ThirdPartyCode.sol";


Is there a reason for this particular spacing?

These three groups indicate very different usage: interfaces are a black box import, only caring about external functions and not whats inside. Third party imports should imo always be clearly separated from the rest because of the fact that it isn't our code you're working with. That leaves "AnythingElse" in the middle, which is not black box and not third party, so it implies a closer relationship

DeividasK · 2023-10-23T09:03:36Z

contracts/STYLE_GUIDE.md

+
+- 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.


This seems like a general engineering best practice. It's useful to talk about it, but a styleguide might not be the first place people would look for it. Depends on how strict we are about what lives in a styleguide.

I feel a style guide like this is the correct place for this. Is there a meaningful difference between style and best practices?

yosriady

Overall LGTM

yosriady · 2023-10-23T12:29:19Z

contracts/STYLE_GUIDE.md

+  - 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
+  // ================================================================


I don't have a strong opinion on this, but should the comment formatting be part of the style guide?

I like consistency and I've seen at least 4 formats to create headers in Chainlink code. Having a default helps imo

yosriady · 2023-10-23T12:30:15Z

contracts/STYLE_GUIDE.md

+- Imports should follow the following format:
+
+```solidity
+import {IInterface} from "../interfaces/IInterface.sol";


Where should external dependency imports (e.g. OpenZeppelin) be in terms of ordering?

At the bottom with ThirdPartyCode

contracts/STYLE_GUIDE.md

DeividasK · 2023-10-23T15:46:53Z

contracts/STYLE_GUIDE.md

+# 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.


Why is 100% not achievable? Are there any specific examples that are hard/impossible to test?

It is, in non-solidity (imo) going for exactly 100 is often not extremely productive. Here I wanted to call out that in Solidity, 100% is achievable.

Well, if you exclude the fact that Foundry coverage sometimes doesn't work on libraries

contracts/STYLE_GUIDE.md

austinborn · 2023-10-23T18:09:51Z

Could we add a section on the topic of whether to prefer wrapped gas tokens vs. native? And how to determine the correct ERC-20 addresses?

jinhoonbang · 2023-10-25T15:45:42Z

contracts/STYLE_GUIDE.md

+
+### 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.


what are the implications of contract size here? e.g. gas efficiency for public variable vs (private var + public getter)

Having variables public simply means a getter with the same name is added to the contract. I believe it is exactly the same gas and size-wise as writing one yourself.

jinhoonbang · 2023-10-25T16:03:20Z

contracts/STYLE_GUIDE.md

+    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";


didn't follow this. is the recommendation here to have both version() and typeAndVersion?

The example contract was confusingly named. The function was just supposed to be example code and has been renamed now

Oozyx · 2023-10-25T16:16:20Z

contracts/STYLE_GUIDE.md

+
+### 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.


Any guideline on private vs internal? Some audit guidelines suggest defaulting to internal, some suggest declaring internal only when needed.

All contract variables should be private by default

It will depend on the exact contract, if you the variable in higher layers, it has to be internal. Defaulting to private is the safest option

Oozyx · 2023-10-25T16:25:54Z

contracts/STYLE_GUIDE.md

+- 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.


When starting a new project, is picking the latest version advisable? Is there merit in picking an older, more battle tested version?

The latest that is already used in CL products would be default imo. The absolute latest (given it has been out for 3+ months) can be considered. There should be no reason so start any project with <0.8.19 as multiple products already use it. If it wasn't safe, we would not be using it

RensR · 2023-10-27T11:53:07Z

Could we add a section on the topic of whether to prefer wrapped gas tokens vs. native? And how to determine the correct ERC-20 addresses?

I think that isn't style related and very much dependent on what is being built and how.

cl-sonarqube-production · 2023-10-27T12:15:16Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

solidity style guide 2.0

8278e86

RensR marked this pull request as ready for review October 18, 2023 14:38

RensR requested review from se3000 and connorwstein as code owners October 18, 2023 14:38

DeividasK reviewed Oct 23, 2023

View reviewed changes

yosriady reviewed Oct 23, 2023

View reviewed changes

austinborn reviewed Oct 23, 2023

View reviewed changes

contracts/STYLE_GUIDE.md Outdated Show resolved Hide resolved

DeividasK reviewed Oct 23, 2023

View reviewed changes

contracts/STYLE_GUIDE.md Outdated Show resolved Hide resolved

austinborn reviewed Oct 23, 2023

View reviewed changes

contracts/STYLE_GUIDE.md Outdated Show resolved Hide resolved

jinhoonbang reviewed Oct 25, 2023

View reviewed changes

Oozyx reviewed Oct 25, 2023

View reviewed changes

split into rules and guidelines

494fab2

improve layout

8540ab1

makramkd approved these changes Nov 2, 2023

View reviewed changes

RensR added this pull request to the merge queue Nov 10, 2023

Merged via the queue into develop with commit 0918b37 Nov 10, 2023

RensR deleted the solidity-style-guide-2.0 branch November 10, 2023 15:02

dmitry-diginex mentioned this pull request Dec 3, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 dmitry-diginex/chainlink#36

Open

snehaagni mentioned this pull request Jan 24, 2024

release/2.8.0 -> master #11883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solidity style guide 2.0 #10995

solidity style guide 2.0 #10995

RensR commented Oct 18, 2023 •

edited

Loading

github-actions bot commented Oct 18, 2023

DeividasK Oct 23, 2023

RensR Oct 27, 2023

DeividasK Oct 23, 2023

RensR Oct 27, 2023

DeividasK Oct 23, 2023

RensR Oct 27, 2023

DeividasK Oct 23, 2023

RensR Oct 27, 2023

yosriady left a comment

yosriady Oct 23, 2023

RensR Oct 27, 2023

yosriady Oct 23, 2023

RensR Oct 27, 2023

DeividasK Oct 23, 2023

RensR Oct 27, 2023

austinborn commented Oct 23, 2023

jinhoonbang Oct 25, 2023

RensR Oct 27, 2023

jinhoonbang Oct 25, 2023

RensR Oct 27, 2023

Oozyx Oct 25, 2023 •

edited

Loading

RensR Oct 27, 2023

Oozyx Oct 25, 2023

RensR Oct 25, 2023

RensR commented Oct 27, 2023

cl-sonarqube-production bot commented Oct 27, 2023


		### Comments

		- Comments should be in the `//` (default) or `///` (natspec) format, not the `/* */` format.


		### 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.


		### 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.

solidity style guide 2.0 #10995

solidity style guide 2.0 #10995

Conversation

RensR commented Oct 18, 2023 • edited Loading

github-actions bot commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosriady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinborn commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oozyx Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RensR commented Oct 27, 2023

cl-sonarqube-production bot commented Oct 27, 2023

RensR commented Oct 18, 2023 •

edited

Loading

Oozyx Oct 25, 2023 •

edited

Loading