Feat/eip-2981 royalties #112
Conversation
|
@carlomigueldy is attempting to deploy a commit to the Developerdao Team on Vercel. A member of the Team first needs to authorize it. |
contract/contracts/PixelAvatars.sol
Outdated
| function _baseURI() internal view virtual override returns (string memory) { | ||
| return baseURI; | ||
| /** | ||
| * @param amount The amount of royalties to be set. |
There was a problem hiding this comment.
I personally would use single line natspec comment and remove the . at the end here for consistensy.
/// @param amount The amount of royalties to be set
There was a problem hiding this comment.
It does actually save up on the final size of the smart contract, right? And saving a little bit of them gas fees on deployment.
| import "@openzeppelin/contracts-upgradeable/utils/StringsUpgradeable.sol"; | ||
| import "./@eip2981/ERC2981ContractWideRoyalties.sol"; | ||
|
|
||
| /// @author Developer DAO |
There was a problem hiding this comment.
Would it make sense to add /// @author Developer DAO to all files you edited?
There was a problem hiding this comment.
For the ERC2981 was a code copied from a repo.
But what are your thinks if adding and @author tag on a code that wasn't originally created here?
| /// @dev Interface for the ERC2981 - Token Royalty standard | ||
| interface IERC2981Royalties { | ||
| /// @notice Called with the sale price to determine how much royalty | ||
| // is owed and to whom. |
There was a problem hiding this comment.
Not sure if it works to use // for this line. I personally would just write everything on one line or use a comment with less words. I could imagine that the EVM can't interpret this line as part of the natspec comment because it is just a single line command
There was a problem hiding this comment.
Yeah good point, though sorry this wasn't my code and was just straight up copied from the repo that has the ERC2981 implementation.
But will change that.
contract/contracts/PixelAvatars.sol
Outdated
| __ERC721_init("Pixel Avatars", "PXLDEV"); | ||
| __Ownable_init(); | ||
|
|
||
| setRoyalties(10000); // On initialize set 10% royalty fee |
There was a problem hiding this comment.
@carlomigueldy this needs to be set to 1000 for a 10% royalty fee.
There was a problem hiding this comment.
Hey, thanks again for the spot :) @itstargetconfirmed
|
Thanks for the review brothers @noahliechti @itstargetconfirmed :) |
3 participants
EIP-2981 Implementation (Secondary Market Shares / Royalties)
This PR is specifically for adding support to royalties via EIP-2981 standard. This is specific to OpenSea royalties implementation.
Here are the references I've used to learn its implementation from the past few months:
I hope these resources helps out for anyone interested to learn more about the secondary market shares preparation/implementation on a smart contract.
Closes #110