Reorder state changes and event emission for consistency

[Amxx]

No description provided.

[frangio]

On second thought about this, I think we should emit events after state changes... It's what we do everywhere else already.

openzeppelin-contracts/contracts/token/ERC20/ERC20.sol

Lines 220 to 222 in 23869e5

	_balances[recipient] += amount;
	emit Transfer(sender, recipient, amount);

[Amxx]

In transferOwnership (and other places where the event contains the "old value") we emit the event before doing the state change. This is so the event can read the old state from storage without having to assign a temporary variable.

This is why I followed this logic ... but its true that in tokens we emit events after ...

[Amxx]

I have the feeling that our logic is "Emit the event after changing the state, unless the event needs knowledge of the previous state"

Maybe we should keep doing that.

[frangio]

Sorry did that last edit before seeing these comments thinking we had agreed.

I get that it's technically consistent if we establish this convention you just mentioned, but it's not immediately apparent that the convention is "do this unless we need to do this other thing", and I don't see an issue in defining a local variable.

[Amxx]

The local variable might cause gas cost (the stack might need to be reordered to push it back to the top when emitting the event). It might even cause stack too deep errors :/