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

Setting royalties as per EIP2981 / ERC2981 #301

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

it-is-zods
Copy link

a small tweak to implement EIP2981 to set royalties.
I understand it is up to the marketplace to honour it (e.g. OpenSea and Rarible have their own "hack" for royalties) but it seems the EIP2981 is starting to be adopted by newer marketplaces and hopefully the dinosaurs will eventually follow.
these changes are small and hopefully make this contract future proof

as per Liarco suggestion, I've created a way to set royalties at any time, but have a default open in the construct

*note: I've only used this in a couple of test contracts, so please do your own tests and submit any feedback and/or tweaks here

a small tweak to implement EIP2981 to set royalties.
I understand it is up to the marketplace to honour it (e.g. OpenSea and Rarible have their own "hack" for royalties) but it seems the EIP2981 is starting to be adopted by newer marketplaces and hopefully the dinosaurs will eventually follow. 
these changes are small and hopefully make this contract future proof

as per Liarco suggestion, I've created a way to set royalties at any time, but have a default open in the construct

*note: I've only used this in a couple of test contracts, so please do your own tests and submit any feedback  and/or tweaks here
@it-is-zods
Copy link
Author

it looks like mammoth OpenSea is looking into EIP2891 as well :)
https://twitter.com/Slokh/status/1530298054057897987?s=20&t=ccZIrE4HxhNs5KbDRxJwOQ

small typo, was missing the _
@liarco
Copy link
Member

liarco commented Jun 7, 2022

Hi @it-is-zods,
Thank you for this, as I said for #302, this won't be included in v2, but I will take this into consideration for v3. I'll keep this open as a reminder since this has a higher priority IMHO.

@W-Lefebvre
Copy link

Hi @liarco,
I see that under your comments there is a message saying "This branch has conflicts that must be resolved".

Does this mean that the function suggested by @it-is-zods conflicts with the rest of the code you created? So is it risky to integrate it now in the current state of the code?

thanks for the answer guys

@vincio71
Copy link

Hi @liarco
when do you think will be available this feature?
Thanks in advance

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.

4 participants