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.tryApprove implementation #3046

Closed

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Dec 22, 2021

Hi, consider adding this useful function implementation.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Dec 22, 2021

Can you share more detail about in what contexts this function is useful?

@k06a
Copy link
Contributor Author

k06a commented Dec 22, 2021

@frangio when I need my contract to set specific approve before calling receiving contract which will later call transferFrom. What I was usually doing is safeApprove(0) + safeApprove(amount). What I need is to “set approve” not to be reverted by amount/allowance check.

@k06a
Copy link
Contributor Author

k06a commented Dec 22, 2021

@k06a
Copy link
Contributor Author

k06a commented Dec 22, 2021

Maybe method could be name forceApprove or sudoApprove or ensureApprove…

@Amxx
Copy link
Collaborator

Amxx commented Dec 23, 2021

@k06a Setting approval to 0, and then to the actual value should only required for EOA which cannot bundle transactions. Its basically done to avoid the new approval being frontrun by someone that would use the old approval and still get the new one.

In that case it is recommended to:

  • set approval to 0
  • check that any previous approval is NOT used while approval is being mined.
  • once approval to 0 is mined, submit the new approval.

If you are writing a smart contract that, as part of its internal logic, does approval + call to third party that triggers a transfer from ... then you should not need this double approval.

@k06a
Copy link
Contributor Author

k06a commented Dec 23, 2021

@Amxx I agree but I can’t call raw approve because implementation can not return bool. Calling safeApprove twice have double allowance call overhead.

@frangio
Copy link
Contributor

frangio commented Dec 27, 2021

So the purpose of this function is not to implement the double approve as a security measure, but as a workaround for tokens that have this requirement. And it was implemented in this way to avoid the double overhead for most cases? "Force approve" does sound like a better name if we decide to merge this.

Can you share a few examples of tokens that have this behavior in approve?

@k06a
Copy link
Contributor Author

k06a commented Dec 27, 2021

@frangio that’s true and I agree about naming. Will provide examples.

@k06a
Copy link
Contributor Author

k06a commented Dec 29, 2021

USDT token is the most famous for such behavior. Here are top contracts by usage:

contract_address count
0xdac17f958d2ee523a2206206994597c13d831ec7 134956902
0x41ab1b6fcbb2fa9dced81acbdec13ea6315f2bf2 193078
0x7654915a1b82d6d2d0afc37c52af556ea8983c7e 30850
0x9f195617fa8fbad9540c5d113a99a0a0172aaedc 21301
0xaff84e86d72edb971341a6a66eb2da209446fa14 20844
0x0778cc2e8bbad3d483e82371606d100cc8604522 20252
0x508325285114821151a18e148f4299ea09a9ca05 7015
0x571a74c42f99d46a55cc85cabac5863662775a16 5682
0x758f4f55d7d00c3dac1155069351526775206f2a 5275
0x5a9c8c6406d341a16aa3010108026f45fc372168 2121
0x0557e0d15aec0b9026dd17aa874fdf7d182a2ceb 2063
0xe389b6db10b3d5c67234cdac2bb08870a3174a05 1906
0x26607f9bf9d62a37b0c78e1d3719fcd1fa32bef9 1773
0xa0b8efce9ee02a4f62af27d5eea63040fad35dec 1186
0x36b68e098d02a97b70fd1b71df1e805ec3d4e4d7 1039
0x0568025c55c21bda4bc488f3107ebfc8b3d3ef2d 906
0x0ab39ac604f992aaec3c36de337c3cd3917a7d26 577
0x46cc7ec70746f4cbd56ce5fa9bb7d648398eaa5c 535
0xa11df03891bb759e60dca01799883d1a8bb8b53f 419
0x0f59dd60695bf8431ad4344e37244d80e1ec296d 311
0xe76900a5f84c08e21ac94f3bcdb1bc90568bf8fc 243
0xd4ba9345a45eb5937f3d13d004d00b3483bda0ed 216
0x59ef697894da6f943ce8d534b98f89cdbfbc763d 215
0x04ddde8d3f302529c08c3c87b946628a7a05948a 147
0xd49bc192fe139f4bd12d769540275856ec381968 129

@frangio
Copy link
Contributor

frangio commented Feb 24, 2023

Superceded by forceApprove in #4067.

@frangio frangio closed this Feb 24, 2023
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.

4 participants