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

Discussion: Either revert or return success, not both #6

Open
deluca-mike opened this issue Sep 3, 2021 · 1 comment
Open

Discussion: Either revert or return success, not both #6

deluca-mike opened this issue Sep 3, 2021 · 1 comment

Comments

@deluca-mike
Copy link
Contributor

deluca-mike commented Sep 3, 2021

There is a bit of an anti-pattern going on in the space where success/ok return values aren't actually used, which can waste gas, and further pushes the need for "safe" helpers to handle this issue.

I believe we should pick one and stick with it: either revert on a failure, and return nothing, or don't revert and return the success status.

Pros for reverting (which are de facto Cons for returning success):

  • EOA can call the functions and immediately see the if it failed or not, without needing to check beyond the status in the tx receipt, and wallets can detect failures pre-flight. Also, if an EOA is using a primitive wallet that has no pre-flight checks, the revert string should show up on popular explorers like etherscan.
  • No need for callers to wrap the calls in requires, in their code, however, everyone is already wrapping their erc20 calls in "safe" helper functions...
  • Not gas wasted on some static return true; and return type, which is meaningless.

Cons for reverting (which are de facto Pros for returning success):

  • Revert strings are baked into the erc20 contract, wasting bytecode space and runtime gas (since they are often ignored and replaced with the ones thrown by the "safe" helpers, for internal calls. Further, more often than not, these revert strings would be in a different "domain" and style.
  • No easy way to fork logic based on if a transfer/approval fails or not.
@deluca-mike deluca-mike changed the title Discussion: Either Revert or Return Success, not both Discussion: Either revert or return success, not both Sep 3, 2021
@JGcarv
Copy link
Contributor

JGcarv commented Sep 3, 2021

I agree with the case that we are using an anti-pattern, which is far from ideal.

However, this is something that comes from ERC20 itself and I don't think we should change. Composability is a big part of the ecosystem, so my take is that the token should do what most people expect it to do. I'm afraid that by making things right we might cause more problems than keeping the wrong like everybody else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants