Inconsistency with the Rarible royalty registry and royalty implementation

[eduardstal]

Question Posted: 06 July 2021, in Discord

wwhchung#4440:

@eugene Nacu | Rarible hello! I'm Wilkins from Manifold. I've been going through your contracts and noticed an inconsistency with the Rarible royalty registry and royalty implementation. (Note: I love how you allow for people to set a provider for backwards compatibility)
anyways, here it is:
https://github.com/rariblecom/protocol-contracts/blob/master/royalties/contracts/LibRoyaltiesV2.sol

defines:
function getRoyalties(LibAsset.AssetType) == 0x44c74bcc

But:
https://github.com/rariblecom/protocol-contracts/blob/master/royalties/contracts/RoyaltiesV2.sol

and
https://github.com/rariblecom/protocol-contracts/blob/master/royalties/contracts/impl/RoyaltiesV2Impl.sol

defines as:
function getRoyalties(uint256 id) external view returns (LibPart.Part[] memory) (which has a signature 0xbb3bafd66)

In implementations:
https://github.com/rariblecom/protocol-contracts/blob/master/royalties-upgradeable/contracts/RoyaltiesV2Upgradeable.sol

and
https://github.com/rariblecom/protocol-contracts/blob/master/royalties-registry/contracts/RoyaltiesRegistry.sol

reference the signature 0x44c74bcc, which is different from the implementation references.

So, the question is:

1. Did you intend to actually use LibAsset.AssetType as the parameter to getRoyalties? If so, then those implementations and interfaces seem to need fixing.
2. If not, then their function signature collides with ours (we implement getRoyalties(uint256) as well, but return two arrays, receivers and bps. Our contracts are out there already in a bunch of places, so I was wondering if we could co-ordinate and either have getRoyalties in Rarible's definition be two arrays as well, or possibly a rename on Rarible's end as to avoid a collision.

Sorry about the collision, I was relying on the signature and definition in LibRoyaltiesV2.sol to avoid it, but it seems the actual usage might be different.

Eugene Nacu | Rarible#9332:

@wwhchung could you send me a code of your contracts so I can check it and find a workaround

wwhchung#4440:

sure, here's an example:

https://etherscan.io/address/0x00372812abcb11c2ea291aeb7f612d9e0524b013#code

specifically, the code is in CreatorCore.sol

bytes4 private constant _INTERFACE_ID_ROYALTIES_CREATORCORE = 0xbb3bafd6;

ICreatorCore.sol function getRoyalties(uint256 tokenId) external view returns (address payable[] memory, uint256[] memory);

a number of artists/creators have this out now

Eugene Nacu | Rarible#9332:

So, I think we need to fix interface id for RoyaltiesV2
Regarding collission - are there many such contracts?
We can set adapter (IRoyaltiesProvider) for these contracts

wwhchung#4440:

well, yes, for rarible specific use case, we can implement RoyaltiesV2 and set a provider. But the reason I wanted to avoid collision was for the longer term. I think it would be beneficial to differentiate between a royalty provider implementation and a base contract implementation.

One of the things I'm coding up right now (and I'll send to you briefly) is an extended concept of your RoyaltyRegistry, one which does the following (and theoretically could be used by any marketplace to do royalty lookup):

Retrieve:
One common interface, like:
getRoyalty(address tokenAddress, uint256 tokenId, uint256 value) public view override returns(address payable[] memory recipients, uint256[] memory amounts)

Logic:
1. Check if providerAddress (contractAddress) supports EIP2981. if so, use it
2. Check if providerAddress (contractAddress)supports rarible v1/v2. if so, use it
3. Check if providerAddress (contractAddress)supports manifold.  if so, use it
4. Check if providerAddress (contractAddress) supports .... (x future spec), if so, use it

The problem with this approach, and if our royalty implementation signatures aren't differentiated, is that this logic kind of fails.

I'll be checking in the code and making the repo public shortly for thoughts from the broader marketplaces.

(just writing some tests right now)

note re: above, when I say providerAddress, it gives similar optionality as what you did to allow old contracts to set up a provider.

---

@eugene Nacu | Rarible ok, I just went ahead and checked it in. It's relatively simple. Would love your thoughts.
https://github.com/manifoldxyz/royalty-registry-solidity

The idea is to provide a global unified royalty interface across all specs.

I understand the provider design, but I also thought that there was a possibility that people would take RoyaltiesV2.sol and use it directly in a smart contract. If this were the case, it would result in a conflict between us and RoyaltiesV2 spec (i.e. a marketplace could not determine the return type appropriately).

for Rarible marketplace specifically, I agree, there's no immediate issue (since I would simply implement IRoyaltyProvider, which I need to do anyways, and set it for all manifold contracts on the Rarible marketplace).
My concern is when:

• Token contracts implement RaribleV2 directly (similar to how token contracts are already directly implementing RaribleV1)

• Other marketplaces which try to get royalties directly, will run into a conflict of return type between contracts that are using RaribleV2 interface and Manifold's contracts.

e.g.

contract NewContract is RoyaltiesV2

vs

contract NewContract is ERC721Creator

will yield supportsInterface = true for 0xbb3bafd6, but will have different return values. Which is problematic for any marketplaces that try to use it directly, or even in the royalty registry concept above.

Eugene Nacu | Rarible#9332:

@wwhchung thanks, will check this

[eduardstal]

Since we need to group and filter the discussions page, I'll be adding this reply as an answer since these kinds of threads(from discord) won't have activity any time soon.