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

Change quantities to uint24 #192

Merged
merged 1 commit into from
Sep 12, 2022
Merged

Change quantities to uint24 #192

merged 1 commit into from
Sep 12, 2022

Conversation

vigneshka
Copy link
Contributor

@vigneshka vigneshka commented Sep 12, 2022

2^24 is 16,777,216 which is a sufficient quantity upper bound
Changing these fields from uint32 -> uint24 will aid in followup change to pack data into BaseMinter aux storage

Fixes: PRO-388

@changeset-bot
Copy link

changeset-bot bot commented Sep 12, 2022

⚠️ No Changeset found

Latest commit: f52b667

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

@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #192 (f52b667) into master (b580669) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #192   +/-   ##
=======================================
  Coverage   90.36%   90.36%           
=======================================
  Files          10       10           
  Lines         301      301           
  Branches       44       44           
=======================================
  Hits          272      272           
  Misses         24       24           
  Partials        5        5           
Impacted Files Coverage Δ
contracts/modules/FixedPriceSignatureMinter.sol 100.00% <ø> (ø)
contracts/modules/MerkleDropMinter.sol 100.00% <ø> (ø)
contracts/modules/BaseMinter.sol 96.49% <100.00%> (ø)
contracts/modules/RangeEditionMinter.sol 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@Vectorized Vectorized left a comment

Choose a reason for hiding this comment

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

Oh. Ok i see

@linear
Copy link

linear bot commented Sep 12, 2022

PRO-387 Make use of MaseMinter aux storage

BaseMinter has 168 bits of available storage in it’s struct. By making this space available for auxilary storage, fields in the minters can be packed into it to reduce a storage slot being used. This will save ~20k gas for each mint schedule that is configured and ~5k for each mint operation

If exposed as aux storage:
Then merkledrop minter can change maxMintable, maxMintablePerAccount, totalMinted to uint24 (more than sufficient upper bound) and pack those + price (uint96) to fit in the aux. Reduce 1 storage write in init and 1 read in mint.
Same with the PublicSaleMinter implementation, price and maxMintablePerAccount can be packed into BaseData aux to reduce 1 slot.
SignatureMinter can also reduce a slot and pack maxMintable and totalMinted.

To serialize and deserialize these 168 bits into fields we care about in a clean way, can we use user defined value types? So have a type like type MerkleDropAux is uint168 and then define library methods to parse and pack fields into the type

See: ethereum/solidity#11691 (comment)

@linear
Copy link

linear bot commented Sep 12, 2022

PRO-388 Change quantity fields to uint24

2^24 is 16,777,216 which is a sufficient quantity upper bound
Changing these fields from uint32 -> uint24 will aid in followup change to pack data into BaseMinter aux storage

Copy link
Contributor

@gigamesh gigamesh left a comment

Choose a reason for hiding this comment

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

I think the 16,777,216 limit could become a problem in the future, like if Taylor Swift wanted to airdrop an edition to all her fans.

However by that point most activity will be on layer 2 and we'll have new contracts, so optimizing for current users is probably the right move. 👍

@vigneshka vigneshka merged commit 510289c into master Sep 12, 2022
@vigneshka vigneshka deleted the vigneshka/quantity-uint24 branch September 12, 2022 17:43
vigneshka added a commit that referenced this pull request Sep 13, 2022
Vectorized pushed a commit that referenced this pull request Sep 13, 2022
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