Does the registration function mix up `toHash` and `fromHash`?

[brookr]

Please forgive me if I'm not reading this correctly...

I'm reviewing this function:

    function registerDelegationAddress(address _collectionAddress, address _delegationAddress, uint256 _expiryDate, uint256 _useCase) public {
        require((_useCase >0 && _useCase < useCaseCounter) || (_useCase == 99));
        bytes32 toHash;
        bytes32 fromHash;
        bytes32 globalHash;
        globalHash = keccak256(abi.encodePacked(msg.sender, _collectionAddress, _delegationAddress, _useCase));
        toHash = keccak256(abi.encodePacked(msg.sender, _collectionAddress, _useCase));
        fromHash = keccak256(abi.encodePacked(_delegationAddress, _collectionAddress, _useCase));
       ...

It seems that the toHash value is created with the msg.sender address, which would be the address of the wallet requesting the delegation, the FROM wallet.

And then the fromHash value is created based on the _delegationAddress passed in to the function, which under normal language circumstances would be referred to as the address we want to delegate TO.

So this is ether exactly backwards, or I'm reading it wrong?

The same struct is pushed into both delegateToHashes and delegateFromHashes, so this might have passed basic testing. But the counter vars, like delegationToCounterPerHash are now keyed off the wrong hash.

[rookmate]

I think you're correct. FROM should be the msg.sender and TO should be _delegationAddress :thinking: And this may even explain why the last test on #3 was failing as the TO and FROM get swapped on registration...

[brookr]

Thanks for the review, @rookmate, much appreciated!

Also: Yay for tests! Really looking forward to seeing the test suite get merged.

[rookmate]

me too! I've suggested some changes to the PR but I'm eager it is merged so we can add more tests

[brookr]

Great to see the code base updated!

Here's a new PR, to address this same issue as it still exists in version 4.33.

[skyn3t2030]

Thanks Guys for the comments.

All delegations registered create a toHash as the cold wallet address delegates TO a hot wallet.

The fromHash is created for the delegation addresses as for they were delegated FROM a cold address.

I hope now is clear.

[brookr]

Thanks for the reply!

All delegations registered create a toHash as the cold wallet address delegates TO a hot wallet.

I'm so sorry, I don't understand yet. If it delegates TO a hot wallet, shouldn't the hot wallet address be in the toHash?

The fromHash is created for the delegation addresses as for they were delegated FROM a cold address.

This sounds backwards to me, and I'm concerned others reviewing the code won't understand why it is in this way. Is there another variable name we could use for these variables, to avoid any confusion?

Maybe:

• toHash could be delegatorHash
• fromHash could be delegatedHash

Or something similar?

[skyn3t2030]

I appreciate your comment and will be taken into consideration in future release after we finalize the main functionality of the contract. This issue will remain open until we change the name of hashes.

[skyn3t2030]

Changed declarations and mappings on the new smart contract.