Use the _update mechanism in ERC721

[Amxx]

Fixes LIB-628

Issues to fix:

• when a batch is minted, no hook is called, so other modules can't react to that
  • ERC721Votes must update the votes accordingly
  • ERC721Enumerable must revert

I fixed these using __unsafe_increaseBalance as a hook. It actually works well for that usecase.

Fixes #4136.
Closes #4405 (superceded).
Closes #4169 (superceded).
Closes #4144 (superceded).

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 7249414

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Major

Not sure what this means? Click here to learn what changesets are.

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

[frangio]

I realized that the constraints are reading msg.sender and I don't think this is good. It goes against the design principle that msg.sender should be used in the external interface, but internally it should be passed as an explicit argument. Users should not and would not expect _update to act differently depending on msg.sender.

I think _update needs a "from" argument, but it doesn't have to be an address, and it doesn't have to be the owner necessarily. I am thinking that this argument should have a user defined type encoding an ADT with one variant for each of the constraints we have, and for the "approved or owner" constraint it should contain the value of msg.sender. (I believe this is essentially a defunctionalization of the constraints.)

As a side note, burn is not in ERC-721, so we are not bound by its current signature and I think we could freely change the internal function to _burn(address from, uint256 tokenId) if it helps.

[Amxx]

I realized that the constraints are reading msg.sender and I don't think this is good. It goes against the design principle that msg.sender should be used in the external interface, but internally it should be passed as an explicit argument. Users should not and would not expect _update to act differently depending on msg.sender.

I think _update needs a "from" argument, but it doesn't have to be an address, and it doesn't have to be the owner necessarily. I am thinking that this argument should have a user defined type encoding an ADT with one variant for each of the constraints we have, and for the "approved or owner" constraint it should contain the value of msg.sender. (I believe this is essentially a defunctionalization of the constraints.)

I would argue that its the constrain that reads msg.sender, not _update itself. So yes, msg.sender can affect the behavior of _update ... but only if the dev asks for it (and it checks according to what the dev requess).

Now I currently see 4 constraints:

• None
• RequireMinted
• RequireNotMinted
• RequireOwnerOrApproved.

We could encode these 4 into an enum, and inside _update do the corresponding logic (calling a private helper?)
It would be less powerfull in terms of which behaviors a dev could implement using constaints ... but maybe you'd be more confortable with that ?

I'm not sure I fully understand what you propose, but it appears to include preparing some encoding ... I wouldn't want the result to be too complex.

[frangio]

msg.sender can affect the behavior of _update ... but only if the dev asks for it

It will be invisible in _transfer and the same argument works there, it's behavior should not depend on the value of msg.sender.

[Amxx]

It will be invisible in _transfer and the same argument works there, it's behavior should not depend on the value of msg.sender.

_transfer does _update(to, tokenId, _constraintMinted) ... and _constraintMinted does not do any msg.sender check.

The only constrain that does a msg.sender check is _constraintApprovedOrOwner, which is only called in the public transferFrom. This refactor does not add any msg.sender check to any function that did not already have one, so I'm not really seeing the issue.

[ernestognw]

Suggested change

	_balances[from] -= 1;
	--_balances[from];

[ernestognw]

Alternatively:

Suggested change

	_balances[from] -= 1;
	_balances[from]--;

[frangio]

I personally prefer -= 1 and += 1. @Amxx what is your preference here?

[Amxx]

+= 1 / -= 1 is what we currently have, and I kept it. If I was to write the contract from scratch today, I would use -- and ++, but I don't have a strong preference.

[ernestognw]

We agreed to keep it as it is because it's not a highly meaningful change. We might reconsider once this PR is done so we can correctly measure the gas savings.

Keeping this convo open and closing the other one.

[ernestognw]

We still need to update the Changeset at .changeset/bright-tomatoes-sing.md with this new _update mechanism.

[frangio]

@ernestognw There is an alternative to this PR in #4419.

I would like to avoid the use of function pointers introduced in this PR.

[frangio]

Closing in favor of #4419.