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

make ERC2981:royaltyInfo public #3305

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 31, 2022

Fixes #3304

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

Copy link
Contributor

@JulissaDantes JulissaDantes left a comment

Choose a reason for hiding this comment

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

Looks good!

@mcIovin
Copy link
Contributor

mcIovin commented Mar 31, 2022

@Amxx and @JulissaDantes - one thought around this (I was the person who submitted this issue...)
If this change gets implemented, the royaltyInfo() function will always be compiled as 'public' (even contracts that inherit ERC2981.sol and override royaltyInfo() will need to declare it as public, rather than 'external.'
This is really useful in order to allow super., however, I don't know if this means the EIP will no longer be strictly adhered to. Unfortunately the EIP (for 2981) has explicit code in it, which uses 'external' rather than 'public'.
Does the fact that the EIP itself uses the word 'external' rather than 'public' matter? I know that the interface Id is calculated from the functions names and arguments, so the visibility of the function won't affect the actual bytes4 of the interface identifier, but how important is it to adhere to the exact coding/wording of the EIP?

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 31, 2022

ERC define interfaces which have the function marked external. Making it public doesn't change the interfaceId or the function selector.

You can see that other ERC (20, 721, ...) also define interfaces with external function that end up being implement in a public way.

@Amxx Amxx merged commit d2832ca into OpenZeppelin:master Mar 31, 2022
Amxx added a commit that referenced this pull request Mar 31, 2022
(cherry picked from commit d2832ca)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
@Amxx Amxx deleted the feature/ERC2981/public branch March 31, 2022 18:10
@mcIovin
Copy link
Contributor

mcIovin commented Mar 31, 2022

@Amxx - I was hoping that was the case; makes sense.
Thanks again!

@@ -1,5 +1,8 @@
# Changelog

## Unreleased
* `ERC2981`: make `royaltiInfo` public to allow super call in overrides. ([#3305](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3305))

Choose a reason for hiding this comment

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

Nothing major, but royaltiInfo should be corrected to royaltyInfo

Amxx added a commit that referenced this pull request Apr 27, 2022
* 4.6.0-rc.0

* Fix release script to only release @OpenZeppelin/contracts

(cherry picked from commit 2bd75a4)

* make ERC2981:royaltyInfo public (#3305)

(cherry picked from commit d2832ca)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

* add transpilation guards to the crosschain mocks (#3306)

(cherry picked from commit 9af5af8)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

* Fix tests on upgradeable contracts after transpilation

(cherry picked from commit 0762479)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

* Remove unused constructor argument

(cherry picked from commit 69c3781)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

* Bump minimum Solidity version for Initializable.sol to 0.8.2 (#3328)

(cherry picked from commit cb14ea3)

* Fix update-comment script to ignore invalid tags

(cherry picked from commit 848fef5)
Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

* 4.6.0

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
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.

royaltyInfo() should/could be public
4 participants