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

Add _mint(to, quantity) function #235

Merged
merged 3 commits into from
Apr 16, 2022
Merged

Add _mint(to, quantity) function #235

merged 3 commits into from
Apr 16, 2022

Conversation

Vectorized
Copy link
Collaborator

@Vectorized Vectorized commented Apr 12, 2022 •
edited

Most people use _mint(to, quantity, "", false) when they do not need safe minting.

Sadly, the compiler will still produce the bytecode needed for safe minting.

<Inserts optimizoor meme here>

This PR adds a _mint(to, quantity) function.

It saves 100 gas per call, and 42495 gas during deployment (about 200 bytes), which is about 6% for a minimal contract.

The code is manually inlined directly. Simply calling _mint(to, quantity, "", false) inside actually leads to increased gas costs (50 gas function call overhead) with no reduction in bytecode size.

⛽ Gas Reports

Before 
·---------------------------------------|---------------------------|-------------|
|         Solc version: 0.8.11          ·  Optimizer enabled: true  ·  Runs: 800  ·
········································|···························|·············|
|  Methods                                                                         
····························|···········|·············|·············|·············|
|  Contract                 ·  Method   ·  Min        ·  Max        ·  Avg        ·
····························|···········|·············|·············|·············|
|  ERC721AGasReporterMock2  ·  mintOne  ·      56462  ·      90662  ·      57146  ·
····························|···········|·············|·············|·············|
|  ERC721AGasReporterMock2  ·  mintTen  ·      74116  ·     108316  ·      74800  ·
····························|···········|·············|·············|·············|
|  Deployments                          ·                                         ·
········································|·············|·············|·············|
|  ERC721AGasReporterMock2              ·          -  ·          -  ·    1128702  ·
·---------------------------------------|-------------|-------------|-------------|
After 
·---------------------------------------|---------------------------|-------------|
|         Solc version: 0.8.11          ·  Optimizer enabled: true  ·  Runs: 800  ·
········································|···························|·············|
|  Methods                                                                         
····························|···········|·············|·············|·············|
|  Contract                 ·  Method   ·  Min        ·  Max        ·  Avg        ·
····························|···········|·············|·············|·············|
|  ERC721AGasReporterMock2  ·  mintOne  ·      56361  ·      90561  ·      57045  ·
····························|···········|·············|·············|·············|
|  ERC721AGasReporterMock2  ·  mintTen  ·      74015  ·     108215  ·      74699  ·
····························|···········|·············|·············|·············|
|  Deployments                          ·                                         ·
········································|·············|·············|·············|
|  ERC721AGasReporterMock2              ·          -  ·          -  ·    1086207  ·
·---------------------------------------|-------------|-------------|-------------|

@Vectorized Vectorized linked an issue Apr 12, 2022 that may be closed by this pull request
Adding a slimmed down mint function #233
Closed
ahbanavi
ahbanavi approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@ahbanavi ahbanavi left a comment

Choose a reason for hiding this comment

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

Wow, this makes my life a lot easier.
I was frustrated doing _mint(addr, quantity, '', false) instead of a simple _mint(addr, quantity) throughout my contract.

Apart from being more user-friendly, it's more optimized too (if an implemented contract doesn't use both _mint and _safeMint).

The downside, lower maintainability, is totally worth it. "pretends to be a maintainer"

@Vectorized Vectorized requested a review from cygaar April 13, 2022 14:44
@cygaar cygaar changed the title Slim _mint Add _mint(to, quantity) function Apr 16, 2022
cygaar
cygaar reviewed Apr 16, 2022
View reviewed changes
contracts/ERC721A.sol
/**
* @dev Equivalent to _mint(to, quantity, '', false).
*/
function _mint(address to, uint256 quantity) internal {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just rename the existing mint function to _safeMint and remove the safe parameter? This will be backwards incompatible, but will make the code cleaner since I don't see ppl calling _mint(to, qty,'', false) going forward

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree to remove the safe parameter and rename _safeMint.
For backward compatibility, we could add back _mint(to, qty, data, safe) and then call _mint and _safeMint based on the safe parameter, but it cost maintainability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’m just worried about the customers calling the ERC721A helpdesk if we break backwards compatibility. Lemme think for a while.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well we've broken it a couple times already. I'm ok cutting a v4 of ERC721A for a change like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Outside rn, brb in a few hours probably

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Code is neater, about 50 gas saved. Let’s go.

@Vectorized Vectorized merged commit b17a1c4 into chiru-labs:main Apr 16, 2022
@Vectorized Vectorized mentioned this pull request Apr 29, 2022
Closed
@Vectorized Vectorized deleted the feature/slimMint branch May 17, 2022 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahbanavi ahbanavi ahbanavi approved these changes

@cygaar cygaar cygaar left review comments

@tunnckoCore tunnckoCore tunnckoCore approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding a slimmed down mint function
4 participants
@Vectorized @tunnckoCore @ahbanavi @cygaar