Known issues/optimisations

[DavidMinarsch]

• [x] address remaining coverage gaps
• [x] we loop and request the same data twice in one function: here and here; probably more can be optimised across these two loops.
• [x] this wondering whether the address lookup can be optimised by storing the list of addresses permanently so we don't need to look up each time.
• [x] if this map was to serviceId instead of serviceOwner we'd probably save a lot of lookups in the loops as we'd only need to lookup unknown addresses; further casual inspection suggests we don't need the owner at all and can get away with only the service id
• [x] remove ERC721Enumerable (see an argument here) - we don't need it anywhere: 1 can use IERC721 instead; 2 can be achieved via simple counter if we remove burn functionality (which we don't need, as here we can simply set the service state to burned, which also makes tokenomics better as for whatever reasons late arriving donations won't cause any issues); 3 same argument if we remove burn - just the identity; 4 same as 1; 5 same as 2
• [x] this and this and this and this and this is not used
• [x] with the removal of burn in ERC721, existence checks can be done more easily by checking once that highest index is below total supply here and here
• [x] Flatten and gas optimise ERC20 by implementing it similarly to the Uniswap contract with a peak at Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC20.sol) - although we cannot directly reference solmate and should avoid too much similarity due to the licensing issue - (take this data as reference: https://github.com/alephao/solidity-benchmarks)
• [x] Restrict agents to have at least one component for symmetry with services having at least one agent.
• [x] Convert all ipfs hashes for v1 support only
• [x] this and this and this and this and this can be made external
• [x] this should be made internal
• [x] The Pausable implementation here does not make sense. We cannot recover the funds anyway once they are in the disperser. So I suggest we remove Pausable here. The transfer from Treasury to Dispenser is governance controlled anyway atm.
• [x] optimise storage layout, especially here
• [x] IStructs creates dependencies between the entire architecture. I think it would be desirable to limit the dependencies to the minimum of the shared data structures, suggesting it is broken up. Affecting e.g. this creates
• [x] We should use a more generic interface here which the Treasury then implements. We only use one method of the treasury interface here and if we don't change it we are forced to implement all treasuries with this interface.
• [x] We have to be careful with this. When we deploy the Treasury will not be live yet. So for some time this functionality is broken. Will it revert if the zero address is initially set here?

[77ph]

• [ ] https://github.com/valory-xyz/autonolas-v1/blob/37ba1f92585f14087f8a8992a390b22d3c966edc/contracts/tokenomics/Depository.sol#L95 remove address token as arg. token = address(product.token);
• [ ] https://github.com/valory-xyz/autonolas-v1/blob/e53b81535a76ea9c4896f8d6c33bb66653b3358c/contracts/tokenomics/Treasury.sol#L165 inconsistent check - first if (bonding) , next token == ETH_TOKEN_ADDRESS. then with token = ETH_TOKEN_ADDRESS and bonding = true there will be misbehavior

[kupermind]

re: this wondering whether the address lookup can be optimised by storing the list of addresses permanently so we don't need to look up each time.

Components and agents are ERC721 tokens and their owners can change, so every time this function is executed, we need to look up for actual owners.