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

Improvement/holo 603 gas optimization combine address mappings #82

Draft
wants to merge 7 commits into
base: testnet
Choose a base branch
from

Conversation

alexanderattar
Copy link
Contributor

@alexanderattar alexanderattar commented Nov 11, 2022

Describe Changes

G-01 Multiple address mappings can be combined into a single mapping of an address to a struct
Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

  • Updated - HolographERC20.sol
  • Updated - HolographERC721.sol
  • Updated - HolographOperator.sol

Checklist before requesting a review

  • I have performed a self-review of my code
  • Code styles have been enforced
  • All Hardhat tests are passing

@alexanderattar alexanderattar marked this pull request as ready for review November 11, 2022 16:44
@alexanderattar
Copy link
Contributor Author

@ACC01ADE I realize the gas savings from this might not be much as the values are not used in the same function and don't fit into the same slot, but this was my way of picking something easy up to get more comfortable updating the contracts before getting into more complicated stuff. If we feel that the potential gas savings don't justify the introduction of the new struct types / ergonomic changes of accessing the values, it's fine to not integrate these changes. lmk what you think

@alexanderattar
Copy link
Contributor Author

alexanderattar commented Nov 11, 2022

You can see some calls that have some gas savings in the screenshots below

@alexanderattar

This comment was marked as outdated.

@alexanderattar

This comment was marked as outdated.

@alexanderattar
Copy link
Contributor Author

HolographERC20 (Before)
image

HolographERC20 (After)
image

HolographERC721 (Before)
image

HolographERC721 (After)
image

@alexanderattar
Copy link
Contributor Author

HolographOperator (Before)
image

HolographOperator (After)
image

@sogoiii sogoiii marked this pull request as draft February 5, 2024 21:19
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.

1 participant