Skip to content

Latest commit

 

History

History
181 lines (118 loc) · 13.3 KB

ReyAdmirado-Q.md

File metadata and controls

181 lines (118 loc) · 13.3 KB

QA:

issue
1 typo
2 use of floating pragma
3 require()/revert() statements should have descriptive reason strings or custom error
4 constants should be defined rather than using magic numbers
5 inconsistency in comments
6 lines are too long
7 open todos
8 Outdated compiler version
9 Use Underscores for Number Literals
10 event is missing indexed fields
11 variable is never used in the contract so it may confuse the reader of code, consider commenting it or removing it to improve readability
12 require() should be used instead of assert()
13 abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
14 use bool instead of making 2 constants to improve readability
15 2** - 1 should be re-written as type(uint).max

1. typo

Cotract --> Contract

addreses --> addresses

DiamondCutProposalCancelation --> DiamondCutProposalCancellation (event name)

recurisiveAggregationInput --> recursiveAggregationInput (many uses in Executor.sol first is shown)

_blockAuxilaryOutput --> _blockAuxiliaryOutput

refference --> reference

exect

initiailize --> initialize

2. use of floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

3. require()/revert() statements should have descriptive reason strings or custom error

4. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

5. inconsistency in comments

6. lines are too long

Usually lines in source code are limited to 80 characters. Its advised to keep lines lower than 120 characters. Today’s screens are much larger so it’s reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

7. open todos

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

8. Outdated compiler version

Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs

9. Use Underscores for Number Literals

There are multiple occasions where certain numbers have been hardcoded, either in variables or in the code itself. Large numbers can become hard to read.

10. event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it’s not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

11. variable is never used in the contract so it may confuse the reader of code, consider commenting it or removing it to improve readability

12. require() should be used instead of assert()

Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction’s available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that “The assert function creates an error of type Panic(uint256). … Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix”.

13. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode instead If all the arguments are strings bytes.concat() should be used

14. use bool instead of making 2 constants to improve readability

although it uses more gas it will lower readability

15. 2** - 1 should be re-written as type(uint).max

Earlier versions of solidity can use uint(-1) instead. Expressions not including the - 1 can often be re-written to accommodate the change