Use the _update mechanism in ERC721 by Amxx · Pull Request #4377 · OpenZeppelin/openzeppelin-contracts (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation175 Commits53 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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)

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.

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.

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.

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.