Add Royalty standard

[superboyiii]

No description provided.

[roman-khimov]

Standards work when they're standards, that is you say "NEP-11" and everyone has the same understanding of what it is. It's impossible to have this if the definition of "NEP-11" changes. Please, do not change NEP-11 or any other standard. We can have as many standards as needed, this method itself can be a separate standard for example. Or we can obsolete NEP-11 in favor of new NEP-X including this method. It'd be much easier to handle for everyone then.

[erikzhang]

But it doesn't break anything and is fully compatible with previous standards. In this case I don't think it's necessary to create a new standard.

[superboyiii]

But it doesn't break anything and is fully compatible with previous standards. In this case I don't think it's necessary to create a new standard.

I agree with Roman, we may add more and more standards on NFT related issues, but we should not add them all in NEP-11 since NEP-11 already has completed. We can't iterate other features on it, NEP-11 doesn't have v1.0, 2.0, 3.0... We should make it clear, different standard should apply on different features, they shouldn't be modified when it's completed. It's also better for developers to know which is doing what. EIP-2981 doesn't break ERC-721 as well.

[EdgeDLT]

Agree with the above. NEP-11 is the base class. Some NFTs might want to extend that with additional methods for loyalties, but others may have no reason to implement them. It makes sense to keep the standards separate.

The same is true for #152 and NEP-17 tokens, also! We shouldn't want to change NEP-17 to add approve/transferFrom/allowance, but adding a new NEP to standardize these methods for NEP-17 tokens that do want to use that functionality makes complete sense.

[superboyiii]

@roman-khimov @erikzhang @EdgeDLT Done.

[vincentgeneste]

Great idea to try and make this a standard feature :) we've been actively trying to get this done since so many months!

Couple questions / comments :

Based on the naming of methods / args, i guess you're trying to replicate EIP-2981, but in their case its a per NFT royalty settings, while in your proposal, there's no args, so i assume you want to make it a per contract setting? I would definitely think a per NFT is what's required for this proposal (so the tokenId should have to be passed).

Any reason not to add contractHash in ReceivedRoyalties event?

Most importantly, in most places, whether NEO or EVM, all fees are expressed in BPS (1000 -> 10%) directly. I think for convenience purposes and to help onboarding other devs, it should be the same here.

[merl111]

I wouldn't call the event ReceivedRoyalties but something like RoyaltiesTransferred, since it's emitted by the market contract, which transfers the royalties, received is misleading.

[superboyiii]

@merl111 @vincentgeneste Done.

[vincentgeneste]

@merl111 @vincentgeneste Done.

Couple more comments from me... :)

• your second royaltyInfo implementation has safe : false, should be safe : true
• typo founction
• we should incorporate multi recipients. While EIP2981 does not support it, it's actually one of the main drawbacks of using it, being limited to only one recipient. So we should allow an array of recipient/values, as well as emit one event per royalty paid
• i do not think isGlobalRoyalty is usefull at all. at the end of the day what we want is to know what the royalty is for one NFT. whether it is a global value or not is irrelevent we have to call the contract. So for simplification, i would get rid of it completely and just use your second implementation with royaltyInfo / tokenId / salePrice. It is up to the NEP11 contract to define if its global or not, the standard should not really care about it.

[superboyiii]

your second royaltyInfo implementation has safe : false, should be safe : true

In second situation, we have a dynamic royaltyInfo based on salePrice, creator could add his own logic to define how much royaltyAmount could be in different salePrice, so it should be not safe.

typo founction

Fixed.

i do not think isGlobalRoyalty is usefull at all. whether it is a global value or not is irrelevent we have to call the contract.

Yes, we have to call the contract, but not necessary to call it for each tokenId. I mean there should be two scenarios: Simple enough or flexible enough. In simple scenario: creators or artists have no requirements for different royalties in different tokenId in one collection, so I'd like to give a simple solution for marketplace to get a clear flag if the royalty is global. It can greatly reduce unnecessary invocation on nodes. If a collection(1 contract, different tokenId) have global royalty, still have to input tokenId, salePrice but return the same storage, it's too strange. These params are unnecessary.
For the second scenario, we give a more flexible solution which EIP-2981 already done. (Specific royalty for different tokenId or even dynamic for different salePrice).

[vincentgeneste]

your second royaltyInfo implementation has safe : false, should be safe : true

In second situation, we have a dynamic royaltyInfo based on salePrice, creator could add his own logic to define how much royaltyAmount could be in different salePrice, so it should be not safe.

typo founction

Fixed.

i do not think isGlobalRoyalty is usefull at all. whether it is a global value or not is irrelevent we have to call the contract.

Yes, we have to call the contract, but not necessary to call it for each tokenId. I mean there should be two scenarios: Simple enough or flexible enough. In simple scenario: creators or artists have no requirements for different royalties in different tokenId in one collection, so I'd like to give a simple solution for marketplace to get a clear flag if the royalty is global. It can greatly reduce unnecessary invocation on nodes. If a collection(1 contract, different tokenId) have global royalty, still have to input tokenId, salePrice but return the same storage, it's too strange. These params are unnecessary. For the second scenario, we give a more flexible solution which EIP-2981 already done. (Specific royalty for different tokenId or even dynamic for different salePrice).

Regarding the safe tag, i still think it should be set to true. Even if someone adds his own logic, it should not require write storage access, as we re just trying to get read only data here,i think it's more "safe" to have it set to safe: true :D (unless you meant like access to price via oracles etc maybe?)

For the isGlobalRoyalty i understand the idea behind it personally, it is to propose two scenarios, i just thought personally that this complexified the standard, for very low optimization, but maybe it is just me, i will let others comment on this!

[superboyiii]

Regarding the safe tag, i still think it should be set to true. Even if someone adds his own logic, it should not require write storage access

Yes, you're right, fixed.

For the isGlobalRoyalty i understand the idea behind it personally, it is to propose two scenarios, i just thought personally that this complexified the standard, for very low optimization, but maybe it is just me, i will let others comment on this!

Yes, maybe we can get more feedback to see if it's necessary.

[vincentgeneste]

@superboyiii what about supporting multiple recipients ? Is this something we want ? Personally I would love to have it as I see many different use cases for this. It’s not here on EIP2981 , but it’s highly likely because of limitations with multiple transactions in one transaction on EVM, which is not an issue at all on NeoVM.

[superboyiii]

@erikzhang @shargon Could you have a review?

[vncoelho]

Do we change to accept or merge this as a draft and make a different pr for number assignment?

Any is good to me

[roman-khimov]

name it as NEP-23

It's already in #156, let's take 24 for this one.

[superboyiii]

I will update the Number soon!

[AnnaShaleva]

The standard itself looks useful to me.

[superboyiii]

Updated @AnnaShaleva

[Jim8y]

I think this one is good to go @shargon

[AnnaShaleva]

LGTM, but if we're going to merge it, then I think we need to start with those NEPs that were approved earlier (#156) to avoid conflicts in README.

[superboyiii]

I think it's not necessary to add count after royaltyRecipient and royaltyAmount in Multi royaltyRecipient

[superboyiii]

@shargon @AnnaShaleva Good to merge!