Fix setExpenditureValues with metatransactions

[area]

Metatransactions do not work as-is with, specifically, setExpenditureValues.

This is because the assumption we made when implementing metatransactions to accommodate our self-calling functions is not true for one particular call to setExpenditurePayouts we make after introducing setExpenditureValues. Specifically, the assumption that if one of our contracts is calling itself, there are no arguments in the function call (represented by the index >= 52 here).

This PR makes one attempt to fix it, but there could be others I've not considered - I am very much open to other suggestions if there is a better one we can arrive at. The solution is similar to how we treat metatransactions in multicalls, however.

As with everything that touches metatransactions, a particularly careful review would be appropriate, as the failure case for metatransactions is catastrophic.

[kronosapiens]

@area can you clarify what is meant by "there are no arguments in the function call"? Do you mean that there are no arguments after the metatransaction flag? Or no arguments entirely?

[kronosapiens]

Also, in a multicall world, could we not dispense with setExpenditureValues entirely and simply use multicall to perform multiple expenditure edits in one transaction?

[area]

@area can you clarify what is meant by "there are no arguments in the function call"? Do you mean that there are no arguments after the metatransaction flag? Or no arguments entirely?

No arguments to the function that the contract is calling on itself. In the original implementation, upgrade() was the function we were thinking about that the contract would call on itself. If msg.data is less than 52, the call cannot be a metatransaction (because a metatransaction will have METATRANSACTION_FLAG and the caller's address appended, for a total of 52 bytes), so for upgrade we were able to bypass this logic with the check against msg.data's length.

Unfortunately, we now have a scenario where msg.data might be longer than 52 bytes, msg.sender might be self, and it might or might not be a metatransaction.

Also, in a multicall world, could we not dispense with setExpenditureValues entirely and simply use multicall to perform multiple expenditure edits in one transaction?

This is true, but not how things have been implemented on the front-end.

[kronosapiens]

Looks good to me. I don't see significant risk to the metatransaction functionality, as most of the shuffling is taking place in ColonyExpenditure. The refactor in MetaTransactionMsgSender didn't have any issues I could see.

I would like to deprecate setExpenditureValues in the future and shift to multicall for this sort of behavior, though, as ultimately this is sort of a hack built on top of a previous hack.

[area]

Deliberately haven't squashed the last commit to serve as a reminder to us how easily critical errors can be made, and how innocuous they can look.

[area]

Also, in a multicall world, could we not dispense with setExpenditureValues entirely and simply use multicall to perform multiple expenditure edits in one transaction?

Thanks to @jakubcolony's efforts, this is now how things have been implemented on the frontend, so maybe this PR should now be replaced by one to remove setExpenditureValues?

[kronosapiens]

Wonderful! I wouldn't mind hanging on to the new constant though, especially after risking death to introduce it.

[area]

That's fair. Once it's confirmed that everything works via multicall and it's successfully deployed, I'll close this and open a new one.

[area]

Superseded by #1259