tokencard / contracts

The Consumer Contract Wallet
GNU General Public License v3.0
94 stars 40 forks source link

Add extra stats and view methods to tokenWhitelist #583

Closed i-stam closed 3 years ago

i-stam commented 4 years ago

With all the counters that we introduced, would it not make more sense to just use the existing array of tokens and check how many tokens are loadable and how many tokens exist?

Something like this:

    function tokenCount() external view returns (uint256) {
        return _tokenAddressArray.length;
    }

    function loadableCount() external view returns (uint256) {
        uint count = 0;
        for (uint i=0; i < _tokenAddressArray.length; i++) {
           if (_tokenInfoMap[_tokenAddressArray[i]].loadable) {
               count.add(1)
           }
        }
        return count;
    }

The idea was that these counters are going to be used the same way that redeemableCounter is used (which is called as part of an actual transaction) so I did a similar implementation. As long as the tokenCounter is concerned, I think that you are right, it doesn't make sense since it can be obtained by checking the array's length.

nostdm commented 4 years ago

It seems to me that it would simplify things if we used the existing map/array to determine how many loadable/redeemable tokens there are. At least we could compare the gas cost of setting the counter with every transaction vs looping over the tokenAddressArray when the count is needed. And it seems that we use the redeemableCount only in the holder.sol contract so even if it cost more gas to loop over the array in a transaction, it would reduce the amount of state in the contract.

Similar to the loadableCount:

    function redeemableCount() external view returns (uint256) {
        uint count = 0;
        for (uint i=0; i < _tokenAddressArray.length; i++) {
           if (_tokenInfoMap[_tokenAddressArray[i]].redeemable) {
               count.add(1)
           }
        }
        return count;
    }