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

Supporting royalties that live alongside the NFT #53

Closed
bjartek opened this issue Dec 20, 2021 · 56 comments
Closed

Supporting royalties that live alongside the NFT #53

bjartek opened this issue Dec 20, 2021 · 56 comments
Assignees
Labels
enhancement New feature or request Feedback Metadata View SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board

Comments

@bjartek
Copy link
Contributor

bjartek commented Dec 20, 2021

Problem

Some projects have complex royalties structures that they want enforced in any plattform that sells or transfers that NFT. In order to support this several solutions have included Royalty data into the NFT resources themselves.

Some examples here are:

  • Versus have Artist Royalty and Minter Royalty stored in the NFT
  • Flovatar has a 1% Minter royalty stored in the NFT
  • NeoCollectibles have a Royalty to the Owner of the main Motorcycle NFT on trades of NFTs in its crews.

Work done to support this thus far

In onflow/flip-fest#16 this was discussed but it was taken out of the final document because more stakeholders wanted a say.

bjartek, dete, rhea had a meeting about this because bjartek (as a proposed) and rhea (as a representative of dapper) had a disagreement about how it should work. In this meetin the following struct was agreed upon

	/// A struct interface for Royalty agreed upon by @dete, @rheaplex, @bjartek
	pub struct interface Royalty {

		/// if nil cannot pay this type
		/// if not nill withdraw that from main vault and put it into distributeRoyalty
		pub fun calculateRoyalty(type:Type, amount:UFix64) : UFix64?

		/// call this with a vault containing the amount given in calculate royalty and it will be distributed accordingly
		pub fun distributeRoyalty(vault: @FungibleToken.Vault)

		/// generate a string that represents all the royalties this NFT has for display purposes
		pub fun displayRoyalty() : String?

	}

The main proposal here came from dete and some subleries of it was discussed and we agreed upon what is stated above.

Here is an implementaion of how that can work for Versus art.
https://github.com/findonflow/find/blob/testing/contracts/Art.cdc#L323

@bjartek bjartek added bug Something isn't working Feedback labels Dec 20, 2021
@psiemens psiemens changed the title Supporting Roylaties that lives with the NFT Supporting royalties that live alongside the NFT Dec 20, 2021
@psiemens psiemens added enhancement New feature or request and removed bug Something isn't working labels Dec 20, 2021
@bjartek
Copy link
Contributor Author

bjartek commented Jan 11, 2022

I have found an issue I do not like with the above model. And that is that it is impossible to emit a good Event when royalty is paid out since the context of the item you want to pay royalty is not sent into the distributeRoyalty method. I guess you could work around this in various wayt but I still think it is tedious. Beeing able to send in some more fields to distributeRoyalty would be preferrable so that we can have a good Royalty event that.

In fact that is one point we did not discuss in the meeting where the above struct was agreed upon. Emitting a good event that can be used across different solutions.

@satyamakgec
Copy link
Contributor

I have found an issue I do not like with the above model. And that is that it is impossible to emit a good Event when royalty is paid out since the context of the item you want to pay royalty is not sent into the distributeRoyalty method. I guess you could work around this in various wayt but I still think it is tedious. Beeing able to send in some more fields to distributeRoyalty would be preferrable so that we can have a good Royalty event that.

In fact that is one point we did not discuss in the meeting where the above struct was agreed upon. Emitting a good event that can be used across different solutions.

Hey @bjartek could you please fill me in about the event data that you are envisioning so I could architect the same stuff in my head. AFAIK the required data needed is Amt get distributed as royalty, beneficiary, description of some type to depict the reason or any data that we want to fill in, Maybe some data related to the NFT_ID as well anything else that you thought so ?

@bjartek
Copy link
Contributor Author

bjartek commented Jan 20, 2022

In my eyes a good royalty event should contain the following fields

  • name: what is the name of this royalty, is it "minter", "originalArist", "marketplace". This is purely for informational purposes
  • amount: The amount of royalty paid
  • type: the type of FT.Vault that was paid
  • receiver: who received the royalty
  • payer: who triggered the payment of this royalty, the purchaser

Fields I am not so sure about are:

  • percentage: the percentage of tx that was paid
  • totalAmount: the total amount of the tx

