-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Remove further hardcoded function resolution #4309
Remove further hardcoded function resolution #4309
Conversation
|
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There are many restrictions to combining the ERC721Enumerable module with "custom" balanceOf. This is why this module is not compatible with ERC721Concecutive (for example).
Maybe we will want to clarify the assumption that this module makes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree with with adding a warning about the risks of using custom balanceOf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging since it already implements the suggestion @Amxx and I mentioned.
For some reason I missed these in #4299.
The one in
ERC721Enumerable._removeTokenFromOwnerEnumeration
is interesting and deserves attention. We were usingERC721.balanceOf
because we are more confident that it's the length of the enumeration than when we use the potentially overriddenbalanceOf
. I've just made it callbalanceOf
now because I don't see a way around this, other than introducing a storage inefficiency by storing the length which is redundant with the balance mapping.For the record here is how I am checking that these changes are exhaustive. (
rg
= ripgrep)The only remaining ones are
ERC2771Context._msgData()
andERC2771Context._msgSender()
in a mock, but without this we get a compiler warning.