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 Approval event on allowance change #65

Merged
merged 2 commits into from
Apr 17, 2020

Conversation

k06a
Copy link

@k06a k06a commented Apr 15, 2020

More details here: makerdao/dss#76

@MicahZoltu
Copy link

MicahZoltu commented Apr 16, 2020

I mildly disagree with the reasoning provided in the linked discussions. I do think there is value in emitting an event for transferFrom that is unique from transfer. I don't agree that approval is the right such event. ERC20 is terrible in many ways, but I don't think the solution to that is for people to silently extend ERC20 without any attempt at creating an actual better standard (like a set of events that provides all of the information one may want, rather than transfer/approval events only). I have lobbied in the past for people using ERC777, but if that is truly hated (which I believe it is by the Uniswap team) then some other standard would be better to follow rather than ad-hoc compatibility with a random subset of tokens in the space.

That being said, since it seems the dapp space is split on this topic, you are forced to pick a side and following OZ/Maker isn't a bad choice.

@k06a
Copy link
Author

k06a commented Apr 16, 2020

@MicahZoltu I agree, any new ERC20 implementors should pick a side or at least be notified of existing differences in implementations.

@moodysalem moodysalem changed the base branch from master to pr-improvements April 17, 2020 20:09
@moodysalem moodysalem merged commit 7509265 into Uniswap:pr-improvements Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants