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

Force royalties on ERC721 #55

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions cookiecutter.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@
"max_supply_amount": 10000,
"permitable": "y",
"royalties": "y",
"force_royalties": "y",
"royalty_percentage_in_decimals": 10.0
}
67 changes: 67 additions & 0 deletions {{cookiecutter.project_name}}/contracts/NFT.vy
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ event ApprovalForAll:
operator: indexed(address)
approved: bool

{%- if cookiecutter.force_royalties == 'y' %}
# @dev This emits when the owner of the contract withdrawals royalties
# @param amount withdrawal by owner of smart contract
event RoyaltiesWithdrawn:
amount: indexed(uint256)

{%- endif %}

owner: public(address)
isMinter: public(HashMap[address, bool])

Expand All @@ -108,6 +116,14 @@ isApprovedForAll: public(HashMap[address, HashMap[address, bool]])

# @dev Mapping from NFT ID to approved address.
idToApprovals: public(HashMap[uint256, address])
{%- if cookiecutter.force_royalties == 'y' %}
# @dev the last balance of the smart contract that stores the royalties of the contract creator
# this balance is reset to 0 the moment the creator withdraws royalties
lastBalance: uint256

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't actually need this!

Suggested change
# @dev the last balance of the smart contract that stores the royalties of the contract creator
# this balance is reset to 0 the moment the creator withdraws royalties
lastBalance: uint256

# @dev we check this value to make sure royalties have been paid
royaltyAmount: uint256
Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest minRoyaltyAmount

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, add a note here to explore means of setting minRoyaltyAmount based on floor price oracle

Copy link
Contributor

Choose a reason for hiding this comment

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

Also also, need to set this in __init__ to a nonzero value

{%- endif %}

{%- if cookiecutter.permitable == 'y' %}
############ ERC-4494 ############
Expand Down Expand Up @@ -301,6 +317,49 @@ def royaltyInfo(_tokenId: uint256, _salePrice: uint256) -> (address, uint256):
return self.owner, royalty
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually needs to be update to integrate well

Suggested change
return self.owner, royalty
return self, max(self.minRoyaltyAmount, royalty)

{%- endif %}

{%- if cookiecutter.force_royalties == 'y' %}
# Helper functions in case market place does not support royalties to execute by this contract with _deductRoyalties()
@internal
@view
def _royaltyInfo(_tokenId: uint256, _salePrice: uint256) -> (address, uint256):
"""
/// @notice Called with the sale price to determine how much royalty
// is owed and to whom. Important; Not all marketplaces respect this, e.g. OpenSea
/// @param _tokenId - the NFT asset queried for royalty information
/// @param _salePrice - the sale price of the NFT asset specified by _tokenId
/// @return receiver - address of who should be sent the royalty payment
/// @return owner address and royaltyAmount - the royalty payment amount for _salePrice
"""

royalty: uint256 = convert(convert(_salePrice, decimal) * ROYALTY_TO_APPLY_TO_PRICE, uint256) # Percentage that accepts decimals
return self.owner, royalty
Copy link
Contributor

Choose a reason for hiding this comment

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

and then here do:

Suggested change
return self.owner, royalty
# NOTE: We are returning `self` as the receiver of the royalty to enforce payment to `self.owner` later
return self, max(self.minRoyaltyAmount, royalty)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fubuloubu this max is a genius solution for a future implementation of a minimum sell price looking at minRoyaltyAmount. Hats off. 🙏


@internal
def royaltyChecker(tokenId: uint256):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def royaltyChecker(tokenId: uint256):
def _enforceRoyalties(caller: address, tokenId: uint256):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fubuloubu why include the caller if you don't use it on the function?

Copy link
Contributor

Choose a reason for hiding this comment

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

In a future scenario you might want to use it to detect which marketplace is triggering the call to update the minimum royalty amount

For now you can probably skip the check if msg.sender is not a contract

