Added Documentation as per NatSpec

[remedcu]

Fixes #

• Added Documentation as per NatSpec to ERC165 and IERC165

[nventuro]

Hello @remedcu, thanks for contributing!

Unfortunately, we don't follow NatSpec on our docs for a variety of reasons. Instead, our documentation is parsed by https://github.com/OpenZeppelin/solidity-docgen and used to generate our documentation website. The following for example is the page for ERC165, generated from that file: https://docs.openzeppelin.com/contracts/3.x/api/introspection#ERC165

I'll be closing this since we don't intend un supporting NatSpec soon. Thank you very much though!

[frangio]

@remedcu Can you share with us in what context you use NatSpec and if there's anything in particular you think we should improve about our existing documentation?

[remedcu]

@frangio I was reviewing another contract which was importing ERC165 and IERC165. This was my first-time seeing about this ERC, so any documentation I can get to understand this is an excellent aid for me. I followed the NatSpec, as it was in the Solidity documentation, but didn't know about the solidity-docgen of OpenZeppelin.

[fulldecent]

The benefit of NatSpec is that it is an industry standard, the compiler supports it, and wallets can be made to parse contracts and show all relevant information at the time a transaction is signed.

This is super important. And that's why the Solidity project has recommend:

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

This recommendation has been in place before this issue was opened.

---

I understand that OpenZeppelin has a conflicting interest to drive traffic and search engine optimization to its commercial website. (This is the same domain that hosts OpenZeppelin Contracts documentation.)

This policy is at the direct expense of the security of people using smart contracts and I do not support OZ's policy here.

[frangio]

@fulldecent We would really appreciate if you could generally be a little more understanding and kind towards our efforts and less suspicious of our so called "conflicting interests". OpenZeppelin Contracts is an open source project that we maintain as a good for the community. Period.

docs.openzeppelin.com is not our "commercial website", it's quite simply where we host the documentation... Would you prefer we hosted on readthedocs.io? This comment makes no sense to me. I'm not at all an SEO expert, and I couldn't care less, but I believe subdomains like the one we use for docs do not contribute to SEO for the company's website. I don't event want to engage in this conversation because it's completely beside the point.

As any open source project, we do need to get funding somewhere. OpenZeppelin the company is funding the project, and it is a for profit company that offers services that complement OpenZeppelin Contracts that you will see mentioned throughout the documentation.

This is not motivated by any sort of ulterior plan. We will never ever compromise security for the commercial benefit of the company. The insinuation is honestly really offensive.

We are using NatSpec. We are not using NatSpec in the best way possible. There are many things on our plate and fixing this is not at the top of the list in terms of priorities.

[fulldecent]

Sorry for the negative tone. Negative doesn't build communities.

docs.openzeppelin.com is a great place to host documents. And having a company support an open product is good—that's the best way it stays supported.

Back to the issue.

NatSpec is the place where we specify typically in (only, unfortunately) English what a function is promised to do. Every time you use MetaMask or some other transaction processor to sign a transaction, this message should be shown to the person that is signing the transaction.

At current (in the places I'm looking) this message is used to direct people to the OZC website for documentation. It will be much more helpful, and following the intent of NatSpec, if every function, parameter and return value was documented with NatSpec tags without requiring people to visit OZC website which is fragile and may not have the intended information there anyway by the time somebody gets there. Full NatSpec support promotes security for people creating transactions on smart contracts.

---

When a maintainer here closes-without-merge a PR that is focused, additive, and solves a problem that I care about, then I want to understand why. If I can solve the problem here (specifically, "don't intend [on] supporting NatSpec", "used to generate [only] our documentation website" and other "variety of reasons") then it allows me to clear the path for more PRs that will fix NatSpec issues.

It was cynical of me to assume that the "variety of reasons" specifically means "to increase traffic to our website". I'm sorry for sharing this and I should have kept those thoughts to myself. And more importantly, I don't care what the motive was, I only care that this is fixed going forward, i.e. complete NatSpec support for all functions, enforced by linters.

As always, I don't expect you (personally), you (people with commit access) or we (including me) to attribute any kind of priority to this or any of my comments. A main reason I use GitHub Issues is to discuss things to learn what appetite there is to accept PRs.

[nventuro]

It should be noted that the NatSpec tag OZ Contracts uses is @dev, which is intended to 'explain to a developer any extra details'. @dev is additionally picked up by solidity-docgen, which then automatically builds the documentation site.

In that sense, the usage here is not wrong. If anything, what is lacking is usage of @notice - arguably not a priority due to poor support by user-facing tools.

[frangio]

Also lacking are @param annotations which is what this PR added. Does MetaMask show this information?

---

There is no specific limitation in solidity-docgen to use @dev only. That's what our template uses.

[nventuro]

There is no specific limitation in solidity-docgen to use @dev only. That's what our template uses.

@dev was used because solc refuses to compile (!!!) if unknown tags are used. 0.8 apparently supports a @custom tag, but that isn't present in e.g. 0.7.0. We chose @dev because we had to pick one from their list, and it seemed the most appropriate one.

[fulldecent]

Solc does parse @param tags, currently MetaMask does not show them. Honestly it looks like a chicken-and-egg problem. But really it's a MetaMask-is-a-glacier problem. It took them years to recognize ERC-721 transactions as distinct from ERC-20. But I expect they will support @param parsing someday.