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

Add SafeERC20.forceApprove() #3851

Closed
wants to merge 1 commit into from

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Dec 2, 2022

Fixes LIB-705

PR Checklist

TBD

  • Tests
  • Documentation
  • Changelog entry

@k06a k06a changed the base branch from master to next-v5.0 December 2, 2022 17:38
@frangio
Copy link
Contributor

frangio commented Dec 2, 2022

Please provide more detail. If we offer this function, what should we recommend to developers? Should they always use this function when they need to call approve? Are there circumstances where they shouldn't use it? Are there risks they should consider?

@k06a
Copy link
Contributor Author

k06a commented Dec 2, 2022

@frangio if your contract is performing token approve and expecting to spend it later in the same transaction This forceApprove is the cheapest way to do so without any allowance pre-checks. In best scenario it will only perform approve(amount), in worst scenario it will make 3 calls: approve(amount), approve(0), approve(amount). So this methods do best effort to set token approve to desired amount.

@frangio
Copy link
Contributor

frangio commented Dec 3, 2022

My question is different: should we be recommending that everyone use forceApprove?

@k06a
Copy link
Contributor Author

k06a commented Dec 3, 2022

@frangio I think it depends on the situation. If developer is going to spend this approve in the same transaction, then yes, I would recommend to use it. If this approve should be spent by someone else in different transaction then I would suggest to use safe increase/decrease.

@frangio
Copy link
Contributor

frangio commented Dec 6, 2022

Great! That recommendation makes a lot of sense to me. We should include this in the docs.

@frangio frangio added this to the 4.9 milestone Feb 2, 2023
@Amxx Amxx mentioned this pull request Feb 22, 2023
3 tasks
@Amxx
Copy link
Collaborator

Amxx commented Feb 22, 2023

This targets next-5.0. Since we want it in 4.9 we need to target master.

Replacing this PR with #4067

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