You point on adding information about the UUID of the object royalty was paid for is also something that I think would be very nice.

@satyamakgec
Copy link
Contributor

Awesome! Thanks for the input

  • Name -> It can be covered under the description. ✅
  • Amt ✅
  • receiver -> ✅
  • type of vault -> ✅
  • payer -> ✅
  • UUID -> ✅

I don't think percentage is necessary as that can be easily calculated by other metadata offchain and neither the totalAmt as the all event summation would give total amount so again can be re-created offchain.

I think every piece of data can be easily retrieved without passing extra data in the purchase function of the storefront/marketplace contract if it is carefully designed.

@franklywatson franklywatson added the SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board label Jan 24, 2022
@joshuahannan
Copy link
Member

@satyamakgec Can you post your proposal here? Like others have said elsewhere, the distribution should probably not happen in the royalty struct. The royalty struct should probably only just return the information about the royalties and the individual marketplaces should be responsible for distributing them.

@satyamakgec
Copy link
Contributor

satyamakgec commented Jan 28, 2022

Yeah sure @joshuahannan , Below is the representation what I think will be sufficient to facilitate the royalty information within the NFTs.

// Struct that keeps the data related to the royalty.
pub struct RoyaltyDetails {

     // Capability of the receiver.
     receiverCapability, 
      // Percentage of salePrice in basis points
     basisPoints,
     // Could be optional, It will be simple string to tells about what for the royalty is.
     description 
}

// Struct used as the return type for the `royaltyInfo` function.
pub struct RoyaltyInfo {

    // Capability of the receiver.
   receiverCapability,
   // It is the amount that get calculated on the basis of the `RoyaltyDetails.basisPoints` & `salePrice`
   saleCut,
  // Optional description
  description
  
}

// Interface that needs to implement by the NFTs to support the Royalty info, It will be a voluntary
// implementation as there will be many cases where marketplace don't want to support it.

// This interfaces doesn't care about how payment of the royalty would happen as it will be cumbersome to 
// handle within the standard and make the standard rigid. The idea here is to make the standard flexible 
// as much as possible so it can support many innovations.
pub struct interface Royalty {
  
  // Below function will calculate the royalty cut on the basis 
  pub fun royaltyInfo(nft_id: UInt64, salePrice: UFix64): [RoyaltyInfo]
}

The idea it to keep it flexible so implementer of the royaltyInfo() could return royalties in different variants, Like decaying royalties, stepped royalties or many other techniques implementer can implement under the royaltyInfo function. Whilst marketplace only needs three values capability which is going to receive the funds, amount of cut needs to transfer & and optional description.

Happy to receive feedback.

@satyamakgec satyamakgec self-assigned this Jan 28, 2022
@bjartek
Copy link
Contributor Author

bjartek commented Jan 31, 2022

In order to calculate the proper royaltyInfo in your above implementation the royaltyInfo has to include the type of Vault you would want the royalty to be paid as. If it does not it is impossible to return the correct receiverCapability in the RoyaltyInfo. (if you do not use an abstraction around receiver that support multiple wallets)

So given that you have an item that is sold for 10.0 and you have an artistCut of 2.5% and a originalMinter cut of 1% the royalty info returned here would be

[ {capToArtist, 0.25, "artist"}, {capToMinter, 0.1, "minter"}]

Is that the correct way to interepet your suggestion?

If that is the case then I think that would work pretty well. My original suggestion was that the "description" field was the key in a map that contains a pair of the other values but it is not very important to me to have it either way.

Also given that the view is implemented on an item I do not see why you would need to send in nft_id here at all. You are calling the view on a reource that has an ID so there is no reason to send it in.

@bluesign
Copy link

bluesign commented Jan 31, 2022

This is nice but lacking much tbh vs original proposal.

It is important to resource to know royalty distributed, so you can also implement royalties on first N sales, royalties with decreasing percentage by some algorithm on sales etc.

Also for example, if resource cannot distribute royalty, it can keep royalty inside of resource, and can try to distribute on next transaction etc.

@satyamakgec
Copy link
Contributor

satyamakgec commented Feb 1, 2022

