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

increaseApproval vs increaseAllowance #120

Closed
wjmelements opened this issue Dec 2, 2019 · 7 comments · Fixed by #123
Closed

increaseApproval vs increaseAllowance #120

wjmelements opened this issue Dec 2, 2019 · 7 comments · Fixed by #123

Comments

@wjmelements
Copy link
Contributor

The specification seems nonstandard. EY prefers increaseAllowance though increaseApproval has been preferred in other locations.

@wjmelements
Copy link
Contributor Author

We will likely patch this in the next batch of upgrades. If you have any comments on the topic please post them here before the end of December 2019.

@wjmelements
Copy link
Contributor Author

Exactly "increaseAllowance" has 1,420 google results while exactly "increaseApproval" has 5,870 google results.

@wjmelements
Copy link
Contributor Author

The convention of increaseAllowance seems to have begun with this pull request, though neither the pull request nor its linked issue discuss the change.

Perhaps @frangio can offer some insight here.

@frangio
Copy link

frangio commented Dec 3, 2019

Hi @wjmelements, can you give me some context on what is the issue here?

@wjmelements
Copy link
Contributor Author

@frangio The community had coordinated on increaseApproval, but openzeppelin changed it to increaseAllowance in your pull request. Do you recall why? There is now inconsistency in this specification, and EY is preferring the less-common increaseAllowance.

@frangio
Copy link

frangio commented Dec 16, 2019

We felt that it was a more consistent name with the ERC20 spec, which uses the term "allowance" rather than "approval". We also felt it was okay to make this breaking change (which we did in the 2.0 major release) because these functions are not part of the ERC20 standard. We never recommended their usage when interacting with generic ERC20 tokens, since there's no guarantee they will be available, and this change made sense from that perspective.

@wjmelements
Copy link
Contributor Author

Okay, we will likely change to increaseAllowance and decreaseAllowance in a future update then.

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 a pull request may close this issue.

2 participants