Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit updated Approval in transferFrom #707

Closed
frangio opened this issue Jan 25, 2018 · 6 comments

Comments

@frangio
Copy link
Member

commented Jan 25, 2018

@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.

@frangio frangio added the discussion label Jan 25, 2018

@shrugs

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

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.

@frangio frangio self-assigned this Apr 9, 2018

@frangio frangio added the contracts label Aug 12, 2018

@frangio frangio added this to the v2.0 milestone Aug 12, 2018

@nventuro

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

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.

ardagokcek added a commit to ardagokcek/openzeppelin-solidity that referenced this issue Sep 2, 2018
Adding TransferFrom Event
Check Issue OpenZeppelin#707 these changes makes the idea with adding the event in IERC20.sol 
Event: 
event TransferFrom(
      address indexed executor,
      address indexed from,
      address indexed to,
      uint256 value);

@frangio frangio modified the milestones: v2.0, v2.1 Sep 4, 2018

@frangio frangio modified the milestones: Test helpers, v2.1 Nov 20, 2018

@frangio frangio changed the title Evaluate possibility of emitting Approval in transferFrom Emit updated Approval in transferFrom Nov 27, 2018

@frangio frangio assigned nventuro and unassigned frangio Nov 27, 2018

@nventuro

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.