Review Tasks

[RobinNagpal]

Structure of Contracts

• [ ] Check order of structs, events, errors, variables, modifiers and functions follows the convention for MarketUpdateProposer.sol
• [ ] Check order of structs, events, errors, variables, modifiers and functions follows the convention for MarketUpdateTimelock.sol
• [ ] Check order of structs, events, errors, variables, modifiers and functions follows the convention for Configurator.sol
• [ ] Check order of structs, events, errors, variables, modifiers and functions follows the convention for ConfiguratorStorage.sol
• [ ] Check order of structs, events, errors, variables, modifiers and functions follows the convention for CometProxyAdmin.sol

Check Variable and function names

• [ ] Check variable names follows the convention for MarketUpdateProposer.sol
• [ ] Check variable names follows the convention for MarketUpdateTimelock.sol
• [ ] Check variable names follows the convention for Configurator.sol
• [ ] Check variable names follows the convention for ConfiguratorStorage.sol
• [ ] Check variable names follows the convention for CometProxyAdmin.sol

Check for events, errors and requires

Things to check

• events are emitted where needed
• All validations are in place with proper require or revert
• Check there are 1-2 tests for each validation
• Check the message for require statement

Checklist

• [ ] Check events, errors and requires follows the convention for MarketUpdateProposer.sol
• [ ] Check events, errors and requires follows the convention for MarketUpdateTimelock.sol
• [ ] Check events, errors and requires follows the convention for Configurator.sol
• [ ] Check events, errors and requires follows the convention for ConfiguratorStorage.sol
• [ ] Check events, errors and requires follows the convention for CometProxyAdmin.sol

Check order of Arguments (what is the best order)

• [ ] MarketUpdateProposer.sol - constructor(address governor, address marketAdmin, address proposalGuardian, ITimelock timelock)
• [ ]

[MSamiTariq]

Checklist of things that conform to the styling guide as mentioned in https://docs.soliditylang.org/en/latest/style-guide.html

• [x] Use 4 spaces per indentation level

• [x] Surround top level declarations in Solidity source with two blank lines

• [x] Within a contract surround function declarations with a single blank line.

• [x] Maximum suggested line length is 120 characters.

• [x] Import statements should always be placed at the top of the file.

• [ ] Functions should be grouped according to their visibility and ordered:

  1. constructor
  2. receive function (if exists)
  3. fallback function (if exists)
  4. external
  5. public
  6. internal
  7. private

• [x] Whitespace in Expressions

• Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations

• Immediately before a comma, semicolon

• More than one space around an assignment or other operator to align with another

• Do not include a whitespace in the receive and fallback functions

• [x] The braces denoting the body of a contract, library, functions and structs should:

• open on the same line as the declaration

• close on their own line at the same indentation level as the beginning of the declaration.

• The opening brace should be preceded by a single space.

• [ ] The modifier order for a function should be:

  1. Visibility
  2. Mutability
  3. Virtual
  4. Override
  5. Custom modifiers

• [x] For long function declarations, it is recommended to drop each argument onto its own line at the same indentation level as the function body

• [x] In variable declarations, do not separate the keyword mapping from its type by a space. Do not separate any nested mapping keyword from its type by whitespace. (Non-conformity at some places)

• [x] Declarations of array variables should not have a space between the type and the brackets.

• [x] Contract elements should be laid out in the following order:

  1. Pragma statements
  2. Import statements
  3. Events
  4. Errors
  5. Interfaces
  6. Libraries
  7. Contracts

• [ ] Inside each contract, library or interface, use the following order:

  1. Type declarations (struct, enums)
  2. State variables
  3. Events
  4. Errors
  5. Modifiers
  6. Functions

• [x] Contracts and libraries should be named using the CapWords style.

• [x] Contract and library names should also match their filenames.

• [x] Structs should be named using the CapWords style.

• [x] Events should be named using the CapWords style.

• [x] Functions should use mixedCase.

• [x] Function arguments should use mixedCase.

• [x] Local and State Variable Names Use mixedCase.

• [x] Modifiers use mixedCase.

• [x] Constants should be named with all capital letters with underscores separating words.

• [x] Enums, in the style of simple type declarations, should be named using the CapWords style.

[MSamiTariq]

Solidity recommends the following three points for ordering of functions, modifier and contracts:

1. Inside each contract, library or interface, use the following order:

• Type declarations (struct, enums)
• State variables
• Events
• Errors
• Modifiers
• Functions

2. Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private

3. The modifier order for a function should be:

• Visibility
• Mutability
• Virtual
• Override
• Custom modifiers

Type declarations (enums, structs) are always used at the top level followed by state variables. There are only a few exceptions to this

Found this anomaly in Comet.sol where a constructor is defined before a modifier

Found this pattern of constructor before modifier in Ownable.sol and TransparentUpgradableProxy.sol too

Since Compound almost always follows this convention, we too can follow this.

At some places, Compound is declaring errors before events, though the recommended way is events before errors. Although there is no clear pattern used by Compound regarding this

For the grouping of functions:

Constructors are always at top except for the cases I already mentioned above where a constructor is used before a modifier.

Receive function should be declared before fallback, but anomaly was detected

There is also a case where the receive function was found at the end of the contract

Another case where receive function is used between other functions

Similarly found the fallback function defined at the end of the contract

At few places, Compound defines fallback at the end of the contract, but at some places, it follows the convention recommended by Solidity like in the following case

Solidity recommends to group external functions before internal, but Compound doesn't follow this convention

There is no strict convention that Compound follows regarding external functions before internal or vice versa. It basically follows a mixture.

Similar is the case for public functions, these should be placed after external functions before internal, but compound doesn't follow a strict convention for this either

Similarly, private functions should be placed at the end, but compound doesn't follow any convention there either

Next we analyze the modifier order in functions

According to Solidity's recommended way, visibility should always be the first modifier, but in Compound, I noticed that override is the first modifier:

Next is that mutability should come after visibility, and Compound follows this convention

Next is that custom modifier should always come last, and Compound also follows this convention