Skip to content

Add SafeERC20.forceApprove()#3851

Closed
k06a wants to merge 1 commit into
OpenZeppelin:next-v5.0from
1inch:feature/force-approve
Closed

Add SafeERC20.forceApprove()#3851
k06a wants to merge 1 commit into
OpenZeppelin:next-v5.0from
1inch:feature/force-approve

Conversation

@k06a

@k06a k06a commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

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

frangio commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

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

k06a commented Dec 2, 2022

Copy link
Copy Markdown
Contributor Author

@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

frangio commented Dec 3, 2022

Copy link
Copy Markdown
Contributor

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

@k06a

k06a commented Dec 3, 2022

Copy link
Copy Markdown
Contributor Author

@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

frangio commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

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

Amxx commented Feb 22, 2023

Copy link
Copy Markdown
Collaborator

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