Openzeppelin-contracts: Emit updated Approval in transferFrom

[frangio]

@martriay has noticed that Approval events are not sufficient to reconstruct the allowance status of an address, because allowance is silently modified (reduced) in transferFrom. In fact, even all of the ERC20 events together don't suffice either, because of the long-standing problem that transferFrom transfers are logged like simple Transfer events with no information on who the spender was.

I want us to evaluate the possibility of emitting an Approval event in the transferFrom function to signal the reduced allowance, if it is ERC20 compliant. I also wonder if this can solve the problem of transferFrom being a "silent" operation, by emitting a recognizable sequence of Transfer and Approval events, but it shouldn't allow the token holder to simulate a transferFrom.

[shrugs]

I'd agree that this would be valuable—in the future I expect quite a few usecases will pop up where people would like to watch for events in order to update off-chain state. If it's not possible to recover the full state using the events, long-polling the contract becomes necessary, which isn't really ideal.

[nventuro]

From my interpretation of the spec, this would be compliant, since it doesn't specify that Approval must only be emitted on approve.

Additionally, a transferFrom call followed by an approve call, setting the allowance to the current allowance value, would emit the exact same two events while not modifying the state in any way, making this a completely valid and useful change IMO.

[frangio]

Here's an argument against, to make for a balanced discussion.

The ERC20 implementations around probably all have the problem that events are not enough to reconstruct allowance. Because of this, all applications will have to work around this by either polling or some other complex mechanism. What value would we be adding, then?

If an application is written specifically to interact with an OpenZeppelin ERC20 instance, and thus can safely rely on events only, it may silently become incorrect when plugged into a non-OpenZeppelin instance. I suppose we could try to consider the implications and base our decision on that, but it feels risky.

[shrugs]

hmm good points. this problems exists across a lot of domains, though; when the world moves forward, you have to decide which world you support; the new, the old, or both.

In the case of early ERCs, it's impossible to know which world you're in because we don't have migrations or 165 interfaces to depend on, so it gets really hazy.

I'm in favor of adding the event here and telling people that it's a reverse-breaking change (is there a better word for that?)

But we should also consider adding 165 interfaces to erc20 as well, for similar reasons.

[nventuro]

I agree with @shrugs, we should push the industry forward, not stagnate because the standard isn't as good as it could be. Good design doesn't happen in a vacuum, these kind of improvements may drive new standards.

More pragmatically, an application may not need to interact with multiple tokens, in which case using our enhanced one may make their life easier.

[nventuro]

I think the arguments exposed are good enough to warrant this being a thing, will open a PR soon.