ghoul-sol / treasure-staking

10 stars 1 forks source link

⚠️ NatSpec documentation is missing #40

Open fulldecent opened 2 years ago

fulldecent commented 2 years ago

NatSpec documentation allows wallets and other Web3 applications to inform a human interacting with the contract what they are about to do. They may see "are you sure you want to call cancelListingwith _nftAddress 0x232398473... and tokenID 11" rather than something like "do you want to call 0x5cd346fa with message data 0x0273eecfb480dcfbab423bd15c71e7ad70c7a9504689ff56edb137a5d37146d6".

Best practice is to include documentation for all public interfaces and this is recommended by the Solidity project:

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI).

https://docs.soliditylang.org/en/v0.8.6/style-guide.html?highlight=natspec

Currently several functions do not have complete NatSpec documentation. And importantly, some important parameter documentation is missing, such as IExtractorStakingRules.canReplace.

Lack of detailed function-level documentation (e.g. NatSpec) increases the risk of errors when creating/reviewing the implementation and errors when blockchain participants interact directly with the contract using external tools.

Recommendation: implement NatSpec for every public, internal, and external function.