create an internal _approve function allowing extensions and EIPs to do approvals without check #427
Conversation
This file contains 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
…Ps to approve without check
dievardump
changed the title
Vectorized
approved these changes
Oct 19, 2022
cygaar
approved these changes
Oct 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation of ERC721A have
_tokenApprovalsprivate and has all the approval management code in the public facingapprovefunction.This sadly means that there is no way, for a contract extending ERC721A, to internally do approvals without the ownership or isApprovedForAll check.
There are however some EIPs meant to improve ERC721 and tokens management that are in need of being able to approve internally without those checks.
An example is EIP-4494 that introduces Permits to ERC721, allowing to give approvals to accounts by only signing a message instead of having it done with a tx. Very similar to EIP-2612 for ERC20.
This EIP could totally change the way we work with marketplaces today, as we would be able to simply sign messages in order to sale items, instead of granting approvalForAll to contracts.
However, EIP-4494 requires the ability to set the approval internally, or at least to bypass the ownership/approvalForAll check, which is right now not possible with the current implementation.
Even if not to accommodate other EIPs and extensions, there can be a lot of reasons for children contracts to be able to do that, similar to the reasons for contracts to be able to
burnitems internally without that check.So, as it was already done with the
_burn()function, this pull request internalize the content of the_approvefunction with a version that has a 3rd parameter, to indicate if the ownership & isApprovalForAll needs to be checked or not before setting the approval.Gas Report
Before
After
pros
allows internal approvals without any check of ownership nor weird hack on isApprovedForAll()
cons
slightly more expensive in gas at deployments (~5000gas) and approvals (~70gas)