[DMA V2] Gas-Optimised

[fenris85]

Gas optimisations changes cherry picked from old Georgi's branch (https://github.com/OasisDEX/oasis-earn-sc/tree/gg/v2-sc-gas-optimized)

Main changes regard OperationRegistry and how we are storing operations - now it's hash of actions hashes along with operation name.

OperationExecutor has additional method for Automation purposes to get opName by hash easily.

Actions that were using read or write to the storage has changed (Aave V3 and Common so far).

Update to Actions required by Migration feature (add onBehalf to Aave V3 Payback, added params mapping Aave V3 Borrow, add Common TokenBalance Action)

[halaprix]

I'd add the check if the operations[operationHash] != bytes32("")

[halaprix]

is .this necessary ?

[halaprix]

that won't work I think, if the executeOp is delegatecall'ed - as this will be the proxy

[robercano]

Some of the changes will break this PR, so we need to merge that one before we merge this.

[robercano]

I'd suggest to abstract this bit away in a separate contract:

contract UseRegistry {
  ServiceRegistry private immutable _registry;
  
  constructor(ServiceRegistry registry_) {
    _registry = registry_
  }
  
 function getRegisteredService(string service) return address
 {
   return _registry.getRegisteredService(service);
 }
}

Right now the end result is very similar, but moving forward this abstracts access to the registry and gives us more flexibility to introduce checks and advanced functionality if needed, without having to change all the actions

[robercano]

This is a tricky one: do we want to pull the whole balance or rather the whole allowance? We will never be able to pull more than the allowance, but if the allowance is also type(uint256).max then we can pull the balance. Should we use the smaller of the two?

[fenris85]

One of the use cases here is that frontend would set up allowance before the tx. And thus exact amount is not known before the tx (it could be aToken for example) the allowance is slightly bigger to cover any additional amount. But if we would pull this allowed amount it might turn out there is little less than the allowance. So I would stick to the token balance, as it's specific to use cases we have right now.

[robercano]

Thanks for the insight!

[robercano]

This will fail for tokens like USDT, please use safeTransferFrom instead. Also, if possible, use SafeERC20 from OpenZeppelin library, instead of our custom implementation of SafeERC20. If the size of the contract is a concern, then we can copy/paste the latest version, but I'd suggest just using it

[robercano]

If we are going to touch the executor I'd suggest moving errors and events into an interface file to clean it up a bit. The interface would contain the public functions, errors, enums and events. ALso other interfaces (like IProxy) if it makes sense.

[robercano]

Is this correct? All of our contracts are AGPL

[robercano]

We could cache the value of this keccak256 in the constructor as immutable and then return it here, saving some gas

[robercano]

Same comment for ReentrancyGuard

[robercano]

This is breaking the public interface and at least 2 files are affected:

• operations-registry.ts which I believe is used in tests and deployment system
• tasks/operation-registry which is one of the tools I did.

Fix should be trivial though

[robercano]

It should return (string name) so it uses the same interface as addOperation. Otherwise I'd change addOperation to use bytes32 name as well

[robercano]

This should be external only