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

Remove hardcoded function resolution #4299

Merged

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Jun 2, 2023

Fixes #3883

This should be removing all hardcoded function resolution.

As a consequence, in ERC721 I have switched it to checked arithmetic for _balances[from] -= 1, to avoid overflow if _ownerOf is overriden and out of sync with the mapping.

@frangio frangio requested review from Amxx and a team June 2, 2023 02:16
@changeset-bot
Copy link

changeset-bot bot commented Jun 2, 2023

🦋 Changeset detected

Latest commit: 71ff0c4

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 frangio marked this pull request as ready for review June 2, 2023 02:36
@Amxx
Copy link
Collaborator

Amxx commented Jun 2, 2023

As a consequence, in ERC721 I have switched it to checked arithmetic for _balances[from] -= 1, to avoid overflow if _ownerOf is overriden and out of sync with the mapping.

That part I'm not sure about. We considered that option in ERC721Consecutive, With an additional balance

mapping(address => uint256) private _balanceFromConsecutiveMints;

function _balanceOf(address user) internal virtual view returns (uint256) {
    unchecked {
        return super._balanceOf(user) + _balanceFromConsecutiveMints[user];
    }
}

Which overflow balancing out. If we make the balance update in _transfer checked, we kill that pattern.


Everything else looks good.

@frangio
Copy link
Contributor Author

frangio commented Jun 2, 2023

I would come back to the ERC721Consecutive issue when we work on the ERC721 hook refactor.

@frangio frangio merged commit ffceb3c into OpenZeppelin:master Jun 2, 2023
@frangio frangio deleted the lib-829-hardcoded-fn-resolution branch June 2, 2023 17:21
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.

Stop using hardcoded resolution of functions
2 participants