Minor improvements from review

[patitonar]

Besides the already applied fixes mentioned in #8, here are some other improvements I identified from my review.

1. Avoid calling depositTokens.at twice in FeeCollector.deposit(). In order to save some gas, the following code:

      IERC20 _tokenInterface = IERC20(depositTokens.at(index));

      _currentBalance = _tokenInterface.balanceOf(address(this));
      
      // Only swap if balance > 0
      if (_currentBalance > 0) {
        // create simple route; token->WETH
        
        path[0] = depositTokens.at(index);

can be modified to:

      address _tokenAddress = depositTokens.at(index);

      _currentBalance = IERC20(_tokenAddress).balanceOf(address(this));
      
      // Only swap if balance > 0
      if (_currentBalance > 0) {
        // create simple route; token->WETH
        
        path[0] = _tokenAddress;

2. Similar to other methods, FeeCollector.setSplitAllocation() can be defined as external instead of public and use calldata for the _allocations parameter

3. In the FeeCollector contract, the depositTokens state variable is modified to include new elements in two methods: the constructor and registerTokenToDepositList. However, different checks are implemented to validate the new tokens added to the list in these two methods.
   In the constructor the following check is provided:

      require(_initialDepositTokens[index] != address(0), "Token cannot be  0 address");

and in registerTokenToDepositList the following ones:

    require(depositTokens.length() < MAX_NUM_FEE_TOKENS, "Too many tokens");
    require(_tokenAddress != weth, "WETH not supported"); // There is no WETH -> WETH pool in uniswap
    require(depositTokens.contains(_tokenAddress) == false, "Already exists");

Does it make sense to include the 4 validations in both cases? The logic could be placed in a new internal method called from the constructor and registerTokenToDepositList

4. Same question from the previous point but for depositTokens from the SmartTreasuryBootstrap contract.

5. Regarding code styles, all external methods seem to be named without a prefix for public and external methods (example: deposit()) and the internal methods seem to be named with a _ prefix (example: _depositAllTokens()). This applies to both contracts except for some external methods from the SmartTreasuryBootstrap such as _setIDLEPrice, _registerTokenToDepositList, _removeTokenFromDepositList and other view methods. Does it make sense to remove the _ prefix from the mentioned methods for consistency and to avoid confusion when reading the code?

[asafsilman]

Hey @patitanor, thank you for the feedback, here's what I've implemented so far

1. I've added this optimisation, thanks
2. I've implemented this change
3. I've added the checks now to the constructor, I cannot use the registerTokenToDepositList function in the constructor at this stage, since the weth state variable is immutable, therefore not read accessible in the constructor.
4. See previous comment, similar changes implemented
5. This was an artifact from early development and is now fixed, thanks for raising this.

git hash: a974958