# check if royalties hace been paid
if self.balance < self.lastBalance:
self._deductRoyalties(tokenId)
# equal the contract balance to the lastBalance for future checks
self.lastBalance = self.balance
Copy link
Contributor

@fubuloubu fubuloubu Apr 14, 2023

Choose a reason for hiding this comment

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

Suggested change
if self.balance < self.lastBalance:
self._deductRoyalties(tokenId)
# equal the contract balance to the lastBalance for future checks
self.lastBalance = self.balance
assert self.balance >= self.minRoyaltyAmount
# Send all balance to the owner (clears `self.balance`)
send(self.owner, self.balance)

Copy link
Contributor Author

@victor-ego victor-ego Apr 15, 2023

Choose a reason for hiding this comment

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

@fubuloubu for assert self.balance >= self.minRoyaltyAmountto make sense, we need to set it back to 0 with the send(self.owner, self.balance)→ OK. Would you allow the creator to modify the minRoyaltyAmount? I think that could be a nice feature is case is set too hight/low at genesis

Copy link
Contributor Author

@victor-ego victor-ego Apr 15, 2023

Choose a reason for hiding this comment

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

Also withdrawRoyalties() might be very beneficial to keep from a fiscal perspective so that the artist can control when he gets paid, therefore pay his/her taxes... checking this with a lawyer expert on the token matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fubuloubu for assert self.balance >= self.minRoyaltyAmountto make sense, we need to set it back to 0 with the send(self.owner, self.balance)→ OK. Would you allow the creator to modify the minRoyaltyAmount? I think that could be a nice feature is case is set too hight/low at genesis

That's the trick! It'll automatically be 0 because the balance will be physically transferred via send

Copy link
Contributor

Choose a reason for hiding this comment

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

Also withdrawRoyalties() might be very beneficial to keep from a fiscal perspective so that the artist can control when he gets paid, therefore pay his/her taxes... checking this with a lawyer expert on the token matter.

From a tax perspective, income is income the moment it's earned (made available to you under your full control), so I don't think there's any real tax optimization tricks here


@external
@payable
def withdrawRoyalties():
assert msg.sender == self.owner
amount: uint256 = self.balance
send(self.owner, amount)
self.lastBalance = 0
log RoyaltiesWithdrawn(amount)

@internal
@payable
def _deductRoyalties(tokenId: uint256):
# we calculate royalties and owners address
self.owner, self.royaltyAmount = self._royaltyInfo(tokenId, msg.value)
# make transaction to the contract
send(self, self.royaltyAmount)
victor-ego marked this conversation as resolved.
Show resolved Hide resolved
{%- endif %}

@view
@internal
def _isApprovedOrOwner(spender: address, tokenId: uint256) -> bool:
Expand Down Expand Up @@ -378,6 +437,10 @@ def transferFrom(owner: address, receiver: address, tokenId: uint256):
@param receiver The new owner.
@param tokenId The NFT to transfer.
"""
{%- if cookiecutter.force_royalties == 'y' %}
# check if royalties have been paid
self.royaltyChecker(tokenId)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify this a bunch!

First, gonna change this function

Suggested change
self.royaltyChecker(tokenId)
self._enforceRoyalties(msg.sender, tokenId)

{%- endif %}
self._transferFrom(owner, receiver, tokenId, msg.sender)


Expand All @@ -403,6 +466,10 @@ def safeTransferFrom(
@param tokenId The NFT to transfer.
@param data Additional data with no specified format, sent in call to `receiver`.
"""
{%- if cookiecutter.force_royalties == 'y' %}
# check if royalties have been paid
self.royaltyChecker(tokenId)
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Suggested change
self.royaltyChecker(tokenId)
self._enforceRoyalties(msg.sender, tokenId)

{%- endif %}
self._transferFrom(owner, receiver, tokenId, msg.sender)
if receiver.is_contract: # check if `receiver` is a contract address
returnValue: bytes4 = ERC721Receiver(receiver).onERC721Received(msg.sender, owner, tokenId, data)
Expand Down