equipInstallation and unequipInstallation functions are missing transfer events

[mudgen]

The equipInstallation and unequipInstallation functions from the InstallationFacet.sol facet are missing transfer events.

These functions remove or add installations to owner addresses but do not emit the ERC1155 events for those actions. The events need to be added to be compliant with the ERC1155 standard.

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/dev/contracts/InstallationDiamond/facets/InstallationFacet.sol#L296

[orionstardust]

@mudgen CC: @cinnabarhorse I think, current implementation of equipInstallation() and unequipInstallation() functions are correct. ERC1155 is transferred to the user in craftInstallations(), and it emits LibERC1155.TransferSingle event.. In equipInstallation() function, there's no change for ERC1155. In unequipInstallation() function, ERC1155 is burnt, and it emits LibERC1155.TransferSingle event. So, current implementation is correct, I think.

[mudgen]

@orionstardust In equipInstallation() the ERC1155 token is transferred to the realm contract but there is no event that shows that transfer. The event should be emitted to comply with the ERC1155 standard. Also, if the event is not emitted then the ERC1155 events will not be consistent with what is returned by function balanceOf(address _owner, uint256 _id) external view returns (uint256); It will also not be consistent with how ERC1155 wearables/items are implemented in the Aavegotchi Diamond.

In unequipInstallation() the TransferFromParent event is not being emitted to show that the tokens were removed from the parent token.

To be technically accurate with the ERC1155 and ERC998 standards the equipInstallation() and unequipInstallation() functions should be adding and removing the tokens from the realm contract (if transferring ownership as is the current strategy). Here is an example:

  function _equipInstallation(
    address _owner,
    uint256 _realmId,
    uint256 _installationId
  ) internal {
    InstallationAppStorage storage s = LibAppStorageInstallation.diamondStorage();
    LibERC1155.removeFromOwner(_owner, _installationId, 1);
    LibERC1155.addToOwner(s.realmDiamond, _installationId, 1);
    emit LibERC1155.TransferSingle(...
    ERC998.addToParent(s.realmDiamond, _realmId, _installationId, 1);
    emit TransferToParent(s.realmDiamond, _realmId, _installationId, 1);
  }

  function _unequipInstallation(
    address _owner,
    uint256 _realmId,
    uint256 _installationId
  ) internal {
    InstallationAppStorage storage s = LibAppStorageInstallation.diamondStorage();
    ERC998.removeFromParent(s.realmDiamond, _realmId, _installationId, 1);
    emit TransferFromParent(..
    LibERC1155._burn(s.realmDiamond, _installationId, 1);
  }

The removeFromOwner and addToOwner internal functions are implemented in both libraries LibERC1155.sol and LibERC998.sol. I suggest removing the duplicate functions unless there is a reason not to.