Simplify storage of haloHaloContract in RewardsManager

[cleanunicorn]

Description

During the RewardsManager deploy a few storage variables are set.

https://github.com/monoceros-alpha/review-halodao-rewards-2021-06/blob/90dd3734a922bb9dcb80fffba34fe9db3065c4d0/code/contracts/RewardsManager.sol#L26-L37

The variable storing the address for the HaloHalo contract is received as _haloHaloContract in the arguments.

https://github.com/monoceros-alpha/review-halodao-rewards-2021-06/blob/90dd3734a922bb9dcb80fffba34fe9db3065c4d0/code/contracts/RewardsManager.sol#L29

The contract's address is stored in haloHaloContract

https://github.com/monoceros-alpha/review-halodao-rewards-2021-06/blob/90dd3734a922bb9dcb80fffba34fe9db3065c4d0/code/contracts/RewardsManager.sol#L34

And it is also stored in a separate variable as the HaloHalo interface

https://github.com/monoceros-alpha/review-halodao-rewards-2021-06/blob/90dd3734a922bb9dcb80fffba34fe9db3065c4d0/code/contracts/RewardsManager.sol#L35

However, both of the values are identical in the contract's storage.

Consider this simplified example

interface SomeContract {
    function doSomething() external;
}

contract Save {
    SomeContract someContract;
    address someContractAddress;
    
    constructor(address _someContractAddress) {
        someContract = SomeContract(_someContractAddress);
        someContractAddress = _someContractAddress;
    }
}

The contract Save receives an argument on deploy and proceeds to save the address both as an address (as someContractAddress), but also as a SomeContract interface (as someContract).

We deployed this contract locally (on a Ganache instance) and checked the both of the storage slots in the contract after a successful deploy.

We connected to the local instance with Hitomi and we're dropped in a web3 enabled local console.

$ hitomi http://localhost:8545
Starting Hitomi v0.3.2.

Connected to http://localhost:8545.

Node version: EthereumJS TestRPC/v2.13.2/ethereum-js
Enode path: None
Protocol version: 99
Chain ID: 1337
Block number: 0
Mining: False (0 hash rate)
Syncing: False

We proceeded to check the first storage slot representing SomeContract someContract;

>>> web3.eth.getStorageAt("0x6F3dC8C964d3F2ce4A1Ba8CD6Da6E7320A69CC6D", 0)
HexBytes('0x5b38da6a701c568545dcfcb03fcb875f56beddc4')

And the second storage slot representing address someContractAddress;

>>> web3.eth.getStorageAt("0x6F3dC8C964d3F2ce4A1Ba8CD6Da6E7320A69CC6D", 1)
HexBytes('0x5b38da6a701c568545dcfcb03fcb875f56beddc4')

We can see both values are identical. Solidity needs to cast an address as an interface to know which methods are available and how to use them. The generated assembly doesn't do anything specific to the initially provided address, this is just syntactic sugar.

Because both values are identical, we don't need to save both of them in separate storage slots, this makes the save (or update) and the read more expensive. Saving only one of them is better because, on read, it doesn't need to retrieve both of them and storage handing (reading and writing) is very expensive.

Recommendation

Use only one of the storage slots to save either the HaloHalo contract and cast when needed to either access the address or the interface.