Openzeppelin-contracts: Change ERC721Token `private` functions to `internal`

[ryana]

🎉 Description

I think the functions inside of the ERC721Token class should not be private but rather internal.

• [ ] 🐛 This is a bug report.
• [x] 📈 This is a feature request.

💻 Environment

• v1.6.0
• Rinkeby
• Truffle

📝 Details

Hi! This weekend I used your ERC721 library to build something at ETHDenver. I had a novel use case for ERC721 tokens that required a small bit of nonstandard behavior. For reasons I will explain in depth below, I needed to allow the contract owner, not just the token owner, to burn a token.

This required me to override this:

  function _burn(uint256 _tokenId) onlyOwnerOf(_tokenId) internal {
    if (approvedFor(_tokenId) != 0) {
      clearApproval(msg.sender, _tokenId);
    }
    removeToken(msg.sender, _tokenId);
    Transfer(msg.sender, 0x0, _tokenId);
  }

to this:

  function _burn(uint256 _tokenId) internal {
    address ownerOfToken = ownerOf(_tokenId);

    if (approvedFor(_tokenId) != 0) {
      clearApproval(ownerOfToken, _tokenId);
    }
    removeToken(ownerOfToken, _tokenId);
    Transfer(ownerOfToken, 0x0, _tokenId);
  }

The problem I ran into is that since clearApproval and removeToken are private, I couldn't just do this. I had to copy in the whole contract (http://github.com/ryana/ethden-eth-gateway/blob/master/contracts/QuickEth.sol)

I don't understand why anything should be private except for the data. i.e.:

  // Total amount of tokens
  uint256 private totalTokens;

  // Mapping from token ID to owner
  mapping (uint256 => address) private tokenOwner;

  // Mapping from token ID to approved address
  mapping (uint256 => address) private tokenApprovals;

  // Mapping from owner to list of owned token IDs
  mapping (address => uint256[]) private ownedTokens;

  // Mapping from token ID to index of the owner tokens list
  mapping(uint256 => uint256) private ownedTokensIndex;

So I am proposing this is changed. I am happy to provide the PR if you all agree.

👍 Other Information

I know that this will probably illicit some "why would you want to do this?"-type responses, so to give you a little more context, the general idea is that I was building a small escrow service that gives merchants who want to get paid in crypto the ability allow their customers to use credit cards. Upon a successful charge, my service would issue an ETH-backed NFT which would be revocable for a certain period of time based on the risk profile of the purchaser. After the revocation window passes, the NFT can be burned and the ETH backing it gets released.

https://bit.ly/quicketh-preso

[carloschida]

As you write in 'Other Information' I don't agree with the change but find your use case useful.
I explain why I don't agree and propose later a workaround to your business case.

Reasons to disagree

These comments are based on private research I undertook for a client for a related project.

Contradiction of the NFT standard EIP-721

I am of the idea that your proposal contradicts the common practices, and understanding of what property is and implies, and thus violates the spirit of the EIP-721 Deed Standard

In the EIP-721 Deed Standard the following is mentioned:

The things to which deeds express ownership are an implementation detail. While writing this standard we considered a diverse univerise of things, and we know you will dream up many more:

Physical property — houses, unique artwork
Digital property — unique pictures of kittens, collectable cards
"Negative value" assets — loans, burdens and other responsibilities

Sticking then to what property means, I refer to the article of Tony Honoré on Wikipedia in German (unluckily the version in English does not contain the text to which I want to refer but I leave a link to the fairly good Google-translated version).
In the table that depicts the 11 rights and obligations of property (as in ownership thereof), one can find:

1. Right to security against expropriation.
2. Duty to prevent damage.

One can easily agree on these two contradicting your proposal in the sense that a non-owner, in the case the issuer, of the token is allowed to destroy the token. Extrapolate this to saying that a construction company has the right to destroy a house even after it sold it.

Lines of credit as contracts contradict (partially) blockchain settlements

In the presentation you shared, you state

It [QuickETH] never issues more NFTs than it has non-spoken-for ETH.

This turns QuickETH into a credit provider. You're are already mitigating some of the risk by saying

If we get a spike in usage, NFTs will not be issues until we can push more ETH into the contract to back the NFTs.

Without getting too philosophical here, I believe that the spirit of settlements on a blockchain is to ensure that only _true_ transactions are recorded. Therefore, providing a line of credit via a NFT that can be traded and later burnt by party exogenous to the transaction, is rather conflicting, even if the tokens have “revocation expiry timestamp”.
If Alice wants to sell Bob a good and they reach a agreement to a price stated in QuickETH's NFT, Alice would need to check the timestamp of each of the tokens Bob has and ponder the risk in which she would incur. This creates a high burden comparable to that of a credit score review.

Business workaround

Assume fully the risk that a credit-line-providing service involves. Grant immediately the amount of ETH they want to acquire charging a commission based on the amount of ETH they want to acquire, your proposed social check, and your own credit experience with them to cover your lending risk and profit. Inasmuch as their credit record evolves adjust the previously mentioned commission.

I hope you find these comments helpful.

[ryana]

Thanks a lot for engaging on this @carloschida. I find these comments very helpful. I'm going to sleep on this and will respond again in the morning.

[ryana]

I understand your point of view with respect to the philosophy behind NFT and I really appreciate you taking the time to enlighten me. Some really great stuff in there I never considered. When I first read your comment last night, I got stuck on the "Negative value" asset example as a perfect example _allowing_ what I'm proposing. But then I realized that even if a loan defaults, the holder of that loan still holds that loan. Nobody should ever take it away from them.

I'm going to close this since I agree now that the way it is implemented is required to stop people from doing exactly what I did, which breaks the idea of deeds :)

---

That being said, I have two more questions which I think others who stumble across this may benefit from:

1. What is the purpose of _burn and _mint having the _ prefix? I thought that meant I SHOULD override these functions, but now as I begin to more fully understand what you're saying, I'm realizing it probably means that I MUST call those at least once in my implementation to conform to the standard. If that's the case, what should I have read to know that?

2. I'm still probably going to deploy a contract like this on mainnet at some point. Clearly it will not be a valid 721 token for the reasons we have discussed. Is there someway I can make my contract signal that? I would like ERC721 wallets to be able to move this asset around, but I would also like to put some sort of asterisk on the token so that people aren't misled. Do any other ERCs discuss how to do that?

Thank you!

[carloschida]

You're most welcome. I'm glad I could be of assistance.

On point 1.
Not so long ago I questioned that my self, because it doesn't always fit the patterns employed in other OOL, and found this discussion useful: What is the purpose of underscores in front of parameters.
Yet I'd stick to the rule 'code is king/law': whether you SHOULD contrasted to whether you MUST or even SHALL NOT becomes a weak reason even when specified in a ERC or alike in comparison to whether you can.

On point 2.
I don't know a way to signal _all_ wallets about the specific nature of your deed (if you decide to implement à la ERC721). Probably you should wait to see how ERC721Metadata evolves (specified somewhere here).
As an additional comment on the business side, you could check out the way Lykke Wallet operates: in their assets directory you can see information related to each token. I could imagine that your business case could use a BlockchainExplorer similar to theirs to see some information at a glance, eg date of issuance, to whom it was originally issued and their credit score, etc.