Cannot register a delegation for all collections and then query a specific collection

[mpeyfuss]

Problem Statement

I believe that an improvement can be made to querying collections. Right now, if I delegate a wallet to all collections using 0x8888888888888888888888888888888888888888 as the value for _collectionAddress, I have to query the retrieveTokenStatus or retrieveGlobalStatusOfDelegation with 0x8888888888888888888888888888888888888888 as the collection address.

If I use an actual collection address, I will get back false, when I should expect back true. This is broken logic, in my opinion, and should be improved.

Potential Solution

The function below should first check against using 0x8888888888888888888888888888888888888888 and the collection address in the hash and if the logic should return false, the function should check against the supplied _collectionAddress.

This likely also impacts other retrieve functions as they all follow a similar pattern of creating a hash.

nftdelegation/smart_contracts/DelegationManagement.sol

Line 604 in 50e3d17

function retrieveGlobalStatusOfDelegation(address _delegatorAddress, address _collectionAddress, address _delegationAddress, uint256 _useCase) public view returns (bool) {

[mpeyfuss]

checkConsolidationStatus does a great job of checking against the supplied _collectionAddress and all collections.

nftdelegation/smart_contracts/DelegationManagement.sol

Line 1189 in 50e3d17

function checkConsolidationStatus(address _wallet1, address _wallet2, address _collectionAddress) public view returns (bool) {

[a2rocket]

Regards to checkConsolidationStatus as its used a predefined use case we have implemented a two way check within the function.

Now regards to the retrieveGlobalStatusOfDelegation the intended design was to make it more generic as the usecase is not predefined. Developers can make more than one call to the contract to check the delegation status.

Example:

Let's say we want to check for delegation status between a vault and the msg.sender on collection 0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db for usecase 1.

A developer can implement the following function that makes 1 call on the 0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db collection and one on the all collections address.

example code:

function checkMintingStatus(address _vault, address _collectionAddress) public view returns(bool) {
    return (dmcRead.retrieveGlobalStatusOfDelegation(_vault, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmcRead.retrieveGlobalStatusOfDelegation(_vault, _collectionAddress, msg.sender, 1));
}

The gas cost for this when called by a contract is just 12553.

By expanding the above example you can check for more usecases based on your purpose.

function checkMintingStatusMoreUseCases(address _vault, address _collectionAddress) public view returns(bool) {
    return (dmcRead.retrieveGlobalStatusOfDelegation(_vault, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmcRead.retrieveGlobalStatusOfDelegation(_vault, 0x8888888888888888888888888888888888888888, msg.sender, 2) || dmcRead.retrieveGlobalStatusOfDelegation(_vault, _collectionAddress, msg.sender, 1) || dmcRead.retrieveGlobalStatusOfDelegation(_vault, _collectionAddress, msg.sender, 2));
}

In this case we check the specified collection for usecases 1 and 2 as well as the all collections address for usecases 1 and 2 and the gas cost is just 20k.

[mpeyfuss]

Sure, it's definitely possible to implement logic to call the contract multiple times. However, ALL_COLLECTIONS is a standard set by the contract, so it only makes sense to check against that in the view functions rather than force that on the developer integrating with this product. If you want to keep as is, I suggest that you remove the check for ALL_COLLECTIONS from checkConsolidationStatus as you are not keeping to the same system across the contract, which makes it more difficult and confusing to use as a third party dev. If you keep the same system across the contract (checking against ALL_COLLECTIONS or not), plus documentation, it is much easier to integrate.

[mpeyfuss]

Seems like this issue has come up before. I think the last line in the comment below really shows what I'm after - aiding adoption of this product.

#32 (comment)