In order to calculate the proper royaltyInfo in your above implementation the royaltyInfo has to include the type of Vault you would want the royalty to be paid as. If it does not it is impossible to return the correct receiverCapability in the RoyaltyInfo. (if you do not use an abstraction around receiver that support multiple wallets)

True, Either we could replace it with the abstract receiver (Don't know when we will have it) or I think we could directly use the address as well and marketplace borrow the capability, This is not a sophisticated way but I think that is the only way until unless we have the abstraction.

So given that you have an item that is sold for 10.0 and you have an artistCut of 2.5% and a originalMinter cut of 1% the royalty info returned here would be

[ {capToArtist, 0.25, "artist"}, {capToMinter, 0.1, "minter"}]

Is that the correct way to interepet your suggestion?

Yes, you interpret it right.

Also given that the view is implemented on an item I do not see why you would need to send in nft_id here at all. You are calling the view on a reource that has an ID so there is no reason to send it in.

Agree with this as I am still in the Ethereum hangover so it makes sense to remove the nft_id.

cc @bjartek

@satyamakgec
Copy link
Contributor

satyamakgec commented Feb 1, 2022

It is important to resource to know royalty distributed, so you can also implement royalties on first N sales, royalties with decreasing percentage by some algorithm on sales etc.

Also for example, if resource cannot distribute royalty, it can keep royalty inside of resource, and can try to distribute on next transaction etc.

It can be a requirement but adding distribution of the royalty within the resource would lead to complexity within the standard itself. We can apply the different algorithms on the basis of timestamp but it could be hard to do so with the no. of sales as we never know when the distribution is going to happen.

I am open to suggestions to find a way to support the No. of sales

cc @bluesign

@bluesign
Copy link

bluesign commented Feb 1, 2022

@satyamakgec I was thinking a bit in the context of other contracts like NFTStorefront for example.

If we can somehow manage a distributeRoyalty function, other contracts can use this instead of managing to split royalties themselves, then for example NFTStoreFront can get rid of cuts somehow mostly or totally.

btw when I say distributeRoyalty, it can also stay as a public function in another contract (let's say RoyaltyHelper), doesn't have to be implemented inside NFT.

in RoyaltyHelper Contract:
fun distributeRoyalty(royalties: [RoyaltyInfo] , vault: @FungibleToken.Vault)

in RoyaltyMetadata can have some default implementation like:

pub struct interface Royalty {
  // Below function will calculate the royalty cut on the basis 
  pub fun royaltyInfo(salePrice: UFix64): [RoyaltyInfo]
  fun  distributeRoyalty(salePrice: UFix64, vault: @FungibleToken.Vault) {
    RoyaltyHelper.distributeRoyalty(royalties: self.royaltyInfo(salePrice: salePrice), vault:vault)
  }
}

so any user somehow can:

  • call royaltyInfo(salePrice: X)
  • calculate totally royalty to be paid
  • withdraw royalties from purchase Vault
  • call distributeRoyalty
  • rest of the vault sent to seller

But my main point is if we suggesting RoyaltyInfo with net prices ( calculated with sale price ) , distribution part is the easy part, also at least we can guarantee that if some Market is using let's say NFTStoreFront contract ( if NFTStoreFront implements this workflow), they are respecting the royalties.

@satyamakgec
Copy link
Contributor

satyamakgec commented Feb 2, 2022

@bluesign
I don't think so that NFTStorefront can get rid of cuts totally as it is going to facilitate the payment of the listing and other operational stuff as well.

For me keeping the payment and the standard it self aside is more cleaner approach as I don't need to write the logic of payments when I am going to design the NFTs and it can lead some inconveniences during the sale of NFT as at two places contains the distribution logic as well, One is distributing the saleCut and other one specifically paying the royalties.

Another thing I have in mind that if we are going to keep the distribution royalty method in the standard then there could be malicious implementation that can affect the purchase of the NFT it self, There are a lot of unknowns if we keep the distribution method under the standard.

But I am also not denying the fact that it will be much more convenient to design the sale algorithm if we have the distribution under the standard umbrella, Maybe one thing more we can do is add a hook/callback in the NFT storefront contract to update the no. of sale in the NFT contract although it is not a cleanliest approach either but it will work and here we have to trust only one contract i.e NFT storefront.

@joshuahannan
Copy link
Member

This is a much more complicated view discussion than the Display one! So many different variables! 😆

It might make more sense to use an address instead of a capability to be generic, so we don't have to worry about specifying the token type in the royalty specification. It could just apply to all token types. Unless maybe there is a specific reason someone would want to restrict who gets paid royalties depending on what token is being used for the payment? I'm not sure.

I am a fan of Satyam's suggestion. I think is is fairly clean. I don't know how much we should be able to enforce about timestamp or first N payments in the royalty standard, but I am not familiar with other royalty standards. To me it seems like that would still work under the hood for the NFTs and can just be in a manual implementation.

And I am fine with keeping the distribution separate from the standard. I can see it being in a separate contract or up to the marketplace developer, but not in the standard. Like others have suggested, some sort of callback in the standard to mark when a sale has happened could be useful, but not actually distributing the royalties.

@psiemens What do you think about this discussion? Is there anything from your suggestions that you'd like to add?

@bjartek
Copy link
Contributor Author

bjartek commented Feb 2, 2022

I am not in favor of using an address. It forces the implementation of the wallet to be the one that is behind a specific path and it does not even ensure that the user has that path linked. There are still accounts that have not FUSD linked/stored.

Lets say somebody wants to use a pooling/split wallet behind the royal for a given item. That should be possible and a capability ensures that is possible.

.find has a profile that is a FT.Receiver and can distribute any kinds of funds. You just register a new wallet in the profile and it will distribute to the first wallet in its configuration that matches the vault you send in.

@satyamakgec
Copy link
Contributor

@bjartek What you are suggesting is something like a resolver where it resolve to the vault that can support the given funds. Let me know if I understand rights & could you please give me the code reference of it ?

@bjartek
Copy link
Contributor Author

bjartek commented Feb 2, 2022

Correct @satyamakgec.

This is the deposit method https://github.com/findonflow/find/blob/main/contracts/Profile.cdc#L386

You can also call this in find to get a better event then the current once with this tx:
https://github.com/findonflow/find/blob/main/transactions/sendFlowWithTagAndMessage.cdc#L80

@satyamakgec
Copy link
Contributor

Thanks @bjartek this is helpful, We could support something like that but I am not sure whether we need to create a generic contract to support that feature so it could use industry wide as well or whether we can directly use the solution you suggested.

I mean we need to standardise that solution as well without doing it we may lead to discrepancies.

@bluesign
Copy link

bluesign commented Feb 2, 2022

@bjartek's solution is great, I think we need a generic receiver interface for sure.

Somehow I am torn on the panic if Vault type not exists vs storing the received Vault ( and later automatically having this Vault type), though as we have storage fees currently latter option is not possible. ( DoS risk )

When I was thinking that recently I proposed to get rid of Storage Fees ( onflow/flow#791 )

I think in any case we need generic receiver for FT that doesn't panic on unknown vault type for royalties.

@bjartek
Copy link
Contributor Author

bjartek commented Feb 2, 2022

Good point @bluesign. I would have no problem switching to that on for .find if it became available. It is not even that hard to implement TBH.

@vleushin
Copy link

vleushin commented Feb 7, 2022

Heya folks. Wondering if you considered to use some sort of proxy contract for storing various FT vaults. Like If I have address X then I get go to that proxy contract and claim everything that is under my address X.

If I want to transfer some funds (e.g. USDC) to address X and that has not initialized needed FT vault, I can transfer it to that proxy contract that will create needed FT vault for address X and hold it until owner of address X claims it.

We can limit the set of supported FT, like FUSD, USDC. We can also remove keys from contract account to make it independent.

@bjartek
Copy link
Contributor Author

bjartek commented Feb 7, 2022

@vleushin that is basically what the .find profile wallet abstraction does. Right now it only works with Flow/FUSD out of the box but adding new ones is easy. However I like @bluesign approach above that extra FT you do not have a storage slot for should just be escrowed.

@psiemens
Copy link
Contributor

psiemens commented Feb 8, 2022

Wondering if you considered to use some sort of proxy contract for storing various FT vaults.

Rhea wrote up a "token switchboard" proxy contract that does this (forked to my GH): https://github.com/psiemens/flow-ft-switchboard/blob/main/cadence/contracts/FungibleTokenSwitchboard.cdc

@bjartek It's very similar to what you have in Profile

We were also thinking that the royalty implementation should just deposit to the proxy.

@bjartek
Copy link
Contributor Author

bjartek commented Feb 8, 2022

This implementation works in effect exactly like we have in the find profile.

It still lacks @bluesign nice concept of escrowing ft that you do not have a receiver for. So in deposit if you do not find a receiver then escrow funds and emit a good event.

I would not want my tx to fail because one royalty holder does not have a new ft type linked.

@justjoolz
Copy link

Splitting Royalty definition from the settlement seems appropriate for the MetadataView

Also perhaps there should be a clear distinction between NFT Royalty and platform Royalty.....

ie. marketplace is responsible for emitting event for their cut and the NFT settlement layer method emits only the baked in royalties stored in the NFT

@satyamakgec
Copy link
Contributor

FYI - There are some improvements we are planning to do in the storefront contract (Not set in stone yet). We can another one to emit an event when the royalty get paid, But it is hard to distinguish right now from other sales cut so we have a plan to support that change in the storefront contract. So if the Storefront contract get used as the marketplace then the end user would get an event of royalty paid.

Something like this RoyatlyPaid(royalty: UFix64, receiver: Address / Proxy contract (separate discussion), description: String, nft_id: UInt64) --- It is a rough structure it will get improved when the actual change get applied.

For what @psiemens mentioned

  1. Multiple royalties and recipients per NFT. --> Would supported by the Royalty View
  2. Compatibility with the NFT Storefront contract. ---> This is planned change in the storefront contract once Royalty view get finalized.
  3. NFT resource can detect when a royalty is paid. ---> It is difficult to do as it would need some changes in the NFT contract it self where storefront contract do some callback to the NFT resource to tell the royalty get paid.
  4. An event is emitted when the royalty is paid. ----> Planned change in the NFT Storefront contract.

I agree with @bjartek & @bluesign here that supporting proxy contract with the escrow functionality make sense instead of direct using the address but I think it is out of scope for this discussion, We can have another conversation about that.

Here is the reference of the Royalty view https://github.com/onflow/flow-nft/blob/wip-royalties/contracts/RoyaltyViews.cdc

Happy to receive the feedback.

@hier01
Copy link

hier01 commented Feb 16, 2022

This may be repeated, but if a marketplace supports royalty payments, most likely it's a single payment to a single address. I don't think they can support distribution to multiple parties--that would need logic. Splits will be handled by the NFT or receiving agency. The marketplaces need a single function: they call with an NFT ID and a sale price; the NFT returns the total amount of the royalty due and the payment address. Maybe there's an identifier to let the NFT determine an adjusted rate for a given marketplace, e.g. for negotiated rates or caps, etc.

This much would be more or less compatible with the EIP-2981. I think this is what OpenSea would be looking for, and I think Rarible, too--they take 2.5% and then "the royalty rate gets delivered to the designated address upon successful sale – all within one single transaction."

Requests for how the NFT splits the royalties would be a separate thing. This would return the array of cuts. The royalties defined in an NFT would serve as the default values. These would determine the total inbound payment due from markets, and also the outbound payments to parties. "Adjustments" could be applied to the defaults to compute differential rates based on number of sales, negotiated rates, etc. NFT should be able to report what adjustment structure was applied on a sale, if any.

The NFT also has to report whether it supports a given standard (boolean). Therefore it might be better to limit the base standard to the single function needed for the marketplaces, or a small set. Other metadata could go in a second standard.

One of my general questions is about currency. The total royalty will be paid in the currency of the sale. On Rarible today, you can only pay for TopShots in FLOW, so I guess that will be the near term solution. The NFT itself can accept FLOW, but how do we handle royalties paid in ETH, etc.?

@bjartek
Copy link
Contributor Author

bjartek commented Feb 16, 2022

@satyamakgec I have expressed my view on this further up in this thread several times but I have to ask, why are you using an Address here and not a Capability to a FT.Receiver. Many solutions already use this pattern today.

If you only use an Address you have no way of knowing what vault they have chosen to be used for this royalty. There are already NFTs that are minted today on mainnet that have this logic in them.

An capability to a FT.Receiver also has the ability to get the address of that if for some reason the current Capability does not check.

My suggestion is quite simple and it is

      pub struct Royalties {
		pub let royalties: [Royalty]

		pub init(royalties: [Royalty]){
			self.royalties=royalties
		}
	}

	
	pub struct Royalty{
		pub let wallet:Capability<&{FungibleToken.Receiver}>
		pub let cut: UFix64
		pub let description:String

		init(wallet:Capability<&{FungibleToken.Receiver}>, cut: UFix64, description:String ){
			self.wallet=wallet
			self.cut=cut
			self.description=description

		}
	}

Note that if you only support a single FT type for this NFT you should expose another view that says that you are only tradable as that Type. But that is yet another discussion.

@bjartek bjartek closed this as completed Feb 16, 2022
@bjartek
Copy link
Contributor Author

bjartek commented Feb 16, 2022

Oh my i managed to close this by accident. I wanted to abort a comment I made and pressed the wrong button.

@bjartek bjartek reopened this Feb 16, 2022
@satyamakgec
Copy link
Contributor

Please have a look at this, Revised standard with capability support.

https://github.com/onflow/flow-nft/blob/wip-royalties/contracts/RoyaltyViews.cdc

Happy to receive the feedback.

@bluesign
Copy link

bluesign commented Mar 8, 2022

@satyamakgec

I am only hesitant on this:

recepient.check() : "Couldn't able to borrow the capability"

It looks like good idea at first, but failing on construct can be little problematic in the future. ( You cannot read royalty, if one of the participants somehow messes up with capability ) This will be called on every request on to generate Royalty Struct.

If one artist gets pissed and removes capability ( or hacked or any other reason ) not only them, but also everyone loses royalty.

I think marketplaces should do the check when sending the royalties.

PS: of course we can argue NFT contract can check too, but as marketplace will check anyway when sending ( like StoreFront does now ) it complicates unnecessary to create the royalty view.

@bjartek
Copy link
Contributor Author

bjartek commented Mar 8, 2022

That is a good point @bluesign also, having the capability even though it does not check is usefull because it also has an address associated with it. So you could emit an event or even escrow the funds if a marketplace has that feature.

But I guess when constructing the view you could only create the Royalty struct for this particular if is does link.

@bluesign
Copy link

bluesign commented Mar 8, 2022

Also does access(contract) allow read access?

access(contract) let royalties: [Royalty]

@bjartek
Copy link
Contributor Author

bjartek commented Mar 8, 2022

This looks very similar to some of my earlier suggestions so I am happy with the gist of it.

According to https://docs.onflow.org/cadence/language/access-control/ access(contract) is not readable by any other resource/contracts so it should either have an accessor or be less restrictive.

@joshuahannan
Copy link
Member

I agree about removing the recepient.check(). That is a good catch.

And yes, royalties should be access(self) and there should be a getter function for it

pub fun getRoyalties(): [Royalty] {
    return self.royalties
}

@bjartek
Copy link
Contributor Author

bjartek commented Mar 8, 2022

With the upcomming mutability change to arrays that are pub let, does it really make a difference if this is pub let vs access(self) and an accessor?

@joshuahannan
Copy link
Member

Not really. That is just what I default to now

@bluesign
Copy link

bluesign commented Mar 8, 2022

I think it is coming soon. onflow/cadence#1267

@satyamakgec
Copy link
Contributor

@satyamakgec

I am only hesitant on this:

recepient.check() : "Couldn't able to borrow the capability"

It looks like good idea at first, but failing on construct can be little problematic in the future. ( You cannot read royalty, if one of the participants somehow messes up with capability ) This will be called on every request on to generate Royalty Struct.

If one artist gets pissed and removes capability ( or hacked or any other reason ) not only them, but also everyone loses royalty.

I think marketplaces should do the check when sending the royalties.

PS: of course we can argue NFT contract can check too, but as marketplace will check anyway when sending ( like StoreFront does now ) it complicates unnecessary to create the royalty view.

In my opinion I don't think it will be problematic, On the other hand it will be good to have an early check as to remove the bad actors upfront instead of handling them afterwards or relying on some other contract to handle the similar case. It is more like if the passenger have the valid passport then only it can fly otherwise the passenger are not allowed to fly, Similarly even if the artist is malicious or whatnot it would get removed from receiving the royalty by the setter of the royalty until unless it provides the valid capability.

So I don't see a big issue here or may be I misunderstood what you are trying to say here.

Although I agree on the access modifier that I changed as suggested.

@joshuahannan
Copy link
Member

@satyamakgec The problem is that whenever someone queries an NFTs royalty view, that NFT has to construct a new copy of Royalties. If any of the royalty recipients revoke their capability, it will make it impossible for any of the other recipients to receive the royalties since the pre-condition will fail, which can't happen. I agree with bluesign that checking the capability should be the responsibility of the marketplace.

Also, looking at the naming, it might make sense to change the naming of the royalties field to cutInfos or something like that since Royalties.royalties is a bit awkward. Anyone have any ideas?

@bluesign
Copy link

bluesign commented Mar 9, 2022

is more like if the passenger have the valid passport then only it can fly otherwise the passenger are not allowed to fly,

In your case you are grounding plane for one passenger.

this structs are generated each time metadata is queried ( they are not designed to be create once than used ) more like create each time from NFT properties. ( think as views to properties )

So lets say, capability can’t check, it causes all metadata query to fail. (Script or transaction to panic)

@bjartek
Copy link
Contributor Author

bjartek commented Mar 9, 2022

Another thing I think we should do when we release new views is to create a implementors guide for them. Because using them properly is important I think.

One of the items we need to handle in this specific case is what to do when resoling royalties if a royalty receiver cap does not check. There are several options that are possible and I think it is worth noting down some of them in a document.

Options in my eyes:

  • skip the item and emit an event that says why royalty was not paid
  • send the funds to another wallet for a marketplace and emit an event.

I would not recommend outright skipping the item without emitting an event.

Another aspect I think it important. is that the source of truth for the royalties should be fetched from the NFT when it is sold, not when it is listed. Contracts are creative with royalty and storing this information in some intermediary outside of the NFT is not something I think we should recommend.

Concrete example:
In the neo contracts one of the royalties is to the Founder of the team. If that hanges the founder royalty will change and that should be reflected if the item was listed before the change.

@bjartek
Copy link
Contributor Author

bjartek commented Mar 9, 2022

Also, looking at the naming, it might make sense to change the naming of the royalties field to cutInfos or something like that since Royalties.royalties is a bit awkward. Anyone have any ideas?

cuts or items or cutInfos work. I agree that the double Royalties is awkward.

@satyamakgec
Copy link
Contributor

@satyamakgec The problem is that whenever someone queries an NFTs royalty view, that NFT has to construct a new copy of Royalties. If any of the royalty recipients revoke their capability, it will make it impossible for any of the other recipients to receive the royalties since the pre-condition will fail, which can't happen. I agree with bluesign that checking the capability should be the responsibility of the marketplace.

Also, looking at the naming, it might make sense to change the naming of the royalties field to cutInfos or something like that since Royalties.royalties is a bit awkward. Anyone have any ideas?

ahh Make sense, I am good with cutInfos.

@hier01
Copy link

hier01 commented Mar 9, 2022

Would there be any use in the Royalty having an identifier, so adjustments could target specific cuts? I think the ability to adjust royalty structures based on negotiated rates or other factors will be important. The Description could serve that purpose, but I'm not sure it was intended for that.

@bjartek
Copy link
Contributor Author

bjartek commented Mar 9, 2022

In my eyes the description is for just that @hier01.

@hier01
Copy link

hier01 commented Mar 9, 2022

Cool, sounds good @bjartek. I'm good with 'cutInfos' or even just 'cuts'. Can I get a pointer to the switchboard thing? Is that a FLIP?

@louisguitton
Copy link

@satyamakgec @psiemens now that the Royalty view has been merged, can you summarise here the status of this issue and if you consider it closed for now?

@joshuahannan
Copy link
Member

@louisguitton Yes, the royalty view is complete, so I'll close this issue. There is documentation in the README, and the changes have been deployed to testnet and mainnet. Not many projects and marketplaces have integrated it yet, but it is definitely ready to start integrating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Feedback Metadata View SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board
Projects
None yet
Development

No branches or pull requests

10 participants