Bubbling up revert reason strings from Executor.sol and modules

[gitpusha]

Why?
This can make debugging and locating the cause of failed transactions way simpler and way faster.

So:
With regards to executeCall and executeDelegatecall in Executor.sol

It would be great if the functions did not only return success but also the bytes memory returndata.

This data could then be used to bubble up revert messages. For example like so:

(bool success, 
 bytes memory revertReason) = to.delegatecall.gas(txGas)(data)

if (!success)  {
  assembly { revertReason := add(revertReason, 68) } 
  revert(string(revertReason));
} 

• You would also probably want to add some safety by checking revertReason.length % 32 == 4 and check that the first 4 bytes are the Error(string) selector.

This would allow, for example, in ModuleManager
require(executeDelegateCall(to, data, gasleft()), "Could not finish initialization");

to bubble up the revert messages received during the executeDelegateCall instead of always overriding any of them with "Could not finish initialization", like so:

(bool success, bytes memory revertReason) = executeDelegateCall(to, data, gasleft())
if (!success)  {
  assembly { revertReason := add(revertReason, 68) } 
  revert(string(revertReason));
}

• You would also probably want to add some safety by checking revertReason.length % 32 == 4 and check that the first 4 bytes are the Error(string) selector.

[rmeissner]

So in general this is possible already (see https://github.com/gnosis/safe-contracts/blob/aa0f3345b609a816ace6c448960ddb852b8a1bbd/contracts/base/ModuleManager.sol#L86) major change would be to add this functionality of this method to other methods (e.g. the setup of the module manager).

Would like to understand better why the low level execute would need to be adjusted. Currently by not exposing the bytes by default we safe gas, since we only allocate the memory if needed.

[cag]

@leckylao @rmeissner So this issue affects the CPK in the linked issue above. I just wanted to add an additional note that solving this doesn't solve it for batch transactions with MultiSend, as MultiSend would also remove the revert data. Maybe it would be worth expanding the scope of this issue?

[rmeissner]

That is a different issue for me, as this issue here is not related to the safe librarie contracts but the core contracts.

[stefek99]

I second this functionality.

Example of a transaction that failed: https://rinkeby.etherscan.io/tx/0x999453d8cf8d30216cc9b16e86a83022b26f3dd0b05d43dc76b03d81611e5c65

Not obvious what actually failed.

require message = GOLD

[rmeissner]

Should be handled with #715