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

royaltyInfo() should/could be public #3304

Closed
mcIovin opened this issue Mar 30, 2022 · 2 comments · Fixed by #3305
Closed

royaltyInfo() should/could be public #3304

mcIovin opened this issue Mar 30, 2022 · 2 comments · Fixed by #3305

Comments

@mcIovin
Copy link
Contributor

mcIovin commented Mar 30, 2022

I want to inherit ERC2981.sol, then I want to override the royaltyInfo() function in order to add a 'require' statement before calling the the inherited ERC2981 royaltyInfo() function (using the super. syntax.)
This creates the following compiler error

TypeError: Member "royaltyInfo" not found or not visible after argument-dependent lookup in type(contract super ...

and I believe this is caused because in ERC2981.sol, royaltyInfo() is declared as external rather than public.

ethereum testing environments

openzeppeling 4.5
Remix IDE
solidity 0.8.4

I would like to add a bit of functionality to the openzeppelin implementation of ERC2981. Specifically, I want to check that a token exists before asking for its royalty information, but the way the code is currently written I don't this can be done.
I think this can be solved by making the function in ERC2981.sol public instead of virtual.
Perhaps there was deliberate reasoning for making this function external, but if not, then it would be useful to have the ability to override royaltyInfo()

// Trying to compile the code below triggers the error.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

import "@openzeppelin/contracts@4.5.0/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts@4.5.0/access/Ownable.sol";
import "@openzeppelin/contracts@4.5.0/utils/Counters.sol";
import "@openzeppelin/contracts@4.5.0/token/common/ERC2981.sol";

contract MyToken is ERC721, ERC2981, Ownable {
using Counters for Counters.Counter;

Counters.Counter private _tokenIdCounter;

constructor() ERC721("MyToken", "MTK") {}

function _baseURI() internal pure override returns (string memory) {
    return "http://some.url/";
}

function safeMint(address to) public onlyOwner {
    uint256 tokenId = _tokenIdCounter.current();
    _tokenIdCounter.increment();
    _safeMint(to, tokenId);
}

function royaltyInfo(uint256 tokenId, uint256 salePrice)
    external
    view
    override
    returns (address, uint256)
{
    // require(_exists(tokenId), "Requested royalty for non-existing token.");    //  <-- This line is why I want to override the function.
    return super.royaltyInfo(tokenId, salePrice);
}

// The following functions are overrides required by Solidity.

function supportsInterface(bytes4 interfaceId)
    public
    view
    override(ERC721, ERC2981)
    returns (bool)
{
    return super.supportsInterface(interfaceId);
}

}

@Amxx
Copy link
Collaborator

Amxx commented Mar 31, 2022

Hello @ati00

You are right, royaltyInfo should be public.

@mcIovin
Copy link
Contributor Author

mcIovin commented Mar 31, 2022

Hi @Amxx,
Thank you so much for taking the time to read my submission and for acting on it so fast!

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 a pull request may close this issue.

2 participants