-
Notifications
You must be signed in to change notification settings - Fork 33
Description
Contract-level documentation is user-visible (because people read Etherscan) and NatSpec specifies that it can be shown to users. Every contract should explain what it does cleary.
For contracts that are deployed (i.e. not abstracts and mix-ins) the top-level documentation should be spectacular.
Recommendation: update contract so that when the compiled contract is opened in Etherscan it will look like a million-dollar effort went into it.
Here is some artwork to get you started:
┌───────────────────────┐
┌─────│ isOpen │─────┬───────────────┐
▼ └───────────────────────┘ │ │
┌───────────────────────┐ │ │
│ onlyStakeDeposited │───────────────────────────────────────┤
└───────────────────────┘ ┌───────────────────────┐ │
│ │ onlyPaymentDeposited │───┤
│ └───────────────────────┘ │
└─────────────────┐ │ │
├─────────────────┘ │
▼ ▼
┌───────────────────────┐ ┌───────────────────────┐
│ isDeposited │────────▶│ isCancelled │
└───────────────────────┘ └───────────────────────┘
│
▼
┌───────────────────────┐
│ isFinalized │
└───────────────────────┘
And if you want to restrict to ASCII:
+-----------------------+
+-----| isOpen |-----+---------------+
v +-----------------------+ | |
+-----------------------+ | |
| onlyStakeDeposited |---------------------------------------+
+-----------------------+ +-----------------------+ |
| | onlyPaymentDeposited |---+
| +-----------------------+ |
+-----------------+ | |
+-----------------+ |
v v
+-----------------------+ +-----------------------+
| isDeposited |-------->| isCancelled |
+-----------------------+ +-----------------------+
|
v
+-----------------------+
| isFinalized |
+-----------------------+
References:
-
Su Squares introduction: https://github.com/su-squares/ethereum-contract/blob/master/contracts/ALLINONE.sol
-
Function-level documentation is incomplete
NatSpec documentation is provided for most functions, that’s great!
The
@noticetag for functions is user-visible documentation. So it should be included for every public function. In the future, wallets and Etherscan will show these notes to end-users when transactions are being signed.Recommendation: achieve 100% public function-level NatSpec coverage (including public variables which are implicitly functions).
Additional notes: as developers we often think “this function name will be obvious to all users”. But the function
isOpen()refers to only a specific state of theCountdownGriefingEscrowcontract. It is very reasonable for someone to look at that and think that all non-terminal states would count as true forisOpen()References:
- One missing documentation:
function isOpen() public view returns (bool validity) {
- One missing documentation: