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

Support ERC173 #2874

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support ERC173 #2874

wants to merge 4 commits into from

Conversation

axic
Copy link
Contributor

@axic axic commented Sep 21, 2021

Perhaps it is a bit premature to open this, but it felt easier to discuss it this way and not in an issue. The last discussion took place on #987. Opened a question I have here: ethereum/EIPs#173 (comment)

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@axic axic force-pushed the erc173 branch 3 times, most recently from e593cce to aa1b4fd Compare September 21, 2021 16:01
@@ -16,11 +17,9 @@ import "../utils/Context.sol";
* `onlyOwner`, which can be applied to your functions to restrict their use to
* the owner.
*/
abstract contract Ownable is Context {
abstract contract Ownable is Context, IERC173 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the whole point of this would be to implement supportsInterface here, but not sure how welcome is it to add it (similar to how it is done in token/ERC721/ERC721.sol for example).

@Amxx
Copy link
Collaborator

Amxx commented Sep 22, 2021

Hello @axic

This is something I considered during the recent introduction of the interfaces folder. This was ruled out because ERC173 is not final, hence its message asking about the status.

When the ERC moves to final we will do this change (but with the IERC173.sol file in the interfaces folder)

@frangio
Copy link
Contributor

frangio commented Nov 18, 2021

I'm skeptical about the usefulness of ERC165 in this context. Will reconsider the PR if ERC173 moves forward.

@frangio frangio closed this Nov 18, 2021
@niccolopetti
Copy link
Contributor

On July 9 2022 ERC173 has finally moved to final status (see commit ethereum/EIPs@110aff0 )

I think it's time to reopen discussion about this, and imho make oz adhere to the new standard

@Amxx
Copy link
Collaborator

Amxx commented Jul 13, 2022

There are two things:

  • ERC173 doesn't include the renounceOwnership function, and instead accepts transferring to 0. It be a breaking change on our end to comply with that.
  • Adding ERC165 would cause a lot of inheritance conflicts around the supportInterface function. This could be painful to devs.

Therefore, I would suggest that if we include that in our codebase, it is seen as a breaking change and is scheduled for v5.0.0

@Amxx Amxx reopened this Jul 13, 2022
@Amxx Amxx added this to the 5.0 milestone Jul 13, 2022
@dastageer-eth
Copy link

This needs to get integrated in open-zeppelin as now, opensea will only support royalty for contracts that has implemented EIP-173

@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Dec 28, 2022
@Amxx
Copy link
Collaborator

Amxx commented Jan 2, 2023

should we keep renounceOwnership for backward compatibility (in addition to transfer to 0 being authorized) ?

@ernestognw ernestognw mentioned this pull request Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants