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

Adding a Royalties Function #20

Closed
GaiaEich opened this issue Feb 22, 2022 · 5 comments
Closed

Adding a Royalties Function #20

GaiaEich opened this issue Feb 22, 2022 · 5 comments

Comments

@GaiaEich
Copy link

We just compared the nft-erc721-collection contract with the Crypto Coven contract [https://etherscan.io/address/0x5180db8f5c931aae63c74266b211f580155ecac8#code]. We noticed a few differences. They import more stuff, and especially the IERC2981 royalty standard is missing in Hashlips' one. Check also the function:

function royaltyInfo(uint256 tokenId, uint256 salePrice)
        external
        view
        override
        returns (address receiver, uint256 royaltyAmount)
    {
        require(_exists(tokenId), "Nonexistent token");

        return (address(this), SafeMath.div(SafeMath.mul(salePrice, 5), 100));
    }

Is there a way to add this in the nft-erc721-collection contract?

@liarco
Copy link
Member

liarco commented Feb 22, 2022

Hi @GaiaEich,
first of all: any customization is totally fine, users are allowed (and encouraged) to test new features and implementations. Nothing is gonna break if you add functions to the contract, you may just have to update the constructor parameters or writing your own tests for the new features, but it depends on what you change.

My own opinion on this EIP2981 is that, while I like it more compared to the OpenSea's proprietary solution discussed in #12, I still feel it a bit weak since we still have no warranty that any marketplaces is gonna use it.
It's still in the hands of the marketplace to decide where to take this "royalty info" from, so I'm afraid that putting this inside the contract can be misleading.

This is just to say thank you very much for sharing this stuff 🙏, I'm gonna keep an eye on it and I might also make a video about how to implement that in our contract, but I'm not sure we are gonna add this feature to our contract by default (at least for now).

@granthughes1999
Copy link

granthughes1999 commented Mar 10, 2022

We just compared the nft-erc721-collection contract with the Crypto Coven contract [https://etherscan.io/address/0x5180db8f5c931aae63c74266b211f580155ecac8#code]. We noticed a few differences. They import more stuff, and especially the IERC2981 royalty standard is missing in Hashlips' one. Check also the function:

function royaltyInfo(uint256 tokenId, uint256 salePrice)
        external
        view
        override
        returns (address receiver, uint256 royaltyAmount)
    {
        require(_exists(tokenId), "Nonexistent token");

        return (address(this), SafeMath.div(SafeMath.mul(salePrice, 5), 100));
    }

Is there a way to add this in the nft-erc721-collection contract?

Hey @GaiaEich,

I was wondering if you were able to get this function working? If so do you mind sharing some more details. Where exactly did you add this function and what steps do you need to take in order to set a royalty price?

Thanks so much,

@GaiaEich
Copy link
Author

Hey @granthughes1999,

Unfortunately not yet. However, I'll let you know when we find a way!

@it-is-zods
Copy link

it-is-zods commented May 27, 2022

Hi @liarco, I understand your point of view but given this standard seems to be picking up attention (albeit from smaller marketplaces only) I think it is worthwhile starting to look into it to make it future proof.
I've created a pull request for the community to comment on - main...it-is-zods:patch-1

I even left a comment in the code so people understand that royalties are not enforceable ;)

@GaiaEich and @granthughes1999 how did you go with this?
Keen to get your thoughts on the proposed changes :)

Lastly, I didn't bother with OpenSea and Rarible "hacks" as it is much easier to go to those marketplaces and edit the collection to set those up.

@GaiaEich
Copy link
Author

GaiaEich commented Jun 9, 2022

Hi @it-is-zods, sorry for the late reply. We got a developer to add a Royalty function for us.

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

No branches or pull requests

4 participants