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

Use unchecked arithmetic in "_transfer" and "_mint" #3513

Merged
merged 8 commits into from Jul 1, 2022
Merged

Use unchecked arithmetic in "_transfer" and "_mint" #3513

merged 8 commits into from Jul 1, 2022

Conversation

PaulRBerg
Copy link
Contributor

Implements #3512.

There are no tests to write for this change.

@Amxx
Copy link
Collaborator

Amxx commented Jun 29, 2022

Thank you @PaulRBerg for the PR.

Can you please make sure the linter pass? You can run npm run lint:fix to automatically fix it.

@Amxx
Copy link
Collaborator

Amxx commented Jun 29, 2022

Also, It would be great to run the ERC20.test.js with ENABLE_GAS_REPORT=true to see the difference.

@PaulRBerg
Copy link
Contributor Author

Sorry for that, I fixed the linting now. Here's the gas comparison below. Looks like both transfer and transferFrom have become cheaper by 136 gas.

Before

before

After

after

@Amxx Amxx added this to the 4.8 milestone Jun 30, 2022
Amxx
Amxx previously approved these changes Jun 30, 2022
@Amxx
Copy link
Collaborator

Amxx commented Jun 30, 2022

Could you also use a small changelog entry please?

@PaulRBerg
Copy link
Contributor Author

Done, ser.

@frangio
Copy link
Contributor

frangio commented Jul 1, 2022

I added comments explaining why we can use unchecked for these operations.

@frangio frangio requested a review from Amxx July 1, 2022 19:12
@Amxx Amxx merged commit 5fbf494 into OpenZeppelin:master Jul 1, 2022
@PaulRBerg PaulRBerg deleted the unchecked-arithmetic-erc20 branch July 2, 2022 07:39
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
mmsqe added a commit to crypto-org-chain/cronos that referenced this pull request Apr 18, 2023
yihuang pushed a commit to crypto-org-chain/cronos that referenced this pull request Apr 18, 2023
…s/contracts (#979)

* Bump @openzeppelin/contracts in /integration_tests/contracts

Bumps [@openzeppelin/contracts](https://github.com/OpenZeppelin/openzeppelin-contracts) from 4.7.3 to 4.8.3.
- [Release notes](https://github.com/OpenZeppelin/openzeppelin-contracts/releases)
- [Changelog](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.3/CHANGELOG.md)
- [Commits](OpenZeppelin/openzeppelin-contracts@v4.7.3...v4.8.3)

---
updated-dependencies:
- dependency-name: "@openzeppelin/contracts"
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

* adjust to 136 cheaper gas

for more info, see OpenZeppelin/openzeppelin-contracts#3513

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: mmsqe <mavis@crypto.com>
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.

None yet

3 participants