Skip to content

Feat/eip-2981 royalties #112

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.

Sign up for GitHub

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

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

Feat/eip-2981 royalties #112

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6c4d5ff
Add sol files for royalties implementation
carlomigueldy Mar 2, 2022
5b7f17c
Implement contract wide royalties
carlomigueldy Mar 2, 2022
efd5948
Set royalties to 1000 -> 10%
carlomigueldy Mar 18, 2022
ca885e5
Update IERC2981Royalties.sol
carlomigueldy Mar 18, 2022
e55183f
Update PixelAvatars.sol
carlomigueldy Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions contract/contracts/PixelAvatars.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/StringsUpgradeable.sol";
import "./@eip2981/ERC2981ContractWideRoyalties.sol";

/// @author Developer DAO
Copy link
Contributor

@01231 01231 Mar 15, 2022

Choose a reason for hiding this comment

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

Would it make sense to add /// @author Developer DAO to all files you edited?

Copy link
Contributor Author

@carlomigueldy carlomigueldy Mar 18, 2022

Choose a reason for hiding this comment

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

@noahliechti

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?

/// @title The Pixel Avatar smart contract that is compliant to ERC721 standard and is upgradeable
contract PixelAvatars is
ERC721EnumerableUpgradeable,
ReentrancyGuardUpgradeable,
OwnableUpgradeable
OwnableUpgradeable,
ERC2981ContractWideRoyalties
{
string public baseURI;
uint256 public mintPrice;
Expand All @@ -29,6 +31,8 @@ contract PixelAvatars is
__ERC721_init("Pixel Avatars", "PXLDEV");
__Ownable_init();

setRoyalties(10000); // On initialize set 10% royalty fee
Copy link

@itstargetconfirmed itstargetconfirmed Mar 16, 2022

Choose a reason for hiding this comment

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

@carlomigueldy this needs to be set to 1000 for a 10% royalty fee.

Copy link
Contributor Author

@carlomigueldy carlomigueldy Mar 18, 2022

Choose a reason for hiding this comment

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

Hey, thanks again for the spot :) @itstargetconfirmed


baseURI = "ipfs://QmZdT5R5XGQVwGnnpiS6dGjUHZh6z8JjpuHhsqcLMMeWiC/";
mintPrice = 12 ether;

Expand All @@ -37,8 +41,11 @@ contract PixelAvatars is
// mintPrice = 0.01 ether;
}

function _baseURI() internal view virtual override returns (string memory) {
return baseURI;
/**
* @param amount The amount of royalties to be set.
Copy link
Contributor

@01231 01231 Mar 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@carlomigueldy carlomigueldy Mar 18, 2022

Choose a reason for hiding this comment

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

Gotcha

Copy link
Contributor Author

@carlomigueldy carlomigueldy Mar 18, 2022

Choose a reason for hiding this comment

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

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.

*/
function setRoyalties(uint256 amount) external onlyOwner {
_setRoyalties(msg.sender, amount);
}

function setBaseURI(string memory _newBaseURI) external onlyOwner {
Expand Down Expand Up @@ -103,4 +110,8 @@ contract PixelAvatars is
function withdraw() external onlyOwner {
payable(msg.sender).transfer(address(this).balance);
}

function _baseURI() internal view virtual override returns (string memory) {
return baseURI;
}
}