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

decrease approval condition fix (greater than and equal to) #1063

Merged
merged 5 commits into from
Jul 20, 2018

Conversation

zaryabbkh
Copy link
Contributor

🚀 Description

The decreaseApproval function in StandardToken.sol has a condition that if the subtracted amount is greater than the allowed amount, simply make the allowance 0, otherwise, it invokes a subtraction function for reducing the approval amount:

 if (_subtractedValue > oldValue) {
      allowed[msg.sender][_spender] = 0;
    } else {
      allowed[msg.sender][_spender] = oldValue.sub(_subtractedValue);
    }

If the subtracted amount is exactly equal to the allowed tokens, this will execute both the first condition, and will then default to the else where the subtract function is called which unnecessarily increases the gas amount of such a transaction. If we modify the condition to be (_subtractedValue >= oldValue), the allowance will be reset to 0. This will only increase 3 units of gas per transaction but it will save 88 units of gas when subtractedValue is equal to the oldValue.

Before update in statement the transaction cost is 16278 to decrease the exactly same allowed amount
before

After the update the transaction cost is 16190
after

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines
  • ✅ I've added tests where applicable to test my new functionality.
  • 📖 I've made sure that my contracts are well-documented.
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

@nventuro
Copy link
Contributor

Cool, thanks @zaryab19aug! I modified the test cases a bit to make them more consistent with the rest of the suite, hopefully a more comprehensive guide will be added soon.

@nventuro nventuro merged commit ebd4b5e into OpenZeppelin:master Jul 20, 2018
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.

2 participants