CW-2981 extension #24
Conversation
|
Will address the comments and add some more tests 👍 |
|
If this is GTM could it be merged? Otherwise before long I'll have to update the PR because things will have shifted.... 🙃 |
Well if things shift it will need to be updated anyway. 🙃 Needs one more thumbs up. |
|
|
||
| pub fn check_royalties(_deps: Deps) -> StdResult<CheckRoyaltiesResponse> { | ||
| Ok(CheckRoyaltiesResponse { | ||
| royalty_payments: true, |
There was a problem hiding this comment.
Wait, always true?
What about ext.royalty_percentage.is_some()
There was a problem hiding this comment.
Me being a little lazy as it's an example contract - it's a check at contract level, for this one the answer to "should I see if, when a token controlled by this contract is sold, royalties are owed?" will always be yes. I think ext.royalty_percentage.is_some() would be relevant if you were querying on the token_id. Then again, this is based on my interpretation of the granularity of the EIP spec, so I could have it wrong 🙂
There was a problem hiding this comment.
Seems like this is a case where we don't need to follow Ethereum exactly. The boolean seems redundant.
There was a problem hiding this comment.
No, it will still return boolean regardless - that's the interface, I'm just saying that ext.royalty_percentage.is_some() is at a different level of granularity - token, vs contract
There was a problem hiding this comment.
Rechecked the spec and this interface needs to return a boolean, so yeah the answer to:
Checking if the NFT being sold on your marketplace implemented royalties
For a function that takes a contract and returns a boolean - this is the correct base case, and then if contracts want to implement custom logic, they can. This is just the most basic correct example, which is that when token is sold, it signals you should call query_royalties_info with the token ID.
Happy to write a better description of this in the README or what have you, but the important thing is to set the expectation from marketplaces that they should check, in case there is more complex behaviour than a default true.
In some ways, this repo is also a way of documenting How To Do Things With NFTs:tm: so I'm not that worried that the base case is ostensibly redundant, just that it is enumerated for those that have a requirement for more custom logic in future.
To that end, I will add a better comment on it in the code now.
There was a problem hiding this comment.
Still not a big fan of the CheckRoyalties query as I think it's mostly pointless. It's more important in the context of Ethereum.
Could just query RoyaltiesInfo and see if it returns a response for the same effect.
There was a problem hiding this comment.
@orkunkl, any thoughts on this?
the-frey
dismissed stale reviews from ethanfrey and JakeHartnell
via
89f3ea7
|
Okay I think that's all the review points closed off. |
|
@the-frey Do you think we can get this PR in this year xp |
|
@orkunkl I mean, if people want to review it then maybe we can get it in lol |
|
Okay @orkunkl and @JakeHartnell have updated again with changes from main which means it has cleared your reviews. If you are both still ok to merge, could you review pls 🙏 |
Appreciate this might need some refining and/or further discussion.
A minimal example of the EIP-2981 spec implemented in CW-land. i.e. #7
It's a very light wrapper/extension for CW721 compliant contracts that adds two additional query messages:
RoyaltyInfoto be called when selling an NFT, to find out what royalties are owedCheckRoyaltiesto be called on the contract, to see if it implements CW-2981This adds royalty information at token mint time.
The main thing are the queries that are called when the marketplace sells an NFT. Different contracts might implement them differently, e.g. to do more complex logic than the straight percentage here, but the returned queries should conform to the shape here.
Aside - could move those (and the extension handlers) to a package, if it makes them more reusable.