Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor consecutive transfer hooks #3753

Merged
merged 4 commits into from Oct 17, 2022
Merged

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Oct 6, 2022

Resolves #3739

In #3739 we realized that the introduction of ERC721Consecutive results in a breaking change, not because the behavior of a contract changes, but because it may stop compiling due to new required disambiguating overrides. An example is the combination of ERC721+ERC721Pausable, or ERC721+ERC721Enumerable, even though these combinations don't involve ERC721Consecutive the user still has to write a disambiguating override for _beforeConsecutiveTokenTransfer.

This understandably results in confusion. This is a proposal for an alternative way to design the hooks, which gives in to the breaking change but with an arguably less disruptive result. The idea is to reuse _beforeTokenTransfer but add a new argument, so that the signature becomes _beforeTokenTransfer(address from, address to, uint firstTokenId, uint lastTokenId). Thus, the same hook is reused for simple and consecutive token transfers. A user that updates the dependency to 4.8 "simply" has to add the new lastTokenId argument to get their contract to compile, and as long as they don't use ERC721Consecutive they can ignore that argument.

This PR is incomplete and won't compile, but I wanted to get the idea out since this is blocking the release of 4.8. I personally think this is an important thing to consider. The effect of having consecutive-related overrides in a contract that doesn't use ERC721Consecutive is not nice at all, and I think it just shows that the design needs to be improved.

We could also consider adding uint size instead of uint lastTokenId as the new argument to the hook.

(Related to #3744)

@frangio frangio requested a review from Amxx October 6, 2022 02:57
@frangio
Copy link
Contributor Author

frangio commented Oct 8, 2022

This needs better documentation.

@frangio frangio marked this pull request as ready for review October 14, 2022 16:20
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Amxx Amxx merged commit 08d5e4a into OpenZeppelin:master Oct 17, 2022
Amxx pushed a commit that referenced this pull request Oct 17, 2022
(cherry picked from commit 08d5e4a)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move _beforeConsecutiveTokenTransfer and _afterConsecutiveTokenTransfer to ERC721Consecutive.sol
2 participants