Force royalties on ERC721 #55

victor-ego · 2023-04-14T21:57:49Z

Force royalties on ERC721 in case the market place does not support/distribute royalties to creators.

fubuloubu

Give this a shot!

fubuloubu · 2023-04-14T22:06:56Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+lastBalance: uint256
+
+# @dev we check this value to make sure royalties have been paid
+royaltyAmount: uint256


Would suggest minRoyaltyAmount

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

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

fubuloubu · 2023-04-14T22:08:02Z

{{cookiecutter.project_name}}/contracts/NFT.vy

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


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)

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

fubuloubu · 2023-04-14T22:10:27Z

{{cookiecutter.project_name}}/contracts/NFT.vy

@@ -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)


We can simplify this a bunch!

First, gonna change this function

Suggested change

self.royaltyChecker(tokenId)

self._enforceRoyalties(msg.sender, tokenId)

fubuloubu · 2023-04-14T22:10:42Z

{{cookiecutter.project_name}}/contracts/NFT.vy

@@ -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)


And here

Suggested change

self.royaltyChecker(tokenId)

self._enforceRoyalties(msg.sender, tokenId)

fubuloubu · 2023-04-14T22:12:15Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+    return self.owner, royalty
+
+@internal
+def royaltyChecker(tokenId: uint256):


Suggested change

def royaltyChecker(tokenId: uint256):

def _enforceRoyalties(caller: address, tokenId: uint256):

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

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

fubuloubu · 2023-04-14T22:13:38Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+# @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
+


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

fubuloubu · 2023-04-14T22:15:32Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+     if self.balance < self.lastBalance:
+        self._deductRoyalties(tokenId)
+        # equal the contract balance to the lastBalance for future checks
+        self.lastBalance = self.balance


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)

@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

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.

@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

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

{{cookiecutter.project_name}}/contracts/NFT.vy

fubuloubu · 2023-04-14T23:18:35Z

also would have to update any tests where there's a transfer occuring to have value=nft.minRoyaltyAmount()

fubuloubu · 2023-04-18T22:21:48Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+# @dev this can also be used to explore a based floor price oracle for a minimum royalty payment to the creator
+# @ dev current implementation sends to the smart contract the apply % in royalties to the creator


Don't need @dev tag three times in a row

fubuloubu · 2023-04-18T22:23:31Z

Missing force_royalties in test case JSONs

victor-ego · 2023-04-18T22:53:54Z

also would have to update any tests where there's a transfer occurring to have value=nft.minRoyaltyAmount()

I made minRoyaltyAmount public so that it could be accessed by nft.minRoyaltyAmount()
Passed the tests like such: nft.transferFrom(receiver, owner, 0, value=nft.minRoyaltyAmount(), sender=receiver) -> didn't pass the test
Hardcoded the value on the tests as string "10000000000000000" or as "0.01 ether" without the quotes and the test still doesn't pass.

fubuloubu

Finally! We got it done!

Were you able to validate on OpenSea testnet?

fubuloubu · 2023-04-22T16:07:51Z

cookiecutter.json

+    "force_royalties": "y",
+    "royalty_percentage_in_decimals": 10.0,


Can you flip these two? Royalty percentage is just for regular royalties, while min royalty amount is for forced royalties

{{cookiecutter.project_name}}/contracts/NFT.vy

fubuloubu · 2023-04-22T17:31:11Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+    return self, max(self.minRoyaltyAmount, royalty)
+
+@internal
+def _enforceRoyalties(tokenId: uint256):


Add a note on how this function might be expanding to allow updating the floor price oracle

fubuloubu · 2023-04-22T17:31:55Z

{{cookiecutter.project_name}}/contracts/NFT.vy

@@ -294,13 +303,40 @@ def royaltyInfo(_tokenId: uint256, _salePrice: uint256) -> (address, uint256):
    /// @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 royaltyAmount - the royalty payment amount for _salePrice
+    /// @return minRoyaltyAmount - the minimum 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


This actually needs to be update to integrate well

Suggested change

return self.owner, royalty

return self, max(self.minRoyaltyAmount, royalty)

fubuloubu · 2023-04-22T17:32:23Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+# Helper functions in case market place does not support royalties to execute by this contract with _enforceRoyalties()
+@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 minRoyaltyAmount - the minimum royalty payment amount for _salePrice
+    """
+
+    royalty: uint256 = convert(convert(_salePrice, decimal) * ROYALTY_TO_APPLY_TO_PRICE, uint256) # Percentage that accepts decimals
+    # NOTE: We are returning `self` as the receiver of the royalty to enforce payment to `self.owner` later
+    return self, max(self.minRoyaltyAmount, royalty)
+


Then this function actually isn't necessary

Suggested change

# Helper functions in case market place does not support royalties to execute by this contract with _enforceRoyalties()

@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 minRoyaltyAmount - the minimum royalty payment amount for _salePrice

"""

royalty: uint256 = convert(convert(_salePrice, decimal) * ROYALTY_TO_APPLY_TO_PRICE, uint256) # Percentage that accepts decimals

# NOTE: We are returning `self` as the receiver of the royalty to enforce payment to `self.owner` later

return self, max(self.minRoyaltyAmount, royalty)

fubuloubu · 2023-04-22T17:32:43Z

{{cookiecutter.project_name}}/contracts/NFT.vy

 @external
+@payable


I usually like to switch these

fubuloubu · 2023-05-09T01:36:13Z

.build/__local__.json

@@ -0,0 +1 @@
+{"contractTypes":{},"manifest":"ethpm/3","sources":{}}


Delete this file, and add .build/ to gitignore

fubuloubu · 2023-05-09T02:10:49Z

tests/Mintable.json

+    "force_royalties": "y",
+    "minRoyaltyAmount": 1000000000000000


