Refactor ERC721 to use _update pattern #4419

frangio · 2023-07-04T05:01:38Z

A simpler alternative to #4377 using:

function _update(address from, address to, uint256 tokenId) internal;

_burn now has a from argument.

Fixes LIB-628

changeset-bot · 2023-07-04T05:01:41Z

⚠️ No Changeset found

Latest commit: cdacda1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Major

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Amxx · 2023-07-04T08:47:21Z

contracts/token/ERC721/ERC721.sol

+    function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual {
+        if (!_isApproved(owner, spender, tokenId)) {
+            address actualOwner = _ownerOf(tokenId);
+            if (owner == actualOwner) {
+                revert ERC721InsufficientApproval(spender, tokenId);
+            } else {
+                revert ERC721IncorrectOwner(owner, tokenId, actualOwner);
+            }
+        }


This function checks the approval assuming owner is correct, and if that returns false, it gets the actual owner to throw the error. So the argument is considered correct in one case, and not correct in the other ?

I'm not really confortable with that.

I mainly did this to get the current tests to pass, I don't know if this is necessary.

Amxx · 2023-07-04T08:49:38Z

contracts/token/ERC721/ERC721.sol

-        owner = ownerOf(tokenId);
-        if (owner != from) {
-            revert ERC721IncorrectOwner(from, tokenId, owner);
+    function _update(address from, address to, uint256 tokenId) internal virtual {


I'm not a fan of having both from and tokenId in the arguments. when you say which token you want to transfer, it should be clear that its from the current owner. My worry is that devs will want to call that internally, and they'll have to get _ownerOf, just to pass it to this function, that will then check it.

In particular, that is true of burn:

When you want to burn a token, you now have to get the owner using one sload, and then verify that you got the correct one by duplicating this sload.

ernestognw · 2023-07-06T21:05:52Z

contracts/interfaces/draft-IERC6093.sol

     */
-    error ERC721IncorrectOwner(address sender, uint256 tokenId, address owner);
+    error ERC721IncorrectOwner(address claimedOwner, uint256 tokenId, address actualOwner);


Would this change also be needed in the EIP itself? I'm worried there's a table with definitions there that might need updating.

Possibly. I made this change because I found the existing names confusing.

I see how the existing names are confusing, but I'd look for a way in which the naming makes sense for all errors. Would it be clear if we rename sender completely to from?

ernestognw · 2023-07-06T21:29:08Z

contracts/token/ERC721/ERC721.sol

+    /**
+     * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override.
+     */
+    function _updateBalance(address account, uint128 value) internal virtual {


I would push a bit for adding _unsafe* prefix. The NatSpec itself mentions its unsafe and I'm not sure if the name correctly reflects that. What do you think?

Also, why not _increaseBalance? The function gives the sense of being a getter.

The natspec is outdated. This function should not be described as unsafe. It will never lead to overflow.

I don't understand why updateBalance would sound like it's a getter. increaseBalance is good too, but I thought the parallel with _update made sense.

I don't understand why updateBalance would sound like it's a getter.

My bad, I meant it sounds like a setter instead, not a value increase.

increaseBalance is good too, but I thought the parallel with _update made sense.

Makes sense as long as the NatSpec correctly reflects what it does. I changed it for _increaseBalance because I'm strongly in favor of that name. We can discuss it.

contracts/token/ERC721/ERC721.sol

ernestognw · 2023-07-06T22:17:43Z

contracts/token/ERC721/extensions/ERC721Pausable.sol

    ) internal virtual override {
-        super._beforeTokenTransfer(from, to, firstTokenId, batchSize);
-
        _requireNotPaused();


I think it's a good moment to use whenNotPaused instead as suggested here

Personally I prefer the function call. They are functionally equivalent but the function call is visible inside the function, I just find it more explicit. I don't know if everyone feels the same though.

I prefer whenNotPaused precisely because it's less explicit (== less pervasive?).

I'm fine with the function call if we agree to keep consistency between this and ERC1155Pausable where we use the modifier instead.

contracts/token/ERC721/extensions/ERC721URIStorage.sol

Co-authored-by: Ernesto García <[email protected]>

Amxx · 2023-07-08T13:52:12Z

contracts/token/ERC721/ERC721.sol

-            revert ERC721InsufficientApproval(_msgSender(), tokenId);
-        }
-
+    function transferFrom(address from, address to, uint256 tokenId) public {


Why are we removing virtual from these function ?

Same reason for _transfer but I suppose the public functions have other considerations. Do you think they should remain virtual?

yes.

Our guidelines are to make (almost) everything virtual. I'm ok to making _transfer, _mint and _burn non virtual, because they can be bypassed by calling _update directly. But apart from these 3, everything else should remain virtual IMO.

frangio · 2023-07-12T03:06:10Z

Closing in favor of #4377.

Refactor ERC721 to use _update pattern

2c9f7bb

frangio requested a review from Amxx July 4, 2023 05:01

Amxx reviewed Jul 4, 2023

View reviewed changes

frangio mentioned this pull request Jul 4, 2023

Use the _update mechanism in ERC721 #4377

Merged

6 tasks

ernestognw reviewed Jul 6, 2023

View reviewed changes

frangio and others added 6 commits July 6, 2023 21:42

Update contracts/token/ERC721/ERC721.sol

930ee64

Co-authored-by: Ernesto García <[email protected]>

Apply suggestions

14ffe4d

Rename updateBalance to increaseBalance

63717fc

Fix tests (omg hopefully)

5f05fc9

Add changeset

38c0bf0

Use _increaseBalance in _update

ca05bc5

ernestognw marked this pull request as ready for review July 7, 2023 20:41

Rollback _increaseBalance in _update

cdacda1

Amxx reviewed Jul 8, 2023

View reviewed changes

frangio closed this Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ERC721 to use _update pattern #4419

Refactor ERC721 to use _update pattern #4419

frangio commented Jul 4, 2023 •

edited

Loading

changeset-bot bot commented Jul 4, 2023 •

edited

Loading

Amxx Jul 4, 2023

frangio Jul 4, 2023

Amxx Jul 4, 2023

Amxx Jul 4, 2023

ernestognw Jul 6, 2023

frangio Jul 7, 2023

ernestognw Jul 7, 2023

ernestognw Jul 6, 2023

frangio Jul 7, 2023 •

edited

Loading

ernestognw Jul 7, 2023

ernestognw Jul 6, 2023

frangio Jul 7, 2023

ernestognw Jul 7, 2023

Amxx Jul 8, 2023

frangio Jul 8, 2023

Amxx Jul 8, 2023

frangio commented Jul 12, 2023

Refactor ERC721 to use _update pattern #4419

Refactor ERC721 to use _update pattern #4419

Conversation

frangio commented Jul 4, 2023 • edited Loading

changeset-bot bot commented Jul 4, 2023 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio commented Jul 12, 2023

frangio commented Jul 4, 2023 •

edited

Loading

changeset-bot bot commented Jul 4, 2023 •

edited

Loading

frangio Jul 7, 2023 •

edited

Loading