openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

Add SafeERC20.forceApprove()

Open k06a opened this issue 3 years ago • 5 comments

PR Checklist

TBD

  • [ ] Tests
  • [ ] Documentation
  • [ ] Changelog entry

k06a avatar Dec 02 '22 17:12 k06a

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?

frangio avatar Dec 02 '22 21:12 frangio

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

k06a avatar Dec 02 '22 22:12 k06a

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

frangio avatar Dec 03 '22 19:12 frangio

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

k06a avatar Dec 03 '22 20:12 k06a

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

frangio avatar Dec 06 '22 18:12 frangio

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

Replacing this PR with #4067

Amxx avatar Feb 22 '23 16:02 Amxx