Move to != owner check from ERC721.approval to ERC721._approval

[Amxx]

In ERC721.approve (public) we do

require(to != owner, "ERC721: approval to current owner");

we don't do it in ERC721._approve (internal)

That means its not ok to approve the owner, unless you do it internally ? IMO the check should move from the public to the internal function. (note that there is already a call to ERC721.ownerOf inside the internal version, so its not like we are saving an sload anyway)

[boxingcoder00]

This may be a foolish question(very new to the space) but what exactly would be the benefit? gas or optimization ?

[ernestognw]

This may be a foolish question(very new to the space) but what exactly would be the benefit? gas or optimization ?

It's just a matter of interface consistency, we expect developers to have reasonable guarantees when they use the library. I think it's reasonable to expect this same check in both the public and internal interface.

[rupak21]

hey i want to work on this issue`

function _approval(address owner, address to, uint256 tokenId) internal virtual {
require(to != owner, "ERC721: approval to current owner");
_tokenApprovals[tokenId] = to;
emit Approval(owner, to, tokenId);
}
`

[Amxx]

hey i want to work on this issue`

function _approval(address owner, address to, uint256 tokenId) internal virtual { require(to != owner, "ERC721: approval to current owner"); _tokenApprovals[tokenId] = to; emit Approval(owner, to, tokenId); } `

There are already 2 PRs for this issue.

Also, the code you shared above changes the internal interface, which is not something we want.

[Saksham010]

hey i want to work on this issuefunction _approval(address owner, address to, uint256 tokenId) internal virtual { require(to != owner, "ERC721: approval to current owner"); _tokenApprovals[tokenId] = to; emit Approval(owner, to, tokenId); }

There are already 2 PRs for this issue.

Also, the code you shared above changes the internal interface, which is not something we want.

Made a PR, it passes all required test

[kr-meet]

@Amxx I want to work on this issue, will you give me an overview

[Amxx]

@kr-meet There is a big refactor of ERC721 that is going on internally right now. IMO we should wait for that refactor to be done before changing other things, otherwize we'll multiply conflicts

[kr-meet]

@kr-meet There is a big refactor of ERC721 that is going on internally right now. IMO we should wait for that refactor to be done before changing other things, otherwize we'll multiply conflicts

no worries, we can wait.