Can this be n and 0?

can be 0, cannot be n [string] but any int. your thinking creating a function that allows the creator to modify this at any stage.

No I meant force royalties is n for this case

fubuloubu · 2023-05-10T01:03:49Z

Can you add .hypothesis/ to gitignore too?

fubuloubu · 2023-06-07T14:37:43Z

{{cookiecutter.project_name}}/.gitignore

 .coverage
 .coverage.*
 .cache
+.DS_Store


Usually files like this are best in your global gitignore
https://docs.github.com/en/get-started/getting-started-with-git/ignoring-files

fubuloubu · 2023-06-07T14:40:06Z

{{cookiecutter.project_name}}/contracts/NFT.vy

+{%- if cookiecutter.force_royalties == 'y' %}
+@external
+@payable
+def __default__():


Typically, when smart contracts built by compilers like solidity or vyper send ether to a contract, it only gives a limited amount of gas to that contract to execute. So, basically I think this won't work since there's too much logic going on inside it

.DS_Store

victor-ego and others added 4 commits April 8, 2023 23:50

made bytes4 nonpayable fixing img issue display

2d5f101

back to original commit changed only nonpayable

d1ed23e

Merge branch 'ApeAcademy:main' into main

348a837

force royalties implementation

37f15f2

fubuloubu reviewed Apr 14, 2023

View reviewed changes

victor-ego added 3 commits April 16, 2023 10:10

clean improvements to force royalties

dffe344

updated tests

2a6f88b

made minRoyaltyAmount public

704470d

fubuloubu approved these changes Apr 18, 2023

View reviewed changes

test fixes and comments on code

759835b

included tests and @payable on transfer functions

cbd6a99

fubuloubu approved these changes Apr 22, 2023

View reviewed changes

victor-ego added 3 commits May 9, 2023 00:33

forcing royalty payments

faf00b3

fulling working force royalties

a9f9856

fixing cookiecutter test case

38771fe

fubuloubu reviewed May 9, 2023

View reviewed changes

victor-ego added 2 commits May 10, 2023 00:23

included some tests

c3c44d9

changes on test and local rm

e2ec909

victor-ego and others added 3 commits May 10, 2023 10:35

cleared files and inclluded on .gitignore

a42bbc9

fixed visibility for _enforceRoyalties

d8d07a1

Apply suggestions from code review

3cb9c2b

fubuloubu approved these changes May 10, 2023

View reviewed changes

fubuloubu enabled auto-merge (squash) May 10, 2023 22:57

victor-ego mentioned this pull request Jun 3, 2023

💥 ERC-2981 (NFT Royalty Standard) Implementation pcaversaccio/snekmate#120

Closed

victor-ego added 2 commits June 7, 2023 13:22

included basic oracle for floor price fluctuations

30aedb9

Merge branch 'main' with internal oracle price

5627f70

auto-merge was automatically disabled June 7, 2023 11:31
Head branch was pushed to by a user without write access

tests

7720741

fubuloubu reviewed Jun 7, 2023

View reviewed changes

removed fallback function due to low gas execution

8f89230

fubuloubu reviewed Jun 7, 2023

View reviewed changes

.DS_Store Outdated Show resolved Hide resolved

remove .DS_Store files

cfc3aa6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force royalties on ERC721 #55

Force royalties on ERC721 #55

victor-ego commented Apr 14, 2023

fubuloubu left a comment

fubuloubu Apr 14, 2023

fubuloubu Apr 14, 2023

fubuloubu Apr 14, 2023

fubuloubu Apr 14, 2023

victor-ego Apr 15, 2023

fubuloubu Apr 14, 2023

fubuloubu Apr 14, 2023

fubuloubu Apr 14, 2023

victor-ego Apr 15, 2023

fubuloubu Apr 16, 2023

fubuloubu Apr 14, 2023

fubuloubu Apr 14, 2023 •

edited

Loading

victor-ego Apr 15, 2023 •

edited

Loading

victor-ego Apr 15, 2023 •

edited

Loading

fubuloubu Apr 16, 2023

fubuloubu Apr 16, 2023

fubuloubu commented Apr 14, 2023

fubuloubu Apr 18, 2023

fubuloubu commented Apr 18, 2023

victor-ego commented Apr 18, 2023

fubuloubu left a comment

fubuloubu Apr 22, 2023

fubuloubu Apr 22, 2023

fubuloubu Apr 22, 2023

fubuloubu Apr 22, 2023

fubuloubu Apr 22, 2023

fubuloubu May 9, 2023

fubuloubu May 9, 2023

victor-ego May 9, 2023

fubuloubu May 9, 2023

fubuloubu commented May 10, 2023

fubuloubu Jun 7, 2023

fubuloubu Jun 7, 2023

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

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

	# @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 this can also be used to explore a based floor price oracle for a minimum royalty payment to the creator
		# @ dev current implementation sends to the smart contract the apply % in royalties to the creator

		"force_royalties": "y",
		"royalty_percentage_in_decimals": 10.0,

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

		@@ -0,0 +1 @@
		{"contractTypes":{},"manifest":"ethpm/3","sources":{}}

Force royalties on ERC721 #55

Are you sure you want to change the base?

Force royalties on ERC721 #55

Conversation

victor-ego commented Apr 14, 2023

fubuloubu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fubuloubu Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

victor-ego Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

victor-ego Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fubuloubu commented Apr 14, 2023

Choose a reason for hiding this comment

fubuloubu commented Apr 18, 2023

victor-ego commented Apr 18, 2023

fubuloubu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fubuloubu commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fubuloubu Apr 14, 2023 •

edited

Loading

victor-ego Apr 15, 2023 •

edited

Loading

victor-ego Apr 15, 2023 •

edited

Loading