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

Fix excessive gas usage when at or near cap. #4969

Conversation

BillSchumacher
Copy link

Fixes #4967

Saves gas by completely avoiding _update if the cap will be exceeded.
image

The "After" test is the current usage and the non-After OverCap test is the PR's usage.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Mar 21, 2024

⚠️ No Changeset found

Latest commit: 94207e6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@BillSchumacher BillSchumacher changed the title See https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4967 Fix excessive gas usage when at or near cap. Mar 21, 2024
@ernestognw
Copy link
Member

Sorry, I'll close. Let me be clear about a couple of things:

  • ERC20 _update customization is not practical. #4967 is a non-issue and seems unrelated to this PR aside from the "gas savings" (which are none).
  • There's no point in optimizing the gas consumption whenever the function will revert. If the reasoning is that a user will pay less if the transaction fails, that's completely wrong! Any wallet will prevent the user from executing an operation if it'll fail. This is easy to catch with a gas estimation that every wallet does.
  • The order of operations is important. By accepting this change we need to make sure that every possible side effect from the order of execution keeps the invariants (possibly yes but there's still no point in these changes). This is why we book an audit before a release.
  • A screenshot of your tests is no proof of any gas improvement, even if it was true. We can't merge without the code and a proper analysis of what you're doing, including compiler settings.

@ernestognw ernestognw closed this Mar 21, 2024
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.

ERC20 _update customization is not practical.
2 participants