From 38d8245384b183cae75feed41bc909f5a41c3bb1 Mon Sep 17 00:00:00 2001 From: Andrei Radu <47434163+AndreiMVP@users.noreply.github.com> Date: Sat, 4 Jun 2022 19:48:16 +0200 Subject: [PATCH 1/3] Update Solidity good practices --- contribution-guidelines/code-style-and-guidelines/solidity.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contribution-guidelines/code-style-and-guidelines/solidity.md b/contribution-guidelines/code-style-and-guidelines/solidity.md index 2c9d7d7..cd410e1 100644 --- a/contribution-guidelines/code-style-and-guidelines/solidity.md +++ b/contribution-guidelines/code-style-and-guidelines/solidity.md @@ -70,7 +70,9 @@ This and the security risks of smart contracts that interact with other smart co * **Convincibility**: Write code that allows other parties to be easily convinced that it works. Remember that more time is spent in reviews/audits/bug bounties than in actually writing the code. * **Reusability**: Write code that is generic enough to avoid starting from scratch every time. This should not hinder the 3 previous priorities. * Do not **over-engineer**. Over-engineering lowers security, increases gas costs, and decreases convincibility. [Kiss](https://en.wikipedia.org/wiki/KISS_principle) ♥. +* Keep the code in one place as much as possible. Don't use separate functions unless it saves on code duplication/gas costs. Inheritance from abstract contracts is usually not recommended as it might make it tedious for the reviewer to check all the inherited code when reviewing a contract. OpenZeppelin contracts are ok to use as they are deemed secure, although they are often over-engineered. +* It is easier for the reviewers to check the code they are used to so it is better to be conservative with the code you write. Having unusual code arhitectures or using new Solidity features is usually not recommended, unless it provides a clear benefit over the altered difficulty of reviewing. * Smart contracts should only do what is **required, no more, no less**. Do not abstract contracts into more generic contracts if the deployed version will only use a subset of the functionality. E.g. if you can only have 2 parties in your contract, only write code for 2 parties. Do not write code supporting an arbitrary number of parties and then set it to 2 at deployment. This is a stark contrast to traditional software where generic abstractions can save time in the long run. In smart contract development, there is no continuous deployments and the additional time spent in security practices that a generic contract requires far outweighs the potential benefit of its reusability. * However, **abstraction** can be useful when contracts need to interact with contracts which are **not known in advance**, either because they won't be finished by the time of deployment or because they will be developed by other projects you don't have control over. [**ERC20**](https://github.com/ethereum/EIPs/issues/20) and [**ERC792**](https://github.com/ethereum/EIPs/issues/792) are good examples of this. -* Contrary to what the compiler warnings may tell you, **send** is generally more secure than **transfer** as **transfer** allows the recipient to block the call. Don't use **transfer** unless there is a good reason to. +* Contrary to what the compiler warnings may tell you, **send** is generally more secure than **transfer** as **transfer** allows the recipient to block the call. Don't use **transfer** unless there is a good reason to. Sending ETH using send is also more secure than call as well as it takes away some of the stress of possible frontrunning by limiting the gas. From 0697b8c233bf3375fc487e34fc7632717a233c14 Mon Sep 17 00:00:00 2001 From: Andrei Radu <47434163+AndreiMVP@users.noreply.github.com> Date: Sat, 4 Jun 2022 19:53:37 +0200 Subject: [PATCH 2/3] Added bold style to some parts --- .../code-style-and-guidelines/solidity.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contribution-guidelines/code-style-and-guidelines/solidity.md b/contribution-guidelines/code-style-and-guidelines/solidity.md index cd410e1..9864adf 100644 --- a/contribution-guidelines/code-style-and-guidelines/solidity.md +++ b/contribution-guidelines/code-style-and-guidelines/solidity.md @@ -70,9 +70,9 @@ This and the security risks of smart contracts that interact with other smart co * **Convincibility**: Write code that allows other parties to be easily convinced that it works. Remember that more time is spent in reviews/audits/bug bounties than in actually writing the code. * **Reusability**: Write code that is generic enough to avoid starting from scratch every time. This should not hinder the 3 previous priorities. * Do not **over-engineer**. Over-engineering lowers security, increases gas costs, and decreases convincibility. [Kiss](https://en.wikipedia.org/wiki/KISS_principle) ♥. -* Keep the code in one place as much as possible. Don't use separate functions unless it saves on code duplication/gas costs. Inheritance from abstract contracts is usually not recommended as it might make it tedious for the reviewer to check all the inherited code when reviewing a contract. OpenZeppelin contracts are ok to use as they are deemed secure, although they are often over-engineered. -* It is easier for the reviewers to check the code they are used to so it is better to be conservative with the code you write. Having unusual code arhitectures or using new Solidity features is usually not recommended, unless it provides a clear benefit over the altered difficulty of reviewing. +* **Keep the code in one place as much as possible**. Don't use separate functions unless it saves on code duplication/gas costs. Inheritance from abstract contracts is usually not recommended as it might make it tedious for the reviewer to check all the inherited code when reviewing a contract. OpenZeppelin contracts are ok to use as they are deemed secure, although they are often over-engineered. +* It is easier for the reviewers to check the code they are used to so it is better to **be conservative with the code you write**. Having unusual code arhitectures or using new Solidity features is usually not recommended, unless it provides a clear benefit over the altered difficulty of reviewing. * Smart contracts should only do what is **required, no more, no less**. Do not abstract contracts into more generic contracts if the deployed version will only use a subset of the functionality. E.g. if you can only have 2 parties in your contract, only write code for 2 parties. Do not write code supporting an arbitrary number of parties and then set it to 2 at deployment. This is a stark contrast to traditional software where generic abstractions can save time in the long run. In smart contract development, there is no continuous deployments and the additional time spent in security practices that a generic contract requires far outweighs the potential benefit of its reusability. * However, **abstraction** can be useful when contracts need to interact with contracts which are **not known in advance**, either because they won't be finished by the time of deployment or because they will be developed by other projects you don't have control over. [**ERC20**](https://github.com/ethereum/EIPs/issues/20) and [**ERC792**](https://github.com/ethereum/EIPs/issues/792) are good examples of this. -* Contrary to what the compiler warnings may tell you, **send** is generally more secure than **transfer** as **transfer** allows the recipient to block the call. Don't use **transfer** unless there is a good reason to. Sending ETH using send is also more secure than call as well as it takes away some of the stress of possible frontrunning by limiting the gas. +* Contrary to what the compiler warnings may tell you, **send** is generally more secure than **transfer** as **transfer** allows the recipient to block the call. Don't use **transfer** unless there is a good reason to. Sending ETH using **send** is also more secure than **call** as well as it takes away some of the stress of possible frontrunning by limiting the gas. From 82335a98814ad20ca61b36ad54dcb6934820b1cd Mon Sep 17 00:00:00 2001 From: Andrei Radu <47434163+AndreiMVP@users.noreply.github.com> Date: Tue, 7 Jun 2022 13:50:32 +0200 Subject: [PATCH 3/3] Update solidity.md --- contribution-guidelines/code-style-and-guidelines/solidity.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contribution-guidelines/code-style-and-guidelines/solidity.md b/contribution-guidelines/code-style-and-guidelines/solidity.md index 9864adf..8694e2e 100644 --- a/contribution-guidelines/code-style-and-guidelines/solidity.md +++ b/contribution-guidelines/code-style-and-guidelines/solidity.md @@ -71,7 +71,7 @@ This and the security risks of smart contracts that interact with other smart co * **Reusability**: Write code that is generic enough to avoid starting from scratch every time. This should not hinder the 3 previous priorities. * Do not **over-engineer**. Over-engineering lowers security, increases gas costs, and decreases convincibility. [Kiss](https://en.wikipedia.org/wiki/KISS_principle) ♥. * **Keep the code in one place as much as possible**. Don't use separate functions unless it saves on code duplication/gas costs. Inheritance from abstract contracts is usually not recommended as it might make it tedious for the reviewer to check all the inherited code when reviewing a contract. OpenZeppelin contracts are ok to use as they are deemed secure, although they are often over-engineered. -* It is easier for the reviewers to check the code they are used to so it is better to **be conservative with the code you write**. Having unusual code arhitectures or using new Solidity features is usually not recommended, unless it provides a clear benefit over the altered difficulty of reviewing. +* It is easier for the reviewers to check the code they are used to so it is better to **be conservative with the code you write**. Having unusual code arhitectures or using new Solidity features is usually not recommended (e.g. custom errors), unless it provides a clear benefit over the altered difficulty of reviewing. * Smart contracts should only do what is **required, no more, no less**. Do not abstract contracts into more generic contracts if the deployed version will only use a subset of the functionality. E.g. if you can only have 2 parties in your contract, only write code for 2 parties. Do not write code supporting an arbitrary number of parties and then set it to 2 at deployment. This is a stark contrast to traditional software where generic abstractions can save time in the long run. In smart contract development, there is no continuous deployments and the additional time spent in security practices that a generic contract requires far outweighs the potential benefit of its reusability. * However, **abstraction** can be useful when contracts need to interact with contracts which are **not known in advance**, either because they won't be finished by the time of deployment or because they will be developed by other projects you don't have control over. [**ERC20**](https://github.com/ethereum/EIPs/issues/20) and [**ERC792**](https://github.com/ethereum/EIPs/issues/792) are good examples of this. * Contrary to what the compiler warnings may tell you, **send** is generally more secure than **transfer** as **transfer** allows the recipient to block the call. Don't use **transfer** unless there is a good reason to. Sending ETH using **send** is also more secure than **call** as well as it takes away some of the stress of possible frontrunning by limiting the gas.