Optimize forceApprove for 0 value calls

[jubeira]

In certain cases, we might want to:

• forceApprove a certain amount for a given spender.
• Call an external function that makes use of that approval, but potentially less: e.g. a swap given out, where the amount of tokens to be pulled by the call is not known in advance.
• Reset the allowance to 0, eliminating any leftover that was not consumed by the call from the previous step.

With the current codebase, if _callOptionalReturnBool returns false, the workflow described above will make 4 calls to token.approve; the last 2 of them will use value = 0. This PR skips the last one, which is unnecessary in this case.

PR Checklist

• Tests
• N/A Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: c3c4388

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ernestognw]

Hi @jubeira, thanks for the PR. The optimization makes sense to avoid duplicated 0 approvals. I have a couple of things to consider before merging:

• The change is slightly breaking (i.e. a extra event won't be emitted), which may disrupt indexers.
• The check adds a bit of extra gas units to any approval that's not 0

I'm not too worried about the event as long as it's documented in the CHANGELOG. On the other side, the extra gas units for the 0 case is what's bugging me.

The SafeERC20 is intended for interacting with arbitrary ERC20 tokens safely, where forceApprove is a special case when the token you interact with is potentially USDT-like and requires setting the approval to 0 before something else. It seems like you know for sure that the last value will be 0, so I'm wondering why it's not enough to just call approve with 0 value?

[jubeira]

Thanks for your comments.

It seems like you know for sure that the last value will be 0, so I'm wondering why it's not enough to just call approve with 0 value?

The problem is that USDT has both issues:

• It requires to start from 0 to change to anything else.
• It doesn't return a boolean.

Calling usdt.approve(spender, 0) will fail using the standard interface.

An example for this sequence can be found here. As you point out, forceApprove will always work no matter what. For non-USDT-like tokens it will make a single approve call (2 in total for the sequence); for USDT-like tokens it will make 2 approve calls (4 in total for the sequence). This change would drop the external calls to 3 for USDT, and keep non-USDT at 2.

It is true that you add an extra if for non-zero approvals, but the gas spent there is arguably less than an external call that will also perform at least one (warm) storage access.