-
Notifications
You must be signed in to change notification settings - Fork 170
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
Comments
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 |
In my eyes a good royalty event should contain the following fields
Fields I am not so sure about are:
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. |
Awesome! Thanks for the input
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. |
@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. |
Yeah sure @joshuahannan , Below is the representation what I think will be sufficient to facilitate the royalty information within the NFTs.
The idea it to keep it flexible so implementer of the Happy to receive feedback. |
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. |
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. |
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.
[ {capToArtist, 0.25, "artist"}, {capToMinter, 0.1, "minter"}] Is that the correct way to interepet your suggestion? Yes, you interpret it right.
Agree with this as I am still in the Ethereum hangover so it makes sense to remove the cc @bjartek |
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 cc @bluesign |
@satyamakgec I was thinking a bit in the context of other contracts like If we can somehow manage a btw when I say in RoyaltyHelper Contract: in RoyaltyMetadata can have some default implementation like:
so any user somehow can:
But my main point is if we suggesting |
@bluesign 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. |
This is a much more complicated view discussion than the 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? |
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. |
@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 ? |
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: |
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. |
@bjartek's solution is great, I think we need a generic receiver interface for sure. Somehow I am torn on the 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. |
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. |
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. |
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 We were also thinking that the royalty implementation should just deposit to the proxy. |
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. |
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 |
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 For what @psiemens mentioned
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. |
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.? |
@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
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. |
Oh my i managed to close this by accident. I wanted to abort a comment I made and pressed the wrong button. |
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. |
I am only hesitant on this: flow-nft/contracts/RoyaltyViews.cdc Line 50 in 226d5b8
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. |
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. |
Also does flow-nft/contracts/RoyaltyViews.cdc Line 62 in 226d5b8
|
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. |
I agree about removing the And yes,
|
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? |
Not really. That is just what I default to now |
I think it is coming soon. onflow/cadence#1267 |
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. |
@satyamakgec The problem is that whenever someone queries an NFTs royalty view, that NFT has to construct a new copy of Also, looking at the naming, it might make sense to change the naming of the |
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) |
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:
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: |
|
ahh Make sense, I am good with |
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. |
In my eyes the description is for just that @hier01. |
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? |
@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? |
@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. |
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:
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
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
The text was updated successfully, but these errors were encountered: