Gas Optimizations

[code423n4]

• function _validatePublishingEnabled() would be cheapier if it did not check all the other states, but just Unpaused:

  if (_state != DataTypes.ProtocolState.Unpaused) {
    revert Errors...();
  }

• Because MAX_HANDLE_LENGTH is 31, I think you can use bytes32 to store the handles. This would be cheaper than dynamic size strings or bytes.

• The same storage is accessed many times in a function, e.g. _dataByPublicationByProfile[profileId][pubId] is accessed 3 times in _processCollect:

  uint256 amount = _dataByPublicationByProfile[profileId][pubId].amount;
  address currency = _dataByPublicationByProfile[profileId][pubId].currency;
  ...
  address recipient = _dataByPublicationByProfile[profileId][pubId].recipient;

Would be better to cache this in a variable and re-use.

• I think it would be cheaper here to first increment the pubCount and the pass the updated value (avoid repeated add operation):

  PublishingLogic.createComment(
      vars,
      _profileById[vars.profileId].pubCount + 1,
      _profileById,
      _pubByIdByProfile,
      _collectModuleWhitelisted,
      _referenceModuleWhitelisted
  );
  _profileById[vars.profileId].pubCount++;

• I wonder do you really need to pass the whole mapping to another function if you use just one element from it, e.g. function createComment in PublishingLogic library takes the whole _profileById mapping but uses only pubCount of one element:

   mapping(uint256 => DataTypes.ProfileStruct) storage _profileById
   ...
  // Validate existence of the pointed publication
  uint256 pubCount = _profileById[vars.profileIdPointed].pubCount;
  if (pubCount < vars.pubIdPointed || vars.pubIdPointed == 0)
    revert Errors.PublicationDoesNotExist();

Can't you just pass only data that you actually need and use, or even pass this pubCount of profileIdPointed from the caller?
There are many more functions that take these big storage pointers as parameters, so in my opinion, you should consider optimizing this.

• ++ and -- are a bit cheaper than += 1 and -= 1 :

_balances[to] += 1;
_balances[owner] -= 1;
_balances[from] -= 1;

[Zer0dot]

The unpaused check is a good catch!

The 32 byte handles is something we've discussed but breaks the assumption that, via upgradeability, handles could be longer. Anyway right now they are stored in 32 byte words because length is packed in the same slot when it's < 32 bytes. The point about storage accesses is not valid because each element in a struct is accessed separately, loading the entire struct is less efficient. Incrementing the publication count with _createComment introduces a vulnerability that allows the comment to comment on itself, so that's invalid too. Passing the pointed pubCount directly increases the LensHub contract size and is not a tradeoff we want to take. Lastly, the increment point is made invalid by the optimizer.

[Zer0dot]

First issue addressed here: lens-protocol/core#68