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.

Sign up for GitHub

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

Jump to bottom

⚡️ Optimise set_minter Functions in ERC20, ERC721, and ERC1155 #154

Merged
merged 3 commits into from
Aug 6, 2023
Merged

⚡️ Optimise set_minter Functions in ERC20, ERC721, and ERC1155 #154

merged 3 commits into from
Aug 6, 2023

Conversation

El-Ku
Copy link
Contributor

@El-Ku El-Ku commented Aug 5, 2023 •
edited by pcaversaccio
Loading

Summary

Since it is ensured in the previous step self._check_owner() that msg.sender is the owner, we can replace self.owner with msg.sender in the following check to save one SLOAD:

- assert minter != self.owner, "AccessControl: minter is owner address"
+ assert minter != msg.sender, "AccessControl: minter is owner address"

This affects the contracts ERC20, ERC721, and ERC1155.

@El-Ku
Update ERC20.vy
91375bc 
msg.sender is owner. saving gas. 

Signed-off-by: EK <46983244+El-Ku@users.noreply.github.com>
@pcaversaccio pcaversaccio changed the title Update ERC20.vy ⚡️ Gas Optimise set_minter Functions in ERC20, ERC721, and ERC1155 Aug 5, 2023
@pcaversaccio pcaversaccio self-requested a review August 5, 2023 10:01
@pcaversaccio pcaversaccio self-assigned this Aug 5, 2023
@pcaversaccio pcaversaccio added the optimisation ⚡️ Code optimisations (e.g. gas improvements) label Aug 5, 2023
@pcaversaccio pcaversaccio added this to the 0.0.3 milestone Aug 5, 2023
@pcaversaccio
♻ Optimise ERC721 and ERC1155
b86b52a 
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
pcaversaccio
pcaversaccio previously approved these changes Aug 5, 2023
View reviewed changes
Copy link
Owner

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this. I amended the PR description for clarity. I also amended the ERC721 and ERC1155 contracts accordingly. You see the gas savings in the gas snapshots of the tests:
image
image
image

I already approve this PR, but will make a final review tomorrow.

@pcaversaccio pcaversaccio changed the title ⚡️ Gas Optimise set_minter Functions in ERC20, ERC721, and ERC1155 ⚡️ Optimise set_minter Functions in ERC20, ERC721, and ERC1155 Aug 5, 2023
@pcaversaccio
♻ Add CHANGELOG
1acfaf5 
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@pcaversaccio pcaversaccio self-requested a review August 5, 2023 11:00
pcaversaccio
pcaversaccio approved these changes Aug 5, 2023
View reviewed changes
Copy link
Owner

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added CHANGELOG entry.

@El-Ku
Copy link
Contributor Author

El-Ku commented Aug 6, 2023

yes thanks. looks much cleaner now indeed.

@pcaversaccio pcaversaccio merged commit 014143b into pcaversaccio:main Aug 6, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcaversaccio pcaversaccio pcaversaccio approved these changes

Assignees

@pcaversaccio pcaversaccio

Labels
optimisation ⚡️ Code optimisations (e.g. gas improvements)
Projects
None yet
Milestone
0.0.3
Development

Successfully merging this pull request may close these issues.
2 participants
@El-Ku @pcaversaccio