HardlyDifficult
commented
1 year ago Missing checks for address(0x0)
Invalid. If the address provided is address(0) then the if directly above this will revert.
Implementation of the comment spec is confusing
Invalid. There is a natspec comment already included that explains these odd functions and why it was done this way.
Use of magic numbers is confusing
Valid NC feedback, will fix. Both examples did have a comment explaining the number but a constant may be more clear.
Missing indexed event parameters
Invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.
Missing natspec comments
Fair feedback -- for natspec we aim for complete coverage of the public interfaces but for internal/private/libraries we have some gaps in order to reduce redundancy, for those we aim to include comments where things are unclear or not obvious from the function and variable names.
@inheritdoc is a natspec supported standard that effectively inlines comments from another file -- for those examples we should have complete coverage already.
Bad order of code
Agree, will move that modifier up.
Maximum line length exceeded
Fair however we use a limit of 120 instead. I believe this is more readable since it prevents excessive wrapping, particularly in comment. This is enforced by our linter.
See the markdown file with the details of this report here.